| Message ID | 1442916619-140607-9-git-send-email-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 22, 2015 at 01:10:19PM +0300, Andy Shevchenko wrote: > There is a chip connected to i2c bus on Intel Galileo Gen2 board. Enable it via > ACPI ID INT3492. > > Cc: Thierry Reding <thierry.reding@gmail.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pwm/Kconfig | 2 +- > drivers/pwm/pwm-pca9685.c | 14 +++++++++++--- > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 062630a..bb114ef 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -242,7 +242,7 @@ config PWM_MXS > > config PWM_PCA9685 > tristate "NXP PCA9685 PWM driver" > - depends on OF && I2C > + depends on I2C > select REGMAP_I2C > help > Generic PWM framework driver for NXP PCA9685 LED controller. > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > index 70448a6..38ea28b 100644 > --- 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. > > /* > * Because the PCA9685 has only one prescaler per chip, changing the period of > @@ -297,7 +299,6 @@ static const struct regmap_config pca9685_regmap_i2c_config = { > static int pca9685_pwm_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > - struct device_node *np = client->dev.of_node; > struct pca9685 *pca; > int ret; > int mode2; > @@ -320,12 +321,12 @@ static int pca9685_pwm_probe(struct i2c_client *client, > > regmap_read(pca->regmap, PCA9685_MODE2, &mode2); > > - if (of_property_read_bool(np, "invert")) > + if (device_property_read_bool(&client->dev, "invert")) > mode2 |= MODE2_INVRT; > else > mode2 &= ~MODE2_INVRT; > > - if (of_property_read_bool(np, "open-drain")) > + if (device_property_read_bool(&client->dev, "open-drain")) > mode2 &= ~MODE2_OUTDRV; > else > mode2 |= MODE2_OUTDRV; > @@ -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. > .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. Thierry
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. > > > .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?
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
On Wed, 2015-09-23 at 14:48 +0200, Thierry Reding wrote: > > > > > 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: > > > > > > > > > .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. Ah, you're right. Will amend this part as well. Thanks! > > Thierry
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 062630a..bb114ef 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -242,7 +242,7 @@ config PWM_MXS config PWM_PCA9685 tristate "NXP PCA9685 PWM driver" - depends on OF && I2C + depends on I2C select REGMAP_I2C help Generic PWM framework driver for NXP PCA9685 LED controller. diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c index 70448a6..38ea28b 100644 --- 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> /* * Because the PCA9685 has only one prescaler per chip, changing the period of @@ -297,7 +299,6 @@ static const struct regmap_config pca9685_regmap_i2c_config = { static int pca9685_pwm_probe(struct i2c_client *client, const struct i2c_device_id *id) { - struct device_node *np = client->dev.of_node; struct pca9685 *pca; int ret; int mode2; @@ -320,12 +321,12 @@ static int pca9685_pwm_probe(struct i2c_client *client, regmap_read(pca->regmap, PCA9685_MODE2, &mode2); - if (of_property_read_bool(np, "invert")) + if (device_property_read_bool(&client->dev, "invert")) mode2 |= MODE2_INVRT; else mode2 &= ~MODE2_INVRT; - if (of_property_read_bool(np, "open-drain")) + if (device_property_read_bool(&client->dev, "open-drain")) mode2 &= ~MODE2_OUTDRV; else mode2 |= MODE2_OUTDRV; @@ -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), .of_match_table = pca9685_dt_ids, }, .probe = pca9685_pwm_probe,
There is a chip connected to i2c bus on Intel Galileo Gen2 board. Enable it via ACPI ID INT3492. Cc: Thierry Reding <thierry.reding@gmail.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/pwm/Kconfig | 2 +- drivers/pwm/pwm-pca9685.c | 14 +++++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-)