LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Samuel Ortiz <sameo@linux.intel.com>
To: Andres Salomon <dilinger@queued.net>
Cc: Grant Likely <grant.likely@secretlab.ca>,
	linux-kernel@vger.kernel.org,
	Mark Brown <broonie@opensource.wolfsonmicro.com>,
	khali@linux-fr.org, ben-linux@fluff.org,
	Peter Korsgaard <jacmet@sunsite.dk>,
	Mauro Carvalho Chehab <mchehab@infradead.org>,
	David Brownell <dbrownell@users.sourceforge.net>,
	linux-i2c@vger.kernel.org, linux-media@vger.kernel.org,
	netdev@vger.kernel.org, spi-devel-general@lists.sourceforge.net,
	Mocean Laboratories <info@mocean-labs.com>
Subject: Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers
Date: Fri, 1 Apr 2011 20:26:08 +0200	[thread overview]
Message-ID: <20110401182607.GC29397@sortiz-mobl> (raw)
In-Reply-To: <20110401104756.2f5c6f7a@debxo>

On Fri, Apr 01, 2011 at 10:47:56AM -0700, Andres Salomon wrote:
> On Fri, 1 Apr 2011 13:20:31 +0200
> Samuel Ortiz <sameo@linux.intel.com> 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:

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,
                .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.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

  parent reply	other threads:[~2011-04-01 18:26 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-02-03  3:54 [RFC] [PATCH 0/19] mfd sharing support Andres Salomon
2011-02-03  3:55 ` [PATCH 01/19] mfd-core: unconditionally add mfd_cell to every platform_device Andres Salomon
2011-02-03  3:58 ` [PATCH 02/19] jz4740: mfd_cell is now implicitly available to drivers Andres Salomon
2011-02-03  8:09   ` Jean Delvare
2011-02-03  4:01 ` [PATCH 03/19] ab3550: " Andres Salomon
2011-02-04  8:20   ` Mattias Wallin
2011-02-03  4:03 ` [PATCH 04/19] ab3100: " Andres Salomon
2011-02-03 12:52   ` Linus Walleij
2011-02-03  4:04 ` [PATCH 05/19] asic3: " Andres Salomon
2011-02-03  4:05 ` [PATCH 06/19] htc-pasic3: " Andres Salomon
2011-02-03  4:08 ` [PATCH 07/19] timberdale: " Andres Salomon
2011-03-31 23:05   ` Grant Likely
2011-04-01 11:20     ` Samuel Ortiz
2011-04-01 17:47       ` Andres Salomon
2011-04-01 17:56         ` Grant Likely
2011-04-01 18:00           ` Grant Likely
2011-04-01 23:52           ` Samuel Ortiz
2011-04-01 23:58             ` Grant Likely
2011-04-02  0:10               ` Andres Salomon
2011-04-04 10:03               ` Samuel Ortiz
2011-04-05  3:04                 ` Grant Likely
2011-04-06 15:23                   ` Samuel Ortiz
2011-04-06 15:58                     ` Greg KH
2011-04-06 17:05                       ` Samuel Ortiz
2011-04-06 17:16                         ` Ben Hutchings
2011-04-06 17:51                           ` Samuel Ortiz
2011-04-06 18:07                             ` Ben Hutchings
2011-04-06 17:56                         ` Greg KH
2011-04-06 18:25                           ` Andres Salomon
2011-04-06 18:38                             ` Greg KH
2011-04-07  8:04                               ` Grant Likely
2011-04-06 18:47                           ` Samuel Ortiz
2011-04-06 18:59                             ` Felipe Balbi
2011-04-06 22:09                               ` Greg KH
2011-04-07  8:09                                 ` Felipe Balbi
2011-04-07 13:40                               ` Samuel Ortiz
2011-04-07 14:35                                 ` Grant Likely
2011-04-07 15:03                                   ` Samuel Ortiz
2011-04-07 18:06                                     ` Grant Likely
2011-04-07 16:24                   ` Grant Likely
2011-04-01 18:26         ` Samuel Ortiz [this message]
2011-02-03  4:09 ` [PATCH 08/19] t7166xb: " Andres Salomon
2011-02-03  4:11 ` [PATCH 09/19] wl1273: " Andres Salomon
2011-02-03  4:12 ` [PATCH 10/19] sh_mobile_sdhi: " Andres Salomon
2011-02-03  4:13 ` [PATCH 11/19] tc6393xb: " Andres Salomon
2011-02-03  4:15 ` [PATCH 12/19] twl4030: " Andres Salomon
2011-02-03  6:05   ` Dmitry Torokhov
2011-02-03  6:39     ` Andres Salomon
2011-02-03  6:53       ` Dmitry Torokhov
2011-02-03  7:03         ` Andres Salomon
2011-02-03  9:31           ` Mark Brown
2011-02-05  2:39             ` Andres Salomon
2011-02-05  3:25               ` Andres Salomon
2011-02-03 12:23           ` Peter Ujfalusi
2011-02-04 10:41           ` Uwe Kleine-König
2011-02-03  4:16 ` [PATCH 13/19] tc6387xb: " Andres Salomon
2011-02-03  4:17 ` [PATCH 14/19] janz: " Andres Salomon
2011-02-03  4:20 ` [PATCH 15/19] mc13xxx: " Andres Salomon
2011-02-04  9:34   ` Uwe Kleine-König
2011-02-04 10:13     ` Uwe Kleine-König
2011-02-04 10:16     ` Andres Salomon
2011-02-03  4:21 ` [PATCH 16/19] mfd-core: drop platform_data/data_size from core mfd_cell struct Andres Salomon
2011-02-03  4:22 ` [PATCH 17/19] mfd-core: add refcounting support to mfd_cells Andres Salomon
2011-02-03  4:23 ` [PATCH 18/19] mfd-core: add platform_device sharing support for mfd Andres Salomon
2011-02-03  4:26 ` [PATCH 19/19] cs5535-mfd: add sharing for acpi/pms cells Andres Salomon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110401182607.GC29397@sortiz-mobl \
    --to=sameo@linux.intel.com \
    --cc=ben-linux@fluff.org \
    --cc=broonie@opensource.wolfsonmicro.com \
    --cc=dbrownell@users.sourceforge.net \
    --cc=dilinger@queued.net \
    --cc=grant.likely@secretlab.ca \
    --cc=info@mocean-labs.com \
    --cc=jacmet@sunsite.dk \
    --cc=khali@linux-fr.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@infradead.org \
    --cc=netdev@vger.kernel.org \
    --cc=spi-devel-general@lists.sourceforge.net \
    --subject='Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).