Re: [PATCH v3 1/1] platform/chrome: add a driver for HPS

[Tzung-Bi Shih <tzungbi@google.com>]

Hi,

Left some follow-up questions before going for the v4.  Please also cc the
following patches to <chrome-platform@lists.linux.dev> in case we would
overlook them from LKML.

On Fri, Feb 11, 2022 at 08:08:45PM +1100, Sami Kyostila wrote:
> ( to 10. helmik. 2022 klo 16.41 Tzung-Bi Shih (tzungbi@google.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: [...]

> > > +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.

> > > +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).