2022-08-23 13:27:12

by Henning Schild

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

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 <[email protected]>
---
drivers/gpio/Kconfig | 3 +-
drivers/gpio/gpio-f7188x.c | 107 ++++++++++++++++++++++++++++---------
2 files changed, 84 insertions(+), 26 deletions(-)

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);
--
2.35.1


2022-08-23 16:37:32

by Henning Schild

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

Am Tue, 23 Aug 2022 17:47:38 +0300
schrieb Andy Shevchenko <[email protected]>:

> 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

2022-08-23 16:48:38

by Andy Shevchenko

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

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.

...

> +#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.

--
With Best Regards,
Andy Shevchenko


2022-08-24 13:28:14

by Simon Guinot

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

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 <[email protected]>:

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


Attachments:
(No filename) (2.94 kB)
signature.asc (849.00 B)
Download all attachments

2022-08-24 13:59:10

by Henning Schild

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

Am Wed, 24 Aug 2022 15:10:55 +0200
schrieb [email protected]:

> 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 <[email protected]>:
>
> 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

2022-08-24 14:13:41

by Hans de Goede

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

Hi Henning,

On 8/24/22 15:50, Henning Schild wrote:
> Am Wed, 24 Aug 2022 15:10:55 +0200
> schrieb [email protected]:
>
>> 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 <[email protected]>:
>>
>> 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

2022-08-24 14:35:26

by Henning Schild

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

Am Wed, 24 Aug 2022 15:54:28 +0200
schrieb Hans de Goede <[email protected]>:

> Hi Henning,
>
> On 8/24/22 15:50, Henning Schild wrote:
> > Am Wed, 24 Aug 2022 15:10:55 +0200
> > schrieb [email protected]:
> >
> >> 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 <[email protected]>:
> >>
> >> 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
>

2022-08-24 14:41:40

by Hans de Goede

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

Hi,

On 8/24/22 16:17, Henning Schild wrote:
> Am Wed, 24 Aug 2022 15:54:28 +0200
> schrieb Hans de Goede <[email protected]>:
>
>> Hi Henning,
>>
>> On 8/24/22 15:50, Henning Schild wrote:
>>> Am Wed, 24 Aug 2022 15:10:55 +0200
>>> schrieb [email protected]:
>>>
>>>> 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 <[email protected]>:
>>>>
>>>> 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

2022-08-24 16:09:40

by Simon Guinot

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

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 <[email protected]>:
> >
> >> Hi Henning,
> >>
> >> On 8/24/22 15:50, Henning Schild wrote:
> >>> Am Wed, 24 Aug 2022 15:10:55 +0200
> >>> schrieb [email protected]:
> >>>
> >>>> 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 <[email protected]>:
> >>>>
> >>>> 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


Attachments:
(No filename) (4.82 kB)
signature.asc (849.00 B)
Download all attachments

2022-08-26 13:58:14

by Linus Walleij

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

On Wed, Aug 24, 2022 at 4:18 PM Henning Schild
<[email protected]> 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