On Mon, Jun 23, 2014 at 02:10:05PM +0200, Denis Carikli wrote: > Signed-off-by: Denis Carikli <[email protected]> > --- > 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 <[email protected]>
> + *
> + * 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 <[email protected]>");
> +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
pgpBhtpC38I6G.pgp
Description: PGP signature
