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

[Jacek Anaszewski]

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

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

[Jacek Anaszewski]

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

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

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

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

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

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

[Felix Brack]

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

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

[Felix Brack]

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

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

[Jacek Anaszewski]

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

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

[Jacek Anaszewski]

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

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

[Jacek Anaszewski]

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

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

[Jacek Anaszewski]

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

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

[Pavel Machek]

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

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

[Pavel Machek]

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

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

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

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

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

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

[Jacek Anaszewski]

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

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

[Jacek Anaszewski]

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

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

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

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

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

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

[Jacek Anaszewski]

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 
> ---
>  .../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 .
> +   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 
> @@ -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 = ;
>  };
> +kernel-booting {
> + type = ;
> + default-state = "pwm0";
> +};
> +sys-stat {
> + type = ;
> + 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(>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(>update_lock);
> + reg = i2c_smbus_read_byte_data(client, LED_REG(maxleds, led->id));
> + ret = reg >> LED_NUM(led->id)/2;
> + mutex_unlock(>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", ))
> + pdata->psc[0] = val & 0xff;

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

[Jacek Anaszewski]

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 
> ---
>  .../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 .
> +   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 
> @@ -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 = ;
>  };
> +kernel-booting {
> + type = ;
> + default-state = "pwm0";
> +};
> +sys-stat {
> + type = ;
> + 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(>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(>update_lock);
> + reg = i2c_smbus_read_byte_data(client, LED_REG(maxleds, led->id));
> + ret = reg >> LED_NUM(led->id)/2;
> + mutex_unlock(>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", ))
> + pdata->psc[0] = val & 0xff;
> + if