Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1501444 > unrolled thread

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

Started byLaurent Pinchart <laurent.pinchart@ideasonboard.com>
First post2016-10-16 14:20 +0200
Last post2016-10-18 10:40 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-10-16 14:20 +0200
    Re: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE Steve Longerbeam <slongerbeam@gmail.com> - 2016-10-18 00:40 +0200
      Re: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-10-18 10:40 +0200

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

FromLaurent Pinchart <laurent.pinchart@ideasonboard.com>
Date2016-10-16 14:20 +0200
SubjectRe: [PATCH v4 3/8] media: adv7180: add support for NEWAVMODE
Message-ID<ssSA2-5KA-7@gated-at.bofh.it>
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 <steve_longerbeam@mentor.com>
> 
> ---
> 
> 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 <media/v4l2-event.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ctrls.h>
> +#include <media/v4l2-of.h>
>  #include <linux/mutex.h>
>  #include <linux/delay.h>
> 
> @@ -106,6 +107,7 @@
>  #define ADV7180_REG_SHAP_FILTER_CTL_1	0x0017
>  #define ADV7180_REG_CTRL_2		0x001d
>  #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 mutex		mutex; /* mutual excl. when accessing chip */
>  	int			irq;
>  	v4l2_std_id		curr_norm;
> +	bool			newavmode;
>  	bool			powered;
>  	bool			streaming;
>  	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, &ep);
> +	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,

Laurent Pinchart

[toc] | [next] | [standalone]

#1502556

FromSteve Longerbeam <slongerbeam@gmail.com>
Date2016-10-18 00:40 +0200
Message-ID<stoJB-1Co-55@gated-at.bofh.it>
In reply to#1501444

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 <steve_longerbeam@mentor.com>
>>
>> ---
>>
>> 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 <media/v4l2-event.h>
>>   #include <media/v4l2-device.h>
>>   #include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-of.h>
>>   #include <linux/mutex.h>
>>   #include <linux/delay.h>
>>
>> @@ -106,6 +107,7 @@
>>   #define ADV7180_REG_SHAP_FILTER_CTL_1	0x0017
>>   #define ADV7180_REG_CTRL_2		0x001d
>>   #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 mutex		mutex; /* mutual excl. when accessing chip */
>>   	int			irq;
>>   	v4l2_std_id		curr_norm;
>> +	bool			newavmode;
>>   	bool			powered;
>>   	bool			streaming;
>>   	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, &ep);
>> +	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);

[toc] | [prev] | [next] | [standalone]

#1502796

FromLaurent Pinchart <laurent.pinchart@ideasonboard.com>
Date2016-10-18 10:40 +0200
Message-ID<sty6d-7Dr-13@gated-at.bofh.it>
In reply to#1502556
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 <steve_longerbeam@mentor.com>
> >> ---
> >> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web