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

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

 



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 <christian@xxxxxxxx>
> > ---
> >  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
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux