Hi David,

"David Rivshin (Allworx)" [off-list ref] hat am 29. Februar 2016
um 19:02 geschrieben:


On Sat, 27 Feb 2016 11:48:45 +0100 (CET)
Stefan Wahren [off-list ref] wrote:

quoted

Hi David,

quoted

"David Rivshin (Allworx)" [off-list ref] hat am 26. Februar
2016
um 22:58 geschrieben:


On Fri, 26 Feb 2016 10:47:46 +0100
Jacek Anaszewski [off-list ref] wrote:

quoted

On 02/25/2016 08:12 PM, David Rivshin (Allworx) wrote:

quoted

On Thu, 25 Feb 2016 11:55:58 +0100
Jacek Anaszewski [off-list ref] wrote:

quoted

On 02/25/2016 03:24 AM, David Rivshin (Allworx) wrote:

quoted

On Wed, 24 Feb 2016 17:04:58 +0100
Jacek Anaszewski [off-list ref] wrote:

quoted

Hi David,

Thanks for the patch. Very nice driver. I have few comments
below.

Thanks Jacek, I have responded the comments inline. I also wanted to
double check whether you noticed some questions I had in the cover
letter [1]. As I mentioned in another email to Rob, in hindsight I'm
guessing I should have included them in the patch comments as well
(or
instead of).

I saw them. I assumed that the review itself will address those
questions.

Fair enough, thanks for the confirmation.

quoted

quoted

Your review comments here effectively answered some of the
questions,
but
the big one I'm still unsure of is whether it actually makes sense
to
have all 4 of these devices supported by a single driver.

It's perfectly fine. Many drivers implement this pattern.

OK, then I'll assume you think this driver is not yet too complicated
for it's own good. Out of curiosity, might that view change if the
3216 specific features were ever implemented, especially GPIO and HW
animation support? Gut feel is that would make 3216 specific code
bigger than the rest of the code combined.

I don't think so.

Thanks, that helps calibrate my intuition for the future.

quoted

quoted

Bigger question is what should be done in terms of the overlap in
device
support between this driver and leds-sn3218? If you think I should
leave
the *3218 support in this driver, then I would propose:
- remove leds-sn3218 and its separate binding doc
- add the "si-en,sn3218" compatible string to this driver and binding
doc
Note that while I expect this driver to work with the 3218 chips, I do
not have one to test against. If we go down this route I would
definitely
want Stefan to test so that I don't accidentally break him.

I'd prefer to have a single driver for the same hardware. Stefan, would
it be possible for you to test David's driver with the hardware you
have an access to?

Stefan, one thing to note: the existing sn3218 driver/binding uses 0-based
'reg' values, and this driver/binding uses 1-based 'reg' values. So your
devicetree(s) would need to be updated for that (as well as the compatible
string).

I didn't see a final answer from Rob as to which way is most appropriate
for these devices yet, so I don't know which way this will end up in the
final patch.

unfortunately i'm very busy. Yes, i will test it, but i can't promise when.

Should i apply this version or wait for the next?

Thanks Stefan. I am hoping to have the next version ready in the next day or
so. To better use your time, it's probably best to wait for that.

i'm okay with replacing leds-sn3218. The pins of the SN3218 in the datasheet [1]
are 1-based.
Sorry, my fault.

[1] - http://www.si-en.com/uploadpdf/s2011517171720.pdf