[PATCH v3 5/7] mtd: m25p80: add support to parse the SPI flash's partitions

[Hu Mingkai-B21284]

> -----Original Message-----
> From: glikely at secretlab.ca [mailto:glikely at secretlab.ca] On Behalf Of Grant
> Likely
> Sent: Friday, October 01, 2010 5:35 AM
> To: Hu Mingkai-B21284
> Cc: linuxppc-dev at ozlabs.org; spi-devel-general at lists.sourceforge.net; linux-
> mtd at lists.infradead.org; Gala Kumar-B11780; Zang Roy-R61911
> Subject: Re: [PATCH v3 5/7] mtd: m25p80: add support to parse the SPI flash's
> partitions
> 
> On Thu, Sep 30, 2010 at 5:00 PM, Mingkai Hu <Mingkai.hu at freescale.com> wrote:
> > Signed-off-by: Mingkai Hu <Mingkai.hu at freescale.com>
> > ---
> > v3:
> >  - Move the SPI flash partition code to the probe function.
> >
> >  drivers/mtd/devices/m25p80.c |   39 +++++++++++++++++++++++++++------------
> >  1 files changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > index 6f512b5..47d53c7 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -772,7 +772,7 @@ static const struct spi_device_id *__devinit
> jedec_probe(struct spi_device *spi)
> >  static int __devinit m25p_probe(struct spi_device *spi)
> >  {
> >        const struct spi_device_id      *id = spi_get_device_id(spi);
> > -       struct flash_platform_data      *data;
> > +       struct flash_platform_data      data, *pdata;
> >        struct m25p                     *flash;
> >        struct flash_info               *info;
> >        unsigned                        i;
> > @@ -782,13 +782,27 @@ static int __devinit m25p_probe(struct spi_device *spi)
> >         * a chip ID, try the JEDEC id commands; they'll work for most
> >         * newer chips, even if we don't recognize the particular chip.
> >         */
> > -       data = spi->dev.platform_data;
> > -       if (data && data->type) {
> > +       pdata = spi->dev.platform_data;
> > +       if (!pdata && spi->dev.of_node) {
> > +               int nr_parts;
> > +               struct mtd_partition *parts;
> > +               struct device_node *np = spi->dev.of_node;
> > +
> > +               nr_parts = of_mtd_parse_partitions(&spi->dev, np, &parts);
> > +               if (nr_parts) {
> > +                       pdata = &data;
> > +                       memset(pdata, 0, sizeof(*pdata));
> > +                       pdata->parts = parts;
> > +                       pdata->nr_parts = nr_parts;
> > +               }
> > +       }
> 
> Yes, this is the correct way to go about adding the partitions.
> However, this patch can be made simpler by not renaming 'data' to
> 'pdata' and by moving the above code down to just before the partition
> information is actually used.  in the OF case, only the parts and the
> nr_parts values written into data, and those values aren't used until
> the last part of the probe function.
> 
> Regardless, in principle this patch is correct:
> 
> Acked-by: Grant Likely <grant.likely at secretlab.ca>
> 
> > +
> > +       if (pdata && pdata->type) {
> >                const struct spi_device_id *plat_id;
> >
> >                for (i = 0; i < ARRAY_SIZE(m25p_ids) - 1; i++) {
> >                        plat_id = &m25p_ids[i];
> > -                       if (strcmp(data->type, plat_id->name))
> > +                       if (strcmp(pdata->type, plat_id->name))
> >                                continue;
> >                        break;
> >                }
> > @@ -796,7 +810,8 @@ static int __devinit m25p_probe(struct spi_device *spi)
> >                if (i < ARRAY_SIZE(m25p_ids) - 1)
> >                        id = plat_id;
> >                else
> > -                       dev_warn(&spi->dev, "unrecognized id %s\n", data-
> >type);
> > +                       dev_warn(&spi->dev, "unrecognized id %s\n",
> > +                                       pdata->type);
> >        }
> >
> >        info = (void *)id->driver_data;
> > @@ -847,8 +862,8 @@ static int __devinit m25p_probe(struct spi_device *spi)
> >                write_sr(flash, 0);
> >        }
> >
> > -       if (data && data->name)
> > -               flash->mtd.name = data->name;
> > +       if (pdata && pdata->name)
> > +               flash->mtd.name = pdata->name;
> >        else
> >                flash->mtd.name = dev_name(&spi->dev);
> >
> > @@ -919,9 +934,9 @@ static int __devinit m25p_probe(struct spi_device *spi)
> >                                        part_probes, &parts, 0);
> >                }
> >
> > -               if (nr_parts <= 0 && data && data->parts) {
> > -                       parts = data->parts;
> > -                       nr_parts = data->nr_parts;
> > +               if (nr_parts <= 0 && pdata && pdata->parts) {
> > +                       parts = pdata->parts;
> > +                       nr_parts = pdata->nr_parts;
> >                }
> 
> As per my comment earlier; since parts and nr_parts isn't needed
> before this point, this block could simply be:
> 
> if (nr_parts <= 0 && data && data->parts) {
>         parts = data->parts;
>         nr_parts = data->nr_parts;
> }
> if (nr_parts <= 0 && spi->dev.of_node)
>         nr_parts = of_mtd_parse_partitions(&spi->dev, np, &parts);
> 
> And most of the other changes to this file goes away.  Simpler, yes?
> 

Yes, you're right, I'll fix it. Also thanks for your suggestion and ACK.

Thanks,
Mingkai