| Message ID | 20220823102344.17624-2-henning.schild@siemens.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add support for another simatic board | expand |
On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote: > Am Tue, 23 Aug 2022 17:47:38 +0300 > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: Hi Andy, Thanks for this new version. It is looking good to me. > > > On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote: > > > Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO chips are > > > very similar to the ones from Fintek. In other subsystems they also > > > share drivers and are called a family of drivers. > > > > > > For the GPIO subsystem the only difference is that the direction > > > bit is reversed and that there is only one data bit per pin. On the > > > SuperIO level the logical device is another one. > > > > > > On a chip level we do not have a manufacturer ID to check and also > > > no revision. > > > > ... > > > > > - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, > > > F71889 and F81866 > > > + * GPIO driver for Fintek and Nuvoton Super-I/O chips > > > > I'm not sure it's good idea to drop it from here. It means reader has > > to get this info in a hard way. > > > > ... > > Let us see what others say. I wanted to keep this in line with what > Kconfig says and the oneliner in the Kconfig was getting pretty > longish. Hence i decided to shorten that. Other drivers also seem to > not list all the possible chips in many places, it is all maint effort > when a new chips is added and the list is in like 5 places. I agree with you that we can drop this line. It was already incomplete and the information is quite readable a few lines below in both the define list and the chip enumeration. > > > > +#define gpio_dir_invert(type) ((type) == nct6116d) > > > +#define gpio_data_single(type) ((type) == nct6116d) > > > > What's prevents us to add a proper prefix to these? I don't like the > > idea of them having "gpio" prefix. > > > > ... > > > > > + pr_info(DRVNAME ": Unsupported device 0x%04x\n", > > > devid); > > > + pr_debug(DRVNAME ": Not a Fintek device at > > > 0x%08x\n", addr); > > > + pr_info(DRVNAME ": Found %s at %#x\n", > > > + pr_info(DRVNAME ": revision %d\n", > > > > Can we, please, utilize pr_fmt()? > > > > > + (int)superio_inb(addr, > > > SIO_FINTEK_DEVREV)); > > > > Explicit casting in printf() means wrong specifier in 99% of cases. > > > > For all the other comments i will wait for a second opinion. I > specifically did not change existing code for more than the functional > changes needed. And a bit of checkpatch.pl fixing. > Beautification could be done on the way but would only cause > inconsistency. That driver is what it is, if someone wants to overhaul > the style ... that should be another patch. One likely not coming from > me. About the int cast, I think you can drop it while you are updating this line. It is unneeded. I have no opinion on the other comments and I am OK with the rest of the patch. Simon
Am Wed, 24 Aug 2022 15:10:55 +0200 schrieb simon.guinot@sequanux.org: > On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote: > > Am Tue, 23 Aug 2022 17:47:38 +0300 > > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > Hi Andy, > > Thanks for this new version. It is looking good to me. > > > > > > On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote: > > > > Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO > > > > chips are very similar to the ones from Fintek. In other > > > > subsystems they also share drivers and are called a family of > > > > drivers. > > > > > > > > For the GPIO subsystem the only difference is that the direction > > > > bit is reversed and that there is only one data bit per pin. On > > > > the SuperIO level the logical device is another one. > > > > > > > > On a chip level we do not have a manufacturer ID to check and > > > > also no revision. > > > > > > ... > > > > > > > - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, > > > > F71889 and F81866 > > > > + * GPIO driver for Fintek and Nuvoton Super-I/O chips > > > > > > I'm not sure it's good idea to drop it from here. It means reader > > > has to get this info in a hard way. > > > > > > ... > > > > Let us see what others say. I wanted to keep this in line with what > > Kconfig says and the oneliner in the Kconfig was getting pretty > > longish. Hence i decided to shorten that. Other drivers also seem to > > not list all the possible chips in many places, it is all maint > > effort when a new chips is added and the list is in like 5 places. > > I agree with you that we can drop this line. It was already incomplete > and the information is quite readable a few lines below in both the > define list and the chip enumeration. > > > > > > > +#define gpio_dir_invert(type) ((type) == nct6116d) > > > > +#define gpio_data_single(type) ((type) == nct6116d) > > > > > > What's prevents us to add a proper prefix to these? I don't like > > > the idea of them having "gpio" prefix. > > > > > > ... > > > > > > > + pr_info(DRVNAME ": Unsupported device > > > > 0x%04x\n", devid); > > > > + pr_debug(DRVNAME ": Not a Fintek > > > > device at 0x%08x\n", addr); > > > > + pr_info(DRVNAME ": Found %s at %#x\n", > > > > + pr_info(DRVNAME ": revision %d\n", > > > > > > Can we, please, utilize pr_fmt()? > > > > > > > + (int)superio_inb(addr, > > > > SIO_FINTEK_DEVREV)); > > > > > > Explicit casting in printf() means wrong specifier in 99% of > > > cases. > > > > For all the other comments i will wait for a second opinion. I > > specifically did not change existing code for more than the > > functional changes needed. And a bit of checkpatch.pl fixing. > > Beautification could be done on the way but would only cause > > inconsistency. That driver is what it is, if someone wants to > > overhaul the style ... that should be another patch. One likely not > > coming from me. > > About the int cast, I think you can drop it while you are updating > this line. It is unneeded. Ok two voices for doing that one fix along the way. I will send a v5 and hope nobody insists on me fixing the other findings in code i never wrote. regards, Henning > I have no opinion on the other comments and I am OK with the rest of > the patch. > > Simon
Hi Henning, On 8/24/22 15:50, Henning Schild wrote: > Am Wed, 24 Aug 2022 15:10:55 +0200 > schrieb simon.guinot@sequanux.org: > >> On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote: >>> Am Tue, 23 Aug 2022 17:47:38 +0300 >>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: >> >> Hi Andy, >> >> Thanks for this new version. It is looking good to me. >> >>> >>>> On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote: >>>>> Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO >>>>> chips are very similar to the ones from Fintek. In other >>>>> subsystems they also share drivers and are called a family of >>>>> drivers. >>>>> >>>>> For the GPIO subsystem the only difference is that the direction >>>>> bit is reversed and that there is only one data bit per pin. On >>>>> the SuperIO level the logical device is another one. >>>>> >>>>> On a chip level we do not have a manufacturer ID to check and >>>>> also no revision. >>>> >>>> ... >>>> >>>>> - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, >>>>> F71889 and F81866 >>>>> + * GPIO driver for Fintek and Nuvoton Super-I/O chips >>>> >>>> I'm not sure it's good idea to drop it from here. It means reader >>>> has to get this info in a hard way. >>>> >>>> ... >>> >>> Let us see what others say. I wanted to keep this in line with what >>> Kconfig says and the oneliner in the Kconfig was getting pretty >>> longish. Hence i decided to shorten that. Other drivers also seem to >>> not list all the possible chips in many places, it is all maint >>> effort when a new chips is added and the list is in like 5 places. >> >> I agree with you that we can drop this line. It was already incomplete >> and the information is quite readable a few lines below in both the >> define list and the chip enumeration. >> >>> >>>>> +#define gpio_dir_invert(type) ((type) == nct6116d) >>>>> +#define gpio_data_single(type) ((type) == nct6116d) >>>> >>>> What's prevents us to add a proper prefix to these? I don't like >>>> the idea of them having "gpio" prefix. >>>> >>>> ... >>>> >>>>> + pr_info(DRVNAME ": Unsupported device >>>>> 0x%04x\n", devid); >>>>> + pr_debug(DRVNAME ": Not a Fintek >>>>> device at 0x%08x\n", addr); >>>>> + pr_info(DRVNAME ": Found %s at %#x\n", >>>>> + pr_info(DRVNAME ": revision %d\n", >>>> >>>> Can we, please, utilize pr_fmt()? >>>> >>>>> + (int)superio_inb(addr, >>>>> SIO_FINTEK_DEVREV)); >>>> >>>> Explicit casting in printf() means wrong specifier in 99% of >>>> cases. >>> >>> For all the other comments i will wait for a second opinion. I >>> specifically did not change existing code for more than the >>> functional changes needed. And a bit of checkpatch.pl fixing. >>> Beautification could be done on the way but would only cause >>> inconsistency. That driver is what it is, if someone wants to >>> overhaul the style ... that should be another patch. One likely not >>> coming from me. >> >> About the int cast, I think you can drop it while you are updating >> this line. It is unneeded. > > Ok two voices for doing that one fix along the way. I will send a v5 > and hope nobody insists on me fixing the other findings in code i never > wrote. You did not write it, but you are using it to do hw-enablement for your company's products. So being asked to also some touch-ups left and right while you are at it really is not unexpected IMHO. Regards, Hans
Am Wed, 24 Aug 2022 15:54:28 +0200 schrieb Hans de Goede <hdegoede@redhat.com>: > Hi Henning, > > On 8/24/22 15:50, Henning Schild wrote: > > Am Wed, 24 Aug 2022 15:10:55 +0200 > > schrieb simon.guinot@sequanux.org: > > > >> On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote: > >>> Am Tue, 23 Aug 2022 17:47:38 +0300 > >>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > >> > >> Hi Andy, > >> > >> Thanks for this new version. It is looking good to me. > >> > >>> > >>>> On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote: > >>>> > >>>>> Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO > >>>>> chips are very similar to the ones from Fintek. In other > >>>>> subsystems they also share drivers and are called a family of > >>>>> drivers. > >>>>> > >>>>> For the GPIO subsystem the only difference is that the direction > >>>>> bit is reversed and that there is only one data bit per pin. On > >>>>> the SuperIO level the logical device is another one. > >>>>> > >>>>> On a chip level we do not have a manufacturer ID to check and > >>>>> also no revision. > >>>> > >>>> ... > >>>> > >>>>> - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, > >>>>> F71889 and F81866 > >>>>> + * GPIO driver for Fintek and Nuvoton Super-I/O chips > >>>> > >>>> I'm not sure it's good idea to drop it from here. It means reader > >>>> has to get this info in a hard way. > >>>> > >>>> ... > >>> > >>> Let us see what others say. I wanted to keep this in line with > >>> what Kconfig says and the oneliner in the Kconfig was getting > >>> pretty longish. Hence i decided to shorten that. Other drivers > >>> also seem to not list all the possible chips in many places, it > >>> is all maint effort when a new chips is added and the list is in > >>> like 5 places. > >> > >> I agree with you that we can drop this line. It was already > >> incomplete and the information is quite readable a few lines below > >> in both the define list and the chip enumeration. > >> > >>> > >>>>> +#define gpio_dir_invert(type) ((type) == nct6116d) > >>>>> +#define gpio_data_single(type) ((type) == nct6116d) > >>>>> > >>>> > >>>> What's prevents us to add a proper prefix to these? I don't like > >>>> the idea of them having "gpio" prefix. > >>>> > >>>> ... > >>>> > >>>>> + pr_info(DRVNAME ": Unsupported device > >>>>> 0x%04x\n", devid); > >>>>> + pr_debug(DRVNAME ": Not a Fintek > >>>>> device at 0x%08x\n", addr); > >>>>> + pr_info(DRVNAME ": Found %s at %#x\n", > >>>>> + pr_info(DRVNAME ": revision %d\n", > >>>> > >>>> Can we, please, utilize pr_fmt()? > >>>> > >>>>> + (int)superio_inb(addr, > >>>>> SIO_FINTEK_DEVREV)); > >>>> > >>>> Explicit casting in printf() means wrong specifier in 99% of > >>>> cases. > >>> > >>> For all the other comments i will wait for a second opinion. I > >>> specifically did not change existing code for more than the > >>> functional changes needed. And a bit of checkpatch.pl fixing. > >>> Beautification could be done on the way but would only cause > >>> inconsistency. That driver is what it is, if someone wants to > >>> overhaul the style ... that should be another patch. One likely > >>> not coming from me. > >> > >> About the int cast, I think you can drop it while you are updating > >> this line. It is unneeded. > > > > Ok two voices for doing that one fix along the way. I will send a v5 > > and hope nobody insists on me fixing the other findings in code i > > never wrote. > > You did not write it, but you are using it to do hw-enablement for > your company's products. So being asked to also some touch-ups > left and right while you are at it really is not unexpected IMHO. Sure thing. Dropping a few characters from a line i touch anyhow is easy enough. But i.e a refactoring to pr_fmt would feel like asking too much in my book. That feels like work of the author or maintainer. In fact i am just doing the homework of what i think should have long been done by Nuvoton. I hope that v5 will be acceptable. Henning > Regards, > > Hans >
Hi, On 8/24/22 16:17, Henning Schild wrote: > Am Wed, 24 Aug 2022 15:54:28 +0200 > schrieb Hans de Goede <hdegoede@redhat.com>: > >> Hi Henning, >> >> On 8/24/22 15:50, Henning Schild wrote: >>> Am Wed, 24 Aug 2022 15:10:55 +0200 >>> schrieb simon.guinot@sequanux.org: >>> >>>> On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote: >>>>> Am Tue, 23 Aug 2022 17:47:38 +0300 >>>>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: >>>> >>>> Hi Andy, >>>> >>>> Thanks for this new version. It is looking good to me. >>>> >>>>> >>>>>> On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote: >>>>>> >>>>>>> Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO >>>>>>> chips are very similar to the ones from Fintek. In other >>>>>>> subsystems they also share drivers and are called a family of >>>>>>> drivers. >>>>>>> >>>>>>> For the GPIO subsystem the only difference is that the direction >>>>>>> bit is reversed and that there is only one data bit per pin. On >>>>>>> the SuperIO level the logical device is another one. >>>>>>> >>>>>>> On a chip level we do not have a manufacturer ID to check and >>>>>>> also no revision. >>>>>> >>>>>> ... >>>>>> >>>>>>> - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, >>>>>>> F71889 and F81866 >>>>>>> + * GPIO driver for Fintek and Nuvoton Super-I/O chips >>>>>> >>>>>> I'm not sure it's good idea to drop it from here. It means reader >>>>>> has to get this info in a hard way. >>>>>> >>>>>> ... >>>>> >>>>> Let us see what others say. I wanted to keep this in line with >>>>> what Kconfig says and the oneliner in the Kconfig was getting >>>>> pretty longish. Hence i decided to shorten that. Other drivers >>>>> also seem to not list all the possible chips in many places, it >>>>> is all maint effort when a new chips is added and the list is in >>>>> like 5 places. >>>> >>>> I agree with you that we can drop this line. It was already >>>> incomplete and the information is quite readable a few lines below >>>> in both the define list and the chip enumeration. >>>> >>>>> >>>>>>> +#define gpio_dir_invert(type) ((type) == nct6116d) >>>>>>> +#define gpio_data_single(type) ((type) == nct6116d) >>>>>>> >>>>>> >>>>>> What's prevents us to add a proper prefix to these? I don't like >>>>>> the idea of them having "gpio" prefix. >>>>>> >>>>>> ... >>>>>> >>>>>>> + pr_info(DRVNAME ": Unsupported device >>>>>>> 0x%04x\n", devid); >>>>>>> + pr_debug(DRVNAME ": Not a Fintek >>>>>>> device at 0x%08x\n", addr); >>>>>>> + pr_info(DRVNAME ": Found %s at %#x\n", >>>>>>> + pr_info(DRVNAME ": revision %d\n", >>>>>> >>>>>> Can we, please, utilize pr_fmt()? >>>>>> >>>>>>> + (int)superio_inb(addr, >>>>>>> SIO_FINTEK_DEVREV)); >>>>>> >>>>>> Explicit casting in printf() means wrong specifier in 99% of >>>>>> cases. >>>>> >>>>> For all the other comments i will wait for a second opinion. I >>>>> specifically did not change existing code for more than the >>>>> functional changes needed. And a bit of checkpatch.pl fixing. >>>>> Beautification could be done on the way but would only cause >>>>> inconsistency. That driver is what it is, if someone wants to >>>>> overhaul the style ... that should be another patch. One likely >>>>> not coming from me. >>>> >>>> About the int cast, I think you can drop it while you are updating >>>> this line. It is unneeded. >>> >>> Ok two voices for doing that one fix along the way. I will send a v5 >>> and hope nobody insists on me fixing the other findings in code i >>> never wrote. >> >> You did not write it, but you are using it to do hw-enablement for >> your company's products. So being asked to also some touch-ups >> left and right while you are at it really is not unexpected IMHO. > > Sure thing. Dropping a few characters from a line i touch anyhow is > easy enough. But i.e a refactoring to pr_fmt would feel like asking too > much in my book. That feels like work of the author or maintainer. Right, but that assumes that the original author / maintainer is still around and actively contributing which unfortunately is not always the case. Regards, Hans
On Wed, Aug 24, 2022 at 04:24:46PM +0200, Hans de Goede wrote: > Hi, > > On 8/24/22 16:17, Henning Schild wrote: > > Am Wed, 24 Aug 2022 15:54:28 +0200 > > schrieb Hans de Goede <hdegoede@redhat.com>: > > > >> Hi Henning, > >> > >> On 8/24/22 15:50, Henning Schild wrote: > >>> Am Wed, 24 Aug 2022 15:10:55 +0200 > >>> schrieb simon.guinot@sequanux.org: > >>> > >>>> On Tue, Aug 23, 2022 at 04:54:59PM +0200, Henning Schild wrote: > >>>>> Am Tue, 23 Aug 2022 17:47:38 +0300 > >>>>> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > >>>> > >>>> Hi Andy, > >>>> > >>>> Thanks for this new version. It is looking good to me. > >>>> > >>>>> > >>>>>> On Tue, Aug 23, 2022 at 12:23:40PM +0200, Henning Schild wrote: > >>>>>> > >>>>>>> Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO > >>>>>>> chips are very similar to the ones from Fintek. In other > >>>>>>> subsystems they also share drivers and are called a family of > >>>>>>> drivers. > >>>>>>> > >>>>>>> For the GPIO subsystem the only difference is that the direction > >>>>>>> bit is reversed and that there is only one data bit per pin. On > >>>>>>> the SuperIO level the logical device is another one. > >>>>>>> > >>>>>>> On a chip level we do not have a manufacturer ID to check and > >>>>>>> also no revision. > >>>>>> > >>>>>> ... > >>>>>> > >>>>>>> - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, > >>>>>>> F71889 and F81866 > >>>>>>> + * GPIO driver for Fintek and Nuvoton Super-I/O chips > >>>>>> > >>>>>> I'm not sure it's good idea to drop it from here. It means reader > >>>>>> has to get this info in a hard way. > >>>>>> > >>>>>> ... > >>>>> > >>>>> Let us see what others say. I wanted to keep this in line with > >>>>> what Kconfig says and the oneliner in the Kconfig was getting > >>>>> pretty longish. Hence i decided to shorten that. Other drivers > >>>>> also seem to not list all the possible chips in many places, it > >>>>> is all maint effort when a new chips is added and the list is in > >>>>> like 5 places. > >>>> > >>>> I agree with you that we can drop this line. It was already > >>>> incomplete and the information is quite readable a few lines below > >>>> in both the define list and the chip enumeration. > >>>> > >>>>> > >>>>>>> +#define gpio_dir_invert(type) ((type) == nct6116d) > >>>>>>> +#define gpio_data_single(type) ((type) == nct6116d) > >>>>>>> > >>>>>> > >>>>>> What's prevents us to add a proper prefix to these? I don't like > >>>>>> the idea of them having "gpio" prefix. > >>>>>> > >>>>>> ... > >>>>>> > >>>>>>> + pr_info(DRVNAME ": Unsupported device > >>>>>>> 0x%04x\n", devid); > >>>>>>> + pr_debug(DRVNAME ": Not a Fintek > >>>>>>> device at 0x%08x\n", addr); > >>>>>>> + pr_info(DRVNAME ": Found %s at %#x\n", > >>>>>>> + pr_info(DRVNAME ": revision %d\n", > >>>>>> > >>>>>> Can we, please, utilize pr_fmt()? > >>>>>> > >>>>>>> + (int)superio_inb(addr, > >>>>>>> SIO_FINTEK_DEVREV)); > >>>>>> > >>>>>> Explicit casting in printf() means wrong specifier in 99% of > >>>>>> cases. > >>>>> > >>>>> For all the other comments i will wait for a second opinion. I > >>>>> specifically did not change existing code for more than the > >>>>> functional changes needed. And a bit of checkpatch.pl fixing. > >>>>> Beautification could be done on the way but would only cause > >>>>> inconsistency. That driver is what it is, if someone wants to > >>>>> overhaul the style ... that should be another patch. One likely > >>>>> not coming from me. > >>>> > >>>> About the int cast, I think you can drop it while you are updating > >>>> this line. It is unneeded. > >>> > >>> Ok two voices for doing that one fix along the way. I will send a v5 > >>> and hope nobody insists on me fixing the other findings in code i > >>> never wrote. > >> > >> You did not write it, but you are using it to do hw-enablement for > >> your company's products. So being asked to also some touch-ups > >> left and right while you are at it really is not unexpected IMHO. > > > > Sure thing. Dropping a few characters from a line i touch anyhow is > > easy enough. But i.e a refactoring to pr_fmt would feel like asking too > > much in my book. That feels like work of the author or maintainer. > > Right, but that assumes that the original author / maintainer is still > around and actively contributing which unfortunately is not always > the case. Actually the original author is not active but he is still keeping an eye on the driver :) I still review and test the patches I catch on the MLs. And I am ready to do some maintenance work if needed. Henning, I think you could have done the pr_fmt conversion. It is not a big deal and it would have been nice. But indeed, you don't have to... Simon
On Wed, Aug 24, 2022 at 4:18 PM Henning Schild <henning.schild@siemens.com> wrote: > > You did not write it, but you are using it to do hw-enablement for > > your company's products. So being asked to also some touch-ups > > left and right while you are at it really is not unexpected IMHO. > > Sure thing. Dropping a few characters from a line i touch anyhow is > easy enough. But i.e a refactoring to pr_fmt would feel like asking too > much in my book. That feels like work of the author or maintainer. > > In fact i am just doing the homework of what i think should have long > been done by Nuvoton. A lot of vendors don't have much active upstream participation, they outsource that work to people like yourself by just ignoring it. > I hope that v5 will be acceptable. Bartosz is applying GPIO patches now, but my principle was that when I feel a patch makes the kernel look better after than before the patch and no new version is coming, I just apply the patch. This is how we deal with "perfect is the enemy of good" in practice. That said, we are all grateful for any improvements you manage to sneak in! Yours, Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 0642f579196f..3f64345fe40b 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -874,10 +874,11 @@ config GPIO_104_IDI_48 module parameter. config GPIO_F7188X - tristate "F71869, F71869A, F71882FG, F71889F and F81866 GPIO support" + tristate "Fintek and Nuvoton Super-I/O GPIO support" help This option enables support for GPIOs found on Fintek Super-I/O chips F71869, F71869A, F71882FG, F71889F and F81866. + As well as Nuvoton Super-I/O chip NCT6116D. To compile this driver as a module, choose M here: the module will be called f7188x-gpio. diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c index 18a3147f5a42..820ac5d60fda 100644 --- a/drivers/gpio/gpio-f7188x.c +++ b/drivers/gpio/gpio-f7188x.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-or-later /* - * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, F71889 and F81866 + * GPIO driver for Fintek and Nuvoton Super-I/O chips * * Copyright (C) 2010-2013 LaCie * @@ -21,23 +21,36 @@ */ #define SIO_LDSEL 0x07 /* Logical device select */ #define SIO_DEVID 0x20 /* Device ID (2 bytes) */ -#define SIO_DEVREV 0x22 /* Device revision */ -#define SIO_MANID 0x23 /* Fintek ID (2 bytes) */ -#define SIO_LD_GPIO 0x06 /* GPIO logical device */ #define SIO_UNLOCK_KEY 0x87 /* Key to enable Super-I/O */ #define SIO_LOCK_KEY 0xAA /* Key to disable Super-I/O */ -#define SIO_FINTEK_ID 0x1934 /* Manufacturer ID */ +/* + * Fintek devices. + */ +#define SIO_FINTEK_DEVREV 0x22 /* Fintek Device revision */ +#define SIO_FINTEK_MANID 0x23 /* Fintek ID (2 bytes) */ + +#define SIO_FINTEK_ID 0x1934 /* Manufacturer ID */ + #define SIO_F71869_ID 0x0814 /* F71869 chipset ID */ #define SIO_F71869A_ID 0x1007 /* F71869A chipset ID */ #define SIO_F71882_ID 0x0541 /* F71882 chipset ID */ #define SIO_F71889_ID 0x0909 /* F71889 chipset ID */ #define SIO_F71889A_ID 0x1005 /* F71889A chipset ID */ #define SIO_F81866_ID 0x1010 /* F81866 chipset ID */ -#define SIO_F81804_ID 0x1502 /* F81804 chipset ID, same for f81966 */ +#define SIO_F81804_ID 0x1502 /* F81804 chipset ID, same for F81966 */ #define SIO_F81865_ID 0x0704 /* F81865 chipset ID */ +#define SIO_LD_GPIO_FINTEK 0x06 /* GPIO logical device */ + +/* + * Nuvoton devices. + */ +#define SIO_NCT6116D_ID 0xD283 /* NCT6116D chipset ID */ + +#define SIO_LD_GPIO_NUVOTON 0x07 /* GPIO logical device */ + enum chips { f71869, @@ -48,6 +61,7 @@ enum chips { f81866, f81804, f81865, + nct6116d, }; static const char * const f7188x_names[] = { @@ -59,10 +73,12 @@ static const char * const f7188x_names[] = { "f81866", "f81804", "f81865", + "nct6116d", }; struct f7188x_sio { int addr; + int device; enum chips type; }; @@ -170,6 +186,9 @@ static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset, /* Output mode register (0:open drain 1:push-pull). */ #define gpio_out_mode(base) (base + 3) +#define gpio_dir_invert(type) ((type) == nct6116d) +#define gpio_data_single(type) ((type) == nct6116d) + static struct f7188x_gpio_bank f71869_gpio_bank[] = { F7188X_GPIO_BANK(0, 6, 0xF0), F7188X_GPIO_BANK(10, 8, 0xE0), @@ -254,6 +273,17 @@ static struct f7188x_gpio_bank f81865_gpio_bank[] = { F7188X_GPIO_BANK(60, 5, 0x90), }; +static struct f7188x_gpio_bank nct6116d_gpio_bank[] = { + F7188X_GPIO_BANK(0, 8, 0xE0), + F7188X_GPIO_BANK(10, 8, 0xE4), + F7188X_GPIO_BANK(20, 8, 0xE8), + F7188X_GPIO_BANK(30, 8, 0xEC), + F7188X_GPIO_BANK(40, 8, 0xF0), + F7188X_GPIO_BANK(50, 8, 0xF4), + F7188X_GPIO_BANK(60, 8, 0xF8), + F7188X_GPIO_BANK(70, 1, 0xFC), +}; + static int f7188x_gpio_get_direction(struct gpio_chip *chip, unsigned offset) { int err; @@ -264,13 +294,16 @@ static int f7188x_gpio_get_direction(struct gpio_chip *chip, unsigned offset) err = superio_enter(sio->addr); if (err) return err; - superio_select(sio->addr, SIO_LD_GPIO); + superio_select(sio->addr, sio->device); dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); superio_exit(sio->addr); - if (dir & 1 << offset) + if (gpio_dir_invert(sio->type)) + dir = ~dir; + + if (dir & BIT(offset)) return GPIO_LINE_DIRECTION_OUT; return GPIO_LINE_DIRECTION_IN; @@ -286,10 +319,14 @@ static int f7188x_gpio_direction_in(struct gpio_chip *chip, unsigned offset) err = superio_enter(sio->addr); if (err) return err; - superio_select(sio->addr, SIO_LD_GPIO); + superio_select(sio->addr, sio->device); dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); - dir &= ~BIT(offset); + + if (gpio_dir_invert(sio->type)) + dir |= BIT(offset); + else + dir &= ~BIT(offset); superio_outb(sio->addr, gpio_dir(bank->regbase), dir); superio_exit(sio->addr); @@ -307,11 +344,11 @@ static int f7188x_gpio_get(struct gpio_chip *chip, unsigned offset) err = superio_enter(sio->addr); if (err) return err; - superio_select(sio->addr, SIO_LD_GPIO); + superio_select(sio->addr, sio->device); dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); dir = !!(dir & BIT(offset)); - if (dir) + if (gpio_data_single(sio->type) || dir) data = superio_inb(sio->addr, gpio_data_out(bank->regbase)); else data = superio_inb(sio->addr, gpio_data_in(bank->regbase)); @@ -332,7 +369,7 @@ static int f7188x_gpio_direction_out(struct gpio_chip *chip, err = superio_enter(sio->addr); if (err) return err; - superio_select(sio->addr, SIO_LD_GPIO); + superio_select(sio->addr, sio->device); data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase)); if (value) @@ -342,7 +379,10 @@ static int f7188x_gpio_direction_out(struct gpio_chip *chip, superio_outb(sio->addr, gpio_data_out(bank->regbase), data_out); dir = superio_inb(sio->addr, gpio_dir(bank->regbase)); - dir |= BIT(offset); + if (gpio_dir_invert(sio->type)) + dir &= ~BIT(offset); + else + dir |= BIT(offset); superio_outb(sio->addr, gpio_dir(bank->regbase), dir); superio_exit(sio->addr); @@ -360,7 +400,7 @@ static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value) err = superio_enter(sio->addr); if (err) return; - superio_select(sio->addr, SIO_LD_GPIO); + superio_select(sio->addr, sio->device); data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase)); if (value) @@ -388,7 +428,7 @@ static int f7188x_gpio_set_config(struct gpio_chip *chip, unsigned offset, err = superio_enter(sio->addr); if (err) return err; - superio_select(sio->addr, SIO_LD_GPIO); + superio_select(sio->addr, sio->device); data = superio_inb(sio->addr, gpio_out_mode(bank->regbase)); if (param == PIN_CONFIG_DRIVE_OPEN_DRAIN) @@ -449,6 +489,10 @@ static int f7188x_gpio_probe(struct platform_device *pdev) data->nr_bank = ARRAY_SIZE(f81865_gpio_bank); data->bank = f81865_gpio_bank; break; + case nct6116d: + data->nr_bank = ARRAY_SIZE(nct6116d_gpio_bank); + data->bank = nct6116d_gpio_bank; + break; default: return -ENODEV; } @@ -479,18 +523,15 @@ static int __init f7188x_find(int addr, struct f7188x_sio *sio) { int err; u16 devid; + u16 manid; err = superio_enter(addr); if (err) return err; err = -ENODEV; - devid = superio_inw(addr, SIO_MANID); - if (devid != SIO_FINTEK_ID) { - pr_debug(DRVNAME ": Not a Fintek device at 0x%08x\n", addr); - goto err; - } + sio->device = SIO_LD_GPIO_FINTEK; devid = superio_inw(addr, SIO_DEVID); switch (devid) { case SIO_F71869_ID: @@ -517,17 +558,33 @@ static int __init f7188x_find(int addr, struct f7188x_sio *sio) case SIO_F81865_ID: sio->type = f81865; break; + case SIO_NCT6116D_ID: + sio->device = SIO_LD_GPIO_NUVOTON; + sio->type = nct6116d; + break; default: - pr_info(DRVNAME ": Unsupported Fintek device 0x%04x\n", devid); + pr_info(DRVNAME ": Unsupported device 0x%04x\n", devid); goto err; } + + /* double check manufacturer where possible */ + if (sio->type != nct6116d) { + manid = superio_inw(addr, SIO_FINTEK_MANID); + if (manid != SIO_FINTEK_ID) { + pr_debug(DRVNAME ": Not a Fintek device at 0x%08x\n", addr); + goto err; + } + } + sio->addr = addr; err = 0; - pr_info(DRVNAME ": Found %s at %#x, revision %d\n", + pr_info(DRVNAME ": Found %s at %#x\n", f7188x_names[sio->type], - (unsigned int) addr, - (int) superio_inb(addr, SIO_DEVREV)); + (unsigned int)addr); + if (sio->type != nct6116d) + pr_info(DRVNAME ": revision %d\n", + (int)superio_inb(addr, SIO_FINTEK_DEVREV)); err: superio_exit(addr);
Add GPIO support for Nuvoton NCT6116 chip. Nuvoton SuperIO chips are very similar to the ones from Fintek. In other subsystems they also share drivers and are called a family of drivers. For the GPIO subsystem the only difference is that the direction bit is reversed and that there is only one data bit per pin. On the SuperIO level the logical device is another one. On a chip level we do not have a manufacturer ID to check and also no revision. Signed-off-by: Henning Schild <henning.schild@siemens.com> --- drivers/gpio/Kconfig | 3 +- drivers/gpio/gpio-f7188x.c | 107 ++++++++++++++++++++++++++++--------- 2 files changed, 84 insertions(+), 26 deletions(-)