| Message ID | 20230421113516.2710454-2-sean@geanix.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/2] iio: adc: stm32-adc: skip adc-diff-channels setup if none is present | expand |
On Fri, 21 Apr 2023 13:35:16 +0200 Sean Nyekjaer <sean@geanix.com> 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> Olivier, You gave some good feedback on v1. Please take a look to see if it has all been addressed in v2. Thanks, Jonathan > --- > Changes since v1: > - Ignore extra channel for timestamps in PIO mode > - Use single ended count in channel creation (Thanks Olivier Moysan) > > drivers/iio/adc/stm32-adc.c | 40 ++++++++++++++++++------------------- > 1 file changed, 20 insertions(+), 20 deletions(-) > > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c > index 14524c1b5583..99bfe995b6f1 100644 > --- a/drivers/iio/adc/stm32-adc.c > +++ b/drivers/iio/adc/stm32-adc.c > @@ -2038,6 +2038,7 @@ static int stm32_adc_legacy_chan_init(struct iio_dev *indio_dev, > struct stm32_adc_diff_channel diff[STM32_ADC_CH_MAX]; > struct device *dev = &indio_dev->dev; > u32 num_diff = adc->num_diff; > + int num_se = nchans - num_diff; > int size = num_diff * sizeof(*diff) / sizeof(u32); > int scan_index = 0, ret, i, c; > u32 smp = 0, smps[STM32_ADC_CH_MAX], chans[STM32_ADC_CH_MAX]; > @@ -2065,28 +2066,27 @@ 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; > - } > - > - /* 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]); > + ret = device_property_read_u32_array(dev, "st,adc-channels", chans, num_se); > + if (ret == 0 && num_se > 0) { > + 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; > } > + > + /* 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++; > } > - stm32_adc_chan_init_one(indio_dev, &channels[scan_index], > - chans[c], 0, scan_index, false); > - scan_index++; > } > > if (adc->nsmps > 0) { > @@ -2307,7 +2307,7 @@ static int stm32_adc_chan_fw_init(struct iio_dev *indio_dev, bool timestamping) > > if (legacy) > ret = stm32_adc_legacy_chan_init(indio_dev, adc, channels, > - num_channels); > + timestamping ? num_channels - 1 : num_channels); > else > ret = stm32_adc_generic_chan_init(indio_dev, adc, channels); > if (ret < 0)
Hi Sean, Sorry for late answer. I was ooo. Please, find my comment below. On 4/21/23 13:35, 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> > --- > Changes since v1: > - Ignore extra channel for timestamps in PIO mode > - Use single ended count in channel creation (Thanks Olivier Moysan) > > drivers/iio/adc/stm32-adc.c | 40 ++++++++++++++++++------------------- > 1 file changed, 20 insertions(+), 20 deletions(-) > > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c > index 14524c1b5583..99bfe995b6f1 100644 > --- a/drivers/iio/adc/stm32-adc.c > +++ b/drivers/iio/adc/stm32-adc.c > @@ -2038,6 +2038,7 @@ static int stm32_adc_legacy_chan_init(struct iio_dev *indio_dev, > struct stm32_adc_diff_channel diff[STM32_ADC_CH_MAX]; > struct device *dev = &indio_dev->dev; > u32 num_diff = adc->num_diff; > + int num_se = nchans - num_diff; > int size = num_diff * sizeof(*diff) / sizeof(u32); > int scan_index = 0, ret, i, c; > u32 smp = 0, smps[STM32_ADC_CH_MAX], chans[STM32_ADC_CH_MAX]; > @@ -2065,28 +2066,27 @@ 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; > - } > - > - /* 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]); > + ret = device_property_read_u32_array(dev, "st,adc-channels", chans, num_se); I can see a change on device_property_read_u32_array() return check. From device_property_read_u32_array() doc, we should avoid calling the function with num_se=0 : "It's recommended to call device_property_count_u32() instead of calling this function with @val equals %NULL and @nval equals 0." Moreover, in case of error we will not return the error status here. I propose to manage error handling as it is done for st,adc-diff-channels property. So, something like this: if (num_se > 0) { ret = device_property_read_u32_array(dev, "st,adc-channels", chans, num_se); if (ret) { dev_err(&indio_dev->dev, "Failed to get st,adc-channels %d\n", ret); return ret; } for (c = 0; c < num_se; c++) { > + if (ret == 0 && num_se > 0) { > + 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; > } > + > + /* 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++; > } > - stm32_adc_chan_init_one(indio_dev, &channels[scan_index], > - chans[c], 0, scan_index, false); > - scan_index++; > } > > if (adc->nsmps > 0) { > @@ -2307,7 +2307,7 @@ static int stm32_adc_chan_fw_init(struct iio_dev *indio_dev, bool timestamping) > > if (legacy) > ret = stm32_adc_legacy_chan_init(indio_dev, adc, channels, > - num_channels); > + timestamping ? num_channels - 1 : num_channels); > else > ret = stm32_adc_generic_chan_init(indio_dev, adc, channels); > if (ret < 0) BRs Olivier
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c index 14524c1b5583..99bfe995b6f1 100644 --- a/drivers/iio/adc/stm32-adc.c +++ b/drivers/iio/adc/stm32-adc.c @@ -2038,6 +2038,7 @@ static int stm32_adc_legacy_chan_init(struct iio_dev *indio_dev, struct stm32_adc_diff_channel diff[STM32_ADC_CH_MAX]; struct device *dev = &indio_dev->dev; u32 num_diff = adc->num_diff; + int num_se = nchans - num_diff; int size = num_diff * sizeof(*diff) / sizeof(u32); int scan_index = 0, ret, i, c; u32 smp = 0, smps[STM32_ADC_CH_MAX], chans[STM32_ADC_CH_MAX]; @@ -2065,28 +2066,27 @@ 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; - } - - /* 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]); + ret = device_property_read_u32_array(dev, "st,adc-channels", chans, num_se); + if (ret == 0 && num_se > 0) { + 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; } + + /* 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++; } - stm32_adc_chan_init_one(indio_dev, &channels[scan_index], - chans[c], 0, scan_index, false); - scan_index++; } if (adc->nsmps > 0) { @@ -2307,7 +2307,7 @@ static int stm32_adc_chan_fw_init(struct iio_dev *indio_dev, bool timestamping) if (legacy) ret = stm32_adc_legacy_chan_init(indio_dev, adc, channels, - num_channels); + timestamping ? num_channels - 1 : num_channels); else ret = stm32_adc_generic_chan_init(indio_dev, adc, channels); if (ret < 0)
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> --- Changes since v1: - Ignore extra channel for timestamps in PIO mode - Use single ended count in channel creation (Thanks Olivier Moysan) drivers/iio/adc/stm32-adc.c | 40 ++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 20 deletions(-)