From patchwork Fri Apr 1 18:26:08 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Samuel Ortiz X-Patchwork-Id: 6266 Return-path: Envelope-to: mchehab@pedra Delivery-date: Fri, 01 Apr 2011 15:28:29 -0300 Received: from mchehab by pedra with local (Exim 4.72) (envelope-from ) id 1Q5j5B-0001nj-G7 for mchehab@pedra; Fri, 01 Apr 2011 15:28:29 -0300 Received: from casper.infradead.org [85.118.1.10] by pedra with IMAP (fetchmail-6.3.17) for (single-drop); Fri, 01 Apr 2011 15:28:29 -0300 (BRT) Received: from vger.kernel.org ([209.132.180.67]) by casper.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id 1Q5j3k-0008Ab-OM; Fri, 01 Apr 2011 18:27:01 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754711Ab1DAS0V (ORCPT + 1 other); Fri, 1 Apr 2011 14:26:21 -0400 Received: from mga11.intel.com ([192.55.52.93]:5662 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753605Ab1DAS0U (ORCPT ); Fri, 1 Apr 2011 14:26:20 -0400 Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga102.fm.intel.com with ESMTP; 01 Apr 2011 11:26:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.63,283,1299484800"; d="scan'208";a="904596570" Received: from unknown (HELO sortiz-mobl) ([10.255.16.99]) by fmsmga001.fm.intel.com with ESMTP; 01 Apr 2011 11:26:09 -0700 Date: Fri, 1 Apr 2011 20:26:08 +0200 From: Samuel Ortiz To: Andres Salomon Cc: Grant Likely , linux-kernel@vger.kernel.org, Mark Brown , khali@linux-fr.org, ben-linux@fluff.org, Peter Korsgaard , Mauro Carvalho Chehab , David Brownell , linux-i2c@vger.kernel.org, linux-media@vger.kernel.org, netdev@vger.kernel.org, spi-devel-general@lists.sourceforge.net, Mocean Laboratories Subject: Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers Message-ID: <20110401182607.GC29397@sortiz-mobl> References: <20110202195417.228e2656@queued.net> <20110202200812.3d8d6cba@queued.net> <20110331230522.GI437@ponder.secretlab.ca> <20110401112030.GA3447@sortiz-mobl> <20110401104756.2f5c6f7a@debxo> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20110401104756.2f5c6f7a@debxo> User-Agent: Mutt/1.5.20 (2009-06-14) Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Sender: On Fri, Apr 01, 2011 at 10:47:56AM -0700, Andres Salomon wrote: > On Fri, 1 Apr 2011 13:20:31 +0200 > Samuel Ortiz wrote: > > > Hi Grant, > > > > On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote: > [...] > > > Gah. Not all devices instantiated via mfd will be an mfd device, > > > which means that the driver may very well expect an *entirely > > > different* platform_device pointer; which further means a very high > > > potential of incorrectly dereferenced structures (as evidenced by a > > > patch series that is not bisectable). For instance, the xilinx ip > > > cores are used by more than just mfd. > > I agree. Since the vast majority of the MFD subdevices are MFD > > specific IPs, I overlooked that part. The impacted drivers are the > > timberdale and the DaVinci voice codec ones. > > Can you please provide pointers to what you're referring to? The only > code that I could find that created platform devices prefixed with > 'timb-' or named 'xilinx_spi' was drivers/mfd/timberdale.c. The xilinx-spi, ocores-i2c, i2c-xiic drivers and to some extend the ks8842 ethernet driver are generic IPs that the timberdale SOC happens to use. So I agree it's extremely unlikely that anyone could come up with a platform that would be re-using e.g. the timb-radio IP, but I think it's less unikely for more generic IPs such as the xilinx-spi one. > > To fix that problem I propose 2 alternatives: > > > > 1) When declaring the sub devices cells, the MFD driver should > > specify an mfd_data_size value for sub devices that are not MFD > > specific. It's the MFD driver responsibility to set the cell > > properly, and the non MFD specific drivers are kept MFD agnostic. > > See my patch below for the timberdale case. > > > > 2) Revert the mfd_get_data() call for getting sub devices platform > > data pointers. That was introduced to ease the MFD cell sharing work, > > so if we take this route we'll need the cs5535 MFD driver to pass its > > cells as platform_data pointer. Andres, can you confirm that this > > would be fine for the mfd_clone_cell() routine to keep working ? > > It would break mfd_clone_cell, as it uses mfd_get_cell to grab the one > to clone. We could change it to accept the cell as an argument. It > would also break mfd_cell_enable/disable, of course. I'm talking about reverting the default behaviour of passing the MFD cell as the platform data, and going back to the cell definitions setting their platform_data pointer explicitely. In that case, the cs5535 driver would have to do something like: .disable = cs5535_mfd_res_disable, mfd_get_cell would then return &cs5535_mfd_cells[ACPI_BAR]. This fix would put all sub devices drivers back to an MFD agnostic state, although the vast majority of them will certainly never be found anywhere else than in their current MFD SoC. That's why I'm still not sure which way to go to fix that problem. Cheers, Samuel. diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c index 155fa04..3e3841d 100644 --- a/drivers/mfd/cs5535-mfd.c +++ b/drivers/mfd/cs5535-mfd.c @@ -106,6 +106,7 @@ static __devinitdata struct mfd_cell cs5535_mfd_cells[] = { .name = "cs5535-acpi", .num_resources = 1, .resources = &cs5535_mfd_resources[ACPI_BAR], + .platform_data = &cs5535_mfd_cells[ACPI_BAR], .enable = cs5535_mfd_res_enable,