| Message ID | 1384220218-12716-2-git-send-email-Li.Xiubo@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 12, 2013 at 09:36:55AM +0800, Xiubo Li wrote: > The FTM PWM device can be found on Vybrid VF610 Tower and Layerscape LS-1 SoCs. > > Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com> > Signed-off-by: Alison Wang <b18965@freescale.com> > Signed-off-by: Jingchang Lu <b35083@freescale.com> > Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/pwm/Kconfig | 10 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-fsl-ftm.c | 402 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 413 insertions(+) > create mode 100644 drivers/pwm/pwm-fsl-ftm.c Sorry for taking so long. Overall this looks pretty good. A few minor comments below. > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c [...] > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/err.h> > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/pwm.h> > +#include <linux/of_address.h> Can you sort these alphabetically, please? > +#include <dt-bindings/clock/vf610-clock.h> This can stay separate, and I would even separate it from the others by a blank line. > +#define FTM_SC 0x00 > +#define FTMSC_CLK_MASK 0x03 Perhaps FTM_SC_CLK_MASK for consistency with the register name? Same for all others below. > +#define FTMSC_CLK_OFFSET 0x03 I think the more common suffix would be _SHIFT. _OFFSET is more typically used for registers, not bitfields. > +#define FTMSC_CLKSYS (0x01 << 3) > +#define FTMSC_CLKFIX (0x02 << 3) > +#define FTMSC_CLKEXT (0x03 << 3) Perhaps "<< 3" should be "<< FTM_SC_CLK_SHIFT"? And the names perhaps FTM_SC_CLK_{SYS,FIX,EXT}? That somehow "namespaces" all of them. > +#define FTMSC_PS_MASK 0x07 > +#define FTMSC_PS_OFFSET 0x00 > + > +#define FTM_CNT 0x04 > +#define FTM_MOD 0x08 > + > +#define FTM_CSC_BASE 0x0C > +#define FTM_CSC(_channel) (FTM_CSC_BASE + ((_channel) * 8)) > +#define FTMCnSC_MSB BIT(5) Same here: FTMCnSC_MSB isn't immediately obvious to be a bit within the FTM_CSC register. FTM_CSC_MSB is. > +#define FTM_CNTIN_VAL 0x00 Do we really need this? > +#define FTM_MAX_CHANNEL 8 This is used only once, so I prefer to just assign .npwm = 8 in the driver's .probe() implementation. > +static int fsl_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + int ret; > + struct fsl_pwm_chip *fpc; > + > + fpc = to_fsl_chip(chip); You could assign this when declaring the fpc variable to save two lines. > + ret = clk_prepare_enable(fpc->sys_clk); > + if (ret) > + return ret; > + > + return 0; You can save another few lines by making this simply: return clk_prepare_enable(fpc->sys_clk); > +static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct fsl_pwm_chip *fpc; > + > + fpc = to_fsl_chip(chip); Same comment here. > +static int fsl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) > +{ > + unsigned long period_cycles, duty_cycles; I don't think there's a need for the _cycles suffix here. > + unsigned long cntin = FTM_CNTIN_VAL; I don't understand why this is needed. FTM_CNTIN_VAL is defined as 0 and you never change cntin subsequently, so why not just use the literal 0 instead? > + struct fsl_pwm_chip *fpc; > + > + fpc = to_fsl_chip(chip); > + > + period_cycles = fsl_rate_to_cycles(fpc, period_ns); > + if (period_cycles > 0xFFFF) { > + dev_err(chip->dev, "required PWM period cycles(%lu) overflow " > + "16-bits counter!\n", period_cycles); > + return -EINVAL; > + } > + > + duty_cycles = fsl_rate_to_cycles(fpc, duty_ns); > + if (duty_cycles >= 0xFFFF) { > + dev_err(chip->dev, "required PWM duty cycles(%lu) overflow " > + "16-bits counter!\n", duty_cycles); > + return -EINVAL; > + } I'm not sure the error messages are all that useful. A -EINVAL error code should make it pretty clear what the problem is. > + writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc->base + FTM_CSC(pwm->hwpwm)); > + > + writel(0xF0, fpc->base + FTM_OUTMASK); > + writel(0x0F, fpc->base + FTM_OUTINIT); The purpose of this eludes me. These seem to be global (not specific to channel pwm->hwpwm) registers, so why are they updated whenever a single channel is reconfigured? > + writel(FTM_CNTIN_VAL, fpc->base + FTM_CNTIN); This could become simply: writel(0, fpc->base + FTM_CNTIN); > + writel(period_cycles + cntin - 1, fpc->base + FTM_MOD); > + writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm)); And these: writel(period - 1, fpc->base + FTM_MOD); writel(duty, fpc->base + FTM_CV(pwm->hwpwm)); Although now that I think about it, this seems broken. The period is set in a global register, so I assume it is valid for all channels. What if you want to use different periods for individual channels? The way I read this the last one to be configured will win and change the period to whatever it wants. Other channels won't even notice. Is there a way to set the period per channel? > +static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc) > +{ > + int ret; > + unsigned long reg; > + > + if (fpc->counter_clk_enable++) > + return 0; Are you sure this is safe? I think you'll need to use either an atomic or a mutex to lock this. > + ret = clk_prepare_enable(fpc->counter_clk); > + if (ret) > + return ret; In case clk_prepare_enable() fails, the counter_clk_enable will need to be decremented in order to track the state correctly, doesn't it? > + > + reg = readl(fpc->base + FTM_SC); > + reg &= ~((FTMSC_CLK_MASK << FTMSC_CLK_OFFSET) | > + (FTMSC_PS_MASK << FTMSC_PS_OFFSET)); > + /* select counter clock source */ There should be an empty line between the above two. > + switch (fpc->counter_clk_select) { > + case VF610_CLK_FTM0: > + reg |= FTMSC_CLKSYS; > + break; > + case VF610_CLK_FTM0_FIX_SEL: > + reg |= FTMSC_CLKFIX; > + break; > + case VF610_CLK_FTM0_EXT_SEL: > + reg |= FTMSC_CLKEXT; > + break; > + default: > + break; > + } > + reg |= fpc->clk_ps; And another one above this line. > + writel(reg, fpc->base + FTM_SC); I think with the proper locking in place what you should do is increment counter_clk_enable only here. That makes avoids having to decrement the count on error. Similarly in fsl_counter_clock_disable() you can postpone decrementing the count until the very end. > +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct fsl_pwm_chip *fpc; > + > + fpc = to_fsl_chip(chip); > + > + fsl_counter_clock_enable(fpc); This can fail. Should the error be propagated? > + > + return 0; > +} > + > +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct fsl_pwm_chip *fpc; > + > + fpc = to_fsl_chip(chip); > + > + fsl_counter_clock_disable(fpc); > +} Same here. Since you can't propagate the error, perhaps an error message would be appropriate here? Also for the locking above, perhaps a good solution would be to acquire the lock around the calls to fsl_counter_clock_{enable,disable}() so that they can safely assume that they are called with the lock held, which will make their implementation a lot simpler. So what you could do is this: static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) { struct fsl_pwm_chip *fpc = to_fsl_chip(chip); int ret; mutex_lock(&fpc->lock); ret = fsl_counter_clock_enable(fpc); mutex_unlock(&fpc->lock); return ret; } And analogously for fsl_pwm_disable(). > +static int fsl_pwm_calculate_ps(struct fsl_pwm_chip *fpc) > +{ > + unsigned long long sys_rate, counter_rate, ratio; > + > + sys_rate = clk_get_rate(fpc->sys_clk); > + if (!sys_rate) > + return -EINVAL; > + > + counter_rate = clk_get_rate(fpc->counter_clk); > + if (!counter_rate) { > + fpc->counter_clk = fpc->sys_clk; > + fpc->counter_clk_select = VF610_CLK_FTM0; > + dev_warn(fpc->chip.dev, > + "the counter source clock is a dummy clock, " > + "so select the system clock as default!\n"); > + } > + > + switch (fpc->counter_clk_select) { > + case VF610_CLK_FTM0_FIX_SEL: > + ratio = 2 * counter_rate - 1; > + do_div(ratio, sys_rate); > + fpc->clk_ps = ratio; > + break; > + case VF610_CLK_FTM0_EXT_SEL: > + ratio = 4 * counter_rate - 1; > + do_div(ratio, sys_rate); > + fpc->clk_ps = ratio; > + break; > + case VF610_CLK_FTM0: > + fpc->clk_ps = 7; Even though it doesn't matter here, you should still add a break. Otherwise if you ever modify the code in the default case, you don't have to remember to add it in then. > +static int fsl_pwm_parse_clk_ps(struct fsl_pwm_chip *fpc) > +{ > + int ret; > + struct of_phandle_args clkspec; > + struct device_node *np = fpc->chip.dev->of_node; > + > + fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm0"); > + if (IS_ERR(fpc->sys_clk)) { > + ret = PTR_ERR(fpc->sys_clk); > + dev_err(fpc->chip.dev, > + "failed to get \"ftm0\" clock %d\n", ret); > + return ret; > + } > + > + fpc->counter_clk = devm_clk_get(fpc->chip.dev, "ftm0_counter"); > + if (IS_ERR(fpc->counter_clk)) { > + ret = PTR_ERR(fpc->counter_clk); > + dev_err(fpc->chip.dev, > + "failed to get \"ftm0_counter\" clock %d\n", > + ret); > + return ret; > + } > + > + ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells", 1, > + &clkspec); > + if (ret) > + return ret; > + > + fpc->counter_clk_select = clkspec.args[0]; This isn't at all pretty. But given that once you have access to a struct clk there's no way to identify it, I don't know of a better alternative. > + > + return fsl_pwm_calculate_ps(fpc); > +} > + > +static int fsl_pwm_probe(struct platform_device *pdev) > +{ > + int ret = 0; There's no need to initialize this. > + struct fsl_pwm_chip *fpc; > + struct resource *res; > + > + fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL); > + if (!fpc) > + return -ENOMEM; > + > + fpc->chip.dev = &pdev->dev; > + fpc->counter_clk_enable = 0; You use kzalloc(), so this is already be zeroed out. > + ret = fsl_pwm_parse_clk_ps(fpc); > + if (ret < 0) > + return ret; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + fpc->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(fpc->base)) { > + ret = PTR_ERR(fpc->base); > + return ret; You can simply do "return PTR_ERR(fpc->base);" here. > + } > + > + fpc->chip.ops = &fsl_pwm_ops; > + fpc->chip.of_xlate = of_pwm_xlate_with_flags; > + fpc->chip.of_pwm_n_cells = 3; > + fpc->chip.base = -1; > + fpc->chip.npwm = FTM_MAX_CHANNEL; > + ret = pwmchip_add(&fpc->chip); There should be a blank linke between the above two. > +static int fsl_pwm_remove(struct platform_device *pdev) > +{ > + struct fsl_pwm_chip *fpc; > + > + fpc = platform_get_drvdata(pdev); These can go on one line. > +static const struct of_device_id fsl_pwm_dt_ids[] = { > + { .compatible = "fsl,vf610-ftm-pwm", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, fsl_pwm_dt_ids); > + > +static struct platform_driver fsl_pwm_driver = { > + .driver = { > + .name = "fsl-ftm-pwm", > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(fsl_pwm_dt_ids), No need for of_match_ptr() since you depend on OF. And while at it, you can drop the initialization of .owner as well, since the core takes care of initializing that nowadays. Thierry
On Thu, Nov 28, 2013 at 10:25:11PM +0100, Thierry Reding wrote: > On Tue, Nov 12, 2013 at 09:36:55AM +0800, Xiubo Li wrote: [...] > > +static int fsl_pwm_parse_clk_ps(struct fsl_pwm_chip *fpc) > > +{ > > + int ret; > > + struct of_phandle_args clkspec; > > + struct device_node *np = fpc->chip.dev->of_node; > > + > > + fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm0"); > > + if (IS_ERR(fpc->sys_clk)) { > > + ret = PTR_ERR(fpc->sys_clk); > > + dev_err(fpc->chip.dev, > > + "failed to get \"ftm0\" clock %d\n", ret); > > + return ret; > > + } > > + > > + fpc->counter_clk = devm_clk_get(fpc->chip.dev, "ftm0_counter"); > > + if (IS_ERR(fpc->counter_clk)) { > > + ret = PTR_ERR(fpc->counter_clk); > > + dev_err(fpc->chip.dev, > > + "failed to get \"ftm0_counter\" clock %d\n", > > + ret); > > + return ret; > > + } > > + > > + ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells", 1, > > + &clkspec); > > + if (ret) > > + return ret; > > + > > + fpc->counter_clk_select = clkspec.args[0]; > > This isn't at all pretty. But given that once you have access to a > struct clk there's no way to identify it, I don't know of a better > alternative. Hi Mike, I've seen this crop up a number of times now, to varying degrees of gravity. In this particular case, the driver needs to know the type of a clock because it needs to program this hardware differently depending on which clock feeds the counter. Since there is no way to obtain any kind of identifying information from a struct clk, drivers need to rely on hacks like this and manually reach into the device tree to obtain that information. I'm aware of similar cases which have been solved by using a mux clock that takes a set of parents and the .set_parent() operation is then used to program registers appropriately. However in all those cases, the mux clock (and the parents) have all been part of a single driver, so I am not sure if the same would be applicable here. Do you have any other suggestions on how this could possibly be solved? Thierry
Hi Thierry, Thanks for your detail comments. > > + switch (fpc->counter_clk_select) { > > + case VF610_CLK_FTM0: > > + reg |= FTMSC_CLKSYS; > > + break; > > + case VF610_CLK_FTM0_FIX_SEL: > > + reg |= FTMSC_CLKFIX; > > + break; > > + case VF610_CLK_FTM0_EXT_SEL: > > + reg |= FTMSC_CLKEXT; > > + break; > > + default: > > + break; > > + } > > + reg |= fpc->clk_ps; > > And another one above this line. > > > + writel(reg, fpc->base + FTM_SC); > > I think with the proper locking in place what you should do is increment > counter_clk_enable only here. That makes avoids having to decrement the > count on error. > > Similarly in fsl_counter_clock_disable() you can postpone decrementing > the count until the very end. > As the other mails we have talked about this that there are 8 channels supported, but they share the same counter clock source. So we need to make sure that when one channel is calling fsl_counter_clock_disable() it shouldn't disable the counter clock if any other channel is still enabled. Similary in fsl_counter clock_enable(). This is why I set counter_clk_enable property here. -- Best Regards,
> > +#define FTM_CNTIN_VAL 0x00 > > Do we really need this? > Maybe not, I think that the initial value maybe modified in the future. And this can be more easy to ajust it. > > + period_cycles = fsl_rate_to_cycles(fpc, period_ns); > > + if (period_cycles > 0xFFFF) { > > + dev_err(chip->dev, "required PWM period cycles(%lu) overflow " > > + "16-bits counter!\n", period_cycles); > > + return -EINVAL; > > + } > > + > > + duty_cycles = fsl_rate_to_cycles(fpc, duty_ns); > > + if (duty_cycles >= 0xFFFF) { > > + dev_err(chip->dev, "required PWM duty cycles(%lu) overflow " > > + "16-bits counter!\n", duty_cycles); > > + return -EINVAL; > > + } > > I'm not sure the error messages are all that useful. A -EINVAL error > code should make it pretty clear what the problem is. > Yes, these could be absent. > > + writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc->base + FTM_CSC(pwm->hwpwm)); > > + > > + writel(0xF0, fpc->base + FTM_OUTMASK); > > + writel(0x0F, fpc->base + FTM_OUTINIT); > > The purpose of this eludes me. These seem to be global (not specific to > channel pwm->hwpwm) registers, so why are they updated whenever a single > channel is reconfigured? > Well, certainly there has one way to update per channel's configuration about this. I will revise it then. > > + writel(period_cycles + cntin - 1, fpc->base + FTM_MOD); > > + writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm)); > > And these: > > writel(period - 1, fpc->base + FTM_MOD); > writel(duty, fpc->base + FTM_CV(pwm->hwpwm)); > > Although now that I think about it, this seems broken. The period is set > in a global register, so I assume it is valid for all channels. What if > you want to use different periods for individual channels? The way I > read this the last one to be configured will win and change the period > to whatever it wants. Other channels won't even notice. > That's right. And all the 8 channels share the same period settings. > Is there a way to set the period per channel? > Not yet. Only could we do is to set the duty value individually for each channel. So here is a limitation for the cusumers that all the 8 channels' period values should be the same. > > +static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc) > > +{ > > + int ret; > > + unsigned long reg; > > + > > + if (fpc->counter_clk_enable++) > > + return 0; > > Are you sure this is safe? I think you'll need to use either an atomic > or a mutex to lock this. > Maybe a mutex lock is a good choice. > > + ret = clk_prepare_enable(fpc->counter_clk); > > + if (ret) > > + return ret; > > In case clk_prepare_enable() fails, the counter_clk_enable will need to > be decremented in order to track the state correctly, doesn't it? > Yes, it should be. > > +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct fsl_pwm_chip *fpc; > > + > > + fpc = to_fsl_chip(chip); > > + > > + fsl_counter_clock_enable(fpc); > > This can fail. Should the error be propagated? > That's better. > > +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device > *pwm) > > +{ > > + struct fsl_pwm_chip *fpc; > > + > > + fpc = to_fsl_chip(chip); > > + > > + fsl_counter_clock_disable(fpc); > > +} > > Same here. Since you can't propagate the error, perhaps an error message > would be appropriate here? > Well, in fsl_counter_clock_disable(fpc); only '0' could be returned, maybe let it a void type value to return is better. Just like: static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc) {} > Also for the locking above, perhaps a good solution would be to acquire > the lock around the calls to fsl_counter_clock_{enable,disable}() so > that they can safely assume that they are called with the lock held, > which will make their implementation a lot simpler. > > So what you could do is this: > > static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device > *pwm) > { > struct fsl_pwm_chip *fpc = to_fsl_chip(chip); > int ret; > > mutex_lock(&fpc->lock); > ret = fsl_counter_clock_enable(fpc); > mutex_unlock(&fpc->lock); > > return ret; > } > > And analogously for fsl_pwm_disable(). > I will think about this. > > > +static int fsl_pwm_calculate_ps(struct fsl_pwm_chip *fpc) > > +{ > > + unsigned long long sys_rate, counter_rate, ratio; > > + > > + sys_rate = clk_get_rate(fpc->sys_clk); > > + if (!sys_rate) > > + return -EINVAL; > > + > > + counter_rate = clk_get_rate(fpc->counter_clk); > > + if (!counter_rate) { > > + fpc->counter_clk = fpc->sys_clk; > > + fpc->counter_clk_select = VF610_CLK_FTM0; > > + dev_warn(fpc->chip.dev, > > + "the counter source clock is a dummy clock, " > > + "so select the system clock as default!\n"); > > + } > > + > > + switch (fpc->counter_clk_select) { > > + case VF610_CLK_FTM0_FIX_SEL: > > + ratio = 2 * counter_rate - 1; > > + do_div(ratio, sys_rate); > > + fpc->clk_ps = ratio; > > + break; > > + case VF610_CLK_FTM0_EXT_SEL: > > + ratio = 4 * counter_rate - 1; > > + do_div(ratio, sys_rate); > > + fpc->clk_ps = ratio; > > + break; > > + case VF610_CLK_FTM0: > > + fpc->clk_ps = 7; > > Even though it doesn't matter here, you should still add a break. > Otherwise if you ever modify the code in the default case, you don't > have to remember to add it in then. > You are right, adding one break is much safer. -- Best Rwgards,
On Fri, Nov 29, 2013 at 06:42:06AM +0000, Li Xiubo wrote: > > > +#define FTM_CNTIN_VAL 0x00 > > > > Do we really need this? > > > > Maybe not, I think that the initial value maybe modified in the future. > And this can be more easy to ajust it. Why would it need to be modified? > > > + writel(period_cycles + cntin - 1, fpc->base + FTM_MOD); > > > + writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm)); > > > > And these: > > > > writel(period - 1, fpc->base + FTM_MOD); > > writel(duty, fpc->base + FTM_CV(pwm->hwpwm)); > > > > Although now that I think about it, this seems broken. The period is set > > in a global register, so I assume it is valid for all channels. What if > > you want to use different periods for individual channels? The way I > > read this the last one to be configured will win and change the period > > to whatever it wants. Other channels won't even notice. > > > > That's right. And all the 8 channels share the same period settings. > > > Is there a way to set the period per channel? > > > > Not yet. Only could we do is to set the duty value individually for each > channel. So here is a limitation for the cusumers that all the 8 channels' > period values should be the same. That will need to be handled some way. Perhaps the easiest would be to check whether or not another channel is already running and check that indeed the period as requested by the new channel matches that of any running channels. If it doesn't match, then pwm_config() should fail. When you do that you can also optimize this a bit by only setting the frequency once. Whenever a new channel is configured it will have the same period and therefore the register doesn't need to be reprogrammed. > > > +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device > > *pwm) > > > +{ > > > + struct fsl_pwm_chip *fpc; > > > + > > > + fpc = to_fsl_chip(chip); > > > + > > > + fsl_counter_clock_disable(fpc); > > > +} > > > > Same here. Since you can't propagate the error, perhaps an error message > > would be appropriate here? > > > > Well, in fsl_counter_clock_disable(fpc); only '0' could be returned, maybe > let it a void type value to return is better. > Just like: > static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc) {} Yes, that sounds good. Thierry
> > > > +#define FTM_CNTIN_VAL 0x00 > > > > > > Do we really need this? > > > > > > > Maybe not, I think that the initial value maybe modified in the future. > > And this can be more easy to ajust it. > > Why would it need to be modified? > Well, for the PWM function modifying it make no sense, so I'll remove this. > > > > + writel(period_cycles + cntin - 1, fpc->base + FTM_MOD); > > > > + writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm)); > > > > > > And these: > > > > > > writel(period - 1, fpc->base + FTM_MOD); > > > writel(duty, fpc->base + FTM_CV(pwm->hwpwm)); > > > > > > Although now that I think about it, this seems broken. The period is > > > set in a global register, so I assume it is valid for all channels. > > > What if you want to use different periods for individual channels? > > > The way I read this the last one to be configured will win and > > > change the period to whatever it wants. Other channels won't even > notice. > > > > > > > That's right. And all the 8 channels share the same period settings. > > > > > Is there a way to set the period per channel? > > > > > > > Not yet. Only could we do is to set the duty value individually for > > each channel. So here is a limitation for the cusumers that all the 8 > channels' > > period values should be the same. > > That will need to be handled some way. Perhaps the easiest would be to > check whether or not another channel is already running and check that > indeed the period as requested by the new channel matches that of any > running channels. If it doesn't match, then pwm_config() should fail. > > When you do that you can also optimize this a bit by only setting the > frequency once. Whenever a new channel is configured it will have the same > period and therefore the register doesn't need to be reprogrammed. > Yes, if it dosen't match it should fail, or nothing to do with the period register if it's not the first channel to be configured. > > > > +static void fsl_pwm_disable(struct pwm_chip *chip, struct > > > > +pwm_device > > > *pwm) > > > > +{ > > > > + struct fsl_pwm_chip *fpc; > > > > + > > > > + fpc = to_fsl_chip(chip); > > > > + > > > > + fsl_counter_clock_disable(fpc); > > > > +} > > > > > > Same here. Since you can't propagate the error, perhaps an error > > > message would be appropriate here? > > > > > > > Well, in fsl_counter_clock_disable(fpc); only '0' could be returned, > > maybe let it a void type value to return is better. > > Just like: > > static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc)
Hi, On Thu, Nov 28, 2013 at 10:37:19PM +0000, Thierry Reding wrote: > On Thu, Nov 28, 2013 at 10:25:11PM +0100, Thierry Reding wrote: > > On Tue, Nov 12, 2013 at 09:36:55AM +0800, Xiubo Li wrote: > [...] > > > +static int fsl_pwm_parse_clk_ps(struct fsl_pwm_chip *fpc) > > > +{ > > > + int ret; > > > + struct of_phandle_args clkspec; > > > + struct device_node *np = fpc->chip.dev->of_node; > > > + > > > + fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm0"); > > > + if (IS_ERR(fpc->sys_clk)) { > > > + ret = PTR_ERR(fpc->sys_clk); > > > + dev_err(fpc->chip.dev, > > > + "failed to get \"ftm0\" clock %d\n", ret); > > > + return ret; > > > + } > > > + > > > + fpc->counter_clk = devm_clk_get(fpc->chip.dev, "ftm0_counter"); > > > + if (IS_ERR(fpc->counter_clk)) { > > > + ret = PTR_ERR(fpc->counter_clk); > > > + dev_err(fpc->chip.dev, > > > + "failed to get \"ftm0_counter\" clock %d\n", > > > + ret); > > > + return ret; > > > + } > > > + > > > + ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells", 1, > > > + &clkspec); > > > + if (ret) > > > + return ret; > > > + > > > + fpc->counter_clk_select = clkspec.args[0]; > > > > This isn't at all pretty. But given that once you have access to a > > struct clk there's no way to identify it, I don't know of a better > > alternative. > > Hi Mike, > > I've seen this crop up a number of times now, to varying degrees of > gravity. In this particular case, the driver needs to know the type of a > clock because it needs to program this hardware differently depending on > which clock feeds the counter. Since there is no way to obtain any kind > of identifying information from a struct clk, drivers need to rely on > hacks like this and manually reach into the device tree to obtain that > information. Which property of the clock is the consumer concerned with in this case? From a quick look at the driver it looks like there are actually a number of different input lines to the device that share the clock-name "ftm0_counter", though they are actually separate and each has a different divider. Have I got that right? If that's the case, having a unique clock-names value for each of those lines would be the solution I'd expect. Then you just have to list the one(s) that are wired up, and the driver can figure out the appropriate line to use either by requesting by name until it finds a match or inspecting the clock-names property. Is there some other property of the parent that we care about here? > > I'm aware of similar cases which have been solved by using a mux clock > that takes a set of parents and the .set_parent() operation is then used > to program registers appropriately. However in all those cases, the mux > clock (and the parents) have all been part of a single driver, so I am > not sure if the same would be applicable here. > > Do you have any other suggestions on how this could possibly be solved? This also feels like the case I describe above. What am I missing? :) Thanks, Mark.
> > > > +static int fsl_pwm_parse_clk_ps(struct fsl_pwm_chip *fpc) > > > > +{ > > > > + int ret; > > > > + struct of_phandle_args clkspec; > > > > + struct device_node *np = fpc->chip.dev->of_node; > > > > + > > > > + fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm0"); > > > > + if (IS_ERR(fpc->sys_clk)) { > > > > + ret = PTR_ERR(fpc->sys_clk); > > > > + dev_err(fpc->chip.dev, > > > > + "failed to get \"ftm0\" clock %d\n", ret); > > > > + return ret; > > > > + } > > > > + > > > > + fpc->counter_clk = devm_clk_get(fpc->chip.dev, "ftm0_counter"); > > > > + if (IS_ERR(fpc->counter_clk)) { > > > > + ret = PTR_ERR(fpc->counter_clk); > > > > + dev_err(fpc->chip.dev, > > > > + "failed to get \"ftm0_counter\" clock %d\n", > > > > + ret); > > > > + return ret; > > > > + } > > > > + > > > > + ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells", > 1, > > > > + &clkspec); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + fpc->counter_clk_select = clkspec.args[0]; > > > > > > This isn't at all pretty. But given that once you have access to a > > > struct clk there's no way to identify it, I don't know of a better > > > alternative. > > > > Hi Mike, > > > > I've seen this crop up a number of times now, to varying degrees of > > gravity. In this particular case, the driver needs to know the type of a > > clock because it needs to program this hardware differently depending on > > which clock feeds the counter. Since there is no way to obtain any kind > > of identifying information from a struct clk, drivers need to rely on > > hacks like this and manually reach into the device tree to obtain that > > information. > > Which property of the clock is the consumer concerned with in this case? > The "ftm0_counter" clock. > From a quick look at the driver it looks like there are actually a > number of different input lines to the device that share the clock-name > "ftm0_counter", though they are actually separate and each has a > different divider. Have I got that right? > Yes mostly, there are three different input lines wired to the counter clock, but they share only one divider. And between the three lines and the divider there is one mux inside of the FTM IP block. The three clock sources are: "ftm0", "ftm0-fix", "ftm0-ext". _____________________________________________________ | | | +++++++++++ FTM Module | ftm0 -------|-->+ + | | + + +++++++++++++ ++++++++++++++ | ftm0-fix ---|-->+ MUX +---->+ divider +---->+ counter + | | + + +++++++++++++ ++++++++++++++ | ftm0-ext ---|-->+ + | | +++++++++++ | |_____________________________________________________| > If that's the case, having a unique clock-names value for each of those > lines would be the solution I'd expect. Then you just have to list the > one(s) that are wired up, and the driver can figure out the appropriate > line to use either by requesting by name until it finds a match or > inspecting the clock-names property. > Hi Kumar, In the list archives for mails of "[RFC][PATCHv5 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.",we have discussed about this. Is there any different with your suggestions or ideas? > Is there some other property of the parent that we care about here? > As I know, not yet. -- Best Regards,
> > + struct fsl_pwm_chip *fpc; > > + > > + fpc = to_fsl_chip(chip); > > + > > + period_cycles = fsl_rate_to_cycles(fpc, period_ns); > > + if (period_cycles > 0xFFFF) { > > + dev_err(chip->dev, "required PWM period cycles(%lu) overflow " > > + "16-bits counter!\n", period_cycles); > > + return -EINVAL; > > + } > > + > > + duty_cycles = fsl_rate_to_cycles(fpc, duty_ns); > > + if (duty_cycles >= 0xFFFF) { > > + dev_err(chip->dev, "required PWM duty cycles(%lu) overflow " > > + "16-bits counter!\n", duty_cycles); > > + return -EINVAL; > > + } > > I'm not sure the error messages are all that useful. A -EINVAL error code > should make it pretty clear what the problem is. > Yes, it is. But for the pwm leds, there hasn't any check about the return values, if there is something wrong, we cannot get any information about this. So I think this error messages are useful for this case. -- Xiubo
On 11 Nov 2013, Li.Xiubo at freescale.com wrote: > +static int fsl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) > +{ > + unsigned long period_cycles, duty_cycles; > + unsigned long cntin = FTM_CNTIN_VAL; > + struct fsl_pwm_chip *fpc; > + > + fpc = to_fsl_chip(chip); > + > + period_cycles = fsl_rate_to_cycles(fpc, period_ns); > + if (period_cycles > 0xFFFF) { > + dev_err(chip->dev, "required PWM period cycles(%lu) overflow " > + "16-bits counter!\n", period_cycles); > + return -EINVAL; > + } > + > + duty_cycles = fsl_rate_to_cycles(fpc, duty_ns); > + if (duty_cycles >= 0xFFFF) { > + dev_err(chip->dev, "required PWM duty cycles(%lu) overflow " > + "16-bits counter!\n", duty_cycles); > + return -EINVAL; > + } > + > + writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc->base + FTM_CSC(pwm->hwpwm)); > + > + writel(0xF0, fpc->base + FTM_OUTMASK); > + writel(0x0F, fpc->base + FTM_OUTINIT); > + writel(FTM_CNTIN_VAL, fpc->base + FTM_CNTIN); > + > + writel(period_cycles + cntin - 1, fpc->base + FTM_MOD); > + writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm)); > + > + return 0; > +} Even though the module can support eight channels, the FTM_MOD is global and is updated in each 'config'. So, if we have two PWMs, and both call 'config' with different periods, the last one to call will win? There doesn't seem to be any locking/inspection of 'MOD'. I think this is global to the FTM, while 'FTM_CV()' is per channel. So we may have 8 PWMs, all with the same period, but with different duty cycles. Is this correct? Or are we only supporting one channel per FTM/PWM? Thanks, Bill Pringlemeir.
> > +static int fsl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > > + int duty_ns, int period_ns) > > +{ > > + unsigned long period_cycles, duty_cycles; > > + unsigned long cntin = FTM_CNTIN_VAL; > > + struct fsl_pwm_chip *fpc; > > + > > + fpc = to_fsl_chip(chip); > > + > > + period_cycles = fsl_rate_to_cycles(fpc, period_ns); > > + if (period_cycles > 0xFFFF) { > > + dev_err(chip->dev, "required PWM period cycles(%lu) overflow " > > + "16-bits counter!\n", period_cycles); > > + return -EINVAL; > > + } > > + > > + duty_cycles = fsl_rate_to_cycles(fpc, duty_ns); > > + if (duty_cycles >= 0xFFFF) { > > + dev_err(chip->dev, "required PWM duty cycles(%lu) overflow " > > + "16-bits counter!\n", duty_cycles); > > + return -EINVAL; > > + } > > + > > + writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc->base + FTM_CSC(pwm->hwpwm)); > > + > > + writel(0xF0, fpc->base + FTM_OUTMASK); > > + writel(0x0F, fpc->base + FTM_OUTINIT); > > + writel(FTM_CNTIN_VAL, fpc->base + FTM_CNTIN); > > + > > + writel(period_cycles + cntin - 1, fpc->base + FTM_MOD); > > + writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm)); > > + > > + return 0; > > +} > > Even though the module can support eight channels, the FTM_MOD is global and > is updated in each 'config'. So, if we have two PWMs, and both call 'config' > with different periods, the last one to call will win? There doesn't seem to > be any locking/inspection of 'MOD'. I think this is global to the FTM, while > 'FTM_CV()' is per channel. > > So we may have 8 PWMs, all with the same period, but with different duty > cycles. Is this correct? Or are we only supporting one channel per FTM/PWM? > Yes, that's right. And in "[PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support" patch, this has been fixed. Best Regards, Xiubo
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 75840b5..8144fb0 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -62,6 +62,16 @@ config PWM_BFIN To compile this driver as a module, choose M here: the module will be called pwm-bfin. +config PWM_FSL_FTM + tristate "Freescale FTM PWM support" + depends on OF + help + Generic FTM PWM framework driver for Freescale VF610 and + Layerscape LS-1 SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-fsl-ftm. + config PWM_IMX tristate "i.MX PWM support" depends on ARCH_MXC diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 77a8c18..f383784 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -3,6 +3,7 @@ obj-$(CONFIG_PWM_SYSFS) += sysfs.o obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o +obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o obj-$(CONFIG_PWM_IMX) += pwm-imx.o obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c new file mode 100644 index 0000000..2420ce0 --- /dev/null +++ b/drivers/pwm/pwm-fsl-ftm.c @@ -0,0 +1,402 @@ +/* + * Freescale FTM PWM Driver + * + * Copyright 2012-2013 Freescale Semiconductor, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/err.h> +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/pwm.h> +#include <linux/of_address.h> +#include <dt-bindings/clock/vf610-clock.h> + +#define FTM_SC 0x00 +#define FTMSC_CLK_MASK 0x03 +#define FTMSC_CLK_OFFSET 0x03 +#define FTMSC_CLKSYS (0x01 << 3) +#define FTMSC_CLKFIX (0x02 << 3) +#define FTMSC_CLKEXT (0x03 << 3) +#define FTMSC_PS_MASK 0x07 +#define FTMSC_PS_OFFSET 0x00 + +#define FTM_CNT 0x04 +#define FTM_MOD 0x08 + +#define FTM_CSC_BASE 0x0C +#define FTM_CSC(_channel) (FTM_CSC_BASE + ((_channel) * 8)) +#define FTMCnSC_MSB BIT(5) +#define FTMCnSC_MSA BIT(4) +#define FTMCnSC_ELSB BIT(3) +#define FTMCnSC_ELSA BIT(2) + +#define FTM_CV_BASE 0x10 +#define FTM_CV(_channel) (FTM_CV_BASE + ((_channel) * 8)) + +#define FTM_CNTIN 0x4C +#define FTM_STATUS 0x50 + +#define FTM_MODE 0x54 +#define FTMMODE_FTMEN BIT(0) +#define FTMMODE_INIT BIT(2) +#define FTMMODE_PWMSYNC BIT(3) + +#define FTM_SYNC 0x58 +#define FTM_OUTINIT 0x5C +#define FTM_OUTMASK 0x60 +#define FTM_COMBINE 0x64 +#define FTM_DEADTIME 0x68 +#define FTM_EXTTRIG 0x6C +#define FTM_POL 0x70 +#define FTM_FMS 0x74 +#define FTM_FILTER 0x78 +#define FTM_FLTCTRL 0x7C +#define FTM_QDCTRL 0x80 +#define FTM_CONF 0x84 +#define FTM_FLTPOL 0x88 +#define FTM_SYNCONF 0x8C +#define FTM_INVCTRL 0x90 +#define FTM_SWOCTRL 0x94 +#define FTM_PWMLOAD 0x98 + +#define FTM_CNTIN_VAL 0x00 +#define FTM_MAX_CHANNEL 8 + +struct fsl_pwm_chip { + struct pwm_chip chip; + + struct clk *sys_clk; + struct clk *counter_clk; + unsigned int counter_clk_select; + unsigned int counter_clk_enable; + unsigned int clk_ps; + + void __iomem *base; +}; + +static inline struct fsl_pwm_chip *to_fsl_chip(struct pwm_chip *chip) +{ + return container_of(chip, struct fsl_pwm_chip, chip); +} + +static int fsl_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) +{ + int ret; + struct fsl_pwm_chip *fpc; + + fpc = to_fsl_chip(chip); + + ret = clk_prepare_enable(fpc->sys_clk); + if (ret) + return ret; + + return 0; +} + +static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct fsl_pwm_chip *fpc; + + fpc = to_fsl_chip(chip); + + clk_disable_unprepare(fpc->sys_clk); +} + +static unsigned long fsl_rate_to_cycles(struct fsl_pwm_chip *fpc, + unsigned long time_ns) +{ + unsigned long long c; + unsigned long ps = 1 << fpc->clk_ps; + + c = clk_get_rate(fpc->counter_clk); + c = c * time_ns; + do_div(c, 1000000000UL); + do_div(c, ps); + + return (unsigned long)c; +} + +static int fsl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + unsigned long period_cycles, duty_cycles; + unsigned long cntin = FTM_CNTIN_VAL; + struct fsl_pwm_chip *fpc; + + fpc = to_fsl_chip(chip); + + period_cycles = fsl_rate_to_cycles(fpc, period_ns); + if (period_cycles > 0xFFFF) { + dev_err(chip->dev, "required PWM period cycles(%lu) overflow " + "16-bits counter!\n", period_cycles); + return -EINVAL; + } + + duty_cycles = fsl_rate_to_cycles(fpc, duty_ns); + if (duty_cycles >= 0xFFFF) { + dev_err(chip->dev, "required PWM duty cycles(%lu) overflow " + "16-bits counter!\n", duty_cycles); + return -EINVAL; + } + + writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc->base + FTM_CSC(pwm->hwpwm)); + + writel(0xF0, fpc->base + FTM_OUTMASK); + writel(0x0F, fpc->base + FTM_OUTINIT); + writel(FTM_CNTIN_VAL, fpc->base + FTM_CNTIN); + + writel(period_cycles + cntin - 1, fpc->base + FTM_MOD); + writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm)); + + return 0; +} + +static int fsl_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, + enum pwm_polarity polarity) +{ + unsigned long reg; + struct fsl_pwm_chip *fpc; + + fpc = to_fsl_chip(chip); + + reg = readl(fpc->base + FTM_POL); + if (polarity == PWM_POLARITY_INVERSED) + reg |= BIT(pwm->hwpwm); + else + reg &= ~BIT(pwm->hwpwm); + writel(reg, fpc->base + FTM_POL); + + return 0; +} + +static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc) +{ + int ret; + unsigned long reg; + + if (fpc->counter_clk_enable++) + return 0; + + ret = clk_prepare_enable(fpc->counter_clk); + if (ret) + return ret; + + reg = readl(fpc->base + FTM_SC); + reg &= ~((FTMSC_CLK_MASK << FTMSC_CLK_OFFSET) | + (FTMSC_PS_MASK << FTMSC_PS_OFFSET)); + /* select counter clock source */ + switch (fpc->counter_clk_select) { + case VF610_CLK_FTM0: + reg |= FTMSC_CLKSYS; + break; + case VF610_CLK_FTM0_FIX_SEL: + reg |= FTMSC_CLKFIX; + break; + case VF610_CLK_FTM0_EXT_SEL: + reg |= FTMSC_CLKEXT; + break; + default: + break; + } + reg |= fpc->clk_ps; + writel(reg, fpc->base + FTM_SC); + + return 0; +} + +static int fsl_counter_clock_disable(struct fsl_pwm_chip *fpc) +{ + unsigned long reg; + + if (--fpc->counter_clk_enable) + return 0; + + writel(0xFF, fpc->base + FTM_OUTMASK); + reg = readl(fpc->base + FTM_SC); + reg &= ~(FTMSC_CLK_MASK << FTMSC_CLK_OFFSET); + writel(reg, fpc->base + FTM_SC); + + clk_disable_unprepare(fpc->counter_clk); + + return 0; +} + +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct fsl_pwm_chip *fpc; + + fpc = to_fsl_chip(chip); + + fsl_counter_clock_enable(fpc); + + return 0; +} + +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct fsl_pwm_chip *fpc; + + fpc = to_fsl_chip(chip); + + fsl_counter_clock_disable(fpc); +} + +static const struct pwm_ops fsl_pwm_ops = { + .request = fsl_pwm_request, + .free = fsl_pwm_free, + .config = fsl_pwm_config, + .set_polarity = fsl_pwm_set_polarity, + .enable = fsl_pwm_enable, + .disable = fsl_pwm_disable, + .owner = THIS_MODULE, +}; + +static int fsl_pwm_calculate_ps(struct fsl_pwm_chip *fpc) +{ + unsigned long long sys_rate, counter_rate, ratio; + + sys_rate = clk_get_rate(fpc->sys_clk); + if (!sys_rate) + return -EINVAL; + + counter_rate = clk_get_rate(fpc->counter_clk); + if (!counter_rate) { + fpc->counter_clk = fpc->sys_clk; + fpc->counter_clk_select = VF610_CLK_FTM0; + dev_warn(fpc->chip.dev, + "the counter source clock is a dummy clock, " + "so select the system clock as default!\n"); + } + + switch (fpc->counter_clk_select) { + case VF610_CLK_FTM0_FIX_SEL: + ratio = 2 * counter_rate - 1; + do_div(ratio, sys_rate); + fpc->clk_ps = ratio; + break; + case VF610_CLK_FTM0_EXT_SEL: + ratio = 4 * counter_rate - 1; + do_div(ratio, sys_rate); + fpc->clk_ps = ratio; + break; + case VF610_CLK_FTM0: + fpc->clk_ps = 7; + default: + break; + } + + return 0; +} + +static int fsl_pwm_parse_clk_ps(struct fsl_pwm_chip *fpc) +{ + int ret; + struct of_phandle_args clkspec; + struct device_node *np = fpc->chip.dev->of_node; + + fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm0"); + if (IS_ERR(fpc->sys_clk)) { + ret = PTR_ERR(fpc->sys_clk); + dev_err(fpc->chip.dev, + "failed to get \"ftm0\" clock %d\n", ret); + return ret; + } + + fpc->counter_clk = devm_clk_get(fpc->chip.dev, "ftm0_counter"); + if (IS_ERR(fpc->counter_clk)) { + ret = PTR_ERR(fpc->counter_clk); + dev_err(fpc->chip.dev, + "failed to get \"ftm0_counter\" clock %d\n", + ret); + return ret; + } + + ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells", 1, + &clkspec); + if (ret) + return ret; + + fpc->counter_clk_select = clkspec.args[0]; + + return fsl_pwm_calculate_ps(fpc); +} + +static int fsl_pwm_probe(struct platform_device *pdev) +{ + int ret = 0; + struct fsl_pwm_chip *fpc; + struct resource *res; + + fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL); + if (!fpc) + return -ENOMEM; + + fpc->chip.dev = &pdev->dev; + fpc->counter_clk_enable = 0; + + ret = fsl_pwm_parse_clk_ps(fpc); + if (ret < 0) + return ret; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + fpc->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(fpc->base)) { + ret = PTR_ERR(fpc->base); + return ret; + } + + fpc->chip.ops = &fsl_pwm_ops; + fpc->chip.of_xlate = of_pwm_xlate_with_flags; + fpc->chip.of_pwm_n_cells = 3; + fpc->chip.base = -1; + fpc->chip.npwm = FTM_MAX_CHANNEL; + ret = pwmchip_add(&fpc->chip); + if (ret < 0) { + dev_err(&pdev->dev, "failed to add PWM chip %d\n", ret); + return ret; + } + + platform_set_drvdata(pdev, fpc); + + return 0; +} + +static int fsl_pwm_remove(struct platform_device *pdev) +{ + struct fsl_pwm_chip *fpc; + + fpc = platform_get_drvdata(pdev); + + return pwmchip_remove(&fpc->chip); +} + +static const struct of_device_id fsl_pwm_dt_ids[] = { + { .compatible = "fsl,vf610-ftm-pwm", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, fsl_pwm_dt_ids); + +static struct platform_driver fsl_pwm_driver = { + .driver = { + .name = "fsl-ftm-pwm", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(fsl_pwm_dt_ids), + }, + .probe = fsl_pwm_probe, + .remove = fsl_pwm_remove, +}; +module_platform_driver(fsl_pwm_driver); + +MODULE_DESCRIPTION("Freescale FTM PWM Driver"); +MODULE_AUTHOR("Xiubo Li <Li.Xiubo@freescale.com>"); +MODULE_ALIAS("platform:fsl-ftm-pwm"); +MODULE_LICENSE("GPL");