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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Wed, Jun 29, 2022 at 9:14 AM Michal Simek <michal.simek@xxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxx/
>
> 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