All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] leds: pca9532: Extend pca9532 device tree support
@ 2017-02-07 18:11 Felix Brack
  2017-02-07 20:45 ` Jacek Anaszewski
  0 siblings, 1 reply; 11+ messages in thread
From: Felix Brack @ 2017-02-07 18:11 UTC (permalink / raw)
  To: riku.voipio, rpurdie, jacek.anaszewski, pavel, linux-leds, linux-kernel
  Cc: Felix Brack, f.brack

This patch extends the device tree support for the pca9532 allowing LEDs to blink, dim or even being unchanged, i.e. not being turned off during driver initialization.

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

diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
index 198f3ba..81b6563 100644
--- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
+++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
@@ -11,12 +11,24 @@ Required properties:
 		"nxp,pca9533"
 	- reg -  I2C slave address
 
+Optional properties:
+	- psc0: 8 bit prescaler value according to NXP data sheet
+	- pwm0: 8 bit PWM value according to NXP data sheet
+	- psc1: 8 bit prescaler value according to NXP data sheet
+	- pwm1: 8 bit PWM value according to NXP data sheet
+
 Each led is represented as a sub-node of the nxp,pca9530.
 
 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>.
+	  In addition to the values mentioned in the document above the additional
+	  values "pwm0" and "pwm1" are valid. The corresponding LED will blink
+	  or will be dimmed depending on the configuration of prescaler and pwm
+	  values (see optional node properties above).
 
 Example:
   #include <dt-bindings/leds/leds-pca9532.h>
@@ -24,6 +36,8 @@ Example:
   leds: pca9530@60 {
     compatible = "nxp,pca9530";
     reg = <0x60>;
+    psc0 = <0x97>; // blink frequency 1Hz
+    pwm0 = <0x80>; // 50% duty cycle (500ms On / 500ms Off)
 
     red-power {
       label = "pca:red:power";
@@ -33,6 +47,14 @@ Example:
       label = "pca:green:power";
       type = <PCA9532_TYPE_LED>;
     };
+    kernel-booting {
+    	type = <PCA9532_TYPE_LED>;
+    	default-state = "pwm0";
+    };
+    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..3353739 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,8 @@ 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;
+	u32 val;
 
 	match = of_match_device(of_pca9532_leds_match, dev);
 	if (!match)
@@ -468,6 +488,15 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
 	if (!pdata)
 		return ERR_PTR(-ENOMEM);
 
+	if (!of_property_read_u32(np, "psc0", &val))
+		pdata->psc[0] = val & 0xff;
+	if (!of_property_read_u32(np, "pwm0", &val))
+		pdata->pwm[0] = val & 0xff;
+	if (!of_property_read_u32(np, "psc1", &val))
+		pdata->psc[1] = val & 0xff;
+	if (!of_property_read_u32(np, "pwm1", &val))
+		pdata->pwm[1] = val & 0xff;
+
 	for_each_child_of_node(np, child) {
 		if (of_property_read_string(child, "label",
 					    &pdata->leds[i].name))
@@ -475,6 +504,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] 11+ messages in thread

* Re: [PATCH] leds: pca9532: Extend pca9532 device tree support
  2017-02-07 18:11 [PATCH] leds: pca9532: Extend pca9532 device tree support Felix Brack
@ 2017-02-07 20:45 ` Jacek Anaszewski
  2017-02-08 16:12   ` Felix Brack
  0 siblings, 1 reply; 11+ messages in thread
From: Jacek Anaszewski @ 2017-02-07 20:45 UTC (permalink / raw)
  To: Felix Brack, riku.voipio, rpurdie, pavel, linux-leds, linux-kernel
  Cc: f.brack

Hi Felix,

Thanks for the patch.

On 02/07/2017 07:11 PM, Felix Brack wrote:
> This patch extends the device tree support for the pca9532 allowing LEDs to blink, dim or even being unchanged, i.e. not being turned off during driver initialization.

Isn't it possible to apply desired settings with existing LED subsystem
brightness file, and delay_on/off files exposed by timer trigger?

Best regards,
Jacek Anaszewski

> Signed-off-by: Felix Brack <fb@ltec.ch>
> ---
>  .../devicetree/bindings/leds/leds-pca9532.txt      | 22 ++++++++++++
>  drivers/leds/leds-pca9532.c                        | 41 +++++++++++++++++++++-
>  include/linux/leds-pca9532.h                       |  4 +--
>  3 files changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
> index 198f3ba..81b6563 100644
> --- a/Documentation/devicetree/bindings/leds/leds-pca9532.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt
> @@ -11,12 +11,24 @@ Required properties:
>  		"nxp,pca9533"
>  	- reg -  I2C slave address
>  
> +Optional properties:
> +	- psc0: 8 bit prescaler value according to NXP data sheet
> +	- pwm0: 8 bit PWM value according to NXP data sheet
> +	- psc1: 8 bit prescaler value according to NXP data sheet
> +	- pwm1: 8 bit PWM value according to NXP data sheet
> +
>  Each led is represented as a sub-node of the nxp,pca9530.
>  
>  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>.
> +	  In addition to the values mentioned in the document above the additional
> +	  values "pwm0" and "pwm1" are valid. The corresponding LED will blink
> +	  or will be dimmed depending on the configuration of prescaler and pwm
> +	  values (see optional node properties above).
>  
>  Example:
>    #include <dt-bindings/leds/leds-pca9532.h>
> @@ -24,6 +36,8 @@ Example:
>    leds: pca9530@60 {
>      compatible = "nxp,pca9530";
>      reg = <0x60>;
> +    psc0 = <0x97>; // blink frequency 1Hz
> +    pwm0 = <0x80>; // 50% duty cycle (500ms On / 500ms Off)
>  
>      red-power {
>        label = "pca:red:power";
> @@ -33,6 +47,14 @@ Example:
>        label = "pca:green:power";
>        type = <PCA9532_TYPE_LED>;
>      };
> +    kernel-booting {
> +    	type = <PCA9532_TYPE_LED>;
> +    	default-state = "pwm0";
> +    };
> +    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..3353739 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,8 @@ 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;
> +	u32 val;
>  
>  	match = of_match_device(of_pca9532_leds_match, dev);
>  	if (!match)
> @@ -468,6 +488,15 @@ pca9532_of_populate_pdata(struct device *dev, struct device_node *np)
>  	if (!pdata)
>  		return ERR_PTR(-ENOMEM);
>  
> +	if (!of_property_read_u32(np, "psc0", &val))
> +		pdata->psc[0] = val & 0xff;
> +	if (!of_property_read_u32(np, "pwm0", &val))
> +		pdata->pwm[0] = val & 0xff;
> +	if (!of_property_read_u32(np, "psc1", &val))
> +		pdata->psc[1] = val & 0xff;
> +	if (!of_property_read_u32(np, "pwm1", &val))
> +		pdata->pwm[1] = val & 0xff;
> +
>  	for_each_child_of_node(np, child) {
>  		if (of_property_read_string(child, "label",
>  					    &pdata->leds[i].name))
> @@ -475,6 +504,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 */
> -
> 

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

* Re: [PATCH] leds: pca9532: Extend pca9532 device tree support
  2017-02-07 20:45 ` Jacek Anaszewski
@ 2017-02-08 16:12   ` Felix Brack
  2017-02-08 19:42     ` Jacek Anaszewski
  0 siblings, 1 reply; 11+ messages in thread
From: Felix Brack @ 2017-02-08 16:12 UTC (permalink / raw)
  To: Jacek Anaszewski, riku.voipio, rpurdie, pavel, linux-leds, linux-kernel
  Cc: f.brack

Hello Jacek,

On 07.02.2017 21:45, Jacek Anaszewski wrote:
> Hi Felix,
> 
> Thanks for the patch.
> 
> On 02/07/2017 07:11 PM, Felix Brack wrote:
>> This patch extends the device tree support for the pca9532 allowing LEDs to blink, dim or even being unchanged, i.e. not being turned off during driver initialization.
> 
> Isn't it possible to apply desired settings with existing LED subsystem
> brightness file, and delay_on/off files exposed by timer trigger?
> 
> Best regards,
> Jacek Anaszewski
> 

This might be a misunderstanding. My patch is not meant to replace
anything for driving the LEDs once the kernel is fully loaded. The LED
subsystem offers quite a lot of possibilities to do this.

My patch mainly deals with the 'default' state of the LEDs immediately
when the driver gets loaded.
Here is an example: I have a system with a LED named 'RUN' which is
turned on steady by U-Boot (indicating "system booting"). When the
PCA9532 driver loads this LED gets turned off due to initialization.
However I would like it remain lit until later a script will make that
'RUN' LED blink (indicating "system running"). This script will of
course use the existing LED subsystem to do so. To keep the 'RUN' LED
lit I need the DT property 'default-state' being set to 'PCA9532_KEEP'.

regards, Felix

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

* Re: [PATCH] leds: pca9532: Extend pca9532 device tree support
  2017-02-08 16:12   ` Felix Brack
@ 2017-02-08 19:42     ` Jacek Anaszewski
  2017-02-09  8:41       ` Felix Brack
  2017-03-29 14:26       ` Felix Brack
  0 siblings, 2 replies; 11+ messages in thread
From: Jacek Anaszewski @ 2017-02-08 19:42 UTC (permalink / raw)
  To: Felix Brack, riku.voipio, rpurdie, pavel, linux-leds, linux-kernel
  Cc: f.brack

Hi Felix,

On 02/08/2017 05:12 PM, Felix Brack wrote:
> Hello Jacek,
> 
> On 07.02.2017 21:45, Jacek Anaszewski wrote:
>> Hi Felix,
>>
>> Thanks for the patch.
>>
>> On 02/07/2017 07:11 PM, Felix Brack wrote:
>>> This patch extends the device tree support for the pca9532 allowing LEDs to blink, dim or even being unchanged, i.e. not being turned off during driver initialization.
>>
>> Isn't it possible to apply desired settings with existing LED subsystem
>> brightness file, and delay_on/off files exposed by timer trigger?
>>
>> Best regards,
>> Jacek Anaszewski
>>
> 
> This might be a misunderstanding. My patch is not meant to replace
> anything for driving the LEDs once the kernel is fully loaded. The LED
> subsystem offers quite a lot of possibilities to do this.
> 
> My patch mainly deals with the 'default' state of the LEDs immediately
> when the driver gets loaded.
> Here is an example: I have a system with a LED named 'RUN' which is
> turned on steady by U-Boot (indicating "system booting"). When the
> PCA9532 driver loads this LED gets turned off due to initialization.
> However I would like it remain lit until later a script will make that
> 'RUN' LED blink (indicating "system running"). This script will of
> course use the existing LED subsystem to do so. To keep the 'RUN' LED
> lit I need the DT property 'default-state' being set to 'PCA9532_KEEP'.

It looks like all you need is default-state property.
I'd rather avoid exposing prescaler and pwm registers in DT.

-- 
Best regards,
Jacek Anaszewski

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

* Re: [PATCH] leds: pca9532: Extend pca9532 device tree support
  2017-02-08 19:42     ` Jacek Anaszewski
@ 2017-02-09  8:41       ` Felix Brack
  2017-02-09 14:24         ` Pavel Machek
  2017-02-09 21:10         ` Jacek Anaszewski
  2017-03-29 14:26       ` Felix Brack
  1 sibling, 2 replies; 11+ messages in thread
From: Felix Brack @ 2017-02-09  8:41 UTC (permalink / raw)
  To: Jacek Anaszewski, riku.voipio, rpurdie, pavel, linux-leds, linux-kernel
  Cc: f.brack

Hello Jacek,

On 08.02.2017 20:42, Jacek Anaszewski wrote:
> Hi Felix,
> 
> On 02/08/2017 05:12 PM, Felix Brack wrote:
>> Hello Jacek,
>>
>> On 07.02.2017 21:45, Jacek Anaszewski wrote:
>>> Hi Felix,
>>>
>>> Thanks for the patch.
>>>
>>> On 02/07/2017 07:11 PM, Felix Brack wrote:
>>>> This patch extends the device tree support for the pca9532 allowing LEDs to blink, dim or even being unchanged, i.e. not being turned off during driver initialization.
>>>
>>> Isn't it possible to apply desired settings with existing LED subsystem
>>> brightness file, and delay_on/off files exposed by timer trigger?
>>>
>>> Best regards,
>>> Jacek Anaszewski
>>>
>>
>> This might be a misunderstanding. My patch is not meant to replace
>> anything for driving the LEDs once the kernel is fully loaded. The LED
>> subsystem offers quite a lot of possibilities to do this.
>>
>> My patch mainly deals with the 'default' state of the LEDs immediately
>> when the driver gets loaded.
>> Here is an example: I have a system with a LED named 'RUN' which is
>> turned on steady by U-Boot (indicating "system booting"). When the
>> PCA9532 driver loads this LED gets turned off due to initialization.
>> However I would like it remain lit until later a script will make that
>> 'RUN' LED blink (indicating "system running"). This script will of
>> course use the existing LED subsystem to do so. To keep the 'RUN' LED
>> lit I need the DT property 'default-state' being set to 'PCA9532_KEEP'.
> 
> It looks like all you need is default-state property.

For the example with keeping the 'RUN' led turned on, yes. However I
would have to configure PSC and PWM registers to make the 'RUN' LED
blink, for example.

> I'd rather avoid exposing prescaler and pwm registers in DT.

I don't see that exposing PSC and PWM registers to the DT would do any
harm. Is there something I'm missing here?

> 

One could pass parameters to the driver but I think that is worse, as
nowadays we have DT.

regards Felix

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

* Re: [PATCH] leds: pca9532: Extend pca9532 device tree support
  2017-02-09  8:41       ` Felix Brack
@ 2017-02-09 14:24         ` Pavel Machek
  2017-02-09 21:04           ` Jacek Anaszewski
  2017-02-10 16:26           ` Felix Brack
  2017-02-09 21:10         ` Jacek Anaszewski
  1 sibling, 2 replies; 11+ messages in thread
From: Pavel Machek @ 2017-02-09 14:24 UTC (permalink / raw)
  To: Felix Brack
  Cc: Jacek Anaszewski, riku.voipio, rpurdie, linux-leds, linux-kernel,
	f.brack

Hi!

> >> This might be a misunderstanding. My patch is not meant to replace
> >> anything for driving the LEDs once the kernel is fully loaded. The LED
> >> subsystem offers quite a lot of possibilities to do this.
> >>
> >> My patch mainly deals with the 'default' state of the LEDs immediately
> >> when the driver gets loaded.
> >> Here is an example: I have a system with a LED named 'RUN' which is
> >> turned on steady by U-Boot (indicating "system booting"). When the
> >> PCA9532 driver loads this LED gets turned off due to initialization.
> >> However I would like it remain lit until later a script will make that
> >> 'RUN' LED blink (indicating "system running"). This script will of
> >> course use the existing LED subsystem to do so. To keep the 'RUN' LED
> >> lit I need the DT property 'default-state' being set to 'PCA9532_KEEP'.
> > 
> > It looks like all you need is default-state property.
> 
> For the example with keeping the 'RUN' led turned on, yes. However I
> would have to configure PSC and PWM registers to make the 'RUN' LED
> blink, for example.

Is that really useful? Keeping the state from u-boot until userland can take
control... ok, why not.

If it is useful, right, you can do it. But it will have to be generic for all
the LEDs, not like this.

You'll want something like

default_trigger = blinking;
default_trigger_parameters = < 2 sec on, 1 sec off >;

or something. You may want to coordinate with the USB LED people, which want
default triggers with parametrs.

									Pavel


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

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

* Re: [PATCH] leds: pca9532: Extend pca9532 device tree support
  2017-02-09 14:24         ` Pavel Machek
@ 2017-02-09 21:04           ` Jacek Anaszewski
  2017-02-10 16:26           ` Felix Brack
  1 sibling, 0 replies; 11+ messages in thread
From: Jacek Anaszewski @ 2017-02-09 21:04 UTC (permalink / raw)
  To: Pavel Machek, Felix Brack
  Cc: riku.voipio, rpurdie, linux-leds, linux-kernel, f.brack

On 02/09/2017 03:24 PM, Pavel Machek wrote:
> Hi!
> 
>>>> This might be a misunderstanding. My patch is not meant to replace
>>>> anything for driving the LEDs once the kernel is fully loaded. The LED
>>>> subsystem offers quite a lot of possibilities to do this.
>>>>
>>>> My patch mainly deals with the 'default' state of the LEDs immediately
>>>> when the driver gets loaded.
>>>> Here is an example: I have a system with a LED named 'RUN' which is
>>>> turned on steady by U-Boot (indicating "system booting"). When the
>>>> PCA9532 driver loads this LED gets turned off due to initialization.
>>>> However I would like it remain lit until later a script will make that
>>>> 'RUN' LED blink (indicating "system running"). This script will of
>>>> course use the existing LED subsystem to do so. To keep the 'RUN' LED
>>>> lit I need the DT property 'default-state' being set to 'PCA9532_KEEP'.
>>>
>>> It looks like all you need is default-state property.
>>
>> For the example with keeping the 'RUN' led turned on, yes. However I
>> would have to configure PSC and PWM registers to make the 'RUN' LED
>> blink, for example.
> 
> Is that really useful? Keeping the state from u-boot until userland can take
> control... ok, why not.
> 
> If it is useful, right, you can do it. But it will have to be generic for all
> the LEDs, not like this.
> 
> You'll want something like
> 
> default_trigger = blinking;

Actually it can be achieved currently by setting "timer" here.

> default_trigger_parameters = < 2 sec on, 1 sec off >;
> 
> or something. You may want to coordinate with the USB LED people, which want
> default triggers with parametrs.

-- 
Best regards,
Jacek Anaszewski

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

* Re: [PATCH] leds: pca9532: Extend pca9532 device tree support
  2017-02-09  8:41       ` Felix Brack
  2017-02-09 14:24         ` Pavel Machek
@ 2017-02-09 21:10         ` Jacek Anaszewski
  1 sibling, 0 replies; 11+ messages in thread
From: Jacek Anaszewski @ 2017-02-09 21:10 UTC (permalink / raw)
  To: Felix Brack, riku.voipio, rpurdie, pavel, linux-leds, linux-kernel
  Cc: f.brack

Hi Felix,

On 02/09/2017 09:41 AM, Felix Brack wrote:
> Hello Jacek,
> 
> On 08.02.2017 20:42, Jacek Anaszewski wrote:
>> Hi Felix,
>>
>> On 02/08/2017 05:12 PM, Felix Brack wrote:
>>> Hello Jacek,
>>>
>>> On 07.02.2017 21:45, Jacek Anaszewski wrote:
>>>> Hi Felix,
>>>>
>>>> Thanks for the patch.
>>>>
>>>> On 02/07/2017 07:11 PM, Felix Brack wrote:
>>>>> This patch extends the device tree support for the pca9532 allowing LEDs to blink, dim or even being unchanged, i.e. not being turned off during driver initialization.
>>>>
>>>> Isn't it possible to apply desired settings with existing LED subsystem
>>>> brightness file, and delay_on/off files exposed by timer trigger?
>>>>
>>>> Best regards,
>>>> Jacek Anaszewski
>>>>
>>>
>>> This might be a misunderstanding. My patch is not meant to replace
>>> anything for driving the LEDs once the kernel is fully loaded. The LED
>>> subsystem offers quite a lot of possibilities to do this.
>>>
>>> My patch mainly deals with the 'default' state of the LEDs immediately
>>> when the driver gets loaded.
>>> Here is an example: I have a system with a LED named 'RUN' which is
>>> turned on steady by U-Boot (indicating "system booting"). When the
>>> PCA9532 driver loads this LED gets turned off due to initialization.
>>> However I would like it remain lit until later a script will make that
>>> 'RUN' LED blink (indicating "system running"). This script will of
>>> course use the existing LED subsystem to do so. To keep the 'RUN' LED
>>> lit I need the DT property 'default-state' being set to 'PCA9532_KEEP'.
>>
>> It looks like all you need is default-state property.
> 
> For the example with keeping the 'RUN' led turned on, yes. However I
> would have to configure PSC and PWM registers to make the 'RUN' LED
> blink, for example.
> 
>> I'd rather avoid exposing prescaler and pwm registers in DT.
> 
> I don't see that exposing PSC and PWM registers to the DT would do any
> harm. Is there something I'm missing here?

It is driver's responsibility to configure registers basing on user
settings. Let's stick to this scheme.

-- 
Best regards,
Jacek Anaszewski

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

* Re: [PATCH] leds: pca9532: Extend pca9532 device tree support
  2017-02-09 14:24         ` Pavel Machek
  2017-02-09 21:04           ` Jacek Anaszewski
@ 2017-02-10 16:26           ` Felix Brack
  1 sibling, 0 replies; 11+ messages in thread
From: Felix Brack @ 2017-02-10 16:26 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Jacek Anaszewski, riku.voipio, rpurdie, linux-leds, linux-kernel

Hello Pavel, Hello Jacek

On 09.02.2017 15:24, Pavel Machek wrote:
> Hi!
> 
>>>> This might be a misunderstanding. My patch is not meant to replace
>>>> anything for driving the LEDs once the kernel is fully loaded. The LED
>>>> subsystem offers quite a lot of possibilities to do this.
>>>>
>>>> My patch mainly deals with the 'default' state of the LEDs immediately
>>>> when the driver gets loaded.
>>>> Here is an example: I have a system with a LED named 'RUN' which is
>>>> turned on steady by U-Boot (indicating "system booting"). When the
>>>> PCA9532 driver loads this LED gets turned off due to initialization.
>>>> However I would like it remain lit until later a script will make that
>>>> 'RUN' LED blink (indicating "system running"). This script will of
>>>> course use the existing LED subsystem to do so. To keep the 'RUN' LED
>>>> lit I need the DT property 'default-state' being set to 'PCA9532_KEEP'.
>>>
>>> It looks like all you need is default-state property.
>>
>> For the example with keeping the 'RUN' led turned on, yes. However I
>> would have to configure PSC and PWM registers to make the 'RUN' LED
>> blink, for example.
> 
> Is that really useful? Keeping the state from u-boot until userland can take
> control... ok, why not.
> 
> If it is useful, right, you can do it. But it will have to be generic for all
> the LEDs, not like this.
> 
> You'll want something like
> 
> default_trigger = blinking;
> default_trigger_parameters = < 2 sec on, 1 sec off >;
> 
> or something. You may want to coordinate with the USB LED people, which want
> default triggers with parametrs.
> 
>
Okay, I absolutely agree with you two. The driver should hide hardware
specific details (such as chip registers) as much as possible. My
approach does the opposite!
I will submit a new version of the patch that should conform much better
to existing DT bindings for LEDs.

many thanks, Felix

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

* Re: [PATCH] leds: pca9532: Extend pca9532 device tree support
  2017-02-08 19:42     ` Jacek Anaszewski
  2017-02-09  8:41       ` Felix Brack
@ 2017-03-29 14:26       ` Felix Brack
  2017-03-29 18:29         ` Jacek Anaszewski
  1 sibling, 1 reply; 11+ messages in thread
From: Felix Brack @ 2017-03-29 14:26 UTC (permalink / raw)
  To: Jacek Anaszewski, riku.voipio, rpurdie, pavel, linux-leds, linux-kernel
  Cc: f.brack

Hello Jacek,

On 08.02.2017 20:42, Jacek Anaszewski wrote:
> Hi Felix,
> 
> On 02/08/2017 05:12 PM, Felix Brack wrote:
>> Hello Jacek,
>>
>> On 07.02.2017 21:45, Jacek Anaszewski wrote:
>>> Hi Felix,
>>>
>>> Thanks for the patch.
>>>
>>> On 02/07/2017 07:11 PM, Felix Brack wrote:
>>>> This patch extends the device tree support for the pca9532 allowing LEDs to blink, dim or even being unchanged, i.e. not being turned off during driver initialization.
>>>
>>> Isn't it possible to apply desired settings with existing LED subsystem
>>> brightness file, and delay_on/off files exposed by timer trigger?
>>>
>>> Best regards,
>>> Jacek Anaszewski
>>>
>>
>> This might be a misunderstanding. My patch is not meant to replace
>> anything for driving the LEDs once the kernel is fully loaded. The LED
>> subsystem offers quite a lot of possibilities to do this.
>>
>> My patch mainly deals with the 'default' state of the LEDs immediately
>> when the driver gets loaded.
>> Here is an example: I have a system with a LED named 'RUN' which is
>> turned on steady by U-Boot (indicating "system booting"). When the
>> PCA9532 driver loads this LED gets turned off due to initialization.
>> However I would like it remain lit until later a script will make that
>> 'RUN' LED blink (indicating "system running"). This script will of
>> course use the existing LED subsystem to do so. To keep the 'RUN' LED
>> lit I need the DT property 'default-state' being set to 'PCA9532_KEEP'.
> 
> It looks like all you need is default-state property.
> I'd rather avoid exposing prescaler and pwm registers in DT.
> 

For the time being there seems to be no generic timer configuration by
means of the DT. I have therefore decided to remove the code dealing
with prescaler and pwm from my patch. This will prevent those registers
from being exposed to the DT. What will remain is the default-state.

Should I send an updated version (v2) of this patch or would it be
better to send a 'new' patch?

regards, Felix

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

* Re: [PATCH] leds: pca9532: Extend pca9532 device tree support
  2017-03-29 14:26       ` Felix Brack
@ 2017-03-29 18:29         ` Jacek Anaszewski
  0 siblings, 0 replies; 11+ messages in thread
From: Jacek Anaszewski @ 2017-03-29 18:29 UTC (permalink / raw)
  To: Felix Brack, riku.voipio, rpurdie, pavel, linux-leds, linux-kernel
  Cc: f.brack

Hi Felix,

On 03/29/2017 04:26 PM, Felix Brack wrote:
> Hello Jacek,
> 
> On 08.02.2017 20:42, Jacek Anaszewski wrote:
>> Hi Felix,
>>
>> On 02/08/2017 05:12 PM, Felix Brack wrote:
>>> Hello Jacek,
>>>
>>> On 07.02.2017 21:45, Jacek Anaszewski wrote:
>>>> Hi Felix,
>>>>
>>>> Thanks for the patch.
>>>>
>>>> On 02/07/2017 07:11 PM, Felix Brack wrote:
>>>>> This patch extends the device tree support for the pca9532 allowing LEDs to blink, dim or even being unchanged, i.e. not being turned off during driver initialization.
>>>>
>>>> Isn't it possible to apply desired settings with existing LED subsystem
>>>> brightness file, and delay_on/off files exposed by timer trigger?
>>>>
>>>> Best regards,
>>>> Jacek Anaszewski
>>>>
>>>
>>> This might be a misunderstanding. My patch is not meant to replace
>>> anything for driving the LEDs once the kernel is fully loaded. The LED
>>> subsystem offers quite a lot of possibilities to do this.
>>>
>>> My patch mainly deals with the 'default' state of the LEDs immediately
>>> when the driver gets loaded.
>>> Here is an example: I have a system with a LED named 'RUN' which is
>>> turned on steady by U-Boot (indicating "system booting"). When the
>>> PCA9532 driver loads this LED gets turned off due to initialization.
>>> However I would like it remain lit until later a script will make that
>>> 'RUN' LED blink (indicating "system running"). This script will of
>>> course use the existing LED subsystem to do so. To keep the 'RUN' LED
>>> lit I need the DT property 'default-state' being set to 'PCA9532_KEEP'.
>>
>> It looks like all you need is default-state property.
>> I'd rather avoid exposing prescaler and pwm registers in DT.
>>
> 
> For the time being there seems to be no generic timer configuration by
> means of the DT. I have therefore decided to remove the code dealing
> with prescaler and pwm from my patch. This will prevent those registers
> from being exposed to the DT. What will remain is the default-state.
> 
> Should I send an updated version (v2) of this patch or would it be
> better to send a 'new' patch?

Please use current patch title and add v2 to the [PATCH] tag.

-- 
Best regards,
Jacek Anaszewski

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

end of thread, other threads:[~2017-03-29 18:30 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-07 18:11 [PATCH] leds: pca9532: Extend pca9532 device tree support Felix Brack
2017-02-07 20:45 ` Jacek Anaszewski
2017-02-08 16:12   ` Felix Brack
2017-02-08 19:42     ` Jacek Anaszewski
2017-02-09  8:41       ` Felix Brack
2017-02-09 14:24         ` Pavel Machek
2017-02-09 21:04           ` Jacek Anaszewski
2017-02-10 16:26           ` Felix Brack
2017-02-09 21:10         ` Jacek Anaszewski
2017-03-29 14:26       ` Felix Brack
2017-03-29 18:29         ` Jacek Anaszewski

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.