[PATCH v2 03/20] mmc: support embedded data field in mmc_host

[Ohad Ben-Cohen]

Hi Vitaly,

On Mon, Aug 2, 2010 at 7:25 PM, Vitaly Wool <vitalywool at gmail.com> wrote:
> On Mon, Aug 2, 2010 at 5:54 PM, Ohad Ben-Cohen <ohad at 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
>