Re: [PATCH v4 RESEND] pwm: atmel: add Timer Counter Block PWM driver
On 08/01/2013 08:10, Thierry Reding wrote:
> On Thu, Dec 20, 2012 at 10:12:56AM +0100, Boris BREZILLON wrote:
>> 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
>
> Should this be "PWM 2 and 3" and "PWM 4 and 5"? Or is PWM 1 shared
> between groups 0 and 1?
>
>> +static int atmel_tcb_pwm_request(struct pwm_chip *chip,
>> + struct pwm_device *pwm)
>> +{
> [...]
>> +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.
>
>> +static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device
>> *pwm)
>> +{
> [...]
>> +/* 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.
>
>> +static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device
>> *pwm)
>> +{
> [...]
>> +/* If duty is 0 reverse polarity */
>> +if (tcbpwm->duty == 0)
>> +polarity = !polarity;
>
> Same here.
>
>> +static int atmel_tcb_pwm_probe(struct platform_device *pdev)
>> +{
> [...]
>> +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", );
>> +if (err < 0) {
>> +dev_err(>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");
>
>> +tc = atmel_tc_alloc(tcblock, "tcb-pwm");
>> +if (tc == NULL) {
>> +dev_err(>dev, "failed to allocate Timer Counter Block\n");
>> +return -ENOMEM;
>> +}
> [...]
>> +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.
>
>> +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 ");
>> +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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 RESEND] pwm: atmel: add Timer Counter Block PWM driver
On Tue, Jan 08, 2013 at 04:21:59PM +0100, Boris BREZILLON wrote: > Do I have to break the error string so that the line does not exceed 80 > characters ? No. > Checkpath script does not complain about it, and the CodingStyle file > specify that visible strings should not be broken... Correct. > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 RESEND] pwm: atmel: add Timer Counter Block PWM driver
On 08/01/2013 08:10, Thierry Reding wrote:
> On Thu, Dec 20, 2012 at 10:12:56AM +0100, Boris BREZILLON wrote:
>> 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
>
> Should 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
>
>> +static int atmel_tcb_pwm_request(struct pwm_chip *chip,
>> + struct pwm_device *pwm)
>> +{
> [...]
>> +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.
>
>> +static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device
>> *pwm)
>> +{
> [...]
>> +/* 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.
*/
>> +static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device
>> *pwm)
>> +{
> [...]
>> +/* If duty is 0 reverse polarity */
>> +if (tcbpwm->duty == 0)
>> +polarity = !polarity;
>
> Same here.
>
>> +static int atmel_tcb_pwm_probe(struct platform_device *pdev)
>> +{
> [...]
>> +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", );
>> +if (err < 0) {
>> +dev_err(>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:
>
>> +tc = atmel_tc_alloc(tcblock, "tcb-pwm");
>> +if (tc == NULL) {
>> +dev_err(>dev, "failed to allocate Timer Counter Block\n");
>> +return -ENOMEM;
>> +}
> [...]
>> +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.
>
>> +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 ");
>> +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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 RESEND] pwm: atmel: add Timer Counter Block PWM driver
On Tue, Jan 08, 2013 at 01:43:56PM +0100, Boris BREZILLON wrote:
> On 08/01/2013 08:10, Thierry Reding wrote:
> > On Thu, Dec 20, 2012 at 10:12:56AM +0100, Boris BREZILLON wrote:
[...]
> >> +static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct
> >> pwm_device *pwm)
> >> +{
> > [...]
> >> + /* 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.
>*/
Yes, that should work.
Thierry
pgpkuZvZg2X6P.pgp
Description: PGP signature
Re: [PATCH v4 RESEND] pwm: atmel: add Timer Counter Block PWM driver
On Tue, Jan 08, 2013 at 01:43:56PM +0100, Boris BREZILLON wrote:
On 08/01/2013 08:10, Thierry Reding wrote:
On Thu, Dec 20, 2012 at 10:12:56AM +0100, Boris BREZILLON wrote:
[...]
+static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct
pwm_device *pwm)
+{
[...]
+ /* 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.
*/
Yes, that should work.
Thierry
pgpkuZvZg2X6P.pgp
Description: PGP signature
Re: [PATCH v4 RESEND] pwm: atmel: add Timer Counter Block PWM driver
On 08/01/2013 08:10, Thierry Reding wrote:
On Thu, Dec 20, 2012 at 10:12:56AM +0100, Boris BREZILLON wrote:
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
Should 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
+static int atmel_tcb_pwm_request(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
[...]
+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.
+static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device
*pwm)
+{
[...]
+/* 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.
*/
+static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device
*pwm)
+{
[...]
+/* If duty is 0 reverse polarity */
+if (tcbpwm-duty == 0)
+polarity = !polarity;
Same here.
+static int atmel_tcb_pwm_probe(struct platform_device *pdev)
+{
[...]
+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:
+tc = atmel_tc_alloc(tcblock, tcb-pwm);
+if (tc == NULL) {
+dev_err(pdev-dev, failed to allocate Timer Counter Block\n);
+return -ENOMEM;
+}
[...]
+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.
+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 b.brezil...@overkiz.com);
+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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 RESEND] pwm: atmel: add Timer Counter Block PWM driver
On Tue, Jan 08, 2013 at 04:21:59PM +0100, Boris BREZILLON wrote: Do I have to break the error string so that the line does not exceed 80 characters ? No. Checkpath script does not complain about it, and the CodingStyle file specify that visible strings should not be broken... Correct. 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 RESEND] pwm: atmel: add Timer Counter Block PWM driver
On 08/01/2013 08:10, Thierry Reding wrote:
On Thu, Dec 20, 2012 at 10:12:56AM +0100, Boris BREZILLON wrote:
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
Should this be PWM 2 and 3 and PWM 4 and 5? Or is PWM 1 shared
between groups 0 and 1?
+static int atmel_tcb_pwm_request(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
[...]
+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.
+static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device
*pwm)
+{
[...]
+/* 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.
+static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device
*pwm)
+{
[...]
+/* If duty is 0 reverse polarity */
+if (tcbpwm-duty == 0)
+polarity = !polarity;
Same here.
+static int atmel_tcb_pwm_probe(struct platform_device *pdev)
+{
[...]
+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);
+tc = atmel_tc_alloc(tcblock, tcb-pwm);
+if (tc == NULL) {
+dev_err(pdev-dev, failed to allocate Timer Counter Block\n);
+return -ENOMEM;
+}
[...]
+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.
+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 b.brezil...@overkiz.com);
+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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 RESEND] pwm: atmel: add Timer Counter Block PWM driver
On Thu, Dec 20, 2012 at 10:12:56AM +0100, Boris BREZILLON wrote:
> 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
Should this be "PWM 2 and 3" and "PWM 4 and 5"? Or is PWM 1 shared
between groups 0 and 1?
> +static int atmel_tcb_pwm_request(struct pwm_chip *chip,
> + struct pwm_device *pwm)
> +{
[...]
> + 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.
> +static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device
> *pwm)
> +{
[...]
> + /* 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.
> +static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device
> *pwm)
> +{
[...]
> + /* If duty is 0 reverse polarity */
> + if (tcbpwm->duty == 0)
> + polarity = !polarity;
Same here.
> +static int atmel_tcb_pwm_probe(struct platform_device *pdev)
> +{
[...]
> + 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", );
> + if (err < 0) {
> + dev_err(>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:
> + tc = atmel_tc_alloc(tcblock, "tcb-pwm");
> + if (tc == NULL) {
> + dev_err(>dev, "failed to allocate Timer Counter Block\n");
> + return -ENOMEM;
> + }
[...]
> +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.
> +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 ");
> +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
pgp8PUjwyYn_P.pgp
Description: PGP signature
Re: [PATCH v4 RESEND] pwm: atmel: add Timer Counter Block PWM driver
On Thu, Dec 20, 2012 at 10:12:56AM +0100, Boris BREZILLON wrote:
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
Should this be PWM 2 and 3 and PWM 4 and 5? Or is PWM 1 shared
between groups 0 and 1?
+static int atmel_tcb_pwm_request(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
[...]
+ 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.
+static void atmel_tcb_pwm_disable(struct pwm_chip *chip, struct pwm_device
*pwm)
+{
[...]
+ /* 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.
+static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device
*pwm)
+{
[...]
+ /* If duty is 0 reverse polarity */
+ if (tcbpwm-duty == 0)
+ polarity = !polarity;
Same here.
+static int atmel_tcb_pwm_probe(struct platform_device *pdev)
+{
[...]
+ 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:
+ tc = atmel_tc_alloc(tcblock, tcb-pwm);
+ if (tc == NULL) {
+ dev_err(pdev-dev, failed to allocate Timer Counter Block\n);
+ return -ENOMEM;
+ }
[...]
+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.
+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 b.brezil...@overkiz.com);
+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
pgp8PUjwyYn_P.pgp
Description: PGP signature
[PATCH v4 RESEND] 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
Signed-off-by: Boris BREZILLON
---
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 10 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
diff --git a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
new file mode 100644
index 000..de0eaed
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
@@ -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>;
+};
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index e513cd9..10b6afc 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -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
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 62a2963..94ba21e 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -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
diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
new file mode 100644
index 000..4852f66
--- /dev/null
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -0,0 +1,428 @@
+/*
+ * Copyright (C) Overkiz SAS 2012
+ *
+ * Author: Boris BREZILLON
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define NPWM 6
+
+#define ATMEL_TC_ACMR_MASK (ATMEL_TC_ACPA | ATMEL_TC_ACPC |\
+
[PATCH v4 RESEND] 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
Signed-off-by: Boris BREZILLON linux-...@overkiz.com
---
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 10 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
diff --git a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
new file mode 100644
index 000..de0eaed
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
@@ -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;
+};
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index e513cd9..10b6afc 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -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
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 62a2963..94ba21e 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -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
diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
new file mode 100644
index 000..4852f66
--- /dev/null
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -0,0 +1,428 @@
+/*
+ * Copyright (C) Overkiz SAS 2012
+ *
+ * Author: Boris BREZILLON b.brezil...@overkiz.com
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include linux/module.h
+#include linux/init.h
+#include linux/clocksource.h
+#include linux/clockchips.h
+#include linux/interrupt.h
+#include linux/irq.h
+
+#include linux/clk.h
+#include linux/err.h
+#include linux/ioport.h
+#include linux/io.h
+#include
