RFC: supporting adv7604.c under soc_camera/rcar_vin

RFC: supporting adv7604.c under soc_camera/rcar_vin

[William Towle]

  The following constitutes parts of our rcar_vin development branch
beyond the update to our hotfixes published earlier this month.
Similarly, these patches are intended to the mainline 3.18 kernel.
Further development is required, but we would like to highlight the
following issues and discuss them before completing the work.

1. Our internal review has noted that our use of v4l2_subdev_has_op()
is not yet ideal (but but does suffice for the purposes of generating
images as-is). These tests are intended to detect whether or not a
camera whose driver is aware of the pad API is present or not, and
ensure we interact with subdevices accordingly. We think we should be
iterating around all camera(s), and testing each subdevice link in
turn. Is this sound, or is there a better way?

2. Our second problem regards the supported formats list in adv7604.c,
which needs further attention. We believe that having entries that go
on to be rejected by rcar_vin_get_formats() may trigger a failure to
initialise cleanly. Workaround code is marked "Ian Hack"; we intend to
remove this and the list entries that cause this issue.

3. Our third problem concerns detecting the resolution of the stream.
Our code works with the obsoleted driver (adv761x.c) in place, but with
our modifications to adv7604.c we have seen a) recovery of a 640x480
image which is cropped rather than scaled, and/or b) recovery of a
2048x2048 image with the stream content in the top left corner. We
think we understand the former problem, but the latter seems to be
caused by full initialisation of the 'struct v4l2_subdev_format
sd_format' variable, and we only have a partial solution [included
as patch 4/8] so far. Of particular concern here is that potential
consequences of changes in this particular patch are not clear.


  Any advice would be appreciated, particularly regarding the first and
last point above.

Cheers,
  Wills.

  Associated patches:
	[PATCH 1/8] Add ability to read default input port from DT
	[PATCH 2/8] adv7604.c: formats, default colourspace, and IRQs
	[PATCH 3/8] WmT: document "adi,adv7612"
	[PATCH 4/8] WmT: m-5mols_core style pad handling for adv7604
	[PATCH 5/8] media: rcar_vin: Add RGB888_1X24 input format support
	[PATCH 6/8] WmT: adv7604 driver compatibility
	[PATCH 7/8] WmT: rcar_vin new ADV7612 support
	[PATCH 8/8] WmT: dts/i vin0/adv7612 (HDMI)

[PATCH 1/8] Add ability to read default input port from DT

[William Towle]

From: Ian Molton <ian.molton@codethink.co.uk>

---
 Documentation/devicetree/bindings/media/i2c/adv7604.txt |    3 +++
 drivers/media/i2c/adv7604.c                             |    8 +++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
index c27cede..bc50da2 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
@@ -33,6 +33,9 @@ Optional Properties:
 
   - reset-gpios: Reference to the GPIO connected to the device's reset pin.
 
+  - default-input: Reference to the chip's default input port. This value
+    should match the pad number for the intended device
+
 Optional Endpoint Properties:
 
   The following three properties are defined in video-interfaces.txt and are
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index e43dd2e..6666803 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -2686,6 +2686,7 @@ static int adv7604_parse_dt(struct adv7604_state *state)
 	struct device_node *endpoint;
 	struct device_node *np;
 	unsigned int flags;
+	u32 v;
 
 	np = state->i2c_clients[ADV7604_PAGE_IO]->dev.of_node;
 
@@ -2695,6 +2696,12 @@ static int adv7604_parse_dt(struct adv7604_state *state)
 		return -EINVAL;
 
 	v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+
+	if (!of_property_read_u32(endpoint, "default-input", &v))
+		state->pdata.default_input = v;
+	else
+		state->pdata.default_input = -1;
+
 	of_node_put(endpoint);
 
 	flags = bus_cfg.bus.parallel.flags;
@@ -2733,7 +2740,6 @@ static int adv7604_parse_dt(struct adv7604_state *state)
 	/* Hardcode the remaining platform data fields. */
 	state->pdata.disable_pwrdnb = 0;
 	state->pdata.disable_cable_det_rst = 0;
-	state->pdata.default_input = -1;
 	state->pdata.blank_data = 1;
 	state->pdata.alt_data_sat = 1;
 	state->pdata.op_format_mode_sel = ADV7604_OP_FORMAT_MODE0;
-- 
1.7.10.4

Re: [PATCH 1/8] Add ability to read default input port from DT

[Jean-Michel Hautbois]

2015-01-29 17:19 GMT+01:00 William Towle <william.towle@codethink.co.uk>:
> From: Ian Molton <ian.molton@codethink.co.uk>
>
> ---
>  Documentation/devicetree/bindings/media/i2c/adv7604.txt |    3 +++
>  drivers/media/i2c/adv7604.c                             |    8 +++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)

Is this really passing through checkpatch ? Without a proper signed-off-by ?

> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> index c27cede..bc50da2 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
> @@ -33,6 +33,9 @@ Optional Properties:
>
>    - reset-gpios: Reference to the GPIO connected to the device's reset pin.
>
> +  - default-input: Reference to the chip's default input port. This value
> +    should match the pad number for the intended device
> +
>  Optional Endpoint Properties:
>
>    The following three properties are defined in video-interfaces.txt and are
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index e43dd2e..6666803 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -2686,6 +2686,7 @@ static int adv7604_parse_dt(struct adv7604_state *state)
>         struct device_node *endpoint;
>         struct device_node *np;
>         unsigned int flags;
> +       u32 v;

Could be named default_input ?

>         np = state->i2c_clients[ADV7604_PAGE_IO]->dev.of_node;
>
> @@ -2695,6 +2696,12 @@ static int adv7604_parse_dt(struct adv7604_state *state)
>                 return -EINVAL;
>
>         v4l2_of_parse_endpoint(endpoint, &bus_cfg);
> +
> +       if (!of_property_read_u32(endpoint, "default-input", &v))
> +               state->pdata.default_input = v;
> +       else
> +               state->pdata.default_input = -1;
> +

OK, so, whatever the value I put in DT, it will be put in the pdata ?
No test against max_port ?

>         of_node_put(endpoint);
>
>         flags = bus_cfg.bus.parallel.flags;
> @@ -2733,7 +2740,6 @@ static int adv7604_parse_dt(struct adv7604_state *state)
>         /* Hardcode the remaining platform data fields. */
>         state->pdata.disable_pwrdnb = 0;
>         state->pdata.disable_cable_det_rst = 0;
> -       state->pdata.default_input = -1;
>         state->pdata.blank_data = 1;
>         state->pdata.alt_data_sat = 1;
>         state->pdata.op_format_mode_sel = ADV7604_OP_FORMAT_MODE0;
> --
> 1.7.10.4
>
> --
> 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

JM

[PATCH 2/8] adv7604.c: formats, default colourspace, and IRQs

[William Towle]

** for lx3.18.x+; from previous description **

The original adv7612-specific driver only contained formats meeting
a certain specification; allowing the adv7612 chip to permit all the
formats the adv7611 supports breaks this restriction and means that
enum_mbus_code can end up containing values that cause problems with
frame capture.

...Specifically, introducing a fully identical supported formats list
breaks video capture for video/x-raw-rgb in particular unless the
code path in rcar_vin.c marked "Ian HACK" is present. Reducing the
list leads to the possibility of gstreamer creating zero-length files.

This patch enables the reduced list by default, and does not enable
the corresponding fixup hack.

Support for 'video/x-raw-yuv' is unaffected in either case.

WmT: rcar_vin.c adv7604 driver compatibility

Add 'struct media_pad pad' member and suitable glue code, so that
soc_camera/rcar_vin can become agnostic to whether an old or new-
style driver (wrt pad API use) can sit underneath

This version has been reworked to include appropriate constant and
datatype names for kernel v3.18
---
 drivers/media/i2c/adv7604.c |  148 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 145 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 6666803..30bbd9d 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -43,7 +43,18 @@
 #include <media/v4l2-dv-timings.h>
 #include <media/v4l2-of.h>
 
-static int debug;
+#if 1	/*  WmT: TESTING  */
+#define HAVE_ADV7612_FORMATS
+	/*  Possible issue: adv7604_formats[] is extensive, whereas
+	    adv7612.c only had RGB888_1X24 (-> V4L2_COLORSPACE_SRGB)
+	    ...this doesn't cover MEDIA_BUS_FMT_YUYV8_2X8, which appears
+	    to be the new default format. Forcing 'code' "help"s
+	 */
+//define HAVE_ADV7611_EXTRA_FORMATS	/* identical list to 7611 */
+//#define HAVE_ADV7604_EXTRA_FORMATS	/* -> "unsupported" warnings */
+#endif
+
+static int debug = 0;
 module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "debug level (0-2)");
 
@@ -80,6 +91,7 @@ MODULE_LICENSE("GPL");
 enum adv7604_type {
 	ADV7604,
 	ADV7611,
+	ADV7612,
 };
 
 struct adv7604_reg_seq {
@@ -818,6 +830,73 @@ static const struct adv7604_format_info adv7611_formats[] = {
 	  ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT },
 };
 
+#ifdef HAVE_ADV7612_FORMATS
+static const struct adv7604_format_info adv7612_formats[] = {
+	{ MEDIA_BUS_FMT_RGB888_1X24, ADV7604_OP_CH_SEL_RGB, true, false,
+	  ADV7604_OP_MODE_SEL_SDR_444 | ADV7604_OP_FORMAT_SEL_8BIT },
+#ifdef HAVE_ADV7611_EXTRA_FORMATS	/* breaks without "Ian HACK"? */
+	{ MEDIA_BUS_FMT_YUYV8_2X8, ADV7604_OP_CH_SEL_RGB, false, false,
+	  ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_8BIT },
+#endif
+	{ MEDIA_BUS_FMT_YVYU8_2X8, ADV7604_OP_CH_SEL_RGB, false, true,
+	  ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_8BIT },
+#ifdef HAVE_ADV7604_EXTRA_FORMATS
+	/* 0x200b not in soc_mediabus.c mbus_fmt[] (07/01/2015) */
+	{ MEDIA_BUS_FMT_YUYV10_2X10, ADV7604_OP_CH_SEL_RGB, false, false,
+	  ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_10BIT },
+	/* 0x200c not in soc_mediabus.c mbus_fmt[] (07/01/2015) */
+	{ MEDIA_BUS_FMT_YVYU10_2X10, ADV7604_OP_CH_SEL_RGB, false, true,
+	  ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_10BIT },
+	/* 0x201e not in soc_mediabus.c mbus_fmt[] (07/01/2015) */
+	{ MEDIA_BUS_FMT_YUYV12_2X12, ADV7604_OP_CH_SEL_RGB, false, false,
+	  ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_12BIT },
+	/* 0x201f not in soc_mediabus.c mbus_fmt[] (07/01/2015) */
+	{ MEDIA_BUS_FMT_YVYU12_2X12, ADV7604_OP_CH_SEL_RGB, false, true,
+	  ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_12BIT },
+#endif
+	{ MEDIA_BUS_FMT_UYVY8_1X16, ADV7604_OP_CH_SEL_RBG, false, false,
+	  ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
+	{ MEDIA_BUS_FMT_VYUY8_1X16, ADV7604_OP_CH_SEL_RBG, false, true,
+	  ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
+#ifdef HAVE_ADV7611_EXTRA_FORMATS	/* inconsistent RGB <-> RGB without "Ian HACK" */
+	{ MEDIA_BUS_FMT_YUYV8_1X16, ADV7604_OP_CH_SEL_RGB, false, false,
+	  ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
+	{ MEDIA_BUS_FMT_YVYU8_1X16, ADV7604_OP_CH_SEL_RGB, false, true,
+	  ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
+#endif
+#ifdef HAVE_ADV7604_EXTRA_FORMATS
+	/* 0x201a not in soc_mediabus.c mbus_fmt[] (07/01/2015) */
+	{ MEDIA_BUS_FMT_UYVY10_1X20, ADV7604_OP_CH_SEL_RBG, false, false,
+	  ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_10BIT },
+	/* 0x201b not in soc_mediabus.c mbus_fmt[] (07/01/2015) */
+	{ MEDIA_BUS_FMT_VYUY10_1X20, ADV7604_OP_CH_SEL_RBG, false, true,
+	  ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_10BIT },
+	/* 0x200d not in soc_mediabus.c mbus_fmt[] (07/01/2015) */
+	{ MEDIA_BUS_FMT_YUYV10_1X20, ADV7604_OP_CH_SEL_RGB, false, false,
+	  ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_10BIT },
+	/* 0x200e not in soc_mediabus.c mbus_fmt[] (07/01/2015) */
+	{ MEDIA_BUS_FMT_YVYU10_1X20, ADV7604_OP_CH_SEL_RGB, false, true,
+	  ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_10BIT },
+	/* 0x2020 not in soc_mediabus.c mbus_fmt[] (07/01/2015) */
+	/* IS in adv7611_formats[] */
+	{ MEDIA_BUS_FMT_UYVY12_1X24, ADV7604_OP_CH_SEL_RBG, false, false,
+	  ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT },
+	/* 0x2021 not in soc_mediabus.c mbus_fmt[] (07/01/2015) */
+	{ MEDIA_BUS_FMT_VYUY12_1X24, ADV7604_OP_CH_SEL_RBG, false, true,
+	  ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT },
+	/* IS in adv7611_formats[] */
+	/* 0x2022 not in soc_mediabus.c mbus_fmt[] (07/01/2015) */
+	/* IS in adv7611_formats[] */
+	{ MEDIA_BUS_FMT_YUYV12_1X24, ADV7604_OP_CH_SEL_RGB, false, false,
+	  ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT },
+	/* 0x2023 not in soc_mediabus.c mbus_fmt[] (07/01/2015) */
+	/* IS in adv7611_formats[] */
+	{ MEDIA_BUS_FMT_YVYU12_1X24, ADV7604_OP_CH_SEL_RGB, false, true,
+	  ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT },
+#endif
+};
+#endif	/*  HAVE_ADV7612_FORMATS  */
+
 static const struct adv7604_format_info *
 adv7604_format_info(struct adv7604_state *state, u32 code)
 {
@@ -1918,6 +1997,10 @@ static int adv7604_set_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 	info = adv7604_format_info(state, format->format.code);
 	if (info == NULL)
 		info = adv7604_format_info(state, MEDIA_BUS_FMT_YUYV8_2X8);
+#ifndef HAVE_ADV7611_EXTRA_FORMATS	/* above is missing for 7612 */
+	if (info == NULL)
+		info = adv7604_format_info(state, MEDIA_BUS_FMT_RGB888_1X24);
+#endif
 
 	adv7604_fill_format(state, &format->format);
 	format->format.code = info->code;
@@ -2516,6 +2599,11 @@ static void adv7611_setup_irqs(struct v4l2_subdev *sd)
 	io_write(sd, 0x41, 0xd0); /* STDI irq for any change, disable INT2 */
 }
 
+static void adv7612_setup_irqs(struct v4l2_subdev *sd)
+{
+	io_write(sd, 0x41, 0xd0); /* disable INT2 */
+}
+
 static void adv7604_unregister_clients(struct adv7604_state *state)
 {
 	unsigned int i;
@@ -2603,6 +2691,19 @@ static const struct adv7604_reg_seq adv7611_recommended_settings_hdmi[] = {
 	{ ADV7604_REG_SEQ_TERM, 0 },
 };
 
+static const struct adv7604_reg_seq adv7612_recommended_settings_hdmi[] = {
+	{ ADV7604_REG(ADV7604_PAGE_CP, 0x6c), 0x00 },
+	{ ADV7604_REG(ADV7604_PAGE_HDMI, 0x9b), 0x03 },
+	{ ADV7604_REG(ADV7604_PAGE_HDMI, 0x6f), 0x08 },
+	{ ADV7604_REG(ADV7604_PAGE_HDMI, 0x85), 0x1f },
+	{ ADV7604_REG(ADV7604_PAGE_HDMI, 0x87), 0x70 },
+	{ ADV7604_REG(ADV7604_PAGE_HDMI, 0x57), 0xda },
+	{ ADV7604_REG(ADV7604_PAGE_HDMI, 0x58), 0x01 },
+	{ ADV7604_REG(ADV7604_PAGE_HDMI, 0x03), 0x98 },
+	{ ADV7604_REG(ADV7604_PAGE_HDMI, 0x4c), 0x44 },
+	{ ADV7604_REG_SEQ_TERM, 0 },
+};
+
 static const struct adv7604_chip_info adv7604_chip_info[] = {
 	[ADV7604] = {
 		.type = ADV7604,
@@ -2665,17 +2766,52 @@ static const struct adv7604_chip_info adv7604_chip_info[] = {
 			BIT(ADV7604_PAGE_REP) |  BIT(ADV7604_PAGE_EDID) |
 			BIT(ADV7604_PAGE_HDMI) | BIT(ADV7604_PAGE_CP),
 	},
+	[ADV7612] = {
+		.type = ADV7612,
+		.has_afe = false,
+		.max_port = ADV7604_PAD_HDMI_PORT_B,
+		.num_dv_ports = 2,
+		.edid_enable_reg = 0x74,
+		.edid_status_reg = 0x76,
+		.lcf_reg = 0xa3,
+		.tdms_lock_mask = 0x43,
+		.cable_det_mask = 0x01,
+		.fmt_change_digital_mask = 0x03,
+#ifdef HAVE_ADV7612_FORMATS
+		.formats = adv7612_formats,
+		.nformats = ARRAY_SIZE(adv7612_formats),
+#else
+		.formats = adv7604_formats,
+		.nformats = ARRAY_SIZE(adv7604_formats),
+#endif	/*  HAVE_ADV7612_FORMATS  */
+		.set_termination = adv7611_set_termination,
+		.setup_irqs = adv7612_setup_irqs,
+		.read_hdmi_pixelclock = adv7611_read_hdmi_pixelclock,
+		.read_cable_det = adv7611_read_cable_det,
+		.recommended_settings = {
+		    [1] = adv7612_recommended_settings_hdmi,
+		},
+		.num_recommended_settings = {
+		    [1] = ARRAY_SIZE(adv7612_recommended_settings_hdmi),
+		},
+		.page_mask = BIT(ADV7604_PAGE_IO) | BIT(ADV7604_PAGE_CEC) |
+			BIT(ADV7604_PAGE_INFOFRAME) | BIT(ADV7604_PAGE_AFE) |
+			BIT(ADV7604_PAGE_REP) |  BIT(ADV7604_PAGE_EDID) |
+			BIT(ADV7604_PAGE_HDMI) | BIT(ADV7604_PAGE_CP),
+	},
 };
 
 static struct i2c_device_id adv7604_i2c_id[] = {
 	{ "adv7604", (kernel_ulong_t)&adv7604_chip_info[ADV7604] },
 	{ "adv7611", (kernel_ulong_t)&adv7604_chip_info[ADV7611] },
+	{ "adv7612", (kernel_ulong_t)&adv7604_chip_info[ADV7612] },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, adv7604_i2c_id);
 
 static struct of_device_id adv7604_of_id[] __maybe_unused = {
 	{ .compatible = "adi,adv7611", .data = &adv7604_chip_info[ADV7611] },
+	{ .compatible = "adi,adv7612", .data = &adv7604_chip_info[ADV7612] },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adv7604_of_id);
@@ -2812,7 +2948,12 @@ static int adv7604_probe(struct i2c_client *client,
 	}
 
 	state->timings = cea640x480;
+	/* FIXME: interesting on adv7612 */
 	state->format = adv7604_format_info(state, MEDIA_BUS_FMT_YUYV8_2X8);
+#ifndef HAVE_ADV7611_EXTRA_FORMATS	/* above is missing for 7612 */
+	if (state->format == NULL)	// state->info->type != ADV7612
+		state->format = adv7604_format_info(state, MEDIA_BUS_FMT_RGB888_1X24);
+#endif
 
 	sd = &state->sd;
 	v4l2_i2c_subdev_init(sd, client, &adv7604_ops);
@@ -2836,8 +2977,9 @@ static int adv7604_probe(struct i2c_client *client,
 	} else {
 		val = (adv_smbus_read_byte_data_check(client, 0xea, false) << 8)
 		    | (adv_smbus_read_byte_data_check(client, 0xeb, false) << 0);
-		if (val != 0x2051) {
-			v4l2_info(sd, "not an adv7611 on address 0x%x\n",
+		if ((state->info->type == ADV7611 && val != 0x2051) ||
+			(state->info->type == ADV7612 && val != 0x2041)) {
+			v4l2_info(sd, "not an adv761x on address 0x%x\n",
 					client->addr << 1);
 			return -ENODEV;
 		}
-- 
1.7.10.4

[PATCH 3/8] WmT: document "adi,adv7612"

[William Towle]

---
 Documentation/devicetree/bindings/media/i2c/adv7604.txt |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7604.txt b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
index bc50da2..1ca6e5a 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7604.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7604.txt
@@ -1,4 +1,4 @@
-* Analog Devices ADV7604/11 video decoder with HDMI receiver
+* Analog Devices ADV7604/11/12 video decoder with HDMI receiver
 
 The ADV7604 and ADV7611 are multiformat video decoders with an integrated HDMI
 receiver. The ADV7604 has four multiplexed HDMI inputs and one analog input,
@@ -10,6 +10,7 @@ Required Properties:
 
   - compatible: Must contain one of the following
     - "adi,adv7611" for the ADV7611
+    - "adi,adv7612" for the ADV7612
 
   - reg: I2C slave address
 
-- 
1.7.10.4

[PATCH 4/8] WmT: m-5mols_core style pad handling for adv7604

[William Towle]

---
 drivers/media/i2c/adv7604.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 30bbd9d..6ed9303 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -1976,7 +1976,11 @@ static int adv7604_get_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
 		struct v4l2_mbus_framefmt *fmt;
 
-		fmt = v4l2_subdev_get_try_format(fh, format->pad);
+		fmt = (fh == NULL) ? NULL
+			: v4l2_subdev_get_try_format(fh, format->pad);
+		if (fmt == NULL)
+			return EINVAL;
+
 		format->format.code = fmt->code;
 	} else {
 		format->format.code = state->format->code;
@@ -2008,7 +2012,11 @@ static int adv7604_set_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
 		struct v4l2_mbus_framefmt *fmt;
 
-		fmt = v4l2_subdev_get_try_format(fh, format->pad);
+		fmt = (fh == NULL) ? NULL
+			: v4l2_subdev_get_try_format(fh, format->pad);
+		if (fmt == NULL)
+			return -EINVAL;
+
 		fmt->code = format->format.code;
 	} else {
 		state->format = info;
-- 
1.7.10.4

Re: [PATCH 4/8] WmT: m-5mols_core style pad handling for adv7604

[Jean-Michel Hautbois]

First of all, this subject puzzles me... What means WmT ??

2015-01-29 17:19 GMT+01:00 William Towle <william.towle@codethink.co.uk>:
> ---
>  drivers/media/i2c/adv7604.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

Again, it it passing checkpatch without signed-off-by ? And a little
description does not hurt :).

> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 30bbd9d..6ed9303 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -1976,7 +1976,11 @@ static int adv7604_get_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>         if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>                 struct v4l2_mbus_framefmt *fmt;
>
> -               fmt = v4l2_subdev_get_try_format(fh, format->pad);
> +               fmt = (fh == NULL) ? NULL
> +                       : v4l2_subdev_get_try_format(fh, format->pad);
> +               if (fmt == NULL)
> +                       return EINVAL;
> +

Mmmh, Hans probably has an explanation on this, I just don't get a use
case where fh can be NULL... So can't see the point of this patch ?

>                 format->format.code = fmt->code;
>         } else {
>                 format->format.code = state->format->code;
> @@ -2008,7 +2012,11 @@ static int adv7604_set_format(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>         if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>                 struct v4l2_mbus_framefmt *fmt;
>
> -               fmt = v4l2_subdev_get_try_format(fh, format->pad);
> +               fmt = (fh == NULL) ? NULL
> +                       : v4l2_subdev_get_try_format(fh, format->pad);
> +               if (fmt == NULL)
> +                       return -EINVAL;
> +
>                 fmt->code = format->format.code;
>         } else {
>                 state->format = info;

Same comment as above.
Thanks,
JM

Re: [PATCH 4/8] WmT: m-5mols_core style pad handling for adv7604

[William Towle]

Hi Jean-Michel and others,

On Thu, 29 Jan 2015, Jean-Michel Hautbois wrote:
> First of all, this subject puzzles me... What means WmT ??

   That's just my initialism, to differentiate my work from that of
colleagues'. I'll submit without those in due course (and SOBs).


>> -               fmt = v4l2_subdev_get_try_format(fh, format->pad);
>> +               fmt = (fh == NULL) ? NULL
>> +                       : v4l2_subdev_get_try_format(fh, format->pad);
>> +               if (fmt == NULL)
>> +                       return EINVAL;
>> +
>
> Mmmh, Hans probably has an explanation on this, I just don't get a use
> case where fh can be NULL... So can't see the point of this patch ?

   There isn't currently a case where fh can be NULL, but we do
introduce them into rcar_vin_try_fmt() in the places where we add
v4l2_subdev_has_op() tests, which I am hoping to clarify.

   I have seen Guennadi's suggestion regarding wrapper functions, and am
also looking at the patchset Hans mentioned. In particular, the
description of "v4l2-subdev: replace v4l2_subdev_fh by
v4l2_subdev_pad_config" stands out as relevant.

   ...Thanks all, I will let you know how I get on.

Cheers,
   Wills.

Re: [PATCH 4/8] WmT: m-5mols_core style pad handling for adv7604

[Hans Verkuil]

On 02/04/15 15:14, William Towle wrote:
> 
> Hi Jean-Michel and others,
> 
> On Thu, 29 Jan 2015, Jean-Michel Hautbois wrote:
>> First of all, this subject puzzles me... What means WmT ??
> 
>   That's just my initialism, to differentiate my work from that of
> colleagues'. I'll submit without those in due course (and SOBs).
> 
> 
>>> -               fmt = v4l2_subdev_get_try_format(fh, format->pad);
>>> +               fmt = (fh == NULL) ? NULL
>>> +                       : v4l2_subdev_get_try_format(fh, format->pad);
>>> +               if (fmt == NULL)
>>> +                       return EINVAL;
>>> +
>>
>> Mmmh, Hans probably has an explanation on this, I just don't get a use
>> case where fh can be NULL... So can't see the point of this patch ?
> 
>   There isn't currently a case where fh can be NULL, but we do
> introduce them into rcar_vin_try_fmt() in the places where we add
> v4l2_subdev_has_op() tests, which I am hoping to clarify.
> 
>   I have seen Guennadi's suggestion regarding wrapper functions, and am
> also looking at the patchset Hans mentioned. In particular, the
> description of "v4l2-subdev: replace v4l2_subdev_fh by
> v4l2_subdev_pad_config" stands out as relevant.

FYI: I've rebased my branch to the latest media_tree master. It's
available here:

http://git.linuxtv.org/cgit.cgi/hverkuil/media_tree.git/log/?h=subdev2

(the last patch fixing adv7180 conflicts will of course be properly
merged in the final version).

I might have time tomorrow to work on this a bit more.

I won't accept wrapper functions: this mess has gone on long enough
and I want to see a proper solution where the old non-pad functions
are removed so everyone uses the same APIs.

Regards,

	Hans

> 
>   ...Thanks all, I will let you know how I get on.
> 
> Cheers,
>   Wills.

[PATCH 5/8] media: rcar_vin: Add RGB888_1X24 input format support

[William Towle]

This adds V4L2_MBUS_FMT_RGB888_1X24 input format support
which is used by the ADV7612 chip.

Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
---
URL:    http://marc.info/?l=linux-sh&m=138002993417489&q=raw
FIXMEs required:
- "From:" as per URL
- adapted for lx3.18 by William Towle -> add S-o-b **
---
 drivers/media/platform/soc_camera/rcar_vin.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index c4f88c3..e4f60d3 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -74,6 +74,7 @@
 #define VNMC_INF_YUV10_BT656	(2 << 16)
 #define VNMC_INF_YUV10_BT601	(3 << 16)
 #define VNMC_INF_YUV16		(5 << 16)
+#define VNMC_INF_RGB888		(6 << 16)
 #define VNMC_VUP		(1 << 10)
 #define VNMC_IM_ODD		(0 << 3)
 #define VNMC_IM_ODD_EVEN	(1 << 3)
@@ -241,7 +242,7 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
 	struct soc_camera_device *icd = priv->ici.icd;
 	struct rcar_vin_cam *cam = icd->host_priv;
 	u32 vnmc, dmr, interrupts;
-	bool progressive = false, output_is_yuv = false;
+	bool progressive = false, output_is_yuv = false, input_is_yuv = false;
 
 	switch (priv->field) {
 	case V4L2_FIELD_TOP:
@@ -275,11 +276,16 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
 	case MEDIA_BUS_FMT_YUYV8_1X16:
 		/* BT.601/BT.1358 16bit YCbCr422 */
 		vnmc |= VNMC_INF_YUV16;
+		input_is_yuv = true;
 		break;
 	case MEDIA_BUS_FMT_YUYV8_2X8:
 		/* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
 		vnmc |= priv->pdata_flags & RCAR_VIN_BT656 ?
 			VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;
+		input_is_yuv = true;
+		break;
+	case MEDIA_BUS_FMT_RGB888_1X24:
+		vnmc |= VNMC_INF_RGB888;
 		break;
 	case MEDIA_BUS_FMT_YUYV10_2X10:
 		/* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */
@@ -328,7 +334,7 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
 	vnmc |= VNMC_VUP;
 
 	/* If input and output use the same colorspace, use bypass mode */
-	if (output_is_yuv)
+	if (input_is_yuv == output_is_yuv)
 		vnmc |= VNMC_BPS;
 
 	/* progressive or interlaced mode */
@@ -1015,6 +1021,7 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
 	case MEDIA_BUS_FMT_YUYV8_1X16:
 	case MEDIA_BUS_FMT_YUYV8_2X8:
 	case MEDIA_BUS_FMT_YUYV10_2X10:
+	case MEDIA_BUS_FMT_RGB888_1X24:
 		if (cam->extra_fmt)
 			break;
 
-- 
1.7.10.4

Re: [PATCH 5/8] media: rcar_vin: Add RGB888_1X24 input format support

[Sergei Shtylyov]

Hello.

On 01/29/2015 07:19 PM, William Towle wrote:

> This adds V4L2_MBUS_FMT_RGB888_1X24 input format support
> which is used by the ADV7612 chip.

> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>

    I wonder why it hasn't been merged still? It's pending since 2013, and I'm 
seeing no objections to it...

WBR, Sergei

Re: [PATCH 5/8] media: rcar_vin: Add RGB888_1X24 input format support

[Guennadi Liakhovetski]

Hi Sergei,

On Thu, 29 Jan 2015, Sergei Shtylyov wrote:

> Hello.
> 
> On 01/29/2015 07:19 PM, William Towle wrote:
> 
> > This adds V4L2_MBUS_FMT_RGB888_1X24 input format support
> > which is used by the ADV7612 chip.
> 
> > Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> 
>    I wonder why it hasn't been merged still? It's pending since 2013, and I'm
> seeing no objections to it...

Indeed, strange. I'm saving it for me to look at it for the next merge... 
and I'll double-check that series. Maybe the series had some objections, 
and for that reason this patch hasn't been picked up. I'll double-check 
anyway.

Thanks
Guennadi

> 
> WBR, Sergei
> 

Re: [PATCH 5/8] media: rcar_vin: Add RGB888_1X24 input format support

[Sergei Shtylyov]

Hello.

On 01/29/2015 09:18 PM, Guennadi Liakhovetski wrote:

>>> This adds V4L2_MBUS_FMT_RGB888_1X24 input format support
>>> which is used by the ADV7612 chip.

>>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>

>>     I wonder why it hasn't been merged still? It's pending since 2013, and I'm
>> seeing no objections to it...

> Indeed, strange. I'm saving it for me to look at it for the next merge...
> and I'll double-check that series. Maybe the series had some objections,

    Indeed, I'm now seeing the patch #1 was objected to. Patch #2 has been 
merged somewhat later.

> Thanks
> Guennadi

WBR, Sergei

Re: [PATCH 5/8] media: rcar_vin: Add RGB888_1X24 input format support

[Guennadi Liakhovetski]

On Thu, 29 Jan 2015, Sergei Shtylyov wrote:

> Hello.
> 
> On 01/29/2015 09:18 PM, Guennadi Liakhovetski wrote:
> 
> > > > This adds V4L2_MBUS_FMT_RGB888_1X24 input format support
> > > > which is used by the ADV7612 chip.
> 
> > > > Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> 
> > >     I wonder why it hasn't been merged still? It's pending since 2013, and
> > > I'm
> > > seeing no objections to it...
> 
> > Indeed, strange. I'm saving it for me to look at it for the next merge...
> > and I'll double-check that series. Maybe the series had some objections,
> 
>    Indeed, I'm now seeing the patch #1 was objected to. Patch #2 has been
> merged somewhat later.

Right, and since this RGB888 format support was needed for the ADV761X 
driver from patch #1, this patch wasn't merged either. Do you need it now 
for something different?

Thanks
Guennadi

Re: [PATCH 5/8] media: rcar_vin: Add RGB888_1X24 input format support

[Sergei Shtylyov]

On 01/29/2015 11:19 PM, Guennadi Liakhovetski wrote:

>>>>> This adds V4L2_MBUS_FMT_RGB888_1X24 input format support
>>>>> which is used by the ADV7612 chip.

>>>>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>

>>>>      I wonder why it hasn't been merged still? It's pending since 2013, and
>>>> I'm
>>>> seeing no objections to it...

>>> Indeed, strange. I'm saving it for me to look at it for the next merge...
>>> and I'll double-check that series. Maybe the series had some objections,

>>     Indeed, I'm now seeing the patch #1 was objected to. Patch #2 has been
>> merged somewhat later.

> Right, and since this RGB888 format support was needed for the ADV761X
> driver from patch #1, this patch wasn't merged either. Do you need it now
> for something different?

    No, the same ADV7612 chip, just the different driver this time, it seems.

> Thanks
> Guennadi

WBR, Sergei

Re: [PATCH 5/8] media: rcar_vin: Add RGB888_1X24 input format support

[Guennadi Liakhovetski]

On Thu, 29 Jan 2015, Sergei Shtylyov wrote:

> On 01/29/2015 11:19 PM, Guennadi Liakhovetski wrote:
> 
> > > > > > This adds V4L2_MBUS_FMT_RGB888_1X24 input format support
> > > > > > which is used by the ADV7612 chip.
> 
> > > > > > Signed-off-by: Valentine Barshak
> > > > > > <valentine.barshak@cogentembedded.com>
> 
> > > > >      I wonder why it hasn't been merged still? It's pending since
> > > > > 2013, and
> > > > > I'm
> > > > > seeing no objections to it...
> 
> > > > Indeed, strange. I'm saving it for me to look at it for the next
> > > > merge...
> > > > and I'll double-check that series. Maybe the series had some objections,
> 
> > >     Indeed, I'm now seeing the patch #1 was objected to. Patch #2 has been
> > > merged somewhat later.
> 
> > Right, and since this RGB888 format support was needed for the ADV761X
> > driver from patch #1, this patch wasn't merged either. Do you need it now
> > for something different?
> 
>    No, the same ADV7612 chip, just the different driver this time, it seems.

Right, I see now. [OT] The problem is - this is not the first time this is 
happening - I didn't get that thread in my INBOX, only in the mailing list 
folder. I subscribe the mailing list from a different email address, than 
the one I'm CC'ed at. So, I anyway should be getting 2 copies of all these 
mails. I received 2 copies of Sergei's mails, but the rest only once... 
Not in spam, not in logs - they just disappear. A day or two ago another 
similar thread also missed my INBOX... Investigating...

Thanks
Guennadi

Re: [PATCH 5/8] media: rcar_vin: Add RGB888_1X24 input format support

[Guennadi Liakhovetski]

On Thu, 29 Jan 2015, Guennadi Liakhovetski wrote:

> Right, I see now. [OT] The problem is - this is not the first time this is 
> happening - I didn't get that thread in my INBOX, only in the mailing list 
> folder. I subscribe the mailing list from a different email address, than 
> the one I'm CC'ed at. So, I anyway should be getting 2 copies of all these 
> mails. I received 2 copies of Sergei's mails, but the rest only once... 
> Not in spam, not in logs - they just disappear. A day or two ago another 
> similar thread also missed my INBOX... Investigating...

Ok, I see now. Only Wills' emails aren't hitting my mailbox. Looks like 
his mail-provider is blocking gmx.de in both directions, so, I'm hoping 
he'll read this mail here...

Thanks
Guennadi

Re: [PATCH 5/8] media: rcar_vin: Add RGB888_1X24 input format support

[Guennadi Liakhovetski]

Hi Wills,

On Thu, 29 Jan 2015, William Towle wrote:

> This adds V4L2_MBUS_FMT_RGB888_1X24 input format support
> which is used by the ADV7612 chip.
> 
> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> ---
> URL:    http://marc.info/?l=linux-sh&m=138002993417489&q=raw
> FIXMEs required:
> - "From:" as per URL
> - adapted for lx3.18 by William Towle -> add S-o-b **

Yes, please, add your Sob and the original authorship. Which, btw, isn't 
this patch a modified version of 
http://lists.kde.org/?l=linux-sh&m=141476801629391&w=4 ? I.e. shouldn't it 
be

From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>

? And yes, I like this version better, because it sets the VNMC_BPS bit in 
one step instead of two.

Thanks
Guennadi

> ---
>  drivers/media/platform/soc_camera/rcar_vin.c |   11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index c4f88c3..e4f60d3 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> @@ -74,6 +74,7 @@
>  #define VNMC_INF_YUV10_BT656	(2 << 16)
>  #define VNMC_INF_YUV10_BT601	(3 << 16)
>  #define VNMC_INF_YUV16		(5 << 16)
> +#define VNMC_INF_RGB888		(6 << 16)
>  #define VNMC_VUP		(1 << 10)
>  #define VNMC_IM_ODD		(0 << 3)
>  #define VNMC_IM_ODD_EVEN	(1 << 3)
> @@ -241,7 +242,7 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
>  	struct soc_camera_device *icd = priv->ici.icd;
>  	struct rcar_vin_cam *cam = icd->host_priv;
>  	u32 vnmc, dmr, interrupts;
> -	bool progressive = false, output_is_yuv = false;
> +	bool progressive = false, output_is_yuv = false, input_is_yuv = false;
>  
>  	switch (priv->field) {
>  	case V4L2_FIELD_TOP:
> @@ -275,11 +276,16 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
>  	case MEDIA_BUS_FMT_YUYV8_1X16:
>  		/* BT.601/BT.1358 16bit YCbCr422 */
>  		vnmc |= VNMC_INF_YUV16;
> +		input_is_yuv = true;
>  		break;
>  	case MEDIA_BUS_FMT_YUYV8_2X8:
>  		/* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
>  		vnmc |= priv->pdata_flags & RCAR_VIN_BT656 ?
>  			VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;
> +		input_is_yuv = true;
> +		break;
> +	case MEDIA_BUS_FMT_RGB888_1X24:
> +		vnmc |= VNMC_INF_RGB888;
>  		break;
>  	case MEDIA_BUS_FMT_YUYV10_2X10:
>  		/* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */
> @@ -328,7 +334,7 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
>  	vnmc |= VNMC_VUP;
>  
>  	/* If input and output use the same colorspace, use bypass mode */
> -	if (output_is_yuv)
> +	if (input_is_yuv == output_is_yuv)
>  		vnmc |= VNMC_BPS;
>  
>  	/* progressive or interlaced mode */
> @@ -1015,6 +1021,7 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
>  	case MEDIA_BUS_FMT_YUYV8_1X16:
>  	case MEDIA_BUS_FMT_YUYV8_2X8:
>  	case MEDIA_BUS_FMT_YUYV10_2X10:
> +	case MEDIA_BUS_FMT_RGB888_1X24:
>  		if (cam->extra_fmt)
>  			break;
>  
> -- 
> 1.7.10.4
> 
> --
> 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
> 

[PATCH 6/8] WmT: adv7604 driver compatibility

[William Towle]

Add 'struct media_pad pad' member and suitable glue code, so that
soc_camera/rcar_vin can become agnostic to whether an old or new-
style driver (wrt pad API use) can sit underneath

This version has been reworked to include appropriate constant and
datatype names for kernel v3.18
---
 drivers/media/platform/soc_camera/soc_camera.c     |  148 +++++++++++++++++++-
 drivers/media/platform/soc_camera/soc_scale_crop.c |   43 +++++-
 include/media/soc_camera.h                         |    1 +
 3 files changed, 182 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index f4be2a1..efc20bf 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -37,8 +37,11 @@
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-dev.h>
 #include <media/v4l2-of.h>
+#if 0
 #include <media/videobuf-core.h>
 #include <media/videobuf2-core.h>
+#endif
+#include <media/v4l2-mediabus.h>
 
 /* Default to VGA resolution */
 #define DEFAULT_WIDTH	640
@@ -453,6 +456,98 @@ static int soc_camera_expbuf(struct file *file, void *priv,
 		return vb2_expbuf(&icd->vb2_vidq, p);
 }
 
+static int soc_camera_init_user_formats_pad(struct soc_camera_device *icd, int src_pad_idx)
+{
+	struct v4l2_subdev *sd= soc_camera_to_subdev(icd);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
+	struct v4l2_subdev_mbus_code_enum code;
+	int fmts= 0, raw_fmts, i, ret;
+
+	code.pad= src_pad_idx;
+	code.index= 0;
+
+	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
+	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
+		while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code))
+			code.index++;
+	} else {
+		u32 pixcode;
+
+		while (!v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index, &pixcode))
+		{
+			code.code= pixcode;
+			code.index++;
+		}
+	}
+	raw_fmts= code.index;
+
+	if (!ici->ops->get_formats) {
+		/*
+		 * Fallback mode - the host will have to serve all
+		 * sensor-provided formats one-to-one to the user
+		 */
+		fmts = raw_fmts;
+	}
+	else {
+		/*
+		 * First pass - only count formats this host-sensor
+		 * configuration can provide
+		 */
+		for (i = 0; i < raw_fmts; i++) {
+			int ret = ici->ops->get_formats(icd, i, NULL);
+			if (ret < 0)
+				return ret;
+			fmts += ret;
+		}
+	}
+
+	if (!fmts)
+		return -ENXIO;
+
+	icd->user_formats =
+		vmalloc(fmts * sizeof(struct soc_camera_format_xlate));
+	if (!icd->user_formats)
+		return -ENOMEM;
+
+	dev_dbg(icd->pdev, "Found %d supported formats.\n", fmts);
+
+	/* Second pass - actually fill data formats */
+	fmts = 0;
+	for (i = 0; i < raw_fmts; i++) {
+		if (!ici->ops->get_formats) {
+			code.index= i;
+			// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
+			if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
+				v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code);
+			} else {
+				u32 pixcode;
+
+				v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index, &pixcode);
+				code.code= pixcode;
+			}
+			icd->user_formats[fmts].host_fmt =
+				soc_mbus_get_fmtdesc(code.code);
+			if (icd->user_formats[fmts].host_fmt)
+				icd->user_formats[fmts++].code = code.code;
+		} else {
+			ret = ici->ops->get_formats(icd, i,
+						    &icd->user_formats[fmts]);
+			if (ret < 0)
+				goto egfmt;
+			fmts += ret;
+		}
+	}
+
+	icd->num_user_formats = fmts;
+	icd->current_fmt = &icd->user_formats[0];
+
+	return 0;
+
+egfmt:
+	vfree(icd->user_formats);
+	return ret;
+}
+
 /* Always entered with .host_lock held */
 static int soc_camera_init_user_formats(struct soc_camera_device *icd)
 {
@@ -1289,6 +1384,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
 {
 	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
 	struct v4l2_mbus_framefmt mf;
+	int src_pad_idx= -1;
 	int ret;
 
 	sd->grp_id = soc_camera_grp_id(icd);
@@ -1307,7 +1403,30 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
 	}
 
 	/* At this point client .probe() should have run already */
-	ret = soc_camera_init_user_formats(icd);
+	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
+	if (!v4l2_subdev_has_op(sd, pad, enum_mbus_code))
+		ret = soc_camera_init_user_formats(icd);
+	else {
+		ret = media_entity_init(&icd->vdev->entity, 1,
+					&icd->pad, 0);
+		if (!ret) {
+			for (src_pad_idx= 0; src_pad_idx < sd->entity.num_pads; src_pad_idx++)
+				if (sd->entity.pads[src_pad_idx].flags == MEDIA_PAD_FL_SOURCE)
+					break;
+
+			if (src_pad_idx < sd->entity.num_pads) {
+				ret = media_entity_create_link(
+					&icd->vdev->entity, 0,
+					&sd->entity, src_pad_idx,
+					MEDIA_LNK_FL_IMMUTABLE |
+					MEDIA_LNK_FL_ENABLED);
+			}
+		}
+
+		if (!ret)
+			ret = soc_camera_init_user_formats_pad(icd,
+							src_pad_idx);
+	}
 	if (ret < 0)
 		goto eusrfmt;
 
@@ -1318,11 +1437,28 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
 		goto evidstart;
 
 	/* Try to improve our guess of a reasonable window format */
-	if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
-		icd->user_width		= mf.width;
-		icd->user_height	= mf.height;
-		icd->colorspace		= mf.colorspace;
-		icd->field		= mf.field;
+	// subdev_has_op -> get_fmt vs g_mbus_fmt
+	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)
+		&& v4l2_subdev_has_op(sd, pad, get_fmt)
+		&& src_pad_idx != -1) {
+		struct v4l2_subdev_format sd_format;
+
+		sd_format.pad= src_pad_idx;
+		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
+
+		if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_format)) {
+			icd->user_width		= sd_format.format.width;
+			icd->user_height	= sd_format.format.height;
+			icd->colorspace		= sd_format.format.colorspace;
+			icd->field		= sd_format.format.field;
+		}
+	} else {
+		if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
+			icd->user_width		= mf.width;
+			icd->user_height	= mf.height;
+			icd->colorspace		= mf.colorspace;
+			icd->field		= mf.field;
+		}
 	}
 	soc_camera_remove_device(icd);
 
diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c b/drivers/media/platform/soc_camera/soc_scale_crop.c
index 8e74fb7..8a1ca05 100644
--- a/drivers/media/platform/soc_camera/soc_scale_crop.c
+++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
@@ -224,9 +224,27 @@ static int client_s_fmt(struct soc_camera_device *icd,
 	bool host_1to1;
 	int ret;
 
-	ret = v4l2_device_call_until_err(sd->v4l2_dev,
-					 soc_camera_grp_id(icd), video,
-					 s_mbus_fmt, mf);
+	// subdev_has_op -> set_fmt vs s_mbus_fmt
+	if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
+		struct v4l2_subdev_format sd_format;
+		struct media_pad *remote_pad;
+
+		remote_pad= media_entity_remote_pad(
+			&icd->vdev->entity.pads[0]);
+		sd_format.pad = remote_pad->index;
+		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
+		sd_format.format= *mf;
+
+		ret = v4l2_device_call_until_err(sd->v4l2_dev,
+			soc_camera_grp_id(icd), pad, set_fmt, NULL,
+			&sd_format);
+
+		mf->width = sd_format.format.width;
+		mf->height = sd_format.format.height;
+	} else {
+		ret = v4l2_device_call_until_err(sd->v4l2_dev,
+			 soc_camera_grp_id(icd), video, s_mbus_fmt, mf);
+	}
 	if (ret < 0)
 		return ret;
 
@@ -264,9 +282,26 @@ static int client_s_fmt(struct soc_camera_device *icd,
 		tmp_h = min(2 * tmp_h, max_height);
 		mf->width = tmp_w;
 		mf->height = tmp_h;
-		ret = v4l2_device_call_until_err(sd->v4l2_dev,
+		// subdev_has_op -> set_fmt vs s_mbus_fmt
+		if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
+			struct v4l2_subdev_format sd_format;
+			struct media_pad *remote_pad;
+
+			remote_pad= media_entity_remote_pad(
+				&icd->vdev->entity.pads[0]);
+			sd_format.pad = remote_pad->index;
+			sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
+			sd_format.format= *mf;
+
+			ret = v4l2_device_call_until_err(sd->v4l2_dev,
+					soc_camera_grp_id(icd),
+					pad, set_fmt, NULL,
+					&sd_format);
+		} else {
+			ret = v4l2_device_call_until_err(sd->v4l2_dev,
 					soc_camera_grp_id(icd), video,
 					s_mbus_fmt, mf);
+		}
 		dev_geo(dev, "Camera scaled to %ux%u\n",
 			mf->width, mf->height);
 		if (ret < 0) {
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index 2f6261f..f0c5238 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -42,6 +42,7 @@ struct soc_camera_device {
 	unsigned char devnum;		/* Device number per host */
 	struct soc_camera_sense *sense;	/* See comment in struct definition */
 	struct video_device *vdev;
+	struct media_pad pad;
 	struct v4l2_ctrl_handler ctrl_handler;
 	const struct soc_camera_format_xlate *current_fmt;
 	struct soc_camera_format_xlate *user_formats;
-- 
1.7.10.4

Re: [PATCH 6/8] WmT: adv7604 driver compatibility

[Guennadi Liakhovetski]

Hi Wills,

Thanks for the patch. First and foremost, the title of the patch is wrong. 
This patch does more than just adding some "adv7604 compatibility." It's 
adding pad-level API to soc-camera.

This is just a rough review. I'm not an expert in media-controller / 
pad-level API, I hope someone with a better knowledge of those areas will 
help me reviewing this.

Another general comment: it has been discussed since a long time, whether 
a wrapper wouldn't be desired to enable a seamless use of both subdev 
drivers using and not using the pad-level API. Maybe it's the right time 
now?..

On Thu, 29 Jan 2015, William Towle wrote:

> Add 'struct media_pad pad' member and suitable glue code, so that
> soc_camera/rcar_vin can become agnostic to whether an old or new-
> style driver (wrt pad API use) can sit underneath
> 
> This version has been reworked to include appropriate constant and
> datatype names for kernel v3.18
> ---
>  drivers/media/platform/soc_camera/soc_camera.c     |  148 +++++++++++++++++++-
>  drivers/media/platform/soc_camera/soc_scale_crop.c |   43 +++++-
>  include/media/soc_camera.h                         |    1 +
>  3 files changed, 182 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index f4be2a1..efc20bf 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -37,8 +37,11 @@
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-of.h>
> +#if 0
>  #include <media/videobuf-core.h>
>  #include <media/videobuf2-core.h>
> +#endif

No. These headers are needed even if the code can be compiled without 
them.

> +#include <media/v4l2-mediabus.h>

Well, maybe. This header is included indirectly via soc_mediabus.h, but 
yes, as I just said above, headers, whose defines, structs etc. are used, 
should be encluded directly. Further, you'll need more headers, e.g. 
media-entity.h, maybe some more.

>  /* Default to VGA resolution */
>  #define DEFAULT_WIDTH	640
> @@ -453,6 +456,98 @@ static int soc_camera_expbuf(struct file *file, void *priv,
>  		return vb2_expbuf(&icd->vb2_vidq, p);
>  }
>  
> +static int soc_camera_init_user_formats_pad(struct soc_camera_device *icd, int src_pad_idx)
> +{
> +	struct v4l2_subdev *sd= soc_camera_to_subdev(icd);
> +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> +	struct v4l2_subdev_mbus_code_enum code;
> +	int fmts= 0, raw_fmts, i, ret;

Please, run this patch through checkpatch.pl. It will tell you to add a 
Signed-off-by line, (hopefully) to add spaces before "=" in multiple 
places, to place braces correctly, to not use C++-style comments etc. Only 
feel free to ignore 80-character warnings.

> +
> +	code.pad= src_pad_idx;
> +	code.index= 0;
> +
> +	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> +	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {

This function is called only once below and only after the above test has 
already returned success. Looks like you don't need it here again and the 
below "else" branch can be dropped completely?

> +		while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code))
> +			code.index++;
> +	} else {
> +		u32 pixcode;
> +
> +		while (!v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index, &pixcode))
> +		{
> +			code.code= pixcode;
> +			code.index++;
> +		}
> +	}
> +	raw_fmts= code.index;
> +
> +	if (!ici->ops->get_formats) {
> +		/*
> +		 * Fallback mode - the host will have to serve all
> +		 * sensor-provided formats one-to-one to the user
> +		 */
> +		fmts = raw_fmts;
> +	}
> +	else {
> +		/*
> +		 * First pass - only count formats this host-sensor
> +		 * configuration can provide
> +		 */
> +		for (i = 0; i < raw_fmts; i++) {
> +			int ret = ici->ops->get_formats(icd, i, NULL);
> +			if (ret < 0)
> +				return ret;
> +			fmts += ret;
> +		}
> +	}
> +
> +	if (!fmts)
> +		return -ENXIO;
> +
> +	icd->user_formats =
> +		vmalloc(fmts * sizeof(struct soc_camera_format_xlate));
> +	if (!icd->user_formats)
> +		return -ENOMEM;
> +
> +	dev_dbg(icd->pdev, "Found %d supported formats.\n", fmts);
> +
> +	/* Second pass - actually fill data formats */
> +	fmts = 0;
> +	for (i = 0; i < raw_fmts; i++) {
> +		if (!ici->ops->get_formats) {
> +			code.index= i;
> +			// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> +			if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {

Same test again?? Or am I missing something? If indeed these tests are 
redundant, after you remove them this function will become very similar to 
the original soc_camera_init_user_formats(), so, maybe some code reuse 
will become possible.

> +				v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code);
> +			} else {
> +				u32 pixcode;
> +
> +				v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index, &pixcode);
> +				code.code= pixcode;
> +			}
> +			icd->user_formats[fmts].host_fmt =
> +				soc_mbus_get_fmtdesc(code.code);
> +			if (icd->user_formats[fmts].host_fmt)
> +				icd->user_formats[fmts++].code = code.code;
> +		} else {
> +			ret = ici->ops->get_formats(icd, i,
> +						    &icd->user_formats[fmts]);
> +			if (ret < 0)
> +				goto egfmt;
> +			fmts += ret;
> +		}
> +	}
> +
> +	icd->num_user_formats = fmts;
> +	icd->current_fmt = &icd->user_formats[0];
> +
> +	return 0;
> +
> +egfmt:
> +	vfree(icd->user_formats);
> +	return ret;
> +}
> +
>  /* Always entered with .host_lock held */
>  static int soc_camera_init_user_formats(struct soc_camera_device *icd)
>  {
> @@ -1289,6 +1384,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
>  {
>  	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>  	struct v4l2_mbus_framefmt mf;
> +	int src_pad_idx= -1;
>  	int ret;
>  
>  	sd->grp_id = soc_camera_grp_id(icd);
> @@ -1307,7 +1403,30 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
>  	}
>  
>  	/* At this point client .probe() should have run already */
> -	ret = soc_camera_init_user_formats(icd);
> +	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> +	if (!v4l2_subdev_has_op(sd, pad, enum_mbus_code))

This is the test, that I meant above.

> +		ret = soc_camera_init_user_formats(icd);
> +	else {
> +		ret = media_entity_init(&icd->vdev->entity, 1,
> +					&icd->pad, 0);

Ok, maybe this hard-coded 1 pad with no extras is justified here, but 
let's here what others say.

> +		if (!ret) {
> +			for (src_pad_idx= 0; src_pad_idx < sd->entity.num_pads; src_pad_idx++)
> +				if (sd->entity.pads[src_pad_idx].flags == MEDIA_PAD_FL_SOURCE)
> +					break;
> +
> +			if (src_pad_idx < sd->entity.num_pads) {
> +				ret = media_entity_create_link(
> +					&icd->vdev->entity, 0,
> +					&sd->entity, src_pad_idx,
> +					MEDIA_LNK_FL_IMMUTABLE |
> +					MEDIA_LNK_FL_ENABLED);

Let's try to preserve the style. I normally try to avoid splitting the 
line after "f(" and adding at least the first function parameter above 
will not make that line longer, than the ones above. So, let's do that.

> +			}
> +		}
> +
> +		if (!ret)
> +			ret = soc_camera_init_user_formats_pad(icd,
> +							src_pad_idx);

Probably no need to break the line here either.

> +	}
>  	if (ret < 0)
>  		goto eusrfmt;
>  
> @@ -1318,11 +1437,28 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
>  		goto evidstart;
>  
>  	/* Try to improve our guess of a reasonable window format */
> -	if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> -		icd->user_width		= mf.width;
> -		icd->user_height	= mf.height;
> -		icd->colorspace		= mf.colorspace;
> -		icd->field		= mf.field;
> +	// subdev_has_op -> get_fmt vs g_mbus_fmt
> +	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)
> +		&& v4l2_subdev_has_op(sd, pad, get_fmt)
> +		&& src_pad_idx != -1) {

The rest of the file puts operations after the first argument, not before 
the second one when breaking the line. Let's do that here too.

Thanks
Guennadi

> +		struct v4l2_subdev_format sd_format;
> +
> +		sd_format.pad= src_pad_idx;
> +		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> +
> +		if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_format)) {
> +			icd->user_width		= sd_format.format.width;
> +			icd->user_height	= sd_format.format.height;
> +			icd->colorspace		= sd_format.format.colorspace;
> +			icd->field		= sd_format.format.field;
> +		}
> +	} else {
> +		if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> +			icd->user_width		= mf.width;
> +			icd->user_height	= mf.height;
> +			icd->colorspace		= mf.colorspace;
> +			icd->field		= mf.field;
> +		}
>  	}
>  	soc_camera_remove_device(icd);
>  
> diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c b/drivers/media/platform/soc_camera/soc_scale_crop.c
> index 8e74fb7..8a1ca05 100644
> --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
> +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
> @@ -224,9 +224,27 @@ static int client_s_fmt(struct soc_camera_device *icd,
>  	bool host_1to1;
>  	int ret;
>  
> -	ret = v4l2_device_call_until_err(sd->v4l2_dev,
> -					 soc_camera_grp_id(icd), video,
> -					 s_mbus_fmt, mf);
> +	// subdev_has_op -> set_fmt vs s_mbus_fmt
> +	if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> +		struct v4l2_subdev_format sd_format;
> +		struct media_pad *remote_pad;
> +
> +		remote_pad= media_entity_remote_pad(
> +			&icd->vdev->entity.pads[0]);
> +		sd_format.pad = remote_pad->index;
> +		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> +		sd_format.format= *mf;
> +
> +		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> +			soc_camera_grp_id(icd), pad, set_fmt, NULL,
> +			&sd_format);
> +
> +		mf->width = sd_format.format.width;
> +		mf->height = sd_format.format.height;
> +	} else {
> +		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> +			 soc_camera_grp_id(icd), video, s_mbus_fmt, mf);
> +	}
>  	if (ret < 0)
>  		return ret;
>  
> @@ -264,9 +282,26 @@ static int client_s_fmt(struct soc_camera_device *icd,
>  		tmp_h = min(2 * tmp_h, max_height);
>  		mf->width = tmp_w;
>  		mf->height = tmp_h;
> -		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> +		// subdev_has_op -> set_fmt vs s_mbus_fmt
> +		if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> +			struct v4l2_subdev_format sd_format;
> +			struct media_pad *remote_pad;
> +
> +			remote_pad= media_entity_remote_pad(
> +				&icd->vdev->entity.pads[0]);
> +			sd_format.pad = remote_pad->index;
> +			sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> +			sd_format.format= *mf;
> +
> +			ret = v4l2_device_call_until_err(sd->v4l2_dev,
> +					soc_camera_grp_id(icd),
> +					pad, set_fmt, NULL,
> +					&sd_format);
> +		} else {
> +			ret = v4l2_device_call_until_err(sd->v4l2_dev,
>  					soc_camera_grp_id(icd), video,
>  					s_mbus_fmt, mf);
> +		}
>  		dev_geo(dev, "Camera scaled to %ux%u\n",
>  			mf->width, mf->height);
>  		if (ret < 0) {
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index 2f6261f..f0c5238 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -42,6 +42,7 @@ struct soc_camera_device {
>  	unsigned char devnum;		/* Device number per host */
>  	struct soc_camera_sense *sense;	/* See comment in struct definition */
>  	struct video_device *vdev;
> +	struct media_pad pad;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	const struct soc_camera_format_xlate *current_fmt;
>  	struct soc_camera_format_xlate *user_formats;
> -- 
> 1.7.10.4
> 
> --
> 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 6/8] WmT: adv7604 driver compatibility

[Guennadi Liakhovetski]

On a second thought:

On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:

> Hi Wills,
> 
> Thanks for the patch. First and foremost, the title of the patch is wrong. 
> This patch does more than just adding some "adv7604 compatibility." It's 
> adding pad-level API to soc-camera.
> 
> This is just a rough review. I'm not an expert in media-controller / 
> pad-level API, I hope someone with a better knowledge of those areas will 
> help me reviewing this.
> 
> Another general comment: it has been discussed since a long time, whether 
> a wrapper wouldn't be desired to enable a seamless use of both subdev 
> drivers using and not using the pad-level API. Maybe it's the right time 
> now?..

This would be a considerable change and would most probably take a rather 
long time, given how busy everyone is. I personally would be fine with a 
(yet another) intermittent solution, whereby we create a 
soc_camera_subdev.c file, in which we collect all those function to call 
either a video or a pad subdev operation, depending on whether the latter 
is available. E.g.

int soc_camera_sd_enum_mbus_fmt(sd, code)
{
	int ret;

	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
		ret = v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, code);
	} else {
		u32 pixcode;

		ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, code->index, &pixcode);
		if (!ret)
			code->code= pixcode;
	}

	return ret;
}

Similarly for other ops. 

Thanks
Guennadi

> 
> On Thu, 29 Jan 2015, William Towle wrote:
> 
> > Add 'struct media_pad pad' member and suitable glue code, so that
> > soc_camera/rcar_vin can become agnostic to whether an old or new-
> > style driver (wrt pad API use) can sit underneath
> > 
> > This version has been reworked to include appropriate constant and
> > datatype names for kernel v3.18
> > ---
> >  drivers/media/platform/soc_camera/soc_camera.c     |  148 +++++++++++++++++++-
> >  drivers/media/platform/soc_camera/soc_scale_crop.c |   43 +++++-
> >  include/media/soc_camera.h                         |    1 +
> >  3 files changed, 182 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> > index f4be2a1..efc20bf 100644
> > --- a/drivers/media/platform/soc_camera/soc_camera.c
> > +++ b/drivers/media/platform/soc_camera/soc_camera.c
> > @@ -37,8 +37,11 @@
> >  #include <media/v4l2-ioctl.h>
> >  #include <media/v4l2-dev.h>
> >  #include <media/v4l2-of.h>
> > +#if 0
> >  #include <media/videobuf-core.h>
> >  #include <media/videobuf2-core.h>
> > +#endif
> 
> No. These headers are needed even if the code can be compiled without 
> them.
> 
> > +#include <media/v4l2-mediabus.h>
> 
> Well, maybe. This header is included indirectly via soc_mediabus.h, but 
> yes, as I just said above, headers, whose defines, structs etc. are used, 
> should be encluded directly. Further, you'll need more headers, e.g. 
> media-entity.h, maybe some more.
> 
> >  /* Default to VGA resolution */
> >  #define DEFAULT_WIDTH	640
> > @@ -453,6 +456,98 @@ static int soc_camera_expbuf(struct file *file, void *priv,
> >  		return vb2_expbuf(&icd->vb2_vidq, p);
> >  }
> >  
> > +static int soc_camera_init_user_formats_pad(struct soc_camera_device *icd, int src_pad_idx)
> > +{
> > +	struct v4l2_subdev *sd= soc_camera_to_subdev(icd);
> > +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> > +	struct v4l2_subdev_mbus_code_enum code;
> > +	int fmts= 0, raw_fmts, i, ret;
> 
> Please, run this patch through checkpatch.pl. It will tell you to add a 
> Signed-off-by line, (hopefully) to add spaces before "=" in multiple 
> places, to place braces correctly, to not use C++-style comments etc. Only 
> feel free to ignore 80-character warnings.
> 
> > +
> > +	code.pad= src_pad_idx;
> > +	code.index= 0;
> > +
> > +	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > +	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> 
> This function is called only once below and only after the above test has 
> already returned success. Looks like you don't need it here again and the 
> below "else" branch can be dropped completely?
> 
> > +		while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code))
> > +			code.index++;
> > +	} else {
> > +		u32 pixcode;
> > +
> > +		while (!v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index, &pixcode))
> > +		{
> > +			code.code= pixcode;
> > +			code.index++;
> > +		}
> > +	}
> > +	raw_fmts= code.index;
> > +
> > +	if (!ici->ops->get_formats) {
> > +		/*
> > +		 * Fallback mode - the host will have to serve all
> > +		 * sensor-provided formats one-to-one to the user
> > +		 */
> > +		fmts = raw_fmts;
> > +	}
> > +	else {
> > +		/*
> > +		 * First pass - only count formats this host-sensor
> > +		 * configuration can provide
> > +		 */
> > +		for (i = 0; i < raw_fmts; i++) {
> > +			int ret = ici->ops->get_formats(icd, i, NULL);
> > +			if (ret < 0)
> > +				return ret;
> > +			fmts += ret;
> > +		}
> > +	}
> > +
> > +	if (!fmts)
> > +		return -ENXIO;
> > +
> > +	icd->user_formats =
> > +		vmalloc(fmts * sizeof(struct soc_camera_format_xlate));
> > +	if (!icd->user_formats)
> > +		return -ENOMEM;
> > +
> > +	dev_dbg(icd->pdev, "Found %d supported formats.\n", fmts);
> > +
> > +	/* Second pass - actually fill data formats */
> > +	fmts = 0;
> > +	for (i = 0; i < raw_fmts; i++) {
> > +		if (!ici->ops->get_formats) {
> > +			code.index= i;
> > +			// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > +			if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> 
> Same test again?? Or am I missing something? If indeed these tests are 
> redundant, after you remove them this function will become very similar to 
> the original soc_camera_init_user_formats(), so, maybe some code reuse 
> will become possible.
> 
> > +				v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code);
> > +			} else {
> > +				u32 pixcode;
> > +
> > +				v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index, &pixcode);
> > +				code.code= pixcode;
> > +			}
> > +			icd->user_formats[fmts].host_fmt =
> > +				soc_mbus_get_fmtdesc(code.code);
> > +			if (icd->user_formats[fmts].host_fmt)
> > +				icd->user_formats[fmts++].code = code.code;
> > +		} else {
> > +			ret = ici->ops->get_formats(icd, i,
> > +						    &icd->user_formats[fmts]);
> > +			if (ret < 0)
> > +				goto egfmt;
> > +			fmts += ret;
> > +		}
> > +	}
> > +
> > +	icd->num_user_formats = fmts;
> > +	icd->current_fmt = &icd->user_formats[0];
> > +
> > +	return 0;
> > +
> > +egfmt:
> > +	vfree(icd->user_formats);
> > +	return ret;
> > +}
> > +
> >  /* Always entered with .host_lock held */
> >  static int soc_camera_init_user_formats(struct soc_camera_device *icd)
> >  {
> > @@ -1289,6 +1384,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
> >  {
> >  	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> >  	struct v4l2_mbus_framefmt mf;
> > +	int src_pad_idx= -1;
> >  	int ret;
> >  
> >  	sd->grp_id = soc_camera_grp_id(icd);
> > @@ -1307,7 +1403,30 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
> >  	}
> >  
> >  	/* At this point client .probe() should have run already */
> > -	ret = soc_camera_init_user_formats(icd);
> > +	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > +	if (!v4l2_subdev_has_op(sd, pad, enum_mbus_code))
> 
> This is the test, that I meant above.
> 
> > +		ret = soc_camera_init_user_formats(icd);
> > +	else {
> > +		ret = media_entity_init(&icd->vdev->entity, 1,
> > +					&icd->pad, 0);
> 
> Ok, maybe this hard-coded 1 pad with no extras is justified here, but 
> let's here what others say.
> 
> > +		if (!ret) {
> > +			for (src_pad_idx= 0; src_pad_idx < sd->entity.num_pads; src_pad_idx++)
> > +				if (sd->entity.pads[src_pad_idx].flags == MEDIA_PAD_FL_SOURCE)
> > +					break;
> > +
> > +			if (src_pad_idx < sd->entity.num_pads) {
> > +				ret = media_entity_create_link(
> > +					&icd->vdev->entity, 0,
> > +					&sd->entity, src_pad_idx,
> > +					MEDIA_LNK_FL_IMMUTABLE |
> > +					MEDIA_LNK_FL_ENABLED);
> 
> Let's try to preserve the style. I normally try to avoid splitting the 
> line after "f(" and adding at least the first function parameter above 
> will not make that line longer, than the ones above. So, let's do that.
> 
> > +			}
> > +		}
> > +
> > +		if (!ret)
> > +			ret = soc_camera_init_user_formats_pad(icd,
> > +							src_pad_idx);
> 
> Probably no need to break the line here either.
> 
> > +	}
> >  	if (ret < 0)
> >  		goto eusrfmt;
> >  
> > @@ -1318,11 +1437,28 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
> >  		goto evidstart;
> >  
> >  	/* Try to improve our guess of a reasonable window format */
> > -	if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> > -		icd->user_width		= mf.width;
> > -		icd->user_height	= mf.height;
> > -		icd->colorspace		= mf.colorspace;
> > -		icd->field		= mf.field;
> > +	// subdev_has_op -> get_fmt vs g_mbus_fmt
> > +	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)
> > +		&& v4l2_subdev_has_op(sd, pad, get_fmt)
> > +		&& src_pad_idx != -1) {
> 
> The rest of the file puts operations after the first argument, not before 
> the second one when breaking the line. Let's do that here too.
> 
> Thanks
> Guennadi
> 
> > +		struct v4l2_subdev_format sd_format;
> > +
> > +		sd_format.pad= src_pad_idx;
> > +		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > +
> > +		if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_format)) {
> > +			icd->user_width		= sd_format.format.width;
> > +			icd->user_height	= sd_format.format.height;
> > +			icd->colorspace		= sd_format.format.colorspace;
> > +			icd->field		= sd_format.format.field;
> > +		}
> > +	} else {
> > +		if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> > +			icd->user_width		= mf.width;
> > +			icd->user_height	= mf.height;
> > +			icd->colorspace		= mf.colorspace;
> > +			icd->field		= mf.field;
> > +		}
> >  	}
> >  	soc_camera_remove_device(icd);
> >  
> > diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c b/drivers/media/platform/soc_camera/soc_scale_crop.c
> > index 8e74fb7..8a1ca05 100644
> > --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
> > +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
> > @@ -224,9 +224,27 @@ static int client_s_fmt(struct soc_camera_device *icd,
> >  	bool host_1to1;
> >  	int ret;
> >  
> > -	ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > -					 soc_camera_grp_id(icd), video,
> > -					 s_mbus_fmt, mf);
> > +	// subdev_has_op -> set_fmt vs s_mbus_fmt
> > +	if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> > +		struct v4l2_subdev_format sd_format;
> > +		struct media_pad *remote_pad;
> > +
> > +		remote_pad= media_entity_remote_pad(
> > +			&icd->vdev->entity.pads[0]);
> > +		sd_format.pad = remote_pad->index;
> > +		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > +		sd_format.format= *mf;
> > +
> > +		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > +			soc_camera_grp_id(icd), pad, set_fmt, NULL,
> > +			&sd_format);
> > +
> > +		mf->width = sd_format.format.width;
> > +		mf->height = sd_format.format.height;
> > +	} else {
> > +		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > +			 soc_camera_grp_id(icd), video, s_mbus_fmt, mf);
> > +	}
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -264,9 +282,26 @@ static int client_s_fmt(struct soc_camera_device *icd,
> >  		tmp_h = min(2 * tmp_h, max_height);
> >  		mf->width = tmp_w;
> >  		mf->height = tmp_h;
> > -		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > +		// subdev_has_op -> set_fmt vs s_mbus_fmt
> > +		if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> > +			struct v4l2_subdev_format sd_format;
> > +			struct media_pad *remote_pad;
> > +
> > +			remote_pad= media_entity_remote_pad(
> > +				&icd->vdev->entity.pads[0]);
> > +			sd_format.pad = remote_pad->index;
> > +			sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > +			sd_format.format= *mf;
> > +
> > +			ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > +					soc_camera_grp_id(icd),
> > +					pad, set_fmt, NULL,
> > +					&sd_format);
> > +		} else {
> > +			ret = v4l2_device_call_until_err(sd->v4l2_dev,
> >  					soc_camera_grp_id(icd), video,
> >  					s_mbus_fmt, mf);
> > +		}
> >  		dev_geo(dev, "Camera scaled to %ux%u\n",
> >  			mf->width, mf->height);
> >  		if (ret < 0) {
> > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> > index 2f6261f..f0c5238 100644
> > --- a/include/media/soc_camera.h
> > +++ b/include/media/soc_camera.h
> > @@ -42,6 +42,7 @@ struct soc_camera_device {
> >  	unsigned char devnum;		/* Device number per host */
> >  	struct soc_camera_sense *sense;	/* See comment in struct definition */
> >  	struct video_device *vdev;
> > +	struct media_pad pad;
> >  	struct v4l2_ctrl_handler ctrl_handler;
> >  	const struct soc_camera_format_xlate *current_fmt;
> >  	struct soc_camera_format_xlate *user_formats;
> > -- 
> > 1.7.10.4
> > 
> > --
> > 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 6/8] WmT: adv7604 driver compatibility

[Laurent Pinchart]

Hi Guennadi,

On Sunday 01 February 2015 12:26:11 Guennadi Liakhovetski wrote:
> On a second thought:
> 
> On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:
> > Hi Wills,
> > 
> > Thanks for the patch. First and foremost, the title of the patch is wrong.
> > This patch does more than just adding some "adv7604 compatibility." It's
> > adding pad-level API to soc-camera.
> > 
> > This is just a rough review. I'm not an expert in media-controller /
> > pad-level API, I hope someone with a better knowledge of those areas will
> > help me reviewing this.
> > 
> > Another general comment: it has been discussed since a long time, whether
> > a wrapper wouldn't be desired to enable a seamless use of both subdev
> > drivers using and not using the pad-level API. Maybe it's the right time
> > now?..
> 
> This would be a considerable change and would most probably take a rather
> long time, given how busy everyone is.

If I understood correctly Hans Verkuil told me over the weekend that he wanted 
to address this problem in the near future. Hans, could you detail your plans 
?

> I personally would be fine with a (yet another) intermittent solution,
> whereby we create a soc_camera_subdev.c file, in which we collect all those
> function to call either a video or a pad subdev operation, depending on
> whether the latter is available. E.g.
> 
> int soc_camera_sd_enum_mbus_fmt(sd, code)
> {
> 	int ret;
> 
> 	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> 		ret = v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, code);
> 	} else {
> 		u32 pixcode;
> 
> 		ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, code->index, 
&pixcode);
> 		if (!ret)
> 			code->code= pixcode;
> 	}
> 
> 	return ret;
> }
> 
> Similarly for other ops.
> 
> Thanks
> Guennadi
> 
> > On Thu, 29 Jan 2015, William Towle wrote:
> > > Add 'struct media_pad pad' member and suitable glue code, so that
> > > soc_camera/rcar_vin can become agnostic to whether an old or new-
> > > style driver (wrt pad API use) can sit underneath
> > > 
> > > This version has been reworked to include appropriate constant and
> > > datatype names for kernel v3.18
> > > ---
> > > 
> > >  drivers/media/platform/soc_camera/soc_camera.c     |  148
> > >  +++++++++++++++++++-
> > >  drivers/media/platform/soc_camera/soc_scale_crop.c |   43 +++++-
> > >  include/media/soc_camera.h                         |    1 +
> > >  3 files changed, 182 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/soc_camera/soc_camera.c
> > > b/drivers/media/platform/soc_camera/soc_camera.c index f4be2a1..efc20bf
> > > 100644
> > > --- a/drivers/media/platform/soc_camera/soc_camera.c
> > > +++ b/drivers/media/platform/soc_camera/soc_camera.c
> > > @@ -37,8 +37,11 @@
> > > 
> > >  #include <media/v4l2-ioctl.h>
> > >  #include <media/v4l2-dev.h>
> > >  #include <media/v4l2-of.h>
> > > 
> > > +#if 0
> > > 
> > >  #include <media/videobuf-core.h>
> > >  #include <media/videobuf2-core.h>
> > > 
> > > +#endif
> > 
> > No. These headers are needed even if the code can be compiled without
> > them.
> > 
> > > +#include <media/v4l2-mediabus.h>
> > 
> > Well, maybe. This header is included indirectly via soc_mediabus.h, but
> > yes, as I just said above, headers, whose defines, structs etc. are used,
> > should be encluded directly. Further, you'll need more headers, e.g.
> > media-entity.h, maybe some more.
> > 
> > >  /* Default to VGA resolution */
> > >  #define DEFAULT_WIDTH	640
> > > 
> > > @@ -453,6 +456,98 @@ static int soc_camera_expbuf(struct file *file,
> > > void *priv,> > 
> > >  		return vb2_expbuf(&icd->vb2_vidq, p);
> > >  
> > >  }
> > > 
> > > +static int soc_camera_init_user_formats_pad(struct soc_camera_device
> > > *icd, int src_pad_idx) +{
> > > +	struct v4l2_subdev *sd= soc_camera_to_subdev(icd);
> > > +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> > > +	struct v4l2_subdev_mbus_code_enum code;
> > > +	int fmts= 0, raw_fmts, i, ret;
> > 
> > Please, run this patch through checkpatch.pl. It will tell you to add a
> > Signed-off-by line, (hopefully) to add spaces before "=" in multiple
> > places, to place braces correctly, to not use C++-style comments etc. Only
> > feel free to ignore 80-character warnings.
> > 
> > > +
> > > +	code.pad= src_pad_idx;
> > > +	code.index= 0;
> > > +
> > > +	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > > +	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> > 
> > This function is called only once below and only after the above test has
> > already returned success. Looks like you don't need it here again and the
> > below "else" branch can be dropped completely?
> > 
> > > +		while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code))
> > > +			code.index++;
> > > +	} else {
> > > +		u32 pixcode;
> > > +
> > > +		while (!v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index,
> > > &pixcode)) +		{
> > > +			code.code= pixcode;
> > > +			code.index++;
> > > +		}
> > > +	}
> > > +	raw_fmts= code.index;
> > > +
> > > +	if (!ici->ops->get_formats) {
> > > +		/*
> > > +		 * Fallback mode - the host will have to serve all
> > > +		 * sensor-provided formats one-to-one to the user
> > > +		 */
> > > +		fmts = raw_fmts;
> > > +	}
> > > +	else {
> > > +		/*
> > > +		 * First pass - only count formats this host-sensor
> > > +		 * configuration can provide
> > > +		 */
> > > +		for (i = 0; i < raw_fmts; i++) {
> > > +			int ret = ici->ops->get_formats(icd, i, NULL);
> > > +			if (ret < 0)
> > > +				return ret;
> > > +			fmts += ret;
> > > +		}
> > > +	}
> > > +
> > > +	if (!fmts)
> > > +		return -ENXIO;
> > > +
> > > +	icd->user_formats =
> > > +		vmalloc(fmts * sizeof(struct soc_camera_format_xlate));
> > > +	if (!icd->user_formats)
> > > +		return -ENOMEM;
> > > +
> > > +	dev_dbg(icd->pdev, "Found %d supported formats.\n", fmts);
> > > +
> > > +	/* Second pass - actually fill data formats */
> > > +	fmts = 0;
> > > +	for (i = 0; i < raw_fmts; i++) {
> > > +		if (!ici->ops->get_formats) {
> > > +			code.index= i;
> > > +			// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > > +			if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> > 
> > Same test again?? Or am I missing something? If indeed these tests are
> > redundant, after you remove them this function will become very similar to
> > the original soc_camera_init_user_formats(), so, maybe some code reuse
> > will become possible.
> > 
> > > +				v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code);
> > > +			} else {
> > > +				u32 pixcode;
> > > +
> > > +				v4l2_subdev_call(sd, video, enum_mbus_fmt, code.index, 
&pixcode);
> > > +				code.code= pixcode;
> > > +			}
> > > +			icd->user_formats[fmts].host_fmt =
> > > +				soc_mbus_get_fmtdesc(code.code);
> > > +			if (icd->user_formats[fmts].host_fmt)
> > > +				icd->user_formats[fmts++].code = code.code;
> > > +		} else {
> > > +			ret = ici->ops->get_formats(icd, i,
> > > +						    &icd->user_formats[fmts]);
> > > +			if (ret < 0)
> > > +				goto egfmt;
> > > +			fmts += ret;
> > > +		}
> > > +	}
> > > +
> > > +	icd->num_user_formats = fmts;
> > > +	icd->current_fmt = &icd->user_formats[0];
> > > +
> > > +	return 0;
> > > +
> > > +egfmt:
> > > +	vfree(icd->user_formats);
> > > +	return ret;
> > > +}
> > > +
> > > 
> > >  /* Always entered with .host_lock held */
> > >  static int soc_camera_init_user_formats(struct soc_camera_device *icd)
> > >  {
> > > 
> > > @@ -1289,6 +1384,7 @@ static int soc_camera_probe_finish(struct
> > > soc_camera_device *icd)> > 
> > >  {
> > >  
> > >  	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > >  	struct v4l2_mbus_framefmt mf;
> > > 
> > > +	int src_pad_idx= -1;
> > > 
> > >  	int ret;
> > >  	
> > >  	sd->grp_id = soc_camera_grp_id(icd);
> > > 
> > > @@ -1307,7 +1403,30 @@ static int soc_camera_probe_finish(struct
> > > soc_camera_device *icd)> > 
> > >  	}
> > >  	
> > >  	/* At this point client .probe() should have run already */
> > > 
> > > -	ret = soc_camera_init_user_formats(icd);
> > > +	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> > > +	if (!v4l2_subdev_has_op(sd, pad, enum_mbus_code))
> > 
> > This is the test, that I meant above.
> > 
> > > +		ret = soc_camera_init_user_formats(icd);
> > > +	else {
> > > +		ret = media_entity_init(&icd->vdev->entity, 1,
> > > +					&icd->pad, 0);
> > 
> > Ok, maybe this hard-coded 1 pad with no extras is justified here, but
> > let's here what others say.
> > 
> > > +		if (!ret) {
> > > +			for (src_pad_idx= 0; src_pad_idx < sd->entity.num_pads;
> > > src_pad_idx++)
> > > +				if (sd->entity.pads[src_pad_idx].flags == 
MEDIA_PAD_FL_SOURCE)
> > > +					break;
> > > +
> > > +			if (src_pad_idx < sd->entity.num_pads) {
> > > +				ret = media_entity_create_link(
> > > +					&icd->vdev->entity, 0,
> > > +					&sd->entity, src_pad_idx,
> > > +					MEDIA_LNK_FL_IMMUTABLE |
> > > +					MEDIA_LNK_FL_ENABLED);
> > 
> > Let's try to preserve the style. I normally try to avoid splitting the
> > line after "f(" and adding at least the first function parameter above
> > will not make that line longer, than the ones above. So, let's do that.
> > 
> > > +			}
> > > +		}
> > > +
> > > +		if (!ret)
> > > +			ret = soc_camera_init_user_formats_pad(icd,
> > > +							src_pad_idx);
> > 
> > Probably no need to break the line here either.
> > 
> > > +	}
> > > 
> > >  	if (ret < 0)
> > >  	
> > >  		goto eusrfmt;
> > > 
> > > @@ -1318,11 +1437,28 @@ static int soc_camera_probe_finish(struct
> > > soc_camera_device *icd)> > 
> > >  		goto evidstart;
> > >  	
> > >  	/* Try to improve our guess of a reasonable window format */
> > > 
> > > -	if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> > > -		icd->user_width		= mf.width;
> > > -		icd->user_height	= mf.height;
> > > -		icd->colorspace		= mf.colorspace;
> > > -		icd->field		= mf.field;
> > > +	// subdev_has_op -> get_fmt vs g_mbus_fmt
> > > +	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)
> > > +		&& v4l2_subdev_has_op(sd, pad, get_fmt)
> > > +		&& src_pad_idx != -1) {
> > 
> > The rest of the file puts operations after the first argument, not before
> > the second one when breaking the line. Let's do that here too.
> > 
> > Thanks
> > Guennadi
> > 
> > > +		struct v4l2_subdev_format sd_format;
> > > +
> > > +		sd_format.pad= src_pad_idx;
> > > +		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > > +
> > > +		if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_format)) {
> > > +			icd->user_width		= sd_format.format.width;
> > > +			icd->user_height	= sd_format.format.height;
> > > +			icd->colorspace		= sd_format.format.colorspace;
> > > +			icd->field		= sd_format.format.field;
> > > +		}
> > > +	} else {
> > > +		if (!v4l2_subdev_call(sd, video, g_mbus_fmt, &mf)) {
> > > +			icd->user_width		= mf.width;
> > > +			icd->user_height	= mf.height;
> > > +			icd->colorspace		= mf.colorspace;
> > > +			icd->field		= mf.field;
> > > +		}
> > > 
> > >  	}
> > >  	soc_camera_remove_device(icd);
> > > 
> > > diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c
> > > b/drivers/media/platform/soc_camera/soc_scale_crop.c index
> > > 8e74fb7..8a1ca05 100644
> > > --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
> > > +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
> > > @@ -224,9 +224,27 @@ static int client_s_fmt(struct soc_camera_device
> > > *icd,
> > > 
> > >  	bool host_1to1;
> > >  	int ret;
> > > 
> > > -	ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > -					 soc_camera_grp_id(icd), video,
> > > -					 s_mbus_fmt, mf);
> > > +	// subdev_has_op -> set_fmt vs s_mbus_fmt
> > > +	if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> > > +		struct v4l2_subdev_format sd_format;
> > > +		struct media_pad *remote_pad;
> > > +
> > > +		remote_pad= media_entity_remote_pad(
> > > +			&icd->vdev->entity.pads[0]);
> > > +		sd_format.pad = remote_pad->index;
> > > +		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > > +		sd_format.format= *mf;
> > > +
> > > +		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > +			soc_camera_grp_id(icd), pad, set_fmt, NULL,
> > > +			&sd_format);
> > > +
> > > +		mf->width = sd_format.format.width;
> > > +		mf->height = sd_format.format.height;
> > > +	} else {
> > > +		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > +			 soc_camera_grp_id(icd), video, s_mbus_fmt, mf);
> > > +	}
> > > 
> > >  	if (ret < 0)
> > >  	
> > >  		return ret;
> > > 
> > > @@ -264,9 +282,26 @@ static int client_s_fmt(struct soc_camera_device
> > > *icd,
> > > 
> > >  		tmp_h = min(2 * tmp_h, max_height);
> > >  		mf->width = tmp_w;
> > >  		mf->height = tmp_h;
> > > 
> > > -		ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > +		// subdev_has_op -> set_fmt vs s_mbus_fmt
> > > +		if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> > > +			struct v4l2_subdev_format sd_format;
> > > +			struct media_pad *remote_pad;
> > > +
> > > +			remote_pad= media_entity_remote_pad(
> > > +				&icd->vdev->entity.pads[0]);
> > > +			sd_format.pad = remote_pad->index;
> > > +			sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> > > +			sd_format.format= *mf;
> > > +
> > > +			ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > +					soc_camera_grp_id(icd),
> > > +					pad, set_fmt, NULL,
> > > +					&sd_format);
> > > +		} else {
> > > +			ret = v4l2_device_call_until_err(sd->v4l2_dev,
> > > 
> > >  					soc_camera_grp_id(icd), video,
> > >  					s_mbus_fmt, mf);
> > > 
> > > +		}
> > > 
> > >  		dev_geo(dev, "Camera scaled to %ux%u\n",
> > >  		
> > >  			mf->width, mf->height);
> > >  		
> > >  		if (ret < 0) {
> > > 
> > > diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> > > index 2f6261f..f0c5238 100644
> > > --- a/include/media/soc_camera.h
> > > +++ b/include/media/soc_camera.h
> > > @@ -42,6 +42,7 @@ struct soc_camera_device {
> > > 
> > >  	unsigned char devnum;		/* Device number per host */
> > >  	struct soc_camera_sense *sense;	/* See comment in struct definition 
*/
> > >  	struct video_device *vdev;
> > > 
> > > +	struct media_pad pad;
> > > 
> > >  	struct v4l2_ctrl_handler ctrl_handler;
> > >  	const struct soc_camera_format_xlate *current_fmt;
> > >  	struct soc_camera_format_xlate *user_formats;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/8] WmT: adv7604 driver compatibility

[Hans Verkuil]

On 02/02/2015 11:01 AM, Laurent Pinchart wrote:
> Hi Guennadi,
> 
> On Sunday 01 February 2015 12:26:11 Guennadi Liakhovetski wrote:
>> On a second thought:
>>
>> On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:
>>> Hi Wills,
>>>
>>> Thanks for the patch. First and foremost, the title of the patch is wrong.
>>> This patch does more than just adding some "adv7604 compatibility." It's
>>> adding pad-level API to soc-camera.
>>>
>>> This is just a rough review. I'm not an expert in media-controller /
>>> pad-level API, I hope someone with a better knowledge of those areas will
>>> help me reviewing this.
>>>
>>> Another general comment: it has been discussed since a long time, whether
>>> a wrapper wouldn't be desired to enable a seamless use of both subdev
>>> drivers using and not using the pad-level API. Maybe it's the right time
>>> now?..
>>
>> This would be a considerable change and would most probably take a rather
>> long time, given how busy everyone is.
> 
> If I understood correctly Hans Verkuil told me over the weekend that he wanted 
> to address this problem in the near future. Hans, could you detail your plans 
> ?

That's correct. This patch series makes all the necessary changes.

https://www.mail-archive.com/linux-media@vger.kernel.org/msg83415.html

Patches 1-4 have been merged already, but I need to do more testing for the
remainder. The Renesas SH7724 board is ideal for that, but unfortunately I
can't get it to work with the current kernel.

Regards,

	Hans

Re: [PATCH 6/8] WmT: adv7604 driver compatibility

[Laurent Pinchart]

Hi Hans,

On Monday 02 February 2015 11:09:27 Hans Verkuil wrote:
> On 02/02/2015 11:01 AM, Laurent Pinchart wrote:
> > On Sunday 01 February 2015 12:26:11 Guennadi Liakhovetski wrote:
> >> On a second thought:
> >> 
> >> On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:
> >>> Hi Wills,
> >>> 
> >>> Thanks for the patch. First and foremost, the title of the patch is
> >>> wrong. This patch does more than just adding some "adv7604
> >>> compatibility." It's adding pad-level API to soc-camera.
> >>> 
> >>> This is just a rough review. I'm not an expert in media-controller /
> >>> pad-level API, I hope someone with a better knowledge of those areas
> >>> will help me reviewing this.
> >>> 
> >>> Another general comment: it has been discussed since a long time,
> >>> whether a wrapper wouldn't be desired to enable a seamless use of both
> >>> subdev drivers using and not using the pad-level API. Maybe it's the
> >>> right time now?..
> >> 
> >> This would be a considerable change and would most probably take a rather
> >> long time, given how busy everyone is.
> > 
> > If I understood correctly Hans Verkuil told me over the weekend that he
> > wanted to address this problem in the near future. Hans, could you detail
> > your plans ?
> 
> That's correct. This patch series makes all the necessary changes.
> 
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg83415.html
> 
> Patches 1-4 have been merged already, but I need to do more testing for the
> remainder. The Renesas SH7724 board is ideal for that, but unfortunately I
> can't get it to work with the current kernel.

I can't help you much with that, but I could test changes using the rcar-vin 
driver with the adv7180 if needed (does the adv7180 generate an image if no 
analog source is connected ?). That will need to wait for two weeks though as 
I don't have access to the hardware right now.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/8] WmT: adv7604 driver compatibility

[Hans Verkuil]

On 02/03/15 16:22, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday 02 February 2015 11:09:27 Hans Verkuil wrote:
>> On 02/02/2015 11:01 AM, Laurent Pinchart wrote:
>>> On Sunday 01 February 2015 12:26:11 Guennadi Liakhovetski wrote:
>>>> On a second thought:
>>>>
>>>> On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:
>>>>> Hi Wills,
>>>>>
>>>>> Thanks for the patch. First and foremost, the title of the patch is
>>>>> wrong. This patch does more than just adding some "adv7604
>>>>> compatibility." It's adding pad-level API to soc-camera.
>>>>>
>>>>> This is just a rough review. I'm not an expert in media-controller /
>>>>> pad-level API, I hope someone with a better knowledge of those areas
>>>>> will help me reviewing this.
>>>>>
>>>>> Another general comment: it has been discussed since a long time,
>>>>> whether a wrapper wouldn't be desired to enable a seamless use of both
>>>>> subdev drivers using and not using the pad-level API. Maybe it's the
>>>>> right time now?..
>>>>
>>>> This would be a considerable change and would most probably take a rather
>>>> long time, given how busy everyone is.
>>>
>>> If I understood correctly Hans Verkuil told me over the weekend that he
>>> wanted to address this problem in the near future. Hans, could you detail
>>> your plans ?
>>
>> That's correct. This patch series makes all the necessary changes.
>>
>> https://www.mail-archive.com/linux-media@vger.kernel.org/msg83415.html
>>
>> Patches 1-4 have been merged already, but I need to do more testing for the
>> remainder. The Renesas SH7724 board is ideal for that, but unfortunately I
>> can't get it to work with the current kernel.
> 
> I can't help you much with that, but I could test changes using the rcar-vin 
> driver with the adv7180 if needed (does the adv7180 generate an image if no 
> analog source is connected ?). 

I expect so, most SDTV receivers do that.

> That will need to wait for two weeks though as 
> I don't have access to the hardware right now.
> 

It's certainly appreciated (I'll see if I can rebase it), but I am not
worried about that driver. It's soc_camera that is affected the most.

Regards,

	Hans

Re: [PATCH 6/8] WmT: adv7604 driver compatibility

[Lars-Peter Clausen]

On 02/03/2015 04:24 PM, Hans Verkuil wrote:
> On 02/03/15 16:22, Laurent Pinchart wrote:
>> Hi Hans,
>>
>> On Monday 02 February 2015 11:09:27 Hans Verkuil wrote:
>>> On 02/02/2015 11:01 AM, Laurent Pinchart wrote:
>>>> On Sunday 01 February 2015 12:26:11 Guennadi Liakhovetski wrote:
>>>>> On a second thought:
>>>>>
>>>>> On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:
>>>>>> Hi Wills,
>>>>>>
>>>>>> Thanks for the patch. First and foremost, the title of the patch is
>>>>>> wrong. This patch does more than just adding some "adv7604
>>>>>> compatibility." It's adding pad-level API to soc-camera.
>>>>>>
>>>>>> This is just a rough review. I'm not an expert in media-controller /
>>>>>> pad-level API, I hope someone with a better knowledge of those areas
>>>>>> will help me reviewing this.
>>>>>>
>>>>>> Another general comment: it has been discussed since a long time,
>>>>>> whether a wrapper wouldn't be desired to enable a seamless use of both
>>>>>> subdev drivers using and not using the pad-level API. Maybe it's the
>>>>>> right time now?..
>>>>>
>>>>> This would be a considerable change and would most probably take a rather
>>>>> long time, given how busy everyone is.
>>>>
>>>> If I understood correctly Hans Verkuil told me over the weekend that he
>>>> wanted to address this problem in the near future. Hans, could you detail
>>>> your plans ?
>>>
>>> That's correct. This patch series makes all the necessary changes.
>>>
>>> https://www.mail-archive.com/linux-media@vger.kernel.org/msg83415.html
>>>
>>> Patches 1-4 have been merged already, but I need to do more testing for the
>>> remainder. The Renesas SH7724 board is ideal for that, but unfortunately I
>>> can't get it to work with the current kernel.
>>
>> I can't help you much with that, but I could test changes using the rcar-vin
>> driver with the adv7180 if needed (does the adv7180 generate an image if no
>> analog source is connected ?).
>
> I expect so, most SDTV receivers do that.

It has a freerun mode, in which it can output various kinds of patterns, see 
https://patchwork.linuxtv.org/patch/27894/

- Lars

Re: [PATCH 6/8] WmT: adv7604 driver compatibility

[Laurent Pinchart]

On Tuesday 03 February 2015 16:29:57 Lars-Peter Clausen wrote:
> On 02/03/2015 04:24 PM, Hans Verkuil wrote:
> > On 02/03/15 16:22, Laurent Pinchart wrote:

[snip]

> >> I can't help you much with that, but I could test changes using the
> >> rcar-vin driver with the adv7180 if needed (does the adv7180 generate an
> >> image if no analog source is connected ?).
> > 
> > I expect so, most SDTV receivers do that.
> 
> It has a freerun mode, in which it can output various kinds of patterns, see
> https://patchwork.linuxtv.org/patch/27894/

Thank you.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/8] WmT: adv7604 driver compatibility

[Laurent Pinchart]

Hi Hans,

On Tuesday 03 February 2015 16:24:19 Hans Verkuil wrote:
> On 02/03/15 16:22, Laurent Pinchart wrote:
> > On Monday 02 February 2015 11:09:27 Hans Verkuil wrote:
> >> On 02/02/2015 11:01 AM, Laurent Pinchart wrote:
> >>> On Sunday 01 February 2015 12:26:11 Guennadi Liakhovetski wrote:
> >>>> On a second thought:
> >>>> 
> >>>> On Sun, 1 Feb 2015, Guennadi Liakhovetski wrote:
> >>>>> Hi Wills,
> >>>>> 
> >>>>> Thanks for the patch. First and foremost, the title of the patch is
> >>>>> wrong. This patch does more than just adding some "adv7604
> >>>>> compatibility." It's adding pad-level API to soc-camera.
> >>>>> 
> >>>>> This is just a rough review. I'm not an expert in media-controller /
> >>>>> pad-level API, I hope someone with a better knowledge of those areas
> >>>>> will help me reviewing this.
> >>>>> 
> >>>>> Another general comment: it has been discussed since a long time,
> >>>>> whether a wrapper wouldn't be desired to enable a seamless use of both
> >>>>> subdev drivers using and not using the pad-level API. Maybe it's the
> >>>>> right time now?..
> >>>> 
> >>>> This would be a considerable change and would most probably take a
> >>>> rather long time, given how busy everyone is.
> >>> 
> >>> If I understood correctly Hans Verkuil told me over the weekend that he
> >>> wanted to address this problem in the near future. Hans, could you
> >>> detail your plans ?
> >> 
> >> That's correct. This patch series makes all the necessary changes.
> >> 
> >> https://www.mail-archive.com/linux-media@vger.kernel.org/msg83415.html
> >> 
> >> Patches 1-4 have been merged already, but I need to do more testing for
> >> the remainder. The Renesas SH7724 board is ideal for that, but
> >> unfortunately I can't get it to work with the current kernel.
> > 
> > I can't help you much with that, but I could test changes using the
> > rcar-vin driver with the adv7180 if needed (does the adv7180 generate an
> > image if no analog source is connected ?).
> 
> I expect so, most SDTV receivers do that.
> 
> > That will need to wait for two weeks though as
> > I don't have access to the hardware right now.
> 
> It's certainly appreciated (I'll see if I can rebase it), but I am not
> worried about that driver. It's soc_camera that is affected the most.

rcar-vin is an soc-camera host driver, isn't it ?

-- 
Regards,

Laurent Pinchart

[PATCH 7/8] WmT: rcar_vin new ADV7612 support

[William Towle]

Add 'struct media_pad pad' member and suitable glue code, so that
soc_camera/rcar_vin can become agnostic to whether an old or new-
style driver (wrt pad API use) can sit underneath

This version has been reworked to include appropriate constant and
datatype names for kernel v3.18

---

** this version for kernel 3.18.x+ (v4l2 constant names) **
** now including: **
| WmT: assume max resolution at init
|
| Make rcar_vin agnostic to the driver beneath having a smaller
| resolution as its default size. Previously, the logic for calculating
| cropping region size -which depends on going from larger to smaller
| values- would have been confused by this.
---
 drivers/media/platform/soc_camera/rcar_vin.c |  112 +++++++++++++++++++++++---
 1 file changed, 101 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index e4f60d3..046fcc1 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -932,9 +932,27 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
 	u32 code;
 	const struct soc_mbus_pixelfmt *fmt;
 
-	ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, idx, &code);
-	if (ret < 0)
-		return 0;
+	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
+	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
+		struct v4l2_subdev_mbus_code_enum c;
+
+		c.index = idx;
+
+		ret = v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &c);
+		if (ret < 0)
+			return 0;
+
+#if 1	/*  ideal  */
+		code = c.code;
+#else	/*  Ian HACK - required with full(er) formats table  */
+		code = MEDIA_BUS_FMT_RGB888_1X24; //HACK
+#endif
+	}
+	else {
+		ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, idx, &code);
+		if (ret < 0)
+			return 0;
+	}
 
 	fmt = soc_mbus_get_fmtdesc(code);
 	if (!fmt) {
@@ -948,11 +966,28 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
 
 	if (!icd->host_priv) {
 		struct v4l2_mbus_framefmt mf;
+		struct v4l2_subdev_format sd_format;
 		struct v4l2_rect rect;
 		struct device *dev = icd->parent;
 		int shift;
 
-		ret = v4l2_subdev_call(sd, video, g_mbus_fmt, &mf);
+		// subdev_has_op -> get_fmt vs g_mbus_fmt
+		if (v4l2_subdev_has_op(sd, pad, get_fmt)) {
+			struct media_pad *remote_pad;
+
+			remote_pad= media_entity_remote_pad(&icd->vdev->entity.pads[0]);
+			sd_format.pad= remote_pad->index;
+			sd_format.which=V4L2_SUBDEV_FORMAT_ACTIVE;
+
+			ret = v4l2_subdev_call(sd, pad, get_fmt, NULL,
+						&sd_format);
+			mf= sd_format.format;
+			mf.width= VIN_MAX_WIDTH;
+			mf.height= VIN_MAX_HEIGHT;
+		}
+		else {
+			ret = v4l2_subdev_call(sd, video, g_mbus_fmt, &mf);
+		}
 		if (ret < 0)
 			return ret;
 
@@ -979,10 +1014,18 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
 
 			mf.width = 1280 >> shift;
 			mf.height = 960 >> shift;
-			ret = v4l2_device_call_until_err(sd->v4l2_dev,
-							 soc_camera_grp_id(icd),
-							 video, s_mbus_fmt,
-							 &mf);
+			// subdev_has_op -> set_fmt vs s_mbus_fmt
+			if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
+				ret = v4l2_device_call_until_err(sd->v4l2_dev,
+						 soc_camera_grp_id(icd),
+						 pad, set_fmt, NULL,
+						 &sd_format);
+			} else {
+				ret = v4l2_device_call_until_err(sd->v4l2_dev,
+						 soc_camera_grp_id(icd),
+						 video, s_mbus_fmt,
+						 &mf);
+			}
 			if (ret < 0)
 				return ret;
 		}
@@ -1099,7 +1142,22 @@ static int rcar_vin_set_crop(struct soc_camera_device *icd,
 	/* On success cam_crop contains current camera crop */
 
 	/* Retrieve camera output window */
-	ret = v4l2_subdev_call(sd, video, g_mbus_fmt, &mf);
+	// subdev_has_op -> get_fmt vs g_mbus_fmt
+	if (v4l2_subdev_has_op(sd, pad, get_fmt))
+	{
+		struct v4l2_subdev_format sd_format;
+		struct media_pad *remote_pad;
+
+		remote_pad= media_entity_remote_pad(&icd->vdev->entity.pads[0]);
+		sd_format.pad= remote_pad->index;
+		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
+		ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_format);
+
+		mf.width= sd_format.format.width;
+		mf.height= sd_format.format.height;
+	} else {
+		ret = v4l2_subdev_call(sd, video, g_mbus_fmt, &mf);
+	}
 	if (ret < 0)
 		return ret;
 
@@ -1314,8 +1372,22 @@ static int rcar_vin_try_fmt(struct soc_camera_device *icd,
 	mf.code = xlate->code;
 	mf.colorspace = pix->colorspace;
 
-	ret = v4l2_device_call_until_err(sd->v4l2_dev, soc_camera_grp_id(icd),
+	// subdev_has_op -> get_fmt vs try_mbus_fmt
+	if (v4l2_subdev_has_op(sd, pad, get_fmt)) {
+		struct v4l2_subdev_format sd_format;
+		struct media_pad *remote_pad;
+
+		remote_pad= media_entity_remote_pad(
+					&icd->vdev->entity.pads[0]);
+		sd_format.pad= remote_pad->index;
+		sd_format.format= mf;
+		ret= v4l2_device_call_until_err(sd->v4l2_dev, soc_camera_grp_id(icd),
+                                        pad, get_fmt, NULL, &sd_format);
+		mf= sd_format.format;
+	} else {
+		ret = v4l2_device_call_until_err(sd->v4l2_dev, soc_camera_grp_id(icd),
 					 video, try_mbus_fmt, &mf);
+	}
 	if (ret < 0)
 		return ret;
 
@@ -1335,10 +1407,28 @@ static int rcar_vin_try_fmt(struct soc_camera_device *icd,
 			 */
 			mf.width = VIN_MAX_WIDTH;
 			mf.height = VIN_MAX_HEIGHT;
-			ret = v4l2_device_call_until_err(sd->v4l2_dev,
+			// subdev_has_op -> get_fmt vs try_mbus_fmt
+			if (v4l2_subdev_has_op(sd, pad, get_fmt)) {
+				struct v4l2_subdev_format sd_format;
+				struct media_pad *remote_pad;
+
+				remote_pad= media_entity_remote_pad(
+					&icd->vdev->entity.pads[0]);
+				sd_format.pad = remote_pad->index;
+				sd_format.which= V4L2_SUBDEV_FORMAT_TRY;
+				sd_format.format= mf;
+				ret = v4l2_device_call_until_err(sd->v4l2_dev,
+							 soc_camera_grp_id(icd),
+							 pad, get_fmt,
+							 NULL, &sd_format);
+				mf= sd_format.format;
+			} else {
+				ret = v4l2_device_call_until_err(sd->v4l2_dev,
 							 soc_camera_grp_id(icd),
 							 video, try_mbus_fmt,
 							 &mf);
+			}
+
 			if (ret < 0) {
 				dev_err(icd->parent,
 					"client try_fmt() = %d\n", ret);
-- 
1.7.10.4

Re: [PATCH 7/8] WmT: rcar_vin new ADV7612 support

[Guennadi Liakhovetski]

Hi Wills,

Same proposed wrappers could be used here.

Thanks
Guennadi

On Thu, 29 Jan 2015, William Towle wrote:

> Add 'struct media_pad pad' member and suitable glue code, so that
> soc_camera/rcar_vin can become agnostic to whether an old or new-
> style driver (wrt pad API use) can sit underneath
> 
> This version has been reworked to include appropriate constant and
> datatype names for kernel v3.18
> 
> ---
> 
> ** this version for kernel 3.18.x+ (v4l2 constant names) **
> ** now including: **
> | WmT: assume max resolution at init
> |
> | Make rcar_vin agnostic to the driver beneath having a smaller
> | resolution as its default size. Previously, the logic for calculating
> | cropping region size -which depends on going from larger to smaller
> | values- would have been confused by this.
> ---
>  drivers/media/platform/soc_camera/rcar_vin.c |  112 +++++++++++++++++++++++---
>  1 file changed, 101 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> index e4f60d3..046fcc1 100644
> --- a/drivers/media/platform/soc_camera/rcar_vin.c
> +++ b/drivers/media/platform/soc_camera/rcar_vin.c
> @@ -932,9 +932,27 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
>  	u32 code;
>  	const struct soc_mbus_pixelfmt *fmt;
>  
> -	ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, idx, &code);
> -	if (ret < 0)
> -		return 0;
> +	// subdev_has_op -> enum_mbus_code vs enum_mbus_fmt
> +	if (v4l2_subdev_has_op(sd, pad, enum_mbus_code)) {
> +		struct v4l2_subdev_mbus_code_enum c;
> +
> +		c.index = idx;
> +
> +		ret = v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &c);
> +		if (ret < 0)
> +			return 0;
> +
> +#if 1	/*  ideal  */
> +		code = c.code;
> +#else	/*  Ian HACK - required with full(er) formats table  */
> +		code = MEDIA_BUS_FMT_RGB888_1X24; //HACK
> +#endif
> +	}
> +	else {
> +		ret = v4l2_subdev_call(sd, video, enum_mbus_fmt, idx, &code);
> +		if (ret < 0)
> +			return 0;
> +	}
>  
>  	fmt = soc_mbus_get_fmtdesc(code);
>  	if (!fmt) {
> @@ -948,11 +966,28 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
>  
>  	if (!icd->host_priv) {
>  		struct v4l2_mbus_framefmt mf;
> +		struct v4l2_subdev_format sd_format;
>  		struct v4l2_rect rect;
>  		struct device *dev = icd->parent;
>  		int shift;
>  
> -		ret = v4l2_subdev_call(sd, video, g_mbus_fmt, &mf);
> +		// subdev_has_op -> get_fmt vs g_mbus_fmt
> +		if (v4l2_subdev_has_op(sd, pad, get_fmt)) {
> +			struct media_pad *remote_pad;
> +
> +			remote_pad= media_entity_remote_pad(&icd->vdev->entity.pads[0]);
> +			sd_format.pad= remote_pad->index;
> +			sd_format.which=V4L2_SUBDEV_FORMAT_ACTIVE;
> +
> +			ret = v4l2_subdev_call(sd, pad, get_fmt, NULL,
> +						&sd_format);
> +			mf= sd_format.format;
> +			mf.width= VIN_MAX_WIDTH;
> +			mf.height= VIN_MAX_HEIGHT;
> +		}
> +		else {
> +			ret = v4l2_subdev_call(sd, video, g_mbus_fmt, &mf);
> +		}
>  		if (ret < 0)
>  			return ret;
>  
> @@ -979,10 +1014,18 @@ static int rcar_vin_get_formats(struct soc_camera_device *icd, unsigned int idx,
>  
>  			mf.width = 1280 >> shift;
>  			mf.height = 960 >> shift;
> -			ret = v4l2_device_call_until_err(sd->v4l2_dev,
> -							 soc_camera_grp_id(icd),
> -							 video, s_mbus_fmt,
> -							 &mf);
> +			// subdev_has_op -> set_fmt vs s_mbus_fmt
> +			if (v4l2_subdev_has_op(sd, pad, set_fmt)) {
> +				ret = v4l2_device_call_until_err(sd->v4l2_dev,
> +						 soc_camera_grp_id(icd),
> +						 pad, set_fmt, NULL,
> +						 &sd_format);
> +			} else {
> +				ret = v4l2_device_call_until_err(sd->v4l2_dev,
> +						 soc_camera_grp_id(icd),
> +						 video, s_mbus_fmt,
> +						 &mf);
> +			}
>  			if (ret < 0)
>  				return ret;
>  		}
> @@ -1099,7 +1142,22 @@ static int rcar_vin_set_crop(struct soc_camera_device *icd,
>  	/* On success cam_crop contains current camera crop */
>  
>  	/* Retrieve camera output window */
> -	ret = v4l2_subdev_call(sd, video, g_mbus_fmt, &mf);
> +	// subdev_has_op -> get_fmt vs g_mbus_fmt
> +	if (v4l2_subdev_has_op(sd, pad, get_fmt))
> +	{
> +		struct v4l2_subdev_format sd_format;
> +		struct media_pad *remote_pad;
> +
> +		remote_pad= media_entity_remote_pad(&icd->vdev->entity.pads[0]);
> +		sd_format.pad= remote_pad->index;
> +		sd_format.which= V4L2_SUBDEV_FORMAT_ACTIVE;
> +		ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_format);
> +
> +		mf.width= sd_format.format.width;
> +		mf.height= sd_format.format.height;
> +	} else {
> +		ret = v4l2_subdev_call(sd, video, g_mbus_fmt, &mf);
> +	}
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1314,8 +1372,22 @@ static int rcar_vin_try_fmt(struct soc_camera_device *icd,
>  	mf.code = xlate->code;
>  	mf.colorspace = pix->colorspace;
>  
> -	ret = v4l2_device_call_until_err(sd->v4l2_dev, soc_camera_grp_id(icd),
> +	// subdev_has_op -> get_fmt vs try_mbus_fmt
> +	if (v4l2_subdev_has_op(sd, pad, get_fmt)) {
> +		struct v4l2_subdev_format sd_format;
> +		struct media_pad *remote_pad;
> +
> +		remote_pad= media_entity_remote_pad(
> +					&icd->vdev->entity.pads[0]);
> +		sd_format.pad= remote_pad->index;
> +		sd_format.format= mf;
> +		ret= v4l2_device_call_until_err(sd->v4l2_dev, soc_camera_grp_id(icd),
> +                                        pad, get_fmt, NULL, &sd_format);
> +		mf= sd_format.format;
> +	} else {
> +		ret = v4l2_device_call_until_err(sd->v4l2_dev, soc_camera_grp_id(icd),
>  					 video, try_mbus_fmt, &mf);
> +	}
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1335,10 +1407,28 @@ static int rcar_vin_try_fmt(struct soc_camera_device *icd,
>  			 */
>  			mf.width = VIN_MAX_WIDTH;
>  			mf.height = VIN_MAX_HEIGHT;
> -			ret = v4l2_device_call_until_err(sd->v4l2_dev,
> +			// subdev_has_op -> get_fmt vs try_mbus_fmt
> +			if (v4l2_subdev_has_op(sd, pad, get_fmt)) {
> +				struct v4l2_subdev_format sd_format;
> +				struct media_pad *remote_pad;
> +
> +				remote_pad= media_entity_remote_pad(
> +					&icd->vdev->entity.pads[0]);
> +				sd_format.pad = remote_pad->index;
> +				sd_format.which= V4L2_SUBDEV_FORMAT_TRY;
> +				sd_format.format= mf;
> +				ret = v4l2_device_call_until_err(sd->v4l2_dev,
> +							 soc_camera_grp_id(icd),
> +							 pad, get_fmt,
> +							 NULL, &sd_format);
> +				mf= sd_format.format;
> +			} else {
> +				ret = v4l2_device_call_until_err(sd->v4l2_dev,
>  							 soc_camera_grp_id(icd),
>  							 video, try_mbus_fmt,
>  							 &mf);
> +			}
> +
>  			if (ret < 0) {
>  				dev_err(icd->parent,
>  					"client try_fmt() = %d\n", ret);
> -- 
> 1.7.10.4
> 
> --
> 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
> 

[PATCH 8/8] WmT: dts/i vin0/adv7612 (HDMI)

[William Towle]

---
 arch/arm/boot/dts/r8a7790-lager.dts |   51 +++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
index be44493..c20b6cb 100644
--- a/arch/arm/boot/dts/r8a7790-lager.dts
+++ b/arch/arm/boot/dts/r8a7790-lager.dts
@@ -249,9 +249,9 @@
 		renesas,function = "usb2";
 	};
 
-	vin1_pins: vin {
-		renesas,groups = "vin1_data8", "vin1_clk";
-		renesas,function = "vin1";
+	vin0_pins: vin0 {
+		renesas,groups = "vin0_data24", "vin0_sync", "vin0_field", "vin0_clkenb", "vin0_clk";
+		renesas,function = "vin0";
 	};
 };
 
@@ -391,15 +391,15 @@
 	pinctrl-0 = <&iic2_pins>;
 	pinctrl-names = "default";
 
-	composite-in@20 {
-		compatible = "adi,adv7180";
-		reg = <0x20>;
-		remote = <&vin1>;
+	adv7612: adv7612@0x4c {
+		compatible = "adi,adv7612";
+		reg = <0x4c>;
+		remote = <&vin0>;
 
 		port {
-			adv7180: endpoint {
-				bus-width = <8>;
-				remote-endpoint = <&vin1ep0>;
+			adv7612_1: endpoint {
+				remote-endpoint = <&vin0ep0>;
+				default-input = <0>;
 			};
 		};
 	};
@@ -419,6 +419,19 @@
 		i2c-gpio,delay-us = <1>;	/* ~100 kHz */
 		#address-cells = <1>;
 		#size-cells = <0>;
+
+		adv7612: adv7612@0x4c {
+			compatible = "adi,adv7612";
+			reg = <0x4c>;
+			remote = <&vin0>;
+
+			port {
+				adv7612_1: endpoint {
+					remote-endpoint = <&vin0ep0>;
+					default-input = <0>;
+				};
+			};
+		};
 	};
 };
 #endif
@@ -457,9 +470,9 @@
 	pinctrl-names = "default";
 };
 
-/* composite video input */
-&vin1 {
-	pinctrl-0 = <&vin1_pins>;
+/* HDMI video input */
+&vin0 {
+	pinctrl-0 = <&vin0_pins>;
 	pinctrl-names = "default";
 
 	status = "ok";
@@ -468,9 +481,13 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		vin1ep0: endpoint {
-			remote-endpoint = <&adv7180>;
-			bus-width = <8>;
+		vin0ep0: endpoint {
+			remote-endpoint = <&adv7612_1>;
+			bus-width = <24>;
+			hsync-active = <0>;
+			vsync-active = <0>;
+			pclk-sample = <1>;
+			data-active = <1>;
 		};
 	};
-};
+};
\ No newline at end of file
-- 
1.7.10.4

Re: [PATCH 8/8] WmT: dts/i vin0/adv7612 (HDMI)

[Sergei Shtylyov]

Hello.

On 01/29/2015 07:19 PM, William Towle wrote:

    No signed off? Although, looking at the patch, I'm not very surprised...

> ---
>   arch/arm/boot/dts/r8a7790-lager.dts |   51 +++++++++++++++++++++++------------
>   1 file changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts
> index be44493..c20b6cb 100644
> --- a/arch/arm/boot/dts/r8a7790-lager.dts
> +++ b/arch/arm/boot/dts/r8a7790-lager.dts
> @@ -249,9 +249,9 @@
>   		renesas,function = "usb2";
>   	};
>
> -	vin1_pins: vin {
> -		renesas,groups = "vin1_data8", "vin1_clk";
> -		renesas,function = "vin1";

    I'm not sure why are you removing the VIN1 pins while adding VIN0 pins.

> +	vin0_pins: vin0 {
> +		renesas,groups = "vin0_data24", "vin0_sync", "vin0_field", "vin0_clkenb", "vin0_clk";
> +		renesas,function = "vin0";
>   	};
>   };
>
> @@ -391,15 +391,15 @@
>   	pinctrl-0 = <&iic2_pins>;
>   	pinctrl-names = "default";
>
> -	composite-in@20 {
> -		compatible = "adi,adv7180";
> -		reg = <0x20>;
> -		remote = <&vin1>;

     Why remove it?

> +	adv7612: adv7612@0x4c {

    We don't call the nodes with chip names. According to the ePAPR standard 
section 2.2.2, node names should reflect the general function of the device.
And drop "0x" prefix from <unit-address> prt of the name please.

> +		compatible = "adi,adv7612";
> +		reg = <0x4c>;
> +		remote = <&vin0>;
>
>   		port {
> -			adv7180: endpoint {
> -				bus-width = <8>;
> -				remote-endpoint = <&vin1ep0>;
> +			adv7612_1: endpoint {
> +				remote-endpoint = <&vin0ep0>;
> +				default-input = <0>;
>   			};
>   		};
>   	};
> @@ -419,6 +419,19 @@
>   		i2c-gpio,delay-us = <1>;	/* ~100 kHz */
>   		#address-cells = <1>;
>   		#size-cells = <0>;
> +
> +		adv7612: adv7612@0x4c {

    Two identical labels? Do we really have 2 ADV7162 chips on the board?
    And the same comments on the node name...

> +			compatible = "adi,adv7612";
> +			reg = <0x4c>;
> +			remote = <&vin0>;
> +
> +			port {
> +				adv7612_1: endpoint {

    Two identical labels?

> +					remote-endpoint = <&vin0ep0>;
> +					default-input = <0>;
> +				};
> +			};
> +		};
>   	};
>   };
>   #endif
> @@ -457,9 +470,9 @@
>   	pinctrl-names = "default";
>   };
>
> -/* composite video input */
> -&vin1 {
> -	pinctrl-0 = <&vin1_pins>;

    Why remove it?

> +/* HDMI video input */
> +&vin0 {
> +	pinctrl-0 = <&vin0_pins>;
>   	pinctrl-names = "default";
>
>   	status = "ok";
> @@ -468,9 +481,13 @@
>   		#address-cells = <1>;
>   		#size-cells = <0>;
>
> -		vin1ep0: endpoint {
> -			remote-endpoint = <&adv7180>;
> -			bus-width = <8>;
> +		vin0ep0: endpoint {
> +			remote-endpoint = <&adv7612_1>;
> +			bus-width = <24>;
> +			hsync-active = <0>;
> +			vsync-active = <0>;
> +			pclk-sample = <1>;
> +			data-active = <1>;
>   		};
>   	};
[...]

WBR, Sergei

Re: RFC: supporting adv7604.c under soc_camera/rcar_vin

[Guennadi Liakhovetski]

Hi Wills,

A general comment: please, don't prefix patch titles with "WmT," this 
isn't the way authors or submitters are credited for their work in the 
Linux kernel.

Thanks
Guennadi

On Thu, 29 Jan 2015, William Towle wrote:

>   The following constitutes parts of our rcar_vin development branch
> beyond the update to our hotfixes published earlier this month.
> Similarly, these patches are intended to the mainline 3.18 kernel.
> Further development is required, but we would like to highlight the
> following issues and discuss them before completing the work.
> 
> 1. Our internal review has noted that our use of v4l2_subdev_has_op()
> is not yet ideal (but but does suffice for the purposes of generating
> images as-is). These tests are intended to detect whether or not a
> camera whose driver is aware of the pad API is present or not, and
> ensure we interact with subdevices accordingly. We think we should be
> iterating around all camera(s), and testing each subdevice link in
> turn. Is this sound, or is there a better way?
> 
> 2. Our second problem regards the supported formats list in adv7604.c,
> which needs further attention. We believe that having entries that go
> on to be rejected by rcar_vin_get_formats() may trigger a failure to
> initialise cleanly. Workaround code is marked "Ian Hack"; we intend to
> remove this and the list entries that cause this issue.
> 
> 3. Our third problem concerns detecting the resolution of the stream.
> Our code works with the obsoleted driver (adv761x.c) in place, but with
> our modifications to adv7604.c we have seen a) recovery of a 640x480
> image which is cropped rather than scaled, and/or b) recovery of a
> 2048x2048 image with the stream content in the top left corner. We
> think we understand the former problem, but the latter seems to be
> caused by full initialisation of the 'struct v4l2_subdev_format
> sd_format' variable, and we only have a partial solution [included
> as patch 4/8] so far. Of particular concern here is that potential
> consequences of changes in this particular patch are not clear.
> 
> 
>   Any advice would be appreciated, particularly regarding the first and
> last point above.
> 
> Cheers,
>   Wills.
> 
>   Associated patches:
> 	[PATCH 1/8] Add ability to read default input port from DT
> 	[PATCH 2/8] adv7604.c: formats, default colourspace, and IRQs
> 	[PATCH 3/8] WmT: document "adi,adv7612"
> 	[PATCH 4/8] WmT: m-5mols_core style pad handling for adv7604
> 	[PATCH 5/8] media: rcar_vin: Add RGB888_1X24 input format support
> 	[PATCH 6/8] WmT: adv7604 driver compatibility
> 	[PATCH 7/8] WmT: rcar_vin new ADV7612 support
> 	[PATCH 8/8] WmT: dts/i vin0/adv7612 (HDMI)
> --
> 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: [Linux-kernel] RFC: supporting adv7604.c under soc_camera/rcar_vin

[William Towle]

Hi all,

   I would like to develop a point in my previous discussion based on
new findings:

On Thu, 29 Jan 2015, William Towle wrote:
> 3. Our third problem concerns detecting the resolution of the stream.
> Our code works with the obsoleted driver (adv761x.c) in place, but with
> our modifications to adv7604.c we have seen a) recovery of a 640x480
> image which is cropped rather than scaled, and/or b) recovery of a
> 2048x2048 image with the stream content in the top left corner.

   We have since ported this code from 3.17 to 3.19 (Hans' "subdev2"
branch) and removed the unnecessary backward compatibility sections.
Some of the the behaviour is somewhat different in the port, but
I'll discuss that separately. Here I intend to discuss a possible bug
in adv7604.c.

   In our 3.17-based submission, we had shim code in soc_camera/rcar_vin
in order to emulate the old driver (originally serving to "test drive"
the new driver in an older kernel). For a test case with gstreamer
capturing a single frame it was sufficient at the time a) to override
the driver's default resolution with something larger when first probed
[emulating adv761x.c defaulting to the maximum supported resolution],
and b) to have a query_dv_timings() call ensuring rcar_vin_try_fmt()
works with the resolution of the live stream [subsequent queries to the
driver stop returning the default resolution after that, also as per
adv761x.c].

   I am currently investigating an enhancement to that solution in
which the enum_dv_timings op is used to recover the maximum supported
resolution of the new driver, and we hit a line in the driver which
exits the corresponding function. It reads:
 	if (timings->pad >= state->source_pad)
 	        return -EINVAL;
   It suffices to comment out this line, but clearly this is not ideal.
Depending on the intended semantics, should it be filtering out all pad
IDs not matching the active one, or all pad IDs that are not valid
input sources? Unfortunately the lager board's adv7180 chip is too
simple to make a sensible comparison case (that we can also run tests
on) here.

   Please advise. Comments would also be welcome regarding whether the
shims describe changes that should live in the driver or elsewhere in
soc_camera/rcar_vin in an acceptable solution.

Cheers,
   Wills.

Re: [Linux-kernel] RFC: supporting adv7604.c under soc_camera/rcar_vin

[Hans Verkuil]

On 03/04/15 10:51, William Towle wrote:
> 
> Hi all,
> 
>   I would like to develop a point in my previous discussion based on
> new findings:
> 
> On Thu, 29 Jan 2015, William Towle wrote:
>> 3. Our third problem concerns detecting the resolution of the stream.
>> Our code works with the obsoleted driver (adv761x.c) in place, but with
>> our modifications to adv7604.c we have seen a) recovery of a 640x480
>> image which is cropped rather than scaled, and/or b) recovery of a
>> 2048x2048 image with the stream content in the top left corner.
> 
>   We have since ported this code from 3.17 to 3.19 (Hans' "subdev2"
> branch) and removed the unnecessary backward compatibility sections.
> Some of the the behaviour is somewhat different in the port, but
> I'll discuss that separately. Here I intend to discuss a possible bug
> in adv7604.c.
> 
>   In our 3.17-based submission, we had shim code in soc_camera/rcar_vin
> in order to emulate the old driver (originally serving to "test drive"
> the new driver in an older kernel). For a test case with gstreamer
> capturing a single frame it was sufficient at the time a) to override
> the driver's default resolution with something larger when first probed
> [emulating adv761x.c defaulting to the maximum supported resolution],
> and b) to have a query_dv_timings() call ensuring rcar_vin_try_fmt()
> works with the resolution of the live stream [subsequent queries to the
> driver stop returning the default resolution after that, also as per
> adv761x.c].
> 
>   I am currently investigating an enhancement to that solution in
> which the enum_dv_timings op is used to recover the maximum supported
> resolution of the new driver, and we hit a line in the driver which
> exits the corresponding function. It reads:
>     if (timings->pad >= state->source_pad)
>             return -EINVAL;
>   It suffices to comment out this line, but clearly this is not ideal.
> Depending on the intended semantics, should it be filtering out all pad
> IDs not matching the active one, or all pad IDs that are not valid
> input sources? Unfortunately the lager board's adv7180 chip is too
> simple to make a sensible comparison case (that we can also run tests
> on) here.

The adv7604 code is not ideal, although the pad test is valid (you shouldn't
be able to ask timings for pads that do not exist).

Perfect code would:

1) check if the requested pad is active (hooked up), and return -EINVAL if not.
2) check if the pad is digital or analog and return a different list of
timings accordingly (the max frequency is different between the two). See
the adv7842.c driver how that should be done.

But in the meantime, why not just set timings->pad to 0 in rcar_vin? Or
get it from platform data or something like that.

>   Please advise. Comments would also be welcome regarding whether the
> shims describe changes that should live in the driver or elsewhere in
> soc_camera/rcar_vin in an acceptable solution.

I'm not entirely sure what it is you are referring to. As you know I am
working to get rid of the duplicated video ops that are also available as
pad ops. No shims required since everything will be converted.

Regards,

	Hans

Re: [Linux-kernel] RFC: supporting adv7604.c under soc_camera/rcar_vin

[William Towle]

On Wed, 4 Mar 2015, Hans Verkuil wrote:
> On 03/04/15 10:51, William Towle wrote:
>>     if (timings->pad >= state->source_pad)
>>             return -EINVAL;
>>   It suffices to comment out this line, but clearly this is not ideal.
>> Depending on the intended semantics, should it be filtering out all pad
>> IDs not matching the active one, or all pad IDs that are not valid
>> input sources? Unfortunately the lager board's adv7180 chip is too
>> simple to make a sensible comparison case (that we can also run tests
>> on) here.
>
> The adv7604 code is not ideal, although the pad test is valid (you shouldn't
> be able to ask timings for pads that do not exist).

   Right, thanks. It seems I have initialisation code that is making
inappropriate assumptions earlier on. I'll investigate this.


>>   Please advise. Comments would also be welcome regarding whether the
>> shims describe changes that should live in the driver or elsewhere in
>> soc_camera/rcar_vin in an acceptable solution.
>
> I'm not entirely sure what it is you are referring to.

   Amongst our various modifications to soc_camera/rcar_vin we have a
'struct media_pad' object in order to communicate with the adv7604
driver, and latterly an array of 'struct v4l2_subdev_pad_config's to
handle format information ... but there is more to be done, obviously,
and you have pointed us in the right direction above :)

Cheers,
   Wills.

Re: [Linux-kernel] RFC: supporting adv7604.c under soc_camera/rcar_vin

[William Towle]

Hello again all,

   Previously I promised to comment further on progress with our work
supporting HDMI input on Lager. After studying commit 4c28078 "[media]
rcar_vin: Add scaling support" on Hans' subdev2 branch, I have come to
the conclusion that the following is actually reasonable behaviour when
no attempt to determine the resolution of the live stream (a non-VGA
720x576 in our case) has been made:

> with
> our modifications to adv7604.c we have seen a) recovery of a 640x480
> image which is cropped rather than scaled, and/or b) recovery of a
> 2048x2048 image with the stream content in the top left corner.

   With our latest patchset, frame capture is essentially working and I
believe a solution is close, but the scaling changes introduce
behaviour that seems to be a regression for this particular test case
- see final point below. We have:

   1. ported code to subdev2 tree (with backward-compatibility for legacy
driver removed)
   2. adjusted adv7604.c to detect the ADV7612, adding calls to
get_fmt/set_fmt/enum_mbus_code pad ops to suit
   3. removed our attempt to query the driver's maximum resolution - the
scaling patch above configures the pre-clip/post-clip rectangles
sensibly without it
   4. modified our attempt to query the live resolution by doing this in
adv7604_fill_format() ... by reinstating adv761x_get_vid_info() as a
lightweight means of recovering the resolution, then using
query_dv_timings to update properly if state->timings is inconsistent
with its results; this leads to the need for:
   5. reverting the following, as not updating 'pix' with the (possibly
updated) resolution set_fmt returns leads to userland requesting an
inappropriately sized output buffer:
 	[commit 4c28078, static int rcar_vin_try_fmt(...)]
 	-       pix->width = mf.width;
 	-       pix->height = mf.height;
 	+       /* Adjust only if VIN cannot scale */
 	+       if (pix->width > mf.width * 2)
 	+               pix->width = mf.width * 2;
 	+       if (pix->height > mf.height * 3)
 	+               pix->height = mf.height * 3;

   Does this seem reasonable, or are there better ways of implementing
any of the above steps?

Cheers,
   Wills.