| Message ID | 1384766002-2852-1-git-send-email-voice.shen@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Thierry, On 11/18/2013 05:13 PM, Bo Shen wrote: > Add Atmel PWM controller driver based on PWM framework. > > This is the basic function implementation of Atmel PWM controller. > It can work with PWM based led and backlight. > > Signed-off-by: Bo Shen <voice.shen@atmel.com> > > Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> > --- > Changes in v8: > - replace pr_err by dev_err > > Changes in v7: > - fix issue for none device tree issues. > > Changes in v6: > - using relaxed version for writel and readl > - add none device tree support > - change some define from lower case to upper case > - split device tree binding document as a separate patch > > Changes in v5: > - call clk_disable directly, if so, it won't cause more than one channel > enabled, the clock can not be disabled. > > Changes in v4: > - check the return value of clk_prepare() > - change channel register size as constant > > Changes in v3: > - change compatible string from "atmel,sama5-pwm" to "atmel,sama5d3-pwm" > - Add PWM led example in binding documentation > - Using micro replace hard code > > Changes in v2: > - Address the comments from Thierry Reding > > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-atmel.c | 380 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 390 insertions(+) > create mode 100644 drivers/pwm/pwm-atmel.c Any new comments on this patch series? Best Regards, Bo Shen > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index eece329..5043572 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -41,6 +41,15 @@ config PWM_AB8500 > To compile this driver as a module, choose M here: the module > will be called pwm-ab8500. > > +config PWM_ATMEL > + tristate "Atmel PWM support" > + depends on ARCH_AT91 > + help > + Generic PWM framework driver for Atmel SoC. > + > + 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 8b754e4..1b99cfb 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -1,6 +1,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_TCB) += pwm-atmel-tcb.o > obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o > obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > new file mode 100644 > index 0000000..67cbac2 > --- /dev/null > +++ b/drivers/pwm/pwm-atmel.c > @@ -0,0 +1,380 @@ > +/* > + * Driver for Atmel Pulse Width Modulation Controller > + * > + * Copyright (C) 2013 Atmel Corporation > + * Bo Shen <voice.shen@atmel.com> > + * > + * Licensed under GPLv2. > + */ > + > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/slab.h> > + > +/* The following is global registers for PWM controller */ > +#define PWM_ENA 0x04 > +#define PWM_DIS 0x08 > +#define PWM_SR 0x0C > +/* Bit field in SR */ > +#define PWM_SR_ALL_CH_ON 0x0F > + > +/* The following register is PWM channel related registers */ > +#define PWM_CH_REG_OFFSET 0x200 > +#define PWM_CH_REG_SIZE 0x20 > + > +#define PWM_CMR 0x0 > +/* Bit field in CMR */ > +#define PWM_CMR_CPOL (1 << 9) > +#define PWM_CMR_UPD_CDTY (1 << 10) > + > +/* The following registers for PWM v1 */ > +#define PWMV1_CDTY 0x04 > +#define PWMV1_CPRD 0x08 > +#define PWMV1_CUPD 0x10 > + > +/* The following registers for PWM v2 */ > +#define PWMV2_CDTY 0x04 > +#define PWMV2_CDTYUPD 0x08 > +#define PWMV2_CPRD 0x0C > +#define PWMV2_CPRDUPD 0x10 > + > +/* Max value for duty and period > + * > + * Although the duty and period register is 32 bit, > + * however only the LSB 16 bits are significant. > + */ > +#define PWM_MAX_DTY 0xFFFF > +#define PWM_MAX_PRD 0xFFFF > +#define PRD_MAX_PRES 10 > + > +struct atmel_pwm_chip { > + struct pwm_chip chip; > + struct clk *clk; > + void __iomem *base; > + > + void (*config)(struct pwm_chip *chip, struct pwm_device *pwm, > + int dty, int prd); > +}; > + > +static inline struct atmel_pwm_chip *to_atmel_pwm_chip(struct pwm_chip *chip) > +{ > + return container_of(chip, struct atmel_pwm_chip, chip); > +} > + > +static inline u32 atmel_pwm_readl(struct atmel_pwm_chip *chip, > + unsigned long offset) > +{ > + return readl_relaxed(chip->base + offset); > +} > + > +static inline void atmel_pwm_writel(struct atmel_pwm_chip *chip, > + unsigned long offset, unsigned long val) > +{ > + writel_relaxed(val, chip->base + offset); > +} > + > +static inline u32 atmel_pwm_ch_readl(struct atmel_pwm_chip *chip, > + unsigned int ch, unsigned long offset) > +{ > + return readl_relaxed(chip->base + PWM_CH_REG_OFFSET + > + ch * PWM_CH_REG_SIZE + offset); > +} > + > +static inline void atmel_pwm_ch_writel(struct atmel_pwm_chip *chip, > + unsigned int ch, unsigned long offset, > + unsigned long val) > +{ > + writel_relaxed(val, chip->base + PWM_CH_REG_OFFSET + > + ch * PWM_CH_REG_SIZE + offset); > +} > + > +static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) > +{ > + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); > + unsigned long clk_rate, prd, dty; > + unsigned long long div; > + int ret, pres = 0; > + > + clk_rate = clk_get_rate(atmel_pwm->clk); > + div = clk_rate; > + > + /* Calculate the period cycles */ > + while (div > PWM_MAX_PRD) { > + div = clk_rate / (1 << pres); > + div = div * period_ns; > + /* 1/Hz = 100000000 ns */ > + do_div(div, 1000000000); > + > + if (pres++ > PRD_MAX_PRES) { > + dev_err(chip->dev, "pres exceed the maximum value\n"); > + return -EINVAL; > + } > + } > + > + /* Calculate the duty cycles */ > + prd = div; > + div *= duty_ns; > + do_div(div, period_ns); > + dty = div; > + > + ret = clk_enable(atmel_pwm->clk); > + if (ret) { > + dev_err(chip->dev, "failed to enable pwm clock\n"); > + return ret; > + } > + > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, pres); > + atmel_pwm->config(chip, pwm, dty, prd); > + > + clk_disable(atmel_pwm->clk); > + > + return 0; > +} > + > +static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm, > + int dty, int prd) > +{ > + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); > + unsigned int val; > + > + /* > + * If the PWM channel is disabled, write value to duty and period > + * registers directly. > + * If the PWM channel is enabled, using the update register, it needs > + * to set bit 10 of CMR to 0 > + */ > + if (test_bit(PWMF_ENABLED, &pwm->flags)) { > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty); > + > + val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR); > + val &= ~PWM_CMR_UPD_CDTY; > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val); > + } else { > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty); > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd); > + } > +} > + > +static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm, > + int dty, int prd) > +{ > + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); > + > + /* > + * If the PWM channel is disabled, write value to duty and period > + * registers directly. > + * If the PWM channel is enabled, using the duty update register to > + * update the value. > + */ > + if (test_bit(PWMF_ENABLED, &pwm->flags)) { > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty); > + } else { > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty); > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd); > + } > +} > + > +static int atmel_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, > + enum pwm_polarity polarity) > +{ > + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); > + u32 val = 0; > + int ret; > + > + if (polarity == PWM_POLARITY_NORMAL) > + val &= ~PWM_CMR_CPOL; > + else > + val |= PWM_CMR_CPOL; > + > + ret = clk_enable(atmel_pwm->clk); > + if (ret) { > + dev_err(chip->dev, "failed to enable pwm clock\n"); > + return ret; > + } > + > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val); > + > + clk_disable(atmel_pwm->clk); > + > + return 0; > +} > + > +static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); > + int ret; > + > + ret = clk_enable(atmel_pwm->clk); > + if (ret) { > + dev_err(chip->dev, "failed to enable pwm clock\n"); > + return ret; > + } > + > + atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm); > + > + return 0; > +} > + > +static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); > + > + atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm); > + > + clk_disable(atmel_pwm->clk); > +} > + > +static const struct pwm_ops atmel_pwm_ops = { > + .config = atmel_pwm_config, > + .set_polarity = atmel_pwm_set_polarity, > + .enable = atmel_pwm_enable, > + .disable = atmel_pwm_disable, > + .owner = THIS_MODULE, > +}; > + > +struct atmel_pwm_data { > + void (*config)(struct pwm_chip *chip, struct pwm_device *pwm, > + int dty, int prd); > +}; > + > +static const struct atmel_pwm_data atmel_pwm_data_v1 = { > + .config = atmel_pwm_config_v1, > +}; > + > +static const struct atmel_pwm_data atmel_pwm_data_v2 = { > + .config = atmel_pwm_config_v2, > +}; > + > +static const struct platform_device_id atmel_pwm_devtypes[] = { > + { > + .name = "at91sam9rl-pwm", > + .driver_data = (kernel_ulong_t)&atmel_pwm_data_v1, > + }, { > + .name = "sama5d3-pwm", > + .driver_data = (kernel_ulong_t)&atmel_pwm_data_v2, > + }, { > + /* sentinel */ > + }, > +}; > +MODULE_DEVICE_TABLE(platform, atmel_pwm_devtypes); > + > +#ifdef CONFIG_OF > +static const struct of_device_id atmel_pwm_dt_ids[] = { > + { > + .compatible = "atmel,at91sam9rl-pwm", > + .data = &atmel_pwm_data_v1, > + }, { > + .compatible = "atmel,sama5d3-pwm", > + .data = &atmel_pwm_data_v2, > + }, { > + /* sentinel */ > + }, > +}; > +MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids); > +#endif > + > +static inline const struct atmel_pwm_data * __init > + atmel_pwm_get_driver_data(struct platform_device *pdev) > +{ > + if (pdev->dev.of_node) { > + const struct of_device_id *match; > + match = of_match_device(atmel_pwm_dt_ids, &pdev->dev); > + if (match == NULL) > + return NULL; > + return match->data; > + } > + > + return (struct atmel_pwm_data *) > + platform_get_device_id(pdev)->driver_data; > +} > + > +static int atmel_pwm_probe(struct platform_device *pdev) > +{ > + const struct atmel_pwm_data *data; > + struct atmel_pwm_chip *atmel_pwm; > + struct resource *res; > + int ret; > + > + atmel_pwm = devm_kzalloc(&pdev->dev, sizeof(*atmel_pwm), GFP_KERNEL); > + if (!atmel_pwm) > + return -ENOMEM; > + > + data = atmel_pwm_get_driver_data(pdev); > + if (!data) > + return -ENODEV; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; > + > + atmel_pwm->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(atmel_pwm->base)) > + return PTR_ERR(atmel_pwm->base); > + > + atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(atmel_pwm->clk)) > + return PTR_ERR(atmel_pwm->clk); > + > + ret = clk_prepare(atmel_pwm->clk); > + if (ret) { > + dev_err(&pdev->dev, "failed to prepare pwm clock\n"); > + return ret; > + } > + > + atmel_pwm->chip.dev = &pdev->dev; > + atmel_pwm->chip.ops = &atmel_pwm_ops; > + if (pdev->dev.of_node) { > + atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; > + atmel_pwm->chip.of_pwm_n_cells = 3; > + atmel_pwm->chip.base = -1; > + } else { > + atmel_pwm->chip.base = pdev->id; > + } > + atmel_pwm->chip.npwm = 4; > + > + atmel_pwm->config = data->config; > + > + ret = pwmchip_add(&atmel_pwm->chip); > + if (ret < 0) { > + dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret); > + goto unprepare_clk; > + } > + > + platform_set_drvdata(pdev, atmel_pwm); > + > +unprepare_clk: > + clk_unprepare(atmel_pwm->clk); > + return ret; > +} > + > +static int atmel_pwm_remove(struct platform_device *pdev) > +{ > + struct atmel_pwm_chip *atmel_pwm = platform_get_drvdata(pdev); > + > + clk_unprepare(atmel_pwm->clk); > + > + return pwmchip_remove(&atmel_pwm->chip); > +} > + > +static struct platform_driver atmel_pwm_driver = { > + .driver = { > + .name = "atmel-pwm", > + .of_match_table = of_match_ptr(atmel_pwm_dt_ids), > + }, > + .id_table = atmel_pwm_devtypes, > + .probe = atmel_pwm_probe, > + .remove = atmel_pwm_remove, > +}; > +module_platform_driver(atmel_pwm_driver); > + > +MODULE_ALIAS("platform:atmel-pwm"); > +MODULE_AUTHOR("Bo Shen <voice.shen@atmel.com>"); > +MODULE_DESCRIPTION("Atmel PWM driver"); > +MODULE_LICENSE("GPL v2"); >
On Mon, Nov 18, 2013 at 05:13:21PM +0800, Bo Shen wrote: [...] > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c [...] > +/* Max value for duty and period Block comments should be of this form: /* * Max value ... * ... */ > +static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) > +{ > + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); > + unsigned long clk_rate, prd, dty; > + unsigned long long div; > + int ret, pres = 0; > + > + clk_rate = clk_get_rate(atmel_pwm->clk); > + div = clk_rate; > + > + /* Calculate the period cycles */ > + while (div > PWM_MAX_PRD) { > + div = clk_rate / (1 << pres); > + div = div * period_ns; > + /* 1/Hz = 100000000 ns */ I don't think that comment is needed. > + do_div(div, 1000000000); > + > + if (pres++ > PRD_MAX_PRES) { > + dev_err(chip->dev, "pres exceed the maximum value\n"); "exceeds" > + return -EINVAL; > + } > + } > + > + /* Calculate the duty cycles */ > + prd = div; > + div *= duty_ns; > + do_div(div, period_ns); > + dty = div; > + > + ret = clk_enable(atmel_pwm->clk); > + if (ret) { > + dev_err(chip->dev, "failed to enable pwm clock\n"); "PWM clock" > +static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm, > + int dty, int prd) > +{ > + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); > + unsigned int val; > + > + /* > + * If the PWM channel is disabled, write value to duty and period > + * registers directly. > + * If the PWM channel is enabled, using the update register, it needs > + * to set bit 10 of CMR to 0 > + */ I think it would make sense to split this comment and move each part into the respective conditional branch. > + if (test_bit(PWMF_ENABLED, &pwm->flags)) { > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty); > + > + val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR); > + val &= ~PWM_CMR_UPD_CDTY; > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val); > + } else { > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty); > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd); > + } > +} > + > +static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm, > + int dty, int prd) > +{ > + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); > + > + /* > + * If the PWM channel is disabled, write value to duty and period > + * registers directly. > + * If the PWM channel is enabled, using the duty update register to > + * update the value. > + */ Same here. > + if (test_bit(PWMF_ENABLED, &pwm->flags)) { > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty); > + } else { > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty); > + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd); > + } > +} Neither version 1 nor version 2 seem to be able to change the period while the channel is enabled. Perhaps that should be checked for in atmel_pwm_config() and an error (-EBUSY) returned? > + > +static int atmel_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, > + enum pwm_polarity polarity) > +{ > + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); > + u32 val = 0; > + int ret; > + > + if (polarity == PWM_POLARITY_NORMAL) > + val &= ~PWM_CMR_CPOL; > + else > + val |= PWM_CMR_CPOL; I think I've mentioned this before, but val is always assigned to 0, so clearing a bit is a superfluous. Perhaps you need to readl the CMR register first before toggling the bit here? > + > + ret = clk_enable(atmel_pwm->clk); > + if (ret) { > + dev_err(chip->dev, "failed to enable pwm clock\n"); "PWM clock" > +#ifdef CONFIG_OF > +static const struct of_device_id atmel_pwm_dt_ids[] = { > + { > + .compatible = "atmel,at91sam9rl-pwm", > + .data = &atmel_pwm_data_v1, > + }, { > + .compatible = "atmel,sama5d3-pwm", > + .data = &atmel_pwm_data_v2, > + }, { > + /* sentinel */ > + }, > +}; > +MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids); > +#endif I don't think you can do this. You use this table in a call to of_match_device() later on, in code which isn't protected by a corresponding #ifdef. > +static inline const struct atmel_pwm_data * __init > + atmel_pwm_get_driver_data(struct platform_device *pdev) I don't think __init is warranted here. In fact I think this will give you a build warning, because this code is called from atmel_pwm_probe(), which in turn isn't marked __init. Also it's probably not worth marking this inline explicitly. It isn't all that short, and the compiler will likely inline it anyway since it's only called once. > +{ > + if (pdev->dev.of_node) { > + const struct of_device_id *match; > + match = of_match_device(atmel_pwm_dt_ids, &pdev->dev); Blank line between the above two for readability. > + if (match == NULL) > + return NULL; > + return match->data; Same here. And "if (!match)" rather than "if (match == NULL)". > + } > + > + return (struct atmel_pwm_data *) > + platform_get_device_id(pdev)->driver_data; Please use a temporary variable here to make this more readable, like so: struct platform_device_id *id = platform_get_device_id(pdev); ... return (struct atmel_pwm_data *)id->driver; > +} > + > +static int atmel_pwm_probe(struct platform_device *pdev) > +{ > + const struct atmel_pwm_data *data; > + struct atmel_pwm_chip *atmel_pwm; > + struct resource *res; > + int ret; > + > + atmel_pwm = devm_kzalloc(&pdev->dev, sizeof(*atmel_pwm), GFP_KERNEL); > + if (!atmel_pwm) > + return -ENOMEM; You could move this further down, so that memory doesn't get allocated if atmel_pwm_get_driver_data() or platform_get_resource() fails. > + > + data = atmel_pwm_get_driver_data(pdev); > + if (!data) > + return -ENODEV; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; No need to check the return value here. devm_ioremap_resource() checks it for you. > + > + atmel_pwm->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(atmel_pwm->base)) > + return PTR_ERR(atmel_pwm->base); > + > + atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(atmel_pwm->clk)) > + return PTR_ERR(atmel_pwm->clk); > + > + ret = clk_prepare(atmel_pwm->clk); > + if (ret) { > + dev_err(&pdev->dev, "failed to prepare pwm clock\n"); "PWM clock" > + return ret; > + } > + > + atmel_pwm->chip.dev = &pdev->dev; > + atmel_pwm->chip.ops = &atmel_pwm_ops; > + if (pdev->dev.of_node) { Blank line between the above two for readability. > + atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; > + atmel_pwm->chip.of_pwm_n_cells = 3; > + atmel_pwm->chip.base = -1; > + } else { > + atmel_pwm->chip.base = pdev->id; That's not correct. The chip cannot be tied to pdev->id, because that ID is the instance number of the device. So typically you would have devices name like this: atmel-pwm.0 atmel-pwm.1 ... Now, if you have that, then you won't be able to register the second instance because the first instance will already have requested PWMs 0-3, and setting .base to 1 will cause PWMs 1-4 to be requested, which intersects with the range of the first instance. The same applies of course if you have other PWM controllers in the system which have similar instance names. So the right thing to do here is to provide that number via platform data so that platform code can define it, knowing in advance all ranges for all other PWM controllers and thereby make sure there's no intersection. > + } > + atmel_pwm->chip.npwm = 4; Blank line between the above two for readability. > + atmel_pwm->config = data->config; > + > + ret = pwmchip_add(&atmel_pwm->chip); > + if (ret < 0) { > + dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret); "PWM chip" Thierry
Hi Thierry, On 12/02/2013 06:59 PM, Thierry Reding wrote: > On Mon, Nov 18, 2013 at 05:13:21PM +0800, Bo Shen wrote: > [...] >> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > [...] >> +/* Max value for duty and period > > Block comments should be of this form: > > /* > * Max value ... > * ... > */ OK, I will use this style. >> +static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, >> + int duty_ns, int period_ns) >> +{ >> + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); >> + unsigned long clk_rate, prd, dty; >> + unsigned long long div; >> + int ret, pres = 0; >> + >> + clk_rate = clk_get_rate(atmel_pwm->clk); >> + div = clk_rate; >> + >> + /* Calculate the period cycles */ >> + while (div > PWM_MAX_PRD) { >> + div = clk_rate / (1 << pres); >> + div = div * period_ns; >> + /* 1/Hz = 100000000 ns */ > > I don't think that comment is needed. This is asked to be added. And, I think keep it and it won't hurt, what do you think? >> + do_div(div, 1000000000); >> + >> + if (pres++ > PRD_MAX_PRES) { >> + dev_err(chip->dev, "pres exceed the maximum value\n"); > > "exceeds" Thanks for correct it. >> + return -EINVAL; >> + } >> + } >> + >> + /* Calculate the duty cycles */ >> + prd = div; >> + div *= duty_ns; >> + do_div(div, period_ns); >> + dty = div; >> + >> + ret = clk_enable(atmel_pwm->clk); >> + if (ret) { >> + dev_err(chip->dev, "failed to enable pwm clock\n"); > > "PWM clock" OK, I will change all low case pwm to upper case PWM. >> +static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm, >> + int dty, int prd) >> +{ >> + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); >> + unsigned int val; >> + >> + /* >> + * If the PWM channel is disabled, write value to duty and period >> + * registers directly. >> + * If the PWM channel is enabled, using the update register, it needs >> + * to set bit 10 of CMR to 0 >> + */ > > I think it would make sense to split this comment and move each part > into the respective conditional branch. OK, I will split them. >> + if (test_bit(PWMF_ENABLED, &pwm->flags)) { >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty); >> + >> + val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR); >> + val &= ~PWM_CMR_UPD_CDTY; >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val); >> + } else { >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty); >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd); >> + } >> +} >> + >> +static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm, >> + int dty, int prd) >> +{ >> + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); >> + >> + /* >> + * If the PWM channel is disabled, write value to duty and period >> + * registers directly. >> + * If the PWM channel is enabled, using the duty update register to >> + * update the value. >> + */ > > Same here. > >> + if (test_bit(PWMF_ENABLED, &pwm->flags)) { >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty); >> + } else { >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty); >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd); >> + } >> +} > > Neither version 1 nor version 2 seem to be able to change the period > while the channel is enabled. Perhaps that should be checked for in > atmel_pwm_config() and an error (-EBUSY) returned? The period is configured in dt in device tree, or platform data in non device tree. Nowhere will update period. So, not code to update period. Am I right? If not, please figure out. >> + >> +static int atmel_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, >> + enum pwm_polarity polarity) >> +{ >> + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); >> + u32 val = 0; >> + int ret; >> + >> + if (polarity == PWM_POLARITY_NORMAL) >> + val &= ~PWM_CMR_CPOL; >> + else >> + val |= PWM_CMR_CPOL; > > I think I've mentioned this before, but val is always assigned to 0, so > clearing a bit is a superfluous. Perhaps you need to readl the CMR > register first before toggling the bit here? Thanks, we should read CMR, and set the CPOL accordingly. >> + >> + ret = clk_enable(atmel_pwm->clk); >> + if (ret) { >> + dev_err(chip->dev, "failed to enable pwm clock\n"); > > "PWM clock" > >> +#ifdef CONFIG_OF >> +static const struct of_device_id atmel_pwm_dt_ids[] = { >> + { >> + .compatible = "atmel,at91sam9rl-pwm", >> + .data = &atmel_pwm_data_v1, >> + }, { >> + .compatible = "atmel,sama5d3-pwm", >> + .data = &atmel_pwm_data_v2, >> + }, { >> + /* sentinel */ >> + }, >> +}; >> +MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids); >> +#endif > > I don't think you can do this. You use this table in a call to > of_match_device() later on, in code which isn't protected by a > corresponding #ifdef. I will remove #ifdef. >> +static inline const struct atmel_pwm_data * __init >> + atmel_pwm_get_driver_data(struct platform_device *pdev) > > I don't think __init is warranted here. In fact I think this will give > you a build warning, because this code is called from atmel_pwm_probe(), > which in turn isn't marked __init. OK, I will remove __init. > Also it's probably not worth marking this inline explicitly. It isn't > all that short, and the compiler will likely inline it anyway since it's > only called once. It only called one, so, it can be inline. >> +{ >> + if (pdev->dev.of_node) { >> + const struct of_device_id *match; >> + match = of_match_device(atmel_pwm_dt_ids, &pdev->dev); > > Blank line between the above two for readability. OK, I will add one blank line. >> + if (match == NULL) >> + return NULL; >> + return match->data; > > Same here. And "if (!match)" rather than "if (match == NULL)". OK, I will change like this. >> + } >> + >> + return (struct atmel_pwm_data *) >> + platform_get_device_id(pdev)->driver_data; > > Please use a temporary variable here to make this more readable, like > so: > > struct platform_device_id *id = platform_get_device_id(pdev); > > ... > > return (struct atmel_pwm_data *)id->driver; > >> +} OK, I will change like this. >> +static int atmel_pwm_probe(struct platform_device *pdev) >> +{ >> + const struct atmel_pwm_data *data; >> + struct atmel_pwm_chip *atmel_pwm; >> + struct resource *res; >> + int ret; >> + >> + atmel_pwm = devm_kzalloc(&pdev->dev, sizeof(*atmel_pwm), GFP_KERNEL); >> + if (!atmel_pwm) >> + return -ENOMEM; > > You could move this further down, so that memory doesn't get allocated > if atmel_pwm_get_driver_data() or platform_get_resource() fails. OK, I will move this down. >> + >> + data = atmel_pwm_get_driver_data(pdev); >> + if (!data) >> + return -ENODEV; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) >> + return -ENODEV; > > No need to check the return value here. devm_ioremap_resource() checks > it for you. OK, I will remove this check. >> + >> + atmel_pwm->base = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(atmel_pwm->base)) >> + return PTR_ERR(atmel_pwm->base); >> + >> + atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL); >> + if (IS_ERR(atmel_pwm->clk)) >> + return PTR_ERR(atmel_pwm->clk); >> + >> + ret = clk_prepare(atmel_pwm->clk); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to prepare pwm clock\n"); > > "PWM clock" > >> + return ret; >> + } >> + >> + atmel_pwm->chip.dev = &pdev->dev; >> + atmel_pwm->chip.ops = &atmel_pwm_ops; >> + if (pdev->dev.of_node) { > > Blank line between the above two for readability. OK, I will add blank line. >> + atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; >> + atmel_pwm->chip.of_pwm_n_cells = 3; >> + atmel_pwm->chip.base = -1; >> + } else { >> + atmel_pwm->chip.base = pdev->id; > > That's not correct. The chip cannot be tied to pdev->id, because that ID > is the instance number of the device. So typically you would have > devices name like this: > > atmel-pwm.0 > atmel-pwm.1 > ... > > Now, if you have that, then you won't be able to register the second > instance because the first instance will already have requested PWMs > 0-3, and setting .base to 1 will cause PWMs 1-4 to be requested, which > intersects with the range of the first instance. > > The same applies of course if you have other PWM controllers in the > system which have similar instance names. > > So the right thing to do here is to provide that number via platform > data so that platform code can define it, knowing in advance all ranges > for all other PWM controllers and thereby make sure there's no > intersection. OK, I will fix this. >> + } >> + atmel_pwm->chip.npwm = 4; > > Blank line between the above two for readability. OK, I will add blank line. >> + atmel_pwm->config = data->config; >> + >> + ret = pwmchip_add(&atmel_pwm->chip); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret); > > "PWM chip" > > Thierry Best Regards, Bo Shen
On Tue, Dec 03, 2013 at 11:09:12AM +0800, Bo Shen wrote: > On 12/02/2013 06:59 PM, Thierry Reding wrote: > >On Mon, Nov 18, 2013 at 05:13:21PM +0800, Bo Shen wrote: [...] > >>diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c [...] > >>+ /* Calculate the period cycles */ > >>+ while (div > PWM_MAX_PRD) { > >>+ div = clk_rate / (1 << pres); > >>+ div = div * period_ns; > >>+ /* 1/Hz = 100000000 ns */ > > > >I don't think that comment is needed. > > This is asked to be added. > And, I think keep it and it won't hurt, what do you think? I think it's obvious that you're converting from nanoseconds because of the _ns prefix in period_ns. But if somebody requested this and everyone else thinks it's useful, I'm okay with keeping it. > >>+ if (test_bit(PWMF_ENABLED, &pwm->flags)) { > >>+ atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty); > >>+ } else { > >>+ atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty); > >>+ atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd); > >>+ } > >>+} > > > >Neither version 1 nor version 2 seem to be able to change the period > >while the channel is enabled. Perhaps that should be checked for in > >atmel_pwm_config() and an error (-EBUSY) returned? > > The period is configured in dt in device tree, or platform data in non > device tree. Nowhere will update period. So, not code to update period. > Am I right? If not, please figure out. The .config() operation allows the period to be specified. Just because nobody currently changes it at runtime doesn't mean it can't be done. It is also possible that whoever wrote the device tree or platform data didn't know that the period must be the same for all PWM channels and therefore might use different values. If you check for this, at least they'll notice. If you don't check for it, then they may end up having the period reconfigured behind their backs, which may cause parts of their setup to behave unexpectedly. Thierry
Hi Thierry, On 12/03/2013 05:43 PM, Thierry Reding wrote: > On Tue, Dec 03, 2013 at 11:09:12AM +0800, Bo Shen wrote: >> On 12/02/2013 06:59 PM, Thierry Reding wrote: >>> On Mon, Nov 18, 2013 at 05:13:21PM +0800, Bo Shen wrote: > [...] >>>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > [...] >>>> + /* Calculate the period cycles */ >>>> + while (div > PWM_MAX_PRD) { >>>> + div = clk_rate / (1 << pres); >>>> + div = div * period_ns; >>>> + /* 1/Hz = 100000000 ns */ >>> >>> I don't think that comment is needed. >> >> This is asked to be added. >> And, I think keep it and it won't hurt, what do you think? > > I think it's obvious that you're converting from nanoseconds because of > the _ns prefix in period_ns. But if somebody requested this and everyone > else thinks it's useful, I'm okay with keeping it. > >>>> + if (test_bit(PWMF_ENABLED, &pwm->flags)) { >>>> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty); >>>> + } else { >>>> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty); >>>> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd); >>>> + } >>>> +} >>> >>> Neither version 1 nor version 2 seem to be able to change the period >>> while the channel is enabled. Perhaps that should be checked for in >>> atmel_pwm_config() and an error (-EBUSY) returned? >> >> The period is configured in dt in device tree, or platform data in non >> device tree. Nowhere will update period. So, not code to update period. >> Am I right? If not, please figure out. > > The .config() operation allows the period to be specified. Just because > nobody currently changes it at runtime doesn't mean it can't be done. > > It is also possible that whoever wrote the device tree or platform data > didn't know that the period must be the same for all PWM channels and > therefore might use different values. If you check for this, at least > they'll notice. If you don't check for it, then they may end up having > the period reconfigured behind their backs, which may cause parts of > their setup to behave unexpectedly. Thanks for this information. I will add code for changing period. > Thierry > Best Regards, Bo Shen
On Wed, Dec 04, 2013 at 10:59:46AM +0800, Bo Shen wrote: > Hi Thierry, > > On 12/03/2013 05:43 PM, Thierry Reding wrote: > >On Tue, Dec 03, 2013 at 11:09:12AM +0800, Bo Shen wrote: > >>On 12/02/2013 06:59 PM, Thierry Reding wrote: > >>>On Mon, Nov 18, 2013 at 05:13:21PM +0800, Bo Shen wrote: > >[...] > >>>>diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > >[...] > >>>>+ /* Calculate the period cycles */ > >>>>+ while (div > PWM_MAX_PRD) { > >>>>+ div = clk_rate / (1 << pres); > >>>>+ div = div * period_ns; > >>>>+ /* 1/Hz = 100000000 ns */ > >>> > >>>I don't think that comment is needed. > >> > >>This is asked to be added. > >>And, I think keep it and it won't hurt, what do you think? > > > >I think it's obvious that you're converting from nanoseconds because of > >the _ns prefix in period_ns. But if somebody requested this and everyone > >else thinks it's useful, I'm okay with keeping it. > > > >>>>+ if (test_bit(PWMF_ENABLED, &pwm->flags)) { > >>>>+ atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty); > >>>>+ } else { > >>>>+ atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty); > >>>>+ atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd); > >>>>+ } > >>>>+} > >>> > >>>Neither version 1 nor version 2 seem to be able to change the period > >>>while the channel is enabled. Perhaps that should be checked for in > >>>atmel_pwm_config() and an error (-EBUSY) returned? > >> > >>The period is configured in dt in device tree, or platform data in non > >>device tree. Nowhere will update period. So, not code to update period. > >>Am I right? If not, please figure out. > > > >The .config() operation allows the period to be specified. Just because > >nobody currently changes it at runtime doesn't mean it can't be done. > > > >It is also possible that whoever wrote the device tree or platform data > >didn't know that the period must be the same for all PWM channels and > >therefore might use different values. If you check for this, at least > >they'll notice. If you don't check for it, then they may end up having > >the period reconfigured behind their backs, which may cause parts of > >their setup to behave unexpectedly. > > Thanks for this information. > I will add code for changing period. Just to clarify: I wouldn't want this code to allow changing the period but rather reject incompatible changes to the period with an error code. Thierry
Hi Thierry, On 12/04/2013 06:03 PM, Thierry Reding wrote: > On Wed, Dec 04, 2013 at 10:59:46AM +0800, Bo Shen wrote: >> Hi Thierry, >> >> On 12/03/2013 05:43 PM, Thierry Reding wrote: >>> On Tue, Dec 03, 2013 at 11:09:12AM +0800, Bo Shen wrote: >>>> On 12/02/2013 06:59 PM, Thierry Reding wrote: >>>>> On Mon, Nov 18, 2013 at 05:13:21PM +0800, Bo Shen wrote: >>> [...] >>>>>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c >>> [...] >>>>>> + /* Calculate the period cycles */ >>>>>> + while (div > PWM_MAX_PRD) { >>>>>> + div = clk_rate / (1 << pres); >>>>>> + div = div * period_ns; >>>>>> + /* 1/Hz = 100000000 ns */ >>>>> >>>>> I don't think that comment is needed. >>>> >>>> This is asked to be added. >>>> And, I think keep it and it won't hurt, what do you think? >>> >>> I think it's obvious that you're converting from nanoseconds because of >>> the _ns prefix in period_ns. But if somebody requested this and everyone >>> else thinks it's useful, I'm okay with keeping it. >>> >>>>>> + if (test_bit(PWMF_ENABLED, &pwm->flags)) { >>>>>> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty); >>>>>> + } else { >>>>>> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty); >>>>>> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd); >>>>>> + } >>>>>> +} >>>>> >>>>> Neither version 1 nor version 2 seem to be able to change the period >>>>> while the channel is enabled. Perhaps that should be checked for in >>>>> atmel_pwm_config() and an error (-EBUSY) returned? >>>> >>>> The period is configured in dt in device tree, or platform data in non >>>> device tree. Nowhere will update period. So, not code to update period. >>>> Am I right? If not, please figure out. >>> >>> The .config() operation allows the period to be specified. Just because >>> nobody currently changes it at runtime doesn't mean it can't be done. >>> >>> It is also possible that whoever wrote the device tree or platform data >>> didn't know that the period must be the same for all PWM channels and >>> therefore might use different values. If you check for this, at least >>> they'll notice. If you don't check for it, then they may end up having >>> the period reconfigured behind their backs, which may cause parts of >>> their setup to behave unexpectedly. >> >> Thanks for this information. >> I will add code for changing period. > > Just to clarify: I wouldn't want this code to allow changing the period > but rather reject incompatible changes to the period with an error code. So, in this patch, just check it as you suggested in previous email, would it be OK? --->8--- Perhaps that should be checked for in atmel_pwm_config() and an error (-EBUSY) returned? ---8<--- > Thierry Best Regards, Bo Shen
On Thu, Dec 05, 2013 at 09:11:28AM +0800, Bo Shen wrote: > Hi Thierry, > > On 12/04/2013 06:03 PM, Thierry Reding wrote: > >On Wed, Dec 04, 2013 at 10:59:46AM +0800, Bo Shen wrote: > >>Hi Thierry, > >> > >>On 12/03/2013 05:43 PM, Thierry Reding wrote: > >>>On Tue, Dec 03, 2013 at 11:09:12AM +0800, Bo Shen wrote: > >>>>On 12/02/2013 06:59 PM, Thierry Reding wrote: > >>>>>On Mon, Nov 18, 2013 at 05:13:21PM +0800, Bo Shen wrote: > >>>[...] > >>>>>>diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > >>>[...] > >>>>>>+ /* Calculate the period cycles */ > >>>>>>+ while (div > PWM_MAX_PRD) { > >>>>>>+ div = clk_rate / (1 << pres); > >>>>>>+ div = div * period_ns; > >>>>>>+ /* 1/Hz = 100000000 ns */ > >>>>> > >>>>>I don't think that comment is needed. > >>>> > >>>>This is asked to be added. > >>>>And, I think keep it and it won't hurt, what do you think? > >>> > >>>I think it's obvious that you're converting from nanoseconds because of > >>>the _ns prefix in period_ns. But if somebody requested this and everyone > >>>else thinks it's useful, I'm okay with keeping it. > >>> > >>>>>>+ if (test_bit(PWMF_ENABLED, &pwm->flags)) { > >>>>>>+ atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty); > >>>>>>+ } else { > >>>>>>+ atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty); > >>>>>>+ atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd); > >>>>>>+ } > >>>>>>+} > >>>>> > >>>>>Neither version 1 nor version 2 seem to be able to change the period > >>>>>while the channel is enabled. Perhaps that should be checked for in > >>>>>atmel_pwm_config() and an error (-EBUSY) returned? > >>>> > >>>>The period is configured in dt in device tree, or platform data in non > >>>>device tree. Nowhere will update period. So, not code to update period. > >>>>Am I right? If not, please figure out. > >>> > >>>The .config() operation allows the period to be specified. Just because > >>>nobody currently changes it at runtime doesn't mean it can't be done. > >>> > >>>It is also possible that whoever wrote the device tree or platform data > >>>didn't know that the period must be the same for all PWM channels and > >>>therefore might use different values. If you check for this, at least > >>>they'll notice. If you don't check for it, then they may end up having > >>>the period reconfigured behind their backs, which may cause parts of > >>>their setup to behave unexpectedly. > >> > >>Thanks for this information. > >>I will add code for changing period. > > > >Just to clarify: I wouldn't want this code to allow changing the period > >but rather reject incompatible changes to the period with an error code. > > So, in this patch, just check it as you suggested in previous email, would > it be OK? > --->8--- > Perhaps that should be checked for in atmel_pwm_config() and an error > (-EBUSY) returned? > ---8<--- Yes. If a user tries to set a period that conflicts with a previously set period (by another PWM channel), then pwm_config() for the second user should fail. Thierry
Hi Thierry, On 12/03/2013 11:09 AM, Bo Shen wrote: >>> + atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; >>> + atmel_pwm->chip.of_pwm_n_cells = 3; >>> + atmel_pwm->chip.base = -1; >>> + } else { >>> + atmel_pwm->chip.base = pdev->id; >> >> That's not correct. The chip cannot be tied to pdev->id, because that ID >> is the instance number of the device. So typically you would have >> devices name like this: >> >> atmel-pwm.0 >> atmel-pwm.1 >> ... >> >> Now, if you have that, then you won't be able to register the second >> instance because the first instance will already have requested PWMs >> 0-3, and setting .base to 1 will cause PWMs 1-4 to be requested, which >> intersects with the range of the first instance. >> >> The same applies of course if you have other PWM controllers in the >> system which have similar instance names. >> >> So the right thing to do here is to provide that number via platform >> data so that platform code can define it, knowing in advance all ranges >> for all other PWM controllers and thereby make sure there's no >> intersection. > > OK, I will fix this. After read deeply of PWM framework, for non device tree, I think we'd better let the PWM core to choose chip.base as device tree, while not pass a number through platform data to it. Or else, it will confuse the user to set the chip.base, must set it in correct value to avoid intersection. And, actually we won't use chip.base in driver itself. Hi J, what's your opinion? Best Regards, Bo Shen
On Tue, Dec 10, 2013 at 10:20:33AM +0800, Bo Shen wrote: > Hi Thierry, > > On 12/03/2013 11:09 AM, Bo Shen wrote: > >>>+ atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; > >>>+ atmel_pwm->chip.of_pwm_n_cells = 3; > >>>+ atmel_pwm->chip.base = -1; > >>>+ } else { > >>>+ atmel_pwm->chip.base = pdev->id; > >> > >>That's not correct. The chip cannot be tied to pdev->id, because that ID > >>is the instance number of the device. So typically you would have > >>devices name like this: > >> > >> atmel-pwm.0 > >> atmel-pwm.1 > >> ... > >> > >>Now, if you have that, then you won't be able to register the second > >>instance because the first instance will already have requested PWMs > >>0-3, and setting .base to 1 will cause PWMs 1-4 to be requested, which > >>intersects with the range of the first instance. > >> > >>The same applies of course if you have other PWM controllers in the > >>system which have similar instance names. > >> > >>So the right thing to do here is to provide that number via platform > >>data so that platform code can define it, knowing in advance all ranges > >>for all other PWM controllers and thereby make sure there's no > >>intersection. > > > >OK, I will fix this. > > After read deeply of PWM framework, for non device tree, I think we'd better > let the PWM core to choose chip.base as device tree, while not pass a number > through platform data to it. Or else, it will confuse the user to set the > chip.base, must set it in correct value to avoid intersection. And, actually > we won't use chip.base in driver itself. Yes, that should work as well, if you make sure that every user actually has the PWM lookup table and doesn't rely on a fixed global index to retrieve the PWM channel. Thierry
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index eece329..5043572 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -41,6 +41,15 @@ config PWM_AB8500 To compile this driver as a module, choose M here: the module will be called pwm-ab8500. +config PWM_ATMEL + tristate "Atmel PWM support" + depends on ARCH_AT91 + help + Generic PWM framework driver for Atmel SoC. + + 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 8b754e4..1b99cfb 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -1,6 +1,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_TCB) += pwm-atmel-tcb.o obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c new file mode 100644 index 0000000..67cbac2 --- /dev/null +++ b/drivers/pwm/pwm-atmel.c @@ -0,0 +1,380 @@ +/* + * Driver for Atmel Pulse Width Modulation Controller + * + * Copyright (C) 2013 Atmel Corporation + * Bo Shen <voice.shen@atmel.com> + * + * Licensed under GPLv2. + */ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/slab.h> + +/* The following is global registers for PWM controller */ +#define PWM_ENA 0x04 +#define PWM_DIS 0x08 +#define PWM_SR 0x0C +/* Bit field in SR */ +#define PWM_SR_ALL_CH_ON 0x0F + +/* The following register is PWM channel related registers */ +#define PWM_CH_REG_OFFSET 0x200 +#define PWM_CH_REG_SIZE 0x20 + +#define PWM_CMR 0x0 +/* Bit field in CMR */ +#define PWM_CMR_CPOL (1 << 9) +#define PWM_CMR_UPD_CDTY (1 << 10) + +/* The following registers for PWM v1 */ +#define PWMV1_CDTY 0x04 +#define PWMV1_CPRD 0x08 +#define PWMV1_CUPD 0x10 + +/* The following registers for PWM v2 */ +#define PWMV2_CDTY 0x04 +#define PWMV2_CDTYUPD 0x08 +#define PWMV2_CPRD 0x0C +#define PWMV2_CPRDUPD 0x10 + +/* Max value for duty and period + * + * Although the duty and period register is 32 bit, + * however only the LSB 16 bits are significant. + */ +#define PWM_MAX_DTY 0xFFFF +#define PWM_MAX_PRD 0xFFFF +#define PRD_MAX_PRES 10 + +struct atmel_pwm_chip { + struct pwm_chip chip; + struct clk *clk; + void __iomem *base; + + void (*config)(struct pwm_chip *chip, struct pwm_device *pwm, + int dty, int prd); +}; + +static inline struct atmel_pwm_chip *to_atmel_pwm_chip(struct pwm_chip *chip) +{ + return container_of(chip, struct atmel_pwm_chip, chip); +} + +static inline u32 atmel_pwm_readl(struct atmel_pwm_chip *chip, + unsigned long offset) +{ + return readl_relaxed(chip->base + offset); +} + +static inline void atmel_pwm_writel(struct atmel_pwm_chip *chip, + unsigned long offset, unsigned long val) +{ + writel_relaxed(val, chip->base + offset); +} + +static inline u32 atmel_pwm_ch_readl(struct atmel_pwm_chip *chip, + unsigned int ch, unsigned long offset) +{ + return readl_relaxed(chip->base + PWM_CH_REG_OFFSET + + ch * PWM_CH_REG_SIZE + offset); +} + +static inline void atmel_pwm_ch_writel(struct atmel_pwm_chip *chip, + unsigned int ch, unsigned long offset, + unsigned long val) +{ + writel_relaxed(val, chip->base + PWM_CH_REG_OFFSET + + ch * PWM_CH_REG_SIZE + offset); +} + +static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); + unsigned long clk_rate, prd, dty; + unsigned long long div; + int ret, pres = 0; + + clk_rate = clk_get_rate(atmel_pwm->clk); + div = clk_rate; + + /* Calculate the period cycles */ + while (div > PWM_MAX_PRD) { + div = clk_rate / (1 << pres); + div = div * period_ns; + /* 1/Hz = 100000000 ns */ + do_div(div, 1000000000); + + if (pres++ > PRD_MAX_PRES) { + dev_err(chip->dev, "pres exceed the maximum value\n"); + return -EINVAL; + } + } + + /* Calculate the duty cycles */ + prd = div; + div *= duty_ns; + do_div(div, period_ns); + dty = div; + + ret = clk_enable(atmel_pwm->clk); + if (ret) { + dev_err(chip->dev, "failed to enable pwm clock\n"); + return ret; + } + + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, pres); + atmel_pwm->config(chip, pwm, dty, prd); + + clk_disable(atmel_pwm->clk); + + return 0; +} + +static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm, + int dty, int prd) +{ + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); + unsigned int val; + + /* + * If the PWM channel is disabled, write value to duty and period + * registers directly. + * If the PWM channel is enabled, using the update register, it needs + * to set bit 10 of CMR to 0 + */ + if (test_bit(PWMF_ENABLED, &pwm->flags)) { + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty); + + val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR); + val &= ~PWM_CMR_UPD_CDTY; + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val); + } else { + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty); + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd); + } +} + +static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm, + int dty, int prd) +{ + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); + + /* + * If the PWM channel is disabled, write value to duty and period + * registers directly. + * If the PWM channel is enabled, using the duty update register to + * update the value. + */ + if (test_bit(PWMF_ENABLED, &pwm->flags)) { + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty); + } else { + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty); + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd); + } +} + +static int atmel_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, + enum pwm_polarity polarity) +{ + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); + u32 val = 0; + int ret; + + if (polarity == PWM_POLARITY_NORMAL) + val &= ~PWM_CMR_CPOL; + else + val |= PWM_CMR_CPOL; + + ret = clk_enable(atmel_pwm->clk); + if (ret) { + dev_err(chip->dev, "failed to enable pwm clock\n"); + return ret; + } + + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val); + + clk_disable(atmel_pwm->clk); + + return 0; +} + +static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); + int ret; + + ret = clk_enable(atmel_pwm->clk); + if (ret) { + dev_err(chip->dev, "failed to enable pwm clock\n"); + return ret; + } + + atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm); + + return 0; +} + +static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); + + atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm); + + clk_disable(atmel_pwm->clk); +} + +static const struct pwm_ops atmel_pwm_ops = { + .config = atmel_pwm_config, + .set_polarity = atmel_pwm_set_polarity, + .enable = atmel_pwm_enable, + .disable = atmel_pwm_disable, + .owner = THIS_MODULE, +}; + +struct atmel_pwm_data { + void (*config)(struct pwm_chip *chip, struct pwm_device *pwm, + int dty, int prd); +}; + +static const struct atmel_pwm_data atmel_pwm_data_v1 = { + .config = atmel_pwm_config_v1, +}; + +static const struct atmel_pwm_data atmel_pwm_data_v2 = { + .config = atmel_pwm_config_v2, +}; + +static const struct platform_device_id atmel_pwm_devtypes[] = { + { + .name = "at91sam9rl-pwm", + .driver_data = (kernel_ulong_t)&atmel_pwm_data_v1, + }, { + .name = "sama5d3-pwm", + .driver_data = (kernel_ulong_t)&atmel_pwm_data_v2, + }, { + /* sentinel */ + }, +}; +MODULE_DEVICE_TABLE(platform, atmel_pwm_devtypes); + +#ifdef CONFIG_OF +static const struct of_device_id atmel_pwm_dt_ids[] = { + { + .compatible = "atmel,at91sam9rl-pwm", + .data = &atmel_pwm_data_v1, + }, { + .compatible = "atmel,sama5d3-pwm", + .data = &atmel_pwm_data_v2, + }, { + /* sentinel */ + }, +}; +MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids); +#endif + +static inline const struct atmel_pwm_data * __init + atmel_pwm_get_driver_data(struct platform_device *pdev) +{ + if (pdev->dev.of_node) { + const struct of_device_id *match; + match = of_match_device(atmel_pwm_dt_ids, &pdev->dev); + if (match == NULL) + return NULL; + return match->data; + } + + return (struct atmel_pwm_data *) + platform_get_device_id(pdev)->driver_data; +} + +static int atmel_pwm_probe(struct platform_device *pdev) +{ + const struct atmel_pwm_data *data; + struct atmel_pwm_chip *atmel_pwm; + struct resource *res; + int ret; + + atmel_pwm = devm_kzalloc(&pdev->dev, sizeof(*atmel_pwm), GFP_KERNEL); + if (!atmel_pwm) + return -ENOMEM; + + data = atmel_pwm_get_driver_data(pdev); + if (!data) + return -ENODEV; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return -ENODEV; + + atmel_pwm->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(atmel_pwm->base)) + return PTR_ERR(atmel_pwm->base); + + atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(atmel_pwm->clk)) + return PTR_ERR(atmel_pwm->clk); + + ret = clk_prepare(atmel_pwm->clk); + if (ret) { + dev_err(&pdev->dev, "failed to prepare pwm clock\n"); + return ret; + } + + atmel_pwm->chip.dev = &pdev->dev; + atmel_pwm->chip.ops = &atmel_pwm_ops; + if (pdev->dev.of_node) { + atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags; + atmel_pwm->chip.of_pwm_n_cells = 3; + atmel_pwm->chip.base = -1; + } else { + atmel_pwm->chip.base = pdev->id; + } + atmel_pwm->chip.npwm = 4; + + atmel_pwm->config = data->config; + + ret = pwmchip_add(&atmel_pwm->chip); + if (ret < 0) { + dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret); + goto unprepare_clk; + } + + platform_set_drvdata(pdev, atmel_pwm); + +unprepare_clk: + clk_unprepare(atmel_pwm->clk); + return ret; +} + +static int atmel_pwm_remove(struct platform_device *pdev) +{ + struct atmel_pwm_chip *atmel_pwm = platform_get_drvdata(pdev); + + clk_unprepare(atmel_pwm->clk); + + return pwmchip_remove(&atmel_pwm->chip); +} + +static struct platform_driver atmel_pwm_driver = { + .driver = { + .name = "atmel-pwm", + .of_match_table = of_match_ptr(atmel_pwm_dt_ids), + }, + .id_table = atmel_pwm_devtypes, + .probe = atmel_pwm_probe, + .remove = atmel_pwm_remove, +}; +module_platform_driver(atmel_pwm_driver); + +MODULE_ALIAS("platform:atmel-pwm"); +MODULE_AUTHOR("Bo Shen <voice.shen@atmel.com>"); +MODULE_DESCRIPTION("Atmel PWM driver"); +MODULE_LICENSE("GPL v2");