[cc'ing spi-devel-general]
On Mon, Jul 26, 2010 at 2:20 AM, Hu Mingkai-B21284 <b21...@freescale.com> wrote:
>
>
>> -----Original Message-----
>> From: glik...@secretlab.ca [mailto:glik...@secretlab.ca] On
>> Behalf Of Grant Likely
>> Sent: Monday, July 26, 2010 3:53 PM
>> To: Hu Mingkai-B21284
>> Cc: linuxppc-...@ozlabs.org; ga...@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
>> <b21...@freescale.com> wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Grant Likely [mailto:glik...@secretlab.ca] On Behalf
>> Of Grant
>> >> Likely
>> >> Sent: Monday, July 26, 2010 8:28 AM
>> >> To: Hu Mingkai-B21284
>> >> Cc: linuxppc-...@ozlabs.org; ga...@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...@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.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
