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

login
register
mail settings
Submitter Mingkai Hu
Date Sept. 30, 2010, 8 a.m.
Message ID <1285833646-12006-6-git-send-email-Mingkai.hu@freescale.com>
Download mbox | patch
Permalink /patch/66133/
State New
Headers show

Comments

Mingkai Hu - Sept. 30, 2010, 8 a.m.
Signed-off-by: Mingkai Hu <Mingkai.hu@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(-)
Grant Likely - Sept. 30, 2010, 9:34 p.m.
On Thu, Sep 30, 2010 at 5:00 PM, Mingkai Hu <Mingkai.hu@freescale.com> wrote:
> Signed-off-by: Mingkai Hu <Mingkai.hu@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@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?

g.
Hu Mingkai-B21284 - Oct. 8, 2010, 2:42 a.m.
> -----Original Message-----
> From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Grant
> Likely
> Sent: Friday, October 01, 2010 5:35 AM
> To: Hu Mingkai-B21284
> Cc: linuxppc-dev@ozlabs.org; spi-devel-general@lists.sourceforge.net; linux-
> mtd@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@freescale.com> wrote:
> > Signed-off-by: Mingkai Hu <Mingkai.hu@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@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

Patch

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;
+		}
+	}
+
+	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;
 		}
 
 		if (nr_parts > 0) {
@@ -937,9 +952,9 @@  static int __devinit m25p_probe(struct spi_device *spi)
 			flash->partitioned = 1;
 			return add_mtd_partitions(&flash->mtd, parts, nr_parts);
 		}
-	} else if (data && data->nr_parts)
+	} else if (pdata && pdata->nr_parts)
 		dev_warn(&spi->dev, "ignoring %d default partitions on %s\n",
-				data->nr_parts, data->name);
+				pdata->nr_parts, pdata->name);
 
 	return add_mtd_device(&flash->mtd) == 1 ? -ENODEV : 0;
 }