Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
2010/8/4 Ohad Ben-Cohen o...@wizery.com: On Wed, Aug 4, 2010 at 2:41 PM, Russell King - ARM Linux Why not arrange for a small piece of code to be built into the kernel when this driver is selected as a module or built-in, which handles the passing of platform data to the driver? It's interesting. The only drawback I can see is that it has an inherent limitation of having only a single wl1271 device on board, ...which is exactly what the *_board_info scheme in the spi and i2c subsystems is designed to solve. This is an established design pattern in the kernel with two precedents, what makes it so hard to implement this idea? There are plenty of examples all over the place. What mainly matters to me is that the stuff can be separated cleanly and in a nicely structured manner into board-xxx.c files in the mach-xxx dirs, which is possible also with the simpler approach so whatever... Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
On Fri, Aug 6, 2010 at 10:07 AM, Linus Walleij
linus.ml.wall...@gmail.com wrote:
2010/8/4 Ohad Ben-Cohen o...@wizery.com:
On Wed, Aug 4, 2010 at 2:41 PM, Russell King - ARM Linux
Why not arrange for a small piece of code to be built into the kernel
when this driver is selected as a module or built-in, which handles
the passing of platform data to the driver?
It's interesting.
The only drawback I can see is that it has an inherent limitation of
having only a single wl1271 device on board,
...which is exactly what the *_board_info scheme in the spi
and i2c subsystems is designed to solve.
This is an established design pattern in the kernel with two
precedents, what makes it so hard to implement this idea?
There are plenty of examples all over the place.
How would a driver ask for the platform data of its specific device ?
Using DEVICE_ID and VENDOR_ID is wrong - those numbers do not identify
a specific device instance (think of a board with two wl1271 devices.
the only difference between them is the index of their mmc
controller).
The only sane way to uniquely identify a specific device instance is
to couple its platform data with the host controller the device is
hardwired to.
So if we want to have an external sdio_board_info table to work, it
would have to map a controller index to sdio_board_info objects.
Something in the lines of:
int sdio_get_platform_data(struct sdio_board_info *data, struct sdio_func *func)
{
if (platform_data[func-card-host-index]) {
memcpy(data, platform_data[func-card-host-index],
sizeof(*data));
return 0;
}
return -ENODEV;
}
typechecking is not preserved (the driver would have to cast
data-platform_data), and there is a possibility for the wrong driver
to pick up the data (at least as much as there was with the original
proposal).
AFAICS, the difference between SDIO and I2C/SPI in that respect, is
that SDIO devices are created dynamically when hardware is probed,
whereas I2C/SPI uses the *_board_info table to populate the device
tree. The I2C/SPI drivers then elegantly get the platform data in the
probe call. In our case, the device is created dynamically, and the
only way to couple it with the correct platform data is using the
knowledge that it is hardwired to a specific host controller.
So using an external repository of platform data objects seem to have
similar disadvantages like the original proposal, but with much more
code.
We have Russell's suggestion which is nice and simple, but it has the
1 device limitation.
Or, we can go back to the approach of creating another platform device
for that chip. That device's name should be wl1271.x where x is the
index of the controller the device is hardwired to. Later, when the
SDIO function probe is invoked, it would register the platform driver
(after dynamically sprintf()ing the name using the
func-card-host-index number), and then
wait_for_completion_interruptible_timeout() for it to probe.
That seem to settle all the concerns raised: we get typechecking, no
wrong driver getting the data, no 1 device limit, no races between the
two probes.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
On Fri, Aug 06, 2010 at 01:02:24PM +0300, Ohad Ben-Cohen wrote:
We have Russell's suggestion which is nice and simple, but it has the
1 device limitation.
You could make it generic by doing something like this:
#define set_device_data(name, type, index, data)\
({ \
extern void __set_device_data(const char *, int, void *, size_t); \
BUILD_BUG_ON(!__same_type(type, *data));\
__set_device_data(name : #type, index, data, sizeof(type)); \
})
#define get_device_data(name, type, index) ({ \
extern void *__get_device_data(const char *, int);\
type *__ptr = __get_device_data(name : #type, index); \
__ptr; })
And now we have something that takes a string and index to use as a lookup
key in some kind of list - and it's typesafe (because the lookup key is
dependent on the stringified type.)
But... at this point I feel that we're getting too complicated, and will
get shouted at to use something like DT which already solves this problem.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
On Fri, 6 Aug 2010, Russell King - ARM Linux wrote:
On Fri, Aug 06, 2010 at 01:02:24PM +0300, Ohad Ben-Cohen wrote:
We have Russell's suggestion which is nice and simple, but it has the
1 device limitation.
You could make it generic by doing something like this:
#define set_device_data(name, type, index, data) \
({ \
extern void __set_device_data(const char *, int, void *, size_t); \
BUILD_BUG_ON(!__same_type(type, *data)); \
__set_device_data(name : #type, index, data, sizeof(type)); \
})
#define get_device_data(name, type, index) ({ \
extern void *__get_device_data(const char *, int); \
type *__ptr = __get_device_data(name : #type, index); \
__ptr; })
And now we have something that takes a string and index to use as a lookup
key in some kind of list - and it's typesafe (because the lookup key is
dependent on the stringified type.)
But... at this point I feel that we're getting too complicated, and will
get shouted at to use something like DT which already solves this problem.
DT is not generally available yet. A simple interim solution would
still be worth it.
Nicolas
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
Hi Vitaly,
On Tue, Aug 3, 2010 at 5:17 PM, Vitaly Wool vitalyw...@gmail.com wrote:
On Mon, Aug 2, 2010 at 11:35 PM, Ohad Ben-Cohen o...@wizery.com wrote:
I'm honestly trying to understand your concerns, but I'm afraid that
just saying it's a hack is not too informative. Can you please
explain what do you think is technically wrong with the proposed
solution ? is it not general enough for other use cases ? will it
break something ?
So if I'd like to set the *same* structure for the *same* WL1271
driver, provided that I'm working with the other platform, I'll need
to do the following:
- add the pointer to the board-specific header;
- add the structure to the board-specific C file and propagate its
pointer to the controller driver;
- add setting the pointer in the core structure into the controller driver.
This is far from being intuitive. This means we need to hack,
generally speaking, all the MMC controller drivers to get it working
on all platforms. This is error prone.
You make it sound really complex.
Let's see what it means to add it to a totally different platform.
As an example, let's take Google's ADP1 which is based on a different
host controller (msm-sdcc), and add the required support (untested of
course, just a quick sketch, patch is based on android's msm git):
diff --git a/arch/arm/mach-msm/board-trout-mmc.c b/arch/arm/mach-msm/board-trout
index 13755f5..df32b2f 100644
--- a/arch/arm/mach-msm/board-trout-mmc.c
+++ b/arch/arm/mach-msm/board-trout-mmc.c
@@ -10,6 +10,7 @@
#include linux/mmc/sdio_ids.h
#include linux/err.h
#include linux/debugfs.h
+#include linux/wl12xx.h
#include asm/gpio.h
#include asm/io.h
@@ -297,11 +298,16 @@ int trout_wifi_reset(int on)
EXPORT_SYMBOL(trout_wifi_reset);
#endif
+struct wl12xx_platform_data trout_wlan_data = {
+ .irq = 62, /* put here your irq number */
+ .board_ref_clock = 1, /* put here your ref clock */
+};
+
static struct mmc_platform_data trout_wifi_data = {
.ocr_mask = MMC_VDD_28_29,
.status = trout_wifi_status,
.register_status_notify = trout_wifi_status_register,
- .embedded_sdio = trout_wifi_emb_data,
+ .embedded_sdio = trout_wlan_data,
};
int __init trout_init_mmc(unsigned int sys_rev)
diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c
index 1697d42..c40f0d1 100755
--- a/drivers/mmc/host/msm_sdcc.c
+++ b/drivers/mmc/host/msm_sdcc.c
@@ -1261,6 +1261,7 @@ msmsdcc_probe(struct platform_device *pdev)
mmc-f_min = msmsdcc_fmin;
mmc-f_max = msmsdcc_fmax;
mmc-ocr_avail = plat-ocr_mask;
+mmc_set_embedded_data(mmc, plat-embedded_sdio);
if (msmsdcc_4bit)
mmc-caps |= MMC_CAP_4_BIT_DATA;
Is it really that complex ?
Thanks,
Ohad.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
On Wed, Aug 04, 2010 at 02:24:39PM +0300, Ohad Ben-Cohen wrote:
Hi Vitaly,
On Tue, Aug 3, 2010 at 5:17 PM, Vitaly Wool vitalyw...@gmail.com wrote:
On Mon, Aug 2, 2010 at 11:35 PM, Ohad Ben-Cohen o...@wizery.com wrote:
I'm honestly trying to understand your concerns, but I'm afraid that
just saying it's a hack is not too informative. Can you please
explain what do you think is technically wrong with the proposed
solution ? is it not general enough for other use cases ? will it
break something ?
So if I'd like to set the *same* structure for the *same* WL1271
driver, provided that I'm working with the other platform, I'll need
to do the following:
- add the pointer to the board-specific header;
- add the structure to the board-specific C file and propagate its
pointer to the controller driver;
- add setting the pointer in the core structure into the controller driver.
This is far from being intuitive. This means we need to hack,
generally speaking, all the MMC controller drivers to get it working
on all platforms. This is error prone.
You make it sound really complex.
Let's see what it means to add it to a totally different platform.
As an example, let's take Google's ADP1 which is based on a different
host controller (msm-sdcc), and add the required support (untested of
course, just a quick sketch, patch is based on android's msm git):
What if some other driver gets attached and tries to use this
platform data? This whole idea sounds extremely silly, cumbersome,
error prone, and is inviting misuse by normal MMC sockets.
Why not arrange for a small piece of code to be built into the kernel
when this driver is selected as a module or built-in, which handles
the passing of platform data to the driver?
Something like:
wl12xx_platform_data.c:
#include linux/module.h
#include whatever/required/for/wl12xx.h
static struct wl12xx_platform_data *platform_data;
int __init wl12xx_set_platform_data(const struct wl12xx_platform_data *data)
{
if (platform_data)
return -EBUSY;
platform_data = kmemdup(data, sizeof(*data), GFP_KERNEL);
if (!platform_data)
return -ENOMEM;
return 0;
}
int wl12xx_get_platform_data(struct wl12xx_platform_data *data)
{
if (platform_data) {
memcpy(data, platform_data, sizeof(*data));
return 0;
}
return -ENODEV;
}
EXPORT_SYMBOL(wl12xx_get_platform_data);
And in the Kconfig, something like:
config WL12XX_PLATFORM_DATA
bool
depends on WL12XX != n
default y
This means there'll be no confusion over who owns the 'embedded data',
typechecking is preserved, and no possibility for the wrong driver to
pick up the data.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
On Wed, Aug 4, 2010 at 2:41 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Wed, Aug 04, 2010 at 02:24:39PM +0300, Ohad Ben-Cohen wrote:
On Tue, Aug 3, 2010 at 5:17 PM, Vitaly Wool vitalyw...@gmail.com wrote:
On Mon, Aug 2, 2010 at 11:35 PM, Ohad Ben-Cohen o...@wizery.com wrote:
I'm honestly trying to understand your concerns, but I'm afraid that
just saying it's a hack is not too informative. Can you please
explain what do you think is technically wrong with the proposed
solution ? is it not general enough for other use cases ? will it
break something ?
So if I'd like to set the *same* structure for the *same* WL1271
driver, provided that I'm working with the other platform, I'll need
to do the following:
- add the pointer to the board-specific header;
- add the structure to the board-specific C file and propagate its
pointer to the controller driver;
- add setting the pointer in the core structure into the controller driver.
This is far from being intuitive. This means we need to hack,
generally speaking, all the MMC controller drivers to get it working
on all platforms. This is error prone.
You make it sound really complex.
Let's see what it means to add it to a totally different platform.
As an example, let's take Google's ADP1 which is based on a different
host controller (msm-sdcc), and add the required support (untested of
course, just a quick sketch, patch is based on android's msm git):
What if some other driver gets attached and tries to use this
platform data? This whole idea sounds extremely silly, cumbersome,
error prone, and is inviting misuse by normal MMC sockets.
Why not arrange for a small piece of code to be built into the kernel
when this driver is selected as a module or built-in, which handles
the passing of platform data to the driver?
It's interesting.
The only drawback I can see is that it has an inherent limitation of
having only a single wl1271 device on board, but there are no such
boards outside development/testing labs.
Vitaly would that work for you too ?
Thanks,
Ohad.
Something like:
wl12xx_platform_data.c:
#include linux/module.h
#include whatever/required/for/wl12xx.h
static struct wl12xx_platform_data *platform_data;
int __init wl12xx_set_platform_data(const struct wl12xx_platform_data *data)
{
if (platform_data)
return -EBUSY;
platform_data = kmemdup(data, sizeof(*data), GFP_KERNEL);
if (!platform_data)
return -ENOMEM;
return 0;
}
int wl12xx_get_platform_data(struct wl12xx_platform_data *data)
{
if (platform_data) {
memcpy(data, platform_data, sizeof(*data));
return 0;
}
return -ENODEV;
}
EXPORT_SYMBOL(wl12xx_get_platform_data);
And in the Kconfig, something like:
config WL12XX_PLATFORM_DATA
bool
depends on WL12XX != n
default y
This means there'll be no confusion over who owns the 'embedded data',
typechecking is preserved, and no possibility for the wrong driver to
pick up the data.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
On Wed, Aug 4, 2010 at 2:42 PM, Ohad Ben-Cohen o...@wizery.com wrote: On Wed, Aug 4, 2010 at 2:41 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Wed, Aug 04, 2010 at 02:24:39PM +0300, Ohad Ben-Cohen wrote: On Tue, Aug 3, 2010 at 5:17 PM, Vitaly Wool vitalyw...@gmail.com wrote: On Mon, Aug 2, 2010 at 11:35 PM, Ohad Ben-Cohen o...@wizery.com wrote: I'm honestly trying to understand your concerns, but I'm afraid that just saying it's a hack is not too informative. Can you please explain what do you think is technically wrong with the proposed solution ? is it not general enough for other use cases ? will it break something ? So if I'd like to set the *same* structure for the *same* WL1271 driver, provided that I'm working with the other platform, I'll need to do the following: - add the pointer to the board-specific header; - add the structure to the board-specific C file and propagate its pointer to the controller driver; - add setting the pointer in the core structure into the controller driver. This is far from being intuitive. This means we need to hack, generally speaking, all the MMC controller drivers to get it working on all platforms. This is error prone. You make it sound really complex. Let's see what it means to add it to a totally different platform. As an example, let's take Google's ADP1 which is based on a different host controller (msm-sdcc), and add the required support (untested of course, just a quick sketch, patch is based on android's msm git): What if some other driver gets attached and tries to use this platform data? This whole idea sounds extremely silly, cumbersome, error prone, and is inviting misuse by normal MMC sockets. Why not arrange for a small piece of code to be built into the kernel when this driver is selected as a module or built-in, which handles the passing of platform data to the driver? It's interesting. The only drawback I can see is that it has an inherent limitation of having only a single wl1271 device on board, but there are no such boards outside development/testing labs. Vitaly would that work for you too ? Works for me, but I've got a remark. If we try to generalize that idea to handle multiple devices it will be similar to the following: static struct wl12xx_platform_data platform_data[MAX_WL12XX_DEVICES]; int __init wl12xx_set_platform_data(const struct wl12xx_platform_data *data, int id); int wl12xx_get_platform_data(struct wl12xx_platform_data *data, int id); which will look pretty much like... yes, a simplified/customized version board_info applied to a single driver. But anyway I'm fine with that. ~Vitaly -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
Hi Ohad, On Mon, Aug 2, 2010 at 11:35 PM, Ohad Ben-Cohen o...@wizery.com wrote: I'm honestly trying to understand your concerns, but I'm afraid that just saying it's a hack is not too informative. Can you please explain what do you think is technically wrong with the proposed solution ? is it not general enough for other use cases ? will it break something ? let's summarize the solution you're proposing. You're trying to add embedded_data which will be scattered all over the place. The patches you introduce do the following: - add the pointer to the mmc core header; - add the primitive to set it from the controller driver; - add the pointer to the board-specific header; - add the structure to the board-specific C file and propagate its pointer to the controller driver; - add setting the pointer in the core structure into the controller driver. So if I'd like to set the *same* structure for the *same* WL1271 driver, provided that I'm working with the other platform, I'll need to do the following: - add the pointer to the board-specific header; - add the structure to the board-specific C file and propagate its pointer to the controller driver; - add setting the pointer in the core structure into the controller driver. This is far from being intuitive. This means we need to hack, generally speaking, all the MMC controller drivers to get it working on all platforms. This is error prone. And I'm not even speaking about the fact that MMC controller driver should not deal with such things at all. Thanks, Vitaly -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
Hi Vitaly, On Thu, Jul 29, 2010 at 7:16 PM, Vitaly Wool vitalyw...@gmail.com wrote: On Thu, Jul 29, 2010 at 8:00 AM, Ohad Ben-Cohen o...@wizery.com wrote: To my understanding, this data doesn't belong to mmc_host. It's not a host data at all. E. g. imagine a GPIO IRQ for some SDIO chip -- it's totally unrelated to host. I think a cleaner way would be to introduce something similar to what we have for SPI, e. g. struct sdio_board_info. This board info will contain platform-specific stuff and vendor id/chip id for each onboard SDIO device. Then the SDIO core will pick up the appropriate data basing on vendor id/chip id. Can you please elaborate some more about your proposal (specifically where does this sdio_board_info get set and how do function drivers access it) ? If I understand you correctly, you suggest to have a global, board-specific table of sdio_board_info structures, which would be accessible to the SDIO core (through the host driver ?). When a new SDIO device is found the core would search this table for the appropriate sdio_board_info struct and make it accessible to the SDIO function driver ? Well, let's look at how it's implemented for SPI. There is the function spi_register_board_info in the SPI core which copies the board info into the local data structure (a linked list, actually). Whenever needed, the core walks through the list to find the appropriate board_info basing on some search key. I think this may be the way to go for SDIO as well. IMHO this is a bit overkill solution for our problem. SPI is using these spi_board_info tables to populate the SPI device trees. These tables are registered early at the board-specific init code, and are later used by SPI core to populate the devices when the SPI master controller is registered. SDIO doesn't normally needs any kind of hard coded data: most devices are dynamically probed and populated. On rare cases like the wl1271, we have some platform-specific data we need to deliver the SDIO function driver (i.e. the irq info in this case). Since the device is hardwired to a specific controller, it does make some sense to pass this private data from the controller's info in the board files, through the host driver, and make it accessible through the specific host instance that drives this controller. Btw, if our problem was be broader (e.g., needs to supply private data for non-hardwired devices), then I agree that a more complex mechanism, such as the one you suggest, would be needed. But currently the problem is very simple and the solution is even simpler: just add 1 private pointer to the host. Hope you find this reasonable, Thanks, Ohad. ~Vitaly -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
Hi Ohad, On Mon, Aug 2, 2010 at 5:54 PM, Ohad Ben-Cohen o...@wizery.com wrote: SPI is using these spi_board_info tables to populate the SPI device trees. These tables are registered early at the board-specific init code, and are later used by SPI core to populate the devices when the SPI master controller is registered. SDIO doesn't normally needs any kind of hard coded data: most devices are dynamically probed and populated. On rare cases like the wl1271, we have some platform-specific data we need to deliver the SDIO function driver (i.e. the irq info in this case). Since the device is hardwired to a specific controller, it does make some sense to pass this private data from the controller's info in the board files, through the host driver, and make it accessible through the specific host instance that drives this controller. Btw, if our problem was be broader (e.g., needs to supply private data for non-hardwired devices), then I agree that a more complex mechanism, such as the one you suggest, would be needed. But currently the problem is very simple and the solution is even simpler: just add 1 private pointer to the host. Hope you find this reasonable, no, actually I don't. I think this is a hack that intrudes into the area it completely doesn't belong to. In fact, one can have 2 views on this problem: either this is a fairly generic problem we need to address, or this is a very specific corner case. Your solution will be treated as a hack in both cases. If we consider it a generic problem, then we need to find a generic solution, which is the board_info solution, for instance. FWIW, I2C also uses this approach now. If we consider it to be a corner case, let's just add a dummy platform_device like it's done in WL1251 implementation and keep the MMC subsystem clean. Thanks, Vitaly -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
Hi Vitaly, On Mon, Aug 2, 2010 at 7:25 PM, Vitaly Wool vitalyw...@gmail.com wrote: On Mon, Aug 2, 2010 at 5:54 PM, Ohad Ben-Cohen o...@wizery.com wrote: SPI is using these spi_board_info tables to populate the SPI device trees. These tables are registered early at the board-specific init code, and are later used by SPI core to populate the devices when the SPI master controller is registered. SDIO doesn't normally needs any kind of hard coded data: most devices are dynamically probed and populated. On rare cases like the wl1271, we have some platform-specific data we need to deliver the SDIO function driver (i.e. the irq info in this case). Since the device is hardwired to a specific controller, it does make some sense to pass this private data from the controller's info in the board files, through the host driver, and make it accessible through the specific host instance that drives this controller. Btw, if our problem was be broader (e.g., needs to supply private data for non-hardwired devices), then I agree that a more complex mechanism, such as the one you suggest, would be needed. But currently the problem is very simple and the solution is even simpler: just add 1 private pointer to the host. Hope you find this reasonable, no, actually I don't. I think this is a hack that intrudes into the area it completely doesn't belong to. In fact, one can have 2 views on this problem: either this is a fairly generic problem we need to address, or this is a very specific corner case. Your solution will be treated as a hack in both cases. If we consider it a generic problem, then we need to find a generic solution, which is the board_info solution, for instance. FWIW, I2C also uses this approach now. If we consider it to be a corner case, let's just add a dummy platform_device like it's done in WL1251 implementation and keep the MMC subsystem clean. Let's start by defining the problem exactly: SDIO devices that are hardwired to a specific controller and have some platform data that needs to be available to the SDIO function driver. It doesn't get more generic than that - it's not needed with common fully-enumerable SDIO devices (at least I'm not aware of such need), hence a generic mechanism of maintaining a global pile of platform data pointers, which can be queried with the device and vendor ID, really sounds like an overkill. But it's not specific to the wl1271 device - I prefer to support this at the MMC level, so we wouldn't have to add an extra platform device for every SDIO function driver that needs to cope with such issue (we already have two drivers - wl1271_sdio.c and wl1251_sdio.c); Adding an extra platform device for every driver is just too much dummy code (that btw adds a well-hidden race condition between the two probe calls), which can be nicely eliminated if we just introduce this new per-host private data pointer. So we have a SDIO device hardwired to a specific controller, that is driven by a specific host controller driver instance. This patch allows this specific host instance to carry platform data for the device that is hardwired to it. The platform data would be available only to SDIO function driver instances of that specific host controller. The solution is generic enough to support all SDIO function drivers with similar needs, and it's extremely simple. I'm honestly trying to understand your concerns, but I'm afraid that just saying it's a hack is not too informative. Can you please explain what do you think is technically wrong with the proposed solution ? is it not general enough for other use cases ? will it break something ? Thanks, Ohad. Thanks, Vitaly -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
Hi Vitaly,
On Wed, Jul 28, 2010 at 10:47 PM, Vitaly Wool vitalyw...@gmail.com wrote:
On Wed, Jul 21, 2010 at 7:33 PM, Ohad Ben-Cohen o...@wizery.com wrote:
Add support to set/get mmc_host private embedded
data.
This is needed to allow software to dynamically
create (and remove) SDIO functions which represents
embedded SDIO devices.
snip
@@ -209,6 +209,8 @@ struct mmc_host {
struct led_trigger *led; /* activity led */
#endif
+ void *embedded_data;
+
To my understanding, this data doesn't belong to mmc_host. It's not a
host data at all. E. g. imagine a GPIO IRQ for some SDIO chip -- it's
totally unrelated to host.
I think a cleaner way would be to introduce something similar to what
we have for SPI, e. g. struct sdio_board_info. This board info will
contain platform-specific stuff and vendor id/chip id for each onboard
SDIO device. Then the SDIO core will pick up the appropriate data
basing on vendor id/chip id.
Can you please elaborate some more about your proposal (specifically
where does this sdio_board_info get set and how do function drivers
access it) ?
If I understand you correctly, you suggest to have a global,
board-specific table of sdio_board_info structures, which would be
accessible to the SDIO core (through the host driver ?). When a new
SDIO device is found the core would search this table for the
appropriate sdio_board_info struct and make it accessible to the SDIO
function driver ?
Thanks,
Ohad.
~Vitaly
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
Hi Ohad, On Thu, Jul 29, 2010 at 8:00 AM, Ohad Ben-Cohen o...@wizery.com wrote: To my understanding, this data doesn't belong to mmc_host. It's not a host data at all. E. g. imagine a GPIO IRQ for some SDIO chip -- it's totally unrelated to host. I think a cleaner way would be to introduce something similar to what we have for SPI, e. g. struct sdio_board_info. This board info will contain platform-specific stuff and vendor id/chip id for each onboard SDIO device. Then the SDIO core will pick up the appropriate data basing on vendor id/chip id. Can you please elaborate some more about your proposal (specifically where does this sdio_board_info get set and how do function drivers access it) ? If I understand you correctly, you suggest to have a global, board-specific table of sdio_board_info structures, which would be accessible to the SDIO core (through the host driver ?). When a new SDIO device is found the core would search this table for the appropriate sdio_board_info struct and make it accessible to the SDIO function driver ? Well, let's look at how it's implemented for SPI. There is the function spi_register_board_info in the SPI core which copies the board info into the local data structure (a linked list, actually). Whenever needed, the core walks through the list to find the appropriate board_info basing on some search key. I think this may be the way to go for SDIO as well. ~Vitaly -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host
Hi Ohad,
On Wed, Jul 21, 2010 at 7:33 PM, Ohad Ben-Cohen o...@wizery.com wrote:
Add support to set/get mmc_host private embedded
data.
This is needed to allow software to dynamically
create (and remove) SDIO functions which represents
embedded SDIO devices.
snip
@@ -209,6 +209,8 @@ struct mmc_host {
struct led_trigger *led; /* activity led */
#endif
+ void *embedded_data;
+
To my understanding, this data doesn't belong to mmc_host. It's not a
host data at all. E. g. imagine a GPIO IRQ for some SDIO chip -- it's
totally unrelated to host.
I think a cleaner way would be to introduce something similar to what
we have for SPI, e. g. struct sdio_board_info. This board info will
contain platform-specific stuff and vendor id/chip id for each onboard
SDIO device. Then the SDIO core will pick up the appropriate data
basing on vendor id/chip id.
~Vitaly
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 03/20] mmc: support embedded data field in mmc_host
Add support to set/get mmc_host private embedded
data.
This is needed to allow software to dynamically
create (and remove) SDIO functions which represents
embedded SDIO devices.
Typically, it will be used to set the context of
a driver that is creating a new SDIO function
(and would then expect to be able to get that context
back upon creation of the new sdio func).
Signed-off-by: Ohad Ben-Cohen o...@wizery.com
---
include/linux/mmc/host.h | 12
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index f65913c..80db597 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -209,6 +209,8 @@ struct mmc_host {
struct led_trigger *led; /* activity led */
#endif
+ void*embedded_data;
+
struct dentry *debugfs_root;
unsigned long private[0] cacheline_aligned;
@@ -264,5 +266,15 @@ static inline void mmc_set_disable_delay(struct mmc_host
*host,
host-disable_delay = disable_delay;
}
+static inline void *mmc_get_embedded_data(struct mmc_host *host)
+{
+ return host-embedded_data;
+}
+
+static inline void mmc_set_embedded_data(struct mmc_host *host, void *data)
+{
+ host-embedded_data = data;
+}
+
#endif
--
1.7.0.4
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
