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...
--
Mike Looijmans
