On Sat, Feb 08, 2014 at 06:23:22PM +0800, Barry Song wrote: > From: Rongjun Ying <[email protected]> > > PWM controller of CSR SiRFSoC can generate 7 independent outputs. Each output
"The PWM controller of the CSR SiRFSoC..." and "Each output's..."
> duty cycle can be adjusted by setting the corresponding wait & hold registers.
>
> Supports 7 independent channel output: 6 for external(channel0-5) and 1 for
> internal(channel6).
This repeats part of the first sentence. Perhaps:
There are 6 external channels (0 to 5) and 1 internal channel (6).
?
> Supports wide frequency range: divide by 2 to 65536*2 of source clock.
"Supports a wide frequency range: the source clock divider can be from 2
up to 65536*2".
> Documentation/devicetree/bindings/pwm/pwm-sirf.txt | 17 +
> arch/arm/boot/dts/atlas6.dtsi | 3 +-
> arch/arm/boot/dts/prima2.dtsi | 3 +-
> drivers/pwm/Kconfig | 9 +
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sirf.c | 308
> ++++++++++++++++++++
> 6 files changed, 339 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sirf.txt
> create mode 100644 drivers/pwm/pwm-sirf.c
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sirf.txt
> b/Documentation/devicetree/bindings/pwm/pwm-sirf.txt
> new file mode 100644
> index 0000000..4b10109
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sirf.txt
> @@ -0,0 +1,17 @@
> +SiRF prima2 & atlas6 PWM drivers
> +
> +Required properties:
> +- compatible: "sirf,prima2-pwm"
> +- reg: physical base address and length of the controller's registers
> +- #pwm-cells: should be 2. The first cell specifies the per-chip index
> + of the PWM to use and the second cell is the period in nanoseconds.
> +- clocks: from common clock binding: the 1st clock is for PWM controller
> + the 2nd clock is the source to generate PWM waves
> +
> +Example:
> +pwm: pwm@b0130000 {
> + compatible = "sirf,prima2-pwm";
> + #pwm-cells = <2>;
> + reg = <0xb0130000 0x10000>;
> + clocks = <&clks 21>, <&clks 1>;
> +};
Please move this into a separate patch and Cc the device tree bindings
maintainers. There's nothing non-standard in this binding as far as I
can tell, but I'd still like to give them an opportunity to object.
Also you should be documenting the clock-names property here as well.
> diff --git a/arch/arm/boot/dts/atlas6.dtsi b/arch/arm/boot/dts/atlas6.dtsi
> index f8674bc..5a09815 100644
> --- a/arch/arm/boot/dts/atlas6.dtsi
> +++ b/arch/arm/boot/dts/atlas6.dtsi
> @@ -614,8 +614,9 @@
>
> pwm@b0130000 {
> compatible = "sirf,prima2-pwm";
> + #pwm-cells = <2>;
> reg = <0xb0130000 0x10000>;
> - clocks = <&clks 21>;
> + clocks = <&clks 21>, <&clks 1>;
> };
>
> efusesys@b0140000 {
> diff --git a/arch/arm/boot/dts/prima2.dtsi b/arch/arm/boot/dts/prima2.dtsi
> index 0e21993..3439e48 100644
> --- a/arch/arm/boot/dts/prima2.dtsi
> +++ b/arch/arm/boot/dts/prima2.dtsi
> @@ -642,8 +642,9 @@
>
> pwm@b0130000 {
> compatible = "sirf,prima2-pwm";
> + #pwm-cells = <2>;
> reg = <0xb0130000 0x10000>;
> - clocks = <&clks 21>;
> + clocks = <&clks 21>, <&clks 1>;
> };
>
> efusesys@b0140000 {
These changes need to go into separate patches and go in through the
arm-soc tree.
> diff --git a/drivers/pwm/pwm-sirf.c b/drivers/pwm/pwm-sirf.c
> new file mode 100644
> index 0000000..b717de0
> --- /dev/null
> +++ b/drivers/pwm/pwm-sirf.c
> @@ -0,0 +1,308 @@
> +/*
> + * SIRF serial SoC PWM device core driver
> + *
> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group
> company.
> + *
> + * Licensed under GPLv2 or later.
> + */
[...]
> +#define SIRF_PWM_CHL_NUM 7
This is only used in a single location with a well-known meaning, so no
need to use a symbolic name.
> +#define SIRF_PWM_BLS_GRP_NUM 16
This isn't used as far as I can tell.
> +struct sirf_pwm {
> + void __iomem *base;
> + struct clk *clk;
> + struct pwm_chip chip;
> + unsigned long src_clk_rate;
> +};
Please drop the alignment of the structure field. Also perhaps move the
chip field to be the first, so that the upcasting below can be turned
into a noop.
> +#define to_sirf_chip(chip) container_of(chip, struct sirf_pwm, chip)
Please make this a static inline function.
> +
> +static unsigned int sirf_pwm_ns_to_cycles(struct pwm_chip *chip, unsigned
> int time_ns)
Please wrap this to go on a single line.
> +{
> + struct sirf_pwm *spwm = to_sirf_chip(chip);
> + u64 dividend;
> + unsigned int cycle;
> +
> + dividend = spwm->src_clk_rate * time_ns + NSEC_PER_SEC / 2;
> + do_div(dividend, NSEC_PER_SEC);
> +
> + cycle = dividend & 0xFFFFFFFFUL;
I don't think you need the mask, since you're casting to a 32-bit
unsigned integer anyway.
> +
> + return cycle > 1 ? cycle : 1;
> +}
> +
> +static int sirf_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + int duty_ns, int period_ns)
Please align arguments on subsequent lines with those of the first.
> +{
> + unsigned int period_cycles, high_cycles, low_cycles;
> + unsigned int val;
u32 please.
> + struct sirf_pwm *spwm = to_sirf_chip(chip);
> +
> + period_cycles = sirf_pwm_ns_to_cycles(chip, period_ns);
> +
> + high_cycles = sirf_pwm_ns_to_cycles(chip, duty_ns);
No need for the blank line above, since there's no blank line below
either.
> + low_cycles = period_cycles - high_cycles;
> +
> + if (period_cycles == 1) {
> + /* bypass mode */
> + val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK);
> + val |= 0x1 << (BYPASS_MODE_BIT + pwm->hwpwm);
> + writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK);
> + dev_warn(chip->dev, "period is too short!\n");
What does the bypass mode do? Would it perhaps be better to make this an
outright error rather than only warning about it?
> + } else {
> + /* divider mode */
> + val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK);
> + val &= ~(0x1 << (BYPASS_MODE_BIT + pwm->hwpwm));
> + writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK);
> +
> + if (high_cycles == period_cycles) {
> + high_cycles--;
> + low_cycles = 1;
> + }
> +
> + writel(high_cycles, spwm->base +
> SIRF_PWM_GET_WAIT_OFFSET(pwm->hwpwm));
> + writel(low_cycles, spwm->base +
> SIRF_PWM_GET_HOLD_OFFSET(pwm->hwpwm));
> + }
> +
> + return 0;
> +}
> +
> +static int sirf_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct sirf_pwm *spwm = to_sirf_chip(chip);
> + unsigned int val;
The type used by readl() and writel() is u32.
> + /* disable preclock */
> + val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
> + val &= ~(1 << pwm->hwpwm);
> + writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
> +
> + /* select preclock source must after disable preclk*/
> + val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK);
> + val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm));
> + writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK);
> + /* wait for some time */
> + udelay(100);
usleep_range() perhaps?
> +static void sirf_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + unsigned int val;
u32 again.
> + struct sirf_pwm *spwm = to_sirf_chip(chip);
> + /* disable output */
Blank line between the above two, please.
> + val = readl(spwm->base + SIRF_PWM_OE);
> + val &= ~(1 << pwm->hwpwm);
> + writel(val, spwm->base + SIRF_PWM_OE);
> +
> + /* disable postclock */
> + val = readl(spwm->base + SIRF_PWM_ENABLE_POSTCLOCK);
> + val &= ~(1 << pwm->hwpwm);
> + writel(val, spwm->base + SIRF_PWM_ENABLE_POSTCLOCK);
> +
> + /* disable preclock */
> + val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
> + val &= ~(1 << pwm->hwpwm);
> + writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
I think you need a lock to synchronize accesses to these registers.
> +}
> +
> +static struct pwm_ops sirf_pwm_ops = {
static const please.
> + .enable = sirf_pwm_enable,
> + .disable = sirf_pwm_disable,
> + .config = sirf_pwm_config,
> + .owner = THIS_MODULE,
> +};
> +
> +static int sirf_pwm_probe(struct platform_device *pdev)
> +{
> + struct sirf_pwm *spwm;
> + struct resource *mem_res;
> + struct clk *clk_pwm_src;
> + int ret;
> +
> + spwm = devm_kzalloc(&pdev->dev, sizeof(struct sirf_pwm),
> + GFP_KERNEL);
"sizeof(*spwm)", please. And the above fits on a single line, no need to
wrap them.
> + if (!spwm)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, spwm);
> +
> + mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + spwm->base = devm_ioremap_resource(&pdev->dev, mem_res);
> + if (!spwm->base)
> + return -ENOMEM;
> +
> + /*
> + * the 1st clock is for PWM controller
> + */
> + spwm->clk = of_clk_get(pdev->dev.of_node, 0);
Use a clock-names property and devm_clk_get() here, please.
> + if (IS_ERR(spwm->clk)) {
> + dev_err(&pdev->dev, "Get PWM controller clock failed.\n");
"failed to get PWM controller clock"?
> + return PTR_ERR(spwm->clk);
> + }
> + clk_prepare_enable(spwm->clk);
Space between the above two. Also you need to check the return value of
clk_prepare_enable().
> +
> + /*
> + * the 2nd clock is the source to generate PWM waves
I'd prefer "signals" over "waves".
> + * it is the OSC on SiRFSoC
The clock is configurable via DT, so this isn't necessarily true. Even
if all hardware in existence currently works that way, it isn't a given
to remain like that forever.
> + */
> + clk_pwm_src = of_clk_get(pdev->dev.of_node, 1);
> + if (IS_ERR(clk_pwm_src)) {
> + dev_err(&pdev->dev, "Get PWM source clock failed.\n");
"failed to get PWM source clock"?
> + return PTR_ERR(clk_pwm_src);
> + }
> + spwm->src_clk_rate = clk_get_rate(clk_pwm_src);
Space between the above two please.
> + clk_put(clk_pwm_src);
Why drop the reference to it here? Shouldn't you keep it around and even
call clk_prepare_enable() on it to make sure it's actually on?
> +
> + spwm->chip.dev = &pdev->dev;
> + spwm->chip.ops = &sirf_pwm_ops;
> + spwm->chip.base = 0;
> + spwm->chip.npwm = SIRF_PWM_CHL_NUM;
> +
> + ret = pwmchip_add(&spwm->chip);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to register pwm\n");
"PWM" please.
> + clk_disable_unprepare(spwm->clk);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int sirf_pwm_remove(struct platform_device *pdev)
> +{
> + struct sirf_pwm *spwm = platform_get_drvdata(pdev);
> + clk_disable_unprepare(spwm->clk);
I'd prefer a blank line between the two above.
> + clk_put(spwm->clk);
> +
> + pwmchip_remove(&spwm->chip);
> + return 0;
This should be "return pwmchip_remove(...);".
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int sirf_pwm_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct sirf_pwm *spwm = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(spwm->clk);
> +
> + return 0;
> +}
> +
> +static void sirf_pwm_config_restore(struct sirf_pwm *spwm)
> +{
> + struct pwm_device *pwm;
> + int i;
> +
> + for (i = 0; i < spwm->chip.npwm; i++) {
> + pwm = &spwm->chip.pwms[i];
> + /*
> + * while restoring from hibernation, state of pwm is enabled,
> + * but PWM hardware is not re-enabled
> + */
> + if (test_bit(PWMF_REQUESTED, &pwm->flags) &&
> + test_bit(PWMF_ENABLED, &pwm->flags))
Indentation is off by one space here.
> + sirf_pwm_enable(&spwm->chip, pwm);
> + }
> +}
> +
> +static int sirf_pwm_resume(struct device *dev)
> +{
> + struct sirf_pwm *spwm = dev_get_drvdata(dev);
> +
> + clk_prepare_enable(spwm->clk);
> +
> + sirf_pwm_config_restore(spwm);
> +
> + return 0;
> +}
> +
> +static int sirf_pwm_restore(struct device *dev)
> +{
> + struct sirf_pwm *spwm = dev_get_drvdata(dev);
> +
> + /* back from hibernation, clock is already enabled */
> + sirf_pwm_config_restore(spwm);
Why is the clock already enabled? Shouldn't it be off until this driver
enables it?
> +
> + return 0;
> +}
> +
> +#else
> +#define sirf_pwm_resume NULL
> +#define sirf_pwm_suspend NULL
> +#define sirf_pwm_restore NULL
> +#endif
> +
> +static const struct dev_pm_ops sirf_pwm_pm_ops = {
> + .suspend = sirf_pwm_suspend,
> + .resume = sirf_pwm_resume,
> + .restore = sirf_pwm_restore,
> +};
Because if you can unify _resume and _restore, this could all be
simplified using SIMPLE_DEV_PM_OPS().
> +
> +static const struct of_device_id sirf_pwm_of_match[] = {
> + { .compatible = "sirf,prima2-pwm", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, sirf_pwm_of_match);
> +
> +static struct platform_driver sirf_pwm_driver = {
> + .driver = {
> + .name = "prima2-pwm",
"sirf-pwm"?
> + .owner = THIS_MODULE,
This is no longer necessary.
> + .pm = &sirf_pwm_pm_ops,
> + .of_match_table = sirf_pwm_of_match,
> + },
> + .probe = sirf_pwm_probe,
> + .remove = sirf_pwm_remove,
> +};
> +
> +module_platform_driver(sirf_pwm_driver);
> +
> +MODULE_DESCRIPTION("SIRF serial SoC PWM device core driver");
> +MODULE_AUTHOR("RongJun Ying <[email protected]>, "
> + "Huayi Li <[email protected]>");
These could simply be two separate MODULE_AUTHOR() entries.
> +MODULE_LICENSE("GPL v2");
The file header says GPL v2 or later, which would be "GPL". "GPL v2" is
GPL v2 only.
pgpslAiKvS8ns.pgp
Description: PGP signature
