Re: [PATCH v2] pwm: Add MC34708 PWM driver support.

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

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

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

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