Message-ID: <CAPuLczsKqFOsqf-d39HOMHZaFQb2vmv054HUOj2+amQGE=oPkw@mail.gmail.com>
Date:   Fri, 28 Jan 2022 18:41:25 +1100
From:   Sami Kyostila <skyostil@...omium.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     LKML <linux-kernel@...r.kernel.org>, dtor@...omium.org,
        evanbenn@...omium.org, arnd@...db.de
Subject: Re: [PATCH 1/2] drivers/misc: add a driver for HPS

to 27. tammik. 2022 klo 20.40 Greg KH (gregkh@...uxfoundation.org) kirjoitti:
>
> 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?

Sure, here's a small example that shows how to read a magic register
value from the device:
https://gist.github.com/skyostil/13d60a288919d18d00b20e81dfe018cd

(Let me know if you'd prefer this to be checked into the tree somewhere.)

Here's also a patch that makes the production daemon use this
interface: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3353640

> >
> > Signed-off-by: Sami Kyöstilä <skyostil@...omium.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@...omium.org>
> > +R:   Evan Benn <evanbenn@...omium.org>
> > +S:   Maintained
> > +F:   drivers/misc/hps-i2c.c
> > +
> >  HSI SUBSYSTEM
> >  M:   Sebastian Reichel <sre@...nel.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.

Will do, thanks for the tip!

>
> thanks,
>
> greg k-h



- Sami

Powered by blists - more mailing lists