From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH v3 1/2] dt-bindings: leds: Add binding for spi-byte LED. References: <20190505200022.32209-1-oss@c-mauderer.de> <20190506162848.GA9522@amd> <54199d69-67a9-eb9d-e46d-b3ea43e2e7a3@c-mauderer.de> <20190506202511.GA4979@amd> <82b27718-3a58-6692-02e4-41b45c16b81e@c-mauderer.de> From: Jacek Anaszewski Message-ID: <6fd9c537-1d89-0c8e-d3a9-f0e1f2ae1fdd@gmail.com> Date: Sat, 11 May 2019 11:11:31 +0200 MIME-Version: 1.0 In-Reply-To: <82b27718-3a58-6692-02e4-41b45c16b81e@c-mauderer.de> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit To: Christian Mauderer , Pavel Machek Cc: Rob Herring , Linux LED Subsystem , devicetree@vger.kernel.org, Dan Murphy , Mark Rutland List-ID: On 5/11/19 8:56 AM, Christian Mauderer wrote: > Hello Jacek, > > On 10/05/2019 22:42, Jacek Anaszewski wrote: >> Hi Christian, >> >> On 5/10/19 9:50 PM, Christian Mauderer wrote: >>> On 07/05/2019 11:52, Christian Mauderer wrote: >>>> On 06/05/2019 22:25, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>>>> Ok, I'm afraid I caused this. What should the compatible be, then? >>>>>>> >>>>>>> Knowing nothing about the h/w other than the above description: >>>>>>> ubiquiti,aircube-leds >>>>>>> >>>>>>> Not sure if that's a registered or correct vendor prefix though. >>>>>>> >>>>>>> Rob >>>>>>> >>>>>> >>>>>> Where would such a vendor prefix be registered? Does that mean that >>>>>> only >>>>>> the vendor is allowed to use it? In that case: How would a reverse >>>>>> engineered prefix look like? >>>>> >>>>> You can use it, too. It is in >>>>> Documentation/devicetree/bindings/vendor-prefixes.txt : >>>>> >>>>> ubnt��� Ubiquiti Networks >>>>> >>>>> So you can probably use ubnt, prefix. >>>>> >>>>>> (still with some missing parts like U-Boot) about two weeks later. >>>>>> I had >>>>>> a look at it and they are not using a device tree. So there is no >>>>>> "official" string that I could deduce from that archive. >>>>> >>>>> Mainline is the master. You are more "official" than them ;-). >>>>> ����������������������������������� Pavel >>>>> >>>> >>>> Hello >>>> >>>> let me summarize the direction before I create a v4: >>>> >>>> Rob Herring suggested "ubnt,acb-spi-led" for the binding name in his >>>> Mail from 06.05.2019 17:59 UTC. If no one objects, I'll use that. >>>> >>>> With the more specific name I'll remove the off-value and max-value from >>>> the device tree. Instead I'll create some look up table in the driver. >>>> based on the name or go back to the defines like in the v1 patch. What >>>> kind of solution would be preferable depends on the next question: >>>> >>>> How should I name the driver? Should I use a device specific name like >>>> in v1 again (most likely now acb-spi-led)? That would allow to >>>> potentially add a hardware supported blinking in that driver. The >>>> alternative would be the more generic name that it has now >>>> (leds-spi-byte) without any plans to add the blinking but it could be >>>> potentially used for example for a digital potentiometer based >>>> brightness setting. >>>> >>>> Note that I didn't really had planned to implement the blinking support >>>> because I don't have a use case for it. So it would be either a feature >>>> that I would add because someone insists. Or it could be added in the >>>> future by a user who wants that feature (maybe Ubiquiti when they >>>> upgrade their kernel?). >>>> >>>> If it is a required feature for that driver: Please note that although >>>> of course I would do some basic tests during development it would be a >>>> mostly unused and therefore untested feature. >>>> >>>> Best regards >>>> >>>> Christian >>>> >>> >>> Hello, >>> >>> sorry for repeating my question. I assume I wrote to much text hiding >>> it: How should I name the driver? >>> >>> The name for the binding is clear (ubnt,acb-spi-led). Only the driver is >>> left (keep leds-spi-byte or rename to leds-ubnt-acb-spi or something >>> else). >> >> Why leds-spi-byte name would prevent addition of blink support? It can >> be always added basing on OF compatible. If it is to be generic SPI >> byte driver, then I'd use leds-spi-byte. Actually also the things >> like allowed brightness levels could be determined basing on that, >> and not in device tree, but in the driver. >> >> Please compare how e.g. drivers/leds/leds-is31fl32xx.c accomplishes >> that. >> > > I would have expected that adding a lot of device specific code (in that > case blinking) to a multi-purpose driver would be bad style. This is a matter of how much customizable the driver is going to be. It can serve as a nice code base for other devices with simple SPI-based protocol. > But I'll go > for the generic name if that is the accepted way. I already mentioned > multiple times that my target is currently only the brightness. So the > device specific code maybe is added quite a bit in the future anyway in > which case it would still be possible to rename a part (if it isn't used > otherwise) or at least split it into it's own c-file. > > I'll prepare a v4 in the near future and send it to the list. I only > learned that it would be a good idea to wait for at least a day for some > other opinions before doing that ;-) Sure thing. The fact that I'm replying to your message doesn't mean I'm expecting your immediate reaction :-) -- Best regards, Jacek Anaszewski