Add drivers/pwm and drivers/pwm/st_pwm.c

This patch adds support for ST Microelectronics Pulse Width Modulator. This is currently used by ST's SPEAr platform and tested on the same.

This patch also adds drivers/pwm directory as suggested by Arnd Bergmann in following discussion:

http://comments.gmane.org/gmane.linux.ports.arm.kernel/118651

...

+/**
+ * struct pwm_device: struct representing pwm device/channel
+ *
+ * pwmd_id: id of pwm device
+ * pwm: pointer to parent pwm ip
+ * label: used for storing label passed in pwm_request
+ * offset: base address offset from parent pwm mmio_base
+ * busy: represents usage status of a pwm device
+ * lock: lock specific to a pwm device

+ * node: node for adding device to parent pwm's devices list
+ *
+ * Each pwm IP contains four independent pwm device/channels. Some or all of
+ * which may be present in our configuration.
+ */

drivers/pwm/st_pwm.c new file +9 -0

+struct pwm_device {
+	unsigned pwmd_id;
+	struct pwm *pwm;
+	const char *label;
+	unsigned offset;
+	unsigned busy;
+	spinlock_t lock;
+	struct list_head node;
+};

+ * devices: list of devices/childrens of pwm ip
+ * node: node for adding pwm to global list of all pwm ips
+ */

drivers/pwm/st_pwm.c new file +52 -0

+struct pwm {
+	unsigned id;
+	void __iomem *mmio_base;
+	struct clk *clk;
+	int clk_enabled;
+	struct platform_device *pdev;
+	spinlock_t lock;
+	struct list_head devices;
+	struct list_head node;
+};
+
+/*
+ * period_ns = 10^9 * (PRESCALE + 1) * PV / PWM_CLK_RATE
+ * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
+ *
+ * PV = (PWM_CLK_RATE * period_ns)/ (10^9 * (PRESCALE + 1))
+ * DC = (PWM_CLK_RATE * duty_ns)/ (10^9 * (PRESCALE + 1))
+ */
+int pwm_config(struct pwm_device *pwmd, int duty_ns, int period_ns)
+{
+	u64 val, div, clk_rate;
+	unsigned long prescale = MIN_PRESCALE, pv, dc;
+	int ret = 0;
+
+	if (!pwmd) {
+		pr_err("pwm: config - NULL pwm device pointer\n");
+		return -EFAULT;
+	}
+
+	if (period_ns == 0 || duty_ns > period_ns) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	/* TODO: Need to optimize this loop */
+	while (1) {
+		div = 1000000000;
+		div *= 1 + prescale;
+		clk_rate = clk_get_rate(pwmd->pwm->clk);
+		val = clk_rate * period_ns;
+		pv = div64_u64(val, div);
+		val = clk_rate * duty_ns;
+		dc = div64_u64(val, div);
+
+		if ((pv == 0) || (dc == 0)) {
+			ret = -EINVAL;
+			goto err;
+		}
+		if ((pv > MAX_PERIOD) || (dc > MAX_DUTY)) {
+			prescale++;
+			if (prescale > MAX_PRESCALE) {
+				ret = -EINVAL;

drivers/pwm/st_pwm.c new file +9 -0

+	/*
+	 * NOTE: the clock to PWM has to be enabled first
+	 * before writing to the registers
+	 */
+	spin_lock(&pwmd->pwm->lock);
+	ret = clk_enable(pwmd->pwm->clk);
+	if (ret) {
+		spin_unlock(&pwmd->pwm->lock);
+		goto err;

Quoted 1 line · On Tue, 31 May 2011 14:21:51 +0530 Viresh Kumar <[email protected]> wrote:

+ * lock: lock specific to a pwm device

More specificity here would be helpful. Precisely which data does the lock protect?

Quoted 1 line

+ * lock: lock specific to current pwm ip

Ditto.

Quoted 39 lines

drivers/pwm/st_pwm.c new file +39 -0

+int pwm_config(struct pwm_device *pwmd, int duty_ns, int period_ns)
+{
+	u64 val, div, clk_rate;
+	unsigned long prescale = MIN_PRESCALE, pv, dc;
+	int ret = 0;
+
+	if (!pwmd) {
+		pr_err("pwm: config - NULL pwm device pointer\n");
+		return -EFAULT;
+	}
+
+	if (period_ns == 0 || duty_ns > period_ns) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	/* TODO: Need to optimize this loop */
+	while (1) {
+		div = 1000000000;
+		div *= 1 + prescale;
+		clk_rate = clk_get_rate(pwmd->pwm->clk);
+		val = clk_rate * period_ns;
+		pv = div64_u64(val, div);
+		val = clk_rate * duty_ns;
+		dc = div64_u64(val, div);
+
+		if ((pv == 0) || (dc == 0)) {
+			ret = -EINVAL;
+			goto err;
+		}
+		if ((pv > MAX_PERIOD) || (dc > MAX_DUTY)) {
+			prescale++;
+			if (prescale > MAX_PRESCALE) {
+				ret = -EINVAL;
+				goto err;
+			}
+			continue;
+		}
+		if ((pv < MIN_PERIOD) || (dc < MIN_DUTY)) {

drivers/pwm/st_pwm.c line 147

Quoted 5 lines

drivers/pwm/st_pwm.c new file +5 -0

+			ret = -EINVAL;
+			goto err;
+		}
+		break;
+	}

gee, is this some sort of puzzle? So human-readable description of what this code is doing would be an improvement.

Quoted 10 lines

drivers/pwm/st_pwm.c new file +10 -0

+	spin_lock(&pwmd->pwm->lock);
+	ret = clk_enable(pwmd->pwm->clk);
+	if (ret) {
+		spin_unlock(&pwmd->pwm->lock);
+		goto err;
+	}
+
+	spin_lock(&pwmd->lock);
+	writel(prescale << PRESCALE_SHIFT, pwmd->pwm->mmio_base +
+			pwmd->offset + PWMCR);

drivers/pwm/st_pwm.c line 167

Quoted 5 lines

drivers/pwm/st_pwm.c new file +5 -0

+	writel(dc, pwmd->pwm->mmio_base + pwmd->offset + PWMDCR);
+	writel(pv, pwmd->pwm->mmio_base + pwmd->offset + PWMPCR);
+	spin_unlock(&pwmd->lock);
+	clk_disable(pwmd->pwm->clk);
+	spin_unlock(&pwmd->pwm->lock);

The nesting rules for these two locks seems sensible and obvious, but I guess documenting the rule wouldn't hurt.

Quoted 10 lines

drivers/pwm/st_pwm.c new file +7 -0

+	return 0;
+err:
+	dev_err(&pwmd->pwm->pdev->dev, "pwm config fail\n");
+	return ret;
+}
+EXPORT_SYMBOL(pwm_config);
+
 
...

Quoted 1 line

+static int __devinit st_pwm_probe(struct platform_device *pdev)

And here things get rather odd.

Most of this file is a generic, non-device specific PWM layer, exported to other modules. But then we get into driver bits which are specific to one paritular type of device. Confused - this is like putting the e100 driver inside net/ipv4/tcp.c?

Quoted 9 lines

Quoted 1 line

+static int __devinit st_pwm_probe(struct platform_device *pdev)

And here things get rather odd.

Most of this file is a generic, non-device specific PWM layer, exported to other modules. But then we get into driver bits which are specific to one paritular type of device. Confused - this is like putting the e100 driver inside net/ipv4/tcp.c?

Sorry but i couldn't get this one completely. :(
Driver is specific to pwm peripheral by ST. This driver can be used for SPEAr or may be other SoC or Devices, and is not at all dependent on SPEAr.

Quoted 10 lines · On Tuesday 07 June 2011 05:55:25 viresh kumar wrote:

Quoted 9 lines

Quoted 1 line

+static int __devinit st_pwm_probe(struct platform_device *pdev)

And here things get rather odd.

Most of this file is a generic, non-device specific PWM layer, exported to other modules. But then we get into driver bits which are specific to one paritular type of device. Confused - this is like putting the e100 driver inside net/ipv4/tcp.c?

Quoted 3 lines

Sorry but i couldn't get this one completely. :(
Driver is specific to pwm peripheral by ST. This driver can be used for SPEAr or may be other SoC or Devices, and is not at all dependent on SPEAr.

It was my suggestion to start drivers/pwm/ with this driver. We currently have a number of pwm drivers spread over the entire tree, all using slight the same interface header. They all look like this one, and are each used on one SOC, so you have to choose at compile-time which one to use.

There are two problems with this of course:
1. the drivers that export the same interface should be in one directory
2. there should be a common abstraction layer to avoid duplicate code and enable building a kernel with multiple PWM drivers builtin.

Moving this driver to drivers/pwm is the first step to address the problem 1, we will move the other drivers in the 3.1 or 3.2 timeframe.

There is independent by Sascha Hauer to work on the abstraction layer for all the drivers. Once that is in, we will change the individual drivers in drivers/pwm accordingly.

This patchset adds drivers/pwm directory as suggested by Arnd Bergmann in following discussion:

http://comments.gmane.org/gmane.linux.ports.arm.kernel/118651

and also adds support for ST Microelectronics Pulse Width Modulator. This is currently used by ST's SPEAr platform and tested on the same.

Changes since V1:
- Moved driver to drivers/pwm/ directory from drivers/misc
- arch/arm/configs/spear3xx_defconfig updated accordingly.

Changes since earlier patchset:
- Moved driver to drivers/misc/ directory.

It was earlier reviewed as part of following patchset:
"[PATCH V6 00/17] Adding devices support for all spear machines"

Viresh Kumar (3):
drivers/pwm st_pwm: Add support for ST's Pulse Width Modulator
SPEAr3xx: Add machine support for ST's PWM IP
SPEAr3xx: Update defconfig for ST's PWM

Quoted 18 lines · Viresh KUMAR

This patchset adds drivers/pwm directory as suggested by Arnd Bergmann in following discussion:

http://comments.gmane.org/gmane.linux.ports.arm.kernel/118651

and also adds support for ST Microelectronics Pulse Width Modulator. This is currently used by ST's SPEAr platform and tested on the same.

Changes since V1:
- Moved driver to drivers/pwm/ directory from drivers/misc
- arch/arm/configs/spear3xx_defconfig updated accordingly.

Changes since earlier patchset:
- Moved driver to drivers/misc/ directory.

It was earlier reviewed as part of following patchset:
"[PATCH V6 00/17] Adding devices support for all spear machines"

Quoted 4 lines

Viresh Kumar (3):
drivers/pwm st_pwm: Add support for ST's Pulse Width Modulator
SPEAr3xx: Add machine support for ST's PWM IP
SPEAr3xx: Update defconfig for ST's PWM

Will you push this patchset through your tree or somebody else will do?

Quoted 24 lines · viresh kumar

Quoted 22 lines · Viresh KUMAR

This patchset adds drivers/pwm directory as suggested by Arnd Bergmann in following discussion:

http://comments.gmane.org/gmane.linux.ports.arm.kernel/118651

and also adds support for ST Microelectronics Pulse Width Modulator. This is currently used by ST's SPEAr platform and tested on the same.

Changes since V1:
- Moved driver to drivers/pwm/ directory from drivers/misc
- arch/arm/configs/spear3xx_defconfig updated accordingly.

Changes since earlier patchset:
- Moved driver to drivers/misc/ directory.

It was earlier reviewed as part of following patchset:
"[PATCH V6 00/17] Adding devices support for all spear machines"

Viresh Kumar (3):
drivers/pwm st_pwm: Add support for ST's Pulse Width Modulator
SPEAr3xx: Add machine support for ST's PWM IP
SPEAr3xx: Update defconfig for ST's PWM

Quoted 1 line

Will you push this patchset through your tree or somebody else will do?

Good question. If Sascha is able to come up with the patch to introduce the generic layer, I think it would be good to have it go though him.

Quoted 3 lines

Good question. If Sascha is able to come up with the patch to introduce the generic layer, I think it would be good to have it go though him.

Just posted a rebased and documented series again. Let's see what happens. The time seems good to finally get this into the kernel.

Quoted 6 lines · Sascha Hauer

Quoted 3 lines

Good question. If Sascha is able to come up with the patch to introduce the generic layer, I think it would be good to have it go though him.

Just posted a rebased and documented series again. Let's see what happens. The time seems good to finally get this into the kernel.

I'd say let's go forward with your framework. The two patches currently because they both add Makefile/Kconfig files in drivers/pwm.

Maybe you can add Viresh's patch to your series, and wait for him to follow up with a patch to convert it?

I've tried to blindly do the conversion myself, but got stuck with the definition of the platform device that has multiple pwms in it. I wonder if it would be better to change the st-pwm platform interface to define one platform_device for each of them.

Quoted 1 line · Arnd Bergmann

On Tuesday 28 June 2011, Sascha Hauer wrote:

Definitely. From a quick look at the driver it seems the pwms all have a seperate register space.

I can add Viresh's patch to my series once we agree on the base patch. In the meantime maybe Viresh starts converting it ;)