Re: [PATCH v3] pwm: add CSR SiRFSoC PWM driver

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 13614 bytes --]

On Sat, Feb 08, 2014 at 06:23:22PM +0800, Barry Song wrote:
> From: Rongjun Ying <Rongjun.ying@csr.com>
> 
> 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 <Rongjun.Ying@csr.com>, "
> +	"Huayi Li <huayi.li@csr.com>");

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.


[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]