Skip to main content.

Three examples of CodingStyle problems

Below are three "live" examples from recent discussions.

Example 1

> --- may03_denx/arch/ppc/syslib/ppc440spe_pcie.c   2007-05-03
> 08:39:15.000000000 -0700 +++
> 0323_denx/arch/ppc/syslib/ppc440spe_pcie.c   2007-06-13 14:44:08.422119832
> -0700 @@ -16,53 +16,95 @@
>  #include <asm/reg.h>
>  #include <asm/io.h>
>  #include <asm/ibm44x.h>
> +#include <asm/machdep.h>

Is this really needed?

>  #include "ppc440spe_pcie.h"
> -

Please don't remove this blank line.

>  static int
>  pcie_read_config(struct pci_bus *bus, unsigned int devfn, int offset,
>             int len, u32 *val)
>  {
>     struct pci_controller *hose = bus->sysdata;
> +   volatile void __iomem *addr;
> +
>  #ifdef CONFIG_PCIE_ENDPOINT
>     /*
>      * Endpoint can not generate upstream(remote) config cycles.
>      */
>     return PCIBIOS_DEVICE_NOT_FOUND;
>  #endif
> -   if (!((PCI_SLOT(devfn) == 1) && (PCI_FUNC(devfn) == 0)))
> -      return PCIBIOS_DEVICE_NOT_FOUND;
> -   /*
> -    * 440SPE rev B implements only one function per port
> -    */
> -   if (ppc440spe_revB())
> -      devfn = 0;
> -
> -   offset += devfn << 12;
> +   if(bus->number)
> +   {

Space after "if" and parenthesis in same line as "if":

   if (bus->number) {

> +      /* To avoid reporting redundant devices*/
> +      if(bus->number ==1 && devfn > 0)

      if (bus->number == 1 && devfn > 0)


> +         return PCIBIOS_DEVICE_NOT_FOUND;
> +      /* if this is first device dont scan for devices with devid >1*/
> +      if((bus->number == (hose->first_busno)) && (devfn > 0))
> +         return PCIBIOS_DEVICE_NOT_FOUND;
> +      /* Don't scan on the immidiate bus which is connected to
> +      ** host bridge for devno > 1
> +      */

This is the preferred Linux comment style:

      /*
       * Don't scan on the immidiate bus which is connected to
       * host bridge for devno > 1
       */

> +      if((bus->number == (hose->first_busno+1)) && (devfn > 0))

      if ((bus->number == hose->first_busno + 1) && (devfn > 0))

> +         return PCIBIOS_DEVICE_NOT_FOUND;
> +   }
> +   else
> +   {

   } else {

> +      /* Dont scan for the devices on host bridge with dev  > 1*/
> +      if (!((PCI_SLOT(devfn) == 0) && (PCI_FUNC(devfn) == 0)))
> +         return PCIBIOS_DEVICE_NOT_FOUND;
> +      if (ppc440spe_revB())
> +         devfn = 0;
> +   }
> +   /* Embed the bus number in address */
> +   if(hose->first_busno == bus->number)
> +   {

Again, "{" in same line as "if". I'm stopping here with the coding style
review. Please clean the whole patch up. Thanks.

> +      if(hose->cfg_data == 0x00000000)
> +      addr = ioremap64(0xd10000000ull,0x1000);

Please use some defines for the hardcoded addresses.

> +      if(hose->cfg_data == 0x20000000)
> +      addr = ioremap64(0xd30000000ull,0x1000);
> +      if(hose->cfg_data == 0x40000000)
> +      addr = ioremap64(0xd50000000ull,0x1000);
>
> -   /*
> -    * Note: the caller has already checked that offset is
> -    * suitably aligned and that len is 1, 2 or 4.
> -    */
> +   }
> +   else
> +      addr = ioremap64((0xd00000000ull | (long long )hose->cfg_data |
> +            (long long)((bus->number ) << 20) |
> +            (long long )(devfn << 12)),   0x1000);
> +
> +   if(!addr) printk(KERN_INFO"ioremap64 failed\n");

Don't write two statements in one line

> +
> +   /* DMER disabled . This will suppress the read/write error reporting to 
PLB bus */
> +   mtdcr(0x116,mfdcr(0x116) | 0x02000000); 

Use some defines and not these hardcoded values here.

[...]

Example 2

> Add support for AMCC 405EP Taihu board
>
> Signed-off-by: Random Hacker random@hacker.org <random@hacker.org>

Please remove the first email address here.

<snip>

> diff --git a/board/amcc/taihu/lcd.c b/board/amcc/taihu/lcd.c
> new file mode 100644
> index 0000000..aa6207c
> --- /dev/null
> +++ b/board/amcc/taihu/lcd.c
> @@ -0,0 +1,230 @@
> +#include <config.h>
> +#include <common.h>
> +#include <command.h>
> +
> +#ifdef CONFIG_TAIHU

Is this needed when this file is located in the taihu board
directory?

> +#define LCD_CMD_ADDR  ((volatile char *)0x50100002)
> +#define LCD_DATA_ADDR ((volatile char *)0x50100003)
> +#define LCD_BLK_CTRL  ((volatile char *)CPLD_REG1_ADDR)

This volatile access is not recommended. I know this is how it is
handled in various drivers already, but we should not use it for
"new" drivers/routines. The in_be32 and friend routines from io.h
should be used instead.

How about this:

#define LCD_CMD_ADDR   0x50100002
#define LCD_DATA_ADDR   0x50100003
#define LCD_BLK_CTRLC   PLD_REG1_ADDR

...

> +static char* amcc_logo = "AMCC 405EP TAIHU EVALUATION KIT";
> +static char addr_flag = 0x80;
> +
> +static void lcd_bl_ctrl(char val)
> +{
> +   char cpld_val;
> +   cpld_val = *LCD_BLK_CTRL;
> +   *LCD_BLK_CTRL = val | cpld_val;
> +}

... and then use

static void lcd_bl_ctrl(char val)
{
   out_8(LCD_BLK_CTRL, in_8(LCD_BLK_CTRL) | val);
}

> +static void lcd_putc(char val)
> +{
> +   int i = 100;
> +   char addr;

Please insert a blank line after the variable declaration.

[...]

> +static void lcd_puts(char* s)
> +{
> +   char* p = s;
> +   int i = 100;
> +
> +   while(i--) {

Space after "while".

> +      if ((*LCD_CMD_ADDR & 0x80) != 0x80) { /*BF = 1 ?*/
> +         udelay(50);
> +         break;
> +      }
> +      udelay(50);
> +   }
> +   if (*LCD_CMD_ADDR & 0x80) {
> +      printf("LCD is busy\n");
> +      return;
> +   }
> +   while(*p) {

Again.

> +      lcd_putc(*p++);
> +   }
> +}
> +
> +static void lcd_put_logo(void)
> +{
> +   int i = 100;
> +   char* p = amcc_logo;
> +
> +   while (i--) {
> +      if ((*LCD_CMD_ADDR & 0x80) != 0x80) { /*BF = 1 ?*/
> +         udelay(50);
> +         break;
> +      }
> +      udelay(50);
> +   }
> +   if (*LCD_CMD_ADDR & 0x80) {
> +      printf("LCD is busy\n");
> +      return;
> +   }
> +   *LCD_CMD_ADDR = 0x80;
> +   while(*p) {

Again.

> +      lcd_putc(*p++);
> +   }
> +}
> +
> +int lcd_init(void)
> +{
> +   puts("LCD: ");
> +   *LCD_CMD_ADDR = 0x38;/*set function:8-bit,2-line,5x7 font type*/
> +   udelay(50);
> +   *LCD_CMD_ADDR = 0x0f;/*set display on,cursor on,blink on*/
> +   udelay(50);
> +   *LCD_CMD_ADDR = 0x01;/*display clear*/
> +   udelay(2000);
> +   *LCD_CMD_ADDR = 0x06;/*set entry*/
> +   udelay(50);
> +   lcd_bl_ctrl(0x02);/*set backlight on*/

Please indent the comments above.

> +   lcd_put_logo();
> +   puts("ready\n");
> +   return 0;
> +
> +}

No blank after "return 0" please.

And so on.

[...]

> +static int do_sw_stat(cmd_tbl_t* cmd_tp, int flags, int argc, char *argv[])
> +{ 
> +   char stat;
> +   int i;

Blank line after var decl.

> +   stat = *(volatile char *)CPLD_REG0_ADDR;

Should be changed to in_8() too.

> +   printf("SW2 status: ");
> +   for (i=0; i<4; i++) /* 4-position */
> +      printf("%d:%s ",i,stat & (0x08 >> i)?"on":"off");
> +   printf("\n");
> +   return 0;
> +}
> +
> +U_BOOT_CMD (
> +   sw2_stat,1,1,do_sw_stat,
> +   "sw2_stat - show status of switch 2\n",
> +   NULL
> +);
> +
> +static int do_led_ctl(cmd_tbl_t* cmd_tp, int flags, int argc, char *argv[]) 
> +{ 
> +   int led_no;
> +   if (argc != 3) {
> +      printf("%s",cmd_tp->usage);

Blank after ",".

> +      return -1;
> +   }
> +   led_no = simple_strtoul(argv[1],NULL,16);
> +   if (led_no != 1 && led_no != 2) {
> +      printf("%s",cmd_tp->usage);

Again.

[...]

> diff --git a/cpu/ppc4xx/405gp_pci.c b/cpu/ppc4xx/405gp_pci.c
> index 7134355..95f0ac5 100644
> --- a/cpu/ppc4xx/405gp_pci.c
> +++ b/cpu/ppc4xx/405gp_pci.c
> @@ -364,6 +364,11 @@ void pci_405gp_fixup_irq(struct pci_cont
>        break;
>     }
>
> +#if defined(CONFIG_TAIHU)
> +   if (PCI_DEV(dev) == 0x6) int_line = 25;
> +   else if(PCI_DEV(dev) == 0x7 ) int_line = 27;
> +#endif

This needs to be moved out of the generic 4xx pci code. I'm aware that
you didn't introduce this part, but since you "touch" it with this patch
it is now time to handle the PCI IRQ assignment in a more generic way,
so that it can be handled in a board specific way.

<snip>

> diff --git a/include/configs/taihu.h b/include/configs/taihu.h
> new file mode 100644
> index 0000000..3f433ab
> --- /dev/null
> +++ b/include/configs/taihu.h
> @@ -0,0 +1,416 @@
> +/*
> + * (C) Copyright 2000-2005
> + * Wolfgang Denk, DENX Software Engineering, wd@denx.de.

Don't you want to add a author/copyright note here? This is valid
for other files from you as well.

Example 3

> Add support for AMCC 405EP Taihu board
>
> Signed-off-by: Random Hacker <random@hacker.org>
>
> ---
> commit 2872d3cb2ddb50f58b6ad42056ef802c501d5e6a
> tree 88b1e3f948c4074348df43f72409894eba15bfe3
> parent 9045f33c023f698660a2e45d1b2194c0711abebc
> author Random Hacker <random@hacker.org> Mon, 11 Jun 2007 00:04:09 -0500
> committer Random Hacker <random@hacker.org> Mon, 11 Jun 2007 00:04:09 -0500
>
>   MAKEALL                     |    8
>   Makefile                    |    3
>   board/amcc/taihu/Makefile   |   49 ++
>   board/amcc/taihu/config.mk  |   24 +
>   board/amcc/taihu/flash.c    | 1083
> +++++++++++++++++++++++++++++++++++++++++++ board/amcc/taihu/lcd.c      | 
> 226 +++++++++
>   board/amcc/taihu/taihu.c    |  248 ++++++++++
>   board/amcc/taihu/u-boot.lds |  150 ++++++
>   board/amcc/taihu/update.c   |  111 ++++
>   board/amcc/taihu/usbdev.c   |  410 ++++++++++++++++
>   common/soft_spi.c           |    2
>   cpu/ppc4xx/405gp_pci.c      |   25 -
>   cpu/ppc4xx/sdram.c          |   10
>   cpu/ppc4xx/start.S          |   32 +
>   dtt/Makefile                |    2
>   dtt/ds1775.c                |  159 ++++++
>   include/configs/taihu.h     |  429 +++++++++++++++++
>   include/dtt.h               |    8
>   include/usb.h               |    2
>   19 files changed, 2957 insertions(+), 24 deletions(-)
>

<snip>

> diff --git a/board/amcc/taihu/lcd.c b/board/amcc/taihu/lcd.c
> new file mode 100644
> index 0000000..167ea42
> --- /dev/null
> +++ b/board/amcc/taihu/lcd.c
> @@ -0,0 +1,226 @@
> +#include <config.h>
> +#include <common.h>
> +#include <command.h>
> +#include <asm/io.h>
> +
> +#define LCD_CMD_ADDR   0x50100002
> +#define LCD_DATA_ADDR   0x50100003
> +#define LCD_BLK_CTRL   CPLD_REG1_ADDR
> +
> +static char* amcc_logo = "AMCC 405EP TAIHU EVALUATION KIT";

Nitpicking: Should be "static char *amcc_log = ...". Please clean
this up through the whole file. Thanks.

> +static int addr_flag = 0x80;
> +
> +static void lcd_bl_ctrl(char val)
> +{
> +   writeb(LCD_BLK_CTRL, readb(LCD_BLK_CTRL) | val);

The writeb/l access macros/functions are not recommended to access
peripheral devices. I know, it's done in multiple drivers in this
way too, but for new code we should use the "correct" access
functions. And this is: in_be32() and friends. Please take a look
at include/asm-ppc/io.h for details.

<snip>

> +int board_early_init_f(void)
> +{
> +   lcd_init();
> +
> +   mtdcr(uicsr, 0xFFFFFFFF);   /* clear all ints */
> +   mtdcr(uicer, 0x00000000);   /* disable all ints */
> +   mtdcr(uiccr, 0x00000000);
> +   mtdcr(uicpr, 0xFFFF7F00);   /* set int polarities */
> +   mtdcr(uictr, 0x00000000);   /* set int trigger levels */
> +   mtdcr(uicsr, 0xFFFFFFFF);   /* clear all ints */
> +   mtdcr(uicvcr, 0x00000001);   /* set vect base=0,INT0 highest priority */
> +
> +   mtebc(pb3ap, 0x158ff600);   /* memory bank 3 (CPLD_LCM) initialization */
> +   mtebc(pb3cr, 0x50118000);

Please use the defines in the board config file for EBC setup. Just
add:

/* Memory Bank 3 (CPLD_LCM) initialization */
#define CFG_EBC_PB3AP           0x158ff600
#define CFG_EBC_PB3CR           0x50118000

to the taihu board config file.

> +   return 0;
> +}
> +
> +
> +/*
> + * board_early_init_r
> + */
> +int board_early_init_r(void)
> +{
> +    extern void set_pci_405gp_fixup_irq(unsigned, unsigned char);

Please move this extern declaration to the top of the file.

> +    set_pci_405gp_fixup_irq( 6, 25 );
> +    set_pci_405gp_fixup_irq( 7, 27 );

Not sure I like this. Please see my comments in the 405gp_pci.c file.

And please not spaces after "(" and before ")".

[...]

> +static int do_led_ctl(cmd_tbl_t* cmd_tp, int flags, int argc, char
> *argv[]) +{
> +   int led_no;
> +   if (argc != 3) {
> +      printf("%s", cmd_tp->usage);
> +      return -1;
> +   }
> +   led_no = simple_strtoul(argv[1], NULL, 16);
> +   if (led_no != 1 && led_no != 2) {
> +      printf("%s", cmd_tp->usage);
> +      return -1;
> +   }
> +   if (strcmp(argv[2],"off") == 0x0 ) {
> +      if (led_no == 1)
> +         out32(GPIO0_OR, in32(GPIO0_OR) | 0x00000002); /* GPIO30 */

I recently added some code for 4xx GPIO handling. Please see
cpu/ppc4xx/gpio.c for details. For this action you could use:

   gpio_write_bit(30, 1);   /* set GPIO30 to '1' */

> +      else
> +         out32(GPIO0_OR, in32(GPIO0_OR) | 0x00000001); /* GPIO31 */
> +   }
> +   else if (strcmp(argv[2],"on") == 0x0 ) {
> +      if (led_no == 1)
> +         out32(GPIO0_OR, in32(GPIO0_OR) & ~0x00000002);

   gpio_write_bit(30, 0);   /* set GPIO30 to '0' */

<snip>

> diff --git a/cpu/ppc4xx/start.S b/cpu/ppc4xx/start.S
> index 78d0042..388e16b 100644
> --- a/cpu/ppc4xx/start.S
> +++ b/cpu/ppc4xx/start.S
> @@ -1856,6 +1856,11 @@ #ifdef CONFIG_BUBINGA
>      ori   r3,r3,CFG_EBC_PB4CR@l
>      mtdcr   ebccfgd,r3
>   #endif
> +#ifdef CONFIG_TAIHU
> +   mfdcr r4, CPC0_BOOT
> +   andi. r5, r4, CPC0_BOOT_SEP@l
> +   bne strap_0         /* serial eeprom present */
> +#endif

I really hesitate to accept board specific changes in such files
as start.S. I know, again it was done before. But the common code
gets uglier and uglier and harder to maintain.

It's not that easy in this case to extract this code into some
board specific files. Do you have any ideas how this could be done?
If not, I'll have to think about it some more. Please contact me
again on this issue next week. Thanks.

<snip>

> diff --git a/dtt/Makefile b/dtt/Makefile
> index e6cb128..c6a670a 100644
> --- a/dtt/Makefile
> +++ b/dtt/Makefile
> @@ -30,7 +30,7 @@ #CFLAGS += -DDEBUG
>
>   LIB   = $(obj)libdtt.a
>
> -COBJS   = lm75.o ds1621.o adm1021.o lm81.o
> +COBJS   = lm75.o ds1621.o adm1021.o lm81.o ds1775.o
>
>   SRCS   := $(COBJS:.o=.c)
>   OBJS   := $(addprefix $(obj),$(COBJS))
> diff --git a/dtt/ds1775.c b/dtt/ds1775.c
> new file mode 100644
> index 0000000..39a648e
> --- /dev/null
> +++ b/dtt/ds1775.c
> @@ -0,0 +1,159 @@
> +/*
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */

Please add a copyright note or at least a line with your (or the
original authors) name and email address to this header.