Anton Vorontsov wrote:

I think this change will break a number of boards, because a lot of them do this:

	if ((np = of_find_node_by_name(NULL, "par_io")) != NULL) {
		par_io_init(np);

So if you're going to change the par_io nodes, you need to change the code as well.

A patch that changes the documentation should also change the code.  And if
you're code changes the device tree, it should also maintain compatibility for
older device trees.

+		#gpio-cells = <2>;
+		compatible = "fsl,mpc8360-qe-pario-bank",
+			     "fsl,mpc8323-qe-pario-bank";
+		reg = <0x1400 0x18>;
+		gpio-controller;
+	};
 
+	qe_pio_e: gpio-controller@1460 {
+		#gpio-cells = <2>;
+		compatible = "fsl,mpc8360-qe-pario-bank",
+			     "fsl,mpc8323-qe-pario-bank";
+		reg = <0x1460 0x18>;
+		gpio-controller;
+	};
 
    vi) Pin configuration nodes
 
+   NOTE: pin configuration nodes are obsolete. Usually, their existance
+         is an evidence of the firmware shortcomings. Such fixups are
+         better handled by the Linux board file, not the device tree.

You can't just delete the par_io documentation without updating the code and
planning for feature removal.  Almost all of the existing code out there for QE
boards expects a par_io node, and the device trees still have them.

+static int qe_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct qe_pio_regs __iomem *regs = mm_gc->regs;
+	u32 pin_mask = 1 << (QE_PIO_PINS - 1 - gpio);
+
+	return !!(in_be32(&regs->cpdata) & pin_mask);

Do we need to do "!!"?  I thought as long as the result was non-zero, it didn't
matter what the actual value is.  "!!" converts non-zero to 1.

+static int qe_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct qe_gpio_chip *qe_gc = to_qe_gpio_chip(mm_gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&qe_gc->lock, flags);
+
+	__par_io_config_pin(mm_gc->regs, gpio, 2, 0, 0, 0);

No magic numbers, please.

+void __init qe_add_gpiochips(void)
+{
+	int ret;
+	struct device_node *np;
+
+	for_each_compatible_node(np, NULL, "fsl,mpc8323-qe-pario-bank") {
+		struct qe_gpio_chip *qe_gc;
+		struct of_mm_gpio_chip *mm_gc;
+		struct of_gpio_chip *of_gc;
+		struct gpio_chip *gc;
+
+		qe_gc = kzalloc(sizeof(*qe_gc), GFP_KERNEL);
+		if (!qe_gc) {
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		spin_lock_init(&qe_gc->lock);
+
+		mm_gc = &qe_gc->mm_gc;
+		of_gc = &mm_gc->of_gc;
+		gc = &of_gc->gc;
+
+		mm_gc->save_regs = qe_gpio_save_regs;
+		of_gc->gpio_cells = 2;
+		gc->ngpio = QE_PIO_PINS;
+		gc->direction_input = qe_gpio_dir_in;
+		gc->direction_output = qe_gpio_dir_out;
+		gc->get = qe_gpio_get;
+		gc->set = qe_gpio_set;
+
+		ret = of_mm_gpiochip_add(np, mm_gc);
+		if (ret)
+			goto err;
+	}
+
+	return;
+err:
+	pr_err("%s: registration failed with status %d\n", np->full_name, ret);
+	of_node_put(np);

Memory leak here.  If of_mm_gpiochip_add() fails or if the 2nd call to kzalloc()
fails, the already-allocated qe_gc objects won't be released.



-- 
Timur Tabi
Linux kernel developer at Freescale