| Message ID | 20240319-ad7944-cleanups-v2-1-50e77269351b@baylibre.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] iio: adc: ad7944: simplify adi,spi-mode property parsing | expand |
On Tue, Mar 19, 2024 at 4:28 PM David Lechner <dlechner@baylibre.com> wrote: > > This simplifies the adi,spi-mode property parsing by using > device_property_match_property_string() instead of two separate > functions. Also, the error return value is now more informative > in cases where there was problem parsing the property. a problem ... > + ret = device_property_match_property_string(dev, "adi,spi-mode", > + ad7944_spi_modes, > + ARRAY_SIZE(ad7944_spi_modes)); > + if (ret < 0) { > + if (ret != -EINVAL) > + return dev_err_probe(dev, ret, > + "getting adi,spi-mode property failed\n"); > - adc->spi_mode = ret; > - } else { Actually we may even leave these unchanged > /* absence of adi,spi-mode property means default mode */ > adc->spi_mode = AD7944_SPI_MODE_DEFAULT; > + } else { > + adc->spi_mode = ret; > } ret = device_property_match_property_string(dev, "adi,spi-mode", ad7944_spi_modes, ARRAY_SIZE(ad7944_spi_modes)); if (ret >= 0) { adc->spi_mode = ret; } else if (ret != -EINVAL) { return dev_err_probe(dev, ret, "getting adi,spi-mode property failed\n"); } else { /* absence of adi,spi-mode property means default mode */ adc->spi_mode = AD7944_SPI_MODE_DEFAULT; } But I can admit this is not an often used approach.
On Tue, Mar 19, 2024 at 10:01 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Mar 19, 2024 at 4:28 PM David Lechner <dlechner@baylibre.com> wrote: > > > > This simplifies the adi,spi-mode property parsing by using > > device_property_match_property_string() instead of two separate > > functions. Also, the error return value is now more informative > > in cases where there was problem parsing the property. > > a problem > > ... > > > + ret = device_property_match_property_string(dev, "adi,spi-mode", > > + ad7944_spi_modes, > > + ARRAY_SIZE(ad7944_spi_modes)); > > + if (ret < 0) { > > + if (ret != -EINVAL) > > + return dev_err_probe(dev, ret, > > + "getting adi,spi-mode property failed\n"); > > > - adc->spi_mode = ret; > > - } else { > > Actually we may even leave these unchanged > > > /* absence of adi,spi-mode property means default mode */ > > adc->spi_mode = AD7944_SPI_MODE_DEFAULT; > > + } else { > > + adc->spi_mode = ret; > > } > > ret = device_property_match_property_string(dev, "adi,spi-mode", > ad7944_spi_modes, > > ARRAY_SIZE(ad7944_spi_modes)); > if (ret >= 0) { > adc->spi_mode = ret; > } else if (ret != -EINVAL) { > return dev_err_probe(dev, ret, > "getting adi,spi-mode > property failed\n"); > } else { > /* absence of adi,spi-mode property means default mode */ > adc->spi_mode = AD7944_SPI_MODE_DEFAULT; > } > > But I can admit this is not an often used approach. > I think Jonathan prefers to have the error path first, so I would like to wait and see if he has an opinion here.
On Tue, 19 Mar 2024 10:28:31 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Tue, Mar 19, 2024 at 10:01 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Tue, Mar 19, 2024 at 4:28 PM David Lechner <dlechner@baylibre.com> wrote: > > > > > > This simplifies the adi,spi-mode property parsing by using > > > device_property_match_property_string() instead of two separate > > > functions. Also, the error return value is now more informative > > > in cases where there was problem parsing the property. > > > > a problem > > I'll fix that up. > > ... > > > > > + ret = device_property_match_property_string(dev, "adi,spi-mode", > > > + ad7944_spi_modes, > > > + ARRAY_SIZE(ad7944_spi_modes)); > > > + if (ret < 0) { > > > + if (ret != -EINVAL) > > > + return dev_err_probe(dev, ret, > > > + "getting adi,spi-mode property failed\n"); > > > > > - adc->spi_mode = ret; > > > - } else { > > > > Actually we may even leave these unchanged > > > > > /* absence of adi,spi-mode property means default mode */ > > > adc->spi_mode = AD7944_SPI_MODE_DEFAULT; > > > + } else { > > > + adc->spi_mode = ret; > > > } > > > > ret = device_property_match_property_string(dev, "adi,spi-mode", > > ad7944_spi_modes, > > > > ARRAY_SIZE(ad7944_spi_modes)); > > if (ret >= 0) { > > adc->spi_mode = ret; > > } else if (ret != -EINVAL) { > > return dev_err_probe(dev, ret, > > "getting adi,spi-mode > > property failed\n"); > > } else { > > /* absence of adi,spi-mode property means default mode */ > > adc->spi_mode = AD7944_SPI_MODE_DEFAULT; > > } > > > > But I can admit this is not an often used approach. > > > > I think Jonathan prefers to have the error path first, so I would like > to wait and see if he has an opinion here. I do prefer error paths first. Thanks. Jonathan
On Tue, 19 Mar 2024 09:27:45 -0500 David Lechner <dlechner@baylibre.com> wrote: > This simplifies the adi,spi-mode property parsing by using > device_property_match_property_string() instead of two separate > functions. Also, the error return value is now more informative > in cases where there was problem parsing the property. > > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: David Lechner <dlechner@baylibre.com> Applied with the patch description tweaked to the togreg-normal branch of iio.git (I'll unwind that back to my more normal branch handling next week) and pushed out for 0-day to take a look at it. Thanks, Jonathan > --- > Changes in v2: > - Reorder error path to avoid else statement > - Link to v1: https://lore.kernel.org/r/20240318-ad7944-cleanups-v1-1-0cbb0349a14f@baylibre.com > --- > drivers/iio/adc/ad7944.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c > index d5ec6b5a41c7..261a3f645fd8 100644 > --- a/drivers/iio/adc/ad7944.c > +++ b/drivers/iio/adc/ad7944.c > @@ -366,7 +366,6 @@ static int ad7944_probe(struct spi_device *spi) > struct ad7944_adc *adc; > bool have_refin = false; > struct regulator *ref; > - const char *str_val; > int ret; > > indio_dev = devm_iio_device_alloc(dev, sizeof(*adc)); > @@ -382,16 +381,18 @@ static int ad7944_probe(struct spi_device *spi) > > adc->timing_spec = chip_info->timing_spec; > > - if (device_property_read_string(dev, "adi,spi-mode", &str_val) == 0) { > - ret = sysfs_match_string(ad7944_spi_modes, str_val); > - if (ret < 0) > - return dev_err_probe(dev, -EINVAL, > - "unsupported adi,spi-mode\n"); > + ret = device_property_match_property_string(dev, "adi,spi-mode", > + ad7944_spi_modes, > + ARRAY_SIZE(ad7944_spi_modes)); > + if (ret < 0) { > + if (ret != -EINVAL) > + return dev_err_probe(dev, ret, > + "getting adi,spi-mode property failed\n"); > > - adc->spi_mode = ret; > - } else { > /* absence of adi,spi-mode property means default mode */ > adc->spi_mode = AD7944_SPI_MODE_DEFAULT; > + } else { > + adc->spi_mode = ret; > } > > if (adc->spi_mode == AD7944_SPI_MODE_CHAIN) > > --- > base-commit: 1446d8ef48196409f811c25071b2cc510a49fc60 > change-id: 20240318-ad7944-cleanups-9f95a7c598b6
Sat, Mar 23, 2024 at 06:29:18PM +0000, Jonathan Cameron kirjoitti: > On Tue, 19 Mar 2024 10:28:31 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > On Tue, Mar 19, 2024 at 10:01 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Tue, Mar 19, 2024 at 4:28 PM David Lechner <dlechner@baylibre.com> wrote: ... > > > > + ret = device_property_match_property_string(dev, "adi,spi-mode", > > > > + ad7944_spi_modes, > > > > + ARRAY_SIZE(ad7944_spi_modes)); > > > > + if (ret < 0) { > > > > + if (ret != -EINVAL) > > > > + return dev_err_probe(dev, ret, > > > > + "getting adi,spi-mode property failed\n"); > > > > > > > - adc->spi_mode = ret; > > > > - } else { > > > > > > Actually we may even leave these unchanged > > > > > > > /* absence of adi,spi-mode property means default mode */ > > > > adc->spi_mode = AD7944_SPI_MODE_DEFAULT; > > > > + } else { > > > > + adc->spi_mode = ret; > > > > } > > > > > > ret = device_property_match_property_string(dev, "adi,spi-mode", > > > ad7944_spi_modes, > > > > > > ARRAY_SIZE(ad7944_spi_modes)); > > > if (ret >= 0) { > > > adc->spi_mode = ret; > > > } else if (ret != -EINVAL) { > > > return dev_err_probe(dev, ret, > > > "getting adi,spi-mode > > > property failed\n"); > > > } else { > > > /* absence of adi,spi-mode property means default mode */ > > > adc->spi_mode = AD7944_SPI_MODE_DEFAULT; > > > } > > > > > > But I can admit this is not an often used approach. > > > > > > > I think Jonathan prefers to have the error path first, so I would like > > to wait and see if he has an opinion here. > I do prefer error paths first. Thanks. Still the above can be refactored to have one line less ret = device_property_match_property_string(dev, "adi,spi-mode", ad7944_spi_modes, ARRAY_SIZE(ad7944_spi_modes)); if (ret == -EINVAL) { /* absence of adi,spi-mode property means default mode */ adc->spi_mode = AD7944_SPI_MODE_DEFAULT; } else if (ret < 0) { return dev_err_probe(dev, ret, "getting adi,spi-mode property failed\n"); } else { adc->spi_mode = ret; }
On Sat, 23 Mar 2024 22:44:37 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > Sat, Mar 23, 2024 at 06:29:18PM +0000, Jonathan Cameron kirjoitti: > > On Tue, 19 Mar 2024 10:28:31 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > On Tue, Mar 19, 2024 at 10:01 AM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Tue, Mar 19, 2024 at 4:28 PM David Lechner <dlechner@baylibre.com> wrote: > > ... > > > > > > + ret = device_property_match_property_string(dev, "adi,spi-mode", > > > > > + ad7944_spi_modes, > > > > > + ARRAY_SIZE(ad7944_spi_modes)); > > > > > + if (ret < 0) { > > > > > + if (ret != -EINVAL) > > > > > + return dev_err_probe(dev, ret, > > > > > + "getting adi,spi-mode property failed\n"); > > > > > > > > > - adc->spi_mode = ret; > > > > > - } else { > > > > > > > > Actually we may even leave these unchanged > > > > > > > > > /* absence of adi,spi-mode property means default mode */ > > > > > adc->spi_mode = AD7944_SPI_MODE_DEFAULT; > > > > > + } else { > > > > > + adc->spi_mode = ret; > > > > > } > > > > > > > > ret = device_property_match_property_string(dev, "adi,spi-mode", > > > > ad7944_spi_modes, > > > > > > > > ARRAY_SIZE(ad7944_spi_modes)); > > > > if (ret >= 0) { > > > > adc->spi_mode = ret; > > > > } else if (ret != -EINVAL) { > > > > return dev_err_probe(dev, ret, > > > > "getting adi,spi-mode > > > > property failed\n"); > > > > } else { > > > > /* absence of adi,spi-mode property means default mode */ > > > > adc->spi_mode = AD7944_SPI_MODE_DEFAULT; > > > > } > > > > > > > > But I can admit this is not an often used approach. > > > > > > > > > > I think Jonathan prefers to have the error path first, so I would like > > > to wait and see if he has an opinion here. > > I do prefer error paths first. Thanks. > > Still the above can be refactored to have one line less > > ret = device_property_match_property_string(dev, "adi,spi-mode", > ad7944_spi_modes, > ARRAY_SIZE(ad7944_spi_modes)); > if (ret == -EINVAL) { > /* absence of adi,spi-mode property means default mode */ > adc->spi_mode = AD7944_SPI_MODE_DEFAULT; > } else if (ret < 0) { > return dev_err_probe(dev, ret, "getting adi,spi-mode property failed\n"); > } else { > adc->spi_mode = ret; > } > True. I'll take a patch doing that, but I'm not going to tweak it again directly. Thanks, Jonathan
diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c index d5ec6b5a41c7..261a3f645fd8 100644 --- a/drivers/iio/adc/ad7944.c +++ b/drivers/iio/adc/ad7944.c @@ -366,7 +366,6 @@ static int ad7944_probe(struct spi_device *spi) struct ad7944_adc *adc; bool have_refin = false; struct regulator *ref; - const char *str_val; int ret; indio_dev = devm_iio_device_alloc(dev, sizeof(*adc)); @@ -382,16 +381,18 @@ static int ad7944_probe(struct spi_device *spi) adc->timing_spec = chip_info->timing_spec; - if (device_property_read_string(dev, "adi,spi-mode", &str_val) == 0) { - ret = sysfs_match_string(ad7944_spi_modes, str_val); - if (ret < 0) - return dev_err_probe(dev, -EINVAL, - "unsupported adi,spi-mode\n"); + ret = device_property_match_property_string(dev, "adi,spi-mode", + ad7944_spi_modes, + ARRAY_SIZE(ad7944_spi_modes)); + if (ret < 0) { + if (ret != -EINVAL) + return dev_err_probe(dev, ret, + "getting adi,spi-mode property failed\n"); - adc->spi_mode = ret; - } else { /* absence of adi,spi-mode property means default mode */ adc->spi_mode = AD7944_SPI_MODE_DEFAULT; + } else { + adc->spi_mode = ret; } if (adc->spi_mode == AD7944_SPI_MODE_CHAIN)
This simplifies the adi,spi-mode property parsing by using device_property_match_property_string() instead of two separate functions. Also, the error return value is now more informative in cases where there was problem parsing the property. Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: David Lechner <dlechner@baylibre.com> --- Changes in v2: - Reorder error path to avoid else statement - Link to v1: https://lore.kernel.org/r/20240318-ad7944-cleanups-v1-1-0cbb0349a14f@baylibre.com --- drivers/iio/adc/ad7944.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) --- base-commit: 1446d8ef48196409f811c25071b2cc510a49fc60 change-id: 20240318-ad7944-cleanups-9f95a7c598b6