Message-ID: <CAPuLcztqUaDWtgYe0KX0VWUMi8DLiO0a5Js4PCck9zH7=h+qCQ@mail.gmail.com>
Date:   Mon, 14 Feb 2022 19:46:50 +1100
From:   Sami Kyostila <skyostil@...omium.org>
To:     Tzung-Bi Shih <tzungbi@...gle.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, bleung@...omium.org,
        arnd@...db.de, gregkh@...uxfoundation.org, evanbenn@...omium.org,
        chrome-platform@...ts.linux.dev
Subject: Re: [PATCH v3 1/1] platform/chrome: add a driver for HPS

ma 14. helmik. 2022 klo 12.48 Tzung-Bi Shih (tzungbi@...gle.com) kirjoitti:
>
> Hi,
>
> Left some follow-up questions before going for the v4.  Please also cc the
> following patches to <chrome-platform@...ts.linux.dev> in case we would
> overlook them from LKML.

Thank you, I appreciate the feedback!

>
> On Fri, Feb 11, 2022 at 08:08:45PM +1100, Sami Kyostila wrote:
> > ( to 10. helmik. 2022 klo 16.41 Tzung-Bi Shih (tzungbi@...gle.com) kirjoitti:
> > >
> > > On Mon, Feb 07, 2022 at 12:36:13PM +1100, Sami Kyöstilä wrote:
> > > > When loaded, the driver exports the sensor to userspace through a
> > > > character device. This device only supports power management, i.e.,
> > > > communication with the sensor must be done through regular I2C
> > > > transmissions from userspace.
> > > >
> > > > Power management is implemented by enabling the respective power GPIO
> > > > while at least one userspace process holds an open fd on the character
> > > > device. By default, the device is powered down if there are no active
> > > > clients.
> > > >
> > > > Note that the driver makes no effort to preserve the state of the sensor
> > > > between power down and power up events. Userspace is responsible for
> > > > reinitializing any needed state once power has been restored.
> > >
> > > It's weird.  If most of the thing is done by userspace programs, couldn't it
> > > set the power GPIO via userspace interface (e.g. [1]) too?
> >
> > I agree that's a little unusual, but there are some good reasons for
> > this to be in the kernel. First, it lets us turn HPS off during system
> > suspend -- which I'm not sure how you'd do from userspace. Second, it
> > avoids the need to give write access to the entire GPIO chip to the
> > hpsd userspace daemon. We just need a single line, while the
> > controller in this case has a total of 360 gpios. Finally, HPS also
> > has an interrupt line, and we're planning to let it wake up the host,
> > which I also believe needs to be done in the kernel.
>
> What prevents it from implementing the I2C communication in kernel?
>
> Did you investigate if there are any existing frameworks for HPS
> (e.g. drivers/iio/)?  I am wondering if kernel already has some abstract code
> for HPS.
>
> Only found one via:
> $ git grep 'human presence sensor'
> drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h: [...]

Yes, we did look into some existing mechanisms in the kernel this
could fit into. HID was probably the closest fit since the spec has
been recently expanded with human presence sensing (in fact, the first
revision of this driver was HID-based[1]), but a few reasons made it a
poor fit:

1. Our HPS hardware doesn't use the HID-I2C wire protocol, so we'd
still need a bespoke I2C communication layer.

2. HPS uses a somewhat complex multi-stage firmware update process,
and we didn't feel comfortable trying to push that down into the
kernel at this point.

3. The feature set of HPS is still evolving, so this doesn't seem like
the right time to bake specific functionality into a syscall
interface.

I also had a quick look at iio, but it felt a bit overkill for the
very small amount of data HPS generates.

My feeling is that HID-I2C is probably the right long term fit, but
it'll take some time (and HW iterations) to get there. Earlier in the
patch series we discussed an intermediate step where we make it
possible to expose individual I2C devices to userspace through
separate device nodes, which will help reduce the attack surface of
giving userspace write access to the entire I2C bus just for HPS.

[1] go/hps-kernel-driver (sorry for the internal link)

> > > > +static void hps_unload(void *drv_data)
> > > > +{
> > > > +     struct hps_drvdata *hps = drv_data;
> > > > +
> > > > +     hps_set_power(hps, true);
> > >
> > > Why does it need to set to true when device removing?
> >
> > By default, HPS is powered on when the system starts and before the
> > driver is loaded. We want to restore it to that default state here.
> > This is needed for example for automated testing, where we can unbind
> > the driver to make sure HPS stays powered.
>
> Echo to the idea of previous comment: does kernel have any frameworks for HPS?
> "HPS is powered on when the system starts" sounds like a chip specific
> implementation.

True, that's more of an implementation detail of our system rather
than any hard guarantee. I'll change this to save and restore the
power state that was there before the driver got loaded.

> > > > +static int hps_suspend(struct device *dev)
> > > > +{
> > > > +     struct i2c_client *client = to_i2c_client(dev);
> > > > +     struct hps_drvdata *hps = i2c_get_clientdata(client);
> > > > +
> > > > +     hps_set_power(hps, false);
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int hps_resume(struct device *dev)
> > > > +{
> > > > +     struct i2c_client *client = to_i2c_client(dev);
> > > > +     struct hps_drvdata *hps = i2c_get_clientdata(client);
> > > > +
> > > > +     hps_set_power(hps, true);
> > > > +     return 0;
> > > > +}
> > >
> > > Does it need to save the old state before suspending?  Instead of turning on
> > > the power after every resumes.
> >
> > No, the runtime pm system makes sure suspend and resume are only
> > called when needed. For example, if someone has an open reference to
> > the device when the system goes to sleep, suspend and resume are
> > called appropriately. If HPS was already suspended, then neither
> > entrypoint gets called when going to sleep or waking up.
>
> It uses UNIVERSAL_DEV_PM_OPS() which also sets for .suspend() and .resume().
> OTOH, the documentation suggests it has deprecated (see include/linux/pm.h).

Thanks, I think I missed this distinction between .suspend/resume()
and .runtime_suspend/resume(). As far as I can tell it shouldn't make
a difference in practice since we are just toggling a GPIO pin, but
I'll nevertheless fix it for consistency.

- Sami

Powered by blists - more mailing lists