From: Jonathan Cameron <jic23@kernel.org> To: Andrej Picej <andrej.picej@norik.com> Cc: haibo.chen@nxp.com, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, lars@metafoo.de, shawnguo@kernel.org, s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, robh@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, upstream@lists.phytec.de Subject: Re: [PATCH 1/2] iio: adc: imx93: Make calibration properties configurable Date: Sun, 24 Mar 2024 14:02:33 +0000 [thread overview] Message-ID: <20240324140233.577ec7d3@jic23-huawei> (raw) In-Reply-To: <20240320100407.1639082-2-andrej.picej@norik.com> 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);
WARNING: multiple messages have this Message-ID (diff)
From: Jonathan Cameron <jic23@kernel.org> To: Andrej Picej <andrej.picej@norik.com> Cc: haibo.chen@nxp.com, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, lars@metafoo.de, shawnguo@kernel.org, s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, robh@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, upstream@lists.phytec.de Subject: Re: [PATCH 1/2] iio: adc: imx93: Make calibration properties configurable Date: Sun, 24 Mar 2024 14:02:33 +0000 [thread overview] Message-ID: <20240324140233.577ec7d3@jic23-huawei> (raw) In-Reply-To: <20240320100407.1639082-2-andrej.picej@norik.com> 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
next prev parent reply other threads:[~2024-03-24 14:02 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 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 [this message] 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
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20240324140233.577ec7d3@jic23-huawei \ --to=jic23@kernel.org \ --cc=andrej.picej@norik.com \ --cc=conor+dt@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=festevam@gmail.com \ --cc=haibo.chen@nxp.com \ --cc=imx@lists.linux.dev \ --cc=kernel@pengutronix.de \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=lars@metafoo.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-iio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=robh@kernel.org \ --cc=s.hauer@pengutronix.de \ --cc=shawnguo@kernel.org \ --cc=upstream@lists.phytec.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.