On Mo, 2025-02-24 at 09:04 +0000, Ryan Chen wrote:

quoted

Subject: Re: [PATCH v16 2/3] i2c: aspeed: support AST2600 i2c new
register
mode driver

On Mo, 2025-02-24 at 13:59 +0800, Ryan Chen wrote:

quoted

Add i2c new register mode driver to support AST2600 i2c new
register
mode. AST2600 i2c controller have legacy and new register mode.
The
new register mode have global register support 4 base clock for
scl
clock selection, and new clock divider mode. The new register
mode
have separate register set to control i2c controller and target.
This
patch is for i2c controller mode driver.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 drivers/i2c/busses/Kconfig       |   11 +
 drivers/i2c/busses/Makefile      |    1 +
 drivers/i2c/busses/i2c-ast2600.c | 1036
++++++++++++++++++++++++++++++
 3 files changed, 1048 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ast2600.c

[...]

quoted

diff --git a/drivers/i2c/busses/i2c-ast2600.c

b/drivers/i2c/busses/i2c-ast2600.c
new file mode 100644
index 000000000000..bfac507693dd

--- /dev/null
+++ b/drivers/i2c/busses/i2c-ast2600.c

@@ -0,0 +1,1036 @@

[...]

quoted

+static int ast2600_i2c_probe(struct platform_device *pdev) {
+	struct device *dev = &pdev->dev;
+	struct ast2600_i2c_bus *i2c_bus;
+	struct resource *res;
+	u32 global_ctrl;
+	int ret;
+
+	i2c_bus = devm_kzalloc(dev, sizeof(*i2c_bus),
GFP_KERNEL);
+	if (!i2c_bus)
+		return -ENOMEM;
+
+	i2c_bus->reg_base = devm_platform_ioremap_resource(pdev,
0);
+	if (IS_ERR(i2c_bus->reg_base))
+		return PTR_ERR(i2c_bus->reg_base);
+
+	i2c_bus->rst = devm_reset_control_get_shared(dev, NULL);
+	if (IS_ERR(i2c_bus->rst))
+		return dev_err_probe(dev, PTR_ERR(i2c_bus->rst),
"Missing reset
+ctrl\n");
+
+	reset_control_deassert(i2c_bus->rst);

No reset_control_assert() in the error paths below? You could get
that and
simplify this by using devm_reset_control_get_shared_deasserted().

Sorry, devm_reset_control_get_shared_deasserted is new for me.
Do you mean modify by following 

i2c_bus->rst = devm_reset_control_get_shared_deasserted(dev, NULL);
if (IS_ERR(i2c_bus->rst))
    return dev_err_probe(dev, PTR_ERR(i2c_bus->rst), "Missing reset
ctrl\n");

- reset_control_deassert(i2c_bus->rst);

Yes see [1]. Remove the reset_control_assert() in ast2600_i2c_remove()
as well.

[1] https://docs.kernel.org/driver-api/reset.html#c.devm_reset_control_get_shared_deasserted

regards
Philipp