Re: [PATCH v4] gpio: winbond: add driver

[Maciej S. Szmigiero]

On 05.01.2018 14:35, Andy Shevchenko wrote:
> On Thu, 2018-01-04 at 22:46 +0100, Maciej S. Szmigiero wrote:
>> This commit adds GPIO driver for Winbond Super I/Os.
>>
>> Currently, only W83627UHG model (also known as Nuvoton NCT6627UD) is
>> supported but in the future a support for other Winbond models, too,
>> can
>> be added to the driver.
>>
>> A module parameter "gpios" sets a bitmask of GPIO ports to enable (bit
>> 0 is
>> GPIO1, bit 1 is GPIO2, etc.).
>> One should be careful which ports one tinkers with since some might be
>> managed by the firmware (for functions like powering on and off,
>> sleeping,
>> BIOS recovery, etc.) and some of GPIO port pins are physically shared
>> with
>> other devices included in the Super I/O chip.
>>
> 
> I still have an impression that it could be MFD based driver will all
> infrastructure needed for the actual devices behind, though here is an
> objection from the author, so, I leave it to maintainers.
> 
> Besides that I don't like global variables in so many amount (perhaps
> custom struct gpio_w83227uhg {...};  static struct ... *...; might
> improve this.
> 
> And one nit below.
> 
> So, there is room for improvement for my opinion, anyway, FWIW,
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

Thanks,
will implement your comments about grouping global parameters in
a single static struct and about __clear_bit in a respin.

Maciej

>> Signed-off-by: Maciej S. Szmigiero <mail@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>> Changes from v1:
>> * Added SPDX license tag,
>>
>> * Removed gpiobase parameter,
>>
>> * Changed uint{8,16}_t types to u{8,16},
>>
>> * Added kerneldoc descriptions of driver structures,
>>
>> * Reformatted winbond_gpio_infos array fields so they are on separate
>> lines,
>>
>> * Added few comments here and there as requested,
>>
>> * Moved port configuration code from separate
>> winbond_gpio_configure_X()
>> functions to one, common, parametrized winbond_gpio_configure_port()
>> function.
>>
>> Changes from v2:
>> * Adapted the driver to use the ISA bus instead of the platform one,
>>
>> * Reformatted register and register bit macros,
>>
>> * Added wb_sio_{notice,warn,err} macros to not repeat pr_* calls with
>> the same prefix again and again,
>>
>> * Added a comment why a double I/O register write is needed,
>>
>> * Converted the code to use for_each_set_bit() where it could be used,
>>
>> * Removed a warning about an impossible condition in
>> winbond_gpio_get_info(),
>>
>> * Made winbond_gpio_get_info() return a reduced GPIO number,
>>
>> * Renamed gpiolib callback functions parameter name from gpio_num to
>> offset,
>>
>> * Changed I/O port number variables to unsigned long type,
>>
>> * Removed explicit casts in logging calls,
>>
>> * hweight() is now used for calculating the total GPIO count.
>>
>> Changes from v3:
>> * Changed the code to use pr_fmt(x) instead of
>> wb_sio_{notice,warn,err}
>> logging macros,
>>
>> * Made the driver warn about but accept extra bits in the "gpios"
>> parameter,
>>
>> * Removed a one case of a redundant 'else',
>>
>> * Swapped a one missed pair of register bit macros,
>>
>> * Changed the code to pass a size in bits, not bytes, to
>> for_each_set_bit() macros.
>>
>>  drivers/gpio/Kconfig        |  16 +
>>  drivers/gpio/Makefile       |   1 +
>>  drivers/gpio/gpio-winbond.c | 723
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 740 insertions(+)
>>  create mode 100644 drivers/gpio/gpio-winbond.c
>>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index 395669bfcc26..8c4018855c3b 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -698,6 +698,22 @@ config GPIO_TS5500
>>  	  blocks of the TS-5500: DIO1, DIO2 and the LCD port, and the
>> TS-5600
>>  	  LCD port.
>>  
>> +config GPIO_WINBOND
>> +	tristate "Winbond Super I/O GPIO support"
>> +	select ISA_BUS_API
>> +	help
>> +	  This option enables support for GPIOs found on Winbond
>> Super I/O
>> +	  chips.
>> +	  Currently, only W83627UHG (also known as Nuvoton NCT6627UD)
>> is
>> +	  supported.
>> +
>> +	  You will need to provide a module parameter "gpios", or a
>> +	  boot-time parameter "gpio_winbond.gpios" with a bitmask of
>> GPIO
>> +	  ports to enable (bit 0 is GPIO1, bit 1 is GPIO2, etc.).
>> +
>> +	  To compile this driver as a module, choose M here: the
>> module will
>> +	  be called gpio-winbond.
>> +
>>  config GPIO_WS16C48
>>  	tristate "WinSystems WS16C48 GPIO support"
>>  	depends on ISA_BUS_API
>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>> index bc5dd673fa11..ff3d36d0a443 100644
>> --- a/drivers/gpio/Makefile
>> +++ b/drivers/gpio/Makefile
>> @@ -139,6 +139,7 @@ obj-$(CONFIG_GPIO_VIPERBOARD)	+= gpio-
>> viperboard.o
>>  obj-$(CONFIG_GPIO_VR41XX)	+= gpio-vr41xx.o
>>  obj-$(CONFIG_GPIO_VX855)	+= gpio-vx855.o
>>  obj-$(CONFIG_GPIO_WHISKEY_COVE)	+= gpio-wcove.o
>> +obj-$(CONFIG_GPIO_WINBOND)	+= gpio-winbond.o
>>  obj-$(CONFIG_GPIO_WM831X)	+= gpio-wm831x.o
>>  obj-$(CONFIG_GPIO_WM8350)	+= gpio-wm8350.o
>>  obj-$(CONFIG_GPIO_WM8994)	+= gpio-wm8994.o
>> diff --git a/drivers/gpio/gpio-winbond.c b/drivers/gpio/gpio-winbond.c
>> new file mode 100644
>> index 000000000000..a06a47f3dcdf
>> --- /dev/null
>> +++ b/drivers/gpio/gpio-winbond.c
>> @@ -0,0 +1,723 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * GPIO interface for Winbond Super I/O chips
>> + * Currently, only W83627UHG (Nuvoton NCT6627UD) is supported.
>> + *
>> + * Author: Maciej S. Szmigiero <mail@xxxxxxxxxxxxxxxxxxxxx>
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/gpio/driver.h>
>> +#include <linux/ioport.h>
>> +#include <linux/isa.h>
>> +#include <linux/module.h>
>> +
>> +#define WB_GPIO_DRIVER_NAME		KBUILD_MODNAME
>> +
>> +#define WB_SIO_BASE			0x2e
>> +#define WB_SIO_BASE_HIGH		0x4e
>> +
>> +#define WB_SIO_EXT_ENTER_KEY		0x87
>> +#define WB_SIO_EXT_EXIT_KEY		0xaa
>> +
>> +/* global chip registers */
>> +
>> +#define WB_SIO_REG_LOGICAL		0x07
>> +
>> +#define WB_SIO_REG_CHIP_MSB		0x20
>> +#define WB_SIO_REG_CHIP_LSB		0x21
>> +
>> +#define WB_SIO_CHIP_ID_W83627UHG	0xa230
>> +#define WB_SIO_CHIP_ID_W83627UHG_MASK	GENMASK(15, 4)
>> +
>> +#define WB_SIO_REG_DPD			0x22
>> +#define WB_SIO_REG_DPD_UARTA		4
>> +#define WB_SIO_REG_DPD_UARTB		5
>> +
>> +#define WB_SIO_REG_IDPD		0x23
>> +#define WB_SIO_REG_IDPD_UARTC		4
>> +#define WB_SIO_REG_IDPD_UARTD		5
>> +#define WB_SIO_REG_IDPD_UARTE		6
>> +#define WB_SIO_REG_IDPD_UARTF		7
>> +
>> +#define WB_SIO_REG_GLOBAL_OPT		0x24
>> +#define WB_SIO_REG_GO_ENFDC		1
>> +
>> +#define WB_SIO_REG_OVTGPIO3456		0x29
>> +#define WB_SIO_REG_OG3456_G3PP		3
>> +#define WB_SIO_REG_OG3456_G4PP		4
>> +#define WB_SIO_REG_OG3456_G5PP		5
>> +#define WB_SIO_REG_OG3456_G6PP		7
>> +
>> +#define WB_SIO_REG_I2C_PS		0x2a
>> +#define WB_SIO_REG_I2CPS_I2CFS		1
>> +
>> +#define WB_SIO_REG_GPIO1_MF		0x2c
>> +#define WB_SIO_REG_G1MF_G1PP		6
>> +#define WB_SIO_REG_G1MF_G2PP		7
>> +#define WB_SIO_REG_G1MF_FS_MASK	GENMASK(1, 0)
>> +#define WB_SIO_REG_G1MF_FS_IR_OFF	0
>> +#define WB_SIO_REG_G1MF_FS_IR		1
>> +#define WB_SIO_REG_G1MF_FS_GPIO1	2
>> +#define WB_SIO_REG_G1MF_FS_UARTB	3
>> +
>> +/* not an actual device number, just a value meaning 'no device' */
>> +#define WB_SIO_DEV_NONE		0xff
>> +
>> +/* registers with offsets >= 0x30 are specific for a particular
>> device */
>> +
>> +/* UART B logical device */
>> +#define WB_SIO_DEV_UARTB		0x03
>> +#define WB_SIO_UARTB_REG_ENABLE	0x30
>> +#define WB_SIO_UARTB_ENABLE_ON		0
>> +
>> +/* UART C logical device */
>> +#define WB_SIO_DEV_UARTC		0x06
>> +#define WB_SIO_UARTC_REG_ENABLE	0x30
>> +#define WB_SIO_UARTC_ENABLE_ON		0
>> +
>> +/* GPIO3, GPIO4 logical device */
>> +#define WB_SIO_DEV_GPIO34		0x07
>> +#define WB_SIO_GPIO34_REG_ENABLE	0x30
>> +#define WB_SIO_GPIO34_ENABLE_3		0
>> +#define WB_SIO_GPIO34_ENABLE_4		1
>> +#define WB_SIO_GPIO34_REG_IO3		0xe0
>> +#define WB_SIO_GPIO34_REG_DATA3	0xe1
>> +#define WB_SIO_GPIO34_REG_INV3		0xe2
>> +#define WB_SIO_GPIO34_REG_IO4		0xe4
>> +#define WB_SIO_GPIO34_REG_DATA4	0xe5
>> +#define WB_SIO_GPIO34_REG_INV4		0xe6
>> +
>> +/* WDTO, PLED, GPIO5, GPIO6 logical device */
>> +#define WB_SIO_DEV_WDGPIO56		0x08
>> +#define WB_SIO_WDGPIO56_REG_ENABLE	0x30
>> +#define WB_SIO_WDGPIO56_ENABLE_5	1
>> +#define WB_SIO_WDGPIO56_ENABLE_6	2
>> +#define WB_SIO_WDGPIO56_REG_IO5	0xe0
>> +#define WB_SIO_WDGPIO56_REG_DATA5	0xe1
>> +#define WB_SIO_WDGPIO56_REG_INV5	0xe2
>> +#define WB_SIO_WDGPIO56_REG_IO6	0xe4
>> +#define WB_SIO_WDGPIO56_REG_DATA6	0xe5
>> +#define WB_SIO_WDGPIO56_REG_INV6	0xe6
>> +
>> +/* GPIO1, GPIO2, SUSLED logical device */
>> +#define WB_SIO_DEV_GPIO12		0x09
>> +#define WB_SIO_GPIO12_REG_ENABLE	0x30
>> +#define WB_SIO_GPIO12_ENABLE_1		0
>> +#define WB_SIO_GPIO12_ENABLE_2		1
>> +#define WB_SIO_GPIO12_REG_IO1		0xe0
>> +#define WB_SIO_GPIO12_REG_DATA1	0xe1
>> +#define WB_SIO_GPIO12_REG_INV1		0xe2
>> +#define WB_SIO_GPIO12_REG_IO2		0xe4
>> +#define WB_SIO_GPIO12_REG_DATA2	0xe5
>> +#define WB_SIO_GPIO12_REG_INV2		0xe6
>> +
>> +/* UART D logical device */
>> +#define WB_SIO_DEV_UARTD		0x0d
>> +#define WB_SIO_UARTD_REG_ENABLE	0x30
>> +#define WB_SIO_UARTD_ENABLE_ON		0
>> +
>> +/* UART E logical device */
>> +#define WB_SIO_DEV_UARTE		0x0e
>> +#define WB_SIO_UARTE_REG_ENABLE	0x30
>> +#define WB_SIO_UARTE_ENABLE_ON		0
>> +
>> +static unsigned long baseparam;
>> +static unsigned long gpios;
>> +static unsigned long ppgpios;
>> +static unsigned long odgpios;
>> +static bool pledgpio;
>> +static bool beepgpio;
>> +static bool i2cgpio;
>> +
>> +static int winbond_sio_enter(unsigned long base)
>> +{
>> +	if (!request_muxed_region(base, 2, WB_GPIO_DRIVER_NAME))
>> +		return -EBUSY;
>> +
>> +	/*
>> +	 * datasheet says two successive writes of the "key" value
>> are needed
>> +	 * in order for chip to enter the "Extended Function Mode"
>> +	 */
>> +	outb(WB_SIO_EXT_ENTER_KEY, base);
>> +	outb(WB_SIO_EXT_ENTER_KEY, base);
>> +
>> +	return 0;
>> +}
>> +
>> +static void winbond_sio_select_logical(unsigned long base, u8 dev)
>> +{
>> +	outb(WB_SIO_REG_LOGICAL, base);
>> +	outb(dev, base + 1);
>> +}
>> +
>> +static void winbond_sio_leave(unsigned long base)
>> +{
>> +	outb(WB_SIO_EXT_EXIT_KEY, base);
>> +
>> +	release_region(base, 2);
>> +}
>> +
>> +static void winbond_sio_reg_write(unsigned long base, u8 reg, u8
>> data)
>> +{
>> +	outb(reg, base);
>> +	outb(data, base + 1);
>> +}
>> +
>> +static u8 winbond_sio_reg_read(unsigned long base, u8 reg)
>> +{
>> +	outb(reg, base);
>> +	return inb(base + 1);
>> +}
>> +
>> +static void winbond_sio_reg_bset(unsigned long base, u8 reg, u8 bit)
>> +{
>> +	u8 val;
>> +
>> +	val = winbond_sio_reg_read(base, reg);
>> +	val |= BIT(bit);
>> +	winbond_sio_reg_write(base, reg, val);
>> +}
>> +
>> +static void winbond_sio_reg_bclear(unsigned long base, u8 reg, u8
>> bit)
>> +{
>> +	u8 val;
>> +
>> +	val = winbond_sio_reg_read(base, reg);
>> +	val &= ~BIT(bit);
>> +	winbond_sio_reg_write(base, reg, val);
>> +}
>> +
>> +static bool winbond_sio_reg_btest(unsigned long base, u8 reg, u8 bit)
>> +{
>> +	return winbond_sio_reg_read(base, reg) & BIT(bit);
>> +}
>> +
>> +/**
>> + * struct winbond_gpio_port_conflict - possibly conflicting device
>> information
>> + * @name:	device name (NULL means no conflicting device
>> defined)
>> + * @dev:	Super I/O logical device number where the testreg
>> register
>> + *		is located (or WB_SIO_DEV_NONE - don't select any
>> + *		logical device)
>> + * @testreg:	register number where the testbit bit is located
>> + * @testbit:	index of a bit to check whether an actual
>> conflict exists
>> + * @warnonly:	if set then a conflict isn't fatal (just warn
>> about it),
>> + *		otherwise disable the particular GPIO port if a
>> conflict
>> + *		is detected
>> + */
>> +struct winbond_gpio_port_conflict {
>> +	const char *name;
>> +	u8 dev;
>> +	u8 testreg;
>> +	u8 testbit;
>> +	bool warnonly;
>> +};
>> +
>> +/**
>> + * struct winbond_gpio_info - information about a particular GPIO
>> port (device)
>> + * @dev:		Super I/O logical device number of the
>> registers
>> + *			specified below
>> + * @enablereg:		port enable bit register number
>> + * @enablebit:		index of a port enable bit
>> + * @outputreg:		output driver mode bit register number
>> + * @outputppbit:	index of a push-pull output driver mode bit
>> + * @ioreg:		data direction register number
>> + * @invreg:		pin data inversion register number
>> + * @datareg:		pin data register number
>> + * @conflict:		description of a device that possibly
>> conflicts with
>> + *			this port
>> + */
>> +struct winbond_gpio_info {
>> +	u8 dev;
>> +	u8 enablereg;
>> +	u8 enablebit;
>> +	u8 outputreg;
>> +	u8 outputppbit;
>> +	u8 ioreg;
>> +	u8 invreg;
>> +	u8 datareg;
>> +	struct winbond_gpio_port_conflict conflict;
>> +};
>> +
>> +static const struct winbond_gpio_info winbond_gpio_infos[6] = {
>> +	{ /* 0 */
>> +		.dev = WB_SIO_DEV_GPIO12,
>> +		.enablereg = WB_SIO_GPIO12_REG_ENABLE,
>> +		.enablebit = WB_SIO_GPIO12_ENABLE_1,
>> +		.outputreg = WB_SIO_REG_GPIO1_MF,
>> +		.outputppbit = WB_SIO_REG_G1MF_G1PP,
>> +		.ioreg = WB_SIO_GPIO12_REG_IO1,
>> +		.invreg = WB_SIO_GPIO12_REG_INV1,
>> +		.datareg = WB_SIO_GPIO12_REG_DATA1,
>> +		.conflict = {
>> +			.name = "UARTB",
>> +			.dev = WB_SIO_DEV_UARTB,
>> +			.testreg = WB_SIO_UARTB_REG_ENABLE,
>> +			.testbit = WB_SIO_UARTB_ENABLE_ON,
>> +			.warnonly = true
>> +		}
>> +	},
>> +	{ /* 1 */
>> +		.dev = WB_SIO_DEV_GPIO12,
>> +		.enablereg = WB_SIO_GPIO12_REG_ENABLE,
>> +		.enablebit = WB_SIO_GPIO12_ENABLE_2,
>> +		.outputreg = WB_SIO_REG_GPIO1_MF,
>> +		.outputppbit = WB_SIO_REG_G1MF_G2PP,
>> +		.ioreg = WB_SIO_GPIO12_REG_IO2,
>> +		.invreg = WB_SIO_GPIO12_REG_INV2,
>> +		.datareg = WB_SIO_GPIO12_REG_DATA2
>> +		/* special conflict handling so doesn't use conflict
>> data */
>> +	},
>> +	{ /* 2 */
>> +		.dev = WB_SIO_DEV_GPIO34,
>> +		.enablereg = WB_SIO_GPIO34_REG_ENABLE,
>> +		.enablebit = WB_SIO_GPIO34_ENABLE_3,
>> +		.outputreg = WB_SIO_REG_OVTGPIO3456,
>> +		.outputppbit = WB_SIO_REG_OG3456_G3PP,
>> +		.ioreg = WB_SIO_GPIO34_REG_IO3,
>> +		.invreg = WB_SIO_GPIO34_REG_INV3,
>> +		.datareg = WB_SIO_GPIO34_REG_DATA3,
>> +		.conflict = {
>> +			.name = "UARTC",
>> +			.dev = WB_SIO_DEV_UARTC,
>> +			.testreg = WB_SIO_UARTC_REG_ENABLE,
>> +			.testbit = WB_SIO_UARTC_ENABLE_ON,
>> +			.warnonly = true
>> +		}
>> +	},
>> +	{ /* 3 */
>> +		.dev = WB_SIO_DEV_GPIO34,
>> +		.enablereg = WB_SIO_GPIO34_REG_ENABLE,
>> +		.enablebit = WB_SIO_GPIO34_ENABLE_4,
>> +		.outputreg = WB_SIO_REG_OVTGPIO3456,
>> +		.outputppbit = WB_SIO_REG_OG3456_G4PP,
>> +		.ioreg = WB_SIO_GPIO34_REG_IO4,
>> +		.invreg = WB_SIO_GPIO34_REG_INV4,
>> +		.datareg = WB_SIO_GPIO34_REG_DATA4,
>> +		.conflict = {
>> +			.name = "UARTD",
>> +			.dev = WB_SIO_DEV_UARTD,
>> +			.testreg = WB_SIO_UARTD_REG_ENABLE,
>> +			.testbit = WB_SIO_UARTD_ENABLE_ON,
>> +			.warnonly = true
>> +		}
>> +	},
>> +	{ /* 4 */
>> +		.dev = WB_SIO_DEV_WDGPIO56,
>> +		.enablereg = WB_SIO_WDGPIO56_REG_ENABLE,
>> +		.enablebit = WB_SIO_WDGPIO56_ENABLE_5,
>> +		.outputreg = WB_SIO_REG_OVTGPIO3456,
>> +		.outputppbit = WB_SIO_REG_OG3456_G5PP,
>> +		.ioreg = WB_SIO_WDGPIO56_REG_IO5,
>> +		.invreg = WB_SIO_WDGPIO56_REG_INV5,
>> +		.datareg = WB_SIO_WDGPIO56_REG_DATA5,
>> +		.conflict = {
>> +			.name = "UARTE",
>> +			.dev = WB_SIO_DEV_UARTE,
>> +			.testreg = WB_SIO_UARTE_REG_ENABLE,
>> +			.testbit = WB_SIO_UARTE_ENABLE_ON,
>> +			.warnonly = true
>> +		}
>> +	},
>> +	{ /* 5 */
>> +		.dev = WB_SIO_DEV_WDGPIO56,
>> +		.enablereg = WB_SIO_WDGPIO56_REG_ENABLE,
>> +		.enablebit = WB_SIO_WDGPIO56_ENABLE_6,
>> +		.outputreg = WB_SIO_REG_OVTGPIO3456,
>> +		.outputppbit = WB_SIO_REG_OG3456_G6PP,
>> +		.ioreg = WB_SIO_WDGPIO56_REG_IO6,
>> +		.invreg = WB_SIO_WDGPIO56_REG_INV6,
>> +		.datareg = WB_SIO_WDGPIO56_REG_DATA6,
>> +		.conflict = {
>> +			.name = "FDC",
>> +			.dev = WB_SIO_DEV_NONE,
>> +			.testreg = WB_SIO_REG_GLOBAL_OPT,
>> +			.testbit = WB_SIO_REG_GO_ENFDC,
>> +			.warnonly = false
>> +		}
>> +	}
>> +};
>> +
>> +/* returns whether changing a pin is allowed */
>> +static bool winbond_gpio_get_info(unsigned int *gpio_num,
>> +				  const struct winbond_gpio_info
>> **info)
>> +{
>> +	bool allow_changing = true;
>> +	unsigned long i;
>> +
>> +	for_each_set_bit(i, &gpios, BITS_PER_LONG) {
>> +		if (*gpio_num < 8)
>> +			break;
>> +
>> +		*gpio_num -= 8;
>> +	}
>> +
>> +	*info = &winbond_gpio_infos[i];
>> +
>> +	/*
>> +	 * GPIO2 (the second port) shares some pins with a basic PC
>> +	 * functionality, which is very likely controlled by the
>> firmware.
>> +	 * Don't allow changing these pins by default.
>> +	 */
>> +	if (i == 1) {
>> +		if (*gpio_num == 0 && !pledgpio)
>> +			allow_changing = false;
>> +		else if (*gpio_num == 1 && !beepgpio)
>> +			allow_changing = false;
>> +		else if ((*gpio_num == 5 || *gpio_num == 6) &&
>> !i2cgpio)
>> +			allow_changing = false;
>> +	}
>> +
>> +	return allow_changing;
>> +}
>> +
>> +static int winbond_gpio_get(struct gpio_chip *gc, unsigned int
>> offset)
>> +{
>> +	unsigned long *base = gpiochip_get_data(gc);
>> +	const struct winbond_gpio_info *info;
>> +	bool val;
>> +
>> +	winbond_gpio_get_info(&offset, &info);
>> +
>> +	val = winbond_sio_enter(*base);
>> +	if (val)
>> +		return val;
>> +
>> +	winbond_sio_select_logical(*base, info->dev);
>> +
>> +	val = winbond_sio_reg_btest(*base, info->datareg, offset);
>> +	if (winbond_sio_reg_btest(*base, info->invreg, offset))
>> +		val = !val;
>> +
>> +	winbond_sio_leave(*base);
>> +
>> +	return val;
>> +}
>> +
>> +static int winbond_gpio_direction_in(struct gpio_chip *gc, unsigned
>> int offset)
>> +{
>> +	unsigned long *base = gpiochip_get_data(gc);
>> +	const struct winbond_gpio_info *info;
>> +	int ret;
>> +
>> +	if (!winbond_gpio_get_info(&offset, &info))
>> +		return -EACCES;
>> +
>> +	ret = winbond_sio_enter(*base);
>> +	if (ret)
>> +		return ret;
>> +
>> +	winbond_sio_select_logical(*base, info->dev);
>> +
>> +	winbond_sio_reg_bset(*base, info->ioreg, offset);
>> +
>> +	winbond_sio_leave(*base);
>> +
>> +	return 0;
>> +}
>> +
>> +static int winbond_gpio_direction_out(struct gpio_chip *gc,
>> +				      unsigned int offset,
>> +				      int val)
>> +{
>> +	unsigned long *base = gpiochip_get_data(gc);
>> +	const struct winbond_gpio_info *info;
>> +	int ret;
>> +
>> +	if (!winbond_gpio_get_info(&offset, &info))
>> +		return -EACCES;
>> +
>> +	ret = winbond_sio_enter(*base);
>> +	if (ret)
>> +		return ret;
>> +
>> +	winbond_sio_select_logical(*base, info->dev);
>> +
>> +	winbond_sio_reg_bclear(*base, info->ioreg, offset);
>> +
>> +	if (winbond_sio_reg_btest(*base, info->invreg, offset))
>> +		val = !val;
>> +
>> +	if (val)
>> +		winbond_sio_reg_bset(*base, info->datareg, offset);
>> +	else
>> +		winbond_sio_reg_bclear(*base, info->datareg, offset);
>> +
>> +	winbond_sio_leave(*base);
>> +
>> +	return 0;
>> +}
>> +
>> +static void winbond_gpio_set(struct gpio_chip *gc, unsigned int
>> offset,
>> +			     int val)
>> +{
>> +	unsigned long *base = gpiochip_get_data(gc);
>> +	const struct winbond_gpio_info *info;
>> +
>> +	if (!winbond_gpio_get_info(&offset, &info))
>> +		return;
>> +
>> +	if (winbond_sio_enter(*base) != 0)
>> +		return;
>> +
>> +	winbond_sio_select_logical(*base, info->dev);
>> +
>> +	if (winbond_sio_reg_btest(*base, info->invreg, offset))
>> +		val = !val;
>> +
>> +	if (val)
>> +		winbond_sio_reg_bset(*base, info->datareg, offset);
>> +	else
>> +		winbond_sio_reg_bclear(*base, info->datareg, offset);
>> +
>> +	winbond_sio_leave(*base);
>> +}
>> +
>> +static struct gpio_chip winbond_gpio_chip = {
>> +	.base			= -1,
>> +	.label			= WB_GPIO_DRIVER_NAME,
>> +	.owner			= THIS_MODULE,
>> +	.can_sleep		= true,
>> +	.get			= winbond_gpio_get,
>> +	.direction_input	= winbond_gpio_direction_in,
>> +	.set			= winbond_gpio_set,
>> +	.direction_output	= winbond_gpio_direction_out,
>> +};
>> +
>> +static void winbond_gpio_configure_port0_pins(unsigned long base)
>> +{
>> +	unsigned int val;
>> +
>> +	val = winbond_sio_reg_read(base, WB_SIO_REG_GPIO1_MF);
>> +	if ((val & WB_SIO_REG_G1MF_FS_MASK) ==
>> WB_SIO_REG_G1MF_FS_GPIO1)
>> +		return;
>> +
>> +	pr_warn("GPIO1 pins were connected to something else (%.2x),
>> fixing\n",
>> +		val);
>> +
>> +	val &= ~WB_SIO_REG_G1MF_FS_MASK;
>> +	val |= WB_SIO_REG_G1MF_FS_GPIO1;
>> +
>> +	winbond_sio_reg_write(base, WB_SIO_REG_GPIO1_MF, val);
>> +}
>> +
>> +static void winbond_gpio_configure_port1_check_i2c(unsigned long
>> base)
>> +{
>> +	i2cgpio = !winbond_sio_reg_btest(base, WB_SIO_REG_I2C_PS,
>> +					 WB_SIO_REG_I2CPS_I2CFS);
>> +	if (!i2cgpio)
>> +		pr_warn("disabling GPIO2.5 and GPIO2.6 as I2C is
>> enabled\n");
>> +}
>> +
>> +static bool winbond_gpio_configure_port(unsigned long base, unsigned
>> int idx)
>> +{
>> +	const struct winbond_gpio_info *info =
>> &winbond_gpio_infos[idx];
>> +	const struct winbond_gpio_port_conflict *conflict = &info-
>>> conflict;
>> +
>> +	/* is there a possible conflicting device defined? */
>> +	if (conflict->name != NULL) {
>> +		if (conflict->dev != WB_SIO_DEV_NONE)
>> +			winbond_sio_select_logical(base, conflict-
>>> dev);
>> +
>> +		if (winbond_sio_reg_btest(base, conflict->testreg,
>> +					  conflict->testbit)) {
>> +			if (conflict->warnonly)
>> +				pr_warn("enabled GPIO%u share pins
>> with active %s\n",
>> +					idx + 1, conflict->name);
>> +			else {
>> +				pr_warn("disabling GPIO%u as %s is
>> enabled\n",
>> +					idx + 1, conflict->name);
>> +				return false;
>> +			}
>> +		}
>> +	}
>> +
>> +	/* GPIO1 and GPIO2 need some (additional) special handling */
>> +	if (idx == 0)
>> +		winbond_gpio_configure_port0_pins(base);
>> +	else if (idx == 1)
>> +		winbond_gpio_configure_port1_check_i2c(base);
>> +
>> +	winbond_sio_select_logical(base, info->dev);
>> +
>> +	winbond_sio_reg_bset(base, info->enablereg, info->enablebit);
>> +
>> +	if (ppgpios & BIT(idx))
>> +		winbond_sio_reg_bset(base, info->outputreg,
>> +				     info->outputppbit);
>> +	else if (odgpios & BIT(idx))
>> +		winbond_sio_reg_bclear(base, info->outputreg,
>> +				       info->outputppbit);
>> +	else
>> +		pr_notice("GPIO%u pins are %s\n", idx + 1,
>> +			  winbond_sio_reg_btest(base, info-
>>> outputreg,
>> +						info->outputppbit) ?
>> +			  "push-pull" :
>> +			  "open drain");
>> +
>> +	return true;
>> +}
>> +
>> +static int winbond_gpio_configure(unsigned long base)
>> +{
>> +	unsigned long i;
>> +
>> +	for_each_set_bit(i, &gpios, BITS_PER_LONG)
>> +		if (!winbond_gpio_configure_port(base, i))
> 
>> +			gpios &= ~BIT(i);
> 
> It might be __clear_bit(i, &gpios); as well.
> 
>> +
>> +	if (!gpios) {
>> +		pr_err("please use 'gpios' module parameter to select
>> some active GPIO ports to enable\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int winbond_gpio_check_chip(unsigned long base)
>> +{
>> +	int ret;
>> +	unsigned int chip;
>> +
>> +	ret = winbond_sio_enter(base);
>> +	if (ret)
>> +		return ret;
>> +
>> +	chip = winbond_sio_reg_read(base, WB_SIO_REG_CHIP_MSB) << 8;
>> +	chip |= winbond_sio_reg_read(base, WB_SIO_REG_CHIP_LSB);
>> +
>> +	pr_notice("chip ID at %lx is %.4x\n", base, chip);
>> +
>> +	if ((chip & WB_SIO_CHIP_ID_W83627UHG_MASK) !=
>> +	    WB_SIO_CHIP_ID_W83627UHG) {
>> +		pr_err("not an our chip\n");
>> +		ret = -ENODEV;
>> +	}
>> +
>> +	winbond_sio_leave(base);
>> +
>> +	return ret;
>> +}
>> +
>> +static int winbond_gpio_imatch(struct device *dev, unsigned int id)
>> +{
>> +	unsigned long gpios_rem;
>> +	int ret;
>> +
>> +	gpios_rem = gpios & ~GENMASK(ARRAY_SIZE(winbond_gpio_infos) -
>> 1, 0);
>> +	if (gpios_rem) {
>> +		pr_warn("unknown ports (%lx) enabled in GPIO ports
>> bitmask\n",
>> +			gpios_rem);
>> +		gpios &= ~gpios_rem;
>> +	}
>> +
>> +	if (ppgpios & odgpios) {
>> +		pr_err("some GPIO ports are set both to push-pull and
>> open drain mode at the same time\n");
>> +		return 0;
>> +	}
>> +
>> +	if (baseparam != 0)
>> +		return winbond_gpio_check_chip(baseparam) == 0;
>> +
>> +	/*
>> +	 * if the 'base' module parameter is unset probe two chip
>> default
>> +	 * I/O port bases
>> +	 */
>> +	baseparam = WB_SIO_BASE;
>> +	ret = winbond_gpio_check_chip(baseparam);
>> +	if (ret == 0)
>> +		return 1;
>> +	if (ret != -ENODEV && ret != -EBUSY)
>> +		return 0;
>> +
>> +	baseparam = WB_SIO_BASE_HIGH;
>> +	return winbond_gpio_check_chip(baseparam) == 0;
>> +}
>> +
>> +static int winbond_gpio_iprobe(struct device *dev, unsigned int id)
>> +{
>> +	int ret;
>> +
>> +	if (baseparam == 0)
>> +		return -EINVAL;
>> +
>> +	ret = winbond_sio_enter(baseparam);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = winbond_gpio_configure(baseparam);
>> +
>> +	winbond_sio_leave(baseparam);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Add 8 gpios for every GPIO port that was enabled in gpios
>> +	 * module parameter (that wasn't disabled earlier in
>> +	 * winbond_gpio_configure() & co. due to, for example, a pin
>> conflict).
>> +	 */
>> +	winbond_gpio_chip.ngpio = hweight_long(gpios) * 8;
>> +
>> +	/*
>> +	 * GPIO6 port has only 5 pins, so if it is enabled we have to
>> adjust
>> +	 * the total count appropriately
>> +	 */
>> +	if (gpios & BIT(5))
>> +		winbond_gpio_chip.ngpio -= (8 - 5);
>> +
>> +	winbond_gpio_chip.parent = dev;
>> +
>> +	return devm_gpiochip_add_data(dev, &winbond_gpio_chip,
>> &baseparam);
>> +}
>> +
>> +static struct isa_driver winbond_gpio_idriver = {
>> +	.driver = {
>> +		.name	= WB_GPIO_DRIVER_NAME,
>> +	},
>> +	.match	= winbond_gpio_imatch,
>> +	.probe	= winbond_gpio_iprobe,
>> +};
>> +
>> +module_isa_driver(winbond_gpio_idriver, 1);
>> +
>> +module_param_named(base, baseparam, ulong, 0444);
>> +MODULE_PARM_DESC(base,
>> +		 "I/O port base (when unset - probe chip default
>> ones)");
>> +
>> +/* This parameter sets which GPIO devices (ports) we enable */
>> +module_param(gpios, ulong, 0444);
>> +MODULE_PARM_DESC(gpios,
>> +		 "bitmask of GPIO ports to enable (bit 0 - GPIO1, bit
>> 1 - GPIO2, etc.");
>> +
>> +/*
>> + * These two parameters below set how we configure GPIO ports output
>> drivers.
>> + * It can't be a one bitmask since we need three values per port:
>> push-pull,
>> + * open-drain and keep as-is (this is the default).
>> + */
>> +module_param(ppgpios, ulong, 0444);
>> +MODULE_PARM_DESC(ppgpios,
>> +		 "bitmask of GPIO ports to set to push-pull mode (bit
>> 0 - GPIO1, bit 1 - GPIO2, etc.");
>> +
>> +module_param(odgpios, ulong, 0444);
>> +MODULE_PARM_DESC(odgpios,
>> +		 "bitmask of GPIO ports to set to open drain mode
>> (bit 0 - GPIO1, bit 1 - GPIO2, etc.");
>> +
>> +/*
>> + * GPIO2.0 and GPIO2.1 control a basic PC functionality that we
>> + * don't allow tinkering with by default (it is very likely that the
>> + * firmware owns these pins).
>> + * These two parameters below allow overriding these prohibitions.
>> + */
>> +module_param(pledgpio, bool, 0644);
>> +MODULE_PARM_DESC(pledgpio,
>> +		 "enable changing value of GPIO2.0 bit (Power LED),
>> default no.");
>> +
>> +module_param(beepgpio, bool, 0644);
>> +MODULE_PARM_DESC(beepgpio,
>> +		 "enable changing value of GPIO2.1 bit (BEEP),
>> default no.");
>> +
>> +MODULE_AUTHOR("Maciej S. Szmigiero <mail@xxxxxxxxxxxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("GPIO interface for Winbond Super I/O chips");
>> +MODULE_LICENSE("GPL");
>