On Fri, Nov 08, 2013 at 10:51:15PM +0000, Sebastian Reichel wrote:
> Merge all TWL family subnode documentation into
> the TWL family documentation file and fix typo
> in its filename.
>
> Signed-off-by: Sebastian Reichel <sre@xxxxxxxxxx>
> ---
Hi,
If all of this is being moved into one place it would be nice to clean
up some of the terminology and style so things are a bit more
consistent.
I'm not opposed to placing all of this in one file, but there are some
additional changes I'd like to see at the same time.
[...]
> diff --git a/Documentation/devicetree/bindings/mfd/twl-family.txt b/Documentation/devicetree/bindings/mfd/twl-family.txt
> new file mode 100644
> index 0000000..a296766
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/twl-family.txt
> @@ -0,0 +1,456 @@
> +Texas Instruments TWL family
> +----------------------------
> +
> +The TWLs are Integrated Power Management Chips.
> +Some version might contain much more analog function like
> +USB transceiver or Audio amplifier.
> +These chips are connected to an i2c bus.
> +
> +
> +Required properties:
> +- compatible : Must be "ti,twl4030";
> + For Integrated power-management/audio CODEC device used in OMAP3
> + based boards
> +- compatible : Must be "ti,twl6030";
> + For Integrated power-management used in OMAP4 based boards
This is confusing. I'd reorganise this to list the possibilities under
the description of the compatible property:
- compatible: should contain:
* "ti,twl4030": For integrated power-management/audio CODEC device used
in OMAP3 boards.
* "ti,twl6030": For integrated power-management used in OMAP4 based
boards.
In general it's nicer to describe compatible properties as "should
contain" rather than "Must be" or similar, we may have backwards
compatible HW variants in future and even if we don't it keeps all of
the documentation consistent. This applies to all the other instances in
the document too.
> +- interrupts : This i2c device has an IRQ line connected to the main SoC
This reads like a description of a boolean property. How about:
- interrupts: interrupt-specifier for the sole interrupt generated by
the device.
I assume that the twl only generates a single interrupt to its parent
interrupt controller.
> +- interrupt-controller : Since the twl support several interrupts internally,
> + it is considered as an interrupt controller cascaded to the SoC one.
> +- #interrupt-cells = <1>;
- #interrupt-cells: must be <1>
It would be nice to have something more here, even if it's just
something like "The twl supports 16 interrupts [0-15]" (note: I have no
idea how many interrupt inputs the twl actually has).
I think it would be better to note that the twl has some interrupt
controller functionality in the description at the top of the file
rather than here. The property description should obviously stay, but we
don't need a long-winded description of why we need the
interrupt-controller property.
> +- interrupt-parent : The parent interrupt controller.
Is this always required? Will this not be implicit most of the time?
> +
> +Optional nodes:
> +- Child nodes contained in the twl. The twl family is made of several
> + variants that support a different number of features. The children
> + nodes will thus depend of the capability of the variant.
> +
> +TWL Regulator
> +-------------
> +
> +Required properties:
> +For twl6030 regulators/LDOs
> +- compatible:
> + - "ti,twl6030-vaux1" for VAUX1 LDO
> + - "ti,twl6030-vaux2" for VAUX2 LDO
> + - "ti,twl6030-vaux3" for VAUX3 LDO
> + - "ti,twl6030-vmmc" for VMMC LDO
> + - "ti,twl6030-vpp" for VPP LDO
> + - "ti,twl6030-vusim" for VUSIM LDO
> + - "ti,twl6030-vana" for VANA LDO
> + - "ti,twl6030-vcxio" for VCXIO LDO
> + - "ti,twl6030-vdac" for VDAC LDO
> + - "ti,twl6030-vusb" for VUSB LDO
> + - "ti,twl6030-v1v8" for V1V8 LDO
> + - "ti,twl6030-v2v1" for V2V1 LDO
> + - "ti,twl6030-vdd1" for VDD1 SMPS
> + - "ti,twl6030-vdd2" for VDD2 SMPS
> + - "ti,twl6030-vdd3" for VDD3 SMPS
> +For twl6032 regulators/LDOs
> +- compatible:
> + - "ti,twl6032-ldo1" for LDO1 LDO
> + - "ti,twl6032-ldo2" for LDO2 LDO
> + - "ti,twl6032-ldo3" for LDO3 LDO
> + - "ti,twl6032-ldo4" for LDO4 LDO
> + - "ti,twl6032-ldo5" for LDO5 LDO
> + - "ti,twl6032-ldo6" for LDO6 LDO
> + - "ti,twl6032-ldo7" for LDO7 LDO
> + - "ti,twl6032-ldoln" for LDOLN LDO
> + - "ti,twl6032-ldousb" for LDOUSB LDO
> + - "ti,twl6032-smps3" for SMPS3 SMPS
> + - "ti,twl6032-smps4" for SMPS4 SMPS
> + - "ti,twl6032-vio" for VIO SMPS
> +For twl4030 regulators/LDOs
> +- compatible:
> + - "ti,twl4030-vaux1" for VAUX1 LDO
> + - "ti,twl4030-vaux2" for VAUX2 LDO
> + - "ti,twl5030-vaux2" for VAUX2 LDO
> + - "ti,twl4030-vaux3" for VAUX3 LDO
> + - "ti,twl4030-vaux4" for VAUX4 LDO
> + - "ti,twl4030-vmmc1" for VMMC1 LDO
> + - "ti,twl4030-vmmc2" for VMMC2 LDO
> + - "ti,twl4030-vpll1" for VPLL1 LDO
> + - "ti,twl4030-vpll2" for VPLL2 LDO
> + - "ti,twl4030-vsim" for VSIM LDO
> + - "ti,twl4030-vdac" for VDAC LDO
> + - "ti,twl4030-vintana2" for VINTANA2 LDO
> + - "ti,twl4030-vio" for VIO LDO
> + - "ti,twl4030-vdd1" for VDD1 SMPS
> + - "ti,twl4030-vdd2" for VDD2 SMPS
> + - "ti,twl4030-vintana1" for VINTANA1 LDO
> + - "ti,twl4030-vintdig" for VINTDIG LDO
> + - "ti,twl4030-vusb1v5" for VUSB1V5 LDO
> + - "ti,twl4030-vusb1v8" for VUSB1V8 LDO
> + - "ti,twl4030-vusb3v1" for VUSB3V1 LDO
> +
> +Optional properties:
> +- Any optional property defined in bindings/regulator/regulator.txt
> +
> +TWL RTC
> +-------
> +
> +Required properties:
> +- compatible : Should be twl4030-rtc
> +
> +TWL PWM
> +-------
> +
> +Supported PWMs:
> +On TWL4030 series: PWM1 and PWM2
> +On TWL6030 series: PWM0 and PWM1
Could you explain what is meant by "supported" here? Are these the only
PWMs that exist, or the only ones we bother to describe?
> +
> +Required properties:
> +- compatible: "ti,twl4030-pwm" or "ti,twl6030-pwm"
- compatible: Should contain one of:
* "ti,twl4030-pwm"
* "ti,twl6030-pwm"
> +- #pwm-cells: should be 2. See pwm.txt in this directory for a description of
> + the cells format.
> +
> +TWL PWM LED
> +-----------
> +
> +Supported PWMs:
> +On TWL4030 series: PWMA and PWMB (connected to LEDA and LEDB terminals)
> +On TWL6030 series: LED PWM (mainly used as charging indicator LED)
> +
> +Required properties:
> +- compatible: "ti,twl4030-pwmled" or "ti,twl6030-pwmled"
> +- #pwm-cells: should be 2. See pwm.txt in this directory for a description of
> + the cells format.
I assume that this pwm documentation comment is now invalid due to the
move to .../mfd/...?
I see that pwm.txt describe the typical usage of the pwm-specifier,
which I assume is what's being referred to, but doesn't describe the
valid range of values for the chip-relative PWM number, nor the value
range of periods. It would be nice to be explicit about the meaning of
the cells here.
> +
> +TWL4030 USB PHY AND COMPARATOR
> +------------------------------
> +
> +Required properties:
> + - compatible : Should be "ti,twl4030-usb"
> + - interrupts : The interrupt numbers to the cpu should be specified. First
> + interrupt number is the otg interrupt number that raises ID interrupts
> + and VBUS interrupts. The second interrupt number is optional.
Nit: the interrupt is described by an interrupt-specifier, and may not
be routed directly to the CPU. How about something like:
- interrupts: A list of interrupt-specifiers:
[1] The OTG interrupt
[2] The VBUS interrupt (optional)
Similarly elsewhere.
Are these the only interrupts generated by the USB PHY and comparator?
> + - <supply-name>-supply : phandle to the regulator device tree node.
> + <supply-name> should be vusb1v5, vusb1v8 and vusb3v1
For other bindings I've asked for each *-supply property to be listed
individually to enable easier searching. It would be nice to do the same
here.
> + - usb_mode : The mode used by the phy to connect to the controller. "1"
> + specifies "ULPI" mode and "2" specifies "CEA2011_3PIN" mode.
I assumes these should be <1> and <2> rather than "1" and "2" (i.e.
they're u32 cells rather than single character strings).
> +
> +TWL6030 USB COMPARATOR
> +----------------------
> +
> +Required properties:
> + - compatible : Should be "ti,twl6030-usb"
> + - interrupts : Two interrupt numbers to the cpu should be specified. First
> + interrupt number is the otg interrupt number that raises ID interrupts when
> + the controller has to act as host and the second interrupt number is the
> + usb interrupt number that raises VBUS interrupts when the controller has to
> + act as device
> + - usb-supply : phandle to the regulator device tree node. It should be vusb
> + if it is twl6030 or ldousb if it is twl6032 subclass.
> +
> +TWL4030 GPIO Controller
> +-----------------------
> +
> +Required properties:
> +- compatible:
> + - "ti,twl4030-gpio" for twl4030 GPIO controller
> +- #gpio-cells : Should be two.
> + - first cell is the pin number
> + - second cell is used to specify optional parameters (unused)
Is there a sane value for the second cell if it's unused (i.e. zero)?
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- #interrupt-cells : Should be 2.
> +- interrupt-controller: Mark the device node as an interrupt controller
> + The first cell is the GPIO number.
> + The second cell is not used.
These should be associated with #interrupt-cells, not
interrupt-controller.
> +- ti,use-leds : Enables LEDA and LEDB outputs if set
Type? Boolean?
> +- ti,debounce : if n-th bit is set, debounces GPIO-n
> +- ti,mmc-cd : if n-th bit is set, GPIO-n controls VMMC(n+1)
> +- ti,pullups : if n-th bit is set, set a pullup on GPIO-n
> +- ti,pulldowns : if n-th bit is set, set a pulldown on GPIO-n
Are these u32 cells?
> +
> +TWL4030 Audio
> +-------------
> +
> +The audio module inside the TWL family consist of an audio codec and
> +a vibrator driver.
> +
> +Required properties:
> +- compatible : must be "ti,twl4030-audio"
> +
> +Optional properties, nodes:
> +
> +Audio functionality:
> +- codec { }: Need to be present if the audio functionality is used. Within this
> + section the following options can be used:
If all of these properties go under the codex node, get rid of the
"optional properties, nodes: " line and just have:
The node must contain a subnode named "codec", which may have the
following properties:
> +- ti,digimic_delay: Delay need after enabling the digimic to reduce artifacts
> + from the start of the recorded sample (in ms)
> +-ti,ramp_delay_value: HS ramp delay configuration to reduce pop noise
> +-ti,hs_extmute: Use external mute for HS pop reduction
Is this a boolean, u32, phandle?
> +-ti,hs_extmute_gpio: Use external GPIO to control the external mute
Similarly?
> +-ti,offset_cncl_path: Offset cancellation path selection, refer to TRM for the
> + valid values.
> +
> +Vibra functionality
> +- ti,enable-vibra: Need to be set to <1> if the vibra functionality is used. if
> + missing or it is 0, the vibra functionality is disabled.
s/Need to be/Should be/
This reads like configuration (whether or not to use vibration), but I
assume what this is actually saying is that a motor is wired up to the
vibrator unit?
If that's not the case, then I'm not sure why we even have this
property.
> +
> +TWL4030 Power
> +-------------
> +
> +The power management module inside the TWL family provides several facilities
> +to control the power resources, including power scripts. For now, the binding
> +only supports the complete shutdown of the system after poweroff.
> +
> +Required properties:
> +- compatible : must be "ti,twl4030-power"
> +
> +Optional properties:
> +- ti,use_poweroff: With this flag, the chip will initiates an ACTIVE-to-OFF or
> + SLEEP-to-OFF transition when the system poweroffs.
> +
> +TWL4030 Watchdog
> +----------------
> +
> +Required properties:
> + compatible = "ti,twl4030-wdt";
This looks like a fragment of a dts rather than a description, and it's
inconsistent with the description of other properties. It would be far
nicer to have:
- compatible: Should contain "ti,twl4030-wdt"
> +
> +TWL4030 Power Button
> +--------------------
> +
> +This module is part of the TWL4030. For more details about the whole
> +chip see Documentation/devicetree/bindings/mfd/twl-familly.txt.
> +
> +This module provides a simple power button event via an Interrupt.
> +
> +Required properties:
> +- compatible: should be one of the following
> + - "ti,twl4030-pwrbutton": For controllers compatible with twl4030
> +- interrupts: should be one of the following
> + - <8>: For controllers compatible with twl4030
I find it odd to describe the precise wiring in the binding, but I guess
this can change as required if a future variant is wired differently.
> +
> +TWL4030 Keypad
> +--------------
> +
> +TWL4030's Keypad controller is used to interface a SoC with a matrix-type
> +keypad device. The keypad controller supports multiple row and column lines.
> +A key can be placed at each intersection of a unique row and a unique column.
> +The keypad controller can sense a key-press and key-release and report the
> +event using a interrupt to the cpu.
> +
> +This binding is based on the matrix-keymap binding with the following
> +changes:
> +
> + * keypad,num-rows and keypad,num-columns are required.
I would list these on separate lines, as done with other properties
(which will make them more difficult to miss).
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
