[PATCH] media: i2c: adv7180: fix reserved bit in Video Selection 2

[PATCH] media: i2c: adv7180: fix reserved bit in Video Selection 2

[Benjamin Marty]

This bit is marked as reserved in the ADV Hardware Reference Manual.

Resetting this bit seems to cause increased video noise. Setting this bit according to the Hardware Reference Manual reduces the video noise immediately.

Signed-off-by: Benjamin Marty <info@benjaminmarty.ch>
---
 drivers/media/i2c/adv7180.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 4f5db195e66d..d99b22286b74 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -1014,7 +1014,8 @@ static int adv7182_init(struct adv7180_state *state)
 
 static int adv7182_set_std(struct adv7180_state *state, unsigned int std)
 {
-	return adv7180_write(state, ADV7182_REG_INPUT_VIDSEL, std << 4);
+	return adv7180_write(state, ADV7182_REG_INPUT_VIDSEL,
+			     (std << 4) | (0x01 << 2));
 }
 
 enum adv7182_input_type {
-- 
2.36.1

Re: [PATCH] media: i2c: adv7180: fix reserved bit in Video Selection 2

[Kieran Bingham]

Hi Benjamin,

Thank you for your patch, and your investigation into this issue.

Quoting Benjamin Marty (2022-05-12 09:08:59)
> This bit is marked as reserved in the ADV Hardware Reference Manual.
> 
> Resetting this bit seems to cause increased video noise. Setting this bit according to the Hardware Reference Manual reduces the video noise immediately.

It's quite minor, but please try to wrap your commit messages:

> Resetting this bit seems to cause increased video noise. Setting this
> bit according to the Hardware Reference Manual reduces the video noise
> immediately.


> Signed-off-by: Benjamin Marty <info@benjaminmarty.ch>
> ---
>  drivers/media/i2c/adv7180.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 4f5db195e66d..d99b22286b74 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -1014,7 +1014,8 @@ static int adv7182_init(struct adv7180_state *state)
>  
>  static int adv7182_set_std(struct adv7180_state *state, unsigned int std)
>  {
> -       return adv7180_write(state, ADV7182_REG_INPUT_VIDSEL, std << 4);
> +       return adv7180_write(state, ADV7182_REG_INPUT_VIDSEL,
> +                            (std << 4) | (0x01 << 2));

This should be defined using a macro and use BIT() to be clearer, for
instance:

 #define ADV7182_REG_INPUT_RESERVED BIT(2)

That definition should live near the defintion of
ADV7182_REG_INPUT_VIDSEL.

If the bit is documented with a better name, then use that of course,
otherwise perhaps even a comment in the code saying that failing to set
the bit increases visible noise would be suitable. (or that setting the
bit reduces noise, I guess it depends on if you think this bit is
performing noise reduction, or if not setting it is introducing noise)

--
Kieran



>  }
>  
>  enum adv7182_input_type {
> -- 
> 2.36.1
>