[PATCH 0/2] gpio: pca9570: add slg7xl45106 support

[PATCH 0/2] gpio: pca9570: add slg7xl45106 support

[Shubhrajyoti Datta]

Add SLG7XL45106 GPO expander support

Shubhrajyoti Datta (2):
  dt-bindings: gpio: pca9570: Add compatible for slg7xl45106
  gpio: pca9570: add slg7xl45106 support

 .../devicetree/bindings/gpio/gpio-pca9570.yaml     |  1 +
 drivers/gpio/gpio-pca9570.c                        | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH 1/2] dt-bindings: gpio: pca9570: Add compatible for slg7xl45106

[Shubhrajyoti Datta]

This patch adds compatible string for the SLG7XL45106,
I2C GPO expander.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---
 Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml b/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
index 338c5312a106..503cfcb7f7c9 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
+++ b/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
@@ -13,6 +13,7 @@ properties:
   compatible:
     enum:
       - nxp,pca9570
+      - dlg,slg7xl45106
 
   reg:
     maxItems: 1
-- 
2.17.1

Re: [PATCH 1/2] dt-bindings: gpio: pca9570: Add compatible for slg7xl45106

[Krzysztof Kozlowski]

On 17/08/2022 11:55, Shubhrajyoti Datta wrote:
> This patch adds compatible string for the SLG7XL45106,
> I2C GPO expander.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
>  Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml b/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
> index 338c5312a106..503cfcb7f7c9 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
> +++ b/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
> @@ -13,6 +13,7 @@ properties:
>    compatible:
>      enum:
>        - nxp,pca9570
> +      - dlg,slg7xl45106

First, this does not match tree, please rebase on some new Linux kernel.
Second, put them in alphabetical order.
Third, these are different manufacturers. Why do you think devices are
compatible?

Best regards,
Krzysztof

RE: [PATCH 1/2] dt-bindings: gpio: pca9570: Add compatible for slg7xl45106

[Datta, Shubhrajyoti]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Thursday, August 18, 2022 2:14 PM
> To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>; linux-
> gpio@vger.kernel.org
> Cc: git-dev (AMD-Xilinx) <git-dev@amd.com>; mans0n@gorani.run;
> devicetree@vger.kernel.org; krzysztof.kozlowski+dt@linaro.org;
> robh+dt@kernel.org; linus.walleij@linaro.org; brgl@bgdev.pl;
> shubhrajyoti.datta@gmail.com
> Subject: Re: [PATCH 1/2] dt-bindings: gpio: pca9570: Add compatible for
> slg7xl45106
> 
> [CAUTION: External Email]
> 
> On 17/08/2022 11:55, Shubhrajyoti Datta wrote:
> > This patch adds compatible string for the SLG7XL45106, I2C GPO
> > expander.
> >
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> > ---
> >  Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
> > b/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
> > index 338c5312a106..503cfcb7f7c9 100644
> > --- a/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
> > @@ -13,6 +13,7 @@ properties:
> >    compatible:
> >      enum:
> >        - nxp,pca9570
> > +      - dlg,slg7xl45106
> 
> First, this does not match tree, please rebase on some new Linux kernel.
> Second, put them in alphabetical order.
> Third, these are different manufacturers. Why do you think devices are
> compatible?

Will rebase and resend.
There was a earlier discussion
https://lore.kernel.org/all/CAHp75Ve_mgam2jcyFG-NggziUScK3JBZ4fmtN+rjd+Vra=ixuw@mail.gmail.com/T/#me3fb70c782159b1c2aed9cc33d6eb4d31193e56e

Based on which I considered the pca.

> 
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: gpio: pca9570: Add compatible for slg7xl45106

[Krzysztof Kozlowski]

On 18/08/2022 12:30, Datta, Shubhrajyoti wrote:
> [AMD Official Use Only - General]
> 
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Thursday, August 18, 2022 2:14 PM
>> To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>; linux-
>> gpio@vger.kernel.org
>> Cc: git-dev (AMD-Xilinx) <git-dev@amd.com>; mans0n@gorani.run;
>> devicetree@vger.kernel.org; krzysztof.kozlowski+dt@linaro.org;
>> robh+dt@kernel.org; linus.walleij@linaro.org; brgl@bgdev.pl;
>> shubhrajyoti.datta@gmail.com
>> Subject: Re: [PATCH 1/2] dt-bindings: gpio: pca9570: Add compatible for
>> slg7xl45106
>>
>> [CAUTION: External Email]
>>
>> On 17/08/2022 11:55, Shubhrajyoti Datta wrote:
>>> This patch adds compatible string for the SLG7XL45106, I2C GPO
>>> expander.
>>>
>>> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
>>> ---
>>>  Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
>>> b/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
>>> index 338c5312a106..503cfcb7f7c9 100644
>>> --- a/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
>>> @@ -13,6 +13,7 @@ properties:
>>>    compatible:
>>>      enum:
>>>        - nxp,pca9570
>>> +      - dlg,slg7xl45106
>>
>> First, this does not match tree, please rebase on some new Linux kernel.
>> Second, put them in alphabetical order.
>> Third, these are different manufacturers. Why do you think devices are
>> compatible?
> 
> Will rebase and resend.
> There was a earlier discussion
> https://lore.kernel.org/all/CAHp75Ve_mgam2jcyFG-NggziUScK3JBZ4fmtN+rjd+Vra=ixuw@mail.gmail.com/T/#me3fb70c782159b1c2aed9cc33d6eb4d31193e56e
> 
> Based on which I considered the pca.

But none of these are explained here in commit msg...

BTW, your previous binding was not even sent to DT mailing list and DT
maintainers. Always use scripts/get_maintainers.pl.

Best regards,
Krzysztof

[PATCH 2/2] gpio: pca9570: add slg7xl45106 support

[Shubhrajyoti Datta]

slg7xl45106 is a I2C GPO expander.
Add a compatible string for the same. Also update the
driver to write and read from it.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---
 drivers/gpio/gpio-pca9570.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-pca9570.c b/drivers/gpio/gpio-pca9570.c
index 2735cd6addcf..b4e422b25ea6 100644
--- a/drivers/gpio/gpio-pca9570.c
+++ b/drivers/gpio/gpio-pca9570.c
@@ -15,6 +15,8 @@
 #include <linux/mutex.h>
 #include <linux/property.h>
 
+#define SLG7XL45106_GPO_REG	0xDB
+
 /**
  * struct pca9570 - GPIO driver data
  * @chip: GPIO controller chip
@@ -25,6 +27,7 @@ struct pca9570 {
 	struct gpio_chip chip;
 	struct mutex lock;
 	u8 out;
+	u8 command;
 };
 
 static int pca9570_read(struct pca9570 *gpio, u8 *value)
@@ -32,7 +35,7 @@ static int pca9570_read(struct pca9570 *gpio, u8 *value)
 	struct i2c_client *client = to_i2c_client(gpio->chip.parent);
 	int ret;
 
-	ret = i2c_smbus_read_byte(client);
+	ret = i2c_smbus_read_byte_data(client, gpio->command);
 	if (ret < 0)
 		return ret;
 
@@ -44,6 +47,9 @@ static int pca9570_write(struct pca9570 *gpio, u8 value)
 {
 	struct i2c_client *client = to_i2c_client(gpio->chip.parent);
 
+	if (gpio->command)
+		return i2c_smbus_write_byte_data(client, gpio->command, value);
+
 	return i2c_smbus_write_byte(client, value);
 }
 
@@ -94,6 +100,7 @@ static void pca9570_set(struct gpio_chip *chip, unsigned int offset, int value)
 static int pca9570_probe(struct i2c_client *client)
 {
 	struct pca9570 *gpio;
+	struct device_node *np = client->dev.of_node;
 
 	gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
 	if (!gpio)
@@ -109,6 +116,9 @@ static int pca9570_probe(struct i2c_client *client)
 	gpio->chip.ngpio = (uintptr_t)device_get_match_data(&client->dev);
 	gpio->chip.can_sleep = true;
 
+	if (of_device_is_compatible(np, "dlg,slg7xl45106"))
+		gpio->command = SLG7XL45106_GPO_REG;
+
 	mutex_init(&gpio->lock);
 
 	/* Read the current output level */
@@ -121,12 +131,14 @@ static int pca9570_probe(struct i2c_client *client)
 
 static const struct i2c_device_id pca9570_id_table[] = {
 	{ "pca9570", 4 },
+	{ "slg7xl45106", 8 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, pca9570_id_table);
 
 static const struct of_device_id pca9570_of_match_table[] = {
 	{ .compatible = "nxp,pca9570", .data = (void *)4 },
+	{ .compatible = "dlg,slg7xl45106", .data = (void *)8 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, pca9570_of_match_table);
-- 
2.17.1

Re: [PATCH 2/2] gpio: pca9570: add slg7xl45106 support

[Krzysztof Kozlowski]

On 17/08/2022 11:55, Shubhrajyoti Datta wrote:
> slg7xl45106 is a I2C GPO expander.
> Add a compatible string for the same. Also update the
> driver to write and read from it.

Please fix your wrapping to match coding style.

> 

>  
> @@ -94,6 +100,7 @@ static void pca9570_set(struct gpio_chip *chip, unsigned int offset, int value)
>  static int pca9570_probe(struct i2c_client *client)
>  {
>  	struct pca9570 *gpio;
> +	struct device_node *np = client->dev.of_node;
>  
>  	gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
>  	if (!gpio)
> @@ -109,6 +116,9 @@ static int pca9570_probe(struct i2c_client *client)
>  	gpio->chip.ngpio = (uintptr_t)device_get_match_data(&client->dev);
>  	gpio->chip.can_sleep = true;
>  
> +	if (of_device_is_compatible(np, "dlg,slg7xl45106"))

Matching is done once and you have driver data for this. No compatibles
in the code, this does not scale.

> +		gpio->command = SLG7XL45106_GPO_REG;
> +
>  	mutex_init(&gpio->lock);
>  
>  	/* Read the current output level */
> @@ -121,12 +131,14 @@ static int pca9570_probe(struct i2c_client *client)
>  
>  static const struct i2c_device_id pca9570_id_table[] = {
>  	{ "pca9570", 4 },
> +	{ "slg7xl45106", 8 },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(i2c, pca9570_id_table);
>  
>  static const struct of_device_id pca9570_of_match_table[] = {
>  	{ .compatible = "nxp,pca9570", .data = (void *)4 },

This is some old tree, isn't it?

> +	{ .compatible = "dlg,slg7xl45106", .data = (void *)8 },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, pca9570_of_match_table);


Best regards,
Krzysztof

Re: [PATCH 2/2] gpio: pca9570: add slg7xl45106 support

[Andy Shevchenko]

On Wed, Aug 17, 2022 at 12:10 PM Shubhrajyoti Datta
<shubhrajyoti.datta@amd.com> wrote:
>
> slg7xl45106 is a I2C GPO expander.
> Add a compatible string for the same. Also update the
> driver to write and read from it.

...

> +       struct device_node *np = client->dev.of_node;

> +       if (of_device_is_compatible(np, "dlg,slg7xl45106"))
> +               gpio->command = SLG7XL45106_GPO_REG;

Please, avoid making the driver OF dependent when there is no reason
to do that. Note, we have device property and fwnode APIs that may
cover a lot of cases for different property providers.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] gpio: pca9570: add slg7xl45106 support

[Krzysztof Kozlowski]

On 17/08/2022 11:55, Shubhrajyoti Datta wrote:
> Add SLG7XL45106 GPO expander support
> 

Please do not Cc fake/non-existing addresses in your submissions:

Your message to svemula-all-with-npw@amd.com couldn't be delivered.
The group svemula-all-with-npw only accepts messages from people in its
organization or on its allowed senders list, and your email address
isn't on the list.
krzysztof.kozlowski     Office 365      svemula-all-with-npw
Sender          Action Required
Sender not allowed


Best regards,
Krzysztof