| Message ID | 1440502442-19531-4-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tomi, On 08/25/2015 01:34 PM, Tomi Valkeinen wrote: > Add DT binding for led-backlight. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > .../bindings/video/backlight/led-backlight.txt | 30 ++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt > > diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt > new file mode 100644 > index 000000000000..fb77051ac230 > --- /dev/null > +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt > @@ -0,0 +1,30 @@ > +led-backlight bindings > + > +Required properties: > + - compatible: "led-backlight" > + - leds: phandle to a led OF node [0] > + - brightness-levels: Array of distinct LED brightness levels. These > + are in the range from 0 to 255, passed to the LED class driver. > + - default-brightness-level: the default brightness level (index into the > + array defined by the "brightness-levels" property) > + - power-supply: regulator for supply voltage > + > +Optional properties: > + - enable-gpios: contains a single GPIO specifier for the GPIO which enables > + and disables the backlight (see GPIO binding[1]) > + > +[0]: Documentation/devicetree/bindings/leds/common.txt > +[1]: Documentation/devicetree/bindings/gpio/gpio.txt > + > +Example: > + > + backlight { > + compatible = "led-backlight"; > + leds = <&backlight_led>; > + > + brightness-levels = <0 4 8 16 32 64 128 255>; brightness level is not a suitable unit for describing LED brightness in a Device Tree, as it is not a physical unit. We have led-max-microamp property for this, expressed in microamperes, please refer to [0] from linux-next. > + default-brightness-level = <6>; This also should be microamperes. > + power-supply = <&vdd_bl_reg>; > + enable-gpios = <&gpio 58 0>; > + }; >
On 25/08/15 16:39, Jacek Anaszewski wrote: >> +Example: >> + >> + backlight { >> + compatible = "led-backlight"; >> + leds = <&backlight_led>; >> + >> + brightness-levels = <0 4 8 16 32 64 128 255>; > > brightness level is not a suitable unit for describing LED brightness > in a Device Tree, as it is not a physical unit. We have led-max-microamp > property for this, expressed in microamperes, please refer to [0] from > linux-next. Hmm, ok, but what should the driver do with microamperes? As far as I see, "enum led_brightness" (which is between 0-255) is used to set the brightness to LEDs. I don't see any function accepting microamperes. >> + default-brightness-level = <6>; > > This also should be microamperes. This is an index to the above brightness-levels array. It's not LED brightness, but backlight brightness, between 0 and ARRAY_SIZE(brightness-levels) - 1. Tomi
On 08/25/2015 05:41 PM, Tomi Valkeinen wrote: > > > On 25/08/15 16:39, Jacek Anaszewski wrote: > >>> +Example: >>> + >>> + backlight { >>> + compatible = "led-backlight"; >>> + leds = <&backlight_led>; >>> + >>> + brightness-levels = <0 4 8 16 32 64 128 255>; >> >> brightness level is not a suitable unit for describing LED brightness >> in a Device Tree, as it is not a physical unit. We have led-max-microamp >> property for this, expressed in microamperes, please refer to [0] from >> linux-next. > > Hmm, ok, but what should the driver do with microamperes? As far as I > see, "enum led_brightness" (which is between 0-255) is used to set the > brightness to LEDs. I don't see any function accepting microamperes. This is implementation detail. You can convert microamperes to enum led_brightness in the driver. Please refer to the discussion [1]. >>> + default-brightness-level = <6>; >> >> This also should be microamperes. > > This is an index to the above brightness-levels array. It's not LED > brightness, but backlight brightness, between 0 and > ARRAY_SIZE(brightness-levels) - 1. You could skip "-level" postfix and have default brightness in microamperes. [1] http://www.spinics.net/lists/linux-leds/msg03416.html
On 26/08/15 10:07, Jacek Anaszewski wrote: > On 08/25/2015 05:41 PM, Tomi Valkeinen wrote: >> >> >> On 25/08/15 16:39, Jacek Anaszewski wrote: >> >>>> +Example: >>>> + >>>> + backlight { >>>> + compatible = "led-backlight"; >>>> + leds = <&backlight_led>; >>>> + >>>> + brightness-levels = <0 4 8 16 32 64 128 255>; >>> >>> brightness level is not a suitable unit for describing LED brightness >>> in a Device Tree, as it is not a physical unit. We have led-max-microamp >>> property for this, expressed in microamperes, please refer to [0] from >>> linux-next. >> >> Hmm, ok, but what should the driver do with microamperes? As far as I >> see, "enum led_brightness" (which is between 0-255) is used to set the >> brightness to LEDs. I don't see any function accepting microamperes. > > This is implementation detail. You can convert microamperes to > enum led_brightness in the driver. Please refer to the discussion [1]. The led_set_brightness() takes "enum led_brightness", so I don't understand what this driver would do with the microampere value. It could, of course, do an arbitrary conversion, say, direct mapping of the mA value to brightness, but that would just confuse things further. If there was a led_set_microampere(), supported by all led devices, then yes, the above should be microamperes. Or are you saying that I should have the binding use microamperes, and just do a naive microampere -> brightness conversion in this driver, and hope that in the future this driver can be changes to pass the micrompere value directly to the underlying LED driver? And speaking of microamperes for LED drivers, what's a meaningful microampere conversion for LED drivers that don't deal with microamperes at all? For example, the LED chip we're using, TLC59108, is basically a PWM chip. Can we somehow calculate microamperes if we know duty cycle and period? Tomi
On 08/26/2015 11:11 AM, Tomi Valkeinen wrote: > > > On 26/08/15 10:07, Jacek Anaszewski wrote: >> On 08/25/2015 05:41 PM, Tomi Valkeinen wrote: >>> >>> >>> On 25/08/15 16:39, Jacek Anaszewski wrote: >>> >>>>> +Example: >>>>> + >>>>> + backlight { >>>>> + compatible = "led-backlight"; >>>>> + leds = <&backlight_led>; >>>>> + >>>>> + brightness-levels = <0 4 8 16 32 64 128 255>; >>>> >>>> brightness level is not a suitable unit for describing LED brightness >>>> in a Device Tree, as it is not a physical unit. We have led-max-microamp >>>> property for this, expressed in microamperes, please refer to [0] from >>>> linux-next. >>> >>> Hmm, ok, but what should the driver do with microamperes? As far as I >>> see, "enum led_brightness" (which is between 0-255) is used to set the >>> brightness to LEDs. I don't see any function accepting microamperes. >> >> This is implementation detail. You can convert microamperes to >> enum led_brightness in the driver. Please refer to the discussion [1]. > > The led_set_brightness() takes "enum led_brightness", so I don't > understand what this driver would do with the microampere value. It > could, of course, do an arbitrary conversion, say, direct mapping of the > mA value to brightness, but that would just confuse things further. OK, I was looking at the problem from LED-centric perspective. Indeed, backlight subsystem has no other way to pass brightness to the LED subsystem than in the form of levels. However, the last word belongs to DT maintainer in this matter. Cc'ing devicetree@vger.kernel.org. > If there was a led_set_microampere(), supported by all led devices, then > yes, the above should be microamperes. > > Or are you saying that I should have the binding use microamperes, and > just do a naive microampere -> brightness conversion in this driver, and > hope that in the future this driver can be changes to pass the > micrompere value directly to the underlying LED driver? > > And speaking of microamperes for LED drivers, what's a meaningful > microampere conversion for LED drivers that don't deal with microamperes > at all? For example, the LED chip we're using, TLC59108, is basically a > PWM chip. Can we somehow calculate microamperes if we know duty cycle > and period? For this type of drivers we have to resort to levels in DT. It shouldn't serve as an excuse for not describing the rest of hardware in a better way, though.
On Wed, Aug 26, 2015 at 4:56 AM, Jacek Anaszewski <j.anaszewski@samsung.com> wrote: > On 08/26/2015 11:11 AM, Tomi Valkeinen wrote: >> >> >> >> On 26/08/15 10:07, Jacek Anaszewski wrote: >>> >>> On 08/25/2015 05:41 PM, Tomi Valkeinen wrote: >>>> >>>> >>>> >>>> On 25/08/15 16:39, Jacek Anaszewski wrote: >>>> >>>>>> +Example: >>>>>> + >>>>>> + backlight { >>>>>> + compatible = "led-backlight"; >>>>>> + leds = <&backlight_led>; >>>>>> + >>>>>> + brightness-levels = <0 4 8 16 32 64 128 255>; >>>>> >>>>> >>>>> brightness level is not a suitable unit for describing LED brightness >>>>> in a Device Tree, as it is not a physical unit. We have >>>>> led-max-microamp >>>>> property for this, expressed in microamperes, please refer to [0] from >>>>> linux-next. >>>> >>>> >>>> Hmm, ok, but what should the driver do with microamperes? As far as I >>>> see, "enum led_brightness" (which is between 0-255) is used to set the >>>> brightness to LEDs. I don't see any function accepting microamperes. >>> >>> >>> This is implementation detail. You can convert microamperes to >>> enum led_brightness in the driver. Please refer to the discussion [1]. >> >> >> The led_set_brightness() takes "enum led_brightness", so I don't >> understand what this driver would do with the microampere value. It >> could, of course, do an arbitrary conversion, say, direct mapping of the >> mA value to brightness, but that would just confuse things further. > > > OK, I was looking at the problem from LED-centric perspective. Indeed, > backlight subsystem has no other way to pass brightness to the LED > subsystem than in the form of levels. However, the last word belongs > to DT maintainer in this matter. > > Cc'ing devicetree@vger.kernel.org. I don't have a simple answer for you... There was a similar discussion for pm8941-wled and "default-brightness-level" units[1]. The conclusion was it should be units matching the h/w so that there is no conversion between bootloader and OS units to h/w units. That principle probably applies here. If the brightness levels are non-linear, then you need a translation from percent to h/w level. What's needed here for h/w levels depends on whether the brightness control is PWM, current control or both. For PWM, units of the PWM control makes sense. For current control, units of microamps probably makes sense. I don't know what you do with both, but I have seen that h/w (FSL PMICs). This all certainly needs some more work on defining some common binding. We already have some bindings for backlights with the LED and PWM bindings (perhaps incomplete?). Do we need another way here? This also introduces possibility of multiple ways to define GPIO controlled backlights: gpio -> gpio-leds -> led-backlight or gpio -> gpio-backlight. We don't want that... This problem is not really specific at all to backlights, but applies to all LEDs. Some LEDs you may not have control beyond on/off or really care about fine-grained control of level, but they are really no different. The main unique thing about backlights is what display are they associated with. Rob [1] https://lkml.org/lkml/2015/7/30/542 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/01/2015 01:12 AM, Rob Herring wrote: > On Wed, Aug 26, 2015 at 4:56 AM, Jacek Anaszewski > <j.anaszewski@samsung.com> wrote: >> On 08/26/2015 11:11 AM, Tomi Valkeinen wrote: >>> >>> >>> >>> On 26/08/15 10:07, Jacek Anaszewski wrote: >>>> >>>> On 08/25/2015 05:41 PM, Tomi Valkeinen wrote: >>>>> >>>>> >>>>> >>>>> On 25/08/15 16:39, Jacek Anaszewski wrote: >>>>> >>>>>>> +Example: >>>>>>> + >>>>>>> + backlight { >>>>>>> + compatible = "led-backlight"; >>>>>>> + leds = <&backlight_led>; >>>>>>> + >>>>>>> + brightness-levels = <0 4 8 16 32 64 128 255>; >>>>>> >>>>>> >>>>>> brightness level is not a suitable unit for describing LED brightness >>>>>> in a Device Tree, as it is not a physical unit. We have >>>>>> led-max-microamp >>>>>> property for this, expressed in microamperes, please refer to [0] from >>>>>> linux-next. >>>>> >>>>> >>>>> Hmm, ok, but what should the driver do with microamperes? As far as I >>>>> see, "enum led_brightness" (which is between 0-255) is used to set the >>>>> brightness to LEDs. I don't see any function accepting microamperes. >>>> >>>> >>>> This is implementation detail. You can convert microamperes to >>>> enum led_brightness in the driver. Please refer to the discussion [1]. >>> >>> >>> The led_set_brightness() takes "enum led_brightness", so I don't >>> understand what this driver would do with the microampere value. It >>> could, of course, do an arbitrary conversion, say, direct mapping of the >>> mA value to brightness, but that would just confuse things further. >> >> >> OK, I was looking at the problem from LED-centric perspective. Indeed, >> backlight subsystem has no other way to pass brightness to the LED >> subsystem than in the form of levels. However, the last word belongs >> to DT maintainer in this matter. >> >> Cc'ing devicetree@vger.kernel.org. > > I don't have a simple answer for you... > > There was a similar discussion for pm8941-wled and > "default-brightness-level" units[1]. The conclusion was it should be > units matching the h/w so that there is no conversion between > bootloader and OS units to h/w units. That principle probably applies > here. > > If the brightness levels are non-linear, then you need a translation > from percent to h/w level. What's needed here for h/w levels depends > on whether the brightness control is PWM, current control or both. For > PWM, units of the PWM control makes sense. For current control, units > of microamps probably makes sense. I don't know what you do with both, > but I have seen that h/w (FSL PMICs). > > This all certainly needs some more work on defining some common > binding. We already have some bindings for backlights with the LED and > PWM bindings (perhaps incomplete?). Do we need another way here? This > also introduces possibility of multiple ways to define GPIO controlled > backlights: gpio -> gpio-leds -> led-backlight or gpio -> > gpio-backlight. We don't want that... > > This problem is not really specific at all to backlights, but applies > to all LEDs. Some LEDs you may not have control beyond on/off or > really care about fine-grained control of level, but they are really > no different. The main unique thing about backlights is what display > are they associated with. Some time ago I was trying to add common LEDs DT properties that would have defined brightness in levels, but other people insisted on microamperes [1]. The discussion was focused on flash LEDs, where currents are significantly greater and there is risk of hardware damage in case underrated LED is connected [2]. Having the property in microamperes removes the need for consulting documentation to check current value for given brightness level. As it was mentioned earlier in this thread, this approach is not suitable for PWM driven leds, and they have their own bindings, which allow for describing brightness in levels. Probably we should add some note to the common LEDs DT bindings that would state explicitly that such exceptions are allowed. In case of backlights which are built upon LED class devices we could stick to brightness levels, as a LED class driver will assert brightness level to the leds-max-microamp value anyway. [1] http://www.spinics.net/lists/linux-leds/msg03416.html [2] http://patchwork.ozlabs.org/patch/456622/
diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt new file mode 100644 index 000000000000..fb77051ac230 --- /dev/null +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt @@ -0,0 +1,30 @@ +led-backlight bindings + +Required properties: + - compatible: "led-backlight" + - leds: phandle to a led OF node [0] + - brightness-levels: Array of distinct LED brightness levels. These + are in the range from 0 to 255, passed to the LED class driver. + - default-brightness-level: the default brightness level (index into the + array defined by the "brightness-levels" property) + - power-supply: regulator for supply voltage + +Optional properties: + - enable-gpios: contains a single GPIO specifier for the GPIO which enables + and disables the backlight (see GPIO binding[1]) + +[0]: Documentation/devicetree/bindings/leds/common.txt +[1]: Documentation/devicetree/bindings/gpio/gpio.txt + +Example: + + backlight { + compatible = "led-backlight"; + leds = <&backlight_led>; + + brightness-levels = <0 4 8 16 32 64 128 255>; + default-brightness-level = <6>; + + power-supply = <&vdd_bl_reg>; + enable-gpios = <&gpio 58 0>; + };
Add DT binding for led-backlight. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- .../bindings/video/backlight/led-backlight.txt | 30 ++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt