Re: [PATCH v3 2/2] iio: adc: Add TI ADS1100 and ADS1000

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 2 Mar 2023 05:20:38 -0800
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 3/2/23 05:16, Lars-Peter Clausen wrote:
> > On 3/1/23 23:49, Mike Looijmans wrote:  
> >>
> >>  
> >>> ...
> >>> ...
> >>>  
> >>>> +static int ads1100_runtime_suspend(struct device *dev)
> >>>> +{
> >>>> +    struct iio_dev *indio_dev = 
> >>>> i2c_get_clientdata(to_i2c_client(dev));
> >>>> +    struct ads1100_data *data = iio_priv(indio_dev);
> >>>> +
> >>>> +    ads1100_set_config_bits(data, ADS1100_CFG_SC, 
> >>>> ADS1100_SINGLESHOT);
> >>>> +    regulator_disable(data->reg_vdd);  
> >>> Wrong devm / non-devm ordering.  
> >>
> >> Don't understand your remark, can you explain further please?
> >>
> >> devm / non-devm ordering would be related to the "probe" function. As 
> >> far as I can tell, I'm not allocating resources after the devm calls. 
> >> And the "remove" is empty.  
> >
> > Strictly speaking we need to unregister the IIO device before 
> > disabling the regulator, otherwise there is a small window where the 
> > IIO device still exists, but doesn't work anymore. This is a very 
> > theoretical scenario though.
> >
> > You are lucky :) There is a new function 
> > `devm_regulator_get_enable()`[1], which will manage the 
> > regulator_disable() for you. Using that will also reduce the 
> > boilerplate in `probe()` a bit
> >
> > - Lars
> >
> > [1] https://lwn.net/Articles/904383/
> >  
> Sorry, just saw that Andy's comment was on the suspend() function, not 
> remove(). In that case there is of course no need for any devm things. 
> But still a good idea to use `devm_regulator_get_enable()` in probe for 
> the boiler plate.
> 
You can't because (annoyingly) devem_regulator_get_enable() doesn't
provide you access to the struct regulator that you need to be able
to turn it of for power management.
That case only works for the leave the power on all the time cases.

Jonathan