| Message ID | 20250311122445.3597100-8-quic_amakhija@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add DSI display support for SA8775P target | expand |
On Tue, Mar 11, 2025 at 05:54:42PM +0530, Ayushi Makhija wrote: > Add anx7625 DSI to DP bridge device nodes. > > Signed-off-by: Ayushi Makhija <quic_amakhija@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 208 ++++++++++++++++++++- > 1 file changed, 207 insertions(+), 1 deletion(-) > So you just gave up after one comment? Context of every email should be trimmed, so if it is not trimmed means something is still there. I know there are reviewers who respond with huge unrelated context, but that's just disrespectful to our time and don't take it as normal. <form letter> This is a friendly reminder during the review process. It seems my or other reviewer's previous comments were not fully addressed. Maybe the feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. </form letter>
On 3/12/2025 5:18 PM, Krzysztof Kozlowski wrote: > On Tue, Mar 11, 2025 at 05:54:42PM +0530, Ayushi Makhija wrote: >> Add anx7625 DSI to DP bridge device nodes. >> >> Signed-off-by: Ayushi Makhija <quic_amakhija@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 208 ++++++++++++++++++++- >> 1 file changed, 207 insertions(+), 1 deletion(-) >> > > So you just gave up after one comment? Context of every email should be > trimmed, so if it is not trimmed means something is still there. I know > there are reviewers who respond with huge unrelated context, but that's > just disrespectful to our time and don't take it as normal. > > <form letter> > This is a friendly reminder during the review process. > > It seems my or other reviewer's previous comments were not fully > addressed. Maybe the feedback got lost between the quotes, maybe you > just forgot to apply it. Please go back to the previous discussion and > either implement all requested changes or keep discussing them. > > Thank you. > </form letter> > Hi Krzysztof, Thanks, for the review. I apologize for any confusion or oversight regarding the recent review comments. Thank you for your patience and understanding. I value your time and feedback and will work to improve the review process. Below are the comments on the patch 7 and patch 8 of the version 1 of the series, that I have addressed in version 2 of patch 7 of the series. Let me know, If I did some mistake or if you have any other suggestions. Comments from Konard: comment 1 > - pinctrl-0 = <&qup_i2c18_default>; > + pinctrl-0 = <&qup_i2c18_default>, > + <&io_expander_intr_active>, > + <&io_expander_reset_active>; Please align the '<'s comment 2 > + interrupt-parent = <&tlmm>; > + interrupts = <98 IRQ_TYPE_EDGE_BOTH>; use interrupts-extended, here and below These above two comments were from the konard in patch 7 in version 1 of the series. I have addressed both the above comments in the version 2 of patch 7 of the series. Comments from Krzysztof: comment 1 > + > + dsi0_int_pin: gpio2_cfg { No underscores, see DTS coding style. I have corrected the above comment in the version 2 of patch 7 of the series. comment 2 > + > + anx_bridge_1: anx7625@58 { Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation In this I have changed the node name as anx_bridge1 : anx7625@58. Let me know, if I did some mistake or you have any other suggestion over the node name. I have took the reference from below: linux/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi.dtsi at 629c635eafbaf18260c8083360745c71674640d2 · torvalds/linux · GitHub comment 3 > + enable-gpios = <&io_expander 1 0>; > + reset-gpios = <&io_expander 0 0>; Use proper defines. For this above comment, I have changed above lines into below lines in patch 7 of version 2 of the series. > + enable-gpios = <&io_expander 1 GPIO_ACTIVE_HIGH>; > + reset-gpios = <&io_expander 0 GPIO_ACTIVE_HIGH>; comment 4 > + > + anx_bridge_2: anx7625@58 { Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation In this I have changed the node name as anx_bridge2 : anx7625@58. Let me know, if I did some mistake or you have any other suggestion over the node name. I have took the reference from below: linux/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi.dtsi at 629c635eafbaf18260c8083360745c71674640d2 · torvalds/linux · GitHub comment 5 And as Rob's bot pointed out: insufficient testing. :( Please be 100% sure everything is tested before you post new version. You shouldn't use reviewers for the job of tools, that's quite waste of our time. Fixed the above warning from DT checker against DT binding in patch 7 of version 2 of the series. Comments from Dmitry: comment 1 Missing dp-connector devices. Please add them together with the bridges. comment 2 Please squash into the previous patch. It doesn't make a lot of sense separately. These both above commented from Dmitry I have addressed in the version 2 of patch 7 of the series. I have squash patch 8 into patch 7 of version 1 into patch 7 of version 2 of the series. Thanks, Ayushi
> These both above commented from Dmitry I have addressed in the version 2 of patch 7 of the series. > I have squash patch 8 into patch 7 of version 1 into patch 7 of version 2 of the series. > > > Thanks, > Ayushi Hi Krzysztof, I hope this message finds you well. I wanted to follow up on the reply I sent. Your feedback is invaluable to us, and we would greatly appreciate any further insights or comments you might have. Thanks, Ayushi
On Fri, Mar 28, 2025 at 03:13:57PM +0530, Ayushi Makhija wrote: > > These both above commented from Dmitry I have addressed in the version 2 of patch 7 of the series. > > I have squash patch 8 into patch 7 of version 1 into patch 7 of version 2 of the series. > > > > > > Thanks, > > Ayushi > > Hi Krzysztof, > > I hope this message finds you well. I wanted to follow up on the reply I sent. Your feedback is invaluable to us, and we would greatly appreciate any further insights or comments you might have. > Granted the lack of response, please make sure that you've addressed all the comments and proceed with the next iteration of the patchset.
On 28/03/2025 13:45, Dmitry Baryshkov wrote: > On Fri, Mar 28, 2025 at 03:13:57PM +0530, Ayushi Makhija wrote: >>> These both above commented from Dmitry I have addressed in the version 2 of patch 7 of the series. >>> I have squash patch 8 into patch 7 of version 1 into patch 7 of version 2 of the series. >>> >>> >>> Thanks, >>> Ayushi >> >> Hi Krzysztof, >> >> I hope this message finds you well. I wanted to follow up on the reply I sent. Your feedback is invaluable to us, and we would greatly appreciate any further insights or comments you might have. >> > > Granted the lack of response, please make sure that you've addressed all > the comments and proceed with the next iteration of the patchset. Just to clarify, I did not plan to respond here, because email style which tries to respond to my comments is unreadable. It's impossible to find what is quote, what is the comment and what is the response. I expected inline responses to the original emails and detailed changelog. Best regards, Krzysztof
On 13/03/2025 13:10, Ayushi Makhija wrote: ... > >> + >> + anx_bridge_1: anx7625@58 { > > Node names should be generic. See also an explanation and list of > examples (not exhaustive) in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > In this I have changed the node name as anx_bridge1 : anx7625@58. Except that it is difficult to understand what is what, let's recap. Original code was: anx_bridge_1: anx7625@58 { You said you changed it to: anx_bridge1 : anx7625@58. and now I give my offer: I offer to buy a beer (or tee/coffee/juice) to anyone who will spot the difference(s) between these two node names, IOW, tell me what changed here. Best regards, Krzysztof
On Fri, 28 Mar 2025 at 16:22, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 28/03/2025 13:45, Dmitry Baryshkov wrote: > > On Fri, Mar 28, 2025 at 03:13:57PM +0530, Ayushi Makhija wrote: > >>> These both above commented from Dmitry I have addressed in the version 2 of patch 7 of the series. > >>> I have squash patch 8 into patch 7 of version 1 into patch 7 of version 2 of the series. > >>> > >>> > >>> Thanks, > >>> Ayushi > >> > >> Hi Krzysztof, > >> > >> I hope this message finds you well. I wanted to follow up on the reply I sent. Your feedback is invaluable to us, and we would greatly appreciate any further insights or comments you might have. > >> > > > > Granted the lack of response, please make sure that you've addressed all > > the comments and proceed with the next iteration of the patchset. > > Just to clarify, I did not plan to respond here, because email style > which tries to respond to my comments is unreadable. It's impossible to > find what is quote, what is the comment and what is the response. > > I expected inline responses to the original emails and detailed changelog. Works for me. I'd say, let's get the next revision and check if it resolves your comments or we have more comments.
diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi index 175f8b1e3b2d..77d86c1d8aa6 100644 --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi @@ -28,6 +28,13 @@ chosen { stdout-path = "serial0:115200n8"; }; + vph_pwr: vph-pwr-regulator { + compatible = "regulator-fixed"; + regulator-name = "vph_pwr"; + regulator-always-on; + regulator-boot-on; + }; + vreg_conn_1p8: vreg_conn_1p8 { compatible = "regulator-fixed"; regulator-name = "vreg_conn_1p8"; @@ -128,6 +135,30 @@ dp1_connector_in: endpoint { }; }; }; + + dp-dsi0-connector { + compatible = "dp-connector"; + label = "DSI0"; + type = "full-size"; + + port { + dp_dsi0_connector_in: endpoint { + remote-endpoint = <&anx7625_1_out>; + }; + }; + }; + + dp-dsi1-connector { + compatible = "dp-connector"; + label = "DSI1"; + type = "full-size"; + + port { + dp_dsi1_connector_in: endpoint { + remote-endpoint = <&anx7625_2_out>; + }; + }; + }; }; &apps_rsc { @@ -517,9 +548,135 @@ &i2c11 { &i2c18 { clock-frequency = <400000>; - pinctrl-0 = <&qup_i2c18_default>; + pinctrl-0 = <&qup_i2c18_default>, + <&io_expander_intr_active>, + <&io_expander_reset_active>; pinctrl-names = "default"; + status = "okay"; + + io_expander: gpio@74 { + compatible = "ti,tca9539"; + reg = <0x74>; + interrupts-extended = <&tlmm 98 IRQ_TYPE_EDGE_BOTH>; + gpio-controller; + #gpio-cells = <2>; + interrupt-controller; + #interrupt-cells = <2>; + + gpio2-hog { + gpio-hog; + gpios = <2 GPIO_ACTIVE_HIGH>; + input; + line-name = "dsi0_int_pin"; + }; + + gpio3-hog { + gpio-hog; + gpios = <3 GPIO_ACTIVE_LOW>; + output-high; + line-name = "dsi0_cbl_det_pin"; + }; + + gpio10-hog { + gpio-hog; + gpios = <10 GPIO_ACTIVE_HIGH>; + input; + line-name = "dsi1_int_pin"; + }; + + gpio11-hog { + gpio-hog; + gpios = <11 GPIO_ACTIVE_LOW>; + output-high; + line-name = "dsi1_cbl_det_pin"; + }; + }; + + i2c-mux@70 { + compatible = "nxp,pca9543"; + #address-cells = <1>; + + #size-cells = <0>; + reg = <0x70>; + + i2c@0 { + reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + + anx_bridge1: anx7625@58 { + compatible = "analogix,anx7625"; + reg = <0x58>; + interrupts-extended = <&io_expander 2 IRQ_TYPE_EDGE_FALLING>; + enable-gpios = <&io_expander 1 GPIO_ACTIVE_HIGH>; + reset-gpios = <&io_expander 0 GPIO_ACTIVE_HIGH>; + vdd10-supply = <&vph_pwr>; + vdd18-supply = <&vph_pwr>; + vdd33-supply = <&vph_pwr>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + dsi2dp_bridge_1_in: port@0 { + reg = <0>; + + anx7625_1_in: endpoint { + remote-endpoint = <&mdss0_dsi0_out>; + }; + }; + + dsi2dp_bridge_1_out: port@1 { + reg = <1>; + + anx7625_1_out: endpoint { + remote-endpoint = <&dp_dsi0_connector_in>; + }; + }; + }; + }; + }; + + i2c@1 { + reg = <1>; + #address-cells = <1>; + #size-cells = <0>; + + anx_bridge2: anx7625@58 { + compatible = "analogix,anx7625"; + reg = <0x58>; + interrupts-extended = <&io_expander 10 IRQ_TYPE_EDGE_FALLING>; + enable-gpios = <&io_expander 9 GPIO_ACTIVE_HIGH>; + reset-gpios = <&io_expander 8 GPIO_ACTIVE_HIGH>; + vdd10-supply = <&vph_pwr>; + vdd18-supply = <&vph_pwr>; + vdd33-supply = <&vph_pwr>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + dsi2dp_bridge_2_in: port@0 { + reg = <0>; + + anx7625_2_in: endpoint { + remote-endpoint = <&mdss0_dsi1_out>; + }; + }; + + dsi2dp_bridge_2_out: port@1 { + reg = <1>; + + anx7625_2_out: endpoint { + remote-endpoint = <&dp_dsi1_connector_in>; + }; + }; + }; + }; + }; + }; + }; &mdss0 { @@ -566,6 +723,40 @@ &mdss0_dp1_phy { status = "okay"; }; +&mdss0_dsi0 { + vdda-supply = <&vreg_l1c>; + + status = "okay"; +}; + +&mdss0_dsi0_out { + data-lanes = <0 1 2 3>; + remote-endpoint = <&anx7625_1_in>; +}; + +&mdss0_dsi0_phy { + vdds-supply = <&vreg_l4a>; + + status = "okay"; +}; + +&mdss0_dsi1 { + vdda-supply = <&vreg_l1c>; + + status = "okay"; +}; + +&mdss0_dsi1_out { + data-lanes = <0 1 2 3>; + remote-endpoint = <&anx7625_2_in>; +}; + +&mdss0_dsi1_phy { + vdds-supply = <&vreg_l4a>; + + status = "okay"; +}; + &pmm8654au_0_gpios { gpio-line-names = "DS_EN", "POFF_COMPLETE", @@ -714,6 +905,21 @@ ethernet0_mdio: ethernet0-mdio-pins { }; }; + io_expander_intr_active: io-expander-intr-active-state { + pins = "gpio98"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + }; + + io_expander_reset_active: io-expander-reset-active-state { + pins = "gpio97"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + output-high; + }; + qup_uart10_default: qup-uart10-state { pins = "gpio46", "gpio47"; function = "qup1_se3";
Add anx7625 DSI to DP bridge device nodes. Signed-off-by: Ayushi Makhija <quic_amakhija@quicinc.com> --- arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 208 ++++++++++++++++++++- 1 file changed, 207 insertions(+), 1 deletion(-)