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