Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device compatible

-----Original Message-----
From: Philipp Zabel <p.zabel@pengutronix.de>
Sent: Friday, February 14, 2025 3:54 AM
To: Wilson Ding <redacted>; linux-kernel@vger.kernel.org;
devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
Cc: andrew@lunn.ch; gregory.clement@bootlin.com;
sebastian.hesselbarth@gmail.com; robh@kernel.org; krzk+dt@kernel.org;
conor+dt@kernel.org; Sanghoon Lee [off-list ref]; Geethasowjanya
Akula [off-list ref]
Subject: [EXTERNAL] Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon
device compatible

On Do, 2025-02-13 at 22:58 -0800, Wilson Ding wrote:

quoted

Introduce the new ops for updating reset line and getting status.
Thus, the reset controller can be accessed through either direct I/O
or regmap interfaces.

Please don't add a new layer of function pointer indirection, just add a new
struct reset_control_ops for the regmap variant.

If just adding a new struct reset_control_ops for the regmap variant, almost
all the functions will be duplicated for regmap variant. 
Besides reset_simple_regmap_assert/deassert(), we also need to have the
regmap version of reset_simple_update(). Since reset_simple_reset() invokes
reset_simple_regmap_assert/deassert(), it also needs to be duplicated.
In this case, there will be too many redundant codes in this file. I doubt if
it is worth to use the reset simple code. Maybe it's better to fork a new file
for the syscon device, such as 'reset-simple-syscon.c'. What do you say?

quoted

It enables the support of the syscon devices with the simple reset
code. To adapt the DT binding of the syscon device, the number of
reset lines must be specified in device data.

If the DT node had a reg property, number of reset lines could be determined
from its size, like for MMIO.

quoted

Signed-off-by: Wilson Ding <redacted>
---
 drivers/reset/reset-simple.c       | 117 +++++++++++++++++++++++------
 include/linux/reset/reset-simple.h |  11 +++
 2 files changed, 107 insertions(+), 21 deletions(-)

diff --git a/drivers/reset/reset-simple.c

b/drivers/reset/reset-simple.c index 276067839830..e4e777d40a79

100644

quoted

--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c

@@ -15,8 +15,10 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/reset-controller.h>
 #include <linux/reset/reset-simple.h>  #include <linux/spinlock.h> @@
-27,10 +29,9 @@ to_reset_simple_data(struct reset_controller_dev *rcdev)
 	return container_of(rcdev, struct reset_simple_data, rcdev);  }

-static int reset_simple_update(struct reset_controller_dev *rcdev,
+static int reset_simple_update_mmio(struct reset_simple_data *data,
 			       unsigned long id, bool assert)

No need to rename or change the function prototype.

quoted

 {
-	struct reset_simple_data *data = to_reset_simple_data(rcdev);
 	int reg_width = sizeof(u32);
 	int bank = id / (reg_width * BITS_PER_BYTE);
 	int offset = id % (reg_width * BITS_PER_BYTE); @@ -51,16 +52,40

@@

quoted

static int reset_simple_update(struct reset_controller_dev *rcdev,
 	return 0;
 }

+static int reset_simple_update_regmap(struct reset_simple_data *data,
+				      unsigned long id, bool assert)

I'd call this reset_simple_regmap_update().

quoted

+{
+	int reg_width = sizeof(u32);
+	int bank = id / (reg_width * BITS_PER_BYTE);
+	int offset = id % (reg_width * BITS_PER_BYTE);
+	u32 mask, val;
+
+	mask = BIT(offset);
+
+	if (assert ^ data->active_low)
+		val = mask;
+	else
+		val = 0;
+
+	return regmap_write_bits(data->regmap,
+				 data->reg_offset + (bank * reg_width),
+				 mask, val);
+}
+
 static int reset_simple_assert(struct reset_controller_dev *rcdev,
 			       unsigned long id)
 {
-	return reset_simple_update(rcdev, id, true);
+	struct reset_simple_data *data = to_reset_simple_data(rcdev);
+
+	return data->ops.update(data, id, true);
 }

 static int reset_simple_deassert(struct reset_controller_dev *rcdev,
 				 unsigned long id)
 {
-	return reset_simple_update(rcdev, id, false);
+	struct reset_simple_data *data = to_reset_simple_data(rcdev);
+
+	return data->ops.update(data, id, false);
 }

No need for indirection. Better to just add separate
reset_simple_regmap_assert/deassert() functions.

See my reply to the first comment.

quoted

 static int reset_simple_reset(struct reset_controller_dev *rcdev, @@
-81,10 +106,9 @@ static int reset_simple_reset(struct reset_controller_dev

*rcdev,

quoted

 	return reset_simple_deassert(rcdev, id);  }

-static int reset_simple_status(struct reset_controller_dev *rcdev,
-			       unsigned long id)
+static int reset_simple_status_mmio(struct reset_simple_data *data,
+			     unsigned long id)
 {
-	struct reset_simple_data *data = to_reset_simple_data(rcdev);
 	int reg_width = sizeof(u32);
 	int bank = id / (reg_width * BITS_PER_BYTE);
 	int offset = id % (reg_width * BITS_PER_BYTE); @@ -95,6 +119,31

@@

quoted

static int reset_simple_status(struct reset_controller_dev *rcdev,
 	return !(reg & BIT(offset)) ^ !data->status_active_low;  }

+static int reset_simple_status_regmap(struct reset_simple_data *data,
+				    unsigned long id)
+{
+	int reg_width = sizeof(u32);
+	int bank = id / (reg_width * BITS_PER_BYTE);
+	int offset = id % (reg_width * BITS_PER_BYTE);
+	u32 reg;
+	int ret;
+
+	ret = regmap_read(data->regmap, data->reg_offset + (bank *

reg_width),

quoted

+			  &reg);
+	if (ret)
+		return ret;
+
+	return !(reg & BIT(offset)) ^ !data->status_active_low; }
+
+static int reset_simple_status(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct reset_simple_data *data = to_reset_simple_data(rcdev);
+
+	return data->ops.status(data, id);
+}

Same as above, no need for indirection.
Just add separate reset_simple_regmap_assert/deassert() functions ...

See my reply to the first comment.

quoted

+
 const struct reset_control_ops reset_simple_ops = {
 	.assert		= reset_simple_assert,
 	.deassert	= reset_simple_deassert,

... and add a const struct reset_control_ops reset_simple_regmap_ops.

See my reply to the first comment.

regards
Philipp

- Wilson