Re: [PATCH 2/2] media: i2c: ov13b10: Support 2 lane mode

[Hao Yao <hao.yao@intel.com>]

Hi Sakari,

On 2025/3/10 15:14, Sakari Ailus wrote:
> Hi Hao,
> 
> On Mon, Mar 10, 2025 at 12:06:04PM +0800, Hao Yao wrote:
>>>> @@ -1459,12 +1539,25 @@ static int ov13b10_check_hwcfg(struct device *dev)
>>>>    	if (ret)
>>>>    		return ret;
>>>> -	if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV13B10_DATA_LANES) {
>>>> +	dlane = bus_cfg.bus.mipi_csi2.num_data_lanes;
>>>> +	if (dlane != OV13B10_4_DATA_LANES && dlane != OV13B10_2_DATA_LANES) {
>>>>    		dev_err(dev, "number of CSI2 data lanes %d is not supported",
>>>> -			bus_cfg.bus.mipi_csi2.num_data_lanes);
>>>> +			dlane);
>>>>    		ret = -EINVAL;
>>>>    		goto out_err;
>>>>    	}
>>>> +	ov13b->data_lanes = dlane;
>>>> +	ov13b->supported_modes = supported_4_lanes_modes;
>>>> +	ov13b->supported_modes_num = ARRAY_SIZE(supported_4_lanes_modes);
>>>> +	if (dlane == OV13B10_2_DATA_LANES) {
>>>> +		ov13b->supported_modes = supported_2_lanes_modes;
>>>> +		ov13b->supported_modes_num =
>>>> +			ARRAY_SIZE(supported_2_lanes_modes);
>>>
>>> How about using switch() here?
>>
>> How about:
>> 	ov13b->data_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
>> 	switch (ov13b->data_lanes) {
>> 	case OV13B10_4_DATA_LANES:
>> 		ov13b->supported_modes = supported_4_lanes_modes;
>> 		ov13b->supported_modes_num =
>> 			ARRAY_SIZE(supported_4_lanes_modes);
>> 		break;
>>
>> 	case OV13B10_2_DATA_LANES:
>> 		ov13b->supported_modes = supported_2_lanes_modes;
>> 		ov13b->supported_modes_num =
>> 			ARRAY_SIZE(supported_2_lanes_modes);
>> 		break;
>>
>> 	default:
>> 		dev_err(dev, "number of CSI2 data lanes %d is not supported",
>> 			ov13b->data_lanes);
>> 		ret = -EINVAL;
>> 		goto out_err;
>> 	}
> 
> Looks good to me. I'd do the assignment to ov13b->data_lanes after checking
> the value though.
> 

Thank you, I will prepare v2 later.
BTW, do you have any suggestions on patch 1/2 ?

Best Regards,
Hao Yao