| Message ID | 20180807160442.8937-1-dmurphy@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] dt: bindings: lm3697: Add bindings for lm3697 driver | expand |
On 7.8.2018 18:04, Dan Murphy wrote: > Add the device tree bindings for the lm3697 > led driver for backlighting and display. Hi Dan, it seems like you missed to fix part of the subject as Jacek suggested. I think it should be: "dt-bindings: leds: Add bindings for lm3697 driver" Best regards, Michal > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > > v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/
Hi Michal. Thanks for the heads-up. "dt-" prefix is indeed more preferred than "dt: ". Dan - I will fix it by myself, no need to resend. Best regards, Jacek Anaszewski On 08/08/2018 09:56 AM, Michal Vokáč wrote: > On 7.8.2018 18:04, Dan Murphy wrote: >> Add the device tree bindings for the lm3697 >> led driver for backlighting and display. > > Hi Dan, > it seems like you missed to fix part of the subject as Jacek suggested. > > I think it should be: "dt-bindings: leds: Add bindings for lm3697 driver" > > Best regards, > Michal >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> >> v2 - Fixed subject and patch commit message - >> https://lore.kernel.org/patchwork/patch/971326/ > >
On Tue 2018-08-07 11:04:41, Dan Murphy wrote: > Add the device tree bindings for the lm3697 > led driver for backlighting and display. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > > v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/ > > .../devicetree/bindings/leds/leds-lm3697.txt | 64 +++++++++++++++++++ > 1 file changed, 64 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt > > diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt > new file mode 100644 > index 000000000000..7b8e490f1ea1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt > @@ -0,0 +1,64 @@ > +* Texas Instruments - LM3697 Highly Efficient White LED Driver > + > +The LM3697 11-bit LED driver provides high- > +performance backlight dimming for 1, 2, or 3 series > +LED strings while delivering up to 90% efficiency. > + > +This device is suitable for Display and Keypad Lighting > + > +Required properties: > + - compatible: > + "ti,lm3967" > + - reg : I2C slave address > + - #address-cells : 1 > + - #size-cells : 0 > + - control-bank-cfg - : Indicates which sink is connected to which control bank > + 0 - All HVLED outputs are controlled by bank A > + 1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A > + 2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A > + 3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A > + 4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A > + 5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A > + 6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B > + 7 - All HVLED outputs are controlled by bank B This is quite long way to describe a bitmask, no? Could we make it so that control-bank-cfg is not needed? > +Optional properties: > + - enable-gpios : gpio pin to enable/disable the device. > + - vled-supply : LED supply > + > +Required child properties: > + - reg : 0 - LED is Controlled by bank A > + 1 - LED is Controlled by bank B Can we compute control-bank-cfg from this? > +Optional child properties: > + - label : see Documentation/devicetree/bindings/leds/common.txt > + - linux,default-trigger : > + see Documentation/devicetree/bindings/leds/common.txt > + > +Example: > + > +led-controller@36 { > + compatible = "ti,lm3967"; > + reg = <0x36>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; > + vled-supply = <&vbatt>; > + control-bank-cfg = <0>; > + > + led@0 { > + reg = <0>; > + label = "white:first_backlight_cluster"; > + linux,default-trigger = "backlight"; > + }; > + > + led@1 { > + reg = <1>; > + label = "white:second_backlight_cluster"; > + linux,default-trigger = "frontlight"; > + }; > +} Does the example show correct config? AFAICT all controls go to bank A according to control-bank-cfg, yet led@1 describes bank B... Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi! > Introduce the lm3697 LED driver for > backlighting and display. > > Datasheet location: > http://www.ti.com/lit/ds/symlink/lm3697.pdf > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > + > +#define LM3697_HVLED1_2_3_A 0 > +#define LM3697_HVLED1_B_HVLED2_3_A 1 > +#define LM3697_HVLED2_B_HVLED1_3_A 2 > +#define LM3697_HVLED1_2_B_HVLED3_A 3 > +#define LM3697_HVLED3_B_HVLED1_2_A 4 > +#define LM3697_HVLED1_3_B_HVLED2_A 5 > +#define LM3697_HVLED1_A_HVLED2_3_B 6 > +#define LM3697_HVLED1_2_3_B 7 That's rather long and verbose way to describe a bitmap, right? > +static const struct regmap_config lm3697_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + > + .max_register = LM3697_CTRL_ENABLE, > + .reg_defaults = lm3697_reg_defs, > + .num_reg_defaults = ARRAY_SIZE(lm3697_reg_defs), > + .cache_type = REGCACHE_RBTREE, > +}; Is rbtree good idea? You don't have that many registers. > +static int lm3697_init(struct lm3697 *priv) > +{ > + int ret; > + .... > + regmap_write(priv->regmap, LM3697_RESET, LM3697_SW_RESET); No error checking required here? > + if (priv->control_bank_config < LM3697_HVLED1_2_3_A || > + priv->control_bank_config > LM3697_HVLED1_2_3_B) { > + dev_err(&priv->client->dev, "Control bank configuration is out of range\n"); > + ret = -EINVAL; > + goto out; > + } > + > + device_for_each_child_node(priv->dev, child) { > + led = &priv->leds[i]; > + > + ret = fwnode_property_read_u32(child, "reg", &led->control_bank); > + if (ret) { > + dev_err(&priv->client->dev, "reg DT property missing\n"); > + goto child_out; > + } > + > + fwnode_property_read_string(child, "linux,default-trigger", > + &led->led_dev.default_trigger); > + > + ret = fwnode_property_read_string(child, "label", &name); > + if (ret) > + snprintf(led->label, sizeof(led->label), > + "%s::", priv->client->name); > + else > + snprintf(led->label, sizeof(led->label), > + "%s:%s", priv->client->name, name); > + > + > + led->priv = priv; > + led->led_dev.name = led->label; > + led->led_dev.brightness_set_blocking = lm3697_brightness_set; > + > + ret = devm_led_classdev_register(priv->dev, &led->led_dev); > + if (ret) { > + dev_err(&priv->client->dev, "led register err: %d\n", ret); > + goto child_out; > + } > + > + if (priv->control_bank_config == LM3697_HVLED1_2_3_A || > + priv->control_bank_config == LM3697_HVLED1_2_3_B) > + goto child_out; This checks if we have just one bank, I see it. Should it also check the led actually uses the correct bank? > + i++; > + fwnode_handle_put(child); > + } > + > +child_out: > + fwnode_handle_put(child); Is not the fwnode_handle_put() done twice for non-error case? > + ret = lm3697_init(led); > + if (ret) > + return ret; > + > + return ret; > +} The if is not needed here. > +static int lm3697_remove(struct i2c_client *client) > +{ > + struct lm3697 *led = i2c_get_clientdata(client); > + int ret; > + > + ret = regmap_update_bits(led->regmap, LM3697_CTRL_ENABLE, > + LM3697_CTRL_A_B_EN, 0); > + if (ret) { > + dev_err(&led->client->dev, "Failed to disable regulator\n"); > + return ret; Misleading, this does nothing with regulators. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Thanks for the review On 08/08/2018 02:59 PM, Pavel Machek wrote: > On Tue 2018-08-07 11:04:41, Dan Murphy wrote: >> Add the device tree bindings for the lm3697 >> led driver for backlighting and display. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> >> v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/ >> >> .../devicetree/bindings/leds/leds-lm3697.txt | 64 +++++++++++++++++++ >> 1 file changed, 64 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt >> >> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt >> new file mode 100644 >> index 000000000000..7b8e490f1ea1 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt >> @@ -0,0 +1,64 @@ >> +* Texas Instruments - LM3697 Highly Efficient White LED Driver >> + >> +The LM3697 11-bit LED driver provides high- >> +performance backlight dimming for 1, 2, or 3 series >> +LED strings while delivering up to 90% efficiency. >> + >> +This device is suitable for Display and Keypad Lighting >> + >> +Required properties: >> + - compatible: >> + "ti,lm3967" >> + - reg : I2C slave address >> + - #address-cells : 1 >> + - #size-cells : 0 >> + - control-bank-cfg - : Indicates which sink is connected to which control bank >> + 0 - All HVLED outputs are controlled by bank A >> + 1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A >> + 2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A >> + 3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A >> + 4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A >> + 5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A >> + 6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B >> + 7 - All HVLED outputs are controlled by bank B > > This is quite long way to describe a bitmask, no? Could we make > it so that control-bank-cfg is not needed? The problem we have here is there is a potential to control 3 different LED string but only 2 sinks. So control bank A can control 2 LED strings and control bank b can control 1 LED string. These values represent device level control and configuration of the LED strings to a specific control bank. I racked my brain trying to figure out how to configure the control banks and associated LED strings. These values are for the device configuration itself and the reg below indicates which control bank the LED node is assigned to. > >> +Optional properties: >> + - enable-gpios : gpio pin to enable/disable the device. >> + - vled-supply : LED supply >> + >> +Required child properties: >> + - reg : 0 - LED is Controlled by bank A >> + 1 - LED is Controlled by bank B > > Can we compute control-bank-cfg from this? Don't see how you could compute this. There is no easy way to give indication to the driver which LED node belongs to which control bank. The control-bank-cfg is a device level property and the reg under the child is a LED string level property denoting the Class node to control bank mapping. Furthermore there are 2 device configurations that can be configured to only use 1 bank for all 3 LED strings. This will be answered in your comments in the code. > >> +Optional child properties: >> + - label : see Documentation/devicetree/bindings/leds/common.txt >> + - linux,default-trigger : >> + see Documentation/devicetree/bindings/leds/common.txt >> + >> +Example: >> + >> +led-controller@36 { >> + compatible = "ti,lm3967"; >> + reg = <0x36>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; >> + vled-supply = <&vbatt>; >> + control-bank-cfg = <0>; >> + >> + led@0 { >> + reg = <0>; >> + label = "white:first_backlight_cluster"; >> + linux,default-trigger = "backlight"; >> + }; >> + >> + led@1 { >> + reg = <1>; >> + label = "white:second_backlight_cluster"; >> + linux,default-trigger = "frontlight"; >> + }; >> +} > > Does the example show correct config? AFAICT all controls go to bank > A according to control-bank-cfg, yet led@1 describes bank B... This I can fix it should be a value between 1 and 6 > > Pavel > -- ------------------ Dan Murphy
On 08/08/2018 04:02 PM, Pavel Machek wrote: > Hi! > >>>> + - #size-cells : 0 >>>> + - control-bank-cfg - : Indicates which sink is connected to which control bank >>>> + 0 - All HVLED outputs are controlled by bank A >>>> + 1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A >>>> + 2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A >>>> + 3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A >>>> + 4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A >>>> + 5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A >>>> + 6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B >>>> + 7 - All HVLED outputs are controlled by bank B >>> >>> This is quite long way to describe a bitmask, no? Could we make >>> it so that control-bank-cfg is not needed? >> >> The problem we have here is there is a potential to control >> 3 different LED string but only 2 sinks. So control bank A can control 2 LED strings and control >> bank b can control 1 LED string. >> > > Can we forget about the LED strings, and just expose the sinks as > Linux LED devices? 2 sinks 3 LED strings. How do you know which LED string is which and what bank it belongs to when setting the brightness. Each Bank has a separate register for brightness control. Dan > Pavel > -- ------------------ Dan Murphy
On Wed 2018-08-08 16:04:43, Dan Murphy wrote: > On 08/08/2018 04:02 PM, Pavel Machek wrote: > > Hi! > > > >>>> + - #size-cells : 0 > >>>> + - control-bank-cfg - : Indicates which sink is connected to which control bank > >>>> + 0 - All HVLED outputs are controlled by bank A > >>>> + 1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A > >>>> + 2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A > >>>> + 3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A > >>>> + 4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A > >>>> + 5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A > >>>> + 6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B > >>>> + 7 - All HVLED outputs are controlled by bank B > >>> > >>> This is quite long way to describe a bitmask, no? Could we make > >>> it so that control-bank-cfg is not needed? > >> > >> The problem we have here is there is a potential to control > >> 3 different LED string but only 2 sinks. So control bank A can control 2 LED strings and control > >> bank b can control 1 LED string. > >> > > > > Can we forget about the LED strings, and just expose the sinks as > > Linux LED devices? > > 2 sinks 3 LED strings. How do you know which LED string is which and what bank it belongs > to when setting the brightness. Each Bank has a separate register for brightness control. Yes, and LED strings are statically assigned to banks, right? So why not simply forget about LED strings for sake of hw abstractions, and work just with banks? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Dan, On 08/08/2018 11:04 PM, Dan Murphy wrote: > On 08/08/2018 04:02 PM, Pavel Machek wrote: >> Hi! >> >>>>> + - #size-cells : 0 >>>>> + - control-bank-cfg - : Indicates which sink is connected to which control bank >>>>> + 0 - All HVLED outputs are controlled by bank A >>>>> + 1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A >>>>> + 2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A >>>>> + 3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A >>>>> + 4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A >>>>> + 5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A >>>>> + 6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B >>>>> + 7 - All HVLED outputs are controlled by bank B >>>> >>>> This is quite long way to describe a bitmask, no? Could we make >>>> it so that control-bank-cfg is not needed? >>> >>> The problem we have here is there is a potential to control >>> 3 different LED string but only 2 sinks. So control bank A can control 2 LED strings and control >>> bank b can control 1 LED string. >>> >> >> Can we forget about the LED strings, and just expose the sinks as >> Linux LED devices? > > 2 sinks 3 LED strings. How do you know which LED string is which and what bank it belongs > to when setting the brightness. Each Bank has a separate register for brightness control. Just a blind shot, without going into details - could you please check if led-sources property documented in the common LED bindings couldn't help here? -- Best regards, Jacek Anaszewski
On 08/08/2018 04:09 PM, Pavel Machek wrote: > On Wed 2018-08-08 16:04:43, Dan Murphy wrote: >> On 08/08/2018 04:02 PM, Pavel Machek wrote: >>> Hi! >>> >>>>>> + - #size-cells : 0 >>>>>> + - control-bank-cfg - : Indicates which sink is connected to which control bank >>>>>> + 0 - All HVLED outputs are controlled by bank A >>>>>> + 1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A >>>>>> + 2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A >>>>>> + 3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A >>>>>> + 4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A >>>>>> + 5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A >>>>>> + 6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B >>>>>> + 7 - All HVLED outputs are controlled by bank B >>>>> >>>>> This is quite long way to describe a bitmask, no? Could we make >>>>> it so that control-bank-cfg is not needed? >>>> >>>> The problem we have here is there is a potential to control >>>> 3 different LED string but only 2 sinks. So control bank A can control 2 LED strings and control >>>> bank b can control 1 LED string. >>>> >>> >>> Can we forget about the LED strings, and just expose the sinks as >>> Linux LED devices? >> >> 2 sinks 3 LED strings. How do you know which LED string is which and what bank it belongs >> to when setting the brightness. Each Bank has a separate register for brightness control. > > Yes, and LED strings are statically assigned to banks, right? > > So why not simply forget about LED strings for sake of hw > abstractions, and work just with banks? How would you set the control bank register for the correct LED string configuration? Dan > Pavel > -- ------------------ Dan Murphy
Jacek On 08/08/2018 04:09 PM, Jacek Anaszewski wrote: > Hi Dan, > > On 08/08/2018 11:04 PM, Dan Murphy wrote: >> On 08/08/2018 04:02 PM, Pavel Machek wrote: >>> Hi! >>> >>>>>> + - #size-cells : 0 >>>>>> + - control-bank-cfg - : Indicates which sink is connected to which control bank >>>>>> + 0 - All HVLED outputs are controlled by bank A >>>>>> + 1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A >>>>>> + 2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A >>>>>> + 3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A >>>>>> + 4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A >>>>>> + 5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A >>>>>> + 6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B >>>>>> + 7 - All HVLED outputs are controlled by bank B >>>>> >>>>> This is quite long way to describe a bitmask, no? Could we make >>>>> it so that control-bank-cfg is not needed? >>>> >>>> The problem we have here is there is a potential to control >>>> 3 different LED string but only 2 sinks. So control bank A can control 2 LED strings and control >>>> bank b can control 1 LED string. >>>> >>> >>> Can we forget about the LED strings, and just expose the sinks as >>> Linux LED devices? >> >> 2 sinks 3 LED strings. How do you know which LED string is which and what bank it belongs >> to when setting the brightness. Each Bank has a separate register for brightness control. > > Just a blind shot, without going into details - could you please check > if led-sources property documented in the common LED bindings couldn't > help here? > I could change the name to led-sources. But this part does not really follow the 1 output to a 1 LED string topology. -- ------------------ Dan Murphy
On Wed 2018-08-08 16:41:16, Dan Murphy wrote: > On 08/08/2018 04:09 PM, Pavel Machek wrote: > > On Wed 2018-08-08 16:04:43, Dan Murphy wrote: > >> On 08/08/2018 04:02 PM, Pavel Machek wrote: > >>> Hi! > >>> > >>>>>> + - #size-cells : 0 > >>>>>> + - control-bank-cfg - : Indicates which sink is connected to which control bank > >>>>>> + 0 - All HVLED outputs are controlled by bank A > >>>>>> + 1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A > >>>>>> + 2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A > >>>>>> + 3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A > >>>>>> + 4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A > >>>>>> + 5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A > >>>>>> + 6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B > >>>>>> + 7 - All HVLED outputs are controlled by bank B > >>>>> > >>>>> This is quite long way to describe a bitmask, no? Could we make > >>>>> it so that control-bank-cfg is not needed? > >>>> > >>>> The problem we have here is there is a potential to control > >>>> 3 different LED string but only 2 sinks. So control bank A can control 2 LED strings and control > >>>> bank b can control 1 LED string. > >>>> > >>> > >>> Can we forget about the LED strings, and just expose the sinks as > >>> Linux LED devices? > >> > >> 2 sinks 3 LED strings. How do you know which LED string is which and what bank it belongs > >> to when setting the brightness. Each Bank has a separate register for brightness control. > > > > Yes, and LED strings are statically assigned to banks, right? > > > > So why not simply forget about LED strings for sake of hw > > abstractions, and work just with banks? > > How would you set the control bank register for the correct LED string configuration? Have property at each LED saying which which HVLEDs it controls? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel On 08/08/2018 04:45 PM, Pavel Machek wrote: > On Wed 2018-08-08 16:41:16, Dan Murphy wrote: >> On 08/08/2018 04:09 PM, Pavel Machek wrote: >>> On Wed 2018-08-08 16:04:43, Dan Murphy wrote: >>>> On 08/08/2018 04:02 PM, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>>>> + - #size-cells : 0 >>>>>>>> + - control-bank-cfg - : Indicates which sink is connected to which control bank >>>>>>>> + 0 - All HVLED outputs are controlled by bank A >>>>>>>> + 1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A >>>>>>>> + 2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A >>>>>>>> + 3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A >>>>>>>> + 4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A >>>>>>>> + 5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A >>>>>>>> + 6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B >>>>>>>> + 7 - All HVLED outputs are controlled by bank B >>>>>>> >>>>>>> This is quite long way to describe a bitmask, no? Could we make >>>>>>> it so that control-bank-cfg is not needed? >>>>>> >>>>>> The problem we have here is there is a potential to control >>>>>> 3 different LED string but only 2 sinks. So control bank A can control 2 LED strings and control >>>>>> bank b can control 1 LED string. >>>>>> >>>>> >>>>> Can we forget about the LED strings, and just expose the sinks as >>>>> Linux LED devices? >>>> >>>> 2 sinks 3 LED strings. How do you know which LED string is which and what bank it belongs >>>> to when setting the brightness. Each Bank has a separate register for brightness control. >>> >>> Yes, and LED strings are statically assigned to banks, right? >>> >>> So why not simply forget about LED strings for sake of hw >>> abstractions, and work just with banks? >> >> How would you set the control bank register for the correct LED string configuration? > > Have property at each LED saying which which HVLEDs it controls? Isn't that what I have already using the reg property? Then we would have to aggregate the configuration and make a determination in the driver. But that does not follow the LED child node ideology. Each output of the LED driver should have a child node. In this case the outputs are the sinks(inputs) and there are only 2 sinks so having 3 LED child nodes would be confusing and there are required properties for each child like label. Each child node would then need to present 1 LED node to the user space to control the LED string. Which would be technically incorrect because you would have 2 LED nodes controlling the same control bank sink. > > Pavel > -- ------------------ Dan Murphy
Hi! > >>> Yes, and LED strings are statically assigned to banks, right? > >>> > >>> So why not simply forget about LED strings for sake of hw > >>> abstractions, and work just with banks? > >> > >> How would you set the control bank register for the correct LED string configuration? > > > > Have property at each LED saying which which HVLEDs it controls? > > Isn't that what I have already using the reg property? > Then we would have to aggregate the configuration and make a determination in the driver. Yes. > But that does not follow the LED child node ideology. > Each output of the LED driver should have a child node. > > In this case the outputs are the sinks(inputs) and there are only 2 sinks so having 3 LED child nodes would be confusing > and there are required properties for each child like label. No, I don't want that. I'd like 2 child nodes, each specifying which HVLEDs it controls. Let me edit the original proposal. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue 2018-08-07 11:04:41, Dan Murphy wrote: > Add the device tree bindings for the lm3697 > led driver for backlighting and display. What I'd like to see: > diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt > new file mode 100644 > index 000000000000..7b8e490f1ea1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt > @@ -0,0 +1,64 @@ > +* Texas Instruments - LM3697 Highly Efficient White LED Driver > + > +The LM3697 11-bit LED driver provides high- > +performance backlight dimming for 1, 2, or 3 series > +LED strings while delivering up to 90% efficiency. > + > +This device is suitable for Display and Keypad Lighting > + > +Required properties: > + - compatible: > + "ti,lm3967" > + - reg : I2C slave address > + - #address-cells : 1 > + - #size-cells : 0 > + - control-bank-cfg - : Indicates which sink is connected to which control bank > + 0 - All HVLED outputs are controlled by bank A > + 1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A > + 2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A > + 3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A > + 4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A > + 5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A > + 6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B > + 7 - All HVLED outputs are controlled by bank B Remove control-bank-cfg. > +Optional properties: > + - enable-gpios : gpio pin to enable/disable the device. > + - vled-supply : LED supply > + > +Required child properties: > + - reg : 0 - LED is Controlled by bank A > + 1 - LED is Controlled by bank B Add required child property: - hvleds = <list> -- set of outputs this child controls. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Jacek and Pavel On 08/09/2018 07:09 AM, Jacek Anaszewski wrote: > Dan, > > On 08/08/2018 11:45 PM, Dan Murphy wrote: >> Jacek >> >> On 08/08/2018 04:09 PM, Jacek Anaszewski wrote: >>> Hi Dan, >>> >>> On 08/08/2018 11:04 PM, Dan Murphy wrote: >>>> On 08/08/2018 04:02 PM, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>>>> + - #size-cells : 0 >>>>>>>> + - control-bank-cfg - : Indicates which sink is connected to which control bank >>>>>>>> + 0 - All HVLED outputs are controlled by bank A >>>>>>>> + 1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A >>>>>>>> + 2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A >>>>>>>> + 3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A >>>>>>>> + 4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A >>>>>>>> + 5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A >>>>>>>> + 6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B >>>>>>>> + 7 - All HVLED outputs are controlled by bank B >>>>>>> >>>>>>> This is quite long way to describe a bitmask, no? Could we make >>>>>>> it so that control-bank-cfg is not needed? >>>>>> >>>>>> The problem we have here is there is a potential to control >>>>>> 3 different LED string but only 2 sinks. So control bank A can control 2 LED strings and control >>>>>> bank b can control 1 LED string. >>>>>> >>>>> >>>>> Can we forget about the LED strings, and just expose the sinks as >>>>> Linux LED devices? >>>> >>>> 2 sinks 3 LED strings. How do you know which LED string is which and what bank it belongs >>>> to when setting the brightness. Each Bank has a separate register for brightness control. >>> >>> Just a blind shot, without going into details - could you please check >>> if led-sources property documented in the common LED bindings couldn't >>> help here? >>> >> >> I could change the name to led-sources. But this part does not really follow the 1 output to a >> 1 LED string topology. > > led-sources was designed for describing the topology where one LED can > be connected to more then one output, see bindings of > max77693-led (in Documentation/devicetree/bindings/mfd/max77693.txt). > > Here the topology is a bit different - more than one LED (string) can be > connected to a single bank, but this is accomplished inside the chip. > Logically LEDs configured that way can be treated as a single LED > (string) connected to two outputs, and what follows they should be > described by a single DT child node. > > led-sources will fit very well for this purpose. You could do > the following mapping: > > 0 - HVLED1 > 1 - HVLED2 > 2 - HVLED3 > > Then, in the child DT nodes you would use these identifiers to describe > the topology: > > Following node would describe strings connected to the outputs > HVLED1 and HVLED2 controlled by bank A. > > led@0 { > reg = <0>; > led-sources = <0>. <1>; > label = "white:first_backlight_cluster"; > linux,default-trigger = "backlight"; > }; > > > IOW I agree with Pavel, but I propose to use already documented common > DT LED property. > I agree to use the led-sources but I still believe this approach may be confusing to other sw devs and will lead to configuration issues by users. This implementation requires the sw dev to know which strings are controlled by which bank. And this method may produce a misconfiguration like something below where HVLED2 is declared in both bank A and bank B led@0 { reg = <0>; led-sources = <0>. <1>; label = "white:first_backlight_cluster"; linux,default-trigger = "backlight"; }; led@1 { reg = <1>; led-sources = <1>. <2>; label = "white:keypad_cluster"; linux,default-trigger = "backlight"; }; The driver will need to be intelligent and declare a miss configuration on the above. Not saying this cannot be done but I am not sure why we want to add all of these extra LoC and intelligence in the kernel driver. The driver cannot make assumptions on the intention. And the device tree documentation will need to pretty much need a lengthy explanation on how to configure the child nodes. The implementation I suggested removes that ambiguity. It is a simple integer that is written to the device as part of the device configuration, which the config is a setting for the device. The child nodes denote which bank the exposed LED node will control. Removing any need for the sw developers new or old to know the specific device configurations. Dan -- ------------------ Dan Murphy
Dan, On 08/09/2018 03:30 PM, Dan Murphy wrote: > Jacek and Pavel > > On 08/09/2018 07:09 AM, Jacek Anaszewski wrote: >> Dan, >> >> On 08/08/2018 11:45 PM, Dan Murphy wrote: >>> Jacek >>> >>> On 08/08/2018 04:09 PM, Jacek Anaszewski wrote: >>>> Hi Dan, >>>> >>>> On 08/08/2018 11:04 PM, Dan Murphy wrote: >>>>> On 08/08/2018 04:02 PM, Pavel Machek wrote: >>>>>> Hi! >>>>>> >>>>>>>>> + - #size-cells : 0 >>>>>>>>> + - control-bank-cfg - : Indicates which sink is connected to which control bank >>>>>>>>> + 0 - All HVLED outputs are controlled by bank A >>>>>>>>> + 1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A >>>>>>>>> + 2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A >>>>>>>>> + 3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A >>>>>>>>> + 4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A >>>>>>>>> + 5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A >>>>>>>>> + 6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B >>>>>>>>> + 7 - All HVLED outputs are controlled by bank B >>>>>>>> >>>>>>>> This is quite long way to describe a bitmask, no? Could we make >>>>>>>> it so that control-bank-cfg is not needed? >>>>>>> >>>>>>> The problem we have here is there is a potential to control >>>>>>> 3 different LED string but only 2 sinks. So control bank A can control 2 LED strings and control >>>>>>> bank b can control 1 LED string. >>>>>>> >>>>>> >>>>>> Can we forget about the LED strings, and just expose the sinks as >>>>>> Linux LED devices? >>>>> >>>>> 2 sinks 3 LED strings. How do you know which LED string is which and what bank it belongs >>>>> to when setting the brightness. Each Bank has a separate register for brightness control. >>>> >>>> Just a blind shot, without going into details - could you please check >>>> if led-sources property documented in the common LED bindings couldn't >>>> help here? >>>> >>> >>> I could change the name to led-sources. But this part does not really follow the 1 output to a >>> 1 LED string topology. >> >> led-sources was designed for describing the topology where one LED can >> be connected to more then one output, see bindings of >> max77693-led (in Documentation/devicetree/bindings/mfd/max77693.txt). >> >> Here the topology is a bit different - more than one LED (string) can be >> connected to a single bank, but this is accomplished inside the chip. >> Logically LEDs configured that way can be treated as a single LED >> (string) connected to two outputs, and what follows they should be >> described by a single DT child node. >> >> led-sources will fit very well for this purpose. You could do >> the following mapping: >> >> 0 - HVLED1 >> 1 - HVLED2 >> 2 - HVLED3 >> >> Then, in the child DT nodes you would use these identifiers to describe >> the topology: >> >> Following node would describe strings connected to the outputs >> HVLED1 and HVLED2 controlled by bank A. >> >> led@0 { >> reg = <0>; >> led-sources = <0>. <1>; >> label = "white:first_backlight_cluster"; >> linux,default-trigger = "backlight"; >> }; >> >> >> IOW I agree with Pavel, but I propose to use already documented common >> DT LED property. >> > > I agree to use the led-sources but I still believe this approach may be confusing to other sw devs > and will lead to configuration issues by users. > > This implementation requires the sw dev to know which strings are controlled by which bank. > And this method may produce a misconfiguration like something below where HVLED2 is declared in > both bank A and bank B > > led@0 { > reg = <0>; > led-sources = <0>. <1>; > label = "white:first_backlight_cluster"; > linux,default-trigger = "backlight"; > }; > > led@1 { > reg = <1>; > led-sources = <1>. <2>; > label = "white:keypad_cluster"; > linux,default-trigger = "backlight"; > }; > > The driver will need to be intelligent and declare a miss configuration on the above. > Not saying this cannot be done but I am not sure why we want to add all of these extra LoC and intelligence > in the kernel driver. It is better do add some complexity to the driver than to the user configurable settings like DT. Besides - you will only need to check if given led-source is already taken by another node. > The driver cannot make assumptions on the intention. And the device tree documentation will need to > pretty much need a lengthy explanation on how to configure the child nodes. Some description will be needed for sure, but I don't expect it to be overwhelmingly lengthy. > The implementation I suggested removes that ambiguity. It is a simple integer that is written to the device > as part of the device configuration, which the config is a setting for the device. Your control-bank-cfg seemed like having much room for improvement, and it would for sure raise questions on why it was implemented that way. Documenting all available combinations of the configuration is seldom the best solution. It often obscures the issue. > The child nodes denote which bank the exposed LED node will control. Removing any need > for the sw developers new or old to know the specific device configurations. In your bindings device configuration is scattered among global control-bank-cfg property and child node's reg property. In my proposal each child node contains all the needed configuration, also in the form of two properties - led-sources and reg. IMHO having all the LED class device related configuration in one place simplifies the analysis. -- Best regards, Jacek Anaszewski
Pavel Thanks for the review On 08/08/2018 02:59 PM, Pavel Machek wrote: > Hi! > >> Introduce the lm3697 LED driver for >> backlighting and display. >> >> Datasheet location: >> http://www.ti.com/lit/ds/symlink/lm3697.pdf >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> > >> + >> +#define LM3697_HVLED1_2_3_A 0 >> +#define LM3697_HVLED1_B_HVLED2_3_A 1 >> +#define LM3697_HVLED2_B_HVLED1_3_A 2 >> +#define LM3697_HVLED1_2_B_HVLED3_A 3 >> +#define LM3697_HVLED3_B_HVLED1_2_A 4 >> +#define LM3697_HVLED1_3_B_HVLED2_A 5 >> +#define LM3697_HVLED1_A_HVLED2_3_B 6 >> +#define LM3697_HVLED1_2_3_B 7 > > That's rather long and verbose way to describe a bitmap, right? It will be removed with v3 > >> +static const struct regmap_config lm3697_regmap_config = { >> + .reg_bits = 8, >> + .val_bits = 8, >> + >> + .max_register = LM3697_CTRL_ENABLE, >> + .reg_defaults = lm3697_reg_defs, >> + .num_reg_defaults = ARRAY_SIZE(lm3697_reg_defs), >> + .cache_type = REGCACHE_RBTREE, >> +}; > > Is rbtree good idea? You don't have that many registers. ack > >> +static int lm3697_init(struct lm3697 *priv) >> +{ >> + int ret; >> + > .... >> + regmap_write(priv->regmap, LM3697_RESET, LM3697_SW_RESET); > > No error checking required here? Ack > >> + if (priv->control_bank_config < LM3697_HVLED1_2_3_A || >> + priv->control_bank_config > LM3697_HVLED1_2_3_B) { >> + dev_err(&priv->client->dev, "Control bank configuration is out of range\n"); >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + device_for_each_child_node(priv->dev, child) { >> + led = &priv->leds[i]; >> + >> + ret = fwnode_property_read_u32(child, "reg", &led->control_bank); >> + if (ret) { >> + dev_err(&priv->client->dev, "reg DT property missing\n"); >> + goto child_out; >> + } >> + >> + fwnode_property_read_string(child, "linux,default-trigger", >> + &led->led_dev.default_trigger); >> + >> + ret = fwnode_property_read_string(child, "label", &name); >> + if (ret) >> + snprintf(led->label, sizeof(led->label), >> + "%s::", priv->client->name); >> + else >> + snprintf(led->label, sizeof(led->label), >> + "%s:%s", priv->client->name, name); >> + >> + >> + led->priv = priv; >> + led->led_dev.name = led->label; >> + led->led_dev.brightness_set_blocking = lm3697_brightness_set; >> + >> + ret = devm_led_classdev_register(priv->dev, &led->led_dev); >> + if (ret) { >> + dev_err(&priv->client->dev, "led register err: %d\n", ret); >> + goto child_out; >> + } >> + >> + if (priv->control_bank_config == LM3697_HVLED1_2_3_A || >> + priv->control_bank_config == LM3697_HVLED1_2_3_B) >> + goto child_out; > > This checks if we have just one bank, I see it. Should it also check > the led actually uses the correct bank? It will be removed with v3 > >> + i++; >> + fwnode_handle_put(child); >> + } >> + >> +child_out: >> + fwnode_handle_put(child); > > Is not the fwnode_handle_put() done twice for non-error case? Ack > >> + ret = lm3697_init(led); >> + if (ret) >> + return ret; >> + >> + return ret; >> +} > > The if is not needed here. > Ack >> +static int lm3697_remove(struct i2c_client *client) >> +{ >> + struct lm3697 *led = i2c_get_clientdata(client); >> + int ret; >> + >> + ret = regmap_update_bits(led->regmap, LM3697_CTRL_ENABLE, >> + LM3697_CTRL_A_B_EN, 0); >> + if (ret) { >> + dev_err(&led->client->dev, "Failed to disable regulator\n"); >> + return ret; > > Misleading, this does nothing with regulators. Ack > > Pavel > -- ------------------ Dan Murphy
diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt new file mode 100644 index 000000000000..7b8e490f1ea1 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt @@ -0,0 +1,64 @@ +* Texas Instruments - LM3697 Highly Efficient White LED Driver + +The LM3697 11-bit LED driver provides high- +performance backlight dimming for 1, 2, or 3 series +LED strings while delivering up to 90% efficiency. + +This device is suitable for Display and Keypad Lighting + +Required properties: + - compatible: + "ti,lm3967" + - reg : I2C slave address + - #address-cells : 1 + - #size-cells : 0 + - control-bank-cfg - : Indicates which sink is connected to which control bank + 0 - All HVLED outputs are controlled by bank A + 1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A + 2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A + 3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A + 4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A + 5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A + 6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B + 7 - All HVLED outputs are controlled by bank B + +Optional properties: + - enable-gpios : gpio pin to enable/disable the device. + - vled-supply : LED supply + +Required child properties: + - reg : 0 - LED is Controlled by bank A + 1 - LED is Controlled by bank B + +Optional child properties: + - label : see Documentation/devicetree/bindings/leds/common.txt + - linux,default-trigger : + see Documentation/devicetree/bindings/leds/common.txt + +Example: + +led-controller@36 { + compatible = "ti,lm3967"; + reg = <0x36>; + #address-cells = <1>; + #size-cells = <0>; + + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; + vled-supply = <&vbatt>; + control-bank-cfg = <0>; + + led@0 { + reg = <0>; + label = "white:first_backlight_cluster"; + linux,default-trigger = "backlight"; + }; + + led@1 { + reg = <1>; + label = "white:second_backlight_cluster"; + linux,default-trigger = "frontlight"; + }; +} + +For more product information please see the link below: +http://www.ti.com/lit/ds/symlink/lm3697.pdf
Add the device tree bindings for the lm3697 led driver for backlighting and display. Signed-off-by: Dan Murphy <dmurphy@ti.com> --- v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/ .../devicetree/bindings/leds/leds-lm3697.txt | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt -- 2.17.0.582.gccdcbd54c