linux-kernel - Re: [PATCH v2] pwm_lpss: Add support for PCI devices

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:	Thu, 17 Apr 2014 13:34:59 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
Cc:	Chew Chiau Ee <chiau.ee.chew@...el.com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Alan Cox <alan@...ux.intel.com>, linux-pwm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] pwm_lpss: Add support for PCI devices

On Thu, Apr 17, 2014 at 12:16:43PM +0100, One Thousand Gnomes wrote:
> > > +static int pci_drv, plat_drv;	/* So we know which drivers registered */
> > 
> > I think that rather than having everything in a single file, perhaps a
> > better approach would be to keep pwm-lpss.c as a common module and then
> > have separate drivers for ACPI (pwm-lpss-acpi) and PCI (pwm-lpss-pci).
> > That way you don't have to keep track of which driver was successfully
> > registered.
> 
> It would then take up 16K for a tiny trivial piece of code

It would help make the driver somewhat less cluttered from a code point
of view. And I suspect that 16 KiB doesn't really matter all that much
on the platforms where this is used.

But if you prefer not to do the split that's fine with me too.

> > > +static const struct pwm_lpss_boardinfo byt_info = {
> > 
> > What does byt_ stand for?
> 
> Baytrail.

Okay, that could use a comment since it's not mentioned anywhere else
and the PCI IDs don't give it away either.

> > > -static int pwm_lpss_probe(struct platform_device *pdev)
> > > +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev,
> > > +			struct resource *r, struct pwm_lpss_boardinfo *info)
> > 
> > Indentation is odd here. Please align arguments one subsequent lines
> > with those of the first.
> 
> That doesn't appear to be present in CodingStyle or indeed most of the
> kernel.

I'm used to it in the PWM subsystem and I'd like to keep it that way for
consistency.

Thierry

Content of type "application/pgp-signature" skipped