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, 2 Apr 2023 17:43:47 +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 1/3] iio: adc: palmas_gpadc: add support for iio
 threshold events
Message-ID: <20230402174347.25e5ff06@jic23-huawei>
In-Reply-To: <20230401184507.GB2403322@dalakolonin.se>
References: <20230319223908.108540-1-risca@dalakolonin.se>
        <20230319223908.108540-2-risca@dalakolonin.se>
        <20230326175101.0ef2d6ae@jic23-huawei>
        <20230401184507.GB2403322@dalakolonin.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, 1 Apr 2023 20:45:07 +0200
Patrik Dahlstr=C3=B6m <risca@dalakolonin.se> wrote:

> On Sun, Mar 26, 2023 at 05:51:01PM +0100, Jonathan Cameron wrote:
> > On Sun, 19 Mar 2023 23:39:06 +0100
> > Patrik Dahlstr=C3=B6m <risca@dalakolonin.se> wrote:
> >  =20
> > > 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.
> > >=20
> > > This change hooks into the IIO events system and exposes to userspace
> > > the ability to configure threshold values for each channel individual=
ly,
> > > but only allow up to 2 such thresholds to be enabled at any given tim=
e. =20
> >=20
> > 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 f=
ifo
> > approach so on setting the third event they push the oldest one out. =20
>=20
> Will do!
>=20
> Is there any preference to any one approach?

Not really.  Userspace should be checking either for an error, or that
the setup matches what it thinks it configured (after finishing configuring)
so it should cope with either.

>=20
> >  =20
> > >=20
> > > 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.
> > >=20
> > > 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 threshold=
s. =20
> >=20
> > 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.
> >=20
> > One option here would be to make it either wakeup is supported, or even=
ts
> > 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).
> >  =20
>=20
> 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.
>=20
> 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.
>=20
> 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?

That will be fine. Obviously good to debug the other issues with resume tho=
ugh!

>=20
> In any case, I will remove the old wakeup code first in v2.
>=20
> >=20
> > A few trivial comments inline. =20
>=20
> I will adress them in v2. They all made perfect sense.
>> > > +	ret =3D devm_add_action_or_reset(&pdev->dev, =20
> >=20
> > 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.
> >  =20
>=20
> I'm one such user.
Fair enough :)

Jonathan

>=20
> >  =20
> > > +				       palmas_disable_wakeup,
> > > +				       adc);
> > > +	if (ret)
> > > +		return ret;
> > > =20
> > >  	return 0;
> > >  } =20
> >=20
> >=20
> >  =20