GPIO driver for Maxim MAX3191x

A common idiom is to assign a value to a bit with:

    if (value)
        set_bit(nr, addr);
    else
        clear_bit(nr, addr);

Likewise common is the one-line expression variant:

    value ? set_bit(nr, addr) : clear_bit(nr, addr);

Commit 9a8ac3ae682e ("dm mpath: cleanup QUEUE_IF_NO_PATH bit manipulation by introducing assign_bit()") introduced assign_bit() to the md subsystem for brevity.

Make it available to others, specifically gpiolib and the upcoming driver for Maxim MAX3191x industrial serializer chips.

As requested by Peter Zijlstra, change the argument order to reflect traditional "dst = src" in C, hence "assign_bit(nr, addr, value)".

drivers/gpio/gpio-max3191x.c new file +23 -0

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

A common idiom is to assign a value to a bit with:

    if (value)
        set_bit(nr, addr);
    else
        clear_bit(nr, addr);

Likewise common is the one-line expression variant:

    value ? set_bit(nr, addr) : clear_bit(nr, addr);

Commit 9a8ac3ae682e ("dm mpath: cleanup QUEUE_IF_NO_PATH bit manipulation by introducing assign_bit()") introduced assign_bit() to the md subsystem for brevity.

Make it available to others, specifically gpiolib and the upcoming driver for Maxim MAX3191x industrial serializer chips.

As requested by Peter Zijlstra, change the argument order to reflect traditional "dst = src" in C, hence "assign_bit(nr, addr, value)".

Cc Bart Van Assche <[email protected]>

Cc Alasdair Kergon <[email protected]>

Cc Mike Snitzer <[email protected]>

Cc Linus Walleij <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Neil Brown <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Theodore Ts'o <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Denys Vlasenko <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

SPI-attached GPIO controllers typically read out all inputs in one go. If callers desire the values of multipe inputs, ideally a single readout should take place to return the desired values. However the current driver API only offers a ->get callback but no ->get_multiple (unlike ->set_multiple, which is present). Thus, to read multiple inputs, a full readout needs to be performed for every single value (barring driver-internal caching), which is inefficient.

In fact, the lack of a ->get_multiple callback has been bemoaned repeatedly by the gpio subsystem maintainer:
http://www.spinics.net/lists/linux-gpio/msg10571.html
http://www.spinics.net/lists/devicetree/msg121734.html

Introduce the missing callback. Add corresponding consumer functions such as gpiod_get_array_value(). Amend linehandle_ioctl() to take advantage of the newly added infrastructure. Update the documentation.

Cc Rojhalat Ibrahim <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
Changes v1 -> v2:

- Patch [1/5]: Change the argument order of assign_bit() to reflect traditional "dst = src" in C. (Peter Zijlstra)

- Patch [2/5]: Update documentation. (Linus Walleij) Drop const qualifier from struct gpio_desc ** in all function signatures to avoid a cast when passing a non-const array created with gpiod_get_array(), which is likely the most common use case.

- Patch [3/5]: Newly inserted patch to introduce common property for number of daisy-chained devices.

- Patch [4/5]: Add vendor prefix to GPIO identifiers, use boolean instead of integer to select mode, rename boolean to ignore undervoltage alarms, separate compatible strings with newlines. (Rob Herring)

- Patch [5/5]: Optimize algorithm in max3191x_get_multiple() to iterate over the bits in the mask, instead of iterating over every chip, thus implicitly skipping chips which are not selected at all by the mask.

[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:

Quoted 13 lines · Lukas Wunner

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.

Quoted 5 lines

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:

Do you have any profiling results that demonstrate significant system-wide performance improvements by making such a change?

For the gpio-thunderx.c driver, the words in the bits/mask exactly match the banks, so the number of iterations doesn't change with the approach you suggest.

In fact, an argument could be made in the other direction. The increased ICache pressure from code bloat and branch prediction misses resulting from extra testing of the mask bits could easily make the system slower using your suggestion.

Quoted 14 lines · David Daney

Quoted 13 lines · Lukas Wunner

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.

Quoted 8 lines

Quoted 5 lines

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:

Do you have any profiling results that demonstrate significant system-wide performance improvements by making such a change?

Sorry, no.

Quoted 3 lines

For the gpio-thunderx.c driver, the words in the bits/mask exactly match the banks, so the number of iterations doesn't change with the approach you suggest.

No, I was specifically referring to a sparsely populated mask above. Let's say the mask consists of several 64-bit words and only the last word has set bits. With the existing algorithm you have as many iterations as you have banks:

	for (bank = 0; bank <= chip->ngpio / 64; bank++) {
		set_bits = bits[bank] & mask[bank];
		clear_bits = ~bits[bank] & mask[bank];
		writeq(set_bits, txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_SET);
		writeq(clear_bits, txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_CLR);
	}

Whereas with the algorithm I'm proposing you only have a single iteration:

	int bit = 0;
	while ((bit = find_next_bit(mask, chip->ngpio, bit)) != chip->ngpio) {
		unsigned int bank = bit / 64;
		set_bits = bits[bank] & mask[bank];
		clear_bits = ~bits[bank] & mask[bank];
		writeq(set_bits, txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_SET);
		writeq(clear_bits, txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_CLR);
		bit = (bank + 1) * 64; /* continue search from next bank */
	}

find_next_bit() uses the clz instruction on ARM, similar instructions exist on most arches, so the search is very fast.

There are a maximum of two banks.

For some thunderx SoCs there is only a single bank, so my code has exactly one iteration. For the SoCs with two banks, my code has exactly two iterations. There are no function calls in the loop.

I can't believe I have spent so much time on this issue, for which no matter what you do, will not impact system performance in any measurable way.

Many serially-attached GPIO and IIO devices are daisy-chainable.

    Examples for GPIO devices are Maxim MAX3191x and TI SN65HVS88x:
    https://datasheets.maximintegrated.com/en/ds/MAX31913.pdf
    http://www.ti.com/lit/ds/symlink/sn65hvs880.pdf

    Examples for IIO devices are TI DAC128S085 and TI DAC161S055:
    http://www.ti.com/lit/ds/symlink/dac128s085.pdf
    http://www.ti.com/lit/ds/symlink/dac161s055.pdf

We already have drivers for daisy-chainable devices in the tree but their devicetree bindings are somewhat inconsistent and ill-named:

The gpio-74x164.c driver uses "registers-number" to convey the number of devices in the daisy-chain. (Sans vendor prefix, multiple vendors sell compatible versions of this chip.)

The gpio-pisosr.c driver takes a different approach and calculates the number of devices in the daisy-chain by dividing the common "ngpios" property (Documentation/devicetree/bindings/gpio/gpio.txt) by 8 (which assumes that each chip has 8 inputs).

Let's standardize on a common "#daisy-chained-devices" property. That name was chosen because it's the term most frequently used in datasheets. (A less frequently used synonym is "cascaded devices".)

Signed-off-by Lukas Wunner <lukas-JFq808J9C/[email protected]>

---
Changes v1 -> v2:

- Newly inserted patch.

 .../devicetree/bindings/common-properties.txt      | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Documentation/devicetree/bindings/common-properties.txt +16 -0

index 697714f8d75c..a3448bfa1c82 100644
@@ -1,4 +1,8 @@
 Common properties
+=================
+
+Endianness
+----------
 
 The Devicetree Specification does not define any properties related to hardware
 byteswapping, but endianness issues show up frequently in porting Linux to

@@ -58,3 +62,25 @@ dev: dev@40031000 {
...
 	      little-endian;
 };
+
+Daisy-chained devices
+---------------------
+
+Many serially-attached GPIO and IIO devices are daisy-chainable.  To the
+host controller, a daisy-chain appears as a single device, but the number
+of inputs and outputs it provides is the sum of inputs and outputs provided
+by all of its devices.  The driver needs to know how many devices the
+daisy-chain comprises to determine the amount of data exchanged, how many
+inputs and outputs to register and so on.
+
+Optional properties:

Add device tree bindings for Maxim MAX3191x industrial serializer.

Cc Mathias Duckeck <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
Changes v1 -> v2:

- Add vendor prefix to GPIO identifiers, use boolean instead of integer to select mode, rename boolean to ignore undervoltage alarms, separate compatible strings with newlines. (Rob Herring)

 .../devicetree/bindings/gpio/gpio-max3191x.txt     | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-max3191x.txt

Many serially-attached GPIO and IIO devices are daisy-chainable.

    Examples for GPIO devices are Maxim MAX3191x and TI SN65HVS88x:
    https://datasheets.maximintegrated.com/en/ds/MAX31913.pdf
    http://www.ti.com/lit/ds/symlink/sn65hvs880.pdf

    Examples for IIO devices are TI DAC128S085 and TI DAC161S055:
    http://www.ti.com/lit/ds/symlink/dac128s085.pdf
    http://www.ti.com/lit/ds/symlink/dac161s055.pdf

We already have drivers for daisy-chainable devices in the tree but their devicetree bindings are somewhat inconsistent and ill-named:

The gpio-74x164.c driver uses "registers-number" to convey the number of devices in the daisy-chain. (Sans vendor prefix, multiple vendors sell compatible versions of this chip.)

The gpio-pisosr.c driver takes a different approach and calculates the number of devices in the daisy-chain by dividing the common "ngpios" property (Documentation/devicetree/bindings/gpio/gpio.txt) by 8 (which assumes that each chip has 8 inputs).

Let's standardize on a common "#daisy-chained-devices" property. That name was chosen because it's the term most frequently used in datasheets. (A less frequently used synonym is "cascaded devices".)

Signed-off-by Lukas Wunner <lukas-JFq808J9C/[email protected]>

---
Changes v1 -> v2:

- Newly inserted patch.

 .../devicetree/bindings/common-properties.txt      | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Many serially-attached GPIO and IIO devices are daisy-chainable.

    Examples for GPIO devices are Maxim MAX3191x and TI SN65HVS88x:
    https://datasheets.maximintegrated.com/en/ds/MAX31913.pdf
    http://www.ti.com/lit/ds/symlink/sn65hvs880.pdf

    Examples for IIO devices are TI DAC128S085 and TI DAC161S055:
    http://www.ti.com/lit/ds/symlink/dac128s085.pdf
    http://www.ti.com/lit/ds/symlink/dac161s055.pdf

We already have drivers for daisy-chainable devices in the tree but their devicetree bindings are somewhat inconsistent and ill-named:

The gpio-74x164.c driver uses "registers-number" to convey the number of devices in the daisy-chain. (Sans vendor prefix, multiple vendors sell compatible versions of this chip.)

The gpio-pisosr.c driver takes a different approach and calculates the number of devices in the daisy-chain by dividing the common "ngpios" property (Documentation/devicetree/bindings/gpio/gpio.txt) by 8 (which assumes that each chip has 8 inputs).

Let's standardize on a common "#daisy-chained-devices" property. That name was chosen because it's the term most frequently used in datasheets. (A less frequently used synonym is "cascaded devices".)

Signed-off-by Lukas Wunner <lukas-JFq808J9C/[email protected]>

---
Changes v1 -> v2:

- Newly inserted patch.

Add device tree bindings for Maxim MAX3191x industrial serializer.

Cc Mathias Duckeck <m.duckeck-XB/[email protected]>

Signed-off-by Lukas Wunner <lukas-JFq808J9C/[email protected]>

---
Changes v1 -> v2:

The driver was developed for and tested with the MAX31913 built into the Revolution Pi by KUNBUS, but should work with all members of the MAX3191x family:

MAX31910: low power
MAX31911: LED drivers
MAX31912: LED drivers + 2nd voltage monitor + low power
MAX31913: LED drivers + 2nd voltage monitor
MAX31953: LED drivers + 2nd voltage monitor + isolation
MAX31963: LED drivers + 2nd voltage monitor + isolation + buck regulator

Cc Mathias Duckeck <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>