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

[Grant Likely]

[cc'ing spi-devel-general]

On Mon, Jul 26, 2010 at 2:20 AM, Hu Mingkai-B21284 <B21284 at freescale.com> wrote:
>
>
>> -----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?

No, it really does not belong in of_spi.c because the spi code has no
knowledge about MTD devices.

I only see two valid options, to either:
a) add it to each MTD driver, or
b) add a hook to the core MTD code so that if an of_node pointer is
provided, and no partition data is provided, then it will parse the
partitions when the MTD device is registered.  You'd also need to code
it in a way that becomes a no-op when CONFIG_OF is not set.

>
> +/*
> + * 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.

It would be called from the spi core code.  However, with two options
I gave above, platform_data shouldn't be needed at all.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.