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

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

[Shubhrajyoti Datta]

Add SLG7XL45106 GPO expander
 
v2:
Use the platform data check instead of compatible
arrange alphabetically
rebase to the latest kernel


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

 .../bindings/gpio/gpio-pca9570.yaml           |  1 +
 drivers/gpio/gpio-pca9570.c                   | 39 +++++++++++++++++--
 2 files changed, 36 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH v2 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>
---
v2:
add alphabetically

 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 1acaa0a3d35a..48bf414aa50e 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
+++ b/Documentation/devicetree/bindings/gpio/gpio-pca9570.yaml
@@ -12,6 +12,7 @@ maintainers:
 properties:
   compatible:
     enum:
+      - dlg,slg7xl45106
       - nxp,pca9570
       - nxp,pca9571
 
-- 
2.17.1

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

[Krzysztof Kozlowski]

On 15/09/2022 12:48, Shubhrajyoti Datta wrote:
> This patch adds compatible string for the SLG7XL45106,
> I2C GPO expander.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

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

[Linus Walleij]

On Thu, Sep 15, 2022 at 1:48 PM Shubhrajyoti Datta
<shubhrajyoti.datta@amd.com> wrote:

> This patch adds compatible string for the SLG7XL45106,
> I2C GPO expander.
>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 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>
---
v2:
Use platform data insted of compatible

 drivers/gpio/gpio-pca9570.c | 39 +++++++++++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-pca9570.c b/drivers/gpio/gpio-pca9570.c
index ab2a652964ec..4f255347a62d 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,12 @@ struct pca9570 {
 	struct gpio_chip chip;
 	struct mutex lock;
 	u8 out;
+	const struct pca9570_platform_data *p_data;
+};
+
+struct pca9570_platform_data {
+	u16 ngpio;
+	u32 command;
 };
 
 static int pca9570_read(struct pca9570 *gpio, u8 *value)
@@ -32,7 +40,11 @@ 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);
+	if (gpio->p_data->command != 0)
+		ret = i2c_smbus_read_byte_data(client, gpio->p_data->command);
+	else
+		ret = i2c_smbus_read_byte(client);
+
 	if (ret < 0)
 		return ret;
 
@@ -44,6 +56,9 @@ static int pca9570_write(struct pca9570 *gpio, u8 value)
 {
 	struct i2c_client *client = to_i2c_client(gpio->chip.parent);
 
+	if (gpio->p_data->command != 0)
+		return i2c_smbus_write_byte_data(client, gpio->p_data->command, value);
+
 	return i2c_smbus_write_byte(client, value);
 }
 
@@ -106,7 +121,8 @@ static int pca9570_probe(struct i2c_client *client)
 	gpio->chip.get = pca9570_get;
 	gpio->chip.set = pca9570_set;
 	gpio->chip.base = -1;
-	gpio->chip.ngpio = (uintptr_t)device_get_match_data(&client->dev);
+	gpio->p_data = device_get_match_data(&client->dev);
+	gpio->chip.ngpio = gpio->p_data->ngpio;
 	gpio->chip.can_sleep = true;
 
 	mutex_init(&gpio->lock);
@@ -122,13 +138,28 @@ static int pca9570_probe(struct i2c_client *client)
 static const struct i2c_device_id pca9570_id_table[] = {
 	{ "pca9570", 4 },
 	{ "pca9571", 8 },
+	{ "slg7xl45106", 8 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, pca9570_id_table);
 
+static const struct pca9570_platform_data pca9570_gpio = {
+	.ngpio = 4,
+};
+
+static const struct pca9570_platform_data pca9571_gpio = {
+	.ngpio = 8,
+};
+
+static const struct pca9570_platform_data slg7xl45106_gpio = {
+	.ngpio = 8,
+	.command = SLG7XL45106_GPO_REG,
+};
+
 static const struct of_device_id pca9570_of_match_table[] = {
-	{ .compatible = "nxp,pca9570", .data = (void *)4 },
-	{ .compatible = "nxp,pca9571", .data = (void *)8 },
+	{ .compatible = "dlg,slg7xl45106", .data = &slg7xl45106_gpio},
+	{ .compatible = "nxp,pca9570", .data = &pca9570_gpio },
+	{ .compatible = "nxp,pca9571", .data = &pca9571_gpio },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, pca9570_of_match_table);
-- 
2.17.1

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

[andy.shevchenko]

Thu, Sep 15, 2022 at 05:18:03PM +0530, Shubhrajyoti Datta kirjoitti:
> slg7xl45106 is a I2C GPO expander.
> Add a compatible string for the same. Also update the
> driver to write and read from it.

It's better, but something to improve.

...

>  /**
>   * struct pca9570 - GPIO driver data
>   * @chip: GPIO controller chip
> @@ -25,6 +27,12 @@ struct pca9570 {
>  	struct gpio_chip chip;
>  	struct mutex lock;
>  	u8 out;
> +	const struct pca9570_platform_data *p_data;

I would put it after 'chip' member, so it will save 4 bytes on 64-bit machines
of some architectures. Also, don't you need to add a kernel doc for a new
member?

> +};

> +struct pca9570_platform_data {
> +	u16 ngpio;
> +	u32 command;
>  };

Strictly speaking this should be defined before struct pca9570, otherwise you
need to have a forward declaration.

> @@ -122,13 +138,28 @@ static int pca9570_probe(struct i2c_client *client)
>  static const struct i2c_device_id pca9570_id_table[] = {
>  	{ "pca9570", 4 },
>  	{ "pca9571", 8 },
> +	{ "slg7xl45106", 8 },
>  	{ /* sentinel */ }

This table should also use your new structure:

	{ "slg7xl45106", (kernel_ulong_t)&slg7xl45106_gpio },

(In the similar way for the existing entries)

Taking the last into consideration I would suggest to split this to two
changes:
1) introducing a new data structure with conversion of the existing members;
2) adding support for a new chip.

>  };

Othewise looks good.

-- 
With Best Regards,
Andy Shevchenko

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

[Sungbo Eo]

Hi,

Thanks for the update.
I was thinking I should reply to your patch in the last month, but I was
a little busy at the time and I forgot to do so...

On 2022-09-15 20:48, 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.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
> v2:
> Use platform data insted of compatible

Moving the command property into the new platform structure is nice.
And please add more description about the device in the commit message.
We don't even know the full name of the vendor from your patch.
I like the older version of your patch in that perspective.
https://lore.kernel.org/all/1656426829-1008-3-git-send-email-shubhrajyoti.datta@xilinx.com/
And a link to the device datasheet would be also nice (if possible).

> 
>  drivers/gpio/gpio-pca9570.c | 39 +++++++++++++++++++++++++++++++++----
>  1 file changed, 35 insertions(+), 4 deletions(-)

And I was also thinking that tpic2810 driver might be more appropriate
then this pca9570 driver for a device with one command byte.
Actually I had forked tpic2810 to create pca9570 to support a device
without any command byte.
Come to think of it, the two drivers may even be consolidated into a
single generic one... What do you think?

Thanks,
Sungbo

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

[Datta, Shubhrajyoti]

[AMD Official Use Only - General]

Hi Sunbo,

> -----Original Message-----
> From: Sungbo Eo <mans0n@gorani.run>
> Sent: Saturday, September 17, 2022 7:31 PM
> To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>
> Cc: linux-gpio@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>;
> devicetree@vger.kernel.org; krzysztof.kozlowski+dt@linaro.org;
> robh+dt@kernel.org; brgl@bgdev.pl; linus.walleij@linaro.org; Andy
> Shevchenko <andy.shevchenko@gmail.com>
> Subject: Re: [PATCH v2 2/2] gpio: pca9570: add slg7xl45106 support
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> Hi,
> 
> Thanks for the update.
> I was thinking I should reply to your patch in the last month, but I was a little
> busy at the time and I forgot to do so...
> 
> On 2022-09-15 20:48, 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.
> >
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> > ---
> > v2:
> > Use platform data insted of compatible
> 
> Moving the command property into the new platform structure is nice.
> And please add more description about the device in the commit message.
> We don't even know the full name of the vendor from your patch.
> I like the older version of your patch in that perspective.
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Fall%2F1656426829-1008-3-git-send-email-
> shubhrajyoti.datta%40xilinx.com%2F&amp;data=05%7C01%7Cshubhrajyoti.d
> atta%40amd.com%7C9758241b75fc461113b608da98b50869%7C3dd8961fe488
> 4e608e11a82d994e183d%7C0%7C0%7C637990201003357055%7CUnknown%7
> CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi
> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=x0RHFhr9X0L3VBzTRyRy
> VfLhm74gx7jBqUs2NEFhKcI%3D&amp;reserved=0
> And a link to the device datasheet would be also nice (if possible).
> 

Will update the description.

> >
> >  drivers/gpio/gpio-pca9570.c | 39
> > +++++++++++++++++++++++++++++++++----
> >  1 file changed, 35 insertions(+), 4 deletions(-)
> 
> And I was also thinking that tpic2810 driver might be more appropriate then
> this pca9570 driver for a device with one command byte.
> Actually I had forked tpic2810 to create pca9570 to support a device without
> any command byte.
> Come to think of it, the two drivers may even be consolidated into a single
> generic one... What do you think?

I agree.
It looks to me that the current driver should work for the tpic2810 also by adding the compatible.
Do you agree?


> 
> Thanks,
> Sungbo

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

[Michal Simek]

Hi,

On 9/19/22 08:32, Datta, Shubhrajyoti wrote:
> [AMD Official Use Only - General]
> 
> Hi Sunbo,
> 
>> -----Original Message-----
>> From: Sungbo Eo <mans0n@gorani.run>
>> Sent: Saturday, September 17, 2022 7:31 PM
>> To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>
>> Cc: linux-gpio@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>;
>> devicetree@vger.kernel.org; krzysztof.kozlowski+dt@linaro.org;
>> robh+dt@kernel.org; brgl@bgdev.pl; linus.walleij@linaro.org; Andy
>> Shevchenko <andy.shevchenko@gmail.com>
>> Subject: Re: [PATCH v2 2/2] gpio: pca9570: add slg7xl45106 support
>>
>> Caution: This message originated from an External Source. Use proper
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> Hi,
>>
>> Thanks for the update.
>> I was thinking I should reply to your patch in the last month, but I was a little
>> busy at the time and I forgot to do so...
>>
>> On 2022-09-15 20:48, 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.
>>>
>>> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
>>> ---
>>> v2:
>>> Use platform data insted of compatible
>>
>> Moving the command property into the new platform structure is nice.
>> And please add more description about the device in the commit message.
>> We don't even know the full name of the vendor from your patch.
>> I like the older version of your patch in that perspective.
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
>> kernel.org%2Fall%2F1656426829-1008-3-git-send-email-
>> shubhrajyoti.datta%40xilinx.com%2F&amp;data=05%7C01%7Cshubhrajyoti.d
>> atta%40amd.com%7C9758241b75fc461113b608da98b50869%7C3dd8961fe488
>> 4e608e11a82d994e183d%7C0%7C0%7C637990201003357055%7CUnknown%7
>> CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi
>> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=x0RHFhr9X0L3VBzTRyRy
>> VfLhm74gx7jBqUs2NEFhKcI%3D&amp;reserved=0
>> And a link to the device datasheet would be also nice (if possible).
>>
> 
> Will update the description.
> 
>>>
>>>   drivers/gpio/gpio-pca9570.c | 39
>>> +++++++++++++++++++++++++++++++++----
>>>   1 file changed, 35 insertions(+), 4 deletions(-)
>>
>> And I was also thinking that tpic2810 driver might be more appropriate then
>> this pca9570 driver for a device with one command byte.
>> Actually I had forked tpic2810 to create pca9570 to support a device without
>> any command byte.
>> Come to think of it, the two drivers may even be consolidated into a single
>> generic one... What do you think?
> 
> I agree.
> It looks to me that the current driver should work for the tpic2810 also by adding the compatible.
> Do you agree?

You will have to solve issue with Kconfig symbols. Anyway if you want to merge 
these two drivers together it has to be done on the top of this series anyway. 
It means get this first part merged and then another device can be on the top of 
this series.

Thanks,
Michal