From: Jonathan Cameron <jic23@kernel.org>
To: Gwendal Grignou <gwendal@chromium.org>
Cc: briannorris@chromium.org, knaack.h@gmx.de, lars@metafoo.de,
pmeerw@pmeerw.net, lee.jones@linaro.org, bleung@chromium.org,
enric.balletbo@collabora.com, dianders@chromium.org,
groeck@chromium.org, fabien.lahoudere@collabora.com,
linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
Subject: Re: [PATCH v2 04/18] platform/mfd:iio: cros_ec: Register sensor through sensorhub
Date: Mon, 21 Oct 2019 17:00:55 +0100 [thread overview]
Message-ID: <20191021170055.448c7f32@archlinux> (raw)
In-Reply-To: <20191021055403.67849-5-gwendal@chromium.org>
On Sun, 20 Oct 2019 22:53:49 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:
> - Remove duplicate code in mfd, since mfd just register
> cros_ec_sensorhub if at least one sensor is present
> - Change iio cros_ec driver to get the pointer to the cros_ec_dev
> through cros_ec_sensorhub.
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
FWIW given I don't known the driver that well.
Looks good to me.
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> Changes in v2:
> - Remove unerelated changes.
> - Remove ec presence test in iio driver, done in cros_ec_sensorhub.
>
> drivers/iio/accel/cros_ec_accel_legacy.c | 6 -
> .../common/cros_ec_sensors/cros_ec_sensors.c | 6 -
> .../cros_ec_sensors/cros_ec_sensors_core.c | 4 +-
> drivers/iio/light/cros_ec_light_prox.c | 6 -
> drivers/mfd/cros_ec_dev.c | 203 ++----------------
> include/linux/platform_data/cros_ec_proto.h | 8 -
> .../linux/platform_data/cros_ec_sensorhub.h | 8 +
> 7 files changed, 23 insertions(+), 218 deletions(-)
>
> diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c
> index fcc3f999e4827..65f85faf6f31d 100644
> --- a/drivers/iio/accel/cros_ec_accel_legacy.c
> +++ b/drivers/iio/accel/cros_ec_accel_legacy.c
> @@ -163,16 +163,10 @@ static const struct iio_chan_spec cros_ec_accel_legacy_channels[] = {
> static int cros_ec_accel_legacy_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> - struct cros_ec_dev *ec = dev_get_drvdata(dev->parent);
> struct iio_dev *indio_dev;
> struct cros_ec_sensors_core_state *state;
> int ret;
>
> - if (!ec || !ec->ec_dev) {
> - dev_warn(&pdev->dev, "No EC device found.\n");
> - return -EINVAL;
> - }
> -
> indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*state));
> if (!indio_dev)
> return -ENOMEM;
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> index a6987726eeb8a..7dce044734678 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> @@ -222,17 +222,11 @@ static const struct iio_info ec_sensors_info = {
> static int cros_ec_sensors_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> - struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
> struct iio_dev *indio_dev;
> struct cros_ec_sensors_state *state;
> struct iio_chan_spec *channel;
> int ret, i;
>
> - if (!ec_dev || !ec_dev->ec_dev) {
> - dev_warn(&pdev->dev, "No CROS EC device found.\n");
> - return -EINVAL;
> - }
> -
> indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*state));
> if (!indio_dev)
> return -ENOMEM;
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index d2609e6feda4d..81a7f692de2f3 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -18,6 +18,7 @@
> #include <linux/slab.h>
> #include <linux/platform_data/cros_ec_commands.h>
> #include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_data/cros_ec_sensorhub.h>
> #include <linux/platform_device.h>
>
> static char *cros_ec_loc[] = {
> @@ -88,7 +89,8 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> {
> struct device *dev = &pdev->dev;
> struct cros_ec_sensors_core_state *state = iio_priv(indio_dev);
> - struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent);
> + struct cros_ec_sensorhub *sensor_hub = dev_get_drvdata(dev->parent);
> + struct cros_ec_dev *ec = sensor_hub->ec;
> struct cros_ec_sensor_platform *sensor_platform = dev_get_platdata(dev);
> u32 ver_mask;
> int ret, i;
> diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
> index c5263b563fc19..d85a391e50c59 100644
> --- a/drivers/iio/light/cros_ec_light_prox.c
> +++ b/drivers/iio/light/cros_ec_light_prox.c
> @@ -169,17 +169,11 @@ static const struct iio_info cros_ec_light_prox_info = {
> static int cros_ec_light_prox_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> - struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
> struct iio_dev *indio_dev;
> struct cros_ec_light_prox_state *state;
> struct iio_chan_spec *channel;
> int ret;
>
> - if (!ec_dev || !ec_dev->ec_dev) {
> - dev_warn(dev, "No CROS EC device found.\n");
> - return -EINVAL;
> - }
> -
> indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
> if (!indio_dev)
> return -ENOMEM;
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index a35104e35cb4e..c4b977a5dd966 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -78,6 +78,10 @@ static const struct mfd_cell cros_ec_rtc_cells[] = {
> { .name = "cros-ec-rtc", },
> };
>
> +static const struct mfd_cell cros_ec_sensorhub_cells[] = {
> + { .name = "cros-ec-sensorhub", },
> +};
> +
> static const struct mfd_cell cros_usbpd_charger_cells[] = {
> { .name = "cros-usbpd-charger", },
> { .name = "cros-usbpd-logger", },
> @@ -117,192 +121,6 @@ static void cros_ec_class_release(struct device *dev)
> kfree(to_cros_ec_dev(dev));
> }
>
> -static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> -{
> - /*
> - * Issue a command to get the number of sensor reported.
> - * Build an array of sensors driver and register them all.
> - */
> - int ret, i, id, sensor_num;
> - struct mfd_cell *sensor_cells;
> - struct cros_ec_sensor_platform *sensor_platforms;
> - int sensor_type[MOTIONSENSE_TYPE_MAX];
> - struct ec_params_motion_sense *params;
> - struct ec_response_motion_sense *resp;
> - struct cros_ec_command *msg;
> -
> - msg = kzalloc(sizeof(struct cros_ec_command) +
> - max(sizeof(*params), sizeof(*resp)), GFP_KERNEL);
> - if (msg == NULL)
> - return;
> -
> - msg->version = 2;
> - 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;
> - params->cmd = MOTIONSENSE_CMD_DUMP;
> -
> - ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> - if (ret < 0) {
> - dev_warn(ec->dev, "cannot get EC sensor information: %d/%d\n",
> - ret, msg->result);
> - goto error;
> - }
> -
> - resp = (struct ec_response_motion_sense *)msg->data;
> - sensor_num = resp->dump.sensor_count;
> - /*
> - * Allocate 2 extra sensors if lid angle sensor and/or FIFO are needed.
> - */
> - sensor_cells = kcalloc(sensor_num + 2, sizeof(struct mfd_cell),
> - GFP_KERNEL);
> - if (sensor_cells == NULL)
> - goto error;
> -
> - sensor_platforms = kcalloc(sensor_num,
> - sizeof(struct cros_ec_sensor_platform),
> - GFP_KERNEL);
> - if (sensor_platforms == NULL)
> - goto error_platforms;
> -
> - memset(sensor_type, 0, sizeof(sensor_type));
> - 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(ec->dev, "no info for EC sensor %d : %d/%d\n",
> - i, ret, msg->result);
> - continue;
> - }
> - switch (resp->info.type) {
> - case MOTIONSENSE_TYPE_ACCEL:
> - sensor_cells[id].name = "cros-ec-accel";
> - break;
> - case MOTIONSENSE_TYPE_BARO:
> - sensor_cells[id].name = "cros-ec-baro";
> - break;
> - case MOTIONSENSE_TYPE_GYRO:
> - sensor_cells[id].name = "cros-ec-gyro";
> - break;
> - case MOTIONSENSE_TYPE_MAG:
> - sensor_cells[id].name = "cros-ec-mag";
> - break;
> - case MOTIONSENSE_TYPE_PROX:
> - sensor_cells[id].name = "cros-ec-prox";
> - break;
> - case MOTIONSENSE_TYPE_LIGHT:
> - sensor_cells[id].name = "cros-ec-light";
> - break;
> - case MOTIONSENSE_TYPE_ACTIVITY:
> - sensor_cells[id].name = "cros-ec-activity";
> - break;
> - default:
> - dev_warn(ec->dev, "unknown type %d\n", resp->info.type);
> - continue;
> - }
> - sensor_platforms[id].sensor_num = i;
> - sensor_cells[id].id = sensor_type[resp->info.type];
> - sensor_cells[id].platform_data = &sensor_platforms[id];
> - sensor_cells[id].pdata_size =
> - sizeof(struct cros_ec_sensor_platform);
> -
> - sensor_type[resp->info.type]++;
> - id++;
> - }
> -
> - if (sensor_type[MOTIONSENSE_TYPE_ACCEL] >= 2)
> - ec->has_kb_wake_angle = true;
> -
> - if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) {
> - sensor_cells[id].name = "cros-ec-ring";
> - id++;
> - }
> - if (cros_ec_check_features(ec,
> - EC_FEATURE_REFINED_TABLET_MODE_HYSTERESIS)) {
> - sensor_cells[id].name = "cros-ec-lid-angle";
> - id++;
> - }
> -
> - ret = mfd_add_devices(ec->dev, 0, sensor_cells, id,
> - NULL, 0, NULL);
> - if (ret)
> - dev_err(ec->dev, "failed to add EC sensors\n");
> -
> - kfree(sensor_platforms);
> -error_platforms:
> - kfree(sensor_cells);
> -error:
> - kfree(msg);
> -}
> -
> -static struct cros_ec_sensor_platform sensor_platforms[] = {
> - { .sensor_num = 0 },
> - { .sensor_num = 1 }
> -};
> -
> -static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
> - {
> - .name = "cros-ec-accel-legacy",
> - .platform_data = &sensor_platforms[0],
> - .pdata_size = sizeof(struct cros_ec_sensor_platform),
> - },
> - {
> - .name = "cros-ec-accel-legacy",
> - .platform_data = &sensor_platforms[1],
> - .pdata_size = sizeof(struct cros_ec_sensor_platform),
> - }
> -};
> -
> -static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
> -{
> - struct cros_ec_device *ec_dev = ec->ec_dev;
> - u8 status;
> - int ret;
> -
> - /*
> - * ECs that need legacy support are the main EC, directly connected to
> - * the AP.
> - */
> - if (ec->cmd_offset != 0)
> - return;
> -
> - /*
> - * Check if EC supports direct memory reads and if EC has
> - * accelerometers.
> - */
> - if (ec_dev->cmd_readmem) {
> - ret = ec_dev->cmd_readmem(ec_dev, EC_MEMMAP_ACC_STATUS, 1,
> - &status);
> - if (ret < 0) {
> - dev_warn(ec->dev, "EC direct read error.\n");
> - return;
> - }
> -
> - /* Check if EC has accelerometers. */
> - if (!(status & EC_MEMMAP_ACC_STATUS_PRESENCE_BIT)) {
> - dev_info(ec->dev, "EC does not have accelerometers.\n");
> - return;
> - }
> - }
> -
> - /*
> - * The device may still support accelerometers:
> - * it would be an older ARM based device that do not suppor the
> - * EC_CMD_GET_FEATURES command.
> - *
> - * Register 2 accelerometers, we will fail in the IIO driver if there
> - * are no sensors.
> - */
> - ret = mfd_add_hotplug_devices(ec->dev, cros_ec_accel_legacy_cells,
> - ARRAY_SIZE(cros_ec_accel_legacy_cells));
> - if (ret)
> - dev_err(ec_dev->dev, "failed to add EC sensors\n");
> -}
> -
> static int ec_device_probe(struct platform_device *pdev)
> {
> int retval = -ENOMEM;
> @@ -358,11 +176,14 @@ static int ec_device_probe(struct platform_device *pdev)
> goto failed;
>
> /* check whether this EC is a sensor hub. */
> - if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
> - cros_ec_sensors_register(ec);
> - else
> - /* Workaroud for older EC firmware */
> - cros_ec_accel_legacy_register(ec);
> + if (cros_ec_get_sensor_count(ec) > 0) {
> + retval = mfd_add_hotplug_devices(ec->dev,
> + cros_ec_sensorhub_cells,
> + ARRAY_SIZE(cros_ec_sensorhub_cells));
> + if (retval)
> + dev_err(ec->dev, "failed to add %s subdevice: %d\n",
> + cros_ec_sensorhub_cells->name, retval);
> + }
>
> /*
> * The following subdevices can be detected by sending the
> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> index f3de0662135d5..691f9e953a96a 100644
> --- a/include/linux/platform_data/cros_ec_proto.h
> +++ b/include/linux/platform_data/cros_ec_proto.h
> @@ -168,14 +168,6 @@ struct cros_ec_device {
> struct platform_device *pd;
> };
>
> -/**
> - * struct cros_ec_sensor_platform - ChromeOS EC sensor platform information.
> - * @sensor_num: Id of the sensor, as reported by the EC.
> - */
> -struct cros_ec_sensor_platform {
> - u8 sensor_num;
> -};
> -
> /**
> * struct cros_ec_platform - ChromeOS EC platform information.
> * @ec_name: Name of EC device (e.g. 'cros-ec', 'cros-pd', ...)
> diff --git a/include/linux/platform_data/cros_ec_sensorhub.h b/include/linux/platform_data/cros_ec_sensorhub.h
> index 7737685591ad3..c18fba660bb62 100644
> --- a/include/linux/platform_data/cros_ec_sensorhub.h
> +++ b/include/linux/platform_data/cros_ec_sensorhub.h
> @@ -10,6 +10,14 @@
>
> #include <linux/platform_data/cros_ec_commands.h>
>
> +/**
> + * struct cros_ec_sensor_platform - ChromeOS EC sensor platform information.
> + * @sensor_num: Id of the sensor, as reported by the EC.
> + */
> +struct cros_ec_sensor_platform {
> + u8 sensor_num;
> +};
> +
> /*
> * struct cros_ec_sensorhub - Sensor Hub device data.
> */
next prev parent reply other threads:[~2019-10-21 16:01 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-21 5:53 [PATCH v2 00/18] cros_ec: Add sensorhub driver and FIFO processing*** SUBJECT HERE Gwendal Grignou
2019-10-21 5:53 ` [PATCH v2 01/18] platform: chrome: Put docs with the code Gwendal Grignou
2019-10-21 15:31 ` Jonathan Cameron
2019-10-24 17:03 ` Enric Balletbo i Serra
2019-10-21 5:53 ` [PATCH v2 02/18] mfd: cros_ec: Add sensor_count and make check_features public Gwendal Grignou
2019-10-21 15:12 ` Enric Balletbo i Serra
2019-10-21 15:49 ` Jonathan Cameron
2019-11-01 9:00 ` Lee Jones
2019-10-21 5:53 ` [PATCH v2 03/18] platform: cros_ec: Add cros_ec_sensor_hub driver Gwendal Grignou
2019-10-21 15:59 ` Jonathan Cameron
2019-10-22 8:32 ` Enric Balletbo i Serra
2019-10-21 5:53 ` [PATCH v2 04/18] platform/mfd:iio: cros_ec: Register sensor through sensorhub Gwendal Grignou
2019-10-21 16:00 ` Jonathan Cameron [this message]
2019-10-22 9:39 ` Enric Balletbo i Serra
2019-10-21 5:53 ` [PATCH v2 05/18] platform: chrome: cros-ec: record event timestamp in the hard irq Gwendal Grignou
2019-10-22 10:03 ` Enric Balletbo i Serra
2019-10-21 5:53 ` [PATCH v2 06/18] platform: chrome: cros_ec: Do not attempt to register a non-positive IRQ number Gwendal Grignou
2019-10-22 10:05 ` Enric Balletbo i Serra
2019-10-21 5:53 ` [PATCH v2 07/18] platform: chrome: cros_ec: handle MKBP more events flag Gwendal Grignou
2019-10-21 16:07 ` Jonathan Cameron
2019-10-21 5:53 ` [PATCH v2 08/18] Revert "Input: cros_ec_keyb - add back missing mask for event_type" Gwendal Grignou
2019-10-21 5:53 ` [PATCH v2 09/18] Revert "Input: cros_ec_keyb: mask out extra flags in event_type" Gwendal Grignou
2019-10-21 5:53 ` [PATCH v2 10/18] platform: chrome: sensorhub: Add FIFO support Gwendal Grignou
2019-10-21 16:27 ` Jonathan Cameron
2019-10-21 22:09 ` Gwendal Grignou
2019-10-22 10:42 ` Jonathan Cameron
2019-10-21 5:53 ` [PATCH v2 11/18] platform: chrome: sensorhub: Add code to spread timestmap Gwendal Grignou
2019-10-21 5:53 ` [PATCH v2 12/18] platform: chrome: sensorhub: Add median filter Gwendal Grignou
2019-10-21 5:53 ` [PATCH v2 13/18] iio: cros_ec: Move function description to .c file Gwendal Grignou
2019-10-21 16:33 ` Jonathan Cameron
2019-10-21 5:53 ` [PATCH v2 14/18] iio: cros_ec: Register to cros_ec_sensorhub when EC supports FIFO Gwendal Grignou
2019-10-21 16:38 ` Jonathan Cameron
2019-10-21 5:54 ` [PATCH v2 15/18] iio: cros_ec: Remove pm function Gwendal Grignou
2019-10-21 16:39 ` Jonathan Cameron
2019-10-21 5:54 ` [PATCH v2 16/18] iio: cros_ec: Expose hwfifo_timeout Gwendal Grignou
2019-10-21 16:42 ` Jonathan Cameron
2019-10-21 5:54 ` [PATCH v2 17/18] iio: cros_ec: Report hwfifo_watermark_max Gwendal Grignou
2019-10-21 16:42 ` Jonathan Cameron
2019-10-21 5:54 ` [PATCH v2 18/18] iio: cros_ec: Use Hertz as unit for sampling frequency Gwendal Grignou
2019-10-21 16:45 ` Jonathan Cameron
2019-10-22 19:11 ` Gwendal Grignou
2019-10-21 15:29 ` [PATCH v2 00/18] cros_ec: Add sensorhub driver and FIFO processing*** SUBJECT HERE Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191021170055.448c7f32@archlinux \
--to=jic23@kernel.org \
--cc=bleung@chromium.org \
--cc=briannorris@chromium.org \
--cc=dianders@chromium.org \
--cc=enric.balletbo@collabora.com \
--cc=fabien.lahoudere@collabora.com \
--cc=groeck@chromium.org \
--cc=gwendal@chromium.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=lee.jones@linaro.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pmeerw@pmeerw.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.