HI,
Am 08.07.2016 um 22:35 schrieb Andrey Utkin andrey_utkin@fastmail.com:
On Fri, Jul 08, 2016 at 08:25:25PM +0200, H. Nikolaus Schaller wrote:
Seems to work with no noticeable differences. Maybe because we are not using microcampere settings at all. So I can't test bugs here...
Generally I like the approach to pass the struct is31fl319x_led into the parse_dt().
One thing I don't understand exactly is why you did replace the formula by a switch in is31fl319x_microamp_to_cs. The formula is designed to accept any value and never report "invalid microamperes" but simply round down to the next available value.
Well, now I see I have underestimated your implementation. The expression
(64000 - aggregated_led_microamp) / 5000 & 0x7
feels wrong in many ways to me. In fact, it was just misleading and counter-intuitive. It must be my lack of mathematical capabilities, but it was hard for me to figure out that your formula works correctly. I didn't even consider such outcome that the formula is right. I went straight to rewriting it. Ridiculous :)
Well, 64000 blows the mind. It is not dividable by 5000, it's outside of the working range.
Then, I assumed that & 0x7 (with subseq. << 4) is just a safety capping, so that when you write to config2 you declare that you touch only bits 4..6. Turns out I was wrong and this masking is a part of arithmetical formula.
Then, a quick look at datasheet reveals that CS values for microamperes don't grow linearly: 0 => 20000, 3 => 5000, 7 => 25000. I understood that this could be turned to arithmetic formula but since the working range is so short, I decided to go fully hardcoded to save some trial and error (which I do all the time and of which I'm tired of).
So let's tabulate your function:
(64000 - aggregated_led_microamp) / 5000
$ for microamp in `seq 5000 5000 40000`; do EXPR="(64000 - $microamp) / 5000 % 8"; echo "$EXPR = `echo $EXPR | bc`"; done (64000 - 5000) / 5000 % 8 = 3 (64000 - 10000) / 5000 % 8 = 2 (64000 - 15000) / 5000 % 8 = 1 (64000 - 20000) / 5000 % 8 = 0 (64000 - 25000) / 5000 % 8 = 7 (64000 - 30000) / 5000 % 8 = 6 (64000 - 35000) / 5000 % 8 = 5 (64000 - 40000) / 5000 % 8 = 4
So feel free to drop the patch which replaces it, and also drop the line added in previous patch "/* TODO FIXME calculation seems wrong */"
Since we don't use it for the Pyra (and maybe never will need it), I don't really care how this functionality will evolve in the future...
Then what's the device it's present in? GTA04?
No, Pyra. But we don't use the microamps settings because the LEDs have series resistors so any (default) value is ok...
Another minor thing is that I think we should also add IS31FL319X_CTRL1 and IS31FL319X_CTRL2 to the reg_default setup.
Fine, I just thought those need to be actually written to device at that stage, so didn't replace these occurances. It felt so from conversation at review.
Yes, they talked about the loop, but these registers are also reset to 0x00.
I will add that myself, squeeze everything into the original patches, add a description of your changes to the patch 0/n message and resubmit.
Fine.
BR, Nikolaus