[PATCH 0/2] gpio: Add support for SLG7XL45106 I2C GPO expander

[PATCH 0/2] gpio: Add support for SLG7XL45106 I2C GPO expander

[Shubhrajyoti Datta]

Add GPO expander support

Resending the series as i see this series didn't went out due to 
some issue with my mail client. Please ignore if this series is 
already received


Raviteja Narayanam (1):
  gpio: Add support for SLG7XL45106 I2C GPO expander

Shubhrajyoti Datta (1):
  dt-bindings: gpio: Add gpio-slg7xl45106.yaml

 .../bindings/gpio/gpio-slg7xl45106.yaml       |  47 +++++++
 MAINTAINERS                                   |   7 +
 drivers/gpio/Kconfig                          |   9 ++
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-slg7xl45106.c               | 133 ++++++++++++++++++
 5 files changed, 197 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-slg7xl45106.yaml
 create mode 100644 drivers/gpio/gpio-slg7xl45106.c

-- 
2.17.1

[PATCH 1/2] dt-bindings: gpio: Add gpio-slg7xl45106.yaml

[Shubhrajyoti Datta]

Add a binding for Dialog Semiconductors slg7xl45106 I2C GPO expander

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
 .../bindings/gpio/gpio-slg7xl45106.yaml       | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-slg7xl45106.yaml

diff --git a/Documentation/devicetree/bindings/gpio/gpio-slg7xl45106.yaml b/Documentation/devicetree/bindings/gpio/gpio-slg7xl45106.yaml
new file mode 100644
index 000000000000..7551ea5c2ef7
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-slg7xl45106.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-slg7xl45106.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: slg7xl45106 I2C GPO expander
+
+maintainers:
+  - Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
+
+properties:
+  compatible:
+    enum:
+      - dlg,slg7xl45106
+
+  reg:
+    maxItems: 1
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        gpio@10 {
+            compatible = "dlg,slg7xl45106";
+            reg = <0x10>;
+            gpio-controller;
+            #gpio-cells = <2>;
+        };
+    };
+
+...
-- 
2.17.1

[PATCH 2/2] gpio: Add support for SLG7XL45106 I2C GPO expander

[Shubhrajyoti Datta]

From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

Dialog semiconductors SLG7XL45106 is an 8-bit I2C GPO expander.
The output port is controlled by a data byte with register
address.

Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
 MAINTAINERS                     |   7 ++
 drivers/gpio/Kconfig            |   9 +++
 drivers/gpio/Makefile           |   1 +
 drivers/gpio/gpio-slg7xl45106.c | 133 ++++++++++++++++++++++++++++++++
 4 files changed, 150 insertions(+)
 create mode 100644 drivers/gpio/gpio-slg7xl45106.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 1fc9ead83d2a..3b3f322b5012 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5875,6 +5875,13 @@ F:	include/linux/regulator/da9211.h
 F:	include/sound/da[79]*.h
 F:	sound/soc/codecs/da[79]*.[ch]
 
+DIALOG SLG7XL45106 GPO DRIVER
+M:	Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
+L:	linux-gpio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/gpio/gpio-slg7xl45106.yaml
+F:	drivers/gpio/gpio-slg7xl45106.c
+
 DIAMOND SYSTEMS GPIO-MM GPIO DRIVER
 M:	William Breathitt Gray <vilhelm.gray@gmail.com>
 L:	linux-gpio@vger.kernel.org
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b01961999ced..1e10f96c9c09 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1101,6 +1101,15 @@ config GPIO_PCF857X
 	  This driver provides an in-kernel interface to those GPIOs using
 	  platform-neutral GPIO calls.
 
+config GPIO_SLG7XL45106
+	tristate "SLG7XL45106 8-Bit I2C GPO expander"
+	help
+	  Say yes here to enable the GPO driver for the Dialog SLG7XL45106 chip.
+	  This expander has 8 output pins.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gpio-slg7xl45106.
+
 config GPIO_TPIC2810
 	tristate "TPIC2810 8-Bit I2C GPO expander"
 	help
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 14352f6dfe8e..8248e4fcc22f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -136,6 +136,7 @@ obj-$(CONFIG_GPIO_SIFIVE)		+= gpio-sifive.o
 obj-$(CONFIG_GPIO_SIM)			+= gpio-sim.o
 obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
 obj-$(CONFIG_GPIO_SL28CPLD)		+= gpio-sl28cpld.o
+obj-$(CONFIG_GPIO_SLG7XL45106)		+= gpio-slg7xl45106.o
 obj-$(CONFIG_GPIO_SODAVILLE)		+= gpio-sodaville.o
 obj-$(CONFIG_GPIO_SPEAR_SPICS)		+= gpio-spear-spics.o
 obj-$(CONFIG_GPIO_SPRD)			+= gpio-sprd.o
diff --git a/drivers/gpio/gpio-slg7xl45106.c b/drivers/gpio/gpio-slg7xl45106.c
new file mode 100644
index 000000000000..61935b8538d1
--- /dev/null
+++ b/drivers/gpio/gpio-slg7xl45106.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for slg7xl45106 I2C GPO expander
+ *
+ * Copyright (C) 2021 Xilinx, Inc.
+ * Copyright (C) 2022 AMD, Inc.
+ *
+ * Based on gpio-pca9570.c
+ * Copyright (C) 2020 Sungbo Eo <mans0n@gorani.run>
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+
+#define SLG7XL45106_GPO_REG	0xDB
+
+/**
+ * struct slg7xl45106 - GPIO driver data
+ * @chip: GPIO controller chip
+ * @lock: Protects write sequences
+ */
+struct slg7xl45106 {
+	struct gpio_chip chip;
+	struct mutex lock;	/* To protect writes */
+};
+
+static int slg7xl45106_read(struct slg7xl45106 *gpio)
+{
+	struct i2c_client *client = to_i2c_client(gpio->chip.parent);
+
+	return i2c_smbus_read_byte_data(client, SLG7XL45106_GPO_REG);
+}
+
+static int slg7xl45106_write(struct slg7xl45106 *gpio, u8 value)
+{
+	struct i2c_client *client = to_i2c_client(gpio->chip.parent);
+
+	return i2c_smbus_write_byte_data(client, SLG7XL45106_GPO_REG, value);
+}
+
+static int slg7xl45106_get_direction(struct gpio_chip *chip,
+				     unsigned int offset)
+{
+	/* This device always output */
+	return GPIO_LINE_DIRECTION_OUT;
+}
+
+static int slg7xl45106_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct slg7xl45106 *gpio = gpiochip_get_data(chip);
+	int ret;
+
+	ret = slg7xl45106_read(gpio);
+	if (ret < 0)
+		return ret;
+
+	return !!(ret & BIT(offset));
+}
+
+static void slg7xl45106_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct slg7xl45106 *gpio = gpiochip_get_data(chip);
+	u8 buffer;
+
+	mutex_lock(&gpio->lock);
+
+	buffer = slg7xl45106_read(gpio);
+	if (buffer < 0)
+		goto out;
+
+	if (value)
+		buffer |= BIT(offset);
+	else
+		buffer &= ~BIT(offset);
+
+	slg7xl45106_write(gpio, buffer);
+
+out:
+	mutex_unlock(&gpio->lock);
+}
+
+static int slg7xl45106_probe(struct i2c_client *client)
+{
+	struct slg7xl45106 *gpio;
+
+	gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	gpio->chip.label = client->name;
+	gpio->chip.parent = &client->dev;
+	gpio->chip.owner = THIS_MODULE;
+	gpio->chip.get_direction = slg7xl45106_get_direction;
+	gpio->chip.get = slg7xl45106_get;
+	gpio->chip.set = slg7xl45106_set;
+	gpio->chip.base = -1;
+	gpio->chip.ngpio = (uintptr_t)device_get_match_data(&client->dev);
+	gpio->chip.can_sleep = true;
+
+	mutex_init(&gpio->lock);
+
+	i2c_set_clientdata(client, gpio);
+
+	return devm_gpiochip_add_data(&client->dev, &gpio->chip, gpio);
+}
+
+static const struct i2c_device_id slg7xl45106_id_table[] = {
+	{ "slg7xl45106", 8 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, slg7xl45106_id_table);
+
+static const struct of_device_id slg7xl45106_of_match_table[] = {
+	{ .compatible = "dlg,slg7xl45106", .data = (void *)8 },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, slg7xl45106_of_match_table);
+
+static struct i2c_driver slg7xl45106_driver = {
+	.driver = {
+		.name = "slg7xl45106",
+		.of_match_table = slg7xl45106_of_match_table,
+	},
+	.probe_new = slg7xl45106_probe,
+	.id_table = slg7xl45106_id_table,
+};
+module_i2c_driver(slg7xl45106_driver);
+
+MODULE_AUTHOR("Raviteja Narayanam <raviteja.narayanam@xilinx.com>");
+MODULE_DESCRIPTION("GPIO expander driver for slg7xl45106");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH 2/2] gpio: Add support for SLG7XL45106 I2C GPO expander

[Andy Shevchenko]

On Tue, Jun 28, 2022 at 4:35 PM Shubhrajyoti Datta
<shubhrajyoti.datta@xilinx.com> wrote:
>
> From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
>
> Dialog semiconductors SLG7XL45106 is an 8-bit I2C GPO expander.
> The output port is controlled by a data byte with register
> address.

1/ Have you checked if there is a driver that sounds very similar to
this already upstream?
2/ Have you tried to use gpio-regmap? Why can it not be used?

...

> +struct slg7xl45106 {
> +       struct gpio_chip chip;

> +       struct mutex lock;      /* To protect writes */

If you switch to regmap I believe this entire struct will be gone completely.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: Add support for SLG7XL45106 I2C GPO expander

[Andy Shevchenko]

On Tue, Jun 28, 2022 at 9:13 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Jun 28, 2022 at 4:35 PM Shubhrajyoti Datta
> <shubhrajyoti.datta@xilinx.com> wrote:
> >
> > From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
> >
> > Dialog semiconductors SLG7XL45106 is an 8-bit I2C GPO expander.
> > The output port is controlled by a data byte with register
> > address.
>
> 1/ Have you checked if there is a driver that sounds very similar to
> this already upstream?

Actually, why can't pca9570 be amended to support this?

> 2/ Have you tried to use gpio-regmap? Why can it not be used?

Same Q for PCA9570 driver. (Maybe gpio-regmap wasn't existed that time?)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: Add support for SLG7XL45106 I2C GPO expander

[Sungbo Eo]

Hi,

On 2022-06-29 04:21, Andy Shevchenko wrote:
> On Tue, Jun 28, 2022 at 9:13 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>>
>> On Tue, Jun 28, 2022 at 4:35 PM Shubhrajyoti Datta
>> <shubhrajyoti.datta@xilinx.com> wrote:
>>>
>>> From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
>>>
>>> Dialog semiconductors SLG7XL45106 is an 8-bit I2C GPO expander.
>>> The output port is controlled by a data byte with register
>>> address.
>>
>> 1/ Have you checked if there is a driver that sounds very similar to
>> this already upstream?
> 
> Actually, why can't pca9570 be amended to support this?
> 
>> 2/ Have you tried to use gpio-regmap? Why can it not be used?
> 
> Same Q for PCA9570 driver. (Maybe gpio-regmap wasn't existed that time?)

IIRC regmap could not be used for pca9570 as pca9570 doesn't have a
concept of "address"; it only accepts a data value.[1]
Please let me know if the situation has changed in the meantime.

It seems the slg7xl45106 driver reads/writes a reg at 0xDB so it is not
compatible with pca9570 driver (in the current state), and (I suppose)
it could be converted to use gpio-regmap.

[1]
https://lore.kernel.org/linux-gpio/69f5d1a1970838b8c4bd8d6e8dba6cac@walle.cc/

Thanks,
Sungbo

Re: [PATCH 2/2] gpio: Add support for SLG7XL45106 I2C GPO expander

[Michal Simek]

Hi,

On 6/29/22 03:00, Sungbo Eo wrote:
> Hi,
> 
> On 2022-06-29 04:21, Andy Shevchenko wrote:
>> On Tue, Jun 28, 2022 at 9:13 PM Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>>
>>> On Tue, Jun 28, 2022 at 4:35 PM Shubhrajyoti Datta
>>> <shubhrajyoti.datta@xilinx.com> wrote:
>>>>
>>>> From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
>>>>
>>>> Dialog semiconductors SLG7XL45106 is an 8-bit I2C GPO expander.
>>>> The output port is controlled by a data byte with register
>>>> address.
>>>
>>> 1/ Have you checked if there is a driver that sounds very similar to
>>> this already upstream?
>>
>> Actually, why can't pca9570 be amended to support this?
>>
>>> 2/ Have you tried to use gpio-regmap? Why can it not be used?
>>
>> Same Q for PCA9570 driver. (Maybe gpio-regmap wasn't existed that time?)
> 
> IIRC regmap could not be used for pca9570 as pca9570 doesn't have a
> concept of "address"; it only accepts a data value.[1]
> Please let me know if the situation has changed in the meantime.
> 
> It seems the slg7xl45106 driver reads/writes a reg at 0xDB so it is not
> compatible with pca9570 driver (in the current state), and (I suppose)
> it could be converted to use gpio-regmap.
> 
> [1]
> https://lore.kernel.org/linux-gpio/69f5d1a1970838b8c4bd8d6e8dba6cac@walle.cc/

As was mentioned driver is based on pca9570 and the only important difference is 
with i2c_smbus_read_byte/i2c_smbus_read_byte_data and especially
i2c_smbus_write_byte/i2c_smbus_write_byte_data.

Read can be aligned without any issue but write will have if/else because of 
i2c_smbus_write_byte_data. Example below.

Something like this. If this change is fine I think there won't be any issue to 
just merge it with pca9570.

Thanks,
Michal

diff --git a/drivers/gpio/gpio-slg7xl45106.c b/drivers/gpio/gpio-slg7xl45106.c
index bf25e6fb6782..b90950ae38c1 100644
--- a/drivers/gpio/gpio-slg7xl45106.c
+++ b/drivers/gpio/gpio-slg7xl45106.c
@@ -22,20 +22,24 @@
  struct slg7xl45106 {
         struct gpio_chip chip;
         struct mutex lock;      /* To protect writes */
+       u32 command;
  };

  static int slg7xl45106_read(struct slg7xl45106 *gpio)
  {
         struct i2c_client *client = to_i2c_client(gpio->chip.parent);

-       return i2c_smbus_read_byte_data(client, SLG7XL45106_GPO_REG);
+       return i2c_smbus_read_byte_data(client, gpio->command);
  }

  static int slg7xl45106_write(struct slg7xl45106 *gpio, u8 value)
  {
         struct i2c_client *client = to_i2c_client(gpio->chip.parent);

-       return i2c_smbus_write_byte_data(client, SLG7XL45106_GPO_REG, value);
+       if (gpio->command)
+               return i2c_smbus_write_byte_data(client, SLG7XL45106_GPO_REG, 
value);
+
+       return i2c_smbus_write_byte(client, value);
  }

  static int slg7xl45106_get_direction(struct gpio_chip *chip,
@@ -93,6 +97,9 @@ static int slg7xl45106_probe(struct i2c_client *client)
         gpio->chip.ngpio = (uintptr_t)device_get_match_data(&client->dev);
         gpio->chip.can_sleep = true;

+       /* will be filled based on compatible string, 0 for pca9570 */
+       gpio->command = SLG7XL45106_GPO_REG;
+
         mutex_init(&gpio->lock);

         i2c_set_clientdata(client, gpio);

Re: [PATCH 2/2] gpio: Add support for SLG7XL45106 I2C GPO expander

[Andy Shevchenko]

On Wed, Jun 29, 2022 at 9:14 AM Michal Simek <michal.simek@amd.com> wrote:
> On 6/29/22 03:00, Sungbo Eo wrote:
> > On 2022-06-29 04:21, Andy Shevchenko wrote:
> >> On Tue, Jun 28, 2022 at 9:13 PM Andy Shevchenko
> >> <andy.shevchenko@gmail.com> wrote:

...

> >> Actually, why can't pca9570 be amended to support this?

> > It seems the slg7xl45106 driver reads/writes a reg at 0xDB so it is not
> > compatible with pca9570 driver (in the current state), and (I suppose)
> > it could be converted to use gpio-regmap.
> >
> > [1]
> > https://lore.kernel.org/linux-gpio/69f5d1a1970838b8c4bd8d6e8dba6cac@walle.cc/
>
> As was mentioned driver is based on pca9570 and the only important difference is
> with i2c_smbus_read_byte/i2c_smbus_read_byte_data and especially
> i2c_smbus_write_byte/i2c_smbus_write_byte_data.
>
> Read can be aligned without any issue but write will have if/else because of
> i2c_smbus_write_byte_data. Example below.
>
> Something like this. If this change is fine I think there won't be any issue to
> just merge it with pca9570.

Thanks, I also would like to see something as below in the result.

> diff --git a/drivers/gpio/gpio-slg7xl45106.c b/drivers/gpio/gpio-slg7xl45106.c
> index bf25e6fb6782..b90950ae38c1 100644
> --- a/drivers/gpio/gpio-slg7xl45106.c
> +++ b/drivers/gpio/gpio-slg7xl45106.c
> @@ -22,20 +22,24 @@
>   struct slg7xl45106 {
>          struct gpio_chip chip;
>          struct mutex lock;      /* To protect writes */
> +       u32 command;
>   };
>
>   static int slg7xl45106_read(struct slg7xl45106 *gpio)
>   {
>          struct i2c_client *client = to_i2c_client(gpio->chip.parent);
>
> -       return i2c_smbus_read_byte_data(client, SLG7XL45106_GPO_REG);
> +       return i2c_smbus_read_byte_data(client, gpio->command);
>   }
>
>   static int slg7xl45106_write(struct slg7xl45106 *gpio, u8 value)
>   {
>          struct i2c_client *client = to_i2c_client(gpio->chip.parent);
>
> -       return i2c_smbus_write_byte_data(client, SLG7XL45106_GPO_REG, value);
> +       if (gpio->command)
> +               return i2c_smbus_write_byte_data(client, SLG7XL45106_GPO_REG,
> value);

Missed change to gpio->command :-)

> +       return i2c_smbus_write_byte(client, value);
>   }
>
>   static int slg7xl45106_get_direction(struct gpio_chip *chip,
> @@ -93,6 +97,9 @@ static int slg7xl45106_probe(struct i2c_client *client)
>          gpio->chip.ngpio = (uintptr_t)device_get_match_data(&client->dev);
>          gpio->chip.can_sleep = true;
>
> +       /* will be filled based on compatible string, 0 for pca9570 */
> +       gpio->command = SLG7XL45106_GPO_REG;
> +
>          mutex_init(&gpio->lock);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: Add support for SLG7XL45106 I2C GPO expander

[Michal Simek]

On 6/29/22 12:15, Andy Shevchenko wrote:
> On Wed, Jun 29, 2022 at 9:14 AM Michal Simek <michal.simek@amd.com> wrote:
>> On 6/29/22 03:00, Sungbo Eo wrote:
>>> On 2022-06-29 04:21, Andy Shevchenko wrote:
>>>> On Tue, Jun 28, 2022 at 9:13 PM Andy Shevchenko
>>>> <andy.shevchenko@gmail.com> wrote:
> 
> ...
> 
>>>> Actually, why can't pca9570 be amended to support this?
> 
>>> It seems the slg7xl45106 driver reads/writes a reg at 0xDB so it is not
>>> compatible with pca9570 driver (in the current state), and (I suppose)
>>> it could be converted to use gpio-regmap.
>>>
>>> [1]
>>> https://lore.kernel.org/linux-gpio/69f5d1a1970838b8c4bd8d6e8dba6cac@walle.cc/
>>
>> As was mentioned driver is based on pca9570 and the only important difference is
>> with i2c_smbus_read_byte/i2c_smbus_read_byte_data and especially
>> i2c_smbus_write_byte/i2c_smbus_write_byte_data.
>>
>> Read can be aligned without any issue but write will have if/else because of
>> i2c_smbus_write_byte_data. Example below.
>>
>> Something like this. If this change is fine I think there won't be any issue to
>> just merge it with pca9570.
> 
> Thanks, I also would like to see something as below in the result.

ok. Good.

> 
>> diff --git a/drivers/gpio/gpio-slg7xl45106.c b/drivers/gpio/gpio-slg7xl45106.c
>> index bf25e6fb6782..b90950ae38c1 100644
>> --- a/drivers/gpio/gpio-slg7xl45106.c
>> +++ b/drivers/gpio/gpio-slg7xl45106.c
>> @@ -22,20 +22,24 @@
>>    struct slg7xl45106 {
>>           struct gpio_chip chip;
>>           struct mutex lock;      /* To protect writes */
>> +       u32 command;
>>    };
>>
>>    static int slg7xl45106_read(struct slg7xl45106 *gpio)
>>    {
>>           struct i2c_client *client = to_i2c_client(gpio->chip.parent);
>>
>> -       return i2c_smbus_read_byte_data(client, SLG7XL45106_GPO_REG);
>> +       return i2c_smbus_read_byte_data(client, gpio->command);
>>    }
>>
>>    static int slg7xl45106_write(struct slg7xl45106 *gpio, u8 value)
>>    {
>>           struct i2c_client *client = to_i2c_client(gpio->chip.parent);
>>
>> -       return i2c_smbus_write_byte_data(client, SLG7XL45106_GPO_REG, value);
>> +       if (gpio->command)
>> +               return i2c_smbus_write_byte_data(client, SLG7XL45106_GPO_REG,
>> value);
> 
> Missed change to gpio->command :-)

I found it too. :-)

Shubhrajyoti: Can you please merge that slg driver to 9570? That dt-binding will 
require small massage too.

Thanks,
Michal

RE: [PATCH 2/2] gpio: Add support for SLG7XL45106 I2C GPO expander

[Datta, Shubhrajyoti]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Simek, Michal <michal.simek@amd.com>
> Sent: Wednesday, June 29, 2022 4:08 PM
> To: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Sungbo Eo <mans0n@gorani.run>; Shubhrajyoti Datta
> <shubhrajyoti.datta@xilinx.com>; open list:GPIO SUBSYSTEM <linux-
> gpio@vger.kernel.org>; Michal Simek <michal.simek@xilinx.com>; git (AMD-
> Xilinx) <git@amd.com>; git <git@xilinx.com>
> Subject: Re: [PATCH 2/2] gpio: Add support for SLG7XL45106 I2C GPO
> expander
> 
> 
> 
> On 6/29/22 12:15, Andy Shevchenko wrote:
> > On Wed, Jun 29, 2022 at 9:14 AM Michal Simek <michal.simek@amd.com>
> wrote:
> >> On 6/29/22 03:00, Sungbo Eo wrote:
> >>> On 2022-06-29 04:21, Andy Shevchenko wrote:
> >>>> On Tue, Jun 28, 2022 at 9:13 PM Andy Shevchenko
> >>>> <andy.shevchenko@gmail.com> wrote:
> >
> > ...
> >
> >>>> Actually, why can't pca9570 be amended to support this?
> >
> >>> It seems the slg7xl45106 driver reads/writes a reg at 0xDB so it is
> >>> not compatible with pca9570 driver (in the current state), and (I
> >>> suppose) it could be converted to use gpio-regmap.
> >>>
> >>> [1]
> >>> https://lore.kernel.org/linux-
> gpio/69f5d1a1970838b8c4bd8d6e8dba6cac@
> >>> walle.cc/
> >>
> >> As was mentioned driver is based on pca9570 and the only important
> >> difference is with i2c_smbus_read_byte/i2c_smbus_read_byte_data and
> >> especially i2c_smbus_write_byte/i2c_smbus_write_byte_data.
> >>
> >> Read can be aligned without any issue but write will have if/else
> >> because of i2c_smbus_write_byte_data. Example below.
> >>
> >> Something like this. If this change is fine I think there won't be
> >> any issue to just merge it with pca9570.
> >
> > Thanks, I also would like to see something as below in the result.
> 
> ok. Good.
> 
> >
> >> diff --git a/drivers/gpio/gpio-slg7xl45106.c
> >> b/drivers/gpio/gpio-slg7xl45106.c index bf25e6fb6782..b90950ae38c1
> >> 100644
> >> --- a/drivers/gpio/gpio-slg7xl45106.c
> >> +++ b/drivers/gpio/gpio-slg7xl45106.c
> >> @@ -22,20 +22,24 @@
> >>    struct slg7xl45106 {
> >>           struct gpio_chip chip;
> >>           struct mutex lock;      /* To protect writes */
> >> +       u32 command;
> >>    };
> >>
> >>    static int slg7xl45106_read(struct slg7xl45106 *gpio)
> >>    {
> >>           struct i2c_client *client =
> >> to_i2c_client(gpio->chip.parent);
> >>
> >> -       return i2c_smbus_read_byte_data(client, SLG7XL45106_GPO_REG);
> >> +       return i2c_smbus_read_byte_data(client, gpio->command);
> >>    }
> >>
> >>    static int slg7xl45106_write(struct slg7xl45106 *gpio, u8 value)
> >>    {
> >>           struct i2c_client *client =
> >> to_i2c_client(gpio->chip.parent);
> >>
> >> -       return i2c_smbus_write_byte_data(client, SLG7XL45106_GPO_REG,
> value);
> >> +       if (gpio->command)
> >> +               return i2c_smbus_write_byte_data(client,
> >> + SLG7XL45106_GPO_REG,
> >> value);
> >
> > Missed change to gpio->command :-)
> 
> I found it too. :-)
> 
> Shubhrajyoti: Can you please merge that slg driver to 9570? That dt-binding
> will require small massage too.

Will fix in the next version.
Thanks 
> 
> Thanks,
> Michal
>