[PATCH 2/2] LEDS: tca6502: add device-tree support for GPIO configuration.

[PATCH 2/2] LEDS: tca6502: add device-tree support for GPIO configuration.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]


The 7 lines driven by the TCA6507 can either drive LEDs or act as output-only
GPIOs.

To make this distinction in devicetree we use the "compatible" property.

If the device attached to a line is "compatible" with "gpio", we treat it
like a GPIO.  If it is "compatible" with "led" (or if no "compatible" value
is set) we treat it like an LED.

Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/Documentation/devicetree/bindings/leds/tca6507.txt b/Documentation/devicetree/bindings/leds/tca6507.txt
index 80ff3dfb1f32..d7221b84987c 100644
--- a/Documentation/devicetree/bindings/leds/tca6507.txt
+++ b/Documentation/devicetree/bindings/leds/tca6507.txt
@@ -2,6 +2,13 @@ LEDs connected to tca6507
 
 Required properties:
 - compatible : should be : "ti,tca6507".
+- #address-cells: must be 1
+- #size-cells: must be 0
+- reg: typically 0x45.
+
+Optional properties:
+- gpio-controller: allows lines to be used as output-only GPIOs.
+- #gpio-cells: if present, must be 0.
 
 Each led is represented as a sub-node of the ti,tca6507 device.
 
@@ -10,6 +17,7 @@ LED sub-node properties:
 - reg : number of LED line (could be from 0 to 6)
 - linux,default-trigger : (optional)
    see Documentation/devicetree/bindings/leds/common.txt
+- compatible: either "led" (the default) or "gpio".
 
 Examples:
 
@@ -19,6 +27,9 @@ tca6507@45 {
 	#size-cells = <0>;
 	reg = <0x45>;
 
+	gpio-controller;
+	#gpio-cells = <2>;
+
 	led0: red-aux@0 {
 		label = "red:aux";
 		reg = <0x0>;
@@ -29,5 +40,10 @@ tca6507@45 {
 		reg = <0x5>;
 		linux,default-trigger = "default-on";
 	};
+
+	wifi-reset@6 {
+		reg = <0x6>;
+		compatible = "gpio";
+	};
 };
 
diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
index f5063f447463..93a2b1759054 100644
--- a/drivers/leds/leds-tca6507.c
+++ b/drivers/leds/leds-tca6507.c
@@ -638,6 +638,9 @@ static int tca6507_probe_gpios(struct i2c_client *client,
 	tca->gpio.direction_output = tca6507_gpio_direction_output;
 	tca->gpio.set = tca6507_gpio_set_value;
 	tca->gpio.dev = &client->dev;
+#ifdef CONFIG_OF_GPIO
+	tca->gpio.of_node = of_node_get(client->dev.of_node);
+#endif
 	err = gpiochip_add(&tca->gpio);
 	if (err) {
 		tca->gpio.ngpio = 0;
@@ -696,6 +699,8 @@ tca6507_led_dt_init(struct i2c_client *client)
 		led.default_trigger =
 			of_get_property(child, "linux,default-trigger", NULL);
 		led.flags = 0;
+		if (of_property_match_string(child, "compatible", "gpio") >= 0)
+			led.flags |= TCA6507_MAKE_GPIO;
 		ret = of_property_read_u32(child, "reg", &reg);
 		if (ret != 0 || reg < 0 || reg >= NUM_LEDS)
 			continue;
@@ -709,6 +714,7 @@ tca6507_led_dt_init(struct i2c_client *client)
 
 	pdata->leds.leds = tca_leds;
 	pdata->leds.num_leds = NUM_LEDS;
+	pdata->gpio_base = -1;
 
 	return pdata;
 }

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/2] LEDS: tca6502: add device-tree support for GPIO configuration.

[Bryan Wu]

On Thu, Oct 31, 2013 at 7:41 PM, NeilBrown <neilb@suse.de> wrote:
>

[PATCH 2/2] LEDS: tca6502: add device-tree support for GPIO configuration.

I think it should be tca6507, right? Typo?

For other parts of this patch, I'm OK for them.

And just a quick scan of the leds-tca6507, I found bunch of typos in
the comments:

* An led-tca6507 device must be provided with platform data.  This data
 * lists for each output: the name, default trigger, and whether the signal
 * is being used as a GPiO rather than an led.  'struct led_plaform_data'

Can we unify to use GPIO and gpio?

 * is used for this.  If 'name' is NULL, the output isn't used.  If 'flags'
 * is TCA6507_MAKE_CPIO, the output is a GPO.

Here should be TCA6507_MAKE_GPIO and GPIO instead of GPO

 * The "struct led_platform_data" can be embedded in a
 * "struct tca6507_platform_data" which adds a 'gpio_base' for the GPiOs,
 * and a 'setup' callback which is called once the GPiOs are available.

Don't use GPiO, please.

Could you please review the code again and submit a cleanup patch to
fix those typos?

Thanks,
-Bryan


> The 7 lines driven by the TCA6507 can either drive LEDs or act as output-only
> GPIOs.
>
> To make this distinction in devicetree we use the "compatible" property.
>
> If the device attached to a line is "compatible" with "gpio", we treat it
> like a GPIO.  If it is "compatible" with "led" (or if no "compatible" value
> is set) we treat it like an LED.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
>
> diff --git a/Documentation/devicetree/bindings/leds/tca6507.txt b/Documentation/devicetree/bindings/leds/tca6507.txt
> index 80ff3dfb1f32..d7221b84987c 100644
> --- a/Documentation/devicetree/bindings/leds/tca6507.txt
> +++ b/Documentation/devicetree/bindings/leds/tca6507.txt
> @@ -2,6 +2,13 @@ LEDs connected to tca6507
>
>  Required properties:
>  - compatible : should be : "ti,tca6507".
> +- #address-cells: must be 1
> +- #size-cells: must be 0
> +- reg: typically 0x45.
> +
> +Optional properties:
> +- gpio-controller: allows lines to be used as output-only GPIOs.
> +- #gpio-cells: if present, must be 0.
>
>  Each led is represented as a sub-node of the ti,tca6507 device.
>
> @@ -10,6 +17,7 @@ LED sub-node properties:
>  - reg : number of LED line (could be from 0 to 6)
>  - linux,default-trigger : (optional)
>     see Documentation/devicetree/bindings/leds/common.txt
> +- compatible: either "led" (the default) or "gpio".
>
>  Examples:
>
> @@ -19,6 +27,9 @@ tca6507@45 {
>         #size-cells = <0>;
>         reg = <0x45>;
>
> +       gpio-controller;
> +       #gpio-cells = <2>;
> +
>         led0: red-aux@0 {
>                 label = "red:aux";
>                 reg = <0x0>;
> @@ -29,5 +40,10 @@ tca6507@45 {
>                 reg = <0x5>;
>                 linux,default-trigger = "default-on";
>         };
> +
> +       wifi-reset@6 {
> +               reg = <0x6>;
> +               compatible = "gpio";
> +       };
>  };
>
> diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
> index f5063f447463..93a2b1759054 100644
> --- a/drivers/leds/leds-tca6507.c
> +++ b/drivers/leds/leds-tca6507.c
> @@ -638,6 +638,9 @@ static int tca6507_probe_gpios(struct i2c_client *client,
>         tca->gpio.direction_output = tca6507_gpio_direction_output;
>         tca->gpio.set = tca6507_gpio_set_value;
>         tca->gpio.dev = &client->dev;
> +#ifdef CONFIG_OF_GPIO
> +       tca->gpio.of_node = of_node_get(client->dev.of_node);
> +#endif
>         err = gpiochip_add(&tca->gpio);
>         if (err) {
>                 tca->gpio.ngpio = 0;
> @@ -696,6 +699,8 @@ tca6507_led_dt_init(struct i2c_client *client)
>                 led.default_trigger =
>                         of_get_property(child, "linux,default-trigger", NULL);
>                 led.flags = 0;
> +               if (of_property_match_string(child, "compatible", "gpio") >= 0)
> +                       led.flags |= TCA6507_MAKE_GPIO;
>                 ret = of_property_read_u32(child, "reg", &reg);
>                 if (ret != 0 || reg < 0 || reg >= NUM_LEDS)
>                         continue;
> @@ -709,6 +714,7 @@ tca6507_led_dt_init(struct i2c_client *client)
>
>         pdata->leds.leds = tca_leds;
>         pdata->leds.num_leds = NUM_LEDS;
> +       pdata->gpio_base = -1;
>
>         return pdata;
>  }

Re: [PATCH 2/2] LEDS: tca6502: add device-tree support for GPIO configuration.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

On Thu, 7 Nov 2013 15:39:00 -0800 Bryan Wu <cooloney@gmail.com> wrote:

> On Thu, Oct 31, 2013 at 7:41 PM, NeilBrown <neilb@suse.de> wrote:
> >
> 
> [PATCH 2/2] LEDS: tca6502: add device-tree support for GPIO configuration.
> 
> I think it should be tca6507, right? Typo?

Obviously I'm still dreaming of the Apple IIe.  Yes, a typo.

> 
> For other parts of this patch, I'm OK for them.
> 
> And just a quick scan of the leds-tca6507, I found bunch of typos in
> the comments:
> 
> * An led-tca6507 device must be provided with platform data.  This data
>  * lists for each output: the name, default trigger, and whether the signal
>  * is being used as a GPiO rather than an led.  'struct led_plaform_data'
> 
> Can we unify to use GPIO and gpio?
> 
>  * is used for this.  If 'name' is NULL, the output isn't used.  If 'flags'
>  * is TCA6507_MAKE_CPIO, the output is a GPO.
> 
> Here should be TCA6507_MAKE_GPIO and GPIO instead of GPO
> 
>  * The "struct led_platform_data" can be embedded in a
>  * "struct tca6507_platform_data" which adds a 'gpio_base' for the GPiOs,
>  * and a 'setup' callback which is called once the GPiOs are available.
> 
> Don't use GPiO, please.
> 
> Could you please review the code again and submit a cleanup patch to
> fix those typos?

Yes, I'll send a patch shortly.

Thanks,
NeilBrown



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/2] LEDS: tca6502: add device-tree support for GPIO configuration.

[Bryan Wu]

On Thu, Nov 7, 2013 at 3:39 PM, Bryan Wu <cooloney@gmail.com> wrote:
> On Thu, Oct 31, 2013 at 7:41 PM, NeilBrown <neilb@suse.de> wrote:
>>
>
> [PATCH 2/2] LEDS: tca6502: add device-tree support for GPIO configuration.
>
> I think it should be tca6507, right? Typo?
>

I fixed this typo and queue up this patch in my devel branch.

-Bryan

> For other parts of this patch, I'm OK for them.
>
> And just a quick scan of the leds-tca6507, I found bunch of typos in
> the comments:
>
> * An led-tca6507 device must be provided with platform data.  This data
>  * lists for each output: the name, default trigger, and whether the signal
>  * is being used as a GPiO rather than an led.  'struct led_plaform_data'
>
> Can we unify to use GPIO and gpio?
>
>  * is used for this.  If 'name' is NULL, the output isn't used.  If 'flags'
>  * is TCA6507_MAKE_CPIO, the output is a GPO.
>
> Here should be TCA6507_MAKE_GPIO and GPIO instead of GPO
>
>  * The "struct led_platform_data" can be embedded in a
>  * "struct tca6507_platform_data" which adds a 'gpio_base' for the GPiOs,
>  * and a 'setup' callback which is called once the GPiOs are available.
>
> Don't use GPiO, please.
>
> Could you please review the code again and submit a cleanup patch to
> fix those typos?
>
> Thanks,
> -Bryan
>
>
>> The 7 lines driven by the TCA6507 can either drive LEDs or act as output-only
>> GPIOs.
>>
>> To make this distinction in devicetree we use the "compatible" property.
>>
>> If the device attached to a line is "compatible" with "gpio", we treat it
>> like a GPIO.  If it is "compatible" with "led" (or if no "compatible" value
>> is set) we treat it like an LED.
>>
>> Signed-off-by: NeilBrown <neilb@suse.de>
>>
>> diff --git a/Documentation/devicetree/bindings/leds/tca6507.txt b/Documentation/devicetree/bindings/leds/tca6507.txt
>> index 80ff3dfb1f32..d7221b84987c 100644
>> --- a/Documentation/devicetree/bindings/leds/tca6507.txt
>> +++ b/Documentation/devicetree/bindings/leds/tca6507.txt
>> @@ -2,6 +2,13 @@ LEDs connected to tca6507
>>
>>  Required properties:
>>  - compatible : should be : "ti,tca6507".
>> +- #address-cells: must be 1
>> +- #size-cells: must be 0
>> +- reg: typically 0x45.
>> +
>> +Optional properties:
>> +- gpio-controller: allows lines to be used as output-only GPIOs.
>> +- #gpio-cells: if present, must be 0.
>>
>>  Each led is represented as a sub-node of the ti,tca6507 device.
>>
>> @@ -10,6 +17,7 @@ LED sub-node properties:
>>  - reg : number of LED line (could be from 0 to 6)
>>  - linux,default-trigger : (optional)
>>     see Documentation/devicetree/bindings/leds/common.txt
>> +- compatible: either "led" (the default) or "gpio".
>>
>>  Examples:
>>
>> @@ -19,6 +27,9 @@ tca6507@45 {
>>         #size-cells = <0>;
>>         reg = <0x45>;
>>
>> +       gpio-controller;
>> +       #gpio-cells = <2>;
>> +
>>         led0: red-aux@0 {
>>                 label = "red:aux";
>>                 reg = <0x0>;
>> @@ -29,5 +40,10 @@ tca6507@45 {
>>                 reg = <0x5>;
>>                 linux,default-trigger = "default-on";
>>         };
>> +
>> +       wifi-reset@6 {
>> +               reg = <0x6>;
>> +               compatible = "gpio";
>> +       };
>>  };
>>
>> diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
>> index f5063f447463..93a2b1759054 100644
>> --- a/drivers/leds/leds-tca6507.c
>> +++ b/drivers/leds/leds-tca6507.c
>> @@ -638,6 +638,9 @@ static int tca6507_probe_gpios(struct i2c_client *client,
>>         tca->gpio.direction_output = tca6507_gpio_direction_output;
>>         tca->gpio.set = tca6507_gpio_set_value;
>>         tca->gpio.dev = &client->dev;
>> +#ifdef CONFIG_OF_GPIO
>> +       tca->gpio.of_node = of_node_get(client->dev.of_node);
>> +#endif
>>         err = gpiochip_add(&tca->gpio);
>>         if (err) {
>>                 tca->gpio.ngpio = 0;
>> @@ -696,6 +699,8 @@ tca6507_led_dt_init(struct i2c_client *client)
>>                 led.default_trigger =
>>                         of_get_property(child, "linux,default-trigger", NULL);
>>                 led.flags = 0;
>> +               if (of_property_match_string(child, "compatible", "gpio") >= 0)
>> +                       led.flags |= TCA6507_MAKE_GPIO;
>>                 ret = of_property_read_u32(child, "reg", &reg);
>>                 if (ret != 0 || reg < 0 || reg >= NUM_LEDS)
>>                         continue;
>> @@ -709,6 +714,7 @@ tca6507_led_dt_init(struct i2c_client *client)
>>
>>         pdata->leds.leds = tca_leds;
>>         pdata->leds.num_leds = NUM_LEDS;
>> +       pdata->gpio_base = -1;
>>
>>         return pdata;
>>  }

PATCH] LEDS: tca6507 - fix up some comments.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3178 bytes --]


In particular fix the capitalisation of GPIO and LED and
correct TCA6507_MAKE_CPIO, but also rewrite the comment about
platform-data to include reference to devicetree.

Reported-by: Bryan Wu <cooloney@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
index 93a2b1759054..80c9d69e2bdd 100644
--- a/drivers/leds/leds-tca6507.c
+++ b/drivers/leds/leds-tca6507.c
@@ -8,7 +8,7 @@
  * double-blink.
  *
  * This driver can configure each line either as a 'GPIO' which is out-only
- * (no pull-up) or as an LED with variable brightness and hardware-assisted
+ * (pull-up resistor required) or as an LED with variable brightness and hardware-assisted
  * blinking.
  *
  * Apart from OFF and ON there are three programmable brightness levels which
@@ -60,21 +60,26 @@
  * and LEDs using the blink.  It can only be reprogrammed when the appropriate
  * counter is zero.  The MASTER level has a single usage count.
  *
- * Each Led has programmable 'on' and 'off' time as milliseconds.  With each
+ * Each LED has programmable 'on' and 'off' time as milliseconds.  With each
  * there is a flag saying if it was explicitly requested or defaulted.
  * Similarly the banks know if each time was explicit or a default.  Defaults
  * are permitted to be changed freely - they are not recognised when matching.
  *
  *
- * An led-tca6507 device must be provided with platform data.  This data
- * lists for each output: the name, default trigger, and whether the signal
- * is being used as a GPiO rather than an led.  'struct led_plaform_data'
- * is used for this.  If 'name' is NULL, the output isn't used.  If 'flags'
- * is TCA6507_MAKE_CPIO, the output is a GPO.
- * The "struct led_platform_data" can be embedded in a
- * "struct tca6507_platform_data" which adds a 'gpio_base' for the GPiOs,
- * and a 'setup' callback which is called once the GPiOs are available.
+ * An led-tca6507 device must be provided with platform data or configured
+ * via devicetree.
+ * The platform-data lists for each output: the name, default trigger,
+ * and whether the signal is being used as a GPIO rather than an LED.
+ * 'struct led_plaform_data' is used for this.  If 'name' is NULL, the
+ * output isn't used.  If 'flags' is TCA6507_MAKE_GPIO, the output is
+ * a GPO.  The "struct led_platform_data" can be embedded in a "struct
+ * tca6507_platform_data" which adds a 'gpio_base' for the GPIOs, and
+ * a 'setup' callback which is called once the GPIOs are available.
  *
+ * When configured via devicetree there is one child for each output.
+ * The "reg" determines the output number and "compatible" determines
+ * whether it is an LED or a GPIO.  "linux,default-trigger" can set a
+ * default trigger.
  */
 
 #include <linux/module.h>
@@ -309,7 +314,7 @@ static void set_level(struct tca6507_chip *tca, int bank, int level)
 	tca->bank[bank].level = level;
 }
 
-/* Record all relevant time code for a given bank */
+/* Record all relevant time codes for a given bank */
 static void set_times(struct tca6507_chip *tca, int bank)
 {
 	int c1, c2;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: PATCH] LEDS: tca6507 - fix up some comments.

[Joe Perches]

On Fri, 2013-11-08 at 14:20 +1100, NeilBrown wrote:
> In particular fix the capitalisation of GPIO and LED and
> correct TCA6507_MAKE_CPIO, but also rewrite the comment about
> platform-data to include reference to devicetree.

trivia:

> diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
[]
> @@ -8,7 +8,7 @@
>   * double-blink.
>   *
>   * This driver can configure each line either as a 'GPIO' which is out-only
> - * (no pull-up) or as an LED with variable brightness and hardware-assisted
> + * (pull-up resistor required) or as an LED with variable brightness and hardware-assisted
>   * blinking.

Please rewrap the comment to 80 cols.

Re: PATCH] LEDS: tca6507 - fix up some comments.

[Bryan Wu]

On Thu, Nov 7, 2013 at 7:24 PM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2013-11-08 at 14:20 +1100, NeilBrown wrote:
>> In particular fix the capitalisation of GPIO and LED and
>> correct TCA6507_MAKE_CPIO, but also rewrite the comment about
>> platform-data to include reference to devicetree.
>
> trivia:
>
>> diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
> []
>> @@ -8,7 +8,7 @@
>>   * double-blink.
>>   *
>>   * This driver can configure each line either as a 'GPIO' which is out-only
>> - * (no pull-up) or as an LED with variable brightness and hardware-assisted
>> + * (pull-up resistor required) or as an LED with variable brightness and hardware-assisted
>>   * blinking.
>
> Please rewrap the comment to 80 cols.
>
>

Hi Neil,

Could you update your patch according Joe's review.

Thanks,
-Bryan

[PATCH - v2] LEDS: tca6507 - fix up some comments.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 14495 bytes --]


In particular fix the capitalisation of GPIO and LED and
correct TCA6507_MAKE_CPIO, but also rewrite the comment about
platform-data to include reference to devicetree.

Also re-wrap comments to fit 80 columns.

Reported-by: Bryan Wu <cooloney@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
index 93a2b1759054..503df834c690 100644
--- a/drivers/leds/leds-tca6507.c
+++ b/drivers/leds/leds-tca6507.c
@@ -4,77 +4,87 @@
  * The TCA6507 is a programmable LED controller that can drive 7
  * separate lines either by holding them low, or by pulsing them
  * with modulated width.
- * The modulation can be varied in a simple pattern to produce a blink or
- * double-blink.
+ * The modulation can be varied in a simple pattern to produce a
+ * blink or double-blink.
  *
- * This driver can configure each line either as a 'GPIO' which is out-only
- * (no pull-up) or as an LED with variable brightness and hardware-assisted
- * blinking.
+ * This driver can configure each line either as a 'GPIO' which is
+ * out-only (pull-up resistor required) or as an LED with variable
+ * brightness and hardware-assisted blinking.
  *
- * Apart from OFF and ON there are three programmable brightness levels which
- * can be programmed from 0 to 15 and indicate how many 500usec intervals in
- * each 8msec that the led is 'on'.  The levels are named MASTER, BANK0 and
- * BANK1.
+ * Apart from OFF and ON there are three programmable brightness
+ * levels which can be programmed from 0 to 15 and indicate how many
+ * 500usec intervals in each 8msec that the led is 'on'.  The levels
+ * are named MASTER, BANK0 and BANK1.
  *
- * There are two different blink rates that can be programmed, each with
- * separate time for rise, on, fall, off and second-off.  Thus if 3 or more
- * different non-trivial rates are required, software must be used for the extra
- * rates. The two different blink rates must align with the two levels BANK0 and
- * BANK1.
- * This driver does not support double-blink so 'second-off' always matches
- * 'off'.
+ * There are two different blink rates that can be programmed, each
+ * with separate time for rise, on, fall, off and second-off.  Thus if
+ * 3 or more different non-trivial rates are required, software must
+ * be used for the extra rates. The two different blink rates must
+ * align with the two levels BANK0 and BANK1.  This driver does not
+ * support double-blink so 'second-off' always matches 'off'.
  *
- * Only 16 different times can be programmed in a roughly logarithmic scale from
- * 64ms to 16320ms.  To be precise the possible times are:
+ * Only 16 different times can be programmed in a roughly logarithmic
+ * scale from 64ms to 16320ms.  To be precise the possible times are:
  *    0, 64, 128, 192, 256, 384, 512, 768,
  *    1024, 1536, 2048, 3072, 4096, 5760, 8128, 16320
  *
- * Times that cannot be closely matched with these must be
- * handled in software.  This driver allows 12.5% error in matching.
+ * Times that cannot be closely matched with these must be handled in
+ * software.  This driver allows 12.5% error in matching.
  *
- * This driver does not allow rise/fall rates to be set explicitly.  When trying
- * to match a given 'on' or 'off' period, an appropriate pair of 'change' and
- * 'hold' times are chosen to get a close match.  If the target delay is even,
- * the 'change' number will be the smaller; if odd, the 'hold' number will be
- * the smaller.
-
- * Choosing pairs of delays with 12.5% errors allows us to match delays in the
- * ranges: 56-72, 112-144, 168-216, 224-27504, 28560-36720.
- * 26% of the achievable sums can be matched by multiple pairings. For example
- * 1536 == 1536+0, 1024+512, or 768+768.  This driver will always choose the
- * pairing with the least maximum - 768+768 in this case.  Other pairings are
- * not available.
+ * This driver does not allow rise/fall rates to be set explicitly.
+ * When trying to match a given 'on' or 'off' period, an appropriate
+ * pair of 'change' and 'hold' times are chosen to get a close match.
+ * If the target delay is even, the 'change' number will be the
+ * smaller; if odd, the 'hold' number will be the smaller.
+
+ * Choosing pairs of delays with 12.5% errors allows us to match
+ * delays in the ranges: 56-72, 112-144, 168-216, 224-27504,
+ * 28560-36720.
+ * 26% of the achievable sums can be matched by multiple pairings.
+ * For example 1536 == 1536+0, 1024+512, or 768+768.
+ * This driver will always choose the pairing with the least
+ * maximum - 768+768 in this case.  Other pairings are not available.
  *
- * Access to the 3 levels and 2 blinks are on a first-come, first-served basis.
- * Access can be shared by multiple leds if they have the same level and
- * either same blink rates, or some don't blink.
- * When a led changes, it relinquishes access and tries again, so it might
- * lose access to hardware blink.
- * If a blink engine cannot be allocated, software blink is used.
- * If the desired brightness cannot be allocated, the closest available non-zero
- * brightness is used.  As 'full' is always available, the worst case would be
- * to have two different blink rates at '1', with Max at '2', then other leds
- * will have to choose between '2' and '16'.  Hopefully this is not likely.
+ * Access to the 3 levels and 2 blinks are on a first-come,
+ * first-served basis.  Access can be shared by multiple leds if they
+ * have the same level and either same blink rates, or some don't
+ * blink.  When a led changes, it relinquishes access and tries again,
+ * so it might lose access to hardware blink.
  *
- * Each bank (BANK0 and BANK1) has two usage counts - LEDs using the brightness
- * and LEDs using the blink.  It can only be reprogrammed when the appropriate
- * counter is zero.  The MASTER level has a single usage count.
+ * If a blink engine cannot be allocated, software blink is used.  If
+ * the desired brightness cannot be allocated, the closest available
+ * non-zero brightness is used.  As 'full' is always available, the
+ * worst case would be to have two different blink rates at '1', with
+ * Max at '2', then other leds will have to choose between '2' and
+ * '16'.  Hopefully this is not likely.
  *
- * Each Led has programmable 'on' and 'off' time as milliseconds.  With each
- * there is a flag saying if it was explicitly requested or defaulted.
- * Similarly the banks know if each time was explicit or a default.  Defaults
- * are permitted to be changed freely - they are not recognised when matching.
+ * Each bank (BANK0 and BANK1) has two usage counts - LEDs using the
+ * brightness and LEDs using the blink.  It can only be reprogrammed
+ * when the appropriate counter is zero.  The MASTER level has a
+ * single usage count.
  *
+ * Each LED has programmable 'on' and 'off' time as milliseconds.
+ * With each there is a flag saying if it was explicitly requested or
+ * defaulted.  Similarly the banks know if each time was explicit or a
+ * default.  Defaults are permitted to be changed freely - they are
+ * not recognised when matching.
  *
- * An led-tca6507 device must be provided with platform data.  This data
- * lists for each output: the name, default trigger, and whether the signal
- * is being used as a GPiO rather than an led.  'struct led_plaform_data'
- * is used for this.  If 'name' is NULL, the output isn't used.  If 'flags'
- * is TCA6507_MAKE_CPIO, the output is a GPO.
- * The "struct led_platform_data" can be embedded in a
- * "struct tca6507_platform_data" which adds a 'gpio_base' for the GPiOs,
- * and a 'setup' callback which is called once the GPiOs are available.
  *
+ * An led-tca6507 device must be provided with platform data or
+ * configured via devicetree.
+ *
+ * The platform-data lists for each output: the name, default trigger,
+ * and whether the signal is being used as a GPIO rather than an LED.
+ * 'struct led_plaform_data' is used for this.  If 'name' is NULL, the
+ * output isn't used.  If 'flags' is TCA6507_MAKE_GPIO, the output is
+ * a GPO.  The "struct led_platform_data" can be embedded in a "struct
+ * tca6507_platform_data" which adds a 'gpio_base' for the GPIOs, and
+ * a 'setup' callback which is called once the GPIOs are available.
+ *
+ * When configured via devicetree there is one child for each output.
+ * The "reg" determines the output number and "compatible" determines
+ * whether it is an LED or a GPIO.  "linux,default-trigger" can set a
+ * default trigger.
  */
 
 #include <linux/module.h>
@@ -192,17 +202,18 @@ MODULE_DEVICE_TABLE(i2c, tca6507_id);
 static int choose_times(int msec, int *c1p, int *c2p)
 {
 	/*
-	 * Choose two timecodes which add to 'msec' as near as possible.
-	 * The first returned is the 'on' or 'off' time.  The second is to be
-	 * used as a 'fade-on' or 'fade-off' time.  If 'msec' is even,
-	 * the first will not be smaller than the second.  If 'msec' is odd,
-	 * the first will not be larger than the second.
-	 * If we cannot get a sum within 1/8 of 'msec' fail with -EINVAL,
-	 * otherwise return the sum that was achieved, plus 1 if the first is
-	 * smaller.
-	 * If two possibilities are equally good (e.g. 512+0, 256+256), choose
-	 * the first pair so there is more change-time visible (i.e. it is
-	 * softer).
+	 * Choose two timecodes which add to 'msec' as near as
+	 * possible.  The first returned is the 'on' or 'off' time.
+	 * The second is to be used as a 'fade-on' or 'fade-off' time.
+	 * If 'msec' is even, the first will not be smaller than the
+	 * second.  If 'msec' is odd, the first will not be larger
+	 * than the second.
+	 * If we cannot get a sum within 1/8 of 'msec' fail with
+	 * -EINVAL, otherwise return the sum that was achieved, plus 1
+	 * if the first is smaller.
+	 * If two possibilities are equally good (e.g. 512+0,
+	 * 256+256), choose the first pair so there is more
+	 * change-time visible (i.e. it is softer).
 	 */
 	int c1, c2;
 	int tmax = msec * 9 / 8;
@@ -255,8 +266,8 @@ static int choose_times(int msec, int *c1p, int *c2p)
 }
 
 /*
- * Update the register file with the appropriate 3-bit state for
- * the given led.
+ * Update the register file with the appropriate 3-bit state for the
+ * given led.
  */
 static void set_select(struct tca6507_chip *tca, int led, int val)
 {
@@ -274,9 +285,9 @@ static void set_select(struct tca6507_chip *tca, int led, int val)
 	}
 }
 
-/* Update the register file with the appropriate 4-bit code for
- * one bank or other.  This can be used for timers, for levels, or
- * for initialisation.
+/* Update the register file with the appropriate 4-bit code for one
+ * bank or other.  This can be used for timers, for levels, or for
+ * initialization.
  */
 static void set_code(struct tca6507_chip *tca, int reg, int bank, int new)
 {
@@ -309,7 +320,7 @@ static void set_level(struct tca6507_chip *tca, int bank, int level)
 	tca->bank[bank].level = level;
 }
 
-/* Record all relevant time code for a given bank */
+/* Record all relevant time codes for a given bank */
 static void set_times(struct tca6507_chip *tca, int bank)
 {
 	int c1, c2;
@@ -317,7 +328,8 @@ static void set_times(struct tca6507_chip *tca, int bank)
 
 	result = choose_times(tca->bank[bank].ontime, &c1, &c2);
 	dev_dbg(&tca->client->dev,
-		"Chose on  times %d(%d) %d(%d) for %dms\n", c1, time_codes[c1],
+		"Chose on  times %d(%d) %d(%d) for %dms\n",
+		c1, time_codes[c1],
 		c2, time_codes[c2], tca->bank[bank].ontime);
 	set_code(tca, TCA6507_FADE_ON, bank, c2);
 	set_code(tca, TCA6507_FULL_ON, bank, c1);
@@ -325,7 +337,8 @@ static void set_times(struct tca6507_chip *tca, int bank)
 
 	result = choose_times(tca->bank[bank].offtime, &c1, &c2);
 	dev_dbg(&tca->client->dev,
-		"Chose off times %d(%d) %d(%d) for %dms\n", c1, time_codes[c1],
+		"Chose off times %d(%d) %d(%d) for %dms\n",
+		c1, time_codes[c1],
 		c2, time_codes[c2], tca->bank[bank].offtime);
 	set_code(tca, TCA6507_FADE_OFF, bank, c2);
 	set_code(tca, TCA6507_FIRST_OFF, bank, c1);
@@ -373,7 +386,8 @@ static void led_release(struct tca6507_led *led)
 
 static int led_prepare(struct tca6507_led *led)
 {
-	/* Assign this led to a bank, configuring that bank if necessary. */
+	/* Assign this led to a bank, configuring that bank if
+	 * necessary. */
 	int level = TO_LEVEL(led->led_cdev.brightness);
 	struct tca6507_chip *tca = led->chip;
 	int c1, c2;
@@ -389,10 +403,10 @@ static int led_prepare(struct tca6507_led *led)
 
 	if (led->ontime == 0 || led->offtime == 0) {
 		/*
-		 * Just set the brightness, choosing first usable bank.
-		 * If none perfect, choose best.
-		 * Count backwards so we check MASTER bank first
-		 * to avoid wasting a timer.
+		 * Just set the brightness, choosing first usable
+		 * bank.  If none perfect, choose best.  Count
+		 * backwards so we check MASTER bank first to avoid
+		 * wasting a timer.
 		 */
 		int best = -1;/* full-on */
 		int diff = 15-level;
@@ -433,9 +447,9 @@ static int led_prepare(struct tca6507_led *led)
 	}
 
 	/*
-	 * We have on/off time so we need to try to allocate a timing bank.
-	 * First check if times are compatible with hardware and give up if
-	 * not.
+	 * We have on/off time so we need to try to allocate a timing
+	 * bank.  First check if times are compatible with hardware
+	 * and give up if not.
 	 */
 	if (choose_times(led->ontime, &c1, &c2) < 0)
 		return -EINVAL;
@@ -523,8 +537,8 @@ static int led_assign(struct tca6507_led *led)
 	err = led_prepare(led);
 	if (err) {
 		/*
-		 * Can only fail on timer setup.  In that case we need to
-		 * re-establish as steady level.
+		 * Can only fail on timer setup.  In that case we need
+		 * to re-establish as steady level.
 		 */
 		led->ontime = 0;
 		led->offtime = 0;
@@ -594,8 +608,8 @@ static void tca6507_gpio_set_value(struct gpio_chip *gc,
 
 	spin_lock_irqsave(&tca->lock, flags);
 	/*
-	 * 'OFF' is floating high, and 'ON' is pulled down, so it has the
-	 * inverse sense of 'val'.
+	 * 'OFF' is floating high, and 'ON' is pulled down, so it has
+	 * the inverse sense of 'val'.
 	 */
 	set_select(tca, tca->gpio_map[offset],
 		   val ? TCA6507_LS_LED_OFF : TCA6507_LS_LED_ON);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/2] LEDS: tca6502: add device-tree support for GPIO configuration.

[Mark Rutland]

On Fri, Nov 01, 2013 at 02:41:20AM +0000, NeilBrown wrote:
> 
> The 7 lines driven by the TCA6507 can either drive LEDs or act as output-only
> GPIOs.
> 
> To make this distinction in devicetree we use the "compatible" property.
> 
> If the device attached to a line is "compatible" with "gpio", we treat it
> like a GPIO.  If it is "compatible" with "led" (or if no "compatible" value
> is set) we treat it like an LED.

I'm nto sure I understand the purpose of this.

Are these lines all GPIOs, some of which happen to by attached to LEDs?

If so, why is handlnig the device as a gpio-controller and using the
gpio-leds binding not sufficient?

THanksm
Mark.

> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/Documentation/devicetree/bindings/leds/tca6507.txt b/Documentation/devicetree/bindings/leds/tca6507.txt
> index 80ff3dfb1f32..d7221b84987c 100644
> --- a/Documentation/devicetree/bindings/leds/tca6507.txt
> +++ b/Documentation/devicetree/bindings/leds/tca6507.txt
> @@ -2,6 +2,13 @@ LEDs connected to tca6507
>  
>  Required properties:
>  - compatible : should be : "ti,tca6507".
> +- #address-cells: must be 1
> +- #size-cells: must be 0
> +- reg: typically 0x45.
> +
> +Optional properties:
> +- gpio-controller: allows lines to be used as output-only GPIOs.
> +- #gpio-cells: if present, must be 0.
>  
>  Each led is represented as a sub-node of the ti,tca6507 device.
>  
> @@ -10,6 +17,7 @@ LED sub-node properties:
>  - reg : number of LED line (could be from 0 to 6)
>  - linux,default-trigger : (optional)
>     see Documentation/devicetree/bindings/leds/common.txt
> +- compatible: either "led" (the default) or "gpio".
>  
>  Examples:
>  
> @@ -19,6 +27,9 @@ tca6507@45 {
>  	#size-cells = <0>;
>  	reg = <0x45>;
>  
> +	gpio-controller;
> +	#gpio-cells = <2>;
> +
>  	led0: red-aux@0 {
>  		label = "red:aux";
>  		reg = <0x0>;
> @@ -29,5 +40,10 @@ tca6507@45 {
>  		reg = <0x5>;
>  		linux,default-trigger = "default-on";
>  	};
> +
> +	wifi-reset@6 {
> +		reg = <0x6>;
> +		compatible = "gpio";
> +	};
>  };
>  
> diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
> index f5063f447463..93a2b1759054 100644
> --- a/drivers/leds/leds-tca6507.c
> +++ b/drivers/leds/leds-tca6507.c
> @@ -638,6 +638,9 @@ static int tca6507_probe_gpios(struct i2c_client *client,
>  	tca->gpio.direction_output = tca6507_gpio_direction_output;
>  	tca->gpio.set = tca6507_gpio_set_value;
>  	tca->gpio.dev = &client->dev;
> +#ifdef CONFIG_OF_GPIO
> +	tca->gpio.of_node = of_node_get(client->dev.of_node);
> +#endif
>  	err = gpiochip_add(&tca->gpio);
>  	if (err) {
>  		tca->gpio.ngpio = 0;
> @@ -696,6 +699,8 @@ tca6507_led_dt_init(struct i2c_client *client)
>  		led.default_trigger =
>  			of_get_property(child, "linux,default-trigger", NULL);
>  		led.flags = 0;
> +		if (of_property_match_string(child, "compatible", "gpio") >= 0)
> +			led.flags |= TCA6507_MAKE_GPIO;
>  		ret = of_property_read_u32(child, "reg", &reg);
>  		if (ret != 0 || reg < 0 || reg >= NUM_LEDS)
>  			continue;
> @@ -709,6 +714,7 @@ tca6507_led_dt_init(struct i2c_client *client)
>  
>  	pdata->leds.leds = tca_leds;
>  	pdata->leds.num_leds = NUM_LEDS;
> +	pdata->gpio_base = -1;
>  
>  	return pdata;
>  }

Re: [PATCH 2/2] LEDS: tca6502: add device-tree support for GPIO configuration.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4652 bytes --]

On Mon, 18 Nov 2013 11:55:12 +0000 Mark Rutland <mark.rutland@arm.com> wrote:

> On Fri, Nov 01, 2013 at 02:41:20AM +0000, NeilBrown wrote:
> > 
> > The 7 lines driven by the TCA6507 can either drive LEDs or act as output-only
> > GPIOs.
> > 
> > To make this distinction in devicetree we use the "compatible" property.
> > 
> > If the device attached to a line is "compatible" with "gpio", we treat it
> > like a GPIO.  If it is "compatible" with "led" (or if no "compatible" value
> > is set) we treat it like an LED.
> 
> I'm nto sure I understand the purpose of this.
> 
> Are these lines all GPIOs, some of which happen to by attached to LEDs?
> 
> If so, why is handlnig the device as a gpio-controller and using the
> gpio-leds binding not sufficient?

No, all the lines are designed to drives LEDs but can happen to be wired up
like GPIOs.  This configuration is explicitly described in the data sheets
for the chip, and is actually used in my board.

The 7 pins either float or sink current. They can switch between the two
modes in various ways under hardware control with a PWM which can have a
varying duty cycle.
So they are very suitable for driving LEDs as the PWM can adjust the apparent
brightness and the varying duty cycle can effect a 'blink' pattern.

But the pins can also be fixed in one state of the other - either never on or
always on.  Floating or constantly sinking current.

This mode combined with a pull-up resistor makes an effective output-only
GPIO.  The same action that would turn the LED on, will pull the GPIO low.
The action that would turn the LED off, will allow the GPIO to go high.

So the device connected to each pin can be treated as an LED or as a GPIO.

I hope that clarifies the situation.

Thanks,
NeilBrown


> 
> THanksm
> Mark.
> 
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/tca6507.txt b/Documentation/devicetree/bindings/leds/tca6507.txt
> > index 80ff3dfb1f32..d7221b84987c 100644
> > --- a/Documentation/devicetree/bindings/leds/tca6507.txt
> > +++ b/Documentation/devicetree/bindings/leds/tca6507.txt
> > @@ -2,6 +2,13 @@ LEDs connected to tca6507
> >  
> >  Required properties:
> >  - compatible : should be : "ti,tca6507".
> > +- #address-cells: must be 1
> > +- #size-cells: must be 0
> > +- reg: typically 0x45.
> > +
> > +Optional properties:
> > +- gpio-controller: allows lines to be used as output-only GPIOs.
> > +- #gpio-cells: if present, must be 0.
> >  
> >  Each led is represented as a sub-node of the ti,tca6507 device.
> >  
> > @@ -10,6 +17,7 @@ LED sub-node properties:
> >  - reg : number of LED line (could be from 0 to 6)
> >  - linux,default-trigger : (optional)
> >     see Documentation/devicetree/bindings/leds/common.txt
> > +- compatible: either "led" (the default) or "gpio".
> >  
> >  Examples:
> >  
> > @@ -19,6 +27,9 @@ tca6507@45 {
> >  	#size-cells = <0>;
> >  	reg = <0x45>;
> >  
> > +	gpio-controller;
> > +	#gpio-cells = <2>;
> > +
> >  	led0: red-aux@0 {
> >  		label = "red:aux";
> >  		reg = <0x0>;
> > @@ -29,5 +40,10 @@ tca6507@45 {
> >  		reg = <0x5>;
> >  		linux,default-trigger = "default-on";
> >  	};
> > +
> > +	wifi-reset@6 {
> > +		reg = <0x6>;
> > +		compatible = "gpio";
> > +	};
> >  };
> >  
> > diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
> > index f5063f447463..93a2b1759054 100644
> > --- a/drivers/leds/leds-tca6507.c
> > +++ b/drivers/leds/leds-tca6507.c
> > @@ -638,6 +638,9 @@ static int tca6507_probe_gpios(struct i2c_client *client,
> >  	tca->gpio.direction_output = tca6507_gpio_direction_output;
> >  	tca->gpio.set = tca6507_gpio_set_value;
> >  	tca->gpio.dev = &client->dev;
> > +#ifdef CONFIG_OF_GPIO
> > +	tca->gpio.of_node = of_node_get(client->dev.of_node);
> > +#endif
> >  	err = gpiochip_add(&tca->gpio);
> >  	if (err) {
> >  		tca->gpio.ngpio = 0;
> > @@ -696,6 +699,8 @@ tca6507_led_dt_init(struct i2c_client *client)
> >  		led.default_trigger =
> >  			of_get_property(child, "linux,default-trigger", NULL);
> >  		led.flags = 0;
> > +		if (of_property_match_string(child, "compatible", "gpio") >= 0)
> > +			led.flags |= TCA6507_MAKE_GPIO;
> >  		ret = of_property_read_u32(child, "reg", &reg);
> >  		if (ret != 0 || reg < 0 || reg >= NUM_LEDS)
> >  			continue;
> > @@ -709,6 +714,7 @@ tca6507_led_dt_init(struct i2c_client *client)
> >  
> >  	pdata->leds.leds = tca_leds;
> >  	pdata->leds.num_leds = NUM_LEDS;
> > +	pdata->gpio_base = -1;
> >  
> >  	return pdata;
> >  }
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]