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@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> >
> > 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;
> >