* [PATCH 0/2] i.MX93 ADC calibration settings
@ 2024-03-20 10:04 ` Andrej Picej
0 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-20 10:04 UTC (permalink / raw)
To: haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
Hi all,
we had some problems with failing ADC calibration on the i.MX93 boards.
Changing default calibration settings fixed this. The board where this
patches are useful is not yet upstream but will be soon (hopefully).
Since we had these patches laying around we thought they might also be
useful for someone else.
Best regards,
Andrej
Andrej Picej (2):
iio: adc: imx93: Make calibration properties configurable
dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
.../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++
drivers/iio/adc/imx93_adc.c | 66 +++++++++++++++++--
2 files changed, 76 insertions(+), 5 deletions(-)
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 0/2] i.MX93 ADC calibration settings
@ 2024-03-20 10:04 ` Andrej Picej
0 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-20 10:04 UTC (permalink / raw)
To: haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
Hi all,
we had some problems with failing ADC calibration on the i.MX93 boards.
Changing default calibration settings fixed this. The board where this
patches are useful is not yet upstream but will be soon (hopefully).
Since we had these patches laying around we thought they might also be
useful for someone else.
Best regards,
Andrej
Andrej Picej (2):
iio: adc: imx93: Make calibration properties configurable
dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
.../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++
drivers/iio/adc/imx93_adc.c | 66 +++++++++++++++++--
2 files changed, 76 insertions(+), 5 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 1/2] iio: adc: imx93: Make calibration properties configurable
2024-03-20 10:04 ` Andrej Picej
@ 2024-03-20 10:04 ` Andrej Picej
-1 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-20 10:04 UTC (permalink / raw)
To: haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
Make calibration properties:
- AVGEN: allow averaging of calibration time,
- NRSMPL: select the number of averaging samples during calibration and
- TSAMP: specifies the sample time of calibration conversions
configurable with device tree properties:
- nxp,calib-avg-en,
- nxp,calib-nr-samples and
- nxp,calib-t-samples.
Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
drivers/iio/adc/imx93_adc.c | 66 ++++++++++++++++++++++++++++++++++---
1 file changed, 61 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/imx93_adc.c b/drivers/iio/adc/imx93_adc.c
index 4ccf4819f1f1..ad24105761ab 100644
--- a/drivers/iio/adc/imx93_adc.c
+++ b/drivers/iio/adc/imx93_adc.c
@@ -18,6 +18,7 @@
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/regulator/consumer.h>
+#include <linux/property.h>
#define IMX93_ADC_DRIVER_NAME "imx93-adc"
@@ -43,6 +44,9 @@
#define IMX93_ADC_MCR_MODE_MASK BIT(29)
#define IMX93_ADC_MCR_NSTART_MASK BIT(24)
#define IMX93_ADC_MCR_CALSTART_MASK BIT(14)
+#define IMX93_ADC_MCR_AVGEN_MASK BIT(13)
+#define IMX93_ADC_MCR_NRSMPL_MASK GENMASK(12, 11)
+#define IMX93_ADC_MCR_TSAMP_MASK GENMASK(10, 9)
#define IMX93_ADC_MCR_ADCLKSE_MASK BIT(8)
#define IMX93_ADC_MCR_PWDN_MASK BIT(0)
#define IMX93_ADC_MSR_CALFAIL_MASK BIT(30)
@@ -145,7 +149,7 @@ static void imx93_adc_config_ad_clk(struct imx93_adc *adc)
static int imx93_adc_calibration(struct imx93_adc *adc)
{
- u32 mcr, msr;
+ u32 mcr, msr, value;
int ret;
/* make sure ADC in power down mode */
@@ -156,12 +160,64 @@ static int imx93_adc_calibration(struct imx93_adc *adc)
mcr &= ~FIELD_PREP(IMX93_ADC_MCR_ADCLKSE_MASK, 1);
writel(mcr, adc->regs + IMX93_ADC_MCR);
- imx93_adc_power_up(adc);
-
/*
- * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR,
- * can add the setting of these bit if need in future.
+ * Set calibration settings:
+ * - AVGEN: allow averaging of calibration time,
+ * - NRSMPL: select the number of averaging samples during calibration,
+ * - TSAMP: specifies the sample time of calibration conversions.
*/
+ if (!device_property_read_u32(adc->dev, "nxp,calib-avg-en", &value)) {
+ mcr &= ~IMX93_ADC_MCR_AVGEN_MASK;
+ mcr |= FIELD_PREP(IMX93_ADC_MCR_AVGEN_MASK, value);
+ }
+
+ if (!device_property_read_u32(adc->dev, "nxp,calib-nr-samples", &value)) {
+ switch (value) {
+ case 16:
+ value = 0x0;
+ break;
+ case 32:
+ value = 0x1;
+ break;
+ case 128:
+ value = 0x2;
+ break;
+ case 512:
+ value = 0x3;
+ break;
+ default:
+ dev_warn(adc->dev, "NRSMPL: wrong value, using default: 512\n");
+ value = 0x3;
+ }
+ mcr &= ~IMX93_ADC_MCR_NRSMPL_MASK;
+ mcr |= FIELD_PREP(IMX93_ADC_MCR_NRSMPL_MASK, value);
+ }
+
+ if (!device_property_read_u32(adc->dev, "nxp,calib-t-samples", &value)) {
+ switch (value) {
+ case 8:
+ value = 0x1;
+ break;
+ case 16:
+ value = 0x2;
+ break;
+ case 22:
+ value = 0x0;
+ break;
+ case 32:
+ value = 0x3;
+ break;
+ default:
+ dev_warn(adc->dev, "TSAMP: wrong value, using default: 22\n");
+ value = 0x0;
+ }
+ mcr &= ~IMX93_ADC_MCR_TSAMP_MASK;
+ mcr |= FIELD_PREP(IMX93_ADC_MCR_TSAMP_MASK, value);
+ }
+
+ writel(mcr, adc->regs + IMX93_ADC_MCR);
+
+ imx93_adc_power_up(adc);
/* run calibration */
mcr = readl(adc->regs + IMX93_ADC_MCR);
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 1/2] iio: adc: imx93: Make calibration properties configurable
@ 2024-03-20 10:04 ` Andrej Picej
0 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-20 10:04 UTC (permalink / raw)
To: haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
Make calibration properties:
- AVGEN: allow averaging of calibration time,
- NRSMPL: select the number of averaging samples during calibration and
- TSAMP: specifies the sample time of calibration conversions
configurable with device tree properties:
- nxp,calib-avg-en,
- nxp,calib-nr-samples and
- nxp,calib-t-samples.
Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
drivers/iio/adc/imx93_adc.c | 66 ++++++++++++++++++++++++++++++++++---
1 file changed, 61 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/imx93_adc.c b/drivers/iio/adc/imx93_adc.c
index 4ccf4819f1f1..ad24105761ab 100644
--- a/drivers/iio/adc/imx93_adc.c
+++ b/drivers/iio/adc/imx93_adc.c
@@ -18,6 +18,7 @@
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/regulator/consumer.h>
+#include <linux/property.h>
#define IMX93_ADC_DRIVER_NAME "imx93-adc"
@@ -43,6 +44,9 @@
#define IMX93_ADC_MCR_MODE_MASK BIT(29)
#define IMX93_ADC_MCR_NSTART_MASK BIT(24)
#define IMX93_ADC_MCR_CALSTART_MASK BIT(14)
+#define IMX93_ADC_MCR_AVGEN_MASK BIT(13)
+#define IMX93_ADC_MCR_NRSMPL_MASK GENMASK(12, 11)
+#define IMX93_ADC_MCR_TSAMP_MASK GENMASK(10, 9)
#define IMX93_ADC_MCR_ADCLKSE_MASK BIT(8)
#define IMX93_ADC_MCR_PWDN_MASK BIT(0)
#define IMX93_ADC_MSR_CALFAIL_MASK BIT(30)
@@ -145,7 +149,7 @@ static void imx93_adc_config_ad_clk(struct imx93_adc *adc)
static int imx93_adc_calibration(struct imx93_adc *adc)
{
- u32 mcr, msr;
+ u32 mcr, msr, value;
int ret;
/* make sure ADC in power down mode */
@@ -156,12 +160,64 @@ static int imx93_adc_calibration(struct imx93_adc *adc)
mcr &= ~FIELD_PREP(IMX93_ADC_MCR_ADCLKSE_MASK, 1);
writel(mcr, adc->regs + IMX93_ADC_MCR);
- imx93_adc_power_up(adc);
-
/*
- * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR,
- * can add the setting of these bit if need in future.
+ * Set calibration settings:
+ * - AVGEN: allow averaging of calibration time,
+ * - NRSMPL: select the number of averaging samples during calibration,
+ * - TSAMP: specifies the sample time of calibration conversions.
*/
+ if (!device_property_read_u32(adc->dev, "nxp,calib-avg-en", &value)) {
+ mcr &= ~IMX93_ADC_MCR_AVGEN_MASK;
+ mcr |= FIELD_PREP(IMX93_ADC_MCR_AVGEN_MASK, value);
+ }
+
+ if (!device_property_read_u32(adc->dev, "nxp,calib-nr-samples", &value)) {
+ switch (value) {
+ case 16:
+ value = 0x0;
+ break;
+ case 32:
+ value = 0x1;
+ break;
+ case 128:
+ value = 0x2;
+ break;
+ case 512:
+ value = 0x3;
+ break;
+ default:
+ dev_warn(adc->dev, "NRSMPL: wrong value, using default: 512\n");
+ value = 0x3;
+ }
+ mcr &= ~IMX93_ADC_MCR_NRSMPL_MASK;
+ mcr |= FIELD_PREP(IMX93_ADC_MCR_NRSMPL_MASK, value);
+ }
+
+ if (!device_property_read_u32(adc->dev, "nxp,calib-t-samples", &value)) {
+ switch (value) {
+ case 8:
+ value = 0x1;
+ break;
+ case 16:
+ value = 0x2;
+ break;
+ case 22:
+ value = 0x0;
+ break;
+ case 32:
+ value = 0x3;
+ break;
+ default:
+ dev_warn(adc->dev, "TSAMP: wrong value, using default: 22\n");
+ value = 0x0;
+ }
+ mcr &= ~IMX93_ADC_MCR_TSAMP_MASK;
+ mcr |= FIELD_PREP(IMX93_ADC_MCR_TSAMP_MASK, value);
+ }
+
+ writel(mcr, adc->regs + IMX93_ADC_MCR);
+
+ imx93_adc_power_up(adc);
/* run calibration */
mcr = readl(adc->regs + IMX93_ADC_MCR);
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
2024-03-20 10:04 ` Andrej Picej
@ 2024-03-20 10:04 ` Andrej Picej
-1 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-20 10:04 UTC (permalink / raw)
To: haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
Document calibration properties and how to set them.
Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
.../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
index dacc526dc695..64958be62a6a 100644
--- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
@@ -46,6 +46,21 @@ properties:
"#io-channel-cells":
const: 1
+ nxp,calib-avg-en:
+ description:
+ Enable or disable averaging of calibration time.
+ enum: [ 0, 1 ]
+
+ nxp,calib-nr-samples:
+ description:
+ Selects the number of averaging samples to be used during calibration.
+ enum: [ 16, 32, 128, 512 ]
+
+ nxp,calib-t-samples:
+ description:
+ Specifies the sample time of calibration conversions.
+ enum: [ 8, 16, 22, 32 ]
+
required:
- compatible
- reg
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
@ 2024-03-20 10:04 ` Andrej Picej
0 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-20 10:04 UTC (permalink / raw)
To: haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
Document calibration properties and how to set them.
Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
.../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
index dacc526dc695..64958be62a6a 100644
--- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
@@ -46,6 +46,21 @@ properties:
"#io-channel-cells":
const: 1
+ nxp,calib-avg-en:
+ description:
+ Enable or disable averaging of calibration time.
+ enum: [ 0, 1 ]
+
+ nxp,calib-nr-samples:
+ description:
+ Selects the number of averaging samples to be used during calibration.
+ enum: [ 16, 32, 128, 512 ]
+
+ nxp,calib-t-samples:
+ description:
+ Specifies the sample time of calibration conversions.
+ enum: [ 8, 16, 22, 32 ]
+
required:
- compatible
- reg
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
2024-03-20 10:04 ` Andrej Picej
@ 2024-03-20 10:26 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-20 10:26 UTC (permalink / raw)
To: Andrej Picej, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
On 20/03/2024 11:04, Andrej Picej wrote:
> Document calibration properties and how to set them.
Bindings are before users.
Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.
There is no file extension in prefixes.
>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> index dacc526dc695..64958be62a6a 100644
> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> @@ -46,6 +46,21 @@ properties:
> "#io-channel-cells":
> const: 1
>
> + nxp,calib-avg-en:
> + description:
> + Enable or disable averaging of calibration time.
> + enum: [ 0, 1 ]
> +
> + nxp,calib-nr-samples:
> + description:
> + Selects the number of averaging samples to be used during calibration.
> + enum: [ 16, 32, 128, 512 ]
> +
> + nxp,calib-t-samples:
> + description:
> + Specifies the sample time of calibration conversions.
> + enum: [ 8, 16, 22, 32 ]
No, use existing, generic properties. Open other bindings for this.
Also, none of these were tested. I am not going to review such untested
code.
It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
@ 2024-03-20 10:26 ` Krzysztof Kozlowski
0 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-20 10:26 UTC (permalink / raw)
To: Andrej Picej, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
On 20/03/2024 11:04, Andrej Picej wrote:
> Document calibration properties and how to set them.
Bindings are before users.
Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.
There is no file extension in prefixes.
>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> index dacc526dc695..64958be62a6a 100644
> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> @@ -46,6 +46,21 @@ properties:
> "#io-channel-cells":
> const: 1
>
> + nxp,calib-avg-en:
> + description:
> + Enable or disable averaging of calibration time.
> + enum: [ 0, 1 ]
> +
> + nxp,calib-nr-samples:
> + description:
> + Selects the number of averaging samples to be used during calibration.
> + enum: [ 16, 32, 128, 512 ]
> +
> + nxp,calib-t-samples:
> + description:
> + Specifies the sample time of calibration conversions.
> + enum: [ 8, 16, 22, 32 ]
No, use existing, generic properties. Open other bindings for this.
Also, none of these were tested. I am not going to review such untested
code.
It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
2024-03-20 10:26 ` Krzysztof Kozlowski
@ 2024-03-20 12:05 ` Andrej Picej
-1 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-20 12:05 UTC (permalink / raw)
To: Krzysztof Kozlowski, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
Hi Krzysztof,
On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
> On 20/03/2024 11:04, Andrej Picej wrote:
>> Document calibration properties and how to set them.
>
> Bindings are before users.
will change patch order when I send a v2.
>
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
> There is no file extension in prefixes.
So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
>
>>
>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>> ---
>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>> index dacc526dc695..64958be62a6a 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>> @@ -46,6 +46,21 @@ properties:
>> "#io-channel-cells":
>> const: 1
>>
>> + nxp,calib-avg-en:
>> + description:
>> + Enable or disable averaging of calibration time.
>> + enum: [ 0, 1 ]
>> +
>> + nxp,calib-nr-samples:
>> + description:
>> + Selects the number of averaging samples to be used during calibration.
>> + enum: [ 16, 32, 128, 512 ]
>> +
>> + nxp,calib-t-samples:
>> + description:
>> + Specifies the sample time of calibration conversions.
>> + enum: [ 8, 16, 22, 32 ]
>
> No, use existing, generic properties. Open other bindings for this.
You mean I should use generic properties for the ADC calibration
settings? Is there already something in place? Because as I understand
it, these calib-* values only effect the calibration process of the ADC.
>
> Also, none of these were tested. I am not going to review such untested
> code.
>
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
You are right, I did not run the dt_binding_check, sorry for this,
forgot that this existed. I see now I have to add the:
> $ref: /schemas/types.yaml#/definitions/uint32
Will fix in v2.
BR,
Andrej
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
@ 2024-03-20 12:05 ` Andrej Picej
0 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-20 12:05 UTC (permalink / raw)
To: Krzysztof Kozlowski, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
Hi Krzysztof,
On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
> On 20/03/2024 11:04, Andrej Picej wrote:
>> Document calibration properties and how to set them.
>
> Bindings are before users.
will change patch order when I send a v2.
>
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
> There is no file extension in prefixes.
So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
>
>>
>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>> ---
>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>> index dacc526dc695..64958be62a6a 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>> @@ -46,6 +46,21 @@ properties:
>> "#io-channel-cells":
>> const: 1
>>
>> + nxp,calib-avg-en:
>> + description:
>> + Enable or disable averaging of calibration time.
>> + enum: [ 0, 1 ]
>> +
>> + nxp,calib-nr-samples:
>> + description:
>> + Selects the number of averaging samples to be used during calibration.
>> + enum: [ 16, 32, 128, 512 ]
>> +
>> + nxp,calib-t-samples:
>> + description:
>> + Specifies the sample time of calibration conversions.
>> + enum: [ 8, 16, 22, 32 ]
>
> No, use existing, generic properties. Open other bindings for this.
You mean I should use generic properties for the ADC calibration
settings? Is there already something in place? Because as I understand
it, these calib-* values only effect the calibration process of the ADC.
>
> Also, none of these were tested. I am not going to review such untested
> code.
>
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
You are right, I did not run the dt_binding_check, sorry for this,
forgot that this existed. I see now I have to add the:
> $ref: /schemas/types.yaml#/definitions/uint32
Will fix in v2.
BR,
Andrej
>
> Best regards,
> Krzysztof
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
2024-03-20 12:05 ` Andrej Picej
@ 2024-03-20 12:15 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-20 12:15 UTC (permalink / raw)
To: Andrej Picej, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
On 20/03/2024 13:05, Andrej Picej wrote:
> Hi Krzysztof,
>
> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
>> On 20/03/2024 11:04, Andrej Picej wrote:
>>> Document calibration properties and how to set them.
>>
>> Bindings are before users.
>
> will change patch order when I send a v2.
>
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
>> There is no file extension in prefixes.
>
> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
Did you run the command I proposed? I don't see much of "/", but except
that looks good.
>
>>
>>>
>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>> ---
>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
>>> 1 file changed, 15 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>> index dacc526dc695..64958be62a6a 100644
>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>> @@ -46,6 +46,21 @@ properties:
>>> "#io-channel-cells":
>>> const: 1
>>>
>>> + nxp,calib-avg-en:
>>> + description:
>>> + Enable or disable averaging of calibration time.
>>> + enum: [ 0, 1 ]
>>> +
>>> + nxp,calib-nr-samples:
>>> + description:
>>> + Selects the number of averaging samples to be used during calibration.
>>> + enum: [ 16, 32, 128, 512 ]
>>> +
>>> + nxp,calib-t-samples:
>>> + description:
>>> + Specifies the sample time of calibration conversions.
>>> + enum: [ 8, 16, 22, 32 ]
>>
>> No, use existing, generic properties. Open other bindings for this.
>
> You mean I should use generic properties for the ADC calibration
> settings? Is there already something in place? Because as I understand
> it, these calib-* values only effect the calibration process of the ADC.
Please take a look at other devices and dtschema. We already have some
properties for this... but maybe they cannot be used?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
@ 2024-03-20 12:15 ` Krzysztof Kozlowski
0 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-20 12:15 UTC (permalink / raw)
To: Andrej Picej, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
On 20/03/2024 13:05, Andrej Picej wrote:
> Hi Krzysztof,
>
> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
>> On 20/03/2024 11:04, Andrej Picej wrote:
>>> Document calibration properties and how to set them.
>>
>> Bindings are before users.
>
> will change patch order when I send a v2.
>
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
>> There is no file extension in prefixes.
>
> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
Did you run the command I proposed? I don't see much of "/", but except
that looks good.
>
>>
>>>
>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>> ---
>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
>>> 1 file changed, 15 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>> index dacc526dc695..64958be62a6a 100644
>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>> @@ -46,6 +46,21 @@ properties:
>>> "#io-channel-cells":
>>> const: 1
>>>
>>> + nxp,calib-avg-en:
>>> + description:
>>> + Enable or disable averaging of calibration time.
>>> + enum: [ 0, 1 ]
>>> +
>>> + nxp,calib-nr-samples:
>>> + description:
>>> + Selects the number of averaging samples to be used during calibration.
>>> + enum: [ 16, 32, 128, 512 ]
>>> +
>>> + nxp,calib-t-samples:
>>> + description:
>>> + Specifies the sample time of calibration conversions.
>>> + enum: [ 8, 16, 22, 32 ]
>>
>> No, use existing, generic properties. Open other bindings for this.
>
> You mean I should use generic properties for the ADC calibration
> settings? Is there already something in place? Because as I understand
> it, these calib-* values only effect the calibration process of the ADC.
Please take a look at other devices and dtschema. We already have some
properties for this... but maybe they cannot be used?
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
2024-03-20 10:04 ` Andrej Picej
@ 2024-03-20 21:41 ` Rob Herring
-1 siblings, 0 replies; 40+ messages in thread
From: Rob Herring @ 2024-03-20 21:41 UTC (permalink / raw)
To: Andrej Picej
Cc: kernel, jic23, devicetree, haibo.chen, s.hauer, linux-iio,
linux-arm-kernel, festevam, linux-kernel, conor+dt, upstream, imx,
shawnguo, lars, krzysztof.kozlowski+dt
On Wed, 20 Mar 2024 11:04:06 +0100, Andrej Picej wrote:
> Document calibration properties and how to set them.
>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml: nxp,calib-avg-en: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml: nxp,calib-nr-samples: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml: nxp,calib-t-samples: missing type definition
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240320100407.1639082-3-andrej.picej@norik.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
@ 2024-03-20 21:41 ` Rob Herring
0 siblings, 0 replies; 40+ messages in thread
From: Rob Herring @ 2024-03-20 21:41 UTC (permalink / raw)
To: Andrej Picej
Cc: kernel, jic23, devicetree, haibo.chen, s.hauer, linux-iio,
linux-arm-kernel, festevam, linux-kernel, conor+dt, upstream, imx,
shawnguo, lars, krzysztof.kozlowski+dt
On Wed, 20 Mar 2024 11:04:06 +0100, Andrej Picej wrote:
> Document calibration properties and how to set them.
>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml: nxp,calib-avg-en: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml: nxp,calib-nr-samples: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml: nxp,calib-t-samples: missing type definition
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240320100407.1639082-3-andrej.picej@norik.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
2024-03-20 10:04 ` Andrej Picej
@ 2024-03-22 6:47 ` kernel test robot
-1 siblings, 0 replies; 40+ messages in thread
From: kernel test robot @ 2024-03-22 6:47 UTC (permalink / raw)
To: Andrej Picej, haibo.chen, linux-iio, devicetree
Cc: oe-kbuild-all, jic23, lars, shawnguo, s.hauer, kernel, festevam,
imx, linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
Hi Andrej,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.8 next-20240322]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andrej-Picej/iio-adc-imx93-Make-calibration-properties-configurable/20240320-184314
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20240320100407.1639082-3-andrej.picej%40norik.com
patch subject: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240322/202403221438.trdG8I0x-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403221438.trdG8I0x-lkp@intel.com/
dtcheck warnings: (new ones prefixed by >>)
Documentation/devicetree/bindings/net/snps,dwmac.yaml: mac-mode: missing type definition
>> Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml: nxp,calib-avg-en: missing type definition
>> Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml: nxp,calib-nr-samples: missing type definition
>> Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml: nxp,calib-t-samples: missing type definition
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
@ 2024-03-22 6:47 ` kernel test robot
0 siblings, 0 replies; 40+ messages in thread
From: kernel test robot @ 2024-03-22 6:47 UTC (permalink / raw)
To: Andrej Picej, haibo.chen, linux-iio, devicetree
Cc: oe-kbuild-all, jic23, lars, shawnguo, s.hauer, kernel, festevam,
imx, linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
Hi Andrej,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.8 next-20240322]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andrej-Picej/iio-adc-imx93-Make-calibration-properties-configurable/20240320-184314
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20240320100407.1639082-3-andrej.picej%40norik.com
patch subject: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240322/202403221438.trdG8I0x-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403221438.trdG8I0x-lkp@intel.com/
dtcheck warnings: (new ones prefixed by >>)
Documentation/devicetree/bindings/net/snps,dwmac.yaml: mac-mode: missing type definition
>> Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml: nxp,calib-avg-en: missing type definition
>> Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml: nxp,calib-nr-samples: missing type definition
>> Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml: nxp,calib-t-samples: missing type definition
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
2024-03-20 12:15 ` Krzysztof Kozlowski
@ 2024-03-22 7:39 ` Andrej Picej
-1 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-22 7:39 UTC (permalink / raw)
To: Krzysztof Kozlowski, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
On 20. 03. 24 13:15, Krzysztof Kozlowski wrote:
> On 20/03/2024 13:05, Andrej Picej wrote:
>> Hi Krzysztof,
>>
>> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
>>> On 20/03/2024 11:04, Andrej Picej wrote:
>>>> Document calibration properties and how to set them.
>>>
>>> Bindings are before users.
>>
>> will change patch order when I send a v2.
>>
>>>
>>> Please use subject prefixes matching the subsystem. You can get them for
>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>>> your patch is touching.
>>> There is no file extension in prefixes.
>>
>> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
>
> Did you run the command I proposed? I don't see much of "/", but except
> that looks good.
Ok noted.
>
>>
>>>
>>>>
>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>> ---
>>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
>>>> 1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>> index dacc526dc695..64958be62a6a 100644
>>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>> @@ -46,6 +46,21 @@ properties:
>>>> "#io-channel-cells":
>>>> const: 1
>>>>
>>>> + nxp,calib-avg-en:
>>>> + description:
>>>> + Enable or disable averaging of calibration time.
>>>> + enum: [ 0, 1 ]
>>>> +
>>>> + nxp,calib-nr-samples:
>>>> + description:
>>>> + Selects the number of averaging samples to be used during calibration.
>>>> + enum: [ 16, 32, 128, 512 ]
>>>> +
>>>> + nxp,calib-t-samples:
>>>> + description:
>>>> + Specifies the sample time of calibration conversions.
>>>> + enum: [ 8, 16, 22, 32 ]
>>>
>>> No, use existing, generic properties. Open other bindings for this.
>>
>> You mean I should use generic properties for the ADC calibration
>> settings? Is there already something in place? Because as I understand
>> it, these calib-* values only effect the calibration process of the ADC.
>
> Please take a look at other devices and dtschema. We already have some
> properties for this... but maybe they cannot be used?
>
I did look into other ADC devices, grep across iio/adc, adc bindings
folders and couldn't find anything closely related to what we are
looking for. Could you please point me to the properties that you think
should be used for this?
Thank you.
Andrej
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
@ 2024-03-22 7:39 ` Andrej Picej
0 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-22 7:39 UTC (permalink / raw)
To: Krzysztof Kozlowski, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
On 20. 03. 24 13:15, Krzysztof Kozlowski wrote:
> On 20/03/2024 13:05, Andrej Picej wrote:
>> Hi Krzysztof,
>>
>> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
>>> On 20/03/2024 11:04, Andrej Picej wrote:
>>>> Document calibration properties and how to set them.
>>>
>>> Bindings are before users.
>>
>> will change patch order when I send a v2.
>>
>>>
>>> Please use subject prefixes matching the subsystem. You can get them for
>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>>> your patch is touching.
>>> There is no file extension in prefixes.
>>
>> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
>
> Did you run the command I proposed? I don't see much of "/", but except
> that looks good.
Ok noted.
>
>>
>>>
>>>>
>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>> ---
>>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
>>>> 1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>> index dacc526dc695..64958be62a6a 100644
>>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>> @@ -46,6 +46,21 @@ properties:
>>>> "#io-channel-cells":
>>>> const: 1
>>>>
>>>> + nxp,calib-avg-en:
>>>> + description:
>>>> + Enable or disable averaging of calibration time.
>>>> + enum: [ 0, 1 ]
>>>> +
>>>> + nxp,calib-nr-samples:
>>>> + description:
>>>> + Selects the number of averaging samples to be used during calibration.
>>>> + enum: [ 16, 32, 128, 512 ]
>>>> +
>>>> + nxp,calib-t-samples:
>>>> + description:
>>>> + Specifies the sample time of calibration conversions.
>>>> + enum: [ 8, 16, 22, 32 ]
>>>
>>> No, use existing, generic properties. Open other bindings for this.
>>
>> You mean I should use generic properties for the ADC calibration
>> settings? Is there already something in place? Because as I understand
>> it, these calib-* values only effect the calibration process of the ADC.
>
> Please take a look at other devices and dtschema. We already have some
> properties for this... but maybe they cannot be used?
>
I did look into other ADC devices, grep across iio/adc, adc bindings
folders and couldn't find anything closely related to what we are
looking for. Could you please point me to the properties that you think
should be used for this?
Thank you.
Andrej
>
> Best regards,
> Krzysztof
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
2024-03-22 7:39 ` Andrej Picej
@ 2024-03-22 8:14 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-22 8:14 UTC (permalink / raw)
To: Andrej Picej, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
On 22/03/2024 08:39, Andrej Picej wrote:
> On 20. 03. 24 13:15, Krzysztof Kozlowski wrote:
>> On 20/03/2024 13:05, Andrej Picej wrote:
>>> Hi Krzysztof,
>>>
>>> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
>>>> On 20/03/2024 11:04, Andrej Picej wrote:
>>>>> Document calibration properties and how to set them.
>>>>
>>>> Bindings are before users.
>>>
>>> will change patch order when I send a v2.
>>>
>>>>
>>>> Please use subject prefixes matching the subsystem. You can get them for
>>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>>>> your patch is touching.
>>>> There is no file extension in prefixes.
>>>
>>> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
>>
>> Did you run the command I proposed? I don't see much of "/", but except
>> that looks good.
>
> Ok noted.
>
>>
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>>> ---
>>>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
>>>>> 1 file changed, 15 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>> index dacc526dc695..64958be62a6a 100644
>>>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>> @@ -46,6 +46,21 @@ properties:
>>>>> "#io-channel-cells":
>>>>> const: 1
>>>>>
>>>>> + nxp,calib-avg-en:
>>>>> + description:
>>>>> + Enable or disable averaging of calibration time.
>>>>> + enum: [ 0, 1 ]
>>>>> +
>>>>> + nxp,calib-nr-samples:
>>>>> + description:
>>>>> + Selects the number of averaging samples to be used during calibration.
>>>>> + enum: [ 16, 32, 128, 512 ]
>>>>> +
>>>>> + nxp,calib-t-samples:
>>>>> + description:
>>>>> + Specifies the sample time of calibration conversions.
>>>>> + enum: [ 8, 16, 22, 32 ]
>>>>
>>>> No, use existing, generic properties. Open other bindings for this.
>>>
>>> You mean I should use generic properties for the ADC calibration
>>> settings? Is there already something in place? Because as I understand
>>> it, these calib-* values only effect the calibration process of the ADC.
>>
>> Please take a look at other devices and dtschema. We already have some
>> properties for this... but maybe they cannot be used?
>>
>
> I did look into other ADC devices, grep across iio/adc, adc bindings
> folders and couldn't find anything closely related to what we are
> looking for. Could you please point me to the properties that you think
> should be used for this?
Indeed, there are few device specific like qcom,avg-samples. We have
though oversampling-ratio, settling-time-us and min-sample-time (which
is not that good because does not use unit suffix).
Then follow up questions:
- nxp,calib-avg-en: Why is it a board-level decision? I would assume
this depends on user choice and what kind of input you have (which could
be board dependent or could be runtime decision).
- nxp,calib-t-samples: what does it mean? Time is expressed in time
units, but there is nothing about units in the property name.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
@ 2024-03-22 8:14 ` Krzysztof Kozlowski
0 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-22 8:14 UTC (permalink / raw)
To: Andrej Picej, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
On 22/03/2024 08:39, Andrej Picej wrote:
> On 20. 03. 24 13:15, Krzysztof Kozlowski wrote:
>> On 20/03/2024 13:05, Andrej Picej wrote:
>>> Hi Krzysztof,
>>>
>>> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
>>>> On 20/03/2024 11:04, Andrej Picej wrote:
>>>>> Document calibration properties and how to set them.
>>>>
>>>> Bindings are before users.
>>>
>>> will change patch order when I send a v2.
>>>
>>>>
>>>> Please use subject prefixes matching the subsystem. You can get them for
>>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>>>> your patch is touching.
>>>> There is no file extension in prefixes.
>>>
>>> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
>>
>> Did you run the command I proposed? I don't see much of "/", but except
>> that looks good.
>
> Ok noted.
>
>>
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>>> ---
>>>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
>>>>> 1 file changed, 15 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>> index dacc526dc695..64958be62a6a 100644
>>>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>> @@ -46,6 +46,21 @@ properties:
>>>>> "#io-channel-cells":
>>>>> const: 1
>>>>>
>>>>> + nxp,calib-avg-en:
>>>>> + description:
>>>>> + Enable or disable averaging of calibration time.
>>>>> + enum: [ 0, 1 ]
>>>>> +
>>>>> + nxp,calib-nr-samples:
>>>>> + description:
>>>>> + Selects the number of averaging samples to be used during calibration.
>>>>> + enum: [ 16, 32, 128, 512 ]
>>>>> +
>>>>> + nxp,calib-t-samples:
>>>>> + description:
>>>>> + Specifies the sample time of calibration conversions.
>>>>> + enum: [ 8, 16, 22, 32 ]
>>>>
>>>> No, use existing, generic properties. Open other bindings for this.
>>>
>>> You mean I should use generic properties for the ADC calibration
>>> settings? Is there already something in place? Because as I understand
>>> it, these calib-* values only effect the calibration process of the ADC.
>>
>> Please take a look at other devices and dtschema. We already have some
>> properties for this... but maybe they cannot be used?
>>
>
> I did look into other ADC devices, grep across iio/adc, adc bindings
> folders and couldn't find anything closely related to what we are
> looking for. Could you please point me to the properties that you think
> should be used for this?
Indeed, there are few device specific like qcom,avg-samples. We have
though oversampling-ratio, settling-time-us and min-sample-time (which
is not that good because does not use unit suffix).
Then follow up questions:
- nxp,calib-avg-en: Why is it a board-level decision? I would assume
this depends on user choice and what kind of input you have (which could
be board dependent or could be runtime decision).
- nxp,calib-t-samples: what does it mean? Time is expressed in time
units, but there is nothing about units in the property name.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
2024-03-22 8:14 ` Krzysztof Kozlowski
@ 2024-03-22 9:58 ` Andrej Picej
-1 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-22 9:58 UTC (permalink / raw)
To: Krzysztof Kozlowski, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
On 22. 03. 24 09:14, Krzysztof Kozlowski wrote:
> On 22/03/2024 08:39, Andrej Picej wrote:
>> On 20. 03. 24 13:15, Krzysztof Kozlowski wrote:
>>> On 20/03/2024 13:05, Andrej Picej wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
>>>>> On 20/03/2024 11:04, Andrej Picej wrote:
>>>>>> Document calibration properties and how to set them.
>>>>>
>>>>> Bindings are before users.
>>>>
>>>> will change patch order when I send a v2.
>>>>
>>>>>
>>>>> Please use subject prefixes matching the subsystem. You can get them for
>>>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>>>>> your patch is touching.
>>>>> There is no file extension in prefixes.
>>>>
>>>> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
>>>
>>> Did you run the command I proposed? I don't see much of "/", but except
>>> that looks good.
>>
>> Ok noted.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>>>> ---
>>>>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
>>>>>> 1 file changed, 15 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>>> index dacc526dc695..64958be62a6a 100644
>>>>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>>> @@ -46,6 +46,21 @@ properties:
>>>>>> "#io-channel-cells":
>>>>>> const: 1
>>>>>>
>>>>>> + nxp,calib-avg-en:
>>>>>> + description:
>>>>>> + Enable or disable averaging of calibration time.
>>>>>> + enum: [ 0, 1 ]
>>>>>> +
>>>>>> + nxp,calib-nr-samples:
>>>>>> + description:
>>>>>> + Selects the number of averaging samples to be used during calibration.
>>>>>> + enum: [ 16, 32, 128, 512 ]
>>>>>> +
>>>>>> + nxp,calib-t-samples:
>>>>>> + description:
>>>>>> + Specifies the sample time of calibration conversions.
>>>>>> + enum: [ 8, 16, 22, 32 ]
>>>>>
>>>>> No, use existing, generic properties. Open other bindings for this.
>>>>
>>>> You mean I should use generic properties for the ADC calibration
>>>> settings? Is there already something in place? Because as I understand
>>>> it, these calib-* values only effect the calibration process of the ADC.
>>>
>>> Please take a look at other devices and dtschema. We already have some
>>> properties for this... but maybe they cannot be used?
>>>
>>
>> I did look into other ADC devices, grep across iio/adc, adc bindings
>> folders and couldn't find anything closely related to what we are
>> looking for. Could you please point me to the properties that you think
>> should be used for this?
>
> Indeed, there are few device specific like qcom,avg-samples. We have
> though oversampling-ratio, settling-time-us and min-sample-time (which
> is not that good because does not use unit suffix).
Ok, these are examples but I think I should not use them, since these
are i.MX93 ADC specific settings, which are used for configuration of
calibration process, and are not related to the standard conversion
process during runtime. Calibration process is the first step that
should be done after every power-on reset.
>
> Then follow up questions:
> - nxp,calib-avg-en: Why is it a board-level decision? I would assume
> this depends on user choice and what kind of input you have (which could
> be board dependent or could be runtime decision).
Not really sure I get your question, so please elaborate if I missed the
point.
This is a user choice, to enable or disable the averaging function in
calibration, but this is a board-level decision, probably relates on
external ADC regulators and input connections. The same options are used
for every ADC channel and this can not be a runtime decision, since
calibration is done before the ADC is even registered.
> - nxp,calib-t-samples: what does it mean? Time is expressed in time
> units, but there is nothing about units in the property name.
>
You are right, basically this is "time" in cycles of AD_CLK. I should at
least add that to the property description.
Best regards,
Andrej Picej
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
@ 2024-03-22 9:58 ` Andrej Picej
0 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-22 9:58 UTC (permalink / raw)
To: Krzysztof Kozlowski, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
On 22. 03. 24 09:14, Krzysztof Kozlowski wrote:
> On 22/03/2024 08:39, Andrej Picej wrote:
>> On 20. 03. 24 13:15, Krzysztof Kozlowski wrote:
>>> On 20/03/2024 13:05, Andrej Picej wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
>>>>> On 20/03/2024 11:04, Andrej Picej wrote:
>>>>>> Document calibration properties and how to set them.
>>>>>
>>>>> Bindings are before users.
>>>>
>>>> will change patch order when I send a v2.
>>>>
>>>>>
>>>>> Please use subject prefixes matching the subsystem. You can get them for
>>>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>>>>> your patch is touching.
>>>>> There is no file extension in prefixes.
>>>>
>>>> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
>>>
>>> Did you run the command I proposed? I don't see much of "/", but except
>>> that looks good.
>>
>> Ok noted.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>>>> ---
>>>>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
>>>>>> 1 file changed, 15 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>>> index dacc526dc695..64958be62a6a 100644
>>>>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>>> @@ -46,6 +46,21 @@ properties:
>>>>>> "#io-channel-cells":
>>>>>> const: 1
>>>>>>
>>>>>> + nxp,calib-avg-en:
>>>>>> + description:
>>>>>> + Enable or disable averaging of calibration time.
>>>>>> + enum: [ 0, 1 ]
>>>>>> +
>>>>>> + nxp,calib-nr-samples:
>>>>>> + description:
>>>>>> + Selects the number of averaging samples to be used during calibration.
>>>>>> + enum: [ 16, 32, 128, 512 ]
>>>>>> +
>>>>>> + nxp,calib-t-samples:
>>>>>> + description:
>>>>>> + Specifies the sample time of calibration conversions.
>>>>>> + enum: [ 8, 16, 22, 32 ]
>>>>>
>>>>> No, use existing, generic properties. Open other bindings for this.
>>>>
>>>> You mean I should use generic properties for the ADC calibration
>>>> settings? Is there already something in place? Because as I understand
>>>> it, these calib-* values only effect the calibration process of the ADC.
>>>
>>> Please take a look at other devices and dtschema. We already have some
>>> properties for this... but maybe they cannot be used?
>>>
>>
>> I did look into other ADC devices, grep across iio/adc, adc bindings
>> folders and couldn't find anything closely related to what we are
>> looking for. Could you please point me to the properties that you think
>> should be used for this?
>
> Indeed, there are few device specific like qcom,avg-samples. We have
> though oversampling-ratio, settling-time-us and min-sample-time (which
> is not that good because does not use unit suffix).
Ok, these are examples but I think I should not use them, since these
are i.MX93 ADC specific settings, which are used for configuration of
calibration process, and are not related to the standard conversion
process during runtime. Calibration process is the first step that
should be done after every power-on reset.
>
> Then follow up questions:
> - nxp,calib-avg-en: Why is it a board-level decision? I would assume
> this depends on user choice and what kind of input you have (which could
> be board dependent or could be runtime decision).
Not really sure I get your question, so please elaborate if I missed the
point.
This is a user choice, to enable or disable the averaging function in
calibration, but this is a board-level decision, probably relates on
external ADC regulators and input connections. The same options are used
for every ADC channel and this can not be a runtime decision, since
calibration is done before the ADC is even registered.
> - nxp,calib-t-samples: what does it mean? Time is expressed in time
> units, but there is nothing about units in the property name.
>
You are right, basically this is "time" in cycles of AD_CLK. I should at
least add that to the property description.
Best regards,
Andrej Picej
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
2024-03-22 9:58 ` Andrej Picej
@ 2024-03-24 13:54 ` Jonathan Cameron
-1 siblings, 0 replies; 40+ messages in thread
From: Jonathan Cameron @ 2024-03-24 13:54 UTC (permalink / raw)
To: Andrej Picej
Cc: Krzysztof Kozlowski, haibo.chen, linux-iio, devicetree, lars,
shawnguo, s.hauer, kernel, festevam, imx, linux-arm-kernel,
linux-kernel, robh, krzysztof.kozlowski+dt, conor+dt, upstream
On Fri, 22 Mar 2024 10:58:54 +0100
Andrej Picej <andrej.picej@norik.com> wrote:
> On 22. 03. 24 09:14, Krzysztof Kozlowski wrote:
> > On 22/03/2024 08:39, Andrej Picej wrote:
> >> On 20. 03. 24 13:15, Krzysztof Kozlowski wrote:
> >>> On 20/03/2024 13:05, Andrej Picej wrote:
> >>>> Hi Krzysztof,
> >>>>
> >>>> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
> >>>>> On 20/03/2024 11:04, Andrej Picej wrote:
> >>>>>> Document calibration properties and how to set them.
> >>>>>
> >>>>> Bindings are before users.
> >>>>
> >>>> will change patch order when I send a v2.
> >>>>
> >>>>>
> >>>>> Please use subject prefixes matching the subsystem. You can get them for
> >>>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> >>>>> your patch is touching.
> >>>>> There is no file extension in prefixes.
> >>>>
> >>>> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
> >>>
> >>> Did you run the command I proposed? I don't see much of "/", but except
> >>> that looks good.
> >>
> >> Ok noted.
> >>
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> >>>>>> ---
> >>>>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
> >>>>>> 1 file changed, 15 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>> index dacc526dc695..64958be62a6a 100644
> >>>>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>> @@ -46,6 +46,21 @@ properties:
> >>>>>> "#io-channel-cells":
> >>>>>> const: 1
> >>>>>>
> >>>>>> + nxp,calib-avg-en:
> >>>>>> + description:
> >>>>>> + Enable or disable averaging of calibration time.
> >>>>>> + enum: [ 0, 1 ]
> >>>>>> +
> >>>>>> + nxp,calib-nr-samples:
> >>>>>> + description:
> >>>>>> + Selects the number of averaging samples to be used during calibration.
> >>>>>> + enum: [ 16, 32, 128, 512 ]
> >>>>>> +
> >>>>>> + nxp,calib-t-samples:
> >>>>>> + description:
> >>>>>> + Specifies the sample time of calibration conversions.
> >>>>>> + enum: [ 8, 16, 22, 32 ]
> >>>>>
> >>>>> No, use existing, generic properties. Open other bindings for this.
> >>>>
> >>>> You mean I should use generic properties for the ADC calibration
> >>>> settings? Is there already something in place? Because as I understand
> >>>> it, these calib-* values only effect the calibration process of the ADC.
> >>>
> >>> Please take a look at other devices and dtschema. We already have some
> >>> properties for this... but maybe they cannot be used?
> >>>
> >>
> >> I did look into other ADC devices, grep across iio/adc, adc bindings
> >> folders and couldn't find anything closely related to what we are
> >> looking for. Could you please point me to the properties that you think
> >> should be used for this?
> >
> > Indeed, there are few device specific like qcom,avg-samples. We have
> > though oversampling-ratio, settling-time-us and min-sample-time (which
> > is not that good because does not use unit suffix).
>
> Ok, these are examples but I think I should not use them, since these
> are i.MX93 ADC specific settings, which are used for configuration of
> calibration process, and are not related to the standard conversion
> process during runtime. Calibration process is the first step that
> should be done after every power-on reset.
>
> >
> > Then follow up questions:
> > - nxp,calib-avg-en: Why is it a board-level decision? I would assume
> > this depends on user choice and what kind of input you have (which could
> > be board dependent or could be runtime decision).
>
> Not really sure I get your question, so please elaborate if I missed the
> point.
> This is a user choice, to enable or disable the averaging function in
> calibration, but this is a board-level decision, probably relates on
> external ADC regulators and input connections. The same options are used
> for every ADC channel and this can not be a runtime decision, since
> calibration is done before the ADC is even registered.
I'll raise this question in reply to the cover letter or patch 1 where
it is perhaps more appropriate, but I'd really like to know more about why
these are useful at all.
>
> > - nxp,calib-t-samples: what does it mean? Time is expressed in time
> > units, but there is nothing about units in the property name.
> >
>
> You are right, basically this is "time" in cycles of AD_CLK. I should at
> least add that to the property description.
>
> Best regards,
> Andrej Picej
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
@ 2024-03-24 13:54 ` Jonathan Cameron
0 siblings, 0 replies; 40+ messages in thread
From: Jonathan Cameron @ 2024-03-24 13:54 UTC (permalink / raw)
To: Andrej Picej
Cc: Krzysztof Kozlowski, haibo.chen, linux-iio, devicetree, lars,
shawnguo, s.hauer, kernel, festevam, imx, linux-arm-kernel,
linux-kernel, robh, krzysztof.kozlowski+dt, conor+dt, upstream
On Fri, 22 Mar 2024 10:58:54 +0100
Andrej Picej <andrej.picej@norik.com> wrote:
> On 22. 03. 24 09:14, Krzysztof Kozlowski wrote:
> > On 22/03/2024 08:39, Andrej Picej wrote:
> >> On 20. 03. 24 13:15, Krzysztof Kozlowski wrote:
> >>> On 20/03/2024 13:05, Andrej Picej wrote:
> >>>> Hi Krzysztof,
> >>>>
> >>>> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
> >>>>> On 20/03/2024 11:04, Andrej Picej wrote:
> >>>>>> Document calibration properties and how to set them.
> >>>>>
> >>>>> Bindings are before users.
> >>>>
> >>>> will change patch order when I send a v2.
> >>>>
> >>>>>
> >>>>> Please use subject prefixes matching the subsystem. You can get them for
> >>>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> >>>>> your patch is touching.
> >>>>> There is no file extension in prefixes.
> >>>>
> >>>> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
> >>>
> >>> Did you run the command I proposed? I don't see much of "/", but except
> >>> that looks good.
> >>
> >> Ok noted.
> >>
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> >>>>>> ---
> >>>>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
> >>>>>> 1 file changed, 15 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>> index dacc526dc695..64958be62a6a 100644
> >>>>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>> @@ -46,6 +46,21 @@ properties:
> >>>>>> "#io-channel-cells":
> >>>>>> const: 1
> >>>>>>
> >>>>>> + nxp,calib-avg-en:
> >>>>>> + description:
> >>>>>> + Enable or disable averaging of calibration time.
> >>>>>> + enum: [ 0, 1 ]
> >>>>>> +
> >>>>>> + nxp,calib-nr-samples:
> >>>>>> + description:
> >>>>>> + Selects the number of averaging samples to be used during calibration.
> >>>>>> + enum: [ 16, 32, 128, 512 ]
> >>>>>> +
> >>>>>> + nxp,calib-t-samples:
> >>>>>> + description:
> >>>>>> + Specifies the sample time of calibration conversions.
> >>>>>> + enum: [ 8, 16, 22, 32 ]
> >>>>>
> >>>>> No, use existing, generic properties. Open other bindings for this.
> >>>>
> >>>> You mean I should use generic properties for the ADC calibration
> >>>> settings? Is there already something in place? Because as I understand
> >>>> it, these calib-* values only effect the calibration process of the ADC.
> >>>
> >>> Please take a look at other devices and dtschema. We already have some
> >>> properties for this... but maybe they cannot be used?
> >>>
> >>
> >> I did look into other ADC devices, grep across iio/adc, adc bindings
> >> folders and couldn't find anything closely related to what we are
> >> looking for. Could you please point me to the properties that you think
> >> should be used for this?
> >
> > Indeed, there are few device specific like qcom,avg-samples. We have
> > though oversampling-ratio, settling-time-us and min-sample-time (which
> > is not that good because does not use unit suffix).
>
> Ok, these are examples but I think I should not use them, since these
> are i.MX93 ADC specific settings, which are used for configuration of
> calibration process, and are not related to the standard conversion
> process during runtime. Calibration process is the first step that
> should be done after every power-on reset.
>
> >
> > Then follow up questions:
> > - nxp,calib-avg-en: Why is it a board-level decision? I would assume
> > this depends on user choice and what kind of input you have (which could
> > be board dependent or could be runtime decision).
>
> Not really sure I get your question, so please elaborate if I missed the
> point.
> This is a user choice, to enable or disable the averaging function in
> calibration, but this is a board-level decision, probably relates on
> external ADC regulators and input connections. The same options are used
> for every ADC channel and this can not be a runtime decision, since
> calibration is done before the ADC is even registered.
I'll raise this question in reply to the cover letter or patch 1 where
it is perhaps more appropriate, but I'd really like to know more about why
these are useful at all.
>
> > - nxp,calib-t-samples: what does it mean? Time is expressed in time
> > units, but there is nothing about units in the property name.
> >
>
> You are right, basically this is "time" in cycles of AD_CLK. I should at
> least add that to the property description.
>
> Best regards,
> Andrej Picej
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/2] i.MX93 ADC calibration settings
2024-03-20 10:04 ` Andrej Picej
@ 2024-03-24 13:55 ` Jonathan Cameron
-1 siblings, 0 replies; 40+ messages in thread
From: Jonathan Cameron @ 2024-03-24 13:55 UTC (permalink / raw)
To: Andrej Picej
Cc: haibo.chen, linux-iio, devicetree, lars, shawnguo, s.hauer,
kernel, festevam, imx, linux-arm-kernel, linux-kernel, robh,
krzysztof.kozlowski+dt, conor+dt, upstream
On Wed, 20 Mar 2024 11:04:04 +0100
Andrej Picej <andrej.picej@norik.com> wrote:
> Hi all,
>
> we had some problems with failing ADC calibration on the i.MX93 boards.
> Changing default calibration settings fixed this. The board where this
> patches are useful is not yet upstream but will be soon (hopefully).
Tell us more. My initial instinct is that this shouldn't be board specific.
What's the trade off we are making here? Time vs precision of calibration or
something else? If these are set to a level by default that doesn't work
for our board, maybe we should just change them for all devices?
Jonathan
>
> Since we had these patches laying around we thought they might also be
> useful for someone else.
>
> Best regards,
> Andrej
>
> Andrej Picej (2):
> iio: adc: imx93: Make calibration properties configurable
> dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
>
> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++
> drivers/iio/adc/imx93_adc.c | 66 +++++++++++++++++--
> 2 files changed, 76 insertions(+), 5 deletions(-)
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/2] i.MX93 ADC calibration settings
@ 2024-03-24 13:55 ` Jonathan Cameron
0 siblings, 0 replies; 40+ messages in thread
From: Jonathan Cameron @ 2024-03-24 13:55 UTC (permalink / raw)
To: Andrej Picej
Cc: haibo.chen, linux-iio, devicetree, lars, shawnguo, s.hauer,
kernel, festevam, imx, linux-arm-kernel, linux-kernel, robh,
krzysztof.kozlowski+dt, conor+dt, upstream
On Wed, 20 Mar 2024 11:04:04 +0100
Andrej Picej <andrej.picej@norik.com> wrote:
> Hi all,
>
> we had some problems with failing ADC calibration on the i.MX93 boards.
> Changing default calibration settings fixed this. The board where this
> patches are useful is not yet upstream but will be soon (hopefully).
Tell us more. My initial instinct is that this shouldn't be board specific.
What's the trade off we are making here? Time vs precision of calibration or
something else? If these are set to a level by default that doesn't work
for our board, maybe we should just change them for all devices?
Jonathan
>
> Since we had these patches laying around we thought they might also be
> useful for someone else.
>
> Best regards,
> Andrej
>
> Andrej Picej (2):
> iio: adc: imx93: Make calibration properties configurable
> dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
>
> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++
> drivers/iio/adc/imx93_adc.c | 66 +++++++++++++++++--
> 2 files changed, 76 insertions(+), 5 deletions(-)
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/2] iio: adc: imx93: Make calibration properties configurable
2024-03-20 10:04 ` Andrej Picej
@ 2024-03-24 14:02 ` Jonathan Cameron
-1 siblings, 0 replies; 40+ messages in thread
From: Jonathan Cameron @ 2024-03-24 14:02 UTC (permalink / raw)
To: Andrej Picej
Cc: haibo.chen, linux-iio, devicetree, lars, shawnguo, s.hauer,
kernel, festevam, imx, linux-arm-kernel, linux-kernel, robh,
krzysztof.kozlowski+dt, conor+dt, upstream
On Wed, 20 Mar 2024 11:04:05 +0100
Andrej Picej <andrej.picej@norik.com> wrote:
> Make calibration properties:
> - AVGEN: allow averaging of calibration time,
Confused. How do you average time? Or is this enabling averaging of
ADC readings at calibration time?
> - NRSMPL: select the number of averaging samples during calibration and
Assuming I read AVGEN right, just have a value of 1 in here mean AVGEN is
disabled.
> - TSAMP: specifies the sample time of calibration conversions
Not sure what this means. Is it acquisition time? Is it time after a mux
changes? Anyhow, more info needed.
This is the only one I can see being possibly board related. But if it
is and is needed for calibration, why not for normal read out?
>
> configurable with device tree properties:
> - nxp,calib-avg-en,
> - nxp,calib-nr-samples and
> - nxp,calib-t-samples.
>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
> drivers/iio/adc/imx93_adc.c | 66 ++++++++++++++++++++++++++++++++++---
> 1 file changed, 61 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/imx93_adc.c b/drivers/iio/adc/imx93_adc.c
> index 4ccf4819f1f1..ad24105761ab 100644
> --- a/drivers/iio/adc/imx93_adc.c
> +++ b/drivers/iio/adc/imx93_adc.c
> @@ -18,6 +18,7 @@
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/property.h>
>
> #define IMX93_ADC_DRIVER_NAME "imx93-adc"
>
> @@ -43,6 +44,9 @@
> #define IMX93_ADC_MCR_MODE_MASK BIT(29)
> #define IMX93_ADC_MCR_NSTART_MASK BIT(24)
> #define IMX93_ADC_MCR_CALSTART_MASK BIT(14)
> +#define IMX93_ADC_MCR_AVGEN_MASK BIT(13)
> +#define IMX93_ADC_MCR_NRSMPL_MASK GENMASK(12, 11)
> +#define IMX93_ADC_MCR_TSAMP_MASK GENMASK(10, 9)
> #define IMX93_ADC_MCR_ADCLKSE_MASK BIT(8)
> #define IMX93_ADC_MCR_PWDN_MASK BIT(0)
> #define IMX93_ADC_MSR_CALFAIL_MASK BIT(30)
> @@ -145,7 +149,7 @@ static void imx93_adc_config_ad_clk(struct imx93_adc *adc)
>
> static int imx93_adc_calibration(struct imx93_adc *adc)
> {
> - u32 mcr, msr;
> + u32 mcr, msr, value;
> int ret;
>
> /* make sure ADC in power down mode */
> @@ -156,12 +160,64 @@ static int imx93_adc_calibration(struct imx93_adc *adc)
> mcr &= ~FIELD_PREP(IMX93_ADC_MCR_ADCLKSE_MASK, 1);
> writel(mcr, adc->regs + IMX93_ADC_MCR);
>
> - imx93_adc_power_up(adc);
> -
> /*
> - * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR,
> - * can add the setting of these bit if need in future.
> + * Set calibration settings:
> + * - AVGEN: allow averaging of calibration time,
> + * - NRSMPL: select the number of averaging samples during calibration,
> + * - TSAMP: specifies the sample time of calibration conversions.
> */
> + if (!device_property_read_u32(adc->dev, "nxp,calib-avg-en", &value)) {
> + mcr &= ~IMX93_ADC_MCR_AVGEN_MASK;
> + mcr |= FIELD_PREP(IMX93_ADC_MCR_AVGEN_MASK, value);
> + }
> +
> + if (!device_property_read_u32(adc->dev, "nxp,calib-nr-samples", &value)) {
Handle error for not present different from a failure to read or similar.
Not present isn't an error, just a fall back to defaults.
For other error codes we should fail the probe.
> + switch (value) {
> + case 16:
> + value = 0x0;
Don't do this in place, meaning of value before this point different to what
you have in it going forwards. Use a different variable name to make that clear.
reg_val vs nr_samples perhaps?
> + break;
> + case 32:
> + value = 0x1;
> + break;
> + case 128:
> + value = 0x2;
> + break;
> + case 512:
> + value = 0x3;
> + break;
> + default:
> + dev_warn(adc->dev, "NRSMPL: wrong value, using default: 512\n");
Fail the probe rather than papering over a wrong value. I'd rather we got the DT fixed
quickly and if someone wanted another value, they really did want it.
We probably do want a default though for the property not being present (given it is new).
so take setting of this variable outside the if(!device_property_read_u32);
> + value = 0x3;
> + }
> + mcr &= ~IMX93_ADC_MCR_NRSMPL_MASK;
> + mcr |= FIELD_PREP(IMX93_ADC_MCR_NRSMPL_MASK, value);
> + }
> +
> + if (!device_property_read_u32(adc->dev, "nxp,calib-t-samples", &value)) {
> + switch (value) {
> + case 8:
> + value = 0x1;
> + break;
> + case 16:
> + value = 0x2;
> + break;
> + case 22:
> + value = 0x0;
> + break;
> + case 32:
> + value = 0x3;
> + break;
> + default:
> + dev_warn(adc->dev, "TSAMP: wrong value, using default: 22\n");
> + value = 0x0;
> + }
> + mcr &= ~IMX93_ADC_MCR_TSAMP_MASK;
> + mcr |= FIELD_PREP(IMX93_ADC_MCR_TSAMP_MASK, value);
> + }
> +
> + writel(mcr, adc->regs + IMX93_ADC_MCR);
> +
> + imx93_adc_power_up(adc);
>
> /* run calibration */
> mcr = readl(adc->regs + IMX93_ADC_MCR);
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/2] iio: adc: imx93: Make calibration properties configurable
@ 2024-03-24 14:02 ` Jonathan Cameron
0 siblings, 0 replies; 40+ messages in thread
From: Jonathan Cameron @ 2024-03-24 14:02 UTC (permalink / raw)
To: Andrej Picej
Cc: haibo.chen, linux-iio, devicetree, lars, shawnguo, s.hauer,
kernel, festevam, imx, linux-arm-kernel, linux-kernel, robh,
krzysztof.kozlowski+dt, conor+dt, upstream
On Wed, 20 Mar 2024 11:04:05 +0100
Andrej Picej <andrej.picej@norik.com> wrote:
> Make calibration properties:
> - AVGEN: allow averaging of calibration time,
Confused. How do you average time? Or is this enabling averaging of
ADC readings at calibration time?
> - NRSMPL: select the number of averaging samples during calibration and
Assuming I read AVGEN right, just have a value of 1 in here mean AVGEN is
disabled.
> - TSAMP: specifies the sample time of calibration conversions
Not sure what this means. Is it acquisition time? Is it time after a mux
changes? Anyhow, more info needed.
This is the only one I can see being possibly board related. But if it
is and is needed for calibration, why not for normal read out?
>
> configurable with device tree properties:
> - nxp,calib-avg-en,
> - nxp,calib-nr-samples and
> - nxp,calib-t-samples.
>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
> drivers/iio/adc/imx93_adc.c | 66 ++++++++++++++++++++++++++++++++++---
> 1 file changed, 61 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/imx93_adc.c b/drivers/iio/adc/imx93_adc.c
> index 4ccf4819f1f1..ad24105761ab 100644
> --- a/drivers/iio/adc/imx93_adc.c
> +++ b/drivers/iio/adc/imx93_adc.c
> @@ -18,6 +18,7 @@
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/property.h>
>
> #define IMX93_ADC_DRIVER_NAME "imx93-adc"
>
> @@ -43,6 +44,9 @@
> #define IMX93_ADC_MCR_MODE_MASK BIT(29)
> #define IMX93_ADC_MCR_NSTART_MASK BIT(24)
> #define IMX93_ADC_MCR_CALSTART_MASK BIT(14)
> +#define IMX93_ADC_MCR_AVGEN_MASK BIT(13)
> +#define IMX93_ADC_MCR_NRSMPL_MASK GENMASK(12, 11)
> +#define IMX93_ADC_MCR_TSAMP_MASK GENMASK(10, 9)
> #define IMX93_ADC_MCR_ADCLKSE_MASK BIT(8)
> #define IMX93_ADC_MCR_PWDN_MASK BIT(0)
> #define IMX93_ADC_MSR_CALFAIL_MASK BIT(30)
> @@ -145,7 +149,7 @@ static void imx93_adc_config_ad_clk(struct imx93_adc *adc)
>
> static int imx93_adc_calibration(struct imx93_adc *adc)
> {
> - u32 mcr, msr;
> + u32 mcr, msr, value;
> int ret;
>
> /* make sure ADC in power down mode */
> @@ -156,12 +160,64 @@ static int imx93_adc_calibration(struct imx93_adc *adc)
> mcr &= ~FIELD_PREP(IMX93_ADC_MCR_ADCLKSE_MASK, 1);
> writel(mcr, adc->regs + IMX93_ADC_MCR);
>
> - imx93_adc_power_up(adc);
> -
> /*
> - * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR,
> - * can add the setting of these bit if need in future.
> + * Set calibration settings:
> + * - AVGEN: allow averaging of calibration time,
> + * - NRSMPL: select the number of averaging samples during calibration,
> + * - TSAMP: specifies the sample time of calibration conversions.
> */
> + if (!device_property_read_u32(adc->dev, "nxp,calib-avg-en", &value)) {
> + mcr &= ~IMX93_ADC_MCR_AVGEN_MASK;
> + mcr |= FIELD_PREP(IMX93_ADC_MCR_AVGEN_MASK, value);
> + }
> +
> + if (!device_property_read_u32(adc->dev, "nxp,calib-nr-samples", &value)) {
Handle error for not present different from a failure to read or similar.
Not present isn't an error, just a fall back to defaults.
For other error codes we should fail the probe.
> + switch (value) {
> + case 16:
> + value = 0x0;
Don't do this in place, meaning of value before this point different to what
you have in it going forwards. Use a different variable name to make that clear.
reg_val vs nr_samples perhaps?
> + break;
> + case 32:
> + value = 0x1;
> + break;
> + case 128:
> + value = 0x2;
> + break;
> + case 512:
> + value = 0x3;
> + break;
> + default:
> + dev_warn(adc->dev, "NRSMPL: wrong value, using default: 512\n");
Fail the probe rather than papering over a wrong value. I'd rather we got the DT fixed
quickly and if someone wanted another value, they really did want it.
We probably do want a default though for the property not being present (given it is new).
so take setting of this variable outside the if(!device_property_read_u32);
> + value = 0x3;
> + }
> + mcr &= ~IMX93_ADC_MCR_NRSMPL_MASK;
> + mcr |= FIELD_PREP(IMX93_ADC_MCR_NRSMPL_MASK, value);
> + }
> +
> + if (!device_property_read_u32(adc->dev, "nxp,calib-t-samples", &value)) {
> + switch (value) {
> + case 8:
> + value = 0x1;
> + break;
> + case 16:
> + value = 0x2;
> + break;
> + case 22:
> + value = 0x0;
> + break;
> + case 32:
> + value = 0x3;
> + break;
> + default:
> + dev_warn(adc->dev, "TSAMP: wrong value, using default: 22\n");
> + value = 0x0;
> + }
> + mcr &= ~IMX93_ADC_MCR_TSAMP_MASK;
> + mcr |= FIELD_PREP(IMX93_ADC_MCR_TSAMP_MASK, value);
> + }
> +
> + writel(mcr, adc->regs + IMX93_ADC_MCR);
> +
> + imx93_adc_power_up(adc);
>
> /* run calibration */
> mcr = readl(adc->regs + IMX93_ADC_MCR);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/2] i.MX93 ADC calibration settings
2024-03-24 13:55 ` Jonathan Cameron
@ 2024-03-25 8:32 ` Andrej Picej
-1 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-25 8:32 UTC (permalink / raw)
To: Jonathan Cameron
Cc: haibo.chen, linux-iio, devicetree, lars, shawnguo, s.hauer,
kernel, festevam, imx, linux-arm-kernel, linux-kernel, robh,
krzysztof.kozlowski+dt, conor+dt, upstream
Hi Jonathan,
On 24. 03. 24 14:55, Jonathan Cameron wrote:
> On Wed, 20 Mar 2024 11:04:04 +0100
> Andrej Picej <andrej.picej@norik.com> wrote:
>
>> Hi all,
>>
>> we had some problems with failing ADC calibration on the i.MX93 boards.
>> Changing default calibration settings fixed this. The board where this
>> patches are useful is not yet upstream but will be soon (hopefully).
>
> Tell us more. My initial instinct is that this shouldn't be board specific.
> What's the trade off we are making here? Time vs precision of calibration or
> something else? If these are set to a level by default that doesn't work
> for our board, maybe we should just change them for all devices?
>
So we have two different boards with the same SoC. On one, the
calibration works with the default values, on the second one the
calibration fails, which makes the ADC unusable. What the ADC lines
measure differ between the boards though. But the implementation is
nothing out of the ordinary.
We tried different things but the only thing that helped is to use
different calibration properties. We tried deferring the probe and
calibration until later boot and after boot, but it did not help.
In the Reference Manual [1] (chapter 72.5.1) it is written:
> 4. Configure desired calibration settings (default values kept for highest accuracy maximum time).
So your assumption is correct, longer calibration time (more averaging
samples) -> higher precision. The default values go for a high accuracy.
And since we use a NRSMPL (Number of Averaging Samples) of 32 instead of
default 512, we reduce the accuracy so the calibration values pass the
internal defined limits.
I'm not sure that changing default values is the right solution here. We
saw default values work with one of the boards. And since the NXP kept
these values adjustable I think there is a reason behind it.
Note: When I say one of the boards I mean one board form. So same board
version, but different HW.
Best regards,
Andrej
[1] i.MX 93 Applications Processor Reference Manual, Rev. 4, 12/2023
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/2] i.MX93 ADC calibration settings
@ 2024-03-25 8:32 ` Andrej Picej
0 siblings, 0 replies; 40+ messages in thread
From: Andrej Picej @ 2024-03-25 8:32 UTC (permalink / raw)
To: Jonathan Cameron
Cc: haibo.chen, linux-iio, devicetree, lars, shawnguo, s.hauer,
kernel, festevam, imx, linux-arm-kernel, linux-kernel, robh,
krzysztof.kozlowski+dt, conor+dt, upstream
Hi Jonathan,
On 24. 03. 24 14:55, Jonathan Cameron wrote:
> On Wed, 20 Mar 2024 11:04:04 +0100
> Andrej Picej <andrej.picej@norik.com> wrote:
>
>> Hi all,
>>
>> we had some problems with failing ADC calibration on the i.MX93 boards.
>> Changing default calibration settings fixed this. The board where this
>> patches are useful is not yet upstream but will be soon (hopefully).
>
> Tell us more. My initial instinct is that this shouldn't be board specific.
> What's the trade off we are making here? Time vs precision of calibration or
> something else? If these are set to a level by default that doesn't work
> for our board, maybe we should just change them for all devices?
>
So we have two different boards with the same SoC. On one, the
calibration works with the default values, on the second one the
calibration fails, which makes the ADC unusable. What the ADC lines
measure differ between the boards though. But the implementation is
nothing out of the ordinary.
We tried different things but the only thing that helped is to use
different calibration properties. We tried deferring the probe and
calibration until later boot and after boot, but it did not help.
In the Reference Manual [1] (chapter 72.5.1) it is written:
> 4. Configure desired calibration settings (default values kept for highest accuracy maximum time).
So your assumption is correct, longer calibration time (more averaging
samples) -> higher precision. The default values go for a high accuracy.
And since we use a NRSMPL (Number of Averaging Samples) of 32 instead of
default 512, we reduce the accuracy so the calibration values pass the
internal defined limits.
I'm not sure that changing default values is the right solution here. We
saw default values work with one of the boards. And since the NXP kept
these values adjustable I think there is a reason behind it.
Note: When I say one of the boards I mean one board form. So same board
version, but different HW.
Best regards,
Andrej
[1] i.MX 93 Applications Processor Reference Manual, Rev. 4, 12/2023
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Upstream] [PATCH 0/2] i.MX93 ADC calibration settings
2024-03-25 8:32 ` Andrej Picej
@ 2024-03-25 8:55 ` Primoz Fiser
-1 siblings, 0 replies; 40+ messages in thread
From: Primoz Fiser @ 2024-03-25 8:55 UTC (permalink / raw)
To: Andrej Picej, Jonathan Cameron
Cc: devicetree, conor+dt, lars, krzysztof.kozlowski+dt, imx,
linux-iio, festevam, s.hauer, upstream, linux-kernel, haibo.chen,
kernel, shawnguo, robh, linux-arm-kernel
Hi Jonathan,
On 25. 03. 24 09:32, Andrej Picej wrote:
> Hi Jonathan,
>
> On 24. 03. 24 14:55, Jonathan Cameron wrote:
>> On Wed, 20 Mar 2024 11:04:04 +0100
>> Andrej Picej <andrej.picej@norik.com> wrote:
>>
>>> Hi all,
>>>
>>> we had some problems with failing ADC calibration on the i.MX93 boards.
>>> Changing default calibration settings fixed this. The board where this
>>> patches are useful is not yet upstream but will be soon (hopefully).
>>
>> Tell us more. My initial instinct is that this shouldn't be board
>> specific.
>> What's the trade off we are making here? Time vs precision of
>> calibration or
>> something else? If these are set to a level by default that doesn't work
>> for our board, maybe we should just change them for all devices?
>>
The imx93_adc driver is quite new.
If you look at line #162, you will find a comment by the original author:
> /*
> * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR,
> * can add the setting of these bit if need in future.
> */
URL:
https://github.com/torvalds/linux/blob/master/drivers/iio/adc/imx93_adc.c#L162
So, for most use-cases the default setting should work, but why not make
them configurable?
So this patch-series just implement what was missing from the beginning
/ was planned for later.
BR,
Primoz
>
> So we have two different boards with the same SoC. On one, the
> calibration works with the default values, on the second one the
> calibration fails, which makes the ADC unusable. What the ADC lines
> measure differ between the boards though. But the implementation is
> nothing out of the ordinary.
>
> We tried different things but the only thing that helped is to use
> different calibration properties. We tried deferring the probe and
> calibration until later boot and after boot, but it did not help.
>
> In the Reference Manual [1] (chapter 72.5.1) it is written:
>
>> 4. Configure desired calibration settings (default values kept for
>> highest accuracy maximum time).
>
> So your assumption is correct, longer calibration time (more averaging
> samples) -> higher precision. The default values go for a high accuracy.
> And since we use a NRSMPL (Number of Averaging Samples) of 32 instead of
> default 512, we reduce the accuracy so the calibration values pass the
> internal defined limits.
>
> I'm not sure that changing default values is the right solution here. We
> saw default values work with one of the boards. And since the NXP kept
> these values adjustable I think there is a reason behind it.
>
> Note: When I say one of the boards I mean one board form. So same board
> version, but different HW.
>
> Best regards,
> Andrej
>
> [1] i.MX 93 Applications Processor Reference Manual, Rev. 4, 12/2023
> _______________________________________________
> upstream mailing list
> upstream@lists.phytec.de
> http://lists.phytec.de/cgi-bin/mailman/listinfo/upstream
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Upstream] [PATCH 0/2] i.MX93 ADC calibration settings
@ 2024-03-25 8:55 ` Primoz Fiser
0 siblings, 0 replies; 40+ messages in thread
From: Primoz Fiser @ 2024-03-25 8:55 UTC (permalink / raw)
To: Andrej Picej, Jonathan Cameron
Cc: devicetree, conor+dt, lars, krzysztof.kozlowski+dt, imx,
linux-iio, festevam, s.hauer, upstream, linux-kernel, haibo.chen,
kernel, shawnguo, robh, linux-arm-kernel
Hi Jonathan,
On 25. 03. 24 09:32, Andrej Picej wrote:
> Hi Jonathan,
>
> On 24. 03. 24 14:55, Jonathan Cameron wrote:
>> On Wed, 20 Mar 2024 11:04:04 +0100
>> Andrej Picej <andrej.picej@norik.com> wrote:
>>
>>> Hi all,
>>>
>>> we had some problems with failing ADC calibration on the i.MX93 boards.
>>> Changing default calibration settings fixed this. The board where this
>>> patches are useful is not yet upstream but will be soon (hopefully).
>>
>> Tell us more. My initial instinct is that this shouldn't be board
>> specific.
>> What's the trade off we are making here? Time vs precision of
>> calibration or
>> something else? If these are set to a level by default that doesn't work
>> for our board, maybe we should just change them for all devices?
>>
The imx93_adc driver is quite new.
If you look at line #162, you will find a comment by the original author:
> /*
> * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR,
> * can add the setting of these bit if need in future.
> */
URL:
https://github.com/torvalds/linux/blob/master/drivers/iio/adc/imx93_adc.c#L162
So, for most use-cases the default setting should work, but why not make
them configurable?
So this patch-series just implement what was missing from the beginning
/ was planned for later.
BR,
Primoz
>
> So we have two different boards with the same SoC. On one, the
> calibration works with the default values, on the second one the
> calibration fails, which makes the ADC unusable. What the ADC lines
> measure differ between the boards though. But the implementation is
> nothing out of the ordinary.
>
> We tried different things but the only thing that helped is to use
> different calibration properties. We tried deferring the probe and
> calibration until later boot and after boot, but it did not help.
>
> In the Reference Manual [1] (chapter 72.5.1) it is written:
>
>> 4. Configure desired calibration settings (default values kept for
>> highest accuracy maximum time).
>
> So your assumption is correct, longer calibration time (more averaging
> samples) -> higher precision. The default values go for a high accuracy.
> And since we use a NRSMPL (Number of Averaging Samples) of 32 instead of
> default 512, we reduce the accuracy so the calibration values pass the
> internal defined limits.
>
> I'm not sure that changing default values is the right solution here. We
> saw default values work with one of the boards. And since the NXP kept
> these values adjustable I think there is a reason behind it.
>
> Note: When I say one of the boards I mean one board form. So same board
> version, but different HW.
>
> Best regards,
> Andrej
>
> [1] i.MX 93 Applications Processor Reference Manual, Rev. 4, 12/2023
> _______________________________________________
> upstream mailing list
> upstream@lists.phytec.de
> http://lists.phytec.de/cgi-bin/mailman/listinfo/upstream
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
2024-03-22 9:58 ` Andrej Picej
@ 2024-03-25 9:58 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-25 9:58 UTC (permalink / raw)
To: Andrej Picej, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
On 22/03/2024 10:58, Andrej Picej wrote:
> On 22. 03. 24 09:14, Krzysztof Kozlowski wrote:
>> On 22/03/2024 08:39, Andrej Picej wrote:
>>> On 20. 03. 24 13:15, Krzysztof Kozlowski wrote:
>>>> On 20/03/2024 13:05, Andrej Picej wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
>>>>>> On 20/03/2024 11:04, Andrej Picej wrote:
>>>>>>> Document calibration properties and how to set them.
>>>>>>
>>>>>> Bindings are before users.
>>>>>
>>>>> will change patch order when I send a v2.
>>>>>
>>>>>>
>>>>>> Please use subject prefixes matching the subsystem. You can get them for
>>>>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>>>>>> your patch is touching.
>>>>>> There is no file extension in prefixes.
>>>>>
>>>>> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
>>>>
>>>> Did you run the command I proposed? I don't see much of "/", but except
>>>> that looks good.
>>>
>>> Ok noted.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>>>>> ---
>>>>>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
>>>>>>> 1 file changed, 15 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>>>> index dacc526dc695..64958be62a6a 100644
>>>>>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>>>> @@ -46,6 +46,21 @@ properties:
>>>>>>> "#io-channel-cells":
>>>>>>> const: 1
>>>>>>>
>>>>>>> + nxp,calib-avg-en:
>>>>>>> + description:
>>>>>>> + Enable or disable averaging of calibration time.
>>>>>>> + enum: [ 0, 1 ]
>>>>>>> +
>>>>>>> + nxp,calib-nr-samples:
>>>>>>> + description:
>>>>>>> + Selects the number of averaging samples to be used during calibration.
>>>>>>> + enum: [ 16, 32, 128, 512 ]
>>>>>>> +
>>>>>>> + nxp,calib-t-samples:
>>>>>>> + description:
>>>>>>> + Specifies the sample time of calibration conversions.
>>>>>>> + enum: [ 8, 16, 22, 32 ]
>>>>>>
>>>>>> No, use existing, generic properties. Open other bindings for this.
>>>>>
>>>>> You mean I should use generic properties for the ADC calibration
>>>>> settings? Is there already something in place? Because as I understand
>>>>> it, these calib-* values only effect the calibration process of the ADC.
>>>>
>>>> Please take a look at other devices and dtschema. We already have some
>>>> properties for this... but maybe they cannot be used?
>>>>
>>>
>>> I did look into other ADC devices, grep across iio/adc, adc bindings
>>> folders and couldn't find anything closely related to what we are
>>> looking for. Could you please point me to the properties that you think
>>> should be used for this?
>>
>> Indeed, there are few device specific like qcom,avg-samples. We have
>> though oversampling-ratio, settling-time-us and min-sample-time (which
>> is not that good because does not use unit suffix).
>
> Ok, these are examples but I think I should not use them, since these
> are i.MX93 ADC specific settings, which are used for configuration of
No vendor prefix, so they rather should be generic, not imx93
specific... But this the binding for imx93, so I don't understand your
statement.
> calibration process, and are not related to the standard conversion
> process during runtime. Calibration process is the first step that
> should be done after every power-on reset.
>
>>
>> Then follow up questions:
>> - nxp,calib-avg-en: Why is it a board-level decision? I would assume
>> this depends on user choice and what kind of input you have (which could
>> be board dependent or could be runtime decision).
>
> Not really sure I get your question, so please elaborate if I missed the
> point.
> This is a user choice, to enable or disable the averaging function in
> calibration, but this is a board-level decision, probably relates on
> external ADC regulators and input connections. The same options are used
> for every ADC channel and this can not be a runtime decision, since
> calibration is done before the ADC is even registered.
You now mix how Linux driver behaves with hardware. Why you cannot
recalibrate later, e.g. when something else is being connected to the
exposed pins?
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
@ 2024-03-25 9:58 ` Krzysztof Kozlowski
0 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-25 9:58 UTC (permalink / raw)
To: Andrej Picej, haibo.chen, linux-iio, devicetree
Cc: jic23, lars, shawnguo, s.hauer, kernel, festevam, imx,
linux-arm-kernel, linux-kernel, robh, krzysztof.kozlowski+dt,
conor+dt, upstream
On 22/03/2024 10:58, Andrej Picej wrote:
> On 22. 03. 24 09:14, Krzysztof Kozlowski wrote:
>> On 22/03/2024 08:39, Andrej Picej wrote:
>>> On 20. 03. 24 13:15, Krzysztof Kozlowski wrote:
>>>> On 20/03/2024 13:05, Andrej Picej wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
>>>>>> On 20/03/2024 11:04, Andrej Picej wrote:
>>>>>>> Document calibration properties and how to set them.
>>>>>>
>>>>>> Bindings are before users.
>>>>>
>>>>> will change patch order when I send a v2.
>>>>>
>>>>>>
>>>>>> Please use subject prefixes matching the subsystem. You can get them for
>>>>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>>>>>> your patch is touching.
>>>>>> There is no file extension in prefixes.
>>>>>
>>>>> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
>>>>
>>>> Did you run the command I proposed? I don't see much of "/", but except
>>>> that looks good.
>>>
>>> Ok noted.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>>>>>> ---
>>>>>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
>>>>>>> 1 file changed, 15 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>>>> index dacc526dc695..64958be62a6a 100644
>>>>>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
>>>>>>> @@ -46,6 +46,21 @@ properties:
>>>>>>> "#io-channel-cells":
>>>>>>> const: 1
>>>>>>>
>>>>>>> + nxp,calib-avg-en:
>>>>>>> + description:
>>>>>>> + Enable or disable averaging of calibration time.
>>>>>>> + enum: [ 0, 1 ]
>>>>>>> +
>>>>>>> + nxp,calib-nr-samples:
>>>>>>> + description:
>>>>>>> + Selects the number of averaging samples to be used during calibration.
>>>>>>> + enum: [ 16, 32, 128, 512 ]
>>>>>>> +
>>>>>>> + nxp,calib-t-samples:
>>>>>>> + description:
>>>>>>> + Specifies the sample time of calibration conversions.
>>>>>>> + enum: [ 8, 16, 22, 32 ]
>>>>>>
>>>>>> No, use existing, generic properties. Open other bindings for this.
>>>>>
>>>>> You mean I should use generic properties for the ADC calibration
>>>>> settings? Is there already something in place? Because as I understand
>>>>> it, these calib-* values only effect the calibration process of the ADC.
>>>>
>>>> Please take a look at other devices and dtschema. We already have some
>>>> properties for this... but maybe they cannot be used?
>>>>
>>>
>>> I did look into other ADC devices, grep across iio/adc, adc bindings
>>> folders and couldn't find anything closely related to what we are
>>> looking for. Could you please point me to the properties that you think
>>> should be used for this?
>>
>> Indeed, there are few device specific like qcom,avg-samples. We have
>> though oversampling-ratio, settling-time-us and min-sample-time (which
>> is not that good because does not use unit suffix).
>
> Ok, these are examples but I think I should not use them, since these
> are i.MX93 ADC specific settings, which are used for configuration of
No vendor prefix, so they rather should be generic, not imx93
specific... But this the binding for imx93, so I don't understand your
statement.
> calibration process, and are not related to the standard conversion
> process during runtime. Calibration process is the first step that
> should be done after every power-on reset.
>
>>
>> Then follow up questions:
>> - nxp,calib-avg-en: Why is it a board-level decision? I would assume
>> this depends on user choice and what kind of input you have (which could
>> be board dependent or could be runtime decision).
>
> Not really sure I get your question, so please elaborate if I missed the
> point.
> This is a user choice, to enable or disable the averaging function in
> calibration, but this is a board-level decision, probably relates on
> external ADC regulators and input connections. The same options are used
> for every ADC channel and this can not be a runtime decision, since
> calibration is done before the ADC is even registered.
You now mix how Linux driver behaves with hardware. Why you cannot
recalibrate later, e.g. when something else is being connected to the
exposed pins?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
2024-03-25 9:58 ` Krzysztof Kozlowski
@ 2024-03-25 14:38 ` Jonathan Cameron
-1 siblings, 0 replies; 40+ messages in thread
From: Jonathan Cameron @ 2024-03-25 14:38 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Andrej Picej, haibo.chen, linux-iio, devicetree, jic23, lars,
shawnguo, s.hauer, kernel, festevam, imx, linux-arm-kernel,
linux-kernel, robh, krzysztof.kozlowski+dt, conor+dt, upstream
On Mon, 25 Mar 2024 10:58:51 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> On 22/03/2024 10:58, Andrej Picej wrote:
> > On 22. 03. 24 09:14, Krzysztof Kozlowski wrote:
> >> On 22/03/2024 08:39, Andrej Picej wrote:
> >>> On 20. 03. 24 13:15, Krzysztof Kozlowski wrote:
> >>>> On 20/03/2024 13:05, Andrej Picej wrote:
> >>>>> Hi Krzysztof,
> >>>>>
> >>>>> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
> >>>>>> On 20/03/2024 11:04, Andrej Picej wrote:
> >>>>>>> Document calibration properties and how to set them.
> >>>>>>
> >>>>>> Bindings are before users.
> >>>>>
> >>>>> will change patch order when I send a v2.
> >>>>>
> >>>>>>
> >>>>>> Please use subject prefixes matching the subsystem. You can get them for
> >>>>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> >>>>>> your patch is touching.
> >>>>>> There is no file extension in prefixes.
> >>>>>
> >>>>> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
> >>>>
> >>>> Did you run the command I proposed? I don't see much of "/", but except
> >>>> that looks good.
> >>>
> >>> Ok noted.
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> >>>>>>> ---
> >>>>>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
> >>>>>>> 1 file changed, 15 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>>> index dacc526dc695..64958be62a6a 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>>> @@ -46,6 +46,21 @@ properties:
> >>>>>>> "#io-channel-cells":
> >>>>>>> const: 1
> >>>>>>>
> >>>>>>> + nxp,calib-avg-en:
> >>>>>>> + description:
> >>>>>>> + Enable or disable averaging of calibration time.
> >>>>>>> + enum: [ 0, 1 ]
> >>>>>>> +
> >>>>>>> + nxp,calib-nr-samples:
> >>>>>>> + description:
> >>>>>>> + Selects the number of averaging samples to be used during calibration.
> >>>>>>> + enum: [ 16, 32, 128, 512 ]
> >>>>>>> +
> >>>>>>> + nxp,calib-t-samples:
> >>>>>>> + description:
> >>>>>>> + Specifies the sample time of calibration conversions.
> >>>>>>> + enum: [ 8, 16, 22, 32 ]
> >>>>>>
> >>>>>> No, use existing, generic properties. Open other bindings for this.
> >>>>>
> >>>>> You mean I should use generic properties for the ADC calibration
> >>>>> settings? Is there already something in place? Because as I understand
> >>>>> it, these calib-* values only effect the calibration process of the ADC.
> >>>>
> >>>> Please take a look at other devices and dtschema. We already have some
> >>>> properties for this... but maybe they cannot be used?
> >>>>
> >>>
> >>> I did look into other ADC devices, grep across iio/adc, adc bindings
> >>> folders and couldn't find anything closely related to what we are
> >>> looking for. Could you please point me to the properties that you think
> >>> should be used for this?
> >>
> >> Indeed, there are few device specific like qcom,avg-samples. We have
> >> though oversampling-ratio, settling-time-us and min-sample-time (which
> >> is not that good because does not use unit suffix).
> >
> > Ok, these are examples but I think I should not use them, since these
> > are i.MX93 ADC specific settings, which are used for configuration of
>
>
> No vendor prefix, so they rather should be generic, not imx93
> specific... But this the binding for imx93, so I don't understand your
> statement.
Based on my current understanding what we have here is not remotely
generic, so standard properties don't make sense (though naming the
nxp ones in a consistent fashion with other bindings is useful)
I'm not entirely convinced there is a strong argument to support them at all
though. Still thinking / gathering info on that.
>
> > calibration process, and are not related to the standard conversion
> > process during runtime. Calibration process is the first step that
> > should be done after every power-on reset.
> >
> >>
> >> Then follow up questions:
> >> - nxp,calib-avg-en: Why is it a board-level decision? I would assume
> >> this depends on user choice and what kind of input you have (which could
> >> be board dependent or could be runtime decision).
> >
> > Not really sure I get your question, so please elaborate if I missed the
> > point.
> > This is a user choice, to enable or disable the averaging function in
> > calibration, but this is a board-level decision, probably relates on
> > external ADC regulators and input connections. The same options are used
> > for every ADC channel and this can not be a runtime decision, since
> > calibration is done before the ADC is even registered.
>
> You now mix how Linux driver behaves with hardware. Why you cannot
> recalibrate later, e.g. when something else is being connected to the
> exposed pins?
Generally we don't make strong efforts to support dev board use cases where
the components wired tend to change. So normally this isn't too much of
a concern. Previously, we've tried to support this stuff and it always
ends up as a mess because of the crazy range of things that can be wired.
Jonathan
>
> Best regards,
> Krzysztof
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties
@ 2024-03-25 14:38 ` Jonathan Cameron
0 siblings, 0 replies; 40+ messages in thread
From: Jonathan Cameron @ 2024-03-25 14:38 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Andrej Picej, haibo.chen, linux-iio, devicetree, jic23, lars,
shawnguo, s.hauer, kernel, festevam, imx, linux-arm-kernel,
linux-kernel, robh, krzysztof.kozlowski+dt, conor+dt, upstream
On Mon, 25 Mar 2024 10:58:51 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> On 22/03/2024 10:58, Andrej Picej wrote:
> > On 22. 03. 24 09:14, Krzysztof Kozlowski wrote:
> >> On 22/03/2024 08:39, Andrej Picej wrote:
> >>> On 20. 03. 24 13:15, Krzysztof Kozlowski wrote:
> >>>> On 20/03/2024 13:05, Andrej Picej wrote:
> >>>>> Hi Krzysztof,
> >>>>>
> >>>>> On 20. 03. 24 11:26, Krzysztof Kozlowski wrote:
> >>>>>> On 20/03/2024 11:04, Andrej Picej wrote:
> >>>>>>> Document calibration properties and how to set them.
> >>>>>>
> >>>>>> Bindings are before users.
> >>>>>
> >>>>> will change patch order when I send a v2.
> >>>>>
> >>>>>>
> >>>>>> Please use subject prefixes matching the subsystem. You can get them for
> >>>>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> >>>>>> your patch is touching.
> >>>>>> There is no file extension in prefixes.
> >>>>>
> >>>>> So: dt-bindings: iio/adc: nxp,imx93-adc: Add calibration properties?
> >>>>
> >>>> Did you run the command I proposed? I don't see much of "/", but except
> >>>> that looks good.
> >>>
> >>> Ok noted.
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> >>>>>>> ---
> >>>>>>> .../bindings/iio/adc/nxp,imx93-adc.yaml | 15 +++++++++++++++
> >>>>>>> 1 file changed, 15 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>>> index dacc526dc695..64958be62a6a 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> >>>>>>> @@ -46,6 +46,21 @@ properties:
> >>>>>>> "#io-channel-cells":
> >>>>>>> const: 1
> >>>>>>>
> >>>>>>> + nxp,calib-avg-en:
> >>>>>>> + description:
> >>>>>>> + Enable or disable averaging of calibration time.
> >>>>>>> + enum: [ 0, 1 ]
> >>>>>>> +
> >>>>>>> + nxp,calib-nr-samples:
> >>>>>>> + description:
> >>>>>>> + Selects the number of averaging samples to be used during calibration.
> >>>>>>> + enum: [ 16, 32, 128, 512 ]
> >>>>>>> +
> >>>>>>> + nxp,calib-t-samples:
> >>>>>>> + description:
> >>>>>>> + Specifies the sample time of calibration conversions.
> >>>>>>> + enum: [ 8, 16, 22, 32 ]
> >>>>>>
> >>>>>> No, use existing, generic properties. Open other bindings for this.
> >>>>>
> >>>>> You mean I should use generic properties for the ADC calibration
> >>>>> settings? Is there already something in place? Because as I understand
> >>>>> it, these calib-* values only effect the calibration process of the ADC.
> >>>>
> >>>> Please take a look at other devices and dtschema. We already have some
> >>>> properties for this... but maybe they cannot be used?
> >>>>
> >>>
> >>> I did look into other ADC devices, grep across iio/adc, adc bindings
> >>> folders and couldn't find anything closely related to what we are
> >>> looking for. Could you please point me to the properties that you think
> >>> should be used for this?
> >>
> >> Indeed, there are few device specific like qcom,avg-samples. We have
> >> though oversampling-ratio, settling-time-us and min-sample-time (which
> >> is not that good because does not use unit suffix).
> >
> > Ok, these are examples but I think I should not use them, since these
> > are i.MX93 ADC specific settings, which are used for configuration of
>
>
> No vendor prefix, so they rather should be generic, not imx93
> specific... But this the binding for imx93, so I don't understand your
> statement.
Based on my current understanding what we have here is not remotely
generic, so standard properties don't make sense (though naming the
nxp ones in a consistent fashion with other bindings is useful)
I'm not entirely convinced there is a strong argument to support them at all
though. Still thinking / gathering info on that.
>
> > calibration process, and are not related to the standard conversion
> > process during runtime. Calibration process is the first step that
> > should be done after every power-on reset.
> >
> >>
> >> Then follow up questions:
> >> - nxp,calib-avg-en: Why is it a board-level decision? I would assume
> >> this depends on user choice and what kind of input you have (which could
> >> be board dependent or could be runtime decision).
> >
> > Not really sure I get your question, so please elaborate if I missed the
> > point.
> > This is a user choice, to enable or disable the averaging function in
> > calibration, but this is a board-level decision, probably relates on
> > external ADC regulators and input connections. The same options are used
> > for every ADC channel and this can not be a runtime decision, since
> > calibration is done before the ADC is even registered.
>
> You now mix how Linux driver behaves with hardware. Why you cannot
> recalibrate later, e.g. when something else is being connected to the
> exposed pins?
Generally we don't make strong efforts to support dev board use cases where
the components wired tend to change. So normally this isn't too much of
a concern. Previously, we've tried to support this stuff and it always
ends up as a mess because of the crazy range of things that can be wired.
Jonathan
>
> Best regards,
> Krzysztof
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Upstream] [PATCH 0/2] i.MX93 ADC calibration settings
2024-03-25 8:55 ` Primoz Fiser
@ 2024-03-25 14:45 ` Jonathan Cameron
-1 siblings, 0 replies; 40+ messages in thread
From: Jonathan Cameron @ 2024-03-25 14:45 UTC (permalink / raw)
To: Primoz Fiser
Cc: Andrej Picej, Jonathan Cameron, devicetree, conor+dt, lars,
krzysztof.kozlowski+dt, imx, linux-iio, festevam, s.hauer,
upstream, linux-kernel, haibo.chen, kernel, shawnguo, robh,
linux-arm-kernel
On Mon, 25 Mar 2024 09:55:23 +0100
Primoz Fiser <primoz.fiser@norik.com> wrote:
> Hi Jonathan,
>
> On 25. 03. 24 09:32, Andrej Picej wrote:
> > Hi Jonathan,
> >
> > On 24. 03. 24 14:55, Jonathan Cameron wrote:
> >> On Wed, 20 Mar 2024 11:04:04 +0100
> >> Andrej Picej <andrej.picej@norik.com> wrote:
> >>
> >>> Hi all,
> >>>
> >>> we had some problems with failing ADC calibration on the i.MX93 boards.
> >>> Changing default calibration settings fixed this. The board where this
> >>> patches are useful is not yet upstream but will be soon (hopefully).
> >>
> >> Tell us more. My initial instinct is that this shouldn't be board
> >> specific.
> >> What's the trade off we are making here? Time vs precision of
> >> calibration or
> >> something else? If these are set to a level by default that doesn't work
> >> for our board, maybe we should just change them for all devices?
> >>
>
> The imx93_adc driver is quite new.
>
> If you look at line #162, you will find a comment by the original author:
>
> > /*
> > * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR,
> > * can add the setting of these bit if need in future.
> > */
>
> URL:
> https://github.com/torvalds/linux/blob/master/drivers/iio/adc/imx93_adc.c#L162
>
> So, for most use-cases the default setting should work, but why not make
> them configurable?
>
> So this patch-series just implement what was missing from the beginning
> / was planned for later.
Hi Primoz,
I doubt anyone reviewed the comment closely enough to say if what it was
suggesting was sensible or not, so the fact it was listed as a todo
doesn't directly impact this discussion.
>
> BR,
> Primoz
>
>
> >
> > So we have two different boards with the same SoC. On one, the
> > calibration works with the default values, on the second one the
> > calibration fails, which makes the ADC unusable. What the ADC lines
> > measure differ between the boards though. But the implementation is
> > nothing out of the ordinary.
> >
> > We tried different things but the only thing that helped is to use
> > different calibration properties. We tried deferring the probe and
> > calibration until later boot and after boot, but it did not help.
> >
> > In the Reference Manual [1] (chapter 72.5.1) it is written:
> >
> >> 4. Configure desired calibration settings (default values kept for
> >> highest accuracy maximum time).
> >
> > So your assumption is correct, longer calibration time (more averaging
> > samples) -> higher precision. The default values go for a high accuracy.
> > And since we use a NRSMPL (Number of Averaging Samples) of 32 instead of
> > default 512, we reduce the accuracy so the calibration values pass the
> > internal defined limits.
Ouch. Let me try to dig into this. Is this effectively relaxing the
constraints? I guess because a value that is perhaps always biased one way
is considered within bounds if those acceptable bounds are wider because
of lower precision?
I was assuming it was the other way around and the device had fixed constraint
limits and you needed to take more samples due to higher noise. Seems the
opposite is true here and that worries me.
I'll definitely need input from NXP on this as a workaround and their
strong support to consider it.
> >
> > I'm not sure that changing default values is the right solution here. We
> > saw default values work with one of the boards. And since the NXP kept
> > these values adjustable I think there is a reason behind it.
I'd assume trade off between time and calibration precision, not the
sort of use I think you are describing.
> >
> > Note: When I say one of the boards I mean one board form. So same board
> > version, but different HW.
Superficially I'm struggling to not see this as broken hardware that it
is out of expected tolerances in some fashion. Maybe I misunderstood
the issue.
Jonathan
> >
> > Best regards,
> > Andrej
> >
> > [1] i.MX 93 Applications Processor Reference Manual, Rev. 4, 12/2023
> > _______________________________________________
> > upstream mailing list
> > upstream@lists.phytec.de
> > http://lists.phytec.de/cgi-bin/mailman/listinfo/upstream
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Upstream] [PATCH 0/2] i.MX93 ADC calibration settings
@ 2024-03-25 14:45 ` Jonathan Cameron
0 siblings, 0 replies; 40+ messages in thread
From: Jonathan Cameron @ 2024-03-25 14:45 UTC (permalink / raw)
To: Primoz Fiser
Cc: Andrej Picej, Jonathan Cameron, devicetree, conor+dt, lars,
krzysztof.kozlowski+dt, imx, linux-iio, festevam, s.hauer,
upstream, linux-kernel, haibo.chen, kernel, shawnguo, robh,
linux-arm-kernel
On Mon, 25 Mar 2024 09:55:23 +0100
Primoz Fiser <primoz.fiser@norik.com> wrote:
> Hi Jonathan,
>
> On 25. 03. 24 09:32, Andrej Picej wrote:
> > Hi Jonathan,
> >
> > On 24. 03. 24 14:55, Jonathan Cameron wrote:
> >> On Wed, 20 Mar 2024 11:04:04 +0100
> >> Andrej Picej <andrej.picej@norik.com> wrote:
> >>
> >>> Hi all,
> >>>
> >>> we had some problems with failing ADC calibration on the i.MX93 boards.
> >>> Changing default calibration settings fixed this. The board where this
> >>> patches are useful is not yet upstream but will be soon (hopefully).
> >>
> >> Tell us more. My initial instinct is that this shouldn't be board
> >> specific.
> >> What's the trade off we are making here? Time vs precision of
> >> calibration or
> >> something else? If these are set to a level by default that doesn't work
> >> for our board, maybe we should just change them for all devices?
> >>
>
> The imx93_adc driver is quite new.
>
> If you look at line #162, you will find a comment by the original author:
>
> > /*
> > * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR,
> > * can add the setting of these bit if need in future.
> > */
>
> URL:
> https://github.com/torvalds/linux/blob/master/drivers/iio/adc/imx93_adc.c#L162
>
> So, for most use-cases the default setting should work, but why not make
> them configurable?
>
> So this patch-series just implement what was missing from the beginning
> / was planned for later.
Hi Primoz,
I doubt anyone reviewed the comment closely enough to say if what it was
suggesting was sensible or not, so the fact it was listed as a todo
doesn't directly impact this discussion.
>
> BR,
> Primoz
>
>
> >
> > So we have two different boards with the same SoC. On one, the
> > calibration works with the default values, on the second one the
> > calibration fails, which makes the ADC unusable. What the ADC lines
> > measure differ between the boards though. But the implementation is
> > nothing out of the ordinary.
> >
> > We tried different things but the only thing that helped is to use
> > different calibration properties. We tried deferring the probe and
> > calibration until later boot and after boot, but it did not help.
> >
> > In the Reference Manual [1] (chapter 72.5.1) it is written:
> >
> >> 4. Configure desired calibration settings (default values kept for
> >> highest accuracy maximum time).
> >
> > So your assumption is correct, longer calibration time (more averaging
> > samples) -> higher precision. The default values go for a high accuracy.
> > And since we use a NRSMPL (Number of Averaging Samples) of 32 instead of
> > default 512, we reduce the accuracy so the calibration values pass the
> > internal defined limits.
Ouch. Let me try to dig into this. Is this effectively relaxing the
constraints? I guess because a value that is perhaps always biased one way
is considered within bounds if those acceptable bounds are wider because
of lower precision?
I was assuming it was the other way around and the device had fixed constraint
limits and you needed to take more samples due to higher noise. Seems the
opposite is true here and that worries me.
I'll definitely need input from NXP on this as a workaround and their
strong support to consider it.
> >
> > I'm not sure that changing default values is the right solution here. We
> > saw default values work with one of the boards. And since the NXP kept
> > these values adjustable I think there is a reason behind it.
I'd assume trade off between time and calibration precision, not the
sort of use I think you are describing.
> >
> > Note: When I say one of the boards I mean one board form. So same board
> > version, but different HW.
Superficially I'm struggling to not see this as broken hardware that it
is out of expected tolerances in some fashion. Maybe I misunderstood
the issue.
Jonathan
> >
> > Best regards,
> > Andrej
> >
> > [1] i.MX 93 Applications Processor Reference Manual, Rev. 4, 12/2023
> > _______________________________________________
> > upstream mailing list
> > upstream@lists.phytec.de
> > http://lists.phytec.de/cgi-bin/mailman/listinfo/upstream
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Upstream] [PATCH 0/2] i.MX93 ADC calibration settings
2024-03-25 14:45 ` Jonathan Cameron
@ 2024-03-29 7:58 ` Primoz Fiser
-1 siblings, 0 replies; 40+ messages in thread
From: Primoz Fiser @ 2024-03-29 7:58 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Andrej Picej, Jonathan Cameron, devicetree, conor+dt, lars,
krzysztof.kozlowski+dt, imx, linux-iio, festevam, s.hauer,
upstream, linux-kernel, haibo.chen, kernel, shawnguo, robh,
linux-arm-kernel
Hi Jonathan,
On 25. 03. 24 15:45, Jonathan Cameron wrote:
> On Mon, 25 Mar 2024 09:55:23 +0100
> Primoz Fiser <primoz.fiser@norik.com> wrote:
>
>> Hi Jonathan,
>>
>> On 25. 03. 24 09:32, Andrej Picej wrote:
>>> Hi Jonathan,
>>>
>>> On 24. 03. 24 14:55, Jonathan Cameron wrote:
>>>> On Wed, 20 Mar 2024 11:04:04 +0100
>>>> Andrej Picej <andrej.picej@norik.com> wrote:
>>>>
>>>>> Hi all,
>>>>>
>>>>> we had some problems with failing ADC calibration on the i.MX93 boards.
>>>>> Changing default calibration settings fixed this. The board where this
>>>>> patches are useful is not yet upstream but will be soon (hopefully).
>>>>
>>>> Tell us more. My initial instinct is that this shouldn't be board
>>>> specific.
>>>> What's the trade off we are making here? Time vs precision of
>>>> calibration or
>>>> something else? If these are set to a level by default that doesn't work
>>>> for our board, maybe we should just change them for all devices?
>>>>
>>
>> The imx93_adc driver is quite new.
>>
>> If you look at line #162, you will find a comment by the original author:
>>
>>> /*
>>> * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR,
>>> * can add the setting of these bit if need in future.
>>> */
>>
>> URL:
>> https://github.com/torvalds/linux/blob/master/drivers/iio/adc/imx93_adc.c#L162
>>
>> So, for most use-cases the default setting should work, but why not make
>> them configurable?
>>
>> So this patch-series just implement what was missing from the beginning
>> / was planned for later.
> Hi Primoz,
>
> I doubt anyone reviewed the comment closely enough to say if what it was
> suggesting was sensible or not, so the fact it was listed as a todo
> doesn't directly impact this discussion.
I agree.
However on the other hand, since we stumbled upon a use-case that
requires adjusting the driver provided default settings of the i.MX93
ADC, this TODO to us is and was a clear indication from the original
author that the driver needs little TLC.
Anyhow, a stance from the author/NXP would be highly welcoming in this
situation.
BR,
Primoz
>
>>
>> BR,
>> Primoz
>>
>>
>>>
>>> So we have two different boards with the same SoC. On one, the
>>> calibration works with the default values, on the second one the
>>> calibration fails, which makes the ADC unusable. What the ADC lines
>>> measure differ between the boards though. But the implementation is
>>> nothing out of the ordinary.
>>>
>>> We tried different things but the only thing that helped is to use
>>> different calibration properties. We tried deferring the probe and
>>> calibration until later boot and after boot, but it did not help.
>>>
>>> In the Reference Manual [1] (chapter 72.5.1) it is written:
>>>
>>>> 4. Configure desired calibration settings (default values kept for
>>>> highest accuracy maximum time).
>>>
>>> So your assumption is correct, longer calibration time (more averaging
>>> samples) -> higher precision. The default values go for a high accuracy.
>>> And since we use a NRSMPL (Number of Averaging Samples) of 32 instead of
>>> default 512, we reduce the accuracy so the calibration values pass the
>>> internal defined limits.
>
> Ouch. Let me try to dig into this. Is this effectively relaxing the
> constraints? I guess because a value that is perhaps always biased one way
> is considered within bounds if those acceptable bounds are wider because
> of lower precision?
>
> I was assuming it was the other way around and the device had fixed constraint
> limits and you needed to take more samples due to higher noise. Seems the
> opposite is true here and that worries me.
>
> I'll definitely need input from NXP on this as a workaround and their
> strong support to consider it.
>
>>>
>>> I'm not sure that changing default values is the right solution here. We
>>> saw default values work with one of the boards. And since the NXP kept
>>> these values adjustable I think there is a reason behind it.
>
> I'd assume trade off between time and calibration precision, not the
> sort of use I think you are describing.
>
>>>
>>> Note: When I say one of the boards I mean one board form. So same board
>>> version, but different HW.
>
> Superficially I'm struggling to not see this as broken hardware that it
> is out of expected tolerances in some fashion. Maybe I misunderstood
> the issue.
>
> Jonathan
>
>>>
>>> Best regards,
>>> Andrej
>>>
>>> [1] i.MX 93 Applications Processor Reference Manual, Rev. 4, 12/2023
>>> _______________________________________________
>>> upstream mailing list
>>> upstream@lists.phytec.de
>>> http://lists.phytec.de/cgi-bin/mailman/listinfo/upstream
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Primoz Fiser | phone: +386-41-390-545
<tel:+386-41-390-545> |
---------------------------------------------------------|
Norik systems d.o.o. | https://www.norik.com
<https://www.norik.com> |
Your embedded software partner | email: info@norik.com
<mailto:info@norik.com> |
Slovenia, EU | phone: +386-41-540-545
<tel:+386-41-540-545> |
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Upstream] [PATCH 0/2] i.MX93 ADC calibration settings
@ 2024-03-29 7:58 ` Primoz Fiser
0 siblings, 0 replies; 40+ messages in thread
From: Primoz Fiser @ 2024-03-29 7:58 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Andrej Picej, Jonathan Cameron, devicetree, conor+dt, lars,
krzysztof.kozlowski+dt, imx, linux-iio, festevam, s.hauer,
upstream, linux-kernel, haibo.chen, kernel, shawnguo, robh,
linux-arm-kernel
Hi Jonathan,
On 25. 03. 24 15:45, Jonathan Cameron wrote:
> On Mon, 25 Mar 2024 09:55:23 +0100
> Primoz Fiser <primoz.fiser@norik.com> wrote:
>
>> Hi Jonathan,
>>
>> On 25. 03. 24 09:32, Andrej Picej wrote:
>>> Hi Jonathan,
>>>
>>> On 24. 03. 24 14:55, Jonathan Cameron wrote:
>>>> On Wed, 20 Mar 2024 11:04:04 +0100
>>>> Andrej Picej <andrej.picej@norik.com> wrote:
>>>>
>>>>> Hi all,
>>>>>
>>>>> we had some problems with failing ADC calibration on the i.MX93 boards.
>>>>> Changing default calibration settings fixed this. The board where this
>>>>> patches are useful is not yet upstream but will be soon (hopefully).
>>>>
>>>> Tell us more. My initial instinct is that this shouldn't be board
>>>> specific.
>>>> What's the trade off we are making here? Time vs precision of
>>>> calibration or
>>>> something else? If these are set to a level by default that doesn't work
>>>> for our board, maybe we should just change them for all devices?
>>>>
>>
>> The imx93_adc driver is quite new.
>>
>> If you look at line #162, you will find a comment by the original author:
>>
>>> /*
>>> * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR,
>>> * can add the setting of these bit if need in future.
>>> */
>>
>> URL:
>> https://github.com/torvalds/linux/blob/master/drivers/iio/adc/imx93_adc.c#L162
>>
>> So, for most use-cases the default setting should work, but why not make
>> them configurable?
>>
>> So this patch-series just implement what was missing from the beginning
>> / was planned for later.
> Hi Primoz,
>
> I doubt anyone reviewed the comment closely enough to say if what it was
> suggesting was sensible or not, so the fact it was listed as a todo
> doesn't directly impact this discussion.
I agree.
However on the other hand, since we stumbled upon a use-case that
requires adjusting the driver provided default settings of the i.MX93
ADC, this TODO to us is and was a clear indication from the original
author that the driver needs little TLC.
Anyhow, a stance from the author/NXP would be highly welcoming in this
situation.
BR,
Primoz
>
>>
>> BR,
>> Primoz
>>
>>
>>>
>>> So we have two different boards with the same SoC. On one, the
>>> calibration works with the default values, on the second one the
>>> calibration fails, which makes the ADC unusable. What the ADC lines
>>> measure differ between the boards though. But the implementation is
>>> nothing out of the ordinary.
>>>
>>> We tried different things but the only thing that helped is to use
>>> different calibration properties. We tried deferring the probe and
>>> calibration until later boot and after boot, but it did not help.
>>>
>>> In the Reference Manual [1] (chapter 72.5.1) it is written:
>>>
>>>> 4. Configure desired calibration settings (default values kept for
>>>> highest accuracy maximum time).
>>>
>>> So your assumption is correct, longer calibration time (more averaging
>>> samples) -> higher precision. The default values go for a high accuracy.
>>> And since we use a NRSMPL (Number of Averaging Samples) of 32 instead of
>>> default 512, we reduce the accuracy so the calibration values pass the
>>> internal defined limits.
>
> Ouch. Let me try to dig into this. Is this effectively relaxing the
> constraints? I guess because a value that is perhaps always biased one way
> is considered within bounds if those acceptable bounds are wider because
> of lower precision?
>
> I was assuming it was the other way around and the device had fixed constraint
> limits and you needed to take more samples due to higher noise. Seems the
> opposite is true here and that worries me.
>
> I'll definitely need input from NXP on this as a workaround and their
> strong support to consider it.
>
>>>
>>> I'm not sure that changing default values is the right solution here. We
>>> saw default values work with one of the boards. And since the NXP kept
>>> these values adjustable I think there is a reason behind it.
>
> I'd assume trade off between time and calibration precision, not the
> sort of use I think you are describing.
>
>>>
>>> Note: When I say one of the boards I mean one board form. So same board
>>> version, but different HW.
>
> Superficially I'm struggling to not see this as broken hardware that it
> is out of expected tolerances in some fashion. Maybe I misunderstood
> the issue.
>
> Jonathan
>
>>>
>>> Best regards,
>>> Andrej
>>>
>>> [1] i.MX 93 Applications Processor Reference Manual, Rev. 4, 12/2023
>>> _______________________________________________
>>> upstream mailing list
>>> upstream@lists.phytec.de
>>> http://lists.phytec.de/cgi-bin/mailman/listinfo/upstream
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Primoz Fiser | phone: +386-41-390-545
<tel:+386-41-390-545> |
---------------------------------------------------------|
Norik systems d.o.o. | https://www.norik.com
<https://www.norik.com> |
Your embedded software partner | email: info@norik.com
<mailto:info@norik.com> |
Slovenia, EU | phone: +386-41-540-545
<tel:+386-41-540-545> |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2024-03-29 7:59 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-20 10:04 [PATCH 0/2] i.MX93 ADC calibration settings Andrej Picej
2024-03-20 10:04 ` Andrej Picej
2024-03-20 10:04 ` [PATCH 1/2] iio: adc: imx93: Make calibration properties configurable Andrej Picej
2024-03-20 10:04 ` Andrej Picej
2024-03-24 14:02 ` Jonathan Cameron
2024-03-24 14:02 ` Jonathan Cameron
2024-03-20 10:04 ` [PATCH 2/2] dt-bindings: iio: adc: nxp,imx93-adc.yaml: Add calibration properties Andrej Picej
2024-03-20 10:04 ` Andrej Picej
2024-03-20 10:26 ` Krzysztof Kozlowski
2024-03-20 10:26 ` Krzysztof Kozlowski
2024-03-20 12:05 ` Andrej Picej
2024-03-20 12:05 ` Andrej Picej
2024-03-20 12:15 ` Krzysztof Kozlowski
2024-03-20 12:15 ` Krzysztof Kozlowski
2024-03-22 7:39 ` Andrej Picej
2024-03-22 7:39 ` Andrej Picej
2024-03-22 8:14 ` Krzysztof Kozlowski
2024-03-22 8:14 ` Krzysztof Kozlowski
2024-03-22 9:58 ` Andrej Picej
2024-03-22 9:58 ` Andrej Picej
2024-03-24 13:54 ` Jonathan Cameron
2024-03-24 13:54 ` Jonathan Cameron
2024-03-25 9:58 ` Krzysztof Kozlowski
2024-03-25 9:58 ` Krzysztof Kozlowski
2024-03-25 14:38 ` Jonathan Cameron
2024-03-25 14:38 ` Jonathan Cameron
2024-03-20 21:41 ` Rob Herring
2024-03-20 21:41 ` Rob Herring
2024-03-22 6:47 ` kernel test robot
2024-03-22 6:47 ` kernel test robot
2024-03-24 13:55 ` [PATCH 0/2] i.MX93 ADC calibration settings Jonathan Cameron
2024-03-24 13:55 ` Jonathan Cameron
2024-03-25 8:32 ` Andrej Picej
2024-03-25 8:32 ` Andrej Picej
2024-03-25 8:55 ` [Upstream] " Primoz Fiser
2024-03-25 8:55 ` Primoz Fiser
2024-03-25 14:45 ` Jonathan Cameron
2024-03-25 14:45 ` Jonathan Cameron
2024-03-29 7:58 ` Primoz Fiser
2024-03-29 7:58 ` Primoz Fiser
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.