| Message ID | 20250307093130.1103961-2-hao.yao@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] media: i2c: ov13b10: Fix h_blank calculation | expand |
Hi Bingbu, Hao, Thanks for the patchset. On Fri, Mar 07, 2025 at 05:31:17PM +0800, Hao Yao wrote: > 1. Fix pixel rate calculation to consider different lane number > 2. Add 2104x1560 60fps 2 data lanes register setting > 3. Support 2 lane in check_hwcfg > 4. Select correct mode considering lane number used > > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > Signed-off-by: Hao Yao <hao.yao@intel.com> > --- > drivers/media/i2c/ov13b10.c | 134 ++++++++++++++++++++++++++++++------ > 1 file changed, 112 insertions(+), 22 deletions(-) > > diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c > index 2e83fc23f321..20481b8d4e79 100644 > --- a/drivers/media/i2c/ov13b10.c > +++ b/drivers/media/i2c/ov13b10.c > @@ -514,6 +514,52 @@ static const struct ov13b10_reg mode_1364x768_120fps_regs[] = { > {0x5001, 0x0d}, > }; > > +static const struct ov13b10_reg mode_2lanes_2104x1560_60fps_regs[] = { > + {0x3016, 0x32}, > + {0x3106, 0x29}, > + {0x0305, 0xaf}, > + {0x3501, 0x06}, > + {0x3662, 0x88}, > + {0x3714, 0x28}, > + {0x3739, 0x10}, > + {0x37c2, 0x14}, > + {0x37d9, 0x06}, > + {0x37e2, 0x0c}, > + {0x3800, 0x00}, > + {0x3801, 0x00}, > + {0x3802, 0x00}, > + {0x3803, 0x08}, > + {0x3804, 0x10}, > + {0x3805, 0x8f}, > + {0x3806, 0x0c}, > + {0x3807, 0x47}, > + {0x3808, 0x08}, > + {0x3809, 0x38}, > + {0x380a, 0x06}, > + {0x380b, 0x18}, > + {0x380c, 0x04}, > + {0x380d, 0x98}, > + {0x380e, 0x06}, > + {0x380f, 0x3e}, > + {0x3810, 0x00}, > + {0x3811, 0x07}, > + {0x3812, 0x00}, > + {0x3813, 0x05}, > + {0x3814, 0x03}, > + {0x3816, 0x03}, > + {0x3820, 0x8b}, > + {0x3c8c, 0x18}, > + {0x4008, 0x00}, > + {0x4009, 0x05}, > + {0x4050, 0x00}, > + {0x4051, 0x05}, > + {0x4501, 0x08}, > + {0x4505, 0x00}, > + {0x4837, 0x0e}, > + {0x5000, 0xfd}, > + {0x5001, 0x0d}, > +}; > + > static const char * const ov13b10_test_pattern_menu[] = { > "Disabled", > "Vertical Color Bar Type 1", > @@ -527,15 +573,16 @@ static const char * const ov13b10_test_pattern_menu[] = { > #define OV13B10_LINK_FREQ_INDEX_0 0 > > #define OV13B10_EXT_CLK 19200000 > -#define OV13B10_DATA_LANES 4 > +#define OV13B10_4_DATA_LANES 4 > +#define OV13B10_2_DATA_LANES 2 > > /* > - * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample > - * data rate => double data rate; number of lanes => 4; bits per pixel => 10 > + * pixel_rate = data_rate * nr_of_lanes / bits_per_pixel > + * data_rate => link_freq * 2; number of lanes => 4; bits per pixel => 10 > */ > -static u64 link_freq_to_pixel_rate(u64 f) > +static u64 link_freq_to_pixel_rate(u64 f, u8 lanes) > { > - f *= 2 * OV13B10_DATA_LANES; > + f *= 2 * lanes; > do_div(f, 10); > > return f; > @@ -559,7 +606,8 @@ static const struct ov13b10_link_freq_config > }; > > /* Mode configs */ > -static const struct ov13b10_mode supported_modes[] = { > +static const struct ov13b10_mode supported_4_lanes_modes[] = { > + /* 4 data lanes */ > { > .width = 4208, > .height = 3120, > @@ -634,6 +682,23 @@ static const struct ov13b10_mode supported_modes[] = { > }, > }; > > +static const struct ov13b10_mode supported_2_lanes_modes[] = { > + /* 2 data lanes */ > + { > + .width = 2104, > + .height = 1560, > + .vts_def = OV13B10_VTS_60FPS, > + .vts_min = OV13B10_VTS_60FPS, > + .link_freq_index = OV13B10_LINK_FREQ_INDEX_0, > + .ppl = 2352, > + .reg_list = { > + .num_of_regs = > + ARRAY_SIZE(mode_2lanes_2104x1560_60fps_regs), > + .regs = mode_2lanes_2104x1560_60fps_regs, > + }, > + }, > +}; > + > struct ov13b10 { > struct v4l2_subdev sd; > struct media_pad pad; > @@ -651,12 +716,20 @@ struct ov13b10 { > struct v4l2_ctrl *hblank; > struct v4l2_ctrl *exposure; > > + /* Supported modes */ > + const struct ov13b10_mode *supported_modes; > + > /* Current mode */ > const struct ov13b10_mode *cur_mode; > > /* Mutex for serialized access */ > struct mutex mutex; > > + u8 supported_modes_num; > + > + /* Data lanes used */ > + u8 data_lanes; > + > /* True if the device has been identified */ > bool identified; > }; > @@ -760,8 +833,8 @@ static int ov13b10_write_reg_list(struct ov13b10 *ov13b, > /* Open sub-device */ > static int ov13b10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > { > - const struct ov13b10_mode *default_mode = &supported_modes[0]; > struct ov13b10 *ov13b = to_ov13b10(sd); > + const struct ov13b10_mode *default_mode = ov13b->supported_modes; > struct v4l2_mbus_framefmt *try_fmt = v4l2_subdev_state_get_format(fh->state, > 0); > > @@ -980,7 +1053,10 @@ static int ov13b10_enum_frame_size(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_frame_size_enum *fse) > { > - if (fse->index >= ARRAY_SIZE(supported_modes)) > + struct ov13b10 *ov13b = to_ov13b10(sd); > + const struct ov13b10_mode *supported_modes = ov13b->supported_modes; > + > + if (fse->index >= ov13b->supported_modes_num) > return -EINVAL; > > if (fse->code != MEDIA_BUS_FMT_SGRBG10_1X10) > @@ -1040,6 +1116,7 @@ ov13b10_set_pad_format(struct v4l2_subdev *sd, > { > struct ov13b10 *ov13b = to_ov13b10(sd); > const struct ov13b10_mode *mode; > + const struct ov13b10_mode *supported_modes = ov13b->supported_modes; > struct v4l2_mbus_framefmt *framefmt; > s32 vblank_def; > s32 vblank_min; > @@ -1054,7 +1131,7 @@ ov13b10_set_pad_format(struct v4l2_subdev *sd, > fmt->format.code = MEDIA_BUS_FMT_SGRBG10_1X10; > > mode = v4l2_find_nearest_size(supported_modes, > - ARRAY_SIZE(supported_modes), > + ov13b->supported_modes_num, > width, height, > fmt->format.width, fmt->format.height); > ov13b10_update_pad_format(mode, fmt); > @@ -1065,7 +1142,8 @@ ov13b10_set_pad_format(struct v4l2_subdev *sd, > ov13b->cur_mode = mode; > __v4l2_ctrl_s_ctrl(ov13b->link_freq, mode->link_freq_index); > link_freq = link_freq_menu_items[mode->link_freq_index]; > - pixel_rate = link_freq_to_pixel_rate(link_freq); > + pixel_rate = link_freq_to_pixel_rate(link_freq, > + ov13b->data_lanes); > __v4l2_ctrl_s_ctrl_int64(ov13b->pixel_rate, pixel_rate); > > /* Update limits and set FPS to default */ > @@ -1312,7 +1390,8 @@ static int ov13b10_init_controls(struct ov13b10 *ov13b) > if (ov13b->link_freq) > ov13b->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > - pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]); > + pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0], > + ov13b->data_lanes); > pixel_rate_min = 0; > /* By default, PIXEL_RATE is read only */ > ov13b->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov13b10_ctrl_ops, > @@ -1423,7 +1502,7 @@ static int ov13b10_get_pm_resources(struct device *dev) > return 0; > } > > -static int ov13b10_check_hwcfg(struct device *dev) > +static int ov13b10_check_hwcfg(struct device *dev, struct ov13b10 *ov13b) > { > struct v4l2_fwnode_endpoint bus_cfg = { > .bus_type = V4L2_MBUS_CSI2_DPHY > @@ -1433,6 +1512,7 @@ static int ov13b10_check_hwcfg(struct device *dev) > unsigned int i, j; > int ret; > u32 ext_clk; > + u8 dlane; > > if (!fwnode) > return -ENXIO; > @@ -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? > + } > + > + ov13b->cur_mode = ov13b->supported_modes; > + dev_dbg(dev, "%u lanes with %u modes selected\n", > + ov13b->data_lanes, ov13b->supported_modes_num); > > if (!bus_cfg.nr_of_link_frequencies) { > dev_err(dev, "no link frequencies defined"); > @@ -1499,17 +1592,17 @@ static int ov13b10_probe(struct i2c_client *client) > bool full_power; > int ret; > > + ov13b = devm_kzalloc(&client->dev, sizeof(*ov13b), GFP_KERNEL); > + if (!ov13b) > + return -ENOMEM; > + > /* Check HW config */ > - ret = ov13b10_check_hwcfg(&client->dev); > + ret = ov13b10_check_hwcfg(&client->dev, ov13b); > if (ret) { > dev_err(&client->dev, "failed to check hwcfg: %d", ret); > return ret; > } > > - ov13b = devm_kzalloc(&client->dev, sizeof(*ov13b), GFP_KERNEL); > - if (!ov13b) > - return -ENOMEM; > - > /* Initialize subdev */ > v4l2_i2c_subdev_init(&ov13b->sd, client, &ov13b10_subdev_ops); > > @@ -1533,9 +1626,6 @@ static int ov13b10_probe(struct i2c_client *client) > } > } > > - /* Set default mode to max resolution */ > - ov13b->cur_mode = &supported_modes[0]; > - > ret = ov13b10_init_controls(ov13b); > if (ret) > goto error_power_off;
Hi Sakari, Thanks for reviewing this. On 2025/3/7 17:43, Sakari Ailus wrote: > Hi Bingbu, Hao, > > Thanks for the patchset. > > On Fri, Mar 07, 2025 at 05:31:17PM +0800, Hao Yao wrote: >> 1. Fix pixel rate calculation to consider different lane number >> 2. Add 2104x1560 60fps 2 data lanes register setting >> 3. Support 2 lane in check_hwcfg >> 4. Select correct mode considering lane number used >> >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >> Signed-off-by: Hao Yao <hao.yao@intel.com> >> --- >> drivers/media/i2c/ov13b10.c | 134 ++++++++++++++++++++++++++++++------ >> 1 file changed, 112 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c >> index 2e83fc23f321..20481b8d4e79 100644 >> --- a/drivers/media/i2c/ov13b10.c >> +++ b/drivers/media/i2c/ov13b10.c >> @@ -514,6 +514,52 @@ static const struct ov13b10_reg mode_1364x768_120fps_regs[] = { >> {0x5001, 0x0d}, >> }; >> >> +static const struct ov13b10_reg mode_2lanes_2104x1560_60fps_regs[] = { >> + {0x3016, 0x32}, >> + {0x3106, 0x29}, >> + {0x0305, 0xaf}, >> + {0x3501, 0x06}, >> + {0x3662, 0x88}, >> + {0x3714, 0x28}, >> + {0x3739, 0x10}, >> + {0x37c2, 0x14}, >> + {0x37d9, 0x06}, >> + {0x37e2, 0x0c}, >> + {0x3800, 0x00}, >> + {0x3801, 0x00}, >> + {0x3802, 0x00}, >> + {0x3803, 0x08}, >> + {0x3804, 0x10}, >> + {0x3805, 0x8f}, >> + {0x3806, 0x0c}, >> + {0x3807, 0x47}, >> + {0x3808, 0x08}, >> + {0x3809, 0x38}, >> + {0x380a, 0x06}, >> + {0x380b, 0x18}, >> + {0x380c, 0x04}, >> + {0x380d, 0x98}, >> + {0x380e, 0x06}, >> + {0x380f, 0x3e}, >> + {0x3810, 0x00}, >> + {0x3811, 0x07}, >> + {0x3812, 0x00}, >> + {0x3813, 0x05}, >> + {0x3814, 0x03}, >> + {0x3816, 0x03}, >> + {0x3820, 0x8b}, >> + {0x3c8c, 0x18}, >> + {0x4008, 0x00}, >> + {0x4009, 0x05}, >> + {0x4050, 0x00}, >> + {0x4051, 0x05}, >> + {0x4501, 0x08}, >> + {0x4505, 0x00}, >> + {0x4837, 0x0e}, >> + {0x5000, 0xfd}, >> + {0x5001, 0x0d}, >> +}; >> + >> static const char * const ov13b10_test_pattern_menu[] = { >> "Disabled", >> "Vertical Color Bar Type 1", >> @@ -527,15 +573,16 @@ static const char * const ov13b10_test_pattern_menu[] = { >> #define OV13B10_LINK_FREQ_INDEX_0 0 >> >> #define OV13B10_EXT_CLK 19200000 >> -#define OV13B10_DATA_LANES 4 >> +#define OV13B10_4_DATA_LANES 4 >> +#define OV13B10_2_DATA_LANES 2 >> >> /* >> - * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample >> - * data rate => double data rate; number of lanes => 4; bits per pixel => 10 >> + * pixel_rate = data_rate * nr_of_lanes / bits_per_pixel >> + * data_rate => link_freq * 2; number of lanes => 4; bits per pixel => 10 >> */ >> -static u64 link_freq_to_pixel_rate(u64 f) >> +static u64 link_freq_to_pixel_rate(u64 f, u8 lanes) >> { >> - f *= 2 * OV13B10_DATA_LANES; >> + f *= 2 * lanes; >> do_div(f, 10); >> >> return f; >> @@ -559,7 +606,8 @@ static const struct ov13b10_link_freq_config >> }; >> >> /* Mode configs */ >> -static const struct ov13b10_mode supported_modes[] = { >> +static const struct ov13b10_mode supported_4_lanes_modes[] = { >> + /* 4 data lanes */ >> { >> .width = 4208, >> .height = 3120, >> @@ -634,6 +682,23 @@ static const struct ov13b10_mode supported_modes[] = { >> }, >> }; >> >> +static const struct ov13b10_mode supported_2_lanes_modes[] = { >> + /* 2 data lanes */ >> + { >> + .width = 2104, >> + .height = 1560, >> + .vts_def = OV13B10_VTS_60FPS, >> + .vts_min = OV13B10_VTS_60FPS, >> + .link_freq_index = OV13B10_LINK_FREQ_INDEX_0, >> + .ppl = 2352, >> + .reg_list = { >> + .num_of_regs = >> + ARRAY_SIZE(mode_2lanes_2104x1560_60fps_regs), >> + .regs = mode_2lanes_2104x1560_60fps_regs, >> + }, >> + }, >> +}; >> + >> struct ov13b10 { >> struct v4l2_subdev sd; >> struct media_pad pad; >> @@ -651,12 +716,20 @@ struct ov13b10 { >> struct v4l2_ctrl *hblank; >> struct v4l2_ctrl *exposure; >> >> + /* Supported modes */ >> + const struct ov13b10_mode *supported_modes; >> + >> /* Current mode */ >> const struct ov13b10_mode *cur_mode; >> >> /* Mutex for serialized access */ >> struct mutex mutex; >> >> + u8 supported_modes_num; >> + >> + /* Data lanes used */ >> + u8 data_lanes; >> + >> /* True if the device has been identified */ >> bool identified; >> }; >> @@ -760,8 +833,8 @@ static int ov13b10_write_reg_list(struct ov13b10 *ov13b, >> /* Open sub-device */ >> static int ov13b10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) >> { >> - const struct ov13b10_mode *default_mode = &supported_modes[0]; >> struct ov13b10 *ov13b = to_ov13b10(sd); >> + const struct ov13b10_mode *default_mode = ov13b->supported_modes; >> struct v4l2_mbus_framefmt *try_fmt = v4l2_subdev_state_get_format(fh->state, >> 0); >> >> @@ -980,7 +1053,10 @@ static int ov13b10_enum_frame_size(struct v4l2_subdev *sd, >> struct v4l2_subdev_state *sd_state, >> struct v4l2_subdev_frame_size_enum *fse) >> { >> - if (fse->index >= ARRAY_SIZE(supported_modes)) >> + struct ov13b10 *ov13b = to_ov13b10(sd); >> + const struct ov13b10_mode *supported_modes = ov13b->supported_modes; >> + >> + if (fse->index >= ov13b->supported_modes_num) >> return -EINVAL; >> >> if (fse->code != MEDIA_BUS_FMT_SGRBG10_1X10) >> @@ -1040,6 +1116,7 @@ ov13b10_set_pad_format(struct v4l2_subdev *sd, >> { >> struct ov13b10 *ov13b = to_ov13b10(sd); >> const struct ov13b10_mode *mode; >> + const struct ov13b10_mode *supported_modes = ov13b->supported_modes; >> struct v4l2_mbus_framefmt *framefmt; >> s32 vblank_def; >> s32 vblank_min; >> @@ -1054,7 +1131,7 @@ ov13b10_set_pad_format(struct v4l2_subdev *sd, >> fmt->format.code = MEDIA_BUS_FMT_SGRBG10_1X10; >> >> mode = v4l2_find_nearest_size(supported_modes, >> - ARRAY_SIZE(supported_modes), >> + ov13b->supported_modes_num, >> width, height, >> fmt->format.width, fmt->format.height); >> ov13b10_update_pad_format(mode, fmt); >> @@ -1065,7 +1142,8 @@ ov13b10_set_pad_format(struct v4l2_subdev *sd, >> ov13b->cur_mode = mode; >> __v4l2_ctrl_s_ctrl(ov13b->link_freq, mode->link_freq_index); >> link_freq = link_freq_menu_items[mode->link_freq_index]; >> - pixel_rate = link_freq_to_pixel_rate(link_freq); >> + pixel_rate = link_freq_to_pixel_rate(link_freq, >> + ov13b->data_lanes); >> __v4l2_ctrl_s_ctrl_int64(ov13b->pixel_rate, pixel_rate); >> >> /* Update limits and set FPS to default */ >> @@ -1312,7 +1390,8 @@ static int ov13b10_init_controls(struct ov13b10 *ov13b) >> if (ov13b->link_freq) >> ov13b->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; >> >> - pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]); >> + pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0], >> + ov13b->data_lanes); >> pixel_rate_min = 0; >> /* By default, PIXEL_RATE is read only */ >> ov13b->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov13b10_ctrl_ops, >> @@ -1423,7 +1502,7 @@ static int ov13b10_get_pm_resources(struct device *dev) >> return 0; >> } >> >> -static int ov13b10_check_hwcfg(struct device *dev) >> +static int ov13b10_check_hwcfg(struct device *dev, struct ov13b10 *ov13b) >> { >> struct v4l2_fwnode_endpoint bus_cfg = { >> .bus_type = V4L2_MBUS_CSI2_DPHY >> @@ -1433,6 +1512,7 @@ static int ov13b10_check_hwcfg(struct device *dev) >> unsigned int i, j; >> int ret; >> u32 ext_clk; >> + u8 dlane; >> >> if (!fwnode) >> return -ENXIO; >> @@ -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; } Best Regards, Hao Yao > >> + } >> + >> + ov13b->cur_mode = ov13b->supported_modes; >> + dev_dbg(dev, "%u lanes with %u modes selected\n", >> + ov13b->data_lanes, ov13b->supported_modes_num); >> >> if (!bus_cfg.nr_of_link_frequencies) { >> dev_err(dev, "no link frequencies defined"); >> @@ -1499,17 +1592,17 @@ static int ov13b10_probe(struct i2c_client *client) >> bool full_power; >> int ret; >> >> + ov13b = devm_kzalloc(&client->dev, sizeof(*ov13b), GFP_KERNEL); >> + if (!ov13b) >> + return -ENOMEM; >> + >> /* Check HW config */ >> - ret = ov13b10_check_hwcfg(&client->dev); >> + ret = ov13b10_check_hwcfg(&client->dev, ov13b); >> if (ret) { >> dev_err(&client->dev, "failed to check hwcfg: %d", ret); >> return ret; >> } >> >> - ov13b = devm_kzalloc(&client->dev, sizeof(*ov13b), GFP_KERNEL); >> - if (!ov13b) >> - return -ENOMEM; >> - >> /* Initialize subdev */ >> v4l2_i2c_subdev_init(&ov13b->sd, client, &ov13b10_subdev_ops); >> >> @@ -1533,9 +1626,6 @@ static int ov13b10_probe(struct i2c_client *client) >> } >> } >> >> - /* Set default mode to max resolution */ >> - ov13b->cur_mode = &supported_modes[0]; >> - >> ret = ov13b10_init_controls(ov13b); >> if (ret) >> goto error_power_off; >
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.
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
Hi Hao, On Tue, Mar 11, 2025 at 02:42:57PM +0800, Hao Yao wrote: > 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 ? That looks good to me.
diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c index 2e83fc23f321..20481b8d4e79 100644 --- a/drivers/media/i2c/ov13b10.c +++ b/drivers/media/i2c/ov13b10.c @@ -514,6 +514,52 @@ static const struct ov13b10_reg mode_1364x768_120fps_regs[] = { {0x5001, 0x0d}, }; +static const struct ov13b10_reg mode_2lanes_2104x1560_60fps_regs[] = { + {0x3016, 0x32}, + {0x3106, 0x29}, + {0x0305, 0xaf}, + {0x3501, 0x06}, + {0x3662, 0x88}, + {0x3714, 0x28}, + {0x3739, 0x10}, + {0x37c2, 0x14}, + {0x37d9, 0x06}, + {0x37e2, 0x0c}, + {0x3800, 0x00}, + {0x3801, 0x00}, + {0x3802, 0x00}, + {0x3803, 0x08}, + {0x3804, 0x10}, + {0x3805, 0x8f}, + {0x3806, 0x0c}, + {0x3807, 0x47}, + {0x3808, 0x08}, + {0x3809, 0x38}, + {0x380a, 0x06}, + {0x380b, 0x18}, + {0x380c, 0x04}, + {0x380d, 0x98}, + {0x380e, 0x06}, + {0x380f, 0x3e}, + {0x3810, 0x00}, + {0x3811, 0x07}, + {0x3812, 0x00}, + {0x3813, 0x05}, + {0x3814, 0x03}, + {0x3816, 0x03}, + {0x3820, 0x8b}, + {0x3c8c, 0x18}, + {0x4008, 0x00}, + {0x4009, 0x05}, + {0x4050, 0x00}, + {0x4051, 0x05}, + {0x4501, 0x08}, + {0x4505, 0x00}, + {0x4837, 0x0e}, + {0x5000, 0xfd}, + {0x5001, 0x0d}, +}; + static const char * const ov13b10_test_pattern_menu[] = { "Disabled", "Vertical Color Bar Type 1", @@ -527,15 +573,16 @@ static const char * const ov13b10_test_pattern_menu[] = { #define OV13B10_LINK_FREQ_INDEX_0 0 #define OV13B10_EXT_CLK 19200000 -#define OV13B10_DATA_LANES 4 +#define OV13B10_4_DATA_LANES 4 +#define OV13B10_2_DATA_LANES 2 /* - * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample - * data rate => double data rate; number of lanes => 4; bits per pixel => 10 + * pixel_rate = data_rate * nr_of_lanes / bits_per_pixel + * data_rate => link_freq * 2; number of lanes => 4; bits per pixel => 10 */ -static u64 link_freq_to_pixel_rate(u64 f) +static u64 link_freq_to_pixel_rate(u64 f, u8 lanes) { - f *= 2 * OV13B10_DATA_LANES; + f *= 2 * lanes; do_div(f, 10); return f; @@ -559,7 +606,8 @@ static const struct ov13b10_link_freq_config }; /* Mode configs */ -static const struct ov13b10_mode supported_modes[] = { +static const struct ov13b10_mode supported_4_lanes_modes[] = { + /* 4 data lanes */ { .width = 4208, .height = 3120, @@ -634,6 +682,23 @@ static const struct ov13b10_mode supported_modes[] = { }, }; +static const struct ov13b10_mode supported_2_lanes_modes[] = { + /* 2 data lanes */ + { + .width = 2104, + .height = 1560, + .vts_def = OV13B10_VTS_60FPS, + .vts_min = OV13B10_VTS_60FPS, + .link_freq_index = OV13B10_LINK_FREQ_INDEX_0, + .ppl = 2352, + .reg_list = { + .num_of_regs = + ARRAY_SIZE(mode_2lanes_2104x1560_60fps_regs), + .regs = mode_2lanes_2104x1560_60fps_regs, + }, + }, +}; + struct ov13b10 { struct v4l2_subdev sd; struct media_pad pad; @@ -651,12 +716,20 @@ struct ov13b10 { struct v4l2_ctrl *hblank; struct v4l2_ctrl *exposure; + /* Supported modes */ + const struct ov13b10_mode *supported_modes; + /* Current mode */ const struct ov13b10_mode *cur_mode; /* Mutex for serialized access */ struct mutex mutex; + u8 supported_modes_num; + + /* Data lanes used */ + u8 data_lanes; + /* True if the device has been identified */ bool identified; }; @@ -760,8 +833,8 @@ static int ov13b10_write_reg_list(struct ov13b10 *ov13b, /* Open sub-device */ static int ov13b10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) { - const struct ov13b10_mode *default_mode = &supported_modes[0]; struct ov13b10 *ov13b = to_ov13b10(sd); + const struct ov13b10_mode *default_mode = ov13b->supported_modes; struct v4l2_mbus_framefmt *try_fmt = v4l2_subdev_state_get_format(fh->state, 0); @@ -980,7 +1053,10 @@ static int ov13b10_enum_frame_size(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state, struct v4l2_subdev_frame_size_enum *fse) { - if (fse->index >= ARRAY_SIZE(supported_modes)) + struct ov13b10 *ov13b = to_ov13b10(sd); + const struct ov13b10_mode *supported_modes = ov13b->supported_modes; + + if (fse->index >= ov13b->supported_modes_num) return -EINVAL; if (fse->code != MEDIA_BUS_FMT_SGRBG10_1X10) @@ -1040,6 +1116,7 @@ ov13b10_set_pad_format(struct v4l2_subdev *sd, { struct ov13b10 *ov13b = to_ov13b10(sd); const struct ov13b10_mode *mode; + const struct ov13b10_mode *supported_modes = ov13b->supported_modes; struct v4l2_mbus_framefmt *framefmt; s32 vblank_def; s32 vblank_min; @@ -1054,7 +1131,7 @@ ov13b10_set_pad_format(struct v4l2_subdev *sd, fmt->format.code = MEDIA_BUS_FMT_SGRBG10_1X10; mode = v4l2_find_nearest_size(supported_modes, - ARRAY_SIZE(supported_modes), + ov13b->supported_modes_num, width, height, fmt->format.width, fmt->format.height); ov13b10_update_pad_format(mode, fmt); @@ -1065,7 +1142,8 @@ ov13b10_set_pad_format(struct v4l2_subdev *sd, ov13b->cur_mode = mode; __v4l2_ctrl_s_ctrl(ov13b->link_freq, mode->link_freq_index); link_freq = link_freq_menu_items[mode->link_freq_index]; - pixel_rate = link_freq_to_pixel_rate(link_freq); + pixel_rate = link_freq_to_pixel_rate(link_freq, + ov13b->data_lanes); __v4l2_ctrl_s_ctrl_int64(ov13b->pixel_rate, pixel_rate); /* Update limits and set FPS to default */ @@ -1312,7 +1390,8 @@ static int ov13b10_init_controls(struct ov13b10 *ov13b) if (ov13b->link_freq) ov13b->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; - pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]); + pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0], + ov13b->data_lanes); pixel_rate_min = 0; /* By default, PIXEL_RATE is read only */ ov13b->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov13b10_ctrl_ops, @@ -1423,7 +1502,7 @@ static int ov13b10_get_pm_resources(struct device *dev) return 0; } -static int ov13b10_check_hwcfg(struct device *dev) +static int ov13b10_check_hwcfg(struct device *dev, struct ov13b10 *ov13b) { struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = V4L2_MBUS_CSI2_DPHY @@ -1433,6 +1512,7 @@ static int ov13b10_check_hwcfg(struct device *dev) unsigned int i, j; int ret; u32 ext_clk; + u8 dlane; if (!fwnode) return -ENXIO; @@ -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); + } + + ov13b->cur_mode = ov13b->supported_modes; + dev_dbg(dev, "%u lanes with %u modes selected\n", + ov13b->data_lanes, ov13b->supported_modes_num); if (!bus_cfg.nr_of_link_frequencies) { dev_err(dev, "no link frequencies defined"); @@ -1499,17 +1592,17 @@ static int ov13b10_probe(struct i2c_client *client) bool full_power; int ret; + ov13b = devm_kzalloc(&client->dev, sizeof(*ov13b), GFP_KERNEL); + if (!ov13b) + return -ENOMEM; + /* Check HW config */ - ret = ov13b10_check_hwcfg(&client->dev); + ret = ov13b10_check_hwcfg(&client->dev, ov13b); if (ret) { dev_err(&client->dev, "failed to check hwcfg: %d", ret); return ret; } - ov13b = devm_kzalloc(&client->dev, sizeof(*ov13b), GFP_KERNEL); - if (!ov13b) - return -ENOMEM; - /* Initialize subdev */ v4l2_i2c_subdev_init(&ov13b->sd, client, &ov13b10_subdev_ops); @@ -1533,9 +1626,6 @@ static int ov13b10_probe(struct i2c_client *client) } } - /* Set default mode to max resolution */ - ov13b->cur_mode = &supported_modes[0]; - ret = ov13b10_init_controls(ov13b); if (ret) goto error_power_off;