From: Jonathan Cameron <jic23@kernel.org>
To: "Patrik Dahlström" <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
Date: Sun, 26 Mar 2023 17:59:28 +0100 [thread overview]
Message-ID: <20230326175928.0dc26a4f@jic23-huawei> (raw)
In-Reply-To: <20230319223908.108540-4-risca@dalakolonin.se>
On Sun, 19 Mar 2023 23:39:08 +0100
Patrik Dahlström <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.
>
> 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.
A few questions / comments inline.
Jonathan
>
> Signed-off-by: Patrik Dahlström <risca@dalakolonin.se>
> ---
> 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.
> 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.
>
> 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;
next prev parent reply other threads:[~2023-03-26 16:44 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-19 22:39 [PATCH 0/3] iio: adc: palmas_gpadc: add iio events Patrik Dahlström
2023-03-19 22:39 ` [PATCH 1/3] iio: adc: palmas_gpadc: add support for iio threshold events Patrik Dahlström
2023-03-21 20:40 ` H. Nikolaus Schaller
2023-04-01 18:15 ` Patrik Dahlström
2023-03-26 16:51 ` Jonathan Cameron
2023-04-01 18:45 ` Patrik Dahlström
2023-04-02 16:43 ` Jonathan Cameron
2023-03-19 22:39 ` [PATCH 2/3] iio: adc: palmas_gpadc: remove adc_wakeupX_data Patrik Dahlström
2023-03-26 16:53 ` Jonathan Cameron
2023-03-19 22:39 ` [PATCH 3/3] iio: adc: palmas_gpadc: remove palmas_adc_wakeup_property Patrik Dahlström
2023-03-26 16:59 ` Jonathan Cameron [this message]
2023-04-01 19:01 ` Patrik Dahlström
2023-03-21 20:40 ` [PATCH 0/3] iio: adc: palmas_gpadc: add iio events H. Nikolaus Schaller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230326175928.0dc26a4f@jic23-huawei \
--to=jic23@kernel.org \
--cc=hns@goldelico.com \
--cc=kernel@pyra-handheld.com \
--cc=lars@metafoo.de \
--cc=letux-kernel@openphoenux.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pgoudagunta@nvidia.com \
--cc=risca@dalakolonin.se \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox