diff mbox series

[v8,11/13] squash! max9286: Disable overlap window

Message ID 20200409121202.11130-12-kieran.bingham+renesas@ideasonboard.com (mailing list archive)
State New
Delegated to: Kieran Bingham
Headers show
Series max9286 v8 - modifications | expand

Commit Message

Kieran Bingham April 9, 2020, 12:12 p.m. UTC
Provide a function to control setting of the overlap window, but disable
it by default.

The function will allow the value to be easily updated in the future,
either statically in the code, or via an external control mechanism.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 42 +++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

Comments

Jacopo Mondi April 9, 2020, 4:32 p.m. UTC | #1
Hi Kieran,

On Thu, Apr 09, 2020 at 01:12:00PM +0100, Kieran Bingham wrote:
> Provide a function to control setting of the overlap window, but disable
> it by default.
>
> The function will allow the value to be easily updated in the future,
> either statically in the code, or via an external control mechanism.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 42 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 008a93910300..61178ae363d6 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -118,6 +118,9 @@
>  #define MAX9286_REV_FLEN(n)		((n) - 20)
>  /* Register 0x49 */
>  #define MAX9286_VIDEO_DETECT_MASK	0x0f
> +/* Register 0x64 */
> +#define MAX9286_ENFSINLAST		BIT(5)
> +#define MAX9286_OVLP_WINDOWH_MASK	GENMASK(4, 0)
>  /* Register 0x69 */
>  #define MAX9286_LFLTBMONMASKED		BIT(7)
>  #define MAX9286_LOCKMONMASKED		BIT(6)
> @@ -632,6 +635,34 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	return 0;
>  }
>
> +/*
> + * The overlap window is a 13 bit value with the low byte in register 0x63 and
> + * the high byte(5bits) stored in the least significant bits of register 0x64.
                   ^ space

But I'm not sure this is useful comment. The register layout is
actually the only documented thing of this register :)

> + */
> +static int max9286_set_overlap_window(struct max9286_priv *priv, u16 window)
> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = max9286_read(priv, 0x64);
> +	if (ret < 0)
> +		return -EIO;
> +
> +	max9286_write(priv, 0x63, window & 0xff);
> +
> +	/*
> +	 * Process the high byte, while preserve existing bits set in 0x64.
> +	 * TODO: Convert this all to regmap so we can utilise regmap_update_bits
> +	 */
> +	window >>= 8;
> +	val = ret & ~MAX9286_OVLP_WINDOWH_MASK;
> +	val |= window & MAX9286_OVLP_WINDOWH_MASK;
> +
> +	max9286_write(priv, 0x64, val);
> +
> +	return 0;
> +}
> +
>  static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
>  {
>  	if (!priv->pixelrate)
> @@ -942,6 +973,17 @@ static int max9286_setup(struct max9286_priv *priv)
>  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
>  		      MAX9286_HVSRC_D14);
>
> +	/*
> +	 * The overlap window seems to provide additional validation by tracking
> +	 * the delay between vsync and frame sync, generating an error if the
> +	 * delay is bigger than the programmed window, though it's not yet clear
> +	 * what value should be set.
> +	 *
> +	 * As it's an optional value and can be disabled, we do so by setting
> +	 * a 0 overlap value.
> +	 */

This is useful instead :)


> +	max9286_set_overlap_window(priv, 0);
> +
>  	/*
>  	 * Wait for 2ms to allow the link to resynchronize after the
>  	 * configuration change.
> --
> 2.20.1
>
Kieran Bingham April 10, 2020, 7:14 a.m. UTC | #2
Hi Jacopo,

On 09/04/2020 17:32, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Thu, Apr 09, 2020 at 01:12:00PM +0100, Kieran Bingham wrote:
>> Provide a function to control setting of the overlap window, but disable
>> it by default.
>>
>> The function will allow the value to be easily updated in the future,
>> either statically in the code, or via an external control mechanism.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/media/i2c/max9286.c | 42 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 42 insertions(+)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index 008a93910300..61178ae363d6 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -118,6 +118,9 @@
>>  #define MAX9286_REV_FLEN(n)		((n) - 20)
>>  /* Register 0x49 */
>>  #define MAX9286_VIDEO_DETECT_MASK	0x0f
>> +/* Register 0x64 */
>> +#define MAX9286_ENFSINLAST		BIT(5)
>> +#define MAX9286_OVLP_WINDOWH_MASK	GENMASK(4, 0)
>>  /* Register 0x69 */
>>  #define MAX9286_LFLTBMONMASKED		BIT(7)
>>  #define MAX9286_LOCKMONMASKED		BIT(6)
>> @@ -632,6 +635,34 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>>  	return 0;
>>  }
>>
>> +/*
>> + * The overlap window is a 13 bit value with the low byte in register 0x63 and
>> + * the high byte(5bits) stored in the least significant bits of register 0x64.
>                    ^ space
> 
> But I'm not sure this is useful comment. The register layout is
> actually the only documented thing of this register :)

Sure, but readers won't necessarily have the documentation :-) I think I
was just trying to explain what was happening because we have so many
unidentified registers (i.e. they're all just numbers, so we can't do,

  max9286_write(priv, MAX9286_OVERLAP_LOW, overlap & 0xff);
  max9286_write(priv, MAX9286_OVERLAP_HIGH, (overlap >> SHIFT) & MASK);

I can drop this comment all the same, although I'd like to document
clearly that it's a 13-bit value as an input, (not a full 16 bit)

Will this be better suited?


/*
 * The overlap window is a 13 bit value stored across two registers.
 * The definition and units of the value is undocumented.
 */



>> + */
>> +static int max9286_set_overlap_window(struct max9286_priv *priv, u16 window)
>> +{
>> +	int ret;
>> +	u8 val;
>> +
>> +	ret = max9286_read(priv, 0x64);
>> +	if (ret < 0)
>> +		return -EIO;
>> +
>> +	max9286_write(priv, 0x63, window & 0xff);
>> +
>> +	/*
>> +	 * Process the high byte, while preserve existing bits set in 0x64.
>> +	 * TODO: Convert this all to regmap so we can utilise regmap_update_bits
>> +	 */
>> +	window >>= 8;
>> +	val = ret & ~MAX9286_OVLP_WINDOWH_MASK;
>> +	val |= window & MAX9286_OVLP_WINDOWH_MASK;
>> +
>> +	max9286_write(priv, 0x64, val);
>> +
>> +	return 0;
>> +}
>> +
>>  static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
>>  {
>>  	if (!priv->pixelrate)
>> @@ -942,6 +973,17 @@ static int max9286_setup(struct max9286_priv *priv)
>>  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
>>  		      MAX9286_HVSRC_D14);
>>
>> +	/*
>> +	 * The overlap window seems to provide additional validation by tracking
>> +	 * the delay between vsync and frame sync, generating an error if the
>> +	 * delay is bigger than the programmed window, though it's not yet clear
>> +	 * what value should be set.
>> +	 *
>> +	 * As it's an optional value and can be disabled, we do so by setting
>> +	 * a 0 overlap value.
>> +	 */
> 
> This is useful instead :)


:-)


>> +	max9286_set_overlap_window(priv, 0);
>> +
>>  	/*
>>  	 * Wait for 2ms to allow the link to resynchronize after the
>>  	 * configuration change.
>> --
>> 2.20.1
>>
Jacopo Mondi April 10, 2020, 7:48 a.m. UTC | #3
Hi Kieran,

On Fri, Apr 10, 2020 at 08:14:18AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 09/04/2020 17:32, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Thu, Apr 09, 2020 at 01:12:00PM +0100, Kieran Bingham wrote:
> >> Provide a function to control setting of the overlap window, but disable
> >> it by default.
> >>
> >> The function will allow the value to be easily updated in the future,
> >> either statically in the code, or via an external control mechanism.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> ---
> >>  drivers/media/i2c/max9286.c | 42 +++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 42 insertions(+)
> >>
> >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> >> index 008a93910300..61178ae363d6 100644
> >> --- a/drivers/media/i2c/max9286.c
> >> +++ b/drivers/media/i2c/max9286.c
> >> @@ -118,6 +118,9 @@
> >>  #define MAX9286_REV_FLEN(n)		((n) - 20)
> >>  /* Register 0x49 */
> >>  #define MAX9286_VIDEO_DETECT_MASK	0x0f
> >> +/* Register 0x64 */
> >> +#define MAX9286_ENFSINLAST		BIT(5)
> >> +#define MAX9286_OVLP_WINDOWH_MASK	GENMASK(4, 0)
> >>  /* Register 0x69 */
> >>  #define MAX9286_LFLTBMONMASKED		BIT(7)
> >>  #define MAX9286_LOCKMONMASKED		BIT(6)
> >> @@ -632,6 +635,34 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >>  	return 0;
> >>  }
> >>
> >> +/*
> >> + * The overlap window is a 13 bit value with the low byte in register 0x63 and
> >> + * the high byte(5bits) stored in the least significant bits of register 0x64.
> >                    ^ space
> >
> > But I'm not sure this is useful comment. The register layout is
> > actually the only documented thing of this register :)
>
> Sure, but readers won't necessarily have the documentation :-) I think I
> was just trying to explain what was happening because we have so many
> unidentified registers (i.e. they're all just numbers, so we can't do,
>
>   max9286_write(priv, MAX9286_OVERLAP_LOW, overlap & 0xff);
>   max9286_write(priv, MAX9286_OVERLAP_HIGH, (overlap >> SHIFT) & MASK);
>
> I can drop this comment all the same, although I'd like to document
> clearly that it's a 13-bit value as an input, (not a full 16 bit)
>
> Will this be better suited?
>
>
> /*
>  * The overlap window is a 13 bit value stored across two registers.
>  * The definition and units of the value is undocumented.
>  */

Oh no worries, if you want to keep that comment the first version is
perfectly fine.
diff mbox series

Patch

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 008a93910300..61178ae363d6 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -118,6 +118,9 @@ 
 #define MAX9286_REV_FLEN(n)		((n) - 20)
 /* Register 0x49 */
 #define MAX9286_VIDEO_DETECT_MASK	0x0f
+/* Register 0x64 */
+#define MAX9286_ENFSINLAST		BIT(5)
+#define MAX9286_OVLP_WINDOWH_MASK	GENMASK(4, 0)
 /* Register 0x69 */
 #define MAX9286_LFLTBMONMASKED		BIT(7)
 #define MAX9286_LOCKMONMASKED		BIT(6)
@@ -632,6 +635,34 @@  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	return 0;
 }
 
+/*
+ * The overlap window is a 13 bit value with the low byte in register 0x63 and
+ * the high byte(5bits) stored in the least significant bits of register 0x64.
+ */
+static int max9286_set_overlap_window(struct max9286_priv *priv, u16 window)
+{
+	int ret;
+	u8 val;
+
+	ret = max9286_read(priv, 0x64);
+	if (ret < 0)
+		return -EIO;
+
+	max9286_write(priv, 0x63, window & 0xff);
+
+	/*
+	 * Process the high byte, while preserve existing bits set in 0x64.
+	 * TODO: Convert this all to regmap so we can utilise regmap_update_bits
+	 */
+	window >>= 8;
+	val = ret & ~MAX9286_OVLP_WINDOWH_MASK;
+	val |= window & MAX9286_OVLP_WINDOWH_MASK;
+
+	max9286_write(priv, 0x64, val);
+
+	return 0;
+}
+
 static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate)
 {
 	if (!priv->pixelrate)
@@ -942,6 +973,17 @@  static int max9286_setup(struct max9286_priv *priv)
 	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
 		      MAX9286_HVSRC_D14);
 
+	/*
+	 * The overlap window seems to provide additional validation by tracking
+	 * the delay between vsync and frame sync, generating an error if the
+	 * delay is bigger than the programmed window, though it's not yet clear
+	 * what value should be set.
+	 *
+	 * As it's an optional value and can be disabled, we do so by setting
+	 * a 0 overlap value.
+	 */
+	max9286_set_overlap_window(priv, 0);
+
 	/*
 	 * Wait for 2ms to allow the link to resynchronize after the
 	 * configuration change.