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

[Thierry Reding]

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

Attachment: pgphpHHsEq740.pgp
Description: PGP signature