[PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs

[PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs

[Moritz Fischer]

From: Moritz Fischer <mdf@kernel.org>

Add cros_ec_readmem() based helpers for I2C/SPI based ECs.
Unlike the LPC based ECs the I2C/SPI based ones cannot just directly
read the mapped region.

This is useful for things like accessing fan speeds or temperatures.

Signed-off-by: Moritz Fischer <mdf@kernel.org>
---

Hi all,


I had submitted this back in May 2017 [1] and then somewhat forgot
about it. So here's a new version :)

As to why is this required? we're using this in some of our devices
[2] that run a modified firmware based on Chromium-EC.
I've been carrying the patch downstream in our tree for a while and
it would be nice if we can merge this upstream so I can stop rebasing
this :)

Thanks,

Moritz

[1] https://lore.kernel.org/patchwork/patch/786065/ 
[2] https://www.ettus.com/product/details/USRP-E320 

---
 drivers/platform/chrome/cros_ec_i2c.c   |  1 +
 drivers/platform/chrome/cros_ec_proto.c | 38 +++++++++++++++++++++++++
 drivers/platform/chrome/cros_ec_spi.c   |  1 +
 include/linux/mfd/cros_ec.h             | 12 ++++++++
 4 files changed, 52 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..2f1d45a7a934 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -636,3 +636,41 @@ 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)
+{
+	int ret;
+	struct ec_params_read_memmap *params;
+	struct cros_ec_command *msg;
+
+	if (offset >= EC_MEMMAP_SIZE - bytes)
+		return -EINVAL;
+
+	msg = kzalloc(sizeof(*msg) + max(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

Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]

Hi Moritz,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.0-rc1 next-20190108]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Moritz-Fischer/platform-chrome-Add-cros_ec_readmem-helpers-for-I2C-SPI-based-ECs/20190108-184758
config: x86_64-randconfig-x005-201901 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/linux/list.h:9:0,
                    from include/linux/kobject.h:19,
                    from include/linux/cdev.h:5,
                    from include/linux/mfd/cros_ec.h:19,
                    from drivers/platform/chrome/cros_ec_proto.c:17:
   drivers/platform/chrome/cros_ec_proto.c: In function 'cros_ec_readmem':
   include/linux/kernel.h:846:29: warning: comparison of distinct pointer types lacks a cast
      (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                                ^
   include/linux/kernel.h:860:4: note: in expansion of macro '__typecheck'
      (__typecheck(x, y) && __no_side_effects(x, y))
       ^~~~~~~~~~~
   include/linux/kernel.h:870:24: note: in expansion of macro '__safe_cmp'
     __builtin_choose_expr(__safe_cmp(x, y), \
                           ^~~~~~~~~~
   include/linux/kernel.h:886:19: note: in expansion of macro '__careful_cmp'
    #define max(x, y) __careful_cmp(x, y, >)
                      ^~~~~~~~~~~~~
>> drivers/platform/chrome/cros_ec_proto.c:650:31: note: in expansion of macro 'max'
     msg = kzalloc(sizeof(*msg) + max(sizeof(*params), bytes), GFP_KERNEL);
                                  ^~~

vim +/max +650 drivers/platform/chrome/cros_ec_proto.c

   639	
   640	int cros_ec_readmem(struct cros_ec_device *ec, unsigned int offset,
   641			    unsigned int bytes, void *dest)
   642	{
   643		int ret;
   644		struct ec_params_read_memmap *params;
   645		struct cros_ec_command *msg;
   646	
   647		if (offset >= EC_MEMMAP_SIZE - bytes)
   648			return -EINVAL;
   649	
 > 650		msg = kzalloc(sizeof(*msg) + max(sizeof(*params), bytes), GFP_KERNEL);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28176 bytes --]

Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs

[Enric Balletbo i Serra]

Hi Moritz,

Many thanks for the patch,I've one concern on this, and I'd be also interested
on Benson and Guenter opinion ...

On 8/1/19 4:56, Moritz Fischer wrote:
> From: Moritz Fischer <mdf@kernel.org>
> 
> Add cros_ec_readmem() based helpers for I2C/SPI based ECs.
> Unlike the LPC based ECs the I2C/SPI based ones cannot just directly
> read the mapped region.
> 
> This is useful for things like accessing fan speeds or temperatures.
> 
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
> 
> Hi all,
> 
> 
> I had submitted this back in May 2017 [1] and then somewhat forgot
> about it. So here's a new version :)
> 
> As to why is this required? we're using this in some of our devices
> [2] that run a modified firmware based on Chromium-EC.
> I've been carrying the patch downstream in our tree for a while and
> it would be nice if we can merge this upstream so I can stop rebasing
> this :)
> 

Right, if we merge this you'll probably reduce your downstream patches but
actually, if I am not wrong, there is no user for this. There isn't an upstream
driver that uses the new functions so we will end up having upstream dead code.
IMO if you want to have this upstream you should also upstream the drivers that
use that code. Makes sense?

Thanks,
  Enric


> Thanks,
> 
> Moritz
> 
> [1] https://lore.kernel.org/patchwork/patch/786065/ 
> [2] https://www.ettus.com/product/details/USRP-E320 
> 
> ---
>  drivers/platform/chrome/cros_ec_i2c.c   |  1 +
>  drivers/platform/chrome/cros_ec_proto.c | 38 +++++++++++++++++++++++++
>  drivers/platform/chrome/cros_ec_spi.c   |  1 +
>  include/linux/mfd/cros_ec.h             | 12 ++++++++
>  4 files changed, 52 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..2f1d45a7a934 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -636,3 +636,41 @@ 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)
> +{
> +	int ret;
> +	struct ec_params_read_memmap *params;
> +	struct cros_ec_command *msg;
> +
> +	if (offset >= EC_MEMMAP_SIZE - bytes)
> +		return -EINVAL;
> +
> +	msg = kzalloc(sizeof(*msg) + max(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;
> 

Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs

[Guenter Roeck]

On Tue, Jan 8, 2019 at 6:28 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Moritz,
>
> Many thanks for the patch,I've one concern on this, and I'd be also interested
> on Benson and Guenter opinion ...
>
> On 8/1/19 4:56, Moritz Fischer wrote:
> > From: Moritz Fischer <mdf@kernel.org>
> >
> > Add cros_ec_readmem() based helpers for I2C/SPI based ECs.
> > Unlike the LPC based ECs the I2C/SPI based ones cannot just directly
> > read the mapped region.
> >
> > This is useful for things like accessing fan speeds or temperatures.
> >
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> >
> > Hi all,
> >
> >
> > I had submitted this back in May 2017 [1] and then somewhat forgot
> > about it. So here's a new version :)
> >
> > As to why is this required? we're using this in some of our devices
> > [2] that run a modified firmware based on Chromium-EC.
> > I've been carrying the patch downstream in our tree for a while and
> > it would be nice if we can merge this upstream so I can stop rebasing
> > this :)
> >
>
> Right, if we merge this you'll probably reduce your downstream patches but
> actually, if I am not wrong, there is no user for this. There isn't an upstream
> driver that uses the new functions so we will end up having upstream dead code.
> IMO if you want to have this upstream you should also upstream the drivers that
> use that code. Makes sense?
>

He has a hwmon driver which I think uses it. Question would rather be
if that is an acceptable use or if it exposes EC internals, ie non-API
data. Comments, anyone ?

Digging ... https://patchwork.kernel.org/patch/9670517/

Guenter

> Thanks,
>   Enric
>
>
> > Thanks,
> >
> > Moritz
> >
> > [1] https://lore.kernel.org/patchwork/patch/786065/
> > [2] https://www.ettus.com/product/details/USRP-E320
> >
> > ---
> >  drivers/platform/chrome/cros_ec_i2c.c   |  1 +
> >  drivers/platform/chrome/cros_ec_proto.c | 38 +++++++++++++++++++++++++
> >  drivers/platform/chrome/cros_ec_spi.c   |  1 +
> >  include/linux/mfd/cros_ec.h             | 12 ++++++++
> >  4 files changed, 52 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..2f1d45a7a934 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -636,3 +636,41 @@ 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)
> > +{
> > +     int ret;
> > +     struct ec_params_read_memmap *params;
> > +     struct cros_ec_command *msg;
> > +
> > +     if (offset >= EC_MEMMAP_SIZE - bytes)
> > +             return -EINVAL;
> > +
> > +     msg = kzalloc(sizeof(*msg) + max(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;
> >

Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs

[Enric Balletbo i Serra]

Hi,

On 8/1/19 15:34, Guenter Roeck wrote:
> On Tue, Jan 8, 2019 at 6:28 AM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>>
>> Hi Moritz,
>>
>> Many thanks for the patch,I've one concern on this, and I'd be also interested
>> on Benson and Guenter opinion ...
>>
>> On 8/1/19 4:56, Moritz Fischer wrote:
>>> From: Moritz Fischer <mdf@kernel.org>
>>>
>>> Add cros_ec_readmem() based helpers for I2C/SPI based ECs.
>>> Unlike the LPC based ECs the I2C/SPI based ones cannot just directly
>>> read the mapped region.
>>>
>>> This is useful for things like accessing fan speeds or temperatures.
>>>
>>> Signed-off-by: Moritz Fischer <mdf@kernel.org>
>>> ---
>>>
>>> Hi all,
>>>
>>>
>>> I had submitted this back in May 2017 [1] and then somewhat forgot
>>> about it. So here's a new version :)
>>>
>>> As to why is this required? we're using this in some of our devices
>>> [2] that run a modified firmware based on Chromium-EC.
>>> I've been carrying the patch downstream in our tree for a while and
>>> it would be nice if we can merge this upstream so I can stop rebasing
>>> this :)
>>>
>>
>> Right, if we merge this you'll probably reduce your downstream patches but
>> actually, if I am not wrong, there is no user for this. There isn't an upstream
>> driver that uses the new functions so we will end up having upstream dead code.
>> IMO if you want to have this upstream you should also upstream the drivers that
>> use that code. Makes sense?
>>
> 
> He has a hwmon driver which I think uses it. 

Nice, sorry Moritz because the hwmon driver was not on my radar, I missed it.
The patch needs some rework I guess so would be nice have all together in the
same patch series.

> Question would rather be
> if that is an acceptable use or if it exposes EC internals, ie non-API
> data. Comments, anyone ?
> 

Yes, that's the question. We have similar command for devices that use the lpc
bus.  On my side I'll take a deeper look. More Comments ?

> Digging ... https://patchwork.kernel.org/patch/9670517/
> 
> Guenter
> 
>> Thanks,
>>   Enric
>>
>>
>>> Thanks,
>>>
>>> Moritz
>>>
>>> [1] https://lore.kernel.org/patchwork/patch/786065/
>>> [2] https://www.ettus.com/product/details/USRP-E320
>>>
>>> ---
>>>  drivers/platform/chrome/cros_ec_i2c.c   |  1 +
>>>  drivers/platform/chrome/cros_ec_proto.c | 38 +++++++++++++++++++++++++
>>>  drivers/platform/chrome/cros_ec_spi.c   |  1 +
>>>  include/linux/mfd/cros_ec.h             | 12 ++++++++
>>>  4 files changed, 52 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..2f1d45a7a934 100644
>>> --- a/drivers/platform/chrome/cros_ec_proto.c
>>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>>> @@ -636,3 +636,41 @@ 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)
>>> +{
>>> +     int ret;
>>> +     struct ec_params_read_memmap *params;
>>> +     struct cros_ec_command *msg;
>>> +
>>> +     if (offset >= EC_MEMMAP_SIZE - bytes)
>>> +             return -EINVAL;
>>> +
>>> +     msg = kzalloc(sizeof(*msg) + max(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;
>>>

Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs

[Moritz Fischer]

Hi,

On Tue, Jan 08, 2019 at 04:00:58PM +0100, Enric Balletbo i Serra wrote:
> Hi,
> 
> On 8/1/19 15:34, Guenter Roeck wrote:
> > On Tue, Jan 8, 2019 at 6:28 AM Enric Balletbo i Serra
> > <enric.balletbo@collabora.com> wrote:
> >>
> >> Hi Moritz,
> >>
> >> Many thanks for the patch,I've one concern on this, and I'd be also interested
> >> on Benson and Guenter opinion ...
> >>
> >> On 8/1/19 4:56, Moritz Fischer wrote:
> >>> From: Moritz Fischer <mdf@kernel.org>
> >>>
> >>> Add cros_ec_readmem() based helpers for I2C/SPI based ECs.

That sentence also should probably get reworked on my end :)
> >>> Unlike the LPC based ECs the I2C/SPI based ones cannot just directly
> >>> read the mapped region.
> >>>
> >>> This is useful for things like accessing fan speeds or temperatures.
> >>>
> >>> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> >>> ---
> >>>
> >>> Hi all,
> >>>
> >>>
> >>> I had submitted this back in May 2017 [1] and then somewhat forgot
> >>> about it. So here's a new version :)
> >>>
> >>> As to why is this required? we're using this in some of our devices
> >>> [2] that run a modified firmware based on Chromium-EC.
> >>> I've been carrying the patch downstream in our tree for a while and
> >>> it would be nice if we can merge this upstream so I can stop rebasing
> >>> this :)
> >>>
> >>
> >> Right, if we merge this you'll probably reduce your downstream patches but
> >> actually, if I am not wrong, there is no user for this. There isn't an upstream
> >> driver that uses the new functions so we will end up having upstream dead code.
> >> IMO if you want to have this upstream you should also upstream the drivers that
> >> use that code. Makes sense?

Sure, see below.
> >>
> > 
> > He has a hwmon driver which I think uses it. 
> 
> Nice, sorry Moritz because the hwmon driver was not on my radar, I missed it.
> The patch needs some rework I guess so would be nice have all together in the
> same patch series.

My fault, sorry, the reason there is that we have a bunch of versions of
this driver internally (some of them thermal drivers, some of the hwmon
drivers). All of them are open source and in our meta-ettus yocto layer,
just need cleanup on our end first.

> 
> > Question would rather be
> > if that is an acceptable use or if it exposes EC internals, ie non-API
> > data. Comments, anyone ?
> > 
> 
> Yes, that's the question. We have similar command for devices that use the lpc
> bus.  On my side I'll take a deeper look. More Comments ?

Yeah basically one comment from my side: The cros_ec_readmem only
exposes a bunch of 'virtual' registers on I2C/SPI based ECs so it's
not like you're leaking non-API data, but merely brings functionality
for I2C/SPI based ECs to parity :)

Thanks for considering, I'll take another look at the Kbuild issue

Moritz