[PATCH v3 00/11] staging: mt7621-gpio: use mediatek as binding instead of custom mtk

[PATCH v3 00/11] staging: mt7621-gpio: use mediatek as binding instead of custom mtk

[Sergio Paracuellos]

The following series updated mt7621-gpio driver to use 'mediatek'
which is already defined in kernel bindings documentation instead
of add a new custom one 'mtk' for this company. mt7621-gpio device-tree
documentation has been added also to the staging driver directory.

v3:
 - Fix condition for check for a valid gpio id in driver probe function
   included in the PATCH 8 ("staging: mt7621-gpio: avoid devm_kzalloc() 
   hidden inside declarations and refactor function a bit") of the series.
 - Rest of patches are not modified at all.

v2:
 - Join some patch ("staging: mt7621-gpio: avoid use of globals
   and use platform_data instead") with the needed fixes to make
   it work pointed out by NeilBrown
 - Add gpio interrupts to documentation and device tree in order to
   can try them
 - Other minor code cleanups have been added also.

After this changes only testing interrupts should be remaining to
get this out of staging.

Hope this helps.

Best regards,
     Sergio

Sergio Paracuellos (11):
  staging: mt7621-gpio: dt-bindings: add documentation for mt7621-gpio
  staging: mt7621-gpio: update TODO file
  staging: mt7621-dts: update gpios related entries to use 'mediatek'
  staging: mt7621-gpio: replace 'mtk' to use correct one 'mediatek'
  staging: mt7621-gpio: avoid use of globals and use platform_data
    instead
  staging: mt7621-dts: add interrupt device tree nodes for the gpio
    controller
  staging: mt7621-gpio: dt-bindings: add interrupt nodes to bindings doc
  staging: mt7621-gpio: avoid devm_kzalloc() hidden inside declarations
    and refactor function a bit
  staging: mt7621-gpio: use ternary operator in return in
    mediatek_gpio_get_direction
  staging: mt7621-gpio: use MTK_BANK_WIDTH instead of magic number
  staging: mt7621-gpio: update TODO list

 drivers/staging/mt7621-dts/mt7621.dtsi             |  11 +-
 drivers/staging/mt7621-gpio/TODO                   |   3 +-
 drivers/staging/mt7621-gpio/gpio-mt7621.c          | 111 +++++++++++++--------
 .../staging/mt7621-gpio/mediatek,mt7621-gpio.txt   |  58 +++++++++++
 4 files changed, 137 insertions(+), 46 deletions(-)
 create mode 100644 drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt

-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 01/11] staging: mt7621-gpio: dt-bindings: add documentation for mt7621-gpio

[Sergio Paracuellos]

This commit add missing dt bindings documentation for mt7621-gpio
driver. There is some missing stuff here about interrupts with is
not also being used in the mt7621.dtsi file. So just include in
staging a incomplete version before moving this to kernel's dt-bindings
place.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 .../staging/mt7621-gpio/mediatek,mt7621-gpio.txt   | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt

diff --git a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
new file mode 100644
index 0000000..54de9f7
--- /dev/null
+++ b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
@@ -0,0 +1,51 @@
+Mediatek SoC GPIO controller bindings
+
+The IP core used inside these SoCs has 3 banks of 32 GPIOs each.
+The registers of all the banks are interwoven inside one single IO range.
+We load one GPIO controller instance per bank. To make this possible
+we support 2 types of nodes. The parent node defines the memory I/O range and
+has 3 children each describing a single bank.
+
+Required properties for the top level node:
+- compatible:
+  - "mediatek,mt7621-gpio" for Mediatek controllers
+- reg : Physical base address and length of the controller's registers
+
+Required properties for the GPIO bank node:
+- compatible:
+  - "mediatek,mt7621-gpio-bank" for Mediatek banks
+- #gpio-cells : Should be two.
+  - first cell is the pin number
+  - second cell is used to specify optional parameters (unused)
+- gpio-controller : Marks the device node as a GPIO controller
+- reg : The id of the bank that the node describes.
+
+Example:
+	gpio@600 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		compatible = "mediatek,mt7621-gpio";
+		reg = <0x600 0x100>;
+
+		gpio0: bank@0 {
+			reg = <0>;
+			compatible = "mediatek,mt7621-gpio-bank";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+
+		gpio1: bank@1 {
+			reg = <1>;
+			compatible = "mediatek,mt7621-gpio-bank";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+
+		gpio2: bank@2 {
+			reg = <2>;
+			compatible = "mediatek,mt7621-gpio-bank";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+	};
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 02/11] staging: mt7621-gpio: update TODO file

[Sergio Paracuellos]

This commit updates TODO file with missing things to
get this driver ready to be mainlined.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/TODO | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/mt7621-gpio/TODO b/drivers/staging/mt7621-gpio/TODO
index 7143905..9077b16 100644
--- a/drivers/staging/mt7621-gpio/TODO
+++ b/drivers/staging/mt7621-gpio/TODO
@@ -1,5 +1,7 @@
 
-- general code review and clean up
+- general code review and clean up 
+- avoid global variables and use drvdata allocated instead
 - ensure device-tree requirements are documented
+- make sure interrupts work
 
 Cc:  NeilBrown <neil@brown.name>
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 03/11] staging: mt7621-dts: update gpios related entries to use 'mediatek'

[Sergio Paracuellos]

Gpio driver for mt7621 is using 'mtk' as binding but in the kernel
is already defined one for this maker which is 'mediatek'. Update
device tree to use the correct one.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-dts/mt7621.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index 9d941b5..115eb04 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -64,26 +64,26 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-			compatible = "mtk,mt7621-gpio";
+			compatible = "mediatek,mt7621-gpio";
 			reg = <0x600 0x100>;
 
 			gpio0: bank@0 {
 				reg = <0>;
-				compatible = "mtk,mt7621-gpio-bank";
+				compatible = "mediatek,mt7621-gpio-bank";
 				gpio-controller;
 				#gpio-cells = <2>;
 			};
 
 			gpio1: bank@1 {
 				reg = <1>;
-				compatible = "mtk,mt7621-gpio-bank";
+				compatible = "mediatek,mt7621-gpio-bank";
 				gpio-controller;
 				#gpio-cells = <2>;
 			};
 
 			gpio2: bank@2 {
 				reg = <2>;
-				compatible = "mtk,mt7621-gpio-bank";
+				compatible = "mediatek,mt7621-gpio-bank";
 				gpio-controller;
 				#gpio-cells = <2>;
 			};
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 04/11] staging: mt7621-gpio: replace 'mtk' to use correct one 'mediatek'

[Sergio Paracuellos]

Gpio driver is using mtk and there is already 'mediatek' binding
defined for this maker. Update driver to use it instead the custom
one 'mtk'.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index a577381..7d17949 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -323,7 +323,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
 	}
 
 	for_each_child_of_node(np, bank)
-		if (of_device_is_compatible(bank, "mtk,mt7621-gpio-bank"))
+		if (of_device_is_compatible(bank, "mediatek,mt7621-gpio-bank"))
 			mediatek_gpio_bank_probe(pdev, bank);
 
 	if (mediatek_gpio_irq_domain)
@@ -334,7 +334,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id mediatek_gpio_match[] = {
-	{ .compatible = "mtk,mt7621-gpio" },
+	{ .compatible = "mediatek,mt7621-gpio" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, mediatek_gpio_match);
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 05/11] staging: mt7621-gpio: avoid use of globals and use platform_data instead

[Sergio Paracuellos]

Gpio driver have a some globals which can be avoided just
using platform_data in a proper form. This commit adds a new
struct mtk_data which includes all of those globals setting them
using platform_set_drvdata and devm_gpiochip_add_data functions.
With this properly set we are able to retrieve driver data along
the code using kernel api's so globals are not needed anymore.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 85 +++++++++++++++++++++----------
 1 file changed, 59 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 7d17949..c701259 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -31,17 +31,20 @@ enum mediatek_gpio_reg {
 	GPIO_REG_EDGE,
 };
 
-static void __iomem *mediatek_gpio_membase;
-static int mediatek_gpio_irq;
-static struct irq_domain *mediatek_gpio_irq_domain;
-
-static struct mtk_gc {
+struct mtk_gc {
 	struct gpio_chip chip;
 	spinlock_t lock;
 	int bank;
 	u32 rising;
 	u32 falling;
-} *gc_map[MTK_MAX_BANK];
+};
+
+struct mtk_data {
+	void __iomem *gpio_membase;
+	int gpio_irq;
+	struct irq_domain *gpio_irq_domain;
+	struct mtk_gc *gc_map[MTK_MAX_BANK];
+};
 
 static inline struct mtk_gc
 *to_mediatek_gpio(struct gpio_chip *chip)
@@ -56,15 +59,19 @@ static inline struct mtk_gc
 static inline void
 mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
 {
-	iowrite32(val, mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
+	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
+	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
+
+	iowrite32(val, gpio_data->gpio_membase + offset);
 }
 
 static inline u32
 mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
 {
+	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
 	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
 
-	return ioread32(mediatek_gpio_membase + offset);
+	return ioread32(gpio_data->gpio_membase + offset);
 }
 
 static void
@@ -137,23 +144,26 @@ mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 static int
 mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
 {
+	struct mtk_data *gpio_data = gpiochip_get_data(chip);
 	struct mtk_gc *rg = to_mediatek_gpio(chip);
 
-	return irq_create_mapping(mediatek_gpio_irq_domain,
+	return irq_create_mapping(gpio_data->gpio_irq_domain,
 				  pin + (rg->bank * MTK_BANK_WIDTH));
 }
 
 static int
 mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 {
+	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
 	const __be32 *id = of_get_property(bank, "reg", NULL);
 	struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
 				sizeof(struct mtk_gc), GFP_KERNEL);
+	int ret;
 
 	if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
 		return -ENOMEM;
 
-	gc_map[be32_to_cpu(*id)] = rg;
+	gpio_data->gc_map[be32_to_cpu(*id)] = rg;
 
 	memset(rg, 0, sizeof(struct mtk_gc));
 
@@ -169,10 +179,17 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 	rg->chip.get_direction = mediatek_gpio_get_direction;
 	rg->chip.get = mediatek_gpio_get;
 	rg->chip.set = mediatek_gpio_set;
-	if (mediatek_gpio_irq_domain)
+	if (gpio_data->gpio_irq_domain)
 		rg->chip.to_irq = mediatek_gpio_to_irq;
 	rg->bank = be32_to_cpu(*id);
 
+	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
+			rg->chip.ngpio, ret);
+		return ret;
+	}
+
 	/* set polarity to low for all gpios */
 	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
 
@@ -184,10 +201,12 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 static void
 mediatek_gpio_irq_handler(struct irq_desc *desc)
 {
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 	int i;
 
 	for (i = 0; i < MTK_MAX_BANK; i++) {
-		struct mtk_gc *rg = gc_map[i];
+		struct mtk_gc *rg = gpio_data->gc_map[i];
 		unsigned long pending;
 		int bit;
 
@@ -197,7 +216,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
 		pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
 
 		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
-			u32 map = irq_find_mapping(mediatek_gpio_irq_domain,
+			u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
 						   (MTK_BANK_WIDTH * i) + bit);
 
 			generic_handle_irq(map);
@@ -209,9 +228,11 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
 static void
 mediatek_gpio_irq_unmask(struct irq_data *d)
 {
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 	int pin = d->hwirq;
 	int bank = pin / 32;
-	struct mtk_gc *rg = gc_map[bank];
+	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
 
@@ -230,9 +251,11 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 static void
 mediatek_gpio_irq_mask(struct irq_data *d)
 {
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 	int pin = d->hwirq;
 	int bank = pin / 32;
-	struct mtk_gc *rg = gc_map[bank];
+	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
 
@@ -251,9 +274,11 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 static int
 mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 {
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 	int pin = d->hwirq;
 	int bank = pin / 32;
-	struct mtk_gc *rg = gc_map[bank];
+	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	u32 mask = BIT(d->hwirq);
 
 	if (!rg)
@@ -308,27 +333,35 @@ mediatek_gpio_probe(struct platform_device *pdev)
 {
 	struct device_node *bank, *np = pdev->dev.of_node;
 	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct mtk_data *gpio_data;
 
-	mediatek_gpio_membase = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(mediatek_gpio_membase))
-		return PTR_ERR(mediatek_gpio_membase);
+	gpio_data = devm_kzalloc(&pdev->dev, sizeof(*gpio_data), GFP_KERNEL);
+	if (!gpio_data)
+		return -ENOMEM;
 
-	mediatek_gpio_irq = irq_of_parse_and_map(np, 0);
-	if (mediatek_gpio_irq) {
-		mediatek_gpio_irq_domain = irq_domain_add_linear(np,
+	gpio_data->gpio_membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(gpio_data->gpio_membase))
+		return PTR_ERR(gpio_data->gpio_membase);
+
+	gpio_data->gpio_irq = irq_of_parse_and_map(np, 0);
+	if (gpio_data->gpio_irq) {
+		gpio_data->gpio_irq_domain = irq_domain_add_linear(np,
 			MTK_MAX_BANK * MTK_BANK_WIDTH,
 			&irq_domain_ops, NULL);
-		if (!mediatek_gpio_irq_domain)
+		if (!gpio_data->gpio_irq_domain)
 			dev_err(&pdev->dev, "irq_domain_add_linear failed\n");
 	}
 
+	platform_set_drvdata(pdev, gpio_data);
+
 	for_each_child_of_node(np, bank)
 		if (of_device_is_compatible(bank, "mediatek,mt7621-gpio-bank"))
 			mediatek_gpio_bank_probe(pdev, bank);
 
-	if (mediatek_gpio_irq_domain)
-		irq_set_chained_handler(mediatek_gpio_irq,
-					mediatek_gpio_irq_handler);
+	if (gpio_data->gpio_irq_domain)
+		irq_set_chained_handler_and_data(gpio_data->gpio_irq,
+						 mediatek_gpio_irq_handler,
+						 gpio_data);
 
 	return 0;
 }
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 06/11] staging: mt7621-dts: add interrupt device tree nodes for the gpio controller

[Sergio Paracuellos]

The GPIO controller of mt7621 can receive interrupts on any
of the GPIOs, either edge or level. It then interrupts the CPU using
GIC INT12. Update device tree accordly.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-dts/mt7621.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index 115eb04..240d396 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -67,6 +67,9 @@
 			compatible = "mediatek,mt7621-gpio";
 			reg = <0x600 0x100>;
 
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
+
 			gpio0: bank@0 {
 				reg = <0>;
 				compatible = "mediatek,mt7621-gpio-bank";
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 07/11] staging: mt7621-gpio: dt-bindings: add interrupt nodes to bindings doc

[Sergio Paracuellos]

Interrupt related stuff for gpio controller in mt7621 was missing
in device tree documentation. Add it to complete documentation for
this driver.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
index 54de9f7..af64092 100644
--- a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
+++ b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
@@ -4,12 +4,16 @@ The IP core used inside these SoCs has 3 banks of 32 GPIOs each.
 The registers of all the banks are interwoven inside one single IO range.
 We load one GPIO controller instance per bank. To make this possible
 we support 2 types of nodes. The parent node defines the memory I/O range and
-has 3 children each describing a single bank.
+has 3 children each describing a single bank. Also the GPIO controller can receive
+interrupts on any of the GPIOs, either edge or level. It then interrupts the CPU
+using GIC INT12.
 
 Required properties for the top level node:
 - compatible:
   - "mediatek,mt7621-gpio" for Mediatek controllers
 - reg : Physical base address and length of the controller's registers
+- interrupt-parent : phandle of the parent interrupt controller.
+- interrupts = Interrupt specifier for the controllers interrupt
 
 Required properties for the GPIO bank node:
 - compatible:
@@ -28,6 +32,9 @@ Example:
 		compatible = "mediatek,mt7621-gpio";
 		reg = <0x600 0x100>;
 
+		interrupt-parent = <&gic>;
+		interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
+
 		gpio0: bank@0 {
 			reg = <0>;
 			compatible = "mediatek,mt7621-gpio-bank";
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 08/11] staging: mt7621-gpio: avoid devm_kzalloc() hidden inside declarations and refactor function a bit

[Sergio Paracuellos]

Driver probe function includes an allocation using devm_kzalloc
which is "hidden" a bit inside the declarations. Extract this
to a better place to increase readability. Also because we are
allocating zeroed memory 'memset' statement is not needed at all.
Condition for checking for a valid gpio id is wrong and it should
be greater or equal instead of only greater so update to be the
good one.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index c701259..016475f 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -156,17 +156,18 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 {
 	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
 	const __be32 *id = of_get_property(bank, "reg", NULL);
-	struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
-				sizeof(struct mtk_gc), GFP_KERNEL);
+	struct mtk_gc *rg;
 	int ret;
 
-	if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
+	if (!id || be32_to_cpu(*id) >= MTK_MAX_BANK)
+		return -EINVAL;
+
+	rg = devm_kzalloc(&pdev->dev, sizeof(struct mtk_gc), GFP_KERNEL);
+	if (!rg)
 		return -ENOMEM;
 
 	gpio_data->gc_map[be32_to_cpu(*id)] = rg;
 
-	memset(rg, 0, sizeof(struct mtk_gc));
-
 	spin_lock_init(&rg->lock);
 
 	rg->chip.parent = &pdev->dev;
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 09/11] staging: mt7621-gpio: use ternary operator in return in mediatek_gpio_get_direction

[Sergio Paracuellos]

This commits replaces if statement and two returns in favour
of a only one return using a ternary operator.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 016475f..9fcdaf4 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -135,10 +135,7 @@ mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 	t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
 	spin_unlock_irqrestore(&rg->lock, flags);
 
-	if (t & BIT(offset))
-		return 0;
-
-	return 1;
+	return (t & BIT(offset)) ? 0 : 1;
 }
 
 static int
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 10/11] staging: mt7621-gpio: use MTK_BANK_WIDTH instead of magic number

[Sergio Paracuellos]

There are some places where magic number '32' is being used to get
the gpio bank. There already exist a definition MTK_BANK_WIDTH
with this value, so just use it instead.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 9fcdaf4..0b403f9 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -229,7 +229,7 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 	int pin = d->hwirq;
-	int bank = pin / 32;
+	int bank = pin / MTK_BANK_WIDTH;
 	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
@@ -252,7 +252,7 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 	int pin = d->hwirq;
-	int bank = pin / 32;
+	int bank = pin / MTK_BANK_WIDTH;
 	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
@@ -275,7 +275,7 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct mtk_data *gpio_data = gpiochip_get_data(gc);
 	int pin = d->hwirq;
-	int bank = pin / 32;
+	int bank = pin / MTK_BANK_WIDTH;
 	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	u32 mask = BIT(d->hwirq);
 
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3 11/11] staging: mt7621-gpio: update TODO list

[Sergio Paracuellos]

Some of the remaining stuff included in TODO list
have been complete. So update this file accordly.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/TODO | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/TODO b/drivers/staging/mt7621-gpio/TODO
index 9077b16..9089833 100644
--- a/drivers/staging/mt7621-gpio/TODO
+++ b/drivers/staging/mt7621-gpio/TODO
@@ -1,7 +1,4 @@
 
-- general code review and clean up 
-- avoid global variables and use drvdata allocated instead
-- ensure device-tree requirements are documented
 - make sure interrupts work
 
 Cc:  NeilBrown <neil@brown.name>
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 05/11] staging: mt7621-gpio: avoid use of globals and use platform_data instead

[NeilBrown]

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

On Wed, May 16 2018, Sergio Paracuellos wrote:

> Gpio driver have a some globals which can be avoided just
> using platform_data in a proper form. This commit adds a new
> struct mtk_data which includes all of those globals setting them
> using platform_set_drvdata and devm_gpiochip_add_data functions.
> With this properly set we are able to retrieve driver data along
> the code using kernel api's so globals are not needed anymore.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/staging/mt7621-gpio/gpio-mt7621.c | 85 +++++++++++++++++++++----------
>  1 file changed, 59 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index 7d17949..c701259 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -31,17 +31,20 @@ enum mediatek_gpio_reg {
>  	GPIO_REG_EDGE,
>  };
>  
> -static void __iomem *mediatek_gpio_membase;
> -static int mediatek_gpio_irq;
> -static struct irq_domain *mediatek_gpio_irq_domain;
> -
> -static struct mtk_gc {
> +struct mtk_gc {
>  	struct gpio_chip chip;
>  	spinlock_t lock;
>  	int bank;
>  	u32 rising;
>  	u32 falling;
> -} *gc_map[MTK_MAX_BANK];
> +};
> +
> +struct mtk_data {
> +	void __iomem *gpio_membase;
> +	int gpio_irq;
> +	struct irq_domain *gpio_irq_domain;
> +	struct mtk_gc *gc_map[MTK_MAX_BANK];
> +};
>  
>  static inline struct mtk_gc
>  *to_mediatek_gpio(struct gpio_chip *chip)
> @@ -56,15 +59,19 @@ static inline struct mtk_gc
>  static inline void
>  mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
>  {
> -	iowrite32(val, mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
> +	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> +	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> +
> +	iowrite32(val, gpio_data->gpio_membase + offset);
>  }
>  
>  static inline u32
>  mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
>  {
> +	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
>  	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
>  
> -	return ioread32(mediatek_gpio_membase + offset);
> +	return ioread32(gpio_data->gpio_membase + offset);
>  }
>  
>  static void
> @@ -137,23 +144,26 @@ mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>  static int
>  mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
>  {
> +	struct mtk_data *gpio_data = gpiochip_get_data(chip);
>  	struct mtk_gc *rg = to_mediatek_gpio(chip);
>  
> -	return irq_create_mapping(mediatek_gpio_irq_domain,
> +	return irq_create_mapping(gpio_data->gpio_irq_domain,
>  				  pin + (rg->bank * MTK_BANK_WIDTH));
>  }
>  
>  static int
>  mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  {
> +	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
>  	const __be32 *id = of_get_property(bank, "reg", NULL);
>  	struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
>  				sizeof(struct mtk_gc), GFP_KERNEL);
> +	int ret;
>  
>  	if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
>  		return -ENOMEM;
>  
> -	gc_map[be32_to_cpu(*id)] = rg;
> +	gpio_data->gc_map[be32_to_cpu(*id)] = rg;
>  
>  	memset(rg, 0, sizeof(struct mtk_gc));
>  
> @@ -169,10 +179,17 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  	rg->chip.get_direction = mediatek_gpio_get_direction;
>  	rg->chip.get = mediatek_gpio_get;
>  	rg->chip.set = mediatek_gpio_set;
> -	if (mediatek_gpio_irq_domain)
> +	if (gpio_data->gpio_irq_domain)
>  		rg->chip.to_irq = mediatek_gpio_to_irq;
>  	rg->bank = be32_to_cpu(*id);
>  
> +	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
> +			rg->chip.ngpio, ret);
> +		return ret;
> +	}
> +

Calling devm_gpiochip_add_data() here looks good.
Not removing
	return gpiochip_add(&rg->chip);
from the end of the function isn't so good :-(

BTW interrupt definitely don't work yet.  The calls
	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
get NULL.  I think some sort of irq_set_chip_data(irq,...) is needed
in mediatek_gpio_gpio_map(), but it is too late at night for it to
all make sense to me :-)

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v3 05/11] staging: mt7621-gpio: avoid use of globals and use platform_data instead

[Sergio Paracuellos]

On Sat, May 19, 2018 at 08:51:43PM +1000, NeilBrown wrote:
> On Wed, May 16 2018, Sergio Paracuellos wrote:
> 
> > Gpio driver have a some globals which can be avoided just
> > using platform_data in a proper form. This commit adds a new
> > struct mtk_data which includes all of those globals setting them
> > using platform_set_drvdata and devm_gpiochip_add_data functions.
> > With this properly set we are able to retrieve driver data along
> > the code using kernel api's so globals are not needed anymore.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >  drivers/staging/mt7621-gpio/gpio-mt7621.c | 85 +++++++++++++++++++++----------
> >  1 file changed, 59 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > index 7d17949..c701259 100644
> > --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > @@ -31,17 +31,20 @@ enum mediatek_gpio_reg {
> >  	GPIO_REG_EDGE,
> >  };
> >  
> > -static void __iomem *mediatek_gpio_membase;
> > -static int mediatek_gpio_irq;
> > -static struct irq_domain *mediatek_gpio_irq_domain;
> > -
> > -static struct mtk_gc {
> > +struct mtk_gc {
> >  	struct gpio_chip chip;
> >  	spinlock_t lock;
> >  	int bank;
> >  	u32 rising;
> >  	u32 falling;
> > -} *gc_map[MTK_MAX_BANK];
> > +};
> > +
> > +struct mtk_data {
> > +	void __iomem *gpio_membase;
> > +	int gpio_irq;
> > +	struct irq_domain *gpio_irq_domain;
> > +	struct mtk_gc *gc_map[MTK_MAX_BANK];
> > +};
> >  
> >  static inline struct mtk_gc
> >  *to_mediatek_gpio(struct gpio_chip *chip)
> > @@ -56,15 +59,19 @@ static inline struct mtk_gc
> >  static inline void
> >  mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
> >  {
> > -	iowrite32(val, mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
> > +	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> > +	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> > +
> > +	iowrite32(val, gpio_data->gpio_membase + offset);
> >  }
> >  
> >  static inline u32
> >  mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
> >  {
> > +	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> >  	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> >  
> > -	return ioread32(mediatek_gpio_membase + offset);
> > +	return ioread32(gpio_data->gpio_membase + offset);
> >  }
> >  
> >  static void
> > @@ -137,23 +144,26 @@ mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> >  static int
> >  mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
> >  {
> > +	struct mtk_data *gpio_data = gpiochip_get_data(chip);
> >  	struct mtk_gc *rg = to_mediatek_gpio(chip);
> >  
> > -	return irq_create_mapping(mediatek_gpio_irq_domain,
> > +	return irq_create_mapping(gpio_data->gpio_irq_domain,
> >  				  pin + (rg->bank * MTK_BANK_WIDTH));
> >  }
> >  
> >  static int
> >  mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> >  {
> > +	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
> >  	const __be32 *id = of_get_property(bank, "reg", NULL);
> >  	struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
> >  				sizeof(struct mtk_gc), GFP_KERNEL);
> > +	int ret;
> >  
> >  	if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
> >  		return -ENOMEM;
> >  
> > -	gc_map[be32_to_cpu(*id)] = rg;
> > +	gpio_data->gc_map[be32_to_cpu(*id)] = rg;
> >  
> >  	memset(rg, 0, sizeof(struct mtk_gc));
> >  
> > @@ -169,10 +179,17 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> >  	rg->chip.get_direction = mediatek_gpio_get_direction;
> >  	rg->chip.get = mediatek_gpio_get;
> >  	rg->chip.set = mediatek_gpio_set;
> > -	if (mediatek_gpio_irq_domain)
> > +	if (gpio_data->gpio_irq_domain)
> >  		rg->chip.to_irq = mediatek_gpio_to_irq;
> >  	rg->bank = be32_to_cpu(*id);
> >  
> > +	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
> > +			rg->chip.ngpio, ret);
> > +		return ret;
> > +	}
> > +
> 
> Calling devm_gpiochip_add_data() here looks good.
> Not removing
> 	return gpiochip_add(&rg->chip);
> from the end of the function isn't so good :-(

True, thanks for pointing this out.

> 
> BTW interrupt definitely don't work yet.  The calls
> 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> get NULL.  I think some sort of irq_set_chip_data(irq,...) is needed
> in mediatek_gpio_gpio_map(), but it is too late at night for it to
> all make sense to me :-)

You are right. It seems irq_set_chip_data must be called in mediatek_gpio_gpio_map
function.

> 
> Thanks,
> NeilBrown

Sent v4 with this two changes.

Hope this helps.

Best regards,
    Sergio Paracuellos


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 05/11] staging: mt7621-gpio: avoid use of globals and use platform_data instead

[NeilBrown]

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

On Sat, May 19 2018, Sergio Paracuellos wrote:

> On Sat, May 19, 2018 at 08:51:43PM +1000, NeilBrown wrote:
>> On Wed, May 16 2018, Sergio Paracuellos wrote:
>> 
>> > Gpio driver have a some globals which can be avoided just
>> > using platform_data in a proper form. This commit adds a new
>> > struct mtk_data which includes all of those globals setting them
>> > using platform_set_drvdata and devm_gpiochip_add_data functions.
>> > With this properly set we are able to retrieve driver data along
>> > the code using kernel api's so globals are not needed anymore.
>> >
>> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>> > ---
>> >  drivers/staging/mt7621-gpio/gpio-mt7621.c | 85 +++++++++++++++++++++----------
>> >  1 file changed, 59 insertions(+), 26 deletions(-)
>> >
>> > diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
>> > index 7d17949..c701259 100644
>> > --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
>> > +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
>> > @@ -31,17 +31,20 @@ enum mediatek_gpio_reg {
>> >  	GPIO_REG_EDGE,
>> >  };
>> >  
>> > -static void __iomem *mediatek_gpio_membase;
>> > -static int mediatek_gpio_irq;
>> > -static struct irq_domain *mediatek_gpio_irq_domain;
>> > -
>> > -static struct mtk_gc {
>> > +struct mtk_gc {
>> >  	struct gpio_chip chip;
>> >  	spinlock_t lock;
>> >  	int bank;
>> >  	u32 rising;
>> >  	u32 falling;
>> > -} *gc_map[MTK_MAX_BANK];
>> > +};
>> > +
>> > +struct mtk_data {
>> > +	void __iomem *gpio_membase;
>> > +	int gpio_irq;
>> > +	struct irq_domain *gpio_irq_domain;
>> > +	struct mtk_gc *gc_map[MTK_MAX_BANK];
>> > +};
>> >  
>> >  static inline struct mtk_gc
>> >  *to_mediatek_gpio(struct gpio_chip *chip)
>> > @@ -56,15 +59,19 @@ static inline struct mtk_gc
>> >  static inline void
>> >  mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
>> >  {
>> > -	iowrite32(val, mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
>> > +	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
>> > +	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
>> > +
>> > +	iowrite32(val, gpio_data->gpio_membase + offset);
>> >  }
>> >  
>> >  static inline u32
>> >  mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
>> >  {
>> > +	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
>> >  	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
>> >  
>> > -	return ioread32(mediatek_gpio_membase + offset);
>> > +	return ioread32(gpio_data->gpio_membase + offset);
>> >  }
>> >  
>> >  static void
>> > @@ -137,23 +144,26 @@ mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>> >  static int
>> >  mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
>> >  {
>> > +	struct mtk_data *gpio_data = gpiochip_get_data(chip);
>> >  	struct mtk_gc *rg = to_mediatek_gpio(chip);
>> >  
>> > -	return irq_create_mapping(mediatek_gpio_irq_domain,
>> > +	return irq_create_mapping(gpio_data->gpio_irq_domain,
>> >  				  pin + (rg->bank * MTK_BANK_WIDTH));
>> >  }
>> >  
>> >  static int
>> >  mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>> >  {
>> > +	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
>> >  	const __be32 *id = of_get_property(bank, "reg", NULL);
>> >  	struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
>> >  				sizeof(struct mtk_gc), GFP_KERNEL);
>> > +	int ret;
>> >  
>> >  	if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
>> >  		return -ENOMEM;
>> >  
>> > -	gc_map[be32_to_cpu(*id)] = rg;
>> > +	gpio_data->gc_map[be32_to_cpu(*id)] = rg;
>> >  
>> >  	memset(rg, 0, sizeof(struct mtk_gc));
>> >  
>> > @@ -169,10 +179,17 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>> >  	rg->chip.get_direction = mediatek_gpio_get_direction;
>> >  	rg->chip.get = mediatek_gpio_get;
>> >  	rg->chip.set = mediatek_gpio_set;
>> > -	if (mediatek_gpio_irq_domain)
>> > +	if (gpio_data->gpio_irq_domain)
>> >  		rg->chip.to_irq = mediatek_gpio_to_irq;
>> >  	rg->bank = be32_to_cpu(*id);
>> >  
>> > +	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
>> > +	if (ret < 0) {
>> > +		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
>> > +			rg->chip.ngpio, ret);
>> > +		return ret;
>> > +	}
>> > +
>> 
>> Calling devm_gpiochip_add_data() here looks good.
>> Not removing
>> 	return gpiochip_add(&rg->chip);
>> from the end of the function isn't so good :-(
>
> True, thanks for pointing this out.
>
>> 
>> BTW interrupt definitely don't work yet.  The calls
>> 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> get NULL.  I think some sort of irq_set_chip_data(irq,...) is needed
>> in mediatek_gpio_gpio_map(), but it is too late at night for it to
>> all make sense to me :-)
>
> You are right. It seems irq_set_chip_data must be called in mediatek_gpio_gpio_map
> function.
>

I think you were a bit hasty here.
Yes, irq_set_chip_data() needs to be called there, but that isn't a
complete fix.
Rather than explain exactly the problem, I rather give you the
opportunity to look through the code and work out exactly what
is happening, and then see why it is wrong.
Interrupts are very close to working.  My patch to make them work is:
$ git diff --stat
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Let me know if you cannot find the problem.

BTW I don't know what Greg prefers, but my preference for this sort of
change is to just resend the problematic patch, as a reply to the
original.  It is easier to be sure that I didn't miss anything that way.
Maybe once you get Reviewed-by for all the patches you can resend
the complete series.
Up to you and Greg though.

Thanks,
NeilBrown

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 05/11] staging: mt7621-gpio: avoid use of globals and use platform_data instead

[Sergio Paracuellos]

On Sun, May 20, 2018 at 08:44:18AM +1000, NeilBrown wrote:
> On Sat, May 19 2018, Sergio Paracuellos wrote:
> 
> > On Sat, May 19, 2018 at 08:51:43PM +1000, NeilBrown wrote:
> >> On Wed, May 16 2018, Sergio Paracuellos wrote:
> >> 
> >> > Gpio driver have a some globals which can be avoided just
> >> > using platform_data in a proper form. This commit adds a new
> >> > struct mtk_data which includes all of those globals setting them
> >> > using platform_set_drvdata and devm_gpiochip_add_data functions.
> >> > With this properly set we are able to retrieve driver data along
> >> > the code using kernel api's so globals are not needed anymore.
> >> >
> >> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> >> > ---
> >> >  drivers/staging/mt7621-gpio/gpio-mt7621.c | 85 +++++++++++++++++++++----------
> >> >  1 file changed, 59 insertions(+), 26 deletions(-)
> >> >
> >> > diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> >> > index 7d17949..c701259 100644
> >> > --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> >> > +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> >> > @@ -31,17 +31,20 @@ enum mediatek_gpio_reg {
> >> >  	GPIO_REG_EDGE,
> >> >  };
> >> >  
> >> > -static void __iomem *mediatek_gpio_membase;
> >> > -static int mediatek_gpio_irq;
> >> > -static struct irq_domain *mediatek_gpio_irq_domain;
> >> > -
> >> > -static struct mtk_gc {
> >> > +struct mtk_gc {
> >> >  	struct gpio_chip chip;
> >> >  	spinlock_t lock;
> >> >  	int bank;
> >> >  	u32 rising;
> >> >  	u32 falling;
> >> > -} *gc_map[MTK_MAX_BANK];
> >> > +};
> >> > +
> >> > +struct mtk_data {
> >> > +	void __iomem *gpio_membase;
> >> > +	int gpio_irq;
> >> > +	struct irq_domain *gpio_irq_domain;
> >> > +	struct mtk_gc *gc_map[MTK_MAX_BANK];
> >> > +};
> >> >  
> >> >  static inline struct mtk_gc
> >> >  *to_mediatek_gpio(struct gpio_chip *chip)
> >> > @@ -56,15 +59,19 @@ static inline struct mtk_gc
> >> >  static inline void
> >> >  mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
> >> >  {
> >> > -	iowrite32(val, mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
> >> > +	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> >> > +	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> >> > +
> >> > +	iowrite32(val, gpio_data->gpio_membase + offset);
> >> >  }
> >> >  
> >> >  static inline u32
> >> >  mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
> >> >  {
> >> > +	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> >> >  	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> >> >  
> >> > -	return ioread32(mediatek_gpio_membase + offset);
> >> > +	return ioread32(gpio_data->gpio_membase + offset);
> >> >  }
> >> >  
> >> >  static void
> >> > @@ -137,23 +144,26 @@ mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> >> >  static int
> >> >  mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
> >> >  {
> >> > +	struct mtk_data *gpio_data = gpiochip_get_data(chip);
> >> >  	struct mtk_gc *rg = to_mediatek_gpio(chip);
> >> >  
> >> > -	return irq_create_mapping(mediatek_gpio_irq_domain,
> >> > +	return irq_create_mapping(gpio_data->gpio_irq_domain,
> >> >  				  pin + (rg->bank * MTK_BANK_WIDTH));
> >> >  }
> >> >  
> >> >  static int
> >> >  mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> >> >  {
> >> > +	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
> >> >  	const __be32 *id = of_get_property(bank, "reg", NULL);
> >> >  	struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
> >> >  				sizeof(struct mtk_gc), GFP_KERNEL);
> >> > +	int ret;
> >> >  
> >> >  	if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
> >> >  		return -ENOMEM;
> >> >  
> >> > -	gc_map[be32_to_cpu(*id)] = rg;
> >> > +	gpio_data->gc_map[be32_to_cpu(*id)] = rg;
> >> >  
> >> >  	memset(rg, 0, sizeof(struct mtk_gc));
> >> >  
> >> > @@ -169,10 +179,17 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> >> >  	rg->chip.get_direction = mediatek_gpio_get_direction;
> >> >  	rg->chip.get = mediatek_gpio_get;
> >> >  	rg->chip.set = mediatek_gpio_set;
> >> > -	if (mediatek_gpio_irq_domain)
> >> > +	if (gpio_data->gpio_irq_domain)
> >> >  		rg->chip.to_irq = mediatek_gpio_to_irq;
> >> >  	rg->bank = be32_to_cpu(*id);
> >> >  
> >> > +	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> >> > +	if (ret < 0) {
> >> > +		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
> >> > +			rg->chip.ngpio, ret);
> >> > +		return ret;
> >> > +	}
> >> > +
> >> 
> >> Calling devm_gpiochip_add_data() here looks good.
> >> Not removing
> >> 	return gpiochip_add(&rg->chip);
> >> from the end of the function isn't so good :-(
> >
> > True, thanks for pointing this out.
> >
> >> 
> >> BTW interrupt definitely don't work yet.  The calls
> >> 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> >> get NULL.  I think some sort of irq_set_chip_data(irq,...) is needed
> >> in mediatek_gpio_gpio_map(), but it is too late at night for it to
> >> all make sense to me :-)
> >
> > You are right. It seems irq_set_chip_data must be called in mediatek_gpio_gpio_map
> > function.
> >
> 
> I think you were a bit hasty here.
> Yes, irq_set_chip_data() needs to be called there, but that isn't a
> complete fix.
> Rather than explain exactly the problem, I rather give you the
> opportunity to look through the code and work out exactly what
> is happening, and then see why it is wrong.
> Interrupts are very close to working.  My patch to make them work is:
> $ git diff --stat
>  drivers/staging/mt7621-gpio/gpio-mt7621.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> Let me know if you cannot find the problem.
> 

You are totally right. Retrieved pointers in irq related functions
weren't the correct ones. Also gpio_data wasn't being passed into
irq_domain_add_linear where NULL was being passed instead. Hopefully
the next one is the good one :-).

> BTW I don't know what Greg prefers, but my preference for this sort of
> change is to just resend the problematic patch, as a reply to the
> original.  It is easier to be sure that I didn't miss anything that way.
> Maybe once you get Reviewed-by for all the patches you can resend
> the complete series.
> Up to you and Greg though.

I think is better to send v5 of whole series but before that I will send
only a PATCH made against v4 of PATCH 5 to your better review. 

> 
> Thanks,
> NeilBrown

Thanks,
    Sergio Paracuellos


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: mt7621-gpio: retrieve correct pointers in interrupt related functions

[Sergio Paracuellos]

The data passed between irq related functions and the ones which have
been retrieved where different. Also first data haven't properly
set on irq_domain_add_linear call where it was passing NULL instead.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>
Tested-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 650286df..077a7c2 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -201,8 +201,7 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 static void
 mediatek_gpio_irq_handler(struct irq_desc *desc)
 {
-	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
-	struct mtk_data *gpio_data = gpiochip_get_data(gc);
+	struct mtk_data *gpio_data = irq_desc_get_handler_data(desc);
 	int i;
 
 	for (i = 0; i < MTK_MAX_BANK; i++) {
@@ -228,8 +227,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
 static void
 mediatek_gpio_irq_unmask(struct irq_data *d)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct mtk_data *gpio_data = gpiochip_get_data(gc);
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
 	int bank = pin / 32;
 	struct mtk_gc *rg = gpio_data->gc_map[bank];
@@ -251,8 +249,7 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 static void
 mediatek_gpio_irq_mask(struct irq_data *d)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct mtk_data *gpio_data = gpiochip_get_data(gc);
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
 	int bank = pin / 32;
 	struct mtk_gc *rg = gpio_data->gc_map[bank];
@@ -274,8 +271,7 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 static int
 mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct mtk_data *gpio_data = gpiochip_get_data(gc);
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
 	int bank = pin / 32;
 	struct mtk_gc *rg = gpio_data->gc_map[bank];
@@ -352,7 +348,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
 	if (gpio_data->gpio_irq) {
 		gpio_data->gpio_irq_domain = irq_domain_add_linear(np,
 			MTK_MAX_BANK * MTK_BANK_WIDTH,
-			&irq_domain_ops, NULL);
+			&irq_domain_ops, gpio_data);
 		if (!gpio_data->gpio_irq_domain)
 			dev_err(&pdev->dev, "irq_domain_add_linear failed\n");
 	}
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: mt7621-gpio: retrieve correct pointers in interrupt related functions

[NeilBrown]

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

On Sun, May 20 2018, Sergio Paracuellos wrote:

> The data passed between irq related functions and the ones which have
> been retrieved where different. Also first data haven't properly
> set on irq_domain_add_linear call where it was passing NULL instead.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Reviewed-by: NeilBrown <neil@brown.name>
Tested-by: NeilBrown <neil@brown.name>

:-)

Thanks,
NeilBrown


> ---
>  drivers/staging/mt7621-gpio/gpio-mt7621.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index 650286df..077a7c2 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -201,8 +201,7 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  static void
>  mediatek_gpio_irq_handler(struct irq_desc *desc)
>  {
> -	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> -	struct mtk_data *gpio_data = gpiochip_get_data(gc);
> +	struct mtk_data *gpio_data = irq_desc_get_handler_data(desc);
>  	int i;
>  
>  	for (i = 0; i < MTK_MAX_BANK; i++) {
> @@ -228,8 +227,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
>  static void
>  mediatek_gpio_irq_unmask(struct irq_data *d)
>  {
> -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> -	struct mtk_data *gpio_data = gpiochip_get_data(gc);
> +	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
>  	int pin = d->hwirq;
>  	int bank = pin / 32;
>  	struct mtk_gc *rg = gpio_data->gc_map[bank];
> @@ -251,8 +249,7 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
>  static void
>  mediatek_gpio_irq_mask(struct irq_data *d)
>  {
> -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> -	struct mtk_data *gpio_data = gpiochip_get_data(gc);
> +	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
>  	int pin = d->hwirq;
>  	int bank = pin / 32;
>  	struct mtk_gc *rg = gpio_data->gc_map[bank];
> @@ -274,8 +271,7 @@ mediatek_gpio_irq_mask(struct irq_data *d)
>  static int
>  mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
>  {
> -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> -	struct mtk_data *gpio_data = gpiochip_get_data(gc);
> +	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
>  	int pin = d->hwirq;
>  	int bank = pin / 32;
>  	struct mtk_gc *rg = gpio_data->gc_map[bank];
> @@ -352,7 +348,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
>  	if (gpio_data->gpio_irq) {
>  		gpio_data->gpio_irq_domain = irq_domain_add_linear(np,
>  			MTK_MAX_BANK * MTK_BANK_WIDTH,
> -			&irq_domain_ops, NULL);
> +			&irq_domain_ops, gpio_data);
>  		if (!gpio_data->gpio_irq_domain)
>  			dev_err(&pdev->dev, "irq_domain_add_linear failed\n");
>  	}
> -- 
> 2.7.4

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] staging: mt7621-gpio: retrieve correct pointers in interrupt related functions

[Sergio Paracuellos]

On Sun, May 20, 2018 at 07:53:08PM +1000, NeilBrown wrote:
> On Sun, May 20 2018, Sergio Paracuellos wrote:
> 
> > The data passed between irq related functions and the ones which have
> > been retrieved where different. Also first data haven't properly
> > set on irq_domain_add_linear call where it was passing NULL instead.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> 
> Reviewed-by: NeilBrown <neil@brown.name>
> Tested-by: NeilBrown <neil@brown.name>
> 
> :-)
> 
> Thanks,
> NeilBrown

Thanks, Neil. 

So I'll send v5 of this series to make things easier. 
I'll join this PATCH with PATCH 5 and make TODO file
empty.

What is missing to make this driver out of staging after this changes?

Should it be moved or you were thinking in move all the mt7621 together?

Best regards,
    Sergio Paracuellos

> 
> 
> > ---
> >  drivers/staging/mt7621-gpio/gpio-mt7621.c | 14 +++++---------
> >  1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > index 650286df..077a7c2 100644
> > --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > @@ -201,8 +201,7 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> >  static void
> >  mediatek_gpio_irq_handler(struct irq_desc *desc)
> >  {
> > -	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> > -	struct mtk_data *gpio_data = gpiochip_get_data(gc);
> > +	struct mtk_data *gpio_data = irq_desc_get_handler_data(desc);
> >  	int i;
> >  
> >  	for (i = 0; i < MTK_MAX_BANK; i++) {
> > @@ -228,8 +227,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
> >  static void
> >  mediatek_gpio_irq_unmask(struct irq_data *d)
> >  {
> > -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > -	struct mtk_data *gpio_data = gpiochip_get_data(gc);
> > +	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
> >  	int pin = d->hwirq;
> >  	int bank = pin / 32;
> >  	struct mtk_gc *rg = gpio_data->gc_map[bank];
> > @@ -251,8 +249,7 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
> >  static void
> >  mediatek_gpio_irq_mask(struct irq_data *d)
> >  {
> > -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > -	struct mtk_data *gpio_data = gpiochip_get_data(gc);
> > +	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
> >  	int pin = d->hwirq;
> >  	int bank = pin / 32;
> >  	struct mtk_gc *rg = gpio_data->gc_map[bank];
> > @@ -274,8 +271,7 @@ mediatek_gpio_irq_mask(struct irq_data *d)
> >  static int
> >  mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
> >  {
> > -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > -	struct mtk_data *gpio_data = gpiochip_get_data(gc);
> > +	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
> >  	int pin = d->hwirq;
> >  	int bank = pin / 32;
> >  	struct mtk_gc *rg = gpio_data->gc_map[bank];
> > @@ -352,7 +348,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
> >  	if (gpio_data->gpio_irq) {
> >  		gpio_data->gpio_irq_domain = irq_domain_add_linear(np,
> >  			MTK_MAX_BANK * MTK_BANK_WIDTH,
> > -			&irq_domain_ops, NULL);
> > +			&irq_domain_ops, gpio_data);
> >  		if (!gpio_data->gpio_irq_domain)
> >  			dev_err(&pdev->dev, "irq_domain_add_linear failed\n");
> >  	}
> > -- 
> > 2.7.4


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 00/10] staging: mt7621-gpio: use mediatek as binding instead of custom mtk

[Sergio Paracuellos]

The following series updated mt7621-gpio driver to use 'mediatek'
which is already defined in kernel bindings documentation instead
of add a new custom one 'mtk' for this company. mt7621-gpio device-tree
documentation has been added also to the staging driver directory.

v5:
 - PATCH 5 ("staging: mt7621-gpio: avoid use of globals and use
   platform_data instead") was still missing folowings to be 
   properly working:
    + The data passed between irq related functions and the ones 
      which have been retrieved were different. Also first data haven't 
      properly set on irq_domain_add_linear call where it was passing 
      NULL instead. Fix those.
 - Join TODO's modifications patches (PATCH 1 and 11) in one which just 
   makes empty the file.

v4:
 - Only PATCH 5 ("staging: mt7621-gpio: avoid use of globals and use
   platform_data instead") has changes:
    + avoid call to gpiochip_add in probe function because function
      devm_gpiochip_add_data is being used instead.
    + Call irq_set_chip_data in mediatek_gpio_gpio_map to make data
      properly passed to interrupts related functions.

v3:
 - Fix condition for check for a valid gpio id in driver probe function
   included in the PATCH 8 ("staging: mt7621-gpio: avoid devm_kzalloc()
   hidden inside declarations and refactor function a bit") of the series.
 - Rest of patches are not modified at all.

v2:
 - Join some patch ("staging: mt7621-gpio: avoid use of globals
   and use platform_data instead") with the needed fixes to make
   it work pointed out by NeilBrown
 - Add gpio interrupts to documentation and device tree in order to
   can try them
 - Other minor code cleanups have been added also.

This changes have been reviewed and tested by NeilBrown but I don't
know if I have to add those information to the patches by myself if
I resend the series or should be added by you Greg when the patches 
are added to the kernel tree.

Hope this helps.

Best regards,
     Sergio Paracuellos


Sergio Paracuellos (10):
  staging: mt7621-gpio: dt-bindings: add documentation for mt7621-gpio
  staging: mt7621-dts: update gpios related entries to use 'mediatek'
  staging: mt7621-gpio: replace 'mtk' to use correct one 'mediatek'
  staging: mt7621-gpio: avoid use of globals and use platform_data
    instead
  staging: mt7621-dts: add interrupt device tree nodes for the gpio
    controller
  staging: mt7621-gpio: dt-bindings: add interrupt nodes to bindings doc
  staging: mt7621-gpio: avoid devm_kzalloc() hidden inside declarations
    and refactor function a bit
  staging: mt7621-gpio: use ternary operator in return in
    mediatek_gpio_get_direction
  staging: mt7621-gpio: use MTK_BANK_WIDTH instead of magic number
  staging: mt7621-gpio: update TODO file

 drivers/staging/mt7621-dts/mt7621.dtsi             |  11 +-
 drivers/staging/mt7621-gpio/TODO                   |   3 -
 drivers/staging/mt7621-gpio/gpio-mt7621.c          | 116 +++++++++++++--------
 .../staging/mt7621-gpio/mediatek,mt7621-gpio.txt   |  58 +++++++++++
 4 files changed, 139 insertions(+), 49 deletions(-)
 create mode 100644 drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt

-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 01/10] staging: mt7621-gpio: dt-bindings: add documentation for mt7621-gpio

[Sergio Paracuellos]

This commit add missing dt bindings documentation for mt7621-gpio
driver. There is some missing stuff here about interrupts with is
not also being used in the mt7621.dtsi file. So just include in
staging a incomplete version before moving this to kernel's dt-bindings
place.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 .../staging/mt7621-gpio/mediatek,mt7621-gpio.txt   | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt

diff --git a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
new file mode 100644
index 0000000..54de9f7
--- /dev/null
+++ b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
@@ -0,0 +1,51 @@
+Mediatek SoC GPIO controller bindings
+
+The IP core used inside these SoCs has 3 banks of 32 GPIOs each.
+The registers of all the banks are interwoven inside one single IO range.
+We load one GPIO controller instance per bank. To make this possible
+we support 2 types of nodes. The parent node defines the memory I/O range and
+has 3 children each describing a single bank.
+
+Required properties for the top level node:
+- compatible:
+  - "mediatek,mt7621-gpio" for Mediatek controllers
+- reg : Physical base address and length of the controller's registers
+
+Required properties for the GPIO bank node:
+- compatible:
+  - "mediatek,mt7621-gpio-bank" for Mediatek banks
+- #gpio-cells : Should be two.
+  - first cell is the pin number
+  - second cell is used to specify optional parameters (unused)
+- gpio-controller : Marks the device node as a GPIO controller
+- reg : The id of the bank that the node describes.
+
+Example:
+	gpio@600 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		compatible = "mediatek,mt7621-gpio";
+		reg = <0x600 0x100>;
+
+		gpio0: bank@0 {
+			reg = <0>;
+			compatible = "mediatek,mt7621-gpio-bank";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+
+		gpio1: bank@1 {
+			reg = <1>;
+			compatible = "mediatek,mt7621-gpio-bank";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+
+		gpio2: bank@2 {
+			reg = <2>;
+			compatible = "mediatek,mt7621-gpio-bank";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+	};
-- 
2.7.4

[PATCH v5 02/10] staging: mt7621-dts: update gpios related entries to use 'mediatek'

[Sergio Paracuellos]

Gpio driver for mt7621 is using 'mtk' as binding but in the kernel
is already defined one for this maker which is 'mediatek'. Update
device tree to use the correct one.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-dts/mt7621.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index 9d941b5..115eb04 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -64,26 +64,26 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-			compatible = "mtk,mt7621-gpio";
+			compatible = "mediatek,mt7621-gpio";
 			reg = <0x600 0x100>;
 
 			gpio0: bank@0 {
 				reg = <0>;
-				compatible = "mtk,mt7621-gpio-bank";
+				compatible = "mediatek,mt7621-gpio-bank";
 				gpio-controller;
 				#gpio-cells = <2>;
 			};
 
 			gpio1: bank@1 {
 				reg = <1>;
-				compatible = "mtk,mt7621-gpio-bank";
+				compatible = "mediatek,mt7621-gpio-bank";
 				gpio-controller;
 				#gpio-cells = <2>;
 			};
 
 			gpio2: bank@2 {
 				reg = <2>;
-				compatible = "mtk,mt7621-gpio-bank";
+				compatible = "mediatek,mt7621-gpio-bank";
 				gpio-controller;
 				#gpio-cells = <2>;
 			};
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 03/10] staging: mt7621-gpio: replace 'mtk' to use correct one 'mediatek'

[Sergio Paracuellos]

Gpio driver is using mtk and there is already 'mediatek' binding
defined for this maker. Update driver to use it instead the custom
one 'mtk'.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index a577381..7d17949 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -323,7 +323,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
 	}
 
 	for_each_child_of_node(np, bank)
-		if (of_device_is_compatible(bank, "mtk,mt7621-gpio-bank"))
+		if (of_device_is_compatible(bank, "mediatek,mt7621-gpio-bank"))
 			mediatek_gpio_bank_probe(pdev, bank);
 
 	if (mediatek_gpio_irq_domain)
@@ -334,7 +334,7 @@ mediatek_gpio_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id mediatek_gpio_match[] = {
-	{ .compatible = "mtk,mt7621-gpio" },
+	{ .compatible = "mediatek,mt7621-gpio" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, mediatek_gpio_match);
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 04/10] staging: mt7621-gpio: avoid use of globals and use platform_data instead

[Sergio Paracuellos]

Gpio driver have a some globals which can be avoided just
using platform_data in a proper form. This commit adds a new
struct mtk_data which includes all of those globals setting them
using platform_set_drvdata and devm_gpiochip_add_data functions.
With this properly set we are able to retrieve driver data along
the code using kernel api's so globals are not needed anymore.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 90 +++++++++++++++++++++----------
 1 file changed, 62 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 7d17949..077a7c2 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -31,17 +31,20 @@ enum mediatek_gpio_reg {
 	GPIO_REG_EDGE,
 };
 
-static void __iomem *mediatek_gpio_membase;
-static int mediatek_gpio_irq;
-static struct irq_domain *mediatek_gpio_irq_domain;
-
-static struct mtk_gc {
+struct mtk_gc {
 	struct gpio_chip chip;
 	spinlock_t lock;
 	int bank;
 	u32 rising;
 	u32 falling;
-} *gc_map[MTK_MAX_BANK];
+};
+
+struct mtk_data {
+	void __iomem *gpio_membase;
+	int gpio_irq;
+	struct irq_domain *gpio_irq_domain;
+	struct mtk_gc *gc_map[MTK_MAX_BANK];
+};
 
 static inline struct mtk_gc
 *to_mediatek_gpio(struct gpio_chip *chip)
@@ -56,15 +59,19 @@ static inline struct mtk_gc
 static inline void
 mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
 {
-	iowrite32(val, mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
+	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
+	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
+
+	iowrite32(val, gpio_data->gpio_membase + offset);
 }
 
 static inline u32
 mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
 {
+	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
 	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
 
-	return ioread32(mediatek_gpio_membase + offset);
+	return ioread32(gpio_data->gpio_membase + offset);
 }
 
 static void
@@ -137,23 +144,26 @@ mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 static int
 mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
 {
+	struct mtk_data *gpio_data = gpiochip_get_data(chip);
 	struct mtk_gc *rg = to_mediatek_gpio(chip);
 
-	return irq_create_mapping(mediatek_gpio_irq_domain,
+	return irq_create_mapping(gpio_data->gpio_irq_domain,
 				  pin + (rg->bank * MTK_BANK_WIDTH));
 }
 
 static int
 mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 {
+	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
 	const __be32 *id = of_get_property(bank, "reg", NULL);
 	struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
 				sizeof(struct mtk_gc), GFP_KERNEL);
+	int ret;
 
 	if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
 		return -ENOMEM;
 
-	gc_map[be32_to_cpu(*id)] = rg;
+	gpio_data->gc_map[be32_to_cpu(*id)] = rg;
 
 	memset(rg, 0, sizeof(struct mtk_gc));
 
@@ -169,25 +179,33 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 	rg->chip.get_direction = mediatek_gpio_get_direction;
 	rg->chip.get = mediatek_gpio_get;
 	rg->chip.set = mediatek_gpio_set;
-	if (mediatek_gpio_irq_domain)
+	if (gpio_data->gpio_irq_domain)
 		rg->chip.to_irq = mediatek_gpio_to_irq;
 	rg->bank = be32_to_cpu(*id);
 
+	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
+			rg->chip.ngpio, ret);
+		return ret;
+	}
+
 	/* set polarity to low for all gpios */
 	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
 
 	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
 
-	return gpiochip_add(&rg->chip);
+	return 0;
 }
 
 static void
 mediatek_gpio_irq_handler(struct irq_desc *desc)
 {
+	struct mtk_data *gpio_data = irq_desc_get_handler_data(desc);
 	int i;
 
 	for (i = 0; i < MTK_MAX_BANK; i++) {
-		struct mtk_gc *rg = gc_map[i];
+		struct mtk_gc *rg = gpio_data->gc_map[i];
 		unsigned long pending;
 		int bit;
 
@@ -197,7 +215,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
 		pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
 
 		for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
-			u32 map = irq_find_mapping(mediatek_gpio_irq_domain,
+			u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
 						   (MTK_BANK_WIDTH * i) + bit);
 
 			generic_handle_irq(map);
@@ -209,9 +227,10 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
 static void
 mediatek_gpio_irq_unmask(struct irq_data *d)
 {
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
 	int bank = pin / 32;
-	struct mtk_gc *rg = gc_map[bank];
+	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
 
@@ -230,9 +249,10 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 static void
 mediatek_gpio_irq_mask(struct irq_data *d)
 {
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
 	int bank = pin / 32;
-	struct mtk_gc *rg = gc_map[bank];
+	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
 
@@ -251,9 +271,10 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 static int
 mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 {
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
 	int bank = pin / 32;
-	struct mtk_gc *rg = gc_map[bank];
+	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	u32 mask = BIT(d->hwirq);
 
 	if (!rg)
@@ -291,6 +312,11 @@ static int
 mediatek_gpio_gpio_map(struct irq_domain *d, unsigned int irq,
 		       irq_hw_number_t hw)
 {
+	int ret;
+
+	ret = irq_set_chip_data(irq, d->host_data);
+	if (ret < 0)
+		return ret;
 	irq_set_chip_and_handler(irq, &mediatek_gpio_irq_chip,
 				 handle_level_irq);
 	irq_set_handler_data(irq, d);
@@ -308,27 +334,35 @@ mediatek_gpio_probe(struct platform_device *pdev)
 {
 	struct device_node *bank, *np = pdev->dev.of_node;
 	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct mtk_data *gpio_data;
 
-	mediatek_gpio_membase = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(mediatek_gpio_membase))
-		return PTR_ERR(mediatek_gpio_membase);
+	gpio_data = devm_kzalloc(&pdev->dev, sizeof(*gpio_data), GFP_KERNEL);
+	if (!gpio_data)
+		return -ENOMEM;
+
+	gpio_data->gpio_membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(gpio_data->gpio_membase))
+		return PTR_ERR(gpio_data->gpio_membase);
 
-	mediatek_gpio_irq = irq_of_parse_and_map(np, 0);
-	if (mediatek_gpio_irq) {
-		mediatek_gpio_irq_domain = irq_domain_add_linear(np,
+	gpio_data->gpio_irq = irq_of_parse_and_map(np, 0);
+	if (gpio_data->gpio_irq) {
+		gpio_data->gpio_irq_domain = irq_domain_add_linear(np,
 			MTK_MAX_BANK * MTK_BANK_WIDTH,
-			&irq_domain_ops, NULL);
-		if (!mediatek_gpio_irq_domain)
+			&irq_domain_ops, gpio_data);
+		if (!gpio_data->gpio_irq_domain)
 			dev_err(&pdev->dev, "irq_domain_add_linear failed\n");
 	}
 
+	platform_set_drvdata(pdev, gpio_data);
+
 	for_each_child_of_node(np, bank)
 		if (of_device_is_compatible(bank, "mediatek,mt7621-gpio-bank"))
 			mediatek_gpio_bank_probe(pdev, bank);
 
-	if (mediatek_gpio_irq_domain)
-		irq_set_chained_handler(mediatek_gpio_irq,
-					mediatek_gpio_irq_handler);
+	if (gpio_data->gpio_irq_domain)
+		irq_set_chained_handler_and_data(gpio_data->gpio_irq,
+						 mediatek_gpio_irq_handler,
+						 gpio_data);
 
 	return 0;
 }
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 05/10] staging: mt7621-dts: add interrupt device tree nodes for the gpio controller

[Sergio Paracuellos]

The GPIO controller of mt7621 can receive interrupts on any
of the GPIOs, either edge or level. It then interrupts the CPU using
GIC INT12. Update device tree accordly.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-dts/mt7621.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index 115eb04..240d396 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -67,6 +67,9 @@
 			compatible = "mediatek,mt7621-gpio";
 			reg = <0x600 0x100>;
 
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
+
 			gpio0: bank@0 {
 				reg = <0>;
 				compatible = "mediatek,mt7621-gpio-bank";
-- 
2.7.4

[PATCH v5 06/10] staging: mt7621-gpio: dt-bindings: add interrupt nodes to bindings doc

[Sergio Paracuellos]

Interrupt related stuff for gpio controller in mt7621 was missing
in device tree documentation. Add it to complete documentation for
this driver.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
index 54de9f7..af64092 100644
--- a/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
+++ b/drivers/staging/mt7621-gpio/mediatek,mt7621-gpio.txt
@@ -4,12 +4,16 @@ The IP core used inside these SoCs has 3 banks of 32 GPIOs each.
 The registers of all the banks are interwoven inside one single IO range.
 We load one GPIO controller instance per bank. To make this possible
 we support 2 types of nodes. The parent node defines the memory I/O range and
-has 3 children each describing a single bank.
+has 3 children each describing a single bank. Also the GPIO controller can receive
+interrupts on any of the GPIOs, either edge or level. It then interrupts the CPU
+using GIC INT12.
 
 Required properties for the top level node:
 - compatible:
   - "mediatek,mt7621-gpio" for Mediatek controllers
 - reg : Physical base address and length of the controller's registers
+- interrupt-parent : phandle of the parent interrupt controller.
+- interrupts = Interrupt specifier for the controllers interrupt
 
 Required properties for the GPIO bank node:
 - compatible:
@@ -28,6 +32,9 @@ Example:
 		compatible = "mediatek,mt7621-gpio";
 		reg = <0x600 0x100>;
 
+		interrupt-parent = <&gic>;
+		interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
+
 		gpio0: bank@0 {
 			reg = <0>;
 			compatible = "mediatek,mt7621-gpio-bank";
-- 
2.7.4

[PATCH v5 07/10] staging: mt7621-gpio: avoid devm_kzalloc() hidden inside declarations and refactor function a bit

[Sergio Paracuellos]

Driver probe function includes an allocation using devm_kzalloc
which is "hidden" a bit inside the declarations. Extract this
to a better place to increase readability. Also because we are
allocating zeroed memory 'memset' statement is not needed at all.
Condition for checking for a valid gpio id is wrong and it should
be greater or equal instead of only greater so update to be the
good one.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 077a7c2..bbe6cce 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -156,17 +156,18 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
 {
 	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
 	const __be32 *id = of_get_property(bank, "reg", NULL);
-	struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
-				sizeof(struct mtk_gc), GFP_KERNEL);
+	struct mtk_gc *rg;
 	int ret;
 
-	if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
+	if (!id || be32_to_cpu(*id) >= MTK_MAX_BANK)
+		return -EINVAL;
+
+	rg = devm_kzalloc(&pdev->dev, sizeof(struct mtk_gc), GFP_KERNEL);
+	if (!rg)
 		return -ENOMEM;
 
 	gpio_data->gc_map[be32_to_cpu(*id)] = rg;
 
-	memset(rg, 0, sizeof(struct mtk_gc));
-
 	spin_lock_init(&rg->lock);
 
 	rg->chip.parent = &pdev->dev;
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 08/10] staging: mt7621-gpio: use ternary operator in return in mediatek_gpio_get_direction

[Sergio Paracuellos]

This commits replaces if statement and two returns in favour
of a only one return using a ternary operator.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index bbe6cce..15a1003 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -135,10 +135,7 @@ mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 	t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
 	spin_unlock_irqrestore(&rg->lock, flags);
 
-	if (t & BIT(offset))
-		return 0;
-
-	return 1;
+	return (t & BIT(offset)) ? 0 : 1;
 }
 
 static int
-- 
2.7.4

[PATCH v5 09/10] staging: mt7621-gpio: use MTK_BANK_WIDTH instead of magic number

[Sergio Paracuellos]

There are some places where magic number '32' is being used to get
the gpio bank. There already exist a definition MTK_BANK_WIDTH
with this value, so just use it instead.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
index 15a1003..5d923a7 100644
--- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
+++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
@@ -227,7 +227,7 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
 {
 	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
-	int bank = pin / 32;
+	int bank = pin / MTK_BANK_WIDTH;
 	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
@@ -249,7 +249,7 @@ mediatek_gpio_irq_mask(struct irq_data *d)
 {
 	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
-	int bank = pin / 32;
+	int bank = pin / MTK_BANK_WIDTH;
 	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	unsigned long flags;
 	u32 rise, fall;
@@ -271,7 +271,7 @@ mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
 {
 	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
 	int pin = d->hwirq;
-	int bank = pin / 32;
+	int bank = pin / MTK_BANK_WIDTH;
 	struct mtk_gc *rg = gpio_data->gc_map[bank];
 	u32 mask = BIT(d->hwirq);
 
-- 
2.7.4

[PATCH v5 10/10] staging: mt7621-gpio: update TODO file

[Sergio Paracuellos]

Remaining stuff included in TODO list have been
complete. So update this file accordly.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-gpio/TODO | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/mt7621-gpio/TODO b/drivers/staging/mt7621-gpio/TODO
index 7143905..2f92cdb 100644
--- a/drivers/staging/mt7621-gpio/TODO
+++ b/drivers/staging/mt7621-gpio/TODO
@@ -1,5 +1,2 @@
 
-- general code review and clean up
-- ensure device-tree requirements are documented
-
 Cc:  NeilBrown <neil@brown.name>
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: mt7621-gpio: retrieve correct pointers in interrupt related functions

[NeilBrown]

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

On Sun, May 20 2018, Sergio Paracuellos wrote:

> On Sun, May 20, 2018 at 07:53:08PM +1000, NeilBrown wrote:
>> On Sun, May 20 2018, Sergio Paracuellos wrote:
>> 
>> > The data passed between irq related functions and the ones which have
>> > been retrieved where different. Also first data haven't properly
>> > set on irq_domain_add_linear call where it was passing NULL instead.
>> >
>> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>> 
>> Reviewed-by: NeilBrown <neil@brown.name>
>> Tested-by: NeilBrown <neil@brown.name>
>> 
>> :-)
>> 
>> Thanks,
>> NeilBrown
>
> Thanks, Neil. 
>
> So I'll send v5 of this series to make things easier. 
> I'll join this PATCH with PATCH 5 and make TODO file
> empty.
>
> What is missing to make this driver out of staging after this changes?

Uhmmm.. I depends on how fussy the gpio maintainer is.
I think the code is pretty good, but there are probably things that
could be improved.
Looking through the code again with my picky-maintainer spectacles on:

- we could probably use module_platform_driver() instead of
  subsys_initcall().
- The various
	u32 mask = BIT(d->hwirq);
  calls look wrong.  hwirq can be as large as 95, and
     1UL << 95
  is unlikely to work well.  I cannot test with a gpio above
  32, but I suspect it wouldn't work.
  These should use something like
      #define PIN_MASK(nr) (1UL << ((nr % MTK_BANK_WIDTH)))

- Locking is a bit weird.  It makes sense to hold the lock across a
  read-modify-write like in mediatek_gpio_direction_input(), but
  holding across a simple read (e.g. mediatec_gpio_get_direction())
  doesn't make much sense, and holding only for the 'write' part
  of a read-modify-write (e.g. mediateck_gpio_irq_umask()) is down
  right weird.

- And now for my pet peeve.
  There are 3 banks - right?  And they are numbered '0' and '1' and
  '2'. Agreed?
  So what is the Maximum bank number?  I think it is "2".
  "3" is the number of banks, or the count of banks.
  Yet we have
  #define MTK_MAX_BANK		3

  claiming that the maximum bank number is 3, which it isn't.
  Dan Carpenter recently guided you to fix

	if (!id || be32_to_cpu(*id) >= MTK_MAX_BANK)
		return -EINVAL;
  which was
	if (!id || be32_to_cpu(*id) > MTK_MAX_BANK)
		return -EINVAL;

  The latter looks right because if something is bigger than the maximum,
  it is obviously a problem, but if it equals the maximum, it should be
  fine.  So the code looked right, but was wrong.  You changed it so
  that it looks wrong, but is right.
  I would prefer to get both - it should look right and be right.
  I would suggest changing MTK_MAX_BANK to MTK_BANK_CNT - which
  means the right thing, and has a name structure similar to
  MTK_BANK_WIDTH.

Now looking at the devicetree binding description:

- #gpio-cells : Should be two.
  - first cell is the pin number
  - second cell is used to specify optional parameters (unused)

I think the second cell can be GPIO_ACTIVE_HIGH or GPIO_ACTIVE_LOW and I
think that is used.
Also I think you would typically put
     interrupt-controller;
     #interrupt-cells = <1>;
in the there so that other devices can reference the GPIO interrupts
if necessary.



Once all that has landed in staging and I've done one final test, I
think it will be ready to submit to linux-gpio and the device-tree list.

>
> Should it be moved or you were thinking in move all the mt7621
> together?

No, I can see no need to keep them together.


Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] staging: mt7621-gpio: retrieve correct pointers in interrupt related functions

[Greg KH]

On Sun, May 20, 2018 at 12:18:23PM +0200, Sergio Paracuellos wrote:
> On Sun, May 20, 2018 at 07:53:08PM +1000, NeilBrown wrote:
> > On Sun, May 20 2018, Sergio Paracuellos wrote:
> > 
> > > The data passed between irq related functions and the ones which have
> > > been retrieved where different. Also first data haven't properly
> > > set on irq_domain_add_linear call where it was passing NULL instead.
> > >
> > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > 
> > Reviewed-by: NeilBrown <neil@brown.name>
> > Tested-by: NeilBrown <neil@brown.name>
> > 
> > :-)
> > 
> > Thanks,
> > NeilBrown
> 
> Thanks, Neil. 
> 
> So I'll send v5 of this series to make things easier. 
> I'll join this PATCH with PATCH 5 and make TODO file
> empty.


Ugh, I'm totally confused now.  What is the "correct" patch series to
consider for merging here?  v5?  Something else?

lost,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: mt7621-gpio: retrieve correct pointers in interrupt related functions

[Greg KH]

On Sun, May 20, 2018 at 02:38:09PM +0200, Greg KH wrote:
> On Sun, May 20, 2018 at 12:18:23PM +0200, Sergio Paracuellos wrote:
> > On Sun, May 20, 2018 at 07:53:08PM +1000, NeilBrown wrote:
> > > On Sun, May 20 2018, Sergio Paracuellos wrote:
> > > 
> > > > The data passed between irq related functions and the ones which have
> > > > been retrieved where different. Also first data haven't properly
> > > > set on irq_domain_add_linear call where it was passing NULL instead.
> > > >
> > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > 
> > > Reviewed-by: NeilBrown <neil@brown.name>
> > > Tested-by: NeilBrown <neil@brown.name>
> > > 
> > > :-)
> > > 
> > > Thanks,
> > > NeilBrown
> > 
> > Thanks, Neil. 
> > 
> > So I'll send v5 of this series to make things easier. 
> > I'll join this PATCH with PATCH 5 and make TODO file
> > empty.
> 
> 
> Ugh, I'm totally confused now.  What is the "correct" patch series to
> consider for merging here?  v5?  Something else?

Actually to make it easier on my side (which I need to optimize for due
to the heavy patch load), I've dropped all of your patches in my queue.
Please resend whatever I have not yet applied that is ready to be
considered for merging.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: mt7621-gpio: retrieve correct pointers in interrupt related functions

[Sergio Paracuellos]

On Sun, May 20, 2018 at 02:39:14PM +0200, Greg KH wrote:
> On Sun, May 20, 2018 at 02:38:09PM +0200, Greg KH wrote:
> > On Sun, May 20, 2018 at 12:18:23PM +0200, Sergio Paracuellos wrote:
> > > On Sun, May 20, 2018 at 07:53:08PM +1000, NeilBrown wrote:
> > > > On Sun, May 20 2018, Sergio Paracuellos wrote:
> > > > 
> > > > > The data passed between irq related functions and the ones which have
> > > > > been retrieved where different. Also first data haven't properly
> > > > > set on irq_domain_add_linear call where it was passing NULL instead.
> > > > >
> > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > 
> > > > Reviewed-by: NeilBrown <neil@brown.name>
> > > > Tested-by: NeilBrown <neil@brown.name>
> > > > 
> > > > :-)
> > > > 
> > > > Thanks,
> > > > NeilBrown
> > > 
> > > Thanks, Neil. 
> > > 
> > > So I'll send v5 of this series to make things easier. 
> > > I'll join this PATCH with PATCH 5 and make TODO file
> > > empty.
> > 
> > 
> > Ugh, I'm totally confused now.  What is the "correct" patch series to
> > consider for merging here?  v5?  Something else?
> 
> Actually to make it easier on my side (which I need to optimize for due
> to the heavy patch load), I've dropped all of your patches in my queue.
> Please resend whatever I have not yet applied that is ready to be
> considered for merging.
> 
> thanks,

I have just resent v5 as you are point out here.

> 
> greg k-h

Best regards,
    Sergio Paracuellos
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: mt7621-gpio: retrieve correct pointers in interrupt related functions

[Sergio Paracuellos]

On Sun, May 20, 2018 at 09:21:48PM +1000, NeilBrown wrote:
> On Sun, May 20 2018, Sergio Paracuellos wrote:
> 
> > On Sun, May 20, 2018 at 07:53:08PM +1000, NeilBrown wrote:
> >> On Sun, May 20 2018, Sergio Paracuellos wrote:
> >> 
> >> > The data passed between irq related functions and the ones which have
> >> > been retrieved where different. Also first data haven't properly
> >> > set on irq_domain_add_linear call where it was passing NULL instead.
> >> >
> >> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> >> 
> >> Reviewed-by: NeilBrown <neil@brown.name>
> >> Tested-by: NeilBrown <neil@brown.name>
> >> 
> >> :-)
> >> 
> >> Thanks,
> >> NeilBrown
> >
> > Thanks, Neil. 
> >
> > So I'll send v5 of this series to make things easier. 
> > I'll join this PATCH with PATCH 5 and make TODO file
> > empty.
> >
> > What is missing to make this driver out of staging after this changes?
> 
> Uhmmm.. I depends on how fussy the gpio maintainer is.
> I think the code is pretty good, but there are probably things that
> could be improved.
> Looking through the code again with my picky-maintainer spectacles on:
> 
> - we could probably use module_platform_driver() instead of
>   subsys_initcall().
> - The various
> 	u32 mask = BIT(d->hwirq);
>   calls look wrong.  hwirq can be as large as 95, and
>      1UL << 95
>   is unlikely to work well.  I cannot test with a gpio above
>   32, but I suspect it wouldn't work.
>   These should use something like
>       #define PIN_MASK(nr) (1UL << ((nr % MTK_BANK_WIDTH)))
> 
> - Locking is a bit weird.  It makes sense to hold the lock across a
>   read-modify-write like in mediatek_gpio_direction_input(), but
>   holding across a simple read (e.g. mediatec_gpio_get_direction())
>   doesn't make much sense, and holding only for the 'write' part
>   of a read-modify-write (e.g. mediateck_gpio_irq_umask()) is down
>   right weird.
> 
> - And now for my pet peeve.
>   There are 3 banks - right?  And they are numbered '0' and '1' and
>   '2'. Agreed?
>   So what is the Maximum bank number?  I think it is "2".
>   "3" is the number of banks, or the count of banks.
>   Yet we have
>   #define MTK_MAX_BANK		3
> 
>   claiming that the maximum bank number is 3, which it isn't.
>   Dan Carpenter recently guided you to fix
> 
> 	if (!id || be32_to_cpu(*id) >= MTK_MAX_BANK)
> 		return -EINVAL;
>   which was
> 	if (!id || be32_to_cpu(*id) > MTK_MAX_BANK)
> 		return -EINVAL;
> 
>   The latter looks right because if something is bigger than the maximum,
>   it is obviously a problem, but if it equals the maximum, it should be
>   fine.  So the code looked right, but was wrong.  You changed it so
>   that it looks wrong, but is right.
>   I would prefer to get both - it should look right and be right.
>   I would suggest changing MTK_MAX_BANK to MTK_BANK_CNT - which
>   means the right thing, and has a name structure similar to
>   MTK_BANK_WIDTH.
> 
> Now looking at the devicetree binding description:
> 
> - #gpio-cells : Should be two.
>   - first cell is the pin number
>   - second cell is used to specify optional parameters (unused)
> 
> I think the second cell can be GPIO_ACTIVE_HIGH or GPIO_ACTIVE_LOW and I
> think that is used.
> Also I think you would typically put
>      interrupt-controller;
>      #interrupt-cells = <1>;
> in the there so that other devices can reference the GPIO interrupts
> if necessary.
> 
> 
> 
> Once all that has landed in staging and I've done one final test, I
> think it will be ready to submit to linux-gpio and the device-tree list.
> 
> >
> > Should it be moved or you were thinking in move all the mt7621
> > together?
> 
> No, I can see no need to keep them together.
> 
> 
> Thanks,
> NeilBrown

I see. Thanks for your clear explanation. I'll try to make this fixes 
also and send them in next series in order to make this driver out 
of staging.

Best regards,
    Sergio Paracuellos