Re: [PATCH v3 1/1] platform/chrome: add a driver for HPS

[Tzung-Bi Shih]

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.