[PATCH 3/6] of/spi: 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: Monday, July 26, 2010 3:53 PM
> To: Hu Mingkai-B21284
> Cc: linuxppc-dev at ozlabs.org; galak at kernel.crashing.org; Zang 
> Roy-R61911
> Subject: Re: [PATCH 3/6] of/spi: add support to parse the SPI 
> flash's partitions
> 
> On Mon, Jul 26, 2010 at 1:25 AM, Hu Mingkai-B21284 
> <B21284 at freescale.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Grant Likely [mailto:glikely at secretlab.ca] On Behalf 
> Of Grant 
> >> Likely
> >> Sent: Monday, July 26, 2010 8:28 AM
> >> To: Hu Mingkai-B21284
> >> Cc: linuxppc-dev at ozlabs.org; galak at kernel.crashing.org; Zang
> >> Roy-R61911
> >> Subject: Re: [PATCH 3/6] of/spi: add support to parse the 
> SPI flash's 
> >> partitions
> >>
> >> On Tue, Jul 20, 2010 at 10:08:22AM +0800, Mingkai Hu wrote:
> >> > Signed-off-by: Mingkai Hu <Mingkai.hu at freescale.com>
> >> > ---
> >> >  drivers/of/of_spi.c       |   11 +++++++++++
> >> >  drivers/spi/spi_mpc8xxx.c |    1 +
> >> >  2 files changed, 12 insertions(+), 0 deletions(-)
> >> >
> >> > diff --git a/drivers/of/of_spi.c b/drivers/of/of_spi.c index 
> >> > 5fed7e3..284ca0e 100644
> >> > --- a/drivers/of/of_spi.c
> >> > +++ b/drivers/of/of_spi.c
> >> > @@ -10,6 +10,8 @@
> >> >  #include <linux/device.h>
> >> >  #include <linux/spi/spi.h>
> >> >  #include <linux/of_spi.h>
> >> > +#include <linux/spi/flash.h>
> >> > +#include <linux/mtd/partitions.h>
> >> >
> >> >  /**
> >> >   * of_register_spi_devices - Register child devices onto
> >> the SPI bus
> >> > @@ -26,6 +28,7 @@ void of_register_spi_devices(struct
> >> spi_master *master, struct device_node *np)
> >> >     const __be32 *prop;
> >> >     int rc;
> >> >     int len;
> >> > +   struct flash_platform_data *pdata;
> >> >
> >> >     for_each_child_of_node(np, nc) {
> >> >             /* Alloc an spi_device */ @@ -81,6 +84,14 @@ void 
> >> > of_register_spi_devices(struct
> >> spi_master *master, struct device_node *np)
> >> >             of_node_get(nc);
> >> >             spi->dev.of_node = nc;
> >> >
> >> > +           /* Parse the mtd partitions */
> >> > +           pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> >> > +           if (!pdata)
> >> > +                   return;
> >> > +           pdata->nr_parts = 
> of_mtd_parse_partitions(&master->dev,
> >> > +                           nc, &pdata->parts);
> >> > +           spi->dev.platform_data = pdata;
> >> > +
> >>
> >> Nack.  Not all spi devices are mtd devices.  In fact, most are not.
> >>
> >> The spi driver itself should call the 
> of_mtd_parse_partitions code to 
> >> get the partition map.  Do not use pdata in this case.
> >>
> >
> > Yes, we can call of_mtd_parse_partitions to get the 
> partiton map, but 
> > how can we pass this map to spi device to register to MTD layer?
> >
> > The spi device is created and registered when call 
> > of_register_spi_device in the spi controller probe 
> function, then the 
> > spi device will traverse the spi device driver list to find 
> the proper 
> > driver, if matched, then call the spi device driver probe 
> code where 
> > the mtd partition info is registered to the mtd layer.
> >
> > so where is the proper place to put the 
> of_mtd_parse_partitions code?
> 
> In the device driver probe code.
> 

This is the way that I did at first, thus we need to add the same code
in all the spi flash driver to get partition map info.

or we add the get partition map code to of_spi.c?

+/*
+ * of_parse_flash_partition - Parse the flash partition on the SPI bus
+ * @spi:       Pointer to spi_device device
+ */
+void of_parse_flash_partition(struct spi_device *spi)
+{
+       struct mtd_partition *parts;
+       struct flash_platform_data *spi_pdata;
+       int nr_parts = 0;
+       static int num_flash;
+       struct device_node *np = spi->dev.archdata.of_node;
+
+       nr_parts = of_mtd_parse_partitions(&spi->dev, np, &parts);
+       if (!nr_parts)
+               goto end;
+
+       spi_pdata = kzalloc(sizeof(*spi_pdata), GFP_KERNEL);
+       if (!spi_pdata)
+               goto end;
+       spi_pdata->name = kzalloc(10, GFP_KERNEL);
+       if (!spi_pdata->name)
+               goto free_flash;
+       snprintf(spi_pdata->name, 10, "SPIFLASH%d", num_flash++);
+
+       spi_pdata->parts = parts;
+       spi_pdata->nr_parts = nr_parts;
+
+       spi->dev.platform_data = spi_pdata;
+
+       return;
+
+free_flash:
+       kfree(spi_pdata);
+end:
+       return;
+}


> >> > diff --git a/drivers/spi/spi_mpc8xxx.c 
> b/drivers/spi/spi_mpc8xxx.c 
> >> > index efed70e..0fadaeb 100644
> >> > --- a/drivers/spi/spi_mpc8xxx.c
> >> > +++ b/drivers/spi/spi_mpc8xxx.c
> >> > @@ -137,6 +137,7 @@ int mpc8xxx_spi_transfer(struct spi_device 
> >> > *spi,
> >> >
> >> >  void mpc8xxx_spi_cleanup(struct spi_device *spi)  {
> >> > +   kfree(spi->dev.platform_data);
> >>
> >> Irrelevant given the above comment, but how does this even work?  
> >> What if a driver was detached and reattached to an 
> spi_device?  The 
> >> platform_data would be freed.  Not to mention that the 
> pointer isn't 
> >> cleared, so the driver would have no idea that it has a freed 
> >> pointer.
> >>
> >
> > It works. If the driver detached, the spi device 
> platform_data will be 
> > released, when reattached, the of_register_spi_devices will 
> be called 
> > again, the platform_data will be allocated.
> 
> Ah right, I wasn't looking in the right place.  On that note 
> however, the cleanup of spi_device allocated by the OF code 
> should also have a reciprocal helper that frees them too.  It 
> shouldn't be done in the individual drivers.
> 

We can define a helper to free the platform_data, but where it should be called?
Maybe spidev_release is a better place to call, after all the platfrom_data is allocated
when the spi device is created, so it should be freed when released.

Thanks,
Mingkai