Re: [PATCH WIP v5 2/9] media: qcom: camss: csiphy-3ph: Use odd bits for configuring C-PHY lanes

On 01/06/2026 13:48, David Heidelberg wrote:
> On 01/06/2026 10:49, Sakari Ailus wrote:
>> Hi David,
>>
>> Thanks for the patch.
>>
>> On Sun, May 31, 2026 at 03:08:10PM +0200, David Heidelberg via B4 Relay wrote:
>>> From: David Heidelberg <[email protected]>
>>>
>>> So far, only D-PHY mode was supported, which uses even bits when enabling
>>> or masking lanes. For C-PHY configuration, the hardware instead requires
>>> using the odd bits.
>>>
>>> Since there can be unrecognized configuration allow returning failure.
>>>
>>> Reviewed-by: Bryan O'Donoghue <[email protected]>
>>> Acked-by: Cory Keitz <[email protected]>
>>> Signed-off-by: David Heidelberg <[email protected]>
>>> ---
>>>    .../platform/qcom/camss/camss-csiphy-2ph-1-0.c     |  8 ++--
>>>    .../platform/qcom/camss/camss-csiphy-3ph-1-0.c     | 49 +++++++++++++++++-----
>>>    drivers/media/platform/qcom/camss/camss-csiphy.c   |  5 +--
>>>    drivers/media/platform/qcom/camss/camss-csiphy.h   |  6 +--
>>>    4 files changed, 48 insertions(+), 20 deletions(-)
>>>
> 
> [...]
> 
>>> -static void csiphy_lanes_enable(struct csiphy_device *csiphy,
>>> -				struct csiphy_config *cfg,
>>> -				s64 link_freq, u8 lane_mask)
>>> +static int csiphy_lanes_enable(struct csiphy_device *csiphy,
>>> +			       struct csiphy_config *cfg,
>>> +			       s64 link_freq, u8 lane_mask)
>>>    {
>>> +	struct device *dev = csiphy->camss->dev;
>>>    	struct csiphy_lanes_cfg *c = &cfg->csi2->lane_cfg;
>>>    	struct csiphy_device_regs *regs = csiphy->regs;
>>>    	u8 settle_cnt;
>>>    	u8 val;
>>>    	int i;
>>>
>>>    	settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
>>>
>>> -	val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
>>> -	for (i = 0; i < c->num_data; i++)
>>> -		val |= BIT(c->data[i].pos * 2);
>>> +	val = 0;
>>> +
>>> +	switch (c->phy_cfg) {
>>> +	case V4L2_MBUS_CSI2_CPHY:
>>> +		for (i = 0; i < c->num_data; i++)
>>> +			val |= BIT((c->data[i].pos * 2) + 1);
>>> +		break;
>>> +	case V4L2_MBUS_CSI2_DPHY:
>>> +		val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
>>> +
>>> +		for (i = 0; i < c->num_data; i++)
>>> +			val |= BIT(c->data[i].pos * 2);
>>> +		break;
>>> +	default:
>>> +		dev_err(dev, "Unsupported bus type %d\n", c->phy_cfg);
>>> +		return -EINVAL;
>>
>> If this is the only reason why you're changing the return type to int, I'd
>> suggest using WARN_ON() in this case. I presume it'd take a driver bug for
>> this to trigger?
> 
> Thank you for the review.
> 
> If others agree, I would also prefer WARN_ON instead of changing return values.
> 
> David
> 
> [...]
> 

In the standalone PHY we validate the PHY type in probe :)

WARN_ON() seems fine to me though.


Reviewed-by: Bryan O'Donoghue <[email protected]>

---
bod