Message-ID: <CAPuLczuizJkwHsKo+W3MEjX7T_fHXApVyou3BuMH_aAQfyk1Vg@mail.gmail.com>
Date:   Fri, 11 Feb 2022 20:08:45 +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
Subject: Re: [PATCH v3 1/1] platform/chrome: add a driver for HPS

( 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:
> > This patch introduces a driver for the ChromeOS screen privacy
> > sensor (aka. HPS). The driver supports a sensor connected to the I2C bus
> > and identified as "GOOG0020" in the ACPI tables.

Thanks for the review!

> The patch uses HPS instead of SPS everywhere.  Would you consider to use
> "human presence sensor" when referring it?

Sure thing, I'll unify the terminology. There are a few different
names going around for the user-facing features powered by the sensor
which makes this a bit confusing.

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

>
> [1]: https://embeddedbits.org/new-linux-kernel-gpio-user-space-interface/
>
> > diff --git a/MAINTAINERS b/MAINTAINERS
> [...]
> > +HPS (ChromeOS screen privacy sensor) DRIVER
>
> Does it make more sense to use "CHROMEOS HPS DRIVER" title?

Good idea, done.

> > diff --git a/drivers/platform/chrome/cros_hps_i2c.c b/drivers/platform/chrome/cros_hps_i2c.c
> [...]
> > +static void hps_set_power(struct hps_drvdata *hps, bool state)
> > +{
> > +     if (!IS_ERR_OR_NULL(hps->enable_gpio))
>
> Could it get rid of the check?  Does the function get called if device probe
> fails?

True, done.

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

> > +static int hps_open(struct inode *inode, struct file *file)
> > +{
> > +     struct hps_drvdata *hps = container_of(file->private_data,
> > +                                            struct hps_drvdata, misc_device);
> > +     struct device *dev = &hps->client->dev;
> > +     int ret;
> > +
> > +     ret = pm_runtime_get_sync(dev);
> > +     if (ret < 0)
> > +             goto pm_get_fail;
> > +     return 0;
> > +
> > +pm_get_fail:
> > +     pm_runtime_put(dev);
> > +     pm_runtime_disable(dev);
>
> The two functions are not effectively symmetric if pm_runtime_get_sync()
> fails.
> - It doesn't need to call pm_runtime_put() if pm_runtime_get_sync() fails.
> - I guess it wouldn't want to pm_runtime_disable() here.  The capability is
>   controlled when the device probing and removing.

According to the documentation, pm_runtime_get_sync() always bumps the
usage count, including in failure cases. However there's
pm_runtime_resume_and_get() which doesn't increment the counter for
failures, so I've switched to that so that we don't have to handle the
error case here. I agree that pm_runtime_disable() isn't really
necessary here -- removed.

> > +static int hps_release(struct inode *inode, struct file *file)
> > +{
> > +     struct hps_drvdata *hps = container_of(file->private_data,
> > +                                            struct hps_drvdata, misc_device);
> > +     struct device *dev = &hps->client->dev;
> > +     int ret;
> > +
> > +     ret = pm_runtime_put(dev);
> > +     if (ret < 0)
> > +             goto pm_put_fail;
> > +     return 0;
> > +
> > +pm_put_fail:
> > +     pm_runtime_disable(dev);
>
> Same here.

Removed.

>
> > +const struct file_operations hps_fops = {
> > +     .owner = THIS_MODULE,
> > +     .open = hps_open,
> > +     .release = hps_release,
> > +};
>
> The struct can be static.

Done.

>
> > +static int hps_i2c_probe(struct i2c_client *client)
> > +{
> > +     struct hps_drvdata *hps;
> > +     int ret = 0;
>
> It doesn't need to be initialized.  It's going to be overridden soon.

Fixed.

> > +     memset(&hps->misc_device, 0, sizeof(hps->misc_device));
> > +     hps->misc_device.parent = &client->dev;
> > +     hps->misc_device.minor = MISC_DYNAMIC_MINOR;
> > +     hps->misc_device.name = "hps";
>
> Does "cros_hps_i2c" or "cros_hps" make more sense?

Changed to "cros-hps" to better match the driver's name (and the
naming convention of other Chrome OS drivers).

> > +     ret = devm_add_action(&client->dev, &hps_unload, hps);
> > +     if (ret) {
> > +             dev_err(&client->dev,
> > +                     "failed to install unload action: %d\n", ret);
> > +             return ret;
> > +     }
>
> Why does it need to call hps_unload() when device removing?  Couldn't it put
> the code in hps_i2c_remove()?

Ah, this was left over from an earlier version where the
setup/teardown sequence was a little more complex. Agreed and moved to
hps_i2c_remove.

>
> > +     hps_set_power(hps, false);
> > +     pm_runtime_enable(&client->dev);
> > +     return ret;
>
> Using `return 0;` makes it clear.

Done.

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

> > +static const struct i2c_device_id hps_i2c_id[] = {
> > +     { "hps", 0 },
>
> Does "cros_hps_i2c" or "cros_hps" make more sense?

Went with "cros-hps".

> > +static struct i2c_driver hps_i2c_driver = {
> > +     .probe_new = hps_i2c_probe,
> > +     .remove = hps_i2c_remove,
> > +     .id_table = hps_i2c_id,
> > +     .driver = {
> > +             .name = "hps",
>
> Does "cros_hps_i2c" or "cros_hps" make more sense?

Ditto.

> > +#ifdef CONFIG_ACPI
> > +             .acpi_match_table = ACPI_PTR(hps_acpi_id),
> > +#endif
>
> It doesn't need the guard as ACPI_PTR() already does.

Ah, good to know!

- Sami

Powered by blists - more mailing lists