[PATCH] m25p80: add support for a callback to platform code on successful device probe

[PATCH] m25p80: add support for a callback to platform code on successful device probe

[Sudhakar Rajashekhara]

On some platforms, MTD device can hold MAC address, calibration
data, serial numbers etc.

On TI's DA850/OMAP-L138/AM18xx EVM, MAC address is stored on
the last sector of the SPI flash, which is exported as an MTD
device.

This patch adds two new members to the 'flash_platform_data'
structure, a function handler which will be called after
add_mtd_device() and an argument to be passed to this function,
for example: offset to read from.

Signed-off-by: Sudhakar Rajashekhara <sudhakar.raj@ti.com>
---
 drivers/mtd/devices/m25p80.c |   15 +++++++++++++--
 include/linux/spi/flash.h    |    2 ++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 81e49a9..b832091 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -765,6 +765,7 @@ static int __devinit m25p_probe(struct spi_device *spi)
 	struct m25p			*flash;
 	struct flash_info		*info;
 	unsigned			i;
+	int				ret;
 
 	/* Platform data helps sort out which chip type we have, as
 	 * well as how this board partitions it.  If we don't have
@@ -924,13 +925,23 @@ static int __devinit m25p_probe(struct spi_device *spi)
 					(long long)(parts[i].size >> 10));
 			}
 			flash->partitioned = 1;
-			return add_mtd_partitions(&flash->mtd, parts, nr_parts);
+			ret = add_mtd_partitions(&flash->mtd, parts, nr_parts);
+			if (!ret)
+				goto out;
+
+			return ret;
 		}
 	} else if (data && data->nr_parts)
 		dev_warn(&spi->dev, "ignoring %d default partitions on %s\n",
 				data->nr_parts, data->name);
 
-	return add_mtd_device(&flash->mtd) == 1 ? -ENODEV : 0;
+	ret = add_mtd_device(&flash->mtd);
+
+out:
+	if (data->setup && !ret)
+		(data->setup)(&flash->mtd, (void *)data->context);
+
+	return (ret == 1) ? -ENODEV : 0;
 }
 
 
diff --git a/include/linux/spi/flash.h b/include/linux/spi/flash.h
index 3f22932..daa4875 100644
--- a/include/linux/spi/flash.h
+++ b/include/linux/spi/flash.h
@@ -24,6 +24,8 @@ struct flash_platform_data {
 	unsigned int	nr_parts;
 
 	char		*type;
+	void		(*setup)(struct mtd_info *, void *context);
+	void		*context;
 
 	/* we'll likely add more ... use JEDEC IDs, etc */
 };
-- 
1.5.6

Re: [PATCH] m25p80: add support for a callback to platform code on successful device probe

[Mike Frysinger]

On Tue, Aug 10, 2010 at 02:42, Sudhakar Rajashekhara wrote:
> On some platforms, MTD device can hold MAC address, calibration
> data, serial numbers etc.

pretty standard behavior ...

> On TI's DA850/OMAP-L138/AM18xx EVM, MAC address is stored on
> the last sector of the SPI flash, which is exported as an MTD
> device.

again, nothing unique here ...

> This patch adds two new members to the 'flash_platform_data'
> structure, a function handler which will be called after
> add_mtd_device() and an argument to be passed to this function,
> for example: offset to read from.

... and your changelog falls apart.  the intro gives no basis that i
can see as to why you need these hooks.

please give examples of what you're actually trying to accomplish and
how these hooks help you accomplish said goal and why it is necessary
that you do these things from kernel space instead of standard
userspace.
-mike

RE: [PATCH] m25p80: add support for a callback to platform code on successful device probe

[Sudhakar Rajashekhara]

Hi,

On Tue, Aug 10, 2010 at 12:47:05, Mike Frysinger wrote:
> On Tue, Aug 10, 2010 at 02:42, Sudhakar Rajashekhara wrote:
> > On some platforms, MTD device can hold MAC address, calibration
> > data, serial numbers etc.
> 
> pretty standard behavior ...
> 
> > On TI's DA850/OMAP-L138/AM18xx EVM, MAC address is stored on
> > the last sector of the SPI flash, which is exported as an MTD
> > device.
> 
> again, nothing unique here ...
> 
> > This patch adds two new members to the 'flash_platform_data'
> > structure, a function handler which will be called after
> > add_mtd_device() and an argument to be passed to this function,
> > for example: offset to read from.
> 
> ... and your changelog falls apart.  the intro gives no basis that i
> can see as to why you need these hooks.
> 
> please give examples of what you're actually trying to accomplish and
> how these hooks help you accomplish said goal and why it is necessary
> that you do these things from kernel space instead of standard
> userspace.

I have submitted an updated version of this patch in which I have
addressed your concerns.

Regards,
Sudhakar

RE: [PATCH] m25p80: add support for a callback to platform code on successful device probe

[Sudhakar Rajashekhara]

On Tue, Aug 10, 2010 at 18:37:47, David Brownell wrote:
> 
> > On Tue, Aug 10, 2010 at 12:47:05, Mike Frysinger wrote:
> 
> > it is necessary
> > > that you do these things from kernel space instead of
> > standard
> > > userspace.
> 
> Hard to get userspace to do this stuff if
> you're doing a network boot, and thus need
> to have the MAC address working early.. it's
> easy if the kernel can get the MAC address,
> else not possible to boot.  Right?
> 

You are absolutely right.

- Sudhakar

RE: [PATCH] m25p80: add support for a callback to platform code on successful device probe

[David Brownell]

> On Tue, Aug 10, 2010 at 12:47:05, Mike Frysinger wrote:

> it is necessary
> > that you do these things from kernel space instead of
> standard
> > userspace.

Hard to get userspace to do this stuff if
you're doing a network boot, and thus need
to have the MAC address working early.. it's
easy if the kernel can get the MAC address,
else not possible to boot.  Right?

Re: [PATCH] m25p80: add support for a callback to platform code on successful device probe

[Sergei Shtylyov]

Hello.

Sudhakar Rajashekhara wrote:

> On some platforms, MTD device can hold MAC address, calibration
> data, serial numbers etc.

> On TI's DA850/OMAP-L138/AM18xx EVM, MAC address is stored on
> the last sector of the SPI flash, which is exported as an MTD
> device.

> This patch adds two new members to the 'flash_platform_data'
> structure, a function handler which will be called after
> add_mtd_device() and an argument to be passed to this function,
> for example: offset to read from.

> Signed-off-by: Sudhakar Rajashekhara <sudhakar.raj@ti.com>
> ---
>  drivers/mtd/devices/m25p80.c |   15 +++++++++++++--
>  include/linux/spi/flash.h    |    2 ++
>  2 files changed, 15 insertions(+), 2 deletions(-)

> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 81e49a9..b832091 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
[...]
> @@ -924,13 +925,23 @@ static int __devinit m25p_probe(struct spi_device *spi)
>  					(long long)(parts[i].size >> 10));
>  			}
>  			flash->partitioned = 1;
> -			return add_mtd_partitions(&flash->mtd, parts, nr_parts);
> +			ret = add_mtd_partitions(&flash->mtd, parts, nr_parts);
> +			if (!ret)
> +				goto out;
> +
> +			return ret;
>  		}
>  	} else if (data && data->nr_parts)
>  		dev_warn(&spi->dev, "ignoring %d default partitions on %s\n",
>  				data->nr_parts, data->name);
>  
> -	return add_mtd_device(&flash->mtd) == 1 ? -ENODEV : 0;
> +	ret = add_mtd_device(&flash->mtd);
> +
> +out:
> +	if (data->setup && !ret)
> +		(data->setup)(&flash->mtd, (void *)data->context);

    Parens not necessary around 'data->setup'.

> diff --git a/include/linux/spi/flash.h b/include/linux/spi/flash.h
> index 3f22932..daa4875 100644
> --- a/include/linux/spi/flash.h
> +++ b/include/linux/spi/flash.h
> @@ -24,6 +24,8 @@ struct flash_platform_data {
>  	unsigned int	nr_parts;
>  
>  	char		*type;
> +	void		(*setup)(struct mtd_info *, void *context);
> +	void		*context;
>  
>  	/* we'll likely add more ... use JEDEC IDs, etc */
>  };

    Hm, is m25p80.c the only SPI flash driver using this structure? I guess 
not -- I'm seeing at least sst25l.c and mtd_dataflash.c...

WBR, Sergei

Re: [PATCH] m25p80: add support for a callback to platform code on successful device probe

[Mike Frysinger]

On Tue, Aug 10, 2010 at 09:07, David Brownell wrote:
>> On Tue, Aug 10, 2010 at 12:47:05, Mike Frysinger wrote:
>> it is necessary
>> > that you do these things from kernel space instead of
>> standard
>> > userspace.
>
> Hard to get userspace to do this stuff if
> you're doing a network boot, and thus need
> to have the MAC address working early.. it's
> easy if the kernel can get the MAC address,
> else not possible to boot.  Right?

no, that isnt true at all.  you can easily have a small initramfs to
handle all of your random setup.  if that's too much overhead, you can
add a late initcall to your boards file that is run after the MTD
devices are probed and let that read the required information.  all of
this is more than documented:
http://www.denx.de/wiki/view/DULG/EthernetDoesNotWorkInLinux

i still see these callbacks as unnecessary overhead to solve problems
that can already be done multiple ways with existing code.
-mike

RE: [PATCH] m25p80: add support for a callback to platform code on successful device probe

[Sudhakar Rajashekhara]

Hi Mike,

On Wed, Aug 11, 2010 at 00:32:00, Mike Frysinger wrote:
> On Tue, Aug 10, 2010 at 09:07, David Brownell wrote:
> >> On Tue, Aug 10, 2010 at 12:47:05, Mike Frysinger wrote:
> >> it is necessary
> >> > that you do these things from kernel space instead of
> >> standard
> >> > userspace.
> >
> > Hard to get userspace to do this stuff if
> > you're doing a network boot, and thus need
> > to have the MAC address working early.. it's
> > easy if the kernel can get the MAC address,
> > else not possible to boot.  Right?
> 
> no, that isnt true at all.  you can easily have a small initramfs to
> handle all of your random setup.  if that's too much overhead, you can
> add a late initcall to your boards file that is run after the MTD
> devices are probed and let that read the required information.  all of
> this is more than documented:
> http://www.denx.de/wiki/view/DULG/EthernetDoesNotWorkInLinux
> 

I looked around in the kernel and found mtd_notifier callbacks which get
called upon addition of MTD device. Inside this callback, I am checking
for the device in which I am interested and doing a mtd->read to get the
MAC address. So this patch can be dropped.

Thanks,
Sudhakar

Re: [PATCH] m25p80: add support for a callback to platform code on successful device probe

[Mike Frysinger]

On Wed, Aug 11, 2010 at 07:56, Sudhakar Rajashekhara wrote:
> On Wed, Aug 11, 2010 at 00:32:00, Mike Frysinger wrote:
>> On Tue, Aug 10, 2010 at 09:07, David Brownell wrote:
>> >> On Tue, Aug 10, 2010 at 12:47:05, Mike Frysinger wrote:
>> >> it is necessary
>> >> > that you do these things from kernel space instead of
>> >> standard
>> >> > userspace.
>> >
>> > Hard to get userspace to do this stuff if
>> > you're doing a network boot, and thus need
>> > to have the MAC address working early.. it's
>> > easy if the kernel can get the MAC address,
>> > else not possible to boot.  Right?
>>
>> no, that isnt true at all.  you can easily have a small initramfs to
>> handle all of your random setup.  if that's too much overhead, you can
>> add a late initcall to your boards file that is run after the MTD
>> devices are probed and let that read the required information.  all of
>> this is more than documented:
>> http://www.denx.de/wiki/view/DULG/EthernetDoesNotWorkInLinux
>
> I looked around in the kernel and found mtd_notifier callbacks which get
> called upon addition of MTD device. Inside this callback, I am checking
> for the device in which I am interested and doing a mtd->read to get the
> MAC address. So this patch can be dropped.

thanks for the follow up explanation
-mike