| Message ID | 20220125092239.2006333-1-sven@svenschwermer.de |
|---|---|
| Headers | show |
| Series | Multicolor PWM LED support | expand |
Hi Sven, On 1/25/22 10:22 AM, sven@svenschwermer.de wrote: > From: Sven Schwermer <sven.schwermer@disruptive-technologies.com> > > Hi, > > As previously discussed [1] on the linux-leds list I am missing > multicolor PWM LED support. In the mean time I have put together a > working prototype for such a driver. This is my first Linux driver > so I'm hoping for some feedback. Here are some questions that came up > while putting this thing together: > > 1. Currently, the max-brightness property is expected as a property to > the multi-led node. That seems consistent with the existing > multicolor class code, but I'm wondering whether it would make > sense to have a max-brigthness for the individual LEDs as well? For the proper mixed color calculation all sub-leds should have the same max_brightness as the global max_brightness. Look at how sub-led intensities are calculated in led_mc_calc_color_components(). See also [0] and [1]. > 2. The current multi-led node definition calls for a node index which > would in turn require the reg property to be set within the node. > In this context, that doesn't seem to make sense. Should this > requirement be lifted from leds-class-multicolor.yaml? reg is required for all DT nodes with address unit in the name. If you skipped the address unit, then reg would be also not required. > 3. I'm not currently reusing any leds-pwm code because there aren't > too many overlaps. Does anyone have suggestions what could be > factored out into a common source file? I think that having a separate pwm driver for multicolor LEDs is a good idea. leds-pwm.c is old and well tested driver, there's no need to tinker at it for no vital reason. And there is not much code to share as you've noticed. > I would appreciate if anyone would test this code. It runs on my > i.MX6ULL-based hardware. > > Best regards, > Sven > > [1]: https://www.spinics.net/lists/linux-leds/msg19988.html > > Sven Schwermer (2): > dt-bindings: leds: Add multicolor PWM LED bindings > leds: Add PWM multicolor driver > > .../bindings/leds/leds-pwm-multicolor.yaml | 73 +++++++ > drivers/leds/Kconfig | 8 + > drivers/leds/Makefile | 1 + > drivers/leds/leds-pwm-multicolor.c | 184 ++++++++++++++++++ > 4 files changed, 266 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml > create mode 100644 drivers/leds/leds-pwm-multicolor.c > [0] Documentation/ABI/testing/sysfs-class-led-multicolor [1] Documentation/leds/leds-class-multicolor.rst
Hi Jacek, Thank you for your feedback! On 1/25/22 23:31, Jacek Anaszewski wrote: >> 1. Currently, the max-brightness property is expected as a property to >> the multi-led node. That seems consistent with the existing >> multicolor class code, but I'm wondering whether it would make >> sense to have a max-brigthness for the individual LEDs as well? > > For the proper mixed color calculation all sub-leds should have > the same max_brightness as the global max_brightness. > > Look at how sub-led intensities are calculated in > led_mc_calc_color_components(). > > See also [0] and [1]. OK, thanks. That makes sense. >> 2. The current multi-led node definition calls for a node index which >> would in turn require the reg property to be set within the node. >> In this context, that doesn't seem to make sense. Should this >> requirement be lifted from leds-class-multicolor.yaml? > > reg is required for all DT nodes with address unit in the name. > If you skipped the address unit, then reg would be also not required. Yes, I realize this. However, leds-class-multicolor.yaml [0] requires the address unit: "^multi-led@([0-9a-f])$" Best regards, Sven [0] Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
Hello Sven, Am Tue, Jan 25, 2022 at 10:22:37AM +0100 schrieb sven@svenschwermer.de: > From: Sven Schwermer <sven.schwermer@disruptive-technologies.com> > As previously discussed [1] on the linux-leds list I am missing > multicolor PWM LED support. In the mean time I have put together a > working prototype for such a driver. This is my first Linux driver > so I'm hoping for some feedback. Here are some questions that came up > while putting this thing together: Wow, I did not expect this so quickly. Thank you. :-) > 1. Currently, the max-brightness property is expected as a property to > the multi-led node. That seems consistent with the existing > multicolor class code, but I'm wondering whether it would make > sense to have a max-brigthness for the individual LEDs as well? > 2. The current multi-led node definition calls for a node index which > would in turn require the reg property to be set within the node. > In this context, that doesn't seem to make sense. Should this > requirement be lifted from leds-class-multicolor.yaml? > 3. I'm not currently reusing any leds-pwm code because there aren't > too many overlaps. Does anyone have suggestions what could be > factored out into a common source file? > > I would appreciate if anyone would test this code. It runs on my > i.MX6ULL-based hardware. > > Best regards, > Sven > > [1]: https://www.spinics.net/lists/linux-leds/msg19988.html You can use the lore.kernel.org archive, e.g. in this case: https://lore.kernel.org/linux-leds/b48eed49-a18e-eed1-f1f4-77b9f1eab39b@gmail.com/T/#t Greets Alex > > Sven Schwermer (2): > dt-bindings: leds: Add multicolor PWM LED bindings > leds: Add PWM multicolor driver > > .../bindings/leds/leds-pwm-multicolor.yaml | 73 +++++++ > drivers/leds/Kconfig | 8 + > drivers/leds/Makefile | 1 + > drivers/leds/leds-pwm-multicolor.c | 184 ++++++++++++++++++ > 4 files changed, 266 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml > create mode 100644 drivers/leds/leds-pwm-multicolor.c > > -- > 2.35.0 >
Hi Sven, On 1/26/22 8:51 AM, Sven Schwermer wrote: > Hi Jacek, > > Thank you for your feedback! > > On 1/25/22 23:31, Jacek Anaszewski wrote: > >>> 1. Currently, the max-brightness property is expected as a >>> property to >>> the multi-led node. That seems consistent with the existing >>> multicolor class code, but I'm wondering whether it would make >>> sense to have a max-brigthness for the individual LEDs as well? >> >> For the proper mixed color calculation all sub-leds should have >> the same max_brightness as the global max_brightness. >> >> Look at how sub-led intensities are calculated in >> led_mc_calc_color_components(). >> >> See also [0] and [1]. > > OK, thanks. That makes sense. > >>> 2. The current multi-led node definition calls for a node index which >>> would in turn require the reg property to be set within the node. >>> In this context, that doesn't seem to make sense. Should this >>> requirement be lifted from leds-class-multicolor.yaml? >> >> reg is required for all DT nodes with address unit in the name. >> If you skipped the address unit, then reg would be also not required. > > Yes, I realize this. However, leds-class-multicolor.yaml [0] requires > the address unit: "^multi-led@([0-9a-f])$" This is only an example and nothing prevents you from dropping address unit in leds-pwm-multicolor DT bindings. We don't have common DT parser for multicolor LEDs and it will be hard to come up with something that will fit neatly for all possible LED controllers anyway. Dropping address unit from leds-class-multicolor.yaml would be too much since it is useful in some cases, see e.g. [2]. [2] Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
From: Sven Schwermer <sven.schwermer@disruptive-technologies.com> Hi, As previously discussed [1] on the linux-leds list I am missing multicolor PWM LED support. In the mean time I have put together a working prototype for such a driver. This is my first Linux driver so I'm hoping for some feedback. Here are some questions that came up while putting this thing together: 1. Currently, the max-brightness property is expected as a property to the multi-led node. That seems consistent with the existing multicolor class code, but I'm wondering whether it would make sense to have a max-brigthness for the individual LEDs as well? 2. The current multi-led node definition calls for a node index which would in turn require the reg property to be set within the node. In this context, that doesn't seem to make sense. Should this requirement be lifted from leds-class-multicolor.yaml? 3. I'm not currently reusing any leds-pwm code because there aren't too many overlaps. Does anyone have suggestions what could be factored out into a common source file? I would appreciate if anyone would test this code. It runs on my i.MX6ULL-based hardware. Best regards, Sven [1]: https://www.spinics.net/lists/linux-leds/msg19988.html Sven Schwermer (2): dt-bindings: leds: Add multicolor PWM LED bindings leds: Add PWM multicolor driver .../bindings/leds/leds-pwm-multicolor.yaml | 73 +++++++ drivers/leds/Kconfig | 8 + drivers/leds/Makefile | 1 + drivers/leds/leds-pwm-multicolor.c | 184 ++++++++++++++++++ 4 files changed, 266 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm-multicolor.yaml create mode 100644 drivers/leds/leds-pwm-multicolor.c