lkml.org 
[lkml]   [2020]   [Feb]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 01/17] platform/chrome: Add EC command msg wrapper
On Mon, Feb 3, 2020 at 7:27 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Prashant,
>
> Many thanks to work on this. Some comments below ...

> > #include <linux/platform_data/cros_ec_commands.h>
> > #include <linux/platform_data/cros_ec_proto.h>
> > @@ -570,6 +571,62 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
> > }
> > EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
> >
> > +/**
> > + * cros_ec_send_cmd_msg() - Utility function to send commands to ChromeOS EC.
>
> I'm wondering if just cros_ec_cmd() shouldn't be a better name. If it's a
> replacement of current user usage of cros_ec_cmd_xfer and
> cros_ec_cmd_xfer_status, this will be used a lot, and have a short name and
> clear will help the users of this helper.
Sounds good. I think in another patch (13/17) there is a point about
how it's tough to get rid of cros_ec_cmd_xfer() without non-trivial
rework to a few drivers, but cros_ec_cmd() is certainly more concise.

>
> > + * @ec: EC device struct.
> > + * @version: Command version number (often 0).
> > + * @command: Command ID including offset.
> > + * @outdata: Data to be sent to the EC.
> > + * @outsize: Size of the &outdata buffer.
> > + * @indata: Data to be received from the EC.
> > + * @insize: Size of the &indata buffer.
> > + *
> > + * This function is a wrapper around &cros_ec_cmd_xfer_status, and performs
>
> You say that is a wrapper around cros_ec_cmd_xfer_status but then you remove
> that function, and rewrite the doc here. Just explain for what is this helper
> without referencing cros_ec_cmd_xfer_status and cros_ec_cmd_xfer.
My apologies, I should have edited it to say cros_ec_cmd_xfer()
instead of _status().
I'll update the documentation to just avoid the two altogether.

>
> > + * some of the common work involved with sending a command to the EC. This
> > + * includes allocating and filling up a &struct cros_ec_command message buffer,
> > + * and copying the received data to another buffer.
> > + *
> > + * Return: The number of bytes transferred on success or negative error code.
> > + */
> > +int cros_ec_send_cmd_msg(struct cros_ec_device *ec, unsigned int version,
> > + unsigned int command, void *outdata,
> > + unsigned int outsize, void *indata,
> > + unsigned int insize)
>
> Should we change the parameter types from "unsigned int" to "u32" to match both
> ec hardware and the storage type in struct cros_ec_command?
Sounds good. I will make the change.
>
> > +{
> > + struct cros_ec_command *msg;
> > + int ret;
> > +
> > + msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
> > + if (!msg)
> > + return -ENOMEM;
> > +
> > + msg->version = version;
> > + msg->command = command;
> > + msg->outsize = outsize;
> > + msg->insize = insize;
> > +
> > + if (outdata && outsize > 0)
> > + memcpy(msg->data, outdata, outsize);
> > +
> > + ret = cros_ec_cmd_xfer(ec, msg);
> > + if (ret < 0) {
> > + dev_err(ec->dev, "Command xfer error (err:%d)\n", ret);
> > + goto cleanup;
> > + } else if (msg->result != EC_RES_SUCCESS) {
> > + dev_dbg(ec->dev, "Command result (err: %d)\n", msg->result);
> > + ret = -EPROTO;
> > + goto cleanup;
> > + }
> > +
> > + if (insize)
> > + memcpy(indata, msg->data, insize);
> > +
> > +cleanup:
> > + kfree(msg);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(cros_ec_send_cmd_msg);
> > +
> > static int get_next_event_xfer(struct cros_ec_device *ec_dev,
> > struct cros_ec_command *msg,
> > struct ec_response_get_next_event_v1 *event,
> > diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> > index 30098a5515231d..166ce26bdd79eb 100644
> > --- a/include/linux/platform_data/cros_ec_proto.h
> > +++ b/include/linux/platform_data/cros_ec_proto.h
> > @@ -201,6 +201,11 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> > int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
> > struct cros_ec_command *msg);
> >
> > +int cros_ec_send_cmd_msg(struct cros_ec_device *ec_dev, unsigned int version,
> > + unsigned int command, void *outdata,
> > + unsigned int outsize, void *indata,
> > + unsigned int insize);
> > +
> > int cros_ec_register(struct cros_ec_device *ec_dev);
> >
> > int cros_ec_unregister(struct cros_ec_device *ec_dev);
> >

\
 
 \ /
  Last update: 2020-02-03 19:25    [W:0.122 / U:4.344 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site