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

[Tony Lindgren <tony@atomide.com>]

* Uwe Kleine-König <ukleinek@strlen.de> [080924 14:58]:
> 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

Sure, I think it's a bit hard to read too. Will repost split.

> 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.)

It also removes omap2_mcbsp_check. There's now:
#define omap_mcbsp_check_valid_id() (id < omap_mcbsp_count)

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

Will add.

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

Will split into a separate patch.

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

Thanks for looking!

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html