Re: [GIT PULL try#2] Blackfin update

[Bryan Wu <bryan.wu@analog.com>]

Hi Joe, please reply to all of this email, because some developers need
to consider your review.

Hi Michael, how do you think of Joe's idea as below.

Thanks, 
Best Regards,
- Bryan Wu

On Wed, 2007-07-25 at 09:46 -0700, Joe Perches wrote:
> On Wed, 2007-07-25 at 23:26 +0800, Bryan Wu wrote:
> > diff --git a/arch/blackfin/kernel/bfin_gpio.c
> > b/arch/blackfin/kernel/bfin_gpio.c
> > index bafcfa5..979cf79 100644
> > --- a/arch/blackfin/kernel/bfin_gpio.c
> > +++ b/arch/blackfin/kernel/bfin_gpio.c
> > @@ -143,22 +148,100 @@ inline int check_gpio(unsigned short gpio)
> 
> []
> 
> > +#ifdef BF537_FAMILY
> > +
> > +#define PMUX_LUT_RES		0
> > +#define PMUX_LUT_OFFSET		1
> > +#define PMUX_LUT_ENTRIES	41
> > +#define PMUX_LUT_SIZE		2
> > +
> > +static unsigned short port_mux_lut[PMUX_LUT_ENTRIES][PMUX_LUT_SIZE] = {
> > +	{P_PPI0_D13, 11}, {P_PPI0_D14, 11}, {P_PPI0_D15, 11},
> > +	{P_SPORT1_TFS, 11}, {P_SPORT1_TSCLK, 11}, {P_SPORT1_DTPRI, 11},
> > +	{P_PPI0_D10, 10}, {P_PPI0_D11, 10}, {P_PPI0_D12, 10},
> > +	{P_SPORT1_RSCLK, 10}, {P_SPORT1_RFS, 10}, {P_SPORT1_DRPRI, 10},
> > +	{P_PPI0_D8, 9}, {P_PPI0_D9, 9}, {P_SPORT1_DRSEC, 9},
> > +	{P_SPORT1_DTSEC, 9}, {P_TMR2, 8}, {P_PPI0_FS3, 8}, {P_TMR3, 7},
> > +	{P_SPI0_SSEL4, 7}, {P_TMR4, 6}, {P_SPI0_SSEL5, 6}, {P_TMR5, 5},
> > +	{P_SPI0_SSEL6, 5}, {P_UART1_RX, 4}, {P_UART1_TX, 4}, {P_TMR6, 4},
> > +	{P_TMR7, 4}, {P_UART0_RX, 3}, {P_UART0_TX, 3}, {P_DMAR0, 3},
> > +	{P_DMAR1, 3}, {P_SPORT0_DTSEC, 1}, {P_SPORT0_DRSEC, 1},
> > +	{P_CAN0_RX, 1}, {P_CAN0_TX, 1}, {P_SPI0_SSEL7, 1},
> > +	{P_SPORT0_TFS, 0}, {P_SPORT0_DTPRI, 0}, {P_SPI0_SSEL2, 0},
> > +	{P_SPI0_SSEL3, 0}
> > +};
> 
> Wouldn't this be better as a struct?
> 
> struct PMUX_LUT {
> 	unsigned short res;
> 	unsigned short offset;
> }
> 
> struct PMUX_LUT port_mux_lut = {
> 	{.res = P_PPI0_D13, .offset = 11},
> 	{.res = P_PPi0_D14, .offset = 11},
> etc?
> 	};
> 
> > +
> > +static void portmux_setup(unsigned short per, unsigned short
> > function)
> > +{
> > +	u16 y, muxreg, offset;
> 
> Shouldn't y be int?
> 
> > @@ -179,22 +262,15 @@ static void default_gpio(unsigned short gpio)
> >  
> >  static int __init bfin_gpio_init(void)
> >  {
> > -	int i;
> > -
> > -	printk(KERN_INFO "Blackfin GPIO Controller\n");
> >  
> > -	for (i = 0; i < MAX_BLACKFIN_GPIOS; i += GPIO_BANKSIZE)
> > -		reserved_map[gpio_bank(i)] = 0;
> > +	str_ident = kzalloc(RESOURCE_LABEL_SIZE * 256, GFP_KERNEL);
> 
> 256?
> 
> perhaps better to use kcalloc(RESOURCE_LABEL_SIZE, 256, GFP_KERNEL)
> and reference str_ident as an array of char* instead of the
> multiplies in set_label, get_label and cmp_label?
> 
> > +	if (!str_ident)
> > +		return -ENOMEM;
> 
> cheers,  Joe