by Pavel Machek

[permalink] [raw]

Subject: Re: [PATCHv4 08/10] backlight: add TI LMU backlight driver

Hi!

> +enum ti_lmu_bl_type {
> + TI_LMU_BL, /* backlight userspace interface */
> + TI_LMU_LED, /* led userspace interface */
> +};
...
> +static int ti_lmu_bl_add_device(struct ti_lmu_bank *lmu_bank)
> +{
> + switch (lmu_bank->type) {
> + case TI_LMU_BL:
> + return ti_lmu_bl_register_backlight(lmu_bank);
> + case TI_LMU_LED:
> + return ti_lmu_bl_register_led(lmu_bank);
> + default:
> + return -EINVAL;
> + }
> +}

Ok, this is somehow unusual/crazy. Single driver with two interfaces.

Do we need the LED interface for something?

If yes, I believe reasonable solution would be to always provide LED
interface, and then have "backlight-trigger" which that would provide
backlight interface for arbitrary LED.

Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (909.00 B)
signature.asc (188.00 B)
Digital signature Download all attachments

2018-04-03 10:51:12

by Pavel Machek

[permalink] [raw]

Subject: Re: [PATCHv4 10/10] ARM: dts: omap4-droid4: update backlight led-controller

On Fri 2018-03-30 19:24:14, Sebastian Reichel wrote:
> From: Sebastian Reichel <[email protected]>
>
> This updates the backlight led-controller node to follow the
> new binding instead of the legacy out-of-tree binding.
>
> Signed-off-by: Sebastian Reichel <[email protected]>

9,10: Acked-by: Pavel Machek <[email protected]>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (467.00 B)
signature.asc (188.00 B)
Digital signature Download all attachments

2018-04-03 10:53:20

On Fri, Mar 30, 2018 at 07:24:13PM +0200, Sebastian Reichel wrote:
> Update binding to integrate the backlight feature directly into
> the main node, as suggested by Rob Herring.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
> ---
> Documentation/devicetree/bindings/mfd/ti-lmu.txt | 119 ++++++++++++-----------
> 1 file changed, 60 insertions(+), 59 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/ti-lmu.txt b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> index c885cf89b8ce..b3433e95dfa5 100644
> --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> @@ -28,10 +28,9 @@ Required properties:
>
> Optional property:
> - enable-gpios: A GPIO specifier for hardware enable pin.
> -
> -Required node:
> - - backlight: All LMU devices have backlight child nodes.
> - For the properties, please refer to [1].
> + - pwm-names: Should be either "lmu-backlight" or unset
> + - pwm: This should be a PWM specifier following ../pwm/pwm.txt and must
> + only be specified, if the backlight should be used in PWM mode.
>
> Optional nodes:
> - fault-monitor: Hardware fault monitoring driver for LM3633 and LM3697.
> @@ -42,8 +41,31 @@ Optional nodes:
> - leds: LED properties for LM3633. Please refer to [2].
> - regulators: Regulator properties for LM3631 and LM3632.
> Please refer to [3].
> + - bank0, bank1, bank2: This contains the backlight configuration
> + for each backlight control bank.
> +
> +Required properties in the bank subnodes:
> + - label: A string describing the backlight. Should contain "keyboard"
> + for a keyboard backlight and "lcd" for LCD panel backlights.
> + - ti,led-sources: A list of channels, that should be driven. Each channel
> + should only be driven by one bank.
> +
> +Optional properties in the bank subnodes:
> + - default-brightness-level: Backlight initial brightness value.
> + Type is <u32>. It is set as soon as backlight
> + device is created.
> + 0 ~ 2047 = LM3631, LM3632, LM3633, LM3695 and
> + LM3697
> + 0 ~ 255 = LM3532
> + - ti,ramp-up-ms, ti,ramp-down-ms: Light dimming effect properties.
> + Type is <u32>. Unit is millisecond.
> + 0 ~ 65 ms = LM3532
> + 0 ~ 4000 ms = LM3631
> + 0 ~ 16000 ms = LM3633 and LM3697
> + - pwm-period: PWM period. Only valid in PWM brightness mode.
> + Type is <u32>. If this property is missing, then control
> + mode is set to I2C by default.
>
> -[1] ../leds/backlight/ti-lmu-backlight.txt
> [2] ../leds/leds-lm3633.txt
> [3] ../regulator/lm363x-regulator.txt
>
> @@ -53,14 +75,11 @@ lm3532@38 {
>
> enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
>
> - backlight {
> - compatible = "ti,lm3532-backlight";
> -
> - lcd {
> - led-sources = <0 1 2>;
> - ramp-up-msec = <30>;
> - ramp-down-msec = <0>;
> - };
> + bank0 {
> + label = "lcd";
> + led-sources = <0 1 2>;
> + ramp-up-msec = <30>;
> + ramp-down-msec = <0>;

Looks like it was already wrong, but these are missing the vendor
prefixes.

> };
> };
>
> @@ -105,13 +124,10 @@ lm3631@29 {

Are so many examples really needed? Examples aren't supposed to
enumerate all possible options...

Rob

2018-04-09 21:31:07

by Sebastian Reichel

[permalink] [raw]

Subject: Re: [PATCHv4 09/10] dt-bindings: mfd: ti-lmu: update for backlight

Hi,

On Mon, Apr 09, 2018 at 04:06:38PM -0500, Rob Herring wrote:
> On Fri, Mar 30, 2018 at 07:24:13PM +0200, Sebastian Reichel wrote:
> > Update binding to integrate the backlight feature directly into
> > the main node, as suggested by Rob Herring.
> >
> > Signed-off-by: Sebastian Reichel <[email protected]>
> > ---
> > Documentation/devicetree/bindings/mfd/ti-lmu.txt | 119 ++++++++++++-----------
> > 1 file changed, 60 insertions(+), 59 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/ti-lmu.txt b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> > index c885cf89b8ce..b3433e95dfa5 100644
> > --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> > +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> > @@ -28,10 +28,9 @@ Required properties:
> >
> > Optional property:
> > - enable-gpios: A GPIO specifier for hardware enable pin.
> > -
> > -Required node:
> > - - backlight: All LMU devices have backlight child nodes.
> > - For the properties, please refer to [1].
> > + - pwm-names: Should be either "lmu-backlight" or unset
> > + - pwm: This should be a PWM specifier following ../pwm/pwm.txt and must
> > + only be specified, if the backlight should be used in PWM mode.
> >
> > Optional nodes:
> > - fault-monitor: Hardware fault monitoring driver for LM3633 and LM3697.
> > @@ -42,8 +41,31 @@ Optional nodes:
> > - leds: LED properties for LM3633. Please refer to [2].
> > - regulators: Regulator properties for LM3631 and LM3632.
> > Please refer to [3].
> > + - bank0, bank1, bank2: This contains the backlight configuration
> > + for each backlight control bank.
> > +
> > +Required properties in the bank subnodes:
> > + - label: A string describing the backlight. Should contain "keyboard"
> > + for a keyboard backlight and "lcd" for LCD panel backlights.
> > + - ti,led-sources: A list of channels, that should be driven. Each channel
> > + should only be driven by one bank.
> > +
> > +Optional properties in the bank subnodes:
> > + - default-brightness-level: Backlight initial brightness value.
> > + Type is <u32>. It is set as soon as backlight
> > + device is created.
> > + 0 ~ 2047 = LM3631, LM3632, LM3633, LM3695 and
> > + LM3697
> > + 0 ~ 255 = LM3532
> > + - ti,ramp-up-ms, ti,ramp-down-ms: Light dimming effect properties.
> > + Type is <u32>. Unit is millisecond.
> > + 0 ~ 65 ms = LM3532
> > + 0 ~ 4000 ms = LM3631
> > + 0 ~ 16000 ms = LM3633 and LM3697
> > + - pwm-period: PWM period. Only valid in PWM brightness mode.
> > + Type is <u32>. If this property is missing, then control
> > + mode is set to I2C by default.
> >
> > -[1] ../leds/backlight/ti-lmu-backlight.txt
> > [2] ../leds/leds-lm3633.txt
> > [3] ../regulator/lm363x-regulator.txt
> >
> > @@ -53,14 +75,11 @@ lm3532@38 {
> >
> > enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
> >
> > - backlight {
> > - compatible = "ti,lm3532-backlight";
> > -
> > - lcd {
> > - led-sources = <0 1 2>;
> > - ramp-up-msec = <30>;
> > - ramp-down-msec = <0>;
> > - };
> > + bank0 {
> > + label = "lcd";
> > + led-sources = <0 1 2>;
> > + ramp-up-msec = <30>;
> > + ramp-down-msec = <0>;
>
> Looks like it was already wrong, but these are missing the vendor
> prefixes.

Oops. I will add the vendor prefix.

>
> > };
> > };
> >
> > @@ -105,13 +124,10 @@ lm3631@29 {
>
> Are so many examples really needed? Examples aren't supposed to
> enumerate all possible options...

I will remove a few in the next version.

-- Sebastian

Attachments:

(No filename) (3.72 kB)
signature.asc (849.00 B)
Download all attachments

2018-04-10 06:44:00

On Fri, 30 Mar 2018, Sebastian Reichel wrote:

> All LMU devices support multiple channels, that can be controlled
> independently. This registers one backlight sub-device per channel.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
> ---
> drivers/mfd/ti-lmu.c | 62 +++++++++++++++++++++++++++++++++++++---------
> include/linux/mfd/ti-lmu.h | 4 +++
> 2 files changed, 54 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mfd/ti-lmu.c b/drivers/mfd/ti-lmu.c
> index f43b8acc30e1..f4311d215dfa 100644
> --- a/drivers/mfd/ti-lmu.c
> +++ b/drivers/mfd/ti-lmu.c
> @@ -56,8 +56,16 @@ static void ti_lmu_disable_hw(void *data)
>
> static const struct mfd_cell lm3532_devices[] = {
> {
> - .name = "ti-lmu-backlight",
> - .id = LM3532,
> + .name = "ti-lmu-led-backlight",
> + .id = 0,

What's the reason for you manually setting .id?

> + },
> + {
> + .name = "ti-lmu-led-backlight",
> + .id = 1,
> + },
> + {
> + .name = "ti-lmu-led-backlight",
> + .id = 2,
> },
> };

--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog