| Message ID | 1280735524-17547-4-git-send-email-Mingkai.hu@freescale.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Grant Likely |
| Headers | show |
On Mon, Aug 2, 2010 at 1:52 AM, Mingkai Hu <Mingkai.hu@freescale.com> wrote: > Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com> > --- > > v2: > - Move the flash partition function from of_spi.c to MTD driver > > drivers/mtd/devices/m25p80.c | 29 +++++++++++++++++++++++++++++ > 1 files changed, 29 insertions(+), 0 deletions(-) > > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index 81e49a9..5f00075 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -752,6 +752,31 @@ static const struct spi_device_id *__devinit jedec_probe(struct spi_device *spi) > return NULL; > } > > +/* > + * parse_flash_partition - Parse the flash partition on the SPI bus > + * @spi: Pointer to spi_device device > + */ > +void parse_flash_partition(struct spi_device *spi) > +{ > + struct mtd_partition *parts; > + struct flash_platform_data *pdata; > + int nr_parts = 0; > + struct device_node *np = spi->dev.of_node; > + > + nr_parts = of_mtd_parse_partitions(&spi->dev, np, &parts); > + if (!nr_parts) > + return; > + > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return; > + > + pdata->parts = parts; > + pdata->nr_parts = nr_parts; > + spi->dev.platform_data = pdata; The driver is not allowed to write to the platform_data pointer in the device structure because it leaves an absolute mess in trying to figure out who owns the pdata pointer. Should it be freed when the driver is unbound? Is it statically allocated? etc. There is a safer way to get the data (see below). > + > + return; > +} > > /* > * board specific setup should have ensured the SPI clock used here > @@ -771,6 +796,10 @@ static int __devinit m25p_probe(struct spi_device *spi) > * a chip ID, try the JEDEC id commands; they'll work for most > * newer chips, even if we don't recognize the particular chip. > */ > + > + /* Parse the flash partition */ > + parse_flash_partition(spi); > + Doing this unconditionally is dangerous, and if a platform_data structure is provided, then it must alwasy take precedence over the device tree data (because the platform code has gone to extra lengths to add a pdata structure; that must mean it is important!) :-) Instead what can be done is to call of_mtd_parse_partitions() directly from the probe routine after checking to see if the pdata structure exists and has partition information. If it doesn't, then call of_mtd_parse_partitions() to get values for parts and nr_parts. Doing it that way completely eliminates the uncertainty over pdata pointer ownership, and eliminates having an anonymous pointer buried in the driver. Cheers, g.
--- On Mon, 8/9/10, Grant Likely <grant.likely@secretlab.ca> wrote: > > + nr_parts = > of_mtd_parse_partitions(&spi->dev, np, &parts); Let's keep OF-specific logic out of drivers like this one ... intended to work without OF. NAK on adding dependencies like OF to drivers and other infrastructure that starts generic.
On Tue, Aug 10, 2010 at 1:14 AM, David Brownell <david-b@pacbell.net> wrote: > > > --- On Mon, 8/9/10, Grant Likely <grant.likely@secretlab.ca> wrote: > > >> > + nr_parts = >> of_mtd_parse_partitions(&spi->dev, np, &parts); > > Let's keep OF-specific logic out of drivers like > this one ... intended to work without OF. > > NAK on adding dependencies like OF to drivers > and other infrastructure that starts generic. The OF hooks compile to no-ops when CONFIG_OF is disabled. I've been very careful about that. g.
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index 81e49a9..5f00075 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -752,6 +752,31 @@ static const struct spi_device_id *__devinit jedec_probe(struct spi_device *spi) return NULL; } +/* + * parse_flash_partition - Parse the flash partition on the SPI bus + * @spi: Pointer to spi_device device + */ +void parse_flash_partition(struct spi_device *spi) +{ + struct mtd_partition *parts; + struct flash_platform_data *pdata; + int nr_parts = 0; + struct device_node *np = spi->dev.of_node; + + nr_parts = of_mtd_parse_partitions(&spi->dev, np, &parts); + if (!nr_parts) + return; + + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); + if (!pdata) + return; + + pdata->parts = parts; + pdata->nr_parts = nr_parts; + spi->dev.platform_data = pdata; + + return; +} /* * board specific setup should have ensured the SPI clock used here @@ -771,6 +796,10 @@ static int __devinit m25p_probe(struct spi_device *spi) * a chip ID, try the JEDEC id commands; they'll work for most * newer chips, even if we don't recognize the particular chip. */ + + /* Parse the flash partition */ + parse_flash_partition(spi); + data = spi->dev.platform_data; if (data && data->type) { const struct spi_device_id *plat_id;
Signed-off-by: Mingkai Hu <Mingkai.hu@freescale.com> --- v2: - Move the flash partition function from of_spi.c to MTD driver drivers/mtd/devices/m25p80.c | 29 +++++++++++++++++++++++++++++ 1 files changed, 29 insertions(+), 0 deletions(-)