On 3/06/19 8:05 AM, Udit Kumar wrote:
> This pacth enables acpi support in esdhc driver
pacth -> patch
>
> Signed-off-by: Udit Kumar <udit.kumar@xxxxxxx>
Looks ok, but some minor comments.
> ---
> drivers/mmc/host/sdhci-of-esdhc.c | 55 +++++++++++++++++++++++++--------------
> 1 file changed, 36 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
> index e20c00f..11d9f48 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -3,6 +3,7 @@
> *
> * Copyright (c) 2007, 2010, 2012 Freescale Semiconductor, Inc.
> * Copyright (c) 2009 MontaVista Software, Inc.
> + * Copyright 2019 NXP
> *
> * Authors: Xiaobo Xie <X.Xie@xxxxxxxxxxxxx>
> * Anton Vorontsov <avorontsov@xxxxxxxxxxxxx>
> @@ -13,6 +14,7 @@
> * your option) any later version.
> */
>
> +#include <linux/acpi.h>
> #include <linux/err.h>
> #include <linux/io.h>
> #include <linux/of.h>
> @@ -75,6 +77,12 @@ struct esdhc_clk_fixup {
> };
> MODULE_DEVICE_TABLE(of, sdhci_esdhc_of_match);
>
> +static const struct acpi_device_id sdhci_esdhc_ids[] = {
> + {"NXP0003" },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(acpi, sdhci_esdhc_ids);
> +
> struct sdhci_esdhc {
> u8 vendor_ver;
> u8 spec_ver;
> @@ -1038,22 +1046,28 @@ static void esdhc_init(struct platform_device *pdev, struct sdhci_host *host)
> match = of_match_node(sdhci_esdhc_of_match, pdev->dev.of_node);
> if (match)
> esdhc->clk_fixup = match->data;
> - np = pdev->dev.of_node;
> - clk = of_clk_get(np, 0);
> - if (!IS_ERR(clk)) {
> - /*
> - * esdhc->peripheral_clock would be assigned with a value
> - * which is eSDHC base clock when use periperal clock.
> - * For ls1046a, the clock value got by common clk API is
> - * peripheral clock while the eSDHC base clock is 1/2
> - * peripheral clock.
> - */
> - if (of_device_is_compatible(np, "fsl,ls1046a-esdhc"))
> - esdhc->peripheral_clock = clk_get_rate(clk) / 2;
> - else
> - esdhc->peripheral_clock = clk_get_rate(clk);
>
> - clk_put(clk);
> + np = pdev->dev.of_node;
> + // in case of device tree, get clock frame work
I prefer to remain consistent with the legacy comment style i.e.
/* In case of device tree, get clock frame work */
> + if (np) {
> + clk = of_clk_get(np, 0);
> + if (!IS_ERR(clk)) {
> + /*
> + * esdhc->peripheral_clock would be assigned a value
> + * which is eSDHC base clock when use periperal clock.
> + * For ls1046a, the clock value got by common clk API is
> + * peripheral clock while the eSDHC base clock is 1/2
> + * peripheral clock.
> + */
> + if (of_device_is_compatible(np, "fsl,ls1046a-esdhc"))
> + esdhc->peripheral_clock = clk_get_rate(clk) / 2;
> + else
> + esdhc->peripheral_clock = clk_get_rate(clk);
> + clk_put(clk);
> + }
> + } else {
> + device_property_read_u32(&pdev->dev, "clock-frequency",
> + &esdhc->peripheral_clock);
> }
Not sure the "if (np) {}" really serves a purpose since of_clk_get(() will
return an error in that case anyway.
>
> if (esdhc->peripheral_clock) {
> @@ -1062,7 +1076,8 @@ static void esdhc_init(struct platform_device *pdev, struct sdhci_host *host)
> val |= ESDHC_PERIPHERAL_CLK_SEL;
> sdhci_writel(host, val, ESDHC_DMA_SYSCTL);
> esdhc_clock_enable(host, true);
> - }
> + } else
> + WARN_ON(1);
Please balance braces:
} else {
WARN_ON(1);
}
> }
>
> static int esdhc_hs400_prepare_ddr(struct mmc_host *mmc)
> @@ -1081,9 +1096,10 @@ static int sdhci_esdhc_probe(struct platform_device *pdev)
>
> np = pdev->dev.of_node;
>
> - if (of_property_read_bool(np, "little-endian"))
> + if (device_property_read_bool(&pdev->dev, "little-endian")) {
> host = sdhci_pltfm_init(pdev, &sdhci_esdhc_le_pdata,
> sizeof(struct sdhci_esdhc));
> + }
> else
Also here:
} else {
> host = sdhci_pltfm_init(pdev, &sdhci_esdhc_be_pdata,
> sizeof(struct sdhci_esdhc));
}
> @@ -1143,8 +1159,8 @@ static int sdhci_esdhc_probe(struct platform_device *pdev)
> ret = mmc_of_parse(host->mmc);
> if (ret)
> goto err;
> -
> - mmc_of_parse_voltage(np, &host->ocr_mask);
> + if (np)
Again not sure the "if (np)" is really needed.
> + mmc_of_parse_voltage(np, &host->ocr_mask);
>
> ret = sdhci_add_host(host);
> if (ret)
> @@ -1160,6 +1176,7 @@ static int sdhci_esdhc_probe(struct platform_device *pdev)
> .driver = {
> .name = "sdhci-esdhc",
> .of_match_table = sdhci_esdhc_of_match,
> + .acpi_match_table = sdhci_esdhc_ids,
> .pm = &esdhc_of_dev_pm_ops,
> },
> .probe = sdhci_esdhc_probe,
>
