From: Enric Balletbo Serra <eballetbo@gmail.com>
To: Pi-Hsun Shih <pihsun@chromium.org>
Cc: Benson Leung <bleung@chromium.org>,
Enric Balletbo i Serra <enric.balletbo@collabora.com>,
Guenter Roeck <groeck@chromium.org>,
open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v7 6/7] platform/chrome: cros_ec: add EC host command support using rpmsg.
Date: Thu, 11 Apr 2019 12:27:47 +0200 [thread overview]
Message-ID: <CAFqH_52DaLP98woJM_Kj9AZkxZZ48MVpck0cXrHLPSxnFUX_2w@mail.gmail.com> (raw)
In-Reply-To: <CANdKZ0ec_aux+tWatB563LJEmOt=RVJb5wH5eknQh6u1NraAqg@mail.gmail.com>
Hi,
Missatge de Pi-Hsun Shih <pihsun@chromium.org> del dia dc., 10 d’abr.
2019 a les 9:13:
>
> Hi,
>
> On Thu, Mar 28, 2019 at 7:16 PM Enric Balletbo Serra
> <eballetbo@gmail.com> wrote:
> >
> > Hi
> >
> > Thanks for sending this upstream, Some few comments and questions
> > below. Apart from these LGTM.
> >
> > Missatge de Peter Shih <pihsun@chromium.org> del dia dc., 27 de març
> > 2019 a les 6:17:
> > >
> > > From: Pi-Hsun Shih <pihsun@chromium.org>
> > >
> > > Add EC host command support through rpmsg.
> > >
> > > Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> > > ---
> > > ...
> > > diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
> > > new file mode 100644
> > > index 000000000000..2ecae806cfc5
> > > --- /dev/null
> > > +++ b/drivers/platform/chrome/cros_ec_rpmsg.c
> > > ...
> > > +static int cros_ec_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
> > > + int len, void *priv, u32 src)
> > > +{
> > > + struct cros_ec_device *ec_dev = dev_get_drvdata(&rpdev->dev);
> > > + struct cros_ec_rpmsg *ec_rpmsg = ec_dev->priv;
> > > + struct cros_ec_rpmsg_response *resp;
> > > +
> > > + if (!len) {
> > > + dev_warn(ec_dev->dev, "rpmsg received empty response");
> >
> > Is this is unlikely to happen?
>
> Yes this should not happen unless the firmware is broken. Should I
> change this to `if(unlikely(!len))`, or use dev_err instead ?
>
> >
> > > + return -EINVAL;
> > > + }
> > > +
> > > + resp = data;
> > > + len -= offsetof(struct cros_ec_rpmsg_response, data);
> > > + if (resp->type == HOST_COMMAND_MARK) {
> > > + if (len > ec_dev->din_size) {
> > > + dev_warn(
> > > + ec_dev->dev,
> > > + "received length %d > din_size %d, truncating",
> > > + len, ec_dev->din_size);
> >
> > How often this warning appears? Should be this a dev_dbg?
>
> This also should not happen unless the firmware is broken, so I think
> it's better to have a warning here when there's something wrong.
>
Ok, fine with me.
> >
> > > + len = ec_dev->din_size;
> > > + }
> > > +
> > > + memcpy(ec_dev->din, resp->data, len);
> > > + complete(&ec_rpmsg->xfer_ack);
> > > + } else if (resp->type == HOST_EVENT_MARK) {
> > > + schedule_work(&ec_rpmsg->host_event_work);
> > > + } else {
> > > + dev_warn(ec_dev->dev, "rpmsg received invalid type = %d",
> > > + resp->type);
> >
> > Is this is unlikely to happen?
>
> Same as above, this should not happen unless the firmware is broken.
>
> >
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int cros_ec_rpmsg_probe(struct rpmsg_device *rpdev)
> > > +{
> > > + struct device *dev = &rpdev->dev;
> > > + struct cros_ec_device *ec_dev;
> > > + struct cros_ec_rpmsg *ec_rpmsg;
> > > + int ret;
> > > +
> > > + ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
> > > + if (!ec_dev)
> > > + return -ENOMEM;
> > > +
> > > + ec_rpmsg = devm_kzalloc(dev, sizeof(*ec_rpmsg), GFP_KERNEL);
> > > + if (!ec_rpmsg)
> > > + return -ENOMEM;
> > > +
> > > + ec_dev->dev = dev;
> > > + ec_dev->priv = ec_rpmsg;
> > > + ec_dev->cmd_xfer = cros_ec_cmd_xfer_rpmsg;
> > > + ec_dev->pkt_xfer = cros_ec_pkt_xfer_rpmsg;
> > > + ec_dev->phys_name = dev_name(&rpdev->dev);
> > > + ec_dev->din_size = sizeof(struct ec_host_response) +
> > > + sizeof(struct ec_response_get_protocol_info);
> > > + ec_dev->dout_size = sizeof(struct ec_host_request);
> > > + dev_set_drvdata(dev, ec_dev);
> > > +
> > > + ec_rpmsg->rpdev = rpdev;
> > > + init_completion(&ec_rpmsg->xfer_ack);
> > > + INIT_WORK(&ec_rpmsg->host_event_work,
> > > + cros_ec_rpmsg_host_event_function);
> > > +
> > > + ret = cros_ec_register(ec_dev);
> > > + if (ret) {
> > > + dev_err(dev, "cannot register EC\n");
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> >
> > cros_ec_register returns 0 on success and prints an error message if
> > something went wrong. Remove the above and just do:
> >
> > return cros_ec_register(ec_dev);
> >
>
> Ack, would change this in v7.
>
Will wait for v7.
Thanks,
Enric
> >
> > > +}
> > > +
> > > +static void cros_ec_rpmsg_remove(struct rpmsg_device *rpdev)
> > > +{
> > > + struct cros_ec_device *ec_dev = dev_get_drvdata(&rpdev->dev);
> > > + struct cros_ec_rpmsg *ec_rpmsg = ec_dev->priv;
> > > +
> > > + cancel_work_sync(&ec_rpmsg->host_event_work);
> > > +}
> > > +
> > > +static const struct of_device_id cros_ec_rpmsg_of_match[] = {
> > > + { .compatible = "google,cros-ec-rpmsg", },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, cros_ec_rpmsg_of_match);
> > > +
> > > +static struct rpmsg_driver cros_ec_driver_rpmsg = {
> > > + .drv = {
> > > + .name = KBUILD_MODNAME,
> > > + .of_match_table = cros_ec_rpmsg_of_match,
> > > + },
> > > + .probe = cros_ec_rpmsg_probe,
> > > + .remove = cros_ec_rpmsg_remove,
> > > + .callback = cros_ec_rpmsg_callback,
> > > +};
> > > +
> > > +module_rpmsg_driver(cros_ec_driver_rpmsg);
> > > +
> > > +MODULE_LICENSE("GPL v2");
> > > +MODULE_DESCRIPTION("ChromeOS EC multi function device (rpmsg)");
> > > --
> > > 2.21.0.392.gf8f6787159e-goog
> > >
next prev parent reply other threads:[~2019-04-11 10:28 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20190327051450.16222-1-pihsun@chromium.org>
2019-03-27 5:14 ` [PATCH v7 1/7] dt-bindings: Add a binding for Mediatek SCP Peter Shih
2019-03-27 5:14 ` [PATCH v7 2/7] remoteproc/mediatek: add SCP support for mt8183 Peter Shih
2019-03-27 5:14 ` [PATCH v7 3/7] remoteproc: mt8183: add reserved memory manager API Peter Shih
2019-03-27 5:14 ` [PATCH v7 4/7] rpmsg: add rpmsg support for mt8183 SCP Peter Shih
2019-03-27 5:14 ` [PATCH v7 5/7] dt-bindings: Add binding for cros-ec-rpmsg Peter Shih
2019-04-06 6:05 ` Rob Herring
2019-03-27 5:14 ` [PATCH v7 6/7] platform/chrome: cros_ec: add EC host command support using rpmsg Peter Shih
2019-03-28 11:15 ` Enric Balletbo Serra
2019-04-10 7:13 ` Pi-Hsun Shih
2019-04-11 10:27 ` Enric Balletbo Serra [this message]
2019-03-27 5:14 ` [PATCH v7 7/7] cros_ec: differentiate SCP from EC by feature bit Peter Shih
2019-03-28 10:54 ` Enric Balletbo Serra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFqH_52DaLP98woJM_Kj9AZkxZZ48MVpck0cXrHLPSxnFUX_2w@mail.gmail.com \
--to=eballetbo@gmail.com \
--cc=bleung@chromium.org \
--cc=enric.balletbo@collabora.com \
--cc=groeck@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pihsun@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).