[PATCH v2 0/3] Add support for PCI mapped devices and rmmod

[PATCH v2 0/3] Add support for PCI mapped devices and rmmod

[Ricardo Ribalda Delgado]

The main purpose of this patchset is to use the driver on a PCI mapped fpga.

This patchset also converts the driver to platform device, so it will support
hot plugged devices and rmmod.

Changeset:

v2
Suggested by Alexandre Courbot <gnurou@gmail.com>
Merge 2 and 3 (convert to platform device and implement remove)

Ricardo Ribalda Delgado (3):
  gpio/xilinx: Remove offset property
  gpio/xilinx: Convert the driver to platform device interface
  gpio/xilinx: Add support for X86 Arch

 drivers/gpio/Kconfig       |   4 +-
 drivers/gpio/gpio-xilinx.c | 114 ++++++++++++++++++++++++++++-----------------
 2 files changed, 73 insertions(+), 45 deletions(-)

-- 
2.1.3

[PATCH v2 1/3] gpio/xilinx: Remove offset property

[Ricardo Ribalda Delgado]

Instead of calculating the register offset per call, pre-calculate it on
probe time.

Acked-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---

v2: Add Acked-by 

 drivers/gpio/gpio-xilinx.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index ba18b06..9483950 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -50,7 +50,6 @@ struct xgpio_instance {
 	struct of_mm_gpio_chip mmchip;
 	u32 gpio_state;
 	u32 gpio_dir;
-	u32 offset;
 	spinlock_t gpio_lock;
 };
 
@@ -65,12 +64,8 @@ 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 !!(xgpio_readreg(regs + XGPIO_DATA_OFFSET) & BIT(gpio));
+	return !!(xgpio_readreg(mm_gc->regs + XGPIO_DATA_OFFSET) & BIT(gpio));
 }
 
 /**
@@ -88,7 +83,6 @@ 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);
 
@@ -98,8 +92,7 @@ static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 	else
 		chip->gpio_state &= ~BIT(gpio);
 
-	xgpio_writereg(regs + chip->offset + XGPIO_DATA_OFFSET,
-							 chip->gpio_state);
+	xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET, chip->gpio_state);
 
 	spin_unlock_irqrestore(&chip->gpio_lock, flags);
 }
@@ -119,13 +112,12 @@ 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 |= BIT(gpio);
-	xgpio_writereg(regs + chip->offset + XGPIO_TRI_OFFSET, chip->gpio_dir);
+	xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir);
 
 	spin_unlock_irqrestore(&chip->gpio_lock, flags);
 
@@ -148,7 +140,6 @@ 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);
 
@@ -157,12 +148,11 @@ static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 		chip->gpio_state |= BIT(gpio);
 	else
 		chip->gpio_state &= ~BIT(gpio);
-	xgpio_writereg(regs + chip->offset + XGPIO_DATA_OFFSET,
-		       chip->gpio_state);
+	xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET, chip->gpio_state);
 
 	/* Clear the GPIO bit in shadow register and set direction as output */
 	chip->gpio_dir &= ~BIT(gpio);
-	xgpio_writereg(regs + chip->offset + XGPIO_TRI_OFFSET, chip->gpio_dir);
+	xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir);
 
 	spin_unlock_irqrestore(&chip->gpio_lock, flags);
 
@@ -178,10 +168,8 @@ static void xgpio_save_regs(struct of_mm_gpio_chip *mm_gc)
 	struct xgpio_instance *chip =
 	    container_of(mm_gc, struct xgpio_instance, mmchip);
 
-	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);
+	xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET,	chip->gpio_state);
+	xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir);
 }
 
 /**
@@ -247,9 +235,6 @@ static int xgpio_of_probe(struct device_node *np)
 		if (!chip)
 			return -ENOMEM;
 
-		/* Add dual channel offset */
-		chip->offset = XGPIO_CHANNEL_OFFSET;
-
 		/* Update GPIO state shadow register with default value */
 		of_property_read_u32(np, "xlnx,dout-default-2",
 				     &chip->gpio_state);
@@ -285,6 +270,10 @@ static int xgpio_of_probe(struct device_node *np)
 			np->full_name, status);
 			return status;
 		}
+
+		/* Add dual channel offset */
+		chip->mmchip.regs += XGPIO_CHANNEL_OFFSET;
+
 		pr_info("XGpio: %s: dual channel registered, base is %d\n",
 					np->full_name, chip->mmchip.gc.base);
 	}
-- 
2.1.3

[PATCH v2 2/3] gpio/xilinx: Convert the driver to platform device interface

[Ricardo Ribalda Delgado]

This way we do not need to transverse the device tree manually and we
support hot plugged devices.

Also Implement remove callback so the driver can be unloaded

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---

v2: Suggested by Alexandre Courbot <gnurou@gmail.com>

Merge convert to platform device and implement remove callback in one patch

 drivers/gpio/gpio-xilinx.c | 79 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 59 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index 9483950..f946d96 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -51,6 +51,11 @@ struct xgpio_instance {
 	u32 gpio_state;
 	u32 gpio_dir;
 	spinlock_t gpio_lock;
+	bool inited;
+};
+
+struct xgpio {
+	struct xgpio_instance port[2];
 };
 
 /**
@@ -173,24 +178,57 @@ static void xgpio_save_regs(struct of_mm_gpio_chip *mm_gc)
 }
 
 /**
+ * xgpio_remove - Remove method for the GPIO device.
+ * @pdev: pointer to the platform device
+ *
+ * This function remove gpiochips and frees all the allocated resources.
+ */
+static int xgpio_remove(struct platform_device *pdev)
+{
+	struct xgpio *xgpio = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < 2; i++) {
+		if (!xgpio->port[i].inited)
+			continue;
+		gpiochip_remove(&xgpio->port[i].mmchip.gc);
+
+		if (i == 1)
+			xgpio->port[i].mmchip.regs -= XGPIO_CHANNEL_OFFSET;
+
+		iounmap(xgpio->port[i].mmchip.regs);
+		kfree(xgpio->port[i].mmchip.gc.label);
+	}
+
+	return 0;
+}
+
+/**
  * xgpio_of_probe - Probe method for the GPIO device.
- * @np: pointer to device tree node
+ * @pdev: pointer to the platform device
  *
  * This function probes the GPIO device in the device tree. It initializes the
  * driver data structure. It returns 0, if the driver is bound to the GPIO
  * device, or a negative value if there is an error.
  */
-static int xgpio_of_probe(struct device_node *np)
+static int xgpio_probe(struct platform_device *pdev)
 {
+	struct xgpio *xgpio;
 	struct xgpio_instance *chip;
 	int status = 0;
+	struct device_node *np = pdev->dev.of_node;
 	const u32 *tree_info;
 	u32 ngpio;
 
-	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
-	if (!chip)
+	xgpio = (struct xgpio *) devm_kzalloc(&pdev->dev, sizeof(*xgpio),
+					      GFP_KERNEL);
+	if (!xgpio)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, xgpio);
+
+	chip = &xgpio->port[0];
+
 	/* Update GPIO state shadow register with default value */
 	of_property_read_u32(np, "xlnx,dout-default", &chip->gpio_state);
 
@@ -210,6 +248,7 @@ static int xgpio_of_probe(struct device_node *np)
 
 	spin_lock_init(&chip->gpio_lock);
 
+	chip->mmchip.gc.dev = &pdev->dev;
 	chip->mmchip.gc.direction_input = xgpio_dir_in;
 	chip->mmchip.gc.direction_output = xgpio_dir_out;
 	chip->mmchip.gc.get = xgpio_get;
@@ -220,20 +259,18 @@ static int xgpio_of_probe(struct device_node *np)
 	/* Call the OF gpio helper to setup and register the GPIO device */
 	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;
 	}
+	chip->inited = true;
 
 	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;
+		chip = &xgpio->port[1];
 
 		/* Update GPIO state shadow register with default value */
 		of_property_read_u32(np, "xlnx,dout-default-2",
@@ -255,6 +292,7 @@ static int xgpio_of_probe(struct device_node *np)
 
 		spin_lock_init(&chip->gpio_lock);
 
+		chip->mmchip.gc.dev = &pdev->dev;
 		chip->mmchip.gc.direction_input = xgpio_dir_in;
 		chip->mmchip.gc.direction_output = xgpio_dir_out;
 		chip->mmchip.gc.get = xgpio_get;
@@ -265,7 +303,7 @@ static int xgpio_of_probe(struct device_node *np)
 		/* Call the OF gpio helper to setup and register the GPIO dev */
 		status = of_mm_gpiochip_add(np, &chip->mmchip);
 		if (status) {
-			kfree(chip);
+			xgpio_remove(pdev);
 			pr_err("%s: error in probe function with status %d\n",
 			np->full_name, status);
 			return status;
@@ -273,6 +311,7 @@ static int xgpio_of_probe(struct device_node *np)
 
 		/* Add dual channel offset */
 		chip->mmchip.regs += XGPIO_CHANNEL_OFFSET;
+		chip->inited = true;
 
 		pr_info("XGpio: %s: dual channel registered, base is %d\n",
 					np->full_name, chip->mmchip.gc.base);
@@ -286,19 +325,19 @@ static const struct of_device_id xgpio_of_match[] = {
 	{ /* end of list */ },
 };
 
-static int __init xgpio_init(void)
-{
-	struct device_node *np;
-
-	for_each_matching_node(np, xgpio_of_match)
-		xgpio_of_probe(np);
+MODULE_DEVICE_TABLE(of, xgpio_of_match);
 
-	return 0;
-}
+static struct platform_driver xgpio_plat_driver = {
+	.probe		= xgpio_probe,
+	.remove		= xgpio_remove,
+	.driver		= {
+			.name = "gpio-xilinx",
+			.owner = THIS_MODULE,
+			.of_match_table	= xgpio_of_match,
+	},
+};
 
-/* Make sure we get initialized before anyone else tries to use us */
-subsys_initcall(xgpio_init);
-/* No exit call at the moment as we cannot unregister of GPIO chips */
+module_platform_driver(xgpio_plat_driver);
 
 MODULE_AUTHOR("Xilinx, Inc.");
 MODULE_DESCRIPTION("Xilinx GPIO driver");
-- 
2.1.3

[PATCH v2 3/3] gpio/xilinx: Add support for X86 Arch

[Ricardo Ribalda Delgado]

Core can be accessed via PCIe on X86 platform.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---

v2: Minor change on title

 drivers/gpio/Kconfig       | 4 ++--
 drivers/gpio/gpio-xilinx.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0959ca9..7f8ba83 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -343,8 +343,8 @@ config GPIO_XGENE
 	  here to enable the GFC GPIO functionality.
 
 config GPIO_XILINX
-	bool "Xilinx GPIO support"
-	depends on PPC_OF || MICROBLAZE || ARCH_ZYNQ
+	tristate "Xilinx GPIO support"
+	depends on OF_GPIO && (PPC_OF || MICROBLAZE || ARCH_ZYNQ || ARCH_X86)
 	help
 	  Say yes here to support the Xilinx FPGA GPIO device
 
diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index f946d96..5463540 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -30,7 +30,7 @@
 #define XGPIO_CHANNEL_OFFSET	0x8
 
 /* Read/Write access to the GPIO registers */
-#ifdef CONFIG_ARCH_ZYNQ
+#if defined(CONFIG_ARCH_ZYNQ) || defined(CONFIG_X86)
 # define xgpio_readreg(offset)		readl(offset)
 # define xgpio_writereg(offset, val)	writel(val, offset)
 #else
-- 
2.1.3

Re: [PATCH v2 1/3] gpio/xilinx: Remove offset property

[Michal Simek]

On 12/10/2014 02:21 PM, Ricardo Ribalda Delgado wrote:
> Instead of calculating the register offset per call, pre-calculate it on
> probe time.
> 
> Acked-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> 
> v2: Add Acked-by 
> 
>  drivers/gpio/gpio-xilinx.c | 33 +++++++++++----------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index ba18b06..9483950 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -50,7 +50,6 @@ struct xgpio_instance {
>  	struct of_mm_gpio_chip mmchip;
>  	u32 gpio_state;
>  	u32 gpio_dir;
> -	u32 offset;

when you remove offset property you should also remove offset from comment
above of this structure.

>  	spinlock_t gpio_lock;
>  };
>  
> @@ -65,12 +64,8 @@ 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 !!(xgpio_readreg(regs + XGPIO_DATA_OFFSET) & BIT(gpio));
> +	return !!(xgpio_readreg(mm_gc->regs + XGPIO_DATA_OFFSET) & BIT(gpio));
>  }
>  
>  /**
> @@ -88,7 +83,6 @@ 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);
>  
> @@ -98,8 +92,7 @@ static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
>  	else
>  		chip->gpio_state &= ~BIT(gpio);
>  
> -	xgpio_writereg(regs + chip->offset + XGPIO_DATA_OFFSET,
> -							 chip->gpio_state);
> +	xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET, chip->gpio_state);
>  
>  	spin_unlock_irqrestore(&chip->gpio_lock, flags);
>  }
> @@ -119,13 +112,12 @@ 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 |= BIT(gpio);
> -	xgpio_writereg(regs + chip->offset + XGPIO_TRI_OFFSET, chip->gpio_dir);
> +	xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir);
>  
>  	spin_unlock_irqrestore(&chip->gpio_lock, flags);
>  
> @@ -148,7 +140,6 @@ 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);
>  
> @@ -157,12 +148,11 @@ static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
>  		chip->gpio_state |= BIT(gpio);
>  	else
>  		chip->gpio_state &= ~BIT(gpio);
> -	xgpio_writereg(regs + chip->offset + XGPIO_DATA_OFFSET,
> -		       chip->gpio_state);
> +	xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET, chip->gpio_state);
>  
>  	/* Clear the GPIO bit in shadow register and set direction as output */
>  	chip->gpio_dir &= ~BIT(gpio);
> -	xgpio_writereg(regs + chip->offset + XGPIO_TRI_OFFSET, chip->gpio_dir);
> +	xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir);
>  
>  	spin_unlock_irqrestore(&chip->gpio_lock, flags);
>  
> @@ -178,10 +168,8 @@ static void xgpio_save_regs(struct of_mm_gpio_chip *mm_gc)
>  	struct xgpio_instance *chip =
>  	    container_of(mm_gc, struct xgpio_instance, mmchip);
>  
> -	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);
> +	xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET,	chip->gpio_state);
> +	xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir);
>  }
>  
>  /**
> @@ -247,9 +235,6 @@ static int xgpio_of_probe(struct device_node *np)
>  		if (!chip)
>  			return -ENOMEM;
>  
> -		/* Add dual channel offset */
> -		chip->offset = XGPIO_CHANNEL_OFFSET;
> -
>  		/* Update GPIO state shadow register with default value */
>  		of_property_read_u32(np, "xlnx,dout-default-2",
>  				     &chip->gpio_state);
> @@ -285,6 +270,10 @@ static int xgpio_of_probe(struct device_node *np)
>  			np->full_name, status);
>  			return status;
>  		}
> +
> +		/* Add dual channel offset */
> +		chip->mmchip.regs += XGPIO_CHANNEL_OFFSET;
> +
>  		pr_info("XGpio: %s: dual channel registered, base is %d\n",
>  					np->full_name, chip->mmchip.gc.base);
>  	}
> 

Thanks,
Michal

Re: [PATCH v2 1/3] gpio/xilinx: Remove offset property

[Ricardo Ribalda Delgado]

Hello Michal

Thanks for the review

I wait for more comments and then I will resend the patchet with this
fixed. I guess is fine to add your reviewed-by?


Thanks!

On Wed, Dec 10, 2014 at 3:18 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 12/10/2014 02:21 PM, Ricardo Ribalda Delgado wrote:
>> Instead of calculating the register offset per call, pre-calculate it on
>> probe time.
>>
>> Acked-by: Alexandre Courbot <acourbot@nvidia.com>
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>> ---
>>
>> v2: Add Acked-by
>>
>>  drivers/gpio/gpio-xilinx.c | 33 +++++++++++----------------------
>>  1 file changed, 11 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
>> index ba18b06..9483950 100644
>> --- a/drivers/gpio/gpio-xilinx.c
>> +++ b/drivers/gpio/gpio-xilinx.c
>> @@ -50,7 +50,6 @@ struct xgpio_instance {
>>       struct of_mm_gpio_chip mmchip;
>>       u32 gpio_state;
>>       u32 gpio_dir;
>> -     u32 offset;
>
> when you remove offset property you should also remove offset from comment
> above of this structure.
>
>>       spinlock_t gpio_lock;
>>  };
>>
>> @@ -65,12 +64,8 @@ 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 !!(xgpio_readreg(regs + XGPIO_DATA_OFFSET) & BIT(gpio));
>> +     return !!(xgpio_readreg(mm_gc->regs + XGPIO_DATA_OFFSET) & BIT(gpio));
>>  }
>>
>>  /**
>> @@ -88,7 +83,6 @@ 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);
>>
>> @@ -98,8 +92,7 @@ static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
>>       else
>>               chip->gpio_state &= ~BIT(gpio);
>>
>> -     xgpio_writereg(regs + chip->offset + XGPIO_DATA_OFFSET,
>> -                                                      chip->gpio_state);
>> +     xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET, chip->gpio_state);
>>
>>       spin_unlock_irqrestore(&chip->gpio_lock, flags);
>>  }
>> @@ -119,13 +112,12 @@ 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 |= BIT(gpio);
>> -     xgpio_writereg(regs + chip->offset + XGPIO_TRI_OFFSET, chip->gpio_dir);
>> +     xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir);
>>
>>       spin_unlock_irqrestore(&chip->gpio_lock, flags);
>>
>> @@ -148,7 +140,6 @@ 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);
>>
>> @@ -157,12 +148,11 @@ static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
>>               chip->gpio_state |= BIT(gpio);
>>       else
>>               chip->gpio_state &= ~BIT(gpio);
>> -     xgpio_writereg(regs + chip->offset + XGPIO_DATA_OFFSET,
>> -                    chip->gpio_state);
>> +     xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET, chip->gpio_state);
>>
>>       /* Clear the GPIO bit in shadow register and set direction as output */
>>       chip->gpio_dir &= ~BIT(gpio);
>> -     xgpio_writereg(regs + chip->offset + XGPIO_TRI_OFFSET, chip->gpio_dir);
>> +     xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir);
>>
>>       spin_unlock_irqrestore(&chip->gpio_lock, flags);
>>
>> @@ -178,10 +168,8 @@ static void xgpio_save_regs(struct of_mm_gpio_chip *mm_gc)
>>       struct xgpio_instance *chip =
>>           container_of(mm_gc, struct xgpio_instance, mmchip);
>>
>> -     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);
>> +     xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET, chip->gpio_state);
>> +     xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir);
>>  }
>>
>>  /**
>> @@ -247,9 +235,6 @@ static int xgpio_of_probe(struct device_node *np)
>>               if (!chip)
>>                       return -ENOMEM;
>>
>> -             /* Add dual channel offset */
>> -             chip->offset = XGPIO_CHANNEL_OFFSET;
>> -
>>               /* Update GPIO state shadow register with default value */
>>               of_property_read_u32(np, "xlnx,dout-default-2",
>>                                    &chip->gpio_state);
>> @@ -285,6 +270,10 @@ static int xgpio_of_probe(struct device_node *np)
>>                       np->full_name, status);
>>                       return status;
>>               }
>> +
>> +             /* Add dual channel offset */
>> +             chip->mmchip.regs += XGPIO_CHANNEL_OFFSET;
>> +
>>               pr_info("XGpio: %s: dual channel registered, base is %d\n",
>>                                       np->full_name, chip->mmchip.gc.base);
>>       }
>>
>
> Thanks,
> Michal



-- 
Ricardo Ribalda

Re: [PATCH v2 2/3] gpio/xilinx: Convert the driver to platform device interface

[Michal Simek]

On 12/10/2014 02:21 PM, Ricardo Ribalda Delgado wrote:
> This way we do not need to transverse the device tree manually and we
> support hot plugged devices.
> 
> Also Implement remove callback so the driver can be unloaded
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> 
> v2: Suggested by Alexandre Courbot <gnurou@gmail.com>
> 
> Merge convert to platform device and implement remove callback in one patch
> 
>  drivers/gpio/gpio-xilinx.c | 79 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 59 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index 9483950..f946d96 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -51,6 +51,11 @@ struct xgpio_instance {
>  	u32 gpio_state;
>  	u32 gpio_dir;
>  	spinlock_t gpio_lock;
> +	bool inited;

like before - you should document it.
I know it is not proper kernel-doc format but I think
you can simple create one more patch before 1/3
and fix kernel-doc format first and then do your changes.

btw: this kernel-doc fixup is in xilinx git repo.

> +};
> +
> +struct xgpio {
> +	struct xgpio_instance port[2];
>  };
>  
>  /**
> @@ -173,24 +178,57 @@ static void xgpio_save_regs(struct of_mm_gpio_chip *mm_gc)
>  }
>  
>  /**
> + * xgpio_remove - Remove method for the GPIO device.
> + * @pdev: pointer to the platform device
> + *
> + * This function remove gpiochips and frees all the allocated resources.
> + */
> +static int xgpio_remove(struct platform_device *pdev)
> +{
> +	struct xgpio *xgpio = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < 2; i++) {
> +		if (!xgpio->port[i].inited)
> +			continue;
> +		gpiochip_remove(&xgpio->port[i].mmchip.gc);
> +
> +		if (i == 1)
> +			xgpio->port[i].mmchip.regs -= XGPIO_CHANNEL_OFFSET;
> +
> +		iounmap(xgpio->port[i].mmchip.regs);
> +		kfree(xgpio->port[i].mmchip.gc.label);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
>   * xgpio_of_probe - Probe method for the GPIO device.
> - * @np: pointer to device tree node
> + * @pdev: pointer to the platform device
>   *
>   * This function probes the GPIO device in the device tree. It initializes the
>   * driver data structure. It returns 0, if the driver is bound to the GPIO
>   * device, or a negative value if there is an error.
>   */
> -static int xgpio_of_probe(struct device_node *np)
> +static int xgpio_probe(struct platform_device *pdev)
>  {
> +	struct xgpio *xgpio;
>  	struct xgpio_instance *chip;
>  	int status = 0;
> +	struct device_node *np = pdev->dev.of_node;
>  	const u32 *tree_info;
>  	u32 ngpio;
>  
> -	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> -	if (!chip)
> +	xgpio = (struct xgpio *) devm_kzalloc(&pdev->dev, sizeof(*xgpio),
> +					      GFP_KERNEL);

don't need to recast it here.

> +	if (!xgpio)
>  		return -ENOMEM;
>  
> +	platform_set_drvdata(pdev, xgpio);
> +
> +	chip = &xgpio->port[0];
> +
>  	/* Update GPIO state shadow register with default value */
>  	of_property_read_u32(np, "xlnx,dout-default", &chip->gpio_state);
>  
> @@ -210,6 +248,7 @@ static int xgpio_of_probe(struct device_node *np)
>  
>  	spin_lock_init(&chip->gpio_lock);
>  
> +	chip->mmchip.gc.dev = &pdev->dev;
>  	chip->mmchip.gc.direction_input = xgpio_dir_in;
>  	chip->mmchip.gc.direction_output = xgpio_dir_out;
>  	chip->mmchip.gc.get = xgpio_get;
> @@ -220,20 +259,18 @@ static int xgpio_of_probe(struct device_node *np)
>  	/* Call the OF gpio helper to setup and register the GPIO device */
>  	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;
>  	}
> +	chip->inited = true;
>  
>  	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;
> +		chip = &xgpio->port[1];
>  
>  		/* Update GPIO state shadow register with default value */
>  		of_property_read_u32(np, "xlnx,dout-default-2",
> @@ -255,6 +292,7 @@ static int xgpio_of_probe(struct device_node *np)
>  
>  		spin_lock_init(&chip->gpio_lock);
>  
> +		chip->mmchip.gc.dev = &pdev->dev;
>  		chip->mmchip.gc.direction_input = xgpio_dir_in;
>  		chip->mmchip.gc.direction_output = xgpio_dir_out;
>  		chip->mmchip.gc.get = xgpio_get;
> @@ -265,7 +303,7 @@ static int xgpio_of_probe(struct device_node *np)
>  		/* Call the OF gpio helper to setup and register the GPIO dev */
>  		status = of_mm_gpiochip_add(np, &chip->mmchip);
>  		if (status) {
> -			kfree(chip);
> +			xgpio_remove(pdev);
>  			pr_err("%s: error in probe function with status %d\n",
>  			np->full_name, status);
>  			return status;
> @@ -273,6 +311,7 @@ static int xgpio_of_probe(struct device_node *np)
>  
>  		/* Add dual channel offset */
>  		chip->mmchip.regs += XGPIO_CHANNEL_OFFSET;
> +		chip->inited = true;
>  
>  		pr_info("XGpio: %s: dual channel registered, base is %d\n",
>  					np->full_name, chip->mmchip.gc.base);
> @@ -286,19 +325,19 @@ static const struct of_device_id xgpio_of_match[] = {
>  	{ /* end of list */ },
>  };
>  
> -static int __init xgpio_init(void)
> -{
> -	struct device_node *np;
> -
> -	for_each_matching_node(np, xgpio_of_match)
> -		xgpio_of_probe(np);
> +MODULE_DEVICE_TABLE(of, xgpio_of_match);
>  
> -	return 0;
> -}
> +static struct platform_driver xgpio_plat_driver = {
> +	.probe		= xgpio_probe,
> +	.remove		= xgpio_remove,
> +	.driver		= {
> +			.name = "gpio-xilinx",
> +			.owner = THIS_MODULE,

remove this line - it was the part of resent cleanup

> +			.of_match_table	= xgpio_of_match,
> +	},
> +};
>  
> -/* Make sure we get initialized before anyone else tries to use us */
> -subsys_initcall(xgpio_init);
> -/* No exit call at the moment as we cannot unregister of GPIO chips */
> +module_platform_driver(xgpio_plat_driver);

subsys is 4. module_platform is different level that's why you are changing
order here. You shouldn't do it. Just keep the same level because
some drivers can depend on it.

Thanks,
Michal

Re: [PATCH v2 3/3] gpio/xilinx: Add support for X86 Arch

[Michal Simek]

On 12/10/2014 02:21 PM, Ricardo Ribalda Delgado wrote:
> Core can be accessed via PCIe on X86 platform.
> 

You should also describe also that you are changing that this driver
can be used as module.

> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---

Thanks,
Michal

Re: [PATCH v2 2/3] gpio/xilinx: Convert the driver to platform device interface

[Ricardo Ribalda Delgado]

Agree on all :)

Regarding the kernel-doc patch I rather add it after the other changes.


Will resend in a second, tell me if you want your reviewed-by or
ack-by for any  of the 3 patches

Thanks!
-- 
Ricardo Ribalda