[08/15] pwm: Add new pwm-samsung driver

Commit Message

Tomasz Figa June 5, 2013, 9:18 p.m. UTC

This patch introduces new Samsung PWM driver, which uses Samsung
PWM/timer master driver to control shared parts of the hardware.

Signed-off-by: Tomasz Figa <[e-mail]>
---
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-samsung.c | 528 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 529 insertions(+)
 create mode 100644 drivers/pwm/pwm-samsung.c

Comments

Heiko Stübner June 13, 2013, 8:14 p.m. UTC | #1

Am Mittwoch, 5. Juni 2013, 23:18:13 schrieb Tomasz Figa:
> This patch introduces new Samsung PWM driver, which uses Samsung
> PWM/timer master driver to control shared parts of the hardware.
> 
> Signed-off-by: Tomasz Figa <[e-mail]>
> ---
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-samsung.c | 528
> ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 529
> insertions(+)
>  create mode 100644 drivers/pwm/pwm-samsung.c
> 
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile

[...]

> +static int pwm_samsung_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct samsung_pwm_chip *chip;
> +	struct resource *res;
> +	int ret;
> +
> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (chip == NULL) {
> +		dev_err(dev, "failed to allocate driver data\n");
> +		return -ENOMEM;
> +	}
> +
> +	chip->chip.dev = &pdev->dev;
> +	chip->chip.ops = &pwm_samsung_ops;
> +	chip->chip.base = -1;
> +	chip->chip.npwm = SAMSUNG_PWM_NUM;
> +
> +	if (pdev->dev.of_node) {
> +		ret = pwm_samsung_parse_dt(chip);
> +		if (ret)
> +			return ret;
> +
> +		chip->chip.of_xlate = of_pwm_xlate_with_flags;
> +		chip->chip.of_pwm_n_cells = 3;
> +	} else {
> +		if (!pdev->dev.platform_data) {
> +			dev_err(&pdev->dev, "no platform data specified\n");
> +			return -EINVAL;
> +		}
> +
> +		memcpy(&chip->variant, pdev->dev.platform_data,
> +							sizeof(chip->variant));
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "failed to get mem resource\n");
> +		return -ENOMEM;
> +	}
> +
> +	chip->base = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!chip->base) {
> +		dev_err(&pdev->dev, "failed to request and map registers\n");
> +		return -ENOMEM;
> +	}
> +
> +	chip->base_clk = devm_clk_get(&pdev->dev, "timers");
> +	if (IS_ERR(chip->base_clk)) {
> +		dev_err(dev, "failed to get timer base clk\n");
> +		return PTR_ERR(chip->base_clk);
> +	}
> +	clk_prepare_enable(chip->base_clk);
> +
> +	chip->tclk0 = devm_clk_get(&pdev->dev, "pwm-tclk0");
> +	chip->tclk1 = devm_clk_get(&pdev->dev, "pwm-tclk1");
> +
> +	ret = pwmchip_add(&chip->chip);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register pwm\n");
> +		goto err_clk_disable;
> +	}
> +
> +	dev_info(dev, "base_clk at %lu, tclk0 at %lu, tclk1 at %lu\n",
> +		clk_get_rate(chip->base_clk),
> +		!IS_ERR(chip->tclk0) ? clk_get_rate(chip->tclk0) : 0,
> +		!IS_ERR(chip->tclk1) ? clk_get_rate(chip->tclk1) : 0);

hmm, these values simply tell some internal state of the pwm, so wouldn't a 
dev_dbg be more appropriate?


> +
> +	platform_set_drvdata(pdev, chip);
> +
> +	return 0;
> +
> +err_clk_disable:
> +	clk_disable_unprepare(chip->base_clk);
> +
> +	return ret;
> +}

Tomasz Figa June 13, 2013, 8:18 p.m. UTC | #2

On Thursday 13 of June 2013 22:14:19 Heiko Stübner wrote:
> Am Mittwoch, 5. Juni 2013, 23:18:13 schrieb Tomasz Figa:
> > This patch introduces new Samsung PWM driver, which uses Samsung
> > PWM/timer master driver to control shared parts of the hardware.
> > 
> > Signed-off-by: Tomasz Figa <[e-mail]>
> > ---
> > 
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-samsung.c | 528
> > 
> > ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 529
> > insertions(+)
> > 
> >  create mode 100644 drivers/pwm/pwm-samsung.c
> > 
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> 
> [...]
> 
> > +static int pwm_samsung_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct samsung_pwm_chip *chip;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > +	if (chip == NULL) {
> > +		dev_err(dev, "failed to allocate driver data\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	chip->chip.dev = &pdev->dev;
> > +	chip->chip.ops = &pwm_samsung_ops;
> > +	chip->chip.base = -1;
> > +	chip->chip.npwm = SAMSUNG_PWM_NUM;
> > +
> > +	if (pdev->dev.of_node) {
> > +		ret = pwm_samsung_parse_dt(chip);
> > +		if (ret)
> > +			return ret;
> > +
> > +		chip->chip.of_xlate = of_pwm_xlate_with_flags;
> > +		chip->chip.of_pwm_n_cells = 3;
> > +	} else {
> > +		if (!pdev->dev.platform_data) {
> > +			dev_err(&pdev->dev, "no platform data 
specified\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		memcpy(&chip->variant, pdev->dev.platform_data,
> > +							sizeof(chip-
>variant));
> > +	}
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "failed to get mem resource\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	chip->base = devm_request_and_ioremap(&pdev->dev, res);
> > +	if (!chip->base) {
> > +		dev_err(&pdev->dev, "failed to request and map 
registers\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	chip->base_clk = devm_clk_get(&pdev->dev, "timers");
> > +	if (IS_ERR(chip->base_clk)) {
> > +		dev_err(dev, "failed to get timer base clk\n");
> > +		return PTR_ERR(chip->base_clk);
> > +	}
> > +	clk_prepare_enable(chip->base_clk);
> > +
> > +	chip->tclk0 = devm_clk_get(&pdev->dev, "pwm-tclk0");
> > +	chip->tclk1 = devm_clk_get(&pdev->dev, "pwm-tclk1");
> > +
> > +	ret = pwmchip_add(&chip->chip);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to register pwm\n");
> > +		goto err_clk_disable;
> > +	}
> > +
> > +	dev_info(dev, "base_clk at %lu, tclk0 at %lu, tclk1 at %lu\n",
> > +		clk_get_rate(chip->base_clk),
> > +		!IS_ERR(chip->tclk0) ? clk_get_rate(chip->tclk0) : 0,
> > +		!IS_ERR(chip->tclk1) ? clk_get_rate(chip->tclk1) : 0);
> 
> hmm, these values simply tell some internal state of the pwm, so
> wouldn't a dev_dbg be more appropriate?

Hmm, I have kept it as dev_info as in old driver, but now as you say it, 
dev_dbg might be more appropriate indeed, as it isn't really anything 
important that users should know...

Best regards,
Tomasz

> > +
> > +	platform_set_drvdata(pdev, chip);
> > +
> > +	return 0;
> > +
> > +err_clk_disable:
> > +	clk_disable_unprepare(chip->base_clk);
> > +
> > +	return ret;
> > +}

Tomasz Figa June 16, 2013, 5:19 p.m. UTC | #3

Hi Kukjin,

On Wednesday 05 of June 2013 23:18:13 Tomasz Figa wrote:
> This patch introduces new Samsung PWM driver, which uses Samsung
> PWM/timer master driver to control shared parts of the hardware.
> 
> Signed-off-by: Tomasz Figa <[e-mail]>
> ---
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-samsung.c | 528
> ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 529
> insertions(+)
>  create mode 100644 drivers/pwm/pwm-samsung.c

I you haven't yet merged this patch, please let me send v2 of this series, 
addressing Heiko's comments and adjustments to my understanding of the PWM 
hardware after getting access to SMDK6410 board, which uses PWM to control 
LCD backlight.

Best regards,
Tomasz

> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 229a599..833c3ac 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
>  obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
>  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung-legacy.o
> +obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
> diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
> new file mode 100644
> index 0000000..61bed3d
> --- /dev/null
> +++ b/drivers/pwm/pwm-samsung.c
> @@ -0,0 +1,528 @@
> +/* drivers/pwm/pwm-samsung.c
> + *
> + * Copyright (c) 2007 Ben Dooks
> + * Copyright (c) 2008 Simtec Electronics
> + *     Ben Dooks <[e-mail]>, <[e-mail]>
> + * Copyright (c) 2013 Tomasz Figa <[e-mail]>
> + *
> + * PWM driver for Samsung SoCs
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published
> by + * the Free Software Foundation; either version 2 of the License.
> +*/
> +
> +#include <linux/clk.h>
> +#include <linux/export.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/time.h>
> +
> +#include <clocksource/samsung_pwm.h>
> +
> +#define REG_TCFG0			0x00
> +#define REG_TCFG1			0x04
> +#define REG_TCON			0x08
> +
> +#define REG_TCNTB(tmr)			(0x0c + ((tmr) * 0xc))
> +#define REG_TCMPB(tmr)			(0x10 + ((tmr) * 0xc))
> +
> +#define TCFG0_PRESCALER_MASK		0xff
> +#define TCFG0_PRESCALER1_SHIFT		8
> +
> +#define TCFG1_MUX_MASK			0xf
> +#define TCFG1_SHIFT(x)			((x) * 4)
> +
> +#define TCON_START(chan)		(1 << (4 * (chan) + 0))
> +#define TCON_MANUALUPDATE(chan)		(1 << (4 * (chan) + 1))
> +#define TCON_INVERT(chan)		(1 << (4 * (chan) + 2))
> +#define TCON_AUTORELOAD(chan)		(1 << (4 * (chan) + 3))
> +
> +struct samsung_pwm_channel {
> +	unsigned long period_ns;
> +	unsigned long duty_ns;
> +	unsigned long tin_ns;
> +};
> +
> +struct samsung_pwm_chip {
> +	struct pwm_chip chip;
> +	struct samsung_pwm_variant variant;
> +	struct samsung_pwm_channel channels[SAMSUNG_PWM_NUM];
> +
> +	void __iomem *base;
> +	struct clk *base_clk;
> +	struct clk *tclk0;
> +	struct clk *tclk1;
> +};
> +#define to_samsung_pwm_chip(chip)	\
> +			container_of(chip, struct samsung_pwm_chip, chip)
> +
> +#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
> +static DEFINE_SPINLOCK(samsung_pwm_lock);
> +#endif
> +
> +static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
> +					unsigned int channel, u8 divisor)
> +{
> +	u8 shift = TCFG1_SHIFT(channel);
> +	unsigned long flags;
> +	u32 reg;
> +	u8 bits;
> +
> +	bits = (fls(divisor) - 1) - pwm->variant.div_base;
> +
> +	spin_lock_irqsave(&samsung_pwm_lock, flags);
> +
> +	reg = readl(pwm->base + REG_TCFG1);
> +	reg &= ~(TCFG1_MUX_MASK << shift);
> +	reg |= bits << shift;
> +	writel(reg, pwm->base + REG_TCFG1);
> +
> +	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
> +}
> +
> +static inline int pwm_samsung_is_tdiv(struct samsung_pwm_chip *chip,
> +							unsigned int chan)
> +{
> +	struct samsung_pwm_variant *variant = &chip->variant;
> +	u32 reg;
> +
> +	reg = readl(chip->base + REG_TCFG1);
> +	reg >>= TCFG1_SHIFT(chan);
> +	reg &= TCFG1_MUX_MASK;
> +
> +	return ((1 << reg) & variant->tclk_mask) == 0;
> +}
> +
> +static unsigned long pwm_samsung_get_tin_rate(struct samsung_pwm_chip
> *chip, +							unsigned 
int chan)
> +{
> +	unsigned long rate;
> +	u32 reg;
> +
> +	rate = clk_get_rate(chip->base_clk);
> +
> +	reg = readl(chip->base + REG_TCFG0);
> +	if (chan >= 2)
> +		reg >>= TCFG0_PRESCALER1_SHIFT;
> +	reg &= TCFG0_PRESCALER_MASK;
> +
> +	return rate / (reg + 1);
> +}
> +
> +static unsigned long pwm_samsung_calc_tin(struct samsung_pwm_chip
> *chip, +					unsigned int chan, 
unsigned long freq)
> +{
> +	struct samsung_pwm_variant *variant = &chip->variant;
> +	unsigned long rate;
> +	unsigned int div;
> +	struct clk *clk;
> +
> +	if (!pwm_samsung_is_tdiv(chip, chan)) {
> +		clk = (chan < 2) ? chip->tclk0 : chip->tclk1;
> +		if (!IS_ERR(clk)) {
> +			rate = clk_get_rate(clk);
> +			if (rate)
> +				return rate;
> +		}
> +
> +		dev_warn(chip->chip.dev,
> +			"tclk of PWM %d is inoperational, using tdiv\n", 
chan);
> +	}
> +
> +	rate = pwm_samsung_get_tin_rate(chip, chan);
> +	dev_dbg(chip->chip.dev, "tin parent at %lu\n", rate);
> +
> +	/*
> +	 * Compare minimum PWM frequency that can be achieved with 
possible
> +	 * divider settings and choose the lowest divisor that can 
generate
> +	 * frequencies lower than requested.
> +	 */
> +	for (div = variant->div_base; div < 4; ++div)
> +		if ((rate >> (variant->bits + div)) < freq)
> +			break;
> +
> +	pwm_samsung_set_divisor(chip, chan, 1 << div);
> +
> +	return rate >> div;
> +}
> +
> +static int pwm_samsung_request(struct pwm_chip *chip, struct pwm_device
> *pwm) +{
> +	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
> +
> +	if (our_chip->variant.output_mask & (1 << pwm->hwpwm))
> +		return 0;
> +
> +	dev_warn(our_chip->chip.dev,
> +			"tried to request PWM channel %d without 
output\n",
> +			pwm->hwpwm);
> +
> +	return -EINVAL;
> +}
> +
> +static int pwm_samsung_enable(struct pwm_chip *chip, struct pwm_device
> *pwm) +{
> +	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
> +	unsigned int channel = pwm->hwpwm;
> +	unsigned long tcon;
> +	unsigned long flags;
> +
> +	if (channel > 0)
> +		++channel;
> +
> +	spin_lock_irqsave(&samsung_pwm_lock, flags);
> +
> +	tcon = __raw_readl(our_chip->base + REG_TCON);
> +
> +	tcon &= ~TCON_MANUALUPDATE(channel);
> +	tcon |= TCON_START(channel) | TCON_AUTORELOAD(channel);
> +
> +	__raw_writel(tcon, our_chip->base + REG_TCON);
> +
> +	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
> +
> +	return 0;
> +}
> +
> +static void pwm_samsung_disable(struct pwm_chip *chip, struct
> pwm_device *pwm) +{
> +	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
> +	unsigned int channel = pwm->hwpwm;
> +	unsigned long tcon;
> +	unsigned long flags;
> +
> +	if (channel > 0)
> +		++channel;
> +
> +	spin_lock_irqsave(&samsung_pwm_lock, flags);
> +
> +	tcon = __raw_readl(our_chip->base + REG_TCON);
> +	tcon &= ~TCON_AUTORELOAD(channel);
> +	__raw_writel(tcon, our_chip->base + REG_TCON);
> +
> +	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
> +}
> +
> +static int pwm_samsung_config(struct pwm_chip *chip, struct pwm_device
> *pwm, +		int duty_ns, int period_ns)
> +{
> +	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
> +	struct samsung_pwm_channel *chan = &our_chip->channels[pwm-
>hwpwm];
> +	unsigned long tin_ns = chan->tin_ns;
> +	unsigned int tcon_chan = pwm->hwpwm;
> +	unsigned long tin_rate;
> +	unsigned long period;
> +	unsigned long flags;
> +	unsigned long tcnt;
> +	long tcmp;
> +	u32 tcon;
> +
> +	/* We currently avoid using 64bit arithmetic by using the
> +	 * fact that anything faster than 1Hz is easily representable
> +	 * by 32bits. */
> +	if (period_ns > NSEC_PER_SEC || duty_ns > NSEC_PER_SEC)
> +		return -ERANGE;
> +
> +	if (period_ns == chan->period_ns && duty_ns == chan->duty_ns)
> +		return 0;
> +
> +	/* The TCMP and TCNT can be read without a lock, they're not
> +	 * shared between the timers. */
> +	tcmp = readl(our_chip->base + REG_TCMPB(pwm->hwpwm));
> +	tcnt = readl(our_chip->base + REG_TCNTB(pwm->hwpwm));
> +
> +	period = NSEC_PER_SEC / period_ns;
> +
> +	dev_dbg(our_chip->chip.dev, "duty_ns=%d, period_ns=%d (%lu)\n",
> +		duty_ns, period_ns, period);
> +
> +	/* Check to see if we are changing the clock rate of the PWM */
> +	if (chan->period_ns != period_ns) {
> +		tin_rate = pwm_samsung_calc_tin(our_chip, pwm->hwpwm, 
period);
> +
> +		chan->period_ns = period_ns;
> +
> +		dev_dbg(our_chip->chip.dev, "tin_rate=%lu\n", tin_rate);
> +
> +		tin_ns = NSEC_PER_SEC / tin_rate;
> +		tcnt = period_ns / tin_ns;
> +
> +		chan->tin_ns = tin_ns;
> +	}
> +
> +	/* Note, counters count down */
> +	tcmp = duty_ns / tin_ns;
> +	tcmp = tcnt - tcmp;
> +
> +	/* the pwm hw only checks the compare register after a decrement,
> +	   so the pin never toggles if tcmp = tcnt */
> +	if (tcmp == tcnt)
> +		tcmp--;
> +
> +	dev_dbg(our_chip->chip.dev, "tin_ns=%lu, tcmp=%ld/%lu\n",
> +							tin_ns, tcmp, 
tcnt);
> +
> +	if (tcmp < 0)
> +		tcmp = 0;
> +
> +	/* Update the PWM register block. */
> +	if (tcon_chan > 0)
> +		++tcon_chan;
> +
> +	spin_lock_irqsave(&samsung_pwm_lock, flags);
> +
> +	tcon = __raw_readl(our_chip->base + REG_TCON);
> +
> +	tcnt--;
> +
> +	tcon &= ~(TCON_START(tcon_chan) | TCON_AUTORELOAD(tcon_chan));
> +	tcon |= TCON_MANUALUPDATE(tcon_chan);
> +
> +	__raw_writel(tcnt, our_chip->base + REG_TCNTB(pwm->hwpwm));
> +	__raw_writel(tcmp, our_chip->base + REG_TCMPB(pwm->hwpwm));
> +	__raw_writel(tcon, our_chip->base + REG_TCON);
> +
> +	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int pwm_samsung_set_polarity(struct pwm_chip *chip,
> +			struct pwm_device *pwm, enum pwm_polarity 
polarity)
> +{
> +	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
> +	unsigned int channel = pwm->hwpwm;
> +	unsigned long flags;
> +	u32 tcon;
> +
> +	if (channel > 0)
> +		++channel;
> +
> +	spin_lock_irqsave(&samsung_pwm_lock, flags);
> +
> +	tcon = __raw_readl(our_chip->base + REG_TCON);
> +
> +	/* Invert in hardware means normal polarity of PWM core */
> +	if (polarity == PWM_POLARITY_NORMAL)
> +		tcon |= TCON_INVERT(channel);
> +	else
> +		tcon &= ~TCON_INVERT(channel);
> +
> +	__raw_writel(tcon, our_chip->base + REG_TCON);
> +
> +	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
> +
> +	return 0;
> +}
> +
> +static struct pwm_ops pwm_samsung_ops = {
> +	.request	= pwm_samsung_request,
> +	.enable		= pwm_samsung_enable,
> +	.disable	= pwm_samsung_disable,
> +	.config		= pwm_samsung_config,
> +	.set_polarity	= pwm_samsung_set_polarity,
> +	.owner		= THIS_MODULE,
> +};
> +
> +#ifdef CONFIG_OF
> +static const struct samsung_pwm_variant s3c24xx_variant = {
> +	.bits		= 16,
> +	.div_base	= 1,
> +	.has_tint_cstat	= false,
> +	.tclk_mask	= (1 << 4),
> +};
> +
> +static const struct samsung_pwm_variant s3c64xx_variant = {
> +	.bits		= 32,
> +	.div_base	= 0,
> +	.has_tint_cstat	= true,
> +	.tclk_mask	= (1 << 7) | (1 << 6) | (1 << 5),
> +};
> +
> +static const struct samsung_pwm_variant s5p64x0_variant = {
> +	.bits		= 32,
> +	.div_base	= 0,
> +	.has_tint_cstat	= true,
> +	.tclk_mask	= 0,
> +};
> +
> +static const struct samsung_pwm_variant s5p_variant = {
> +	.bits		= 32,
> +	.div_base	= 0,
> +	.has_tint_cstat	= true,
> +	.tclk_mask	= (1 << 5),
> +};
> +
> +static const struct of_device_id samsung_pwm_matches[] = {
> +	{ .compatible = "samsung,s3c2410-pwm", .data = &s3c24xx_variant },
> +	{ .compatible = "samsung,s3c6400-pwm", .data = &s3c64xx_variant },
> +	{ .compatible = "samsung,s5p6440-pwm", .data = &s5p64x0_variant },
> +	{ .compatible = "samsung,s5pc100-pwm", .data = &s5p_variant },
> +	{ .compatible = "samsung,exynos4210-pwm", .data = &s5p64x0_variant 
},
> +	{},
> +};
> +#endif
> +
> +static int pwm_samsung_parse_dt(struct samsung_pwm_chip *chip)
> +{
> +	struct device_node *np = chip->chip.dev->of_node;
> +	const struct of_device_id *match;
> +	struct property *prop;
> +	const __be32 *cur;
> +	u32 val;
> +
> +	match = of_match_node(samsung_pwm_matches, np);
> +	if (!match)
> +		return -ENODEV;
> +
> +	memcpy(&chip->variant, match->data, sizeof(chip->variant));
> +
> +	of_property_for_each_u32(np, "samsung,pwm-outputs", prop, cur, 
val) {
> +		if (val >= SAMSUNG_PWM_NUM) {
> +			pr_warning("%s: invalid channel index in 
samsung,pwm-outputs
> property\n", +								
__func__);
> +			continue;
> +		}
> +		chip->variant.output_mask |= 1 << val;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pwm_samsung_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct samsung_pwm_chip *chip;
> +	struct resource *res;
> +	int ret;
> +
> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (chip == NULL) {
> +		dev_err(dev, "failed to allocate driver data\n");
> +		return -ENOMEM;
> +	}
> +
> +	chip->chip.dev = &pdev->dev;
> +	chip->chip.ops = &pwm_samsung_ops;
> +	chip->chip.base = -1;
> +	chip->chip.npwm = SAMSUNG_PWM_NUM;
> +
> +	if (pdev->dev.of_node) {
> +		ret = pwm_samsung_parse_dt(chip);
> +		if (ret)
> +			return ret;
> +
> +		chip->chip.of_xlate = of_pwm_xlate_with_flags;
> +		chip->chip.of_pwm_n_cells = 3;
> +	} else {
> +		if (!pdev->dev.platform_data) {
> +			dev_err(&pdev->dev, "no platform data 
specified\n");
> +			return -EINVAL;
> +		}
> +
> +		memcpy(&chip->variant, pdev->dev.platform_data,
> +							sizeof(chip-
>variant));
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "failed to get mem resource\n");
> +		return -ENOMEM;
> +	}
> +
> +	chip->base = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!chip->base) {
> +		dev_err(&pdev->dev, "failed to request and map 
registers\n");
> +		return -ENOMEM;
> +	}
> +
> +	chip->base_clk = devm_clk_get(&pdev->dev, "timers");
> +	if (IS_ERR(chip->base_clk)) {
> +		dev_err(dev, "failed to get timer base clk\n");
> +		return PTR_ERR(chip->base_clk);
> +	}
> +	clk_prepare_enable(chip->base_clk);
> +
> +	chip->tclk0 = devm_clk_get(&pdev->dev, "pwm-tclk0");
> +	chip->tclk1 = devm_clk_get(&pdev->dev, "pwm-tclk1");
> +
> +	ret = pwmchip_add(&chip->chip);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register pwm\n");
> +		goto err_clk_disable;
> +	}
> +
> +	dev_info(dev, "base_clk at %lu, tclk0 at %lu, tclk1 at %lu\n",
> +		clk_get_rate(chip->base_clk),
> +		!IS_ERR(chip->tclk0) ? clk_get_rate(chip->tclk0) : 0,
> +		!IS_ERR(chip->tclk1) ? clk_get_rate(chip->tclk1) : 0);
> +
> +	platform_set_drvdata(pdev, chip);
> +
> +	return 0;
> +
> +err_clk_disable:
> +	clk_disable_unprepare(chip->base_clk);
> +
> +	return ret;
> +}
> +
> +static int pwm_samsung_remove(struct platform_device *pdev)
> +{
> +	struct samsung_pwm_chip *chip = platform_get_drvdata(pdev);
> +	int err;
> +
> +	err = pwmchip_remove(&chip->chip);
> +	if (err < 0)
> +		return err;
> +
> +	clk_disable_unprepare(chip->base_clk);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int pwm_samsung_suspend(struct device *dev)
> +{
> +	struct samsung_pwm_chip *chip = dev_get_drvdata(dev);
> +	int i;
> +
> +	/* No one preserve these values during suspend so reset them
> +	 * Otherwise driver leaves PWM unconfigured if same values
> +	 * passed to pwm_config
> +	 */
> +	for (i = 0; i < SAMSUNG_PWM_NUM; ++i) {
> +		chip->channels[i].period_ns = 0;
> +		chip->channels[i].duty_ns = 0;
> +	}
> +
> +	return 0;
> +}
> +#endif
> +
> +static struct dev_pm_ops pwm_samsung_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pwm_samsung_suspend, NULL)
> +};
> +
> +static struct platform_driver pwm_samsung_driver = {
> +	.driver		= {
> +		.name	= "samsung-pwm",
> +		.owner	= THIS_MODULE,
> +		.pm	= &pwm_samsung_pm_ops,
> +		.of_match_table = of_match_ptr(samsung_pwm_matches),
> +	},
> +	.probe		= pwm_samsung_probe,
> +	.remove		= pwm_samsung_remove,
> +};
> +module_platform_driver(pwm_samsung_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Tomasz Figa <[e-mail]>");
> +MODULE_ALIAS("platform:samsung-pwm");

Kim Kukjin June 16, 2013, 8:45 p.m. UTC | #4

On 06/17/13 02:19, Tomasz Figa wrote:
> Hi Kukjin,
>
> On Wednesday 05 of June 2013 23:18:13 Tomasz Figa wrote:
>> This patch introduces new Samsung PWM driver, which uses Samsung
>> PWM/timer master driver to control shared parts of the hardware.
>>
>> Signed-off-by: Tomasz Figa<[e-mail]>
>> ---
>>   drivers/pwm/Makefile      |   1 +
>>   drivers/pwm/pwm-samsung.c | 528
>> ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 529
>> insertions(+)
>>   create mode 100644 drivers/pwm/pwm-samsung.c
>
> I you haven't yet merged this patch, please let me send v2 of this series,
> addressing Heiko's comments and adjustments to my understanding of the PWM
> hardware after getting access to SMDK6410 board, which uses PWM to control
> LCD backlight.
>
Sure, let me hold on until getting your updated series.

Thanks,
- Kukjin

Thierry Reding June 17, 2013, 8:29 p.m. UTC | #5

On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
> This patch introduces new Samsung PWM driver, which uses Samsung
> PWM/timer master driver to control shared parts of the hardware.
> 
> Signed-off-by: Tomasz Figa <[e-mail]>

Sorry for jumping in so late, I've been busy with other things lately.

> ---
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-samsung.c | 528 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 529 insertions(+)
>  create mode 100644 drivers/pwm/pwm-samsung.c
> 
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 229a599..833c3ac 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
>  obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
>  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung-legacy.o
> +obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
>  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
>  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
> diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
> new file mode 100644
> index 0000000..61bed3d
> --- /dev/null
> +++ b/drivers/pwm/pwm-samsung.c
> @@ -0,0 +1,528 @@
> +/* drivers/pwm/pwm-samsung.c

Nit: this line can be dropped. It serves no purpose.

> + *
> + * Copyright (c) 2007 Ben Dooks
> + * Copyright (c) 2008 Simtec Electronics
> + *     Ben Dooks <[e-mail]>, <[e-mail]>
> + * Copyright (c) 2013 Tomasz Figa <[e-mail]>
> + *
> + * PWM driver for Samsung SoCs
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License.
> +*/

Nit: the */ should align with the * above.

> +struct samsung_pwm_channel {
> +	unsigned long period_ns;
> +	unsigned long duty_ns;
> +	unsigned long tin_ns;
> +};
> +
> +struct samsung_pwm_chip {
> +	struct pwm_chip chip;
> +	struct samsung_pwm_variant variant;
> +	struct samsung_pwm_channel channels[SAMSUNG_PWM_NUM];

The new driver for Renesas did something similar, but I want to
discourage storing per-channel data within the chip structure.

The PWM framework provides a way to store this information along with
the PWM device (see pwm_{set,get}_chip_data()).

> +
> +	void __iomem *base;
> +	struct clk *base_clk;
> +	struct clk *tclk0;
> +	struct clk *tclk1;
> +};
> +#define to_samsung_pwm_chip(chip)	\
> +			container_of(chip, struct samsung_pwm_chip, chip)

Can you turn this into a static inline function please?

> +#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
> +static DEFINE_SPINLOCK(samsung_pwm_lock);
> +#endif

Why is this lock global? Shouldn't it more correctly be part of
samsung_pwm_chip?

> +static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
> +					unsigned int channel, u8 divisor)

Nit: please align arguments on subsequent lines with the first argument
of the first line. There's many more of these but I haven't mentioned
them all explicitly.

> +static inline int pwm_samsung_is_tdiv(struct samsung_pwm_chip *chip,

Any particular reason for making this inline?

> +static int pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +		int duty_ns, int period_ns)
> +{
> +	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
> +	struct samsung_pwm_channel *chan = &our_chip->channels[pwm->hwpwm];
> +	unsigned long tin_ns = chan->tin_ns;
> +	unsigned int tcon_chan = pwm->hwpwm;
> +	unsigned long tin_rate;
> +	unsigned long period;
> +	unsigned long flags;
> +	unsigned long tcnt;

Many of these unsigned long variable could be declared on a single line
to make the function shorter.

> +	long tcmp;
> +	u32 tcon;
> +
> +	/* We currently avoid using 64bit arithmetic by using the
> +	 * fact that anything faster than 1Hz is easily representable
> +	 * by 32bits. */

Can you turn these into proper block-style comments? Like so:

	/*
	 * We currently...
	 * ...
	 * by 32 bits.
	 */

> +	if (period_ns > NSEC_PER_SEC || duty_ns > NSEC_PER_SEC)
> +		return -ERANGE;

Note that technically you only need to check period_ns because the core
already ensures that duty_ns <= period_ns.

> +static int pwm_samsung_set_polarity(struct pwm_chip *chip,
> +			struct pwm_device *pwm, enum pwm_polarity polarity)
> +{
> +	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
> +	unsigned int channel = pwm->hwpwm;
> +	unsigned long flags;
> +	u32 tcon;
> +
> +	if (channel > 0)
> +		++channel;

You have to repeat that in quite a few places, so I wonder if it'd make
sense to wrap it into a function and add a comment about why the
increment is necessary.

> +static struct pwm_ops pwm_samsung_ops = {

"static const" please.

> +#ifdef CONFIG_OF
> +static const struct samsung_pwm_variant s3c24xx_variant = {
> +	.bits		= 16,
> +	.div_base	= 1,
> +	.has_tint_cstat	= false,
> +	.tclk_mask	= (1 << 4),
> +};
> +
> +static const struct samsung_pwm_variant s3c64xx_variant = {
> +	.bits		= 32,
> +	.div_base	= 0,
> +	.has_tint_cstat	= true,
> +	.tclk_mask	= (1 << 7) | (1 << 6) | (1 << 5),
> +};
> +
> +static const struct samsung_pwm_variant s5p64x0_variant = {
> +	.bits		= 32,
> +	.div_base	= 0,
> +	.has_tint_cstat	= true,
> +	.tclk_mask	= 0,
> +};
> +
> +static const struct samsung_pwm_variant s5p_variant = {
> +	.bits		= 32,
> +	.div_base	= 0,
> +	.has_tint_cstat	= true,
> +	.tclk_mask	= (1 << 5),
> +};
> +
> +static const struct of_device_id samsung_pwm_matches[] = {
> +	{ .compatible = "samsung,s3c2410-pwm", .data = &s3c24xx_variant },
> +	{ .compatible = "samsung,s3c6400-pwm", .data = &s3c64xx_variant },
> +	{ .compatible = "samsung,s5p6440-pwm", .data = &s5p64x0_variant },
> +	{ .compatible = "samsung,s5pc100-pwm", .data = &s5p_variant },
> +	{ .compatible = "samsung,exynos4210-pwm", .data = &s5p64x0_variant },
> +	{},
> +};
> +#endif
> +
> +static int pwm_samsung_parse_dt(struct samsung_pwm_chip *chip)
> +{
> +	struct device_node *np = chip->chip.dev->of_node;
> +	const struct of_device_id *match;
> +	struct property *prop;
> +	const __be32 *cur;
> +	u32 val;
> +
> +	match = of_match_node(samsung_pwm_matches, np);
> +	if (!match)
> +		return -ENODEV;
> +
> +	memcpy(&chip->variant, match->data, sizeof(chip->variant));
> +
> +	of_property_for_each_u32(np, "samsung,pwm-outputs", prop, cur, val) {
> +		if (val >= SAMSUNG_PWM_NUM) {
> +			pr_warning("%s: invalid channel index in samsung,pwm-outputs property\n",
> +								__func__);
> +			continue;
> +		}
> +		chip->variant.output_mask |= 1 << val;

Could the output_mask be moved to the struct samsung_pwm_chip instead?
The reason I ask is because it would allow you to make the variant
constant throughout the driver.

> +static int pwm_samsung_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct samsung_pwm_chip *chip;
> +	struct resource *res;
> +	int ret;
> +
> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (chip == NULL) {
> +		dev_err(dev, "failed to allocate driver data\n");
> +		return -ENOMEM;
> +	}
> +
> +	chip->chip.dev = &pdev->dev;
> +	chip->chip.ops = &pwm_samsung_ops;
> +	chip->chip.base = -1;
> +	chip->chip.npwm = SAMSUNG_PWM_NUM;
> +
> +	if (pdev->dev.of_node) {

Maybe add an IS_ENABLED(CONFIG_OF) check here? That'd allow all OF-
related code to be thrown away if OF isn't selected.

> +		ret = pwm_samsung_parse_dt(chip);
> +		if (ret)
> +			return ret;
> +
> +		chip->chip.of_xlate = of_pwm_xlate_with_flags;
> +		chip->chip.of_pwm_n_cells = 3;
> +	} else {
> +		if (!pdev->dev.platform_data) {
> +			dev_err(&pdev->dev, "no platform data specified\n");
> +			return -EINVAL;
> +		}
> +
> +		memcpy(&chip->variant, pdev->dev.platform_data,
> +							sizeof(chip->variant));
> +	}

Obviously this needs some modification in order for the variant to
become constant. But I think you can easily do so by making the driver
match using the platform_driver's id_table field, similar to how the
matching is done for OF.

> +	chip->base = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!chip->base) {
> +		dev_err(&pdev->dev, "failed to request and map registers\n");
> +		return -ENOMEM;
> +	}

devm_request_and_ioremap() is now deprecated and in the process of being
removed. You should use devm_ioremap_resource() instead.

> +
> +	chip->base_clk = devm_clk_get(&pdev->dev, "timers");
> +	if (IS_ERR(chip->base_clk)) {
> +		dev_err(dev, "failed to get timer base clk\n");
> +		return PTR_ERR(chip->base_clk);
> +	}
> +	clk_prepare_enable(chip->base_clk);

You need to check the return value of clk_prepare_enable(). And if I was
very pedantic, there should be a blank line before this one.

> +	ret = pwmchip_add(&chip->chip);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register pwm\n");

"failed to register PWM chip" please.

> +		goto err_clk_disable;
> +	}
> +
> +	dev_info(dev, "base_clk at %lu, tclk0 at %lu, tclk1 at %lu\n",
> +		clk_get_rate(chip->base_clk),
> +		!IS_ERR(chip->tclk0) ? clk_get_rate(chip->tclk0) : 0,
> +		!IS_ERR(chip->tclk1) ? clk_get_rate(chip->tclk1) : 0);
> +
> +	platform_set_drvdata(pdev, chip);
> +
> +	return 0;
> +
> +err_clk_disable:
> +	clk_disable_unprepare(chip->base_clk);
> +
> +	return ret;

There's only a single case where this can actually happen, so I don't
think you need the label here. Just put the clk_disable_unprepare() call
and the return statement where you jump to the label.

> +#ifdef CONFIG_PM

I think this should really be CONFIG_PM_SLEEP.

> +static struct dev_pm_ops pwm_samsung_pm_ops = {

"static const" please.

Thierry

Tomasz Figa June 17, 2013, 8:50 p.m. UTC | #6

Hi Thierry,

On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
> > This patch introduces new Samsung PWM driver, which uses Samsung
> > PWM/timer master driver to control shared parts of the hardware.
> > 
> > Signed-off-by: Tomasz Figa <[e-mail]>
> 
> Sorry for jumping in so late, I've been busy with other things lately.
> 
> > ---
> > 
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-samsung.c | 528
> >  ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 529
> >  insertions(+)
> >  create mode 100644 drivers/pwm/pwm-samsung.c
> > 
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 229a599..833c3ac 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
> > 
> >  obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
> >  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
> >  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung-legacy.o
> > 
> > +obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> > 
> >  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
> >  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> >  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
> > 
> > diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
> > new file mode 100644
> > index 0000000..61bed3d
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-samsung.c
> > @@ -0,0 +1,528 @@
> > +/* drivers/pwm/pwm-samsung.c
> 
> Nit: this line can be dropped. It serves no purpose.

OK.

> > + *
> > + * Copyright (c) 2007 Ben Dooks
> > + * Copyright (c) 2008 Simtec Electronics
> > + *     Ben Dooks <[e-mail]>, <[e-mail]>
> > + * Copyright (c) 2013 Tomasz Figa <[e-mail]>
> > + *
> > + * PWM driver for Samsung SoCs
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify + * it under the terms of the GNU General Public License as
> > published by + * the Free Software Foundation; either version 2 of
> > the License. +*/
> 
> Nit: the */ should align with the * above.

OK.

> > +struct samsung_pwm_channel {
> > +	unsigned long period_ns;
> > +	unsigned long duty_ns;
> > +	unsigned long tin_ns;
> > +};
> > +
> > +struct samsung_pwm_chip {
> > +	struct pwm_chip chip;
> > +	struct samsung_pwm_variant variant;
> > +	struct samsung_pwm_channel channels[SAMSUNG_PWM_NUM];
> 
> The new driver for Renesas did something similar, but I want to
> discourage storing per-channel data within the chip structure.
> 
> The PWM framework provides a way to store this information along with
> the PWM device (see pwm_{set,get}_chip_data()).

OK, this should be better indeed.

> > +
> > +	void __iomem *base;
> > +	struct clk *base_clk;
> > +	struct clk *tclk0;
> > +	struct clk *tclk1;
> > +};
> > +#define to_samsung_pwm_chip(chip)	\
> > +			container_of(chip, struct samsung_pwm_chip, chip)
> 
> Can you turn this into a static inline function please?

OK.

> > +#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
> > +static DEFINE_SPINLOCK(samsung_pwm_lock);
> > +#endif
> 
> Why is this lock global? Shouldn't it more correctly be part of
> samsung_pwm_chip?

There are few registers shared with samsung_pwm_timer clocksource driver 
and so normally the spinlock is exported from it. However on on some 
platforms (namely Exynos >=4x12) kernel can be compiled without that 
driver, so the lock must be defined locally, just to synchronize multiple 
PWM channels, as they share registers as well.

> > +static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
> > +					unsigned int channel, u8 divisor)
> 
> Nit: please align arguments on subsequent lines with the first argument
> of the first line. There's many more of these but I haven't mentioned
> them all explicitly.

OK.

> > +static inline int pwm_samsung_is_tdiv(struct samsung_pwm_chip *chip,
> 
> Any particular reason for making this inline?

Nope. Fixed in v2 that I'm going to send soon.

> > +static int pwm_samsung_config(struct pwm_chip *chip, struct
> > pwm_device *pwm, +		int duty_ns, int period_ns)
> > +{
> > +	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
> > +	struct samsung_pwm_channel *chan = &our_chip->channels[pwm-
>hwpwm];
> > +	unsigned long tin_ns = chan->tin_ns;
> > +	unsigned int tcon_chan = pwm->hwpwm;
> > +	unsigned long tin_rate;
> > +	unsigned long period;
> > +	unsigned long flags;
> > +	unsigned long tcnt;
> 
> Many of these unsigned long variable could be declared on a single line
> to make the function shorter.

OK. I have almost completely reworked this function in v2 and it needs 
just 5 variables here.

> > +	long tcmp;
> > +	u32 tcon;
> > +
> > +	/* We currently avoid using 64bit arithmetic by using the
> > +	 * fact that anything faster than 1Hz is easily representable
> > +	 * by 32bits. */
> 
> Can you turn these into proper block-style comments? Like so:

Sure.

> 	/*
> 	 * We currently...
> 	 * ...
> 	 * by 32 bits.
> 	 */
> 
> > +	if (period_ns > NSEC_PER_SEC || duty_ns > NSEC_PER_SEC)
> > +		return -ERANGE;
> 
> Note that technically you only need to check period_ns because the core
> already ensures that duty_ns <= period_ns.

OK.

> > +static int pwm_samsung_set_polarity(struct pwm_chip *chip,
> > +			struct pwm_device *pwm, enum pwm_polarity 
polarity)
> > +{
> > +	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
> > +	unsigned int channel = pwm->hwpwm;
> > +	unsigned long flags;
> > +	u32 tcon;
> > +
> > +	if (channel > 0)
> > +		++channel;
> 
> You have to repeat that in quite a few places, so I wonder if it'd make
> sense to wrap it into a function and add a comment about why the
> increment is necessary.

Hmm, might make sense to put this into a function indeed. Basically this 
is a trick to work around broken bit layout in TCON register.

> > +static struct pwm_ops pwm_samsung_ops = {
> 
> "static const" please.
> 

OK.

> > +#ifdef CONFIG_OF
> > +static const struct samsung_pwm_variant s3c24xx_variant = {
> > +	.bits		= 16,
> > +	.div_base	= 1,
> > +	.has_tint_cstat	= false,
> > +	.tclk_mask	= (1 << 4),
> > +};
> > +
> > +static const struct samsung_pwm_variant s3c64xx_variant = {
> > +	.bits		= 32,
> > +	.div_base	= 0,
> > +	.has_tint_cstat	= true,
> > +	.tclk_mask	= (1 << 7) | (1 << 6) | (1 << 5),
> > +};
> > +
> > +static const struct samsung_pwm_variant s5p64x0_variant = {
> > +	.bits		= 32,
> > +	.div_base	= 0,
> > +	.has_tint_cstat	= true,
> > +	.tclk_mask	= 0,
> > +};
> > +
> > +static const struct samsung_pwm_variant s5p_variant = {
> > +	.bits		= 32,
> > +	.div_base	= 0,
> > +	.has_tint_cstat	= true,
> > +	.tclk_mask	= (1 << 5),
> > +};
> > +
> > +static const struct of_device_id samsung_pwm_matches[] = {
> > +	{ .compatible = "samsung,s3c2410-pwm", .data = &s3c24xx_variant },
> > +	{ .compatible = "samsung,s3c6400-pwm", .data = &s3c64xx_variant },
> > +	{ .compatible = "samsung,s5p6440-pwm", .data = &s5p64x0_variant },
> > +	{ .compatible = "samsung,s5pc100-pwm", .data = &s5p_variant },
> > +	{ .compatible = "samsung,exynos4210-pwm", .data = &s5p64x0_variant
> > },
> > +	{},
> > +};
> > +#endif
> > +
> > +static int pwm_samsung_parse_dt(struct samsung_pwm_chip *chip)
> > +{
> > +	struct device_node *np = chip->chip.dev->of_node;
> > +	const struct of_device_id *match;
> > +	struct property *prop;
> > +	const __be32 *cur;
> > +	u32 val;
> > +
> > +	match = of_match_node(samsung_pwm_matches, np);
> > +	if (!match)
> > +		return -ENODEV;
> > +
> > +	memcpy(&chip->variant, match->data, sizeof(chip->variant));
> > +
> > +	of_property_for_each_u32(np, "samsung,pwm-outputs", prop, cur, 
val)
> > {
> > +		if (val >= SAMSUNG_PWM_NUM) {
> > +			pr_warning("%s: invalid channel index in 
samsung,pwm-outputs
> > property\n", +								
__func__);
> > +			continue;
> > +		}
> > +		chip->variant.output_mask |= 1 << val;
> 
> Could the output_mask be moved to the struct samsung_pwm_chip instead?
> The reason I ask is because it would allow you to make the variant
> constant throughout the driver.

Let me see what I can do about it.

> > +static int pwm_samsung_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct samsung_pwm_chip *chip;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > +	if (chip == NULL) {
> > +		dev_err(dev, "failed to allocate driver data\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	chip->chip.dev = &pdev->dev;
> > +	chip->chip.ops = &pwm_samsung_ops;
> > +	chip->chip.base = -1;
> > +	chip->chip.npwm = SAMSUNG_PWM_NUM;
> > +
> > +	if (pdev->dev.of_node) {
> 
> Maybe add an IS_ENABLED(CONFIG_OF) check here? That'd allow all OF-
> related code to be thrown away if OF isn't selected.

OK.

> > +		ret = pwm_samsung_parse_dt(chip);
> > +		if (ret)
> > +			return ret;
> > +
> > +		chip->chip.of_xlate = of_pwm_xlate_with_flags;
> > +		chip->chip.of_pwm_n_cells = 3;
> > +	} else {
> > +		if (!pdev->dev.platform_data) {
> > +			dev_err(&pdev->dev, "no platform data 
specified\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		memcpy(&chip->variant, pdev->dev.platform_data,
> > +							sizeof(chip-
>variant));
> > +	}
> 
> Obviously this needs some modification in order for the variant to
> become constant. But I think you can easily do so by making the driver
> match using the platform_driver's id_table field, similar to how the
> matching is done for OF.

Generally output_mask is board-dependent and is passed inside a variant 
struct using platform_data pointer.

Same platform data is used in samsung_pwm_timer clocksource driver, so I 
just reused it here without adding the need to rename platform device at 
runtime (see arch/arm/plat-samsung/devs.c).

> > +	chip->base = devm_request_and_ioremap(&pdev->dev, res);
> > +	if (!chip->base) {
> > +		dev_err(&pdev->dev, "failed to request and map 
registers\n");
> > +		return -ENOMEM;
> > +	}
> 
> devm_request_and_ioremap() is now deprecated and in the process of being
> removed. You should use devm_ioremap_resource() instead.

OK.

> > +
> > +	chip->base_clk = devm_clk_get(&pdev->dev, "timers");
> > +	if (IS_ERR(chip->base_clk)) {
> > +		dev_err(dev, "failed to get timer base clk\n");
> > +		return PTR_ERR(chip->base_clk);
> > +	}
> > +	clk_prepare_enable(chip->base_clk);
> 
> You need to check the return value of clk_prepare_enable(). And if I was
> very pedantic, there should be a blank line before this one.

OK.

> > +	ret = pwmchip_add(&chip->chip);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to register pwm\n");
> 
> "failed to register PWM chip" please.

OK.

> > +		goto err_clk_disable;
> > +	}
> > +
> > +	dev_info(dev, "base_clk at %lu, tclk0 at %lu, tclk1 at %lu\n",
> > +		clk_get_rate(chip->base_clk),
> > +		!IS_ERR(chip->tclk0) ? clk_get_rate(chip->tclk0) : 0,
> > +		!IS_ERR(chip->tclk1) ? clk_get_rate(chip->tclk1) : 0);
> > +
> > +	platform_set_drvdata(pdev, chip);
> > +
> > +	return 0;
> > +
> > +err_clk_disable:
> > +	clk_disable_unprepare(chip->base_clk);
> > +
> > +	return ret;
> 
> There's only a single case where this can actually happen, so I don't
> think you need the label here. Just put the clk_disable_unprepare() call
> and the return statement where you jump to the label.

Right. There were more labels here before and it made sense then, but now 
it really doesn't make any indeed.

> > +#ifdef CONFIG_PM
> 
> I think this should really be CONFIG_PM_SLEEP.

Right.

> > +static struct dev_pm_ops pwm_samsung_pm_ops = {
> 
> "static const" please.

OK.

Thanks for your review.

Generally this driver suffers from too many leftovers from old one that I 
was too lazy to completely rewrite. I have already fixed some of the 
issues you mentioned in v2 that I have in the works, but I will address 
rest of them as well.

Best regards,
Tomasz

Tomasz Figa June 18, 2013, 5:59 p.m. UTC | #7

Hi Thierry,

On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
> > This patch introduces new Samsung PWM driver, which uses Samsung
> > PWM/timer master driver to control shared parts of the hardware.
> > 
> > Signed-off-by: Tomasz Figa <[e-mail]>
> 
> Sorry for jumping in so late, I've been busy with other things lately.
> 
> > ---
> > 
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-samsung.c | 528
> >  ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 529
> >  insertions(+)
> >  create mode 100644 drivers/pwm/pwm-samsung.c
> > 
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 229a599..833c3ac 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
> > 
> >  obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
> >  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
> >  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung-legacy.o
> > 
> > +obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> > 
> >  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
> >  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> >  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
> > 
> > diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
> > new file mode 100644
> > index 0000000..61bed3d
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-samsung.c
> > @@ -0,0 +1,528 @@
> > +/* drivers/pwm/pwm-samsung.c
> 
> Nit: this line can be dropped. It serves no purpose.
> 
> > + *
> > + * Copyright (c) 2007 Ben Dooks
> > + * Copyright (c) 2008 Simtec Electronics
> > + *     Ben Dooks <[e-mail]>, <[e-mail]>
> > + * Copyright (c) 2013 Tomasz Figa <[e-mail]>
> > + *
> > + * PWM driver for Samsung SoCs
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify + * it under the terms of the GNU General Public License as
> > published by + * the Free Software Foundation; either version 2 of
> > the License. +*/
> 
> Nit: the */ should align with the * above.
> 
> > +struct samsung_pwm_channel {
> > +	unsigned long period_ns;
> > +	unsigned long duty_ns;
> > +	unsigned long tin_ns;
> > +};
> > +
> > +struct samsung_pwm_chip {
> > +	struct pwm_chip chip;
> > +	struct samsung_pwm_variant variant;
> > +	struct samsung_pwm_channel channels[SAMSUNG_PWM_NUM];
> 
> The new driver for Renesas did something similar, but I want to
> discourage storing per-channel data within the chip structure.
> 
> The PWM framework provides a way to store this information along with
> the PWM device (see pwm_{set,get}_chip_data()).
> 
> > +
> > +	void __iomem *base;
> > +	struct clk *base_clk;
> > +	struct clk *tclk0;
> > +	struct clk *tclk1;
> > +};
> > +#define to_samsung_pwm_chip(chip)	\
> > +			container_of(chip, struct samsung_pwm_chip, chip)
> 
> Can you turn this into a static inline function please?
> 
> > +#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
> > +static DEFINE_SPINLOCK(samsung_pwm_lock);
> > +#endif
> 
> Why is this lock global? Shouldn't it more correctly be part of
> samsung_pwm_chip?
> 
> > +static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
> > +					unsigned int channel, u8 divisor)
> 
> Nit: please align arguments on subsequent lines with the first argument
> of the first line. There's many more of these but I haven't mentioned
> them all explicitly.

Hmm, I'm addressing all your comments that aren't addressed yet in v2 at 
the moment and I'm wondering if this is really the correct way of breaking 
function headers...

According to Documentation/CodingStyle:

/* Quotation starts */
Statements longer than 80 columns will be broken into sensible chunks, 
unless exceeding 80 columns significantly increases readability and does 
not hide information. Descendants are always substantially shorter than 
the parent and are placed substantially to the right. The same applies to 
function headers with a long argument list. However, never break user-
visible strings such as printk messages, because that breaks the ability 
to grep for them.
/* Quotation ends */

Do I understand this incorrectly or does the above fragment state that 
broken lines must be aligned to the right?

Best regards,
Tomasz

> > +static inline int pwm_samsung_is_tdiv(struct samsung_pwm_chip *chip,
> 
> Any particular reason for making this inline?
> 
> > +static int pwm_samsung_config(struct pwm_chip *chip, struct
> > pwm_device *pwm, +		int duty_ns, int period_ns)
> > +{
> > +	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
> > +	struct samsung_pwm_channel *chan = &our_chip->channels[pwm-
>hwpwm];
> > +	unsigned long tin_ns = chan->tin_ns;
> > +	unsigned int tcon_chan = pwm->hwpwm;
> > +	unsigned long tin_rate;
> > +	unsigned long period;
> > +	unsigned long flags;
> > +	unsigned long tcnt;
> 
> Many of these unsigned long variable could be declared on a single line
> to make the function shorter.
> 
> > +	long tcmp;
> > +	u32 tcon;
> > +
> > +	/* We currently avoid using 64bit arithmetic by using the
> > +	 * fact that anything faster than 1Hz is easily representable
> > +	 * by 32bits. */
> 
> Can you turn these into proper block-style comments? Like so:
> 
> 	/*
> 	 * We currently...
> 	 * ...
> 	 * by 32 bits.
> 	 */
> 
> > +	if (period_ns > NSEC_PER_SEC || duty_ns > NSEC_PER_SEC)
> > +		return -ERANGE;
> 
> Note that technically you only need to check period_ns because the core
> already ensures that duty_ns <= period_ns.
> 
> > +static int pwm_samsung_set_polarity(struct pwm_chip *chip,
> > +			struct pwm_device *pwm, enum pwm_polarity 
polarity)
> > +{
> > +	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
> > +	unsigned int channel = pwm->hwpwm;
> > +	unsigned long flags;
> > +	u32 tcon;
> > +
> > +	if (channel > 0)
> > +		++channel;
> 
> You have to repeat that in quite a few places, so I wonder if it'd make
> sense to wrap it into a function and add a comment about why the
> increment is necessary.
> 
> > +static struct pwm_ops pwm_samsung_ops = {
> 
> "static const" please.
> 
> > +#ifdef CONFIG_OF
> > +static const struct samsung_pwm_variant s3c24xx_variant = {
> > +	.bits		= 16,
> > +	.div_base	= 1,
> > +	.has_tint_cstat	= false,
> > +	.tclk_mask	= (1 << 4),
> > +};
> > +
> > +static const struct samsung_pwm_variant s3c64xx_variant = {
> > +	.bits		= 32,
> > +	.div_base	= 0,
> > +	.has_tint_cstat	= true,
> > +	.tclk_mask	= (1 << 7) | (1 << 6) | (1 << 5),
> > +};
> > +
> > +static const struct samsung_pwm_variant s5p64x0_variant = {
> > +	.bits		= 32,
> > +	.div_base	= 0,
> > +	.has_tint_cstat	= true,
> > +	.tclk_mask	= 0,
> > +};
> > +
> > +static const struct samsung_pwm_variant s5p_variant = {
> > +	.bits		= 32,
> > +	.div_base	= 0,
> > +	.has_tint_cstat	= true,
> > +	.tclk_mask	= (1 << 5),
> > +};
> > +
> > +static const struct of_device_id samsung_pwm_matches[] = {
> > +	{ .compatible = "samsung,s3c2410-pwm", .data = &s3c24xx_variant },
> > +	{ .compatible = "samsung,s3c6400-pwm", .data = &s3c64xx_variant },
> > +	{ .compatible = "samsung,s5p6440-pwm", .data = &s5p64x0_variant },
> > +	{ .compatible = "samsung,s5pc100-pwm", .data = &s5p_variant },
> > +	{ .compatible = "samsung,exynos4210-pwm", .data = &s5p64x0_variant
> > },
> > +	{},
> > +};
> > +#endif
> > +
> > +static int pwm_samsung_parse_dt(struct samsung_pwm_chip *chip)
> > +{
> > +	struct device_node *np = chip->chip.dev->of_node;
> > +	const struct of_device_id *match;
> > +	struct property *prop;
> > +	const __be32 *cur;
> > +	u32 val;
> > +
> > +	match = of_match_node(samsung_pwm_matches, np);
> > +	if (!match)
> > +		return -ENODEV;
> > +
> > +	memcpy(&chip->variant, match->data, sizeof(chip->variant));
> > +
> > +	of_property_for_each_u32(np, "samsung,pwm-outputs", prop, cur, 
val)
> > {
> > +		if (val >= SAMSUNG_PWM_NUM) {
> > +			pr_warning("%s: invalid channel index in 
samsung,pwm-outputs
> > property\n", +								
__func__);
> > +			continue;
> > +		}
> > +		chip->variant.output_mask |= 1 << val;
> 
> Could the output_mask be moved to the struct samsung_pwm_chip instead?
> The reason I ask is because it would allow you to make the variant
> constant throughout the driver.
> 
> > +static int pwm_samsung_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct samsung_pwm_chip *chip;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > +	if (chip == NULL) {
> > +		dev_err(dev, "failed to allocate driver data\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	chip->chip.dev = &pdev->dev;
> > +	chip->chip.ops = &pwm_samsung_ops;
> > +	chip->chip.base = -1;
> > +	chip->chip.npwm = SAMSUNG_PWM_NUM;
> > +
> > +	if (pdev->dev.of_node) {
> 
> Maybe add an IS_ENABLED(CONFIG_OF) check here? That'd allow all OF-
> related code to be thrown away if OF isn't selected.
> 
> > +		ret = pwm_samsung_parse_dt(chip);
> > +		if (ret)
> > +			return ret;
> > +
> > +		chip->chip.of_xlate = of_pwm_xlate_with_flags;
> > +		chip->chip.of_pwm_n_cells = 3;
> > +	} else {
> > +		if (!pdev->dev.platform_data) {
> > +			dev_err(&pdev->dev, "no platform data 
specified\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		memcpy(&chip->variant, pdev->dev.platform_data,
> > +							sizeof(chip-
>variant));
> > +	}
> 
> Obviously this needs some modification in order for the variant to
> become constant. But I think you can easily do so by making the driver
> match using the platform_driver's id_table field, similar to how the
> matching is done for OF.
> 
> > +	chip->base = devm_request_and_ioremap(&pdev->dev, res);
> > +	if (!chip->base) {
> > +		dev_err(&pdev->dev, "failed to request and map 
registers\n");
> > +		return -ENOMEM;
> > +	}
> 
> devm_request_and_ioremap() is now deprecated and in the process of being
> removed. You should use devm_ioremap_resource() instead.
> 
> > +
> > +	chip->base_clk = devm_clk_get(&pdev->dev, "timers");
> > +	if (IS_ERR(chip->base_clk)) {
> > +		dev_err(dev, "failed to get timer base clk\n");
> > +		return PTR_ERR(chip->base_clk);
> > +	}
> > +	clk_prepare_enable(chip->base_clk);
> 
> You need to check the return value of clk_prepare_enable(). And if I was
> very pedantic, there should be a blank line before this one.
> 
> > +	ret = pwmchip_add(&chip->chip);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to register pwm\n");
> 
> "failed to register PWM chip" please.
> 
> > +		goto err_clk_disable;
> > +	}
> > +
> > +	dev_info(dev, "base_clk at %lu, tclk0 at %lu, tclk1 at %lu\n",
> > +		clk_get_rate(chip->base_clk),
> > +		!IS_ERR(chip->tclk0) ? clk_get_rate(chip->tclk0) : 0,
> > +		!IS_ERR(chip->tclk1) ? clk_get_rate(chip->tclk1) : 0);
> > +
> > +	platform_set_drvdata(pdev, chip);
> > +
> > +	return 0;
> > +
> > +err_clk_disable:
> > +	clk_disable_unprepare(chip->base_clk);
> > +
> > +	return ret;
> 
> There's only a single case where this can actually happen, so I don't
> think you need the label here. Just put the clk_disable_unprepare() call
> and the return statement where you jump to the label.
> 
> > +#ifdef CONFIG_PM
> 
> I think this should really be CONFIG_PM_SLEEP.
> 
> > +static struct dev_pm_ops pwm_samsung_pm_ops = {
> 
> "static const" please.
> 
> Thierry

Kim Kukjin June 18, 2013, 6:06 p.m. UTC | #8

On 06/19/13 02:59, Tomasz Figa wrote:
> Hi Thierry,
>

[...]

>>> +static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
>>> +					unsigned int channel, u8 divisor)
>>
>> Nit: please align arguments on subsequent lines with the first argument
>> of the first line. There's many more of these but I haven't mentioned
>> them all explicitly.
>
> Hmm, I'm addressing all your comments that aren't addressed yet in v2 at
> the moment and I'm wondering if this is really the correct way of breaking
> function headers...
>

static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
				    unsigned int channel, u8 divisor)


I also would preferred to use above style :)

- Kukjin

> According to Documentation/CodingStyle:
>
> /* Quotation starts */
> Statements longer than 80 columns will be broken into sensible chunks,
> unless exceeding 80 columns significantly increases readability and does
> not hide information. Descendants are always substantially shorter than
> the parent and are placed substantially to the right. The same applies to
> function headers with a long argument list. However, never break user-
> visible strings such as printk messages, because that breaks the ability
> to grep for them.
> /* Quotation ends */
>
> Do I understand this incorrectly or does the above fragment state that
> broken lines must be aligned to the right?
>
> Best regards,
> Tomasz

Tomasz Figa June 18, 2013, 6:13 p.m. UTC | #9

On Wednesday 19 of June 2013 03:06:31 Kukjin Kim wrote:
> On 06/19/13 02:59, Tomasz Figa wrote:
> > Hi Thierry,
> 
> [...]
> 
> >>> +static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
> >>> +					unsigned int channel, u8 divisor)
> >> 
> >> Nit: please align arguments on subsequent lines with the first
> >> argument
> >> of the first line. There's many more of these but I haven't mentioned
> >> them all explicitly.
> > 
> > Hmm, I'm addressing all your comments that aren't addressed yet in v2
> > at the moment and I'm wondering if this is really the correct way of
> > breaking function headers...
> 
> static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
> 				    unsigned int channel, u8 divisor)
> 
> 
> I also would preferred to use above style :)

Personally I find it looking better as well, but this is about being 
compliant with kernel coding style guidelines (which also says that 
indentation should be done using tabs). Please correct my understanding of 
the quote below if it is incorrect.

Best regards,
Tomasz

> 
> - Kukjin
> 
> > According to Documentation/CodingStyle:
> > 
> > /* Quotation starts */
> > Statements longer than 80 columns will be broken into sensible chunks,
> > unless exceeding 80 columns significantly increases readability and
> > does not hide information. Descendants are always substantially
> > shorter than the parent and are placed substantially to the right.
> > The same applies to function headers with a long argument list.
> > However, never break user- visible strings such as printk messages,
> > because that breaks the ability to grep for them.
> > /* Quotation ends */
> > 
> > Do I understand this incorrectly or does the above fragment state that
> > broken lines must be aligned to the right?
> > 
> > Best regards,
> > Tomasz

Tomasz Figa June 18, 2013, 7:41 p.m. UTC | #10

On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
> > This patch introduces new Samsung PWM driver, which uses Samsung
> > PWM/timer master driver to control shared parts of the hardware.
> > 
> > Signed-off-by: Tomasz Figa <[e-mail]>
> 
> Sorry for jumping in so late, I've been busy with other things lately.
> 
> > ---
> > 
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-samsung.c | 528
> >  ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 529
> >  insertions(+)
> >  create mode 100644 drivers/pwm/pwm-samsung.c
> > 
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 229a599..833c3ac 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
> > 
> >  obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
> >  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
> >  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung-legacy.o
> > 
> > +obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
> > 
> >  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
> >  obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> >  obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
> > 
> > diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
> > new file mode 100644
> > index 0000000..61bed3d
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-samsung.c
> > @@ -0,0 +1,528 @@
> > +/* drivers/pwm/pwm-samsung.c
> 
> Nit: this line can be dropped. It serves no purpose.
> 
> > + *
> > + * Copyright (c) 2007 Ben Dooks
> > + * Copyright (c) 2008 Simtec Electronics
> > + *     Ben Dooks <[e-mail]>, <[e-mail]>
> > + * Copyright (c) 2013 Tomasz Figa <[e-mail]>
> > + *
> > + * PWM driver for Samsung SoCs
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify + * it under the terms of the GNU General Public License as
> > published by + * the Free Software Foundation; either version 2 of
> > the License. +*/
> 
> Nit: the */ should align with the * above.
> 
> > +struct samsung_pwm_channel {
> > +	unsigned long period_ns;
> > +	unsigned long duty_ns;
> > +	unsigned long tin_ns;
> > +};
> > +
> > +struct samsung_pwm_chip {
> > +	struct pwm_chip chip;
> > +	struct samsung_pwm_variant variant;
> > +	struct samsung_pwm_channel channels[SAMSUNG_PWM_NUM];
> 
> The new driver for Renesas did something similar, but I want to
> discourage storing per-channel data within the chip structure.
> 
> The PWM framework provides a way to store this information along with
> the PWM device (see pwm_{set,get}_chip_data()).

OK, this looks good, but in my case is not really useful. I need to access 
those channel data in my suspend/resume callbacks and obviously I don't 
have access to any pwm_device there. Any suggestions?

Best regards,
Tomasz

> > +
> > +	void __iomem *base;
> > +	struct clk *base_clk;
> > +	struct clk *tclk0;
> > +	struct clk *tclk1;
> > +};
> > +#define to_samsung_pwm_chip(chip)	\
> > +			container_of(chip, struct samsung_pwm_chip, chip)
> 
> Can you turn this into a static inline function please?
> 
> > +#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
> > +static DEFINE_SPINLOCK(samsung_pwm_lock);
> > +#endif
> 
> Why is this lock global? Shouldn't it more correctly be part of
> samsung_pwm_chip?
> 
> > +static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
> > +					unsigned int channel, u8 divisor)
> 
> Nit: please align arguments on subsequent lines with the first argument
> of the first line. There's many more of these but I haven't mentioned
> them all explicitly.
> 
> > +static inline int pwm_samsung_is_tdiv(struct samsung_pwm_chip *chip,
> 
> Any particular reason for making this inline?
> 
> > +static int pwm_samsung_config(struct pwm_chip *chip, struct
> > pwm_device *pwm, +		int duty_ns, int period_ns)
> > +{
> > +	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
> > +	struct samsung_pwm_channel *chan = &our_chip->channels[pwm-
>hwpwm];
> > +	unsigned long tin_ns = chan->tin_ns;
> > +	unsigned int tcon_chan = pwm->hwpwm;
> > +	unsigned long tin_rate;
> > +	unsigned long period;
> > +	unsigned long flags;
> > +	unsigned long tcnt;
> 
> Many of these unsigned long variable could be declared on a single line
> to make the function shorter.
> 
> > +	long tcmp;
> > +	u32 tcon;
> > +
> > +	/* We currently avoid using 64bit arithmetic by using the
> > +	 * fact that anything faster than 1Hz is easily representable
> > +	 * by 32bits. */
> 
> Can you turn these into proper block-style comments? Like so:
> 
> 	/*
> 	 * We currently...
> 	 * ...
> 	 * by 32 bits.
> 	 */
> 
> > +	if (period_ns > NSEC_PER_SEC || duty_ns > NSEC_PER_SEC)
> > +		return -ERANGE;
> 
> Note that technically you only need to check period_ns because the core
> already ensures that duty_ns <= period_ns.
> 
> > +static int pwm_samsung_set_polarity(struct pwm_chip *chip,
> > +			struct pwm_device *pwm, enum pwm_polarity 
polarity)
> > +{
> > +	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
> > +	unsigned int channel = pwm->hwpwm;
> > +	unsigned long flags;
> > +	u32 tcon;
> > +
> > +	if (channel > 0)
> > +		++channel;
> 
> You have to repeat that in quite a few places, so I wonder if it'd make
> sense to wrap it into a function and add a comment about why the
> increment is necessary.
> 
> > +static struct pwm_ops pwm_samsung_ops = {
> 
> "static const" please.
> 
> > +#ifdef CONFIG_OF
> > +static const struct samsung_pwm_variant s3c24xx_variant = {
> > +	.bits		= 16,
> > +	.div_base	= 1,
> > +	.has_tint_cstat	= false,
> > +	.tclk_mask	= (1 << 4),
> > +};
> > +
> > +static const struct samsung_pwm_variant s3c64xx_variant = {
> > +	.bits		= 32,
> > +	.div_base	= 0,
> > +	.has_tint_cstat	= true,
> > +	.tclk_mask	= (1 << 7) | (1 << 6) | (1 << 5),
> > +};
> > +
> > +static const struct samsung_pwm_variant s5p64x0_variant = {
> > +	.bits		= 32,
> > +	.div_base	= 0,
> > +	.has_tint_cstat	= true,
> > +	.tclk_mask	= 0,
> > +};
> > +
> > +static const struct samsung_pwm_variant s5p_variant = {
> > +	.bits		= 32,
> > +	.div_base	= 0,
> > +	.has_tint_cstat	= true,
> > +	.tclk_mask	= (1 << 5),
> > +};
> > +
> > +static const struct of_device_id samsung_pwm_matches[] = {
> > +	{ .compatible = "samsung,s3c2410-pwm", .data = &s3c24xx_variant },
> > +	{ .compatible = "samsung,s3c6400-pwm", .data = &s3c64xx_variant },
> > +	{ .compatible = "samsung,s5p6440-pwm", .data = &s5p64x0_variant },
> > +	{ .compatible = "samsung,s5pc100-pwm", .data = &s5p_variant },
> > +	{ .compatible = "samsung,exynos4210-pwm", .data = &s5p64x0_variant
> > },
> > +	{},
> > +};
> > +#endif
> > +
> > +static int pwm_samsung_parse_dt(struct samsung_pwm_chip *chip)
> > +{
> > +	struct device_node *np = chip->chip.dev->of_node;
> > +	const struct of_device_id *match;
> > +	struct property *prop;
> > +	const __be32 *cur;
> > +	u32 val;
> > +
> > +	match = of_match_node(samsung_pwm_matches, np);
> > +	if (!match)
> > +		return -ENODEV;
> > +
> > +	memcpy(&chip->variant, match->data, sizeof(chip->variant));
> > +
> > +	of_property_for_each_u32(np, "samsung,pwm-outputs", prop, cur, 
val)
> > {
> > +		if (val >= SAMSUNG_PWM_NUM) {
> > +			pr_warning("%s: invalid channel index in 
samsung,pwm-outputs
> > property\n", +								
__func__);
> > +			continue;
> > +		}
> > +		chip->variant.output_mask |= 1 << val;
> 
> Could the output_mask be moved to the struct samsung_pwm_chip instead?
> The reason I ask is because it would allow you to make the variant
> constant throughout the driver.
> 
> > +static int pwm_samsung_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct samsung_pwm_chip *chip;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > +	if (chip == NULL) {
> > +		dev_err(dev, "failed to allocate driver data\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	chip->chip.dev = &pdev->dev;
> > +	chip->chip.ops = &pwm_samsung_ops;
> > +	chip->chip.base = -1;
> > +	chip->chip.npwm = SAMSUNG_PWM_NUM;
> > +
> > +	if (pdev->dev.of_node) {
> 
> Maybe add an IS_ENABLED(CONFIG_OF) check here? That'd allow all OF-
> related code to be thrown away if OF isn't selected.
> 
> > +		ret = pwm_samsung_parse_dt(chip);
> > +		if (ret)
> > +			return ret;
> > +
> > +		chip->chip.of_xlate = of_pwm_xlate_with_flags;
> > +		chip->chip.of_pwm_n_cells = 3;
> > +	} else {
> > +		if (!pdev->dev.platform_data) {
> > +			dev_err(&pdev->dev, "no platform data 
specified\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		memcpy(&chip->variant, pdev->dev.platform_data,
> > +							sizeof(chip-
>variant));
> > +	}
> 
> Obviously this needs some modification in order for the variant to
> become constant. But I think you can easily do so by making the driver
> match using the platform_driver's id_table field, similar to how the
> matching is done for OF.
> 
> > +	chip->base = devm_request_and_ioremap(&pdev->dev, res);
> > +	if (!chip->base) {
> > +		dev_err(&pdev->dev, "failed to request and map 
registers\n");
> > +		return -ENOMEM;
> > +	}
> 
> devm_request_and_ioremap() is now deprecated and in the process of being
> removed. You should use devm_ioremap_resource() instead.
> 
> > +
> > +	chip->base_clk = devm_clk_get(&pdev->dev, "timers");
> > +	if (IS_ERR(chip->base_clk)) {
> > +		dev_err(dev, "failed to get timer base clk\n");
> > +		return PTR_ERR(chip->base_clk);
> > +	}
> > +	clk_prepare_enable(chip->base_clk);
> 
> You need to check the return value of clk_prepare_enable(). And if I was
> very pedantic, there should be a blank line before this one.
> 
> > +	ret = pwmchip_add(&chip->chip);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to register pwm\n");
> 
> "failed to register PWM chip" please.
> 
> > +		goto err_clk_disable;
> > +	}
> > +
> > +	dev_info(dev, "base_clk at %lu, tclk0 at %lu, tclk1 at %lu\n",
> > +		clk_get_rate(chip->base_clk),
> > +		!IS_ERR(chip->tclk0) ? clk_get_rate(chip->tclk0) : 0,
> > +		!IS_ERR(chip->tclk1) ? clk_get_rate(chip->tclk1) : 0);
> > +
> > +	platform_set_drvdata(pdev, chip);
> > +
> > +	return 0;
> > +
> > +err_clk_disable:
> > +	clk_disable_unprepare(chip->base_clk);
> > +
> > +	return ret;
> 
> There's only a single case where this can actually happen, so I don't
> think you need the label here. Just put the clk_disable_unprepare() call
> and the return statement where you jump to the label.
> 
> > +#ifdef CONFIG_PM
> 
> I think this should really be CONFIG_PM_SLEEP.
> 
> > +static struct dev_pm_ops pwm_samsung_pm_ops = {
> 
> "static const" please.
> 
> Thierry

Thierry Reding June 18, 2013, 9:33 p.m. UTC | #11

On Tue, Jun 18, 2013 at 08:13:51PM +0200, Tomasz Figa wrote:
> On Wednesday 19 of June 2013 03:06:31 Kukjin Kim wrote:
> > On 06/19/13 02:59, Tomasz Figa wrote:
> > > Hi Thierry,
> > 
> > [...]
> > 
> > >>> +static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
> > >>> +					unsigned int channel, u8 divisor)
> > >> 
> > >> Nit: please align arguments on subsequent lines with the first
> > >> argument
> > >> of the first line. There's many more of these but I haven't mentioned
> > >> them all explicitly.
> > > 
> > > Hmm, I'm addressing all your comments that aren't addressed yet in v2
> > > at the moment and I'm wondering if this is really the correct way of
> > > breaking function headers...
> > 
> > static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
> > 				    unsigned int channel, u8 divisor)
> > 
> > 
> > I also would preferred to use above style :)
> 
> Personally I find it looking better as well, but this is about being 
> compliant with kernel coding style guidelines (which also says that 
> indentation should be done using tabs). Please correct my understanding of 
> the quote below if it is incorrect.

"placed substantially to the right" doesn't imply right-aligned. My
understanding is that it should be indented enough to make it stand
apart. I don't recall ever seeing right-aligned code.

And regarding tabs, you should be indenting using tabs as far as
possible and use spaces for the final alignment, so:

static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
				    unsigned int channel, u8 divisor)

In case your emailer doesn't highlight it, that's four tabs and four
spaces.

Thierry

Tomasz Figa June 18, 2013, 9:35 p.m. UTC | #12

On Tuesday 18 of June 2013 23:33:45 Thierry Reding wrote:
> On Tue, Jun 18, 2013 at 08:13:51PM +0200, Tomasz Figa wrote:
> > On Wednesday 19 of June 2013 03:06:31 Kukjin Kim wrote:
> > > On 06/19/13 02:59, Tomasz Figa wrote:
> > > > Hi Thierry,
> > > 
> > > [...]
> > > 
> > > >>> +static void pwm_samsung_set_divisor(struct samsung_pwm_chip
> > > >>> *pwm,
> > > >>> +					unsigned int channel, u8 
divisor)
> > > >> 
> > > >> Nit: please align arguments on subsequent lines with the first
> > > >> argument
> > > >> of the first line. There's many more of these but I haven't
> > > >> mentioned
> > > >> them all explicitly.
> > > > 
> > > > Hmm, I'm addressing all your comments that aren't addressed yet in
> > > > v2
> > > > at the moment and I'm wondering if this is really the correct way
> > > > of
> > > > breaking function headers...
> > > 
> > > static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
> > > 
> > > 				    unsigned int channel, u8 divisor)
> > > 
> > > I also would preferred to use above style :)
> > 
> > Personally I find it looking better as well, but this is about being
> > compliant with kernel coding style guidelines (which also says that
> > indentation should be done using tabs). Please correct my
> > understanding of the quote below if it is incorrect.
> 
> "placed substantially to the right" doesn't imply right-aligned. My
> understanding is that it should be indented enough to make it stand
> apart. I don't recall ever seeing right-aligned code.
> 
> And regarding tabs, you should be indenting using tabs as far as
> possible and use spaces for the final alignment, so:
> 
> static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
> 				    unsigned int channel, u8 divisor)
> 
> In case your emailer doesn't highlight it, that's four tabs and four
> spaces.

OK. Thank you for clearing up my misunderstanding.

Btw. I see it as spaces-only in my mail client somehow.

Best regards,
Tomasz

Thierry Reding June 18, 2013, 9:40 p.m. UTC | #13

On Tue, Jun 18, 2013 at 09:41:09PM +0200, Tomasz Figa wrote:
> On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> > On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
[...]
> > > +struct samsung_pwm_channel {
> > > +	unsigned long period_ns;
> > > +	unsigned long duty_ns;
> > > +	unsigned long tin_ns;
> > > +};
> > > +
> > > +struct samsung_pwm_chip {
> > > +	struct pwm_chip chip;
> > > +	struct samsung_pwm_variant variant;
> > > +	struct samsung_pwm_channel channels[SAMSUNG_PWM_NUM];
> > 
> > The new driver for Renesas did something similar, but I want to
> > discourage storing per-channel data within the chip structure.
> > 
> > The PWM framework provides a way to store this information along with
> > the PWM device (see pwm_{set,get}_chip_data()).
> 
> OK, this looks good, but in my case is not really useful. I need to access 
> those channel data in my suspend/resume callbacks and obviously I don't 
> have access to any pwm_device there. Any suggestions?

You do have access to the struct pwm_chip, and that already has an array
of struct pwm_devices, so you could obtain the driver-specific data from
those (see pwm_chip.pwms).

Thierry

Tomasz Figa June 18, 2013, 9:42 p.m. UTC | #14

On Tuesday 18 of June 2013 23:40:23 Thierry Reding wrote:
> On Tue, Jun 18, 2013 at 09:41:09PM +0200, Tomasz Figa wrote:
> > On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> > > On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
> [...]
> 
> > > > +struct samsung_pwm_channel {
> > > > +	unsigned long period_ns;
> > > > +	unsigned long duty_ns;
> > > > +	unsigned long tin_ns;
> > > > +};
> > > > +
> > > > +struct samsung_pwm_chip {
> > > > +	struct pwm_chip chip;
> > > > +	struct samsung_pwm_variant variant;
> > > > +	struct samsung_pwm_channel channels[SAMSUNG_PWM_NUM];
> > > 
> > > The new driver for Renesas did something similar, but I want to
> > > discourage storing per-channel data within the chip structure.
> > > 
> > > The PWM framework provides a way to store this information along
> > > with
> > > the PWM device (see pwm_{set,get}_chip_data()).
> > 
> > OK, this looks good, but in my case is not really useful. I need to
> > access those channel data in my suspend/resume callbacks and
> > obviously I don't have access to any pwm_device there. Any
> > suggestions?
> 
> You do have access to the struct pwm_chip, and that already has an array
> of struct pwm_devices, so you could obtain the driver-specific data
> from those (see pwm_chip.pwms).

Well, if it's legal to access internals of pwm_chip from PWM drivers then 
there is no problem. Based on other subsystems, like regulators or clocks 
I have concluded that this structure should be dereferenced only inside 
PWM core.

Best regards,
Tomasz

Thierry Reding June 18, 2013, 10:17 p.m. UTC | #15

On Mon, Jun 17, 2013 at 10:50:40PM +0200, Tomasz Figa wrote:
> On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> > On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
[...]
> > > +#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
> > > +static DEFINE_SPINLOCK(samsung_pwm_lock);
> > > +#endif
> > 
> > Why is this lock global? Shouldn't it more correctly be part of
> > samsung_pwm_chip?
> 
> There are few registers shared with samsung_pwm_timer clocksource driver 
> and so normally the spinlock is exported from it. However on on some 
> platforms (namely Exynos >=4x12) kernel can be compiled without that 
> driver, so the lock must be defined locally, just to synchronize multiple 
> PWM channels, as they share registers as well.

Okay, I think this needs further explanation. The clocksource driver is
used for what exactly? From a quick look it seems to be very much PWM-
specific. According to the device tree binding for the PWM driver, the
timer blocks can also be used as clock sources and clock event timers.
So if I understand correctly you have setups where you use one or more
channels as clock source or clock event timer and one or more channels
as PWM outputs.

In that case it's a very bad idea to use a global lock to synchronize
accesses. You need to do much more than that. To properly split this
across several drivers there needs to be a mechanism to allocate
channels for use either as clock source/event timer or PWM. Otherwise,
how do you know that drivers aren't stepping on each other's toes?

> > > +		ret = pwm_samsung_parse_dt(chip);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		chip->chip.of_xlate = of_pwm_xlate_with_flags;
> > > +		chip->chip.of_pwm_n_cells = 3;
> > > +	} else {
> > > +		if (!pdev->dev.platform_data) {
> > > +			dev_err(&pdev->dev, "no platform data 
> specified\n");
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		memcpy(&chip->variant, pdev->dev.platform_data,
> > > +							sizeof(chip-
> >variant));
> > > +	}
> > 
> > Obviously this needs some modification in order for the variant to
> > become constant. But I think you can easily do so by making the driver
> > match using the platform_driver's id_table field, similar to how the
> > matching is done for OF.
> 
> Generally output_mask is board-dependent and is passed inside a variant 
> struct using platform_data pointer.

That's okay. But output_mask is the only thing that's board-dependent.
Everything else in the variant is SoC dependent judging by the OF device
table. So really only the output_mask should be part of the platform
data.

> Same platform data is used in samsung_pwm_timer clocksource driver, so I 
> just reused it here without adding the need to rename platform device at 
> runtime (see arch/arm/plat-samsung/devs.c).

Looking a bit at git log for the clocksource driver, there's this
commit:

	a3ce54f clocksource: samsung_pwm_timer: Do not request PWM mem region

That's an ugly workaround for sharing registers between two drivers.
There's a reason why drivers do request_mem_region(), and it is
precisely to prevent them from accessing the same registers. As I
already said above, I think you need to come up with some sort of API to
share resources between the drivers.

There was a similar issue a few months back with the pwm-tiehrpwm and
pwm-tiecap drivers, which use a shared block of registers. Initially
something similar was done as you do here, but eventually we came up
with a much better solution that involved introducing a new driver for
the shared functionality and an exported API.

The situation seems to be somewhat different here since you actually
share the same resources for different functionality instead of sharing
one subset of register across multiple drivers, but I think a similar
solution can be applied here.

Thierry

Thierry Reding June 18, 2013, 10:20 p.m. UTC | #16

On Tue, Jun 18, 2013 at 11:42:37PM +0200, Tomasz Figa wrote:
> On Tuesday 18 of June 2013 23:40:23 Thierry Reding wrote:
> > On Tue, Jun 18, 2013 at 09:41:09PM +0200, Tomasz Figa wrote:
> > > On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> > > > On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
> > [...]
> > 
> > > > > +struct samsung_pwm_channel {
> > > > > +	unsigned long period_ns;
> > > > > +	unsigned long duty_ns;
> > > > > +	unsigned long tin_ns;
> > > > > +};
> > > > > +
> > > > > +struct samsung_pwm_chip {
> > > > > +	struct pwm_chip chip;
> > > > > +	struct samsung_pwm_variant variant;
> > > > > +	struct samsung_pwm_channel channels[SAMSUNG_PWM_NUM];
> > > > 
> > > > The new driver for Renesas did something similar, but I want to
> > > > discourage storing per-channel data within the chip structure.
> > > > 
> > > > The PWM framework provides a way to store this information along
> > > > with
> > > > the PWM device (see pwm_{set,get}_chip_data()).
> > > 
> > > OK, this looks good, but in my case is not really useful. I need to
> > > access those channel data in my suspend/resume callbacks and
> > > obviously I don't have access to any pwm_device there. Any
> > > suggestions?
> > 
> > You do have access to the struct pwm_chip, and that already has an array
> > of struct pwm_devices, so you could obtain the driver-specific data
> > from those (see pwm_chip.pwms).
> 
> Well, if it's legal to access internals of pwm_chip from PWM drivers then 
> there is no problem. Based on other subsystems, like regulators or clocks 
> I have concluded that this structure should be dereferenced only inside 
> PWM core.

It's quite alright to do that. As a matter of fact you already do the
same within the .probe() callback to set things up.

Thierry

Tomasz Figa June 18, 2013, 10:45 p.m. UTC | #17

On Wednesday 19 of June 2013 00:17:06 Thierry Reding wrote:
> On Mon, Jun 17, 2013 at 10:50:40PM +0200, Tomasz Figa wrote:
> > On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> > > On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
> [...]
> 
> > > > +#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
> > > > +static DEFINE_SPINLOCK(samsung_pwm_lock);
> > > > +#endif
> > > 
> > > Why is this lock global? Shouldn't it more correctly be part of
> > > samsung_pwm_chip?
> > 
> > There are few registers shared with samsung_pwm_timer clocksource
> > driver and so normally the spinlock is exported from it. However on
> > on some platforms (namely Exynos >=4x12) kernel can be compiled
> > without that driver, so the lock must be defined locally, just to
> > synchronize multiple PWM channels, as they share registers as well.
> 
> Okay, I think this needs further explanation. The clocksource driver is
> used for what exactly? From a quick look it seems to be very much PWM-
> specific. According to the device tree binding for the PWM driver, the
> timer blocks can also be used as clock sources and clock event timers.
> So if I understand correctly you have setups where you use one or more
> channels as clock source or clock event timer and one or more channels
> as PWM outputs.
> 
> In that case it's a very bad idea to use a global lock to synchronize
> accesses. You need to do much more than that. To properly split this
> across several drivers there needs to be a mechanism to allocate
> channels for use either as clock source/event timer or PWM. Otherwise,
> how do you know that drivers aren't stepping on each other's toes?
> 
> > > > +		ret = pwm_samsung_parse_dt(chip);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		chip->chip.of_xlate = of_pwm_xlate_with_flags;
> > > > +		chip->chip.of_pwm_n_cells = 3;
> > > > +	} else {
> > > > +		if (!pdev->dev.platform_data) {
> > > > +			dev_err(&pdev->dev, "no platform data
> > 
> > specified\n");
> > 
> > > > +			return -EINVAL;
> > > > +		}
> > > > +
> > > > +		memcpy(&chip->variant, pdev->dev.platform_data,
> > > > +							
sizeof(chip-
> > >
> > >variant));
> > >
> > > > +	}
> > > 
> > > Obviously this needs some modification in order for the variant to
> > > become constant. But I think you can easily do so by making the
> > > driver
> > > match using the platform_driver's id_table field, similar to how the
> > > matching is done for OF.
> > 
> > Generally output_mask is board-dependent and is passed inside a
> > variant
> > struct using platform_data pointer.
> 
> That's okay. But output_mask is the only thing that's board-dependent.
> Everything else in the variant is SoC dependent judging by the OF device
> table. So really only the output_mask should be part of the platform
> data.
> 
> > Same platform data is used in samsung_pwm_timer clocksource driver, so
> > I just reused it here without adding the need to rename platform
> > device at runtime (see arch/arm/plat-samsung/devs.c).
> 
> Looking a bit at git log for the clocksource driver, there's this
> commit:
> 
> 	a3ce54f clocksource: samsung_pwm_timer: Do not request PWM mem 
region
> 
> That's an ugly workaround for sharing registers between two drivers.
> There's a reason why drivers do request_mem_region(), and it is
> precisely to prevent them from accessing the same registers. As I
> already said above, I think you need to come up with some sort of API to
> share resources between the drivers.
> 
> There was a similar issue a few months back with the pwm-tiehrpwm and
> pwm-tiecap drivers, which use a shared block of registers. Initially
> something similar was done as you do here, but eventually we came up
> with a much better solution that involved introducing a new driver for
> the shared functionality and an exported API.
> 
> The situation seems to be somewhat different here since you actually
> share the same resources for different functionality instead of sharing
> one subset of register across multiple drivers, but I think a similar
> solution can be applied here.

Well, current design, or rather lack of thereof, has been established 
after a significant amount of discussion, mostly with ARM SoC and MFD 
maintainers.

It started from a simple reorganization of the clocksource driver to be 
multiplatform friendly, without any means of synchronizing both drivers 
other than local_irq_save() and _restore() that used to be there 
originally, before I started my work on this. This was enough to work 
correctly, because both drivers at the same time are used only on 
uniprocessor systems.

Here's the thread with patches:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16664/focus=17267

Then I opened a Pandora's Box by asking for opinion in this thread:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16480/focus=16500

After that I started working on a complex framework for sharing this IP 
block that would solve all the problems with synchronization, channel 
allocation, device tree parsing, variant management, etc.

Here's an initial version, which started as an MFD master driver, which 
would let 2 client drivers use the hardware (only the clocksource part was 
implemented in that series):
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17464/focus=229250

Still, that version didn't receive too good feedback, with comments 
pointing me to change the architecture to master-slave, with the 
clocksource driver being the master and PWM driver being the slave. And so 
v5 was made:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17864

This version was generally accepted, but then we discussed on IRC (mostly 
me and Arnd) whether such complex infrastructure is really needed and 
concluded that for this platform a simple shared spinlock would be enough, 
based on following reasons:
- on all SoCs on which the clocksource driver is going to be used there is 
only one instance of the PWM block, with 5 channels
- on all existing boards there are always at least two channels that don't 
have outputs
- operations on particular PWM channels must be synchronized anyway, 
because there are registers shared by all PWM channels (TCON being most 
important where enable, autoreload and invert bits are located for all 
channels).

And so we got to current design which is basically a shared spinlock and 
per-board output_mask, which specifies which channels have outputs on 
particular boards. If you look to the clocksource driver, you can see that 
it tries to allocate two output-less channels for timekeeping purposes and 
if it fails to do so, it simply panics.

IMHO this solution is fine, because it's simple, has little overhead and 
it just works in case of platforms for which it is needed.

Best regards,
Tomasz

Thierry Reding June 19, 2013, 9:43 a.m. UTC | #18

On Wed, Jun 19, 2013 at 12:45:02AM +0200, Tomasz Figa wrote:
> On Wednesday 19 of June 2013 00:17:06 Thierry Reding wrote:
> > On Mon, Jun 17, 2013 at 10:50:40PM +0200, Tomasz Figa wrote:
> > > On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> > > > On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
> > [...]
> > 
> > > > > +#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
> > > > > +static DEFINE_SPINLOCK(samsung_pwm_lock);
> > > > > +#endif
> > > > 
> > > > Why is this lock global? Shouldn't it more correctly be part of
> > > > samsung_pwm_chip?
> > > 
> > > There are few registers shared with samsung_pwm_timer clocksource
> > > driver and so normally the spinlock is exported from it. However on
> > > on some platforms (namely Exynos >=4x12) kernel can be compiled
> > > without that driver, so the lock must be defined locally, just to
> > > synchronize multiple PWM channels, as they share registers as well.
> > 
> > Okay, I think this needs further explanation. The clocksource driver is
> > used for what exactly? From a quick look it seems to be very much PWM-
> > specific. According to the device tree binding for the PWM driver, the
> > timer blocks can also be used as clock sources and clock event timers.
> > So if I understand correctly you have setups where you use one or more
> > channels as clock source or clock event timer and one or more channels
> > as PWM outputs.
> > 
> > In that case it's a very bad idea to use a global lock to synchronize
> > accesses. You need to do much more than that. To properly split this
> > across several drivers there needs to be a mechanism to allocate
> > channels for use either as clock source/event timer or PWM. Otherwise,
> > how do you know that drivers aren't stepping on each other's toes?
> > 
> > > > > +		ret = pwm_samsung_parse_dt(chip);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +
> > > > > +		chip->chip.of_xlate = of_pwm_xlate_with_flags;
> > > > > +		chip->chip.of_pwm_n_cells = 3;
> > > > > +	} else {
> > > > > +		if (!pdev->dev.platform_data) {
> > > > > +			dev_err(&pdev->dev, "no platform data
> > > 
> > > specified\n");
> > > 
> > > > > +			return -EINVAL;
> > > > > +		}
> > > > > +
> > > > > +		memcpy(&chip->variant, pdev->dev.platform_data,
> > > > > +							
> sizeof(chip-
> > > >
> > > >variant));
> > > >
> > > > > +	}
> > > > 
> > > > Obviously this needs some modification in order for the variant to
> > > > become constant. But I think you can easily do so by making the
> > > > driver
> > > > match using the platform_driver's id_table field, similar to how the
> > > > matching is done for OF.
> > > 
> > > Generally output_mask is board-dependent and is passed inside a
> > > variant
> > > struct using platform_data pointer.
> > 
> > That's okay. But output_mask is the only thing that's board-dependent.
> > Everything else in the variant is SoC dependent judging by the OF device
> > table. So really only the output_mask should be part of the platform
> > data.
> > 
> > > Same platform data is used in samsung_pwm_timer clocksource driver, so
> > > I just reused it here without adding the need to rename platform
> > > device at runtime (see arch/arm/plat-samsung/devs.c).
> > 
> > Looking a bit at git log for the clocksource driver, there's this
> > commit:
> > 
> > 	a3ce54f clocksource: samsung_pwm_timer: Do not request PWM mem 
> region
> > 
> > That's an ugly workaround for sharing registers between two drivers.
> > There's a reason why drivers do request_mem_region(), and it is
> > precisely to prevent them from accessing the same registers. As I
> > already said above, I think you need to come up with some sort of API to
> > share resources between the drivers.
> > 
> > There was a similar issue a few months back with the pwm-tiehrpwm and
> > pwm-tiecap drivers, which use a shared block of registers. Initially
> > something similar was done as you do here, but eventually we came up
> > with a much better solution that involved introducing a new driver for
> > the shared functionality and an exported API.
> > 
> > The situation seems to be somewhat different here since you actually
> > share the same resources for different functionality instead of sharing
> > one subset of register across multiple drivers, but I think a similar
> > solution can be applied here.
> 
> Well, current design, or rather lack of thereof, has been established 
> after a significant amount of discussion, mostly with ARM SoC and MFD 
> maintainers.
> 
> It started from a simple reorganization of the clocksource driver to be 
> multiplatform friendly, without any means of synchronizing both drivers 
> other than local_irq_save() and _restore() that used to be there 
> originally, before I started my work on this. This was enough to work 
> correctly, because both drivers at the same time are used only on 
> uniprocessor systems.
> 
> Here's the thread with patches:
> http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16664/focus=17267
> 
> Then I opened a Pandora's Box by asking for opinion in this thread:
> http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16480/focus=16500
> 
> After that I started working on a complex framework for sharing this IP 
> block that would solve all the problems with synchronization, channel 
> allocation, device tree parsing, variant management, etc.
> 
> Here's an initial version, which started as an MFD master driver, which 
> would let 2 client drivers use the hardware (only the clocksource part was 
> implemented in that series):
> http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17464/focus=229250
> 
> Still, that version didn't receive too good feedback, with comments 
> pointing me to change the architecture to master-slave, with the 
> clocksource driver being the master and PWM driver being the slave. And so 
> v5 was made:
> http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17864

Yeah, this seemed to be going in the right direction.

> This version was generally accepted, but then we discussed on IRC (mostly 
> me and Arnd) whether such complex infrastructure is really needed and 
> concluded that for this platform a simple shared spinlock would be enough, 
> based on following reasons:
> - on all SoCs on which the clocksource driver is going to be used there is 
> only one instance of the PWM block, with 5 channels

You say that now. But then at some point somebody decides that it might
be useful to have a second instance.

> - on all existing boards there are always at least two channels that don't 
> have outputs

I don't see how that's relevant here.

> - operations on particular PWM channels must be synchronized anyway, 
> because there are registers shared by all PWM channels (TCON being most 
> important where enable, autoreload and invert bits are located for all 
> channels).

And that's precisely why there should be a more explicit way of
synchronizing than just acquiring a lock. Otherwise you have no
possibility whatsoever to detect when a board tries to use both the
clocksource and PWM drivers in incompatible ways. The lock may prevent
concurrent accesses to the shared registers, but that won't help you if
the PWM driver suddenly decides to use an output that was originally
meant to be used as clock source.

> And so we got to current design which is basically a shared spinlock and 
> per-board output_mask, which specifies which channels have outputs on 
> particular boards. If you look to the clocksource driver, you can see that 
> it tries to allocate two output-less channels for timekeeping purposes and 
> if it fails to do so, it simply panics.
> 
> IMHO this solution is fine, because it's simple, has little overhead and 
> it just works in case of platforms for which it is needed.

Okay, I like simple. But I see too much potential for this to break
in the future. There are just too many assumptions for my taste. It
isn't anything that I'm looking forward to have to maintain.

Thierry

Tomasz Figa June 19, 2013, 10:23 a.m. UTC | #19

On Wednesday 19 of June 2013 11:43:56 Thierry Reding wrote:
> On Wed, Jun 19, 2013 at 12:45:02AM +0200, Tomasz Figa wrote:
> > On Wednesday 19 of June 2013 00:17:06 Thierry Reding wrote:
> > > On Mon, Jun 17, 2013 at 10:50:40PM +0200, Tomasz Figa wrote:
> > > > On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> > > > > On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
> > > [...]
> > > 
> > > > > > +#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
> > > > > > +static DEFINE_SPINLOCK(samsung_pwm_lock);
> > > > > > +#endif
> > > > > 
> > > > > Why is this lock global? Shouldn't it more correctly be part of
> > > > > samsung_pwm_chip?
> > > > 
> > > > There are few registers shared with samsung_pwm_timer clocksource
> > > > driver and so normally the spinlock is exported from it. However on
> > > > on some platforms (namely Exynos >=4x12) kernel can be compiled
> > > > without that driver, so the lock must be defined locally, just to
> > > > synchronize multiple PWM channels, as they share registers as well.
> > > 
> > > Okay, I think this needs further explanation. The clocksource driver
> > > is
> > > used for what exactly? From a quick look it seems to be very much
> > > PWM-
> > > specific. According to the device tree binding for the PWM driver,
> > > the
> > > timer blocks can also be used as clock sources and clock event
> > > timers.
> > > So if I understand correctly you have setups where you use one or
> > > more
> > > channels as clock source or clock event timer and one or more
> > > channels
> > > as PWM outputs.
> > > 
> > > In that case it's a very bad idea to use a global lock to synchronize
> > > accesses. You need to do much more than that. To properly split this
> > > across several drivers there needs to be a mechanism to allocate
> > > channels for use either as clock source/event timer or PWM.
> > > Otherwise,
> > > how do you know that drivers aren't stepping on each other's toes?
> > > 
> > > > > > +		ret = pwm_samsung_parse_dt(chip);
> > > > > > +		if (ret)
> > > > > > +			return ret;
> > > > > > +
> > > > > > +		chip->chip.of_xlate = of_pwm_xlate_with_flags;
> > > > > > +		chip->chip.of_pwm_n_cells = 3;
> > > > > > +	} else {
> > > > > > +		if (!pdev->dev.platform_data) {
> > > > > > +			dev_err(&pdev->dev, "no platform data
> > > > 
> > > > specified\n");
> > > > 
> > > > > > +			return -EINVAL;
> > > > > > +		}
> > > > > > +
> > > > > > +		memcpy(&chip->variant, pdev->dev.platform_data,
> > > > > > +
> > 
> > sizeof(chip-
> > 
> > > > >variant));
> > > > >
> > > > > > +	}
> > > > > 
> > > > > Obviously this needs some modification in order for the variant
> > > > > to
> > > > > become constant. But I think you can easily do so by making the
> > > > > driver
> > > > > match using the platform_driver's id_table field, similar to how
> > > > > the
> > > > > matching is done for OF.
> > > > 
> > > > Generally output_mask is board-dependent and is passed inside a
> > > > variant
> > > > struct using platform_data pointer.
> > > 
> > > That's okay. But output_mask is the only thing that's
> > > board-dependent.
> > > Everything else in the variant is SoC dependent judging by the OF
> > > device
> > > table. So really only the output_mask should be part of the platform
> > > data.
> > > 
> > > > Same platform data is used in samsung_pwm_timer clocksource driver,
> > > > so
> > > > I just reused it here without adding the need to rename platform
> > > > device at runtime (see arch/arm/plat-samsung/devs.c).
> > > 
> > > Looking a bit at git log for the clocksource driver, there's this
> > > 
> > > commit:
> > > 	a3ce54f clocksource: samsung_pwm_timer: Do not request PWM mem
> > 
> > region
> > 
> > > That's an ugly workaround for sharing registers between two drivers.
> > > There's a reason why drivers do request_mem_region(), and it is
> > > precisely to prevent them from accessing the same registers. As I
> > > already said above, I think you need to come up with some sort of API
> > > to
> > > share resources between the drivers.
> > > 
> > > There was a similar issue a few months back with the pwm-tiehrpwm and
> > > pwm-tiecap drivers, which use a shared block of registers. Initially
> > > something similar was done as you do here, but eventually we came up
> > > with a much better solution that involved introducing a new driver
> > > for
> > > the shared functionality and an exported API.
> > > 
> > > The situation seems to be somewhat different here since you actually
> > > share the same resources for different functionality instead of
> > > sharing
> > > one subset of register across multiple drivers, but I think a similar
> > > solution can be applied here.
> > 
> > Well, current design, or rather lack of thereof, has been established
> > after a significant amount of discussion, mostly with ARM SoC and MFD
> > maintainers.
> > 
> > It started from a simple reorganization of the clocksource driver to be
> > multiplatform friendly, without any means of synchronizing both drivers
> > other than local_irq_save() and _restore() that used to be there
> > originally, before I started my work on this. This was enough to work
> > correctly, because both drivers at the same time are used only on
> > uniprocessor systems.
> > 
> > Here's the thread with patches:
> > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16664/focus=1726
> > 7
> > 
> > Then I opened a Pandora's Box by asking for opinion in this thread:
> > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16480/focus=1650
> > 0
> > 
> > After that I started working on a complex framework for sharing this IP
> > block that would solve all the problems with synchronization, channel
> > allocation, device tree parsing, variant management, etc.
> > 
> > Here's an initial version, which started as an MFD master driver, which
> > would let 2 client drivers use the hardware (only the clocksource part
> > was implemented in that series):
> > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17464/focus=2292
> > 50
> > 
> > Still, that version didn't receive too good feedback, with comments
> > pointing me to change the architecture to master-slave, with the
> > clocksource driver being the master and PWM driver being the slave. And
> > so v5 was made:
> > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17864
> 
> Yeah, this seemed to be going in the right direction.
> 
> > This version was generally accepted, but then we discussed on IRC
> > (mostly me and Arnd) whether such complex infrastructure is really
> > needed and concluded that for this platform a simple shared spinlock
> > would be enough, based on following reasons:
> > - on all SoCs on which the clocksource driver is going to be used there
> > is only one instance of the PWM block, with 5 channels
> 
> You say that now. But then at some point somebody decides that it might
> be useful to have a second instance.

No, he won't.

Simply because no new SoC is going to be made which will use the 
samsung_pwm_timer driver and all existing ones have only a single instance 
of this IP.

> > - on all existing boards there are always at least two channels that
> > don't have outputs
> 
> I don't see how that's relevant here.

This is the information passed through output_mask. Clocksource driver can 
use _only_ channels without output and PWM driver can use _only_ channels 
with output.

Both drivers receive the same output_mask value, either from the same 
variant structure or from device tree.

> > - operations on particular PWM channels must be synchronized anyway,
> > because there are registers shared by all PWM channels (TCON being most
> > important where enable, autoreload and invert bits are located for all
> > channels).
> 
> And that's precisely why there should be a more explicit way of
> synchronizing than just acquiring a lock. Otherwise you have no
> possibility whatsoever to detect when a board tries to use both the
> clocksource and PWM drivers in incompatible ways. The lock may prevent
> concurrent accesses to the shared registers, but that won't help you if
> the PWM driver suddenly decides to use an output that was originally
> meant to be used as clock source.

See above.

> > And so we got to current design which is basically a shared spinlock
> > and
> > per-board output_mask, which specifies which channels have outputs on
> > particular boards. If you look to the clocksource driver, you can see
> > that it tries to allocate two output-less channels for timekeeping
> > purposes and if it fails to do so, it simply panics.
> > 
> > IMHO this solution is fine, because it's simple, has little overhead
> > and
> > it just works in case of platforms for which it is needed.
> 
> Okay, I like simple. But I see too much potential for this to break
> in the future. There are just too many assumptions for my taste. It
> isn't anything that I'm looking forward to have to maintain.

Those are completely safe assumptions for existing platforms. Any new 
platform with the same PWM IP will use this driver exclusively, without the 
clocksource driver and so without the problems you mentioned.

Best regards,
Tomasz

Tomasz Figa June 24, 2013, 5:52 p.m. UTC | #20

Hi Thierry,

On Wednesday 19 of June 2013 12:23:52 Tomasz Figa wrote:
> On Wednesday 19 of June 2013 11:43:56 Thierry Reding wrote:
> > On Wed, Jun 19, 2013 at 12:45:02AM +0200, Tomasz Figa wrote:
> > > On Wednesday 19 of June 2013 00:17:06 Thierry Reding wrote:
> > > > On Mon, Jun 17, 2013 at 10:50:40PM +0200, Tomasz Figa wrote:
> > > > > On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> > > > > > On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
> > > > [...]
> > > > 
> > > > > > > +#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
> > > > > > > +static DEFINE_SPINLOCK(samsung_pwm_lock);
> > > > > > > +#endif
> > > > > > 
> > > > > > Why is this lock global? Shouldn't it more correctly be part
> > > > > > of
> > > > > > samsung_pwm_chip?
> > > > > 
> > > > > There are few registers shared with samsung_pwm_timer
> > > > > clocksource
> > > > > driver and so normally the spinlock is exported from it. However
> > > > > on
> > > > > on some platforms (namely Exynos >=4x12) kernel can be compiled
> > > > > without that driver, so the lock must be defined locally, just
> > > > > to
> > > > > synchronize multiple PWM channels, as they share registers as
> > > > > well.
> > > > 
> > > > Okay, I think this needs further explanation. The clocksource
> > > > driver
> > > > is
> > > > used for what exactly? From a quick look it seems to be very much
> > > > PWM-
> > > > specific. According to the device tree binding for the PWM driver,
> > > > the
> > > > timer blocks can also be used as clock sources and clock event
> > > > timers.
> > > > So if I understand correctly you have setups where you use one or
> > > > more
> > > > channels as clock source or clock event timer and one or more
> > > > channels
> > > > as PWM outputs.
> > > > 
> > > > In that case it's a very bad idea to use a global lock to
> > > > synchronize
> > > > accesses. You need to do much more than that. To properly split
> > > > this
> > > > across several drivers there needs to be a mechanism to allocate
> > > > channels for use either as clock source/event timer or PWM.
> > > > Otherwise,
> > > > how do you know that drivers aren't stepping on each other's toes?
> > > > 
> > > > > > > +		ret = pwm_samsung_parse_dt(chip);
> > > > > > > +		if (ret)
> > > > > > > +			return ret;
> > > > > > > +
> > > > > > > +		chip->chip.of_xlate = of_pwm_xlate_with_flags;
> > > > > > > +		chip->chip.of_pwm_n_cells = 3;
> > > > > > > +	} else {
> > > > > > > +		if (!pdev->dev.platform_data) {
> > > > > > > +			dev_err(&pdev->dev, "no platform data
> > > > > 
> > > > > specified\n");
> > > > > 
> > > > > > > +			return -EINVAL;
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		memcpy(&chip->variant, pdev->dev.platform_data,
> > > > > > > +
> > > 
> > > sizeof(chip-
> > > 
> > > > > >variant));
> > > > > >
> > > > > > > +	}
> > > > > > 
> > > > > > Obviously this needs some modification in order for the
> > > > > > variant
> > > > > > to
> > > > > > become constant. But I think you can easily do so by making
> > > > > > the
> > > > > > driver
> > > > > > match using the platform_driver's id_table field, similar to
> > > > > > how
> > > > > > the
> > > > > > matching is done for OF.
> > > > > 
> > > > > Generally output_mask is board-dependent and is passed inside a
> > > > > variant
> > > > > struct using platform_data pointer.
> > > > 
> > > > That's okay. But output_mask is the only thing that's
> > > > board-dependent.
> > > > Everything else in the variant is SoC dependent judging by the OF
> > > > device
> > > > table. So really only the output_mask should be part of the
> > > > platform
> > > > data.
> > > > 
> > > > > Same platform data is used in samsung_pwm_timer clocksource
> > > > > driver,
> > > > > so
> > > > > I just reused it here without adding the need to rename platform
> > > > > device at runtime (see arch/arm/plat-samsung/devs.c).
> > > > 
> > > > Looking a bit at git log for the clocksource driver, there's this
> > > > 
> > > > commit:
> > > > 	a3ce54f clocksource: samsung_pwm_timer: Do not request PWM 
mem
> > > 
> > > region
> > > 
> > > > That's an ugly workaround for sharing registers between two
> > > > drivers.
> > > > There's a reason why drivers do request_mem_region(), and it is
> > > > precisely to prevent them from accessing the same registers. As I
> > > > already said above, I think you need to come up with some sort of
> > > > API
> > > > to
> > > > share resources between the drivers.
> > > > 
> > > > There was a similar issue a few months back with the pwm-tiehrpwm
> > > > and
> > > > pwm-tiecap drivers, which use a shared block of registers.
> > > > Initially
> > > > something similar was done as you do here, but eventually we came
> > > > up
> > > > with a much better solution that involved introducing a new driver
> > > > for
> > > > the shared functionality and an exported API.
> > > > 
> > > > The situation seems to be somewhat different here since you
> > > > actually
> > > > share the same resources for different functionality instead of
> > > > sharing
> > > > one subset of register across multiple drivers, but I think a
> > > > similar
> > > > solution can be applied here.
> > > 
> > > Well, current design, or rather lack of thereof, has been
> > > established
> > > after a significant amount of discussion, mostly with ARM SoC and
> > > MFD
> > > maintainers.
> > > 
> > > It started from a simple reorganization of the clocksource driver to
> > > be
> > > multiplatform friendly, without any means of synchronizing both
> > > drivers
> > > other than local_irq_save() and _restore() that used to be there
> > > originally, before I started my work on this. This was enough to
> > > work
> > > correctly, because both drivers at the same time are used only on
> > > uniprocessor systems.
> > > 
> > > Here's the thread with patches:
> > > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16664/focus=1
> > > 726
> > > 7
> > > 
> > > Then I opened a Pandora's Box by asking for opinion in this thread:
> > > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16480/focus=1
> > > 650
> > > 0
> > > 
> > > After that I started working on a complex framework for sharing this
> > > IP
> > > block that would solve all the problems with synchronization,
> > > channel
> > > allocation, device tree parsing, variant management, etc.
> > > 
> > > Here's an initial version, which started as an MFD master driver,
> > > which
> > > would let 2 client drivers use the hardware (only the clocksource
> > > part
> > > was implemented in that series):
> > > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17464/focus=2
> > > 292
> > > 50
> > > 
> > > Still, that version didn't receive too good feedback, with comments
> > > pointing me to change the architecture to master-slave, with the
> > > clocksource driver being the master and PWM driver being the slave.
> > > And
> > > so v5 was made:
> > > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17864
> > 
> > Yeah, this seemed to be going in the right direction.
> > 
> > > This version was generally accepted, but then we discussed on IRC
> > > (mostly me and Arnd) whether such complex infrastructure is really
> > > needed and concluded that for this platform a simple shared spinlock
> > > would be enough, based on following reasons:
> > > - on all SoCs on which the clocksource driver is going to be used
> > > there
> > > is only one instance of the PWM block, with 5 channels
> > 
> > You say that now. But then at some point somebody decides that it
> > might
> > be useful to have a second instance.
> 
> No, he won't.
> 
> Simply because no new SoC is going to be made which will use the
> samsung_pwm_timer driver and all existing ones have only a single
> instance of this IP.
> 
> > > - on all existing boards there are always at least two channels that
> > > don't have outputs
> > 
> > I don't see how that's relevant here.
> 
> This is the information passed through output_mask. Clocksource driver
> can use _only_ channels without output and PWM driver can use _only_
> channels with output.
> 
> Both drivers receive the same output_mask value, either from the same
> variant structure or from device tree.
> 
> > > - operations on particular PWM channels must be synchronized anyway,
> > > because there are registers shared by all PWM channels (TCON being
> > > most
> > > important where enable, autoreload and invert bits are located for
> > > all
> > > channels).
> > 
> > And that's precisely why there should be a more explicit way of
> > synchronizing than just acquiring a lock. Otherwise you have no
> > possibility whatsoever to detect when a board tries to use both the
> > clocksource and PWM drivers in incompatible ways. The lock may prevent
> > concurrent accesses to the shared registers, but that won't help you
> > if
> > the PWM driver suddenly decides to use an output that was originally
> > meant to be used as clock source.
> 
> See above.
> 
> > > And so we got to current design which is basically a shared spinlock
> > > and
> > > per-board output_mask, which specifies which channels have outputs
> > > on
> > > particular boards. If you look to the clocksource driver, you can
> > > see
> > > that it tries to allocate two output-less channels for timekeeping
> > > purposes and if it fails to do so, it simply panics.
> > > 
> > > IMHO this solution is fine, because it's simple, has little overhead
> > > and
> > > it just works in case of platforms for which it is needed.
> > 
> > Okay, I like simple. But I see too much potential for this to break
> > in the future. There are just too many assumptions for my taste. It
> > isn't anything that I'm looking forward to have to maintain.
> 
> Those are completely safe assumptions for existing platforms. Any new
> platform with the same PWM IP will use this driver exclusively, without
> the clocksource driver and so without the problems you mentioned.

Would you mind taking into account things I pointed in reply above?

Best regards,
Tomasz

Thierry Reding June 24, 2013, 7:50 p.m. UTC | #21

On Mon, Jun 24, 2013 at 07:52:11PM +0200, Tomasz Figa wrote:
> Hi Thierry,
> 
> On Wednesday 19 of June 2013 12:23:52 Tomasz Figa wrote:
> > On Wednesday 19 of June 2013 11:43:56 Thierry Reding wrote:
> > > On Wed, Jun 19, 2013 at 12:45:02AM +0200, Tomasz Figa wrote:
> > > > On Wednesday 19 of June 2013 00:17:06 Thierry Reding wrote:
> > > > > On Mon, Jun 17, 2013 at 10:50:40PM +0200, Tomasz Figa wrote:
> > > > > > On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> > > > > > > On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
> > > > > [...]
> > > > > 
> > > > > > > > +#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
> > > > > > > > +static DEFINE_SPINLOCK(samsung_pwm_lock);
> > > > > > > > +#endif
> > > > > > > 
> > > > > > > Why is this lock global? Shouldn't it more correctly be part
> > > > > > > of
> > > > > > > samsung_pwm_chip?
> > > > > > 
> > > > > > There are few registers shared with samsung_pwm_timer
> > > > > > clocksource
> > > > > > driver and so normally the spinlock is exported from it. However
> > > > > > on
> > > > > > on some platforms (namely Exynos >=4x12) kernel can be compiled
> > > > > > without that driver, so the lock must be defined locally, just
> > > > > > to
> > > > > > synchronize multiple PWM channels, as they share registers as
> > > > > > well.
> > > > > 
> > > > > Okay, I think this needs further explanation. The clocksource
> > > > > driver
> > > > > is
> > > > > used for what exactly? From a quick look it seems to be very much
> > > > > PWM-
> > > > > specific. According to the device tree binding for the PWM driver,
> > > > > the
> > > > > timer blocks can also be used as clock sources and clock event
> > > > > timers.
> > > > > So if I understand correctly you have setups where you use one or
> > > > > more
> > > > > channels as clock source or clock event timer and one or more
> > > > > channels
> > > > > as PWM outputs.
> > > > > 
> > > > > In that case it's a very bad idea to use a global lock to
> > > > > synchronize
> > > > > accesses. You need to do much more than that. To properly split
> > > > > this
> > > > > across several drivers there needs to be a mechanism to allocate
> > > > > channels for use either as clock source/event timer or PWM.
> > > > > Otherwise,
> > > > > how do you know that drivers aren't stepping on each other's toes?
> > > > > 
> > > > > > > > +		ret = pwm_samsung_parse_dt(chip);
> > > > > > > > +		if (ret)
> > > > > > > > +			return ret;
> > > > > > > > +
> > > > > > > > +		chip->chip.of_xlate = of_pwm_xlate_with_flags;
> > > > > > > > +		chip->chip.of_pwm_n_cells = 3;
> > > > > > > > +	} else {
> > > > > > > > +		if (!pdev->dev.platform_data) {
> > > > > > > > +			dev_err(&pdev->dev, "no platform data
> > > > > > 
> > > > > > specified\n");
> > > > > > 
> > > > > > > > +			return -EINVAL;
> > > > > > > > +		}
> > > > > > > > +
> > > > > > > > +		memcpy(&chip->variant, pdev->dev.platform_data,
> > > > > > > > +
> > > > 
> > > > sizeof(chip-
> > > > 
> > > > > > >variant));
> > > > > > >
> > > > > > > > +	}
> > > > > > > 
> > > > > > > Obviously this needs some modification in order for the
> > > > > > > variant
> > > > > > > to
> > > > > > > become constant. But I think you can easily do so by making
> > > > > > > the
> > > > > > > driver
> > > > > > > match using the platform_driver's id_table field, similar to
> > > > > > > how
> > > > > > > the
> > > > > > > matching is done for OF.
> > > > > > 
> > > > > > Generally output_mask is board-dependent and is passed inside a
> > > > > > variant
> > > > > > struct using platform_data pointer.
> > > > > 
> > > > > That's okay. But output_mask is the only thing that's
> > > > > board-dependent.
> > > > > Everything else in the variant is SoC dependent judging by the OF
> > > > > device
> > > > > table. So really only the output_mask should be part of the
> > > > > platform
> > > > > data.
> > > > > 
> > > > > > Same platform data is used in samsung_pwm_timer clocksource
> > > > > > driver,
> > > > > > so
> > > > > > I just reused it here without adding the need to rename platform
> > > > > > device at runtime (see arch/arm/plat-samsung/devs.c).
> > > > > 
> > > > > Looking a bit at git log for the clocksource driver, there's this
> > > > > 
> > > > > commit:
> > > > > 	a3ce54f clocksource: samsung_pwm_timer: Do not request PWM 
> mem
> > > > 
> > > > region
> > > > 
> > > > > That's an ugly workaround for sharing registers between two
> > > > > drivers.
> > > > > There's a reason why drivers do request_mem_region(), and it is
> > > > > precisely to prevent them from accessing the same registers. As I
> > > > > already said above, I think you need to come up with some sort of
> > > > > API
> > > > > to
> > > > > share resources between the drivers.
> > > > > 
> > > > > There was a similar issue a few months back with the pwm-tiehrpwm
> > > > > and
> > > > > pwm-tiecap drivers, which use a shared block of registers.
> > > > > Initially
> > > > > something similar was done as you do here, but eventually we came
> > > > > up
> > > > > with a much better solution that involved introducing a new driver
> > > > > for
> > > > > the shared functionality and an exported API.
> > > > > 
> > > > > The situation seems to be somewhat different here since you
> > > > > actually
> > > > > share the same resources for different functionality instead of
> > > > > sharing
> > > > > one subset of register across multiple drivers, but I think a
> > > > > similar
> > > > > solution can be applied here.
> > > > 
> > > > Well, current design, or rather lack of thereof, has been
> > > > established
> > > > after a significant amount of discussion, mostly with ARM SoC and
> > > > MFD
> > > > maintainers.
> > > > 
> > > > It started from a simple reorganization of the clocksource driver to
> > > > be
> > > > multiplatform friendly, without any means of synchronizing both
> > > > drivers
> > > > other than local_irq_save() and _restore() that used to be there
> > > > originally, before I started my work on this. This was enough to
> > > > work
> > > > correctly, because both drivers at the same time are used only on
> > > > uniprocessor systems.
> > > > 
> > > > Here's the thread with patches:
> > > > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16664/focus=1
> > > > 726
> > > > 7
> > > > 
> > > > Then I opened a Pandora's Box by asking for opinion in this thread:
> > > > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16480/focus=1
> > > > 650
> > > > 0
> > > > 
> > > > After that I started working on a complex framework for sharing this
> > > > IP
> > > > block that would solve all the problems with synchronization,
> > > > channel
> > > > allocation, device tree parsing, variant management, etc.
> > > > 
> > > > Here's an initial version, which started as an MFD master driver,
> > > > which
> > > > would let 2 client drivers use the hardware (only the clocksource
> > > > part
> > > > was implemented in that series):
> > > > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17464/focus=2
> > > > 292
> > > > 50
> > > > 
> > > > Still, that version didn't receive too good feedback, with comments
> > > > pointing me to change the architecture to master-slave, with the
> > > > clocksource driver being the master and PWM driver being the slave.
> > > > And
> > > > so v5 was made:
> > > > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17864
> > > 
> > > Yeah, this seemed to be going in the right direction.
> > > 
> > > > This version was generally accepted, but then we discussed on IRC
> > > > (mostly me and Arnd) whether such complex infrastructure is really
> > > > needed and concluded that for this platform a simple shared spinlock
> > > > would be enough, based on following reasons:
> > > > - on all SoCs on which the clocksource driver is going to be used
> > > > there
> > > > is only one instance of the PWM block, with 5 channels
> > > 
> > > You say that now. But then at some point somebody decides that it
> > > might
> > > be useful to have a second instance.
> > 
> > No, he won't.
> > 
> > Simply because no new SoC is going to be made which will use the
> > samsung_pwm_timer driver and all existing ones have only a single
> > instance of this IP.
> > 
> > > > - on all existing boards there are always at least two channels that
> > > > don't have outputs
> > > 
> > > I don't see how that's relevant here.
> > 
> > This is the information passed through output_mask. Clocksource driver
> > can use _only_ channels without output and PWM driver can use _only_
> > channels with output.
> > 
> > Both drivers receive the same output_mask value, either from the same
> > variant structure or from device tree.
> > 
> > > > - operations on particular PWM channels must be synchronized anyway,
> > > > because there are registers shared by all PWM channels (TCON being
> > > > most
> > > > important where enable, autoreload and invert bits are located for
> > > > all
> > > > channels).
> > > 
> > > And that's precisely why there should be a more explicit way of
> > > synchronizing than just acquiring a lock. Otherwise you have no
> > > possibility whatsoever to detect when a board tries to use both the
> > > clocksource and PWM drivers in incompatible ways. The lock may prevent
> > > concurrent accesses to the shared registers, but that won't help you
> > > if
> > > the PWM driver suddenly decides to use an output that was originally
> > > meant to be used as clock source.
> > 
> > See above.
> > 
> > > > And so we got to current design which is basically a shared spinlock
> > > > and
> > > > per-board output_mask, which specifies which channels have outputs
> > > > on
> > > > particular boards. If you look to the clocksource driver, you can
> > > > see
> > > > that it tries to allocate two output-less channels for timekeeping
> > > > purposes and if it fails to do so, it simply panics.
> > > > 
> > > > IMHO this solution is fine, because it's simple, has little overhead
> > > > and
> > > > it just works in case of platforms for which it is needed.
> > > 
> > > Okay, I like simple. But I see too much potential for this to break
> > > in the future. There are just too many assumptions for my taste. It
> > > isn't anything that I'm looking forward to have to maintain.
> > 
> > Those are completely safe assumptions for existing platforms. Any new
> > platform with the same PWM IP will use this driver exclusively, without
> > the clocksource driver and so without the problems you mentioned.
> 
> Would you mind taking into account things I pointed in reply above?

I took those into account. But quite frankly they're no excuse for not
having at least a half-decent design. Even if you could guarantee that
the same IP wasn't used in a different combination than you expect,
keeping what you have now sets a very bad example and people will copy
because they don't know better and I'll have to explain this all over
again.

Thierry

Tomasz Figa June 24, 2013, 8:03 p.m. UTC | #22

On Monday 24 of June 2013 21:50:21 Thierry Reding wrote:
> On Mon, Jun 24, 2013 at 07:52:11PM +0200, Tomasz Figa wrote:
> > Hi Thierry,
> > 
> > On Wednesday 19 of June 2013 12:23:52 Tomasz Figa wrote:
> > > On Wednesday 19 of June 2013 11:43:56 Thierry Reding wrote:
> > > > On Wed, Jun 19, 2013 at 12:45:02AM +0200, Tomasz Figa wrote:
> > > > > On Wednesday 19 of June 2013 00:17:06 Thierry Reding wrote:
> > > > > > On Mon, Jun 17, 2013 at 10:50:40PM +0200, Tomasz Figa wrote:
> > > > > > > On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> > > > > > > > On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa 
wrote:
> > > > > > [...]
> > > > > > 
> > > > > > > > > +#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
> > > > > > > > > +static DEFINE_SPINLOCK(samsung_pwm_lock);
> > > > > > > > > +#endif
> > > > > > > > 
> > > > > > > > Why is this lock global? Shouldn't it more correctly be
> > > > > > > > part
> > > > > > > > of
> > > > > > > > samsung_pwm_chip?
> > > > > > > 
> > > > > > > There are few registers shared with samsung_pwm_timer
> > > > > > > clocksource
> > > > > > > driver and so normally the spinlock is exported from it.
> > > > > > > However
> > > > > > > on
> > > > > > > on some platforms (namely Exynos >=4x12) kernel can be
> > > > > > > compiled
> > > > > > > without that driver, so the lock must be defined locally,
> > > > > > > just
> > > > > > > to
> > > > > > > synchronize multiple PWM channels, as they share registers
> > > > > > > as
> > > > > > > well.
> > > > > > 
> > > > > > Okay, I think this needs further explanation. The clocksource
> > > > > > driver
> > > > > > is
> > > > > > used for what exactly? From a quick look it seems to be very
> > > > > > much
> > > > > > PWM-
> > > > > > specific. According to the device tree binding for the PWM
> > > > > > driver,
> > > > > > the
> > > > > > timer blocks can also be used as clock sources and clock event
> > > > > > timers.
> > > > > > So if I understand correctly you have setups where you use one
> > > > > > or
> > > > > > more
> > > > > > channels as clock source or clock event timer and one or more
> > > > > > channels
> > > > > > as PWM outputs.
> > > > > > 
> > > > > > In that case it's a very bad idea to use a global lock to
> > > > > > synchronize
> > > > > > accesses. You need to do much more than that. To properly
> > > > > > split
> > > > > > this
> > > > > > across several drivers there needs to be a mechanism to
> > > > > > allocate
> > > > > > channels for use either as clock source/event timer or PWM.
> > > > > > Otherwise,
> > > > > > how do you know that drivers aren't stepping on each other's
> > > > > > toes?
> > > > > > 
> > > > > > > > > +		ret = pwm_samsung_parse_dt(chip);
> > > > > > > > > +		if (ret)
> > > > > > > > > +			return ret;
> > > > > > > > > +
> > > > > > > > > +		chip->chip.of_xlate = 
of_pwm_xlate_with_flags;
> > > > > > > > > +		chip->chip.of_pwm_n_cells = 3;
> > > > > > > > > +	} else {
> > > > > > > > > +		if (!pdev->dev.platform_data) {
> > > > > > > > > +			dev_err(&pdev->dev, "no platform 
data
> > > > > > > 
> > > > > > > specified\n");
> > > > > > > 
> > > > > > > > > +			return -EINVAL;
> > > > > > > > > +		}
> > > > > > > > > +
> > > > > > > > > +		memcpy(&chip->variant, pdev-
>dev.platform_data,
> > > > > > > > > +
> > > > > 
> > > > > sizeof(chip-
> > > > > 
> > > > > > > >variant));
> > > > > > > >
> > > > > > > > > +	}
> > > > > > > > 
> > > > > > > > Obviously this needs some modification in order for the
> > > > > > > > variant
> > > > > > > > to
> > > > > > > > become constant. But I think you can easily do so by
> > > > > > > > making
> > > > > > > > the
> > > > > > > > driver
> > > > > > > > match using the platform_driver's id_table field, similar
> > > > > > > > to
> > > > > > > > how
> > > > > > > > the
> > > > > > > > matching is done for OF.
> > > > > > > 
> > > > > > > Generally output_mask is board-dependent and is passed
> > > > > > > inside a
> > > > > > > variant
> > > > > > > struct using platform_data pointer.
> > > > > > 
> > > > > > That's okay. But output_mask is the only thing that's
> > > > > > board-dependent.
> > > > > > Everything else in the variant is SoC dependent judging by the
> > > > > > OF
> > > > > > device
> > > > > > table. So really only the output_mask should be part of the
> > > > > > platform
> > > > > > data.
> > > > > > 
> > > > > > > Same platform data is used in samsung_pwm_timer clocksource
> > > > > > > driver,
> > > > > > > so
> > > > > > > I just reused it here without adding the need to rename
> > > > > > > platform
> > > > > > > device at runtime (see arch/arm/plat-samsung/devs.c).
> > > > > > 
> > > > > > Looking a bit at git log for the clocksource driver, there's
> > > > > > this
> > > > > > 
> > > > > > commit:
> > > > > > 	a3ce54f clocksource: samsung_pwm_timer: Do not request PWM
> > 
> > mem
> > 
> > > > > region
> > > > > 
> > > > > > That's an ugly workaround for sharing registers between two
> > > > > > drivers.
> > > > > > There's a reason why drivers do request_mem_region(), and it
> > > > > > is
> > > > > > precisely to prevent them from accessing the same registers.
> > > > > > As I
> > > > > > already said above, I think you need to come up with some sort
> > > > > > of
> > > > > > API
> > > > > > to
> > > > > > share resources between the drivers.
> > > > > > 
> > > > > > There was a similar issue a few months back with the
> > > > > > pwm-tiehrpwm
> > > > > > and
> > > > > > pwm-tiecap drivers, which use a shared block of registers.
> > > > > > Initially
> > > > > > something similar was done as you do here, but eventually we
> > > > > > came
> > > > > > up
> > > > > > with a much better solution that involved introducing a new
> > > > > > driver
> > > > > > for
> > > > > > the shared functionality and an exported API.
> > > > > > 
> > > > > > The situation seems to be somewhat different here since you
> > > > > > actually
> > > > > > share the same resources for different functionality instead
> > > > > > of
> > > > > > sharing
> > > > > > one subset of register across multiple drivers, but I think a
> > > > > > similar
> > > > > > solution can be applied here.
> > > > > 
> > > > > Well, current design, or rather lack of thereof, has been
> > > > > established
> > > > > after a significant amount of discussion, mostly with ARM SoC
> > > > > and
> > > > > MFD
> > > > > maintainers.
> > > > > 
> > > > > It started from a simple reorganization of the clocksource
> > > > > driver to
> > > > > be
> > > > > multiplatform friendly, without any means of synchronizing both
> > > > > drivers
> > > > > other than local_irq_save() and _restore() that used to be there
> > > > > originally, before I started my work on this. This was enough to
> > > > > work
> > > > > correctly, because both drivers at the same time are used only
> > > > > on
> > > > > uniprocessor systems.
> > > > > 
> > > > > Here's the thread with patches:
> > > > > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16664/foc
> > > > > us=1
> > > > > 726
> > > > > 7
> > > > > 
> > > > > Then I opened a Pandora's Box by asking for opinion in this
> > > > > thread:
> > > > > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16480/foc
> > > > > us=1
> > > > > 650
> > > > > 0
> > > > > 
> > > > > After that I started working on a complex framework for sharing
> > > > > this
> > > > > IP
> > > > > block that would solve all the problems with synchronization,
> > > > > channel
> > > > > allocation, device tree parsing, variant management, etc.
> > > > > 
> > > > > Here's an initial version, which started as an MFD master
> > > > > driver,
> > > > > which
> > > > > would let 2 client drivers use the hardware (only the
> > > > > clocksource
> > > > > part
> > > > > was implemented in that series):
> > > > > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17464/foc
> > > > > us=2
> > > > > 292
> > > > > 50
> > > > > 
> > > > > Still, that version didn't receive too good feedback, with
> > > > > comments
> > > > > pointing me to change the architecture to master-slave, with the
> > > > > clocksource driver being the master and PWM driver being the
> > > > > slave.
> > > > > And
> > > > > so v5 was made:
> > > > > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17864
> > > > 
> > > > Yeah, this seemed to be going in the right direction.
> > > > 
> > > > > This version was generally accepted, but then we discussed on
> > > > > IRC
> > > > > (mostly me and Arnd) whether such complex infrastructure is
> > > > > really
> > > > > needed and concluded that for this platform a simple shared
> > > > > spinlock
> > > > > would be enough, based on following reasons:
> > > > > - on all SoCs on which the clocksource driver is going to be
> > > > > used
> > > > > there
> > > > > is only one instance of the PWM block, with 5 channels
> > > > 
> > > > You say that now. But then at some point somebody decides that it
> > > > might
> > > > be useful to have a second instance.
> > > 
> > > No, he won't.
> > > 
> > > Simply because no new SoC is going to be made which will use the
> > > samsung_pwm_timer driver and all existing ones have only a single
> > > instance of this IP.
> > > 
> > > > > - on all existing boards there are always at least two channels
> > > > > that
> > > > > don't have outputs
> > > > 
> > > > I don't see how that's relevant here.
> > > 
> > > This is the information passed through output_mask. Clocksource
> > > driver
> > > can use _only_ channels without output and PWM driver can use _only_
> > > channels with output.
> > > 
> > > Both drivers receive the same output_mask value, either from the
> > > same
> > > variant structure or from device tree.
> > > 
> > > > > - operations on particular PWM channels must be synchronized
> > > > > anyway,
> > > > > because there are registers shared by all PWM channels (TCON
> > > > > being
> > > > > most
> > > > > important where enable, autoreload and invert bits are located
> > > > > for
> > > > > all
> > > > > channels).
> > > > 
> > > > And that's precisely why there should be a more explicit way of
> > > > synchronizing than just acquiring a lock. Otherwise you have no
> > > > possibility whatsoever to detect when a board tries to use both
> > > > the
> > > > clocksource and PWM drivers in incompatible ways. The lock may
> > > > prevent
> > > > concurrent accesses to the shared registers, but that won't help
> > > > you
> > > > if
> > > > the PWM driver suddenly decides to use an output that was
> > > > originally
> > > > meant to be used as clock source.
> > > 
> > > See above.
> > > 
> > > > > And so we got to current design which is basically a shared
> > > > > spinlock
> > > > > and
> > > > > per-board output_mask, which specifies which channels have
> > > > > outputs
> > > > > on
> > > > > particular boards. If you look to the clocksource driver, you
> > > > > can
> > > > > see
> > > > > that it tries to allocate two output-less channels for
> > > > > timekeeping
> > > > > purposes and if it fails to do so, it simply panics.
> > > > > 
> > > > > IMHO this solution is fine, because it's simple, has little
> > > > > overhead
> > > > > and
> > > > > it just works in case of platforms for which it is needed.
> > > > 
> > > > Okay, I like simple. But I see too much potential for this to
> > > > break
> > > > in the future. There are just too many assumptions for my taste.
> > > > It
> > > > isn't anything that I'm looking forward to have to maintain.
> > > 
> > > Those are completely safe assumptions for existing platforms. Any
> > > new
> > > platform with the same PWM IP will use this driver exclusively,
> > > without
> > > the clocksource driver and so without the problems you mentioned.
> > 
> > Would you mind taking into account things I pointed in reply above?
> 
> I took those into account. But quite frankly they're no excuse for not
> having at least a half-decent design. Even if you could guarantee that
> the same IP wasn't used in a different combination than you expect,
> keeping what you have now sets a very bad example and people will copy
> because they don't know better and I'll have to explain this all over
> again.

OK. So we just need to prevent people from blindly copying this.

Wouldn't adding a big comment about why this is enough for this platform 
and why anything more sophisticated would be just overengineering in this 
case be enough?

As you could see in several versions of the series I linked in my previous 
replies, anything that looked decently simply added a lot of glue code and 
cross module dependencies without serving any useful purpose, other than 
just looking good.

This driver is already a lot better than previous one, because as opposed 
to the old one, it gives synchronization that is technically correct. Not 
even saying about a lot of other things fixed, like multiplatform-
awareness, OF support, coding style, proper handling of dividers, etc., 
etc. It would be really bad if all this was put to waste...

Best regards,
Tomasz

Thierry Reding June 24, 2013, 8:28 p.m. UTC | #23

On Mon, Jun 24, 2013 at 10:03:12PM +0200, Tomasz Figa wrote:
[...]
> OK. So we just need to prevent people from blindly copying this.
> 
> Wouldn't adding a big comment about why this is enough for this platform 
> and why anything more sophisticated would be just overengineering in this 
> case be enough?

That'd be sugarcoating. I don't think this is a good idea at all and
using a properly encapsulated driver with proper shared API to access
shared registers wouldn't be overengineering in my opinion. It would
in fact be good engineering.

> This driver is already a lot better than previous one, because as opposed 
> to the old one, it gives synchronization that is technically correct. Not 
> even saying about a lot of other things fixed, like multiplatform-
> awareness, OF support, coding style, proper handling of dividers, etc., 
> etc. It would be really bad if all this was put to waste...

I certainly wouldn't want any of this going to waste, and quite frankly
letting you get away with just the comment is already more compromise
than I really like.

The TI drivers used to have a similar problem and I required them to
come up with a good solution. It'd be fair to require the same of you.
But maybe I'm getting soft.

Thierry

Arnd Bergmann June 24, 2013, 8:55 p.m. UTC | #24

On Monday 24 June 2013, Tomasz Figa wrote:
> Wouldn't adding a big comment about why this is enough for this platform 
> and why anything more sophisticated would be just overengineering in this 
> case be enough?
> 
> As you could see in several versions of the series I linked in my previous 
> replies, anything that looked decently simply added a lot of glue code and 
> cross module dependencies without serving any useful purpose, other than 
> just looking good.
> 
> This driver is already a lot better than previous one, because as opposed 
> to the old one, it gives synchronization that is technically correct. Not 
> even saying about a lot of other things fixed, like multiplatform-
> awareness, OF support, coding style, proper handling of dividers, etc., 
> etc. It would be really bad if all this was put to waste...

I agree. The shared spinlock as the interface between the two drivers
is clearly a hack, but I think it is an appropriate hack in this special
case and that's why I suggested using that over the more complex solutions
that Tomasz suggested.

The important part is that the DT binding reasonable and could support
more complex scenarios even though they are highly unlikely.
We can always change the kernel code to something else if we need to,
but I could not come up with good *and* simple design. The most obvious
solution would be to make the clocksource driver the master that exports
an interface to the pwm driver, but that just adds complexity for the
common case that just uses the pwm driver but not the clocksource driver.

The attempt to create a separate MFD glue driver that abstracts the
interface in the way that pwm-tiecap does also was a failure here, because
one of the two consumers has to run very early, before we have access to
most of the device infrastructure the kernel provides.

	Arnd

Patch

diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 229a599..833c3ac 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -9,6 +9,7 @@  obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung-legacy.o
+obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
new file mode 100644
index 0000000..61bed3d
--- /dev/null
+++ b/drivers/pwm/pwm-samsung.c
@@ -0,0 +1,528 @@ 
+/* drivers/pwm/pwm-samsung.c
+ *
+ * Copyright (c) 2007 Ben Dooks
+ * Copyright (c) 2008 Simtec Electronics
+ *     Ben Dooks <[e-mail]>, <[e-mail]>
+ * Copyright (c) 2013 Tomasz Figa <[e-mail]>
+ *
+ * PWM driver for Samsung SoCs
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+*/
+
+#include <linux/clk.h>
+#include <linux/export.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/time.h>
+
+#include <clocksource/samsung_pwm.h>
+
+#define REG_TCFG0			0x00
+#define REG_TCFG1			0x04
+#define REG_TCON			0x08
+
+#define REG_TCNTB(tmr)			(0x0c + ((tmr) * 0xc))
+#define REG_TCMPB(tmr)			(0x10 + ((tmr) * 0xc))
+
+#define TCFG0_PRESCALER_MASK		0xff
+#define TCFG0_PRESCALER1_SHIFT		8
+
+#define TCFG1_MUX_MASK			0xf
+#define TCFG1_SHIFT(x)			((x) * 4)
+
+#define TCON_START(chan)		(1 << (4 * (chan) + 0))
+#define TCON_MANUALUPDATE(chan)		(1 << (4 * (chan) + 1))
+#define TCON_INVERT(chan)		(1 << (4 * (chan) + 2))
+#define TCON_AUTORELOAD(chan)		(1 << (4 * (chan) + 3))
+
+struct samsung_pwm_channel {
+	unsigned long period_ns;
+	unsigned long duty_ns;
+	unsigned long tin_ns;
+};
+
+struct samsung_pwm_chip {
+	struct pwm_chip chip;
+	struct samsung_pwm_variant variant;
+	struct samsung_pwm_channel channels[SAMSUNG_PWM_NUM];
+
+	void __iomem *base;
+	struct clk *base_clk;
+	struct clk *tclk0;
+	struct clk *tclk1;
+};
+#define to_samsung_pwm_chip(chip)	\
+			container_of(chip, struct samsung_pwm_chip, chip)
+
+#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
+static DEFINE_SPINLOCK(samsung_pwm_lock);
+#endif
+
+static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
+					unsigned int channel, u8 divisor)
+{
+	u8 shift = TCFG1_SHIFT(channel);
+	unsigned long flags;
+	u32 reg;
+	u8 bits;
+
+	bits = (fls(divisor) - 1) - pwm->variant.div_base;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	reg = readl(pwm->base + REG_TCFG1);
+	reg &= ~(TCFG1_MUX_MASK << shift);
+	reg |= bits << shift;
+	writel(reg, pwm->base + REG_TCFG1);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+}
+
+static inline int pwm_samsung_is_tdiv(struct samsung_pwm_chip *chip,
+							unsigned int chan)
+{
+	struct samsung_pwm_variant *variant = &chip->variant;
+	u32 reg;
+
+	reg = readl(chip->base + REG_TCFG1);
+	reg >>= TCFG1_SHIFT(chan);
+	reg &= TCFG1_MUX_MASK;
+
+	return ((1 << reg) & variant->tclk_mask) == 0;
+}
+
+static unsigned long pwm_samsung_get_tin_rate(struct samsung_pwm_chip *chip,
+							unsigned int chan)
+{
+	unsigned long rate;
+	u32 reg;
+
+	rate = clk_get_rate(chip->base_clk);
+
+	reg = readl(chip->base + REG_TCFG0);
+	if (chan >= 2)
+		reg >>= TCFG0_PRESCALER1_SHIFT;
+	reg &= TCFG0_PRESCALER_MASK;
+
+	return rate / (reg + 1);
+}
+
+static unsigned long pwm_samsung_calc_tin(struct samsung_pwm_chip *chip,
+					unsigned int chan, unsigned long freq)
+{
+	struct samsung_pwm_variant *variant = &chip->variant;
+	unsigned long rate;
+	unsigned int div;
+	struct clk *clk;
+
+	if (!pwm_samsung_is_tdiv(chip, chan)) {
+		clk = (chan < 2) ? chip->tclk0 : chip->tclk1;
+		if (!IS_ERR(clk)) {
+			rate = clk_get_rate(clk);
+			if (rate)
+				return rate;
+		}
+
+		dev_warn(chip->chip.dev,
+			"tclk of PWM %d is inoperational, using tdiv\n", chan);
+	}
+
+	rate = pwm_samsung_get_tin_rate(chip, chan);
+	dev_dbg(chip->chip.dev, "tin parent at %lu\n", rate);
+
+	/*
+	 * Compare minimum PWM frequency that can be achieved with possible
+	 * divider settings and choose the lowest divisor that can generate
+	 * frequencies lower than requested.
+	 */
+	for (div = variant->div_base; div < 4; ++div)
+		if ((rate >> (variant->bits + div)) < freq)
+			break;
+
+	pwm_samsung_set_divisor(chip, chan, 1 << div);
+
+	return rate >> div;
+}
+
+static int pwm_samsung_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+
+	if (our_chip->variant.output_mask & (1 << pwm->hwpwm))
+		return 0;
+
+	dev_warn(our_chip->chip.dev,
+			"tried to request PWM channel %d without output\n",
+			pwm->hwpwm);
+
+	return -EINVAL;
+}
+
+static int pwm_samsung_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	unsigned int channel = pwm->hwpwm;
+	unsigned long tcon;
+	unsigned long flags;
+
+	if (channel > 0)
+		++channel;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	tcon = __raw_readl(our_chip->base + REG_TCON);
+
+	tcon &= ~TCON_MANUALUPDATE(channel);
+	tcon |= TCON_START(channel) | TCON_AUTORELOAD(channel);
+
+	__raw_writel(tcon, our_chip->base + REG_TCON);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+
+	return 0;
+}
+
+static void pwm_samsung_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	unsigned int channel = pwm->hwpwm;
+	unsigned long tcon;
+	unsigned long flags;
+
+	if (channel > 0)
+		++channel;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	tcon = __raw_readl(our_chip->base + REG_TCON);
+	tcon &= ~TCON_AUTORELOAD(channel);
+	__raw_writel(tcon, our_chip->base + REG_TCON);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+}
+
+static int pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
+		int duty_ns, int period_ns)
+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	struct samsung_pwm_channel *chan = &our_chip->channels[pwm->hwpwm];
+	unsigned long tin_ns = chan->tin_ns;
+	unsigned int tcon_chan = pwm->hwpwm;
+	unsigned long tin_rate;
+	unsigned long period;
+	unsigned long flags;
+	unsigned long tcnt;
+	long tcmp;
+	u32 tcon;
+
+	/* We currently avoid using 64bit arithmetic by using the
+	 * fact that anything faster than 1Hz is easily representable
+	 * by 32bits. */
+	if (period_ns > NSEC_PER_SEC || duty_ns > NSEC_PER_SEC)
+		return -ERANGE;
+
+	if (period_ns == chan->period_ns && duty_ns == chan->duty_ns)
+		return 0;
+
+	/* The TCMP and TCNT can be read without a lock, they're not
+	 * shared between the timers. */
+	tcmp = readl(our_chip->base + REG_TCMPB(pwm->hwpwm));
+	tcnt = readl(our_chip->base + REG_TCNTB(pwm->hwpwm));
+
+	period = NSEC_PER_SEC / period_ns;
+
+	dev_dbg(our_chip->chip.dev, "duty_ns=%d, period_ns=%d (%lu)\n",
+		duty_ns, period_ns, period);
+
+	/* Check to see if we are changing the clock rate of the PWM */
+	if (chan->period_ns != period_ns) {
+		tin_rate = pwm_samsung_calc_tin(our_chip, pwm->hwpwm, period);
+
+		chan->period_ns = period_ns;
+
+		dev_dbg(our_chip->chip.dev, "tin_rate=%lu\n", tin_rate);
+
+		tin_ns = NSEC_PER_SEC / tin_rate;
+		tcnt = period_ns / tin_ns;
+
+		chan->tin_ns = tin_ns;
+	}
+
+	/* Note, counters count down */
+	tcmp = duty_ns / tin_ns;
+	tcmp = tcnt - tcmp;
+
+	/* the pwm hw only checks the compare register after a decrement,
+	   so the pin never toggles if tcmp = tcnt */
+	if (tcmp == tcnt)
+		tcmp--;
+
+	dev_dbg(our_chip->chip.dev, "tin_ns=%lu, tcmp=%ld/%lu\n",
+							tin_ns, tcmp, tcnt);
+
+	if (tcmp < 0)
+		tcmp = 0;
+
+	/* Update the PWM register block. */
+	if (tcon_chan > 0)
+		++tcon_chan;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	tcon = __raw_readl(our_chip->base + REG_TCON);
+
+	tcnt--;
+
+	tcon &= ~(TCON_START(tcon_chan) | TCON_AUTORELOAD(tcon_chan));
+	tcon |= TCON_MANUALUPDATE(tcon_chan);
+
+	__raw_writel(tcnt, our_chip->base + REG_TCNTB(pwm->hwpwm));
+	__raw_writel(tcmp, our_chip->base + REG_TCMPB(pwm->hwpwm));
+	__raw_writel(tcon, our_chip->base + REG_TCON);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+
+	return 0;
+}
+
+static int pwm_samsung_set_polarity(struct pwm_chip *chip,
+			struct pwm_device *pwm, enum pwm_polarity polarity)
+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	unsigned int channel = pwm->hwpwm;
+	unsigned long flags;
+	u32 tcon;
+
+	if (channel > 0)
+		++channel;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	tcon = __raw_readl(our_chip->base + REG_TCON);
+
+	/* Invert in hardware means normal polarity of PWM core */
+	if (polarity == PWM_POLARITY_NORMAL)
+		tcon |= TCON_INVERT(channel);
+	else
+		tcon &= ~TCON_INVERT(channel);
+
+	__raw_writel(tcon, our_chip->base + REG_TCON);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+
+	return 0;
+}
+
+static struct pwm_ops pwm_samsung_ops = {
+	.request	= pwm_samsung_request,
+	.enable		= pwm_samsung_enable,
+	.disable	= pwm_samsung_disable,
+	.config		= pwm_samsung_config,
+	.set_polarity	= pwm_samsung_set_polarity,
+	.owner		= THIS_MODULE,
+};
+
+#ifdef CONFIG_OF
+static const struct samsung_pwm_variant s3c24xx_variant = {
+	.bits		= 16,
+	.div_base	= 1,
+	.has_tint_cstat	= false,
+	.tclk_mask	= (1 << 4),
+};
+
+static const struct samsung_pwm_variant s3c64xx_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+	.tclk_mask	= (1 << 7) | (1 << 6) | (1 << 5),
+};
+
+static const struct samsung_pwm_variant s5p64x0_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+	.tclk_mask	= 0,
+};
+
+static const struct samsung_pwm_variant s5p_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+	.tclk_mask	= (1 << 5),
+};
+
+static const struct of_device_id samsung_pwm_matches[] = {
+	{ .compatible = "samsung,s3c2410-pwm", .data = &s3c24xx_variant },
+	{ .compatible = "samsung,s3c6400-pwm", .data = &s3c64xx_variant },
+	{ .compatible = "samsung,s5p6440-pwm", .data = &s5p64x0_variant },
+	{ .compatible = "samsung,s5pc100-pwm", .data = &s5p_variant },
+	{ .compatible = "samsung,exynos4210-pwm", .data = &s5p64x0_variant },
+	{},
+};
+#endif
+
+static int pwm_samsung_parse_dt(struct samsung_pwm_chip *chip)
+{
+	struct device_node *np = chip->chip.dev->of_node;
+	const struct of_device_id *match;
+	struct property *prop;
+	const __be32 *cur;
+	u32 val;
+
+	match = of_match_node(samsung_pwm_matches, np);
+	if (!match)
+		return -ENODEV;
+
+	memcpy(&chip->variant, match->data, sizeof(chip->variant));
+
+	of_property_for_each_u32(np, "samsung,pwm-outputs", prop, cur, val) {
+		if (val >= SAMSUNG_PWM_NUM) {
+			pr_warning("%s: invalid channel index in samsung,pwm-outputs property\n",
+								__func__);
+			continue;
+		}
+		chip->variant.output_mask |= 1 << val;
+	}
+
+	return 0;
+}
+
+static int pwm_samsung_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct samsung_pwm_chip *chip;
+	struct resource *res;
+	int ret;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL) {
+		dev_err(dev, "failed to allocate driver data\n");
+		return -ENOMEM;
+	}
+
+	chip->chip.dev = &pdev->dev;
+	chip->chip.ops = &pwm_samsung_ops;
+	chip->chip.base = -1;
+	chip->chip.npwm = SAMSUNG_PWM_NUM;
+
+	if (pdev->dev.of_node) {
+		ret = pwm_samsung_parse_dt(chip);
+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = of_pwm_xlate_with_flags;
+		chip->chip.of_pwm_n_cells = 3;
+	} else {
+		if (!pdev->dev.platform_data) {
+			dev_err(&pdev->dev, "no platform data specified\n");
+			return -EINVAL;
+		}
+
+		memcpy(&chip->variant, pdev->dev.platform_data,
+							sizeof(chip->variant));
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to get mem resource\n");
+		return -ENOMEM;
+	}
+
+	chip->base = devm_request_and_ioremap(&pdev->dev, res);
+	if (!chip->base) {
+		dev_err(&pdev->dev, "failed to request and map registers\n");
+		return -ENOMEM;
+	}
+
+	chip->base_clk = devm_clk_get(&pdev->dev, "timers");
+	if (IS_ERR(chip->base_clk)) {
+		dev_err(dev, "failed to get timer base clk\n");
+		return PTR_ERR(chip->base_clk);
+	}
+	clk_prepare_enable(chip->base_clk);
+
+	chip->tclk0 = devm_clk_get(&pdev->dev, "pwm-tclk0");
+	chip->tclk1 = devm_clk_get(&pdev->dev, "pwm-tclk1");
+
+	ret = pwmchip_add(&chip->chip);
+	if (ret < 0) {
+		dev_err(dev, "failed to register pwm\n");
+		goto err_clk_disable;
+	}
+
+	dev_info(dev, "base_clk at %lu, tclk0 at %lu, tclk1 at %lu\n",
+		clk_get_rate(chip->base_clk),
+		!IS_ERR(chip->tclk0) ? clk_get_rate(chip->tclk0) : 0,
+		!IS_ERR(chip->tclk1) ? clk_get_rate(chip->tclk1) : 0);
+
+	platform_set_drvdata(pdev, chip);
+
+	return 0;
+
+err_clk_disable:
+	clk_disable_unprepare(chip->base_clk);
+
+	return ret;
+}
+
+static int pwm_samsung_remove(struct platform_device *pdev)
+{
+	struct samsung_pwm_chip *chip = platform_get_drvdata(pdev);
+	int err;
+
+	err = pwmchip_remove(&chip->chip);
+	if (err < 0)
+		return err;
+
+	clk_disable_unprepare(chip->base_clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int pwm_samsung_suspend(struct device *dev)
+{
+	struct samsung_pwm_chip *chip = dev_get_drvdata(dev);
+	int i;
+
+	/* No one preserve these values during suspend so reset them
+	 * Otherwise driver leaves PWM unconfigured if same values
+	 * passed to pwm_config
+	 */
+	for (i = 0; i < SAMSUNG_PWM_NUM; ++i) {
+		chip->channels[i].period_ns = 0;
+		chip->channels[i].duty_ns = 0;
+	}
+
+	return 0;
+}
+#endif
+
+static struct dev_pm_ops pwm_samsung_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pwm_samsung_suspend, NULL)
+};
+
+static struct platform_driver pwm_samsung_driver = {
+	.driver		= {
+		.name	= "samsung-pwm",
+		.owner	= THIS_MODULE,
+		.pm	= &pwm_samsung_pm_ops,
+		.of_match_table = of_match_ptr(samsung_pwm_matches),
+	},
+	.probe		= pwm_samsung_probe,
+	.remove		= pwm_samsung_remove,
+};
+module_platform_driver(pwm_samsung_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Tomasz Figa <[e-mail]>");
+MODULE_ALIAS("platform:samsung-pwm");