Thierry Reding wrote at Tuesday, December 20, 2011 3:32 AM:
> Signed-off-by: Thierry Reding <[email protected]>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
