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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 5 Sep 2014 10:20:53 +0000
From:	"Chen, Alvin" <alvin.chen@...el.com>
To:	"Shevchenko, Andriy" <andriy.shevchenko@...el.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"Kweh, Hock Leong" <hock.leong.kweh@...el.com>,
	"sebastian@...akpoint.cc" <sebastian@...akpoint.cc>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"Ong, Boon Leong" <boon.leong.ong@...el.com>,
	"gnurou@...il.com" <gnurou@...il.com>,
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"Westerberg, Mika" <mika.westerberg@...el.com>,
	"dvhart@...ux.intel.com" <dvhart@...ux.intel.com>,
	"atull@...nsource.altera.com" <atull@...nsource.altera.com>
Subject: RE: [PATCH 1/3 v2] GPIO: gpio-dwapb: Enable platform driver binding
 to MFD driver

> > -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.

> > +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.


> > -	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.

> > +	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?

> > +	bool is_of;
> 
> is_pdata_alloc (it might be not only OF case in future).
> 
OK


> > +	if (!pdata) {
> 
> (*)

> > +		pdata = dwapb_gpio_get_pdata_of(dev);
> > +		if (IS_ERR(pdata))
> > +			return PTR_ERR(pdata);
> 
> > +		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.

> > +	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.