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

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

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

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Aug 23, 2017 at 4:44 AM, Lukas Wunner <lukas@wunner.de> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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