Re: [PATCH v3 4/4] mmc: sdhci-esdhc-imx: add device tree probe support

[Anton Vorontsov <cbouatmailru@gmail.com>]

Hi,

On Wed, Jul 06, 2011 at 03:05:18PM -0600, Grant Likely wrote:
> On Thu, Jul 07, 2011 at 12:47:50AM +0800, Shawn Guo wrote:
> > The patch adds device tree probe support for sdhci-esdhc-imx driver.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Wolfram Sang <w.sang@pengutronix.de>
> > Cc: Chris Ball <cjb@laptop.org>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
[...]
> > +Optional properties:
> > +- fsl,card-wired : Indicate the card is wired to host permanently
> > +- fsl,cd-internal : Indicate to use controller internal card detection
> > +- fsl,wp-internal : Indicate to use controller internal write protection
> > +- cd-gpios : Specify GPIOs for card detection
> > +- wp-gpios : Specify GPIOs for write protection
[...]
> > +	cd-gpios = <&gpio0 6 0>; /* GPIO1_6 */
> > +	wp-gpios = <&gpio0 5 0>; /* GPIO1_5 */

Is there any particular reason why we started using named GPIOs
in the device tree? To me, that's quite drastic change... should
we start using named regs and interrupts as well? Personally, I
don't think that the idea is great, as now you don't know where
to expect memory resources, interrupt resources and, eventually
GPIO resources.

Just a few disadvantages off the top of my head:

- You can't statically validate your device tree for correctness.
  Today dtc checks #{address,size}-cells sanity for 'regs' and
  'ranges';
- You can't automatically convert named resources into 'struct
  resource *', as we do for platform devices now;
- Any pros for using named resources in the device tree? I don't
  see any.

So, I suggest to at least discuss this stuff a little bit more
before polluting device trees with dubious ideas.

p.s.

As for this particular patch, I really don't see why we should
use named GPIOs. For consistency, I'd suggest to reuse bindings
from Documentation/devicetree/bindings/mmc/mmc-spi-slot.txt.

Plus, fsl,cd-internal and fsl,wp-internal is not needed: either
you specify GPIOs or not. That already signals whether driver
should use internal detection (i.e. 'present' register) or
external (i.e. GPIO).

And also, why {cd,wp}-gpioS (plural)?

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com