On Fri, Nov 22, 2024 at 12:05 AM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> On Mon, Nov 18, 2024 at 12:18:17PM +0100, Łukasz Bartosik wrote:
> > On Mon, Nov 18, 2024 at 12:08 PM Dmitry Baryshkov
> > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > >
> > > On Mon, 18 Nov 2024 at 12:38, Łukasz Bartosik <ukaszb@xxxxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Nov 18, 2024 at 11:21 AM Dmitry Baryshkov
> > > > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Mon, 18 Nov 2024 at 12:10, Łukasz Bartosik <ukaszb@xxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > On Fri, Nov 15, 2024 at 6:00 PM Dmitry Baryshkov
> > > > > > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Fri, Nov 15, 2024 at 03:52:33PM +0000, Łukasz Bartosik wrote:
> > > > > > > > From: Pavan Holla <pholla@xxxxxxxxxxxx>
> > > > > > > >
> > > > > > > > Implementation of a UCSI transport driver for ChromeOS.
> > > > > > > > This driver will be loaded if the ChromeOS EC implements a PPM.
> > > > > > > >
> > > > > > > > Signed-off-by: Pavan Holla <pholla@xxxxxxxxxxxx>
> > > > > > > > Co-developed-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
> > > > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
> > > > > > > > Co-developed-by: Łukasz Bartosik <ukaszb@xxxxxxxxxxxx>
> > > > > > > > Signed-off-by: Łukasz Bartosik <ukaszb@xxxxxxxxxxxx>
> > > > > > > > ---
> > > > > > > > MAINTAINERS | 7 +
> > > > > > > > drivers/usb/typec/ucsi/Kconfig | 13 ++
> > > > > > > > drivers/usb/typec/ucsi/Makefile | 1 +
> > > > > > > > drivers/usb/typec/ucsi/cros_ec_ucsi.c | 248 ++++++++++++++++++++++++++
> > > > > > > > 4 files changed, 269 insertions(+)
> > > > > > > > create mode 100644 drivers/usb/typec/ucsi/cros_ec_ucsi.c
> > > > > > >
> > > > > > > > +static int cros_ucsi_sync_control(struct ucsi *ucsi, u64 cmd)
> > > > > > > > +{
> > > > > > > > + return ucsi_sync_control_common(ucsi, cmd);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +struct ucsi_operations cros_ucsi_ops = {
> > > > > > > > + .read_version = cros_ucsi_read_version,
> > > > > > > > + .read_cci = cros_ucsi_read_cci,
> > > > > > > > + .read_message_in = cros_ucsi_read_message_in,
> > > > > > > > + .async_control = cros_ucsi_async_control,
> > > > > > > > + .sync_control = cros_ucsi_sync_control,
> > > > > > >
> > > > > > > .sync_control = ucsi_sync_control_common,
> > > > > > >
> > > > > >
> > > > > > I will change.
> > > > > >
> > > > > > > > +};
> > > > > > > > +
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > +
> > > > > > > > +static int __maybe_unused cros_ucsi_suspend(struct device *dev)
> > > > > > > > +{
> > > > > > > > + struct cros_ucsi_data *udata = dev_get_drvdata(dev);
> > > > > > > > +
> > > > > > > > + cancel_work_sync(&udata->work);
> > > > > > > > +
> > > > > > > > + return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void __maybe_unused cros_ucsi_complete(struct device *dev)
> > > > > > > > +{
> > > > > > > > + struct cros_ucsi_data *udata = dev_get_drvdata(dev);
> > > > > > > > +
> > > > > > > > + ucsi_resume(udata->ucsi);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static const struct dev_pm_ops cros_ucsi_pm_ops = {
> > > > > > > > +#ifdef CONFIG_PM_SLEEP
> > > > > > > > + .suspend = cros_ucsi_suspend,
> > > > > > > > + .complete = cros_ucsi_complete,
> > > > > > > > +#endif
> > > > > > >
> > > > > > > SET_SYSTEM_SLEEP_PM_OPS ? Or even better, DEFINE_SIMPLE_DEV_PM_OPS() ?
> > > > > > >
> > > > > > > What is the reason for using complete() instead of resume()?
> > > > > > >
> > > > > >
> > > > > > Due to change in
> > > > > > https://lore.kernel.org/linux-usb/20240910101527.603452-1-ukaszb@xxxxxxxxxxxx/T/#m25bc17cc0a8d30439830415018c7f44f342900d1
> > > > > > we moved from using macro SIMPLE_DEV_PM_OPS and resume() to complete().
> > > > > > Per Heikki's suggestion I also squashed this change into "usb: typec:
> > > > > > ucsi: Implement ChromeOS UCSI driver".
> > > > >
> > > > > Neither original patch, nor this one document, why this is necessary
> > > > >
> > > >
> > > > The https://lore.kernel.org/linux-usb/20240910101527.603452-1-ukaszb@xxxxxxxxxxxx/T/#m25bc17cc0a8d30439830415018c7f44f342900d1
> > > > commit messages says:
> > > > "On platforms using cros_ec_lpc, resume is split into .resume_early
> > > > and .complete.
> > > > To avoid missing EC events, use .complete to schedule work when resuming."
> > > >
> > > > I will add this as a comment on top of cros_ucsi_pm_ops struct.
> > > > Do you find it sufficient ?
> > >
> > > IMHO, no
> > >
> >
> > Ok. If you please tell me what is not clear or missing in this
> > explanation in your opinion
> > then I will update it.
>
> For me it is a question of imbalance. The .complete() should be paired
> with .prepare(), .suspend() with .resume(), etc. Also by using just
> .suspend and .complete you are missing all other suspend/resume cases
> which are covered by SET_SYSTEM_SLEEP_PM_OPS() or
> SET_LATE_SYSTEM_SLEEP_PM_OPS().
>
> So, back to your question. I'm reviewing the UCSI part. I don't know
> anything about the cros_ec_lpc.c or any other CrOS EC drivers. You are
> doing some non-standard thing in your driver. As such you should provide
> a sensible explanation for it. If I understand correctly, it might be
> something in line with "UCSI is used with the systems using
> cros_ec_lpc.c. On such systems ... is not available until the
> .complete() callback of the cros_ec_lpc is executed. For this reason,
> delay ucsi_resume() until the .complete() stage."
>
I will incorporate your comment.
Thanks,
Lukasz
> >
> > Thanks,
> > Lukasz
> >
> > > >
> > > > Thanks,
> > > > Lukasz
> > > >
> > > > > >
> > > > > > Thank you,
> > > > > > Lukasz
> > > > > >
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static const struct platform_device_id cros_ucsi_id[] = {
> > > > > > > > + { KBUILD_MODNAME, 0 },
> > > > > > > > + {}
> > > > > > > > +};
> > > > > > > > +MODULE_DEVICE_TABLE(platform, cros_ucsi_id);
> > > > > > > > +
> > > > > > > > +static struct platform_driver cros_ucsi_driver = {
> > > > > > > > + .driver = {
> > > > > > > > + .name = KBUILD_MODNAME,
> > > > > > > > + .pm = &cros_ucsi_pm_ops,
> > > > > > > > + },
> > > > > > > > + .id_table = cros_ucsi_id,
> > > > > > > > + .probe = cros_ucsi_probe,
> > > > > > > > + .remove = cros_ucsi_remove,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +module_platform_driver(cros_ucsi_driver);
> > > > > > > > +
> > > > > > > > +MODULE_LICENSE("GPL");
> > > > > > > > +MODULE_DESCRIPTION("UCSI driver for ChromeOS EC");
> > > > > > > > --
> > > > > > > > 2.47.0.338.g60cca15819-goog
> > > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > With best wishes
> > > > > > > Dmitry
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > With best wishes
> > > > > Dmitry
> > >
> > >
> > >
> > > --
> > > With best wishes
> > > Dmitry
>
> --
> With best wishes
> Dmitry
