Re: [PATCH 1/2] media: add SECO cec driver

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 3793 bytes --]

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@gmail.com>
> > > +/* ----------------------------------------------------------------------- */

[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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]