Am Tue, 23 Aug 2022 17:47:38 +0300 schrieb Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>: > 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. > > +#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. Henning
