Re: [PATCH 2/2] media: i2c: Add Sony IMX908 image sensor driver

[Lachlan.Michael@xxxxxxxx]

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.