| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 14 Feb 2014 10:06:21 +0000
From: Mark Rutland <mark.rutland@....com>
To: Milo Kim <milo.kim@...com>
Cc: Lee Jones <lee.jones@...aro.org>, Jingoo Han <jg1.han@...sung.com>,
Bryan Wu <cooloney@...il.com>, Mark Brown <broonie@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Samuel Ortiz <sameo@...ux.intel.com>
Subject: Re: [PATCH 10/10] Documentation: Add device tree bindings for TI
LMU devices
On Fri, Feb 14, 2014 at 06:32:44AM +0000, Milo Kim wrote:
> Bindings for TI LMU, backlight, LM3631 regulator and LM3633 LED are added.
>
> Cc: devicetree@...r.kernel.org
> Cc: Bryan Wu <cooloney@...il.com>
> Cc: Jingoo Han <jg1.han@...sung.com>
> Cc: Lee Jones <lee.jones@...aro.org>
> Cc: Mark Brown <broonie@...nel.org>
> Cc: Samuel Ortiz <sameo@...ux.intel.com>
> Signed-off-by: Milo Kim <milo.kim@...com>
> ---
> .../devicetree/bindings/leds/leds-lm3633.txt | 39 +++++
> Documentation/devicetree/bindings/mfd/ti-lmu.txt | 182 ++++++++++++++++++++
> .../bindings/regulator/lm3631-regulator.txt | 49 ++++++
> .../bindings/video/backlight/ti-lmu-backlight.txt | 127 ++++++++++++++
> 4 files changed, 397 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3633.txt
> create mode 100644 Documentation/devicetree/bindings/mfd/ti-lmu.txt
> create mode 100644 Documentation/devicetree/bindings/regulator/lm3631-regulator.txt
> create mode 100644 Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3633.txt b/Documentation/devicetree/bindings/leds/leds-lm3633.txt
> new file mode 100644
> index 0000000..4adeb62
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-lm3633.txt
> @@ -0,0 +1,39 @@
> +TI LMU LM3633 LED device tree bindings
> +
> +Required properties:
> + - compatible: "ti,lm3633-leds"
> + - lvled1-used, lvled2-used, lvled3-used, lvled4-used, lvled5-used, lvled6-used
> + : LED string configuration. Each child node should include this information
> + about which LED string is used.
Which child nodes? They weren't mentioned until this point.
If properties need to be in a child node, mention the child node first,
and make it clear where the properties are expected to be.
> +
> +Optional properties:
> + - chan-name: LED channel name
Any reason to abbreviate "channel" to "chan"?
What's this for?
> + - max-current-milliamp: Max current setting. Unit is mA.
The code and examples treat this as an 8-bit value, but this fact isn't
mentioned here.
> +
> +Example:
> +
> +lm3633@36 {
> + compatible = "ti,lm3633";
It wasn't mentioned that this had to be a sub-node of a "ti,lm3633"
node. Please describe above and refer to the document for the
"ti,lm3633" binding.
[...]
> diff --git a/Documentation/devicetree/bindings/mfd/ti-lmu.txt b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> new file mode 100644
> index 0000000..2b3ecca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> @@ -0,0 +1,182 @@
> +TI LMU(Lighting Management Unit) device tree bindings
> +
> +TI LMU driver supports lighting devices belows.
> +
> + Name Device tree properties
> + ------ ------------------------
> + LM3532 Backlight
> + LM3631 Backlight and regulator
> + LM3633 Backlight and LED
> + LM3695 Backlight
> + LM3697 Backlight
> +
> +Those have shared device tree properties.
> +
> +Required properties:
> + - compatible: "ti,lm3532", "ti,lm3631", "ti,lm3633", "ti,lm3695", "ti,lm3697"
Should be one of, rather than all at once?
> + - reg: I2C slave address.
> + 0x38 is LM3532
> + 0x29 is LM3631
> + 0x36 is LM3633, LM3697
> + 0x63 is LM3695
> + - ti,enable-gpio: GPIO number of hardware enable pin
We refer to GPIOs with more than a number...
> +
> +For the TI LMU backlight properties, please refer to:
> +Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt
> +
> +For the LM3631 regulator properties, please refer to:
> +Documentation/devicetree/bindings/regulator/lm3631-regulator.txt
> +
> +For the LM3633 LED properties, please refer to:
> +Documentation/devicetree/bindings/leds/leds-lm3633.txt
Are these expected as subnodes?
> +
> +Examples:
> +
> +lm3532@38 {
> + compatible = "ti,lm3532";
> + reg = <0x38>;
> +
> + /* GPIO134 for HWEN pin */
> + ti,enable-gpio = <&gpio5 6 GPIO_ACTIVE_HIGH>;
> +
> + backlight {
> + compatible = "ti,lmu-backlight", "ti,lm3532-backlight";
This looks backwards. The most general string should be later in the
list. This applies elsewhere too.
[...]
> diff --git a/Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt b/Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt
> new file mode 100644
> index 0000000..554ddca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/backlight/ti-lmu-backlight.txt
> @@ -0,0 +1,127 @@
> +TI LMU backlight device tree bindings
> +
> +Required properties:
> + - compatible: One of lists below with "ti,lmu-backlight" should be set.
> + "ti,lm3532-backlight"
> + "ti,lm3631-backlight"
> + "ti,lm3633-backlight"
> + "ti,lm3695-backlight"
> + "ti,lm3697-backlight"
Do you mean that "ti,lmu-backlight" should be a fallback entry in the
compatible list?
> + - hvled1-used, hvled2-used, hvled3-used: Backlight string configuration.
> + Each backlight child node should include this information about
> + which backlight string is used.
> +
> +Optional properties
> + - bl-name: Backlight device name
Why bother abbreviating backlight to bl?
What's this for anyway? Surely something else has to link to the
backlight node, and a meaningful name should be implied.
> + - max-current-milliamp: Max current setting. Unit is mA.
Type?
> + - initial-brightness: Backlight initial brightness
Type? Units?
> + - ramp-up: Light effect for ramp up rate. Unit is msec.
> + - ramp-down: Light effect for ramp down rate. Unit is msec.
> + - pwm-period: PWM period. Only valid for PWM brightness control mode.
Type? Units?
> + - pwms, pwm-names: For the PWM user nodes, please refer to
How many do you expect?
What are each of them for?
What are they named?
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists