[PATCH V3] leds: pca955x: Add ACPI support for pca955x

[PATCH V3] leds: pca955x: Add ACPI support for pca955x

[Tin Huynh]

This patch enables ACPI support for leds-pca955x driver.

Signed-off-by: Tin Huynh <tnhuynh@apm.com>
---
 drivers/leds/leds-pca955x.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

Change from V2:
 -Correct coding conventions.

Change from V1: 
 -Remove CONFIG_ACPI.

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 840401a..b168ebe 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -40,6 +40,7 @@
  *  bits the chip supports.
  */
 
+#include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/delay.h>
 #include <linux/string.h>
@@ -100,6 +101,15 @@ struct pca955x_chipdef {
 };
 MODULE_DEVICE_TABLE(i2c, pca955x_id);
 
+static const struct acpi_device_id pca955x_acpi_ids[] = {
+	{ .id = "PCA9550", .driver_data = pca9550 },
+	{ .id = "PCA9551", .driver_data = pca9551 },
+	{ .id = "PCA9552", .driver_data = pca9552 },
+	{ .id = "PCA9553", .driver_data = pca9553 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, pca955x_acpi_ids);
+
 struct pca955x {
 	struct mutex lock;
 	struct pca955x_led *leds;
@@ -250,7 +260,16 @@ static int pca955x_probe(struct i2c_client *client,
 	struct led_platform_data *pdata;
 	int i, err;
 
-	chip = &pca955x_chipdefs[id->driver_data];
+	if (id) {
+		chip = &pca955x_chipdefs[id->driver_data];
+	} else {
+		const struct acpi_device_id *acpi_id;
+
+		acpi_id = acpi_match_device(pca955x_acpi_ids, &client->dev);
+		if (!acpi_id)
+			return -ENODEV;
+		chip = &pca955x_chipdefs[acpi_id->driver_data];
+	}
 	adapter = to_i2c_adapter(client->dev.parent);
 	pdata = dev_get_platdata(&client->dev);
 
@@ -358,6 +377,7 @@ static int pca955x_remove(struct i2c_client *client)
 static struct i2c_driver pca955x_driver = {
 	.driver = {
 		.name	= "leds-pca955x",
+		.acpi_match_table = ACPI_PTR(pca955x_acpi_ids),
 	},
 	.probe	= pca955x_probe,
 	.remove	= pca955x_remove,
-- 
1.7.1

Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x

[Jacek Anaszewski]

Hi Tin,

How this patch is different from the one already merged?

Best regards,
Jacek Anaszewski

On 11/30/2016 04:08 AM, Tin Huynh wrote:
> This patch enables ACPI support for leds-pca955x driver.
>
> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
> ---
>  drivers/leds/leds-pca955x.c |   22 +++++++++++++++++++++-
>  1 files changed, 21 insertions(+), 1 deletions(-)
>
> Change from V2:
>  -Correct coding conventions.
>
> Change from V1:
>  -Remove CONFIG_ACPI.
>
> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
> index 840401a..b168ebe 100644
> --- a/drivers/leds/leds-pca955x.c
> +++ b/drivers/leds/leds-pca955x.c
> @@ -40,6 +40,7 @@
>   *  bits the chip supports.
>   */
>
> +#include <linux/acpi.h>
>  #include <linux/module.h>
>  #include <linux/delay.h>
>  #include <linux/string.h>
> @@ -100,6 +101,15 @@ struct pca955x_chipdef {
>  };
>  MODULE_DEVICE_TABLE(i2c, pca955x_id);
>
> +static const struct acpi_device_id pca955x_acpi_ids[] = {
> +	{ .id = "PCA9550", .driver_data = pca9550 },
> +	{ .id = "PCA9551", .driver_data = pca9551 },
> +	{ .id = "PCA9552", .driver_data = pca9552 },
> +	{ .id = "PCA9553", .driver_data = pca9553 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, pca955x_acpi_ids);
> +
>  struct pca955x {
>  	struct mutex lock;
>  	struct pca955x_led *leds;
> @@ -250,7 +260,16 @@ static int pca955x_probe(struct i2c_client *client,
>  	struct led_platform_data *pdata;
>  	int i, err;
>
> -	chip = &pca955x_chipdefs[id->driver_data];
> +	if (id) {
> +		chip = &pca955x_chipdefs[id->driver_data];
> +	} else {
> +		const struct acpi_device_id *acpi_id;
> +
> +		acpi_id = acpi_match_device(pca955x_acpi_ids, &client->dev);
> +		if (!acpi_id)
> +			return -ENODEV;
> +		chip = &pca955x_chipdefs[acpi_id->driver_data];
> +	}
>  	adapter = to_i2c_adapter(client->dev.parent);
>  	pdata = dev_get_platdata(&client->dev);
>
> @@ -358,6 +377,7 @@ static int pca955x_remove(struct i2c_client *client)
>  static struct i2c_driver pca955x_driver = {
>  	.driver = {
>  		.name	= "leds-pca955x",
> +		.acpi_match_table = ACPI_PTR(pca955x_acpi_ids),
>  	},
>  	.probe	= pca955x_probe,
>  	.remove	= pca955x_remove,
>

Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x

[Jacek Anaszewski]

On 11/30/2016 08:51 AM, Jacek Anaszewski wrote:
> Hi Tin,
>
> How this patch is different from the one already merged?
>
> Best regards,
> Jacek Anaszewski
>
> On 11/30/2016 04:08 AM, Tin Huynh wrote:
>> This patch enables ACPI support for leds-pca955x driver.
>>
>> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
>> ---
>>  drivers/leds/leds-pca955x.c |   22 +++++++++++++++++++++-
>>  1 files changed, 21 insertions(+), 1 deletions(-)
>>
>> Change from V2:
>>  -Correct coding conventions.
>>
>> Change from V1:
>>  -Remove CONFIG_ACPI.
>>
>> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
>> index 840401a..b168ebe 100644
>> --- a/drivers/leds/leds-pca955x.c
>> +++ b/drivers/leds/leds-pca955x.c
>> @@ -40,6 +40,7 @@
>>   *  bits the chip supports.
>>   */
>>
>> +#include <linux/acpi.h>
>>  #include <linux/module.h>
>>  #include <linux/delay.h>
>>  #include <linux/string.h>
>> @@ -100,6 +101,15 @@ struct pca955x_chipdef {
>>  };
>>  MODULE_DEVICE_TABLE(i2c, pca955x_id);
>>
>> +static const struct acpi_device_id pca955x_acpi_ids[] = {
>> +    { .id = "PCA9550", .driver_data = pca9550 },
>> +    { .id = "PCA9551", .driver_data = pca9551 },
>> +    { .id = "PCA9552", .driver_data = pca9552 },
>> +    { .id = "PCA9553", .driver_data = pca9553 },
>> +    { }

OK, I see that you brought back explicit properties in the
structure initializer. Is there some vital reason for that?
You're mentioning "correcting coding conventions" in the
patch changelog. checkpatch.pl --strict doesn't complain about
that, so what coding conventions you have on mind?

>> +MODULE_DEVICE_TABLE(acpi, pca955x_acpi_ids);
>> +
>>  struct pca955x {
>>      struct mutex lock;
>>      struct pca955x_led *leds;
>> @@ -250,7 +260,16 @@ static int pca955x_probe(struct i2c_client *client,
>>      struct led_platform_data *pdata;
>>      int i, err;
>>
>> -    chip = &pca955x_chipdefs[id->driver_data];
>> +    if (id) {
>> +        chip = &pca955x_chipdefs[id->driver_data];
>> +    } else {
>> +        const struct acpi_device_id *acpi_id;
>> +
>> +        acpi_id = acpi_match_device(pca955x_acpi_ids, &client->dev);
>> +        if (!acpi_id)
>> +            return -ENODEV;
>> +        chip = &pca955x_chipdefs[acpi_id->driver_data];
>> +    }
>>      adapter = to_i2c_adapter(client->dev.parent);
>>      pdata = dev_get_platdata(&client->dev);
>>
>> @@ -358,6 +377,7 @@ static int pca955x_remove(struct i2c_client *client)
>>  static struct i2c_driver pca955x_driver = {
>>      .driver = {
>>          .name    = "leds-pca955x",
>> +        .acpi_match_table = ACPI_PTR(pca955x_acpi_ids),
>>      },
>>      .probe    = pca955x_probe,
>>      .remove    = pca955x_remove,
>>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-leds" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x

[Tin Huynh]

On Wed, Nov 30, 2016 at 3:01 PM, Jacek Anaszewski
<j.anaszewski@samsung.com> wrote:
>
> On 11/30/2016 08:51 AM, Jacek Anaszewski wrote:
>>
>> Hi Tin,
>>
>> How this patch is different from the one already merged?
>>
>> Best regards,
>> Jacek Anaszewski
>>
>> On 11/30/2016 04:08 AM, Tin Huynh wrote:
>>>
>>> This patch enables ACPI support for leds-pca955x driver.
>>>
>>> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
>>> ---
>>>  drivers/leds/leds-pca955x.c |   22 +++++++++++++++++++++-
>>>  1 files changed, 21 insertions(+), 1 deletions(-)
>>>
>>> Change from V2:
>>>  -Correct coding conventions.
>>>
>>> Change from V1:
>>>  -Remove CONFIG_ACPI.
>>>
>>> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
>>> index 840401a..b168ebe 100644
>>> --- a/drivers/leds/leds-pca955x.c
>>> +++ b/drivers/leds/leds-pca955x.c
>>> @@ -40,6 +40,7 @@
>>>   *  bits the chip supports.
>>>   */
>>>
>>> +#include <linux/acpi.h>
>>>  #include <linux/module.h>
>>>  #include <linux/delay.h>
>>>  #include <linux/string.h>
>>> @@ -100,6 +101,15 @@ struct pca955x_chipdef {
>>>  };
>>>  MODULE_DEVICE_TABLE(i2c, pca955x_id);
>>>
>>> +static const struct acpi_device_id pca955x_acpi_ids[] = {
>>> +    { .id = "PCA9550", .driver_data = pca9550 },
>>> +    { .id = "PCA9551", .driver_data = pca9551 },
>>> +    { .id = "PCA9552", .driver_data = pca9552 },
>>> +    { .id = "PCA9553", .driver_data = pca9553 },
>>> +    { }
>
>
> OK, I see that you brought back explicit properties in the
> structure initializer. Is there some vital reason for that?
> You're mentioning "correcting coding conventions" in the
> patch changelog. checkpatch.pl --strict doesn't complain about
> that, so what coding conventions you have on mind?


>
>
>>> +MODULE_DEVICE_TABLE(acpi, pca955x_acpi_ids);
>>> +
>>>  struct pca955x {
>>>      struct mutex lock;
>>>      struct pca955x_led *leds;
>>> @@ -250,7 +260,16 @@ static int pca955x_probe(struct i2c_client *client,
>>>      struct led_platform_data *pdata;
>>>      int i, err;
>>>
>>> -    chip = &pca955x_chipdefs[id->driver_data];
>>> +    if (id) {
>>> +        chip = &pca955x_chipdefs[id->driver_data];
>>> +    } else {
>>> +        const struct acpi_device_id *acpi_id;

I added '{}' follow if

Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x

[Jacek Anaszewski]

On 11/30/2016 09:06 AM, Tin Huynh wrote:
> On Wed, Nov 30, 2016 at 3:01 PM, Jacek Anaszewski
> <j.anaszewski@samsung.com> wrote:
>>
>> On 11/30/2016 08:51 AM, Jacek Anaszewski wrote:
>>>
>>> Hi Tin,
>>>
>>> How this patch is different from the one already merged?
>>>
>>> Best regards,
>>> Jacek Anaszewski
>>>
>>> On 11/30/2016 04:08 AM, Tin Huynh wrote:
>>>>
>>>> This patch enables ACPI support for leds-pca955x driver.
>>>>
>>>> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
>>>> ---
>>>>  drivers/leds/leds-pca955x.c |   22 +++++++++++++++++++++-
>>>>  1 files changed, 21 insertions(+), 1 deletions(-)
>>>>
>>>> Change from V2:
>>>>  -Correct coding conventions.
>>>>
>>>> Change from V1:
>>>>  -Remove CONFIG_ACPI.
>>>>
>>>> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
>>>> index 840401a..b168ebe 100644
>>>> --- a/drivers/leds/leds-pca955x.c
>>>> +++ b/drivers/leds/leds-pca955x.c
>>>> @@ -40,6 +40,7 @@
>>>>   *  bits the chip supports.
>>>>   */
>>>>
>>>> +#include <linux/acpi.h>
>>>>  #include <linux/module.h>
>>>>  #include <linux/delay.h>
>>>>  #include <linux/string.h>
>>>> @@ -100,6 +101,15 @@ struct pca955x_chipdef {
>>>>  };
>>>>  MODULE_DEVICE_TABLE(i2c, pca955x_id);
>>>>
>>>> +static const struct acpi_device_id pca955x_acpi_ids[] = {
>>>> +    { .id = "PCA9550", .driver_data = pca9550 },
>>>> +    { .id = "PCA9551", .driver_data = pca9551 },
>>>> +    { .id = "PCA9552", .driver_data = pca9552 },
>>>> +    { .id = "PCA9553", .driver_data = pca9553 },
>>>> +    { }
>>
>>
>> OK, I see that you brought back explicit properties in the
>> structure initializer. Is there some vital reason for that?
>> You're mentioning "correcting coding conventions" in the
>> patch changelog. checkpatch.pl --strict doesn't complain about
>> that, so what coding conventions you have on mind?
>
>
>>
>>
>>>> +MODULE_DEVICE_TABLE(acpi, pca955x_acpi_ids);
>>>> +
>>>>  struct pca955x {
>>>>      struct mutex lock;
>>>>      struct pca955x_led *leds;
>>>> @@ -250,7 +260,16 @@ static int pca955x_probe(struct i2c_client *client,
>>>>      struct led_platform_data *pdata;
>>>>      int i, err;
>>>>
>>>> -    chip = &pca955x_chipdefs[id->driver_data];
>>>> +    if (id) {
>>>> +        chip = &pca955x_chipdefs[id->driver_data];
>>>> +    } else {
>>>> +        const struct acpi_device_id *acpi_id;
>
> I added '{}' follow if

You had it already in V1. Please verify if the patch applied
to the for-next branch of linux-leds.git has the shape you intended:

https://git.kernel.org/cgit/linux/kernel/git/j.anaszewski/linux-leds.git/commit/?h=for-next&id=e46895b71a26da404c4d95cb2bab1a67cf8b20bc

-- 
Best regards,
Jacek Anaszewski

RE: [PATCH V3] leds: pca955x: Add ACPI support for pca955x

[Phong Vo]

+-----Original Message-----
+From: Jacek Anaszewski [mailto:j.anaszewski@samsung.com]
+Sent: Wednesday, November 30, 2016 3:18 PM
+To: Tin Huynh
+Cc: Mika Westerberg; Rafael J. Wysocki; Richard Purdie; linux-
+leds@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
+acpi@vger.kernel.org; Loc Ho; Thang Nguyen; Phong Vo; patches
+Subject: Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x
+
+On 11/30/2016 09:06 AM, Tin Huynh wrote:
+> On Wed, Nov 30, 2016 at 3:01 PM, Jacek Anaszewski
+> <j.anaszewski@samsung.com> wrote:
+>>
+>> On 11/30/2016 08:51 AM, Jacek Anaszewski wrote:
+>>>
+>>> Hi Tin,
+>>>
+>>> How this patch is different from the one already merged?
+>>>
+>>> Best regards,
+>>> Jacek Anaszewski
+>>>

Hi Jacek, I am answering on behalf of Tin.
This patch is for the leds:pca955x driver while the previous one was for
leds:pca963x driver!
They are almost the same in the coding construct, but different drivers.

+>>> On 11/30/2016 04:08 AM, Tin Huynh wrote:
+>>>>
+>>>> This patch enables ACPI support for leds-pca955x driver.
+>>>>
+>>>> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
+>>>> ---
+>>>>  drivers/leds/leds-pca955x.c |   22 +++++++++++++++++++++-
+>>>>  1 files changed, 21 insertions(+), 1 deletions(-)
+>>>>
+>>>> Change from V2:
+>>>>  -Correct coding conventions.
+>>>>
+>>>> Change from V1:
+>>>>  -Remove CONFIG_ACPI.
+>>>>
+>>>> diff --git a/drivers/leds/leds-pca955x.c
+>>>> b/drivers/leds/leds-pca955x.c index 840401a..b168ebe 100644
+>>>> --- a/drivers/leds/leds-pca955x.c
+>>>> +++ b/drivers/leds/leds-pca955x.c
+>>>> @@ -40,6 +40,7 @@
+>>>>   *  bits the chip supports.
+>>>>   */
+>>>>
+>>>> +#include <linux/acpi.h>
+>>>>  #include <linux/module.h>
+>>>>  #include <linux/delay.h>
+>>>>  #include <linux/string.h>
+>>>> @@ -100,6 +101,15 @@ struct pca955x_chipdef {  };
+>>>> MODULE_DEVICE_TABLE(i2c, pca955x_id);
+>>>>
+>>>> +static const struct acpi_device_id pca955x_acpi_ids[] = {
+>>>> +    { .id = "PCA9550", .driver_data = pca9550 },
+>>>> +    { .id = "PCA9551", .driver_data = pca9551 },
+>>>> +    { .id = "PCA9552", .driver_data = pca9552 },
+>>>> +    { .id = "PCA9553", .driver_data = pca9553 },
+>>>> +    { }
+>>
+>>
+>> OK, I see that you brought back explicit properties in the structure
+>> initializer. Is there some vital reason for that?

It's not vital, but to make it consistent with what was done for pca963x,
and also per suggestion by Mika on reviewing  a different driver mux:954x in
another thread.
I would think this would make the definition clearer.

+>> You're mentioning "correcting coding conventions" in the patch
+>> changelog. checkpatch.pl --strict doesn't complain about that, so
+>> what coding conventions you have on mind?
+>
+>
+>>
+>>
+>>>> +MODULE_DEVICE_TABLE(acpi, pca955x_acpi_ids);
+>>>> +
+>>>>  struct pca955x {
+>>>>      struct mutex lock;
+>>>>      struct pca955x_led *leds;
+>>>> @@ -250,7 +260,16 @@ static int pca955x_probe(struct i2c_client
+*client,
+>>>>      struct led_platform_data *pdata;
+>>>>      int i, err;
+>>>>
+>>>> -    chip = &pca955x_chipdefs[id->driver_data];
+>>>> +    if (id) {
+>>>> +        chip = &pca955x_chipdefs[id->driver_data];
+>>>> +    } else {
+>>>> +        const struct acpi_device_id *acpi_id;
+>
+> I added '{}' follow if
+
+You had it already in V1. Please verify if the patch applied to the for-
+next branch of linux-leds.git has the shape you intended:
+
+https://git.kernel.org/cgit/linux/kernel/git/j.anaszewski/linux-
+leds.git/commit/?h=for-next&id=e46895b71a26da404c4d95cb2bab1a67cf8b20bc
+
+--
+Best regards,
+Jacek Anaszewski

Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x

[Jacek Anaszewski]

Hi Phong,

On 11/30/2016 09:23 AM, Phong Vo wrote:
> +-----Original Message-----
> +From: Jacek Anaszewski [mailto:j.anaszewski@samsung.com]
> +Sent: Wednesday, November 30, 2016 3:18 PM
> +To: Tin Huynh
> +Cc: Mika Westerberg; Rafael J. Wysocki; Richard Purdie; linux-
> +leds@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> +acpi@vger.kernel.org; Loc Ho; Thang Nguyen; Phong Vo; patches
> +Subject: Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x
> +
> +On 11/30/2016 09:06 AM, Tin Huynh wrote:
> +> On Wed, Nov 30, 2016 at 3:01 PM, Jacek Anaszewski
> +> <j.anaszewski@samsung.com> wrote:
> +>>
> +>> On 11/30/2016 08:51 AM, Jacek Anaszewski wrote:
> +>>>
> +>>> Hi Tin,
> +>>>
> +>>> How this patch is different from the one already merged?
> +>>>
> +>>> Best regards,
> +>>> Jacek Anaszewski
> +>>>
>
> Hi Jacek, I am answering on behalf of Tin.
> This patch is for the leds:pca955x driver while the previous one was for
> leds:pca963x driver!
> They are almost the same in the coding construct, but different drivers.

Ah, indeed, that's why I got lost with patch version numbering :-)

> +>>> On 11/30/2016 04:08 AM, Tin Huynh wrote:
> +>>>>
> +>>>> This patch enables ACPI support for leds-pca955x driver.
> +>>>>
> +>>>> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
> +>>>> ---
> +>>>>  drivers/leds/leds-pca955x.c |   22 +++++++++++++++++++++-
> +>>>>  1 files changed, 21 insertions(+), 1 deletions(-)
> +>>>>
> +>>>> Change from V2:
> +>>>>  -Correct coding conventions.
> +>>>>
> +>>>> Change from V1:
> +>>>>  -Remove CONFIG_ACPI.
> +>>>>
> +>>>> diff --git a/drivers/leds/leds-pca955x.c
> +>>>> b/drivers/leds/leds-pca955x.c index 840401a..b168ebe 100644
> +>>>> --- a/drivers/leds/leds-pca955x.c
> +>>>> +++ b/drivers/leds/leds-pca955x.c
> +>>>> @@ -40,6 +40,7 @@
> +>>>>   *  bits the chip supports.
> +>>>>   */
> +>>>>
> +>>>> +#include <linux/acpi.h>
> +>>>>  #include <linux/module.h>
> +>>>>  #include <linux/delay.h>
> +>>>>  #include <linux/string.h>
> +>>>> @@ -100,6 +101,15 @@ struct pca955x_chipdef {  };
> +>>>> MODULE_DEVICE_TABLE(i2c, pca955x_id);
> +>>>>
> +>>>> +static const struct acpi_device_id pca955x_acpi_ids[] = {
> +>>>> +    { .id = "PCA9550", .driver_data = pca9550 },
> +>>>> +    { .id = "PCA9551", .driver_data = pca9551 },
> +>>>> +    { .id = "PCA9552", .driver_data = pca9552 },
> +>>>> +    { .id = "PCA9553", .driver_data = pca9553 },
> +>>>> +    { }
> +>>
> +>>
> +>> OK, I see that you brought back explicit properties in the structure
> +>> initializer. Is there some vital reason for that?
>
> It's not vital, but to make it consistent with what was done for pca963x,

For pca963x I applied the version without explicit properties.
Note that this is consistent with pca963x_id array above the added
pca963x_acpi_ids. For pca955x the situation is the same.

> and also per suggestion by Mika on reviewing  a different driver mux:954x in
> another thread.

Could you give a reference to that thread? In the review of V1 of this
patch Mika mentioned "{ "PCA9553", pca9553 }" scheme.

> I would think this would make the definition clearer.
>
> +>> You're mentioning "correcting coding conventions" in the patch
> +>> changelog. checkpatch.pl --strict doesn't complain about that, so
> +>> what coding conventions you have on mind?
> +>
> +>
> +>>
> +>>
> +>>>> +MODULE_DEVICE_TABLE(acpi, pca955x_acpi_ids);
> +>>>> +
> +>>>>  struct pca955x {
> +>>>>      struct mutex lock;
> +>>>>      struct pca955x_led *leds;
> +>>>> @@ -250,7 +260,16 @@ static int pca955x_probe(struct i2c_client
> +*client,
> +>>>>      struct led_platform_data *pdata;
> +>>>>      int i, err;
> +>>>>
> +>>>> -    chip = &pca955x_chipdefs[id->driver_data];
> +>>>> +    if (id) {
> +>>>> +        chip = &pca955x_chipdefs[id->driver_data];
> +>>>> +    } else {
> +>>>> +        const struct acpi_device_id *acpi_id;
> +>
> +> I added '{}' follow if
> +
> +You had it already in V1. Please verify if the patch applied to the for-
> +next branch of linux-leds.git has the shape you intended:
> +
> +https://git.kernel.org/cgit/linux/kernel/git/j.anaszewski/linux-
> +leds.git/commit/?h=for-next&id=e46895b71a26da404c4d95cb2bab1a67cf8b20bc
> +
> +--
> +Best regards,
> +Jacek Anaszewski
> --
> To unsubscribe from this list: send the line "unsubscribe linux-leds" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>


-- 
Best regards,
Jacek Anaszewski

RE: [PATCH V3] leds: pca955x: Add ACPI support for pca955x

[Phong Vo]

+-----Original Message-----
+From: Jacek Anaszewski [mailto:j.anaszewski@samsung.com]
+Sent: Wednesday, November 30, 2016 4:00 PM
+To: Phong Vo
+Cc: Mika Westerberg; Rafael J. Wysocki; Richard Purdie; linux-
+leds@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
+acpi@vger.kernel.org; Loc Ho; Thang Nguyen; patches; Tin Huynh
+Subject: Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x
+
+Hi Phong,
+
+On 11/30/2016 09:23 AM, Phong Vo wrote:
+> +-----Original Message-----
+> +From: Jacek Anaszewski [mailto:j.anaszewski@samsung.com]
+> +Sent: Wednesday, November 30, 2016 3:18 PM
+> +To: Tin Huynh
+> +Cc: Mika Westerberg; Rafael J. Wysocki; Richard Purdie; linux-
+> +leds@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
+> +acpi@vger.kernel.org; Loc Ho; Thang Nguyen; Phong Vo; patches
+> +Subject: Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x
+> +
+> +On 11/30/2016 09:06 AM, Tin Huynh wrote:
+> +> On Wed, Nov 30, 2016 at 3:01 PM, Jacek Anaszewski
+> +> <j.anaszewski@samsung.com> wrote:
+> +>>
+> +>> On 11/30/2016 08:51 AM, Jacek Anaszewski wrote:
+> +>>>
+> +>>> Hi Tin,
+> +>>>
+> +>>> How this patch is different from the one already merged?
+> +>>>
+> +>>> Best regards,
+> +>>> Jacek Anaszewski
+> +>>>
+>
+> Hi Jacek, I am answering on behalf of Tin.
+> This patch is for the leds:pca955x driver while the previous one was
+> for leds:pca963x driver!
+> They are almost the same in the coding construct, but different
+drivers.
+
+Ah, indeed, that's why I got lost with patch version numbering :-)
+
+> +>>> On 11/30/2016 04:08 AM, Tin Huynh wrote:
+> +>>>>
+> +>>>> This patch enables ACPI support for leds-pca955x driver.
+> +>>>>
+> +>>>> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
+> +>>>> ---
+> +>>>>  drivers/leds/leds-pca955x.c |   22 +++++++++++++++++++++-
+> +>>>>  1 files changed, 21 insertions(+), 1 deletions(-)
+> +>>>>
+> +>>>> Change from V2:
+> +>>>>  -Correct coding conventions.
+> +>>>>
+> +>>>> Change from V1:
+> +>>>>  -Remove CONFIG_ACPI.
+> +>>>>
+> +>>>> diff --git a/drivers/leds/leds-pca955x.c
+> +>>>> b/drivers/leds/leds-pca955x.c index 840401a..b168ebe 100644
+> +>>>> --- a/drivers/leds/leds-pca955x.c
+> +>>>> +++ b/drivers/leds/leds-pca955x.c
+> +>>>> @@ -40,6 +40,7 @@
+> +>>>>   *  bits the chip supports.
+> +>>>>   */
+> +>>>>
+> +>>>> +#include <linux/acpi.h>
+> +>>>>  #include <linux/module.h>
+> +>>>>  #include <linux/delay.h>
+> +>>>>  #include <linux/string.h>
+> +>>>> @@ -100,6 +101,15 @@ struct pca955x_chipdef {  };
+> +>>>> MODULE_DEVICE_TABLE(i2c, pca955x_id);
+> +>>>>
+> +>>>> +static const struct acpi_device_id pca955x_acpi_ids[] = {
+> +>>>> +    { .id = "PCA9550", .driver_data = pca9550 },
+> +>>>> +    { .id = "PCA9551", .driver_data = pca9551 },
+> +>>>> +    { .id = "PCA9552", .driver_data = pca9552 },
+> +>>>> +    { .id = "PCA9553", .driver_data = pca9553 },
+> +>>>> +    { }
+> +>>
+> +>>
+> +>> OK, I see that you brought back explicit properties in the
+> +>> structure initializer. Is there some vital reason for that?
+>
+> It's not vital, but to make it consistent with what was done for
+> pca963x,
+
+For pca963x I applied the version without explicit properties.
+Note that this is consistent with pca963x_id array above the added
+pca963x_acpi_ids. For pca955x the situation is the same.
+
+> and also per suggestion by Mika on reviewing  a different driver
+> mux:954x in another thread.
+
+Could you give a reference to that thread? In the review of V1 of this
+patch Mika mentioned "{ "PCA9553", pca9553 }" scheme.
+

Actually it was Peter Rosin (not Mika) on linux-i2c and the reference to
that is follows

https://lkml.org/lkml/2016/11/18/732

I am including Robin here.

Thanks.

+> I would think this would make the definition clearer.
+>
+> +>> You're mentioning "correcting coding conventions" in the patch
+> +>> changelog. checkpatch.pl --strict doesn't complain about that, so
+> +>> what coding conventions you have on mind?
+> +>
+> +>
+> +>>
+> +>>
+> +>>>> +MODULE_DEVICE_TABLE(acpi, pca955x_acpi_ids);
+> +>>>> +
+> +>>>>  struct pca955x {
+> +>>>>      struct mutex lock;
+> +>>>>      struct pca955x_led *leds;
+> +>>>> @@ -250,7 +260,16 @@ static int pca955x_probe(struct i2c_client
+> +*client,
+> +>>>>      struct led_platform_data *pdata;
+> +>>>>      int i, err;
+> +>>>>
+> +>>>> -    chip = &pca955x_chipdefs[id->driver_data];
+> +>>>> +    if (id) {
+> +>>>> +        chip = &pca955x_chipdefs[id->driver_data];
+> +>>>> +    } else {
+> +>>>> +        const struct acpi_device_id *acpi_id;
+> +>
+> +> I added '{}' follow if
+> +
+> +You had it already in V1. Please verify if the patch applied to the
+> +for- next branch of linux-leds.git has the shape you intended:
+> +
+> +https://git.kernel.org/cgit/linux/kernel/git/j.anaszewski/linux-
+> +leds.git/commit/?h=for-next&id=e46895b71a26da404c4d95cb2bab1a67cf8b20
+> +bc
+> +
+> +--
+> +Best regards,
+> +Jacek Anaszewski
+> --
+> To unsubscribe from this list: send the line "unsubscribe linux-leds"
+> in the body of a message to majordomo@vger.kernel.org More majordomo
+> info at  http://vger.kernel.org/majordomo-info.html
+>
+>
+>
+
+
+--
+Best regards,
+Jacek Anaszewski

Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x

[Jacek Anaszewski]

On 11/30/2016 10:10 AM, Phong Vo wrote:
> +-----Original Message-----
> +From: Jacek Anaszewski [mailto:j.anaszewski@samsung.com]
> +Sent: Wednesday, November 30, 2016 4:00 PM
> +To: Phong Vo
> +Cc: Mika Westerberg; Rafael J. Wysocki; Richard Purdie; linux-
> +leds@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> +acpi@vger.kernel.org; Loc Ho; Thang Nguyen; patches; Tin Huynh
> +Subject: Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x
> +
> +Hi Phong,
> +
> +On 11/30/2016 09:23 AM, Phong Vo wrote:
> +> +-----Original Message-----
> +> +From: Jacek Anaszewski [mailto:j.anaszewski@samsung.com]
> +> +Sent: Wednesday, November 30, 2016 3:18 PM
> +> +To: Tin Huynh
> +> +Cc: Mika Westerberg; Rafael J. Wysocki; Richard Purdie; linux-
> +> +leds@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> +> +acpi@vger.kernel.org; Loc Ho; Thang Nguyen; Phong Vo; patches
> +> +Subject: Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x
> +> +
> +> +On 11/30/2016 09:06 AM, Tin Huynh wrote:
> +> +> On Wed, Nov 30, 2016 at 3:01 PM, Jacek Anaszewski
> +> +> <j.anaszewski@samsung.com> wrote:
> +> +>>
> +> +>> On 11/30/2016 08:51 AM, Jacek Anaszewski wrote:
> +> +>>>
> +> +>>> Hi Tin,
> +> +>>>
> +> +>>> How this patch is different from the one already merged?
> +> +>>>
> +> +>>> Best regards,
> +> +>>> Jacek Anaszewski
> +> +>>>
> +>
> +> Hi Jacek, I am answering on behalf of Tin.
> +> This patch is for the leds:pca955x driver while the previous one was
> +> for leds:pca963x driver!
> +> They are almost the same in the coding construct, but different
> +drivers.
> +
> +Ah, indeed, that's why I got lost with patch version numbering :-)
> +
> +> +>>> On 11/30/2016 04:08 AM, Tin Huynh wrote:
> +> +>>>>
> +> +>>>> This patch enables ACPI support for leds-pca955x driver.
> +> +>>>>
> +> +>>>> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
> +> +>>>> ---
> +> +>>>>  drivers/leds/leds-pca955x.c |   22 +++++++++++++++++++++-
> +> +>>>>  1 files changed, 21 insertions(+), 1 deletions(-)
> +> +>>>>
> +> +>>>> Change from V2:
> +> +>>>>  -Correct coding conventions.
> +> +>>>>
> +> +>>>> Change from V1:
> +> +>>>>  -Remove CONFIG_ACPI.
> +> +>>>>
> +> +>>>> diff --git a/drivers/leds/leds-pca955x.c
> +> +>>>> b/drivers/leds/leds-pca955x.c index 840401a..b168ebe 100644
> +> +>>>> --- a/drivers/leds/leds-pca955x.c
> +> +>>>> +++ b/drivers/leds/leds-pca955x.c
> +> +>>>> @@ -40,6 +40,7 @@
> +> +>>>>   *  bits the chip supports.
> +> +>>>>   */
> +> +>>>>
> +> +>>>> +#include <linux/acpi.h>
> +> +>>>>  #include <linux/module.h>
> +> +>>>>  #include <linux/delay.h>
> +> +>>>>  #include <linux/string.h>
> +> +>>>> @@ -100,6 +101,15 @@ struct pca955x_chipdef {  };
> +> +>>>> MODULE_DEVICE_TABLE(i2c, pca955x_id);
> +> +>>>>
> +> +>>>> +static const struct acpi_device_id pca955x_acpi_ids[] = {
> +> +>>>> +    { .id = "PCA9550", .driver_data = pca9550 },
> +> +>>>> +    { .id = "PCA9551", .driver_data = pca9551 },
> +> +>>>> +    { .id = "PCA9552", .driver_data = pca9552 },
> +> +>>>> +    { .id = "PCA9553", .driver_data = pca9553 },
> +> +>>>> +    { }
> +> +>>
> +> +>>
> +> +>> OK, I see that you brought back explicit properties in the
> +> +>> structure initializer. Is there some vital reason for that?
> +>
> +> It's not vital, but to make it consistent with what was done for
> +> pca963x,
> +
> +For pca963x I applied the version without explicit properties.
> +Note that this is consistent with pca963x_id array above the added
> +pca963x_acpi_ids. For pca955x the situation is the same.
> +
> +> and also per suggestion by Mika on reviewing  a different driver
> +> mux:954x in another thread.
> +
> +Could you give a reference to that thread? In the review of V1 of this
> +patch Mika mentioned "{ "PCA9553", pca9553 }" scheme.
> +
>
> Actually it was Peter Rosin (not Mika) on linux-i2c and the reference to
> that is follows
>
> https://lkml.org/lkml/2016/11/18/732
>
> I am including Robin here.
>
> Thanks.

Thanks for the link. I prefer to stick to the style of the surrounding
code, so let's drop ".id =" and ".driver_data =" from the initializers.

Best regards,
Jacek Anaszewski

> +> I would think this would make the definition clearer.
> +>
> +> +>> You're mentioning "correcting coding conventions" in the patch
> +> +>> changelog. checkpatch.pl --strict doesn't complain about that, so
> +> +>> what coding conventions you have on mind?
> +> +>
> +> +>
> +> +>>
> +> +>>
> +> +>>>> +MODULE_DEVICE_TABLE(acpi, pca955x_acpi_ids);
> +> +>>>> +
> +> +>>>>  struct pca955x {
> +> +>>>>      struct mutex lock;
> +> +>>>>      struct pca955x_led *leds;
> +> +>>>> @@ -250,7 +260,16 @@ static int pca955x_probe(struct i2c_client
> +> +*client,
> +> +>>>>      struct led_platform_data *pdata;
> +> +>>>>      int i, err;
> +> +>>>>
> +> +>>>> -    chip = &pca955x_chipdefs[id->driver_data];
> +> +>>>> +    if (id) {
> +> +>>>> +        chip = &pca955x_chipdefs[id->driver_data];
> +> +>>>> +    } else {
> +> +>>>> +        const struct acpi_device_id *acpi_id;
> +> +>
> +> +> I added '{}' follow if
> +> +
> +> +You had it already in V1. Please verify if the patch applied to the
> +> +for- next branch of linux-leds.git has the shape you intended:
> +> +
> +> +https://git.kernel.org/cgit/linux/kernel/git/j.anaszewski/linux-
> +> +leds.git/commit/?h=for-next&id=e46895b71a26da404c4d95cb2bab1a67cf8b20
> +> +bc
> +> +
> +> +--
> +> +Best regards,
> +> +Jacek Anaszewski
> +> --
> +> To unsubscribe from this list: send the line "unsubscribe linux-leds"
> +> in the body of a message to majordomo@vger.kernel.org More majordomo
> +> info at  http://vger.kernel.org/majordomo-info.html
> +>
> +>
> +>
> +
> +
> +--
> +Best regards,
> +Jacek Anaszewski
>
>
>

Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x

[Peter Rosin]

On 2016-11-30 10:10, Phong Vo wrote:
> +-----Original Message-----
> +From: Jacek Anaszewski [mailto:j.anaszewski@samsung.com]
> +
> +Hi Phong,
> +
> +On 11/30/2016 09:23 AM, Phong Vo wrote:
> +> +-----Original Message-----
> +> +From: Jacek Anaszewski [mailto:j.anaszewski@samsung.com]
> +> +
> +> +On 11/30/2016 09:06 AM, Tin Huynh wrote:
> +> +> On Wed, Nov 30, 2016 at 3:01 PM, Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
> +> +>> On 11/30/2016 08:51 AM, Jacek Anaszewski wrote:
> +> +>>> On 11/30/2016 04:08 AM, Tin Huynh wrote:
> +> +>>>> +static const struct acpi_device_id pca955x_acpi_ids[] = {
> +> +>>>> +    { .id = "PCA9550", .driver_data = pca9550 },
> +> +>>>> +    { .id = "PCA9551", .driver_data = pca9551 },
> +> +>>>> +    { .id = "PCA9552", .driver_data = pca9552 },
> +> +>>>> +    { .id = "PCA9553", .driver_data = pca9553 },
> +> +>>>> +    { }
> +> +>>
> +> +>>
> +> +>> OK, I see that you brought back explicit properties in the
> +> +>> structure initializer. Is there some vital reason for that?
> +>
> +> It's not vital, but to make it consistent with what was done for
> +> pca963x,
> +
> +For pca963x I applied the version without explicit properties.
> +Note that this is consistent with pca963x_id array above the added
> +pca963x_acpi_ids. For pca955x the situation is the same.
> +
> +> and also per suggestion by Mika on reviewing  a different driver
> +> mux:954x in another thread.
> +
> +Could you give a reference to that thread? In the review of V1 of this
> +patch Mika mentioned "{ "PCA9553", pca9553 }" scheme.
> +
> 
> Actually it was Peter Rosin (not Mika) on linux-i2c and the reference to
> that is follows
> 
> https://lkml.org/lkml/2016/11/18/732
> 
> I am including Robin here.

I tried to say that I *personally* would have added the explicit
field specifiers but that it didn't seem like the norm and that both
of the approaches would therefore be perfectly ok by me (as there are
other examples of acpi id table initializers with field specifiers).

To sum up, I don't really care. But my question lingers, is there some
compelling reason to not have the explicit field specifiers?

Cheers,
Peter

Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x

[Jacek Anaszewski]

Hi Peter,

On 11/30/2016 10:36 AM, Peter Rosin wrote:
> On 2016-11-30 10:10, Phong Vo wrote:
>> +-----Original Message-----
>> +From: Jacek Anaszewski [mailto:j.anaszewski@samsung.com]
>> +
>> +Hi Phong,
>> +
>> +On 11/30/2016 09:23 AM, Phong Vo wrote:
>> +> +-----Original Message-----
>> +> +From: Jacek Anaszewski [mailto:j.anaszewski@samsung.com]
>> +> +
>> +> +On 11/30/2016 09:06 AM, Tin Huynh wrote:
>> +> +> On Wed, Nov 30, 2016 at 3:01 PM, Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
>> +> +>> On 11/30/2016 08:51 AM, Jacek Anaszewski wrote:
>> +> +>>> On 11/30/2016 04:08 AM, Tin Huynh wrote:
>> +> +>>>> +static const struct acpi_device_id pca955x_acpi_ids[] = {
>> +> +>>>> +    { .id = "PCA9550", .driver_data = pca9550 },
>> +> +>>>> +    { .id = "PCA9551", .driver_data = pca9551 },
>> +> +>>>> +    { .id = "PCA9552", .driver_data = pca9552 },
>> +> +>>>> +    { .id = "PCA9553", .driver_data = pca9553 },
>> +> +>>>> +    { }
>> +> +>>
>> +> +>>
>> +> +>> OK, I see that you brought back explicit properties in the
>> +> +>> structure initializer. Is there some vital reason for that?
>> +>
>> +> It's not vital, but to make it consistent with what was done for
>> +> pca963x,
>> +
>> +For pca963x I applied the version without explicit properties.
>> +Note that this is consistent with pca963x_id array above the added
>> +pca963x_acpi_ids. For pca955x the situation is the same.
>> +
>> +> and also per suggestion by Mika on reviewing  a different driver
>> +> mux:954x in another thread.
>> +
>> +Could you give a reference to that thread? In the review of V1 of this
>> +patch Mika mentioned "{ "PCA9553", pca9553 }" scheme.
>> +
>>
>> Actually it was Peter Rosin (not Mika) on linux-i2c and the reference to
>> that is follows
>>
>> https://lkml.org/lkml/2016/11/18/732
>>
>> I am including Robin here.
>
> I tried to say that I *personally* would have added the explicit
> field specifiers but that it didn't seem like the norm and that both
> of the approaches would therefore be perfectly ok by me (as there are
> other examples of acpi id table initializers with field specifiers).
>
> To sum up, I don't really care. But my question lingers, is there some
> compelling reason to not have the explicit field specifiers?

It certainly downgrades code readability, but in case there is
similar surrounding code there are two options to keep the things
consistent - either stick to the current style or change it.
IMHO the latter would generate only unnecessary noise in this
particular case.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH V3] leds: pca955x: Add ACPI support for pca955x

[Mika Westerberg]

On Wed, Nov 30, 2016 at 10:58:33AM +0100, Jacek Anaszewski wrote:
> It certainly downgrades code readability, but in case there is
> similar surrounding code there are two options to keep the things
> consistent - either stick to the current style or change it.
> IMHO the latter would generate only unnecessary noise in this
> particular case.

Also that's the style we have been using when adding ACPI support to
drivers.