Re: [PATCH v3 1/5] soc: mediatek: pwrap: Use readx_poll_timeout() instead of custom function

[Matthias Brugger]

On 17/05/2022 11:41, AngeloGioacchino Del Regno wrote:
> Il 17/05/22 11:25, Matthias Brugger ha scritto:
> > On 16/05/2022 14:46, AngeloGioacchino Del Regno wrote:
> > > Function pwrap_wait_for_state() is a function that polls an address
> > > through a helper function, but this is the very same operation that
> > > the readx_poll_timeout macro means to do.
> > > Convert all instances of calling pwrap_wait_for_state() to instead
> > > use the read_poll_timeout macro.
> > >
> > > Signed-off-by: AngeloGioacchino Del Regno 
> > > <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
> > > Tested-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
> > > ---
> > >   drivers/soc/mediatek/mtk-pmic-wrap.c | 60 +++++++++++++++-------------
> > >   1 file changed, 33 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c 
> > > b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > > index bf39a64f3ecc..54a5300ab72b 100644
> > > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> > > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > > @@ -13,6 +13,9 @@
> > >   #include <linux/regmap.h>
> > >   #include <linux/reset.h>
> > > +#define PWRAP_POLL_DELAY_US    10
> > > +#define PWRAP_POLL_TIMEOUT_US    10000
> > > +
> > >   #define PWRAP_MT8135_BRIDGE_IORD_ARB_EN        0x4
> > >   #define PWRAP_MT8135_BRIDGE_WACS3_EN        0x10
> > >   #define PWRAP_MT8135_BRIDGE_INIT_DONE3        0x14
> > > @@ -1241,27 +1244,14 @@ static bool pwrap_is_fsm_idle_and_sync_idle(struct 
> > > pmic_wrapper *wrp)
> > >           (val & PWRAP_STATE_SYNC_IDLE0);
> > >   }
> > > -static int pwrap_wait_for_state(struct pmic_wrapper *wrp,
> > > -        bool (*fp)(struct pmic_wrapper *))
> > > -{
> > > -    unsigned long timeout;
> > > -
> > > -    timeout = jiffies + usecs_to_jiffies(10000);
> > > -
> > > -    do {
> > > -        if (time_after(jiffies, timeout))
> > > -            return fp(wrp) ? 0 : -ETIMEDOUT;
> > > -        if (fp(wrp))
> > > -            return 0;
> > > -    } while (1);
> > > -}
> > > -
> > >   static int pwrap_read16(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
> > >   {
> > > +    bool tmp;
> > >       int ret;
> > >       u32 val;
> > > -    ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_idle);
> > > +    ret = readx_poll_timeout(pwrap_is_fsm_idle, wrp, tmp, tmp,
> >
> > hm, if we make the cond (tmp > 0) that would help to understand the code. At 
> > least I had to think about it for a moment. But I leave it to you if you think 
> > it's worth the effort.
>
> I would prefer size over readability in this case... if we do (tmp > 0), it would
> be incorrect to keep tmp as a `bool`, we would have to set it as an integer var,
> which is unnecessarily bigger (that's the reason why I wrote it like so!).
>
> Another way to increase human readability would be to do (tmp == true), but it
> looks a bit weird to me, doesn't it?
> If you disagree about that looking weird, though, I can go with that one, perhaps!

You are right, just leave it as it is.

Regards,
Matthias