| Message ID | 20220915114803.26185-3-shubhrajyoti.datta@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: pca9570: add slg7xl45106 support | expand |
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.
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
[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&data=05%7C01%7Cshubhrajyoti.d > atta%40amd.com%7C9758241b75fc461113b608da98b50869%7C3dd8961fe488 > 4e608e11a82d994e183d%7C0%7C0%7C637990201003357055%7CUnknown%7 > CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi > LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=x0RHFhr9X0L3VBzTRyRy > VfLhm74gx7jBqUs2NEFhKcI%3D&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
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&data=05%7C01%7Cshubhrajyoti.d >> atta%40amd.com%7C9758241b75fc461113b608da98b50869%7C3dd8961fe488 >> 4e608e11a82d994e183d%7C0%7C0%7C637990201003357055%7CUnknown%7 >> CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi >> LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=x0RHFhr9X0L3VBzTRyRy >> VfLhm74gx7jBqUs2NEFhKcI%3D&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
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);
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(-)