Re: [PATCH] PCM038: Added config option for PCM970 development board

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Alexander,

On Thu, Apr 26, 2012 at 12:35:38AM +0400, Alexander Shiyan wrote:
> This change includes:
> - Adding Kconfig option for PCM970 development board.
> - Moving some devices from PCM038, which really not installed on module.
> - Some reformatting source code.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>

Can you please try and make the patch smaller so that we can see what is
going on?

>  
>  #include "pll.h"
>  
> +#define GPIO_SPI_CS0	(GPIO_PORTD + 28)
> +
>  static struct fec_platform_data fec_info = {
> -	.xcv_type = MII100,
> -	.phy_addr = 1,
> +	.xcv_type	= MII100,
> +	.phy_addr	= 1,
>  };

This only adds to the patch size and increases the chance that a 'git
blame' only directs to cosmetic changes instead of the commit
introducing a real change.

I do not enforce a particular alignment. I agree that having struct
initializers aligned looks nice, but it also has the disadvantage that
the alignment may break with a longer variable name.

So in short, if an author has decided for one or the other, just keep
it like it is.

>  
> -static int pcm038_spi_cs[] = {GPIO_PORTD + 28};
> +static int pcm038_spi_cs[] = { GPIO_SPI_CS0, };

Seems unrelated to this patch.

> -/**
> - * If the PLL settings are in place switch the CPU core frequency to the max. value
> - */
> -static int pcm038_power_init(void)
> -{
> -	uint32_t spctl0;
> -
> -	spctl0 = get_pll_spctl10();
> -
> -	/* PLL registers already set to their final values? */
> -	if (spctl0 == SPCTL0_VAL && MPCTL0 == MPCTL0_VAL) {
> -		console_flush();
> -		if (!pmic_power()) {
> -			/* wait for required power level to run the CPU at 400 MHz */
> -			udelay(100000);
> -			CSCR = CSCR_VAL_FINAL;
> -			PCDR0 = 0x130410c3;
> -			PCDR1 = 0x09030911;
> -			/* Clocks have changed. Notify clients */
> -			clock_notifier_call_chain();
> -		} else {
> -			printf("Failed to initialize PMIC. Will continue with low CPU speed\n");
> -		}
> -	}
> -
> -	/* clock gating enable */
> -	GPCR = 0x00050f08;
> -
> -	return 0;
> -}
> -
> -late_initcall(pcm038_power_init);

You move pcm038_power_init from a late_initcall to a direct call
somewhere else. There might be reasons to do this, but such a change
should not be hidden in a big patch which according to the commit log
factors out baseboard code. Also I don't see why the function moves
to another place in this patch.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox