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