linux-kernel - Re: [PATCH V3] pwm: lpc32xx - Add a driver for the motor PWM

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 9 Sep 2014 17:05:48 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	Alban Bedel <alban.bedel@...onic-design.de>
Cc:	Thierry Reding <thierry.reding@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	Kumar Gala <galak@...eaurora.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Pawel Moll <Pawel.Moll@....com>,
	Rob Herring <robh+dt@...nel.org>,
	Roland Stigge <stigge@...com.de>
Subject: Re: [PATCH V3] pwm: lpc32xx - Add a driver for the motor PWM

On Tue, Sep 09, 2014 at 04:42:41PM +0100, Alban Bedel wrote:
> The LPC32xx motor PWMs have two output pin, A and B, with B = !A.
> The driver can switch the polarity to allow use either output pin A
> or output pin B.
> 
> Signed-off-by: Alban Bedel <alban.bedel@...onic-design.de>
> ---
> V3: * Updated to current mainline API
>     * Fixed LPC32xx vs. LPC32XX
>     * Various coding style fix
> V2: * Splitted the DTS to its own patch
> ---
>  .../devicetree/bindings/pwm/lpc32xx-motor-pwm.txt  |  24 +++
>  drivers/pwm/Kconfig                                |  10 +
>  drivers/pwm/Makefile                               |   1 +
>  drivers/pwm/pwm-lpc32xx-motor.c                    | 210 +++++++++++++++++++++
>  4 files changed, 245 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt
>  create mode 100644 drivers/pwm/pwm-lpc32xx-motor.c
> 
> diff --git a/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt
> new file mode 100644
> index 0000000..decc27c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt
> @@ -0,0 +1,24 @@
> +LPC32xx Motor PWM controller
> +
> +The LPC32xx motor PWMs have two output pin, A and B, with B = !A.
> +By default, output A should be used, if output B is used the PWM
> +polarity should be inverted using the linux,polarity property.
> +
> +Required properties:
> +- compatible: should be "nxp,lpc3220-motor-pwm"
> +- reg: physical base address and length of the controller's registers
> +
> +Optional properties:
> +- linux,polarity: Bit mask of the polarity to use for each output,
> +      a bit set to 0 indicate the default polarity, a bit set to 1
> +      indicate an inverted polarity. In other word this set if output
> +      pin A or output pin B has the correct polarity.

What exactly does linux have to do with the choice of pin? Why should
this be "linux,polarity"?

> +
> +Examples:
> +
> +mpwm@...e8000 {
> +	compatible = "nxp,lpc3220-motor-pwm";
> +	reg = <0x400E8000 0x78>;
> +	linux,polarity = <0x5>; /* Use outputs B0, A1 and B2 */

This doesn't match the description of there being two output pins. I
take it the description above is somewhat misleading?

> +	#pwm-cells = <2>;

The format of these cells should be described.

Wouldn't it make more sense to describe the polarity in the
pwm-specifier? It seems like a property of the connection rather than
the PWM controller itself.

[...]

> +	lpc32xx->clk = devm_clk_get(&pdev->dev, NULL);

No clock was described in the binding.

Is there only the one clock feeding the pwm? (rather than separate
interface and pwm clocks).

Please describe clocks in the binding. If the clock inputs are named,
please use clock-names.

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/