Re: [PATCHv4 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.

[Sascha Hauer <s.hauer@pengutronix.de>]

On Fri, Sep 13, 2013 at 01:58:42PM +0800, Xiubo Li wrote:
> This adds the Document for Freescale FTM PWM driver under
> Documentation/devicetree/bindings/pwm/.
> 
> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> ---
>  .../devicetree/bindings/pwm/pwm-fsl-ftm.txt        | 36 ++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> new file mode 100644
> index 0000000..e736806
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> @@ -0,0 +1,36 @@
> +Freescale FTM PWM controller
> +
> +Required properties:
> +- compatible: Should be "fsl,vf610-ftm-pwm"
> +- reg: Physical base address and length of the controller's registers
> +- #pwm-cells: Should be 3. See pwm.txt in this directory for a description of
> +  the cells format.
> +- clock-names : Includes the following module clock source entries:
> +    "ftm0" (system clock),
> +    "ftm0_fix_sel" (fixed frequency clock),
> +    "ftm0_ext_sel" (external clock)
> +- clocks : Must contain an entry list for entries in clock-names.
> +- fsl,pwm-counter-clk: The FTM PWM counter clock source, should be one of the
> +  entries in clock-names.
> +- For each channel's pinctrl, the "chN-active" and "chN-idle" states should be
> +  implemented at the same time.

I thought about this active/idle pinctrl stuff again. It should be
removed from the PWM driver. The above is used to control the PWM output
state when the PWM is disabled, so that for example a backlight stays
disabled after a call to pwm_disable.

IMO this is wrong. Instead, the pwm client drivers (pwm_backlight)
shouldn't assume a particular output state of the PWM after a pwm
disable. Instead they should simply set the PWM to a duty cycle of 0%
(or 100% for inverted) to effectively disable it. If then a PWM driver
can optimize this to actually disabling the PWM completely, then fine,
but that's an optimization.

If the handoff of PWM pins from bootloader state to regular state causes
problems like flashing backlights or LEDs due to the pinctrl framework
configuring the iomux before the PWM driver takes over, then the pinctrl
should be done from the client drivers (pwm_backlight, pwm_led), not
from the pwm driver and not from the pinctrl framework before
initializing the clients.

So NACK to this complex pinctrl setup in this pwm driver.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |