All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] Extend pca9532 device tree support
@ 2017-03-30 13:33 Felix Brack
  2017-04-02 14:41 ` Jacek Anaszewski
  0 siblings, 1 reply; 14+ messages in thread
From: Felix Brack @ 2017-03-30 13:33 UTC (permalink / raw)
  To: jacek.anaszewski, rpurdie, pavel, mark.rutland, riku.voipio,
	linux-leds, devicetree
  Cc: linux-kernel, fb

This patch extends the device tree support for the pca9532 by adding
the leds 'default-state' property.

Changes in v2:
- remove prescaler and pwm configuration by none generic
  DT properties

Signed-off-by: Felix Brack <fb@ltec.ch>
---
 .../devicetree/bindings/leds/leds-pca9532.txt      | 10 +++++++
 drivers/leds/leds-pca9532.c                        | 31 +++++++++++++++++++++-
 include/linux/leds-pca9532.h                       |  4 +--
 3 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
index 198f3ba..8374075 100644
--- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
+++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
@@ -17,6 +17,8 @@ Optional sub-node properties:
 	- label: see Documentation/devicetree/bindings/leds/common.txt
 	- type: Output configuration, see dt-bindings/leds/leds-pca9532.h (default NONE)
 	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
+	- default-state: see Documentation/devicetree/bindings/leds/common.txt
+	  This property is only valid for sub-nodes of type <PCA9532_TYPE_LED>.
 
 Example:
   #include <dt-bindings/leds/leds-pca9532.h>
@@ -33,6 +35,14 @@ Example:
       label = "pca:green:power";
       type = <PCA9532_TYPE_LED>;
     };
+    kernel-booting {
+    	type = <PCA9532_TYPE_LED>;
+    	default-state = "on";
+    };
+    sys-stat {
+    	type = <PCA9532_TYPE_LED>;
+    	default-state = "keep"; // don't touch, was set by U-Boot
+    };
   };
 
 For more product information please see the link below:
diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index 06e6310..66ef280 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -254,6 +254,21 @@ static void pca9532_input_work(struct work_struct *work)
 	mutex_unlock(&data->update_lock);
 }
 
+static enum pca9532_state pca9532_getled(struct pca9532_led *led)
+{
+	struct i2c_client *client = led->client;
+	struct pca9532_data *data = i2c_get_clientdata(client);
+	u8 maxleds = data->chip_info->num_leds;
+	char reg;
+	enum pca9532_state ret;
+
+	mutex_lock(&data->update_lock);
+	reg = i2c_smbus_read_byte_data(client, LED_REG(maxleds, led->id));
+	ret = reg >> LED_NUM(led->id)/2;
+	mutex_unlock(&data->update_lock);
+	return ret;
+}
+
 #ifdef CONFIG_LEDS_PCA9532_GPIO
 static int pca9532_gpio_request_pin(struct gpio_chip *gc, unsigned offset)
 {
@@ -366,7 +381,10 @@ static int pca9532_configure(struct i2c_client *client,
 			gpios++;
 			break;
 		case PCA9532_TYPE_LED:
-			led->state = pled->state;
+			if (pled->state == PCA9532_KEEP)
+				led->state = pca9532_getled(led);
+			else
+				led->state = pled->state;
 			led->name = pled->name;
 			led->ldev.name = led->name;
 			led->ldev.default_trigger = pled->default_trigger;
@@ -456,6 +474,7 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
 	const struct of_device_id *match;
 	int devid, maxleds;
 	int i = 0;
+	const char *state;
 
 	match = of_match_device(of_pca9532_leds_match, dev);
 	if (!match)
@@ -475,6 +494,16 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
 		of_property_read_u32(child, "type", &pdata->leds[i].type);
 		of_property_read_string(child, "linux,default-trigger",
 					&pdata->leds[i].default_trigger);
+		if (!of_property_read_string(child, "default-state", &state)) {
+			if (!strcmp(state, "on"))
+				pdata->leds[i].state = PCA9532_ON;
+			else if (!strcmp(state, "keep"))
+				pdata->leds[i].state = PCA9532_KEEP;
+			else if (!strcmp(state, "pwm0"))
+				pdata->leds[i].state = PCA9532_PWM0;
+			else if (!strcmp(state, "pwm1"))
+				pdata->leds[i].state = PCA9532_PWM1;
+		}
 		if (++i >= maxleds) {
 			of_node_put(child);
 			break;
diff --git a/include/linux/leds-pca9532.h b/include/linux/leds-pca9532.h
index d215b45..a327b1aa 100644
--- a/include/linux/leds-pca9532.h
+++ b/include/linux/leds-pca9532.h
@@ -22,7 +22,8 @@ enum pca9532_state {
 	PCA9532_OFF  = 0x0,
 	PCA9532_ON   = 0x1,
 	PCA9532_PWM0 = 0x2,
-	PCA9532_PWM1 = 0x3
+	PCA9532_PWM1 = 0x3,
+	PCA9532_KEEP = 0xff
 };
 
 struct pca9532_led {
@@ -44,4 +45,3 @@ struct pca9532_platform_data {
 };
 
 #endif /* __LINUX_PCA9532_H */
-
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Extend pca9532 device tree support
  2017-03-30 13:33 [PATCH v2] Extend pca9532 device tree support Felix Brack
@ 2017-04-02 14:41 ` Jacek Anaszewski
       [not found]   ` <b54c77d5-b4f7-9517-8c5b-9200c766204a-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Jacek Anaszewski @ 2017-04-02 14:41 UTC (permalink / raw)
  To: Felix Brack, rpurdie, pavel, mark.rutland, riku.voipio,
	linux-leds, devicetree
  Cc: linux-kernel

Hi Felix,

Thanks for the patch.

I made a few cosmetic amendments to it, please refer below.

On 03/30/2017 03:33 PM, Felix Brack wrote:
> This patch extends the device tree support for the pca9532 by adding
> the leds 'default-state' property.
> 

Added leds: pca9532: prefix to the patch title.

Removed below change history from the commit message.

> Changes in v2:
> - remove prescaler and pwm configuration by none generic
>   DT properties
> 
> Signed-off-by: Felix Brack <fb@ltec.ch>
> ---
>  .../devicetree/bindings/leds/leds-pca9532.txt      | 10 +++++++
>  drivers/leds/leds-pca9532.c                        | 31 +++++++++++++++++++++-
>  include/linux/leds-pca9532.h                       |  4 +--
>  3 files changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
> index 198f3ba..8374075 100644
> --- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
> @@ -17,6 +17,8 @@ Optional sub-node properties:
>  	- label: see Documentation/devicetree/bindings/leds/common.txt
>  	- type: Output configuration, see dt-bindings/leds/leds-pca9532.h (default NONE)
>  	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
> +	- default-state: see Documentation/devicetree/bindings/leds/common.txt
> +	  This property is only valid for sub-nodes of type <PCA9532_TYPE_LED>.
>  
>  Example:
>    #include <dt-bindings/leds/leds-pca9532.h>
> @@ -33,6 +35,14 @@ Example:
>        label = "pca:green:power";
>        type = <PCA9532_TYPE_LED>;
>      };
> +    kernel-booting {
> +    	type = <PCA9532_TYPE_LED>;
> +    	default-state = "on";
> +    };
> +    sys-stat {
> +    	type = <PCA9532_TYPE_LED>;
> +    	default-state = "keep"; // don't touch, was set by U-Boot
> +    };

Adjusted above indentation to match the preceding lines.

>    };
>  
>  For more product information please see the link below:
> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> index 06e6310..66ef280 100644
> --- a/drivers/leds/leds-pca9532.c
> +++ b/drivers/leds/leds-pca9532.c
> @@ -254,6 +254,21 @@ static void pca9532_input_work(struct work_struct *work)
>  	mutex_unlock(&data->update_lock);
>  }
>  
> +static enum pca9532_state pca9532_getled(struct pca9532_led *led)
> +{
> +	struct i2c_client *client = led->client;
> +	struct pca9532_data *data = i2c_get_clientdata(client);
> +	u8 maxleds = data->chip_info->num_leds;
> +	char reg;
> +	enum pca9532_state ret;
> +
> +	mutex_lock(&data->update_lock);
> +	reg = i2c_smbus_read_byte_data(client, LED_REG(maxleds, led->id));
> +	ret = reg >> LED_NUM(led->id)/2;
> +	mutex_unlock(&data->update_lock);
> +	return ret;
> +}
> +
>  #ifdef CONFIG_LEDS_PCA9532_GPIO
>  static int pca9532_gpio_request_pin(struct gpio_chip *gc, unsigned offset)
>  {
> @@ -366,7 +381,10 @@ static int pca9532_configure(struct i2c_client *client,
>  			gpios++;
>  			break;
>  		case PCA9532_TYPE_LED:
> -			led->state = pled->state;
> +			if (pled->state == PCA9532_KEEP)
> +				led->state = pca9532_getled(led);
> +			else
> +				led->state = pled->state;
>  			led->name = pled->name;
>  			led->ldev.name = led->name;
>  			led->ldev.default_trigger = pled->default_trigger;
> @@ -456,6 +474,7 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
>  	const struct of_device_id *match;
>  	int devid, maxleds;
>  	int i = 0;
> +	const char *state;
>  
>  	match = of_match_device(of_pca9532_leds_match, dev);
>  	if (!match)
> @@ -475,6 +494,16 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
>  		of_property_read_u32(child, "type", &pdata->leds[i].type);
>  		of_property_read_string(child, "linux,default-trigger",
>  					&pdata->leds[i].default_trigger);
> +		if (!of_property_read_string(child, "default-state", &state)) {
> +			if (!strcmp(state, "on"))
> +				pdata->leds[i].state = PCA9532_ON;
> +			else if (!strcmp(state, "keep"))
> +				pdata->leds[i].state = PCA9532_KEEP;
> +			else if (!strcmp(state, "pwm0"))
> +				pdata->leds[i].state = PCA9532_PWM0;
> +			else if (!strcmp(state, "pwm1"))
> +				pdata->leds[i].state = PCA9532_PWM1;
> +		}
>  		if (++i >= maxleds) {
>  			of_node_put(child);
>  			break;
> diff --git a/include/linux/leds-pca9532.h b/include/linux/leds-pca9532.h
> index d215b45..a327b1aa 100644
> --- a/include/linux/leds-pca9532.h
> +++ b/include/linux/leds-pca9532.h
> @@ -22,7 +22,8 @@ enum pca9532_state {
>  	PCA9532_OFF  = 0x0,
>  	PCA9532_ON   = 0x1,
>  	PCA9532_PWM0 = 0x2,
> -	PCA9532_PWM1 = 0x3
> +	PCA9532_PWM1 = 0x3,
> +	PCA9532_KEEP = 0xff

Added a comma at the end of the above line to avoid the need for
changing two lines on addition of a new enum value, like in this case.

>  };
>  
>  struct pca9532_led {
> @@ -44,4 +45,3 @@ struct pca9532_platform_data {
>  };
>  
>  #endif /* __LINUX_PCA9532_H */
> -
> 

The patch, with the aforementioned amendments, has been applied to
the for-next branch of linux-leds.git.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Extend pca9532 device tree support
  2017-04-02 14:41 ` Jacek Anaszewski
@ 2017-04-06 15:50       ` Pavel Machek
  0 siblings, 0 replies; 14+ messages in thread
From: Pavel Machek @ 2017-04-06 15:50 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Felix Brack, rpurdie-Fm38FmjxZ/leoWH0uzbU5w,
	mark.rutland-5wv7dgnIgG8, riku.voipio-X3B1VOXEql0,
	linux-leds-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

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

Hi!

> > diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
> > index 198f3ba..8374075 100644
> > --- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
> > +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
> > @@ -17,6 +17,8 @@ Optional sub-node properties:
> >  	- label: see Documentation/devicetree/bindings/leds/common.txt
> >  	- type: Output configuration, see dt-bindings/leds/leds-pca9532.h (default NONE)
> >  	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
> > +	- default-state: see Documentation/devicetree/bindings/leds/common.txt
> > +	  This property is only valid for sub-nodes of type <PCA9532_TYPE_LED>.
> >  
> >  Example:
> >    #include <dt-bindings/leds/leds-pca9532.h>
> > @@ -33,6 +35,14 @@ Example:
> >        label = "pca:green:power";
> >        type = <PCA9532_TYPE_LED>;
> >      };
> > +    kernel-booting {
> > +    	type = <PCA9532_TYPE_LED>;
> > +    	default-state = "on";
> > +    };
> > +    sys-stat {
> > +    	type = <PCA9532_TYPE_LED>;
> > +    	default-state = "keep"; // don't touch, was set by U-Boot
> > +    };
> 
> Adjusted above indentation to match the preceding lines.

> > @@ -475,6 +494,16 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
> >  		of_property_read_u32(child, "type", &pdata->leds[i].type);
> >  		of_property_read_string(child, "linux,default-trigger",
> >  					&pdata->leds[i].default_trigger);
> > +		if (!of_property_read_string(child, "default-state", &state)) {
> > +			if (!strcmp(state, "on"))
> > +				pdata->leds[i].state = PCA9532_ON;
> > +			else if (!strcmp(state, "keep"))
> > +				pdata->leds[i].state = PCA9532_KEEP;
> > +			else if (!strcmp(state, "pwm0"))
> > +				pdata->leds[i].state = PCA9532_PWM0;
> > +			else if (!strcmp(state, "pwm1"))
> > +				pdata->leds[i].state = PCA9532_PWM1;
> > +		}
> >  		if (++i >= maxleds) {
> >  			of_node_put(child);
> >  			break;

This seems to look for "pwm0" and "pwm1" strings, which do not seem to
be documented.

Plus... is it useful to have default-state? We already have default
trigger. If we keep the value by default (on PC, we do something like
that) this patch should not be neccessary?
									Pavel
									
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Extend pca9532 device tree support
@ 2017-04-06 15:50       ` Pavel Machek
  0 siblings, 0 replies; 14+ messages in thread
From: Pavel Machek @ 2017-04-06 15:50 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Felix Brack, rpurdie, mark.rutland, riku.voipio, linux-leds,
	devicetree, linux-kernel

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

Hi!

> > diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
> > index 198f3ba..8374075 100644
> > --- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
> > +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
> > @@ -17,6 +17,8 @@ Optional sub-node properties:
> >  	- label: see Documentation/devicetree/bindings/leds/common.txt
> >  	- type: Output configuration, see dt-bindings/leds/leds-pca9532.h (default NONE)
> >  	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
> > +	- default-state: see Documentation/devicetree/bindings/leds/common.txt
> > +	  This property is only valid for sub-nodes of type <PCA9532_TYPE_LED>.
> >  
> >  Example:
> >    #include <dt-bindings/leds/leds-pca9532.h>
> > @@ -33,6 +35,14 @@ Example:
> >        label = "pca:green:power";
> >        type = <PCA9532_TYPE_LED>;
> >      };
> > +    kernel-booting {
> > +    	type = <PCA9532_TYPE_LED>;
> > +    	default-state = "on";
> > +    };
> > +    sys-stat {
> > +    	type = <PCA9532_TYPE_LED>;
> > +    	default-state = "keep"; // don't touch, was set by U-Boot
> > +    };
> 
> Adjusted above indentation to match the preceding lines.

> > @@ -475,6 +494,16 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
> >  		of_property_read_u32(child, "type", &pdata->leds[i].type);
> >  		of_property_read_string(child, "linux,default-trigger",
> >  					&pdata->leds[i].default_trigger);
> > +		if (!of_property_read_string(child, "default-state", &state)) {
> > +			if (!strcmp(state, "on"))
> > +				pdata->leds[i].state = PCA9532_ON;
> > +			else if (!strcmp(state, "keep"))
> > +				pdata->leds[i].state = PCA9532_KEEP;
> > +			else if (!strcmp(state, "pwm0"))
> > +				pdata->leds[i].state = PCA9532_PWM0;
> > +			else if (!strcmp(state, "pwm1"))
> > +				pdata->leds[i].state = PCA9532_PWM1;
> > +		}
> >  		if (++i >= maxleds) {
> >  			of_node_put(child);
> >  			break;

This seems to look for "pwm0" and "pwm1" strings, which do not seem to
be documented.

Plus... is it useful to have default-state? We already have default
trigger. If we keep the value by default (on PC, we do something like
that) this patch should not be neccessary?
									Pavel
									
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Extend pca9532 device tree support
  2017-04-06 15:50       ` Pavel Machek
@ 2017-04-06 19:00         ` Jacek Anaszewski
  -1 siblings, 0 replies; 14+ messages in thread
From: Jacek Anaszewski @ 2017-04-06 19:00 UTC (permalink / raw)
  To: Pavel Machek, Felix Brack
  Cc: rpurdie-Fm38FmjxZ/leoWH0uzbU5w, mark.rutland-5wv7dgnIgG8,
	riku.voipio-X3B1VOXEql0, linux-leds-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

Hi Pavel,

On 04/06/2017 05:50 PM, Pavel Machek wrote:
> Hi!
> 
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>> index 198f3ba..8374075 100644
>>> --- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>> +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>> @@ -17,6 +17,8 @@ Optional sub-node properties:
>>>  	- label: see Documentation/devicetree/bindings/leds/common.txt
>>>  	- type: Output configuration, see dt-bindings/leds/leds-pca9532.h (default NONE)
>>>  	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
>>> +	- default-state: see Documentation/devicetree/bindings/leds/common.txt
>>> +	  This property is only valid for sub-nodes of type <PCA9532_TYPE_LED>.
>>>  
>>>  Example:
>>>    #include <dt-bindings/leds/leds-pca9532.h>
>>> @@ -33,6 +35,14 @@ Example:
>>>        label = "pca:green:power";
>>>        type = <PCA9532_TYPE_LED>;
>>>      };
>>> +    kernel-booting {
>>> +    	type = <PCA9532_TYPE_LED>;
>>> +    	default-state = "on";
>>> +    };
>>> +    sys-stat {
>>> +    	type = <PCA9532_TYPE_LED>;
>>> +    	default-state = "keep"; // don't touch, was set by U-Boot
>>> +    };
>>
>> Adjusted above indentation to match the preceding lines.
> 
>>> @@ -475,6 +494,16 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
>>>  		of_property_read_u32(child, "type", &pdata->leds[i].type);
>>>  		of_property_read_string(child, "linux,default-trigger",
>>>  					&pdata->leds[i].default_trigger);
>>> +		if (!of_property_read_string(child, "default-state", &state)) {
>>> +			if (!strcmp(state, "on"))
>>> +				pdata->leds[i].state = PCA9532_ON;
>>> +			else if (!strcmp(state, "keep"))
>>> +				pdata->leds[i].state = PCA9532_KEEP;
>>> +			else if (!strcmp(state, "pwm0"))
>>> +				pdata->leds[i].state = PCA9532_PWM0;
>>> +			else if (!strcmp(state, "pwm1"))
>>> +				pdata->leds[i].state = PCA9532_PWM1;
>>> +		}
>>>  		if (++i >= maxleds) {
>>>  			of_node_put(child);
>>>  			break;
> 
> This seems to look for "pwm0" and "pwm1" strings, which do not seem to
> be documented.
> 
> Plus... is it useful to have default-state? We already have default
> trigger. If we keep the value by default (on PC, we do something like
> that) this patch should not be neccessary?

Thanks for the heads-up. Dropping the patch for now.
I guess that pwm0/1 got propagated to v2 by an omission.

Regarding default-on: Felix, do you have any use case that require
default-on set to "keep"?

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Extend pca9532 device tree support
@ 2017-04-06 19:00         ` Jacek Anaszewski
  0 siblings, 0 replies; 14+ messages in thread
From: Jacek Anaszewski @ 2017-04-06 19:00 UTC (permalink / raw)
  To: Pavel Machek, Felix Brack
  Cc: rpurdie, mark.rutland, riku.voipio, linux-leds, devicetree, linux-kernel

Hi Pavel,

On 04/06/2017 05:50 PM, Pavel Machek wrote:
> Hi!
> 
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>> index 198f3ba..8374075 100644
>>> --- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>> +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>> @@ -17,6 +17,8 @@ Optional sub-node properties:
>>>  	- label: see Documentation/devicetree/bindings/leds/common.txt
>>>  	- type: Output configuration, see dt-bindings/leds/leds-pca9532.h (default NONE)
>>>  	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
>>> +	- default-state: see Documentation/devicetree/bindings/leds/common.txt
>>> +	  This property is only valid for sub-nodes of type <PCA9532_TYPE_LED>.
>>>  
>>>  Example:
>>>    #include <dt-bindings/leds/leds-pca9532.h>
>>> @@ -33,6 +35,14 @@ Example:
>>>        label = "pca:green:power";
>>>        type = <PCA9532_TYPE_LED>;
>>>      };
>>> +    kernel-booting {
>>> +    	type = <PCA9532_TYPE_LED>;
>>> +    	default-state = "on";
>>> +    };
>>> +    sys-stat {
>>> +    	type = <PCA9532_TYPE_LED>;
>>> +    	default-state = "keep"; // don't touch, was set by U-Boot
>>> +    };
>>
>> Adjusted above indentation to match the preceding lines.
> 
>>> @@ -475,6 +494,16 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
>>>  		of_property_read_u32(child, "type", &pdata->leds[i].type);
>>>  		of_property_read_string(child, "linux,default-trigger",
>>>  					&pdata->leds[i].default_trigger);
>>> +		if (!of_property_read_string(child, "default-state", &state)) {
>>> +			if (!strcmp(state, "on"))
>>> +				pdata->leds[i].state = PCA9532_ON;
>>> +			else if (!strcmp(state, "keep"))
>>> +				pdata->leds[i].state = PCA9532_KEEP;
>>> +			else if (!strcmp(state, "pwm0"))
>>> +				pdata->leds[i].state = PCA9532_PWM0;
>>> +			else if (!strcmp(state, "pwm1"))
>>> +				pdata->leds[i].state = PCA9532_PWM1;
>>> +		}
>>>  		if (++i >= maxleds) {
>>>  			of_node_put(child);
>>>  			break;
> 
> This seems to look for "pwm0" and "pwm1" strings, which do not seem to
> be documented.
> 
> Plus... is it useful to have default-state? We already have default
> trigger. If we keep the value by default (on PC, we do something like
> that) this patch should not be neccessary?

Thanks for the heads-up. Dropping the patch for now.
I guess that pwm0/1 got propagated to v2 by an omission.

Regarding default-on: Felix, do you have any use case that require
default-on set to "keep"?

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Extend pca9532 device tree support
  2017-04-06 19:00         ` Jacek Anaszewski
@ 2017-04-07  8:22             ` Felix Brack
  -1 siblings, 0 replies; 14+ messages in thread
From: Felix Brack @ 2017-04-07  8:22 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek
  Cc: rpurdie-Fm38FmjxZ/leoWH0uzbU5w, mark.rutland-5wv7dgnIgG8,
	riku.voipio-X3B1VOXEql0, linux-leds-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

Hello Jacek,

On 06.04.2017 21:00, Jacek Anaszewski wrote:
> Hi Pavel,
> 
> On 04/06/2017 05:50 PM, Pavel Machek wrote:
>> Hi!
>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>> index 198f3ba..8374075 100644
>>>> --- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>> +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>> @@ -17,6 +17,8 @@ Optional sub-node properties:
>>>>  	- label: see Documentation/devicetree/bindings/leds/common.txt
>>>>  	- type: Output configuration, see dt-bindings/leds/leds-pca9532.h (default NONE)
>>>>  	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
>>>> +	- default-state: see Documentation/devicetree/bindings/leds/common.txt
>>>> +	  This property is only valid for sub-nodes of type <PCA9532_TYPE_LED>.
>>>>  
>>>>  Example:
>>>>    #include <dt-bindings/leds/leds-pca9532.h>
>>>> @@ -33,6 +35,14 @@ Example:
>>>>        label = "pca:green:power";
>>>>        type = <PCA9532_TYPE_LED>;
>>>>      };
>>>> +    kernel-booting {
>>>> +    	type = <PCA9532_TYPE_LED>;
>>>> +    	default-state = "on";
>>>> +    };
>>>> +    sys-stat {
>>>> +    	type = <PCA9532_TYPE_LED>;
>>>> +    	default-state = "keep"; // don't touch, was set by U-Boot
>>>> +    };
>>>
>>> Adjusted above indentation to match the preceding lines.
>>
>>>> @@ -475,6 +494,16 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
>>>>  		of_property_read_u32(child, "type", &pdata->leds[i].type);
>>>>  		of_property_read_string(child, "linux,default-trigger",
>>>>  					&pdata->leds[i].default_trigger);
>>>> +		if (!of_property_read_string(child, "default-state", &state)) {
>>>> +			if (!strcmp(state, "on"))
>>>> +				pdata->leds[i].state = PCA9532_ON;
>>>> +			else if (!strcmp(state, "keep"))
>>>> +				pdata->leds[i].state = PCA9532_KEEP;
>>>> +			else if (!strcmp(state, "pwm0"))
>>>> +				pdata->leds[i].state = PCA9532_PWM0;
>>>> +			else if (!strcmp(state, "pwm1"))
>>>> +				pdata->leds[i].state = PCA9532_PWM1;
>>>> +		}
>>>>  		if (++i >= maxleds) {
>>>>  			of_node_put(child);
>>>>  			break;
>>
>> This seems to look for "pwm0" and "pwm1" strings, which do not seem to
>> be documented.
>>
>> Plus... is it useful to have default-state? We already have default
>> trigger. If we keep the value by default (on PC, we do something like
>> that) this patch should not be neccessary?
> 
> Thanks for the heads-up. Dropping the patch for now.

No, please do not drop the patch.

> I guess that pwm0/1 got propagated to v2 by an omission.
> 

Yes, I agree. However the two strings do not break anything and behave
analog to the 'on' or 'keep' string. Though this code could be removed
if absolutely necessary. An alternative would be to add a description
for the strings. Just to be clear: these strings have nothing to with
the exposition of device specific registers to the DT.

> Regarding default-on: Felix, do you have any use case that require
> default-on set to "keep"?
> 

This patch is not about 'default-on' which is a value that could be
assigned to the property 'linux,default-trigger' (according to DT
bindings documentation file 'common.txt').
My patch does not introduce anything new with the'keep' state, it rather
completes the existing bindings according to the description in
Documentation/devicetree/bindings/leds/common.txt which states:

....
- default-state : The initial state of the LED. Valid values are "on",
"off", and "keep". If the LED is already on or off and the default-state
property is set the to same value, then no glitch should be produced
where the LED momentarily turns off (or on). The "keep" setting will
keep the LED at whatever its current state is, without producing a
glitch.  The default is off if this property is not present.
....

One of my use cases is to turn a LED on by U-Boot. This LED must remain
on until eventually, under certain conditions, some userland code
changes it's state.
Setting 'default-state' to 'keep' is how you can sort of tell the
kernel, or better the driver, 'not to initialize the LED' which would
turn it off.

kind regards, Felix
--
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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Extend pca9532 device tree support
@ 2017-04-07  8:22             ` Felix Brack
  0 siblings, 0 replies; 14+ messages in thread
From: Felix Brack @ 2017-04-07  8:22 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek
  Cc: rpurdie, mark.rutland, riku.voipio, linux-leds, devicetree, linux-kernel

Hello Jacek,

On 06.04.2017 21:00, Jacek Anaszewski wrote:
> Hi Pavel,
> 
> On 04/06/2017 05:50 PM, Pavel Machek wrote:
>> Hi!
>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>> index 198f3ba..8374075 100644
>>>> --- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>> +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>> @@ -17,6 +17,8 @@ Optional sub-node properties:
>>>>  	- label: see Documentation/devicetree/bindings/leds/common.txt
>>>>  	- type: Output configuration, see dt-bindings/leds/leds-pca9532.h (default NONE)
>>>>  	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
>>>> +	- default-state: see Documentation/devicetree/bindings/leds/common.txt
>>>> +	  This property is only valid for sub-nodes of type <PCA9532_TYPE_LED>.
>>>>  
>>>>  Example:
>>>>    #include <dt-bindings/leds/leds-pca9532.h>
>>>> @@ -33,6 +35,14 @@ Example:
>>>>        label = "pca:green:power";
>>>>        type = <PCA9532_TYPE_LED>;
>>>>      };
>>>> +    kernel-booting {
>>>> +    	type = <PCA9532_TYPE_LED>;
>>>> +    	default-state = "on";
>>>> +    };
>>>> +    sys-stat {
>>>> +    	type = <PCA9532_TYPE_LED>;
>>>> +    	default-state = "keep"; // don't touch, was set by U-Boot
>>>> +    };
>>>
>>> Adjusted above indentation to match the preceding lines.
>>
>>>> @@ -475,6 +494,16 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
>>>>  		of_property_read_u32(child, "type", &pdata->leds[i].type);
>>>>  		of_property_read_string(child, "linux,default-trigger",
>>>>  					&pdata->leds[i].default_trigger);
>>>> +		if (!of_property_read_string(child, "default-state", &state)) {
>>>> +			if (!strcmp(state, "on"))
>>>> +				pdata->leds[i].state = PCA9532_ON;
>>>> +			else if (!strcmp(state, "keep"))
>>>> +				pdata->leds[i].state = PCA9532_KEEP;
>>>> +			else if (!strcmp(state, "pwm0"))
>>>> +				pdata->leds[i].state = PCA9532_PWM0;
>>>> +			else if (!strcmp(state, "pwm1"))
>>>> +				pdata->leds[i].state = PCA9532_PWM1;
>>>> +		}
>>>>  		if (++i >= maxleds) {
>>>>  			of_node_put(child);
>>>>  			break;
>>
>> This seems to look for "pwm0" and "pwm1" strings, which do not seem to
>> be documented.
>>
>> Plus... is it useful to have default-state? We already have default
>> trigger. If we keep the value by default (on PC, we do something like
>> that) this patch should not be neccessary?
> 
> Thanks for the heads-up. Dropping the patch for now.

No, please do not drop the patch.

> I guess that pwm0/1 got propagated to v2 by an omission.
> 

Yes, I agree. However the two strings do not break anything and behave
analog to the 'on' or 'keep' string. Though this code could be removed
if absolutely necessary. An alternative would be to add a description
for the strings. Just to be clear: these strings have nothing to with
the exposition of device specific registers to the DT.

> Regarding default-on: Felix, do you have any use case that require
> default-on set to "keep"?
> 

This patch is not about 'default-on' which is a value that could be
assigned to the property 'linux,default-trigger' (according to DT
bindings documentation file 'common.txt').
My patch does not introduce anything new with the'keep' state, it rather
completes the existing bindings according to the description in
Documentation/devicetree/bindings/leds/common.txt which states:

....
- default-state : The initial state of the LED. Valid values are "on",
"off", and "keep". If the LED is already on or off and the default-state
property is set the to same value, then no glitch should be produced
where the LED momentarily turns off (or on). The "keep" setting will
keep the LED at whatever its current state is, without producing a
glitch.  The default is off if this property is not present.
....

One of my use cases is to turn a LED on by U-Boot. This LED must remain
on until eventually, under certain conditions, some userland code
changes it's state.
Setting 'default-state' to 'keep' is how you can sort of tell the
kernel, or better the driver, 'not to initialize the LED' which would
turn it off.

kind regards, Felix

^ permalink raw reply	[flat|nested] 14+ messages in thread

* default-state LED property (was Re: [PATCH v2] Extend pca9532 device tree support)
  2017-04-07  8:22             ` Felix Brack
@ 2017-04-07 11:57                 ` Pavel Machek
  -1 siblings, 0 replies; 14+ messages in thread
From: Pavel Machek @ 2017-04-07 11:57 UTC (permalink / raw)
  To: Felix Brack
  Cc: Jacek Anaszewski, rpurdie-Fm38FmjxZ/leoWH0uzbU5w,
	mark.rutland-5wv7dgnIgG8, riku.voipio-X3B1VOXEql0,
	linux-leds-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linus.walleij-QSEj5FYQhm4dnm+yROfE0A,
	robh-DgEjT+Ai2ygdnm+yROfE0A

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

Hi!

> >> This seems to look for "pwm0" and "pwm1" strings, which do not seem to
> >> be documented.
> >>
> >> Plus... is it useful to have default-state? We already have default
> >> trigger. If we keep the value by default (on PC, we do something like
> >> that) this patch should not be neccessary?
> > 
> > Thanks for the heads-up. Dropping the patch for now.
> 
> No, please do not drop the patch.
> 
> > I guess that pwm0/1 got propagated to v2 by an omission.
> 
> Yes, I agree. However the two strings do not break anything and behave
> analog to the 'on' or 'keep' string. Though this code could be removed
> if absolutely necessary. An alternative would be to add a description
> for the strings. Just to be clear: these strings have nothing to with
> the exposition of device specific registers to the DT.

Well, they should be removed. No need to keep surprises in code.

> > Regarding default-on: Felix, do you have any use case that require
> > default-on set to "keep"?
> 
> This patch is not about 'default-on' which is a value that could be
> assigned to the property 'linux,default-trigger' (according to DT
> bindings documentation file 'common.txt').
> My patch does not introduce anything new with the'keep' state, it rather
> completes the existing bindings according to the description in
> Documentation/devicetree/bindings/leds/common.txt which states:

Hmm, that was introduce by 1d1a77ddc8acfa3d506f1958e09a12085e71fc69
. I don't quite like the patch -- it duplicates default-trigger
functionality without good reason.

IMO we should

1) keep the led state by default

2) deprecate the default-state property

3) use default-triggers to emulate the functionality if someone really
needs it.

> One of my use cases is to turn a LED on by U-Boot. This LED must remain
> on until eventually, under certain conditions, some userland code
> changes it's state.
> Setting 'default-state' to 'keep' is how you can sort of tell the
> kernel, or better the driver, 'not to initialize the LED' which would
> turn it off.

Yes, that's reasonable. Perhaps 'keep' should be the default.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* default-state LED property (was Re: [PATCH v2] Extend pca9532 device tree support)
@ 2017-04-07 11:57                 ` Pavel Machek
  0 siblings, 0 replies; 14+ messages in thread
From: Pavel Machek @ 2017-04-07 11:57 UTC (permalink / raw)
  To: Felix Brack
  Cc: Jacek Anaszewski, rpurdie, mark.rutland, riku.voipio, linux-leds,
	devicetree, linux-kernel, linus.walleij, robh

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

Hi!

> >> This seems to look for "pwm0" and "pwm1" strings, which do not seem to
> >> be documented.
> >>
> >> Plus... is it useful to have default-state? We already have default
> >> trigger. If we keep the value by default (on PC, we do something like
> >> that) this patch should not be neccessary?
> > 
> > Thanks for the heads-up. Dropping the patch for now.
> 
> No, please do not drop the patch.
> 
> > I guess that pwm0/1 got propagated to v2 by an omission.
> 
> Yes, I agree. However the two strings do not break anything and behave
> analog to the 'on' or 'keep' string. Though this code could be removed
> if absolutely necessary. An alternative would be to add a description
> for the strings. Just to be clear: these strings have nothing to with
> the exposition of device specific registers to the DT.

Well, they should be removed. No need to keep surprises in code.

> > Regarding default-on: Felix, do you have any use case that require
> > default-on set to "keep"?
> 
> This patch is not about 'default-on' which is a value that could be
> assigned to the property 'linux,default-trigger' (according to DT
> bindings documentation file 'common.txt').
> My patch does not introduce anything new with the'keep' state, it rather
> completes the existing bindings according to the description in
> Documentation/devicetree/bindings/leds/common.txt which states:

Hmm, that was introduce by 1d1a77ddc8acfa3d506f1958e09a12085e71fc69
. I don't quite like the patch -- it duplicates default-trigger
functionality without good reason.

IMO we should

1) keep the led state by default

2) deprecate the default-state property

3) use default-triggers to emulate the functionality if someone really
needs it.

> One of my use cases is to turn a LED on by U-Boot. This LED must remain
> on until eventually, under certain conditions, some userland code
> changes it's state.
> Setting 'default-state' to 'keep' is how you can sort of tell the
> kernel, or better the driver, 'not to initialize the LED' which would
> turn it off.

Yes, that's reasonable. Perhaps 'keep' should be the default.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Extend pca9532 device tree support
  2017-04-07  8:22             ` Felix Brack
@ 2017-04-09 12:37                 ` Jacek Anaszewski
  -1 siblings, 0 replies; 14+ messages in thread
From: Jacek Anaszewski @ 2017-04-09 12:37 UTC (permalink / raw)
  To: Felix Brack, Pavel Machek
  Cc: rpurdie-Fm38FmjxZ/leoWH0uzbU5w, mark.rutland-5wv7dgnIgG8,
	riku.voipio-X3B1VOXEql0, linux-leds-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

Hello Felix,

On 04/07/2017 10:22 AM, Felix Brack wrote:
> Hello Jacek,
> 
> On 06.04.2017 21:00, Jacek Anaszewski wrote:
>> Hi Pavel,
>>
>> On 04/06/2017 05:50 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>> index 198f3ba..8374075 100644
>>>>> --- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>> @@ -17,6 +17,8 @@ Optional sub-node properties:
>>>>>  	- label: see Documentation/devicetree/bindings/leds/common.txt
>>>>>  	- type: Output configuration, see dt-bindings/leds/leds-pca9532.h (default NONE)
>>>>>  	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
>>>>> +	- default-state: see Documentation/devicetree/bindings/leds/common.txt
>>>>> +	  This property is only valid for sub-nodes of type <PCA9532_TYPE_LED>.
>>>>>  
>>>>>  Example:
>>>>>    #include <dt-bindings/leds/leds-pca9532.h>
>>>>> @@ -33,6 +35,14 @@ Example:
>>>>>        label = "pca:green:power";
>>>>>        type = <PCA9532_TYPE_LED>;
>>>>>      };
>>>>> +    kernel-booting {
>>>>> +    	type = <PCA9532_TYPE_LED>;
>>>>> +    	default-state = "on";
>>>>> +    };
>>>>> +    sys-stat {
>>>>> +    	type = <PCA9532_TYPE_LED>;
>>>>> +    	default-state = "keep"; // don't touch, was set by U-Boot
>>>>> +    };
>>>>
>>>> Adjusted above indentation to match the preceding lines.
>>>
>>>>> @@ -475,6 +494,16 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
>>>>>  		of_property_read_u32(child, "type", &pdata->leds[i].type);
>>>>>  		of_property_read_string(child, "linux,default-trigger",
>>>>>  					&pdata->leds[i].default_trigger);
>>>>> +		if (!of_property_read_string(child, "default-state", &state)) {
>>>>> +			if (!strcmp(state, "on"))
>>>>> +				pdata->leds[i].state = PCA9532_ON;
>>>>> +			else if (!strcmp(state, "keep"))
>>>>> +				pdata->leds[i].state = PCA9532_KEEP;
>>>>> +			else if (!strcmp(state, "pwm0"))
>>>>> +				pdata->leds[i].state = PCA9532_PWM0;
>>>>> +			else if (!strcmp(state, "pwm1"))
>>>>> +				pdata->leds[i].state = PCA9532_PWM1;
>>>>> +		}
>>>>>  		if (++i >= maxleds) {
>>>>>  			of_node_put(child);
>>>>>  			break;
>>>
>>> This seems to look for "pwm0" and "pwm1" strings, which do not seem to
>>> be documented.
>>>
>>> Plus... is it useful to have default-state? We already have default
>>> trigger. If we keep the value by default (on PC, we do something like
>>> that) this patch should not be neccessary?
>>
>> Thanks for the heads-up. Dropping the patch for now.
> 
> No, please do not drop the patch.
> 
>> I guess that pwm0/1 got propagated to v2 by an omission.
>>
> 
> Yes, I agree. However the two strings do not break anything and behave
> analog to the 'on' or 'keep' string. Though this code could be removed
> if absolutely necessary. An alternative would be to add a description
> for the strings. Just to be clear: these strings have nothing to with
> the exposition of device specific registers to the DT.
> 
>> Regarding default-on: Felix, do you have any use case that require
>> default-on set to "keep"?
>>
> 
> This patch is not about 'default-on' which is a value that could be
> assigned to the property 'linux,default-trigger' (according to DT
> bindings documentation file 'common.txt').
> My patch does not introduce anything new with the'keep' state, it rather
> completes the existing bindings according to the description in
> Documentation/devicetree/bindings/leds/common.txt which states:
> 
> ....
> - default-state : The initial state of the LED. Valid values are "on",
> "off", and "keep". If the LED is already on or off and the default-state
> property is set the to same value, then no glitch should be produced
> where the LED momentarily turns off (or on). The "keep" setting will
> keep the LED at whatever its current state is, without producing a
> glitch.  The default is off if this property is not present.
> ....
> 
> One of my use cases is to turn a LED on by U-Boot. This LED must remain
> on until eventually, under certain conditions, some userland code
> changes it's state.
> Setting 'default-state' to 'keep' is how you can sort of tell the
> kernel, or better the driver, 'not to initialize the LED' which would
> turn it off.

Thanks for the explanation. Could you please sent v3 with removed pwm*
cases then?

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Extend pca9532 device tree support
@ 2017-04-09 12:37                 ` Jacek Anaszewski
  0 siblings, 0 replies; 14+ messages in thread
From: Jacek Anaszewski @ 2017-04-09 12:37 UTC (permalink / raw)
  To: Felix Brack, Pavel Machek
  Cc: rpurdie, mark.rutland, riku.voipio, linux-leds, devicetree, linux-kernel

Hello Felix,

On 04/07/2017 10:22 AM, Felix Brack wrote:
> Hello Jacek,
> 
> On 06.04.2017 21:00, Jacek Anaszewski wrote:
>> Hi Pavel,
>>
>> On 04/06/2017 05:50 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>> index 198f3ba..8374075 100644
>>>>> --- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>> @@ -17,6 +17,8 @@ Optional sub-node properties:
>>>>>  	- label: see Documentation/devicetree/bindings/leds/common.txt
>>>>>  	- type: Output configuration, see dt-bindings/leds/leds-pca9532.h (default NONE)
>>>>>  	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
>>>>> +	- default-state: see Documentation/devicetree/bindings/leds/common.txt
>>>>> +	  This property is only valid for sub-nodes of type <PCA9532_TYPE_LED>.
>>>>>  
>>>>>  Example:
>>>>>    #include <dt-bindings/leds/leds-pca9532.h>
>>>>> @@ -33,6 +35,14 @@ Example:
>>>>>        label = "pca:green:power";
>>>>>        type = <PCA9532_TYPE_LED>;
>>>>>      };
>>>>> +    kernel-booting {
>>>>> +    	type = <PCA9532_TYPE_LED>;
>>>>> +    	default-state = "on";
>>>>> +    };
>>>>> +    sys-stat {
>>>>> +    	type = <PCA9532_TYPE_LED>;
>>>>> +    	default-state = "keep"; // don't touch, was set by U-Boot
>>>>> +    };
>>>>
>>>> Adjusted above indentation to match the preceding lines.
>>>
>>>>> @@ -475,6 +494,16 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
>>>>>  		of_property_read_u32(child, "type", &pdata->leds[i].type);
>>>>>  		of_property_read_string(child, "linux,default-trigger",
>>>>>  					&pdata->leds[i].default_trigger);
>>>>> +		if (!of_property_read_string(child, "default-state", &state)) {
>>>>> +			if (!strcmp(state, "on"))
>>>>> +				pdata->leds[i].state = PCA9532_ON;
>>>>> +			else if (!strcmp(state, "keep"))
>>>>> +				pdata->leds[i].state = PCA9532_KEEP;
>>>>> +			else if (!strcmp(state, "pwm0"))
>>>>> +				pdata->leds[i].state = PCA9532_PWM0;
>>>>> +			else if (!strcmp(state, "pwm1"))
>>>>> +				pdata->leds[i].state = PCA9532_PWM1;
>>>>> +		}
>>>>>  		if (++i >= maxleds) {
>>>>>  			of_node_put(child);
>>>>>  			break;
>>>
>>> This seems to look for "pwm0" and "pwm1" strings, which do not seem to
>>> be documented.
>>>
>>> Plus... is it useful to have default-state? We already have default
>>> trigger. If we keep the value by default (on PC, we do something like
>>> that) this patch should not be neccessary?
>>
>> Thanks for the heads-up. Dropping the patch for now.
> 
> No, please do not drop the patch.
> 
>> I guess that pwm0/1 got propagated to v2 by an omission.
>>
> 
> Yes, I agree. However the two strings do not break anything and behave
> analog to the 'on' or 'keep' string. Though this code could be removed
> if absolutely necessary. An alternative would be to add a description
> for the strings. Just to be clear: these strings have nothing to with
> the exposition of device specific registers to the DT.
> 
>> Regarding default-on: Felix, do you have any use case that require
>> default-on set to "keep"?
>>
> 
> This patch is not about 'default-on' which is a value that could be
> assigned to the property 'linux,default-trigger' (according to DT
> bindings documentation file 'common.txt').
> My patch does not introduce anything new with the'keep' state, it rather
> completes the existing bindings according to the description in
> Documentation/devicetree/bindings/leds/common.txt which states:
> 
> ....
> - default-state : The initial state of the LED. Valid values are "on",
> "off", and "keep". If the LED is already on or off and the default-state
> property is set the to same value, then no glitch should be produced
> where the LED momentarily turns off (or on). The "keep" setting will
> keep the LED at whatever its current state is, without producing a
> glitch.  The default is off if this property is not present.
> ....
> 
> One of my use cases is to turn a LED on by U-Boot. This LED must remain
> on until eventually, under certain conditions, some userland code
> changes it's state.
> Setting 'default-state' to 'keep' is how you can sort of tell the
> kernel, or better the driver, 'not to initialize the LED' which would
> turn it off.

Thanks for the explanation. Could you please sent v3 with removed pwm*
cases then?

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Extend pca9532 device tree support
  2017-04-09 12:37                 ` Jacek Anaszewski
@ 2017-04-09 13:11                     ` Felix Brack
  -1 siblings, 0 replies; 14+ messages in thread
From: Felix Brack @ 2017-04-09 13:11 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek
  Cc: rpurdie-Fm38FmjxZ/leoWH0uzbU5w, mark.rutland-5wv7dgnIgG8,
	riku.voipio-X3B1VOXEql0, linux-leds-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

Hello Jacek,

On 09.04.2017 14:37, Jacek Anaszewski wrote:
> Hello Felix,
> 
> On 04/07/2017 10:22 AM, Felix Brack wrote:
>> Hello Jacek,
>>
>> On 06.04.2017 21:00, Jacek Anaszewski wrote:
>>> Hi Pavel,
>>>
>>> On 04/06/2017 05:50 PM, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>>> index 198f3ba..8374075 100644
>>>>>> --- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>>> @@ -17,6 +17,8 @@ Optional sub-node properties:
>>>>>>  	- label: see Documentation/devicetree/bindings/leds/common.txt
>>>>>>  	- type: Output configuration, see dt-bindings/leds/leds-pca9532.h (default NONE)
>>>>>>  	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
>>>>>> +	- default-state: see Documentation/devicetree/bindings/leds/common.txt
>>>>>> +	  This property is only valid for sub-nodes of type <PCA9532_TYPE_LED>.
>>>>>>  
>>>>>>  Example:
>>>>>>    #include <dt-bindings/leds/leds-pca9532.h>
>>>>>> @@ -33,6 +35,14 @@ Example:
>>>>>>        label = "pca:green:power";
>>>>>>        type = <PCA9532_TYPE_LED>;
>>>>>>      };
>>>>>> +    kernel-booting {
>>>>>> +    	type = <PCA9532_TYPE_LED>;
>>>>>> +    	default-state = "on";
>>>>>> +    };
>>>>>> +    sys-stat {
>>>>>> +    	type = <PCA9532_TYPE_LED>;
>>>>>> +    	default-state = "keep"; // don't touch, was set by U-Boot
>>>>>> +    };
>>>>>
>>>>> Adjusted above indentation to match the preceding lines.
>>>>
>>>>>> @@ -475,6 +494,16 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
>>>>>>  		of_property_read_u32(child, "type", &pdata->leds[i].type);
>>>>>>  		of_property_read_string(child, "linux,default-trigger",
>>>>>>  					&pdata->leds[i].default_trigger);
>>>>>> +		if (!of_property_read_string(child, "default-state", &state)) {
>>>>>> +			if (!strcmp(state, "on"))
>>>>>> +				pdata->leds[i].state = PCA9532_ON;
>>>>>> +			else if (!strcmp(state, "keep"))
>>>>>> +				pdata->leds[i].state = PCA9532_KEEP;
>>>>>> +			else if (!strcmp(state, "pwm0"))
>>>>>> +				pdata->leds[i].state = PCA9532_PWM0;
>>>>>> +			else if (!strcmp(state, "pwm1"))
>>>>>> +				pdata->leds[i].state = PCA9532_PWM1;
>>>>>> +		}
>>>>>>  		if (++i >= maxleds) {
>>>>>>  			of_node_put(child);
>>>>>>  			break;
>>>>
>>>> This seems to look for "pwm0" and "pwm1" strings, which do not seem to
>>>> be documented.
>>>>
>>>> Plus... is it useful to have default-state? We already have default
>>>> trigger. If we keep the value by default (on PC, we do something like
>>>> that) this patch should not be neccessary?
>>>
>>> Thanks for the heads-up. Dropping the patch for now.
>>
>> No, please do not drop the patch.
>>
>>> I guess that pwm0/1 got propagated to v2 by an omission.
>>>
>>
>> Yes, I agree. However the two strings do not break anything and behave
>> analog to the 'on' or 'keep' string. Though this code could be removed
>> if absolutely necessary. An alternative would be to add a description
>> for the strings. Just to be clear: these strings have nothing to with
>> the exposition of device specific registers to the DT.
>>
>>> Regarding default-on: Felix, do you have any use case that require
>>> default-on set to "keep"?
>>>
>>
>> This patch is not about 'default-on' which is a value that could be
>> assigned to the property 'linux,default-trigger' (according to DT
>> bindings documentation file 'common.txt').
>> My patch does not introduce anything new with the'keep' state, it rather
>> completes the existing bindings according to the description in
>> Documentation/devicetree/bindings/leds/common.txt which states:
>>
>> ....
>> - default-state : The initial state of the LED. Valid values are "on",
>> "off", and "keep". If the LED is already on or off and the default-state
>> property is set the to same value, then no glitch should be produced
>> where the LED momentarily turns off (or on). The "keep" setting will
>> keep the LED at whatever its current state is, without producing a
>> glitch.  The default is off if this property is not present.
>> ....
>>
>> One of my use cases is to turn a LED on by U-Boot. This LED must remain
>> on until eventually, under certain conditions, some userland code
>> changes it's state.
>> Setting 'default-state' to 'keep' is how you can sort of tell the
>> kernel, or better the driver, 'not to initialize the LED' which would
>> turn it off.
> 
> Thanks for the explanation. Could you please sent v3 with removed pwm*
> cases then?
> 

Yes, I will try to do so next week.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] Extend pca9532 device tree support
@ 2017-04-09 13:11                     ` Felix Brack
  0 siblings, 0 replies; 14+ messages in thread
From: Felix Brack @ 2017-04-09 13:11 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek
  Cc: rpurdie, mark.rutland, riku.voipio, linux-leds, devicetree, linux-kernel

Hello Jacek,

On 09.04.2017 14:37, Jacek Anaszewski wrote:
> Hello Felix,
> 
> On 04/07/2017 10:22 AM, Felix Brack wrote:
>> Hello Jacek,
>>
>> On 06.04.2017 21:00, Jacek Anaszewski wrote:
>>> Hi Pavel,
>>>
>>> On 04/06/2017 05:50 PM, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>>> index 198f3ba..8374075 100644
>>>>>> --- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
>>>>>> @@ -17,6 +17,8 @@ Optional sub-node properties:
>>>>>>  	- label: see Documentation/devicetree/bindings/leds/common.txt
>>>>>>  	- type: Output configuration, see dt-bindings/leds/leds-pca9532.h (default NONE)
>>>>>>  	- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt
>>>>>> +	- default-state: see Documentation/devicetree/bindings/leds/common.txt
>>>>>> +	  This property is only valid for sub-nodes of type <PCA9532_TYPE_LED>.
>>>>>>  
>>>>>>  Example:
>>>>>>    #include <dt-bindings/leds/leds-pca9532.h>
>>>>>> @@ -33,6 +35,14 @@ Example:
>>>>>>        label = "pca:green:power";
>>>>>>        type = <PCA9532_TYPE_LED>;
>>>>>>      };
>>>>>> +    kernel-booting {
>>>>>> +    	type = <PCA9532_TYPE_LED>;
>>>>>> +    	default-state = "on";
>>>>>> +    };
>>>>>> +    sys-stat {
>>>>>> +    	type = <PCA9532_TYPE_LED>;
>>>>>> +    	default-state = "keep"; // don't touch, was set by U-Boot
>>>>>> +    };
>>>>>
>>>>> Adjusted above indentation to match the preceding lines.
>>>>
>>>>>> @@ -475,6 +494,16 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
>>>>>>  		of_property_read_u32(child, "type", &pdata->leds[i].type);
>>>>>>  		of_property_read_string(child, "linux,default-trigger",
>>>>>>  					&pdata->leds[i].default_trigger);
>>>>>> +		if (!of_property_read_string(child, "default-state", &state)) {
>>>>>> +			if (!strcmp(state, "on"))
>>>>>> +				pdata->leds[i].state = PCA9532_ON;
>>>>>> +			else if (!strcmp(state, "keep"))
>>>>>> +				pdata->leds[i].state = PCA9532_KEEP;
>>>>>> +			else if (!strcmp(state, "pwm0"))
>>>>>> +				pdata->leds[i].state = PCA9532_PWM0;
>>>>>> +			else if (!strcmp(state, "pwm1"))
>>>>>> +				pdata->leds[i].state = PCA9532_PWM1;
>>>>>> +		}
>>>>>>  		if (++i >= maxleds) {
>>>>>>  			of_node_put(child);
>>>>>>  			break;
>>>>
>>>> This seems to look for "pwm0" and "pwm1" strings, which do not seem to
>>>> be documented.
>>>>
>>>> Plus... is it useful to have default-state? We already have default
>>>> trigger. If we keep the value by default (on PC, we do something like
>>>> that) this patch should not be neccessary?
>>>
>>> Thanks for the heads-up. Dropping the patch for now.
>>
>> No, please do not drop the patch.
>>
>>> I guess that pwm0/1 got propagated to v2 by an omission.
>>>
>>
>> Yes, I agree. However the two strings do not break anything and behave
>> analog to the 'on' or 'keep' string. Though this code could be removed
>> if absolutely necessary. An alternative would be to add a description
>> for the strings. Just to be clear: these strings have nothing to with
>> the exposition of device specific registers to the DT.
>>
>>> Regarding default-on: Felix, do you have any use case that require
>>> default-on set to "keep"?
>>>
>>
>> This patch is not about 'default-on' which is a value that could be
>> assigned to the property 'linux,default-trigger' (according to DT
>> bindings documentation file 'common.txt').
>> My patch does not introduce anything new with the'keep' state, it rather
>> completes the existing bindings according to the description in
>> Documentation/devicetree/bindings/leds/common.txt which states:
>>
>> ....
>> - default-state : The initial state of the LED. Valid values are "on",
>> "off", and "keep". If the LED is already on or off and the default-state
>> property is set the to same value, then no glitch should be produced
>> where the LED momentarily turns off (or on). The "keep" setting will
>> keep the LED at whatever its current state is, without producing a
>> glitch.  The default is off if this property is not present.
>> ....
>>
>> One of my use cases is to turn a LED on by U-Boot. This LED must remain
>> on until eventually, under certain conditions, some userland code
>> changes it's state.
>> Setting 'default-state' to 'keep' is how you can sort of tell the
>> kernel, or better the driver, 'not to initialize the LED' which would
>> turn it off.
> 
> Thanks for the explanation. Could you please sent v3 with removed pwm*
> cases then?
> 

Yes, I will try to do so next week.

--
regards Felix

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-04-09 13:11 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-30 13:33 [PATCH v2] Extend pca9532 device tree support Felix Brack
2017-04-02 14:41 ` Jacek Anaszewski
     [not found]   ` <b54c77d5-b4f7-9517-8c5b-9200c766204a-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-04-06 15:50     ` Pavel Machek
2017-04-06 15:50       ` Pavel Machek
2017-04-06 19:00       ` Jacek Anaszewski
2017-04-06 19:00         ` Jacek Anaszewski
     [not found]         ` <a8509d26-6e35-e2f7-9784-90f3c54accbf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-04-07  8:22           ` Felix Brack
2017-04-07  8:22             ` Felix Brack
     [not found]             ` <ec012568-f540-4e87-d8c4-4065fbcd1c2a-GovowT2ENgg@public.gmane.org>
2017-04-07 11:57               ` default-state LED property (was Re: [PATCH v2] Extend pca9532 device tree support) Pavel Machek
2017-04-07 11:57                 ` Pavel Machek
2017-04-09 12:37               ` [PATCH v2] Extend pca9532 device tree support Jacek Anaszewski
2017-04-09 12:37                 ` Jacek Anaszewski
     [not found]                 ` <2f3815ab-71e1-b63d-aba8-167e11f719de-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-04-09 13:11                   ` Felix Brack
2017-04-09 13:11                     ` Felix Brack

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.