Thread (19 messages) 19 messages, 4 authors, 2025-11-08

RE: [PATCH v21 3/4] i2c: ast2600: Add controller driver for new register layout

From: Ryan Chen <ryan_chen@aspeedtech.com>
Date: 2025-11-07 06:26:45
Also in: linux-aspeed, linux-devicetree, linux-i2c, lkml, openbmc

Subject: Re: [PATCH v21 3/4] i2c: ast2600: Add controller driver for new
register layout

On 27/10/2025 07:12, Ryan Chen wrote:
quoted
Add i2c-ast2600 new register mode driver to support AST2600 i2c new
register mode. This i2c-ast2600 new driver and the legacy i2c-aspeed
driver both match the same compatible string "aspeed,ast2600-i2c-bus"
because they target the same I2C controller IP on AST2600. However,
AST2600 SoCs may configure
Where did you document ABI compatibility of new driver with old DTS, which
do not have required properties and uses old reg?

I don't see that. I actually clearly see:
quoted
+	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);
+
+	rst = devm_reset_control_get_shared_deasserted(dev, NULL);
+	if (IS_ERR(rst))
+		return dev_err_probe(dev, PTR_ERR(rst), "Missing reset ctrl\n");
+
+	i2c_bus->global_regs =
+		syscon_regmap_lookup_by_phandle(dev_of_node(dev),
"aspeed,global-regs");
quoted
+	if (IS_ERR(i2c_bus->global_regs))
+		return PTR_ERR(i2c_bus->global_regs);

ABI break. No explanation of that ABI break, not even mentioning it.

There is no reason to break the ABI. Your new driver *must* support both
interfaces and both DTB. You *cannot* (please listen carefully, because in the
past we wasted a lot of time discussing simple statements) rely on legacy
driver binding to old DTB, because it is the same compatible.

You need to rewrite all this to handle both DTBs in backwards compatible way.

NAK
I think I understand your point about ABI break.
I will try to merge i2c-ast260.c with legacy i2c-aspeed.c
Add new file i2c-aspeed-core.c to do legacy probe and i2c-ast2600 probe.

	if (of_device_is_compatible(dev_of_node(dev), "aspeed,ast2600-i2c-bus") &&
	    of_parse_phandle(dev_of_node(dev), "aspeed,global-regs", 0)) {
		ret = ast2600_i2c_probe(pdev);
	} else {
		ret = aspeed_i2c_probe(pdev);
	}

That would not have ABI break.
Best regards,
Krzysztof
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help