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

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

[...]