* [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs @ 2019-01-08 3:56 Moritz Fischer 2019-01-08 11:23 ` kbuild test robot 2019-01-08 14:28 ` Enric Balletbo i Serra 0 siblings, 2 replies; 6+ messages in thread From: Moritz Fischer @ 2019-01-08 3:56 UTC (permalink / raw) To: linux-kernel Cc: bleung, enric.balletbo, groeck, lee.jones, moritz, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs 2019-01-08 3:56 [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs Moritz Fischer @ 2019-01-08 11:23 ` kbuild test robot 2019-01-08 14:28 ` Enric Balletbo i Serra 1 sibling, 0 replies; 6+ messages in thread From: kbuild test robot @ 2019-01-08 11:23 UTC (permalink / raw) To: Moritz Fischer Cc: kbuild-all, linux-kernel, bleung, enric.balletbo, groeck, lee.jones, moritz, Moritz Fischer [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs 2019-01-08 3:56 [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs Moritz Fischer 2019-01-08 11:23 ` kbuild test robot @ 2019-01-08 14:28 ` Enric Balletbo i Serra 2019-01-08 14:34 ` Guenter Roeck 1 sibling, 1 reply; 6+ messages in thread From: Enric Balletbo i Serra @ 2019-01-08 14:28 UTC (permalink / raw) To: Moritz Fischer, linux-kernel Cc: bleung, groeck, lee.jones, moritz, Moritz Fischer 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; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs 2019-01-08 14:28 ` Enric Balletbo i Serra @ 2019-01-08 14:34 ` Guenter Roeck 2019-01-08 15:00 ` Enric Balletbo i Serra 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2019-01-08 14:34 UTC (permalink / raw) To: Enric Balletbo i Serra Cc: Moritz Fischer, linux-kernel, Benson Leung, Guenter Roeck, Lee Jones, moritz, Moritz Fischer 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; > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs 2019-01-08 14:34 ` Guenter Roeck @ 2019-01-08 15:00 ` Enric Balletbo i Serra 2019-01-08 18:12 ` Moritz Fischer 0 siblings, 1 reply; 6+ messages in thread From: Enric Balletbo i Serra @ 2019-01-08 15:00 UTC (permalink / raw) To: Guenter Roeck Cc: Moritz Fischer, linux-kernel, Benson Leung, Guenter Roeck, Lee Jones, moritz, Moritz Fischer 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; >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs 2019-01-08 15:00 ` Enric Balletbo i Serra @ 2019-01-08 18:12 ` Moritz Fischer 0 siblings, 0 replies; 6+ messages in thread From: Moritz Fischer @ 2019-01-08 18:12 UTC (permalink / raw) To: Enric Balletbo i Serra Cc: Guenter Roeck, Moritz Fischer, linux-kernel, Benson Leung, Guenter Roeck, Lee Jones, moritz, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-08 18:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-01-08 3:56 [PATCH v2] platform/chrome: Add cros_ec_readmem() helpers for I2C/SPI based ECs Moritz Fischer 2019-01-08 11:23 ` kbuild test robot 2019-01-08 14:28 ` Enric Balletbo i Serra 2019-01-08 14:34 ` Guenter Roeck 2019-01-08 15:00 ` Enric Balletbo i Serra 2019-01-08 18:12 ` Moritz Fischer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox