Re: [PATCH v4 2/2] media: i2c: imx678: Add driver for Sony IMX678

[Tarang Raval]

Hi Jai.

> Quoting Tarang Raval (2026-06-06 13:47:36)
> > Hi Jai,
> >
> > Sorry, in my first review I missed a few minor issues listed below.
> >
>
> No worries, thank you for the reviews.
>
> > I also noticed one major issue in the driver. Please check the comments below.
> >
> > Other than that, the driver looks perfect.
> >
> > > Add a V4L2 subdev driver for the Sony IMX678 image sensor.
> > >
> > > IMX678 is a diagonal 8.86 mm (Type 1/1.8) CMOS active pixel type
> > > solid-state image sensor with a square pixel array and 8.40 M effective
> > > pixels.
> > >
> > > The following features are supported by this driver:
> > > - MIPI RAW12 output
> > > - Monochrome and Color (Bayer filter) variants
> > > - Multiple input clock frequencies
> > > - Multiple link frequencies
> > > - VBLANK and HBLANK control for variable framerate
> > > - VFLIP and HFLIP control for flipping readout
> > > - Exposure and analogue gain control
> > > - Test pattern control
> > >
> > > Following features are not currently supported:
> > > - MIPI RAW10 output
> > > - Pixel-perfect crop reporting, accounting for the shift-by-1 when
> > >   doing HFLIP/VFLIP where the sensor maintains RGGB bayer ordering
> > >
> > > Along with the ones below which depend on the new raw sensor model:
> > > - Embedded data stream
> > > - Freely configurable cropping
> > > - Increased framerate when cropping
> > > - 2x2 binning support
> > >
> > > Signed-off-by: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx>
> >
> > ...
> >
> > > +static const u32 codes_bayer[] = {
> > > +       MEDIA_BUS_FMT_SRGGB12_1X12,
> > > +};
> > > +
> > > +static const u32 codes_monochrome[] = {
> > > +       MEDIA_BUS_FMT_Y12_1X12,   /* 12-bit mono */
> >
> > Above comment adds no useful information and can be dropped.
> >
> > > +};
> > > +
> > > +static const struct imx678_model_info imx678_aaqr_info = {
> > > +       .type = IMX678_COLOR,
> > > +       .codes = codes_bayer,
> > > +       .num_codes = ARRAY_SIZE(codes_bayer),
> > > +};
> > > +
> > > +static const struct imx678_model_info imx678_aamr_info = {
> > > +       .type = IMX678_MONOCHROME,
> > > +       .codes = codes_monochrome,
> > > +       .num_codes = ARRAY_SIZE(codes_monochrome),
> > > +};
> > > +
> > > +static const char * const imx678_supply_name[] = {
> > > +       "avdd",  /* Analog (3.3V) supply */
> > > +       "dvdd",  /* Digital Core (1.1V) supply */
> > > +       "ovdd",  /* IF (1.8V) supply */
> > > +};
> > > +
> > > +struct imx678 {
> > > +       struct v4l2_subdev sd;
> > > +       struct media_pad pad;
> > > +       struct regmap *cci;
> > > +
> > > +       const struct imx678_model_info *info;
> > > +
> > > +       struct clk *xclk;
> > > +       u32 xclk_freq;
> > > +
> > > +       /* chosen INCK_SEL register value */
> > > +       u8  inck_sel_val;
> > > +
> > > +       /* Link configurations */
> > > +       enum imx678_lanemode lane_mode;
> > > +       unsigned long link_freq_bitmap;
> > > +
> > > +       struct gpio_desc *reset_gpio;
> > > +       struct regulator_bulk_data supplies[ARRAY_SIZE(imx678_supply_name)];
> > > +
> > > +       struct v4l2_ctrl_handler ctrl_handler;
> > > +
> > > +       /* V4L2 Controls */
> > > +       struct v4l2_ctrl *exposure;
> > > +       struct v4l2_ctrl *vblank;
> > > +       struct v4l2_ctrl *hblank;
> > > +
> > > +       /* Tracking sensor VMAX/HMAX value */
> > > +       u32 vmax;
> > > +};
> > > +
> > > +static inline struct imx678 *to_imx678(struct v4l2_subdev *_sd)
> > > +{
> > > +       return container_of(_sd, struct imx678, sd);
> >
> > Use container_of_const.
> >
>
> Why is that necessary?

container_of_const() preserves const and avoids accidentally casting it away.
For non-const pointers it behaves the same as container_of(), while for const
pointers it preserves constness.

>
> > > +}
> >
> > ...
> >
> > > +static int imx678_set_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > +       struct imx678 *imx678 = container_of(ctrl->handler, struct imx678,
> > > +                                            ctrl_handler);
> >
> > Use container_of_const.
> >
> > > +       struct i2c_client *client = v4l2_get_subdevdata(&imx678->sd);
> > > +       const struct v4l2_mbus_framefmt *format;
> > > +       struct v4l2_subdev_state *state;
> > > +       int rpm_in_use;
> > > +       int ret = 0;
> > > +
> > > +       state = v4l2_subdev_get_locked_active_state(&imx678->sd);
> > > +       format = v4l2_subdev_state_get_format(state, IMX678_SOURCE_PAD);
> > > +
> > > +       if (ctrl->id == V4L2_CID_VBLANK) {
> > > +               u32 current_exposure = imx678->exposure->cur.val;
> > > +
> > > +               imx678->vmax = format->height + ctrl->val;
> >
> > ........(1)
> >
> > > +
> > > +               current_exposure = clamp_t(u32, current_exposure,
> > > +                                          IMX678_EXPOSURE_MIN,
> > > +                                          imx678->vmax - IMX678_SHR_MIN);
> > > +               ret = __v4l2_ctrl_modify_range(imx678->exposure,
> > > +                                              IMX678_EXPOSURE_MIN,
> > > +                                              imx678->vmax - IMX678_SHR_MIN,
> > > +                                              1, current_exposure);
> > > +               if (ret)
> > > +                       return ret;
> > > +       }
> > > +
> > > +       /*
> > > +        * Applying V4L2 control value only happens when power is up for
> > > +        * streaming
> > > +        */
> > > +       rpm_in_use = pm_runtime_get_if_in_use(&client->dev);
> > > +       if (!rpm_in_use)
> > > +               return 0;
> >
> > As in the last revision, as I suggested before, I will again suggest using
> > pm_runtime_get_if_active() here instead of pm_runtime_get_if_in_use().
> >
> > This does not seem to align with the comment above:
> > Applying V4L2 control value only happens when power is up for streaming
> >
> > "Power is up" implies that the device is in the runtime PM ACTIVE state,
> > rather than simply having a non-zero usage count.
> >
>
> I agree with the comment being slightly misleading, but same as the last
> revision, I still don't fully buy your argument here :-)
>
> In the case you talk about, where PM is ACTIVE but usage count == 0, we
> anyway know that the count will only increase when .enable_streams is
> called, at which point the driver will anyway write *all* the registers
> including calling set_ctrl for each control with the cached values.
>
> So why should we do (redundant) writes here?

I think this is mostly a difference in expectations.

My view is that if the device is runtime PM ACTIVE, the hardware is accessible
and register writes can be performed. In that case, I would expect a control
change to be applied to hardware immediately.

With pm_runtime_get_if_in_use(), there is a state where the device is still
ACTIVE but control changes are only cached in software and not written to
hardware until streaming starts again. While the value is not lost, I would
expect hardware and control state to remain synchronized whenever the device
is already active.

So I understand the cached-control argument, but if the hardware is accessible,
I would prefer applying the control immediately rather than deferring it.

> > I also don't understand why we need to be strict here and require the
> > runtime PM usage count to be greater than zero. What matters before accessing
> > the hardware registers is that the device is powered and accessible, not
> > whether there is an active user holding a runtime PM reference.
> >
> > Anyway, rpm_in_use does not seem necessary here. The check could be simplified to:
> > if (pm_runtime_get_if_active(&client->dev) <= 0)
>
> The rpm_in_use value is used below in this function to ensure we don't do
> pm_runtime_put() in case of a negative retval. This is not really handled
> by most drivers today, but I wanted to fix it here given recent discussion
> [1] and annoying Sashiko reports.
>
> [1]: https://lore.kernel.org/all/ahyh0ZlwlZqr7VNa%40kekkonen.localdomain

Thats my understanding as well. With:
if (pm_runtime_get_if_active(&client->dev) <= 0)
	return 0;

both the 0 and negative return paths exit immediately, so neither the switch
statement nor pm_runtime_put() can be reached.

Best Regards,
Tarang