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

[Andy Shevchenko]

On Sat, 2017-12-30 at 22:02 +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.
> 

Thanks for an update.
My comments below.

First of all, looking more at this driver, why don't we create a
gpiochip per real "port" during actual configuration?

And I still have filing that this one suitable for MFD.

Anyone, does it make sense?

> +#define WB_SIO_REG_G1MF_G2PP		7
> +#define WB_SIO_REG_G1MF_G1PP		6

Forgot to swap.

> +#define wb_sio_notice(...) pr_notice(WB_GPIO_DRIVER_NAME ": "
> __VA_ARGS__)
> +#define wb_sio_warn(...) pr_warn(WB_GPIO_DRIVER_NAME ": "
> __VA_ARGS__)
> +#define wb_sio_err(...) pr_err(WB_GPIO_DRIVER_NAME ": " __VA_ARGS__)

What I meant is to

#define pr_fmt(x) ...

Look at the kernel sources, there are a lot of examples.

> +/* 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, sizeof(gpios)) {
> +		if (*gpio_num < 8)
> +			break;
> +
> +		*gpio_num -= 8;
> +	}

Why not hweight() here?

unsigned int shift = hweight_long(gpios) * 8;
unsigned int index = fls_long(gpios); // AFAIU

*offset -= *offset >= shift ? shift : shift - 8;
*info = &winbond_gpio_infos[index];

...

> +
> +	*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_configure(unsigned long base)
> +{
> +	unsigned long i;
> +
> +	for_each_set_bit(i, &gpios, sizeof(gpios))
> +		if (!winbond_gpio_configure_port(base, i))
> +			gpios &= ~BIT(i);

> +
> +	if (!gpios) {
> +		wb_sio_err("please use 'gpios' module parameter to
> select some active GPIO ports to enable\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> 

> +static int winbond_gpio_imatch(struct device *dev, unsigned int id)
> +{
> +	int ret;
> +

> +	if (gpios & ~GENMASK(ARRAY_SIZE(winbond_gpio_infos) - 1, 0))
> {
> +		wb_sio_err("unknown ports enabled in GPIO ports
> bitmask\n");
> +		return 0;
> +	}

Do we care? Perhaps just enforce mask based on the size and leave
garbage out.

> +	/*
> +	 * 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;

> +	else if (ret != -ENODEV && ret != -EBUSY)

Redundant 'else'.

> +		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);

...like registering MFD children in that call directly?

> +
> +	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);

So, if we still are going use this, taking into consideration above
proposal, it would make sense just to cache values in some internal
struct and use above, right?

> +
> +	winbond_gpio_chip.parent = dev;
> +
> +	return devm_gpiochip_add_data(dev, &winbond_gpio_chip,
> &baseparam);
> +}

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy