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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 3 Oct 2018 18:14:01 +0200
From:   jacopo mondi <jacopo@...ndi.org>
To:     ektor5 <ek5.chimenti@...il.com>
Cc:     hverkuil@...all.nl, luca.pisani@...o.org, jose.abreu@...opsys.com,
        sean@...s.org, sakari.ailus@...ux.intel.com,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Jacob Chen <jacob-chen@...wrt.com>,
        Jacopo Mondi <jacopo+renesas@...ndi.org>,
        Neil Armstrong <narmstrong@...libre.com>,
        Todor Tomov <todor.tomov@...aro.org>,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH 1/2] media: add SECO cec driver

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@...il.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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)