Re: [PATCH v3] spi: enable spi_board_info to be registered after spi_master

[Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Mon, 2 Aug 2010 13:53:25 +0800
Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> >  /**
> > @@ -365,6 +370,24 @@ struct spi_device *spi_new_device(struct
> > spi_master *master, }
> >  EXPORT_SYMBOL_GPL(spi_new_device);
> >
> > +/* Has to be called when board_lock is acquired */
> > +static void spi_scan_masterlist(struct spi_board_info *bi)
> > +{
> > +       struct spi_master *master;
> > +       struct spi_device *dev;
> > +
> > +       list_for_each_entry(master, &spi_master_list, list) {
> > +               if (master->bus_num != bi->bus_num)
> > +                       continue;
> > +
> > +               dev = spi_new_device(master, bi);
> > +               if (!dev)
> > +                       dev_err(master->dev.parent,
> > +                               "can't create new device for %s\n",
> > +                               bi->modalias);
> > +       }
> > +}
> > +
> 
> The abstraction isn't quite at the right place because the same code
> block is now duplicated twice.  Consider the following instead:
> 
> static void spi_match_master_to_boardinfo(struct spi_master *master,
> 				struct spi_board_info *bi)
> {
> 	struct spi_device *dev;
> 
> 	if (master->bus_num != bi->bus_num)
> 		return;
> 
> 	dev = spi_new_device(master, bi);
> 	if (!dev)
> 		dev_err(master->dev.parent, "can't create new device
> for %s\n", bi->modalias);
> }
> 
> And then from spi_register_board_info, do:
> 
> 	list_for_each_entry(master, &spi_master_list, list)
> 		spi_match_master_to_boardinfo(master, &bi->boardinfo);
> 
> And in spi_register_master, do:
> 
> 	list_for_each_entry(bi, &board_list, list)
> 		spi_match_master_to_boardinfo(master, &bi->boardinfo);
> 
> It will be less code that way.
> 
> Otherwise, this patch looks good to me.  Unfortunately, it's too late
> for 2.6.36, but I'll pick it up into my test branch in prep for the
> 2.6.37 cycle.
> 
> Cheers,
> g.

Yeah, make much sense! Thanks for the thorough reviews.

- Feng



------------------------------------------------------------------------------
The Palm PDK Hot Apps Program offers developers who use the
Plug-In Development Kit to bring their C/C++ apps to Palm for a share
of $1 Million in cash or HP Products. Visit us here for more details:
http://p.sf.net/sfu/dev2dev-palm