[RFC 6/7] pwm: Add Tegra2 SoC support

[Stephen Warren]

Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> Signed-off-by: Thierry Reding <thierry.reding at avionic-design.de>

This seems reasonable, besides Olof's comments. I made a few comments
below, although I understand most were present in the original patch
you're upstreaming.

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig

> +config PWM_TEGRA
> +	tristate "NVIDIA Tegra PWM support"
> +	depends on ARCH_TEGRA
> +	default n

I think you need some help text there.

> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c

> +struct tegra_pwm_chip {
> +	struct pwm_chip		chip;
> +	struct device		*dev;
> +
> +	struct clk		*clk;
> +
> +	int			clk_enb[4];

s/clk_enb/enable/? This is really more about whether the PWM channel is
enabled than the clock; the fact the clock has to be enabled for the PWM
to run is an implementation detail.

> +static int tegra_pwm_config(struct pwm_chip *chip, unsigned int pwm,
> +		int duty_ns, int period_ns)
...
> +	/* the struct clk may be shared across multiple PWM devices, so
> +	 * only enable the PWM if this device has been enabled
> +	 */
> +	if (pc->clk_enb[num])
> +		val |= PWM_ENABLE;

That comment doesn't describe what the code is doing. Perhaps if a comment
is needed at all:

/* If the PWM channel is enabled, keep it enabled */

> +static int tegra_pwm_probe(struct platform_device *pdev)
...
> +	pwm->chip.label = "tegra-pwm";
> +	pwm->chip.ops = &tegra_pwm_ops;
> +	pwm->chip.base = -1;
> +	pwm->chip.npwm = 4;

I mentioned in an earlier patch it'd be a good idea to store a struct
device * in pcm_chip. That'd also avoid allow the label field to be
removed, since you can just use dev_name(pwm_chip->dev) then.

> +static int __init tegra_pwm_init(void)
> +{
> +	return platform_driver_register(&tegra_pwm_driver);
> +}
> +subsys_initcall(tegra_pwm_init);
> +
> +static void __exit tegra_pwm_exit(void)
> +{
> +	platform_driver_unregister(&tegra_pwm_driver);
> +}
> +module_exit(tegra_pwm_exit);

Can you use module_init() for tegra_pwm_init() instead. That had better
be possible, since the Kconfig allows building this as a module. If so,
you can replace that quoted block with:

module_platform_driver(tegra_pwm_driver);

> +MODULE_LICENSE("GPL v2");

The license header says GPLv2+, so this should just be "GPL" according
to the meanings in include/linux/module.h.

-- 
nvpublic