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. 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. -- 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 > > > >
