[PATCH 2/3] mmc: davinci_mmc: add DT support
Manjunathappa, Prakash
prakash.pm at ti.com
Tue Feb 5 00:28:14 EST 2013
Hi Mark,
On Thu, Jan 31, 2013 at 16:53:03, Mark Rutland wrote:
> Hello,
>
> I have a few comments on the devicetree binding and the way it's parsed.
>
Thanks for review.
> On Thu, Jan 31, 2013 at 10:33:06AM +0000, Manjunathappa, Prakash wrote:
> > Adds device tree support for davinci_mmc. Also add
> > binding documentation.
> > Tested with non-dma PIO mode and without GPIO
> > card_detect/write_protect option because of
> > dependencies on EDMA and GPIO modules DT support.
> >
> > Signed-off-by: Manjunathappa, Prakash <prakash.pm at ti.com>
> > Cc: linux-mmc at vger.kernel.org
> > Cc: linux-arm-kernel at lists.infradead.org
> > Cc: linux-kernel at vger.kernel.org
> > Cc: davinci-linux-open-source at linux.davincidsp.com
> > Cc: devicetree-discuss at lists.ozlabs.org
> > Cc: cjb at laptop.org
> > Cc: Sekhar Nori <nsekhar at ti.com>
> > Cc: mporter at ti.com
> > ---
> > .../devicetree/bindings/mmc/davinci_mmc.txt | 23 +++++++
> > drivers/mmc/host/davinci_mmc.c | 69 +++++++++++++++++++-
> > 2 files changed, 91 insertions(+), 1 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/mmc/davinci_mmc.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/davinci_mmc.txt b/Documentation/devicetree/bindings/mmc/davinci_mmc.txt
> > new file mode 100644
> > index 0000000..144bee6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mmc/davinci_mmc.txt
> > @@ -0,0 +1,23 @@
> > +* TI Highspeed MMC host controller for DaVinci
> > +
> > +The Highspeed MMC Host Controller on TI DaVinci family
> > +provides an interface for MMC, SD and SDIO types of memory cards.
> > +
> > +This file documents differences between the core properties described
> > +by mmc.txt and the properties used by the davinci_mmc driver.
> > +
> > +Required properties:
> > +- compatible:
> > + Should be "ti,davinci_mmc", for davinci controllers
>
> "ti,davinci-mmc" (with '-' rather than '_') would be preferable.
>
I agree, will correct it.
> > +
> > +Optional properties:
> > +- bus-width: Number of data lines, can be <1>, <4>, or <8>, default <1>
> > +- max-frequency: maximum operating clock frequency.
>
> You said you only described differences from mmc.txt, but this is listed in
> mmc.txt.
>
Agreed, I will remove it from here.
> > +- caps: Check for Host capabilities in <linux/mmc/host.h>
>
> Is this a set of binary flags? Also, is this Linux-specific?
>
> I really don't think this should be in the devicetree binding. Why do you need
> this?
>
I was using this to specify the below controller capabilities:
MMC_CAP_MMC_HIGHSPEED and MMC_CAP_SD_HIGHSPEED,
Found from discussion[1] that this can be derived from max-frequency,
That is above capabilities are supported when max-frequency >= 50MHz.
[1] https://lkml.org/lkml/2012/10/15/231
> > +- version: version of controller.
>
> This should be encoded as part of the compatible string.
>
Agreed will make change accordingly to accommodate version info in compatible string.
> > +Example:
> > + mmc1: mmc at 0x4809c000 {
> > + compatible = "ti,omap4-hsmmc";
> > + reg = <0x4809c000 0x400>;
> > + bus-width = <4>;
> > + };
>
> It would be nice if the example referred to this binding rather than another.
>
Agreed, I will change.
> > diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
> > index 382b79d..ce6730d 100644
> > --- a/drivers/mmc/host/davinci_mmc.c
> > +++ b/drivers/mmc/host/davinci_mmc.c
> > @@ -34,6 +34,7 @@
> > #include <linux/dma-mapping.h>
> > #include <linux/edma.h>
> > #include <linux/mmc/mmc.h>
> > +#include <linux/of.h>
> >
> > #include <linux/platform_data/mmc-davinci.h>
> >
> > @@ -1156,16 +1157,75 @@ static void __init init_mmcsd_host(struct mmc_davinci_host *host)
> >
> > mmc_davinci_reset_ctrl(host, 0);
> > }
> > +#ifdef CONFIG_OF
> > +static struct davinci_mmc_config
> > + *mmc_of_get_pdata(struct platform_device *pdev)
> > +{
> > + struct device_node *np;
> > + struct davinci_mmc_config *pdata = NULL;
> > + u32 data;
> > + int ret;
> > +
> > + pdata = pdev->dev.platform_data;
> > + if (!pdata) {
> > + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata) {
> > + dev_err(&pdev->dev, "Failed to allocate memory for struct davinci_mmc_config\n");
> > + goto nodata;
> > + }
> > + }
>
> Why do you need to conditionally allocate this? This only seems to be called
> once.
>
This is common function for DT and non-DT kernel(will be removing #ifdef CONFIG_OF),
So above check is necessary for to allocate pdata for DT kernel.
> > +
> > + np = pdev->dev.of_node;
> > + if (!np)
> > + goto nodata;
>
> Why not just return immediately here? You do nothing special at nodata.
>
Following convention to not have more than 1 return from function and have
Common exit point. May not be necessary now since we have devm_* calls now.
Can I still prefer to keep this goto?
> > +
> > + ret = of_property_read_u32(np, "bus-width", &data);
> > + if (ret)
> > + dev_info(&pdev->dev, "wires not specified, defaulting to 4 bit mode\n");
> > + pdata->wires = data;
>
> That dev_info doesn't seem to match up with what the next line is doing (data
> might not have been initialised). Also, unless I've misunderstood, it doesn't
> match up with the default of 1 mentioned in the binding doc.
>
Yes with missing bus-width property module comes in 1 bit mode, I will correct it.
> > +
> > + ret = of_property_read_u32(np, "max-frequency", &data);
> > + if (ret)
> > + dev_info(&pdev->dev, "max-frequency not specified, defaulting to 25MHz\n");
> > + pdata->max_freq = data;
>
> Again, the dev_info doesn't match up with the line below it. I notice
> the default's not one specified in the binding. Is this frequency chosen
> from the hardware docs, or is it an arbitrary choice. If not arbitrary,
> it might be worth specifying in the binding.
>
Agreed, I will make the changes for the default values.
>
> > +
> > + ret = of_property_read_u32(np, "caps", &data);
> > + if (ret)
> > + dev_info(&pdev->dev, "card capability not specified\n");
> > + pdata->caps = data;
>
> Again, you may be using garbage data.
>
Ok I will correct it.
> > +
> > + ret = of_property_read_u32(np, "version", &data);
> > + if (ret)
> > + dev_err(&pdev->dev, "version not specified\n");
> > + pdata->version = data;
>
> And again, though I'd prefer to see this property go entirely.
>
Yes this is going to go away.
> > +
> > +nodata:
> > + return pdata;
> > +}
> > +
> > +#else
> > +static struct davinci_mmc_config
> > + *mmc_of_get_pdata(struct platform_device *pdev)
> > +{
> > + return pdev->dev.platform_data;
> > +}
> > +#endif
>
> This is poorly named if it's required for !CONFIG_OF.
>
> Why not change this to something like mmc_parse_pdata, returning an
> error code. For !CONFIG_OF, it can simply return 0, which should be less
> surprising for anyone else reading this code.
>
I will remove #ifdef CONFIG_OF conditional compilation and consideration
your suggestion to name function as mmc_parse_pdata.
> >
> > static int __init davinci_mmcsd_probe(struct platform_device *pdev)
> > {
> > - struct davinci_mmc_config *pdata = pdev->dev.platform_data;
> > + struct davinci_mmc_config *pdata = NULL;
> > struct mmc_davinci_host *host = NULL;
> > struct mmc_host *mmc = NULL;
> > struct resource *r, *mem = NULL;
> > int ret = 0, irq = 0;
> > size_t mem_size;
> >
> > + pdata = mmc_of_get_pdata(pdev);
> > + if (pdata == NULL) {
> > + dev_err(&pdev->dev, "Can not get platform data\n");
> > + return -ENOENT;
> > + }
> > +
> > /* REVISIT: when we're fully converted, fail if pdata is NULL */
> >
> > ret = -ENODEV;
> > @@ -1403,11 +1463,18 @@ static const struct dev_pm_ops davinci_mmcsd_pm = {
> > #define davinci_mmcsd_pm_ops NULL
> > #endif
> >
> > +static const struct of_device_id davinci_mmc_of_match[] = {
> > + {.compatible = "ti,davinci_mmc", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, davinci_mmc_of_match);
>
> For supporting multiple versions, you could either use some auxdata
> here, or check each one in davince_mmcsd_probe.
>
I will consider keeping auxdata via compatible field.
> > +
> > static struct platform_driver davinci_mmcsd_driver = {
> > .driver = {
> > .name = "davinci_mmc",
> > .owner = THIS_MODULE,
> > .pm = davinci_mmcsd_pm_ops,
> > + .of_match_table = of_match_ptr(davinci_mmc_of_match),
> > },
> > .remove = __exit_p(davinci_mmcsd_remove),
> > };
>
> Where does davinci_mmcsd_probe get called from, and how is the
> of_match_table used? Should it not be set as .probe on the driver?
>
Driver probe is registered in module_init.
And are you suggesting me to use module_platform_driver? If yes can it not
be taken separately as it is unrelated to DT support I am adding.
of_match_table is used by device tree. (will point to NULL for !CONFIG_OF)
Thanks,
Prakash
More information about the devicetree-discuss
mailing list