[PATCH 1/3] iio: adc: stm32-adc: warn if dt uses legacy channel config

[PATCH 1/3] iio: adc: stm32-adc: warn if dt uses legacy channel config

[Sean Nyekjaer]

Since nearly all stm32 dt's are using the legacy adc channel config,
we should warn users about using it.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/adc/stm32-adc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 1aadb2ad2cab..d8e755d0cc52 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -1993,6 +1993,8 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm
 	const struct stm32_adc_info *adc_info = adc->cfg->adc_info;
 	int num_channels = 0, ret;
 
+	dev_warn(&indio_dev->dev, "using legacy channel config\n");
+
 	ret = device_property_count_u32(dev, "st,adc-channels");
 	if (ret > adc_info->max_channels) {
 		dev_err(&indio_dev->dev, "Bad st,adc-channels?\n");
-- 
2.39.1

[PATCH 2/3] iio: adc: stm32-adc: skip adc-diff-channels setup if none is present

[Sean Nyekjaer]

If no adc differential channels are defined driver will fail with EINVAL:
stm32-adc: probe of 48003000.adc:adc@0 failed with error -22

Fix this by skipping the initialization if no channels are defined.

This applies only to the legacy way of initializing adc channels.

Fixes: d7705f35448a ("iio: adc: stm32-adc: convert to device properties")
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/adc/stm32-adc.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index d8e755d0cc52..a04fcb2dc80a 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -2008,16 +2008,15 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm
 	 * to get the *real* number of channels.
 	 */
 	ret = device_property_count_u32(dev, "st,adc-diff-channels");
-	if (ret < 0)
-		return ret;
-
-	ret /= (int)(sizeof(struct stm32_adc_diff_channel) / sizeof(u32));
-	if (ret > adc_info->max_channels) {
-		dev_err(&indio_dev->dev, "Bad st,adc-diff-channels?\n");
-		return -EINVAL;
-	} else if (ret > 0) {
-		adc->num_diff = ret;
-		num_channels += ret;
+	if (ret > 0) {
+		ret /= (int)(sizeof(struct stm32_adc_diff_channel) / sizeof(u32));
+		if (ret > adc_info->max_channels) {
+			dev_err(&indio_dev->dev, "Bad st,adc-diff-channels?\n");
+			return -EINVAL;
+		} else if (ret > 0) {
+			adc->num_diff = ret;
+			num_channels += ret;
+		}
 	}
 
 	/* Optional sample time is provided either for each, or all channels */
-- 
2.39.1

[PATCH 3/3] iio: adc: stm32-adc: skip adc-channels setup if none is present

[Sean Nyekjaer]

If only adc differential channels are defined driver will fail with
stm32-adc: probe of 48003000.adc:adc@0 failed with error -22

Fix this by skipping the initialization if no channels are defined.

This applies only to the legacy way of initializing adc channels.

Fixes: d7705f35448a ("iio: adc: stm32-adc: convert to device properties")
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/adc/stm32-adc.c | 38 +++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index a04fcb2dc80a..6d87cfaadb5d 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -2065,28 +2065,30 @@ static int stm32_adc_legacy_chan_init(struct iio_dev *indio_dev,
 		}
 	}
 
-	ret = device_property_read_u32_array(dev, "st,adc-channels", chans,
-					     nchans);
-	if (ret)
-		return ret;
-
-	for (c = 0; c < nchans; c++) {
-		if (chans[c] >= adc_info->max_channels) {
-			dev_err(&indio_dev->dev, "Invalid channel %d\n",
-				chans[c]);
-			return -EINVAL;
-		}
+	if (nchans - num_diff > 0) {
+		ret = device_property_read_u32_array(dev, "st,adc-channels", chans,
+				nchans);
+		if (ret)
+			return ret;
 
-		/* Channel can't be configured both as single-ended & diff */
-		for (i = 0; i < num_diff; i++) {
-			if (chans[c] == diff[i].vinp) {
-				dev_err(&indio_dev->dev, "channel %d misconfigured\n",	chans[c]);
+		for (c = 0; c < nchans; c++) {
+			if (chans[c] >= adc_info->max_channels) {
+				dev_err(&indio_dev->dev, "Invalid channel %d\n",
+						chans[c]);
 				return -EINVAL;
 			}
-		}
-		stm32_adc_chan_init_one(indio_dev, &channels[scan_index],
+
+			/* Channel can't be configured both as single-ended & diff */
+			for (i = 0; i < num_diff; i++) {
+				if (chans[c] == diff[i].vinp) {
+					dev_err(&indio_dev->dev, "channel %d misconfigured\n",	chans[c]);
+					return -EINVAL;
+				}
+			}
+			stm32_adc_chan_init_one(indio_dev, &channels[scan_index],
 					chans[c], 0, scan_index, false);
-		scan_index++;
+			scan_index++;
+		}
 	}
 
 	if (adc->nsmps > 0) {
-- 
2.39.1

Re: [Linux-stm32] [PATCH 1/3] iio: adc: stm32-adc: warn if dt uses legacy channel config

[Fabrice Gasnier]

On 3/27/23 10:34, Sean Nyekjaer wrote:
> Since nearly all stm32 dt's are using the legacy adc channel config,
> we should warn users about using it.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/iio/adc/stm32-adc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 1aadb2ad2cab..d8e755d0cc52 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -1993,6 +1993,8 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm
>  	const struct stm32_adc_info *adc_info = adc->cfg->adc_info;
>  	int num_channels = 0, ret;
>  
> +	dev_warn(&indio_dev->dev, "using legacy channel config\n");
> +

Hi Sean,

I'd recommend to avoid adding a dev_warn() on a per driver basis, for
depreacted DT properties. Still I'm not sure if there's some policy in
this area.

IMHO, deprecated properties should be checked by using dt tools
(dt_binding_check / dtbs_check or other mean?). But I have no idea if
this is going to report warnings and when. Purpose would be to avoid
introducing no dts files with these. As commented by Olivier on Patch 3,
we've some downstream patches to adopt the generic bindings (not
upstream 'yet').

Another downside is regarding backward compatibility. In case an old dtb
is used to boot the kernel, as long as there's no functionality loss,
I'd advise not to use any warning here. That's a valid use of an old dt.

In all case, thanks for pointing issues (e.g. Patch 2 & 3), regarding
this patch, I'd nack adding this warning. Please drop this change if you
re-submit or turn this into a dev_dbg().

Best Regards,
Fabrice

>  	ret = device_property_count_u32(dev, "st,adc-channels");
>  	if (ret > adc_info->max_channels) {
>  		dev_err(&indio_dev->dev, "Bad st,adc-channels?\n");

Re: [Linux-stm32] [PATCH 1/3] iio: adc: stm32-adc: warn if dt uses legacy channel config

[Jonathan Cameron]

On Thu, 30 Mar 2023 17:30:32 +0200
Fabrice Gasnier <fabrice.gasnier@foss.st.com> wrote:

> On 3/27/23 10:34, Sean Nyekjaer wrote:
> > Since nearly all stm32 dt's are using the legacy adc channel config,
> > we should warn users about using it.
> > 
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> >  drivers/iio/adc/stm32-adc.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> > index 1aadb2ad2cab..d8e755d0cc52 100644
> > --- a/drivers/iio/adc/stm32-adc.c
> > +++ b/drivers/iio/adc/stm32-adc.c
> > @@ -1993,6 +1993,8 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm
> >  	const struct stm32_adc_info *adc_info = adc->cfg->adc_info;
> >  	int num_channels = 0, ret;
> >  
> > +	dev_warn(&indio_dev->dev, "using legacy channel config\n");
> > +  
> 
> Hi Sean,
> 
> I'd recommend to avoid adding a dev_warn() on a per driver basis, for
> depreacted DT properties. Still I'm not sure if there's some policy in
> this area.
> 
> IMHO, deprecated properties should be checked by using dt tools
> (dt_binding_check / dtbs_check or other mean?). But I have no idea if
> this is going to report warnings and when. Purpose would be to avoid
> introducing no dts files with these. As commented by Olivier on Patch 3,
> we've some downstream patches to adopt the generic bindings (not
> upstream 'yet').
> 
> Another downside is regarding backward compatibility. In case an old dtb
> is used to boot the kernel, as long as there's no functionality loss,
> I'd advise not to use any warning here. That's a valid use of an old dt.
> 
> In all case, thanks for pointing issues (e.g. Patch 2 & 3), regarding
> this patch, I'd nack adding this warning. Please drop this change if you
> re-submit or turn this into a dev_dbg().
> 

Agreed. Better to change to dev_dbg().

Other two patches look good to me, but will leave a bit more time for others
to comment before I pick them up.  As a small side note. They are fixes and
this first patch is not, so they should have been before it in the series
so I can route them to mainline faster than the non fix.

Jonathan

> Best Regards,
> Fabrice
> 
> >  	ret = device_property_count_u32(dev, "st,adc-channels");
> >  	if (ret > adc_info->max_channels) {
> >  		dev_err(&indio_dev->dev, "Bad st,adc-channels?\n");  

Re: [Linux-stm32] [PATCH 1/3] iio: adc: stm32-adc: warn if dt uses legacy channel config

[Sean Nyekjær]

> On 1 Apr 2023, at 16.04, Jonathan Cameron <jic23@kernel.org> wrote:
> 
> On Thu, 30 Mar 2023 17:30:32 +0200
> Fabrice Gasnier <fabrice.gasnier@foss.st.com> wrote:
> 
>> On 3/27/23 10:34, Sean Nyekjaer wrote:
>>> Since nearly all stm32 dt's are using the legacy adc channel config,
>>> we should warn users about using it.
>>> 
>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>>> ---
>>> drivers/iio/adc/stm32-adc.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>> 
>>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
>>> index 1aadb2ad2cab..d8e755d0cc52 100644
>>> --- a/drivers/iio/adc/stm32-adc.c
>>> +++ b/drivers/iio/adc/stm32-adc.c
>>> @@ -1993,6 +1993,8 @@ static int stm32_adc_get_legacy_chan_count(struct iio_dev *indio_dev, struct stm
>>> const struct stm32_adc_info *adc_info = adc->cfg->adc_info;
>>> int num_channels = 0, ret;
>>> 
>>> + dev_warn(&indio_dev->dev, "using legacy channel config\n");
>>> +  
>> 
>> Hi Sean,
>> 
>> I'd recommend to avoid adding a dev_warn() on a per driver basis, for
>> depreacted DT properties. Still I'm not sure if there's some policy in
>> this area.
>> 
>> IMHO, deprecated properties should be checked by using dt tools
>> (dt_binding_check / dtbs_check or other mean?). But I have no idea if
>> this is going to report warnings and when. Purpose would be to avoid
>> introducing no dts files with these. As commented by Olivier on Patch 3,
>> we've some downstream patches to adopt the generic bindings (not
>> upstream 'yet').
>> 
>> Another downside is regarding backward compatibility. In case an old dtb
>> is used to boot the kernel, as long as there's no functionality loss,
>> I'd advise not to use any warning here. That's a valid use of an old dt.
>> 
>> In all case, thanks for pointing issues (e.g. Patch 2 & 3), regarding
>> this patch, I'd nack adding this warning. Please drop this change if you
>> re-submit or turn this into a dev_dbg().
>> 
> 
> Agreed. Better to change to dev_dbg().
> 
> Other two patches look good to me, but will leave a bit more time for others
> to comment before I pick them up.  As a small side note. They are fixes and
> this first patch is not, so they should have been before it in the series
> so I can route them to mainline faster than the non fix.
> 
> Jonathan

Hi Jonathan,

I’ll resubmit the first patch with dev_dbg() as a single commit, and then the fixes as a separate series :)

/Sean

> 
>> Best Regards,
>> Fabrice
>> 
>>> ret = device_property_count_u32(dev, "st,adc-channels");
>>> if (ret > adc_info->max_channels) {
>>> dev_err(&indio_dev->dev, "Bad st,adc-channels?\n");  

Re: [Linux-stm32] [PATCH 3/3] iio: adc: stm32-adc: skip adc-channels setup if none is present

[Olivier MOYSAN]

Hi Sean,

Thanks for your patch.
You're right. The DT updates to use the generic bindings are not yet 
upstreamed, and there are some regressions on legacy bindings support.
Please, find here after some comments.

BRs
Olivier

On 3/27/23 10:34, Sean Nyekjaer wrote:
> If only adc differential channels are defined driver will fail with
> stm32-adc: probe of 48003000.adc:adc@0 failed with error -22
> 
> Fix this by skipping the initialization if no channels are defined.
> 
> This applies only to the legacy way of initializing adc channels.
> 
> Fixes: d7705f35448a ("iio: adc: stm32-adc: convert to device properties")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>   drivers/iio/adc/stm32-adc.c | 38 +++++++++++++++++++------------------
>   1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index a04fcb2dc80a..6d87cfaadb5d 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -2065,28 +2065,30 @@ static int stm32_adc_legacy_chan_init(struct iio_dev *indio_dev,
>   		}
>   	}
>   

In PIO mode an extra channel channel is defined for timestamps. This 
additional channel must be ignored in channel count when initializing 
single and diff channels.

This can be handled in stm32_adc_legacy_chan_init() call from 
stm32_adc_legacy_chan_init() function:

         ret = stm32_adc_legacy_chan_init(indio_dev, adc, channels, 
timestamping ? num_channels - 1 : num_channels);

> -	ret = device_property_read_u32_array(dev, "st,adc-channels", chans,
> -					     nchans);
> -	if (ret)
> -		return ret;
> -
> -	for (c = 0; c < nchans; c++) {
> -		if (chans[c] >= adc_info->max_channels) {
> -			dev_err(&indio_dev->dev, "Invalid channel %d\n",
> -				chans[c]);
> -			return -EINVAL;
> -		}
> +	if (nchans - num_diff > 0) {
> +		ret = device_property_read_u32_array(dev, "st,adc-channels", chans,
> +				nchans);

num_se = nchans - num_diff represents single ended channels number.
single ended count has to be used also in 
device_property_read_u32_array() call:
ret = device_property_read_u32_array(dev, "st,adc-channels", chans, 
num_se);

> +		if (ret)
> +			return ret;
> 
> -		/* Channel can't be configured both as single-ended & diff */
> -		for (i = 0; i < num_diff; i++) {
> -			if (chans[c] == diff[i].vinp) {
> -				dev_err(&indio_dev->dev, "channel %d misconfigured\n",	chans[c]);
> +		for (c = 0; c < nchans; c++) {

and also in this for loop:
for (c = 0; c < num_se; c++) {

> +			if (chans[c] >= adc_info->max_channels) {
> +				dev_err(&indio_dev->dev, "Invalid channel %d\n",
> +						chans[c]);
>   				return -EINVAL;
>   			}
> -		}
> -		stm32_adc_chan_init_one(indio_dev, &channels[scan_index],
> +
> +			/* Channel can't be configured both as single-ended & diff */
> +			for (i = 0; i < num_diff; i++) {
> +				if (chans[c] == diff[i].vinp) {
> +					dev_err(&indio_dev->dev, "channel %d misconfigured\n",	chans[c]);
> +					return -EINVAL;
> +				}
> +			}
> +			stm32_adc_chan_init_one(indio_dev, &channels[scan_index],
>   					chans[c], 0, scan_index, false);
> -		scan_index++;
> +			scan_index++;
> +		}
>   	}
>   
>   	if (adc->nsmps > 0) {