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@xxxxxxxxxxxxxx> 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?
>
> >
> > 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?
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.
> >
> > Signed-off-by: Patrik Dahlström <risca@xxxxxxxxxxxxxx>
>
> >
> > @@ -280,6 +326,9 @@ static int palmas_gpadc_read_prepare(struct palmas_gpadc *adc, int adc_chan)
> > {
> > int ret;
> >
> > + if (palmas_gpadc_channel_is_freerunning(adc, adc_chan))
> > + return 0; // ADC already running
>
> /* */
>
> ...
>
> >
> > +static int palmas_gpadc_get_high_threshold_raw(struct palmas_gpadc *adc,
> > + struct palmas_adc_event *ev)
> > +{
> > + const int INL = 2;
> > + const int adc_chan = ev->channel;
> > + const int orig = adc->thresh_data[adc_chan].high_thresh;
> > + int val = orig;
> > + int gain_drift;
> > + int offset_drift;
> > +
> > + if (!val)
> > + return 0;
> > +
> > + val = (val * 1000) / adc->adc_info[adc_chan].gain;
> > +
> > + if (!adc->adc_info[adc_chan].is_uncalibrated) {
> > + val = (val * adc->adc_info[adc_chan].gain_error +
> > + adc->adc_info[adc_chan].offset) /
> > + 1000;
> > + gain_drift = 1002;
> > + offset_drift = 2;
> > + }
> > + else {
> > + gain_drift = 1022;
> > + offset_drift = 36;
> > + }
> > +
> > + // add tolerance to threshold
> > + val = ((val + INL) * gain_drift) / 1000 + offset_drift;
> > +
> > + // clamp to max possible value
> /* clamp .. */
> val = min(val, 0xFFF);
>
>
> > + if (val > 0xFFF)
> > + val = 0xFFF;
> > +
> > + return val;
> > +}
> > +
> > +static int palmas_gpadc_get_low_threshold_raw(struct palmas_gpadc *adc,
> > + struct palmas_adc_event *ev)
> > +{
> > + const int INL = 2;
> > + const int adc_chan = ev->channel;
> > + const int orig = adc->thresh_data[adc_chan].low_thresh;
> > + int val = orig;
> > + int gain_drift;
> > + int offset_drift;
> > +
> > + if (!val)
> > + return val;
> > +
> > + val = (val * 1000) / adc->adc_info[adc_chan].gain;
> > +
> > + if (!adc->adc_info[adc_chan].is_uncalibrated) {
> > + val = (val * adc->adc_info[adc_chan].gain_error -
> > + adc->adc_info[adc_chan].offset) /
> > + 1000;
> > + gain_drift = 998;
> > + offset_drift = 2;
> > + }
> > + else {
> > + gain_drift = 978;
> > + offset_drift = 36;
> > + }
> > +
> > + // calculate tolerances
> /* */
>
> + I think this could do with more information on why a tweak is needed.
>
> > + val = ((val - INL) * gain_drift) / 1000 - offset_drift;
> > +
> > + // clamp to minimum 0
>
> /* */ for all comments.
>
> val = max(0, val); then comment may not be needed.
>
> > + if (val < 0)
> > + val = 0;
> > +
> > + return val;
> > +}
>
> > +static int palmas_gpadc_reconfigure_event_channels(struct palmas_gpadc *adc)
> > +{
> > + bool was_enabled = adc->wakeup1_enable || adc->wakeup2_enable;
> > + bool enable;
> > +
> > + adc->wakeup1_enable = adc->event0.channel == -1 ? false : true;
> > + adc->wakeup2_enable = adc->event1.channel == -1 ? false : true;
> > +
> > + enable = adc->wakeup1_enable || adc->wakeup2_enable;
> > + if (!was_enabled && enable)
> > + device_wakeup_enable(adc->dev);
> > + else if (was_enabled && !enable)
> > + device_wakeup_disable(adc->dev);
> > +
> > + if (!enable)
> > + return palmas_adc_wakeup_reset(adc);
> > +
> > + // adjust levels
>
> /* adjust levels */
>
> > + if (adc->wakeup1_enable)
> > + palmas_adc_event_to_wakeup(adc, &adc->event0, &adc->wakeup1_data);
> > + if (adc->wakeup2_enable)
> > + palmas_adc_event_to_wakeup(adc, &adc->event1, &adc->wakeup2_data);
> > +
> > + return palmas_adc_wakeup_configure(adc);
> > +}
> > +
> > +static int palmas_gpadc_enable_event_config(struct palmas_gpadc *adc,
> > + const struct iio_chan_spec *chan, enum iio_event_direction dir)
> > +{
> > + struct palmas_adc_event *ev;
> > + int adc_chan = chan->channel;
> > +
> > + if (palmas_gpadc_get_event_channel(adc, adc_chan, dir))
> > + /* already enabled */
> > + return 0;
> > +
> > + if (adc->event0.channel == -1)
>
> I'd add brackets for all legs of this if / else once one of them needs
> it. Tends to end up more readable.
>
> > + ev = &adc->event0;
> > + else if (adc->event1.channel == -1) {
> > + /* event0 has to be the lowest channel */
> > + if (adc_chan < adc->event0.channel) {
> > + adc->event1 = adc->event0;
> > + ev = &adc->event0;
> > + }
> > + else
> > + ev = &adc->event1;
> > + }
>
> } else /*...
>
> > + else /* both AUTO channels already in use */ {
> > + dev_warn(adc->dev, "event0 - %d, event1 - %d\n",
> > + adc->event0.channel, adc->event1.channel);
> > + return -EBUSY;
> > + }
> > +
> > + ev->channel = adc_chan;
> > + ev->direction = dir;
> > +
> > + return palmas_gpadc_reconfigure_event_channels(adc);
> > +}
>
> > +
> > +static int palmas_gpadc_write_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan, enum iio_event_type type,
> > + enum iio_event_direction dir, enum iio_event_info info, int val,
> > + int val2)
>
> Prefer parameters aligned just after (
>
> > +{
> ...
>
>
> >
> > static int palmas_gpadc_probe(struct platform_device *pdev)
>
> ...
>
> > /* set the current source 0 (value 0/5/15/20 uA => 0..3) */
> > if (gpadc_pdata->ch0_current <= 1)
> > adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_0;
> > @@ -610,20 +998,23 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
> > return dev_err_probe(adc->dev, ret,
> > "iio_device_register() failed\n");
> >
> > - device_set_wakeup_capable(&pdev->dev, 1);
> > for (i = 0; i < PALMAS_ADC_CH_MAX; i++) {
> > if (!(adc->adc_info[i].is_uncalibrated))
> > palmas_gpadc_calibrate(adc, i);
> > }
> >
> > + device_set_wakeup_capable(&pdev->dev, 1);
> > if (adc->wakeup1_enable || adc->wakeup2_enable) {
> > - device_wakeup_enable(&pdev->dev);
> > - ret = devm_add_action_or_reset(&pdev->dev,
> > - palmas_disable_wakeup,
> > - &pdev->dev);
> > + ret = palmas_adc_wakeup_configure(adc);
> > if (ret)
> > return ret;
> > + device_wakeup_enable(&pdev->dev);
>
> > }
> > + 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.
>
> > + palmas_disable_wakeup,
> > + adc);
> > + if (ret)
> > + return ret;
> >
> > return 0;
> > }
>
>
>
