[v1,8/8] pwm-pca9685: enable ACPI device found on Galileo Gen2
diff mbox

Message ID 1442916619-140607-9-git-send-email-andriy.shevchenko@linux.intel.com
State New
Headers show

Commit Message

Andy Shevchenko Sept. 22, 2015, 10:10 a.m. UTC
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(-)

Comments

Thierry Reding Sept. 22, 2015, 2:37 p.m. UTC | #1
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
Andy Shevchenko Sept. 23, 2015, 8:41 a.m. UTC | #2
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?
Thierry Reding Sept. 23, 2015, 12:48 p.m. UTC | #3
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
Andy Shevchenko Sept. 23, 2015, 12:56 p.m. UTC | #4
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

Patch
diff mbox

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,