[PATCH RFC] s5k5baf: add camera sensor driver

[Sylwester Nawrocki]

Hi Andrzej,

On 04/16/2013 03:38 PM, 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: Andrzej Hajda <a.hajda at samsung.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> ---

It's worth to note that this driver currently doesn't use the asynchronous 
sub-device probing API [1] but the intention is to switch to it once it's
settled.

[1] http://www.spinics.net/lists/linux-sh/msg18565.html

A few more comments below...

>  .../devicetree/bindings/media/samsung-s5k5baf.txt  |   50 +
>  MAINTAINERS                                        |    8 +
>  drivers/media/i2c/Kconfig                          |    7 +
>  drivers/media/i2c/Makefile                         |    1 +
>  drivers/media/i2c/s5k5baf.c                        | 1962 ++++++++++++++++++++
>  include/media/s5k5baf.h                            |   48 +
>  6 files changed, 2076 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
>  create mode 100644 drivers/media/i2c/s5k5baf.c
>  create mode 100644 include/media/s5k5baf.h
> 
> diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
> new file mode 100644
> index 0000000..1099c1d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
> @@ -0,0 +1,50 @@
> +Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor with embedded SoC ISP
> +-------------------------------------------------------------
> +
> +Required properties:
> +
> +- compatible	  : "samsung,s5k5baf";
> +- reg		  : i2c slave address of the sensor;
> +- vdda-supply	  : analog power supply 2.8V (2.6V to 3.0V);
> +- vdd_reg-supply  : regulator input power 1.8V (1.7V to 1.9V)

s/power/power supply ?

Underscores should be avoided in the regulator supply names AFAIK, so

s/vdd_reg/vddreg

> +                    or 2.8V (2.6V to 3.0);
> +- vddio-supply	  : I/O supply 1.8V (1.65V to 1.95V)

s/supply/power supply ?

> +                    or 2.8V (2.5V to 3.1V);
> +- gpios		  : standby and reset gpios;

You are not clear about the order, how about

- gpios	: GPIOs connected to STDBYN and RSTN pins, in order: STBYN, RSTN;

?
> +- clock-frequency : master clock frequency in Hz;
> +
> +Optional properties:
> +
> +- samsung,hflip	  : horizontal image flip
> +- samsung,vflip	  : vertical image flip

Optional clock properties are missing:

 clocks : contains the sensor's master clock specifier;
 clock-names : contains "mclk" entry;

MCLK happens to be the clock input pin name in the datasheet.

> +The device node should contain one 'port' child node with one child 'endpoint'
> +node, according to the bindings defined in Documentation/devicetree/bindings/
> +media/video-interfaces.txt. The following are properties specific to those nodes.
> +
> +endpoint node
> +-------------
> +
> +- data-lanes	  : (optional) an array specifying active physical MIPI-CSI2
> +		    data output lanes and their mapping to logical lanes; the
> +		    array's content is unused, only its length is meaningful;
> +
> +Example:
> +
> +s5k5bafx at 2d {
> +	compatible = "samsung,s5k5baf";
> +	reg = <0x2d>;
> +	vdda-supply = <&cam_io_en_reg>;
> +	vdd_reg-supply = <&vt_core_15v_reg>;
> +	vddio-supply = <&vtcam_reg>;
> +	gpios = <&gpl2 0 1>,
> +		<&gpl2 1 1>;
> +	clock-frequency = <24000000>;
> +
> +	port {
> +		s5k5bafx_ep: endpoint {
> +			remote-endpoint = <&csis1_ep>;
> +			data-lanes = <1>;
> +		};
> +	};
> +};

> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 9e7ce8b..e487f7d 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -553,6 +553,13 @@ config VIDEO_S5K4ECGX
>            This is a V4L2 sensor-level driver for Samsung S5K4ECGX 5M
>            camera sensor with an embedded SoC image signal processor.
>  
> +config VIDEO_S5K5BAF
> +	tristate "Samsung S5K5BAF sensor support"
> +	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> +	---help---
> +	  This is a V4L2 sensor-level driver for Samsung S5K5BAF 2M
> +	  camera sensor with an embedded SoC image signal processor.
> +
>  source "drivers/media/i2c/smiapp/Kconfig"
>  
>  config VIDEO_S5C73M3

> diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
> new file mode 100644
> index 0000000..0bae2d3
> --- /dev/null
> +++ b/drivers/media/i2c/s5k5baf.c
> @@ -0,0 +1,1962 @@
> +/*
> + * Driver for Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor
> + * with embedded SoC ISP.
> + *
> + * Copyright (C) 2013, Samsung Electronics Co., Ltd.
> + * Andrzej Hajda <a.hajda at samsung.com>
> + *
> + * Based on S5K6AA driver authored by Sylwester Nawrocki
> + * Sylwester Nawrocki <s.nawrocki at samsung.com>

I would rephrase it to:

  + * Based on S5K6AA driver authored by Sylwester Nawrocki
  + * Copyright (C) 2013, Samsung Electronics Co., Ltd.

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */

> +
> +static const char * const s5k5baf_supply_names[] = {
> +	"vdda",		/* Analog power supply 2.8V (2.6V to 3.0V) */
> +	"vdd_reg",	/* Regulator input power 1.8V (1.7V to 1.9V)

"vddreg"

> +			   or 2.8V (2.6V to 3.0) */
> +	"vddio",	/* I/O supply 1.8V (1.65V to 1.95V)
> +			   or 2.8V (2.5V to 3.1V) */
> +};
> +#define S5K5BAF_NUM_SUPPLIES ARRAY_SIZE(s5k5baf_supply_names)

> +struct s5k5baf_ctrls {
> +	struct v4l2_ctrl_handler handler;
> +	/* Auto / manual white balance cluster */
> +	struct v4l2_ctrl *awb;
> +	struct v4l2_ctrl *gain_red;
> +	struct v4l2_ctrl *gain_blue;
> +	struct v4l2_ctrl *gain_green;
> +	/* Mirror cluster */
> +	struct v4l2_ctrl *hflip;
> +	struct v4l2_ctrl *vflip;
> +	/* Auto exposure / manual exposure and gain cluster */
> +	struct v4l2_ctrl *auto_exp;
> +	struct v4l2_ctrl *exposure;
> +	struct v4l2_ctrl *gain;

Let's put each cluster in separate anonymous struct, so e.g.

+	struct { /* Auto exposure / manual exposure and gain cluster */
+		struct v4l2_ctrl *auto_exp;
+		struct v4l2_ctrl *exposure;
+		struct v4l2_ctrl *gain;
+	};

> +};
> +

> +/*
> + * V4L2 subdev controls
> + */

> +static int s5k5baf_log_status(struct v4l2_subdev *sd)
> +{
> +	v4l2_ctrl_handler_log_status(sd->ctrl_handler, sd->name);
> +	return 0;
> +}

This function is not needed, you could use now v4l2_ctrl_subdev_log_status()
directly.

> +#define V4L2_CID_RED_GAIN	(V4L2_CTRL_CLASS_CAMERA | 0x1001)
> +#define V4L2_CID_GREEN_GAIN	(V4L2_CTRL_CLASS_CAMERA | 0x1002)
> +#define V4L2_CID_BLUE_GAIN	(V4L2_CTRL_CLASS_CAMERA | 0x1003)

A range should be reserved for the private controls in 
include/uapi/linux/v4l2-controls.h and used as a base here, so control
IDs of various drivers are not overlapping.

> +/*
> + * V4L2 subdev internal operations
> + */

> +static void s5k5baf_check_fw_revision(struct s5k5baf *state)
> +{
> +	u16 api_ver = 0, fw_rev = 0, s_id = 0;
> +
> +	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);
> +	if (state->error)
> +		return;
> +
> +	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;
> +
> +	v4l2_err(&state->sd, "FW API version not supported\n");
> +	state->error = -ENODEV;

When we initially discussed it in private my intention was to use 
'state->error' mainly to ease handling of multiple I2C write calls. 
I have a feeling that the code would be easier to follow if it would
be returning here the error value explicitly.

> +}
> +
> +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) {
> +		v4l2_err(sd, "failed to register subdev %s\n",
> +			 state->cis_sd.name);
> +		return ret;
> +	}
> +
> +	ret = media_entity_create_link(&state->cis_sd.entity,
> +			0, &state->sd.entity, 0,
> +			MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> +
> +	mutex_lock(&state->lock);
> +
> +	s5k5baf_power_on(state);
> +	s5k5baf_hw_init(state);
> +	s5k5baf_check_fw_revision(state);
> +	s5k5baf_power_off(state);
> +
> +	ret = state->error;
> +	state->error = 0;

Probably makes sense to create a helper function that would get and
clear state->error, e.g. s5k5baf_error_get_clear() ?

> +
> +	mutex_unlock(&state->lock);
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_subdev_ops s5k5baf_cis_subdev_ops = {
> +	.pad	= &s5k5baf_cis_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops s5k5baf_cis_subdev_internal_ops = {
> +	.open = s5k5baf_open,
> +};
> +
> +static const struct v4l2_subdev_internal_ops s5k5baf_subdev_internal_ops = {
> +	.registered = s5k5baf_registered,

The 'unregistered' callback is missing, it should undo anything what's 
done in s5k5baf_registered() or s5k5baf_registered() should be protected 
against multiple calls.

> +	.open = s5k5baf_open,
> +};
> +
> +static const struct v4l2_subdev_core_ops s5k5baf_core_ops = {
> +	.s_power = s5k5baf_set_power,
> +	.log_status = s5k5baf_log_status,

	.log_status = v4l2_ctrl_subdev_log_status,

> +};
> +
> +static const struct v4l2_subdev_ops s5k5baf_subdev_ops = {
> +	.core = &s5k5baf_core_ops,
> +	.pad = &s5k5baf_pad_ops,
> +	.video = &s5k5baf_video_ops,
> +};
> +
> +static void s5k5baf_configure_gpios(struct s5k5baf *state)
> +{
> +	static const char const *name[] = { "S5K5BAF_STBY", "S5K5BAF_RST" };
> +	struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
> +	struct s5k5baf_gpio *g = state->pdata.gpios;
> +	int ret, i;
> +
> +	if (state->error)
> +		return;
> +
> +	for (i = 0; i < GPIO_NUM; ++i) {
> +		int flags = GPIOF_EXPORT | GPIOF_DIR_OUT;
> +		if (g[i].level)
> +			flags |= GPIOF_INIT_HIGH;
> +		ret = devm_gpio_request_one(&c->dev, g[i].gpio, flags, name[i]);
> +		if (ret) {
> +			v4l2_err(c, "failed to request gpio %s\n", name[i]);
> +			state->error = ret;
> +			return;
> +		}
> +	}
> +	return;

Not needed, but anyway would be better to change the return value type to 'int'.

> +}
> +
> +static void s5k5baf_get_platform_data(struct s5k5baf *state, struct device *dev)
> +{
> +	struct device_node *node = dev->of_node;
> +	struct s5k5baf_platform_data *pd;
> +	struct device_node *node_ep;
> +	struct v4l2_of_endpoint ep;
> +	enum of_gpio_flags of_flags;
> +
> +	if (state->error)
> +		return;
> +
> +	if (!node) {
> +		pd = dev->platform_data;
> +		if (!pd) {
> +			dev_err(dev, "No platform data\n");
> +			state->error = -EINVAL;
> +		}
> +		state->pdata = *pd;
> +		return;
> +	}
> +
> +	pd = &state->pdata;
> +
> +	of_property_read_u32(node, "clock-frequency", &pd->mclk_frequency);
> +	pd->hflip = of_property_read_bool(node, "samsung,hflip");
> +	pd->vflip = of_property_read_bool(node, "samsung,vflip");
> +	pd->gpios[STBY].gpio = of_get_gpio_flags(node, STBY, &of_flags);
> +	pd->gpios[STBY].level = !(of_flags & OF_GPIO_ACTIVE_LOW);
> +	pd->gpios[RST].gpio = of_get_gpio_flags(node, RST, &of_flags);
> +	pd->gpios[RST].level = !(of_flags & OF_GPIO_ACTIVE_LOW);
> +
> +	node_ep = v4l2_of_get_next_endpoint(node, NULL);
> +	if (!node_ep) {
> +		dev_err(dev, "no endpoint defined\n");
> +		state->error = -EINVAL;
> +		return;
> +	}
> +	v4l2_of_parse_endpoint(node_ep, &ep);
> +	of_node_put(node_ep);
> +	pd->bus_type = ep.bus_type;
> +	if (pd->bus_type == V4L2_MBUS_CSI2)
> +		pd->nlanes = ep.bus.mipi_csi2.num_data_lanes;
> +}
> +
> +static void s5k5baf_configure_subdevs(struct s5k5baf *state,
> +				     struct i2c_client *c)
> +{
> +	struct v4l2_subdev *sd;
> +
> +	if (state->error)
> +		return;
> +
> +	sd = &state->cis_sd;
> +	v4l2_subdev_init(sd, &s5k5baf_cis_subdev_ops);
> +	sd->owner = c->driver->driver.owner;
> +	v4l2_set_subdevdata(sd, state);
> +	strlcpy(sd->name, "S5K5BAF-CIS", sizeof(sd->name));
> +
> +	sd->internal_ops = &s5k5baf_cis_subdev_internal_ops;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +
> +	state->cis_pad.flags = MEDIA_PAD_FL_SOURCE;
> +	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
> +	state->error = media_entity_init(&sd->entity, 1, &state->cis_pad, 0);
> +	if (state->error)
> +		goto err;
> +
> +	sd = &state->sd;
> +	v4l2_i2c_subdev_init(sd, c, &s5k5baf_subdev_ops);
> +	strlcpy(sd->name, "S5K5BAF-ISP", sizeof(sd->name));
> +
> +	sd->internal_ops = &s5k5baf_subdev_internal_ops;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +
> +	state->pads[0].flags = MEDIA_PAD_FL_SINK;
> +	state->pads[1].flags = MEDIA_PAD_FL_SOURCE;
> +	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV;
> +	state->error = media_entity_init(&sd->entity, 2, state->pads, 0);
> +	if (!state->error)
> +		return;
> +
> +	media_entity_cleanup(&state->cis_sd.entity);
> +err:
> +	dev_err(&c->dev, "cannot init media entity %s\n", sd->name);

Not sure if this error log is needed. Might be better to log this
upon return from this function, so 'goto' can be avoided.

> +}
> +
> +static void s5k5baf_configure_regulators(struct s5k5baf *state)
> +{
> +	struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
> +	int i;
> +
> +	if (state->error)
> +		return;
> +
> +	for (i = 0; i < S5K5BAF_NUM_SUPPLIES; i++)
> +		state->supplies[i].supply = s5k5baf_supply_names[i];
> +
> +	state->error = devm_regulator_bulk_get(&c->dev, S5K5BAF_NUM_SUPPLIES,
> +				      state->supplies);
> +	if (state->error)
> +		v4l2_err(c, "failed to get regulators\n");
> +}
> +
> +static int s5k5baf_probe(struct i2c_client *c,
> +			const struct i2c_device_id *id)
> +{
> +	struct s5k5baf *state;
> +
> +	state = devm_kzalloc(&c->dev, sizeof(*state), GFP_KERNEL);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	s5k5baf_get_platform_data(state, &c->dev);
> +	s5k5baf_configure_subdevs(state, c);
> +	s5k5baf_configure_gpios(state);
> +	s5k5baf_configure_regulators(state);
> +	s5k5baf_initialize_ctrls(state);
> +
> +	mutex_init(&state->lock);

Might make sense to do this as one of first steps, right after the 
private driver data structure allocation.

Some cleanup steps are missing here on the error paths, e.g.
media_entity_cleanup(&sd->entity);

What about changing the return value of all above functions used in 
probe() to 'int' so we can better track the error paths ?

> +	return state->error;
> +}
> +
> +static int s5k5baf_remove(struct i2c_client *c)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(c);
> +	struct s5k5baf *state = to_s5k5baf(sd);
> +
> +	v4l2_device_unregister_subdev(sd);
> +	v4l2_ctrl_handler_free(sd->ctrl_handler);
> +	media_entity_cleanup(&sd->entity);
> +
> +	sd = &state->cis_sd;
> +	v4l2_device_unregister_subdev(sd);
> +	media_entity_cleanup(&sd->entity);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id s5k5baf_id[] = {
> +	{ DRIVER_NAME, 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, s5k5baf_id);
> +
> +static const struct of_device_id s5k5baf_of_match[] = {
> +	{ .compatible = "samsung," DRIVER_NAME },

Hmm, it looks a bit obfuscated to me, how about writing whole compatible 
string explicitly ?

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, s5k5baf_of_match);
> +
> +static struct i2c_driver s5k5baf_i2c_driver = {
> +	.driver = {
> +		.of_match_table = s5k5baf_of_match,
> +		.name = DRIVER_NAME
> +	},
> +	.probe		= s5k5baf_probe,
> +	.remove		= s5k5baf_remove,
> +	.id_table	= s5k5baf_id,
> +};
> +
> +module_i2c_driver(s5k5baf_i2c_driver);
> +
> +MODULE_DESCRIPTION("Samsung S5K5BAF(X) UXGA camera driver");
> +MODULE_AUTHOR("Andrzej Hajda <a.hajda at samsung.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/media/s5k5baf.h b/include/media/s5k5baf.h
> new file mode 100644
> index 0000000..957e708
> --- /dev/null
> +++ b/include/media/s5k5baf.h
> @@ -0,0 +1,48 @@
> +/*
> + * S5K5BAF camera sensor driver header
> + *
> + * Copyright (C) 2011 Samsung Electronics Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef S5K5BAF_H
> +#define S5K5BAF_H
> +
> +#include <media/v4l2-mediabus.h>
> +
> +/**
> + * struct s5k5baf_gpio - data structure describing a GPIO
> + * @gpio:  GPIO number
> + * @level: indicates active state of the @gpio
> + */
> +struct s5k5baf_gpio {
> +	int gpio;
> +	int level;
> +};
> +
> +/**
> + * struct s5k5baf_platform_data - s5k5baf driver platform data
> + * @set_power:   an additional callback to the board code, called
> + *               after enabling the regulators and before switching
> + *               the sensor off
> + * @mclk_frequency: sensor's master clock frequency in Hz
> + * @gpios:       GPIOs driving pins: STBY, RESET
> + * @nlanes:      maximum number of MIPI-CSI lanes used
> + * @hflip:       default horizontal image flip value, non zero to enable
> + * @vflip:       default vertical image flip value, non zero to enable
> + */
> +
> +struct s5k5baf_platform_data {
> +	u32 mclk_frequency;
> +	struct s5k5baf_gpio gpios[2];
> +	enum v4l2_mbus_type bus_type;
> +	u8 nlanes;
> +	u8 hflip:1;
> +	u8 vflip:1;
> +};
> +
> +#endif /* S5K5BAF_H */

Since we are not going to be using platform_data, I think this whole
header file could be removed for now. Let's not add a dead code. If it
happens someone ever needs platform_data I think it can be added then.


Thanks,
Sylwester