Re: [PATCH 1/4] powerpc/mpc512x: Add initial support for TWR-MPC5125

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2404 bytes --]

Hi Vladimir,

(if possible, please provide a diffstat with the patches)

> diff --git a/arch/powerpc/platforms/512x/clock.c b/arch/powerpc/platforms/512x/clock.c
> index 3dc2a8d..962c0ba 100644
> --- a/arch/powerpc/platforms/512x/clock.c
> +++ b/arch/powerpc/platforms/512x/clock.c
> @@ -669,6 +669,13 @@ static void psc_calc_rate(struct clk *clk, int pscnum, struct device_node *np)
>  	clk->rate = mclk_src / mclk_div;
>  }
>  
> +
> +#ifdef CONFIG_PPC_MPC5125
> +#define PSC_PREFIX "mpc5125"
> +#else
> +#define PSC_PREFIX "mpc5121"
> +#endif
> +
>  /*
>   * Find all psc nodes in device tree and assign a clock
>   * with name "psc%d_mclk" and dev pointing at the device
> @@ -680,7 +687,7 @@ static void psc_clks_init(void)
>  	const u32 *cell_index;
>  	struct platform_device *ofdev;
>  
> -	for_each_compatible_node(np, NULL, "fsl,mpc5121-psc") {
> +	for_each_compatible_node(np, NULL, "fsl," PSC_PREFIX "-psc") {

Uh, that makes it impossible to have one kernel for mpc5121/5.

> -void __init mpc512x_psc_fifo_init(void)
> +void __init mpc512x_psc_fifo_init(char *psc_name)
>  {
>  	struct device_node *np;
>  	void __iomem *psc;
>  	unsigned int tx_fifo_size;
>  	unsigned int rx_fifo_size;
> +	char *default_psc = "fsl,mpc5121-psc";
>  	int fifobase = 0; /* current fifo address in 32 bit words */
>  
> -	for_each_compatible_node(np, NULL, "fsl,mpc5121-psc") {
> +	if (!psc_name)
> +		psc_name = default_psc;
> +
> +	for_each_compatible_node(np, NULL, psc_name) {

I think this goes more to the right direction, although you passed the
non-default string for mpc5125 in the board-config, which is the wrong place,
because it is a platform thing.

What about something like:

	if of_find_compatible_node(startpoint, NULL, "fsl,mpc5121-psc")
		psc_compat = "fsl,mpc5121-psc";
	else if of_find_compatible_node(startpoint, NULL, "fsl,mpc5125-psc")
		psc_compat = "fsl,mpc5125-psc";
	else if
		/* Problem handling */

Dunno, might be worth to put it into a function as it could be used here and in
the block above.

Also, I noticed quite a number of magic values (e.g. 0x76). I guess those are
register and bit names, which should be used instead.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]