Re: [PATCH v1 8/8] pwm-pca9685: enable ACPI device found on Galileo Gen2

[Thierry Reding <thierry.reding@gmail.com>]

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

On Wed, Sep 23, 2015 at 11:41:26AM +0300, Andy Shevchenko wrote:
> On Tue, 2015-09-22 at 16:37 +0200, Thierry Reding wrote:
> > On Tue, Sep 22, 2015 at 01:10:19PM +0300, Andy Shevchenko wrote:
> 
> > > --- a/drivers/pwm/pwm-pca9685.c
> > > +++ b/drivers/pwm/pwm-pca9685.c
> > > @@ -26,6 +26,8 @@
> > >  #include <linux/regmap.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/delay.h>
> > > +#include <linux/acpi.h>
> > > +#include <linux/property.h>
> > 
> > These should be kept sorted. I know that delay.h isn't properly 
> > ordered
> > either, I missed that during patch review. Please keep new ones 
> > ordered
> > alphabetically and I'll sort out the delay.h via a separate patch.
> 
> Will do in next version.
> 
> > @@ -363,6 +364,12 @@ static const struct i2c_device_id pca9685_id[]
> > > = {
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, pca9685_id);
> > >  
> > > +static const struct acpi_device_id pca9685_acpi_ids[] = {
> > > +	{ "INT3492", 0 },
> > > +	{ /* sentinel */ },
> > > +};
> > > +MODULE_DEVICE_TABLE(acpi, pca9685_acpi_ids);
> > > +
> > >  static const struct of_device_id pca9685_dt_ids[] = {
> > >  	{ .compatible = "nxp,pca9685-pwm", },
> > >  	{ /* sentinel */ }
> > > @@ -372,6 +379,7 @@ MODULE_DEVICE_TABLE(of, pca9685_dt_ids);
> > >  static struct i2c_driver pca9685_i2c_driver = {
> > >  	.driver = {
> > >  		.name = "pca9685-pwm",
> > > +		.acpi_match_table = ACPI_PTR(pca9685_acpi_ids),
> > 
> > I think you now need #ifdef protection for the ACPI ID table, 
> > otherwise
> > the compiler will warn that the table is unused for !ACPI.
> 
> No, there is no warning, just checked a build with CONFIG_ACPI=n.
> 
> Tried even with C=1 W=2, and driver compiled in and a module.
> In all variants no warning regarding the topic is issued.
> 
> $ gcc --version
> gcc (Debian 5.2.1-17) 5.2.1 20150911
> 
> Perhaps this would explain what is happening there.
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=28901
> 
> So, I will add #ifdef in the code as well, though I'm not a big fan of
> conditional compilation.

I'm pretty sure I've seen warnings for this with 5.2.0, but I'll put
this in my tree to check. Irrespective I think it should have the #ifdef
protection because I'm very certain that the warning is there with some
versions of GCC that people might still be using. And I don't much like
conditional compilation either, but anything producing a warning will
cause someone to write a patch to fix it, so I just want to be proactive
in avoiding that kind of churn.

> > 
> > >  		.of_match_table = pca9685_dt_ids,
> > 
> > Similarly to the above, this should use of_match_ptr(), which in turn
> > will require #ifdef protection for the table to avoid warnings.
> 
> Hmm... my patch do not touches that part. Perhaps another patch for
> this?

Your patch does touch that part by removing the dependency on OF. That
makes it possible to build this code with !OF, which in turn would make
the OF match table unused.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]