[PATCH v7 03/11] pwm: add support for atmel-hlcdc-pwm device

[Thierry Reding]

On Mon, Oct 06, 2014 at 01:50:09PM +0200, Boris Brezillon wrote:
> On Mon, 6 Oct 2014 12:46:35 +0200 Thierry Reding  gmail.com> wrote:
> > On Wed, Oct 01, 2014 at 04:53:00PM +0200, Boris Brezillon wrote:
[...]
> > > + if (pres > ATMEL_HLCDC_PWMPS_MAX)
> > > + return -EINVAL;
> > 
> > I think the condition above needs to be "pres == ATMEL_HLCDC_PWMPS_MAX",
> > otherwise this will never be true.
> 
> Actually the previous loop is:
> 
>   for (pres = 0; pres *<=* ATMEL_HLCDC_PWMPS_MAX; pres++)
> 
> thus pres will be equal to ATMEL_HLCDC_PWMPS_MAX + 1 when no
> appropriate prescaler is found.

Indeed so.

> > > + regmap_update_bits(hlcdc->regmap, ATMEL_HLCDC_CFG(0),
> > > +ATMEL_HLCDC_CLKPWMSEL, gencfg);
> > > + }
> > > +
> > > + do_div(pwmcval, period_ns);
> > > + if (pwmcval > 255)
> > 
> > The PWM core already makes sure that duty_ns <= period_ns, so pwmcval
> > could be anywhere between 0 and 256 here. Where does the disconnect come
> > from? Why not make pwmcval = duty_ns * 255 if that's the maximum?
> 
> Here is what the datasheet says:
> 
> "Due to the comparison mechanism, the output pulse has a width between
> zero and 255 PWM counter cycles. Thus by adding a simple passive filter
> outside the chip, an analog voltage between 0 and (255/256) ? VDD can
> be obtained (for the positive polarity case, or between (1/256) ? VDD
> and VDD for the negative polarity case). Other voltage values can be
> obtained by adding active external circuitry."
> 
> Given this explanation we should divide by 256, but 256/256 is a
> forbidden value, hence I just use the maximum available one (255) when
> I'm asked to configure a duty cycle occupying the whole period.

Okay, perhaps you can summarize the above explanation from the datasheet
in a comment to clarify.

> > > + pwmcval = 255;
> > > +
> > > + pwmcfg |= ATMEL_HLCDC_PWMCVAL(pwmcval);
> > > +
> > > + regmap_update_bits(hlcdc->regmap, ATMEL_HLCDC_CFG(6),
> > > +ATMEL_HLCDC_PWMCVAL_MASK | ATMEL_HLCDC_PWMPS_MASK,
> > > +pwmcfg);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int atmel_hlcdc_pwm_set_polarity(struct pwm_chip *c,
> > > + struct pwm_device *pwm,
> > > + enum pwm_polarity polarity)
> > > +{
> > > + struct atmel_hlcdc_pwm_chip *chip =
> > > + pwm_chip_to_atmel_hlcdc_pwm_chip(c);
> > > + struct atmel_hlcdc *hlcdc = chip->hlcdc;
> > > + u32 cfg = 0;
> > > +
> > > + if (polarity == PWM_POLARITY_NORMAL)
> > > + cfg = ATMEL_HLCDC_PWMPOL;
> > 
> > That's strange. Inverse polarity is the default on this hardware?
> 
> Quote from the datasheet:
> 
> "
> ? PWMPOL: LCD Controller PWM Signal Polarity
> This bit defines the polarity of the PWM output signal. If set to one,
> the output pulses are high level (the output will be high when- ever
> the value in the counter is less than the value CVAL) If set to zero,
> the output pulses are low level.
> "
> 
> My understanding is that ATMEL_HLCDC_PWMPOL should be set when using
> normal polarity (and my tests confirm that it works as expected ;-)).

Yes, sounds good then.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[PATCH v7 03/11] pwm: add support for atmel-hlcdc-pwm device

[Boris Brezillon]

On Mon, 6 Oct 2014 12:46:35 +0200
Thierry Reding  wrote:

> On Wed, Oct 01, 2014 at 04:53:00PM +0200, Boris Brezillon wrote:
> [...]
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index b800783..afb896b 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -50,6 +50,16 @@ config PWM_ATMEL
> >   To compile this driver as a module, choose M here: the module
> >   will be called pwm-atmel.
> >  
> > +config PWM_ATMEL_HLCDC_PWM
> > +   tristate "Atmel HLCDC PWM support"
> > +   select MFD_ATMEL_HLCDC
> > +   depends on OF
> 
> This isn't really necessary since MFD_ATMEL_HLCDC already depends on OF.
> 
> > diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
> [...]
> > new file mode 100644
> > index 000..0238f7a
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-atmel-hlcdc.c
> > @@ -0,0 +1,229 @@
> > +/*
> > + * Copyright (C) 2014 Free Electrons
> > + * Copyright (C) 2014 Atmel
> > + *
> > + * Author: Boris BREZILLON 
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License version 2 as 
> > published by
> > + * the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful, but 
> > WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
> > for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along 
> > with
> > + * this program.  If not, see .
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define ATMEL_HLCDC_PWMCVAL_MASK   GENMASK(15, 8)
> > +#define ATMEL_HLCDC_PWMCVAL(x) ((x << 8) & 
> > ATMEL_HLCDC_PWMCVAL_MASK)
> 
> You might want to use an extra pair of parentheses around the "x" above.
> 
> > +struct atmel_hlcdc_pwm_chip {
> 
> Can we make this...
> 
> > +   struct pwm_chip chip;
> > +   struct atmel_hlcdc *hlcdc;
> > +   struct clk *cur_clk;
> > +};
> > +
> > +static inline struct atmel_hlcdc_pwm_chip *
> > +pwm_chip_to_atmel_hlcdc_pwm_chip(struct pwm_chip *chip)
> 
> ... and this a little shorter? There is a lot of line-wrapping below
> only because this is very long. It seems like just dropping the
> pwm_chip_ prefix on this function would be enough to not exceed the
> 78/80 character limit.
> 
> > +{
> > +   return container_of(chip, struct atmel_hlcdc_pwm_chip, chip);
> > +}
> > +
> > +static int atmel_hlcdc_pwm_config(struct pwm_chip *c,
> > + struct pwm_device *pwm,
> > + int duty_ns, int period_ns)
> > +{
> > +   struct atmel_hlcdc_pwm_chip *chip =
> > +   pwm_chip_to_atmel_hlcdc_pwm_chip(c);
> > +   struct atmel_hlcdc *hlcdc = chip->hlcdc;
> > +   struct clk *new_clk = hlcdc->slow_clk;
> > +   u64 pwmcval = duty_ns * 256;
> > +   unsigned long clk_freq;
> > +   u64 clk_period_ns;
> > +   u32 pwmcfg;
> > +   int pres;
> > +
> > +   clk_freq = clk_get_rate(new_clk);
> > +   clk_period_ns = 10;
> 
> NSEC_PER_SEC?
> 
> > +   clk_period_ns *= 256;
> 
> Perhaps collapse the above two in a single line:
> 
>   clk_period_ns = NSEC_PER_SEC * 256;
> 
> ?
> 
> > +   do_div(clk_period_ns, clk_freq);
> > +
> > +   if (clk_period_ns > period_ns) {
> > +   new_clk = hlcdc->sys_clk;
> > +   clk_freq = clk_get_rate(new_clk);
> > +   clk_period_ns = 10;
> > +   clk_period_ns *= 256;
> 
> Maybe:
> 
>   clk_period_ns = NSEC_PER_SEC * 256;
> 
> ?
> 
> > +   do_div(clk_period_ns, clk_freq);
> > +   }
> > +
> > +   for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++) {
> > +   if ((clk_period_ns << pres) >= period_ns)
> > +   break;
> > +   }
> 
> Technically there's no need for the curly braces.
> 
> > +
> > +   if (pres > ATMEL_HLCDC_PWMPS_MAX)
> > +   return -EINVAL;
> 
> I think the condition above needs to be "pres == ATMEL_HLCDC_PWMPS_MAX",
> otherwise this will never be true.

Actually the previous loop is:

for (pres = 0; pres *<=* ATMEL_HLCDC_PWMPS_MAX; pres++)

thus pres will be equal to ATMEL_HLCDC_PWMPS_MAX + 1 when no
appropriate prescaler is found.

> 
> > +
> > +   pwmcfg = ATMEL_HLCDC_PWMPS(pres);
> > +
> > +   if (new_clk != chip->cur_clk) {
> > +   u32 gencfg = 0;
> > +
> > +   clk_prepare_enable(new_clk);
> 
> This can fail so it needs error-checking.
> 
> > +   clk_disable_unprepare(chip->cur_clk);
> > +   chip->cur_clk = new_clk;
> > +
> > +   if (new_clk != hlcdc->slow_clk)
> > +   gencfg = ATMEL_HLCDC_CLKPWMSEL;
> 
> There are lots of negations here, which caused me to think that there
> was a third clock involved here, but it seems like new_clk can either be
> slow_clk or sys_clk.
> 
> 

[PATCH v7 03/11] pwm: add support for atmel-hlcdc-pwm device

[Thierry Reding]

On Wed, Oct 01, 2014 at 04:53:00PM +0200, Boris Brezillon wrote:
[...]
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b800783..afb896b 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -50,6 +50,16 @@ config PWM_ATMEL
> To compile this driver as a module, choose M here: the module
> will be called pwm-atmel.
>  
> +config PWM_ATMEL_HLCDC_PWM
> + tristate "Atmel HLCDC PWM support"
> + select MFD_ATMEL_HLCDC
> + depends on OF

This isn't really necessary since MFD_ATMEL_HLCDC already depends on OF.

> diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
[...]
> new file mode 100644
> index 000..0238f7a
> --- /dev/null
> +++ b/drivers/pwm/pwm-atmel-hlcdc.c
> @@ -0,0 +1,229 @@
> +/*
> + * Copyright (C) 2014 Free Electrons
> + * Copyright (C) 2014 Atmel
> + *
> + * Author: Boris BREZILLON 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published 
> by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program.  If not, see .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define ATMEL_HLCDC_PWMCVAL_MASK GENMASK(15, 8)
> +#define ATMEL_HLCDC_PWMCVAL(x)   ((x << 8) & 
> ATMEL_HLCDC_PWMCVAL_MASK)

You might want to use an extra pair of parentheses around the "x" above.

> +struct atmel_hlcdc_pwm_chip {

Can we make this...

> + struct pwm_chip chip;
> + struct atmel_hlcdc *hlcdc;
> + struct clk *cur_clk;
> +};
> +
> +static inline struct atmel_hlcdc_pwm_chip *
> +pwm_chip_to_atmel_hlcdc_pwm_chip(struct pwm_chip *chip)

... and this a little shorter? There is a lot of line-wrapping below
only because this is very long. It seems like just dropping the
pwm_chip_ prefix on this function would be enough to not exceed the
78/80 character limit.

> +{
> + return container_of(chip, struct atmel_hlcdc_pwm_chip, chip);
> +}
> +
> +static int atmel_hlcdc_pwm_config(struct pwm_chip *c,
> +   struct pwm_device *pwm,
> +   int duty_ns, int period_ns)
> +{
> + struct atmel_hlcdc_pwm_chip *chip =
> + pwm_chip_to_atmel_hlcdc_pwm_chip(c);
> + struct atmel_hlcdc *hlcdc = chip->hlcdc;
> + struct clk *new_clk = hlcdc->slow_clk;
> + u64 pwmcval = duty_ns * 256;
> + unsigned long clk_freq;
> + u64 clk_period_ns;
> + u32 pwmcfg;
> + int pres;
> +
> + clk_freq = clk_get_rate(new_clk);
> + clk_period_ns = 10;

NSEC_PER_SEC?

> + clk_period_ns *= 256;

Perhaps collapse the above two in a single line:

clk_period_ns = NSEC_PER_SEC * 256;

?

> + do_div(clk_period_ns, clk_freq);
> +
> + if (clk_period_ns > period_ns) {
> + new_clk = hlcdc->sys_clk;
> + clk_freq = clk_get_rate(new_clk);
> + clk_period_ns = 10;
> + clk_period_ns *= 256;

Maybe:

clk_period_ns = NSEC_PER_SEC * 256;

?

> + do_div(clk_period_ns, clk_freq);
> + }
> +
> + for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++) {
> + if ((clk_period_ns << pres) >= period_ns)
> + break;
> + }

Technically there's no need for the curly braces.

> +
> + if (pres > ATMEL_HLCDC_PWMPS_MAX)
> + return -EINVAL;

I think the condition above needs to be "pres == ATMEL_HLCDC_PWMPS_MAX",
otherwise this will never be true.

> +
> + pwmcfg = ATMEL_HLCDC_PWMPS(pres);
> +
> + if (new_clk != chip->cur_clk) {
> + u32 gencfg = 0;
> +
> + clk_prepare_enable(new_clk);

This can fail so it needs error-checking.

> + clk_disable_unprepare(chip->cur_clk);
> + chip->cur_clk = new_clk;
> +
> + if (new_clk != hlcdc->slow_clk)
> + gencfg = ATMEL_HLCDC_CLKPWMSEL;

There are lots of negations here, which caused me to think that there
was a third clock involved here, but it seems like new_clk can either be
slow_clk or sys_clk.

Perhaps making this condition "new_clk == hlcdc->sys_clk" would improve
clarity here. Maybe a comment somewhere would help?

> + regmap_update_bits(hlcdc->regmap, ATMEL_HLCDC_CFG(0),
> +ATMEL_HLCDC_CLKPWMSEL, gencfg);
> + }
> +
> + do_div(pwmcval, period_ns);
> + if (pwmcval > 255)

The PWM core already makes sure that duty_ns <= period_ns, so pwmcval
could be anywhere between 0 and 256 here. Where does the disconnect 

[PATCH v7 03/11] pwm: add support for atmel-hlcdc-pwm device

[Boris Brezillon]

From: Boris BREZILLON 

The HLCDC IP available in some Atmel SoCs (i.e. sam9x5i.e. at91sam9n12,
at91sam9x5 family or sama5d3 family) provide a PWM device.

This driver add support for a PWM chip exposing a single PWM device (which
will most likely be used to drive a backlight device).

Signed-off-by: Boris Brezillon 
Tested-by: Anthony Harivel 
Tested-by: Ludovic Desroches 
---
 drivers/pwm/Kconfig   |  10 ++
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-atmel-hlcdc.c | 229 ++
 3 files changed, 240 insertions(+)
 create mode 100644 drivers/pwm/pwm-atmel-hlcdc.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b800783..afb896b 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -50,6 +50,16 @@ config PWM_ATMEL
  To compile this driver as a module, choose M here: the module
  will be called pwm-atmel.

+config PWM_ATMEL_HLCDC_PWM
+   tristate "Atmel HLCDC PWM support"
+   select MFD_ATMEL_HLCDC
+   depends on OF
+   help
+ Generic PWM framework driver for Atmel HLCDC PWM.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-atmel.
+
 config PWM_ATMEL_TCB
tristate "Atmel TC Block PWM support"
depends on ATMEL_TCLIB && OF
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index f8c577d..eb0aae5 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_PWM)   += core.o
 obj-$(CONFIG_PWM_SYSFS)+= sysfs.o
 obj-$(CONFIG_PWM_AB8500)   += pwm-ab8500.o
 obj-$(CONFIG_PWM_ATMEL)+= pwm-atmel.o
+obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)  += pwm-atmel-hlcdc.o
 obj-$(CONFIG_PWM_ATMEL_TCB)+= pwm-atmel-tcb.o
 obj-$(CONFIG_PWM_BCM_KONA) += pwm-bcm-kona.o
 obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o
diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
new file mode 100644
index 000..0238f7a
--- /dev/null
+++ b/drivers/pwm/pwm-atmel-hlcdc.c
@@ -0,0 +1,229 @@
+/*
+ * Copyright (C) 2014 Free Electrons
+ * Copyright (C) 2014 Atmel
+ *
+ * Author: Boris BREZILLON 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define ATMEL_HLCDC_PWMCVAL_MASK   GENMASK(15, 8)
+#define ATMEL_HLCDC_PWMCVAL(x) ((x << 8) & ATMEL_HLCDC_PWMCVAL_MASK)
+#define ATMEL_HLCDC_PWMPOL BIT(4)
+#define ATMEL_HLCDC_PWMPS_MASK GENMASK(2, 0)
+#define ATMEL_HLCDC_PWMPS_MAX  0x6
+#define ATMEL_HLCDC_PWMPS(x)   ((x) & ATMEL_HLCDC_PWMPS_MASK)
+
+struct atmel_hlcdc_pwm_chip {
+   struct pwm_chip chip;
+   struct atmel_hlcdc *hlcdc;
+   struct clk *cur_clk;
+};
+
+static inline struct atmel_hlcdc_pwm_chip *
+pwm_chip_to_atmel_hlcdc_pwm_chip(struct pwm_chip *chip)
+{
+   return container_of(chip, struct atmel_hlcdc_pwm_chip, chip);
+}
+
+static int atmel_hlcdc_pwm_config(struct pwm_chip *c,
+ struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+   struct atmel_hlcdc_pwm_chip *chip =
+   pwm_chip_to_atmel_hlcdc_pwm_chip(c);
+   struct atmel_hlcdc *hlcdc = chip->hlcdc;
+   struct clk *new_clk = hlcdc->slow_clk;
+   u64 pwmcval = duty_ns * 256;
+   unsigned long clk_freq;
+   u64 clk_period_ns;
+   u32 pwmcfg;
+   int pres;
+
+   clk_freq = clk_get_rate(new_clk);
+   clk_period_ns = 10;
+   clk_period_ns *= 256;
+   do_div(clk_period_ns, clk_freq);
+
+   if (clk_period_ns > period_ns) {
+   new_clk = hlcdc->sys_clk;
+   clk_freq = clk_get_rate(new_clk);
+   clk_period_ns = 10;
+   clk_period_ns *= 256;
+   do_div(clk_period_ns, clk_freq);
+   }
+
+   for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++) {
+   if ((clk_period_ns << pres) >= period_ns)
+   break;
+   }
+
+   if (pres > ATMEL_HLCDC_PWMPS_MAX)
+   return -EINVAL;
+
+   pwmcfg = ATMEL_HLCDC_PWMPS(pres);
+
+   if (new_clk != chip->cur_clk) {
+   u32 gencfg = 0;
+
+   clk_prepare_enable(new_clk);
+   clk_disable_unprepare(chip->cur_clk);
+   chip->cur_clk = new_clk;
+
+