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

[Jai Luthra]

Hi Tarang,

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?

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

> 
> > +
> > +       switch (ctrl->id) {
> > +       case V4L2_CID_VBLANK:
> > +               cci_write(imx678->cci, IMX678_REG_VMAX, imx678->vmax, &ret);
> > +               fallthrough; /* SHR = VMAX - exposure, so update it */
> > +       case V4L2_CID_EXPOSURE: {
> > +               u32 shr = imx678->vmax - imx678->exposure->val;
> > +
> > +               cci_write(imx678->cci, IMX678_REG_SHR, shr, &ret);
> > +               break;
> > +       }
> > +       case V4L2_CID_ANALOGUE_GAIN:
> > +               cci_write(imx678->cci, IMX678_REG_GAIN, ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_HBLANK: {
> > +               u32 hmax = (format->width + ctrl->val) / IMX678_PIX_PER_CLK;
> > +
> > +               cci_write(imx678->cci, IMX678_REG_HMAX, hmax, &ret);
> > +               break;
> > +       }
> > +       case V4L2_CID_TEST_PATTERN: {
> > +               cci_write(imx678->cci, IMX678_REG_TPG_COLORWIDTH,
> > +                         IMX678_TPG_COLORWIDTH_160PIX, &ret);
> > +               cci_write(imx678->cci, IMX678_REG_TPG_PATSEL_DUOUT,
> > +                         imx678_tpg_val[ctrl->val], &ret);
> > +               cci_write(imx678->cci, IMX678_REG_TPG_EN_DUOUT,
> > +                         (ctrl->val) ? 1 : 0,
> > +                         &ret);
> > +               break;
> > +       }
> > +       case V4L2_CID_HFLIP:
> > +               cci_write(imx678->cci, IMX678_REG_WINMODEH, ctrl->val, &ret);
> > +               break;
> > +       case V4L2_CID_VFLIP:
> > +               cci_write(imx678->cci, IMX678_REG_WINMODEV, ctrl->val, &ret);
> > +               break;
> > +       default:
> > +               dev_warn(&client->dev,
> > +                        "ctrl(id:0x%x,val:0x%x) is not handled\n",
> > +                        ctrl->id, ctrl->val);
> > +               break;
> > +       }
> > +
> > +       if (rpm_in_use > 0)
> > +               pm_runtime_put(&client->dev);
> > +
> > +       return ret;
> > +}
> > +
> 
> ...
> 
> > +static int imx678_set_pad_format(struct v4l2_subdev *sd,
> > +                                struct v4l2_subdev_state *sd_state,
> > +                                struct v4l2_subdev_format *fmt)
> > +{
> > +       struct imx678 *imx678 = to_imx678(sd);
> > +       struct v4l2_mbus_framefmt *format;
> > +       const struct v4l2_rect *crop;
> > +       int ret = 0;
> > +
> > +       if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
> > +           v4l2_subdev_is_streaming(sd))
> > +               return -EBUSY;
> > +
> > +       crop = v4l2_subdev_state_get_crop(sd_state, fmt->pad);
> > +
> > +       fmt->format.width = crop->width;
> > +       fmt->format.height = crop->height;
> > +       fmt->format.code = imx678_get_format_code(imx678, fmt->format.code);
> > +       fmt->format.field = V4L2_FIELD_NONE;
> > +       fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> > +       fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > +       fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > +       fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
> > +
> > +       format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> > +
> > +       if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > +               ret = imx678_set_framing_limits(imx678, &fmt->format);
> > +
> > +       if (!ret)
> > +               *format = fmt->format;
> 
> Update the ACTIVE format before adjusting the framing controls so that control
> callbacks operate on the correct frame dimensions.
> 
> Currently, the VBLANK control handler recalculates the exposure limits using
> the previous height value.
> 
> You can verify this by adding a debug print at (1) (In imx678_set_ctrl) for format->height.
> 

Yikes, my bad. I updated this function at the last moment to not mess with
the active format in case set_framing_limit gives an error, but missed that
the active state format is used by s_ctrl.

> For example:
> Streaming is running with a format of 3856 × 2160.
> Streaming is stopped.
> The media link is reconfigured to 1920 × 1080.
> 
> At this point, the ACTIVE format has not yet been updated. As a result, the
> debug print at (1) will still show 2160 instead of 1080.
> 

Although it shouldn't cause issues as we don't allow changing resolution at
all.. at least not in this patch. I'll still fix it in my branch with crop
and binning support.

I just realized though that we don't need to call set_framing_limits at all
in a fixed-resolution driver. I'll drop it in next revision.

> > +
> > +       return ret;
> > +}
> > +
> > +static int imx678_get_selection(struct v4l2_subdev *sd,
> > +                               struct v4l2_subdev_state *sd_state,
> > +                               struct v4l2_subdev_selection *sel)
> > +{
> > +       switch (sel->target) {
> > +       case V4L2_SEL_TGT_CROP:
> > +               sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
> > +               return 0;
> > +
> > +       case V4L2_SEL_TGT_NATIVE_SIZE:
> > +               sel->r = imx678_native_area;
> > +               return 0;
> > +
> > +       case V4L2_SEL_TGT_CROP_DEFAULT:
> > +       case V4L2_SEL_TGT_CROP_BOUNDS:
> > +               sel->r = imx678_active_area;
> > +               return 0;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> > +
> > +static int imx678_init_state(struct v4l2_subdev *sd,
> > +                            struct v4l2_subdev_state *state)
> > +{
> > +       struct imx678 *imx678 = to_imx678(sd);
> > +       struct v4l2_rect *crop;
> > +       struct v4l2_subdev_format fmt = {
> > +               .which = V4L2_SUBDEV_FORMAT_TRY,
> > +               .pad = IMX678_SOURCE_PAD,
> > +               .format = {
> > +                       .code = imx678_default_mbus_code(imx678),
> > +                       .width = imx678_active_area.width,
> > +                       .height = imx678_active_area.height,
> > +               },
> > +       };
> > +
> > +       crop = v4l2_subdev_state_get_crop(state, IMX678_SOURCE_PAD);
> > +       *crop = imx678_active_area;
> > +       imx678_set_pad_format(sd, state, &fmt);
> > +
> > +       return 0;
> 
> return imx678_set_pad_format(..).
> 
> > +}
> 
> ...
> 
> > +static int imx678_identify_model(struct imx678 *imx678)
> > +{
> > +       struct i2c_client *client = v4l2_get_subdevdata(&imx678->sd);
> > +       const struct imx678_model_info *info;
> > +       enum imx678_type detected;
> > +       int ret = 0;
> > +       u64 val = 0;
> 
> val = 0 initialization is unnecessary, as val is populated by cci_read()
> before it is used.
> 

Similar to ret, I personally find it easier to read when values that are
used but not explicitly written to within the same function, are
initialized to 0.

> > +
> > +       info = device_get_match_data(&client->dev);
> > +
> > +       /*
> > +        * This sensor's ID registers become accessible 80ms after coming out
> > +        * of STANDBY mode.
> > +        */
> > +       cci_write(imx678->cci, IMX678_REG_MODE_SELECT, 0, &ret);
> > +       fsleep(IMX678_MODULE_ID_DELAY);
> > +
> > +       cci_read(imx678->cci, IMX678_REG_MODULE_ID, &val, &ret);
> > +
> > +       if (ret) {
> > +               dev_err(&client->dev,
> > +                       "I2C transaction failed ret = %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       if (val != IMX678_ID) {
> > +               dev_err(&client->dev,
> > +                       "Chip ID mismatch: %x!=%llx\n", IMX678_ID, val);
> > +               return -ENXIO;
> > +       }
> > +
> > +       cci_read(imx678->cci, IMX678_REG_MONOCHROME, &val, &ret);
> > +
> > +       if (ret) {
> > +               dev_err(&client->dev,
> > +                       "I2C transaction failed ret = %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       detected = val & IMX678_TYPE;
> > +
> > +       /* Prefer to use sensor type specified in device tree */
> > +       if (info) {
> > +               imx678->info = info;
> > +               if (detected != info->type)
> > +                       dev_err(&client->dev,
> > +                               "detected %s sensor, DT specifies %s; using DT value\n",
> > +                               detected == IMX678_COLOR ? "color" : "mono",
> > +                               info->type == IMX678_COLOR ? "color" : "mono");
> > +       } else {
> > +               imx678->info = detected == IMX678_MONOCHROME ?
> > +                              &imx678_aamr_info : &imx678_aaqr_info;
> > +               dev_info(&client->dev,
> > +                        "sensor type missing in DT; detected %s sensor\n",
> > +                        detected == IMX678_MONOCHROME ? "mono" : "color");
> > +       }
> > +
> > +       return 0;
> > +}
> 
> ...
> 
> > +static int imx678_probe(struct i2c_client *client)
> > +{
> > +       struct device *dev = &client->dev;
> > +       struct imx678 *imx678;
> > +       int ret, i;
> > +
> > +       imx678 = devm_kzalloc(&client->dev, sizeof(*imx678), GFP_KERNEL);
> > +       if (!imx678)
> > +               return -ENOMEM;
> > +
> > +       v4l2_i2c_subdev_init(&imx678->sd, client, &imx678_subdev_ops);
> > +
> > +       imx678->cci = devm_cci_regmap_init_i2c(client, 16);
> > +       if (IS_ERR(imx678->cci))
> > +               return dev_err_probe(dev, PTR_ERR(imx678->cci),
> > +                                    "failed to init CCI\n");
> > +
> > +       if (imx678_check_hwcfg(dev, imx678))
> > +               return -EINVAL;
> > +
> > +       imx678->xclk = devm_v4l2_sensor_clk_get(dev, NULL);
> > +       if (IS_ERR(imx678->xclk))
> > +               return dev_err_probe(dev, PTR_ERR(imx678->xclk),
> > +                                    "failed to get xclk\n");
> > +
> > +       imx678->xclk_freq = clk_get_rate(imx678->xclk);
> > +
> > +       for (i = 0; i < ARRAY_SIZE(imx678_inck_table); ++i) {
> > +               if (imx678_inck_table[i].xclk_hz == imx678->xclk_freq) {
> > +                       imx678->inck_sel_val = imx678_inck_table[i].inck_sel;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (i == ARRAY_SIZE(imx678_inck_table))
> > +               return dev_err_probe(dev, -EINVAL,
> > +                                    "unsupported XCLK rate %u Hz\n",
> > +                                    imx678->xclk_freq);
> > +
> > +       for (i = 0; i < ARRAY_SIZE(imx678_supply_name); i++)
> > +               imx678->supplies[i].supply = imx678_supply_name[i];
> > +
> > +       ret = devm_regulator_bulk_get(&client->dev,
> > +                                     ARRAY_SIZE(imx678_supply_name),
> > +                                     imx678->supplies);
> > +       if (ret)
> > +               return dev_err_probe(dev, ret, "failed to get regulators\n");
> > +
> > +       imx678->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > +                                                    GPIOD_OUT_HIGH);
> > +       if (IS_ERR(imx678->reset_gpio))
> > +               return dev_err_probe(dev, PTR_ERR(imx678->reset_gpio),
> > +                                    "failed to get reset GPIO\n");
> > +
> > +       ret = imx678_power_on(dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = imx678_identify_model(imx678);
> > +       if (ret)
> > +               goto error_power_off;
> > +
> > +       pm_runtime_set_active(dev);
> > +       pm_runtime_enable(dev);
> > +
> > +       ret = imx678_init_controls(imx678);
> > +       if (ret)
> > +               goto error_pm_runtime;
> > +
> > +       imx678->sd.internal_ops = &imx678_internal_ops;
> > +       imx678->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > +                               V4L2_SUBDEV_FL_HAS_EVENTS;
> 
> Drop V4L2_SUBDEV_FL_HAS_EVENTS flag.
> 

Ack.

[snip]

Thanks,
    Jai