Re: [PATCH v10 2/3] mmc: sdhci-msm: Initial support for Qualcomm chipsets

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/05/2014 06:41 AM, Ulf Hansson wrote:
On 4 March 2014 20:27, Georgi Djakov <gdjakov@xxxxxxxxxx> wrote:
[..]
+
+struct sdhci_msm_pltfm_data {
+       u32 caps;                               /* Supported UHS-I Modes */
+       u32 caps2;                              /* More capabilities */

Why do you need these caps, there are already a part of the mmc host.


Thanks! Will remove.

+       struct regulator *vdd;                  /* VDD/VCC regulator */
+       struct regulator *vdd_io;               /* VDD IO regulator */

Why do you need to duplicate the regulators for sdhci_msm? sdhci core
is already taking care of regulators, I suppose you should adopt to
that!?


Ok, I'll try to adopt this.

+};
+
+struct sdhci_msm_host {
+       struct platform_device *pdev;
+       void __iomem *core_mem; /* MSM SDCC mapped address */
+       int pwr_irq;            /* power irq */
+       struct clk *clk;        /* main SD/MMC bus clock */
+       struct clk *pclk;       /* SDHC peripheral bus clock */
+       struct clk *bus_clk;    /* SDHC bus voter clock */
+       struct sdhci_msm_pltfm_data pdata;
+       struct mmc_host *mmc;
+       struct sdhci_pltfm_data sdhci_msm_pdata;
+};
+
[..]
+static int sdhci_msm_vreg_init(struct device *dev,
+                              struct sdhci_msm_pltfm_data *pdata)
+{
+       pdata->vdd = devm_regulator_get(dev, "vdd-supply");
+       if (IS_ERR(pdata->vdd))
+               return PTR_ERR(pdata->vdd);
+
+       pdata->vdd_io = devm_regulator_get(dev, "vdd-io-supply");
+       if (IS_ERR(pdata->vdd_io))
+               return PTR_ERR(pdata->vdd_io);
+
+       return 0;
+}
+

The hole regulator handling seems like it's being duplicated from the
sdhci core. If the regulator handling from the sdhci core don't suite
your need I guess you should extend that instead - to prevent the
duplication of code and structs.

Moreover I think it's time for sdhci core to move on to use the
mmc_regulator_get_supply() API. Additionally it should be able to use
mmc_regulator_set_ocr() API to change voltage. I guess that's not
related to this patch though, but just wanted to provide you my view
on it.

Ok, I see. Thanks for the hints!

[..]
+       ret = sdhci_add_host(host);
+       if (ret) {
+               dev_err(&pdev->dev, "Add host failed (%d)\n", ret);
+               goto vreg_disable;
+       }
+
+       ret = clk_set_rate(msm_host->clk, host->max_clk);

Don't you need to set the rate before adding the host?


I will just make use of the sdhci core for clock setup too. Thanks for reviewing!

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




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux