From: "Patrik Dahlström" <risca@dalakolonin.se>
To: Jonathan Cameron <jic23@kernel.org>
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: Sat, 1 Apr 2023 21:01:44 +0200 [thread overview]
Message-ID: <20230401190144.GC2403322@dalakolonin.se> (raw)
In-Reply-To: <20230326175928.0dc26a4f@jic23-huawei>
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@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.
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@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.
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;
>
>
next prev parent reply other threads:[~2023-04-01 19:01 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
2023-04-01 19:01 ` Patrik Dahlström [this message]
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=20230401190144.GC2403322@dalakolonin.se \
--to=risca@dalakolonin.se \
--cc=hns@goldelico.com \
--cc=jic23@kernel.org \
--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 \
/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