Re: [PATCH v2 8/8] ARM: LPC32xx: Device tree support

[Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org>]

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-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html