Return-path: Received: from mail-yw0-f46.google.com ([209.85.213.46]:42102 "EHLO mail-yw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932141Ab0HDMom convert rfc822-to-8bit (ORCPT ); Wed, 4 Aug 2010 08:44:42 -0400 MIME-Version: 1.0 In-Reply-To: <20100804114147.GB6852@n2100.arm.linux.org.uk> References: <1279733634-21974-1-git-send-email-ohad@wizery.com> <1279733634-21974-4-git-send-email-ohad@wizery.com> <20100804114147.GB6852@n2100.arm.linux.org.uk> From: Ohad Ben-Cohen Date: Wed, 4 Aug 2010 15:42:32 +0300 Message-ID: Subject: Re: [PATCH v2 03/20] mmc: support embedded data field in mmc_host To: Russell King - ARM Linux Cc: Vitaly Wool , Kalle Valo , Pandita Vikram , Nicolas Pitre , Tony Lindgren , linux-wireless@vger.kernel.org, Roger Quadros , linux-mmc@vger.kernel.org, San Mehat , Chikkature Rajashekar Madhusudhan , Luciano Coelho , linux-omap@vger.kernel.org, akpm@linux-foundation.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-wireless-owner@vger.kernel.org List-ID: On Wed, Aug 4, 2010 at 2:41 PM, Russell King - ARM Linux 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 wrote: >> > On Mon, Aug 2, 2010 at 11:35 PM, Ohad Ben-Cohen 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 > #include > > 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. >