Hello,
On Wed, Oct 03, 2018 at 05:50:04PM +0200, ektor5 wrote:
> Hi Jacopo,
> Thanks for the quick reply, I will respond inline,
>
> On Wed, Oct 03, 2018 at 11:35:32AM +0200, jacopo mondi wrote:
> > Hi Ettore,
> > thanks for the patch.
> >
> > A few comments below, please have a look...
> >
> > On Tue, Oct 02, 2018 at 06:59:55PM +0200, ektor5 wrote:
> > > From: Ettore Chimenti <ek5.chimenti@xxxxxxxxx>
> > > +/* ----------------------------------------------------------------------- */
[snip]
> > > +
> > > +#ifdef CONFIG_PM_SLEEP
> >
> > I see CONFIG_PM_SLEEP is only selected if support for
> > 'suspend'/'hibernate' is enabled. Is this what you want, or you should
> > check for CONFIG_PM?
>
> I was just inspired by the implementation of cros-ec-cec, but I think
> this is right, because the device actually has suspend/hibernate states.
>
Your device maybe does... I feel like CONFIG_PM is the right choice, but
I let others to comment further.
> >
> > > +static int secocec_suspend(struct device *dev)
> > > +{
> > > + u16 val;
> > > + int status;
> > > +
> > > + dev_dbg(dev, "Device going to suspend, disabling");
> > > +
> > > + /* Clear the status register */
> > > + status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> > > + if (status)
> > > + goto err;
> > > +
> > > + status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> > > + if (status)
> > > + goto err;
> > > +
> > > + /* Disable the interrupts */
> > > + status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> > > + if (status)
> > > + goto err;
> > > +
> > > + status = smb_wr16(SECOCEC_ENABLE_REG_1, val &
> > > + ~SECOCEC_ENABLE_REG_1_CEC & ~SECOCEC_ENABLE_REG_1_IR);
> > > + if (status)
> > > + goto err;
> > > +
> > > + return 0;
> > > +
> > > +err:
> > > + dev_err(dev, "Suspend failed (err: %d)", status);
> > > + return status;
> > > +}
> > > +
> > > +static int secocec_resume(struct device *dev)
> > > +{
> > > + u16 val;
> > > + int status;
> > > +
> > > + dev_dbg(dev, "Resuming device from suspend");
> > > +
> > > + /* Clear the status register */
> > > + status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> > > + if (status)
> > > + goto err;
> > > +
> > > + status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> > > + if (status)
> > > + goto err;
> > > +
> > > + /* Enable the interrupts */
> > > + status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> > > + if (status)
> > > + goto err;
> > > +
> > > + status = smb_wr16(SECOCEC_ENABLE_REG_1, val | SECOCEC_ENABLE_REG_1_CEC);
> > > + if (status)
> > > + goto err;
> > > +
> > > + dev_dbg(dev, "Device resumed from suspend");
> > > +
> > > + return 0;
> > > +
> > > +err:
> > > + dev_err(dev, "Resume failed (err: %d)", status);
> > > + return status;
> > > +}
> > > +
> > > +static SIMPLE_DEV_PM_OPS(secocec_pm_ops, secocec_suspend, secocec_resume);
> > > +#define SECOCEC_PM_OPS (&secocec_pm_ops)
> > > +#else
> > > +#define SECOCEC_PM_OPS NULL
> > > +#endif
> > > +
> > > +#ifdef CONFIG_ACPI
> > > +static const struct acpi_device_id secocec_acpi_match[] = {
> > > + {"CEC00001", 0},
> > > + {},
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(acpi, secocec_acpi_match);
> > > +#endif
> > > +
> > > +static struct platform_driver secocec_driver = {
> > > + .driver = {
> > > + .name = SECOCEC_DEV_NAME,
> > > + .acpi_match_table = ACPI_PTR(secocec_acpi_match),
> > > + .pm = SECOCEC_PM_OPS,
> > > + },
> > > + .probe = secocec_probe,
> > > + .remove = secocec_remove,
> > > +};
> >
> > As you can see most of my comments are nits or trivial things. I would
> > wait for more feedbacks on the CEC and x86/SMbus part from others before
> > sending v2 if I were you :)
> >
> > Thanks
> > j
> >
>
> Many thanks, this is my first patch, so I need plenty of comments. :)
>
I wish my first patches were as good as this one!
Thanks for sharing
j
Attachment:
signature.asc
Description: PGP signature
