Messages in this thread | | | Date | Thu, 17 Apr 2014 13:34:59 +0200 | From | Thierry Reding <> | 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 [unhandled content-type:application/pgp-signature] | |