Re: [PATCH 2/2] phy: qcom: usb-hs: honour qcom,vendor-init-seq raw ULPI writes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Using `seq->addr == 0` as a loop terminator prevents writing to offset 0 in `qcom,init-seq`.
--

commit 23768611bb7332cb583754c2260443d91ea51fb4
Author: Herman van Hazendonk <github.com@herrie.org>

phy: qcom: usb-hs: honour qcom,vendor-init-seq raw ULPI writes

This patch adds support for the optional qcom,vendor-init-seq DT property
to write directly to raw ULPI register addresses. It also hardens the parse
logic shared between qcom,init-seq and qcom,vendor-init-seq by factoring it
out into qcom_usb_hs_phy_parse_init_seq().

> diff --git a/drivers/phy/qualcomm/phy-qcom-usb-hs.c b/drivers/phy/qualcomm/phy-qcom-usb-hs.c
> index 98a18987f1bec..89fbe8f8d0015 100644
> --- a/drivers/phy/qualcomm/phy-qcom-usb-hs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-usb-hs.c
[ ... ]
> @@ -199,6 +219,59 @@ static const struct phy_ops qcom_usb_hs_phy_ops = {
[ ... ]
> +static int qcom_usb_hs_phy_parse_init_seq(struct ulpi *ulpi,
> +					  const char *propname,
> +					  struct ulpi_seq **out)
> +{
[ ... ]
> +	if (size) {
> +		int ret = of_property_read_u8_array(ulpi->dev.of_node,
> +						    propname, (u8 *)seq, size);
> +		if (ret)
> +			return ret;
> +	}
> +	/* NUL-terminate so the power_on loop's seq->addr-as-sentinel works. */
> +	seq[size / 2].addr = 0;

[Severity: High]
This is a pre-existing issue, but does using seq->addr == 0 as a sentinel
prevent writing to offset 0 in qcom,init-seq?

In qcom_usb_hs_phy_power_on(), the initialization loop terminates when
seq->addr is 0:

	for (seq = uphy->init_seq; seq->addr; seq++) {
		ret = ulpi_write(ulpi, ULPI_EXT_VENDOR_SPECIFIC + seq->addr,
				 seq->val);

Since seq->addr represents an offset added to ULPI_EXT_VENDOR_SPECIFIC
for qcom,init-seq, an offset of 0 is valid and maps to the first vendor
specific register. If a device tree specifies a write to offset 0, won't
the loop prematurely terminate and silently truncate the remainder of the
initialization sequence?

> +	seq[size / 2].val = 0;
> +
> +	*out = seq;
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603054809.565723-1-github.com@herrie.org?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy