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. > > > + 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. > > > +} > > + > > +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 > >