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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YvUEu8bUc2RgtRpi@76cbfcf04d45>
Date:   Thu, 11 Aug 2022 15:31:39 +0200
From:   simon.guinot@...uanux.org
To:     Henning Schild <henning.schild@...mens.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Pavel Machek <pavel@....cz>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Lee Jones <lee@...nel.org>, linux-gpio@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
        platform-driver-x86@...r.kernel.org,
        Sheng-Yuan Huang <syhuang3@...oton.com>,
        Tasanakorn Phaipool <tasanakorn@...il.com>
Subject: Re: [PATCH v2 1/4] gpio-f7188x: Add GPIO support for Nuvoton NCT6116

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@...mens.com>

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.

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)