Re: [PATCH v12] media: Add t4ka3 camera sensor driver
From: Kate Hsuan
Date: Tue Mar 24 2026 - 22:53:49 EST
Hi Sakari,
Thank you for reviewing it.
On Tue, Mar 24, 2026 at 5:33 PM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Kate,
>
> Thanks for the update.
>
> On Mon, Mar 23, 2026 at 03:16:47PM +0800, Kate Hsuan wrote:
>
> ...
>
> > +static int t4ka3_set_pad_format(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_format *format)
> > +{
> > + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> > + struct v4l2_mbus_framefmt *try_fmt;
> > + struct v4l2_mbus_framefmt *fmt = &format->format;
> > + struct v4l2_rect *crop =
> > + v4l2_subdev_state_get_crop(sd_state, format->pad);
> > + unsigned int width, height;
> > + int min, max, def, ret = 0;
> > +
> > + /* Limit set_fmt max size to crop width / height */
> > + width = clamp_val(ALIGN(format->format.width, 2),
> > + T4KA3_MIN_CROP_WIDTH, crop->width);
> > + height = clamp_val(ALIGN(format->format.height, 2),
> > + T4KA3_MIN_CROP_HEIGHT, crop->height);
> > + t4ka3_fill_format(sensor, &format->format, width, height);
> > +
> > + if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>
> You can move this check after the format assignment below and just return
> 0.
The code below
"""
*v4l2_subdev_state_get_format(sd_state, 0) = format->format;
if (format->which == V4L2_SUBDEV_FORMAT_TRY)
return 0;
"""
already assigned the format and checked the try state, so I'll remove
this check.
>
> > + try_fmt = v4l2_subdev_state_get_format(sd_state, 0);
> > + *try_fmt = format->format;
> > + return 0;
> > + }
> > +
> > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE && sensor->streaming)
> > + return -EBUSY;
> > +
> > + *v4l2_subdev_state_get_format(sd_state, 0) = format->format;
> > +
> > + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> > + return 0;
> > +
> > + t4ka3_calc_mode(sensor, fmt, crop);
> > +
> > + /* vblank range is height dependent adjust and reset to default */
> > + t4ka3_get_vblank_limits(sensor, sd_state, &min, &max, &def);
> > + ret = __v4l2_ctrl_modify_range(sensor->ctrls.vblank, min, max, 1, def);
> > + if (ret)
> > + return ret;
> > +
> > + ret = __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, def);
> > + if (ret)
> > + return ret;
> > +
> > + def = T4KA3_PIXELS_PER_LINE - fmt->width;
> > + ret = __v4l2_ctrl_modify_range(sensor->ctrls.hblank, def, def, 1, def);
> > + if (ret)
> > + return ret;
> > +
> > + return __v4l2_ctrl_s_ctrl(sensor->ctrls.hblank, def);
> > +}
>
> ...
>
> > +static int t4ka3_set_mode(struct t4ka3_data *sensor,
> > + struct v4l2_subdev_state *state)
> > +{
> > + struct v4l2_mbus_framefmt *fmt = v4l2_subdev_state_get_format(state, 0);
> > + int ret = 0;
> > +
> > + cci_write(sensor->regmap, T4KA3_REG_HORZ_OUTPUT_SIZE, fmt->width, &ret);
> > + /* Write mode-height - 2 otherwise things don't work, hw-bug ? */
> > + cci_write(sensor->regmap, T4KA3_REG_VERT_OUTPUT_SIZE,
> > + fmt->height - 2, &ret);
> > + /*
> > + * Note overwritten by __v4l2_ctrl_handler_setup() based on
> > + * vblank ctrl
> > + */
> > + cci_write(sensor->regmap, T4KA3_REG_FRAME_LENGTH_LINES,
> > + T4KA3_LINES_PER_FRAME_30FPS, &ret);
> > + cci_write(sensor->regmap, T4KA3_REG_PIXELS_PER_LINE,
> > + T4KA3_PIXELS_PER_LINE, &ret);
>
> These two appear to be redundant as they're written though
> __v4l2_ctrl_handler_setup() below.
Okay. I'll check this again.
>
> > + /* Always use the full sensor, using window to crop */
> > + cci_write(sensor->regmap, T4KA3_REG_HORZ_START, 0, &ret);
> > + cci_write(sensor->regmap, T4KA3_REG_VERT_START, 0, &ret);
> > + cci_write(sensor->regmap, T4KA3_REG_HORZ_END,
> > + T4KA3_NATIVE_WIDTH - 1, &ret);
> > + cci_write(sensor->regmap, T4KA3_REG_VERT_END,
> > + T4KA3_NATIVE_HEIGHT - 1, &ret);
> > + /* Set window */
> > + cci_write(sensor->regmap, T4KA3_REG_WIN_START_X,
> > + sensor->mode.win_x, &ret);
> > + cci_write(sensor->regmap, T4KA3_REG_WIN_START_Y,
> > + sensor->mode.win_y, &ret);
> > + cci_write(sensor->regmap, T4KA3_REG_WIN_WIDTH, fmt->width, &ret);
> > + cci_write(sensor->regmap, T4KA3_REG_WIN_HEIGHT, fmt->height, &ret);
> > + /* Write 1 to unknown register 0x0900 */
> > + cci_write(sensor->regmap, T4KA3_REG_0900, 1, &ret);
> > + cci_write(sensor->regmap, T4KA3_REG_BINNING,
> > + T4KA3_BINNING_VAL(sensor->mode.binning), &ret);
> > +
> > + return ret;
> > +}
>
> ...
>
> > +static int t4ka3_check_hwcfg(struct t4ka3_data *sensor)
> > +{
> > + struct fwnode_handle *fwnode = dev_fwnode(sensor->dev);
> > + struct v4l2_fwnode_endpoint bus_cfg = {
> > + .bus_type = V4L2_MBUS_CSI2_DPHY,
> > + };
> > + struct fwnode_handle *endpoint;
> > + unsigned long link_freq_bitmap;
> > + int ret;
> > +
> > + endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > +
> > + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
> > + fwnode_handle_put(endpoint);
> > + if (ret)
> > + return ret;
> > +
> > + ret = v4l2_link_freq_to_bitmap(sensor->dev, bus_cfg.link_frequencies,
> > + bus_cfg.nr_of_link_frequencies,
> > + link_freq_menu_items,
> > + ARRAY_SIZE(link_freq_menu_items),
> > + &link_freq_bitmap);
> > +
> > + if (ret == -ENOENT)
> > + goto out_free_bus_cfg;
> > +
> > + if (ret == -ENODATA)
> > + goto out_free_bus_cfg;
>
> Please check for any non-zero return value instead.
will replace it with
"""
if (ret < 0)
goto out_free_bus_cfg;
"""
>
> > +
> > + sensor->link_freq_index = ffs(link_freq_bitmap) - 1;
> > +
> > + /* 4 MIPI lanes */
> > + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> > + ret = dev_err_probe(sensor->dev, -EINVAL,
> > + "number of CSI2 data lanes %u is not supported\n",
> > + bus_cfg.bus.mipi_csi2.num_data_lanes);
> > + goto out_free_bus_cfg;
> > + }
> > +
> > + sensor->mipi_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
> > +
> > +out_free_bus_cfg:
> > + v4l2_fwnode_endpoint_free(&bus_cfg);
> > +
> > + return ret;
> > +}
>
> I'll take this one now but please submit a patch on top to address the
> above.
According to the emails, I'll propose a v13 patch to include the
MAINTAINER change and the fixes :)
>
> --
> Kind regards,
>
> Sakari Ailus
>
--
BR,
Kate