[3/7] dt-bindings: pinctrl: ds90ux9xx: add description of TI DS90Ux9xx pinmux

Message ID 20181008211205.2900-4-vz@mleia.com
State Changes Requested
Headers show
Series
  • mfd/pinctrl: add initial support of TI DS90Ux9xx ICs
Related show

Commit Message

Vladimir Zapolskiy Oct. 8, 2018, 9:12 p.m.
From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

TI DS90Ux9xx de-/serializers have a capability to multiplex pin functions,
in particular a pin may have selectable functions of GPIO, GPIO line
transmitter, one of I2S lines, one of RGB24 video signal lines and so on.

The change adds a description of DS90Ux9xx pin multiplexers and GPIO
controllers.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 .../bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt | 83 +++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt

Comments

Linus Walleij Oct. 10, 2018, 8:45 a.m. | #1
Hi Vladimir,

thanks for your patch!

Can we change the subject to something like "add DT bindings" rather than
"add description" as it is more specific and makes it easier for me as
maintainer.

On Mon, Oct 8, 2018 at 11:12 PM Vladimir Zapolskiy <vz@mleia.com> wrote:

> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>
> TI DS90Ux9xx de-/serializers have a capability to multiplex pin functions,
> in particular a pin may have selectable functions of GPIO, GPIO line
> transmitter, one of I2S lines, one of RGB24 video signal lines and so on.
>
> The change adds a description of DS90Ux9xx pin multiplexers and GPIO
> controllers.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
(...)
> +- gpio-ranges: Mapping to pin controller pins (as described in
> +       Documentation/devicetree/bindings/gpio/gpio.txt)
> +
> +Optional properties:
> +- ti,video-depth-18bit: Sets video bridge pins to RGB 18-bit mode.
> +
> +Available pins, groups and functions (reference to device datasheets):

Please reference the generic binding you're using here:
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

Apart from these small nitpicks it looks very standard and good.

Yours,
Linus Walleij
Laurent Pinchart Oct. 12, 2018, 12:01 p.m. | #2
Hi Vladimir,

Thank you for the patch.

On Tuesday, 9 October 2018 00:12:01 EEST Vladimir Zapolskiy wrote:
> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> 
> TI DS90Ux9xx de-/serializers have a capability to multiplex pin functions,
> in particular a pin may have selectable functions of GPIO, GPIO line
> transmitter, one of I2S lines, one of RGB24 video signal lines and so on.
> 
> The change adds a description of DS90Ux9xx pin multiplexers and GPIO
> controllers.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---
>  .../bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt | 83 +++++++++++++++++++
>  1 file changed, 83 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
> 
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
> b/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt new
> file mode 100644
> index 000000000000..fbfa1a3cdf9f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
> @@ -0,0 +1,83 @@
> +TI DS90Ux9xx de-/serializer pinmux and GPIO subcontroller
> +
> +Required properties:
> +- compatible: Must contain a generic "ti,ds90ux9xx-pinctrl" value and
> +	may contain one more specific value from the list:
> +	"ti,ds90ux925-pinctrl",
> +	"ti,ds90ux926-pinctrl",
> +	"ti,ds90ux927-pinctrl",
> +	"ti,ds90ux928-pinctrl",
> +	"ti,ds90ux940-pinctrl".

No need for a subnode, you can mark the main DT node with gpio-controller 
directly.

> +- gpio-controller: Marks the device node as a GPIO controller.
> +
> +- #gpio-cells: Must be set to 2,
> +	- the first cell is the GPIO offset number within the controller,
> +	- the second cell is used to specify the GPIO line polarity.
> +
> +- gpio-ranges: Mapping to pin controller pins (as described in
> +	Documentation/devicetree/bindings/gpio/gpio.txt)
> +
> +Optional properties:
> +- ti,video-depth-18bit: Sets video bridge pins to RGB 18-bit mode.

Please use standard properties to configure bus width. There is one defined in 
Documentation/devicetree/bindings/media/video-interfaces.txt.

> +Available pins, groups and functions (reference to device datasheets):
> +
> +function: "gpio" ("gpio4" is on DS90Ux925 and DS90Ux926 only,
> +		  "gpio9" is on DS90Ux940 only)
> + - pins: "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6",
> +	 "gpio7", "gpio8", "gpio9"
> +
> +function: "gpio-remote"
> + - pins: "gpio0", "gpio1", "gpio2", "gpio3"
> +
> +function: "pass" (DS90Ux940 specific only)
> + - pins: "gpio0", "gpio3"

What do those functions mean ?

> +function: "i2s-1"
> + - group: "i2s-1"
> +
> +function: "i2s-2"
> + - group: "i2s-2"
> +
> +function: "i2s-3" (DS90Ux927, DS90Ux928 and DS90Ux940 specific only)
> + - group: "i2s-3"
> +
> +function: "i2s-4" (DS90Ux927, DS90Ux928 and DS90Ux940 specific only)
> + - group: "i2s-4"
> +
> +function: "i2s-m" (DS90Ux928 and DS90Ux940 specific only)
> + - group: "i2s-m"

Do we really need all this ? I think a better model would be to describe the 
audio interfaces explicitly, and configure pinmuxing automatically based on 
which audio interfaces are in use.

> +function: "parallel" (DS90Ux925 and DS90Ux926 specific only)
> + - group: "parallel"
> +
> +Example (deserializer with pins GPIO[3:0] set to bridged output
> +	 function and pin GPIO4 in standard hogged GPIO function):
> +
> +deserializer {
> +	compatible = "ti,ds90ub928q", "ti,ds90ux9xx";
> +
> +	ds90ux928_pctrl: pin-controller {
> +		compatible = "ti,ds90ux928-pinctrl", "ti,ds90ux9xx-pinctrl";
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		gpio-ranges = <&ds90ux928_pctrl 0 0 8>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&ds90ux928_pins>;
> +
> +		ds90ux928_pins: pinmux {
> +			gpio-remote {
> +				pins = "gpio0", "gpio1", "gpio2", "gpio3";
> +				function = "gpio-remote";
> +			};
> +		};
> +
> +		rst {
> +			gpio-hog;
> +			gpios = <4 GPIO_ACTIVE_HIGH>;
> +			output-high;
> +		};
> +	};
> +};
Vladimir Zapolskiy Oct. 13, 2018, 1:47 p.m. | #3
Hi Laurent,

thank you for review, please find my comments below.

On 10/12/2018 03:01 PM, Laurent Pinchart wrote:
> Hi Vladimir,
> 
> Thank you for the patch.
> 
> On Tuesday, 9 October 2018 00:12:01 EEST Vladimir Zapolskiy wrote:
>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>
>> TI DS90Ux9xx de-/serializers have a capability to multiplex pin functions,
>> in particular a pin may have selectable functions of GPIO, GPIO line
>> transmitter, one of I2S lines, one of RGB24 video signal lines and so on.
>>
>> The change adds a description of DS90Ux9xx pin multiplexers and GPIO
>> controllers.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>> ---
>>  .../bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt | 83 +++++++++++++++++++
>>  1 file changed, 83 insertions(+)
>>  create mode 100644
>> Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
>> b/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt new
>> file mode 100644
>> index 000000000000..fbfa1a3cdf9f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
>> @@ -0,0 +1,83 @@
>> +TI DS90Ux9xx de-/serializer pinmux and GPIO subcontroller
>> +
>> +Required properties:
>> +- compatible: Must contain a generic "ti,ds90ux9xx-pinctrl" value and
>> +	may contain one more specific value from the list:
>> +	"ti,ds90ux925-pinctrl",
>> +	"ti,ds90ux926-pinctrl",
>> +	"ti,ds90ux927-pinctrl",
>> +	"ti,ds90ux928-pinctrl",
>> +	"ti,ds90ux940-pinctrl".
> 
> No need for a subnode, you can mark the main DT node with gpio-controller 
> directly.

If the IC is seen as an MFD, and you guess I highly prefer it and I object
the "overkill" argument, then the subnode is requred.

Also the more complicated part of the subcontroller and its device driver
is to provide pinmuxing function to consumers rather than to allow GPIO
line configuration.

The pinctrl/GPIO driver can not be alloyed with the base driver's code
to sustain maintainability, so it will reside in drivers/pinctrl as
a separate cell driver, and by the way that is the reason why it earns
its own very non-trivial DT binding description documentation.

>> +- gpio-controller: Marks the device node as a GPIO controller.
>> +
>> +- #gpio-cells: Must be set to 2,
>> +	- the first cell is the GPIO offset number within the controller,
>> +	- the second cell is used to specify the GPIO line polarity.
>> +
>> +- gpio-ranges: Mapping to pin controller pins (as described in
>> +	Documentation/devicetree/bindings/gpio/gpio.txt)
>> +
>> +Optional properties:
>> +- ti,video-depth-18bit: Sets video bridge pins to RGB 18-bit mode.
> 
> Please use standard properties to configure bus width. There is one defined in 
> Documentation/devicetree/bindings/media/video-interfaces.txt.

Here it is not a bus width description or property, but rather it is a custom
pinmux control.

It could make sense to reduce the scope of the property to "parallel" pin
function only though, like in

	ds90ux927_0_pins: pinmux {
		parallel {
			groups = "parallel";
			function = "parallel";
			ti,video-depth-18bit;
		};
	};

Alternatively the removal of the property would be almost loseless, it is
needed just in one very specific case, please reference to the driver code
for details, there you'll find a comment in ds90ux9xx_parse_dt_properties()
function.

>> +Available pins, groups and functions (reference to device datasheets):
>> +
>> +function: "gpio" ("gpio4" is on DS90Ux925 and DS90Ux926 only,
>> +		  "gpio9" is on DS90Ux940 only)
>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6",
>> +	 "gpio7", "gpio8", "gpio9"
>> +
>> +function: "gpio-remote"
>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3"
>> +
>> +function: "pass" (DS90Ux940 specific only)
>> + - pins: "gpio0", "gpio3"
> 
> What do those functions mean ?
> 

"gpio" function should be already familiar to you.

"gpio-remote" function is the pin function for a GPIO line bridging.

"pass" function sets a pin to a status pin function for detecting
display timing issues, namely DE or Vsync length value mismatch.

>> +function: "i2s-1"
>> + - group: "i2s-1"
>> +
>> +function: "i2s-2"
>> + - group: "i2s-2"
>> +
>> +function: "i2s-3" (DS90Ux927, DS90Ux928 and DS90Ux940 specific only)
>> + - group: "i2s-3"
>> +
>> +function: "i2s-4" (DS90Ux927, DS90Ux928 and DS90Ux940 specific only)
>> + - group: "i2s-4"
>> +
>> +function: "i2s-m" (DS90Ux928 and DS90Ux940 specific only)
>> + - group: "i2s-m"
> 
> Do we really need all this ? I think a better model would be to describe the 
> audio interfaces explicitly, and configure pinmuxing automatically based on 
> which audio interfaces are in use.

Yes, all the pin functions are needed, because they are transparent pinmux
controls.

I really don't want to copy a part of gpio and pinctrl frameworks to
the driver to hunt out why a user configured audio bridging and a GPIO
line, and then something goes funny due to a pin conflict. To forget
about such very possible pin and pin function conflicts I'm happy to
shift the task to the neat Linus' frameworks.

>> +function: "parallel" (DS90Ux925 and DS90Ux926 specific only)
>> + - group: "parallel"
>> +
>> +Example (deserializer with pins GPIO[3:0] set to bridged output
>> +	 function and pin GPIO4 in standard hogged GPIO function):
>> +
>> +deserializer {
>> +	compatible = "ti,ds90ub928q", "ti,ds90ux9xx";
>> +
>> +	ds90ux928_pctrl: pin-controller {
>> +		compatible = "ti,ds90ux928-pinctrl", "ti,ds90ux9xx-pinctrl";
>> +		gpio-controller;
>> +		#gpio-cells = <2>;
>> +		gpio-ranges = <&ds90ux928_pctrl 0 0 8>;
>> +
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&ds90ux928_pins>;
>> +
>> +		ds90ux928_pins: pinmux {
>> +			gpio-remote {
>> +				pins = "gpio0", "gpio1", "gpio2", "gpio3";
>> +				function = "gpio-remote";
>> +			};
>> +		};
>> +
>> +		rst {
>> +			gpio-hog;
>> +			gpios = <4 GPIO_ACTIVE_HIGH>;
>> +			output-high;
>> +		};
>> +	};
>> +};
> 

--
Best wishes,
Vladimir
Laurent Pinchart Oct. 16, 2018, 12:48 p.m. | #4
Hi Vladimir,

On Saturday, 13 October 2018 16:47:48 EEST Vladimir Zapolskiy wrote:
> On 10/12/2018 03:01 PM, Laurent Pinchart wrote:
> > On Tuesday, 9 October 2018 00:12:01 EEST Vladimir Zapolskiy wrote:
> >> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> >> 
> >> TI DS90Ux9xx de-/serializers have a capability to multiplex pin
> >> functions, in particular a pin may have selectable functions of GPIO,
> >> GPIO line transmitter, one of I2S lines, one of RGB24 video signal lines
> >> and so on.
> >> 
> >> The change adds a description of DS90Ux9xx pin multiplexers and GPIO
> >> controllers.
> >> 
> >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> >> ---
> >> 
> >>  .../bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt | 83 +++++++++++++++++++
> >>  1 file changed, 83 insertions(+)
> >>  create mode 100644
> >> 
> >> Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
> >> b/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt new
> >> file mode 100644
> >> index 000000000000..fbfa1a3cdf9f
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
> >> @@ -0,0 +1,83 @@
> >> +TI DS90Ux9xx de-/serializer pinmux and GPIO subcontroller
> >> +
> >> +Required properties:
> >> +- compatible: Must contain a generic "ti,ds90ux9xx-pinctrl" value and
> >> +	may contain one more specific value from the list:
> >> +	"ti,ds90ux925-pinctrl",
> >> +	"ti,ds90ux926-pinctrl",
> >> +	"ti,ds90ux927-pinctrl",
> >> +	"ti,ds90ux928-pinctrl",
> >> +	"ti,ds90ux940-pinctrl".
> > 
> > No need for a subnode, you can mark the main DT node with gpio-controller
> > directly.
> 
> If the IC is seen as an MFD, and you guess I highly prefer it and I object
> the "overkill" argument, then the subnode is requred.
> 
> Also the more complicated part of the subcontroller and its device driver
> is to provide pinmuxing function to consumers rather than to allow GPIO
> line configuration.
> 
> The pinctrl/GPIO driver can not be alloyed with the base driver's code
> to sustain maintainability, so it will reside in drivers/pinctrl as
> a separate cell driver, and by the way that is the reason why it earns
> its own very non-trivial DT binding description documentation.
> 
> >> +- gpio-controller: Marks the device node as a GPIO controller.
> >> +
> >> +- #gpio-cells: Must be set to 2,
> >> +	- the first cell is the GPIO offset number within the controller,
> >> +	- the second cell is used to specify the GPIO line polarity.
> >> +
> >> +- gpio-ranges: Mapping to pin controller pins (as described in
> >> +	Documentation/devicetree/bindings/gpio/gpio.txt)
> >> +
> >> +Optional properties:
> >> +- ti,video-depth-18bit: Sets video bridge pins to RGB 18-bit mode.
> > 
> > Please use standard properties to configure bus width. There is one
> > defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
> 
> Here it is not a bus width description or property, but rather it is a
> custom pinmux control.
> 
> It could make sense to reduce the scope of the property to "parallel" pin
> function only though, like in
> 
> 	ds90ux927_0_pins: pinmux {
> 		parallel {
> 			groups = "parallel";
> 			function = "parallel";
> 			ti,video-depth-18bit;
> 		};
> 	};
> 
> Alternatively the removal of the property would be almost loseless, it is
> needed just in one very specific case, please reference to the driver code
> for details, there you'll find a comment in ds90ux9xx_parse_dt_properties()
> function.

Based on the information I gathered from the DS90UH92[567] datasheets, the 
restriction on GPIO usage related to 18-bit mode is due to the signals being 
multiplexed on the same pins on DS90UH925. It could also be that the FPD-Link 
protocol itself can't carry both GPIO and the extra 6 colour bits.

In any case, I don't think the property belongs here. The bus width should be 
specified in the DT bindings for the video ports, and the drivers should then 
use that information to configure other parameters, possibly GPIO-specific, if 
needed.

> >> +Available pins, groups and functions (reference to device datasheets):
> >> +
> >> +function: "gpio" ("gpio4" is on DS90Ux925 and DS90Ux926 only,
> >> +		  "gpio9" is on DS90Ux940 only)
> >> + - pins: "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6",
> >> +	 "gpio7", "gpio8", "gpio9"
> >> +
> >> +function: "gpio-remote"
> >> + - pins: "gpio0", "gpio1", "gpio2", "gpio3"
> >> +
> >> +function: "pass" (DS90Ux940 specific only)
> >> + - pins: "gpio0", "gpio3"
> > 
> > What do those functions mean ?
> 
> "gpio" function should be already familiar to you.

I assume this function is only available for the local device, not the remote 
one ?

> "gpio-remote" function is the pin function for a GPIO line bridging.
> 
> "pass" function sets a pin to a status pin function for detecting
> display timing issues, namely DE or Vsync length value mismatch.

All this is not clear at all from the proposed DT bindings, it should be 
properly documented.

> >> +function: "i2s-1"
> >> + - group: "i2s-1"
> >> +
> >> +function: "i2s-2"
> >> + - group: "i2s-2"
> >> +
> >> +function: "i2s-3" (DS90Ux927, DS90Ux928 and DS90Ux940 specific only)
> >> + - group: "i2s-3"
> >> +
> >> +function: "i2s-4" (DS90Ux927, DS90Ux928 and DS90Ux940 specific only)
> >> + - group: "i2s-4"
> >> +
> >> +function: "i2s-m" (DS90Ux928 and DS90Ux940 specific only)
> >> + - group: "i2s-m"
> > 
> > Do we really need all this ? I think a better model would be to describe
> > the audio interfaces explicitly, and configure pinmuxing automatically
> > based on which audio interfaces are in use.
> 
> Yes, all the pin functions are needed, because they are transparent pinmux
> controls.
> 
> I really don't want to copy a part of gpio and pinctrl frameworks to
> the driver to hunt out why a user configured audio bridging and a GPIO
> line, and then something goes funny due to a pin conflict. To forget
> about such very possible pin and pin function conflicts I'm happy to
> shift the task to the neat Linus' frameworks.

And I still believe this is overkill and confusing, and disagrees that this is 
an MFD device.

> >> +function: "parallel" (DS90Ux925 and DS90Ux926 specific only)
> >> + - group: "parallel"
> >> +
> >> +Example (deserializer with pins GPIO[3:0] set to bridged output
> >> +	 function and pin GPIO4 in standard hogged GPIO function):
> >> +
> >> +deserializer {
> >> +	compatible = "ti,ds90ub928q", "ti,ds90ux9xx";
> >> +
> >> +	ds90ux928_pctrl: pin-controller {
> >> +		compatible = "ti,ds90ux928-pinctrl", "ti,ds90ux9xx-pinctrl";
> >> +		gpio-controller;
> >> +		#gpio-cells = <2>;
> >> +		gpio-ranges = <&ds90ux928_pctrl 0 0 8>;
> >> +
> >> +		pinctrl-names = "default";
> >> +		pinctrl-0 = <&ds90ux928_pins>;
> >> +
> >> +		ds90ux928_pins: pinmux {
> >> +			gpio-remote {
> >> +				pins = "gpio0", "gpio1", "gpio2", "gpio3";
> >> +				function = "gpio-remote";
> >> +			};
> >> +		};
> >> +
> >> +		rst {
> >> +			gpio-hog;
> >> +			gpios = <4 GPIO_ACTIVE_HIGH>;
> >> +			output-high;
> >> +		};
> >> +	};
> >> +};
Rob Herring Oct. 17, 2018, 3:02 p.m. | #5
On Wed, Oct 10, 2018 at 10:45:43AM +0200, Linus Walleij wrote:
> Hi Vladimir,
> 
> thanks for your patch!
> 
> Can we change the subject to something like "add DT bindings" rather than
> "add description" as it is more specific and makes it easier for me as
> maintainer.

To add to the nitpicking, The subject already says DT and bindings, so 
no need to repeat it. I'd just drop "description of" if anything.

Rob
Luca Ceresoli Oct. 30, 2018, 4:44 p.m. | #6
Hi Vladimir,

On 16/10/18 14:48, Laurent Pinchart wrote:
> Hi Vladimir,
> 
> On Saturday, 13 October 2018 16:47:48 EEST Vladimir Zapolskiy wrote:
>> On 10/12/2018 03:01 PM, Laurent Pinchart wrote:
>>> On Tuesday, 9 October 2018 00:12:01 EEST Vladimir Zapolskiy wrote:
>>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>>>
>>>> TI DS90Ux9xx de-/serializers have a capability to multiplex pin
>>>> functions, in particular a pin may have selectable functions of GPIO,
>>>> GPIO line transmitter, one of I2S lines, one of RGB24 video signal lines
>>>> and so on.
>>>>
>>>> The change adds a description of DS90Ux9xx pin multiplexers and GPIO
>>>> controllers.
>>>>
>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]
>>>> +Available pins, groups and functions (reference to device datasheets):
>>>> +
>>>> +function: "gpio" ("gpio4" is on DS90Ux925 and DS90Ux926 only,
>>>> +		  "gpio9" is on DS90Ux940 only)
>>>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6",
>>>> +	 "gpio7", "gpio8", "gpio9"
>>>> +
>>>> +function: "gpio-remote"
>>>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3"
>>>> +
>>>> +function: "pass" (DS90Ux940 specific only)
>>>> + - pins: "gpio0", "gpio3"
>>>
>>> What do those functions mean ?
>>
>> "gpio" function should be already familiar to you.
> 
> I assume this function is only available for the local device, not the remote 
> one ?
> 
>> "gpio-remote" function is the pin function for a GPIO line bridging.
>>
>> "pass" function sets a pin to a status pin function for detecting
>> display timing issues, namely DE or Vsync length value mismatch.
> 
> All this is not clear at all from the proposed DT bindings, it should be 
> properly documented.

It's not clear to me as well. The "gpio-remote" can mean two different
things (at least in the camera serdes TI chips):

 - a GPIO input on the the *local* chip, replicated as an output on the
   *remote* chip
 - a GPIO input on the the *remote* chip, replicated as an output on the
   *local* chip

How to you differentiate them in DT?

The "pass" function is also not clear. A comprehensive example would
help a lot.

Bye,
Vladimir Zapolskiy Oct. 31, 2018, 8:31 p.m. | #7
Hi Luca,

On 10/30/2018 06:44 PM, Luca Ceresoli wrote:
> Hi Vladimir,
> 
> On 16/10/18 14:48, Laurent Pinchart wrote:
>> Hi Vladimir,
>>
>> On Saturday, 13 October 2018 16:47:48 EEST Vladimir Zapolskiy wrote:
>>> On 10/12/2018 03:01 PM, Laurent Pinchart wrote:
>>>> On Tuesday, 9 October 2018 00:12:01 EEST Vladimir Zapolskiy wrote:
>>>>> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>>>>
>>>>> TI DS90Ux9xx de-/serializers have a capability to multiplex pin
>>>>> functions, in particular a pin may have selectable functions of GPIO,
>>>>> GPIO line transmitter, one of I2S lines, one of RGB24 video signal lines
>>>>> and so on.
>>>>>
>>>>> The change adds a description of DS90Ux9xx pin multiplexers and GPIO
>>>>> controllers.
>>>>>
>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> [...]
>>>>> +Available pins, groups and functions (reference to device datasheets):
>>>>> +
>>>>> +function: "gpio" ("gpio4" is on DS90Ux925 and DS90Ux926 only,
>>>>> +		  "gpio9" is on DS90Ux940 only)
>>>>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6",
>>>>> +	 "gpio7", "gpio8", "gpio9"
>>>>> +
>>>>> +function: "gpio-remote"
>>>>> + - pins: "gpio0", "gpio1", "gpio2", "gpio3"
>>>>> +
>>>>> +function: "pass" (DS90Ux940 specific only)
>>>>> + - pins: "gpio0", "gpio3"
>>>>
>>>> What do those functions mean ?
>>>
>>> "gpio" function should be already familiar to you.
>>
>> I assume this function is only available for the local device, not the remote 
>> one ?
>>
>>> "gpio-remote" function is the pin function for a GPIO line bridging.
>>>
>>> "pass" function sets a pin to a status pin function for detecting
>>> display timing issues, namely DE or Vsync length value mismatch.
>>
>> All this is not clear at all from the proposed DT bindings, it should be 
>> properly documented.
> 
> It's not clear to me as well. The "gpio-remote" can mean two different
> things (at least in the camera serdes TI chips):
> 
>  - a GPIO input on the the *local* chip, replicated as an output on the
>    *remote* chip
>  - a GPIO input on the the *remote* chip, replicated as an output on the
>    *local* chip
> 
> How to you differentiate them in DT?
> 

"gpio-remote" function is directly translated into "GPIOx Remote Enable"
bit setting, the documentation says:

1) Deserializer IC pin configuration:

	Enable GPIO control from remote Serializer. The GPIO pin will
	be an output, and the value is received from the remote Serializer.

2) Serializer IC pin configuration:

	Enable GPIO control from remote Deserializer. The GPIO pin will
	be an output, and the value is received from the remote Deserializer.

So, it is always an output signal, the line signal is "bridged" (repeated)
as a corresponding line signal on a remote IC, note that there is no
difference between serializer and deserializer ICs.

> The "pass" function is also not clear. A comprehensive example would
> help a lot.

As this devicetree documentation says, the "pass" pin function is specific
for DS90Ux940 deserializer, I would suggest to check its datasheet for
getting a comprehensive answer, but I've already copy-pasted information
from the datasheet into my previous answer to Laurent.

The reason why the "pass" pin function is listed is quite simple, the
pin function interferes other pin functions, see DS90Ux940 GPIO0 and
GPIO3 controls, I hope I've managed to describe it properly by
DS90UX940_GPIO(0, ...) and DS90UX940_GPIO(3, ...) pin descriptions in
the pinctrl driver.

--
Best wishes,
Vladimir

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
new file mode 100644
index 000000000000..fbfa1a3cdf9f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/ti,ds90ux9xx-pinctrl.txt
@@ -0,0 +1,83 @@ 
+TI DS90Ux9xx de-/serializer pinmux and GPIO subcontroller
+
+Required properties:
+- compatible: Must contain a generic "ti,ds90ux9xx-pinctrl" value and
+	may contain one more specific value from the list:
+	"ti,ds90ux925-pinctrl",
+	"ti,ds90ux926-pinctrl",
+	"ti,ds90ux927-pinctrl",
+	"ti,ds90ux928-pinctrl",
+	"ti,ds90ux940-pinctrl".
+
+- gpio-controller: Marks the device node as a GPIO controller.
+
+- #gpio-cells: Must be set to 2,
+	- the first cell is the GPIO offset number within the controller,
+	- the second cell is used to specify the GPIO line polarity.
+
+- gpio-ranges: Mapping to pin controller pins (as described in
+	Documentation/devicetree/bindings/gpio/gpio.txt)
+
+Optional properties:
+- ti,video-depth-18bit: Sets video bridge pins to RGB 18-bit mode.
+
+Available pins, groups and functions (reference to device datasheets):
+
+function: "gpio" ("gpio4" is on DS90Ux925 and DS90Ux926 only,
+		  "gpio9" is on DS90Ux940 only)
+ - pins: "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6",
+	 "gpio7", "gpio8", "gpio9"
+
+function: "gpio-remote"
+ - pins: "gpio0", "gpio1", "gpio2", "gpio3"
+
+function: "pass" (DS90Ux940 specific only)
+ - pins: "gpio0", "gpio3"
+
+function: "i2s-1"
+ - group: "i2s-1"
+
+function: "i2s-2"
+ - group: "i2s-2"
+
+function: "i2s-3" (DS90Ux927, DS90Ux928 and DS90Ux940 specific only)
+ - group: "i2s-3"
+
+function: "i2s-4" (DS90Ux927, DS90Ux928 and DS90Ux940 specific only)
+ - group: "i2s-4"
+
+function: "i2s-m" (DS90Ux928 and DS90Ux940 specific only)
+ - group: "i2s-m"
+
+function: "parallel" (DS90Ux925 and DS90Ux926 specific only)
+ - group: "parallel"
+
+Example (deserializer with pins GPIO[3:0] set to bridged output
+	 function and pin GPIO4 in standard hogged GPIO function):
+
+deserializer {
+	compatible = "ti,ds90ub928q", "ti,ds90ux9xx";
+
+	ds90ux928_pctrl: pin-controller {
+		compatible = "ti,ds90ux928-pinctrl", "ti,ds90ux9xx-pinctrl";
+		gpio-controller;
+		#gpio-cells = <2>;
+		gpio-ranges = <&ds90ux928_pctrl 0 0 8>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&ds90ux928_pins>;
+
+		ds90ux928_pins: pinmux {
+			gpio-remote {
+				pins = "gpio0", "gpio1", "gpio2", "gpio3";
+				function = "gpio-remote";
+			};
+		};
+
+		rst {
+			gpio-hog;
+			gpios = <4 GPIO_ACTIVE_HIGH>;
+			output-high;
+		};
+	};
+};