All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v7] s5k5baf: add camera sensor driver
@ 2013-08-21 14:41 Andrzej Hajda
  2013-08-22 20:01 ` Stephen Warren
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Andrzej Hajda @ 2013-08-21 14:41 UTC (permalink / raw)
  To: linux-media
  Cc: Andrzej Hajda, laurent.pinchart, linux-samsung-soc, devicetree,
	Sylwester Nawrocki, Kyungmin Park, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Grant Likely

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

diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
new file mode 100644
index 0000000..d680d99
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
@@ -0,0 +1,59 @@
+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);
+- vddreg-supply	  : regulator input power supply 1.8V (1.7V to 1.9V)
+		    or 2.8V (2.6V to 3.0);
+- vddio-supply	  : I/O power supply 1.8V (1.65V to 1.95V)
+		    or 2.8V (2.5V to 3.1V);
+- stbyn-gpios	  : GPIO connected to STDBYN pin;
+- rstn-gpios	  : GPIO connected to RSTN pin;
+- clocks	  : the sensor's master clock specifier (from the common
+		    clock bindings);
+- clock-names	  : must be "mclk";
+
+Optional properties:
+
+- clock-frequency : the frequency at which the "mclk" clock should be
+		    configured to operate, in Hz; if this property is not
+		    specified default 24 MHz value will be used.
+
+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) specifies MIPI CSI-2 data lanes as covered in
+  video-interfaces.txt. This property can be only used to specify number
+  of data lanes, i.e. the array's content is unused, only its length is
+  meaningful. When this property is not specified default value of 1 lane
+  will be used.
+
+Example:
+
+s5k5bafx@2d {
+	compatible = "samsung,s5k5baf";
+	reg = <0x2d>;
+	vdda-supply = <&cam_io_en_reg>;
+	vddreg-supply = <&vt_core_15v_reg>;
+	vddio-supply = <&vtcam_reg>;
+	stbyn-gpios = <&gpl2 0 1>;
+	rstn-gpios = <&gpl2 1 1>;
+	clock-names = "mclk";
+	clocks = <&clock_cam 0>;
+	clock-frequency = <24000000>;
+
+	port {
+		s5k5bafx_ep: endpoint {
+			remote-endpoint = <&csis1_ep>;
+			data-lanes = <1>;
+		};
+	};
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index bf61e04..3d5ec58 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7103,6 +7103,13 @@ L:	linux-media@vger.kernel.org
 S:	Supported
 F:	drivers/media/i2c/s5c73m3/*
 
+SAMSUNG S5K5BAF CAMERA DRIVER
+M:	Kyungmin Park <kyungmin.park@samsung.com>
+M:	Andrzej Hajda <a.hajda@samsung.com>
+L:	linux-media@vger.kernel.org
+S:	Supported
+F:	drivers/media/i2c/s5k5baf.c
+
 SERIAL DRIVERS
 M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
 L:	linux-serial@vger.kernel.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index b2cd8ca..3367ec2 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -571,6 +571,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/Makefile b/drivers/media/i2c/Makefile
index dc20653..d590925 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_VIDEO_SR030PC30)	+= sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)	+= noon010pc30.o
 obj-$(CONFIG_VIDEO_S5K6AA)	+= s5k6aa.o
 obj-$(CONFIG_VIDEO_S5K4ECGX)	+= s5k4ecgx.o
+obj-$(CONFIG_VIDEO_S5K5BAF)	+= s5k5baf.o
 obj-$(CONFIG_VIDEO_S5C73M3)	+= s5c73m3/
 obj-$(CONFIG_VIDEO_ADP1653)	+= adp1653.o
 obj-$(CONFIG_VIDEO_AS3645A)	+= as3645a.o
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
@@ -0,0 +1,2045 @@
+/*
+ * 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@samsung.com>
+ *
+ * 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 version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/media.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+#include <media/media-entity.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+#include <media/v4l2-mediabus.h>
+#include <media/v4l2-of.h>
+
+static int debug;
+module_param(debug, int, 0644);
+
+#define S5K5BAF_DRIVER_NAME		"s5k5baf"
+#define S5K5BAF_DEFAULT_MCLK_FREQ	24000000U
+#define S5K5BAF_CLK_NAME		"mclk"
+
+#define S5K5BAF_CIS_WIDTH		1600
+#define S5K5BAF_CIS_HEIGHT		1200
+#define S5K5BAF_WIN_WIDTH_MIN		8
+#define S5K5BAF_WIN_HEIGHT_MIN		8
+#define S5K5BAF_GAIN_RED_DEF		127
+#define S5K5BAF_GAIN_GREEN_DEF		95
+#define S5K5BAF_GAIN_BLUE_DEF		180
+/* Default number of MIPI CSI-2 data lanes used */
+#define S5K5BAF_DEF_NUM_LANES		1
+
+#define AHB_MSB_ADDR_PTR		0xfcfc
+
+/*
+ * Register interface pages (the most significant word of the address)
+ */
+#define PAGE_IF_HW			0xd000
+#define PAGE_IF_SW			0x7000
+
+/*
+ * H/W register Interface (PAGE_IF_HW)
+ */
+#define REG_SW_LOAD_COMPLETE		0x0014
+#define REG_CMDWR_PAGE			0x0028
+#define REG_CMDWR_ADDR			0x002a
+#define REG_CMDRD_PAGE			0x002c
+#define REG_CMDRD_ADDR			0x002e
+#define REG_CMD_BUF			0x0f12
+#define REG_SET_HOST_INT		0x1000
+#define REG_CLEAR_HOST_INT		0x1030
+#define REG_PATTERN_SET			0x3100
+#define REG_PATTERN_WIDTH		0x3118
+#define REG_PATTERN_HEIGHT		0x311a
+#define REG_PATTERN_PARAM		0x311c
+
+/*
+ * S/W register interface (PAGE_IF_SW)
+ */
+
+/* Firmware revision information */
+#define REG_FW_APIVER			0x012e
+#define  S5K5BAF_FW_APIVER		0x0001
+#define REG_FW_REVISION			0x0130
+#define REG_FW_SENSOR_ID		0x0152
+
+/* Initialization parameters */
+/* Master clock frequency in KHz */
+#define REG_I_INCLK_FREQ_L		0x01b8
+#define REG_I_INCLK_FREQ_H		0x01ba
+#define  MIN_MCLK_FREQ_KHZ		6000U
+#define  MAX_MCLK_FREQ_KHZ		48000U
+#define REG_I_USE_NPVI_CLOCKS		0x01c6
+#define  NPVI_CLOCKS			1
+#define REG_I_USE_NMIPI_CLOCKS		0x01c8
+#define  NMIPI_CLOCKS			1
+#define REG_I_BLOCK_INTERNAL_PLL_CALC	0x01ca
+
+/* Clock configurations, n = 0..2. REG_I_* frequency unit is 4 kHz. */
+#define REG_I_OPCLK_4KHZ(n)		((n) * 6 + 0x01cc)
+#define REG_I_MIN_OUTRATE_4KHZ(n)	((n) * 6 + 0x01ce)
+#define REG_I_MAX_OUTRATE_4KHZ(n)	((n) * 6 + 0x01d0)
+#define  SCLK_PVI_FREQ			24000
+#define  SCLK_MIPI_FREQ			48000
+#define  PCLK_MIN_FREQ			6000
+#define  PCLK_MAX_FREQ			48000
+#define REG_I_USE_REGS_API		0x01de
+#define REG_I_INIT_PARAMS_UPDATED	0x01e0
+#define REG_I_ERROR_INFO		0x01e2
+
+/* General purpose parameters */
+#define REG_USER_BRIGHTNESS		0x01e4
+#define REG_USER_CONTRAST		0x01e6
+#define REG_USER_SATURATION		0x01e8
+#define REG_USER_SHARPBLUR		0x01ea
+
+#define REG_G_SPEC_EFFECTS		0x01ee
+#define REG_G_ENABLE_PREV		0x01f0
+#define REG_G_ENABLE_PREV_CHG		0x01f2
+#define REG_G_NEW_CFG_SYNC		0x01f8
+#define REG_G_PREVREQ_IN_WIDTH		0x01fa
+#define REG_G_PREVREQ_IN_HEIGHT		0x01fc
+#define REG_G_PREVREQ_IN_XOFFS		0x01fe
+#define REG_G_PREVREQ_IN_YOFFS		0x0200
+#define REG_G_PREVZOOM_IN_WIDTH		0x020a
+#define REG_G_PREVZOOM_IN_HEIGHT	0x020c
+#define REG_G_PREVZOOM_IN_XOFFS		0x020e
+#define REG_G_PREVZOOM_IN_YOFFS		0x0210
+#define REG_G_INPUTS_CHANGE_REQ		0x021a
+#define REG_G_ACTIVE_PREV_CFG		0x021c
+#define REG_G_PREV_CFG_CHG		0x021e
+#define REG_G_PREV_OPEN_AFTER_CH	0x0220
+#define REG_G_PREV_CFG_ERROR		0x0222
+#define  CFG_ERROR_RANGE		0x0b
+#define REG_G_PREV_CFG_BYPASS_CHANGED	0x022a
+#define REG_G_ACTUAL_P_FR_TIME		0x023a
+#define REG_G_ACTUAL_P_OUT_RATE		0x023c
+#define REG_G_ACTUAL_C_FR_TIME		0x023e
+#define REG_G_ACTUAL_C_OUT_RATE		0x0240
+
+/* Preview control section. n = 0...4. */
+#define PREG(n, x)			((n) * 0x26 + x)
+#define REG_P_OUT_WIDTH(n)		PREG(n, 0x0242)
+#define REG_P_OUT_HEIGHT(n)		PREG(n, 0x0244)
+#define REG_P_FMT(n)			PREG(n, 0x0246)
+#define REG_P_MAX_OUT_RATE(n)		PREG(n, 0x0248)
+#define REG_P_MIN_OUT_RATE(n)		PREG(n, 0x024a)
+#define REG_P_PVI_MASK(n)		PREG(n, 0x024c)
+#define  PVI_MASK_MIPI			0x52
+#define REG_P_CLK_INDEX(n)		PREG(n, 0x024e)
+#define  CLK_PVI_INDEX			0
+#define  CLK_MIPI_INDEX			NPVI_CLOCKS
+#define REG_P_FR_RATE_TYPE(n)		PREG(n, 0x0250)
+#define  FR_RATE_DYNAMIC		0
+#define  FR_RATE_FIXED			1
+#define  FR_RATE_FIXED_ACCURATE		2
+#define REG_P_FR_RATE_Q_TYPE(n)		PREG(n, 0x0252)
+#define  FR_RATE_Q_DYNAMIC		0
+#define  FR_RATE_Q_BEST_FRRATE		1 /* Binning enabled */
+#define  FR_RATE_Q_BEST_QUALITY		2 /* Binning disabled */
+/* Frame period in 0.1 ms units */
+#define REG_P_MAX_FR_TIME(n)		PREG(n, 0x0254)
+#define REG_P_MIN_FR_TIME(n)		PREG(n, 0x0256)
+#define  S5K5BAF_MIN_FR_TIME		333  /* x100 us */
+#define  S5K5BAF_MAX_FR_TIME		6500 /* x100 us */
+/* The below 5 registers are for "device correction" values */
+#define REG_P_SATURATION(n)		PREG(n, 0x0258)
+#define REG_P_SHARP_BLUR(n)		PREG(n, 0x025a)
+#define REG_P_GLAMOUR(n)		PREG(n, 0x025c)
+#define REG_P_COLORTEMP(n)		PREG(n, 0x025e)
+#define REG_P_GAMMA_INDEX(n)		PREG(n, 0x0260)
+#define REG_P_PREV_MIRROR(n)		PREG(n, 0x0262)
+#define REG_P_CAP_MIRROR(n)		PREG(n, 0x0264)
+#define REG_P_CAP_ROTATION(n)		PREG(n, 0x0266)
+
+/* Extended image property controls */
+/* Exposure time in 10 us units */
+#define REG_SF_USR_EXPOSURE_L		0x03bc
+#define REG_SF_USR_EXPOSURE_H		0x03be
+#define REG_SF_USR_EXPOSURE_CHG		0x03c0
+#define REG_SF_USR_TOT_GAIN		0x03c2
+#define REG_SF_USR_TOT_GAIN_CHG		0x03c4
+#define REG_SF_RGAIN			0x03c6
+#define REG_SF_RGAIN_CHG		0x03c8
+#define REG_SF_GGAIN			0x03ca
+#define REG_SF_GGAIN_CHG		0x03cc
+#define REG_SF_BGAIN			0x03ce
+#define REG_SF_BGAIN_CHG		0x03d0
+#define REG_SF_WBGAIN_CHG		0x03d2
+#define REG_SF_FLICKER_QUANT		0x03d4
+#define REG_SF_FLICKER_QUANT_CHG	0x03d6
+
+/* Output interface (parallel/MIPI) setup */
+#define REG_OIF_EN_MIPI_LANES		0x03f2
+#define REG_OIF_EN_PACKETS		0x03f4
+#define  EN_PACKETS_CSI2		0xc3
+#define REG_OIF_CFG_CHG			0x03f6
+
+/* Auto-algorithms enable mask */
+#define REG_DBG_AUTOALG_EN		0x03f8
+#define  AALG_ALL_EN			BIT(0)
+#define  AALG_AE_EN			BIT(1)
+#define  AALG_DIVLEI_EN			BIT(2)
+#define  AALG_WB_EN			BIT(3)
+#define  AALG_USE_WB_FOR_ISP		BIT(4)
+#define  AALG_FLICKER_EN		BIT(5)
+#define  AALG_FIT_EN			BIT(6)
+#define  AALG_WRHW_EN			BIT(7)
+
+/* Pointers to color correction matrices */
+#define REG_PTR_CCM_HORIZON		0x06d0
+#define REG_PTR_CCM_INCANDESCENT	0x06d4
+#define REG_PTR_CCM_WARM_WHITE		0x06d8
+#define REG_PTR_CCM_COOL_WHITE		0x06dc
+#define REG_PTR_CCM_DL50		0x06e0
+#define REG_PTR_CCM_DL65		0x06e4
+#define REG_PTR_CCM_OUTDOOR		0x06ec
+
+#define REG_ARR_CCM(n)			(0x2800 + 36 * (n))
+
+static const char * const s5k5baf_supply_names[] = {
+	"vdda",		/* Analog power supply 2.8V (2.6V to 3.0V) */
+	"vddreg",	/* Regulator input power supply 1.8V (1.7V to 1.9V)
+			   or 2.8V (2.6V to 3.0) */
+	"vddio",	/* I/O power 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_gpio {
+	int gpio;
+	int level;
+};
+
+enum s5k5baf_gpio_id {
+	STBY,
+	RST,
+	GPIO_NUM,
+};
+
+enum s5k5baf_pads_id {
+	PAD_CIS,
+	PAD_OUT,
+	CIS_PAD_NUM = 1,
+	ISP_PAD_NUM = 2
+};
+
+struct s5k5baf_pixfmt {
+	enum v4l2_mbus_pixelcode code;
+	u32 colorspace;
+	/* REG_P_FMT(x) register value */
+	u16 reg_p_fmt;
+};
+
+struct s5k5baf_ctrls {
+	struct v4l2_ctrl_handler handler;
+	struct { /* Auto / manual white balance cluster */
+		struct v4l2_ctrl *awb;
+		struct v4l2_ctrl *gain_red;
+		struct v4l2_ctrl *gain_blue;
+	};
+	struct { /* Mirror cluster */
+		struct v4l2_ctrl *hflip;
+		struct v4l2_ctrl *vflip;
+	};
+	struct { /* Auto exposure / manual exposure and gain cluster */
+		struct v4l2_ctrl *auto_exp;
+		struct v4l2_ctrl *exposure;
+		struct v4l2_ctrl *gain;
+	};
+};
+
+struct s5k5baf {
+	struct s5k5baf_gpio gpios[GPIO_NUM];
+	enum v4l2_mbus_type bus_type;
+	u8 nlanes;
+	struct regulator_bulk_data supplies[S5K5BAF_NUM_SUPPLIES];
+
+	struct clk *clock;
+	u32 mclk_frequency;
+
+	struct v4l2_subdev cis_sd;
+	struct media_pad cis_pad;
+
+	struct v4l2_subdev sd;
+	struct media_pad pads[ISP_PAD_NUM];
+
+	/* protects the struct members below */
+	struct mutex lock;
+
+	int error;
+
+	struct v4l2_rect crop_sink;
+	struct v4l2_rect compose;
+	struct v4l2_rect crop_source;
+	/* index to s5k5baf_formats array */
+	int pixfmt;
+	/* actual frame interval in 100us */
+	u16 fiv;
+	/* requested frame interval in 100us */
+	u16 req_fiv;
+
+	struct s5k5baf_ctrls ctrls;
+
+	unsigned int streaming:1;
+	unsigned int apply_cfg:1;
+	unsigned int apply_crop:1;
+	unsigned int power;
+};
+
+static const struct s5k5baf_pixfmt s5k5baf_formats[] = {
+	{ V4L2_MBUS_FMT_VYUY8_2X8,	V4L2_COLORSPACE_JPEG,	5 },
+	/* range 16-240 */
+	{ V4L2_MBUS_FMT_VYUY8_2X8,	V4L2_COLORSPACE_REC709,	6 },
+	{ V4L2_MBUS_FMT_RGB565_2X8_BE,	V4L2_COLORSPACE_JPEG,	0 },
+};
+
+static struct v4l2_rect s5k5baf_cis_rect = { 0, 0, S5K5BAF_CIS_WIDTH,
+				     S5K5BAF_CIS_HEIGHT };
+
+static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
+{
+	return &container_of(ctrl->handler, struct s5k5baf, ctrls.handler)->sd;
+}
+
+static inline bool s5k5baf_is_cis_subdev(struct v4l2_subdev *sd)
+{
+	return sd->entity.type == MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
+}
+
+static inline struct s5k5baf *to_s5k5baf(struct v4l2_subdev *sd)
+{
+	if (s5k5baf_is_cis_subdev(sd))
+		return container_of(sd, struct s5k5baf, cis_sd);
+	else
+		return container_of(sd, struct s5k5baf, sd);
+}
+
+static u16 s5k5baf_i2c_read(struct s5k5baf *state, u16 addr)
+{
+	struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
+	u16 w, r;
+	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);
+	ret = i2c_transfer(c->adapter, msg, 2);
+	r = ntohs(r);
+
+	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;
+}
+
+static void s5k5baf_i2c_write(struct s5k5baf *state, u16 addr, u16 val)
+{
+	u8 buf[4] = { addr >> 8, addr & 0xFF, val >> 8, val & 0xFF };
+	struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
+	int ret;
+
+	if (state->error)
+		return;
+
+	ret = i2c_master_send(c, buf, 4);
+	v4l2_dbg(3, debug, c, "i2c_write: 0x%04x : 0x%04x\n", addr, val);
+
+	if (ret != 4) {
+		v4l2_err(c, "i2c_write: error during transfer (%d)\n", ret);
+		state->error = ret;
+	}
+}
+
+static u16 s5k5baf_read(struct s5k5baf *state, u16 addr)
+{
+	s5k5baf_i2c_write(state, REG_CMDRD_ADDR, addr);
+	return s5k5baf_i2c_read(state, REG_CMD_BUF);
+}
+
+static void s5k5baf_write(struct s5k5baf *state, u16 addr, u16 val)
+{
+	s5k5baf_i2c_write(state, REG_CMDWR_ADDR, addr);
+	s5k5baf_i2c_write(state, REG_CMD_BUF, val);
+}
+
+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;
+
+	buf[0] = __constant_htons(REG_CMD_BUF);
+	for (n = 1; n <= count; ++n)
+		buf[n] = htons(*seq++);
+
+	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;
+	}
+}
+
+#define s5k5baf_write_seq(state, addr, seq...) \
+	s5k5baf_write_arr_seq(state, addr, sizeof((char[]){ seq }), \
+			      (const u16 []){ seq });
+
+/* add items count at the beginning of the list */
+#define NSEQ(seq...) sizeof((char[]){ seq }), seq
+
+/*
+ * s5k5baf_write_nseq() - Writes sequences of values to sensor memory via i2c
+ * @nseq: sequence of u16 words in format:
+ *	(N, address, value[1]...value[N-1])*,0
+ * Ex.:
+ *	u16 seq[] = { NSEQ(0x4000, 1, 1), NSEQ(0x4010, 640, 480), 0 };
+ *	ret = s5k5baf_write_nseq(c, seq);
+ */
+static void s5k5baf_write_nseq(struct s5k5baf *state, const u16 *nseq)
+{
+	int count;
+
+	while ((count = *nseq++)) {
+		u16 addr = *nseq++;
+		--count;
+
+		s5k5baf_write_arr_seq(state, addr, count, nseq);
+		nseq += count;
+	}
+}
+
+static void s5k5baf_synchronize(struct s5k5baf *state, int timeout, u16 addr)
+{
+	unsigned long end = jiffies + msecs_to_jiffies(timeout);
+	u16 reg;
+
+	s5k5baf_write(state, addr, 1);
+	do {
+		reg = s5k5baf_read(state, addr);
+		if (state->error || !reg)
+			return;
+		usleep_range(5000, 10000);
+	} while (time_is_after_jiffies(end));
+
+	v4l2_err(&state->sd, "timeout on register synchronize (%#x)\n", addr);
+	state->error = -ETIMEDOUT;
+}
+
+static void s5k5baf_hw_patch(struct s5k5baf *state)
+{
+	static const u16 nseq_patch[] = {
+		NSEQ(0x1668,
+		0xb5fe, 0x0007, 0x683c, 0x687e, 0x1da5, 0x88a0, 0x2800, 0xd00b,
+		0x88a8, 0x2800, 0xd008, 0x8820, 0x8829, 0x4288, 0xd301, 0x1a40,
+		0xe000, 0x1a08, 0x9001, 0xe001, 0x2019, 0x9001, 0x4916, 0x466b,
+		0x8a48, 0x8118, 0x8a88, 0x8158, 0x4814, 0x8940, 0x0040, 0x2103,
+		0xf000, 0xf826, 0x88a1, 0x4288, 0xd908, 0x8828, 0x8030, 0x8868,
+		0x8070, 0x88a8, 0x6038, 0xbcfe, 0xbc08, 0x4718, 0x88a9, 0x4288,
+		0xd906, 0x8820, 0x8030, 0x8860, 0x8070, 0x88a0, 0x6038, 0xe7f2,
+		0x9801, 0xa902, 0xf000, 0xf812, 0x0033, 0x0029, 0x9a02, 0x0020,
+		0xf000, 0xf814, 0x6038, 0xe7e6, 0x1a28, 0x7000, 0x0d64, 0x7000,
+		0x4778, 0x46c0, 0xf004, 0xe51f, 0xa464, 0x0000, 0x4778, 0x46c0,
+		0xc000, 0xe59f, 0xff1c, 0xe12f, 0x6009, 0x0000, 0x4778, 0x46c0,
+		0xc000, 0xe59f, 0xff1c, 0xe12f, 0x622f, 0x0000),
+		NSEQ(0x2080,
+		0xb510, 0xf000, 0xf8f4, 0xbc10, 0xbc08, 0x4718, 0xb5f0, 0xb08b,
+		0x0006, 0x2000, 0x9004, 0x6835, 0x6874, 0x68b0, 0x900a, 0x68f0,
+		0x9009, 0x4f7d, 0x8979, 0x084a, 0x88a8, 0x88a3, 0x4298, 0xd300,
+		0x0018, 0xf000, 0xf907, 0x9007, 0x0021, 0x0028, 0xaa04, 0xf000,
+		0xf909, 0x9006, 0x88a8, 0x2800, 0xd102, 0x27ff, 0x1c7f, 0xe047,
+		0x88a0, 0x2800, 0xd101, 0x2700, 0xe042, 0x8820, 0x466b, 0x8198,
+		0x8860, 0x81d8, 0x8828, 0x8118, 0x8868, 0x8158, 0xa802, 0xc803,
+		0xf000, 0xf8f8, 0x9008, 0x8aba, 0x9808, 0x466b, 0x4342, 0x9202,
+		0x8820, 0x8198, 0x8860, 0x81d8, 0x980a, 0x9903, 0xf000, 0xf8ea,
+		0x9a02, 0x17d1, 0x0e09, 0x1889, 0x1209, 0x4288, 0xdd1f, 0x8820,
+		0x466b, 0x8198, 0x8860, 0x81d8, 0x980a, 0x9903, 0xf000, 0xf8da,
+		0x9001, 0x8828, 0x466b, 0x8118, 0x8868, 0x8158, 0x980a, 0x9902,
+		0xf000, 0xf8d0, 0x8ab9, 0x9a08, 0x4351, 0x17ca, 0x0e12, 0x1851,
+		0x120a, 0x9901, 0xf000, 0xf8b6, 0x0407, 0x0c3f, 0xe000, 0x2700,
+		0x8820, 0x466b, 0xaa05, 0x8198, 0x8860, 0x81d8, 0x8828, 0x8118,
+		0x8868, 0x8158, 0xa802, 0xc803, 0x003b, 0xf000, 0xf8bb, 0x88a1,
+		0x88a8, 0x003a, 0xf000, 0xf8be, 0x0004, 0xa804, 0xc803, 0x9a09,
+		0x9b07, 0xf000, 0xf8af, 0xa806, 0xc805, 0x0021, 0xf000, 0xf8b2,
+		0x6030, 0xb00b, 0xbcf0, 0xbc08, 0x4718, 0xb5f1, 0x9900, 0x680c,
+		0x493a, 0x694b, 0x698a, 0x4694, 0x69cd, 0x6a0e, 0x4f38, 0x42bc,
+		0xd800, 0x0027, 0x4937, 0x6b89, 0x0409, 0x0c09, 0x4a35, 0x1e92,
+		0x6bd2, 0x0412, 0x0c12, 0x429f, 0xd801, 0x0020, 0xe031, 0x001f,
+		0x434f, 0x0a3f, 0x42a7, 0xd301, 0x0018, 0xe02a, 0x002b, 0x434b,
+		0x0a1b, 0x42a3, 0xd303, 0x0220, 0xf000, 0xf88c, 0xe021, 0x0029,
+		0x4351, 0x0a09, 0x42a1, 0xd301, 0x0028, 0xe01a, 0x0031, 0x4351,
+		0x0a09, 0x42a1, 0xd304, 0x0220, 0x0011, 0xf000, 0xf87b, 0xe010,
+		0x491e, 0x8c89, 0x000a, 0x4372, 0x0a12, 0x42a2, 0xd301, 0x0030,
+		0xe007, 0x4662, 0x434a, 0x0a12, 0x42a2, 0xd302, 0x0220, 0xf000,
+		0xf869, 0x4b16, 0x4d18, 0x8d99, 0x1fca, 0x3af9, 0xd00a, 0x2001,
+		0x0240, 0x8468, 0x0220, 0xf000, 0xf85d, 0x9900, 0x6008, 0xbcf8,
+		0xbc08, 0x4718, 0x8d19, 0x8469, 0x9900, 0x6008, 0xe7f7, 0xb570,
+		0x2200, 0x490e, 0x480e, 0x2401, 0xf000, 0xf852, 0x0022, 0x490d,
+		0x480d, 0x2502, 0xf000, 0xf84c, 0x490c, 0x480d, 0x002a, 0xf000,
+		0xf847, 0xbc70, 0xbc08, 0x4718, 0x0d64, 0x7000, 0x0470, 0x7000,
+		0xa120, 0x0007, 0x0402, 0x7000, 0x14a0, 0x7000, 0x208d, 0x7000,
+		0x622f, 0x0000, 0x1669, 0x7000, 0x6445, 0x0000, 0x21ab, 0x7000,
+		0x2aa9, 0x0000, 0x4778, 0x46c0, 0xc000, 0xe59f, 0xff1c, 0xe12f,
+		0x5f49, 0x0000, 0x4778, 0x46c0, 0xc000, 0xe59f, 0xff1c, 0xe12f,
+		0x5fc7, 0x0000, 0x4778, 0x46c0, 0xc000, 0xe59f, 0xff1c, 0xe12f,
+		0x5457, 0x0000, 0x4778, 0x46c0, 0xc000, 0xe59f, 0xff1c, 0xe12f,
+		0x5fa3, 0x0000, 0x4778, 0x46c0, 0xc000, 0xe59f, 0xff1c, 0xe12f,
+		0x51f9, 0x0000, 0x4778, 0x46c0, 0xf004, 0xe51f, 0xa464, 0x0000,
+		0x4778, 0x46c0, 0xc000, 0xe59f, 0xff1c, 0xe12f, 0xa007, 0x0000,
+		0x6546, 0x2062, 0x3120, 0x3220, 0x3130, 0x0030, 0xe010, 0x0208,
+		0x0058, 0x0000),
+		0
+	};
+
+	s5k5baf_write_nseq(state, nseq_patch);
+}
+
+static void s5k5baf_hw_set_clocks(struct s5k5baf *state)
+{
+	unsigned long mclk = state->mclk_frequency / 1000;
+	u16 status;
+	static const u16 nseq_clk_cfg[] = {
+		NSEQ(REG_I_USE_NPVI_CLOCKS,
+		  NPVI_CLOCKS, NMIPI_CLOCKS, 0,
+		  SCLK_PVI_FREQ / 4, PCLK_MIN_FREQ / 4, PCLK_MAX_FREQ / 4,
+		  SCLK_MIPI_FREQ / 4, PCLK_MIN_FREQ / 4, PCLK_MAX_FREQ / 4),
+		NSEQ(REG_I_USE_REGS_API, 1),
+		0
+	};
+
+	s5k5baf_write_seq(state, REG_I_INCLK_FREQ_L, mclk & 0xffff, mclk >> 16);
+	s5k5baf_write_nseq(state, nseq_clk_cfg);
+
+	s5k5baf_synchronize(state, 250, REG_I_INIT_PARAMS_UPDATED);
+	status = s5k5baf_read(state, REG_I_ERROR_INFO);
+	if (!state->error && status) {
+		v4l2_err(&state->sd, "error configuring PLL (%d)\n", status);
+		state->error = -EINVAL;
+	}
+}
+
+static void s5k5baf_hw_set_ccm(struct s5k5baf *state)
+{
+	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)
+{
+	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);
+}
+
+static void s5k5baf_hw_sync_cfg(struct s5k5baf *state)
+{
+	s5k5baf_write(state, REG_G_PREV_CFG_CHG, 1);
+	if (state->apply_crop) {
+		s5k5baf_write(state, REG_G_INPUTS_CHANGE_REQ, 1);
+		s5k5baf_write(state, REG_G_PREV_CFG_BYPASS_CHANGED, 1);
+	}
+	s5k5baf_synchronize(state, 500, REG_G_NEW_CFG_SYNC);
+}
+/* Set horizontal and vertical image flipping */
+static void s5k5baf_hw_set_mirror(struct s5k5baf *state)
+{
+	u16 flip = state->ctrls.vflip->val | (state->ctrls.vflip->val << 1);
+
+	s5k5baf_write(state, REG_P_PREV_MIRROR(0), flip);
+	if (state->streaming)
+		s5k5baf_hw_sync_cfg(state);
+}
+
+/* Configure auto/manual white balance and R/G/B gains */
+static void s5k5baf_hw_set_awb(struct s5k5baf *state, int awb)
+{
+	struct s5k5baf_ctrls *ctrls = &state->ctrls;
+	u16 reg;
+
+	reg = s5k5baf_read(state, REG_DBG_AUTOALG_EN);
+
+	if (!awb)
+		s5k5baf_write_seq(state, REG_SF_RGAIN,
+				  ctrls->gain_red->val, 1,
+				  S5K5BAF_GAIN_GREEN_DEF, 1,
+				  ctrls->gain_blue->val, 1,
+				  1);
+	reg = awb ? reg | AALG_WB_EN : reg & ~AALG_WB_EN;
+	s5k5baf_write(state, REG_DBG_AUTOALG_EN, reg);
+}
+
+/* Program FW with exposure time, 'exposure' in us units */
+static void s5k5baf_hw_set_user_exposure(struct s5k5baf *state, int exposure)
+{
+	unsigned int time = exposure / 10;
+
+	s5k5baf_write_seq(state, REG_SF_USR_EXPOSURE_L,
+			  time & 0xffff, time >> 16, 1);
+}
+
+static void s5k5baf_hw_set_user_gain(struct s5k5baf *state, int gain)
+{
+	s5k5baf_write_seq(state, REG_SF_USR_TOT_GAIN, gain, 1);
+}
+
+/* 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);
+
+	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_anti_flicker(struct s5k5baf *state, int v)
+{
+	u16 auto_alg;
+
+	auto_alg = s5k5baf_read(state, REG_DBG_AUTOALG_EN);
+
+	if (v == V4L2_CID_POWER_LINE_FREQUENCY_AUTO) {
+		auto_alg |= AALG_FLICKER_EN;
+	} else {
+		auto_alg &= ~AALG_FLICKER_EN;
+		/* The V4L2_CID_LINE_FREQUENCY control values match
+		 * the register values */
+		s5k5baf_write_seq(state, REG_SF_FLICKER_QUANT, v, 1);
+	}
+
+	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;
+	} else {
+		s5k5baf_write(state, REG_G_SPEC_EFFECTS, colorfx[val]);
+	}
+}
+
+static int s5k5baf_find_pixfmt(struct v4l2_mbus_framefmt *mf)
+{
+	int i, c = -1;
+
+	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;
+}
+
+static int s5k5baf_clear_error(struct s5k5baf *state)
+{
+	int ret = state->error;
+
+	state->error = 0;
+	return ret;
+}
+
+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;
+	};
+
+	s5k5baf_write_seq(state, REG_OIF_EN_MIPI_LANES,
+			  state->nlanes, en_packets, 1);
+
+	return s5k5baf_clear_error(state);
+}
+
+static u16 s5k5baf_get_cfg_error(struct s5k5baf *state)
+{
+	u16 err = s5k5baf_read(state, REG_G_PREV_CFG_ERROR);
+	if (err)
+		s5k5baf_write(state, REG_G_PREV_CFG_ERROR, 0);
+	return err;
+}
+
+static void s5k5baf_hw_set_fiv(struct s5k5baf *state, u16 fiv)
+{
+	s5k5baf_write(state, REG_P_MAX_FR_TIME(0), fiv);
+	s5k5baf_hw_sync_cfg(state);
+}
+
+static void s5k5baf_hw_find_min_fiv(struct s5k5baf *state)
+{
+	u16 err, fiv;
+	int n;
+
+	fiv = s5k5baf_read(state,  REG_G_ACTUAL_P_FR_TIME);
+	if (state->error)
+		return;
+
+	for (n = 5; n > 0; --n) {
+		s5k5baf_hw_set_fiv(state, fiv);
+		err = s5k5baf_get_cfg_error(state);
+		if (state->error)
+			return;
+		switch (err) {
+		case CFG_ERROR_RANGE:
+			++fiv;
+			break;
+		case 0:
+			state->fiv = fiv;
+			v4l2_info(&state->sd,
+				  "found valid frame interval: %d00us\n", fiv);
+			return;
+		default:
+			v4l2_err(&state->sd,
+				 "error setting frame interval: %d\n", err);
+			state->error = -EINVAL;
+		}
+	};
+	v4l2_err(&state->sd, "cannot find correct frame interval\n");
+	state->error = -ERANGE;
+}
+
+static void s5k5baf_hw_validate_cfg(struct s5k5baf *state)
+{
+	u16 err;
+
+	err = s5k5baf_get_cfg_error(state);
+	if (state->error)
+		return;
+
+	switch (err) {
+	case 0:
+		state->apply_cfg = 1;
+		return;
+	case CFG_ERROR_RANGE:
+		s5k5baf_hw_find_min_fiv(state);
+		if (!state->error)
+			state->apply_cfg = 1;
+		return;
+	default:
+		v4l2_err(&state->sd,
+			 "error setting format: %d\n", err);
+		state->error = -EINVAL;
+	}
+}
+
+static void s5k5baf_rescale(struct v4l2_rect *r, const struct v4l2_rect *v,
+			    const struct v4l2_rect *n,
+			    const struct v4l2_rect *d)
+{
+	r->left = v->left * n->width / d->width;
+	r->top = v->top * n->height / d->height;
+	r->width = v->width * n->width / d->width;
+	r->height = v->height * n->height / d->height;
+}
+
+static int s5k5baf_hw_set_crop_rects(struct s5k5baf *state)
+{
+	struct v4l2_rect *p, r;
+	u16 err;
+	int ret;
+
+	p = &state->crop_sink;
+	s5k5baf_write_seq(state, REG_G_PREVREQ_IN_WIDTH, p->width, p->height,
+			  p->left, p->top);
+
+	s5k5baf_rescale(&r, &state->crop_source, &state->crop_sink,
+			&state->compose);
+	s5k5baf_write_seq(state, REG_G_PREVZOOM_IN_WIDTH, r.width, r.height,
+			  r.left, r.top);
+
+	s5k5baf_synchronize(state, 500, REG_G_INPUTS_CHANGE_REQ);
+	s5k5baf_synchronize(state, 500, REG_G_PREV_CFG_BYPASS_CHANGED);
+	err = s5k5baf_get_cfg_error(state);
+	ret = s5k5baf_clear_error(state);
+	if (ret < 0)
+		return ret;
+
+	switch (err) {
+	case 0:
+		break;
+	case CFG_ERROR_RANGE:
+		/* retry crop with frame interval set to max */
+		s5k5baf_hw_set_fiv(state, S5K5BAF_MAX_FR_TIME);
+		err = s5k5baf_get_cfg_error(state);
+		ret = s5k5baf_clear_error(state);
+		if (ret < 0)
+			return ret;
+		if (err) {
+			v4l2_err(&state->sd,
+				 "crop error on max frame interval: %d\n", err);
+			state->error = -EINVAL;
+		}
+		s5k5baf_hw_set_fiv(state, state->req_fiv);
+		s5k5baf_hw_validate_cfg(state);
+		break;
+	default:
+		v4l2_err(&state->sd, "crop error: %d\n", err);
+		return -EINVAL;
+	}
+
+	if (!state->apply_cfg)
+		return 0;
+
+	p = &state->crop_source;
+	s5k5baf_write_seq(state, REG_P_OUT_WIDTH(0), p->width, p->height);
+	s5k5baf_hw_set_fiv(state, state->req_fiv);
+	s5k5baf_hw_validate_cfg(state);
+
+	return s5k5baf_clear_error(state);
+}
+
+static void s5k5baf_hw_set_config(struct s5k5baf *state)
+{
+	u16 reg_fmt = s5k5baf_formats[state->pixfmt].reg_p_fmt;
+	struct v4l2_rect *r = &state->crop_source;
+
+	s5k5baf_write_seq(state, REG_P_OUT_WIDTH(0),
+			  r->width, r->height, reg_fmt,
+			  PCLK_MAX_FREQ >> 2, PCLK_MIN_FREQ >> 2,
+			  PVI_MASK_MIPI, CLK_MIPI_INDEX,
+			  FR_RATE_FIXED, FR_RATE_Q_DYNAMIC,
+			  state->req_fiv, S5K5BAF_MIN_FR_TIME);
+	s5k5baf_hw_sync_cfg(state);
+	s5k5baf_hw_validate_cfg(state);
+}
+
+
+static void s5k5baf_hw_set_test_pattern(struct s5k5baf *state, int id)
+{
+	s5k5baf_i2c_write(state, REG_PATTERN_WIDTH, 800);
+	s5k5baf_i2c_write(state, REG_PATTERN_HEIGHT, 511);
+	s5k5baf_i2c_write(state, REG_PATTERN_PARAM, 0);
+	s5k5baf_i2c_write(state, REG_PATTERN_SET, id);
+}
+
+static void s5k5baf_gpio_assert(struct s5k5baf *state, int id)
+{
+	struct s5k5baf_gpio *gpio = &state->gpios[id];
+
+	gpio_set_value(gpio->gpio, gpio->level);
+}
+
+static void s5k5baf_gpio_deassert(struct s5k5baf *state, int id)
+{
+	struct s5k5baf_gpio *gpio = &state->gpios[id];
+
+	gpio_set_value(gpio->gpio, !gpio->level);
+}
+
+static int s5k5baf_power_on(struct s5k5baf *state)
+{
+	int ret;
+
+	state->clock = clk_get(state->sd.dev, S5K5BAF_CLK_NAME);
+	if (IS_ERR(state->clock))
+		return PTR_ERR(state->clock);
+
+	ret = regulator_bulk_enable(S5K5BAF_NUM_SUPPLIES, state->supplies);
+	if (ret < 0)
+		goto err_clk_put;
+
+	ret = clk_set_rate(state->clock, state->mclk_frequency);
+	if (ret < 0)
+		goto err_reg_dis;
+
+	ret = clk_prepare_enable(state->clock);
+	if (ret < 0)
+		goto err_reg_dis;
+
+	v4l2_dbg(1, debug, &state->sd, "clock frequency: %ld\n",
+		 clk_get_rate(state->clock));
+
+	s5k5baf_gpio_deassert(state, STBY);
+	usleep_range(50, 100);
+	s5k5baf_gpio_deassert(state, RST);
+	return 0;
+
+err_reg_dis:
+	regulator_bulk_disable(S5K5BAF_NUM_SUPPLIES, state->supplies);
+err_clk_put:
+	clk_put(state->clock);
+	v4l2_err(&state->sd, "%s() failed (%d)\n", __func__, ret);
+	return ret;
+}
+
+static int s5k5baf_power_off(struct s5k5baf *state)
+{
+	int ret;
+
+	state->streaming = 0;
+	state->apply_cfg = 0;
+	state->apply_crop = 0;
+
+	s5k5baf_gpio_assert(state, RST);
+	s5k5baf_gpio_assert(state, STBY);
+
+	if (!IS_ERR(state->clock))
+		clk_disable_unprepare(state->clock);
+
+	ret = regulator_bulk_disable(S5K5BAF_NUM_SUPPLIES,
+					state->supplies);
+	if (ret < 0)
+		v4l2_err(&state->sd, "failed to disable regulators\n");
+
+	if (!IS_ERR(state->clock))
+		clk_put(state->clock);
+
+	return 0;
+}
+
+static void s5k5baf_hw_init(struct s5k5baf *state)
+{
+	s5k5baf_i2c_write(state, AHB_MSB_ADDR_PTR, PAGE_IF_HW);
+	s5k5baf_i2c_write(state, REG_CLEAR_HOST_INT, 0);
+	s5k5baf_i2c_write(state, REG_SW_LOAD_COMPLETE, 1);
+	s5k5baf_i2c_write(state, REG_CMDRD_PAGE, PAGE_IF_SW);
+	s5k5baf_i2c_write(state, REG_CMDWR_PAGE, PAGE_IF_SW);
+}
+
+/*
+ * V4L2 subdev core and video operations
+ */
+
+static void s5k5baf_initialize_data(struct s5k5baf *state)
+{
+	state->pixfmt = 0;
+	state->req_fiv = 10000 / 15;
+	state->fiv = state->req_fiv;
+}
+
+static int s5k5baf_set_power(struct v4l2_subdev *sd, int on)
+{
+	struct s5k5baf *state = to_s5k5baf(sd);
+	int ret = 0;
+
+	mutex_lock(&state->lock);
+
+	if (!on != state->power)
+		goto out;
+
+	if (on) {
+		s5k5baf_initialize_data(state);
+		ret = s5k5baf_power_on(state);
+		if (ret < 0)
+			goto out;
+
+		s5k5baf_hw_init(state);
+		s5k5baf_hw_patch(state);
+		s5k5baf_i2c_write(state, REG_SET_HOST_INT, 1);
+		s5k5baf_hw_set_clocks(state);
+
+		ret = s5k5baf_hw_set_video_bus(state);
+		if (ret < 0)
+			goto out;
+
+		s5k5baf_hw_set_cis(state);
+		s5k5baf_hw_set_ccm(state);
+
+		ret = s5k5baf_clear_error(state);
+		if (!ret)
+			state->power++;
+	} else {
+		s5k5baf_power_off(state);
+		state->power--;
+	}
+
+out:
+	mutex_unlock(&state->lock);
+
+	if (!ret && on)
+		ret = v4l2_ctrl_handler_setup(&state->ctrls.handler);
+
+	return ret;
+}
+
+static void s5k5baf_hw_set_stream(struct s5k5baf *state, int enable)
+{
+	s5k5baf_write_seq(state, REG_G_ENABLE_PREV, enable, 1);
+}
+
+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);
+
+	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;
+}
+
+static int s5k5baf_g_frame_interval(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_frame_interval *fi)
+{
+	struct s5k5baf *state = to_s5k5baf(sd);
+
+	mutex_lock(&state->lock);
+	fi->interval.numerator = state->fiv;
+	fi->interval.denominator = 10000;
+	mutex_unlock(&state->lock);
+
+	return 0;
+}
+
+static void s5k5baf_set_frame_interval(struct s5k5baf *state,
+				       struct v4l2_subdev_frame_interval *fi)
+{
+	struct v4l2_fract *i = &fi->interval;
+
+	if (fi->interval.denominator == 0)
+		state->req_fiv = S5K5BAF_MAX_FR_TIME;
+	else
+		state->req_fiv = clamp_t(u32,
+					 i->numerator * 10000 / i->denominator,
+					 S5K5BAF_MIN_FR_TIME,
+					 S5K5BAF_MAX_FR_TIME);
+
+	state->fiv = state->req_fiv;
+	if (state->apply_cfg) {
+		s5k5baf_hw_set_fiv(state, state->req_fiv);
+		s5k5baf_hw_validate_cfg(state);
+	}
+	*i = (struct v4l2_fract){ state->fiv, 10000 };
+	if (state->fiv == state->req_fiv)
+		v4l2_info(&state->sd, "frame interval changed to %d00us\n",
+			  state->fiv);
+}
+
+static int s5k5baf_s_frame_interval(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_frame_interval *fi)
+{
+	struct s5k5baf *state = to_s5k5baf(sd);
+
+	mutex_lock(&state->lock);
+	s5k5baf_set_frame_interval(state, fi);
+	mutex_unlock(&state->lock);
+	return 0;
+}
+
+/*
+ * V4L2 subdev pad level and video operations
+ */
+static int s5k5baf_enum_frame_interval(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_fh *fh,
+			      struct v4l2_subdev_frame_interval_enum *fie)
+{
+	if (fie->index > S5K5BAF_MAX_FR_TIME - S5K5BAF_MIN_FR_TIME ||
+	    fie->pad != PAD_CIS)
+		return -EINVAL;
+
+	v4l_bound_align_image(&fie->width, S5K5BAF_WIN_WIDTH_MIN,
+			      S5K5BAF_CIS_WIDTH, 1,
+			      &fie->height, S5K5BAF_WIN_HEIGHT_MIN,
+			      S5K5BAF_CIS_HEIGHT, 1, 0);
+
+	fie->interval.numerator = S5K5BAF_MIN_FR_TIME + fie->index;
+	fie->interval.denominator = 10000;
+
+	return 0;
+}
+
+static int s5k5baf_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_fh *fh,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->pad == PAD_CIS) {
+		if (code->index > 0)
+			return -EINVAL;
+		code->code = V4L2_MBUS_FMT_FIXED;
+		return 0;
+	}
+
+	if (code->index >= ARRAY_SIZE(s5k5baf_formats))
+		return -EINVAL;
+
+	code->code = s5k5baf_formats[code->index].code;
+	return 0;
+}
+
+static int s5k5baf_enum_frame_size(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_frame_size_enum *fse)
+{
+	int i;
+
+	if (fse->index > 0)
+		return -EINVAL;
+
+	if (fse->pad == PAD_CIS) {
+		fse->code = V4L2_MBUS_FMT_FIXED;
+		fse->min_width = S5K5BAF_CIS_WIDTH;
+		fse->max_width = S5K5BAF_CIS_WIDTH;
+		fse->min_height = S5K5BAF_CIS_HEIGHT;
+		fse->max_height = S5K5BAF_CIS_HEIGHT;
+		return 0;
+	}
+
+	i = ARRAY_SIZE(s5k5baf_formats);
+	while (--i)
+		if (fse->code == s5k5baf_formats[i].code)
+			break;
+	fse->code = s5k5baf_formats[i].code;
+	fse->min_width = S5K5BAF_WIN_WIDTH_MIN;
+	fse->max_width = S5K5BAF_CIS_WIDTH;
+	fse->max_height = S5K5BAF_WIN_HEIGHT_MIN;
+	fse->min_height = S5K5BAF_CIS_HEIGHT;
+
+	return 0;
+}
+
+static void s5k5baf_try_cis_format(struct v4l2_mbus_framefmt *mf)
+{
+	mf->width = S5K5BAF_CIS_WIDTH;
+	mf->height = S5K5BAF_CIS_HEIGHT;
+	mf->code = V4L2_MBUS_FMT_FIXED;
+	mf->colorspace = V4L2_COLORSPACE_JPEG;
+	mf->field = V4L2_FIELD_NONE;
+}
+
+static int s5k5baf_try_isp_format(struct v4l2_mbus_framefmt *mf)
+{
+	int pixfmt;
+
+	v4l_bound_align_image(&mf->width, S5K5BAF_WIN_WIDTH_MIN,
+			      S5K5BAF_CIS_WIDTH, 1,
+			      &mf->height, S5K5BAF_WIN_HEIGHT_MIN,
+			      S5K5BAF_CIS_HEIGHT, 1, 0);
+
+	pixfmt = s5k5baf_find_pixfmt(mf);
+
+	mf->colorspace = s5k5baf_formats[pixfmt].colorspace;
+	mf->code = s5k5baf_formats[pixfmt].code;
+	mf->field = V4L2_FIELD_NONE;
+
+	return pixfmt;
+}
+
+static int s5k5baf_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			  struct v4l2_subdev_format *fmt)
+{
+	struct s5k5baf *state = to_s5k5baf(sd);
+	const struct s5k5baf_pixfmt *pixfmt;
+	struct v4l2_mbus_framefmt *mf;
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		mf = v4l2_subdev_get_try_format(fh, fmt->pad);
+		fmt->format = *mf;
+		return 0;
+	}
+
+	mf = &fmt->format;
+	if (fmt->pad == PAD_CIS) {
+		s5k5baf_try_cis_format(mf);
+		return 0;
+	}
+	mf->field = V4L2_FIELD_NONE;
+	mutex_lock(&state->lock);
+	pixfmt = &s5k5baf_formats[state->pixfmt];
+	mf->width = state->crop_source.width;
+	mf->height = state->crop_source.height;
+	mf->code = pixfmt->code;
+	mf->colorspace = pixfmt->colorspace;
+	mutex_unlock(&state->lock);
+
+	return 0;
+}
+
+static int s5k5baf_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			  struct v4l2_subdev_format *fmt)
+{
+	struct v4l2_mbus_framefmt *mf = &fmt->format;
+	struct s5k5baf *state = to_s5k5baf(sd);
+	const struct s5k5baf_pixfmt *pixfmt;
+	int ret = 0;
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		*v4l2_subdev_get_try_format(fh, fmt->pad) = *mf;
+		return 0;
+	}
+
+	if (fmt->pad == PAD_CIS) {
+		s5k5baf_try_cis_format(mf);
+		return 0;
+	}
+
+	mutex_lock(&state->lock);
+
+	if (state->streaming) {
+		mutex_unlock(&state->lock);
+		return -EBUSY;
+	}
+
+	state->pixfmt = s5k5baf_try_isp_format(mf);
+	pixfmt = &s5k5baf_formats[state->pixfmt];
+	mf->code = pixfmt->code;
+	mf->colorspace = pixfmt->colorspace;
+	mf->width = state->crop_source.width;
+	mf->height = state->crop_source.height;
+
+	mutex_unlock(&state->lock);
+	return ret;
+}
+
+enum selection_rect { R_CIS, R_CROP_SINK, R_COMPOSE, R_CROP_SOURCE, R_INVALID };
+
+static enum selection_rect s5k5baf_get_sel_rect(u32 pad, u32 target)
+{
+	switch (target) {
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		return pad ? R_COMPOSE : R_CIS;
+	case V4L2_SEL_TGT_CROP:
+		return pad ? R_CROP_SOURCE : R_CROP_SINK;
+	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+		return pad ? R_INVALID : R_CROP_SINK;
+	case V4L2_SEL_TGT_COMPOSE:
+		return pad ? R_INVALID : R_COMPOSE;
+	default:
+		return R_INVALID;
+	}
+}
+
+static int s5k5baf_is_bound_target(u32 target)
+{
+	return (target == V4L2_SEL_TGT_CROP_BOUNDS ||
+		target == V4L2_SEL_TGT_COMPOSE_BOUNDS);
+}
+
+static int s5k5baf_get_selection(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_fh *fh,
+				 struct v4l2_subdev_selection *sel)
+{
+	static enum selection_rect rtype;
+	struct s5k5baf *state = to_s5k5baf(sd);
+
+	rtype = s5k5baf_get_sel_rect(sel->pad, sel->target);
+
+	switch (rtype) {
+	case R_INVALID:
+		return -EINVAL;
+	case R_CIS:
+		sel->r = s5k5baf_cis_rect;
+		return 0;
+	default:
+		break;
+	}
+
+	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
+		if (rtype == R_COMPOSE)
+			sel->r = *v4l2_subdev_get_try_compose(fh, sel->pad);
+		else
+			sel->r = *v4l2_subdev_get_try_crop(fh, sel->pad);
+		return 0;
+	}
+
+	mutex_lock(&state->lock);
+	switch (rtype) {
+	case R_CROP_SINK:
+		sel->r = state->crop_sink;
+		break;
+	case R_COMPOSE:
+		sel->r = state->compose;
+		break;
+	case R_CROP_SOURCE:
+		sel->r = state->crop_source;
+		break;
+	default:
+		break;
+	}
+	if (s5k5baf_is_bound_target(sel->target)) {
+		sel->r.left = 0;
+		sel->r.top = 0;
+	}
+	mutex_unlock(&state->lock);
+
+	return 0;
+}
+
+/* bounds range [start, start+len) to [0, max) and aligns to 2 */
+static void s5k5baf_bound_range(u32 *start, u32 *len, u32 max)
+{
+	if (*len > max)
+		*len = max;
+	if (*start + *len > max)
+		*start = max - *len;
+	*start &= ~1;
+	*len &= ~1;
+	if (*len < S5K5BAF_WIN_WIDTH_MIN)
+		*len = S5K5BAF_WIN_WIDTH_MIN;
+}
+
+static void s5k5baf_bound_rect(struct v4l2_rect *r, u32 width, u32 height)
+{
+	s5k5baf_bound_range(&r->left, &r->width, width);
+	s5k5baf_bound_range(&r->top, &r->height, height);
+}
+
+static void s5k5baf_set_rect_and_adjust(struct v4l2_rect **rects,
+					enum selection_rect first,
+					struct v4l2_rect *v)
+{
+	struct v4l2_rect *r, *br;
+	enum selection_rect i = first;
+
+	*rects[first] = *v;
+	do {
+		r = rects[i];
+		br = rects[i - 1];
+		s5k5baf_bound_rect(r, br->width, br->height);
+	} while (++i != R_INVALID);
+	*v = *rects[first];
+}
+
+static bool s5k5baf_cmp_rect(const struct v4l2_rect *r1,
+			     const struct v4l2_rect *r2)
+{
+	return !memcmp(r1, r2, sizeof(*r1));
+}
+
+static int s5k5baf_set_selection(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_fh *fh,
+				 struct v4l2_subdev_selection *sel)
+{
+	static enum selection_rect rtype;
+	struct s5k5baf *state = to_s5k5baf(sd);
+	struct v4l2_rect **rects;
+	int ret = 0;
+
+	rtype = s5k5baf_get_sel_rect(sel->pad, sel->target);
+	if (rtype == R_INVALID || s5k5baf_is_bound_target(sel->target))
+		return -EINVAL;
+
+	/* allow only scaling on compose */
+	if (rtype == R_COMPOSE) {
+		sel->r.left = 0;
+		sel->r.top = 0;
+	}
+
+	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
+		rects = (struct v4l2_rect * []) {
+				&s5k5baf_cis_rect,
+				v4l2_subdev_get_try_crop(fh, PAD_CIS),
+				v4l2_subdev_get_try_compose(fh, PAD_CIS),
+				v4l2_subdev_get_try_crop(fh, PAD_OUT)
+			};
+		s5k5baf_set_rect_and_adjust(rects, rtype, &sel->r);
+		return 0;
+	}
+
+	rects = (struct v4l2_rect * []) {
+			&s5k5baf_cis_rect,
+			&state->crop_sink,
+			&state->compose,
+			&state->crop_source
+		};
+	mutex_lock(&state->lock);
+	if (state->streaming) {
+		/* adjust sel->r to avoid output resolution change */
+		if (rtype < R_CROP_SOURCE) {
+			if (sel->r.width < state->crop_source.width)
+				sel->r.width = state->crop_source.width;
+			if (sel->r.height < state->crop_source.height)
+				sel->r.height = state->crop_source.height;
+		} else {
+			sel->r.width = state->crop_source.width;
+			sel->r.height = state->crop_source.height;
+		}
+	}
+	s5k5baf_set_rect_and_adjust(rects, rtype, &sel->r);
+	if (!s5k5baf_cmp_rect(&state->crop_sink, &s5k5baf_cis_rect) ||
+	    !s5k5baf_cmp_rect(&state->compose, &s5k5baf_cis_rect))
+		state->apply_crop = 1;
+	if (state->streaming)
+		ret = s5k5baf_hw_set_crop_rects(state);
+	mutex_unlock(&state->lock);
+
+	return ret;
+}
+
+static const struct v4l2_subdev_pad_ops s5k5baf_cis_pad_ops = {
+	.enum_mbus_code		= s5k5baf_enum_mbus_code,
+	.enum_frame_size	= s5k5baf_enum_frame_size,
+	.get_fmt		= s5k5baf_get_fmt,
+	.set_fmt		= s5k5baf_set_fmt,
+};
+
+static const struct v4l2_subdev_pad_ops s5k5baf_pad_ops = {
+	.enum_mbus_code		= s5k5baf_enum_mbus_code,
+	.enum_frame_size	= s5k5baf_enum_frame_size,
+	.enum_frame_interval	= s5k5baf_enum_frame_interval,
+	.get_fmt		= s5k5baf_get_fmt,
+	.set_fmt		= s5k5baf_set_fmt,
+	.get_selection		= s5k5baf_get_selection,
+	.set_selection		= s5k5baf_set_selection,
+};
+
+static const struct v4l2_subdev_video_ops s5k5baf_video_ops = {
+	.g_frame_interval	= s5k5baf_g_frame_interval,
+	.s_frame_interval	= s5k5baf_s_frame_interval,
+	.s_stream		= s5k5baf_s_stream,
+};
+
+/*
+ * V4L2 subdev controls
+ */
+
+static int s5k5baf_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
+	struct s5k5baf *state = to_s5k5baf(sd);
+	int ret;
+
+	v4l2_dbg(1, debug, sd, "ctrl: %s, value: %d\n", ctrl->name, ctrl->val);
+
+	mutex_lock(&state->lock);
+
+	if (state->power == 0)
+		goto unlock;
+
+	switch (ctrl->id) {
+	case V4L2_CID_AUTO_WHITE_BALANCE:
+		s5k5baf_hw_set_awb(state, ctrl->val);
+		break;
+
+	case V4L2_CID_BRIGHTNESS:
+		s5k5baf_write(state, REG_USER_BRIGHTNESS, ctrl->val);
+		break;
+
+	case V4L2_CID_COLORFX:
+		s5k5baf_hw_set_colorfx(state, ctrl->val);
+		break;
+
+	case V4L2_CID_CONTRAST:
+		s5k5baf_write(state, REG_USER_CONTRAST, ctrl->val);
+		break;
+
+	case V4L2_CID_EXPOSURE_AUTO:
+		s5k5baf_hw_set_auto_exposure(state, ctrl->val);
+		break;
+
+	case V4L2_CID_HFLIP:
+		s5k5baf_hw_set_mirror(state);
+		break;
+
+	case V4L2_CID_POWER_LINE_FREQUENCY:
+		s5k5baf_hw_set_anti_flicker(state, ctrl->val);
+		break;
+
+	case V4L2_CID_SATURATION:
+		s5k5baf_write(state, REG_USER_SATURATION, ctrl->val);
+		break;
+
+	case V4L2_CID_SHARPNESS:
+		s5k5baf_write(state, REG_USER_SHARPBLUR, ctrl->val);
+		break;
+
+	case V4L2_CID_WHITE_BALANCE_TEMPERATURE:
+		s5k5baf_write(state, REG_P_COLORTEMP(0), ctrl->val);
+		if (state->apply_cfg)
+			s5k5baf_hw_sync_cfg(state);
+		break;
+
+	case V4L2_CID_TEST_PATTERN:
+		s5k5baf_hw_set_test_pattern(state, ctrl->val);
+		break;
+	}
+unlock:
+	ret = s5k5baf_clear_error(state);
+	mutex_unlock(&state->lock);
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops s5k5baf_ctrl_ops = {
+	.s_ctrl	= s5k5baf_s_ctrl,
+};
+
+static const char * const s5k5baf_test_pattern_menu[] = {
+	"Disabled",
+	"Blank",
+	"Bars",
+	"Gradients",
+	"Textile",
+	"Textile2",
+	"Squares"
+};
+
+static int s5k5baf_initialize_ctrls(struct s5k5baf *state)
+{
+	const struct v4l2_ctrl_ops *ops = &s5k5baf_ctrl_ops;
+	struct s5k5baf_ctrls *ctrls = &state->ctrls;
+	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
+	int ret;
+
+	ret = v4l2_ctrl_handler_init(hdl, 16);
+	if (ret < 0) {
+		v4l2_err(&state->sd, "cannot init ctrl handler (%d)\n", ret);
+		return ret;
+	}
+
+	/* Auto white balance cluster */
+	ctrls->awb = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTO_WHITE_BALANCE,
+				       0, 1, 1, 1);
+	ctrls->gain_red = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE,
+					    0, 255, 1, S5K5BAF_GAIN_RED_DEF);
+	ctrls->gain_blue = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BLUE_BALANCE,
+					     0, 255, 1, S5K5BAF_GAIN_BLUE_DEF);
+	v4l2_ctrl_auto_cluster(3, &ctrls->awb, 0, false);
+
+	ctrls->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
+	ctrls->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
+	v4l2_ctrl_cluster(2, &ctrls->hflip);
+
+	ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
+				V4L2_CID_EXPOSURE_AUTO,
+				V4L2_EXPOSURE_MANUAL, 0, V4L2_EXPOSURE_AUTO);
+	/* Exposure time: x 1 us */
+	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
+					    0, 6000000U, 1, 100000U);
+	/* Total gain: 256 <=> 1x */
+	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN,
+					0, 256, 1, 256);
+	v4l2_ctrl_auto_cluster(3, &ctrls->auto_exp, 0, false);
+
+	v4l2_ctrl_new_std_menu(hdl, ops, V4L2_CID_POWER_LINE_FREQUENCY,
+			       V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
+			       V4L2_CID_POWER_LINE_FREQUENCY_AUTO);
+
+	v4l2_ctrl_new_std_menu(hdl, ops, V4L2_CID_COLORFX,
+			       V4L2_COLORFX_SKY_BLUE, ~0x6f, V4L2_COLORFX_NONE);
+
+	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_WHITE_BALANCE_TEMPERATURE,
+			  0, 256, 1, 0);
+
+	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION, -127, 127, 1, 0);
+	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS, -127, 127, 1, 0);
+	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, -127, 127, 1, 0);
+	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SHARPNESS, -127, 127, 1, 0);
+
+	v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(s5k5baf_test_pattern_menu) - 1,
+				     0, 0, s5k5baf_test_pattern_menu);
+
+	if (hdl->error) {
+		v4l2_err(&state->sd, "error creating controls (%d)\n",
+			 hdl->error);
+		ret = hdl->error;
+		v4l2_ctrl_handler_free(hdl);
+		return ret;
+	}
+
+	state->sd.ctrl_handler = hdl;
+	return 0;
+}
+
+/*
+ * 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;
+
+	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;
+
+	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);
+}
+
+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,
+	.unregistered = s5k5baf_unregistered,
+	.open = s5k5baf_open,
+};
+
+static const struct v4l2_subdev_core_ops s5k5baf_core_ops = {
+	.s_power = s5k5baf_set_power,
+	.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 int 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->gpios;
+	int ret, i;
+
+	for (i = 0; i < GPIO_NUM; ++i) {
+		int flags = 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 < 0) {
+			v4l2_err(c, "failed to request gpio %s\n", name[i]);
+			return ret;
+		}
+	}
+	return 0;
+}
+
+static int s5k5baf_parse_gpios(struct s5k5baf_gpio *gpios, struct device *dev)
+{
+	static const char * const names[] = {
+		"stbyn-gpios",
+		"rstn-gpios",
+	};
+	struct device_node *node = dev->of_node;
+	enum of_gpio_flags flags;
+	int ret, i;
+
+	for (i = 0; i < GPIO_NUM; ++i) {
+		ret = of_get_named_gpio_flags(node, names[i], 0, &flags);
+		if (ret < 0) {
+			dev_err(dev, "no %s GPIO pin provided\n", names[i]);
+			return ret;
+		}
+		gpios[i].gpio = ret;
+		gpios[i].level = !(flags & OF_GPIO_ACTIVE_LOW);
+	}
+
+	return 0;
+}
+
+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;
+	}
+
+	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;
+}
+
+static int s5k5baf_configure_subdevs(struct s5k5baf *state,
+				     struct i2c_client *c)
+{
+	struct v4l2_subdev *sd;
+	int ret;
+
+	sd = &state->cis_sd;
+	v4l2_subdev_init(sd, &s5k5baf_cis_subdev_ops);
+	sd->owner = THIS_MODULE;
+	v4l2_set_subdevdata(sd, state);
+	snprintf(sd->name, sizeof(sd->name), "S5K5BAF-CIS %d-%04x",
+		 i2c_adapter_id(c->adapter), c->addr);
+
+	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;
+	ret = media_entity_init(&sd->entity, CIS_PAD_NUM, &state->cis_pad, 0);
+	if (ret < 0)
+		goto err;
+
+	sd = &state->sd;
+	v4l2_i2c_subdev_init(sd, c, &s5k5baf_subdev_ops);
+	snprintf(sd->name, sizeof(sd->name), "S5K5BAF-ISP %d-%04x",
+		 i2c_adapter_id(c->adapter), c->addr);
+
+	sd->internal_ops = &s5k5baf_subdev_internal_ops;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+	state->pads[PAD_CIS].flags = MEDIA_PAD_FL_SINK;
+	state->pads[PAD_OUT].flags = MEDIA_PAD_FL_SOURCE;
+	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV;
+	ret = media_entity_init(&sd->entity, ISP_PAD_NUM, state->pads, 0);
+
+	if (!ret)
+		return 0;
+
+	media_entity_cleanup(&state->cis_sd.entity);
+err:
+	dev_err(&c->dev, "cannot init media entity %s\n", sd->name);
+	return ret;
+}
+
+static int s5k5baf_configure_regulators(struct s5k5baf *state)
+{
+	struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
+	int ret;
+	int i;
+
+	for (i = 0; i < S5K5BAF_NUM_SUPPLIES; i++)
+		state->supplies[i].supply = s5k5baf_supply_names[i];
+
+	ret = devm_regulator_bulk_get(&c->dev, S5K5BAF_NUM_SUPPLIES,
+				      state->supplies);
+	if (ret < 0)
+		v4l2_err(c, "failed to get regulators\n");
+	return ret;
+}
+
+static int s5k5baf_probe(struct i2c_client *c,
+			const struct i2c_device_id *id)
+{
+	struct s5k5baf *state;
+	int ret;
+
+	state = devm_kzalloc(&c->dev, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	mutex_init(&state->lock);
+	state->crop_sink = s5k5baf_cis_rect;
+	state->compose = s5k5baf_cis_rect;
+	state->crop_source = s5k5baf_cis_rect;
+
+	ret = s5k5baf_parse_device_node(state, &c->dev);
+	if (ret < 0)
+		return ret;
+
+	ret = s5k5baf_configure_subdevs(state, c);
+	if (ret < 0)
+		return ret;
+
+	ret = s5k5baf_configure_gpios(state);
+	if (ret < 0)
+		goto err_me;
+
+	ret = s5k5baf_configure_regulators(state);
+	if (ret < 0)
+		goto err_me;
+
+	ret = s5k5baf_power_on(state);
+	if (ret < 0) {
+		ret = -EPROBE_DEFER;
+		goto err_me;
+	}
+	s5k5baf_hw_init(state);
+	ret = s5k5baf_check_fw_revision(state);
+
+	s5k5baf_power_off(state);
+	if (ret < 0)
+		goto err_me;
+
+	ret = s5k5baf_initialize_ctrls(state);
+	if (ret < 0)
+		goto err_me;
+
+	ret = v4l2_async_register_subdev(&state->sd);
+	if (ret < 0)
+		goto err_ctrl;
+
+	return 0;
+
+err_ctrl:
+	v4l2_ctrl_handler_free(state->sd.ctrl_handler);
+err_me:
+	media_entity_cleanup(&state->sd.entity);
+	media_entity_cleanup(&state->cis_sd.entity);
+	return ret;
+}
+
+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[] = {
+	{ S5K5BAF_DRIVER_NAME, 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, s5k5baf_id);
+
+static const struct of_device_id s5k5baf_of_match[] = {
+	{ .compatible = "samsung,s5k5baf" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, s5k5baf_of_match);
+
+static struct i2c_driver s5k5baf_i2c_driver = {
+	.driver = {
+		.of_match_table = s5k5baf_of_match,
+		.name = S5K5BAF_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@samsung.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v7] s5k5baf: add camera sensor driver
  2013-08-21 14:41 [PATCH v7] s5k5baf: add camera sensor driver Andrzej Hajda
@ 2013-08-22 20:01 ` Stephen Warren
  2013-08-22 22:39 ` Tomasz Figa
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Stephen Warren @ 2013-08-22 20:01 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: linux-media, laurent.pinchart, linux-samsung-soc, devicetree,
	Sylwester Nawrocki, Kyungmin Park, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Grant Likely

On 08/21/2013 08:41 AM, 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.

The binding,
Acked-by: Stephen Warren <swarren@nvidia.com>

(although it would be great if another DT binding maintainer gave it a
quick look-over to make sure I didn't miss anything!)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v7] s5k5baf: add camera sensor driver
  2013-08-21 14:41 [PATCH v7] s5k5baf: add camera sensor driver Andrzej Hajda
  2013-08-22 20:01 ` Stephen Warren
@ 2013-08-22 22:39 ` Tomasz Figa
  2013-08-23  9:23   ` Sylwester Nawrocki
  2013-08-23  9:48 ` Pawel Moll
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Tomasz Figa @ 2013-08-22 22:39 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: linux-media, laurent.pinchart, linux-samsung-soc, devicetree,
	Sylwester Nawrocki, Kyungmin Park, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Grant Likely

Hi Andrzej,

Please see some minor comments inline.

On Wednesday 21 of 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
> 
> diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
> b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt new file
> mode 100644
> index 0000000..d680d99
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
> @@ -0,0 +1,59 @@
> +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;

Can this sensor have an aribitrary slave address or only a set of well 
known possible addresses (e.g. listed in documentation)?

> +- vdda-supply	  : analog power supply 2.8V (2.6V to 3.0V);
> +- vddreg-supply	  : regulator input power supply 1.8V (1.7V to 
1.9V)
> +		    or 2.8V (2.6V to 3.0);
> +- vddio-supply	  : I/O power supply 1.8V (1.65V to 1.95V)
> +		    or 2.8V (2.5V to 3.1V);
> +- stbyn-gpios	  : GPIO connected to STDBYN pin;
> +- rstn-gpios	  : GPIO connected to RSTN pin;

Both GPIOs above have names suggesting that they are active low. I wonder 
how the GPIO flags cell is interpreted here, namely the polarity bit.

Otherwise the binding looks good.

Best regards,
Tomasz


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v7] s5k5baf: add camera sensor driver
  2013-08-22 22:39 ` Tomasz Figa
@ 2013-08-23  9:23   ` Sylwester Nawrocki
  2013-08-23  9:48     ` Sylwester Nawrocki
  0 siblings, 1 reply; 12+ messages in thread
From: Sylwester Nawrocki @ 2013-08-23  9:23 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Andrzej Hajda, linux-media, laurent.pinchart, linux-samsung-soc,
	devicetree, Kyungmin Park, Rob Herring, Pawel Moll, Mark Rutland,
	Stephen Warren, Ian Campbell, Grant Likely

On 08/23/2013 12:39 AM, Tomasz Figa wrote:
>> Documentation/devicetree/bindings/media/samsung-s5k5baf.txt create mode
>> > 100644 drivers/media/i2c/s5k5baf.c
>> > 
>> > diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
>> > b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt new file
>> > mode 100644
>> > index 0000000..d680d99
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
>> > @@ -0,0 +1,59 @@
>> > +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;
>
> Can this sensor have an aribitrary slave address or only a set of well 
> known possible addresses (e.g. listed in documentation)?

According to the datasheet it can have one of two I2C addresses (0x2D, 0x3C),
selectable by a dedicated pin. Also they may be revisions of the device that
use different addresses. I believe what addresses are possible is out of
scope of this binding document. We can handle whatever is used.

>> > +- vdda-supply	  : analog power supply 2.8V (2.6V to 3.0V);
>> > +- vddreg-supply	  : regulator input power supply 1.8V (1.7V to 
> 1.9V)
>> > +		    or 2.8V (2.6V to 3.0);
>> > +- vddio-supply	  : I/O power supply 1.8V (1.65V to 1.95V)
>> > +		    or 2.8V (2.5V to 3.1V);
>> > +- stbyn-gpios	  : GPIO connected to STDBYN pin;
>> > +- rstn-gpios	  : GPIO connected to RSTN pin;
>
> Both GPIOs above have names suggesting that they are active low. I wonder 
> how the GPIO flags cell is interpreted here, namely the polarity bit.

That's a good point. The GPIO flags are be used to specify active state
of the GPIO. Some sensors happen to use different active state for those
signals. It's not the case for this sensor though AFAICT.

Anyway IMO it would be better to name those gpios: "stby-gpios",
"rst-gpios" in case there appear revisions that have their pin named STDBY
or RST rather than STDBYN, RSTN. That seems rather unlikely though, but
since there are devices to which that could apply I think for consistency
it might be better to remove indication of polarity from the GPIO names.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v7] s5k5baf: add camera sensor driver
  2013-08-21 14:41 [PATCH v7] s5k5baf: add camera sensor driver Andrzej Hajda
  2013-08-22 20:01 ` Stephen Warren
  2013-08-22 22:39 ` Tomasz Figa
@ 2013-08-23  9:48 ` Pawel Moll
  2013-08-23 12:53 ` Laurent Pinchart
  2013-08-27  9:14 ` Mark Rutland
  4 siblings, 0 replies; 12+ messages in thread
From: Pawel Moll @ 2013-08-23  9:48 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: linux-media, laurent.pinchart, linux-samsung-soc, devicetree,
	Sylwester Nawrocki, Kyungmin Park, rob.herring, Mark Rutland,
	Stephen Warren, Ian Campbell, grant.likely

On Wed, 2013-08-21 at 15:41 +0100, Andrzej Hajda wrote:
> diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
> new file mode 100644
> index 0000000..d680d99
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
> @@ -0,0 +1,59 @@
> +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);
> +- vddreg-supply          : regulator input power supply 1.8V (1.7V to 1.9V)
> +                   or 2.8V (2.6V to 3.0);
> +- vddio-supply   : I/O power supply 1.8V (1.65V to 1.95V)
> +                   or 2.8V (2.5V to 3.1V);
> +- stbyn-gpios    : GPIO connected to STDBYN pin;
> +- rstn-gpios     : GPIO connected to RSTN pin;
> +- clocks         : the sensor's master clock specifier (from the common
> +                   clock bindings);
> +- clock-names    : must be "mclk";
> +
> +Optional properties:
> +
> +- clock-frequency : the frequency at which the "mclk" clock should be
> +                   configured to operate, in Hz; if this property is not
> +                   specified default 24 MHz value will be used.
> +
> +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) specifies MIPI CSI-2 data lanes as covered in
> +  video-interfaces.txt. This property can be only used to specify number
> +  of data lanes, i.e. the array's content is unused, only its length is
> +  meaningful. When this property is not specified default value of 1 lane
> +  will be used.
> +
> +Example:
> +
> +s5k5bafx@2d {
> +       compatible = "samsung,s5k5baf";
> +       reg = <0x2d>;
> +       vdda-supply = <&cam_io_en_reg>;
> +       vddreg-supply = <&vt_core_15v_reg>;
> +       vddio-supply = <&vtcam_reg>;
> +       stbyn-gpios = <&gpl2 0 1>;
> +       rstn-gpios = <&gpl2 1 1>;
> +       clock-names = "mclk";
> +       clocks = <&clock_cam 0>;
> +       clock-frequency = <24000000>;
> +
> +       port {
> +               s5k5bafx_ep: endpoint {
> +                       remote-endpoint = <&csis1_ep>;
> +                       data-lanes = <1>;
> +               };
> +       };
> +};

For the binding:

Acked-by: Pawel Moll <pawel.moll@arm.com>

As to the discussion about GPIO naming, I'll stand by the "call it what
it is called in the documentation" stanza...

Thanks!

Pawel



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v7] s5k5baf: add camera sensor driver
  2013-08-23  9:23   ` Sylwester Nawrocki
@ 2013-08-23  9:48     ` Sylwester Nawrocki
  0 siblings, 0 replies; 12+ messages in thread
From: Sylwester Nawrocki @ 2013-08-23  9:48 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Andrzej Hajda, linux-media, laurent.pinchart, linux-samsung-soc,
	devicetree, Kyungmin Park, Rob Herring, Pawel Moll, Mark Rutland,
	Stephen Warren, Ian Campbell, Grant Likely

On 08/23/2013 11:23 AM, Sylwester Nawrocki wrote:
>>>> +- stbyn-gpios	  : GPIO connected to STDBYN pin;
>>>> >> > +- rstn-gpios	  : GPIO connected to RSTN pin;
>> >
>> > Both GPIOs above have names suggesting that they are active low. I wonder 
>> > how the GPIO flags cell is interpreted here, namely the polarity bit.

To be more clear, the polarity bit specifies GPIO state at the GPIO controller
(SoC) that corresponds to active STANDBY or RESET signal state at the sensor.
So it is supposed to cover any inverter in between the sensor and an SoC.

> That's a good point. The GPIO flags are be used to specify active state
> of the GPIO. Some sensors happen to use different active state for those
> signals. It's not the case for this sensor though AFAICT.
> 
> Anyway IMO it would be better to name those gpios: "stby-gpios",
> "rst-gpios" in case there appear revisions that have their pin named STDBY
> or RST rather than STDBYN, RSTN. That seems rather unlikely though, but
> since there are devices to which that could apply I think for consistency
> it might be better to remove indication of polarity from the GPIO names.



-- 
Sylwester Nawrocki
Samsung R&D Institute Poland

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v7] s5k5baf: add camera sensor driver
  2013-08-21 14:41 [PATCH v7] s5k5baf: add camera sensor driver Andrzej Hajda
                   ` (2 preceding siblings ...)
  2013-08-23  9:48 ` Pawel Moll
@ 2013-08-23 12:53 ` Laurent Pinchart
  2013-08-26 12:34   ` Andrzej Hajda
  2013-08-27  9:14 ` Mark Rutland
  4 siblings, 1 reply; 12+ messages in thread
From: Laurent Pinchart @ 2013-08-23 12:53 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: linux-media, linux-samsung-soc, devicetree, Sylwester Nawrocki,
	Kyungmin Park, Rob Herring, Pawel Moll, Mark Rutland,
	Stephen Warren, Ian Campbell

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v7] s5k5baf: add camera sensor driver
  2013-08-23 12:53 ` Laurent Pinchart
@ 2013-08-26 12:34   ` Andrzej Hajda
  2013-08-27 11:52     ` Laurent Pinchart
  0 siblings, 1 reply; 12+ messages in thread
From: Andrzej Hajda @ 2013-08-26 12:34 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-media, linux-samsung-soc, devicetree, Sylwester Nawrocki,
	Kyungmin Park, Rob Herring, Pawel Moll, Mark Rutland,
	Stephen Warren, Ian Campbell

Hi Laurent,

Thank you for the review.

On 08/23/2013 02:53 PM, Laurent Pinchart wrote:
> 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.
OK
>
>> +static struct v4l2_rect s5k5baf_cis_rect = { 0, 0, S5K5BAF_CIS_WIDTH,
>> +				     S5K5BAF_CIS_HEIGHT };
> Shouldn't this be const ?
OK
>
>> +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.
OK
>
>> +	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 ?
OK
>
>> +	ret = i2c_transfer(c->adapter, msg, 2);
>> +	r = ntohs(r);
> And be16_to_cpu() here.
OK
>
>> +
>> +	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.
I have introduced state->error to avoid code bloat. With this 'pattern'
error is checked in about 10 places in the code, of course without
scarifying code correctness.
Replacing this pattern with classic 'return error directly from function'
would result with adding error checks after all calls to i2c i/o functions
and after calls to many functions which those i2c i/o calls contains.
According to my rough estimates it is about 70 places.

Similar pattern is used already in v4l2_ctrl_handler::error.

>
>> +	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 ?
OK
>
>> +
>> +	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.
OK
>
>> +{
>> +	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.
That's the magic, but I will add an comment about where it comes from.
>
>> +{
>> +	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.
OK.
>
>> +
>> +	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 ?
OK, I will remove this check.
>
>> +	} 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 ?
Yes, in case of incorrect DT bindings.
>
>> +	};
>> +
>> +	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 ?
Yes, will be corrected.
>
>> +	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 ?
They share common code, not much but still :)
I see no gain in separating it.
>
>> +	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.
OK
>
>> +	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.
The sensor exposes two subdevs: s5k5baf_cis and s5k5baf_isp.
v4l2_device is not aware of it - he registers only the subdev bound to
i2c client - isp.
The registration of cis subdev is performed by the sensor in .registered
callback. Without .registered callback cis subdev will not be registered.

This is similar solution to smiapp and s5c73m3 drivers.

Regarding .unregistered callback, it seems that removing it would not harm -
there should be added only check in .registered to avoid its
re-registration.
On the other hand IMO if sensor driver is responsible for registration
it should
be responsible for unregistration of subdev, what do you think about it?

And about links: v4l2_device_unregister_subdev calls
media_entity_remove_links,
so in case device is re-registered driver should re-create them.

>
>> +}
> [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.
OK

>
>> +	}
>> +
>> +	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;
>> +}

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v7] s5k5baf: add camera sensor driver
  2013-08-21 14:41 [PATCH v7] s5k5baf: add camera sensor driver Andrzej Hajda
                   ` (3 preceding siblings ...)
  2013-08-23 12:53 ` Laurent Pinchart
@ 2013-08-27  9:14 ` Mark Rutland
  2013-09-02 16:21   ` Sylwester Nawrocki
  4 siblings, 1 reply; 12+ messages in thread
From: Mark Rutland @ 2013-08-27  9:14 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: linux-media, laurent.pinchart, linux-samsung-soc, devicetree,
	Sylwester Nawrocki, Kyungmin Park, rob.herring, Pawel Moll,
	Stephen Warren, Ian Campbell, grant.likely

Hi,

[trimming down to relevant context]

> +endpoint node
> +-------------
> +
> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
> +  video-interfaces.txt. This property can be only used to specify number
> +  of data lanes, i.e. the array's content is unused, only its length is
> +  meaningful. When this property is not specified default value of 1 lane
> +  will be used.

Apologies for having not replied to the last posting, but having looked
at the documentation I was provided last time [1], I don't think the
values in the data-lanes property should be described as unused. That
may be the way the Linux driver functions at present, but it's not how
the generic video-interfaces binding documentation describes the
property.

If the CSI transmitter hardware doesn't support logical remapping of
lanes, then the only valid values for data-lanes would be a contiguous
list of lane IDs starting at 1, ending at 4 at most. Valid values for
the property would be one of:

data-lanes = <1>;
data-lanes = <1>, <2>;
data-lanes = <1>, <2>, <3>;
data-lanes = <1>, <2>, <3>, <4>;

We can mention the fact the hardware doesn't support remapping of lanes,
and therefore the list must start with lane 1 and end with (at most)
lane 4. That way a dts will match the generic binding and actually
describe the hardware, and it's possible for Linux (or any other OS) to
factor out the parsing of data-lanes later as desired.

I don't think we should offer freedom to encode garbage in the dt when
we can just as easily encourage more standard use of bindings that will
make our lives easier in the long-term.

Thanks,
Mark.

[1] http://www.mipi.org/specifications/camera-interface#CSI2

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v7] s5k5baf: add camera sensor driver
  2013-08-26 12:34   ` Andrzej Hajda
@ 2013-08-27 11:52     ` Laurent Pinchart
  0 siblings, 0 replies; 12+ messages in thread
From: Laurent Pinchart @ 2013-08-27 11:52 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: linux-media, linux-samsung-soc, devicetree, Sylwester Nawrocki,
	Kyungmin Park, Rob Herring, Pawel Moll, Mark Rutland,
	Stephen Warren, Ian Campbell

Hi Andrejz,

On Monday 26 August 2013 14:34:21 Andrzej Hajda wrote:
> On 08/23/2013 02:53 PM, Laurent Pinchart wrote:
> > 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]

> >> +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.
> 
> I have introduced state->error to avoid code bloat. With this 'pattern'
> error is checked in about 10 places in the code, of course without
> scarifying code correctness.
> Replacing this pattern with classic 'return error directly from function'
> would result with adding error checks after all calls to i2c i/o functions
> and after calls to many functions which those i2c i/o calls contains.
> According to my rough estimates it is about 70 places.
> 
> Similar pattern is used already in v4l2_ctrl_handler::error.
> 
> >> +	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 ?
> 
> OK
> 
> >> +
> >> +	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 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 ?
> 
> Yes, in case of incorrect DT bindings.

Shouldn't it be caught at probe time instead then ?

> >> +	};
> >> +
> >> +	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 ?
> 
> Yes, will be corrected.
> 
> >> +	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]

> >> +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.
> 
> The sensor exposes two subdevs: s5k5baf_cis and s5k5baf_isp.
> v4l2_device is not aware of it - he registers only the subdev bound to
> i2c client - isp.
> The registration of cis subdev is performed by the sensor in .registered
> callback. Without .registered callback cis subdev will not be registered.

You're right, my bad. I had overlooked that, I thought you were registering 
the ISP subdev. Could you please add a comment to the .registered() handler to 
document this ?

> This is similar solution to smiapp and s5c73m3 drivers.
> 
> Regarding .unregistered callback, it seems that removing it would not harm -
> there should be added only check in .registered to avoid its re-
> registration. On the other hand IMO if sensor driver is responsible for
> registration it should be responsible for unregistration of subdev, what do
> you think about it?

I agree.

> And about links: v4l2_device_unregister_subdev calls
> media_entity_remove_links,
> so in case device is re-registered driver should re-create them.
> 
> >> +}

[snip]

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v7] s5k5baf: add camera sensor driver
  2013-08-27  9:14 ` Mark Rutland
@ 2013-09-02 16:21   ` Sylwester Nawrocki
  2013-09-02 16:55     ` Mark Rutland
  0 siblings, 1 reply; 12+ messages in thread
From: Sylwester Nawrocki @ 2013-09-02 16:21 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Andrzej Hajda, linux-media, laurent.pinchart, linux-samsung-soc,
	devicetree, Kyungmin Park, rob.herring, Pawel Moll,
	Stephen Warren, Ian Campbell, grant.likely

Hi Mark,

On 08/27/2013 11:14 AM, Mark Rutland wrote:
>> +endpoint node
>> +-------------
>> +
>> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
>> +  video-interfaces.txt. This property can be only used to specify number
>> +  of data lanes, i.e. the array's content is unused, only its length is
>> +  meaningful. When this property is not specified default value of 1 lane
>> +  will be used.
> 
> Apologies for having not replied to the last posting, but having looked
> at the documentation I was provided last time [1], I don't think the
> values in the data-lanes property should be described as unused. That
> may be the way the Linux driver functions at present, but it's not how
> the generic video-interfaces binding documentation describes the
> property.
> 
> If the CSI transmitter hardware doesn't support logical remapping of
> lanes, then the only valid values for data-lanes would be a contiguous
> list of lane IDs starting at 1, ending at 4 at most. Valid values for
> the property would be one of:
> 
> data-lanes = <1>;
> data-lanes = <1>, <2>;
> data-lanes = <1>, <2>, <3>;
> data-lanes = <1>, <2>, <3>, <4>;
> 
> We can mention the fact the hardware doesn't support remapping of lanes,
> and therefore the list must start with lane 1 and end with (at most)
> lane 4. That way a dts will match the generic binding and actually
> describe the hardware, and it's possible for Linux (or any other OS) to
> factor out the parsing of data-lanes later as desired.
> 
> I don't think we should offer freedom to encode garbage in the dt when
> we can just as easily encourage more standard use of bindings that will
> make our lives easier in the long-term.

I entirely agree, that's a very accurate analysis.

Presumably the data-lanes property's descriptions could be improved so
it is said explicitly that array elements 0...N - 1, where N = 4, 
correspond to logical data lanes 1...N.

Then considering the values in the data-lanes property, I didn't make
the description terribly specific about the fact that pool of indexes
0...4 is used for the clock lane and 4 data lanes. The values could well
be H/W specific, but it seems more sensible to enforce common range.
It may not match exactly with documentation of various hardware. E.g.
OMAP, see page 1661, register CSI2_COMPLEXIO_CFG [1], uses indexes 
1..5 to indicate position of a data lane and 0 is used to mark a lane 
as unused.

I think we should have similarly defined value 0 to indicate an unused 
lane. None of drivers in mainline uses this line remapping feature, so 
changing meaning of the array values wouldn't presumably have any bad 
side effects. I'm not sure if it's OK to make a change like this now. 
IIUC the MIPI CSI-2 standard requires consecutive data lane indexes, 
so valid set of data lanes could be only: <1>, <1, 2>, <1, 2, 3>, 
<1, 2, 3, 4>. So there seems to be no issue for MIPI CSI-2. But for 
future protocols the current convention might not have been flexible 
enough.

> [1] http://www.mipi.org/specifications/camera-interface#CSI2

[2] http://www.ti.com/general/docs/lit/getliterature.tsp?literatureNumber=swpu231ao&fileType=pdf

--
Regards,
Sylwester

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v7] s5k5baf: add camera sensor driver
  2013-09-02 16:21   ` Sylwester Nawrocki
@ 2013-09-02 16:55     ` Mark Rutland
  0 siblings, 0 replies; 12+ messages in thread
From: Mark Rutland @ 2013-09-02 16:55 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: Andrzej Hajda, linux-media, laurent.pinchart, linux-samsung-soc,
	devicetree, Kyungmin Park, rob.herring, Pawel Moll,
	Stephen Warren, Ian Campbell, grant.likely

On Mon, Sep 02, 2013 at 05:21:58PM +0100, Sylwester Nawrocki wrote:
> Hi Mark,

Hi Sylwester,

> 
> On 08/27/2013 11:14 AM, Mark Rutland wrote:
> >> +endpoint node
> >> +-------------
> >> +
> >> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
> >> +  video-interfaces.txt. This property can be only used to specify number
> >> +  of data lanes, i.e. the array's content is unused, only its length is
> >> +  meaningful. When this property is not specified default value of 1 lane
> >> +  will be used.
> > 
> > Apologies for having not replied to the last posting, but having looked
> > at the documentation I was provided last time [1], I don't think the
> > values in the data-lanes property should be described as unused. That
> > may be the way the Linux driver functions at present, but it's not how
> > the generic video-interfaces binding documentation describes the
> > property.
> > 
> > If the CSI transmitter hardware doesn't support logical remapping of
> > lanes, then the only valid values for data-lanes would be a contiguous
> > list of lane IDs starting at 1, ending at 4 at most. Valid values for
> > the property would be one of:
> > 
> > data-lanes = <1>;
> > data-lanes = <1>, <2>;
> > data-lanes = <1>, <2>, <3>;
> > data-lanes = <1>, <2>, <3>, <4>;
> > 
> > We can mention the fact the hardware doesn't support remapping of lanes,
> > and therefore the list must start with lane 1 and end with (at most)
> > lane 4. That way a dts will match the generic binding and actually
> > describe the hardware, and it's possible for Linux (or any other OS) to
> > factor out the parsing of data-lanes later as desired.
> > 
> > I don't think we should offer freedom to encode garbage in the dt when
> > we can just as easily encourage more standard use of bindings that will
> > make our lives easier in the long-term.
> 
> I entirely agree, that's a very accurate analysis.
> 
> Presumably the data-lanes property's descriptions could be improved so
> it is said explicitly that array elements 0...N - 1, where N = 4, 
> correspond to logical data lanes 1...N.

That makes sense to me.

> 
> Then considering the values in the data-lanes property, I didn't make
> the description terribly specific about the fact that pool of indexes
> 0...4 is used for the clock lane and 4 data lanes. The values could well
> be H/W specific, but it seems more sensible to enforce common range.
> It may not match exactly with documentation of various hardware. E.g.
> OMAP, see page 1661, register CSI2_COMPLEXIO_CFG [1], uses indexes 
> 1..5 to indicate position of a data lane and 0 is used to mark a lane 
> as unused.

Ah, I see. I guess this boils down to what the "physical indexes"
referred to by the video-interfaces binding actually are. If an
interface may only ever have 5 lanes, then using a logical index sounds
fine. But if we ever have the case where a CSI transmitter has more than
5 lanes (so it could communicate with multiple receviers), then the
value has to become hardware-specific. At that point, we'd possibly need
to define remapping of the clocks too. I'm not that familiar with CSI so
I'm not sure if such a device is possible.

> 
> I think we should have similarly defined value 0 to indicate an unused 
> lane. None of drivers in mainline uses this line remapping feature, so 
> changing meaning of the array values wouldn't presumably have any bad 
> side effects. I'm not sure if it's OK to make a change like this now. 
> IIUC the MIPI CSI-2 standard requires consecutive data lane indexes, 
> so valid set of data lanes could be only: <1>, <1, 2>, <1, 2, 3>, 
> <1, 2, 3, 4>. So there seems to be no issue for MIPI CSI-2. But for 
> future protocols the current convention might not have been flexible 
> enough.

Given that the video-interfaces binding refers to the clock being on
lane 0, I'm not sure it makes sense to reserve this as an unused id --
we'd be saying the lane is the clock to say it's unused, but maybe that
doesn't matter.

Thanks,
Mark.

> 
> > [1] http://www.mipi.org/specifications/camera-interface#CSI2
> 
> [2] http://www.ti.com/general/docs/lit/getliterature.tsp?literatureNumber=swpu231ao&fileType=pdf
 
> --
> Regards,
> Sylwester
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-09-02 16:55 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-21 14:41 [PATCH v7] s5k5baf: add camera sensor driver Andrzej Hajda
2013-08-22 20:01 ` Stephen Warren
2013-08-22 22:39 ` Tomasz Figa
2013-08-23  9:23   ` Sylwester Nawrocki
2013-08-23  9:48     ` Sylwester Nawrocki
2013-08-23  9:48 ` Pawel Moll
2013-08-23 12:53 ` Laurent Pinchart
2013-08-26 12:34   ` Andrzej Hajda
2013-08-27 11:52     ` Laurent Pinchart
2013-08-27  9:14 ` Mark Rutland
2013-09-02 16:21   ` Sylwester Nawrocki
2013-09-02 16:55     ` Mark Rutland

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.