[PATCH v17 4/5] media: qcom: camss: Add support for PHY API devices

[Bryan O'Donoghue]

On 06/09/2026 15:32, sashiko-bot at kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Selects a non-existent Kconfig symbol PHY_QCOM_MIPI_CSI2.
> - [High] Hardware PHY state and resources are leaked on early probe and registration error paths.
> - [Medium] Missing return value checks for phy_mipi_dphy_get_default_config_for_hsclk() and phy_set_mode() can lead to uninitialized PHY configuration.
> --
> 
> commit cc3786ea5902bcb687c49374e8e8db29c5127496
> Author: Bryan O'Donoghue <bryan.odonoghue at linaro.org>
> 
> media: qcom: camss: Add support for PHY API devices
> 
> Add the ability to use a PHY pointer which interacts with the standard PHY
> API. In the first instance the code will try to use the new PHY interface.
> If no PHYs are present in the DT then the legacy method will be attempted.
> 
>> diff --git a/drivers/media/platform/qcom/camss/Kconfig b/drivers/media/platform/qcom/camss/Kconfig
>> index 4eda48cb1adf0..1edc5e5a1829e 100644
>> --- a/drivers/media/platform/qcom/camss/Kconfig
>> +++ b/drivers/media/platform/qcom/camss/Kconfig
>> @@ -7,3 +7,4 @@ config VIDEO_QCOM_CAMSS
>>   	select VIDEO_V4L2_SUBDEV_API
>>   	select VIDEOBUF2_DMA_SG
>>   	select V4L2_FWNODE
>> +	select PHY_QCOM_MIPI_CSI2
> 
> [Severity: High]
> Does this select a nonexistent config symbol? Selecting a symbol that isn't
> defined anywhere in the kernel causes a silent build dependency failure,
> which will leave the required PHY infrastructure unbuilt.

No the dependency is described in the cover-letter
> 
> [ ... ]
> 
>> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
>> index 539ac4888b608..e00748dd83b02 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> 
> [ ... ]
> 
>> @@ -306,11 +308,88 @@ static int csiphy_stream_on(struct csiphy_device *csiphy)
>> +	link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp, num_lanes);
>> +
>> +	if (link_freq < 0) {
>> +		dev_err(dev,
>> +			"Cannot get CSI2 transmitter's link frequency\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	phy_mipi_dphy_get_default_config_for_hsclk(link_freq, num_lanes, dphy_cfg);
>> +
>> +	phy_set_mode(csiphy->phy, PHY_MODE_MIPI_DPHY);
>> +
>> +	ret = phy_configure(csiphy->phy, &dphy_opts);
> 
> [Severity: Medium]
> Should we check the return values of phy_mipi_dphy_get_default_config_for_hsclk()
> and phy_set_mode() here?
> 
> If link_freq evaluates to 0, it appears phy_mipi_dphy_get_default_config_for_hsclk()
> could return -EINVAL and leave the output dphy_opts structure zero-filled. Passing
> this uninitialized configuration directly to phy_configure() could lead to
> erratic hardware behavior.

Yeah but no link_freq <= 0 should cover that case. I'll add the 
evaluation of the return types anyway.

> 
> [ ... ]
> 
>> @@ -709,6 +788,56 @@ int msm_csiphy_subdev_init(struct camss *camss,
>> +	ret = phy_init(csiphy->phy);
>> +	if (ret)
>> +		dev_err(dev, "%s init fail %d\n", csiphy->name, ret);
>> +
>> +done:
>> +	return ret;
>> +}
> 
> [ ... ]
> 
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index 2123f6388e3d7..84097d82d99c9 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
> 
> [ ... ]
> 
>> @@ -4832,14 +4890,21 @@ static int camss_init_subdevices(struct camss *camss)
>>   	unsigned int i;
>>   	int ret;
>>   
>> +	camss_detect_legacy_phy(camss);
>> +
>>   	for (i = 0; i < camss->res->csiphy_num; i++) {
>> -		ret = msm_csiphy_subdev_init(camss, &camss->csiphy[i],
>> -					     &res->csiphy_res[i],
>> -					     res->csiphy_res[i].csiphy.id);
>> +		if (!camss->legacy_phy) {
>> +			ret = msm_csiphy_subdev_init(camss, i);
>> +		} else {
>> +			ret = msm_csiphy_subdev_init_legacy(camss,
>> +							    &camss->csiphy[i],
>> +							    &res->csiphy_res[i],
>> +							    res->csiphy_res[i].csiphy.id);
>> +		}
>> +
>>   		if (ret < 0) {
>> -			dev_err(camss->dev,
>> -				"Failed to init csiphy%d sub-device: %d\n",
>> -				i, ret);
>> +			dev_err(camss->dev, "csiphy %d init fail\n",
>> +				res->csiphy_res[i].csiphy.id);
>>   			return ret;
>>   		}
>>   	}
> 
> [Severity: High]
> Does this error path leak the PHY initialization state?
> 
> When msm_csiphy_subdev_init() succeeds, it calls phy_init() to acquire the PHY.
> If a subsequent iteration of this loop fails, or if a later initialization step
> like camss_probe() fails, the code returns immediately or goes to an error label
> without rolling back the previously initialized PHYs (which require phy_exit()).
> 
> Additionally, in msm_csiphy_register_entity() located in camss-csiphy.c:
> 
> 	ret = csiphy_init_formats(sd, NULL);
> 	if (ret < 0) {
> 		dev_err(dev, "Failed to init format: %d\n", ret);
> 		return ret;
> 	}
> 
> A failure here returns early without calling phy_exit(). When the calling code
> in camss_register_entities() rolls back, it decrements the index and skips this
> failing entity, which seems to permanently leak its PHY initialization state.
> 

yes missed the unwind.

---
bod