[PATCH 3/6] of/spi: add support to parse the SPI flash's partitions

[Grant Likely]

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.

>> > 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.

g.