RE: [PATCH v8] media: imx258: Add imx258 camera sensor driver

["Yeh, Andy" <andy.yeh@xxxxxxxxx>]

Both comments are OKAY.

Thanks.

-----Original Message-----
From: Sakari Ailus [mailto:sakari.ailus@xxxxxxxxxxxxxxx] 
Sent: Thursday, March 15, 2018 6:31 AM
To: Yeh, Andy <andy.yeh@xxxxxxxxx>
Cc: linux-media@xxxxxxxxxxxxxxx; tfiga@xxxxxxxxxxxx; Chen, JasonX Z <jasonx.z.chen@xxxxxxxxx>; Chiang, AlanX <alanx.chiang@xxxxxxxxx>; Lai, Jim <jim.lai@xxxxxxxxx>
Subject: Re: [PATCH v8] media: imx258: Add imx258 camera sensor driver

Hi Andy,

Thanks for the update. Two minor comments below.

On Thu, Mar 15, 2018 at 12:24:19AM +0800, Andy Yeh wrote:
...
> +static int imx258_set_ctrl(struct v4l2_ctrl *ctrl) {
> +	struct imx258 *imx258 =
> +		container_of(ctrl->handler, struct imx258, ctrl_handler);
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> +	int ret = 0;
> +
> +	/*
> +	 * Applying V4L2 control value only happens
> +	 * when power is up for streaming
> +	 */
> +	if (pm_runtime_get_if_in_use(&client->dev) == 0)
> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_ANALOGUE_GAIN:
> +		ret = imx258_write_reg(imx258, IMX258_REG_ANALOG_GAIN,
> +				IMX258_REG_VALUE_16BIT,
> +				ctrl->val);
> +		break;
> +	case V4L2_CID_EXPOSURE:
> +		ret = imx258_write_reg(imx258, IMX258_REG_EXPOSURE,
> +				IMX258_REG_VALUE_16BIT,
> +				ctrl->val);
> +		break;
> +	case V4L2_CID_DIGITAL_GAIN:
> +		ret = imx258_update_digital_gain(imx258, IMX258_REG_VALUE_16BIT,
> +				ctrl->val);
> +		break;
> +	case V4L2_CID_VBLANK:
> +		/*
> +		 * Auto Frame Length Line Control is enabled by default.
> +		 * Not need control Vblank Register.
> +		 */
> +		break;
> +	default:
> +		dev_info(&client->dev,
> +			 "ctrl(id:0x%x,val:0x%x) is not handled\n",
> +			 ctrl->id, ctrl->val);

As this is an error, I'd set ret to e.g. -EINVAL here.


> +		break;
> +	}
> +
> +	pm_runtime_put(&client->dev);
> +
> +	return ret;
> +}

...

> +/* Initialize control handlers */
> +static int imx258_init_controls(struct imx258 *imx258) {
> +	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> +	struct v4l2_ctrl_handler *ctrl_hdlr;
> +	s64 exposure_max;
> +	s64 vblank_def;
> +	s64 vblank_min;
> +	s64 pixel_rate_min;
> +	s64 pixel_rate_max;
> +	int ret;
> +
> +	ctrl_hdlr = &imx258->ctrl_handler;
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> +	if (ret)
> +		return ret;
> +
> +	mutex_init(&imx258->mutex);
> +	ctrl_hdlr->lock = &imx258->mutex;
> +	imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> +				&imx258_ctrl_ops,
> +				V4L2_CID_LINK_FREQ,
> +				ARRAY_SIZE(link_freq_menu_items) - 1,
> +				0,
> +				link_freq_menu_items);
> +
> +	if (imx258->link_freq)
> +		imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
> +	pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
> +	/* By default, PIXEL_RATE is read only */
> +	imx258->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> +					V4L2_CID_PIXEL_RATE,
> +					pixel_rate_min, pixel_rate_max,
> +					1, pixel_rate_max);
> +
> +
> +	vblank_def = imx258->cur_mode->vts_def - imx258->cur_mode->height;
> +	vblank_min = imx258->cur_mode->vts_min - imx258->cur_mode->height;
> +	imx258->vblank = v4l2_ctrl_new_std(
> +				ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_VBLANK,
> +				vblank_min,
> +				IMX258_VTS_MAX - imx258->cur_mode->height, 1,
> +				vblank_def);
> +
> +	imx258->hblank = v4l2_ctrl_new_std(
> +				ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_HBLANK,
> +				IMX258_PPL_DEFAULT - imx258->cur_mode->width,
> +				IMX258_PPL_DEFAULT - imx258->cur_mode->width,
> +				1,
> +				IMX258_PPL_DEFAULT - imx258->cur_mode->width);
> +
> +	if (!imx258->hblank) {

Could you align handling for NULL hblank control with NULL link_freq above?

> +		ret = -EINVAL;
> +		goto error;
> +	}
> +
> +	imx258->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	exposure_max = imx258->cur_mode->vts_def - 8;
> +	imx258->exposure = v4l2_ctrl_new_std(
> +				ctrl_hdlr, &imx258_ctrl_ops,
> +				V4L2_CID_EXPOSURE, IMX258_EXPOSURE_MIN,
> +				IMX258_EXPOSURE_MAX, IMX258_EXPOSURE_STEP,
> +				IMX258_EXPOSURE_DEFAULT);
> +
> +	v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> +				IMX258_ANA_GAIN_MIN, IMX258_ANA_GAIN_MAX,
> +				IMX258_ANA_GAIN_STEP, IMX258_ANA_GAIN_DEFAULT);
> +
> +	v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> +				IMX258_DGTL_GAIN_MIN, IMX258_DGTL_GAIN_MAX,
> +				IMX258_DGTL_GAIN_STEP,
> +				IMX258_DGTL_GAIN_DEFAULT);
> +
> +	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx258_ctrl_ops,
> +				     V4L2_CID_TEST_PATTERN,
> +				     ARRAY_SIZE(imx258_test_pattern_menu) - 1,
> +				     0, 0, imx258_test_pattern_menu);
> +
> +	if (ctrl_hdlr->error) {
> +		ret = ctrl_hdlr->error;
> +		dev_err(&client->dev, "%s control init failed (%d)\n",
> +			__func__, ret);
> +		goto error;
> +	}
> +
> +	imx258->sd.ctrl_handler = ctrl_hdlr;
> +
> +	return 0;
> +
> +error:
> +	v4l2_ctrl_handler_free(ctrl_hdlr);
> +	mutex_destroy(&imx258->mutex);
> +
> +	return ret;
> +}

--
Kind regards,

Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx