Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1575957 > unrolled thread

[PATCH] leds: pca9532: Extend pca9532 device tree support

Back to article view | Back to linux.kernel

Contents

  [PATCH] leds: pca9532: Extend pca9532 device tree support Felix Brack <fb@ltec.ch> - 2017-02-07 20:00 +0100
    Re: [PATCH] leds: pca9532: Extend pca9532 device tree support Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2017-02-07 22:00 +0100
      Re: [PATCH] leds: pca9532: Extend pca9532 device tree support Felix Brack <fb@ltec.ch> - 2017-02-08 18:30 +0100
        Re: [PATCH] leds: pca9532: Extend pca9532 device tree support Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2017-02-08 20:50 +0100
          Re: [PATCH] leds: pca9532: Extend pca9532 device tree support Felix Brack <fb@ltec.ch> - 2017-02-09 10:30 +0100
            Re: [PATCH] leds: pca9532: Extend pca9532 device tree support Pavel Machek <pavel@ucw.cz> - 2017-02-09 15:30 +0100
              Re: [PATCH] leds: pca9532: Extend pca9532 device tree support Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2017-02-09 22:20 +0100
              Re: [PATCH] leds: pca9532: Extend pca9532 device tree support Felix Brack <fb@ltec.ch> - 2017-02-10 17:30 +0100
            Re: [PATCH] leds: pca9532: Extend pca9532 device tree support Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2017-02-09 22:20 +0100

#1575957 — [PATCH] leds: pca9532: Extend pca9532 device tree support

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

[toc] | [next] | [standalone]

#1576060

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

[toc] | [prev] | [next] | [standalone]

#1576747

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

[toc] | [prev] | [next] | [standalone]

#1576994

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

[toc] | [prev] | [next] | [standalone]

#1577403

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

[toc] | [prev] | [next] | [standalone]

#1577643

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

[toc] | [prev] | [next] | [standalone]

#1577960

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

[toc] | [prev] | [next] | [standalone]

#1578595

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

[toc] | [prev] | [next] | [standalone]

#1577961

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web