Re: [PATCH v4 5/8] i2c: i2c-core-of: follow i2c-parent phandle to probe devices from added nodes

["Sverdlin, Alexander" <alexander.sverdlin@siemens.com>]

Hi Luca!

On Tue, 2024-09-17 at 10:53 +0200, Luca Ceresoli wrote:
> When device tree nodes are added, the I2C core tries to probe client
> devices based on the classic DT structure:
> 
>   i2c@abcd0000 {
>       some-client@42 { compatible = "xyz,blah"; ... };
>   };
> 
> However for hotplug connectors described via device tree overlays there is
> additional level of indirection, which is needed to decouple the overlay
> and the base tree:
> 
>   --- base device tree ---
> 
>   i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... };
>   i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... };
> 
>   connector {
>       i2c-ctrl {
>           i2c-parent = <&i2c1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
> 
>       i2c-sensors {
>           i2c-parent = <&i2c5>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
> 
>   --- device tree overlay ---
> 
>   ...
>   // This node will overlay on the i2c-ctrl node of the base tree

Why don't you overlay it right over &i2c1?
It should have worked since commit ea7513bbc041
("i2c/of: Add OF_RECONFIG notifier handler").
Doesn't it work for your use-case?

>   i2c-ctrl {
>       eeprom@50 { compatible = "atmel,24c64"; ... };
>   };
>   ...
> 
>   --- resulting device tree ---
> 
>   i2c1: i2c@abcd0000 { compatible = "xyz,i2c-ctrl"; ... };
>   i2c5: i2c@cafe0000 { compatible = "xyz,i2c-ctrl"; ... };
> 
>   connector {
>       i2c-ctrl {
>           i2c-parent = <&i2c1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
> 
>           eeprom@50 { compatible = "atmel,24c64"; ... };
>       };
> 
>       i2c-sensors {
>           i2c-parent = <&i2c5>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
> 
> Here i2c-ctrl (same goes for i2c-sensors) represent the part of I2C bus
> that is on the hot-pluggable add-on. On hot-plugging it will physically
> connect to the I2C adapter on the base board. Let's call the 'i2c-ctrl'
> node an "extension node".
> 
> In order to decouple the overlay from the base tree, the I2C adapter
> (i2c@abcd0000) and the extension node (i2c-ctrl) are separate
> nodes. Rightfully, only the former will probe into an I2C adapter, and it
> will do that perhaps during boot, long before overlay insertion.
> 
> The extension node won't probe into an I2C adapter or any other device or
> bus, so its subnodes ('eeprom@50') won't be interpreted as I2C clients by
> current I2C core code. However it has an 'i2c-parent' phandle to point to
> the corresponding I2C adapter node. This tells those nodes are I2C clients
> of the adapter in that other node.
> 
> Extend the i2c-core-of code to look for the adapter via the 'i2c-parent'
> phandle when the regular adapter lookup does not find one. This allows all
> clients to be probed: both those on the base board (described in the base
> device tree) and those on the add-on and described by an overlay.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---
> 
> Note: while this patch works for normal hotplug and unplug, it has some
> weaknesses too, due to the implementation being in a OF change
> notifier. Two cases come to mind:
> 
>  1. In the base device tree there must be _no_ nodes under the "extension
>     node" (i2c-ctrl), or they won't be picked up as they are not
>     dynamically added.
> 
>  2. In case the I2C adapter is unbound and rebound, or it probes after
>     overlay insertion, it will miss the OF notifier events and so it won't
>     find the devices in the extension node.
> 
> The first case is not a limiting factor: fixed I2C devices should just stay
> under the good old I2C adapter node.
> 
> The second case is a limiting factor, even though not happening in "normal"
> use cases. I cannot see any solution without making the adapter aware of
> the "bus extensions" it has, so on its probe it can always go look for any
> devices there. Taking into account the case of multiple connectors each
> having an extension of the same bus, this may look as follows in device
> tree:
> 
>   --- base device tree ---
> 
>   i2c1: i2c@abcd0000 {
>       compatible = "xyz,i2c-ctrl"; ...
>       i2c-bus-extensions = <&i2c_ctrl_conn0, &i2c_ctrl_conn1>;
>   };
> 
>   connector@0 {
>       i2c_ctrl_conn0: i2c-ctrl {
>           i2c-parent = <&i2c1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
> 
>   connector@1 {
>       i2c_ctrl_conn1: i2c-ctrl {
>           i2c-parent = <&i2c1>;
>           #address-cells = <1>;
>           #size-cells = <0>;
>       };
>   };
> 
> I'd love to have some feedback and opinions about the basic idea before
> digging into the details of this additional step.
> 
> ---
> 
> Changes in v4:
>  - fix a typo in commit message
> 
> This patch first appeared in v3.
> ---
>  drivers/i2c/i2c-core-of.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index a6c407d36800..71c559539a13 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -170,6 +170,15 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
>  	switch (of_reconfig_get_state_change(action, rd)) {
>  	case OF_RECONFIG_CHANGE_ADD:
>  		adap = of_find_i2c_adapter_by_node(rd->dn->parent);
> +		if (adap == NULL) {
> +			struct device_node *i2c_bus;
> +
> +			i2c_bus = of_parse_phandle(rd->dn->parent, "i2c-parent", 0);
> +			if (i2c_bus) {
> +				adap = of_find_i2c_adapter_by_node(i2c_bus);
> +				of_node_put(i2c_bus);
> +			}
> +		}
>  		if (adap == NULL)
>  			return NOTIFY_OK;	/* not for us */
>  

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com