On Mon, 29 Feb 2016 20:40:49 +0100 (CET) Stefan Wahren <stefan.wahren@xxxxxxxx> wrote: > Hi David, > > > "David Rivshin (Allworx)" <drivshin.allworx@xxxxxxxxx> hat am 29. Februar 2016 > > um 19:02 geschrieben: > > > > > > On Sat, 27 Feb 2016 11:48:45 +0100 (CET) > > Stefan Wahren <stefan.wahren@xxxxxxxx> wrote: > > > > > Hi David, > > > > > > > "David Rivshin (Allworx)" <drivshin.allworx@xxxxxxxxx> hat am 26. Februar > > > > 2016 > > > > um 22:58 geschrieben: > > > > > > > > > > > > On Fri, 26 Feb 2016 10:47:46 +0100 > > > > Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx> wrote: > > > > > > > > > On 02/25/2016 08:12 PM, David Rivshin (Allworx) wrote: > > > > > > On Thu, 25 Feb 2016 11:55:58 +0100 > > > > > > Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx> wrote: > > > > > > > > > > > >> On 02/25/2016 03:24 AM, David Rivshin (Allworx) wrote: > > > > > >>> On Wed, 24 Feb 2016 17:04:58 +0100 > > > > > >>> Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx> wrote: > > > > > >>> > > > > > >>>> 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. > > > > > > > > > > > >>> 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. > > > > > > > > > > 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. No worries, I was quite unsure whether to do the 'reg' devicetree property as 1-based or 0-based myself. I went with 1-based originally mostly because it made writing devicetrees an easier match with the schematics I have. FYI, I don't quite have the next version ready to post yet, but I hope to do it tomorrow (assuming I can steal enough time to retest). > > [1] - http://www.si-en.com/uploadpdf/s2011517171720.pdf -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
