Date:   Mon, 21 Jan 2019 18:32:48 +0100
From:   Enric Balletbo Serra <eballetbo@...il.com>
To:     Moritz Fischer <mdf@...nel.org>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Benson Leung <bleung@...omium.org>,
        Guenter Roeck <groeck@...omium.org>,
        Lee Jones <lee.jones@...aro.org>, moritz@...us.com
Subject: Re: [PATCH v3] platform/chrome: Add cros_ec_readmem() helpers for
 I2C/SPI based ECs

Hi Moritz,

Missatge de Moritz Fischer <mdf@...nel.org> del dia dj., 10 de gen.
2019 a les 5:40:
>
> Add cros_ec_readmem() helpers for I2C/SPI based ECs.
> Unlike the LPC based ECs the I2C/SPI based ones cannot just directly
> read the mapped region, but have to resort to EC_CMD_READ_MEMMAP.
>
> This is useful for things like accessing fan speeds or temperatures.
>
> Signed-off-by: Moritz Fischer <mdf@...nel.org>
> ---
>
> Hi all,
>
> This can be used for a hwmon or thermal driver such as [1], but also
> alone can be used with 'ectool' via the ioctl() interface.
>

ooi, did you try this with a Chromebook? Or only with your device?

I was a bit worried on the side effects to expose this command to
userspace, so I did a quick test, and to be honest I didn't like the
result. Issuing the ioctl resulted on a reboot on the Chromebook I
tested, which is not good. I didn't investigate more but I can say
that this change, at least, will need some rework. I'll try to
investigate a bit more on what is happening.

Thanks,
 Enric

> Thanks,
> Moritz
>
>
> Changes from v2:
> - Addressed KBuild complaint about max() with size_t vs unsigned int
>   parameters
>
> Changes from v1:
> - Moved to drivers/platform/proto
>
>
> [1] https://patchwork.kernel.org/patch/9670517/ (needs work)
> ---
>  drivers/platform/chrome/cros_ec_i2c.c   |  1 +
>  drivers/platform/chrome/cros_ec_proto.c | 39 +++++++++++++++++++++++++
>  drivers/platform/chrome/cros_ec_spi.c   |  1 +
>  include/linux/mfd/cros_ec.h             | 12 ++++++++
>  4 files changed, 53 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_i2c.c b/drivers/platform/chrome/cros_ec_i2c.c
> index ef9b4763356f..c3a9bee37b1d 100644
> --- a/drivers/platform/chrome/cros_ec_i2c.c
> +++ b/drivers/platform/chrome/cros_ec_i2c.c
> @@ -303,6 +303,7 @@ static int cros_ec_i2c_probe(struct i2c_client *client,
>         ec_dev->irq = client->irq;
>         ec_dev->cmd_xfer = cros_ec_cmd_xfer_i2c;
>         ec_dev->pkt_xfer = cros_ec_pkt_xfer_i2c;
> +       ec_dev->cmd_readmem = cros_ec_readmem;
>         ec_dev->phys_name = client->adapter->name;
>         ec_dev->din_size = sizeof(struct ec_host_response_i2c) +
>                            sizeof(struct ec_response_get_protocol_info);
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index cc7baf0ecb3c..d2ba662fc713 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -636,3 +636,42 @@ u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev)
>         return host_event;
>  }
>  EXPORT_SYMBOL(cros_ec_get_host_event);
> +
> +int cros_ec_readmem(struct cros_ec_device *ec, unsigned int offset,
> +                   unsigned int bytes, void *dest)
> +{
> +       struct ec_params_read_memmap *params;
> +       struct cros_ec_command *msg;
> +       int ret;
> +
> +       if (offset >= EC_MEMMAP_SIZE - bytes)
> +               return -EINVAL;
> +
> +       msg = kzalloc(sizeof(*msg) + max_t(size_t, sizeof(*params), bytes),
> +                     GFP_KERNEL);
> +       if (!msg)
> +               return -ENOMEM;
> +
> +       msg->version = 0;
> +       msg->command = EC_CMD_READ_MEMMAP;
> +       msg->insize = bytes;
> +       msg->outsize = sizeof(*params);
> +
> +       params = (struct ec_params_read_memmap *)msg->data;
> +       params->offset = offset;
> +       params->size = bytes;
> +
> +       ret = cros_ec_cmd_xfer_status(ec, msg);
> +       if (ret < 0) {
> +               dev_warn(ec->dev, "cannot read mapped reg: %d/%d\n",
> +                        ret, msg->result);
> +               goto out_free;
> +       }
> +
> +       memcpy(dest, msg->data, bytes);
> +
> +out_free:
> +       kfree(msg);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_readmem);
> diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> index 2060d1483043..b95c1a25adfc 100644
> --- a/drivers/platform/chrome/cros_ec_spi.c
> +++ b/drivers/platform/chrome/cros_ec_spi.c
> @@ -666,6 +666,7 @@ static int cros_ec_spi_probe(struct spi_device *spi)
>         ec_dev->irq = spi->irq;
>         ec_dev->cmd_xfer = cros_ec_cmd_xfer_spi;
>         ec_dev->pkt_xfer = cros_ec_pkt_xfer_spi;
> +       ec_dev->cmd_readmem = cros_ec_readmem;
>         ec_dev->phys_name = dev_name(&ec_spi->spi->dev);
>         ec_dev->din_size = EC_MSG_PREAMBLE_COUNT +
>                            sizeof(struct ec_host_response) +
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index de8b588c8776..0228fb42dcda 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -335,6 +335,18 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
>   */
>  u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
>
> +/**
> + * cros_ec_readmem - Read mapped memory in the ChromeOS EC
> + *
> + * @ec: Device to read from
> + * @offset: Offset to read within the mapped region
> + * @bytes: number of bytes to read
> + * @data: Return data
> + * @return: 0 if Ok, -ve on error
> + */
> +int cros_ec_readmem(struct cros_ec_device *ec, unsigned int offset,
> +                   unsigned int bytes, void *dest);
> +
>  /* sysfs stuff */
>  extern struct attribute_group cros_ec_attr_group;
>  extern struct attribute_group cros_ec_lightbar_attr_group;
> --
> 2.20.1
>

Powered by blists - more mailing lists