| Message ID | 1384800901-21711-3-git-send-email-tim.kryger@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 18, 2013 at 10:54:58AM -0800, Tim Kryger wrote: [...] > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig [...] > +config PWM_BCM_KONA > + tristate "Kona PWM support" > + depends on ARCH_BCM_MOBILE > + default y Why do you want this to be the default? Shouldn't this be something that a default configuration selects explicitly? > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile [...] > obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o > +obj-$(CONFIG_PWM_BCM_KONA) += pwm-bcm-kona.o 'C' < 'F' > diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c [...] > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/ioport.h> > +#include <linux/math64.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/slab.h> > +#include <linux/types.h> > + > +#define KONA_PWM_CHANNEL_CNT 6 You use this exactly once, so there's no need for this define. > +#define PWM_CONTROL_OFFSET (0x00000000) I'd prefer if you dropped the _OFFSET suffix here. > +#define PWM_CONTROL_INITIAL (0x3f3f3f00) Can this not be expressed as a bitmask of values for the individual fields. > +#define PWMOUT_POLARITY(chan) (0x1 << (8 + chan)) This seems to only account for bits 8-13, what about the others? > +#define PWMOUT_ENABLE(chan) (0x1 << chan) Well, this accounts for bits 0-5, that still leaves 16-21 and 24-29. Also perhaps PWMOUT_POLARITY and PWMOUT_ENABLE should be defined as PWM_CONTROL_POLARITY and PWM_CONTROL_ENABLE. That makes it easy to see which register they are related to. > +#define PRESCALE_OFFSET (0x00000004) > +#define PRESCALE_SHIFT(chan) (chan << 2) I'm confused. This allocates 2 bits for each channel... > +#define PRESCALE_MASK(chan) (~(0x7 << (chan << 2))) > +#define PRESCALE_MIN (0x00000000) > +#define PRESCALE_MAX (0x00000007) ... but 0x7 requires at least 3 bits. > +#define PERIOD_COUNT_OFFSET(chan) (0x00000008 + (chan << 3)) > +#define PERIOD_COUNT_MIN (0x00000002) > +#define PERIOD_COUNT_MAX (0x00ffffff) Why PERIOD_COUNT? PERIOD is descriptive enough. Or is this the name as found in the manual? > +#define DUTY_CYCLE_HIGH_OFFSET(chan) (0x0000000c + (chan << 3)) > +#define DUTY_CYCLE_HIGH_MIN (0x00000000) > +#define DUTY_CYCLE_HIGH_MAX (0x00ffffff) By definition the duty-cycle is where the signal is high. Again, if this is how the manual names the registers it's fine. > +struct kona_pwmc { > + struct pwm_chip chip; > + void __iomem *base; > + struct clk *clk; > +}; > + > +static void kona_pwmc_apply_settings(struct kona_pwmc *kp, int chan) unsigned int for "chan"? > +{ > + /* New settings take effect on rising edge of enable bit */ > + writel(readl(kp->base + PWM_CONTROL_OFFSET) & ~PWMOUT_ENABLE(chan), > + kp->base + PWM_CONTROL_OFFSET); > + writel(readl(kp->base + PWM_CONTROL_OFFSET) | PWMOUT_ENABLE(chan), > + kp->base + PWM_CONTROL_OFFSET); That's too cluttered for my taste. Please make this more explicit: value = readl(...); value &= ~...; writel(value, ...); value = readl(...); value |= ...; writel(value, ...); > +} > + > +static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) > +{ > + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); > + u64 val, div, clk_rate; > + unsigned long prescale = PRESCALE_MIN, pc, dc; > + int chan = pwm->hwpwm; pwm->hwpwm is unsigned, so chan should be as well. > + > + /* > + * Find period count, duty count and prescale to suit duty_ns and > + * period_ns. This is done according to formulas described below: > + * > + * period_ns = 10^9 * (PRESCALE + 1) * PC / PWM_CLK_RATE > + * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE > + * > + * PC = (PWM_CLK_RATE * period_ns) / (10^9 * (PRESCALE + 1)) > + * DC = (PWM_CLK_RATE * duty_ns) / (10^9 * (PRESCALE + 1)) > + */ > + > + clk_rate = clk_get_rate(kp->clk); > + while (1) { Newline between the above two lines please. > + div = 1000000000; > + div *= 1 + prescale; > + val = clk_rate * period_ns; > + pc = div64_u64(val, div); > + val = clk_rate * duty_ns; > + dc = div64_u64(val, div); > + > + /* If duty_ns or period_ns are not achievable then return */ > + if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN) > + return -EINVAL; > + > + /* > + * If pc or dc have crossed their upper limit, then increase > + * prescale and recalculate pc and dc. > + */ > + if (pc > PERIOD_COUNT_MAX || dc > DUTY_CYCLE_HIGH_MAX) { > + if (++prescale > PRESCALE_MAX) > + return -EINVAL; > + continue; > + } This looks unintuitive to me, perhaps: if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX) break; if (++prescale > PRESCALE_MAX) return -EINVAL; ? > + /* Program prescale */ > + writel((readl(kp->base + PRESCALE_OFFSET) & PRESCALE_MASK(chan)) | > + prescale << PRESCALE_SHIFT(chan), > + kp->base + PRESCALE_OFFSET); Again, please split this into separate read/modify/write steps. > + > + /* Program period count */ > + writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan)); > + > + /* Program duty cycle high count */ > + writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan)); I don't think we need the comments. The register names are fairly descriptive, so the comments add no value. > + > + if (test_bit(PWMF_ENABLED, &pwm->flags)) > + kona_pwmc_apply_settings(kp, chan); > + > + return 0; > +} > + > +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + return kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period); > +} Why can't this just enable the channel? Why go through all the trouble of running the whole computations again? > + > +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); > + int chan = pwm->hwpwm; > + > + /* > + * The PWM hardware lacks a proper way to be disabled so > + * we just program zero duty cycle high count instead > + */ So clearing the enable bit doesn't disable the PWM channel? > + > + writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan)); > + kona_pwmc_apply_settings(kp, chan); > +} > + > +static const struct pwm_ops kona_pwm_ops = { > + .config = kona_pwmc_config, > + .owner = THIS_MODULE, > + .enable = kona_pwmc_enable, > + .disable = kona_pwmc_disable, > +}; Please move the .owner field to be the last field. Also you did define the PWMOUT_POLARITY field, which indicates that the hardware supports changing the signal's polarity, yet you don't implement the polarity feature. Why not? > +static int kona_pwmc_probe(struct platform_device *pdev) > +{ > + struct kona_pwmc *kp; > + struct resource *res; > + int ret = 0; I don't think this needs to be initialized. > + > + dev_dbg(&pdev->dev, "bcm_kona_pwmc probe\n"); Can this be removed? > + > + kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL); > + if (kp == NULL) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + kp->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(kp->base)) > + return PTR_ERR(kp->base); > + > + kp->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(kp->clk)) { > + dev_err(&pdev->dev, "Clock get failed : Err %d\n", ret); ret would be 0 here, indicating no error. This should probably be PTR_ERR(kp->clk). Also please make the error message more consistent, this one and the one below use completely different styles. Also, "Err" isn't very useful in an error message. Something like: dev_err(&pdev->dev, "failed to get clock: %d\n", PTR_ERR(kp->clk)); would be good. > + return PTR_ERR(kp->clk); > + } > + > + ret = clk_prepare_enable(kp->clk); > + if (ret < 0) > + return ret; Do you really want the clock enabled all the time? Why not just clk_enable() whenever a PWM is enabled? If you need the clock for register access, you can also bracket register accesses with clk_enable() and clk_disable(). Perhaps the power savings aren't worth the added effort, so if you'd rather not do that, I'm fine with it, too. > + > + /* Set smooth mode, push/pull, and normal polarity for all channels */ > + writel(PWM_CONTROL_INITIAL, kp->base + PWM_CONTROL_OFFSET); I'd expect to see bitfield definitions for smooth mode and push/pull, and PWM_CONTROL_INITIAL to be defined in terms of those. Better yet would be to have a value constructed at runtime with the initial value. > + dev_set_drvdata(&pdev->dev, kp); platform_set_drvdata(), please. > + kp->chip.dev = &pdev->dev; > + kp->chip.ops = &kona_pwm_ops; > + kp->chip.base = -1; > + kp->chip.npwm = KONA_PWM_CHANNEL_CNT; > + > + ret = pwmchip_add(&kp->chip); > + if (ret < 0) { > + clk_disable_unprepare(kp->clk); > + dev_err(&pdev->dev, "pwmchip_add() failed: Err %d\n", ret); For consistency with my above recommendation: dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret); Also, I'd move the error message before clk_disable_unprepare(). There's no technical reason really, but it's far more common that way around. > + } > + > + return ret; > +} > + > +static int kona_pwmc_remove(struct platform_device *pdev) > +{ > + struct kona_pwmc *kp = platform_get_drvdata(pdev); > + > + clk_disable_unprepare(kp->clk); > + return pwmchip_remove(&kp->chip); > +} > + > +static const struct of_device_id bcm_kona_pwmc_dt[] = { > + {.compatible = "brcm,kona-pwm"}, Needs spaces after { and before }. > + {}, Should be: { }. > +}; > +MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt); > + > +static struct platform_driver kona_pwmc_driver = { > + > + .driver = { > + .name = "bcm-kona-pwm", > + .owner = THIS_MODULE, > + .of_match_table = bcm_kona_pwmc_dt, > + }, The alignment is weird, should be: .driver = { .name = "bcm-kona-pwm", .owner = THIS_MODULE, .of_match_table = bcm_kona_pwmc_dt, }, You can also leave out the .owner field, that's assigned automatically by the driver core. > + > + .probe = kona_pwmc_probe, No blank line before this one. > + .remove = kona_pwmc_remove, > +}; > + > +module_platform_driver(kona_pwmc_driver); No blank line before this one. > + > +MODULE_AUTHOR("Broadcom"); I don't think Broadcom qualifies as author. This should be the name of whoever wrote the code. There are a few drivers that contain the company name in the MODULE_AUTHOR, but I don't think those are correct either. > +MODULE_DESCRIPTION("Driver for KONA PWMC"); "Driver for KONA PWM controller"? > +MODULE_LICENSE("GPL"); According to the header comment this should be "GPL v2". > +MODULE_VERSION("1.0"); I don't think we need this. Thierry
On Mon, Nov 25, 2013 at 3:08 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Nov 18, 2013 at 10:54:58AM -0800, Tim Kryger wrote: > [...] >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > [...] >> +config PWM_BCM_KONA >> + tristate "Kona PWM support" >> + depends on ARCH_BCM_MOBILE >> + default y > > Why do you want this to be the default? Shouldn't this be something that > a default configuration selects explicitly? This hardware is present on all recent Broadcom mobile SoCs so it is reasonable to expect that one would want the driver enabled but I think it makes sense to allow it be compiled out just in case it is unused. >> +#define PWM_CONTROL_INITIAL (0x3f3f3f00) > > Can this not be expressed as a bitmask of values for the individual > fields. > >> +#define PWMOUT_POLARITY(chan) (0x1 << (8 + chan)) > > This seems to only account for bits 8-13, what about the others? > >> +#define PWMOUT_ENABLE(chan) (0x1 << chan) > > Well, this accounts for bits 0-5, that still leaves 16-21 and 24-29. 29:24 - Configures each PWM channel for smooth transitions or glitches 21:16 - Configures each PWM channel for push/pull or open drain output >> +#define PRESCALE_OFFSET (0x00000004) >> +#define PRESCALE_SHIFT(chan) (chan << 2) > > I'm confused. This allocates 2 bits for each channel... > >> +#define PRESCALE_MASK(chan) (~(0x7 << (chan << 2))) >> +#define PRESCALE_MIN (0x00000000) >> +#define PRESCALE_MAX (0x00000007) > > ... but 0x7 requires at least 3 bits. Actually this is allocating 2^2 bits for each channel. >> +#define PERIOD_COUNT_OFFSET(chan) (0x00000008 + (chan << 3)) >> +#define PERIOD_COUNT_MIN (0x00000002) >> +#define PERIOD_COUNT_MAX (0x00ffffff) > > Why PERIOD_COUNT? PERIOD is descriptive enough. Or is this the name as > found in the manual? I agree but as you suspected this name comes from the hardware docs. >> +#define DUTY_CYCLE_HIGH_OFFSET(chan) (0x0000000c + (chan << 3)) >> +#define DUTY_CYCLE_HIGH_MIN (0x00000000) >> +#define DUTY_CYCLE_HIGH_MAX (0x00ffffff) > > By definition the duty-cycle is where the signal is high. Again, if this > is how the manual names the registers it's fine. Same comment as above. >> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm) >> +{ >> + return kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period); >> +} > > Why can't this just enable the channel? Why go through all the trouble > of running the whole computations again? The hardware is always enabled and at best can be be configured to operate at zero duty. The settings in HW may have already been triggered and might not be what you want. For example: /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable /sys/class/pwm/pwmchip0/pwm1# echo 5000 > period /sys/class/pwm/pwmchip0/pwm1# echo 2000 > duty_cycle /sys/class/pwm/pwmchip0/pwm1# echo 0 > enable /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable >> + >> +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm) >> +{ >> + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); >> + int chan = pwm->hwpwm; >> + >> + /* >> + * The PWM hardware lacks a proper way to be disabled so >> + * we just program zero duty cycle high count instead >> + */ > > So clearing the enable bit doesn't disable the PWM channel? That is correct. They picked a terrible name for this bit. >> + >> + writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan)); >> + kona_pwmc_apply_settings(kp, chan); >> +} >> + >> +static const struct pwm_ops kona_pwm_ops = { >> + .config = kona_pwmc_config, >> + .owner = THIS_MODULE, >> + .enable = kona_pwmc_enable, >> + .disable = kona_pwmc_disable, >> +}; > > Please move the .owner field to be the last field. Also you did define > the PWMOUT_POLARITY field, which indicates that the hardware supports > changing the signal's polarity, yet you don't implement the polarity > feature. Why not? I wanted to keep this driver simple for now. >> + ret = clk_prepare_enable(kp->clk); >> + if (ret < 0) >> + return ret; > > Do you really want the clock enabled all the time? Why not just > clk_enable() whenever a PWM is enabled? If you need the clock for > register access, you can also bracket register accesses with > clk_enable() and clk_disable(). Perhaps the power savings aren't worth > the added effort, so if you'd rather not do that, I'm fine with it, too. I intend to follow up with a patch to do exactly that but I want to establish the baseline functionality first. >> +MODULE_AUTHOR("Broadcom"); > > I don't think Broadcom qualifies as author. This should be the name of > whoever wrote the code. There are a few drivers that contain the company > name in the MODULE_AUTHOR, but I don't think those are correct either. Would you be fine with two lines here? Something like: MODULE_AUTHOR("Broadcom Corporation <bcm-kernel-feedback-list@broadcom.com>"); MODULE_AUTHOR("Tim Kryger <tkryger@broadcom.com>"); Thanks for the review. I will make all of the other changes you have recommended. -Tim
On Mon, Nov 25, 2013 at 05:38:44PM -0800, Tim Kryger wrote: > On Mon, Nov 25, 2013 at 3:08 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Mon, Nov 18, 2013 at 10:54:58AM -0800, Tim Kryger wrote: > > [...] > >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig [...] > >> +#define PWM_CONTROL_INITIAL (0x3f3f3f00) > > > > Can this not be expressed as a bitmask of values for the individual > > fields. > > > >> +#define PWMOUT_POLARITY(chan) (0x1 << (8 + chan)) > > > > This seems to only account for bits 8-13, what about the others? > > > >> +#define PWMOUT_ENABLE(chan) (0x1 << chan) > > > > Well, this accounts for bits 0-5, that still leaves 16-21 and 24-29. > > 29:24 - Configures each PWM channel for smooth transitions or glitches > 21:16 - Configures each PWM channel for push/pull or open drain output Excellent, now if you can turn that into bit definitions and define PWM_CONTROL_INITIAL in terms of those, then I'll be very happy. One other thing I didn't pay attention to before: while it's quite unlikely ever to happen, you might want to spend an extra pair of parentheses around "chan", just in case. > >> +#define PRESCALE_OFFSET (0x00000004) > >> +#define PRESCALE_SHIFT(chan) (chan << 2) > > > > I'm confused. This allocates 2 bits for each channel... > > > >> +#define PRESCALE_MASK(chan) (~(0x7 << (chan << 2))) > >> +#define PRESCALE_MIN (0x00000000) > >> +#define PRESCALE_MAX (0x00000007) > > > > ... but 0x7 requires at least 3 bits. > > Actually this is allocating 2^2 bits for each channel. Doh! Indeed. Perhaps writing it as (~(0x7 << (chan * 4))) would make it easier to digest for slow-witted people like myself. > >> +#define PERIOD_COUNT_OFFSET(chan) (0x00000008 + (chan << 3)) > >> +#define PERIOD_COUNT_MIN (0x00000002) > >> +#define PERIOD_COUNT_MAX (0x00ffffff) > > > > Why PERIOD_COUNT? PERIOD is descriptive enough. Or is this the name as > > found in the manual? > > I agree but as you suspected this name comes from the hardware docs. Okay, that's fine then. Do you have a pointer to that documentation? > >> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm) > >> +{ > >> + return kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period); > >> +} > > > > Why can't this just enable the channel? Why go through all the trouble > > of running the whole computations again? > > The hardware is always enabled and at best can be be configured to > operate at zero duty. What good are the PWMOUT_ENABLE bits then? Is that really only used for triggering updates? That's what another comment suggests, but if so, can the comment in kona_pwmc_apply_settings() be extended to mention that? > The settings in HW may have already been triggered and might not be > what you want. > > For example: > > /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable > /sys/class/pwm/pwmchip0/pwm1# echo 5000 > period > /sys/class/pwm/pwmchip0/pwm1# echo 2000 > duty_cycle > /sys/class/pwm/pwmchip0/pwm1# echo 0 > enable > /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable I'm not exactly sure what this is supposed to demonstrate. > >> + > >> + writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan)); > >> + kona_pwmc_apply_settings(kp, chan); > >> +} > >> + > >> +static const struct pwm_ops kona_pwm_ops = { > >> + .config = kona_pwmc_config, > >> + .owner = THIS_MODULE, > >> + .enable = kona_pwmc_enable, > >> + .disable = kona_pwmc_disable, > >> +}; > > > > Please move the .owner field to be the last field. Also you did define > > the PWMOUT_POLARITY field, which indicates that the hardware supports > > changing the signal's polarity, yet you don't implement the polarity > > feature. Why not? > > I wanted to keep this driver simple for now. Fair enough. > > Do you really want the clock enabled all the time? Why not just > > clk_enable() whenever a PWM is enabled? If you need the clock for > > register access, you can also bracket register accesses with > > clk_enable() and clk_disable(). Perhaps the power savings aren't worth > > the added effort, so if you'd rather not do that, I'm fine with it, too. > > I intend to follow up with a patch to do exactly that but I want to > establish the baseline functionality first. Okay, that's fine. > >> +MODULE_AUTHOR("Broadcom"); > > > > I don't think Broadcom qualifies as author. This should be the name of > > whoever wrote the code. There are a few drivers that contain the company > > name in the MODULE_AUTHOR, but I don't think those are correct either. > > Would you be fine with two lines here? Something like: > > MODULE_AUTHOR("Broadcom Corporation <bcm-kernel-feedback-list@broadcom.com>"); > MODULE_AUTHOR("Tim Kryger <tkryger@broadcom.com>"); Not sure. That email address certainly doesn't look like it belongs to the driver author. Neither did Broadcom actually write the driver, you did, right? If you're concerned about your employer not being credited you're listed with an @broadcom.com email address and there's the copyright statement. All of that said, I wasn't able to dig up a normative policy and either of your proposed alternatives do exist in the kernel, so I withdraw any objections regarding MODULE_AUTHOR. Take your pick. Thierry
On Tue, Nov 26, 2013 at 1:45 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Nov 25, 2013 at 05:38:44PM -0800, Tim Kryger wrote: > Okay, that's fine then. Do you have a pointer to that documentation? Unfortunately the documentation is not publicly available at the moment. >> The hardware is always enabled and at best can be be configured to >> operate at zero duty. > > What good are the PWMOUT_ENABLE bits then? Is that really only used for > triggering updates? That's what another comment suggests, but if so, can > the comment in kona_pwmc_apply_settings() be extended to mention that? The guidance from the documentation is to treat the enable bits as a trigger. Sure I can expand the comment to try and make this more clear. >> The settings in HW may have already been triggered and might not be >> what you want. >> >> For example: >> >> /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable >> /sys/class/pwm/pwmchip0/pwm1# echo 5000 > period >> /sys/class/pwm/pwmchip0/pwm1# echo 2000 > duty_cycle >> /sys/class/pwm/pwmchip0/pwm1# echo 0 > enable >> /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable > > I'm not exactly sure what this is supposed to demonstrate. The computation has to be performed for every operation except the disable. > All of that said, I wasn't able to dig up a normative policy and either > of your proposed alternatives do exist in the kernel, so I withdraw any > objections regarding MODULE_AUTHOR. Take your pick. Thanks
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 75840b5..4ed5d8b 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -53,6 +53,16 @@ config PWM_ATMEL_TCB To compile this driver as a module, choose M here: the module will be called pwm-atmel-tcb. +config PWM_BCM_KONA + tristate "Kona PWM support" + depends on ARCH_BCM_MOBILE + default y + help + Generic PWM framework driver for Broadcom Kona PWM block. + + To compile this driver as a module, choose M here: the module + will be called pwm-bcm-kona. + config PWM_BFIN tristate "Blackfin PWM support" depends on BFIN_GPTIMERS diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 77a8c18..dd96938 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_BCM_KONA) += pwm-bcm-kona.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-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c new file mode 100644 index 0000000..cdf30d9 --- /dev/null +++ b/drivers/pwm/pwm-bcm-kona.c @@ -0,0 +1,226 @@ +/* + * Copyright (C) 2013 Broadcom Corporation + * + * 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 version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/ioport.h> +#include <linux/math64.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/slab.h> +#include <linux/types.h> + +#define KONA_PWM_CHANNEL_CNT 6 + +#define PWM_CONTROL_OFFSET (0x00000000) +#define PWM_CONTROL_INITIAL (0x3f3f3f00) +#define PWMOUT_POLARITY(chan) (0x1 << (8 + chan)) +#define PWMOUT_ENABLE(chan) (0x1 << chan) + +#define PRESCALE_OFFSET (0x00000004) +#define PRESCALE_SHIFT(chan) (chan << 2) +#define PRESCALE_MASK(chan) (~(0x7 << (chan << 2))) +#define PRESCALE_MIN (0x00000000) +#define PRESCALE_MAX (0x00000007) + +#define PERIOD_COUNT_OFFSET(chan) (0x00000008 + (chan << 3)) +#define PERIOD_COUNT_MIN (0x00000002) +#define PERIOD_COUNT_MAX (0x00ffffff) + +#define DUTY_CYCLE_HIGH_OFFSET(chan) (0x0000000c + (chan << 3)) +#define DUTY_CYCLE_HIGH_MIN (0x00000000) +#define DUTY_CYCLE_HIGH_MAX (0x00ffffff) + +struct kona_pwmc { + struct pwm_chip chip; + void __iomem *base; + struct clk *clk; +}; + +static void kona_pwmc_apply_settings(struct kona_pwmc *kp, int chan) +{ + /* New settings take effect on rising edge of enable bit */ + writel(readl(kp->base + PWM_CONTROL_OFFSET) & ~PWMOUT_ENABLE(chan), + kp->base + PWM_CONTROL_OFFSET); + writel(readl(kp->base + PWM_CONTROL_OFFSET) | PWMOUT_ENABLE(chan), + kp->base + PWM_CONTROL_OFFSET); +} + +static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); + u64 val, div, clk_rate; + unsigned long prescale = PRESCALE_MIN, pc, dc; + int chan = pwm->hwpwm; + + /* + * Find period count, duty count and prescale to suit duty_ns and + * period_ns. This is done according to formulas described below: + * + * period_ns = 10^9 * (PRESCALE + 1) * PC / PWM_CLK_RATE + * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE + * + * PC = (PWM_CLK_RATE * period_ns) / (10^9 * (PRESCALE + 1)) + * DC = (PWM_CLK_RATE * duty_ns) / (10^9 * (PRESCALE + 1)) + */ + + clk_rate = clk_get_rate(kp->clk); + while (1) { + div = 1000000000; + div *= 1 + prescale; + val = clk_rate * period_ns; + pc = div64_u64(val, div); + val = clk_rate * duty_ns; + dc = div64_u64(val, div); + + /* If duty_ns or period_ns are not achievable then return */ + if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN) + return -EINVAL; + + /* + * If pc or dc have crossed their upper limit, then increase + * prescale and recalculate pc and dc. + */ + if (pc > PERIOD_COUNT_MAX || dc > DUTY_CYCLE_HIGH_MAX) { + if (++prescale > PRESCALE_MAX) + return -EINVAL; + continue; + } + break; + } + + /* Program prescale */ + writel((readl(kp->base + PRESCALE_OFFSET) & PRESCALE_MASK(chan)) | + prescale << PRESCALE_SHIFT(chan), + kp->base + PRESCALE_OFFSET); + + /* Program period count */ + writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan)); + + /* Program duty cycle high count */ + writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan)); + + if (test_bit(PWMF_ENABLED, &pwm->flags)) + kona_pwmc_apply_settings(kp, chan); + + return 0; +} + +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + return kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period); +} + +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct kona_pwmc *kp = dev_get_drvdata(chip->dev); + int chan = pwm->hwpwm; + + /* + * The PWM hardware lacks a proper way to be disabled so + * we just program zero duty cycle high count instead + */ + + writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan)); + kona_pwmc_apply_settings(kp, chan); +} + +static const struct pwm_ops kona_pwm_ops = { + .config = kona_pwmc_config, + .owner = THIS_MODULE, + .enable = kona_pwmc_enable, + .disable = kona_pwmc_disable, +}; + +static int kona_pwmc_probe(struct platform_device *pdev) +{ + struct kona_pwmc *kp; + struct resource *res; + int ret = 0; + + dev_dbg(&pdev->dev, "bcm_kona_pwmc probe\n"); + + kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL); + if (kp == NULL) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + kp->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(kp->base)) + return PTR_ERR(kp->base); + + kp->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(kp->clk)) { + dev_err(&pdev->dev, "Clock get failed : Err %d\n", ret); + return PTR_ERR(kp->clk); + } + + ret = clk_prepare_enable(kp->clk); + if (ret < 0) + return ret; + + /* Set smooth mode, push/pull, and normal polarity for all channels */ + writel(PWM_CONTROL_INITIAL, kp->base + PWM_CONTROL_OFFSET); + + dev_set_drvdata(&pdev->dev, kp); + + kp->chip.dev = &pdev->dev; + kp->chip.ops = &kona_pwm_ops; + kp->chip.base = -1; + kp->chip.npwm = KONA_PWM_CHANNEL_CNT; + + ret = pwmchip_add(&kp->chip); + if (ret < 0) { + clk_disable_unprepare(kp->clk); + dev_err(&pdev->dev, "pwmchip_add() failed: Err %d\n", ret); + } + + return ret; +} + +static int kona_pwmc_remove(struct platform_device *pdev) +{ + struct kona_pwmc *kp = platform_get_drvdata(pdev); + + clk_disable_unprepare(kp->clk); + return pwmchip_remove(&kp->chip); +} + +static const struct of_device_id bcm_kona_pwmc_dt[] = { + {.compatible = "brcm,kona-pwm"}, + {}, +}; +MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt); + +static struct platform_driver kona_pwmc_driver = { + + .driver = { + .name = "bcm-kona-pwm", + .owner = THIS_MODULE, + .of_match_table = bcm_kona_pwmc_dt, + }, + + .probe = kona_pwmc_probe, + .remove = kona_pwmc_remove, +}; + +module_platform_driver(kona_pwmc_driver); + +MODULE_AUTHOR("Broadcom"); +MODULE_DESCRIPTION("Driver for KONA PWMC"); +MODULE_LICENSE("GPL"); +MODULE_VERSION("1.0");