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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20140910104220.44558cf7@avionic-0020>
Date:	Wed, 10 Sep 2014 10:42:20 +0200
From:	Alban Bedel <alban.bedel@...onic-design.de>
To:	Mark Rutland <mark.rutland@....com>
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, 9 Sep 2014 17:05:48 +0100
Mark Rutland <mark.rutland@....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@...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"?

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

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

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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)