diff mbox

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

Message ID 1377054462-6283-5-git-send-email-Li.Xiubo@freescale.com (mailing list archive)
State New, archived
Headers show

Commit Message

Xiubo Li Aug. 21, 2013, 3:07 a.m. UTC
Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 .../devicetree/bindings/pwm/fsl-ftm-pwm.txt        | 52 ++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt

Comments

Tomasz Figa Aug. 21, 2013, 7:30 p.m. UTC | #1
Hi Xiubo,

Please see my comments inline.

On Wednesday 21 of August 2013 11:07:42 Xiubo Li wrote:
> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> ---
>  .../devicetree/bindings/pwm/fsl-ftm-pwm.txt        | 52
> ++++++++++++++++++++++ 1 file changed, 52 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt new file mode
> 100644
> index 0000000..698965b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> @@ -0,0 +1,52 @@
> +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. Number of cells being used to specify PWM
> property.
> +  First cell specifies the per-chip channel index of the PWM
> to use, the 
> +  second cell is the period in nanoseconds and bit 0 in
> the third cell is 
> +  used to encode the polarity of PWM output. Set bit
> 0 of the third in PWM 
> +  specifier to 1 for inverse polarity & set to 0
> for normal polarity.

If the meaning of flags cell is the same as in generic, default PWM 
specifier format, then it should be noted here and generic PWM binding 
documentation mentioned. 

> +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler,
> divide-by 2^n(n = 0 ~ 7).

Is it a hardware-specific property?

> +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM)
> mode.

Could you explain meaning of this property?

> +- fsl,pwm-number: the number of PWM devices, and is must equal to the
> number +  of "fsl,pwm-channels".

This is redundant, because you can simply count how many entries have been 
specified in fsl,pwm-channels.

> +- fsl,pwm-channels: the channels' order which is be used for pwm in
> ftm0 +  module, and they must be one or some of 0 ~ 7, because the ftm0
> only has +  8 channels can be used.

Could you explain meaning of this property more precisely? I'm interested 
especially how is this related to the PWM IP block and boards.

> +- for very channel, the revlatived the pinctrl should be at least two
                           ^ typo?

> state +  {"enN", "dsN"}, which "en" means "enable", "ds" means
> "disable" and "N" +  means the order of the channel.

I'd suggest a more readable naming convention, for example chN-active and 
chN-idle. These words seem to be more common across existing bindings.

Best regards,
Tomasz
Xiubo Li-B47053 Aug. 22, 2013, 2:55 a.m. UTC | #2
Hi Tomasz,

Thanks for your comments.


> > +- #pwm-cells: Should be 3. Number of cells being used to specify PWM
> > property.
> > +  First cell specifies the per-chip channel index of the PWM
> > to use, the
> > +  second cell is the period in nanoseconds and bit 0 in
> > the third cell is
> > +  used to encode the polarity of PWM output. Set bit
> > 0 of the third in PWM
> > +  specifier to 1 for inverse polarity & set to 0
> > for normal polarity.
> 
> If the meaning of flags cell is the same as in generic, default PWM
> specifier format, then it should be noted here and generic PWM binding
> documentation mentioned.
> 

OK, How about the following ?
- #pwm-cells: Should be 3. See pwm.txt in this directory for a
  description of the cells format.

I will replace it in v2.


> > +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler,
> > divide-by 2^n(n = 0 ~ 7).
> 
> Is it a hardware-specific property?

Yes, I will revise it in v2. 

> 
> > +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM)
> > mode.
> 
> Could you explain meaning of this property?
> 

Well, this feature will be removed from the pwm core in v2.


> > +- fsl,pwm-number: the number of PWM devices, and is must equal to the
> > number +  of "fsl,pwm-channels".
> 
> This is redundant, because you can simply count how many entries have
> been specified in fsl,pwm-channels.
>

Yes, I will revise it in v2.
And this would be renamed to " fsl,pwm-channel-number", which can be more
Readable and understood.

 
> > +- fsl,pwm-channels: the channels' order which is be used for pwm in
> > ftm0 +  module, and they must be one or some of 0 ~ 7, because the
> > ftm0 only has +  8 channels can be used.
> 
> Could you explain meaning of this property more precisely? I'm interested
> especially how is this related to the PWM IP block and boards.
> 

Yes.
There are 8 channels most. While the pinctrls of 4th and 5th channels could be
used by uart's Rx and Tx, then these 2 channels won't be used for pwm output,
so there will be 6 channels available by the pwm.
Thus, the pwm chip will register only 6 pwms(6 channels) most("fsl,pwm-channel-orders
= {0 1 2 3 6 7}").And also the "fsl,pwm-channel-number" will be 6.

If hasn't any other problems, I will revise It in v2.
And this will be renamed to "fsl,pwm-channel-orders", which can be more readable
and understood.

> > +- for very channel, the revlatived the pinctrl should be at least two
>                            ^ typo?
> 
> > state +  {"enN", "dsN"}, which "en" means "enable", "ds" means
> > "disable" and "N" +  means the order of the channel.
> 
> I'd suggest a more readable naming convention, for example chN-active and
> chN-idle. These words seem to be more common across existing bindings.
> 

That's a good idea, I will think it over and revise it in v2.


Thanks very much.
--
Best Regards,
Xiubo
Sascha Hauer Aug. 22, 2013, 6:26 a.m. UTC | #3
On Thu, Aug 22, 2013 at 02:55:42AM +0000, Xiubo Li-B47053 wrote:
> Hi Tomasz,
> 
> Thanks for your comments.
> 
> 
> > Could you explain meaning of this property more precisely? I'm interested
> > especially how is this related to the PWM IP block and boards.
> > 
> 
> Yes.
> There are 8 channels most. While the pinctrls of 4th and 5th channels could be
> used by uart's Rx and Tx, then these 2 channels won't be used for pwm output,
> so there will be 6 channels available by the pwm.
> Thus, the pwm chip will register only 6 pwms(6 channels) most("fsl,pwm-channel-orders
> = {0 1 2 3 6 7}").And also the "fsl,pwm-channel-number" will be 6.

If the chip has eight PWMs I would register all of them. If some of them
are not routed out by the pinmux then just nothing happens if you use
them. In a sane devicetree they won't be referenced anyway when they are
not routed out of the SoC.

Sascha
Xiubo Li-B47053 Aug. 22, 2013, 7:32 a.m. UTC | #4
Hi Sascha,

> > > Could you explain meaning of this property more precisely? I'm
> > > interested especially how is this related to the PWM IP block and
> boards.
> > >
> >
> > Yes.
> > There are 8 channels most. While the pinctrls of 4th and 5th channels
> > could be used by uart's Rx and Tx, then these 2 channels won't be used
> > for pwm output, so there will be 6 channels available by the pwm.
> > Thus, the pwm chip will register only 6 pwms(6 channels)
> > most("fsl,pwm-channel-orders = {0 1 2 3 6 7}").And also the "fsl,pwm-
> > channel-number" will be 6.
> 
> If the chip has eight PWMs I would register all of them. If some of them
> are not routed out by the pinmux then just nothing happens if you use
> them. In a sane devicetree they won't be referenced anyway when they are
> not routed out of the SoC.
> 

Yes, that's perfect well.

I will do it in v2.

Thanks very much.
--
Best Regards,
Xiubo
Tomasz Figa Aug. 22, 2013, 8:25 a.m. UTC | #5
On Thursday 22 of August 2013 02:55:42 Xiubo Li-B47053 wrote:
> Hi Tomasz,
> 
> Thanks for your comments.
> 
> > > +- #pwm-cells: Should be 3. Number of cells being used to specify
> > > PWM
> > > property.
> > > +  First cell specifies the per-chip channel index of the PWM
> > > to use, the
> > > +  second cell is the period in nanoseconds and bit 0 in
> > > the third cell is
> > > +  used to encode the polarity of PWM output. Set bit
> > > 0 of the third in PWM
> > > +  specifier to 1 for inverse polarity & set to 0
> > > for normal polarity.
> > 
> > If the meaning of flags cell is the same as in generic, default PWM
> > specifier format, then it should be noted here and generic PWM binding
> > documentation mentioned.
> 
> OK, How about the following ?
> - #pwm-cells: Should be 3. See pwm.txt in this directory for a
>   description of the cells format.

I meant just the last cell, which stores flags, but actually this might be 
a good idea, but with slightly extended description. Something among those 
lines:

 - #pwm-cells: Should be 3. The default three cell format specified by 
generic PWM bindings are used. Refer to the documentation of generic PWM 
bindings for more information about the meaning of cells.

> I will replace it in v2.
> 
> > > +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler,
> > > divide-by 2^n(n = 0 ~ 7).
> > 
> > Is it a hardware-specific property?
> 
> Yes, I will revise it in v2.

I'd like to hear a bit more elaborate description of this property. What 
are the factors that decide what value should be used here?

> > > +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM)
> > > mode.
> > 
> > Could you explain meaning of this property?
> 
> Well, this feature will be removed from the pwm core in v2.

OK.

> > > +- fsl,pwm-number: the number of PWM devices, and is must equal to
> > > the
> > > number +  of "fsl,pwm-channels".
> > 
> > This is redundant, because you can simply count how many entries have
> > been specified in fsl,pwm-channels.
> 
> Yes, I will revise it in v2.
> And this would be renamed to " fsl,pwm-channel-number", which can be
> more Readable and understood.

I meant that you don't need to specify how many entries other property has 
in another property, because device tree provides information about sizes 
of all properties. So, in parsing code, you would be able to simply get 
the size of "fsl,pwm-channels" property in bytes, divide that by 
sizeof(u32) and get the numer of cells specified.

Also see below.

> > > +- fsl,pwm-channels: the channels' order which is be used for pwm in
> > > ftm0 +  module, and they must be one or some of 0 ~ 7, because the
> > > ftm0 only has +  8 channels can be used.
> > 
> > Could you explain meaning of this property more precisely? I'm
> > interested especially how is this related to the PWM IP block and
> > boards.
> Yes.
> There are 8 channels most. While the pinctrls of 4th and 5th channels
> could be used by uart's Rx and Tx, then these 2 channels won't be used
> for pwm output, so there will be 6 channels available by the pwm.
> Thus, the pwm chip will register only 6 pwms(6 channels)
> most("fsl,pwm-channel-orders = {0 1 2 3 6 7}").And also the
> "fsl,pwm-channel-number" will be 6.
> 
> If hasn't any other problems, I will revise It in v2.
> And this will be renamed to "fsl,pwm-channel-orders", which can be more
> readable and understood.

As Sascha Hauer already suggested, you could get rid of both this and 
"fsl,pwm-channel-number" properties and simply register all the channels. 
This way you will have a deterministic 1:1 mapping of real hardware 
channels to channels specified in device tree, which is definitely the way 
to go.

Now as a safety measure, you could simply move the specification of 
pinctrl states from SoC family or SoC level dtsi file to board level dts, 
where only pinctrl states of channels used by a particular board would be 
specified, so nothing could break operation of other devices that share 
pin muxes with remaining channels.

> > > +- for very channel, the revlatived the pinctrl should be at least
> > > two
> > > 
> >                            ^ typo?
> > > 
> > > state +  {"enN", "dsN"}, which "en" means "enable", "ds" means
> > > "disable" and "N" +  means the order of the channel.
> > 
> > I'd suggest a more readable naming convention, for example chN-active
> > and chN-idle. These words seem to be more common across existing
> > bindings.
> That's a good idea, I will think it over and revise it in v2.

OK. Looking forward to v2 then. Thanks.

Best regards,
Tomasz
Xiubo Li-B47053 Aug. 22, 2013, 9:52 a.m. UTC | #6
Hi Tomasz,



> > > If the meaning of flags cell is the same as in generic, default PWM
> > > specifier format, then it should be noted here and generic PWM
> > > binding documentation mentioned.
> >
> > OK, How about the following ?
> > - #pwm-cells: Should be 3. See pwm.txt in this directory for a
> >   description of the cells format.
> 
> I meant just the last cell, which stores flags, but actually this might
> be a good idea, but with slightly extended description. Something among
> those
> lines:
> 
>  - #pwm-cells: Should be 3. The default three cell format specified by
> generic PWM bindings are used. Refer to the documentation of generic PWM
> bindings for more information about the meaning of cells.
> 

That's perfect.


> > > > +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler,
> > > > divide-by 2^n(n = 0 ~ 7).
> > >
> > > Is it a hardware-specific property?
> >
> > Yes, I will revise it in v2.
> 
> I'd like to hear a bit more elaborate description of this property. What
> are the factors that decide what value should be used here?
> 

Sorry, "the ftm0 pwm clock's prescaler" maybe also confuse you, it should be
"the ftm pwm counter clock input prescaler".

The ftm's counter clock can be selected as :
System clock,
Fixed frequency clock,
External clock.

And the ftm module clock has only system clock source.

The fixed frequency clock is an alternative clock source for the FTM counter that allows
the selection of a clock other than the system clock or an external clock. This clock input
is defined by chip integration. Due to FTM hardware implementation limitations, the 
frequency of the fixed frequency clock must not exceed 1/2 of the system clock frequency.

The external clock passes through a synchronizer clocked by the system clock to assure
that counter transitions are properly aligned to system clock transitions.Therefore, to
meet Nyquist criteria considering also jitter, the frequency of the external clock source
must not exceed 1/4 of the system clock frequency.

So, if the fixed frequency clock or external clock equal or exceed the system clock...


> > > > +- fsl,pwm-number: the number of PWM devices, and is must equal to
> > > > the
> > > > number +  of "fsl,pwm-channels".
> > >
> > > This is redundant, because you can simply count how many entries
> > > have been specified in fsl,pwm-channels.
> >
> > Yes, I will revise it in v2.
> > And this would be renamed to " fsl,pwm-channel-number", which can be
> > more Readable and understood.
> 
> I meant that you don't need to specify how many entries other property
> has in another property, because device tree provides information about
> sizes of all properties. So, in parsing code, you would be able to simply
> get the size of "fsl,pwm-channels" property in bytes, divide that by
> sizeof(u32) and get the numer of cells specified.
> 

OK, I will revise it in v2.

> > > > +- fsl,pwm-channels: the channels' order which is be used for pwm
> > > > +in
> > > > ftm0 +  module, and they must be one or some of 0 ~ 7, because the
> > > > ftm0 only has +  8 channels can be used.
> > >
> > > Could you explain meaning of this property more precisely? I'm
> > > interested especially how is this related to the PWM IP block and
> > > boards.
> > Yes.
> > There are 8 channels most. While the pinctrls of 4th and 5th channels
> > could be used by uart's Rx and Tx, then these 2 channels won't be used
> > for pwm output, so there will be 6 channels available by the pwm.
> > Thus, the pwm chip will register only 6 pwms(6 channels)
> > most("fsl,pwm-channel-orders = {0 1 2 3 6 7}").And also the
> > "fsl,pwm-channel-number" will be 6.
> >
> > If hasn't any other problems, I will revise It in v2.
> > And this will be renamed to "fsl,pwm-channel-orders", which can be
> > more readable and understood.
> 
> As Sascha Hauer already suggested, you could get rid of both this and
> "fsl,pwm-channel-number" properties and simply register all the channels.
> This way you will have a deterministic 1:1 mapping of real hardware
> channels to channels specified in device tree, which is definitely the
> way to go.
> 
> Now as a safety measure, you could simply move the specification of
> pinctrl states from SoC family or SoC level dtsi file to board level dts,
> where only pinctrl states of channels used by a particular board would be
> specified, so nothing could break operation of other devices that share
> pin muxes with remaining channels.

Yes, I was also considering it, but not very be clear yet.
Thanks very much for your and Sascha's help.
I will try to implement it in v2.


Thanks very much.

--
Best regards,
Xiubo
Tomasz Figa Aug. 22, 2013, 12:17 p.m. UTC | #7
On Thursday 22 of August 2013 09:52:42 Xiubo Li-B47053 wrote:
> Hi Tomasz,
> 
> > > > If the meaning of flags cell is the same as in generic, default PWM
> > > > specifier format, then it should be noted here and generic PWM
> > > > binding documentation mentioned.
> > > 
> > > OK, How about the following ?
> > > - #pwm-cells: Should be 3. See pwm.txt in this directory for a
> > > 
> > >   description of the cells format.
> > 
> > I meant just the last cell, which stores flags, but actually this might
> > be a good idea, but with slightly extended description. Something among
> > those
> > 
> > lines:
> >  - #pwm-cells: Should be 3. The default three cell format specified by
> > 
> > generic PWM bindings are used. Refer to the documentation of generic
> > PWM
> > bindings for more information about the meaning of cells.
> 
> That's perfect.

OK.

> > > > > +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler,
> > > > > divide-by 2^n(n = 0 ~ 7).
> > > > 
> > > > Is it a hardware-specific property?
> > > 
> > > Yes, I will revise it in v2.
> > 
> > I'd like to hear a bit more elaborate description of this property.
> > What
> > are the factors that decide what value should be used here?
> 
> Sorry, "the ftm0 pwm clock's prescaler" maybe also confuse you, it should
> be "the ftm pwm counter clock input prescaler".
> 
> The ftm's counter clock can be selected as :
> System clock,
> Fixed frequency clock,
> External clock.
> 
> And the ftm module clock has only system clock source.
> 
> The fixed frequency clock is an alternative clock source for the FTM
> counter that allows the selection of a clock other than the system clock
> or an external clock. This clock input is defined by chip integration.
> Due to FTM hardware implementation limitations, the frequency of the
> fixed frequency clock must not exceed 1/2 of the system clock frequency.
> 
> The external clock passes through a synchronizer clocked by the system
> clock to assure that counter transitions are properly aligned to system
> clock transitions.Therefore, to meet Nyquist criteria considering also
> jitter, the frequency of the external clock source must not exceed 1/4
> of the system clock frequency.
> 
> So, if the fixed frequency clock or external clock equal or exceed the
> system clock...

Can't the driver check the frequency of fixed or external clock and 
calculate optimal divisor value so that it is less than 1/4 of system clock 
frequency?

> > > > > +- fsl,pwm-number: the number of PWM devices, and is must equal
> > > > > to
> > > > > the
> > > > > number +  of "fsl,pwm-channels".
> > > > 
> > > > This is redundant, because you can simply count how many entries
> > > > have been specified in fsl,pwm-channels.
> > > 
> > > Yes, I will revise it in v2.
> > > And this would be renamed to " fsl,pwm-channel-number", which can be
> > > more Readable and understood.
> > 
> > I meant that you don't need to specify how many entries other property
> > has in another property, because device tree provides information about
> > sizes of all properties. So, in parsing code, you would be able to
> > simply get the size of "fsl,pwm-channels" property in bytes, divide
> > that by sizeof(u32) and get the numer of cells specified.
> 
> OK, I will revise it in v2.

As I noted below, it won't be needed anymore. I just commented on this as a 
side note.

> > > > > +- fsl,pwm-channels: the channels' order which is be used for pwm
> > > > > +in
> > > > > ftm0 +  module, and they must be one or some of 0 ~ 7, because
> > > > > the
> > > > > ftm0 only has +  8 channels can be used.
> > > > 
> > > > Could you explain meaning of this property more precisely? I'm
> > > > interested especially how is this related to the PWM IP block and
> > > > boards.
> > > 
> > > Yes.
> > > There are 8 channels most. While the pinctrls of 4th and 5th channels
> > > could be used by uart's Rx and Tx, then these 2 channels won't be
> > > used
> > > for pwm output, so there will be 6 channels available by the pwm.
> > > Thus, the pwm chip will register only 6 pwms(6 channels)
> > > most("fsl,pwm-channel-orders = {0 1 2 3 6 7}").And also the
> > > "fsl,pwm-channel-number" will be 6.
> > > 
> > > If hasn't any other problems, I will revise It in v2.
> > > And this will be renamed to "fsl,pwm-channel-orders", which can be
> > > more readable and understood.
> > 
> > As Sascha Hauer already suggested, you could get rid of both this and
> > "fsl,pwm-channel-number" properties and simply register all the
> > channels. This way you will have a deterministic 1:1 mapping of real
> > hardware channels to channels specified in device tree, which is
> > definitely the way to go.
> > 
> > Now as a safety measure, you could simply move the specification of
> > pinctrl states from SoC family or SoC level dtsi file to board level
> > dts, where only pinctrl states of channels used by a particular board
> > would be specified, so nothing could break operation of other devices
> > that share pin muxes with remaining channels.
> 
> Yes, I was also considering it, but not very be clear yet.
> Thanks very much for your and Sascha's help.
> I will try to implement it in v2.

OK, good.

Best regards,
Tomasz
Thierry Reding Aug. 23, 2013, 7:36 a.m. UTC | #8
On Thu, Aug 22, 2013 at 08:26:10AM +0200, Sascha Hauer wrote:
> On Thu, Aug 22, 2013 at 02:55:42AM +0000, Xiubo Li-B47053 wrote:
> > Hi Tomasz,
> > 
> > Thanks for your comments.
> > 
> > 
> > > Could you explain meaning of this property more precisely? I'm interested
> > > especially how is this related to the PWM IP block and boards.
> > > 
> > 
> > Yes.
> > There are 8 channels most. While the pinctrls of 4th and 5th channels could be
> > used by uart's Rx and Tx, then these 2 channels won't be used for pwm output,
> > so there will be 6 channels available by the pwm.
> > Thus, the pwm chip will register only 6 pwms(6 channels) most("fsl,pwm-channel-orders
> > = {0 1 2 3 6 7}").And also the "fsl,pwm-channel-number" will be 6.
> 
> If the chip has eight PWMs I would register all of them. If some of them
> are not routed out by the pinmux then just nothing happens if you use
> them. In a sane devicetree they won't be referenced anyway when they are
> not routed out of the SoC.

In that case, shouldn't this be hooked up to the pinctrl subsystem as
well? As I understand the above, the logical thing would be for each PWM
channel's .request() operation to configure the pinmuxing appropriately.
And if it can't be configured as necessary then .request() should return
an error (or propagate the error from the pinctrl subsystem).

Thierry
Thierry Reding Aug. 23, 2013, 8:04 a.m. UTC | #9
On Thu, Aug 22, 2013 at 10:25:30AM +0200, Tomasz Figa wrote:
> On Thursday 22 of August 2013 02:55:42 Xiubo Li-B47053 wrote:
> > Hi Tomasz,
> > 
> > Thanks for your comments.
> > 
> > > > +- #pwm-cells: Should be 3. Number of cells being used to specify
> > > > PWM
> > > > property.
> > > > +  First cell specifies the per-chip channel index of the PWM
> > > > to use, the
> > > > +  second cell is the period in nanoseconds and bit 0 in
> > > > the third cell is
> > > > +  used to encode the polarity of PWM output. Set bit
> > > > 0 of the third in PWM
> > > > +  specifier to 1 for inverse polarity & set to 0
> > > > for normal polarity.
> > > 
> > > If the meaning of flags cell is the same as in generic, default PWM
> > > specifier format, then it should be noted here and generic PWM binding
> > > documentation mentioned.
> > 
> > OK, How about the following ?
> > - #pwm-cells: Should be 3. See pwm.txt in this directory for a
> >   description of the cells format.
> 
> I meant just the last cell, which stores flags, but actually this might be 
> a good idea, but with slightly extended description. Something among those 
> lines:
> 
>  - #pwm-cells: Should be 3. The default three cell format specified by 
> generic PWM bindings are used. Refer to the documentation of generic PWM 
> bindings for more information about the meaning of cells.

Actually I prefer the second proposal, that is:

> > - #pwm-cells: Should be 3. See pwm.txt in this directory for a
> >   description of the cells format.

We agreed on that wording in another thread and I'd prefer to be
consistent across bindings.

Thierry
Thierry Reding Aug. 23, 2013, 9:10 a.m. UTC | #10
On Wed, Aug 21, 2013 at 11:07:42AM +0800, Xiubo Li wrote:
> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> ---
>  .../devicetree/bindings/pwm/fsl-ftm-pwm.txt        | 52 ++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt

It used to be that device tree bindings documentation was squashed into
the same commit as the driver. I see some point in splitting this up
given that this will eventually go into a completely separate tree, but
I just want to make sure this is now the official way of splitting
patches so I can tell that to people in good conscience.

> diff --git a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> new file mode 100644
> index 0000000..698965b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> @@ -0,0 +1,52 @@
> +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. Number of cells being used to specify PWM property.
> +  First cell specifies the per-chip channel index of the PWM to use, the
> +  second cell is the period in nanoseconds and bit 0 in the third cell is
> +  used to encode the polarity of PWM output. Set bit 0 of the third in PWM
> +  specifier to 1 for inverse polarity & set to 0 for normal polarity.
> +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by 2^n(n = 0 ~ 7).
> +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.
> +- fsl,pwm-number: the number of PWM devices, and is must equal to the number
> +  of "fsl,pwm-channels".
> +- fsl,pwm-channels: the channels' order which is be used for pwm in ftm0
> +  module, and they must be one or some of 0 ~ 7, because the ftm0 only has
> +  8 channels can be used.
> +- for very channel, the revlatived the pinctrl should be at least two state
> +  {"enN", "dsN"}, which "en" means "enable", "ds" means "disable" and "N"
> +  means the order of the channel.

This is missing a description of the clocks and clock-names properties.

> +
> +Example:
> +
> +pwm0: pwm@40038000 {
> +	      compatible = "fsl,vf610-ftm-pwm";
> +	      reg = <0x40038000 0x1000>;
> +	      #pwm-cells = <3>;
> +	      pinctrl-names = "en0", "ds0", "en3", "ds3";
> +	      pinctrl-0 = <&pinctrl_pwm0_ch0_en>;
> +	      pinctrl-1 = <&pinctrl_pwm0_ch0_ds>;
> +	      pinctrl-2 = <&pinctrl_pwm0_ch3_en>;
> +	      pinctrl-3 = <&pinctrl_pwm0_ch3_ds>;
> +	      fsl,pwm-clk-ps = <7>;
> +	      fsl,pwm-cpwm = <0>;
> +	      fsl,pwm-number = <2>;
> +	      fsl,pwm-channels = <0 3>;
> +	      ...

And this mixes tabs and spaces for indentation.

> +      };
> +
> +leds {
> +	compatible = "pwm-leds";
> +	led {
> +		label = "fsl_led";
> +		pwms = <&pwm0 0 10000000 0>;
> +		max-brightness = <127>;
> +	};
> +	backlight {
> +		label = "fsl_backlight";
> +		pwms = <&pwm0 3 10000000 1>;
> +		max-brightness = <100>;
> +	};

I don't think I like this at all. This example suggests to use the
pwm-leds driver for backlight control. Why not use pwm-backlight
instead?

Thierry
Stephen Warren Aug. 23, 2013, 7:29 p.m. UTC | #11
On 08/23/2013 01:36 AM, Thierry Reding wrote:
> On Thu, Aug 22, 2013 at 08:26:10AM +0200, Sascha Hauer wrote:
>> On Thu, Aug 22, 2013 at 02:55:42AM +0000, Xiubo Li-B47053 wrote:
>>> Hi Tomasz,
>>> 
>>> Thanks for your comments.
>>> 
>>> 
>>>> Could you explain meaning of this property more precisely?
>>>> I'm interested especially how is this related to the PWM IP
>>>> block and boards.
>>>> 
>>> 
>>> Yes. There are 8 channels most. While the pinctrls of 4th and
>>> 5th channels could be used by uart's Rx and Tx, then these 2
>>> channels won't be used for pwm output, so there will be 6
>>> channels available by the pwm. Thus, the pwm chip will register
>>> only 6 pwms(6 channels) most("fsl,pwm-channel-orders = {0 1 2 3
>>> 6 7}").And also the "fsl,pwm-channel-number" will be 6.
>> 
>> If the chip has eight PWMs I would register all of them. If some
>> of them are not routed out by the pinmux then just nothing
>> happens if you use them. In a sane devicetree they won't be
>> referenced anyway when they are not routed out of the SoC.
> 
> In that case, shouldn't this be hooked up to the pinctrl subsystem
> as well? As I understand the above, the logical thing would be for
> each PWM channel's .request() operation to configure the pinmuxing
> appropriately. And if it can't be configured as necessary then
> .request() should return an error (or propagate the error from the
> pinctrl subsystem).

I think the pin-muxing should be static, i.e. set up when the PWM
device as a whole probe()s, rather than being twiddled at request/free
time. Certainly the pinmux support in the device core is now set up to
acquire the default state right before probe(). I don't see a need to
do anything custom here.
Stephen Warren Aug. 23, 2013, 7:36 p.m. UTC | #12
On 08/23/2013 03:10 AM, Thierry Reding wrote:
> On Wed, Aug 21, 2013 at 11:07:42AM +0800, Xiubo Li wrote:
>> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com> --- 
>> .../devicetree/bindings/pwm/fsl-ftm-pwm.txt        | 52
>> ++++++++++++++++++++++ 1 file changed, 52 insertions(+) create
>> mode 100644
>> Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt

>> diff --git
>> a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
>> b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt

>> +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. Number of
>> cells being used to specify PWM property. +  First cell specifies
>> the per-chip channel index of the PWM to use, the +  second cell
>> is the period in nanoseconds and bit 0 in the third cell is +
>> used to encode the polarity of PWM output. Set bit 0 of the third
>> in PWM +  specifier to 1 for inverse polarity & set to 0 for
>> normal polarity.

Lines 2..n of a property description should be indented so it's easier
to see where each entry in the list starts.

>> +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by
>> 2^n(n = 0 ~ 7). +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.

>> +- fsl,pwm-number: the number of PWM devices, and is must equal
>> to the number +  of "fsl,pwm-channels".

Isn't that value a static facet of the HW, and hence it can be
determined solely from the compatible value?

>> +- fsl,pwm-channels: the channels' order which is be used for pwm
>> in ftm0 +  module, and they must be one or some of 0 ~ 7, because
>> the ftm0 only has +  8 channels can be used.

Why is there a need to re-order the channels? Why not simply reference
the actual physical channel IDs in client nodes?

>> +- for very channel, the revlatived the pinctrl should be at
>> least two state +  {"enN", "dsN"}, which "en" means "enable",
>> "ds" means "disable" and "N" +  means the order of the channel.

revlatived??

Why is there a need for pinctrl interaction at all?

Is the "N" in these property names the index into fsl,pwm-channels, or
the physical channel number in the controller HW, or something else?

"dis" would be better than "ds", although "enable-chN", "disable-chN"
would be even better.

Note that it's not possible to enable multiple pinctrl states at once,
so what happens when channel 0 is enabled, yet channel 1 is disabled,
and you want to enable "en0" and "ds1" at the same time?
Xiubo Li-B47053 Aug. 26, 2013, 5:35 a.m. UTC | #13
Hi Stephen,


> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
> Freescale FTM PWM
> 
> On 08/23/2013 01:36 AM, Thierry Reding wrote:
> > On Thu, Aug 22, 2013 at 08:26:10AM +0200, Sascha Hauer wrote:
> >> On Thu, Aug 22, 2013 at 02:55:42AM +0000, Xiubo Li-B47053 wrote:
> >>> Hi Tomasz,
> >>>
> >>> Thanks for your comments.
> >>>
> >>>
> >>>> Could you explain meaning of this property more precisely?
> >>>> I'm interested especially how is this related to the PWM IP block
> >>>> and boards.
> >>>>
> >>>
> >>> Yes. There are 8 channels most. While the pinctrls of 4th and 5th
> >>> channels could be used by uart's Rx and Tx, then these 2 channels
> >>> won't be used for pwm output, so there will be 6 channels available
> >>> by the pwm. Thus, the pwm chip will register only 6 pwms(6 channels)
> >>> most("fsl,pwm-channel-orders = {0 1 2 3
> >>> 6 7}").And also the "fsl,pwm-channel-number" will be 6.
> >>
> >> If the chip has eight PWMs I would register all of them. If some of
> >> them are not routed out by the pinmux then just nothing happens if
> >> you use them. In a sane devicetree they won't be referenced anyway
> >> when they are not routed out of the SoC.
> >
> > In that case, shouldn't this be hooked up to the pinctrl subsystem as
> > well? As I understand the above, the logical thing would be for each
> > PWM channel's .request() operation to configure the pinmuxing
> > appropriately. And if it can't be configured as necessary then
> > .request() should return an error (or propagate the error from the
> > pinctrl subsystem).
> 
> I think the pin-muxing should be static, i.e. set up when the PWM device
> as a whole probe()s, rather than being twiddled at request/free time.
> Certainly the pinmux support in the device core is now set up to acquire
> the default state right before probe(). I don't see a need to do anything
> custom here.

As we have tolk about this before in [PATCH 1/4]:
"
> > Why do you need to manipulate the pinctrl to en/disable a channel?
> >
> 
> This is because in Vybrid VF610 TOWER board, there are 4 leds, and each
> led's one point(diode's positive pole) is connected to 3.3V, and the
> other point is connected to pwm's one channel. When the 4 pinctrls are
> configured as enable at the same time, the 4 pinctrls is low valtage, and
> the 4 leds will be lighted up as default, then when you enable/disable
> one led will effects others.
> 
> These pinctrls are belong to pwm, and I don't think led or other customer
> could control them directly.
> So, here I authorize the 4 pinctrls to each channel controls.
>
"
For the reason above, I have to control the pinctrls separately.

If all the pinctrls set as default state, the 8 pinctrls must be controlled together.
And the 4 leds will all be lighted up as default and will influence each other.

Thanks very much.

--
Best Regards.
Xiubo
Xiubo Li-B47053 Aug. 26, 2013, 5:46 a.m. UTC | #14
Hi Thierry,


> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
> Freescale FTM PWM
> 
> On Thu, Aug 22, 2013 at 08:26:10AM +0200, Sascha Hauer wrote:
> > On Thu, Aug 22, 2013 at 02:55:42AM +0000, Xiubo Li-B47053 wrote:
> > > Hi Tomasz,
> > >
> > > Thanks for your comments.
> > >
> > >
> > > > Could you explain meaning of this property more precisely? I'm
> > > > interested especially how is this related to the PWM IP block and
> boards.
> > > >
> > >
> > > Yes.
> > > There are 8 channels most. While the pinctrls of 4th and 5th
> > > channels could be used by uart's Rx and Tx, then these 2 channels
> > > won't be used for pwm output, so there will be 6 channels available
> by the pwm.
> > > Thus, the pwm chip will register only 6 pwms(6 channels)
> > > most("fsl,pwm-channel-orders = {0 1 2 3 6 7}").And also the "fsl,pwm-
> channel-number" will be 6.
> >
> > If the chip has eight PWMs I would register all of them. If some of
> > them are not routed out by the pinmux then just nothing happens if you
> > use them. In a sane devicetree they won't be referenced anyway when
> > they are not routed out of the SoC.
> 
> In that case, shouldn't this be hooked up to the pinctrl subsystem as
> well? As I understand the above, the logical thing would be for each PWM
> channel's .request() operation to configure the pinmuxing appropriately.
> And if it can't be configured as necessary then .request() should return
> an error (or propagate the error from the pinctrl subsystem).
> 

That's maybe better, if so, the pinctrl configuration must be split into two steps:
1, get the channel pinctrl "active" and "idle" states by callig pinctrl_lookup_state() in .request().
2, select the proper state in .enable()/.disable().




Thanks very much.

--
Best Regards.
Xiubo
Stephen Warren Aug. 26, 2013, 8:01 p.m. UTC | #15
On 08/25/2013 11:35 PM, Xiubo Li-B47053 wrote:
>> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
...
>>> Why do you need to manipulate the pinctrl to en/disable a channel?
>>
>> This is because in Vybrid VF610 TOWER board, there are 4 leds, and each
>> led's one point(diode's positive pole) is connected to 3.3V, and the
>> other point is connected to pwm's one channel. When the 4 pinctrls are
>> configured as enable at the same time, the 4 pinctrls is low valtage, and
>> the 4 leds will be lighted up as default, then when you enable/disable
>> one led will effects others.
>>
>> These pinctrls are belong to pwm, and I don't think led or other customer
>> could control them directly.
>> So, here I authorize the 4 pinctrls to each channel controls.
>>
> "
> For the reason above, I have to control the pinctrls separately.
> 
> If all the pinctrls set as default state, the 8 pinctrls must be controlled together.
> And the 4 leds will all be lighted up as default and will influence each other.

Sorry, that still doesn't make much sense. Either way though, having
separate pinctrl setup for a single device isn't going to work. You'll
either need to have all combinations of 4 (8?) PWMs represented as
pinctrl states(!), or register separate PWM devices so that they get
independant pinctrl states.
Xiubo Li-B47053 Aug. 27, 2013, 3:48 a.m. UTC | #16
Hi Stephen,


> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
> Freescale FTM PWM
> 
> On 08/25/2013 11:35 PM, Xiubo Li-B47053 wrote:
> >> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
> ...
> >>> Why do you need to manipulate the pinctrl to en/disable a channel?
> >>
> >> This is because in Vybrid VF610 TOWER board, there are 4 leds, and
> >> each led's one point(diode's positive pole) is connected to 3.3V, and
> >> the other point is connected to pwm's one channel. When the 4
> >> pinctrls are configured as enable at the same time, the 4 pinctrls is
> >> low valtage, and the 4 leds will be lighted up as default, then when
> >> you enable/disable one led will effects others.
> >>
> >> These pinctrls are belong to pwm, and I don't think led or other
> >> customer could control them directly.
> >> So, here I authorize the 4 pinctrls to each channel controls.
> >>
> > "
> > For the reason above, I have to control the pinctrls separately.
> >
> > If all the pinctrls set as default state, the 8 pinctrls must be
> controlled together.
> > And the 4 leds will all be lighted up as default and will influence
> each other.
> 
> Sorry, that still doesn't make much sense. Either way though, having
> separate pinctrl setup for a single device isn't going to work. You'll
> either need to have all combinations of 4 (8?) PWMs represented as
> pinctrl states(!), or register separate PWM devices so that they get
> independant pinctrl states.
>
Well, I have ever thought about registering separate PWM device for each channel.
But, if so, how should I control the pinctrl of each PWM(actually one channel of FTM PWM) ?
I must select "default" state(the "default" state here, the pinctrl must be setup as "dsN" or "chN-idle" state we discussed before) when probing, and when customers .request-->.config-->.enable the PWM I also must select an "active" state to config the pinctrl...
Thus, this is still not static. I think this isn't much different from the current.

Also if having all combinations of 8 PWMs represented as pinctrl states, how could I deal with the problem about LEDs ?



Thanks very much,

--
Best Regards,
Xiubo
Stephen Warren Aug. 27, 2013, 4:04 a.m. UTC | #17
On 08/26/2013 09:48 PM, Xiubo Li-B47053 wrote:
> Hi Stephen,
> 
> 
>> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
>> Freescale FTM PWM
>>
>> On 08/25/2013 11:35 PM, Xiubo Li-B47053 wrote:
>>>> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
>> ...
>>>>> Why do you need to manipulate the pinctrl to en/disable a channel?
>>>>
>>>> This is because in Vybrid VF610 TOWER board, there are 4 leds, and
>>>> each led's one point(diode's positive pole) is connected to 3.3V, and
>>>> the other point is connected to pwm's one channel. When the 4
>>>> pinctrls are configured as enable at the same time, the 4 pinctrls is
>>>> low valtage, and the 4 leds will be lighted up as default, then when
>>>> you enable/disable one led will effects others.
>>>>
>>>> These pinctrls are belong to pwm, and I don't think led or other
>>>> customer could control them directly.
>>>> So, here I authorize the 4 pinctrls to each channel controls.
>>>>
>>> "
>>> For the reason above, I have to control the pinctrls separately.
>>>
>>> If all the pinctrls set as default state, the 8 pinctrls must be
>> controlled together.
>>> And the 4 leds will all be lighted up as default and will influence
>> each other.
>>
>> Sorry, that still doesn't make much sense. ...

What may help here is to show the exact content you expect the various
en/dis states to have, so that we can see exactly which pinctrl options
get set when and why.
Kumar Gala Aug. 30, 2013, 7:19 p.m. UTC | #18
Should have at least something w/regards to a commit message.

On Aug 20, 2013, at 10:07 PM, Xiubo Li wrote:

> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> ---
> .../devicetree/bindings/pwm/fsl-ftm-pwm.txt        | 52 ++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> new file mode 100644
> index 0000000..698965b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> @@ -0,0 +1,52 @@
> +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. Number of cells being used to specify PWM property.
> +  First cell specifies the per-chip channel index of the PWM to use, the
> +  second cell is the period in nanoseconds and bit 0 in the third cell is
> +  used to encode the polarity of PWM output. Set bit 0 of the third in PWM
> +  specifier to 1 for inverse polarity & set to 0 for normal polarity.
> +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by 2^n(n = 0 ~ 7).
> +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.

Should describe this in more detail, what does the value actually mean for what modes there are?

> +- fsl,pwm-number: the number of PWM devices, and is must equal to the number
> +  of "fsl,pwm-channels".

If they must be equal why do we need both?

> +- fsl,pwm-channels: the channels' order which is be used for pwm in ftm0
> +  module, and they must be one or some of 0 ~ 7, because the ftm0 only has
> +  8 channels can be used.
> +- for very channel, the revlatived the pinctrl should be at least two state

revlatived?

> +  {"enN", "dsN"}, which "en" means "enable", "ds" means "disable" and "N"
> +  means the order of the channel.
> +
> +Example:
> +
> +pwm0: pwm@40038000 {
> +	      compatible = "fsl,vf610-ftm-pwm";
> +	      reg = <0x40038000 0x1000>;
> +	      #pwm-cells = <3>;
> +	      pinctrl-names = "en0", "ds0", "en3", "ds3";
> +	      pinctrl-0 = <&pinctrl_pwm0_ch0_en>;
> +	      pinctrl-1 = <&pinctrl_pwm0_ch0_ds>;
> +	      pinctrl-2 = <&pinctrl_pwm0_ch3_en>;
> +	      pinctrl-3 = <&pinctrl_pwm0_ch3_ds>;
> +	      fsl,pwm-clk-ps = <7>;
> +	      fsl,pwm-cpwm = <0>;
> +	      fsl,pwm-number = <2>;
> +	      fsl,pwm-channels = <0 3>;
> +	      ...
> +      };
> +
> +leds {
> +	compatible = "pwm-leds";
> +	led {
> +		label = "fsl_led";
> +		pwms = <&pwm0 0 10000000 0>;
> +		max-brightness = <127>;
> +	};
> +	backlight {
> +		label = "fsl_backlight";
> +		pwms = <&pwm0 3 10000000 1>;
> +		max-brightness = <100>;
> +	};
> +};
> -- 
> 1.8.0
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
Stephen Warren Aug. 30, 2013, 8:11 p.m. UTC | #19
On 08/30/2013 01:19 PM, Kumar Gala wrote:
> Should have at least something w/regards to a commit message.
> 
> On Aug 20, 2013, at 10:07 PM, Xiubo Li wrote:
> 
>> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
>> ---
>> .../devicetree/bindings/pwm/fsl-ftm-pwm.txt        | 52 ++++++++++++++++++++++
>> 1 file changed, 52 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
>> new file mode 100644
>> index 0000000..698965b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
>> @@ -0,0 +1,52 @@
>> +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. Number of cells being used to specify PWM property.
>> +  First cell specifies the per-chip channel index of the PWM to use, the
>> +  second cell is the period in nanoseconds and bit 0 in the third cell is
>> +  used to encode the polarity of PWM output. Set bit 0 of the third in PWM
>> +  specifier to 1 for inverse polarity & set to 0 for normal polarity.
>> +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by 2^n(n = 0 ~ 7).
>> +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.
> 
> Should describe this in more detail, what does the value actually mean for what modes there are?

Assuming "CPWM" is clearly explained in the HW documentation for this
chip (I have no idea if that's actually the case), then is it still
necessary to explain what this means in *detail*? Perhaps simply "see
section XXX in the TRM" or "see register XXX, bit YYY in the HW
documentation" would be enough?
Xiubo Li-B47053 Sept. 2, 2013, 2:18 a.m. UTC | #20
> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
> Freescale FTM PWM
> 
> Should have at least something w/regards to a commit message.
> 

I have sent a V2 patch and have added it.


> > +  used to encode the polarity of PWM output. Set bit 0 of the third
> > +in PWM
> > +  specifier to 1 for inverse polarity & set to 0 for normal polarity.
> > +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by 2^n(n = 0
> ~ 7).
> > +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.
> 
> Should describe this in more detail, what does the value actually mean
> for what modes there are?
> 

It's maybe a very useful feature for FTM core, but for PWM is not.
This isn't needed any more for PWM, and in V2 patch I have removed CPWM mode.

> > +- fsl,pwm-number: the number of PWM devices, and is must equal to the
> > +number
> > +  of "fsl,pwm-channels".
> 
> If they must be equal why do we need both?
> 

I have replaced both of them too in V2.

> > +- fsl,pwm-channels: the channels' order which is be used for pwm in
> > +ftm0
> > +  module, and they must be one or some of 0 ~ 7, because the ftm0
> > +only has
> > +  8 channels can be used.
> > +- for very channel, the revlatived the pinctrl should be at least two
> > +state
> 
> revlatived?
> 

This too.

--
Best Regards.
Xiubo
Xiubo Li-B47053 Sept. 3, 2013, 5:25 a.m. UTC | #21
> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
> Freescale FTM PWM
> 
> On 08/30/2013 01:19 PM, Kumar Gala wrote:
> > Should have at least something w/regards to a commit message.
> >
> > On Aug 20, 2013, at 10:07 PM, Xiubo Li wrote:
> >
> >> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> >> ---
> >> .../devicetree/bindings/pwm/fsl-ftm-pwm.txt        | 52
> ++++++++++++++++++++++
> >> 1 file changed, 52 insertions(+)
> >> create mode 100644
> >> Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> >> b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> >> new file mode 100644
> >> index 0000000..698965b
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> >> @@ -0,0 +1,52 @@
> >> +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. Number of cells being used to specify PWM
> property.
> >> +  First cell specifies the per-chip channel index of the PWM to use,
> >> +the
> >> +  second cell is the period in nanoseconds and bit 0 in the third
> >> +cell is
> >> +  used to encode the polarity of PWM output. Set bit 0 of the third
> >> +in PWM
> >> +  specifier to 1 for inverse polarity & set to 0 for normal polarity.
> >> +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by 2^n(n = 0
> ~ 7).
> >> +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.
> >
> > Should describe this in more detail, what does the value actually mean
> for what modes there are?
> 
> Assuming "CPWM" is clearly explained in the HW documentation for this
> chip (I have no idea if that's actually the case), then is it still
> necessary to explain what this means in *detail*? Perhaps simply "see
> section XXX in the TRM" or "see register XXX, bit YYY in the HW
> documentation" would be enough?
>
If to clearly explain the 'CPWM' mode, there maybe need much more words, I think just simply explain it, and then for more detail information "see section XXX in the TRM" or "see register XXX, bit YYY in HW documentation".


Thanks.

--
Best Regards,
Xiubo
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
new file mode 100644
index 0000000..698965b
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
@@ -0,0 +1,52 @@ 
+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. Number of cells being used to specify PWM property.
+  First cell specifies the per-chip channel index of the PWM to use, the
+  second cell is the period in nanoseconds and bit 0 in the third cell is
+  used to encode the polarity of PWM output. Set bit 0 of the third in PWM
+  specifier to 1 for inverse polarity & set to 0 for normal polarity.
+- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by 2^n(n = 0 ~ 7).
+- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.
+- fsl,pwm-number: the number of PWM devices, and is must equal to the number
+  of "fsl,pwm-channels".
+- fsl,pwm-channels: the channels' order which is be used for pwm in ftm0
+  module, and they must be one or some of 0 ~ 7, because the ftm0 only has
+  8 channels can be used.
+- for very channel, the revlatived the pinctrl should be at least two state
+  {"enN", "dsN"}, which "en" means "enable", "ds" means "disable" and "N"
+  means the order of the channel.
+
+Example:
+
+pwm0: pwm@40038000 {
+	      compatible = "fsl,vf610-ftm-pwm";
+	      reg = <0x40038000 0x1000>;
+	      #pwm-cells = <3>;
+	      pinctrl-names = "en0", "ds0", "en3", "ds3";
+	      pinctrl-0 = <&pinctrl_pwm0_ch0_en>;
+	      pinctrl-1 = <&pinctrl_pwm0_ch0_ds>;
+	      pinctrl-2 = <&pinctrl_pwm0_ch3_en>;
+	      pinctrl-3 = <&pinctrl_pwm0_ch3_ds>;
+	      fsl,pwm-clk-ps = <7>;
+	      fsl,pwm-cpwm = <0>;
+	      fsl,pwm-number = <2>;
+	      fsl,pwm-channels = <0 3>;
+	      ...
+      };
+
+leds {
+	compatible = "pwm-leds";
+	led {
+		label = "fsl_led";
+		pwms = <&pwm0 0 10000000 0>;
+		max-brightness = <127>;
+	};
+	backlight {
+		label = "fsl_backlight";
+		pwms = <&pwm0 3 10000000 1>;
+		max-brightness = <100>;
+	};
+};