From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
To: Matthias Brugger <matthias.bgg@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
nfraprado@collabora.com, rex-bc.chen@mediatek.com,
zhiyong.tao@mediatek.com
Subject: Re: [PATCH v3 1/5] soc: mediatek: pwrap: Use readx_poll_timeout() instead of custom function
Date: Tue, 17 May 2022 11:41:28 +0200 [thread overview]
Message-ID: <9f197c69-fe04-b636-afb7-8474763c8a3a@collabora.com> (raw)
In-Reply-To: <f8acbc75-970c-62fb-ad0d-914e512104a7@gmail.com>
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!
Cheers,
Angelo
next prev parent reply other threads:[~2022-05-17 9:42 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-16 12:46 [PATCH v3 0/5] MediaTek PMIC Wrap improvements and cleanups AngeloGioacchino Del Regno
2022-05-16 12:46 ` [PATCH v3 1/5] soc: mediatek: pwrap: Use readx_poll_timeout() instead of custom function AngeloGioacchino Del Regno
2022-05-17 9:25 ` Matthias Brugger
2022-05-17 9:41 ` AngeloGioacchino Del Regno [this message]
2022-05-17 9:44 ` Matthias Brugger
2022-05-16 12:46 ` [PATCH v3 2/5] soc: mediatek: pwrap: Switch to devm_platform_ioremap_resource_byname() AngeloGioacchino Del Regno
2022-05-16 12:46 ` [PATCH v3 3/5] soc: mediatek: pwrap: Move and check return value of platform_get_irq() AngeloGioacchino Del Regno
2022-05-17 9:18 ` Matthias Brugger
2022-05-17 9:34 ` AngeloGioacchino Del Regno
2022-05-17 9:49 ` Matthias Brugger
2022-05-17 10:35 ` AngeloGioacchino Del Regno
2022-05-16 12:46 ` [PATCH v3 4/5] soc: mediatek: pwrap: Move IO pointers to new structure AngeloGioacchino Del Regno
2022-05-16 12:46 ` [PATCH v3 5/5] soc: mediatek: pwrap: Compress of_device_id entries to one line AngeloGioacchino Del Regno
2022-05-17 9:23 ` Matthias Brugger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9f197c69-fe04-b636-afb7-8474763c8a3a@collabora.com \
--to=angelogioacchino.delregno@collabora.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=nfraprado@collabora.com \
--cc=rex-bc.chen@mediatek.com \
--cc=zhiyong.tao@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).