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

[Matthias Brugger <matthias.bgg@gmail.com>]

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@collabora.com>
>>> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>>> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>>> ---
>>>   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