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