Re: [PATCH] sh: added PWM driver for SH7723 using the TPU

[Paul Mundt <lethal@linux-sh.org>]

On Wed, Feb 03, 2010 at 03:35:45PM +0100, Pietrek, Markus wrote:
> diff --git a/arch/sh/drivers/Kconfig b/arch/sh/drivers/Kconfig
> index 420c6b2..83c8794 100644
> --- a/arch/sh/drivers/Kconfig
> +++ b/arch/sh/drivers/Kconfig
> @@ -16,4 +16,11 @@ config PUSH_SWITCH
>           This enables support for the push switch framework, a simple
>           framework that allows for sysfs driven switch status reporting.
> 
> +config SH_PWM_TPU
> +       bool "PWM with Timer Pulse Unit (TPU) Driver"
> +       depends on CPU_SUBTYPE_SH7723
> +       select HAVE_PWM
> +        help
> +          Provides a PWM driver using the Timer Pulse Unit of the SH7723
> +
The cleaner way to do this would be to have something like:

config SYS_SUPPORTS_TPU
	bool

in arch/sh/Kconfig, followed by having SH7723 select that. Then your
SH_PMW_TPU config option can just depend on SYS_SUPPORTS_TPU, and it will
be automatically visible for any future CPU that indicates block support.

> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/gpio.h>
> +#include <linux/io.h>
> +#include <linux/pwm.h>
> +
> +#ifdef CONFIG_CPU_SUBTYPE_SH7723
> +# include <cpu/sh7723.h>
> +#endif
> +
Presumably you wanted this header for ..

> +static const int gpios[CHANNELS] = {
> +#ifdef CONFIG_CPU_SUBTYPE_SH7723
> +       GPIO_PTG0,
> +       GPIO_PTG1,
> +       GPIO_PTG2,
> +       GPIO_PTG3,
> +#else
> +# error no GPIO configuration for cpu
> +#endif
> +};
> +
this. However, you should simply construct the GPIO map through platform
data and pass it along when registering the device from the CPU code.

> +EXPORT_SYMBOL(pwm_disable);
> +
Is EXPORT_SYMBOL() intentional? By convention we primarily use
EXPORT_SYMBOL_GPL() for new exports.

> +/* driver registering functions */
> +
> +static int __devinit pwm_tpu_probe(struct platform_device *pdev)
> +{
[snip]

> +       priv->base = ioremap(res->start, (res->end-res->start)+1);

Although you didn't succumb to the off-by-1 bug that most people do, you
should still use resource_size() here to avoid ambiguity. :-)

> +MODULE_AUTHOR("Markus Pietrek");
> +MODULE_DESCRIPTION("PWM/Timer Pulse Unit TPU control");
> +MODULE_LICENSE("GPL");
> +
> +arch_initcall(pwm_tpu_init);
> +module_exit(pwm_tpu_exit);

arch_initcall() is a bit early, presumably you have some users of this
code that actually needs to have this initialized that early on?

Looks good otherwise.