Re: [PATCH 11/30] staging: mt7621-mmc: Replace macro sdr_get_field with function
On Mon, Apr 30, 2018 at 08:33:07AM +1000, NeilBrown wrote: > On Sun, Apr 29 2018, Christian Lütke-Stetzkamp wrote: > > > Currently sdr_get_field is a macro, to bring the code in line with the > > upstream driver mtk-sd, it is changed to a function. > > > > Signed-off-by: Christian Lütke-Stetzkamp > > --- > > drivers/staging/mt7621-mmc/mt6575_sd.h | 10 +- > > drivers/staging/mt7621-mmc/sd.c| 14 -- > > 2 files changed, 13 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/staging/mt7621-mmc/mt6575_sd.h > > b/drivers/staging/mt7621-mmc/mt6575_sd.h > > index 90b4ee6b396b..195c7333f77d 100644 > > --- a/drivers/staging/mt7621-mmc/mt6575_sd.h > > +++ b/drivers/staging/mt7621-mmc/mt6575_sd.h > > @@ -980,10 +980,10 @@ static inline void sdr_set_field(void __iomem *reg, > > u32 field, u32 val) > > writel(tv, reg); > > } > > > > -#define sdr_get_field(reg, field, val) > > \ > > -do { \ > > - volatile unsigned int tv = sdr_read32(reg); \ > > - val = ((tv & (field)) >> (ffs((unsigned int)field) - 1)); \ > > -} while (0) > > +static inline void sdr_get_field(void __iomem *reg, u32 field, u32 *val) > > +{ > > + unsigned int tv = readl(reg); > > + *val = ((tv & field) >> (ffs((unsigned int)field) - 1)); > > +} > > Converting this to a function it good, but it should return val rather > than set *val. Yes, I completely agree, that your proposal is easier to understand, again the reason for me to choose the current version is the current code in the mtk-sd driver. > > > > #endif > > diff --git a/drivers/staging/mt7621-mmc/sd.c > > b/drivers/staging/mt7621-mmc/sd.c > > index 00dacf8c9625..f4da48a5772b 100644 > > --- a/drivers/staging/mt7621-mmc/sd.c > > +++ b/drivers/staging/mt7621-mmc/sd.c > > @@ -1577,8 +1577,8 @@ static int msdc_tune_cmdrsp(struct msdc_host *host, > > struct mmc_command *cmd) > >==*/ > > > > // save the previous tune result > > - sdr_get_field(MSDC_IOCON,MSDC_IOCON_RSPL,orig_rsmpl); > > - sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_CMDRRDLY, orig_rrdly); > > + sdr_get_field(MSDC_IOCON,MSDC_IOCON_RSPL,&orig_rsmpl); > > + sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_CMDRRDLY, &orig_rrdly); > > So this becomes > > > + orig_rsmpl = sdr_get_field(MSDC_IOCON,MSDC_IOCON_RSPL); > > + orig_rrdly = sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_CMDRRDLY); > > which is much easier to understand at first glance. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 11/30] staging: mt7621-mmc: Replace macro sdr_get_field with function
On Sun, Apr 29 2018, Christian Lütke-Stetzkamp wrote: > Currently sdr_get_field is a macro, to bring the code in line with the > upstream driver mtk-sd, it is changed to a function. > > Signed-off-by: Christian Lütke-Stetzkamp > --- > drivers/staging/mt7621-mmc/mt6575_sd.h | 10 +- > drivers/staging/mt7621-mmc/sd.c| 14 -- > 2 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/staging/mt7621-mmc/mt6575_sd.h > b/drivers/staging/mt7621-mmc/mt6575_sd.h > index 90b4ee6b396b..195c7333f77d 100644 > --- a/drivers/staging/mt7621-mmc/mt6575_sd.h > +++ b/drivers/staging/mt7621-mmc/mt6575_sd.h > @@ -980,10 +980,10 @@ static inline void sdr_set_field(void __iomem *reg, u32 > field, u32 val) > writel(tv, reg); > } > > -#define sdr_get_field(reg, field, val) > \ > -do { \ > - volatile unsigned int tv = sdr_read32(reg); \ > - val = ((tv & (field)) >> (ffs((unsigned int)field) - 1)); \ > -} while (0) > +static inline void sdr_get_field(void __iomem *reg, u32 field, u32 *val) > +{ > + unsigned int tv = readl(reg); > + *val = ((tv & field) >> (ffs((unsigned int)field) - 1)); > +} Converting this to a function it good, but it should return val rather than set *val. > > #endif > diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c > index 00dacf8c9625..f4da48a5772b 100644 > --- a/drivers/staging/mt7621-mmc/sd.c > +++ b/drivers/staging/mt7621-mmc/sd.c > @@ -1577,8 +1577,8 @@ static int msdc_tune_cmdrsp(struct msdc_host *host, > struct mmc_command *cmd) > ==*/ > > // save the previous tune result > - sdr_get_field(MSDC_IOCON,MSDC_IOCON_RSPL,orig_rsmpl); > - sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_CMDRRDLY, orig_rrdly); > + sdr_get_field(MSDC_IOCON,MSDC_IOCON_RSPL,&orig_rsmpl); > + sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_CMDRRDLY, &orig_rrdly); So this becomes > + orig_rsmpl = sdr_get_field(MSDC_IOCON,MSDC_IOCON_RSPL); > + orig_rrdly = sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_CMDRRDLY); which is much easier to understand at first glance. Thanks, NeilBrown > > rrdly = 0; > do { > @@ -1640,7 +1640,7 @@ static int msdc_tune_bread(struct mmc_host *mmc, struct > mmc_request *mrq) > int result = -1; > u32 skip = 1; > > - sdr_get_field(MSDC_IOCON, MSDC_IOCON_DSPL, orig_dsmpl); > + sdr_get_field(MSDC_IOCON, MSDC_IOCON_DSPL, &orig_dsmpl); > > /* Tune Method 2. */ > sdr_set_field(MSDC_IOCON, MSDC_IOCON_DDLSEL, 1); > @@ -1664,7 +1664,9 @@ static int msdc_tune_bread(struct mmc_host *mmc, struct > mmc_request *mrq) > } > result = msdc_do_request(mmc, mrq); > > - sdr_get_field(SDC_DCRC_STS, SDC_DCRC_STS_POS | > SDC_DCRC_STS_NEG, dcrc); /* RO */ > + sdr_get_field(SDC_DCRC_STS, > + SDC_DCRC_STS_POS | SDC_DCRC_STS_NEG, > + &dcrc); /* RO */ > if (!ddr) > dcrc &= ~SDC_DCRC_STS_NEG; > ERR_MSG("TUNE_BREAD<%s> dcrc<0x%x> > DATRDDLY0/1<0x%x><0x%x> dsmpl<0x%x>", > @@ -1751,8 +1753,8 @@ static int msdc_tune_bwrite(struct mmc_host *mmc, > struct mmc_request *mrq) > > // MSDC_IOCON_DDR50CKD need to check. [Fix me] > > - sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_DATWRDLY, orig_wrrdly); > - sdr_get_field(MSDC_IOCON,MSDC_IOCON_DSPL,orig_dsmpl); > + sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_DATWRDLY, &orig_wrrdly); > + sdr_get_field(MSDC_IOCON,MSDC_IOCON_DSPL,&orig_dsmpl); > > /* Tune Method 2. just DAT0 */ > sdr_set_field(MSDC_IOCON, MSDC_IOCON_DDLSEL, 1); > -- > 2.16.1 signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 11/30] staging: mt7621-mmc: Replace macro sdr_get_field with function
Currently sdr_get_field is a macro, to bring the code in line with the upstream driver mtk-sd, it is changed to a function. Signed-off-by: Christian Lütke-Stetzkamp --- drivers/staging/mt7621-mmc/mt6575_sd.h | 10 +- drivers/staging/mt7621-mmc/sd.c| 14 -- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/drivers/staging/mt7621-mmc/mt6575_sd.h b/drivers/staging/mt7621-mmc/mt6575_sd.h index 90b4ee6b396b..195c7333f77d 100644 --- a/drivers/staging/mt7621-mmc/mt6575_sd.h +++ b/drivers/staging/mt7621-mmc/mt6575_sd.h @@ -980,10 +980,10 @@ static inline void sdr_set_field(void __iomem *reg, u32 field, u32 val) writel(tv, reg); } -#define sdr_get_field(reg, field, val) \ -do { \ - volatile unsigned int tv = sdr_read32(reg); \ - val = ((tv & (field)) >> (ffs((unsigned int)field) - 1)); \ -} while (0) +static inline void sdr_get_field(void __iomem *reg, u32 field, u32 *val) +{ + unsigned int tv = readl(reg); + *val = ((tv & field) >> (ffs((unsigned int)field) - 1)); +} #endif diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c index 00dacf8c9625..f4da48a5772b 100644 --- a/drivers/staging/mt7621-mmc/sd.c +++ b/drivers/staging/mt7621-mmc/sd.c @@ -1577,8 +1577,8 @@ static int msdc_tune_cmdrsp(struct msdc_host *host, struct mmc_command *cmd) ==*/ // save the previous tune result - sdr_get_field(MSDC_IOCON,MSDC_IOCON_RSPL,orig_rsmpl); - sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_CMDRRDLY, orig_rrdly); + sdr_get_field(MSDC_IOCON,MSDC_IOCON_RSPL,&orig_rsmpl); + sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_CMDRRDLY, &orig_rrdly); rrdly = 0; do { @@ -1640,7 +1640,7 @@ static int msdc_tune_bread(struct mmc_host *mmc, struct mmc_request *mrq) int result = -1; u32 skip = 1; - sdr_get_field(MSDC_IOCON, MSDC_IOCON_DSPL, orig_dsmpl); + sdr_get_field(MSDC_IOCON, MSDC_IOCON_DSPL, &orig_dsmpl); /* Tune Method 2. */ sdr_set_field(MSDC_IOCON, MSDC_IOCON_DDLSEL, 1); @@ -1664,7 +1664,9 @@ static int msdc_tune_bread(struct mmc_host *mmc, struct mmc_request *mrq) } result = msdc_do_request(mmc, mrq); - sdr_get_field(SDC_DCRC_STS, SDC_DCRC_STS_POS | SDC_DCRC_STS_NEG, dcrc); /* RO */ + sdr_get_field(SDC_DCRC_STS, + SDC_DCRC_STS_POS | SDC_DCRC_STS_NEG, + &dcrc); /* RO */ if (!ddr) dcrc &= ~SDC_DCRC_STS_NEG; ERR_MSG("TUNE_BREAD<%s> dcrc<0x%x> DATRDDLY0/1<0x%x><0x%x> dsmpl<0x%x>", @@ -1751,8 +1753,8 @@ static int msdc_tune_bwrite(struct mmc_host *mmc, struct mmc_request *mrq) // MSDC_IOCON_DDR50CKD need to check. [Fix me] - sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_DATWRDLY, orig_wrrdly); - sdr_get_field(MSDC_IOCON,MSDC_IOCON_DSPL,orig_dsmpl); + sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_DATWRDLY, &orig_wrrdly); + sdr_get_field(MSDC_IOCON,MSDC_IOCON_DSPL,&orig_dsmpl); /* Tune Method 2. just DAT0 */ sdr_set_field(MSDC_IOCON, MSDC_IOCON_DDLSEL, 1); -- 2.16.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel