[PATCH 0/2] gpio-f7188x: Add F81886 gpio function

[PATCH 0/2] gpio-f7188x: Add F81886 gpio function

[Peter Hung]

Fintek F81886 SuperIO spec:
http://www.alldatasheet.com/datasheet-pdf/pdf/459085/FINTEK/F81866AD-I.html
The detail gpio check & configuration could be found within spec P126.

It controls gpio is the same with F7188x and has max 9 set of gpios.
The gpio address is below:
	GPIO0x based: 0xf0
	GPIO1x based: 0xe0
	GPIO2x based: 0xd0
	GPIO3x based: 0xc0
	GPIO4x based: 0xb0
	GPIO5x based: 0xa0
	GPIO6x based: 0x90
	GPIO7x based: 0x80
	GPIO8x based: 0x88 <-- not 0x70.

The first patch only add basic structure for F81866 to use all unverified
gpio, and second patch add filter function to make sure only enabled gpios
are exported.

Peter Hung (2):
  gpio-f7188x: add Fintek F81866 SuperIO support
  gpio-f7188x: filter non-export gpio for F81866

 drivers/gpio/gpio-f7188x.c | 176 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 175 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH 1/2] gpio-f7188x: add Fintek F81866 SuperIO support

[Peter Hung]

add basic F81866 SuperIO gpio support

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/gpio/gpio-f7188x.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
index 5e3c4fa..a6a9641 100644
--- a/drivers/gpio/gpio-f7188x.c
+++ b/drivers/gpio/gpio-f7188x.c
@@ -36,14 +36,16 @@
 #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_F81866_ID		0x1010	/* F81866 chipset ID */
 
-enum chips { f71869, f71869a, f71882fg, f71889f };
+enum chips { f71869, f71869a, f71882fg, f71889f, f81866 };
 
 static const char * const f7188x_names[] = {
 	"f71869",
 	"f71869a",
 	"f71882fg",
 	"f71889f",
+	"f81866",
 };
 
 struct f7188x_sio {
@@ -190,6 +192,18 @@ static struct f7188x_gpio_bank f71889_gpio_bank[] = {
 	F7188X_GPIO_BANK(70, 8, 0x80),
 };
 
+static struct f7188x_gpio_bank f81866_gpio_bank[] = {
+	F7188X_GPIO_BANK(0, 8, 0xF0),
+	F7188X_GPIO_BANK(10, 8, 0xE0),
+	F7188X_GPIO_BANK(20, 8, 0xD0),
+	F7188X_GPIO_BANK(30, 8, 0xC0),
+	F7188X_GPIO_BANK(40, 8, 0xB0),
+	F7188X_GPIO_BANK(50, 8, 0xA0),
+	F7188X_GPIO_BANK(60, 8, 0x90),
+	F7188X_GPIO_BANK(70, 8, 0x80),
+	F7188X_GPIO_BANK(80, 8, 0x88),
+};
+
 static int f7188x_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
 {
 	int err;
@@ -322,6 +336,10 @@ static int f7188x_gpio_probe(struct platform_device *pdev)
 		data->nr_bank = ARRAY_SIZE(f71889_gpio_bank);
 		data->bank = f71889_gpio_bank;
 		break;
+	case f81866:
+		data->nr_bank = ARRAY_SIZE(f81866_gpio_bank);
+		data->bank = f81866_gpio_bank;
+		break;
 	default:
 		return -ENODEV;
 	}
@@ -399,6 +417,9 @@ static int __init f7188x_find(int addr, struct f7188x_sio *sio)
 	case SIO_F71889_ID:
 		sio->type = f71889f;
 		break;
+	case SIO_F81866_ID:
+		sio->type = f81866;
+		break;
 	default:
 		pr_info(DRVNAME ": Unsupported Fintek device 0x%04x\n", devid);
 		goto err;
-- 
1.9.1

[PATCH 2/2] gpio-f7188x: filter non-export gpio for F81866

[Peter Hung]

Dont export gpios which not enabled by motherboard manufacturer.

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/gpio/gpio-f7188x.c | 153 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
index a6a9641..f7fe7aa 100644
--- a/drivers/gpio/gpio-f7188x.c
+++ b/drivers/gpio/gpio-f7188x.c
@@ -38,6 +38,12 @@
 #define SIO_F71889_ID		0x0909	/* F71889 chipset ID */
 #define SIO_F81866_ID		0x1010	/* F81866 chipset ID */
 
+#define F81866_PORT_SEL_REG	0x27
+#define F81866_MULTI_FUN1_REG	0x28
+#define F81866_MULTI_FUN3_REG	0x29
+#define F81866_MULTI_FUN4_REG	0x2B
+#define F81866_GPIO_EN_REG	0x2C
+
 enum chips { f71869, f71869a, f71882fg, f71889f, f81866 };
 
 static const char * const f7188x_names[] = {
@@ -93,6 +99,15 @@ static inline void superio_outb(int base, int reg, int val)
 	outb(val, base + 1);
 }
 
+static inline void superio_mask_outb(int base, int reg, int mask, int val)
+{
+	u8 tmp;
+
+	tmp = superio_inb(base, reg);
+	tmp = (tmp & ~mask) | (val & mask);
+	superio_outb(base, reg, tmp);
+}
+
 static inline int superio_enter(int base)
 {
 	/* Don't step on other drivers' I/O space by accident. */
@@ -304,6 +319,125 @@ static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	superio_exit(sio->addr);
 }
 
+static int f81866_verify_gpioset(int base, int set)
+{
+	int err;
+	u8 tmp;
+
+	err = superio_enter(base);
+	if (err)
+		return err;
+
+	err = -ENODEV;
+
+	switch (set) {
+	case 0:
+		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
+				BIT(0), 0);
+		tmp = superio_inb(base, F81866_GPIO_EN_REG);
+		if ((tmp & 0x1f) != 0x1f)
+			break; /* one in GPIO00~GPIO04 is not enable */
+
+		tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
+		if ((tmp & 0xc) != 0x0)
+			break;
+
+		err = 0; /* GPIO0x set is all enabled */
+		break;
+	case 1:
+		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
+				BIT(0), BIT(2));
+		tmp = superio_inb(base, F81866_GPIO_EN_REG);
+		if ((tmp & 0xef) != 0xef)
+			break; /* one in GPIO10~GPIO17 is not enable */
+
+		tmp = superio_inb(base, F81866_MULTI_FUN3_REG);
+		if ((tmp & 0x03) != 0x00)
+			break; /* one in GPIO12~GPIO13 is not enable */
+
+		err = 0; /* GPIO1x set is all enabled */
+		break;
+	case 2:
+		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
+				BIT(0), BIT(3));
+		tmp = superio_inb(base, F81866_GPIO_EN_REG);
+		if ((tmp & 0xff) != 0xff)
+			break; /* one in GPIO20~GPIO27 is not enable */
+
+		tmp = superio_inb(base, F81866_MULTI_FUN3_REG);
+		if ((tmp & 0x08) != 0x00)
+			break; /* GPIO20 is not enable */
+
+		err = 0; /* GPIO2x set is all enabled */
+		break;
+	case 3:
+		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(0), 0);
+		tmp = superio_inb(base, F81866_MULTI_FUN3_REG);
+		if ((tmp & 0x30) != 0x00)
+			break; /* GPIO3x is not enable */
+
+		err = 0; /* GPIO3x set is all enabled */
+		break;
+	case 4:
+		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(0), 0);
+		tmp = superio_inb(base, F81866_MULTI_FUN3_REG);
+		if ((tmp & 0xc0) != 0x00)
+			break; /* GPIO4x is not enable */
+
+		err = 0; /* GPIO4x set is all enabled */
+		break;
+	case 5:
+		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2),
+				0);
+		tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
+		if ((tmp & 0x43) != 0x40)
+			break; /* GPIO5x is not enable */
+
+		err = 0; /* GPIO5x set is all enabled */
+		break;
+	case 6:
+		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
+				BIT(0), 0);
+		tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
+		if ((tmp & 0x4c) != 0x40)
+			break; /* GPIO60~64 is not enable */
+
+		tmp = superio_inb(base, F81866_MULTI_FUN4_REG);
+		if ((tmp & 0xe0) != 0xe0)
+			break; /* GPIO65~67 is not enable */
+
+		err = 0; /* GPIO6x set is all enabled */
+		break;
+	case 7:
+		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
+				BIT(0), 0);
+		tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
+		if ((tmp & 0x20) != 0x20)
+			break; /* GPIO7x is not enable */
+
+		tmp = superio_inb(base, F81866_MULTI_FUN4_REG);
+		if ((tmp & 0x01) != 0x00)
+			break; /* GPIO70 is not enable */
+
+		err = 0; /* GPIO7x set is all enabled */
+		break;
+	case 8:
+		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2),
+				0);
+		tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
+		if ((tmp & 0x20) != 0x20)
+			break; /* GPIO8x is not enable */
+
+		err = 0; /* GPIO8x set is all enabled */
+		break;
+	default:
+		break;
+	}
+
+	superio_exit(base);
+	return err;
+}
+
 /*
  * Platform device and driver.
  */
@@ -351,6 +485,15 @@ static int f7188x_gpio_probe(struct platform_device *pdev)
 	for (i = 0; i < data->nr_bank; i++) {
 		struct f7188x_gpio_bank *bank = &data->bank[i];
 
+		/*
+		 * Dont export GPIO sysfs if pin set is not enable by MB
+		 * manufacturer.
+		 */
+		if (sio->type == f81866 && f81866_verify_gpioset(sio->addr, i))
+			continue;
+
+		dev_dbg(&pdev->dev, "%s: register GPIO%xx set\n", __func__,
+				bank->chip.base >> 4);
 		bank->chip.dev = &pdev->dev;
 		bank->data = data;
 
@@ -368,6 +511,11 @@ static int f7188x_gpio_probe(struct platform_device *pdev)
 err_gpiochip:
 	for (i = i - 1; i >= 0; i--) {
 		struct f7188x_gpio_bank *bank = &data->bank[i];
+
+		/* Some GPIO is not export, not need to remove */
+		if (!bank->data || !bank->chip.dev)
+			continue;
+
 		gpiochip_remove(&bank->chip);
 	}
 
@@ -381,6 +529,11 @@ static int f7188x_gpio_remove(struct platform_device *pdev)
 
 	for (i = 0; i < data->nr_bank; i++) {
 		struct f7188x_gpio_bank *bank = &data->bank[i];
+
+		/* Some GPIO is not export, not need to remove */
+		if (!bank->data || !bank->chip.dev)
+			continue;
+
 		gpiochip_remove(&bank->chip);
 	}
 
-- 
1.9.1

Re: [PATCH 2/2] gpio-f7188x: filter non-export gpio for F81866

[Andy Shevchenko]

On Tue, Jan 12, 2016 at 9:41 AM, Peter Hung <hpeter@gmail.com> wrote:
> Dont export gpios which not enabled by motherboard manufacturer.

'Do not' or 'Don't'.

Perhaps 'GPIOs'.

'which are not enabled'

> +#define F81866_PORT_SEL_REG    0x27
> +#define F81866_MULTI_FUN1_REG  0x28
> +#define F81866_MULTI_FUN3_REG  0x29
> +#define F81866_MULTI_FUN4_REG  0x2B
> +#define F81866_GPIO_EN_REG     0x2C
> +

Move this before IDs block.

Use same prefix SIO_ and drop _REG, e.g.
SIO_F81866_PORT_SEL

FUN -> FN or FUNC (Personally I prefer shorter)

>  enum chips { f71869, f71869a, f71882fg, f71889f, f81866 };
>
>  static const char * const f7188x_names[] = {
> @@ -93,6 +99,15 @@ static inline void superio_outb(int base, int reg, int val)
>         outb(val, base + 1);
>  }
>
> +static inline void superio_mask_outb(int base, int reg, int mask, int val)

Usually we named such as _update or _update_bits.
superio_update()

> +{
> +       u8 tmp;
> +
> +       tmp = superio_inb(base, reg);
> +       tmp = (tmp & ~mask) | (val & mask);
> +       superio_outb(base, reg, tmp);
> +}
> +
>  static inline int superio_enter(int base)
>  {
>         /* Don't step on other drivers' I/O space by accident. */
> @@ -304,6 +319,125 @@ static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>         superio_exit(sio->addr);
>  }
>
> +static int f81866_verify_gpioset(int base, int set)

…_gpio_verify() looks more suitable to the existing scheme.

> +{
> +       int err;
> +       u8 tmp;
> +
> +       err = superio_enter(base);
> +       if (err)
> +               return err;
> +
> +       err = -ENODEV;
> +
> +       switch (set) {
> +       case 0:
> +               superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
> +                               BIT(0), 0);
> +               tmp = superio_inb(base, F81866_GPIO_EN_REG);
> +               if ((tmp & 0x1f) != 0x1f)

Magic number.

> +                       break; /* one in GPIO00~GPIO04 is not enable */
> +
> +               tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
> +               if ((tmp & 0xc) != 0x0)

Magic and no need to compare to != 0 explicitly.

> +                       break;
> +
> +               err = 0; /* GPIO0x set is all enabled */

Maybe you can refactor this in better way.
Assign set of values in switch-case and do check after, only once.

Same comments are applicable to other cases below.

> +               break;
> +       case 1:
> +               superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
> +                               BIT(0), BIT(2));
> +               tmp = superio_inb(base, F81866_GPIO_EN_REG);
> +               if ((tmp & 0xef) != 0xef)
> +                       break; /* one in GPIO10~GPIO17 is not enable */
> +
> +               tmp = superio_inb(base, F81866_MULTI_FUN3_REG);
> +               if ((tmp & 0x03) != 0x00)
> +                       break; /* one in GPIO12~GPIO13 is not enable */
> +
> +               err = 0; /* GPIO1x set is all enabled */
> +               break;
> +       case 2:
> +               superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
> +                               BIT(0), BIT(3));
> +               tmp = superio_inb(base, F81866_GPIO_EN_REG);
> +               if ((tmp & 0xff) != 0xff)
> +                       break; /* one in GPIO20~GPIO27 is not enable */
> +
> +               tmp = superio_inb(base, F81866_MULTI_FUN3_REG);
> +               if ((tmp & 0x08) != 0x00)
> +                       break; /* GPIO20 is not enable */
> +
> +               err = 0; /* GPIO2x set is all enabled */
> +               break;
> +       case 3:
> +               superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(0), 0);
> +               tmp = superio_inb(base, F81866_MULTI_FUN3_REG);
> +               if ((tmp & 0x30) != 0x00)
> +                       break; /* GPIO3x is not enable */
> +
> +               err = 0; /* GPIO3x set is all enabled */
> +               break;
> +       case 4:
> +               superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(0), 0);
> +               tmp = superio_inb(base, F81866_MULTI_FUN3_REG);
> +               if ((tmp & 0xc0) != 0x00)
> +                       break; /* GPIO4x is not enable */
> +
> +               err = 0; /* GPIO4x set is all enabled */
> +               break;
> +       case 5:
> +               superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2),
> +                               0);
> +               tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
> +               if ((tmp & 0x43) != 0x40)
> +                       break; /* GPIO5x is not enable */
> +
> +               err = 0; /* GPIO5x set is all enabled */
> +               break;
> +       case 6:
> +               superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
> +                               BIT(0), 0);
> +               tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
> +               if ((tmp & 0x4c) != 0x40)
> +                       break; /* GPIO60~64 is not enable */
> +
> +               tmp = superio_inb(base, F81866_MULTI_FUN4_REG);
> +               if ((tmp & 0xe0) != 0xe0)
> +                       break; /* GPIO65~67 is not enable */
> +
> +               err = 0; /* GPIO6x set is all enabled */
> +               break;
> +       case 7:
> +               superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
> +                               BIT(0), 0);
> +               tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
> +               if ((tmp & 0x20) != 0x20)
> +                       break; /* GPIO7x is not enable */
> +
> +               tmp = superio_inb(base, F81866_MULTI_FUN4_REG);
> +               if ((tmp & 0x01) != 0x00)
> +                       break; /* GPIO70 is not enable */
> +
> +               err = 0; /* GPIO7x set is all enabled */
> +               break;
> +       case 8:
> +               superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2),
> +                               0);
> +               tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
> +               if ((tmp & 0x20) != 0x20)
> +                       break; /* GPIO8x is not enable */
> +
> +               err = 0; /* GPIO8x set is all enabled */
> +               break;
> +       default:
> +               break;
> +       }
> +
> +       superio_exit(base);
> +       return err;
> +}
> +
>  /*
>   * Platform device and driver.
>   */
> @@ -351,6 +485,15 @@ static int f7188x_gpio_probe(struct platform_device *pdev)
>         for (i = 0; i < data->nr_bank; i++) {
>                 struct f7188x_gpio_bank *bank = &data->bank[i];
>
> +               /*
> +                * Dont export GPIO sysfs if pin set is not enable by MB

Don't

> +                * manufacturer.
> +                */
> +               if (sio->type == f81866 && f81866_verify_gpioset(sio->addr, i))
> +                       continue;
> +
> +               dev_dbg(&pdev->dev, "%s: register GPIO%xx set\n", __func__,

%x ?

> +                               bank->chip.base >> 4);
>                 bank->chip.dev = &pdev->dev;
>                 bank->data = data;
>
> @@ -368,6 +511,11 @@ static int f7188x_gpio_probe(struct platform_device *pdev)
>  err_gpiochip:
>         for (i = i - 1; i >= 0; i--) {
>                 struct f7188x_gpio_bank *bank = &data->bank[i];
> +
> +               /* Some GPIO is not export, not need to remove */
> +               if (!bank->data || !bank->chip.dev)
> +                       continue;
> +
>                 gpiochip_remove(&bank->chip);
>         }
>
> @@ -381,6 +529,11 @@ static int f7188x_gpio_remove(struct platform_device *pdev)
>
>         for (i = 0; i < data->nr_bank; i++) {
>                 struct f7188x_gpio_bank *bank = &data->bank[i];
> +
> +               /* Some GPIO is not export, not need to remove */
> +               if (!bank->data || !bank->chip.dev)
> +                       continue;
> +
>                 gpiochip_remove(&bank->chip);



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio-f7188x: filter non-export gpio for F81866

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 6127 bytes --]

On Tue, Jan 12, 2016 at 03:41:39PM +0800, Peter Hung wrote:
> Dont export gpios which not enabled by motherboard manufacturer.
> 
> Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>

Hi Peter,

> ---
>  drivers/gpio/gpio-f7188x.c | 153 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 153 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
> index a6a9641..f7fe7aa 100644
> --- a/drivers/gpio/gpio-f7188x.c
> +++ b/drivers/gpio/gpio-f7188x.c
> @@ -38,6 +38,12 @@
>  #define SIO_F71889_ID		0x0909	/* F71889 chipset ID */
>  #define SIO_F81866_ID		0x1010	/* F81866 chipset ID */
>  
> +#define F81866_PORT_SEL_REG	0x27
> +#define F81866_MULTI_FUN1_REG	0x28
> +#define F81866_MULTI_FUN3_REG	0x29
> +#define F81866_MULTI_FUN4_REG	0x2B
> +#define F81866_GPIO_EN_REG	0x2C
> +
>  enum chips { f71869, f71869a, f71882fg, f71889f, f81866 };
>  
>  static const char * const f7188x_names[] = {
> @@ -93,6 +99,15 @@ static inline void superio_outb(int base, int reg, int val)
>  	outb(val, base + 1);
>  }
>  
> +static inline void superio_mask_outb(int base, int reg, int mask, int val)
> +{
> +	u8 tmp;
> +
> +	tmp = superio_inb(base, reg);
> +	tmp = (tmp & ~mask) | (val & mask);
> +	superio_outb(base, reg, tmp);
> +}
> +
>  static inline int superio_enter(int base)
>  {
>  	/* Don't step on other drivers' I/O space by accident. */
> @@ -304,6 +319,125 @@ static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>  	superio_exit(sio->addr);
>  }
>  
> +static int f81866_verify_gpioset(int base, int set)
> +{
> +	int err;
> +	u8 tmp;
> +
> +	err = superio_enter(base);
> +	if (err)
> +		return err;
> +
> +	err = -ENODEV;
> +
> +	switch (set) {
> +	case 0:
> +		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
> +				BIT(0), 0);
> +		tmp = superio_inb(base, F81866_GPIO_EN_REG);
> +		if ((tmp & 0x1f) != 0x1f)
> +			break; /* one in GPIO00~GPIO04 is not enable */
> +
> +		tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
> +		if ((tmp & 0xc) != 0x0)
> +			break;
> +
> +		err = 0; /* GPIO0x set is all enabled */
> +		break;
> +	case 1:
> +		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
> +				BIT(0), BIT(2));
> +		tmp = superio_inb(base, F81866_GPIO_EN_REG);
> +		if ((tmp & 0xef) != 0xef)
> +			break; /* one in GPIO10~GPIO17 is not enable */
> +
> +		tmp = superio_inb(base, F81866_MULTI_FUN3_REG);
> +		if ((tmp & 0x03) != 0x00)
> +			break; /* one in GPIO12~GPIO13 is not enable */
> +
> +		err = 0; /* GPIO1x set is all enabled */
> +		break;
> +	case 2:
> +		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
> +				BIT(0), BIT(3));
> +		tmp = superio_inb(base, F81866_GPIO_EN_REG);
> +		if ((tmp & 0xff) != 0xff)
> +			break; /* one in GPIO20~GPIO27 is not enable */
> +
> +		tmp = superio_inb(base, F81866_MULTI_FUN3_REG);
> +		if ((tmp & 0x08) != 0x00)
> +			break; /* GPIO20 is not enable */
> +
> +		err = 0; /* GPIO2x set is all enabled */
> +		break;
> +	case 3:
> +		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(0), 0);
> +		tmp = superio_inb(base, F81866_MULTI_FUN3_REG);
> +		if ((tmp & 0x30) != 0x00)
> +			break; /* GPIO3x is not enable */
> +
> +		err = 0; /* GPIO3x set is all enabled */
> +		break;
> +	case 4:
> +		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(0), 0);
> +		tmp = superio_inb(base, F81866_MULTI_FUN3_REG);
> +		if ((tmp & 0xc0) != 0x00)
> +			break; /* GPIO4x is not enable */
> +
> +		err = 0; /* GPIO4x set is all enabled */
> +		break;
> +	case 5:
> +		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2),
> +				0);
> +		tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
> +		if ((tmp & 0x43) != 0x40)
> +			break; /* GPIO5x is not enable */
> +
> +		err = 0; /* GPIO5x set is all enabled */
> +		break;
> +	case 6:
> +		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
> +				BIT(0), 0);
> +		tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
> +		if ((tmp & 0x4c) != 0x40)
> +			break; /* GPIO60~64 is not enable */
> +
> +		tmp = superio_inb(base, F81866_MULTI_FUN4_REG);
> +		if ((tmp & 0xe0) != 0xe0)
> +			break; /* GPIO65~67 is not enable */
> +
> +		err = 0; /* GPIO6x set is all enabled */
> +		break;
> +	case 7:
> +		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2) |
> +				BIT(0), 0);
> +		tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
> +		if ((tmp & 0x20) != 0x20)
> +			break; /* GPIO7x is not enable */
> +
> +		tmp = superio_inb(base, F81866_MULTI_FUN4_REG);
> +		if ((tmp & 0x01) != 0x00)
> +			break; /* GPIO70 is not enable */
> +
> +		err = 0; /* GPIO7x set is all enabled */
> +		break;
> +	case 8:
> +		superio_mask_outb(base, F81866_PORT_SEL_REG, BIT(3) | BIT(2),
> +				0);
> +		tmp = superio_inb(base, F81866_MULTI_FUN1_REG);
> +		if ((tmp & 0x20) != 0x20)
> +			break; /* GPIO8x is not enable */
> +
> +		err = 0; /* GPIO8x set is all enabled */
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	superio_exit(base);
> +	return err;
> +}
> +
>  /*
>   * Platform device and driver.
>   */
> @@ -351,6 +485,15 @@ static int f7188x_gpio_probe(struct platform_device *pdev)
>  	for (i = 0; i < data->nr_bank; i++) {
>  		struct f7188x_gpio_bank *bank = &data->bank[i];
>  
> +		/*
> +		 * Dont export GPIO sysfs if pin set is not enable by MB
> +		 * manufacturer.

What does MB stands for ?

> +		 */
> +		if (sio->type == f81866 && f81866_verify_gpioset(sio->addr, i))
> +			continue;

This whole filtering mechanism relies on the fact that the multiplexing
configuration has been correctly applied by the BIOS (if applied at
all). But I wonder if it is often the case. For example, I have several
boards for which it is not true. And to make the GPIOs available, I need
first to fix the multiplexing pin configuration of the Super I/O.

Maybe it would be more correct to rely on the hardware description of a
board (Device Tree or ACPI) to decide which GPIO bank can be enabled or
not.

Simon

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/2] gpio-f7188x: filter non-export gpio for F81866

[Peter Hung]

Hi Simon,

>> @@ -351,6 +485,15 @@ static int f7188x_gpio_probe(struct platform_device *pdev)
>>   	for (i = 0; i < data->nr_bank; i++) {
>>   		struct f7188x_gpio_bank *bank = &data->bank[i];
>>
>> +		/*
>> +		 * Dont export GPIO sysfs if pin set is not enable by MB
>> +		 * manufacturer.
>
> What does MB stands for ?

The MB is stands for "Motherboard", Due to the limit of 80 words length,
I use the abbreviation.

>> +		 */
>> +		if (sio->type == f81866 && f81866_verify_gpioset(sio->addr, i))
>> +			continue;
>
> This whole filtering mechanism relies on the fact that the multiplexing
> configuration has been correctly applied by the BIOS (if applied at
> all). But I wonder if it is often the case. For example, I have several
> boards for which it is not true. And to make the GPIOs available, I need
> first to fix the multiplexing pin configuration of the Super I/O.
>
> Maybe it would be more correct to rely on the hardware description of a
> board (Device Tree or ACPI) to decide which GPIO bank can be enabled or
> not.

That's good for control by Device Tree or ACPI.

IMO, we shouldn't export GPIOs not enabled if the BIOS had written
wrong configuration to SuperIO, but it's only my opinion. Should I
do filter for this?

Thanks for your advices.
-- 
With Best Regards,
Peter Hung

Re: [PATCH 2/2] gpio-f7188x: filter non-export gpio for F81866

[Peter Hung]

Hi Andy,

Andy Shevchenko 於 2016/1/12 下午 05:33 寫道:
> On Tue, Jan 12, 2016 at 9:41 AM, Peter Hung <hpeter@gmail.com> wrote:
>> Dont export gpios which not enabled by motherboard manufacturer.
>
> 'Do not' or 'Don't'.
>
> Perhaps 'GPIOs'.
>
> 'which are not enabled'

OK.

>> +#define F81866_PORT_SEL_REG    0x27
>> +#define F81866_MULTI_FUN1_REG  0x28
>> +#define F81866_MULTI_FUN3_REG  0x29
>> +#define F81866_MULTI_FUN4_REG  0x2B
>> +#define F81866_GPIO_EN_REG     0x2C
>> +
>
> Move this before IDs block.

OK.


>> +static inline void superio_mask_outb(int base, int reg, int mask, int val)
>
> Usually we named such as _update or _update_bits.
> superio_update()

OK

>> +static int f81866_verify_gpioset(int base, int set)
>
> …_gpio_verify() looks more suitable to the existing scheme.

OK

>> +                * manufacturer.
>> +                */
>> +               if (sio->type == f81866 && f81866_verify_gpioset(sio->addr, i))
>> +                       continue;
>> +
>> +               dev_dbg(&pdev->dev, "%s: register GPIO%xx set\n", __func__,
>
> %x ?

Sorry for misunderstand. This section will print debug message as GPIO3x
set if GPIO3 set enabled, so I used %xx.

We are discussing about the patch with maintainer. It maybe drop if
don't need filter.

Thanks for your advices.
-- 
With Best Regards,
Peter Hung
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] gpio-f7188x: filter non-export gpio for F81866

[Peter Hung]

Hi Andy,

Andy Shevchenko 於 2016/1/12 下午 05:33 寫道:
> On Tue, Jan 12, 2016 at 9:41 AM, Peter Hung <hpeter@gmail.com> wrote:
>> Dont export gpios which not enabled by motherboard manufacturer.
>
> 'Do not' or 'Don't'.
>
> Perhaps 'GPIOs'.
>
> 'which are not enabled'

OK.

>> +#define F81866_PORT_SEL_REG    0x27
>> +#define F81866_MULTI_FUN1_REG  0x28
>> +#define F81866_MULTI_FUN3_REG  0x29
>> +#define F81866_MULTI_FUN4_REG  0x2B
>> +#define F81866_GPIO_EN_REG     0x2C
>> +
>
> Move this before IDs block.

OK.


>> +static inline void superio_mask_outb(int base, int reg, int mask, int val)
>
> Usually we named such as _update or _update_bits.
> superio_update()

OK

>> +static int f81866_verify_gpioset(int base, int set)
>
> …_gpio_verify() looks more suitable to the existing scheme.

OK

>> +                * manufacturer.
>> +                */
>> +               if (sio->type == f81866 && f81866_verify_gpioset(sio->addr, i))
>> +                       continue;
>> +
>> +               dev_dbg(&pdev->dev, "%s: register GPIO%xx set\n", __func__,
>
> %x ?

Sorry for misunderstand. This section will print debug message as GPIO3x
set if GPIO3 set enabled, so I used %xx.

We are discussing about the patch with maintainer. It maybe drop if
don't need filter.

Thanks for your advices.
-- 
With Best Regards,
Peter Hung