RE: [PATCH] mmc: fsl_esdhc_imx: add wait_dat0() support

[Bough Chen]

> -----Original Message-----
> From: Peng Fan
> Sent: 2020年11月3日 16:17
> To: Jaehoon Chung <jh80.ch...@samsung.com>; Bough Chen
> <haibo.c...@nxp.com>; u-boot@lists.denx.de
> Cc: dl-uboot-imx <uboot-...@nxp.com>
> Subject: RE: [PATCH] mmc: fsl_esdhc_imx: add wait_dat0() support
> 
> > Subject: Re: [PATCH] mmc: fsl_esdhc_imx: add wait_dat0() support
> >
> > On 11/2/20 8:17 PM, haibo.c...@nxp.com wrote:
> > > From: Haibo Chen <haibo.c...@nxp.com>
> > >
> > > Add wait_dat0() support, upper layer will use this callback.
> > >
> > > Signed-off-by: Haibo Chen <haibo.c...@nxp.com>
> > > ---
> > >  drivers/mmc/fsl_esdhc_imx.c | 23 +++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > >
> > > diff --git a/drivers/mmc/fsl_esdhc_imx.c
> > > b/drivers/mmc/fsl_esdhc_imx.c index 22040c67a8..dc6a6006fa 100644
> > > --- a/drivers/mmc/fsl_esdhc_imx.c
> > > +++ b/drivers/mmc/fsl_esdhc_imx.c
> > > @@ -1646,6 +1646,28 @@ static int
> > > fsl_esdhc_set_enhanced_strobe(struct
> > > udevice *dev)  }  #endif
> > >
> > > +static int fsl_esdhc_wait_dat0(struct udevice *dev, int state,
> > > +                         int timeout_us)
> > > +{
> > > + int ret = -ETIMEDOUT;
> > > + bool dat0_high;
> > > + bool target_dat0_high = !!state;
> > > + struct fsl_esdhc_priv *priv = dev_get_priv(dev);
> > > + struct fsl_esdhc *regs = priv->esdhc_regs;
> > > +
> > > + timeout_us = DIV_ROUND_UP(timeout_us, 10); /* check every 10
> us. */
> > > + while (timeout_us--) {
> > > +         dat0_high = !!(esdhc_read32(&regs->prsstat) &
> PRSSTAT_DAT0);
> > > +         if (dat0_high == target_dat0_high) {
> > > +                 ret = 0;
> > > +                 break;
> > > +         }
> > > + udelay(10);
> >
> > Fix indent.
> > And can't use wait_for_bit_xx()?
> 
> +1, read_poll_timeout or similar should be used here.

Do not use read_poll_timeout here is because the parameter 'state' can be 0 or 
1.
if use read_poll_timeout, should like this:
read_poll_timeout(esdhc_read32, &regs->prsstat, tmp, !!(tmp & PRSSTAT_DAT0) == 
!!state, 10, timeout_us);

I think the condition is a little bit complicated, not that readable, but if 
you prefer this, I can do that.

Best Regards
Haibo Chen
> 
> Thanks,
> Peng.
> 
> >
> > Best  Regards,
> > Jaehoon Chung
> >
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > >  static const struct dm_mmc_ops fsl_esdhc_ops = {
> > >   .get_cd         = fsl_esdhc_get_cd,
> > >   .send_cmd       = fsl_esdhc_send_cmd,
> > > @@ -1656,6 +1678,7 @@ static const struct dm_mmc_ops
> fsl_esdhc_ops =
> > {
> > > #if CONFIG_IS_ENABLED(MMC_HS400_ES_SUPPORT)
> > >   .set_enhanced_strobe = fsl_esdhc_set_enhanced_strobe,  #endif
> > > + .wait_dat0 = fsl_esdhc_wait_dat0,
> > >  };
> > >  #endif
> > >
> > >