Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116

From: Henning Schild <henning.schild@siemens.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <brgl@bgdev.pl>, Pavel Machek <pavel@ucw.cz>,
	Hans de Goede <hdegoede@redhat.com>,
	Mark Gross <markgross@kernel.org>, Lee Jones <lee@kernel.org>,
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-leds@vger.kernel.org, platform-driver-x86@vger.kernel.org,
	Sheng-Yuan Huang <syhuang3@nuvoton.com>,
	Tasanakorn Phaipool <tasanakorn@gmail.com>,
	simon.guinot@sequanux.org
Subject: Re: [PATCH v4 1/5] gpio-f7188x: Add GPIO support for Nuvoton NCT6116
Date: Tue, 23 Aug 2022 16:54:59 +0200	[thread overview]
Message-ID: <20220823165459.143e1c30@md1za8fc.ad001.siemens.net> (raw)
In-Reply-To: <YwToilxquEZGqzQD@smile.fi.intel.com>

Am Tue, 23 Aug 2022 17:47:38 +0300
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> 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