Re: [PATCH 1/2] drivers/misc: add a driver for HPS

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Jan 27, 2022 at 07:35:44PM +1100, Sami Kyöstilä wrote:
> This patch introduces a driver for the ChromeOS snooping protection
> 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 initial version of the device only supports power
> management, i.e., communicating with the sensor must be done through I2C
> 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.

How about a userspace tool that interacts with this new ioctl interface
as well so that we can understand how the driver is supposed to work?


> 
> Signed-off-by: Sami Kyöstilä <skyostil@chromium.org>
> ---
> 
>  MAINTAINERS            |   6 ++
>  drivers/misc/Kconfig   |  10 ++
>  drivers/misc/Makefile  |   1 +
>  drivers/misc/hps-i2c.c | 223 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 240 insertions(+)
>  create mode 100644 drivers/misc/hps-i2c.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ea3e6c914384..9dea4b8c2ab5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8798,6 +8798,12 @@ S:	Maintained
>  W:	http://artax.karlin.mff.cuni.cz/~mikulas/vyplody/hpfs/index-e.cgi
>  F:	fs/hpfs/
>  
> +HPS (ChromeOS snooping protection sensor) DRIVER
> +M:	Sami Kyöstilä <skyostil@chromium.org>
> +R:	Evan Benn <evanbenn@chromium.org>
> +S:	Maintained
> +F:	drivers/misc/hps-i2c.c
> +
>  HSI SUBSYSTEM
>  M:	Sebastian Reichel <sre@kernel.org>
>  S:	Maintained
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 0f5a49fc7c9e..b48b7803f537 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -244,6 +244,16 @@ config HP_ILO
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called hpilo.
>  
> +config HPS_I2C
> +	tristate "ChromeOS HPS device support"
> +	depends on HID && I2C && PM
> +	help
> +	  Say Y here if you want to enable support for the ChromeOS
> +	  anti-snooping 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 QCOM_COINCELL
>  	tristate "Qualcomm coincell charger support"
>  	depends on MFD_SPMI_PMIC || COMPILE_TEST
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index a086197af544..162a7d530dab 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_SGI_GRU)		+= sgi-gru/
>  obj-$(CONFIG_CS5535_MFGPT)	+= cs5535-mfgpt.o
>  obj-$(CONFIG_GEHC_ACHC)		+= gehc-achc.o
>  obj-$(CONFIG_HP_ILO)		+= hpilo.o
> +obj-$(CONFIG_HPS_I2C)		+= hps-i2c.o
>  obj-$(CONFIG_APDS9802ALS)	+= apds9802als.o
>  obj-$(CONFIG_ISL29003)		+= isl29003.o
>  obj-$(CONFIG_ISL29020)		+= isl29020.o
> diff --git a/drivers/misc/hps-i2c.c b/drivers/misc/hps-i2c.c
> new file mode 100644
> index 000000000000..fe9f073b0352
> --- /dev/null
> +++ b/drivers/misc/hps-i2c.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the ChromeOS anti-snooping 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/cdev.h>
> +#include <linux/fs.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +
> +#define HPS_ACPI_ID		"GOOG0020"
> +#define HPS_MAX_DEVICES		1
> +
> +struct hps_drvdata {
> +	struct i2c_client *client;
> +
> +	struct cdev cdev;
> +	struct class *cdev_class;

As you only have 1 device, please just use the miscdev interface, not
the chardev interface.  Makes your code much smaller and easier to
review and maintain over time and does not "burn" a whole major number
for this tiny driver.

thanks,

greg k-h