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? Best regards -- 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
