[v2] pwm: Add MC34708 PWM driver support.

Signed-off-by: Denis Carikli <denis@eukrea.com>
---
Changelog v1->v2:

- The driver now uses retrives mc13xxx without having to export it
  trough a globally exported function.
- The .enable() and .disable() are now handled.
- The registers calculations have been reworked.
- Defines have been reworked to be more readable.
- The driver only supports the mc34708, so now we don't claim to support
  other devices anymore, and the prefix has been changed from mc13xxx
  to mc34708. The documentation was also updated to reflect that.
- Spelling errors have been fixed.
- There is now less code thanks to the use of mc13xxx_reg_rmw and
  range checking functions.
- Many other cosmetics fixes and code cleanups.
---
 Documentation/devicetree/bindings/mfd/mc13xxx.txt |    3 +
 drivers/mfd/mc13xxx-core.c                        |   16 ++
 drivers/pwm/Kconfig                               |    6 +
 drivers/pwm/Makefile                              |    1 +
 drivers/pwm/pwm-mc34708.c                         |  224 +++++++++++++++++++++
 5 files changed, 250 insertions(+)
 create mode 100644 drivers/pwm/pwm-mc34708.c

Mon, 23 Jun 2014 14:10:05 +0200 от Denis Carikli <denis@eukrea.com>:
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---

...
> @@ -681,6 +695,7 @@ int mc13xxx_common_init(struct device *dev)
>  			&pdata->regulators, sizeof(pdata->regulators));
>  		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-led",
>  				pdata->leds, sizeof(*pdata->leds));
> +		mc13xxx_add_subdevice(mc13xxx, "%s-pwm");
>  		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-pwrbutton",
>  				pdata->buttons, sizeof(*pdata->buttons));
>  		if (mc13xxx->flags & MC13XXX_USE_CODEC)
> @@ -692,6 +707,7 @@ int mc13xxx_common_init(struct device *dev)
>  	} else {
>  		mc13xxx_add_subdevice(mc13xxx, "%s-regulator");
>  		mc13xxx_add_subdevice(mc13xxx, "%s-led");
> +		mc13xxx_add_subdevice(mc13xxx, "%s-pwm");
>  		mc13xxx_add_subdevice(mc13xxx, "%s-pwrbutton");
>  		if (mc13xxx->flags & MC13XXX_USE_CODEC)
>  			mc13xxx_add_subdevice(mc13xxx, "%s-codec");

What a difference for DT and non-DT?

...
> +	/* Actual write to the registers */
> +	mc13xxx_lock(mc13xxx);
> +
> +	ret = mc13xxx_reg_rmw(mc13xxx, MC134708_PWM,
> +					MC134708_PWM_MASK << period_offset,
> +					pwm_clkdiv << period_offset);
> +	if (ret) {
> +		mc13xxx_unlock(mc13xxx);
> +		return ret;
> +	}

You do not need lock()/unlock() here and in the rest of driver code.

...
> +static struct platform_driver pwm_mc34708_driver = {
> +	.driver	= {
> +		.name = "mc34708-pwm",
> +		.of_match_table = of_match_ptr(pwm_mc34708_of_match),
> +	},
> +	.probe = pwm_mc34708_probe,
> +	.remove = pwm_mc34708_remove,
> +};
> +module_platform_driver(pwm_mc34708_driver);

module_platform_driver_probe()

---

On Mon, Jun 23, 2014 at 02:10:05PM +0200, Denis Carikli wrote:
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
> Changelog v1->v2:
> 
> - The driver now uses retrives mc13xxx without having to export it
>   trough a globally exported function.
> - The .enable() and .disable() are now handled.
> - The registers calculations have been reworked.
> - Defines have been reworked to be more readable.
> - The driver only supports the mc34708, so now we don't claim to support
>   other devices anymore, and the prefix has been changed from mc13xxx
>   to mc34708. The documentation was also updated to reflect that.
> - Spelling errors have been fixed.
> - There is now less code thanks to the use of mc13xxx_reg_rmw and
>   range checking functions.
> - Many other cosmetics fixes and code cleanups.
> ---
>  Documentation/devicetree/bindings/mfd/mc13xxx.txt |    3 +
>  drivers/mfd/mc13xxx-core.c                        |   16 ++
>  drivers/pwm/Kconfig                               |    6 +
>  drivers/pwm/Makefile                              |    1 +
>  drivers/pwm/pwm-mc34708.c                         |  224 +++++++++++++++++++++
>  5 files changed, 250 insertions(+)
>  create mode 100644 drivers/pwm/pwm-mc34708.c
> 
> diff --git a/Documentation/devicetree/bindings/mfd/mc13xxx.txt b/Documentation/devicetree/bindings/mfd/mc13xxx.txt
> index 8aba488..464a663 100644
> --- a/Documentation/devicetree/bindings/mfd/mc13xxx.txt
> +++ b/Documentation/devicetree/bindings/mfd/mc13xxx.txt
> @@ -22,6 +22,9 @@ Sub-nodes:
>    Each led node should contain "reg", which used as LED ID (described below).
>    Optional properties "label" and "linux,default-trigger" is described in
>    Documentation/devicetree/bindings/leds/common.txt.
> +- pwm: For MC34708, contain the PWM controller:
> +  - compatible: must be "fsl,mc34708-pwm".

Shouldn't this be "MC34708" and "fsl,mc134708-pwm"?

> +  - #pwm-cells: must be 2.
>  - regulators : Contain the regulator nodes. The regulators are bound using
>    their names as listed below with their registers and bits for enabling.
>  
> diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
> index acf5dd7..71b7d84c 100644
> --- a/drivers/mfd/mc13xxx-core.c
> +++ b/drivers/mfd/mc13xxx-core.c
> @@ -599,6 +599,20 @@ static int mc13xxx_add_subdevice_pdata(struct mc13xxx *mc13xxx,
>  	if (!cell.name)
>  		return -ENOMEM;
>  
> +	/* mfd_add_devices won't adds a .of_node to the child's dev if the

"won't add"

> +	 * cell's .off_compatible is NULL, which result in

".of_compatible", "results in"

> +	 * of_node_to_pwmchip beeing unable to find the pwm device.

"being", "PWM device"

Also I would prefer to remove the reference to of_node_to_pwmchip in the
above description. It's a non-exported API and if it were to be renamed
this comment would likely become stale.

Perhaps: "... results in the PWM core being unable to find the PWM
device."?

> +	 */
> +	if (!strncmp(format, "%s-pwm", sizeof("%s-pwm"))) {

Why special-case the PWM subdevice? Doesn't the comment really apply to
all of the subdevices? Even if the subsystems don't rely on the OF node
I think it would still be useful to set it up properly.

> +		if (snprintf(buf, sizeof(buf),
> +			     "fsl,%s", cell.name) > sizeof(buf))
> +			return -E2BIG;
> +
> +		cell.of_compatible = kmemdup(buf, strlen(buf) + 1, GFP_KERNEL);
> +		if (!cell.of_compatible)
> +			return -ENOMEM;

Can't the above simply be:

	cell.of_compatible = kasprintf(GFP_KERNEL, "fsl,%s", cell.name);

?

> +	}
> +
>  	return mfd_add_devices(mc13xxx->dev, -1, &cell, 1, NULL, 0, NULL);
>  }
>  
> @@ -681,6 +695,7 @@ int mc13xxx_common_init(struct device *dev)
>  			&pdata->regulators, sizeof(pdata->regulators));
>  		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-led",
>  				pdata->leds, sizeof(*pdata->leds));
> +		mc13xxx_add_subdevice(mc13xxx, "%s-pwm");
>  		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-pwrbutton",
>  				pdata->buttons, sizeof(*pdata->buttons));
>  		if (mc13xxx->flags & MC13XXX_USE_CODEC)
> @@ -692,6 +707,7 @@ int mc13xxx_common_init(struct device *dev)
>  	} else {
>  		mc13xxx_add_subdevice(mc13xxx, "%s-regulator");
>  		mc13xxx_add_subdevice(mc13xxx, "%s-led");
> +		mc13xxx_add_subdevice(mc13xxx, "%s-pwm");
>  		mc13xxx_add_subdevice(mc13xxx, "%s-pwrbutton");
>  		if (mc13xxx->flags & MC13XXX_USE_CODEC)
>  			mc13xxx_add_subdevice(mc13xxx, "%s-codec");

All of the above should be a separate patch that can be applied to the
MFD tree.

> diff --git a/drivers/pwm/pwm-mc34708.c b/drivers/pwm/pwm-mc34708.c
[...]
> +/*
> + * Copyright 2014 Eukréa Electromatique <denis@eukrea.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +

One blank line is enough.

> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +#include <linux/mfd/mc13783.h>
> +
> +/* PWM register address */
> +#define MC134708_PWM		0x37
> +
> +/* PWM register fields:
> + * Bit #    RegisterName Description
> + *  [0->5]  PWM1DUTY:   PWM1 duty cycle
> + *  [6->11] PWM1CLKDIV: PWM1 duty cycle
> + * [12->17] PWM2DUTY:   PWM2 clock divide setting
> + * [18->23] PWM2CLKDIV: PWM2 clock divide setting
> + */

Block comments should be of this format:

	/*
	 * ...
	 */

> +#define MC134708_PWM_MASK			0x3f
> +#define MC134708_PWM_NUM_OFFSET			0x0c
> +
> +#define MC134708_PWM_DUTY_OFFSET(pwm_id)	(pwm_id * MC134708_PWM_NUM_OFFSET)
> +#define MC134708_PWM_PERIOD_OFFSET(pwm_id)	((pwm_id * MC134708_PWM_NUM_OFFSET) + 0x06)

You'll need to wrap pwm_id in parentheses to make sure the expansion
does what it's supposed to.

> +
> +/* MC34708 PWM Constraints */
> +#define MC13708_BASE_CLK_FREQ	2000000

Is this really a fixed constant or is there a struct clk that can be
used to obtain this at runtime?

> +#define MC13708_PWM_MAX_DUTY	32
> +#define MC13708_PWM_MAX_CLKDIV	64
> +
> +#define MC13708_MIN_PWM_PERIOD	(NSEC_PER_SEC / MC13708_BASE_CLK_FREQ)
> +#define MC13708_MAX_PWM_PERIOD	(MC13708_MIN_PWM_PERIOD * MC13708_PWM_MAX_CLKDIV)
> +
> +#define MC134708_PWMS_NUM	2

This is really only needed in one place (see other comments later), so
you can use the literal when assigning it to pwm_chip's .npwm field.

> +
> +struct mc34708_pwm_regs {

_regs isn't really suitable here. These aren't really registers or
register contents.

There's also the mc34708 vs. mc134708 inconsistency here.

> +	int enabled;

bool

> +	int pwm_duty;

unsigned

> +};
> +
> +struct mc34708_pwm_chip {
> +	struct pwm_chip pwm_chip;
> +	struct mc13xxx *mc13xxx;
> +	struct mc34708_pwm_regs *pwms[MC134708_PWMS_NUM];

You don't need this. Please use the PWM subsystem's pwm_set_chip_data()
to set this data for each PWM device. You can look at the pwm-samsung,
pwm-renesas-tpu, pwm-lp3943, pwm-bfin or pwm-atmel-tcb drivers for
reference.

> +};
> +
> +static inline
> +struct mc34708_pwm_chip *to_mc34708_chip(struct pwm_chip *chip)

There's no need to wrap this, it fits on a single line just fine.

> +{
> +	return container_of(chip, struct mc34708_pwm_chip, pwm_chip);
> +}
> +
> +static int
> +pwm_mc34708_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      int duty_ns, int period_ns)

Similarly here. This should be:

static int pwm_mc34708_config(struct pwm_chip *chip, struct pwm_device *pwm,
			      int duty_ns, int period_ns)

> +{
> +	struct mc34708_pwm_chip *mc34708_chip = to_mc34708_chip(chip);
> +	struct mc34708_pwm_regs *pwmr = mc34708_chip->pwms[pwm->hwpwm];

If you properly set up chip data, then this becomes:

	struct mc34708_pwm_regs *pwmr = pwm_get_chip_data(pwm);

> +	struct mc13xxx *mc13xxx = mc34708_chip->mc13xxx;
> +
> +	int duty_offset = MC134708_PWM_DUTY_OFFSET(pwm->hwpwm);
> +	int period_offset = MC134708_PWM_PERIOD_OFFSET(pwm->hwpwm);

These should be unsigned.

> +
> +	int pwm_clkdiv, pwm_duty, ret = 0;

The first two of these can also be unsigned. And there's no need for the
blank lines above.

> +
> +	/* Period */
> +	period_ns = clamp(period_ns, (int)MC13708_MIN_PWM_PERIOD,
> +			  (int)MC13708_MAX_PWM_PERIOD);

Rather than clamping the value here, shouldn't this be considered an
error?

> +	pwm_clkdiv = DIV_ROUND_UP(NSEC_PER_SEC, period_ns); /* Frequency (Hz) */
> +	pwm_clkdiv = DIV_ROUND_UP(MC13708_BASE_CLK_FREQ,
> +				  pwm_clkdiv) - 1; /* Clock divisor */
> +
> +	/* Duty cycle */
> +	pwm_duty = DIV_ROUND_UP(MC13708_PWM_MAX_DUTY * duty_ns, period_ns);
> +
> +	/* Actual write to the registers */
> +	mc13xxx_lock(mc13xxx);
> +
> +	ret = mc13xxx_reg_rmw(mc13xxx, MC134708_PWM,
> +					MC134708_PWM_MASK << period_offset,
> +					pwm_clkdiv << period_offset);

For readability I'd prefer this to be broken out, somewhat like this:

	mask = MC134708_PWM_MASK << period_offset;
	value = pwm_clkdiv << period_offset;

	ret = mc13xxx_reg_rmw(mc13xx, MC134708_PWM, mask, value);
	...

> +	if (ret) {
> +		mc13xxx_unlock(mc13xxx);
> +		return ret;
> +	}
> +
> +	/* The MC34708 doesn't have an enable bit for its PWM unit,
> +	 * so we cache the pwm duty value for the .enable()
> +	 */
> +	pwmr->pwm_duty = pwm_duty;
> +
> +	if (pwmr->enabled) {
> +		ret = mc13xxx_reg_rmw(mc13xxx, MC134708_PWM,
> +				MC134708_PWM_MASK << duty_offset,
> +				pwm_duty << duty_offset);

Similarily here.

> +	}
> +	mc13xxx_unlock(mc13xxx);

There should be a blank line between the above two.

> +
> +	return ret;
> +}
> +
> +static int pwm_mc34708_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct mc34708_pwm_chip *mc34708_chip = to_mc34708_chip(chip);
> +	struct mc34708_pwm_regs *pwmr = mc34708_chip->pwms[pwm->hwpwm];
> +	struct mc13xxx *mc13xxx = mc34708_chip->mc13xxx;
> +	int duty_offset = MC134708_PWM_DUTY_OFFSET(pwm->hwpwm);

unsigned. And perhaps since there's no period being written here this
could be simply "offset"?

> +	int ret;
> +
> +	mc13xxx_lock(mc13xxx);
> +
> +	ret = mc13xxx_reg_rmw(mc13xxx, MC134708_PWM,
> +			MC134708_PWM_MASK << duty_offset,
> +			pwmr->pwm_duty << duty_offset);

Similarly this could be broken out for readability.

> +	pwmr->enabled = 1;
> +
> +	mc13xxx_unlock(mc13xxx);
> +
> +	return ret;
> +}
> +
> +static void pwm_mc34708_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct mc34708_pwm_chip *mc34708_chip = to_mc34708_chip(chip);
> +	struct mc34708_pwm_regs *pwmr = mc34708_chip->pwms[pwm->hwpwm];
> +	struct mc13xxx *mc13xxx = mc34708_chip->mc13xxx;
> +	int duty_offset = MC134708_PWM_DUTY_OFFSET(pwm->hwpwm);
> +
> +	mc13xxx_lock(mc13xxx);
> +
> +	/* To disable the PWM, the duty cycle bits have to be cleared */
> +	mc13xxx_reg_rmw(mc13xxx, MC134708_PWM,
> +			MC134708_PWM_MASK << duty_offset,
> +			0 << duty_offset);
> +	pwmr->enabled = 0;
> +
> +	mc13xxx_unlock(mc13xxx);
> +}

Same comments as for .enable()

> +static const struct pwm_ops pwm_mc34708_ops = {
> +	.enable		= pwm_mc34708_enable,
> +	.disable	= pwm_mc34708_disable,
> +	.config		= pwm_mc34708_config,
> +	.owner		= THIS_MODULE,
> +};

Please don't use artificial padding here. Single spaces around '=' will
do just fine.

> +static int pwm_mc34708_probe(struct platform_device *pdev)
> +{
> +	struct mc34708_pwm_chip *chip;
> +	struct mc13xxx *mc13xxx;
> +	int err, i;
> +
> +	mc13xxx = dev_get_drvdata(pdev->dev.parent);

You could assign this when initializing to save a few lines.

> +	if (!mc13xxx)
> +		return -EINVAL;

Can this really ever happen?

> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +			return -ENOMEM;

There's an extra tab here.

> +
> +	for (i = 0; i < MC134708_PWMS_NUM; i++) {
> +		chip->pwms[i] = devm_kzalloc(&pdev->dev,
> +			sizeof(struct mc34708_pwm_regs), GFP_KERNEL);
> +	}

When you switch to pwm_{set,get}_chip_data() the typical way to allocate
this is in a separate .request() function (and free it in .free()).

> +MODULE_ALIAS("platform:mc34708-pwm");
> +MODULE_AUTHOR("Denis Carikli <denis@eukrea.com>");
> +MODULE_DESCRIPTION("mc34708 Pulse Width Modulation Driver");

This could probably mention Freescale as the vendor? Or perhaps for
consistency with other MFD subdrivers:

	"PWM Driver for Freescale MC134708 PMIC"

?

Thierry