LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Felipe Balbi <me@felipebalbi.com>
To: Pierre Ossman <drzeus-list@drzeus.cx>
Cc: Carlos Aguiar <carlos.aguiar@indt.org.br>,
	Tony Lindgren <tony@atomide.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 05/18] MMC: OMAP: Introduce new multislot structure and change driver to use it
Date: Wed, 5 Mar 2008 22:54:09 +0200	[thread overview]
Message-ID: <20080305205408.GA16229@kedavra.cpe.vivax.com.br> (raw)
In-Reply-To: <20080207183744.1491c995@poseidon.drzeus.cx>

On Thu, Feb 07, 2008 at 06:37:44PM +0100, Pierre Ossman wrote:
> On Mon, 28 Jan 2008 15:07:23 -0400
> Carlos Aguiar <carlos.aguiar@indt.org.br> wrote:
> 
> > From: Juha Yrjola <juha.yrjola@solidboot.com>
> > 
> > Introduce new MMC multislot structure and change driver to use it.
> > 
> > Note that MMC clocking is now enabled in mmc_omap_select_slot()
> > and disabled in mmc_omap_release_slot().
> > 
> > Signed-off-by: Juha Yrjola <juha.yrjola@solidboot.com>
> > Signed-off-by: Jarkko Lavinen <jarkko.lavinen@nokia.com>
> > Signed-off-by: Carlos Eduardo Aguiar <carlos.aguiar@indt.org.br>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> 
> I still think this muxed mmc host thing is a bad idea, but it's your nightmare...

Not nightmare. OMAP1/2 have mmc/sd/sdio controller supporting one or
more mmc cards sharing the same bus.

> 
> > +
> > +/* Access to the R/O switch is required for production testing
> > + * purposes. */
> > +static ssize_t
> > +mmc_omap_show_ro(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct mmc_host *mmc = container_of(dev, struct mmc_host, class_dev);
> > +	struct mmc_omap_slot *slot = mmc_priv(mmc);
> > +
> > +	return sprintf(buf, "%d\n", slot->pdata->get_ro(mmc_dev(mmc),
> > +							slot->id));
> > +}
> > +
> > +static DEVICE_ATTR(ro, S_IRUGO, mmc_omap_show_ro, NULL);
> > +
> 
> This is unrelated to the slot stuff and should be in its own patch. Also, it should probably be in the core, not a driver.
> 
> > +
> > +	mmc->caps = MMC_CAP_MULTIWRITE | MMC_CAP_MMC_HIGHSPEED |
> > +		    MMC_CAP_SD_HIGHSPEED;
> 
> This is also unrelated. From what I've seen, the OMAP is a SD controller and does not support high speed MMC. The fact that you also conditionally set the max frequency later also suggests that this code is entirely incorrect.
> 

Not true. I can't show you not even parts of the technical documentation
but OMAP has mmc/sd/sdio controller. On OMAP1/2 it handles several mmc
cards sharing the same bus.

And that's what mmc multislot is doing. Probably it still needs some
tweks though. But that's up to Carlos.

> > +
> > +	r = mmc_add_host(mmc);
> > +	if (r < 0)
> > +		return r;
> > +
> > +	if (slot->pdata->name != NULL) {
> > +		r = device_create_file(&mmc->class_dev,
> > +					&dev_attr_slot_name);
> > +		if (r < 0)
> > +			goto err_remove_host;
> > +	}
> > +
> > +	if (slot->pdata->get_ro != NULL) {
> > +		r = device_create_file(&mmc->class_dev,
> > +					&dev_attr_ro);
> > +	}
> > +
> 
> You have a bit of a race here with userspace in case you use the uevent to trigger things.
> 
> -- 
>      -- Pierre Ossman
> 
>   Linux kernel, MMC maintainer        http://www.kernel.org
>   PulseAudio, core developer          http://pulseaudio.org
>   rdesktop, core developer          http://www.rdesktop.org
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Best Regards,

Felipe Balbi
me@felipebalbi.com
http://blog.felipebalbi.com

  reply	other threads:[~2008-03-05 20:52 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-01-28 19:07 Carlos Aguiar
2008-01-29  1:17 ` Roel Kluin
2008-02-07 17:37 ` Pierre Ossman
2008-03-05 20:54   ` Felipe Balbi [this message]
2008-03-06  6:34     ` Pierre Ossman
2008-03-14 19:35 [PATCH 00/18] MMC: OMAP: Sync MMC OMAP driver with mainline tree Carlos Aguiar
2008-03-24 12:26 ` Pierre Ossman
2008-03-26 20:08   ` [PATCH 05/18] MMC: OMAP: Introduce new multislot structure and change driver to use it Carlos Aguiar
2008-03-14 19:36 Carlos Aguiar
2008-03-24 12:13 ` Pierre Ossman

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=20080305205408.GA16229@kedavra.cpe.vivax.com.br \
    --to=me@felipebalbi.com \
    --cc=carlos.aguiar@indt.org.br \
    --cc=drzeus-list@drzeus.cx \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tony@atomide.com \
    --subject='Re: [PATCH 05/18] MMC: OMAP: Introduce new multislot structure and change driver to use it' \
    /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).