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

[simon . guinot]

On Tue, Aug 09, 2022 at 05:04:39PM +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.
> 
> Signed-off-by: Henning Schild <henning.schild@xxxxxxxxxxx>

Hi Henning,

This patch is looking good to me. I only have a couple of minor
comments. Please see them below.

> ---
>  drivers/gpio/gpio-f7188x.c | 70 +++++++++++++++++++++++++++-----------
>  1 file changed, 51 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
> index 18a3147f5a42..4d8f38bc3b45 100644
> --- a/drivers/gpio/gpio-f7188x.c
> +++ b/drivers/gpio/gpio-f7188x.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * GPIO driver for Fintek Super-I/O F71869, F71869A, F71882, F71889 and F81866
> + * and Nuvoton Super-I/O NCT6116D
>   *
>   * Copyright (C) 2010-2013 LaCie
>   *
> @@ -22,13 +23,11 @@
>  #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 */
> +#define SIO_LD_GPIO_FINTEK	0x06	/* GPIO logical device */
>  #define SIO_F71869_ID		0x0814	/* F71869 chipset ID */
>  #define SIO_F71869A_ID		0x1007	/* F71869A chipset ID */
>  #define SIO_F71882_ID		0x0541	/* F71882 chipset ID */
> @@ -38,6 +37,8 @@
>  #define SIO_F81804_ID		0x1502  /* F81804 chipset ID, same for f81966 */
>  #define SIO_F81865_ID		0x0704	/* F81865 chipset ID */
>  
> +#define SIO_LD_GPIO_NUVOTON	0x07	/* GPIO logical device */
> +#define SIO_NCT6116D_ID		0xD283  /* NCT6116D chipset ID */

Can we do better to make the definitions above more readable ? With the
new additions I find it a little bit unclear.

Maybe we could add a comment on the top of the Fintek and Nuvoton
specific sections ? Or maybe we could group the LD_GPIO_ definitions
in a dedicated section ? Or something else :)

>  
>  enum chips {
>  	f71869,
> @@ -48,6 +49,7 @@ enum chips {
>  	f81866,
>  	f81804,
>  	f81865,
> +	nct6116d,
>  };
>  
>  static const char * const f7188x_names[] = {
> @@ -59,10 +61,12 @@ static const char * const f7188x_names[] = {
>  	"f81866",
>  	"f81804",
>  	"f81865",
> +	"nct6116d",
>  };
>  
>  struct f7188x_sio {
>  	int addr;
> +	int device;
>  	enum chips type;
>  };
>  
> @@ -170,6 +174,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_needs_invert(device)	((device) != SIO_LD_GPIO_FINTEK)
> +#define gpio_single_data(device)	((device) != SIO_LD_GPIO_FINTEK)

Since this macros are only used to get/set GPIO direction, then I think
we should use the "gpio_dir_" prefix.

Also is there any reason to match the LD GPIO value rather than the
chipset type ?

I think we should enable this specific path only for a Nuvoton NCT6116
device for now (by matching the NCT6116 chipset type). So if more
devices are added later then we are sure they still go on the original
path.

Attachment: signature.asc
Description: PGP signature