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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 18 May 2022 at 00:08, Kieran Bingham
<kieran.bingham@xxxxxxxxxxxxxxxx> wrote:
>
> Quoting Dave Stevenson (2022-05-17 18:50:11)
> > On Fri, 13 May 2022 at 11:43, Kieran Bingham
> > <kieran.bingham@xxxxxxxxxxxxxxxx> wrote:
> > >
> > > Quoting Dave Stevenson (2022-05-12 14:56:45)
> > > > Hi Benjamin.
> > > >
> > > > On Thu, 12 May 2022 at 13:11, Benjamin Marty <info@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxxx>
> >
> > > But I think I could add this already:
> > >
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>
> 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
> > > > >



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux