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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy