Hi, Ulf Thank you very much for your advice. 1. Version history is really great information, however it doesn't belong inside the change log itsefl. Instead add "---" and a newline here, then put it all below that. 【LiWei】OK，I will fix it;
2. We have an API, mmc_regulator_set_vqmmc(), that you probably should use here instead. 【LiWei】Yes, you are right.I use mmc_regulator_set_vqmmc() instead and test OK, I will modify it in patch v9; 3. Overall the code looks okay to me, however I am wondering about the relationship with the original k3 driver code and hi6220 code. Almost no code is being re-used between the different variants. Why is that? 【LiWei】So far, it seems that dw_mci_hi3660_set_ios() and dw_mci_hi3660_switch_voltage() are not re-used, I think the main reason is that Hi3660 support uhs-sdr12/-uhs-sdr25/uhs-sdr50/uhs-sdr104, On the other hand, we have custom register settings, such as: #define AO_SCTRL_SEL18 BIT(10) #define AO_SCTRL_CTRL3 0x40C #define SOC_SCTRL_SCPERCTRL5 (0x314) #define SDCARD_IO_SEL18 BIT(2) So we can't merge relevant code. Please help review patch v9, thank you! -----邮件原件----- 发件人: Ulf Hansson [mailto:ulf.hans...@linaro.org] 发送时间: 2017年8月8日 19:04 收件人: liwei (CM) 抄送: Adrian Hunter; Jaehoon Chung; Shawn Lin; Wolfram Sang; Heiner Kallweit; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Guodong Xu 主题: Re: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660 On 2 August 2017 at 05:17, Li Wei <liwei...@huawei.com> wrote: > Add sd card support for hi3660 soc > > Signed-off-by: Li Wei <liwei...@huawei.com> > Signed-off-by: Chen Jun <chenju...@huawei.com> > > Major changes in v3: > - solve review comments from Heiner Kallweit. > *use the GENMASK and FIELD_PREP macros replace the bit shift operation. > *use usleep_range() replace udelay() and mdelay(). > > Major changes in v4: > - solve review comments from Jaehoon Chung. > *move common register for dwmmc controller to dwmmc header file. > *modify definitions type of some register variables. > *get rid of the magic numbers. > > Major changes in v5: > - further improve coding style. > > Major changes in v6: > - solve review comments for Jaehoon Chung. > *modify dw_mci_hi3660_set_ios() to static. > *fix the comment style. > > Major changes in v7: > - solve review comments for John Stultz. > *remove reset code in dw_mmc-k3.c,use reset in core mmc. > > Major changes in v8: > - modify patch v7 name and dependency order. Version history is really great information, however it doesn't belong inside the change log itsefl. Instead add "---" and a newline here, then put it all below that. [...] > +static int dw_mci_hi3660_switch_voltage(struct mmc_host *mmc, > + struct mmc_ios *ios) { > + int ret; > + int min_uv = 0; > + int max_uv = 0; > + struct dw_mci_slot *slot = mmc_priv(mmc); > + struct k3_priv *priv; > + struct dw_mci *host; > + > + host = slot->host; > + priv = host->priv; > + > + if (!priv || !priv->reg) > + return 0; > + > + if (priv->ctrl_id == DWMMC_SDIO_ID) > + return 0; > + > + if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) { > + ret = dw_mci_set_sel18(host, 0); > + if (ret) > + return ret; > + min_uv = 2950000; > + max_uv = 2950000; > + } else if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) { > + ret = dw_mci_set_sel18(host, 1); > + if (ret) > + return ret; > + min_uv = 1800000; > + max_uv = 1800000; > + } > + > + if (IS_ERR_OR_NULL(mmc->supply.vqmmc)) > + return 0; > + > + ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, > + max_uv); We have an API, mmc_regulator_set_vqmmc(), that you probably should use here instead. > + if (ret) { > + dev_err(host->dev, "Regulator set error %d: %d - %d\n", > + ret, min_uv, max_uv); > + return ret; > + } > + return 0; > +} > + > +static const struct dw_mci_drv_data hi3660_data = { > + .init = dw_mci_hi3660_init, > + .set_ios = dw_mci_hi3660_set_ios, > + .parse_dt = dw_mci_hi6220_parse_dt, > + .execute_tuning = dw_mci_hi3660_execute_tuning, > + .switch_voltage = dw_mci_hi3660_switch_voltage, }; > + > static const struct of_device_id dw_mci_k3_match[] = { > + { .compatible = "hisilicon,hi3660-dw-mshc", .data = > + &hi3660_data, }, > { .compatible = "hisilicon,hi4511-dw-mshc", .data = &k3_drv_data, }, > { .compatible = "hisilicon,hi6220-dw-mshc", .data = &hi6220_data, }, > {}, > diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h > index 75da3756955d..5403758bf621 100644 > --- a/drivers/mmc/host/dw_mmc.h > +++ b/drivers/mmc/host/dw_mmc.h > @@ -314,6 +314,8 @@ struct dw_mci_board { > #define SDMMC_DSCADDR 0x094 > #define SDMMC_BUFADDR 0x098 > #define SDMMC_CDTHRCTL 0x100 > +#define SDMMC_UHS_REG_EXT 0x108 > +#define SDMMC_ENABLE_SHIFT 0x110 > #define SDMMC_DATA(x) (x) > /* > * Registers to support idmac 64-bit address mode > -- > 2.11.0 > Overall the code looks okay to me, however I am wondering about the relationship with the original k3 driver code and hi6220 code. Almost no code is being re-used between the different variants. Why is that? Kind regards Uffe
