Re: [PATCH 07/10] phy: qcom-ipq806x-usb: parse the tx-deamp-3_5db property as documented

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The patch breaks backward compatibility with existing device trees by replacing the `qcom,tx-deamp_3_5db` property with `qcom,tx-deamp-3_5db` without providing a fallback. The commit message incorrectly claims that no in-tree device tree uses either spelling, but `arch/arm/boot/dts/qcom/qcom-ipq8064-v2.0.dtsi` actively sets `qcom,tx-deamp_3_5db = <32>;`. Consequently, the driver will silently fail to read this property from existing deployed DTBs and fall back to the default value, leading to an unintended hardware configuration change.
--

--- Patch [7]: [PATCH 07/10] phy: qcom-ipq806x-usb: parse the tx-deamp-3_5db property as documented ---
commit 0d730340df80865649e4381722ca35e1603f1cad
Author: Dmitry Baryshkov <[email protected]>

phy: qcom-ipq806x-usb: parse the tx-deamp-3_5db property as documented

This commit updates the driver to parse the "qcom,tx-deamp-3_5db" property,
matching the device tree binding documentation. It replaces the old
"qcom,tx-deamp_3_5db" spelling.

> The driver reads the transmit de-emphasis override from
> qcom,tx-deamp_3_5db while the binding documents qcom,tx-deamp-3_5db,
> so a device tree following the binding is silently ignored and the
> default is used instead. No in-tree device tree uses either spelling.

Is the statement about in-tree device trees accurate? Looking at the source,
arch/arm/boot/dts/qcom/qcom-ipq8064-v2.0.dtsi actively sets
"qcom,tx-deamp_3_5db = <32>;".

> diff --git a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> index 3d3e9d6da298c..86e8e869a743f 100644
> --- a/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> +++ b/drivers/phy/qualcomm/phy-qcom-ipq806x-usb.c
> @@ -561,7 +561,7 @@ static int qcom_ipq806x_usb_phy_probe(struct platform_device *pdev)
>  				     &phy_dwc3->rx_eq))
>  		phy_dwc3->rx_eq = SSPHY_RX_EQ_VALUE;
>  
> -	if (device_property_read_u32(&pdev->dev, "qcom,tx-deamp_3_5db",
> +	if (device_property_read_u32(&pdev->dev, "qcom,tx-deamp-3_5db",

[Severity: Critical]
Does replacing this property without a fallback introduce a regression for
backward compatibility with existing device trees?

Since device tree bindings are a stable kernel ABI, systems booting with an
older DTB (or the current in-tree DTB) might silently fail to read this
property and fall back to the default value instead of the configured value.

Should qcom_ipq806x_usb_phy_probe() maintain support for the legacy
underscore property as a fallback?

>  				     &phy_dwc3->tx_deamp_3_5db))
>  		phy_dwc3->tx_deamp_3_5db = SSPHY_TX_DEEMPH_3_5DB;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy