2021-05-13 20:21:24

by Alexandru Ardelean

[permalink] [raw]
Subject: [PATCH v4 00/12] ad_sigma_delta: convert all drivers to device-managed

Well, for lack of a better title that's what this series does.
It merges Jonathan's patches from:
* https://lore.kernel.org/linux-iio/[email protected]/
Patch 3/3 was a polished a bit with my comments from that review and also
to use the devm_ad_sd_setup_buffer_and_trigger() function.
* https://lore.kernel.org/linux-iio/[email protected]/
Added only to base the conversion to devm_

The AD Sigma Delta family of ADC drivers share a lot of the logic in the
ad_sigma_delta lib-driver.

This set introduces a devm_ad_sd_setup_buffer_and_trigger() call, which
aims to replace the 'ad_sd_{setup,cleanup}_buffer_and_trigger()' pair.

This helps with converting the AD7780, AD7791, AD7793 and AD7192
drivers use be fully converted to device-managed functions.

Changelog v3 -> v4:
* https://lore.kernel.org/linux-iio/[email protected]/
* patch 'iio: adc: ad7192: handle zero Avdd regulator value'
is now 'iio: adc: ad7192: handle regulator voltage error first'
- now checking the regulator_voltage() return first for an error

Changelog v2 -> v3:
* https://lore.kernel.org/linux-iio/[email protected]/
* patch 'iio: adc: ad7192: handle zero Avdd regulator value as error'
is now 'iio: adc: ad7192: handle zero Avdd regulator value'
essentially just doing a simple 'if (voltage_uv >= 0)' check now

Changelog v1 -> v2:
* https://lore.kernel.org/linux-iio/[email protected]/
* add my S-o-b tags on all patches; with @deviqon.com email
- Note: I'm a little unsure about the correctness of these tags; there
are a few mixed-in, with Reviewed-by & Signed-off-by; I'm fine if
Jonathan tweaks these as needed;
* added patch 'iio: adc: ad7192: handle zero Avdd regulator value as error'
* all Fixes patches should be now at the beginning of the series

Alexandru Ardelean (8):
iio: adc: ad7192: handle regulator voltage error first
iio: adc: ad_sigma_delta: introduct
devm_ad_sd_setup_buffer_and_trigger()
iio: adc: ad7793: convert to device-managed functions
iio: adc: ad7791: convert to device-managed functions
iio: adc: ad7780: convert to device-managed functions
iio: adc: ad7192: use devm_clk_get_optional() for mclk
iio: adc: ad7192: convert to device-managed functions
iio: adc: ad_sigma_delta: remove
ad_sd_{setup,cleanup}_buffer_and_trigger()

Jonathan Cameron (4):
iio: adc: ad7124: Fix missbalanced regulator enable / disable on
error.
iio: adc: ad7124: Fix potential overflow due to non sequential channel
numbers
iio: adc: ad7192: Avoid disabling a clock that was never enabled.
iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop
remove()

drivers/iio/adc/ad7124.c | 84 +++++++++-------------
drivers/iio/adc/ad7192.c | 98 +++++++++++---------------
drivers/iio/adc/ad7780.c | 38 +++-------
drivers/iio/adc/ad7791.c | 44 ++++--------
drivers/iio/adc/ad7793.c | 53 ++++----------
drivers/iio/adc/ad_sigma_delta.c | 82 ++++++++-------------
include/linux/iio/adc/ad_sigma_delta.h | 4 +-
7 files changed, 144 insertions(+), 259 deletions(-)

--
2.31.1



2021-05-13 20:21:30

by Alexandru Ardelean

[permalink] [raw]
Subject: [PATCH v4 10/12] iio: adc: ad7192: convert to device-managed functions

With the devm_ad_sd_setup_buffer_and_trigger() helper, it's a bit easier
now to convert the probe of the AD7192 driver to use device-managed
functions.

The regulators and the mclk requires devm_add_action_or_reset() callbacks
though.

Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/adc/ad7192.c | 89 ++++++++++++++++------------------------
1 file changed, 36 insertions(+), 53 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 523cf3bc955b..ee8ed9481025 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -908,6 +908,16 @@ static int ad7192_channels_config(struct iio_dev *indio_dev)
return 0;
}

+static void ad7192_reg_disable(void *reg)
+{
+ regulator_disable(reg);
+}
+
+static void ad7192_clk_disable(void *clk)
+{
+ clk_disable_unprepare(clk);
+}
+
static int ad7192_probe(struct spi_device *spi)
{
struct ad7192_state *st;
@@ -937,33 +947,38 @@ static int ad7192_probe(struct spi_device *spi)
return ret;
}

+ ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->avdd);
+ if (ret)
+ return ret;
+
st->dvdd = devm_regulator_get(&spi->dev, "dvdd");
- if (IS_ERR(st->dvdd)) {
- ret = PTR_ERR(st->dvdd);
- goto error_disable_avdd;
- }
+ if (IS_ERR(st->dvdd))
+ return PTR_ERR(st->dvdd);

ret = regulator_enable(st->dvdd);
if (ret) {
dev_err(&spi->dev, "Failed to enable specified DVdd supply\n");
- goto error_disable_avdd;
+ return ret;
}

+ ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->dvdd);
+ if (ret)
+ return ret;
+
ret = regulator_get_voltage(st->avdd);
if (ret < 0) {
dev_err(&spi->dev, "Device tree error, reference voltage undefined\n");
- goto error_disable_avdd;
+ return ret;
}
st->int_vref_mv = ret / 1000;

- spi_set_drvdata(spi, indio_dev);
st->chip_info = of_device_get_match_data(&spi->dev);
indio_dev->name = st->chip_info->name;
indio_dev->modes = INDIO_DIRECT_MODE;

ret = ad7192_channels_config(indio_dev);
if (ret < 0)
- goto error_disable_dvdd;
+ return ret;

if (st->chip_info->chip_id == CHIPID_AD7195)
indio_dev->info = &ad7195_info;
@@ -972,17 +987,15 @@ static int ad7192_probe(struct spi_device *spi)

ad_sd_init(&st->sd, indio_dev, spi, &ad7192_sigma_delta_info);

- ret = ad_sd_setup_buffer_and_trigger(indio_dev);
+ ret = devm_ad_sd_setup_buffer_and_trigger(&spi->dev, indio_dev);
if (ret)
- goto error_disable_dvdd;
+ return ret;

st->fclk = AD7192_INT_FREQ_MHZ;

st->mclk = devm_clk_get_optional(&spi->dev, "mclk");
- if (IS_ERR(st->mclk)) {
- ret = PTR_ERR(st->mclk);
- goto error_remove_trigger;
- }
+ if (IS_ERR(st->mclk))
+ return PTR_ERR(st->mclk);

st->clock_sel = ad7192_of_clock_select(st);

@@ -990,55 +1003,26 @@ static int ad7192_probe(struct spi_device *spi)
st->clock_sel == AD7192_CLK_EXT_MCLK2) {
ret = clk_prepare_enable(st->mclk);
if (ret < 0)
- goto error_remove_trigger;
+ return ret;
+
+ ret = devm_add_action_or_reset(&spi->dev, ad7192_clk_disable,
+ st->mclk);
+ if (ret)
+ return ret;

st->fclk = clk_get_rate(st->mclk);
if (!ad7192_valid_external_frequency(st->fclk)) {
- ret = -EINVAL;
dev_err(&spi->dev,
"External clock frequency out of bounds\n");
- goto error_disable_clk;
+ return -EINVAL;
}
}

ret = ad7192_setup(st, spi->dev.of_node);
if (ret)
- goto error_disable_clk;
-
- ret = iio_device_register(indio_dev);
- if (ret < 0)
- goto error_disable_clk;
- return 0;
-
-error_disable_clk:
- if (st->clock_sel == AD7192_CLK_EXT_MCLK1_2 ||
- st->clock_sel == AD7192_CLK_EXT_MCLK2)
- clk_disable_unprepare(st->mclk);
-error_remove_trigger:
- ad_sd_cleanup_buffer_and_trigger(indio_dev);
-error_disable_dvdd:
- regulator_disable(st->dvdd);
-error_disable_avdd:
- regulator_disable(st->avdd);
-
- return ret;
-}
-
-static int ad7192_remove(struct spi_device *spi)
-{
- struct iio_dev *indio_dev = spi_get_drvdata(spi);
- struct ad7192_state *st = iio_priv(indio_dev);
-
- iio_device_unregister(indio_dev);
- if (st->clock_sel == AD7192_CLK_EXT_MCLK1_2 ||
- st->clock_sel == AD7192_CLK_EXT_MCLK2)
- clk_disable_unprepare(st->mclk);
- ad_sd_cleanup_buffer_and_trigger(indio_dev);
-
- regulator_disable(st->dvdd);
- regulator_disable(st->avdd);
+ return ret;

- return 0;
+ return devm_iio_device_register(&spi->dev, indio_dev);
}

static const struct of_device_id ad7192_of_match[] = {
@@ -1056,7 +1040,6 @@ static struct spi_driver ad7192_driver = {
.of_match_table = ad7192_of_match,
},
.probe = ad7192_probe,
- .remove = ad7192_remove,
};
module_spi_driver(ad7192_driver);

--
2.31.1


2021-05-13 20:21:50

by Alexandru Ardelean

[permalink] [raw]
Subject: [PATCH v4 01/12] iio: adc: ad7124: Fix missbalanced regulator enable / disable on error.

From: Jonathan Cameron <[email protected]>

If the devm_regulator_get() call succeeded but not the regulator_enable()
then regulator_disable() would be called on a regulator that was not
enabled.

Fix this by moving regulator enabling / disabling over to
devm_ management via devm_add_action_or_reset.

Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support")
Reviewed-by: Alexandru Ardelean <[email protected]>
Signed-off-by: Jonathan Cameron <[email protected]>
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/adc/ad7124.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 9d3952b4674f..437116a07cf1 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -850,6 +850,11 @@ static int ad7124_setup(struct ad7124_state *st)
return ret;
}

+static void ad7124_reg_disable(void *r)
+{
+ regulator_disable(r);
+}
+
static int ad7124_probe(struct spi_device *spi)
{
const struct ad7124_chip_info *info;
@@ -895,17 +900,20 @@ static int ad7124_probe(struct spi_device *spi)
ret = regulator_enable(st->vref[i]);
if (ret)
return ret;
+
+ ret = devm_add_action_or_reset(&spi->dev, ad7124_reg_disable,
+ st->vref[i]);
+ if (ret)
+ return ret;
}

st->mclk = devm_clk_get(&spi->dev, "mclk");
- if (IS_ERR(st->mclk)) {
- ret = PTR_ERR(st->mclk);
- goto error_regulator_disable;
- }
+ if (IS_ERR(st->mclk))
+ return PTR_ERR(st->mclk);

ret = clk_prepare_enable(st->mclk);
if (ret < 0)
- goto error_regulator_disable;
+ return ret;

ret = ad7124_soft_reset(st);
if (ret < 0)
@@ -935,11 +943,6 @@ static int ad7124_probe(struct spi_device *spi)
ad_sd_cleanup_buffer_and_trigger(indio_dev);
error_clk_disable_unprepare:
clk_disable_unprepare(st->mclk);
-error_regulator_disable:
- for (i = ARRAY_SIZE(st->vref) - 1; i >= 0; i--) {
- if (!IS_ERR_OR_NULL(st->vref[i]))
- regulator_disable(st->vref[i]);
- }

return ret;
}
@@ -948,17 +951,11 @@ static int ad7124_remove(struct spi_device *spi)
{
struct iio_dev *indio_dev = spi_get_drvdata(spi);
struct ad7124_state *st = iio_priv(indio_dev);
- int i;

iio_device_unregister(indio_dev);
ad_sd_cleanup_buffer_and_trigger(indio_dev);
clk_disable_unprepare(st->mclk);

- for (i = ARRAY_SIZE(st->vref) - 1; i >= 0; i--) {
- if (!IS_ERR_OR_NULL(st->vref[i]))
- regulator_disable(st->vref[i]);
- }
-
return 0;
}

--
2.31.1


2021-06-09 20:12:40

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v4 00/12] ad_sigma_delta: convert all drivers to device-managed

On Thu, 13 May 2021 15:07:40 +0300
Alexandru Ardelean <[email protected]> wrote:

> Well, for lack of a better title that's what this series does.
> It merges Jonathan's patches from:
> * https://lore.kernel.org/linux-iio/[email protected]/
> Patch 3/3 was a polished a bit with my comments from that review and also
> to use the devm_ad_sd_setup_buffer_and_trigger() function.
> * https://lore.kernel.org/linux-iio/[email protected]/
> Added only to base the conversion to devm_
>
> The AD Sigma Delta family of ADC drivers share a lot of the logic in the
> ad_sigma_delta lib-driver.
>
> This set introduces a devm_ad_sd_setup_buffer_and_trigger() call, which
> aims to replace the 'ad_sd_{setup,cleanup}_buffer_and_trigger()' pair.
>
> This helps with converting the AD7780, AD7791, AD7793 and AD7192
> drivers use be fully converted to device-managed functions.

Remainder of series applied to the togreg branch of iio.git and pushed out
as testing for 0-day to poke at it.

Thanks,

Jonathan

>
> Changelog v3 -> v4:
> * https://lore.kernel.org/linux-iio/[email protected]/
> * patch 'iio: adc: ad7192: handle zero Avdd regulator value'
> is now 'iio: adc: ad7192: handle regulator voltage error first'
> - now checking the regulator_voltage() return first for an error
>
> Changelog v2 -> v3:
> * https://lore.kernel.org/linux-iio/[email protected]/
> * patch 'iio: adc: ad7192: handle zero Avdd regulator value as error'
> is now 'iio: adc: ad7192: handle zero Avdd regulator value'
> essentially just doing a simple 'if (voltage_uv >= 0)' check now
>
> Changelog v1 -> v2:
> * https://lore.kernel.org/linux-iio/[email protected]/
> * add my S-o-b tags on all patches; with @deviqon.com email
> - Note: I'm a little unsure about the correctness of these tags; there
> are a few mixed-in, with Reviewed-by & Signed-off-by; I'm fine if
> Jonathan tweaks these as needed;
> * added patch 'iio: adc: ad7192: handle zero Avdd regulator value as error'
> * all Fixes patches should be now at the beginning of the series
>
> Alexandru Ardelean (8):
> iio: adc: ad7192: handle regulator voltage error first
> iio: adc: ad_sigma_delta: introduct
> devm_ad_sd_setup_buffer_and_trigger()
> iio: adc: ad7793: convert to device-managed functions
> iio: adc: ad7791: convert to device-managed functions
> iio: adc: ad7780: convert to device-managed functions
> iio: adc: ad7192: use devm_clk_get_optional() for mclk
> iio: adc: ad7192: convert to device-managed functions
> iio: adc: ad_sigma_delta: remove
> ad_sd_{setup,cleanup}_buffer_and_trigger()
>
> Jonathan Cameron (4):
> iio: adc: ad7124: Fix missbalanced regulator enable / disable on
> error.
> iio: adc: ad7124: Fix potential overflow due to non sequential channel
> numbers
> iio: adc: ad7192: Avoid disabling a clock that was never enabled.
> iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop
> remove()
>
> drivers/iio/adc/ad7124.c | 84 +++++++++-------------
> drivers/iio/adc/ad7192.c | 98 +++++++++++---------------
> drivers/iio/adc/ad7780.c | 38 +++-------
> drivers/iio/adc/ad7791.c | 44 ++++--------
> drivers/iio/adc/ad7793.c | 53 ++++----------
> drivers/iio/adc/ad_sigma_delta.c | 82 ++++++++-------------
> include/linux/iio/adc/ad_sigma_delta.h | 4 +-
> 7 files changed, 144 insertions(+), 259 deletions(-)
>