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

[Michal Simek <michal.simek@amd.com>]

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);