GPIO driver for Maxim MAX3191x

[ ... ]

Cc Bart Van Assche <[email protected]>

Cc Alasdair Kergon <[email protected]>

Cc Mike Snitzer <[email protected]>

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

include/linux/bitops.h +13 -0

+/**
+ * assign_bit - Assign value to a bit in memory
+ * @value: the value to assign
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ */
+static __always_inline void assign_bit(bool value, long nr,
+				       volatile unsigned long *addr)
+{
+	if (value)
+		set_bit(nr, addr);
+	else
+		clear_bit(nr, addr);

Quoted 4 lines · Lukas Wunner

Cc Bart Van Assche <[email protected]>

Cc Alasdair Kergon <[email protected]>

Cc Mike Snitzer <[email protected]>

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

This Cc-list is incomplete. Previous <linux/bitops.h> patches went in through Andrew Morton's tree so I think an ack from Andrew Morton is needed before this patch can be sent to Linus Torvalds. Please also Cc other frequent contributors to this header file, e.g. Ingo Molnar and Peter Zijlstra. Please also consider to Cc the LKML for this patch or even for the entire series.

Review

Quoted 2 lines

(quoted diff) +2 -0

+static __always_inline void assign_bit(bool value, long nr,
+				       volatile unsigned long *addr)

Why has __always_inline been specified? What makes you think that you know better than the compiler whether or not these functions should be inlined?

include/linux/bitops.h +14 -0

index a83c822c35c2..097af36887c0 100644
@@ -226,6 +226,30 @@ static inline unsigned long __ffs64(u64 word)
 	return __ffs((unsigned long)word);
 }
 
+/**
+ * assign_bit - Assign value to a bit in memory
+ * @value: the value to assign
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ */
+static __always_inline void assign_bit(bool value, long nr,
+				       volatile unsigned long *addr)
+{
+	if (value)
+		set_bit(nr, addr);
+	else
+		clear_bit(nr, addr);
+}

Quoted 22 lines · Lukas Wunner

include/linux/bitops.h +14 -0

index a83c822c35c2..097af36887c0 100644
@@ -226,6 +226,30 @@ static inline unsigned long __ffs64(u64 word)
 	return __ffs((unsigned long)word);
 }
 
+/**
+ * assign_bit - Assign value to a bit in memory
+ * @value: the value to assign
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ */
+static __always_inline void assign_bit(bool value, long nr,
+				       volatile unsigned long *addr)
+{
+	if (value)
+		set_bit(nr, addr);
+	else
+		clear_bit(nr, addr);
+}

include/linux/bitops.h static inline unsigned long __ffs64(u64 word) · line 252

Quoted 10 lines

include/linux/bitops.h +10 -0

+
+static __always_inline void __assign_bit(bool value, long nr,
+					 volatile unsigned long *addr)
+{
+	if (value)
+		__set_bit(nr, addr);
+	else
+		__clear_bit(nr, addr);
+}
+

I dislike the argument order, in C you naturally write: dst = src. So I would have expected:

	assign_bit(nr, addr, val);

but we have quite a few of these backwards functions in the kernel (like most of the atomic_t family) and I didn't check to see if the existing bitops are part of that 'tradition'.

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, in particular gpiolib and the upcoming driver for Maxim MAX3191x industrial serializer chips.

Cc Bart Van Assche <[email protected]>

Cc Alasdair Kergon <[email protected]>

Cc Mike Snitzer <[email protected]>

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

include/linux/bitops.h +7 -0

+static __always_inline void assign_bit(bool value, long nr,
+                                      volatile unsigned long *addr)
+{
+       if (value)
+               set_bit(nr, addr);
+       else
+               clear_bit(nr, addr);

Can someone explain when I use the bitop_foo() and when I use the __bitop_foo(). I am asking so I can understand patch 2/4 in this series.

Quoted 3 lines · Linus Walleij

Can someone explain when I use the bitop_foo() and when I use the __bitop_foo(). I am asking so I can understand patch 2/4 in this series.

That's easy. From Documentation/core-api/atomic_ops.rst:

----------------------------------------------------------------------
Finally, there are non-atomic versions of the bitmask operations provided. They are used in contexts where some other higher-level SMP locking scheme is being used to protect the bitmask, and thus less expensive non-atomic operations may be used in the implementation. They have names similar to the above bitmask operation interfaces, except that two underscores are prefixed to the interface name. ::

        void __set_bit(unsigned long nr, volatile unsigned long *addr);
        void __clear_bit(unsigned long nr, volatile unsigned long *addr);
        void __change_bit(unsigned long nr, volatile unsigned long *addr);
        int __test_and_set_bit(unsigned long nr, volatile unsigned long *addr);
        int __test_and_clear_bit(unsigned long nr, volatile unsigned long *addr);
        int __test_and_change_bit(unsigned long nr, volatile unsigned long *addr);

These non-atomic variants also do not require any special memory barrier semantics.

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.

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

Quoted 13 lines · Lukas Wunner

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

Quoted 3 lines

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.

Could you please patch
Documentation/gpio/consumer.txt
to mention the new functions as well?

Maybe we should have a section in Documentation/gpio/driver.txt, that doc is a bit incomplete right now.

Quoted 18 lines · Linus Walleij

Quoted 16 lines · Lukas Wunner

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.

Quoted 3 lines

Could you please patch
Documentation/gpio/consumer.txt
to mention the new functions as well?

Missed that, will fix in v2. There is an oddity in that document in that it claims gpiod_get_value() / gpiod_set_value() do not return errors. The kerneldoc in gpiolib.c and driver.h says otherwise, it allows the get functions to return a negative errno. Now what?

At least with SPI-attached GPIO controllers, the transmission is never guaranteed to succeed, so errors can occur both for input and output GPIOs. The MAX3191x is input-only and does pass SPI errors back to the caller. Output drivers such as gpio-74x164.c silently ignore SPI errors, which is arguably a problem.

Quoted 2 lines

Maybe we should have a section in Documentation/gpio/driver.txt, that doc is a bit incomplete right now.

First time I've seen that file. :-) I used the kerneldoc as guidance when writing the gpio-max3191x.c driver and found it sufficient, so I'm not sure how driver.txt could be expanded?

Quoted 5 lines · Lukas Wunner

At least with SPI-attached GPIO controllers, the transmission is never guaranteed to succeed, so errors can occur both for input and output GPIOs. The MAX3191x is input-only and does pass SPI errors back to the caller. Output drivers such as gpio-74x164.c silently ignore SPI errors, which is arguably a problem.

This has historical reasons.

Quoted 7 lines · Linus Walleij

Quoted 5 lines · Lukas Wunner

At least with SPI-attached GPIO controllers, the transmission is never guaranteed to succeed, so errors can occur both for input and output GPIOs. The MAX3191x is input-only and does pass SPI errors back to the caller. Output drivers such as gpio-74x164.c silently ignore SPI errors, which is arguably a problem.

Quoted 1 line

This has historical reasons.

Would you generally be open for allowing errors to be returned from the set functions as well?

The Revolution Pi uses SPI-attached
digital outputs and knowing if access to them fails would be useful.

For consumers, switching the return value from void to int shouldn't cause any fallout. They just don't check the return value initially.

However the ->set and ->set_multiple callbacks in all the GPIO drivers would need to be changed, that's where the real work is.

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.

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.

drivers/gpio/gpiolib.c +1 -0

@@ -365,28 +365,28 @@ static long linehandle_ioctl(struct file *filep, unsigned int cmd,
 	struct linehandle_state *lh = filep->private_data;
 	void __user *ip = (void __user *)arg;
 	struct gpiohandle_data ghd;
+	int vals[GPIOHANDLES_MAX];

So I'm wondering if the const keyword does more harm than good in this case. Let me know what you think.

Quoted 2 lines · Lukas Wunner

So I'm wondering if the const keyword does more harm than good in this case. Let me know what you think.

I've had similar experiences.

I'm not very sensitive about it, do whatever makes most sense to you.

What is important is that we get get_mutiple() in place.

Add device tree bindings for Maxim MAX3191x industrial serializer.

Cc Mathias Duckeck <[email protected]>

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

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

Documentation/devicetree/bindings/gpio/gpio-max3191x.txt new file +0 -0

new file mode 100644
index 000000000000..18182ecaa199

Documentation/devicetree/bindings/gpio/gpio-max3191x.txt new file +6 -0

+
+For other required and optional properties of SPI slave nodes please refer to
+../spi/spi-bus.txt.
+
+Example:
+	gpio@0 {

Documentation/devicetree/bindings/gpio/gpio-max3191x.txt new file +2 -0

+		spi-max-frequency = <25000000>;
+	};

Signature · 6 lines

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Documentation/devicetree/bindings/gpio/gpio-max3191x.txt line 15

Quoted 5 lines · Lukas Wunner

+ - modesel-gpios: GPIO pins to configure modesel of each chip.

Documentation/devicetree/bindings/gpio/gpio-max3191x.txt new file +4 -0

+			The number of GPIOs must be equal to "maxim,nchips".
+ - fault-gpios: 	GPIO pins to read undervoltage fault of each chip.
+ - db0-gpios:	GPIO pins to configure debounce of each chip.
+ - db1-gpios:	GPIO pins to configure debounce of each chip.

Perhaps an array db-gpios with 2 entries.

Review

Quoted 2 lines

+ - maxim,no-vcc24v:Boolean, whether the chips are powered through

(quoted diff) +1 -0

+			5VOUT instead of VCC24V.

Use the regulator binding here?

Thanks Rob for the helpful review!

Quoted 7 lines · Rob Herring

Quoted 5 lines · Lukas Wunner

+ - modesel-gpios:  GPIO pins to configure modesel of each chip.
+                   The number of GPIOs must be equal to "maxim,nchips".
+ - fault-gpios:    GPIO pins to read undervoltage fault of each chip.
+ - db0-gpios:      GPIO pins to configure debounce of each chip.
+ - db1-gpios:      GPIO pins to configure debounce of each chip.

Quoted 1 line

Perhaps an array db-gpios with 2 entries.

Each of the db0-gpios and db1-gpios is already an array with one pin for each chip in the daisy-chain.

So it would have to be a two-dimensional array, which AFAICS is not supported by the devicetree spec, or is it?

However I realize that for clarity I should amend fault-gpios, db0-gpios and db1-gpios with the same text as modesel-gpios:
The number of GPIOs must be equal to "maxim,nchips".

Quoted 4 lines

Quoted 2 lines

+ - maxim,no-vcc24v:Boolean, whether the chips are powered through
+ 5VOUT instead of VCC24V.

Use the regulator binding here?

I'd have to look at the regulator's current voltage to determine through which pin the chips in the daisy-chain are powered (5VOUT or VCC24V).

But if the regulator is generating 5V I couldn't discern if it's a faulting 24V supply or a non-faulting 5V supply.

So a boolean does seem necessary, however I realize now that "no-vcc24v" is misleading, I've changed it to "maxim,ignore-undervoltage" for clarity:

- maxim,ignore-undervoltage:
Boolean, whether to ignore undervoltage alarms signaled by the "maxim,fault-gpios" and by the status byte (in 16-bit mode). Use this if the chips are powered through 5VOUT instead of VCC24V, in which case they will constantly signal undervoltage.

Quoted 1 line · Lukas Wunner

Add device tree bindings for Maxim MAX3191x industrial serializer.

Quoted 2 lines

+Optional properties:
+ - maxim,nchips: Number of chips in the daisy-chain (default is 1).

Hi Rob,

sorry to bother you again, one more question popped up regarding the MAX3191x DT bindings:

Quoted 2 lines · Rob Herring

Quoted 1 line · Lukas Wunner

Add device tree bindings for Maxim MAX3191x industrial serializer.

[snip]

Quoted 2 lines

Quoted 2 lines

+Optional properties:
+ - maxim,nchips: Number of chips in the daisy-chain (default is 1).

Many I/O chips are daisy-chainable, so I've been wondering if a common property should be introduced?

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

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

Thanks,

Lukas

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

They are similar to but more versatile than the TI SN65HVS880/2/3/5 supported by gpio-pisosr.c as they clock out a CRC checksum to guard against electromagnetic interference, as well as undervoltage and overtemperature indicator bits. If these features are not needed they can be disabled by pulling the "modesel" pin high and then gpio-pisosr.c can be used instead of the present driver.

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

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