Re: [PATCH v2 5/5] gpio: Add driver for Maxim MAX3191x industrial serializer

[Lukas Wunner <lukas@wunner.de>]

[cc += William Breathitt Gray, Geert Uytterhoeven, Phil Reid,
       David Daney, Iban Rodriguez]

The drivers

       gpio-104-dio-48e.c, gpio-74x164.c, gpio-gpio-mm.c,
       gpio-pca953x.c, gpio-thunderx.c, gpio-ws16c48.c,
       gpio-xilinx.c

currently use an inefficient algorithm for their ->set_multiple
callback:  They iterate over every chip (or bank or gpio pin),
check if any bits are set in the mask for this particular chip,
and if so, update the affected GPIOs.  If the mask is sparsely
populated, you'll waste CPU time checking chips even though
they're not affected by the operation at all.

Iterating over the chips is backwards, it is more efficient to
iterate over the bits set in the mask, identify the corresponding
chip and update its affected GPIOs.  The gpio-max3191x.c driver
I posted yesterday contains an example for such an algorithm and
you may want to improve your ->set_mutiple implementation accordingly:


> +static int max3191x_get_multiple(struct gpio_chip *gpio, unsigned long *mask,
> +				 unsigned long *bits)
> +{
> +	struct max3191x_chip *max3191x = gpiochip_get_data(gpio);
> +	int ret, bit = 0, wordlen = max3191x_wordlen(max3191x);
> +
> +	mutex_lock(&max3191x->lock);
> +	ret = max3191x_readout_locked(max3191x);
> +	if (ret)
> +		goto out_unlock;
> +
> +	while ((bit = find_next_bit(mask, gpio->ngpio, bit)) != gpio->ngpio) {
> +		unsigned int chipnum = bit / MAX3191X_NGPIO;
> +		unsigned long in, shift, index;
> +
> +		if (max3191x_chip_is_faulting(max3191x, chipnum)) {
> +			ret = -EIO;
> +			goto out_unlock;
> +		}
> +
> +		in = ((u8 *)max3191x->xfer.rx_buf)[chipnum * wordlen];
> +		shift = round_down(bit % BITS_PER_LONG, MAX3191X_NGPIO);
> +		index = bit / BITS_PER_LONG;
> +		bits[index] &= ~(mask[index] & (0xff << shift));
> +		bits[index] |= mask[index] & (in << shift); /* copy bits */
> +
> +		bit = (chipnum + 1) * MAX3191X_NGPIO; /* go to next chip */
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&max3191x->lock);
> +	return ret;
> +}


I've used a while loop, obviously the same can be achieved with a
for loop but I found that harder to read and it didn't save any LoC.

This particular chip has 8 inputs (= MAX3191X_NGPIO) that can be read
concurrently.

The series containing this driver introduces a ->get_multiple callback.
Since you've implemented a ->set_multiple callback, you may want to
add a ->get_multiple callback to your driver as well.  (If supported
by the hardware, which is not the case for output-only chips such as
gpio-74x164.c.)

You can find the full series here:
https://www.spinics.net/lists/linux-gpio/msg25997.html

If you want to fetch the series from a git repo, use this branch:
https://github.com/RevolutionPi/linux/commits/revpi-4.9

Thanks,

Lukas