[PATCH v3 1/2] leds/tca6507: Add support for devicetree.

[PATCH v3 1/2] leds/tca6507: Add support for devicetree.

[Marek Belisko]

Support added only for leds (not for gpio's).

Signed-off-by: Marek Belisko <marek.belisko@open-nandra.com>
---

Changes from v2:
- change compatible property to "ti,tca6507"
- add documentation for linux,default-trigger

Changes from v1:
- return proper error value not NULL from tca6507_led_dt_init()
- remove empty lines
- remove kfree()

 drivers/leds/leds-tca6507.c |   70 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 67 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
index dabcf7a..fdc1ea6 100644
--- a/drivers/leds/leds-tca6507.c
+++ b/drivers/leds/leds-tca6507.c
@@ -667,6 +667,66 @@ static void tca6507_remove_gpio(struct tca6507_chip *tca)
 }
 #endif /* CONFIG_GPIOLIB */
 
+#ifdef CONFIG_OF
+static struct tca6507_platform_data * __devinit tca6507_led_dt_init(struct i2c_client *client)
+{
+	struct device_node *np = client->dev.of_node, *child;
+	struct tca6507_platform_data *pdata;
+	struct led_info *tca_leds;
+	int count = 0;
+
+	for_each_child_of_node(np, child)
+		count++;
+	if (!count)
+		return ERR_PTR(-ENODEV);
+
+	if (count > NUM_LEDS)
+		return ERR_PTR(-ENODEV);
+
+	tca_leds = devm_kzalloc(&client->dev, sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);
+	if (!tca_leds)
+		return ERR_PTR(-ENOMEM);
+
+	for_each_child_of_node(np, child) {
+		struct led_info led;
+		u32 reg;
+		int ret;
+
+		led.name = of_get_property(child, "label", NULL) ? : child->name;
+		led.default_trigger =
+			of_get_property(child, "linux,default-trigger", NULL);
+
+		ret = of_property_read_u32(child, "reg", &reg);
+
+		if (ret != 0)
+			continue;
+		tca_leds[reg] = led;
+	}
+	pdata = devm_kzalloc(&client->dev, sizeof(struct tca6507_platform_data), GFP_KERNEL);
+	if (!pdata) {
+		return ERR_PTR(-ENOMEM);
+	}
+
+	pdata->leds.leds = tca_leds;
+	pdata->leds.num_leds = NUM_LEDS;
+
+	return pdata;
+}
+
+static const struct of_device_id of_tca6507_leds_match[] = {
+	{ .compatible = "ti,tca6507", },
+	{},
+};
+
+#else
+static int __devinit tca6507_led_dt_init(struct i2c_client *client, struct tca6507_platform_data *data)
+{
+	return -1;
+}
+
+#define of_tca6507_leds_match NULL
+#endif
+
 static int __devinit tca6507_probe(struct i2c_client *client,
 				   const struct i2c_device_id *id)
 {
@@ -683,9 +743,12 @@ static int __devinit tca6507_probe(struct i2c_client *client,
 		return -EIO;
 
 	if (!pdata || pdata->leds.num_leds != NUM_LEDS) {
-		dev_err(&client->dev, "Need %d entries in platform-data list\n",
-			NUM_LEDS);
-		return -ENODEV;
+		pdata = tca6507_led_dt_init(client);
+		if (IS_ERR(pdata)) {
+			dev_err(&client->dev, "Need %d entries in platform-data list\n",
+				NUM_LEDS);
+			return PTR_ERR(pdata);
+		}
 	}
 	tca = devm_kzalloc(&client->dev, sizeof(*tca), GFP_KERNEL);
 	if (!tca)
@@ -750,6 +813,7 @@ static struct i2c_driver tca6507_driver = {
 	.driver   = {
 		.name    = "leds-tca6507",
 		.owner   = THIS_MODULE,
+		.of_match_table = of_tca6507_leds_match,
 	},
 	.probe    = tca6507_probe,
 	.remove   = __devexit_p(tca6507_remove),
-- 
1.7.9.5

[PATCH v3 2/2] Add documentation for tca6507 devicetree bindings.

[Marek Belisko]

Signed-off-by: Marek Belisko <marek.belisko@open-nandra.com>
---
 Documentation/devicetree/bindings/leds/tca6507.txt |   40 ++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/tca6507.txt

diff --git a/Documentation/devicetree/bindings/leds/tca6507.txt b/Documentation/devicetree/bindings/leds/tca6507.txt
new file mode 100644
index 0000000..a49eb0c
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/tca6507.txt
@@ -0,0 +1,40 @@
+LEDs conected to tca6507
+
+Required properties:
+- compatible : should be : "ti,tca6507".
+
+Each led is represented as a sub-node of the ti,tca6507 device.
+
+LED sub-node properties:
+- label : label for this LED
+- reg : number of LED line (could be from 0 to 6)
+- linux,default-trigger :  (optional) This parameter, if present, is a
+  string defining the trigger assigned to the LED.  Current triggers are:
+    "backlight" - LED will act as a back-light, controlled by the framebuffer
+		  system
+    "default-on" - LED will turn on
+    "heartbeat" - LED "double" flashes at a load average based rate
+    "ide-disk" - LED indicates disk activity
+    "timer" - LED flashes at a fixed, configurable rate
+
+Examples:
+
+tca6507@45 {
+	compatible = "ti,tca6507";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x45>;
+
+	led0: red_aux@0 {
+		label = "red:aux";
+		reg = <0x0>;
+	};
+
+	led1: green_aux@1 {
+		label = "green:aux";
+		reg = <0x5>;
+		linux,default-trigger = "default-on";
+	};
+
+};
+
-- 
1.7.9.5

Re: [PATCH v3 1/2] leds/tca6507: Add support for devicetree.

[Bryan Wu]

On Mon, Nov 12, 2012 at 1:25 PM, Marek Belisko
<marek.belisko@open-nandra.com> wrote:
> Support added only for leds (not for gpio's).
>
> Signed-off-by: Marek Belisko <marek.belisko@open-nandra.com>
> ---
>
> Changes from v2:
> - change compatible property to "ti,tca6507"
> - add documentation for linux,default-trigger
>
> Changes from v1:
> - return proper error value not NULL from tca6507_led_dt_init()
> - remove empty lines
> - remove kfree()
>
>  drivers/leds/leds-tca6507.c |   70 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 67 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
> index dabcf7a..fdc1ea6 100644
> --- a/drivers/leds/leds-tca6507.c
> +++ b/drivers/leds/leds-tca6507.c
> @@ -667,6 +667,66 @@ static void tca6507_remove_gpio(struct tca6507_chip *tca)
>  }
>  #endif /* CONFIG_GPIOLIB */
>
> +#ifdef CONFIG_OF
> +static struct tca6507_platform_data * __devinit tca6507_led_dt_init(struct i2c_client *client)
> +{
> +       struct device_node *np = client->dev.of_node, *child;
> +       struct tca6507_platform_data *pdata;
> +       struct led_info *tca_leds;
> +       int count = 0;
> +
> +       for_each_child_of_node(np, child)
> +               count++;
> +       if (!count)
> +               return ERR_PTR(-ENODEV);
> +
> +       if (count > NUM_LEDS)
> +               return ERR_PTR(-ENODEV);
> +
> +       tca_leds = devm_kzalloc(&client->dev, sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);

I guess it should be 'sizeof(struct led_info) * count' instead of
'sizeof(struct led_info) * NUM_LEDS'

> +       if (!tca_leds)
> +               return ERR_PTR(-ENOMEM);
> +
> +       for_each_child_of_node(np, child) {
> +               struct led_info led;
> +               u32 reg;
> +               int ret;
> +
> +               led.name = of_get_property(child, "label", NULL) ? : child->name;
> +               led.default_trigger =
> +                       of_get_property(child, "linux,default-trigger", NULL);
> +
> +               ret = of_property_read_u32(child, "reg", &reg);
> +
one more empty line

> +               if (ret != 0)
> +                       continue;

you can put one empty line here,
> +               tca_leds[reg] = led;
> +       }
> +       pdata = devm_kzalloc(&client->dev, sizeof(struct tca6507_platform_data), GFP_KERNEL);
> +       if (!pdata) {

no need {} here for just one line of this scope.

> +               return ERR_PTR(-ENOMEM);
> +       }
remove this.

> +
> +       pdata->leds.leds = tca_leds;
> +       pdata->leds.num_leds = NUM_LEDS;

I think it should '= count' instead of '= NUM_LEDS'.

> +
> +       return pdata;
> +}
> +
> +static const struct of_device_id of_tca6507_leds_match[] = {
> +       { .compatible = "ti,tca6507", },
> +       {},
> +};
> +
> +#else
> +static int __devinit tca6507_led_dt_init(struct i2c_client *client, struct tca6507_platform_data *data)
> +{
> +       return -1;
> +}
> +
> +#define of_tca6507_leds_match NULL
> +#endif
> +
>  static int __devinit tca6507_probe(struct i2c_client *client,
>                                    const struct i2c_device_id *id)
>  {
> @@ -683,9 +743,12 @@ static int __devinit tca6507_probe(struct i2c_client *client,
>                 return -EIO;
>
>         if (!pdata || pdata->leds.num_leds != NUM_LEDS) {
> -               dev_err(&client->dev, "Need %d entries in platform-data list\n",
> -                       NUM_LEDS);
> -               return -ENODEV;
> +               pdata = tca6507_led_dt_init(client);
> +               if (IS_ERR(pdata)) {
> +                       dev_err(&client->dev, "Need %d entries in platform-data list\n",
> +                               NUM_LEDS);
> +                       return PTR_ERR(pdata);
> +               }
>         }
>         tca = devm_kzalloc(&client->dev, sizeof(*tca), GFP_KERNEL);
>         if (!tca)
> @@ -750,6 +813,7 @@ static struct i2c_driver tca6507_driver = {
>         .driver   = {
>                 .name    = "leds-tca6507",
>                 .owner   = THIS_MODULE,
> +               .of_match_table = of_tca6507_leds_match,
>         },
>         .probe    = tca6507_probe,
>         .remove   = __devexit_p(tca6507_remove),
> --
> 1.7.9.5
>

Re: [PATCH v3 2/2] Add documentation for tca6507 devicetree bindings.

[Bryan Wu]

It looks fine to me, but I need a DT maintainer's ack. Stephen, could
you help on this. I will merge this patch via my tree.

Thanks,
-Bryan

On Mon, Nov 12, 2012 at 1:25 PM, Marek Belisko
<marek.belisko@open-nandra.com> wrote:
> Signed-off-by: Marek Belisko <marek.belisko@open-nandra.com>
> ---
>  Documentation/devicetree/bindings/leds/tca6507.txt |   40 ++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/tca6507.txt
>
> diff --git a/Documentation/devicetree/bindings/leds/tca6507.txt b/Documentation/devicetree/bindings/leds/tca6507.txt
> new file mode 100644
> index 0000000..a49eb0c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/tca6507.txt
> @@ -0,0 +1,40 @@
> +LEDs conected to tca6507
> +
> +Required properties:
> +- compatible : should be : "ti,tca6507".
> +
> +Each led is represented as a sub-node of the ti,tca6507 device.
> +
> +LED sub-node properties:
> +- label : label for this LED
> +- reg : number of LED line (could be from 0 to 6)
> +- linux,default-trigger :  (optional) This parameter, if present, is a
> +  string defining the trigger assigned to the LED.  Current triggers are:
> +    "backlight" - LED will act as a back-light, controlled by the framebuffer
> +                 system
> +    "default-on" - LED will turn on
> +    "heartbeat" - LED "double" flashes at a load average based rate
> +    "ide-disk" - LED indicates disk activity
> +    "timer" - LED flashes at a fixed, configurable rate
> +
> +Examples:
> +
> +tca6507@45 {
> +       compatible = "ti,tca6507";
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +       reg = <0x45>;
> +
> +       led0: red_aux@0 {
> +               label = "red:aux";
> +               reg = <0x0>;
> +       };
> +
> +       led1: green_aux@1 {
> +               label = "green:aux";
> +               reg = <0x5>;
> +               linux,default-trigger = "default-on";
> +       };
> +
> +};
> +
> --
> 1.7.9.5
>

Re: [PATCH v3 2/2] Add documentation for tca6507 devicetree bindings.

[Stephen Warren]

On 11/13/2012 06:05 PM, Bryan Wu wrote:
> It looks fine to me, but I need a DT maintainer's ack. Stephen, could
> you help on this. I will merge this patch via my tree.

I'm not actually a DT maintainer; I CC'd Grant and Rob. Marek probably
wants to repost the whole patch to them. Comments below.

>> diff --git a/Documentation/devicetree/bindings/leds/tca6507.txt b/Documentation/devicetree/bindings/leds/tca6507.txt
...
>> +LED sub-node properties:
...
>> +  string defining the trigger assigned to the LED.  Current triggers are:
>> +    "backlight" - LED will act as a back-light, controlled by the framebuffer
>> +                 system
>> +    "default-on" - LED will turn on
>> +    "heartbeat" - LED "double" flashes at a load average based rate
>> +    "ide-disk" - LED indicates disk activity
>> +    "timer" - LED flashes at a fixed, configurable rate

It would be useful to point at a single canonical document that
describes the triggers, rather than duplicating the list into every binding.

Aside from that, I think this looks reasonable.

Oh, looking at Documentation/devicetree/bindings/gpio/led.txt, should a
default-state property be supported?

Re: [PATCH v3 2/2] Add documentation for tca6507 devicetree bindings.

[Bryan Wu]

On Wed, Nov 14, 2012 at 11:26 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 11/13/2012 06:05 PM, Bryan Wu wrote:
>> It looks fine to me, but I need a DT maintainer's ack. Stephen, could
>> you help on this. I will merge this patch via my tree.
>
> I'm not actually a DT maintainer; I CC'd Grant and Rob.

Sure, thanks for this.

> Marek probably wants to repost the whole patch to them. Comments below.
>
>>> diff --git a/Documentation/devicetree/bindings/leds/tca6507.txt b/Documentation/devicetree/bindings/leds/tca6507.txt
> ...
>>> +LED sub-node properties:
> ...
>>> +  string defining the trigger assigned to the LED.  Current triggers are:
>>> +    "backlight" - LED will act as a back-light, controlled by the framebuffer
>>> +                 system
>>> +    "default-on" - LED will turn on
>>> +    "heartbeat" - LED "double" flashes at a load average based rate
>>> +    "ide-disk" - LED indicates disk activity
>>> +    "timer" - LED flashes at a fixed, configurable rate
>
> It would be useful to point at a single canonical document that
> describes the triggers, rather than duplicating the list into every binding.
>

Yeah, actually I saw several leds related DT binding txt are similar
and probably based on Documentation/devicetree/bindings/gpio/led.txt.
It's good to have a common one. Marek, are you going to do this? Thanks.

> Aside from that, I think this looks reasonable.
>
> Oh, looking at Documentation/devicetree/bindings/gpio/led.txt, should a
> default-state property be supported?

I'm not sure about this for ti,tca6507 and it really depends on the
driver implementation. But we can add this later.

-Bryan

Re: [PATCH v3 2/2] Add documentation for tca6507 devicetree bindings.

[Grant Likely]

On Mon, 12 Nov 2012 22:25:11 +0100, Marek Belisko <marek.belisko@open-nandra.com> wrote:
> Signed-off-by: Marek Belisko <marek.belisko@open-nandra.com>
> ---
>  Documentation/devicetree/bindings/leds/tca6507.txt |   40 ++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/tca6507.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/tca6507.txt b/Documentation/devicetree/bindings/leds/tca6507.txt
> new file mode 100644
> index 0000000..a49eb0c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/tca6507.txt
> @@ -0,0 +1,40 @@
> +LEDs conected to tca6507
> +
> +Required properties:
> +- compatible : should be : "ti,tca6507".
> +
> +Each led is represented as a sub-node of the ti,tca6507 device.
> +
> +LED sub-node properties:
> +- label : label for this LED
> +- reg : number of LED line (could be from 0 to 6)
> +- linux,default-trigger :  (optional) This parameter, if present, is a
> +  string defining the trigger assigned to the LED.  Current triggers are:
> +    "backlight" - LED will act as a back-light, controlled by the framebuffer
> +		  system
> +    "default-on" - LED will turn on
> +    "heartbeat" - LED "double" flashes at a load average based rate
> +    "ide-disk" - LED indicates disk activity
> +    "timer" - LED flashes at a fixed, configurable rate

This is cut-and-paste from bindings/gpio/led.txt. It should be split
into a separate file and referenced by both bindings. Otherwise the
binding looks fine.

> +
> +Examples:
> +
> +tca6507@45 {
> +	compatible = "ti,tca6507";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	reg = <0x45>;
> +
> +	led0: red_aux@0 {
> +		label = "red:aux";
> +		reg = <0x0>;
> +	};
> +
> +	led1: green_aux@1 {

Don't use underscores in node names. Use '-'. (There isn't a technical
reason for this. It is just convention)

g.