[U-Boot] [PATCH v2] net: Use ARRAY_SIZE at appropriate places
Jagan Teki
jagannadh.teki at gmail.com
Mon Jul 1 19:00:11 CEST 2013
On Mon, Jul 1, 2013 at 10:26 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> Hi Axel,
>
> Thanks for your v2.
>
> On Sun, Jun 30, 2013 at 9:02 AM, Axel Lin <axel.lin at ingics.com> wrote:
>> Use ARRAY_SIZE instead of having similar implementation in each drivers.
>>
>> Signed-off-by: Axel Lin <axel.lin at ingics.com>
>> Cc: Albert Aribaud <albert.u.boot at aribaud.net>
>> Cc: Ben Warren <biggerbadderben at gmail.com>
>> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj at jcrosoft.com>
>> Cc: Joe Hershberger <joe.hershberger at ni.com>
>> Cc: Marek Vasut <marex at denx.de>
>> Cc: Mike Frysinger <vapier at gentoo.org>
>> Cc: Nobuhiro Iwamatsu <iwamatsu at nigauri.org>
>> Cc: TsiChungLiew <Tsi-Chung.Liew at freescale.com>
>> Cc: Wolfgang Denk <wd at denx.de>
>> Cc: York Sun <yorksun at freescale.com>
>>
>> Signed-off-by: Axel Lin <axel.lin at 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
>>
>>
>>
More information about the U-Boot
mailing list