RE: [RFC][PATCHv5 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.
> > This adds the Document for Freescale FTM PWM driver under > > Documentation/devicetree/bindings/pwm/. > > > > Signed-off-by: Xiubo Li > > --- > > .../devicetree/bindings/pwm/pwm-fsl-ftm.txt| 34 > ++ > > 1 file changed, 34 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt > > > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt > > b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt > > new file mode 100644 > > index 000..175b762 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt > > @@ -0,0 +1,34 @@ > > +Freescale FTM PWM controller > > What does FTM stand for, and can we spell out PWM at least once early on. > "FTM" is for short of "FlexTimer Module", I'll use the full name then. > > + > > +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. See pwm.txt in this directory for a > > +description of > > + the cells format. > > +- clock-names : Includes the following module clock source entries: > > +"ftm0" (system clock), > > +"ftm0_fix_sel" (fixed frequency clock), > > +"ftm0_ext_sel" (external clock) > > +- clocks : Must contain a clock specifier for each entry in > > +clock-names, > > + See ../clock/clock-bindings.txt for details of the property values. > > +- fsl,pwm-counter-clk: The FTM PWM counter clock source, should be > > +one of the > > + entries in clock-names. > > Why do we need this, why not just have only the clock-names/clocks > reference the clk that is actually used? > As I have replied before, the FTM has two clock sources: the module clock and the counter clock. The counter clock source is selectable depends on different board and also the hardware design: + * FTM source clock is selectable * Source clock can be the system clock, the fixed frequency clock, or an external clock * Fixed frequency clock is an additional clock input to allow the selection of an on chip clock source other than the system clock * Selecting external clock connects FTM clock to a chip level input pin therefore allowing to synchronize the FTM counter with an off chip clock source - >From the above description we can see that the external clock source can allow >to synchronize the FTM counter with an off chip clock source. As the chip spec permits the counter clock source be selectable, so the different board maybe has different implementation, if the driver do not support, this will be a bug. > > +- pinctrl-names: must contain a "default" entry. > > +- pinctrl-NNN: One property must exist for each entry in pinctrl-names. > > + See ../pinctrl/pinctrl-bindings.txt for details of the property > values. > > let's drop the .. when making directory references. > Is absolute path " Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt " or just "pinctrl/pinctrl-bindings.txt " ? -- Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [RFC][PATCHv5 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.
This adds the Document for Freescale FTM PWM driver under Documentation/devicetree/bindings/pwm/. Signed-off-by: Xiubo Li li.xi...@freescale.com --- .../devicetree/bindings/pwm/pwm-fsl-ftm.txt| 34 ++ 1 file changed, 34 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt diff --git a/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt new file mode 100644 index 000..175b762 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt @@ -0,0 +1,34 @@ +Freescale FTM PWM controller What does FTM stand for, and can we spell out PWM at least once early on. FTM is for short of FlexTimer Module, I'll use the full name then. + +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. See pwm.txt in this directory for a +description of + the cells format. +- clock-names : Includes the following module clock source entries: +ftm0 (system clock), +ftm0_fix_sel (fixed frequency clock), +ftm0_ext_sel (external clock) +- clocks : Must contain a clock specifier for each entry in +clock-names, + See ../clock/clock-bindings.txt for details of the property values. +- fsl,pwm-counter-clk: The FTM PWM counter clock source, should be +one of the + entries in clock-names. Why do we need this, why not just have only the clock-names/clocks reference the clk that is actually used? As I have replied before, the FTM has two clock sources: the module clock and the counter clock. The counter clock source is selectable depends on different board and also the hardware design: + * FTM source clock is selectable * Source clock can be the system clock, the fixed frequency clock, or an external clock * Fixed frequency clock is an additional clock input to allow the selection of an on chip clock source other than the system clock * Selecting external clock connects FTM clock to a chip level input pin therefore allowing to synchronize the FTM counter with an off chip clock source - From the above description we can see that the external clock source can allow to synchronize the FTM counter with an off chip clock source. As the chip spec permits the counter clock source be selectable, so the different board maybe has different implementation, if the driver do not support, this will be a bug. +- pinctrl-names: must contain a default entry. +- pinctrl-NNN: One property must exist for each entry in pinctrl-names. + See ../pinctrl/pinctrl-bindings.txt for details of the property values. let's drop the .. when making directory references. Is absolute path Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt or just pinctrl/pinctrl-bindings.txt ? -- Xiubo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv5 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.
Hi Rob, Pawel, Mark, Stephon, Ian
Could someone help review and ACK the devicetree binding patch please ?
Thanks very much!
Best Regards,
Xiubo
> -Original Message-
> From: Thierry Reding [mailto:thierry.red...@gmail.com]
> Sent: Tuesday, October 08, 2013 9:57 PM
> To: swar...@wwwdotorg.org; ian.campb...@citrix.com; mark.rutl...@arm.com;
> pawel.m...@arm.com; rob.herr...@calxeda.com
> Cc: Guo Shawn-R65073; s.ha...@pengutronix.de; t.f...@samsung.com;
> grant.lik...@linaro.org; li...@arm.linux.org.uk; r...@landley.net; linux-
> arm-ker...@lists.infradead.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org; linux-
> d...@vger.kernel.org; Xiubo Li-B47053
> Subject: Re: [PATCHv5 4/4] Documentation: Add device tree bindings for
> Freescale FTM PWM.
>
> On Mon, Sep 30, 2013 at 02:13:31PM +0800, Xiubo Li wrote:
> > This adds the Document for Freescale FTM PWM driver under
> > Documentation/devicetree/bindings/pwm/.
> >
> > Signed-off-by: Xiubo Li
> > ---
> > .../devicetree/bindings/pwm/pwm-fsl-ftm.txt| 33
> ++
> > 1 file changed, 33 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
>
> Can someone of the device tree bindings maintainers review and ACK this?
> I think it'd be good to squash this patch with the driver patch (patch 1
> of this series). I can do that when I apply this.
>
> Thierry
>
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> > b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> > new file mode 100644
> > index 000..2c6969a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> > @@ -0,0 +1,33 @@
> > +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. See pwm.txt in this directory for a
> > +description of
> > + the cells format.
> > +- clock-names : Includes the following module clock source entries:
> > +"ftm0" (system clock),
> > +"ftm0_fix_sel" (fixed frequency clock),
> > +"ftm0_ext_sel" (external clock)
> > +- clocks : Must contain a clock specifier for each entry in clock-
> names.
> > +- fsl,pwm-counter-clk: The FTM PWM counter clock source, should be
> > +one of the
> > + entries in clock-names.
> > +- pinctrl-names: must contain a "default" entry.
> > +- pinctrl-NNN: One property must exist for each entry in pinctrl-names.
> > + See ../pinctrl/pinctrl-bindings.txt for details of the property
> values.
> > +
> > +
> > +Example:
> > +
> > +pwm0: pwm@40038000 {
> > + compatible = "fsl,vf610-ftm-pwm";
> > + reg = <0x40038000 0x1000>;
> > + #pwm-cells = <3>;
> > + clock-names = "ftm0", "ftm0_fix_sel", "ftm0_ext_sel";
> > + clocks = < VF610_CLK_FTM0>,
> > + < VF610_CLK_FTM0_FIX_SEL>,
> > + < VF610_CLK_FTM0_EXT_SEL>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <_pwm0_1>;
> > + fsl,pwm-counter-clk = "ftm0_ext_sel"; };
> > --
> > 1.8.0
> >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv5 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.
Hi Rob, Pawel, Mark, Stephon, Ian
Could someone help review and ACK the devicetree binding patch please ?
Thanks very much!
Best Regards,
Xiubo
-Original Message-
From: Thierry Reding [mailto:thierry.red...@gmail.com]
Sent: Tuesday, October 08, 2013 9:57 PM
To: swar...@wwwdotorg.org; ian.campb...@citrix.com; mark.rutl...@arm.com;
pawel.m...@arm.com; rob.herr...@calxeda.com
Cc: Guo Shawn-R65073; s.ha...@pengutronix.de; t.f...@samsung.com;
grant.lik...@linaro.org; li...@arm.linux.org.uk; r...@landley.net; linux-
arm-ker...@lists.infradead.org; linux-...@vger.kernel.org; linux-
ker...@vger.kernel.org; devicet...@vger.kernel.org; linux-
d...@vger.kernel.org; Xiubo Li-B47053
Subject: Re: [PATCHv5 4/4] Documentation: Add device tree bindings for
Freescale FTM PWM.
On Mon, Sep 30, 2013 at 02:13:31PM +0800, Xiubo Li wrote:
This adds the Document for Freescale FTM PWM driver under
Documentation/devicetree/bindings/pwm/.
Signed-off-by: Xiubo Li li.xi...@freescale.com
---
.../devicetree/bindings/pwm/pwm-fsl-ftm.txt| 33
++
1 file changed, 33 insertions(+)
create mode 100644
Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
Can someone of the device tree bindings maintainers review and ACK this?
I think it'd be good to squash this patch with the driver patch (patch 1
of this series). I can do that when I apply this.
Thierry
diff --git a/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
new file mode 100644
index 000..2c6969a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
@@ -0,0 +1,33 @@
+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. See pwm.txt in this directory for a
+description of
+ the cells format.
+- clock-names : Includes the following module clock source entries:
+ftm0 (system clock),
+ftm0_fix_sel (fixed frequency clock),
+ftm0_ext_sel (external clock)
+- clocks : Must contain a clock specifier for each entry in clock-
names.
+- fsl,pwm-counter-clk: The FTM PWM counter clock source, should be
+one of the
+ entries in clock-names.
+- pinctrl-names: must contain a default entry.
+- pinctrl-NNN: One property must exist for each entry in pinctrl-names.
+ See ../pinctrl/pinctrl-bindings.txt for details of the property
values.
+
+
+Example:
+
+pwm0: pwm@40038000 {
+ compatible = fsl,vf610-ftm-pwm;
+ reg = 0x40038000 0x1000;
+ #pwm-cells = 3;
+ clock-names = ftm0, ftm0_fix_sel, ftm0_ext_sel;
+ clocks = clks VF610_CLK_FTM0,
+ clks VF610_CLK_FTM0_FIX_SEL,
+ clks VF610_CLK_FTM0_EXT_SEL;
+ pinctrl-names = default;
+ pinctrl-0 = pinctrl_pwm0_1;
+ fsl,pwm-counter-clk = ftm0_ext_sel; };
--
1.8.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv4 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.
> > +- fsl,pwm-counter-clk: The FTM PWM counter clock source, should be > > +one of the > > + entries in clock-names. > > So the IP block has 3 input clocks, and also a mux to select which one to > use? That sounds slightly unusual, but possible. > > If there is really only 1 clock input to the IP block, and the mux is > part of some other module, then this binding should have only 1 entry in > clocks. > Yes, there are 3 input clocks that can be selectable, and the mux is inside the FTM IP block. Thanks. -- Best Regard, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv4 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.
+- fsl,pwm-counter-clk: The FTM PWM counter clock source, should be +one of the + entries in clock-names. So the IP block has 3 input clocks, and also a mux to select which one to use? That sounds slightly unusual, but possible. If there is really only 1 clock input to the IP block, and the mux is part of some other module, then this binding should have only 1 entry in clocks. Yes, there are 3 input clocks that can be selectable, and the mux is inside the FTM IP block. Thanks. -- Best Regard, Xiubo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv3 1/4] pwm: Add Freescale FTM PWM driver support
> Subject: Re: [PATCHv3 1/4] pwm: Add Freescale FTM PWM driver support
>
> On Fri, Sep 06, 2013 at 04:08:24PM +0800, Xiubo Li wrote:
> > The FTM PWM device can be found on Vybrid VF610 Tower and Layerscape
> LS-1 SoCs.
> >
> > +
> > +static int fsl_pwm_probe(struct platform_device *pdev) {
> > + int ret = 0;
> > + struct fsl_pwm_chip *fpc;
> > + struct resource *res;
> > +
> > + fpc = devm_kzalloc(>dev, sizeof(*fpc), GFP_KERNEL);
>
> You don't have to output a message when kzalloc fails, but still you
> should check for the result and return an error if necessary.
>
Okey, I will.
--
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv3 2/4] ARM: dts: Add Freescale FTM PWM node for VF610.
> > diff --git a/arch/arm/boot/dts/vf610.dtsi
> > b/arch/arm/boot/dts/vf610.dtsi index 67d929c..44787b5 100644
> > --- a/arch/arm/boot/dts/vf610.dtsi
> > +++ b/arch/arm/boot/dts/vf610.dtsi
> > @@ -140,6 +140,37 @@
> > clock-names = "pit";
> > };
> >
> > + pwm0: pwm@40038000 {
> > + compatible = "fsl,vf610-ftm-pwm";
> > + #pwm-cells = <3>;
> > + reg = <0x40038000 0x1000>;
> > + clock-names = "ftm0", "ftm0_fix_sel",
> "ftm0_ext_sel";
> > + clocks = < VF610_CLK_FTM0>,
> > + < VF610_CLK_FTM0_FIX_SEL>,
> > + < VF610_CLK_FTM0_EXT_SEL>;
> > + pinctrl-names = "ch0-active", "ch0-idle", "ch1-
> active", "ch1-idle",
> > + "ch2-active", "ch2-idle", "ch3-
> active", "ch3-idle",
> > + "ch4-active", "ch4-idle", "ch5-
> active", "ch5-idle",
> > + "ch6-active", "ch6-idle", "ch7-
> active", "ch7-idle";
> > + pinctrl-0 = <_pwm0_ch0_active>;
> > + pinctrl-1 = <_pwm0_ch0_idle>;
> > + pinctrl-2 = <_pwm0_ch1_active>;
> > + pinctrl-3 = <_pwm0_ch1_idle>;
> > + pinctrl-4 = <_pwm0_ch2_active>;
> > + pinctrl-5 = <_pwm0_ch2_idle>;
> > + pinctrl-6 = <_pwm0_ch3_active>;
> > + pinctrl-7 = <_pwm0_ch3_idle>;
> > + pinctrl-8 = <_pwm0_ch4_active>;
> > + pinctrl-9 = <_pwm0_ch4_idle>;
> > + pinctrl-10 = <_pwm0_ch5_active>;
> > + pinctrl-11 = <_pwm0_ch5_idle>;
> > + pinctrl-12 = <_pwm0_ch6_active>;
> > + pinctrl-13 = <_pwm0_ch6_idle>;
> > + pinctrl-14 = <_pwm0_ch7_active>;
> > + pinctrl-15 = <_pwm0_ch7_idle>;
> > + status = "disabled";
> > + };
>
> This is a SoC file, but the pinmux is board specific. The pinmux settings
> probably shouldn't be here.
>
Yes, agreed.
I will revise it soon.
--
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv3 2/4] ARM: dts: Add Freescale FTM PWM node for VF610.
diff --git a/arch/arm/boot/dts/vf610.dtsi
b/arch/arm/boot/dts/vf610.dtsi index 67d929c..44787b5 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -140,6 +140,37 @@
clock-names = pit;
};
+ pwm0: pwm@40038000 {
+ compatible = fsl,vf610-ftm-pwm;
+ #pwm-cells = 3;
+ reg = 0x40038000 0x1000;
+ clock-names = ftm0, ftm0_fix_sel,
ftm0_ext_sel;
+ clocks = clks VF610_CLK_FTM0,
+ clks VF610_CLK_FTM0_FIX_SEL,
+ clks VF610_CLK_FTM0_EXT_SEL;
+ pinctrl-names = ch0-active, ch0-idle, ch1-
active, ch1-idle,
+ ch2-active, ch2-idle, ch3-
active, ch3-idle,
+ ch4-active, ch4-idle, ch5-
active, ch5-idle,
+ ch6-active, ch6-idle, ch7-
active, ch7-idle;
+ pinctrl-0 = pinctrl_pwm0_ch0_active;
+ pinctrl-1 = pinctrl_pwm0_ch0_idle;
+ pinctrl-2 = pinctrl_pwm0_ch1_active;
+ pinctrl-3 = pinctrl_pwm0_ch1_idle;
+ pinctrl-4 = pinctrl_pwm0_ch2_active;
+ pinctrl-5 = pinctrl_pwm0_ch2_idle;
+ pinctrl-6 = pinctrl_pwm0_ch3_active;
+ pinctrl-7 = pinctrl_pwm0_ch3_idle;
+ pinctrl-8 = pinctrl_pwm0_ch4_active;
+ pinctrl-9 = pinctrl_pwm0_ch4_idle;
+ pinctrl-10 = pinctrl_pwm0_ch5_active;
+ pinctrl-11 = pinctrl_pwm0_ch5_idle;
+ pinctrl-12 = pinctrl_pwm0_ch6_active;
+ pinctrl-13 = pinctrl_pwm0_ch6_idle;
+ pinctrl-14 = pinctrl_pwm0_ch7_active;
+ pinctrl-15 = pinctrl_pwm0_ch7_idle;
+ status = disabled;
+ };
This is a SoC file, but the pinmux is board specific. The pinmux settings
probably shouldn't be here.
Yes, agreed.
I will revise it soon.
--
Xiubo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv3 1/4] pwm: Add Freescale FTM PWM driver support
Subject: Re: [PATCHv3 1/4] pwm: Add Freescale FTM PWM driver support
On Fri, Sep 06, 2013 at 04:08:24PM +0800, Xiubo Li wrote:
The FTM PWM device can be found on Vybrid VF610 Tower and Layerscape
LS-1 SoCs.
+
+static int fsl_pwm_probe(struct platform_device *pdev) {
+ int ret = 0;
+ struct fsl_pwm_chip *fpc;
+ struct resource *res;
+
+ fpc = devm_kzalloc(pdev-dev, sizeof(*fpc), GFP_KERNEL);
You don't have to output a message when kzalloc fails, but still you
should check for the result and return an error if necessary.
Okey, I will.
--
Xiubo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv3 1/4] pwm: Add Freescale FTM PWM driver support
> Subject: Re: [PATCHv3 1/4] pwm: Add Freescale FTM PWM driver support > > On Mon, Sep 09, 2013 at 02:20:09PM +0200, Thierry Reding wrote: > > On Fri, Sep 06, 2013 at 04:08:24PM +0800, Xiubo Li wrote: > > > The FTM PWM device can be found on Vybrid VF610 Tower and Layerscape > LS-1 SoCs. > > > > > > Signed-off-by: Xiubo Li > > > Signed-off-by: Jingchang Lu > > > --- > > > drivers/pwm/Kconfig | 10 + > > > drivers/pwm/Makefile | 1 + > > > drivers/pwm/pwm-fsl-ftm.c | 505 > > > ++ > > > 3 files changed, 516 insertions(+) > > > create mode 100644 drivers/pwm/pwm-fsl-ftm.c > > > > This looks pretty good to me. I noticed that you didn't Cc Sascha who > > commented on this a lot. Can you please resend with him Cc'ed to make > > sure he sees this version of the series? > > I'm already aware of this, no need to resend. I'll have a look over it > tomorrow. > I'm very sorry, next time I will. Thanks very much. -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv3 1/4] pwm: Add Freescale FTM PWM driver support
Subject: Re: [PATCHv3 1/4] pwm: Add Freescale FTM PWM driver support On Mon, Sep 09, 2013 at 02:20:09PM +0200, Thierry Reding wrote: On Fri, Sep 06, 2013 at 04:08:24PM +0800, Xiubo Li wrote: The FTM PWM device can be found on Vybrid VF610 Tower and Layerscape LS-1 SoCs. Signed-off-by: Xiubo Li li.xi...@freescale.com Signed-off-by: Jingchang Lu b35...@freescale.com --- drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-fsl-ftm.c | 505 ++ 3 files changed, 516 insertions(+) create mode 100644 drivers/pwm/pwm-fsl-ftm.c This looks pretty good to me. I noticed that you didn't Cc Sascha who commented on this a lot. Can you please resend with him Cc'ed to make sure he sees this version of the series? I'm already aware of this, no need to resend. I'll have a look over it tomorrow. I'm very sorry, next time I will. Thanks very much. -- Best Regards, Xiubo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support
> Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support > > On Tue, Sep 03, 2013 at 04:17:09AM +0000, Xiubo Li-B47053 wrote: > > > Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support > > > > > > You simply don't need the available field. You don't need to track > > > whether they are available. If a user enables a pwm which is not > > > routed out of the SoC (disabled in the iomux) simply nothing will > > > happen except for a slightly increased power consumption. > > > > > If the there is not need to explicitly specify the channels are > > available or not, so there is no doubt that the 'available' field will > > be dropt. Why I added this here is because that the 4th and 5th > > channels' pinctrls are used as UART TX and RX as I have mentioned > > before, so here if you configure these two pinctrls, the UART TX and > > RX will be polluted, there maybe some other cases like this. > > If you misconfigure your iomux then usually unexptected things happen. > That is not the problem of the PWM driver, but the problem of the one > writing the devicetree. The kernel will print a message for conflicting > iomux settings. That should be hint enough to fix it. > That sounds good. Actully there isn't any conflicting messages will be printed. I will think it over. -- Best Regards, Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support
Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support On Tue, Sep 03, 2013 at 04:17:09AM +, Xiubo Li-B47053 wrote: Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support You simply don't need the available field. You don't need to track whether they are available. If a user enables a pwm which is not routed out of the SoC (disabled in the iomux) simply nothing will happen except for a slightly increased power consumption. If the there is not need to explicitly specify the channels are available or not, so there is no doubt that the 'available' field will be dropt. Why I added this here is because that the 4th and 5th channels' pinctrls are used as UART TX and RX as I have mentioned before, so here if you configure these two pinctrls, the UART TX and RX will be polluted, there maybe some other cases like this. If you misconfigure your iomux then usually unexptected things happen. That is not the problem of the PWM driver, but the problem of the one writing the devicetree. The kernel will print a message for conflicting iomux settings. That should be hint enough to fix it. That sounds good. Actully there isn't any conflicting messages will be printed. I will think it over. -- Best Regards, Xiubo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
> 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 > >> --- > >> .../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 000..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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support
> Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support
>
> On Mon, Sep 02, 2013 at 03:33:37AM +0000, Xiubo Li-B47053 wrote:
> >
> > > > +static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device
> > > > +*pwm) {
> > > > + struct fsl_pwm_chip *fpc;
> > > > + struct fsl_pwm_data *pwm_data;
> > > > +
> > > > + fpc = to_fsl_chip(chip);
> > > > +
> > > > + pwm_data = pwm_get_chip_data(pwm);
> > > > + if (!pwm_data)
> > > > + return;
> > >
> > > THis check seems unnecessary.
> > >
> >
> > But if do not check it here, I must check it in the following code.
> >
> > > > +
> > > > + if (pwm_data->available != FSL_AVAILABLE)
> > > > + return;
> > > > +
> >
> > So the ' struct fsl_pwm_data' may be removed in the future.
> >
> > >
> > > > +
> > > > +
> > > > + pwm_data->period_cycles = period_cycles;
> > > > + pwm_data->duty_cycles = duty_cycles;
> > >
> > > These fields are set but never read. Please drop them.
> > >
> > > If you drop the 'available' field also the you can drop chip_data
> > > completely.
> > >
> >
> > I think I may move the 'available' field to the PWM driver data struct.
>
> You simply don't need the available field. You don't need to track
> whether they are available. If a user enables a pwm which is not routed
> out of the SoC (disabled in the iomux) simply nothing will happen except
> for a slightly increased power consumption.
>
If the there is not need to explicitly specify the channels are available or
not, so there is no doubt that the 'available' field will be dropt.
Why I added this here is because that the 4th and 5th channels' pinctrls are
used as UART TX and RX as I have mentioned before, so here if you configure
these two pinctrls, the UART TX and RX will be polluted, there maybe some other
cases like this.
So, if there is no need to worry about this in PWM driver, the customer should
be aware of it and be responsible for the potential risk.
I will think it over and optimize it then.
Thanks very much.
--
Best Regards.
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support
Subject: Re: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support
On Mon, Sep 02, 2013 at 03:33:37AM +, Xiubo Li-B47053 wrote:
+static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device
+*pwm) {
+ struct fsl_pwm_chip *fpc;
+ struct fsl_pwm_data *pwm_data;
+
+ fpc = to_fsl_chip(chip);
+
+ pwm_data = pwm_get_chip_data(pwm);
+ if (!pwm_data)
+ return;
THis check seems unnecessary.
But if do not check it here, I must check it in the following code.
+
+ if (pwm_data-available != FSL_AVAILABLE)
+ return;
+
So the ' struct fsl_pwm_data' may be removed in the future.
+
+
+ pwm_data-period_cycles = period_cycles;
+ pwm_data-duty_cycles = duty_cycles;
These fields are set but never read. Please drop them.
If you drop the 'available' field also the you can drop chip_data
completely.
I think I may move the 'available' field to the PWM driver data struct.
You simply don't need the available field. You don't need to track
whether they are available. If a user enables a pwm which is not routed
out of the SoC (disabled in the iomux) simply nothing will happen except
for a slightly increased power consumption.
If the there is not need to explicitly specify the channels are available or
not, so there is no doubt that the 'available' field will be dropt.
Why I added this here is because that the 4th and 5th channels' pinctrls are
used as UART TX and RX as I have mentioned before, so here if you configure
these two pinctrls, the UART TX and RX will be polluted, there maybe some other
cases like this.
So, if there is no need to worry about this in PWM driver, the customer should
be aware of it and be responsible for the potential risk.
I will think it over and optimize it then.
Thanks very much.
--
Best Regards.
Xiubo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
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.xi...@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 000..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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support
> > +static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device
> > +*pwm) {
> > + struct fsl_pwm_chip *fpc;
> > + struct fsl_pwm_data *pwm_data;
> > +
> > + fpc = to_fsl_chip(chip);
> > +
> > + pwm_data = pwm_get_chip_data(pwm);
> > + if (!pwm_data)
> > + return;
>
> THis check seems unnecessary.
>
But if do not check it here, I must check it in the following code.
> > +
> > + if (pwm_data->available != FSL_AVAILABLE)
> > + return;
> > +
So the ' struct fsl_pwm_data' may be removed in the future.
>
> > +
> > +
> > + pwm_data->period_cycles = period_cycles;
> > + pwm_data->duty_cycles = duty_cycles;
>
> These fields are set but never read. Please drop them.
>
> If you drop the 'available' field also the you can drop chip_data
> completely.
>
I think I may move the 'available' field to the PWM driver data struct.
> > +
> > + writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc->base + FTM_CSC(pwm->hwpwm));
> > +
> > + writel(0xF0, fpc->base + FTM_OUTMASK);
> > + writel(0x0F, fpc->base + FTM_OUTINIT);
> > + writel(FTM_CNTIN_VAL, fpc->base + FTM_CNTIN);
> > +
> > + writel(period_cycles + cntin - 1, fpc->base + FTM_MOD);
> > + writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm));
> > +
> > + return 0;
> > +}
> > +
> > +static int fsl_pwm_set_polarity(struct pwm_chip *chip, struct
> pwm_device *pwm,
> > + enum pwm_polarity polarity)
> > +{
> > + unsigned long reg;
> > + struct fsl_pwm_data *pwm_data;
> > + struct fsl_pwm_chip *fpc;
> > +
> > + fpc = to_fsl_chip(chip);
> > +
> > + pwm_data = pwm_get_chip_data(pwm);
> > + if (!pwm_data)
> > + return -EINVAL;
> > +
> > + if (pwm_data->available != FSL_AVAILABLE)
> > + return -EINVAL;
> > +
> > + reg = readl(fpc->base + FTM_POL);
> > + reg &= ~BIT(pwm->hwpwm);
>
> Either drop this line...
>
This is just for unmasking this bit field.
Here it's not needed, so I will revise it.
> > + if (polarity == PWM_POLARITY_INVERSED)
> > + reg |= BIT(pwm->hwpwm);
> > + else
> > + reg &= ~BIT(pwm->hwpwm);
>
> ...or this one
>
> > +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device
> > +*pwm) {
> > + int ret;
> > + struct fsl_pwm_chip *fpc;
> > + struct pinctrl_state *pins_state;
> > + struct fsl_pwm_data *pwm_data;
> > + const char *statename;
> > +
> > + fpc = to_fsl_chip(chip);
> > +
> > + pwm_data = pwm_get_chip_data(pwm);
> > + if (!pwm_data)
> > + return -EINVAL;
> > +
> > + if (pwm_data->available != FSL_AVAILABLE)
> > + return -EINVAL;
> > +
> > + statename = kasprintf(GFP_KERNEL, "ch%d-active", pwm->hwpwm);
>
> You loose memory here and in fsl_pwm_disable aswell.
>
Yes, I will revise it.
> > + pins_state = pinctrl_lookup_state(fpc->pinctrl,
> > + statename);
> > + /* enable pins to be muxed in and configured */
> > + if (!IS_ERR(pins_state)) {
> > + ret = pinctrl_select_state(fpc->pinctrl, pins_state);
> > + if (ret)
> > + dev_warn(chip->dev, "could not set default pins\n");
> > + } else
> > + dev_warn(chip->dev, "could not get default pinstate\n");
>
> Either it's ok to do without pinctrl or it's not ok, so either return an
> error or drop the warnings. Polluting the kernel log with such messages
> from a frequently called function is not a good idea.
>
Well, I will just print out some error logs and return the error.
--
Best Regards.
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv2 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.
> Subject: Re: [PATCHv2 4/4] Documentation: Add device tree bindings for
> Freescale FTM PWM.
>
...
> > +
> > +pwm0: pwm@40038000 {
> > + compatible = "fsl,vf610-ftm-pwm";
> > + reg = <0x40038000 0x1000>;
> > + #pwm-cells = <3>;
> > + clock-names = "ftm0", "ftm0_fix_sel", "ftm0_ext_sel";
> > + clocks = < VF610_CLK_FTM0>,
> > + < VF610_CLK_FTM0_FIX_SEL>,
> > + < VF610_CLK_FTM0_EXT_SEL>;
> > + pinctrl-names = "ch0-active", "ch0-idle", "ch1-active", "ch1-
> idle",
> > + ;
> > + pinctrl-0 = <_pwm0_ch0_active>;
> > + pinctrl-1 = <_pwm0_ch0_idle>;
> > + pinctrl-2 = <_pwm0_ch1_active>;
> > + pinctrl-3 = <_pwm0_ch1_idle>;
> > + ...
> > + fsl,pwm-counter-clk = "ftm0_ext_sel";
> > + fsl,pwm-avaliable-chs = <0 3 5 6>;
>
> I don't think this proerty is useful. Just enable all channels. I think
> this was mentioned before.
>
Yes.
Actully this property is located in board level dts file.
I have added and requested all the channels in SoC level dtsi file, and in
board level dts file to tell the customer the limitation, I think is much
safter and better.
--
Best Regards.
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv2 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.
Subject: Re: [PATCHv2 4/4] Documentation: Add device tree bindings for
Freescale FTM PWM.
...
+
+pwm0: pwm@40038000 {
+ compatible = fsl,vf610-ftm-pwm;
+ reg = 0x40038000 0x1000;
+ #pwm-cells = 3;
+ clock-names = ftm0, ftm0_fix_sel, ftm0_ext_sel;
+ clocks = clks VF610_CLK_FTM0,
+ clks VF610_CLK_FTM0_FIX_SEL,
+ clks VF610_CLK_FTM0_EXT_SEL;
+ pinctrl-names = ch0-active, ch0-idle, ch1-active, ch1-
idle,
+ ;
+ pinctrl-0 = pinctrl_pwm0_ch0_active;
+ pinctrl-1 = pinctrl_pwm0_ch0_idle;
+ pinctrl-2 = pinctrl_pwm0_ch1_active;
+ pinctrl-3 = pinctrl_pwm0_ch1_idle;
+ ...
+ fsl,pwm-counter-clk = ftm0_ext_sel;
+ fsl,pwm-avaliable-chs = 0 3 5 6;
I don't think this proerty is useful. Just enable all channels. I think
this was mentioned before.
Yes.
Actully this property is located in board level dts file.
I have added and requested all the channels in SoC level dtsi file, and in
board level dts file to tell the customer the limitation, I think is much
safter and better.
--
Best Regards.
Xiubo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv2 1/4] pwm: Add Freescale FTM PWM driver support
+static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device
+*pwm) {
+ struct fsl_pwm_chip *fpc;
+ struct fsl_pwm_data *pwm_data;
+
+ fpc = to_fsl_chip(chip);
+
+ pwm_data = pwm_get_chip_data(pwm);
+ if (!pwm_data)
+ return;
THis check seems unnecessary.
But if do not check it here, I must check it in the following code.
+
+ if (pwm_data-available != FSL_AVAILABLE)
+ return;
+
So the ' struct fsl_pwm_data' may be removed in the future.
+
+
+ pwm_data-period_cycles = period_cycles;
+ pwm_data-duty_cycles = duty_cycles;
These fields are set but never read. Please drop them.
If you drop the 'available' field also the you can drop chip_data
completely.
I think I may move the 'available' field to the PWM driver data struct.
+
+ writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc-base + FTM_CSC(pwm-hwpwm));
+
+ writel(0xF0, fpc-base + FTM_OUTMASK);
+ writel(0x0F, fpc-base + FTM_OUTINIT);
+ writel(FTM_CNTIN_VAL, fpc-base + FTM_CNTIN);
+
+ writel(period_cycles + cntin - 1, fpc-base + FTM_MOD);
+ writel(duty_cycles + cntin, fpc-base + FTM_CV(pwm-hwpwm));
+
+ return 0;
+}
+
+static int fsl_pwm_set_polarity(struct pwm_chip *chip, struct
pwm_device *pwm,
+ enum pwm_polarity polarity)
+{
+ unsigned long reg;
+ struct fsl_pwm_data *pwm_data;
+ struct fsl_pwm_chip *fpc;
+
+ fpc = to_fsl_chip(chip);
+
+ pwm_data = pwm_get_chip_data(pwm);
+ if (!pwm_data)
+ return -EINVAL;
+
+ if (pwm_data-available != FSL_AVAILABLE)
+ return -EINVAL;
+
+ reg = readl(fpc-base + FTM_POL);
+ reg = ~BIT(pwm-hwpwm);
Either drop this line...
This is just for unmasking this bit field.
Here it's not needed, so I will revise it.
+ if (polarity == PWM_POLARITY_INVERSED)
+ reg |= BIT(pwm-hwpwm);
+ else
+ reg = ~BIT(pwm-hwpwm);
...or this one
+static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device
+*pwm) {
+ int ret;
+ struct fsl_pwm_chip *fpc;
+ struct pinctrl_state *pins_state;
+ struct fsl_pwm_data *pwm_data;
+ const char *statename;
+
+ fpc = to_fsl_chip(chip);
+
+ pwm_data = pwm_get_chip_data(pwm);
+ if (!pwm_data)
+ return -EINVAL;
+
+ if (pwm_data-available != FSL_AVAILABLE)
+ return -EINVAL;
+
+ statename = kasprintf(GFP_KERNEL, ch%d-active, pwm-hwpwm);
You loose memory here and in fsl_pwm_disable aswell.
Yes, I will revise it.
+ pins_state = pinctrl_lookup_state(fpc-pinctrl,
+ statename);
+ /* enable pins to be muxed in and configured */
+ if (!IS_ERR(pins_state)) {
+ ret = pinctrl_select_state(fpc-pinctrl, pins_state);
+ if (ret)
+ dev_warn(chip-dev, could not set default pins\n);
+ } else
+ dev_warn(chip-dev, could not get default pinstate\n);
Either it's ok to do without pinctrl or it's not ok, so either return an
error or drop the warnings. Polluting the kernel log with such messages
from a frequently called function is not a good idea.
Well, I will just print out some error logs and return the error.
--
Best Regards.
Xiubo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4] pwm: add freescale ftm pwm driver support
> Actually it should even be:
>
> #define FTM_CSC(channel) \
> (FTM_CSC_BASE + ((channel) * 8))
>
Well, yes, It should be, as Sascha has comment about this before, I have
already revise it.
> > Firstly, we should be clear that the fpc->clk is chip's work clock.
> > If so, after the .request() is called and before .enable() is called,
> > the custumer will call .config(), in which will read/write the pwm chip
> registers, if the module clock is still disabled, then the system will
> hang up.
>
> Okay. In that case perhaps the better thing to do is call clk_prepare()
> during driver probe and only clk_enable() here.
>
Yes, it is.
> > > Perhaps time_ns should be "unsigned long"?
> > >
> >
> > Shouldn't this be same with "int duty_ns" and "int period_ns", which
> > are defined by struct pwm_ops { ...
> > int (*config)(struct pwm_chip *chip,
> > struct pwm_device *pwm,
> > int duty_ns, int period_ns); ...
> > } ?
>
> Well, the plan is to eventually make duty_ns and period_ns unsigned int
> or unsigned long because negative values don't make any sense for them.
> With that in mind I think it makes sense to use the proper type here now.
>
Okey, I will think it over again and revise it.
> > > > +static int fsl_pwm_config_channel(struct pwm_chip *chip,
> > >
> > > I think you can safely drop the _channel suffix from the PWM
> operations.
> > >
> >
> > By adding _channel suffix just for more comprehensave about the pwm's
> muti-channel operation.
> > If this is redundant here, I will drop it.
>
> The operations are implicitly per-channel operations. So yes, the
> _channel suffix is redundant here.
>
Agree, I will drop it.
> > > > + fpc = to_fsl_chip(chip);
> > > > +
> > > > + if (WARN_ON(!test_bit(PWMF_REQUESTED, >flags)))
> > > > + return -ESHUTDOWN;
> > >
> > > Erm... how do you think this could ever happen? Users need to
> > > request a PWM to obtain a struct pwm_device, in which case
> > > PWMF_REQUESTED will always be set. There are a few other occurrences
> > > throughout the rest of the driver that I haven't pointed out
> explicitly.
> > >
> >
> > Does the following case is exist ?
> > The customer in one thread has .free(pwm_1), while in another thread,
> > which maybe had slept in for some reason, will
> call .config/.enable/.disable?
> >
> > If so, as I have explained before, if the pwm_1 has been freed, the
> > module clock maybe disabled too, so if the .config is call the system
> will hang up.
>
> While the above could possibly happen, there's no way the core could
> prevent it. And your explicit test couldn't either. So what usually
> happens is that a driver requests a PWM device and then has exclusive
> access to it. Any other driver that wants to use the same PWM device
> can't because it will get an -EBUSY return.
>
> So in your hypothetical case above, if one driver does stuff like that
> with a PWM device then that's a driver bug, not something the PWM core
> should be required to handle.
>
Okey, I will drop this.
> > While I think the following is better in code.
> >
> > dev_err(fpc->chip.dev,
> > "failed to parse property: %d\n",
> > ret);
>
> Why? You're quoting which property failed to parse so you should use the
> correct character for quoting, which is either the apostrophe (') or the
> quotation mark (").
>
For my first impression, I just thought that maybe a little better.
Okey, I will adopt this in the feature.
--
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4] pwm: add freescale ftm pwm driver support
Actually it should even be:
#define FTM_CSC(channel) \
(FTM_CSC_BASE + ((channel) * 8))
Well, yes, It should be, as Sascha has comment about this before, I have
already revise it.
Firstly, we should be clear that the fpc-clk is chip's work clock.
If so, after the .request() is called and before .enable() is called,
the custumer will call .config(), in which will read/write the pwm chip
registers, if the module clock is still disabled, then the system will
hang up.
Okay. In that case perhaps the better thing to do is call clk_prepare()
during driver probe and only clk_enable() here.
Yes, it is.
Perhaps time_ns should be unsigned long?
Shouldn't this be same with int duty_ns and int period_ns, which
are defined by struct pwm_ops { ...
int (*config)(struct pwm_chip *chip,
struct pwm_device *pwm,
int duty_ns, int period_ns); ...
} ?
Well, the plan is to eventually make duty_ns and period_ns unsigned int
or unsigned long because negative values don't make any sense for them.
With that in mind I think it makes sense to use the proper type here now.
Okey, I will think it over again and revise it.
+static int fsl_pwm_config_channel(struct pwm_chip *chip,
I think you can safely drop the _channel suffix from the PWM
operations.
By adding _channel suffix just for more comprehensave about the pwm's
muti-channel operation.
If this is redundant here, I will drop it.
The operations are implicitly per-channel operations. So yes, the
_channel suffix is redundant here.
Agree, I will drop it.
+ fpc = to_fsl_chip(chip);
+
+ if (WARN_ON(!test_bit(PWMF_REQUESTED, pwm-flags)))
+ return -ESHUTDOWN;
Erm... how do you think this could ever happen? Users need to
request a PWM to obtain a struct pwm_device, in which case
PWMF_REQUESTED will always be set. There are a few other occurrences
throughout the rest of the driver that I haven't pointed out
explicitly.
Does the following case is exist ?
The customer in one thread has .free(pwm_1), while in another thread,
which maybe had slept in for some reason, will
call .config/.enable/.disable?
If so, as I have explained before, if the pwm_1 has been freed, the
module clock maybe disabled too, so if the .config is call the system
will hang up.
While the above could possibly happen, there's no way the core could
prevent it. And your explicit test couldn't either. So what usually
happens is that a driver requests a PWM device and then has exclusive
access to it. Any other driver that wants to use the same PWM device
can't because it will get an -EBUSY return.
So in your hypothetical case above, if one driver does stuff like that
with a PWM device then that's a driver bug, not something the PWM core
should be required to handle.
Okey, I will drop this.
While I think the following is better in code.
dev_err(fpc-chip.dev,
failed to parse fsl,pwm-clk-ps property: %d\n,
ret);
Why? You're quoting which property failed to parse so you should use the
correct character for quoting, which is either the apostrophe (') or the
quotation mark ().
For my first impression, I just thought that maybe a little better.
Okey, I will adopt this in the feature.
--
Xiubo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4] pwm: add freescale ftm pwm driver support
Hi Thierry,
See the comments below.
> I think the correct capitalizations are: "Freescale", "FTM" and "PWM".
> There are other occurrences in the rest of the driver that I haven't
> explicitly pointed out.
>
Yes, that's much better.
> > +config PWM_FTM
>
> Perhaps name this PWM_FSL_FTM to match the driver name?
>
OK.
> And fix this up to be "pwm-fsl-ftm".
Yes, I will.
> > +#define FTM_CSC_BASE0x0C
> > +#define FTM_CSC(_CHANNEL) \
> > + (FTM_CSC_BASE + (_CHANNEL * 0x08))
>
> I prefer lowercase variables in macros:
>
> #define FTM_CSC(channel) \
> (FTM_CSC_BASE + (channel * 8))
>
Yes, That's better.
> > +#define FTM_PWMMODE (FTMCnSC_MSB)
> > +#define FTM_HIGH_TRUE (FTMCnSC_ELSB)
> > +#define FTM_LOW_TRUE(FTMCnSC_ELSA)
>
> What's the reason for redefining these? Can't you just use the FTMCnSC_*
> defines directly?
>
Just for more readable and comprehension.
I will revise it by not losing above two key elements.
> > +#define FTM_CV(_CHANNEL) \
> > + (FTM_CV_BASE + (_CHANNEL * 0x08))
>
> Same comment as for FTM_CSC above.
>
Yes.
> > +#define FTM_MAX_CHANNEL 0x08
>
> There should be no need for this. The only place where you use this is
> when assigning it to pwm_chip.npwm, so you may as well use the literal
> there.
>
I have recoded the driver, and the macro is used more than one time now.
> > + struct platform_device *pdev;
>
> You never use this platform_device. And you have to assign >dev to
> the pwm_chip.dev anyway, so why not just use that consistently and drop
> the pdev field?
>
Yes, I have droped the pdev field now.
> > + unsigned intcpwm;
> > + struct fsl_pwm fpwms[FTM_MAX_CHANNEL];
>
> Please don't do this. Use pwm_set_chip_data()/pwm_get_chip_data() to
> associate per-channel specific data.
>
I have replaced them now.
> > + /* pinctrl handles */
>
> Nit: it's only a single handle.
>
Yes.
> > + struct pinctrl *pinctrl;
>
> You also use tabs and spaces inconsistently here. I suggest you use a
> single space between the data type and the field name, that way it's much
> easier to stay consistent.
>
Now I have revised all about this.
> > +static int fsl_pwm_request_channel(struct pwm_chip *chip,
> > + struct pwm_device *pwm)
>
> The arguments on the trailing line aren't properly aligned. They should
> be aligned with the arguments on the first line.
>
The same as the above comment.
> > + ret = clk_prepare_enable(fpc->clk);
>
> This should probably be just clk_prepare(). Or is there some reason why
> you can't delay clk_enable() to the .enable() operation?
>
Firstly, we should be clear that the fpc->clk is chip's work clock.
If so, after the .request() is called and before .enable() is called, the
custumer will call .config(),
in which will read/write the pwm chip registers, if the module clock is still
disabled, then the system will hang up.
> > +static int fsl_updata_config(struct fsl_pwm_chip *fpc,
> > +struct pwm_device *pwm)
>
> Parameter alignment again. Please also check all other functions.
>
Yes, I have revise all about this.
> > +{
> > + int i;
>
> This should be unsigned int.
>
I will revise it.
> > +static unsigned long
> > +fsl_rate_to_cycles(struct fsl_pwm_chip *fpc, int time_ns)
>
> The above two lines are 78 characters when joined, so there's no need to
> split them.
>
OK, I have revise all about this.
> Perhaps time_ns should be "unsigned long"?
>
Shouldn't this be same with "int duty_ns" and "int period_ns", which are
defined by
struct pwm_ops {
...
int (*config)(struct pwm_chip *chip,
struct pwm_device *pwm,
int duty_ns, int period_ns);
...
} ?
> > + ps = (0x01 << fpc->clk_ps);
>
> This is fairly short, so you could do it along with the variable
> declaration. Also there's no need for the parentheses or the hexadecimal
> prefix (0x01 == 1):
>
> unsigned long ps = 1 << fpc->clk_ps;
>
OK, I will revise it then.
> > + /* The module clk is HZ/s, the time_ns parameter
> > +* is based nanosecond, so here should divide
> > +* 10UL.
> > +*/
>
> Block comments should be:
>
> /*
>* ...
>* ...
>*/
>
> Also HZ/s isn't a valid unit for a clock frequency. And time_ns also has
> the _ns suffix to designate the unit, so as a whole that comment doesn't
> add any real information and can just as well be dropped.
>
I will revise it.
> > +static int fsl_pwm_config_channel(struct pwm_chip *chip,
>
> I think you can safely drop the _channel suffix from the PWM operations.
>
By adding _channel suffix just for more comprehensave about the pwm's
muti-channel operation.
If this is redundant here, I will drop it.
> > + fpc = to_fsl_chip(chip);
> > +
> > + if (WARN_ON(!test_bit(PWMF_REQUESTED, >flags)))
> > + return -ESHUTDOWN;
>
> Erm... how do you think this
RE: [PATCH 3/4] ARM: dts: Enables ftm pwm device for Vybrid VF610 TOWER board.
Hi Thierry, > Subject: Re: [PATCH 3/4] ARM: dts: Enables ftm pwm device for Vybrid > VF610 TOWER board. > > On Wed, Aug 21, 2013 at 11:07:41AM +0800, Xiubo Li wrote: > > Signed-off-by: Xiubo Li > > --- > > arch/arm/boot/dts/vf610-twr.dts | 17 + > > 1 file changed, 17 insertions(+) > > Same comments as for patch 2/4. > Yes, I will revise it then. Thanks very much. -- Best Regards. Xiubo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/4] ARM: dts: Add Freescale ftm pwm node for VF610.
Hi Thierry, > Subject: Re: [PATCH 2/4] ARM: dts: Add Freescale ftm pwm node for VF610. > > On Wed, Aug 21, 2013 at 11:07:40AM +0800, Xiubo Li wrote: > > Signed-off-by: Xiubo Li > > --- > > arch/arm/boot/dts/vf610.dtsi | 83 > > +++- > > 1 file changed, 82 insertions(+), 1 deletion(-) > > Please also pay attention to the correct capitalization in the subject > here. "FTM" and "PWM". Furthermore this could probably use more than a > single line as patch description. > Yes, I will 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/4] ARM: dts: Add Freescale ftm pwm node for VF610.
Hi Thierry, Subject: Re: [PATCH 2/4] ARM: dts: Add Freescale ftm pwm node for VF610. On Wed, Aug 21, 2013 at 11:07:40AM +0800, Xiubo Li wrote: Signed-off-by: Xiubo Li li.xi...@freescale.com --- arch/arm/boot/dts/vf610.dtsi | 83 +++- 1 file changed, 82 insertions(+), 1 deletion(-) Please also pay attention to the correct capitalization in the subject here. FTM and PWM. Furthermore this could probably use more than a single line as patch description. Yes, I will 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 3/4] ARM: dts: Enables ftm pwm device for Vybrid VF610 TOWER board.
Hi Thierry, Subject: Re: [PATCH 3/4] ARM: dts: Enables ftm pwm device for Vybrid VF610 TOWER board. On Wed, Aug 21, 2013 at 11:07:41AM +0800, Xiubo Li wrote: Signed-off-by: Xiubo Li li.xi...@freescale.com --- arch/arm/boot/dts/vf610-twr.dts | 17 + 1 file changed, 17 insertions(+) Same comments as for patch 2/4. Yes, I will revise it then. Thanks very much. -- Best Regards. Xiubo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4] pwm: add freescale ftm pwm driver support
Hi Thierry,
See the comments below.
I think the correct capitalizations are: Freescale, FTM and PWM.
There are other occurrences in the rest of the driver that I haven't
explicitly pointed out.
Yes, that's much better.
+config PWM_FTM
Perhaps name this PWM_FSL_FTM to match the driver name?
OK.
And fix this up to be pwm-fsl-ftm.
Yes, I will.
+#define FTM_CSC_BASE0x0C
+#define FTM_CSC(_CHANNEL) \
+ (FTM_CSC_BASE + (_CHANNEL * 0x08))
I prefer lowercase variables in macros:
#define FTM_CSC(channel) \
(FTM_CSC_BASE + (channel * 8))
Yes, That's better.
+#define FTM_PWMMODE (FTMCnSC_MSB)
+#define FTM_HIGH_TRUE (FTMCnSC_ELSB)
+#define FTM_LOW_TRUE(FTMCnSC_ELSA)
What's the reason for redefining these? Can't you just use the FTMCnSC_*
defines directly?
Just for more readable and comprehension.
I will revise it by not losing above two key elements.
+#define FTM_CV(_CHANNEL) \
+ (FTM_CV_BASE + (_CHANNEL * 0x08))
Same comment as for FTM_CSC above.
Yes.
+#define FTM_MAX_CHANNEL 0x08
There should be no need for this. The only place where you use this is
when assigning it to pwm_chip.npwm, so you may as well use the literal
there.
I have recoded the driver, and the macro is used more than one time now.
+ struct platform_device *pdev;
You never use this platform_device. And you have to assign pdev-dev to
the pwm_chip.dev anyway, so why not just use that consistently and drop
the pdev field?
Yes, I have droped the pdev field now.
+ unsigned intcpwm;
+ struct fsl_pwm fpwms[FTM_MAX_CHANNEL];
Please don't do this. Use pwm_set_chip_data()/pwm_get_chip_data() to
associate per-channel specific data.
I have replaced them now.
+ /* pinctrl handles */
Nit: it's only a single handle.
Yes.
+ struct pinctrl *pinctrl;
You also use tabs and spaces inconsistently here. I suggest you use a
single space between the data type and the field name, that way it's much
easier to stay consistent.
Now I have revised all about this.
+static int fsl_pwm_request_channel(struct pwm_chip *chip,
+ struct pwm_device *pwm)
The arguments on the trailing line aren't properly aligned. They should
be aligned with the arguments on the first line.
The same as the above comment.
+ ret = clk_prepare_enable(fpc-clk);
This should probably be just clk_prepare(). Or is there some reason why
you can't delay clk_enable() to the .enable() operation?
Firstly, we should be clear that the fpc-clk is chip's work clock.
If so, after the .request() is called and before .enable() is called, the
custumer will call .config(),
in which will read/write the pwm chip registers, if the module clock is still
disabled, then the system will hang up.
+static int fsl_updata_config(struct fsl_pwm_chip *fpc,
+struct pwm_device *pwm)
Parameter alignment again. Please also check all other functions.
Yes, I have revise all about this.
+{
+ int i;
This should be unsigned int.
I will revise it.
+static unsigned long
+fsl_rate_to_cycles(struct fsl_pwm_chip *fpc, int time_ns)
The above two lines are 78 characters when joined, so there's no need to
split them.
OK, I have revise all about this.
Perhaps time_ns should be unsigned long?
Shouldn't this be same with int duty_ns and int period_ns, which are
defined by
struct pwm_ops {
...
int (*config)(struct pwm_chip *chip,
struct pwm_device *pwm,
int duty_ns, int period_ns);
...
} ?
+ ps = (0x01 fpc-clk_ps);
This is fairly short, so you could do it along with the variable
declaration. Also there's no need for the parentheses or the hexadecimal
prefix (0x01 == 1):
unsigned long ps = 1 fpc-clk_ps;
OK, I will revise it then.
+ /* The module clk is HZ/s, the time_ns parameter
+* is based nanosecond, so here should divide
+* 10UL.
+*/
Block comments should be:
/*
* ...
* ...
*/
Also HZ/s isn't a valid unit for a clock frequency. And time_ns also has
the _ns suffix to designate the unit, so as a whole that comment doesn't
add any real information and can just as well be dropped.
I will revise it.
+static int fsl_pwm_config_channel(struct pwm_chip *chip,
I think you can safely drop the _channel suffix from the PWM operations.
By adding _channel suffix just for more comprehensave about the pwm's
muti-channel operation.
If this is redundant here, I will drop it.
+ fpc = to_fsl_chip(chip);
+
+ if (WARN_ON(!test_bit(PWMF_REQUESTED, pwm-flags)))
+ return -ESHUTDOWN;
Erm... how do you think this could ever happen? Users need to request a
PWM to obtain a struct pwm_device, in which case PWMF_REQUESTED will
always be set. There are a few other occurrences
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
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 +, 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
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 +, 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
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 +, 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4] Documentation: Add device tree bindings for Freescale FTM PWM
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4] pwm: add freescale ftm pwm driver support
TO Sascha,
Thanks very much for your quick reply.
> > > > + fpc = to_fsl_chip(chip);
> > > > +
> > > > + if (WARN_ON(!test_bit(PWMF_REQUESTED, >flags)))
> > > > + return -ESHUTDOWN;
> > > > +
> > > > + statename = kasprintf(GFP_KERNEL, "en%d", pwm->hwpwm);
> > > > + pins_state = pinctrl_lookup_state(fpc->pinctrl,
> > > > + statename);
> > > > + /* enable pins to be muxed in and configured */
> > > > + if (!IS_ERR(pins_state)) {
> > > > + ret = pinctrl_select_state(fpc->pinctrl, pins_state);
> > > > + if (ret)
> > > > + dev_warn(>pdev->dev,
> > > > + "could not set default pins\n");
> > >
> > > 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.
> >
>
> I think the inactive state of a PWM is pretty much undefined by the PWM
> framework and left to the drivers.
>
> I stumbled upon this aswell. It would be good to think about the inactive
> state and how the PWM framework could help us here getting things right.
>
> There are several things to consider. For a noninverted PWM the inactive
> state should probably logic 0. For an inverted PWM it should probably be
> logic 1. I guess several PWM devices have an undefined inactive state,
> most of the PWM devices probably can control the inactive state by
> setting the duty cycle to 100% / 0% without actually disabling the PWM.
>
> Using the pinctrl is one way to control the inactive state and probaby
> the only one before initialization. It might be good to wire this up in
> the core instead of the individual PWM drivers.
>
> These are just the thoughts which first came to my mind.
>
That's a very good idea, and I have also thought about it before.
But from the power dissipation:
If so, upon my board is up, the pwm core should be alive even I won't use it.
I think the pwm core should be in idle mode when not using it,
when any of it's channels is requested and enabled, the pwm core will keep
alive.
What do you think about it ?
> Thierry, any more input about this?
>
>
> > > > + fpc = dev_get_drvdata(dev);
> > > > +
> > > > + ret = kstrtouint(buf, 0, );
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + mutex_lock(>lock);
> > > > + if (!!(val) != !!(fpc->cpwm)) {
> > > > + fpc->cpwm = !!val;
> > > > + fsl_updata_config(fpc, NULL);
> > > > + }
> > > > + mutex_unlock(>lock);
> > > > +
> > > > + return count;
> > > > +}
> > >
> > > What is this cpwm thingy?
> >
> > Up-down counting mode:
> > CNTIN(a register) defines the starting value of the count and MOD(a
> > register) defines the final value of the count. The value of CNTIN is
> > loaded into the FTM counter, and the counter increments until the
> > value of MOD is reached, at which point the counter is decremented
> until it returns to the value of CNTIN and the up-down counting restarts.
>
> The current PWM framework only cares about period times and duty cycles.
> Why would I want to care about this?
I will think it over, If this hasn't any help here, I'll drop it in v2.
Thanks very much.
--
Best Regards,
Xiubo Li
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4] pwm: add freescale ftm pwm driver support
TO Sascha,
Thanks very much for your comments.
> Subject: Re: [PATCH 1/4] pwm: add freescale ftm pwm driver support
>
> On Wed, Aug 21, 2013 at 11:07:39AM +0800, Xiubo Li wrote:
> > +
> > +#define FTM_CSC_BASE0x0C
> > +#define FTM_CSC(_CHANNEL) \
> > + (FTM_CSC_BASE + (_CHANNEL * 0x08))
>
> I see this more and more in FSL code. This is unsafe! Consider what
> happens when we call FTM_CSC(1 + 1). The result is certainly not what you
> want.
>
Oh yes, I'll fix it in v2.
> > +
> > +static int fsl_pwm_request_channel(struct pwm_chip *chip,
> > + struct pwm_device *pwm)
> > +{
> > + int ret = 0;
>
> No need to initialize this variable.
>
Yeah, I'll fix it in v2.
> > + ret = clk_prepare_enable(fpc->clk);
> > + if (ret) {
> > + dev_err(>pdev->dev,
> > + "failed to clk_prepare_enable "
> > + "ftm pwm module clock : %d\n",
> > + ret);
>
> Just return ret. We do not need a message for each failed function in the
> kernel.
>
OK, I will remove it in v2.
> > +static int fsl_updata_config(struct fsl_pwm_chip *fpc,
> > +struct pwm_device *pwm)
> > +{
> > + int i;
> > + unsigned long reg, cntin = FTM_CNTIN_VAL;
> > + struct pwm_chip *chip = >chip;
> > +
> > + reg = readl(fpc->base + FTM_SC);
> > + reg &= ~(FTMSC_CPWMS);
> > + reg |= (fpc->cpwm ? FTMSC_CPWMS : 0x00);
> > + writel(reg, fpc->base + FTM_SC);
> > +
> > + if (pwm && fpc->cpwm) {
> > + writel(fpc->fpwms[pwm->hwpwm].period_cycles / 2 + cntin,
> > + fpc->base + FTM_MOD);
> > + writel(fpc->fpwms[pwm->hwpwm].duty_cycles / 2 + cntin,
> > + fpc->base + FTM_CV(pwm->hwpwm));
> > + } else if (pwm) {
> > + writel(fpc->fpwms[pwm->hwpwm].period_cycles + cntin - 1,
> > + fpc->base + FTM_MOD);
> > + writel(fpc->fpwms[pwm->hwpwm].duty_cycles + cntin,
> > + fpc->base + FTM_CV(pwm->hwpwm));
> > + }
> > +
> > + if (pwm)
> > + return 0;
> > +
> > + for (i = 0; i < chip->npwm; i++) {
> > + if (!test_bit(PWMF_ENABLED, >pwms[i].flags))
> > + continue;
> > + if (fpc->cpwm) {
> > + writel(fpc->fpwms[i].period_cycles / 2 + cntin,
> > + fpc->base + FTM_MOD);
> > + writel(fpc->fpwms[i].duty_cycles / 2 + cntin,
> > + fpc->base + FTM_CV(i));
> > + } else {
> > + writel(fpc->fpwms[i].period_cycles + cntin - 1,
> > + fpc->base + FTM_MOD);
> > + writel(fpc->fpwms[i].duty_cycles + cntin,
> > + fpc->base + FTM_CV(i));
> > + }
> > + }
> > +
>
> The behaviour of this function is completely different for pwm == NULL
> and pwm != NULL. This indicates that it should really be two functions.
> This probably makes the intention of this code much clearer.
>
OK, I will split it into two functions in v2.
> > + period_cycles = fsl_rate_to_cycles(fpc, period_ns);
> > + if (period_cycles > 0x) {
> > + dev_warn(>pdev->dev,
> > + "required PWM period cycles(%lu) "
> > + "overflow 16-bits counter!\n",
> > + period_cycles);
> > + period_cycles = 0x;
>
> If you can't fulfill the requirements you have to return an error. It's
> the caller that needs to know the failure. The caller can't read read the
> syslog.
>
OK, I will fix it in v2.
> > +static int fsl_pwm_enable_channel(struct pwm_chip *chip,
> > + struct pwm_device *pwm)
> > +{
> > + int ret;
> > + unsigned long reg;
> > + struct fsl_pwm_chip *fpc;
> > + struct pinctrl_state *pins_state;
> > + const char *statename;
> > +
> > + fpc = to_fsl_chip(chip);
> > +
> > + if (WARN_ON(!test_bit(PWMF_REQUESTED, >flags)))
> > + return -ESHUTDOWN;
> > +
> > + statename = kasprintf(GFP_KERNEL, "en%d", pwm->hwpwm);
> > + pins_state = pinctrl_lookup_state(fpc->pinctrl,
> > + statename);
> > + /* enable pins to be muxed in and configured */
> > + if (!IS_ERR(pins_state)) {
> > + ret = pinctrl_select_state(fpc->pinctrl, pins_state);
> > + if (ret)
> > + dev_warn(>pdev->dev,
> > + "could not set default pins\n");
>
> 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
RE: [PATCH 1/4] pwm: add freescale ftm pwm driver support
TO Sascha,
Thanks very much for your comments.
Subject: Re: [PATCH 1/4] pwm: add freescale ftm pwm driver support
On Wed, Aug 21, 2013 at 11:07:39AM +0800, Xiubo Li wrote:
+
+#define FTM_CSC_BASE0x0C
+#define FTM_CSC(_CHANNEL) \
+ (FTM_CSC_BASE + (_CHANNEL * 0x08))
I see this more and more in FSL code. This is unsafe! Consider what
happens when we call FTM_CSC(1 + 1). The result is certainly not what you
want.
Oh yes, I'll fix it in v2.
+
+static int fsl_pwm_request_channel(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
+ int ret = 0;
No need to initialize this variable.
Yeah, I'll fix it in v2.
+ ret = clk_prepare_enable(fpc-clk);
+ if (ret) {
+ dev_err(fpc-pdev-dev,
+ failed to clk_prepare_enable
+ ftm pwm module clock : %d\n,
+ ret);
Just return ret. We do not need a message for each failed function in the
kernel.
OK, I will remove it in v2.
+static int fsl_updata_config(struct fsl_pwm_chip *fpc,
+struct pwm_device *pwm)
+{
+ int i;
+ unsigned long reg, cntin = FTM_CNTIN_VAL;
+ struct pwm_chip *chip = fpc-chip;
+
+ reg = readl(fpc-base + FTM_SC);
+ reg = ~(FTMSC_CPWMS);
+ reg |= (fpc-cpwm ? FTMSC_CPWMS : 0x00);
+ writel(reg, fpc-base + FTM_SC);
+
+ if (pwm fpc-cpwm) {
+ writel(fpc-fpwms[pwm-hwpwm].period_cycles / 2 + cntin,
+ fpc-base + FTM_MOD);
+ writel(fpc-fpwms[pwm-hwpwm].duty_cycles / 2 + cntin,
+ fpc-base + FTM_CV(pwm-hwpwm));
+ } else if (pwm) {
+ writel(fpc-fpwms[pwm-hwpwm].period_cycles + cntin - 1,
+ fpc-base + FTM_MOD);
+ writel(fpc-fpwms[pwm-hwpwm].duty_cycles + cntin,
+ fpc-base + FTM_CV(pwm-hwpwm));
+ }
+
+ if (pwm)
+ return 0;
+
+ for (i = 0; i chip-npwm; i++) {
+ if (!test_bit(PWMF_ENABLED, chip-pwms[i].flags))
+ continue;
+ if (fpc-cpwm) {
+ writel(fpc-fpwms[i].period_cycles / 2 + cntin,
+ fpc-base + FTM_MOD);
+ writel(fpc-fpwms[i].duty_cycles / 2 + cntin,
+ fpc-base + FTM_CV(i));
+ } else {
+ writel(fpc-fpwms[i].period_cycles + cntin - 1,
+ fpc-base + FTM_MOD);
+ writel(fpc-fpwms[i].duty_cycles + cntin,
+ fpc-base + FTM_CV(i));
+ }
+ }
+
The behaviour of this function is completely different for pwm == NULL
and pwm != NULL. This indicates that it should really be two functions.
This probably makes the intention of this code much clearer.
OK, I will split it into two functions in v2.
+ period_cycles = fsl_rate_to_cycles(fpc, period_ns);
+ if (period_cycles 0x) {
+ dev_warn(fpc-pdev-dev,
+ required PWM period cycles(%lu)
+ overflow 16-bits counter!\n,
+ period_cycles);
+ period_cycles = 0x;
If you can't fulfill the requirements you have to return an error. It's
the caller that needs to know the failure. The caller can't read read the
syslog.
OK, I will fix it in v2.
+static int fsl_pwm_enable_channel(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
+ int ret;
+ unsigned long reg;
+ struct fsl_pwm_chip *fpc;
+ struct pinctrl_state *pins_state;
+ const char *statename;
+
+ fpc = to_fsl_chip(chip);
+
+ if (WARN_ON(!test_bit(PWMF_REQUESTED, pwm-flags)))
+ return -ESHUTDOWN;
+
+ statename = kasprintf(GFP_KERNEL, en%d, pwm-hwpwm);
+ pins_state = pinctrl_lookup_state(fpc-pinctrl,
+ statename);
+ /* enable pins to be muxed in and configured */
+ if (!IS_ERR(pins_state)) {
+ ret = pinctrl_select_state(fpc-pinctrl, pins_state);
+ if (ret)
+ dev_warn(fpc-pdev-dev,
+ could not set default pins\n);
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.
RE: [PATCH 1/4] pwm: add freescale ftm pwm driver support
TO Sascha,
Thanks very much for your quick reply.
+ fpc = to_fsl_chip(chip);
+
+ if (WARN_ON(!test_bit(PWMF_REQUESTED, pwm-flags)))
+ return -ESHUTDOWN;
+
+ statename = kasprintf(GFP_KERNEL, en%d, pwm-hwpwm);
+ pins_state = pinctrl_lookup_state(fpc-pinctrl,
+ statename);
+ /* enable pins to be muxed in and configured */
+ if (!IS_ERR(pins_state)) {
+ ret = pinctrl_select_state(fpc-pinctrl, pins_state);
+ if (ret)
+ dev_warn(fpc-pdev-dev,
+ could not set default pins\n);
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.
I think the inactive state of a PWM is pretty much undefined by the PWM
framework and left to the drivers.
I stumbled upon this aswell. It would be good to think about the inactive
state and how the PWM framework could help us here getting things right.
There are several things to consider. For a noninverted PWM the inactive
state should probably logic 0. For an inverted PWM it should probably be
logic 1. I guess several PWM devices have an undefined inactive state,
most of the PWM devices probably can control the inactive state by
setting the duty cycle to 100% / 0% without actually disabling the PWM.
Using the pinctrl is one way to control the inactive state and probaby
the only one before initialization. It might be good to wire this up in
the core instead of the individual PWM drivers.
These are just the thoughts which first came to my mind.
That's a very good idea, and I have also thought about it before.
But from the power dissipation:
If so, upon my board is up, the pwm core should be alive even I won't use it.
I think the pwm core should be in idle mode when not using it,
when any of it's channels is requested and enabled, the pwm core will keep
alive.
What do you think about it ?
Thierry, any more input about this?
+ fpc = dev_get_drvdata(dev);
+
+ ret = kstrtouint(buf, 0, val);
+ if (ret)
+ return ret;
+
+ mutex_lock(fpc-lock);
+ if (!!(val) != !!(fpc-cpwm)) {
+ fpc-cpwm = !!val;
+ fsl_updata_config(fpc, NULL);
+ }
+ mutex_unlock(fpc-lock);
+
+ return count;
+}
What is this cpwm thingy?
Up-down counting mode:
CNTIN(a register) defines the starting value of the count and MOD(a
register) defines the final value of the count. The value of CNTIN is
loaded into the FTM counter, and the counter increments until the
value of MOD is reached, at which point the counter is decremented
until it returns to the value of CNTIN and the up-down counting restarts.
The current PWM framework only cares about period times and duty cycles.
Why would I want to care about this?
I will think it over, If this hasn't any help here, I'll drop it in v2.
Thanks very much.
--
Best Regards,
Xiubo Li
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
