Re: [PATCH v3] gpio: winbond: add driver
On 04.01.2018 18:35, Andy Shevchenko wrote:
> On Thu, 2018-01-04 at 00:41 +0100, Maciej S. Szmigiero wrote:
>> On 03.01.2018 20:05, Andy Shevchenko wrote:
>>> On Sat, 2017-12-30 at 22:02 +0100, Maciej S. Szmigiero wrote:
This commit adds GPIO driver for Winbond Super I/Os.
>
>>> First of all, looking more at this driver, why don't we create a
>>> gpiochip per real "port" during actual configuration?
>>
>> Hmm.. there is only a one 'chip' here, so why would the driver want to
>> register multiple ones?
>>
>> That would also create at least one additional point of failure if
>> one or more such gpiochip(s) register but one fails to do so.
>>
>>> And I still have filing that this one suitable for MFD.
>>
>> As I wrote previously, that would necessitate rewriting also w83627ehf
>> hwmon and w83627hf_wdt drivers, and would make the driver stand out
>> against other, similar Super I/O drivers.
>>
>>> Anyone, does it make sense?
>
> OK, at least I shared my point.
>
+/* 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)) {
>
> sizeof(gpios) will produce wrong number for you. It's rather
> BITS_PER_LONG here. Right?
Yes, you are right.
BTW: It happened to work fine when tested on x86-64 since the first
8 bits of "gpios" cover all the existing GPIO ports.
+ 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];
>>>
>>> ...
>>
>> Unfortunately, this code doesn't produce the same results as the code
>> above.
>>
>> First, in this code "index" does not depend on "gpio_num" (or
>> "offset")
>> passed to winbond_gpio_get_info() function, so gpio 0 (on the first
>> GPIO
>> device or port) will access the same winbond_gpio_infos entry as gpio
>> 18
>> (which is located on the third GPIO port).
>
> Actually, it does depend on gpio_num (it's your point to break the
> loop).
I meant that in your code "index" does not depend on "gpio_num" (also
known as "offset").
> So, fls(*offset) then (I renamed gpio_num to offset in my example).
"index = fls(*offset)" isn't correct either, since with "offset" == 10
(0b1010) and "gpios" == 0b1101 we want "index" pointing at the second
enabled GPIO port, that is, the third port (so index should equal 2),
but with the same "offset" and "gpios" == 0b we want "index" to
equal 1.
That is, "index" should depend both on "gpios" and "offset", since when
counting gpio numbers in "offset" we skip disabled GPIO ports (that is,
these that have their bits in "gpios" unset), but they are counted in
"index" because this variable points at static array of hardware GPIO
ports descriptions, which always contains all ports (enabled or not).
>> In fact, the calculated "index" would always point to the last enabled
>> GPIO port (since that is the meaning of "gpios" MSB, assuming this
>> user-provided parameter was properly verified or sanitized).
>
> Yes, I missed that.
>
>> Second, the calculated "offset" would end negative for anything but
>> the
>> very last GPIO port (ok, not really negative since it is an unsigned
>> type,
>> but still not correct either).
>
> So, sounds like hweight_int(*offset) then. No?
(Guess you meant hweight_long() above since I wasn't able to find
hweight_int() in the tree).
"*offset = hweight_long(*offset)" isn't correct since it doesn't
produce the same results as "*offset %= 8" and that is what we need to
calculate.
"index = hweight_long(*offset)" isn't correct since "offset" isn't a
bitmask, it is a number, and "index" (for the reasons described in the
previous comment) should also depend on "gpios" value.
>> And that even not taking into account the special case of GPIO6 port
>> that has only 5 gpios.
>
> This doesn't matter because of check in ternary operator.
>
>> What we want in this code is for "i" (or "index") to contain the GPIO
>> port number for the passed "gpio_num" (or "offset") and that this
>> last variable ends reduced modulo 8 from its original value.
>
> Yep.
>
+ 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.
>>
>> Can be done either way, but I think notifying user that he or she has
>> provided an incorrect parameter value is a good thing - we can use a
>> accept-but-warn style.
>
> I would prefer latter (accept-but-warn).
>
Will do.
Mac
Re: [PATCH v3] gpio: winbond: add driver
On Thu, 2018-01-04 at 00:41 +0100, Maciej S. Szmigiero wrote:
> On 03.01.2018 20:05, Andy Shevchenko wrote:
> > On Sat, 2017-12-30 at 22:02 +0100, Maciej S. Szmigiero wrote:
> > > This commit adds GPIO driver for Winbond Super I/Os.
> > First of all, looking more at this driver, why don't we create a
> > gpiochip per real "port" during actual configuration?
>
> Hmm.. there is only a one 'chip' here, so why would the driver want to
> register multiple ones?
>
> That would also create at least one additional point of failure if
> one or more such gpiochip(s) register but one fails to do so.
>
> > And I still have filing that this one suitable for MFD.
>
> As I wrote previously, that would necessitate rewriting also w83627ehf
> hwmon and w83627hf_wdt drivers, and would make the driver stand out
> against other, similar Super I/O drivers.
>
> > Anyone, does it make sense?
OK, at least I shared my point.
> > > +/* 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)) {
sizeof(gpios) will produce wrong number for you. It's rather
BITS_PER_LONG here. Right?
> > > + 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];
> >
> > ...
>
> Unfortunately, this code doesn't produce the same results as the code
> above.
>
> First, in this code "index" does not depend on "gpio_num" (or
> "offset")
> passed to winbond_gpio_get_info() function, so gpio 0 (on the first
> GPIO
> device or port) will access the same winbond_gpio_infos entry as gpio
> 18
> (which is located on the third GPIO port).
Actually, it does depend on gpio_num (it's your point to break the
loop).
So, fls(*offset) then (I renamed gpio_num to offset in my example).
> In fact, the calculated "index" would always point to the last enabled
> GPIO port (since that is the meaning of "gpios" MSB, assuming this
> user-provided parameter was properly verified or sanitized).
Yes, I missed that.
> Second, the calculated "offset" would end negative for anything but
> the
> very last GPIO port (ok, not really negative since it is an unsigned
> type,
> but still not correct either).
So, sounds like hweight_int(*offset) then. No?
> And that even not taking into account the special case of GPIO6 port
> that has only 5 gpios.
This doesn't matter because of check in ternary operator.
> What we want in this code is for "i" (or "index") to contain the GPIO
> port number for the passed "gpio_num" (or "offset") and that this
> last variable ends reduced modulo 8 from its original value.
Yep.
> > > + 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.
>
> Can be done either way, but I think notifying user that he or she has
> provided an incorrect parameter value is a good thing - we can use a
> accept-but-warn style.
I would prefer latter (accept-but-warn).
--
Andy Shevchenko
Intel Finland Oy
Re: [PATCH v3] gpio: winbond: add driver
On 03.01.2018 20:05, Andy Shevchenko wrote:
> 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?
Hmm.. there is only a one 'chip' here, so why would the driver want to
register multiple ones?
That would also create at least one additional point of failure if
one or more such gpiochip(s) register but one fails to do so.
> And I still have filing that this one suitable for MFD.
As I wrote previously, that would necessitate rewriting also w83627ehf
hwmon and w83627hf_wdt drivers, and would make the driver stand out
against other, similar Super I/O drivers.
> Anyone, does it make sense?
>
>> +#define WB_SIO_REG_G1MF_G2PP7
>> +#define WB_SIO_REG_G1MF_G1PP6
>
> Forgot to swap.
Good catch, will 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.
I see now what you meant, will do it then.
>> +/* 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];
>
> ...
Unfortunately, this code doesn't produce the same results as the code
above.
First, in this code "index" does not depend on "gpio_num" (or "offset")
passed to winbond_gpio_get_info() function, so gpio 0 (on the first GPIO
device or port) will access the same winbond_gpio_infos entry as gpio 18
(which is located on the third GPIO port).
In fact, the calculated "index" would always point to the last enabled
GPIO port (since that is the meaning of "gpios" MSB, assuming this
user-provided parameter was properly verified or sanitized).
Second, the calculated "offset" would end negative for anything but the
very last GPIO port (ok, not really negative since it is an unsigned type,
but still not correct either).
And that even not taking into account the special case of GPIO6 port
that has only 5 gpios.
What we want in this code is for "i" (or "index") to contain the GPIO
port number for the passed "gpio_num" (or "offset") and that this
last variable ends reduced modulo 8 from its original value.
>> +
>> +*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 enabl
Re: [PATCH v3] gpio: winbond: add driver
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 abo
Re: [PATCH v3] gpio: winbond: add driver
Hi Maciej, Thank you for the patch! Yet something to improve: [auto build test ERROR on gpio/for-next] [also build test ERROR on v4.15-rc6 next-20180102] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Maciej-S-Szmigiero/gpio-winbond-add-driver/20180103-011623 base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next config: mips-allmodconfig (attached as .config) compiler: mips-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=mips All errors (new ones prefixed by >>): >> drivers/gpio/Kconfig:13:error: recursive dependency detected! For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpio/Kconfig:13: symbol GPIOLIB is selected by STX104 For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/iio/adc/Kconfig:659: symbol STX104 depends on ISA_BUS_API For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" arch/Kconfig:818:symbol ISA_BUS_API is selected by GPIO_WINBOND For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpio/Kconfig:701:symbol GPIO_WINBOND depends on GPIOLIB .config:9795:warning: symbol value '' invalid for MIPS_CPS_NS16550_BASE .config:9796:warning: symbol value '' invalid for MIPS_CPS_NS16550_SHIFT -- >> drivers/gpio/Kconfig:13:error: recursive dependency detected! For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpio/Kconfig:13: symbol GPIOLIB is selected by STX104 For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/iio/adc/Kconfig:659: symbol STX104 depends on ISA_BUS_API For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" arch/Kconfig:818:symbol ISA_BUS_API is selected by GPIO_WINBOND For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpio/Kconfig:701:symbol GPIO_WINBOND depends on GPIOLIB .config:9795:warning: symbol value '' invalid for MIPS_CPS_NS16550_BASE .config:9796:warning: symbol value '' invalid for MIPS_CPS_NS16550_SHIFT make[3]: *** [silentoldconfig] Error 1 make[2]: *** [silentoldconfig] Error 2 make[1]: *** No rule to make target 'include/config/auto.conf', needed by 'include/config/kernel.release'. make[1]: Target 'prepare' not remade because of errors. make: *** [sub-make] Error 2 vim +13 drivers/gpio/Kconfig a9c5fff5 David Brownell 2008-02-04 4 7563bbf8 Mark Brown 2012-04-15 5 config ARCH_HAVE_CUSTOM_GPIO_H 7563bbf8 Mark Brown 2012-04-15 6 bool 7563bbf8 Mark Brown 2012-04-15 7 help 7563bbf8 Mark Brown 2012-04-15 8Selecting this config option from the architecture Kconfig allows 7563bbf8 Mark Brown 2012-04-15 9the architecture to provide a custom asm/gpio.h implementation 7563bbf8 Mark Brown 2012-04-15 10overriding the default implementations. New uses of this are 7563bbf8 Mark Brown 2012-04-15 11strongly discouraged. 7563bbf8 Mark Brown 2012-04-15 12 7444a72e Michael Buesch 2008-07-25 @13 menuconfig GPIOLIB 7444a72e Michael Buesch 2008-07-25 14 bool "GPIO Support" 60a5eaba Linus Walleij 2016-06-08 15 select ANON_INODES 7444a72e Michael Buesch 2008-07-25 16 help 7444a72e Michael Buesch 2008-07-25 17This enables GPIO support through the generic GPIO library. 7444a72e Michael Buesch 2008-07-25 18You only need to enable this, if you also want to enable e849dc04 Linus Walleij 2011-05-20 19one or more of the GPIO drivers below. 7444a72e Michael Buesch 2008-07-25 20 7444a72e Michael Buesch 2008-07-25 21If unsure, say N. a9c5fff5 David Brownell 2008-02-04 22 :: The code at line 13 was first introduced by commit :: 7444a72effa632fcd8edc566f880d96fe213c73b gpiolib: allow user-selection :: TO: Michael Buesch :: CC: Linus Torvalds --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
