Re: [PATCH 3/3] iio: adc: palmas_gpadc: remove palmas_adc_wakeup_property

[Patrik Dahlström]

On Sun, Mar 26, 2023 at 05:59:28PM +0100, Jonathan Cameron wrote:
> On Sun, 19 Mar 2023 23:39:08 +0100
> Patrik Dahlström <risca@xxxxxxxxxxxxxx> wrote:
> 
> > This struct contain essentially the same information as
> > palmas_adc_event and palmas_gpadc_thresholds combined, but with more
> > ambiguity: the code decided whether to trigger on rising or falling edge
> > based on the high threshold being non-zero.
> > 
> > Since its use in platform data has now been removed, we can remove it
> > entirely.
> > 
> > Lastly, the use case for waking up the cpu from sleep mode when a
> > threshold has been passed is no longer the primary use for events so all
> > code is changed to say "event" instead of "wakeup".
> Good. I nearly pointed this out in the earlier patch.  The wakeup naming
> was confusing. However, I'd prefer that was done in a separate patch to
> any other changes.  It's hard to spot the meaningful stuff when there
> is a lot of renaming going on.

Since I was doing this patch last, it made little sense to keep the wakeup
naming when removing the wakeup property. However, as you pointed out in
your review of the previous patches, it would be better to first remove the
wakeup property and then add iio events support.
> 
> A few questions / comments inline.
> 
> Jonathan
> 
> > 
> > Signed-off-by: Patrik Dahlström <risca@xxxxxxxxxxxxxx>
> > ---
> >  drivers/iio/adc/palmas_gpadc.c | 94 +++++++++++++---------------------
> >  include/linux/mfd/palmas.h     |  6 ---
> >  2 files changed, 36 insertions(+), 64 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
> > index 419d7db51345..042b68f29444 100644
> > --- a/drivers/iio/adc/palmas_gpadc.c
> > +++ b/drivers/iio/adc/palmas_gpadc.c
> > @@ -122,10 +122,8 @@ struct palmas_gpadc {
> >  	int				irq_auto_1;
> >  	struct palmas_gpadc_info	*adc_info;
> >  	struct completion		conv_completion;
> > -	struct palmas_adc_wakeup_property wakeup1_data;
> > -	struct palmas_adc_wakeup_property wakeup2_data;
> > -	bool				wakeup1_enable;
> > -	bool				wakeup2_enable;
> > +	bool				event0_enable;
> > +	bool				event1_enable;
> >  	int				auto_conversion_period;
> >  	struct mutex			lock;
> >  	struct palmas_adc_event		event0;
> > @@ -592,50 +590,26 @@ static int palmas_gpadc_read_event_config(struct iio_dev *indio_dev,
> >  	return ret;
> >  }
> >  
> > -static void palmas_adc_event_to_wakeup(struct palmas_gpadc *adc,
> > -				       struct palmas_adc_event *ev,
> > -				       struct palmas_adc_wakeup_property *wakeup)
> > -{
> > -	wakeup->adc_channel_number = ev->channel;
> > -	if (ev->direction == IIO_EV_DIR_RISING) {
> > -		wakeup->adc_low_threshold = 0;
> > -		wakeup->adc_high_threshold =
> > -			palmas_gpadc_get_high_threshold_raw(adc, &adc->event0);
> > -	}
> > -	else {
> > -		wakeup->adc_low_threshold =
> > -			palmas_gpadc_get_low_threshold_raw(adc, &adc->event0);
> > -		wakeup->adc_high_threshold = 0;
> > -	}
> > -}
> > -
> > -static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc);
> > -static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc);
> > +static int palmas_adc_events_configure(struct palmas_gpadc *adc);
> > +static int palmas_adc_events_reset(struct palmas_gpadc *adc);
> >  
> >  static int palmas_gpadc_reconfigure_event_channels(struct palmas_gpadc *adc)
> >  {
> > -	bool was_enabled = adc->wakeup1_enable || adc->wakeup2_enable;
> > +	bool was_enabled = adc->event0_enable || adc->event1_enable;
> >  	bool enable;
> >  
> > -	adc->wakeup1_enable = adc->event0.channel == -1 ? false : true;
> > -	adc->wakeup2_enable = adc->event1.channel == -1 ? false : true;
> > +	adc->event0_enable = adc->event0.channel == -1 ? false : true;
> > +	adc->event1_enable = adc->event1.channel == -1 ? false : true;
> >  
> > -	enable = adc->wakeup1_enable || adc->wakeup2_enable;
> > +	enable = adc->event0_enable || adc->event1_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
> > -	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);
> > +	return enable ?
> > +		palmas_adc_events_configure(adc) :
> > +		palmas_adc_events_reset(adc);
> >  }
> >  
> >  static int palmas_gpadc_enable_event_config(struct palmas_gpadc *adc,
> > @@ -864,12 +838,14 @@ static int palmas_gpadc_get_adc_dt_data(struct platform_device *pdev,
> >  	return 0;
> >  }
> >  
> > -static void palmas_disable_wakeup(void *data)
> > +static void palmas_disable_events(void *data)
> >  {
> >  	struct palmas_gpadc *adc = data;
> >  
> > -	if (adc->wakeup1_enable || adc->wakeup2_enable)
> > +	if (adc->event0_enable || adc->event1_enable) {
> > +		palmas_adc_events_reset(adc);
> 
> I can't immediately follow why this reset is needed when it wasn't before.
> Perhaps that will be clearer once the renames aren't in the same patch.

The original code would only enable adc events when entering any kind of
sleep mode and then reset when resuming, hence the name wakeupX_enable. The
new code allow adc events to be enabled at any time. palmas_disable_events
is run when unloading the module and as such it makes sense to also reset
the adc.

> 
> >  		device_wakeup_disable(adc->dev);
> > +	}
> >  }
> >  
> >  static int palmas_gpadc_probe(struct platform_device *pdev)
> > @@ -993,7 +969,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
> >  
> >  	device_set_wakeup_capable(&pdev->dev, 1);
> >  	ret = devm_add_action_or_reset(&pdev->dev,
> > -				       palmas_disable_wakeup,
> > +				       palmas_disable_events,
> >  				       adc);
> >  	if (ret)
> >  		return ret;
> > @@ -1001,7 +977,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > -static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
> > +static int palmas_adc_events_configure(struct palmas_gpadc *adc)
> >  {
> >  	int adc_period, conv;
> >  	int i;
> > @@ -1027,16 +1003,18 @@ static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
> >  	}
> >  
> >  	conv = 0;
> > -	if (adc->wakeup1_enable) {
> > +	if (adc->event0_enable) {
> > +		struct palmas_adc_event *ev = &adc->event0;
> >  		int polarity;
> >  
> > -		ch0 = adc->wakeup1_data.adc_channel_number;
> > +		ch0 = ev->channel;
> >  		conv |= PALMAS_GPADC_AUTO_CTRL_AUTO_CONV0_EN;
> > -		if (adc->wakeup1_data.adc_high_threshold > 0) {
> > -			thres = adc->wakeup1_data.adc_high_threshold;
> > +
> > +		if (ev->direction == IIO_EV_DIR_RISING) {
> > +			thres = palmas_gpadc_get_high_threshold_raw(adc, ev);
> >  			polarity = 0;
> >  		} else {
> > -			thres = adc->wakeup1_data.adc_low_threshold;
> > +			thres = palmas_gpadc_get_low_threshold_raw(adc, ev);
> >  			polarity = PALMAS_GPADC_THRES_CONV0_MSB_THRES_CONV0_POL;
> >  		}
> >  
> > @@ -1058,16 +1036,18 @@ static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
> >  		}
> >  	}
> >  
> > -	if (adc->wakeup2_enable) {
> > +	if (adc->event1_enable) {
> > +		struct palmas_adc_event *ev = &adc->event1;
> >  		int polarity;
> >  
> > -		ch1 = adc->wakeup2_data.adc_channel_number;
> > +		ch1 = ev->channel;
> >  		conv |= PALMAS_GPADC_AUTO_CTRL_AUTO_CONV1_EN;
> > -		if (adc->wakeup2_data.adc_high_threshold > 0) {
> > -			thres = adc->wakeup2_data.adc_high_threshold;
> > +
> > +		if (ev->direction == IIO_EV_DIR_RISING) {
> > +			thres = palmas_gpadc_get_high_threshold_raw(adc, ev);
> >  			polarity = 0;
> >  		} else {
> > -			thres = adc->wakeup2_data.adc_low_threshold;
> > +			thres = palmas_gpadc_get_low_threshold_raw(adc, ev);
> >  			polarity = PALMAS_GPADC_THRES_CONV1_MSB_THRES_CONV1_POL;
> >  		}
> >  
> > @@ -1106,7 +1086,7 @@ static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
> >  	return ret;
> >  }
> >  
> > -static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc)
> > +static int palmas_adc_events_reset(struct palmas_gpadc *adc)
> >  {
> >  	int ret;
> >  
> > @@ -1128,15 +1108,14 @@ static int palmas_gpadc_suspend(struct device *dev)
> >  {
> >  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> >  	struct palmas_gpadc *adc = iio_priv(indio_dev);
> > -	int ret;
> 
> ?  Seems unrelated - perhaps should be in earlier patch.

You're right. I'll look into it.

> 
> >  
> >  	if (!device_may_wakeup(dev))
> >  		return 0;
> >  
> > -	if (adc->wakeup1_enable)
> > +	if (adc->event0_enable)
> >  		enable_irq_wake(adc->irq_auto_0);
> >  
> > -	if (adc->wakeup2_enable)
> > +	if (adc->event1_enable)
> >  		enable_irq_wake(adc->irq_auto_1);
> >  
> >  	return 0;
> > @@ -1146,15 +1125,14 @@ static int palmas_gpadc_resume(struct device *dev)
> >  {
> >  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> >  	struct palmas_gpadc *adc = iio_priv(indio_dev);
> > -	int ret;
> 
> ?
> 
> >  
> >  	if (!device_may_wakeup(dev))
> >  		return 0;
> >  
> > -	if (adc->wakeup1_enable)
> > +	if (adc->event0_enable)
> >  		disable_irq_wake(adc->irq_auto_0);
> >  
> > -	if (adc->wakeup2_enable)
> > +	if (adc->event1_enable)
> >  		disable_irq_wake(adc->irq_auto_1);
> >  
> >  	return 0;
> 
>