PWM: add and enable CSR SiRFSoC PWM driver
v4 cover PWM: add and enable CSR SiRFSoC PWM driver
From: Barry Song <[email protected]>
Barry Song (3):
pwm: sirf: add dt-binding document
ARM: dts: sirf: fix the pwm-cells and clocks for pwm
ARM: prima2_defconfig: enable PWM and sirf PWM driver
Huayi Li (1):
pwm: add CSR SiRFSoC PWM driver
Documentation/devicetree/bindings/pwm/pwm-sirf.txt | 23 ++ arch/arm/boot/dts/atlas6.dtsi | 5 +- arch/arm/boot/dts/prima2.dtsi | 6 +- arch/arm/configs/prima2_defconfig | 2 + drivers/pwm/Kconfig | 9 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sirf.c | 329 ++++++++++++++++++++ 7 files changed, 371 insertions(+), 4 deletions(-) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sirf.txt create mode 100644 drivers/pwm/pwm-sirf.c
Signature · 1 line
1.7.5.4
PATCH v4 1/4 pwm: add CSR SiRFSoC PWM driver
From: Huayi Li <[email protected]>
PWM controller of CSR SiRFSoC can generate 7 independent outputs. Each output duty cycle can be adjusted by setting the corresponding wait & hold registers. There are 6 external channels (0 to 5) and 1 internal channel (6). Supports a wide frequency range: the source clock divider can be from 2 up to 65536*2.
---
-v4: fix many issues from Thierry's feedbacks
drivers/pwm/Kconfig | 9 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sirf.c | 329 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 339 insertions(+), 0 deletions(-) create mode 100644 drivers/pwm/pwm-sirf.c
drivers/pwm/Kconfig +9 -0
@@ -176,6 +176,15 @@ config PWM_SAMSUNG To compile this driver as a module, choose M here: the module will be called pwm-samsung. +config PWM_SIRF + tristate "SiRF PWM support" + depends on (ARCH_SIRF || COMPILE_TEST) && COMMON_CLK && OF + help + Generic PWM framework driver for SiRF SoC. + + To compile this driver as a module, choose M here: the module + will be called pwm-sirf. + config PWM_SPEAR tristate "STMicroelectronics SPEAr PWM support" depends on PLAT_SPEAR
drivers/pwm/Makefile +1 -0
@@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o obj-$(CONFIG_PWM_PXA) += pwm-pxa.o obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o +obj-$(CONFIG_PWM_SIRF) += pwm-sirf.o obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
drivers/pwm/pwm-sirf.c new file +329 -0
329 lines not on this page · raw message
Signature · 1 line
1.7.5.4
PATCH v4 1/4 pwm: add CSR SiRFSoC PWM driver
Quoted 7 lines · Barry Song
From: Huayi Li <[email protected]>
PWM controller of CSR SiRFSoC can generate 7 independent outputs. Each output duty cycle can be adjusted by setting the corresponding wait & hold registers. There are 6 external channels (0 to 5) and 1 internal channel (6). Supports a wide frequency range: the source clock divider can be from 2 up to 65536*2.
This commit message is too wide. Please always wrap the commit message at 72 characters. See this for reference[0].
[0]: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
Quoted 1 line
drivers/pwm/Kconfig +0
-0
[...]
drivers/pwm/Kconfig config PWM_SAMSUNG · line 181
Quoted 3 lines
drivers/pwm/Kconfig +3
-0+config PWM_SIRF + tristate "SiRF PWM support" + depends on (ARCH_SIRF || COMPILE_TEST) && COMMON_CLK && OF
I'd prefer these to be split into more lines for readability, like so:
depends on ARCH_SIRF || COMPILE_TEST
depends on COMMON_CLK
depends on OF
Quoted 1 line
drivers/pwm/pwm-sirf.c +0
-0
[...]
Quoted 3 lines
+ * Licensed under GPLv2.
+ */
+#include <linux/kernel.h>
Could use a blank line between the above two, visually separating the header comment from the list of include files.
drivers/pwm/pwm-sirf.c line 17
Quoted 9 lines
drivers/pwm/pwm-sirf.c +9
-0+#include <linux/init.h> +#include <linux/module.h> +#include <linux/device.h> +#include <linux/platform_device.h> +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/pwm.h> +#include <linux/of.h> +#include <linux/io.h>
Also please keep these sorted alphabetically.
drivers/pwm/pwm-sirf.c line 34
Quoted 2 lines
drivers/pwm/pwm-sirf.c +2
-0+struct sirf_pwm { +struct pwm_chip chip;
There's a tab between pwm_chip and chip here. It should be a space.
drivers/pwm/pwm-sirf.c line 39
Quoted 5 lines
drivers/pwm/pwm-sirf.c +5
-0+struct mutex mutex; +void __iomem *base; +struct clk *pwmc_clk; +/* select OSC(26MHz) as clock source to generate PWM signals */ +struct clk *sigsrc_clk;
Maybe I'm missing something, but what in this driver is selecting the OSC as clock source?
drivers/pwm/pwm-sirf.c line 40
Quoted 1 line
drivers/pwm/pwm-sirf.c +1
-0+unsigned long sigsrc_clk_rate;
Why do you cache this value? Can't you simply query the clock framework for it when you need it?
drivers/pwm/pwm-sirf.c line 60
Quoted 13 lines
drivers/pwm/pwm-sirf.c +13
-0+static u32 sirf_pwm_ns_to_cycles (struct pwm_chip *chip,u32 time_ns) +{ +struct sirf_pwm *spwm =to_sirf_pwm_chip (chip); +u64 dividend; +u32 cycle; + + dividend = (u64 )spwm->sigsrc_clk_rate * time_ns +NSEC_PER_SEC /2 ; +do_div (dividend,NSEC_PER_SEC ); + + cycle = dividend; + +return cycle >1 ? cycle :1 ; +}
I think you can do without the cycle variable here.
drivers/pwm/pwm-sirf.c line 63
Quoted 2 lines
drivers/pwm/pwm-sirf.c +2
-0+static int sirf_pwm_config (struct pwm_chip *chip,struct pwm_device *pwm, +int duty_ns,int period_ns)
Please always align the first argument on subsequent lines with the first argument on the first line.
drivers/pwm/pwm-sirf.c line 69
Quoted 6 lines
drivers/pwm/pwm-sirf.c +6
-0+{ +u32 period_cycles, high_cycles, low_cycles; +struct sirf_pwm *spwm =to_sirf_pwm_chip (chip); + +/* use OSC to generate PWM signals */ + period_cycles =sirf_pwm_ns_to_cycles (chip, period_ns);
Again, how is this OSC specific?
drivers/pwm/pwm-sirf.c line 81
Quoted 12 lines
drivers/pwm/pwm-sirf.c +12
-0+if (period_cycles ==1 ) +return -EINVAL ; + + high_cycles =sirf_pwm_ns_to_cycles (chip, duty_ns); + low_cycles = period_cycles - high_cycles; + +mutex_lock (&spwm->mutex); + +if (high_cycles == period_cycles) { + high_cycles--; + low_cycles =1 ; + }
This perhaps could use a comment. Why is it that the hardware can't be programmed to high_cycles == period_cycles?
Also since you're only accessing local variables here you can safely move this block out of the lock.
drivers/pwm/pwm-sirf.c line 105
Quoted 13 lines
drivers/pwm/pwm-sirf.c +13
-0+static int sirf_pwm_enable (struct pwm_chip *chip,struct pwm_device *pwm) +{ +struct sirf_pwm *spwm =to_sirf_pwm_chip (chip); +u32 val; + +mutex_lock (&spwm->mutex); + +/* 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*/
Missing space between "preclk" and "*/".
drivers/pwm/pwm-sirf.c line 108
Quoted 3 lines
drivers/pwm/pwm-sirf.c +3
-0+ val =readl (spwm->base +SIRF_PWM_SELECT_PRECLK ); + val &= ~(0x1 << (BYPASS_MODE_BIT + pwm->hwpwm)); + val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm));
Perhaps the source field is used to configure which clock is used? In that case I suggest you reshuffle the comments a bit. For example I think it would be clearer if you stated in the file's header comment that the driver currently only works if the OSC is used as the signal source clock and make a comment here that this selects OSC.
One thing that worries me slightly is that there's no way we can determine from a struct clk what clock it actually is. So it's impossible to derive from a struct clk that it is indeed the OSC and therefore this driver has no means to be extensible in the future.
But I have no idea how to solve this properly. Perhaps this would somehow need to be modeled within the clock framework?
Quoted 6 lines
drivers/pwm/pwm-sirf.c +6
-0+static int sirf_pwm_probe (struct platform_device *pdev) +{ +struct sirf_pwm *spwm; +struct resource *mem_res; +int ret; +
drivers/pwm/pwm-sirf.c line 183
Quoted 10 lines
drivers/pwm/pwm-sirf.c +10
-0+ spwm =devm_kzalloc (&pdev->dev,sizeof (*spwm),GFP_KERNEL ); +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 ;
devm_ioremap_resource() returns an ERR_PTR() encoded error on failure, so the proper way to check for this is:
if (IS_ERR(spwm->base)) return PTR_ERR(spwm->base);
drivers/pwm/pwm-sirf.c line 188
Quoted 4 lines
drivers/pwm/pwm-sirf.c +4
-0+/* +* clock for PWM controller +*/ + spwm->pwmc_clk =devm_clk_get (&pdev->dev,"pwmc" );
I think you could drop the pwmc_ prefix, but I don't feel too strongly about that.
drivers/pwm/pwm-sirf.c line 200
Quoted 12 lines
drivers/pwm/pwm-sirf.c +12
-0+if (IS_ERR (spwm->pwmc_clk)) { +dev_err (&pdev->dev,"failed to get PWM controller clock\n" ); +return PTR_ERR (spwm->pwmc_clk); + } + + ret =clk_prepare_enable (spwm->pwmc_clk); +if (ret) +return ret; + +/* +* clocks to generate PWM signals, we use OSC with 26MHz +*/
Only a single clock is requested here, so "clock to generate...". And again there's a reference to OSC and 26 MHz here, but the code is completely generic and it could in fact be any other clock.
drivers/pwm/pwm-sirf.c line 206
Quoted 6 lines
drivers/pwm/pwm-sirf.c +6
-0+ spwm->sigsrc_clk =devm_clk_get (&pdev->dev,"sigsrc0" ); +if (IS_ERR (spwm->sigsrc_clk)) { +dev_err (&pdev->dev,"failed to get PWM signal source clock0\n" ); + ret =PTR_ERR (spwm->sigsrc_clk); + goto err_src_clk; + }
Why is this called "sigsrc0" if there's only one?
drivers/pwm/pwm-sirf.c line 212
Quoted 1 line
drivers/pwm/pwm-sirf.c +1
-0+ spwm->sigsrc_clk_rate =clk_get_rate (spwm->sigsrc_clk);
I guess this could be no issue at all if sigsrc_clk indeed always is OSC, but caching the rate of the source clock is bad because it gives you no chance at all to adapt to clock rate changes later on (if that even happens). So in order not to set a bad example, I'd rather see this not cached, but queried when needed.
drivers/pwm/pwm-sirf.c line 216
Quoted 3 lines
drivers/pwm/pwm-sirf.c +3
-0+ spwm->chip.dev = &pdev->dev; + spwm->chip.ops = &sirf_pwm_ops; + spwm->chip.base =0 ;
I think you should want to set this to -1.
drivers/pwm/pwm-sirf.c line 223
Quoted 1 line
drivers/pwm/pwm-sirf.c +1
-0+dev_err (&pdev->dev,"failed to register PWM\n" );
"PWM chip", please.
drivers/pwm/pwm-sirf.c line 245
Quoted 9 lines
drivers/pwm/pwm-sirf.c +9
-0+static int sirf_pwm_remove (struct platform_device *pdev) +{ +struct sirf_pwm *spwm =platform_get_drvdata (pdev); + +clk_disable_unprepare (spwm->pwmc_clk); +clk_disable_unprepare (spwm->sigsrc_clk); + +return pwmchip_remove (&spwm->chip); +}
Don't you want to remove the chip first, before disabling all the clocks? pwmchip_remove() may end up accessing registers that need the clock enabled.
drivers/pwm/pwm-sirf.c line 256
Quoted 9 lines
drivers/pwm/pwm-sirf.c +9
-0+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->pwmc_clk); + +return 0 ; +}
Why doesn't this disable (and unprepare) the source clock?
drivers/pwm/pwm-sirf.c line 266
Quoted 9 lines
drivers/pwm/pwm-sirf.c +9
-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,
pwm -> PWM, please.
drivers/pwm/pwm-sirf.c line 296
Quoted 11 lines
drivers/pwm/pwm-sirf.c +11
-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); + +return 0 ; +} + +#else
Gratuitous blank line above this one.
drivers/pwm/pwm-sirf.c line 324
Quoted 11 lines
drivers/pwm/pwm-sirf.c +11
-0+static struct platform_driver sirf_pwm_driver = { + .driver = { + .name ="sirf-pwm" , + .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);
Another gratuitous blank line above this one.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/4da5760d/attachment-0001.sig>
PATCH v4 1/4 pwm: add CSR SiRFSoC PWM driver
2014-03-19 5:03 GMT+08:00 Thierry Reding <[email protected]>:
Quoted 20 lines · Barry Song
Quoted 7 lines
From: Huayi Li <[email protected]>
PWM controller of CSR SiRFSoC can generate 7 independent outputs. Each output duty cycle can be adjusted by setting the corresponding wait & hold registers. There are 6 external channels (0 to 5) and 1 internal channel (6). Supports a wide frequency range: the source clock divider can be from 2 up to 65536*2.
This commit message is too wide. Please always wrap the commit message at 72 characters. See this for reference[0].
[0]: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
Quoted 1 line
drivers/pwm/Kconfig +0
-0[...]
drivers/pwm/Kconfig config PWM_SAMSUNG · line 181
Quoted 3 lines
drivers/pwm/Kconfig +3
-0+config PWM_SIRF + tristate "SiRF PWM support" + depends on (ARCH_SIRF || COMPILE_TEST) && COMMON_CLK && OF
Quoted 5 lines
I'd prefer these to be split into more lines for readability, like so:
depends on
ARCH_SIRF||COMPILE_TEST
depends onCOMMON_CLK
depends on OF
ok
Quoted 9 lines
Quoted 2 lines
drivers/pwm/pwm-sirf.c +0
-0[...]
Quoted 3 lines
+ * Licensed under GPLv2.
+ */
+#include <linux/kernel.h>Could use a blank line between the above two, visually separating the header comment from the list of include files.
ok.
Quoted 13 lines
drivers/pwm/pwm-sirf.c line 17
Quoted 10 lines
drivers/pwm/pwm-sirf.c +9
-0+#include <linux/init.h> +#include <linux/module.h> +#include <linux/device.h> +#include <linux/platform_device.h> +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/pwm.h> +#include <linux/of.h> +#include <linux/io.h> Also please keep these sorted alphabetically.
Quoted 4 lines
drivers/pwm/pwm-sirf.c line 34
Quoted 2 lines
drivers/pwm/pwm-sirf.c +2
-0+struct sirf_pwm { +struct pwm_chip chip;There's a tab between
pwm_chipand chip here. It should be a space.
ok
Quoted 9 lines
drivers/pwm/pwm-sirf.c line 39
Quoted 6 lines
drivers/pwm/pwm-sirf.c +5
-0+struct mutex mutex; +void __iomem *base; +struct clk *pwmc_clk; +/* select OSC(26MHz) as clock source to generate PWM signals */ +struct clk *sigsrc_clk;Maybe I'm missing something, but what in this driver is selecting the OSC as clock source?
it can select one of rtc(32KHz), osc(26MHz) and 3 plls in several hundreds Hz. 26MHz is a fixed one which can generate a wide range of pwm waves.
and the PWM also supports bypass, which can bypass the above five to clock out directly.
Quoted 5 lines
Quoted 2 lines
+ unsigned long
sigsrc_clk_rate;Why do you cache this value? Can't you simply query the clock framework for it when you need it?
since the freq is fixed, we cache it now. but we can drop the cache.
Quoted 16 lines
Quoted 4 lines
drivers/pwm/pwm-sirf.c +3
-0+static u32 sirf_pwm_ns_to_cycles (struct pwm_chip *chip,u32 time_ns) +{ +struct sirf_pwm *spwm =to_sirf_pwm_chip (chip);drivers/pwm/pwm-sirf.c line 60
Quoted 10 lines
drivers/pwm/pwm-sirf.c +10
-0+u64 dividend; +u32 cycle; + + dividend = (u64 )spwm->sigsrc_clk_rate * time_ns +NSEC_PER_SEC /2 ; +do_div (dividend,NSEC_PER_SEC ); + + cycle = dividend; + +return cycle >1 ? cycle :1 ; +}I think you can do without the cycle variable here.
right
Quoted 6 lines
drivers/pwm/pwm-sirf.c line 63
Quoted 3 lines
drivers/pwm/pwm-sirf.c +2
-0+static int sirf_pwm_config (struct pwm_chip *chip,struct pwm_device *pwm, +int duty_ns,int period_ns)Please always align the first argument on subsequent lines with the first argument on the first line.
ok
Quoted 9 lines
drivers/pwm/pwm-sirf.c line 69
Quoted 7 lines
drivers/pwm/pwm-sirf.c +6
-0+{ +u32 period_cycles, high_cycles, low_cycles; +struct sirf_pwm *spwm =to_sirf_pwm_chip (chip); + +/* use OSC to generate PWM signals */ + period_cycles =sirf_pwm_ns_to_cycles (chip, period_ns);Again, how is this OSC specific?
as above
Quoted 16 lines
drivers/pwm/pwm-sirf.c line 81
Quoted 13 lines
drivers/pwm/pwm-sirf.c +12
-0+if (period_cycles ==1 ) +return -EINVAL ; + + high_cycles =sirf_pwm_ns_to_cycles (chip, duty_ns); + low_cycles = period_cycles - high_cycles; + +mutex_lock (&spwm->mutex); + +if (high_cycles == period_cycles) { + high_cycles--; + low_cycles =1 ; + }This perhaps could use a comment. Why is it that the hardware can't be programmed to
high_cycles==period_cycles?
high=period means that is not a wave but a fixed high voltage level.
Quoted 4 lines
Also since you're only accessing local variables here you can safely move this block out of the lock.
Quoted 15 lines
drivers/pwm/pwm-sirf.c line 105
Quoted 13 lines
drivers/pwm/pwm-sirf.c +13
-0+static int sirf_pwm_enable (struct pwm_chip *chip,struct pwm_device *pwm) +{ +struct sirf_pwm *spwm =to_sirf_pwm_chip (chip); +u32 val; + +mutex_lock (&spwm->mutex); + +/* 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*/ Missing space between "preclk" and "*/".
will fix.
Quoted 10 lines
drivers/pwm/pwm-sirf.c line 108
Quoted 4 lines
drivers/pwm/pwm-sirf.c +3
-0+ val =readl (spwm->base +SIRF_PWM_SELECT_PRECLK ); + val &= ~(0x1 << (BYPASS_MODE_BIT + pwm->hwpwm)); + val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm));Perhaps the source field is used to configure which clock is used? In that case I suggest you reshuffle the comments a bit. For example I think it would be clearer if you stated in the file's header comment that the driver currently only works if the OSC is used as the signal source clock and make a comment here that this selects OSC.
well. that is the plan for the 1st step.
Quoted 5 lines
One thing that worries me slightly is that there's no way we can determine from a struct clk what clock it actually is. So it's impossible to derive from a struct clk that it is indeed the OSC and therefore this driver has no means to be extensible in the future.
we might extend, for example, using 32KHz as source, we can get a very low frequency wave.
Quoted 3 lines
But I have no idea how to solve this properly. Perhaps this would somehow need to be modeled within the clock framework?
since this pwm can also bypass, bypass is actually a clock out not a pwm out. so hooking into clock framework might be good to do in the next step
Quoted 21 lines
Quoted 7 lines
drivers/pwm/pwm-sirf.c +6
-0+static int sirf_pwm_probe (struct platform_device *pdev) +{ +struct sirf_pwm *spwm; +struct resource *mem_res; +int ret; +drivers/pwm/pwm-sirf.c line 183
Quoted 10 lines
drivers/pwm/pwm-sirf.c +10
-0+ spwm =devm_kzalloc (&pdev->dev,sizeof (*spwm),GFP_KERNEL ); +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 ;
devm_ioremap_resource()returns anERR_PTR()encoded error on failure, so the proper way to check for this is:
Quoted 2 lines
if (IS_ERR(spwm->base))
return PTR_ERR(spwm->base);real.
Quoted 8 lines
drivers/pwm/pwm-sirf.c line 188
Quoted 5 lines
drivers/pwm/pwm-sirf.c +4
-0+/* +* clock for PWM controller +*/ + spwm->pwmc_clk =devm_clk_get (&pdev->dev,"pwmc" );I think you could drop the
pwmc_prefix, but I don't feel too strongly about that.
if there is no other clock as signal source, it is good. now there is another one, that is why this clock get prefix.
Quoted 17 lines
drivers/pwm/pwm-sirf.c line 200
Quoted 13 lines
drivers/pwm/pwm-sirf.c +12
-0+if (IS_ERR (spwm->pwmc_clk)) { +dev_err (&pdev->dev,"failed to get PWM controller clock\n" ); +return PTR_ERR (spwm->pwmc_clk); + } + + ret =clk_prepare_enable (spwm->pwmc_clk); +if (ret) +return ret; + +/* +* clocks to generate PWM signals, we use OSC with 26MHz +*/ Only a single clock is requested here, so "clock to generate...". And again there's a reference to OSC and 26 MHz here, but the code is completely generic and it could in fact be any other clock.
right.
Quoted 9 lines
drivers/pwm/pwm-sirf.c line 206
Quoted 7 lines
drivers/pwm/pwm-sirf.c +6
-0+ spwm->sigsrc_clk =devm_clk_get (&pdev->dev,"sigsrc0" ); +if (IS_ERR (spwm->sigsrc_clk)) { +dev_err (&pdev->dev,"failed to get PWM signal source clock0\n" ); + ret =PTR_ERR (spwm->sigsrc_clk); + goto err_src_clk; + }Why is this called "sigsrc0" if there's only one?
there are five, OSC is the first one in the HW index.
Quoted 8 lines
Quoted 2 lines
+
spwm->sigsrc_clk_rate=clk_get_rate(spwm->sigsrc_clk);I guess this could be no issue at all if
sigsrc_clkindeed always is OSC, but caching the rate of the source clock is bad because it gives you no chance at all to adapt to clock rate changes later on (if that even happens). So in order not to set a bad example, I'd rather see this not cached, but queried when needed.
ok. understood.
Quoted 6 lines
drivers/pwm/pwm-sirf.c line 216
Quoted 4 lines
drivers/pwm/pwm-sirf.c +3
-0+ spwm->chip.dev = &pdev->dev; + spwm->chip.ops = &sirf_pwm_ops; + spwm->chip.base =0 ;I think you should want to set this to -1.
ok
Quoted 5 lines
Quoted 2 lines
+
dev_err(&pdev->dev, "failed to register PWM\n");"PWM chip", please.
ok
Quoted 5 lines
Quoted 5 lines
drivers/pwm/pwm-sirf.c +5
-0+static int sirf_pwm_remove (struct platform_device *pdev) +{ +struct sirf_pwm *spwm =platform_get_drvdata (pdev); + +clk_disable_unprepare (spwm->pwmc_clk);
Quoted 8 lines
drivers/pwm/pwm-sirf.c line 245
Quoted 4 lines
drivers/pwm/pwm-sirf.c +4
-0+clk_disable_unprepare (spwm->sigsrc_clk); + +return pwmchip_remove (&spwm->chip); +}Don't you want to remove the chip first, before disabling all the clocks?
pwmchip_remove()may end up accessing registers that need the clock enabled.
i will check this issue. it seems generally we disable hw before removing SW to avoid SW get more events like interrupts.
Quoted 12 lines
drivers/pwm/pwm-sirf.c line 256
Quoted 10 lines
drivers/pwm/pwm-sirf.c +9
-0+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->pwmc_clk); + +return 0 ; +}Why doesn't this disable (and unprepare) the source clock?
we can have.
Quoted 12 lines
drivers/pwm/pwm-sirf.c line 266
Quoted 10 lines
drivers/pwm/pwm-sirf.c +9
-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, pwm -> PWM, please.
ok
Quoted 14 lines
drivers/pwm/pwm-sirf.c line 296
Quoted 12 lines
drivers/pwm/pwm-sirf.c +11
-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); + +return 0 ; +} + +#else Gratuitous blank line above this one.
ok
Quoted 15 lines
drivers/pwm/pwm-sirf.c line 324
Quoted 12 lines
drivers/pwm/pwm-sirf.c +11
-0+static struct platform_driver sirf_pwm_driver = { + .driver = { + .name ="sirf-pwm" , + .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);Another gratuitous blank line above this one.
ok
Quoted 1 line
Thierry
-barry