[PATCH v4 1/3] leds: netxbig: add device tree binding

[Simon Guinot]

On Mon, Sep 21, 2015 at 10:38:45AM -0500, Rob Herring wrote:

Hi Rob,

Thanks for your feedback. Please, see my answers below.

> On 09/17/2015 10:59 AM, Simon Guinot wrote:
> > This patch adds device tree support for the netxbig LEDs.
> > 
> > This also introduces a additionnal DT binding for the GPIO extension bus
> > (netxbig-gpio-ext) used to configure the LEDs. Since this bus could also
> > be used to control other devices, then it seems more suitable to have it
> > in a separate DT binding.
> > 
> > Signed-off-by: Simon Guinot <simon.guinot at sequanux.org>
> > Acked-by: Linus Walleij <linus.walleij at linaro.org>
> > ---
> >  .../devicetree/bindings/gpio/netxbig-gpio-ext.txt  |  22 ++
> >  .../devicetree/bindings/leds/leds-netxbig.txt      |  92 ++++++++
> >  drivers/leds/leds-netxbig.c                        | 258 +++++++++++++++++++--
> >  include/dt-bindings/leds/leds-netxbig.h            |  18 ++
> >  4 files changed, 369 insertions(+), 21 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> >  create mode 100644 Documentation/devicetree/bindings/leds/leds-netxbig.txt
> >  create mode 100644 include/dt-bindings/leds/leds-netxbig.h
> > 
> > Changes for v2:
> > - Check timer mode value retrieved from DT.
> > - In netxbig_leds_get_of_pdata, don't use unsigned long variables to get
> >   timer delay values from DT with function of_property_read_u32_index.
> >   Instead, use a temporary u32 variable. This allows to silence a static
> >   checker warning.
> > - Make timer property optional in the binding documentation. It is now
> >   aligned with the driver code.
> > 
> > Changes for v3:
> > - Fix pointer usage with the temporary u32 variable while calling
> >   of_property_read_u32_index.
> > 
> > Changes for v4:
> > - In DT binding document netxbig-gpio-ext.txt, detail byte order for
> >   registers and latch mechanism for "enable-gpio".
> > - In leds-netxbig.c, add some error messages.
> > - In leds-netxbig.c, fix some "sizeof" style issues.
> > - In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the
> >   of_property_read_string() calls after the error-prone checks.
> > - Add some Acked-by.
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> > new file mode 100644
> > index 000000000000..50ec2e690701
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> > @@ -0,0 +1,22 @@
> > +Binding for the GPIO extension bus found on some LaCie/Seagate boards
> > +(Example: 2Big/5Big Network v2, 2Big NAS).
> > +
> > +Required properties:
> > +- compatible: "lacie,netxbig-gpio-ext".
> > +- addr-gpios: GPIOs representing the address register (LSB -> MSB).
> > +- data-gpios: GPIOs representing the data register (LSB -> MSB).
> > +- enable-gpio: latches the new configuration (address, data) on raising edge.
> > +
> > +Example:
> > +
> > +netxbig_gpio_ext: netxbig-gpio-ext {
> > +	compatible = "lacie,netxbig-gpio-ext";
> > +
> > +	addr-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH
> > +		      &gpio1 16 GPIO_ACTIVE_HIGH
> > +		      &gpio1 17 GPIO_ACTIVE_HIGH>;
> > +	data-gpios = <&gpio1 12 GPIO_ACTIVE_HIGH
> > +		      &gpio1 13 GPIO_ACTIVE_HIGH
> > +		      &gpio1 14 GPIO_ACTIVE_HIGH>;
> > +	enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
> > +};
> > diff --git a/Documentation/devicetree/bindings/leds/leds-netxbig.txt b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
> > new file mode 100644
> > index 000000000000..efadbecbfeb9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
> > @@ -0,0 +1,92 @@
> > +Binding for the CPLD LEDs (GPIO extension bus) found on some LaCie/Seagate
> > +boards (Example: 2Big/5Big Network v2, 2Big NAS).
> > +
> > +Required properties:
> > +- compatible: "lacie,netxbig-leds".
> > +- gpio-ext: Phandle for the gpio-ext bus.
> 
> Would being a subnode of gpio-ext work instead?

Well, it is really unclear to me. I think that the GPIO extension bus
can be seen as a "kind of" GPIO chip. And devices which are using a GPIO
resource are not represented in DT as sub-nodes of the GPIO node. That's
why I chose to use a phandle here. 

Let me know if this representation is not correct.

> 
> > +
> > +Optional properties:
> > +- timers: Timer array. Each timer entry is represented by three integers:
> > +  Mode (gpio-ext bus), delay_on and delay_off.
> > +
> > +Each LED is represented as a sub-node of the netxbig-leds device.
> > +
> > +Required sub-node properties:
> > +- mode-addr: Mode register address on gpio-ext bus.
> > +- mode-val: Mode to value mapping. Each entry is represented by two integers:
> > +  A mode and the corresponding value on the gpio-ext bus.
> 
> I somewhat wonder if this is too low level and should just be in the
> driver instead. I guess with only 3 addr and data lines, it is okay. It
> wouldn't scale to a large number of registers though.

Even if it is not the case now, note that this values could be different
for an another board. For example, we already have some x86-based boards
(still no mainlined yet) which are using the leds-netxbig driver but
with a different gpio-ext configuration. This could happen with some
ARM-based boards too.

That's why I think it is better to have this properties in the DT rather
than hardcoded values in the driver.

> 
> > +- bright-addr: Brightness register address on gpio-ext bus.
> > +- bright-max: Maximum brightness value.
> 
> We already have a somewhat standard max-brightness property.

Yes, Jacek mentioned that too. It made the change in the v5.

Thanks,

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150922/b387f3ff/attachment.sig>