Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752178AbdHHLDu (ORCPT ); Tue, 8 Aug 2017 07:03:50 -0400 Received: from mail-qt0-f171.google.com ([209.85.216.171]:34361 "EHLO mail-qt0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752035AbdHHLDr (ORCPT ); Tue, 8 Aug 2017 07:03:47 -0400 MIME-Version: 1.0 In-Reply-To: <20170802031742.60363-2-liwei213@huawei.com> References: <20170802031742.60363-1-liwei213@huawei.com> <20170802031742.60363-2-liwei213@huawei.com> From: Ulf Hansson Date: Tue, 8 Aug 2017 13:03:46 +0200 Message-ID: Subject: Re: [PATCH v8 2/2] mmc: dw_mmc-k3: add sd support for hi3660 To: Li Wei Cc: Adrian Hunter , Jaehoon Chung , Shawn Lin , Wolfram Sang , Heiner Kallweit , "linux-mmc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Guodong Xu Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4122 Lines: 123 On 2 August 2017 at 05:17, Li Wei wrote: > Add sd card support for hi3660 soc > > Signed-off-by: Li Wei > Signed-off-by: Chen Jun > > 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