* Arnd Bergmann wrote:

On Wednesday 22 February 2012, Thierry Reding wrote:

quoted

 #include <linux/module.h>
+#include <linux/of_pwm.h>
 #include <linux/pwm.h>

You should probably reorder the patches for bisectability, or move the
of_* related changes out of this patch into patch 3. At the point
where patch 2 is applied, linux/of_pwm.h does not exist yet.

You're right. The correct thing would be to move the include into patch 3.
I'll make sure to check for bisectability in the next series.

quoted

+/**
+ * pwmchip_find() - iterator for locating a specific pwm_chip
+ * @data: data to pass to match function
+ * @match: callback function to check pwm_chip
+ */
+struct pwm_chip *pwmchip_find(void *data, int (*match)(struct pwm_chip *chip,
+						       void *data))
+{
+	struct pwm_chip *ret = NULL;
+	struct pwm_chip *chip;
+
+	mutex_lock(&pwm_lock);
+
+	list_for_each_entry(chip, &pwm_chips, list) {
+		if (match(chip, data)) {
+			ret = chip;
+			break;
+		}
+	}
+
+	mutex_unlock(&pwm_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pwmchip_find);

Is this only used for the device tree functions? If so, I would recommend
making it less generic and always search for a device node.

It is currently only used to look up a struct pwm_chip for a given struct
device_node, yes. But I can see other uses for this. For instance this could
be useful if we ever want to provide an alternative way of requesting a PWM
on a per-chip basis.

quoted

+static int pwm_show(struct seq_file *s, void *unused)
+{
+	const char *prefix = "";
+	struct pwm_chip *chip;
+
+	list_for_each_entry(chip, &pwm_chips, list) {
+		struct device *dev = chip->dev;
+
+		seq_printf(s, "%s%s/%s, %d PWM devices\n", prefix,
+			   dev->bus ? dev->bus->name : "no-bus",
+			   dev_name(dev), chip->npwm);
+
+		if (chip->ops->dbg_show)
+			chip->ops->dbg_show(chip, s);
+		else
+			pwm_dbg_show(chip, s);
+
+		prefix = "\n";
+	}
+
+	return 0;
+}
+
+static int pwm_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pwm_show, NULL);
+}

When you have a seq_file with a (possibly long) list of entries, better
use seq_open instead of single_open and print each item in the
->next() callback function.

Yes, that should work better. I just noticed that pwm_show() is actually
missing mutex_lock() and mutex_unlock() to protect against chip removal. With
the iterator interface that should be easy to add.

I was again basically looking at gpiolib for inspiration (maybe I should stop
doing that :-). Maybe gpiolib should be reworked to use seq_file's iterator
interface as well? Just in case anybody else turns to gpiolib for
inspiration.

Thierry