Re: [PATCH 5/6] mmc: Add OF bindings support for mmc host controller capabilities

[Grant Likely <grant.likely@secretlab.ca>]

On Mon, Nov 07, 2011 at 07:51:26PM +0530, Thomas Abraham wrote:
> On 5 November 2011 01:27, Olof Johansson <olof@lixom.net> wrote:
> > On Thu, Nov 03, 2011 at 02:06:02AM +0530, Thomas Abraham wrote:
> >> Device nodes representing sd/mmc controllers in a device tree would include
> >> mmc host controller capabilities. Add support for parsing of mmc host
> >> controller capabilities included in device nodes.
> >>
> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> >> ---
> >>  .../devicetree/bindings/mmc/linux-mmc-host.txt     |   13 ++++++++
> >>  drivers/mmc/core/host.c                            |   31 ++++++++++++++++++++
> >>  include/linux/mmc/host.h                           |    1 +
> >>  3 files changed, 45 insertions(+), 0 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/mmc/linux-mmc-host.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/mmc/linux-mmc-host.txt b/Documentation/devicetree/bindings/mmc/linux-mmc-host.txt
> >> new file mode 100644
> >> index 0000000..714b2b1
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mmc/linux-mmc-host.txt
> >> @@ -0,0 +1,13 @@
> >> +* Linux MMC Host Controller Capabilities
> >> +
> >> +The following bindings can be used in a device node to specify any board
> >> +specific mmc host controller capabilities.
> >> +
> >> +- linux,mmc_cap_4_bit_data - Host can do 4-bit transfers
> >> +- linux,mmc_cap_mmc_highspeed - Host can do MMC high-speed timing
> >> +- linux,mmc_cap_sd_highspeed - Host can do SD high-speed timing
> >> +- linux,mmc_cap_needs_poll - Host needs polling for card detection
> >> +- linux,mmc_cap_8_bit_data - Host can do 8-bit transfer
> >> +- linux,mmc_cap_disable - Host can be disabled and re-enabled to save power
> >> +- linux,mmc_cap_nonremovable - Host is connected to nonremovable media
> >> +- linux,mmc_cap_erase - Host allows erase/trim commands
> >
> > linux-prefixed properties are a big red flag. The device tree should describe
> > the hardware, not what linux does with the hardware.
> 
> The vendor-prefixes documentation for device tree bindings includes
> 'linux' as an option. And I was trying to encode the linux specific
> host controller capabilities using the above bindings.
> 
> >
> > See previous comments about "support-8bit" for encoding exactly the same
> > hardware capability in a linux-agnostic way. What the sdhci driver chooses to
> > do with it is up to the driver, and in some cases it means it will set the
> > capabilities flag.
> >
> > Same goes for the other properties. It should not go in as it is
> > implemented in this patch, it needs to be fixed up.
> 
> Ok. I will remove all these linux specific bindings and replace with a
> more generic ones. Bindings will be defined for all the linux defined
> host capabilities. Non-linux platforms can add additional bindings as
> required. A function to parse such properties from a controller device
> node could actually be shared among all the mmc/sd host controller
> drivers in linux. I will redo this patch and submit again.
> 
> Thanks Olof for your review and comments.

This patch appears to be conceptually wrong.  Many of these properties
are merely static capabilities of each individual device which the
driver should already have knowledge of, and be setting the capability
bits appropriately on its own.

So, you /don't/ want to simply create a 1:1 list between the current
linux capability bits (which are subject to change) and the device
tree properties (which ideally must not be changed after they are
defined).

What you need to do is figure out which properties are required to
describe the hardware about things that the driver wouldn't already
know.  For example, 'polling' is something the driver would already
know because it is an aspect of the specific device, but
'nonremovable' is a physical characteristic of some boards.  'disable'
and the speed timings are also something I would expect the driver to
know about and what to set itself.

So, only define properties for things that the device-specific driver
cannot know for itself; or are the sort of parameters that a
multi-device driver is already using for differentiation (ie. appears
in pdata instead of directly set by the driver).  You should be able
to justify the need for each property that gets defined.

g.