[PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 6872 bytes --]

Supporting the second channel in the driver.
Offset is 0x8 and both channnels share the same
IRQ.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 drivers/gpio/gpio-xilinx.c | 93 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 81 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index 9ae7aa8..385dcb0 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -1,7 +1,7 @@
 /*
- * Xilinx gpio driver
+ * Xilinx gpio driver for xps/axi_gpio IP.
  *
- * Copyright 2008 Xilinx, Inc.
+ * Copyright 2008 - 2013 Xilinx, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2
@@ -26,10 +26,17 @@
 #define XGPIO_DATA_OFFSET   (0x0)	/* Data register  */
 #define XGPIO_TRI_OFFSET    (0x4)	/* I/O direction register  */

+#define XGPIO_CHANNEL_OFFSET	0x8
+
+/* Read/Write access to the GPIO registers */
+#define xgpio_readreg(offset)		__raw_readl(offset)
+#define xgpio_writereg(offset, val)	__raw_writel(val, offset)
+
 struct xgpio_instance {
 	struct of_mm_gpio_chip mmchip;
 	u32 gpio_state;		/* GPIO state shadow register */
 	u32 gpio_dir;		/* GPIO direction shadow register */
+	u32 offset;
 	spinlock_t gpio_lock;	/* Lock used for synchronization */
 };

@@ -44,8 +51,12 @@ struct xgpio_instance {
 static int xgpio_get(struct gpio_chip *gc, unsigned int gpio)
 {
 	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct xgpio_instance *chip =
+	    container_of(mm_gc, struct xgpio_instance, mmchip);
+
+	void __iomem *regs = mm_gc->regs + chip->offset;

-	return (in_be32(mm_gc->regs + XGPIO_DATA_OFFSET) >> gpio) & 1;
+	return (xgpio_readreg(regs + XGPIO_DATA_OFFSET) >> gpio) & 1;
 }

 /**
@@ -63,6 +74,7 @@ static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
 	struct xgpio_instance *chip =
 	    container_of(mm_gc, struct xgpio_instance, mmchip);
+	void __iomem *regs = mm_gc->regs;

 	spin_lock_irqsave(&chip->gpio_lock, flags);

@@ -71,7 +83,9 @@ static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 		chip->gpio_state |= 1 << gpio;
 	else
 		chip->gpio_state &= ~(1 << gpio);
-	out_be32(mm_gc->regs + XGPIO_DATA_OFFSET, chip->gpio_state);
+
+	xgpio_writereg(regs + chip->offset + XGPIO_DATA_OFFSET,
+							 chip->gpio_state);

 	spin_unlock_irqrestore(&chip->gpio_lock, flags);
 }
@@ -91,12 +105,13 @@ static int xgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
 	struct xgpio_instance *chip =
 	    container_of(mm_gc, struct xgpio_instance, mmchip);
+	void __iomem *regs = mm_gc->regs;

 	spin_lock_irqsave(&chip->gpio_lock, flags);

 	/* Set the GPIO bit in shadow register and set direction as input */
 	chip->gpio_dir |= (1 << gpio);
-	out_be32(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir);
+	xgpio_writereg(regs + chip->offset + XGPIO_TRI_OFFSET, chip->gpio_dir);

 	spin_unlock_irqrestore(&chip->gpio_lock, flags);

@@ -119,6 +134,7 @@ static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
 	struct xgpio_instance *chip =
 	    container_of(mm_gc, struct xgpio_instance, mmchip);
+	void __iomem *regs = mm_gc->regs;

 	spin_lock_irqsave(&chip->gpio_lock, flags);

@@ -127,11 +143,12 @@ static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 		chip->gpio_state |= 1 << gpio;
 	else
 		chip->gpio_state &= ~(1 << gpio);
-	out_be32(mm_gc->regs + XGPIO_DATA_OFFSET, chip->gpio_state);
+	xgpio_writereg(regs + chip->offset + XGPIO_DATA_OFFSET,
+		       chip->gpio_state);

 	/* Clear the GPIO bit in shadow register and set direction as output */
 	chip->gpio_dir &= (~(1 << gpio));
-	out_be32(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir);
+	xgpio_writereg(regs + chip->offset + XGPIO_TRI_OFFSET, chip->gpio_dir);

 	spin_unlock_irqrestore(&chip->gpio_lock, flags);

@@ -147,8 +164,10 @@ static void xgpio_save_regs(struct of_mm_gpio_chip *mm_gc)
 	struct xgpio_instance *chip =
 	    container_of(mm_gc, struct xgpio_instance, mmchip);

-	out_be32(mm_gc->regs + XGPIO_DATA_OFFSET, chip->gpio_state);
-	out_be32(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir);
+	xgpio_writereg(mm_gc->regs + chip->offset + XGPIO_DATA_OFFSET,
+							chip->gpio_state);
+	xgpio_writereg(mm_gc->regs + chip->offset + XGPIO_TRI_OFFSET,
+							 chip->gpio_dir);
 }

 /**
@@ -183,9 +202,6 @@ static int xgpio_of_probe(struct device_node *np)
 	/* Check device node and parent device node for device width */
 	chip->mmchip.gc.ngpio = 32; /* By default assume full GPIO controller */
 	tree_info = of_get_property(np, "xlnx,gpio-width", NULL);
-	if (!tree_info)
-		tree_info = of_get_property(np->parent,
-					    "xlnx,gpio-width", NULL);
 	if (tree_info)
 		chip->mmchip.gc.ngpio = be32_to_cpup(tree_info);

@@ -206,6 +222,59 @@ static int xgpio_of_probe(struct device_node *np)
 		       np->full_name, status);
 		return status;
 	}
+
+	pr_info("XGpio: %s: registered, base is %d\n", np->full_name,
+							chip->mmchip.gc.base);
+
+	tree_info = of_get_property(np, "xlnx,is-dual", NULL);
+	if (tree_info && be32_to_cpup(tree_info)) {
+		chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+		if (!chip)
+			return -ENOMEM;
+
+		/* Add dual channel offset */
+		chip->offset = XGPIO_CHANNEL_OFFSET;
+
+		/* Update GPIO state shadow register with default value */
+		tree_info = of_get_property(np, "xlnx,dout-default-2", NULL);
+		if (tree_info)
+			chip->gpio_state = be32_to_cpup(tree_info);
+
+		/* Update GPIO direction shadow register with default value */
+		/* By default, all pins are inputs */
+		chip->gpio_dir = 0xFFFFFFFF;
+		tree_info = of_get_property(np, "xlnx,tri-default-2", NULL);
+		if (tree_info)
+			chip->gpio_dir = be32_to_cpup(tree_info);
+
+		/* Check device node and parent device node for device width */
+		/* By default assume full GPIO controller */
+		chip->mmchip.gc.ngpio = 32;
+		tree_info = of_get_property(np, "xlnx,gpio2-width", NULL);
+		if (tree_info)
+			chip->mmchip.gc.ngpio = be32_to_cpup(tree_info);
+
+		spin_lock_init(&chip->gpio_lock);
+
+		chip->mmchip.gc.direction_input = xgpio_dir_in;
+		chip->mmchip.gc.direction_output = xgpio_dir_out;
+		chip->mmchip.gc.get = xgpio_get;
+		chip->mmchip.gc.set = xgpio_set;
+
+		chip->mmchip.save_regs = xgpio_save_regs;
+
+		/* Call the OF gpio helper to setup and register the GPIO dev */
+		status = of_mm_gpiochip_add(np, &chip->mmchip);
+		if (status) {
+			kfree(chip);
+			pr_err("%s: error in probe function with status %d\n",
+			np->full_name, status);
+			return status;
+		}
+		pr_info("XGpio: %s: dual channel registered, base is %d\n",
+					np->full_name, chip->mmchip.gc.base);
+	}
+
 	return 0;
 }

--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 2/2] DT: Add documentation for gpio-xilinx

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2154 bytes --]

Describe gpio-xilinx binding.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 .../devicetree/bindings/gpio/gpio-xilinx.txt       | 43 ++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xilinx.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-xilinx.txt b/Documentation/devicetree/bindings/gpio/gpio-xilinx.txt
new file mode 100644
index 0000000..65bf386
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-xilinx.txt
@@ -0,0 +1,43 @@
+Xilinx plb/axi GPIO controller
+
+Required properties:
+- compatible : Should be "xlnx,xps-gpio-1.00.a"
+- reg : Address and length of the register set for the device
+- #gpio-cells : Should be two. The first cell is the pin number and the
+  second cell is used to specify optional parameters (currently unused).
+- gpio-controller : Marks the device node as a GPIO controller.
+
+Optional properties:
+- interrupts : Interrupt mapping for GPIO IRQ.
+- interrupt-parent : Phandle for the interrupt controller that
+   services interrupts for this device.
+- xlnx,all-inputs : if n-th bit is setup, GPIO-n is input
+- xlnx,dout-default : if n-th bit is 1, GPIO-n default value is 1
+- xlnx,gpio-width : gpio width
+- xlnx,tri-default : if n-th bit is 1, GPIO-n is in tristate mode
+- xlnx,is-dual : if 1, controller also uses the second channel
+- xlnx,all-inputs-2 : as above but for the second channel
+- xlnx,dout-default-2 : as above but the second channel
+- xlnx,gpio2-width : as above but for the second channel
+- xlnx,tri-default-2 : as above but for the second channel
+
+
+Example:
+gpio: gpio@40000000 {
+	#gpio-cells = <2>;
+	compatible = "xlnx,xps-gpio-1.00.a";
+	gpio-controller ;
+	interrupt-parent = <&microblaze_0_intc>;
+	interrupts = < 6 2 >;
+	reg = < 0x40000000 0x10000 >;
+	xlnx,all-inputs = <0x0>;
+	xlnx,all-inputs-2 = <0x0>;
+	xlnx,dout-default = <0x0>;
+	xlnx,dout-default-2 = <0x0>;
+	xlnx,gpio-width = <0x2>;
+	xlnx,gpio2-width = <0x2>;
+	xlnx,interrupt-present = <0x1>;
+	xlnx,is-dual = <0x1>;
+	xlnx,tri-default = <0xffffffff>;
+	xlnx,tri-default-2 = <0xffffffff>;
+} ;
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Linus Walleij]

On Wed, May 29, 2013 at 1:27 PM, Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> wrote:

> Supporting the second channel in the driver.
> Offset is 0x8 and both channnels share the same
> IRQ.
>
> Signed-off-by: Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>

(...)
> +/* Read/Write access to the GPIO registers */
> +#define xgpio_readreg(offset)          __raw_readl(offset)
> +#define xgpio_writereg(offset, val)    __raw_writel(val, offset)

So you're swithing in_be32/out_be32 to the CPU-dependent
__raw_readl/__raw_writel functions? Why?

Can you explain exactly why you are using __raw_* accessors
rather than e.g. atleast readl_relaxed()/writel_relaxed()
or even plain readl/writel so you know the writes will hit
the hardware as immediately as possible?

I'd prefer this step to be a separate patch.

>  struct xgpio_instance {
>         struct of_mm_gpio_chip mmchip;
>         u32 gpio_state;         /* GPIO state shadow register */
>         u32 gpio_dir;           /* GPIO direction shadow register */
> +       u32 offset;
>         spinlock_t gpio_lock;   /* Lock used for synchronization */
>  };

Why not take this opportunity to move the comments out to
kerneldoc above this struct, plus document what "offset"
means.

> -       return (in_be32(mm_gc->regs + XGPIO_DATA_OFFSET) >> gpio) & 1;
> +       return (xgpio_readreg(regs + XGPIO_DATA_OFFSET) >> gpio) & 1;


Another way would be:

#include <linux/bitops.h>

return !!(xgpio_readreg(regs + XGPIO_DATA_OFFSET & BIT(gpio));

> +
> +       pr_info("XGpio: %s: registered, base is %d\n", np->full_name,
> +                                                       chip->mmchip.gc.base);
> +
> +       tree_info = of_get_property(np, "xlnx,is-dual", NULL);

This looks like you want to use of_property_read_bool().

Have you documented these new bindings? It doesn't seem so.
Documentation/devicetree/bindings/gpio/*...

If it's undocumented so far, this is a good oppotunity to add it.

> +       if (tree_info && be32_to_cpup(tree_info)) {
> +               chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +               if (!chip)
> +                       return -ENOMEM;
> +
> +               /* Add dual channel offset */
> +               chip->offset = XGPIO_CHANNEL_OFFSET;
> +
> +               /* Update GPIO state shadow register with default value */
> +               tree_info = of_get_property(np, "xlnx,dout-default-2", NULL);
> +               if (tree_info)
> +                       chip->gpio_state = be32_to_cpup(tree_info);

This is basically a jam table (hardware set-up) in the device tree.

I don't exactly like this. Is this necessary?

> +               /* Update GPIO direction shadow register with default value */
> +               /* By default, all pins are inputs */
> +               chip->gpio_dir = 0xFFFFFFFF;
> +               tree_info = of_get_property(np, "xlnx,tri-default-2", NULL);
> +               if (tree_info)
> +                       chip->gpio_dir = be32_to_cpup(tree_info);

Dito.

> +               /* Check device node and parent device node for device width */
> +               /* By default assume full GPIO controller */
> +               chip->mmchip.gc.ngpio = 32;
> +               tree_info = of_get_property(np, "xlnx,gpio2-width", NULL);
> +               if (tree_info)
> +                       chip->mmchip.gc.ngpio = be32_to_cpup(tree_info);

Seems fine, but document it in the binding.

Yours,
Linus Walleij

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Linus Walleij]

On Wed, May 29, 2013 at 1:27 PM, Michal Simek <michal.simek@xilinx.com> wrote:

> Supporting the second channel in the driver.
> Offset is 0x8 and both channnels share the same
> IRQ.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

(...)
> +/* Read/Write access to the GPIO registers */
> +#define xgpio_readreg(offset)          __raw_readl(offset)
> +#define xgpio_writereg(offset, val)    __raw_writel(val, offset)

So you're swithing in_be32/out_be32 to the CPU-dependent
__raw_readl/__raw_writel functions? Why?

Can you explain exactly why you are using __raw_* accessors
rather than e.g. atleast readl_relaxed()/writel_relaxed()
or even plain readl/writel so you know the writes will hit
the hardware as immediately as possible?

I'd prefer this step to be a separate patch.

>  struct xgpio_instance {
>         struct of_mm_gpio_chip mmchip;
>         u32 gpio_state;         /* GPIO state shadow register */
>         u32 gpio_dir;           /* GPIO direction shadow register */
> +       u32 offset;
>         spinlock_t gpio_lock;   /* Lock used for synchronization */
>  };

Why not take this opportunity to move the comments out to
kerneldoc above this struct, plus document what "offset"
means.

> -       return (in_be32(mm_gc->regs + XGPIO_DATA_OFFSET) >> gpio) & 1;
> +       return (xgpio_readreg(regs + XGPIO_DATA_OFFSET) >> gpio) & 1;


Another way would be:

#include <linux/bitops.h>

return !!(xgpio_readreg(regs + XGPIO_DATA_OFFSET & BIT(gpio));

> +
> +       pr_info("XGpio: %s: registered, base is %d\n", np->full_name,
> +                                                       chip->mmchip.gc.base);
> +
> +       tree_info = of_get_property(np, "xlnx,is-dual", NULL);

This looks like you want to use of_property_read_bool().

Have you documented these new bindings? It doesn't seem so.
Documentation/devicetree/bindings/gpio/*...

If it's undocumented so far, this is a good oppotunity to add it.

> +       if (tree_info && be32_to_cpup(tree_info)) {
> +               chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +               if (!chip)
> +                       return -ENOMEM;
> +
> +               /* Add dual channel offset */
> +               chip->offset = XGPIO_CHANNEL_OFFSET;
> +
> +               /* Update GPIO state shadow register with default value */
> +               tree_info = of_get_property(np, "xlnx,dout-default-2", NULL);
> +               if (tree_info)
> +                       chip->gpio_state = be32_to_cpup(tree_info);

This is basically a jam table (hardware set-up) in the device tree.

I don't exactly like this. Is this necessary?

> +               /* Update GPIO direction shadow register with default value */
> +               /* By default, all pins are inputs */
> +               chip->gpio_dir = 0xFFFFFFFF;
> +               tree_info = of_get_property(np, "xlnx,tri-default-2", NULL);
> +               if (tree_info)
> +                       chip->gpio_dir = be32_to_cpup(tree_info);

Dito.

> +               /* Check device node and parent device node for device width */
> +               /* By default assume full GPIO controller */
> +               chip->mmchip.gc.ngpio = 32;
> +               tree_info = of_get_property(np, "xlnx,gpio2-width", NULL);
> +               if (tree_info)
> +                       chip->mmchip.gc.ngpio = be32_to_cpup(tree_info);

Seems fine, but document it in the binding.

Yours,
Linus Walleij

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 5591 bytes --]

Hi Linus,

On 05/30/2013 09:46 PM, Linus Walleij wrote:
> On Wed, May 29, 2013 at 1:27 PM, Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> wrote:
> 
>> Supporting the second channel in the driver.
>> Offset is 0x8 and both channnels share the same
>> IRQ.
>>
>> Signed-off-by: Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> 
> (...)
>> +/* Read/Write access to the GPIO registers */
>> +#define xgpio_readreg(offset)          __raw_readl(offset)
>> +#define xgpio_writereg(offset, val)    __raw_writel(val, offset)
> 
> So you're swithing in_be32/out_be32 to the CPU-dependent
> __raw_readl/__raw_writel functions? Why?

The reason is that this driver can be used on ARM where in_be32/out_be32
is not implemented.

> Can you explain exactly why you are using __raw_* accessors
> rather than e.g. atleast readl_relaxed()/writel_relaxed()
> or even plain readl/writel so you know the writes will hit
> the hardware as immediately as possible?

Using __raw* function ensure that it is working on all
cpus. Microblaze big/little endian, PPC big endian and ARM little endian.

The correct way how to implement this is based on my previous
discussion to detect endians directly on IP.

But for this gpio case without interrupt connected(it means without
interrupt logic) there are just 2 registers data and tristate
(http://www.xilinx.com/support/documentation/ip_documentation/axi_gpio/v1_01_b/ds744_axi_gpio.pdf)
and auto detection can't be done.

> I'd prefer this step to be a separate patch.

ok. Will do based on my discussion around xilinxfb.

>>  struct xgpio_instance {
>>         struct of_mm_gpio_chip mmchip;
>>         u32 gpio_state;         /* GPIO state shadow register */
>>         u32 gpio_dir;           /* GPIO direction shadow register */
>> +       u32 offset;
>>         spinlock_t gpio_lock;   /* Lock used for synchronization */
>>  };
> 
> Why not take this opportunity to move the comments out to
> kerneldoc above this struct, plus document what "offset"
> means.

Good point. Will fix.

> 
>> -       return (in_be32(mm_gc->regs + XGPIO_DATA_OFFSET) >> gpio) & 1;
>> +       return (xgpio_readreg(regs + XGPIO_DATA_OFFSET) >> gpio) & 1;
> 
> 
> Another way would be:
> 
> #include <linux/bitops.h>
> 
> return !!(xgpio_readreg(regs + XGPIO_DATA_OFFSET & BIT(gpio));
> 
>> +
>> +       pr_info("XGpio: %s: registered, base is %d\n", np->full_name,
>> +                                                       chip->mmchip.gc.base);
>> +
>> +       tree_info = of_get_property(np, "xlnx,is-dual", NULL);
> 
> This looks like you want to use of_property_read_bool().

Ah yeah.

> Have you documented these new bindings? It doesn't seem so.
> Documentation/devicetree/bindings/gpio/*...
> 
> If it's undocumented so far, this is a good oppotunity to add it.

Isn't it enough what it is in 2/2?
Or do you want to describe current binding in the first patch
and then extend it in this patch when dual channel is added?


>> +       if (tree_info && be32_to_cpup(tree_info)) {
>> +               chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>> +               if (!chip)
>> +                       return -ENOMEM;
>> +
>> +               /* Add dual channel offset */
>> +               chip->offset = XGPIO_CHANNEL_OFFSET;
>> +
>> +               /* Update GPIO state shadow register with default value */
>> +               tree_info = of_get_property(np, "xlnx,dout-default-2", NULL);
>> +               if (tree_info)
>> +                       chip->gpio_state = be32_to_cpup(tree_info);
> 
> This is basically a jam table (hardware set-up) in the device tree.

Not sure what you mean by that. Xilinx GPIO is soft IP which can be configured
to different configurations before bitstream is generated.
At the end you will get different setting/addresses setup for every pin
which is described by these xlnx,X descriptions.

> I don't exactly like this. Is this necessary?

If you mean names or values in there that all of them are autogenerated
from design tools and they are reflect IP hardware description and all
configuration options which you can have there.
It means that all these values give you exact hardware description.

Do I answer your question?


> 
>> +               /* Update GPIO direction shadow register with default value */
>> +               /* By default, all pins are inputs */
>> +               chip->gpio_dir = 0xFFFFFFFF;
>> +               tree_info = of_get_property(np, "xlnx,tri-default-2", NULL);
>> +               if (tree_info)
>> +                       chip->gpio_dir = be32_to_cpup(tree_info);
> 
> Dito.
> 
>> +               /* Check device node and parent device node for device width */
>> +               /* By default assume full GPIO controller */
>> +               chip->mmchip.gc.ngpio = 32;
>> +               tree_info = of_get_property(np, "xlnx,gpio2-width", NULL);
>> +               if (tree_info)
>> +                       chip->mmchip.gc.ngpio = be32_to_cpup(tree_info);
> 
> Seems fine, but document it in the binding.

I will look at new fdt function to shorten this code to look better.

Thanks for your review,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 5533 bytes --]

Hi Linus,

On 05/30/2013 09:46 PM, Linus Walleij wrote:
> On Wed, May 29, 2013 at 1:27 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> 
>> Supporting the second channel in the driver.
>> Offset is 0x8 and both channnels share the same
>> IRQ.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> 
> (...)
>> +/* Read/Write access to the GPIO registers */
>> +#define xgpio_readreg(offset)          __raw_readl(offset)
>> +#define xgpio_writereg(offset, val)    __raw_writel(val, offset)
> 
> So you're swithing in_be32/out_be32 to the CPU-dependent
> __raw_readl/__raw_writel functions? Why?

The reason is that this driver can be used on ARM where in_be32/out_be32
is not implemented.

> Can you explain exactly why you are using __raw_* accessors
> rather than e.g. atleast readl_relaxed()/writel_relaxed()
> or even plain readl/writel so you know the writes will hit
> the hardware as immediately as possible?

Using __raw* function ensure that it is working on all
cpus. Microblaze big/little endian, PPC big endian and ARM little endian.

The correct way how to implement this is based on my previous
discussion to detect endians directly on IP.

But for this gpio case without interrupt connected(it means without
interrupt logic) there are just 2 registers data and tristate
(http://www.xilinx.com/support/documentation/ip_documentation/axi_gpio/v1_01_b/ds744_axi_gpio.pdf)
and auto detection can't be done.

> I'd prefer this step to be a separate patch.

ok. Will do based on my discussion around xilinxfb.

>>  struct xgpio_instance {
>>         struct of_mm_gpio_chip mmchip;
>>         u32 gpio_state;         /* GPIO state shadow register */
>>         u32 gpio_dir;           /* GPIO direction shadow register */
>> +       u32 offset;
>>         spinlock_t gpio_lock;   /* Lock used for synchronization */
>>  };
> 
> Why not take this opportunity to move the comments out to
> kerneldoc above this struct, plus document what "offset"
> means.

Good point. Will fix.

> 
>> -       return (in_be32(mm_gc->regs + XGPIO_DATA_OFFSET) >> gpio) & 1;
>> +       return (xgpio_readreg(regs + XGPIO_DATA_OFFSET) >> gpio) & 1;
> 
> 
> Another way would be:
> 
> #include <linux/bitops.h>
> 
> return !!(xgpio_readreg(regs + XGPIO_DATA_OFFSET & BIT(gpio));
> 
>> +
>> +       pr_info("XGpio: %s: registered, base is %d\n", np->full_name,
>> +                                                       chip->mmchip.gc.base);
>> +
>> +       tree_info = of_get_property(np, "xlnx,is-dual", NULL);
> 
> This looks like you want to use of_property_read_bool().

Ah yeah.

> Have you documented these new bindings? It doesn't seem so.
> Documentation/devicetree/bindings/gpio/*...
> 
> If it's undocumented so far, this is a good oppotunity to add it.

Isn't it enough what it is in 2/2?
Or do you want to describe current binding in the first patch
and then extend it in this patch when dual channel is added?


>> +       if (tree_info && be32_to_cpup(tree_info)) {
>> +               chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>> +               if (!chip)
>> +                       return -ENOMEM;
>> +
>> +               /* Add dual channel offset */
>> +               chip->offset = XGPIO_CHANNEL_OFFSET;
>> +
>> +               /* Update GPIO state shadow register with default value */
>> +               tree_info = of_get_property(np, "xlnx,dout-default-2", NULL);
>> +               if (tree_info)
>> +                       chip->gpio_state = be32_to_cpup(tree_info);
> 
> This is basically a jam table (hardware set-up) in the device tree.

Not sure what you mean by that. Xilinx GPIO is soft IP which can be configured
to different configurations before bitstream is generated.
At the end you will get different setting/addresses setup for every pin
which is described by these xlnx,X descriptions.

> I don't exactly like this. Is this necessary?

If you mean names or values in there that all of them are autogenerated
from design tools and they are reflect IP hardware description and all
configuration options which you can have there.
It means that all these values give you exact hardware description.

Do I answer your question?


> 
>> +               /* Update GPIO direction shadow register with default value */
>> +               /* By default, all pins are inputs */
>> +               chip->gpio_dir = 0xFFFFFFFF;
>> +               tree_info = of_get_property(np, "xlnx,tri-default-2", NULL);
>> +               if (tree_info)
>> +                       chip->gpio_dir = be32_to_cpup(tree_info);
> 
> Dito.
> 
>> +               /* Check device node and parent device node for device width */
>> +               /* By default assume full GPIO controller */
>> +               chip->mmchip.gc.ngpio = 32;
>> +               tree_info = of_get_property(np, "xlnx,gpio2-width", NULL);
>> +               if (tree_info)
>> +                       chip->mmchip.gc.ngpio = be32_to_cpup(tree_info);
> 
> Seems fine, but document it in the binding.

I will look at new fdt function to shorten this code to look better.

Thanks for your review,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Linus Walleij]

On Fri, May 31, 2013 at 7:43 AM, Michal Simek <monstr@monstr.eu> wrote:
> On 05/30/2013 09:46 PM, Linus Walleij wrote:

>> (...)
>>> +/* Read/Write access to the GPIO registers */
>>> +#define xgpio_readreg(offset)          __raw_readl(offset)
>>> +#define xgpio_writereg(offset, val)    __raw_writel(val, offset)
>>
>> So you're swithing in_be32/out_be32 to the CPU-dependent
>> __raw_readl/__raw_writel functions? Why?
>
> The reason is that this driver can be used on ARM where in_be32/out_be32
> is not implemented.

OK I buy this (and the following explanation).

I think readl/writel is always in LE (PCI) endianness anyway, which
is likely not what you want. (I suspect the next point was about
that.)

>> Have you documented these new bindings? It doesn't seem so.
>> Documentation/devicetree/bindings/gpio/*...
>>
>> If it's undocumented so far, this is a good oppotunity to add it.
>
> Isn't it enough what it is in 2/2?

I didn't see 2/2, I guess I wasn't on CC...

Anyway I guess it's this:
http://marc.info/?l=linux-kernel&m=136982686732560&w=2

It's OK, but fix the boolean member so as to just needing to
be present:

xlnx,is-dual;

Rather than

xlnx,is-dual = <1>;

> Or do you want to describe current binding in the first patch
> and then extend it in this patch when dual channel is added?

Nah. 2/2 is fine.

>> This is basically a jam table (hardware set-up) in the device tree.
>
> Not sure what you mean by that. Xilinx GPIO is soft IP which can be configured
> to different configurations before bitstream is generated.
> At the end you will get different setting/addresses setup for every pin
> which is described by these xlnx,X descriptions.
>
>> I don't exactly like this. Is this necessary?
>
> If you mean names or values in there that all of them are autogenerated
> from design tools and they are reflect IP hardware description and all
> configuration options which you can have there.
> It means that all these values give you exact hardware description.
>
> Do I answer your question?

Yes, this is OK, I buy that explanation. I thought it was
something else.

I think the overall problem is that I do not understand what a
"channel" is in this context, and thus it is hard to understand the
patch as a whole. 2/2 could add some more verbose explanation
about the HW IP so I get comfortable and can understand the
whole hardware block...

Yours,
Linus Walleij

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 4345 bytes --]

On 05/31/2013 09:14 AM, Linus Walleij wrote:
> On Fri, May 31, 2013 at 7:43 AM, Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org> wrote:
>> On 05/30/2013 09:46 PM, Linus Walleij wrote:
> 
>>> (...)
>>>> +/* Read/Write access to the GPIO registers */
>>>> +#define xgpio_readreg(offset)          __raw_readl(offset)
>>>> +#define xgpio_writereg(offset, val)    __raw_writel(val, offset)
>>>
>>> So you're swithing in_be32/out_be32 to the CPU-dependent
>>> __raw_readl/__raw_writel functions? Why?
>>
>> The reason is that this driver can be used on ARM where in_be32/out_be32
>> is not implemented.
> 
> OK I buy this (and the following explanation).
> 
> I think readl/writel is always in LE (PCI) endianness anyway, which
> is likely not what you want. (I suspect the next point was about
> that.)

readl/writel yes it is all the time little endian
but __raw_readl/__raw_writel is just *(u32 *)ptr access
without any endian checking and barriers.

Probably the best way how to handle is to write
#ifdef ARCH_ZYNQ
# define xgpio_readreg(offset)          readl(offset)
# define xgpio_writereg(offset, val)    writel(val, offset)
#else
# define xgpio_readreg(offset)          __raw_readl(offset)
# define xgpio_writereg(offset, val)    __raw_writel(val, offset)
#endif

But still it is not correct in sense that I shouldn't pretend
that __raw_readl is ok to run on ppc and microblaze big endian.
But using another ifdef BIG_ENDIAN or ARCH don't improve it.

If there is one more register which I can use for autodetection,
it will be easy to choose but that's not this case.

>>> Have you documented these new bindings? It doesn't seem so.
>>> Documentation/devicetree/bindings/gpio/*...
>>>
>>> If it's undocumented so far, this is a good oppotunity to add it.
>>
>> Isn't it enough what it is in 2/2?
> 
> I didn't see 2/2, I guess I wasn't on CC...
> 
> Anyway I guess it's this:
> http://marc.info/?l=linux-kernel&m=136982686732560&w=2

Yes. it is. I am using patman and you are probably not listed
in MAINTAINERS for this folder to automatically add you.
Will add you manually.

> It's OK, but fix the boolean member so as to just needing to
> be present:
> 
> xlnx,is-dual;
> 
> Rather than
> 
> xlnx,is-dual = <1>;

Surely I can do it but it means to change our BSP and because
this IP is used on Microblaze from beginning this change
breaks all DTSes from past.
That's why I would prefer not to change logic here because
it just breaks all Microblaze DTSes which were generated
till this change (All of them contains xlnx,is-dual = <0>
if dual channel is not used).

I will definitely look at dt function in the whole driver
and use the

>> Or do you want to describe current binding in the first patch
>> and then extend it in this patch when dual channel is added?
> 
> Nah. 2/2 is fine.

ok.

>>> This is basically a jam table (hardware set-up) in the device tree.
>>
>> Not sure what you mean by that. Xilinx GPIO is soft IP which can be configured
>> to different configurations before bitstream is generated.
>> At the end you will get different setting/addresses setup for every pin
>> which is described by these xlnx,X descriptions.
>>
>>> I don't exactly like this. Is this necessary?
>>
>> If you mean names or values in there that all of them are autogenerated
>> from design tools and they are reflect IP hardware description and all
>> configuration options which you can have there.
>> It means that all these values give you exact hardware description.
>>
>> Do I answer your question?
> 
> Yes, this is OK, I buy that explanation. I thought it was
> something else.

ok

> I think the overall problem is that I do not understand what a
> "channel" is in this context, and thus it is hard to understand the
> patch as a whole. 2/2 could add some more verbose explanation
> about the HW IP so I get comfortable and can understand the
> whole hardware block...

ok. Good.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 4315 bytes --]

On 05/31/2013 09:14 AM, Linus Walleij wrote:
> On Fri, May 31, 2013 at 7:43 AM, Michal Simek <monstr@monstr.eu> wrote:
>> On 05/30/2013 09:46 PM, Linus Walleij wrote:
> 
>>> (...)
>>>> +/* Read/Write access to the GPIO registers */
>>>> +#define xgpio_readreg(offset)          __raw_readl(offset)
>>>> +#define xgpio_writereg(offset, val)    __raw_writel(val, offset)
>>>
>>> So you're swithing in_be32/out_be32 to the CPU-dependent
>>> __raw_readl/__raw_writel functions? Why?
>>
>> The reason is that this driver can be used on ARM where in_be32/out_be32
>> is not implemented.
> 
> OK I buy this (and the following explanation).
> 
> I think readl/writel is always in LE (PCI) endianness anyway, which
> is likely not what you want. (I suspect the next point was about
> that.)

readl/writel yes it is all the time little endian
but __raw_readl/__raw_writel is just *(u32 *)ptr access
without any endian checking and barriers.

Probably the best way how to handle is to write
#ifdef ARCH_ZYNQ
# define xgpio_readreg(offset)          readl(offset)
# define xgpio_writereg(offset, val)    writel(val, offset)
#else
# define xgpio_readreg(offset)          __raw_readl(offset)
# define xgpio_writereg(offset, val)    __raw_writel(val, offset)
#endif

But still it is not correct in sense that I shouldn't pretend
that __raw_readl is ok to run on ppc and microblaze big endian.
But using another ifdef BIG_ENDIAN or ARCH don't improve it.

If there is one more register which I can use for autodetection,
it will be easy to choose but that's not this case.

>>> Have you documented these new bindings? It doesn't seem so.
>>> Documentation/devicetree/bindings/gpio/*...
>>>
>>> If it's undocumented so far, this is a good oppotunity to add it.
>>
>> Isn't it enough what it is in 2/2?
> 
> I didn't see 2/2, I guess I wasn't on CC...
> 
> Anyway I guess it's this:
> http://marc.info/?l=linux-kernel&m=136982686732560&w=2

Yes. it is. I am using patman and you are probably not listed
in MAINTAINERS for this folder to automatically add you.
Will add you manually.

> It's OK, but fix the boolean member so as to just needing to
> be present:
> 
> xlnx,is-dual;
> 
> Rather than
> 
> xlnx,is-dual = <1>;

Surely I can do it but it means to change our BSP and because
this IP is used on Microblaze from beginning this change
breaks all DTSes from past.
That's why I would prefer not to change logic here because
it just breaks all Microblaze DTSes which were generated
till this change (All of them contains xlnx,is-dual = <0>
if dual channel is not used).

I will definitely look at dt function in the whole driver
and use the

>> Or do you want to describe current binding in the first patch
>> and then extend it in this patch when dual channel is added?
> 
> Nah. 2/2 is fine.

ok.

>>> This is basically a jam table (hardware set-up) in the device tree.
>>
>> Not sure what you mean by that. Xilinx GPIO is soft IP which can be configured
>> to different configurations before bitstream is generated.
>> At the end you will get different setting/addresses setup for every pin
>> which is described by these xlnx,X descriptions.
>>
>>> I don't exactly like this. Is this necessary?
>>
>> If you mean names or values in there that all of them are autogenerated
>> from design tools and they are reflect IP hardware description and all
>> configuration options which you can have there.
>> It means that all these values give you exact hardware description.
>>
>> Do I answer your question?
> 
> Yes, this is OK, I buy that explanation. I thought it was
> something else.

ok

> I think the overall problem is that I do not understand what a
> "channel" is in this context, and thus it is hard to understand the
> patch as a whole. 2/2 could add some more verbose explanation
> about the HW IP so I get comfortable and can understand the
> whole hardware block...

ok. Good.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Linus Walleij]

On Fri, May 31, 2013 at 9:34 AM, Michal Simek <monstr@monstr.eu> wrote:
> On 05/31/2013 09:14 AM, Linus Walleij wrote:

>> It's OK, but fix the boolean member so as to just needing to
>> be present:
>>
>> xlnx,is-dual;
>>
>> Rather than
>>
>> xlnx,is-dual = <1>;
>
> Surely I can do it but it means to change our BSP and because
> this IP is used on Microblaze from beginning this change
> breaks all DTSes from past.

I think of_property_read_bool() will accept
xlnx,is-dual = <1>; to mean the same as xlnx,is-dual;
try it.

Yours,
Linus Walleij

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

On 06/17/2013 07:29 AM, Linus Walleij wrote:
> On Fri, May 31, 2013 at 9:34 AM, Michal Simek <monstr@monstr.eu> wrote:
>> On 05/31/2013 09:14 AM, Linus Walleij wrote:
> 
>>> It's OK, but fix the boolean member so as to just needing to
>>> be present:
>>>
>>> xlnx,is-dual;
>>>
>>> Rather than
>>>
>>> xlnx,is-dual = <1>;
>>
>> Surely I can do it but it means to change our BSP and because
>> this IP is used on Microblaze from beginning this change
>> breaks all DTSes from past.
> 
> I think of_property_read_bool() will accept
> xlnx,is-dual = <1>; to mean the same as xlnx,is-dual;
> try it.

First of all sorry for delay.
You are right that of_property_read_bool()
also accept xlnx,is-dual = <1>;
but also accept and return 1 when xlnx,is-dual = <0>;
which is incorrect behaviour.

If of_prorety_read_bool return 0 for case when xlnx,is-dual = <0>
we can use it.
Maybe this will be good change for this function behaviour.
If there is also value then use it. 0 -> false, 1 -> true

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Linus Walleij]

On Thu, Jun 20, 2013 at 9:51 AM, Michal Simek <monstr@monstr.eu> wrote:
> On 06/17/2013 07:29 AM, Linus Walleij wrote:

>> I think of_property_read_bool() will accept
>> xlnx,is-dual = <1>; to mean the same as xlnx,is-dual;
>> try it.
>
> First of all sorry for delay.
> You are right that of_property_read_bool()
> also accept xlnx,is-dual = <1>;
> but also accept and return 1 when xlnx,is-dual = <0>;
> which is incorrect behaviour.

OK but that is a coding issue, not a DT bindings design issue.
Can't we think a bit outside the box?
What about something like this:

static bool is_dual (struct device_node *np)
{
    struct property *prop = of_find_property(np, "xlnx,is-dual", NULL);
    int ret;
    u32 val;

    if (!prop)
        return false;

    ret = of_property_read_u32(np, "xlnx,is-dual", &val);
    if (ret < 0)
        return true; /* node exists but has no cells */

    return !!val;
}

Yours,
Linus Walleij

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 2105 bytes --]

On 06/20/2013 11:23 AM, Linus Walleij wrote:
> On Thu, Jun 20, 2013 at 9:51 AM, Michal Simek <monstr-pSz03upnqPeHXe+LvDLADg@public.gmane.org> wrote:
>> On 06/17/2013 07:29 AM, Linus Walleij wrote:
> 
>>> I think of_property_read_bool() will accept
>>> xlnx,is-dual = <1>; to mean the same as xlnx,is-dual;
>>> try it.
>>
>> First of all sorry for delay.
>> You are right that of_property_read_bool()
>> also accept xlnx,is-dual = <1>;
>> but also accept and return 1 when xlnx,is-dual = <0>;
>> which is incorrect behaviour.
> 
> OK but that is a coding issue, not a DT bindings design issue.
> Can't we think a bit outside the box?

Before that fyi: I am working on supporing irq in this driver too.
Sure.

> What about something like this:
> 
> static bool is_dual (struct device_node *np)
> {
>     struct property *prop = of_find_property(np, "xlnx,is-dual", NULL);
>     int ret;
>     u32 val;
> 
>     if (!prop)
>         return false;
> 
>     ret = of_property_read_u32(np, "xlnx,is-dual", &val);
>     if (ret < 0)
>         return true; /* node exists but has no cells */
> 
>     return !!val;
> }

we can do it in this way but what I don't like on this is that IP
is design to support 2 channels right now.
It can happen that Xilinx decides to extend this for new channels.
Register map is prepared for it and there is enough space to do it.

And when this is done then is-dual (which is current name which
is used in hardware configuration from design tools) will contain
larger value >1.
I agree that is-dual is not the best name.

What about to do it differently?
Generate number of channel in the description.
And also do for() loop in the probe function to read values based
on this channel number.

Thanks,
Michal




-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2075 bytes --]

On 06/20/2013 11:23 AM, Linus Walleij wrote:
> On Thu, Jun 20, 2013 at 9:51 AM, Michal Simek <monstr@monstr.eu> wrote:
>> On 06/17/2013 07:29 AM, Linus Walleij wrote:
> 
>>> I think of_property_read_bool() will accept
>>> xlnx,is-dual = <1>; to mean the same as xlnx,is-dual;
>>> try it.
>>
>> First of all sorry for delay.
>> You are right that of_property_read_bool()
>> also accept xlnx,is-dual = <1>;
>> but also accept and return 1 when xlnx,is-dual = <0>;
>> which is incorrect behaviour.
> 
> OK but that is a coding issue, not a DT bindings design issue.
> Can't we think a bit outside the box?

Before that fyi: I am working on supporing irq in this driver too.
Sure.

> What about something like this:
> 
> static bool is_dual (struct device_node *np)
> {
>     struct property *prop = of_find_property(np, "xlnx,is-dual", NULL);
>     int ret;
>     u32 val;
> 
>     if (!prop)
>         return false;
> 
>     ret = of_property_read_u32(np, "xlnx,is-dual", &val);
>     if (ret < 0)
>         return true; /* node exists but has no cells */
> 
>     return !!val;
> }

we can do it in this way but what I don't like on this is that IP
is design to support 2 channels right now.
It can happen that Xilinx decides to extend this for new channels.
Register map is prepared for it and there is enough space to do it.

And when this is done then is-dual (which is current name which
is used in hardware configuration from design tools) will contain
larger value >1.
I agree that is-dual is not the best name.

What about to do it differently?
Generate number of channel in the description.
And also do for() loop in the probe function to read values based
on this channel number.

Thanks,
Michal




-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Linus Walleij]

On Thu, Jun 20, 2013 at 12:59 PM, Michal Simek <monstr@monstr.eu> wrote:
> On 06/20/2013 11:23 AM, Linus Walleij wrote:

>> What about something like this:
>>
>> static bool is_dual (struct device_node *np)
>> {
>>     struct property *prop = of_find_property(np, "xlnx,is-dual", NULL);
>>     int ret;
>>     u32 val;
>>
>>     if (!prop)
>>         return false;
>>
>>     ret = of_property_read_u32(np, "xlnx,is-dual", &val);
>>     if (ret < 0)
>>         return true; /* node exists but has no cells */
>>
>>     return !!val;
>> }
>
> we can do it in this way but what I don't like on this is that IP
> is design to support 2 channels right now.
> It can happen that Xilinx decides to extend this for new channels.
> Register map is prepared for it and there is enough space to do it.
>
> And when this is done then is-dual (which is current name which
> is used in hardware configuration from design tools) will contain
> larger value >1.
> I agree that is-dual is not the best name.

You don't say. It's about as smart as this:

#define NUMBER_TWO 4

Oh well, that's not your fault.

But seriously:

  xlnx,is-dual;

without an argument can still be taken to mean "2", and
you still need to inperpret the absence if this parameter
as "1" do you not?

> What about to do it differently?
> Generate number of channel in the description.
> And also do for() loop in the probe function to read values based
> on this channel number.

Sorry I can't translate this, can you send a patch?

Yours,
Linus Walleij

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 3293 bytes --]

On 06/20/2013 01:33 PM, Linus Walleij wrote:
> On Thu, Jun 20, 2013 at 12:59 PM, Michal Simek <monstr@monstr.eu> wrote:
>> On 06/20/2013 11:23 AM, Linus Walleij wrote:
> 
>>> What about something like this:
>>>
>>> static bool is_dual (struct device_node *np)
>>> {
>>>     struct property *prop = of_find_property(np, "xlnx,is-dual", NULL);
>>>     int ret;
>>>     u32 val;
>>>
>>>     if (!prop)
>>>         return false;
>>>
>>>     ret = of_property_read_u32(np, "xlnx,is-dual", &val);
>>>     if (ret < 0)
>>>         return true; /* node exists but has no cells */
>>>
>>>     return !!val;
>>> }
>>
>> we can do it in this way but what I don't like on this is that IP
>> is design to support 2 channels right now.
>> It can happen that Xilinx decides to extend this for new channels.
>> Register map is prepared for it and there is enough space to do it.
>>
>> And when this is done then is-dual (which is current name which
>> is used in hardware configuration from design tools) will contain
>> larger value >1.
>> I agree that is-dual is not the best name.
> 
> You don't say. It's about as smart as this:
> 
> #define NUMBER_TWO 4
> 
> Oh well, that's not your fault.
> 
> But seriously:
> 
>   xlnx,is-dual;
> 
> without an argument can still be taken to mean "2", and
> you still need to inperpret the absence if this parameter
> as "1" do you not?
>
>> What about to do it differently?
>> Generate number of channel in the description.
>> And also do for() loop in the probe function to read values based
>> on this channel number.
> 
> Sorry I can't translate this, can you send a patch?

Sure.

xlnx,is-dual is always present in the HW and in all DTSes and it
is generated for several years

Based on my experience with hardware guys what happen when they add
new channel is that they will use xlnx,is-dual = 2 for 3 channels,
xlnx,is-dual = 3 for 4 channels, etc. They won't care about names
but they are working like that.

It means for me is really problematic it try to work with this
value as boolean even that name is confusing.

That's why it is much easier for me to start to use new property
which will describe number of channels but this value is not
used in design tools. Let's say number-of-channels = 1 or 2
in DT binding which will describe number channels in this IP.
Is this acceptable for you?



If you look how that dual channel is supported you will see that it is
probe_first_channel
if there is second channel probe it too.

And code there is very similar.
That's why I am thinking about using the loop to remove that code
duplication.
Something like this in "psedo code"

for(i = 0; i < number-of-channel; i++) {
	char *str_def = "xlnx,dout-default"
	if(i)
		add -(i+1) suffix to str_def (-2 for example just to reflect how design tools generates it)
		
	of_property_read_u32(np, str_def, &chip->gpio_state);
...
	gpiochip_add()
}

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/2] GPIO: Add support for dual channel in gpio-xilinx.c

[Linus Walleij]

On Thu, Jun 20, 2013 at 2:12 PM, Michal Simek <monstr@monstr.eu> wrote:

> xlnx,is-dual is always present in the HW and in all DTSes and it
> is generated for several years
>
> Based on my experience with hardware guys what happen when they add
> new channel is that they will use xlnx,is-dual = 2 for 3 channels,
> xlnx,is-dual = 3 for 4 channels, etc. They won't care about names
> but they are working like that.
>
> It means for me is really problematic it try to work with this
> value as boolean even that name is confusing.

OK I will have to live with this.

The problem with reviewing DT bindings is that for me as a
subsystem maintainer I'm interacting with a quite complex
universe:

1. Bindings from people like the MIPS camp where some people
  obviously sat down in a committee meeting and tried to define
  a binding in advance, which is then deployed and we have to
  support. Sometimes a real nice piece of work such as the
  PCI hose mappings.

2. Bindings that we need to evolve as part of this community
  review process, where the judgement of a mapping's
  applicability and sanity is very much up to the subsystem
  maintainer. (This is the vast class of bindings.)

3. Bindings that John Doe from Idaho came up with in his
  basement and then deployed to the entire world, so that
  we cannot review or amend it but just have to support as
  they are because they are already live in numerous
  systems.

This would be a case of (3) whereas I'm mostly used to
a binding discussion of type (2) and that is why this gets
so locked up.

> That's why it is much easier for me to start to use new property
> which will describe number of channels but this value is not
> used in design tools. Let's say number-of-channels = 1 or 2
> in DT binding which will describe number channels in this IP.
> Is this acceptable for you?

This is much nicer and readable.

Yours,
Linus Walleij