[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 [off-list ref] wrote:

On Mon, Aug 2, 2010 at 5:54 PM, Ohad Ben-Cohen [off-list ref] wrote:

quoted

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