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

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

[Shawn Guo]

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>
---
 drivers/mtd/devices/mtd_dataflash.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index 8f6b02c..64d7242 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,11 @@ struct dataflash {
 	struct mtd_info		mtd;
 };
 
+static const struct of_device_id dataflash_dt_ids[] = {
+	{ .compatible = "atmel,dataflash", },
+	{ /* sentinel */ }
+};
+
 /* ......................................................................... */
 
 /*
@@ -634,6 +641,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 +683,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 +935,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

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

[Shawn Guo]

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 at 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,
-- 
1.7.4.1

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

[Mike Frysinger]

shouldnt there be #ifdef's around this ?  not everyone supports devicetrees.
-mike

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

[Dmitry Eremin-Solenikov]

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.

-- 
With best wishes
Dmitry

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

[Grant Likely]

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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Grant Likely]

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 at 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
> 

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

[Shawn Guo]

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?

-- 
Regards,
Shawn

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

[Shawn Guo]

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 at 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 at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 

[PATCH v3] mtd: dataflash: add device tree probe support

[Shawn Guo]

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 v2:
 * Change compatible string to "at45" from "at45xxx"
 * Add "#ifdef CONFIG_OF" for dataflash_dt_ids

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                |   18 ++++++++++++++++--
 2 files changed, 30 insertions(+), 2 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..ef66ddd
--- /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 at 1 {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	compatible = "atmel,at45db321d", "atmel,at45", "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..c86fa57 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -17,6 +17,8 @@
 #include <linux/mutex.h>
 #include <linux/err.h>
 #include <linux/math64.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 #include <linux/spi/spi.h>
 #include <linux/spi/flash.h>
@@ -24,7 +26,6 @@
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 
-
 /*
  * DataFlash is a kind of SPI flash.  Most AT45 chips have two buffers in
  * each chip, which may be used for double buffered I/O; but this driver
@@ -98,6 +99,16 @@ struct dataflash {
 	struct mtd_info		mtd;
 };
 
+#ifdef CONFIG_OF
+static const struct of_device_id dataflash_dt_ids[] = {
+	{ .compatible = "atmel,at45", },
+	{ .compatible = "atmel,dataflash", },
+	{ /* sentinel */ }
+};
+#else
+#define dataflash_dt_ids NULL
+#endif
+
 /* ......................................................................... */
 
 /*
@@ -634,6 +645,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 +687,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 +939,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

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

[Artem Bityutskiy]

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?


-- 
Best Regards,
Artem Bityutskiy

[PATCH v3] mtd: dataflash: add device tree probe support

[Artem Bityutskiy]

On Fri, 2011-07-15 at 16:38 +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>

Pushed to l2-mtd-2.6.git, thanks!

-- 
Best Regards,
Artem Bityutskiy

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

[Shawn Guo]

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.

-- 
Regards,
Shawn

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

[Artem Bityutskiy]

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?

-- 
Best Regards,
Artem Bityutskiy

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

[Shawn Guo]

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.

-- 
Regards,
Shawn

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

[Artem Bityutskiy]

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.

-- 
Best Regards,
Artem Bityutskiy

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

[Mike Frysinger]

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

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

[Shawn Guo]

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'.

-- 
Regards,
Shawn

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

[Artem Bityutskiy]

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.

-- 
Best Regards,
Artem Bityutskiy