Thread (22 messages) 22 messages, 5 authors, 2014-09-22

RE: [PATCH 1/3 v2] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver

From: Chen, Alvin <hidden>
Date: 2014-09-05 10:21:27
Also in: linux-gpio, lkml

quoted
-static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
+static u32 _dwapb_irq_handler(struct dwapb_gpio *gpio)
What about dwapb_do_irq() ?
OK, I will improve it.
quoted
+static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) {
+	u32 worked;
+	struct dwapb_gpio *gpio = (struct dwapb_gpio *)dev_id;
No need to cast explicitly from void *.
OK.

quoted
-	port->bgc.gc.ngpio = ngpio;
-	port->bgc.gc.of_node = port_np;
+#ifdef CONFIG_OF_GPIO
Do we really need this #ifdef ?
of_node will be NULL anyway, or I missed something?
Yes, otherwise, can't compile it. Please refer 'struct gpio_chip', 'gc.of_node' is in OF_GPIO micro also.
quoted
+	if (pp->irq)
irq == 0 is a valid hwirq (hardware irq) number. Yes, there is unlikely we have it
somewhere, but still it's possible. And yes, IRQ framework doesn't work with
virq == 0 (*virtual* irq), but accepts hwirq == 0. I recommend to use int type for
irq line number, and recognize negative value (usually -1) as no irq needed /
found.
Understand. But if you refer the original code, you can see:
irq = irq_of_parse_and_map(node, 0);
If (!irq) {
......
return;
}
From above code, if irq=0, it indicates irq is not supported for OF devices. If we use '-1' to indicate irq is not supported. To make OF work, then our code should be:

irq = irq_of_parse_and_map(node, 0);
If (!irq) {
pp->irq = -1;
return;
} else {
pp->irq = irq;
}
Then the code looks strange.

How do you think?
quoted
+	bool is_of;
is_pdata_alloc (it might be not only OF case in future).
OK

quoted
+	if (!pdata) {
(*)
quoted
+		pdata = dwapb_gpio_get_pdata_of(dev);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
quoted
+		is_of = true;
+	} else {
+		is_of = false;
Instead of above three lines, how about
bool is_pdata_alloc = !pdata;

And (*) if (is_pdata_alloc) ...
OK. I will improve this part.
quoted
+	if (is_of) {
+		dwapb_free_pdata_of(dev, pdata);
+		pdata = NULL;
Besides that pdata assignment is probably redundant, you may use plain
kmalloc/kfree and avoid unnecessary devm_* calls.
OK.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help