linux-kernel - RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM 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]

Date:	Thu, 22 Aug 2013 02:55:42 +0000
From:	Xiubo Li-B47053 <B47053@...escale.com>
To:	Tomasz Figa <tomasz.figa@...il.com>
CC:	Guo Shawn-R65073 <r65073@...escale.com>,
	"thierry.reding@...il.com" <thierry.reding@...il.com>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"rob@...dley.net" <rob@...dley.net>,
	"ian.campbell@...rix.com" <ian.campbell@...rix.com>,
	"swarren@...dotorg.org" <swarren@...dotorg.org>,
	"mark.rutland@....com" <mark.rutland@....com>,
	"pawel.moll@....com" <pawel.moll@....com>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>
Subject: RE: [PATCH 4/4] Documentation: Add device tree bindings for
 Freescale FTM PWM

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

--
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/