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

[Kate Hsuan]

Hi Sakari and Hans,

On Wed, Mar 18, 2026 at 1:54 AM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Kate,
>
> On Tue, Mar 17, 2026 at 09:24:20PM +0800, Kate Hsuan wrote:
> > Hi Sakari,
> >
> > Thank you for reviewing it.
> >
> > And thank you, Hans
> >
> > On Tue, Mar 17, 2026 at 6:00 AM Sakari Ailus
> > <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> > >
> > > Hi Kate,
> > >
> > > Thanks for the patch.
> > >
> > > Where have you seen this sensor being used, if I may ask?
> >
> > It can be found on the Xiaomi Pad2 that is based on an Intel Cherry
> > Trail platform.
>
> Ack, thanks for the info!
>
> > > > +static struct v4l2_mbus_framefmt *t4ka3_get_active_format(struct t4ka3_data *sensor)
> > > > +{
> > > > +     struct v4l2_subdev_state *active_state =
> > > > +             v4l2_subdev_get_locked_active_state(&sensor->sd);
> > > > +
> > > > +     return v4l2_subdev_state_get_format(active_state, 0);
> > > > +}
> > > > +
> > > > +static struct v4l2_rect *t4ka3_get_active_crop(struct t4ka3_data *sensor)
> > > > +{
> > > > +     struct v4l2_subdev_state *active_state =
> > > > +             v4l2_subdev_get_locked_active_state(&sensor->sd);
> > > > +
> > > > +     return v4l2_subdev_state_get_crop(active_state, 0);
> > >
> > > Please avoid adding such helpers.
> > As Hans mentioned, we can put active format and crop in the t4ka3_data
> > or keep the helpers.
>
> What prevents you doing:
>
>         struct v4l2_subdev_state *active_state =
>                 v4l2_subdev_get_locked_active_state(&sensor->sd);
>         struct v4l2_rect *r = v4l2_subdev_state_get_crop(active_state, 0);
>
> in the code? In a lot of the cases you could simply pass the state to the
> function using it as the caller already has it.
>
> It also seems the driver operates in active state whereas any state should
> be valid. In general, file handle specific try state should resemble the
> active state as much as possible with the exception that it won't be
> applied to hardware.
>
> In the long run I'd also expect the control values to move to state.
>

If the caller has the state,  the state will be used and passed to the
function without calling the code mentioned above, such as
(v4l2_subdev_get_locked_active_state()...).
v4l2_subdev_state_get_format() and v4l2_subdev_state_get_crop will be
used in this case.

If the caller doesn't have it, the code mentioned above will be called
to get fmt and crop.

> ...
>
> > > > +static int t4ka3_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > +{
> > > > +     struct t4ka3_data *sensor = ctrl_to_t4ka3(ctrl);
> > > > +     struct v4l2_mbus_framefmt *fmt;
> > > > +     int ret;
> > > > +
> > > > +     /* Update exposure range on vblank changes */
> > > > +     if (ctrl->id == V4L2_CID_VBLANK) {
> > > > +             ret = t4ka3_update_exposure_range(sensor);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > > > +
> > > > +     fmt = t4ka3_get_active_format(sensor);
> > >
> > > You could assign this in declaration.
> > Okay
> > >
> > > > +
> > > > +     /* Only apply changes to the controls if the device is powered up */
> > > > +     if (!pm_runtime_get_if_in_use(sensor->sd.dev)) {
> > > > +             t4ka3_set_bayer_order(sensor, fmt);
> > >
> > > Does this call belong here?
> > I think it can be. It is a simple update of t4ka3_hv_flip_bayer_order.
>
> Why are you doing it here? It basically assigns the Bayer order to the
> active format based on the flipping controls. Why not to do that when
> changing the flipping controls?

What I understand is
Move the t4ka3_set_bayer_order() into t4ka3_t_vflip() and change the
value based on the change of vflip and hflip value.
in t4ka3_t_vflip(), t4ka3_set_bayer_order is called in the function so
setting up bayer order here can be dropped.

A helper is needed and will invoke v4l2_subdev_get_fmt() to find the format.
>
> > >
> > > > +             return 0;
> > > > +     }
> > > > +
> > > > +     switch (ctrl->id) {
> > > > +     case V4L2_CID_TEST_PATTERN:
> > > > +             ret = t4ka3_test_pattern(sensor, ctrl->val);
> > > > +             break;
> > > > +     case V4L2_CID_VFLIP:
> > > > +             ret = t4ka3_t_vflip(&sensor->sd, ctrl->val, T4KA3_VFLIP_BIT);
> > > > +             break;
> > > > +     case V4L2_CID_HFLIP:
> > > > +             ret = t4ka3_t_vflip(&sensor->sd, ctrl->val, T4KA3_HFLIP_BIT);
> > > > +             break;
> > > > +     case V4L2_CID_VBLANK:
> > > > +             ret = cci_write(sensor->regmap, T4KA3_REG_FRAME_LENGTH_LINES,
> > > > +                             fmt->height + ctrl->val, NULL);
> > > > +             break;
> > > > +     case V4L2_CID_EXPOSURE:
> > > > +             ret = cci_write(sensor->regmap, T4KA3_REG_COARSE_INTEGRATION_TIME,
> > > > +                             ctrl->val, NULL);
> > > > +             break;
> > > > +     case V4L2_CID_ANALOGUE_GAIN:
> > > > +             ret = cci_write(sensor->regmap, T4KA3_REG_GLOBAL_GAIN,
> > > > +                             ctrl->val, NULL);
> > > > +             break;
> > > > +     default:
> > > > +             ret = -EINVAL;
> > > > +             break;
> > > > +     }
> > > > +
> > > > +     pm_runtime_put(sensor->sd.dev);
> > >
> > > Newline here?
> > Okay
> > >
> > > > +     return ret;
> > > > +}
>
> ...
>
> > > > +static int t4ka3_disable_stream(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
> > > > +                             u32 pad, u64 streams_mask)
> > > > +{
> > > > +     struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> > > > +     int ret;
> > > > +
> > > > +     ret = cci_write(sensor->regmap, T4KA3_REG_STREAM, 0, NULL);
> > > > +     pm_runtime_put(sensor->sd.dev);
> > > > +     sensor->streaming = 0;
> > > > +     return ret;
> > >
> > > Return 0 here but complain about it.
> > Do you mean return 0 here and print a message when ret != 0?
>
> Yes, please.
>
> ...
>
> > > > +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;
> > > > +
> > > > +     /*
> > > > +      * Sometimes the fwnode graph is initialized by the bridge driver.
> > > > +      * Bridge drivers doing this may also add GPIO mappings, wait for this.
> > > > +      */
> > >
> > > No need for such a comment.
> > I'll drop it.
> >
> > >
> > > > +     endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > > > +     if (!endpoint)
> > > > +             return dev_err_probe(sensor->dev, -EPROBE_DEFER,
> > > > +                                  "waiting for fwnode graph endpoint\n");
> > >
> > > This
> > > <URL:https://git.linuxtv.org/sailus/media_tree.git/commit/?h=cleanup&id=8181d18d45d593d8499cbf0e83de08c6d913516c>
> > > will be merged soon.
> > Does it mean "return  -EPROBE_DEFER;" is enough?
>
> You can omit checking for errors here.

OKay
>
> > >
> > > > +
> > > > +     ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
> > > > +     fwnode_handle_put(endpoint);
> > > > +     if (ret)
> > > > +             return ret;
>
> ...
>
> > > > +static int t4ka3_pm_resume(struct device *dev)
> > > > +{
> > > > +     struct t4ka3_data *sensor = dev_get_drvdata(dev);
> > > > +     u16 sensor_id;
> > > > +     int ret;
> > > > +
> > > > +     usleep_range(5000, 6000);
> > > > +
> > > > +     gpiod_set_value_cansleep(sensor->powerdown_gpio, 0);
> > > > +     gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> > > > +
> > > > +     /* waiting for the sensor after powering up */
> > > > +     msleep(20);
> > >
> > > fsleep() maybe?
> > I can change it.
> > >
> > > > +
> > > > +     ret = t4ka3_detect(sensor, &sensor_id);
> > > > +     if (ret) {
> > > > +             dev_err(sensor->dev, "sensor detect failed\n");
> > > > +             return ret;
> > >
> > > What about gpio values in this case?
> > both powerdown_gpio and reset_gpio are 0 when resuming and 1 when suspended.
> > t4ka3_detect() reads the sensor name through i2c. If it finds the
> > product ID then return 0;
>
> What if t4ka3_detect() returns an error? What happens then?

That means the sensor replies with an unexpected value and returns
-ENODEV to notify that "no such device" and then power it off.
I need I have to power it off when t4ka3_detect() returns an error. I'll fix it.
Or I can move it into probe().
>
> --
> Kind regards,
>
> Sakari Ailus
>


-- 
BR,
Kate