Hi,
Thanks for reviews of previous iterations.
This patchset introduces USB physical connector bindings, together with working example. I have removed RFC prefix - the patchset seems to be heading to a happy end :)
v5: fixed extra parenthesis in dts, renamed extcon function. v4: improved binding descriptions, added missing reg in dts. v3: Separate binding for Samsung 11-pin connector, added full-blown USB-C example. v2: I have addressed comments by Rob and Laurent, thanks
Changes in datail are described in the patches.
Regards Andrzej
Andrzej Hajda (5): dt-bindings: add bindings for USB physical connector dt-bindings: add bindings for Samsung micro-USB 11-pin connector arm64: dts: exynos: add micro-USB connector node to TM2 platforms arm64: dts: exynos: add OF graph between MHL and USB connector extcon: add possibility to get extcon device by OF node
Maciej Purski (1): drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL
.../connector/samsung,usb-connector-11pin.txt | 49 +++++++++++ .../bindings/connector/usb-connector.txt | 75 +++++++++++++++++ .../boot/dts/exynos/exynos5433-tm2-common.dtsi | 38 ++++++++- drivers/extcon/extcon.c | 44 +++++++--- drivers/gpu/drm/bridge/sil-sii8620.c | 97 +++++++++++++++++++++- include/linux/extcon.h | 6 ++ 6 files changed, 293 insertions(+), 16 deletions(-) create mode 100644 Documentation/devicetree/bindings/connector/samsung,usb-connector-11pin.txt create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
These bindings allow to describe most known standard USB connectors and it should be possible to extend it if necessary. USB connectors, beside USB can be used to route other protocols, for example UART, Audio, MHL. In such case every device passing data through the connector should have appropriate graph bindings.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com --- v4: - improved 'type' description (Rob), - improved description of 2nd example (Rob). v3: - removed MHL port (samsung connector will have separate bindings), - added 2nd example for USB-C, - improved formatting. v2: - moved connector type(A,B,C) to compatible string (Rob), - renamed size property to type (Rob), - changed type description to be less confusing (Laurent), - removed vendor specific compatibles (implied by graph port number), - added requirement of connector being a child of IC (Rob), - removed max-mode (subtly suggested by Rob, it should be detected anyway by USB Controller in runtime, downside is that device is not able to report its real capabilities, maybe better would be to make it optional(?)), - assigned port numbers to data buses (Rob).
Regards Andrzej --- .../bindings/connector/usb-connector.txt | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt new file mode 100644 index 000000000000..e1463f14af38 --- /dev/null +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt @@ -0,0 +1,75 @@ +USB Connector +============= + +USB connector node represents physical USB connector. It should be +a child of USB interface controller. + +Required properties: +- compatible: describes type of the connector, must be one of: + "usb-a-connector", + "usb-b-connector", + "usb-c-connector". + +Optional properties: +- label: symbolic name for the connector, +- type: size of the connector, should be specified in case of USB-A, USB-B + non-fullsize connectors: "mini", "micro". + +Required nodes: +- any data bus to the connector should be modeled using the OF graph bindings + specified in bindings/graph.txt, unless the bus is between parent node and + the connector. Since single connector can have multpile data buses every bus + has assigned OF graph port number as follows: + 0: High Speed (HS), present in all connectors, + 1: Super Speed (SS), present in SS capable connectors, + 2: Sideband use (SBU), present in USB-C. + +Examples +-------- + +1. Micro-USB connector with HS lines routed via controller (MUIC): + +muic-max77843@66 { + ... + usb_con: connector { + compatible = "usb-b-connector"; + label = "micro-USB"; + type = "micro"; + }; +}; + +2. USB-C connector attached to CC controller (s2mm005), HS lines routed +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort. +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY. + +ccic: s2mm005@33 { + ... + usb_con: connector { + compatible = "usb-c-connector"; + label = "USB-C"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + usb_con_hs: endpoint { + remote-endpoint = <&max77865_usbc_hs>; + }; + }; + port@1 { + reg = <1>; + usb_con_ss: endpoint { + remote-endpoint = <&usbdrd_phy_ss>; + }; + }; + port@2 { + reg = <2>; + usb_con_sbu: endpoint { + remote-endpoint = <&dp_aux>; + }; + }; + }; + }; +};
Hi,
On Tue, Feb 27, 2018 at 08:11:29AM +0100, Andrzej Hajda wrote:
+2. USB-C connector attached to CC controller (s2mm005), HS lines routed +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort. +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
+ccic: s2mm005@33 {
- ...
- usb_con: connector {
compatible = "usb-c-connector";label = "USB-C";
Is this child node really necessary? There will never be more then one connector per CC line.
We should prefer device_graph* functions over of_graph* and acpi_graph* functions in the drivers so we don't have to handle the same thing multiple times with separate APIs. Is it still possible if there is that connector child node?
ports {#address-cells = <1>;#size-cells = <0>;port@0 {reg = <0>;usb_con_hs: endpoint {remote-endpoint = <&max77865_usbc_hs>;};};port@1 {reg = <1>;usb_con_ss: endpoint {remote-endpoint = <&usbdrd_phy_ss>;};};port@2 {reg = <2>;usb_con_sbu: endpoint {remote-endpoint = <&dp_aux>;};};};- };
+};
Thanks,
On 02.03.2018 14:13, Heikki Krogerus wrote:
Hi,
On Tue, Feb 27, 2018 at 08:11:29AM +0100, Andrzej Hajda wrote:
+2. USB-C connector attached to CC controller (s2mm005), HS lines routed +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort. +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
+ccic: s2mm005@33 {
- ...
- usb_con: connector {
compatible = "usb-c-connector";label = "USB-C";Is this child node really necessary? There will never be more then one connector per CC line.
But there can be more connectors/cc-lines per IC, for example EZ-PD CCG5[1].
[1]: http://www.cypress.com/products/ez-pd-ccg5-two-port-usb-type-c-and-power-del...
We should prefer device_graph* functions over of_graph* and
I guess you mean fwnode_graph* functions.
acpi_graph* functions in the drivers so we don't have to handle the same thing multiple times with separate APIs. Is it still possible if there is that connector child node?
Bindings proposed here are OF bindings, I suppose the most important is to follow OF specification and guidelines and these bindings tries to follow it. It looks like it should not be a problem for fwnode framework to handle such bindings, but it is just my guess. I have not seen any fwnode* specification I am not sure what is the real purpose of this framework, but it seems to be just in-kernel abstraction for different firmware standards (OF, ACPI), so even if it lacks at the moment some functionality it should not be a barrier for OF bindings.
Regards Andrzej
ports {#address-cells = <1>;#size-cells = <0>;port@0 {reg = <0>;usb_con_hs: endpoint {remote-endpoint = <&max77865_usbc_hs>;};};port@1 {reg = <1>;usb_con_ss: endpoint {remote-endpoint = <&usbdrd_phy_ss>;};};port@2 {reg = <2>;usb_con_sbu: endpoint {remote-endpoint = <&dp_aux>;};};};- };
+};
Thanks,
On Mon, Mar 05, 2018 at 09:18:10AM +0100, Andrzej Hajda wrote:
On 02.03.2018 14:13, Heikki Krogerus wrote:
Hi,
On Tue, Feb 27, 2018 at 08:11:29AM +0100, Andrzej Hajda wrote:
+2. USB-C connector attached to CC controller (s2mm005), HS lines routed +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort. +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
+ccic: s2mm005@33 {
- ...
- usb_con: connector {
compatible = "usb-c-connector";label = "USB-C";Is this child node really necessary? There will never be more then one connector per CC line.
But there can be more connectors/cc-lines per IC, for example EZ-PD CCG5[1].
OK, in that case the child node is of course needed.
We should prefer device_graph* functions over of_graph* and
I guess you mean fwnode_graph* functions.
Yes.
acpi_graph* functions in the drivers so we don't have to handle the same thing multiple times with separate APIs. Is it still possible if there is that connector child node?
Bindings proposed here are OF bindings, I suppose the most important is to follow OF specification and guidelines and these bindings tries to follow it. It looks like it should not be a problem for fwnode framework to handle such bindings, but it is just my guess. I have not seen any fwnode* specification I am not sure what is the real purpose of this framework, but it seems to be just in-kernel abstraction for different firmware standards (OF, ACPI), so even if it lacks at the moment some functionality it should not be a barrier for OF bindings.
Sure thing.
Thanks,
On Tue, Feb 27, 2018 at 08:11:29AM +0100, Andrzej Hajda wrote:
These bindings allow to describe most known standard USB connectors and it should be possible to extend it if necessary. USB connectors, beside USB can be used to route other protocols, for example UART, Audio, MHL. In such case every device passing data through the connector should have appropriate graph bindings.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com
v4:
- improved 'type' description (Rob),
- improved description of 2nd example (Rob).
v3:
- removed MHL port (samsung connector will have separate bindings),
- added 2nd example for USB-C,
- improved formatting.
v2:
- moved connector type(A,B,C) to compatible string (Rob),
- renamed size property to type (Rob),
- changed type description to be less confusing (Laurent),
- removed vendor specific compatibles (implied by graph port number),
- added requirement of connector being a child of IC (Rob),
- removed max-mode (subtly suggested by Rob, it should be detected anyway by USB Controller in runtime, downside is that device is not able to report its real capabilities, maybe better would be to make it optional(?)),
- assigned port numbers to data buses (Rob).
Regards Andrzej
.../bindings/connector/usb-connector.txt | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
Reviewed-by: Rob Herring robh@kernel.org
Hi,
On 27/02/18 09:11, Andrzej Hajda wrote:
These bindings allow to describe most known standard USB connectors and it should be possible to extend it if necessary. USB connectors, beside USB can be used to route other protocols, for example UART, Audio, MHL. In such case every device passing data through the connector should have appropriate graph bindings.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com
v4:
- improved 'type' description (Rob),
- improved description of 2nd example (Rob).
v3:
- removed MHL port (samsung connector will have separate bindings),
- added 2nd example for USB-C,
- improved formatting.
v2:
- moved connector type(A,B,C) to compatible string (Rob),
- renamed size property to type (Rob),
- changed type description to be less confusing (Laurent),
- removed vendor specific compatibles (implied by graph port number),
- added requirement of connector being a child of IC (Rob),
- removed max-mode (subtly suggested by Rob, it should be detected anyway by USB Controller in runtime, downside is that device is not able to report its real capabilities, maybe better would be to make it optional(?)),
- assigned port numbers to data buses (Rob).
Regards Andrzej
.../bindings/connector/usb-connector.txt | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt new file mode 100644 index 000000000000..e1463f14af38 --- /dev/null +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt @@ -0,0 +1,75 @@ +USB Connector +=============
+USB connector node represents physical USB connector. It should be +a child of USB interface controller.
+Required properties: +- compatible: describes type of the connector, must be one of:
- "usb-a-connector",
- "usb-b-connector",
- "usb-c-connector".
compatible should be just "usb-connector"
Type should be a property
type: type of usb connector "A", "B", "AB", "C" AB is for dual-role connectors.
micro super-speed and high-speed connectors are different. How do you differentiate that?
+Optional properties: +- label: symbolic name for the connector,
Why do you need label? We can't maintain consistency as people will put creative names there. Device/bus driver could generate a valid label.
+- type: size of the connector, should be specified in case of USB-A, USB-B
- non-fullsize connectors: "mini", "micro".
type is misleading. Type is usually A/B/C. It should be size here instead.
size: size of the connector if not standard size. "mini", "micro"
If not specified it is treated as standard sized connector. e.g. for Type-C there is no mini/micro. so size doesn't have to be specificed
What about Type-C connector?
+Required nodes: +- any data bus to the connector should be modeled using the OF graph bindings
s/modeled/modelled
- specified in bindings/graph.txt, unless the bus is between parent node and
- the connector. Since single connector can have multpile data buses every bus
s/multpile/multiple
- has assigned OF graph port number as follows:
- 0: High Speed (HS), present in all connectors,
- 1: Super Speed (SS), present in SS capable connectors,
- 2: Sideband use (SBU), present in USB-C.
+Examples +--------
+1. Micro-USB connector with HS lines routed via controller (MUIC):
+muic-max77843@66 {
- ...
- usb_con: connector {
compatible = "usb-b-connector";label = "micro-USB";type = "micro";- };
+};
+2. USB-C connector attached to CC controller (s2mm005), HS lines routed +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort. +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
+ccic: s2mm005@33 {
- ...
- usb_con: connector {
compatible = "usb-c-connector";label = "USB-C";
The label is not consistent with the earlier example.
ports {#address-cells = <1>;#size-cells = <0>;port@0 {reg = <0>;usb_con_hs: endpoint {remote-endpoint = <&max77865_usbc_hs>;};};port@1 {reg = <1>;usb_con_ss: endpoint {remote-endpoint = <&usbdrd_phy_ss>;};};port@2 {reg = <2>;usb_con_sbu: endpoint {remote-endpoint = <&dp_aux>;};};};- };
+};
On 09.03.2018 11:24, Roger Quadros wrote:
Hi,
On 27/02/18 09:11, Andrzej Hajda wrote:
These bindings allow to describe most known standard USB connectors and it should be possible to extend it if necessary. USB connectors, beside USB can be used to route other protocols, for example UART, Audio, MHL. In such case every device passing data through the connector should have appropriate graph bindings.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com
v4:
- improved 'type' description (Rob),
- improved description of 2nd example (Rob).
v3:
- removed MHL port (samsung connector will have separate bindings),
- added 2nd example for USB-C,
- improved formatting.
v2:
- moved connector type(A,B,C) to compatible string (Rob),
- renamed size property to type (Rob),
- changed type description to be less confusing (Laurent),
- removed vendor specific compatibles (implied by graph port number),
- added requirement of connector being a child of IC (Rob),
- removed max-mode (subtly suggested by Rob, it should be detected anyway by USB Controller in runtime, downside is that device is not able to report its real capabilities, maybe better would be to make it optional(?)),
- assigned port numbers to data buses (Rob).
Regards Andrzej
.../bindings/connector/usb-connector.txt | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt new file mode 100644 index 000000000000..e1463f14af38 --- /dev/null +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt @@ -0,0 +1,75 @@ +USB Connector +=============
+USB connector node represents physical USB connector. It should be +a child of USB interface controller.
+Required properties: +- compatible: describes type of the connector, must be one of:
- "usb-a-connector",
- "usb-b-connector",
- "usb-c-connector".
compatible should be just "usb-connector"
Type should be a property
type: type of usb connector "A", "B", "AB", "C" AB is for dual-role connectors.
I have proposed such property (and size also) in my first RFC [1]. Rod did not like it :)
[1]: https://marc.info/?l=devicetree&m=150660411515233&w=2
micro super-speed and high-speed connectors are different. How do you differentiate that?
I am aware of it, and property differentiating it was also in my earlier iterations. If there will be need to differentiate such connectors, adding property max-speed or max-mode would do the thing.
+Optional properties: +- label: symbolic name for the connector,
Why do you need label? We can't maintain consistency as people will put creative names there. Device/bus driver could generate a valid label.
It follows convention for other connectors: HDMI, DVI, ....
+- type: size of the connector, should be specified in case of USB-A, USB-B
- non-fullsize connectors: "mini", "micro".
type is misleading. Type is usually A/B/C. It should be size here instead.
As you can see in discussion for previous iterations it follows convention already established for other connectors.
size: size of the connector if not standard size. "mini", "micro"
If not specified it is treated as standard sized connector. e.g. for Type-C there is no mini/micro. so size doesn't have to be specificed
Few lines above it is described: should be specified in case of USB-A, USB-B non-fullsize connectors.
What about Type-C connector?
+Required nodes: +- any data bus to the connector should be modeled using the OF graph bindings
s/modeled/modelled
- specified in bindings/graph.txt, unless the bus is between parent node and
- the connector. Since single connector can have multpile data buses every bus
s/multpile/multiple
OK, I will fix both typos.
- has assigned OF graph port number as follows:
- 0: High Speed (HS), present in all connectors,
- 1: Super Speed (SS), present in SS capable connectors,
- 2: Sideband use (SBU), present in USB-C.
+Examples +--------
+1. Micro-USB connector with HS lines routed via controller (MUIC):
+muic-max77843@66 {
- ...
- usb_con: connector {
compatible = "usb-b-connector";label = "micro-USB";type = "micro";- };
+};
+2. USB-C connector attached to CC controller (s2mm005), HS lines routed +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort. +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
+ccic: s2mm005@33 {
- ...
- usb_con: connector {
compatible = "usb-c-connector";label = "USB-C";The label is not consistent with the earlier example.
Why?
Regards Andrzej
ports {#address-cells = <1>;#size-cells = <0>;port@0 {reg = <0>;usb_con_hs: endpoint {remote-endpoint = <&max77865_usbc_hs>;};};port@1 {reg = <1>;usb_con_ss: endpoint {remote-endpoint = <&usbdrd_phy_ss>;};};port@2 {reg = <2>;usb_con_sbu: endpoint {remote-endpoint = <&dp_aux>;};};};- };
+};
On 12.03.2018 08:02, Andrzej Hajda wrote:
On 09.03.2018 11:24, Roger Quadros wrote:
Hi,
On 27/02/18 09:11, Andrzej Hajda wrote:
These bindings allow to describe most known standard USB connectors and it should be possible to extend it if necessary. USB connectors, beside USB can be used to route other protocols, for example UART, Audio, MHL. In such case every device passing data through the connector should have appropriate graph bindings.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com
v4:
- improved 'type' description (Rob),
- improved description of 2nd example (Rob).
v3:
- removed MHL port (samsung connector will have separate bindings),
- added 2nd example for USB-C,
- improved formatting.
v2:
- moved connector type(A,B,C) to compatible string (Rob),
- renamed size property to type (Rob),
- changed type description to be less confusing (Laurent),
- removed vendor specific compatibles (implied by graph port number),
- added requirement of connector being a child of IC (Rob),
- removed max-mode (subtly suggested by Rob, it should be detected anyway by USB Controller in runtime, downside is that device is not able to report its real capabilities, maybe better would be to make it optional(?)),
- assigned port numbers to data buses (Rob).
Regards Andrzej
.../bindings/connector/usb-connector.txt | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt new file mode 100644 index 000000000000..e1463f14af38 --- /dev/null +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt @@ -0,0 +1,75 @@ +USB Connector +=============
+USB connector node represents physical USB connector. It should be +a child of USB interface controller.
+Required properties: +- compatible: describes type of the connector, must be one of:
- "usb-a-connector",
- "usb-b-connector",
- "usb-c-connector".
compatible should be just "usb-connector"
Type should be a property
type: type of usb connector "A", "B", "AB", "C" AB is for dual-role connectors.
I have proposed such property (and size also) in my first RFC [1]. Rod did not like it :)
Ups, ugly typo, I meant Rob, of course.
Regards Andrzej
micro super-speed and high-speed connectors are different. How do you differentiate that?
I am aware of it, and property differentiating it was also in my earlier iterations. If there will be need to differentiate such connectors, adding property max-speed or max-mode would do the thing.
+Optional properties: +- label: symbolic name for the connector,
Why do you need label? We can't maintain consistency as people will put creative names there. Device/bus driver could generate a valid label.
It follows convention for other connectors: HDMI, DVI, ....
+- type: size of the connector, should be specified in case of USB-A, USB-B
- non-fullsize connectors: "mini", "micro".
type is misleading. Type is usually A/B/C. It should be size here instead.
As you can see in discussion for previous iterations it follows convention already established for other connectors.
size: size of the connector if not standard size. "mini", "micro"
If not specified it is treated as standard sized connector. e.g. for Type-C there is no mini/micro. so size doesn't have to be specificed
Few lines above it is described: should be specified in case of USB-A, USB-B non-fullsize connectors.
What about Type-C connector?
+Required nodes: +- any data bus to the connector should be modeled using the OF graph bindings
s/modeled/modelled
- specified in bindings/graph.txt, unless the bus is between parent node and
- the connector. Since single connector can have multpile data buses every bus
s/multpile/multiple
OK, I will fix both typos.
- has assigned OF graph port number as follows:
- 0: High Speed (HS), present in all connectors,
- 1: Super Speed (SS), present in SS capable connectors,
- 2: Sideband use (SBU), present in USB-C.
+Examples +--------
+1. Micro-USB connector with HS lines routed via controller (MUIC):
+muic-max77843@66 {
- ...
- usb_con: connector {
compatible = "usb-b-connector";label = "micro-USB";type = "micro";- };
+};
+2. USB-C connector attached to CC controller (s2mm005), HS lines routed +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort. +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
+ccic: s2mm005@33 {
- ...
- usb_con: connector {
compatible = "usb-c-connector";label = "USB-C";The label is not consistent with the earlier example.
Why?
Regards Andrzej
ports {#address-cells = <1>;#size-cells = <0>;port@0 {reg = <0>;usb_con_hs: endpoint {remote-endpoint = <&max77865_usbc_hs>;};};port@1 {reg = <1>;usb_con_ss: endpoint {remote-endpoint = <&usbdrd_phy_ss>;};};port@2 {reg = <2>;usb_con_sbu: endpoint {remote-endpoint = <&dp_aux>;};};};- };
+};
Andrezej,
Why don't you have any of the USB maintainers in to/cc?
Greg Kroah-Hartman gregkh@linuxfoundation.org (supporter:USB SUBSYSTEM) Felipe Balbi balbi@kernel.org (maintainer:USB GADGET/PERIPHERAL SUBSYSTEM)
On 12/03/18 09:02, Andrzej Hajda wrote:
On 09.03.2018 11:24, Roger Quadros wrote:
Hi,
On 27/02/18 09:11, Andrzej Hajda wrote:
These bindings allow to describe most known standard USB connectors and it should be possible to extend it if necessary. USB connectors, beside USB can be used to route other protocols, for example UART, Audio, MHL. In such case every device passing data through the connector should have appropriate graph bindings.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com
v4:
- improved 'type' description (Rob),
- improved description of 2nd example (Rob).
v3:
- removed MHL port (samsung connector will have separate bindings),
- added 2nd example for USB-C,
- improved formatting.
v2:
- moved connector type(A,B,C) to compatible string (Rob),
- renamed size property to type (Rob),
- changed type description to be less confusing (Laurent),
- removed vendor specific compatibles (implied by graph port number),
- added requirement of connector being a child of IC (Rob),
- removed max-mode (subtly suggested by Rob, it should be detected anyway by USB Controller in runtime, downside is that device is not able to report its real capabilities, maybe better would be to make it optional(?)),
- assigned port numbers to data buses (Rob).
Regards Andrzej
.../bindings/connector/usb-connector.txt | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt new file mode 100644 index 000000000000..e1463f14af38 --- /dev/null +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
Should this lie in bindings/usb/connector?
@@ -0,0 +1,75 @@ +USB Connector +=============
+USB connector node represents physical USB connector. It should be +a child of USB interface controller.
+Required properties: +- compatible: describes type of the connector, must be one of:
- "usb-a-connector",
- "usb-b-connector",
- "usb-c-connector".
compatible should be just "usb-connector"
Type should be a property
type: type of usb connector "A", "B", "AB", "C" AB is for dual-role connectors.
I have proposed such property (and size also) in my first RFC [1]. Rod did not like it :)
This is what Rob says here https://patchwork.kernel.org/patch/9976043/ "We did "type" for hdmi-connector, but I think I'd really prefer compatible be used to distinguish as least where it may matter to s/w. In the HDMI case, they all are pretty much the same, just different physical size."
So the question is. Does it matter to this particular software implementation if it is type A,B,C connector? If yes, how?
Type A will never have any alternate function. It is always dedicated to USB.
Also does the size "full", "micro", "mini" matter to software?
micro super-speed and high-speed connectors are different. How do you differentiate that?
I am aware of it, and property differentiating it was also in my earlier iterations. If there will be need to differentiate such connectors, adding property max-speed or max-mode would do the thing.
USB controller binding (bindings/usb/generic.txt) has the speed. I don't think there is any value for replicating it for the connector. Maybe it could be added later if needed.
+Optional properties: +- label: symbolic name for the connector,
Why do you need label? We can't maintain consistency as people will put creative names there. Device/bus driver could generate a valid label.
It follows convention for other connectors: HDMI, DVI, ....
+- type: size of the connector, should be specified in case of USB-A, USB-B
- non-fullsize connectors: "mini", "micro".
type is misleading. Type is usually A/B/C. It should be size here instead.
As you can see in discussion for previous iterations it follows convention already established for other connectors.
size: size of the connector if not standard size. "mini", "micro"
If not specified it is treated as standard sized connector. e.g. for Type-C there is no mini/micro. so size doesn't have to be specificed
Few lines above it is described: should be specified in case of USB-A, USB-B non-fullsize connectors.
What about Type-C connector?
+Required nodes: +- any data bus to the connector should be modeled using the OF graph bindings
s/modeled/modelled
- specified in bindings/graph.txt, unless the bus is between parent node and
- the connector. Since single connector can have multpile data buses every bus
s/multpile/multiple
OK, I will fix both typos.
- has assigned OF graph port number as follows:
- 0: High Speed (HS), present in all connectors,
- 1: Super Speed (SS), present in SS capable connectors,
- 2: Sideband use (SBU), present in USB-C.
+Examples +--------
+1. Micro-USB connector with HS lines routed via controller (MUIC):
+muic-max77843@66 {
- ...
- usb_con: connector {
compatible = "usb-b-connector";label = "micro-USB";type = "micro";- };
+};
+2. USB-C connector attached to CC controller (s2mm005), HS lines routed +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort. +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
+ccic: s2mm005@33 {
- ...
- usb_con: connector {
compatible = "usb-c-connector";label = "USB-C";The label is not consistent with the earlier example.
Why?
Because 1st example is "<size>-USB" and second one is "USB-<type>".
How is label going to be used? Is it being presented to end user? If yes it should indicate what's important to the user. i.e. its function. e.g. "USB-MHL" or "USB-DisplayPort"
Regards Andrzej
ports {#address-cells = <1>;#size-cells = <0>;port@0 {reg = <0>;usb_con_hs: endpoint {remote-endpoint = <&max77865_usbc_hs>;};};port@1 {reg = <1>;usb_con_ss: endpoint {remote-endpoint = <&usbdrd_phy_ss>;};};port@2 {reg = <2>;usb_con_sbu: endpoint {remote-endpoint = <&dp_aux>;};};};- };
+};
On 12.03.2018 11:41, Roger Quadros wrote:
Andrezej,
Why don't you have any of the USB maintainers in to/cc?
Greg Kroah-Hartman gregkh@linuxfoundation.org (supporter:USB SUBSYSTEM) Felipe Balbi balbi@kernel.org (maintainer:USB GADGET/PERIPHERAL SUBSYSTEM)
Serious omission, sorry for that.
On 12/03/18 09:02, Andrzej Hajda wrote:
On 09.03.2018 11:24, Roger Quadros wrote:
Hi,
On 27/02/18 09:11, Andrzej Hajda wrote:
These bindings allow to describe most known standard USB connectors and it should be possible to extend it if necessary. USB connectors, beside USB can be used to route other protocols, for example UART, Audio, MHL. In such case every device passing data through the connector should have appropriate graph bindings.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com
v4:
- improved 'type' description (Rob),
- improved description of 2nd example (Rob).
v3:
- removed MHL port (samsung connector will have separate bindings),
- added 2nd example for USB-C,
- improved formatting.
v2:
- moved connector type(A,B,C) to compatible string (Rob),
- renamed size property to type (Rob),
- changed type description to be less confusing (Laurent),
- removed vendor specific compatibles (implied by graph port number),
- added requirement of connector being a child of IC (Rob),
- removed max-mode (subtly suggested by Rob, it should be detected anyway by USB Controller in runtime, downside is that device is not able to report its real capabilities, maybe better would be to make it optional(?)),
- assigned port numbers to data buses (Rob).
Regards Andrzej
.../bindings/connector/usb-connector.txt | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt new file mode 100644 index 000000000000..e1463f14af38 --- /dev/null +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
Should this lie in bindings/usb/connector?
I guess this is question for DT people. For me both locations are OK.
@@ -0,0 +1,75 @@ +USB Connector +=============
+USB connector node represents physical USB connector. It should be +a child of USB interface controller.
+Required properties: +- compatible: describes type of the connector, must be one of:
- "usb-a-connector",
- "usb-b-connector",
- "usb-c-connector".
compatible should be just "usb-connector"
Type should be a property
type: type of usb connector "A", "B", "AB", "C" AB is for dual-role connectors.
I have proposed such property (and size also) in my first RFC [1]. Rod did not like it :)
This is what Rob says here https://patchwork.kernel.org/patch/9976043/ "We did "type" for hdmi-connector, but I think I'd really prefer compatible be used to distinguish as least where it may matter to s/w. In the HDMI case, they all are pretty much the same, just different physical size."
So the question is. Does it matter to this particular software implementation if it is type A,B,C connector? If yes, how?
IMHO both approaches can work from S/W POV. As I understood it moving usb type to compatible was rather matter of devicetree guidelines I am not aware of. Again question to Rob.
Type A will never have any alternate function. It is always dedicated to USB.
Also does the size "full", "micro", "mini" matter to software?
micro super-speed and high-speed connectors are different. How do you differentiate that?
I am aware of it, and property differentiating it was also in my earlier iterations. If there will be need to differentiate such connectors, adding property max-speed or max-mode would do the thing.
USB controller binding (bindings/usb/generic.txt) has the speed. I don't think there is any value for replicating it for the connector. Maybe it could be added later if needed.
+Optional properties: +- label: symbolic name for the connector,
Why do you need label? We can't maintain consistency as people will put creative names there. Device/bus driver could generate a valid label.
It follows convention for other connectors: HDMI, DVI, ....
+- type: size of the connector, should be specified in case of USB-A, USB-B
- non-fullsize connectors: "mini", "micro".
type is misleading. Type is usually A/B/C. It should be size here instead.
As you can see in discussion for previous iterations it follows convention already established for other connectors.
size: size of the connector if not standard size. "mini", "micro"
If not specified it is treated as standard sized connector. e.g. for Type-C there is no mini/micro. so size doesn't have to be specificed
Few lines above it is described: should be specified in case of USB-A, USB-B non-fullsize connectors.
What about Type-C connector?
+Required nodes: +- any data bus to the connector should be modeled using the OF graph bindings
s/modeled/modelled
- specified in bindings/graph.txt, unless the bus is between parent node and
- the connector. Since single connector can have multpile data buses every bus
s/multpile/multiple
OK, I will fix both typos.
- has assigned OF graph port number as follows:
- 0: High Speed (HS), present in all connectors,
- 1: Super Speed (SS), present in SS capable connectors,
- 2: Sideband use (SBU), present in USB-C.
+Examples +--------
+1. Micro-USB connector with HS lines routed via controller (MUIC):
+muic-max77843@66 {
- ...
- usb_con: connector {
compatible = "usb-b-connector";label = "micro-USB";type = "micro";- };
+};
+2. USB-C connector attached to CC controller (s2mm005), HS lines routed +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort. +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
+ccic: s2mm005@33 {
- ...
- usb_con: connector {
compatible = "usb-c-connector";label = "USB-C";The label is not consistent with the earlier example.
Why?
Because 1st example is "<size>-USB" and second one is "USB-<type>".
How is label going to be used? Is it being presented to end user?
I suppose it could be presented to user, and not-interpreted by S/W.
If yes it should indicate what's important to the user. i.e. its function. e.g. "USB-MHL" or "USB-DisplayPort"
Good idea, to emphasize ports capabilities. I guess it could be most useful in case of multiple USB ports, they could be then named according to their visible properties/labels, for example: Front-USB, Green-USB, USB-1.
Regards Andrzej
Regards Andrzej
ports {#address-cells = <1>;#size-cells = <0>;port@0 {reg = <0>;usb_con_hs: endpoint {remote-endpoint = <&max77865_usbc_hs>;};};port@1 {reg = <1>;usb_con_ss: endpoint {remote-endpoint = <&usbdrd_phy_ss>;};};port@2 {reg = <2>;usb_con_sbu: endpoint {remote-endpoint = <&dp_aux>;};};};- };
+};
On 12/03/18 10:41, Roger Quadros wrote: [...]
@@ -0,0 +1,75 @@ +USB Connector +=============
+USB connector node represents physical USB connector. It should be +a child of USB interface controller.
+Required properties: +- compatible: describes type of the connector, must be one of:
- "usb-a-connector",
- "usb-b-connector",
- "usb-c-connector".
compatible should be just "usb-connector"
Type should be a property
type: type of usb connector "A", "B", "AB", "C" AB is for dual-role connectors.
I have proposed such property (and size also) in my first RFC [1]. Rod did not like it :)
This is what Rob says here https://patchwork.kernel.org/patch/9976043/ "We did "type" for hdmi-connector, but I think I'd really prefer compatible be used to distinguish as least where it may matter to s/w. In the HDMI case, they all are pretty much the same, just different physical size."
So the question is. Does it matter to this particular software implementation if it is type A,B,C connector? If yes, how?
Type A will never have any alternate function. It is always dedicated to USB.
In USB spec terms, at least. In reality there are things like the cool trick Rockchip SoCs do whereby they can expose the debug UART Rx/Tx through the OTG port's D+/D- pins, and that is on a type A connector in many products. I'm guessing that's probably beyond the scope of this binding, though.
Also does the size "full", "micro", "mini" matter to software?
If it means the user can look in sysfs to easily correlate logical ports with physical connectors that's certainly handy (e.g. on something like Odroid-XU where the two USB3 ports are brought out to an A and a micro-AB connector respectively).
Robin.
On 15/03/18 13:46, Robin Murphy wrote:
On 12/03/18 10:41, Roger Quadros wrote: [...]
@@ -0,0 +1,75 @@ +USB Connector +=============
+USB connector node represents physical USB connector. It should be +a child of USB interface controller.
+Required properties: +- compatible: describes type of the connector, must be one of: + "usb-a-connector", + "usb-b-connector", + "usb-c-connector".
compatible should be just "usb-connector"
Type should be a property
type: type of usb connector "A", "B", "AB", "C" AB is for dual-role connectors.
I have proposed such property (and size also) in my first RFC [1]. Rod did not like it :)
This is what Rob says here https://patchwork.kernel.org/patch/9976043/ "We did "type" for hdmi-connector, but I think I'd really prefer compatible be used to distinguish as least where it may matter to s/w. In the HDMI case, they all are pretty much the same, just different physical size."
So the question is. Does it matter to this particular software implementation if it is type A,B,C connector? If yes, how?
Type A will never have any alternate function. It is always dedicated to USB.
In USB spec terms, at least. In reality there are things like the cool trick Rockchip SoCs do whereby they can expose the debug UART Rx/Tx through the OTG port's D+/D- pins, and that is on a type A connector in many products. I'm guessing that's probably beyond the scope of this binding, though.
Also does the size "full", "micro", "mini" matter to software?
If it means the user can look in sysfs to easily correlate logical ports with physical connectors that's certainly handy (e.g. on something like Odroid-XU where the two USB3 ports are brought out to an A and a micro-AB connector respectively).
But this logic fails if both connectors are the same type/size. This is where the label comes in handy. The labels can be unique and end user can identify the port using that.
Robin.
Samsung micro-USB 11-pin connector beside standard micro-USB pins, has pins dedicated to route MHL traffic.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com Reviewed-by: Rob Herring robh@kernel.org --- v4: - removed description of property inherited from usb-connector (Rob), - fixed example description. --- .../connector/samsung,usb-connector-11pin.txt | 49 ++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/connector/samsung,usb-connector-11pin.txt
diff --git a/Documentation/devicetree/bindings/connector/samsung,usb-connector-11pin.txt b/Documentation/devicetree/bindings/connector/samsung,usb-connector-11pin.txt new file mode 100644 index 000000000000..22256e295a7a --- /dev/null +++ b/Documentation/devicetree/bindings/connector/samsung,usb-connector-11pin.txt @@ -0,0 +1,49 @@ +Samsung micro-USB 11-pin connector +================================== + +Samsung micro-USB 11-pin connector is an extension of micro-USB connector. +It is present in multiple Samsung mobile devices. +It has additional pins to route MHL traffic simultanously with USB. + +The bindings are superset of usb-connector bindings for micro-USB connector[1]. + +Required properties: +- compatible: must be: "samsung,usb-connector-11pin", "usb-b-connector", +- type: must be "micro". + +Required nodes: +- any data bus to the connector should be modeled using the OF graph bindings + specified in bindings/graph.txt, unless the bus is between parent node and + the connector. Since single connector can have multpile data buses every bus + has assigned OF graph port number as follows: + 0: High Speed (HS), + 3: Mobile High-Definition Link (MHL), specific to 11-pin Samsung micro-USB. + +[1]: bindings/connector/usb-connector.txt + +Example +------- + +Micro-USB connector with HS lines routed via controller (MUIC) and MHL lines +connected to HDMI-MHL bridge (sii8620): + +muic-max77843@66 { + ... + usb_con: connector { + compatible = "samsung,usb-connector-11pin", "usb-b-connector"; + label = "micro-USB"; + type = "micro"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@3 { + reg = <3>; + usb_con_mhl: endpoint { + remote-endpoint = <&sii8620_mhl>; + }; + }; + }; + }; +};
Since USB connector bindings are available we can describe it on TM2(e).
Signed-off-by: Andrzej Hajda a.hajda@samsung.com --- arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi index 0b61dda99569..f604f6b1a9c2 100644 --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi @@ -836,6 +836,13 @@
muic: max77843-muic { compatible = "maxim,max77843-muic"; + + musb_con: musb_connector { + compatible = "samsung,usb-connector-11pin", + "usb-b-connector"; + label = "micro-USB"; + type = "micro"; + }; };
regulators {
On Tue, Feb 27, 2018 at 08:11:31AM +0100, Andrzej Hajda wrote:
Since USB connector bindings are available we can describe it on TM2(e).
Signed-off-by: Andrzej Hajda a.hajda@samsung.com
arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi | 7 +++++++ 1 file changed, 7 insertions(+)
Thanks, applied.
Best regards, Krzysztof
OF graph describes MHL data lanes between MHL and respective USB connector.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com --- v5: removed extra parenthesis (kbuild test robot) v4: added missing reg property in connector's port node (Krzysztof) --- .../boot/dts/exynos/exynos5433-tm2-common.dtsi | 31 +++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi index f604f6b1a9c2..03453b822093 100644 --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi @@ -817,9 +817,22 @@ clocks = <&pmu_system_controller 0>; clock-names = "xtal";
- port { - mhl_to_hdmi: endpoint { - remote-endpoint = <&hdmi_to_mhl>; + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + mhl_to_hdmi: endpoint { + remote-endpoint = <&hdmi_to_mhl>; + }; + }; + + port@1 { + reg = <1>; + mhl_to_musb_con: endpoint { + remote-endpoint = <&musb_con_to_mhl>; + }; }; }; }; @@ -842,6 +855,18 @@ "usb-b-connector"; label = "micro-USB"; type = "micro"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@3 { + reg = <3>; + musb_con_to_mhl: endpoint { + remote-endpoint = <&mhl_to_musb_con>; + }; + }; + }; }; };
On Tue, Feb 27, 2018 at 08:11:32AM +0100, Andrzej Hajda wrote:
OF graph describes MHL data lanes between MHL and respective USB connector.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com
v5: removed extra parenthesis (kbuild test robot) v4: added missing reg property in connector's port node (Krzysztof)
.../boot/dts/exynos/exynos5433-tm2-common.dtsi | 31 +++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-)
Thanks, applied.
Best regards, Krzysztof
Since extcon property is not allowed in DT, extcon subsystem requires another way to get extcon device. Lets try the simplest approach - get edev by of_node.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com Acked-by: Chanwoo Choi cw00.choi@samsung.com --- v5: function renamed to extcon_find_edev_by_node (Andy) v2: changed label to follow local convention (Chanwoo) --- drivers/extcon/extcon.c | 44 ++++++++++++++++++++++++++++++++++---------- include/linux/extcon.h | 6 ++++++ 2 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c index cb38c2747684..47a5ca9eb86d 100644 --- a/drivers/extcon/extcon.c +++ b/drivers/extcon/extcon.c @@ -1336,6 +1336,28 @@ void extcon_dev_unregister(struct extcon_dev *edev) EXPORT_SYMBOL_GPL(extcon_dev_unregister);
#ifdef CONFIG_OF + +/* + * extcon_find_edev_by_node - Find the extcon device from devicetree. + * @node : OF node identyfying edev + * + * Return the pointer of extcon device if success or ERR_PTR(err) if fail. + */ +struct extcon_dev *extcon_find_edev_by_node(struct device_node *node) +{ + struct extcon_dev *edev; + + mutex_lock(&extcon_dev_list_lock); + list_for_each_entry(edev, &extcon_dev_list, entry) + if (edev->dev.parent && edev->dev.parent->of_node == node) + goto out; + edev = ERR_PTR(-EPROBE_DEFER); +out: + mutex_unlock(&extcon_dev_list_lock); + + return edev; +} + /* * extcon_get_edev_by_phandle - Get the extcon device from devicetree. * @dev : the instance to the given device @@ -1363,25 +1385,27 @@ struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index) return ERR_PTR(-ENODEV); }
- mutex_lock(&extcon_dev_list_lock); - list_for_each_entry(edev, &extcon_dev_list, entry) { - if (edev->dev.parent && edev->dev.parent->of_node == node) { - mutex_unlock(&extcon_dev_list_lock); - of_node_put(node); - return edev; - } - } - mutex_unlock(&extcon_dev_list_lock); + edev = extcon_find_edev_by_node(node); of_node_put(node);
- return ERR_PTR(-EPROBE_DEFER); + return edev; } + #else + +struct extcon_dev *extcon_find_edev_by_node(struct device_node *node) +{ + return ERR_PTR(-ENOSYS); +} + struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index) { return ERR_PTR(-ENOSYS); } + #endif /* CONFIG_OF */ + +EXPORT_SYMBOL_GPL(extcon_find_edev_by_node); EXPORT_SYMBOL_GPL(extcon_get_edev_by_phandle);
/** diff --git a/include/linux/extcon.h b/include/linux/extcon.h index 6d94e82c8ad9..7f033b1ea568 100644 --- a/include/linux/extcon.h +++ b/include/linux/extcon.h @@ -230,6 +230,7 @@ extern void devm_extcon_unregister_notifier_all(struct device *dev, * Following APIs get the extcon_dev from devicetree or by through extcon name. */ extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name); +extern struct extcon_dev *extcon_find_edev_by_node(struct device_node *node); extern struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index);
@@ -283,6 +284,11 @@ static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name) return ERR_PTR(-ENODEV); }
+static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node) +{ + return ERR_PTR(-ENODEV); +} + static inline struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index) {
Hi,
On 2018년 02월 27일 16:11, Andrzej Hajda wrote:
Since extcon property is not allowed in DT, extcon subsystem requires another way to get extcon device. Lets try the simplest approach - get edev by of_node.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com Acked-by: Chanwoo Choi cw00.choi@samsung.com
v5: function renamed to extcon_find_edev_by_node (Andy) v2: changed label to follow local convention (Chanwoo)
drivers/extcon/extcon.c | 44 ++++++++++++++++++++++++++++++++++---------- include/linux/extcon.h | 6 ++++++ 2 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c index cb38c2747684..47a5ca9eb86d 100644 --- a/drivers/extcon/extcon.c +++ b/drivers/extcon/extcon.c @@ -1336,6 +1336,28 @@ void extcon_dev_unregister(struct extcon_dev *edev) EXPORT_SYMBOL_GPL(extcon_dev_unregister);
#ifdef CONFIG_OF
+/*
- extcon_find_edev_by_node - Find the extcon device from devicetree.
- @node : OF node identyfying edev
s/identyfying/identifying
- Return the pointer of extcon device if success or ERR_PTR(err) if fail.
- */
+struct extcon_dev *extcon_find_edev_by_node(struct device_node *node) +{
- struct extcon_dev *edev;
- mutex_lock(&extcon_dev_list_lock);
- list_for_each_entry(edev, &extcon_dev_list, entry)
if (edev->dev.parent && edev->dev.parent->of_node == node)goto out;- edev = ERR_PTR(-EPROBE_DEFER);
+out:
- mutex_unlock(&extcon_dev_list_lock);
- return edev;
+}
/*
- extcon_get_edev_by_phandle - Get the extcon device from devicetree.
- @dev : the instance to the given device
@@ -1363,25 +1385,27 @@ struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index) return ERR_PTR(-ENODEV); }
- mutex_lock(&extcon_dev_list_lock);
- list_for_each_entry(edev, &extcon_dev_list, entry) {
if (edev->dev.parent && edev->dev.parent->of_node == node) {mutex_unlock(&extcon_dev_list_lock);of_node_put(node);return edev;}- }
- mutex_unlock(&extcon_dev_list_lock);
- edev = extcon_find_edev_by_node(node); of_node_put(node);
- return ERR_PTR(-EPROBE_DEFER);
- return edev;
}
#else
+struct extcon_dev *extcon_find_edev_by_node(struct device_node *node) +{
- return ERR_PTR(-ENOSYS);
+}
struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index) { return ERR_PTR(-ENOSYS); }
#endif /* CONFIG_OF */
+EXPORT_SYMBOL_GPL(extcon_find_edev_by_node); EXPORT_SYMBOL_GPL(extcon_get_edev_by_phandle);
/** diff --git a/include/linux/extcon.h b/include/linux/extcon.h index 6d94e82c8ad9..7f033b1ea568 100644 --- a/include/linux/extcon.h +++ b/include/linux/extcon.h @@ -230,6 +230,7 @@ extern void devm_extcon_unregister_notifier_all(struct device *dev,
- Following APIs get the extcon_dev from devicetree or by through extcon name.
*/ extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name); +extern struct extcon_dev *extcon_find_edev_by_node(struct device_node *node); extern struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index);
@@ -283,6 +284,11 @@ static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name) return ERR_PTR(-ENODEV); }
+static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node) +{
- return ERR_PTR(-ENODEV);
+}
static inline struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index) {
Since extcon property is not allowed in DT, extcon subsystem requires another way to get extcon device. Lets try the simplest approach - get edev by of_node.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com Acked-by: Chanwoo Choi cw00.choi@samsung.com --- v5: function renamed to extcon_find_edev_by_node (Andy) v2: changed label to follow local convention (Chanwoo) --- drivers/extcon/extcon.c | 44 ++++++++++++++++++++++++++++++++++---------- include/linux/extcon.h | 6 ++++++ 2 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c index cb38c2747684..8bff5fd18185 100644 --- a/drivers/extcon/extcon.c +++ b/drivers/extcon/extcon.c @@ -1336,6 +1336,28 @@ void extcon_dev_unregister(struct extcon_dev *edev) EXPORT_SYMBOL_GPL(extcon_dev_unregister);
#ifdef CONFIG_OF + +/* + * extcon_find_edev_by_node - Find the extcon device from devicetree. + * @node : OF node identifying edev + * + * Return the pointer of extcon device if success or ERR_PTR(err) if fail. + */ +struct extcon_dev *extcon_find_edev_by_node(struct device_node *node) +{ + struct extcon_dev *edev; + + mutex_lock(&extcon_dev_list_lock); + list_for_each_entry(edev, &extcon_dev_list, entry) + if (edev->dev.parent && edev->dev.parent->of_node == node) + goto out; + edev = ERR_PTR(-EPROBE_DEFER); +out: + mutex_unlock(&extcon_dev_list_lock); + + return edev; +} + /* * extcon_get_edev_by_phandle - Get the extcon device from devicetree. * @dev : the instance to the given device @@ -1363,25 +1385,27 @@ struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index) return ERR_PTR(-ENODEV); }
- mutex_lock(&extcon_dev_list_lock); - list_for_each_entry(edev, &extcon_dev_list, entry) { - if (edev->dev.parent && edev->dev.parent->of_node == node) { - mutex_unlock(&extcon_dev_list_lock); - of_node_put(node); - return edev; - } - } - mutex_unlock(&extcon_dev_list_lock); + edev = extcon_find_edev_by_node(node); of_node_put(node);
- return ERR_PTR(-EPROBE_DEFER); + return edev; } + #else + +struct extcon_dev *extcon_find_edev_by_node(struct device_node *node) +{ + return ERR_PTR(-ENOSYS); +} + struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index) { return ERR_PTR(-ENOSYS); } + #endif /* CONFIG_OF */ + +EXPORT_SYMBOL_GPL(extcon_find_edev_by_node); EXPORT_SYMBOL_GPL(extcon_get_edev_by_phandle);
/** diff --git a/include/linux/extcon.h b/include/linux/extcon.h index 6d94e82c8ad9..7f033b1ea568 100644 --- a/include/linux/extcon.h +++ b/include/linux/extcon.h @@ -230,6 +230,7 @@ extern void devm_extcon_unregister_notifier_all(struct device *dev, * Following APIs get the extcon_dev from devicetree or by through extcon name. */ extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name); +extern struct extcon_dev *extcon_find_edev_by_node(struct device_node *node); extern struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index);
@@ -283,6 +284,11 @@ static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name) return ERR_PTR(-ENODEV); }
+static inline struct extcon_dev *extcon_find_edev_by_node(struct device_node *node) +{ + return ERR_PTR(-ENODEV); +} + static inline struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index) {
From: Maciej Purski m.purski@samsung.com
Currently MHL chip must be turned on permanently to detect MHL cable. It duplicates micro-USB controller's (MUIC) functionality and consumes unnecessary power. Lets use extcon attached to MUIC to enable MHL chip only if it detects MHL cable.
Signed-off-by: Maciej Purski m.purski@samsung.com Signed-off-by: Andrzej Hajda a.hajda@samsung.com --- v5: updated extcon API
This is rework of the patch by Maciej with following changes: - use micro-USB port bindings to get extcon, instead of extcon property, - fixed remove sequence, - fixed extcon get state logic.
Code finding extcon node is hacky IMO, I guess ultimately it should be done via some framework (maybe even extcon), or at least via helper, I hope it can stay as is until the proper solution will be merged.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com --- drivers/gpu/drm/bridge/sil-sii8620.c | 97 ++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 9e785b8e0ea2..62b0adabcac2 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -17,6 +17,7 @@
#include <linux/clk.h> #include <linux/delay.h> +#include <linux/extcon.h> #include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/interrupt.h> @@ -25,6 +26,7 @@ #include <linux/list.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/of_graph.h> #include <linux/regulator/consumer.h> #include <linux/slab.h>
@@ -81,6 +83,10 @@ struct sii8620 { struct edid *edid; unsigned int gen2_write_burst:1; enum sii8620_mt_state mt_state; + struct extcon_dev *extcon; + struct notifier_block extcon_nb; + struct work_struct extcon_wq; + int cable_state; struct list_head mt_queue; struct { int r_size; @@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) ctx->rc_dev = rc_dev; }
+static void sii8620_cable_out(struct sii8620 *ctx) +{ + disable_irq(to_i2c_client(ctx->dev)->irq); + sii8620_hw_off(ctx); +} + +static void sii8620_extcon_work(struct work_struct *work) +{ + struct sii8620 *ctx = + container_of(work, struct sii8620, extcon_wq); + int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL); + + if (state == ctx->cable_state) + return; + + ctx->cable_state = state; + + if (state > 0) + sii8620_cable_in(ctx); + else + sii8620_cable_out(ctx); +} + +static int sii8620_extcon_notifier(struct notifier_block *self, + unsigned long event, void *ptr) +{ + struct sii8620 *ctx = + container_of(self, struct sii8620, extcon_nb); + + schedule_work(&ctx->extcon_wq); + + return NOTIFY_DONE; +} + +static int sii8620_extcon_init(struct sii8620 *ctx) +{ + struct extcon_dev *edev; + struct device_node *musb, *muic; + int ret; + + /* get micro-USB connector node */ + musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1); + /* next get micro-USB Interface Controller node */ + muic = of_get_next_parent(musb); + + if (!muic) { + dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n"); + return 0; + } + + edev = extcon_find_edev_by_node(muic); + of_node_put(muic); + if (IS_ERR(edev)) { + if (PTR_ERR(edev) == -EPROBE_DEFER) + return -EPROBE_DEFER; + dev_err(ctx->dev, "Invalid or missing extcon\n"); + return PTR_ERR(edev); + } + + ctx->extcon = edev; + ctx->extcon_nb.notifier_call = sii8620_extcon_notifier; + INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work); + ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb); + if (ret) { + dev_err(ctx->dev, "failed to register notifier for MHL\n"); + return ret; + } + + return 0; +} + static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge) { return container_of(bridge, struct sii8620, bridge); @@ -2307,13 +2384,20 @@ static int sii8620_probe(struct i2c_client *client, if (ret) return ret;
+ ret = sii8620_extcon_init(ctx); + if (ret < 0) { + dev_err(ctx->dev, "failed to initialize EXTCON\n"); + return ret; + } + i2c_set_clientdata(client, ctx);
ctx->bridge.funcs = &sii8620_bridge_funcs; ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge);
- sii8620_cable_in(ctx); + if (!ctx->extcon) + sii8620_cable_in(ctx);
return 0; } @@ -2322,8 +2406,15 @@ static int sii8620_remove(struct i2c_client *client) { struct sii8620 *ctx = i2c_get_clientdata(client);
- disable_irq(to_i2c_client(ctx->dev)->irq); - sii8620_hw_off(ctx); + if (ctx->extcon) { + extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL, + &ctx->extcon_nb); + flush_work(&ctx->extcon_wq); + if (ctx->cable_state > 0) + sii8620_cable_out(ctx); + } else { + sii8620_cable_out(ctx); + } drm_bridge_remove(&ctx->bridge);
return 0;
Hi,
On 2018년 02월 27일 16:11, Andrzej Hajda wrote:
From: Maciej Purski m.purski@samsung.com
Currently MHL chip must be turned on permanently to detect MHL cable. It duplicates micro-USB controller's (MUIC) functionality and consumes unnecessary power. Lets use extcon attached to MUIC to enable MHL chip only if it detects MHL cable.
Signed-off-by: Maciej Purski m.purski@samsung.com Signed-off-by: Andrzej Hajda a.hajda@samsung.com
v5: updated extcon API
This is rework of the patch by Maciej with following changes:
- use micro-USB port bindings to get extcon, instead of extcon property,
- fixed remove sequence,
- fixed extcon get state logic.
Code finding extcon node is hacky IMO, I guess ultimately it should be done via some framework (maybe even extcon), or at least via helper, I hope it can stay as is until the proper solution will be merged.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com
drivers/gpu/drm/bridge/sil-sii8620.c | 97 ++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 9e785b8e0ea2..62b0adabcac2 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -17,6 +17,7 @@
#include <linux/clk.h> #include <linux/delay.h> +#include <linux/extcon.h> #include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/interrupt.h> @@ -25,6 +26,7 @@ #include <linux/list.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/of_graph.h> #include <linux/regulator/consumer.h> #include <linux/slab.h>
@@ -81,6 +83,10 @@ struct sii8620 { struct edid *edid; unsigned int gen2_write_burst:1; enum sii8620_mt_state mt_state;
- struct extcon_dev *extcon;
- struct notifier_block extcon_nb;
- struct work_struct extcon_wq;
- int cable_state; struct list_head mt_queue; struct { int r_size;
@@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) ctx->rc_dev = rc_dev; }
+static void sii8620_cable_out(struct sii8620 *ctx) +{
- disable_irq(to_i2c_client(ctx->dev)->irq);
- sii8620_hw_off(ctx);
+}
+static void sii8620_extcon_work(struct work_struct *work) +{
- struct sii8620 *ctx =
container_of(work, struct sii8620, extcon_wq);- int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL);
- if (state == ctx->cable_state)
return;- ctx->cable_state = state;
- if (state > 0)
sii8620_cable_in(ctx);- else
sii8620_cable_out(ctx);+}
+static int sii8620_extcon_notifier(struct notifier_block *self,
unsigned long event, void *ptr)+{
- struct sii8620 *ctx =
container_of(self, struct sii8620, extcon_nb);- schedule_work(&ctx->extcon_wq);
- return NOTIFY_DONE;
+}
+static int sii8620_extcon_init(struct sii8620 *ctx) +{
- struct extcon_dev *edev;
- struct device_node *musb, *muic;
- int ret;
- /* get micro-USB connector node */
- musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1);
- /* next get micro-USB Interface Controller node */
- muic = of_get_next_parent(musb);
- if (!muic) {
dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n");return 0;- }
- edev = extcon_find_edev_by_node(muic);
- of_node_put(muic);
- if (IS_ERR(edev)) {
if (PTR_ERR(edev) == -EPROBE_DEFER)return -EPROBE_DEFER;dev_err(ctx->dev, "Invalid or missing extcon\n");return PTR_ERR(edev);- }
- ctx->extcon = edev;
- ctx->extcon_nb.notifier_call = sii8620_extcon_notifier;
- INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work);
- ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb);
You better to use devm_extcon_register_notifier().
- if (ret) {
dev_err(ctx->dev, "failed to register notifier for MHL\n");return ret;- }
- return 0;
+}
static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge) { return container_of(bridge, struct sii8620, bridge); @@ -2307,13 +2384,20 @@ static int sii8620_probe(struct i2c_client *client, if (ret) return ret;
ret = sii8620_extcon_init(ctx);
if (ret < 0) {
dev_err(ctx->dev, "failed to initialize EXTCON\n");return ret;}
i2c_set_clientdata(client, ctx);
ctx->bridge.funcs = &sii8620_bridge_funcs; ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge);
- sii8620_cable_in(ctx);
if (!ctx->extcon)
sii8620_cable_in(ctx);return 0;
} @@ -2322,8 +2406,15 @@ static int sii8620_remove(struct i2c_client *client) { struct sii8620 *ctx = i2c_get_clientdata(client);
- disable_irq(to_i2c_client(ctx->dev)->irq);
- sii8620_hw_off(ctx);
- if (ctx->extcon) {
extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL,&ctx->extcon_nb);
Don't need to unregister the notifier if using devm_extcon_register_notifier().
flush_work(&ctx->extcon_wq);if (ctx->cable_state > 0)sii8620_cable_out(ctx);} else {
sii8620_cable_out(ctx);} drm_bridge_remove(&ctx->bridge);
return 0;
If you use the resource managed function (devm_extcon_register_notifier), Looks good to me. Reviewed-by: Chanwoo Choi cw00.choi@samsung.com
On 27.02.2018 12:08, Chanwoo Choi wrote:
Hi,
On 2018년 02월 27일 16:11, Andrzej Hajda wrote:
From: Maciej Purski m.purski@samsung.com
Currently MHL chip must be turned on permanently to detect MHL cable. It duplicates micro-USB controller's (MUIC) functionality and consumes unnecessary power. Lets use extcon attached to MUIC to enable MHL chip only if it detects MHL cable.
Signed-off-by: Maciej Purski m.purski@samsung.com Signed-off-by: Andrzej Hajda a.hajda@samsung.com
v5: updated extcon API
This is rework of the patch by Maciej with following changes:
- use micro-USB port bindings to get extcon, instead of extcon property,
- fixed remove sequence,
- fixed extcon get state logic.
Code finding extcon node is hacky IMO, I guess ultimately it should be done via some framework (maybe even extcon), or at least via helper, I hope it can stay as is until the proper solution will be merged.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com
drivers/gpu/drm/bridge/sil-sii8620.c | 97 ++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 9e785b8e0ea2..62b0adabcac2 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -17,6 +17,7 @@
#include <linux/clk.h> #include <linux/delay.h> +#include <linux/extcon.h> #include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/interrupt.h> @@ -25,6 +26,7 @@ #include <linux/list.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/of_graph.h> #include <linux/regulator/consumer.h> #include <linux/slab.h>
@@ -81,6 +83,10 @@ struct sii8620 { struct edid *edid; unsigned int gen2_write_burst:1; enum sii8620_mt_state mt_state;
- struct extcon_dev *extcon;
- struct notifier_block extcon_nb;
- struct work_struct extcon_wq;
- int cable_state; struct list_head mt_queue; struct { int r_size;
@@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) ctx->rc_dev = rc_dev; }
+static void sii8620_cable_out(struct sii8620 *ctx) +{
- disable_irq(to_i2c_client(ctx->dev)->irq);
- sii8620_hw_off(ctx);
+}
+static void sii8620_extcon_work(struct work_struct *work) +{
- struct sii8620 *ctx =
container_of(work, struct sii8620, extcon_wq);- int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL);
- if (state == ctx->cable_state)
return;- ctx->cable_state = state;
- if (state > 0)
sii8620_cable_in(ctx);- else
sii8620_cable_out(ctx);+}
+static int sii8620_extcon_notifier(struct notifier_block *self,
unsigned long event, void *ptr)+{
- struct sii8620 *ctx =
container_of(self, struct sii8620, extcon_nb);- schedule_work(&ctx->extcon_wq);
- return NOTIFY_DONE;
+}
+static int sii8620_extcon_init(struct sii8620 *ctx) +{
- struct extcon_dev *edev;
- struct device_node *musb, *muic;
- int ret;
- /* get micro-USB connector node */
- musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1);
- /* next get micro-USB Interface Controller node */
- muic = of_get_next_parent(musb);
- if (!muic) {
dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n");return 0;- }
- edev = extcon_find_edev_by_node(muic);
- of_node_put(muic);
- if (IS_ERR(edev)) {
if (PTR_ERR(edev) == -EPROBE_DEFER)return -EPROBE_DEFER;dev_err(ctx->dev, "Invalid or missing extcon\n");return PTR_ERR(edev);- }
- ctx->extcon = edev;
- ctx->extcon_nb.notifier_call = sii8620_extcon_notifier;
- INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work);
- ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb);
You better to use devm_extcon_register_notifier().
With devm version I risk that in case of device unbind notification will be called after .remove callback, it seems to me quite dangerous. Or am I missing something?
Regards Andrzej
- if (ret) {
dev_err(ctx->dev, "failed to register notifier for MHL\n");return ret;- }
- return 0;
+}
static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge) { return container_of(bridge, struct sii8620, bridge); @@ -2307,13 +2384,20 @@ static int sii8620_probe(struct i2c_client *client, if (ret) return ret;
ret = sii8620_extcon_init(ctx);
if (ret < 0) {
dev_err(ctx->dev, "failed to initialize EXTCON\n");return ret;}
i2c_set_clientdata(client, ctx);
ctx->bridge.funcs = &sii8620_bridge_funcs; ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge);
- sii8620_cable_in(ctx);
if (!ctx->extcon)
sii8620_cable_in(ctx);return 0;
} @@ -2322,8 +2406,15 @@ static int sii8620_remove(struct i2c_client *client) { struct sii8620 *ctx = i2c_get_clientdata(client);
- disable_irq(to_i2c_client(ctx->dev)->irq);
- sii8620_hw_off(ctx);
- if (ctx->extcon) {
extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL,&ctx->extcon_nb);Don't need to unregister the notifier if using devm_extcon_register_notifier().
flush_work(&ctx->extcon_wq);if (ctx->cable_state > 0)sii8620_cable_out(ctx);} else {
sii8620_cable_out(ctx);} drm_bridge_remove(&ctx->bridge);
return 0;
If you use the resource managed function (devm_extcon_register_notifier), Looks good to me. Reviewed-by: Chanwoo Choi cw00.choi@samsung.com
Hi,
On 2018년 02월 27일 21:05, Andrzej Hajda wrote:
On 27.02.2018 12:08, Chanwoo Choi wrote:
Hi,
On 2018년 02월 27일 16:11, Andrzej Hajda wrote:
From: Maciej Purski m.purski@samsung.com
Currently MHL chip must be turned on permanently to detect MHL cable. It duplicates micro-USB controller's (MUIC) functionality and consumes unnecessary power. Lets use extcon attached to MUIC to enable MHL chip only if it detects MHL cable.
Signed-off-by: Maciej Purski m.purski@samsung.com Signed-off-by: Andrzej Hajda a.hajda@samsung.com
v5: updated extcon API
This is rework of the patch by Maciej with following changes:
- use micro-USB port bindings to get extcon, instead of extcon property,
- fixed remove sequence,
- fixed extcon get state logic.
Code finding extcon node is hacky IMO, I guess ultimately it should be done via some framework (maybe even extcon), or at least via helper, I hope it can stay as is until the proper solution will be merged.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com
drivers/gpu/drm/bridge/sil-sii8620.c | 97 ++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 9e785b8e0ea2..62b0adabcac2 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -17,6 +17,7 @@
#include <linux/clk.h> #include <linux/delay.h> +#include <linux/extcon.h> #include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/interrupt.h> @@ -25,6 +26,7 @@ #include <linux/list.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/of_graph.h> #include <linux/regulator/consumer.h> #include <linux/slab.h>
@@ -81,6 +83,10 @@ struct sii8620 { struct edid *edid; unsigned int gen2_write_burst:1; enum sii8620_mt_state mt_state;
- struct extcon_dev *extcon;
- struct notifier_block extcon_nb;
- struct work_struct extcon_wq;
- int cable_state; struct list_head mt_queue; struct { int r_size;
@@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) ctx->rc_dev = rc_dev; }
+static void sii8620_cable_out(struct sii8620 *ctx) +{
- disable_irq(to_i2c_client(ctx->dev)->irq);
- sii8620_hw_off(ctx);
+}
+static void sii8620_extcon_work(struct work_struct *work) +{
- struct sii8620 *ctx =
container_of(work, struct sii8620, extcon_wq);- int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL);
- if (state == ctx->cable_state)
return;- ctx->cable_state = state;
- if (state > 0)
sii8620_cable_in(ctx);- else
sii8620_cable_out(ctx);+}
+static int sii8620_extcon_notifier(struct notifier_block *self,
unsigned long event, void *ptr)+{
- struct sii8620 *ctx =
container_of(self, struct sii8620, extcon_nb);- schedule_work(&ctx->extcon_wq);
- return NOTIFY_DONE;
+}
+static int sii8620_extcon_init(struct sii8620 *ctx) +{
- struct extcon_dev *edev;
- struct device_node *musb, *muic;
- int ret;
- /* get micro-USB connector node */
- musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1);
- /* next get micro-USB Interface Controller node */
- muic = of_get_next_parent(musb);
- if (!muic) {
dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n");return 0;- }
- edev = extcon_find_edev_by_node(muic);
- of_node_put(muic);
- if (IS_ERR(edev)) {
if (PTR_ERR(edev) == -EPROBE_DEFER)return -EPROBE_DEFER;dev_err(ctx->dev, "Invalid or missing extcon\n");return PTR_ERR(edev);- }
- ctx->extcon = edev;
- ctx->extcon_nb.notifier_call = sii8620_extcon_notifier;
- INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work);
- ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb);
You better to use devm_extcon_register_notifier().
With devm version I risk that in case of device unbind notification will be called after .remove callback, it seems to me quite dangerous. Or am I missing something?
If you use the cancel_work_sync() in remove() instead of flush_work(), you can use the 'devm_extcon_*'.
Regards Andrzej
- if (ret) {
dev_err(ctx->dev, "failed to register notifier for MHL\n");return ret;- }
- return 0;
+}
static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge) { return container_of(bridge, struct sii8620, bridge); @@ -2307,13 +2384,20 @@ static int sii8620_probe(struct i2c_client *client, if (ret) return ret;
ret = sii8620_extcon_init(ctx);
if (ret < 0) {
dev_err(ctx->dev, "failed to initialize EXTCON\n");return ret;}
i2c_set_clientdata(client, ctx);
ctx->bridge.funcs = &sii8620_bridge_funcs; ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge);
- sii8620_cable_in(ctx);
if (!ctx->extcon)
sii8620_cable_in(ctx);return 0;
} @@ -2322,8 +2406,15 @@ static int sii8620_remove(struct i2c_client *client) { struct sii8620 *ctx = i2c_get_clientdata(client);
- disable_irq(to_i2c_client(ctx->dev)->irq);
- sii8620_hw_off(ctx);
- if (ctx->extcon) {
extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL,&ctx->extcon_nb);Don't need to unregister the notifier if using devm_extcon_register_notifier().
flush_work(&ctx->extcon_wq);if (ctx->cable_state > 0)sii8620_cable_out(ctx);} else {
sii8620_cable_out(ctx);} drm_bridge_remove(&ctx->bridge);
return 0;
If you use the resource managed function (devm_extcon_register_notifier), Looks good to me. Reviewed-by: Chanwoo Choi cw00.choi@samsung.com
-- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27.02.2018 23:26, Chanwoo Choi wrote:
Hi,
On 2018년 02월 27일 21:05, Andrzej Hajda wrote:
On 27.02.2018 12:08, Chanwoo Choi wrote:
Hi,
On 2018년 02월 27일 16:11, Andrzej Hajda wrote:
From: Maciej Purski m.purski@samsung.com
Currently MHL chip must be turned on permanently to detect MHL cable. It duplicates micro-USB controller's (MUIC) functionality and consumes unnecessary power. Lets use extcon attached to MUIC to enable MHL chip only if it detects MHL cable.
Signed-off-by: Maciej Purski m.purski@samsung.com Signed-off-by: Andrzej Hajda a.hajda@samsung.com
v5: updated extcon API
This is rework of the patch by Maciej with following changes:
- use micro-USB port bindings to get extcon, instead of extcon property,
- fixed remove sequence,
- fixed extcon get state logic.
Code finding extcon node is hacky IMO, I guess ultimately it should be done via some framework (maybe even extcon), or at least via helper, I hope it can stay as is until the proper solution will be merged.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com
drivers/gpu/drm/bridge/sil-sii8620.c | 97 ++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 9e785b8e0ea2..62b0adabcac2 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -17,6 +17,7 @@
#include <linux/clk.h> #include <linux/delay.h> +#include <linux/extcon.h> #include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/interrupt.h> @@ -25,6 +26,7 @@ #include <linux/list.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/of_graph.h> #include <linux/regulator/consumer.h> #include <linux/slab.h>
@@ -81,6 +83,10 @@ struct sii8620 { struct edid *edid; unsigned int gen2_write_burst:1; enum sii8620_mt_state mt_state;
- struct extcon_dev *extcon;
- struct notifier_block extcon_nb;
- struct work_struct extcon_wq;
- int cable_state; struct list_head mt_queue; struct { int r_size;
@@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) ctx->rc_dev = rc_dev; }
+static void sii8620_cable_out(struct sii8620 *ctx) +{
- disable_irq(to_i2c_client(ctx->dev)->irq);
- sii8620_hw_off(ctx);
+}
+static void sii8620_extcon_work(struct work_struct *work) +{
- struct sii8620 *ctx =
container_of(work, struct sii8620, extcon_wq);- int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL);
- if (state == ctx->cable_state)
return;- ctx->cable_state = state;
- if (state > 0)
sii8620_cable_in(ctx);- else
sii8620_cable_out(ctx);+}
+static int sii8620_extcon_notifier(struct notifier_block *self,
unsigned long event, void *ptr)+{
- struct sii8620 *ctx =
container_of(self, struct sii8620, extcon_nb);- schedule_work(&ctx->extcon_wq);
- return NOTIFY_DONE;
+}
+static int sii8620_extcon_init(struct sii8620 *ctx) +{
- struct extcon_dev *edev;
- struct device_node *musb, *muic;
- int ret;
- /* get micro-USB connector node */
- musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1);
- /* next get micro-USB Interface Controller node */
- muic = of_get_next_parent(musb);
- if (!muic) {
dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n");return 0;- }
- edev = extcon_find_edev_by_node(muic);
- of_node_put(muic);
- if (IS_ERR(edev)) {
if (PTR_ERR(edev) == -EPROBE_DEFER)return -EPROBE_DEFER;dev_err(ctx->dev, "Invalid or missing extcon\n");return PTR_ERR(edev);- }
- ctx->extcon = edev;
- ctx->extcon_nb.notifier_call = sii8620_extcon_notifier;
- INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work);
- ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb);
You better to use devm_extcon_register_notifier().
With devm version I risk that in case of device unbind notification will be called after .remove callback, it seems to me quite dangerous. Or am I missing something?
If you use the cancel_work_sync() in remove() instead of flush_work(), you can use the 'devm_extcon_*'.
cancel_work_sync() does not prevent works scheduled later from execution [1] and this scenario is possible with devm_extcon_register_notifier() and cancel_work_sync(). So we end up with: sii8620_remove() calls cancel_work_sync() ... notifier(called asynchronously) schedules sii8620_extcon_work() ... notifier is removed by devm framework sii8620 context is destroyed by devm framework ... sii8620_extcon_work is executed on destroyed context !!! BUG !!!
For me it seems that devm_extcon_register_notifier is not safe in this case.
[1]: Since documentation was not clear I have performed live test confirming my suspicions.
Regards Andrzej
Regards Andrzej
- if (ret) {
dev_err(ctx->dev, "failed to register notifier for MHL\n");return ret;- }
- return 0;
+}
static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge) { return container_of(bridge, struct sii8620, bridge); @@ -2307,13 +2384,20 @@ static int sii8620_probe(struct i2c_client *client, if (ret) return ret;
ret = sii8620_extcon_init(ctx);
if (ret < 0) {
dev_err(ctx->dev, "failed to initialize EXTCON\n");return ret;}
i2c_set_clientdata(client, ctx);
ctx->bridge.funcs = &sii8620_bridge_funcs; ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge);
- sii8620_cable_in(ctx);
if (!ctx->extcon)
sii8620_cable_in(ctx);return 0;
} @@ -2322,8 +2406,15 @@ static int sii8620_remove(struct i2c_client *client) { struct sii8620 *ctx = i2c_get_clientdata(client);
- disable_irq(to_i2c_client(ctx->dev)->irq);
- sii8620_hw_off(ctx);
- if (ctx->extcon) {
extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL,&ctx->extcon_nb);Don't need to unregister the notifier if using devm_extcon_register_notifier().
flush_work(&ctx->extcon_wq);if (ctx->cable_state > 0)sii8620_cable_out(ctx);} else {
sii8620_cable_out(ctx);} drm_bridge_remove(&ctx->bridge);
return 0;
If you use the resource managed function (devm_extcon_register_notifier), Looks good to me. Reviewed-by: Chanwoo Choi cw00.choi@samsung.com
-- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018년 02월 28일 22:44, Andrzej Hajda wrote:
On 27.02.2018 23:26, Chanwoo Choi wrote:
Hi,
On 2018년 02월 27일 21:05, Andrzej Hajda wrote:
On 27.02.2018 12:08, Chanwoo Choi wrote:
Hi,
On 2018년 02월 27일 16:11, Andrzej Hajda wrote:
From: Maciej Purski m.purski@samsung.com
Currently MHL chip must be turned on permanently to detect MHL cable. It duplicates micro-USB controller's (MUIC) functionality and consumes unnecessary power. Lets use extcon attached to MUIC to enable MHL chip only if it detects MHL cable.
Signed-off-by: Maciej Purski m.purski@samsung.com Signed-off-by: Andrzej Hajda a.hajda@samsung.com
v5: updated extcon API
This is rework of the patch by Maciej with following changes:
- use micro-USB port bindings to get extcon, instead of extcon property,
- fixed remove sequence,
- fixed extcon get state logic.
Code finding extcon node is hacky IMO, I guess ultimately it should be done via some framework (maybe even extcon), or at least via helper, I hope it can stay as is until the proper solution will be merged.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com
drivers/gpu/drm/bridge/sil-sii8620.c | 97 ++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 9e785b8e0ea2..62b0adabcac2 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -17,6 +17,7 @@
#include <linux/clk.h> #include <linux/delay.h> +#include <linux/extcon.h> #include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/interrupt.h> @@ -25,6 +26,7 @@ #include <linux/list.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/of_graph.h> #include <linux/regulator/consumer.h> #include <linux/slab.h>
@@ -81,6 +83,10 @@ struct sii8620 { struct edid *edid; unsigned int gen2_write_burst:1; enum sii8620_mt_state mt_state;
- struct extcon_dev *extcon;
- struct notifier_block extcon_nb;
- struct work_struct extcon_wq;
- int cable_state; struct list_head mt_queue; struct { int r_size;
@@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) ctx->rc_dev = rc_dev; }
+static void sii8620_cable_out(struct sii8620 *ctx) +{
- disable_irq(to_i2c_client(ctx->dev)->irq);
- sii8620_hw_off(ctx);
+}
+static void sii8620_extcon_work(struct work_struct *work) +{
- struct sii8620 *ctx =
container_of(work, struct sii8620, extcon_wq);- int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL);
- if (state == ctx->cable_state)
return;- ctx->cable_state = state;
- if (state > 0)
sii8620_cable_in(ctx);- else
sii8620_cable_out(ctx);+}
+static int sii8620_extcon_notifier(struct notifier_block *self,
unsigned long event, void *ptr)+{
- struct sii8620 *ctx =
container_of(self, struct sii8620, extcon_nb);- schedule_work(&ctx->extcon_wq);
- return NOTIFY_DONE;
+}
+static int sii8620_extcon_init(struct sii8620 *ctx) +{
- struct extcon_dev *edev;
- struct device_node *musb, *muic;
- int ret;
- /* get micro-USB connector node */
- musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1);
- /* next get micro-USB Interface Controller node */
- muic = of_get_next_parent(musb);
- if (!muic) {
dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n");return 0;- }
- edev = extcon_find_edev_by_node(muic);
- of_node_put(muic);
- if (IS_ERR(edev)) {
if (PTR_ERR(edev) == -EPROBE_DEFER)return -EPROBE_DEFER;dev_err(ctx->dev, "Invalid or missing extcon\n");return PTR_ERR(edev);- }
- ctx->extcon = edev;
- ctx->extcon_nb.notifier_call = sii8620_extcon_notifier;
- INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work);
- ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb);
You better to use devm_extcon_register_notifier().
With devm version I risk that in case of device unbind notification will be called after .remove callback, it seems to me quite dangerous. Or am I missing something?
If you use the cancel_work_sync() in remove() instead of flush_work(), you can use the 'devm_extcon_*'.
cancel_work_sync() does not prevent works scheduled later from execution [1] and this scenario is possible with devm_extcon_register_notifier() and cancel_work_sync(). So we end up with: sii8620_remove() calls cancel_work_sync() ... notifier(called asynchronously) schedules sii8620_extcon_work() ... notifier is removed by devm framework sii8620 context is destroyed by devm framework ... sii8620_extcon_work is executed on destroyed context !!! BUG !!!
For me it seems that devm_extcon_register_notifier is not safe in this case.
[1]: Since documentation was not clear I have performed live test confirming my suspicions.
You're right. Sorry for confusion. Reviewed-by: Chanwoo Choi cw00.choi@samsung.com
[snip]
Hi Rob, Chanwoo, Krzysztof,
On 27.02.2018 08:11, Andrzej Hajda wrote:
Hi,
Thanks for reviews of previous iterations.
This patchset introduces USB physical connector bindings, together with working example. I have removed RFC prefix - the patchset seems to be heading to a happy end :)
v5: fixed extra parenthesis in dts, renamed extcon function. v4: improved binding descriptions, added missing reg in dts. v3: Separate binding for Samsung 11-pin connector, added full-blown USB-C example. v2: I have addressed comments by Rob and Laurent, thanks
Changes in datail are described in the patches.
Regards Andrzej
Andrzej Hajda (5): dt-bindings: add bindings for USB physical connector dt-bindings: add bindings for Samsung micro-USB 11-pin connector arm64: dts: exynos: add micro-USB connector node to TM2 platforms arm64: dts: exynos: add OF graph between MHL and USB connector extcon: add possibility to get extcon device by OF node
Maciej Purski (1): drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL
It looks like all patches received R-B or acks (I forgot add Krzysztof's acks for dts part). Now I have a question how to merge them. The only functional dependency is between bridge and extcon, and from the formal PoV bindings should be merged 1st. I can merge it: 1. All patches via drm-misc tree. 2. All patches except dts via drm-misc, and Krzysztof will merge dts via samsung-soc tree.
Is it OK, for all? Better ideas?
Regards Andrzej
.../connector/samsung,usb-connector-11pin.txt | 49 +++++++++++ .../bindings/connector/usb-connector.txt | 75 +++++++++++++++++ .../boot/dts/exynos/exynos5433-tm2-common.dtsi | 38 ++++++++- drivers/extcon/extcon.c | 44 +++++++--- drivers/gpu/drm/bridge/sil-sii8620.c | 97 +++++++++++++++++++++- include/linux/extcon.h | 6 ++ 6 files changed, 293 insertions(+), 16 deletions(-) create mode 100644 Documentation/devicetree/bindings/connector/samsung,usb-connector-11pin.txt create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
On Tue, Mar 6, 2018 at 1:53 PM, Andrzej Hajda a.hajda@samsung.com wrote:
Hi Rob, Chanwoo, Krzysztof,
On 27.02.2018 08:11, Andrzej Hajda wrote:
Hi,
Thanks for reviews of previous iterations.
This patchset introduces USB physical connector bindings, together with working example. I have removed RFC prefix - the patchset seems to be heading to a happy end :)
v5: fixed extra parenthesis in dts, renamed extcon function. v4: improved binding descriptions, added missing reg in dts. v3: Separate binding for Samsung 11-pin connector, added full-blown USB-C example. v2: I have addressed comments by Rob and Laurent, thanks
Changes in datail are described in the patches.
Regards Andrzej
Andrzej Hajda (5): dt-bindings: add bindings for USB physical connector dt-bindings: add bindings for Samsung micro-USB 11-pin connector arm64: dts: exynos: add micro-USB connector node to TM2 platforms arm64: dts: exynos: add OF graph between MHL and USB connector extcon: add possibility to get extcon device by OF node
Maciej Purski (1): drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL
It looks like all patches received R-B or acks (I forgot add Krzysztof's acks for dts part). Now I have a question how to merge them. The only functional dependency is between bridge and extcon, and from the formal PoV bindings should be merged 1st. I can merge it:
- All patches via drm-misc tree.
- All patches except dts via drm-misc, and Krzysztof will merge dts via
samsung-soc tree.
2) option please. The DTS should always go through arm-soc because it is independent of driver code. I'll take the DTS today.
Best regards, Krzysztof
Hi Rob and Andrzej,
On 2018년 03월 06일 21:53, Andrzej Hajda wrote:
Hi Rob, Chanwoo, Krzysztof,
On 27.02.2018 08:11, Andrzej Hajda wrote:
Hi,
Thanks for reviews of previous iterations.
This patchset introduces USB physical connector bindings, together with working example. I have removed RFC prefix - the patchset seems to be heading to a happy end :)
v5: fixed extra parenthesis in dts, renamed extcon function. v4: improved binding descriptions, added missing reg in dts. v3: Separate binding for Samsung 11-pin connector, added full-blown USB-C example. v2: I have addressed comments by Rob and Laurent, thanks
Changes in datail are described in the patches.
Regards Andrzej
Andrzej Hajda (5): dt-bindings: add bindings for USB physical connector dt-bindings: add bindings for Samsung micro-USB 11-pin connector arm64: dts: exynos: add micro-USB connector node to TM2 platforms arm64: dts: exynos: add OF graph between MHL and USB connector extcon: add possibility to get extcon device by OF node
Maciej Purski (1): drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL
It looks like all patches received R-B or acks (I forgot add Krzysztof's acks for dts part). Now I have a question how to merge them. The only functional dependency is between bridge and extcon, and from the formal PoV bindings should be merged 1st. I can merge it:
- All patches via drm-misc tree.
- All patches except dts via drm-misc, and Krzysztof will merge dts via
samsung-soc tree.
Is it OK, for all? Better ideas?
Krzysztof picked the dts patches. I'll make the immutable branch based on v4.16-rc1 and apply them except for dts patchs. And I'll send the immutable branch to Rob and Andrzej.
On 2018년 03월 07일 11:12, Chanwoo Choi wrote:
Hi Rob and Andrzej,
On 2018년 03월 06일 21:53, Andrzej Hajda wrote:
Hi Rob, Chanwoo, Krzysztof,
On 27.02.2018 08:11, Andrzej Hajda wrote:
Hi,
Thanks for reviews of previous iterations.
This patchset introduces USB physical connector bindings, together with working example. I have removed RFC prefix - the patchset seems to be heading to a happy end :)
v5: fixed extra parenthesis in dts, renamed extcon function. v4: improved binding descriptions, added missing reg in dts. v3: Separate binding for Samsung 11-pin connector, added full-blown USB-C example. v2: I have addressed comments by Rob and Laurent, thanks
Changes in datail are described in the patches.
Regards Andrzej
Andrzej Hajda (5): dt-bindings: add bindings for USB physical connector dt-bindings: add bindings for Samsung micro-USB 11-pin connector arm64: dts: exynos: add micro-USB connector node to TM2 platforms arm64: dts: exynos: add OF graph between MHL and USB connector extcon: add possibility to get extcon device by OF node
Maciej Purski (1): drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL
It looks like all patches received R-B or acks (I forgot add Krzysztof's acks for dts part). Now I have a question how to merge them. The only functional dependency is between bridge and extcon, and from the formal PoV bindings should be merged 1st. I can merge it:
- All patches via drm-misc tree.
- All patches except dts via drm-misc, and Krzysztof will merge dts via
samsung-soc tree.
Is it OK, for all? Better ideas?
Krzysztof picked the dts patches. I'll make the immutable branch based on v4.16-rc1 and apply them except for dts patchs. And I'll send the immutable branch to Rob and Andrzej.
I made the immutable branch[1] as following: If you agree, I'll send pull request. [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git/log/?h=ib...
Or you can make the immutable branch and send pull request to Rob and me.
Hi Chanwoo, Archit,
On 07.03.2018 05:48, Chanwoo Choi wrote:
On 2018년 03월 07일 11:12, Chanwoo Choi wrote:
Hi Rob and Andrzej,
On 2018년 03월 06일 21:53, Andrzej Hajda wrote:
Hi Rob, Chanwoo, Krzysztof,
On 27.02.2018 08:11, Andrzej Hajda wrote:
Hi,
Thanks for reviews of previous iterations.
This patchset introduces USB physical connector bindings, together with working example. I have removed RFC prefix - the patchset seems to be heading to a happy end :)
v5: fixed extra parenthesis in dts, renamed extcon function. v4: improved binding descriptions, added missing reg in dts. v3: Separate binding for Samsung 11-pin connector, added full-blown USB-C example. v2: I have addressed comments by Rob and Laurent, thanks
Changes in datail are described in the patches.
Regards Andrzej
Andrzej Hajda (5): dt-bindings: add bindings for USB physical connector dt-bindings: add bindings for Samsung micro-USB 11-pin connector arm64: dts: exynos: add micro-USB connector node to TM2 platforms arm64: dts: exynos: add OF graph between MHL and USB connector extcon: add possibility to get extcon device by OF node
Maciej Purski (1): drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL
It looks like all patches received R-B or acks (I forgot add Krzysztof's acks for dts part). Now I have a question how to merge them. The only functional dependency is between bridge and extcon, and from the formal PoV bindings should be merged 1st. I can merge it:
- All patches via drm-misc tree.
- All patches except dts via drm-misc, and Krzysztof will merge dts via
samsung-soc tree.
Is it OK, for all? Better ideas?
Krzysztof picked the dts patches. I'll make the immutable branch based on v4.16-rc1 and apply them except for dts patchs. And I'll send the immutable branch to Rob and Andrzej.
I made the immutable branch[1] as following: If you agree, I'll send pull request. [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git/log/?h=ib...
Or you can make the immutable branch and send pull request to Rob and me.
It seems you took v5 instead of v6 version of extcon patch.
Beside it I am OK with your immutable branch, Archit is it OK to do it this way in drm-misc?
Regards
Andrzej
On Wed, Mar 7, 2018 at 12:13 PM, Andrzej Hajda a.hajda@samsung.com wrote:
Hi Chanwoo, Archit,
On 07.03.2018 05:48, Chanwoo Choi wrote:
On 2018년 03월 07일 11:12, Chanwoo Choi wrote:
Hi Rob and Andrzej,
On 2018년 03월 06일 21:53, Andrzej Hajda wrote:
Hi Rob, Chanwoo, Krzysztof,
On 27.02.2018 08:11, Andrzej Hajda wrote:
Hi,
Thanks for reviews of previous iterations.
This patchset introduces USB physical connector bindings, together with working example. I have removed RFC prefix - the patchset seems to be heading to a happy end :)
v5: fixed extra parenthesis in dts, renamed extcon function. v4: improved binding descriptions, added missing reg in dts. v3: Separate binding for Samsung 11-pin connector, added full-blown USB-C example. v2: I have addressed comments by Rob and Laurent, thanks
Changes in datail are described in the patches.
Regards Andrzej
Andrzej Hajda (5): dt-bindings: add bindings for USB physical connector dt-bindings: add bindings for Samsung micro-USB 11-pin connector arm64: dts: exynos: add micro-USB connector node to TM2 platforms arm64: dts: exynos: add OF graph between MHL and USB connector extcon: add possibility to get extcon device by OF node
Maciej Purski (1): drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL
It looks like all patches received R-B or acks (I forgot add Krzysztof's acks for dts part). Now I have a question how to merge them. The only functional dependency is between bridge and extcon, and from the formal PoV bindings should be merged 1st. I can merge it:
- All patches via drm-misc tree.
- All patches except dts via drm-misc, and Krzysztof will merge dts via
samsung-soc tree.
Is it OK, for all? Better ideas?
Krzysztof picked the dts patches. I'll make the immutable branch based on v4.16-rc1 and apply them except for dts patchs. And I'll send the immutable branch to Rob and Andrzej.
I made the immutable branch[1] as following: If you agree, I'll send pull request. [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git/log/?h=ib...
Or you can make the immutable branch and send pull request to Rob and me.
It seems you took v5 instead of v6 version of extcon patch.
I also took v5: https://patchwork.kernel.org/patch/10244407/ https://patchwork.kernel.org/patch/10244431/
There was no v6 in samsung-soc patchwork. Is it a problem for DTS?
BR, Krzysztof
On 07.03.2018 12:22, Krzysztof Kozlowski wrote:
On Wed, Mar 7, 2018 at 12:13 PM, Andrzej Hajda a.hajda@samsung.com wrote:
Hi Chanwoo, Archit,
On 07.03.2018 05:48, Chanwoo Choi wrote:
On 2018년 03월 07일 11:12, Chanwoo Choi wrote:
Hi Rob and Andrzej,
On 2018년 03월 06일 21:53, Andrzej Hajda wrote:
Hi Rob, Chanwoo, Krzysztof,
On 27.02.2018 08:11, Andrzej Hajda wrote:
Hi,
Thanks for reviews of previous iterations.
This patchset introduces USB physical connector bindings, together with working example. I have removed RFC prefix - the patchset seems to be heading to a happy end :)
v5: fixed extra parenthesis in dts, renamed extcon function. v4: improved binding descriptions, added missing reg in dts. v3: Separate binding for Samsung 11-pin connector, added full-blown USB-C example. v2: I have addressed comments by Rob and Laurent, thanks
Changes in datail are described in the patches.
Regards Andrzej
Andrzej Hajda (5): dt-bindings: add bindings for USB physical connector dt-bindings: add bindings for Samsung micro-USB 11-pin connector arm64: dts: exynos: add micro-USB connector node to TM2 platforms arm64: dts: exynos: add OF graph between MHL and USB connector extcon: add possibility to get extcon device by OF node
Maciej Purski (1): drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL
It looks like all patches received R-B or acks (I forgot add Krzysztof's acks for dts part). Now I have a question how to merge them. The only functional dependency is between bridge and extcon, and from the formal PoV bindings should be merged 1st. I can merge it:
- All patches via drm-misc tree.
- All patches except dts via drm-misc, and Krzysztof will merge dts via
samsung-soc tree.
Is it OK, for all? Better ideas?
Krzysztof picked the dts patches. I'll make the immutable branch based on v4.16-rc1 and apply them except for dts patchs. And I'll send the immutable branch to Rob and Andrzej.
I made the immutable branch[1] as following: If you agree, I'll send pull request. [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git/log/?h=ib...
Or you can make the immutable branch and send pull request to Rob and me.
It seems you took v5 instead of v6 version of extcon patch.
I also took v5: https://patchwork.kernel.org/patch/10244407/ https://patchwork.kernel.org/patch/10244431/
There was no v6 in samsung-soc patchwork. Is it a problem for DTS?
No, v6 was only typo fix in comment, and only extcon patch has v6.
Regards Andrzej
BR, Krzysztof
Hi Andrzej, Archit,
On 2018년 03월 07일 20:13, Andrzej Hajda wrote:
Hi Chanwoo, Archit,
On 07.03.2018 05:48, Chanwoo Choi wrote:
On 2018년 03월 07일 11:12, Chanwoo Choi wrote:
Hi Rob and Andrzej,
On 2018년 03월 06일 21:53, Andrzej Hajda wrote:
Hi Rob, Chanwoo, Krzysztof,
On 27.02.2018 08:11, Andrzej Hajda wrote:
Hi,
Thanks for reviews of previous iterations.
This patchset introduces USB physical connector bindings, together with working example. I have removed RFC prefix - the patchset seems to be heading to a happy end :)
v5: fixed extra parenthesis in dts, renamed extcon function. v4: improved binding descriptions, added missing reg in dts. v3: Separate binding for Samsung 11-pin connector, added full-blown USB-C example. v2: I have addressed comments by Rob and Laurent, thanks
Changes in datail are described in the patches.
Regards Andrzej
Andrzej Hajda (5): dt-bindings: add bindings for USB physical connector dt-bindings: add bindings for Samsung micro-USB 11-pin connector arm64: dts: exynos: add micro-USB connector node to TM2 platforms arm64: dts: exynos: add OF graph between MHL and USB connector extcon: add possibility to get extcon device by OF node
Maciej Purski (1): drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL
It looks like all patches received R-B or acks (I forgot add Krzysztof's acks for dts part). Now I have a question how to merge them. The only functional dependency is between bridge and extcon, and from the formal PoV bindings should be merged 1st. I can merge it:
- All patches via drm-misc tree.
- All patches except dts via drm-misc, and Krzysztof will merge dts via
samsung-soc tree.
Is it OK, for all? Better ideas?
Krzysztof picked the dts patches. I'll make the immutable branch based on v4.16-rc1 and apply them except for dts patchs. And I'll send the immutable branch to Rob and Andrzej.
I made the immutable branch[1] as following: If you agree, I'll send pull request. [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git/log/?h=ib...
Or you can make the immutable branch and send pull request to Rob and me.
It seems you took v5 instead of v6 version of extcon patch.
My mistake. I picked v6 and made the new immutable branch. After Archit confirm it, I'll send pull request.
Beside it I am OK with your immutable branch, Archit is it OK to do it this way in drm-misc?
Regards
Andrzej
-- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Chanwoo,
On 08.03.2018 02:52, Chanwoo Choi wrote:
Hi Andrzej, Archit,
On 2018년 03월 07일 20:13, Andrzej Hajda wrote:
Hi Chanwoo, Archit,
On 07.03.2018 05:48, Chanwoo Choi wrote:
On 2018년 03월 07일 11:12, Chanwoo Choi wrote:
Hi Rob and Andrzej,
On 2018년 03월 06일 21:53, Andrzej Hajda wrote:
Hi Rob, Chanwoo, Krzysztof,
On 27.02.2018 08:11, Andrzej Hajda wrote:
Hi,
Thanks for reviews of previous iterations.
This patchset introduces USB physical connector bindings, together with working example. I have removed RFC prefix - the patchset seems to be heading to a happy end :)
v5: fixed extra parenthesis in dts, renamed extcon function. v4: improved binding descriptions, added missing reg in dts. v3: Separate binding for Samsung 11-pin connector, added full-blown USB-C example. v2: I have addressed comments by Rob and Laurent, thanks
Changes in datail are described in the patches.
Regards Andrzej
Andrzej Hajda (5): dt-bindings: add bindings for USB physical connector dt-bindings: add bindings for Samsung micro-USB 11-pin connector arm64: dts: exynos: add micro-USB connector node to TM2 platforms arm64: dts: exynos: add OF graph between MHL and USB connector extcon: add possibility to get extcon device by OF node
Maciej Purski (1): drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL
It looks like all patches received R-B or acks (I forgot add Krzysztof's acks for dts part). Now I have a question how to merge them. The only functional dependency is between bridge and extcon, and from the formal PoV bindings should be merged 1st. I can merge it:
- All patches via drm-misc tree.
- All patches except dts via drm-misc, and Krzysztof will merge dts via
samsung-soc tree.
Is it OK, for all? Better ideas?
Krzysztof picked the dts patches. I'll make the immutable branch based on v4.16-rc1 and apply them except for dts patchs. And I'll send the immutable branch to Rob and Andrzej.
I made the immutable branch[1] as following: If you agree, I'll send pull request. [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git/log/?h=ib...
Or you can make the immutable branch and send pull request to Rob and me.
It seems you took v5 instead of v6 version of extcon patch.
My mistake. I picked v6 and made the new immutable branch. After Archit confirm it, I'll send pull request.
Lets just queue all patches (except dts) via extcon branch (thanks Daniel for advice), without making immutable branch. It seems to be a simplest acceptable approach.
You can add my ack to sii8620 bridge patch (as bridge maintainer), to fulfill process requirements.
Regards Andrzej
Hi Andrzej and Rob,
On 2018년 03월 09일 18:20, Andrzej Hajda wrote:
Hi Chanwoo,
On 08.03.2018 02:52, Chanwoo Choi wrote:
Hi Andrzej, Archit,
On 2018년 03월 07일 20:13, Andrzej Hajda wrote:
Hi Chanwoo, Archit,
On 07.03.2018 05:48, Chanwoo Choi wrote:
On 2018년 03월 07일 11:12, Chanwoo Choi wrote:
Hi Rob and Andrzej,
On 2018년 03월 06일 21:53, Andrzej Hajda wrote:
Hi Rob, Chanwoo, Krzysztof,
On 27.02.2018 08:11, Andrzej Hajda wrote: > Hi, > > Thanks for reviews of previous iterations. > > This patchset introduces USB physical connector bindings, together with > working example. > I have removed RFC prefix - the patchset seems to be heading > to a happy end :) > > v5: fixed extra parenthesis in dts, renamed extcon function. > v4: improved binding descriptions, added missing reg in dts. > v3: Separate binding for Samsung 11-pin connector, added full-blown USB-C > example. > v2: I have addressed comments by Rob and Laurent, thanks > > Changes in datail are described in the patches. > > Regards > Andrzej > > > Andrzej Hajda (5): > dt-bindings: add bindings for USB physical connector > dt-bindings: add bindings for Samsung micro-USB 11-pin connector > arm64: dts: exynos: add micro-USB connector node to TM2 platforms > arm64: dts: exynos: add OF graph between MHL and USB connector > extcon: add possibility to get extcon device by OF node > > Maciej Purski (1): > drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL It looks like all patches received R-B or acks (I forgot add Krzysztof's acks for dts part). Now I have a question how to merge them. The only functional dependency is between bridge and extcon, and from the formal PoV bindings should be merged 1st. I can merge it:
- All patches via drm-misc tree.
- All patches except dts via drm-misc, and Krzysztof will merge dts via
samsung-soc tree.
Is it OK, for all? Better ideas?
Krzysztof picked the dts patches. I'll make the immutable branch based on v4.16-rc1 and apply them except for dts patchs. And I'll send the immutable branch to Rob and Andrzej.
I made the immutable branch[1] as following: If you agree, I'll send pull request. [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git/log/?h=ib...
Or you can make the immutable branch and send pull request to Rob and me.
It seems you took v5 instead of v6 version of extcon patch.
My mistake. I picked v6 and made the new immutable branch. After Archit confirm it, I'll send pull request.
Lets just queue all patches (except dts) via extcon branch (thanks Daniel for advice), without making immutable branch. It seems to be a simplest acceptable approach.
You can add my ack to sii8620 bridge patch (as bridge maintainer), to fulfill process requirements.
I added your ack to sii8620 bridge patch and send pull request for extcon, drm bridge and device-tree.
-----------------------
The following changes since commit 7928b2cbe55b2a410a0f5c1f154610059c57b1b2:
Linux 4.16-rc1 (2018-02-11 15:04:29 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git ib-extcon-drm-dt-v4.17
for you to fetch changes up to 688838442147d9dd94c2ef7c2c31a35cf150c5fa:
drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL (2018-03-12 10:18:26 +0900)
---------------------------------------------------------------- Andrzej Hajda (3): dt-bindings: add bindings for USB physical connector dt-bindings: add bindings for Samsung micro-USB 11-pin connector extcon: add possibility to get extcon device by OF node
Maciej Purski (1): drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL
.../connector/samsung,usb-connector-11pin.txt | 49 +++++++++++ .../bindings/connector/usb-connector.txt | 75 +++++++++++++++++ drivers/extcon/extcon.c | 44 +++++++--- drivers/gpu/drm/bridge/sil-sii8620.c | 97 +++++++++++++++++++++- include/linux/extcon.h | 6 ++ 5 files changed, 258 insertions(+), 13 deletions(-) create mode 100644 Documentation/devicetree/bindings/connector/samsung,usb-connector-11pin.txt create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Chanwoo Choi -
Heikki Krogerus -
Krzysztof Kozlowski -
Rob Herring -
Robin Murphy -
Roger Quadros