Message-ID: <20230401184507.GB2403322@dalakolonin.se>
Date:   Sat, 1 Apr 2023 20:45:07 +0200
From:   Patrik Dahlström <risca@...akolonin.se>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        letux-kernel@...nphoenux.org, kernel@...a-handheld.com,
        pgoudagunta@...dia.com, hns@...delico.com, lars@...afoo.de
Subject: Re: [PATCH 1/3] iio: adc: palmas_gpadc: add support for iio
 threshold events

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@...akolonin.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?

> 
> > 
> > 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@...akolonin.se>
> 
> >  
> > @@ -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;
> >  }
> 
> 
> 

Powered by blists - more mailing lists