Re: [PATCH v7] s5k5baf: add camera sensor driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Andrzej,

Thank you for the patch.

On Wednesday 21 August 2013 16:41:31 Andrzej Hajda wrote:
> Driver for Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor
> with embedded SoC ISP.
> The driver exposes the sensor as two V4L2 subdevices:
> - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format,
>   no controls.
> - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200,
>   pre/post ISP cropping, downscaling via selection API, controls.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> Hi,
> 
> This patch incorporates Stephen's suggestions, thanks.
> 
> Regards
> Andrzej
> 
> v7
> - changed description of 'clock-frequency' DT property
> 
> v6
> - endpoint node presence is now optional,
> - added asynchronous subdev registration support and clock
>   handling,
> - use named gpios in DT bindings
> 
> v5
> - removed hflip/vflip device tree properties
> 
> v4
> - GPL changed to GPLv2,
> - bitfields replaced by u8,
> - cosmetic changes,
> - corrected s_stream flow,
> - gpio pins are no longer exported,
> - added I2C addresses to subdev names,
> - CIS subdev registration postponed after
>   succesfull HW initialization,
> - added enums for pads,
> - selections are initialized only during probe,
> - default resolution changed to 1600x1200,
> - state->error pattern removed from few other functions,
> - entity link creation moved to registered callback.
> 
> v3:
> - narrowed state->error usage to i2c and power errors,
> - private gain controls replaced by red/blue balance user controls,
> - added checks to devicetree gpio node parsing
> 
> v2:
> - lower-cased driver name,
> - removed underscore from regulator names,
> - removed platform data code,
> - v4l controls grouped in anonymous structs,
> - added s5k5baf_clear_error function,
> - private controls definitions moved to uapi header file,
> - added v4l2-controls.h reservation for private controls,
> - corrected subdev registered/unregistered code,
> - .log_status sudbev op set to v4l2 helper,
> - moved entity link creation to probe routines,
> - added cleanup on error to probe function.
> ---
>  .../devicetree/bindings/media/samsung-s5k5baf.txt  |   59 +
>  MAINTAINERS                                        |    7 +
>  drivers/media/i2c/Kconfig                          |    7 +
>  drivers/media/i2c/Makefile                         |    1 +
>  drivers/media/i2c/s5k5baf.c                        | 2045 +++++++++++++++++
>  5 files changed, 2119 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/media/samsung-s5k5baf.txt create mode
> 100644 drivers/media/i2c/s5k5baf.c

[snip]

> diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
> new file mode 100644
> index 0000000..f21d9f1
> --- /dev/null
> +++ b/drivers/media/i2c/s5k5baf.c

[snip]

> +enum s5k5baf_pads_id {
> +	PAD_CIS,
> +	PAD_OUT,
> +	CIS_PAD_NUM = 1,
> +	ISP_PAD_NUM = 2
> +};

You can just use #define's here, the enum doesn't bring any additional value 
and isn't very explicit.

> +static struct v4l2_rect s5k5baf_cis_rect = { 0, 0, S5K5BAF_CIS_WIDTH,
> +				     S5K5BAF_CIS_HEIGHT };

Shouldn't this be const ?

> +static u16 s5k5baf_i2c_read(struct s5k5baf *state, u16 addr)
> +{
> +	struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
> +	u16 w, r;

You should declare these variables as __be16.

> +	struct i2c_msg msg[] = {
> +		{ .addr = c->addr, .flags = 0,
> +		  .len = 2, .buf = (u8 *)&w },
> +		{ .addr = c->addr, .flags = I2C_M_RD,
> +		  .len = 2, .buf = (u8 *)&r },
> +	};
> +	int ret;
> +
> +	if (state->error)
> +		return 0;
> +
> +	w = htons(addr);

Wouldln't cpu_to_be16() be more appropriate ?

> +	ret = i2c_transfer(c->adapter, msg, 2);
> +	r = ntohs(r);

And be16_to_cpu() here.

> +
> +	v4l2_dbg(3, debug, c, "i2c_read: 0x%04x : 0x%04x\n", addr, r);
> +
> +	if (ret != 2) {
> +		v4l2_err(c, "i2c_read: error during transfer (%d)\n", ret);
> +		state->error = ret;
> +	}
> +	return r;
> +}

[snip]

> +static void s5k5baf_write_arr_seq(struct s5k5baf *state, u16 addr,
> +				  u16 count, const u16 *seq)
> +{
> +	struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
> +	u16 buf[count + 1];
> +	int ret, n;
> +
> +	s5k5baf_i2c_write(state, REG_CMDWR_ADDR, addr);
> +	if (state->error)
> +		return;

I would have a preference for returning an error directly from the write 
function instead of storing it in state->error, that would be more explicit. 
The same is true for all read/write functions.

> +	buf[0] = __constant_htons(REG_CMD_BUF);
> +	for (n = 1; n <= count; ++n)
> +		buf[n] = htons(*seq++);

cpu_to_be16()/be16_to_cpu() here as well ?

> +
> +	n *= 2;
> +	ret = i2c_master_send(c, (char *)buf, n);
> +	v4l2_dbg(3, debug, c, "i2c_write_seq(count=%d): %*ph\n", count,
> +		 min(2 * count, 64), seq - count);
> +
> +	if (ret != n) {
> +		v4l2_err(c, "i2c_write_seq: error during transfer (%d)\n", ret);
> +		state->error = ret;
> +	}
> +}

[snip]

> +static void s5k5baf_hw_set_ccm(struct s5k5baf *state)

A small comment explaining what this function configures would be nice.

> +{
> +	static const u16 nseq_cfg[] = {
> +		NSEQ(REG_PTR_CCM_HORIZON,
> +		REG_ARR_CCM(0), PAGE_IF_SW,
> +		REG_ARR_CCM(1), PAGE_IF_SW,
> +		REG_ARR_CCM(2), PAGE_IF_SW,
> +		REG_ARR_CCM(3), PAGE_IF_SW,
> +		REG_ARR_CCM(4), PAGE_IF_SW,
> +		REG_ARR_CCM(5), PAGE_IF_SW),
> +		NSEQ(REG_PTR_CCM_OUTDOOR,
> +		REG_ARR_CCM(6), PAGE_IF_SW),
> +		NSEQ(REG_ARR_CCM(0),
> +		/* horizon */
> +		0x010d, 0xffa7, 0xfff5, 0x003b, 0x00ef, 0xff38,
> +		0xfe42, 0x0270, 0xff71, 0xfeed, 0x0198, 0x0198,
> +		0xff95, 0xffa3, 0x0260, 0x00ec, 0xff33, 0x00f4,
> +		/* incandescent */
> +		0x010d, 0xffa7, 0xfff5, 0x003b, 0x00ef, 0xff38,
> +		0xfe42, 0x0270, 0xff71, 0xfeed, 0x0198, 0x0198,
> +		0xff95, 0xffa3, 0x0260, 0x00ec, 0xff33, 0x00f4,
> +		/* warm white */
> +		0x01ea, 0xffb9, 0xffdb, 0x0127, 0x0109, 0xff3c,
> +		0xff2b, 0x021b, 0xff48, 0xff03, 0x0207, 0x0113,
> +		0xffca, 0xff93, 0x016f, 0x0164, 0xff55, 0x0163,
> +		/* cool white */
> +		0x01ea, 0xffb9, 0xffdb, 0x0127, 0x0109, 0xff3c,
> +		0xff2b, 0x021b, 0xff48, 0xff03, 0x0207, 0x0113,
> +		0xffca, 0xff93, 0x016f, 0x0164, 0xff55, 0x0163,
> +		/* daylight 5000K */
> +		0x0194, 0xffad, 0xfffe, 0x00c5, 0x0103, 0xff5d,
> +		0xfee3, 0x01ae, 0xff27, 0xff18, 0x018f, 0x00c8,
> +		0xffe8, 0xffaa, 0x01c8, 0x0132, 0xff3e, 0x0100,
> +		/* daylight 6500K */
> +		0x0194, 0xffad, 0xfffe, 0x00c5, 0x0103, 0xff5d,
> +		0xfee3, 0x01ae, 0xff27, 0xff18, 0x018f, 0x00c8,
> +		0xffe8, 0xffaa, 0x01c8, 0x0132, 0xff3e, 0x0100,
> +		/* outdoor */
> +		0x01cc, 0xffc3, 0x0009, 0x00a2, 0x0106, 0xff3f,
> +		0xfed8, 0x01fe, 0xff08, 0xfec7, 0x00f5, 0x0119,
> +		0xffdf, 0x0024, 0x01a8, 0x0170, 0xffad, 0x011b),
> +		0
> +	};
> +	s5k5baf_write_nseq(state, nseq_cfg);
> +}
> +
> +static void s5k5baf_hw_set_cis(struct s5k5baf *state)

Same here.

> +{
> +	static const u16 nseq_cfg[] = {
> +		NSEQ(0xc202, 0x0700),
> +		NSEQ(0xf260, 0x0001),
> +		NSEQ(0xf414, 0x0030),
> +		NSEQ(0xc204, 0x0100),
> +		NSEQ(0xf402, 0x0092, 0x007f),
> +		NSEQ(0xf700, 0x0040),
> +		NSEQ(0xf708,
> +		0x0000, 0x0000, 0x0000, 0x0000, 0x0000,
> +		0x0040, 0x0040, 0x0040, 0x0040, 0x0040,
> +		0x0001, 0x0015, 0x0001, 0x0040),
> +		NSEQ(0xf48a, 0x0048),
> +		NSEQ(0xf10a, 0x008b),
> +		NSEQ(0xf900, 0x0067),
> +		NSEQ(0xf406, 0x0092, 0x007f, 0x0003, 0x0003, 0x0003),
> +		NSEQ(0xf442, 0x0000, 0x0000),
> +		NSEQ(0xf448, 0x0000),
> +		NSEQ(0xf456, 0x0001, 0x0010, 0x0000),
> +		NSEQ(0xf41a, 0x00ff, 0x0003, 0x0030),
> +		NSEQ(0xf410, 0x0001, 0x0000),
> +		NSEQ(0xf416, 0x0001),
> +		NSEQ(0xf424, 0x0000),
> +		NSEQ(0xf422, 0x0000),
> +		NSEQ(0xf41e, 0x0000),
> +		NSEQ(0xf428, 0x0000, 0x0000, 0x0000),
> +		NSEQ(0xf430, 0x0000, 0x0000, 0x0008, 0x0005, 0x000f, 0x0001,
> +		0x0040, 0x0040, 0x0010),
> +		NSEQ(0xf4d6, 0x0090, 0x0000),
> +		NSEQ(0xf47c, 0x000c, 0x0000),
> +		NSEQ(0xf49a, 0x0008, 0x0000),
> +		NSEQ(0xf4a2, 0x0008, 0x0000),
> +		NSEQ(0xf4b2, 0x0013, 0x0000, 0x0013, 0x0000),
> +		NSEQ(0xf4aa, 0x009b, 0x00fb, 0x009b, 0x00fb),
> +		0
> +	};
> +
> +	s5k5baf_i2c_write(state, REG_CMDWR_PAGE, PAGE_IF_HW);
> +	s5k5baf_write_nseq(state, nseq_cfg);
> +	s5k5baf_i2c_write(state, REG_CMDWR_PAGE, PAGE_IF_SW);
> +}

[snip]

> +/* Set auto/manual exposure and total gain */
> +static void s5k5baf_hw_set_auto_exposure(struct s5k5baf *state, int value)
> +{
> +	unsigned int exp_time = state->ctrls.exposure->val;
> +	u16 auto_alg;
> +
> +	auto_alg = s5k5baf_read(state, REG_DBG_AUTOALG_EN);

Wouldn't it be faster to cache the register value in the state structure 
instead of reading it back ? By the way, you might want to have a look at 
regmap.

> +
> +	if (value == V4L2_EXPOSURE_AUTO) {
> +		auto_alg |= AALG_AE_EN | AALG_DIVLEI_EN;
> +	} else {
> +		s5k5baf_hw_set_user_exposure(state, exp_time);
> +		s5k5baf_hw_set_user_gain(state, state->ctrls.gain->val);
> +		auto_alg &= ~(AALG_AE_EN | AALG_DIVLEI_EN);
> +	}
> +
> +	s5k5baf_write(state, REG_DBG_AUTOALG_EN, auto_alg);
> +}

> +static void s5k5baf_hw_set_colorfx(struct s5k5baf *state, int val)
> +{
> +	static const u16 colorfx[] = {
> +		[V4L2_COLORFX_NONE] = 0,
> +		[V4L2_COLORFX_BW] = 1,
> +		[V4L2_COLORFX_NEGATIVE] = 2,
> +		[V4L2_COLORFX_SEPIA] = 3,
> +		[V4L2_COLORFX_SKY_BLUE] = 4,
> +		[V4L2_COLORFX_SKETCH] = 5,
> +	};
> +
> +	if (val >= ARRAY_SIZE(colorfx)) {
> +		v4l2_err(&state->sd, "colorfx(%d) out of range(%d)\n",
> +			 val, ARRAY_SIZE(colorfx));
> +		state->error = -EINVAL;

Can this happen, given the range of admissible values for the control ?

> +	} else {
> +		s5k5baf_write(state, REG_G_SPEC_EFFECTS, colorfx[val]);
> +	}
> +}
> +
> +static int s5k5baf_find_pixfmt(struct v4l2_mbus_framefmt *mf)
> +{
> +	int i, c = -1;

I tend to use unsigned int for integer variables that store unsigned content 
(i in this case), but that might just be me.

> +
> +	for (i = 0; i < ARRAY_SIZE(s5k5baf_formats); i++) {
> +		if (mf->colorspace != s5k5baf_formats[i].colorspace)
> +			continue;
> +		if (mf->code == s5k5baf_formats[i].code)
> +			return i;
> +		if (c < 0)
> +			c = i;
> +	}
> +	return (c < 0) ? 0 : c;
> +}

[snip]

> +static int s5k5baf_hw_set_video_bus(struct s5k5baf *state)
> +{
> +	u16 en_packets;
> +
> +	switch (state->bus_type) {
> +	case V4L2_MBUS_CSI2:
> +		en_packets = EN_PACKETS_CSI2;
> +		break;
> +	case V4L2_MBUS_PARALLEL:
> +		en_packets = 0;
> +		break;
> +	default:
> +		v4l2_err(&state->sd, "unknown video bus: %d\n",
> +			 state->bus_type);
> +		return -EINVAL;

Can this happen ?

> +	};
> +
> +	s5k5baf_write_seq(state, REG_OIF_EN_MIPI_LANES,
> +			  state->nlanes, en_packets, 1);
> +
> +	return s5k5baf_clear_error(state);
> +}

[snip]

> +static int s5k5baf_s_stream(struct v4l2_subdev *sd, int on)
> +{
> +	struct s5k5baf *state = to_s5k5baf(sd);
> +	int ret;
> +
> +	if (state->streaming == !!on)
> +		return 0;
> +
> +	mutex_lock(&state->lock);

Shouldn't the lock protect the state->streaming check above ?

> +	if (on) {
> +		s5k5baf_hw_set_config(state);
> +		ret = s5k5baf_hw_set_crop_rects(state);
> +		if (ret < 0)
> +			goto out;
> +		s5k5baf_hw_set_stream(state, 1);
> +		s5k5baf_i2c_write(state, 0xb0cc, 0x000b);
> +	} else {
> +		s5k5baf_hw_set_stream(state, 0);
> +	}
> +	ret = s5k5baf_clear_error(state);
> +	if (!ret)
> +		state->streaming = !state->streaming;
> +
> +out:
> +	mutex_unlock(&state->lock);
> +
> +	return ret;
> +}

[snip]

> +/*
> + * V4L2 subdev internal operations
> + */
> +static int s5k5baf_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	struct v4l2_mbus_framefmt *mf;
> +
> +	mf = v4l2_subdev_get_try_format(fh, PAD_CIS);
> +	s5k5baf_try_cis_format(mf);
> +
> +	if (s5k5baf_is_cis_subdev(sd))
> +		return 0;

What about defining two open operations instead, one for each subdev ?

> +	mf = v4l2_subdev_get_try_format(fh, PAD_OUT);
> +	mf->colorspace = s5k5baf_formats[0].colorspace;
> +	mf->code = s5k5baf_formats[0].code;
> +	mf->width = s5k5baf_cis_rect.width;
> +	mf->height = s5k5baf_cis_rect.height;
> +	mf->field = V4L2_FIELD_NONE;
> +
> +	*v4l2_subdev_get_try_crop(fh, PAD_CIS) = s5k5baf_cis_rect;
> +	*v4l2_subdev_get_try_compose(fh, PAD_CIS) = s5k5baf_cis_rect;
> +	*v4l2_subdev_get_try_crop(fh, PAD_OUT) = s5k5baf_cis_rect;
> +
> +	return 0;
> +}
> +
> +static int s5k5baf_check_fw_revision(struct s5k5baf *state)
> +{
> +	u16 api_ver = 0, fw_rev = 0, s_id = 0;
> +	int ret;
> +
> +	api_ver = s5k5baf_read(state, REG_FW_APIVER);
> +	fw_rev = s5k5baf_read(state, REG_FW_REVISION) & 0xff;
> +	s_id = s5k5baf_read(state, REG_FW_SENSOR_ID);
> +	ret = s5k5baf_clear_error(state);
> +	if (ret < 0)
> +		return ret;
> +
> +	v4l2_info(&state->sd, "FW API=%#x, revision=%#x sensor_id=%#x\n",
> +		  api_ver, fw_rev, s_id);
> +
> +	if (api_ver == S5K5BAF_FW_APIVER)
> +		return 0;

I would have dealt with the error inside the if, but that's up to you.

> +	v4l2_err(&state->sd, "FW API version not supported\n");
> +	return -ENODEV;
> +}
> +
> +static int s5k5baf_registered(struct v4l2_subdev *sd)
> +{
> +	struct s5k5baf *state = to_s5k5baf(sd);
> +	int ret;
> +
> +	ret = v4l2_device_register_subdev(sd->v4l2_dev, &state->cis_sd);
> +	if (ret < 0)
> +		v4l2_err(sd, "failed to register subdev %s\n",
> +			 state->cis_sd.name);
> +	else
> +		ret = media_entity_create_link(&state->cis_sd.entity, PAD_CIS,
> +					       &state->sd.entity, PAD_CIS,
> +					       MEDIA_LNK_FL_IMMUTABLE |
> +					       MEDIA_LNK_FL_ENABLED);
> +	return ret;
> +}
> +
> +static void s5k5baf_unregistered(struct v4l2_subdev *sd)
> +{
> +	struct s5k5baf *state = to_s5k5baf(sd);
> +	v4l2_device_unregister_subdev(&state->cis_sd);

The unregistered operation is called from v4l2_device_unregister_subdev(). 
Calling it again will be a no-op, the function will return immediately. You 
can thus get rid of the unregistered operation completely.

Similarly, the registered operation is called from 
v4l2_device_register_subdev(). You can get rid of it as well and just create 
the link in the probe function.

> +}

[snip]

> +static int s5k5baf_parse_device_node(struct s5k5baf *state, struct device
> *dev) +{
> +	struct device_node *node = dev->of_node;
> +	struct device_node *node_ep;
> +	struct v4l2_of_endpoint ep;
> +	int ret;
> +
> +	if (!node) {
> +		dev_err(dev, "no device-tree node provided\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_property_read_u32(node, "clock-frequency",
> +				   &state->mclk_frequency);
> +	if (ret < 0) {
> +		state->mclk_frequency = S5K5BAF_DEFAULT_MCLK_FREQ;
> +		dev_info(dev, "using default %u Hz clock frequency\n",
> +			 state->mclk_frequency);
> +	}
> +
> +	ret = s5k5baf_parse_gpios(state->gpios, dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	node_ep = v4l2_of_get_next_endpoint(node, NULL);
> +	if (!node_ep) {
> +		/* Default data bus configuration: MIPI CSI-2, 1 data lane. */
> +		state->bus_type = V4L2_MBUS_CSI2;
> +		state->nlanes = S5K5BAF_DEF_NUM_LANES;
> +		dev_warn(dev, "no endpoint defined at node %s\n",
> +			 node->full_name);
> +		return 0;

Shouldn't this be a fatal error ? If there's no endpoint the sensor isn't part 
of any media pipeline, so it's unusable anyway.

> +	}
> +
> +	v4l2_of_parse_endpoint(node_ep, &ep);
> +	of_node_put(node_ep);
> +	state->bus_type = ep.bus_type;
> +
> +	if (state->bus_type == V4L2_MBUS_CSI2)
> +		state->nlanes = ep.bus.mipi_csi2.num_data_lanes;
> +
> +	return 0;
> +}

-- 
Regards,

Laurent Pinchart