[PATCH 00/24] V4L2: subdevice pad-level API wrapper

[PATCH 00/24] V4L2: subdevice pad-level API wrapper

[Guennadi Liakhovetski]

This is the first very crude shot at the subdevice pad-level API wrapper.
The actual wrapper is added in patch #21, previous 20 patches are
preparation... They apply on top of the last version of my async / clock
patch series, respectively, on top of the announced branch of my linuxtv
git-tree. Patches 2 and 4 from this series should actually be merged into
respective patches from the async series.

I'm publishing this patch-series now, because I don't know when and how
much time I'll have to improve it... Maybe you don't want to spend too much
time reviewing implementation details, but comments to general concepts
would be appreciated.

Further note, that patches 8-12 aren't really required. We can keep the
deprecated struct soc_camera_link for now, or use a more gentle and slow
way to remove it.

Guennadi Liakhovetski (24):
  V4L2: (cosmetic) remove redundant use of unlikely()
  imx074: fix error handling for failed async subdevice registration
  mt9t031: fix NULL dereference during probe()
  V4L2: fix Oops on rmmod path
  V4L2: allow dummy file-handle initialisation by v4l2_fh_init()
  V4L2: add a common V4L2 subdevice platform data type
  soc-camera: switch to using the new struct v4l2_subdev_platform_data
  ARM: update all soc-camera users to new platform data layout
  SH: update all soc-camera users to new platform data layout
  soc-camera: update soc-camera-platform & its users to a new platform
    data layout
  soc-camera: completely remove struct soc_camera_link
  V4L2: soc-camera: retrieve subdevice platform data from struct
    v4l2_subdev
  ARM: pcm037: convert custom GPIO-based power function to a regulator
  mx3-camera: clean up the use of platform data, add driver owner
  mx3-camera: support asynchronous subdevice registration
  V4L2: mt9p031: add support for V4L2 clock and asynchronous probing
  V4L2: mt9p031: add support for .g_mbus_config() video operation
  V4L2: mt9p031: power down the sensor if no supported device has been
    detected
  V4L2: add struct v4l2_subdev_try_buf
  V4L2: add a subdev pointer to struct v4l2_subdev_fh
  V4L2: add a subdevice-driver pad-operation wrapper
  V4L2: soc-camera: use the pad-operation wrapper
  V4L2: mt9p031: add struct v4l2_subdev_platform_data to platform data
  ARM: pcm037: support mt9p031 / mt9p006 camera sensors

 Documentation/video4linux/soc-camera.txt       |    2 +-
 arch/arm/mach-at91/board-sam9m10g45ek.c        |   19 +-
 arch/arm/mach-imx/mach-imx27_visstrim_m10.c    |   17 +-
 arch/arm/mach-imx/mach-mx27_3ds.c              |   21 +-
 arch/arm/mach-imx/mach-mx31_3ds.c              |   23 +-
 arch/arm/mach-imx/mach-mx35_3ds.c              |   12 +-
 arch/arm/mach-imx/mach-pcm037.c                |  120 +++++++--
 arch/arm/mach-imx/mx31moboard-marxbot.c        |   17 +-
 arch/arm/mach-imx/mx31moboard-smartbot.c       |   17 +-
 arch/arm/mach-omap1/board-ams-delta.c          |   17 +-
 arch/arm/mach-pxa/em-x270.c                    |   15 +-
 arch/arm/mach-pxa/ezx.c                        |   36 ++-
 arch/arm/mach-pxa/mioa701.c                    |   11 +-
 arch/arm/mach-pxa/palmz72.c                    |   21 +-
 arch/arm/mach-pxa/pcm990-baseboard.c           |   44 ++--
 arch/arm/mach-shmobile/board-ap4evb.c          |    5 +-
 arch/arm/mach-shmobile/board-armadillo800eva.c |   17 +-
 arch/arm/mach-shmobile/board-mackerel.c        |   23 +-
 arch/sh/boards/mach-ap325rxa/setup.c           |   43 ++--
 arch/sh/boards/mach-ecovec24/setup.c           |   51 +++--
 arch/sh/boards/mach-kfr2r09/setup.c            |   15 +-
 arch/sh/boards/mach-migor/setup.c              |   30 ++-
 drivers/media/i2c/mt9p031.c                    |   55 ++++-
 drivers/media/i2c/soc_camera/imx074.c          |    5 +-
 drivers/media/i2c/soc_camera/mt9t031.c         |    7 +-
 drivers/media/platform/soc_camera/mx3_camera.c |   29 ++-
 drivers/media/platform/soc_camera/soc_camera.c |   49 +++-
 drivers/media/usb/em28xx/em28xx-camera.c       |   12 +-
 drivers/media/v4l2-core/Makefile               |    3 +
 drivers/media/v4l2-core/v4l2-async.c           |   18 +-
 drivers/media/v4l2-core/v4l2-fh.c              |    8 +-
 drivers/media/v4l2-core/v4l2-pad-wrap.c        |  329 ++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-subdev.c          |    1 +
 include/linux/platform_data/camera-mx3.h       |    3 +
 include/media/mt9p031.h                        |    3 +
 include/media/soc_camera.h                     |   53 +----
 include/media/soc_camera_platform.h            |    8 +-
 include/media/v4l2-pad-wrap.h                  |   23 ++
 include/media/v4l2-subdev.h                    |   31 ++-
 39 files changed, 909 insertions(+), 304 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-pad-wrap.c
 create mode 100644 include/media/v4l2-pad-wrap.h

-- 
1.7.2.5

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH 01/24] V4L2: (cosmetic) remove redundant use of unlikely()

[Guennadi Liakhovetski]

BUG*() and WARN*() macros specify their conditions as unlikely, using
BUG_ON(unlikely(condition)) is redundant, remove it.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 include/media/v4l2-subdev.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 21174af..eb91366 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -625,8 +625,8 @@ struct v4l2_subdev_fh {
 	v4l2_subdev_get_try_##fun_name(struct v4l2_subdev_fh *fh,	\
 				       unsigned int pad)		\
 	{								\
-		BUG_ON(unlikely(pad >= vdev_to_v4l2_subdev(		\
-					fh->vfh.vdev)->entity.num_pads)); \
+		BUG_ON(pad >= vdev_to_v4l2_subdev(			\
+					fh->vfh.vdev)->entity.num_pads); \
 		return &fh->pad[pad].field_name;			\
 	}
 
-- 
1.7.2.5

[PATCH 02/24] imx074: fix error handling for failed async subdevice registration

[Guennadi Liakhovetski]

If v4l2_async_register_subdev() fails, don't skip the clean up.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/soc_camera/imx074.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/imx074.c b/drivers/media/i2c/soc_camera/imx074.c
index c0eed84..23de859 100644
--- a/drivers/media/i2c/soc_camera/imx074.c
+++ b/drivers/media/i2c/soc_camera/imx074.c
@@ -472,7 +472,9 @@ static int imx074_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto eprobe;
 
-	return v4l2_async_register_subdev(&priv->subdev);
+	ret = v4l2_async_register_subdev(&priv->subdev);
+	if (!ret)
+		return 0;
 
 epwrinit:
 eprobe:
-- 
1.7.2.5

[PATCH 03/24] mt9t031: fix NULL dereference during probe()

[Guennadi Liakhovetski]

When .s_power() is called during probing, the video device isn't available
yet. Fix Oops, caused by dereferencing it.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/soc_camera/mt9t031.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/mt9t031.c b/drivers/media/i2c/soc_camera/mt9t031.c
index ea791e3..e7f0a08 100644
--- a/drivers/media/i2c/soc_camera/mt9t031.c
+++ b/drivers/media/i2c/soc_camera/mt9t031.c
@@ -619,9 +619,12 @@ static int mt9t031_s_power(struct v4l2_subdev *sd, int on)
 		ret = soc_camera_power_on(&client->dev, ssdd, mt9t031->clk);
 		if (ret < 0)
 			return ret;
-		vdev->dev.type = &mt9t031_dev_type;
+		if (vdev)
+			/* Skip during probing, when vdev isn't available yet */
+			vdev->dev.type = &mt9t031_dev_type;
 	} else {
-		vdev->dev.type = NULL;
+		if (vdev)
+			vdev->dev.type = NULL;
 		soc_camera_power_off(&client->dev, ssdd, mt9t031->clk);
 	}
 
-- 
1.7.2.5

[PATCH 04/24] V4L2: fix Oops on rmmod path

[Guennadi Liakhovetski]

v4l2_async_cleanup() clears the sd->dev pointer, avoid dereferencing it in
v4l2_async_unregister().

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/v4l2-core/v4l2-async.c |   18 ++++++------------
 1 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 98db2e0..5d6b428 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -123,16 +123,6 @@ static void v4l2_async_cleanup(struct v4l2_async_subdev_list *asdl)
 	sd->dev = NULL;
 }
 
-static void v4l2_async_unregister(struct v4l2_async_subdev_list *asdl)
-{
-	struct v4l2_subdev *sd = v4l2_async_to_subdev(asdl);
-
-	v4l2_async_cleanup(asdl);
-
-	/* If we handled USB devices, we'd have to lock the parent too */
-	device_release_driver(sd->dev);
-}
-
 int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
 				 struct v4l2_async_notifier *notifier)
 {
@@ -203,9 +193,13 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 	list_for_each_entry_safe(asdl, tmp, &notifier->done, list) {
 		if (dev) {
 			struct v4l2_subdev *sd = v4l2_async_to_subdev(asdl);
-			dev[i++] = get_device(sd->dev);
+			dev[i] = get_device(sd->dev);
 		}
-		v4l2_async_unregister(asdl);
+		v4l2_async_cleanup(asdl);
+
+		/* If we handled USB devices, we'd have to lock the parent too */
+		if (dev)
+			device_release_driver(dev[i++]);
 
 		if (notifier->unbind)
 			notifier->unbind(notifier, asdl);
-- 
1.7.2.5

[PATCH 05/24] V4L2: allow dummy file-handle initialisation by v4l2_fh_init()

[Guennadi Liakhovetski]

v4l2_fh_init() can be used to initialise dummy file-handles with vdev ==
NULL.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/v4l2-core/v4l2-fh.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
index e57c002..7ae608b 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -33,10 +33,12 @@
 void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
 {
 	fh->vdev = vdev;
-	/* Inherit from video_device. May be overridden by the driver. */
-	fh->ctrl_handler = vdev->ctrl_handler;
+	if (vdev) {
+		/* Inherit from video_device. May be overridden by the driver. */
+		fh->ctrl_handler = vdev->ctrl_handler;
+		set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
+	}
 	INIT_LIST_HEAD(&fh->list);
-	set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
 	fh->prio = V4L2_PRIORITY_UNSET;
 	init_waitqueue_head(&fh->wait);
 	INIT_LIST_HEAD(&fh->available);
-- 
1.7.2.5

[PATCH 06/24] V4L2: add a common V4L2 subdevice platform data type

[Guennadi Liakhovetski]

This struct shall be used by subdevice drivers to pass per-subdevice data,
e.g. power supplies, to generic V4L2 methods, at the same time allowing
optional host-specific extensions via the host_priv pointer. To avoid
having to pass two pointers to those methods, add a pointer to this new
struct to struct v4l2_subdev.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 include/media/v4l2-subdev.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index eb91366..b15c6e0 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -561,6 +561,17 @@ struct v4l2_subdev_internal_ops {
 /* Set this flag if this subdev generates events. */
 #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
 
+struct regulator_bulk_data;
+
+struct v4l2_subdev_platform_data {
+	/* Optional regulators uset to power on/off the subdevice */
+	struct regulator_bulk_data *regulators;
+	int num_regulators;
+
+	/* Per-subdevice data, specific for a certain video host device */
+	void *host_priv;
+};
+
 /* Each instance of a subdev driver should create this struct, either
    stand-alone or embedded in a larger struct.
  */
@@ -589,6 +600,8 @@ struct v4l2_subdev {
 	/* pointer to the physical device */
 	struct device *dev;
 	struct v4l2_async_subdev_list asdl;
+	/* common part of subdevice platform data */
+	struct v4l2_subdev_platform_data *pdata;
 };
 
 static inline struct v4l2_subdev *v4l2_async_to_subdev(
-- 
1.7.2.5

[PATCH 07/24] soc-camera: switch to using the new struct v4l2_subdev_platform_data

[Guennadi Liakhovetski]

This prepares soc-camera to use struct v4l2_subdev_platform_data for its
subdevice-facing API, which would allow subdevice driver re-use.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/platform/soc_camera/soc_camera.c |   20 ++++++++++----------
 include/media/soc_camera.h                     |   17 +++++++++--------
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index a790f81..c06e660 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -76,8 +76,8 @@ int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc *ssdd,
 		dev_err(dev, "Cannot enable clock: %d\n", ret);
 		return ret;
 	}
-	ret = regulator_bulk_enable(ssdd->num_regulators,
-					ssdd->regulators);
+	ret = regulator_bulk_enable(ssdd->sd_pdata.num_regulators,
+				    ssdd->sd_pdata.regulators);
 	if (ret < 0) {
 		dev_err(dev, "Cannot enable regulators\n");
 		goto eregenable;
@@ -95,8 +95,8 @@ int soc_camera_power_on(struct device *dev, struct soc_camera_subdev_desc *ssdd,
 	return 0;
 
 epwron:
-	regulator_bulk_disable(ssdd->num_regulators,
-			       ssdd->regulators);
+	regulator_bulk_disable(ssdd->sd_pdata.num_regulators,
+			       ssdd->sd_pdata.regulators);
 eregenable:
 	if (clk)
 		v4l2_clk_disable(clk);
@@ -120,8 +120,8 @@ int soc_camera_power_off(struct device *dev, struct soc_camera_subdev_desc *ssdd
 		}
 	}
 
-	err = regulator_bulk_disable(ssdd->num_regulators,
-				     ssdd->regulators);
+	err = regulator_bulk_disable(ssdd->sd_pdata.num_regulators,
+				     ssdd->sd_pdata.regulators);
 	if (err < 0) {
 		dev_err(dev, "Cannot disable regulators\n");
 		ret = ret ? : err;
@@ -137,8 +137,8 @@ EXPORT_SYMBOL(soc_camera_power_off);
 int soc_camera_power_init(struct device *dev, struct soc_camera_subdev_desc *ssdd)
 {
 
-	return devm_regulator_bulk_get(dev, ssdd->num_regulators,
-				       ssdd->regulators);
+	return devm_regulator_bulk_get(dev, ssdd->sd_pdata.num_regulators,
+				       ssdd->sd_pdata.regulators);
 }
 EXPORT_SYMBOL(soc_camera_power_init);
 
@@ -2033,8 +2033,8 @@ static int soc_camera_pdrv_probe(struct platform_device *pdev)
 	 * in soc_camera_async_bind(). Also note, that in that case regulators
 	 * are attached to the I2C device and not to the camera platform device.
 	 */
-	ret = devm_regulator_bulk_get(&pdev->dev, ssdd->num_regulators,
-				      ssdd->regulators);
+	ret = devm_regulator_bulk_get(&pdev->dev, ssdd->sd_pdata.num_regulators,
+				      ssdd->sd_pdata.regulators);
 	if (ret < 0)
 		return ret;
 
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index 2d3c939..1331278 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -146,10 +146,6 @@ struct soc_camera_subdev_desc {
 	/* sensor driver private platform data */
 	void *drv_priv;
 
-	/* Optional regulators that have to be managed on power on/off events */
-	struct regulator_bulk_data *regulators;
-	int num_regulators;
-
 	/* Optional callbacks to power on or off and reset the sensor */
 	int (*power)(struct device *, int);
 	int (*reset)(struct device *);
@@ -162,6 +158,9 @@ struct soc_camera_subdev_desc {
 	int (*set_bus_param)(struct soc_camera_subdev_desc *, unsigned long flags);
 	unsigned long (*query_bus_param)(struct soc_camera_subdev_desc *);
 	void (*free_bus)(struct soc_camera_subdev_desc *);
+
+	/* Optional regulators that have to be managed on power on/off events */
+	struct v4l2_subdev_platform_data sd_pdata;
 };
 
 struct soc_camera_host_desc {
@@ -202,10 +201,6 @@ struct soc_camera_link {
 
 	void *priv;
 
-	/* Optional regulators that have to be managed on power on/off events */
-	struct regulator_bulk_data *regulators;
-	int num_regulators;
-
 	/* Optional callbacks to power on or off and reset the sensor */
 	int (*power)(struct device *, int);
 	int (*reset)(struct device *);
@@ -218,6 +213,12 @@ struct soc_camera_link {
 	unsigned long (*query_bus_param)(struct soc_camera_link *);
 	void (*free_bus)(struct soc_camera_link *);
 
+	/* Optional regulators that have to be managed on power on/off events */
+	struct regulator_bulk_data *regulators;
+	int num_regulators;
+
+	void *host_priv;
+
 	/*
 	 * Host part - keep at bottom and compatible to
 	 * struct soc_camera_host_desc
-- 
1.7.2.5

[PATCH 08/24] ARM: update all soc-camera users to new platform data layout

[Guennadi Liakhovetski]

This patch moves almost all ARM soc-camera users towards re-using subdevice
drivers. Only mach-shmobile/board-mackerel.c will be updated separately,
together with other soc-camera-platform users.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 arch/arm/mach-at91/board-sam9m10g45ek.c        |   19 ++++++----
 arch/arm/mach-imx/mach-imx27_visstrim_m10.c    |   17 ++++++---
 arch/arm/mach-imx/mach-mx27_3ds.c              |   21 +++++++----
 arch/arm/mach-imx/mach-mx31_3ds.c              |   23 ++++++++----
 arch/arm/mach-imx/mach-mx35_3ds.c              |   12 ++++---
 arch/arm/mach-imx/mach-pcm037.c                |   28 ++++++++++-----
 arch/arm/mach-imx/mx31moboard-marxbot.c        |   17 ++++++---
 arch/arm/mach-imx/mx31moboard-smartbot.c       |   17 ++++++---
 arch/arm/mach-omap1/board-ams-delta.c          |   17 ++++++---
 arch/arm/mach-pxa/em-x270.c                    |   15 +++++---
 arch/arm/mach-pxa/ezx.c                        |   36 ++++++++++++-------
 arch/arm/mach-pxa/mioa701.c                    |   11 ++++--
 arch/arm/mach-pxa/palmz72.c                    |   21 +++++++----
 arch/arm/mach-pxa/pcm990-baseboard.c           |   44 ++++++++++++++---------
 arch/arm/mach-shmobile/board-ap4evb.c          |    5 ++-
 arch/arm/mach-shmobile/board-armadillo800eva.c |   17 ++++++---
 16 files changed, 205 insertions(+), 115 deletions(-)

diff --git a/arch/arm/mach-at91/board-sam9m10g45ek.c b/arch/arm/mach-at91/board-sam9m10g45ek.c
index 2a94896..8c768dd 100644
--- a/arch/arm/mach-at91/board-sam9m10g45ek.c
+++ b/arch/arm/mach-at91/board-sam9m10g45ek.c
@@ -200,7 +200,7 @@ static struct isi_platform_data __initdata isi_data = {
  */
 #if defined(CONFIG_SOC_CAMERA_OV2640) || \
 	defined(CONFIG_SOC_CAMERA_OV2640_MODULE)
-static unsigned long isi_camera_query_bus_param(struct soc_camera_link *link)
+static unsigned long isi_camera_query_bus_param(struct soc_camera_subdev_desc *desc)
 {
 	/* ISI board for ek using default 8-bits connection */
 	return SOCAM_DATAWIDTH_8;
@@ -229,12 +229,17 @@ static struct i2c_board_info i2c_camera = {
 	I2C_BOARD_INFO("ov2640", 0x30),
 };
 
-static struct soc_camera_link iclink_ov2640 = {
-	.bus_id			= 0,
-	.board_info		= &i2c_camera,
-	.i2c_adapter_id		= 0,
-	.power			= i2c_camera_power,
-	.query_bus_param	= isi_camera_query_bus_param,
+static struct soc_camera_desc iclink_ov2640 = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv	= &iclink_ov2640,
+		.power			= i2c_camera_power,
+		.query_bus_param	= isi_camera_query_bus_param,
+	},
+	.host_desc	= {
+		.bus_id			= 0,
+		.board_info		= &i2c_camera,
+		.i2c_adapter_id		= 0,
+	},
 };
 
 static struct platform_device isi_ov2640 = {
diff --git a/arch/arm/mach-imx/mach-imx27_visstrim_m10.c b/arch/arm/mach-imx/mach-imx27_visstrim_m10.c
index 29ac8ee6..686138c 100644
--- a/arch/arm/mach-imx/mach-imx27_visstrim_m10.c
+++ b/arch/arm/mach-imx/mach-imx27_visstrim_m10.c
@@ -224,12 +224,17 @@ static struct i2c_board_info visstrim_i2c_camera =  {
 	I2C_BOARD_INFO("tvp5150", 0x5d),
 };
 
-static struct soc_camera_link iclink_tvp5150 = {
-	.bus_id         = 0,
-	.board_info     = &visstrim_i2c_camera,
-	.i2c_adapter_id = 0,
-	.power = visstrim_camera_power,
-	.reset = visstrim_camera_reset,
+static struct soc_camera_desc iclink_tvp5150 = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv	= &iclink_tvp5150,
+		.power			= visstrim_camera_power,
+		.reset			= visstrim_camera_reset,
+	},
+	.host_desc	= {
+		.bus_id			= 0,
+		.board_info		= &visstrim_i2c_camera,
+		.i2c_adapter_id		= 0,
+	},
 };
 
 static struct mx2_camera_platform_data visstrim_camera = {
diff --git a/arch/arm/mach-imx/mach-mx27_3ds.c b/arch/arm/mach-imx/mach-mx27_3ds.c
index 25b3e4c..2c7d7e8 100644
--- a/arch/arm/mach-imx/mach-mx27_3ds.c
+++ b/arch/arm/mach-imx/mach-mx27_3ds.c
@@ -397,13 +397,20 @@ static struct regulator_bulk_data mx27_3ds_camera_regs[] = {
 	{ .supply = "cmos_2v8" },
 };
 
-static struct soc_camera_link iclink_ov2640 = {
-	.bus_id		= 0,
-	.board_info	= &mx27_3ds_i2c_camera,
-	.i2c_adapter_id	= 0,
-	.power		= mx27_3ds_camera_power,
-	.regulators	= mx27_3ds_camera_regs,
-	.num_regulators	= ARRAY_SIZE(mx27_3ds_camera_regs),
+static struct soc_camera_desc iclink_ov2640 = {
+	.subdev_desc	= {
+		.power		= mx27_3ds_camera_power,
+		.sd_pdata	= {
+			.regulators	= mx27_3ds_camera_regs,
+			.num_regulators	= ARRAY_SIZE(mx27_3ds_camera_regs),
+			.host_priv	= &iclink_ov2640,
+		},
+	},
+	.host_desc	= {
+		.bus_id		= 0,
+		.i2c_adapter_id	= 0,
+		.board_info	= &mx27_3ds_i2c_camera,
+	},
 };
 
 static struct platform_device mx27_3ds_ov2640 = {
diff --git a/arch/arm/mach-imx/mach-mx31_3ds.c b/arch/arm/mach-imx/mach-mx31_3ds.c
index 1ed9161..d49cde9 100644
--- a/arch/arm/mach-imx/mach-mx31_3ds.c
+++ b/arch/arm/mach-imx/mach-mx31_3ds.c
@@ -235,13 +235,20 @@ static struct regulator_bulk_data mx31_3ds_camera_regs[] = {
 	{ .supply = "cmos_2v8" },
 };
 
-static struct soc_camera_link iclink_ov2640 = {
-	.bus_id		= 0,
-	.board_info	= &mx31_3ds_i2c_camera,
-	.i2c_adapter_id	= 0,
-	.power		= mx31_3ds_camera_power,
-	.regulators	= mx31_3ds_camera_regs,
-	.num_regulators	= ARRAY_SIZE(mx31_3ds_camera_regs),
+static struct soc_camera_desc iclink_ov2640 = {
+	.subdev_desc	= {
+		.power		= mx31_3ds_camera_power,
+		.sd_pdata	= {
+			.regulators	= mx31_3ds_camera_regs,
+			.num_regulators	= ARRAY_SIZE(mx31_3ds_camera_regs),
+			.host_priv	= &iclink_ov2640,
+		},
+	},
+	.host_desc	= {
+		.bus_id		= 0,
+		.board_info	= &mx31_3ds_i2c_camera,
+		.i2c_adapter_id	= 0,
+	},
 };
 
 static struct platform_device mx31_3ds_ov2640 = {
@@ -747,7 +754,7 @@ static void __init mx31_3ds_init(void)
 				 ARRAY_SIZE(mx31_3ds_camera_gpios));
 	if (ret) {
 		pr_err("Failed to request camera gpios");
-		iclink_ov2640.power = NULL;
+		iclink_ov2640.subdev_desc.power = NULL;
 	}
 
 	mx31_3ds_init_camera();
diff --git a/arch/arm/mach-imx/mach-mx35_3ds.c b/arch/arm/mach-imx/mach-mx35_3ds.c
index a42f4f0..958748b 100644
--- a/arch/arm/mach-imx/mach-mx35_3ds.c
+++ b/arch/arm/mach-imx/mach-mx35_3ds.c
@@ -294,11 +294,13 @@ static struct i2c_board_info mx35_3ds_i2c_camera = {
 	I2C_BOARD_INFO("ov2640", 0x30),
 };
 
-static struct soc_camera_link iclink_ov2640 = {
-	.bus_id		= 0,
-	.board_info	= &mx35_3ds_i2c_camera,
-	.i2c_adapter_id	= 0,
-	.power		= NULL,
+static struct soc_camera_desc iclink_ov2640 = {
+	.subdev_desc.sd_pdata.host_priv	= &iclink_ov2640,
+	.host_desc	= {
+		.bus_id		= 0,
+		.board_info	= &mx35_3ds_i2c_camera,
+		.i2c_adapter_id	= 0,
+	},
 };
 
 static struct platform_device mx35_3ds_ov2640 = {
diff --git a/arch/arm/mach-imx/mach-pcm037.c b/arch/arm/mach-imx/mach-pcm037.c
index bc0261e..ef55ac1 100644
--- a/arch/arm/mach-imx/mach-pcm037.c
+++ b/arch/arm/mach-imx/mach-pcm037.c
@@ -303,17 +303,25 @@ static struct i2c_board_info pcm037_i2c_camera[] = {
 	},
 };
 
-static struct soc_camera_link iclink_mt9v022 = {
-	.bus_id		= 0,		/* Must match with the camera ID */
-	.board_info	= &pcm037_i2c_camera[1],
-	.i2c_adapter_id	= 2,
+static struct soc_camera_desc iclink_mt9v022 = {
+	.subdev_desc.sd_pdata.host_priv	= &iclink_mt9v022,
+	.host_desc	= {
+		.bus_id		= 0,		/* Must match with the camera ID */
+		.board_info	= &pcm037_i2c_camera[1],
+		.i2c_adapter_id	= 2,
+	},
 };
 
-static struct soc_camera_link iclink_mt9t031 = {
-	.bus_id		= 0,		/* Must match with the camera ID */
-	.power		= pcm037_camera_power,
-	.board_info	= &pcm037_i2c_camera[0],
-	.i2c_adapter_id	= 2,
+static struct soc_camera_desc iclink_mt9t031 = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &iclink_mt9t031,
+		.power		= pcm037_camera_power,
+	},
+	.host_desc	= {
+		.bus_id		= 0,		/* Must match with the camera ID */
+		.board_info	= &pcm037_i2c_camera[0],
+		.i2c_adapter_id	= 2,
+	},
 };
 
 static struct i2c_board_info pcm037_i2c_devices[] = {
@@ -653,7 +661,7 @@ static void __init pcm037_init(void)
 	if (!ret)
 		gpio_direction_output(IOMUX_TO_GPIO(MX31_PIN_CSI_D5), 1);
 	else
-		iclink_mt9t031.power = NULL;
+		iclink_mt9t031.subdev_desc.power = NULL;
 
 	pcm037_init_camera();
 
diff --git a/arch/arm/mach-imx/mx31moboard-marxbot.c b/arch/arm/mach-imx/mx31moboard-marxbot.c
index a4f43e9..9a1103b 100644
--- a/arch/arm/mach-imx/mx31moboard-marxbot.c
+++ b/arch/arm/mach-imx/mx31moboard-marxbot.c
@@ -168,12 +168,17 @@ static struct i2c_board_info marxbot_i2c_devices[] = {
 	},
 };
 
-static struct soc_camera_link base_iclink = {
-	.bus_id		= 0,		/* Must match with the camera ID */
-	.power		= marxbot_basecam_power,
-	.reset		= marxbot_basecam_reset,
-	.board_info	= &marxbot_i2c_devices[0],
-	.i2c_adapter_id	= 0,
+static struct soc_camera_desc base_iclink = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &base_iclink,
+		.power		= marxbot_basecam_power,
+		.reset		= marxbot_basecam_reset,
+	},
+	.host_desc	= {
+		.bus_id		= 0,		/* Must match with the camera ID */
+		.board_info	= &marxbot_i2c_devices[0],
+		.i2c_adapter_id	= 0,
+	},
 };
 
 static struct platform_device marxbot_camera[] = {
diff --git a/arch/arm/mach-imx/mx31moboard-smartbot.c b/arch/arm/mach-imx/mx31moboard-smartbot.c
index 04ae45d..0656dd5 100644
--- a/arch/arm/mach-imx/mx31moboard-smartbot.c
+++ b/arch/arm/mach-imx/mx31moboard-smartbot.c
@@ -78,12 +78,17 @@ static struct i2c_board_info smartbot_i2c_devices[] = {
 	},
 };
 
-static struct soc_camera_link base_iclink = {
-	.bus_id		= 0,		/* Must match with the camera ID */
-	.power		= smartbot_cam_power,
-	.reset		= smartbot_cam_reset,
-	.board_info	= &smartbot_i2c_devices[0],
-	.i2c_adapter_id	= 0,
+static struct soc_camera_desc base_iclink = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &base_iclink,
+		.power		= smartbot_cam_power,
+		.reset		= smartbot_cam_reset,
+	},
+	.host_desc	= {
+		.bus_id		= 0,		/* Must match with the camera ID */
+		.board_info	= &smartbot_i2c_devices[0],
+		.i2c_adapter_id	= 0,
+	},
 };
 
 static struct platform_device smartbot_camera[] = {
diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c
index 2aab761..019c62e 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -423,12 +423,17 @@ static int ams_delta_camera_power(struct device *dev, int power)
 #define ams_delta_camera_power	NULL
 #endif
 
-static struct soc_camera_link ams_delta_iclink = {
-	.bus_id         = 0,	/* OMAP1 SoC camera bus */
-	.i2c_adapter_id = 1,
-	.board_info     = &ams_delta_camera_board_info[0],
-	.module_name    = "ov6650",
-	.power		= ams_delta_camera_power,
+static struct soc_camera_desc ams_delta_iclink = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &ams_delta_iclink,
+		.power		= ams_delta_camera_power,
+	},
+	.host_desc	= {
+		.bus_id         = 0,	/* OMAP1 SoC camera bus */
+		.i2c_adapter_id = 1,
+		.board_info     = &ams_delta_camera_board_info[0],
+		.module_name    = "ov6650",
+	},
 };
 
 static struct platform_device ams_delta_camera_device = {
diff --git a/arch/arm/mach-pxa/em-x270.c b/arch/arm/mach-pxa/em-x270.c
index 446563a..4df154e 100644
--- a/arch/arm/mach-pxa/em-x270.c
+++ b/arch/arm/mach-pxa/em-x270.c
@@ -1007,11 +1007,16 @@ static struct i2c_board_info em_x270_i2c_cam_info[] = {
 	},
 };
 
-static struct soc_camera_link iclink = {
-	.bus_id		= 0,
-	.power		= em_x270_sensor_power,
-	.board_info	= &em_x270_i2c_cam_info[0],
-	.i2c_adapter_id	= 0,
+static struct soc_camera_desc iclink = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &iclink,
+		.power		= em_x270_sensor_power,
+	},
+	.host_desc	= {
+		.bus_id		= 0,
+		.board_info	= &em_x270_i2c_cam_info[0],
+		.i2c_adapter_id	= 0,
+	},
 };
 
 static struct platform_device em_x270_camera = {
diff --git a/arch/arm/mach-pxa/ezx.c b/arch/arm/mach-pxa/ezx.c
index dca1070..607cf6c 100644
--- a/arch/arm/mach-pxa/ezx.c
+++ b/arch/arm/mach-pxa/ezx.c
@@ -750,13 +750,18 @@ static struct i2c_board_info a780_camera_i2c_board_info = {
 	I2C_BOARD_INFO("mt9m111", 0x5d),
 };
 
-static struct soc_camera_link a780_iclink = {
-	.bus_id         = 0,
-	.flags          = SOCAM_SENSOR_INVERT_PCLK,
-	.i2c_adapter_id = 0,
-	.board_info     = &a780_camera_i2c_board_info,
-	.power          = a780_camera_power,
-	.reset          = a780_camera_reset,
+static struct soc_camera_desc a780_iclink = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &a780_iclink,
+		.flags          = SOCAM_SENSOR_INVERT_PCLK,
+		.power          = a780_camera_power,
+		.reset          = a780_camera_reset,
+	},
+	.host_desc	= {
+		.bus_id         = 0,
+		.i2c_adapter_id = 0,
+		.board_info     = &a780_camera_i2c_board_info,
+	},
 };
 
 static struct platform_device a780_camera = {
@@ -1025,12 +1030,17 @@ static struct i2c_board_info a910_camera_i2c_board_info = {
 	I2C_BOARD_INFO("mt9m111", 0x5d),
 };
 
-static struct soc_camera_link a910_iclink = {
-	.bus_id         = 0,
-	.i2c_adapter_id = 0,
-	.board_info     = &a910_camera_i2c_board_info,
-	.power          = a910_camera_power,
-	.reset          = a910_camera_reset,
+static struct soc_camera_desc a910_iclink = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &a910_iclink,
+		.power          = a910_camera_power,
+		.reset          = a910_camera_reset,
+	},
+	.host_desc	= {
+		.bus_id         = 0,
+		.i2c_adapter_id = 0,
+		.board_info     = &a910_camera_i2c_board_info,
+	},
 };
 
 static struct platform_device a910_camera = {
diff --git a/arch/arm/mach-pxa/mioa701.c b/arch/arm/mach-pxa/mioa701.c
index f8979b9..2e140ab 100644
--- a/arch/arm/mach-pxa/mioa701.c
+++ b/arch/arm/mach-pxa/mioa701.c
@@ -631,10 +631,13 @@ static struct i2c_board_info mioa701_i2c_devices[] = {
 	},
 };
 
-static struct soc_camera_link iclink = {
-	.bus_id		= 0, /* Match id in pxa27x_device_camera in device.c */
-	.board_info	= &mioa701_i2c_devices[0],
-	.i2c_adapter_id	= 0,
+static struct soc_camera_desc iclink = {
+	.subdev_desc.sd_pdata.host_priv = &iclink,
+	.host_desc	= {
+		.bus_id		= 0, /* Match id in pxa27x_device_camera in device.c */
+		.board_info	= &mioa701_i2c_devices[0],
+		.i2c_adapter_id	= 0,
+	},
 };
 
 struct i2c_pxa_platform_data i2c_pdata = {
diff --git a/arch/arm/mach-pxa/palmz72.c b/arch/arm/mach-pxa/palmz72.c
index 18b7fcd..32858fa 100644
--- a/arch/arm/mach-pxa/palmz72.c
+++ b/arch/arm/mach-pxa/palmz72.c
@@ -306,14 +306,19 @@ static int palmz72_camera_reset(struct device *dev)
 	return 0;
 }
 
-static struct soc_camera_link palmz72_iclink = {
-	.bus_id		= 0, /* Match id in pxa27x_device_camera in device.c */
-	.board_info	= &palmz72_i2c_device[0],
-	.i2c_adapter_id	= 0,
-	.module_name	= "ov96xx",
-	.power		= &palmz72_camera_power,
-	.reset		= &palmz72_camera_reset,
-	.flags		= SOCAM_DATAWIDTH_8,
+static struct soc_camera_desc palmz72_iclink = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &palmz72_iclink,
+		.power		= &palmz72_camera_power,
+		.reset		= &palmz72_camera_reset,
+		.flags		= SOCAM_DATAWIDTH_8,
+	},
+	.host_desc	= {
+		.bus_id		= 0, /* Match id in pxa27x_device_camera in device.c */
+		.board_info	= &palmz72_i2c_device[0],
+		.i2c_adapter_id	= 0,
+		.module_name	= "ov96xx",
+	},
 };
 
 static struct i2c_gpio_platform_data palmz72_i2c_bus_data = {
diff --git a/arch/arm/mach-pxa/pcm990-baseboard.c b/arch/arm/mach-pxa/pcm990-baseboard.c
index fb7f1d1..ab299f0 100644
--- a/arch/arm/mach-pxa/pcm990-baseboard.c
+++ b/arch/arm/mach-pxa/pcm990-baseboard.c
@@ -415,7 +415,7 @@ static struct pca953x_platform_data pca9536_data = {
 
 static int gpio_bus_switch = -EINVAL;
 
-static int pcm990_camera_set_bus_param(struct soc_camera_link *link,
+static int pcm990_camera_set_bus_param(struct soc_camera_subdev_desc *desc,
 				       unsigned long flags)
 {
 	if (gpio_bus_switch < 0) {
@@ -433,7 +433,7 @@ static int pcm990_camera_set_bus_param(struct soc_camera_link *link,
 	return 0;
 }
 
-static unsigned long pcm990_camera_query_bus_param(struct soc_camera_link *link)
+static unsigned long pcm990_camera_query_bus_param(struct soc_camera_subdev_desc *desc)
 {
 	int ret;
 
@@ -451,7 +451,7 @@ static unsigned long pcm990_camera_query_bus_param(struct soc_camera_link *link)
 		return SOCAM_DATAWIDTH_10;
 }
 
-static void pcm990_camera_free_bus(struct soc_camera_link *link)
+static void pcm990_camera_free_bus(struct soc_camera_subdev_desc *desc)
 {
 	if (gpio_bus_switch < 0)
 		return;
@@ -481,22 +481,32 @@ static struct i2c_board_info pcm990_camera_i2c[] = {
 	},
 };
 
-static struct soc_camera_link iclink[] = {
+static struct soc_camera_desc iclink[] = {
 	{
-		.bus_id			= 0, /* Must match with the camera ID */
-		.board_info		= &pcm990_camera_i2c[0],
-		.priv			= &mt9v022_pdata,
-		.i2c_adapter_id		= 0,
-		.query_bus_param	= pcm990_camera_query_bus_param,
-		.set_bus_param		= pcm990_camera_set_bus_param,
-		.free_bus		= pcm990_camera_free_bus,
+		.subdev_desc	= {
+			.sd_pdata.host_priv	= &iclink[0],
+			.query_bus_param	= pcm990_camera_query_bus_param,
+			.set_bus_param		= pcm990_camera_set_bus_param,
+			.free_bus		= pcm990_camera_free_bus,
+			.drv_priv		= &mt9v022_pdata,
+		},
+		.host_desc	= {
+			.bus_id			= 0, /* Must match with the camera ID */
+			.board_info		= &pcm990_camera_i2c[0],
+			.i2c_adapter_id		= 0,
+		},
 	}, {
-		.bus_id			= 0, /* Must match with the camera ID */
-		.board_info		= &pcm990_camera_i2c[1],
-		.i2c_adapter_id		= 0,
-		.query_bus_param	= pcm990_camera_query_bus_param,
-		.set_bus_param		= pcm990_camera_set_bus_param,
-		.free_bus		= pcm990_camera_free_bus,
+		.subdev_desc	= {
+			.sd_pdata.host_priv	= &iclink[1],
+			.query_bus_param	= pcm990_camera_query_bus_param,
+			.set_bus_param		= pcm990_camera_set_bus_param,
+			.free_bus		= pcm990_camera_free_bus,
+		},
+		.host_desc	= {
+			.bus_id			= 0, /* Must match with the camera ID */
+			.board_info		= &pcm990_camera_i2c[1],
+			.i2c_adapter_id		= 0,
+		},
 	},
 };
 
diff --git a/arch/arm/mach-shmobile/board-ap4evb.c b/arch/arm/mach-shmobile/board-ap4evb.c
index 450e06b..3fcfa77 100644
--- a/arch/arm/mach-shmobile/board-ap4evb.c
+++ b/arch/arm/mach-shmobile/board-ap4evb.c
@@ -873,7 +873,10 @@ static struct platform_device leds_device = {
 };
 
 /* I2C */
-static struct soc_camera_subdev_desc imx074_desc;
+static struct soc_camera_subdev_desc imx074_desc = {
+	.sd_pdata.host_priv = &imx074_desc,
+};
+
 static struct i2c_board_info i2c0_devices[] = {
 	{
 		I2C_BOARD_INFO("ak4643", 0x13),
diff --git a/arch/arm/mach-shmobile/board-armadillo800eva.c b/arch/arm/mach-shmobile/board-armadillo800eva.c
index f2ec077..247e945 100644
--- a/arch/arm/mach-shmobile/board-armadillo800eva.c
+++ b/arch/arm/mach-shmobile/board-armadillo800eva.c
@@ -727,12 +727,17 @@ static struct mt9t112_camera_info mt9t111_info = {
 	.divider = { 16, 0, 0, 7, 0, 10, 14, 7, 7 },
 };
 
-static struct soc_camera_link mt9t111_link = {
-	.i2c_adapter_id	= 0,
-	.bus_id		= 0,
-	.board_info	= &i2c_camera_mt9t111,
-	.power		= mt9t111_power,
-	.priv		= &mt9t111_info,
+static struct soc_camera_desc mt9t111_link = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &mt9t111_link,
+		.power		= mt9t111_power,
+		.drv_priv	= &mt9t111_info,
+	},
+	.host_desc	= {
+		.i2c_adapter_id	= 0,
+		.bus_id		= 0,
+		.board_info	= &i2c_camera_mt9t111,
+	},
 };
 
 static struct platform_device camera_device = {
-- 
1.7.2.5

[PATCH 09/24] SH: update all soc-camera users to new platform data layout

[Guennadi Liakhovetski]

This patch moves almost all SH soc-camera users towards re-using subdevice
drivers. Only mach-ap325rxa/setup.c will be updated separately, together
with other soc-camera-platform users.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 arch/sh/boards/mach-ecovec24/setup.c |   51 ++++++++++++++++++++++------------
 arch/sh/boards/mach-kfr2r09/setup.c  |   15 ++++++---
 arch/sh/boards/mach-migor/setup.c    |   30 +++++++++++++------
 3 files changed, 63 insertions(+), 33 deletions(-)

diff --git a/arch/sh/boards/mach-ecovec24/setup.c b/arch/sh/boards/mach-ecovec24/setup.c
index aaff767..51e25e1 100644
--- a/arch/sh/boards/mach-ecovec24/setup.c
+++ b/arch/sh/boards/mach-ecovec24/setup.c
@@ -809,12 +809,17 @@ static struct tw9910_video_info tw9910_info = {
 	.mpout		= TW9910_MPO_FIELD,
 };
 
-static struct soc_camera_link tw9910_link = {
-	.i2c_adapter_id	= 0,
-	.bus_id		= 1,
-	.power		= tw9910_power,
-	.board_info	= &i2c_camera[0],
-	.priv		= &tw9910_info,
+static struct soc_camera_desc tw9910_link = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &tw9910_link,
+		.power		= tw9910_power,
+		.drv_priv	= &tw9910_info,
+	},
+	.host_desc	= {
+		.i2c_adapter_id	= 0,
+		.bus_id		= 1,
+		.board_info	= &i2c_camera[0],
+	},
 };
 
 /* mt9t112 */
@@ -832,12 +837,17 @@ static struct mt9t112_camera_info mt9t112_info1 = {
 	.divider = { 0x49, 0x6, 0, 6, 0, 9, 9, 6, 0 }, /* for 24MHz */
 };
 
-static struct soc_camera_link mt9t112_link1 = {
-	.i2c_adapter_id	= 0,
-	.power		= mt9t112_power1,
-	.bus_id		= 0,
-	.board_info	= &i2c_camera[1],
-	.priv		= &mt9t112_info1,
+static struct soc_camera_desc mt9t112_link1 = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &mt9t112_link1,
+		.power		= mt9t112_power1,
+		.drv_priv	= &mt9t112_info1,
+	},
+	.host_desc	= {
+		.i2c_adapter_id	= 0,
+		.bus_id		= 0,
+		.board_info	= &i2c_camera[1],
+	},
 };
 
 static int mt9t112_power2(struct device *dev, int mode)
@@ -854,12 +864,17 @@ static struct mt9t112_camera_info mt9t112_info2 = {
 	.divider = { 0x49, 0x6, 0, 6, 0, 9, 9, 6, 0 }, /* for 24MHz */
 };
 
-static struct soc_camera_link mt9t112_link2 = {
-	.i2c_adapter_id	= 1,
-	.power		= mt9t112_power2,
-	.bus_id		= 1,
-	.board_info	= &i2c_camera[2],
-	.priv		= &mt9t112_info2,
+static struct soc_camera_desc mt9t112_link2 = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &mt9t112_link2,
+		.power		= mt9t112_power2,
+		.drv_priv	= &mt9t112_info2,
+	},
+	.host_desc	= {
+		.i2c_adapter_id	= 1,
+		.bus_id		= 1,
+		.board_info	= &i2c_camera[2],
+	},
 };
 
 static struct platform_device camera_devices[] = {
diff --git a/arch/sh/boards/mach-kfr2r09/setup.c b/arch/sh/boards/mach-kfr2r09/setup.c
index ab502f12..5cb62c7 100644
--- a/arch/sh/boards/mach-kfr2r09/setup.c
+++ b/arch/sh/boards/mach-kfr2r09/setup.c
@@ -331,11 +331,16 @@ static struct rj54n1_pdata rj54n1_priv = {
 	.ioctl_high	= false,
 };
 
-static struct soc_camera_link rj54n1_link = {
-	.power		= camera_power,
-	.board_info	= &kfr2r09_i2c_camera,
-	.i2c_adapter_id	= 1,
-	.priv		= &rj54n1_priv,
+static struct soc_camera_desc rj54n1_link = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &rj54n1_link,
+		.power		= camera_power,
+		.drv_priv	= &rj54n1_priv,
+	},
+	.host_desc	= {
+		.board_info	= &kfr2r09_i2c_camera,
+		.i2c_adapter_id	= 1,
+	},
 };
 
 static struct platform_device kfr2r09_camera = {
diff --git a/arch/sh/boards/mach-migor/setup.c b/arch/sh/boards/mach-migor/setup.c
index 8b73194e..5df19d7 100644
--- a/arch/sh/boards/mach-migor/setup.c
+++ b/arch/sh/boards/mach-migor/setup.c
@@ -447,11 +447,16 @@ static struct i2c_board_info migor_i2c_camera[] = {
 
 static struct ov772x_camera_info ov7725_info;
 
-static struct soc_camera_link ov7725_link = {
-	.power		= ov7725_power,
-	.board_info	= &migor_i2c_camera[0],
-	.i2c_adapter_id	= 0,
-	.priv		= &ov7725_info,
+static struct soc_camera_desc ov7725_link = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &ov7725_link,
+		.power		= ov7725_power,
+		.drv_priv	= &ov7725_info,
+	},
+	.host_desc	= {
+		.board_info	= &migor_i2c_camera[0],
+		.i2c_adapter_id	= 0,
+	},
 };
 
 static struct tw9910_video_info tw9910_info = {
@@ -459,11 +464,16 @@ static struct tw9910_video_info tw9910_info = {
 	.mpout		= TW9910_MPO_FIELD,
 };
 
-static struct soc_camera_link tw9910_link = {
-	.power		= tw9910_power,
-	.board_info	= &migor_i2c_camera[1],
-	.i2c_adapter_id	= 0,
-	.priv		= &tw9910_info,
+static struct soc_camera_desc tw9910_link = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &tw9910_link,
+		.power		= tw9910_power,
+		.drv_priv	= &tw9910_info,
+	},
+	.host_desc	= {
+		.board_info	= &migor_i2c_camera[1],
+		.i2c_adapter_id	= 0,
+	},
 };
 
 static struct platform_device migor_camera[] = {
-- 
1.7.2.5

[PATCH 10/24] soc-camera: update soc-camera-platform & its users to a new platform data layout

[Guennadi Liakhovetski]

This patch completes removal of struct soc_camera_link by all platforms.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 arch/arm/mach-shmobile/board-mackerel.c |   23 ++++++++++------
 arch/sh/boards/mach-ap325rxa/setup.c    |   43 +++++++++++++++++++-----------
 include/media/soc_camera_platform.h     |    8 +----
 3 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
index db968a5..d6a88f8 100644
--- a/arch/arm/mach-shmobile/board-mackerel.c
+++ b/arch/arm/mach-shmobile/board-mackerel.c
@@ -1181,12 +1181,17 @@ static struct soc_camera_platform_info camera_info = {
 	.set_capture = camera_set_capture,
 };
 
-static struct soc_camera_link camera_link = {
-	.bus_id		= 0,
-	.add_device	= mackerel_camera_add,
-	.del_device	= mackerel_camera_del,
-	.module_name	= "soc_camera_platform",
-	.priv		= &camera_info,
+static struct soc_camera_desc camera_link = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &camera_link,
+		.drv_priv	= &camera_info,
+	},
+	.host_desc	= {
+		.bus_id		= 0,
+		.add_device	= mackerel_camera_add,
+		.del_device	= mackerel_camera_del,
+		.module_name	= "soc_camera_platform",
+	},
 };
 
 static struct platform_device *camera_device;
@@ -1198,13 +1203,13 @@ static void mackerel_camera_release(struct device *dev)
 
 static int mackerel_camera_add(struct soc_camera_device *icd)
 {
-	return soc_camera_platform_add(icd, &camera_device, &camera_link,
-				       mackerel_camera_release, 0);
+	return soc_camera_platform_add(icd, &camera_device,
+			&camera_link.subdev_desc, mackerel_camera_release, 0);
 }
 
 static void mackerel_camera_del(struct soc_camera_device *icd)
 {
-	soc_camera_platform_del(icd, camera_device, &camera_link);
+	soc_camera_platform_del(icd, camera_device, &camera_link.subdev_desc);
 }
 
 static struct sh_mobile_ceu_info sh_mobile_ceu_info = {
diff --git a/arch/sh/boards/mach-ap325rxa/setup.c b/arch/sh/boards/mach-ap325rxa/setup.c
index 5620e33..4a1be94 100644
--- a/arch/sh/boards/mach-ap325rxa/setup.c
+++ b/arch/sh/boards/mach-ap325rxa/setup.c
@@ -351,12 +351,17 @@ static struct soc_camera_platform_info camera_info = {
 	.set_capture = camera_set_capture,
 };
 
-static struct soc_camera_link camera_link = {
-	.bus_id		= 0,
-	.add_device	= ap325rxa_camera_add,
-	.del_device	= ap325rxa_camera_del,
-	.module_name	= "soc_camera_platform",
-	.priv		= &camera_info,
+static struct soc_camera_desc camera_link = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &camera_link,
+		.drv_priv	= &camera_info,
+	},
+	.host_desc	= {
+		.bus_id		= 0,
+		.module_name	= "soc_camera_platform",
+		.add_device	= ap325rxa_camera_add,
+		.del_device	= ap325rxa_camera_del,
+	},
 };
 
 static struct platform_device *camera_device;
@@ -368,21 +373,22 @@ static void ap325rxa_camera_release(struct device *dev)
 
 static int ap325rxa_camera_add(struct soc_camera_device *icd)
 {
-	int ret = soc_camera_platform_add(icd, &camera_device, &camera_link,
-					  ap325rxa_camera_release, 0);
+	int ret = soc_camera_platform_add(icd, &camera_device,
+			&camera_link.subdev_desc, ap325rxa_camera_release, 0);
 	if (ret < 0)
 		return ret;
 
 	ret = camera_probe();
 	if (ret < 0)
-		soc_camera_platform_del(icd, camera_device, &camera_link);
+		soc_camera_platform_del(icd, camera_device,
+					&camera_link.subdev_desc);
 
 	return ret;
 }
 
 static void ap325rxa_camera_del(struct soc_camera_device *icd)
 {
-	soc_camera_platform_del(icd, camera_device, &camera_link);
+	soc_camera_platform_del(icd, camera_device, &camera_link.subdev_desc);
 }
 #endif /* CONFIG_I2C */
 
@@ -505,12 +511,17 @@ static struct ov772x_camera_info ov7725_info = {
 	.edgectrl	= OV772X_AUTO_EDGECTRL(0xf, 0),
 };
 
-static struct soc_camera_link ov7725_link = {
-	.bus_id		= 0,
-	.power		= ov7725_power,
-	.board_info	= &ap325rxa_i2c_camera[0],
-	.i2c_adapter_id	= 0,
-	.priv		= &ov7725_info,
+static struct soc_camera_desc ov7725_link = {
+	.subdev_desc	= {
+		.sd_pdata.host_priv = &ov7725_link,
+		.power		= ov7725_power,
+		.drv_priv	= &ov7725_info,
+	},
+	.host_desc	= {
+		.bus_id		= 0,
+		.board_info	= &ap325rxa_i2c_camera[0],
+		.i2c_adapter_id	= 0,
+	},
 };
 
 static struct platform_device ap325rxa_camera[] = {
diff --git a/include/media/soc_camera_platform.h b/include/media/soc_camera_platform.h
index 1e5065da..04aacb8 100644
--- a/include/media/soc_camera_platform.h
+++ b/include/media/soc_camera_platform.h
@@ -34,12 +34,10 @@ static inline void soc_camera_platform_release(struct platform_device **pdev)
 
 static inline int soc_camera_platform_add(struct soc_camera_device *icd,
 					  struct platform_device **pdev,
-					  struct soc_camera_link *plink,
+					  struct soc_camera_subdev_desc *ssdd,
 					  void (*release)(struct device *dev),
 					  int id)
 {
-	struct soc_camera_subdev_desc *ssdd =
-		(struct soc_camera_subdev_desc *)plink;
 	struct soc_camera_platform_info *info = ssdd->drv_priv;
 	int ret;
 
@@ -70,10 +68,8 @@ static inline int soc_camera_platform_add(struct soc_camera_device *icd,
 
 static inline void soc_camera_platform_del(const struct soc_camera_device *icd,
 					   struct platform_device *pdev,
-					   const struct soc_camera_link *plink)
+					   const struct soc_camera_subdev_desc *ssdd)
 {
-	const struct soc_camera_subdev_desc *ssdd =
-		(const struct soc_camera_subdev_desc *)plink;
 	if (&icd->sdesc->subdev_desc != ssdd || !pdev)
 		return;
 
-- 
1.7.2.5

[PATCH 11/24] soc-camera: completely remove struct soc_camera_link

[Guennadi Liakhovetski]

This updates the last user of struct soc_camera_link and finally removes it.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 Documentation/video4linux/soc-camera.txt |    2 +-
 drivers/media/usb/em28xx/em28xx-camera.c |   12 +++++--
 include/media/soc_camera.h               |   48 ------------------------------
 3 files changed, 9 insertions(+), 53 deletions(-)

diff --git a/Documentation/video4linux/soc-camera.txt b/Documentation/video4linux/soc-camera.txt
index f62fcdb..04da87a 100644
--- a/Documentation/video4linux/soc-camera.txt
+++ b/Documentation/video4linux/soc-camera.txt
@@ -85,7 +85,7 @@ respective V4L2 operations.
 Camera API
 ----------
 
-Sensor drivers can use struct soc_camera_link, typically provided by the
+Sensor drivers can use struct soc_camera_desc, typically provided by the
 platform, and used to specify to which camera host bus the sensor is connected,
 and optionally provide platform .power and .reset methods for the camera. This
 struct is provided to the camera driver via the I2C client device platform data
diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
index 73cc50a..365b601 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -43,10 +43,14 @@ static unsigned short omnivision_sensor_addrs[] = {
 };
 
 
-static struct soc_camera_link camlink = {
-	.bus_id = 0,
-	.flags = 0,
-	.module_name = "em28xx",
+static struct soc_camera_desc camlink = {
+	.subdev_desc	= {
+		.flags = 0,
+	},
+	.host_desc	= {
+		.bus_id = 0,
+		.module_name = "em28xx",
+	},
 };
 
 
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index 1331278..a2a3b4f 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -190,54 +190,6 @@ struct soc_camera_desc {
 };
 
 /* Prepare to replace this struct: don't change its layout any more! */
-struct soc_camera_link {
-	/*
-	 * Subdevice part - keep at top and compatible to
-	 * struct soc_camera_subdev_desc
-	 */
-
-	/* Per camera SOCAM_SENSOR_* bus flags */
-	unsigned long flags;
-
-	void *priv;
-
-	/* Optional callbacks to power on or off and reset the sensor */
-	int (*power)(struct device *, int);
-	int (*reset)(struct device *);
-	/*
-	 * some platforms may support different data widths than the sensors
-	 * native ones due to different data line routing. Let the board code
-	 * overwrite the width flags.
-	 */
-	int (*set_bus_param)(struct soc_camera_link *, unsigned long flags);
-	unsigned long (*query_bus_param)(struct soc_camera_link *);
-	void (*free_bus)(struct soc_camera_link *);
-
-	/* Optional regulators that have to be managed on power on/off events */
-	struct regulator_bulk_data *regulators;
-	int num_regulators;
-
-	void *host_priv;
-
-	/*
-	 * Host part - keep at bottom and compatible to
-	 * struct soc_camera_host_desc
-	 */
-
-	/* Camera bus id, used to match a camera and a bus */
-	int bus_id;
-	int i2c_adapter_id;
-	struct i2c_board_info *board_info;
-	const char *module_name;
-
-	/*
-	 * For non-I2C devices platform has to provide methods to add a device
-	 * to the system and to remove it
-	 */
-	int (*add_device)(struct soc_camera_device *);
-	void (*del_device)(struct soc_camera_device *);
-};
-
 static inline struct soc_camera_host *to_soc_camera_host(
 	const struct device *dev)
 {
-- 
1.7.2.5

[PATCH 12/24] V4L2: soc-camera: retrieve subdevice platform data from struct v4l2_subdev

[Guennadi Liakhovetski]

Instead of expecting subdevice drivers to have a standard type as their
platform data, use the new .pdata member of struct v4l2_subdev. This allows
the use of arbitrary subdevice drivers with soc-camera in asynchronous
mode.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/soc_camera/imx074.c          |    1 +
 drivers/media/platform/soc_camera/soc_camera.c |   10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/soc_camera/imx074.c b/drivers/media/i2c/soc_camera/imx074.c
index 23de859..321496a 100644
--- a/drivers/media/i2c/soc_camera/imx074.c
+++ b/drivers/media/i2c/soc_camera/imx074.c
@@ -457,6 +457,7 @@ static int imx074_probe(struct i2c_client *client,
 	priv->fmt	= &imx074_colour_fmts[0];
 
 	priv->subdev.dev = &client->dev;
+	priv->subdev.pdata = &ssdd->sd_pdata;
 
 	priv->clk = v4l2_clk_get(&client->dev, "mclk");
 	if (IS_ERR(priv->clk)) {
diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index c06e660..3113287 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1363,6 +1363,11 @@ static int soc_camera_i2c_init(struct soc_camera_device *icd,
 		return -ENODEV;
 	}
 
+	/*
+	 * Only soc-camera originated subdevice drivers can be used in
+	 * synchronous mode. They all use struct soc_camera_subdev_desc for
+	 * platform data.
+	 */
 	shd->board_info->platform_data = &sdesc->subdev_desc;
 
 	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
@@ -1438,8 +1443,9 @@ static int soc_camera_async_bound(struct v4l2_async_notifier *notifier,
 		 */
 		if (client) {
 			struct soc_camera_desc *sdesc = to_soc_camera_desc(icd);
-			struct soc_camera_subdev_desc *ssdd =
-				soc_camera_i2c_to_desc(client);
+			struct v4l2_subdev_platform_data *sd_pdata = sd->sd_pdata;
+			struct soc_camera_subdev_desc *ssdd = sd_pdata ?
+				sd_pdata->host_priv : NULL;
 			if (ssdd) {
 				memcpy(&sdesc->subdev_desc, ssdd,
 				       sizeof(sdesc->subdev_desc));
-- 
1.7.2.5

[PATCH 13/24] ARM: pcm037: convert custom GPIO-based power function to a regulator

[Guennadi Liakhovetski]

Add a fixed-voltage GPIO-enabled regulator to switch the camera on and off
instead of using a .power() platform callback.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 arch/arm/mach-imx/mach-pcm037.c |   54 ++++++++++++++++++++++++++++-----------
 1 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-imx/mach-pcm037.c b/arch/arm/mach-imx/mach-pcm037.c
index ef55ac1..f138481 100644
--- a/arch/arm/mach-imx/mach-pcm037.c
+++ b/arch/arm/mach-imx/mach-pcm037.c
@@ -288,12 +288,39 @@ static struct at24_platform_data board_eeprom = {
 	.flags = AT24_FLAG_ADDR16,
 };
 
-static int pcm037_camera_power(struct device *dev, int on)
-{
-	/* disable or enable the camera in X7 or X8 PCM970 connector */
-	gpio_set_value(IOMUX_TO_GPIO(MX31_PIN_CSI_D5), !on);
-	return 0;
-}
+/* Fixed 3.3V regulator to be used by cameras */
+static struct regulator_consumer_supply vcc_cam_consumers[] = {
+	REGULATOR_SUPPLY("vcc", "2-005d"),
+};
+
+static struct regulator_init_data vcc_cam_init_data = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+	.num_consumer_supplies  = ARRAY_SIZE(vcc_cam_consumers),
+	.consumer_supplies      = vcc_cam_consumers,
+};
+
+static struct fixed_voltage_config vcc_cam_info = {
+	.supply_name = "Camera Vcc",
+	.microvolts = 2800000,
+	.gpio = IOMUX_TO_GPIO(MX31_PIN_CSI_D5),
+	.init_data = &vcc_cam_init_data,
+};
+
+static struct platform_device vcc_cam = {
+	.name = "reg-fixed-voltage",
+	.id   = 1,
+	.dev  = {
+		.platform_data = &vcc_cam_info,
+	},
+};
+
+static struct regulator_bulk_data cam_supply[] = {
+	{
+		.supply = "vcc",
+	},
+};
 
 static struct i2c_board_info pcm037_i2c_camera[] = {
 	{
@@ -314,8 +341,11 @@ static struct soc_camera_desc iclink_mt9v022 = {
 
 static struct soc_camera_desc iclink_mt9t031 = {
 	.subdev_desc	= {
-		.sd_pdata.host_priv = &iclink_mt9t031,
-		.power		= pcm037_camera_power,
+		.sd_pdata	= {
+			.num_regulators = ARRAY_SIZE(cam_supply),
+			.regulators = cam_supply,
+			.host_priv = &iclink_mt9t031,
+		},
 	},
 	.host_desc	= {
 		.bus_id		= 0,		/* Must match with the camera ID */
@@ -445,6 +475,7 @@ err:
 static struct platform_device *devices[] __initdata = {
 	&pcm037_flash,
 	&pcm037_sram_device,
+	&vcc_cam,
 	&pcm037_mt9t031,
 	&pcm037_mt9v022,
 };
@@ -656,13 +687,6 @@ static void __init pcm037_init(void)
 	imx31_add_mx3_sdc_fb(&mx3fb_pdata);
 
 	/* CSI */
-	/* Camera power: default - off */
-	ret = gpio_request(IOMUX_TO_GPIO(MX31_PIN_CSI_D5), "mt9t031-power");
-	if (!ret)
-		gpio_direction_output(IOMUX_TO_GPIO(MX31_PIN_CSI_D5), 1);
-	else
-		iclink_mt9t031.subdev_desc.power = NULL;
-
 	pcm037_init_camera();
 
 	pcm970_sja1000_resources[1].start =
-- 
1.7.2.5

[PATCH 14/24] mx3-camera: clean up the use of platform data, add driver owner

[Guennadi Liakhovetski]

During probe verify availability of platform data before dereferencing it,
then use the stored pointer instead of re-calculating it. Also add an
.owner field to the driver object for proper module locking.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/platform/soc_camera/mx3_camera.c |   23 ++++++++++++-----------
 1 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/soc_camera/mx3_camera.c b/drivers/media/platform/soc_camera/mx3_camera.c
index 1047e3e..94203f6 100644
--- a/drivers/media/platform/soc_camera/mx3_camera.c
+++ b/drivers/media/platform/soc_camera/mx3_camera.c
@@ -1144,6 +1144,7 @@ static struct soc_camera_host_ops mx3_soc_camera_host_ops = {
 
 static int mx3_camera_probe(struct platform_device *pdev)
 {
+	struct mx3_camera_pdata	*pdata = pdev->dev.platform_data;
 	struct mx3_camera_dev *mx3_cam;
 	struct resource *res;
 	void __iomem *base;
@@ -1151,16 +1152,16 @@ static int mx3_camera_probe(struct platform_device *pdev)
 	struct soc_camera_host *soc_host;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		err = -ENODEV;
-		goto egetres;
-	}
+	if (!res)
+		return -ENODEV;
+
+	if (!pdata)
+		return -EINVAL;
 
 	mx3_cam = vzalloc(sizeof(*mx3_cam));
 	if (!mx3_cam) {
 		dev_err(&pdev->dev, "Could not allocate mx3 camera object\n");
-		err = -ENOMEM;
-		goto ealloc;
+		return -ENOMEM;
 	}
 
 	mx3_cam->clk = clk_get(&pdev->dev, NULL);
@@ -1169,8 +1170,8 @@ static int mx3_camera_probe(struct platform_device *pdev)
 		goto eclkget;
 	}
 
-	mx3_cam->pdata = pdev->dev.platform_data;
-	mx3_cam->platform_flags = mx3_cam->pdata->flags;
+	mx3_cam->pdata = pdata;
+	mx3_cam->platform_flags = pdata->flags;
 	if (!(mx3_cam->platform_flags & MX3_CAMERA_DATAWIDTH_MASK)) {
 		/*
 		 * Platform hasn't set available data widths. This is bad.
@@ -1189,7 +1190,7 @@ static int mx3_camera_probe(struct platform_device *pdev)
 	if (mx3_cam->platform_flags & MX3_CAMERA_DATAWIDTH_15)
 		mx3_cam->width_flags |= 1 << 14;
 
-	mx3_cam->mclk = mx3_cam->pdata->mclk_10khz * 10000;
+	mx3_cam->mclk = pdata->mclk_10khz * 10000;
 	if (!mx3_cam->mclk) {
 		dev_warn(&pdev->dev,
 			 "mclk_10khz == 0! Please, fix your platform data. "
@@ -1240,8 +1241,7 @@ eioremap:
 	clk_put(mx3_cam->clk);
 eclkget:
 	vfree(mx3_cam);
-ealloc:
-egetres:
+
 	return err;
 }
 
@@ -1276,6 +1276,7 @@ static int mx3_camera_remove(struct platform_device *pdev)
 static struct platform_driver mx3_camera_driver = {
 	.driver		= {
 		.name	= MX3_CAM_DRV_NAME,
+		.owner	= THIS_MODULE,
 	},
 	.probe		= mx3_camera_probe,
 	.remove		= mx3_camera_remove,
-- 
1.7.2.5

[PATCH 15/24] mx3-camera: support asynchronous subdevice registration

[Guennadi Liakhovetski]

To support asynchronous subdevice registration we only have to pass a
subdevice descriptor array from driver platform data to soc-camera for
camera host driver registration.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/platform/soc_camera/mx3_camera.c |    6 ++++++
 include/linux/platform_data/camera-mx3.h       |    3 +++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/media/platform/soc_camera/mx3_camera.c b/drivers/media/platform/soc_camera/mx3_camera.c
index 94203f6..75215bc 100644
--- a/drivers/media/platform/soc_camera/mx3_camera.c
+++ b/drivers/media/platform/soc_camera/mx3_camera.c
@@ -19,6 +19,7 @@
 #include <linux/sched.h>
 #include <linux/dma/ipu-dma.h>
 
+#include <media/v4l2-async.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-dev.h>
 #include <media/videobuf2-dma-contig.h>
@@ -1224,6 +1225,11 @@ static int mx3_camera_probe(struct platform_device *pdev)
 		goto eallocctx;
 	}
 
+	if (pdata->asd_sizes) {
+		soc_host->asd = pdata->asd;
+		soc_host->asd_sizes = pdata->asd_sizes;
+	}
+
 	err = soc_camera_host_register(soc_host);
 	if (err)
 		goto ecamhostreg;
diff --git a/include/linux/platform_data/camera-mx3.h b/include/linux/platform_data/camera-mx3.h
index f226ee3..96f0f78 100644
--- a/include/linux/platform_data/camera-mx3.h
+++ b/include/linux/platform_data/camera-mx3.h
@@ -33,6 +33,7 @@
 #define MX3_CAMERA_DATAWIDTH_MASK (MX3_CAMERA_DATAWIDTH_4 | MX3_CAMERA_DATAWIDTH_8 | \
 				   MX3_CAMERA_DATAWIDTH_10 | MX3_CAMERA_DATAWIDTH_15)
 
+struct v4l2_async_subdev;
 /**
  * struct mx3_camera_pdata - i.MX3x camera platform data
  * @flags:	MX3_CAMERA_* flags
@@ -43,6 +44,8 @@ struct mx3_camera_pdata {
 	unsigned long flags;
 	unsigned long mclk_10khz;
 	struct device *dma_dev;
+	struct v4l2_async_subdev **asd;	/* Flat array, arranged in groups */
+	int *asd_sizes;			/* 0-terminated array pf asd group sizes */
 };
 
 #endif
-- 
1.7.2.5

[PATCH 16/24] V4L2: mt9p031: add support for V4L2 clock and asynchronous probing

[Guennadi Liakhovetski]

This adds support for V4L2 clock and asynchronous subdevice probing.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/mt9p031.c |   31 +++++++++++++++++++++++++++++--
 1 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index e328332..9ba38f5 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -23,7 +23,9 @@
 #include <linux/videodev2.h>
 
 #include <media/mt9p031.h>
+#include <media/v4l2-async.h>
 #include <media/v4l2-chip-ident.h>
+#include <media/v4l2-clk.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-subdev.h>
@@ -117,6 +119,7 @@ struct mt9p031 {
 	struct media_pad pad;
 	struct v4l2_rect crop;  /* Sensor window */
 	struct v4l2_mbus_framefmt format;
+	struct v4l2_clk *clk;
 	struct mt9p031_platform_data *pdata;
 	struct mutex power_lock; /* lock to protect power_count */
 	int power_count;
@@ -258,6 +261,10 @@ static inline int mt9p031_pll_disable(struct mt9p031 *mt9p031)
 
 static int mt9p031_power_on(struct mt9p031 *mt9p031)
 {
+	int ret = v4l2_clk_enable(mt9p031->clk);
+	if (ret < 0)
+		return ret;
+
 	/* Ensure RESET_BAR is low */
 	if (mt9p031->reset != -1) {
 		gpio_set_value(mt9p031->reset, 0);
@@ -287,6 +294,8 @@ static void mt9p031_power_off(struct mt9p031 *mt9p031)
 
 	if (mt9p031->pdata->set_xclk)
 		mt9p031->pdata->set_xclk(&mt9p031->subdev, 0);
+
+	v4l2_clk_disable(mt9p031->clk);
 }
 
 static int __mt9p031_set_power(struct mt9p031 *mt9p031, bool on)
@@ -912,6 +921,7 @@ static int mt9p031_probe(struct i2c_client *client,
 {
 	struct mt9p031_platform_data *pdata = client->dev.platform_data;
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+	struct v4l2_clk *clk;
 	struct mt9p031 *mt9p031;
 	unsigned int i;
 	int ret;
@@ -927,11 +937,20 @@ static int mt9p031_probe(struct i2c_client *client,
 		return -EIO;
 	}
 
+	clk = v4l2_clk_get(&client->dev, "mclk");
+	if (IS_ERR(clk)) {
+		dev_info(&client->dev, "Error %ld getting clock\n", PTR_ERR(clk));
+		return -EPROBE_DEFER;
+	}
+
 	mt9p031 = kzalloc(sizeof(*mt9p031), GFP_KERNEL);
-	if (mt9p031 == NULL)
-		return -ENOMEM;
+	if (mt9p031 == NULL) {
+		ret = -ENOMEM;
+		goto done;
+	}
 
 	mt9p031->pdata = pdata;
+	mt9p031->clk = clk;
 	mt9p031->output_control	= MT9P031_OUTPUT_CONTROL_DEF;
 	mt9p031->mode2 = MT9P031_READ_MODE_2_ROW_BLC;
 	mt9p031->model = did->driver_data;
@@ -1010,6 +1029,11 @@ static int mt9p031_probe(struct i2c_client *client,
 	}
 
 	ret = mt9p031_pll_setup(mt9p031);
+	if (ret < 0)
+		goto done;
+
+	mt9p031->subdev.dev = &client->dev;
+	ret = v4l2_async_register_subdev(&mt9p031->subdev);
 
 done:
 	if (ret < 0) {
@@ -1019,6 +1043,7 @@ done:
 		v4l2_ctrl_handler_free(&mt9p031->ctrls);
 		media_entity_cleanup(&mt9p031->subdev.entity);
 		kfree(mt9p031);
+		v4l2_clk_put(clk);
 	}
 
 	return ret;
@@ -1029,11 +1054,13 @@ static int mt9p031_remove(struct i2c_client *client)
 	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
 	struct mt9p031 *mt9p031 = to_mt9p031(subdev);
 
+	v4l2_async_unregister_subdev(subdev);
 	v4l2_ctrl_handler_free(&mt9p031->ctrls);
 	v4l2_device_unregister_subdev(subdev);
 	media_entity_cleanup(&subdev->entity);
 	if (mt9p031->reset != -1)
 		gpio_free(mt9p031->reset);
+	v4l2_clk_put(mt9p031->clk);
 	kfree(mt9p031);
 
 	return 0;
-- 
1.7.2.5

[PATCH 17/24] V4L2: mt9p031: add support for .g_mbus_config() video operation

[Guennadi Liakhovetski]

.g_mbus_config() subdevice video operation is required for subdevice
drivers to be used with the soc-camera framework.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/mt9p031.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 9ba38f5..eb2de22 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -390,6 +390,18 @@ static int mt9p031_set_params(struct mt9p031 *mt9p031)
 	return ret;
 }
 
+static int mt9p031_g_mbus_config(struct v4l2_subdev *sd,
+				 struct v4l2_mbus_config *cfg)
+{
+	cfg->type = V4L2_MBUS_PARALLEL;
+	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_HSYNC_ACTIVE_HIGH |
+		V4L2_MBUS_VSYNC_ACTIVE_HIGH |
+		V4L2_MBUS_PCLK_SAMPLE_FALLING |
+		V4L2_MBUS_DATA_ACTIVE_HIGH;
+
+	return 0;
+}
+
 static int mt9p031_s_stream(struct v4l2_subdev *subdev, int enable)
 {
 	struct mt9p031 *mt9p031 = to_mt9p031(subdev);
@@ -888,6 +900,7 @@ static struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
 };
 
 static struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {
+	.g_mbus_config	= mt9p031_g_mbus_config,
 	.s_stream       = mt9p031_s_stream,
 };
 
-- 
1.7.2.5

[PATCH 18/24] V4L2: mt9p031: power down the sensor if no supported device has been detected

[Guennadi Liakhovetski]

The mt9p031 driver first accesses the I2C device in its .registered()
method. While doing that it furst powers the device up, but if probing
fails, it doesn't power the chip back down. This patch fixes that bug.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/mt9p031.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index eb2de22..70f4525 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -844,7 +844,7 @@ static int mt9p031_registered(struct v4l2_subdev *subdev)
 	ret = mt9p031_power_on(mt9p031);
 	if (ret < 0) {
 		dev_err(&client->dev, "MT9P031 power up failed\n");
-		return ret;
+		goto done;
 	}
 
 	/* Read out the chip version register */
@@ -852,13 +852,15 @@ static int mt9p031_registered(struct v4l2_subdev *subdev)
 	if (data != MT9P031_CHIP_VERSION_VALUE) {
 		dev_err(&client->dev, "MT9P031 not detected, wrong version "
 			"0x%04x\n", data);
-		return -ENODEV;
+		ret = -ENODEV;
 	}
 
+done:
 	mt9p031_power_off(mt9p031);
 
-	dev_info(&client->dev, "MT9P031 detected at address 0x%02x\n",
-		 client->addr);
+	if (!ret)
+		dev_info(&client->dev, "MT9P031 detected at address 0x%02x\n",
+			 client->addr);
 
 	return ret;
 }
-- 
1.7.2.5

[PATCH 19/24] V4L2: add struct v4l2_subdev_try_buf

[Guennadi Liakhovetski]

This patch adds struct v4l2_subdev_try_buf, used as a temporary buffer for
"try" pad configuration data. Defining such a struct will simplify memory
allocation for such buffers.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 include/media/v4l2-subdev.h |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index b15c6e0..4424a7c 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -618,14 +618,16 @@ static inline struct v4l2_subdev *v4l2_async_to_subdev(
 /*
  * Used for storing subdev information per file handle
  */
+struct v4l2_subdev_try_buf {
+	struct v4l2_mbus_framefmt try_fmt;
+	struct v4l2_rect try_crop;
+	struct v4l2_rect try_compose;
+};
+
 struct v4l2_subdev_fh {
 	struct v4l2_fh vfh;
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-	struct {
-		struct v4l2_mbus_framefmt try_fmt;
-		struct v4l2_rect try_crop;
-		struct v4l2_rect try_compose;
-	} *pad;
+	struct v4l2_subdev_try_buf *pad;
 #endif
 };
 
-- 
1.7.2.5

[PATCH 20/24] V4L2: add a subdev pointer to struct v4l2_subdev_fh

[Guennadi Liakhovetski]

This is useful for cases, when there's no video-device associated with a
V4L2 file-handle, e.g. in case of a dummy file-handle.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/v4l2-core/v4l2-subdev.c |    1 +
 include/media/v4l2-subdev.h           |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 996c248..ec9f0d8 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -34,6 +34,7 @@
 
 static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
 {
+	fh->subdev = sd;
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	fh->pad = kzalloc(sizeof(*fh->pad) * sd->entity.num_pads, GFP_KERNEL);
 	if (fh->pad == NULL)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 4424a7c..0581781 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -626,6 +626,7 @@ struct v4l2_subdev_try_buf {
 
 struct v4l2_subdev_fh {
 	struct v4l2_fh vfh;
+	struct v4l2_subdev *subdev;
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	struct v4l2_subdev_try_buf *pad;
 #endif
@@ -640,8 +641,7 @@ struct v4l2_subdev_fh {
 	v4l2_subdev_get_try_##fun_name(struct v4l2_subdev_fh *fh,	\
 				       unsigned int pad)		\
 	{								\
-		BUG_ON(pad >= vdev_to_v4l2_subdev(			\
-					fh->vfh.vdev)->entity.num_pads); \
+		BUG_ON(pad >= fh->subdev->entity.num_pads);		\
 		return &fh->pad[pad].field_name;			\
 	}
 
-- 
1.7.2.5

[PATCH 21/24] V4L2: add a subdevice-driver pad-operation wrapper

[Guennadi Liakhovetski]

Some subdevice drivers implement only the pad-level API, making them
unusable with V4L2 camera host drivers, using the plain subdevice
video API. This patch implements a wrapper to allow those two types
of drivers to be used together. So far only a subset of operations is
supported, the rest shall be added as needed.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/v4l2-core/Makefile        |    3 +
 drivers/media/v4l2-core/v4l2-pad-wrap.c |  329 +++++++++++++++++++++++++++++++
 include/media/v4l2-pad-wrap.h           |   23 +++
 include/media/v4l2-subdev.h             |    2 +
 4 files changed, 357 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-pad-wrap.c
 create mode 100644 include/media/v4l2-pad-wrap.h

diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 4c33b8d6..85dda29 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -13,6 +13,9 @@ endif
 ifeq ($(CONFIG_OF),y)
   videodev-objs += v4l2-of.o
 endif
+ifeq ($(CONFIG_VIDEO_V4L2_SUBDEV_API),y)
+  videodev-objs += v4l2-pad-wrap.o
+endif
 
 obj-$(CONFIG_VIDEO_V4L2) += videodev.o
 obj-$(CONFIG_VIDEO_V4L2_INT_DEVICE) += v4l2-int-device.o
diff --git a/drivers/media/v4l2-core/v4l2-pad-wrap.c b/drivers/media/v4l2-core/v4l2-pad-wrap.c
new file mode 100644
index 0000000..f45a5e7
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-pad-wrap.c
@@ -0,0 +1,329 @@
+/*
+ * V4L2 pad operation wrapper for pure subdevice bridge drivers
+ *
+ * Copyright (C) 2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * 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/module.h>
+#include <linux/slab.h>
+#include <media/v4l2-subdev.h>
+
+struct v4l2_subdev_pad_wrapper {
+	struct v4l2_subdev_ops		sd_ops;
+	struct v4l2_subdev_video_ops	video;
+	struct v4l2_subdev_fh		fh;
+	u32				pad;
+	const struct v4l2_subdev_ops	*ops_orig;
+	bool				touched;
+	struct v4l2_subdev_try_buf	try[];
+};
+
+/*
+ * It seems some subdev drivers, implementing the pad-level API, expect their
+ * pad devnode(s) to be opened at least once before any their operations can be
+ * called. This initialiser emulates such an open() / close() cycle.
+ */
+static int wrap_init_once(struct v4l2_subdev *sd,
+			  struct v4l2_subdev_pad_wrapper *wrap)
+{
+	int ret;
+
+	if (wrap->touched)
+		return 0;
+
+	ret = sd->internal_ops->open(sd, &wrap->fh);
+	if (ret < 0)
+		return ret;
+
+	wrap->touched = true;
+
+	return sd->internal_ops->close(sd, &wrap->fh);
+}
+
+static int wrap_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
+{
+	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
+				struct v4l2_subdev_pad_wrapper, sd_ops);
+	struct v4l2_subdev_selection sel = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+		.pad = wrap->pad,
+		.target = V4L2_SEL_TGT_CROP_BOUNDS,
+		.flags = V4L2_SEL_FLAG_KEEP_CONFIG,
+	};
+	struct v4l2_rect rect;
+	int ret = wrap_init_once(sd, wrap);
+	if (ret < 0)
+		return ret;
+
+	ret = sd->ops->pad->get_selection(sd, &wrap->fh, &sel);
+	if (ret < 0)
+		return ret;
+
+	rect = sel.r;
+
+	sel.target = V4L2_SEL_TGT_CROP_DEFAULT;
+
+	ret = sd->ops->pad->get_selection(sd, &wrap->fh, &sel);
+	if (ret < 0)
+		return ret;
+
+	a->bounds = rect;
+	a->defrect = sel.r;
+	a->pixelaspect.numerator = 1;
+	a->pixelaspect.denominator = 1;
+
+	return 0;
+}
+
+static int wrap_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
+{
+	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
+				struct v4l2_subdev_pad_wrapper, sd_ops);
+	struct v4l2_subdev_crop crop = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+		.pad = wrap->pad,
+	};
+	int ret = wrap_init_once(sd, wrap);
+	if (ret < 0)
+		return ret;
+
+	ret = sd->ops->pad->get_crop(sd, &wrap->fh, &crop);
+	if (ret < 0)
+		return ret;
+
+	a->c = crop.rect;
+
+	return 0;
+}
+
+static int wrap_s_crop(struct v4l2_subdev *sd, const struct v4l2_crop *a)
+{
+	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
+				struct v4l2_subdev_pad_wrapper, sd_ops);
+	struct v4l2_subdev_crop crop = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+		.pad = wrap->pad,
+		.rect = a->c,
+	};
+	int ret = wrap_init_once(sd, wrap);
+	if (ret < 0)
+		return ret;
+
+	return sd->ops->pad->set_crop(sd, &wrap->fh, &crop);
+}
+
+static int wrap_g_mbus_fmt(struct v4l2_subdev *sd,
+			   struct v4l2_mbus_framefmt *mf)
+{
+	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
+				struct v4l2_subdev_pad_wrapper, sd_ops);
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+		.pad = wrap->pad,
+	};
+	int ret = wrap_init_once(sd, wrap);
+	if (ret < 0)
+		return ret;
+
+	ret = sd->ops->pad->get_fmt(sd, &wrap->fh, &fmt);
+	if (ret < 0)
+		return ret;
+
+	*mf = fmt.format;
+
+	return 0;
+}
+
+static int wrap_try_mbus_fmt(struct v4l2_subdev *sd,
+			     struct v4l2_mbus_framefmt *mf)
+{
+	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
+				struct v4l2_subdev_pad_wrapper, sd_ops);
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_TRY,
+		.pad = wrap->pad,
+		.format = *mf,
+	};
+	/*
+	 * *TRY operations are using temporary buffers in the filehandle, so,
+	 * drivers can expect to be between an .open() and a .close() calls
+	 */
+	int ret_close, ret = sd->internal_ops->open(sd, &wrap->fh);
+	if (ret < 0)
+		return ret;
+
+	ret = sd->ops->pad->set_fmt(sd, &wrap->fh, &fmt);
+	ret_close = sd->internal_ops->close(sd, &wrap->fh);
+	if (ret < 0)
+		return ret;
+
+	if (!ret_close)
+		*mf = fmt.format;
+
+	return ret_close;
+}
+
+static int wrap_s_mbus_fmt(struct v4l2_subdev *sd,
+			   struct v4l2_mbus_framefmt *mf)
+{
+	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
+				struct v4l2_subdev_pad_wrapper, sd_ops);
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+		.pad = wrap->pad,
+		.format = *mf,
+	};
+	int ret = wrap_init_once(sd, wrap);
+	if (ret < 0)
+		return ret;
+
+	ret = sd->ops->pad->set_fmt(sd, &wrap->fh, &fmt);
+	if (ret < 0)
+		return ret;
+
+	*mf = fmt.format;
+
+	return 0;
+}
+
+static int wrap_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
+			      enum v4l2_mbus_pixelcode *code)
+{
+	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
+				struct v4l2_subdev_pad_wrapper, sd_ops);
+	struct v4l2_subdev_mbus_code_enum code_enum = {
+		.pad = wrap->pad,
+		.index = index,
+	};
+	int ret = wrap_init_once(sd, wrap);
+	if (ret < 0)
+		return ret;
+
+	ret = sd->ops->pad->enum_mbus_code(sd, &wrap->fh, &code_enum);
+	if (ret < 0)
+		return ret;
+
+	*code = code_enum.code;
+
+	return 0;
+}
+
+static int wrap_enum_framesizes(struct v4l2_subdev *sd,
+				struct v4l2_frmsizeenum *fsize)
+{
+	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
+				struct v4l2_subdev_pad_wrapper, sd_ops);
+	struct v4l2_subdev_frame_size_enum fse = {
+		.index = fsize->index,
+		.pad = wrap->pad,
+		.code = fsize->pixel_format,
+	};
+	int ret = wrap_init_once(sd, wrap);
+	if (ret < 0)
+		return ret;
+
+	ret = sd->ops->pad->enum_frame_size(sd, &wrap->fh, &fse);
+	if (ret < 0)
+		return ret;
+
+	if (fse.min_width == fse.max_width &&
+	    fse.min_height == fse.max_height) {
+		fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
+		fsize->discrete.width = fse.min_width;
+		fsize->discrete.height = fse.min_height;
+	} else {
+		fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
+		fsize->stepwise.min_width = fse.min_width;
+		fsize->stepwise.min_height = fse.min_height;
+		fsize->stepwise.max_width = fse.max_width;
+		fsize->stepwise.max_height = fse.max_height;
+		fsize->stepwise.step_width = 1;
+		fsize->stepwise.step_height = 1;
+	}
+
+	return 0;
+}
+
+int v4l2_subdev_pad_wrap(struct v4l2_subdev *sd)
+{
+	struct v4l2_subdev_pad_wrapper *wrap;
+	const struct v4l2_subdev_video_ops *video_orig = sd->ops->video;
+	const struct v4l2_subdev_pad_ops *pad_orig = sd->ops->pad;
+	int i;
+
+	if (!pad_orig)
+		return -ENOSYS;
+
+	wrap = kzalloc(sizeof(*wrap) + sd->entity.num_pads * sizeof(*wrap->fh.pad),
+		       GFP_KERNEL);
+	if (!wrap)
+		return -ENOMEM;
+
+	for (i = 0; i < sd->entity.num_pads; i++)
+		if (sd->entity.pads[i].flags == MEDIA_PAD_FL_SOURCE) {
+			wrap->pad = sd->entity.pads[i].index;
+			break;
+		}
+
+	if (i == sd->entity.num_pads) {
+		kfree(wrap);
+		return -ENODEV;
+	}
+
+	wrap->fh.subdev = sd;
+	wrap->fh.pad = wrap->try;
+	v4l2_fh_init(&wrap->fh.vfh, sd->devnode);
+
+	/* Do subdev drivers use fh? Do we need a v4l2_fh_add()? */
+
+	memcpy(&wrap->sd_ops, sd->ops, sizeof(wrap->sd_ops));
+	if (video_orig)
+		memcpy(&wrap->video, video_orig, sizeof(wrap->video));
+
+	/*
+	 * Assign wrappers individually. This way we won't overwrite by accident
+	 * new video operations before their wrappers are implemented here.
+	 */
+	if (pad_orig->get_selection && (!video_orig || !video_orig->cropcap))
+		wrap->video.cropcap = wrap_cropcap;
+	if (pad_orig->get_crop && (!video_orig || !video_orig->g_crop))
+		wrap->video.g_crop = wrap_g_crop;
+	if (pad_orig->set_crop && (!video_orig || !video_orig->s_crop))
+		wrap->video.s_crop = wrap_s_crop;
+	if (pad_orig->get_fmt && (!video_orig || !video_orig->g_mbus_fmt))
+		wrap->video.g_mbus_fmt = wrap_g_mbus_fmt;
+	if (pad_orig->set_fmt && (!video_orig || !video_orig->s_mbus_fmt)) {
+		wrap->video.s_mbus_fmt = wrap_s_mbus_fmt;
+		wrap->video.try_mbus_fmt = wrap_try_mbus_fmt;
+	}
+	if (pad_orig->enum_mbus_code && (!video_orig || !video_orig->enum_mbus_fmt))
+		wrap->video.enum_mbus_fmt = wrap_enum_mbus_fmt;
+	if (pad_orig->enum_frame_size && (!video_orig || !video_orig->enum_framesizes))
+		wrap->video.enum_framesizes = wrap_enum_framesizes;
+
+	wrap->ops_orig = sd->ops;
+	wrap->sd_ops.video = &wrap->video;
+	sd->ops = &wrap->sd_ops;
+
+	sd->flags |= V4L2_SUBDEV_FL_PADOPS_WRAP;
+
+	return 0;
+}
+EXPORT_SYMBOL(v4l2_subdev_pad_wrap);
+
+void v4l2_subdev_pad_unwrap(struct v4l2_subdev *sd)
+{
+	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
+				struct v4l2_subdev_pad_wrapper, sd_ops);
+
+	if (!(sd->flags & V4L2_SUBDEV_FL_PADOPS_WRAP))
+		return;
+
+	sd->ops = wrap->ops_orig;
+	kfree(wrap);
+}
+EXPORT_SYMBOL(v4l2_subdev_pad_unwrap);
diff --git a/include/media/v4l2-pad-wrap.h b/include/media/v4l2-pad-wrap.h
new file mode 100644
index 0000000..800e608
--- /dev/null
+++ b/include/media/v4l2-pad-wrap.h
@@ -0,0 +1,23 @@
+/*
+ * V4L2 pad operation wrapper for pure subdevice bridge drivers
+ *
+ * Copyright (C) 2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * 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.
+ */
+
+#ifndef V4L2_PAD_WRAP_H
+#define V4L2_PAD_WRAP_H
+
+struct v4l2_subdev;
+#if defined(CONFIG_MEDIA_CONTROLLER)
+int v4l2_subdev_pad_wrap(struct v4l2_subdev *sd);
+void v4l2_subdev_pad_unwrap(struct v4l2_subdev *sd);
+#else
+#define v4l2_subdev_pad_wrap(sd) ({ (void)(sd); -ENOSYS; })
+#define v4l2_subdev_pad_unwrap(sd) do { (void)(sd); } while (0)
+#endif
+
+#endif
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 0581781..7d53177 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -560,6 +560,8 @@ struct v4l2_subdev_internal_ops {
 #define V4L2_SUBDEV_FL_HAS_DEVNODE		(1U << 2)
 /* Set this flag if this subdev generates events. */
 #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
+/* Set this flag if this subdev emulates video operations */
+#define V4L2_SUBDEV_FL_PADOPS_WRAP		(1U << 4)
 
 struct regulator_bulk_data;
 
-- 
1.7.2.5

[PATCH 22/24] V4L2: soc-camera: use the pad-operation wrapper

[Guennadi Liakhovetski]

This patch adds support for the pad-operation wrapper to soc-camera, which
allows pure pad-level subdevice drivers, e.g. mt9p031 to be used with
soc-camera.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/platform/soc_camera/soc_camera.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 3113287..dfd1741 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -36,6 +36,7 @@
 #include <media/v4l2-common.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-dev.h>
+#include <media/v4l2-pad-wrap.h>
 #include <media/videobuf-core.h>
 #include <media/videobuf2-core.h>
 
@@ -62,7 +63,9 @@ struct soc_camera_async_client {
 	struct v4l2_async_subdev *sensor;
 	struct v4l2_async_notifier notifier;
 	struct platform_device *pdev;
-	struct list_head list;		/* needed for clean up */
+	/* needed for clean up */
+	struct list_head list;
+	struct v4l2_subdev *subdev;
 };
 
 static int soc_camera_video_start(struct soc_camera_device *icd);
@@ -1301,10 +1304,14 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
 	if (ret < 0)
 		return ret;
 
+	ret = v4l2_subdev_pad_wrap(sd);
+	if (ret < 0 && ret != -ENOSYS)
+		return ret;
+
 	ret = soc_camera_add_device(icd);
 	if (ret < 0) {
 		dev_err(icd->pdev, "Couldn't activate the camera: %d\n", ret);
-		return ret;
+		goto eadddev;
 	}
 
 	/* At this point client .probe() should have run already */
@@ -1329,6 +1336,8 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
 
 	return 0;
 
+eadddev:
+	v4l2_subdev_pad_unwrap(sd);
 evidstart:
 	soc_camera_free_user_formats(icd);
 eusrfmt:
@@ -1693,6 +1702,8 @@ static int soc_camera_remove(struct soc_camera_device *icd)
 {
 	struct soc_camera_desc *sdesc = to_soc_camera_desc(icd);
 	struct video_device *vdev = icd->vdev;
+	struct v4l2_subdev *sd = icd->sasc ? icd->sasc->subdev :
+		soc_camera_to_subdev(icd);
 
 	v4l2_ctrl_handler_free(&icd->ctrl_handler);
 	if (vdev) {
@@ -1700,6 +1711,9 @@ static int soc_camera_remove(struct soc_camera_device *icd)
 		icd->vdev = NULL;
 	}
 
+	/* Before cleaning up the sensor subdevice */
+	v4l2_subdev_pad_unwrap(sd);
+
 	if (sdesc->host_desc.board_info) {
 		soc_camera_i2c_free(icd);
 	} else {
@@ -1867,6 +1881,7 @@ void soc_camera_host_unregister(struct soc_camera_host *ici)
 			/* as long as we hold the device, sasc won't be freed */
 			get_device(icd->pdev);
 			list_add(&icd->sasc->list, &notifiers);
+			icd->sasc->subdev = soc_camera_to_subdev(icd);
 		}
 	mutex_unlock(&list_lock);
 
-- 
1.7.2.5

[PATCH 23/24] V4L2: mt9p031: add struct v4l2_subdev_platform_data to platform data

[Guennadi Liakhovetski]

Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows
the driver to use generic functions to manage sensor power supplies.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/i2c/mt9p031.c |    1 +
 include/media/mt9p031.h     |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 70f4525..ca2cc6e 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -1048,6 +1048,7 @@ static int mt9p031_probe(struct i2c_client *client,
 		goto done;
 
 	mt9p031->subdev.dev = &client->dev;
+	mt9p031->subdev.pdata = &pdata->sd_pdata;
 	ret = v4l2_async_register_subdev(&mt9p031->subdev);
 
 done:
diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h
index 0c97b19..7bf7b53 100644
--- a/include/media/mt9p031.h
+++ b/include/media/mt9p031.h
@@ -1,6 +1,8 @@
 #ifndef MT9P031_H
 #define MT9P031_H
 
+#include <media/v4l2-subdev.h>
+
 struct v4l2_subdev;
 
 /*
@@ -15,6 +17,7 @@ struct mt9p031_platform_data {
 	int reset;
 	int ext_freq;
 	int target_freq;
+	struct v4l2_subdev_platform_data sd_pdata;
 };
 
 #endif
-- 
1.7.2.5

[PATCH 24/24] ARM: pcm037: support mt9p031 / mt9p006 camera sensors

[Guennadi Liakhovetski]

We don't know how to support pluggable camera sensors yet. This is just
an example, how support for an mt9p031 or mt9p006 camera sensor could be
added to pcm037.

not-Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 arch/arm/mach-imx/mach-pcm037.c |   44 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/mach-pcm037.c b/arch/arm/mach-imx/mach-pcm037.c
index f138481..18ba328 100644
--- a/arch/arm/mach-imx/mach-pcm037.c
+++ b/arch/arm/mach-imx/mach-pcm037.c
@@ -36,6 +36,7 @@
 #include <linux/regulator/fixed.h>
 
 #include <media/soc_camera.h>
+#include <media/mt9p031.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -363,6 +364,22 @@ static struct i2c_board_info pcm037_i2c_devices[] = {
 	}
 };
 
+static struct mt9p031_platform_data mt9p031_pdata = {
+	.target_freq = 20000000,
+	.ext_freq = 20000000,
+	.sd_pdata	= {
+		.num_regulators = ARRAY_SIZE(cam_supply),
+		.regulators = cam_supply,
+	},
+};
+
+static struct i2c_board_info pcm037_i2c2_devices[] = {
+	{
+		I2C_BOARD_INFO("mt9p031", 0x48),
+		.platform_data = &mt9p031_pdata,
+	},
+};
+
 static struct platform_device pcm037_mt9t031 = {
 	.name	= "soc-camera-pdrv",
 	.id	= 0,
@@ -441,9 +458,30 @@ static const struct imxmmc_platform_data sdhc_pdata __initconst = {
 	.exit = pcm970_sdhc1_exit,
 };
 
+static struct soc_camera_async_subdev mt9p006_sd = {
+	.asd.hw = {
+		.bus_type = V4L2_ASYNC_BUS_I2C,
+		.match.i2c = {
+			.adapter_id = 2,
+			.address = 0x48,
+		},
+	},
+	.role = SOCAM_SUBDEV_DATA_SOURCE,
+};
+
+static struct v4l2_async_subdev *cam_subdevs[] = {
+	/* Single 1-element group */
+	&mt9p006_sd.asd,
+};
+
+/* 0-terminated array of group-sizes */
+static int cam_subdev_sizes[] = {ARRAY_SIZE(cam_subdevs), 0};
+
 struct mx3_camera_pdata camera_pdata __initdata = {
 	.flags		= MX3_CAMERA_DATAWIDTH_8 | MX3_CAMERA_DATAWIDTH_10,
 	.mclk_10khz	= 2000,
+	.asd		= cam_subdevs,
+	.asd_sizes	= cam_subdev_sizes,
 };
 
 static phys_addr_t mx3_camera_base __initdata;
@@ -476,8 +514,8 @@ static struct platform_device *devices[] __initdata = {
 	&pcm037_flash,
 	&pcm037_sram_device,
 	&vcc_cam,
-	&pcm037_mt9t031,
-	&pcm037_mt9v022,
+//	&pcm037_mt9t031,
+//	&pcm037_mt9v022,
 };
 
 static const struct fb_videomode fb_modedb[] = {
@@ -677,6 +715,8 @@ static void __init pcm037_init(void)
 	/* I2C adapters and devices */
 	i2c_register_board_info(1, pcm037_i2c_devices,
 			ARRAY_SIZE(pcm037_i2c_devices));
+	i2c_register_board_info(2, pcm037_i2c2_devices,
+			ARRAY_SIZE(pcm037_i2c2_devices));
 
 	imx31_add_imx_i2c1(&pcm037_i2c1_data);
 	imx31_add_imx_i2c2(&pcm037_i2c2_data);
-- 
1.7.2.5

Re: [PATCH 24/24] ARM: pcm037: support mt9p031 / mt9p006 camera sensors

[Guennadi Liakhovetski]

Forgot to mention explicitly: this patch is only an example, not even 
nearly to be considered for applying.

Thanks
Guennadi

On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote:

> We don't know how to support pluggable camera sensors yet. This is just
> an example, how support for an mt9p031 or mt9p006 camera sensor could be
> added to pcm037.
> 
> not-Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  arch/arm/mach-imx/mach-pcm037.c |   44 +++++++++++++++++++++++++++++++++++++-
>  1 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/mach-pcm037.c b/arch/arm/mach-imx/mach-pcm037.c
> index f138481..18ba328 100644
> --- a/arch/arm/mach-imx/mach-pcm037.c
> +++ b/arch/arm/mach-imx/mach-pcm037.c
> @@ -36,6 +36,7 @@
>  #include <linux/regulator/fixed.h>
>  
>  #include <media/soc_camera.h>
> +#include <media/mt9p031.h>
>  
>  #include <asm/mach-types.h>
>  #include <asm/mach/arch.h>
> @@ -363,6 +364,22 @@ static struct i2c_board_info pcm037_i2c_devices[] = {
>  	}
>  };
>  
> +static struct mt9p031_platform_data mt9p031_pdata = {
> +	.target_freq = 20000000,
> +	.ext_freq = 20000000,
> +	.sd_pdata	= {
> +		.num_regulators = ARRAY_SIZE(cam_supply),
> +		.regulators = cam_supply,
> +	},
> +};
> +
> +static struct i2c_board_info pcm037_i2c2_devices[] = {
> +	{
> +		I2C_BOARD_INFO("mt9p031", 0x48),
> +		.platform_data = &mt9p031_pdata,
> +	},
> +};
> +
>  static struct platform_device pcm037_mt9t031 = {
>  	.name	= "soc-camera-pdrv",
>  	.id	= 0,
> @@ -441,9 +458,30 @@ static const struct imxmmc_platform_data sdhc_pdata __initconst = {
>  	.exit = pcm970_sdhc1_exit,
>  };
>  
> +static struct soc_camera_async_subdev mt9p006_sd = {
> +	.asd.hw = {
> +		.bus_type = V4L2_ASYNC_BUS_I2C,
> +		.match.i2c = {
> +			.adapter_id = 2,
> +			.address = 0x48,
> +		},
> +	},
> +	.role = SOCAM_SUBDEV_DATA_SOURCE,
> +};
> +
> +static struct v4l2_async_subdev *cam_subdevs[] = {
> +	/* Single 1-element group */
> +	&mt9p006_sd.asd,
> +};
> +
> +/* 0-terminated array of group-sizes */
> +static int cam_subdev_sizes[] = {ARRAY_SIZE(cam_subdevs), 0};
> +
>  struct mx3_camera_pdata camera_pdata __initdata = {
>  	.flags		= MX3_CAMERA_DATAWIDTH_8 | MX3_CAMERA_DATAWIDTH_10,
>  	.mclk_10khz	= 2000,
> +	.asd		= cam_subdevs,
> +	.asd_sizes	= cam_subdev_sizes,
>  };
>  
>  static phys_addr_t mx3_camera_base __initdata;
> @@ -476,8 +514,8 @@ static struct platform_device *devices[] __initdata = {
>  	&pcm037_flash,
>  	&pcm037_sram_device,
>  	&vcc_cam,
> -	&pcm037_mt9t031,
> -	&pcm037_mt9v022,
> +//	&pcm037_mt9t031,
> +//	&pcm037_mt9v022,
>  };
>  
>  static const struct fb_videomode fb_modedb[] = {
> @@ -677,6 +715,8 @@ static void __init pcm037_init(void)
>  	/* I2C adapters and devices */
>  	i2c_register_board_info(1, pcm037_i2c_devices,
>  			ARRAY_SIZE(pcm037_i2c_devices));
> +	i2c_register_board_info(2, pcm037_i2c2_devices,
> +			ARRAY_SIZE(pcm037_i2c2_devices));
>  
>  	imx31_add_imx_i2c1(&pcm037_i2c1_data);
>  	imx31_add_imx_i2c2(&pcm037_i2c2_data);
> -- 
> 1.7.2.5

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 23/24] V4L2: mt9p031: add struct v4l2_subdev_platform_data to platform data

[Guennadi Liakhovetski]

On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote:

> Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows
> the driver to use generic functions to manage sensor power supplies.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

A small addition to this one too: to be absolutely honest, I also had to 
replace 12-bit formats with their 8-bit counterparts, because only 8 data 
lanes are connected to my camera host. We'll need to somehow properly 
solve this too.

Thanks
Guennadi

> ---
>  drivers/media/i2c/mt9p031.c |    1 +
>  include/media/mt9p031.h     |    3 +++
>  2 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 70f4525..ca2cc6e 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -1048,6 +1048,7 @@ static int mt9p031_probe(struct i2c_client *client,
>  		goto done;
>  
>  	mt9p031->subdev.dev = &client->dev;
> +	mt9p031->subdev.pdata = &pdata->sd_pdata;
>  	ret = v4l2_async_register_subdev(&mt9p031->subdev);
>  
>  done:
> diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h
> index 0c97b19..7bf7b53 100644
> --- a/include/media/mt9p031.h
> +++ b/include/media/mt9p031.h
> @@ -1,6 +1,8 @@
>  #ifndef MT9P031_H
>  #define MT9P031_H
>  
> +#include <media/v4l2-subdev.h>
> +
>  struct v4l2_subdev;
>  
>  /*
> @@ -15,6 +17,7 @@ struct mt9p031_platform_data {
>  	int reset;
>  	int ext_freq;
>  	int target_freq;
> +	struct v4l2_subdev_platform_data sd_pdata;
>  };
>  
>  #endif
> -- 
> 1.7.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 05/24] V4L2: allow dummy file-handle initialisation by v4l2_fh_init()

[Hans Verkuil]

On Thu April 18 2013 23:35:26 Guennadi Liakhovetski wrote:
> v4l2_fh_init() can be used to initialise dummy file-handles with vdev ==
> NULL.

Why would you want that?

Anyway, this would definitely have to be documented as well in v4l2-fh.h.

I'm still going through your patch series so there may be a good reason
for allowing this, but it definitely doesn't make me happy.

Regards,

	Hans

> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/v4l2-core/v4l2-fh.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
> index e57c002..7ae608b 100644
> --- a/drivers/media/v4l2-core/v4l2-fh.c
> +++ b/drivers/media/v4l2-core/v4l2-fh.c
> @@ -33,10 +33,12 @@
>  void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
>  {
>  	fh->vdev = vdev;
> -	/* Inherit from video_device. May be overridden by the driver. */
> -	fh->ctrl_handler = vdev->ctrl_handler;
> +	if (vdev) {
> +		/* Inherit from video_device. May be overridden by the driver. */
> +		fh->ctrl_handler = vdev->ctrl_handler;
> +		set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
> +	}
>  	INIT_LIST_HEAD(&fh->list);
> -	set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
>  	fh->prio = V4L2_PRIORITY_UNSET;
>  	init_waitqueue_head(&fh->wait);
>  	INIT_LIST_HEAD(&fh->available);
> 

Re: [PATCH 06/24] V4L2: add a common V4L2 subdevice platform data type

[Hans Verkuil]

On Thu April 18 2013 23:35:27 Guennadi Liakhovetski wrote:
> This struct shall be used by subdevice drivers to pass per-subdevice data,
> e.g. power supplies, to generic V4L2 methods, at the same time allowing
> optional host-specific extensions via the host_priv pointer. To avoid
> having to pass two pointers to those methods, add a pointer to this new
> struct to struct v4l2_subdev.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  include/media/v4l2-subdev.h |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index eb91366..b15c6e0 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -561,6 +561,17 @@ struct v4l2_subdev_internal_ops {
>  /* Set this flag if this subdev generates events. */
>  #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
>  
> +struct regulator_bulk_data;
> +
> +struct v4l2_subdev_platform_data {
> +	/* Optional regulators uset to power on/off the subdevice */
> +	struct regulator_bulk_data *regulators;
> +	int num_regulators;
> +
> +	/* Per-subdevice data, specific for a certain video host device */
> +	void *host_priv;
> +};
> +
>  /* Each instance of a subdev driver should create this struct, either
>     stand-alone or embedded in a larger struct.
>   */
> @@ -589,6 +600,8 @@ struct v4l2_subdev {
>  	/* pointer to the physical device */
>  	struct device *dev;
>  	struct v4l2_async_subdev_list asdl;
> +	/* common part of subdevice platform data */
> +	struct v4l2_subdev_platform_data *pdata;
>  };
>  
>  static inline struct v4l2_subdev *v4l2_async_to_subdev(
> 

Sorry, this is the wrong approach.

This is data that is of no use to the subdev driver itself. It really is
v4l2_subdev_host_platform_data, and as such must be maintained by the bridge
driver.

It can use v4l2_get/set_subdev_hostdata() to associate this struct with a
subdev, though.

Regards,

	Hans

Re: [PATCH 06/24] V4L2: add a common V4L2 subdevice platform data type

[Guennadi Liakhovetski]

Hi Hans

Thanks for reviewing.

On Fri, 19 Apr 2013, Hans Verkuil wrote:

> On Thu April 18 2013 23:35:27 Guennadi Liakhovetski wrote:
> > This struct shall be used by subdevice drivers to pass per-subdevice data,
> > e.g. power supplies, to generic V4L2 methods, at the same time allowing
> > optional host-specific extensions via the host_priv pointer. To avoid
> > having to pass two pointers to those methods, add a pointer to this new
> > struct to struct v4l2_subdev.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >  include/media/v4l2-subdev.h |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index eb91366..b15c6e0 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -561,6 +561,17 @@ struct v4l2_subdev_internal_ops {
> >  /* Set this flag if this subdev generates events. */
> >  #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
> >  
> > +struct regulator_bulk_data;
> > +
> > +struct v4l2_subdev_platform_data {
> > +	/* Optional regulators uset to power on/off the subdevice */
> > +	struct regulator_bulk_data *regulators;
> > +	int num_regulators;
> > +
> > +	/* Per-subdevice data, specific for a certain video host device */
> > +	void *host_priv;
> > +};
> > +
> >  /* Each instance of a subdev driver should create this struct, either
> >     stand-alone or embedded in a larger struct.
> >   */
> > @@ -589,6 +600,8 @@ struct v4l2_subdev {
> >  	/* pointer to the physical device */
> >  	struct device *dev;
> >  	struct v4l2_async_subdev_list asdl;
> > +	/* common part of subdevice platform data */
> > +	struct v4l2_subdev_platform_data *pdata;
> >  };
> >  
> >  static inline struct v4l2_subdev *v4l2_async_to_subdev(
> > 
> 
> Sorry, this is the wrong approach.
> 
> This is data that is of no use to the subdev driver itself. It really is
> v4l2_subdev_host_platform_data, and as such must be maintained by the bridge
> driver.

I don't think so. It has been discussed and agreed upon, that only 
subdevice drivers know when to switch power on and off, because only they 
know when they need to access the hardware. So, they have to manage 
regulators. In fact, those regulators supply power to respective 
subdevices, e.g. a camera sensor. Why should the bridge driver manage 
them? The V4L2 core can (and probably should) provide helper functions for 
that, like soc-camera currently does, but in any case it's the subdevice 
driver, that has to call them.

Thanks
Guennadi

> It can use v4l2_get/set_subdev_hostdata() to associate this struct with a
> subdev, though.
> 
> Regards,
> 
> 	Hans

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 21/24] V4L2: add a subdevice-driver pad-operation wrapper

[Hans Verkuil]

On Thu April 18 2013 23:35:42 Guennadi Liakhovetski wrote:
> Some subdevice drivers implement only the pad-level API, making them
> unusable with V4L2 camera host drivers, using the plain subdevice
> video API. This patch implements a wrapper to allow those two types
> of drivers to be used together. So far only a subset of operations is
> supported, the rest shall be added as needed.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>

I wish you'd discussed this with me before spending time on this. This is
really not the right approach to this problem.

The problem is that all the non-pad ops should be converted to the pad ops.
There is no point in having two different implementations of the same thing.

IMHO the best approach is to convert the users of the old ops to the pad ops
and add helper functions to v4l2-subdev.c that subdev drivers can use to
still support the old ops by calling the new pad op instead. The next step
is to convert all bridge driver uses of the old ops to the new ops and once
that's done we can drop the old ops. This second step can be done at leisure.

So in v4l2-subdev.c you would have a helper like this:

int v4l2_subdev_video_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
                             enum v4l2_mbus_pixelcode *code)
{
	struct v4l2_subdev_mbus_code_enum mbus_code;
	int err;

	memset(&mbus_code, 0, sizeof(mbus_code));
	mbus_code.index = index;
	err = v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &mbus_code);
	if (err >= 0)
		*code = mbus_code.code;
	return err;
}

This helper would then be used in a subdev driver as the video enum_mbus_fmt op.

I now also understand your earlier patch allowing dummy v4l2_fh structs. That's
definitely not the right approach, instead the v4l2_subdev_get_try_##fun_name
macro (which IMHO should be dropped: just write out the functions in full) should
be modified to handle a NULL v4l2_fh:

#define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name)           \
        static inline struct rtype *                                    \
        v4l2_subdev_get_try_##fun_name(struct v4l2_subdev_fh *fh,       \
                                       unsigned int pad)                \
        {                                                               \
		if (fh == NULL)						\
			return NULL;					\
                BUG_ON(unlikely(pad >= vdev_to_v4l2_subdev(             \
                                        fh->vfh.vdev)->entity.num_pads)); \
                return &fh->pad[pad].field_name;                        \
        }

Is there any point to the try variants if you don't have file handles? If
there is (and I don't see it), then v4l2_subdev could get a pointer to a
struct v4l2_subdev_try_buf and the macro could use that if fh == NULL.

Regards,

	Hans

> ---
>  drivers/media/v4l2-core/Makefile        |    3 +
>  drivers/media/v4l2-core/v4l2-pad-wrap.c |  329 +++++++++++++++++++++++++++++++
>  include/media/v4l2-pad-wrap.h           |   23 +++
>  include/media/v4l2-subdev.h             |    2 +
>  4 files changed, 357 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/v4l2-core/v4l2-pad-wrap.c
>  create mode 100644 include/media/v4l2-pad-wrap.h
> 
> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> index 4c33b8d6..85dda29 100644
> --- a/drivers/media/v4l2-core/Makefile
> +++ b/drivers/media/v4l2-core/Makefile
> @@ -13,6 +13,9 @@ endif
>  ifeq ($(CONFIG_OF),y)
>    videodev-objs += v4l2-of.o
>  endif
> +ifeq ($(CONFIG_VIDEO_V4L2_SUBDEV_API),y)
> +  videodev-objs += v4l2-pad-wrap.o
> +endif
>  
>  obj-$(CONFIG_VIDEO_V4L2) += videodev.o
>  obj-$(CONFIG_VIDEO_V4L2_INT_DEVICE) += v4l2-int-device.o
> diff --git a/drivers/media/v4l2-core/v4l2-pad-wrap.c b/drivers/media/v4l2-core/v4l2-pad-wrap.c
> new file mode 100644
> index 0000000..f45a5e7
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-pad-wrap.c
> @@ -0,0 +1,329 @@
> +/*
> + * V4L2 pad operation wrapper for pure subdevice bridge drivers
> + *
> + * Copyright (C) 2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * 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/module.h>
> +#include <linux/slab.h>
> +#include <media/v4l2-subdev.h>
> +
> +struct v4l2_subdev_pad_wrapper {
> +	struct v4l2_subdev_ops		sd_ops;
> +	struct v4l2_subdev_video_ops	video;
> +	struct v4l2_subdev_fh		fh;
> +	u32				pad;
> +	const struct v4l2_subdev_ops	*ops_orig;
> +	bool				touched;
> +	struct v4l2_subdev_try_buf	try[];
> +};
> +
> +/*
> + * It seems some subdev drivers, implementing the pad-level API, expect their
> + * pad devnode(s) to be opened at least once before any their operations can be
> + * called. This initialiser emulates such an open() / close() cycle.
> + */
> +static int wrap_init_once(struct v4l2_subdev *sd,
> +			  struct v4l2_subdev_pad_wrapper *wrap)
> +{
> +	int ret;
> +
> +	if (wrap->touched)
> +		return 0;
> +
> +	ret = sd->internal_ops->open(sd, &wrap->fh);
> +	if (ret < 0)
> +		return ret;
> +
> +	wrap->touched = true;
> +
> +	return sd->internal_ops->close(sd, &wrap->fh);
> +}
> +
> +static int wrap_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
> +{
> +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> +				struct v4l2_subdev_pad_wrapper, sd_ops);
> +	struct v4l2_subdev_selection sel = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +		.pad = wrap->pad,
> +		.target = V4L2_SEL_TGT_CROP_BOUNDS,
> +		.flags = V4L2_SEL_FLAG_KEEP_CONFIG,
> +	};
> +	struct v4l2_rect rect;
> +	int ret = wrap_init_once(sd, wrap);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sd->ops->pad->get_selection(sd, &wrap->fh, &sel);
> +	if (ret < 0)
> +		return ret;
> +
> +	rect = sel.r;
> +
> +	sel.target = V4L2_SEL_TGT_CROP_DEFAULT;
> +
> +	ret = sd->ops->pad->get_selection(sd, &wrap->fh, &sel);
> +	if (ret < 0)
> +		return ret;
> +
> +	a->bounds = rect;
> +	a->defrect = sel.r;
> +	a->pixelaspect.numerator = 1;
> +	a->pixelaspect.denominator = 1;
> +
> +	return 0;
> +}
> +
> +static int wrap_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> +{
> +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> +				struct v4l2_subdev_pad_wrapper, sd_ops);
> +	struct v4l2_subdev_crop crop = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +		.pad = wrap->pad,
> +	};
> +	int ret = wrap_init_once(sd, wrap);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sd->ops->pad->get_crop(sd, &wrap->fh, &crop);
> +	if (ret < 0)
> +		return ret;
> +
> +	a->c = crop.rect;
> +
> +	return 0;
> +}
> +
> +static int wrap_s_crop(struct v4l2_subdev *sd, const struct v4l2_crop *a)
> +{
> +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> +				struct v4l2_subdev_pad_wrapper, sd_ops);
> +	struct v4l2_subdev_crop crop = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +		.pad = wrap->pad,
> +		.rect = a->c,
> +	};
> +	int ret = wrap_init_once(sd, wrap);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sd->ops->pad->set_crop(sd, &wrap->fh, &crop);
> +}
> +
> +static int wrap_g_mbus_fmt(struct v4l2_subdev *sd,
> +			   struct v4l2_mbus_framefmt *mf)
> +{
> +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> +				struct v4l2_subdev_pad_wrapper, sd_ops);
> +	struct v4l2_subdev_format fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +		.pad = wrap->pad,
> +	};
> +	int ret = wrap_init_once(sd, wrap);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sd->ops->pad->get_fmt(sd, &wrap->fh, &fmt);
> +	if (ret < 0)
> +		return ret;
> +
> +	*mf = fmt.format;
> +
> +	return 0;
> +}
> +
> +static int wrap_try_mbus_fmt(struct v4l2_subdev *sd,
> +			     struct v4l2_mbus_framefmt *mf)
> +{
> +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> +				struct v4l2_subdev_pad_wrapper, sd_ops);
> +	struct v4l2_subdev_format fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_TRY,
> +		.pad = wrap->pad,
> +		.format = *mf,
> +	};
> +	/*
> +	 * *TRY operations are using temporary buffers in the filehandle, so,
> +	 * drivers can expect to be between an .open() and a .close() calls
> +	 */
> +	int ret_close, ret = sd->internal_ops->open(sd, &wrap->fh);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sd->ops->pad->set_fmt(sd, &wrap->fh, &fmt);
> +	ret_close = sd->internal_ops->close(sd, &wrap->fh);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!ret_close)
> +		*mf = fmt.format;
> +
> +	return ret_close;
> +}
> +
> +static int wrap_s_mbus_fmt(struct v4l2_subdev *sd,
> +			   struct v4l2_mbus_framefmt *mf)
> +{
> +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> +				struct v4l2_subdev_pad_wrapper, sd_ops);
> +	struct v4l2_subdev_format fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +		.pad = wrap->pad,
> +		.format = *mf,
> +	};
> +	int ret = wrap_init_once(sd, wrap);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sd->ops->pad->set_fmt(sd, &wrap->fh, &fmt);
> +	if (ret < 0)
> +		return ret;
> +
> +	*mf = fmt.format;
> +
> +	return 0;
> +}
> +
> +static int wrap_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
> +			      enum v4l2_mbus_pixelcode *code)
> +{
> +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> +				struct v4l2_subdev_pad_wrapper, sd_ops);
> +	struct v4l2_subdev_mbus_code_enum code_enum = {
> +		.pad = wrap->pad,
> +		.index = index,
> +	};
> +	int ret = wrap_init_once(sd, wrap);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sd->ops->pad->enum_mbus_code(sd, &wrap->fh, &code_enum);
> +	if (ret < 0)
> +		return ret;
> +
> +	*code = code_enum.code;
> +
> +	return 0;
> +}
> +
> +static int wrap_enum_framesizes(struct v4l2_subdev *sd,
> +				struct v4l2_frmsizeenum *fsize)
> +{
> +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> +				struct v4l2_subdev_pad_wrapper, sd_ops);
> +	struct v4l2_subdev_frame_size_enum fse = {
> +		.index = fsize->index,
> +		.pad = wrap->pad,
> +		.code = fsize->pixel_format,
> +	};
> +	int ret = wrap_init_once(sd, wrap);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sd->ops->pad->enum_frame_size(sd, &wrap->fh, &fse);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (fse.min_width == fse.max_width &&
> +	    fse.min_height == fse.max_height) {
> +		fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> +		fsize->discrete.width = fse.min_width;
> +		fsize->discrete.height = fse.min_height;
> +	} else {
> +		fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> +		fsize->stepwise.min_width = fse.min_width;
> +		fsize->stepwise.min_height = fse.min_height;
> +		fsize->stepwise.max_width = fse.max_width;
> +		fsize->stepwise.max_height = fse.max_height;
> +		fsize->stepwise.step_width = 1;
> +		fsize->stepwise.step_height = 1;
> +	}
> +
> +	return 0;
> +}
> +
> +int v4l2_subdev_pad_wrap(struct v4l2_subdev *sd)
> +{
> +	struct v4l2_subdev_pad_wrapper *wrap;
> +	const struct v4l2_subdev_video_ops *video_orig = sd->ops->video;
> +	const struct v4l2_subdev_pad_ops *pad_orig = sd->ops->pad;
> +	int i;
> +
> +	if (!pad_orig)
> +		return -ENOSYS;
> +
> +	wrap = kzalloc(sizeof(*wrap) + sd->entity.num_pads * sizeof(*wrap->fh.pad),
> +		       GFP_KERNEL);
> +	if (!wrap)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < sd->entity.num_pads; i++)
> +		if (sd->entity.pads[i].flags == MEDIA_PAD_FL_SOURCE) {
> +			wrap->pad = sd->entity.pads[i].index;
> +			break;
> +		}
> +
> +	if (i == sd->entity.num_pads) {
> +		kfree(wrap);
> +		return -ENODEV;
> +	}
> +
> +	wrap->fh.subdev = sd;
> +	wrap->fh.pad = wrap->try;
> +	v4l2_fh_init(&wrap->fh.vfh, sd->devnode);
> +
> +	/* Do subdev drivers use fh? Do we need a v4l2_fh_add()? */
> +
> +	memcpy(&wrap->sd_ops, sd->ops, sizeof(wrap->sd_ops));
> +	if (video_orig)
> +		memcpy(&wrap->video, video_orig, sizeof(wrap->video));
> +
> +	/*
> +	 * Assign wrappers individually. This way we won't overwrite by accident
> +	 * new video operations before their wrappers are implemented here.
> +	 */
> +	if (pad_orig->get_selection && (!video_orig || !video_orig->cropcap))
> +		wrap->video.cropcap = wrap_cropcap;
> +	if (pad_orig->get_crop && (!video_orig || !video_orig->g_crop))
> +		wrap->video.g_crop = wrap_g_crop;
> +	if (pad_orig->set_crop && (!video_orig || !video_orig->s_crop))
> +		wrap->video.s_crop = wrap_s_crop;
> +	if (pad_orig->get_fmt && (!video_orig || !video_orig->g_mbus_fmt))
> +		wrap->video.g_mbus_fmt = wrap_g_mbus_fmt;
> +	if (pad_orig->set_fmt && (!video_orig || !video_orig->s_mbus_fmt)) {
> +		wrap->video.s_mbus_fmt = wrap_s_mbus_fmt;
> +		wrap->video.try_mbus_fmt = wrap_try_mbus_fmt;
> +	}
> +	if (pad_orig->enum_mbus_code && (!video_orig || !video_orig->enum_mbus_fmt))
> +		wrap->video.enum_mbus_fmt = wrap_enum_mbus_fmt;
> +	if (pad_orig->enum_frame_size && (!video_orig || !video_orig->enum_framesizes))
> +		wrap->video.enum_framesizes = wrap_enum_framesizes;
> +
> +	wrap->ops_orig = sd->ops;
> +	wrap->sd_ops.video = &wrap->video;
> +	sd->ops = &wrap->sd_ops;
> +
> +	sd->flags |= V4L2_SUBDEV_FL_PADOPS_WRAP;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(v4l2_subdev_pad_wrap);
> +
> +void v4l2_subdev_pad_unwrap(struct v4l2_subdev *sd)
> +{
> +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> +				struct v4l2_subdev_pad_wrapper, sd_ops);
> +
> +	if (!(sd->flags & V4L2_SUBDEV_FL_PADOPS_WRAP))
> +		return;
> +
> +	sd->ops = wrap->ops_orig;
> +	kfree(wrap);
> +}
> +EXPORT_SYMBOL(v4l2_subdev_pad_unwrap);
> diff --git a/include/media/v4l2-pad-wrap.h b/include/media/v4l2-pad-wrap.h
> new file mode 100644
> index 0000000..800e608
> --- /dev/null
> +++ b/include/media/v4l2-pad-wrap.h
> @@ -0,0 +1,23 @@
> +/*
> + * V4L2 pad operation wrapper for pure subdevice bridge drivers
> + *
> + * Copyright (C) 2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * 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.
> + */
> +
> +#ifndef V4L2_PAD_WRAP_H
> +#define V4L2_PAD_WRAP_H
> +
> +struct v4l2_subdev;
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +int v4l2_subdev_pad_wrap(struct v4l2_subdev *sd);
> +void v4l2_subdev_pad_unwrap(struct v4l2_subdev *sd);
> +#else
> +#define v4l2_subdev_pad_wrap(sd) ({ (void)(sd); -ENOSYS; })
> +#define v4l2_subdev_pad_unwrap(sd) do { (void)(sd); } while (0)
> +#endif
> +
> +#endif
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 0581781..7d53177 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -560,6 +560,8 @@ struct v4l2_subdev_internal_ops {
>  #define V4L2_SUBDEV_FL_HAS_DEVNODE		(1U << 2)
>  /* Set this flag if this subdev generates events. */
>  #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
> +/* Set this flag if this subdev emulates video operations */
> +#define V4L2_SUBDEV_FL_PADOPS_WRAP		(1U << 4)
>  
>  struct regulator_bulk_data;
>  
> 

Re: [PATCH 06/24] V4L2: add a common V4L2 subdevice platform data type

[Hans Verkuil]

On Fri April 19 2013 09:48:27 Guennadi Liakhovetski wrote:
> Hi Hans
> 
> Thanks for reviewing.
> 
> On Fri, 19 Apr 2013, Hans Verkuil wrote:
> 
> > On Thu April 18 2013 23:35:27 Guennadi Liakhovetski wrote:
> > > This struct shall be used by subdevice drivers to pass per-subdevice data,
> > > e.g. power supplies, to generic V4L2 methods, at the same time allowing
> > > optional host-specific extensions via the host_priv pointer. To avoid
> > > having to pass two pointers to those methods, add a pointer to this new
> > > struct to struct v4l2_subdev.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> > >  include/media/v4l2-subdev.h |   13 +++++++++++++
> > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index eb91366..b15c6e0 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -561,6 +561,17 @@ struct v4l2_subdev_internal_ops {
> > >  /* Set this flag if this subdev generates events. */
> > >  #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
> > >  
> > > +struct regulator_bulk_data;
> > > +
> > > +struct v4l2_subdev_platform_data {
> > > +	/* Optional regulators uset to power on/off the subdevice */
> > > +	struct regulator_bulk_data *regulators;
> > > +	int num_regulators;
> > > +
> > > +	/* Per-subdevice data, specific for a certain video host device */
> > > +	void *host_priv;
> > > +};
> > > +
> > >  /* Each instance of a subdev driver should create this struct, either
> > >     stand-alone or embedded in a larger struct.
> > >   */
> > > @@ -589,6 +600,8 @@ struct v4l2_subdev {
> > >  	/* pointer to the physical device */
> > >  	struct device *dev;
> > >  	struct v4l2_async_subdev_list asdl;
> > > +	/* common part of subdevice platform data */
> > > +	struct v4l2_subdev_platform_data *pdata;
> > >  };
> > >  
> > >  static inline struct v4l2_subdev *v4l2_async_to_subdev(
> > > 
> > 
> > Sorry, this is the wrong approach.
> > 
> > This is data that is of no use to the subdev driver itself. It really is
> > v4l2_subdev_host_platform_data, and as such must be maintained by the bridge
> > driver.
> 
> I don't think so. It has been discussed and agreed upon, that only 
> subdevice drivers know when to switch power on and off, because only they 
> know when they need to access the hardware. So, they have to manage 
> regulators. In fact, those regulators supply power to respective 
> subdevices, e.g. a camera sensor. Why should the bridge driver manage 
> them? The V4L2 core can (and probably should) provide helper functions for 
> that, like soc-camera currently does, but in any case it's the subdevice 
> driver, that has to call them.

Ah, OK. I just realized I missed some context there. I didn't pay much
attention to the regulator discussions since that's not my area of expertise.

In that case my only comment is to drop the host_priv pointer since that just
duplicates v4l2_get/set_subdev_hostdata().

Regards,

	Hans

> 
> Thanks
> Guennadi
> 
> > It can use v4l2_get/set_subdev_hostdata() to associate this struct with a
> > subdev, though.
> > 
> > Regards,
> > 
> > 	Hans
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 21/24] V4L2: add a subdevice-driver pad-operation wrapper

[Guennadi Liakhovetski]

On Fri, 19 Apr 2013, Hans Verkuil wrote:

> On Thu April 18 2013 23:35:42 Guennadi Liakhovetski wrote:
> > Some subdevice drivers implement only the pad-level API, making them
> > unusable with V4L2 camera host drivers, using the plain subdevice
> > video API. This patch implements a wrapper to allow those two types
> > of drivers to be used together. So far only a subset of operations is
> > supported, the rest shall be added as needed.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> I wish you'd discussed this with me before spending time on this. This is
> really not the right approach to this problem.

I don't see this as such a strict requirement. I think, many systems work 
sufficiently well with subdevice video ops and don't need pad operations. 
Those systems simply don't have all the complexity, that really led to the 
pad-level API / MC. Think about simple camera interfaces, that can only 
capture data from a camera and DMA it to RAM with no or very little 
processing. No fancy scalers, converters, compressors etc. Do you really 
want to spend time converting such host drivers to the pad-level API and 
either use fake file-handles or really export everything to the user-space 
and make users configure both the camera source and the host sink pads 
manually independently... Isn't it an overkill? With my approach you just 
add "I want to stay with subdev ops and use a wrapper with pad-enabled 
sensor drivers" to the _host_ driver, because it's really the host driver, 
that has to be punished for being lazy. And you're done. No need to modify 
subdevice drivers first to add wrappers and then to remove them again...

Thanks
Guennadi

> The problem is that all the non-pad ops should be converted to the pad ops.
> There is no point in having two different implementations of the same thing.
> 
> IMHO the best approach is to convert the users of the old ops to the pad ops
> and add helper functions to v4l2-subdev.c that subdev drivers can use to
> still support the old ops by calling the new pad op instead. The next step
> is to convert all bridge driver uses of the old ops to the new ops and once
> that's done we can drop the old ops. This second step can be done at leisure.
> 
> So in v4l2-subdev.c you would have a helper like this:
> 
> int v4l2_subdev_video_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
>                              enum v4l2_mbus_pixelcode *code)
> {
> 	struct v4l2_subdev_mbus_code_enum mbus_code;
> 	int err;
> 
> 	memset(&mbus_code, 0, sizeof(mbus_code));
> 	mbus_code.index = index;
> 	err = v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &mbus_code);
> 	if (err >= 0)
> 		*code = mbus_code.code;
> 	return err;
> }
> 
> This helper would then be used in a subdev driver as the video enum_mbus_fmt op.
> 
> I now also understand your earlier patch allowing dummy v4l2_fh structs. That's
> definitely not the right approach, instead the v4l2_subdev_get_try_##fun_name
> macro (which IMHO should be dropped: just write out the functions in full) should
> be modified to handle a NULL v4l2_fh:
> 
> #define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name)           \
>         static inline struct rtype *                                    \
>         v4l2_subdev_get_try_##fun_name(struct v4l2_subdev_fh *fh,       \
>                                        unsigned int pad)                \
>         {                                                               \
> 		if (fh == NULL)						\
> 			return NULL;					\
>                 BUG_ON(unlikely(pad >= vdev_to_v4l2_subdev(             \
>                                         fh->vfh.vdev)->entity.num_pads)); \
>                 return &fh->pad[pad].field_name;                        \
>         }
> 
> Is there any point to the try variants if you don't have file handles? If
> there is (and I don't see it), then v4l2_subdev could get a pointer to a
> struct v4l2_subdev_try_buf and the macro could use that if fh == NULL.
> 
> Regards,
> 
> 	Hans
> 
> > ---
> >  drivers/media/v4l2-core/Makefile        |    3 +
> >  drivers/media/v4l2-core/v4l2-pad-wrap.c |  329 +++++++++++++++++++++++++++++++
> >  include/media/v4l2-pad-wrap.h           |   23 +++
> >  include/media/v4l2-subdev.h             |    2 +
> >  4 files changed, 357 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/media/v4l2-core/v4l2-pad-wrap.c
> >  create mode 100644 include/media/v4l2-pad-wrap.h
> > 
> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> > index 4c33b8d6..85dda29 100644
> > --- a/drivers/media/v4l2-core/Makefile
> > +++ b/drivers/media/v4l2-core/Makefile
> > @@ -13,6 +13,9 @@ endif
> >  ifeq ($(CONFIG_OF),y)
> >    videodev-objs += v4l2-of.o
> >  endif
> > +ifeq ($(CONFIG_VIDEO_V4L2_SUBDEV_API),y)
> > +  videodev-objs += v4l2-pad-wrap.o
> > +endif
> >  
> >  obj-$(CONFIG_VIDEO_V4L2) += videodev.o
> >  obj-$(CONFIG_VIDEO_V4L2_INT_DEVICE) += v4l2-int-device.o
> > diff --git a/drivers/media/v4l2-core/v4l2-pad-wrap.c b/drivers/media/v4l2-core/v4l2-pad-wrap.c
> > new file mode 100644
> > index 0000000..f45a5e7
> > --- /dev/null
> > +++ b/drivers/media/v4l2-core/v4l2-pad-wrap.c
> > @@ -0,0 +1,329 @@
> > +/*
> > + * V4L2 pad operation wrapper for pure subdevice bridge drivers
> > + *
> > + * Copyright (C) 2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > + *
> > + * 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/module.h>
> > +#include <linux/slab.h>
> > +#include <media/v4l2-subdev.h>
> > +
> > +struct v4l2_subdev_pad_wrapper {
> > +	struct v4l2_subdev_ops		sd_ops;
> > +	struct v4l2_subdev_video_ops	video;
> > +	struct v4l2_subdev_fh		fh;
> > +	u32				pad;
> > +	const struct v4l2_subdev_ops	*ops_orig;
> > +	bool				touched;
> > +	struct v4l2_subdev_try_buf	try[];
> > +};
> > +
> > +/*
> > + * It seems some subdev drivers, implementing the pad-level API, expect their
> > + * pad devnode(s) to be opened at least once before any their operations can be
> > + * called. This initialiser emulates such an open() / close() cycle.
> > + */
> > +static int wrap_init_once(struct v4l2_subdev *sd,
> > +			  struct v4l2_subdev_pad_wrapper *wrap)
> > +{
> > +	int ret;
> > +
> > +	if (wrap->touched)
> > +		return 0;
> > +
> > +	ret = sd->internal_ops->open(sd, &wrap->fh);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	wrap->touched = true;
> > +
> > +	return sd->internal_ops->close(sd, &wrap->fh);
> > +}
> > +
> > +static int wrap_cropcap(struct v4l2_subdev *sd, struct v4l2_cropcap *a)
> > +{
> > +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> > +				struct v4l2_subdev_pad_wrapper, sd_ops);
> > +	struct v4l2_subdev_selection sel = {
> > +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +		.pad = wrap->pad,
> > +		.target = V4L2_SEL_TGT_CROP_BOUNDS,
> > +		.flags = V4L2_SEL_FLAG_KEEP_CONFIG,
> > +	};
> > +	struct v4l2_rect rect;
> > +	int ret = wrap_init_once(sd, wrap);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = sd->ops->pad->get_selection(sd, &wrap->fh, &sel);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	rect = sel.r;
> > +
> > +	sel.target = V4L2_SEL_TGT_CROP_DEFAULT;
> > +
> > +	ret = sd->ops->pad->get_selection(sd, &wrap->fh, &sel);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	a->bounds = rect;
> > +	a->defrect = sel.r;
> > +	a->pixelaspect.numerator = 1;
> > +	a->pixelaspect.denominator = 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static int wrap_g_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
> > +{
> > +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> > +				struct v4l2_subdev_pad_wrapper, sd_ops);
> > +	struct v4l2_subdev_crop crop = {
> > +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +		.pad = wrap->pad,
> > +	};
> > +	int ret = wrap_init_once(sd, wrap);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = sd->ops->pad->get_crop(sd, &wrap->fh, &crop);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	a->c = crop.rect;
> > +
> > +	return 0;
> > +}
> > +
> > +static int wrap_s_crop(struct v4l2_subdev *sd, const struct v4l2_crop *a)
> > +{
> > +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> > +				struct v4l2_subdev_pad_wrapper, sd_ops);
> > +	struct v4l2_subdev_crop crop = {
> > +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +		.pad = wrap->pad,
> > +		.rect = a->c,
> > +	};
> > +	int ret = wrap_init_once(sd, wrap);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return sd->ops->pad->set_crop(sd, &wrap->fh, &crop);
> > +}
> > +
> > +static int wrap_g_mbus_fmt(struct v4l2_subdev *sd,
> > +			   struct v4l2_mbus_framefmt *mf)
> > +{
> > +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> > +				struct v4l2_subdev_pad_wrapper, sd_ops);
> > +	struct v4l2_subdev_format fmt = {
> > +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +		.pad = wrap->pad,
> > +	};
> > +	int ret = wrap_init_once(sd, wrap);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = sd->ops->pad->get_fmt(sd, &wrap->fh, &fmt);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*mf = fmt.format;
> > +
> > +	return 0;
> > +}
> > +
> > +static int wrap_try_mbus_fmt(struct v4l2_subdev *sd,
> > +			     struct v4l2_mbus_framefmt *mf)
> > +{
> > +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> > +				struct v4l2_subdev_pad_wrapper, sd_ops);
> > +	struct v4l2_subdev_format fmt = {
> > +		.which = V4L2_SUBDEV_FORMAT_TRY,
> > +		.pad = wrap->pad,
> > +		.format = *mf,
> > +	};
> > +	/*
> > +	 * *TRY operations are using temporary buffers in the filehandle, so,
> > +	 * drivers can expect to be between an .open() and a .close() calls
> > +	 */
> > +	int ret_close, ret = sd->internal_ops->open(sd, &wrap->fh);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = sd->ops->pad->set_fmt(sd, &wrap->fh, &fmt);
> > +	ret_close = sd->internal_ops->close(sd, &wrap->fh);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (!ret_close)
> > +		*mf = fmt.format;
> > +
> > +	return ret_close;
> > +}
> > +
> > +static int wrap_s_mbus_fmt(struct v4l2_subdev *sd,
> > +			   struct v4l2_mbus_framefmt *mf)
> > +{
> > +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> > +				struct v4l2_subdev_pad_wrapper, sd_ops);
> > +	struct v4l2_subdev_format fmt = {
> > +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +		.pad = wrap->pad,
> > +		.format = *mf,
> > +	};
> > +	int ret = wrap_init_once(sd, wrap);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = sd->ops->pad->set_fmt(sd, &wrap->fh, &fmt);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*mf = fmt.format;
> > +
> > +	return 0;
> > +}
> > +
> > +static int wrap_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
> > +			      enum v4l2_mbus_pixelcode *code)
> > +{
> > +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> > +				struct v4l2_subdev_pad_wrapper, sd_ops);
> > +	struct v4l2_subdev_mbus_code_enum code_enum = {
> > +		.pad = wrap->pad,
> > +		.index = index,
> > +	};
> > +	int ret = wrap_init_once(sd, wrap);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = sd->ops->pad->enum_mbus_code(sd, &wrap->fh, &code_enum);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*code = code_enum.code;
> > +
> > +	return 0;
> > +}
> > +
> > +static int wrap_enum_framesizes(struct v4l2_subdev *sd,
> > +				struct v4l2_frmsizeenum *fsize)
> > +{
> > +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> > +				struct v4l2_subdev_pad_wrapper, sd_ops);
> > +	struct v4l2_subdev_frame_size_enum fse = {
> > +		.index = fsize->index,
> > +		.pad = wrap->pad,
> > +		.code = fsize->pixel_format,
> > +	};
> > +	int ret = wrap_init_once(sd, wrap);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = sd->ops->pad->enum_frame_size(sd, &wrap->fh, &fse);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (fse.min_width == fse.max_width &&
> > +	    fse.min_height == fse.max_height) {
> > +		fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> > +		fsize->discrete.width = fse.min_width;
> > +		fsize->discrete.height = fse.min_height;
> > +	} else {
> > +		fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> > +		fsize->stepwise.min_width = fse.min_width;
> > +		fsize->stepwise.min_height = fse.min_height;
> > +		fsize->stepwise.max_width = fse.max_width;
> > +		fsize->stepwise.max_height = fse.max_height;
> > +		fsize->stepwise.step_width = 1;
> > +		fsize->stepwise.step_height = 1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int v4l2_subdev_pad_wrap(struct v4l2_subdev *sd)
> > +{
> > +	struct v4l2_subdev_pad_wrapper *wrap;
> > +	const struct v4l2_subdev_video_ops *video_orig = sd->ops->video;
> > +	const struct v4l2_subdev_pad_ops *pad_orig = sd->ops->pad;
> > +	int i;
> > +
> > +	if (!pad_orig)
> > +		return -ENOSYS;
> > +
> > +	wrap = kzalloc(sizeof(*wrap) + sd->entity.num_pads * sizeof(*wrap->fh.pad),
> > +		       GFP_KERNEL);
> > +	if (!wrap)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < sd->entity.num_pads; i++)
> > +		if (sd->entity.pads[i].flags == MEDIA_PAD_FL_SOURCE) {
> > +			wrap->pad = sd->entity.pads[i].index;
> > +			break;
> > +		}
> > +
> > +	if (i == sd->entity.num_pads) {
> > +		kfree(wrap);
> > +		return -ENODEV;
> > +	}
> > +
> > +	wrap->fh.subdev = sd;
> > +	wrap->fh.pad = wrap->try;
> > +	v4l2_fh_init(&wrap->fh.vfh, sd->devnode);
> > +
> > +	/* Do subdev drivers use fh? Do we need a v4l2_fh_add()? */
> > +
> > +	memcpy(&wrap->sd_ops, sd->ops, sizeof(wrap->sd_ops));
> > +	if (video_orig)
> > +		memcpy(&wrap->video, video_orig, sizeof(wrap->video));
> > +
> > +	/*
> > +	 * Assign wrappers individually. This way we won't overwrite by accident
> > +	 * new video operations before their wrappers are implemented here.
> > +	 */
> > +	if (pad_orig->get_selection && (!video_orig || !video_orig->cropcap))
> > +		wrap->video.cropcap = wrap_cropcap;
> > +	if (pad_orig->get_crop && (!video_orig || !video_orig->g_crop))
> > +		wrap->video.g_crop = wrap_g_crop;
> > +	if (pad_orig->set_crop && (!video_orig || !video_orig->s_crop))
> > +		wrap->video.s_crop = wrap_s_crop;
> > +	if (pad_orig->get_fmt && (!video_orig || !video_orig->g_mbus_fmt))
> > +		wrap->video.g_mbus_fmt = wrap_g_mbus_fmt;
> > +	if (pad_orig->set_fmt && (!video_orig || !video_orig->s_mbus_fmt)) {
> > +		wrap->video.s_mbus_fmt = wrap_s_mbus_fmt;
> > +		wrap->video.try_mbus_fmt = wrap_try_mbus_fmt;
> > +	}
> > +	if (pad_orig->enum_mbus_code && (!video_orig || !video_orig->enum_mbus_fmt))
> > +		wrap->video.enum_mbus_fmt = wrap_enum_mbus_fmt;
> > +	if (pad_orig->enum_frame_size && (!video_orig || !video_orig->enum_framesizes))
> > +		wrap->video.enum_framesizes = wrap_enum_framesizes;
> > +
> > +	wrap->ops_orig = sd->ops;
> > +	wrap->sd_ops.video = &wrap->video;
> > +	sd->ops = &wrap->sd_ops;
> > +
> > +	sd->flags |= V4L2_SUBDEV_FL_PADOPS_WRAP;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(v4l2_subdev_pad_wrap);
> > +
> > +void v4l2_subdev_pad_unwrap(struct v4l2_subdev *sd)
> > +{
> > +	struct v4l2_subdev_pad_wrapper *wrap = container_of(sd->ops,
> > +				struct v4l2_subdev_pad_wrapper, sd_ops);
> > +
> > +	if (!(sd->flags & V4L2_SUBDEV_FL_PADOPS_WRAP))
> > +		return;
> > +
> > +	sd->ops = wrap->ops_orig;
> > +	kfree(wrap);
> > +}
> > +EXPORT_SYMBOL(v4l2_subdev_pad_unwrap);
> > diff --git a/include/media/v4l2-pad-wrap.h b/include/media/v4l2-pad-wrap.h
> > new file mode 100644
> > index 0000000..800e608
> > --- /dev/null
> > +++ b/include/media/v4l2-pad-wrap.h
> > @@ -0,0 +1,23 @@
> > +/*
> > + * V4L2 pad operation wrapper for pure subdevice bridge drivers
> > + *
> > + * Copyright (C) 2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > + *
> > + * 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.
> > + */
> > +
> > +#ifndef V4L2_PAD_WRAP_H
> > +#define V4L2_PAD_WRAP_H
> > +
> > +struct v4l2_subdev;
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +int v4l2_subdev_pad_wrap(struct v4l2_subdev *sd);
> > +void v4l2_subdev_pad_unwrap(struct v4l2_subdev *sd);
> > +#else
> > +#define v4l2_subdev_pad_wrap(sd) ({ (void)(sd); -ENOSYS; })
> > +#define v4l2_subdev_pad_unwrap(sd) do { (void)(sd); } while (0)
> > +#endif
> > +
> > +#endif
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 0581781..7d53177 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -560,6 +560,8 @@ struct v4l2_subdev_internal_ops {
> >  #define V4L2_SUBDEV_FL_HAS_DEVNODE		(1U << 2)
> >  /* Set this flag if this subdev generates events. */
> >  #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
> > +/* Set this flag if this subdev emulates video operations */
> > +#define V4L2_SUBDEV_FL_PADOPS_WRAP		(1U << 4)
> >  
> >  struct regulator_bulk_data;
> >  
> > 
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 21/24] V4L2: add a subdevice-driver pad-operation wrapper

[Hans Verkuil]

On Fri April 19 2013 10:52:23 Guennadi Liakhovetski wrote:
> On Fri, 19 Apr 2013, Hans Verkuil wrote:
> 
> > On Thu April 18 2013 23:35:42 Guennadi Liakhovetski wrote:
> > > Some subdevice drivers implement only the pad-level API, making them
> > > unusable with V4L2 camera host drivers, using the plain subdevice
> > > video API. This patch implements a wrapper to allow those two types
> > > of drivers to be used together. So far only a subset of operations is
> > > supported, the rest shall be added as needed.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > 
> > Nacked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > I wish you'd discussed this with me before spending time on this. This is
> > really not the right approach to this problem.
> 
> I don't see this as such a strict requirement. I think, many systems work 
> sufficiently well with subdevice video ops and don't need pad operations. 
> Those systems simply don't have all the complexity, that really led to the 
> pad-level API / MC. Think about simple camera interfaces, that can only 
> capture data from a camera and DMA it to RAM with no or very little 
> processing. No fancy scalers, converters, compressors etc. Do you really 
> want to spend time converting such host drivers to the pad-level API and 
> either use fake file-handles or really export everything to the user-space 
> and make users configure both the camera source and the host sink pads 
> manually independently... Isn't it an overkill?

No it isn't overkill. Having two APIs doing the same thing is always a bad
idea. Just look at what you are doing now. If we had one API you wouldn't
need to do this work, would you? The current situation leads to some subdevs
that implement one API, some that implement another and some that implement
both. And the poor bridge drivers just have to figure out which subdev it is.

It's just inconsistent and it is really not that hard to fix. If I make myself
angry for a day or two I'd have the subdevs converted to support both APIs
through the helper functions and then we (well, probably me) can convert
bridge drivers one by one.

Frankly, I've tried to explain this situation to others, and it is just very
confusing for them: 'so tell me again which API should I use in my new subdev
driver?'. The only correct solution is to work towards dropping the old API,
then there is no confusion anymore and everything is consistent again.

> With my approach you just 
> add "I want to stay with subdev ops and use a wrapper with pad-enabled 
> sensor drivers" to the _host_ driver, because it's really the host driver, 
> that has to be punished for being lazy. And you're done. No need to modify 
> subdevice drivers first to add wrappers and then to remove them again...

And then you're stuck with that wrapper layer for all eternity. It's the
quick hack approach that I don't want to see in the v4l2 core. I'm working
hard to convert all drivers to use the new frameworks so that I can get
rid of legacy code in the core, and I don't want to introduce new hacks that
I need to clean up in the future.

There really aren't all that many drivers that use these ops, so it's not
(as far as I can see) a particularly huge or difficult job.

> > Is there any point to the try variants if you don't have file handles? If
> > there is (and I don't see it), then v4l2_subdev could get a pointer to a
> > struct v4l2_subdev_try_buf and the macro could use that if fh == NULL.

I just noticed that try_mbus_fmt is used in bridge drivers, so we do have to
support the try variants. But I think that it is sufficient if the try macro
just returns NULL if no fh is given. Because in that case there is no reason
to store the updated try value in the filehandle struct.

Regards,

	Hans

Re: [PATCH 00/24] V4L2: subdevice pad-level API wrapper

[Hans Verkuil]

On Thu April 18 2013 23:35:21 Guennadi Liakhovetski wrote:
> This is the first very crude shot at the subdevice pad-level API wrapper.
> The actual wrapper is added in patch #21, previous 20 patches are
> preparation... They apply on top of the last version of my async / clock
> patch series, respectively, on top of the announced branch of my linuxtv
> git-tree. Patches 2 and 4 from this series should actually be merged into
> respective patches from the async series.
> 
> I'm publishing this patch-series now, because I don't know when and how
> much time I'll have to improve it... Maybe you don't want to spend too much
> time reviewing implementation details, but comments to general concepts
> would be appreciated.
> 
> Further note, that patches 8-12 aren't really required. We can keep the
> deprecated struct soc_camera_link for now, or use a more gentle and slow
> way to remove it.

I just wanted to say that there is much here that I like very a lot. I
would suggest splitting the patch series in two: first the struct
v4l2_subdev_platform_data & soc_camera_link removal, then the patches
dealing with the pad-level API.

The first part looks very nice, really the only problem I have there is
with the host_priv field, which should be easy enough to fix.

Regards,

	Hans

Re: [PATCH 05/24] V4L2: allow dummy file-handle initialisation by v4l2_fh_init()

[Laurent Pinchart]

Hi Hans,

On Friday 19 April 2013 09:22:50 Hans Verkuil wrote:
> On Thu April 18 2013 23:35:26 Guennadi Liakhovetski wrote:
> > v4l2_fh_init() can be used to initialise dummy file-handles with vdev ==
> > NULL.
> 
> Why would you want that?

The reason is that subdev pad operations require a file handle and use it as a 
context to store the try rectangles. The wrappers thus need to create a dummy 
file handle.

> Anyway, this would definitely have to be documented as well in v4l2-fh.h.
> 
> I'm still going through your patch series so there may be a good reason
> for allowing this, but it definitely doesn't make me happy.
> 
> Regards,
> 
> 	Hans
> 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > 
> >  drivers/media/v4l2-core/v4l2-fh.c |    8 +++++---
> >  1 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-fh.c
> > b/drivers/media/v4l2-core/v4l2-fh.c index e57c002..7ae608b 100644
> > --- a/drivers/media/v4l2-core/v4l2-fh.c
> > +++ b/drivers/media/v4l2-core/v4l2-fh.c
> > @@ -33,10 +33,12 @@
> > 
> >  void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
> >  {
> >  
> >  	fh->vdev = vdev;
> > 
> > -	/* Inherit from video_device. May be overridden by the driver. */
> > -	fh->ctrl_handler = vdev->ctrl_handler;
> > +	if (vdev) {
> > +		/* Inherit from video_device. May be overridden by the driver. */
> > +		fh->ctrl_handler = vdev->ctrl_handler;
> > +		set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
> > +	}
> > 
> >  	INIT_LIST_HEAD(&fh->list);
> > 
> > -	set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
> > 
> >  	fh->prio = V4L2_PRIORITY_UNSET;
> >  	init_waitqueue_head(&fh->wait);
> >  	INIT_LIST_HEAD(&fh->available);
-- 
Regards,

Laurent Pinchart

Re: [PATCH 18/24] V4L2: mt9p031: power down the sensor if no supported device has been detected

[Laurent Pinchart]

Hi Guennadi,

Thanks for the patch.

On Thursday 18 April 2013 23:35:39 Guennadi Liakhovetski wrote:
> The mt9p031 driver first accesses the I2C device in its .registered()
> method. While doing that it furst powers the device up, but if probing

s/furst/first/

> fails, it doesn't power the chip back down. This patch fixes that bug.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>
> ---
>  drivers/media/i2c/mt9p031.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index eb2de22..70f4525 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -844,7 +844,7 @@ static int mt9p031_registered(struct v4l2_subdev
> *subdev) ret = mt9p031_power_on(mt9p031);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "MT9P031 power up failed\n");
> -		return ret;
> +		goto done;

Not here. If power on fails, there's no need to power off.

>  	}
> 
>  	/* Read out the chip version register */
> @@ -852,13 +852,15 @@ static int mt9p031_registered(struct v4l2_subdev
> *subdev) if (data != MT9P031_CHIP_VERSION_VALUE) {
>  		dev_err(&client->dev, "MT9P031 not detected, wrong version "
>  			"0x%04x\n", data);
> -		return -ENODEV;
> +		ret = -ENODEV;
>  	}
> 
> +done:
>  	mt9p031_power_off(mt9p031);
> 
> -	dev_info(&client->dev, "MT9P031 detected at address 0x%02x\n",
> -		 client->addr);
> +	if (!ret)
> +		dev_info(&client->dev, "MT9P031 detected at address 0x%02x\n",
> +			 client->addr);
> 
>  	return ret;
>  }

It would be easier to just move the power off line right after the 
mt9p031_read() call and leave the rest unchanged.

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index 28cf95b..8de84c0 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -849,18 +849,18 @@ static int mt9p031_registered(struct v4l2_subdev 
*subdev)
 
        /* Read out the chip version register */
        data = mt9p031_read(client, MT9P031_CHIP_VERSION);
+       mt9p031_power_off(mt9p031);
+
        if (data != MT9P031_CHIP_VERSION_VALUE) {
                dev_err(&client->dev, "MT9P031 not detected, wrong version "
                        "0x%04x\n", data);
                return -ENODEV;
        }
 
-       mt9p031_power_off(mt9p031);
-
        dev_info(&client->dev, "MT9P031 detected at address 0x%02x\n",
                 client->addr);
 
-       return ret;
+       return 0;
 }
 
 static int mt9p031_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh 
*fh)

If you're happy with that there's no need to resubmit, I'll apply the patch to 
my tree for v3.11.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 23/24] V4L2: mt9p031: add struct v4l2_subdev_platform_data to platform data

[Laurent Pinchart]

Hi Guennadi,

On Thursday 18 April 2013 23:47:26 Guennadi Liakhovetski wrote:
> On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote:
> > Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows
> > the driver to use generic functions to manage sensor power supplies.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> 
> A small addition to this one too: to be absolutely honest, I also had to
> replace 12-bit formats with their 8-bit counterparts, because only 8 data
> lanes are connected to my camera host. We'll need to somehow properly
> solve this too.

That information should be conveyed by platform/DT data for the host, and be 
used to convert the 12-bit media bus code into a 8-bit media bus code in the 
host (a core helper function would probably be helpful).

> > ---
> > 
> >  drivers/media/i2c/mt9p031.c |    1 +
> >  include/media/mt9p031.h     |    3 +++
> >  2 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> > index 70f4525..ca2cc6e 100644
> > --- a/drivers/media/i2c/mt9p031.c
> > +++ b/drivers/media/i2c/mt9p031.c
> > @@ -1048,6 +1048,7 @@ static int mt9p031_probe(struct i2c_client *client,
> >  		goto done;
> >  	
> >  	mt9p031->subdev.dev = &client->dev;
> > +	mt9p031->subdev.pdata = &pdata->sd_pdata;
> >  	ret = v4l2_async_register_subdev(&mt9p031->subdev);
> >  
> >  done:
> > diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h
> > index 0c97b19..7bf7b53 100644
> > --- a/include/media/mt9p031.h
> > +++ b/include/media/mt9p031.h
> > @@ -1,6 +1,8 @@
> >  #ifndef MT9P031_H
> >  #define MT9P031_H
> > 
> > +#include <media/v4l2-subdev.h>
> > +
> >  struct v4l2_subdev;
> >  /*
> > @@ -15,6 +17,7 @@ struct mt9p031_platform_data {
> >  	int reset;
> >  	int ext_freq;
> >  	int target_freq;
> > +	struct v4l2_subdev_platform_data sd_pdata;
> >  };
> >  
> >  #endif
-- 
Regards,

Laurent Pinchart

Re: [PATCH 18/24] V4L2: mt9p031: power down the sensor if no supported device has been detected

[Guennadi Liakhovetski]

Hi Laurent

On Mon, 22 Apr 2013, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thanks for the patch.
> 
> On Thursday 18 April 2013 23:35:39 Guennadi Liakhovetski wrote:
> > The mt9p031 driver first accesses the I2C device in its .registered()
> > method. While doing that it furst powers the device up, but if probing
> 
> s/furst/first/
> 
> > fails, it doesn't power the chip back down. This patch fixes that bug.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >
> > ---
> >  drivers/media/i2c/mt9p031.c |   10 ++++++----
> >  1 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> > index eb2de22..70f4525 100644
> > --- a/drivers/media/i2c/mt9p031.c
> > +++ b/drivers/media/i2c/mt9p031.c
> > @@ -844,7 +844,7 @@ static int mt9p031_registered(struct v4l2_subdev
> > *subdev) ret = mt9p031_power_on(mt9p031);
> >  	if (ret < 0) {
> >  		dev_err(&client->dev, "MT9P031 power up failed\n");
> > -		return ret;
> > +		goto done;
> 
> Not here. If power on fails, there's no need to power off.

Oops, right.

> >  	}
> > 
> >  	/* Read out the chip version register */
> > @@ -852,13 +852,15 @@ static int mt9p031_registered(struct v4l2_subdev
> > *subdev) if (data != MT9P031_CHIP_VERSION_VALUE) {
> >  		dev_err(&client->dev, "MT9P031 not detected, wrong version "
> >  			"0x%04x\n", data);
> > -		return -ENODEV;
> > +		ret = -ENODEV;
> >  	}
> > 
> > +done:
> >  	mt9p031_power_off(mt9p031);
> > 
> > -	dev_info(&client->dev, "MT9P031 detected at address 0x%02x\n",
> > -		 client->addr);
> > +	if (!ret)
> > +		dev_info(&client->dev, "MT9P031 detected at address 0x%02x\n",
> > +			 client->addr);
> > 
> >  	return ret;
> >  }
> 
> It would be easier to just move the power off line right after the 
> mt9p031_read() call and leave the rest unchanged.

Sure, please, do.

Thanks
Guennadi

> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 28cf95b..8de84c0 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -849,18 +849,18 @@ static int mt9p031_registered(struct v4l2_subdev 
> *subdev)
>  
>         /* Read out the chip version register */
>         data = mt9p031_read(client, MT9P031_CHIP_VERSION);
> +       mt9p031_power_off(mt9p031);
> +
>         if (data != MT9P031_CHIP_VERSION_VALUE) {
>                 dev_err(&client->dev, "MT9P031 not detected, wrong version "
>                         "0x%04x\n", data);
>                 return -ENODEV;
>         }
>  
> -       mt9p031_power_off(mt9p031);
> -
>         dev_info(&client->dev, "MT9P031 detected at address 0x%02x\n",
>                  client->addr);
>  
> -       return ret;
> +       return 0;
>  }
>  
>  static int mt9p031_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh 
> *fh)
> 
> If you're happy with that there's no need to resubmit, I'll apply the patch to 
> my tree for v3.11.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 23/24] V4L2: mt9p031: add struct v4l2_subdev_platform_data to platform data

[Guennadi Liakhovetski]

On Mon, 22 Apr 2013, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Thursday 18 April 2013 23:47:26 Guennadi Liakhovetski wrote:
> > On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote:
> > > Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows
> > > the driver to use generic functions to manage sensor power supplies.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > 
> > A small addition to this one too: to be absolutely honest, I also had to
> > replace 12-bit formats with their 8-bit counterparts, because only 8 data
> > lanes are connected to my camera host. We'll need to somehow properly
> > solve this too.
> 
> That information should be conveyed by platform/DT data for the host, and be 
> used to convert the 12-bit media bus code into a 8-bit media bus code in the 
> host (a core helper function would probably be helpful).

Yes, and we discussed this before too, I think. I proposed based then to 
implement some compatibility table of "trivial" transformations, like a 
12-bit Bayer, right-shifted by 4 bits, produces a respective 8-bit Bayer 
etc. Such transformations would fit nicely in soc_mediabus.c ;-) This just 
needs to be implemented...

Sure, I'd be happy to move soc_mediabus.c to 
drivers/media/v4l2-core/v4l2-mediabus.c.

Thanks
Guennadi

> > > ---
> > > 
> > >  drivers/media/i2c/mt9p031.c |    1 +
> > >  include/media/mt9p031.h     |    3 +++
> > >  2 files changed, 4 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> > > index 70f4525..ca2cc6e 100644
> > > --- a/drivers/media/i2c/mt9p031.c
> > > +++ b/drivers/media/i2c/mt9p031.c
> > > @@ -1048,6 +1048,7 @@ static int mt9p031_probe(struct i2c_client *client,
> > >  		goto done;
> > >  	
> > >  	mt9p031->subdev.dev = &client->dev;
> > > +	mt9p031->subdev.pdata = &pdata->sd_pdata;
> > >  	ret = v4l2_async_register_subdev(&mt9p031->subdev);
> > >  
> > >  done:
> > > diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h
> > > index 0c97b19..7bf7b53 100644
> > > --- a/include/media/mt9p031.h
> > > +++ b/include/media/mt9p031.h
> > > @@ -1,6 +1,8 @@
> > >  #ifndef MT9P031_H
> > >  #define MT9P031_H
> > > 
> > > +#include <media/v4l2-subdev.h>
> > > +
> > >  struct v4l2_subdev;
> > >  /*
> > > @@ -15,6 +17,7 @@ struct mt9p031_platform_data {
> > >  	int reset;
> > >  	int ext_freq;
> > >  	int target_freq;
> > > +	struct v4l2_subdev_platform_data sd_pdata;
> > >  };
> > >  
> > >  #endif
> -- 
> Regards,
> 
> Laurent Pinchart
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 23/24] V4L2: mt9p031: add struct v4l2_subdev_platform_data to platform data

[Laurent Pinchart]

Hi Guennadi,

On Thursday 18 April 2013 23:35:44 Guennadi Liakhovetski wrote:
> Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows
> the driver to use generic functions to manage sensor power supplies.

The mt9p031 driver now handles its regulators explicitly, please see

commit 97f212767a4d0fbddbf4786ccedacb47fc210548
Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date:   Tue May 8 10:10:36 2012 -0300

    [media] mt9p031: Add support for regulators
    
    Enable the regulators when powering the sensor up, and disable them when
    powering it down.
    The regulators are mandatory. Boards that don't allow controlling the
    sensor power lines must provide fixed voltage regulators.
    
    Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/i2c/mt9p031.c |    1 +
>  include/media/mt9p031.h     |    3 +++
>  2 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index 70f4525..ca2cc6e 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -1048,6 +1048,7 @@ static int mt9p031_probe(struct i2c_client *client,
>  		goto done;
> 
>  	mt9p031->subdev.dev = &client->dev;
> +	mt9p031->subdev.pdata = &pdata->sd_pdata;
>  	ret = v4l2_async_register_subdev(&mt9p031->subdev);
> 
>  done:
> diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h
> index 0c97b19..7bf7b53 100644
> --- a/include/media/mt9p031.h
> +++ b/include/media/mt9p031.h
> @@ -1,6 +1,8 @@
>  #ifndef MT9P031_H
>  #define MT9P031_H
> 
> +#include <media/v4l2-subdev.h>
> +
>  struct v4l2_subdev;
> 
>  /*
> @@ -15,6 +17,7 @@ struct mt9p031_platform_data {
>  	int reset;
>  	int ext_freq;
>  	int target_freq;
> +	struct v4l2_subdev_platform_data sd_pdata;
>  };
> 
>  #endif
-- 
Regards,

Laurent Pinchart

Re: [PATCH 23/24] V4L2: mt9p031: add struct v4l2_subdev_platform_data to platform data

[Laurent Pinchart]

On Monday 22 April 2013 14:39:57 Guennadi Liakhovetski wrote:
> On Mon, 22 Apr 2013, Laurent Pinchart wrote:
> > On Thursday 18 April 2013 23:47:26 Guennadi Liakhovetski wrote:
> > > On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote:
> > > > Adding struct v4l2_subdev_platform_data to mt9p031's platform data
> > > > allows the driver to use generic functions to manage sensor power
> > > > supplies.
> > > > 
> > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > 
> > > A small addition to this one too: to be absolutely honest, I also had to
> > > replace 12-bit formats with their 8-bit counterparts, because only 8
> > > data lanes are connected to my camera host. We'll need to somehow
> > > properly solve this too.
> > 
> > That information should be conveyed by platform/DT data for the host, and
> > be used to convert the 12-bit media bus code into a 8-bit media bus code
> > in the host (a core helper function would probably be helpful).
> 
> Yes, and we discussed this before too, I think. I proposed based then to
> implement some compatibility table of "trivial" transformations, like a
> 12-bit Bayer, right-shifted by 4 bits, produces a respective 8-bit Bayer
> etc. Such transformations would fit nicely in soc_mediabus.c ;-) This just
> needs to be implemented...
> 
> Sure, I'd be happy to move soc_mediabus.c to
> drivers/media/v4l2-core/v4l2-mediabus.c.

And the OMAP3 ISP driver has something similiar in 
drivers/media/platform/omap3isp/ispvideo.c

> > > > ---
> > > > 
> > > >  drivers/media/i2c/mt9p031.c |    1 +
> > > >  include/media/mt9p031.h     |    3 +++
> > > >  2 files changed, 4 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> > > > index 70f4525..ca2cc6e 100644
> > > > --- a/drivers/media/i2c/mt9p031.c
> > > > +++ b/drivers/media/i2c/mt9p031.c
> > > > @@ -1048,6 +1048,7 @@ static int mt9p031_probe(struct i2c_client
> > > > *client,
> > > > 
> > > >  		goto done;
> > > >  	
> > > >  	mt9p031->subdev.dev = &client->dev;
> > > > 
> > > > +	mt9p031->subdev.pdata = &pdata->sd_pdata;
> > > > 
> > > >  	ret = v4l2_async_register_subdev(&mt9p031->subdev);
> > > >  
> > > >  done:
> > > > diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h
> > > > index 0c97b19..7bf7b53 100644
> > > > --- a/include/media/mt9p031.h
> > > > +++ b/include/media/mt9p031.h
> > > > @@ -1,6 +1,8 @@
> > > > 
> > > >  #ifndef MT9P031_H
> > > >  #define MT9P031_H
> > > > 
> > > > +#include <media/v4l2-subdev.h>
> > > > +
> > > > 
> > > >  struct v4l2_subdev;
> > > >  /*
> > > > 
> > > > @@ -15,6 +17,7 @@ struct mt9p031_platform_data {
> > > > 
> > > >  	int reset;
> > > >  	int ext_freq;
> > > >  	int target_freq;
> > > > 
> > > > +	struct v4l2_subdev_platform_data sd_pdata;
> > > > 
> > > >  };
> > > >  
> > > >  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH 23/24] V4L2: mt9p031: add struct v4l2_subdev_platform_data to platform data

[Sascha Hauer]

Hi Guennadi,

On Mon, Apr 22, 2013 at 02:39:57PM +0200, Guennadi Liakhovetski wrote:
> On Mon, 22 Apr 2013, Laurent Pinchart wrote:
> 
> > Hi Guennadi,
> > 
> > On Thursday 18 April 2013 23:47:26 Guennadi Liakhovetski wrote:
> > > On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote:
> > > > Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows
> > > > the driver to use generic functions to manage sensor power supplies.
> > > > 
> > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > 
> > > A small addition to this one too: to be absolutely honest, I also had to
> > > replace 12-bit formats with their 8-bit counterparts, because only 8 data
> > > lanes are connected to my camera host. We'll need to somehow properly
> > > solve this too.
> > 
> > That information should be conveyed by platform/DT data for the host, and be 
> > used to convert the 12-bit media bus code into a 8-bit media bus code in the 
> > host (a core helper function would probably be helpful).
> 
> Yes, and we discussed this before too, I think. I proposed based then to 
> implement some compatibility table of "trivial" transformations, like a 
> 12-bit Bayer, right-shifted by 4 bits, produces a respective 8-bit Bayer 
> etc. Such transformations would fit nicely in soc_mediabus.c ;-) This just 
> needs to be implemented...

These "trivial" transformations may turn out not to be so trivial. In
the devicetree we would then need kind of 'shift-4-bit-left' properties.

How about instead describing the sensor node with:

	mbus-formats = <0x3010, 0x2013>;

and the corresponding host interface with:

	mbus-formats = <0x3013, 0x2001>;

This would allow to describe arbitrary transformations without having to
limit to the 'trivial' ones. The result would be easier to understand
also I think.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 23/24] V4L2: mt9p031: add struct v4l2_subdev_platform_data to platform data

[Guennadi Liakhovetski]

Hi Sascha

On Fri, 26 Apr 2013, Sascha Hauer wrote:

> Hi Guennadi,
> 
> On Mon, Apr 22, 2013 at 02:39:57PM +0200, Guennadi Liakhovetski wrote:
> > On Mon, 22 Apr 2013, Laurent Pinchart wrote:
> > 
> > > Hi Guennadi,
> > > 
> > > On Thursday 18 April 2013 23:47:26 Guennadi Liakhovetski wrote:
> > > > On Thu, 18 Apr 2013, Guennadi Liakhovetski wrote:
> > > > > Adding struct v4l2_subdev_platform_data to mt9p031's platform data allows
> > > > > the driver to use generic functions to manage sensor power supplies.
> > > > > 
> > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > > 
> > > > A small addition to this one too: to be absolutely honest, I also had to
> > > > replace 12-bit formats with their 8-bit counterparts, because only 8 data
> > > > lanes are connected to my camera host. We'll need to somehow properly
> > > > solve this too.
> > > 
> > > That information should be conveyed by platform/DT data for the host, and be 
> > > used to convert the 12-bit media bus code into a 8-bit media bus code in the 
> > > host (a core helper function would probably be helpful).
> > 
> > Yes, and we discussed this before too, I think. I proposed based then to 
> > implement some compatibility table of "trivial" transformations, like a 
> > 12-bit Bayer, right-shifted by 4 bits, produces a respective 8-bit Bayer 
> > etc. Such transformations would fit nicely in soc_mediabus.c ;-) This just 
> > needs to be implemented...
> 
> These "trivial" transformations may turn out not to be so trivial. In
> the devicetree we would then need kind of 'shift-4-bit-left' properties.

We already have a "data-shift" property exactly for this purpose.

> How about instead describing the sensor node with:
> 
> 	mbus-formats = <0x3010, 0x2013>;
> 
> and the corresponding host interface with:
> 
> 	mbus-formats = <0x3013, 0x2001>;

How would this describe _how_ the transformation should be performed? And 
why does the host driver need mbus formats? The translation is from mbus 
formats to fourcc formats (in memory). If you use those as bridge DT node 
properties, that would only tell the bridge driver which mbus format to 
request from the subdevice when requested fourcc format X. This decision 
soc-camera currently performs automatically, if this is ever needed as a 
configuration parameter, we'll think then where and how to put it.

Thanks
Guennadi

> This would allow to describe arbitrary transformations without having to
> limit to the 'trivial' ones. The result would be easier to understand
> also I think.

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 23/24] V4L2: mt9p031: add struct v4l2_subdev_platform_data to platform data

[Sascha Hauer]

On Fri, Apr 26, 2013 at 10:43:28AM +0200, Guennadi Liakhovetski wrote:
> Hi Sascha
> 
> On Fri, 26 Apr 2013, Sascha Hauer wrote:
> 
> > > > 
> > > > That information should be conveyed by platform/DT data for the host, and be 
> > > > used to convert the 12-bit media bus code into a 8-bit media bus code in the 
> > > > host (a core helper function would probably be helpful).
> > > 
> > > Yes, and we discussed this before too, I think. I proposed based then to 
> > > implement some compatibility table of "trivial" transformations, like a 
> > > 12-bit Bayer, right-shifted by 4 bits, produces a respective 8-bit Bayer 
> > > etc. Such transformations would fit nicely in soc_mediabus.c ;-) This just 
> > > needs to be implemented...
> > 
> > These "trivial" transformations may turn out not to be so trivial. In
> > the devicetree we would then need kind of 'shift-4-bit-left' properties.
> 
> We already have a "data-shift" property exactly for this purpose.
> 
> > How about instead describing the sensor node with:
> > 
> > 	mbus-formats = <0x3010, 0x2013>;
> > 
> > and the corresponding host interface with:
> > 
> > 	mbus-formats = <0x3013, 0x2001>;
> 
> How would this describe _how_ the transformation should be performed?

nth index in the sensor array matches nth index in the csi array. The
above describes:

V4L2_MBUS_FMT_SGBRG12_1X12 on the sensor matches V4L2_MBUS_FMT_SGBRG8_1X8 on the host
V4L2_MBUS_FMT_Y12_1X12 on the sensor matches V4L2_MBUS_FMT_Y8_1X8 on the host

effectively implementing a shift by four bits. But also more complicated
transformations could be described, like a colour space converter
implemented in a DSP (not sure if anyone does this, but you get the
idea)

> And why does the host driver need mbus formats?

Because mbus formats are effectively the input of a host driver. I
assumed that we translate the mbus formats the sensor can output into
the corresponding mbus formats that arrive at the host interface. Then
afterwards the usual translation from mbus to fourcc a host interface
can do is performed.
I think what you aim at instead is a translation directly from the
sensor to memory which I think is more complicated to build correctly.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 23/24] V4L2: mt9p031: add struct v4l2_subdev_platform_data to platform data

[Laurent Pinchart]

Hi Sascha,

On Friday 26 April 2013 11:15:56 Sascha Hauer wrote:
> On Fri, Apr 26, 2013 at 10:43:28AM +0200, Guennadi Liakhovetski wrote:
> > On Fri, 26 Apr 2013, Sascha Hauer wrote:
> > > > > That information should be conveyed by platform/DT data for the
> > > > > host, and be used to convert the 12-bit media bus code into a 8-bit
> > > > > media bus code in the host (a core helper function would probably
> > > > > be helpful).
> > > > 
> > > > Yes, and we discussed this before too, I think. I proposed based then
> > > > to implement some compatibility table of "trivial" transformations,
> > > > like a 12-bit Bayer, right-shifted by 4 bits, produces a respective 8-
> > > > bit Bayer etc. Such transformations would fit nicely in soc_mediabus.c
> > > > ;-) This just needs to be implemented...
> > > 
> > > These "trivial" transformations may turn out not to be so trivial. In
> > > the devicetree we would then need kind of 'shift-4-bit-left' properties.
> > 
> > We already have a "data-shift" property exactly for this purpose.
> > 
> > > How about instead describing the sensor node with:
> > > 	mbus-formats = <0x3010, 0x2013>;
> > > 
> > > and the corresponding host interface with:
> > > 	mbus-formats = <0x3013, 0x2001>;
> > 
> > How would this describe _how_ the transformation should be performed?
> 
> nth index in the sensor array matches nth index in the csi array. The
> above describes:
> 
> V4L2_MBUS_FMT_SGBRG12_1X12 on the sensor matches V4L2_MBUS_FMT_SGBRG8_1X8 on
> the host V4L2_MBUS_FMT_Y12_1X12 on the sensor matches V4L2_MBUS_FMT_Y8_1X8
> on the host
> 
> effectively implementing a shift by four bits. But also more complicated
> transformations could be described, like a colour space converter
> implemented in a DSP (not sure if anyone does this, but you get the
> idea)

If there's a component on the board between the sensor and the host it should 
be modeled as a proper subdev. I don't think trying to describe anything more 
complex than shifting the lanes in the device tree sensor and/or bridge data 
is a good idea.

> > And why does the host driver need mbus formats?
> 
> Because mbus formats are effectively the input of a host driver. I assumed
> that we translate the mbus formats the sensor can output into the
> corresponding mbus formats that arrive at the host interface. Then
> afterwards the usual translation from mbus to fourcc a host interface can do
> is performed. I think what you aim at instead is a translation directly from
> the sensor to memory which I think is more complicated to build correctly.

The sensor knows what mbus formats it supports at its output, and the host 
knows what mbus formats it supports at its inputs. Both information can be 
queried from user space and kernel space. With the data lanes shift property 
the host can then compute the mbus format it will receive at its input. I 
don't think we need to specify that in DT.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 06/24] V4L2: add a common V4L2 subdevice platform data type

[Guennadi Liakhovetski]

Hi Hans

Sorry for reviving this old thread. I was going to resubmit a part of 
those patches for mainlining and then I found this your comment, which I 
didn't reply to back then.

On Fri, 19 Apr 2013, Hans Verkuil wrote:

> On Fri April 19 2013 09:48:27 Guennadi Liakhovetski wrote:
> > Hi Hans
> > 
> > Thanks for reviewing.
> > 
> > On Fri, 19 Apr 2013, Hans Verkuil wrote:
> > 
> > > On Thu April 18 2013 23:35:27 Guennadi Liakhovetski wrote:
> > > > This struct shall be used by subdevice drivers to pass per-subdevice data,
> > > > e.g. power supplies, to generic V4L2 methods, at the same time allowing
> > > > optional host-specific extensions via the host_priv pointer. To avoid
> > > > having to pass two pointers to those methods, add a pointer to this new
> > > > struct to struct v4l2_subdev.
> > > > 
> > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > > ---
> > > >  include/media/v4l2-subdev.h |   13 +++++++++++++
> > > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index eb91366..b15c6e0 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -561,6 +561,17 @@ struct v4l2_subdev_internal_ops {
> > > >  /* Set this flag if this subdev generates events. */
> > > >  #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
> > > >  
> > > > +struct regulator_bulk_data;
> > > > +
> > > > +struct v4l2_subdev_platform_data {
> > > > +	/* Optional regulators uset to power on/off the subdevice */
> > > > +	struct regulator_bulk_data *regulators;
> > > > +	int num_regulators;
> > > > +
> > > > +	/* Per-subdevice data, specific for a certain video host device */
> > > > +	void *host_priv;
> > > > +};
> > > > +
> > > >  /* Each instance of a subdev driver should create this struct, either
> > > >     stand-alone or embedded in a larger struct.
> > > >   */
> > > > @@ -589,6 +600,8 @@ struct v4l2_subdev {
> > > >  	/* pointer to the physical device */
> > > >  	struct device *dev;
> > > >  	struct v4l2_async_subdev_list asdl;
> > > > +	/* common part of subdevice platform data */
> > > > +	struct v4l2_subdev_platform_data *pdata;
> > > >  };
> > > >  
> > > >  static inline struct v4l2_subdev *v4l2_async_to_subdev(
> > > > 
> > > 
> > > Sorry, this is the wrong approach.
> > > 
> > > This is data that is of no use to the subdev driver itself. It really is
> > > v4l2_subdev_host_platform_data, and as such must be maintained by the bridge
> > > driver.
> > 
> > I don't think so. It has been discussed and agreed upon, that only 
> > subdevice drivers know when to switch power on and off, because only they 
> > know when they need to access the hardware. So, they have to manage 
> > regulators. In fact, those regulators supply power to respective 
> > subdevices, e.g. a camera sensor. Why should the bridge driver manage 
> > them? The V4L2 core can (and probably should) provide helper functions for 
> > that, like soc-camera currently does, but in any case it's the subdevice 
> > driver, that has to call them.
> 
> Ah, OK. I just realized I missed some context there. I didn't pay much
> attention to the regulator discussions since that's not my area of expertise.
> 
> In that case my only comment is to drop the host_priv pointer since that just
> duplicates v4l2_get/set_subdev_hostdata().

I think it's different. This is _platform_ data, whereas struct 
v4l2_subdev::host_priv is more like run-time data. This field is for the 
per-subdevice host-specific data, that the platform has to pass to the 
host driver. In the soc-camera case this is the largest bulk of the data, 
that platforms currently pass to the soc-camera framework in the host part 
of struct soc_camera_link. This data most importantly includes I2C 
information. Yes, this _could_ be passed to soc-camera separately from the 
host driver, but that would involve quite some refactoring of the "legacy" 
synchronous probing mode, which I'd like to avoid if possible. This won't 
be used in the asynchronous case. Do you think we can keep this pointer in 
this sruct? We could rename it to avoid confusion with the field, that you 
told about.

Thanks
Guennadi

> > > It can use v4l2_get/set_subdev_hostdata() to associate this struct with a
> > > subdev, though.
> > > 
> > > Regards,
> > > 
> > > 	Hans

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 06/24] V4L2: add a common V4L2 subdevice platform data type

[Hans Verkuil]

Hi Guennadi,

Sorry for the delay, I only saw this today while I was going through my
mail backlog.

On 10/17/2013 08:24 PM, Guennadi Liakhovetski wrote:
> Hi Hans
> 
> Sorry for reviving this old thread. I was going to resubmit a part of 
> those patches for mainlining and then I found this your comment, which I 
> didn't reply to back then.
> 
> On Fri, 19 Apr 2013, Hans Verkuil wrote:
> 
>> On Fri April 19 2013 09:48:27 Guennadi Liakhovetski wrote:
>>> Hi Hans
>>>
>>> Thanks for reviewing.
>>>
>>> On Fri, 19 Apr 2013, Hans Verkuil wrote:
>>>
>>>> On Thu April 18 2013 23:35:27 Guennadi Liakhovetski wrote:
>>>>> This struct shall be used by subdevice drivers to pass per-subdevice data,
>>>>> e.g. power supplies, to generic V4L2 methods, at the same time allowing
>>>>> optional host-specific extensions via the host_priv pointer. To avoid
>>>>> having to pass two pointers to those methods, add a pointer to this new
>>>>> struct to struct v4l2_subdev.
>>>>>
>>>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>>>> ---
>>>>>  include/media/v4l2-subdev.h |   13 +++++++++++++
>>>>>  1 files changed, 13 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>>>> index eb91366..b15c6e0 100644
>>>>> --- a/include/media/v4l2-subdev.h
>>>>> +++ b/include/media/v4l2-subdev.h
>>>>> @@ -561,6 +561,17 @@ struct v4l2_subdev_internal_ops {
>>>>>  /* Set this flag if this subdev generates events. */
>>>>>  #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
>>>>>  
>>>>> +struct regulator_bulk_data;
>>>>> +
>>>>> +struct v4l2_subdev_platform_data {
>>>>> +	/* Optional regulators uset to power on/off the subdevice */
>>>>> +	struct regulator_bulk_data *regulators;
>>>>> +	int num_regulators;
>>>>> +
>>>>> +	/* Per-subdevice data, specific for a certain video host device */
>>>>> +	void *host_priv;
>>>>> +};
>>>>> +
>>>>>  /* Each instance of a subdev driver should create this struct, either
>>>>>     stand-alone or embedded in a larger struct.
>>>>>   */
>>>>> @@ -589,6 +600,8 @@ struct v4l2_subdev {
>>>>>  	/* pointer to the physical device */
>>>>>  	struct device *dev;
>>>>>  	struct v4l2_async_subdev_list asdl;
>>>>> +	/* common part of subdevice platform data */
>>>>> +	struct v4l2_subdev_platform_data *pdata;
>>>>>  };
>>>>>  
>>>>>  static inline struct v4l2_subdev *v4l2_async_to_subdev(
>>>>>
>>>>
>>>> Sorry, this is the wrong approach.
>>>>
>>>> This is data that is of no use to the subdev driver itself. It really is
>>>> v4l2_subdev_host_platform_data, and as such must be maintained by the bridge
>>>> driver.
>>>
>>> I don't think so. It has been discussed and agreed upon, that only 
>>> subdevice drivers know when to switch power on and off, because only they 
>>> know when they need to access the hardware. So, they have to manage 
>>> regulators. In fact, those regulators supply power to respective 
>>> subdevices, e.g. a camera sensor. Why should the bridge driver manage 
>>> them? The V4L2 core can (and probably should) provide helper functions for 
>>> that, like soc-camera currently does, but in any case it's the subdevice 
>>> driver, that has to call them.
>>
>> Ah, OK. I just realized I missed some context there. I didn't pay much
>> attention to the regulator discussions since that's not my area of expertise.
>>
>> In that case my only comment is to drop the host_priv pointer since that just
>> duplicates v4l2_get/set_subdev_hostdata().
> 
> I think it's different. This is _platform_ data, whereas struct 
> v4l2_subdev::host_priv is more like run-time data.

You mean subdev_hostdata() instead of host_priv, right?

> This field is for the 
> per-subdevice host-specific data, that the platform has to pass to the 
> host driver. In the soc-camera case this is the largest bulk of the data, 
> that platforms currently pass to the soc-camera framework in the host part 
> of struct soc_camera_link. This data most importantly includes I2C 
> information. Yes, this _could_ be passed to soc-camera separately from the 
> host driver, but that would involve quite some refactoring of the "legacy" 
> synchronous probing mode, which I'd like to avoid if possible. This won't 
> be used in the asynchronous case. Do you think we can keep this pointer in 
> this sruct? We could rename it to avoid confusion with the field, that you 
> told about.

I'm wondering: do we need host_priv at all? Can't drivers use container_of to
go from struct v4l2_subdev_platform_data to the platform_data struct containing
v4l2_subdev_platform_data?

That would be a cleaner solution IMHO. Using host_priv basically forces you
to split up the platform_data into two parts, and a void pointer isn't very
type-safe.

Regards,

	Hans

Re: [PATCH 06/24] V4L2: add a common V4L2 subdevice platform data type

[Laurent Pinchart]

Hi Hans,

On Monday 04 November 2013 12:24:10 Hans Verkuil wrote:
> Hi Guennadi,
> 
> Sorry for the delay, I only saw this today while I was going through my
> mail backlog.
> 
> On 10/17/2013 08:24 PM, Guennadi Liakhovetski wrote:
> > Hi Hans
> > 
> > Sorry for reviving this old thread. I was going to resubmit a part of
> > those patches for mainlining and then I found this your comment, which I
> > didn't reply to back then.
> > 
> > On Fri, 19 Apr 2013, Hans Verkuil wrote:
> >> On Fri April 19 2013 09:48:27 Guennadi Liakhovetski wrote:
> >>> Hi Hans
> >>> 
> >>> Thanks for reviewing.
> >>> 
> >>> On Fri, 19 Apr 2013, Hans Verkuil wrote:
> >>>> On Thu April 18 2013 23:35:27 Guennadi Liakhovetski wrote:
> >>>>> This struct shall be used by subdevice drivers to pass per-subdevice
> >>>>> data, e.g. power supplies, to generic V4L2 methods, at the same time
> >>>>> allowing optional host-specific extensions via the host_priv pointer.
> >>>>> To avoid having to pass two pointers to those methods, add a pointer
> >>>>> to this new struct to struct v4l2_subdev.
> >>>>> 
> >>>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >>>>> ---
> >>>>> 
> >>>>>  include/media/v4l2-subdev.h |   13 +++++++++++++
> >>>>>  1 files changed, 13 insertions(+), 0 deletions(-)
> >>>>> 
> >>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>>>> index eb91366..b15c6e0 100644
> >>>>> --- a/include/media/v4l2-subdev.h
> >>>>> +++ b/include/media/v4l2-subdev.h
> >>>>> @@ -561,6 +561,17 @@ struct v4l2_subdev_internal_ops {
> >>>>> 
> >>>>>  /* Set this flag if this subdev generates events. */
> >>>>>  #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
> >>>>> 
> >>>>> +struct regulator_bulk_data;
> >>>>> +
> >>>>> +struct v4l2_subdev_platform_data {
> >>>>> +	/* Optional regulators uset to power on/off the subdevice */
> >>>>> +	struct regulator_bulk_data *regulators;
> >>>>> +	int num_regulators;
> >>>>> +
> >>>>> +	/* Per-subdevice data, specific for a certain video host device */
> >>>>> +	void *host_priv;
> >>>>> +};
> >>>>> +
> >>>>> 
> >>>>>  /* Each instance of a subdev driver should create this struct, either
> >>>>>     stand-alone or embedded in a larger struct.
> >>>>>   */
> >>>>> @@ -589,6 +600,8 @@ struct v4l2_subdev {
> >>>>> 
> >>>>>  	/* pointer to the physical device */
> >>>>>  	struct device *dev;
> >>>>>  	struct v4l2_async_subdev_list asdl;
> >>>>> 
> >>>>> +	/* common part of subdevice platform data */
> >>>>> +	struct v4l2_subdev_platform_data *pdata;
> >>>>> 
> >>>>>  };
> >>>>>  
> >>>>>  static inline struct v4l2_subdev *v4l2_async_to_subdev(
> >>>> 
> >>>> Sorry, this is the wrong approach.
> >>>> 
> >>>> This is data that is of no use to the subdev driver itself. It really
> >>>> is v4l2_subdev_host_platform_data, and as such must be maintained by
> >>>> the bridge driver.
> >>> 
> >>> I don't think so. It has been discussed and agreed upon, that only
> >>> subdevice drivers know when to switch power on and off, because only
> >>> they know when they need to access the hardware. So, they have to manage
> >>> regulators. In fact, those regulators supply power to respective
> >>> subdevices, e.g. a camera sensor. Why should the bridge driver manage
> >>> them? The V4L2 core can (and probably should) provide helper functions
> >>> for that, like soc-camera currently does, but in any case it's the
> >>> subdevice driver, that has to call them.
> >> 
> >> Ah, OK. I just realized I missed some context there. I didn't pay much
> >> attention to the regulator discussions since that's not my area of
> >> expertise.
> >> 
> >> In that case my only comment is to drop the host_priv pointer since that
> >> just duplicates v4l2_get/set_subdev_hostdata().
> > 
> > I think it's different. This is _platform_ data, whereas struct
> > v4l2_subdev::host_priv is more like run-time data.
> 
> You mean subdev_hostdata() instead of host_priv, right?
> 
> > This field is for the
> > per-subdevice host-specific data, that the platform has to pass to the
> > host driver. In the soc-camera case this is the largest bulk of the data,
> > that platforms currently pass to the soc-camera framework in the host part
> > of struct soc_camera_link. This data most importantly includes I2C
> > information. Yes, this _could_ be passed to soc-camera separately from the
> > host driver, but that would involve quite some refactoring of the "legacy"
> > synchronous probing mode, which I'd like to avoid if possible. This won't
> > be used in the asynchronous case. Do you think we can keep this pointer in
> > this sruct? We could rename it to avoid confusion with the field, that you
> > told about.
> 
> I'm wondering: do we need host_priv at all? Can't drivers use container_of
> to go from struct v4l2_subdev_platform_data to the platform_data struct
> containing v4l2_subdev_platform_data?
> 
> That would be a cleaner solution IMHO. Using host_priv basically forces you
> to split up the platform_data into two parts, and a void pointer isn't very
> type-safe.

For what it's worth, that's the solution I was thinking about, so it has my 
preference as well.

-- 
Regards,

Laurent Pinchart