[PATCH v3 0/5] MediaTek PMIC Wrap improvements and cleanups

[PATCH v3 0/5] MediaTek PMIC Wrap improvements and cleanups

[AngeloGioacchino Del Regno]

This series is meant to improve the mtk-pmic-wrap driver;
that's done by removing the custom pwrap_wait_for_state() function
and correctly using the readx_poll_timeout macro instead, which is
doing exactly the same as the former.

As also shown in a patch [1] by Zhiyong Tao (MediaTek), performing
a tight loop is not desired: because of the operation timing in the
SPMI PMICs on these platforms, it makes more sense to wait for some
microseconds before trying to read again, reducing CPU busy time
around these state waits. For this purpose, a ~10uS delay was chosen.

While at it, I also took the occasion to tidy up this driver a
little by optimizing its probe() function.

[1]: https://patchwork.kernel.org/project/linux-mediatek/patch/20220329115824.13005-2-zhiyong.tao@mediatek.com/

Changes in v3:
 - Added two more cleanup patches

Changes in v2:
 - Fixed a critical typo in patch 1/3. Thanks Nicolas!

AngeloGioacchino Del Regno (5):
  soc: mediatek: pwrap: Use readx_poll_timeout() instead of custom
    function
  soc: mediatek: pwrap: Switch to
    devm_platform_ioremap_resource_byname()
  soc: mediatek: pwrap: Move and check return value of
    platform_get_irq()
  soc: mediatek: pwrap: Move IO pointers to new structure
  soc: mediatek: pwrap: Compress of_device_id entries to one line

 drivers/soc/mediatek/mtk-pmic-wrap.c | 224 ++++++++++++---------------
 1 file changed, 95 insertions(+), 129 deletions(-)

-- 
2.35.1

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

[AngeloGioacchino Del Regno]

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,
+				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
 	if (ret) {
 		pwrap_leave_fsm_vldclr(wrp);
 		return ret;
@@ -1273,7 +1263,8 @@ static int pwrap_read16(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
 		val = (adr >> 1) << 16;
 	pwrap_writel(wrp, val, PWRAP_WACS2_CMD);
 
-	ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_vldclr);
+	ret = readx_poll_timeout(pwrap_is_fsm_vldclr, wrp, tmp, tmp,
+				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
 	if (ret)
 		return ret;
 
@@ -1290,11 +1281,14 @@ static int pwrap_read16(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
 
 static int pwrap_read32(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
 {
+	bool tmp;
 	int ret, msb;
 
 	*rdata = 0;
 	for (msb = 0; msb < 2; msb++) {
-		ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_idle);
+		ret = readx_poll_timeout(pwrap_is_fsm_idle, wrp, tmp, tmp,
+					 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
+
 		if (ret) {
 			pwrap_leave_fsm_vldclr(wrp);
 			return ret;
@@ -1303,7 +1297,8 @@ static int pwrap_read32(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
 		pwrap_writel(wrp, ((msb << 30) | (adr << 16)),
 			     PWRAP_WACS2_CMD);
 
-		ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_vldclr);
+		ret = readx_poll_timeout(pwrap_is_fsm_vldclr, wrp, tmp, tmp,
+					 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
 		if (ret)
 			return ret;
 
@@ -1323,9 +1318,11 @@ static int pwrap_read(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
 
 static int pwrap_write16(struct pmic_wrapper *wrp, u32 adr, u32 wdata)
 {
+	bool tmp;
 	int ret;
 
-	ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_idle);
+	ret = readx_poll_timeout(pwrap_is_fsm_idle, wrp, tmp, tmp,
+				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
 	if (ret) {
 		pwrap_leave_fsm_vldclr(wrp);
 		return ret;
@@ -1344,10 +1341,12 @@ static int pwrap_write16(struct pmic_wrapper *wrp, u32 adr, u32 wdata)
 
 static int pwrap_write32(struct pmic_wrapper *wrp, u32 adr, u32 wdata)
 {
+	bool tmp;
 	int ret, msb, rdata;
 
 	for (msb = 0; msb < 2; msb++) {
-		ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_idle);
+		ret = readx_poll_timeout(pwrap_is_fsm_idle, wrp, tmp, tmp,
+					 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
 		if (ret) {
 			pwrap_leave_fsm_vldclr(wrp);
 			return ret;
@@ -1388,6 +1387,7 @@ static int pwrap_regmap_write(void *context, u32 adr, u32 wdata)
 
 static int pwrap_reset_spislave(struct pmic_wrapper *wrp)
 {
+	bool tmp;
 	int ret, i;
 
 	pwrap_writel(wrp, 0, PWRAP_HIPRIO_ARB_EN);
@@ -1407,7 +1407,8 @@ static int pwrap_reset_spislave(struct pmic_wrapper *wrp)
 		pwrap_writel(wrp, wrp->master->spi_w | PWRAP_MAN_CMD_OP_OUTS,
 				PWRAP_MAN_CMD);
 
-	ret = pwrap_wait_for_state(wrp, pwrap_is_sync_idle);
+	ret = readx_poll_timeout(pwrap_is_sync_idle, wrp, tmp, tmp,
+				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
 	if (ret) {
 		dev_err(wrp->dev, "%s fail, ret=%d\n", __func__, ret);
 		return ret;
@@ -1458,14 +1459,15 @@ static int pwrap_init_sidly(struct pmic_wrapper *wrp)
 static int pwrap_init_dual_io(struct pmic_wrapper *wrp)
 {
 	int ret;
+	bool tmp;
 	u32 rdata;
 
 	/* Enable dual IO mode */
 	pwrap_write(wrp, wrp->slave->dew_regs[PWRAP_DEW_DIO_EN], 1);
 
 	/* Check IDLE & INIT_DONE in advance */
-	ret = pwrap_wait_for_state(wrp,
-				   pwrap_is_fsm_idle_and_sync_idle);
+	ret = readx_poll_timeout(pwrap_is_fsm_idle_and_sync_idle, wrp, tmp, tmp,
+				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
 	if (ret) {
 		dev_err(wrp->dev, "%s fail, ret=%d\n", __func__, ret);
 		return ret;
@@ -1570,6 +1572,7 @@ static bool pwrap_is_pmic_cipher_ready(struct pmic_wrapper *wrp)
 static int pwrap_init_cipher(struct pmic_wrapper *wrp)
 {
 	int ret;
+	bool tmp;
 	u32 rdata = 0;
 
 	pwrap_writel(wrp, 0x1, PWRAP_CIPHER_SWRST);
@@ -1624,14 +1627,16 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
 	}
 
 	/* wait for cipher data ready@AP */
-	ret = pwrap_wait_for_state(wrp, pwrap_is_cipher_ready);
+	ret = readx_poll_timeout(pwrap_is_cipher_ready, wrp, tmp, tmp,
+				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
 	if (ret) {
 		dev_err(wrp->dev, "cipher data ready@AP fail, ret=%d\n", ret);
 		return ret;
 	}
 
 	/* wait for cipher data ready@PMIC */
-	ret = pwrap_wait_for_state(wrp, pwrap_is_pmic_cipher_ready);
+	ret = readx_poll_timeout(pwrap_is_pmic_cipher_ready, wrp, tmp, tmp,
+				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
 	if (ret) {
 		dev_err(wrp->dev,
 			"timeout waiting for cipher data ready@PMIC\n");
@@ -1640,7 +1645,8 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
 
 	/* wait for cipher mode idle */
 	pwrap_write(wrp, wrp->slave->dew_regs[PWRAP_DEW_CIPHER_MODE], 0x1);
-	ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_idle_and_sync_idle);
+	ret = readx_poll_timeout(pwrap_is_fsm_idle_and_sync_idle, wrp, tmp, tmp,
+				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
 	if (ret) {
 		dev_err(wrp->dev, "cipher mode idle fail, ret=%d\n", ret);
 		return ret;
-- 
2.35.1

[PATCH v3 2/5] soc: mediatek: pwrap: Switch to devm_platform_ioremap_resource_byname()

[AngeloGioacchino Del Regno]

In order to simplify ioremapping resources, instead of calling
platform_get_resource_byname() and then devm_ioremap_resource(),
simply call devm_platform_ioremap_resource_byname().

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 | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
index 54a5300ab72b..852514366f1f 100644
--- a/drivers/soc/mediatek/mtk-pmic-wrap.c
+++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
@@ -2191,7 +2191,6 @@ static int pwrap_probe(struct platform_device *pdev)
 	struct pmic_wrapper *wrp;
 	struct device_node *np = pdev->dev.of_node;
 	const struct of_device_id *of_slave_id = NULL;
-	struct resource *res;
 
 	if (np->child)
 		of_slave_id = of_match_node(of_slave_match_tbl, np->child);
@@ -2211,8 +2210,7 @@ static int pwrap_probe(struct platform_device *pdev)
 	wrp->slave = of_slave_id->data;
 	wrp->dev = &pdev->dev;
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwrap");
-	wrp->base = devm_ioremap_resource(wrp->dev, res);
+	wrp->base = devm_platform_ioremap_resource_byname(pdev, "pwrap");
 	if (IS_ERR(wrp->base))
 		return PTR_ERR(wrp->base);
 
@@ -2226,9 +2224,7 @@ static int pwrap_probe(struct platform_device *pdev)
 	}
 
 	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_BRIDGE)) {
-		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-				"pwrap-bridge");
-		wrp->bridge_base = devm_ioremap_resource(wrp->dev, res);
+		wrp->bridge_base = devm_platform_ioremap_resource_byname(pdev, "pwrap-bridge");
 		if (IS_ERR(wrp->bridge_base))
 			return PTR_ERR(wrp->bridge_base);
 
-- 
2.35.1

[PATCH v3 3/5] soc: mediatek: pwrap: Move and check return value of platform_get_irq()

[AngeloGioacchino Del Regno]

Move the call to platform_get_irq() earlier in the probe function
and check for its return value: if no interrupt is specified, it
wouldn't make sense to try to call devm_request_irq() so, in that
case, we can simply return early.

Moving the platform_get_irq() call also makes it possible to use
one less goto, as clocks aren't required at that stage.

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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
index 852514366f1f..332cbcabc299 100644
--- a/drivers/soc/mediatek/mtk-pmic-wrap.c
+++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
@@ -2204,6 +2204,10 @@ static int pwrap_probe(struct platform_device *pdev)
 	if (!wrp)
 		return -ENOMEM;
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
 	platform_set_drvdata(pdev, wrp);
 
 	wrp->master = of_device_get_match_data(&pdev->dev);
@@ -2316,7 +2320,6 @@ static int pwrap_probe(struct platform_device *pdev)
 	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
 		pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
 
-	irq = platform_get_irq(pdev, 0);
 	ret = devm_request_irq(wrp->dev, irq, pwrap_interrupt,
 			       IRQF_TRIGGER_HIGH,
 			       "mt-pmic-pwrap", wrp);
-- 
2.35.1

[PATCH v3 4/5] soc: mediatek: pwrap: Move IO pointers to new structure

[AngeloGioacchino Del Regno]

In the PMIC Wrapper driver each PMIC has its own regmap configuration
and its own pwrap_{read/write}() callbacks, but it's just about either
a 32 bits vs 16 bits register, and only one of them uses 32bits regs:
this means that the same ops are assigned over and over again to all
of the supported PMICs.

It is therefore possible to avoid reassigning the same things over
and over, reducing the amount of lines, without any impact on human
readability of this driver: add a pwrap_slv_regops structure and
move the callbacks and regmap_config pointer in there instead.
This allows to assign just one pointer to that shared data in the
per-pmic struct pwrap_slv_type.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/soc/mediatek/mtk-pmic-wrap.c | 61 +++++++++++++++-------------
 1 file changed, 32 insertions(+), 29 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
index 332cbcabc299..7d02e1d4faf4 100644
--- a/drivers/soc/mediatek/mtk-pmic-wrap.c
+++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
@@ -1143,12 +1143,9 @@ enum pwrap_type {
 };
 
 struct pmic_wrapper;
-struct pwrap_slv_type {
-	const u32 *dew_regs;
-	enum pmic_type type;
+
+struct pwrap_slv_regops {
 	const struct regmap_config *regmap;
-	/* Flags indicating the capability for the target slave */
-	u32 caps;
 	/*
 	 * pwrap operations are highly associated with the PMIC types,
 	 * so the pointers added increases flexibility allowing determination
@@ -1158,6 +1155,14 @@ struct pwrap_slv_type {
 	int (*pwrap_write)(struct pmic_wrapper *wrp, u32 adr, u32 wdata);
 };
 
+struct pwrap_slv_type {
+	const u32 *dew_regs;
+	enum pmic_type type;
+	const struct pwrap_slv_regops *regops;
+	/* Flags indicating the capability for the target slave */
+	u32 caps;
+};
+
 struct pmic_wrapper {
 	struct device *dev;
 	void __iomem *base;
@@ -1313,7 +1318,7 @@ static int pwrap_read32(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
 
 static int pwrap_read(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
 {
-	return wrp->slave->pwrap_read(wrp, adr, rdata);
+	return wrp->slave->regops->pwrap_read(wrp, adr, rdata);
 }
 
 static int pwrap_write16(struct pmic_wrapper *wrp, u32 adr, u32 wdata)
@@ -1372,7 +1377,7 @@ static int pwrap_write32(struct pmic_wrapper *wrp, u32 adr, u32 wdata)
 
 static int pwrap_write(struct pmic_wrapper *wrp, u32 adr, u32 wdata)
 {
-	return wrp->slave->pwrap_write(wrp, adr, wdata);
+	return wrp->slave->regops->pwrap_write(wrp, adr, wdata);
 }
 
 static int pwrap_regmap_read(void *context, u32 adr, u32 *rdata)
@@ -1891,69 +1896,67 @@ static const struct regmap_config pwrap_regmap_config32 = {
 	.max_register = 0xffff,
 };
 
+static const struct pwrap_slv_regops pwrap_regops16 = {
+	.pwrap_read = pwrap_read16,
+	.pwrap_write = pwrap_write16,
+	.regmap = &pwrap_regmap_config16,
+};
+
+static const struct pwrap_slv_regops pwrap_regops32 = {
+	.pwrap_read = pwrap_read32,
+	.pwrap_write = pwrap_write32,
+	.regmap = &pwrap_regmap_config32,
+};
+
 static const struct pwrap_slv_type pmic_mt6323 = {
 	.dew_regs = mt6323_regs,
 	.type = PMIC_MT6323,
-	.regmap = &pwrap_regmap_config16,
+	.regops = &pwrap_regops16,
 	.caps = PWRAP_SLV_CAP_SPI | PWRAP_SLV_CAP_DUALIO |
 		PWRAP_SLV_CAP_SECURITY,
-	.pwrap_read = pwrap_read16,
-	.pwrap_write = pwrap_write16,
 };
 
 static const struct pwrap_slv_type pmic_mt6351 = {
 	.dew_regs = mt6351_regs,
 	.type = PMIC_MT6351,
-	.regmap = &pwrap_regmap_config16,
+	.regops = &pwrap_regops16,
 	.caps = 0,
-	.pwrap_read = pwrap_read16,
-	.pwrap_write = pwrap_write16,
 };
 
 static const struct pwrap_slv_type pmic_mt6357 = {
 	.dew_regs = mt6357_regs,
 	.type = PMIC_MT6357,
-	.regmap = &pwrap_regmap_config16,
+	.regops = &pwrap_regops16,
 	.caps = 0,
-	.pwrap_read = pwrap_read16,
-	.pwrap_write = pwrap_write16,
 };
 
 static const struct pwrap_slv_type pmic_mt6358 = {
 	.dew_regs = mt6358_regs,
 	.type = PMIC_MT6358,
-	.regmap = &pwrap_regmap_config16,
+	.regops = &pwrap_regops16,
 	.caps = PWRAP_SLV_CAP_SPI | PWRAP_SLV_CAP_DUALIO,
-	.pwrap_read = pwrap_read16,
-	.pwrap_write = pwrap_write16,
 };
 
 static const struct pwrap_slv_type pmic_mt6359 = {
 	.dew_regs = mt6359_regs,
 	.type = PMIC_MT6359,
-	.regmap = &pwrap_regmap_config16,
+	.regops = &pwrap_regops16,
 	.caps = PWRAP_SLV_CAP_DUALIO,
-	.pwrap_read = pwrap_read16,
-	.pwrap_write = pwrap_write16,
 };
 
 static const struct pwrap_slv_type pmic_mt6380 = {
 	.dew_regs = NULL,
 	.type = PMIC_MT6380,
-	.regmap = &pwrap_regmap_config32,
+	.regops = &pwrap_regops32,
 	.caps = 0,
-	.pwrap_read = pwrap_read32,
-	.pwrap_write = pwrap_write32,
 };
 
 static const struct pwrap_slv_type pmic_mt6397 = {
 	.dew_regs = mt6397_regs,
 	.type = PMIC_MT6397,
-	.regmap = &pwrap_regmap_config16,
+	.regops = &pwrap_regops16,
 	.caps = PWRAP_SLV_CAP_SPI | PWRAP_SLV_CAP_DUALIO |
 		PWRAP_SLV_CAP_SECURITY,
-	.pwrap_read = pwrap_read16,
-	.pwrap_write = pwrap_write16,
 };
 
 static const struct of_device_id of_slave_match_tbl[] = {
@@ -2326,7 +2329,7 @@ static int pwrap_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_out2;
 
-	wrp->regmap = devm_regmap_init(wrp->dev, NULL, wrp, wrp->slave->regmap);
+	wrp->regmap = devm_regmap_init(wrp->dev, NULL, wrp, wrp->slave->regops->regmap);
 	if (IS_ERR(wrp->regmap)) {
 		ret = PTR_ERR(wrp->regmap);
 		goto err_out2;
-- 
2.35.1

[PATCH v3 5/5] soc: mediatek: pwrap: Compress of_device_id entries to one line

[AngeloGioacchino Del Regno]

Compress each entry of the of_device_id tables in this driver to one
line instead of three, as they fit just fine in a single line.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/soc/mediatek/mtk-pmic-wrap.c | 90 ++++++++--------------------
 1 file changed, 24 insertions(+), 66 deletions(-)

diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
index 7d02e1d4faf4..c018092e6cbe 100644
--- a/drivers/soc/mediatek/mtk-pmic-wrap.c
+++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
@@ -1960,33 +1960,17 @@ static const struct pwrap_slv_type pmic_mt6397 = {
 };
 
 static const struct of_device_id of_slave_match_tbl[] = {
-	{
-		.compatible = "mediatek,mt6323",
-		.data = &pmic_mt6323,
-	}, {
-		.compatible = "mediatek,mt6351",
-		.data = &pmic_mt6351,
-	}, {
-		.compatible = "mediatek,mt6357",
-		.data = &pmic_mt6357,
-	}, {
-		.compatible = "mediatek,mt6358",
-		.data = &pmic_mt6358,
-	}, {
-		.compatible = "mediatek,mt6359",
-		.data = &pmic_mt6359,
-	}, {
-		/* The MT6380 PMIC only implements a regulator, so we bind it
-		 * directly instead of using a MFD.
-		 */
-		.compatible = "mediatek,mt6380-regulator",
-		.data = &pmic_mt6380,
-	}, {
-		.compatible = "mediatek,mt6397",
-		.data = &pmic_mt6397,
-	}, {
-		/* sentinel */
-	}
+	{ .compatible = "mediatek,mt6323", .data = &pmic_mt6323 },
+	{ .compatible = "mediatek,mt6351", .data = &pmic_mt6351 },
+	{ .compatible = "mediatek,mt6357", .data = &pmic_mt6357 },
+	{ .compatible = "mediatek,mt6358", .data = &pmic_mt6358 },
+	{ .compatible = "mediatek,mt6359", .data = &pmic_mt6359 },
+	/* The MT6380 PMIC only implements a regulator, so we bind it
+	 * directly instead of using a MFD.
+	 */
+	{ .compatible = "mediatek,mt6380-regulator", .data = &pmic_mt6380 },
+	{ .compatible = "mediatek,mt6397", .data = &pmic_mt6397 },
+	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, of_slave_match_tbl);
 
@@ -2145,45 +2129,19 @@ static struct pmic_wrapper_type pwrap_mt8186 = {
 };
 
 static const struct of_device_id of_pwrap_match_tbl[] = {
-	{
-		.compatible = "mediatek,mt2701-pwrap",
-		.data = &pwrap_mt2701,
-	}, {
-		.compatible = "mediatek,mt6765-pwrap",
-		.data = &pwrap_mt6765,
-	}, {
-		.compatible = "mediatek,mt6779-pwrap",
-		.data = &pwrap_mt6779,
-	}, {
-		.compatible = "mediatek,mt6797-pwrap",
-		.data = &pwrap_mt6797,
-	}, {
-		.compatible = "mediatek,mt6873-pwrap",
-		.data = &pwrap_mt6873,
-	}, {
-		.compatible = "mediatek,mt7622-pwrap",
-		.data = &pwrap_mt7622,
-	}, {
-		.compatible = "mediatek,mt8135-pwrap",
-		.data = &pwrap_mt8135,
-	}, {
-		.compatible = "mediatek,mt8173-pwrap",
-		.data = &pwrap_mt8173,
-	}, {
-		.compatible = "mediatek,mt8183-pwrap",
-		.data = &pwrap_mt8183,
-	}, {
-		.compatible = "mediatek,mt8186-pwrap",
-		.data = &pwrap_mt8186,
-	}, {
-		.compatible = "mediatek,mt8195-pwrap",
-		.data = &pwrap_mt8195,
-	}, {
-		.compatible = "mediatek,mt8516-pwrap",
-		.data = &pwrap_mt8516,
-	}, {
-		/* sentinel */
-	}
+	{ .compatible = "mediatek,mt2701-pwrap", .data = &pwrap_mt2701 },
+	{ .compatible = "mediatek,mt6765-pwrap", .data = &pwrap_mt6765 },
+	{ .compatible = "mediatek,mt6779-pwrap", .data = &pwrap_mt6779 },
+	{ .compatible = "mediatek,mt6797-pwrap", .data = &pwrap_mt6797 },
+	{ .compatible = "mediatek,mt6873-pwrap", .data = &pwrap_mt6873 },
+	{ .compatible = "mediatek,mt7622-pwrap", .data = &pwrap_mt7622 },
+	{ .compatible = "mediatek,mt8135-pwrap", .data = &pwrap_mt8135 },
+	{ .compatible = "mediatek,mt8173-pwrap", .data = &pwrap_mt8173 },
+	{ .compatible = "mediatek,mt8183-pwrap", .data = &pwrap_mt8183 },
+	{ .compatible = "mediatek,mt8186-pwrap", .data = &pwrap_mt8186 },
+	{ .compatible = "mediatek,mt8195-pwrap", .data = &pwrap_mt8195 },
+	{ .compatible = "mediatek,mt8516-pwrap", .data = &pwrap_mt8516 },
+	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, of_pwrap_match_tbl);
 
-- 
2.35.1

Re: [PATCH v3 3/5] soc: mediatek: pwrap: Move and check return value of platform_get_irq()

[Matthias Brugger]

On 16/05/2022 14:46, AngeloGioacchino Del Regno wrote:
> Move the call to platform_get_irq() earlier in the probe function
> and check for its return value: if no interrupt is specified, it
> wouldn't make sense to try to call devm_request_irq() so, in that
> case, we can simply return early.
> 
> Moving the platform_get_irq() call also makes it possible to use
> one less goto, as clocks aren't required at that stage.
> 
> 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 | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 852514366f1f..332cbcabc299 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -2204,6 +2204,10 @@ static int pwrap_probe(struct platform_device *pdev)
>   	if (!wrp)
>   		return -ENOMEM;
>   
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
>   	platform_set_drvdata(pdev, wrp);
>   
>   	wrp->master = of_device_get_match_data(&pdev->dev);
> @@ -2316,7 +2320,6 @@ static int pwrap_probe(struct platform_device *pdev)
>   	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
>   		pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
>   
> -	irq = platform_get_irq(pdev, 0);

For better readability of the code I'd prefer to keep platform_get_irq next to 
devm_request_irq. I understand that you did this change so that you don't have 
to code
if (irq < 0) {
     ret = irq;
     goto err_out2;
}

Or do I miss something?

Regards,
Matthias

>   	ret = devm_request_irq(wrp->dev, irq, pwrap_interrupt,
>   			       IRQF_TRIGGER_HIGH,
>   			       "mt-pmic-pwrap", wrp);

Re: [PATCH v3 5/5] soc: mediatek: pwrap: Compress of_device_id entries to one line

[Matthias Brugger]

On 16/05/2022 14:46, AngeloGioacchino Del Regno wrote:
> Compress each entry of the of_device_id tables in this driver to one
> line instead of three, as they fit just fine in a single line.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/soc/mediatek/mtk-pmic-wrap.c | 90 ++++++++--------------------
>   1 file changed, 24 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 7d02e1d4faf4..c018092e6cbe 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -1960,33 +1960,17 @@ static const struct pwrap_slv_type pmic_mt6397 = {
>   };
>   
>   static const struct of_device_id of_slave_match_tbl[] = {
> -	{
> -		.compatible = "mediatek,mt6323",
> -		.data = &pmic_mt6323,
> -	}, {
> -		.compatible = "mediatek,mt6351",
> -		.data = &pmic_mt6351,
> -	}, {
> -		.compatible = "mediatek,mt6357",
> -		.data = &pmic_mt6357,
> -	}, {
> -		.compatible = "mediatek,mt6358",
> -		.data = &pmic_mt6358,
> -	}, {
> -		.compatible = "mediatek,mt6359",
> -		.data = &pmic_mt6359,
> -	}, {
> -		/* The MT6380 PMIC only implements a regulator, so we bind it
> -		 * directly instead of using a MFD.
> -		 */
> -		.compatible = "mediatek,mt6380-regulator",
> -		.data = &pmic_mt6380,
> -	}, {
> -		.compatible = "mediatek,mt6397",
> -		.data = &pmic_mt6397,
> -	}, {
> -		/* sentinel */
> -	}
> +	{ .compatible = "mediatek,mt6323", .data = &pmic_mt6323 },
> +	{ .compatible = "mediatek,mt6351", .data = &pmic_mt6351 },
> +	{ .compatible = "mediatek,mt6357", .data = &pmic_mt6357 },
> +	{ .compatible = "mediatek,mt6358", .data = &pmic_mt6358 },
> +	{ .compatible = "mediatek,mt6359", .data = &pmic_mt6359 },
> +	/* The MT6380 PMIC only implements a regulator, so we bind it

Nit-pick alert: new line here for readability.

Regards,
Matthias

> +	 * directly instead of using a MFD.
> +	 */
> +	{ .compatible = "mediatek,mt6380-regulator", .data = &pmic_mt6380 },
> +	{ .compatible = "mediatek,mt6397", .data = &pmic_mt6397 },
> +	{ /* sentinel */ }
>   };
>   MODULE_DEVICE_TABLE(of, of_slave_match_tbl);
>   
> @@ -2145,45 +2129,19 @@ static struct pmic_wrapper_type pwrap_mt8186 = {
>   };
>   
>   static const struct of_device_id of_pwrap_match_tbl[] = {
> -	{
> -		.compatible = "mediatek,mt2701-pwrap",
> -		.data = &pwrap_mt2701,
> -	}, {
> -		.compatible = "mediatek,mt6765-pwrap",
> -		.data = &pwrap_mt6765,
> -	}, {
> -		.compatible = "mediatek,mt6779-pwrap",
> -		.data = &pwrap_mt6779,
> -	}, {
> -		.compatible = "mediatek,mt6797-pwrap",
> -		.data = &pwrap_mt6797,
> -	}, {
> -		.compatible = "mediatek,mt6873-pwrap",
> -		.data = &pwrap_mt6873,
> -	}, {
> -		.compatible = "mediatek,mt7622-pwrap",
> -		.data = &pwrap_mt7622,
> -	}, {
> -		.compatible = "mediatek,mt8135-pwrap",
> -		.data = &pwrap_mt8135,
> -	}, {
> -		.compatible = "mediatek,mt8173-pwrap",
> -		.data = &pwrap_mt8173,
> -	}, {
> -		.compatible = "mediatek,mt8183-pwrap",
> -		.data = &pwrap_mt8183,
> -	}, {
> -		.compatible = "mediatek,mt8186-pwrap",
> -		.data = &pwrap_mt8186,
> -	}, {
> -		.compatible = "mediatek,mt8195-pwrap",
> -		.data = &pwrap_mt8195,
> -	}, {
> -		.compatible = "mediatek,mt8516-pwrap",
> -		.data = &pwrap_mt8516,
> -	}, {
> -		/* sentinel */
> -	}
> +	{ .compatible = "mediatek,mt2701-pwrap", .data = &pwrap_mt2701 },
> +	{ .compatible = "mediatek,mt6765-pwrap", .data = &pwrap_mt6765 },
> +	{ .compatible = "mediatek,mt6779-pwrap", .data = &pwrap_mt6779 },
> +	{ .compatible = "mediatek,mt6797-pwrap", .data = &pwrap_mt6797 },
> +	{ .compatible = "mediatek,mt6873-pwrap", .data = &pwrap_mt6873 },
> +	{ .compatible = "mediatek,mt7622-pwrap", .data = &pwrap_mt7622 },
> +	{ .compatible = "mediatek,mt8135-pwrap", .data = &pwrap_mt8135 },
> +	{ .compatible = "mediatek,mt8173-pwrap", .data = &pwrap_mt8173 },
> +	{ .compatible = "mediatek,mt8183-pwrap", .data = &pwrap_mt8183 },
> +	{ .compatible = "mediatek,mt8186-pwrap", .data = &pwrap_mt8186 },
> +	{ .compatible = "mediatek,mt8195-pwrap", .data = &pwrap_mt8195 },
> +	{ .compatible = "mediatek,mt8516-pwrap", .data = &pwrap_mt8516 },
> +	{ /* sentinel */ }
>   };
>   MODULE_DEVICE_TABLE(of, of_pwrap_match_tbl);
>   

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

[Matthias Brugger]

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.

Regards,
Matthias

> +				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
>   	if (ret) {
>   		pwrap_leave_fsm_vldclr(wrp);
>   		return ret;
> @@ -1273,7 +1263,8 @@ static int pwrap_read16(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
>   		val = (adr >> 1) << 16;
>   	pwrap_writel(wrp, val, PWRAP_WACS2_CMD);
>   
> -	ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_vldclr);
> +	ret = readx_poll_timeout(pwrap_is_fsm_vldclr, wrp, tmp, tmp,
> +				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
>   	if (ret)
>   		return ret;
>   
> @@ -1290,11 +1281,14 @@ static int pwrap_read16(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
>   
>   static int pwrap_read32(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
>   {
> +	bool tmp;
>   	int ret, msb;
>   
>   	*rdata = 0;
>   	for (msb = 0; msb < 2; msb++) {
> -		ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_idle);
> +		ret = readx_poll_timeout(pwrap_is_fsm_idle, wrp, tmp, tmp,
> +					 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
> +
>   		if (ret) {
>   			pwrap_leave_fsm_vldclr(wrp);
>   			return ret;
> @@ -1303,7 +1297,8 @@ static int pwrap_read32(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
>   		pwrap_writel(wrp, ((msb << 30) | (adr << 16)),
>   			     PWRAP_WACS2_CMD);
>   
> -		ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_vldclr);
> +		ret = readx_poll_timeout(pwrap_is_fsm_vldclr, wrp, tmp, tmp,
> +					 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
>   		if (ret)
>   			return ret;
>   
> @@ -1323,9 +1318,11 @@ static int pwrap_read(struct pmic_wrapper *wrp, u32 adr, u32 *rdata)
>   
>   static int pwrap_write16(struct pmic_wrapper *wrp, u32 adr, u32 wdata)
>   {
> +	bool tmp;
>   	int ret;
>   
> -	ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_idle);
> +	ret = readx_poll_timeout(pwrap_is_fsm_idle, wrp, tmp, tmp,
> +				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
>   	if (ret) {
>   		pwrap_leave_fsm_vldclr(wrp);
>   		return ret;
> @@ -1344,10 +1341,12 @@ static int pwrap_write16(struct pmic_wrapper *wrp, u32 adr, u32 wdata)
>   
>   static int pwrap_write32(struct pmic_wrapper *wrp, u32 adr, u32 wdata)
>   {
> +	bool tmp;
>   	int ret, msb, rdata;
>   
>   	for (msb = 0; msb < 2; msb++) {
> -		ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_idle);
> +		ret = readx_poll_timeout(pwrap_is_fsm_idle, wrp, tmp, tmp,
> +					 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
>   		if (ret) {
>   			pwrap_leave_fsm_vldclr(wrp);
>   			return ret;
> @@ -1388,6 +1387,7 @@ static int pwrap_regmap_write(void *context, u32 adr, u32 wdata)
>   
>   static int pwrap_reset_spislave(struct pmic_wrapper *wrp)
>   {
> +	bool tmp;
>   	int ret, i;
>   
>   	pwrap_writel(wrp, 0, PWRAP_HIPRIO_ARB_EN);
> @@ -1407,7 +1407,8 @@ static int pwrap_reset_spislave(struct pmic_wrapper *wrp)
>   		pwrap_writel(wrp, wrp->master->spi_w | PWRAP_MAN_CMD_OP_OUTS,
>   				PWRAP_MAN_CMD);
>   
> -	ret = pwrap_wait_for_state(wrp, pwrap_is_sync_idle);
> +	ret = readx_poll_timeout(pwrap_is_sync_idle, wrp, tmp, tmp,
> +				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
>   	if (ret) {
>   		dev_err(wrp->dev, "%s fail, ret=%d\n", __func__, ret);
>   		return ret;
> @@ -1458,14 +1459,15 @@ static int pwrap_init_sidly(struct pmic_wrapper *wrp)
>   static int pwrap_init_dual_io(struct pmic_wrapper *wrp)
>   {
>   	int ret;
> +	bool tmp;
>   	u32 rdata;
>   
>   	/* Enable dual IO mode */
>   	pwrap_write(wrp, wrp->slave->dew_regs[PWRAP_DEW_DIO_EN], 1);
>   
>   	/* Check IDLE & INIT_DONE in advance */
> -	ret = pwrap_wait_for_state(wrp,
> -				   pwrap_is_fsm_idle_and_sync_idle);
> +	ret = readx_poll_timeout(pwrap_is_fsm_idle_and_sync_idle, wrp, tmp, tmp,
> +				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
>   	if (ret) {
>   		dev_err(wrp->dev, "%s fail, ret=%d\n", __func__, ret);
>   		return ret;
> @@ -1570,6 +1572,7 @@ static bool pwrap_is_pmic_cipher_ready(struct pmic_wrapper *wrp)
>   static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>   {
>   	int ret;
> +	bool tmp;
>   	u32 rdata = 0;
>   
>   	pwrap_writel(wrp, 0x1, PWRAP_CIPHER_SWRST);
> @@ -1624,14 +1627,16 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>   	}
>   
>   	/* wait for cipher data ready@AP */
> -	ret = pwrap_wait_for_state(wrp, pwrap_is_cipher_ready);
> +	ret = readx_poll_timeout(pwrap_is_cipher_ready, wrp, tmp, tmp,
> +				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
>   	if (ret) {
>   		dev_err(wrp->dev, "cipher data ready@AP fail, ret=%d\n", ret);
>   		return ret;
>   	}
>   
>   	/* wait for cipher data ready@PMIC */
> -	ret = pwrap_wait_for_state(wrp, pwrap_is_pmic_cipher_ready);
> +	ret = readx_poll_timeout(pwrap_is_pmic_cipher_ready, wrp, tmp, tmp,
> +				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
>   	if (ret) {
>   		dev_err(wrp->dev,
>   			"timeout waiting for cipher data ready@PMIC\n");
> @@ -1640,7 +1645,8 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>   
>   	/* wait for cipher mode idle */
>   	pwrap_write(wrp, wrp->slave->dew_regs[PWRAP_DEW_CIPHER_MODE], 0x1);
> -	ret = pwrap_wait_for_state(wrp, pwrap_is_fsm_idle_and_sync_idle);
> +	ret = readx_poll_timeout(pwrap_is_fsm_idle_and_sync_idle, wrp, tmp, tmp,
> +				 PWRAP_POLL_DELAY_US, PWRAP_POLL_TIMEOUT_US);
>   	if (ret) {
>   		dev_err(wrp->dev, "cipher mode idle fail, ret=%d\n", ret);
>   		return ret;

Re: [PATCH v3 3/5] soc: mediatek: pwrap: Move and check return value of platform_get_irq()

[AngeloGioacchino Del Regno]

Il 17/05/22 11:18, Matthias Brugger ha scritto:
> 
> 
> On 16/05/2022 14:46, AngeloGioacchino Del Regno wrote:
>> Move the call to platform_get_irq() earlier in the probe function
>> and check for its return value: if no interrupt is specified, it
>> wouldn't make sense to try to call devm_request_irq() so, in that
>> case, we can simply return early.
>>
>> Moving the platform_get_irq() call also makes it possible to use
>> one less goto, as clocks aren't required at that stage.
>>
>> 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 | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c 
>> b/drivers/soc/mediatek/mtk-pmic-wrap.c
>> index 852514366f1f..332cbcabc299 100644
>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
>> @@ -2204,6 +2204,10 @@ static int pwrap_probe(struct platform_device *pdev)
>>       if (!wrp)
>>           return -ENOMEM;
>> +    irq = platform_get_irq(pdev, 0);
>> +    if (irq < 0)
>> +        return irq;
>> +
>>       platform_set_drvdata(pdev, wrp);
>>       wrp->master = of_device_get_match_data(&pdev->dev);
>> @@ -2316,7 +2320,6 @@ static int pwrap_probe(struct platform_device *pdev)
>>       if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
>>           pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
>> -    irq = platform_get_irq(pdev, 0);
> 
> For better readability of the code I'd prefer to keep platform_get_irq next to 
> devm_request_irq. I understand that you did this change so that you don't have to code
> if (irq < 0) {
>      ret = irq;
>      goto err_out2;
> }
> 
> Or do I miss something?
> 

That's for the sake of reducing gotos in the code... but there's a bigger
picture that I haven't explained in this commit and that will come later
because I currently don't have the necessary time to perform a "decent"
testing.

As I was explaining - the bigger pictures implies adding a new function for
clock teardown, that we will add as a devm action:

devm_add_action_or_reset(&pdev->dev, pwrap_clk_disable_unprepare, wrp)

...so that we will be able to remove *all* gotos from the probe function.

Sounds good?

Cheers,
Angelo

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

[AngeloGioacchino Del Regno]

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

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@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

Re: [PATCH v3 3/5] soc: mediatek: pwrap: Move and check return value of platform_get_irq()

[Matthias Brugger]

On 17/05/2022 11:34, AngeloGioacchino Del Regno wrote:
> Il 17/05/22 11:18, Matthias Brugger ha scritto:
>>
>>
>> On 16/05/2022 14:46, AngeloGioacchino Del Regno wrote:
>>> Move the call to platform_get_irq() earlier in the probe function
>>> and check for its return value: if no interrupt is specified, it
>>> wouldn't make sense to try to call devm_request_irq() so, in that
>>> case, we can simply return early.
>>>
>>> Moving the platform_get_irq() call also makes it possible to use
>>> one less goto, as clocks aren't required at that stage.
>>>
>>> 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 | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c 
>>> b/drivers/soc/mediatek/mtk-pmic-wrap.c
>>> index 852514366f1f..332cbcabc299 100644
>>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
>>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
>>> @@ -2204,6 +2204,10 @@ static int pwrap_probe(struct platform_device *pdev)
>>>       if (!wrp)
>>>           return -ENOMEM;
>>> +    irq = platform_get_irq(pdev, 0);
>>> +    if (irq < 0)
>>> +        return irq;
>>> +
>>>       platform_set_drvdata(pdev, wrp);
>>>       wrp->master = of_device_get_match_data(&pdev->dev);
>>> @@ -2316,7 +2320,6 @@ static int pwrap_probe(struct platform_device *pdev)
>>>       if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
>>>           pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
>>> -    irq = platform_get_irq(pdev, 0);
>>
>> For better readability of the code I'd prefer to keep platform_get_irq next to 
>> devm_request_irq. I understand that you did this change so that you don't have 
>> to code
>> if (irq < 0) {
>>      ret = irq;
>>      goto err_out2;
>> }
>>
>> Or do I miss something?
>>
> 
> That's for the sake of reducing gotos in the code... but there's a bigger
> picture that I haven't explained in this commit and that will come later
> because I currently don't have the necessary time to perform a "decent"
> testing.
> 
> As I was explaining - the bigger pictures implies adding a new function for
> clock teardown, that we will add as a devm action:
> 
> devm_add_action_or_reset(&pdev->dev, pwrap_clk_disable_unprepare, wrp)
> 
> ...so that we will be able to remove *all* gotos from the probe function.
> 
> Sounds good?
> 

Sounds good, but that means we could get rid of the goto as well. Anyway I 
prefer to have platform_get_irq next to devm_request_irq. If we can get rid of 
the goto in the future, great.

Regards,
Matthias

Re: [PATCH v3 3/5] soc: mediatek: pwrap: Move and check return value of platform_get_irq()

[AngeloGioacchino Del Regno]

Il 17/05/22 11:49, Matthias Brugger ha scritto:
> 
> 
> On 17/05/2022 11:34, AngeloGioacchino Del Regno wrote:
>> Il 17/05/22 11:18, Matthias Brugger ha scritto:
>>>
>>>
>>> On 16/05/2022 14:46, AngeloGioacchino Del Regno wrote:
>>>> Move the call to platform_get_irq() earlier in the probe function
>>>> and check for its return value: if no interrupt is specified, it
>>>> wouldn't make sense to try to call devm_request_irq() so, in that
>>>> case, we can simply return early.
>>>>
>>>> Moving the platform_get_irq() call also makes it possible to use
>>>> one less goto, as clocks aren't required at that stage.
>>>>
>>>> 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 | 5 ++++-
>>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c 
>>>> b/drivers/soc/mediatek/mtk-pmic-wrap.c
>>>> index 852514366f1f..332cbcabc299 100644
>>>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
>>>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
>>>> @@ -2204,6 +2204,10 @@ static int pwrap_probe(struct platform_device *pdev)
>>>>       if (!wrp)
>>>>           return -ENOMEM;
>>>> +    irq = platform_get_irq(pdev, 0);
>>>> +    if (irq < 0)
>>>> +        return irq;
>>>> +
>>>>       platform_set_drvdata(pdev, wrp);
>>>>       wrp->master = of_device_get_match_data(&pdev->dev);
>>>> @@ -2316,7 +2320,6 @@ static int pwrap_probe(struct platform_device *pdev)
>>>>       if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
>>>>           pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
>>>> -    irq = platform_get_irq(pdev, 0);
>>>
>>> For better readability of the code I'd prefer to keep platform_get_irq next to 
>>> devm_request_irq. I understand that you did this change so that you don't have 
>>> to code
>>> if (irq < 0) {
>>>      ret = irq;
>>>      goto err_out2;
>>> }
>>>
>>> Or do I miss something?
>>>
>>
>> That's for the sake of reducing gotos in the code... but there's a bigger
>> picture that I haven't explained in this commit and that will come later
>> because I currently don't have the necessary time to perform a "decent"
>> testing.
>>
>> As I was explaining - the bigger pictures implies adding a new function for
>> clock teardown, that we will add as a devm action:
>>
>> devm_add_action_or_reset(&pdev->dev, pwrap_clk_disable_unprepare, wrp)
>>
>> ...so that we will be able to remove *all* gotos from the probe function.
>>
>> Sounds good?
>>
> 
> Sounds good, but that means we could get rid of the goto as well. Anyway I prefer 
> to have platform_get_irq next to devm_request_irq. If we can get rid of the goto in 
> the future, great.

Oki, then I'll send a v4 and avoid to move that one elsewhere - will keep the goto
as well.

Looking back at it, effectively, it doesn't really make sense to move that call!

Cheers,
Angelo