Re: [PATCH 11/30] staging: mt7621-mmc: Replace macro sdr_get_field with function

2018-04-30 Thread Christian Lütke-Stetzkamp 
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,_rsmpl);
> > +   sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_CMDRRDLY, _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

2018-04-29 Thread NeilBrown 
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,_rsmpl);
> + sdr_get_field(MSDC_PAD_TUNE, MSDC_PAD_TUNE_CMDRRDLY, _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, _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,
> +   ); /* 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, _wrrdly);
> + sdr_get_field(MSDC_IOCON,MSDC_IOCON_DSPL,_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