* Re: [PATCH v2 2/2] iio: adc: stm32-adc: skip adc-channels setup if none is present
2023-04-21 11:35 ` [PATCH v2 2/2] iio: adc: stm32-adc: skip adc-channels " Sean Nyekjaer
@ 2023-05-01 16:09 ` Jonathan Cameron
2023-05-02 14:45 ` Olivier MOYSAN
1 sibling, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2023-05-01 16:09 UTC (permalink / raw)
To: Sean Nyekjaer
Cc: olivier.moysan, alexandre.torgue, nuno.sa, linux-iio, linux-stm32
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)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2 2/2] iio: adc: stm32-adc: skip adc-channels setup if none is present
2023-04-21 11:35 ` [PATCH v2 2/2] iio: adc: stm32-adc: skip adc-channels " Sean Nyekjaer
2023-05-01 16:09 ` Jonathan Cameron
@ 2023-05-02 14:45 ` Olivier MOYSAN
1 sibling, 0 replies; 4+ messages in thread
From: Olivier MOYSAN @ 2023-05-02 14:45 UTC (permalink / raw)
To: Sean Nyekjaer, jic23, alexandre.torgue, nuno.sa, Fabrice GASNIER
Cc: linux-iio, linux-stm32
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
^ permalink raw reply [flat|nested] 4+ messages in thread