Re: [PATCH v7 2/3] usb: typec: ucsi: Implement ChromeOS UCSI driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

>
> 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





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux