| Newsgroups |
org.kernel.vger.linux-devicetree,org.kernel.vger.linux-kernel,org.kernel.vger.linux-media |
| Message-ID |
<TY6PR01MB17839D1B955BFC76430F6152392D32@TY6PR01MB17839.jpnprd01.prod.outlook.com> |
Dear Laurent,
Thank-you for your review.
<snip>
>> +#define IMX908_DEFAULT_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10
>> +
>> +/* IMX908 total area includes effective area plus
>
>This should be
>
>/*
>* IMX908 total area includes effective area plus
>
>> + * 10 effective OB pixels in vertical direction,
>> + * 10 pixels OB side ignored area in height
>> + */
I will review the comments for alignment in v2.
<snip>
>> +struct imx908_mode {
>> + /* Sensor timing */
>> + u16 hmax; /* clocks per line */
>> + u32 vmax; /* lines per frame */
>> +
>> + /* Derived values */
>> + u64 pixel_rate;
>> +};
>
>This structure should be dropped. It will be easier to do with a
>hardcoded pixel_rate value (see below), but should be done even if we
>decide to keep the pixel rate dynamic. I can give it a try based on v2
>of the driver if you're having trouble.
In v2 I have
- hardcoded the pixel_rate
- dropped the pixel_rate from the structure
- moved hmax and vmax to the main struct imx908 and deleted struct imx908_mode
<snip>
>> +/* Compute pixel-rate from link-freq/lanes/bpp */
>> +static inline u64 imx908_pixel_rate(u8 lanes, u8 bpp, u64 linkfreq_hz)
>> +{
>> + u64 total = lanes * linkfreq_hz * 2; /* DDR: 2 * linkfreq */
>> +
>> + return div_u64(total, bpp);
>
>In his IMX678 driver, Jai hardcodes the pixel rate to 594MHz, which is
>8 * 74.25MHz (IMX908_XHS_HZ). The rationale is that HMAX is expressed in
>units of the 74.25 MHz internal clock, so we assumed that the pixel
>array is not read out using the link clock but using a clock derived
>from the internal clock. The multiplier was chosen to ensure that the
>minimum HMAX value corresponds to a positive HBLANK, as negative HBLANK
>would confuse userspace (and developers). Having a fixed pixel rate
>greatly simplifies the driver, as you don't have to update the pixel
>rate control, and HMAX calculation from HBLANK becomes a division by 8.
>
>We're not sure how the pixel array of the IMX678 is clocked exactly, so
>I don't know if the above corresponds to reality, neither for the IMX678
>nor the IMX908. If you could provide more information about the pixel
>array readout, that would be greatly appreciated. Note that the pixel
>rate value does not have to match the physical readout clock, as it is
>only used by userspace to calculate timings (to convert between h/v
>blank and fps).
>
>Jai can provide more information if needed.
After a little bit of wavering in v2 I have hardcoded the pixel rate as per IMX678.
In parallel I have tried to get more information from the engineers internally,
however this is still on-going.
I have also tweaked the constraints on the minimum HMAX, you can see this
in v2.
>> +static int imx908_link_freq_to_datarate_sel(struct imx908 *imx, u64 link_freq_hz, u8 *sel)
>> +{
>> + for (unsigned int i = 0; i < ARRAY_SIZE(imx908_link_freqs); i++) {
>> + if (imx908_link_freqs[i] == link_freq_hz) {
>> + *sel = imx908_datarate_sel[i];
>> + return 0;
>> + }
>> + }
>
>A blank line would be nice here.
Ok
>> + dev_err(imx->dev, "Unsupported link frequency %llu Hz\n", link_freq_hz);
>
>The caller prints a similar error message, you can drop this one.
Ok.
>> + return -EINVAL;
>> +}
>> +
<snip>
>> +
>> +static void imx908_update_framing_limits(struct imx908 *imx, struct v4l2_subdev_state *state)
>
>Please wrap the line.
Ok.
<snip>
>> + if (ret) {
>> + dev_err(imx->dev, "Mode register write failed: %d\n", ret);
>> + return ret;
>> + }
>
>You can drop the error check here.
Removed in v2.
> > + for (unsigned int i = 0; i < ARRAY_SIZE(tuning_regs); ++i)
> > + cci_write(imx->cci, tuning_regs[i], value, &ret);
> > +
> > + if (ret) {
> > + dev_err(imx->dev, "Tuning register write failed: %d\n", ret);
> > + return ret;
> > + }
>
> And this one too.
Removed in v2.
<snip>
>> + ret = imx908_link_freq_to_datarate_sel(imx, link_freq, &datarate_sel);
>> + if (ret) {
>> + dev_err(imx->dev, "Unsupported link_freq=%llu for DATARATE_SEL\n",
>> + link_freq);
>> + return ret;
>> + }
>
>The link frequency is validated at probe time, so the
>imx908_link_freq_to_datarate_sel() can never return an error. Drop error
>checking here too.
Ok, understood.
>> + cci_write(imx->cci, IMX908_REG_DATARATE_SEL, datarate_sel, &ret);
>> +
>> + /* Lane config */
>> + cci_write(imx->cci, IMX908_REG_LANEMODE, imx->num_lanes - 1, &ret);
>> + if (ret) {
>> + dev_err(imx->dev, "Clock/lane configuration failed: %d\n", ret);
>> + return ret;
>> + }
>
>Drop this error check here too.
Will remove in v2.
>> +
>> + /* Recommended black level offset is 50 in 10-bit, 200 for others */
>> + u16 blklevel = (mdbit == IMX908_MDBIT_RAW10) ? 50 : 200;
>> +
>> + cci_write(imx->cci, IMX908_REG_BLKLEVEL, blklevel, &ret);
>> +
>> + return ret;
>
>The error is propagated through the chained cci_write() calls and
>handled here. That's all you need, cci_write() will print a message on
>failure, with the register address and value.
Understood, will fix this section in v2.
<snip>
>> + /* Get active state */
>> + sd_state = v4l2_subdev_get_locked_active_state(&imx->sd);
>
>The caller already has the active state, pass it as an argument to the
>imx908_start_streaming() function.
Ok.
<snip>
>> +/* --------------------------- V4L2 controls ------------------------------ */
>> +
>> +static int imx908_set_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct imx908 *imx = container_of(ctrl->handler, struct imx908, ctrls.handler);
>> + struct v4l2_subdev_state *state;
>> + struct i2c_client *client = v4l2_get_subdevdata(&imx->sd);
>> + const struct v4l2_mbus_framefmt *format;
>> + int ret = 0;
>> +
>> + /* Retrieve the active, locked subdev state format dimensions */
>> + state = v4l2_subdev_get_locked_active_state(&imx->sd);
>> + format = v4l2_subdev_state_get_format(state, IMX908_SOURCE_PAD);
>> +
>> + /* Applying V4L2 controls only if powered up */
>> + if (!pm_runtime_get_if_in_use(&client->dev))
>
> if (!pm_runtime_get_if_in_use(imx->dev))
>
>and similarly below, and drop the client local variable.
Understood.
>> + return 0;
>> +
>> + switch (ctrl->id) {
>> + case V4L2_CID_EXPOSURE: {
>> + u32 lines = clamp_t(u32, ctrl->val, 1,
>> + imx->mode.vmax - IMX908_MIN_SHR0);
>
>Drop this, the control framework already clamps to the limits.
Fixed in v2.
>> + ret = imx908_set_exposure_lines(imx, lines);
>> + break;
>> + }
>> +
>> + case V4L2_CID_ANALOGUE_GAIN:
>> +
>
>Drop this blank line.
Ok.
>> + u32 reg = clamp_t(u32, ctrl->val, IMX908_ANA_GAIN_MIN,
>> + IMX908_ANA_GAIN_MAX);
>
>Drop this, the control framework already clamps to the limits.
Fixed in v2.
>> +
>> + cci_write(imx->cci, IMX908_REG_GAIN, reg, &ret);
>> + break;
>> +
>> + case V4L2_CID_VBLANK: {
>> + const struct v4l2_rect *crop;
>> +
>> + crop = v4l2_subdev_state_get_crop(state, IMX908_SOURCE_PAD);
>> +
>> + u32 min_vblank = imx908_calc_min_vblank(crop);
>> + u32 max_vblank = imx908_calc_max_vblank(format->height);
>> + u32 vblank = clamp_t(u32, ctrl->val, min_vblank, max_vblank);
>> +
>> + imx->mode.vmax = imx908_calc_vmax(format->height, vblank);
>> +
>> + u32 exposure = min_t(u32, imx->ctrls.exposure->val,
>> + imx->mode.vmax - IMX908_MIN_SHR0);
>> +
>> + __v4l2_ctrl_modify_range(imx->ctrls.exposure,
>> + IMX908_EXPOSURE_MIN,
>> + imx->mode.vmax - IMX908_MIN_SHR0,
>> + IMX908_EXPOSURE_STEP, exposure);
>
>All the the above should go before the pm_runtime_get_if_in_use() call,
>as the exposure control limits needs to be updated even if the sensor
>isn't streaming. You can add a
>
> if (ctrl->id == V4L2_CID_VBLANK) {
> ...
> }
>
>block there.
This has been modified per your suggestions in v2.
<snip>
>> + ret = imx908_start_streaming(imx);
>> + if (ret) {
>> + pm_runtime_mark_last_busy(imx->dev);
>> + pm_runtime_put_autosuspend(imx->dev);
>
>I would add
>
> return ret;
>
>> + }
>> +
>> + return ret;
>
>and here write
>
> return 0;
>
>to clearly isolate the error path from the success path.
>
>That's a personal preference though, up to you.
I will follow your suggestion.
<snip>
>> + ret = imx908_set_selection(sd, sd_state, &sel);
>> + ret = imx908_set_pad_format(sd, sd_state, &fmt);
>
>Those two functions return errors only when called with invalid
>arguments. Invalid .which, .pad or .target values in sel or fmt would be
>driver bugs, so I think it's safe to not check for errors. You shouldn't
>assign ret, and you can
>
> return 0;
Ok.
<snip>
>> + ret = regulator_bulk_enable(ARRAY_SIZE(imx908_supply_names), imx->supplies);
>> + if (ret) {
>> + dev_err(imx->dev, "%s: failed to enable regulators\n", __func__);
>
>You can drop the "%s: " prefix, the error message is clear enough and
>doesn't need the function name.
Fixed in v2.
<snip>
>> + return dev_err_probe(imx->dev, -EINVAL, "unsupported xclk %u Hz\n", imx->xclk_freq);
>
> return dev_err_probe(imx->dev, -EINVAL, "unsupported xclk %u Hz\n",
> imx->xclk_freq);
I have updated the driver in v2 with stricter 80 column lin-breaking.
<snip>
>> +static int imx908_init_controls(struct imx908 *imx)
>> +{
>> + struct v4l2_ctrl_handler *hdl = &imx->ctrls.handler;
>> + struct v4l2_ctrl *link_freq_ctl;
>> + struct v4l2_fwnode_device_properties props;
>> + int ret;
>> + s32 hblank, min_hblank;
>> + u32 max_exp, max_hblank, min_vblank, max_vblank;
>> + u16 min_hmax;
>
>We tend to follow a "reverse christmas tree" order for variable
>declaration (it's one of those coding style rules that aim at increasing
>consistency but are otherwise completely arbitrary).
>
> u32 max_exp, max_hblank, min_vblank, max_vblank;
> s32 hblank, min_hblank;
> u16 min_hmax;
> int ret;
>
>I think hblank and min_hblank can be u32, as they're calculated below
>with imx908_hmax_to_hblank() that returns a u32.
Understood about the indenting and fixed the assignment for these variables.
>> +
>> + ret = v4l2_ctrl_handler_init(hdl, 11);
>> + if (ret)
>> + return ret;
>> +
>> + /* Initialize the pixel_rate control */
>> + u8 bpp = imx908_bits_per_pixel(IMX908_DEFAULT_MBUS_CODE);
>> + u64 link_freq = imx908_link_freqs[imx->link_freq_idx];
>> + u64 pixel_rate = imx908_pixel_rate(imx->num_lanes, bpp, link_freq);
>> +
>> + imx->ctrls.pixel_rate = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops, V4L2_CID_PIXEL_RATE,
>> + pixel_rate, pixel_rate, 1, pixel_rate);
>> + if (imx->ctrls.pixel_rate)
>> + imx->ctrls.pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +
>> + /* Set link Frequency */
>> + link_freq_ctl = v4l2_ctrl_new_int_menu(hdl, &imx908_ctrl_ops,
>> + V4L2_CID_LINK_FREQ,
>> + ARRAY_SIZE(imx908_link_freqs) - 1,
>> + imx->link_freq_idx,
>> + imx908_link_freqs);
>> +
>> + if (link_freq_ctl)
>> + link_freq_ctl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> +
>> + /* Initialize HMAX and VMAX */
>> + imx->mode.hmax = IMX908_HMAX_DEFAULT;
>> + imx->mode.vmax = IMX908_VMAX_DEFAULT;
>> +
>> + /* Set vblank */
>> + min_vblank = IMX908_VMAX_DEFAULT - imx908_active_area.height;
>> + max_vblank = imx908_calc_max_vblank(imx908_active_area.height);
>
>As you declare the bpp, link_freq and pixel_rate variables just above
>the code that uses them, instead of at the beginning of the function, I
>would do the same here:
>
> u32 min_vblank = IMX908_VMAX_DEFAULT - imx908_active_area.height;
> u32 max_vblank = imx908_calc_max_vblank(imx908_active_area.height);
>
>Unless Sakari has a stricter rule about declaring all variables at the
>beginning of the function ?
In v2 I have mostly tried to go with the inline declarations but if it is better
at the beginning of the function please comment again when I submit v2.
>> +
>> + imx->ctrls.vblank = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops,
>> + V4L2_CID_VBLANK,
>> + min_vblank,
>> + max_vblank,
>> + 1,
>> + min_vblank);
>> +
>> + /* Set hblank */
>> + min_hmax = imx908_calc_min_hmax(imx908_active_area.width, pixel_rate);
>> + min_hblank = imx908_hmax_to_hblank(min_hmax, pixel_rate,
>> + imx908_active_area.width);
>> + max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX, pixel_rate,
>> + imx908_active_area.width);
>> + hblank = imx908_hmax_to_hblank(IMX908_HMAX_DEFAULT, pixel_rate,
>> + imx908_active_area.width);
>
>Same here.
>
> u16 min_hmax = imx908_calc_min_hmax(imx908_active_area.width, pixel_rate);
> u32 min_hblank = imx908_hmax_to_hblank(min_hmax, pixel_rate,
> imx908_active_area.width);
> u32 max_hblank = imx908_hmax_to_hblank(IMX908_HMAX_MAX, pixel_rate,
> imx908_active_area.width);
> u32 hblank = imx908_hmax_to_hblank(IMX908_HMAX_DEFAULT, pixel_rate,
> imx908_active_area.width);
Ok.
>> +
>> + imx->ctrls.hblank = v4l2_ctrl_new_std(hdl, &imx908_ctrl_ops,
>> + V4L2_CID_HBLANK,
>> + min_hblank,
>> + max_hblank,
>> + 1,
>> + hblank);
>> + max_exp = imx->mode.vmax - IMX908_MIN_SHR0;
>
> u32 max_exp = imx->mode.vmax - IMX908_MIN_SHR0;
>
>or better
>
> u32 max_exp = IMX908_VMAX_DEFAULT - IMX908_MIN_SHR0;
>
>as that's more explicit.
Modified in v2.
<snip>
>> + /* Pick up rotation and orientation if defined in overlay */
>> + ret = v4l2_fwnode_device_parse(imx->dev, &props);
>> + if (ret)
>> + return ret;
>> +
>> + ret = v4l2_ctrl_new_fwnode_properties(hdl, &imx908_ctrl_ops, &props);
>> + if (ret)
>> + return ret;
>
>You need to call v4l2_ctrl_handler_free() in the error path here.
>Better, just drop error checking. The function sets hdl->error upon
>failure, so the error check just below is enough.
Ok.
>> +
>> + if (hdl->error) {
>> + ret = hdl->error;
>> + v4l2_ctrl_handler_free(hdl);
>> + return ret;
>> + }
>> +
>> + imx->sd.ctrl_handler = hdl;
>> +
>> + return 0;
>> +}
>> +
>> +/* Confirm the ID of the HW device */
>> +static int imx908_identify_model(struct imx908 *imx)
>> +{
>> + int ret;
>> + int err;
>> + u64 val;
>> + u16 chip_id;
>> +
>> + /*
>> + * The TYPE_ID registers are not accessible after power-up while
>> + * the device remains in standby. Exit standby and wait for the
>> + * required stabilization period before reading the chip ID.
>> + */
>> + ret = cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_CANCEL, NULL);
>> + if (ret)
>> + return ret;
>> +
>> + msleep(24); /* Regulator stabilization after standby cancel. */
>> +
>> + ret = cci_read(imx->cci, IMX908_REG_TYPE_ID, &val, &err);
>> + if (ret || err)
> + return ret ? ret : err;
>
>You don't need to check both ret and err. The err pointer to cci_read()
>(and cci_write()) is meant to chain calls with error checking at the
>end. Just check ret and pass NULL as the last argument to cci_read().
Understood.
>> +
>> + chip_id = val;
>> + dev_info(imx->dev, "IMX908 chip ID: 0x%04x\n", chip_id);
>
>This should be a dev_dbg() message, drivers should be silent at probe
>time when everything goes fine to avoid slowing down the boot process.
It will be dev_dbg in v2.
>> +
>> + if (chip_id != IMX908_CHIP_ID) {
>> + dev_err(imx->dev, "Unexpected chip ID 0x%04x (expected 0x%04x)\n",
>> + chip_id, IMX908_CHIP_ID);
>> + return -ENXIO;
>> + }
>> +
>> + /* Set to standby mode */
>> + ret = cci_write(imx->cci, IMX908_REG_STANDBY, IMX908_STANDBY_EN, NULL);
>> + if (ret)
>> + dev_err(imx->dev, "failed to enter standby state: %d\n", ret);
>
>This error shouldn't be ignored:
>
> return ret;
> }
Ok.
>> +
>> + return 0;
>> +}
>> +
>> +static int imx908_probe(struct i2c_client *client)
>> +{
>> + struct imx908 *imx;
>> + int ret;
>> +
>> + /* Allocate Memory */
>> + imx = devm_kzalloc(&client->dev, sizeof(*imx), GFP_KERNEL);
>> + if (!imx)
>> + return dev_err_probe(&client->dev, -ENOMEM,
>> + "failed to allocate IMX908 device structure\n");
>
>I would simply
>
> return -ENOMEM;
>
>here. kzalloc() failures are logged by the allocator.
Fixed per your suggestion in v2.
>> + imx->dev = &client->dev;
>> +
>> + /* Initialize V4L2 subdevice */
>> + v4l2_i2c_subdev_init(&imx->sd, client, &imx908_subdev_ops);
>> + imx->sd.internal_ops = &imx908_internal_ops;
>> +
>> + /* Register access initialization. Set 2-byte (16-bit) addresses */
>> + imx->cci = devm_cci_regmap_init_i2c(client, 16);
>> + if (IS_ERR(imx->cci))
>> + return dev_err_probe(&client->dev, PTR_ERR(imx->cci),
>> + "CCI regmap init failed\n");
>> +
>> + /* Get mandatory input clock from DT (INCK) */
>> + imx->xclk = devm_clk_get(imx->dev, "xclk");
>
>The DT bindings don't specify a clock name, use NULL instead of "xclk".
Ok.
>> + if (IS_ERR(imx->xclk))
>> + return dev_err_probe(imx->dev, PTR_ERR(imx->xclk), "xclk\n");
>
>That's a bit short as an error message.
>
> return dev_err_probe(imx->dev, PTR_ERR(imx->xclk),
> "failed to get clock\n");
I'm mostly too verbose but good to see I can swing both ways :-)
>> +
>> + /* Get clock frequency and check against acceptable HW values */
>> + imx->xclk_freq = clk_get_rate(imx->xclk);
>
>imx->xclk_freq is used in imx908_get_inck_sel() only. You can drop the
>field from the structure, and pass it as an argument to the function.
Done in v2.
>> + ret = imx908_get_inck_sel(imx);
>> + if (ret)
>> + return ret;
>> +
>> + /* GPIO reset acquisition */
>> + imx->reset_gpio = devm_gpiod_get_optional(imx->dev, "reset",
>> + GPIOD_OUT_HIGH);
>> +
>> + if (IS_ERR(imx->reset_gpio))
>> + return dev_err_probe(imx->dev, PTR_ERR(imx->reset_gpio),
>> + "reset gpio\n");
>
>Same here, "failed to get reset GPIO\n".
Ok.
>> +
>> + /* Link to power supplies */
>> + ret = imx908_get_regulators(imx);
>> + if (ret)
>> + return dev_err_probe(&client->dev, ret,
>> + "regulator get failed\n");
>
>And for consistency, "failed to get regulators\n". Same comment for the
>messages below.
Understood.