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

[PATCH v2] 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>
---
version 2:
- Fixed Kieran's remarks

 drivers/media/i2c/adv7180.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 4f5db195e66d..992111fe249e 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -43,6 +43,7 @@
 #define ADV7180_INPUT_CONTROL_INSEL_MASK		0x0f
 
 #define ADV7182_REG_INPUT_VIDSEL			0x0002
+#define ADV7182_REG_INPUT_RESERVED			BIT(2)
 
 #define ADV7180_REG_OUTPUT_CONTROL			0x0003
 #define ADV7180_REG_EXTENDED_OUTPUT_CONTROL		0x0004
@@ -1014,7 +1015,9 @@ 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);
+	/* Failing to set the reserved bit can result in increased video noise */
+	return adv7180_write(state, ADV7182_REG_INPUT_VIDSEL,
+			     (std << 4) | ADV7182_REG_INPUT_RESERVED);
 }
 
 enum adv7182_input_type {
-- 
2.36.1

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

[Dave Stevenson]

Hi Benjamin.

On Thu, 12 May 2022 at 13:11, Benjamin Marty <info@benjaminmarty.ch> wrote:
>
> 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>
> ---
> version 2:
> - Fixed Kieran's remarks
>
>  drivers/media/i2c/adv7180.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index 4f5db195e66d..992111fe249e 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -43,6 +43,7 @@
>  #define ADV7180_INPUT_CONTROL_INSEL_MASK               0x0f
>
>  #define ADV7182_REG_INPUT_VIDSEL                       0x0002
> +#define ADV7182_REG_INPUT_RESERVED                     BIT(2)

Responding to Kieran's comment on V1:
> 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)

I went digging through the datasheet for this info as I care about
ADV728[0|1|2]M.

https://www.analog.com/media/en/technical-documentation/data-sheets/ADV7182.pdf
page 68 defines bits 0-3 as reserved, and "set to default" which is
0100b.
https://www.analog.com/media/en/technical-documentation/user-guides/ADV7280_7281_7282_7283_UG-637.pdf
page 70 says the same for ADV7280/ADV7281/ADV7282/ADV7283.

So no name or detail in the docs over what the bits do.

The patch does mean the driver more closely follows the datasheet, so
it looks good to me.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

I'll try to find a couple of minutes to get my hardware out and
confirm I see the change in video noise.

  Dave

>  #define ADV7180_REG_OUTPUT_CONTROL                     0x0003
>  #define ADV7180_REG_EXTENDED_OUTPUT_CONTROL            0x0004
> @@ -1014,7 +1015,9 @@ 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);
> +       /* Failing to set the reserved bit can result in increased video noise */
> +       return adv7180_write(state, ADV7182_REG_INPUT_VIDSEL,
> +                            (std << 4) | ADV7182_REG_INPUT_RESERVED);
>  }
>
>  enum adv7182_input_type {
> --
> 2.36.1
>

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

[Kieran Bingham]

Quoting Dave Stevenson (2022-05-12 14:56:45)
> Hi Benjamin.
> 
> On Thu, 12 May 2022 at 13:11, Benjamin Marty <info@benjaminmarty.ch> wrote:
> >
> > 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>
> > ---
> > version 2:
> > - Fixed Kieran's remarks
> >
> >  drivers/media/i2c/adv7180.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > index 4f5db195e66d..992111fe249e 100644
> > --- a/drivers/media/i2c/adv7180.c
> > +++ b/drivers/media/i2c/adv7180.c
> > @@ -43,6 +43,7 @@
> >  #define ADV7180_INPUT_CONTROL_INSEL_MASK               0x0f
> >
> >  #define ADV7182_REG_INPUT_VIDSEL                       0x0002
> > +#define ADV7182_REG_INPUT_RESERVED                     BIT(2)
> 
> Responding to Kieran's comment on V1:
> > 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)
> 
> I went digging through the datasheet for this info as I care about
> ADV728[0|1|2]M.
> 
> https://www.analog.com/media/en/technical-documentation/data-sheets/ADV7182.pdf
> page 68 defines bits 0-3 as reserved, and "set to default" which is
> 0100b.
> https://www.analog.com/media/en/technical-documentation/user-guides/ADV7280_7281_7282_7283_UG-637.pdf
> page 70 says the same for ADV7280/ADV7281/ADV7282/ADV7283.
> 
> So no name or detail in the docs over what the bits do.
> 
> The patch does mean the driver more closely follows the datasheet, so
> it looks good to me.
> 
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> I'll try to find a couple of minutes to get my hardware out and
> confirm I see the change in video noise.

Great, Is there any way we can identify (easily?) if this is introducing
noise reduction, or preventing noise being added?

If it's introducing noise reduction, as a feature, that's quite
different to causing noise if it's not set ... (Unless perhaps people
have a desire to add noise :D)

But I think I could add this already:


Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> 
>   Dave
> 
> >  #define ADV7180_REG_OUTPUT_CONTROL                     0x0003
> >  #define ADV7180_REG_EXTENDED_OUTPUT_CONTROL            0x0004
> > @@ -1014,7 +1015,9 @@ 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);
> > +       /* Failing to set the reserved bit can result in increased video noise */
> > +       return adv7180_write(state, ADV7182_REG_INPUT_VIDSEL,
> > +                            (std << 4) | ADV7182_REG_INPUT_RESERVED);
> >  }
> >
> >  enum adv7182_input_type {
> > --
> > 2.36.1
> >

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

[Dave Stevenson]

On Fri, 13 May 2022 at 11:43, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Dave Stevenson (2022-05-12 14:56:45)
> > Hi Benjamin.
> >
> > On Thu, 12 May 2022 at 13:11, Benjamin Marty <info@benjaminmarty.ch> wrote:
> > >
> > > 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>
> > > ---
> > > version 2:
> > > - Fixed Kieran's remarks
> > >
> > >  drivers/media/i2c/adv7180.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > > index 4f5db195e66d..992111fe249e 100644
> > > --- a/drivers/media/i2c/adv7180.c
> > > +++ b/drivers/media/i2c/adv7180.c
> > > @@ -43,6 +43,7 @@
> > >  #define ADV7180_INPUT_CONTROL_INSEL_MASK               0x0f
> > >
> > >  #define ADV7182_REG_INPUT_VIDSEL                       0x0002
> > > +#define ADV7182_REG_INPUT_RESERVED                     BIT(2)
> >
> > Responding to Kieran's comment on V1:
> > > 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)
> >
> > I went digging through the datasheet for this info as I care about
> > ADV728[0|1|2]M.
> >
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADV7182.pdf
> > page 68 defines bits 0-3 as reserved, and "set to default" which is
> > 0100b.
> > https://www.analog.com/media/en/technical-documentation/user-guides/ADV7280_7281_7282_7283_UG-637.pdf
> > page 70 says the same for ADV7280/ADV7281/ADV7282/ADV7283.
> >
> > So no name or detail in the docs over what the bits do.
> >
> > The patch does mean the driver more closely follows the datasheet, so
> > it looks good to me.
> >
> > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > I'll try to find a couple of minutes to get my hardware out and
> > confirm I see the change in video noise.
>
> Great, Is there any way we can identify (easily?) if this is introducing
> noise reduction, or preventing noise being added?
>
> If it's introducing noise reduction, as a feature, that's quite
> different to causing noise if it's not set ... (Unless perhaps people
> have a desire to add noise :D)

OK, I dug out my hardware. I'm doing the nasty with:
i2ctransfer -y -f 10 w2@0x21 0x02 0x84
and
i2ctransfer -y -f 10 w2@0x21 0x02 0x80
to flip back and forth between the two settings on my PAL source.

It does reduce the noise, but also softens the image significantly.

As slightly iffy photos to show the difference
https://photos.app.goo.gl/hLKxv3TP93gX864y8 is the new setting.
https://photos.app.goo.gl/sWxEhdvxHLUkGL1C8 is the old setting.
(Yes it's a very old F1 race that happened to be on this DVD/HDD recorder).

I couldn't honestly say I prefer one over the other (analogue video
really is grim!), but it does mean that we're following the datasheet
more accurately, so:

Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> But I think I could add this already:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> >
> >   Dave
> >
> > >  #define ADV7180_REG_OUTPUT_CONTROL                     0x0003
> > >  #define ADV7180_REG_EXTENDED_OUTPUT_CONTROL            0x0004
> > > @@ -1014,7 +1015,9 @@ 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);
> > > +       /* Failing to set the reserved bit can result in increased video noise */
> > > +       return adv7180_write(state, ADV7182_REG_INPUT_VIDSEL,
> > > +                            (std << 4) | ADV7182_REG_INPUT_RESERVED);
> > >  }
> > >
> > >  enum adv7182_input_type {
> > > --
> > > 2.36.1
> > >

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

[Kieran Bingham]

Quoting Dave Stevenson (2022-05-17 18:50:11)
> On Fri, 13 May 2022 at 11:43, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting Dave Stevenson (2022-05-12 14:56:45)
> > > Hi Benjamin.
> > >
> > > On Thu, 12 May 2022 at 13:11, Benjamin Marty <info@benjaminmarty.ch> wrote:
> > > >
> > > > 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>
> > > > ---
> > > > version 2:
> > > > - Fixed Kieran's remarks
> > > >
> > > >  drivers/media/i2c/adv7180.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > > > index 4f5db195e66d..992111fe249e 100644
> > > > --- a/drivers/media/i2c/adv7180.c
> > > > +++ b/drivers/media/i2c/adv7180.c
> > > > @@ -43,6 +43,7 @@
> > > >  #define ADV7180_INPUT_CONTROL_INSEL_MASK               0x0f
> > > >
> > > >  #define ADV7182_REG_INPUT_VIDSEL                       0x0002
> > > > +#define ADV7182_REG_INPUT_RESERVED                     BIT(2)
> > >
> > > Responding to Kieran's comment on V1:
> > > > 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)
> > >
> > > I went digging through the datasheet for this info as I care about
> > > ADV728[0|1|2]M.
> > >
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/ADV7182.pdf
> > > page 68 defines bits 0-3 as reserved, and "set to default" which is
> > > 0100b.
> > > https://www.analog.com/media/en/technical-documentation/user-guides/ADV7280_7281_7282_7283_UG-637.pdf
> > > page 70 says the same for ADV7280/ADV7281/ADV7282/ADV7283.
> > >
> > > So no name or detail in the docs over what the bits do.
> > >
> > > The patch does mean the driver more closely follows the datasheet, so
> > > it looks good to me.
> > >
> > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > >
> > > I'll try to find a couple of minutes to get my hardware out and
> > > confirm I see the change in video noise.
> >
> > Great, Is there any way we can identify (easily?) if this is introducing
> > noise reduction, or preventing noise being added?
> >
> > If it's introducing noise reduction, as a feature, that's quite
> > different to causing noise if it's not set ... (Unless perhaps people
> > have a desire to add noise :D)
> 
> OK, I dug out my hardware. I'm doing the nasty with:
> i2ctransfer -y -f 10 w2@0x21 0x02 0x84
> and
> i2ctransfer -y -f 10 w2@0x21 0x02 0x80
> to flip back and forth between the two settings on my PAL source.
> 
> It does reduce the noise, but also softens the image significantly.
> 
> As slightly iffy photos to show the difference
> https://photos.app.goo.gl/hLKxv3TP93gX864y8 is the new setting.
> https://photos.app.goo.gl/sWxEhdvxHLUkGL1C8 is the old setting.
> (Yes it's a very old F1 race that happened to be on this DVD/HDD recorder).

It's a really tough test case, as I expect these are frame captures from
two separate time points, rather than some paused frame, but I would say
the text on the old setting is clearer. That could easily be due to
differences in the actual content though.

It might make me think this should be a control that could be
dynamically set to allow the user to decide...

Benjamin, how does it look on your system? I presume setting this bit
improves image quality for your use case.


> I couldn't honestly say I prefer one over the other (analogue video
> really is grim!), but it does mean that we're following the datasheet
> more accurately, so:
> 
> Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> > But I think I could add this already:
> >
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

From a driver perspective, with no other existing expecatation - I would
say matching the datasheet is the correct thing to do anyway.

--
Kieran



> >
> > >
> > >   Dave
> > >
> > > >  #define ADV7180_REG_OUTPUT_CONTROL                     0x0003
> > > >  #define ADV7180_REG_EXTENDED_OUTPUT_CONTROL            0x0004
> > > > @@ -1014,7 +1015,9 @@ 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);
> > > > +       /* Failing to set the reserved bit can result in increased video noise */
> > > > +       return adv7180_write(state, ADV7182_REG_INPUT_VIDSEL,
> > > > +                            (std << 4) | ADV7182_REG_INPUT_RESERVED);
> > > >  }
> > > >
> > > >  enum adv7182_input_type {
> > > > --
> > > > 2.36.1
> > > >

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

[Dave Stevenson]

On Wed, 18 May 2022 at 00:08, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Dave Stevenson (2022-05-17 18:50:11)
> > On Fri, 13 May 2022 at 11:43, Kieran Bingham
> > <kieran.bingham@ideasonboard.com> wrote:
> > >
> > > Quoting Dave Stevenson (2022-05-12 14:56:45)
> > > > Hi Benjamin.
> > > >
> > > > On Thu, 12 May 2022 at 13:11, Benjamin Marty <info@benjaminmarty.ch> wrote:
> > > > >
> > > > > 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>
> > > > > ---
> > > > > version 2:
> > > > > - Fixed Kieran's remarks
> > > > >
> > > > >  drivers/media/i2c/adv7180.c | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > > > > index 4f5db195e66d..992111fe249e 100644
> > > > > --- a/drivers/media/i2c/adv7180.c
> > > > > +++ b/drivers/media/i2c/adv7180.c
> > > > > @@ -43,6 +43,7 @@
> > > > >  #define ADV7180_INPUT_CONTROL_INSEL_MASK               0x0f
> > > > >
> > > > >  #define ADV7182_REG_INPUT_VIDSEL                       0x0002
> > > > > +#define ADV7182_REG_INPUT_RESERVED                     BIT(2)
> > > >
> > > > Responding to Kieran's comment on V1:
> > > > > 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)
> > > >
> > > > I went digging through the datasheet for this info as I care about
> > > > ADV728[0|1|2]M.
> > > >
> > > > https://www.analog.com/media/en/technical-documentation/data-sheets/ADV7182.pdf
> > > > page 68 defines bits 0-3 as reserved, and "set to default" which is
> > > > 0100b.
> > > > https://www.analog.com/media/en/technical-documentation/user-guides/ADV7280_7281_7282_7283_UG-637.pdf
> > > > page 70 says the same for ADV7280/ADV7281/ADV7282/ADV7283.
> > > >
> > > > So no name or detail in the docs over what the bits do.
> > > >
> > > > The patch does mean the driver more closely follows the datasheet, so
> > > > it looks good to me.
> > > >
> > > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > >
> > > > I'll try to find a couple of minutes to get my hardware out and
> > > > confirm I see the change in video noise.
> > >
> > > Great, Is there any way we can identify (easily?) if this is introducing
> > > noise reduction, or preventing noise being added?
> > >
> > > If it's introducing noise reduction, as a feature, that's quite
> > > different to causing noise if it's not set ... (Unless perhaps people
> > > have a desire to add noise :D)
> >
> > OK, I dug out my hardware. I'm doing the nasty with:
> > i2ctransfer -y -f 10 w2@0x21 0x02 0x84
> > and
> > i2ctransfer -y -f 10 w2@0x21 0x02 0x80
> > to flip back and forth between the two settings on my PAL source.
> >
> > It does reduce the noise, but also softens the image significantly.
> >
> > As slightly iffy photos to show the difference
> > https://photos.app.goo.gl/hLKxv3TP93gX864y8 is the new setting.
> > https://photos.app.goo.gl/sWxEhdvxHLUkGL1C8 is the old setting.
> > (Yes it's a very old F1 race that happened to be on this DVD/HDD recorder).
>
> It's a really tough test case, as I expect these are frame captures from
> two separate time points, rather than some paused frame, but I would say
> the text on the old setting is clearer. That could easily be due to
> differences in the actual content though.

Yes the frames are from a few seconds apart as I thought I'd left the
remote at home (actually I hadn't). I'll recapture them later today
for the same frame from a better source (probably a DVD).
The current settings are sharper, but significantly noisier.

> It might make me think this should be a control that could be
> dynamically set to allow the user to decide...
>
> Benjamin, how does it look on your system? I presume setting this bit
> improves image quality for your use case.
>
>
> > I couldn't honestly say I prefer one over the other (analogue video
> > really is grim!), but it does mean that we're following the datasheet
> > more accurately, so:
> >
> > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > > But I think I could add this already:
> > >
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> From a driver perspective, with no other existing expecatation - I would
> say matching the datasheet is the correct thing to do anyway.

That's my view too.

  Dave

> --
> Kieran
>
>
>
> > >
> > > >
> > > >   Dave
> > > >
> > > > >  #define ADV7180_REG_OUTPUT_CONTROL                     0x0003
> > > > >  #define ADV7180_REG_EXTENDED_OUTPUT_CONTROL            0x0004
> > > > > @@ -1014,7 +1015,9 @@ 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);
> > > > > +       /* Failing to set the reserved bit can result in increased video noise */
> > > > > +       return adv7180_write(state, ADV7182_REG_INPUT_VIDSEL,
> > > > > +                            (std << 4) | ADV7182_REG_INPUT_RESERVED);
> > > > >  }
> > > > >
> > > > >  enum adv7182_input_type {
> > > > > --
> > > > > 2.36.1
> > > > >

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

[Benjamin Marty]

Am Mi., 18. Mai 2022 um 01:09 Uhr schrieb Kieran Bingham
<kieran.bingham@ideasonboard.com>:
> Benjamin, how does it look on your system? I presume setting this bit
> improves image quality for your use case.

Yes, it fixes the analog noise/grain issue. I have noticed too that
the image is getting softer, as described by Dave.

Comparison Images from my side (Not the exact same frame):
https://drive.google.com/file/d/1gdwKUGb7GcvMVJG0uSomhMPeSfDwXId-
https://drive.google.com/file/d/1oitfhl4txzxOabI-TplpsKMqP2mr0TKy

> From a driver perspective, with no other existing expecatation - I would
> say matching the datasheet is the correct thing to do anyway.

I'm also agreeing on the driver should be complying with the
Datasheet. I think it's a "Bug" and not a "Feature" that the Image
gets sharpened but more grainy when not setting this bit.

Furthermore, I have opened a Ticket at ADV to get more details, but
I'm not really expecting a proper Answer.

Benjamin

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

[Dave Stevenson]

On Wed, 18 May 2022 at 13:57, Benjamin Marty <info@benjaminmarty.ch> wrote:
>
> Am Mi., 18. Mai 2022 um 01:09 Uhr schrieb Kieran Bingham
> <kieran.bingham@ideasonboard.com>:
> > Benjamin, how does it look on your system? I presume setting this bit
> > improves image quality for your use case.
>
> Yes, it fixes the analog noise/grain issue. I have noticed too that
> the image is getting softer, as described by Dave.
>
> Comparison Images from my side (Not the exact same frame):
> https://drive.google.com/file/d/1gdwKUGb7GcvMVJG0uSomhMPeSfDwXId-
> https://drive.google.com/file/d/1oitfhl4txzxOabI-TplpsKMqP2mr0TKy

More exciting images from me, and they are from the same frame (DVD paused)
https://photos.app.goo.gl/ztbW6uf6C8AguVnz7 for some stills from Tom &
Jerry, and the Fantastic 4.
The first image in each case is the register at 0x84 (new setting),
and the second as 0x80 (old setting).

The final images from Fantastic 4 are the most telling. The old
setting produces a lot of crawling noise on the land areas of the
earth, whilst with the new setting that is almost totally gone, so on
that basis I'd vote for the new.

I should add that I am using the I2P block in the chip to deinterlace
the source. From what I understand of the I2P it's a pretty simple
line doubling so that won't overly help image quality, but shouldn't
make a difference in this case.

  Dave

> > From a driver perspective, with no other existing expecatation - I would
> > say matching the datasheet is the correct thing to do anyway.
>
> I'm also agreeing on the driver should be complying with the
> Datasheet. I think it's a "Bug" and not a "Feature" that the Image
> gets sharpened but more grainy when not setting this bit.
>
> Furthermore, I have opened a Ticket at ADV to get more details, but
> I'm not really expecting a proper Answer.
>
> Benjamin