Message-ID: <44ff8e33-3300-4753-ea40-fa26d21cdd46@collabora.com>
Date:   Tue, 22 Oct 2019 10:32:43 +0200
From:   Enric Balletbo i Serra <enric.balletbo@...labora.com>
To:     Jonathan Cameron <jic23@...nel.org>,
        Gwendal Grignou <gwendal@...omium.org>
Cc:     briannorris@...omium.org, knaack.h@....de, lars@...afoo.de,
        pmeerw@...erw.net, lee.jones@...aro.org, bleung@...omium.org,
        dianders@...omium.org, groeck@...omium.org,
        fabien.lahoudere@...labora.com, linux-kernel@...r.kernel.org,
        linux-iio@...r.kernel.org, Nick Crews <ncrews@...omium.org>
Subject: Re: [PATCH v2 03/18] platform: cros_ec: Add cros_ec_sensor_hub driver

Hi Gwendal,

Complementing the Jonathan's review, few bits more.

On 21/10/19 17:59, Jonathan Cameron wrote:
> On Sun, 20 Oct 2019 22:53:48 -0700
> Gwendal Grignou <gwendal@...omium.org> wrote:
> 
>> Similar to HID sensor stack, the new driver sits between cros_ec_dev
>> and the iio device drivers:
>>
>> EC based iio device topology would be:
>> iio:device1 ->
>> ...0/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.6.auto/
>>                                          cros-ec-sensorhub.7.auto/
>>                                          cros-ec-accel.15.auto/
>>                                          iio:device1
>>
>> It will be expanded to control EC sensor FIFO.
>>
>> Signed-off-by: Gwendal Grignou <gwendal@...omium.org>
> 
> A few bits and pieces inline.
> 
> Thanks,
> 
> Jonathan
> 
> 
>> ---
>> Changes in v2:
>> - Remove unerelated changes.
>> - Fix spelling.
>> - Use !x instead of x == NULL
>> - Use platform_ API directly to register IIO sensors from
>>   cros_ec_sensorhub.
>>
>>  drivers/iio/common/cros_ec_sensors/Kconfig    |   2 +-
>>  drivers/platform/chrome/Kconfig               |  12 ++
>>  drivers/platform/chrome/Makefile              |   1 +
>>  drivers/platform/chrome/cros_ec_sensorhub.c   | 202 ++++++++++++++++++
>>  .../linux/platform_data/cros_ec_sensorhub.h   |  21 ++
>>  5 files changed, 237 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/platform/chrome/cros_ec_sensorhub.c
>>  create mode 100644 include/linux/platform_data/cros_ec_sensorhub.h
>>
>> diff --git a/drivers/iio/common/cros_ec_sensors/Kconfig b/drivers/iio/common/cros_ec_sensors/Kconfig
>> index cdbb29cfb9076..fefad95727907 100644
>> --- a/drivers/iio/common/cros_ec_sensors/Kconfig
>> +++ b/drivers/iio/common/cros_ec_sensors/Kconfig
>> @@ -4,7 +4,7 @@
>>  #
>>  config IIO_CROS_EC_SENSORS_CORE
>>  	tristate "ChromeOS EC Sensors Core"
>> -	depends on SYSFS && CROS_EC
>> +	depends on SYSFS && CROS_EC_SENSORHUB
>>  	select IIO_BUFFER
>>  	select IIO_TRIGGERED_BUFFER
>>  	help
>> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
>> index ee5f08ea57b6c..56a25317a6bee 100644
>> --- a/drivers/platform/chrome/Kconfig
>> +++ b/drivers/platform/chrome/Kconfig
>> @@ -190,6 +190,18 @@ config CROS_EC_DEBUGFS
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called cros_ec_debugfs.
>>  
>> +config CROS_EC_SENSORHUB
>> +	tristate "ChromeOS EC MEMS Sensor Hub"
>> +	depends on CROS_EC && IIO
>> +	help
>> +	  Allow loading IIO sensors. This driver is loaded by MFD and will in
>> +	  turn query the EC and register the sensors.
>> +	  It also spreads the sensor data coming from the EC to the IIO sensor
>> +	  object.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called cros_ec_sensorhub.
>> +
>>  config CROS_EC_SYSFS
>>  	tristate "ChromeOS EC control and information through sysfs"
>>  	depends on MFD_CROS_EC_DEV && SYSFS
>> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
>> index 477ec3d1d1c98..a164c40dc0996 100644
>> --- a/drivers/platform/chrome/Makefile
>> +++ b/drivers/platform/chrome/Makefile
>> @@ -17,6 +17,7 @@ obj-$(CONFIG_CROS_EC_PROTO)		+= cros_ec_proto.o cros_ec_trace.o
>>  obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT)	+= cros_kbd_led_backlight.o
>>  obj-$(CONFIG_CROS_EC_CHARDEV)		+= cros_ec_chardev.o
>>  obj-$(CONFIG_CROS_EC_LIGHTBAR)		+= cros_ec_lightbar.o
>> +obj-$(CONFIG_CROS_EC_SENSORHUB)		+= cros_ec_sensorhub.o
>>  obj-$(CONFIG_CROS_EC_VBC)		+= cros_ec_vbc.o
>>  obj-$(CONFIG_CROS_EC_DEBUGFS)		+= cros_ec_debugfs.o
>>  obj-$(CONFIG_CROS_EC_SYSFS)		+= cros_ec_sysfs.o
>> diff --git a/drivers/platform/chrome/cros_ec_sensorhub.c b/drivers/platform/chrome/cros_ec_sensorhub.c
>> new file mode 100644
>> index 0000000000000..5fea4c28c5c95
>> --- /dev/null
>> +++ b/drivers/platform/chrome/cros_ec_sensorhub.c
>> @@ -0,0 +1,202 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * SensorHub: driver that discover sensors behind
>> + * a ChromeOS Embedded controller.
>> + *
>> + * Copyright 2019 Google LLC
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/device.h>
>> +#include <linux/fs.h>
>> +#include <linux/miscdevice.h>

The two includes above are not needed.

>> +#include <linux/module.h>
>> +#include <linux/mfd/cros_ec.h>
>> +#include <linux/platform_data/cros_ec_commands.h>
>> +#include <linux/platform_data/cros_ec_proto.h>
>> +#include <linux/platform_device.h>

>> +#include <linux/poll.h>

Not needed.

>> +#include <linux/slab.h>

>> +#include <linux/types.h>
>> +#include <linux/uaccess.h>

Also, these two are not needed.

>> +
>> +#include <linux/platform_data/cros_ec_sensorhub.h>
>> +
>> +#define DRV_NAME		"cros-ec-sensorhub"
>> +
>> +

Please don't use multiple blank lines

For new files introduced in chrome/platform I'd like if you can fix the issues
reported by checkpatch with the --strict option for v3. I'm not going to report
these issues below (also, is my preference but optional)

>> +static struct device_type cros_ec_sensorhub_dev_type = {
>> +	.name	= "cros_ec_iio_sensor",
>> +};
>> +
>> +static int cros_ec_sensorhub_allocate_single_sensor(
>> +		struct device *parent,
>> +		char *sensor_name,
>> +		int sensor_num)
>> +{
>> +	struct platform_device *pdev;
>> +	struct cros_ec_sensor_platform sensor_platforms = {
>> +		.sensor_num = sensor_num,
>> +	};
>> +	int ret;
>> +
>> +	pdev = platform_device_alloc(sensor_name, PLATFORM_DEVID_AUTO);
>> +	if (!pdev)
>> +		return -ENOMEM;
>> +
>> +	pdev->dev.parent = parent;
>> +	pdev->dev.type = &cros_ec_sensorhub_dev_type;
>> +
>> +	ret = platform_device_add_data(pdev, &sensor_platforms,
>> +			sizeof(sensor_platforms));
>> +	if (ret)
>> +		goto fail_device;
>> +
>> +	ret = platform_device_add(pdev);
>> +	if (ret)
>> +		goto fail_device;
>> +
>> +	return 0;
>> +
>> +fail_device:
>> +	platform_device_put(pdev);
>> +	return ret;

Instead of doing alloc, add_data, device_add, can we just do a single step with
platform_device_register_data ? Similar to what we did in
drivers/platform/chrome/wilco_ec/core.c should work I guess (cc'ing Nick)

Also, we need to store the created devices and free on remove. I think this is
not implemented.


>> +}
>> +
>> +static int cros_ec_sensorhub_register(struct device *dev,
>> +		struct cros_ec_dev *ec)
>> +{
>> +	int ret, i, id, sensor_num;
>> +	int sensor_type[MOTIONSENSE_TYPE_MAX] = { 0 };
>> +	struct ec_params_motion_sense *params;
>> +	struct ec_response_motion_sense *resp;
>> +	struct cros_ec_command *msg;
>> +	char *name;
>> +
>> +	sensor_num = cros_ec_get_sensor_count(ec);
>> +	if (sensor_num < 0) {
>> +		dev_err(dev,
>> +			"Unable to retrieve sensor information (err:%d)\n",
>> +			sensor_num);
>> +		return sensor_num;
>> +	}
>> +
>> +	if (sensor_num == 0) {
>> +		dev_err(dev, "Zero sensors reported.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Prepare a message to send INFO command to each sensor. */
>> +	msg = kzalloc(sizeof(struct cros_ec_command) +
>> +		      max(sizeof(*params), sizeof(*resp)), GFP_KERNEL);
>> +	if (!msg) {
>> +		ret = -ENOMEM;
>> +		goto error;
> 
> If you get here, the kzalloc failed, so there is nothing to free.
> Hence should just be a return -ENOMEM I think.
> 
>> +	}
>> +
>> +	msg->version = 1;
>> +	msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
>> +	msg->outsize = sizeof(*params);
>> +	msg->insize = sizeof(*resp);
>> +	params = (struct ec_params_motion_sense *)msg->data;
>> +	resp = (struct ec_response_motion_sense *)msg->data;
>> +
>> +	id = 0;
>> +	for (i = 0; i < sensor_num; i++) {
>> +		params->cmd = MOTIONSENSE_CMD_INFO;
>> +		params->info.sensor_num = i;
>> +		ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
>> +		if (ret < 0) {
>> +			dev_warn(dev, "no info for EC sensor %d : %d/%d\n",
>> +				 i, ret, msg->result);
>> +			continue;
>> +		}
>> +		switch (resp->info.type) {
>> +		case MOTIONSENSE_TYPE_ACCEL:
>> +			name = "cros-ec-accel";
>> +			break;
>> +		case MOTIONSENSE_TYPE_BARO:
>> +			name = "cros-ec-baro";
>> +			break;
>> +		case MOTIONSENSE_TYPE_GYRO:
>> +			name = "cros-ec-gyro";
>> +			break;
>> +		case MOTIONSENSE_TYPE_MAG:
>> +			name = "cros-ec-mag";
>> +			break;
>> +		case MOTIONSENSE_TYPE_PROX:
>> +			name = "cros-ec-prox";
>> +			break;
>> +		case MOTIONSENSE_TYPE_LIGHT:
>> +			name = "cros-ec-light";
>> +			break;
>> +		case MOTIONSENSE_TYPE_ACTIVITY:
>> +			name = "cros-ec-activity";
>> +			break;
>> +		default:
>> +			dev_warn(dev, "unknown type %d\n", resp->info.type);
>> +			continue;
>> +		}
>> +		ret = cros_ec_sensorhub_allocate_single_sensor(dev, name, i);
>> +		if (ret)
>> +			goto error;
>> +
>> +		sensor_type[resp->info.type]++;
>> +	}
>> +
>> +	if (sensor_type[MOTIONSENSE_TYPE_ACCEL] >= 2)
>> +		ec->has_kb_wake_angle = true;
>> +
>> +	if (cros_ec_check_features(ec,
>> +				EC_FEATURE_REFINED_TABLET_MODE_HYSTERESIS)) {
>> +		ret = cros_ec_sensorhub_allocate_single_sensor(
>> +				dev, "cros-ec-lid-angle", 0);
>> +	}
>> +
>> +error:
>> +	kfree(msg);
>> +	return ret;
>> +}
>> +
>> +static int cros_ec_sensorhub_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct cros_ec_dev *ec = dev_get_drvdata(dev->parent);
>> +	int ret;
>> +	struct cros_ec_sensorhub *data =
>> +		kzalloc(sizeof(struct cros_ec_sensorhub), GFP_KERNEL);
>> +
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->ec = ec;
>> +	dev_set_drvdata(dev, data);
> 
> Superficially this doesn't seem to be used.
> 
>> +
>> +	/* Check whether this EC is a sensor hub. */
>> +	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE)) {
>> +		ret = cros_ec_sensorhub_register(dev, ec);
>> +	} else {
>> +		ret = cros_ec_sensorhub_allocate_single_sensor(
>> +				dev, "cros-ec-accel-legacy", 0);
>> +		ret |= cros_ec_sensorhub_allocate_single_sensor(
>> +				dev, "cros-ec-accel-legacy", 1);
> 
> Doing an |= with a return value is a good way to get some really
> odd bugs in the future.  Please report only the first error and
> cleanly.  If the first one failed we are going to fail to probe
> anyway so don't call the second.
> 
> 
>> +	}
>> +	if (ret)
>> +		dev_err(dev, "failed to add EC sensors: error %d\n", ret);
> 
> Is this particular error useful?  I'd be more tempted to report
> and error for each of the two types of registration above with
> more information on what actually failed.
> 
>> +	return ret;
>> +}
>> +
>> +static struct platform_driver cros_ec_sensorhub_driver = {
>> +	.driver = {
>> +		.name = DRV_NAME,
>> +	},
>> +	.probe = cros_ec_sensorhub_probe,

        .remove?

>> +};
>> +
>> +module_platform_driver(cros_ec_sensorhub_driver);
>> +
>> +MODULE_ALIAS("platform:" DRV_NAME);
>> +MODULE_AUTHOR("Gwendal Grignou <gwendal@...omium.org>");
>> +MODULE_DESCRIPTION("ChromeOS EC MEMS Sensor Hub Driver");
>> +MODULE_LICENSE("GPL");
>> +
>> diff --git a/include/linux/platform_data/cros_ec_sensorhub.h b/include/linux/platform_data/cros_ec_sensorhub.h
>> new file mode 100644
>> index 0000000000000..7737685591ad3
>> --- /dev/null
>> +++ b/include/linux/platform_data/cros_ec_sensorhub.h
>> @@ -0,0 +1,21 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * cros_ec_sensorhub- Chrome OS EC MEMS Sensor Hub driver.

Remove the cros_ec_sensorhub prefix. If for some weird reason the file changes
his name is easy to forget to update the 'cros_ec_sensorhub-' text. So just
remove as doesn't really apport anything.

>> + *
>> + * Copyright (C) 2019 Google, Inc

I think that actually current copyright used by Google is
   'Copyright 2019 Google LLC'

>> + */
>> +
>> +#ifndef __LINUX_PLATFORM_DATA_CROS_EC_SENSORHUB_H
>> +#define __LINUX_PLATFORM_DATA_CROS_EC_SENSORHUB_H
>> +
>> +#include <linux/platform_data/cros_ec_commands.h>
>> +
>> +/*
>> + * struct cros_ec_sensorhub - Sensor Hub device data.
>> + */

Can we document this in kernel-doc format?

>> +struct cros_ec_sensorhub {
>> +	/* Embedded Controller where the hub is located. */
>> +	struct cros_ec_dev *ec;
>> +};
>> +
>> +#endif   /* __LINUX_PLATFORM_DATA_CROS_EC_SENSORHUB_H */
> 

Powered by blists - more mailing lists