Re: [PATCH v12] media: Add t4ka3 camera sensor driver

[Kate Hsuan <hpa@redhat.com>]

Hi Sakari,

Thank you for reviewing it.

On Tue, Mar 24, 2026 at 5:33 PM Sakari Ailus
<sakari.ailus@linux.intel.com> 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