Hi Rob,

On 06/09/2017 01:13 PM, Cédric Le Goater wrote:

Hello,

quoted

quoted

quoted

Thanks for the patch.

Generally I'd split it into two patches:
1/2: addition of  LED class specific DT support
2/2: addition of GPIO support (for this one please cc also
     GPIO subsystem maintainer)

OK. I will in next version.
 

quoted

Please also see my comments below.

On 05/09/2017 08:36 AM, Cédric Le Goater wrote:

quoted

The PCA955x family of chips are I2C LED blinkers whose pins not used
to control LEDs can be used as general purpose I/Os (GPIOs).

The following adds support for device tree and Open Firmware to be
able do define different operation modes for each pin. See bindings
documentation for more details. The pca955x driver is then extended
with a gpio_chip when pins are operating in GPIO mode.

Signed-off-by: Cédric Le Goater <redacted>
---
 .../devicetree/bindings/leds/leds-pca955x.txt      | 103 ++++++++
 drivers/leds/Kconfig                               |  11 +
 drivers/leds/leds-pca955x.c                        | 290 ++++++++++++++++++---
 3 files changed, 374 insertions(+), 30 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pca955x.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-pca955x.txt b/Documentation/devicetree/bindings/leds/leds-pca955x.txt
new file mode 100644
index 000000000000..98d1053dd1b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-pca955x.txt

@@ -0,0 +1,103 @@
+LEDs connected to pca9550, pca9551, pca9552, pca9553,
+
+Required properties:
+- compatible : should be one of :
+	"nxp,pca9550"
+	"nxp,pca9551"
+	"nxp,pca9552"
+	"nxp,pca9553"
+- #address-cells: must be 1
+- #size-cells: must be 0
+- reg: I2C slave address. depends on the model.
+
+Optional properties:
+- gpio-controller: allows pins to be used as GPIOs.
+- #gpio-cells: if present, must not be 0.
+- gpio-base : base number of the pins used as GPIOs. If there are more
+              than one, they should be contiguous. See 'type' property
+              below.
+
+LED sub-node properties:
+- reg   : number of LED line.
+		from 0 to  1 in pca9550
+		from 0 to  7 in pca9551
+		from 0 to 15 in pca9552
+		from 0 to  3 in pca9553
+- compatible: either "none", "led" (default) or "gpio".
+- label : (optional)
+          see Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger : (optional)
+          see Documentation/devicetree/bindings/leds/common.txt
+
+Examples:
+
+pca9552: pca9552@60 {
+	compatible = "nxp,pca9552";
+	#address-cells = <1>;
+        #size-cells = <0>;
+	reg = <0x60>;
+
+	gpio-controller;
+	#gpio-cells = <2>;
+	gpio-base = <12>;
+
+	gpio@12 {
+		label = "GPIO12";
+		reg = <12>;
+		compatible = "gpio";
+	};
+	gpio@13 {
+		label = "GPIO13";
+		reg = <13>;
+		compatible = "gpio";
+	};
+	gpio@14 {
+		label = "GPIO14";
+		reg = <14>;
+		compatible = "gpio";
+	};
+	gpio@15 {
+		label = "GPIO15";
+		reg = <15>;
+		compatible = "gpio";
+	};

I think that for GPIO pins we should have GPIO specific bindings,
but DT maintainer would have to give his opinion here.

I have got my inspiration from the tca6507 bindings.

Right, but AFAIK there was no DT maintainer position at that time
and those bindings don't have relevant ack. I'd rather wait for
DT maintainer opinion so as to avoid spawning wrong practices.

quoted

quoted

Also I'm not sure if using compatible property this way is correct.

yes.

So, is the usage of the 'compatible' property correct ? 

If not, we could use a 'type' property with an include file defining 
the possible values. This is a common pattern.

Could you please confirm if using compatible property this way
is acceptable?

It follows Documentation/devicetree/bindings/leds/tca6507.txt.

-- 
Best regards,
Jacek Anaszewski
--
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