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

[Grant Likely <grant.likely@secretlab.ca>]

On Tue, Jul 20, 2010 at 10:08:22AM +0800, Mingkai Hu wrote:
> Signed-off-by: Mingkai Hu <Mingkai.hu@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.

>  		/* Register the new device */
>  		request_module(spi->modalias);
>  		rc = spi_add_device(spi);
> 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.

>  	kfree(spi->controller_state);
>  }
>  
> -- 
> 1.6.4
> 
>