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
