Skip to contentSkip to conversation

i2c: device-tree: Handling child nodes which are not i2c devices

RFC Jon Hunter 5 participants 7 messages Apr 15, 2016 – Aug 1, 2016 3 messages not in the archive

Subsystems (2)
lore
Share & tools
lore

Jon Hunter

author maintainer
#0 lore raw

Hi all,

For Tegra we have an i2c device for display port, namely the display port auxiliary channel (or dpaux) as specified by the display port standard. If an design using Tegra does not utilise the display port interface, then the pads assigned to the dpaux can be re-assigned to another generic i2c controller (i2c6 for Tegra124/210). In other words, the pads can be re-used for a generic i2c interface.

The registers that control whether the pads are mapped to the dpaux or i2c6 are located in the dpaux register space. Therefore, I am looking at adding pin controller support for dpaux so that i2c6 can request these pads if it is enabled and I was hoping to add a pinmux node the to dpaux device in device-tree to do this. For example, something like ...

	dpaux@0,545c0000 {
		...

		/* pinctrl node */
		pinmux {
			...
		};
	};

Although the above works, when doing this I noticed that when the device booted, I would seeing the following error messages on boot ...

i2c i2c-5: of_i2c: modalias failure on ...

These error messages being caused by the new pinmux node because it is not recognised as an i2c device. To avoid this error messages we have come up with a couple solutions but wanted to get some feedback on the best approach.

1. Add a i2c-bus sub-node to the dpaux binding (suggested by Stephen Warren), so we would have something like the below. Then i2c devices for dpaux would be place in the i2c-bus sub-node.

	dpaux@0,545c0000 {
		...

		/* pinctrl node */
		pinmux {
			...
		};

		/* place-holder for i2c devices */
		i2c-bus {
			...
		};
	};	

To make the above work ideally we would like to make the 'i2c-bus' node a generic solution for all i2c devices, so the i2c core would check for the presence of this node and if it is found then would default to this node for looking for i2c-devices.

2. When registering i2c devices via device-tree, the function of_i2c_register_devices() checks to see if OF_POPULATED flag is set for a given node. If it is set, then the node is skipped. I believe this was added for device-tree overlays (commit 4f001fd30145 i2c:
Mark instantiated device nodes with OF_POPULATE). Another option is for the dpaux driver to mark the pinmux node as populated before registering the i2c adapter and this will prevent the i2c core from trying to parse the pinmux node. I am not sure if this would be frowned upon in anyway or if we can guarantee that no future changes to DT overlays would change this in a way where it would not work.

Let me know your thoughts.

Cheers
Jon

author maintainer
#1 lore raw

Hi all,

Any thoughts on this? Let me know if this is not clear in anyway.

Cheers
Jon

Quoted 78 lines · Jon Hunter

Hi all,

For Tegra we have an i2c device for display port, namely the display port auxiliary channel (or dpaux) as specified by the display port standard. If an design using Tegra does not utilise the display port interface, then the pads assigned to the dpaux can be re-assigned to another generic i2c controller (i2c6 for Tegra124/210). In other words, the pads can be re-used for a generic i2c interface.

The registers that control whether the pads are mapped to the dpaux or i2c6 are located in the dpaux register space. Therefore, I am looking at adding pin controller support for dpaux so that i2c6 can request these pads if it is enabled and I was hoping to add a pinmux node the to dpaux device in device-tree to do this. For example, something like ...

	dpaux@0,545c0000 {
		...

		/* pinctrl node */
		pinmux {
			...
		};
	};

Although the above works, when doing this I noticed that when the device booted, I would seeing the following error messages on boot ...

i2c i2c-5: of_i2c: modalias failure on ...

These error messages being caused by the new pinmux node because it is not recognised as an i2c device. To avoid this error messages we have come up with a couple solutions but wanted to get some feedback on the best approach.

1. Add a i2c-bus sub-node to the dpaux binding (suggested by Stephen Warren), so we would have something like the below. Then i2c devices for dpaux would be place in the i2c-bus sub-node.

	dpaux@0,545c0000 {
		...

		/* pinctrl node */
		pinmux {
			...
		};

		/* place-holder for i2c devices */
		i2c-bus {
			...
		};
	};	

To make the above work ideally we would like to make the 'i2c-bus' node a generic solution for all i2c devices, so the i2c core would check for the presence of this node and if it is found then would default to this node for looking for i2c-devices.

2. When registering i2c devices via device-tree, the function of_i2c_register_devices() checks to see if OF_POPULATED flag is set for a given node. If it is set, then the node is skipped. I believe this was added for device-tree overlays (commit 4f001fd30145 i2c:
Mark instantiated device nodes with OF_POPULATE). Another option is for the dpaux driver to mark the pinmux node as populated before registering the i2c adapter and this will prevent the i2c core from trying to parse the pinmux node. I am not sure if this would be frowned upon in anyway or if we can guarantee that no future changes to DT overlays would change this in a way where it would not work.

Let me know your thoughts.

Cheers
Jon

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

#3 lore raw
Quoted 3 lines · Jon Hunter

Hi all,

Any thoughts on this? Let me know if this is not clear in anyway.

I'm not sure if you're expecting comment from me since I suggested option (1) below. For the record though I do think that's the better option; it's very general/explicit, should apply well to any OS environment, and relying on OF_POPULATED implies an order that the driver must initialize things in, so that it ensures OF_POPULATED is set before any I2C children are handled.

Quoted 70 lines · Jon Hunter
Quoted 69 lines

Hi all,

For Tegra we have an i2c device for display port, namely the display port auxiliary channel (or dpaux) as specified by the display port standard. If an design using Tegra does not utilise the display port interface, then the pads assigned to the dpaux can be re-assigned to another generic i2c controller (i2c6 for Tegra124/210). In other words, the pads can be re-used for a generic i2c interface.

The registers that control whether the pads are mapped to the dpaux or i2c6 are located in the dpaux register space. Therefore, I am looking at adding pin controller support for dpaux so that i2c6 can request these pads if it is enabled and I was hoping to add a pinmux node the to dpaux device in device-tree to do this. For example, something like ...

	dpaux@0,545c0000 {
		...

		/* pinctrl node */
		pinmux {
			...
		};
	};

Although the above works, when doing this I noticed that when the device booted, I would seeing the following error messages on boot ...

i2c i2c-5: of_i2c: modalias failure on ...

These error messages being caused by the new pinmux node because it is not recognised as an i2c device. To avoid this error messages we have come up with a couple solutions but wanted to get some feedback on the best approach.

1. Add a i2c-bus sub-node to the dpaux binding (suggested by Stephen

    Warren), so we would have something like the below. Then i2c devices
    for dpaux would be place in the i2c-bus sub-node.

	dpaux@0,545c0000 {
		...

		/* pinctrl node */
		pinmux {
			...
		};

		/* place-holder for i2c devices */
		i2c-bus {
			...
		};
	};	

    To make the above work ideally we would like to make the 'i2c-bus'
    node a generic solution for all i2c devices, so the i2c core would
    check for the presence of this node and if it is found then would
    default to this node for looking for i2c-devices.

2. When registering i2c devices via device-tree, the function of_i2c_register_devices() checks to see if OF_POPULATED flag is set for a given node. If it is set, then the node is skipped. I believe this was added for device-tree overlays (commit 4f001fd30145 i2c:
Mark instantiated device nodes with OF_POPULATE). Another option is for the dpaux driver to mark the pinmux node as populated before registering the i2c adapter and this will prevent the i2c core from trying to parse the pinmux node. I am not sure if this would be frowned upon in anyway or if we can guarantee that no future changes to DT overlays would change this in a way where it would not work.

Let me know your thoughts.

author maintainer
#4 lore raw

Hi Wolfram,

Any thoughts on this? Would you accept option #1 below?

Cheers
Jon

Quoted 83 lines · Stephen Warren
Quoted 3 lines · Jon Hunter

Hi all,

Any thoughts on this? Let me know if this is not clear in anyway.

I'm not sure if you're expecting comment from me since I suggested option (1) below. For the record though I do think that's the better option; it's very general/explicit, should apply well to any OS environment, and relying on OF_POPULATED implies an order that the driver must initialize things in, so that it ensures OF_POPULATED is set before any I2C children are handled.

Quoted 70 lines · Jon Hunter
Quoted 69 lines

Hi all,

For Tegra we have an i2c device for display port, namely the display port auxiliary channel (or dpaux) as specified by the display port standard. If an design using Tegra does not utilise the display port interface, then the pads assigned to the dpaux can be re-assigned to another generic i2c controller (i2c6 for Tegra124/210). In other words, the pads can be re-used for a generic i2c interface.

The registers that control whether the pads are mapped to the dpaux or i2c6 are located in the dpaux register space. Therefore, I am looking at adding pin controller support for dpaux so that i2c6 can request these pads if it is enabled and I was hoping to add a pinmux node the to dpaux device in device-tree to do this. For example, something like ...

    dpaux@0,545c0000 {
        ...

        /* pinctrl node */
        pinmux {
            ...
        };
    };

Although the above works, when doing this I noticed that when the device booted, I would seeing the following error messages on boot ...

i2c i2c-5: of_i2c: modalias failure on ...

These error messages being caused by the new pinmux node because it is not recognised as an i2c device. To avoid this error messages we have come up with a couple solutions but wanted to get some feedback on the best approach.

1. Add a i2c-bus sub-node to the dpaux binding (suggested by Stephen

    Warren), so we would have something like the below. Then i2c devices
    for dpaux would be place in the i2c-bus sub-node.

    dpaux@0,545c0000 {
        ...

        /* pinctrl node */
        pinmux {
            ...
        };

        /* place-holder for i2c devices */
        i2c-bus {
            ...
        };
    };   

    To make the above work ideally we would like to make the 'i2c-bus'
    node a generic solution for all i2c devices, so the i2c core would
    check for the presence of this node and if it is found then would
    default to this node for looking for i2c-devices.

2. When registering i2c devices via device-tree, the function of_i2c_register_devices() checks to see if OF_POPULATED flag is set for a given node. If it is set, then the node is skipped. I believe this was added for device-tree overlays (commit 4f001fd30145 i2c:
Mark instantiated device nodes with OF_POPULATE). Another option is for the dpaux driver to mark the pinmux node as populated before registering the i2c adapter and this will prevent the i2c core from trying to parse the pinmux node. I am not sure if this would be frowned upon in anyway or if we can guarantee that no future changes to DT overlays would change this in a way where it would not work.

Let me know your thoughts.

#5 lore raw
In reply to Jon Hunter
Quoted 52 lines · Jon Hunter

Hi all,

For Tegra we have an i2c device for display port, namely the display port auxiliary channel (or dpaux) as specified by the display port standard. If an design using Tegra does not utilise the display port interface, then the pads assigned to the dpaux can be re-assigned to another generic i2c controller (i2c6 for Tegra124/210). In other words, the pads can be re-used for a generic i2c interface.

The registers that control whether the pads are mapped to the dpaux or i2c6 are located in the dpaux register space. Therefore, I am looking at adding pin controller support for dpaux so that i2c6 can request these pads if it is enabled and I was hoping to add a pinmux node the to dpaux device in device-tree to do this. For example, something like ...

	dpaux@0,545c0000 {
		...

		/* pinctrl node */
		pinmux {
			...
		};
	};

Although the above works, when doing this I noticed that when the device booted, I would seeing the following error messages on boot ...

i2c i2c-5: of_i2c: modalias failure on ...

These error messages being caused by the new pinmux node because it is not recognised as an i2c device. To avoid this error messages we have come up with a couple solutions but wanted to get some feedback on the best approach.

1. Add a i2c-bus sub-node to the dpaux binding (suggested by Stephen Warren), so we would have something like the below. Then i2c devices for dpaux would be place in the i2c-bus sub-node.

	dpaux@0,545c0000 {
		...

		/* pinctrl node */
		pinmux {
			...
		};

		/* place-holder for i2c devices */
		i2c-bus {
			...
		};
	};	
Quoted 1 line

From a binding perspective, this makes the most sense to me.

I believe we have variants of this (container nodes for actual busses owned by a controller) in practice today elsewhere.

Quoted 4 lines

To make the above work ideally we would like to make the 'i2c-bus' node a generic solution for all i2c devices, so the i2c core would check for the presence of this node and if it is found then would default to this node for looking for i2c-devices.

I don't have strong opinions on this (having a generic subnode name for the actual bus exposed by a controller) either way.

I can imagine that it would be possible for a single controller to expose multiple I2C (or other) busses, so it might make sense for this to be the preferred style, but not necessarily enforeced as a generic binding.

Thanks,
Mark

A message that is not in the archive (sent off-list or not yet received): [email protected]

#7 lore raw
Quoted 27 lines · Jon Hunter

Hi all,

For Tegra we have an i2c device for display port, namely the display port auxiliary channel (or dpaux) as specified by the display port standard. If an design using Tegra does not utilise the display port interface, then the pads assigned to the dpaux can be re-assigned to another generic i2c controller (i2c6 for Tegra124/210). In other words, the pads can be re-used for a generic i2c interface.

The registers that control whether the pads are mapped to the dpaux or i2c6 are located in the dpaux register space. Therefore, I am looking at adding pin controller support for dpaux so that i2c6 can request these pads if it is enabled and I was hoping to add a pinmux node the to dpaux device in device-tree to do this. For example, something like ...

        dpaux@0,545c0000 {
                ...

                /* pinctrl node */
                pinmux {
                        ...
                };
        };

Although the above works, when doing this I noticed that when the device booted, I would seeing the following error messages on boot ...

Quoted 6 lines

i2c i2c-5: of_i2c: modalias failure on ...

These error messages being caused by the new pinmux node because it is not recognised as an i2c device. To avoid this error messages we have come up with a couple solutions but wanted to get some feedback on the best approach.

Hi,

I'm finding this issue as well on Rockchip SoCs, this is the specific node that is raising this warning when an adapter is registered with it:

edp: dp@ff970000 {
    compatible = "rockchip,rk3288-dp";
    ...
    ports {
        ...
    };
};

Has there been any agreement since this thread waned?

Thanks,

Tomeu

Quoted 36 lines

1. Add a i2c-bus sub-node to the dpaux binding (suggested by Stephen Warren), so we would have something like the below. Then i2c devices for dpaux would be place in the i2c-bus sub-node.

        dpaux@0,545c0000 {
                ...

                /* pinctrl node */
                pinmux {
                        ...
                };

                /* place-holder for i2c devices */
                i2c-bus {
                        ...
                };
        };

To make the above work ideally we would like to make the 'i2c-bus' node a generic solution for all i2c devices, so the i2c core would check for the presence of this node and if it is found then would default to this node for looking for i2c-devices.

2. When registering i2c devices via device-tree, the function of_i2c_register_devices() checks to see if OF_POPULATED flag is set for a given node. If it is set, then the node is skipped. I believe this was added for device-tree overlays (commit 4f001fd30145 i2c:
Mark instantiated device nodes with OF_POPULATE). Another option is for the dpaux driver to mark the pinmux node as populated before registering the i2c adapter and this will prevent the i2c core from trying to parse the pinmux node. I am not sure if this would be frowned upon in anyway or if we can guarantee that no future changes to DT overlays would change this in a way where it would not work.

Let me know your thoughts.

Quoted 7 lines

Cheers
Jon

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CAAObsKC-DGH4GedCcZdv2zvesaTSo6ugsHSQU=J0=P1cZeyCDA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#9 lore raw
Quoted 49 lines · Tomeu Vizoso
Quoted 27 lines · Jon Hunter

Hi all,

For Tegra we have an i2c device for display port, namely the display port auxiliary channel (or dpaux) as specified by the display port standard. If an design using Tegra does not utilise the display port interface, then the pads assigned to the dpaux can be re-assigned to another generic i2c controller (i2c6 for Tegra124/210). In other words, the pads can be re-used for a generic i2c interface.

The registers that control whether the pads are mapped to the dpaux or i2c6 are located in the dpaux register space. Therefore, I am looking at adding pin controller support for dpaux so that i2c6 can request these pads if it is enabled and I was hoping to add a pinmux node the to dpaux device in device-tree to do this. For example, something like ...

        dpaux@0,545c0000 {
                ...

                /* pinctrl node */
                pinmux {
                        ...
                };
        };

Although the above works, when doing this I noticed that when the device booted, I would seeing the following error messages on boot ...

Quoted 6 lines

i2c i2c-5: of_i2c: modalias failure on ...

These error messages being caused by the new pinmux node because it is not recognised as an i2c device. To avoid this error messages we have come up with a couple solutions but wanted to get some feedback on the best approach.

Hi,

I'm finding this issue as well on Rockchip SoCs, this is the specific node that is raising this warning when an adapter is registered with it:

edp: dp@ff970000 {
    compatible = "rockchip,rk3288-dp";
    ...
    ports {
        ...
    };
};
Quoted 5 lines

Has there been any agreement since this thread waned?

Thanks,

Tomeu

This is currently soaking in linux-next:

dt-bindings: i2c: Add support for 'i2c-bus' subnode
https://git.kernel.org/cgit/linux/kernel/git/wsa/linux.git/commit/?h=i2c/for-next&id=98b00488459e

i2c: core: Add support for 'i2c-bus' subnode
https://git.kernel.org/cgit/linux/kernel/git/wsa/linux.git/commit/?h=i2c/for-next&id=7e4c224abfe8

Cheers,
Peter

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html