linux-kernel - Re: [RFC v2 1/8] video: tegra: Add nvhost 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]

Message-ID: <50B7AAA6.70702@wwwdotorg.org>
Date:	Thu, 29 Nov 2012 11:34:14 -0700
From:	Stephen Warren <swarren@...dotorg.org>
To:	Terje Bergström <tbergstrom@...dia.com>
CC:	Thierry Reding <thierry.reding@...onic-design.de>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC v2 1/8] video: tegra: Add nvhost driver

On 11/29/2012 03:21 AM, Terje Bergström wrote:
> On 28.11.2012 23:23, Thierry Reding wrote:
...
>>> +     regs = platform_get_resource(dev, IORESOURCE_MEM, 0);
>>> +     intr0 = platform_get_resource(dev, IORESOURCE_IRQ, 0);
>>> +     intr1 = platform_get_resource(dev, IORESOURCE_IRQ, 1);
>>> +
>>> +     if (!regs || !intr0 || !intr1) {
>>
>> I prefer to have these checked for explicitly, one by one for
>> readability and potentially more useful diagnostics.
> 
> Can do.
> 
>> Also you should be using platform_get_irq() for interrupts. Furthermore
>> the host1x DT node (and the TRM) name the interrupts "syncpt" and
>> "general", so maybe those would be more useful variable names than
>> "intr0" and "intr1".
>>
>> But since you don't use them anyway they shouldn't be part of this
>> patch.
> 
> True. I might also as well delete the general interrupt altogether, as
> we don't use it for any real purpose.

Do make sure the interrupts still are part of the DT binding though, so
that the binding fully describes the HW, and the interrupt is available
to retrieve if we ever do use it in the future.

>>> +     for (i = 0; i < pdata->num_clks; i++)
>>> +             clk_prepare_enable(pdata->clk[i]);
>>> +     nvhost_syncpt_reset(&host->syncpt);
>>> +     for (i = 0; i < pdata->num_clks; i++)
>>> +             clk_disable_unprepare(pdata->clk[i]);
>>
>> Stephen already hinted at this when discussing the AUXDATA. You should
>> explicitly request the clocks.
> 
> I'm not too happy about that idea. The clock management code is generic
> for all modules, and that's why it's driven by a data structure. Now
> Stephen and you ask me to unroll the loops and make copies of the code
> to facilitate different modules and different SoCs.

You can still create tables of clocks inside the driver and loop over
them. So, loop unrolling isn't related to my comments at least. It's
just that clk_get() shouldn't take its parameters from platform data.

But if these are clocks for (arbitrary) child modules (that may or may
not exist dynamically), why aren't the drivers for the child modules
managing them?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/