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

[Alban Bedel <alban.bedel@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 3306 bytes --]

On Tue, 9 Sep 2014 17:05:48 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> 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@avionic-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"?

Right, I'll remove this in favor of the standard 3 cells pwm specifiers.

> > +
> > +Examples:
> > +
> > +mpwm@400e8000 {
> > +	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.

Yes, will be done.

> 
> > +	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).

There is only one clock for this PWM block.

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

No clock is defined in the current LPC32xx DTS, what should I do in
this case?

Alban

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]