Hi, Missatge de Pi-Hsun Shih <pih...@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 > <eballe...@gmail.com> wrote: > > > > Hi > > > > Thanks for sending this upstream, Some few comments and questions > > below. Apart from these LGTM. > > > > Missatge de Peter Shih <pih...@chromium.org> del dia dc., 27 de març > > 2019 a les 6:17: > > > > > > From: Pi-Hsun Shih <pih...@chromium.org> > > > > > > Add EC host command support through rpmsg. > > > > > > Signed-off-by: Pi-Hsun Shih <pih...@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 > > >