On Mon, 6 Mar 2023 12:21:44 +0100 Mike Looijmans <mike.looijmans@xxxxxxxx> 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@xxxxxxxxxxxxxxxxx > 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@xxxxxxxx> 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@xxxxxxxx> > > 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! >
