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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html