[2/5] pwm: kona: Introduce Kona PWM controller support

Add support for the six-channel Kona PWM controller found on Broadcom
mobile SoCs like bcm281xx.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Alex Elder <elder@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
---
 drivers/pwm/Kconfig        |  10 ++
 drivers/pwm/Makefile       |   1 +
 drivers/pwm/pwm-bcm-kona.c | 226 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 237 insertions(+)
 create mode 100644 drivers/pwm/pwm-bcm-kona.c

On Mon, Nov 18, 2013 at 10:54:58AM -0800, Tim Kryger wrote:
[...]
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
[...]
> +config PWM_BCM_KONA
> +	tristate "Kona PWM support"
> +	depends on ARCH_BCM_MOBILE
> +	default y

Why do you want this to be the default? Shouldn't this be something that
a default configuration selects explicitly?

> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
[...]
>  obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
> +obj-$(CONFIG_PWM_BCM_KONA)	+= pwm-bcm-kona.o

'C' < 'F'

> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
[...]
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#define KONA_PWM_CHANNEL_CNT		6

You use this exactly once, so there's no need for this define.

> +#define PWM_CONTROL_OFFSET		(0x00000000)

I'd prefer if you dropped the _OFFSET suffix here.

> +#define PWM_CONTROL_INITIAL		(0x3f3f3f00)

Can this not be expressed as a bitmask of values for the individual
fields.

> +#define PWMOUT_POLARITY(chan)		(0x1 << (8 + chan))

This seems to only account for bits 8-13, what about the others?

> +#define PWMOUT_ENABLE(chan)		(0x1 << chan)

Well, this accounts for bits 0-5, that still leaves 16-21 and 24-29.

Also perhaps PWMOUT_POLARITY and PWMOUT_ENABLE should be defined as
PWM_CONTROL_POLARITY and PWM_CONTROL_ENABLE. That makes it easy to see
which register they are related to.

> +#define PRESCALE_OFFSET			(0x00000004)
> +#define PRESCALE_SHIFT(chan)		(chan << 2)

I'm confused. This allocates 2 bits for each channel...

> +#define PRESCALE_MASK(chan)		(~(0x7 << (chan << 2)))
> +#define PRESCALE_MIN			(0x00000000)
> +#define PRESCALE_MAX			(0x00000007)

... but 0x7 requires at least 3 bits.

> +#define PERIOD_COUNT_OFFSET(chan)	(0x00000008 + (chan << 3))
> +#define PERIOD_COUNT_MIN		(0x00000002)
> +#define PERIOD_COUNT_MAX		(0x00ffffff)

Why PERIOD_COUNT? PERIOD is descriptive enough. Or is this the name as
found in the manual?

> +#define DUTY_CYCLE_HIGH_OFFSET(chan)	(0x0000000c + (chan << 3))
> +#define DUTY_CYCLE_HIGH_MIN		(0x00000000)
> +#define DUTY_CYCLE_HIGH_MAX		(0x00ffffff)

By definition the duty-cycle is where the signal is high. Again, if this
is how the manual names the registers it's fine.

> +struct kona_pwmc {
> +	struct pwm_chip chip;
> +	void __iomem *base;
> +	struct clk *clk;
> +};
> +
> +static void kona_pwmc_apply_settings(struct kona_pwmc *kp, int chan)

unsigned int for "chan"?

> +{
> +	/* New settings take effect on rising edge of enable  bit */
> +	writel(readl(kp->base + PWM_CONTROL_OFFSET) & ~PWMOUT_ENABLE(chan),
> +	       kp->base + PWM_CONTROL_OFFSET);
> +	writel(readl(kp->base + PWM_CONTROL_OFFSET) | PWMOUT_ENABLE(chan),
> +	       kp->base + PWM_CONTROL_OFFSET);

That's too cluttered for my taste. Please make this more explicit:

	value = readl(...);
	value &= ~...;
	writel(value, ...);

	value = readl(...);
	value |= ...;
	writel(value, ...);

> +}
> +
> +static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +				int duty_ns, int period_ns)
> +{
> +	struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
> +	u64 val, div, clk_rate;
> +	unsigned long prescale = PRESCALE_MIN, pc, dc;
> +	int chan = pwm->hwpwm;

pwm->hwpwm is unsigned, so chan should be as well.

> +
> +	/*
> +	 * Find period count, duty count and prescale to suit duty_ns and
> +	 * period_ns. This is done according to formulas described below:
> +	 *
> +	 * period_ns = 10^9 * (PRESCALE + 1) * PC / PWM_CLK_RATE
> +	 * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
> +	 *
> +	 * PC = (PWM_CLK_RATE * period_ns) / (10^9 * (PRESCALE + 1))
> +	 * DC = (PWM_CLK_RATE * duty_ns) / (10^9 * (PRESCALE + 1))
> +	 */
> +
> +	clk_rate = clk_get_rate(kp->clk);
> +	while (1) {

Newline between the above two lines please.

> +		div = 1000000000;
> +		div *= 1 + prescale;
> +		val = clk_rate * period_ns;
> +		pc = div64_u64(val, div);
> +		val = clk_rate * duty_ns;
> +		dc = div64_u64(val, div);
> +
> +		/* If duty_ns or period_ns are not achievable then return */
> +		if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN)
> +			return -EINVAL;
> +
> +		/*
> +		 * If pc or dc have crossed their upper limit, then increase
> +		 * prescale and recalculate pc and dc.
> +		 */
> +		if (pc > PERIOD_COUNT_MAX || dc > DUTY_CYCLE_HIGH_MAX) {
> +			if (++prescale > PRESCALE_MAX)
> +				return -EINVAL;
> +			continue;
> +		}

This looks unintuitive to me, perhaps:

		if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX)
			break;

		if (++prescale > PRESCALE_MAX)
			return -EINVAL;

?

> +	/* Program prescale */
> +	writel((readl(kp->base + PRESCALE_OFFSET) & PRESCALE_MASK(chan)) |
> +	       prescale << PRESCALE_SHIFT(chan),
> +	       kp->base + PRESCALE_OFFSET);

Again, please split this into separate read/modify/write steps.

> +
> +	/* Program period count */
> +	writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan));
> +
> +	/* Program duty cycle high count */
> +	writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));

I don't think we need the comments. The register names are fairly
descriptive, so the comments add no value.

> +
> +	if (test_bit(PWMF_ENABLED, &pwm->flags))
> +		kona_pwmc_apply_settings(kp, chan);
> +
> +	return 0;
> +}
> +
> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	return kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
> +}

Why can't this just enable the channel? Why go through all the trouble
of running the whole computations again?

> +
> +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
> +	int chan = pwm->hwpwm;
> +
> +	/*
> +	 * The PWM hardware lacks a proper way to be disabled so
> +	 * we just program zero duty cycle high count instead
> +	 */

So clearing the enable bit doesn't disable the PWM channel?

> +
> +	writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> +	kona_pwmc_apply_settings(kp, chan);
> +}
> +
> +static const struct pwm_ops kona_pwm_ops = {
> +	.config = kona_pwmc_config,
> +	.owner = THIS_MODULE,
> +	.enable = kona_pwmc_enable,
> +	.disable = kona_pwmc_disable,
> +};

Please move the .owner field to be the last field. Also you did define
the PWMOUT_POLARITY field, which indicates that the hardware supports
changing the signal's polarity, yet you don't implement the polarity
feature. Why not?

> +static int kona_pwmc_probe(struct platform_device *pdev)
> +{
> +	struct kona_pwmc *kp;
> +	struct resource *res;
> +	int ret = 0;

I don't think this needs to be initialized.

> +
> +	dev_dbg(&pdev->dev, "bcm_kona_pwmc probe\n");

Can this be removed?

> +
> +	kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL);
> +	if (kp == NULL)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	kp->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(kp->base))
> +		return PTR_ERR(kp->base);
> +
> +	kp->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(kp->clk)) {
> +		dev_err(&pdev->dev, "Clock get failed : Err %d\n", ret);

ret would be 0 here, indicating no error. This should probably be
PTR_ERR(kp->clk). Also please make the error message more consistent,
this one and the one below use completely different styles. Also, "Err"
isn't very useful in an error message. Something like:

		dev_err(&pdev->dev, "failed to get clock: %d\n",
			PTR_ERR(kp->clk));

would be good.

> +		return PTR_ERR(kp->clk);
> +	}
> +
> +	ret = clk_prepare_enable(kp->clk);
> +	if (ret < 0)
> +		return ret;

Do you really want the clock enabled all the time? Why not just
clk_enable() whenever a PWM is enabled? If you need the clock for
register access, you can also bracket register accesses with
clk_enable() and clk_disable(). Perhaps the power savings aren't worth
the added effort, so if you'd rather not do that, I'm fine with it, too.

> +
> +	/* Set smooth mode, push/pull, and normal polarity for all channels */
> +	writel(PWM_CONTROL_INITIAL, kp->base + PWM_CONTROL_OFFSET);

I'd expect to see bitfield definitions for smooth mode and push/pull,
and PWM_CONTROL_INITIAL to be defined in terms of those. Better yet
would be to have a value constructed at runtime with the initial value.

> +	dev_set_drvdata(&pdev->dev, kp);

platform_set_drvdata(), please.

> +	kp->chip.dev = &pdev->dev;
> +	kp->chip.ops = &kona_pwm_ops;
> +	kp->chip.base = -1;
> +	kp->chip.npwm = KONA_PWM_CHANNEL_CNT;
> +
> +	ret = pwmchip_add(&kp->chip);
> +	if (ret < 0) {
> +		clk_disable_unprepare(kp->clk);
> +		dev_err(&pdev->dev, "pwmchip_add() failed: Err %d\n", ret);

For consistency with my above recommendation:

		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);

Also, I'd move the error message before clk_disable_unprepare(). There's
no technical reason really, but it's far more common that way around.

> +	}
> +
> +	return ret;
> +}
> +
> +static int kona_pwmc_remove(struct platform_device *pdev)
> +{
> +	struct kona_pwmc *kp = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(kp->clk);
> +	return pwmchip_remove(&kp->chip);
> +}
> +
> +static const struct of_device_id bcm_kona_pwmc_dt[] = {
> +	{.compatible = "brcm,kona-pwm"},

Needs spaces after { and before }.

> +	{},

Should be: { }.

> +};
> +MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt);
> +
> +static struct platform_driver kona_pwmc_driver = {
> +
> +	.driver = {
> +		   .name = "bcm-kona-pwm",
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = bcm_kona_pwmc_dt,
> +		   },

The alignment is weird, should be:

	.driver = {
		.name = "bcm-kona-pwm",
		.owner = THIS_MODULE,
		.of_match_table = bcm_kona_pwmc_dt,
	},

You can also leave out the .owner field, that's assigned automatically
by the driver core.

> +
> +	.probe = kona_pwmc_probe,

No blank line before this one.

> +	.remove = kona_pwmc_remove,
> +};
> +
> +module_platform_driver(kona_pwmc_driver);

No blank line before this one.

> +
> +MODULE_AUTHOR("Broadcom");

I don't think Broadcom qualifies as author. This should be the name of
whoever wrote the code. There are a few drivers that contain the company
name in the MODULE_AUTHOR, but I don't think those are correct either.

> +MODULE_DESCRIPTION("Driver for KONA PWMC");

"Driver for KONA PWM controller"?

> +MODULE_LICENSE("GPL");

According to the header comment this should be "GPL v2".

> +MODULE_VERSION("1.0");

I don't think we need this.

Thierry

On Mon, Nov 25, 2013 at 3:08 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Nov 18, 2013 at 10:54:58AM -0800, Tim Kryger wrote:
> [...]
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> [...]
>> +config PWM_BCM_KONA
>> +     tristate "Kona PWM support"
>> +     depends on ARCH_BCM_MOBILE
>> +     default y
>
> Why do you want this to be the default? Shouldn't this be something that
> a default configuration selects explicitly?

This hardware is present on all recent Broadcom mobile SoCs so it is
reasonable to expect that one would want the driver enabled but I
think it makes sense to allow it be compiled out just in case it is
unused.

>> +#define PWM_CONTROL_INITIAL          (0x3f3f3f00)
>
> Can this not be expressed as a bitmask of values for the individual
> fields.
>
>> +#define PWMOUT_POLARITY(chan)                (0x1 << (8 + chan))
>
> This seems to only account for bits 8-13, what about the others?
>
>> +#define PWMOUT_ENABLE(chan)          (0x1 << chan)
>
> Well, this accounts for bits 0-5, that still leaves 16-21 and 24-29.

29:24 - Configures each PWM channel for smooth transitions or glitches
21:16 - Configures each PWM channel for push/pull or open drain output


>> +#define PRESCALE_OFFSET                      (0x00000004)
>> +#define PRESCALE_SHIFT(chan)         (chan << 2)
>
> I'm confused. This allocates 2 bits for each channel...
>
>> +#define PRESCALE_MASK(chan)          (~(0x7 << (chan << 2)))
>> +#define PRESCALE_MIN                 (0x00000000)
>> +#define PRESCALE_MAX                 (0x00000007)
>
> ... but 0x7 requires at least 3 bits.

Actually this is allocating 2^2 bits for each channel.

>> +#define PERIOD_COUNT_OFFSET(chan)    (0x00000008 + (chan << 3))
>> +#define PERIOD_COUNT_MIN             (0x00000002)
>> +#define PERIOD_COUNT_MAX             (0x00ffffff)
>
> Why PERIOD_COUNT? PERIOD is descriptive enough. Or is this the name as
> found in the manual?

I agree but as you suspected this name comes from the hardware docs.

>> +#define DUTY_CYCLE_HIGH_OFFSET(chan) (0x0000000c + (chan << 3))
>> +#define DUTY_CYCLE_HIGH_MIN          (0x00000000)
>> +#define DUTY_CYCLE_HIGH_MAX          (0x00ffffff)
>
> By definition the duty-cycle is where the signal is high. Again, if this
> is how the manual names the registers it's fine.

Same comment as above.

>> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +     return kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
>> +}
>
> Why can't this just enable the channel? Why go through all the trouble
> of running the whole computations again?

The hardware is always enabled and at best can be be configured to
operate at zero duty.

The settings in HW may have already been triggered and might not be
what you want.

For example:

/sys/class/pwm/pwmchip0/pwm1# echo 1 > enable
/sys/class/pwm/pwmchip0/pwm1# echo 5000 > period
/sys/class/pwm/pwmchip0/pwm1# echo 2000 > duty_cycle
/sys/class/pwm/pwmchip0/pwm1# echo 0 > enable
/sys/class/pwm/pwmchip0/pwm1# echo 1 > enable

>> +
>> +static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +     struct kona_pwmc *kp = dev_get_drvdata(chip->dev);
>> +     int chan = pwm->hwpwm;
>> +
>> +     /*
>> +      * The PWM hardware lacks a proper way to be disabled so
>> +      * we just program zero duty cycle high count instead
>> +      */
>
> So clearing the enable bit doesn't disable the PWM channel?

That is correct.  They picked a terrible name for this bit.

>> +
>> +     writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
>> +     kona_pwmc_apply_settings(kp, chan);
>> +}
>> +
>> +static const struct pwm_ops kona_pwm_ops = {
>> +     .config = kona_pwmc_config,
>> +     .owner = THIS_MODULE,
>> +     .enable = kona_pwmc_enable,
>> +     .disable = kona_pwmc_disable,
>> +};
>
> Please move the .owner field to be the last field. Also you did define
> the PWMOUT_POLARITY field, which indicates that the hardware supports
> changing the signal's polarity, yet you don't implement the polarity
> feature. Why not?

I wanted to keep this driver simple for now.

>> +     ret = clk_prepare_enable(kp->clk);
>> +     if (ret < 0)
>> +             return ret;
>
> Do you really want the clock enabled all the time? Why not just
> clk_enable() whenever a PWM is enabled? If you need the clock for
> register access, you can also bracket register accesses with
> clk_enable() and clk_disable(). Perhaps the power savings aren't worth
> the added effort, so if you'd rather not do that, I'm fine with it, too.

I intend to follow up with a patch to do exactly that but I want to
establish the baseline functionality first.

>> +MODULE_AUTHOR("Broadcom");
>
> I don't think Broadcom qualifies as author. This should be the name of
> whoever wrote the code. There are a few drivers that contain the company
> name in the MODULE_AUTHOR, but I don't think those are correct either.

Would you be fine with two lines here?  Something like:

MODULE_AUTHOR("Broadcom Corporation <bcm-kernel-feedback-list@broadcom.com>");
MODULE_AUTHOR("Tim Kryger <tkryger@broadcom.com>");


Thanks for the review.  I will make all of the other changes you have
recommended.

-Tim

On Mon, Nov 25, 2013 at 05:38:44PM -0800, Tim Kryger wrote:
> On Mon, Nov 25, 2013 at 3:08 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Mon, Nov 18, 2013 at 10:54:58AM -0800, Tim Kryger wrote:
> > [...]
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
[...]
> >> +#define PWM_CONTROL_INITIAL          (0x3f3f3f00)
> >
> > Can this not be expressed as a bitmask of values for the individual
> > fields.
> >
> >> +#define PWMOUT_POLARITY(chan)                (0x1 << (8 + chan))
> >
> > This seems to only account for bits 8-13, what about the others?
> >
> >> +#define PWMOUT_ENABLE(chan)          (0x1 << chan)
> >
> > Well, this accounts for bits 0-5, that still leaves 16-21 and 24-29.
> 
> 29:24 - Configures each PWM channel for smooth transitions or glitches
> 21:16 - Configures each PWM channel for push/pull or open drain output

Excellent, now if you can turn that into bit definitions and define
PWM_CONTROL_INITIAL in terms of those, then I'll be very happy.

One other thing I didn't pay attention to before: while it's quite
unlikely ever to happen, you might want to spend an extra pair of
parentheses around "chan", just in case.

> >> +#define PRESCALE_OFFSET                      (0x00000004)
> >> +#define PRESCALE_SHIFT(chan)         (chan << 2)
> >
> > I'm confused. This allocates 2 bits for each channel...
> >
> >> +#define PRESCALE_MASK(chan)          (~(0x7 << (chan << 2)))
> >> +#define PRESCALE_MIN                 (0x00000000)
> >> +#define PRESCALE_MAX                 (0x00000007)
> >
> > ... but 0x7 requires at least 3 bits.
> 
> Actually this is allocating 2^2 bits for each channel.

Doh! Indeed. Perhaps writing it as (~(0x7 << (chan * 4))) would make it
easier to digest for slow-witted people like myself.

> >> +#define PERIOD_COUNT_OFFSET(chan)    (0x00000008 + (chan << 3))
> >> +#define PERIOD_COUNT_MIN             (0x00000002)
> >> +#define PERIOD_COUNT_MAX             (0x00ffffff)
> >
> > Why PERIOD_COUNT? PERIOD is descriptive enough. Or is this the name as
> > found in the manual?
> 
> I agree but as you suspected this name comes from the hardware docs.

Okay, that's fine then. Do you have a pointer to that documentation?

> >> +static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >> +{
> >> +     return kona_pwmc_config(chip, pwm, pwm->duty_cycle, pwm->period);
> >> +}
> >
> > Why can't this just enable the channel? Why go through all the trouble
> > of running the whole computations again?
> 
> The hardware is always enabled and at best can be be configured to
> operate at zero duty.

What good are the PWMOUT_ENABLE bits then? Is that really only used for
triggering updates? That's what another comment suggests, but if so, can
the comment in kona_pwmc_apply_settings() be extended to mention that?

> The settings in HW may have already been triggered and might not be
> what you want.
> 
> For example:
> 
> /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable
> /sys/class/pwm/pwmchip0/pwm1# echo 5000 > period
> /sys/class/pwm/pwmchip0/pwm1# echo 2000 > duty_cycle
> /sys/class/pwm/pwmchip0/pwm1# echo 0 > enable
> /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable

I'm not exactly sure what this is supposed to demonstrate.

> >> +
> >> +     writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan));
> >> +     kona_pwmc_apply_settings(kp, chan);
> >> +}
> >> +
> >> +static const struct pwm_ops kona_pwm_ops = {
> >> +     .config = kona_pwmc_config,
> >> +     .owner = THIS_MODULE,
> >> +     .enable = kona_pwmc_enable,
> >> +     .disable = kona_pwmc_disable,
> >> +};
> >
> > Please move the .owner field to be the last field. Also you did define
> > the PWMOUT_POLARITY field, which indicates that the hardware supports
> > changing the signal's polarity, yet you don't implement the polarity
> > feature. Why not?
> 
> I wanted to keep this driver simple for now.

Fair enough.

> > Do you really want the clock enabled all the time? Why not just
> > clk_enable() whenever a PWM is enabled? If you need the clock for
> > register access, you can also bracket register accesses with
> > clk_enable() and clk_disable(). Perhaps the power savings aren't worth
> > the added effort, so if you'd rather not do that, I'm fine with it, too.
> 
> I intend to follow up with a patch to do exactly that but I want to
> establish the baseline functionality first.

Okay, that's fine.

> >> +MODULE_AUTHOR("Broadcom");
> >
> > I don't think Broadcom qualifies as author. This should be the name of
> > whoever wrote the code. There are a few drivers that contain the company
> > name in the MODULE_AUTHOR, but I don't think those are correct either.
> 
> Would you be fine with two lines here?  Something like:
> 
> MODULE_AUTHOR("Broadcom Corporation <bcm-kernel-feedback-list@broadcom.com>");
> MODULE_AUTHOR("Tim Kryger <tkryger@broadcom.com>");

Not sure. That email address certainly doesn't look like it belongs to
the driver author. Neither did Broadcom actually write the driver, you
did, right? If you're concerned about your employer not being credited
you're listed with an @broadcom.com email address and there's the
copyright statement.

All of that said, I wasn't able to dig up a normative policy and either
of your proposed alternatives do exist in the kernel, so I withdraw any
objections regarding MODULE_AUTHOR. Take your pick.

Thierry

On Tue, Nov 26, 2013 at 1:45 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Nov 25, 2013 at 05:38:44PM -0800, Tim Kryger wrote:

> Okay, that's fine then. Do you have a pointer to that documentation?

Unfortunately the documentation is not publicly available at the moment.

>> The hardware is always enabled and at best can be be configured to
>> operate at zero duty.
>
> What good are the PWMOUT_ENABLE bits then? Is that really only used for
> triggering updates? That's what another comment suggests, but if so, can
> the comment in kona_pwmc_apply_settings() be extended to mention that?

The guidance from the documentation is to treat the enable bits as a trigger.

Sure I can expand the comment to try and make this more clear.

>> The settings in HW may have already been triggered and might not be
>> what you want.
>>
>> For example:
>>
>> /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable
>> /sys/class/pwm/pwmchip0/pwm1# echo 5000 > period
>> /sys/class/pwm/pwmchip0/pwm1# echo 2000 > duty_cycle
>> /sys/class/pwm/pwmchip0/pwm1# echo 0 > enable
>> /sys/class/pwm/pwmchip0/pwm1# echo 1 > enable
>
> I'm not exactly sure what this is supposed to demonstrate.

The computation has to be performed for every operation except the disable.

> All of that said, I wasn't able to dig up a normative policy and either
> of your proposed alternatives do exist in the kernel, so I withdraw any
> objections regarding MODULE_AUTHOR. Take your pick.

Thanks