| Message ID | 1412175188-28278-4-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 0000000..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 <boris.brezillon@free-electrons.com> > + * > + * 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 <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/clk.h> > +#include <linux/mfd/atmel-hlcdc.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/regmap.h> > + > +#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 = 1000000000; 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 = 1000000000; > + 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 come from? Why not make pwmcval = duty_ns * 255 if that's the maximum? > + 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? > +static int atmel_hlcdc_pwm_enable(struct pwm_chip *c, > + struct pwm_device *pwm) There's no need for line-wrapping here. The above fits on one line just fine. > +{ > + struct atmel_hlcdc_pwm_chip *chip = > + pwm_chip_to_atmel_hlcdc_pwm_chip(c); > + struct atmel_hlcdc *hlcdc = chip->hlcdc; > + u32 status; > + > + regmap_write(hlcdc->regmap, ATMEL_HLCDC_EN, ATMEL_HLCDC_PWM); > + while (!regmap_read(hlcdc->regmap, ATMEL_HLCDC_SR, &status) && > + !(status & ATMEL_HLCDC_PWM)) > + ; This loop isn't very readable. Can you improve it? Perhaps: do { err = regmap_read(hlcdc->regmap, ATMEL_HLCDC_SR, &status); if (err < 0) return err; } while ((status & ATMEL_HLCDC_PWM) == 0); That also allows errors to be properly propagated. Perhaps you also want to put a usleep_range() or similar in there. > +static void atmel_hlcdc_pwm_disable(struct pwm_chip *c, > + struct pwm_device *pwm) > +{ > + struct atmel_hlcdc_pwm_chip *chip = > + pwm_chip_to_atmel_hlcdc_pwm_chip(c); > + struct atmel_hlcdc *hlcdc = chip->hlcdc; > + u32 status; > + > + regmap_write(hlcdc->regmap, ATMEL_HLCDC_DIS, ATMEL_HLCDC_PWM); > + while (!regmap_read(hlcdc->regmap, ATMEL_HLCDC_SR, &status) && > + (status & ATMEL_HLCDC_PWM)) > + ; Same here. > +static int atmel_hlcdc_pwm_probe(struct platform_device *pdev) > +{ > + struct atmel_hlcdc_pwm_chip *chip; > + struct device *dev = &pdev->dev; > + struct atmel_hlcdc *hlcdc; > + int ret; > + > + hlcdc = dev_get_drvdata(dev->parent); > + if (!hlcdc) > + return -EINVAL; Can this really happen? > + ret = clk_prepare_enable(hlcdc->periph_clk); > + if (ret) > + return ret; > + > + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; Don't you want to disable and unprepare the clock here? Perhaps in order to avoid this call clk_prepare_enable() only after all resources have been allocated. > +MODULE_ALIAS("platform:atmel-hlcdc-pwm"); > +MODULE_AUTHOR("Boris Brezillon <boris.brezillon@free-electrons.com>"); > +MODULE_DESCRIPTION("Atmel HLCDC PWM driver"); > +MODULE_LICENSE("GPL"); According to the file header this needs to be "GPL v2". Thierry
On Mon, 6 Oct 2014 12:46:35 +0200 Thierry Reding <thierry.reding@gmail.com> 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 0000000..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 <boris.brezillon@free-electrons.com> > > + * > > + * 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 <http://www.gnu.org/licenses/>. > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/mfd/atmel-hlcdc.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/pwm.h> > > +#include <linux/regmap.h> > > + > > +#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 = 1000000000; > > 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 = 1000000000; > > + 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. > > 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 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. > > > + 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 ;-)). I'll address all other comments you made in this review. Thanks, Boris
On Mon, Oct 06, 2014 at 01:50:09PM +0200, Boris Brezillon wrote: > On Mon, 6 Oct 2014 12:46:35 +0200 Thierry Reding <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
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 0000000..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 <boris.brezillon@free-electrons.com> + * + * 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 <http://www.gnu.org/licenses/>. + */ + +#include <linux/clk.h> +#include <linux/mfd/atmel-hlcdc.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/regmap.h> + +#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 = 1000000000; + 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 = 1000000000; + 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; + + if (new_clk != hlcdc->slow_clk) + gencfg = ATMEL_HLCDC_CLKPWMSEL; + regmap_update_bits(hlcdc->regmap, ATMEL_HLCDC_CFG(0), + ATMEL_HLCDC_CLKPWMSEL, gencfg); + } + + do_div(pwmcval, period_ns); + if (pwmcval > 255) + 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; + + regmap_update_bits(hlcdc->regmap, ATMEL_HLCDC_CFG(6), + ATMEL_HLCDC_PWMPOL, cfg); + + return 0; +} + +static int atmel_hlcdc_pwm_enable(struct pwm_chip *c, + struct pwm_device *pwm) +{ + struct atmel_hlcdc_pwm_chip *chip = + pwm_chip_to_atmel_hlcdc_pwm_chip(c); + struct atmel_hlcdc *hlcdc = chip->hlcdc; + u32 status; + + regmap_write(hlcdc->regmap, ATMEL_HLCDC_EN, ATMEL_HLCDC_PWM); + while (!regmap_read(hlcdc->regmap, ATMEL_HLCDC_SR, &status) && + !(status & ATMEL_HLCDC_PWM)) + ; + + return 0; +} + +static void atmel_hlcdc_pwm_disable(struct pwm_chip *c, + struct pwm_device *pwm) +{ + struct atmel_hlcdc_pwm_chip *chip = + pwm_chip_to_atmel_hlcdc_pwm_chip(c); + struct atmel_hlcdc *hlcdc = chip->hlcdc; + u32 status; + + regmap_write(hlcdc->regmap, ATMEL_HLCDC_DIS, ATMEL_HLCDC_PWM); + while (!regmap_read(hlcdc->regmap, ATMEL_HLCDC_SR, &status) && + (status & ATMEL_HLCDC_PWM)) + ; +} + +static const struct pwm_ops atmel_hlcdc_pwm_ops = { + .config = atmel_hlcdc_pwm_config, + .set_polarity = atmel_hlcdc_pwm_set_polarity, + .enable = atmel_hlcdc_pwm_enable, + .disable = atmel_hlcdc_pwm_disable, + .owner = THIS_MODULE, +}; + +static int atmel_hlcdc_pwm_probe(struct platform_device *pdev) +{ + struct atmel_hlcdc_pwm_chip *chip; + struct device *dev = &pdev->dev; + struct atmel_hlcdc *hlcdc; + int ret; + + hlcdc = dev_get_drvdata(dev->parent); + if (!hlcdc) + return -EINVAL; + + ret = clk_prepare_enable(hlcdc->periph_clk); + if (ret) + return ret; + + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); + if (!chip) + return -ENOMEM; + + chip->hlcdc = hlcdc; + chip->chip.ops = &atmel_hlcdc_pwm_ops; + chip->chip.dev = dev; + chip->chip.base = -1; + chip->chip.npwm = 1; + chip->chip.of_xlate = of_pwm_xlate_with_flags; + chip->chip.of_pwm_n_cells = 3; + chip->chip.can_sleep = 1; + + ret = pwmchip_add(&chip->chip); + if (ret) + return ret; + + platform_set_drvdata(pdev, chip); + + return 0; +} + +static int atmel_hlcdc_pwm_remove(struct platform_device *pdev) +{ + struct atmel_hlcdc_pwm_chip *chip = platform_get_drvdata(pdev); + + clk_disable_unprepare(chip->hlcdc->periph_clk); + + return pwmchip_remove(&chip->chip); +} + +static const struct of_device_id atmel_hlcdc_pwm_dt_ids[] = { + { .compatible = "atmel,hlcdc-pwm" }, + { /* sentinel */ }, +}; + +static struct platform_driver atmel_hlcdc_pwm_driver = { + .driver = { + .name = "atmel-hlcdc-pwm", + .of_match_table = atmel_hlcdc_pwm_dt_ids, + }, + .probe = atmel_hlcdc_pwm_probe, + .remove = atmel_hlcdc_pwm_remove, +}; +module_platform_driver(atmel_hlcdc_pwm_driver); + +MODULE_ALIAS("platform:atmel-hlcdc-pwm"); +MODULE_AUTHOR("Boris Brezillon <boris.brezillon@free-electrons.com>"); +MODULE_DESCRIPTION("Atmel HLCDC PWM driver"); +MODULE_LICENSE("GPL");