Messages in this thread |  | | Date | Mon, 25 Nov 2013 12:08:56 +0100 | From | Thierry Reding <> | Subject | Re: [PATCH 2/5] pwm: kona: Introduce Kona PWM controller support |
| |
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 [unhandled content-type:application/pgp-signature] |  |