[PATCH v3 0/4 REPOST] OF infrastructure for SPI devices

[PATCH v3 0/4 REPOST] OF infrastructure for SPI devices

[Grant Likely]

I don't know what to do with these patches.  I'd really like to see them
in .27, and now that akpm has cleared his queue, the prerequisite patch
has been merged so they are ready to go in.  However, even though there
has been favourable reception on the SPI and linuxppc lists for these
changes I don't have any acks from anybody yet.

David, can you please reply on if you are okay with these patches
getting merged?  If so, do you want me to merge them via my tree, or
do you want to pick them up?

Thanks,
g.

--
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.

[PATCH v3 1/4] of: adapt of_find_i2c_driver() to be usable by SPI also

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

SPI has a similar problem as I2C in that it needs to determine an
appropriate modalias value for each device node.  This patch adapts
the of_i2c of_find_i2c_driver() function to be usable by of_spi also.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/of/base.c   |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/of/of_i2c.c |   64 ++-----------------------------------
 include/linux/of.h  |    1 +
 3 files changed, 92 insertions(+), 61 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 23ffb7c..ad8ac1a 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -385,3 +385,91 @@ struct device_node *of_find_matching_node(struct device_node *from,
 	return np;
 }
 EXPORT_SYMBOL(of_find_matching_node);
+
+/**
+ * of_modalias_table: Table of explicit compatible ==> modalias mappings
+ *
+ * This table allows particulare compatible property values to be mapped
+ * to modalias strings.  This is useful for busses which do not directly
+ * understand the OF device tree but are populated based on data contained
+ * within the device tree.  SPI and I2C are the two current users of this
+ * table.
+ *
+ * In most cases, devices do not need to be listed in this table because
+ * the modalias value can be derived directly from the compatible table.
+ * However, if for any reason a value cannot be derived, then this table
+ * provides a method to override the implicit derivation.
+ *
+ * At the moment, a single table is used for all bus types because it is
+ * assumed that the data size is small and that the compatible values
+ * should already be distinct enough to differentiate between SPI, I2C
+ * and other devices.
+ */
+struct of_modalias_table {
+	char *of_device;
+	char *modalias;
+};
+static struct of_modalias_table of_modalias_table[] = {
+	/* Empty for now; add entries as needed */
+};
+
+/**
+ * of_modalias_node - Lookup appropriate modalias for a device node
+ * @node:	pointer to a device tree node
+ * @modalias:	Pointer to buffer that modalias value will be copied into
+ * @len:	Length of modalias value
+ *
+ * Based on the value of the compatible property, this routine will determine
+ * an appropriate modalias value for a particular device tree node.  Three
+ * separate methods are used to derive a modalias value.
+ *
+ * First method is to lookup the compatible value in of_modalias_table.
+ * Second is to look for a "linux,<modalias>" entry in the compatible list
+ * and used that for modalias.  Third is to strip off the manufacturer
+ * prefix from the first compatible entry and use the remainder as modalias
+ *
+ * This routine returns 0 on success
+ */
+int of_modalias_node(struct device_node *node, char *modalias, int len)
+{
+	int i, cplen;
+	const char *compatible;
+	const char *p;
+
+	/* 1. search for exception list entry */
+	for (i = 0; i < ARRAY_SIZE(of_modalias_table); i++) {
+		compatible = of_modalias_table[i].of_device;
+		if (!of_device_is_compatible(node, compatible))
+			continue;
+		strlcpy(modalias, of_modalias_table[i].modalias, len);
+		return 0;
+	}
+
+	compatible = of_get_property(node, "compatible", &cplen);
+	if (!compatible)
+		return -ENODEV;
+
+	/* 2. search for linux,<modalias> entry */
+	p = compatible;
+	while (cplen > 0) {
+		if (!strncmp(p, "linux,", 6)) {
+			p += 6;
+			strlcpy(modalias, p, len);
+			return 0;
+		}
+
+		i = strlen(p) + 1;
+		p += i;
+		cplen -= i;
+	}
+
+	/* 3. take first compatible entry and strip manufacturer */
+	p = strchr(compatible, ',');
+	if (!p)
+		return -ENODEV;
+	p++;
+	strlcpy(modalias, p, len);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_modalias_node);
+
diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
index 344e1b0..6a98dc8 100644
--- a/drivers/of/of_i2c.c
+++ b/drivers/of/of_i2c.c
@@ -16,62 +16,6 @@
 #include <linux/of_i2c.h>
 #include <linux/module.h>
 
-struct i2c_driver_device {
-	char    *of_device;
-	char    *i2c_type;
-};
-
-static struct i2c_driver_device i2c_devices[] = {
-};
-
-static int of_find_i2c_driver(struct device_node *node,
-			      struct i2c_board_info *info)
-{
-	int i, cplen;
-	const char *compatible;
-	const char *p;
-
-	/* 1. search for exception list entry */
-	for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
-		if (!of_device_is_compatible(node, i2c_devices[i].of_device))
-			continue;
-		if (strlcpy(info->type, i2c_devices[i].i2c_type,
-			    I2C_NAME_SIZE) >= I2C_NAME_SIZE)
-			return -ENOMEM;
-
-		return 0;
-	}
-
-	compatible = of_get_property(node, "compatible", &cplen);
-	if (!compatible)
-		return -ENODEV;
-
-	/* 2. search for linux,<i2c-type> entry */
-	p = compatible;
-	while (cplen > 0) {
-		if (!strncmp(p, "linux,", 6)) {
-			p += 6;
-			if (strlcpy(info->type, p,
-				    I2C_NAME_SIZE) >= I2C_NAME_SIZE)
-				return -ENOMEM;
-			return 0;
-		}
-
-		i = strlen(p) + 1;
-		p += i;
-		cplen -= i;
-	}
-
-	/* 3. take fist compatible entry and strip manufacturer */
-	p = strchr(compatible, ',');
-	if (!p)
-		return -ENODEV;
-	p++;
-	if (strlcpy(info->type, p, I2C_NAME_SIZE) >= I2C_NAME_SIZE)
-		return -ENOMEM;
-	return 0;
-}
-
 void of_register_i2c_devices(struct i2c_adapter *adap,
 			     struct device_node *adap_node)
 {
@@ -83,6 +27,9 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
 		const u32 *addr;
 		int len;
 
+		if (of_modalias_node(node, info.type, sizeof(info.type)) < 0)
+			continue;
+
 		addr = of_get_property(node, "reg", &len);
 		if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
 			printk(KERN_ERR
@@ -92,11 +39,6 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
 
 		info.irq = irq_of_parse_and_map(node, 0);
 
-		if (of_find_i2c_driver(node, &info) < 0) {
-			irq_dispose_mapping(info.irq);
-			continue;
-		}
-
 		info.addr = *addr;
 
 		request_module(info.type);
diff --git a/include/linux/of.h b/include/linux/of.h
index 59a61bd..79886ad 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -70,5 +70,6 @@ extern int of_n_addr_cells(struct device_node *np);
 extern int of_n_size_cells(struct device_node *np);
 extern const struct of_device_id *of_match_node(
 	const struct of_device_id *matches, const struct device_node *node);
+extern int of_modalias_node(struct device_node *node, char *modalias, int len);
 
 #endif /* _LINUX_OF_H */

Re: [PATCH v3 1/4] of: adapt of_find_i2c_driver() to be usable by SPI also

[Jon Smirl]

On 7/25/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
>  SPI has a similar problem as I2C in that it needs to determine an
>  appropriate modalias value for each device node.  This patch adapts
>  the of_i2c of_find_i2c_driver() function to be usable by of_spi also.
>
>  Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>  ---
>
>   drivers/of/base.c   |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/of/of_i2c.c |   64 ++-----------------------------------
>   include/linux/of.h  |    1 +
>   3 files changed, 92 insertions(+), 61 deletions(-)
>
>  diff --git a/drivers/of/base.c b/drivers/of/base.c
>  index 23ffb7c..ad8ac1a 100644
>  --- a/drivers/of/base.c
>  +++ b/drivers/of/base.c
>  @@ -385,3 +385,91 @@ struct device_node *of_find_matching_node(struct device_node *from,
>         return np;
>   }
>   EXPORT_SYMBOL(of_find_matching_node);
>  +
>  +/**
>  + * of_modalias_table: Table of explicit compatible ==> modalias mappings
>  + *
>  + * This table allows particulare compatible property values to be mapped
>  + * to modalias strings.  This is useful for busses which do not directly
>  + * understand the OF device tree but are populated based on data contained
>  + * within the device tree.  SPI and I2C are the two current users of this
>  + * table.
>  + *
>  + * In most cases, devices do not need to be listed in this table because
>  + * the modalias value can be derived directly from the compatible table.
>  + * However, if for any reason a value cannot be derived, then this table
>  + * provides a method to override the implicit derivation.
>  + *
>  + * At the moment, a single table is used for all bus types because it is
>  + * assumed that the data size is small and that the compatible values
>  + * should already be distinct enough to differentiate between SPI, I2C
>  + * and other devices.

Maybe add a section recommending to update the alias list in the linux
device driver before adding entries here? This table should be a last
resort. I'm not even sure this table should exist, what would be a
case where we would need to make an entry here instead of fixing the
device driver by adding an alias name?

The list of alias part numbers really belong in the devices drivers,
not some global table that has to always be kept in memory.

>  + */
>  +struct of_modalias_table {
>  +       char *of_device;
>  +       char *modalias;
>  +};
>  +static struct of_modalias_table of_modalias_table[] = {
>  +       /* Empty for now; add entries as needed */
>  +};
>  +
>  +/**
>  + * of_modalias_node - Lookup appropriate modalias for a device node
>  + * @node:      pointer to a device tree node
>  + * @modalias:  Pointer to buffer that modalias value will be copied into
>  + * @len:       Length of modalias value
>  + *
>  + * Based on the value of the compatible property, this routine will determine
>  + * an appropriate modalias value for a particular device tree node.  Three
>  + * separate methods are used to derive a modalias value.
>  + *
>  + * First method is to lookup the compatible value in of_modalias_table.
>  + * Second is to look for a "linux,<modalias>" entry in the compatible list
>  + * and used that for modalias.  Third is to strip off the manufacturer
>  + * prefix from the first compatible entry and use the remainder as modalias

I also think this is a problem. Embedding the name of Linux device
drivers into device firmware makes it almost impossible to rename the
device driver.  Again, what is a case where generic part numbers can't
be listed in the alias section of the linux device driver?

Even eeprom was just fixed to take generic part numbers (at24).

>  + *
>  + * This routine returns 0 on success
>  + */
>  +int of_modalias_node(struct device_node *node, char *modalias, int len)
>  +{
>  +       int i, cplen;
>  +       const char *compatible;
>  +       const char *p;
>  +
>  +       /* 1. search for exception list entry */
>  +       for (i = 0; i < ARRAY_SIZE(of_modalias_table); i++) {
>  +               compatible = of_modalias_table[i].of_device;
>  +               if (!of_device_is_compatible(node, compatible))
>  +                       continue;
>  +               strlcpy(modalias, of_modalias_table[i].modalias, len);
>  +               return 0;
>  +       }
>  +
>  +       compatible = of_get_property(node, "compatible", &cplen);
>  +       if (!compatible)
>  +               return -ENODEV;
>  +
>  +       /* 2. search for linux,<modalias> entry */
>  +       p = compatible;
>  +       while (cplen > 0) {
>  +               if (!strncmp(p, "linux,", 6)) {
>  +                       p += 6;
>  +                       strlcpy(modalias, p, len);
>  +                       return 0;
>  +               }
>  +
>  +               i = strlen(p) + 1;
>  +               p += i;
>  +               cplen -= i;
>  +       }
>  +
>  +       /* 3. take first compatible entry and strip manufacturer */
>  +       p = strchr(compatible, ',');
>  +       if (!p)
>  +               return -ENODEV;
>  +       p++;
>  +       strlcpy(modalias, p, len);
>  +       return 0;
>  +}
>  +EXPORT_SYMBOL_GPL(of_modalias_node);
>  +
>  diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
>  index 344e1b0..6a98dc8 100644
>  --- a/drivers/of/of_i2c.c
>  +++ b/drivers/of/of_i2c.c
>  @@ -16,62 +16,6 @@
>   #include <linux/of_i2c.h>
>   #include <linux/module.h>
>
>  -struct i2c_driver_device {
>  -       char    *of_device;
>  -       char    *i2c_type;
>  -};
>  -
>  -static struct i2c_driver_device i2c_devices[] = {
>  -};
>  -
>  -static int of_find_i2c_driver(struct device_node *node,
>  -                             struct i2c_board_info *info)
>  -{
>  -       int i, cplen;
>  -       const char *compatible;
>  -       const char *p;
>  -
>  -       /* 1. search for exception list entry */
>  -       for (i = 0; i < ARRAY_SIZE(i2c_devices); i++) {
>  -               if (!of_device_is_compatible(node, i2c_devices[i].of_device))
>  -                       continue;
>  -               if (strlcpy(info->type, i2c_devices[i].i2c_type,
>  -                           I2C_NAME_SIZE) >= I2C_NAME_SIZE)
>  -                       return -ENOMEM;
>  -
>  -               return 0;
>  -       }
>  -
>  -       compatible = of_get_property(node, "compatible", &cplen);
>  -       if (!compatible)
>  -               return -ENODEV;
>  -
>  -       /* 2. search for linux,<i2c-type> entry */
>  -       p = compatible;
>  -       while (cplen > 0) {
>  -               if (!strncmp(p, "linux,", 6)) {
>  -                       p += 6;
>  -                       if (strlcpy(info->type, p,
>  -                                   I2C_NAME_SIZE) >= I2C_NAME_SIZE)
>  -                               return -ENOMEM;
>  -                       return 0;
>  -               }
>  -
>  -               i = strlen(p) + 1;
>  -               p += i;
>  -               cplen -= i;
>  -       }
>  -
>  -       /* 3. take fist compatible entry and strip manufacturer */
>  -       p = strchr(compatible, ',');
>  -       if (!p)
>  -               return -ENODEV;
>  -       p++;
>  -       if (strlcpy(info->type, p, I2C_NAME_SIZE) >= I2C_NAME_SIZE)
>  -               return -ENOMEM;
>  -       return 0;
>  -}
>  -
>   void of_register_i2c_devices(struct i2c_adapter *adap,
>                              struct device_node *adap_node)
>   {
>  @@ -83,6 +27,9 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
>                 const u32 *addr;
>                 int len;
>
>  +               if (of_modalias_node(node, info.type, sizeof(info.type)) < 0)
>  +                       continue;
>  +
>                 addr = of_get_property(node, "reg", &len);
>                 if (!addr || len < sizeof(int) || *addr > (1 << 10) - 1) {
>                         printk(KERN_ERR
>  @@ -92,11 +39,6 @@ void of_register_i2c_devices(struct i2c_adapter *adap,
>
>                 info.irq = irq_of_parse_and_map(node, 0);
>
>  -               if (of_find_i2c_driver(node, &info) < 0) {
>  -                       irq_dispose_mapping(info.irq);
>  -                       continue;
>  -               }
>  -
>                 info.addr = *addr;
>
>                 request_module(info.type);
>  diff --git a/include/linux/of.h b/include/linux/of.h
>  index 59a61bd..79886ad 100644
>  --- a/include/linux/of.h
>  +++ b/include/linux/of.h
>  @@ -70,5 +70,6 @@ extern int of_n_addr_cells(struct device_node *np);
>   extern int of_n_size_cells(struct device_node *np);
>   extern const struct of_device_id *of_match_node(
>         const struct of_device_id *matches, const struct device_node *node);
>  +extern int of_modalias_node(struct device_node *node, char *modalias, int len);
>
>   #endif /* _LINUX_OF_H */
>
>
>  --
>  To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>  the body of a message to majordomo@vger.kernel.org
>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>  Please read the FAQ at  http://www.tux.org/lkml/
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH v3 1/4] of: adapt of_find_i2c_driver() to be usable by SPI also

[Grant Likely]

On Fri, Jul 25, 2008 at 11:40 AM, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 7/25/08, Grant Likely <grant.likely@secretlab.ca> wrote:
>> From: Grant Likely <grant.likely@secretlab.ca>
>>
>>  + * At the moment, a single table is used for all bus types because it is
>>  + * assumed that the data size is small and that the compatible values
>>  + * should already be distinct enough to differentiate between SPI, I2C
>>  + * and other devices.
>
> Maybe add a section recommending to update the alias list in the linux
> device driver before adding entries here? This table should be a last
> resort. I'm not even sure this table should exist, what would be a
> case where we would need to make an entry here instead of fixing the
> device driver by adding an alias name?

In principle I agree.  However, this patch is simply porting the i2c
specific code to something that can be used by both SPI and I2C.  I
don't want to rework the actual mechanism in this particular patch.  I
can submit an additional patch to change this along with reworking
some of the behavior that needs to be improved.

>>  + * First method is to lookup the compatible value in of_modalias_table.
>>  + * Second is to look for a "linux,<modalias>" entry in the compatible list
>>  + * and used that for modalias.  Third is to strip off the manufacturer
>>  + * prefix from the first compatible entry and use the remainder as modalias
>
> I also think this is a problem. Embedding the name of Linux device
> drivers into device firmware makes it almost impossible to rename the
> device driver.  Again, what is a case where generic part numbers can't
> be listed in the alias section of the linux device driver?
>
> Even eeprom was just fixed to take generic part numbers (at24).

Again, I agree, but this change is very much a stop gap measure to get
things working in a sane way without having to create bad device tree
bindings (device tree bindings are hard to change, code is not).  I've
been considering posting a patch to remove this clause from the
functions, but that needs to be reviewed separately from this change.

Thanks,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 1/4] of: adapt of_find_i2c_driver() to be usable by SPI also

[Jon Smirl]

On 7/25/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, Jul 25, 2008 at 11:40 AM, Jon Smirl <jonsmirl@gmail.com> wrote:
>  > On 7/25/08, Grant Likely <grant.likely@secretlab.ca> wrote:
>  >> From: Grant Likely <grant.likely@secretlab.ca>
>  >>
>
> >>  + * At the moment, a single table is used for all bus types because it is
>  >>  + * assumed that the data size is small and that the compatible values
>  >>  + * should already be distinct enough to differentiate between SPI, I2C
>  >>  + * and other devices.
>  >
>  > Maybe add a section recommending to update the alias list in the linux
>  > device driver before adding entries here? This table should be a last
>  > resort. I'm not even sure this table should exist, what would be a
>  > case where we would need to make an entry here instead of fixing the
>  > device driver by adding an alias name?
>
>
> In principle I agree.  However, this patch is simply porting the i2c
>  specific code to something that can be used by both SPI and I2C.  I
>  don't want to rework the actual mechanism in this particular patch.  I
>  can submit an additional patch to change this along with reworking
>  some of the behavior that needs to be improved.
>
>
>  >>  + * First method is to lookup the compatible value in of_modalias_table.
>  >>  + * Second is to look for a "linux,<modalias>" entry in the compatible list
>  >>  + * and used that for modalias.  Third is to strip off the manufacturer
>  >>  + * prefix from the first compatible entry and use the remainder as modalias
>  >
>  > I also think this is a problem. Embedding the name of Linux device
>  > drivers into device firmware makes it almost impossible to rename the
>  > device driver.  Again, what is a case where generic part numbers can't
>  > be listed in the alias section of the linux device driver?
>  >
>  > Even eeprom was just fixed to take generic part numbers (at24).
>
>
> Again, I agree, but this change is very much a stop gap measure to get
>  things working in a sane way without having to create bad device tree
>  bindings (device tree bindings are hard to change, code is not).  I've
>  been considering posting a patch to remove this clause from the
>  functions, but that needs to be reviewed separately from this change.

Isn't putting "compatible="linux,modalias"" into your device tree a
really bad idea?

>
>  Thanks,
>
> g.
>
>  --
>  Grant Likely, B.Sc., P.Eng.
>  Secret Lab Technologies Ltd.
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH v3 1/4] of: adapt of_find_i2c_driver() to be usable by SPI also

[Grant Likely]

On Fri, Jul 25, 2008 at 1:02 PM, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 7/25/08, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Fri, Jul 25, 2008 at 11:40 AM, Jon Smirl <jonsmirl@gmail.com> wrote:
>>  > On 7/25/08, Grant Likely <grant.likely@secretlab.ca> wrote:
>>  >>  + * First method is to lookup the compatible value in of_modalias_table.
>>  >>  + * Second is to look for a "linux,<modalias>" entry in the compatible list
>>  >>  + * and used that for modalias.  Third is to strip off the manufacturer
>>  >>  + * prefix from the first compatible entry and use the remainder as modalias
>>  >
>>  > I also think this is a problem. Embedding the name of Linux device
>>  > drivers into device firmware makes it almost impossible to rename the
>>  > device driver.  Again, what is a case where generic part numbers can't
>>  > be listed in the alias section of the linux device driver?
>>  >
>>  > Even eeprom was just fixed to take generic part numbers (at24).
>>
>> Again, I agree, but this change is very much a stop gap measure to get
>>  things working in a sane way without having to create bad device tree
>>  bindings (device tree bindings are hard to change, code is not).  I've
>>  been considering posting a patch to remove this clause from the
>>  functions, but that needs to be reviewed separately from this change.
>
> Isn't putting "compatible="linux,modalias"" into your device tree a
> really bad idea?

Yes, it is, but I still need to preserve existing behavior in this
patch.  That change needs to be reviewed separately.  I'll submit
another patch to deal with this.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[PATCH v3 2/4] spi: split up spi_new_device() to allow two stage registration.

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

spi_new_device() allocates and registers an spi device all in one swoop.
If the driver needs to add extra data to the spi_device before it is
registered, then this causes problems.

This patch splits the allocation and registration portions of code out
of spi_new_device() and creates three new functions; spi_alloc_device(),
spi_register_device(), and spi_device_release().  spi_new_device() is
modified to use the new functions for allocation and registration.
None of the existing users of spi_new_device() should be affected by
this change.

Drivers using the new API can forego the use of an spi_board_info
structure to describe the device layout and populate data into the
spi_device structure directly.

This change is in preparation for adding an OF device tree parser to
generate spi_devices based on data in the device tree.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/spi/spi.c       |  139 ++++++++++++++++++++++++++++++++---------------
 include/linux/spi/spi.h |   10 +++
 2 files changed, 105 insertions(+), 44 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index ecca4a6..2077ef5 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -178,6 +178,96 @@ struct boardinfo {
 static LIST_HEAD(board_list);
 static DEFINE_MUTEX(board_lock);
 
+/**
+ * spi_alloc_device - Allocate a new SPI device
+ * @master: Controller to which device is connected
+ * Context: can sleep
+ *
+ * Allows a driver to allocate and initialize and spi_device without
+ * registering it immediately.  This allows a driver to directly
+ * fill the spi_device with device parameters before calling
+ * spi_add_device() on it.
+ *
+ * Caller is responsible to call spi_add_device() on the returned
+ * spi_device structure to add it to the SPI master.  If the caller
+ * needs to discard the spi_device without adding it, then it should
+ * call spi_dev_put() on it.
+ *
+ * Returns a pointer to the new device, or NULL.
+ */
+struct spi_device *spi_alloc_device(struct spi_master *master)
+{
+	struct spi_device	*spi;
+	struct device		*dev = master->dev.parent;
+
+	if (!spi_master_get(master))
+		return NULL;
+
+	spi = kzalloc(sizeof *spi, GFP_KERNEL);
+	if (!spi) {
+		dev_err(dev, "cannot alloc spi_device\n");
+		spi_master_put(master);
+		return NULL;
+	}
+
+	spi->master = master;
+	spi->dev.parent = dev;
+	spi->dev.bus = &spi_bus_type;
+	spi->dev.release = spidev_release;
+	device_initialize(&spi->dev);
+	return spi;
+}
+EXPORT_SYMBOL_GPL(spi_alloc_device);
+
+/**
+ * spi_add_device - Add an spi_device allocated with spi_alloc_device
+ * @spi: spi_device to register
+ *
+ * Companion function to spi_alloc_device.  Devices allocated with
+ * spi_alloc_device can be added onto the spi bus with this function.
+ *
+ * Returns 0 on success; non-zero on failure
+ */
+int spi_add_device(struct spi_device *spi)
+{
+	struct device *dev = spi->master->dev.parent;
+	int status;
+
+	/* Chipselects are numbered 0..max; validate. */
+	if (spi->chip_select >= spi->master->num_chipselect) {
+		dev_err(dev, "cs%d > max %d\n",
+			spi->chip_select,
+			spi->master->num_chipselect);
+		return -EINVAL;
+	}
+
+	/* Set the bus ID string */
+	snprintf(spi->dev.bus_id, sizeof spi->dev.bus_id,
+			"%s.%u", spi->master->dev.bus_id,
+			spi->chip_select);
+
+	/* drivers may modify this initial i/o setup */
+	status = spi->master->setup(spi);
+	if (status < 0) {
+		dev_err(dev, "can't %s %s, status %d\n",
+				"setup", spi->dev.bus_id, status);
+		return status;
+	}
+
+	/* driver core catches callers that misbehave by defining
+	 * devices that already exist.
+	 */
+	status = device_add(&spi->dev);
+	if (status < 0) {
+		dev_err(dev, "can't %s %s, status %d\n",
+				"add", spi->dev.bus_id, status);
+		return status;
+	}
+
+	dev_dbg(dev, "registered child %s\n", spi->dev.bus_id);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spi_add_device);
 
 /**
  * spi_new_device - instantiate one new SPI device
@@ -197,7 +287,6 @@ struct spi_device *spi_new_device(struct spi_master *master,
 				  struct spi_board_info *chip)
 {
 	struct spi_device	*proxy;
-	struct device		*dev = master->dev.parent;
 	int			status;
 
 	/* NOTE:  caller did any chip->bus_num checks necessary.
@@ -207,66 +296,28 @@ struct spi_device *spi_new_device(struct spi_master *master,
 	 * suggests syslogged diagnostics are best here (ugh).
 	 */
 
-	/* Chipselects are numbered 0..max; validate. */
-	if (chip->chip_select >= master->num_chipselect) {
-		dev_err(dev, "cs%d > max %d\n",
-			chip->chip_select,
-			master->num_chipselect);
-		return NULL;
-	}
-
-	if (!spi_master_get(master))
+	proxy = spi_alloc_device(master);
+	if (!proxy)
 		return NULL;
 
 	WARN_ON(strlen(chip->modalias) >= sizeof(proxy->modalias));
 
-	proxy = kzalloc(sizeof *proxy, GFP_KERNEL);
-	if (!proxy) {
-		dev_err(dev, "can't alloc dev for cs%d\n",
-			chip->chip_select);
-		goto fail;
-	}
-	proxy->master = master;
 	proxy->chip_select = chip->chip_select;
 	proxy->max_speed_hz = chip->max_speed_hz;
 	proxy->mode = chip->mode;
 	proxy->irq = chip->irq;
 	strlcpy(proxy->modalias, chip->modalias, sizeof(proxy->modalias));
-
-	snprintf(proxy->dev.bus_id, sizeof proxy->dev.bus_id,
-			"%s.%u", master->dev.bus_id,
-			chip->chip_select);
-	proxy->dev.parent = dev;
-	proxy->dev.bus = &spi_bus_type;
 	proxy->dev.platform_data = (void *) chip->platform_data;
 	proxy->controller_data = chip->controller_data;
 	proxy->controller_state = NULL;
-	proxy->dev.release = spidev_release;
 
-	/* drivers may modify this initial i/o setup */
-	status = master->setup(proxy);
+	status = spi_add_device(proxy);
 	if (status < 0) {
-		dev_err(dev, "can't %s %s, status %d\n",
-				"setup", proxy->dev.bus_id, status);
-		goto fail;
+		spi_dev_put(proxy);
+		return NULL;
 	}
 
-	/* driver core catches callers that misbehave by defining
-	 * devices that already exist.
-	 */
-	status = device_register(&proxy->dev);
-	if (status < 0) {
-		dev_err(dev, "can't %s %s, status %d\n",
-				"add", proxy->dev.bus_id, status);
-		goto fail;
-	}
-	dev_dbg(dev, "registered child %s\n", proxy->dev.bus_id);
 	return proxy;
-
-fail:
-	spi_master_put(master);
-	kfree(proxy);
-	return NULL;
 }
 EXPORT_SYMBOL_GPL(spi_new_device);
 
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index a9cc29d..d203d08 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -778,8 +778,18 @@ spi_register_board_info(struct spi_board_info const *info, unsigned n)
  * use spi_new_device() to describe each device.  You can also call
  * spi_unregister_device() to start making that device vanish, but
  * normally that would be handled by spi_unregister_master().
+ *
+ * You can also use spi_alloc_device() and spi_add_device() to
+ * for a two stage registration of an SPI device.  This gives the caller
+ * some more control over the spi_device structure before it is registered
  */
 extern struct spi_device *
+spi_alloc_device(struct spi_master *master);
+
+extern int
+spi_add_device(struct spi_device *spi);
+
+extern struct spi_device *
 spi_new_device(struct spi_master *, struct spi_board_info *);
 
 static inline void

Re: [PATCH v3 2/4] spi: split up spi_new_device() to allow two stage registration.

[David Brownell]

On Friday 25 July 2008, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> 
> spi_new_device() allocates and registers an spi device all in one swoop.
> If the driver needs to add extra data to the spi_device before it is
> registered, then this causes problems.

Mention an example please ... you have dev->archdata in mind, yes?


> This patch splits the allocation and registration portions of code out
> of spi_new_device() and creates three new functions; spi_alloc_device(),
> spi_register_device(), and spi_device_release().

Comment needs fixing:  *TWO* new functions, spi_device_release() was
not needed ...


> spi_new_device() is 
> modified to use the new functions for allocation and registration.
> None of the existing users of spi_new_device() should be affected by
> this change.
> 
> Drivers using the new API can forego the use of an spi_board_info

Strike "an".  :)

> structure to describe the device layout and populate data into the
> spi_device structure directly.
> 
> This change is in preparation for adding an OF device tree parser to
> generate spi_devices based on data in the device tree.
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Given the comment fixes noted above and below:


Acked-by: David Brownell <dbrownell@users.sourceforge.net>

Thanks.



> ---
> 
>  drivers/spi/spi.c       |  139 ++++++++++++++++++++++++++++++++---------------
>  include/linux/spi/spi.h |   10 +++
>  2 files changed, 105 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index ecca4a6..2077ef5 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -178,6 +178,96 @@ struct boardinfo {
>  static LIST_HEAD(board_list);
>  static DEFINE_MUTEX(board_lock);
>  
> +/**
> + * spi_alloc_device - Allocate a new SPI device
> + * @master: Controller to which device is connected
> + * Context: can sleep
> + *
> + * Allows a driver to allocate and initialize and spi_device without

"a" spi_device

> + * registering it immediately.  This allows a driver to directly
> + * fill the spi_device with device parameters before calling
> + * spi_add_device() on it.
> + *
> + * Caller is responsible to call spi_add_device() on the returned
> + * spi_device structure to add it to the SPI master.  If the caller
> + * needs to discard the spi_device without adding it, then it should
> + * call spi_dev_put() on it.
> + *
> + * Returns a pointer to the new device, or NULL.
> + */
> +struct spi_device *spi_alloc_device(struct spi_master *master)
> +{
> +	struct spi_device	*spi;
> +	struct device		*dev = master->dev.parent;
> +
> +	if (!spi_master_get(master))
> +		return NULL;
> +
> +	spi = kzalloc(sizeof *spi, GFP_KERNEL);
> +	if (!spi) {
> +		dev_err(dev, "cannot alloc spi_device\n");
> +		spi_master_put(master);
> +		return NULL;
> +	}
> +
> +	spi->master = master;
> +	spi->dev.parent = dev;
> +	spi->dev.bus = &spi_bus_type;
> +	spi->dev.release = spidev_release;
> +	device_initialize(&spi->dev);
> +	return spi;
> +}
> +EXPORT_SYMBOL_GPL(spi_alloc_device);
> +
> +/**
> + * spi_add_device - Add an spi_device allocated with spi_alloc_device

Strike "an".


> + * @spi: spi_device to register
> + *
> + * Companion function to spi_alloc_device.  Devices allocated with
> + * spi_alloc_device can be added onto the spi bus with this function.
> + *
> + * Returns 0 on success; non-zero on failure
> + */
> +int spi_add_device(struct spi_device *spi)
> +{
> +	struct device *dev = spi->master->dev.parent;
> +	int status;
> +
> +	/* Chipselects are numbered 0..max; validate. */
> +	if (spi->chip_select >= spi->master->num_chipselect) {
> +		dev_err(dev, "cs%d > max %d\n",

Nit;  "cs%d >= max %d" ... no need to carry this minor bug forward.


> +			spi->chip_select,
> +			spi->master->num_chipselect);
> +		return -EINVAL;
> +	}
> +
> +	/* Set the bus ID string */
> +	snprintf(spi->dev.bus_id, sizeof spi->dev.bus_id,
> +			"%s.%u", spi->master->dev.bus_id,
> +			spi->chip_select);
> +
> +	/* drivers may modify this initial i/o setup */
> +	status = spi->master->setup(spi);

Hmm, I just noticed this *pre-existing* bug:  if there's already
a device with this chipselect (so the device_add will fail, later),
its configuration will be trashed here.  That's not a reason to NAK
this patch.  (I think the fix would be as simple as calling setup
in spi_drv_probe, and making sure that's always called even if for
some odd reason the driver doesn't have its own probe routine.  If
you want to fix that, great ... otherwise I'll try to get it done
before 2.6.27 finishes.)


> +	if (status < 0) {
> +		dev_err(dev, "can't %s %s, status %d\n",
> +				"setup", spi->dev.bus_id, status);
> +		return status;
> +	}
> +
> +	/* driver core catches callers that misbehave by defining
> +	 * devices that already exist.
> +	 */
> +	status = device_add(&spi->dev);
> +	if (status < 0) {
> +		dev_err(dev, "can't %s %s, status %d\n",
> +				"add", spi->dev.bus_id, status);
> +		return status;
> +	}
> +
> +	dev_dbg(dev, "registered child %s\n", spi->dev.bus_id);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_add_device);
>  
>  /**
>   * spi_new_device - instantiate one new SPI device
> @@ -197,7 +287,6 @@ struct spi_device *spi_new_device(struct spi_master *master,
>  				  struct spi_board_info *chip)
>  {
>  	struct spi_device	*proxy;
> -	struct device		*dev = master->dev.parent;
>  	int			status;
>  
>  	/* NOTE:  caller did any chip->bus_num checks necessary.
> @@ -207,66 +296,28 @@ struct spi_device *spi_new_device(struct spi_master *master,
>  	 * suggests syslogged diagnostics are best here (ugh).
>  	 */
>  
> -	/* Chipselects are numbered 0..max; validate. */
> -	if (chip->chip_select >= master->num_chipselect) {
> -		dev_err(dev, "cs%d > max %d\n",
> -			chip->chip_select,
> -			master->num_chipselect);
> -		return NULL;
> -	}
> -
> -	if (!spi_master_get(master))
> +	proxy = spi_alloc_device(master);
> +	if (!proxy)
>  		return NULL;
>  
>  	WARN_ON(strlen(chip->modalias) >= sizeof(proxy->modalias));
>  
> -	proxy = kzalloc(sizeof *proxy, GFP_KERNEL);
> -	if (!proxy) {
> -		dev_err(dev, "can't alloc dev for cs%d\n",
> -			chip->chip_select);
> -		goto fail;
> -	}
> -	proxy->master = master;
>  	proxy->chip_select = chip->chip_select;
>  	proxy->max_speed_hz = chip->max_speed_hz;
>  	proxy->mode = chip->mode;
>  	proxy->irq = chip->irq;
>  	strlcpy(proxy->modalias, chip->modalias, sizeof(proxy->modalias));
> -
> -	snprintf(proxy->dev.bus_id, sizeof proxy->dev.bus_id,
> -			"%s.%u", master->dev.bus_id,
> -			chip->chip_select);
> -	proxy->dev.parent = dev;
> -	proxy->dev.bus = &spi_bus_type;
>  	proxy->dev.platform_data = (void *) chip->platform_data;
>  	proxy->controller_data = chip->controller_data;
>  	proxy->controller_state = NULL;
> -	proxy->dev.release = spidev_release;
>  
> -	/* drivers may modify this initial i/o setup */
> -	status = master->setup(proxy);
> +	status = spi_add_device(proxy);
>  	if (status < 0) {
> -		dev_err(dev, "can't %s %s, status %d\n",
> -				"setup", proxy->dev.bus_id, status);
> -		goto fail;
> +		spi_dev_put(proxy);
> +		return NULL;
>  	}
>  
> -	/* driver core catches callers that misbehave by defining
> -	 * devices that already exist.
> -	 */
> -	status = device_register(&proxy->dev);
> -	if (status < 0) {
> -		dev_err(dev, "can't %s %s, status %d\n",
> -				"add", proxy->dev.bus_id, status);
> -		goto fail;
> -	}
> -	dev_dbg(dev, "registered child %s\n", proxy->dev.bus_id);
>  	return proxy;
> -
> -fail:
> -	spi_master_put(master);
> -	kfree(proxy);
> -	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(spi_new_device);
>  
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index a9cc29d..d203d08 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -778,8 +778,18 @@ spi_register_board_info(struct spi_board_info const *info, unsigned n)
>   * use spi_new_device() to describe each device.  You can also call
>   * spi_unregister_device() to start making that device vanish, but
>   * normally that would be handled by spi_unregister_master().
> + *
> + * You can also use spi_alloc_device() and spi_add_device() to
> + * for

to, for, sex, ate ... voudou will appreciate!!  ;)

Make that "to use" a two stage registration "sequence".

> 		a two stage registration of an SPI device.  This gives the caller 
> + * some more control over the spi_device structure before it is registered

"... before it is registered, but requires that caller to initialize fields that
would otherwise be defined using the board info."


>   */
>  extern struct spi_device *
> +spi_alloc_device(struct spi_master *master);
> +
> +extern int
> +spi_add_device(struct spi_device *spi);
> +
> +extern struct spi_device *
>  spi_new_device(struct spi_master *, struct spi_board_info *);
>  
>  static inline void
> 

Re: [PATCH v3 2/4] spi: split up spi_new_device() to allow two stage registration.

[Grant Likely]

On Fri, Jul 25, 2008 at 3:00 PM, David Brownell <david-b@pacbell.net> wrote:
> On Friday 25 July 2008, Grant Likely wrote:
>>
>> This change is in preparation for adding an OF device tree parser to
>> generate spi_devices based on data in the device tree.
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>
> Given the comment fixes noted above and below:
>
>
> Acked-by: David Brownell <dbrownell@users.sourceforge.net>

Cool, I'll get all this fixed.  Thanks.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[PATCH v3 3/4] spi: Add OF binding support for SPI busses

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

This patch adds support for populating an SPI bus based on data in the
OF device tree.  This is useful for powerpc platforms which use the
device tree instead of discrete code for describing platform layout.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/of/Kconfig     |    6 +++
 drivers/of/Makefile    |    1 +
 drivers/of/of_spi.c    |   93 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_spi.h |   18 +++++++++
 4 files changed, 118 insertions(+), 0 deletions(-)

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 3a7a11a..edd6e92 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -13,3 +13,9 @@ config OF_I2C
 	depends on PPC_OF && I2C
 	help
 	  OpenFirmware I2C accessors
+
+config OF_SPI
+	def_tristate SPI
+	depends on OF && PPC_OF && SPI
+	help
+	  OpenFirmware SPI accessors
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 548772e..4c3c6f8 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -2,3 +2,4 @@ obj-y = base.o
 obj-$(CONFIG_OF_DEVICE) += device.o platform.o
 obj-$(CONFIG_OF_GPIO)   += gpio.o
 obj-$(CONFIG_OF_I2C)	+= of_i2c.o
+obj-$(CONFIG_OF_SPI)	+= of_spi.o
diff --git a/drivers/of/of_spi.c b/drivers/of/of_spi.c
new file mode 100644
index 0000000..b01eec0
--- /dev/null
+++ b/drivers/of/of_spi.c
@@ -0,0 +1,93 @@
+/*
+ * SPI OF support routines
+ * Copyright (C) 2008 Secret Lab Technologies Ltd.
+ *
+ * Support routines for deriving SPI device attachments from the device
+ * tree.
+ */
+
+#include <linux/of.h>
+#include <linux/device.h>
+#include <linux/spi/spi.h>
+#include <linux/of_spi.h>
+
+/**
+ * of_register_spi_devices - Register child devices onto the SPI bus
+ * @master:	Pointer to spi_master device
+ * @np:		parent node of SPI device nodes
+ *
+ * Registers an spi_device for each child node of 'np' which has a 'reg'
+ * property.
+ */
+void of_register_spi_devices(struct spi_master *master, struct device_node *np)
+{
+	struct spi_device *spi;
+	struct device_node *nc;
+	const u32 *prop;
+	int rc;
+	int len;
+
+	for_each_child_of_node(np, nc) {
+		/* Alloc an spi_device */
+		spi = spi_alloc_device(master);
+		if (!spi) {
+			dev_err(&master->dev, "spi_device alloc error for %s\n",
+				nc->full_name);
+			spi_dev_put(spi);
+			continue;
+		}
+
+		/* Select device driver */
+		if (of_modalias_node(nc, spi->modalias,
+				     sizeof(spi->modalias)) < 0) {
+			dev_err(&master->dev, "cannot find modalias for %s\n",
+				nc->full_name);
+			spi_dev_put(spi);
+			continue;
+		}
+
+		/* Device address */
+		prop = of_get_property(nc, "reg", &len);
+		if (!prop || len < sizeof(*prop)) {
+			dev_err(&master->dev, "%s has no 'reg' property\n",
+				nc->full_name);
+			spi_dev_put(spi);
+			continue;
+		}
+		spi->chip_select = *prop;
+
+		/* Mode (clock phase/polarity/etc.) */
+		if (of_find_property(nc, "spi-cpha", NULL))
+			spi->mode |= SPI_CPHA;
+		if (of_find_property(nc, "spi-cpol", NULL))
+			spi->mode |= SPI_CPOL;
+
+		/* Device speed */
+		prop = of_get_property(nc, "spi-max-frequency", &len);
+		if (!prop || len < sizeof(*prop)) {
+			dev_err(&master->dev, "%s has no 'spi-max-frequency' property\n",
+				nc->full_name);
+			spi_dev_put(spi);
+			continue;
+		}
+		spi->max_speed_hz = *prop;
+
+		/* IRQ */
+		spi->irq = irq_of_parse_and_map(nc, 0);
+
+		/* Store a pointer to the node in the device structure */
+		of_node_get(nc);
+		spi->dev.archdata.of_node = nc;
+
+		/* Register the new device */
+		request_module(spi->modalias);
+		rc = spi_add_device(spi);
+		if (rc) {
+			dev_err(&master->dev, "spi_device register error %s\n",
+				nc->full_name);
+			spi_dev_put(spi);
+		}
+
+	}
+}
+EXPORT_SYMBOL(of_register_spi_devices);
diff --git a/include/linux/of_spi.h b/include/linux/of_spi.h
new file mode 100644
index 0000000..5f71ee8
--- /dev/null
+++ b/include/linux/of_spi.h
@@ -0,0 +1,18 @@
+/*
+ * OpenFirmware SPI support routines
+ * Copyright (C) 2008 Secret Lab Technologies Ltd.
+ *
+ * Support routines for deriving SPI device attachments from the device
+ * tree.
+ */
+
+#ifndef __LINUX_OF_SPI_H
+#define __LINUX_OF_SPI_H
+
+#include <linux/of.h>
+#include <linux/spi/spi.h>
+
+extern void of_register_spi_devices(struct spi_master *master,
+				    struct device_node *np);
+
+#endif /* __LINUX_OF_SPI */

[PATCH v3 4/4] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

Adds support for the dedicated SPI device on the Freescale MPC5200(b)
SoC.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/spi/Kconfig             |    8 +
 drivers/spi/Makefile            |    1 
 drivers/spi/mpc52xx_spi.c       |  595 +++++++++++++++++++++++++++++++++++++++
 include/linux/spi/mpc52xx_spi.h |   10 +
 4 files changed, 614 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 2303521..68e4a4a 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -116,6 +116,14 @@ config SPI_LM70_LLP
 	  which interfaces to an LM70 temperature sensor using
 	  a parallel port.
 
+config SPI_MPC52xx
+	tristate "Freescale MPC52xx SPI (non-PSC) controller support"
+	depends on PPC_MPC52xx && SPI
+	select SPI_MASTER_OF
+	help
+	  This drivers supports the MPC52xx SPI controller in master SPI
+	  mode.
+
 config SPI_MPC52xx_PSC
 	tristate "Freescale MPC52xx PSC SPI controller"
 	depends on PPC_MPC52xx && EXPERIMENTAL
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 7fca043..340b878 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_SPI_PXA2XX)		+= pxa2xx_spi.o
 obj-$(CONFIG_SPI_OMAP_UWIRE)		+= omap_uwire.o
 obj-$(CONFIG_SPI_OMAP24XX)		+= omap2_mcspi.o
 obj-$(CONFIG_SPI_MPC52xx_PSC)		+= mpc52xx_psc_spi.o
+obj-$(CONFIG_SPI_MPC52xx)		+= mpc52xx_spi.o
 obj-$(CONFIG_SPI_MPC83xx)		+= spi_mpc83xx.o
 obj-$(CONFIG_SPI_S3C24XX_GPIO)		+= spi_s3c24xx_gpio.o
 obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
new file mode 100644
index 0000000..453690f
--- /dev/null
+++ b/drivers/spi/mpc52xx_spi.c
@@ -0,0 +1,595 @@
+/*
+ * MPC52xx SPI master driver.
+ * Copyright (C) 2008 Secret Lab Technologies Ltd.
+ *
+ * This is the driver for the MPC5200's dedicated SPI device (not for a
+ * PSC in SPI mode)
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/of_platform.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/mpc52xx_spi.h>
+#include <linux/of_spi.h>
+#include <linux/io.h>
+#include <asm/time.h>
+#include <asm/mpc52xx.h>
+
+MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
+MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
+MODULE_LICENSE("GPL");
+
+/* Register offsets */
+#define SPI_CTRL1	0x00
+#define SPI_CTRL1_SPIE		(1 << 7)
+#define SPI_CTRL1_SPE		(1 << 6)
+#define SPI_CTRL1_MSTR		(1 << 4)
+#define SPI_CTRL1_CPOL		(1 << 3)
+#define SPI_CTRL1_CPHA		(1 << 2)
+#define SPI_CTRL1_SSOE		(1 << 1)
+#define SPI_CTRL1_LSBFE		(1 << 0)
+
+#define SPI_CTRL2	0x01
+#define SPI_BRR		0x04
+
+#define SPI_STATUS	0x05
+#define SPI_STATUS_SPIF		(1 << 7)
+#define SPI_STATUS_WCOL		(1 << 6)
+#define SPI_STATUS_MODF		(1 << 4)
+
+#define SPI_DATA	0x09
+#define SPI_PORTDATA	0x0d
+#define SPI_DATADIR	0x10
+
+/* FSM state return values */
+#define FSM_STOP	0
+#define FSM_POLL	1
+#define FSM_CONTINUE	2
+
+/* Driver internal data */
+struct mpc52xx_spi {
+	struct spi_master *master;
+	u32 sysclk;
+	void __iomem *regs;
+	int irq0;	/* MODF irq */
+	int irq1;	/* SPIF irq */
+	int ipb_freq;
+
+	/* Statistics */
+	int msg_count;
+	int wcol_count;
+	int wcol_ticks;
+	u32 wcol_tx_timestamp;
+	int modf_count;
+	int byte_count;
+
+	/* Hooks for platform modification of behaviour */
+	void (*premessage)(struct spi_message *m, void *context);
+	void *premessage_context;
+
+	struct list_head queue;		/* queue of pending messages */
+	spinlock_t lock;
+	struct work_struct work;
+
+
+	/* Details of current transfer (length, and buffer pointers) */
+	struct spi_message *message;	/* current message */
+	struct spi_transfer *transfer;	/* current transfer */
+	int (*state)(int irq, struct mpc52xx_spi *ms, u8 status, u8 data);
+	int len;
+	int timestamp;
+	u8 *rx_buf;
+	const u8 *tx_buf;
+	int cs_change;
+};
+
+/*
+ * CS control function
+ */
+static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
+{
+	if (value)
+		writeb(0, ms->regs + SPI_PORTDATA); /* Assert SS pin */
+	else
+		writeb(0x08, ms->regs + SPI_PORTDATA); /* Deassert SS pin */
+}
+
+/*
+ * Start a new transfer.  This is called both by the idle state
+ * for the first transfer in a message, and by the wait state when the
+ * previous transfer in a message is complete.
+ */
+static void mpc52xx_spi_start_transfer(struct mpc52xx_spi *ms)
+{
+	ms->rx_buf = ms->transfer->rx_buf;
+	ms->tx_buf = ms->transfer->tx_buf;
+	ms->len = ms->transfer->len;
+
+	/* Activate the chip select */
+	if (ms->cs_change)
+		mpc52xx_spi_chipsel(ms, 1);
+	ms->cs_change = ms->transfer->cs_change;
+
+	/* Write out the first byte */
+	ms->wcol_tx_timestamp = get_tbl();
+	if (ms->tx_buf)
+		writeb(*ms->tx_buf++, ms->regs + SPI_DATA);
+	else
+		writeb(0, ms->regs + SPI_DATA);
+}
+
+/* Forward declaration of state handlers */
+static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
+					 u8 status, u8 data);
+static int mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms,
+				     u8 status, u8 data);
+
+/*
+ * IDLE state
+ *
+ * No transfers are in progress; if another transfer is pending then retrieve
+ * it and kick it off.  Otherwise, stop processing the state machine
+ */
+static int
+mpc52xx_spi_fsmstate_idle(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
+{
+	struct spi_message *m;
+	struct spi_device *spi;
+	int spr, sppr;
+	u8 ctrl1;
+
+	if (status && (irq != NO_IRQ))
+		dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
+			status);
+
+	/* Check if there is another transfer waiting */
+	if (list_empty(&ms->queue))
+		return FSM_STOP;
+
+	/* Get the next message */
+	spin_lock(&ms->lock);
+
+	/* Call the pre-message hook with a pointer to the next
+	 * message.  The pre-message hook may enqueue a new message for
+	 * changing the chip select value to the head of the queue */
+	m = list_first_entry(&ms->queue, struct spi_message, queue);
+	if (ms->premessage)
+		ms->premessage(m, ms->premessage_context);
+
+	/* reget the head of the queue (the premessage hook may have enqueued
+	 * something before it.) and drop the spinlock */
+	ms->message = list_first_entry(&ms->queue, struct spi_message, queue);
+	list_del_init(&ms->message->queue);
+	spin_unlock(&ms->lock);
+
+	/* Setup the controller parameters */
+	ctrl1 = SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR;
+	spi = ms->message->spi;
+	if (spi->mode & SPI_CPHA)
+		ctrl1 |= SPI_CTRL1_CPHA;
+	if (spi->mode & SPI_CPOL)
+		ctrl1 |= SPI_CTRL1_CPOL;
+	if (spi->mode & SPI_LSB_FIRST)
+		ctrl1 |= SPI_CTRL1_LSBFE;
+	writeb(ctrl1, ms->regs + SPI_CTRL1);
+
+	/* Setup the controller speed */
+	/* minimum divider is '2'.  Also, add '1' to force rounding up. */
+	sppr = ((ms->ipb_freq / ms->message->spi->max_speed_hz) + 1) >> 1;
+	spr = 0;
+	if (sppr < 1)
+		sppr = 1;
+	while (((sppr - 1) & ~0x7) != 0) {
+		sppr = (sppr + 1) >> 1; /* add '1' to force rounding up */
+		spr++;
+	}
+	sppr--;		/* sppr quantity in register is offset by 1 */
+	if (spr > 7) {
+		/* Don't overrun limits of SPI baudrate register */
+		spr = 7;
+		sppr = 7;
+	}
+	writeb(sppr << 4 | spr, ms->regs + SPI_BRR); /* Set speed */
+
+	ms->cs_change = 1;
+	ms->transfer = container_of(ms->message->transfers.next,
+				    struct spi_transfer, transfer_list);
+
+	mpc52xx_spi_start_transfer(ms);
+	ms->state = mpc52xx_spi_fsmstate_transfer;
+
+#if defined(VERBOSE_DEBUG)
+	dev_info(&ms->master->dev, "msg:%p, max_speed:%i, brr:%.2x\n",
+		 ms->message, ms->message->spi->max_speed_hz,
+		 readb(ms->regs + SPI_BRR));
+#endif
+
+	return FSM_CONTINUE;
+}
+
+/*
+ * TRANSFER state
+ *
+ * In the middle of a transfer.  If the SPI core has completed processing
+ * a byte, then read out the received data and write out the next byte
+ * (unless this transfer is finished; in which case go on to the wait
+ * state)
+ */
+static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
+					 u8 status, u8 data)
+{
+	if (!status)
+		return ms->irq0 == NO_IRQ ? FSM_POLL : FSM_STOP;
+
+	if (status & SPI_STATUS_WCOL) {
+		/* The SPI device is stoopid.  At slower speeds, it may raise
+		 * the SPIF flag before the state machine is actually finished.
+		 * which causes a collision (internal to the state machine
+		 * only).  The manual recommends inserting a delay between
+		 * receving the interrupt and sending the next byte, but
+		 * it can also be worked around simply by retrying the
+		 * transfer which is what we do here. */
+		ms->wcol_count++;
+		ms->wcol_ticks += get_tbl() - ms->wcol_tx_timestamp;
+		ms->wcol_tx_timestamp = get_tbl();
+		data = 0;
+		if (ms->tx_buf)
+			data = *(ms->tx_buf-1);
+		writeb(data, ms->regs + SPI_DATA); /* try again */
+		return FSM_CONTINUE;
+	} else if (status & SPI_STATUS_MODF) {
+		ms->modf_count++;
+		dev_err(&ms->master->dev, "mod fault\n");
+		mpc52xx_spi_chipsel(ms, 0);
+		ms->message->status = -EIO;
+		if (ms->message->complete)
+			ms->message->complete(ms->message->context);
+		ms->state = mpc52xx_spi_fsmstate_idle;
+		return FSM_CONTINUE;
+	}
+
+	/* Read data out of the spi device */
+	ms->byte_count++;
+	if (ms->rx_buf)
+		*ms->rx_buf++ = data;
+
+	/* Is the transfer complete? */
+	ms->len--;
+	if (ms->len == 0) {
+		ms->timestamp = get_tbl();
+		ms->timestamp += ms->transfer->delay_usecs * tb_ticks_per_usec;
+		ms->state = mpc52xx_spi_fsmstate_wait;
+		return FSM_CONTINUE;
+	}
+
+	/* Write out the next byte */
+	ms->wcol_tx_timestamp = get_tbl();
+	if (ms->tx_buf)
+		writeb(*ms->tx_buf++, ms->regs + SPI_DATA);
+	else
+		writeb(0, ms->regs + SPI_DATA);
+
+	return FSM_CONTINUE;
+}
+
+/*
+ * WAIT state
+ *
+ * A transfer has completed; need to wait for the delay period to complete
+ * before starting the next transfer
+ */
+static int
+mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
+{
+	if (status && irq != NO_IRQ)
+		dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
+			status);
+
+	if (((int)get_tbl()) - ms->timestamp < 0)
+		return FSM_POLL;
+
+	ms->message->actual_length += ms->transfer->len;
+
+	/* Check if there is another transfer in this message.  If there
+	 * aren't then deactivate CS, notify sender, and drop back to idle
+	 * to start the next message. */
+	if (ms->transfer->transfer_list.next == &ms->message->transfers) {
+		ms->msg_count++;
+		mpc52xx_spi_chipsel(ms, 0);
+		ms->message->status = 0;
+		if (ms->message->complete)
+			ms->message->complete(ms->message->context);
+		ms->state = mpc52xx_spi_fsmstate_idle;
+		return FSM_CONTINUE;
+	}
+
+	/* There is another transfer; kick it off */
+
+	if (ms->cs_change)
+		mpc52xx_spi_chipsel(ms, 0);
+
+	ms->transfer = container_of(ms->transfer->transfer_list.next,
+				    struct spi_transfer, transfer_list);
+	mpc52xx_spi_start_transfer(ms);
+	ms->state = mpc52xx_spi_fsmstate_transfer;
+	return FSM_CONTINUE;
+}
+
+/*
+ * IRQ handler
+ */
+static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
+{
+	struct mpc52xx_spi *ms = _ms;
+	int rc = FSM_CONTINUE;
+	u8 status, data;
+
+	while (rc == FSM_CONTINUE) {
+		/* Interrupt cleared by read of STATUS followed by
+		 * read of DATA registers*/
+		status = readb(ms->regs + SPI_STATUS);
+		data = readb(ms->regs + SPI_DATA); /* clear status */
+		rc = ms->state(irq, ms, status, data);
+	}
+
+	if (rc == FSM_POLL)
+		schedule_work(&ms->work);
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * Workqueue method of running the state machine
+ */
+static void mpc52xx_spi_wq(struct work_struct *work)
+{
+	struct mpc52xx_spi *ms = container_of(work, struct mpc52xx_spi, work);
+	mpc52xx_spi_irq(NO_IRQ, ms);
+}
+
+/*
+ * spi_master callbacks
+ */
+
+static int mpc52xx_spi_setup(struct spi_device *spi)
+{
+	return 0;
+}
+
+static int mpc52xx_spi_transfer(struct spi_device *spi, struct spi_message *m)
+{
+	struct mpc52xx_spi *ms = spi_master_get_devdata(spi->master);
+	unsigned long flags;
+
+	m->actual_length = 0;
+	m->status = -EINPROGRESS;
+
+	spin_lock_irqsave(&ms->lock, flags);
+	list_add_tail(&m->queue, &ms->queue);
+	spin_unlock_irqrestore(&ms->lock, flags);
+	schedule_work(&ms->work);
+
+	return 0;
+}
+
+/*
+ * Hook to modify premessage hook
+ */
+void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
+				     void (*hook)(struct spi_message *m,
+						  void *context),
+				     void *hook_context)
+{
+	struct mpc52xx_spi *ms = spi_master_get_devdata(master);
+	ms->premessage = hook;
+	ms->premessage_context = hook_context;
+}
+EXPORT_SYMBOL(mpc52xx_spi_set_premessage_hook);
+
+/*
+ * SysFS files
+ */
+static int
+*mpc52xx_spi_sysfs_get_counter(struct mpc52xx_spi *ms, const char *name)
+{
+	if (strcmp(name, "msg_count") == 0)
+		return &ms->msg_count;
+	if (strcmp(name, "byte_count") == 0)
+		return &ms->byte_count;
+	if (strcmp(name, "wcol_count") == 0)
+		return &ms->wcol_count;
+	if (strcmp(name, "wcol_ticks") == 0)
+		return &ms->wcol_ticks;
+	if (strcmp(name, "modf_count") == 0)
+		return &ms->modf_count;
+	return NULL;
+}
+
+static ssize_t mpc52xx_spi_show_count(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	struct spi_master *master = container_of(dev, struct spi_master, dev);
+	struct mpc52xx_spi *ms = spi_master_get_devdata(master);
+	int *counter;
+
+	counter = mpc52xx_spi_sysfs_get_counter(ms, attr->attr.name);
+	if (!counter)
+		return sprintf(buf, "error\n");
+	return sprintf(buf, "%d\n", *counter);
+}
+
+static ssize_t mpc52xx_spi_set_count(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct spi_master *master = container_of(dev, struct spi_master, dev);
+	struct mpc52xx_spi *ms = spi_master_get_devdata(master);
+	int *counter;
+	int value = simple_strtoul(buf, NULL, 0);
+
+	counter = mpc52xx_spi_sysfs_get_counter(ms, attr->attr.name);
+	if (counter)
+		*counter = value;
+	return count;
+}
+
+DEVICE_ATTR(msg_count, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
+DEVICE_ATTR(byte_count, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
+DEVICE_ATTR(wcol_count, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
+DEVICE_ATTR(wcol_ticks, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
+DEVICE_ATTR(modf_count, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
+
+/*
+ * OF Platform Bus Binding
+ */
+static int __devinit mpc52xx_spi_of_probe(struct of_device *op,
+					  const struct of_device_id *match)
+{
+	struct spi_master *master;
+	struct mpc52xx_spi *ms;
+	void __iomem *regs;
+	const u32 *prop;
+	int rc, len;
+
+	/* MMIO registers */
+	dev_dbg(&op->dev, "probing mpc5200 SPI device\n");
+	regs = of_iomap(op->node, 0);
+	if (!regs)
+		return -ENODEV;
+
+	/* initialize the device */
+	writeb(SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR, regs+SPI_CTRL1);
+	writeb(0x0, regs + SPI_CTRL2);
+	writeb(0xe, regs + SPI_DATADIR);	/* Set output pins */
+	writeb(0x8, regs + SPI_PORTDATA);	/* Deassert /SS signal */
+
+	/* Clear the status register and re-read it to check for a MODF
+	 * failure.  This driver cannot currently handle multiple masters
+	 * on the SPI bus.  This fault will also occur if the SPI signals
+	 * are not connected to any pins (port_config setting) */
+	readb(regs + SPI_STATUS);
+	readb(regs + SPI_DATA);
+	if (readb(regs + SPI_STATUS) & SPI_STATUS_MODF) {
+		dev_err(&op->dev, "mode fault; is port_config correct?\n");
+		return -EIO;
+	}
+
+	dev_dbg(&op->dev, "allocating spi_master struct\n");
+	master = spi_alloc_master(&op->dev, sizeof *ms);
+	if (!master)
+		return -ENOMEM;
+	master->bus_num = -1;
+	master->num_chipselect = 1;
+	prop = of_get_property(op->node, "num-slaves", &len);
+	if (prop && len >= sizeof(*prop))
+		master->num_chipselect = *prop;
+
+	master->setup = mpc52xx_spi_setup;
+	master->transfer = mpc52xx_spi_transfer;
+	dev_set_drvdata(&op->dev, master);
+
+	ms = spi_master_get_devdata(master);
+	ms->master = master;
+	ms->regs = regs;
+	ms->irq0 = irq_of_parse_and_map(op->node, 0);
+	ms->irq1 = irq_of_parse_and_map(op->node, 1);
+	ms->state = mpc52xx_spi_fsmstate_idle;
+	ms->ipb_freq = mpc52xx_find_ipb_freq(op->node);
+	spin_lock_init(&ms->lock);
+	INIT_LIST_HEAD(&ms->queue);
+	INIT_WORK(&ms->work, mpc52xx_spi_wq);
+
+	dev_dbg(&op->dev, "registering spi_master struct\n");
+	rc = spi_register_master(master);
+	if (rc < 0)
+		goto err_register;
+
+	/* Decide if interrupts can be used */
+	if ((ms->irq0 != NO_IRQ) && (ms->irq1 != NO_IRQ)) {
+		rc = request_irq(ms->irq0, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
+				  "mpc5200-spi-modf", ms);
+		rc |= request_irq(ms->irq1, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
+				  "mpc5200-spi-spiF", ms);
+		if (rc) {
+			free_irq(ms->irq0, ms);
+			free_irq(ms->irq1, ms);
+			ms->irq0 = ms->irq1 = NO_IRQ;
+			dev_info(&op->dev, "using polled mode\n");
+		}
+	} else {
+		/* operate in polled mode */
+		ms->irq0 = ms->irq1 = NO_IRQ;
+		dev_info(&op->dev, "using polled mode\n");
+	}
+
+	/* Create SysFS files */
+	rc = device_create_file(&ms->master->dev, &dev_attr_msg_count);
+	rc |= device_create_file(&ms->master->dev, &dev_attr_byte_count);
+	rc |= device_create_file(&ms->master->dev, &dev_attr_wcol_count);
+	rc |= device_create_file(&ms->master->dev, &dev_attr_wcol_ticks);
+	rc |= device_create_file(&ms->master->dev, &dev_attr_modf_count);
+	if (rc)
+		dev_info(&ms->master->dev, "error creating sysfs files\n");
+
+	dev_info(&ms->master->dev, "registered MPC5200 SPI bus\n");
+
+	of_register_spi_devices(master, op->node);
+
+	return rc;
+
+ err_register:
+	dev_err(&ms->master->dev, "initialization failed\n");
+	spi_master_put(master);
+	return rc;
+}
+
+static void __devexit mpc52xx_spi_of_remove(struct of_device *op)
+{
+	struct spi_master *master = dev_get_drvdata(&op->dev);
+	struct mpc52xx_spi *ms = spi_master_get_devdata(master);
+
+	device_remove_file(&ms->master->dev, &dev_attr_msg_count);
+	device_remove_file(&ms->master->dev, &dev_attr_byte_count);
+	device_remove_file(&ms->master->dev, &dev_attr_wcol_count);
+	device_remove_file(&ms->master->dev, &dev_attr_wcol_ticks);
+	device_remove_file(&ms->master->dev, &dev_attr_modf_count);
+
+	free_irq(ms->irq0, ms);
+	free_irq(ms->irq1, ms);
+
+	spi_unregister_master(master);
+	spi_master_put(master);
+	iounmap(ms->regs);
+}
+
+static struct of_device_id mpc52xx_spi_of_match[] __devinitdata = {
+	{ .compatible = "fsl,mpc5200-spi", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mpc52xx_psc_spi_of_match);
+
+static struct of_platform_driver mpc52xx_spi_of_driver = {
+	.owner = THIS_MODULE,
+	.name = "mpc52xx-spi",
+	.match_table = mpc52xx_spi_of_match,
+	.probe = mpc52xx_spi_of_probe,
+	.remove = __exit_p(mpc52xx_spi_of_remove),
+};
+
+static int __init mpc52xx_spi_init(void)
+{
+	return of_register_platform_driver(&mpc52xx_spi_of_driver);
+}
+module_init(mpc52xx_spi_init);
+
+static void __exit mpc52xx_spi_exit(void)
+{
+	of_unregister_platform_driver(&mpc52xx_spi_of_driver);
+}
+module_exit(mpc52xx_spi_exit);
+
diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
new file mode 100644
index 0000000..d1004cf
--- /dev/null
+++ b/include/linux/spi/mpc52xx_spi.h
@@ -0,0 +1,10 @@
+
+#ifndef INCLUDE_MPC5200_SPI_H
+#define INCLUDE_MPC5200_SPI_H
+
+extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
+					    void (*hook)(struct spi_message *m,
+							 void *context),
+					    void *hook_context);
+
+#endif

Re: [PATCH v3 4/4] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver

[Daniel Walker]

On Fri, 2008-07-25 at 03:33 -0400, Grant Likely wrote:

> +	if (status && (irq != NO_IRQ))
> +		dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> +			status);
> +
> +	/* Check if there is another transfer waiting */
> +	if (list_empty(&ms->queue))
> +		return FSM_STOP;

I don't think doing list_empty outside the critical section is totally
safe.. You might want to move it down inside the spin_lock() section.

> +	/* Get the next message */
> +	spin_lock(&ms->lock);

The part that's a little confusing here is that the interrupt can
actually activate the workqueue .. So I'm wondering if maybe you could
have this interrupt driven any workqueue driven at the same time? If you
could then you would need the above to be 
spin_lock_irq/spin_lock_irqsave ..

Daniel

Re: [PATCH v3 4/4] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver

[Grant Likely]

On Fri, Jul 25, 2008 at 2:19 PM, Daniel Walker <dwalker@mvista.com> wrote:
> On Fri, 2008-07-25 at 03:33 -0400, Grant Likely wrote:
>
>> +     if (status && (irq != NO_IRQ))
>> +             dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
>> +                     status);
>> +
>> +     /* Check if there is another transfer waiting */
>> +     if (list_empty(&ms->queue))
>> +             return FSM_STOP;
>
> I don't think doing list_empty outside the critical section is totally
> safe.. You might want to move it down inside the spin_lock() section.

This should be fine.  This is the only place where items are dequeued,
and it will only ever be called from the ISR or the work queue.  The
work queue and IRQ will never be active at the same time (I'll add a
comment to the fact).  It also looks like list_empty is perfectly safe
to call without the protection of a spin lock (but somebody correct me
if I'm out to lunch).  It doesn't dereference any of the pointers in
the list_head structure.

>
>> +     /* Get the next message */
>> +     spin_lock(&ms->lock);
>
> The part that's a little confusing here is that the interrupt can
> actually activate the workqueue .. So I'm wondering if maybe you could
> have this interrupt driven any workqueue driven at the same time? If you
> could then you would need the above to be
> spin_lock_irq/spin_lock_irqsave ..

Ditto here, since the irq and workqueue are not enabled at the same
time there is no worry about collision.

Cheers,
g.



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 4/4] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver

[Daniel Walker]

On Fri, 2008-07-25 at 22:45 -0400, Grant Likely wrote:
> On Fri, Jul 25, 2008 at 2:19 PM, Daniel Walker <dwalker@mvista.com> wrote:
> > On Fri, 2008-07-25 at 03:33 -0400, Grant Likely wrote:
> >
> >> +     if (status && (irq != NO_IRQ))
> >> +             dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> >> +                     status);
> >> +
> >> +     /* Check if there is another transfer waiting */
> >> +     if (list_empty(&ms->queue))
> >> +             return FSM_STOP;
> >
> > I don't think doing list_empty outside the critical section is totally
> > safe.. You might want to move it down inside the spin_lock() section.
> 
> This should be fine.  This is the only place where items are dequeued,
> and it will only ever be called from the ISR or the work queue.  The
> work queue and IRQ will never be active at the same time (I'll add a
> comment to the fact).  It also looks like list_empty is perfectly safe
> to call without the protection of a spin lock (but somebody correct me
> if I'm out to lunch).  It doesn't dereference any of the pointers in
> the list_head structure.

The list_empty wouldn't crash, but the result isn't necessarily
accurate.

> >
> >> +     /* Get the next message */
> >> +     spin_lock(&ms->lock);
> >
> > The part that's a little confusing here is that the interrupt can
> > actually activate the workqueue .. So I'm wondering if maybe you could
> > have this interrupt driven any workqueue driven at the same time? If you
> > could then you would need the above to be
> > spin_lock_irq/spin_lock_irqsave ..
> 
> Ditto here, since the irq and workqueue are not enabled at the same
> time there is no worry about collision.

Why are you waking up the work queue from inside the irq handler? You
might also want to break out the handling from inside the irq handler
and call that from the workqueue, instead of re-calling the irq handler
function from the workqueue.. It's a little confusing from a context
perspective.

Daniel

Re: [PATCH v3 4/4] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver

[Grant Likely]

On Sat, Jul 26, 2008 at 12:47 AM, Daniel Walker <dwalker@mvista.com> wrote:
> On Fri, 2008-07-25 at 22:45 -0400, Grant Likely wrote:
>> On Fri, Jul 25, 2008 at 2:19 PM, Daniel Walker <dwalker@mvista.com> wrote:
>> > On Fri, 2008-07-25 at 03:33 -0400, Grant Likely wrote:
>> >
>> >> +     if (status && (irq != NO_IRQ))
>> >> +             dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
>> >> +                     status);
>> >> +
>> >> +     /* Check if there is another transfer waiting */
>> >> +     if (list_empty(&ms->queue))
>> >> +             return FSM_STOP;
>> >
>> > I don't think doing list_empty outside the critical section is totally
>> > safe.. You might want to move it down inside the spin_lock() section.
>>
>> This should be fine.  This is the only place where items are dequeued,
>> and it will only ever be called from the ISR or the work queue.  The
>> work queue and IRQ will never be active at the same time (I'll add a
>> comment to the fact).  It also looks like list_empty is perfectly safe
>> to call without the protection of a spin lock (but somebody correct me
>> if I'm out to lunch).  It doesn't dereference any of the pointers in
>> the list_head structure.
>
> The list_empty wouldn't crash, but the result isn't necessarily
> accurate.

It doesn't need to be accurate because the spinlock is obtained before
actually trying to dequeue anything.  Even if it held the spinlock
there would still be uncertainty depending on which routine ran first.
 If this function ran first, then it's going to stop the state machine
regardless and the enqueue function will have to kick it off again
anyway.

But, I've taken a look at the code, and you're right, the spin lock
does need to be held while running the state machine because I do have
a case where the IRQ and workqueue could get executed at the same
time.  I'll fix it up.

>
>> >
>> >> +     /* Get the next message */
>> >> +     spin_lock(&ms->lock);
>> >
>> > The part that's a little confusing here is that the interrupt can
>> > actually activate the workqueue .. So I'm wondering if maybe you could
>> > have this interrupt driven any workqueue driven at the same time? If you
>> > could then you would need the above to be
>> > spin_lock_irq/spin_lock_irqsave ..
>>
>> Ditto here, since the irq and workqueue are not enabled at the same
>> time there is no worry about collision.
>
> Why are you waking up the work queue from inside the irq handler? You
> might also want to break out the handling from inside the irq handler
> and call that from the workqueue, instead of re-calling the irq handler
> function from the workqueue.. It's a little confusing from a context
> perspective.

Sure, I can rework that.  The irq handler would then just simply be a
straight call into the state machine runner.  I just dropped the
additional 4 lines of code for conciseness, but I can change it around
for clarity... actually, with the spin lock stuff above I need to do
this rework.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 4/4] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver

[David Brownell]

On Friday 25 July 2008, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> 
> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> SoC.

It'd be a bit more clear if you said dedicated SPI "controller";
"device" sounds like it's a "struct spi_device".

Capsule summary:  fault handling needs work.  (Details below.)


> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> 
>  drivers/spi/Kconfig             |    8 +
>  drivers/spi/Makefile            |    1 
>  drivers/spi/mpc52xx_spi.c       |  595 +++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/mpc52xx_spi.h |   10 +
>  4 files changed, 614 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 2303521..68e4a4a 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -116,6 +116,14 @@ config SPI_LM70_LLP
>  	  which interfaces to an LM70 temperature sensor using
>  	  a parallel port.
>  
> +config SPI_MPC52xx
> +	tristate "Freescale MPC52xx SPI (non-PSC) controller support"
> +	depends on PPC_MPC52xx && SPI
> +	select SPI_MASTER_OF
> +	help
> +	  This drivers supports the MPC52xx SPI controller in master SPI
> +	  mode.
> +
>  config SPI_MPC52xx_PSC
>  	tristate "Freescale MPC52xx PSC SPI controller"
>  	depends on PPC_MPC52xx && EXPERIMENTAL
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 7fca043..340b878 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_SPI_PXA2XX)		+= pxa2xx_spi.o
>  obj-$(CONFIG_SPI_OMAP_UWIRE)		+= omap_uwire.o
>  obj-$(CONFIG_SPI_OMAP24XX)		+= omap2_mcspi.o
>  obj-$(CONFIG_SPI_MPC52xx_PSC)		+= mpc52xx_psc_spi.o
> +obj-$(CONFIG_SPI_MPC52xx)		+= mpc52xx_spi.o
>  obj-$(CONFIG_SPI_MPC83xx)		+= spi_mpc83xx.o
>  obj-$(CONFIG_SPI_S3C24XX_GPIO)		+= spi_s3c24xx_gpio.o
>  obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
> diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
> new file mode 100644
> index 0000000..453690f
> --- /dev/null
> +++ b/drivers/spi/mpc52xx_spi.c
> @@ -0,0 +1,595 @@
> +/*
> + * MPC52xx SPI master driver.
> + * Copyright (C) 2008 Secret Lab Technologies Ltd.
> + *
> + * This is the driver for the MPC5200's dedicated SPI device (not for a
> + * PSC in SPI mode)
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/of_platform.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/mpc52xx_spi.h>
> +#include <linux/of_spi.h>
> +#include <linux/io.h>
> +#include <asm/time.h>
> +#include <asm/mpc52xx.h>
> +
> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
> +MODULE_LICENSE("GPL");
> +
> +/* Register offsets */
> +#define SPI_CTRL1	0x00
> +#define SPI_CTRL1_SPIE		(1 << 7)
> +#define SPI_CTRL1_SPE		(1 << 6)
> +#define SPI_CTRL1_MSTR		(1 << 4)
> +#define SPI_CTRL1_CPOL		(1 << 3)
> +#define SPI_CTRL1_CPHA		(1 << 2)
> +#define SPI_CTRL1_SSOE		(1 << 1)
> +#define SPI_CTRL1_LSBFE		(1 << 0)
> +
> +#define SPI_CTRL2	0x01
> +#define SPI_BRR		0x04
> +
> +#define SPI_STATUS	0x05
> +#define SPI_STATUS_SPIF		(1 << 7)
> +#define SPI_STATUS_WCOL		(1 << 6)
> +#define SPI_STATUS_MODF		(1 << 4)
> +
> +#define SPI_DATA	0x09
> +#define SPI_PORTDATA	0x0d
> +#define SPI_DATADIR	0x10
> +
> +/* FSM state return values */
> +#define FSM_STOP	0
> +#define FSM_POLL	1
> +#define FSM_CONTINUE	2
> +
> +/* Driver internal data */
> +struct mpc52xx_spi {
> +	struct spi_master *master;
> +	u32 sysclk;
> +	void __iomem *regs;
> +	int irq0;	/* MODF irq */
> +	int irq1;	/* SPIF irq */
> +	int ipb_freq;
> +
> +	/* Statistics */
> +	int msg_count;
> +	int wcol_count;
> +	int wcol_ticks;
> +	u32 wcol_tx_timestamp;
> +	int modf_count;
> +	int byte_count;
> +
> +	/* Hooks for platform modification of behaviour */
> +	void (*premessage)(struct spi_message *m, void *context);
> +	void *premessage_context;
> +
> +	struct list_head queue;		/* queue of pending messages */
> +	spinlock_t lock;
> +	struct work_struct work;
> +
> +
> +	/* Details of current transfer (length, and buffer pointers) */
> +	struct spi_message *message;	/* current message */
> +	struct spi_transfer *transfer;	/* current transfer */
> +	int (*state)(int irq, struct mpc52xx_spi *ms, u8 status, u8 data);
> +	int len;
> +	int timestamp;
> +	u8 *rx_buf;
> +	const u8 *tx_buf;
> +	int cs_change;
> +};
> +
> +/*
> + * CS control function
> + */
> +static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
> +{
> +	if (value)
> +		writeb(0, ms->regs + SPI_PORTDATA); /* Assert SS pin */
> +	else
> +		writeb(0x08, ms->regs + SPI_PORTDATA); /* Deassert SS pin */
> +}
> +
> +/*
> + * Start a new transfer.  This is called both by the idle state
> + * for the first transfer in a message, and by the wait state when the
> + * previous transfer in a message is complete.
> + */
> +static void mpc52xx_spi_start_transfer(struct mpc52xx_spi *ms)
> +{
> +	ms->rx_buf = ms->transfer->rx_buf;
> +	ms->tx_buf = ms->transfer->tx_buf;
> +	ms->len = ms->transfer->len;
> +
> +	/* Activate the chip select */
> +	if (ms->cs_change)
> +		mpc52xx_spi_chipsel(ms, 1);
> +	ms->cs_change = ms->transfer->cs_change;
> +
> +	/* Write out the first byte */
> +	ms->wcol_tx_timestamp = get_tbl();
> +	if (ms->tx_buf)
> +		writeb(*ms->tx_buf++, ms->regs + SPI_DATA);
> +	else
> +		writeb(0, ms->regs + SPI_DATA);
> +}
> +
> +/* Forward declaration of state handlers */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> +					 u8 status, u8 data);
> +static int mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms,
> +				     u8 status, u8 data);
> +
> +/*
> + * IDLE state
> + *
> + * No transfers are in progress; if another transfer is pending then retrieve
> + * it and kick it off.  Otherwise, stop processing the state machine
> + */
> +static int
> +mpc52xx_spi_fsmstate_idle(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> +	struct spi_message *m;
> +	struct spi_device *spi;
> +	int spr, sppr;
> +	u8 ctrl1;
> +
> +	if (status && (irq != NO_IRQ))
> +		dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> +			status);
> +
> +	/* Check if there is another transfer waiting */
> +	if (list_empty(&ms->queue))
> +		return FSM_STOP;

As Daniel noted:  the queue is protected by the spinlock,
so grab that first.  And you said you'd fix that.


> +
> +	/* Get the next message */
> +	spin_lock(&ms->lock);
> +
> +	/* Call the pre-message hook with a pointer to the next
> +	 * message.  The pre-message hook may enqueue a new message for
> +	 * changing the chip select value to the head of the queue */
> +	m = list_first_entry(&ms->queue, struct spi_message, queue);

I don't quite see the point of this pre-message extension;
the kerneldoc there is kind of opaque.  How could it queue a
new message that would affect the head of the queue??  The
relevant data structures aren't even visible to that function!

That said:

> +	if (ms->premessage)
> +		ms->premessage(m, ms->premessage_context);
> +
> +	/* reget the head of the queue (the premessage hook may have enqueued
> +	 * something before it.) and drop the spinlock */
> +	ms->message = list_first_entry(&ms->queue, struct spi_message, queue);

	if (ms->premessage) {
		hook();
		ms->message = ...
	} else
		ms->message = m;

... would be more clear to me, at least if I could see a way
for that "premessage hook" to modify the queue in any way other
than calling spi_transfer() to *append* an entry.

Also, it'd be more clear to have this function use "m" for its
working state not "ms->message" ... and at least some versions
of GCC would generate much better code that way, since they
wouldn't need to reload the register.


> +	list_del_init(&ms->message->queue);
> +	spin_unlock(&ms->lock);
> +
> +	/* Setup the controller parameters */
> +	ctrl1 = SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR;
> +	spi = ms->message->spi;
> +	if (spi->mode & SPI_CPHA)
> +		ctrl1 |= SPI_CTRL1_CPHA;
> +	if (spi->mode & SPI_CPOL)
> +		ctrl1 |= SPI_CTRL1_CPOL;
> +	if (spi->mode & SPI_LSB_FIRST)
> +		ctrl1 |= SPI_CTRL1_LSBFE;
> +	writeb(ctrl1, ms->regs + SPI_CTRL1);
> +
> +	/* Setup the controller speed */
> +	/* minimum divider is '2'.  Also, add '1' to force rounding up. */
> +	sppr = ((ms->ipb_freq / ms->message->spi->max_speed_hz) + 1) >> 1;
> +	spr = 0;
> +	if (sppr < 1)
> +		sppr = 1;
> +	while (((sppr - 1) & ~0x7) != 0) {
> +		sppr = (sppr + 1) >> 1; /* add '1' to force rounding up */
> +		spr++;
> +	}
> +	sppr--;		/* sppr quantity in register is offset by 1 */
> +	if (spr > 7) {
> +		/* Don't overrun limits of SPI baudrate register */
> +		spr = 7;
> +		sppr = 7;

So here you're setting the SPI clock rate faster than the spi_device
says it can handle?  Bad idea!  There should be an error report.  In
this case, it's best done in the setup() callback -- it could just
compute and save the SPI_BRR value in chip-specific data -- so that
you never get errors at this point.


> +	}
> +	writeb(sppr << 4 | spr, ms->regs + SPI_BRR); /* Set speed */

It'd be better to have that "set speed" logic be a subroutine, so
that you can use it for both per-message setup and for per-transfer
overrides.

In a similar vein, you're ignoring spi->bits_per_word completely...
looks like you're assuming it's always eight.


> +
> +	ms->cs_change = 1;
> +	ms->transfer = container_of(ms->message->transfers.next,
> +				    struct spi_transfer, transfer_list);
> +
> +	mpc52xx_spi_start_transfer(ms);
> +	ms->state = mpc52xx_spi_fsmstate_transfer;
> +
> +#if defined(VERBOSE_DEBUG)
> +	dev_info(&ms->master->dev, "msg:%p, max_speed:%i, brr:%.2x\n",

So just make this dev_vdbg() ... not #ifdef + dev_info().

> +		 ms->message, ms->message->spi->max_speed_hz,
> +		 readb(ms->regs + SPI_BRR));
> +#endif
> +
> +	return FSM_CONTINUE;
> +}
> +
> +/*
> + * TRANSFER state
> + *
> + * In the middle of a transfer.  If the SPI core has completed processing
> + * a byte, then read out the received data and write out the next byte
> + * (unless this transfer is finished; in which case go on to the wait
> + * state)
> + */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> +					 u8 status, u8 data)
> +{
> +	if (!status)
> +		return ms->irq0 == NO_IRQ ? FSM_POLL : FSM_STOP;
> +
> +	if (status & SPI_STATUS_WCOL) {
> +		/* The SPI device is stoopid.  At slower speeds, it may raise
> +		 * the SPIF flag before the state machine is actually finished.
> +		 * which causes a collision (internal to the state machine
> +		 * only).  The manual recommends inserting a delay between
> +		 * receving the interrupt and sending the next byte, but
> +		 * it can also be worked around simply by retrying the
> +		 * transfer which is what we do here. */
> +		ms->wcol_count++;
> +		ms->wcol_ticks += get_tbl() - ms->wcol_tx_timestamp;
> +		ms->wcol_tx_timestamp = get_tbl();
> +		data = 0;
> +		if (ms->tx_buf)
> +			data = *(ms->tx_buf-1);
> +		writeb(data, ms->regs + SPI_DATA); /* try again */
> +		return FSM_CONTINUE;
> +	} else if (status & SPI_STATUS_MODF) {
> +		ms->modf_count++;
> +		dev_err(&ms->master->dev, "mod fault\n");

Is this "mode fault"?  A "mod fault" would be one of the
weak episodes of "The Mod Squad". ;)


> +		mpc52xx_spi_chipsel(ms, 0);
> +		ms->message->status = -EIO;
> +		if (ms->message->complete)
> +			ms->message->complete(ms->message->context);

All messages must have completion functions; don't check.

And drop the spinlock before calling the completion, since
it's normal for such functions to resubmit ... and hence
re-enter this driver.


> +		ms->state = mpc52xx_spi_fsmstate_idle;
> +		return FSM_CONTINUE;
> +	}
> +
> +	/* Read data out of the spi device */
> +	ms->byte_count++;
> +	if (ms->rx_buf)
> +		*ms->rx_buf++ = data;
> +
> +	/* Is the transfer complete? */
> +	ms->len--;
> +	if (ms->len == 0) {
> +		ms->timestamp = get_tbl();
> +		ms->timestamp += ms->transfer->delay_usecs * tb_ticks_per_usec;
> +		ms->state = mpc52xx_spi_fsmstate_wait;
> +		return FSM_CONTINUE;
> +	}
> +
> +	/* Write out the next byte */
> +	ms->wcol_tx_timestamp = get_tbl();
> +	if (ms->tx_buf)
> +		writeb(*ms->tx_buf++, ms->regs + SPI_DATA);
> +	else
> +		writeb(0, ms->regs + SPI_DATA);
> +
> +	return FSM_CONTINUE;
> +}
> +
> +/*
> + * WAIT state
> + *
> + * A transfer has completed; need to wait for the delay period to complete
> + * before starting the next transfer
> + */
> +static int
> +mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> +	if (status && irq != NO_IRQ)
> +		dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> +			status);
> +
> +	if (((int)get_tbl()) - ms->timestamp < 0)
> +		return FSM_POLL;
> +
> +	ms->message->actual_length += ms->transfer->len;

Subtract ms->len though, yes?  And abort the transfer if ms->len is
nonzero (controller reported an error or whatever).

> +
> +	/* Check if there is another transfer in this message.  If there
> +	 * aren't then deactivate CS, notify sender, and drop back to idle
> +	 * to start the next message. */
> +	if (ms->transfer->transfer_list.next == &ms->message->transfers) {
> +		ms->msg_count++;
> +		mpc52xx_spi_chipsel(ms, 0);
> +		ms->message->status = 0;
> +		if (ms->message->complete)
> +			ms->message->complete(ms->message->context);

See above about calling completions.

> +		ms->state = mpc52xx_spi_fsmstate_idle;
> +		return FSM_CONTINUE;
> +	}
> +
> +	/* There is another transfer; kick it off */
> +
> +	if (ms->cs_change)
> +		mpc52xx_spi_chipsel(ms, 0);
> +
> +	ms->transfer = container_of(ms->transfer->transfer_list.next,
> +				    struct spi_transfer, transfer_list);
> +	mpc52xx_spi_start_transfer(ms);
> +	ms->state = mpc52xx_spi_fsmstate_transfer;
> +	return FSM_CONTINUE;
> +}
> +
> +/*
> + * IRQ handler
> + */
> +static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
> +{
> +	struct mpc52xx_spi *ms = _ms;
> +	int rc = FSM_CONTINUE;
> +	u8 status, data;
> +
> +	while (rc == FSM_CONTINUE) {
> +		/* Interrupt cleared by read of STATUS followed by
> +		 * read of DATA registers*/
> +		status = readb(ms->regs + SPI_STATUS);
> +		data = readb(ms->regs + SPI_DATA); /* clear status */
> +		rc = ms->state(irq, ms, status, data);
> +	}
> +
> +	if (rc == FSM_POLL)
> +		schedule_work(&ms->work);

When the POLL return is from mpc52xx_spi_fsmstate_wait() should
this maybe be a schedule_work_delayed() ... ?

> +
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * Workqueue method of running the state machine
> + */
> +static void mpc52xx_spi_wq(struct work_struct *work)
> +{
> +	struct mpc52xx_spi *ms = container_of(work, struct mpc52xx_spi, work);
> +	mpc52xx_spi_irq(NO_IRQ, ms);
> +}
> +
> +/*
> + * spi_master callbacks
> + */
> +
> +static int mpc52xx_spi_setup(struct spi_device *spi)
> +{
> +	return 0;

Very unhealthy.  This is claiming success for *ALL* settings,
even invalid ones.  You should validate things here:

  - spi->max_speed_hz is within range of what this supports.

  - (spi->mode & ~(SPI_CPOL|SPI_CPHA|SPI_LSB_FIRST)) == 0
	... since those are the only mode bits you support

  - spi->bits_per_word is valid ... I think that means 8
    (or, synonymously, 0), but I can't tell.

  - spi->chip_select is valid


> +}
> +
> +static int mpc52xx_spi_transfer(struct spi_device *spi, struct spi_message *m)
> +{
> +	struct mpc52xx_spi *ms = spi_master_get_devdata(spi->master);
> +	unsigned long flags;
> +
> +	m->actual_length = 0;
> +	m->status = -EINPROGRESS;

Before adding this to the queue, I suggest you verify that you
can handle this.  Your state machine assumes you did that, but
you aren't ...

Without changing the body of the code you've written already,
I suggest just scanning all the transfers in this message for
bits_per_word or max_speed_hz being nonzero, returning -EINVAL
if any transfer said to not use defaults for that spi_device.
And maybe verify that m->complete is non-null.


> +
> +	spin_lock_irqsave(&ms->lock, flags);
> +	list_add_tail(&m->queue, &ms->queue);
> +	spin_unlock_irqrestore(&ms->lock, flags);

> +	schedule_work(&ms->work);

That looks goofy.  The workqueue just fakes out an IRQ;
but you don't check whether your state machine is active
before doing that, so a *real* IRQ could come in at the
same time and cause confusion.

Probably better to

	if (ms->state == mpc52xx_spi_fsmstate_idle)
		schedule_work()
	spin_unlock_irqrestore(...)

maybe even just call mpc52xx_spi_fsmstate_idle() directly
instead of punting to a (possibly busy) workqueue.


> +
> +	return 0;
> +}
> +
> +/*
> + * Hook to modify premessage hook

Opaque; why does this exist?  If it's not well motivated I'd
rather not see it.  And if it's well motivated I wonder why
it should be specific to this driver ...


> + */
> +void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
> +				     void (*hook)(struct spi_message *m,
> +						  void *context),
> +				     void *hook_context)
> +{
> +	struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> +	ms->premessage = hook;
> +	ms->premessage_context = hook_context;

These ms->premessage values may be changed while something is
accessing them ... you should hold ms->lock to prevent that.

> +}
> +EXPORT_SYMBOL(mpc52xx_spi_set_premessage_hook);

EXPORT_SYMBOL_GPL?


> +
> +/*
> + * SysFS files
> + */
> +static int
> +*mpc52xx_spi_sysfs_get_counter(struct mpc52xx_spi *ms, const char *name)
> +{
> +	if (strcmp(name, "msg_count") == 0)
> +		return &ms->msg_count;
> +	if (strcmp(name, "byte_count") == 0)
> +		return &ms->byte_count;
> +	if (strcmp(name, "wcol_count") == 0)
> +		return &ms->wcol_count;
> +	if (strcmp(name, "wcol_ticks") == 0)
> +		return &ms->wcol_ticks;
> +	if (strcmp(name, "modf_count") == 0)
> +		return &ms->modf_count;
> +	return NULL;
> +}
> +
> +static ssize_t mpc52xx_spi_show_count(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buf)
> +{
> +	struct spi_master *master = container_of(dev, struct spi_master, dev);
> +	struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> +	int *counter;
> +
> +	counter = mpc52xx_spi_sysfs_get_counter(ms, attr->attr.name);
> +	if (!counter)
> +		return sprintf(buf, "error\n");
> +	return sprintf(buf, "%d\n", *counter);
> +}
> +
> +static ssize_t mpc52xx_spi_set_count(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct spi_master *master = container_of(dev, struct spi_master, dev);
> +	struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> +	int *counter;
> +	int value = simple_strtoul(buf, NULL, 0);

Checkpatch suggests strict_strtoul(), which would indeed be better...


> +
> +	counter = mpc52xx_spi_sysfs_get_counter(ms, attr->attr.name);
> +	if (counter)
> +		*counter = value;
> +	return count;
> +}
> +
> +DEVICE_ATTR(msg_count, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
> +DEVICE_ATTR(byte_count, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
> +DEVICE_ATTR(wcol_count, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
> +DEVICE_ATTR(wcol_ticks, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
> +DEVICE_ATTR(modf_count, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
> +
> +/*
> + * OF Platform Bus Binding
> + */
> +static int __devinit mpc52xx_spi_of_probe(struct of_device *op,
> +					  const struct of_device_id *match)
> +{
> +	struct spi_master *master;
> +	struct mpc52xx_spi *ms;
> +	void __iomem *regs;
> +	const u32 *prop;
> +	int rc, len;
> +
> +	/* MMIO registers */
> +	dev_dbg(&op->dev, "probing mpc5200 SPI device\n");
> +	regs = of_iomap(op->node, 0);
> +	if (!regs)
> +		return -ENODEV;
> +
> +	/* initialize the device */
> +	writeb(SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR, regs+SPI_CTRL1);
> +	writeb(0x0, regs + SPI_CTRL2);
> +	writeb(0xe, regs + SPI_DATADIR);	/* Set output pins */
> +	writeb(0x8, regs + SPI_PORTDATA);	/* Deassert /SS signal */
> +
> +	/* Clear the status register and re-read it to check for a MODF
> +	 * failure.  This driver cannot currently handle multiple masters
> +	 * on the SPI bus.  This fault will also occur if the SPI signals
> +	 * are not connected to any pins (port_config setting) */
> +	readb(regs + SPI_STATUS);
> +	readb(regs + SPI_DATA);
> +	if (readb(regs + SPI_STATUS) & SPI_STATUS_MODF) {
> +		dev_err(&op->dev, "mode fault; is port_config correct?\n");
> +		return -EIO;
> +	}
> +
> +	dev_dbg(&op->dev, "allocating spi_master struct\n");
> +	master = spi_alloc_master(&op->dev, sizeof *ms);
> +	if (!master)
> +		return -ENOMEM;
> +	master->bus_num = -1;
> +	master->num_chipselect = 1;
> +	prop = of_get_property(op->node, "num-slaves", &len);
> +	if (prop && len >= sizeof(*prop))
> +		master->num_chipselect = *prop;

But you don't actually handle more than one chipselect, do you?
Either add full support for them, or rip this out and make sure
that mpc52xx_spi_setup only allows chipselect zero.

> +
> +	master->setup = mpc52xx_spi_setup;
> +	master->transfer = mpc52xx_spi_transfer;
> +	dev_set_drvdata(&op->dev, master);
> +
> +	ms = spi_master_get_devdata(master);
> +	ms->master = master;
> +	ms->regs = regs;
> +	ms->irq0 = irq_of_parse_and_map(op->node, 0);
> +	ms->irq1 = irq_of_parse_and_map(op->node, 1);
> +	ms->state = mpc52xx_spi_fsmstate_idle;
> +	ms->ipb_freq = mpc52xx_find_ipb_freq(op->node);
> +	spin_lock_init(&ms->lock);
> +	INIT_LIST_HEAD(&ms->queue);
> +	INIT_WORK(&ms->work, mpc52xx_spi_wq);
> +
> +	dev_dbg(&op->dev, "registering spi_master struct\n");
> +	rc = spi_register_master(master);
> +	if (rc < 0)
> +		goto err_register;
> +
> +	/* Decide if interrupts can be used */
> +	if ((ms->irq0 != NO_IRQ) && (ms->irq1 != NO_IRQ)) {

I understand that NO_IRQ is supposed to vanish everywhere,
so these tests should become "if (ms->irq0 && ms->irq1)".

That said ... with two distinct interrupts, I'm thinking
there must be a better solution than having them do exactly
the same thing.

Plus, a more informative labeling policy would be passing
dev_name(&spi_master->dev) ...


> +		rc = request_irq(ms->irq0, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> +				  "mpc5200-spi-modf", ms);
> +		rc |= request_irq(ms->irq1, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> +				  "mpc5200-spi-spiF", ms);

I'm not a fan of that "rc |=" idiom, but I guess it works here.

As noted elsewhere:  IRQF_DISABLED will probably be needed.


> +		if (rc) {
> +			free_irq(ms->irq0, ms);
> +			free_irq(ms->irq1, ms);
> +			ms->irq0 = ms->irq1 = NO_IRQ;
> +			dev_info(&op->dev, "using polled mode\n");
> +		}
> +	} else {
> +		/* operate in polled mode */
> +		ms->irq0 = ms->irq1 = NO_IRQ;
> +		dev_info(&op->dev, "using polled mode\n");

irq0 = 0;
irq1 = 0;

> +	}
> +
> +	/* Create SysFS files */
> +	rc = device_create_file(&ms->master->dev, &dev_attr_msg_count);
> +	rc |= device_create_file(&ms->master->dev, &dev_attr_byte_count);
> +	rc |= device_create_file(&ms->master->dev, &dev_attr_wcol_count);
> +	rc |= device_create_file(&ms->master->dev, &dev_attr_wcol_ticks);
> +	rc |= device_create_file(&ms->master->dev, &dev_attr_modf_count);

The minimalist in me wonders if those device files should exist,
or at least be moved to debugfs.


> +	if (rc)
> +		dev_info(&ms->master->dev, "error creating sysfs files\n");

The practical person in me notes that this continues after an otherwise
correct setup, but then returns a failure code from probe().  Which is
clearly a bug ...


> +
> +	dev_info(&ms->master->dev, "registered MPC5200 SPI bus\n");
> +
> +	of_register_spi_devices(master, op->node);
> +
> +	return rc;
> +
> + err_register:
> +	dev_err(&ms->master->dev, "initialization failed\n");
> +	spi_master_put(master);
> +	return rc;
> +}
> +
> +static void __devexit mpc52xx_spi_of_remove(struct of_device *op)
> +{
> +	struct spi_master *master = dev_get_drvdata(&op->dev);
> +	struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> +
> +	device_remove_file(&ms->master->dev, &dev_attr_msg_count);
> +	device_remove_file(&ms->master->dev, &dev_attr_byte_count);
> +	device_remove_file(&ms->master->dev, &dev_attr_wcol_count);
> +	device_remove_file(&ms->master->dev, &dev_attr_wcol_ticks);
> +	device_remove_file(&ms->master->dev, &dev_attr_modf_count);
> +
> +	free_irq(ms->irq0, ms);
> +	free_irq(ms->irq1, ms);
> +
> +	spi_unregister_master(master);
> +	spi_master_put(master);
> +	iounmap(ms->regs);
> +}
> +
> +static struct of_device_id mpc52xx_spi_of_match[] __devinitdata = {
> +	{ .compatible = "fsl,mpc5200-spi", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mpc52xx_psc_spi_of_match);
> +
> +static struct of_platform_driver mpc52xx_spi_of_driver = {
> +	.owner = THIS_MODULE,
> +	.name = "mpc52xx-spi",
> +	.match_table = mpc52xx_spi_of_match,
> +	.probe = mpc52xx_spi_of_probe,
> +	.remove = __exit_p(mpc52xx_spi_of_remove),
> +};
> +
> +static int __init mpc52xx_spi_init(void)
> +{
> +	return of_register_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_init(mpc52xx_spi_init);
> +
> +static void __exit mpc52xx_spi_exit(void)
> +{
> +	of_unregister_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_exit(mpc52xx_spi_exit);
> +
> diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
> new file mode 100644
> index 0000000..d1004cf
> --- /dev/null
> +++ b/include/linux/spi/mpc52xx_spi.h
> @@ -0,0 +1,10 @@
> +
> +#ifndef INCLUDE_MPC5200_SPI_H
> +#define INCLUDE_MPC5200_SPI_H
> +
> +extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
> +					    void (*hook)(struct spi_message *m,
> +							 void *context),
> +					    void *hook_context);
> +
> +#endif
> 

Re: [PATCH v3 0/4 REPOST] OF infrastructure for SPI devices

[Jon Smirl]

On 7/25/08, Grant Likely <grant.likely@secretlab.ca> wrote:
> I don't know what to do with these patches.  I'd really like to see them
>  in .27, and now that akpm has cleared his queue, the prerequisite patch
>  has been merged so they are ready to go in.  However, even though there
>  has been favourable reception on the SPI and linuxppc lists for these
>  changes I don't have any acks from anybody yet.

I have compatible hardware but it has an MMC card socket hooked up. I
haven't figured out yet how to get this driver hooked to the MMC
subsystem and make the file system on the card work.  (I've been
working on ALSA).

I believe I need this:
http://lkml.org/lkml/2008/6/5/209

Then I need to get everything hooked together. Point me in the right
direction and I can help with testing.


>
>  David, can you please reply on if you are okay with these patches
>  getting merged?  If so, do you want me to merge them via my tree, or
>  do you want to pick them up?
>
>  Thanks,
>  g.
>
>  --
>  Grant Likely, B.Sc. P.Eng.
>  Secret Lab Technologies Ltd.
>
> --
>  To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>  the body of a message to majordomo@vger.kernel.org
>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>  Please read the FAQ at  http://www.tux.org/lkml/
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH v3 0/4 REPOST] OF infrastructure for SPI devices

[Jon Smirl]

On 7/25/08, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 7/25/08, Grant Likely <grant.likely@secretlab.ca> wrote:
>  > I don't know what to do with these patches.  I'd really like to see them
>  >  in .27, and now that akpm has cleared his queue, the prerequisite patch
>  >  has been merged so they are ready to go in.  However, even though there
>  >  has been favourable reception on the SPI and linuxppc lists for these
>  >  changes I don't have any acks from anybody yet.
>
>
> I have compatible hardware but it has an MMC card socket hooked up. I
>  haven't figured out yet how to get this driver hooked to the MMC
>  subsystem and make the file system on the card work.  (I've been
>  working on ALSA).
>
>  I believe I need this:
>  http://lkml.org/lkml/2008/6/5/209
>
>  Then I need to get everything hooked together. Point me in the right
>  direction and I can help with testing.

Grant, your SPI driver works with MMC cards. I needed to add the patch
noted above.
http://lkml.org/lkml/2008/6/5/209
What's the status of merging Anton's patch?

And then tweak names until everything matches.

		spi@f00 {
			#address-cells = <1>;
			#size-cells = <0>;
			compatible = "fsl,mpc5200b-spi","fsl,mpc5200-spi";
			reg = <0xf00 0x20>;
			interrupts = <0x2 0xd 0x0 0x2 0xe 0x0>;
			interrupt-parent = <&mpc5200_pic>;

			mmc-slot@0 {
				compatible = "linux,mmc-spi";
				reg = <0>;
				spi-max-frequency = <20000000>;
				/* Unregulated slot. */
				voltage-range = <3300 3300>;
				/*gpios = <&sdcsr_pio 1 0   /*  WP */
				/*		 &sdcsr_pio 0 1>; /* nCD */
			};
		};

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 7503b81..eb056b9 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1400,7 +1400,7 @@ static int __devexit mmc_spi_remove(struct
spi_device *spi)

 static struct spi_driver mmc_spi_driver = {
 	.driver = {
-		.name =		"mmc_spi",
+		.name =		"mmc-spi",
 		.bus =		&spi_bus_type,
 		.owner =	THIS_MODULE,
 	},
diff --git a/drivers/of/of_mmc_spi.c b/drivers/of/of_mmc_spi.c
index aa4e017..f189cdb 100644
--- a/drivers/of/of_mmc_spi.c
+++ b/drivers/of/of_mmc_spi.c
@@ -95,7 +95,7 @@ static int of_mmc_spi_add(struct device *dev)
 	const u32 *voltage_range;
 	int size;

-	if (!np || !of_device_is_compatible(np, "mmc-spi"))
+	if (!np || !of_device_is_compatible(np, "linux,mmc-spi"))
 		return NOTIFY_DONE;

 	oms = kzalloc(sizeof(*oms), GFP_KERNEL);
@@ -152,7 +152,7 @@ static int of_mmc_spi_del(struct device *dev)
 	struct device_node *np = dev->archdata.of_node;
 	struct of_mmc_spi *oms;

-	if (!np || !of_device_is_compatible(np, "mmc-spi") ||
+	if (!np || !of_device_is_compatible(np, "linux,mmc-spi") ||
 			!dev->platform_data)
 		return NOTIFY_DONE;

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH v3 0/4 REPOST] OF infrastructure for SPI devices

[David Brownell]

On Friday 25 July 2008, Grant Likely wrote:
> I don't know what to do with these patches.  I'd really like to see them
> in .27, and now that akpm has cleared his queue, the prerequisite patch
> has been merged so they are ready to go in.  However, even though there
> has been favourable reception on the SPI and linuxppc lists for these
> changes I don't have any acks from anybody yet.
> 
> David, can you please reply on if you are okay with these patches
> getting merged?  If so, do you want me to merge them via my tree, or
> do you want to pick them up?

OK ... to recap, #1 and #3 are OF-specific, I'll be agnostic.
If other OF folk think it's OK, great.

Some cleanup was needed for #2, but it was basically ok ...
I'd like to see the final version, and if it goes via an
OF push that's OK by me.  (Though I'd like to see that change
make it into 2.6.27 regardless, so let me know.)

And #4 isn't quite cooked yet.

Re: [PATCH v3 0/4 REPOST] OF infrastructure for SPI devices

[Grant Likely]

On Sun, Jul 27, 2008 at 02:49:28PM -0700, David Brownell wrote:
> On Friday 25 July 2008, Grant Likely wrote:
> > I don't know what to do with these patches.  I'd really like to see them
> > in .27, and now that akpm has cleared his queue, the prerequisite patch
> > has been merged so they are ready to go in.  However, even though there
> > has been favourable reception on the SPI and linuxppc lists for these
> > changes I don't have any acks from anybody yet.
> > 
> > David, can you please reply on if you are okay with these patches
> > getting merged?  If so, do you want me to merge them via my tree, or
> > do you want to pick them up?
> 
> OK ... to recap, #1 and #3 are OF-specific, I'll be agnostic.
> If other OF folk think it's OK, great.
> 
> Some cleanup was needed for #2, but it was basically ok ...
> I'd like to see the final version, and if it goes via an
> OF push that's OK by me.  (Though I'd like to see that change
> make it into 2.6.27 regardless, so let me know.)

Thanks for the review.  1-3 is already merged into my git tree and Ben
has pulled them.  I hope that's okay.  Here's a link to the commit:

http://git.kernel.org/?p=linux/kernel/git/benh/powerpc.git;a=commitdiff;h=dc87c98e8f635a718f1abb2c3e15fc77c0001651

Let me know if you see anything else that needs to be changed and I'll
submit a fixup patch.

> And #4 isn't quite cooked yet.

Okay, I'll hack on this more and post v4.

Thanks again,
g.