Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:

Signed-off-by: Thierry Reding <redacted>

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.

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

I think you need some help text there.

+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