diff mbox

[v2] mtd: dataflash: add device tree probe support

Message ID 1310658757-17018-1-git-send-email-shawn.guo@linaro.org
State New
Headers show

Commit Message

Shawn Guo July 14, 2011, 3:52 p.m. UTC 
It adds device tree probe support for mtd_dataflash driver.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
Changes since v1:
 * Add "atmel,at45xxx" into the match table
 * Add binding document

 .../devicetree/bindings/mtd/atmel-dataflash.txt    |   14 ++++++++++++++
 drivers/mtd/devices/mtd_dataflash.c                |   13 ++++++++++++-
 2 files changed, 26 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/atmel-dataflash.txt

Comments

Mike Frysinger July 14, 2011, 5:10 p.m. UTC | #1 
shouldnt there be #ifdef's around this ?  not everyone supports devicetrees.
-mike
Dmitry Baryshkov July 14, 2011, 5:52 p.m. UTC | #2 
On Thu, Jul 14, 2011 at 11:52:37PM +0800, Shawn Guo wrote:
> It adds device tree probe support for mtd_dataflash driver.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> ---
> Changes since v1:
>  * Add "atmel,at45xxx" into the match table
>  * Add binding document

I´m sorry for being unclean on this. Device family is just at45,
so I´d suggest the following compatibility list:
"atmel,at45d789000", "atmel,at45"

You might want to add "atmel,dataflash" or "mtd,dataflash", but I see
no point in that.
Grant Likely July 15, 2011, 2:54 a.m. UTC | #3 
On Thu, Jul 14, 2011 at 09:52:02PM +0400, Dmitry Eremin-Solenikov wrote:
> On Thu, Jul 14, 2011 at 11:52:37PM +0800, Shawn Guo wrote:
> > It adds device tree probe support for mtd_dataflash driver.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> > ---
> > Changes since v1:
> >  * Add "atmel,at45xxx" into the match table
> >  * Add binding document
> 
> I´m sorry for being unclean on this. Device family is just at45,
> so I´d suggest the following compatibility list:
> "atmel,at45d789000", "atmel,at45"
> 
> You might want to add "atmel,dataflash" or "mtd,dataflash", but I see
> no point in that.

Dmitry is right.  at45xxx is not a good compatible value.

> 
> -- 
> With best wishes
> Dmitry
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Grant Likely July 15, 2011, 2:54 a.m. UTC | #4 
On Thu, Jul 14, 2011 at 11:52:37PM +0800, Shawn Guo wrote:
> It adds device tree probe support for mtd_dataflash driver.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> ---
> Changes since v1:
>  * Add "atmel,at45xxx" into the match table
>  * Add binding document
> 
>  .../devicetree/bindings/mtd/atmel-dataflash.txt    |   14 ++++++++++++++
>  drivers/mtd/devices/mtd_dataflash.c                |   13 ++++++++++++-
>  2 files changed, 26 insertions(+), 1 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mtd/atmel-dataflash.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-dataflash.txt b/Documentation/devicetree/bindings/mtd/atmel-dataflash.txt
> new file mode 100644
> index 0000000..3783962
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/atmel-dataflash.txt
> @@ -0,0 +1,14 @@
> +* Atmel Data Flash
> +
> +Required properties:
> +- compatible : "atmel,<model>", "atmel,<series>", "atmel,dataflash".
> +
> +Example:
> +
> +flash@1 {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	compatible = "atmel,at45db321d", "atmel,at45xxx", "atmel,dataflash";
> +	spi-max-frequency = <25000000>;
> +	reg = <1>;
> +};
> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
> index 8f6b02c..8e091b0 100644
> --- a/drivers/mtd/devices/mtd_dataflash.c
> +++ b/drivers/mtd/devices/mtd_dataflash.c
> @@ -24,6 +24,8 @@
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/partitions.h>
>  
> +#include <linux/of.h>
> +#include <linux/of_device.h>

This shouldn't be separate from the regular block of #include
statements above it.
>  
>  /*
>   * DataFlash is a kind of SPI flash.  Most AT45 chips have two buffers in
> @@ -98,6 +100,12 @@ struct dataflash {
>  	struct mtd_info		mtd;
>  };
>  
> +static const struct of_device_id dataflash_dt_ids[] = {
> +	{ .compatible = "atmel,at45xxx", },
> +	{ .compatible = "atmel,dataflash", },
> +	{ /* sentinel */ }
> +};
> +

This should be protected with a #ifdef CONFIG_OF/#else/#endif, and
there should be a MODULE_DEVICE_TABLE().

>  /* ......................................................................... */
>  
>  /*
> @@ -634,6 +642,7 @@ add_dataflash_otp(struct spi_device *spi, char *name,
>  {
>  	struct dataflash		*priv;
>  	struct mtd_info			*device;
> +	struct mtd_part_parser_data	ppdata;
>  	struct flash_platform_data	*pdata = spi->dev.platform_data;
>  	char				*otp_tag = "";
>  	int				err = 0;
> @@ -675,7 +684,8 @@ add_dataflash_otp(struct spi_device *spi, char *name,
>  			pagesize, otp_tag);
>  	dev_set_drvdata(&spi->dev, priv);
>  
> -	err = mtd_device_parse_register(device, NULL, 0,
> +	ppdata.of_node = spi->dev.of_node;
> +	err = mtd_device_parse_register(device, NULL, &ppdata,
>  			pdata ? pdata->parts : NULL,
>  			pdata ? pdata->nr_parts : 0);
>  
> @@ -926,6 +936,7 @@ static struct spi_driver dataflash_driver = {
>  		.name		= "mtd_dataflash",
>  		.bus		= &spi_bus_type,
>  		.owner		= THIS_MODULE,
> +		.of_match_table = dataflash_dt_ids,
>  	},
>  
>  	.probe		= dataflash_probe,
> -- 
> 1.7.4.1
>
Shawn Guo July 15, 2011, 4:42 a.m. UTC | #5 
On Thu, Jul 14, 2011 at 09:52:02PM +0400, Dmitry Eremin-Solenikov wrote:
> On Thu, Jul 14, 2011 at 11:52:37PM +0800, Shawn Guo wrote:
> > It adds device tree probe support for mtd_dataflash driver.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> > ---
> > Changes since v1:
> >  * Add "atmel,at45xxx" into the match table
> >  * Add binding document
> 
> I´m sorry for being unclean on this. Device family is just at45,
> so I´d suggest the following compatibility list:
> "atmel,at45d789000", "atmel,at45"
> 
Ok.

> You might want to add "atmel,dataflash" or "mtd,dataflash", but I see
> no point in that.
> 
The binding I'm adding is for atmel dataflash (following the driver
name).  I want the binding to be scalable to cover possible new
atmel dataflash family other than at45.  We do not want to change
binding every time one new family comes out, do we?
Shawn Guo July 15, 2011, 4:49 a.m. UTC | #6 
On Thu, Jul 14, 2011 at 08:54:07PM -0600, Grant Likely wrote:
> On Thu, Jul 14, 2011 at 11:52:37PM +0800, Shawn Guo wrote:
> > It adds device tree probe support for mtd_dataflash driver.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> > ---
> > Changes since v1:
> >  * Add "atmel,at45xxx" into the match table
> >  * Add binding document
> > 
> >  .../devicetree/bindings/mtd/atmel-dataflash.txt    |   14 ++++++++++++++
> >  drivers/mtd/devices/mtd_dataflash.c                |   13 ++++++++++++-
> >  2 files changed, 26 insertions(+), 1 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/mtd/atmel-dataflash.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/atmel-dataflash.txt b/Documentation/devicetree/bindings/mtd/atmel-dataflash.txt
> > new file mode 100644
> > index 0000000..3783962
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/atmel-dataflash.txt
> > @@ -0,0 +1,14 @@
> > +* Atmel Data Flash
> > +
> > +Required properties:
> > +- compatible : "atmel,<model>", "atmel,<series>", "atmel,dataflash".
> > +
> > +Example:
> > +
> > +flash@1 {
> > +	#address-cells = <1>;
> > +	#size-cells = <1>;
> > +	compatible = "atmel,at45db321d", "atmel,at45xxx", "atmel,dataflash";
> > +	spi-max-frequency = <25000000>;
> > +	reg = <1>;
> > +};
> > diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
> > index 8f6b02c..8e091b0 100644
> > --- a/drivers/mtd/devices/mtd_dataflash.c
> > +++ b/drivers/mtd/devices/mtd_dataflash.c
> > @@ -24,6 +24,8 @@
> >  #include <linux/mtd/mtd.h>
> >  #include <linux/mtd/partitions.h>
> >  
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> 
> This shouldn't be separate from the regular block of #include
> statements above it.

The patch does not show the context.  Here is the existing code.  I
just followed the pattern.

#include <linux/module.h>
#include <linux/init.h>
#include <linux/slab.h>
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/mutex.h>
#include <linux/err.h>
#include <linux/math64.h>

#include <linux/spi/spi.h>
#include <linux/spi/flash.h>

#include <linux/mtd/mtd.h>
#include <linux/mtd/partitions.h>

But okay, I will add the OF headers right behind math64.h to not make
it any worse.

> >  
> >  /*
> >   * DataFlash is a kind of SPI flash.  Most AT45 chips have two buffers in
> > @@ -98,6 +100,12 @@ struct dataflash {
> >  	struct mtd_info		mtd;
> >  };
> >  
> > +static const struct of_device_id dataflash_dt_ids[] = {
> > +	{ .compatible = "atmel,at45xxx", },
> > +	{ .compatible = "atmel,dataflash", },
> > +	{ /* sentinel */ }
> > +};
> > +
> 
> This should be protected with a #ifdef CONFIG_OF/#else/#endif, and
> there should be a MODULE_DEVICE_TABLE().
> 
I personally hate #ifdef stuff.  But okay, I can do it since there
are people being concerned by this little waste of space.

Regards,
Shawn

> >  /* ......................................................................... */
> >  
> >  /*
> > @@ -634,6 +642,7 @@ add_dataflash_otp(struct spi_device *spi, char *name,
> >  {
> >  	struct dataflash		*priv;
> >  	struct mtd_info			*device;
> > +	struct mtd_part_parser_data	ppdata;
> >  	struct flash_platform_data	*pdata = spi->dev.platform_data;
> >  	char				*otp_tag = "";
> >  	int				err = 0;
> > @@ -675,7 +684,8 @@ add_dataflash_otp(struct spi_device *spi, char *name,
> >  			pagesize, otp_tag);
> >  	dev_set_drvdata(&spi->dev, priv);
> >  
> > -	err = mtd_device_parse_register(device, NULL, 0,
> > +	ppdata.of_node = spi->dev.of_node;
> > +	err = mtd_device_parse_register(device, NULL, &ppdata,
> >  			pdata ? pdata->parts : NULL,
> >  			pdata ? pdata->nr_parts : 0);
> >  
> > @@ -926,6 +936,7 @@ static struct spi_driver dataflash_driver = {
> >  		.name		= "mtd_dataflash",
> >  		.bus		= &spi_bus_type,
> >  		.owner		= THIS_MODULE,
> > +		.of_match_table = dataflash_dt_ids,
> >  	},
> >  
> >  	.probe		= dataflash_probe,
> > -- 
> > 1.7.4.1
> > 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
Artem Bityutskiy July 20, 2011, 4:40 a.m. UTC | #7 
On Fri, 2011-07-15 at 12:49 +0800, Shawn Guo wrote:
> > > +static const struct of_device_id dataflash_dt_ids[] = {
> > > +	{ .compatible = "atmel,at45xxx", },
> > > +	{ .compatible = "atmel,dataflash", },
> > > +	{ /* sentinel */ }
> > > +};
> > > +
> > 
> > This should be protected with a #ifdef CONFIG_OF/#else/#endif, and
> > there should be a MODULE_DEVICE_TABLE().
> > 
> I personally hate #ifdef stuff.  But okay, I can do it since there
> are people being concerned by this little waste of space.

I guess the question is - will it compile and work if CONFIG_OF is
unset?
Shawn Guo July 20, 2011, 4:55 a.m. UTC | #8 
On Wed, Jul 20, 2011 at 07:40:38AM +0300, Artem Bityutskiy wrote:
> On Fri, 2011-07-15 at 12:49 +0800, Shawn Guo wrote:
> > > > +static const struct of_device_id dataflash_dt_ids[] = {
> > > > +	{ .compatible = "atmel,at45xxx", },
> > > > +	{ .compatible = "atmel,dataflash", },
> > > > +	{ /* sentinel */ }
> > > > +};
> > > > +
> > > 
> > > This should be protected with a #ifdef CONFIG_OF/#else/#endif, and
> > > there should be a MODULE_DEVICE_TABLE().
> > > 
> > I personally hate #ifdef stuff.  But okay, I can do it since there
> > are people being concerned by this little waste of space.
> 
> I guess the question is - will it compile and work if CONFIG_OF is
> unset?
> 
Yes, it will compile, as 'struct of_device_id' is defined in
include/linux/mod_devicetable.h unconditionally.
Artem Bityutskiy July 20, 2011, 5:17 a.m. UTC | #9 
On Wed, 2011-07-20 at 12:55 +0800, Shawn Guo wrote:
> On Wed, Jul 20, 2011 at 07:40:38AM +0300, Artem Bityutskiy wrote:
> > On Fri, 2011-07-15 at 12:49 +0800, Shawn Guo wrote:
> > > > > +static const struct of_device_id dataflash_dt_ids[] = {
> > > > > +	{ .compatible = "atmel,at45xxx", },
> > > > > +	{ .compatible = "atmel,dataflash", },
> > > > > +	{ /* sentinel */ }
> > > > > +};
> > > > > +
> > > > 
> > > > This should be protected with a #ifdef CONFIG_OF/#else/#endif, and
> > > > there should be a MODULE_DEVICE_TABLE().
> > > > 
> > > I personally hate #ifdef stuff.  But okay, I can do it since there
> > > are people being concerned by this little waste of space.
> > 
> > I guess the question is - will it compile and work if CONFIG_OF is
> > unset?
> > 
> Yes, it will compile, as 'struct of_device_id' is defined in
> include/linux/mod_devicetable.h unconditionally.

And it will work correctly even though dataflash_dt_ids is not NULL, it
will not confuse MTD layer?
Shawn Guo July 20, 2011, 5:28 a.m. UTC | #10 
On Wed, Jul 20, 2011 at 08:17:45AM +0300, Artem Bityutskiy wrote:
> On Wed, 2011-07-20 at 12:55 +0800, Shawn Guo wrote:
> > On Wed, Jul 20, 2011 at 07:40:38AM +0300, Artem Bityutskiy wrote:
> > > On Fri, 2011-07-15 at 12:49 +0800, Shawn Guo wrote:
> > > > > > +static const struct of_device_id dataflash_dt_ids[] = {
> > > > > > +	{ .compatible = "atmel,at45xxx", },
> > > > > > +	{ .compatible = "atmel,dataflash", },
> > > > > > +	{ /* sentinel */ }
> > > > > > +};
> > > > > > +
> > > > > 
> > > > > This should be protected with a #ifdef CONFIG_OF/#else/#endif, and
> > > > > there should be a MODULE_DEVICE_TABLE().
> > > > > 
> > > > I personally hate #ifdef stuff.  But okay, I can do it since there
> > > > are people being concerned by this little waste of space.
> > > 
> > > I guess the question is - will it compile and work if CONFIG_OF is
> > > unset?
> > > 
> > Yes, it will compile, as 'struct of_device_id' is defined in
> > include/linux/mod_devicetable.h unconditionally.
> 
> And it will work correctly even though dataflash_dt_ids is not NULL, it
> will not confuse MTD layer?
> 
I think for non-dt case, dataflash_dt_ids is not used anyway.  So yes,
it will not confuse MTD layer, at least from my testing.
Artem Bityutskiy July 20, 2011, 5:32 a.m. UTC | #11 
On Wed, 2011-07-20 at 13:28 +0800, Shawn Guo wrote:
> On Wed, Jul 20, 2011 at 08:17:45AM +0300, Artem Bityutskiy wrote:
> > On Wed, 2011-07-20 at 12:55 +0800, Shawn Guo wrote:
> > > On Wed, Jul 20, 2011 at 07:40:38AM +0300, Artem Bityutskiy wrote:
> > > > On Fri, 2011-07-15 at 12:49 +0800, Shawn Guo wrote:
> > > > > > > +static const struct of_device_id dataflash_dt_ids[] = {
> > > > > > > +	{ .compatible = "atmel,at45xxx", },
> > > > > > > +	{ .compatible = "atmel,dataflash", },
> > > > > > > +	{ /* sentinel */ }
> > > > > > > +};
> > > > > > > +
> > > > > > 
> > > > > > This should be protected with a #ifdef CONFIG_OF/#else/#endif, and
> > > > > > there should be a MODULE_DEVICE_TABLE().
> > > > > > 
> > > > > I personally hate #ifdef stuff.  But okay, I can do it since there
> > > > > are people being concerned by this little waste of space.
> > > > 
> > > > I guess the question is - will it compile and work if CONFIG_OF is
> > > > unset?
> > > > 
> > > Yes, it will compile, as 'struct of_device_id' is defined in
> > > include/linux/mod_devicetable.h unconditionally.
> > 
> > And it will work correctly even though dataflash_dt_ids is not NULL, it
> > will not confuse MTD layer?
> > 
> I think for non-dt case, dataflash_dt_ids is not used anyway.  So yes,
> it will not confuse MTD layer, at least from my testing.

And it is not error-prone? I mean, it is not very likely that someone
changing MTD could make wrong assumptions and break dataflash driver? If
so, then we probably do not need ifdefs indeed.
Mike Frysinger July 20, 2011, 5:38 a.m. UTC | #12 
On Wed, Jul 20, 2011 at 01:32, Artem Bityutskiy wrote:
> On Wed, 2011-07-20 at 13:28 +0800, Shawn Guo wrote:
>> On Wed, Jul 20, 2011 at 08:17:45AM +0300, Artem Bityutskiy wrote:
>> > On Wed, 2011-07-20 at 12:55 +0800, Shawn Guo wrote:
>> > > On Wed, Jul 20, 2011 at 07:40:38AM +0300, Artem Bityutskiy wrote:
>> > > > On Fri, 2011-07-15 at 12:49 +0800, Shawn Guo wrote:
>> > > > > > > +static const struct of_device_id dataflash_dt_ids[] = {
>> > > > > > > + { .compatible = "atmel,at45xxx", },
>> > > > > > > + { .compatible = "atmel,dataflash", },
>> > > > > > > + { /* sentinel */ }
>> > > > > > > +};
>> > > > > > > +
>> > > > > >
>> > > > > > This should be protected with a #ifdef CONFIG_OF/#else/#endif, and
>> > > > > > there should be a MODULE_DEVICE_TABLE().
>> > > > > >
>> > > > > I personally hate #ifdef stuff.  But okay, I can do it since there
>> > > > > are people being concerned by this little waste of space.
>> > > >
>> > > > I guess the question is - will it compile and work if CONFIG_OF is
>> > > > unset?
>> > > >
>> > > Yes, it will compile, as 'struct of_device_id' is defined in
>> > > include/linux/mod_devicetable.h unconditionally.
>> >
>> > And it will work correctly even though dataflash_dt_ids is not NULL, it
>> > will not confuse MTD layer?
>> >
>> I think for non-dt case, dataflash_dt_ids is not used anyway.  So yes,
>> it will not confuse MTD layer, at least from my testing.
>
> And it is not error-prone? I mean, it is not very likely that someone
> changing MTD could make wrong assumptions and break dataflash driver? If
> so, then we probably do not need ifdefs indeed.

except for the part where device tree code shouldnt be wasting space
on non-device tree systems
-mike
Shawn Guo July 20, 2011, 5:52 a.m. UTC | #13 
On Wed, Jul 20, 2011 at 08:32:45AM +0300, Artem Bityutskiy wrote:
> On Wed, 2011-07-20 at 13:28 +0800, Shawn Guo wrote:
> > On Wed, Jul 20, 2011 at 08:17:45AM +0300, Artem Bityutskiy wrote:
> > > On Wed, 2011-07-20 at 12:55 +0800, Shawn Guo wrote:
> > > > On Wed, Jul 20, 2011 at 07:40:38AM +0300, Artem Bityutskiy wrote:
> > > > > On Fri, 2011-07-15 at 12:49 +0800, Shawn Guo wrote:
> > > > > > > > +static const struct of_device_id dataflash_dt_ids[] = {
> > > > > > > > +	{ .compatible = "atmel,at45xxx", },
> > > > > > > > +	{ .compatible = "atmel,dataflash", },
> > > > > > > > +	{ /* sentinel */ }
> > > > > > > > +};
> > > > > > > > +
> > > > > > > 
> > > > > > > This should be protected with a #ifdef CONFIG_OF/#else/#endif, and
> > > > > > > there should be a MODULE_DEVICE_TABLE().
> > > > > > > 
> > > > > > I personally hate #ifdef stuff.  But okay, I can do it since there
> > > > > > are people being concerned by this little waste of space.
> > > > > 
> > > > > I guess the question is - will it compile and work if CONFIG_OF is
> > > > > unset?
> > > > > 
> > > > Yes, it will compile, as 'struct of_device_id' is defined in
> > > > include/linux/mod_devicetable.h unconditionally.
> > > 
> > > And it will work correctly even though dataflash_dt_ids is not NULL, it
> > > will not confuse MTD layer?
> > > 
> > I think for non-dt case, dataflash_dt_ids is not used anyway.  So yes,
> > it will not confuse MTD layer, at least from my testing.
> 
> And it is not error-prone? I mean, it is not very likely that someone
> changing MTD could make wrong assumptions and break dataflash driver? If
> so, then we probably do not need ifdefs indeed.
> 
No, it's not error-prone.  The dataflash_dt_ids assigned to
.of_match_table is used by OF device core to match driver and device.
I do not think MTD change could impact this match.

The argument about this #ifdef thing was the driver could be used by
some platforms that will never migrate to device tree.  This table
will be just a waste of bytes for those platforms, if we do not
protect the table by '#ifdef CONFIG_OF'.
Artem Bityutskiy July 20, 2011, 5:52 a.m. UTC | #14 
On Wed, 2011-07-20 at 13:52 +0800, Shawn Guo wrote:
> The argument about this #ifdef thing was the driver could be used by
> some platforms that will never migrate to device tree.  This table
> will be just a waste of bytes for those platforms, if we do not
> protect the table by '#ifdef CONFIG_OF'.

OK, anyway, I picked your v2 already and less bytes wastage is a good
enough argument.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/mtd/atmel-dataflash.txt b/Documentation/devicetree/bindings/mtd/atmel-dataflash.txt
new file mode 100644
index 0000000..3783962
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/atmel-dataflash.txt
@@ -0,0 +1,14 @@ 
+* Atmel Data Flash
+
+Required properties:
+- compatible : "atmel,<model>", "atmel,<series>", "atmel,dataflash".
+
+Example:
+
+flash@1 {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	compatible = "atmel,at45db321d", "atmel,at45xxx", "atmel,dataflash";
+	spi-max-frequency = <25000000>;
+	reg = <1>;
+};
diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index 8f6b02c..8e091b0 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -24,6 +24,8 @@ 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 /*
  * DataFlash is a kind of SPI flash.  Most AT45 chips have two buffers in
@@ -98,6 +100,12 @@  struct dataflash {
 	struct mtd_info		mtd;
 };
 
+static const struct of_device_id dataflash_dt_ids[] = {
+	{ .compatible = "atmel,at45xxx", },
+	{ .compatible = "atmel,dataflash", },
+	{ /* sentinel */ }
+};
+
 /* ......................................................................... */
 
 /*
@@ -634,6 +642,7 @@  add_dataflash_otp(struct spi_device *spi, char *name,
 {
 	struct dataflash		*priv;
 	struct mtd_info			*device;
+	struct mtd_part_parser_data	ppdata;
 	struct flash_platform_data	*pdata = spi->dev.platform_data;
 	char				*otp_tag = "";
 	int				err = 0;
@@ -675,7 +684,8 @@  add_dataflash_otp(struct spi_device *spi, char *name,
 			pagesize, otp_tag);
 	dev_set_drvdata(&spi->dev, priv);
 
-	err = mtd_device_parse_register(device, NULL, 0,
+	ppdata.of_node = spi->dev.of_node;
+	err = mtd_device_parse_register(device, NULL, &ppdata,
 			pdata ? pdata->parts : NULL,
 			pdata ? pdata->nr_parts : 0);
 
@@ -926,6 +936,7 @@  static struct spi_driver dataflash_driver = {
 		.name		= "mtd_dataflash",
 		.bus		= &spi_bus_type,
 		.owner		= THIS_MODULE,
+		.of_match_table = dataflash_dt_ids,
 	},
 
 	.probe		= dataflash_probe,