[U-Boot] [PATCH 01/13] Blackfin: BF60x: new processor header files

[Bob Liu]

Hi Wolfgang,

Thank you for your review.

On Sun, Sep 2, 2012 at 10:10 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Bob Liu,
>
> In message <1345526833-10804-1-git-send-email-lliubbo at gmail.com> you wrote:
>> Add header files for blackfin new processor bf60x.
>>
>> Signed-off-by: Bob Liu <lliubbo at gmail.com>
>> ---
>>  arch/blackfin/include/asm/blackfin_cdef.h         |    3 +
>>  arch/blackfin/include/asm/blackfin_def.h          |    5 +
>>  arch/blackfin/include/asm/blackfin_local.h        |    3 +
>>  arch/blackfin/include/asm/mach-bf609/BF609_cdef.h |  543 +++
>>  arch/blackfin/include/asm/mach-bf609/BF609_def.h  | 3758 +++++++++++++++++++++
>>  arch/blackfin/include/asm/mach-bf609/anomaly.h    |  128 +
>>  arch/blackfin/include/asm/mach-bf609/def_local.h  |    5 +
>>  arch/blackfin/include/asm/mach-bf609/portmux.h    |  257 ++
>>  arch/blackfin/include/asm/mach-bf609/ports.h      |  103 +
>>  arch/blackfin/include/asm/mach-common/bits/cgu.h  |   80 +
>>  arch/blackfin/include/asm/mach-common/bits/dde.h  |   88 +
>>  arch/blackfin/include/asm/mach-common/bits/mpu.h  |    6 +-
>>  arch/blackfin/include/asm/mach-common/bits/pll.h  |    5 +
>>  13 files changed, 4983 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/blackfin/include/asm/mach-bf609/BF609_cdef.h
>>  create mode 100644 arch/blackfin/include/asm/mach-bf609/BF609_def.h
>>  create mode 100644 arch/blackfin/include/asm/mach-bf609/anomaly.h
>>  create mode 100644 arch/blackfin/include/asm/mach-bf609/def_local.h
>>  create mode 100644 arch/blackfin/include/asm/mach-bf609/portmux.h
>>  create mode 100644 arch/blackfin/include/asm/mach-bf609/ports.h
>>  create mode 100644 arch/blackfin/include/asm/mach-common/bits/cgu.h
>>  create mode 100644 arch/blackfin/include/asm/mach-common/bits/dde.h
>
> Please make sure to have the string "PATCH" included with all patch
> submissions, otherwise your patches are lost to patchwork, and most
> likely to the respective custodian as well.
>
>> --- /dev/null
>> +++ b/arch/blackfin/include/asm/mach-bf609/BF609_cdef.h
> ...
>> +#define bfin_read_CGU_STAT() bfin_read32(CGU_STAT)
>> +#define bfin_read_CGU_CLKOUTSEL() bfin_read32(CGU_CLKOUTSEL)
>> +#define bfin_read_CGU_CTL() bfin_read32(CGU_CTL)
>> +#define bfin_write_CGU_CTL(val) bfin_write32(CGU_CTL, val)
>> +#define bfin_read_CGU_DIV() bfin_read32(CGU_DIV)
>> +#define bfin_write_CGU_DIV(val) bfin_write32(CGU_DIV, val)
>
> We don't allow CamelCaps identifiers.  Please fix globally.
>

Sorry, i didn't get your idea here.

>
>> +#define CNT_CFG                     0xFFC00400         /* CNT0 Configuration Register */
>> +#define CNT_IMSK                    0xFFC00404         /* CNT0 Interrupt Mask Register */
>> +#define CNT_STAT                    0xFFC00408         /* CNT0 Status Register */
>> +#define CNT_CMD                     0xFFC0040C         /* CNT0 Command Register */
>> +#define CNT_DEBNCE                  0xFFC00410         /* CNT0 Debounce Register */
>> +#define CNT_CNTR                    0xFFC00414         /* CNT0 Counter Register */
>> +#define CNT_MAX                     0xFFC00418         /* CNT0 Maximum Count Register */
>> +#define CNT_MIN                     0xFFC0041C         /* CNT0 Minimum Count Register */
>
> We don't allow register access based on raw addresses or base address
> plus offset.  Please define proper C structs to describe your
> hardware.   Please fix globally.
>

C structs can be defined to access these registers in other place.
But this file is come from our hardware team i can't change it.

>
>> --- /dev/null
>> +++ b/arch/blackfin/include/asm/mach-bf609/anomaly.h
>> @@ -0,0 +1,128 @@
>> +/*
>> + * DO NOT EDIT THIS FILE
>> + * This file is under version control at
>> + *   svn://sources.blackfin.uclinux.org/toolchain/trunk/proc-defs/header-frags/
>> + * and can be replaced with that version at any time
>> + * DO NOT EDIT THIS FILE
>
> This is bullshit.  If you submit code to U-Boot, it gets maintained in
> the U-Boot git repository.  Dump this.
>
>> + * Copyright 2004-2010 Analog Devices Inc.
>> + * Licensed under the ADI BSD license.
>> + *   https://docs.blackfin.uclinux.org/doku.php?id=adi_bsd
>
> Is this GPL compatible??
>
> The link is dead and returns only
>
>         This topic does not exist yet
>
>> +#define ANOMALY_05000074 (1)
>
> Please do not put parens around simple defines.  Please fix globally.
>

Will be fixed.

> Checkpatch throws a ton of errors that all need fixing.
>

Which Checkpatch script are you using?
In my test there are only WARNING: line over 80 characters.

total: 0 errors, 3546 warnings, 5018 lines checked

NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
MULTISTATEMENT_MACRO_USE_DO_WHILE

/home/bob/u-boot2/0001-Blackfin-BF60x-new-processor-header-files.patch
has style problems, please review.

> Also note that we don't allow fixed network parameters (like
> CONFIG_ETHADDR) in board config files.
>

Will be fixed.

> Review stops here (except for the last part with the licensing stuff).
>

Thank you !

-- 
Regards,
--Bob