linux-kernel - Re: [PATCH v2 8/8] ARM: LPC32xx: Device tree 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:	Wed, 18 Apr 2012 14:30:20 +0200
From:	Roland Stigge <stigge@...com.de>
To:	Thierry Reding <thierry.reding@...onic-design.de>
CC:	arm@...nel.org, linux-arm-kernel@...ts.infradead.org,
	gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
	dmitry.torokhov@...il.com, axel.lin@...il.com,
	broonie@...nsource.wolfsonmicro.com, marek.vasut@...il.com,
	devel@...verdev.osuosl.org, kevin.wells@....com,
	srinivas.bakki@....com
Subject: Re: [PATCH v2 8/8] ARM: LPC32xx: Device tree support

Hi Thierry,

thanks for your suggestions.

On 04/18/2012 08:02 AM, Thierry Reding wrote:
>> arch/arm/mach-lpc32xx/irq.c                           |   78
>> +++++--
> 
> Could this perhaps be split into another patch. Basically this is
> a conversion to IRQ domain *and* device tree support. But maybe it
> isn't worth the effort.

I was thinking the same. :-) Specifically, using
irq_domain_add_legacy() already builds upon available DT nodes. DT for
irq in turn seems to require irqdomain. So  would only split those two
on explicit request of reviewers when we have a reasonable solution to
this dilemma. Otherwise, I'd keep it in one patch.

>> arch/arm/mach-lpc32xx/phy3250.c                       |  146
>> +++++--------
> 
> While at it, this should probably be renamed board-dt.c or
> something similar since it is no longer phy3250 specific.

As Arnd wrote, there are board specific bits left (open questions
about how to DT'ize spi's controller_data, and DT-conversion of AMBA
CLCD). However, I will move board-independent stuff over to common.c.
(Keeping lpc32xx_auxdata_lookup[] in phy3250.c due to remaining
platform_data, though.)

>> -	platform_add_devices(phy3250_devs, ARRAY_SIZE(phy3250_devs)); -
>> for (i = 0; i < ARRAY_SIZE(amba_devs); i++) { -		struct
>> amba_device *d = amba_devs[i]; -		amba_device_register(d,
>> &iomem_resource); -	} +	tmp =
>> __raw_readl(LPC32XX_CLKPWR_DMA_CLK_CTRL); +	__raw_writel((tmp |
>> LPC32XX_CLKPWR_DMACLKCTRL_CLK_EN), +
>> LPC32XX_CLKPWR_DMA_CLK_CTRL); /* Test clock needed for UDA1380
>> initial init */ __raw_writel(LPC32XX_CLKPWR_TESTCLK2_SEL_MOSC | 
>> LPC32XX_CLKPWR_TESTCLK_TESTCLK2_EN, 
>> LPC32XX_CLKPWR_TEST_CLK_SEL);
> 
> A lot of these seem to be gratuitous here. Can control of these
> clocks not be exposed via the clock framework? That would allow the
> controllers to only activate them when actually needed. This could
> also be done in follow up patches, though.

Correct, clock framework is on my list of following patches.

> 
> [...]
>> +	/* Register GPIOs used on this board */ +	if
>> (gpio_request(SPI0_CS_GPIO, "spi0 cs")) +		printk(KERN_ERR "Error
>> requesting gpio %u", +			SPI0_CS_GPIO); +	else if
>> (gpio_direction_output(SPI0_CS_GPIO, 1)) +		printk(KERN_ERR
>> "Error setting gpio %u to output", +			SPI0_CS_GPIO);
> 
> This should be initialized based on data from the device tree.

Correct. This will be done in the later patch regarding spi controller
DT conversion. Still discussing this with Grant. For now, I'm
considering the above hunk as just moving around the resp. lines
without change. Please let me keep it until the SPI issue is solved.

Thanks,

Roland
--
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/