Re: [PATCH 1/5] Basic support for the Arcom/Eurotech Viper SBC.

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
From: Eric Miao
Date: Wednesday, August 6, 2008 - 8:50 pm

On Wed, Aug 6, 2008 at 9:19 PM, Marc Zyngier <maz@misterjones.org> wrote:

These are ugly, consider pr_debug() please.


This could be defined in the header file indeed.


It's usually not a good idea to export something like this, you
may want to hide the details of VIPER_CPLD in this file, and
only export those higher-level APIs.


Ditto


this is really weird


this is unreadable and confused, plus the parentheses are not
necessary when there is only one simple statement.

What about this?

v1 = (v1 != v2 || v1 == 0xff) ? 0 : v1; /* a v1i6 board */


This is ugly. If the code is fully verified, I'd prefer to remove this
completely. This is quite low-level code, and no one will ever
want to turn debug on/off frequently to inspect this code.


You may omit the parentheses here, or simplier:

    viper_irq_enabled_mask &= ~(1 << (irq - VIPER_IRQ(0)))


ditto


or simplified to

    return (VIPER_HI_IRQ_STATUS << 8 | VIPER_LO_IRQ_STATUS) &
viper_irq_enabled_mask?


this


this


this


and this, better be removed


this can be simplified to:

gpio_set_value(VIPER_PSU_DATA_GPIO, step & i);


Or alternatively pr_err(), I don't have preference, either is OK to me.


This viper_init_cpufreq() can actually better be included in the
#ifdef CONFIG_CPU_FREQ .. #endif, and use the following
code structure:

#ifdef CONFIG_CPU_FREQ
...
static void __init viper_init_cpufreq(void)
{
    ....
}
#else
static inline void viper_init_cpufreq(void) {}
#endif

So you don't have to worry about whether CONFIG_CPU_FREQ
is selected or not, just invoke viper_init_cpufreq() in the following
code.


These should really be specified by the corresponding driver's
request_irq() with appropriate IRQF_* flags.


Please don't, viper_init_irq() isn't supposed to handle the
initializations of these GPIO, and viper_init_irq() isn't supposed
to fail.

Move this block of code elsewhere please.


I doubt this is necessary here.


braces isn't necessary, while (1) ; will do.


Recent changes have moved ac97 device to a central place so
board code doesn't have to declare this invididually. And use
the corresponding pxa_set_ac97_info() please.


Again, simpler, gpio_set_value(VIPER_LCD_EN_GPIO, on)


I'd suggest to use the generic PWM-based backlight driver, please
see drivers/video/backlight/pwm-bl.c and arch/arm/mach-pxa/pwm.c,
along with zylonite/mainstone for the sample usage.


Use "lcd_conn" type please.


No parentheses please


We now have dedicated pxa serial driver, please use that
driver for internal UARTs.


Isn't ndelay/udelay/mdelay not precise enough? Provided that they
use loops per jiffy so they work with different CPU frequencies??


External SRAM can be directly mapped if necessary, it's
usually no need to create a dedicated device for it.


No, MFP_DIR_OUT isn't supposed to be used externally, use
gpio_direction_output() please.


ditto


ditto


ditto


ditto


pr_info??


ARRAY_AND_SIZE() if you like it


make a dedicated viper_init_i2c() for this block of code please.


If you have wakeup on GPIO1, please refer to mainstone for
a sample of using gpio-keys to specifiy the wakeup source.


These are not necessary, they are encoded in GPIOxx_* macros,
which means you have to modify the MFP table.


Give your name and email address here please, so maybe years
later, I can still find someone to verify this code :)




-- 
Cheers
- eric
--
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
[PATCH 0/5] Support for Arcom/Eurotech Viper SBC, Marc Zyngier, (Wed Aug 6, 6:19 am)
Re: [PATCH 1/5] Basic support for the Arcom/Eurotech Viper ..., Eric Miao, (Wed Aug 6, 8:50 pm)