| Message ID | 20180817151528.21623-1-dmurphy@ti.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v5,1/2] dt-bindings: leds: Add bindings for lm3697 driver | expand |
On Fri, 17 Aug 2018 10:15:27 -0500, 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> > --- > > v5 - Fix the comment for the example - https://lore.kernel.org/patchwork/patch/975060/ > > v4 - Removed HVLED definition in favor of HVLED place definition - https://lore.kernel.org/patchwork/patch/974812/ > v3 - Updated subject with prefered title - https://lore.kernel.org/patchwork/patch/972337/ > v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/ > > .../devicetree/bindings/leds/leds-lm3697.txt | 86 +++++++++++++++++++ > 1 file changed, 86 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt > Reviewed-by: Rob Herring <robh@kernel.org>
On Fri 2018-08-17 10:15:27, 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> Acked-by: Pavel Machek <pavel@ucw.cz> Some nits are below. > +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. LED core is 8-bit only... so full dynamic range can not be currently used in linux -- right? Is there any plan to change/fix that? > +This device is suitable for Display and Keypad Lighting "display and keypad lighting." > +Optional properties: > + - enable-gpios : gpio pin to enable/disable the device. Remove "." at end of sentence, for consistency. "GPIO"? > +All HVLED strings controlled by control bank A ":"? > +led-controller@36 { > + compatible = "ti,lm3967"; > + reg = <0x36>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; > + vled-supply = <&vbatt>; > + > + led@0 { > + reg = <0>; > + led-sources = <1 1 1>; > + label = "white:backlight_cluster"; > + linux,default-trigger = "backlight"; > + }; > +} > + > +For more product information please see the link below: > +http://www.ti.com/lit/ds/symlink/lm3697.pdf
Hi! > +/** > + * struct lm3697 - > + * @enable_gpio - Hardware enable gpio > + * @regulator - LED supply regulator pointer > + * @client - Pointer to the I2C client > + * @regmap - Devices register map > + * @dev - Pointer to the devices device struct > + * @lock - Lock for reading/writing the device > + * @leds - Array of LED strings. > + */ extra . > + ret = regmap_write(led->priv->regmap, brt_msb_reg, brt_val); > + if (ret) { > + dev_err(&led->priv->client->dev, "Cannot write MSB\n"); > + goto out; > + } > +out: I'd avoid this goto. > +static int lm3697_set_control_bank(struct lm3697 *priv) > +{ > + u8 control_bank_config = 0; > + struct lm3697_led *led; > + int ret, i; > + > + led = &priv->leds[0]; > + if (led->control_bank == LM3697_CONTROL_A) > + led = &priv->leds[1]; I'd expect CONTROL_A to correspond to leds[0]...? > + for (i = 0; i < LM3697_MAX_LED_STRINGS; i++) { > + if (led->hvled_strings[i] == LM3697_HVLED_ASSIGNMENT) > + control_bank_config |= 1 << i; > + } Extra {}s. > + priv->regulator = devm_regulator_get(&priv->client->dev, "vled"); > + if (IS_ERR(priv->regulator)) > + priv->regulator = NULL; If vled regulator is specified in dt and _get fails, is it worth a warning? Pavel
Jacek On 08/24/2018 05:05 AM, Pavel Machek wrote: > Hi! > >> +/** >> + * struct lm3697 - >> + * @enable_gpio - Hardware enable gpio >> + * @regulator - LED supply regulator pointer >> + * @client - Pointer to the I2C client >> + * @regmap - Devices register map >> + * @dev - Pointer to the devices device struct >> + * @lock - Lock for reading/writing the device >> + * @leds - Array of LED strings. >> + */ > > extra . > >> + ret = regmap_write(led->priv->regmap, brt_msb_reg, brt_val); >> + if (ret) { >> + dev_err(&led->priv->client->dev, "Cannot write MSB\n"); >> + goto out; >> + } >> +out: > > I'd avoid this goto. > >> +static int lm3697_set_control_bank(struct lm3697 *priv) >> +{ >> + u8 control_bank_config = 0; >> + struct lm3697_led *led; >> + int ret, i; >> + >> + led = &priv->leds[0]; >> + if (led->control_bank == LM3697_CONTROL_A) >> + led = &priv->leds[1]; > > I'd expect CONTROL_A to correspond to leds[0]...? > >> + for (i = 0; i < LM3697_MAX_LED_STRINGS; i++) { >> + if (led->hvled_strings[i] == LM3697_HVLED_ASSIGNMENT) >> + control_bank_config |= 1 << i; >> + } > > Extra {}s. > >> + priv->regulator = devm_regulator_get(&priv->client->dev, "vled"); >> + if (IS_ERR(priv->regulator)) >> + priv->regulator = NULL; > > If vled regulator is specified in dt and _get fails, is it worth a > warning? > > Pavel > Do you want v6 or a new patch on top? Dan
Hi Pavel, On 08/24/2018 11:55 AM, Pavel Machek wrote: > On Fri 2018-08-17 10:15:27, 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> > > Acked-by: Pavel Machek <pavel@ucw.cz> > > Some nits are below. > >> +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. > > LED core is 8-bit only... That's not true since this commit: commit 1bd465e6b0e2b559db47420fea686507a01cfab0 Author: Guennadi Liakhovetski <lg@denx.de> Date: Sat Jan 10 18:54:39 2009 +0000 leds: allow led-drivers to use a variable range of brightness values This patch allows drivers to override the default maximum brightness value of 255. We take care to preserve backwards-compatibility as much as possible, so that user-space ABI doesn't change for existing drivers. LED trigger code has also been updated to use the per-LED maximum. Signed-off-by: Guennadi Liakhovetski <lg@denx.de> Signed-off-by: Richard Purdie <rpurdie@linux.intel.com> but I missed that max_brightness is not initialized in the driver, so the core will set it to LED_FULL (255) anyway. > so full dynamic range can not be currently > used in linux -- right? Is there any plan to change/fix that? > >> +This device is suitable for Display and Keypad Lighting > > "display and keypad lighting." > >> +Optional properties: >> + - enable-gpios : gpio pin to enable/disable the device. > > Remove "." at end of sentence, for consistency. "GPIO"? > >> +All HVLED strings controlled by control bank A > > ":"? > >> +led-controller@36 { >> + compatible = "ti,lm3967"; >> + reg = <0x36>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; >> + vled-supply = <&vbatt>; >> + >> + led@0 { >> + reg = <0>; >> + led-sources = <1 1 1>; >> + label = "white:backlight_cluster"; >> + linux,default-trigger = "backlight"; >> + }; >> +} >> + >> +For more product information please see the link below: >> +http://www.ti.com/lit/ds/symlink/lm3697.pdf >
Dan, On 08/24/2018 01:58 PM, Dan Murphy wrote: > Jacek > > On 08/24/2018 05:05 AM, Pavel Machek wrote: >> Hi! >> >>> +/** >>> + * struct lm3697 - >>> + * @enable_gpio - Hardware enable gpio >>> + * @regulator - LED supply regulator pointer >>> + * @client - Pointer to the I2C client >>> + * @regmap - Devices register map >>> + * @dev - Pointer to the devices device struct >>> + * @lock - Lock for reading/writing the device >>> + * @leds - Array of LED strings. >>> + */ >> >> extra . >> >>> + ret = regmap_write(led->priv->regmap, brt_msb_reg, brt_val); >>> + if (ret) { >>> + dev_err(&led->priv->client->dev, "Cannot write MSB\n"); >>> + goto out; >>> + } >>> +out: >> >> I'd avoid this goto. >> >>> +static int lm3697_set_control_bank(struct lm3697 *priv) >>> +{ >>> + u8 control_bank_config = 0; >>> + struct lm3697_led *led; >>> + int ret, i; >>> + >>> + led = &priv->leds[0]; >>> + if (led->control_bank == LM3697_CONTROL_A) >>> + led = &priv->leds[1]; >> >> I'd expect CONTROL_A to correspond to leds[0]...? >> >>> + for (i = 0; i < LM3697_MAX_LED_STRINGS; i++) { >>> + if (led->hvled_strings[i] == LM3697_HVLED_ASSIGNMENT) >>> + control_bank_config |= 1 << i; >>> + } >> >> Extra {}s. >> >>> + priv->regulator = devm_regulator_get(&priv->client->dev, "vled"); >>> + if (IS_ERR(priv->regulator)) >>> + priv->regulator = NULL; >> >> If vled regulator is specified in dt and _get fails, is it worth a >> warning? >> >> Pavel >> > > Do you want v6 or a new patch on top? Please submit v6.
Pavel On 08/24/2018 05:05 AM, Pavel Machek wrote: > Hi! > I am going to go ahead and update this code anyway and if we get the ti-lmu code in then we can adapt this code to use that framework >> +/** >> + * struct lm3697 - >> + * @enable_gpio - Hardware enable gpio >> + * @regulator - LED supply regulator pointer >> + * @client - Pointer to the I2C client >> + * @regmap - Devices register map >> + * @dev - Pointer to the devices device struct >> + * @lock - Lock for reading/writing the device >> + * @leds - Array of LED strings. >> + */ > > extra . ACK > >> + ret = regmap_write(led->priv->regmap, brt_msb_reg, brt_val); >> + if (ret) { >> + dev_err(&led->priv->client->dev, "Cannot write MSB\n"); >> + goto out; >> + } >> +out: > > I'd avoid this goto. Ack > >> +static int lm3697_set_control_bank(struct lm3697 *priv) >> +{ >> + u8 control_bank_config = 0; >> + struct lm3697_led *led; >> + int ret, i; >> + >> + led = &priv->leds[0]; >> + if (led->control_bank == LM3697_CONTROL_A) >> + led = &priv->leds[1]; > > I'd expect CONTROL_A to correspond to leds[0]...? Not always there can be a situation where all HVLEDs can be configured for control bank B > >> + for (i = 0; i < LM3697_MAX_LED_STRINGS; i++) { >> + if (led->hvled_strings[i] == LM3697_HVLED_ASSIGNMENT) >> + control_bank_config |= 1 << i; >> + } > > Extra {}s. Ack > >> + priv->regulator = devm_regulator_get(&priv->client->dev, "vled"); >> + if (IS_ERR(priv->regulator)) >> + priv->regulator = NULL; > > If vled regulator is specified in dt and _get fails, is it worth a > warning? Hmmm. That would be annoying on boot to have a warning on an optional parameter. But if we need it I can add it. Dan > > Pavel >
Pavel On 08/24/2018 04:55 AM, Pavel Machek wrote: > On Fri 2018-08-17 10:15:27, 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> > > Acked-by: Pavel Machek <pavel@ucw.cz> > > Some nits are below. > >> +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. > > LED core is 8-bit only... so full dynamic range can not be currently > used in linux -- right? Is there any plan to change/fix that? > Jacek answered this. >> +This device is suitable for Display and Keypad Lighting > > "display and keypad lighting." > Ack >> +Optional properties: >> + - enable-gpios : gpio pin to enable/disable the device. > > Remove "." at end of sentence, for consistency. "GPIO"? > >> +All HVLED strings controlled by control bank A > > ":"? Not sure what you are asking for here. > >> +led-controller@36 { >> + compatible = "ti,lm3967"; >> + reg = <0x36>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; >> + vled-supply = <&vbatt>; >> + >> + led@0 { >> + reg = <0>; >> + led-sources = <1 1 1>; >> + label = "white:backlight_cluster"; >> + linux,default-trigger = "backlight"; >> + }; >> +} >> + >> +For more product information please see the link below: >> +http://www.ti.com/lit/ds/symlink/lm3697.pdf >
Hi! > >> +All HVLED strings controlled by control bank A > > > > ":"? > > Not sure what you are asking for here. Text looked like missing ":" at end of line. Best regards, Pavel
diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt new file mode 100644 index 000000000000..76a529c69438 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt @@ -0,0 +1,86 @@ +* 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 + +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 + - led-sources : Indicates which HVLED string is associated to which + control bank. Each element in the array is associated + with a specific HVLED string. Element 0 is HVLED1, + element 1 is HVLED2 and element 2 HVLED3. + Additional information is contained + in Documentation/devicetree/bindings/leds/common.txt + 0 - HVLED is not active in this control bank + 1 - HVLED string is controlled by this control bank + +Optional child properties: + - label : see Documentation/devicetree/bindings/leds/common.txt + - linux,default-trigger : + see Documentation/devicetree/bindings/leds/common.txt + +Example: + +HVLED string 1 and 3 are controlled by control bank A and HVLED 2 string is +controlled by control bank B. + +led-controller@36 { + compatible = "ti,lm3967"; + reg = <0x36>; + #address-cells = <1>; + #size-cells = <0>; + + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; + vled-supply = <&vbatt>; + + led@0 { + reg = <0>; + led-sources = <1 0 1>; + label = "white:first_backlight_cluster"; + linux,default-trigger = "backlight"; + }; + + led@1 { + reg = <1>; + led-sources = <0 1 0>; + label = "white:second_backlight_cluster"; + linux,default-trigger = "backlight"; + }; +} + +All HVLED strings controlled by control bank A + +led-controller@36 { + compatible = "ti,lm3967"; + reg = <0x36>; + #address-cells = <1>; + #size-cells = <0>; + + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; + vled-supply = <&vbatt>; + + led@0 { + reg = <0>; + led-sources = <1 1 1>; + label = "white:backlight_cluster"; + linux,default-trigger = "backlight"; + }; +} + +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> --- v5 - Fix the comment for the example - https://lore.kernel.org/patchwork/patch/975060/ v4 - Removed HVLED definition in favor of HVLED place definition - https://lore.kernel.org/patchwork/patch/974812/ v3 - Updated subject with prefered title - https://lore.kernel.org/patchwork/patch/972337/ v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/ .../devicetree/bindings/leds/leds-lm3697.txt | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt