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

[Hao Yao <hao.yao@xxxxxxxxx>]

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