Message-ID: <20130523214517.GA18249@avionic-0098.adnet.avionic-design.de>
Date:	Thu, 23 May 2013 23:45:17 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>
Cc:	linux-sh@...r.kernel.org, linux-leds@...r.kernel.org,
	linux-kernel@...r.kernel.org, Magnus Damm <magnus.damm@...il.com>,
	Paul Mundt <lethal@...ux-sh.org>
Subject: Re: [PATCH v2 04/11] pwm: Add Renesas TPU PWM driver

On Wed, Apr 24, 2013 at 10:50:09PM +0200, Laurent Pinchart wrote:
> The Timer Pulse Unit (TPU is a 4-channels 16-bit timer used to generate
> waveforms. This driver exposes PWM functions through the PWM API for
> other drivers to use.
> 
> The code is loosely based on the leds-renesas-tpu driver by Magnus Damm
> and the TPU PWM driver shipped in the Armadillo EVA 800 kernel sources.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>
> Tested-by: Simon Horman <horms@...ge.net.au>

Sorry for taking so long to look at this...

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0e0bfa0..d57ef66 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -115,6 +115,13 @@ config PWM_PXA
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-pxa.
>  
> +config PWM_RENESAS_TPU
> +	tristate "Renesas TPU PWM support"
> +	depends on ARCH_SHMOBILE
> +	help
> +	  This driver exposes the Timer Pulse Unit (TPU) PWM controller found
> +	  in Renesas chips through the PWM API.

If the driver can be built, it'd be good to include a short sentence
saying what it will be called, just like for the other drivers.

> diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
[...]
> +struct tpu_pwm_device {
> +	bool timer_on;			/* Whether the timer is running */
> +
> +	struct tpu_pwm_channel_data *pdata;
> +	struct tpu_device *tpu;
> +	unsigned int channel;		/* Channel number in the TPU */

I don't think you need this. The pwm_device.hwpwm field carries the same
information.

> +
> +	unsigned int prescaler;
> +	u16 period;
> +	u16 duty;
> +};
> +
> +struct tpu_device {
> +	struct platform_device *pdev;
> +	struct pwm_chip chip;
> +	spinlock_t lock;
> +
> +	void __iomem *base;
> +	struct clk *clk;
> +
> +	struct tpu_pwm_device pwms[TPU_CHANNEL_MAX];
> +};

Can't you reuse the infrastructure built into the PWM subsystem? You can
associate chip-specific data with each PWM device. You can look at the
pwm-atmel-tcb and pwm-bfin drivers for usage examples. In a nutshell you
hook the .request() function and setup the driver-specific structure and
associate them with the PWM using pwm_set_chip_data().

This has the advantage that you don't need the pwms array in tpu_device
and you also don't need TPU_CHANNEL_MAX because only the pwm_chip.npwm
field needs to contain the number of channels.

> +static int tpu_pwm_timer_start(struct tpu_pwm_device *pwm)
> +{
> +	int ret;
> +
> +	if (!pwm->timer_on) {
> +		/* Wake up device and enable clock. */
> +		pm_runtime_get_sync(&pwm->tpu->pdev->dev);
> +		ret = clk_prepare_enable(pwm->tpu->clk);
> +		if (ret) {
> +			dev_err(&pwm->tpu->pdev->dev, "cannot enable clock\n");
> +			return ret;
> +		}
> +		pwm->timer_on = true;
> +	}
> +
> +	/* Make sure the channel is stopped, as we need to reconfigure it
> +	 * completely. First drive the pin to the inactive state to avoid
> +	 * glitches.
> +	 */

Can you please stick to the standard coding style for block comments?
That is:

	/*
	 * Make sure...
	 * ...
	 * glitches.
	 */

> +/* -----------------------------------------------------------------------------
> + * PWM API
> + */

I find these separators more distracting than helpful. But if you have a
strong preference for them I can live with it.

> +
> +static int tpu_pwm_request(struct pwm_chip *chip, struct pwm_device *_pwm)
> +{
> +	struct tpu_device *tpu = to_tpu_device(chip);
> +	struct tpu_pwm_device *pwm = &tpu->pwms[_pwm->hwpwm];
> +
> +	return pwm->pdata == NULL ? -EPROBE_DEFER : 0;
> +}

If you use the same method as the pwm-bfin or pwm-atmel-tcb drivers, you
could initialize each PWM channel here and associated them with the PWM
device using pwm_set_chip_data() (as I hinted at earlier).

> +static int tpu_pwm_config(struct pwm_chip *chip, struct pwm_device *_pwm,
> +			  int duty_ns, int period_ns)
[...]
> +	if (period_ns <= 0 || duty_ns < 0 || duty_ns > period_ns)
> +		return -EINVAL;

The core already performs these checks so they can be dropped.

> +	/* Pick a prescaler to avoid overflowing the counter.
> +	 * TODO: Pick the highest acceptable prescaler.
> +	 */
> +	clk_prepare_enable(tpu->clk);

Shouldn't you check the return value here?

> +	clk_rate = clk_get_rate(tpu->clk);
> +	clk_disable_unprepare(pwm->tpu->clk);

And does the clock really need to be enabled to obtain the rate?

> +	for (prescaler = 0; prescaler < ARRAY_SIZE(prescalers); ++prescaler) {
> +		period = clk_rate / prescalers[prescaler]
> +		       / (NSEC_PER_SEC / period_ns);

I prefer to have the operator on the previous line, as in:

		period = clk_rate / prescalers[prescaler] /
			 (NSEC_PER_SEC / period_ns);

> +	duty_only = pwm->prescaler == prescaler && pwm->period == period;

Maybe the following would be easier to read?

	if (prescaler == pwm->prescaler && period == pwm->period)
		duty_only = true;

And initialize duty_only = false when declaring it.

> +static int tpu_probe(struct platform_device *pdev)
> +{
> +	struct tpu_pwm_platform_data *pdata = pdev->dev.platform_data;
> +	struct tpu_device *tpu;
> +	struct resource *res;
> +	unsigned int i;
> +	int ret;
> +
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "missing platform data\n");
> +		return -ENXIO;
> +	}
> +
> +	tpu = devm_kzalloc(&pdev->dev, sizeof(*tpu), GFP_KERNEL);
> +	if (tpu == NULL) {
> +		dev_err(&pdev->dev, "failed to allocate driver data\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* Map memory, get clock and pin control. */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "failed to get I/O memory\n");
> +		return -ENXIO;
> +	}
> +
> +	tpu->base = devm_ioremap_nocache(&pdev->dev, res->start,
> +					 resource_size(res));
> +	if (tpu->base == NULL) {
> +		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> +		return -ENXIO;
> +	}

Isn't this mising a request_mem_region()? Couldn't you just use a call
to devm_ioremap_resource() instead?

> +static struct platform_driver tpu_driver = {
> +	.probe		= tpu_probe,
> +	.remove		= tpu_remove,
> +	.driver		= {
> +		.name	= "renesas_tpu_pwm",

Can this perhaps be renamed to "renesas-tpu-pwm" for consistency with
other drivers?

And I think this is missing

		.owner = THIS_MODULE,

as well. I know not all drivers have this either, but I have that on my
TODO already.

> diff --git a/include/linux/platform_data/pwm-renesas-tpu.h b/include/linux/platform_data/pwm-renesas-tpu.h
[...]
> +#define TPU_CHANNEL_MAX		4
> +
> +#define TPU_PWM_ID(device, channel) \
> +	((device) * TPU_CHANNEL_MAX + (channel))

Please don't do this. It assumes a global namespace for PWM devices.
However the global namespace is only for compatibility with legacy code
and shouldn't be depended on by new drivers. You should be using either
DT to dynamically refer to PWM devices or use a PWM lookup table. Refer
to Documentation/pwm.txt for details.

> +struct tpu_pwm_channel_data {
> +	bool enabled;
> +	bool active_low;

Maybe this should be enum pwm_polarity?

> +};
> +
> +struct tpu_pwm_platform_data {
> +	struct tpu_pwm_channel_data channels[TPU_CHANNEL_MAX];
> +};

This could be a little difficult to handle in the absence of
TPU_CHANNEL_MAX. Depending on the hardware, I suppose you could just
handle it via the .request() function. In that case all channels would
be disabled by default and enabled automatically when requested by a
user driver.

Thierry

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists