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?
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.
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.