On Sat, 1 Apr 2023 20:45:07 +0200 Patrik Dahlström risca@dalakolonin.se wrote:
On Sun, Mar 26, 2023 at 05:51:01PM +0100, Jonathan Cameron wrote:
On Sun, 19 Mar 2023 23:39:06 +0100 Patrik Dahlström risca@dalakolonin.se wrote:
The palmas gpadc block has support for monitoring up to 2 ADC channels and issue an interrupt if they reach past a set threshold. The gpadc driver had limited support for this through the adc_wakeup{1,2}_data platform data. This however only allow a fixed threshold to be set at boot, and would only enable it when entering sleep mode.
This change hooks into the IIO events system and exposes to userspace the ability to configure threshold values for each channel individually, but only allow up to 2 such thresholds to be enabled at any given time.
Add a comment here on what happens if userspace tries to set more than two. It's not as obvious as you'd think as we have some drivers that use a fifo approach so on setting the third event they push the oldest one out.
Will do!
Is there any preference to any one approach?
Not really. Userspace should be checking either for an error, or that the setup matches what it thinks it configured (after finishing configuring) so it should cope with either.
The logic around suspend and resume had to be adjusted so that user space configuration don't get reset on resume. Instead, any configured adc auto wakeup gets enabled during probe.
Enabling a threshold from userspace will overwrite the adc wakeup configuration set during probe. Depending on how you look at it, this could also mean we allow userspace to update the adc wakeup thresholds.
I'm not sure I read the code right, but can you end up enabling a wakeup that wasn't previously present? That seems likely something we should not be doing after boot.
One option here would be to make it either wakeup is supported, or events are supported. I suspect no one uses the wakeup anyway so that shouldn't matter much (+ you remove it in next patch - do that first and this code becomes more obvious).
My use case is for monitoring a volume wheel connected to one of the ADC inputs of the palmas chip. By off-loading the monitoring to a separate chip, the SoC can go to sleep and only wake up when the wheel is moved.
It made sense for my use case, but I see your point. IIO events and wakeup triggers should be treated as separate things. I will look into defining the dev_pm_info of the device. Then userspace should be able to control wakeup from /sys/devices/.../power/wakeup.
However, suspend and resume is a bit flaky on my board so testing might be too. If the board reacts and at least tries to resume should indicate that the code works, no?
That will be fine. Obviously good to debug the other issues with resume though!
In any case, I will remove the old wakeup code first in v2.
A few trivial comments inline.
I will adress them in v2. They all made perfect sense.
- ret = devm_add_action_or_reset(&pdev->dev,
Add a comment for this to explain why it might need disabling even if it wasn't enabled above. I think if you just drop wakeup support in general that will be fine given we have no known users.
I'm one such user.
Fair enough :)
Jonathan
palmas_disable_wakeup,
adc);
if (ret)
return ret;
return 0;
}