Re: [PATCH 1/3] ARM: OMAP: Allocate McBSP devices dynamically, add 34xx support

["Uwe Kleine-König" <ukleinek@strlen.de>]

Hello,

I wonder if this patch should be further split.

To my taste this should be at least:

 - Allocate McBSP devices dynamically,
 - add 34xx support

maybe more, see below.

> diff --git a/arch/arm/mach-omap1/mcbsp.c b/arch/arm/mach-omap1/mcbsp.c
> index afb5789..7de7c69 100644
> --- a/arch/arm/mach-omap1/mcbsp.c
> +++ b/arch/arm/mach-omap1/mcbsp.c
> @@ -103,30 +103,6 @@ static inline void omap_mcbsp_clk_init(struct mcbsp_internal_clk *mclk)
>  { }
>  #endif
>  
> -static int omap1_mcbsp_check(unsigned int id)
> -{
> -	/* REVISIT: Check correctly for number of registered McBSPs */
> -	if (cpu_is_omap730()) {
> -		if (id > OMAP_MAX_MCBSP_COUNT - 2) {
> -		       printk(KERN_ERR "OMAP-McBSP: McBSP%d doesn't exist\n",
> -				id + 1);
> -		       return -ENODEV;
> -		}
> -		return 0;
> -	}
> -
> -	if (cpu_is_omap15xx() || cpu_is_omap16xx()) {
> -		if (id > OMAP_MAX_MCBSP_COUNT - 1) {
> -			printk(KERN_ERR "OMAP-McBSP: McBSP%d doesn't exist\n",
> -				id + 1);
> -			return -ENODEV;
> -		}
> -		return 0;
> -	}
> -
> -	return -ENODEV;
> -}
> -
>  static void omap1_mcbsp_request(unsigned int id)
>  {
>  	/*
> @@ -151,7 +127,6 @@ static void omap1_mcbsp_free(unsigned int id)
>  }
>  
>  static struct omap_mcbsp_ops omap1_mcbsp_ops = {
> -	.check		= omap1_mcbsp_check,
Why can this function go away?  IMHO it should either go away in a
separate commit or be extended to be more general to apply for the 34xx
case, too.  (Same for omap2_mcbsp_check.)

> @@ -185,7 +226,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
>  #define OMAP34XX_MCBSP_PDATA_SZ		0
>  #endif
>  
> -int __init omap2_mcbsp_init(void)
> +static int __init omap2_mcbsp_init(void)
>  {
>  	int i;
>  
OK, this is trivial, but if you ask me, at least note it in the commit
message.

> @@ -196,9 +237,18 @@ int __init omap2_mcbsp_init(void)
>  	}
>  
>  	if (cpu_is_omap24xx())
> +		omap_mcbsp_count = OMAP24XX_MCBSP_PDATA_SZ;
> +	if (cpu_is_omap34xx())
> +		omap_mcbsp_count = OMAP34XX_MCBSP_PDATA_SZ;
> +
> +	mcbsp_ptr = kzalloc(omap_mcbsp_count * sizeof(struct omap_mcbsp *),
> +								GFP_KERNEL);
> +	if (!mcbsp_ptr)
> +		return -ENOMEM;
> +
> +	if (cpu_is_omap24xx())
>  		omap_mcbsp_register_board_cfg(omap24xx_mcbsp_pdata,
>  						OMAP24XX_MCBSP_PDATA_SZ);
> -
>  	if (cpu_is_omap34xx())
>  		omap_mcbsp_register_board_cfg(omap34xx_mcbsp_pdata,
>  						OMAP34XX_MCBSP_PDATA_SZ);
Hhm, it looks there is already some 34xx support in the file.  Should
the commit log be more detailed?

(Note, I didn't check the rest of the patch.)

Best regards
Uwe