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. -- To unsubscribe from this list: send the line "unsubscribe linux-leds" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
