Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
On 10/10/18 6:11 AM, Christoph Hellwig wrote:
Thanks for getting these drivers submitted upstream!
I don't really know anything about PWM, so just some random nitpicking
below..
+ iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
* already has a higher precedence than +, so no need for the inner
braces.
+ duty = ioread32(pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
Same here.
Will remove the braces in both places.
+ /* (1 << (16+scale)) * 10^9/rate = real_period */
unsigned long scalePow = (pwm->approx_period * (u64)rate) / 10;
no camcel case, please.
My bad. I will fix that.
+ int scale = ilog2(scalePow) - 16;
+
+ scale = clamp(scale, 0, 0xf);
Why not:
int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
Sure.
+static int sifive_pwm_clock_notifier(struct notifier_block *nb,
+unsigned long event, void *data)
+{
+ struct clk_notifier_data *ndata = data;
+ struct sifive_pwm_device *pwm = container_of(nb,
+struct sifive_pwm_device,
+notifier);
I don't think there are any guidlines, but I always prefer to just move
the whole container_of onto a new line:
struct sifive_pwm_device *pwm =
container_of(nb, struct sifive_pwm_device, notifier);
Done.
Regards,
Atish
+static struct platform_driver sifive_pwm_driver = {
+ .probe = sifive_pwm_probe,
+ .remove = sifive_pwm_remove,
+ .driver = {
+ .name = "pwm-sifivem",
+ .of_match_table = of_match_ptr(sifive_pwm_of_match),
+ },
+};
What about using tabs to align this a little more nicely?
static struct platform_driver sifive_pwm_driver = {
.probe = sifive_pwm_probe,
.remove = sifive_pwm_remove,
.driver = {
.name = "pwm-sifivem",
.of_match_table = of_match_ptr(sifive_pwm_of_match),
},
};
Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
On 10/10/18 6:11 AM, Christoph Hellwig wrote:
Thanks for getting these drivers submitted upstream!
I don't really know anything about PWM, so just some random nitpicking
below..
+ iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
* already has a higher precedence than +, so no need for the inner
braces.
+ duty = ioread32(pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
Same here.
Will remove the braces in both places.
+ /* (1 << (16+scale)) * 10^9/rate = real_period */
unsigned long scalePow = (pwm->approx_period * (u64)rate) / 10;
no camcel case, please.
My bad. I will fix that.
+ int scale = ilog2(scalePow) - 16;
+
+ scale = clamp(scale, 0, 0xf);
Why not:
int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
Sure.
+static int sifive_pwm_clock_notifier(struct notifier_block *nb,
+unsigned long event, void *data)
+{
+ struct clk_notifier_data *ndata = data;
+ struct sifive_pwm_device *pwm = container_of(nb,
+struct sifive_pwm_device,
+notifier);
I don't think there are any guidlines, but I always prefer to just move
the whole container_of onto a new line:
struct sifive_pwm_device *pwm =
container_of(nb, struct sifive_pwm_device, notifier);
Done.
Regards,
Atish
+static struct platform_driver sifive_pwm_driver = {
+ .probe = sifive_pwm_probe,
+ .remove = sifive_pwm_remove,
+ .driver = {
+ .name = "pwm-sifivem",
+ .of_match_table = of_match_ptr(sifive_pwm_of_match),
+ },
+};
What about using tabs to align this a little more nicely?
static struct platform_driver sifive_pwm_driver = {
.probe = sifive_pwm_probe,
.remove = sifive_pwm_remove,
.driver = {
.name = "pwm-sifivem",
.of_match_table = of_match_ptr(sifive_pwm_of_match),
},
};
Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
On 10/10/18 7:13 AM, Thierry Reding wrote:
On Tue, Oct 09, 2018 at 11:51:23AM -0700, Atish Patra wrote:
From: "Wesley W. Terpstra"
Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
Signed-off-by: Wesley W. Terpstra
[Atish: Various fixes and code cleanup]
Signed-off-by: Atish Patra
---
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-sifive.c | 240 +++
3 files changed, 251 insertions(+)
create mode 100644 drivers/pwm/pwm-sifive.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 504d2527..dd12144d 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -378,6 +378,16 @@ config PWM_SAMSUNG
To compile this driver as a module, choose M here: the module
will be called pwm-samsung.
+config PWM_SIFIVE
+ tristate "SiFive PWM support"
+ depends on OF
+ depends on COMMON_CLK
+ help
+ Generic PWM framework driver for SiFive SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-sifive.
+
config PWM_SPEAR
tristate "STMicroelectronics SPEAr PWM support"
depends on PLAT_SPEAR
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9c676a0d..30089ca6 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR)+= pwm-rcar.o
obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
obj-$(CONFIG_PWM_ROCKCHIP)+= pwm-rockchip.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
+obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
obj-$(CONFIG_PWM_STI) += pwm-sti.o
obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
new file mode 100644
index ..99580025
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,240 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 SiFive
+ */
+#include
What do you need this for? Your driver should only be dealing with enum
pwm_polarity, not the defines from the above header. This works but only
because PWM_POLARITY_INVERTED and PWM_POLARITY_INVERSED happen to be the
same value.
+#include
+#include
+#include
+#include
+#include
+#include
Keep these in alphabetical order, please.
+
+#define MAX_PWM4
+
+/* Register offsets */
+#define REG_PWMCFG 0x0
+#define REG_PWMCOUNT 0x8
+#define REG_PWMS 0x10
+#defineREG_PWMCMP0 0x20
+
+/* PWMCFG fields */
+#define BIT_PWM_SCALE 0
+#define BIT_PWM_STICKY 8
+#define BIT_PWM_ZERO_ZMP 9
+#define BIT_PWM_DEGLITCH 10
+#define BIT_PWM_EN_ALWAYS 12
+#define BIT_PWM_EN_ONCE13
+#define BIT_PWM0_CENTER16
+#define BIT_PWM0_GANG 24
+#define BIT_PWM0_IP28
+
+#define SIZE_PWMCMP4
+#define MASK_PWM_SCALE 0xf
+
+struct sifive_pwm_device {
+ struct pwm_chip chip;
+ struct notifier_block notifier;
+ struct clk *clk;
+ void __iomem*regs;
+ unsigned intapprox_period;
+ unsigned intreal_period;
+};
No need to align these. A single space is enough to separate variable
type and name.
+
+static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c)
+{
+ return container_of(c, struct sifive_pwm_device, chip);
+}
+
+static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+ unsigned int duty_cycle;
+ u32 frac;
+
+ duty_cycle = state->duty_cycle;
+ if (!state->enabled)
+ duty_cycle = 0;
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ duty_cycle = state->period - duty_cycle;
That's not actually polarity inversion. This is "lightweight" inversion
which should be up to the consumer, not the PWM driver, to implement. If
you don't actually have a knob in hardware to switch the polarity, don't
support it.
I couldn't find anything about polarity support in the spec. Of course,
I might be complete idiot as well :). I will wait for Wesly's confirmation.
+
+ frac = ((u64)duty_cycle << 16) / state->period;
+ frac = min(frac, 0xU);
+
+ iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
writel()?
+
+ if (state->enabled) {
+ state->period = pwm->real_period;
+ state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ state->duty_cycle = state->period - state->duty_cycle;
+ }
+
+ return 0;
+}
+
+static void
Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
On 10/10/18 7:13 AM, Thierry Reding wrote:
On Tue, Oct 09, 2018 at 11:51:23AM -0700, Atish Patra wrote:
From: "Wesley W. Terpstra"
Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
Signed-off-by: Wesley W. Terpstra
[Atish: Various fixes and code cleanup]
Signed-off-by: Atish Patra
---
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-sifive.c | 240 +++
3 files changed, 251 insertions(+)
create mode 100644 drivers/pwm/pwm-sifive.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 504d2527..dd12144d 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -378,6 +378,16 @@ config PWM_SAMSUNG
To compile this driver as a module, choose M here: the module
will be called pwm-samsung.
+config PWM_SIFIVE
+ tristate "SiFive PWM support"
+ depends on OF
+ depends on COMMON_CLK
+ help
+ Generic PWM framework driver for SiFive SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-sifive.
+
config PWM_SPEAR
tristate "STMicroelectronics SPEAr PWM support"
depends on PLAT_SPEAR
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9c676a0d..30089ca6 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR)+= pwm-rcar.o
obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
obj-$(CONFIG_PWM_ROCKCHIP)+= pwm-rockchip.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
+obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
obj-$(CONFIG_PWM_STI) += pwm-sti.o
obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
new file mode 100644
index ..99580025
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,240 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 SiFive
+ */
+#include
What do you need this for? Your driver should only be dealing with enum
pwm_polarity, not the defines from the above header. This works but only
because PWM_POLARITY_INVERTED and PWM_POLARITY_INVERSED happen to be the
same value.
+#include
+#include
+#include
+#include
+#include
+#include
Keep these in alphabetical order, please.
+
+#define MAX_PWM4
+
+/* Register offsets */
+#define REG_PWMCFG 0x0
+#define REG_PWMCOUNT 0x8
+#define REG_PWMS 0x10
+#defineREG_PWMCMP0 0x20
+
+/* PWMCFG fields */
+#define BIT_PWM_SCALE 0
+#define BIT_PWM_STICKY 8
+#define BIT_PWM_ZERO_ZMP 9
+#define BIT_PWM_DEGLITCH 10
+#define BIT_PWM_EN_ALWAYS 12
+#define BIT_PWM_EN_ONCE13
+#define BIT_PWM0_CENTER16
+#define BIT_PWM0_GANG 24
+#define BIT_PWM0_IP28
+
+#define SIZE_PWMCMP4
+#define MASK_PWM_SCALE 0xf
+
+struct sifive_pwm_device {
+ struct pwm_chip chip;
+ struct notifier_block notifier;
+ struct clk *clk;
+ void __iomem*regs;
+ unsigned intapprox_period;
+ unsigned intreal_period;
+};
No need to align these. A single space is enough to separate variable
type and name.
+
+static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c)
+{
+ return container_of(c, struct sifive_pwm_device, chip);
+}
+
+static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+ unsigned int duty_cycle;
+ u32 frac;
+
+ duty_cycle = state->duty_cycle;
+ if (!state->enabled)
+ duty_cycle = 0;
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ duty_cycle = state->period - duty_cycle;
That's not actually polarity inversion. This is "lightweight" inversion
which should be up to the consumer, not the PWM driver, to implement. If
you don't actually have a knob in hardware to switch the polarity, don't
support it.
I couldn't find anything about polarity support in the spec. Of course,
I might be complete idiot as well :). I will wait for Wesly's confirmation.
+
+ frac = ((u64)duty_cycle << 16) / state->period;
+ frac = min(frac, 0xU);
+
+ iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
writel()?
+
+ if (state->enabled) {
+ state->period = pwm->real_period;
+ state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ state->duty_cycle = state->period - state->duty_cycle;
+ }
+
+ return 0;
+}
+
+static void
Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
On Tue, Oct 09, 2018 at 11:51:23AM -0700, Atish Patra wrote:
> From: "Wesley W. Terpstra"
>
> Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
>
> Signed-off-by: Wesley W. Terpstra
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sifive.c | 240
> +++
> 3 files changed, 251 insertions(+)
> create mode 100644 drivers/pwm/pwm-sifive.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 504d2527..dd12144d 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -378,6 +378,16 @@ config PWM_SAMSUNG
> To compile this driver as a module, choose M here: the module
> will be called pwm-samsung.
>
> +config PWM_SIFIVE
> + tristate "SiFive PWM support"
> + depends on OF
> + depends on COMMON_CLK
> + help
> + Generic PWM framework driver for SiFive SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-sifive.
> +
> config PWM_SPEAR
> tristate "STMicroelectronics SPEAr PWM support"
> depends on PLAT_SPEAR
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9c676a0d..30089ca6 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
> obj-$(CONFIG_PWM_RENESAS_TPU)+= pwm-renesas-tpu.o
> obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> obj-$(CONFIG_PWM_SAMSUNG)+= pwm-samsung.o
> +obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
> obj-$(CONFIG_PWM_STI)+= pwm-sti.o
> obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> new file mode 100644
> index ..99580025
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,240 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 SiFive
> + */
> +#include
What do you need this for? Your driver should only be dealing with enum
pwm_polarity, not the defines from the above header. This works but only
because PWM_POLARITY_INVERTED and PWM_POLARITY_INVERSED happen to be the
same value.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Keep these in alphabetical order, please.
> +
> +#define MAX_PWM 4
> +
> +/* Register offsets */
> +#define REG_PWMCFG 0x0
> +#define REG_PWMCOUNT 0x8
> +#define REG_PWMS 0x10
> +#define REG_PWMCMP0 0x20
> +
> +/* PWMCFG fields */
> +#define BIT_PWM_SCALE0
> +#define BIT_PWM_STICKY 8
> +#define BIT_PWM_ZERO_ZMP 9
> +#define BIT_PWM_DEGLITCH 10
> +#define BIT_PWM_EN_ALWAYS12
> +#define BIT_PWM_EN_ONCE 13
> +#define BIT_PWM0_CENTER 16
> +#define BIT_PWM0_GANG24
> +#define BIT_PWM0_IP 28
> +
> +#define SIZE_PWMCMP 4
> +#define MASK_PWM_SCALE 0xf
> +
> +struct sifive_pwm_device {
> + struct pwm_chip chip;
> + struct notifier_block notifier;
> + struct clk *clk;
> + void __iomem*regs;
> + unsigned intapprox_period;
> + unsigned intreal_period;
> +};
No need to align these. A single space is enough to separate variable
type and name.
> +
> +static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip
> *c)
> +{
> + return container_of(c, struct sifive_pwm_device, chip);
> +}
> +
> +static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
> + struct pwm_state *state)
> +{
> + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> + unsigned int duty_cycle;
> + u32 frac;
> +
> + duty_cycle = state->duty_cycle;
> + if (!state->enabled)
> + duty_cycle = 0;
> + if (state->polarity == PWM_POLARITY_NORMAL)
> + duty_cycle = state->period - duty_cycle;
That's not actually polarity inversion. This is "lightweight" inversion
which should be up to the consumer, not the PWM driver, to implement. If
you don't actually have a knob in hardware to switch the polarity, don't
support it.
> +
> + frac = ((u64)duty_cycle << 16) / state->period;
> + frac = min(frac, 0xU);
> +
> + iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
writel()?
> +
> + if (state->enabled) {
> + state->period = pwm->real_period;
> + state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
> + if (state->polarity == PWM_POLARITY_NORMAL)
> + state->duty_cycle = state->period - state->duty_cycle;
> + }
> +
> + return 0;
> +}
> +
> +static void sifive_pwm_get_state(struct pwm_chip *chip, struct
Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
On Tue, Oct 09, 2018 at 11:51:23AM -0700, Atish Patra wrote:
> From: "Wesley W. Terpstra"
>
> Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
>
> Signed-off-by: Wesley W. Terpstra
> [Atish: Various fixes and code cleanup]
> Signed-off-by: Atish Patra
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sifive.c | 240
> +++
> 3 files changed, 251 insertions(+)
> create mode 100644 drivers/pwm/pwm-sifive.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 504d2527..dd12144d 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -378,6 +378,16 @@ config PWM_SAMSUNG
> To compile this driver as a module, choose M here: the module
> will be called pwm-samsung.
>
> +config PWM_SIFIVE
> + tristate "SiFive PWM support"
> + depends on OF
> + depends on COMMON_CLK
> + help
> + Generic PWM framework driver for SiFive SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-sifive.
> +
> config PWM_SPEAR
> tristate "STMicroelectronics SPEAr PWM support"
> depends on PLAT_SPEAR
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9c676a0d..30089ca6 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
> obj-$(CONFIG_PWM_RENESAS_TPU)+= pwm-renesas-tpu.o
> obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> obj-$(CONFIG_PWM_SAMSUNG)+= pwm-samsung.o
> +obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
> obj-$(CONFIG_PWM_STI)+= pwm-sti.o
> obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> new file mode 100644
> index ..99580025
> --- /dev/null
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -0,0 +1,240 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 SiFive
> + */
> +#include
What do you need this for? Your driver should only be dealing with enum
pwm_polarity, not the defines from the above header. This works but only
because PWM_POLARITY_INVERTED and PWM_POLARITY_INVERSED happen to be the
same value.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Keep these in alphabetical order, please.
> +
> +#define MAX_PWM 4
> +
> +/* Register offsets */
> +#define REG_PWMCFG 0x0
> +#define REG_PWMCOUNT 0x8
> +#define REG_PWMS 0x10
> +#define REG_PWMCMP0 0x20
> +
> +/* PWMCFG fields */
> +#define BIT_PWM_SCALE0
> +#define BIT_PWM_STICKY 8
> +#define BIT_PWM_ZERO_ZMP 9
> +#define BIT_PWM_DEGLITCH 10
> +#define BIT_PWM_EN_ALWAYS12
> +#define BIT_PWM_EN_ONCE 13
> +#define BIT_PWM0_CENTER 16
> +#define BIT_PWM0_GANG24
> +#define BIT_PWM0_IP 28
> +
> +#define SIZE_PWMCMP 4
> +#define MASK_PWM_SCALE 0xf
> +
> +struct sifive_pwm_device {
> + struct pwm_chip chip;
> + struct notifier_block notifier;
> + struct clk *clk;
> + void __iomem*regs;
> + unsigned intapprox_period;
> + unsigned intreal_period;
> +};
No need to align these. A single space is enough to separate variable
type and name.
> +
> +static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip
> *c)
> +{
> + return container_of(c, struct sifive_pwm_device, chip);
> +}
> +
> +static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
> + struct pwm_state *state)
> +{
> + struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
> + unsigned int duty_cycle;
> + u32 frac;
> +
> + duty_cycle = state->duty_cycle;
> + if (!state->enabled)
> + duty_cycle = 0;
> + if (state->polarity == PWM_POLARITY_NORMAL)
> + duty_cycle = state->period - duty_cycle;
That's not actually polarity inversion. This is "lightweight" inversion
which should be up to the consumer, not the PWM driver, to implement. If
you don't actually have a knob in hardware to switch the polarity, don't
support it.
> +
> + frac = ((u64)duty_cycle << 16) / state->period;
> + frac = min(frac, 0xU);
> +
> + iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
writel()?
> +
> + if (state->enabled) {
> + state->period = pwm->real_period;
> + state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
> + if (state->polarity == PWM_POLARITY_NORMAL)
> + state->duty_cycle = state->period - state->duty_cycle;
> + }
> +
> + return 0;
> +}
> +
> +static void sifive_pwm_get_state(struct pwm_chip *chip, struct
Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
On Wed, Oct 10, 2018 at 06:11:29AM -0700, Christoph Hellwig wrote:
[...]
> > +static struct platform_driver sifive_pwm_driver = {
> > + .probe = sifive_pwm_probe,
> > + .remove = sifive_pwm_remove,
> > + .driver = {
> > + .name = "pwm-sifivem",
> > + .of_match_table = of_match_ptr(sifive_pwm_of_match),
> > + },
> > +};
>
> What about using tabs to align this a little more nicely?
>
> static struct platform_driver sifive_pwm_driver = {
> .probe = sifive_pwm_probe,
> .remove = sifive_pwm_remove,
> .driver = {
> .name = "pwm-sifivem",
> .of_match_table = of_match_ptr(sifive_pwm_of_match),
> },
> };
I discourage people from doing that because down the road somebody might
add a field here that's longer than the alignment tabs and then either
it becomes ugly or they either have to realign everything to keep it
pretty. Single spaces around '=' don't have that problem if used
consistently.
Thierry
signature.asc
Description: PGP signature
Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
On Wed, Oct 10, 2018 at 06:11:29AM -0700, Christoph Hellwig wrote:
[...]
> > +static struct platform_driver sifive_pwm_driver = {
> > + .probe = sifive_pwm_probe,
> > + .remove = sifive_pwm_remove,
> > + .driver = {
> > + .name = "pwm-sifivem",
> > + .of_match_table = of_match_ptr(sifive_pwm_of_match),
> > + },
> > +};
>
> What about using tabs to align this a little more nicely?
>
> static struct platform_driver sifive_pwm_driver = {
> .probe = sifive_pwm_probe,
> .remove = sifive_pwm_remove,
> .driver = {
> .name = "pwm-sifivem",
> .of_match_table = of_match_ptr(sifive_pwm_of_match),
> },
> };
I discourage people from doing that because down the road somebody might
add a field here that's longer than the alignment tabs and then either
it becomes ugly or they either have to realign everything to keep it
pretty. Single spaces around '=' don't have that problem if used
consistently.
Thierry
signature.asc
Description: PGP signature
Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
Thanks for getting these drivers submitted upstream!
I don't really know anything about PWM, so just some random nitpicking
below..
> + iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
* already has a higher precedence than +, so no need for the inner
braces.
> + duty = ioread32(pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
Same here.
> + /* (1 << (16+scale)) * 10^9/rate = real_period */
unsigned long scalePow = (pwm->approx_period * (u64)rate) / 10;
no camcel case, please.
> + int scale = ilog2(scalePow) - 16;
> +
> + scale = clamp(scale, 0, 0xf);
Why not:
int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
> +static int sifive_pwm_clock_notifier(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct clk_notifier_data *ndata = data;
> + struct sifive_pwm_device *pwm = container_of(nb,
> + struct sifive_pwm_device,
> + notifier);
I don't think there are any guidlines, but I always prefer to just move
the whole container_of onto a new line:
struct sifive_pwm_device *pwm =
container_of(nb, struct sifive_pwm_device, notifier);
> +static struct platform_driver sifive_pwm_driver = {
> + .probe = sifive_pwm_probe,
> + .remove = sifive_pwm_remove,
> + .driver = {
> + .name = "pwm-sifivem",
> + .of_match_table = of_match_ptr(sifive_pwm_of_match),
> + },
> +};
What about using tabs to align this a little more nicely?
static struct platform_driver sifive_pwm_driver = {
.probe = sifive_pwm_probe,
.remove = sifive_pwm_remove,
.driver = {
.name = "pwm-sifivem",
.of_match_table = of_match_ptr(sifive_pwm_of_match),
},
};
Re: [RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
Thanks for getting these drivers submitted upstream!
I don't really know anything about PWM, so just some random nitpicking
below..
> + iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
* already has a higher precedence than +, so no need for the inner
braces.
> + duty = ioread32(pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
Same here.
> + /* (1 << (16+scale)) * 10^9/rate = real_period */
unsigned long scalePow = (pwm->approx_period * (u64)rate) / 10;
no camcel case, please.
> + int scale = ilog2(scalePow) - 16;
> +
> + scale = clamp(scale, 0, 0xf);
Why not:
int scale = clamp(ilog2(scale_pow) - 16, 0, 0xf);
> +static int sifive_pwm_clock_notifier(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct clk_notifier_data *ndata = data;
> + struct sifive_pwm_device *pwm = container_of(nb,
> + struct sifive_pwm_device,
> + notifier);
I don't think there are any guidlines, but I always prefer to just move
the whole container_of onto a new line:
struct sifive_pwm_device *pwm =
container_of(nb, struct sifive_pwm_device, notifier);
> +static struct platform_driver sifive_pwm_driver = {
> + .probe = sifive_pwm_probe,
> + .remove = sifive_pwm_remove,
> + .driver = {
> + .name = "pwm-sifivem",
> + .of_match_table = of_match_ptr(sifive_pwm_of_match),
> + },
> +};
What about using tabs to align this a little more nicely?
static struct platform_driver sifive_pwm_driver = {
.probe = sifive_pwm_probe,
.remove = sifive_pwm_remove,
.driver = {
.name = "pwm-sifivem",
.of_match_table = of_match_ptr(sifive_pwm_of_match),
},
};
[RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
From: "Wesley W. Terpstra"
Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
Signed-off-by: Wesley W. Terpstra
[Atish: Various fixes and code cleanup]
Signed-off-by: Atish Patra
---
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-sifive.c | 240 +++
3 files changed, 251 insertions(+)
create mode 100644 drivers/pwm/pwm-sifive.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 504d2527..dd12144d 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -378,6 +378,16 @@ config PWM_SAMSUNG
To compile this driver as a module, choose M here: the module
will be called pwm-samsung.
+config PWM_SIFIVE
+ tristate "SiFive PWM support"
+ depends on OF
+ depends on COMMON_CLK
+ help
+ Generic PWM framework driver for SiFive SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-sifive.
+
config PWM_SPEAR
tristate "STMicroelectronics SPEAr PWM support"
depends on PLAT_SPEAR
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9c676a0d..30089ca6 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR)+= pwm-rcar.o
obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
+obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
obj-$(CONFIG_PWM_SPEAR)+= pwm-spear.o
obj-$(CONFIG_PWM_STI) += pwm-sti.o
obj-$(CONFIG_PWM_STM32)+= pwm-stm32.o
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
new file mode 100644
index ..99580025
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,240 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 SiFive
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define MAX_PWM4
+
+/* Register offsets */
+#define REG_PWMCFG 0x0
+#define REG_PWMCOUNT 0x8
+#define REG_PWMS 0x10
+#defineREG_PWMCMP0 0x20
+
+/* PWMCFG fields */
+#define BIT_PWM_SCALE 0
+#define BIT_PWM_STICKY 8
+#define BIT_PWM_ZERO_ZMP 9
+#define BIT_PWM_DEGLITCH 10
+#define BIT_PWM_EN_ALWAYS 12
+#define BIT_PWM_EN_ONCE13
+#define BIT_PWM0_CENTER16
+#define BIT_PWM0_GANG 24
+#define BIT_PWM0_IP28
+
+#define SIZE_PWMCMP4
+#define MASK_PWM_SCALE 0xf
+
+struct sifive_pwm_device {
+ struct pwm_chip chip;
+ struct notifier_block notifier;
+ struct clk *clk;
+ void __iomem*regs;
+ unsigned intapprox_period;
+ unsigned intreal_period;
+};
+
+static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c)
+{
+ return container_of(c, struct sifive_pwm_device, chip);
+}
+
+static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+ unsigned int duty_cycle;
+ u32 frac;
+
+ duty_cycle = state->duty_cycle;
+ if (!state->enabled)
+ duty_cycle = 0;
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ duty_cycle = state->period - duty_cycle;
+
+ frac = ((u64)duty_cycle << 16) / state->period;
+ frac = min(frac, 0xU);
+
+ iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
+
+ if (state->enabled) {
+ state->period = pwm->real_period;
+ state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ state->duty_cycle = state->period - state->duty_cycle;
+ }
+
+ return 0;
+}
+
+static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev,
+struct pwm_state *state)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+ unsigned long duty;
+
+ duty = ioread32(pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
+
+ state->period = pwm->real_period;
+ state->duty_cycle = ((u64)duty * pwm->real_period) >> 16;
+ state->polarity = PWM_POLARITY_INVERSED;
+ state->enabled= duty > 0;
+}
+
+static const struct pwm_ops sifive_pwm_ops = {
+ .get_state = sifive_pwm_get_state,
+ .apply = sifive_pwm_apply,
+ .owner = THIS_MODULE,
+};
+
+static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip,
+ const struct of_phandle_args *args)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+
[RFC 2/4] pwm: sifive: Add a driver for SiFive SoC PWM
From: "Wesley W. Terpstra"
Adds a PWM driver for PWM chip present in SiFive's HiFive Unleashed SoC.
Signed-off-by: Wesley W. Terpstra
[Atish: Various fixes and code cleanup]
Signed-off-by: Atish Patra
---
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-sifive.c | 240 +++
3 files changed, 251 insertions(+)
create mode 100644 drivers/pwm/pwm-sifive.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 504d2527..dd12144d 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -378,6 +378,16 @@ config PWM_SAMSUNG
To compile this driver as a module, choose M here: the module
will be called pwm-samsung.
+config PWM_SIFIVE
+ tristate "SiFive PWM support"
+ depends on OF
+ depends on COMMON_CLK
+ help
+ Generic PWM framework driver for SiFive SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-sifive.
+
config PWM_SPEAR
tristate "STMicroelectronics SPEAr PWM support"
depends on PLAT_SPEAR
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9c676a0d..30089ca6 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR)+= pwm-rcar.o
obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
+obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
obj-$(CONFIG_PWM_SPEAR)+= pwm-spear.o
obj-$(CONFIG_PWM_STI) += pwm-sti.o
obj-$(CONFIG_PWM_STM32)+= pwm-stm32.o
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
new file mode 100644
index ..99580025
--- /dev/null
+++ b/drivers/pwm/pwm-sifive.c
@@ -0,0 +1,240 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 SiFive
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define MAX_PWM4
+
+/* Register offsets */
+#define REG_PWMCFG 0x0
+#define REG_PWMCOUNT 0x8
+#define REG_PWMS 0x10
+#defineREG_PWMCMP0 0x20
+
+/* PWMCFG fields */
+#define BIT_PWM_SCALE 0
+#define BIT_PWM_STICKY 8
+#define BIT_PWM_ZERO_ZMP 9
+#define BIT_PWM_DEGLITCH 10
+#define BIT_PWM_EN_ALWAYS 12
+#define BIT_PWM_EN_ONCE13
+#define BIT_PWM0_CENTER16
+#define BIT_PWM0_GANG 24
+#define BIT_PWM0_IP28
+
+#define SIZE_PWMCMP4
+#define MASK_PWM_SCALE 0xf
+
+struct sifive_pwm_device {
+ struct pwm_chip chip;
+ struct notifier_block notifier;
+ struct clk *clk;
+ void __iomem*regs;
+ unsigned intapprox_period;
+ unsigned intreal_period;
+};
+
+static inline struct sifive_pwm_device *to_sifive_pwm_chip(struct pwm_chip *c)
+{
+ return container_of(c, struct sifive_pwm_device, chip);
+}
+
+static int sifive_pwm_apply(struct pwm_chip *chip, struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+ unsigned int duty_cycle;
+ u32 frac;
+
+ duty_cycle = state->duty_cycle;
+ if (!state->enabled)
+ duty_cycle = 0;
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ duty_cycle = state->period - duty_cycle;
+
+ frac = ((u64)duty_cycle << 16) / state->period;
+ frac = min(frac, 0xU);
+
+ iowrite32(frac, pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
+
+ if (state->enabled) {
+ state->period = pwm->real_period;
+ state->duty_cycle = ((u64)frac * pwm->real_period) >> 16;
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ state->duty_cycle = state->period - state->duty_cycle;
+ }
+
+ return 0;
+}
+
+static void sifive_pwm_get_state(struct pwm_chip *chip, struct pwm_device *dev,
+struct pwm_state *state)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+ unsigned long duty;
+
+ duty = ioread32(pwm->regs + REG_PWMCMP0 + (dev->hwpwm * SIZE_PWMCMP));
+
+ state->period = pwm->real_period;
+ state->duty_cycle = ((u64)duty * pwm->real_period) >> 16;
+ state->polarity = PWM_POLARITY_INVERSED;
+ state->enabled= duty > 0;
+}
+
+static const struct pwm_ops sifive_pwm_ops = {
+ .get_state = sifive_pwm_get_state,
+ .apply = sifive_pwm_apply,
+ .owner = THIS_MODULE,
+};
+
+static struct pwm_device *sifive_pwm_xlate(struct pwm_chip *chip,
+ const struct of_phandle_args *args)
+{
+ struct sifive_pwm_device *pwm = to_sifive_pwm_chip(chip);
+
