Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 26 Mar 2023 17:59:28 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Patrik =?UTF-8?B?RGFobHN0csO2bQ==?= <risca@dalakolonin.se>
Cc:     linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
        letux-kernel@openphoenux.org, kernel@pyra-handheld.com,
        pgoudagunta@nvidia.com, hns@goldelico.com, lars@metafoo.de
Subject: Re: [PATCH 3/3] iio: adc: palmas_gpadc: remove
 palmas_adc_wakeup_property
Message-ID: <20230326175928.0dc26a4f@jic23-huawei>
In-Reply-To: <20230319223908.108540-4-risca@dalakolonin.se>
References: <20230319223908.108540-1-risca@dalakolonin.se>
        <20230319223908.108540-4-risca@dalakolonin.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, 19 Mar 2023 23:39:08 +0100
Patrik Dahlstr=C3=B6m <risca@dalakolonin.se> 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.
>=20
> Since its use in platform data has now been removed, we can remove it
> entirely.
>=20
> 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.

A few questions / comments inline.

Jonathan

>=20
> Signed-off-by: Patrik Dahlstr=C3=B6m <risca@dalakolonin.se>
> ---
>  drivers/iio/adc/palmas_gpadc.c | 94 +++++++++++++---------------------
>  include/linux/mfd/palmas.h     |  6 ---
>  2 files changed, 36 insertions(+), 64 deletions(-)
>=20
> diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpad=
c.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 ii=
o_dev *indio_dev,
>  	return ret;
>  }
> =20
> -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 =3D ev->channel;
> -	if (ev->direction =3D=3D IIO_EV_DIR_RISING) {
> -		wakeup->adc_low_threshold =3D 0;
> -		wakeup->adc_high_threshold =3D
> -			palmas_gpadc_get_high_threshold_raw(adc, &adc->event0);
> -	}
> -	else {
> -		wakeup->adc_low_threshold =3D
> -			palmas_gpadc_get_low_threshold_raw(adc, &adc->event0);
> -		wakeup->adc_high_threshold =3D 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);
> =20
>  static int palmas_gpadc_reconfigure_event_channels(struct palmas_gpadc *=
adc)
>  {
> -	bool was_enabled =3D adc->wakeup1_enable || adc->wakeup2_enable;
> +	bool was_enabled =3D adc->event0_enable || adc->event1_enable;
>  	bool enable;
> =20
> -	adc->wakeup1_enable =3D adc->event0.channel =3D=3D -1 ? false : true;
> -	adc->wakeup2_enable =3D adc->event1.channel =3D=3D -1 ? false : true;
> +	adc->event0_enable =3D adc->event0.channel =3D=3D -1 ? false : true;
> +	adc->event1_enable =3D adc->event1.channel =3D=3D -1 ? false : true;
> =20
> -	enable =3D adc->wakeup1_enable || adc->wakeup2_enable;
> +	enable =3D 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);
> =20
> -	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);
>  }
> =20
>  static int palmas_gpadc_enable_event_config(struct palmas_gpadc *adc,
> @@ -864,12 +838,14 @@ static int palmas_gpadc_get_adc_dt_data(struct plat=
form_device *pdev,
>  	return 0;
>  }
> =20
> -static void palmas_disable_wakeup(void *data)
> +static void palmas_disable_events(void *data)
>  {
>  	struct palmas_gpadc *adc =3D data;
> =20
> -	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.

>  		device_wakeup_disable(adc->dev);
> +	}
>  }
> =20
>  static int palmas_gpadc_probe(struct platform_device *pdev)
> @@ -993,7 +969,7 @@ static int palmas_gpadc_probe(struct platform_device =
*pdev)
> =20
>  	device_set_wakeup_capable(&pdev->dev, 1);
>  	ret =3D 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;
>  }
> =20
> -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 pal=
mas_gpadc *adc)
>  	}
> =20
>  	conv =3D 0;
> -	if (adc->wakeup1_enable) {
> +	if (adc->event0_enable) {
> +		struct palmas_adc_event *ev =3D &adc->event0;
>  		int polarity;
> =20
> -		ch0 =3D adc->wakeup1_data.adc_channel_number;
> +		ch0 =3D ev->channel;
>  		conv |=3D PALMAS_GPADC_AUTO_CTRL_AUTO_CONV0_EN;
> -		if (adc->wakeup1_data.adc_high_threshold > 0) {
> -			thres =3D adc->wakeup1_data.adc_high_threshold;
> +
> +		if (ev->direction =3D=3D IIO_EV_DIR_RISING) {
> +			thres =3D palmas_gpadc_get_high_threshold_raw(adc, ev);
>  			polarity =3D 0;
>  		} else {
> -			thres =3D adc->wakeup1_data.adc_low_threshold;
> +			thres =3D palmas_gpadc_get_low_threshold_raw(adc, ev);
>  			polarity =3D PALMAS_GPADC_THRES_CONV0_MSB_THRES_CONV0_POL;
>  		}
> =20
> @@ -1058,16 +1036,18 @@ static int palmas_adc_wakeup_configure(struct pal=
mas_gpadc *adc)
>  		}
>  	}
> =20
> -	if (adc->wakeup2_enable) {
> +	if (adc->event1_enable) {
> +		struct palmas_adc_event *ev =3D &adc->event1;
>  		int polarity;
> =20
> -		ch1 =3D adc->wakeup2_data.adc_channel_number;
> +		ch1 =3D ev->channel;
>  		conv |=3D PALMAS_GPADC_AUTO_CTRL_AUTO_CONV1_EN;
> -		if (adc->wakeup2_data.adc_high_threshold > 0) {
> -			thres =3D adc->wakeup2_data.adc_high_threshold;
> +
> +		if (ev->direction =3D=3D IIO_EV_DIR_RISING) {
> +			thres =3D palmas_gpadc_get_high_threshold_raw(adc, ev);
>  			polarity =3D 0;
>  		} else {
> -			thres =3D adc->wakeup2_data.adc_low_threshold;
> +			thres =3D palmas_gpadc_get_low_threshold_raw(adc, ev);
>  			polarity =3D PALMAS_GPADC_THRES_CONV1_MSB_THRES_CONV1_POL;
>  		}
> =20
> @@ -1106,7 +1086,7 @@ static int palmas_adc_wakeup_configure(struct palma=
s_gpadc *adc)
>  	return ret;
>  }
> =20
> -static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc)
> +static int palmas_adc_events_reset(struct palmas_gpadc *adc)
>  {
>  	int ret;
> =20
> @@ -1128,15 +1108,14 @@ static int palmas_gpadc_suspend(struct device *de=
v)
>  {
>  	struct iio_dev *indio_dev =3D dev_get_drvdata(dev);
>  	struct palmas_gpadc *adc =3D iio_priv(indio_dev);
> -	int ret;

?  Seems unrelated - perhaps should be in earlier patch.

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

?

> =20
>  	if (!device_may_wakeup(dev))
>  		return 0;
> =20
> -	if (adc->wakeup1_enable)
> +	if (adc->event0_enable)
>  		disable_irq_wake(adc->irq_auto_0);
> =20
> -	if (adc->wakeup2_enable)
> +	if (adc->event1_enable)
>  		disable_irq_wake(adc->irq_auto_1);
> =20
>  	return 0;