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

["Datta, Shubhrajyoti" <shubhrajyoti.datta@xxxxxxx>]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Simek, Michal <michal.simek@xxxxxxx>
> Sent: Wednesday, June 29, 2022 4:08 PM
> To: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Cc: Sungbo Eo <mans0n@xxxxxxxxxx>; Shubhrajyoti Datta
> <shubhrajyoti.datta@xxxxxxxxxx>; open list:GPIO SUBSYSTEM <linux-
> gpio@xxxxxxxxxxxxxxx>; Michal Simek <michal.simek@xxxxxxxxxx>; git (AMD-
> Xilinx) <git@xxxxxxx>; git <git@xxxxxxxxxx>
> 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@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@
> >>> 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
>