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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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.

-- 
Regards,

Sakari Ailus