Re: [PATCH] pwm: add spear pwm driver support
On 19 October 2012 15:31, Shiraz Hashim wrote: > On Fri, Oct 19, 2012 at 11:29:43AM +0530, Shiraz HASHIM wrote: >> On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote: >> > On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim >> > wrote: >> > > + pc->mmio_base = devm_request_and_ioremap(>dev, r); >> > > + if (!pc->mmio_base) >> > > + return -EADDRNOTAVAIL; >> > >> > Just check, i believe there is a routine which will do above two in a >> > single >> > call.. >> > >> >> I would cross check. >> > > Couldn't find a suitable call. Even i couldn't :( But i remember there was something similar.. No issues, leave it. -- 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] pwm: add spear pwm driver support
On Fri, Oct 19, 2012 at 11:29:43AM +0530, Shiraz HASHIM wrote: > On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote: > > On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim wrote: > > > + pc->mmio_base = devm_request_and_ioremap(>dev, r); > > > + if (!pc->mmio_base) > > > + return -EADDRNOTAVAIL; > > > > Just check, i believe there is a routine which will do above two in a single > > call.. > > > > I would cross check. > Couldn't find a suitable call. -- regards Shiraz -- 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] pwm: add spear pwm driver support
On 19 October 2012 15:13, Shiraz Hashim wrote:
> It may not be required as pwms which are not enabled do not have
> their clocks enabled. Hence, perhaps we can do following,
>
> 8<---
> static int spear_pwm_remove(struct platform_device *pdev)
> {
> struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
> int i;
>
> if (WARN_ON(!pc))
> return -ENODEV;
>
> for (i = 0; i < NUM_PWM; i++) {
> struct pwm_device *pwm = >chip.pwms[i];
>
> if (test_bit(PWMF_ENABLED, >flags)) {
> spear_pwm_writel(pc, i, PWMCR, 0);
> clk_disable(pc->clk);
> }
> }
>
> /* clk was prepared in probe, hence unprepare it here */
> clk_unprepare(pc->clk);
> return pwmchip_remove(>chip);
> }
Better.
--
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] pwm: add spear pwm driver support
Hi Viresh,
On Fri, Oct 19, 2012 at 12:23:08PM +0530, viresh kumar wrote:
> On Fri, Oct 19, 2012 at 11:29 AM, Shiraz Hashim wrote:
> > On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote:
> >> On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim
> >> wrote:
>
> >> > +static int __devexit spear_pwm_remove(struct platform_device *pdev)
> >> > +{
> >> > + struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
> >> > + int i;
> >> > +
> >> > + if (WARN_ON(!pc))
> >> > + return -ENODEV;
> >> > +
> >> > + for (i = 0; i < NUM_PWM; i++) {
> >> > + struct pwm_device *pwmd = >chip.pwms[i];
> >> > +
> >> > + if (!test_bit(PWMF_ENABLED, >flags))
>
> One point here: If i am not wrong you want to disable pwmd if it is enabled.
> Shouldn't you check for if (test_bit(PWMF_ENABLED, >flags)) instead?
>
You are right. The code is un-necessarily disabling all pwms while trying to
enable the clock for those which are not enabled also.
> >> > + if (clk_prepare_enable(pc->clk) < 0)
> >> > + continue;
> >> > +
> >> > + spear_pwm_writel(pc, i, PWMCR, 0);
> >> > + clk_disable_unprepare(pc->clk);
> >> > + }
> >>
> >> You are enabling/disabling clock N times here and each of these will
> >> write to an register. Do something better.
> >>
> >
> > I need to shut down all active pwms, how else would you suggest that ?
>
> Sorry, i misread the code on the second go :(
>
> I am proposing something like:
>
> static int __devexit spear_pwm_remove(struct platform_device *pdev)
> {
>struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
>int i, clk_enabled = 0;
>
>if (WARN_ON(!pc))
>return -ENODEV;
>
>for (i = 0; i < NUM_PWM; i++) {
>struct pwm_device *pwmd = >chip.pwms[i];
>
>if (!test_bit(PWMF_ENABLED, >flags) && !clk_enabled)
>if (clk_prepare_enable(pc->clk) < 0)
>continue;
> else
> clk_enabled++;
>
>spear_pwm_writel(pc, i, PWMCR, 0);
>}
>
>if (clk_enabled)
> clk_disable_unprepare(pc->clk);
>...
> }
It may not be required as pwms which are not enabled do not have
their clocks enabled. Hence, perhaps we can do following,
8<---
static int spear_pwm_remove(struct platform_device *pdev)
{
struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
int i;
if (WARN_ON(!pc))
return -ENODEV;
for (i = 0; i < NUM_PWM; i++) {
struct pwm_device *pwm = >chip.pwms[i];
if (test_bit(PWMF_ENABLED, >flags)) {
spear_pwm_writel(pc, i, PWMCR, 0);
clk_disable(pc->clk);
}
}
/* clk was prepared in probe, hence unprepare it here */
clk_unprepare(pc->clk);
return pwmchip_remove(>chip);
}
>8
--
regards
Shiraz
--
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] pwm: add spear pwm driver support
On Fri, Oct 19, 2012 at 11:29 AM, Shiraz Hashim wrote:
> On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote:
>> On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim wrote:
>> > +static int __devexit spear_pwm_remove(struct platform_device *pdev)
>> > +{
>> > + struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
>> > + int i;
>> > +
>> > + if (WARN_ON(!pc))
>> > + return -ENODEV;
>> > +
>> > + for (i = 0; i < NUM_PWM; i++) {
>> > + struct pwm_device *pwmd = >chip.pwms[i];
>> > +
>> > + if (!test_bit(PWMF_ENABLED, >flags))
One point here: If i am not wrong you want to disable pwmd if it is enabled.
Shouldn't you check for if (test_bit(PWMF_ENABLED, >flags)) instead?
>> > + if (clk_prepare_enable(pc->clk) < 0)
>> > + continue;
>> > +
>> > + spear_pwm_writel(pc, i, PWMCR, 0);
>> > + clk_disable_unprepare(pc->clk);
>> > + }
>>
>> You are enabling/disabling clock N times here and each of these will
>> write to an register. Do something better.
>>
>
> I need to shut down all active pwms, how else would you suggest that ?
Sorry, i misread the code on the second go :(
I am proposing something like:
static int __devexit spear_pwm_remove(struct platform_device *pdev)
{
struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
int i, clk_enabled = 0;
if (WARN_ON(!pc))
return -ENODEV;
for (i = 0; i < NUM_PWM; i++) {
struct pwm_device *pwmd = >chip.pwms[i];
if (!test_bit(PWMF_ENABLED, >flags) && !clk_enabled)
if (clk_prepare_enable(pc->clk) < 0)
continue;
else
clk_enabled++;
spear_pwm_writel(pc, i, PWMCR, 0);
}
if (clk_enabled)
clk_disable_unprepare(pc->clk);
...
}
--
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] pwm: add spear pwm driver support
On 19 October 2012 11:29, Shiraz Hashim wrote: >> > + clk_disable_unprepare(pc->clk); >> >> call only disable from here. Leave it prepared for ever. >> > > and unprepare only in _remove. Okay. yes. > I need to shut down all active pwms, how else would you suggest that ? I misread the code. Leave this comment. -- 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] pwm: add spear pwm driver support
Hi Viresh,
On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote:
> On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim wrote:
> > diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> > b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> > +== ST SPEAr SoC PWM controller ==
> > +
> > +Required properties:
> > +- compatible: should be one of:
> > + - "st,spear-pwm"
> > + - "st,spear13xx-pwm"
>
> This has be matching with an version of the IP, as discussed earlier for many
> driver.
>
> Because ST hasn't released any specific version numbers, you must mention
> name of the SoC where the IP first appeared. That will mark its version. So,
> in short don't use generic names like spear or spear13xx :)
>
Okay. So I would rename compatible fields and accordingly as suggested
by Thierry, I would choose doc file name as 'spear-pwm.txt'.
> > +- reg: physical base address and length of the controller's registers
> > +- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on
> > SPEAr. The
> > + first cell specifies the per-chip index of the PWM to use and the second
> > + cell is the duty cycle in nanoseconds.
> > +
> > +Example:
> > +
> > +pwm: pwm@a800 {
> > +compatible ="st,spear-pwm";
> > +reg = <0xa800 0x1000>;
> > +#pwm-cells = <2>;
> > +status = "disabled";
> > +};
>
> An example on how other nodes will link to it by passing id and duty cycle
> would be better.
>
I don't think it is required here, it is already covered in pwm.txt.
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index ed81720..3ff3e6f 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -112,6 +112,16 @@ config PWM_SAMSUNG
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-samsung.
> >
> > +config PWM_SPEAR
> > + tristate "STMicroelectronics SPEAR PWM support"
>
> SPEAr
>
Yes, already pointed by Thierry.
> > + depends on PLAT_SPEAR
> > + help
> > + Generic PWM framework driver for the PWM controller on ST
> > + SPEAr SoCs.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-spear.
> > +
> > config PWM_TEGRA
> > tristate "NVIDIA Tegra PWM support"
> > depends on ARCH_TEGRA
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index acfe482..6512786 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
> > obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
> > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
> > obj-$(CONFIG_PWM_TEGRA)+= pwm-tegra.o
> > +obj-$(CONFIG_PWM_SPEAR)+= pwm-spear.o
> > obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
> > obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
> > obj-$(CONFIG_PWM_TWL6030) += pwm-twl6030.o
> > diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
> > new file mode 100644
> > index 000..814395b
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-spear.c
> > @@ -0,0 +1,287 @@
> > +/*
> > + * ST Microelectronics SPEAr Pulse Width Modulator driver
> > + *
> > + * Copyright (C) 2012 ST Microelectronics
> > + * Shiraz Hashim
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +/* PWM registers and bits definitions */
> > +#define PWMCR 0x00/* Control Register */
> > +#define PWMDCR 0x04/* Duty Cycle Register */
> > +#define PWMPCR 0x08/* Period Register */
> > +/* Following only available on 13xx SoCs */
> > +#define PWMMCR 0x3C/* Master Control Register */
> > +
> > +#define PWM_ENABLE 0x1
> > +
> > +#define MIN_PRESCALE 0x00
> > +#define MAX_PRESCALE 0x3FFF
> > +#define PRESCALE_SHIFT 2
> > +
> > +#define MIN_DUTY 0x0001
> > +#define MAX_DUTY 0x
> > +
> > +#define MIN_PERIOD 0x0001
> > +#define MAX_PERIOD 0x
> > +
> > +#define NUM_PWM4
> > +
> > +/**
> > + * struct pwm: struct representing pwm ip
>
> spear_pwm_chip: struct representing pwm chip
>
The whole kernel doc requires a fix, would do that.
> > + * mmio_base: base address of pwm
>
> base would be enough.
>
mmio_base isn't too bad.
> > + * clk: pointer to clk structure of pwm ip
> > + * chip: linux pwm chip representation
> > + * dev: pointer to device structure of pwm
> > + */
> > +struct spear_pwm_chip {
> > + void
Re: [PATCH] pwm: add spear pwm driver support
Hi Viresh,
On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote:
On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim shiraz.has...@st.com wrote:
diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
+== ST SPEAr SoC PWM controller ==
+
+Required properties:
+- compatible: should be one of:
+ - st,spear-pwm
+ - st,spear13xx-pwm
This has be matching with an version of the IP, as discussed earlier for many
driver.
Because ST hasn't released any specific version numbers, you must mention
name of the SoC where the IP first appeared. That will mark its version. So,
in short don't use generic names like spear or spear13xx :)
Okay. So I would rename compatible fields and accordingly as suggested
by Thierry, I would choose doc file name as 'spear-pwm.txt'.
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on
SPEAr. The
+ first cell specifies the per-chip index of the PWM to use and the second
+ cell is the duty cycle in nanoseconds.
+
+Example:
+
+pwm: pwm@a800 {
+compatible =st,spear-pwm;
+reg = 0xa800 0x1000;
+#pwm-cells = 2;
+status = disabled;
+};
An example on how other nodes will link to it by passing id and duty cycle
would be better.
I don't think it is required here, it is already covered in pwm.txt.
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..3ff3e6f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -112,6 +112,16 @@ config PWM_SAMSUNG
To compile this driver as a module, choose M here: the module
will be called pwm-samsung.
+config PWM_SPEAR
+ tristate STMicroelectronics SPEAR PWM support
SPEAr
Yes, already pointed by Thierry.
+ depends on PLAT_SPEAR
+ help
+ Generic PWM framework driver for the PWM controller on ST
+ SPEAr SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-spear.
+
config PWM_TEGRA
tristate NVIDIA Tegra PWM support
depends on ARCH_TEGRA
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..6512786 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
obj-$(CONFIG_PWM_TEGRA)+= pwm-tegra.o
+obj-$(CONFIG_PWM_SPEAR)+= pwm-spear.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
obj-$(CONFIG_PWM_TWL6030) += pwm-twl6030.o
diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
new file mode 100644
index 000..814395b
--- /dev/null
+++ b/drivers/pwm/pwm-spear.c
@@ -0,0 +1,287 @@
+/*
+ * ST Microelectronics SPEAr Pulse Width Modulator driver
+ *
+ * Copyright (C) 2012 ST Microelectronics
+ * Shiraz Hashim shiraz.has...@st.com
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed as is without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include linux/clk.h
+#include linux/err.h
+#include linux/io.h
+#include linux/ioport.h
+#include linux/kernel.h
+#include linux/math64.h
+#include linux/module.h
+#include linux/of.h
+#include linux/platform_device.h
+#include linux/pwm.h
+#include linux/slab.h
+
+/* PWM registers and bits definitions */
+#define PWMCR 0x00/* Control Register */
+#define PWMDCR 0x04/* Duty Cycle Register */
+#define PWMPCR 0x08/* Period Register */
+/* Following only available on 13xx SoCs */
+#define PWMMCR 0x3C/* Master Control Register */
+
+#define PWM_ENABLE 0x1
+
+#define MIN_PRESCALE 0x00
+#define MAX_PRESCALE 0x3FFF
+#define PRESCALE_SHIFT 2
+
+#define MIN_DUTY 0x0001
+#define MAX_DUTY 0x
+
+#define MIN_PERIOD 0x0001
+#define MAX_PERIOD 0x
+
+#define NUM_PWM4
+
+/**
+ * struct pwm: struct representing pwm ip
spear_pwm_chip: struct representing pwm chip
The whole kernel doc requires a fix, would do that.
+ * mmio_base: base address of pwm
base would be enough.
mmio_base isn't too bad.
+ * clk: pointer to clk structure of pwm ip
+ * chip: linux pwm chip representation
+ * dev: pointer to device structure of pwm
+ */
+struct spear_pwm_chip {
+ void __iomem *mmio_base;
+ struct clk *clk;
+ struct pwm_chip chip;
+
Re: [PATCH] pwm: add spear pwm driver support
On 19 October 2012 11:29, Shiraz Hashim shiraz.has...@st.com wrote: + clk_disable_unprepare(pc-clk); call only disable from here. Leave it prepared for ever. and unprepare only in _remove. Okay. yes. I need to shut down all active pwms, how else would you suggest that ? I misread the code. Leave this comment. -- 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] pwm: add spear pwm driver support
On Fri, Oct 19, 2012 at 11:29 AM, Shiraz Hashim shiraz.has...@st.com wrote:
On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote:
On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim shiraz.has...@st.com wrote:
+static int __devexit spear_pwm_remove(struct platform_device *pdev)
+{
+ struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
+ int i;
+
+ if (WARN_ON(!pc))
+ return -ENODEV;
+
+ for (i = 0; i NUM_PWM; i++) {
+ struct pwm_device *pwmd = pc-chip.pwms[i];
+
+ if (!test_bit(PWMF_ENABLED, pwmd-flags))
One point here: If i am not wrong you want to disable pwmd if it is enabled.
Shouldn't you check for if (test_bit(PWMF_ENABLED, pwmd-flags)) instead?
+ if (clk_prepare_enable(pc-clk) 0)
+ continue;
+
+ spear_pwm_writel(pc, i, PWMCR, 0);
+ clk_disable_unprepare(pc-clk);
+ }
You are enabling/disabling clock N times here and each of these will
write to an register. Do something better.
I need to shut down all active pwms, how else would you suggest that ?
Sorry, i misread the code on the second go :(
I am proposing something like:
static int __devexit spear_pwm_remove(struct platform_device *pdev)
{
struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
int i, clk_enabled = 0;
if (WARN_ON(!pc))
return -ENODEV;
for (i = 0; i NUM_PWM; i++) {
struct pwm_device *pwmd = pc-chip.pwms[i];
if (!test_bit(PWMF_ENABLED, pwmd-flags) !clk_enabled)
if (clk_prepare_enable(pc-clk) 0)
continue;
else
clk_enabled++;
spear_pwm_writel(pc, i, PWMCR, 0);
}
if (clk_enabled)
clk_disable_unprepare(pc-clk);
...
}
--
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] pwm: add spear pwm driver support
Hi Viresh,
On Fri, Oct 19, 2012 at 12:23:08PM +0530, viresh kumar wrote:
On Fri, Oct 19, 2012 at 11:29 AM, Shiraz Hashim shiraz.has...@st.com wrote:
On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote:
On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim shiraz.has...@st.com
wrote:
+static int __devexit spear_pwm_remove(struct platform_device *pdev)
+{
+ struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
+ int i;
+
+ if (WARN_ON(!pc))
+ return -ENODEV;
+
+ for (i = 0; i NUM_PWM; i++) {
+ struct pwm_device *pwmd = pc-chip.pwms[i];
+
+ if (!test_bit(PWMF_ENABLED, pwmd-flags))
One point here: If i am not wrong you want to disable pwmd if it is enabled.
Shouldn't you check for if (test_bit(PWMF_ENABLED, pwmd-flags)) instead?
You are right. The code is un-necessarily disabling all pwms while trying to
enable the clock for those which are not enabled also.
+ if (clk_prepare_enable(pc-clk) 0)
+ continue;
+
+ spear_pwm_writel(pc, i, PWMCR, 0);
+ clk_disable_unprepare(pc-clk);
+ }
You are enabling/disabling clock N times here and each of these will
write to an register. Do something better.
I need to shut down all active pwms, how else would you suggest that ?
Sorry, i misread the code on the second go :(
I am proposing something like:
static int __devexit spear_pwm_remove(struct platform_device *pdev)
{
struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
int i, clk_enabled = 0;
if (WARN_ON(!pc))
return -ENODEV;
for (i = 0; i NUM_PWM; i++) {
struct pwm_device *pwmd = pc-chip.pwms[i];
if (!test_bit(PWMF_ENABLED, pwmd-flags) !clk_enabled)
if (clk_prepare_enable(pc-clk) 0)
continue;
else
clk_enabled++;
spear_pwm_writel(pc, i, PWMCR, 0);
}
if (clk_enabled)
clk_disable_unprepare(pc-clk);
...
}
It may not be required as pwms which are not enabled do not have
their clocks enabled. Hence, perhaps we can do following,
8---
static int spear_pwm_remove(struct platform_device *pdev)
{
struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
int i;
if (WARN_ON(!pc))
return -ENODEV;
for (i = 0; i NUM_PWM; i++) {
struct pwm_device *pwm = pc-chip.pwms[i];
if (test_bit(PWMF_ENABLED, pwm-flags)) {
spear_pwm_writel(pc, i, PWMCR, 0);
clk_disable(pc-clk);
}
}
/* clk was prepared in probe, hence unprepare it here */
clk_unprepare(pc-clk);
return pwmchip_remove(pc-chip);
}
8
--
regards
Shiraz
--
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] pwm: add spear pwm driver support
On 19 October 2012 15:13, Shiraz Hashim shiraz.has...@st.com wrote:
It may not be required as pwms which are not enabled do not have
their clocks enabled. Hence, perhaps we can do following,
8---
static int spear_pwm_remove(struct platform_device *pdev)
{
struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
int i;
if (WARN_ON(!pc))
return -ENODEV;
for (i = 0; i NUM_PWM; i++) {
struct pwm_device *pwm = pc-chip.pwms[i];
if (test_bit(PWMF_ENABLED, pwm-flags)) {
spear_pwm_writel(pc, i, PWMCR, 0);
clk_disable(pc-clk);
}
}
/* clk was prepared in probe, hence unprepare it here */
clk_unprepare(pc-clk);
return pwmchip_remove(pc-chip);
}
Better.
--
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] pwm: add spear pwm driver support
On Fri, Oct 19, 2012 at 11:29:43AM +0530, Shiraz HASHIM wrote: On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote: On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim shiraz.has...@st.com wrote: + pc-mmio_base = devm_request_and_ioremap(pdev-dev, r); + if (!pc-mmio_base) + return -EADDRNOTAVAIL; Just check, i believe there is a routine which will do above two in a single call.. I would cross check. Couldn't find a suitable call. -- regards Shiraz -- 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] pwm: add spear pwm driver support
On 19 October 2012 15:31, Shiraz Hashim shiraz.has...@st.com wrote: On Fri, Oct 19, 2012 at 11:29:43AM +0530, Shiraz HASHIM wrote: On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote: On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim shiraz.has...@st.com wrote: + pc-mmio_base = devm_request_and_ioremap(pdev-dev, r); + if (!pc-mmio_base) + return -EADDRNOTAVAIL; Just check, i believe there is a routine which will do above two in a single call.. I would cross check. Couldn't find a suitable call. Even i couldn't :( But i remember there was something similar.. No issues, leave it. -- 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] pwm: add spear pwm driver support
On Thu, Oct 18, 2012 at 6:59 PM, Shiraz Hashim wrote: >> Is there a reason to make this conditional? It looks like SPEAr has >> moved to OF, so this will always be enabled anyway, won't it? > > Yes, I would remove it, SPEAr cannot boot without DT. Add a dependency on OF in the Kconfig then. Also i haven't seen any suspend/resume routines here. Missed them? -- 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] pwm: add spear pwm driver support
On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim wrote:
> diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> +== ST SPEAr SoC PWM controller ==
> +
> +Required properties:
> +- compatible: should be one of:
> + - "st,spear-pwm"
> + - "st,spear13xx-pwm"
This has be matching with an version of the IP, as discussed earlier for many
driver.
Because ST hasn't released any specific version numbers, you must mention
name of the SoC where the IP first appeared. That will mark its version. So,
in short don't use generic names like spear or spear13xx :)
> +- reg: physical base address and length of the controller's registers
> +- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on
> SPEAr. The
> + first cell specifies the per-chip index of the PWM to use and the second
> + cell is the duty cycle in nanoseconds.
> +
> +Example:
> +
> +pwm: pwm@a800 {
> +compatible ="st,spear-pwm";
> +reg = <0xa800 0x1000>;
> +#pwm-cells = <2>;
> +status = "disabled";
> +};
An example on how other nodes will link to it by passing id and duty cycle
would be better.
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index ed81720..3ff3e6f 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -112,6 +112,16 @@ config PWM_SAMSUNG
> To compile this driver as a module, choose M here: the module
> will be called pwm-samsung.
>
> +config PWM_SPEAR
> + tristate "STMicroelectronics SPEAR PWM support"
SPEAr
> + depends on PLAT_SPEAR
> + help
> + Generic PWM framework driver for the PWM controller on ST
> + SPEAr SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-spear.
> +
> config PWM_TEGRA
> tristate "NVIDIA Tegra PWM support"
> depends on ARCH_TEGRA
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index acfe482..6512786 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
> obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
> obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
> obj-$(CONFIG_PWM_TEGRA)+= pwm-tegra.o
> +obj-$(CONFIG_PWM_SPEAR)+= pwm-spear.o
> obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
> obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
> obj-$(CONFIG_PWM_TWL6030) += pwm-twl6030.o
> diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
> new file mode 100644
> index 000..814395b
> --- /dev/null
> +++ b/drivers/pwm/pwm-spear.c
> @@ -0,0 +1,287 @@
> +/*
> + * ST Microelectronics SPEAr Pulse Width Modulator driver
> + *
> + * Copyright (C) 2012 ST Microelectronics
> + * Shiraz Hashim
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* PWM registers and bits definitions */
> +#define PWMCR 0x00/* Control Register */
> +#define PWMDCR 0x04/* Duty Cycle Register */
> +#define PWMPCR 0x08/* Period Register */
> +/* Following only available on 13xx SoCs */
> +#define PWMMCR 0x3C/* Master Control Register */
> +
> +#define PWM_ENABLE 0x1
> +
> +#define MIN_PRESCALE 0x00
> +#define MAX_PRESCALE 0x3FFF
> +#define PRESCALE_SHIFT 2
> +
> +#define MIN_DUTY 0x0001
> +#define MAX_DUTY 0x
> +
> +#define MIN_PERIOD 0x0001
> +#define MAX_PERIOD 0x
> +
> +#define NUM_PWM4
> +
> +/**
> + * struct pwm: struct representing pwm ip
spear_pwm_chip: struct representing pwm chip
> + * mmio_base: base address of pwm
base would be enough.
> + * clk: pointer to clk structure of pwm ip
> + * chip: linux pwm chip representation
> + * dev: pointer to device structure of pwm
> + */
> +struct spear_pwm_chip {
> + void __iomem *mmio_base;
> + struct clk *clk;
> + struct pwm_chip chip;
> + struct device *dev;
> +};
> +
> +static inline struct spear_pwm_chip *to_spear_pwm_chip(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct spear_pwm_chip, chip);
> +}
> +
> +static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int
> num,
include types.h for u32
> + unsigned long offset)
> +{
> + return readl_relaxed(chip->mmio_base + (num << 4) + offset);
> +}
> +
> +static inline void spear_pwm_writel(struct spear_pwm_chip *chip,
> + unsigned int num, unsigned long offset, unsigned long val)
> +{
> +
Re: [PATCH] pwm: add spear pwm driver support
On Thu, Oct 18, 2012 at 06:59:28PM +0530, Shiraz Hashim wrote:
> Hi Thierry,
>
> Thanks for the quick review.
>
> On Thu, Oct 18, 2012 at 02:08:20PM +0200, Thierry Reding wrote:
> > On Thu, Oct 18, 2012 at 04:58:32PM +0530, Shiraz Hashim wrote:
[...]
> > > + first cell specifies the per-chip index of the PWM to use and the
> > > second
> > > + cell is the duty cycle in nanoseconds.
> >
> > This should be "period in nanoseconds". I know this is wrong in the
> > binding documentation for other drivers but I've recently committed a
> > patch that fixes it.
>
> Okay but I couldn't see use of pwm->period set by of_pwm_simple_xlate
> anywhere. Am I missing something ?
It's set by the call to pwm_set_period().
> > > +/* PWM registers and bits definitions */
> > > +#define PWMCR0x00/* Control Register */
> > > +#define PWMDCR 0x04/* Duty Cycle Register */
> > > +#define PWMPCR 0x08/* Period Register */
> > > +/* Following only available on 13xx SoCs */
> > > +#define PWMMCR 0x3C/* Master Control Register */
> > > +
> > > +#define PWM_ENABLE 0x1
> > > +
> > > +#define MIN_PRESCALE 0x00
> > > +#define MAX_PRESCALE 0x3FFF
> > > +#define PRESCALE_SHIFT 2
> > > +
> > > +#define MIN_DUTY 0x0001
> > > +#define MAX_DUTY 0x
> > > +
> > > +#define MIN_PERIOD 0x0001
> > > +#define MAX_PERIOD 0x
> >
> > Would it make sense to perhaps group the bitfields with the matching
> > register definitions to make their use more obvious. Also I prefer
> > lowercase hexadecimal digits, but that's pure bikeshedding.
> >
>
> Sure I would group them, but uppercase hexadecimal digits clearly
> seperate the value (number) which otherwise can be mixed (while
> reading) with normal letters. Isn't it ?
As I said, this is really very subjective, so if you prefer uppercase,
feel free to keep it. =)
> > > +static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned
> > > int num,
> > > + unsigned long offset)
> >
> > I personally like it better to have function arguments aligned, like so:
> >
> > static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int
> > num,
> > unsigned long offset)
> >
> > Note, those are as many 8-spaces tabs with only spaces to align them
> > properly. But again, pure bikeshedding and I won't force the issue.
> >
>
> Would do that. Are you aware of some (vim) configuration which can
> autmatically do this while editing code ?
I'm not aware of any such setting, but the idea is interesting. I
usually do that automatically out of habit, but having the editor do it
would be nice as well.
> > __devinit will go away sometime soon, so please don't use it in new
> > code.
> >
>
> Okay. You mean all init sections would eventually be removed. I
> would read more about it.
Yes, commit 45f035a (only in linux-next I think) has some details.
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_AUTHOR("Shiraz Hashim ");
> > > +MODULE_AUTHOR("Viresh Kumar ");
> >
> > I don't think this works: the second entry will replace the first. Have
> > you verified that both authors appear in the output of modinfo?
>
> This is the output of modinfo (compiled for linux-3.5)
>
> $ modinfo pwm-spear.ko
> filename: drivers/pwm/pwm-spear.ko
> alias: platform:st-pwm
> author: Viresh Kumar
> author: Shiraz Hashim
> license:GPL
> alias: of:N*T*Cst,spear13xx-pwm*
> alias: of:N*T*Cst,spear-pwm*
> depends:
> intree: Y
> vermagic: 3.5.0-test-00138-g08e3584 SMP mod_unload modversions ARMv7
> p2v8
Interesting. I thought I'd seen this fail only recently. Will need to
investigate.
> > > +MODULE_ALIAS("platform:st-pwm");
> >
> > Should this not rather be "platform:spear-pwm"?
>
> Yes, I would double check these kind of small issues before
> sending patches next time.
No biggie. That's why we have reviews.
Thierry
pgp6NF7dQPYDX.pgp
Description: PGP signature
Re: [PATCH] pwm: add spear pwm driver support
Hi Thierry,
Thanks for the quick review.
On Thu, Oct 18, 2012 at 02:08:20PM +0200, Thierry Reding wrote:
> On Thu, Oct 18, 2012 at 04:58:32PM +0530, Shiraz Hashim wrote:
> > Add support for pwm devices present on SPEAr platforms. These pwm
> > devices support 4 channel output with programmable duty cycle and
> > frequency.
> >
> > More details on these pwm devices can be obtained from relevant chapter
> > of reference manual, present at following[1] location.
>
> Please make sure to spell PWM consistently. It should be in all
> uppercase in text. Lowercase should only be used in variable names or
> the patch subject prefix. See other commit messages for examples. The
> same applies to the rest of this patch, which seems to use a random mix
> of both.
>
> And maybe this shouldn't say "Add support for pwm devices" but rather
> "Add support for PWM chips..." and "These PWM chips support..."
I would do that.
> >
> > 1. http://www.st.com/internet/mcu/product/251211.jsp
> >
> > Cc: Thierry Reding
> > Signed-off-by: Shiraz Hashim
> > Signed-off-by: Viresh Kumar
> > Reviewed-by: Vipin Kumar
> > ---
> > .../devicetree/bindings/pwm/st-spear-pwm.txt | 19 ++
> > drivers/pwm/Kconfig| 10 +
> > drivers/pwm/Makefile |1 +
> > drivers/pwm/pwm-spear.c| 287
> >
> > 4 files changed, 317 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> > create mode 100644 drivers/pwm/pwm-spear.c
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> > b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> > new file mode 100644
> > index 000..b3dd1a0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
>
> I think this should either be "spear-pwm.txt" to mirror the driver name,
> or, to follow the Tegra example, "st,spear-pwm.txt" to mirror the
> compatible property.
Okay. I would rename it to 'st,spear-pwm.txt'.
> > @@ -0,0 +1,19 @@
> > +== ST SPEAr SoC PWM controller ==
> > +
> > +Required properties:
> > +- compatible: should be one of:
> > + - "st,spear-pwm"
> > + - "st,spear13xx-pwm"
> > +- reg: physical base address and length of the controller's registers
> > +- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on
> > SPEAr. The
>
> I think you can break the "The" to the next line to make it fit within
> the 80 character limit.
Sure.
> > + first cell specifies the per-chip index of the PWM to use and the second
> > + cell is the duty cycle in nanoseconds.
>
> This should be "period in nanoseconds". I know this is wrong in the
> binding documentation for other drivers but I've recently committed a
> patch that fixes it.
Okay but I couldn't see use of pwm->period set by of_pwm_simple_xlate
anywhere. Am I missing something ?
>
> > +
> > +Example:
> > +
> > +pwm: pwm@a800 {
> > +compatible ="st,spear-pwm";
> > +reg = <0xa800 0x1000>;
> > +#pwm-cells = <2>;
> > +status = "disabled";
> > +};
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index ed81720..3ff3e6f 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -112,6 +112,16 @@ config PWM_SAMSUNG
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-samsung.
> >
> > +config PWM_SPEAR
> > + tristate "STMicroelectronics SPEAR PWM support"
>
> You've spelled this "SPEAr" above and below, why not keep that spelling
> here as well?
>
Thanks for pointing out, would fix it.
> > + depends on PLAT_SPEAR
> > + help
> > + Generic PWM framework driver for the PWM controller on ST
> > + SPEAr SoCs.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-spear.
> > +
> > config PWM_TEGRA
> > tristate "NVIDIA Tegra PWM support"
> > depends on ARCH_TEGRA
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index acfe482..6512786 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
> > obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
> > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
> > obj-$(CONFIG_PWM_TEGRA)+= pwm-tegra.o
> > +obj-$(CONFIG_PWM_SPEAR)+= pwm-spear.o
> > obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
> > obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
> > obj-$(CONFIG_PWM_TWL6030) += pwm-twl6030.o
> > diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
> > new file mode 100644
> > index 000..814395b
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-spear.c
> > @@ -0,0 +1,287 @@
> > +/*
> > + * ST Microelectronics SPEAr Pulse Width Modulator driver
> > + *
> > + * Copyright (C) 2012 ST Microelectronics
> > + * Shiraz Hashim
> > + *
> > + * This file
Re: [PATCH] pwm: add spear pwm driver support
On Thu, Oct 18, 2012 at 04:58:32PM +0530, Shiraz Hashim wrote:
> Add support for pwm devices present on SPEAr platforms. These pwm
> devices support 4 channel output with programmable duty cycle and
> frequency.
>
> More details on these pwm devices can be obtained from relevant chapter
> of reference manual, present at following[1] location.
Please make sure to spell PWM consistently. It should be in all
uppercase in text. Lowercase should only be used in variable names or
the patch subject prefix. See other commit messages for examples. The
same applies to the rest of this patch, which seems to use a random mix
of both.
And maybe this shouldn't say "Add support for pwm devices" but rather
"Add support for PWM chips..." and "These PWM chips support..."
>
> 1. http://www.st.com/internet/mcu/product/251211.jsp
>
> Cc: Thierry Reding
> Signed-off-by: Shiraz Hashim
> Signed-off-by: Viresh Kumar
> Reviewed-by: Vipin Kumar
> ---
> .../devicetree/bindings/pwm/st-spear-pwm.txt | 19 ++
> drivers/pwm/Kconfig| 10 +
> drivers/pwm/Makefile |1 +
> drivers/pwm/pwm-spear.c| 287
>
> 4 files changed, 317 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> create mode 100644 drivers/pwm/pwm-spear.c
>
> diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
> new file mode 100644
> index 000..b3dd1a0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
I think this should either be "spear-pwm.txt" to mirror the driver name,
or, to follow the Tegra example, "st,spear-pwm.txt" to mirror the
compatible property.
> @@ -0,0 +1,19 @@
> +== ST SPEAr SoC PWM controller ==
> +
> +Required properties:
> +- compatible: should be one of:
> + - "st,spear-pwm"
> + - "st,spear13xx-pwm"
> +- reg: physical base address and length of the controller's registers
> +- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on
> SPEAr. The
I think you can break the "The" to the next line to make it fit within
the 80 character limit.
> + first cell specifies the per-chip index of the PWM to use and the second
> + cell is the duty cycle in nanoseconds.
This should be "period in nanoseconds". I know this is wrong in the
binding documentation for other drivers but I've recently committed a
patch that fixes it.
> +
> +Example:
> +
> +pwm: pwm@a800 {
> +compatible ="st,spear-pwm";
> +reg = <0xa800 0x1000>;
> +#pwm-cells = <2>;
> +status = "disabled";
> +};
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index ed81720..3ff3e6f 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -112,6 +112,16 @@ config PWM_SAMSUNG
> To compile this driver as a module, choose M here: the module
> will be called pwm-samsung.
>
> +config PWM_SPEAR
> + tristate "STMicroelectronics SPEAR PWM support"
You've spelled this "SPEAr" above and below, why not keep that spelling
here as well?
> + depends on PLAT_SPEAR
> + help
> + Generic PWM framework driver for the PWM controller on ST
> + SPEAr SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-spear.
> +
> config PWM_TEGRA
> tristate "NVIDIA Tegra PWM support"
> depends on ARCH_TEGRA
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index acfe482..6512786 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3)+= pwm-puv3.o
> obj-$(CONFIG_PWM_PXA)+= pwm-pxa.o
> obj-$(CONFIG_PWM_SAMSUNG)+= pwm-samsung.o
> obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
> +obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
> obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
> obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
> obj-$(CONFIG_PWM_TWL6030)+= pwm-twl6030.o
> diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
> new file mode 100644
> index 000..814395b
> --- /dev/null
> +++ b/drivers/pwm/pwm-spear.c
> @@ -0,0 +1,287 @@
> +/*
> + * ST Microelectronics SPEAr Pulse Width Modulator driver
> + *
> + * Copyright (C) 2012 ST Microelectronics
> + * Shiraz Hashim
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* PWM registers and bits definitions */
> +#define PWMCR0x00/* Control Register */
> +#define PWMDCR 0x04/* Duty Cycle Register */
>
[PATCH] pwm: add spear pwm driver support
Add support for pwm devices present on SPEAr platforms. These pwm
devices support 4 channel output with programmable duty cycle and
frequency.
More details on these pwm devices can be obtained from relevant chapter
of reference manual, present at following[1] location.
1. http://www.st.com/internet/mcu/product/251211.jsp
Cc: Thierry Reding
Signed-off-by: Shiraz Hashim
Signed-off-by: Viresh Kumar
Reviewed-by: Vipin Kumar
---
.../devicetree/bindings/pwm/st-spear-pwm.txt | 19 ++
drivers/pwm/Kconfig| 10 +
drivers/pwm/Makefile |1 +
drivers/pwm/pwm-spear.c| 287
4 files changed, 317 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
create mode 100644 drivers/pwm/pwm-spear.c
diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
new file mode 100644
index 000..b3dd1a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
@@ -0,0 +1,19 @@
+== ST SPEAr SoC PWM controller ==
+
+Required properties:
+- compatible: should be one of:
+ - "st,spear-pwm"
+ - "st,spear13xx-pwm"
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on
SPEAr. The
+ first cell specifies the per-chip index of the PWM to use and the second
+ cell is the duty cycle in nanoseconds.
+
+Example:
+
+pwm: pwm@a800 {
+compatible ="st,spear-pwm";
+reg = <0xa800 0x1000>;
+#pwm-cells = <2>;
+status = "disabled";
+};
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..3ff3e6f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -112,6 +112,16 @@ config PWM_SAMSUNG
To compile this driver as a module, choose M here: the module
will be called pwm-samsung.
+config PWM_SPEAR
+ tristate "STMicroelectronics SPEAR PWM support"
+ depends on PLAT_SPEAR
+ help
+ Generic PWM framework driver for the PWM controller on ST
+ SPEAr SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-spear.
+
config PWM_TEGRA
tristate "NVIDIA Tegra PWM support"
depends on ARCH_TEGRA
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..6512786 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
obj-$(CONFIG_PWM_TEGRA)+= pwm-tegra.o
+obj-$(CONFIG_PWM_SPEAR)+= pwm-spear.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
obj-$(CONFIG_PWM_TWL6030) += pwm-twl6030.o
diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
new file mode 100644
index 000..814395b
--- /dev/null
+++ b/drivers/pwm/pwm-spear.c
@@ -0,0 +1,287 @@
+/*
+ * ST Microelectronics SPEAr Pulse Width Modulator driver
+ *
+ * Copyright (C) 2012 ST Microelectronics
+ * Shiraz Hashim
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* PWM registers and bits definitions */
+#define PWMCR 0x00/* Control Register */
+#define PWMDCR 0x04/* Duty Cycle Register */
+#define PWMPCR 0x08/* Period Register */
+/* Following only available on 13xx SoCs */
+#define PWMMCR 0x3C/* Master Control Register */
+
+#define PWM_ENABLE 0x1
+
+#define MIN_PRESCALE 0x00
+#define MAX_PRESCALE 0x3FFF
+#define PRESCALE_SHIFT 2
+
+#define MIN_DUTY 0x0001
+#define MAX_DUTY 0x
+
+#define MIN_PERIOD 0x0001
+#define MAX_PERIOD 0x
+
+#define NUM_PWM4
+
+/**
+ * struct pwm: struct representing pwm ip
+ *
+ * mmio_base: base address of pwm
+ * clk: pointer to clk structure of pwm ip
+ * chip: linux pwm chip representation
+ * dev: pointer to device structure of pwm
+ */
+struct spear_pwm_chip {
+ void __iomem *mmio_base;
+ struct clk *clk;
+ struct pwm_chip chip;
+ struct device *dev;
+};
+
+static inline struct spear_pwm_chip *to_spear_pwm_chip(struct pwm_chip *chip)
+{
+ return container_of(chip, struct spear_pwm_chip, chip);
+}
+
+static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int
num,
+ unsigned long offset)
+{
+ return readl_relaxed(chip->mmio_base +
[PATCH] pwm: add spear pwm driver support
Add support for pwm devices present on SPEAr platforms. These pwm
devices support 4 channel output with programmable duty cycle and
frequency.
More details on these pwm devices can be obtained from relevant chapter
of reference manual, present at following[1] location.
1. http://www.st.com/internet/mcu/product/251211.jsp
Cc: Thierry Reding thierry.red...@avionic-design.de
Signed-off-by: Shiraz Hashim shiraz.has...@st.com
Signed-off-by: Viresh Kumar viresh.ku...@linaro.org
Reviewed-by: Vipin Kumar vipin.ku...@st.com
---
.../devicetree/bindings/pwm/st-spear-pwm.txt | 19 ++
drivers/pwm/Kconfig| 10 +
drivers/pwm/Makefile |1 +
drivers/pwm/pwm-spear.c| 287
4 files changed, 317 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
create mode 100644 drivers/pwm/pwm-spear.c
diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
new file mode 100644
index 000..b3dd1a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
@@ -0,0 +1,19 @@
+== ST SPEAr SoC PWM controller ==
+
+Required properties:
+- compatible: should be one of:
+ - st,spear-pwm
+ - st,spear13xx-pwm
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on
SPEAr. The
+ first cell specifies the per-chip index of the PWM to use and the second
+ cell is the duty cycle in nanoseconds.
+
+Example:
+
+pwm: pwm@a800 {
+compatible =st,spear-pwm;
+reg = 0xa800 0x1000;
+#pwm-cells = 2;
+status = disabled;
+};
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..3ff3e6f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -112,6 +112,16 @@ config PWM_SAMSUNG
To compile this driver as a module, choose M here: the module
will be called pwm-samsung.
+config PWM_SPEAR
+ tristate STMicroelectronics SPEAR PWM support
+ depends on PLAT_SPEAR
+ help
+ Generic PWM framework driver for the PWM controller on ST
+ SPEAr SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-spear.
+
config PWM_TEGRA
tristate NVIDIA Tegra PWM support
depends on ARCH_TEGRA
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..6512786 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
obj-$(CONFIG_PWM_TEGRA)+= pwm-tegra.o
+obj-$(CONFIG_PWM_SPEAR)+= pwm-spear.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
obj-$(CONFIG_PWM_TWL6030) += pwm-twl6030.o
diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
new file mode 100644
index 000..814395b
--- /dev/null
+++ b/drivers/pwm/pwm-spear.c
@@ -0,0 +1,287 @@
+/*
+ * ST Microelectronics SPEAr Pulse Width Modulator driver
+ *
+ * Copyright (C) 2012 ST Microelectronics
+ * Shiraz Hashim shiraz.has...@st.com
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed as is without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include linux/clk.h
+#include linux/err.h
+#include linux/io.h
+#include linux/ioport.h
+#include linux/kernel.h
+#include linux/math64.h
+#include linux/module.h
+#include linux/of.h
+#include linux/platform_device.h
+#include linux/pwm.h
+#include linux/slab.h
+
+/* PWM registers and bits definitions */
+#define PWMCR 0x00/* Control Register */
+#define PWMDCR 0x04/* Duty Cycle Register */
+#define PWMPCR 0x08/* Period Register */
+/* Following only available on 13xx SoCs */
+#define PWMMCR 0x3C/* Master Control Register */
+
+#define PWM_ENABLE 0x1
+
+#define MIN_PRESCALE 0x00
+#define MAX_PRESCALE 0x3FFF
+#define PRESCALE_SHIFT 2
+
+#define MIN_DUTY 0x0001
+#define MAX_DUTY 0x
+
+#define MIN_PERIOD 0x0001
+#define MAX_PERIOD 0x
+
+#define NUM_PWM4
+
+/**
+ * struct pwm: struct representing pwm ip
+ *
+ * mmio_base: base address of pwm
+ * clk: pointer to clk structure of pwm ip
+ * chip: linux pwm chip representation
+ * dev: pointer to device structure of pwm
+ */
+struct spear_pwm_chip {
+ void __iomem *mmio_base;
+ struct clk *clk;
+ struct pwm_chip chip;
+ struct device *dev;
+};
+
+static inline struct spear_pwm_chip *to_spear_pwm_chip(struct pwm_chip *chip)
+{
Re: [PATCH] pwm: add spear pwm driver support
On Thu, Oct 18, 2012 at 04:58:32PM +0530, Shiraz Hashim wrote:
Add support for pwm devices present on SPEAr platforms. These pwm
devices support 4 channel output with programmable duty cycle and
frequency.
More details on these pwm devices can be obtained from relevant chapter
of reference manual, present at following[1] location.
Please make sure to spell PWM consistently. It should be in all
uppercase in text. Lowercase should only be used in variable names or
the patch subject prefix. See other commit messages for examples. The
same applies to the rest of this patch, which seems to use a random mix
of both.
And maybe this shouldn't say Add support for pwm devices but rather
Add support for PWM chips... and These PWM chips support...
1. http://www.st.com/internet/mcu/product/251211.jsp
Cc: Thierry Reding thierry.red...@avionic-design.de
Signed-off-by: Shiraz Hashim shiraz.has...@st.com
Signed-off-by: Viresh Kumar viresh.ku...@linaro.org
Reviewed-by: Vipin Kumar vipin.ku...@st.com
---
.../devicetree/bindings/pwm/st-spear-pwm.txt | 19 ++
drivers/pwm/Kconfig| 10 +
drivers/pwm/Makefile |1 +
drivers/pwm/pwm-spear.c| 287
4 files changed, 317 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
create mode 100644 drivers/pwm/pwm-spear.c
diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
new file mode 100644
index 000..b3dd1a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
I think this should either be spear-pwm.txt to mirror the driver name,
or, to follow the Tegra example, st,spear-pwm.txt to mirror the
compatible property.
@@ -0,0 +1,19 @@
+== ST SPEAr SoC PWM controller ==
+
+Required properties:
+- compatible: should be one of:
+ - st,spear-pwm
+ - st,spear13xx-pwm
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on
SPEAr. The
I think you can break the The to the next line to make it fit within
the 80 character limit.
+ first cell specifies the per-chip index of the PWM to use and the second
+ cell is the duty cycle in nanoseconds.
This should be period in nanoseconds. I know this is wrong in the
binding documentation for other drivers but I've recently committed a
patch that fixes it.
+
+Example:
+
+pwm: pwm@a800 {
+compatible =st,spear-pwm;
+reg = 0xa800 0x1000;
+#pwm-cells = 2;
+status = disabled;
+};
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..3ff3e6f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -112,6 +112,16 @@ config PWM_SAMSUNG
To compile this driver as a module, choose M here: the module
will be called pwm-samsung.
+config PWM_SPEAR
+ tristate STMicroelectronics SPEAR PWM support
You've spelled this SPEAr above and below, why not keep that spelling
here as well?
+ depends on PLAT_SPEAR
+ help
+ Generic PWM framework driver for the PWM controller on ST
+ SPEAr SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-spear.
+
config PWM_TEGRA
tristate NVIDIA Tegra PWM support
depends on ARCH_TEGRA
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..6512786 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3)+= pwm-puv3.o
obj-$(CONFIG_PWM_PXA)+= pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG)+= pwm-samsung.o
obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
+obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
obj-$(CONFIG_PWM_TWL6030)+= pwm-twl6030.o
diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
new file mode 100644
index 000..814395b
--- /dev/null
+++ b/drivers/pwm/pwm-spear.c
@@ -0,0 +1,287 @@
+/*
+ * ST Microelectronics SPEAr Pulse Width Modulator driver
+ *
+ * Copyright (C) 2012 ST Microelectronics
+ * Shiraz Hashim shiraz.has...@st.com
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed as is without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include linux/clk.h
+#include linux/err.h
+#include linux/io.h
+#include linux/ioport.h
+#include linux/kernel.h
+#include linux/math64.h
+#include linux/module.h
+#include linux/of.h
+#include linux/platform_device.h
+#include linux/pwm.h
+#include linux/slab.h
+
+/* PWM registers and bits definitions */
+#define PWMCR
Re: [PATCH] pwm: add spear pwm driver support
Hi Thierry,
Thanks for the quick review.
On Thu, Oct 18, 2012 at 02:08:20PM +0200, Thierry Reding wrote:
On Thu, Oct 18, 2012 at 04:58:32PM +0530, Shiraz Hashim wrote:
Add support for pwm devices present on SPEAr platforms. These pwm
devices support 4 channel output with programmable duty cycle and
frequency.
More details on these pwm devices can be obtained from relevant chapter
of reference manual, present at following[1] location.
Please make sure to spell PWM consistently. It should be in all
uppercase in text. Lowercase should only be used in variable names or
the patch subject prefix. See other commit messages for examples. The
same applies to the rest of this patch, which seems to use a random mix
of both.
And maybe this shouldn't say Add support for pwm devices but rather
Add support for PWM chips... and These PWM chips support...
I would do that.
1. http://www.st.com/internet/mcu/product/251211.jsp
Cc: Thierry Reding thierry.red...@avionic-design.de
Signed-off-by: Shiraz Hashim shiraz.has...@st.com
Signed-off-by: Viresh Kumar viresh.ku...@linaro.org
Reviewed-by: Vipin Kumar vipin.ku...@st.com
---
.../devicetree/bindings/pwm/st-spear-pwm.txt | 19 ++
drivers/pwm/Kconfig| 10 +
drivers/pwm/Makefile |1 +
drivers/pwm/pwm-spear.c| 287
4 files changed, 317 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
create mode 100644 drivers/pwm/pwm-spear.c
diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
new file mode 100644
index 000..b3dd1a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
I think this should either be spear-pwm.txt to mirror the driver name,
or, to follow the Tegra example, st,spear-pwm.txt to mirror the
compatible property.
Okay. I would rename it to 'st,spear-pwm.txt'.
@@ -0,0 +1,19 @@
+== ST SPEAr SoC PWM controller ==
+
+Required properties:
+- compatible: should be one of:
+ - st,spear-pwm
+ - st,spear13xx-pwm
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on
SPEAr. The
I think you can break the The to the next line to make it fit within
the 80 character limit.
Sure.
+ first cell specifies the per-chip index of the PWM to use and the second
+ cell is the duty cycle in nanoseconds.
This should be period in nanoseconds. I know this is wrong in the
binding documentation for other drivers but I've recently committed a
patch that fixes it.
Okay but I couldn't see use of pwm-period set by of_pwm_simple_xlate
anywhere. Am I missing something ?
+
+Example:
+
+pwm: pwm@a800 {
+compatible =st,spear-pwm;
+reg = 0xa800 0x1000;
+#pwm-cells = 2;
+status = disabled;
+};
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..3ff3e6f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -112,6 +112,16 @@ config PWM_SAMSUNG
To compile this driver as a module, choose M here: the module
will be called pwm-samsung.
+config PWM_SPEAR
+ tristate STMicroelectronics SPEAR PWM support
You've spelled this SPEAr above and below, why not keep that spelling
here as well?
Thanks for pointing out, would fix it.
+ depends on PLAT_SPEAR
+ help
+ Generic PWM framework driver for the PWM controller on ST
+ SPEAr SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-spear.
+
config PWM_TEGRA
tristate NVIDIA Tegra PWM support
depends on ARCH_TEGRA
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..6512786 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
obj-$(CONFIG_PWM_TEGRA)+= pwm-tegra.o
+obj-$(CONFIG_PWM_SPEAR)+= pwm-spear.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
obj-$(CONFIG_PWM_TWL6030) += pwm-twl6030.o
diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
new file mode 100644
index 000..814395b
--- /dev/null
+++ b/drivers/pwm/pwm-spear.c
@@ -0,0 +1,287 @@
+/*
+ * ST Microelectronics SPEAr Pulse Width Modulator driver
+ *
+ * Copyright (C) 2012 ST Microelectronics
+ * Shiraz Hashim shiraz.has...@st.com
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed as is without any
+ * warranty
Re: [PATCH] pwm: add spear pwm driver support
On Thu, Oct 18, 2012 at 06:59:28PM +0530, Shiraz Hashim wrote: Hi Thierry, Thanks for the quick review. On Thu, Oct 18, 2012 at 02:08:20PM +0200, Thierry Reding wrote: On Thu, Oct 18, 2012 at 04:58:32PM +0530, Shiraz Hashim wrote: [...] + first cell specifies the per-chip index of the PWM to use and the second + cell is the duty cycle in nanoseconds. This should be period in nanoseconds. I know this is wrong in the binding documentation for other drivers but I've recently committed a patch that fixes it. Okay but I couldn't see use of pwm-period set by of_pwm_simple_xlate anywhere. Am I missing something ? It's set by the call to pwm_set_period(). +/* PWM registers and bits definitions */ +#define PWMCR0x00/* Control Register */ +#define PWMDCR 0x04/* Duty Cycle Register */ +#define PWMPCR 0x08/* Period Register */ +/* Following only available on 13xx SoCs */ +#define PWMMCR 0x3C/* Master Control Register */ + +#define PWM_ENABLE 0x1 + +#define MIN_PRESCALE 0x00 +#define MAX_PRESCALE 0x3FFF +#define PRESCALE_SHIFT 2 + +#define MIN_DUTY 0x0001 +#define MAX_DUTY 0x + +#define MIN_PERIOD 0x0001 +#define MAX_PERIOD 0x Would it make sense to perhaps group the bitfields with the matching register definitions to make their use more obvious. Also I prefer lowercase hexadecimal digits, but that's pure bikeshedding. Sure I would group them, but uppercase hexadecimal digits clearly seperate the value (number) which otherwise can be mixed (while reading) with normal letters. Isn't it ? As I said, this is really very subjective, so if you prefer uppercase, feel free to keep it. =) +static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int num, + unsigned long offset) I personally like it better to have function arguments aligned, like so: static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int num, unsigned long offset) Note, those are as many 8-spaces tabs with only spaces to align them properly. But again, pure bikeshedding and I won't force the issue. Would do that. Are you aware of some (vim) configuration which can autmatically do this while editing code ? I'm not aware of any such setting, but the idea is interesting. I usually do that automatically out of habit, but having the editor do it would be nice as well. __devinit will go away sometime soon, so please don't use it in new code. Okay. You mean all init sections would eventually be removed. I would read more about it. Yes, commit 45f035a (only in linux-next I think) has some details. +MODULE_LICENSE(GPL); +MODULE_AUTHOR(Shiraz Hashim shiraz.has...@st.com); +MODULE_AUTHOR(Viresh Kumar viresh.ku...@linaro.com); I don't think this works: the second entry will replace the first. Have you verified that both authors appear in the output of modinfo? This is the output of modinfo (compiled for linux-3.5) $ modinfo pwm-spear.ko filename: drivers/pwm/pwm-spear.ko alias: platform:st-pwm author: Viresh Kumar viresh.ku...@linaro.com author: Shiraz Hashim shiraz.has...@st.com license:GPL alias: of:N*T*Cst,spear13xx-pwm* alias: of:N*T*Cst,spear-pwm* depends: intree: Y vermagic: 3.5.0-test-00138-g08e3584 SMP mod_unload modversions ARMv7 p2v8 Interesting. I thought I'd seen this fail only recently. Will need to investigate. +MODULE_ALIAS(platform:st-pwm); Should this not rather be platform:spear-pwm? Yes, I would double check these kind of small issues before sending patches next time. No biggie. That's why we have reviews. Thierry pgp6NF7dQPYDX.pgp Description: PGP signature
Re: [PATCH] pwm: add spear pwm driver support
On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim shiraz.has...@st.com wrote:
diff --git a/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
b/Documentation/devicetree/bindings/pwm/st-spear-pwm.txt
+== ST SPEAr SoC PWM controller ==
+
+Required properties:
+- compatible: should be one of:
+ - st,spear-pwm
+ - st,spear13xx-pwm
This has be matching with an version of the IP, as discussed earlier for many
driver.
Because ST hasn't released any specific version numbers, you must mention
name of the SoC where the IP first appeared. That will mark its version. So,
in short don't use generic names like spear or spear13xx :)
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on
SPEAr. The
+ first cell specifies the per-chip index of the PWM to use and the second
+ cell is the duty cycle in nanoseconds.
+
+Example:
+
+pwm: pwm@a800 {
+compatible =st,spear-pwm;
+reg = 0xa800 0x1000;
+#pwm-cells = 2;
+status = disabled;
+};
An example on how other nodes will link to it by passing id and duty cycle
would be better.
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..3ff3e6f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -112,6 +112,16 @@ config PWM_SAMSUNG
To compile this driver as a module, choose M here: the module
will be called pwm-samsung.
+config PWM_SPEAR
+ tristate STMicroelectronics SPEAR PWM support
SPEAr
+ depends on PLAT_SPEAR
+ help
+ Generic PWM framework driver for the PWM controller on ST
+ SPEAr SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-spear.
+
config PWM_TEGRA
tristate NVIDIA Tegra PWM support
depends on ARCH_TEGRA
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..6512786 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
obj-$(CONFIG_PWM_TEGRA)+= pwm-tegra.o
+obj-$(CONFIG_PWM_SPEAR)+= pwm-spear.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
obj-$(CONFIG_PWM_TWL6030) += pwm-twl6030.o
diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
new file mode 100644
index 000..814395b
--- /dev/null
+++ b/drivers/pwm/pwm-spear.c
@@ -0,0 +1,287 @@
+/*
+ * ST Microelectronics SPEAr Pulse Width Modulator driver
+ *
+ * Copyright (C) 2012 ST Microelectronics
+ * Shiraz Hashim shiraz.has...@st.com
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed as is without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include linux/clk.h
+#include linux/err.h
+#include linux/io.h
+#include linux/ioport.h
+#include linux/kernel.h
+#include linux/math64.h
+#include linux/module.h
+#include linux/of.h
+#include linux/platform_device.h
+#include linux/pwm.h
+#include linux/slab.h
+
+/* PWM registers and bits definitions */
+#define PWMCR 0x00/* Control Register */
+#define PWMDCR 0x04/* Duty Cycle Register */
+#define PWMPCR 0x08/* Period Register */
+/* Following only available on 13xx SoCs */
+#define PWMMCR 0x3C/* Master Control Register */
+
+#define PWM_ENABLE 0x1
+
+#define MIN_PRESCALE 0x00
+#define MAX_PRESCALE 0x3FFF
+#define PRESCALE_SHIFT 2
+
+#define MIN_DUTY 0x0001
+#define MAX_DUTY 0x
+
+#define MIN_PERIOD 0x0001
+#define MAX_PERIOD 0x
+
+#define NUM_PWM4
+
+/**
+ * struct pwm: struct representing pwm ip
spear_pwm_chip: struct representing pwm chip
+ * mmio_base: base address of pwm
base would be enough.
+ * clk: pointer to clk structure of pwm ip
+ * chip: linux pwm chip representation
+ * dev: pointer to device structure of pwm
+ */
+struct spear_pwm_chip {
+ void __iomem *mmio_base;
+ struct clk *clk;
+ struct pwm_chip chip;
+ struct device *dev;
+};
+
+static inline struct spear_pwm_chip *to_spear_pwm_chip(struct pwm_chip *chip)
+{
+ return container_of(chip, struct spear_pwm_chip, chip);
+}
+
+static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int
num,
include types.h for u32
+ unsigned long offset)
+{
+ return readl_relaxed(chip-mmio_base + (num 4) + offset);
+}
+
+static inline void spear_pwm_writel(struct spear_pwm_chip *chip,
+ unsigned int num, unsigned long
Re: [PATCH] pwm: add spear pwm driver support
On Thu, Oct 18, 2012 at 6:59 PM, Shiraz Hashim shiraz.has...@st.com wrote: Is there a reason to make this conditional? It looks like SPEAr has moved to OF, so this will always be enabled anyway, won't it? Yes, I would remove it, SPEAr cannot boot without DT. Add a dependency on OF in the Kconfig then. Also i haven't seen any suspend/resume routines here. Missed them? -- 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/
