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

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

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@linux.intel.com> 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.

...

> > > +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?

> >
> > > +             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.

> >
> > > +
> > > +     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?

-- 
Kind regards,

Sakari Ailus