tegra: dpaux: pinctrl proposal

Background:
==========
On tegra124 and tegra132 devices the pads used by the Display Port Auxiliary (DPAUX) channel are multiplexed such that they can also be used by one of the internal i2c controllers. Note that this is different from i2c-over-AUX supported by the DPAUX controller. The register that configures these pads is part of the DPAUX controllers register set and so requires the clock for the DPAUX controller to be enabled to access the register as well as keeping the SOR (serial output resource) power domain enabled.

Currently, there is no pinctrl device for these pads and so cannot be easily mapped to function as an i2c interface. Furthermore, when using the pads for the DPAUX channel, the DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) directly writes the to appropriate register to setup the pads.

There are some products based upon the tegra132 that use these pads for an internal i2c controller and hence we want to support this configuration in the kernel.

Proposal:
========
Add a DPAUX MFD device that consists of a DPAUX controller, for the Display Port Auxiliary related functionality and a DPAUX pad controller, for handling the pinctrl for the DPAUX pads. Both the DPAUX controller and DPAUX pad controller need to access the DPAUX register set and therefore, by making the MFD compatible with "simple-mfd" and "syscon", a regmap for the DPAUX registers will be created to synchronise register accesses made by the drivers.

* PGP Signed by an unknown key

Quoted 15 lines · Jon Hunter

Background:
==========
On tegra124 and tegra132 devices the pads used by the Display Port Auxiliary (DPAUX) channel are multiplexed such that they can also be used by one of the internal i2c controllers. Note that this is different from i2c-over-AUX supported by the DPAUX controller. The register that configures these pads is part of the DPAUX controllers register set and so requires the clock for the DPAUX controller to be enabled to access the register as well as keeping the SOR (serial output resource) power domain enabled.

Currently, there is no pinctrl device for these pads and so cannot be easily mapped to function as an i2c interface. Furthermore, when using the pads for the DPAUX channel, the DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) directly writes the to appropriate register to setup the pads.

Quoted 3 lines

There are some products based upon the tegra132 that use these pads for an internal i2c controller and hence we want to support this configuration in the kernel.

Good timing, I was going to (reluctantly) add this to my long TODO list. I generally like the proposal.

Quoted 8 lines

Proposal:
========
Add a DPAUX MFD device that consists of a DPAUX controller, for the Display Port Auxiliary related functionality and a DPAUX pad controller, for handling the pinctrl for the DPAUX pads. Both the DPAUX controller and DPAUX pad controller need to access the DPAUX register set and therefore, by making the MFD compatible with "simple-mfd" and "syscon", a regmap for the DPAUX registers will be created to synchronise register accesses made by the drivers.

Can we not do without an MFD here? Not only would it break DT ABI, but it's also way more complicated than it needs to be in my opinion, we're only sharing a single register (or perhaps even two) after all. Keeping everything in a single DT node would also make the binding less awkward because the power domain doesn't apply to the pad controller part of DPAUX.

Can't the dpaux driver simply register the pinmux controller itself?

Quoted 18 lines · Thierry Reding

* PGP Signed by an unknown key

Quoted 15 lines · Jon Hunter

Background:
==========
On tegra124 and tegra132 devices the pads used by the Display Port Auxiliary (DPAUX) channel are multiplexed such that they can also be used by one of the internal i2c controllers. Note that this is different from i2c-over-AUX supported by the DPAUX controller. The register that configures these pads is part of the DPAUX controllers register set and so requires the clock for the DPAUX controller to be enabled to access the register as well as keeping the SOR (serial output resource) power domain enabled.

Currently, there is no pinctrl device for these pads and so cannot be easily mapped to function as an i2c interface. Furthermore, when using the pads for the DPAUX channel, the DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) directly writes the to appropriate register to setup the pads.

Quoted 6 lines

Quoted 3 lines

There are some products based upon the tegra132 that use these pads for an internal i2c controller and hence we want to support this configuration in the kernel.

Good timing, I was going to (reluctantly) add this to my long TODO list. I generally like the proposal.

Ok, great.

Quoted 9 lines

Quoted 8 lines

Proposal:
========
Add a DPAUX MFD device that consists of a DPAUX controller, for the Display Port Auxiliary related functionality and a DPAUX pad controller, for handling the pinctrl for the DPAUX pads. Both the DPAUX controller and DPAUX pad controller need to access the DPAUX register set and therefore, by making the MFD compatible with "simple-mfd" and "syscon", a regmap for the DPAUX registers will be created to synchronise register accesses made by the drivers.

Quoted 8 lines

Can we not do without an MFD here? Not only would it break DT ABI, but it's also way more complicated than it needs to be in my opinion, we're only sharing a single register (or perhaps even two) after all. Keeping everything in a single DT node would also make the binding less awkward because the power domain doesn't apply to the pad controller part of DPAUX.

Can't the dpaux driver simply register the pinmux controller itself?

Do you think something that looks like the below?

Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt +27 -0

+Example (tegra124 DPAUX):
+
+/ {
+       ...
+
+       host1x {
+               compatible = "nvidia,tegra124-host1x", "simple-bus";
+               ...
+
+               dpaux: dpaux@0,545c0000 {
+                       compatible = "nvidia,tegra124-dpaux",
+                       reg = <0x0 0x545c0000 0x0 0x40000>;
+                       interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
+                       clocks = <&tegra_car TEGRA124_CLK_DPAUX>,
+                                <&tegra_car TEGRA124_CLK_PLL_DP>;
+                       clock-names = "dpaux", "parent";
+                       resets = <&tegra_car 181>;
+                       reset-names = "dpaux";
+                       pinctrl-0 = <&dpaux_state>;
+                       pinctrl-names = "default";
+                       status = "disabled";
+
+                       dpaux_padctl@0,545c0124 {
+                               compatible = "nvidia,tegra124-dpaux-padctl";
+
+                               dpaux_state: dpaux_state0 {
+                                       dpaux {

* PGP Signed by an unknown key

Quoted 76 lines · Jon Hunter

Quoted 18 lines · Thierry Reding

Quoted 1 line

Old Signed by an unknown key

Quoted 15 lines · Jon Hunter

Background:
==========
On tegra124 and tegra132 devices the pads used by the Display Port Auxiliary (DPAUX) channel are multiplexed such that they can also be used by one of the internal i2c controllers. Note that this is different from i2c-over-AUX supported by the DPAUX controller. The register that configures these pads is part of the DPAUX controllers register set and so requires the clock for the DPAUX controller to be enabled to access the register as well as keeping the SOR (serial output resource) power domain enabled.

Currently, there is no pinctrl device for these pads and so cannot be easily mapped to function as an i2c interface. Furthermore, when using the pads for the DPAUX channel, the DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) directly writes the to appropriate register to setup the pads.

Quoted 6 lines

Quoted 3 lines

There are some products based upon the tegra132 that use these pads for an internal i2c controller and hence we want to support this configuration in the kernel.

Good timing, I was going to (reluctantly) add this to my long TODO list. I generally like the proposal.

Ok, great.

Quoted 9 lines

Quoted 8 lines

Proposal:
========
Add a DPAUX MFD device that consists of a DPAUX controller, for the Display Port Auxiliary related functionality and a DPAUX pad controller, for handling the pinctrl for the DPAUX pads. Both the DPAUX controller and DPAUX pad controller need to access the DPAUX register set and therefore, by making the MFD compatible with "simple-mfd" and "syscon", a regmap for the DPAUX registers will be created to synchronise register accesses made by the drivers.

Quoted 8 lines

Can we not do without an MFD here? Not only would it break DT ABI, but it's also way more complicated than it needs to be in my opinion, we're only sharing a single register (or perhaps even two) after all. Keeping everything in a single DT node would also make the binding less awkward because the power domain doesn't apply to the pad controller part of DPAUX.

Can't the dpaux driver simply register the pinmux controller itself?

Do you think something that looks like the below?

(quoted diff) +27 -0

+Example (tegra124 DPAUX):
+
+/ {
+       ...
+
+       host1x {
+               compatible = "nvidia,tegra124-host1x", "simple-bus";
+               ...
+
+               dpaux: dpaux@0,545c0000 {
+                       compatible = "nvidia,tegra124-dpaux",
+                       reg = <0x0 0x545c0000 0x0 0x40000>;
+                       interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
+                       clocks = <&tegra_car TEGRA124_CLK_DPAUX>,
+                                <&tegra_car TEGRA124_CLK_PLL_DP>;
+                       clock-names = "dpaux", "parent";
+                       resets = <&tegra_car 181>;
+                       reset-names = "dpaux";
+                       pinctrl-0 = <&dpaux_state>;
+                       pinctrl-names = "default";
+                       status = "disabled";
+
+                       dpaux_padctl@0,545c0124 {
+                               compatible = "nvidia,tegra124-dpaux-padctl";
+
+                               dpaux_state: dpaux_state0 {
+                                       dpaux {

Review

Quoted 10 lines

(quoted diff) +10 -0

+                                               nvidia,function = "dpaux";
+                                       };
+                               };
+
+                               i2c_state: i2c_state0 {
+                                       i2c {
+                                               nvidia,function = "i2c";
+                                       };
+                               };
+                       };

Why even have this subnode? Couldn't we simply have this:

	host1x@... {
		...

		dpaux@... {
			compatible = "nvidia,tegra124-dpaux";
			...
			pinctrl-0 = <&dpaux_aux_state>;
			pinctrl-1 = <&dpaux_i2c_state>;
			pinctrl-names = "aux", "i2c";
			...

			dpaux_aux_state: pinmux-aux {
				...
			};

			dpaux_i2c_state: pinmux-i2c {
				...
			};
		};
	};

?

We might need to add in indices to tell apart DPAUX and DPAUX1, though perhaps we could refer to these states by path instead of phandle to avoid that. Anyway, I don't see any particular reason why a subnode would be necessary.

Quoted 113 lines · Thierry Reding

* PGP Signed by an unknown key

Quoted 76 lines · Jon Hunter

Quoted 18 lines · Thierry Reding

Quoted 1 line

Old Signed by an unknown key

Quoted 15 lines · Jon Hunter

Background:
==========
On tegra124 and tegra132 devices the pads used by the Display Port Auxiliary (DPAUX) channel are multiplexed such that they can also be used by one of the internal i2c controllers. Note that this is different from i2c-over-AUX supported by the DPAUX controller. The register that configures these pads is part of the DPAUX controllers register set and so requires the clock for the DPAUX controller to be enabled to access the register as well as keeping the SOR (serial output resource) power domain enabled.

Currently, there is no pinctrl device for these pads and so cannot be easily mapped to function as an i2c interface. Furthermore, when using the pads for the DPAUX channel, the DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) directly writes the to appropriate register to setup the pads.

Quoted 6 lines

Quoted 3 lines

There are some products based upon the tegra132 that use these pads for an internal i2c controller and hence we want to support this configuration in the kernel.

Good timing, I was going to (reluctantly) add this to my long TODO list. I generally like the proposal.

Ok, great.

Quoted 9 lines

Quoted 8 lines

Proposal:
========
Add a DPAUX MFD device that consists of a DPAUX controller, for the Display Port Auxiliary related functionality and a DPAUX pad controller, for handling the pinctrl for the DPAUX pads. Both the DPAUX controller and DPAUX pad controller need to access the DPAUX register set and therefore, by making the MFD compatible with "simple-mfd" and "syscon", a regmap for the DPAUX registers will be created to synchronise register accesses made by the drivers.

Quoted 8 lines

Can we not do without an MFD here? Not only would it break DT ABI, but it's also way more complicated than it needs to be in my opinion, we're only sharing a single register (or perhaps even two) after all. Keeping everything in a single DT node would also make the binding less awkward because the power domain doesn't apply to the pad controller part of DPAUX.

Can't the dpaux driver simply register the pinmux controller itself?

Do you think something that looks like the below?

(quoted diff) +27 -0

+Example (tegra124 DPAUX):
+
+/ {
+       ...
+
+       host1x {
+               compatible = "nvidia,tegra124-host1x", "simple-bus";
+               ...
+
+               dpaux: dpaux@0,545c0000 {
+                       compatible = "nvidia,tegra124-dpaux",
+                       reg = <0x0 0x545c0000 0x0 0x40000>;
+                       interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
+                       clocks = <&tegra_car TEGRA124_CLK_DPAUX>,
+                                <&tegra_car TEGRA124_CLK_PLL_DP>;
+                       clock-names = "dpaux", "parent";
+                       resets = <&tegra_car 181>;
+                       reset-names = "dpaux";
+                       pinctrl-0 = <&dpaux_state>;
+                       pinctrl-names = "default";
+                       status = "disabled";
+
+                       dpaux_padctl@0,545c0124 {
+                               compatible = "nvidia,tegra124-dpaux-padctl";
+
+                               dpaux_state: dpaux_state0 {
+                                       dpaux {

Review

Quoted 10 lines

(quoted diff) +10 -0

+                                               nvidia,function = "dpaux";
+                                       };
+                               };
+
+                               i2c_state: i2c_state0 {
+                                       i2c {
+                                               nvidia,function = "i2c";
+                                       };
+                               };
+                       };

Why even have this subnode? Couldn't we simply have this:

	host1x@... {
		...

		dpaux@... {
			compatible = "nvidia,tegra124-dpaux";
			...
			pinctrl-0 = <&dpaux_aux_state>;
			pinctrl-1 = <&dpaux_i2c_state>;
			pinctrl-names = "aux", "i2c";
			...

			dpaux_aux_state: pinmux-aux {
				...
			};

			dpaux_i2c_state: pinmux-i2c {
				...
			};
		};
	};

Quoted 6 lines

?

We might need to add in indices to tell apart DPAUX and DPAUX1, though perhaps we could refer to these states by path instead of phandle to avoid that. Anyway, I don't see any particular reason why a subnode would be necessary.

My thinking was that we would have a pinctrl driver for dpaux in drivers/pinctrl/pinctrl-tegra-dpaux.c and therefore, I had assumed that we would need a sub-node and compatible string to probe the device.

Are you sugguesting that the pinctrl driver for dpaux lives in drivers/gpu/drm/tegra/dpaux.c?

Sorry if I am misunderstanding something here.

Quoted 149 lines · Jon Hunter

Quoted 136 lines · Thierry Reding

* PGP Signed by an unknown key

Quoted 106 lines · Jon Hunter

Quoted 33 lines · Thierry Reding

Quoted 1 line

Old Signed by an unknown key

Quoted 24 lines · Jon Hunter

Background:
==========
On tegra124 and tegra132 devices the pads used by the Display Port Auxiliary
(DPAUX) channel are multiplexed such that they can also be used by one of the
internal i2c controllers. Note that this is different from i2c-over-AUX
supported by the DPAUX controller. The register that configures these pads is
part of the DPAUX controllers register set and so requires the clock for the
DPAUX controller to be enabled to access the register as well as keeping the
SOR (serial output resource) power domain enabled.

Currently, there is no pinctrl device for these pads and so cannot be easily
mapped to function as an i2c interface. Furthermore, when using the pads for
the DPAUX channel, the DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) directly
writes the to appropriate register to setup the pads.

Quoted 5 lines

There are some products based upon the tegra132 that use these pads for an
internal i2c controller and hence we want to support this configuration in the
kernel.

Quoted 3 lines

Good timing, I was going to (reluctantly) add this to my long TODO list.
I generally like the proposal.

Ok, great.

Quoted 24 lines

Quoted 5 lines

Proposal:
========
Add a DPAUX MFD device that consists of a DPAUX controller, for the Display
Port Auxiliary related functionality and a DPAUX pad controller,

Quoted 8 lines

for handling
the pinctrl for the DPAUX pads. Both the DPAUX controller and DPAUX pad
controller need to access the DPAUX register set and therefore, by making the
MFD compatible with "simple-mfd" and "syscon", a regmap for the DPAUX registers
will be created to synchronise register accesses made by the drivers.

Can we not do without an MFD here? Not only would it break DT ABI, but it's also way more complicated than it needs to be in my opinion, we're
only sharing a single register (or perhaps even two) after all. Keeping
everything in a single DT node would also make the binding less awkward
because the power domain doesn't apply to the pad controller part of DPAUX.

Quoted 1 line

Can't the dpaux driver simply register the pinmux controller itself?

Do you think something that looks like the below?

(quoted diff) +24 -0

+Example (tegra124 DPAUX):
+
+/ {
+       ...
+
+       host1x {
+               compatible = "nvidia,tegra124-host1x", "simple-bus";
+               ...
+
+               dpaux: dpaux@0,545c0000 {
+                       compatible = "nvidia,tegra124-dpaux",
+                       reg = <0x0 0x545c0000 0x0 0x40000>;
+                       interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
+                       clocks = <&tegra_car TEGRA124_CLK_DPAUX>,
+                                <&tegra_car TEGRA124_CLK_PLL_DP>;
+                       clock-names = "dpaux", "parent";
+                       resets = <&tegra_car 181>;
+                       reset-names = "dpaux";
+                       pinctrl-0 = <&dpaux_state>;
+                       pinctrl-names = "default";
+                       status = "disabled";
+
+                       dpaux_padctl@0,545c0124 {
+                               compatible =

"nvidia,tegra124-dpaux-padctl";

(quoted diff) +4 -0

+
+                               dpaux_state: dpaux_state0 {
+                                       dpaux {
+                                               nvidia,function =

"dpaux";

(quoted diff) +6 -0

+                                       };
+                               };
+
+                               i2c_state: i2c_state0 {
+                                       i2c {
+                                               nvidia,function =

"i2c";

Review

Quoted 3 lines

(quoted diff) +3 -0

+                                       };
+                               };
+                       };

Why even have this subnode? Couldn't we simply have this:

    host1x@... {
        ...

        dpaux@... {
            compatible = "nvidia,tegra124-dpaux";
            ...
            pinctrl-0 = <&dpaux_aux_state>;
            pinctrl-1 = <&dpaux_i2c_state>;
            pinctrl-names = "aux", "i2c";
            ...

            dpaux_aux_state: pinmux-aux {
                ...
            };

            dpaux_i2c_state: pinmux-i2c {
                ...
            };
        };
    };

Quoted 6 lines

?

We might need to add in indices to tell apart DPAUX and DPAUX1, though perhaps we could refer to these states by path instead of phandle to avoid that. Anyway, I don't see any particular reason why a subnode would be necessary.

My thinking was that we would have a pinctrl driver for dpaux in drivers/pinctrl/pinctrl-tegra-dpaux.c and therefore, I had assumed that we would need a sub-node and compatible string to probe the device.

Quoted 4 lines

Are you sugguesting that the pinctrl driver for dpaux lives in drivers/gpu/drm/tegra/dpaux.c?

Sorry if I am misunderstanding something here.

I think a single DT node for the single HW block makes sense. IIUC, that would most correctly reflect how the HW is actually structured.

I don't see any conceptual reason why the driver that binds to that node can't register as both a pinctrl driver plus anything else it needs to. For example, there are plenty of Linux drivers that register as both GPIO and pinctrl drivers already. If having the same "struct device" register with multiple subsystems doesn't work out (IIRC some subsystems attempt to own the struct device's one driver_data field), then the top-level driver can internally create whatever child devices it needs to do its job. Using MFD to do that feels like overkill in this situation since those child devices are unlikely to ever show up with some different parent device or register offset. Either way, the choice of whether to use MFD or not shouldn't affect the DT binding in any way.

Quoted 150 lines · Stephen Warren

Quoted 149 lines · Jon Hunter

Quoted 136 lines · Thierry Reding

* PGP Signed by an unknown key

Quoted 106 lines · Jon Hunter

Quoted 33 lines · Thierry Reding

Quoted 1 line

Old Signed by an unknown key

Quoted 24 lines · Jon Hunter

Background:
==========
On tegra124 and tegra132 devices the pads used by the Display Port Auxiliary
(DPAUX) channel are multiplexed such that they can also be used by one of the
internal i2c controllers. Note that this is different from i2c-over-AUX
supported by the DPAUX controller. The register that configures these pads is
part of the DPAUX controllers register set and so requires the clock for the
DPAUX controller to be enabled to access the register as well as keeping the
SOR (serial output resource) power domain enabled.

Currently, there is no pinctrl device for these pads and so cannot be easily
mapped to function as an i2c interface. Furthermore, when using the pads for
the DPAUX channel, the DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) directly
writes the to appropriate register to setup the pads.

Quoted 5 lines

There are some products based upon the tegra132 that use these pads for an
internal i2c controller and hence we want to support this configuration in the
kernel.

Quoted 3 lines

Good timing, I was going to (reluctantly) add this to my long TODO list.
I generally like the proposal.

Ok, great.

Quoted 24 lines

Quoted 5 lines

Proposal:
========
Add a DPAUX MFD device that consists of a DPAUX controller, for the Display
Port Auxiliary related functionality and a DPAUX pad controller,

Quoted 8 lines

for handling
the pinctrl for the DPAUX pads. Both the DPAUX controller and DPAUX pad
controller need to access the DPAUX register set and therefore, by making the
MFD compatible with "simple-mfd" and "syscon", a regmap for the DPAUX registers
will be created to synchronise register accesses made by the drivers.

Can we not do without an MFD here? Not only would it break DT ABI, but it's also way more complicated than it needs to be in my opinion, we're
only sharing a single register (or perhaps even two) after all. Keeping
everything in a single DT node would also make the binding less awkward
because the power domain doesn't apply to the pad controller part of DPAUX.

Quoted 1 line

Can't the dpaux driver simply register the pinmux controller itself?

Do you think something that looks like the below?

(quoted diff) +24 -0

+Example (tegra124 DPAUX):
+
+/ {
+       ...
+
+       host1x {
+               compatible = "nvidia,tegra124-host1x", "simple-bus";
+               ...
+
+               dpaux: dpaux@0,545c0000 {
+                       compatible = "nvidia,tegra124-dpaux",
+                       reg = <0x0 0x545c0000 0x0 0x40000>;
+                       interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
+                       clocks = <&tegra_car TEGRA124_CLK_DPAUX>,
+                                <&tegra_car TEGRA124_CLK_PLL_DP>;
+                       clock-names = "dpaux", "parent";
+                       resets = <&tegra_car 181>;
+                       reset-names = "dpaux";
+                       pinctrl-0 = <&dpaux_state>;
+                       pinctrl-names = "default";
+                       status = "disabled";
+
+                       dpaux_padctl@0,545c0124 {
+                               compatible =

"nvidia,tegra124-dpaux-padctl";

(quoted diff) +4 -0

+
+                               dpaux_state: dpaux_state0 {
+                                       dpaux {
+                                               nvidia,function =

"dpaux";

(quoted diff) +6 -0

+                                       };
+                               };
+
+                               i2c_state: i2c_state0 {
+                                       i2c {
+                                               nvidia,function =

"i2c";

Review

Quoted 3 lines

(quoted diff) +3 -0

+                                       };
+                               };
+                       };

Why even have this subnode? Couldn't we simply have this:

    host1x@... {
        ...

        dpaux@... {
            compatible = "nvidia,tegra124-dpaux";
            ...
            pinctrl-0 = <&dpaux_aux_state>;
            pinctrl-1 = <&dpaux_i2c_state>;
            pinctrl-names = "aux", "i2c";
            ...

            dpaux_aux_state: pinmux-aux {
                ...
            };

            dpaux_i2c_state: pinmux-i2c {
                ...
            };
        };
    };

Quoted 6 lines

?

We might need to add in indices to tell apart DPAUX and DPAUX1, though perhaps we could refer to these states by path instead of phandle to avoid that. Anyway, I don't see any particular reason why a subnode would be necessary.

My thinking was that we would have a pinctrl driver for dpaux in drivers/pinctrl/pinctrl-tegra-dpaux.c and therefore, I had assumed that we would need a sub-node and compatible string to probe the device.

Quoted 7 lines

Quoted 4 lines

Are you sugguesting that the pinctrl driver for dpaux lives in drivers/gpu/drm/tegra/dpaux.c?

Sorry if I am misunderstanding something here.

I think a single DT node for the single HW block makes sense. IIUC, that would most correctly reflect how the HW is actually structured.

Yes that would be more aligned with the HW.

Quoted 11 lines

I don't see any conceptual reason why the driver that binds to that node can't register as both a pinctrl driver plus anything else it needs to. For example, there are plenty of Linux drivers that register as both GPIO and pinctrl drivers already. If having the same "struct device" register with multiple subsystems doesn't work out (IIRC some subsystems attempt to own the struct device's one driver_data field), then the top-level driver can internally create whatever child devices it needs to do its job. Using MFD to do that feels like overkill in this situation since those child devices are unlikely to ever show up with some different parent device or register offset. Either way, the choice of whether to use MFD or not shouldn't affect the DT binding in any way.

Looking at it there should not be a problem here with regard to the driver_data member of the device struct and so I don't see why the tegra_dpaux_probe() could not call pinctrl_register() to register the device.

However, it does mean that all the pinctrl/pinmux/pinconf ops for this pinctrl device will need to live in drivers/gpu/drm/tegra/dpaux.c which is fine, but I *believe* that would require moving drivers/pinctrl/pinctrl-utils.h to include/linux/pinctrl/ in order to make use of these functions. May be that is fine too. I could put together a patch series and see what everyone thinks.

Quoted 2 lines

Quoted 2 lines

I don't see any conceptual reason why the driver that binds to that node can't register as both a pinctrl driver plus anything else it needs to.

Quoted 4 lines

Looking at it there should not be a problem here with regard to the driver_data member of the device struct and so I don't see why the tegra_dpaux_probe() could not call pinctrl_register() to register the device.

Yes, I think that'd be the best solution.

Quoted 3 lines

However, it does mean that all the pinctrl/pinmux/pinconf ops for this pinctrl device will need to live in drivers/gpu/drm/tegra/dpaux.c which is fine,

Quoted 3 lines

but I *believe* that would require moving drivers/pinctrl/pinctrl-utils.h to include/linux/pinctrl/ in order to make use of these functions.

Quoted 2 lines

May be that is fine too. I could put
together a patch series and see what everyone thinks.

I guess it depends mostly on whether Linus (Cc'ed) is willing to have drivers outside of drivers/pinctrl implement pin controllers.

Quoted 2 lines · Thierry Reding

Quoted 2 lines

Quoted 2 lines

I don't see any conceptual reason why the driver that binds to that node can't register as both a pinctrl driver plus anything else it needs to.

(...)

Quoted 6 lines

Quoted 4 lines

Looking at it there should not be a problem here with regard to the driver_data member of the device struct and so I don't see why the tegra_dpaux_probe() could not call pinctrl_register() to register the device.

Yes, I think that'd be the best solution.

I'm pretty much ready to go to any compromises to get DRM/GPU drivers to use internal kernel subsystems. The tendency there is to reimplement all kernel driver frameworks and hammer registers they need to access.

There are good reasons for. These drivers are usually so complex that things like probing and power up/down become a nightmare with cross-subsystem dependencies.

They are a special case. I had a long discussion with Intel's Daniel Vetter about this and they (Intel) eventually used GPIO for some stuff where it would fit nicely, but didn't go to use fixed regulators as I had suggested.

Quoted 3 lines

Quoted 3 lines

However, it does mean that all the pinctrl/pinmux/pinconf ops for this pinctrl device will need to live in drivers/gpu/drm/tegra/dpaux.c which is fine,

Yeah that's cool I already have e.g. GPIO chips all over the map, including DRM IIRC.

Quoted 3 lines

Quoted 3 lines

but I *believe* that would require moving drivers/pinctrl/pinctrl-utils.h to include/linux/pinctrl/ in order to make use of these functions.

Well I originally intended that to be private and neat, but whatever. Call it pinctrl-utils-internal.h or something then.

Quoted 5 lines

Quoted 2 lines

May be that is fine too. I could put
together a patch series and see what everyone thinks.

I guess it depends mostly on whether Linus (Cc'ed) is willing to have drivers outside of drivers/pinctrl implement pin controllers.

For this case: go ahead.

Quoted 2 lines · Thierry Reding

Quoted 2 lines

Quoted 2 lines

I don't see any conceptual reason why the driver that binds to that node can't register as both a pinctrl driver plus anything else it needs to.

(...)

Quoted 5 lines

Quoted 4 lines

Looking at it there should not be a problem here with regard to the driver_data member of the device struct and so I don't see why the tegra_dpaux_probe() could not call pinctrl_register() to register the device.

Quoted 1 line

Yes, I think that'd be the best solution.

I'm pretty much ready to go to any compromises to get DRM/GPU drivers to use internal kernel subsystems. The tendency there is to reimplement all kernel driver frameworks and hammer registers they need to access.

Quoted 11 lines · Linus Walleij

Quoted 2 lines · Thierry Reding

Quoted 2 lines

Quoted 2 lines

I don't see any conceptual reason why the driver that binds to that node can't register as both a pinctrl driver plus anything else it needs to.

(...)

Quoted 5 lines

Quoted 4 lines

Looking at it there should not be a problem here with regard to the driver_data member of the device struct and so I don't see why the tegra_dpaux_probe() could not call pinctrl_register() to register the device.

Quoted 6 lines

Quoted 1 line

Yes, I think that'd be the best solution.

I'm pretty much ready to go to any compromises to get DRM/GPU drivers to use internal kernel subsystems. The tendency there is to reimplement all kernel driver frameworks and hammer registers they need to access.

Thanks Linus, but after looking at this further I don't think this approach will work after all. In order to call pinctrl_register() from outside the pinctrl directory, also means exposing the pinctrl_dev and pinctrl_desc structures and so this will get messy. Or at least a bigger churn than I would have hoped for.

Thierry, what if we add a drivers/pinctrl/pinctrl-tegra-dpaux.c and then simply call platform_device_register_data() from tegra_dpaux_probe() to register the device? I could still use the parent of_node to match the platform data for the dpaux pin controller.

* PGP Signed by an unknown key

Quoted 20 lines · Jon Hunter

Quoted 11 lines · Linus Walleij

Quoted 2 lines · Thierry Reding

Quoted 2 lines

Quoted 2 lines

I don't see any conceptual reason why the driver that binds to that node can't register as both a pinctrl driver plus anything else it needs to.

(...)

Quoted 5 lines

Quoted 4 lines

Looking at it there should not be a problem here with regard to the driver_data member of the device struct and so I don't see why the tegra_dpaux_probe() could not call pinctrl_register() to register the device.

Quoted 6 lines

Quoted 1 line

Yes, I think that'd be the best solution.

I'm pretty much ready to go to any compromises to get DRM/GPU drivers to use internal kernel subsystems. The tendency there is to reimplement all kernel driver frameworks and hammer registers they need to access.

Quoted 5 lines

Thanks Linus, but after looking at this further I don't think this approach will work after all. In order to call pinctrl_register() from outside the pinctrl directory, also means exposing the pinctrl_dev and pinctrl_desc structures and so this will get messy. Or at least a bigger churn than I would have hoped for.

It's not that much churn, is it? The structure definitions could be moved to include/linux/pinctrl/machine.h. That's included by core.h in drivers/pinctrl anyway, so that should really be all there is to it.

Quoted 4 lines

Thierry, what if we add a drivers/pinctrl/pinctrl-tegra-dpaux.c and then simply call platform_device_register_data() from tegra_dpaux_probe() to register the device? I could still use the parent of_node to match the platform data for the dpaux pin controller.

That'd work for me too. But if that's what it comes down to, I don't see a need to go through the driver model. Why not simply expose a simple set of functions to setup and tear down the pinctrl that the DPAUX driver can call?

I'd still clearly prefer to have the pinctrl code live directly in the DPAUX driver, so I think we should at least give that a shot.

I'd still clearly prefer to have the pinctrl code live directly in the DPAUX driver, so I think we should at least give that a shot.

Quoted 2 lines · Thierry Reding

I'd still clearly prefer to have the pinctrl code live directly in the DPAUX driver, so I think we should at least give that a shot.

I have been working on this more this week and the good news is that by using some of the pinconf-generic handlers I can simplify the code and avoid moving headers and structures around.

However, I have ran into another issue. The current binding looks like this ...

              dpaux: dpaux@0,545c0000 {
                      compatible = "nvidia,tegra124-dpaux";
                      reg = <0x0 0x545c0000 0x0 0x40000>;
                      interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
                      clocks = <&tegra_car TEGRA124_CLK_DPAUX>,
                               <&tegra_car TEGRA124_CLK_PLL_DP>;
                      clock-names = "dpaux", "parent";
                      resets = <&tegra_car 181>;
                      reset-names = "dpaux";
                      status = "disabled";

                      dpaux_state: dpaux_state0 {
                              dpaux {
                                      groups = "dpaux_io";
                                      function = "dpaux";
                                      nvidia,dpaux-drvi;
                                      nvidia,dpaux-drvz;
                                      nvidia,dpaux-cmh;
                              };
                      };

                      i2c_state: i2c_state0 {
                              i2c {
                                      groups = "dpaux_io";
                                      function = "i2c";
                              };
                      };

This all works well, however, because the display-port binding now has child devices which are not i2c clients I see the following messages during kernel boot ...

[    1.607606] i2c i2c-6: of_i2c: modalias failure on
/host1x@0,50000000/dpaux@0,545c0000/dpaux_state0
[    1.616658] i2c i2c-6: of_i2c: modalias failure on

/host1x@0,50000000/dpaux@0,545c0000/i2c_state0

In other words, i2c_add_adapter() (which is called by probing the dpaux) then parses the child nodes looking for i2c client devices. However, these child devices are not i2c client devices and hence the above error messages. I can't find an easy way to avoid this. There is no side-effect from these messages, but I would prefer not to see them.

Thierry, my understanding is the i2c-over-aux protocol is a simplified version of the i2c protocol. From a DT perspective would a dpaux device ever have i2c client devices as children like a normal i2c device has? I am wondering if it would be valid in this case to tell the i2c-core not to search for children. Although that sounds like a hack. I am not sure if the i2c folks would allow us to make these dev_dbg() prints.

Quoted 44 lines · Jon Hunter

Quoted 2 lines · Thierry Reding

I'd still clearly prefer to have the pinctrl code live directly in the DPAUX driver, so I think we should at least give that a shot.

I have been working on this more this week and the good news is that by using some of the pinconf-generic handlers I can simplify the code and avoid moving headers and structures around.

However, I have ran into another issue. The current binding looks like this ...

              dpaux: dpaux@0,545c0000 {
                      compatible = "nvidia,tegra124-dpaux";
                      reg = <0x0 0x545c0000 0x0 0x40000>;
                      interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
                      clocks = <&tegra_car TEGRA124_CLK_DPAUX>,
                               <&tegra_car TEGRA124_CLK_PLL_DP>;
                      clock-names = "dpaux", "parent";
                      resets = <&tegra_car 181>;
                      reset-names = "dpaux";
                      status = "disabled";

                      dpaux_state: dpaux_state0 {
                              dpaux {
                                      groups = "dpaux_io";
                                      function = "dpaux";
                                      nvidia,dpaux-drvi;
                                      nvidia,dpaux-drvz;
                                      nvidia,dpaux-cmh;
                              };
                      };

                      i2c_state: i2c_state0 {
                              i2c {
                                      groups = "dpaux_io";
                                      function = "i2c";
                              };
                      };

This all works well, however, because the display-port binding now has child devices which are not i2c clients I see the following messages during kernel boot ...

Quoted 4 lines

[    1.607606] i2c i2c-6: of_i2c: modalias failure on
/host1x@0,50000000/dpaux@0,545c0000/dpaux_state0
[    1.616658] i2c i2c-6: of_i2c: modalias failure on

/host1x@0,50000000/dpaux@0,545c0000/i2c_state0

Hmm. The DT binding doc for dpaux doesn't say anything about the device being an I2C controller and hence inheriting/aggregating the core I2C schema. It should...

Equally, being an I2C controller means the node should have #address-cells/#size-cells properties for the I2C address.

Quoted 5 lines

In other words, i2c_add_adapter() (which is called by probing the dpaux) then parses the child nodes looking for i2c client devices. However, these child devices are not i2c client devices and hence the above error messages. I can't find an easy way to avoid this. There is no side-effect from these messages, but I would prefer not to see them.

If this were a completely new binding, I think the best way would be to have the dpaux node contain a child node for each semantic service implemented by the device, e.g.:

dpaux {
    compatible = "nvidia,tegra124-dpaux";
    ...
    pinctrl {
        dpaux_state: dpaux_state0 {
            ...
        i2c_state: i2c_state0 {
            ...
    };
    i2c-bus {
        // i2c devices go here
        // I2C core pointed at this sub-node, not the dpaux node
    };
};

I guess we can't change the binding this way since it already exists, unless we introduce a new compatible value to distinguish the old and new styles.

Perhaps i2c_add_adapter should only attempt to instantiate devices for sub-nodes that contain a compatible and/or a reg property?

Quoted 2 lines · Thierry Reding

Quoted 2 lines

Quoted 2 lines

I don't see any conceptual reason why the driver that binds to that node can't register as both a pinctrl driver plus anything else it needs to.

(...)

Quoted 6 lines

Quoted 4 lines

Looking at it there should not be a problem here with regard to the driver_data member of the device struct and so I don't see why the tegra_dpaux_probe() could not call pinctrl_register() to register the device.

Yes, I think that'd be the best solution.

I'm pretty much ready to go to any compromises to get DRM/GPU drivers to use internal kernel subsystems. The tendency there is to reimplement all kernel driver frameworks and hammer registers they need to access.

There are good reasons for. These drivers are usually so complex that things like probing and power up/down become a nightmare with cross-subsystem dependencies.

They are a special case. I had a long discussion with Intel's Daniel Vetter about this and they (Intel) eventually used GPIO for some stuff where it would fit nicely, but didn't go to use fixed regulators as I had suggested.

Quoted 3 lines

Quoted 3 lines

However, it does mean that all the pinctrl/pinmux/pinconf ops for this pinctrl device will need to live in drivers/gpu/drm/tegra/dpaux.c which is fine,

Yeah that's cool I already have e.g. GPIO chips all over the map, including DRM IIRC.

Quoted 3 lines

Quoted 3 lines

but I *believe* that would require moving drivers/pinctrl/pinctrl-utils.h to include/linux/pinctrl/ in order to make use of these functions.

Well I originally intended that to be private and neat, but whatever. Call it pinctrl-utils-internal.h or something then.

Quoted 2 lines · Linus Walleij

Well I originally intended that to be private and neat, but whatever. Call it pinctrl-utils-internal.h or something then.

Sorry for the bikeshed: perhaps s/internal/provider/? If all providers would need this, does it even make sense to merge it into include/linuxp/pinctrl/pinctrl.h, which defines all the other stuff providers use?