Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
To:     broonie@kernel.org
Cc:     gregkh@linuxfoundation.org, rafael@kernel.org,
        linux-kernel@vger.kernel.org
Subject: [PATCH v2 3/4] regmap-irq: Minor adjustments to .handle_mask_sync()
Date:   Thu, 11 May 2023 10:13:41 +0100
Message-Id: <20230511091342.26604-4-aidanmacdonald.0x0@gmail.com>
In-Reply-To: <20230511091342.26604-1-aidanmacdonald.0x0@gmail.com>
References: <20230511091342.26604-1-aidanmacdonald.0x0@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

If a .handle_mask_sync() callback is provided it supersedes all
inbuilt handling of mask registers, and judging by the commit
69af4bcaa08d ("regmap-irq: Add handle_mask_sync() callback") it
is intended to completely replace all default IRQ masking logic.

The implementation has two minor inconsistencies, which can be
fixed without breaking compatibility:

(1) mask_base must be set to enable .handle_mask_sync(), even
    though mask_base is otherwise unused. This is easily fixed
    because mask_base is already optional.

(2) Unmask registers aren't accounted for -- they are part of
    the default IRQ masking logic and are just a bit-inverted
    version of mask registers. It would be a bad idea to allow
    them to be used at the same time as .handle_mask_sync(),
    as the result would be confusing and unmaintainable, so
    make sure this can't happen.

Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 drivers/base/regmap/regmap-irq.c | 65 +++++++++++++++-----------------
 1 file changed, 31 insertions(+), 34 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 7cb457af2332..95bf457204bf 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -113,23 +113,21 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 	 * suppress pointless writes.
 	 */
 	for (i = 0; i < d->chip->num_regs; i++) {
-		if (d->mask_base) {
-			if (d->chip->handle_mask_sync)
-				d->chip->handle_mask_sync(i, d->mask_buf_def[i],
-							  d->mask_buf[i],
-							  d->chip->irq_drv_data);
-			else {
-				reg = d->get_irq_reg(d, d->mask_base, i);
-				ret = regmap_update_bits(d->map, reg,
-						d->mask_buf_def[i],
-						d->mask_buf[i]);
-				if (ret)
-					dev_err(d->map->dev, "Failed to sync masks in %x\n",
-						reg);
-			}
+		if (d->chip->handle_mask_sync)
+			d->chip->handle_mask_sync(i, d->mask_buf_def[i],
+						  d->mask_buf[i],
+						  d->chip->irq_drv_data);
+
+		if (d->mask_base && !d->chip->handle_mask_sync) {
+			reg = d->get_irq_reg(d, d->mask_base, i);
+			ret = regmap_update_bits(d->map, reg,
+						 d->mask_buf_def[i],
+						 d->mask_buf[i]);
+			if (ret)
+				dev_err(d->map->dev, "Failed to sync masks in %x\n", reg);
 		}
 
-		if (d->unmask_base) {
+		if (d->unmask_base && !d->chip->handle_mask_sync) {
 			reg = d->get_irq_reg(d, d->unmask_base, i);
 			ret = regmap_update_bits(d->map, reg,
 					d->mask_buf_def[i], ~d->mask_buf[i]);
@@ -785,28 +783,27 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	for (i = 0; i < chip->num_regs; i++) {
 		d->mask_buf[i] = d->mask_buf_def[i];
 
-		if (d->mask_base) {
-			if (chip->handle_mask_sync) {
-				ret = chip->handle_mask_sync(i,
-							     d->mask_buf_def[i],
-							     d->mask_buf[i],
-							     chip->irq_drv_data);
-				if (ret)
-					goto err_alloc;
-			} else {
-				reg = d->get_irq_reg(d, d->mask_base, i);
-				ret = regmap_update_bits(d->map, reg,
-						d->mask_buf_def[i],
-						d->mask_buf[i]);
-				if (ret) {
-					dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
-						reg, ret);
-					goto err_alloc;
-				}
+		if (chip->handle_mask_sync) {
+			ret = chip->handle_mask_sync(i, d->mask_buf_def[i],
+						     d->mask_buf[i],
+						     chip->irq_drv_data);
+			if (ret)
+				goto err_alloc;
+		}
+
+		if (d->mask_base && !chip->handle_mask_sync) {
+			reg = d->get_irq_reg(d, d->mask_base, i);
+			ret = regmap_update_bits(d->map, reg,
+						 d->mask_buf_def[i],
+						 d->mask_buf[i]);
+			if (ret) {
+				dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
+					reg, ret);
+				goto err_alloc;
 			}
 		}
 
-		if (d->unmask_base) {
+		if (d->unmask_base && !chip->handle_mask_sync) {
 			reg = d->get_irq_reg(d, d->unmask_base, i);
 			ret = regmap_update_bits(d->map, reg,
 					d->mask_buf_def[i], ~d->mask_buf[i]);
-- 
2.39.2