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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to