pwm: atmel: add Timer Counter Block PWM driver
Subsystems (3)
PATCH RESEND v4 1/1 pwm: atmel: add Timer Counter Block PWM driver
Hi,
Sorry for resend. The previous version still has alignment issues on atmel_tcb_pwm_set_polarity, atmel_tcb_pwm_request and atmel_tcb_pwm_config function parameters.
This patch adds a PWM driver based on Atmel Timer Counter Block. Timer Counter Block is used in Waveform generator mode.
A Timer Counter Block provides up to 6 PWM devices grouped by 2:
* group 0 = PWM 0 and 1
* group 1 = PWM 1 and 2
* group 2 = PMW 3 and 4
PWM devices in a given group must be configured with the same period value.
If a PWM device in a group tries to change the period value and the other device is already configured with a different value an error will be returned.
This driver requires device tree support.
The Timer Counter Block number used to create a PWM chip is given by tc-block field in an "atmel,tcb-pwm" compatible node.
This patch was tested on kizbox board (at91sam9g20 SoC) with pwm-leds.
Regards,
Boris
---
Changes since v1:
- Fix device tree binding Documentation
- Fix Kconfig issues (missing OF dependency, deprecated HAVE_PWM select, ...)
- Fix various coding style issues.
- Cleanup code and add some comments.
Changes since v2:
- Replace kzalloc/kfree with managed versions (devm_kzalloc/devm_kfree).
- Add one cell to device tree binding to support polarity flag.
- Replace min computation (2 div -> 1 mul + 1 div).
Changes since v3:
- Fix device tree binding Documentation
- Fix Kconfig description
- Fix coding style issues (function parameters alignment)
- Replace 1000000000 value with NSEC_PER_SEC macro
- Get rid of newcmr variable in enable/disable functions
- Remove unneeded devm_kfree
- Add missing atmel_tc_free
.../devicetree/bindings/pwm/atmel-tcb-pwm.txt | 18 + drivers/pwm/Kconfig | 12 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-atmel-tcb.c | 428 ++++++++++++++++++++ 4 files changed, 459 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt create mode 100644 drivers/pwm/pwm-atmel-tcb.c
Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt new file +18 -0
@@ -0,0 +1,18 @@ +Atmel TCB PWM controller + +Required properties: +- compatible: should be "atmel,tcb-pwm" +- #pwm-cells: Should be 3. The first cell specifies the per-chip index + of the PWM to use, the second cell is the period in nanoseconds and + bit 0 in the third cell is used to encode the polarity of PWM output. + Set bit 0 of the third cell in PWM specifier to 1 for inverse polarity & + set to 0 for normal polarity. +- tc-block: The Timer Counter block to use as a PWM chip. + +Example: + +pwm { + compatible = "atmel,tcb-pwm"; + #pwm-cells = <3>; + tc-block = <1>; +};
drivers/pwm/Kconfig +12 -0
@@ -37,6 +37,18 @@ config PWM_AB8500 To compile this driver as a module, choose M here: the module will be called pwm-ab8500. +config PWM_ATMEL_TCB + tristate "TC Block PWM" + depends on ATMEL_TCLIB && OF + help + Generic PWM framework driver for Atmel Timer Counter Block. + + A Timer Counter Block provides 6 PWM devices grouped by 2. + Devices in a given group must have the same period. + + To compile this driver as a module, choose M here: the module + will be called pwm-atmel-tcb. + config PWM_BFIN tristate "Blackfin PWM support" depends on BFIN_GPTIMERS
drivers/pwm/Makefile +1 -0
@@ -1,5 +1,6 @@ obj-$(CONFIG_PWM) += core.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_IMX) += pwm-imx.o obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
drivers/pwm/pwm-atmel-tcb.c new file +428 -0
428 lines not on this page · raw message
Signature · 1 line
1.7.0.4
Quoted 6 lines · Boris BREZILLON
Hi,
Sorry for resend. The previous version still has alignment issues on
atmel_tcb_pwm_set_polarity,atmel_tcb_pwm_requestandatmel_tcb_pwm_configfunction parameters.
Quoted 7 lines
This patch adds a PWM driver based on Atmel Timer Counter Block. Timer Counter Block is used in Waveform generator mode.
A Timer Counter Block provides up to 6 PWM devices grouped by 2:
* group 0 = PWM 0 and 1
* group 1 = PWM 1 and 2
* group 2 = PMW 3 and 4
Should this be "PWM 2 and 3" and "PWM 4 and 5"? Or is PWM 1 shared between groups 0 and 1?
Quoted 3 lines
drivers/pwm/pwm-atmel-tcb.c new file +3
-0+static int atmel_tcb_pwm_request (struct pwm_chip *chip, +struct pwm_device *pwm) +{
[...]
drivers/pwm/pwm-atmel-tcb.c line 109
Quoted 1 line
drivers/pwm/pwm-atmel-tcb.c new file +1
-0+clk_enable (tc->clk[group]);
You need to check the return value of clk_enable(). There's always a small possibility that it may fail.
Quoted 2 lines
(quoted diff) +2
-0+static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{
[...]
drivers/pwm/pwm-atmel-tcb.c line 139
Quoted 3 lines
drivers/pwm/pwm-atmel-tcb.c new file +3
-0+/* If duty is 0 reverse polarity */ +if (tcbpwm->duty ==0 ) + polarity = !polarity;
Rather than commenting on what the code does, this should say why it does so.
Quoted 2 lines
drivers/pwm/pwm-atmel-tcb.c new file +2
-0+static int atmel_tcb_pwm_enable (struct pwm_chip *chip,struct pwm_device *pwm) +{
[...]
drivers/pwm/pwm-atmel-tcb.c line 139
Quoted 3 lines
drivers/pwm/pwm-atmel-tcb.c new file +3
-0+/* If duty is 0 reverse polarity */ +if (tcbpwm->duty ==0 ) + polarity = !polarity;
Same here.
Quoted 2 lines
(quoted diff) +2
-0+static int atmel_tcb_pwm_probe(struct platform_device *pdev) +{
[...]
drivers/pwm/pwm-atmel-tcb.c line 356
Quoted 10 lines
drivers/pwm/pwm-atmel-tcb.c new file +10
-0+struct atmel_tcb_pwm_chip *tcbpwm; +struct device_node *np = pdev->dev.of_node; +struct atmel_tc *tc; +int err; +int tcblock; + + err =of_property_read_u32 (np,"tc-block" , &tcblock); +if (err <0 ) { + dev_err(&pdev->dev, + "failed to get tc block number from device tree (error: %d)\n",
Maybe: "failed to get Timer Counter Block number..." to make it consistent with the error message below:
Quoted 5 lines
drivers/pwm/pwm-atmel-tcb.c new file +5
-0+ tc =atmel_tc_alloc (tcblock,"tcb-pwm" ); +if (tc ==NULL ) { +dev_err (&pdev->dev,"failed to allocate Timer Counter Block\n" ); +return -ENOMEM ; + }
[...]
drivers/pwm/pwm-atmel-tcb.c line 413
Quoted 5 lines
drivers/pwm/pwm-atmel-tcb.c new file +5
-0+static const struct of_device_id atmel_tcb_pwm_dt_ids[] = { + { .compatible ="atmel,tcb-pwm" , }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, mxs_pwm_dt_ids);
This is still wrong.
drivers/pwm/pwm-atmel-tcb.c line 427
Quoted 13 lines
drivers/pwm/pwm-atmel-tcb.c new file +13
-0+static struct platform_driver atmel_tcb_pwm_driver = { + .driver = { + .name ="atmel-tcb-pwm" , + .of_match_table = atmel_tcb_pwm_dt_ids, + }, + .probe = atmel_tcb_pwm_probe, + .remove = atmel_tcb_pwm_remove, +}; +module_platform_driver (atmel_tcb_pwm_driver); + +MODULE_AUTHOR ("Boris BREZILLON <[email protected]>" ); +MODULE_DESCRIPTION ("Atmel Timer Counter Pulse Width Modulation Driver" ); +MODULE_ALIAS ("platform:atmel-tcb-pwm" );
I don't think you needMODULE_ALIAS() if the alias is the same as the driver name.
Thierry
Quoted 10 lines · Thierry Reding
Quoted 9 lines · Boris BREZILLON
Hi,
Sorry for resend. The previous version still has alignment issues on
atmel_tcb_pwm_set_polarity,atmel_tcb_pwm_requestandatmel_tcb_pwm_configfunction parameters.This patch adds a PWM driver based on Atmel Timer Counter Block. Timer Counter Block is used in Waveform generator mode.
Quoted 7 lines
Quoted 4 lines
A Timer Counter Block provides up to 6 PWM devices grouped by 2:
* group 0 = PWM 0 and 1
* group 1 = PWM 1 and 2
* group 2 = PMW 3 and 4Should this be "PWM 2 and 3" and "PWM 4 and 5"? Or is PWM 1 shared between groups 0 and 1?
This is a mistake, this should be:
* group 0 = PWM 0 and 1
* group 1 = PWM 2 and 3
* group 2 = PMW 4 and 5
Quoted 13 lines
Quoted 4 lines
drivers/pwm/pwm-atmel-tcb.c new file +3
-0+static int atmel_tcb_pwm_request (struct pwm_chip *chip, +struct pwm_device *pwm) +{[...]
drivers/pwm/pwm-atmel-tcb.c line 109
Quoted 1 line
drivers/pwm/pwm-atmel-tcb.c new file +1
-0+clk_enable (tc->clk[group]);You need to check the return value of
clk_enable(). There's always a small possibility that it may fail.Quoted 2 lines
(quoted diff) +2
-0+static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{[...]
Quoted 7 lines
drivers/pwm/pwm-atmel-tcb.c line 139
Quoted 3 lines
drivers/pwm/pwm-atmel-tcb.c new file +3
-0+/* If duty is 0 reverse polarity */ +if (tcbpwm->duty ==0 ) + polarity = !polarity;Rather than commenting on what the code does, this should say why it does so.
Is this an acceptable explanation ?
/*
* If duty is 0 the timer will be stopped and we have to
* configure the output correctly on software trigger:
* - set output to high if PWM_POLARITY_INVERSED
* - set output to low if PWM_POLARITY_NORMAL *
* This is why we're reverting polarity in this case. */
Quoted 59 lines
Quoted 2 lines
drivers/pwm/pwm-atmel-tcb.c new file +2
-0+static int atmel_tcb_pwm_enable (struct pwm_chip *chip,struct pwm_device *pwm) +{[...]
drivers/pwm/pwm-atmel-tcb.c line 139
Quoted 3 lines
drivers/pwm/pwm-atmel-tcb.c new file +3
-0+/* If duty is 0 reverse polarity */ +if (tcbpwm->duty ==0 ) + polarity = !polarity;Same here.
Quoted 2 lines
(quoted diff) +2
-0+static int atmel_tcb_pwm_probe(struct platform_device *pdev) +{[...]
drivers/pwm/pwm-atmel-tcb.c line 356
Quoted 10 lines
drivers/pwm/pwm-atmel-tcb.c new file +10
-0+struct atmel_tcb_pwm_chip *tcbpwm; +struct device_node *np = pdev->dev.of_node; +struct atmel_tc *tc; +int err; +int tcblock; + + err =of_property_read_u32 (np,"tc-block" , &tcblock); +if (err <0 ) { + dev_err(&pdev->dev, + "failed to get tc block number from device tree (error: %d)\n",Maybe: "failed to get Timer Counter Block number..." to make it consistent with the error message below:
Quoted 5 lines
drivers/pwm/pwm-atmel-tcb.c new file +5
-0+ tc =atmel_tc_alloc (tcblock,"tcb-pwm" ); +if (tc ==NULL ) { +dev_err (&pdev->dev,"failed to allocate Timer Counter Block\n" ); +return -ENOMEM ; + }[...]
drivers/pwm/pwm-atmel-tcb.c line 413
Quoted 5 lines
drivers/pwm/pwm-atmel-tcb.c new file +5
-0+static const struct of_device_id atmel_tcb_pwm_dt_ids[] = { + { .compatible ="atmel,tcb-pwm" , }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, mxs_pwm_dt_ids);This is still wrong.
drivers/pwm/pwm-atmel-tcb.c line 427
Quoted 13 lines
drivers/pwm/pwm-atmel-tcb.c new file +13
-0+static struct platform_driver atmel_tcb_pwm_driver = { + .driver = { + .name ="atmel-tcb-pwm" , + .of_match_table = atmel_tcb_pwm_dt_ids, + }, + .probe = atmel_tcb_pwm_probe, + .remove = atmel_tcb_pwm_remove, +}; +module_platform_driver (atmel_tcb_pwm_driver); + +MODULE_AUTHOR ("Boris BREZILLON <[email protected]>" ); +MODULE_DESCRIPTION ("Atmel Timer Counter Pulse Width Modulation Driver" ); +MODULE_ALIAS ("platform:atmel-tcb-pwm" );I don't think you
needMODULE_ALIAS()if the alias is the same as the driver name.Thierry
Quoted 2 lines · Boris BREZILLON
Quoted 1 line · Thierry Reding
On Thu, Dec 20, 2012 at 10:12:56AM +0100, Boris BREZILLON wrote:
[...]
Quoted 13 lines
Quoted 10 lines
Quoted 2 lines
(quoted diff) +2
-0+static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{[...]
drivers/pwm/pwm-atmel-tcb.c line 139
Quoted 3 lines
drivers/pwm/pwm-atmel-tcb.c new file +3
-0+/* If duty is 0 reverse polarity */ +if (tcbpwm->duty ==0 ) + polarity = !polarity;Rather than commenting on what the code does, this should say why it does so.
Is this an acceptable explanation ?
Quoted 8 lines
/*
* If duty is 0 the timer will be stopped and we have to
* configure the output correctly on software trigger:
* - set output to high ifPWM_POLARITY_INVERSED
* - set output to low ifPWM_POLARITY_NORMAL*
* This is why we're reverting polarity in this case. */
Yes, that should work.
Thierry
Quoted 49 lines · Thierry Reding
Quoted 6 lines · Boris BREZILLON
Hi,
Sorry for resend. The previous version still has alignment issues on
atmel_tcb_pwm_set_polarity,atmel_tcb_pwm_requestandatmel_tcb_pwm_configfunction parameters.Quoted 7 lines
This patch adds a PWM driver based on Atmel Timer Counter Block. Timer Counter Block is used in Waveform generator mode.
A Timer Counter Block provides up to 6 PWM devices grouped by 2:
* group 0 = PWM 0 and 1
* group 1 = PWM 1 and 2
* group 2 = PMW 3 and 4Should this be "PWM 2 and 3" and "PWM 4 and 5"? Or is PWM 1 shared between groups 0 and 1?
Quoted 3 lines
drivers/pwm/pwm-atmel-tcb.c new file +3
-0+static int atmel_tcb_pwm_request (struct pwm_chip *chip, +struct pwm_device *pwm) +{[...]
drivers/pwm/pwm-atmel-tcb.c line 109
Quoted 1 line
drivers/pwm/pwm-atmel-tcb.c new file +1
-0+clk_enable (tc->clk[group]);You need to check the return value of
clk_enable(). There's always a small possibility that it may fail.Quoted 2 lines
(quoted diff) +2
-0+static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{[...]
drivers/pwm/pwm-atmel-tcb.c line 139
Quoted 3 lines
drivers/pwm/pwm-atmel-tcb.c new file +3
-0+/* If duty is 0 reverse polarity */ +if (tcbpwm->duty ==0 ) + polarity = !polarity;Rather than commenting on what the code does, this should say why it does so.
Quoted 2 lines
drivers/pwm/pwm-atmel-tcb.c new file +2
-0+static int atmel_tcb_pwm_enable (struct pwm_chip *chip,struct pwm_device *pwm) +{[...]
drivers/pwm/pwm-atmel-tcb.c line 139
Quoted 3 lines
drivers/pwm/pwm-atmel-tcb.c new file +3
-0+/* If duty is 0 reverse polarity */ +if (tcbpwm->duty ==0 ) + polarity = !polarity;Same here.
Quoted 2 lines
(quoted diff) +2
-0+static int atmel_tcb_pwm_probe(struct platform_device *pdev) +{[...]
Quoted 13 lines
drivers/pwm/pwm-atmel-tcb.c line 356
Quoted 10 lines
drivers/pwm/pwm-atmel-tcb.c new file +10
-0+struct atmel_tcb_pwm_chip *tcbpwm; +struct device_node *np = pdev->dev.of_node; +struct atmel_tc *tc; +int err; +int tcblock; + + err =of_property_read_u32 (np,"tc-block" , &tcblock); +if (err <0 ) { + dev_err(&pdev->dev, + "failed to get tc block number from device tree (error: %d)\n",Maybe: "failed to get Timer Counter Block number..." to make it consistent with the error message below:
Do I have to break the error string so that the line does not exceed 80 characters ? Checkpath script does not complain about it, and the CodingStyle file specify that visible strings should not be broken...
Same question applies to this error, which I converted to a multi-line error in a previous patch version:
dev_err(chip->dev, "failed to configure period_ns:\n" "the other PWM device in this group is already\n" "configured with a different period_ns value\n");
Quoted 34 lines
Quoted 6 lines
drivers/pwm/pwm-atmel-tcb.c new file +5
-0+ tc =atmel_tc_alloc (tcblock,"tcb-pwm" ); +if (tc ==NULL ) { +dev_err (&pdev->dev,"failed to allocate Timer Counter Block\n" ); +return -ENOMEM ; + }[...]
drivers/pwm/pwm-atmel-tcb.c line 413
Quoted 5 lines
drivers/pwm/pwm-atmel-tcb.c new file +5
-0+static const struct of_device_id atmel_tcb_pwm_dt_ids[] = { + { .compatible ="atmel,tcb-pwm" , }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, mxs_pwm_dt_ids);This is still wrong.
drivers/pwm/pwm-atmel-tcb.c line 427
Quoted 13 lines
drivers/pwm/pwm-atmel-tcb.c new file +13
-0+static struct platform_driver atmel_tcb_pwm_driver = { + .driver = { + .name ="atmel-tcb-pwm" , + .of_match_table = atmel_tcb_pwm_dt_ids, + }, + .probe = atmel_tcb_pwm_probe, + .remove = atmel_tcb_pwm_remove, +}; +module_platform_driver (atmel_tcb_pwm_driver); + +MODULE_AUTHOR ("Boris BREZILLON <[email protected]>" ); +MODULE_DESCRIPTION ("Atmel Timer Counter Pulse Width Modulation Driver" ); +MODULE_ALIAS ("platform:atmel-tcb-pwm" );I don't think you
needMODULE_ALIAS()if the alias is the same as the driver name.Thierry
Quoted 2 lines · Boris BREZILLON
Do I have to break the error string so that the line does not exceed 80 characters ?
No.
Quoted 2 lines
Checkpath script does not complain about it, and the CodingStyle file specify that visible strings should not be broken...
Correct.
Quoted 7 lines
Same question applies to this error, which I converted to a multi-line error in a previous patch version:
dev_err(chip->dev, "failed to configure period_ns:\n" "the other PWM device in this group is already\n" "configured with a different period_ns value\n");
Which is a bad idea. It appears in log files as multiple lines, which makes parsing the error for analysis difficult (eg, you may have a log analyser which tells you how many times an error occurs - the above would be treated as three separate errors.