Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hey Ricardo,

On 11-05-16 23:38, Ricardo Ribalda Delgado wrote:

Some update

I have not received anything to test it and I will be out of the
office from today until the 30th :S. So it seems that I have not way
to test the changes.
That's understandable, meanwhile I've been running some experiments and 
mostly reading the datasheet more closely. You hadn't replied to my 
earlier mention of it so I'll only talk about that now.


Right now, the GDC (e.g. GRPPWM) register is set via some math using the 
period:

/*
 * From manual: duty cycle = (GDC / 256) ->
 *(time_on / period) = (GDC / 256) ->
 *GDC = ((time_on * 256) / period)
 */
gdc = (time_on * 256) / period;

This is wrong in my opinion, as the manual states:

When DMBLNK bit is programmed with 1, GRPPWM and GRPFREQ registers define a
global blinking pattern, where GRPFREQ contains the blinking period 
(from 24 Hz to

10.73 s) and GRPPWM the duty cycle (ON/OFF ratio in %).

First, when DMBLNK is programmed with 0, GRPFREQ is a don't care, so no 
'blinking' takes place. Which means, since we want blink_set to work, we 
need DMBLNK and that can be enabled by default, as the LDR register 
controls whether we can blink at all (11b vs 10b)


Then the GRPFREQ register, as done now, sets a frequency for the blink 
between 41ms and 10.73 seconds.


Finally, the GRPPWM/gdc register can be used to globally control the 
brightness of leds which are linked (via LDR=11b). So if we have a color 
setup, e.g. #4488FF and we globally dim this color via gdc to 10%, we 
get #04080F on the output. To add this behavior we'd be adding some more 
hacks as the led framework doesn't really support linked outputs at all, 
(nice new feature, to link leds together with a global control?, other 
subsystems do something like that, cpufreq i guess) who decides the 
value of gdc. There is no toggle to set the global brightness. What 
could be possible, is to leave whatever setting is in the brightness and 
read/report the individual led brightness as it where the gdc. Which 
could actually work thinking about it. But (and I think it is impossible 
now anyway) it would break individual color control during the blink. 
e.g. /sys/class/led/[red,green,blue]/brightness are always equal and 
writing to either during blink sets the global brightness.


But that should be a separate patch that I'm thinking a bit more about.

So for now, I recommend to set GRPPWM initially to 0xff, e.g. leds full 
on, until the above patch is added. We then blink at whatever color is 
set to the output and only control the blink via the GRPFREQ, without 
the possibility to change brightness during a blink.


Olliver

Sorry about that.

Regards!

On Fri, Apr 22, 2016 at 10:48 AM, Ricardo Ribalda Delgado
  wrote:

Hi

I  am on trip until next monday. I have arranged also some hw to be sent to
me that day.

Can we continue the conversation then? I know I told you that I will review
this yesterday, but I did not have the time , sorry

Regards!

On 22 Apr 2016 09:21, "Olliver Schinagl"  wrote:

Hi Ricardo,


On 20-04-16 11:17, Ricardo Ribalda Delgado wrote:

Hello again

On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl
wrote:


The devil is in the details :)

:)

Saving mode2 sounds like a good compromise then.

But I still believe that we should limit the lock to ledout. No matter
what we do, we cannot have two leds blinking at different frequencies
on the same chip.

So to save a mutex a little bit, we take the risk that nobody else
enables
the blink or if they do, enable it in the same way?
If it saves so much, then I guess its worth the risk I suppose?

Give me a day to go through the chip doc and see if I can find a good
compromise, that at least warranties that the leds that are enable
stay enabled ;)

Right, I also went over the datasheet, and I think we can simplyfy two
things.

For one, yes, move the mode2 register completly to the probe section. Set
the DMBLINK led to always 1. It does not get cleared, I was wrong. We have
to set it to as with 0 we do not get any blinking at all (grpfreq gets
ignored).

Furthermore, we should change:
  -gdc = (time_on * 256) / period;
+   gdc = 0x00;

Because the calculation does not make sense. GDC is the global
brightness/pwm/dimming control. It is used to uniformly change the blink
rate on all the linked leds.

"General brightness for the 16 outputs is controlled through 256 linear
steps to FFh"
I don't think that is the intention of the gdc is it? Looking at the
original gdc code, it thus sets the global BRIGHTNESS based on the
period/on_time. I don't think that is what we expect when we enable blink.

 From my understanding, the grppwm is super-imposed, thus by setting gdc to
0, we do not superimpose anything and the original brightness is retained.
(If i'm wrong here, we need to set gdc to 0xff.

Because of this, I even recommend removing gdc all 

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hey Ricardo,

On 11-05-16 23:38, Ricardo Ribalda Delgado wrote:

Some update

I have not received anything to test it and I will be out of the
office from today until the 30th :S. So it seems that I have not way
to test the changes.
That's understandable, meanwhile I've been running some experiments and 
mostly reading the datasheet more closely. You hadn't replied to my 
earlier mention of it so I'll only talk about that now.


Right now, the GDC (e.g. GRPPWM) register is set via some math using the 
period:

/*
 * From manual: duty cycle = (GDC / 256) ->
 *(time_on / period) = (GDC / 256) ->
 *GDC = ((time_on * 256) / period)
 */
gdc = (time_on * 256) / period;

This is wrong in my opinion, as the manual states:

When DMBLNK bit is programmed with 1, GRPPWM and GRPFREQ registers define a
global blinking pattern, where GRPFREQ contains the blinking period 
(from 24 Hz to

10.73 s) and GRPPWM the duty cycle (ON/OFF ratio in %).

First, when DMBLNK is programmed with 0, GRPFREQ is a don't care, so no 
'blinking' takes place. Which means, since we want blink_set to work, we 
need DMBLNK and that can be enabled by default, as the LDR register 
controls whether we can blink at all (11b vs 10b)


Then the GRPFREQ register, as done now, sets a frequency for the blink 
between 41ms and 10.73 seconds.


Finally, the GRPPWM/gdc register can be used to globally control the 
brightness of leds which are linked (via LDR=11b). So if we have a color 
setup, e.g. #4488FF and we globally dim this color via gdc to 10%, we 
get #04080F on the output. To add this behavior we'd be adding some more 
hacks as the led framework doesn't really support linked outputs at all, 
(nice new feature, to link leds together with a global control?, other 
subsystems do something like that, cpufreq i guess) who decides the 
value of gdc. There is no toggle to set the global brightness. What 
could be possible, is to leave whatever setting is in the brightness and 
read/report the individual led brightness as it where the gdc. Which 
could actually work thinking about it. But (and I think it is impossible 
now anyway) it would break individual color control during the blink. 
e.g. /sys/class/led/[red,green,blue]/brightness are always equal and 
writing to either during blink sets the global brightness.


But that should be a separate patch that I'm thinking a bit more about.

So for now, I recommend to set GRPPWM initially to 0xff, e.g. leds full 
on, until the above patch is added. We then blink at whatever color is 
set to the output and only control the blink via the GRPFREQ, without 
the possibility to change brightness during a blink.


Olliver

Sorry about that.

Regards!

On Fri, Apr 22, 2016 at 10:48 AM, Ricardo Ribalda Delgado
  wrote:

Hi

I  am on trip until next monday. I have arranged also some hw to be sent to
me that day.

Can we continue the conversation then? I know I told you that I will review
this yesterday, but I did not have the time , sorry

Regards!

On 22 Apr 2016 09:21, "Olliver Schinagl"  wrote:

Hi Ricardo,


On 20-04-16 11:17, Ricardo Ribalda Delgado wrote:

Hello again

On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl
wrote:


The devil is in the details :)

:)

Saving mode2 sounds like a good compromise then.

But I still believe that we should limit the lock to ledout. No matter
what we do, we cannot have two leds blinking at different frequencies
on the same chip.

So to save a mutex a little bit, we take the risk that nobody else
enables
the blink or if they do, enable it in the same way?
If it saves so much, then I guess its worth the risk I suppose?

Give me a day to go through the chip doc and see if I can find a good
compromise, that at least warranties that the leds that are enable
stay enabled ;)

Right, I also went over the datasheet, and I think we can simplyfy two
things.

For one, yes, move the mode2 register completly to the probe section. Set
the DMBLINK led to always 1. It does not get cleared, I was wrong. We have
to set it to as with 0 we do not get any blinking at all (grpfreq gets
ignored).

Furthermore, we should change:
  -gdc = (time_on * 256) / period;
+   gdc = 0x00;

Because the calculation does not make sense. GDC is the global
brightness/pwm/dimming control. It is used to uniformly change the blink
rate on all the linked leds.

"General brightness for the 16 outputs is controlled through 256 linear
steps to FFh"
I don't think that is the intention of the gdc is it? Looking at the
original gdc code, it thus sets the global BRIGHTNESS based on the
period/on_time. I don't think that is what we expect when we enable blink.

 From my understanding, the grppwm is super-imposed, thus by setting gdc to
0, we do not superimpose anything and the original brightness is retained.
(If i'm wrong here, we need to set gdc to 0xff.

Because of this, I even recommend removing gdc all together, which saves
another i2c write.

Or am I wrong here?

Olliver

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hey Ricardo,

On 11-05-16 23:38, Ricardo Ribalda Delgado wrote:

Some update

I have not received anything to test it and I will be out of the
office from today until the 30th :S. So it seems that I have not way
to test the changes.
That's understandable, meanwhile I've been running some experiments and 
mostly reading the datasheet more closely. You hadn't replied to my 
earlier mention of it so I'll only talk about that now.


Right now, the GDC (e.g. GRPPWM) register is set via some math using the 
period:

/*
 * From manual: duty cycle = (GDC / 256) ->
 *(time_on / period) = (GDC / 256) ->
 *GDC = ((time_on * 256) / period)
 */
gdc = (time_on * 256) / period;

This is wrong in my opinion, as the manual states:

When DMBLNK bit is programmed with 1, GRPPWM and GRPFREQ registers define a
global blinking pattern, where GRPFREQ contains the blinking period 
(from 24 Hz to

10.73 s) and GRPPWM the duty cycle (ON/OFF ratio in %).

First, when DMBLNK is programmed with 0, GRPFREQ is a don't care, so no 
'blinking' takes place. Which means, since we want blink_set to work, we 
need DMBLNK and that can be enabled by default, as the LDR register 
controls whether we can blink at all (11b vs 10b)


Then the GRPFREQ register, as done now, sets a frequency for the blink 
between 41ms and 10.73 seconds.


Finally, the GRPPWM/gdc register can be used to globally control the 
brightness of leds which are linked (via LDR=11b). So if we have a color 
setup, e.g. #4488FF and we globally dim this color via gdc to 10%, we 
get #04080F on the output. To add this behavior we'd be adding some more 
hacks as the led framework doesn't really support linked outputs at all, 
(nice new feature, to link leds together with a global control?, other 
subsystems do something like that, cpufreq i guess) who decides the 
value of gdc. There is no toggle to set the global brightness. What 
could be possible, is to leave whatever setting is in the brightness and 
read/report the individual led brightness as it where the gdc. Which 
could actually work thinking about it. But (and I think it is impossible 
now anyway) it would break individual color control during the blink. 
e.g. /sys/class/led/[red,green,blue]/brightness are always equal and 
writing to either during blink sets the global brightness.


But that should be a separate patch that I'm thinking a bit more about.

So for now, I recommend to set GRPPWM initially to 0xff, e.g. leds full 
on, until the above patch is added. We then blink at whatever color is 
set to the output and only control the blink via the GRPFREQ, without 
the possibility to change brightness during a blink.


Olliver

Sorry about that.

Regards!

On Fri, Apr 22, 2016 at 10:48 AM, Ricardo Ribalda Delgado
 wrote:

Hi

I  am on trip until next monday. I have arranged also some hw to be sent to
me that day.

Can we continue the conversation then? I know I told you that I will review
this yesterday, but I did not have the time , sorry

Regards!

On 22 Apr 2016 09:21, "Olliver Schinagl"  wrote:

Hi Ricardo,


On 20-04-16 11:17, Ricardo Ribalda Delgado wrote:

Hello again

On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl 
wrote:


The devil is in the details :)

:)

Saving mode2 sounds like a good compromise then.

But I still believe that we should limit the lock to ledout. No matter
what we do, we cannot have two leds blinking at different frequencies
on the same chip.

So to save a mutex a little bit, we take the risk that nobody else
enables
the blink or if they do, enable it in the same way?
If it saves so much, then I guess its worth the risk I suppose?

Give me a day to go through the chip doc and see if I can find a good
compromise, that at least warranties that the leds that are enable
stay enabled ;)

Right, I also went over the datasheet, and I think we can simplyfy two
things.

For one, yes, move the mode2 register completly to the probe section. Set
the DMBLINK led to always 1. It does not get cleared, I was wrong. We have
to set it to as with 0 we do not get any blinking at all (grpfreq gets
ignored).

Furthermore, we should change:
  -gdc = (time_on * 256) / period;
+   gdc = 0x00;

Because the calculation does not make sense. GDC is the global
brightness/pwm/dimming control. It is used to uniformly change the blink
rate on all the linked leds.

"General brightness for the 16 outputs is controlled through 256 linear
steps to FFh"
I don't think that is the intention of the gdc is it? Looking at the
original gdc code, it thus sets the global BRIGHTNESS based on the
period/on_time. I don't think that is what we expect when we enable blink.

 From my understanding, the grppwm is super-imposed, thus by setting gdc to
0, we do not superimpose anything and the original brightness is retained.
(If i'm wrong here, we need to set gdc to 0xff.

Because of this, I even recommend removing gdc all 

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hey Ricardo,

On 11-05-16 23:38, Ricardo Ribalda Delgado wrote:

Some update

I have not received anything to test it and I will be out of the
office from today until the 30th :S. So it seems that I have not way
to test the changes.
That's understandable, meanwhile I've been running some experiments and 
mostly reading the datasheet more closely. You hadn't replied to my 
earlier mention of it so I'll only talk about that now.


Right now, the GDC (e.g. GRPPWM) register is set via some math using the 
period:

/*
 * From manual: duty cycle = (GDC / 256) ->
 *(time_on / period) = (GDC / 256) ->
 *GDC = ((time_on * 256) / period)
 */
gdc = (time_on * 256) / period;

This is wrong in my opinion, as the manual states:

When DMBLNK bit is programmed with 1, GRPPWM and GRPFREQ registers define a
global blinking pattern, where GRPFREQ contains the blinking period 
(from 24 Hz to

10.73 s) and GRPPWM the duty cycle (ON/OFF ratio in %).

First, when DMBLNK is programmed with 0, GRPFREQ is a don't care, so no 
'blinking' takes place. Which means, since we want blink_set to work, we 
need DMBLNK and that can be enabled by default, as the LDR register 
controls whether we can blink at all (11b vs 10b)


Then the GRPFREQ register, as done now, sets a frequency for the blink 
between 41ms and 10.73 seconds.


Finally, the GRPPWM/gdc register can be used to globally control the 
brightness of leds which are linked (via LDR=11b). So if we have a color 
setup, e.g. #4488FF and we globally dim this color via gdc to 10%, we 
get #04080F on the output. To add this behavior we'd be adding some more 
hacks as the led framework doesn't really support linked outputs at all, 
(nice new feature, to link leds together with a global control?, other 
subsystems do something like that, cpufreq i guess) who decides the 
value of gdc. There is no toggle to set the global brightness. What 
could be possible, is to leave whatever setting is in the brightness and 
read/report the individual led brightness as it where the gdc. Which 
could actually work thinking about it. But (and I think it is impossible 
now anyway) it would break individual color control during the blink. 
e.g. /sys/class/led/[red,green,blue]/brightness are always equal and 
writing to either during blink sets the global brightness.


But that should be a separate patch that I'm thinking a bit more about.

So for now, I recommend to set GRPPWM initially to 0xff, e.g. leds full 
on, until the above patch is added. We then blink at whatever color is 
set to the output and only control the blink via the GRPFREQ, without 
the possibility to change brightness during a blink.


Olliver

Sorry about that.

Regards!

On Fri, Apr 22, 2016 at 10:48 AM, Ricardo Ribalda Delgado
 wrote:

Hi

I  am on trip until next monday. I have arranged also some hw to be sent to
me that day.

Can we continue the conversation then? I know I told you that I will review
this yesterday, but I did not have the time , sorry

Regards!

On 22 Apr 2016 09:21, "Olliver Schinagl"  wrote:

Hi Ricardo,


On 20-04-16 11:17, Ricardo Ribalda Delgado wrote:

Hello again

On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl 
wrote:


The devil is in the details :)

:)

Saving mode2 sounds like a good compromise then.

But I still believe that we should limit the lock to ledout. No matter
what we do, we cannot have two leds blinking at different frequencies
on the same chip.

So to save a mutex a little bit, we take the risk that nobody else
enables
the blink or if they do, enable it in the same way?
If it saves so much, then I guess its worth the risk I suppose?

Give me a day to go through the chip doc and see if I can find a good
compromise, that at least warranties that the leds that are enable
stay enabled ;)

Right, I also went over the datasheet, and I think we can simplyfy two
things.

For one, yes, move the mode2 register completly to the probe section. Set
the DMBLINK led to always 1. It does not get cleared, I was wrong. We have
to set it to as with 0 we do not get any blinking at all (grpfreq gets
ignored).

Furthermore, we should change:
  -gdc = (time_on * 256) / period;
+   gdc = 0x00;

Because the calculation does not make sense. GDC is the global
brightness/pwm/dimming control. It is used to uniformly change the blink
rate on all the linked leds.

"General brightness for the 16 outputs is controlled through 256 linear
steps to FFh"
I don't think that is the intention of the gdc is it? Looking at the
original gdc code, it thus sets the global BRIGHTNESS based on the
period/on_time. I don't think that is what we expect when we enable blink.

 From my understanding, the grppwm is super-imposed, thus by setting gdc to
0, we do not superimpose anything and the original brightness is retained.
(If i'm wrong here, we need to set gdc to 0xff.

Because of this, I even recommend removing gdc all together, which saves
another i2c write.

Or am I wrong here?

Olliver

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hi Ricardo,

On 20-04-16 11:17, Ricardo Ribalda Delgado wrote:

Hello again

On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl  wrote:


The devil is in the details :)

:)

Saving mode2 sounds like a good compromise then.

But I still believe that we should limit the lock to ledout. No matter
what we do, we cannot have two leds blinking at different frequencies
on the same chip.

So to save a mutex a little bit, we take the risk that nobody else enables
the blink or if they do, enable it in the same way?
If it saves so much, then I guess its worth the risk I suppose?

Give me a day to go through the chip doc and see if I can find a good
compromise, that at least warranties that the leds that are enable
stay enabled ;)
Right, I also went over the datasheet, and I think we can simplyfy two 
things.


For one, yes, move the mode2 register completly to the probe section. 
Set the DMBLINK led to always 1. It does not get cleared, I was wrong. 
We have to set it to as with 0 we do not get any blinking at all 
(grpfreq gets ignored).


Furthermore, we should change:
 -gdc = (time_on * 256) / period;
+   gdc = 0x00;

Because the calculation does not make sense. GDC is the global 
brightness/pwm/dimming control. It is used to uniformly change the blink 
rate on all the linked leds.


"General brightness for the 16 outputs is controlled through 256 linear 
steps to FFh"
I don't think that is the intention of the gdc is it? Looking at the 
original gdc code, it thus sets the global BRIGHTNESS based on the 
period/on_time. I don't think that is what we expect when we enable blink.


From my understanding, the grppwm is super-imposed, thus by setting gdc 
to 0, we do not superimpose anything and the original brightness is 
retained. (If i'm wrong here, we need to set gdc to 0xff.


Because of this, I even recommend removing gdc all together, which saves 
another i2c write.


Or am I wrong here?

Olliver


Regards!

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hi Ricardo,

On 20-04-16 11:17, Ricardo Ribalda Delgado wrote:

Hello again

On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl  wrote:


The devil is in the details :)

:)

Saving mode2 sounds like a good compromise then.

But I still believe that we should limit the lock to ledout. No matter
what we do, we cannot have two leds blinking at different frequencies
on the same chip.

So to save a mutex a little bit, we take the risk that nobody else enables
the blink or if they do, enable it in the same way?
If it saves so much, then I guess its worth the risk I suppose?

Give me a day to go through the chip doc and see if I can find a good
compromise, that at least warranties that the leds that are enable
stay enabled ;)
Right, I also went over the datasheet, and I think we can simplyfy two 
things.


For one, yes, move the mode2 register completly to the probe section. 
Set the DMBLINK led to always 1. It does not get cleared, I was wrong. 
We have to set it to as with 0 we do not get any blinking at all 
(grpfreq gets ignored).


Furthermore, we should change:
 -gdc = (time_on * 256) / period;
+   gdc = 0x00;

Because the calculation does not make sense. GDC is the global 
brightness/pwm/dimming control. It is used to uniformly change the blink 
rate on all the linked leds.


"General brightness for the 16 outputs is controlled through 256 linear 
steps to FFh"
I don't think that is the intention of the gdc is it? Looking at the 
original gdc code, it thus sets the global BRIGHTNESS based on the 
period/on_time. I don't think that is what we expect when we enable blink.


From my understanding, the grppwm is super-imposed, thus by setting gdc 
to 0, we do not superimpose anything and the original brightness is 
retained. (If i'm wrong here, we need to set gdc to 0xff.


Because of this, I even recommend removing gdc all together, which saves 
another i2c write.


Or am I wrong here?

Olliver


Regards!

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hey,

On 20-04-16 11:17, Ricardo Ribalda Delgado wrote:

Hello again

On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl  wrote:


The devil is in the details :)

:)

Saving mode2 sounds like a good compromise then.

But I still believe that we should limit the lock to ledout. No matter
what we do, we cannot have two leds blinking at different frequencies
on the same chip.

So to save a mutex a little bit, we take the risk that nobody else enables
the blink or if they do, enable it in the same way?
If it saves so much, then I guess its worth the risk I suppose?

Give me a day to go through the chip doc and see if I can find a good
compromise, that at least warranties that the leds that are enable
stay enabled ;)
sure thing. I have read the docs quite a few times for using it directly 
via i2c, but yeah I'll wait.


Regards!

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hey,

On 20-04-16 11:17, Ricardo Ribalda Delgado wrote:

Hello again

On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl  wrote:


The devil is in the details :)

:)

Saving mode2 sounds like a good compromise then.

But I still believe that we should limit the lock to ledout. No matter
what we do, we cannot have two leds blinking at different frequencies
on the same chip.

So to save a mutex a little bit, we take the risk that nobody else enables
the blink or if they do, enable it in the same way?
If it saves so much, then I guess its worth the risk I suppose?

Give me a day to go through the chip doc and see if I can find a good
compromise, that at least warranties that the leds that are enable
stay enabled ;)
sure thing. I have read the docs quite a few times for using it directly 
via i2c, but yeah I'll wait.


Regards!

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Ricardo Ribalda Delgado]

Hello again

On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl  wrote:

> The devil is in the details :)
:)
>>
>> Saving mode2 sounds like a good compromise then.
>>
>> But I still believe that we should limit the lock to ledout. No matter
>> what we do, we cannot have two leds blinking at different frequencies
>> on the same chip.
>
> So to save a mutex a little bit, we take the risk that nobody else enables
> the blink or if they do, enable it in the same way?
> If it saves so much, then I guess its worth the risk I suppose?

Give me a day to go through the chip doc and see if I can find a good
compromise, that at least warranties that the leds that are enable
stay enabled ;)

Regards!



-- 
Ricardo Ribalda

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Ricardo Ribalda Delgado]

Hello again

On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl  wrote:

> The devil is in the details :)
:)
>>
>> Saving mode2 sounds like a good compromise then.
>>
>> But I still believe that we should limit the lock to ledout. No matter
>> what we do, we cannot have two leds blinking at different frequencies
>> on the same chip.
>
> So to save a mutex a little bit, we take the risk that nobody else enables
> the blink or if they do, enable it in the same way?
> If it saves so much, then I guess its worth the risk I suppose?

Give me a day to go through the chip doc and see if I can find a good
compromise, that at least warranties that the leds that are enable
stay enabled ;)

Regards!



-- 
Ricardo Ribalda

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

On 20-04-16 10:56, Ricardo Ribalda Delgado wrote:

Hi

On Wed, Apr 20, 2016 at 10:51 AM, Olliver Schinagl  wrote:


As I said before, the reason for this proposal is that the code NEVER
clears PCA963X_MODE2_DMBLNK, only sets it.
Unfortunately I do not have the HW to test this change.

The code never clears it, but the hardware does. So we have to set it
everytime we enable blink.

Ok, that was the part I was missing. I was not aware that the hw was
clearing it.

The devil is in the details :)

Saving mode2 sounds like a good compromise then.

But I still believe that we should limit the lock to ledout. No matter
what we do, we cannot have two leds blinking at different frequencies
on the same chip.
So to save a mutex a little bit, we take the risk that nobody else 
enables the blink or if they do, enable it in the same way?

If it saves so much, then I guess its worth the risk I suppose?



Regards

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

On 20-04-16 10:56, Ricardo Ribalda Delgado wrote:

Hi

On Wed, Apr 20, 2016 at 10:51 AM, Olliver Schinagl  wrote:


As I said before, the reason for this proposal is that the code NEVER
clears PCA963X_MODE2_DMBLNK, only sets it.
Unfortunately I do not have the HW to test this change.

The code never clears it, but the hardware does. So we have to set it
everytime we enable blink.

Ok, that was the part I was missing. I was not aware that the hw was
clearing it.

The devil is in the details :)

Saving mode2 sounds like a good compromise then.

But I still believe that we should limit the lock to ledout. No matter
what we do, we cannot have two leds blinking at different frequencies
on the same chip.
So to save a mutex a little bit, we take the risk that nobody else 
enables the blink or if they do, enable it in the same way?

If it saves so much, then I guess its worth the risk I suppose?



Regards

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Ricardo Ribalda Delgado]

Hi

On Wed, Apr 20, 2016 at 10:51 AM, Olliver Schinagl  wrote:

>> As I said before, the reason for this proposal is that the code NEVER
>> clears PCA963X_MODE2_DMBLNK, only sets it.
>> Unfortunately I do not have the HW to test this change.
>
> The code never clears it, but the hardware does. So we have to set it
> everytime we enable blink.

Ok, that was the part I was missing. I was not aware that the hw was
clearing it.

Saving mode2 sounds like a good compromise then.

But I still believe that we should limit the lock to ledout. No matter
what we do, we cannot have two leds blinking at different frequencies
on the same chip.


Regards

-- 
Ricardo Ribalda

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Ricardo Ribalda Delgado]

Hi

On Wed, Apr 20, 2016 at 10:51 AM, Olliver Schinagl  wrote:

>> As I said before, the reason for this proposal is that the code NEVER
>> clears PCA963X_MODE2_DMBLNK, only sets it.
>> Unfortunately I do not have the HW to test this change.
>
> The code never clears it, but the hardware does. So we have to set it
> everytime we enable blink.

Ok, that was the part I was missing. I was not aware that the hw was
clearing it.

Saving mode2 sounds like a good compromise then.

But I still believe that we should limit the lock to ledout. No matter
what we do, we cannot have two leds blinking at different frequencies
on the same chip.


Regards

-- 
Ricardo Ribalda

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hey Ricardo,

On 20-04-16 10:01, Ricardo Ribalda Delgado wrote:

Hi Ollivier


On Wed, Apr 20, 2016 at 9:21 AM, Olliver Schinagl  wrote:

What I am propossing is at probe():

replace:

if (pdata) {
/* Configure output: open-drain or totem pole (push-pull) */
if (pdata->outdrv == PCA963X_OPEN_DRAIN)
i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x01);
else
i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x05);
}

with something like (forgive the spacing):


u8 mode2 = PCA963X_MODE2_DMBLNK | 0x1;
if (pdata && pdata->outdrv == PCA963X_OPEN_DRAIN)
mode2 |= 0x4;
i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2, mode2);


and then remove from pca963x_blink() these lines:

u8 mode2 = i2c_smbus_read_byte_data(pca963x->chip->client,
PCA963X_MODE2);

if (!(mode2 & PCA963X_MODE2_DMBLNK))
i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2,
mode2 | PCA963X_MODE2_DMBLNK);

As I said before, the reason for this proposal is that the code NEVER
clears PCA963X_MODE2_DMBLNK, only sets it.
Unfortunately I do not have the HW to test this change.
The code never clears it, but the hardware does. So we have to set it 
everytime we enable blink.


So I don't think this can work at all, as you now never enable blink 
(when you enable it from user space, at probe it works fine via the 
default trigger).


What about storing the mode2 register on the chip level, since we never 
change it after probe, and only toggle the blink bit in blink_set() and 
write the stored mode2 register


so i2c_write(pca963x->chip->mode2 | MODE2_DMBLNK)

The only advantage I see here though is that we save a read + (quite 
likley) potentially a write, with an always write. So it saves 1 i2c 
read command.


Olliver



Now I understand your concern, the i2c operations are slow and time
consuming making the mutex very expensive.

The thing is, to be able to write the blink bit, we need to read the whole
mode2 register, to do a proper read-modify-write. We don't know what's in
the mode2 register and we only want to write the bit if it is actually not
set to begin with, to save a i2c write operation.

As I said earlier, nowhere in the code clears that bit. The bit is
only set. so no reason to read/modify/write. We can set that bit at
probe time and assume that it will not be changed.


We start this function already however with with two write calls of
sequential registers, the grp and pwm enable registers. There is even a call
to automatically update these registers, which I think we'd use
i2c_master_send() to set the address via the auto-increment register and
enable auto increment of these two registers. Now we reduced the 2 seperate
calls into one bigger 'faster' call. So 1 win there. But! it will require us
however to change the other calls to disable auto increment via de mode1
register. Since this is an extra i2c_write operation, it makes the other i2c
writes more expensive, which may happen much more often (enable blink only
happens occasionally, changing the brightness may change a lot (fade in fade
out).

Be careful with that. Sometimes this chips are connected to the smbus,
wich has a limited number of operations/lengths. We should keep the
i2c_smbus_write_byte_data() call, because that makes the driver
compatible with more hardware.


So unless i'm totally misunderstanding something, I don't think we can safe
much here at all.

To be clear: My proposal is that (after being tested), move the set of
DMBLINK to probe, remove the read/modify/write from blink() and keep
the locking as it is now, only protecting ledout.

Also you need to fix the patch that breaks bisect, but I believe that
you are already working on that.

I have reviewed a bit Documentation/device-tree and it seems that
there is already a binding for active-low. Instead of nxp,active-low
you should call it just active-low. But I am not a device-tree expert.

Finally, you mention that you are fixing some checkpatch errors, but I
cannot replicate those in my side :S

ricardo@pilix:~/curro/linux$ scripts/checkpatch.pl -f
drivers/leds/leds-pca963x.c
total: 0 errors, 0 warnings, 439 lines checked

drivers/leds/leds-pca963x.c has no obvious style problems and is ready
for submission.

So maybe the errors you are fixing are introduced by your patches.
About the other style patches, I do not know what is the policy of the
Maintainer in that matter, especially when the driver did not break
originally checkpatch.



Regards!


---
Ricardo

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hey Ricardo,

On 20-04-16 10:01, Ricardo Ribalda Delgado wrote:

Hi Ollivier


On Wed, Apr 20, 2016 at 9:21 AM, Olliver Schinagl  wrote:

What I am propossing is at probe():

replace:

if (pdata) {
/* Configure output: open-drain or totem pole (push-pull) */
if (pdata->outdrv == PCA963X_OPEN_DRAIN)
i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x01);
else
i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x05);
}

with something like (forgive the spacing):


u8 mode2 = PCA963X_MODE2_DMBLNK | 0x1;
if (pdata && pdata->outdrv == PCA963X_OPEN_DRAIN)
mode2 |= 0x4;
i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2, mode2);


and then remove from pca963x_blink() these lines:

u8 mode2 = i2c_smbus_read_byte_data(pca963x->chip->client,
PCA963X_MODE2);

if (!(mode2 & PCA963X_MODE2_DMBLNK))
i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2,
mode2 | PCA963X_MODE2_DMBLNK);

As I said before, the reason for this proposal is that the code NEVER
clears PCA963X_MODE2_DMBLNK, only sets it.
Unfortunately I do not have the HW to test this change.
The code never clears it, but the hardware does. So we have to set it 
everytime we enable blink.


So I don't think this can work at all, as you now never enable blink 
(when you enable it from user space, at probe it works fine via the 
default trigger).


What about storing the mode2 register on the chip level, since we never 
change it after probe, and only toggle the blink bit in blink_set() and 
write the stored mode2 register


so i2c_write(pca963x->chip->mode2 | MODE2_DMBLNK)

The only advantage I see here though is that we save a read + (quite 
likley) potentially a write, with an always write. So it saves 1 i2c 
read command.


Olliver



Now I understand your concern, the i2c operations are slow and time
consuming making the mutex very expensive.

The thing is, to be able to write the blink bit, we need to read the whole
mode2 register, to do a proper read-modify-write. We don't know what's in
the mode2 register and we only want to write the bit if it is actually not
set to begin with, to save a i2c write operation.

As I said earlier, nowhere in the code clears that bit. The bit is
only set. so no reason to read/modify/write. We can set that bit at
probe time and assume that it will not be changed.


We start this function already however with with two write calls of
sequential registers, the grp and pwm enable registers. There is even a call
to automatically update these registers, which I think we'd use
i2c_master_send() to set the address via the auto-increment register and
enable auto increment of these two registers. Now we reduced the 2 seperate
calls into one bigger 'faster' call. So 1 win there. But! it will require us
however to change the other calls to disable auto increment via de mode1
register. Since this is an extra i2c_write operation, it makes the other i2c
writes more expensive, which may happen much more often (enable blink only
happens occasionally, changing the brightness may change a lot (fade in fade
out).

Be careful with that. Sometimes this chips are connected to the smbus,
wich has a limited number of operations/lengths. We should keep the
i2c_smbus_write_byte_data() call, because that makes the driver
compatible with more hardware.


So unless i'm totally misunderstanding something, I don't think we can safe
much here at all.

To be clear: My proposal is that (after being tested), move the set of
DMBLINK to probe, remove the read/modify/write from blink() and keep
the locking as it is now, only protecting ledout.

Also you need to fix the patch that breaks bisect, but I believe that
you are already working on that.

I have reviewed a bit Documentation/device-tree and it seems that
there is already a binding for active-low. Instead of nxp,active-low
you should call it just active-low. But I am not a device-tree expert.

Finally, you mention that you are fixing some checkpatch errors, but I
cannot replicate those in my side :S

ricardo@pilix:~/curro/linux$ scripts/checkpatch.pl -f
drivers/leds/leds-pca963x.c
total: 0 errors, 0 warnings, 439 lines checked

drivers/leds/leds-pca963x.c has no obvious style problems and is ready
for submission.

So maybe the errors you are fixing are introduced by your patches.
About the other style patches, I do not know what is the policy of the
Maintainer in that matter, especially when the driver did not break
originally checkpatch.



Regards!


---
Ricardo

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Ricardo Ribalda Delgado]

Hi Ollivier


On Wed, Apr 20, 2016 at 9:21 AM, Olliver Schinagl  wrote:

What I am propossing is at probe():

replace:

if (pdata) {
/* Configure output: open-drain or totem pole (push-pull) */
if (pdata->outdrv == PCA963X_OPEN_DRAIN)
i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x01);
else
i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x05);
}

with something like (forgive the spacing):


u8 mode2 = PCA963X_MODE2_DMBLNK | 0x1;
if (pdata && pdata->outdrv == PCA963X_OPEN_DRAIN)
   mode2 |= 0x4;
i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2, mode2);


and then remove from pca963x_blink() these lines:

u8 mode2 = i2c_smbus_read_byte_data(pca963x->chip->client,
PCA963X_MODE2);

if (!(mode2 & PCA963X_MODE2_DMBLNK))
i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2,
mode2 | PCA963X_MODE2_DMBLNK);

As I said before, the reason for this proposal is that the code NEVER
clears PCA963X_MODE2_DMBLNK, only sets it.
Unfortunately I do not have the HW to test this change.

>
> Now I understand your concern, the i2c operations are slow and time
> consuming making the mutex very expensive.
>
> The thing is, to be able to write the blink bit, we need to read the whole
> mode2 register, to do a proper read-modify-write. We don't know what's in
> the mode2 register and we only want to write the bit if it is actually not
> set to begin with, to save a i2c write operation.

As I said earlier, nowhere in the code clears that bit. The bit is
only set. so no reason to read/modify/write. We can set that bit at
probe time and assume that it will not be changed.

>
> We start this function already however with with two write calls of
> sequential registers, the grp and pwm enable registers. There is even a call
> to automatically update these registers, which I think we'd use
> i2c_master_send() to set the address via the auto-increment register and
> enable auto increment of these two registers. Now we reduced the 2 seperate
> calls into one bigger 'faster' call. So 1 win there. But! it will require us
> however to change the other calls to disable auto increment via de mode1
> register. Since this is an extra i2c_write operation, it makes the other i2c
> writes more expensive, which may happen much more often (enable blink only
> happens occasionally, changing the brightness may change a lot (fade in fade
> out).

Be careful with that. Sometimes this chips are connected to the smbus,
wich has a limited number of operations/lengths. We should keep the
i2c_smbus_write_byte_data() call, because that makes the driver
compatible with more hardware.

>
> So unless i'm totally misunderstanding something, I don't think we can safe
> much here at all.

To be clear: My proposal is that (after being tested), move the set of
DMBLINK to probe, remove the read/modify/write from blink() and keep
the locking as it is now, only protecting ledout.

Also you need to fix the patch that breaks bisect, but I believe that
you are already working on that.

I have reviewed a bit Documentation/device-tree and it seems that
there is already a binding for active-low. Instead of nxp,active-low
you should call it just active-low. But I am not a device-tree expert.

Finally, you mention that you are fixing some checkpatch errors, but I
cannot replicate those in my side :S

ricardo@pilix:~/curro/linux$ scripts/checkpatch.pl -f
drivers/leds/leds-pca963x.c
total: 0 errors, 0 warnings, 439 lines checked

drivers/leds/leds-pca963x.c has no obvious style problems and is ready
for submission.

So maybe the errors you are fixing are introduced by your patches.
About the other style patches, I do not know what is the policy of the
Maintainer in that matter, especially when the driver did not break
originally checkpatch.



Regards!


---
Ricardo

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Ricardo Ribalda Delgado]

Hi Ollivier


On Wed, Apr 20, 2016 at 9:21 AM, Olliver Schinagl  wrote:

What I am propossing is at probe():

replace:

if (pdata) {
/* Configure output: open-drain or totem pole (push-pull) */
if (pdata->outdrv == PCA963X_OPEN_DRAIN)
i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x01);
else
i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x05);
}

with something like (forgive the spacing):


u8 mode2 = PCA963X_MODE2_DMBLNK | 0x1;
if (pdata && pdata->outdrv == PCA963X_OPEN_DRAIN)
   mode2 |= 0x4;
i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2, mode2);


and then remove from pca963x_blink() these lines:

u8 mode2 = i2c_smbus_read_byte_data(pca963x->chip->client,
PCA963X_MODE2);

if (!(mode2 & PCA963X_MODE2_DMBLNK))
i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2,
mode2 | PCA963X_MODE2_DMBLNK);

As I said before, the reason for this proposal is that the code NEVER
clears PCA963X_MODE2_DMBLNK, only sets it.
Unfortunately I do not have the HW to test this change.

>
> Now I understand your concern, the i2c operations are slow and time
> consuming making the mutex very expensive.
>
> The thing is, to be able to write the blink bit, we need to read the whole
> mode2 register, to do a proper read-modify-write. We don't know what's in
> the mode2 register and we only want to write the bit if it is actually not
> set to begin with, to save a i2c write operation.

As I said earlier, nowhere in the code clears that bit. The bit is
only set. so no reason to read/modify/write. We can set that bit at
probe time and assume that it will not be changed.

>
> We start this function already however with with two write calls of
> sequential registers, the grp and pwm enable registers. There is even a call
> to automatically update these registers, which I think we'd use
> i2c_master_send() to set the address via the auto-increment register and
> enable auto increment of these two registers. Now we reduced the 2 seperate
> calls into one bigger 'faster' call. So 1 win there. But! it will require us
> however to change the other calls to disable auto increment via de mode1
> register. Since this is an extra i2c_write operation, it makes the other i2c
> writes more expensive, which may happen much more often (enable blink only
> happens occasionally, changing the brightness may change a lot (fade in fade
> out).

Be careful with that. Sometimes this chips are connected to the smbus,
wich has a limited number of operations/lengths. We should keep the
i2c_smbus_write_byte_data() call, because that makes the driver
compatible with more hardware.

>
> So unless i'm totally misunderstanding something, I don't think we can safe
> much here at all.

To be clear: My proposal is that (after being tested), move the set of
DMBLINK to probe, remove the read/modify/write from blink() and keep
the locking as it is now, only protecting ledout.

Also you need to fix the patch that breaks bisect, but I believe that
you are already working on that.

I have reviewed a bit Documentation/device-tree and it seems that
there is already a binding for active-low. Instead of nxp,active-low
you should call it just active-low. But I am not a device-tree expert.

Finally, you mention that you are fixing some checkpatch errors, but I
cannot replicate those in my side :S

ricardo@pilix:~/curro/linux$ scripts/checkpatch.pl -f
drivers/leds/leds-pca963x.c
total: 0 errors, 0 warnings, 439 lines checked

drivers/leds/leds-pca963x.c has no obvious style problems and is ready
for submission.

So maybe the errors you are fixing are introduced by your patches.
About the other style patches, I do not know what is the policy of the
Maintainer in that matter, especially when the driver did not break
originally checkpatch.



Regards!


---
Ricardo

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hey Ricardo,

On 19-04-16 15:42, Ricardo Ribalda Delgado wrote:

Hi again

On Tue, Apr 19, 2016 at 3:27 PM, Olliver Schinagl  wrote:

Hey Ricardo,
Without actually looking at the code right now, but the driver does a
read/modify/write on the register, and a register is shared among several
leds. So in that regard, it makes sense and I don't think it's very
expensive to move the lock, since we have to lock for the write a few lines
down anyway.

Actually, the code is only making sure that PCA963X_MODE2_DMBLNK is
on. It is never cleared afterwards.

i do not think this can work at all actually.

While trying to move those lines to probe and thinking about the 
consequences, I noticed blink is now never enabled again.
E.g. the probe reads the blink bit at probe, updates its internal 
trigger to timer etc and now during probe, if there is no 
default-trigger, we now have the correct trigger set.


However, when we enable blink via the timer trigger for example, the 
blink_set() gets executed and it writes the blink bit.


mode2 = i2c_smbus_read_byte_data(pca963x->chip->client, PCA963X_MODE2);
if (!(mode2 & PCA963X_MODE2_DMBLNK))
i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2,
mode2 | PCA963X_MODE2_DMBLNK);


so after the read, we immediatly do a write.

Now I understand your concern, the i2c operations are slow and time 
consuming making the mutex very expensive.


The thing is, to be able to write the blink bit, we need to read the 
whole mode2 register, to do a proper read-modify-write. We don't know 
what's in the mode2 register and we only want to write the bit if it is 
actually not set to begin with, to save a i2c write operation.


We start this function already however with with two write calls of 
sequential registers, the grp and pwm enable registers. There is even a 
call to automatically update these registers, which I think we'd use 
i2c_master_send() to set the address via the auto-increment register and 
enable auto increment of these two registers. Now we reduced the 2 
seperate calls into one bigger 'faster' call. So 1 win there. But! it 
will require us however to change the other calls to disable auto 
increment via de mode1 register. Since this is an extra i2c_write 
operation, it makes the other i2c writes more expensive, which may 
happen much more often (enable blink only happens occasionally, changing 
the brightness may change a lot (fade in fade out).


So unless i'm totally misunderstanding something, I don't think we can 
safe much here at all.


The only win would be by not reading the mode2 in the mutex, but what if 
we read the register, someone else modifies it, and we write to it again?


olliver



It will be great if you could set that bit on probe and remove those
two lines and verify that it works on real hardware.


The move of the lock can be a bit expensive. i2c writes can take a
while to be performed, this is why only ledout was protected
initially.

Best regards

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hey Ricardo,

On 19-04-16 15:42, Ricardo Ribalda Delgado wrote:

Hi again

On Tue, Apr 19, 2016 at 3:27 PM, Olliver Schinagl  wrote:

Hey Ricardo,
Without actually looking at the code right now, but the driver does a
read/modify/write on the register, and a register is shared among several
leds. So in that regard, it makes sense and I don't think it's very
expensive to move the lock, since we have to lock for the write a few lines
down anyway.

Actually, the code is only making sure that PCA963X_MODE2_DMBLNK is
on. It is never cleared afterwards.

i do not think this can work at all actually.

While trying to move those lines to probe and thinking about the 
consequences, I noticed blink is now never enabled again.
E.g. the probe reads the blink bit at probe, updates its internal 
trigger to timer etc and now during probe, if there is no 
default-trigger, we now have the correct trigger set.


However, when we enable blink via the timer trigger for example, the 
blink_set() gets executed and it writes the blink bit.


mode2 = i2c_smbus_read_byte_data(pca963x->chip->client, PCA963X_MODE2);
if (!(mode2 & PCA963X_MODE2_DMBLNK))
i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2,
mode2 | PCA963X_MODE2_DMBLNK);


so after the read, we immediatly do a write.

Now I understand your concern, the i2c operations are slow and time 
consuming making the mutex very expensive.


The thing is, to be able to write the blink bit, we need to read the 
whole mode2 register, to do a proper read-modify-write. We don't know 
what's in the mode2 register and we only want to write the bit if it is 
actually not set to begin with, to save a i2c write operation.


We start this function already however with with two write calls of 
sequential registers, the grp and pwm enable registers. There is even a 
call to automatically update these registers, which I think we'd use 
i2c_master_send() to set the address via the auto-increment register and 
enable auto increment of these two registers. Now we reduced the 2 
seperate calls into one bigger 'faster' call. So 1 win there. But! it 
will require us however to change the other calls to disable auto 
increment via de mode1 register. Since this is an extra i2c_write 
operation, it makes the other i2c writes more expensive, which may 
happen much more often (enable blink only happens occasionally, changing 
the brightness may change a lot (fade in fade out).


So unless i'm totally misunderstanding something, I don't think we can 
safe much here at all.


The only win would be by not reading the mode2 in the mutex, but what if 
we read the register, someone else modifies it, and we write to it again?


olliver



It will be great if you could set that bit on probe and remove those
two lines and verify that it works on real hardware.


The move of the lock can be a bit expensive. i2c writes can take a
while to be performed, this is why only ledout was protected
initially.

Best regards

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Ricardo Ribalda Delgado]

Hi again

On Tue, Apr 19, 2016 at 3:27 PM, Olliver Schinagl  wrote:
> Hey Ricardo,

> Without actually looking at the code right now, but the driver does a
> read/modify/write on the register, and a register is shared among several
> leds. So in that regard, it makes sense and I don't think it's very
> expensive to move the lock, since we have to lock for the write a few lines
> down anyway.

Actually, the code is only making sure that PCA963X_MODE2_DMBLNK is
on. It is never cleared afterwards.

It will be great if you could set that bit on probe and remove those
two lines and verify that it works on real hardware.


The move of the lock can be a bit expensive. i2c writes can take a
while to be performed, this is why only ledout was protected
initially.

Best regards

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Ricardo Ribalda Delgado]

Hi again

On Tue, Apr 19, 2016 at 3:27 PM, Olliver Schinagl  wrote:
> Hey Ricardo,

> Without actually looking at the code right now, but the driver does a
> read/modify/write on the register, and a register is shared among several
> leds. So in that regard, it makes sense and I don't think it's very
> expensive to move the lock, since we have to lock for the write a few lines
> down anyway.

Actually, the code is only making sure that PCA963X_MODE2_DMBLNK is
on. It is never cleared afterwards.

It will be great if you could set that bit on probe and remove those
two lines and verify that it works on real hardware.


The move of the lock can be a bit expensive. i2c writes can take a
while to be performed, this is why only ledout was protected
initially.

Best regards

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hey Ricardo,

On 19-04-16 13:18, Ricardo Ribalda Delgado wrote:

Hi Ollivier

Sorry to not reply to the patches, but I am not subscribed to linux-leds

Regarding:
[PATCH 2/6] leds: pca963x: Lock i2c r/w access

I am not sure why this patch is needed. the only thing that should be
protected is the write to ledout.

It seems that mode2 needs to be set to PCA963X_MODE2_DMBLNK, or at
least, the driver never clears that bit. Couldnt we just set it at
probe time and remove the read/write of it? I do not have the hardware
at the moment, so it should be something that you need to test.
Without actually looking at the code right now, but the driver does a 
read/modify/write on the register, and a register is shared among 
several leds. So in that regard, it makes sense and I don't think it's 
very expensive to move the lock, since we have to lock for the write a 
few lines down anyway.


[PATCH 4/6] leds: pca963x: Reduce magic values
Maybe you want to create the inverse macro of PCA963X_LEDOUT_LDR, so
you can do something linke

PCA963X_LEDOUT_LDR_INV(ledout, pca963x->led_num) != PCA963X_LEDOUT_LED_GRP_PWM

Good point, I'll add it, I like it.



[PATCH 3/6] leds: pca963x: Add defines and remove some magic values

I am not big fan of defining things that are not used. and the magic
assigment to n_leds is perfectly fine IMHO
Well i needed some of the defines for the invert part and then I figured 
just add everything that the datasheet defines to make everything 
exlusive/easy to use.


But I can remove unused defines if desired.


For PCA963X_LEDOUT_LDR.  Do not forget the parenthesis around led_num.
Also replace %4 with &3 to be consisten.t

Yeah, i'll check and fix that.


Regards!

On Tue, Apr 19, 2016 at 11:39 AM, Olliver Schinagl  wrote:

On 19-04-16 11:23, Jacek Anaszewski wrote:

Hi Olliver,

Thanks for the patches.
Adding driver authors on cc.

Ah sorry about that, thanks. I guess get_maintainers doesn't do that.

As for the compile bug, I'll fix that with v2, it only applies on the
intermediate patches, not on the whole set.

Olliver


On 04/19/2016 09:40 AM, Olliver Schinagl wrote:

Using the pca963x for a while, I noticed something that may look like
some
i2c accessing issues where sometimes data was incorrectly written to the
bus,
possibly because we where not properly locking the i2c reads. Though I'm
not
familiar enough with the i2c framework to be certain reads need to be
locked
at all. A patch was added to properly lock i2c access more tightly.

Furthermore there was no method to support inverted outputs. This series
adds a property to the device tree to inform the driver that the output
is inverted (active-high vs active-low).

Additionally, this patch set does some cleanups to please checkpatch, and
removes a few magic values.

Olliver Schinagl (6):
leds: pca963x: Alphabetize headers
leds: pca963x: Lock i2c r/w access
leds: pca963x: Add defines and remove some magic values
leds: pca963x: Reduce magic values
leds: pca963x: Inform the output that it is inverted
leds: pca963x: Remove whitespace and checkpatch problems

   Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
   drivers/leds/leds-pca963x.c| 243
++---
   include/linux/platform_data/leds-pca963x.h |   1 +
   3 files changed, 171 insertions(+), 74 deletions(-)

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Hey Ricardo,

On 19-04-16 13:18, Ricardo Ribalda Delgado wrote:

Hi Ollivier

Sorry to not reply to the patches, but I am not subscribed to linux-leds

Regarding:
[PATCH 2/6] leds: pca963x: Lock i2c r/w access

I am not sure why this patch is needed. the only thing that should be
protected is the write to ledout.

It seems that mode2 needs to be set to PCA963X_MODE2_DMBLNK, or at
least, the driver never clears that bit. Couldnt we just set it at
probe time and remove the read/write of it? I do not have the hardware
at the moment, so it should be something that you need to test.
Without actually looking at the code right now, but the driver does a 
read/modify/write on the register, and a register is shared among 
several leds. So in that regard, it makes sense and I don't think it's 
very expensive to move the lock, since we have to lock for the write a 
few lines down anyway.


[PATCH 4/6] leds: pca963x: Reduce magic values
Maybe you want to create the inverse macro of PCA963X_LEDOUT_LDR, so
you can do something linke

PCA963X_LEDOUT_LDR_INV(ledout, pca963x->led_num) != PCA963X_LEDOUT_LED_GRP_PWM

Good point, I'll add it, I like it.



[PATCH 3/6] leds: pca963x: Add defines and remove some magic values

I am not big fan of defining things that are not used. and the magic
assigment to n_leds is perfectly fine IMHO
Well i needed some of the defines for the invert part and then I figured 
just add everything that the datasheet defines to make everything 
exlusive/easy to use.


But I can remove unused defines if desired.


For PCA963X_LEDOUT_LDR.  Do not forget the parenthesis around led_num.
Also replace %4 with &3 to be consisten.t

Yeah, i'll check and fix that.


Regards!

On Tue, Apr 19, 2016 at 11:39 AM, Olliver Schinagl  wrote:

On 19-04-16 11:23, Jacek Anaszewski wrote:

Hi Olliver,

Thanks for the patches.
Adding driver authors on cc.

Ah sorry about that, thanks. I guess get_maintainers doesn't do that.

As for the compile bug, I'll fix that with v2, it only applies on the
intermediate patches, not on the whole set.

Olliver


On 04/19/2016 09:40 AM, Olliver Schinagl wrote:

Using the pca963x for a while, I noticed something that may look like
some
i2c accessing issues where sometimes data was incorrectly written to the
bus,
possibly because we where not properly locking the i2c reads. Though I'm
not
familiar enough with the i2c framework to be certain reads need to be
locked
at all. A patch was added to properly lock i2c access more tightly.

Furthermore there was no method to support inverted outputs. This series
adds a property to the device tree to inform the driver that the output
is inverted (active-high vs active-low).

Additionally, this patch set does some cleanups to please checkpatch, and
removes a few magic values.

Olliver Schinagl (6):
leds: pca963x: Alphabetize headers
leds: pca963x: Lock i2c r/w access
leds: pca963x: Add defines and remove some magic values
leds: pca963x: Reduce magic values
leds: pca963x: Inform the output that it is inverted
leds: pca963x: Remove whitespace and checkpatch problems

   Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
   drivers/leds/leds-pca963x.c| 243
++---
   include/linux/platform_data/leds-pca963x.h |   1 +
   3 files changed, 171 insertions(+), 74 deletions(-)

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Ricardo Ribalda Delgado]

Hi Ollivier

Sorry to not reply to the patches, but I am not subscribed to linux-leds

Regarding:
[PATCH 2/6] leds: pca963x: Lock i2c r/w access

I am not sure why this patch is needed. the only thing that should be
protected is the write to ledout.

It seems that mode2 needs to be set to PCA963X_MODE2_DMBLNK, or at
least, the driver never clears that bit. Couldnt we just set it at
probe time and remove the read/write of it? I do not have the hardware
at the moment, so it should be something that you need to test.

[PATCH 4/6] leds: pca963x: Reduce magic values
Maybe you want to create the inverse macro of PCA963X_LEDOUT_LDR, so
you can do something linke

PCA963X_LEDOUT_LDR_INV(ledout, pca963x->led_num) != PCA963X_LEDOUT_LED_GRP_PWM


[PATCH 3/6] leds: pca963x: Add defines and remove some magic values

I am not big fan of defining things that are not used. and the magic
assigment to n_leds is perfectly fine IMHO

For PCA963X_LEDOUT_LDR.  Do not forget the parenthesis around led_num.
Also replace %4 with &3 to be consisten.t

Regards!

On Tue, Apr 19, 2016 at 11:39 AM, Olliver Schinagl  wrote:
> On 19-04-16 11:23, Jacek Anaszewski wrote:
>>
>> Hi Olliver,
>>
>> Thanks for the patches.
>> Adding driver authors on cc.
>
> Ah sorry about that, thanks. I guess get_maintainers doesn't do that.
>
> As for the compile bug, I'll fix that with v2, it only applies on the
> intermediate patches, not on the whole set.
>
> Olliver
>
>>
>> On 04/19/2016 09:40 AM, Olliver Schinagl wrote:
>>>
>>> Using the pca963x for a while, I noticed something that may look like
>>> some
>>> i2c accessing issues where sometimes data was incorrectly written to the
>>> bus,
>>> possibly because we where not properly locking the i2c reads. Though I'm
>>> not
>>> familiar enough with the i2c framework to be certain reads need to be
>>> locked
>>> at all. A patch was added to properly lock i2c access more tightly.
>>>
>>> Furthermore there was no method to support inverted outputs. This series
>>> adds a property to the device tree to inform the driver that the output
>>> is inverted (active-high vs active-low).
>>>
>>> Additionally, this patch set does some cleanups to please checkpatch, and
>>> removes a few magic values.
>>>
>>> Olliver Schinagl (6):
>>>leds: pca963x: Alphabetize headers
>>>leds: pca963x: Lock i2c r/w access
>>>leds: pca963x: Add defines and remove some magic values
>>>leds: pca963x: Reduce magic values
>>>leds: pca963x: Inform the output that it is inverted
>>>leds: pca963x: Remove whitespace and checkpatch problems
>>>
>>>   Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
>>>   drivers/leds/leds-pca963x.c| 243
>>> ++---
>>>   include/linux/platform_data/leds-pca963x.h |   1 +
>>>   3 files changed, 171 insertions(+), 74 deletions(-)
>>>
>>
>>
>



-- 
Ricardo Ribalda

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Ricardo Ribalda Delgado]

Hi Ollivier

Sorry to not reply to the patches, but I am not subscribed to linux-leds

Regarding:
[PATCH 2/6] leds: pca963x: Lock i2c r/w access

I am not sure why this patch is needed. the only thing that should be
protected is the write to ledout.

It seems that mode2 needs to be set to PCA963X_MODE2_DMBLNK, or at
least, the driver never clears that bit. Couldnt we just set it at
probe time and remove the read/write of it? I do not have the hardware
at the moment, so it should be something that you need to test.

[PATCH 4/6] leds: pca963x: Reduce magic values
Maybe you want to create the inverse macro of PCA963X_LEDOUT_LDR, so
you can do something linke

PCA963X_LEDOUT_LDR_INV(ledout, pca963x->led_num) != PCA963X_LEDOUT_LED_GRP_PWM


[PATCH 3/6] leds: pca963x: Add defines and remove some magic values

I am not big fan of defining things that are not used. and the magic
assigment to n_leds is perfectly fine IMHO

For PCA963X_LEDOUT_LDR.  Do not forget the parenthesis around led_num.
Also replace %4 with &3 to be consisten.t

Regards!

On Tue, Apr 19, 2016 at 11:39 AM, Olliver Schinagl  wrote:
> On 19-04-16 11:23, Jacek Anaszewski wrote:
>>
>> Hi Olliver,
>>
>> Thanks for the patches.
>> Adding driver authors on cc.
>
> Ah sorry about that, thanks. I guess get_maintainers doesn't do that.
>
> As for the compile bug, I'll fix that with v2, it only applies on the
> intermediate patches, not on the whole set.
>
> Olliver
>
>>
>> On 04/19/2016 09:40 AM, Olliver Schinagl wrote:
>>>
>>> Using the pca963x for a while, I noticed something that may look like
>>> some
>>> i2c accessing issues where sometimes data was incorrectly written to the
>>> bus,
>>> possibly because we where not properly locking the i2c reads. Though I'm
>>> not
>>> familiar enough with the i2c framework to be certain reads need to be
>>> locked
>>> at all. A patch was added to properly lock i2c access more tightly.
>>>
>>> Furthermore there was no method to support inverted outputs. This series
>>> adds a property to the device tree to inform the driver that the output
>>> is inverted (active-high vs active-low).
>>>
>>> Additionally, this patch set does some cleanups to please checkpatch, and
>>> removes a few magic values.
>>>
>>> Olliver Schinagl (6):
>>>leds: pca963x: Alphabetize headers
>>>leds: pca963x: Lock i2c r/w access
>>>leds: pca963x: Add defines and remove some magic values
>>>leds: pca963x: Reduce magic values
>>>leds: pca963x: Inform the output that it is inverted
>>>leds: pca963x: Remove whitespace and checkpatch problems
>>>
>>>   Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
>>>   drivers/leds/leds-pca963x.c| 243
>>> ++---
>>>   include/linux/platform_data/leds-pca963x.h |   1 +
>>>   3 files changed, 171 insertions(+), 74 deletions(-)
>>>
>>
>>
>



-- 
Ricardo Ribalda

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

On 19-04-16 11:23, Jacek Anaszewski wrote:

Hi Olliver,

Thanks for the patches.
Adding driver authors on cc.

Ah sorry about that, thanks. I guess get_maintainers doesn't do that.

As for the compile bug, I'll fix that with v2, it only applies on the 
intermediate patches, not on the whole set.


Olliver


On 04/19/2016 09:40 AM, Olliver Schinagl wrote:
Using the pca963x for a while, I noticed something that may look like 
some
i2c accessing issues where sometimes data was incorrectly written to 
the bus,
possibly because we where not properly locking the i2c reads. Though 
I'm not
familiar enough with the i2c framework to be certain reads need to be 
locked

at all. A patch was added to properly lock i2c access more tightly.

Furthermore there was no method to support inverted outputs. This series
adds a property to the device tree to inform the driver that the output
is inverted (active-high vs active-low).

Additionally, this patch set does some cleanups to please checkpatch, 
and

removes a few magic values.

Olliver Schinagl (6):
   leds: pca963x: Alphabetize headers
   leds: pca963x: Lock i2c r/w access
   leds: pca963x: Add defines and remove some magic values
   leds: pca963x: Reduce magic values
   leds: pca963x: Inform the output that it is inverted
   leds: pca963x: Remove whitespace and checkpatch problems

  Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
  drivers/leds/leds-pca963x.c| 243 
++---

  include/linux/platform_data/leds-pca963x.h |   1 +
  3 files changed, 171 insertions(+), 74 deletions(-)

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

On 19-04-16 11:23, Jacek Anaszewski wrote:

Hi Olliver,

Thanks for the patches.
Adding driver authors on cc.

Ah sorry about that, thanks. I guess get_maintainers doesn't do that.

As for the compile bug, I'll fix that with v2, it only applies on the 
intermediate patches, not on the whole set.


Olliver


On 04/19/2016 09:40 AM, Olliver Schinagl wrote:
Using the pca963x for a while, I noticed something that may look like 
some
i2c accessing issues where sometimes data was incorrectly written to 
the bus,
possibly because we where not properly locking the i2c reads. Though 
I'm not
familiar enough with the i2c framework to be certain reads need to be 
locked

at all. A patch was added to properly lock i2c access more tightly.

Furthermore there was no method to support inverted outputs. This series
adds a property to the device tree to inform the driver that the output
is inverted (active-high vs active-low).

Additionally, this patch set does some cleanups to please checkpatch, 
and

removes a few magic values.

Olliver Schinagl (6):
   leds: pca963x: Alphabetize headers
   leds: pca963x: Lock i2c r/w access
   leds: pca963x: Add defines and remove some magic values
   leds: pca963x: Reduce magic values
   leds: pca963x: Inform the output that it is inverted
   leds: pca963x: Remove whitespace and checkpatch problems

  Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
  drivers/leds/leds-pca963x.c| 243 
++---

  include/linux/platform_data/leds-pca963x.h |   1 +
  3 files changed, 171 insertions(+), 74 deletions(-)

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Jacek Anaszewski]

Hi Olliver,

Thanks for the patches.
Adding driver authors on cc.

On 04/19/2016 09:40 AM, Olliver Schinagl wrote:

Using the pca963x for a while, I noticed something that may look like some
i2c accessing issues where sometimes data was incorrectly written to the bus,
possibly because we where not properly locking the i2c reads. Though I'm not
familiar enough with the i2c framework to be certain reads need to be locked
at all. A patch was added to properly lock i2c access more tightly.

Furthermore there was no method to support inverted outputs. This series
adds a property to the device tree to inform the driver that the output
is inverted (active-high vs active-low).

Additionally, this patch set does some cleanups to please checkpatch, and
removes a few magic values.

Olliver Schinagl (6):
   leds: pca963x: Alphabetize headers
   leds: pca963x: Lock i2c r/w access
   leds: pca963x: Add defines and remove some magic values
   leds: pca963x: Reduce magic values
   leds: pca963x: Inform the output that it is inverted
   leds: pca963x: Remove whitespace and checkpatch problems

  Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
  drivers/leds/leds-pca963x.c| 243 ++---
  include/linux/platform_data/leds-pca963x.h |   1 +
  3 files changed, 171 insertions(+), 74 deletions(-)




--
Best regards,
Jacek Anaszewski

Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Jacek Anaszewski]

Hi Olliver,

Thanks for the patches.
Adding driver authors on cc.

On 04/19/2016 09:40 AM, Olliver Schinagl wrote:

Using the pca963x for a while, I noticed something that may look like some
i2c accessing issues where sometimes data was incorrectly written to the bus,
possibly because we where not properly locking the i2c reads. Though I'm not
familiar enough with the i2c framework to be certain reads need to be locked
at all. A patch was added to properly lock i2c access more tightly.

Furthermore there was no method to support inverted outputs. This series
adds a property to the device tree to inform the driver that the output
is inverted (active-high vs active-low).

Additionally, this patch set does some cleanups to please checkpatch, and
removes a few magic values.

Olliver Schinagl (6):
   leds: pca963x: Alphabetize headers
   leds: pca963x: Lock i2c r/w access
   leds: pca963x: Add defines and remove some magic values
   leds: pca963x: Reduce magic values
   leds: pca963x: Inform the output that it is inverted
   leds: pca963x: Remove whitespace and checkpatch problems

  Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
  drivers/leds/leds-pca963x.c| 243 ++---
  include/linux/platform_data/leds-pca963x.h |   1 +
  3 files changed, 171 insertions(+), 74 deletions(-)




--
Best regards,
Jacek Anaszewski

[PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Using the pca963x for a while, I noticed something that may look like some
i2c accessing issues where sometimes data was incorrectly written to the bus,
possibly because we where not properly locking the i2c reads. Though I'm not
familiar enough with the i2c framework to be certain reads need to be locked
at all. A patch was added to properly lock i2c access more tightly.

Furthermore there was no method to support inverted outputs. This series
adds a property to the device tree to inform the driver that the output
is inverted (active-high vs active-low).

Additionally, this patch set does some cleanups to please checkpatch, and
removes a few magic values.

Olliver Schinagl (6):
  leds: pca963x: Alphabetize headers
  leds: pca963x: Lock i2c r/w access
  leds: pca963x: Add defines and remove some magic values
  leds: pca963x: Reduce magic values
  leds: pca963x: Inform the output that it is inverted
  leds: pca963x: Remove whitespace and checkpatch problems

 Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
 drivers/leds/leds-pca963x.c| 243 ++---
 include/linux/platform_data/leds-pca963x.h |   1 +
 3 files changed, 171 insertions(+), 74 deletions(-)

-- 
2.8.0.rc3

[PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

[Olliver Schinagl]

Using the pca963x for a while, I noticed something that may look like some
i2c accessing issues where sometimes data was incorrectly written to the bus,
possibly because we where not properly locking the i2c reads. Though I'm not
familiar enough with the i2c framework to be certain reads need to be locked
at all. A patch was added to properly lock i2c access more tightly.

Furthermore there was no method to support inverted outputs. This series
adds a property to the device tree to inform the driver that the output
is inverted (active-high vs active-low).

Additionally, this patch set does some cleanups to please checkpatch, and
removes a few magic values.

Olliver Schinagl (6):
  leds: pca963x: Alphabetize headers
  leds: pca963x: Lock i2c r/w access
  leds: pca963x: Add defines and remove some magic values
  leds: pca963x: Reduce magic values
  leds: pca963x: Inform the output that it is inverted
  leds: pca963x: Remove whitespace and checkpatch problems

 Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
 drivers/leds/leds-pca963x.c| 243 ++---
 include/linux/platform_data/leds-pca963x.h |   1 +
 3 files changed, 171 insertions(+), 74 deletions(-)

-- 
2.8.0.rc3