Re: [PATCH v2] Add GPIO-based MMC/SD driver

[David Brownell]

Hmm, I just now saw your reply ...

> On Monday 21 July 2008 22:41:35 David Brownell wrote:
> > So what this driver adds is mostly a userspace interface, yes?
> 
> Mostly. It also provides a simple platform device.

Yeah but I don't like interfaces like that ... which provide
less functional clones of existing interfaces.  If the goal is
convenience, then just wrap the existing ones together to give
the "one stop shopping" experience.

 
> > Please switch to the more conventional "MOSI" (Master Out, Slave In)
> > and "MISO" (Master In, Slave Out).  That's unambiguous.  If you want
> > to be a bit more clear you could say that MOSI goes to the MMC/SD
> > card pin 2 (card data in), and MISO goes to MMC/SD card pin 7 (card
> > data out).
> 
> Maybe.

Not clear what you mean.  You are using "DI" and "DO", which is quite
ambiguous (data in to card?  or data in to host?).  And you don't say
which direction is meant.  So that's ambigious ... just like "maybe".


> 
> > > > +	if (res < 8)
> > > > +		pdata->p.no_spi_delay = 0; /* Default: Delay turned on */
> > > > +	if (res < 7)
> > > > +		pdata->p.max_bus_speed = 5000000; /* Default: 5Mhz */
> > 
> 
> > So I'd default to leaving the delay off
> 
> How to?

Simplest to just rip out the support for using one!  And in any case:
as a rule, any delay should be a function of the target bus speed.

If you wanted to get fancy you could measure how long it takes to issue a
non-inlined GPIO call, and solve for the delay:

	cycle time = 1/max_speed_hz
	cycle_time = 4 * call_cost + 2 * delay_len

One way to measure would be wrapping ktime_get_ts() calls around a loop that
toggles the SPI clock a few hundred times.


> > > > +	if (res < 6)
> > > > +		pdata->p.mode = 0; /* Default: SPI mode 0 */
> > 
> > Don't need to do this.  The mmc_spi driver forces mode 0 in
> > any case...
> 
> Where's that documented?

UTSL ... mmc_spi_probe(), first thing.  :)


> > > > +config GPIOMMC
> > > > +	tristate "MMC/SD over GPIO-based SPI"
> > 
> > "Sysfs interface for ..."
> > 
> > Because we can already do MMC/SD over GPIO-SPI,
> > without using this code.
> 
> This also adds a platform device interface for convenience
> (as it also uses that internally).

Don't export that.  It's a (less expressive) subset of the standard
way to do that.


> > > > + * @no_spi_delay: Do not use delays in the lowlevel SPI bitbanging code.
> > > > + *                This is not standards compliant, but may be required for some
> > > > + *                embedded machines to gain reasonable speed.
> > 
> > Rather than emphasize "not standards compliant", I'd instead emphasize that
> > bitbanged SPI is so slow that you probably don't want to slow it down any
> > more by additional delays between per-bit operations!
> 
> I'd say you cannot tell how fast the GPIOs are. They can be pretty fast
> on a huge machine.

Huge machine tends to mean lots of busses to traverse before getting to
where the actual I/O gets performed.  Like a PCI posted write, followed
by a read to make sure the write completed and the I/O was performed.

Regardless, the point isn't standards, it's platform performance ... which
is, in all cases I've used bitbanged SPI, at most 1 MHz when non-inlined
GPIO calls are used.  And many times faster with inlined calls that go
directly to the GPIO controller...


> > Again, please don't promote a *SECOND* platform-device based mechanism
> 
> It is not a second mechanism, it is a mechanism on top of the first two
> mechanisms.

There's already a way to define an MMC-over-SPI device.  This is another
one.  In what way would that not be a "second" mechanism??


> > > > +Registering devices via platform-device
> > > > +=======================================
> > > > +
> > > > +The platform-device interface is used for registering MMC/SD devices that are
> > > > +part of the hardware platform. This is most useful only for embedded machines
> > > > +with MMC/SD devices statically connected to the platform GPIO bus.
> > 
> > There is no "platform GPIO bus" ... 
> 
> hm??

If you look in /sys/bus you won't see GPIO.  It's not a bus.


> > This is the interface I'd rather just not see exposed ...
> 
> People asked me to expose it. It was hidden in the first patch that
> I submitted. I'm not going to change this just to see the next one
> yelling "I want to see this interface in public" again.

Could you send me some URLs for that feedback?  Again, I'm not keen
on adding a second (and less functional) way to do this.


> 
> > > > +Registering devices via sysfs
> > > > +=============================
> > 
> > This is the interface I don't have any particular issues with.
> 
> Yeah, but it's gone. See updated patches.

I meant "the userspace interface".  I did see the update patch
that changed it to use about configfs.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/