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