Message ID | 20081030195630.GA13640@oksana.dev.rtsoft.ru (mailing list archive) |
---|---|
State | Accepted, archived |
Commit | 3f1c6ebf57b815ad709e89291e446935fee78f75 |
Headers | show |
On Thu, Oct 30, 2008 at 1:56 PM, Anton Vorontsov <avorontsov@ru.mvista.com> wrote: > The bindings describes a case where MMC/SD/SDIO slot directly connected > to a SPI bus. Such setups are widely used on embedded PowerPC boards. > > The patch also adds the mmc-spi-slot entry to the OpenFirmware modalias > table. > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> Mostly looks good to me. A few comments below. > --- > .../powerpc/dts-bindings/mmc-spi-slot.txt | 23 ++++++++++++++++++++ > drivers/of/base.c | 1 + > 2 files changed, 24 insertions(+), 0 deletions(-) > create mode 100644 Documentation/powerpc/dts-bindings/mmc-spi-slot.txt > > diff --git a/Documentation/powerpc/dts-bindings/mmc-spi-slot.txt b/Documentation/powerpc/dts-bindings/mmc-spi-slot.txt > new file mode 100644 > index 0000000..c39ac28 > --- /dev/null > +++ b/Documentation/powerpc/dts-bindings/mmc-spi-slot.txt > @@ -0,0 +1,23 @@ > +MMC/SD/SDIO slot directly connected to a SPI bus > + > +Required properties: > +- compatible : should be "mmc-spi-slot". > +- reg : should specify SPI address (chip-select number). > +- spi-max-frequency : maximum frequency for this device (Hz). > +- voltage-ranges : two cells are required, first cell specifies minimum > + slot voltage (mV), second cell specifies maximum slot voltage (mV). > + Several ranges could be specified. > +- gpios : (optional) may specify GPIOs in this order: Card-Detect GPIO, > + Write-Protect GPIO. I wonder if we're following the example of irq mappings too closely for the gpios property. I like the layout of the property (<controller> <specifier>), but I think the 'gpios' name is getting too overloaded. In this case a single property 'gpios' is being used to encode 2 unrelated bits of information; the write protect pin and the card detect pins. In this particular case I think it is better to use 2 properties in this case; something like 'spi-writeprotect-gpio' and 'spi-carddetect-gpio' using the same specifier format. Doing so adds a bit more clarity to the purpose of the properties. I my mind I differentiate this from other examples (for instance a series of CS pins) based on how closely related the pin functions are. So I would say for the following examples... 1) GPIO data bus (SPI, MDIO and I2C are great examples); all pins must be present - single gpio property 2) This MMC case (pins are optional and unrelated); separate gpio properties 3) LCD with backlight and contrast control pins; one gpio property for backlight pins, one for constrast pins. Thoughts? > + > +Example: > + > + mmc-slot@0 { > + compatible = "fsl,mpc8323rdb-mmc-slot", > + "mmc-spi-slot"; > + reg = <0>; > + gpios = <&qe_pio_d 14 1 > + &qe_pio_d 15 0>; > + voltage-ranges = <3300 3300>; > + spi-max-frequency = <50000000>; > + }; > diff --git a/drivers/of/base.c b/drivers/of/base.c > index 7c79e94..c6797ca 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -411,6 +411,7 @@ struct of_modalias_table { > }; > static struct of_modalias_table of_modalias_table[] = { > { "fsl,mcu-mpc8349emitx", "mcu-mpc8349emitx" }, > + { "mmc-spi-slot", "mmc_spi" }, > }; > > /** > -- > 1.5.6.3 > >
On Thu, Oct 30, 2008 at 02:37:31PM -0600, Grant Likely wrote: [...] > > +- gpios : (optional) may specify GPIOs in this order: Card-Detect GPIO, > > + Write-Protect GPIO. > > I wonder if we're following the example of irq mappings too closely > for the gpios property. I like the layout of the property > (<controller> <specifier>), but I think the 'gpios' name is getting > too overloaded. In this case a single property 'gpios' is being used > to encode 2 unrelated bits of information; the write protect pin and > the card detect pins. > > In this particular case I think it is better to use 2 properties in > this case; something like 'spi-writeprotect-gpio' and > 'spi-carddetect-gpio' using the same specifier format. Doing so adds > a bit more clarity to the purpose of the properties. > > I my mind I differentiate this from other examples (for instance a > series of CS pins) based on how closely related the pin functions are. > So I would say for the following examples... > 1) GPIO data bus (SPI, MDIO and I2C are great examples); all pins must > be present - single gpio property > 2) This MMC case (pins are optional and unrelated); separate gpio properties > 3) LCD with backlight and contrast control pins; one gpio property for > backlight pins, one for constrast pins. > > Thoughts? It's pretty trivial to implement (of_get_named_gpio() -- could be just factored out of of_get_gpio()). Though, 1. The idea is quite extreme. It needs discussion, and furthermore, we need to define when do we use gpios = <> and when something-gpio = <>; We need to be consistent, and to be consistent, the rules should be clear and written. 2. We should think about it very very carefully. Do we want to lose the track of gpios? For example, there are quite defined rules when (and in what properties) you may encounter memory addresses, when and where you can encounter interrupt specifiers. We do the same for gpios, and so far it works great. We need to think about any possible drawbacks of the scheme you purpose (we would never know where to expect gpios - it isn't a problem per se, but maybe it could lead to some problem in future? I don't know.) Quite honestly I don't like the idea... maybe I just used to interrupts = <>, reg = <>, ranges = <>, interrupt-map = <> and so forth, and now my subconsciousness tells me "it's wrong to do something-interrupt = <> stuff." ;-) Anyway, your proposal is forward and backward compatible with the existing scheme, and can even coexist. Thus I'd prefer to stay with the today's gpios = <>. We can always start use the new scheme when it will be thought out enough. Thanks, p.s. I'd prefer a new thread for this discussion, somewhere in devicetree-discuss@ozlabs.org, so that it won't relate to this particular patch.
On Fri, Oct 31, 2008 at 02:02:53AM +0300, Anton Vorontsov wrote: > On Thu, Oct 30, 2008 at 02:37:31PM -0600, Grant Likely wrote: > [...] > > > +- gpios : (optional) may specify GPIOs in this order: Card-Detect GPIO, > > > + Write-Protect GPIO. > > > > I wonder if we're following the example of irq mappings too closely > > for the gpios property. I like the layout of the property > > (<controller> <specifier>), but I think the 'gpios' name is getting > > too overloaded. In this case a single property 'gpios' is being used > > to encode 2 unrelated bits of information; the write protect pin and > > the card detect pins. > > > > In this particular case I think it is better to use 2 properties in > > this case; something like 'spi-writeprotect-gpio' and > > 'spi-carddetect-gpio' using the same specifier format. Doing so adds > > a bit more clarity to the purpose of the properties. > > > > I my mind I differentiate this from other examples (for instance a > > series of CS pins) based on how closely related the pin functions are. > > So I would say for the following examples... > > 1) GPIO data bus (SPI, MDIO and I2C are great examples); all pins must > > be present - single gpio property > > 2) This MMC case (pins are optional and unrelated); separate gpio properties > > 3) LCD with backlight and contrast control pins; one gpio property for > > backlight pins, one for constrast pins. > > > > Thoughts? > > It's pretty trivial to implement (of_get_named_gpio() -- could be just > factored out of of_get_gpio()). > > Though, > > 1. The idea is quite extreme. It needs discussion, and furthermore, > we need to define when do we use gpios = <> and when something-gpio = > <>; We need to be consistent, and to be consistent, the rules should > be clear and written. > > 2. We should think about it very very carefully. Do we want to lose the > track of gpios? For example, there are quite defined rules when (and > in what properties) you may encounter memory addresses, when and > where you can encounter interrupt specifiers. We do the same for > gpios, and so far it works great. We need to think about any possible > drawbacks of the scheme you purpose (we would never know where to > expect gpios - it isn't a problem per se, but maybe it could lead > to some problem in future? I don't know.) > > Quite honestly I don't like the idea... maybe I just used to > interrupts = <>, reg = <>, ranges = <>, interrupt-map = <> and so > forth, and now my subconsciousness tells me "it's wrong to do > something-interrupt = <> stuff." ;-) Fwiw, I agree. The current scheme works, adding new places to look for gpio specifiers will just complexify things. Long lists of gpios may be somewhat awkward to work with, but I don't think it's sufficiently bad to warrant another scheme.
On Fri, Oct 31, 2008 at 02:02:53AM +0300, Anton Vorontsov wrote: [...] > It's pretty trivial to implement (of_get_named_gpio() -- could be just > factored out of of_get_gpio()). > > Though, > > 1. The idea is quite extreme. It needs discussion, and furthermore, > we need to define when do we use gpios = <> and when something-gpio = > <>; We need to be consistent, and to be consistent, the rules should > be clear and written. > > 2. We should think about it very very carefully. Do we want to lose the > track of gpios? For example, there are quite defined rules when (and > in what properties) you may encounter memory addresses, when and > where you can encounter interrupt specifiers. We do the same for > gpios, and so far it works great. We need to think about any possible > drawbacks of the scheme you purpose (we would never know where to > expect gpios - it isn't a problem per se, but maybe it could lead > to some problem in future? I don't know.) > > Quite honestly I don't like the idea... maybe I just used to > interrupts = <>, reg = <>, ranges = <>, interrupt-map = <> and so > forth, and now my subconsciousness tells me "it's wrong to do > something-interrupt = <> stuff." ;-) Btw, not that I hate this new scheme, sometimes the scheme is even inevitable. For example when we have gpios with two or more ellipsis: gpios = <... ...>. But this should be a separate discussion, really.
diff --git a/Documentation/powerpc/dts-bindings/mmc-spi-slot.txt b/Documentation/powerpc/dts-bindings/mmc-spi-slot.txt new file mode 100644 index 0000000..c39ac28 --- /dev/null +++ b/Documentation/powerpc/dts-bindings/mmc-spi-slot.txt @@ -0,0 +1,23 @@ +MMC/SD/SDIO slot directly connected to a SPI bus + +Required properties: +- compatible : should be "mmc-spi-slot". +- reg : should specify SPI address (chip-select number). +- spi-max-frequency : maximum frequency for this device (Hz). +- voltage-ranges : two cells are required, first cell specifies minimum + slot voltage (mV), second cell specifies maximum slot voltage (mV). + Several ranges could be specified. +- gpios : (optional) may specify GPIOs in this order: Card-Detect GPIO, + Write-Protect GPIO. + +Example: + + mmc-slot@0 { + compatible = "fsl,mpc8323rdb-mmc-slot", + "mmc-spi-slot"; + reg = <0>; + gpios = <&qe_pio_d 14 1 + &qe_pio_d 15 0>; + voltage-ranges = <3300 3300>; + spi-max-frequency = <50000000>; + }; diff --git a/drivers/of/base.c b/drivers/of/base.c index 7c79e94..c6797ca 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -411,6 +411,7 @@ struct of_modalias_table { }; static struct of_modalias_table of_modalias_table[] = { { "fsl,mcu-mpc8349emitx", "mcu-mpc8349emitx" }, + { "mmc-spi-slot", "mmc_spi" }, }; /**
The bindings describes a case where MMC/SD/SDIO slot directly connected to a SPI bus. Such setups are widely used on embedded PowerPC boards. The patch also adds the mmc-spi-slot entry to the OpenFirmware modalias table. Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> --- .../powerpc/dts-bindings/mmc-spi-slot.txt | 23 ++++++++++++++++++++ drivers/of/base.c | 1 + 2 files changed, 24 insertions(+), 0 deletions(-) create mode 100644 Documentation/powerpc/dts-bindings/mmc-spi-slot.txt