Re: [PATCH 2/6] mtd: m25p80: Convert to device table matching

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 3 Aug 2009 19:54:50 -0700
David Brownell <david-b@pacbell.net> wrote:

> On Thursday 30 July 2009, Anton Vorontsov wrote:
> > This patch converts the m25p80 driver so that now it uses .id_table
> > for device matching, making it properly detect devices on OpenFirmware
> > platforms (prior to this patch the driver misdetected non-JEDEC chips,
> > seeing all chips as "m25p80").
> 
> I suspect "detect" is a misnomer there.  It only "detects" JEDEC chips.
> All others got explicit declarations ... so if there's misbehavior for
> other chips, it's because those declarations were poorly handled.  Maybe
> they were not properly flagged as non-JDEC
> 
>  
> > Also, now jedec_probe() only does jedec probing, nothing else. If it
> > is not able to detect a chip, NULL is returned and the driver fall
> > backs to the information specified by the platform (platform_data, or
> > exact ID).
> 
> I'd rather keep the warning, so there's a clue about what's really
> going on:  JEDEC chip found, but its ID is not handled.
> 

afaik there was no response to David's review comments, so this patch
is in the "stuck" state.


> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---
> >  drivers/mtd/devices/m25p80.c |  146 +++++++++++++++++++++++-------------------
> >  1 files changed, 80 insertions(+), 66 deletions(-)
> > 
> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > index 10ed195..0d74b38 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > 			... deletia ...
> 
> > @@ -481,74 +480,83 @@ struct flash_info {
> >  #define	SECT_4K		0x01		/* OPCODE_BE_4K works uniformly */
> >  };
> >  
> > +#define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\
> > +	((kernel_ulong_t)&(struct flash_info) {				\
> > +		.jedec_id = (_jedec_id),				\
> > +		.ext_id = (_ext_id),					\
> > +		.sector_size = (_sector_size),				\
> > +		.n_sectors = (_n_sectors),				\
> > +		.flags = (_flags),					\
> > +	})
> 
> Anonymous inlined structures ... kind of ugly, but I can
> understand why you might not want to declare and name a
> few dozen single-use structures.
> 
> 
> >  
> >  /* NOTE: double check command sets and memory organization when you add
> >   * more flash chips.  This current list focusses on newer chips, which
> >   * have been converging on command sets which including JEDEC ID.
> >   */
> > -static struct flash_info __devinitdata m25p_data [] = {
> > -
> > +static const struct spi_device_id m25p_ids[] = {
> >  	/* Atmel -- some are (confusingly) marketed as "DataFlash" */
> > -	{ "at25fs010",  0x1f6601, 0, 32 * 1024, 4, SECT_4K, },
> > -	{ "at25fs040",  0x1f6604, 0, 64 * 1024, 8, SECT_4K, },
> > +	{ "at25fs010",  INFO(0x1f6601, 0, 32 * 1024, 4, SECT_4K) },
> > +	{ "at25fs040",  INFO(0x1f6604, 0, 64 * 1024, 8, SECT_4K) },
> >  
> > 		... deletia ...
> >  
> 
> > @@ -596,6 +602,7 @@ static struct flash_info *__devinit jedec_probe(struct spi_device *spi)
> >   */
> >  static int __devinit m25p_probe(struct spi_device *spi)
> >  {
> > +	const struct spi_device_id	*id;
> >  	struct flash_platform_data	*data;
> >  	struct m25p			*flash;
> >  	struct flash_info		*info;
> > @@ -608,32 +615,38 @@ static int __devinit m25p_probe(struct spi_device *spi)
> >  	 */
> >  	data = spi->dev.platform_data;
> >  	if (data && data->type) {
> 
> At this point I wonder why you're not changing the probe sequence
> more.  Get "id" and then "id" here.  If it's for "m25p80" assume
> it's an old-style board init and do the current logic.  Else just
> verify "info".
> 
> There's a new error case of course:  new-style but data->type
> doesn't match id->name.
> 
> 
> > -		for (i = 0, info = m25p_data;
> > -				i < ARRAY_SIZE(m25p_data);
> > -				i++, info++) {
> > -			if (strcmp(data->type, info->name) == 0)
> > -				break;
> > +		for (i = 0; i < ARRAY_SIZE(m25p_ids) - 1; i++) {
> > +			id = &m25p_ids[i];
> > +			info = (void *)m25p_ids[i].driver_data;
> > +			if (strcmp(data->type, id->name))
> > +				continue;
> > +			break;
> >  		}
> >  
> >  		/* unrecognized chip? */
> > -		if (i == ARRAY_SIZE(m25p_data)) {
> > +		if (i == ARRAY_SIZE(m25p_ids) - 1) {
> 
> Better:  "if (info == NULL) ..."   You've got all the pointers
> in hand; don't use indices.
> 
> >  			DEBUG(MTD_DEBUG_LEVEL0, "%s: unrecognized id %s\n",
> >  					dev_name(&spi->dev), data->type);
> >  			info = NULL;
> >  
> >  		/* recognized; is that chip really what's there? */
> >  		} else if (info->jedec_id) {
> > -			struct flash_info	*chip = jedec_probe(spi);
> > +			id = jedec_probe(spi);
> >  
> > -			if (!chip || chip != info) {
> > +			if (id != &m25p_ids[i]) {
> 
> Again, don't use indices except during the lookup.
> 
> >  				dev_warn(&spi->dev, "found %s, expected %s\n",
> > -						chip ? chip->name : "UNKNOWN",
> > -						info->name);
> > +						id ? id->name : "UNKNOWN",
> > +						m25p_ids[i].name);
> >  				info = NULL;
> >  			}
> >  		}
> > -	} else
> > -		info = jedec_probe(spi);
> > +	} else {
> > +		id = jedec_probe(spi);
> > +		if (!id)
> > +			id = spi_get_device_id(spi);
> > +
> > +		info = (void *)id->driver_data;
> > +	}
> >  
> >  	if (!info)
> >  		return -ENODEV;
>