Re: [Kernel] [PATCH v3 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver - OFF-LIST REPLY
Hi Andrey,
Am 13.07.2016 um 21:28 schrieb Andrey Utkin andrey_utkin@fastmail.com:
On Wed, Jul 13, 2016 at 10:18:16PM +0300, Andrey Utkin wrote:
I see that it turned out to having to revert max current code.
Yes.
Are you already working on it? If no, I may work on it again a bit later.
Yes, please go ahead. I won't find time soon and you had already implemented it. Just rewrite your branch (I haven't pulled or tested it yet).
Ok.
Done. Appended a commit to work/andrey/leds/is31fl319x: "Revert aggregated max current behaviour change"
Have tested a little. I think we have broken something. The LEDs remain dark.
One thing we also need to fix is the bindings (still talks about private led-max-current).
And I think about modifying
static int is31fl319x_microamp_to_cs(struct device *dev, u32 microamp) { /* invert and round down to nearest value (range check done by caller) */ const u32 max = IS31FL319X_CURRENT_MAX + IS31FL319X_CURRENT_DEFAULT + IS31FL319X_CURRENT_STEP - 1; return ((max - microamp) / IS31FL319X_CURRENT_STEP) % 8; }
The reason is if we really specify the max. LED current there should be no limit reported on it. If it is 3 mA it is 3 mA. If it is 100 mA it is 100 mA.
So the best effort the driver can do is to take the next lower available current limit.
Well, we still have a problem <5mA... And should probably turn off completely? Or in that case make probing fail.
I will have to take a deeper look what is wrong, but not before tomorrow morning.
BR, Nikolaus
On Thu, Jul 14, 2016 at 07:15:19PM +0200, H. Nikolaus Schaller wrote:
Have tested a little. I think we have broken something. The LEDs remain dark.
Oops.
One thing we also need to fix is the bindings (still talks about private led-max-current).
I don't understand what you mean here.
And I think about modifying
static int is31fl319x_microamp_to_cs(struct device *dev, u32 microamp) { /* invert and round down to nearest value (range check done by caller) */ const u32 max = IS31FL319X_CURRENT_MAX + IS31FL319X_CURRENT_DEFAULT + IS31FL319X_CURRENT_STEP - 1; return ((max - microamp) / IS31FL319X_CURRENT_STEP) % 8; }
Ok. But IMO CS register values map is so irregular that it requires documentation. I would add it in comment to this function.
The reason is if we really specify the max. LED current there should be no limit reported on it. If it is 3 mA it is 3 mA. If it is 100 mA it is 100 mA.
Again I don't understand what you want to say.
So the best effort the driver can do is to take the next lower available current limit.
This is currently done in line 195:
led->max_microamp -= led->max_microamp % IS31FL319X_CURRENT_STEP;
Well, we still have a problem <5mA... And should probably turn off completely? Or in that case make probing fail.
I'd leave it off, emitting a runtime warning.
I will have to take a deeper look what is wrong, but not before tomorrow morning.
I'd like to fix it myself, but just staring at diff feels not productive. It's hard to solve this telepathically. What about SSH access for me and USB webcam connected to Pyra itself and directed towards LEDs?
Hi,
Am 14.07.2016 um 23:55 schrieb Andrey Utkin andrey_utkin@fastmail.com:
On Thu, Jul 14, 2016 at 07:15:19PM +0200, H. Nikolaus Schaller wrote:
Have tested a little. I think we have broken something. The LEDs remain dark.
Oops.
One thing we also need to fix is the bindings (still talks about private led-max-current).
I don't understand what you mean here.
I will fix that. It is like a typo and fixing is easier than explaining. I just didn't have time yesterday to fix, commit and push it.
And I think about modifying
static int is31fl319x_microamp_to_cs(struct device *dev, u32 microamp) { /* invert and round down to nearest value (range check done by caller) */ const u32 max = IS31FL319X_CURRENT_MAX + IS31FL319X_CURRENT_DEFAULT + IS31FL319X_CURRENT_STEP - 1; return ((max - microamp) / IS31FL319X_CURRENT_STEP) % 8; }
Ok. But IMO CS register values map is so irregular that it requires documentation. I would add it in comment to this function.
Yes, indeed. I am not happy with as well how the formula looks like.
Maybe it is better to start with microamp / IS31FL319X_CURRENT_STEP (which is 0..n) and then apply the inversion trick i.e. (12-value)%8. And add a comment.
The reason is if we really specify the max. LED current there should be no limit reported on it. If it is 3 mA it is 3 mA. If it is 100 mA it is 100 mA.
Again I don't understand what you want to say.
So the best effort the driver can do is to take the next lower available current limit.
This is currently done in line 195:
led->max_microamp -= led->max_microamp % IS31FL319X_CURRENT_STEP;
but calculated for each LED. Could be done for all in total.
Well, we still have a problem <5mA... And should probably turn off completely? Or in that case make probing fail.
I'd leave it off, emitting a runtime warning.
probe failing is probably better since turning off all LEDs needs another flag because the brightness setter must know that after successful probing. And it shows a design issue which should not happen in practice.
I will have to take a deeper look what is wrong, but not before tomorrow morning.
I'd like to fix it myself, but just staring at diff feels not productive. It's hard to solve this telepathically. What about SSH access for me and USB webcam connected to Pyra itself and directed towards LEDs?
Well, setting this up is more complex for me than adding one or two printk and fixing it :) And I should at least look at dmesg or /sys/class/leds. Maybe the max_brightness remains 0.
So I missed to provide a good bug report.
BR, Nikolaus
Am 15.07.2016 um 06:38 schrieb H. Nikolaus Schaller hns@goldelico.com:
Hi,
Am 14.07.2016 um 23:55 schrieb Andrey Utkin andrey_utkin@fastmail.com:
On Thu, Jul 14, 2016 at 07:15:19PM +0200, H. Nikolaus Schaller wrote:
Have tested a little. I think we have broken something. The LEDs remain dark.
Oops.
One thing we also need to fix is the bindings (still talks about private led-max-current).
I don't understand what you mean here.
I will fix that. It is like a typo and fixing is easier than explaining. I just didn't have time yesterday to fix, commit and push it.
And I think about modifying
static int is31fl319x_microamp_to_cs(struct device *dev, u32 microamp) { /* invert and round down to nearest value (range check done by caller) */ const u32 max = IS31FL319X_CURRENT_MAX + IS31FL319X_CURRENT_DEFAULT + IS31FL319X_CURRENT_STEP - 1; return ((max - microamp) / IS31FL319X_CURRENT_STEP) % 8; }
Ok. But IMO CS register values map is so irregular that it requires documentation. I would add it in comment to this function.
Yes, indeed. I am not happy with as well how the formula looks like.
Maybe it is better to start with microamp / IS31FL319X_CURRENT_STEP (which is 0..n) and then apply the inversion trick i.e. (12-value)%8. And add a comment.
The reason is if we really specify the max. LED current there should be no limit reported on it. If it is 3 mA it is 3 mA. If it is 100 mA it is 100 mA.
Again I don't understand what you want to say.
So the best effort the driver can do is to take the next lower available current limit.
This is currently done in line 195:
led->max_microamp -= led->max_microamp % IS31FL319X_CURRENT_STEP;but calculated for each LED. Could be done for all in total.
Well, we still have a problem <5mA... And should probably turn off completely? Or in that case make probing fail.
I'd leave it off, emitting a runtime warning.
probe failing is probably better since turning off all LEDs needs another flag because the brightness setter must know that after successful probing. And it shows a design issue which should not happen in practice.
I will have to take a deeper look what is wrong, but not before tomorrow morning.
I'd like to fix it myself, but just staring at diff feels not productive. It's hard to solve this telepathically. What about SSH access for me and USB webcam connected to Pyra itself and directed towards LEDs?
Well, setting this up is more complex for me than adding one or two printk and fixing it :) And I should at least look at dmesg or /sys/class/leds. Maybe the max_brightness remains 0.
So I missed to provide a good bug report.
Problem:
root@letux:~# lsmod |fgrep is31root@letux:~# modprobe -c|fgrep is31 alias of:N*T*Cissi,is31fl3190 leds_is31fl319x alias of:N*T*Cissi,is31fl3190C* leds_is31fl319x alias of:N*T*Cissi,is31fl3191 leds_is31fl319x alias of:N*T*Cissi,is31fl3191C* leds_is31fl319x alias of:N*T*Cissi,is31fl3193 leds_is31fl319x alias of:N*T*Cissi,is31fl3193C* leds_is31fl319x alias of:N*T*Cissi,is31fl3196 leds_is31fl319x alias of:N*T*Cissi,is31fl3196C* leds_is31fl319x alias of:N*T*Cissi,is31fl3199 leds_is31fl319x alias of:N*T*Cissi,is31fl3199C* leds_is31fl319x alias of:N*T*Csi_en,sn3199 leds_is31fl319x alias of:N*T*Csi_en,sn3199C* leds_is31fl319x root@letux:~#
which means that chip matching/driver loading does no longer work because there is no i2c probe information any more (empty i2c table?).
Doing a manual modprobe makes it work.
I think we have to add back an explicty MODULE_ALIAS for i2c.
So it is questionable if Jacek did propose something good here. Or if the is31fl32 driver is a correct blueprint in that respect.
BR, Nikolaus
Am 15.07.2016 um 09:56 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 15.07.2016 um 06:38 schrieb H. Nikolaus Schaller hns@goldelico.com:
Hi,
Am 14.07.2016 um 23:55 schrieb Andrey Utkin andrey_utkin@fastmail.com:
On Thu, Jul 14, 2016 at 07:15:19PM +0200, H. Nikolaus Schaller wrote:
Have tested a little. I think we have broken something. The LEDs remain dark.
Oops.
One thing we also need to fix is the bindings (still talks about private led-max-current).
I don't understand what you mean here.
I will fix that. It is like a typo and fixing is easier than explaining. I just didn't have time yesterday to fix, commit and push it.
And I think about modifying
static int is31fl319x_microamp_to_cs(struct device *dev, u32 microamp) { /* invert and round down to nearest value (range check done by caller) */ const u32 max = IS31FL319X_CURRENT_MAX + IS31FL319X_CURRENT_DEFAULT + IS31FL319X_CURRENT_STEP - 1; return ((max - microamp) / IS31FL319X_CURRENT_STEP) % 8; }
Ok. But IMO CS register values map is so irregular that it requires documentation. I would add it in comment to this function.
Yes, indeed. I am not happy with as well how the formula looks like.
Maybe it is better to start with microamp / IS31FL319X_CURRENT_STEP (which is 0..n) and then apply the inversion trick i.e. (12-value)%8. And add a comment.
The reason is if we really specify the max. LED current there should be no limit reported on it. If it is 3 mA it is 3 mA. If it is 100 mA it is 100 mA.
Again I don't understand what you want to say.
So the best effort the driver can do is to take the next lower available current limit.
This is currently done in line 195:
led->max_microamp -= led->max_microamp % IS31FL319X_CURRENT_STEP;but calculated for each LED. Could be done for all in total.
Well, we still have a problem <5mA... And should probably turn off completely? Or in that case make probing fail.
I'd leave it off, emitting a runtime warning.
probe failing is probably better since turning off all LEDs needs another flag because the brightness setter must know that after successful probing. And it shows a design issue which should not happen in practice.
I will have to take a deeper look what is wrong, but not before tomorrow morning.
I'd like to fix it myself, but just staring at diff feels not productive. It's hard to solve this telepathically. What about SSH access for me and USB webcam connected to Pyra itself and directed towards LEDs?
Well, setting this up is more complex for me than adding one or two printk and fixing it :) And I should at least look at dmesg or /sys/class/leds. Maybe the max_brightness remains 0.
So I missed to provide a good bug report.
Problem:
root@letux:~# lsmod |fgrep is31root@letux:~# modprobe -c|fgrep is31 alias of:N*T*Cissi,is31fl3190 leds_is31fl319x alias of:N*T*Cissi,is31fl3190C* leds_is31fl319x alias of:N*T*Cissi,is31fl3191 leds_is31fl319x alias of:N*T*Cissi,is31fl3191C* leds_is31fl319x alias of:N*T*Cissi,is31fl3193 leds_is31fl319x alias of:N*T*Cissi,is31fl3193C* leds_is31fl319x alias of:N*T*Cissi,is31fl3196 leds_is31fl319x alias of:N*T*Cissi,is31fl3196C* leds_is31fl319x alias of:N*T*Cissi,is31fl3199 leds_is31fl319x alias of:N*T*Cissi,is31fl3199C* leds_is31fl319x alias of:N*T*Csi_en,sn3199 leds_is31fl319x alias of:N*T*Csi_en,sn3199C* leds_is31fl319x root@letux:~#
which means that chip matching/driver loading does no longer work because there is no i2c probe information any more (empty i2c table?).
Doing a manual modprobe makes it work.
I think we have to add back an explicty MODULE_ALIAS for i2c.
So it is questionable if Jacek did propose something good here. Or if the is31fl32 driver is a correct blueprint in that respect.
Here are my commits:
http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hn...
Just waiting for Jacek's response...
BR, Nikolaus
Am 15.07.2016 um 10:41 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 15.07.2016 um 09:56 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 15.07.2016 um 06:38 schrieb H. Nikolaus Schaller hns@goldelico.com:
Hi,
Am 14.07.2016 um 23:55 schrieb Andrey Utkin andrey_utkin@fastmail.com:
On Thu, Jul 14, 2016 at 07:15:19PM +0200, H. Nikolaus Schaller wrote:
Have tested a little. I think we have broken something. The LEDs remain dark.
Oops.
One thing we also need to fix is the bindings (still talks about private led-max-current).
I don't understand what you mean here.
I will fix that. It is like a typo and fixing is easier than explaining. I just didn't have time yesterday to fix, commit and push it.
And I think about modifying
static int is31fl319x_microamp_to_cs(struct device *dev, u32 microamp) { /* invert and round down to nearest value (range check done by caller) */ const u32 max = IS31FL319X_CURRENT_MAX + IS31FL319X_CURRENT_DEFAULT + IS31FL319X_CURRENT_STEP - 1; return ((max - microamp) / IS31FL319X_CURRENT_STEP) % 8; }
Ok. But IMO CS register values map is so irregular that it requires documentation. I would add it in comment to this function.
Yes, indeed. I am not happy with as well how the formula looks like.
Maybe it is better to start with microamp / IS31FL319X_CURRENT_STEP (which is 0..n) and then apply the inversion trick i.e. (12-value)%8. And add a comment.
The reason is if we really specify the max. LED current there should be no limit reported on it. If it is 3 mA it is 3 mA. If it is 100 mA it is 100 mA.
Again I don't understand what you want to say.
So the best effort the driver can do is to take the next lower available current limit.
This is currently done in line 195:
led->max_microamp -= led->max_microamp % IS31FL319X_CURRENT_STEP;but calculated for each LED. Could be done for all in total.
Well, we still have a problem <5mA... And should probably turn off completely? Or in that case make probing fail.
I'd leave it off, emitting a runtime warning.
probe failing is probably better since turning off all LEDs needs another flag because the brightness setter must know that after successful probing. And it shows a design issue which should not happen in practice.
I will have to take a deeper look what is wrong, but not before tomorrow morning.
I'd like to fix it myself, but just staring at diff feels not productive. It's hard to solve this telepathically. What about SSH access for me and USB webcam connected to Pyra itself and directed towards LEDs?
Well, setting this up is more complex for me than adding one or two printk and fixing it :) And I should at least look at dmesg or /sys/class/leds. Maybe the max_brightness remains 0.
So I missed to provide a good bug report.
Problem:
root@letux:~# lsmod |fgrep is31root@letux:~# modprobe -c|fgrep is31 alias of:N*T*Cissi,is31fl3190 leds_is31fl319x alias of:N*T*Cissi,is31fl3190C* leds_is31fl319x alias of:N*T*Cissi,is31fl3191 leds_is31fl319x alias of:N*T*Cissi,is31fl3191C* leds_is31fl319x alias of:N*T*Cissi,is31fl3193 leds_is31fl319x alias of:N*T*Cissi,is31fl3193C* leds_is31fl319x alias of:N*T*Cissi,is31fl3196 leds_is31fl319x alias of:N*T*Cissi,is31fl3196C* leds_is31fl319x alias of:N*T*Cissi,is31fl3199 leds_is31fl319x alias of:N*T*Cissi,is31fl3199C* leds_is31fl319x alias of:N*T*Csi_en,sn3199 leds_is31fl319x alias of:N*T*Csi_en,sn3199C* leds_is31fl319x root@letux:~#
which means that chip matching/driver loading does no longer work because there is no i2c probe information any more (empty i2c table?).
Doing a manual modprobe makes it work.
I think we have to add back an explicty MODULE_ALIAS for i2c.
So it is questionable if Jacek did propose something good here. Or if the is31fl32 driver is a correct blueprint in that respect.
Here are my commits:
http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hn...
Just waiting for Jacek's response...
Works after re-adding the i2c_device_table entries. So the is31fl32 driver seems to be broken in that area.
I have added two more patches for the is31fl32...
Should we post them in the same patch set or separately (since they are bug fixes for a different driver)?
BR, Nikolaus
On Fri, Jul 15, 2016 at 01:34:57PM +0200, H. Nikolaus Schaller wrote:
Works after re-adding the i2c_device_table entries. So the is31fl32 driver seems to be broken in that area.
Great. Well, without access to hardware I couldn't check this :)
I have added two more patches for the is31fl32...
Should we post them in the same patch set or separately (since they are bug fixes for a different driver)?
My vote is for two separate patches for that (so total count is 3).
I'd change the commit message of last one: - drop "drivers: ", - drop stress on "*complete*", - reword to avoid saying "driver.ko" as it looks strange, - modprobed -> probed to avoid kind of tautology.
Like this:
led: is31fl32xx: define complete i2c_device_id table
Otherwise the driver module will not be automatically probed.
Am 15.07.2016 um 14:21 schrieb Andrey Utkin andrey_utkin@fastmail.com:
On Fri, Jul 15, 2016 at 01:34:57PM +0200, H. Nikolaus Schaller wrote:
Works after re-adding the i2c_device_table entries. So the is31fl32 driver seems to be broken in that area.
Great. Well, without access to hardware I couldn't check this :)
This is why I thought it is easier if I do it for this step. Let's cross fingers that this was the last -v we have to submit...
I have added two more patches for the is31fl32...
Should we post them in the same patch set or separately (since they are bug fixes for a different driver)?
My vote is for two separate patches for that (so total count is 3).
I have put it into a separate feature branch now, so that it is easier to handle (post, modify, drop, cover-letter) that patch set:
http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hn...
I'd change the commit message of last one:
- drop "drivers: ",
- drop stress on "*complete*",
- reword to avoid saying "driver.ko" as it looks strange,
- modprobed -> probed to avoid kind of tautology.
For the is31fl31 driver this is just an internal comment since we will squeeze everything into v4.
Like this:
led: is31fl32xx: define complete i2c_device_id table
Otherwise the driver module will not be automatically probed.
Good! Done.
BR, Nikolaus
-
Andrey Utkin -
H. Nikolaus Schaller