Messages in this thread | | | Date | Tue, 17 May 2022 11:44:12 +0200 | Subject | Re: [PATCH v3 1/5] soc: mediatek: pwrap: Use readx_poll_timeout() instead of custom function | From | 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@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
| |