Re: [PATCH v5 2/2] media: i2c: mira016: Add driver for Mira016

[Tarang Raval]

Hi Jacopo,

I noticed a few issues. Could you please check the comments below?

> Add driver for the ams OSRAM Mira016 sensor.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> ---
>  MAINTAINERS                 |    1 +
>  drivers/media/i2c/Kconfig   |   12 +
>  drivers/media/i2c/Makefile  |    1 +
>  drivers/media/i2c/mira016.c | 2309 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 2323 insertions(+)

...

> +/* Register select. */
> +#define MIRA016_BANK_SEL_REG           CCI_REG8(0xe000)
> +#define MIRA016_ACTIVE_CONTEXT_REG     CCI_REG8(0x4002)
> +#define MIRA016_NEXT_ACTIVE_CONTEXT_REG        CCI_REG8(0xe003)
> +#define MIRA016_RW_CONTEXT_REG         CCI_REG8(0xe004)
> +#define MIRA016_AUTO_SWITCH_CONTEXT_REG        CCI_REG8(0xe005)
> +#define MIRA016_PARAM_HOLD_REG         CCI_REG8(0x0006)
> +#define MIRA016_DISABLE_CONTEXTSYNC_REG        CCI_REG8(0xe008)
> +#define MIRA016_DISABLE_CONTEXTSYNC    BIT(0)
> +#define MIRA016_CMD_REQ_1_REG          CCI_REG8(0x000a)
> +#define MIRA016_CMD_HALT_BLOCK_REG     CCI_REG8(0x000c)

A few of these macros are unused. Can we remove them?

> +
> +/* Chip id */
> +#define MIRA016_CHIP_ID_REG            CCI_REG8(0x011B)
> +#define MIRA016_CHIP_ID                        33

...

> +static const struct cci_reg_sequence mira016_8b_fine_gain_init[] = {
> +     { CCI_REG8(0xe000), 0x0 },
> +     { CCI_REG8(0x01bb), 0xb4 },
...
> +     { CCI_REG8(0xe000), 0x1 },
> +     { CCI_REG8(0xe000), 0x1 },
> +     { CCI_REG8(0xe024), 0x3 },
> +     { CCI_REG8(0xe000), 0x0 },
> +     { CCI_REG8(0xe000), 0x0 },
> +     { CCI_REG8(0xe000), 0x0 },
> +     { CCI_REG8(0x005c), 0x0 },
> +     { CCI_REG8(0x005d), 0x18 },
> +     { CCI_REG8(0xe000), 0x0 },
> +};

I noticed some registers are written multiple times within each
of these arrays. Are all the repeated writes required, or can
the redundant ones be removed?

Like for the above array 0xe000 register.

> +static void mira016_update_pad_format(struct mira016 *mira016,
> +                                     struct v4l2_mbus_framefmt *fmt, u32 code)

third argument is unused. Can we remove it?

> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(mira016_mbus_formats); ++i) {
> +               if (mira016_mbus_formats[i] == fmt->code)
> +                       break;
> +       }
> +       if (i == ARRAY_SIZE(mira016_mbus_formats))
> +               fmt->code = mira016_mbus_formats[0];
> +
> +       fmt->width = MIRA016_PIXEL_ARRAY_WIDTH;
> +       fmt->height = MIRA016_PIXEL_ARRAY_HEIGHT;
> +       fmt->field = V4L2_FIELD_NONE;
> +       fmt->colorspace = V4L2_COLORSPACE_RAW;
> +       fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +       fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +       fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> +}

...

> +static int mira016_set_pad_format(struct v4l2_subdev *sd,
> +                                 const struct v4l2_subdev_client_info *ci,
> +                                 struct v4l2_subdev_state *state,
> +                                 struct v4l2_subdev_format *fmt)
> +{
> +       struct mira016 *mira016 = to_mira016(sd);
> +       struct v4l2_rect *crop;
> +       u32 pixel_rate;
> +       u32 min_vblank;
> +       u32 gain_max;
> +       int ret;
> +
> +       mira016_update_pad_format(mira016, &fmt->format, fmt->format.code);
> +       *v4l2_subdev_state_get_format(state, 0) = fmt->format;
> +
> +       crop = v4l2_subdev_state_get_crop(state, 0);
> +       crop->width = fmt->format.width;
> +       crop->height = fmt->format.height;
> +       crop->left = MIRA016_PIXEL_ARRAY_LEFT;
> +       crop->top = MIRA016_PIXEL_ARRAY_TOP;
> +
> +       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> +               return 0;
> +
> +       /*
> +        * Update the row length: changing the image format implies changing the
> +        * row_length parameter, which changes the line duration and the pixel
> +        * rate consequentially. Also, changing the image format changes the
> +        * analogue gain limits.
> +        *
> +        * Do not allow to change image format while the subdevice is streaming.
> +        *
> +        * TODO: row length depends on binning, update it also in the
> +        * implementation of set_selection.
> +        */
> +       if (v4l2_subdev_is_streaming(sd))
> +               return -EBUSY;

State is overwritten before the streaming check, so a failed call
with -EBUSY still changes the active format.

Should we move this check to the top of the function and only check
it for V4L2_SUBDEV_FORMAT_ACTIVE?

> +
> +       switch (fmt->format.code) {
> +       case MEDIA_BUS_FMT_Y8_1X8:
> +               gain_max = ARRAY_SIZE(mira016_gain_lut_8bit);
> +               break;
> +       case MEDIA_BUS_FMT_Y10_1X10:
> +               gain_max = ARRAY_SIZE(mira016_gain_lut_10bit);
> +               break;
> +       case MEDIA_BUS_FMT_Y12_1X12:
> +       default:
> +               /*
> +                * TODO: Clarify how to handle 12 bit 2x fixed gain which
> +                * changes the line timings while streaming. Only allow 1x
> +                * for the time being.
> +                */
> +               gain_max = 1;
> +               break;
> +       }
> +
> +       ret = __v4l2_ctrl_modify_range(mira016->gain, 1, gain_max, 1, 1);
> +       if (ret)
> +               return ret;
> +
> +       mira016_calc_row_length(mira016, state);
> +
> +       min_vblank = mira016_calc_min_vblank(mira016, crop->height);
> +       ret = __v4l2_ctrl_modify_range(mira016->vblank, min_vblank,
> +                                      MIRA016_MAX_VBLANK, 1, min_vblank);
> +       if (ret)
> +               return ret;
> +
> +       pixel_rate = mira016_calc_prate(mira016, crop->width);
> +
> +       return __v4l2_ctrl_modify_range(mira016->prate, pixel_rate, pixel_rate,
> +                                       1, pixel_rate);
> +}
> +
> +                                 u64 streams_mask)
> +{
> +       struct mira016 *mira016 = to_mira016(sd);
> +       struct i2c_client *client = v4l2_get_subdevdata(&mira016->sd);

We can use mira016->dev directly instead &client->dev.

> +       int ret;

...

> +static int mira016_disable_streams(struct v4l2_subdev *sd,
> +                                  struct v4l2_subdev_state *state, u32 pad,
> +                                  u64 streams_mask)
> +{
> +       struct mira016 *mira016 = to_mira016(sd);
> +       struct i2c_client *client = v4l2_get_subdevdata(&mira016->sd);

same here.

> +

...

> +static int mira016_init_state(struct v4l2_subdev *sd,
> +                             struct v4l2_subdev_state *state)
> +{
> +       struct v4l2_subdev_format fmt = {
> +               .which = V4L2_SUBDEV_FORMAT_TRY,
> +               .pad = 0,
> +               .format = {
> +                       .code = MEDIA_BUS_FMT_Y8_1X8,
> +                       .width = MIRA016_PIXEL_ARRAY_WIDTH,
> +                       .height = MIRA016_PIXEL_ARRAY_HEIGHT
> +               },
> +       };
> +
> +       mira016_set_pad_format(sd, NULL, state, &fmt);
> +
> +       return 0;

You can directly return the result of
mira016_set_pad_format() here.

> +}

...

> +static int mira016_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +       struct mira016 *mira016 =
> +               container_of(ctrl->handler, struct mira016, ctrl_handler);
> +       struct i2c_client *client = v4l2_get_subdevdata(&mira016->sd);

we can use mira016->dev directly instead &client->dev.

> +       struct v4l2_subdev_state *state;
> +       struct v4l2_rect *crop;
> +       int ret = 0;
> +
> +       state = v4l2_subdev_get_locked_active_state(&mira016->sd);
> +       crop = v4l2_subdev_state_get_crop(state, 0);
> +
> +       if (ctrl->id == V4L2_CID_VBLANK) {
> +               s32 exposure_max = crop->height + ctrl->val
> +                                - MIRA016_FRAME_INTEGRATION_DIFF;
> +               s32 exposure_def = min(exposure_max,
> +                                      mira016->exposure->val);
> +
> +               ret = __v4l2_ctrl_modify_range(mira016->exposure,
> +                                              mira016->exposure->minimum,
> +                                              exposure_max,
> +                                              mira016->exposure->step,
> +                                              exposure_def);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       if (!pm_runtime_get_if_in_use(&client->dev))
> +               return 0;
> +
> +       switch (ctrl->id) {
> +       case V4L2_CID_EXPOSURE:
> +               ret = mira016_write_exposure_reg(mira016, ctrl->val);
> +               break;
> +       case V4L2_CID_VBLANK:
> +               ret = mira016_write_frame_duration_reg(mira016, state, ctrl->val);
> +               break;
> +       case V4L2_CID_ANALOGUE_GAIN:
> +               ret = mira016_write_analogue_gain(mira016, state, ctrl->val);
> +               break;

Why do you not introduce vflip and hflip controls here?

> +       default:
> +               ret = -EINVAL;
> +               break;
> +       }
> +
> +       pm_runtime_put_autosuspend(&client->dev);
> +
> +       return ret;
> +}

...

> +static int mira016_init_controls(struct mira016 *mira016)
> +{
> +       struct i2c_client *client = v4l2_get_subdevdata(&mira016->sd);

we can use mira016->dev directly.

> +       struct v4l2_fwnode_device_properties props;
> +       struct v4l2_ctrl_handler *ctrl_hdlr;
> +       struct v4l2_ctrl *link_freq;
> +       struct v4l2_ctrl *hblank;
> +       u32 min_exposure_lines;
> +       u32 def_exposure;
> +       u32 min_vblank;
> +       u32 def_vblank;
> +       u32 pixel_rate;
> +       int ret;
> +

...

> +static u8 mira016_m_to_pll_m(u32 pll_m)
> +{
> +       /* Table 12: Lookup table for “M to PLL_DIV_M” mapping */
> +       static const struct pll_m_div {
> +               u8 m_min;
> +               u8 m_max;
> +               u8 pll_m_min;
> +               u8 pll_m_max;
> +       } pll_m_lut[] = {
> +               { 16, 31, 224, 239 }, { 32, 63, 192, 233 },

I’m not sure, but I think 233 might be a typo and should be 223,
Since the m range and PLL code range don’t match.

> +               { 64, 127, 128, 191 }, { 1285, 0, 127 },
> +       };
> +
> +       for (unsigned int i = 0; i < ARRAY_SIZE(pll_m_lut); ++i) {
> +               const struct pll_m_div *p = &pll_m_lut[i];
> +
> +               if (pll_m > p->m_max)
> +                       continue;
> +
> +               return p->pll_m_min + pll_m - p->m_min;
> +       }
> +
> +       return 0;
> +}

...

> +               for (m = MIRA016_PLL_M_MIN; m < MIRA016_PLL_M_MAX; ++m) {
> +                       u32 pll2 = pll1 * m;
> +
> +                       if (pll2 < MIRA016_PLL_PLL2_MIN ||
> +                           pll2 > MIRA016_PLL_PLL2_MAX)
> +                               continue;
> +
> +                       if (pll2 == target_mbps) {
> +                               found = true;
> +                               break;
> +                       }
> +
> +                       if (abs(pll2 - target_mbps) < best) {

Since these are unsigned int values, it would be better to use
abs_diff() instead of abs() here.

> +                               n_best = n;
> +                               m_best = m;
> +                               best = abs(pll2 - target_mbps);
> +                       }
> +               }
> +               if (found)
> +                       break;
> +       }

...

> +static int mira016_get_regulators(struct mira016 *mira016)
> +{
> +       struct i2c_client *client = v4l2_get_subdevdata(&mira016->sd);

We can remove this as well.

> +       for (unsigned int i = 0; i < ARRAY_SIZE(mira016_supplies); i++)
> +               mira016->supplies[i].supply = mira016_supplies[i];
> +
> +       return devm_regulator_bulk_get(&client->dev,
> +                                      ARRAY_SIZE(mira016_supplies),
> +                                      mira016->supplies);
> +}
> +

...

> +static int mira016_probe(struct i2c_client *client)
> +{
> +       struct device *dev = &client->dev;
> +       struct mira016 *mira016;
> +       int ret;
> +
> +       mira016 = devm_kzalloc(&client->dev, sizeof(*mira016), GFP_KERNEL);
> +       if (!mira016)
> +               return -ENOMEM;
> +
> +       mira016->dev = &client->dev;
> +
> +       ret = mira016_parse_endpoint(dev, mira016);
> +       if (ret)
> +               return ret;
> +
> +       v4l2_i2c_subdev_init(&mira016->sd, client, &mira016_subdev_ops);
> +
> +       mira016->regmap = devm_cci_regmap_init_i2c(client, 16);
> +       if (IS_ERR(mira016->regmap))
> +               return dev_err_probe(dev, PTR_ERR(mira016->regmap),
> +                                    "failed to initialize CCI\n");
> +
> +       mira016->xclk = devm_v4l2_sensor_clk_get(dev, NULL);
> +       if (IS_ERR(mira016->xclk))
> +               return dev_err_probe(dev, PTR_ERR(mira016->xclk),
> +                                    "failed to get xclk\n");
> +
> +       mira016->xclk_freq = clk_get_rate(mira016->xclk);
> +       if (mira016_validate_xclk_freq(mira016)) {
> +               dev_err(dev, "xclk frequency not supported: %d Hz\n",

Please use dev_err_probe.

> +                       mira016->xclk_freq);
> +               return -EINVAL;
> +       }
> +
> +       ret = mira016_get_regulators(mira016);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "failed to get regulators\n");
> +
> +       mira016->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +                                                     GPIOD_OUT_HIGH);
> +       if (IS_ERR(mira016->reset_gpio))
> +               return dev_err_probe(dev, PTR_ERR(mira016->reset_gpio),
> +                                    "failed to get reset gpio\n");
> +
> +       /*
> +        * Calculate the PLL configuration based on the link frequency
> +        * selected by .dts and compute the sensor timing bases.
> +        *
> +        * Initialize row_length to a value matching the default format for
> +        * exposure and frame time limits calculations.
> +        */
> +       mira016_pll_calc(mira016);
> +       mira016_timings_calc(mira016);
> +       mira016->timings.row_length = 1262;

1262 doesn’t match any of the supported formats. Is this a typo?
Should it be 1062 instead?

> +
> +       ret = mira016_power_on(dev);
> +       if (ret)
> +               return ret;
> +
> +       /* Enable runtime PM and power on the device */
> +       pm_runtime_set_active(dev);
> +       pm_runtime_enable(dev);
> +
> +       ret = mira016_identify_module(mira016);
> +       if (ret)
> +               goto error_power_off;
> +
> +       ret = mira016_init_controls(mira016);
> +       if (ret)
> +               goto error_power_off;
> +
> +       /* Initialize subdev */
> +       mira016->sd.internal_ops = &mira016_internal_ops;
> +       mira016->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +       mira016->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +       /* Initialize source pads */
> +       mira016->pad.flags = MEDIA_PAD_FL_SOURCE;
> +
> +       ret = media_entity_pads_init(&mira016->sd.entity, 1, &mira016->pad);
> +       if (ret) {
> +               dev_err_probe(dev, ret, "failed to init entity pads\n");
> +               goto error_handler_free;
> +       }
> +
> +       mira016->sd.state_lock = mira016->ctrl_handler.lock;
> +       ret = v4l2_subdev_init_finalize(&mira016->sd);
> +       if (ret < 0) {
> +               dev_err_probe(dev, ret, "subdev init error\n");
> +               goto error_media_entity;
> +       }
> +
> +       ret = v4l2_async_register_subdev_sensor(&mira016->sd);
> +       if (ret < 0) {
> +               dev_err_probe(dev, ret,
> +                             "failed to register sensor sub-device\n");
> +               goto error_subdev_cleanup;
> +       }
> +
> +       pm_runtime_set_autosuspend_delay(dev, 1000);
> +       pm_runtime_use_autosuspend(dev);
> +       pm_runtime_idle(dev);
> +
> +       return 0;
> +
> +error_subdev_cleanup:
> +       v4l2_subdev_cleanup(&mira016->sd);
> +error_media_entity:
> +       media_entity_cleanup(&mira016->sd.entity);
> +error_handler_free:
> +       v4l2_ctrl_handler_free(mira016->sd.ctrl_handler);
> +error_power_off:
> +       pm_runtime_disable(dev);
> +       if (!pm_runtime_status_suspended(&client->dev))

Could we use dev here as well, just for consistency?

> +               mira016_power_off(dev);
> +       pm_runtime_set_suspended(dev);
> +       return ret;
> +}
> +

...

> +static const struct of_device_id mira016_dt_ids[] = {
> +       { .compatible = "ams,mira016" },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mira016_dt_ids);
> +
> +static struct i2c_driver mira016_i2c_driver = {
> +       .driver = {
> +               .name = "mira016",
> +               .of_match_table = mira016_dt_ids,
> +               .pm = pm_ptr(&mira016_pm_ops),
> +       },
> +       .probe = mira016_probe,
> +       .remove = mira016_remove,
> +};
> +
> +module_i2c_driver(mira016_i2c_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("ams OSRAM MIRA016 sensor driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.55.0

Best Regards,
Tarang