All resources that the driver needs have managed API now. Switch to
using them to make code clearer and drop ti_ads7950_remove().
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/iio/adc/ti-ads7950.c | 98 +++++++++++++++---------------------
1 file changed, 40 insertions(+), 58 deletions(-)
diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index d31397f37ec4..1c53e000bdcc 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -528,19 +528,26 @@ static int ti_ads7950_init_hw(struct ti_ads7950_state *st)
return 0;
}
+static void ti_ads7950_power_off(void *data)
+{
+ struct ti_ads7950_state *st = data;
+
+ regulator_disable(st->reg);
+}
+
static int ti_ads7950_probe(struct spi_device *spi)
{
struct ti_ads7950_state *st;
struct iio_dev *indio_dev;
const struct ti_ads7950_chip_info *info;
- int ret;
+ int error;
spi->bits_per_word = 16;
spi->mode |= SPI_CS_WORD;
- ret = spi_setup(spi);
- if (ret < 0) {
+ error = spi_setup(spi);
+ if (error) {
dev_err(&spi->dev, "Error in spi setup\n");
- return ret;
+ return error;
}
indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
@@ -598,36 +605,36 @@ static int ti_ads7950_probe(struct spi_device *spi)
mutex_init(&st->slock);
st->reg = devm_regulator_get(&spi->dev, "vref");
- if (IS_ERR(st->reg)) {
- ret = dev_err_probe(&spi->dev, PTR_ERR(st->reg),
+ error = PTR_ERR_OR_ZERO(st->reg);
+ if (error)
+ return dev_err_probe(&spi->dev, error,
"Failed to get regulator \"vref\"\n");
- goto error_destroy_mutex;
- }
- ret = regulator_enable(st->reg);
- if (ret) {
- dev_err(&spi->dev, "Failed to enable regulator \"vref\"\n");
- goto error_destroy_mutex;
- }
+ error = regulator_enable(st->reg);
+ if (error)
+ return dev_err_probe(&spi->dev, error,
+ "Failed to enable regulator \"vref\"\n");
- ret = iio_triggered_buffer_setup(indio_dev, NULL,
- &ti_ads7950_trigger_handler, NULL);
- if (ret) {
- dev_err(&spi->dev, "Failed to setup triggered buffer\n");
- goto error_disable_reg;
- }
+ error = devm_add_action_or_reset(&spi->dev, ti_ads7950_power_off, st);
+ if (error)
+ return error;
- ret = ti_ads7950_init_hw(st);
- if (ret) {
- dev_err(&spi->dev, "Failed to init adc chip\n");
- goto error_cleanup_ring;
- }
+ error = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, NULL,
+ &ti_ads7950_trigger_handler,
+ NULL);
+ if (error)
+ return dev_err_probe(&spi->dev, error,
+ "Failed to setup triggered buffer\n");
- ret = iio_device_register(indio_dev);
- if (ret) {
- dev_err(&spi->dev, "Failed to register iio device\n");
- goto error_cleanup_ring;
- }
+ error = ti_ads7950_init_hw(st);
+ if (error)
+ return dev_err_probe(&spi->dev, error,
+ "Failed to init adc chip\n");
+
+ error = devm_iio_device_register(&spi->dev, indio_dev);
+ if (error)
+ return dev_err_probe(&spi->dev, error,
+ "Failed to register iio device\n");
/* Add GPIO chip */
st->chip.label = dev_name(&st->spi->dev);
@@ -642,36 +649,12 @@ static int ti_ads7950_probe(struct spi_device *spi)
st->chip.get = ti_ads7950_get;
st->chip.set = ti_ads7950_set;
- ret = gpiochip_add_data(&st->chip, st);
- if (ret) {
- dev_err(&spi->dev, "Failed to init GPIOs\n");
- goto error_iio_device;
- }
+ error = devm_gpiochip_add_data(&spi->dev, &st->chip, st);
+ if (error)
+ return dev_err_probe(&spi->dev, error,
+ "Failed to init GPIOs\n");
return 0;
-
-error_iio_device:
- iio_device_unregister(indio_dev);
-error_cleanup_ring:
- iio_triggered_buffer_cleanup(indio_dev);
-error_disable_reg:
- regulator_disable(st->reg);
-error_destroy_mutex:
- mutex_destroy(&st->slock);
-
- return ret;
-}
-
-static void ti_ads7950_remove(struct spi_device *spi)
-{
- struct iio_dev *indio_dev = spi_get_drvdata(spi);
- struct ti_ads7950_state *st = iio_priv(indio_dev);
-
- gpiochip_remove(&st->chip);
- iio_device_unregister(indio_dev);
- iio_triggered_buffer_cleanup(indio_dev);
- regulator_disable(st->reg);
- mutex_destroy(&st->slock);
}
static const struct spi_device_id ti_ads7950_id[] = {
@@ -714,7 +697,6 @@ static struct spi_driver ti_ads7950_driver = {
.of_match_table = ads7950_of_table,
},
.probe = ti_ads7950_probe,
- .remove = ti_ads7950_remove,
.id_table = ti_ads7950_id,
};
module_spi_driver(ti_ads7950_driver);
--
2.53.0.335.g19a08e0c02-googOn Wed, 18 Feb 2026 18:29:28 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> All resources that the driver needs have managed API now. Switch to
> using them to make code clearer and drop ti_ads7950_remove().
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Hi Dmitry
One additional comment from me.
> static int ti_ads7950_probe(struct spi_device *spi)
> {
> indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> @@ -598,36 +605,36 @@ static int ti_ads7950_probe(struct spi_device *spi)
> mutex_init(&st->slock);
>
> st->reg = devm_regulator_get(&spi->dev, "vref");
> - if (IS_ERR(st->reg)) {
> - ret = dev_err_probe(&spi->dev, PTR_ERR(st->reg),
> + error = PTR_ERR_OR_ZERO(st->reg);
To me this reads worse than original IS_ERR() / PTR_ERR() pair.
> + if (error)
> + return dev_err_probe(&spi->dev, error,
> "Failed to get regulator \"vref\"\n");
> - goto error_destroy_mutex;
> - }
>
> - ret = regulator_enable(st->reg);
> - if (ret) {
> - dev_err(&spi->dev, "Failed to enable regulator \"vref\"\n");
> - goto error_destroy_mutex;
> - }
> + error = regulator_enable(st->reg);
> + if (error)
> + return dev_err_probe(&spi->dev, error,
> + "Failed to enable regulator \"vref\"\n");
On Sun, Feb 22, 2026 at 02:09:23PM +0000, Jonathan Cameron wrote:
> On Wed, 18 Feb 2026 18:29:28 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> > All resources that the driver needs have managed API now. Switch to
> > using them to make code clearer and drop ti_ads7950_remove().
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Hi Dmitry
>
> One additional comment from me.
>
> > static int ti_ads7950_probe(struct spi_device *spi)
> > {
>
> > indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > @@ -598,36 +605,36 @@ static int ti_ads7950_probe(struct spi_device *spi)
> > mutex_init(&st->slock);
> >
> > st->reg = devm_regulator_get(&spi->dev, "vref");
> > - if (IS_ERR(st->reg)) {
> > - ret = dev_err_probe(&spi->dev, PTR_ERR(st->reg),
> > + error = PTR_ERR_OR_ZERO(st->reg);
>
> To me this reads worse than original IS_ERR() / PTR_ERR() pair.
OK, I'll keep that in mind. It is no longer there anyways after
converting to devm_regulator_get_enable_read_voltage() that David
suggested.
Thanks.
--
Dmitry
On 2/18/26 8:29 PM, Dmitry Torokhov wrote:
> All resources that the driver needs have managed API now. Switch to
> using them to make code clearer and drop ti_ads7950_remove().
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/iio/adc/ti-ads7950.c | 98 +++++++++++++++---------------------
> 1 file changed, 40 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index d31397f37ec4..1c53e000bdcc 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -528,19 +528,26 @@ static int ti_ads7950_init_hw(struct ti_ads7950_state *st)
> return 0;
> }
>
> +static void ti_ads7950_power_off(void *data)
> +{
> + struct ti_ads7950_state *st = data;
> +
> + regulator_disable(st->reg);
> +}
> +
For the regulator part, we can simplify this even more.
The part where we call regulator_get_voltage(st->reg); in
ti_ads7950_get_range() doesn't actually need to be done there.
It was something that I just naively copied from another driver.
(This was my first IIO driver after all!)
Instead, we can use devm_regulator_get_enable_read_voltage()
in the probe function and just store the voltage in
struct ti_ads7950_state.
I would do this in a separate patch first, then the rest of of
the devm stuff after that.
> static int ti_ads7950_probe(struct spi_device *spi)
> {
> struct ti_ads7950_state *st;
> struct iio_dev *indio_dev;
> const struct ti_ads7950_chip_info *info;
> - int ret;
> + int error;
As in the other patches, please do not rename ret. It is adding noise
in the diff. (And I like ret better anyway.)
On Wed, Feb 18, 2026 at 06:29:28PM -0800, Dmitry Torokhov wrote:
> All resources that the driver needs have managed API now. Switch to
> using them to make code clearer and drop ti_ads7950_remove().
...
> static int ti_ads7950_probe(struct spi_device *spi)
> {
> struct ti_ads7950_state *st;
> struct iio_dev *indio_dev;
> const struct ti_ads7950_chip_info *info;
> - int ret;
> + int error;
Unrelated change.
...
> spi->bits_per_word = 16;
> spi->mode |= SPI_CS_WORD;
> - ret = spi_setup(spi);
> - if (ret < 0) {
> + error = spi_setup(spi);
> + if (error) {
> dev_err(&spi->dev, "Error in spi setup\n");
> - return ret;
> + return error;
> }
Ditto.
And since there is already dev_err_probe() in use, I would expect this
also be converted.
Would be also nice to use
struct device *dev = &spi->dev;
to make less LoCs and/or make them shorter.
--
With Best Regards,
Andy Shevchenko
On Thu, Feb 19, 2026 at 09:59:35AM +0200, Andy Shevchenko wrote:
> On Wed, Feb 18, 2026 at 06:29:28PM -0800, Dmitry Torokhov wrote:
> > All resources that the driver needs have managed API now. Switch to
> > using them to make code clearer and drop ti_ads7950_remove().
>
> ...
>
> > static int ti_ads7950_probe(struct spi_device *spi)
> > {
> > struct ti_ads7950_state *st;
> > struct iio_dev *indio_dev;
> > const struct ti_ads7950_chip_info *info;
> > - int ret;
> > + int error;
>
> Unrelated change.
>
> ...
>
> > spi->bits_per_word = 16;
> > spi->mode |= SPI_CS_WORD;
> > - ret = spi_setup(spi);
> > - if (ret < 0) {
> > + error = spi_setup(spi);
> > + if (error) {
> > dev_err(&spi->dev, "Error in spi setup\n");
> > - return ret;
> > + return error;
> > }
>
> Ditto.
>
> And since there is already dev_err_probe() in use, I would expect this
> also be converted.
But that would be unrelated change ;)
>
> Would be also nice to use
>
> struct device *dev = &spi->dev;
>
> to make less LoCs and/or make them shorter.
I actually dislike introducing such temporaries in majority of the
cases. It makes it hard to follow what device we are dealing with and
does not make the code significantly shorter.
Thanks.
--
Dmitry
On Fri, Feb 20, 2026 at 04:09:47PM -0800, Dmitry Torokhov wrote:
> On Thu, Feb 19, 2026 at 09:59:35AM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 18, 2026 at 06:29:28PM -0800, Dmitry Torokhov wrote:
...
> > > + if (error) {
> > > dev_err(&spi->dev, "Error in spi setup\n");
> > > - return ret;
> > > + return error;
> > > }
> >
> > And since there is already dev_err_probe() in use, I would expect this
> > also be converted.
>
> But that would be unrelated change ;)
I never told that this should be done here.
...
> > Would be also nice to use
> >
> > struct device *dev = &spi->dev;
> >
> > to make less LoCs and/or make them shorter.
>
> I actually dislike introducing such temporaries in majority of the
> cases. It makes it hard to follow what device we are dealing with and
> does not make the code significantly shorter.
May be, but my experience is telling me different story.
So we have a disagreement here, I leave it to Jonathan
to mediate.
--
With Best Regards,
Andy Shevchenko
On Sun, 22 Feb 2026 21:12:17 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> On Fri, Feb 20, 2026 at 04:09:47PM -0800, Dmitry Torokhov wrote:
> > On Thu, Feb 19, 2026 at 09:59:35AM +0200, Andy Shevchenko wrote:
> > > On Wed, Feb 18, 2026 at 06:29:28PM -0800, Dmitry Torokhov wrote:
>
> ...
>
> > > > + if (error) {
> > > > dev_err(&spi->dev, "Error in spi setup\n");
> > > > - return ret;
> > > > + return error;
> > > > }
> > >
> > > And since there is already dev_err_probe() in use, I would expect this
> > > also be converted.
> >
> > But that would be unrelated change ;)
>
> I never told that this should be done here.
>
> ...
>
> > > Would be also nice to use
> > >
> > > struct device *dev = &spi->dev;
> > >
> > > to make less LoCs and/or make them shorter.
> >
> > I actually dislike introducing such temporaries in majority of the
> > cases. It makes it hard to follow what device we are dealing with and
> > does not make the code significantly shorter.
>
> May be, but my experience is telling me different story.
> So we have a disagreement here, I leave it to Jonathan
> to mediate.
>
When there are multiple struct device isntances that we access
in the driver I fully agree with Dmitry that it is helpful to
fully enumerate them. However, for IIO drivers we keep the
one buried in struct iio_dev fairly well hidden so the confusion
"opportunity" doesn't tend to occur. As such I tend to come down just
on the "have a local variable" side of things.
Jonathan
© 2016 - 2026 Red Hat, Inc.