subject:"Re\: \[PATCH v4 3\/8\] media\: adv7180\: add support for NEWAVMODE"

Re: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE

2016-11-14 Thread Steve Longerbeam




On 11/14/2016 03:28 AM, Hans Verkuil wrote:

On 08/03/2016 08:03 PM, Steve Longerbeam wrote:

Parse the optional v4l2 endpoint DT node. If the bus type is
V4L2_MBUS_BT656 and the endpoint node specifies "newavmode",
configure the BT.656 bus in NEWAVMODE.

Signed-off-by: Steve Longerbeam 

---

v4: no changes
v3:
- the newavmode endpoint property is now private to adv7180.
---
  .../devicetree/bindings/media/i2c/adv7180.txt  |  4 ++
  drivers/media/i2c/adv7180.c| 46 --
  2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7180.txt 
b/Documentation/devicetree/bindings/media/i2c/adv7180.txt
index 0d50115..6c175d2 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7180.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7180.txt
@@ -15,6 +15,10 @@ Required Properties :
"adi,adv7282"
"adi,adv7282-m"
  
+Optional Endpoint Properties :

+- newavmode: a boolean property to indicate the BT.656 bus is operating
+  in Analog Device's NEWAVMODE. Valid for BT.656 busses only.

This is too vague.

Based on the ADV7280/ADV7281/ADV7282/ADV7283 Hardware Reference Manual I
would say something like this:

- newavmode: a boolean property to indicate the BT.656 bus is operating
   in Analog Device's NEWAVMODE. Valid for BT.656 busses only. When enabled
   the generated EAV/SAV codes are suitable for Analog Devices encoders.
   Otherwise these codes are setup according to 
   See bit 4 of user sub map register 0x31 in the Hardware Reference Manual.

I may have asked this before, but do you actually have hardware that needs
this? If so, it may be useful to give it as an example and explain why it
is needed.

If not, then I wonder if this cannot be dropped until we DO see hardware
that needs it.


Hi Hans, thanks for reviewing this, but at least for imx6, I don't
need this patch anymore.

Recently I dug deeper into the current bt.656 programming in
adv7180.c. The driver manually configures the bus to have 21
blank lines in odd fields, and 22 blank lines in even fields (via
NVEND register) for NTSC.

That leaves 525 - (21 +22) = 482 active lines in NTSC.

After configuring the imx6 host bridge to crop those extra 2 lines,
it is capturing good 720x480 NTSC images now.

So I no longer need this patch to enable NEWAVMODE.

However I still see some issues.

First, adv7180.c attempts to enable  BT.656-4 mode, but according
to the datasheet, that cannot be enabled without first enabling
NEWAVMODE. So the attempt to enable BT.656-4 mode is a no-op,
it is currently doing nothing. So I suggest removing that attempt.

Second, it is wrong for the host bridge to have to make an assumption
about cropping for a sensor. The adv7180 needs to communicate to
hosts about the number of field blanking lines it has configured, maybe
via get_selection. I.e., report full sensor frame via get_fmt, and 720x482
via get_selection.


Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE

2016-11-14 Thread Hans Verkuil

On 08/03/2016 08:03 PM, Steve Longerbeam wrote:
> Parse the optional v4l2 endpoint DT node. If the bus type is
> V4L2_MBUS_BT656 and the endpoint node specifies "newavmode",
> configure the BT.656 bus in NEWAVMODE.
> 
> Signed-off-by: Steve Longerbeam 
> 
> ---
> 
> v4: no changes
> v3:
> - the newavmode endpoint property is now private to adv7180.
> ---
>  .../devicetree/bindings/media/i2c/adv7180.txt  |  4 ++
>  drivers/media/i2c/adv7180.c| 46 
> --
>  2 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7180.txt 
> b/Documentation/devicetree/bindings/media/i2c/adv7180.txt
> index 0d50115..6c175d2 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adv7180.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/adv7180.txt
> @@ -15,6 +15,10 @@ Required Properties :
>   "adi,adv7282"
>   "adi,adv7282-m"
>  
> +Optional Endpoint Properties :
> +- newavmode: a boolean property to indicate the BT.656 bus is operating
> +  in Analog Device's NEWAVMODE. Valid for BT.656 busses only.

This is too vague.

Based on the ADV7280/ADV7281/ADV7282/ADV7283 Hardware Reference Manual I
would say something like this:

- newavmode: a boolean property to indicate the BT.656 bus is operating
  in Analog Device's NEWAVMODE. Valid for BT.656 busses only. When enabled
  the generated EAV/SAV codes are suitable for Analog Devices encoders.
  Otherwise these codes are setup according to 
  See bit 4 of user sub map register 0x31 in the Hardware Reference Manual.

I may have asked this before, but do you actually have hardware that needs
this? If so, it may be useful to give it as an example and explain why it
is needed.

If not, then I wonder if this cannot be dropped until we DO see hardware
that needs it.

Regards,

Hans


> +
>  Example:
>  
>   i2c0@1c22000 {
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 6e093c22..467953e 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -31,6 +31,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -106,6 +107,7 @@
>  #define ADV7180_REG_SHAP_FILTER_CTL_10x0017
>  #define ADV7180_REG_CTRL_2   0x001d
>  #define ADV7180_REG_VSYNC_FIELD_CTL_10x0031
> +#define ADV7180_VSYNC_FIELD_CTL_1_NEWAVMODE 0x02
>  #define ADV7180_REG_MANUAL_WIN_CTL_1 0x003d
>  #define ADV7180_REG_MANUAL_WIN_CTL_2 0x003e
>  #define ADV7180_REG_MANUAL_WIN_CTL_3 0x003f
> @@ -214,6 +216,7 @@ struct adv7180_state {
>   struct mutexmutex; /* mutual excl. when accessing chip */
>   int irq;
>   v4l2_std_id curr_norm;
> + boolnewavmode;
>   boolpowered;
>   boolstreaming;
>   u8  input;
> @@ -864,9 +867,15 @@ static int adv7180_init(struct adv7180_state *state)
>   if (ret < 0)
>   return ret;
>  
> - /* Manually set V bit end position in NTSC mode */
> - return adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
> - ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
> + if (!state->newavmode) {
> + /* Manually set V bit end position in NTSC mode */
> + ret = adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
> + ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
>  }
>  
>  static int adv7180_set_std(struct adv7180_state *state, unsigned int std)
> @@ -1217,6 +1226,13 @@ static int init_device(struct adv7180_state *state)
>   if (ret)
>   goto out_unlock;
>  
> + if (state->newavmode) {
> + ret = adv7180_write(state, ADV7180_REG_VSYNC_FIELD_CTL_1,
> + ADV7180_VSYNC_FIELD_CTL_1_NEWAVMODE);
> + if (ret < 0)
> + goto out_unlock;
> + }
> +
>   ret = adv7180_program_std(state);
>   if (ret)
>   goto out_unlock;
> @@ -1257,6 +1273,28 @@ out_unlock:
>   return ret;
>  }
>  
> +static void adv7180_of_parse(struct adv7180_state *state)
> +{
> + struct i2c_client *client = state->client;
> + struct device_node *np = client->dev.of_node;
> + struct device_node *endpoint;
> + struct v4l2_of_endpoint ep;
> +
> + endpoint = of_graph_get_next_endpoint(np, NULL);
> + if (!endpoint) {
> + v4l_warn(client, "endpoint node not found\n");
> + return;
> + }
> +
> + v4l2_of_parse_endpoint(endpoint, );
> + if (ep.bus_type == V4L2_MBUS_BT656) {
> + if (of_property_read_bool(endpoint, "newavmode"))
> + state->newavmode = true;
> + }
> +
> + of_node_put(endpoint);
> +}
> +
>  static int

Re: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE

2016-10-18 Thread Laurent Pinchart

Hi Steve,

On Monday 17 Oct 2016 15:36:14 Steve Longerbeam wrote:
> On 10/16/2016 05:18 AM, Laurent Pinchart wrote:
> > On Wednesday 03 Aug 2016 11:03:45 Steve Longerbeam wrote:
> >> Parse the optional v4l2 endpoint DT node. If the bus type is
> >> V4L2_MBUS_BT656 and the endpoint node specifies "newavmode",
> >> configure the BT.656 bus in NEWAVMODE.
> >> 
> >> Signed-off-by: Steve Longerbeam 
> >> ---
> >> v4: no changes
> >> v3:
> >> - the newavmode endpoint property is now private to adv7180.
> >> ---
> >> .../devicetree/bindings/media/i2c/adv7180.txt  |  4 ++
> >> drivers/media/i2c/adv7180.c| 46 ++--
> >> 2 files changed, 47 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7180.txt
> >> b/Documentation/devicetree/bindings/media/i2c/adv7180.txt index
> >> 0d50115..6c175d2 100644
> >> --- a/Documentation/devicetree/bindings/media/i2c/adv7180.txt
> >> +++ b/Documentation/devicetree/bindings/media/i2c/adv7180.txt
> >> @@ -15,6 +15,10 @@ Required Properties :
> >>"adi,adv7282"
> >>"adi,adv7282-m"
> >> 
> >> +Optional Endpoint Properties :
> >> +- newavmode: a boolean property to indicate the BT.656 bus is operating
> >> +  in Analog Device's NEWAVMODE. Valid for BT.656 busses only.
> > 
> > This is a vendor-specific property, it should be prefixed with "adi,".
> 
> Ok, I'll do that in next version.
> 
> >   Could
> > 
> > you also explain how this mode works ? I'd like to make sure it qualifies
> > for a DT property.
> 
> The blurb in the ADV718x manual is terse:
> 
> "When NEWAVMODE is 0 (enabled), EAV/SAV codes are generated to
> suit Analog Devices encoders. No adjustments are possible."
> 
> "Setting NEWAVMODE to 1 (default) enables the manual position
> of the VSYNC, FIELD, and AV codes using Register 0x32 to
> Register 0x33 and Register 0xE5 to Register 0xEA. Default register
> settings are CCIR656 compliant;"
> 
> So it's not clear to me how the generated EAV and SAV codes are
> different from standard CCIR656, but apparently they are.

Shouldn't we understand how it works to create proper DT bindings ? :-)

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE

2016-10-17 Thread Steve Longerbeam




On 10/16/2016 05:18 AM, Laurent Pinchart wrote:

Hi Steve,

Thank you for the patch.

On Wednesday 03 Aug 2016 11:03:45 Steve Longerbeam wrote:

Parse the optional v4l2 endpoint DT node. If the bus type is
V4L2_MBUS_BT656 and the endpoint node specifies "newavmode",
configure the BT.656 bus in NEWAVMODE.

Signed-off-by: Steve Longerbeam 

---

v4: no changes
v3:
- the newavmode endpoint property is now private to adv7180.
---
  .../devicetree/bindings/media/i2c/adv7180.txt  |  4 ++
  drivers/media/i2c/adv7180.c| 46 +--
  2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7180.txt
b/Documentation/devicetree/bindings/media/i2c/adv7180.txt index
0d50115..6c175d2 100644
--- a/Documentation/devicetree/bindings/media/i2c/adv7180.txt
+++ b/Documentation/devicetree/bindings/media/i2c/adv7180.txt
@@ -15,6 +15,10 @@ Required Properties :
"adi,adv7282"
"adi,adv7282-m"

+Optional Endpoint Properties :
+- newavmode: a boolean property to indicate the BT.656 bus is operating
+  in Analog Device's NEWAVMODE. Valid for BT.656 busses only.

This is a vendor-specific property, it should be prefixed with "adi,".


Ok, I'll do that in next version.


  Could
you also explain how this mode works ? I'd like to make sure it qualifies for
a DT property.


The blurb in the ADV718x manual is terse:

"When NEWAVMODE is 0 (enabled), EAV/SAV codes are generated to
suit Analog Devices encoders. No adjustments are possible."

"Setting NEWAVMODE to 1 (default) enables the manual position
of the VSYNC, FIELD, and AV codes using Register 0x32 to
Register 0x33 and Register 0xE5 to Register 0xEA. Default register
settings are CCIR656 compliant;"

So it's not clear to me how the generated EAV and SAV codes are
different from standard CCIR656, but apparently they are.

Steve





+
  Example:

i2c0@1c22000 {
diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 6e093c22..467953e 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -31,6 +31,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 

@@ -106,6 +107,7 @@
  #define ADV7180_REG_SHAP_FILTER_CTL_1 0x0017
  #define ADV7180_REG_CTRL_20x001d
  #define ADV7180_REG_VSYNC_FIELD_CTL_1 0x0031
+#define ADV7180_VSYNC_FIELD_CTL_1_NEWAVMODE 0x02
  #define ADV7180_REG_MANUAL_WIN_CTL_1  0x003d
  #define ADV7180_REG_MANUAL_WIN_CTL_2  0x003e
  #define ADV7180_REG_MANUAL_WIN_CTL_3  0x003f
@@ -214,6 +216,7 @@ struct adv7180_state {
struct mutexmutex; /* mutual excl. when accessing chip */
int irq;
v4l2_std_id curr_norm;
+   boolnewavmode;
boolpowered;
boolstreaming;
u8  input;
@@ -864,9 +867,15 @@ static int adv7180_init(struct adv7180_state *state)
if (ret < 0)
return ret;

-   /* Manually set V bit end position in NTSC mode */
-   return adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
-   ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
+   if (!state->newavmode) {
+   /* Manually set V bit end position in NTSC mode */
+   ret = adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
+   ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
+   if (ret < 0)
+   return ret;
+   }
+
+   return 0;
  }

  static int adv7180_set_std(struct adv7180_state *state, unsigned int std)
@@ -1217,6 +1226,13 @@ static int init_device(struct adv7180_state *state)
if (ret)
goto out_unlock;

+   if (state->newavmode) {
+   ret = adv7180_write(state, ADV7180_REG_VSYNC_FIELD_CTL_1,
+   ADV7180_VSYNC_FIELD_CTL_1_NEWAVMODE);
+   if (ret < 0)
+   goto out_unlock;
+   }
+
ret = adv7180_program_std(state);
if (ret)
goto out_unlock;
@@ -1257,6 +1273,28 @@ out_unlock:
return ret;
  }

+static void adv7180_of_parse(struct adv7180_state *state)
+{
+   struct i2c_client *client = state->client;
+   struct device_node *np = client->dev.of_node;
+   struct device_node *endpoint;
+   struct v4l2_of_endpoint ep;
+
+   endpoint = of_graph_get_next_endpoint(np, NULL);
+   if (!endpoint) {
+   v4l_warn(client, "endpoint node not found\n");
+   return;
+   }
+
+   v4l2_of_parse_endpoint(endpoint, );
+   if (ep.bus_type == V4L2_MBUS_BT656) {
+   if (of_property_read_bool(endpoint, "newavmode"))
+   state->newavmode = true;
+   }
+
+   of_node_put(endpoint);
+}
+
  static int adv7180_probe(struct i2c_client *client,

Re: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE

2016-10-16 Thread Laurent Pinchart

Hi Steve,

Thank you for the patch.

On Wednesday 03 Aug 2016 11:03:45 Steve Longerbeam wrote:
> Parse the optional v4l2 endpoint DT node. If the bus type is
> V4L2_MBUS_BT656 and the endpoint node specifies "newavmode",
> configure the BT.656 bus in NEWAVMODE.
> 
> Signed-off-by: Steve Longerbeam 
> 
> ---
> 
> v4: no changes
> v3:
> - the newavmode endpoint property is now private to adv7180.
> ---
>  .../devicetree/bindings/media/i2c/adv7180.txt  |  4 ++
>  drivers/media/i2c/adv7180.c| 46 +--
>  2 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7180.txt
> b/Documentation/devicetree/bindings/media/i2c/adv7180.txt index
> 0d50115..6c175d2 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adv7180.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/adv7180.txt
> @@ -15,6 +15,10 @@ Required Properties :
>   "adi,adv7282"
>   "adi,adv7282-m"
> 
> +Optional Endpoint Properties :
> +- newavmode: a boolean property to indicate the BT.656 bus is operating
> +  in Analog Device's NEWAVMODE. Valid for BT.656 busses only.

This is a vendor-specific property, it should be prefixed with "adi,". Could 
you also explain how this mode works ? I'd like to make sure it qualifies for 
a DT property.

> +
>  Example:
> 
>   i2c0@1c22000 {
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 6e093c22..467953e 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -31,6 +31,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
> 
> @@ -106,6 +107,7 @@
>  #define ADV7180_REG_SHAP_FILTER_CTL_10x0017
>  #define ADV7180_REG_CTRL_2   0x001d
>  #define ADV7180_REG_VSYNC_FIELD_CTL_10x0031
> +#define ADV7180_VSYNC_FIELD_CTL_1_NEWAVMODE 0x02
>  #define ADV7180_REG_MANUAL_WIN_CTL_1 0x003d
>  #define ADV7180_REG_MANUAL_WIN_CTL_2 0x003e
>  #define ADV7180_REG_MANUAL_WIN_CTL_3 0x003f
> @@ -214,6 +216,7 @@ struct adv7180_state {
>   struct mutexmutex; /* mutual excl. when accessing chip */
>   int irq;
>   v4l2_std_id curr_norm;
> + boolnewavmode;
>   boolpowered;
>   boolstreaming;
>   u8  input;
> @@ -864,9 +867,15 @@ static int adv7180_init(struct adv7180_state *state)
>   if (ret < 0)
>   return ret;
> 
> - /* Manually set V bit end position in NTSC mode */
> - return adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
> - ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
> + if (!state->newavmode) {
> + /* Manually set V bit end position in NTSC mode */
> + ret = adv7180_write(state, ADV7180_REG_NTSC_V_BIT_END,
> + ADV7180_NTSC_V_BIT_END_MANUAL_NVEND);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
>  }
> 
>  static int adv7180_set_std(struct adv7180_state *state, unsigned int std)
> @@ -1217,6 +1226,13 @@ static int init_device(struct adv7180_state *state)
>   if (ret)
>   goto out_unlock;
> 
> + if (state->newavmode) {
> + ret = adv7180_write(state, ADV7180_REG_VSYNC_FIELD_CTL_1,
> + ADV7180_VSYNC_FIELD_CTL_1_NEWAVMODE);
> + if (ret < 0)
> + goto out_unlock;
> + }
> +
>   ret = adv7180_program_std(state);
>   if (ret)
>   goto out_unlock;
> @@ -1257,6 +1273,28 @@ out_unlock:
>   return ret;
>  }
> 
> +static void adv7180_of_parse(struct adv7180_state *state)
> +{
> + struct i2c_client *client = state->client;
> + struct device_node *np = client->dev.of_node;
> + struct device_node *endpoint;
> + struct v4l2_of_endpoint ep;
> +
> + endpoint = of_graph_get_next_endpoint(np, NULL);
> + if (!endpoint) {
> + v4l_warn(client, "endpoint node not found\n");
> + return;
> + }
> +
> + v4l2_of_parse_endpoint(endpoint, );
> + if (ep.bus_type == V4L2_MBUS_BT656) {
> + if (of_property_read_bool(endpoint, "newavmode"))
> + state->newavmode = true;
> + }
> +
> + of_node_put(endpoint);
> +}
> +
>  static int adv7180_probe(struct i2c_client *client,
>const struct i2c_device_id *id)
>  {
> @@ -1279,6 +1317,8 @@ static int adv7180_probe(struct i2c_client *client,
>   state->field = V4L2_FIELD_ALTERNATE;
>   state->chip_info = (struct adv7180_chip_info *)id->driver_data;
> 
> + adv7180_of_parse(state);
> +
>   if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
>   state->csi_client = i2c_new_dummy(client->adapter,
>   ADV7180_DEFAULT_CSI_I2C_ADDR);

-- 
Regards,

Re: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE

Re: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE

Re: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE

Re: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE

Re: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE

5 matches

Site Navigation

Mail list logo

Footer information