Re: [PATCH -mm 1/4] Blackfin: architecture update patch

[Paul Mundt <lethal@linux-sh.org>]

On Wed, Mar 21, 2007 at 06:06:58PM +0800, Wu, Bryan wrote:
> +++ linux-2.6/arch/blackfin/Kconfig	2007-03-21 14:38:42.000000000 +0800
> @@ -21,6 +21,10 @@ config RWSEM_XCHGADD_ALGORITHM
>  	bool
>  	default n
>  
> +config BLACKFIN
> +	bool
> +	default y
> +
>  config BFIN
>  	bool
>  	default y

Again, there's no reason to have both of these. Pick one and stick with
it.

> diff -purN linux-2.6-orig/arch/blackfin/kernel/flat.c linux-2.6/arch/blackfin/kernel/flat.c
> --- linux-2.6-orig/arch/blackfin/kernel/flat.c	1970-01-01 08:00:00.000000000 +0800
> +++ linux-2.6/arch/blackfin/kernel/flat.c	2007-03-21 14:43:37.000000000 +0800
[snip]
> +	switch (type) {
> +		case FLAT_BFIN_RELOC_TYPE_16_BIT:
> +		case FLAT_BFIN_RELOC_TYPE_16H_BIT:
> +			usptr = (unsigned short *)ptr;
> +#ifdef DEBUG_BFIN_RELOC
> +			pr_info(" *usptr = %x", get_unaligned(usptr));
> +#endif
> +			val = get_unaligned(usptr);

The debugging here looks broken, just use pr_debug() and DEBUG as normal,
then you can kill off this DEBUG_BFIN_RELOC thing.

> @@ -346,3 +351,46 @@ unsigned long get_wchan(struct task_stru
>  	while (count++ < 16);
>  	return 0;
>  }
> +
> +#if !defined(CONFIG_NO_ACCESS_CHECK)
> +int _access_ok(unsigned long addr, unsigned long size)

You may want to change this to CONFIG_ACCESS_CHECK instead, it's a bit
more intuitive than checking ! CONFIG_NO_ACCESS_CHECK.

> --- linux-2.6-orig/arch/blackfin/mach-bf533/boards/ezkit.c	2007-03-21 17:32:30.000000000 +0800
> +++ linux-2.6/arch/blackfin/mach-bf533/boards/ezkit.c	2007-03-21 14:42:17.000000000 +0800
> @@ -6,7 +6,7 @@
>   * Created:      2005
>   * Description:
>   *
> - * Rev:          $Id: ezkit.c,v 1.27 2006/11/24 10:23:54 aubrey Exp $
> + * Rev:          $Id: ezkit.c 2794 2007-03-05 05:27:47Z cooloney $
>   *
>   * Modified:     Robin Getz <rgetz@blackfin.uclinux.org> - Named the boards
>   *               Copyright 2005 National ICT Australia (NICTA)

The RCS tags should be killed off as well, it's clear that they don't
really mean anthing, even in this case.

> diff -purN linux-2.6-orig/arch/blackfin/mach-bf533/boards/generic_board.c linux-2.6/arch/blackfin/mach-bf533/boards/generic_board.c
> --- linux-2.6-orig/arch/blackfin/mach-bf533/boards/generic_board.c	2007-03-21 17:32:30.000000000 +0800
> +++ linux-2.6/arch/blackfin/mach-bf533/boards/generic_board.c	2007-03-21 14:42:17.000000000 +0800
> @@ -6,7 +6,7 @@
>   * Created:      2005
>   * Description:
>   *
> - * Rev:          $Id: generic_board.c,v 1.9 2006/09/23 06:35:21 vapier Exp $
> + * Rev:          $Id: generic_board.c 2684 2007-01-22 03:59:04Z vapier $
>   *
>   * Modified:
>   *               Copyright 2005 National ICT Australia (NICTA)

An then do-nothing patches like this can be left out..

> +#if defined(CONFIG_USB_ISP1760_HCD) || defined(CONFIG_USB_ISP1760_HCD_MODULE)
> +static struct resource bfin_isp1761_resources[] = {
> +	[0] = {
> +		.name	= "isp1761-regs",
> +		.start  = ISP1761_BASE + 0x00000000,
> +		.end    = ISP1761_BASE + 0x000fffff,
> +		.flags  = IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.start  = ISP1761_IRQ,
> +		.end    = ISP1761_IRQ,
> +		.flags  = IORESOURCE_IRQ | IORESOURCE_IRQ_LOWLEVEL,
> +	},

Here you have low level.

> +};
> +
> +static struct platform_device bfin_isp1761_device = {
> +	.name           = "isp1761",
> +	.id             = 0,
> +	.num_resources  = ARRAY_SIZE(bfin_isp1761_resources),
> +	.resource       = bfin_isp1761_resources,
> +};
> +
> +static struct platform_device *bfin_isp1761_devices[] = {
> +	&bfin_isp1761_device,
> +};
> +
> +int __init bfin_isp1761_init(void)
> +{
> +	unsigned int num_devices=ARRAY_SIZE(bfin_isp1761_devices);
> +
> +	printk(KERN_INFO "%s(): registering device resources\n", __FUNCTION__);
> +	set_irq_type(ISP1761_IRQ, IRQF_TRIGGER_FALLING);
> +
But here you set it to falling?

> diff -purN linux-2.6-orig/include/asm-blackfin/asm-offsets.h linux-2.6/include/asm-blackfin/asm-offsets.h
> --- linux-2.6-orig/include/asm-blackfin/asm-offsets.h	1970-01-01 08:00:00.000000000 +0800
> +++ linux-2.6/include/asm-blackfin/asm-offsets.h	2007-03-21 15:21:10.000000000 +0800

This is generated by Kbuild, there's no point in including it in the
diff.

> -
> +/* CSYNC implementation for C file */
>  #if defined(ANOMALY_05000312) && defined(ANOMALY_05000244)

Is there some reason these aren't config options? Perhaps an errata
sub-menu might be more intuitive.

> diff -purN linux-2.6-orig/include/asm-blackfin/entry.h linux-2.6/include/asm-blackfin/entry.h
> --- linux-2.6-orig/include/asm-blackfin/entry.h	2007-03-21 17:32:31.000000000 +0800
> +++ linux-2.6/include/asm-blackfin/entry.h	2007-03-21 14:15:41.000000000 +0800
> @@ -52,10 +52,8 @@
>  #define RESTORE_ALL_SYS		restore_context_no_interrupts
>  #define RESTORE_CONTEXT		restore_context_with_interrupts
>  
> -#define STR(X) STR1(X)
> -#define STR1(X) #X
> -# define PT_OFF_ORIG_P0		208
> -# define PT_OFF_SR		8
> +#define PT_OFF_ORIG_P0		208
> +#define PT_OFF_SR		8
>  

Should these be in asm-offsets?

The rest of the patch looks good, this is certainly an improvement!