Re: [RFC PATCH v2] phy: qcom-mipi-csi2: round the requested settle interval up

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] phy: qcom-mipi-csi2: valid 0-value settle count rejected as error
--

commit 1f8cc2f9aa959801a8f709525f71df45c6300c09
Author: Jizhou Tong <fqwqf@fqwqf.xyz>

phy: qcom-mipi-csi2: round the requested settle interval up

The settling counter represents the requested interval in timer ticks,
with the existing six-tick hardware offset. Truncating the division
programs fewer ticks than requested when the interval is not an exact
multiple of the timer period. Rounding the final division alone is
insufficient, as the earlier unit interval calculation already rounds down.

Link: https://lore.kernel.org/all/sashiko-outbox-166399@kernel.org/

> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> index 966d79c98f9b2..f07699ba1fde6 100644
> --- a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> @@ -220,23 +221,19 @@ static void phy_qcom_mipi_csi2_reset(struct mipi_csi2phy_device *csi2phy)
>   */
>  static u8 phy_qcom_mipi_csi2_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
>  {
[ ... ]
> -	if ((t_hs_settle_ps / timer_period_ps) < 6)
> +	if (div_u64(t_hs_settle_ps, timer_period_ps) < 6)
>  		return 0;
>  
> -	settle_cnt = t_hs_settle_ps / timer_period_ps - 6;
> +	settle_cnt = DIV_U64_ROUND_UP(t_hs_settle_ps, timer_period_ps) - 6;
>  
>  	return settle_cnt;
>  }

[Severity: Medium]
If the required settle time evaluates to exactly 6 timer periods,
DIV_U64_ROUND_UP(t_hs_settle_ps, timer_period_ps) will equal 6, and
settle_cnt will correctly evaluate to 0.

However, the caller phy_qcom_mipi_csi2_lanes_enable() treats a 0 return
value as an error:

drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c:phy_qcom_mipi_csi2_lanes_enable() {
    ...
	settle_cnt = phy_qcom_mipi_csi2_settle_cnt_calc(cfg->link_freq, csi2phy->timer_clk_rate);
	if (!settle_cnt)
		return -ENODEV;
    ...
}

Because a 0 count is a valid hardware configuration (indicating no additional
timer ticks are needed beyond the baseline 6), does this wrongfully fail
device initialization for combinations of link frequencies and timer clocks
that exactly require 6 timer periods?

Could the caller's error handling be updated to differentiate between a valid
0 count and an actual error, perhaps by having
phy_qcom_mipi_csi2_settle_cnt_calc() return a negative error code for invalid
configurations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/010601a1332f343c-b1381230-9344-4bef-a709-99e07b18d513-000000@ap-northeast-1.amazonses.com?part=1

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