Re: [PATCH 24/29] mmc: add a simple generic OF parser

[Magnus Damm <magnus.damm@xxxxxxxxx>]

On Fri, May 4, 2012 at 12:05 AM, Guennadi Liakhovetski
<g.liakhovetski@xxxxxx> wrote:
> Many MMC host drivers already use OF data to obtain their platform-specific
> configuration. Each of them is doing this in its special way, whereas many
> parameters are identical and can easily be generalised. This patch adds
> such a generic parser. So far it only adds and handles very few basic
> properties. New ones can be added in the future as required.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>

I usually don't say things this loud, but here it is: NAK!

To begin with: this is not what we agreed on earlier. I asked you to
only implement a small subset to begin with. This so we don't end up
supporting a bunch of DT flags forever.

In general it's a good idea to make something generic, but I think now
is too early for our drivers on SH and SH-Mobile ARM. We need to
experiment more with PINCTRL as well as GPIO bindings via DT. When we
have done that I think we should revisit.

Please see below for my reasons:

> +       if (of_get_property(node, "mmc,4_bit_data", NULL))
> +               mmc->caps |= MMC_CAP_4_BIT_DATA;
> +       if (of_get_property(node, "mmc,8_bit_data", NULL))
> +               mmc->caps |= MMC_CAP_8_BIT_DATA;

I suspect that these overlap with PINCTRL. I don't want to add support
for potentially duplicated functionality.

> +       gpio = of_get_named_gpio_flags(node, "cd-gpios", 0, &flags);
> +       ret = mmc_gpio_request_cd(mmc, gpio);
> +       if (ret < 0)
> +               dev_warn(mmc->parent, "Failed to obtain CD GPIO: %d\n", ret);
> +
> +       gpio = of_get_named_gpio_flags(node, "ro-gpios", 0, &flags);
> +       ret = mmc_gpio_request_ro(mmc, gpio);
> +       if (ret < 0)
> +               dev_warn(mmc->parent, "Failed to obtain RO GPIO: %d\n", ret);
> +}

We want to have stable DT bindings for GPIO and PINCTRL before we let
this loose with DT.

So please ignore this patch for now. Thanks for your efforts anyway.

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html