* [PATCH v3 0/1] Add a driver for the ChromeOS screen privacy sensor (HPS)
@ 2022-02-07 1:36 Sami Kyöstilä
2022-02-07 1:36 ` [PATCH v3 1/1] platform/chrome: add a driver for HPS Sami Kyöstilä
0 siblings, 1 reply; 6+ messages in thread
From: Sami Kyöstilä @ 2022-02-07 1:36 UTC (permalink / raw)
To: LKML; +Cc: bleung, arnd, gregkh, evanbenn, Sami Kyöstilä
This series adds a driver for the ChromeOS screen privacy sensor
(a.k.a. HPS) device, attached on the I2C bus. The sensor is a
hardware-isolated module which reports a high-level presence signal,
e.g., whether there is person in front of the computer or not.
The driver exposes the device to userspace as a character device, which
can be used to control the power state of the device. The driver
automatically turns power off to the sensor unless a process is using
it.
More information about HPS, its firmware and communication protocol can
be found at
https://chromium.googlesource.com/chromiumos/platform/hps-firmware.
Changes in v3:
- Moved from drivers/misc to drivers/platform/chrome.
Changes in v2:
- Removed custom ioctl interface.
- Reworked to use miscdev.
Sami Kyöstilä (1):
platform/chrome: add a driver for HPS
MAINTAINERS | 6 +
drivers/platform/chrome/Kconfig | 10 ++
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_hps_i2c.c | 184 +++++++++++++++++++++++++
4 files changed, 201 insertions(+)
create mode 100644 drivers/platform/chrome/cros_hps_i2c.c
--
2.35.0.263.gb82422642f-goog
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/1] platform/chrome: add a driver for HPS
2022-02-07 1:36 [PATCH v3 0/1] Add a driver for the ChromeOS screen privacy sensor (HPS) Sami Kyöstilä
@ 2022-02-07 1:36 ` Sami Kyöstilä
2022-02-10 5:41 ` Tzung-Bi Shih
0 siblings, 1 reply; 6+ messages in thread
From: Sami Kyöstilä @ 2022-02-07 1:36 UTC (permalink / raw)
To: LKML; +Cc: bleung, arnd, gregkh, evanbenn, Sami Kyöstilä
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.
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.
The device firmware, I2C protocol and other documentation is available
at https://chromium.googlesource.com/chromiumos/platform/hps-firmware.
Signed-off-by: Sami Kyöstilä <skyostil@chromium.org>
---
Changes in v3:
- Moved from drivers/misc to drivers/platform/chrome.
Changes in v2:
- Removed custom ioctl interface.
- Reworked to use miscdev.
MAINTAINERS | 6 +
drivers/platform/chrome/Kconfig | 10 ++
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_hps_i2c.c | 184 +++++++++++++++++++++++++
4 files changed, 201 insertions(+)
create mode 100644 drivers/platform/chrome/cros_hps_i2c.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 69a2935daf6c..94a8c178598e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8800,6 +8800,12 @@ S: Maintained
W: http://artax.karlin.mff.cuni.cz/~mikulas/vyplody/hpfs/index-e.cgi
F: fs/hpfs/
+HPS (ChromeOS screen privacy sensor) DRIVER
+M: Sami Kyöstilä <skyostil@chromium.org>
+R: Evan Benn <evanbenn@chromium.org>
+S: Maintained
+F: drivers/platform/chrome/cros_hps_i2c.c
+
HSI SUBSYSTEM
M: Sebastian Reichel <sre@kernel.org>
S: Maintained
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index ccc23d8686e8..a20da7b638df 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -217,6 +217,16 @@ config CROS_EC_TYPEC
To compile this driver as a module, choose M here: the module will be
called cros_ec_typec.
+config CROS_HPS_I2C
+ tristate "ChromeOS HPS device"
+ depends on HID && I2C && PM
+ help
+ Say Y here if you want to enable support for the ChromeOS
+ screen privacy sensor (HPS), attached via I2C. The driver supports a
+ sensor connected to the I2C bus and exposes it as a character device.
+ To save power, the sensor is automatically powered down when no
+ clients are accessing it.
+
config CROS_USBPD_LOGGER
tristate "Logging driver for USB PD charger"
depends on CHARGER_CROS_USBPD
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index f901d2e43166..66bd745d5931 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_CROS_EC_DEBUGFS) += cros_ec_debugfs.o
cros-ec-sensorhub-objs := cros_ec_sensorhub.o cros_ec_sensorhub_ring.o cros_ec_trace.o
obj-$(CONFIG_CROS_EC_SENSORHUB) += cros-ec-sensorhub.o
obj-$(CONFIG_CROS_EC_SYSFS) += cros_ec_sysfs.o
+obj-$(CONFIG_CROS_HPS_I2C) += cros_hps_i2c.o
obj-$(CONFIG_CROS_USBPD_LOGGER) += cros_usbpd_logger.o
obj-$(CONFIG_CROS_USBPD_NOTIFY) += cros_usbpd_notify.o
diff --git a/drivers/platform/chrome/cros_hps_i2c.c b/drivers/platform/chrome/cros_hps_i2c.c
new file mode 100644
index 000000000000..9071c9324fa7
--- /dev/null
+++ b/drivers/platform/chrome/cros_hps_i2c.c
@@ -0,0 +1,184 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for the ChromeOS screen privacy sensor (HPS), attached via I2C.
+ *
+ * The driver exposes HPS as a character device, although currently no read or
+ * write operations are supported. Instead, the driver only controls the power
+ * state of the sensor, keeping it on only while userspace holds an open file
+ * descriptor to the HPS device.
+ *
+ * Copyright 2022 Google LLC.
+ */
+
+#include <linux/acpi.h>
+#include <linux/fs.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+
+#define HPS_ACPI_ID "GOOG0020"
+
+struct hps_drvdata {
+ struct i2c_client *client;
+ struct miscdevice misc_device;
+ struct gpio_desc *enable_gpio;
+};
+
+static void hps_set_power(struct hps_drvdata *hps, bool state)
+{
+ if (!IS_ERR_OR_NULL(hps->enable_gpio))
+ gpiod_set_value_cansleep(hps->enable_gpio, state);
+}
+
+static void hps_unload(void *drv_data)
+{
+ struct hps_drvdata *hps = drv_data;
+
+ hps_set_power(hps, true);
+}
+
+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);
+ return ret;
+}
+
+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);
+ return ret;
+}
+
+const struct file_operations hps_fops = {
+ .owner = THIS_MODULE,
+ .open = hps_open,
+ .release = hps_release,
+};
+
+static int hps_i2c_probe(struct i2c_client *client)
+{
+ struct hps_drvdata *hps;
+ int ret = 0;
+
+ hps = devm_kzalloc(&client->dev, sizeof(*hps), GFP_KERNEL);
+ if (!hps)
+ return -ENOMEM;
+
+ 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";
+ hps->misc_device.fops = &hps_fops;
+
+ i2c_set_clientdata(client, hps);
+ hps->client = client;
+ hps->enable_gpio = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_HIGH);
+ if (IS_ERR(hps->enable_gpio)) {
+ ret = PTR_ERR(hps->enable_gpio);
+ dev_err(&client->dev, "failed to get enable gpio: %d\n", ret);
+ return ret;
+ }
+
+ 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;
+ }
+
+ ret = misc_register(&hps->misc_device);
+ if (ret) {
+ dev_err(&client->dev, "failed to initialize misc device: %d\n", ret);
+ return ret;
+ }
+
+ hps_set_power(hps, false);
+ pm_runtime_enable(&client->dev);
+ return ret;
+}
+
+static int hps_i2c_remove(struct i2c_client *client)
+{
+ struct hps_drvdata *hps = i2c_get_clientdata(client);
+
+ pm_runtime_disable(&client->dev);
+ misc_deregister(&hps->misc_device);
+ return 0;
+}
+
+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;
+}
+static UNIVERSAL_DEV_PM_OPS(hps_pm_ops, hps_suspend, hps_resume, NULL);
+
+static const struct i2c_device_id hps_i2c_id[] = {
+ { "hps", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, hps_i2c_id);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id hps_acpi_id[] = {
+ { HPS_ACPI_ID, 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, hps_acpi_id);
+#endif /* CONFIG_ACPI */
+
+static struct i2c_driver hps_i2c_driver = {
+ .probe_new = hps_i2c_probe,
+ .remove = hps_i2c_remove,
+ .id_table = hps_i2c_id,
+ .driver = {
+ .name = "hps",
+ .pm = &hps_pm_ops,
+#ifdef CONFIG_ACPI
+ .acpi_match_table = ACPI_PTR(hps_acpi_id),
+#endif
+ },
+};
+module_i2c_driver(hps_i2c_driver);
+
+MODULE_ALIAS("acpi:" HPS_ACPI_ID);
+MODULE_AUTHOR("Sami Kyöstilä <skyostil@chromium.org>");
+MODULE_DESCRIPTION("Driver for ChromeOS HPS");
+MODULE_LICENSE("GPL");
--
2.35.0.263.gb82422642f-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] platform/chrome: add a driver for HPS
2022-02-07 1:36 ` [PATCH v3 1/1] platform/chrome: add a driver for HPS Sami Kyöstilä
@ 2022-02-10 5:41 ` Tzung-Bi Shih
2022-02-11 9:08 ` Sami Kyostila
0 siblings, 1 reply; 6+ messages in thread
From: Tzung-Bi Shih @ 2022-02-10 5:41 UTC (permalink / raw)
To: Sami Kyöstilä; +Cc: LKML, bleung, arnd, gregkh, evanbenn
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.
The patch uses HPS instead of SPS everywhere. Would you consider to use
"human presence sensor" when referring it?
> 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?
[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?
> 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?
> +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?
> +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.
> +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.
> +const struct file_operations hps_fops = {
> + .owner = THIS_MODULE,
> + .open = hps_open,
> + .release = hps_release,
> +};
The struct can be static.
> +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.
> + 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?
> + 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()?
> + hps_set_power(hps, false);
> + pm_runtime_enable(&client->dev);
> + return ret;
Using `return 0;` makes it clear.
> +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.
> +static const struct i2c_device_id hps_i2c_id[] = {
> + { "hps", 0 },
Does "cros_hps_i2c" or "cros_hps" make more sense?
> +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?
> +#ifdef CONFIG_ACPI
> + .acpi_match_table = ACPI_PTR(hps_acpi_id),
> +#endif
It doesn't need the guard as ACPI_PTR() already does.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] platform/chrome: add a driver for HPS
2022-02-10 5:41 ` Tzung-Bi Shih
@ 2022-02-11 9:08 ` Sami Kyostila
2022-02-14 1:48 ` Tzung-Bi Shih
0 siblings, 1 reply; 6+ messages in thread
From: Sami Kyostila @ 2022-02-11 9:08 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: LKML, bleung, arnd, gregkh, evanbenn
( 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:
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] platform/chrome: add a driver for HPS
2022-02-11 9:08 ` Sami Kyostila
@ 2022-02-14 1:48 ` Tzung-Bi Shih
2022-02-14 8:46 ` Sami Kyostila
0 siblings, 1 reply; 6+ messages in thread
From: Tzung-Bi Shih @ 2022-02-14 1:48 UTC (permalink / raw)
To: Sami Kyostila; +Cc: LKML, bleung, arnd, gregkh, evanbenn, chrome-platform
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).
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] platform/chrome: add a driver for HPS
2022-02-14 1:48 ` Tzung-Bi Shih
@ 2022-02-14 8:46 ` Sami Kyostila
0 siblings, 0 replies; 6+ messages in thread
From: Sami Kyostila @ 2022-02-14 8:46 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: LKML, bleung, arnd, gregkh, evanbenn, chrome-platform
ma 14. helmik. 2022 klo 12.48 Tzung-Bi Shih (tzungbi@google.com) kirjoitti:
>
> 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.
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@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: [...]
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-14 8:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-07 1:36 [PATCH v3 0/1] Add a driver for the ChromeOS screen privacy sensor (HPS) Sami Kyöstilä
2022-02-07 1:36 ` [PATCH v3 1/1] platform/chrome: add a driver for HPS Sami Kyöstilä
2022-02-10 5:41 ` Tzung-Bi Shih
2022-02-11 9:08 ` Sami Kyostila
2022-02-14 1:48 ` Tzung-Bi Shih
2022-02-14 8:46 ` Sami Kyostila
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).