* Uwe Kleine-König <ukleinek@xxxxxxxxx> [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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
