[PATCH 0/4] GPIO driver for Maxim MAX3191x

[PATCH 0/4] GPIO driver for Maxim MAX3191x

[Lukas Wunner]

GPIO driver for Maxim MAX31910, MAX31911, MAX31912, MAX31913,
MAX31953 and MAX31963 industrial serializer, a daisy-chainable
chip to make 8 digital 24V inputs available via SPI.  Supports
CRC checksums to guard against electromagnetic interference,
as well as undervoltage and overtemperature detection.

The chip is used by the "Revolution Pi" family of open source PLCs
based on the Raspberry Pi (https://revolution.kunbus.com/).

In a typical SCADA system, all input signals are read periodically,
say, every 5 or 10 ms, and stored in a so-called "process image".
To make this perform well with serializers, add a ->get_multiple
callback to struct gpio_chip, add corresponding consumer functions
and wire it up with linehandle_ioctl().

Thanks,

Lukas


Lukas Wunner (4):
  bitops: Introduce assign_bit()
  gpio: Introduce ->get_multiple callback
  dt-bindings: gpio: max3191x: Document new driver
  gpio: Add driver for Maxim MAX3191x industrial serializer

 .../devicetree/bindings/gpio/gpio-max3191x.txt     |  37 ++
 drivers/gpio/Kconfig                               |  10 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-max3191x.c                       | 482 +++++++++++++++++++++
 drivers/gpio/gpiolib.c                             | 181 +++++++-
 drivers/gpio/gpiolib.h                             |   4 +
 drivers/md/dm-mpath.c                              |   8 -
 include/linux/bitops.h                             |  24 +
 include/linux/gpio/consumer.h                      |  44 ++
 include/linux/gpio/driver.h                        |   5 +
 10 files changed, 777 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-max3191x.txt
 create mode 100644 drivers/gpio/gpio-max3191x.c

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] dt-bindings: gpio: max3191x: Document new driver

[Lukas Wunner]

Add device tree bindings for Maxim MAX3191x industrial serializer.

Cc: Mathias Duckeck <m.duckeck@kunbus.de>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 .../devicetree/bindings/gpio/gpio-max3191x.txt     | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-max3191x.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-max3191x.txt b/Documentation/devicetree/bindings/gpio/gpio-max3191x.txt
new file mode 100644
index 000000000000..18182ecaa199
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-max3191x.txt
@@ -0,0 +1,37 @@
+GPIO driver for Maxim MAX3191x industrial serializer
+
+Required properties:
+ - compatible:		"maxim,max31910", "maxim,max31911", "maxim,max31912",
+			"maxim,max31913", "maxim,max31953", "maxim,max31963"
+ - gpio-controller:	Marks the device node as a GPIO controller.
+ - #gpio-cells: 	Should be two. For consumer use see gpio.txt.
+
+Optional properties:
+ - maxim,nchips:	Number of chips in the daisy-chain (default is 1).
+ - 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.
+ - maxim,modesel:	Mode to use if hardwired (and thus not selectable
+			through GPIOs): 0 for 16-bit mode, 1 for 8-bit mode.
+ - maxim,no-vcc24v:	Boolean, whether the chips are powered through 5VOUT
+			instead of VCC24V.
+
+For other required and optional properties of SPI slave nodes please refer to
+../spi/spi-bus.txt.
+
+Example:
+	gpio@0 {
+		compatible = "maxim,max31913";
+		gpio-controller;
+		#gpio-cells = <2>;
+
+		modesel-gpios = <&gpio2 23>;
+		fault-gpios   = <&gpio2 24 GPIO_ACTIVE_LOW>;
+		db0-gpios     = <&gpio2 25>;
+		db1-gpios     = <&gpio2 26>;
+
+		reg = <0>;
+		spi-max-frequency = <25000000>;
+	};
-- 
2.11.0

Re: [PATCH 3/4] dt-bindings: gpio: max3191x: Document new driver

[Rob Herring]

On Mon, Aug 21, 2017 at 03:12:00PM +0200, Lukas Wunner wrote:
> Add device tree bindings for Maxim MAX3191x industrial serializer.
> 
> Cc: Mathias Duckeck <m.duckeck@kunbus.de>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  .../devicetree/bindings/gpio/gpio-max3191x.txt     | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-max3191x.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-max3191x.txt b/Documentation/devicetree/bindings/gpio/gpio-max3191x.txt
> new file mode 100644
> index 000000000000..18182ecaa199
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-max3191x.txt
> @@ -0,0 +1,37 @@
> +GPIO driver for Maxim MAX3191x industrial serializer
> +
> +Required properties:
> + - compatible:		"maxim,max31910", "maxim,max31911", "maxim,max31912",
> +			"maxim,max31913", "maxim,max31953", "maxim,max31963"

One valid combination per line please.

> + - gpio-controller:	Marks the device node as a GPIO controller.
> + - #gpio-cells: 	Should be two. For consumer use see gpio.txt.
> +
> +Optional properties:
> + - maxim,nchips:	Number of chips in the daisy-chain (default is 1).
> + - 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.

Perhaps an array db-gpios with 2 entries.

These gpios all need a vendor prefix.

> + - maxim,modesel:	Mode to use if hardwired (and thus not selectable
> +			through GPIOs): 0 for 16-bit mode, 1 for 8-bit mode.

Make this a boolean and define the default when not present (and 
modesel-gpios not present).

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

Use the regulator binding here?

> +
> +For other required and optional properties of SPI slave nodes please refer to
> +../spi/spi-bus.txt.
> +
> +Example:
> +	gpio@0 {
> +		compatible = "maxim,max31913";
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +
> +		modesel-gpios = <&gpio2 23>;
> +		fault-gpios   = <&gpio2 24 GPIO_ACTIVE_LOW>;
> +		db0-gpios     = <&gpio2 25>;
> +		db1-gpios     = <&gpio2 26>;
> +
> +		reg = <0>;

reg usually comes after compatible. Also, reg needs to be documented 
above.

> +		spi-max-frequency = <25000000>;
> +	};
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] dt-bindings: gpio: max3191x: Document new driver

[Lukas Wunner]

Thanks Rob for the helpful review!

On Tue, Aug 22, 2017 at 07:48:47PM -0500, Rob Herring wrote:
> On Mon, Aug 21, 2017 at 03:12:00PM +0200, Lukas Wunner wrote:
> > + - 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.
> 
> 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".


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

Thanks,

Lukas
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] dt-bindings: gpio: max3191x: Document new driver

[Rob Herring]

On Wed, Aug 23, 2017 at 4:44 AM, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> Thanks Rob for the helpful review!
>
> On Tue, Aug 22, 2017 at 07:48:47PM -0500, Rob Herring wrote:
>> On Mon, Aug 21, 2017 at 03:12:00PM +0200, Lukas Wunner wrote:
>> > + - 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.
>>
>> 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.

Okay.

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

Not in the sense that the dimensions are encoded into the property,
but the binding doc can define that. You know one dimension is 2, so
you could figure out the other. But it's fine as is.

> 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".

Really, this binding should be 1 node per chip instead, but I don't
know how you would describe that. You'd need a parent node with reg
for the chipselect, and then child nodes for each chip.

>> > + - 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).

No, the supply properties should correspond to the power pins/rails.
So it would be whichever property is present that tells you if 5V or
24V is hooked up.

> 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.

But I'm not that concerned with a single property like this if you
feel strongly about it and want to avoid the complexity of the
regulator binding.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] dt-bindings: gpio: max3191x: Document new driver

[Lukas Wunner]

Hi Rob,

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

On Tue, Aug 22, 2017 at 07:48:47PM -0500, Rob Herring wrote:
> On Mon, Aug 21, 2017 at 03:12:00PM +0200, Lukas Wunner wrote:
> > Add device tree bindings for Maxim MAX3191x industrial serializer.
[snip]
> > +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

Re: [PATCH 3/4] dt-bindings: gpio: max3191x: Document new driver

[Lukas Wunner]

Hi Rob,

gentle ping on this question:

On Tue, Sep 05, 2017 at 10:16:24AM +0200, Lukas Wunner wrote:
> Hi Rob,
> 
> sorry to bother you again, one more question popped up regarding the
> MAX3191x DT bindings:
> 
> On Tue, Aug 22, 2017 at 07:48:47PM -0500, Rob Herring wrote:
> > On Mon, Aug 21, 2017 at 03:12:00PM +0200, Lukas Wunner wrote:
> > > Add device tree bindings for Maxim MAX3191x industrial serializer.
> [snip]
> > > +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

[PATCH 4/4] gpio: Add driver for Maxim MAX3191x industrial serializer

[Lukas Wunner]

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@kunbus.de>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpio/Kconfig         |  10 +
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-max3191x.c | 482 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 493 insertions(+)
 create mode 100644 drivers/gpio/gpio-max3191x.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index fc9b75c9e3ba..c42e5e75353f 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1246,6 +1246,16 @@ config GPIO_74X164
 	  shift registers. This driver can be used to provide access
 	  to more gpio outputs.
 
+config GPIO_MAX3191X
+	tristate "Maxim MAX3191x industrial serializer"
+	select CRC8
+	help
+	  GPIO driver for Maxim MAX31910, MAX31911, MAX31912, MAX31913,
+	  MAX31953 and MAX31963 industrial serializer, a daisy-chainable
+	  chip to make 8 digital 24V inputs available via SPI.  Supports
+	  CRC checksums to guard against electromagnetic interference,
+	  as well as undervoltage and overtemperature detection.
+
 config GPIO_MAX7301
 	tristate "Maxim MAX7301 GPIO expander"
 	select GPIO_MAX730X
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 37f2029218cb..f1482ee7fd23 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_ARCH_LPC32XX)	+= gpio-lpc32xx.o
 obj-$(CONFIG_GPIO_LP873X)	+= gpio-lp873x.o
 obj-$(CONFIG_GPIO_LP87565)	+= gpio-lp87565.o
 obj-$(CONFIG_GPIO_LYNXPOINT)	+= gpio-lynxpoint.o
+obj-$(CONFIG_GPIO_MAX3191X)	+= gpio-max3191x.o
 obj-$(CONFIG_GPIO_MAX730X)	+= gpio-max730x.o
 obj-$(CONFIG_GPIO_MAX7300)	+= gpio-max7300.o
 obj-$(CONFIG_GPIO_MAX7301)	+= gpio-max7301.o
diff --git a/drivers/gpio/gpio-max3191x.c b/drivers/gpio/gpio-max3191x.c
new file mode 100644
index 000000000000..308a43b0acf0
--- /dev/null
+++ b/drivers/gpio/gpio-max3191x.c
@@ -0,0 +1,482 @@
+/*
+ * gpio-max3191x.c - GPIO driver for Maxim MAX3191x industrial serializer
+ *
+ * Copyright (C) 2017 KUNBUS GmbH
+ *
+ * The MAX3191x makes 8 digital 24V inputs available via SPI.
+ * Multiple chips can be daisy-chained, the spec does not impose
+ * a limit on the number of chips and neither does this driver.
+ *
+ * Either of two modes is selectable: In 8-bit mode, only the state
+ * of the inputs is clocked out to achieve high readout speeds;
+ * In 16-bit mode, an additional status byte is clocked out with
+ * a CRC and indicator bits for undervoltage and overtemperature.
+ * The driver returns an error instead of potentially bogus data
+ * if any of these fault conditions occur.  However it does allow
+ * readout of non-faulting chips in the same daisy-chain.
+ *
+ * MAX3191x supports four debounce settings and the driver is
+ * capable of configuring these differently for each chip in the
+ * daisy-chain.
+ *
+ * https://datasheets.maximintegrated.com/en/ds/MAX31910.pdf
+ * https://datasheets.maximintegrated.com/en/ds/MAX31911.pdf
+ * https://datasheets.maximintegrated.com/en/ds/MAX31912.pdf
+ * https://datasheets.maximintegrated.com/en/ds/MAX31913.pdf
+ * https://datasheets.maximintegrated.com/en/ds/MAX31953-MAX31963.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License (version 2) as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/bitmap.h>
+#include <linux/crc8.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+
+enum max3191x_mode {
+	STATUS_BYTE_ENABLED,
+	STATUS_BYTE_DISABLED,
+};
+
+/**
+ * struct max3191x_chip - max3191x daisy-chain
+ * @gpio: GPIO controller struct
+ * @lock: protects read sequences
+ * @nchips: number of chips in the daisy-chain
+ * @has_vcc24v: whether the chips are powered through VCC24V or 5VOUT;
+ *	if 5VOUT, the chips will constantly signal undervoltage;
+ *	for simplicity, all chips in the daisy-chain are assumed
+ *	to be powered the same way
+ * @mode_hardwired: whether the modesel pin is hardwired or configurable
+ * @mode: current mode, 0 for 16-bit, 1 for 8-bit;
+ *	for simplicity, all chips in the daisy-chain are assumed
+ *	to use the same mode
+ * @modesel_pins: GPIO pins to configure modesel of each chip
+ * @fault_pins: GPIO pins to detect fault of each chip
+ * @db0_pins: GPIO pins to configure debounce of each chip
+ * @db1_pins: GPIO pins to configure debounce of each chip
+ * @mesg: SPI message to perform a readout
+ * @xfer: SPI transfer used by @mesg
+ * @crc_error: bitmap signaling CRC error for each chip
+ * @overtemp: bitmap signaling overtemperature alarm for each chip
+ * @undervolt1: bitmap signaling undervoltage alarm for each chip
+ * @undervolt2: bitmap signaling undervoltage warning for each chip
+ * @fault: bitmap signaling assertion of @fault_pins for each chip
+ */
+struct max3191x_chip {
+	struct gpio_chip gpio;
+	struct mutex lock;
+	u32 nchips;
+	bool has_vcc24v;
+	bool mode_hardwired;
+	enum max3191x_mode mode;
+	struct gpio_descs *modesel_pins;
+	struct gpio_descs *fault_pins;
+	struct gpio_descs *db0_pins;
+	struct gpio_descs *db1_pins;
+	struct spi_message mesg;
+	struct spi_transfer xfer;
+	unsigned long *crc_error;
+	unsigned long *overtemp;
+	unsigned long *undervolt1;
+	unsigned long *undervolt2;
+	unsigned long *fault;
+};
+
+#define MAX3191X_NGPIO 8
+#define MAX3191X_CRC8_POLYNOMIAL 0xa8 /* (x^5) + x^4 + x^2 + x^0 */
+
+DECLARE_CRC8_TABLE(max3191x_crc8);
+
+static int max3191x_get_direction(struct gpio_chip *gpio, unsigned int offset)
+{
+	return 1; /* always in */
+}
+
+static int max3191x_direction_input(struct gpio_chip *gpio, unsigned int offset)
+{
+	return 0;
+}
+
+static int max3191x_direction_output(struct gpio_chip *gpio,
+				     unsigned int offset, int value)
+{
+	return -EINVAL;
+}
+
+static void max3191x_set(struct gpio_chip *gpio, unsigned int offset, int value)
+{ }
+
+static void max3191x_set_multiple(struct gpio_chip *gpio, unsigned long *mask,
+				  unsigned long *bits)
+{ }
+
+static unsigned int max3191x_wordlen(struct max3191x_chip *max3191x)
+{
+	return max3191x->mode == STATUS_BYTE_ENABLED ? 2 : 1;
+}
+
+static int max3191x_readout_locked(struct max3191x_chip *max3191x)
+{
+	struct device *dev = max3191x->gpio.parent;
+	struct spi_device *spi = to_spi_device(dev);
+	int val, i, ot = 0, uv1 = 0;
+
+	val = spi_sync(spi, &max3191x->mesg);
+	if (val) {
+		dev_err_ratelimited(dev, "SPI receive error %d\n", val);
+		return val;
+	}
+
+	for (i = 0; i < max3191x->nchips; i++) {
+		if (max3191x->mode == STATUS_BYTE_ENABLED) {
+			u8 in	  = ((u8 *)max3191x->xfer.rx_buf)[i * 2];
+			u8 status = ((u8 *)max3191x->xfer.rx_buf)[i * 2 + 1];
+
+			val = (status & 0xf8) != crc8(max3191x_crc8, &in, 1, 0);
+			__assign_bit(val, i, max3191x->crc_error);
+			if (val)
+				dev_err_ratelimited(dev,
+					"chip %d: CRC error\n", i);
+
+			ot  = (status >> 1) & 1;
+			__assign_bit(ot, i, max3191x->overtemp);
+			if (ot)
+				dev_err_ratelimited(dev,
+					"chip %d: overtemperature\n", i);
+
+			if (max3191x->has_vcc24v) {
+				uv1 = !((status >> 2) & 1);
+				__assign_bit(uv1, i, max3191x->undervolt1);
+				if (uv1)
+					dev_err_ratelimited(dev,
+						"chip %d: undervoltage\n", i);
+
+				val = !(status & 1);
+				__assign_bit(val, i, max3191x->undervolt2);
+				if (val && !uv1)
+					dev_warn_ratelimited(dev,
+						"chip %d: voltage warn\n", i);
+			}
+		}
+
+		if (max3191x->fault_pins && max3191x->has_vcc24v) {
+			val = gpiod_get_value_cansleep(
+					max3191x->fault_pins->desc[i]);
+			if (val < 0) {
+				dev_err_ratelimited(dev,
+					"GPIO read error %d\n", val);
+				return val;
+			}
+			__assign_bit(val, i, max3191x->fault);
+			if (val && !uv1 && !ot)
+				dev_err_ratelimited(dev,
+					"chip %d: fault\n", i);
+		}
+	}
+
+	return 0;
+}
+
+static bool max3191x_chip_is_faulting(struct max3191x_chip *max3191x,
+				      u16 chipnum)
+{
+	/* without status byte the only diagnostic is the fault pin */
+	if (max3191x->has_vcc24v && test_bit(chipnum, max3191x->fault))
+		return true;
+
+	if (max3191x->mode == STATUS_BYTE_DISABLED)
+		return false;
+
+	return test_bit(chipnum, max3191x->crc_error) ||
+	       test_bit(chipnum, max3191x->overtemp)  ||
+	       (max3191x->has_vcc24v &&
+		test_bit(chipnum, max3191x->undervolt1));
+}
+
+static int max3191x_get(struct gpio_chip *gpio, unsigned int offset)
+{
+	struct max3191x_chip *max3191x = gpiochip_get_data(gpio);
+	int ret, chipnum, wordlen = max3191x_wordlen(max3191x);
+	u8 in;
+
+	mutex_lock(&max3191x->lock);
+	ret = max3191x_readout_locked(max3191x);
+	if (ret)
+		goto out_unlock;
+
+	chipnum = offset / MAX3191X_NGPIO;
+	if (max3191x_chip_is_faulting(max3191x, chipnum)) {
+		ret = -EIO;
+		goto out_unlock;
+	}
+
+	in = ((u8 *)max3191x->xfer.rx_buf)[chipnum * wordlen];
+	ret = (in >> (offset % MAX3191X_NGPIO)) & 1;
+
+out_unlock:
+	mutex_unlock(&max3191x->lock);
+	return ret;
+}
+
+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, i, wordlen = max3191x_wordlen(max3191x);
+
+	mutex_lock(&max3191x->lock);
+	ret = max3191x_readout_locked(max3191x);
+	if (ret)
+		goto out_unlock;
+
+	bitmap_andnot(bits, bits, mask, gpio->ngpio); /* clear desired bits */
+
+	for (i = 0; i < max3191x->nchips; i++) {
+		unsigned long *chipmask = mask + i / sizeof(long);
+		unsigned long *chipbits = bits + i / sizeof(long);
+		int shift = MAX3191X_NGPIO * (i % sizeof(long));
+		unsigned long in;
+
+		if (!((0xff << shift) & *chipmask))
+			continue; /* not interested in inputs of this chip */
+
+		if (max3191x_chip_is_faulting(max3191x, i)) {
+			ret = -EIO;
+			goto out_unlock;
+		}
+
+		in = ((u8 *)max3191x->xfer.rx_buf)[i * wordlen] << shift;
+		*chipbits |= in & *chipmask; /* set bits for "high" inputs */
+	}
+
+out_unlock:
+	mutex_unlock(&max3191x->lock);
+	return ret;
+}
+
+static int max3191x_set_config(struct gpio_chip *gpio, unsigned int offset,
+			       unsigned long config)
+{
+	struct max3191x_chip *max3191x = gpiochip_get_data(gpio);
+	u32 debounce, chipnum, db0_val, db1_val;
+
+	if (pinconf_to_config_param(config) != PIN_CONFIG_INPUT_DEBOUNCE)
+		return -ENOTSUPP;
+
+	if (!max3191x->db0_pins || !max3191x->db1_pins)
+		return -EINVAL;
+
+	debounce = pinconf_to_config_argument(config);
+	switch (debounce) {
+	case 0:
+		db0_val = 0;
+		db1_val = 0;
+		break;
+	case 1 ... 25:
+		db0_val = 0;
+		db1_val = 1;
+		break;
+	case 26 ... 750:
+		db0_val = 1;
+		db1_val = 0;
+		break;
+	case 751 ... 3000:
+		db0_val = 1;
+		db1_val = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&max3191x->lock);
+	chipnum = offset / MAX3191X_NGPIO;
+	gpiod_set_value_cansleep(max3191x->db0_pins->desc[chipnum], db0_val);
+	gpiod_set_value_cansleep(max3191x->db1_pins->desc[chipnum], db1_val);
+	mutex_unlock(&max3191x->lock);
+	return 0;
+}
+
+static void gpiod_set_array_single_value_cansleep(unsigned int ndescs,
+						  struct gpio_desc **desc,
+						  int value)
+{
+	int i, values[ndescs];
+
+	for (i = 0; i < ndescs; i++)
+		values[i] = value;
+
+	gpiod_set_array_value_cansleep(ndescs, desc, values);
+}
+
+static struct gpio_descs *devm_gpiod_get_array_optional_count(
+				struct device *dev, const char *con_id,
+				enum gpiod_flags flags, unsigned int expected)
+{
+	struct gpio_descs *descs;
+	int found = gpiod_count(dev, con_id);
+
+	if (found == -ENOENT)
+		return NULL;
+
+	if (found != expected) {
+		dev_err(dev, "ignoring %s-gpios: found %d, expected %u\n",
+			con_id, found, expected);
+		return NULL;
+	}
+
+	descs = devm_gpiod_get_array_optional(dev, con_id, flags);
+
+	if (IS_ERR(descs)) {
+		dev_err(dev, "failed to get %s-gpios: %ld\n",
+			con_id, PTR_ERR(descs));
+		return NULL;
+	}
+
+	return descs;
+}
+
+static int max3191x_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct max3191x_chip *max3191x;
+	int n, ret;
+	u32 mode;
+
+	max3191x = devm_kzalloc(dev, sizeof(*max3191x), GFP_KERNEL);
+	if (!max3191x)
+		return -ENOMEM;
+	spi_set_drvdata(spi, max3191x);
+
+	max3191x->nchips = 1;
+	device_property_read_u32(dev, "maxim,nchips", &max3191x->nchips);
+
+	n = BITS_TO_LONGS(max3191x->nchips);
+	max3191x->crc_error   = devm_kcalloc(dev, n, sizeof(long), GFP_KERNEL);
+	max3191x->undervolt1  = devm_kcalloc(dev, n, sizeof(long), GFP_KERNEL);
+	max3191x->undervolt2  = devm_kcalloc(dev, n, sizeof(long), GFP_KERNEL);
+	max3191x->overtemp    = devm_kcalloc(dev, n, sizeof(long), GFP_KERNEL);
+	max3191x->fault       = devm_kcalloc(dev, n, sizeof(long), GFP_KERNEL);
+	max3191x->xfer.rx_buf = devm_kcalloc(dev, max3191x->nchips,
+								2, GFP_KERNEL);
+	if (!max3191x->crc_error || !max3191x->undervolt1 ||
+	    !max3191x->overtemp  || !max3191x->undervolt2 ||
+	    !max3191x->fault     || !max3191x->xfer.rx_buf)
+		return -ENOMEM;
+
+	max3191x->modesel_pins = devm_gpiod_get_array_optional_count(dev,
+				 "modesel", GPIOD_ASIS, max3191x->nchips);
+	max3191x->fault_pins   = devm_gpiod_get_array_optional_count(dev,
+				 "fault", GPIOD_IN, max3191x->nchips);
+	max3191x->db0_pins     = devm_gpiod_get_array_optional_count(dev,
+				 "db0", GPIOD_OUT_LOW, max3191x->nchips);
+	max3191x->db1_pins     = devm_gpiod_get_array_optional_count(dev,
+				 "db1", GPIOD_OUT_LOW, max3191x->nchips);
+
+	max3191x->mode = STATUS_BYTE_ENABLED;
+	if (!device_property_read_u32(dev, "maxim,modesel", &mode)) {
+		if (mode == STATUS_BYTE_ENABLED ||
+		    mode == STATUS_BYTE_DISABLED) {
+			max3191x->mode = mode;
+			max3191x->mode_hardwired = true;
+		} else {
+			dev_err(dev, "ignoring invalid maxim,modesel %d\n",
+				mode);
+		}
+	}
+	if (max3191x->modesel_pins)
+		gpiod_set_array_single_value_cansleep(
+			max3191x->modesel_pins->ndescs,
+			max3191x->modesel_pins->desc, max3191x->mode);
+
+	max3191x->has_vcc24v = !device_property_read_bool(dev,
+							  "maxim,no-vcc24v");
+
+	max3191x->xfer.len = max3191x->nchips * max3191x_wordlen(max3191x);
+	spi_message_init_with_transfers(&max3191x->mesg, &max3191x->xfer, 1);
+
+	max3191x->gpio.label = spi->modalias;
+	max3191x->gpio.owner = THIS_MODULE;
+	max3191x->gpio.parent = dev;
+	max3191x->gpio.base = -1;
+	max3191x->gpio.ngpio = max3191x->nchips * MAX3191X_NGPIO;
+	max3191x->gpio.can_sleep = true;
+
+	max3191x->gpio.get_direction = max3191x_get_direction;
+	max3191x->gpio.direction_input = max3191x_direction_input;
+	max3191x->gpio.direction_output = max3191x_direction_output;
+	max3191x->gpio.set = max3191x_set;
+	max3191x->gpio.set_multiple = max3191x_set_multiple;
+	max3191x->gpio.get = max3191x_get;
+	max3191x->gpio.get_multiple = max3191x_get_multiple;
+	max3191x->gpio.set_config = max3191x_set_config;
+
+	mutex_init(&max3191x->lock);
+
+	ret = gpiochip_add_data(&max3191x->gpio, max3191x);
+	if (ret) {
+		mutex_destroy(&max3191x->lock);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int max3191x_remove(struct spi_device *spi)
+{
+	struct max3191x_chip *max3191x = spi_get_drvdata(spi);
+
+	gpiochip_remove(&max3191x->gpio);
+	mutex_destroy(&max3191x->lock);
+
+	return 0;
+}
+
+static int __init max3191x_register_driver(struct spi_driver *sdrv)
+{
+	crc8_populate_msb(max3191x_crc8, MAX3191X_CRC8_POLYNOMIAL);
+	return spi_register_driver(sdrv);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id max3191x_of_id[] = {
+	{ .compatible = "maxim,max31910" },
+	{ .compatible = "maxim,max31911" },
+	{ .compatible = "maxim,max31912" },
+	{ .compatible = "maxim,max31913" },
+	{ .compatible = "maxim,max31953" },
+	{ .compatible = "maxim,max31963" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max3191x_of_id);
+#endif
+
+static const struct spi_device_id max3191x_spi_id[] = {
+	{ "max31910" },
+	{ "max31911" },
+	{ "max31912" },
+	{ "max31913" },
+	{ "max31953" },
+	{ "max31963" },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, max3191x_spi_id);
+
+static struct spi_driver max3191x_driver = {
+	.driver = {
+		.name		= "max3191x",
+		.of_match_table	= of_match_ptr(max3191x_of_id),
+	},
+	.probe	  = max3191x_probe,
+	.remove	  = max3191x_remove,
+	.id_table = max3191x_spi_id,
+};
+module_driver(max3191x_driver, max3191x_register_driver, spi_unregister_driver);
+
+MODULE_AUTHOR("Lukas Wunner <lukas@wunner.de>");
+MODULE_DESCRIPTION("GPIO driver for Maxim MAX3191x industrial serializer");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

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

[Linus Walleij]

On Mon, Aug 21, 2017 at 3:12 PM, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:

> 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/JSsFECOqzQB+pC5nmwQ@public.gmane.org>
> Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>

This looks like a fine piece of driver.

We just need to get the infrastructure in place and I can
merge it.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] bitops: Introduce assign_bit()

[Lukas Wunner]

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 <bart.vanassche@wdc.com>
Cc: Alasdair Kergon <agk@redhat.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/md/dm-mpath.c  |  8 --------
 include/linux/bitops.h | 24 ++++++++++++++++++++++++
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 0e8ab5bb3575..c79c113b7e7d 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -638,14 +638,6 @@ static void process_queued_bios(struct work_struct *work)
 	blk_finish_plug(&plug);
 }
 
-static void assign_bit(bool value, long nr, unsigned long *addr)
-{
-	if (value)
-		set_bit(nr, addr);
-	else
-		clear_bit(nr, addr);
-}
-
 /*
  * If we run out of usable paths, should we queue I/O or error it?
  */
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index a83c822c35c2..097af36887c0 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -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);
+}
+
+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);
+}
+
 #ifdef __KERNEL__
 
 #ifndef set_mask_bits
-- 
2.11.0

Re: [PATCH 1/4] bitops: Introduce assign_bit()

[Bart Van Assche]

On Mon, 2017-08-21 at 15:12 +0200, Lukas Wunner wrote:
> [ ... ]
> Cc: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Alasdair Kergon <agk@redhat.com>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

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.

> +/**
> + * 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);
> +}
> +
> +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);
> +}

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?

Thanks,

Bart.

Re: [PATCH 1/4] bitops: Introduce assign_bit()

[Lukas Wunner]

On Mon, Aug 21, 2017 at 04:18:44PM +0000, Bart Van Assche wrote:
> On Mon, 2017-08-21 at 15:12 +0200, Lukas Wunner wrote:
> > Cc: Bart Van Assche <bart.vanassche@wdc.com>
> > Cc: Alasdair Kergon <agk@redhat.com>
> > Cc: Mike Snitzer <snitzer@redhat.com>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> 
> 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.

Fair enough, adding more folks to cc.  Does anyone have objections
or comments to the below patch and to merging it through linux-gpio?

It's part of this series:
https://www.spinics.net/lists/linux-gpio/msg25067.html

Looking at the mnemonics of x86 and arm I couldn't find one which
would avoid the jump and be faster/shorter than the inline functions
below, so putting this in include/linux/bitops.h (rather than
arch/*/include/asm/) seemed appropriate.  Can anyone imagine
doing the same quicker with inline assembly?


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

I carried this over from existing functions, see e.g. commit 1a1d48a4a8fd
("linux/bitmap: Force inlining of bitmap weight functions").

Thanks,

Lukas

-- >8 --
Subject: [PATCH 1/4] bitops: Introduce assign_bit()

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 <bart.vanassche@wdc.com>
Cc: Alasdair Kergon <agk@redhat.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/md/dm-mpath.c  |  8 --------
 include/linux/bitops.h | 24 ++++++++++++++++++++++++
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 0e8ab5bb3575..c79c113b7e7d 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -638,14 +638,6 @@ static void process_queued_bios(struct work_struct *work)
 	blk_finish_plug(&plug);
 }
 
-static void assign_bit(bool value, long nr, unsigned long *addr)
-{
-	if (value)
-		set_bit(nr, addr);
-	else
-		clear_bit(nr, addr);
-}
-
 /*
  * If we run out of usable paths, should we queue I/O or error it?
  */
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index a83c822c35c2..097af36887c0 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -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);
+}
+
+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);
+}
+
 #ifdef __KERNEL__
 
 #ifndef set_mask_bits
-- 
2.11.0

Re: [PATCH 1/4] bitops: Introduce assign_bit()

[Peter Zijlstra]

On Tue, Aug 22, 2017 at 10:30:50AM +0200, Lukas Wunner wrote:
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index a83c822c35c2..097af36887c0 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -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);
> +}
> +
> +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'.

Re: [PATCH 1/4] bitops: Introduce assign_bit()

[Lukas Wunner]

On Tue, Aug 22, 2017 at 11:27:31AM +0200, Peter Zijlstra wrote:
> On Tue, Aug 22, 2017 at 10:30:50AM +0200, Lukas Wunner wrote:
> > diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> > index a83c822c35c2..097af36887c0 100644
> > --- a/include/linux/bitops.h
> > +++ b/include/linux/bitops.h
> > @@ -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);
> > +}
> > +
> > +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'.

The functions in include/linux/bitmap.h do follow the dst-then-src
pattern.  I carried over the argument order from Bart's function
to minimize the impact on the md subsystem, but will be happy to
respin with the order you're suggesting.  Will wait a bit though
to see if there are further comments.

Thanks,

Lukas

Re: [PATCH 1/4] bitops: Introduce assign_bit()

[Linus Walleij]

On Mon, Aug 21, 2017 at 3:12 PM, Lukas Wunner <lukas@wunner.de> wrote:

> 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 <bart.vanassche@wdc.com>
> Cc: Alasdair Kergon <agk@redhat.com>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Looks reasonable.

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

What I have never understood is the semantic difference between
bitop_foo() and __bitop_foo().

And I even use them quite a lot.

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.

I guess this is why I generally detest the __annotation, since it is
so unspecific and unclear, but that is not something I expect you
to solve in this patch.

Yours,
Linus Walleij

Re: [PATCH 1/4] bitops: Introduce assign_bit()

[Bart Van Assche]

On Wed, 2017-08-23 at 09:32 +0200, Linus Walleij wrote:
> 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.
----------------------------------------------------------------------

Bart.

Re: [PATCH 1/4] bitops: Introduce assign_bit()

[Linus Walleij]

On Wed, Aug 23, 2017 at 7:09 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Wed, 2017-08-23 at 09:32 +0200, Linus Walleij wrote:
>> 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.

All right that makes sense.

On Rusty Russell's API ladder
http://sweng.the-davies.net/Home/rustys-api-design-manifesto
this scores 3/10 "read the documentation and you will get
it right".

We could raise it to 6-7 by renaming them all
set_bit_nonatomic() etc (or even set_bit_na()), I almost feel like
sending a script to Torvalds to do that after the merge window, but I
fear some people would beat me up for it.

Yours,
Linus Walleij

[PATCH 2/4] gpio: Introduce ->get_multiple callback

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

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpio/gpiolib.c        | 181 +++++++++++++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |   4 +
 include/linux/gpio/consumer.h |  44 ++++++++++
 include/linux/gpio/driver.h   |   5 ++
 4 files changed, 223 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 18bba1748a35..ea02916df0ad 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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];
 	int i;
 
 	if (cmd == GPIOHANDLE_GET_LINE_VALUES_IOCTL) {
-		int val;
+		/* TODO: check if descriptors are really input */
+		int ret = gpiod_get_array_value_complex(false,
+							true,
+							lh->numdescs,
+			     (const struct gpio_desc **)lh->descs,
+							vals);
+		if (ret)
+			return ret;
 
 		memset(&ghd, 0, sizeof(ghd));
-
-		/* TODO: check if descriptors are really input */
-		for (i = 0; i < lh->numdescs; i++) {
-			val = gpiod_get_value_cansleep(lh->descs[i]);
-			if (val < 0)
-				return val;
-			ghd.values[i] = val;
-		}
+		for (i = 0; i < lh->numdescs; i++)
+			ghd.values[i] = vals[i];
 
 		if (copy_to_user(ip, &ghd, sizeof(ghd)))
 			return -EFAULT;
 
 		return 0;
 	} else if (cmd == GPIOHANDLE_SET_LINE_VALUES_IOCTL) {
-		int vals[GPIOHANDLES_MAX];
-
 		/* TODO: check if descriptors are really output */
 		if (copy_from_user(&ghd, ip, sizeof(ghd)))
 			return -EFAULT;
@@ -2473,6 +2473,71 @@ static int _gpiod_get_raw_value(const struct gpio_desc *desc)
 	return value;
 }
 
+static int gpio_chip_get_multiple(struct gpio_chip *chip,
+				  unsigned long *mask, unsigned long *bits)
+{
+	if (chip->get_multiple) {
+		return chip->get_multiple(chip, mask, bits);
+	} else if (chip->get) {
+		int i, value;
+
+		for_each_set_bit(i, mask, chip->ngpio) {
+			value = chip->get(chip, i);
+			if (value < 0)
+				return value;
+			__assign_bit(value, i, bits);
+		}
+		return 0;
+	}
+	return -EIO;
+}
+
+int gpiod_get_array_value_complex(bool raw, bool can_sleep,
+				  unsigned int array_size,
+				  const struct gpio_desc **desc_array,
+				  int *value_array)
+{
+	int i = 0;
+
+	while (i < array_size) {
+		struct gpio_chip *chip = desc_array[i]->gdev->chip;
+		unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
+		unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
+		int first, j, ret;
+
+		if (!can_sleep)
+			WARN_ON(chip->can_sleep);
+
+		/* collect all inputs belonging to the same chip */
+		first = i;
+		memset(mask, 0, sizeof(mask));
+		do {
+			const struct gpio_desc *desc = desc_array[i];
+			int hwgpio = gpio_chip_hwgpio(desc);
+
+			__set_bit(hwgpio, mask);
+			i++;
+		} while ((i < array_size) &&
+			 (desc_array[i]->gdev->chip == chip));
+
+		ret = gpio_chip_get_multiple(chip, mask, bits);
+		if (ret)
+			return ret;
+
+		for (j = first; j < i; j++) {
+			const struct gpio_desc *desc = desc_array[j];
+			int hwgpio = gpio_chip_hwgpio(desc);
+			int value = test_bit(hwgpio, bits);
+
+			if (!raw && test_bit(FLAG_ACTIVE_LOW, &desc->flags))
+				value = !value;
+			value_array[j] = value;
+			trace_gpio_value(desc_to_gpio(desc), 1, value);
+		}
+	}
+	return 0;
+}
+
 /**
  * gpiod_get_raw_value() - return a gpio's raw value
  * @desc: gpio whose value will be returned
@@ -2521,6 +2586,53 @@ int gpiod_get_value(const struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_get_value);
 
+/**
+ * gpiod_get_raw_array_value() - read raw values from an array of GPIOs
+ * @array_size: number of elements in the descriptor / value arrays
+ * @desc_array: array of GPIO descriptors whose values will be read
+ * @value_array: array to store the read values
+ *
+ * Read the raw values of the GPIOs, i.e. the values of the physical lines
+ * without regard for their ACTIVE_LOW status.  Return 0 in case of success,
+ * else an error code.
+ *
+ * This function should be called from contexts where we cannot sleep,
+ * and it will complain if the GPIO chip functions potentially sleep.
+ */
+int gpiod_get_raw_array_value(unsigned int array_size,
+			      const struct gpio_desc **desc_array,
+			      int *value_array)
+{
+	if (!desc_array)
+		return -EINVAL;
+	return gpiod_get_array_value_complex(true, false, array_size,
+					     desc_array, value_array);
+}
+EXPORT_SYMBOL_GPL(gpiod_get_raw_array_value);
+
+/**
+ * gpiod_get_array_value() - read values from an array of GPIOs
+ * @array_size: number of elements in the descriptor / value arrays
+ * @desc_array: array of GPIO descriptors whose values will be read
+ * @value_array: array to store the read values
+ *
+ * Read the logical values of the GPIOs, i.e. taking their ACTIVE_LOW status
+ * into account.  Return 0 in case of success, else an error code.
+ *
+ * This function should be called from contexts where we cannot sleep,
+ * and it will complain if the GPIO chip functions potentially sleep.
+ */
+int gpiod_get_array_value(unsigned int array_size,
+			  const struct gpio_desc **desc_array,
+			  int *value_array)
+{
+	if (!desc_array)
+		return -EINVAL;
+	return gpiod_get_array_value_complex(false, false, array_size,
+					     desc_array, value_array);
+}
+EXPORT_SYMBOL_GPL(gpiod_get_array_value);
+
 /*
  *  _gpio_set_open_drain_value() - Set the open drain gpio's value.
  * @desc: gpio descriptor whose state need to be set.
@@ -2950,6 +3062,53 @@ int gpiod_get_value_cansleep(const struct gpio_desc *desc)
 EXPORT_SYMBOL_GPL(gpiod_get_value_cansleep);
 
 /**
+ * gpiod_get_raw_array_value_cansleep() - read raw values from an array of GPIOs
+ * @array_size: number of elements in the descriptor / value arrays
+ * @desc_array: array of GPIO descriptors whose values will be read
+ * @value_array: array to store the read values
+ *
+ * Read the raw values of the GPIOs, i.e. the values of the physical lines
+ * without regard for their ACTIVE_LOW status.  Return 0 in case of success,
+ * else an error code.
+ *
+ * This function is to be called from contexts that can sleep.
+ */
+int gpiod_get_raw_array_value_cansleep(unsigned int array_size,
+				       const struct gpio_desc **desc_array,
+				       int *value_array)
+{
+	might_sleep_if(extra_checks);
+	if (!desc_array)
+		return -EINVAL;
+	return gpiod_get_array_value_complex(true, true, array_size,
+					     desc_array, value_array);
+}
+EXPORT_SYMBOL_GPL(gpiod_get_raw_array_value_cansleep);
+
+/**
+ * gpiod_get_array_value_cansleep() - read values from an array of GPIOs
+ * @array_size: number of elements in the descriptor / value arrays
+ * @desc_array: array of GPIO descriptors whose values will be read
+ * @value_array: array to store the read values
+ *
+ * Read the logical values of the GPIOs, i.e. taking their ACTIVE_LOW status
+ * into account.  Return 0 in case of success, else an error code.
+ *
+ * This function is to be called from contexts that can sleep.
+ */
+int gpiod_get_array_value_cansleep(unsigned int array_size,
+				   const struct gpio_desc **desc_array,
+				   int *value_array)
+{
+	might_sleep_if(extra_checks);
+	if (!desc_array)
+		return -EINVAL;
+	return gpiod_get_array_value_complex(false, true, array_size,
+					     desc_array, value_array);
+}
+EXPORT_SYMBOL_GPL(gpiod_get_array_value_cansleep);
+
+/**
  * gpiod_set_raw_value_cansleep() - assign a gpio's raw value
  * @desc: gpio whose value will be assigned
  * @value: value to assign
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index d003ccb12781..fe75ed71c90f 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -180,6 +180,10 @@ static inline bool acpi_can_fallback_to_crs(struct acpi_device *adev,
 #endif
 
 struct gpio_desc *gpiochip_get_desc(struct gpio_chip *chip, u16 hwnum);
+int gpiod_get_array_value_complex(bool raw, bool can_sleep,
+				  unsigned int array_size,
+				  const struct gpio_desc **desc_array,
+				  int *value_array);
 void gpiod_set_array_value_complex(bool raw, bool can_sleep,
 				   unsigned int array_size,
 				   struct gpio_desc **desc_array,
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 8f702fcbe485..0dfbf5a9349b 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -99,10 +99,16 @@ int gpiod_direction_output_raw(struct gpio_desc *desc, int value);
 
 /* Value get/set from non-sleeping context */
 int gpiod_get_value(const struct gpio_desc *desc);
+int gpiod_get_array_value(unsigned int array_size,
+			  const struct gpio_desc **desc_array,
+			  int *value_array);
 void gpiod_set_value(struct gpio_desc *desc, int value);
 void gpiod_set_array_value(unsigned int array_size,
 			   struct gpio_desc **desc_array, int *value_array);
 int gpiod_get_raw_value(const struct gpio_desc *desc);
+int gpiod_get_raw_array_value(unsigned int array_size,
+			      const struct gpio_desc **desc_array,
+			      int *value_array);
 void gpiod_set_raw_value(struct gpio_desc *desc, int value);
 void gpiod_set_raw_array_value(unsigned int array_size,
 			       struct gpio_desc **desc_array,
@@ -110,11 +116,17 @@ void gpiod_set_raw_array_value(unsigned int array_size,
 
 /* Value get/set from sleeping context */
 int gpiod_get_value_cansleep(const struct gpio_desc *desc);
+int gpiod_get_array_value_cansleep(unsigned int array_size,
+				   const struct gpio_desc **desc_array,
+				   int *value_array);
 void gpiod_set_value_cansleep(struct gpio_desc *desc, int value);
 void gpiod_set_array_value_cansleep(unsigned int array_size,
 				    struct gpio_desc **desc_array,
 				    int *value_array);
 int gpiod_get_raw_value_cansleep(const struct gpio_desc *desc);
+int gpiod_get_raw_array_value_cansleep(unsigned int array_size,
+				       const struct gpio_desc **desc_array,
+				       int *value_array);
 void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value);
 void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
 					struct gpio_desc **desc_array,
@@ -305,6 +317,14 @@ static inline int gpiod_get_value(const struct gpio_desc *desc)
 	WARN_ON(1);
 	return 0;
 }
+static inline int gpiod_get_array_value(unsigned int array_size,
+					const struct gpio_desc **desc_array,
+					int *value_array)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+	return 0;
+}
 static inline void gpiod_set_value(struct gpio_desc *desc, int value)
 {
 	/* GPIO can never have been requested */
@@ -323,6 +343,14 @@ static inline int gpiod_get_raw_value(const struct gpio_desc *desc)
 	WARN_ON(1);
 	return 0;
 }
+static inline int gpiod_get_raw_array_value(unsigned int array_size,
+					    const struct gpio_desc **desc_array,
+					    int *value_array)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+	return 0;
+}
 static inline void gpiod_set_raw_value(struct gpio_desc *desc, int value)
 {
 	/* GPIO can never have been requested */
@@ -342,6 +370,14 @@ static inline int gpiod_get_value_cansleep(const struct gpio_desc *desc)
 	WARN_ON(1);
 	return 0;
 }
+static inline int gpiod_get_array_value_cansleep(unsigned int array_size,
+				     const struct gpio_desc **desc_array,
+				     int *value_array)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+	return 0;
+}
 static inline void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
 {
 	/* GPIO can never have been requested */
@@ -360,6 +396,14 @@ static inline int gpiod_get_raw_value_cansleep(const struct gpio_desc *desc)
 	WARN_ON(1);
 	return 0;
 }
+static inline int gpiod_get_raw_array_value_cansleep(unsigned int array_size,
+					 const struct gpio_desc **desc_array,
+					 int *value_array)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+	return 0;
+}
 static inline void gpiod_set_raw_value_cansleep(struct gpio_desc *desc,
 						int value)
 {
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index c97f8325e8bf..f793530a8a1a 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -35,6 +35,8 @@ struct module;
  * @direction_input: configures signal "offset" as input, or returns error
  * @direction_output: configures signal "offset" as output, or returns error
  * @get: returns value for signal "offset", 0=low, 1=high, or negative error
+ * @get_multiple: reads values for multiple signals defined by "mask" and
+ *	stores them in "bits", returns 0 on success or negative error
  * @set: assigns output value for signal "offset"
  * @set_multiple: assigns output values for multiple signals defined by "mask"
  * @set_config: optional hook for all kinds of settings. Uses the same
@@ -126,6 +128,9 @@ struct gpio_chip {
 						unsigned offset, int value);
 	int			(*get)(struct gpio_chip *chip,
 						unsigned offset);
+	int			(*get_multiple)(struct gpio_chip *chip,
+						unsigned long *mask,
+						unsigned long *bits);
 	void			(*set)(struct gpio_chip *chip,
 						unsigned offset, int value);
 	void			(*set_multiple)(struct gpio_chip *chip,
-- 
2.11.0

Re: [PATCH 2/4] gpio: Introduce ->get_multiple callback

[Linus Walleij]

On Mon, Aug 21, 2017 at 3:12 PM, Lukas Wunner <lukas@wunner.de> wrote:

> 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 <lukas@wunner.de>

Lean, mean, and clean and introducing the interface that I've
been missing for some time. Good work!

Let's hope we can merge this early for v4.15 once v4.14-rc1 is out
and pile some drivers supporting it on top of it too.

I am only worried on how to get __assign_bit() into place in time,
or whether I should carry that patch in the GPIO tree simply?
We *could* add a local version of it and then switch it over
with a follow-up patch...

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.

Yours,
Linus Walleij

Re: [PATCH 2/4] gpio: Introduce ->get_multiple callback

[Lukas Wunner]

On Wed, Aug 23, 2017 at 09:38:21AM +0200, Linus Walleij wrote:
> On Mon, Aug 21, 2017 at 3:12 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > 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.
> 
> 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?

Here are the two occurrences of the false claim in consumer.txt:

	It should be checked, since the get/set calls don't return errors
	and since misconfiguration is possible.

	The get/set calls do not return errors because "invalid GPIO"
	should have been reported earlier from gpiod_direction_*().

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.

> 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?

Thanks!

Lukas

Re: [PATCH 2/4] gpio: Introduce ->get_multiple callback

[Linus Walleij]

On Sun, Aug 27, 2017 at 7:34 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Wed, Aug 23, 2017 at 09:38:21AM +0200, Linus Walleij wrote:
>> On Mon, Aug 21, 2017 at 3:12 PM, Lukas Wunner <lukas@wunner.de> wrote:
>> > 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.
>>
>> 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?

Probably errors in the documentation.

Feel free to correct it.

> 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.

>> 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?

Don't worry about it if you feel it's superfluous.

Yours,
Linus Walleij

Re: [PATCH 2/4] gpio: Introduce ->get_multiple callback

[Lukas Wunner]

On Thu, Aug 31, 2017 at 03:48:15PM +0200, Linus Walleij wrote:
> On Sun, Aug 27, 2017 at 7:34 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > 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.

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.

Thanks,

Lukas

Re: [PATCH 2/4] gpio: Introduce ->get_multiple callback

[Linus Walleij]

On Thu, Aug 31, 2017 at 5:46 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Thu, Aug 31, 2017 at 03:48:15PM +0200, Linus Walleij wrote:
>> On Sun, Aug 27, 2017 at 7:34 PM, Lukas Wunner <lukas@wunner.de> wrote:
>> > 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.
>
> Would you generally be open for allowing errors to be returned from
> the set functions as well?

Yes if we can make a series fixing the fallout.

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

True.

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

Indeed.

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

Yeah :/

But there are not so many of them, luckily.

Yours,
Linus Walleij

Re: [PATCH 2/4] gpio: Introduce ->get_multiple callback

[Lukas Wunner]

Hi Linus,

one question popped up regarding this patch that I'd like to clarify
before reposting a revised series:

On Mon, Aug 21, 2017 at 03:12:00PM +0200, Lukas Wunner wrote:
> 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.
[...]
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -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];
>  	int i;
>  
>  	if (cmd == GPIOHANDLE_GET_LINE_VALUES_IOCTL) {
> -		int val;
> +		/* TODO: check if descriptors are really input */
> +		int ret = gpiod_get_array_value_complex(false,
> +							true,
> +							lh->numdescs,
> +			     (const struct gpio_desc **)lh->descs,
> +							vals);

I've designed the function signature of

    gpiod_get_array_value_complex()
    gpiod_get_raw_array_value()
    gpiod_get_array_value()
    gpiod_get_raw_array_value_cansleep()
    gpiod_get_array_value_cansleep()

such that a "const struct gpio_desc **" argument is passed in (the point
being the const).  This was done to enforce const-correctness and for
consistency with the family of functions to read a single GPIO, such as
gpiod_get_value() which take a "const struct gpio_desc *".

However after actually using the newly introduced functions in a driver,
I've discovered that the const keyword is mostly just an annoyance in this
case:  When acquiring GPIOs with gpiod_get_array(), the resulting
struct gpio_desc ** is not const.  Thus, a cast is necessary whenever
feeding the result of gpiod_get_array() to gpiod_get_array_value(),
which may well be the most common use case.  (It's probably less common
that folks construct the array by hand.)  A cast is also already necessary
for the invocation of gpiod_get_array_value_complex() quoted above.

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

Thanks,

Lukas

Re: [PATCH 2/4] gpio: Introduce ->get_multiple callback

[Linus Walleij]

On Wed, Oct 4, 2017 at 10:32 PM, Lukas Wunner <lukas@wunner.de> wrote:

> 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.

Yours,
Linus Walleij

Re: [PATCH 2/4] gpio: Introduce ->get_multiple callback

[Lukas Wunner]

On Sat, Oct 07, 2017 at 01:23:17PM +0200, Linus Walleij wrote:
> On Wed, Oct 4, 2017 at 10:32 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > 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.

Yes.  My apologies for the delay.

I've dropped the const qualifier, the rationale for the compiler error
it triggers is given in:

http://c-faq.com/ansi/constmismatch.html

Briefly, it is legal to pass a pointer to non-const if the function
signature specifies const, but not for a pointer-to-pointer, i.e.
a second level of indirection.  The error is supposed to "help" the
developer keep the const promises but I don't find it helpful at all.
It necessitates a cast unless a const struct gpio_desc ** is passed in.
When would that be the case in reality?  Only if the array is statically
declared, but usually the gpio_desc structs are obtained at runtime,
so the the cast would likely *always* be needed.

Thanks,

Lukas