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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 6 Mar 2023 12:21:44 +0100
Mike Looijmans <mike.looijmans@topic.nl> wrote:

> Met vriendelijke groet / kind regards,
> 
> Mike Looijmans
> System Expert
> 
> 
> TOPIC Embedded Products B.V.
> Materiaalweg 4, 5681 RJ Best
> The Netherlands
> 
> T: +31 (0) 499 33 69 69
> E: mike.looijmans@topicproducts.com
> W: www.topic.nl
> 
> Please consider the environment before printing this e-mail
> On 04-03-2023 18:57, Jonathan Cameron wrote:
> > On Tue, 28 Feb 2023 07:31:51 +0100
> > Mike Looijmans <mike.looijmans@topic.nl> wrote:
> >  
> >> The ADS1100 is a 16-bit ADC (at 8 samples per second).
> >> The ADS1000 is similar, but has a fixed data rate.
> >>
> >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>  
> > Hi Mike,
> >
> > A few minor things + one request for a test as trying to chase a possible
> > ref count overflow around the runtime_pm was giving me a enough of a headache
> > that it's easier to ask you just to poke it and see.  If it doesn't fail as
> > I expect I'll take a closer look!
> >
> > Jonathan
> >
> > ...  
> >> +	data->client = client;
> >> +	mutex_init(&data->lock);
> >> +
> >> +	indio_dev->name = "ads1100";
> >> +	indio_dev->modes = INDIO_DIRECT_MODE;
> >> +	indio_dev->channels = &ads1100_channel;
> >> +	indio_dev->num_channels = 1;
> >> +	indio_dev->info = &ads1100_info;
> >> +
> >> +	data->reg_vdd = devm_regulator_get(dev, "vdd");
> >> +	if (IS_ERR(data->reg_vdd))
> >> +		return dev_err_probe(dev, PTR_ERR(data->reg_vdd),
> >> +				     "Failed to get vdd regulator\n");
> >> +
> >> +	ret = regulator_enable(data->reg_vdd);
> >> +	if (ret < 0)
> >> +		return dev_err_probe(dev, PTR_ERR(data->reg_vdd),
> >> +				     "Failed to enable vdd regulator\n");
> >> +
> >> +	ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
> >> +	if (ret)
> >> +		return ret;  
> > Please could you check a subtle interaction of runtime pm and this devm managed
> > flow.
> >
> > I think we can hit the following flow.
> > 1) In runtime suspend (wait long enough for this to happen).
> > 2) Unbind the driver (rmmod will do)
> > 3) During the unbind we exit suspend then enter it again before we call remove
> >     (that's just part of the normal remove flow).
> > 4) We then end up calling regulator disable when it's already disabled.
> >
> > We've traditionally avoided that by having the remove explicitly call
> > pm_runtime_get_sync() before we then disable runtime pm.  I don't
> > think that happens with devm_pm_runtime_enable() but I could be missing
> > a path where it does.
> >
> > If the sequence goes wrong you should get a warning about an unbalanced regulator
> > disable.  The fix would be an extra devm_add_action_or_reset() before the
> > devm_iio_device_register() below that just calls pm_runtime_get_sync()
> > to force the state to on.
> >
> > Gah. These subtle paths always give me a headache.
> > We don't normally have too much problem with this because many
> > runtime_resume / suspend functions don't change reference counts.  
> 
> Just did this test, waited a few seconds, checked 
> /sys/kernel/debug/regulator... that the regulator had been disabled.
> 
> Then executed:
> echo -n 3-004a > /sys/bus/i2c/drivers/ads1100/unbind
> 
> to unload the driver, and no messages were added to the kernel log.
> 
> I could see the driver going away and removing itself from iio and 
> regulators.
> 
> Tried this a couple of times (using bind/unbind), and no problem reported.
> 
> Hopes this helps with your headaches...

Thanks!

>