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 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 zero Avdd regulator value as error
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 | 94 ++++++++++++--------------
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, 145 insertions(+), 254 deletions(-)
--
2.31.1
From: Jonathan Cameron <[email protected]>
Channel numbering must start at 0 and then not have any holes, or
it is possible to overflow the available storage. Note this bug was
introduced as part of a fix to ensure we didn't rely on the ordering
of child nodes. So we need to support arbitrary ordering but they all
need to be there somewhere.
Note I hit this when using qemu to test the rest of this series.
Arguably this isn't the best fix, but it is probably the most minimal
option for backporting etc.
Fixes: d7857e4ee1ba6 ("iio: adc: ad7124: Fix DT channel configuration")
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 | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 437116a07cf1..a27db78ea13e 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -771,6 +771,13 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
if (ret)
goto err;
+ if (channel >= indio_dev->num_channels) {
+ dev_err(indio_dev->dev.parent,
+ "Channel index >= number of channels\n");
+ ret = -EINVAL;
+ goto err;
+ }
+
ret = of_property_read_u32_array(child, "diff-channels",
ain, 2);
if (ret)
--
2.31.1
This change fixes a corner-case, where the returned voltage is actually
zero. This is also what patch ab0afa65bbc7 ("staging: iio: adc: ad7192:
fail probe on get_voltage") was trying to do.
But as Jonathan pointed out, a zero-value would signal that the probe has
succeeded, putting the driver is a semi-initialized state.
Fixes: ab0afa65bbc7 ("staging: iio: adc: ad7192: fail probe on get_voltage")
Cc: Alexandru Tachici <[email protected]>
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/adc/ad7192.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index d3be67aa0522..79df54e0dc96 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -950,6 +950,11 @@ static int ad7192_probe(struct spi_device *spi)
}
voltage_uv = regulator_get_voltage(st->avdd);
+ if (voltage_uv == 0) {
+ ret = -EINVAL;
+ dev_err(&spi->dev, "Zero value provided for AVdd supply\n");
+ goto error_disable_avdd;
+ }
if (voltage_uv > 0) {
st->int_vref_mv = voltage_uv / 1000;
--
2.31.1
From: Jonathan Cameron <[email protected]>
Found by inspection.
If the internal clock source is being used, the driver doesn't
call clk_prepare_enable() and as such we should not call
clk_disable_unprepare()
Use the same condition to protect the disable path as is used
on the enable one. Note this will all get simplified when
the driver moves over to a full devm_ flow, but that would make
backporting the fix harder.
Fix obviously predates move out of staging, but backporting will
become more complex (and is unlikely to happen), hence that patch
is given in the fixes tag.
Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
Cc: Alexandru Tachici <[email protected]>
Reviewed-by: Alexandru Ardelean <[email protected]>
Signed-off-by: Jonathan Cameron <[email protected]>
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/adc/ad7192.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 2ed580521d81..d3be67aa0522 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -1014,7 +1014,9 @@ static int ad7192_probe(struct spi_device *spi)
return 0;
error_disable_clk:
- clk_disable_unprepare(st->mclk);
+ 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:
@@ -1031,7 +1033,9 @@ static int ad7192_remove(struct spi_device *spi)
struct ad7192_state *st = iio_priv(indio_dev);
iio_device_unregister(indio_dev);
- clk_disable_unprepare(st->mclk);
+ 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);
--
2.31.1
This is a version of ad_sd_setup_buffer_and_trigger() with all underlying
functions (that are used) being replaced with their device-managed
variants.
One thing to take care here is with {devm_}iio_trigger_alloc(), where both
functions take a parent-device object as the first parameter.
To make sure nothing quirky is happening, the devm_ad_sd_probe_trigger()
function is checking that the provided 'dev' reference is the same as the
one stored on the 'struct ad_sigma_delta' driver data.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/adc/ad_sigma_delta.c | 60 ++++++++++++++++++++++++++
include/linux/iio/adc/ad_sigma_delta.h | 3 ++
2 files changed, 63 insertions(+)
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 69b979331ccd..d5801a47be07 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -513,6 +513,46 @@ static int ad_sd_probe_trigger(struct iio_dev *indio_dev)
return ret;
}
+static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_dev)
+{
+ struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
+ int ret;
+
+ if (dev != &sigma_delta->spi->dev) {
+ dev_err(dev, "Trigger parent should be '%s', got '%s'\n",
+ dev_name(dev), dev_name(&sigma_delta->spi->dev));
+ return -EFAULT;
+ }
+
+ sigma_delta->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
+ iio_device_id(indio_dev));
+ if (sigma_delta->trig == NULL)
+ return -ENOMEM;
+
+ sigma_delta->trig->ops = &ad_sd_trigger_ops;
+ init_completion(&sigma_delta->completion);
+
+ sigma_delta->irq_dis = true;
+ ret = devm_request_irq(dev, sigma_delta->spi->irq,
+ ad_sd_data_rdy_trig_poll,
+ sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
+ indio_dev->name,
+ sigma_delta);
+ if (ret)
+ return ret;
+
+ iio_trigger_set_drvdata(sigma_delta->trig, sigma_delta);
+
+ ret = devm_iio_trigger_register(dev, sigma_delta->trig);
+ if (ret)
+ return ret;
+
+ /* select default trigger */
+ indio_dev->trig = iio_trigger_get(sigma_delta->trig);
+
+ return 0;
+}
+
static void ad_sd_remove_trigger(struct iio_dev *indio_dev)
{
struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
@@ -556,6 +596,26 @@ void ad_sd_cleanup_buffer_and_trigger(struct iio_dev *indio_dev)
}
EXPORT_SYMBOL_GPL(ad_sd_cleanup_buffer_and_trigger);
+/**
+ * devm_ad_sd_setup_buffer_and_trigger() - Device-managed buffer & trigger setup
+ * @dev: Device object to which to bind the life-time of the resources attached
+ * @indio_dev: The IIO device
+ */
+int devm_ad_sd_setup_buffer_and_trigger(struct device *dev, struct iio_dev *indio_dev)
+{
+ int ret;
+
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+ &iio_pollfunc_store_time,
+ &ad_sd_trigger_handler,
+ &ad_sd_buffer_setup_ops);
+ if (ret)
+ return ret;
+
+ return devm_ad_sd_probe_trigger(dev, indio_dev);
+}
+EXPORT_SYMBOL_GPL(devm_ad_sd_setup_buffer_and_trigger);
+
/**
* ad_sd_init() - Initializes a ad_sigma_delta struct
* @sigma_delta: The ad_sigma_delta device
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index 7199280d89ca..be81ad39fb7a 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -26,6 +26,7 @@ struct ad_sd_calib_data {
};
struct ad_sigma_delta;
+struct device;
struct iio_dev;
/**
@@ -135,6 +136,8 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
int ad_sd_setup_buffer_and_trigger(struct iio_dev *indio_dev);
void ad_sd_cleanup_buffer_and_trigger(struct iio_dev *indio_dev);
+int devm_ad_sd_setup_buffer_and_trigger(struct device *dev, struct iio_dev *indio_dev);
+
int ad_sd_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig);
#endif
--
2.31.1
With the devm_ad_sd_setup_buffer_and_trigger() helper, it's a bit easier
now to convert the probe of the AD7791 driver to use device-managed
functions.
Only the regulator disable requires a devm_add_action_or_reset() callback.
This change does that, cleaning up the driver a bit.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/adc/ad7791.c | 44 ++++++++++++----------------------------
1 file changed, 13 insertions(+), 31 deletions(-)
diff --git a/drivers/iio/adc/ad7791.c b/drivers/iio/adc/ad7791.c
index d57ad966e17c..cb579aa89f39 100644
--- a/drivers/iio/adc/ad7791.c
+++ b/drivers/iio/adc/ad7791.c
@@ -394,6 +394,11 @@ static int ad7791_setup(struct ad7791_state *st,
st->mode);
}
+static void ad7791_reg_disable(void *reg)
+{
+ regulator_disable(reg);
+}
+
static int ad7791_probe(struct spi_device *spi)
{
struct ad7791_platform_data *pdata = spi->dev.platform_data;
@@ -420,11 +425,13 @@ static int ad7791_probe(struct spi_device *spi)
if (ret)
return ret;
+ ret = devm_add_action_or_reset(&spi->dev, ad7791_reg_disable, st->reg);
+ if (ret)
+ return ret;
+
st->info = &ad7791_chip_infos[spi_get_device_id(spi)->driver_data];
ad_sd_init(&st->sd, indio_dev, spi, &ad7791_sigma_delta_info);
- spi_set_drvdata(spi, indio_dev);
-
indio_dev->name = spi_get_device_id(spi)->name;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->channels = st->info->channels;
@@ -434,39 +441,15 @@ static int ad7791_probe(struct spi_device *spi)
else
indio_dev->info = &ad7791_no_filter_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_reg;
+ return ret;
ret = ad7791_setup(st, pdata);
if (ret)
- goto error_remove_trigger;
-
- ret = iio_device_register(indio_dev);
- if (ret)
- goto error_remove_trigger;
-
- return 0;
-
-error_remove_trigger:
- ad_sd_cleanup_buffer_and_trigger(indio_dev);
-error_disable_reg:
- regulator_disable(st->reg);
-
- return ret;
-}
-
-static int ad7791_remove(struct spi_device *spi)
-{
- struct iio_dev *indio_dev = spi_get_drvdata(spi);
- struct ad7791_state *st = iio_priv(indio_dev);
-
- iio_device_unregister(indio_dev);
- ad_sd_cleanup_buffer_and_trigger(indio_dev);
-
- regulator_disable(st->reg);
+ return ret;
- return 0;
+ return devm_iio_device_register(&spi->dev, indio_dev);
}
static const struct spi_device_id ad7791_spi_ids[] = {
@@ -484,7 +467,6 @@ static struct spi_driver ad7791_driver = {
.name = "ad7791",
},
.probe = ad7791_probe,
- .remove = ad7791_remove,
.id_table = ad7791_spi_ids,
};
module_spi_driver(ad7791_driver);
--
2.31.1
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 | 93 ++++++++++++++++------------------------
1 file changed, 37 insertions(+), 56 deletions(-)
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 18e731f1471a..50696959c018 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,41 +947,44 @@ 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;
+
voltage_uv = regulator_get_voltage(st->avdd);
if (voltage_uv == 0) {
- ret = -EINVAL;
dev_err(&spi->dev, "Zero value provided for AVdd supply\n");
- goto error_disable_avdd;
+ return -EINVAL;
}
if (voltage_uv > 0) {
st->int_vref_mv = voltage_uv / 1000;
} else {
- ret = voltage_uv;
dev_err(&spi->dev, "Device tree error, reference voltage undefined\n");
- goto error_disable_avdd;
+ return voltage_uv;
}
- 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;
@@ -980,17 +993,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);
@@ -998,55 +1009,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[] = {
@@ -1064,7 +1046,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
The devm_clk_get_optional() helper returns NULL when devm_clk_get() returns
-ENOENT.
This makes things slightly cleaner. The added benefit is mostly cosmetic.
Also, a minor detail with this call, is that the reference for the parent
device is taken as `spi->dev` instead of `&st->sd.spi->dev` (which looks a
little quirky).
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/adc/ad7192.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 79df54e0dc96..18e731f1471a 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -326,7 +326,7 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
clock_sel = AD7192_CLK_INT;
/* use internal clock */
- if (PTR_ERR(st->mclk) == -ENOENT) {
+ if (st->mclk) {
if (of_property_read_bool(np, "adi,int-clock-output-enable"))
clock_sel = AD7192_CLK_INT_CO;
} else {
@@ -986,8 +986,8 @@ static int ad7192_probe(struct spi_device *spi)
st->fclk = AD7192_INT_FREQ_MHZ;
- st->mclk = devm_clk_get(&st->sd.spi->dev, "mclk");
- if (IS_ERR(st->mclk) && PTR_ERR(st->mclk) != -ENOENT) {
+ st->mclk = devm_clk_get_optional(&spi->dev, "mclk");
+ if (IS_ERR(st->mclk)) {
ret = PTR_ERR(st->mclk);
goto error_remove_trigger;
}
--
2.31.1
Since all AD Sigma-Delta drivers now use the
devm_ad_sd_setup_buffer_and_trigger() function, we can remove the old
ad_sd_{setup,cleanup}_buffer_and_trigger() functions.
This way we can discourage new drivers that use the ad_sigma_delta
lib-driver to use these (older functions).
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/adc/ad_sigma_delta.c | 86 --------------------------
include/linux/iio/adc/ad_sigma_delta.h | 3 -
2 files changed, 89 deletions(-)
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index d5801a47be07..1d652d9b2f5c 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -470,49 +470,6 @@ EXPORT_SYMBOL_GPL(ad_sd_validate_trigger);
static const struct iio_trigger_ops ad_sd_trigger_ops = {
};
-static int ad_sd_probe_trigger(struct iio_dev *indio_dev)
-{
- struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
- int ret;
-
- sigma_delta->trig = iio_trigger_alloc(&sigma_delta->spi->dev,
- "%s-dev%d", indio_dev->name,
- iio_device_id(indio_dev));
- if (sigma_delta->trig == NULL) {
- ret = -ENOMEM;
- goto error_ret;
- }
- sigma_delta->trig->ops = &ad_sd_trigger_ops;
- init_completion(&sigma_delta->completion);
-
- sigma_delta->irq_dis = true;
- ret = request_irq(sigma_delta->spi->irq,
- ad_sd_data_rdy_trig_poll,
- sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
- indio_dev->name,
- sigma_delta);
- if (ret)
- goto error_free_trig;
-
- iio_trigger_set_drvdata(sigma_delta->trig, sigma_delta);
-
- ret = iio_trigger_register(sigma_delta->trig);
- if (ret)
- goto error_free_irq;
-
- /* select default trigger */
- indio_dev->trig = iio_trigger_get(sigma_delta->trig);
-
- return 0;
-
-error_free_irq:
- free_irq(sigma_delta->spi->irq, sigma_delta);
-error_free_trig:
- iio_trigger_free(sigma_delta->trig);
-error_ret:
- return ret;
-}
-
static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_dev)
{
struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
@@ -553,49 +510,6 @@ static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_de
return 0;
}
-static void ad_sd_remove_trigger(struct iio_dev *indio_dev)
-{
- struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
-
- iio_trigger_unregister(sigma_delta->trig);
- free_irq(sigma_delta->spi->irq, sigma_delta);
- iio_trigger_free(sigma_delta->trig);
-}
-
-/**
- * ad_sd_setup_buffer_and_trigger() -
- * @indio_dev: The IIO device
- */
-int ad_sd_setup_buffer_and_trigger(struct iio_dev *indio_dev)
-{
- int ret;
-
- ret = iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
- &ad_sd_trigger_handler, &ad_sd_buffer_setup_ops);
- if (ret)
- return ret;
-
- ret = ad_sd_probe_trigger(indio_dev);
- if (ret) {
- iio_triggered_buffer_cleanup(indio_dev);
- return ret;
- }
-
- return 0;
-}
-EXPORT_SYMBOL_GPL(ad_sd_setup_buffer_and_trigger);
-
-/**
- * ad_sd_cleanup_buffer_and_trigger() -
- * @indio_dev: The IIO device
- */
-void ad_sd_cleanup_buffer_and_trigger(struct iio_dev *indio_dev)
-{
- ad_sd_remove_trigger(indio_dev);
- iio_triggered_buffer_cleanup(indio_dev);
-}
-EXPORT_SYMBOL_GPL(ad_sd_cleanup_buffer_and_trigger);
-
/**
* devm_ad_sd_setup_buffer_and_trigger() - Device-managed buffer & trigger setup
* @dev: Device object to which to bind the life-time of the resources attached
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index be81ad39fb7a..c525fd51652f 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -133,9 +133,6 @@ int ad_sd_calibrate_all(struct ad_sigma_delta *sigma_delta,
int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
struct spi_device *spi, const struct ad_sigma_delta_info *info);
-int ad_sd_setup_buffer_and_trigger(struct iio_dev *indio_dev);
-void ad_sd_cleanup_buffer_and_trigger(struct iio_dev *indio_dev);
-
int devm_ad_sd_setup_buffer_and_trigger(struct device *dev, struct iio_dev *indio_dev);
int ad_sd_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig);
--
2.31.1
With the devm_ad_sd_setup_buffer_and_trigger() helper, it's a bit easier
now to convert the probe of the AD7780 driver to use device-managed
functions.
Only the regulator disable requires a devm_add_action_or_reset() callback.
This change does that, cleaning up the driver a bit.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/adc/ad7780.c | 38 ++++++++++----------------------------
1 file changed, 10 insertions(+), 28 deletions(-)
diff --git a/drivers/iio/adc/ad7780.c b/drivers/iio/adc/ad7780.c
index 42e7e8e595d1..42bb952f4738 100644
--- a/drivers/iio/adc/ad7780.c
+++ b/drivers/iio/adc/ad7780.c
@@ -300,6 +300,11 @@ static int ad7780_init_gpios(struct device *dev, struct ad7780_state *st)
return 0;
}
+static void ad7780_reg_disable(void *reg)
+{
+ regulator_disable(reg);
+}
+
static int ad7780_probe(struct spi_device *spi)
{
struct ad7780_state *st;
@@ -318,8 +323,6 @@ static int ad7780_probe(struct spi_device *spi)
st->chip_info =
&ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
- spi_set_drvdata(spi, indio_dev);
-
indio_dev->name = spi_get_device_id(spi)->name;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->channels = &st->chip_info->channel;
@@ -340,35 +343,15 @@ static int ad7780_probe(struct spi_device *spi)
return ret;
}
- ret = ad_sd_setup_buffer_and_trigger(indio_dev);
+ ret = devm_add_action_or_reset(&spi->dev, ad7780_reg_disable, st->reg);
if (ret)
- goto error_disable_reg;
+ return ret;
- ret = iio_device_register(indio_dev);
+ ret = devm_ad_sd_setup_buffer_and_trigger(&spi->dev, indio_dev);
if (ret)
- goto error_cleanup_buffer_and_trigger;
-
- return 0;
-
-error_cleanup_buffer_and_trigger:
- ad_sd_cleanup_buffer_and_trigger(indio_dev);
-error_disable_reg:
- regulator_disable(st->reg);
-
- return ret;
-}
-
-static int ad7780_remove(struct spi_device *spi)
-{
- struct iio_dev *indio_dev = spi_get_drvdata(spi);
- struct ad7780_state *st = iio_priv(indio_dev);
-
- iio_device_unregister(indio_dev);
- ad_sd_cleanup_buffer_and_trigger(indio_dev);
-
- regulator_disable(st->reg);
+ return ret;
- return 0;
+ return devm_iio_device_register(&spi->dev, indio_dev);
}
static const struct spi_device_id ad7780_id[] = {
@@ -385,7 +368,6 @@ static struct spi_driver ad7780_driver = {
.name = "ad7780",
},
.probe = ad7780_probe,
- .remove = ad7780_remove,
.id_table = ad7780_id,
};
module_spi_driver(ad7780_driver);
--
2.31.1
With the devm_ad_sd_setup_buffer_and_trigger() helper, it's a bit easier
now to convert the probe of the AD7793 driver to use device-managed
functions.
Only the regulator disable requires a devm_add_action_or_reset() callback.
This change does that, cleaning up the driver a bit.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/adc/ad7793.c | 53 ++++++++++++----------------------------
1 file changed, 15 insertions(+), 38 deletions(-)
diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c
index 5e980a06258e..5dab2e5b5bac 100644
--- a/drivers/iio/adc/ad7793.c
+++ b/drivers/iio/adc/ad7793.c
@@ -768,6 +768,11 @@ static const struct ad7793_chip_info ad7793_chip_info_tbl[] = {
},
};
+static void ad7793_reg_disable(void *reg)
+{
+ regulator_disable(reg);
+}
+
static int ad7793_probe(struct spi_device *spi)
{
const struct ad7793_platform_data *pdata = spi->dev.platform_data;
@@ -802,11 +807,13 @@ static int ad7793_probe(struct spi_device *spi)
if (ret)
return ret;
+ ret = devm_add_action_or_reset(&spi->dev, ad7793_reg_disable, st->reg);
+ if (ret)
+ return ret;
+
vref_mv = regulator_get_voltage(st->reg);
- if (vref_mv < 0) {
- ret = vref_mv;
- goto error_disable_reg;
- }
+ if (vref_mv < 0)
+ return vref_mv;
vref_mv /= 1000;
} else {
@@ -816,50 +823,21 @@ static int ad7793_probe(struct spi_device *spi)
st->chip_info =
&ad7793_chip_info_tbl[spi_get_device_id(spi)->driver_data];
- spi_set_drvdata(spi, indio_dev);
-
indio_dev->name = spi_get_device_id(spi)->name;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->channels = st->chip_info->channels;
indio_dev->num_channels = st->chip_info->num_channels;
indio_dev->info = st->chip_info->iio_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_reg;
+ return ret;
ret = ad7793_setup(indio_dev, pdata, vref_mv);
if (ret)
- goto error_remove_trigger;
-
- ret = iio_device_register(indio_dev);
- if (ret)
- goto error_remove_trigger;
-
- return 0;
-
-error_remove_trigger:
- ad_sd_cleanup_buffer_and_trigger(indio_dev);
-error_disable_reg:
- if (pdata->refsel != AD7793_REFSEL_INTERNAL)
- regulator_disable(st->reg);
-
- return ret;
-}
-
-static int ad7793_remove(struct spi_device *spi)
-{
- const struct ad7793_platform_data *pdata = spi->dev.platform_data;
- struct iio_dev *indio_dev = spi_get_drvdata(spi);
- struct ad7793_state *st = iio_priv(indio_dev);
-
- iio_device_unregister(indio_dev);
- ad_sd_cleanup_buffer_and_trigger(indio_dev);
-
- if (pdata->refsel != AD7793_REFSEL_INTERNAL)
- regulator_disable(st->reg);
+ return ret;
- return 0;
+ return devm_iio_device_register(&spi->dev, indio_dev);
}
static const struct spi_device_id ad7793_id[] = {
@@ -881,7 +859,6 @@ static struct spi_driver ad7793_driver = {
.name = "ad7793",
},
.probe = ad7793_probe,
- .remove = ad7793_remove,
.id_table = ad7793_id,
};
module_spi_driver(ad7793_driver);
--
2.31.1
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
From: Jonathan Cameron <[email protected]>
As not many steps were not already devm_ managed, use
devm_add_action_or_reset() to handle the rest.
This also uses the new devm_ad_sd_setup_buffer_and_trigger() function.
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 | 48 +++++++++++++---------------------------
1 file changed, 15 insertions(+), 33 deletions(-)
diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index a27db78ea13e..e45c600fccc0 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -862,6 +862,11 @@ static void ad7124_reg_disable(void *r)
regulator_disable(r);
}
+static void ad7124_clk_disable(void *c)
+{
+ clk_disable_unprepare(c);
+}
+
static int ad7124_probe(struct spi_device *spi)
{
const struct ad7124_chip_info *info;
@@ -883,8 +888,6 @@ static int ad7124_probe(struct spi_device *spi)
ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info);
- spi_set_drvdata(spi, indio_dev);
-
indio_dev->name = st->chip_info->name;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &ad7124_info;
@@ -922,48 +925,28 @@ static int ad7124_probe(struct spi_device *spi)
if (ret < 0)
return ret;
+ ret = devm_add_action_or_reset(&spi->dev, ad7124_clk_disable, st->mclk);
+ if (ret)
+ return ret;
+
ret = ad7124_soft_reset(st);
if (ret < 0)
- goto error_clk_disable_unprepare;
+ return ret;
ret = ad7124_check_chip_id(st);
if (ret)
- goto error_clk_disable_unprepare;
+ return ret;
ret = ad7124_setup(st);
if (ret < 0)
- goto error_clk_disable_unprepare;
+ return ret;
- ret = ad_sd_setup_buffer_and_trigger(indio_dev);
+ ret = devm_ad_sd_setup_buffer_and_trigger(&spi->dev, indio_dev);
if (ret < 0)
- goto error_clk_disable_unprepare;
-
- ret = iio_device_register(indio_dev);
- if (ret < 0) {
- dev_err(&spi->dev, "Failed to register iio device\n");
- goto error_remove_trigger;
- }
-
- return 0;
-
-error_remove_trigger:
- ad_sd_cleanup_buffer_and_trigger(indio_dev);
-error_clk_disable_unprepare:
- clk_disable_unprepare(st->mclk);
-
- return ret;
-}
-
-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);
+ return ret;
- iio_device_unregister(indio_dev);
- ad_sd_cleanup_buffer_and_trigger(indio_dev);
- clk_disable_unprepare(st->mclk);
+ return devm_iio_device_register(&spi->dev, indio_dev);
- return 0;
}
static const struct of_device_id ad7124_of_match[] = {
@@ -981,7 +964,6 @@ static struct spi_driver ad71124_driver = {
.of_match_table = ad7124_of_match,
},
.probe = ad7124_probe,
- .remove = ad7124_remove,
};
module_spi_driver(ad71124_driver);
--
2.31.1
On Tue, 11 May 2021 10:18:23 +0300
Alexandru Ardelean <[email protected]> wrote:
> This change fixes a corner-case, where the returned voltage is actually
> zero. This is also what patch ab0afa65bbc7 ("staging: iio: adc: ad7192:
> fail probe on get_voltage") was trying to do.
>
> But as Jonathan pointed out, a zero-value would signal that the probe has
> succeeded, putting the driver is a semi-initialized state.
>
> Fixes: ab0afa65bbc7 ("staging: iio: adc: ad7192: fail probe on get_voltage")
> Cc: Alexandru Tachici <[email protected]>
> Signed-off-by: Alexandru Ardelean <[email protected]>
Given that voltage_uv == 1 would result in a situation no worse than
for voltage_uv == 0 perhaps we should just change the following condition to
if (voltage_uv >= 0) ?
Jonathan
> ---
> drivers/iio/adc/ad7192.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index d3be67aa0522..79df54e0dc96 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -950,6 +950,11 @@ static int ad7192_probe(struct spi_device *spi)
> }
>
> voltage_uv = regulator_get_voltage(st->avdd);
> + if (voltage_uv == 0) {
> + ret = -EINVAL;
> + dev_err(&spi->dev, "Zero value provided for AVdd supply\n");
> + goto error_disable_avdd;
> + }
>
> if (voltage_uv > 0) {
> st->int_vref_mv = voltage_uv / 1000;
On Tue, 11 May 2021 at 17:12, Jonathan Cameron <[email protected]> wrote:
>
> On Tue, 11 May 2021 10:18:23 +0300
> Alexandru Ardelean <[email protected]> wrote:
>
> > This change fixes a corner-case, where the returned voltage is actually
> > zero. This is also what patch ab0afa65bbc7 ("staging: iio: adc: ad7192:
> > fail probe on get_voltage") was trying to do.
> >
> > But as Jonathan pointed out, a zero-value would signal that the probe has
> > succeeded, putting the driver is a semi-initialized state.
> >
> > Fixes: ab0afa65bbc7 ("staging: iio: adc: ad7192: fail probe on get_voltage")
> > Cc: Alexandru Tachici <[email protected]>
> > Signed-off-by: Alexandru Ardelean <[email protected]>
>
> Given that voltage_uv == 1 would result in a situation no worse than
> for voltage_uv == 0 perhaps we should just change the following condition to
>
> if (voltage_uv >= 0) ?
hmmm, you're right;
i think had some narrow vision about this;
will send a v3
>
> Jonathan
>
> > ---
> > drivers/iio/adc/ad7192.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> > index d3be67aa0522..79df54e0dc96 100644
> > --- a/drivers/iio/adc/ad7192.c
> > +++ b/drivers/iio/adc/ad7192.c
> > @@ -950,6 +950,11 @@ static int ad7192_probe(struct spi_device *spi)
> > }
> >
> > voltage_uv = regulator_get_voltage(st->avdd);
> > + if (voltage_uv == 0) {
> > + ret = -EINVAL;
> > + dev_err(&spi->dev, "Zero value provided for AVdd supply\n");
> > + goto error_disable_avdd;
> > + }
> >
> > if (voltage_uv > 0) {
> > st->int_vref_mv = voltage_uv / 1000;
>