Re: [PATCH v2 07/10] arm64: dts: qcom: sa8775p-ride: add anx7625 DSI to DP bridge nodes

[Ayushi Makhija <quic_amakhija@quicinc.com>]

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