RE: [GIT PULL try#2] Blackfin update

[Hennerich, Michael]

Hi Joe,

>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?
>> 	};
>>

Sure - makes much more sense  

>> > +
>> > +static void portmux_setup(unsigned short per, unsigned short
>> > function)
>> > +{
>> > +	u16 y, muxreg, offset;
>>
>> Shouldn't y be int?

Not necessarily.
Y is the loop counter which counts up to PMUX_LUT_ENTRIES.
I preferably use 16bit variables for loop counters in case I know they
would never overflow.

Reason why I'm doing this is because, Blackfin features zero overhead
hardware loops. Unfortunately the loop counter register is only 16-bit.
Gcc will turn this into slightly better code.  

>>
>> > @@ -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?

Just a number on top of my head (one page of memory), that fits current
and future needs. I guess we should better turn this into a define.

>>
>> 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;
>>

Sure - much nicer to read.

>> cheers,  Joe

Thanks for the feedback!

Regards, Michael
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/