* [PATCH v5 0/1] Add a driver for the ChromeOS human presence sensor (HPS) @ 2022-10-12 4:09 Dan Callaghan 2022-10-12 4:09 ` [PATCH v5 1/1] platform/chrome: add a driver for HPS Dan Callaghan 0 siblings, 1 reply; 5+ messages in thread From: Dan Callaghan @ 2022-10-12 4:09 UTC (permalink / raw) To: chrome-platform Cc: LKML, Sami Kyöstilä, Benson Leung, Tzung-Bi Shih, Dan Callaghan This series adds a driver for the ChromeOS human presence 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 v5: - Updated MAINTAINERS. Changes in v4: - Simplified open/release pm logic. - Renamed device to "cros-hps". - Stylistics cleanups. 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 | 151 +++++++++++++++++++++++++ 4 files changed, 168 insertions(+) create mode 100644 drivers/platform/chrome/cros_hps_i2c.c base-commit: 8edd2752b0aa498b3a61f3caee8f79f7e0567fad -- 2.38.0.rc1.362.ged0d419d3c-goog ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v5 1/1] platform/chrome: add a driver for HPS 2022-10-12 4:09 [PATCH v5 0/1] Add a driver for the ChromeOS human presence sensor (HPS) Dan Callaghan @ 2022-10-12 4:09 ` Dan Callaghan 2022-10-12 8:46 ` Tzung-Bi Shih 0 siblings, 1 reply; 5+ messages in thread From: Dan Callaghan @ 2022-10-12 4:09 UTC (permalink / raw) To: chrome-platform Cc: LKML, Sami Kyöstilä, Benson Leung, Tzung-Bi Shih, Dan Callaghan From: Sami Kyöstilä <skyostil@chromium.org> This patch introduces a driver for the ChromeOS human presence 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> Signed-off-by: Dan Callaghan <dcallagh@chromium.org> --- Changes in v5: - Updated MAINTAINERS. Changes in v4: - Simplified open/release pm logic. - Renamed device to "cros-hps". - Stylistics cleanups. 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 | 151 +++++++++++++++++++++++++ 4 files changed, 168 insertions(+) create mode 100644 drivers/platform/chrome/cros_hps_i2c.c diff --git a/MAINTAINERS b/MAINTAINERS index c1e4977cb1c3..13bfab399219 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4903,6 +4903,12 @@ S: Maintained F: drivers/platform/chrome/cros_usbpd_notify.c F: include/linux/platform_data/cros_usbpd_notify.h +CHROMEOS HPS DRIVER +M: Dan Callaghan <dcallagh@chromium.org> +R: Sami Kyöstilä <skyostil@chromium.org> +S: Maintained +F: drivers/platform/chrome/cros_hps_i2c.c + CHRONTEL CH7322 CEC DRIVER M: Joe Tessler <jrt@google.com> L: linux-media@vger.kernel.org diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig index 6b954c5acadb..c1ca247987d2 100644 --- a/drivers/platform/chrome/Kconfig +++ b/drivers/platform/chrome/Kconfig @@ -228,6 +228,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 + human presence 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 2950610101f1..f6068d077a40 100644 --- a/drivers/platform/chrome/Makefile +++ b/drivers/platform/chrome/Makefile @@ -27,6 +27,7 @@ obj-$(CONFIG_CROS_EC_DEBUGFS) += cros_ec_debugfs.o cros-ec-sensorhub-objs := cros_ec_sensorhub.o cros_ec_sensorhub_ring.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..a5505951f989 --- /dev/null +++ b/drivers/platform/chrome/cros_hps_i2c.c @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Driver for the ChromeOS human presence 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) +{ + gpiod_set_value_cansleep(hps->enable_gpio, state); +} + +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; + + return pm_runtime_resume_and_get(dev); +} + +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; + + return pm_runtime_put(dev); +} + +static 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; + + 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 = "cros-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 = 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 0; +} + +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); + hps_set_power(hps, true); + 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[] = { + { "cros-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 = "cros-hps", + .pm = &hps_pm_ops, + .acpi_match_table = ACPI_PTR(hps_acpi_id), + }, +}; +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.38.0.rc1.362.ged0d419d3c-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v5 1/1] platform/chrome: add a driver for HPS 2022-10-12 4:09 ` [PATCH v5 1/1] platform/chrome: add a driver for HPS Dan Callaghan @ 2022-10-12 8:46 ` Tzung-Bi Shih 2022-10-13 4:29 ` Dan Callaghan 0 siblings, 1 reply; 5+ messages in thread From: Tzung-Bi Shih @ 2022-10-12 8:46 UTC (permalink / raw) To: Dan Callaghan Cc: chrome-platform, LKML, Sami Kyöstilä, Benson Leung On Wed, Oct 12, 2022 at 03:09:18PM +1100, Dan Callaghan wrote: > --- It doesn't need a cover letter if the series only has 1 patch in general. Instead, it could put additional information (and changelogs) after "---". > diff --git a/drivers/platform/chrome/cros_hps_i2c.c b/drivers/platform/chrome/cros_hps_i2c.c [...] > +static int hps_i2c_probe(struct i2c_client *client) > +{ > + struct hps_drvdata *hps; > + int ret; > + > + hps = devm_kzalloc(&client->dev, sizeof(*hps), GFP_KERNEL); > + if (!hps) > + return -ENOMEM; > + > + memset(&hps->misc_device, 0, sizeof(hps->misc_device)); The memset can be dropped. `hps` is z-allocated. > + hps->misc_device.parent = &client->dev; > + hps->misc_device.minor = MISC_DYNAMIC_MINOR; > + hps->misc_device.name = "cros-hps"; > + hps->misc_device.fops = &hps_fops; > + > + i2c_set_clientdata(client, hps); > + hps->client = client; To be neat, I would prefer to insert a blank line here. > + 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 = 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); IIUC, the GPIO will raise to HIGH in the first place, and then fall to LOW until here. Is it an expected behavior? How about gpiod_get() with GPIOD_OUT_LOW? > +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); > + hps_set_power(hps, true); Why does it need to raise the GPIO again when removing the device? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 1/1] platform/chrome: add a driver for HPS 2022-10-12 8:46 ` Tzung-Bi Shih @ 2022-10-13 4:29 ` Dan Callaghan 2022-10-13 5:49 ` Tzung-Bi Shih 0 siblings, 1 reply; 5+ messages in thread From: Dan Callaghan @ 2022-10-13 4:29 UTC (permalink / raw) To: Tzung-Bi Shih Cc: chrome-platform, LKML, Sami Kyöstilä, Benson Leung Excerpts from Tzung-Bi Shih’s message of 2022-10-12 19:46:51 +1100: > On Wed, Oct 12, 2022 at 03:09:18PM +1100, Dan Callaghan wrote: > > --- > > It doesn't need a cover letter if the series only has 1 patch in general. > Instead, it could put additional information (and changelogs) after "---". Understood, I'll omit the cover letter in future postings. > > diff --git a/drivers/platform/chrome/cros_hps_i2c.c b/drivers/platform/chrome/cros_hps_i2c.c > [...] > > +static int hps_i2c_probe(struct i2c_client *client) > > +{ > > + struct hps_drvdata *hps; > > + int ret; > > + > > + hps = devm_kzalloc(&client->dev, sizeof(*hps), GFP_KERNEL); > > + if (!hps) > > + return -ENOMEM; > > + > > + memset(&hps->misc_device, 0, sizeof(hps->misc_device)); > > The memset can be dropped. `hps` is z-allocated. I'll take this out. > > + hps->misc_device.parent = &client->dev; > > + hps->misc_device.minor = MISC_DYNAMIC_MINOR; > > + hps->misc_device.name = "cros-hps"; > > + hps->misc_device.fops = &hps_fops; > > + > > + i2c_set_clientdata(client, hps); > > + hps->client = client; > > To be neat, I would prefer to insert a blank line here. Sure, will add one. > > + 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 = 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); > > IIUC, the GPIO will raise to HIGH in the first place, and then fall > to LOW until here. Is it an expected behavior? How about gpiod_get() > with GPIOD_OUT_LOW? It might seem a little unusual, but it is intentional. The enable line is already high when we enter the kernel from firmware. Acquiring the GPIO line with GPIOD_OUT_HIGH preserves its existing state (high) in case later steps fail. We power off the periphal only once the driver is successfully bound and has taken control of its power state. > > +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); > > + hps_set_power(hps, true); > > Why does it need to raise the GPIO again when removing the device? Similar to the above, we want to preserve the default power state (i.e. powered on) whenever the driver is not bound to the device. This behaviour made sense to us mainly because we were originally controlling the peripheral entirely from userspace, so it was always powered on by default. Do you think this behaviour is acceptable, or do we need to change it? -- Dan Callaghan <dcallagh@chromium.org> Software Engineer, Google ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 1/1] platform/chrome: add a driver for HPS 2022-10-13 4:29 ` Dan Callaghan @ 2022-10-13 5:49 ` Tzung-Bi Shih 0 siblings, 0 replies; 5+ messages in thread From: Tzung-Bi Shih @ 2022-10-13 5:49 UTC (permalink / raw) To: Dan Callaghan Cc: chrome-platform, LKML, Sami Kyöstilä, Benson Leung On Wed, Oct 12, 2022 at 09:29:03PM -0700, Dan Callaghan wrote: > Excerpts from Tzung-Bi Shih’s message of 2022-10-12 19:46:51 +1100: > > On Wed, Oct 12, 2022 at 03:09:18PM +1100, Dan Callaghan wrote: > > > + 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 = 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); > > > > IIUC, the GPIO will raise to HIGH in the first place, and then fall > > to LOW until here. Is it an expected behavior? How about gpiod_get() > > with GPIOD_OUT_LOW? > > It might seem a little unusual, but it is intentional. The enable line is > already high when we enter the kernel from firmware. Acquiring the GPIO > line with GPIOD_OUT_HIGH preserves its existing state (high) in case later > steps fail. > > We power off the periphal only once the driver is successfully bound and has > taken control of its power state. I see. Please put some context comments before calling devm_gpiod_get(). > > > +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); > > > + hps_set_power(hps, true); > > > > Why does it need to raise the GPIO again when removing the device? > > Similar to the above, we want to preserve the default power state > (i.e. powered on) whenever the driver is not bound to the device. > > This behaviour made sense to us mainly because we were originally controlling > the peripheral entirely from userspace, so it was always powered on by default. > > Do you think this behaviour is acceptable, or do we need to change it? I think it's fine. Please put some context comments before calling hps_set_power(). ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-10-13 5:50 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-12 4:09 [PATCH v5 0/1] Add a driver for the ChromeOS human presence sensor (HPS) Dan Callaghan 2022-10-12 4:09 ` [PATCH v5 1/1] platform/chrome: add a driver for HPS Dan Callaghan 2022-10-12 8:46 ` Tzung-Bi Shih 2022-10-13 4:29 ` Dan Callaghan 2022-10-13 5:49 ` Tzung-Bi Shih
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).