linux-kernel - RE: [PATCH 4/8] regulator: da9055: Add DT support

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:	Thu, 6 Feb 2014 11:31:13 +0000
From:	"Opensource [Adam Thomson]" <Adam.Thomson.Opensource@...semi.com>
To:	Mark Brown <broonie@...nel.org>,
	"Opensource [Adam Thomson]" <Adam.Thomson.Opensource@...semi.com>
CC:	Lee Jones <lee.jones@...aro.org>,
	"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Alessandro Zummo <a.zummo@...ertech.it>,
	Guenter Roeck <linux@...ck-us.net>
Subject: RE: [PATCH 4/8] regulator: da9055: Add DT support

On Wed, Feb 05, 2014 at 18:37:21PM +0000, Mark Brown wrote:

> On Wed, Feb 05, 2014 at 05:48:35PM +0000, Adam Thomson wrote:
> 
> > +#ifdef CONFIG_OF
> > +#include <linux/of.h>
> > +#include <linux/regulator/of_regulator.h>
> > +#endif /* CONFIG_OF */
> 
> Don't do ifdefs for includes like this, it's not worth it.

Fine. Seen examples of both in the kernel, but happy to remove it.

> 
> > +	for_each_child_of_node(nproot, np) {
> > +		if (!of_node_cmp(np->name,
> > +				 regulator->info->reg_desc.name)) {
> > +			config->init_data = of_get_regulator_init_data(
> > +				&pdev->dev, np);
> > +			config->of_node = np;
> > +			break;
> > +		}
> > +	}
> 
> I think you're looking for of_regulator_match() here.

Used another driver as an example for this, but if there's a better method
then I'm happy to use it. Will have a look.

> 
> >  	if (pdata && pdata->regulators)
> >  		config.init_data = pdata->regulators[pdev->id];
> > +	else {
> > +		ret = da9055_regulator_dt_init(pdev, regulator, &config);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> 
> Coding style, both sides of the if should have braces if one does.

Fine. Will update.

> 
> >  	/* Only LDO 5 and 6 has got the over current interrupt */
> >  	if (pdev->id == DA9055_ID_LDO5 || pdev->id ==  DA9055_ID_LDO6) {
> > -		irq = platform_get_irq_byname(pdev, "REGULATOR");
> > -		irq = regmap_irq_get_virq(da9055->irq_data, irq);
> > +		irq = regmap_irq_get_virq(da9055->irq_data,
> > +					  DA9055_IRQ_REGULATOR);
> 
> This seems like a bit of a step backwards - what happened in the MFD
> (and why didn't it update the users to avoid breaking bisection)?

I tested this on target, when doing tests for devicetree. What was happening was
that platform_get_irq_byname() was returning the VIRQ number already (368 in one
test case where onkey was being probed) rather than the local IRQ number for the
device (the resource information seemed to have been updated with the VIRQ
number instead of the local IRQ number). So when that was passed to
regmap_irq_get_virq() it would then return an incorrect IRQ number (0 in the
same scenario, when I enabled DEBUG in irqdomain.c, I would see the message
"error: hwirq 0x170 is too large for da9055_irq"). That incorrect irq was then
being passed to devm_request_threaded_irq() which subsequently failed. This is
why I made the change. Is it preferrable to use platform_get_irq_byname()
instead of regmap_irq_get_virq() as using them both doesn't seem to work, unless
I'm missing something fundamental here.