On 16/02/11 08:42, Grant Likely wrote:
> On Tue, Feb 15, 2011 at 03:04:49PM +0800, Thomas Chou wrote:
>> Dear Ryan,
>>
>> On 02/14/2011 10:20 AM, Ryan Mallon wrote:
>>> On 02/14/2011 03:10 PM, Thomas Chou wrote:
>>>> This patch adds a new SPI driver to support the Altera SOPC Builder
>>>> SPI component. It uses the bitbanging library.
>>>>
>>>> Signed-off-by: Thomas Chou<[email protected]>
>>>> ---
>>>> +struct altera_spi {
>>>> + /* bitbang has to be first */
>>>> + struct spi_bitbang bitbang;
>>> Is this still true? I had a quick look and can't see anything which
>>> relies on spi_bitbang being the first entry. Things like this should be
>>> using container_of so that position in the struct is irrelevant.
>>>
>>> ~Ryan
>>>
>> Yes, sadly true. This is due to the implementation of the bitbanging
>> library, spi_bitbang.c, which assumes the struct spi_bitbang is the
>> first of drvdata. Though it could be changed in the future (beyond
>> this little driver), every bitbanging library user has to follow
>> this for now.
> Should be easy to fix if it is indeed still true (I haven't dug deep
> enough to find the design error yet). Anybody want to volunteer?
>
> g.
>
The problem is that spi_master_get_devdata is used to get both
struct spi_bitbang and the controller dependent structure, which means
that struct spi_bitbang must be the first entry in the container
structure.
The following incomplete, untested patch shows a possible way to fix
this by introducing spi_alloc_master_bitbang. spi_master_get_devdata
now returns a pointer to struct spi_bitbang only. Drivers which need
an additional container struct should allocate it themselves and
pass the bitbang field to spi_alloc_master_bitbang. The driver
specific container can be fetched with:
static inline struct my_spi *to_my_spi(struct spi_master *master)
{
struct spi_bitbang *bitbang = spi_master_get_devdata(master);
return container_of(bitbang, struct my_spi, bitbang);
}
This patch only adds the alloc function and shows the necessary
changes to the spi_gpio.c driver, which is actually pretty minimal.
If there is any interest I can code up a proper patch.
~Ryan
---
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 34bb17f..52ec691 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -27,6 +27,7 @@
#include <linux/slab.h>
#include <linux/mod_devicetable.h>
#include <linux/spi/spi.h>
+#include <linux/spi/spi_bitbang.h>
#include <linux/of_spi.h>
#include <linux/pm_runtime.h>
@@ -560,6 +561,24 @@ struct spi_master *spi_alloc_master(struct device *dev,
unsigned size)
}
EXPORT_SYMBOL_GPL(spi_alloc_master);
+struct spi_master *spi_alloc_master_bitbang(struct device *dev,
+ struct spi_bitbang *bitbang)
+{
+ struct spi_master *master;
+
+ master = kzalloc(sizeof(struct spi_master), GFP_KERNEL);
+ if (!master)
+ return NULL;
+
+ device_initialize(&master->dev);
+ master->dev.class = &spi_master_class;
+ master->dev.parent = get_device(dev);
+ spi_master_set_devdata(master, bitbang);
+
+ return master;
+}
+EXPORT_SYMBOL_GPL(spi_alloc_master_bitbang);
+
/**
* spi_register_master - register SPI master controller
* @master: initialized master, originally from spi_alloc_master()
diff --git a/drivers/spi/spi_gpio.c b/drivers/spi/spi_gpio.c
index 63e51b0..169dbf0 100644
--- a/drivers/spi/spi_gpio.c
+++ b/drivers/spi/spi_gpio.c
@@ -329,12 +329,17 @@ static int __init spi_gpio_probe(struct platform_device
*pdev)
if (status < 0)
return status;
- master = spi_alloc_master(&pdev->dev, sizeof *spi_gpio);
- if (!master) {
+ spi_gpio = kzalloc(sizeof(struct spi_gpio), GFP_KERNEL);
+ if (!spi_gpio) {
status = -ENOMEM;
goto gpio_free;
}
- spi_gpio = spi_master_get_devdata(master);
+
+ master = spi_alloc_master_bitbang(&pdev->dev, &spi_gpio->bitbang);
+ if (!master) {
+ status = -ENOMEM;
+ goto spi_gpio_free;
+ }
platform_set_drvdata(pdev, spi_gpio);
spi_gpio->pdev = pdev;
@@ -367,6 +372,8 @@ static int __init spi_gpio_probe(struct platform_device
*pdev)
status = spi_bitbang_start(&spi_gpio->bitbang);
if (status < 0) {
spi_master_put(spi_gpio->bitbang.master);
+spi_gpio_free:
+ kfree(spi_gpio);
gpio_free:
if (SPI_MISO_GPIO != SPI_GPIO_NO_MISO)
gpio_free(SPI_MISO_GPIO);
@@ -391,6 +398,7 @@ static int __exit spi_gpio_remove(struct platform_device
*pdev)
/* stop() unregisters child devices too */
status = spi_bitbang_stop(&spi_gpio->bitbang);
spi_master_put(spi_gpio->bitbang.master);
+ kfree(spi_gpio);
platform_set_drvdata(pdev, NULL);
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index b4d7710..3f72a34 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -336,7 +336,8 @@ static inline void spi_master_put(struct spi_master *master)
/* the spi driver core manages memory for the spi_master classdev */
extern struct spi_master *
spi_alloc_master(struct device *host, unsigned size);
-
+extern struct spi_master *
+spi_alloc_master_bitbang(struct device *host, struct spi_bitbang *bitbang);
extern int spi_register_master(struct spi_master *master);
extern void spi_unregister_master(struct spi_master *master);
------------------------------------------------------------------------------
The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE:
Pinpoint memory and threading errors before they happen.
Find and fix more than 250 security defects in the development cycle.
Locate bottlenecks in serial and parallel code that limit performance.
http://p.sf.net/sfu/intel-dev2devfeb
_______________________________________________
spi-devel-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
