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
