RE: [PATCH V4][3/4] mmc: Add dw mobile mmc cmdq rtk driver

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

 



>> Add Realtek mmc driver to make good use Synopsys DesignWare mmc cmdq 
>> host driver.
>>
>> Signed-off-by: Jyan Chou <jyanchou@xxxxxxxxxxx>
>>
>> ---
>> v3 -> v4:
>> - Modify dma setting's code to fix linux coding style.

> And coding style of all other parts were ignored. You must fix it everywhere in your code.

Thanks for your remind, we checked other linux coding style issues.
...

>> +
>> +static void dw_mci_rtk_init_card(struct mmc_host *host, struct 
>> +mmc_card *card) {
>> +     /* In Realtek Platform, we need to attach eMMC card onto mmc host
>> +      * during eMMC initialization because of the following reason:
>> +      * When system cannot run the hs400, we need to down speed to hs200
>> +      * and call mmc_hw_reset and modify the mmc card attribute through mmc host.
>> +      * At this moment, system will show errors if host->card = NULL.
>> +      */
>> +     host->card = card;
>> +}
>> +
>> +static int dw_mci_rtk_parse_dt(struct dw_mci *host) {
>> +     struct dw_mci_rtkemmc_host *priv;
>> +     const u32 *prop;
>> +     int size;
>> +
>> +     priv = devm_kzalloc(host->dev, sizeof(struct 
>> + dw_mci_rtkemmc_host), GFP_KERNEL);

> sizeof(*)

We had corrected it.

>> +     if (!priv)
>> +             return -ENOMEM;
>> +
>> +     priv->pinctrl = devm_pinctrl_get(host->dev);
>> +     if (IS_ERR(priv->pinctrl))
>> +             dev_dbg(host->dev, "no pinctrl\n");
>> +
>> +     priv->pins_default = pinctrl_lookup_state(priv->pinctrl,
>> +                                               PINCTRL_STATE_DEFAULT);
>> +     if (IS_ERR(priv->pins_default))
>> +             dev_warn(host->dev, "could not get default state\n");
>> +

> So this is required by the driver but not by the bindings.

These pinctrl setting are required by the driver passing from the bindings.

>> +     priv->pins_sdr50 = pinctrl_lookup_state(priv->pinctrl,
>> +                                             "sdr50");
>> +     if (IS_ERR(priv->pins_sdr50))
>> +             dev_warn(host->dev, "could not get sdr50 state\n");
>> +
>> +     priv->pins_hs200 = pinctrl_lookup_state(priv->pinctrl,
>> +                                             "hs200");
>> +     if (IS_ERR(priv->pins_hs200))
>> +             dev_warn(host->dev, "could not get hs200 state\n");
>> +
>> +     priv->pins_hs400 = pinctrl_lookup_state(priv->pinctrl,
>> +                                             "hs400");
>> +     if (IS_ERR(priv->pins_hs400))
>> +             dev_warn(host->dev, "could not get hs400 state\n");
>> +
>> +     priv->pins_tune0 = pinctrl_lookup_state(priv->pinctrl,
>> +                                             "tune0");
>> +     if (IS_ERR(priv->pins_tune0))
>> +             dev_warn(host->dev, "could not get tune0 state\n");
>> +
>> +     priv->pins_tune1 = pinctrl_lookup_state(priv->pinctrl,
>> +                                             "tune1");
>> +     if (IS_ERR(priv->pins_tune1))
>> +             dev_warn(host->dev, "could not get tune1 state\n");
>> +
>> +     priv->pins_tune2 = pinctrl_lookup_state(priv->pinctrl,
>> +                                             "tune2");
>> +     if (IS_ERR(priv->pins_tune2))
>> +             dev_warn(host->dev, "could not get tune2 state\n");
>> +
>> +     priv->pins_tune3 = pinctrl_lookup_state(priv->pinctrl,
>> +                                             "tune3");
>> +     if (IS_ERR(priv->pins_tune3))
>> +             dev_warn(host->dev, "could not get tune3 state\n");
>> +
>> +     priv->pins_tune4 = pinctrl_lookup_state(priv->pinctrl,
>> +                                             "tune4");
>> +
>> +     if (IS_ERR(priv->pins_tune4))
>> +             dev_warn(host->dev, "could not get tune4 state\n");
>> +
>> +     priv->vp0 = devm_clk_get(host->dev, "vp0");
>> +     if (IS_ERR(priv->vp0))
>> +             dev_err(host->dev, "could not get vp0 clk\n");
>> +
>> +     priv->vp1 = devm_clk_get(host->dev, "vp1");
>> +     if (IS_ERR(priv->vp1))
>> +             dev_err(host->dev, "could not get vp1 clk\n");

> dev_err_probe. Everywhere where applicable.

We had replace dev_err to dev_err_probe for error prints during probe.

>> +
>> +     priv->emmc_mode = 0;
>> +     prop = of_get_property(host->dev->of_node, "speed-step", &size);
>> +     if (prop) {
>> +             priv->emmc_mode = of_read_number(prop, 1);
>> +             dev_info(host->dev, "emmc mode : %d\n", 
>> + priv->emmc_mode);

> Drop

Okay.

>> +     } else {
>> +             dev_info(host->dev, "use default emmc sdr50 mode !\n");

> Drop, why is this a problem?

Okay.

>> +     }
>> +
>> +     priv->is_cqe = 0;
>> +     prop = of_get_property(host->dev->of_node, "cqe", &size);
>> +     if (prop) {
>> +             priv->is_cqe = of_read_number(prop, 1);
>> +             dev_info(host->dev, "cmdq mode : %d\n", priv->is_cqe);

> Drop

Okay.

>> +     } else {
>> +             dev_info(host->dev, "use default eMMC legacy mode !\n");

> Drop

Okay.

>> +     }
>> +
>> +     prop = of_get_property(host->dev->of_node, "rdq-ctrl", &size);
>> +     if (prop) {
>> +             priv->rdq_ctrl = of_read_number(prop, 1);
>> +             dev_info(host->dev, "get rdq-ctrl : %u\n", 
>> + priv->rdq_ctrl);

> Drop

Okay.

>> +     } else {
>> +             priv->rdq_ctrl = 0;
>> +             dev_info(host->dev, "no dqs_dly_tape switch node, use 
>> + default 0x0 !!\n");

> Drop

Okay.

>> +     }
>> +
>> +     priv->m2tmx = syscon_regmap_lookup_by_phandle(host->dev->of_node, "realtek,m2tmx");
>> +     if (IS_ERR_OR_NULL(priv->m2tmx))
>> +             dev_err(host->dev, "can not get m2mtx node.\n");
>> +
>> +     host->priv = priv;
>> +
>> +     return 0;
>> +}
>> +
>> +static int dw_mci_rtk_init(struct dw_mci *host) {
>> +     struct dw_mci_rtkemmc_host *priv = host->priv;
>> +
>> +     host->pdata->caps2 = MMC_CAP2_NO_SDIO | MMC_CAP2_NO_SD;
>> +
>> +     if (priv->emmc_mode >= 2)
>> +             host->pdata->caps2 |= MMC_CAP2_HS200_1_8V_SDR;
>> +     if (priv->emmc_mode >= 3) {
>> +             host->pdata->caps |= MMC_CAP_1_8V_DDR;
>> +             host->pdata->caps2 |= MMC_CAP2_HS400_1_8V;
>> +     }
>> +
>> +     if (priv->is_cqe > 0)
>> +             host->pdata->caps2 |= (MMC_CAP2_CQE | 
>> + MMC_CAP2_CQE_DCMD);
>> +
>> +     host->irq_flags = IRQF_SHARED;
>> +
>> +     mcq_writel(host, CP, 0x0);
>> +
>> +     /*Enable L4 gated*/

> Read Linux coding style. Multiple times if needed.

Thanks for your remind.

>> +     mcq_writel(host, OTHER1, mcq_readl(host, OTHER1) &
>> +             ~(SDMMC_L4_GATED_DIS | SDMMC_L4_GATED_DIS1));
>> +
>> +     mcq_writel(host, OTHER1, mcq_readl(host, OTHER1) &
>> +                (~(SDMMC_DQS_CTRL_GATE_DIS | 
>> + SDMMC_DBUS_MAS_GATING_DIS)));
>> +
>> +     /*Set the eMMC wrapper little Endian*/
>> +     mcq_writel(host, AHB, mcq_readl(host, AHB) | SDMMC_AHB_BIG);
>> +
>> +     mcq_writel(host, OTHER1,
>> +                mcq_readl(host, OTHER1) | 
>> + SDMMC_STARK_CARD_STOP_ENABLE);
>> +
>> +     /*set eMMC instead of nand*/
>> +     regmap_update_bits_base(priv->m2tmx, SDMMC_NAND_DMA_SEL,
>> +                             SDMMC_SRAM_DMA_SEL, SDMMC_SRAM_DMA_SEL, 
>> + NULL, false, true);
>> +
>> +     /*Set the clk initial phase*/
>> +     dw_mci_rtk_phase_tuning(host, 0, 0);
>> +
>> +     return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int dw_mci_rtk_suspend(struct device *dev) {
>> +     struct dw_mci *host = dev_get_drvdata(dev);
>> +     int ret = 0;
>> +
>> +     ret = dw_mci_cqe_runtime_suspend(dev);
>> +     mcq_writel(host, AHB, 0);
>> +
>> +     return ret;
>> +}
>> +
>> +static int dw_mci_rtk_resume(struct device *dev) {
>> +     struct dw_mci *host = dev_get_drvdata(dev);
>> +     int ret = 0;
>> +
>> +     mcq_writel(host, AHB, mcq_readl(host, AHB) | SDMMC_AHB_BIG);
>> +     ret = dw_mci_cqe_runtime_resume(dev);
>> +
>> +     return ret;
>> +}
>> +#else
>> +static int dw_mci_rtk_suspend(struct device *dev) {
>> +     dev_info(dev, "User should enable CONFIG_PM kernel config\n");

> NAK, come on. I asked to drop it. Did you just ignore the feedback? Yep...

Okay.

>> +
>> +     return 0;
>> +}
>> +
>> +static int dw_mci_rtk_resume(struct device *dev) {
>> +     dev_info(dev, "User should enable CONFIG_PM kernel config\n");

> NAK

We had removed it.

>> +
>> +     return 0;
>> +}
>> +#endif /*CONFIG_PM*/
>> +static const struct dev_pm_ops rtk_dev_pm_ops = {
>> +     SET_SYSTEM_SLEEP_PM_OPS(dw_mci_rtk_suspend,
>> +                             dw_mci_rtk_resume)
>> +     SET_RUNTIME_PM_OPS(dw_mci_cqe_runtime_suspend,
>> +                        dw_mci_cqe_runtime_resume,
>> +                        NULL)
>> +};
>> +
>> +static void dw_mci_rtk_shutdown(struct platform_device *pdev) {
>> +     dev_info(&pdev->dev, "[eMMC] Shutdown\n");

> NAK

We had removed it.

>> +     dw_mci_cqe_runtime_resume(&pdev->dev);
>> +}
>> +
>> +static unsigned long dw_mci_rtk_dwmmc_caps[1] = {
>> +     MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA |
>> +     MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED |
>> +     MMC_CAP_NONREMOVABLE | MMC_CAP_CMD23, };
>> +
>> +static const struct dw_mci_drv_data rtk_drv_data = {
>> +     .caps                   = dw_mci_rtk_dwmmc_caps,
>> +     .num_caps               = ARRAY_SIZE(dw_mci_rtk_dwmmc_caps),
>> +     .set_ios                = dw_mci_rtk_set_ios,
>> +     .execute_tuning         = dw_mci_rtk_execute_tuning,
>> +     .parse_dt               = dw_mci_rtk_parse_dt,
>> +     .init                   = dw_mci_rtk_init,
>> +     .prepare_hs400_tuning   = dw_mci_rtk_prepare_hs400_tuning,
>> +     .hs400_complete         = dw_mci_rtk_hs400_complete,
>> +     .init_card              = dw_mci_rtk_init_card,
>> +};
>> +
>> +static const struct of_device_id dw_mci_rtk_match[] = {
>> +     { .compatible = "realtek,rtd1325-dw-cqe-emmc",
>> +             .data = &rtk_drv_data },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(of, dw_mci_rtk_match);
>> +
>> +int dw_mci_cqe_pltfm_register(struct platform_device *pdev,
>> +                           const struct dw_mci_drv_data *drv_data) {
>> +     struct dw_mci *host;
>> +     struct resource *regs;
>> +
>> +     host = devm_kzalloc(&pdev->dev, sizeof(struct dw_mci), 
>> + GFP_KERNEL);

> sizeof(*)

We had corrected it.

>> +     if (!host)
>> +             return -ENOMEM;
>> +
>> +     host->irq = platform_get_irq(pdev, 0);
>> +     if (host->irq < 0)
>> +             return host->irq;
>> +
>> +     host->drv_data = drv_data;
>> +     host->pdev = pdev;
>> +     host->dev = &pdev->dev;
>> +     host->irq_flags = 0;
>> +     host->pdata = pdev->dev.platform_data;
>> +
>> +     regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     host->regs = devm_ioremap_resource(&pdev->dev, regs);

> Use helper for this.

> Open existing, recent drivers and use the them as template or some set of patterns. Sorry, but upstreaming your vendor code will be painful, because you started from some old, buggier version.

We had replaced platform_get_resource , devm_ioremap_resource  by devm_platform_ioremap_resource.

>> +     if (IS_ERR(host->regs))
>> +             return PTR_ERR(host->regs);
>> +
>> +     /* Get registers' physical base address */
>> +     host->phy_regs = regs->start;
>> +
>> +     platform_set_drvdata(pdev, host);
>> +
>> +     return dw_mci_cqe_probe(host);
>> +}
>> +
>> +static int dw_mci_rtk_probe(struct platform_device *pdev) {
>> +     const struct dw_mci_drv_data *drv_data;
>> +     const struct of_device_id *match;
>> +
>> +     if (!pdev->dev.of_node)
>> +             return -ENODEV;
>> +
>> +     match = of_match_node(dw_mci_rtk_match, pdev->dev.of_node);
>> +     drv_data = match->data;
>> +
>> +     return dw_mci_cqe_pltfm_register(pdev, drv_data); }
>> +
>> +int dw_mci_rtk_remove(struct platform_device *pdev) {
>> +     struct dw_mci *host = platform_get_drvdata(pdev);
>> +
>> +     dw_mci_cqe_remove(host);
>> +     return 0;

Best regards,
Jyan

-----Original Message-----
From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 
Sent: Monday, October 30, 2023 3:44 PM
To: Jyan Chou [周芷安] <jyanchou@xxxxxxxxxxx>; ulf.hansson@xxxxxxxxxx; adrian.hunter@xxxxxxxxx; jh80.chung@xxxxxxxxxxx; riteshh@xxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; asutoshd@xxxxxxxxxxxxxx; p.zabel@xxxxxxxxxxxxxx
Cc: linux-mmc@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; arnd@xxxxxxxx; briannorris@xxxxxxxxxxxx; doug@xxxxxxxxxxxxx; tonyhuang.sunplus@xxxxxxxxx; abel.vesa@xxxxxxxxxx; william.qiu@xxxxxxxxxxxxxxxx
Subject: Re: [PATCH V4][3/4] mmc: Add dw mobile mmc cmdq rtk driver


External mail.



On 30/10/2023 07:27, Jyan Chou wrote:
> Add Realtek mmc driver to make good use Synopsys DesignWare mmc cmdq 
> host driver.
>
> Signed-off-by: Jyan Chou <jyanchou@xxxxxxxxxxx>
>
> ---
> v3 -> v4:
> - Modify dma setting's code to fix linux coding style.

And coding style of all other parts were ignored. You must fix it everywhere in your code.

...

> +
> +static void dw_mci_rtk_init_card(struct mmc_host *host, struct 
> +mmc_card *card) {
> +     /* In Realtek Platform, we need to attach eMMC card onto mmc host
> +      * during eMMC initialization because of the following reason:
> +      * When system cannot run the hs400, we need to down speed to hs200
> +      * and call mmc_hw_reset and modify the mmc card attribute through mmc host.
> +      * At this moment, system will show errors if host->card = NULL.
> +      */
> +     host->card = card;
> +}
> +
> +static int dw_mci_rtk_parse_dt(struct dw_mci *host) {
> +     struct dw_mci_rtkemmc_host *priv;
> +     const u32 *prop;
> +     int size;
> +
> +     priv = devm_kzalloc(host->dev, sizeof(struct 
> + dw_mci_rtkemmc_host), GFP_KERNEL);

sizeof(*)

> +     if (!priv)
> +             return -ENOMEM;
> +
> +     priv->pinctrl = devm_pinctrl_get(host->dev);
> +     if (IS_ERR(priv->pinctrl))
> +             dev_dbg(host->dev, "no pinctrl\n");
> +
> +     priv->pins_default = pinctrl_lookup_state(priv->pinctrl,
> +                                               PINCTRL_STATE_DEFAULT);
> +     if (IS_ERR(priv->pins_default))
> +             dev_warn(host->dev, "could not get default state\n");
> +

So this is required by the driver but not by the bindings.

> +     priv->pins_sdr50 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "sdr50");
> +     if (IS_ERR(priv->pins_sdr50))
> +             dev_warn(host->dev, "could not get sdr50 state\n");
> +
> +     priv->pins_hs200 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "hs200");
> +     if (IS_ERR(priv->pins_hs200))
> +             dev_warn(host->dev, "could not get hs200 state\n");
> +
> +     priv->pins_hs400 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "hs400");
> +     if (IS_ERR(priv->pins_hs400))
> +             dev_warn(host->dev, "could not get hs400 state\n");
> +
> +     priv->pins_tune0 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "tune0");
> +     if (IS_ERR(priv->pins_tune0))
> +             dev_warn(host->dev, "could not get tune0 state\n");
> +
> +     priv->pins_tune1 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "tune1");
> +     if (IS_ERR(priv->pins_tune1))
> +             dev_warn(host->dev, "could not get tune1 state\n");
> +
> +     priv->pins_tune2 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "tune2");
> +     if (IS_ERR(priv->pins_tune2))
> +             dev_warn(host->dev, "could not get tune2 state\n");
> +
> +     priv->pins_tune3 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "tune3");
> +     if (IS_ERR(priv->pins_tune3))
> +             dev_warn(host->dev, "could not get tune3 state\n");
> +
> +     priv->pins_tune4 = pinctrl_lookup_state(priv->pinctrl,
> +                                             "tune4");
> +
> +     if (IS_ERR(priv->pins_tune4))
> +             dev_warn(host->dev, "could not get tune4 state\n");
> +
> +     priv->vp0 = devm_clk_get(host->dev, "vp0");
> +     if (IS_ERR(priv->vp0))
> +             dev_err(host->dev, "could not get vp0 clk\n");
> +
> +     priv->vp1 = devm_clk_get(host->dev, "vp1");
> +     if (IS_ERR(priv->vp1))
> +             dev_err(host->dev, "could not get vp1 clk\n");

dev_err_probe. Everywhere where applicable.

> +
> +     priv->emmc_mode = 0;
> +     prop = of_get_property(host->dev->of_node, "speed-step", &size);
> +     if (prop) {
> +             priv->emmc_mode = of_read_number(prop, 1);
> +             dev_info(host->dev, "emmc mode : %d\n", 
> + priv->emmc_mode);

Drop

> +     } else {
> +             dev_info(host->dev, "use default emmc sdr50 mode !\n");

Drop, why is this a problem?

> +     }
> +
> +     priv->is_cqe = 0;
> +     prop = of_get_property(host->dev->of_node, "cqe", &size);
> +     if (prop) {
> +             priv->is_cqe = of_read_number(prop, 1);
> +             dev_info(host->dev, "cmdq mode : %d\n", priv->is_cqe);

Drop


> +     } else {
> +             dev_info(host->dev, "use default eMMC legacy mode !\n");

Drop


> +     }
> +
> +     prop = of_get_property(host->dev->of_node, "rdq-ctrl", &size);
> +     if (prop) {
> +             priv->rdq_ctrl = of_read_number(prop, 1);
> +             dev_info(host->dev, "get rdq-ctrl : %u\n", 
> + priv->rdq_ctrl);

Drop


> +     } else {
> +             priv->rdq_ctrl = 0;
> +             dev_info(host->dev, "no dqs_dly_tape switch node, use 
> + default 0x0 !!\n");

Drop

> +     }
> +
> +     priv->m2tmx = syscon_regmap_lookup_by_phandle(host->dev->of_node, "realtek,m2tmx");
> +     if (IS_ERR_OR_NULL(priv->m2tmx))
> +             dev_err(host->dev, "can not get m2mtx node.\n");
> +
> +     host->priv = priv;
> +
> +     return 0;
> +}
> +
> +static int dw_mci_rtk_init(struct dw_mci *host) {
> +     struct dw_mci_rtkemmc_host *priv = host->priv;
> +
> +     host->pdata->caps2 = MMC_CAP2_NO_SDIO | MMC_CAP2_NO_SD;
> +
> +     if (priv->emmc_mode >= 2)
> +             host->pdata->caps2 |= MMC_CAP2_HS200_1_8V_SDR;
> +     if (priv->emmc_mode >= 3) {
> +             host->pdata->caps |= MMC_CAP_1_8V_DDR;
> +             host->pdata->caps2 |= MMC_CAP2_HS400_1_8V;
> +     }
> +
> +     if (priv->is_cqe > 0)
> +             host->pdata->caps2 |= (MMC_CAP2_CQE | 
> + MMC_CAP2_CQE_DCMD);
> +
> +     host->irq_flags = IRQF_SHARED;
> +
> +     mcq_writel(host, CP, 0x0);
> +
> +     /*Enable L4 gated*/

Read Linux coding style. Multiple times if needed.

> +     mcq_writel(host, OTHER1, mcq_readl(host, OTHER1) &
> +             ~(SDMMC_L4_GATED_DIS | SDMMC_L4_GATED_DIS1));
> +
> +     mcq_writel(host, OTHER1, mcq_readl(host, OTHER1) &
> +                (~(SDMMC_DQS_CTRL_GATE_DIS | 
> + SDMMC_DBUS_MAS_GATING_DIS)));
> +
> +     /*Set the eMMC wrapper little Endian*/
> +     mcq_writel(host, AHB, mcq_readl(host, AHB) | SDMMC_AHB_BIG);
> +
> +     mcq_writel(host, OTHER1,
> +                mcq_readl(host, OTHER1) | 
> + SDMMC_STARK_CARD_STOP_ENABLE);
> +
> +     /*set eMMC instead of nand*/
> +     regmap_update_bits_base(priv->m2tmx, SDMMC_NAND_DMA_SEL,
> +                             SDMMC_SRAM_DMA_SEL, SDMMC_SRAM_DMA_SEL, 
> + NULL, false, true);
> +
> +     /*Set the clk initial phase*/
> +     dw_mci_rtk_phase_tuning(host, 0, 0);
> +
> +     return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int dw_mci_rtk_suspend(struct device *dev) {
> +     struct dw_mci *host = dev_get_drvdata(dev);
> +     int ret = 0;
> +
> +     ret = dw_mci_cqe_runtime_suspend(dev);
> +     mcq_writel(host, AHB, 0);
> +
> +     return ret;
> +}
> +
> +static int dw_mci_rtk_resume(struct device *dev) {
> +     struct dw_mci *host = dev_get_drvdata(dev);
> +     int ret = 0;
> +
> +     mcq_writel(host, AHB, mcq_readl(host, AHB) | SDMMC_AHB_BIG);
> +     ret = dw_mci_cqe_runtime_resume(dev);
> +
> +     return ret;
> +}
> +#else
> +static int dw_mci_rtk_suspend(struct device *dev) {
> +     dev_info(dev, "User should enable CONFIG_PM kernel config\n");

NAK, come on. I asked to drop it. Did you just ignore the feedback? Yep...

> +
> +     return 0;
> +}
> +
> +static int dw_mci_rtk_resume(struct device *dev) {
> +     dev_info(dev, "User should enable CONFIG_PM kernel config\n");

NAK

> +
> +     return 0;
> +}
> +#endif /*CONFIG_PM*/
> +static const struct dev_pm_ops rtk_dev_pm_ops = {
> +     SET_SYSTEM_SLEEP_PM_OPS(dw_mci_rtk_suspend,
> +                             dw_mci_rtk_resume)
> +     SET_RUNTIME_PM_OPS(dw_mci_cqe_runtime_suspend,
> +                        dw_mci_cqe_runtime_resume,
> +                        NULL)
> +};
> +
> +static void dw_mci_rtk_shutdown(struct platform_device *pdev) {
> +     dev_info(&pdev->dev, "[eMMC] Shutdown\n");

NAK

> +     dw_mci_cqe_runtime_resume(&pdev->dev);
> +}
> +
> +static unsigned long dw_mci_rtk_dwmmc_caps[1] = {
> +     MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA |
> +     MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED |
> +     MMC_CAP_NONREMOVABLE | MMC_CAP_CMD23, };
> +
> +static const struct dw_mci_drv_data rtk_drv_data = {
> +     .caps                   = dw_mci_rtk_dwmmc_caps,
> +     .num_caps               = ARRAY_SIZE(dw_mci_rtk_dwmmc_caps),
> +     .set_ios                = dw_mci_rtk_set_ios,
> +     .execute_tuning         = dw_mci_rtk_execute_tuning,
> +     .parse_dt               = dw_mci_rtk_parse_dt,
> +     .init                   = dw_mci_rtk_init,
> +     .prepare_hs400_tuning   = dw_mci_rtk_prepare_hs400_tuning,
> +     .hs400_complete         = dw_mci_rtk_hs400_complete,
> +     .init_card              = dw_mci_rtk_init_card,
> +};
> +
> +static const struct of_device_id dw_mci_rtk_match[] = {
> +     { .compatible = "realtek,rtd1325-dw-cqe-emmc",
> +             .data = &rtk_drv_data },
> +     {},
> +};
> +MODULE_DEVICE_TABLE(of, dw_mci_rtk_match);
> +
> +int dw_mci_cqe_pltfm_register(struct platform_device *pdev,
> +                           const struct dw_mci_drv_data *drv_data) {
> +     struct dw_mci *host;
> +     struct resource *regs;
> +
> +     host = devm_kzalloc(&pdev->dev, sizeof(struct dw_mci), 
> + GFP_KERNEL);

sizeof(*)

> +     if (!host)
> +             return -ENOMEM;
> +
> +     host->irq = platform_get_irq(pdev, 0);
> +     if (host->irq < 0)
> +             return host->irq;
> +
> +     host->drv_data = drv_data;
> +     host->pdev = pdev;
> +     host->dev = &pdev->dev;
> +     host->irq_flags = 0;
> +     host->pdata = pdev->dev.platform_data;
> +
> +     regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +     host->regs = devm_ioremap_resource(&pdev->dev, regs);

Use helper for this.

Open existing, recent drivers and use the them as template or some set of patterns. Sorry, but upstreaming your vendor code will be painful, because you started from some old, buggier version.

> +     if (IS_ERR(host->regs))
> +             return PTR_ERR(host->regs);
> +
> +     /* Get registers' physical base address */
> +     host->phy_regs = regs->start;
> +
> +     platform_set_drvdata(pdev, host);
> +
> +     return dw_mci_cqe_probe(host);
> +}
> +
> +static int dw_mci_rtk_probe(struct platform_device *pdev) {
> +     const struct dw_mci_drv_data *drv_data;
> +     const struct of_device_id *match;
> +
> +     if (!pdev->dev.of_node)
> +             return -ENODEV;
> +
> +     match = of_match_node(dw_mci_rtk_match, pdev->dev.of_node);
> +     drv_data = match->data;
> +
> +     return dw_mci_cqe_pltfm_register(pdev, drv_data); }
> +
> +int dw_mci_rtk_remove(struct platform_device *pdev) {
> +     struct dw_mci *host = platform_get_drvdata(pdev);
> +
> +     dw_mci_cqe_remove(host);
> +     return 0;


Best regards,
Krzysztof





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux