Re: [PATCH 7/10] arch/sh: snapgear board cleanups

[Paul M. <le...@li...>]

On Thu, Jul 26, 2007 at 03:31:58PM +1000, David McCullough wrote:
> @@ -37,7 +40,12 @@ static irqreturn_t eraseconfig_interrupt(int irq, void *dev_id)
>  {
>  	volatile char dummy __attribute__((unused)) = * (volatile char *) 0xb8000000;
>  
> +#ifdef CONFIG_LEDMAN
> +	extern void ledman_signalreset(void);
> +	ledman_signalreset();
> +#else
>  	printk("SnapGear: erase switch interrupt!\n");
> +#endif
>  
>  	return IRQ_HANDLED;
>  }

You guys should really try to hammer this in to drivers/leds so it can be
generically supported, if you don't have equivalent functionality there
already.

> + * This is set up by the setup-routine at boot-time
> + */
> +#define PARAM	((unsigned char *)empty_zero_page)
> +
> +#define LOADER_TYPE (*(unsigned long *) (PARAM+0x00c))
> +#define INITRD_START (*(unsigned long *) (PARAM+0x010))
> +#define INITRD_SIZE (*(unsigned long *) (PARAM+0x014))
> +
This is really not something we want board code poking at.

> +static struct resource sg_mtd_ram_resource = {
> +	.flags		= IORESOURCE_MEM,
> +};
> +
> +static struct platdata_mtd_ram sg_mtd_ram_data = {
> +	.mapname	= "Romfs",
> +	.bankwidth	= 1,
> +	.root_dev	= 1,
> +};
> +
> +static struct platform_device sg_mtd_ram_device = {
> +	.name                   = "mtd-ram",
> +	.id                     = 0,
> +	.dev.platform_data      = &sg_mtd_ram_data,
> +	.num_resources          = 1,
> +	.resource               = &sg_mtd_ram_resource,
> +};
> +
This seems like it might be generally useful, perhaps it's worth having
some system-wide platform devices and just make this conditional on your
concatenated romfs thing (or some similar config option). The driver
model isn't available until late in the boot anyways, so we'll already
have parsed all of the INITRD settings by then (where we just don't
bother registering the driver if there's nothing present).

> +static int __init sg_devices_setup(void)
> +{
> +	int ret = 0, ret2 = 0;
> +
> +	if (sg_mtd_ram_resource.start)
> +		ret = platform_device_register(&sg_mtd_ram_device);
> +
> +#ifdef CONFIG_RTC_DRV_DS1302
> +	ret2 = platform_device_register(&sg_rtc_device);
> +	if (ret2)
> +#endif
> +	{
> +#ifdef CONFIG_RTC_DRV_SH
> +		ret2 = platform_device_register(&sh_rtc_device);
> +#endif
> +	}
> +	return ret ? ret : ret2;
> +}
> +
> +__initcall(sg_devices_setup);
> +
Do the RTC devices really need to be ifdef'ed? You should simply have a
devices array flagged as initdata and register it all in one shot. If
support for the devices hasn't been built in to the kernel, the device
will never be claimed, and it will be freed automatically anyways.

This also illustrates why platform_add_devices() is a good thing. Take a
look at arch/sh/boards/renesas/r7780rp/setup.c.