Am Wed, 24 Aug 2022 15:54:28 +0200 schrieb Hans de Goede <hdegoede@xxxxxxxxxx>: > Hi Henning, > > On 8/24/22 15:50, Henning Schild wrote: > > Am Wed, 24 Aug 2022 15:10:55 +0200 > > schrieb simon.guinot@xxxxxxxxxxxx: > > > >> 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@xxxxxxxxxxxxxxx>: > >> > >> 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 >
