On Mon, Jul 1, 2013 at 10:26 PM, Jagan Teki <jagannadh.t...@gmail.com> wrote:
> Hi Axel,
>
> Thanks for your v2.
>
> On Sun, Jun 30, 2013 at 9:02 AM, Axel Lin <axel....@ingics.com> wrote:
>> Use ARRAY_SIZE instead of having similar implementation in each drivers.
>>
>> Signed-off-by: Axel Lin <axel....@ingics.com>
>> Cc: Albert Aribaud <albert.u.b...@aribaud.net>
>> Cc: Ben Warren <biggerbadder...@gmail.com>
>> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagn...@jcrosoft.com>
>> Cc: Joe Hershberger <joe.hershber...@ni.com>
>> Cc: Marek Vasut <ma...@denx.de>
>> Cc: Mike Frysinger <vap...@gentoo.org>
>> Cc: Nobuhiro Iwamatsu <iwama...@nigauri.org>
>> Cc: TsiChungLiew <tsi-chung.l...@freescale.com>
>> Cc: Wolfgang Denk <w...@denx.de>
>> Cc: York Sun <york...@freescale.com>
>>
>> Signed-off-by: Axel Lin <axel....@ingics.com>
>> ---
>> Hi Jagan Teki,
>> This is v2 per your request.
>>
>> v2:
>> Fix checkpatch issues, now checkpatch only shows below warnings:
>> a few WARNING: Avoid CamelCase
>> and a WARNING: line over 80 characters
>> #134: FILE: drivers/net/npe/IxEthDBFeatures.c:150:
>> + if
>> (ixEthDBTrafficClassDefinitions[trafficClassDefinitionIndex][IX_ETH_DB_NPE_A_FUNCTIONALITY_ID_INDEX]
>> == npeAImageId.functionalityId) {
>
> I think you can fix this warning and may be you can send a separate
> patch for "CamelCase:"
> if you want.
>
> And also..
>
>>
>> Obviously, I prefer v1 for below reasons:
>> 1. The diff in v2 becomes bigger and makes it harder for review.
>> 2. The whole file in drivers/net/npe/IxEthDBFeatures.c uses 4 spaces as
>> indent,
>> only the lines touched by this patch uses tab as indent (to make
>> checkpatch happy).
>> This makes the code looks odd after apply v2, I don't think this is an
>> improvement.
>>
>> Well, it's up to maintainer to pick up the version he prefer.
>>
>> Regards,
>> Axel
>>
>> drivers/net/ax88180.c | 2 +-
>> drivers/net/fsl_mcdmafec.c | 2 +-
>> drivers/net/lan91c96.c | 2 +-
>> drivers/net/mcffec.c | 2 +-
>> drivers/net/mcfmii.c | 2 +-
>> drivers/net/ne2000.c | 2 +-
>> drivers/net/npe/IxEthDBFeatures.c | 28 ++++++++++++++--------------
>> drivers/net/npe/IxOsalIoMem.c | 3 +--
>> drivers/net/npe/include/IxEthDBPortDefs.h | 2 +-
>> drivers/net/npe/include/IxOsalTypes.h | 2 +-
>> 10 files changed, 23 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/net/ax88180.c b/drivers/net/ax88180.c
>> index f501768..7f0cfe5 100644
>> --- a/drivers/net/ax88180.c
>> +++ b/drivers/net/ax88180.c
>> @@ -157,7 +157,7 @@ static void ax88180_mac_reset (struct eth_device *dev)
>> OUTW (dev, MISC_RESET_MAC, MISC);
>> tmpval = INW (dev, MISC);
>>
>> - for (i = 0; i < (sizeof (program_seq) / sizeof (program_seq[0]));
>> i++)
>> + for (i = 0; i < ARRAY_SIZE(program_seq); i++)
>> OUTW (dev, program_seq[i].value, program_seq[i].offset);
>> }
>>
>> diff --git a/drivers/net/fsl_mcdmafec.c b/drivers/net/fsl_mcdmafec.c
>> index 63842cd..0e18764 100644
>> --- a/drivers/net/fsl_mcdmafec.c
>> +++ b/drivers/net/fsl_mcdmafec.c
>> @@ -520,7 +520,7 @@ int mcdmafec_initialize(bd_t * bis)
>> u32 tmp = CONFIG_SYS_INTSRAM + 0x2000;
>> #endif
>>
>> - for (i = 0; i < sizeof(fec_info) / sizeof(fec_info[0]); i++) {
>> + for (i = 0; i < ARRAY_SIZE(fec_info); i++) {
>>
>> dev =
>> (struct eth_device *)memalign(CONFIG_SYS_CACHELINE_SIZE,
>> diff --git a/drivers/net/lan91c96.c b/drivers/net/lan91c96.c
>> index 11d350e..47c15c4 100644
>> --- a/drivers/net/lan91c96.c
>> +++ b/drivers/net/lan91c96.c
>> @@ -779,7 +779,7 @@ static int lan91c96_detect_chip(struct eth_device *dev)
>> SMC_SELECT_BANK(dev, 3);
>> chip_id = (SMC_inw(dev, 0xA) & LAN91C96_REV_CHIPID) >> 4;
>> SMC_SELECT_BANK(dev, 0);
>> - for (r = 0; r < sizeof(supported_chips) / sizeof(struct id_type);
>> r++)
>> + for (r = 0; r < ARRAY_SIZE(supported_chips); r++)
>> if (chip_id == supported_chips[r].id)
>> return r;
>> return 0;
>> diff --git a/drivers/net/mcffec.c b/drivers/net/mcffec.c
>> index ed7459c..7ae6320 100644
>> --- a/drivers/net/mcffec.c
>> +++ b/drivers/net/mcffec.c
>> @@ -559,7 +559,7 @@ int mcffec_initialize(bd_t * bis)
>> u32 tmp = CONFIG_SYS_INIT_RAM_ADDR + 0x1000;
>> #endif
>>
>> - for (i = 0; i < sizeof(fec_info) / sizeof(fec_info[0]); i++) {
>> + for (i = 0; i < ARRAY_SIZE(fec_info); i++) {
>>
>> dev =
>> (struct eth_device *)memalign(CONFIG_SYS_CACHELINE_SIZE,
>> diff --git a/drivers/net/mcfmii.c b/drivers/net/mcfmii.c
>> index 5e64dbd..63bc8f8 100644
>> --- a/drivers/net/mcfmii.c
>> +++ b/drivers/net/mcfmii.c
>> @@ -186,7 +186,7 @@ int mii_discover_phy(struct eth_device *dev)
>> printf("PHY @ 0x%x pass %d\n", phyno, pass);
>> #endif
>>
>> - for (i = 0; (i < (sizeof(phyinfo) /
>> sizeof(phy_info_t)))
>> + for (i = 0; (i < ARRAY_SIZE(phyinfo))
>> && (phyinfo[i].phyid != 0); i++) {
>> if (phyinfo[i].phyid == phytype) {
>> #ifdef ET_DEBUG
>> diff --git a/drivers/net/ne2000.c b/drivers/net/ne2000.c
>> index 3939158..e6cd3e9 100644
>> --- a/drivers/net/ne2000.c
>> +++ b/drivers/net/ne2000.c
>> @@ -228,7 +228,7 @@ int get_prom(u8* mac_addr, u8* base_addr)
>>
>> mdelay (10);
>>
>> - for (i = 0; i < sizeof (program_seq) / sizeof (program_seq[0]); i++)
>> + for (i = 0; i < ARRAY_SIZE(program_seq); i++)
>> n2k_outb (program_seq[i].value, program_seq[i].offset);
>>
>> PRINTK ("PROM:");
>> diff --git a/drivers/net/npe/IxEthDBFeatures.c
>> b/drivers/net/npe/IxEthDBFeatures.c
>> index c5b680a..d43efaa 100644
>> --- a/drivers/net/npe/IxEthDBFeatures.c
>> +++ b/drivers/net/npe/IxEthDBFeatures.c
>> @@ -143,22 +143,22 @@ void ixEthDBFeatureCapabilityScan(void)
>> IX_ENSURE(sizeof (ixEthDBTrafficClassDefinitions) != 0,
>> "DB: no traffic class definitions found, check IxEthDBQoS.h");
>>
>> /* find the traffic class definition index compatible with
>> the current NPE A functionality ID */
>> - for (trafficClassDefinitionIndex = 0 ;
>> - trafficClassDefinitionIndex < sizeof
>> (ixEthDBTrafficClassDefinitions) / sizeof
>> (ixEthDBTrafficClassDefinitions[0]);
>> - trafficClassDefinitionIndex++)
>> - {
>> - if
>> (ixEthDBTrafficClassDefinitions[trafficClassDefinitionIndex][IX_ETH_DB_NPE_A_FUNCTIONALITY_ID_INDEX]
>> == npeAImageId.functionalityId)
>> - {
>> - /* found it */
>> - break;
>> - }
>> - }
>> + for (trafficClassDefinitionIndex = 0;
>> + trafficClassDefinitionIndex <
>> + ARRAY_SIZE(ixEthDBTrafficClassDefinitions);
>> + trafficClassDefinitionIndex++) {
>> + if
>> (ixEthDBTrafficClassDefinitions[trafficClassDefinitionIndex][IX_ETH_DB_NPE_A_FUNCTIONALITY_ID_INDEX]
>> == npeAImageId.functionalityId) {
>> + /* found it */
>> + break;
>> + }
>> + }
>>
> Above for loop along with if should be a block separable, means
>
> for (trafficClassDefinitionIndex = 0;
> trafficClassDefinitionIndex <
> ARRAY_SIZE(ixEthDBTrafficClassDefinitions);
> trafficClassDefinitionIndex++) {
> if (..........) {
Previous msg sent intermediately,
See my exact comment here.
for (trafficClassDefinitionIndex = 0;
trafficClassDefinitionIndex <
ARRAY_SIZE(ixEthDBTrafficClassDefinitions);
trafficClassDefinitionIndex++) {
if (..........) {
The above way is easy to understand where the code block starts, It
just my opinion you may find the different way
but it should be understandable where should code block start for a
given condition statement.
--
Thanks,
Jagan.
>
>
> --
> Thanks,
> Jagan.
>
>> /* select the default case if we went over the array
>> boundary */
>> - if (trafficClassDefinitionIndex == sizeof
>> (ixEthDBTrafficClassDefinitions) / sizeof
>> (ixEthDBTrafficClassDefinitions[0]))
>> - {
>> - trafficClassDefinitionIndex = 0; /* the first record is
>> the default case */
>> - }
>> + if (trafficClassDefinitionIndex ==
>> + ARRAY_SIZE(ixEthDBTrafficClassDefinitions)) {
>> + /* the first record is the default case */
>> + trafficClassDefinitionIndex = 0;
>> + }
>>
>> /* select queue assignment structure based on the traffic
>> class configuration index */
>> queueStructureIndex =
>> ixEthDBTrafficClassDefinitions[trafficClassDefinitionIndex][IX_ETH_DB_QUEUE_ASSIGNMENT_INDEX];
>> diff --git a/drivers/net/npe/IxOsalIoMem.c b/drivers/net/npe/IxOsalIoMem.c
>> index 34df92b..15a16b8 100644
>> --- a/drivers/net/npe/IxOsalIoMem.c
>> +++ b/drivers/net/npe/IxOsalIoMem.c
>> @@ -66,8 +66,7 @@ ixOsalMemMapFind (UINT32 requestedAddress,
>> {
>> UINT32 mapIndex;
>>
>> - UINT32 numMapElements =
>> - sizeof (ixOsalGlobalMemoryMap) / sizeof (IxOsalMemoryMap);
>> + UINT32 numMapElements = ARRAY_SIZE(ixOsalGlobalMemoryMap);
>>
>> for (mapIndex = 0; mapIndex < numMapElements; mapIndex++)
>> {
>> diff --git a/drivers/net/npe/include/IxEthDBPortDefs.h
>> b/drivers/net/npe/include/IxEthDBPortDefs.h
>> index c3acbdd..eac48cb 100644
>> --- a/drivers/net/npe/include/IxEthDBPortDefs.h
>> +++ b/drivers/net/npe/include/IxEthDBPortDefs.h
>> @@ -106,7 +106,7 @@ static const IxEthDBPortDefinition
>> ixEthDBPortDefinitions[] =
>> * @def IX_ETH_DB_NUMBER_OF_PORTS
>> * @brief number of supported ports
>> */
>> -#define IX_ETH_DB_NUMBER_OF_PORTS (sizeof (ixEthDBPortDefinitions) / sizeof
>> (ixEthDBPortDefinitions[0]))
>> +#define IX_ETH_DB_NUMBER_OF_PORTS ARRAY_SIZE(ixEthDBPortDefinitions)
>>
>> /**
>> * @def IX_ETH_DB_UNKNOWN_PORT
>> diff --git a/drivers/net/npe/include/IxOsalTypes.h
>> b/drivers/net/npe/include/IxOsalTypes.h
>> index 06e71de..615c655 100644
>> --- a/drivers/net/npe/include/IxOsalTypes.h
>> +++ b/drivers/net/npe/include/IxOsalTypes.h
>> @@ -93,7 +93,7 @@ typedef volatile INT32 VINT32;
>>
>>
>> #ifndef NUMELEMS
>> -#define NUMELEMS(x) (sizeof(x) / sizeof((x)[0]))
>> +#define NUMELEMS(x) ARRAY_SIZE(x)
>> #endif
>>
>>
>> --
>> 1.8.1.2
>>
>>
>>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
