Hi Kieran,
On 2022-10-10 14:53:12 +0100, Kieran Bingham wrote:
> Hi Niklas,
>
> Your other patch for adv748x has the prefix:
>
> media: i2c: adv748x:
>
> I'm not sure if the i2c: is required though.
> There seems to be a mix of usage in the driver already so I don't think
> it matters too much.
Me neither, so I use both in an random pattern to not discriminate.
Let me know what you prefer and I will try to use that in the future.
>
> Quoting Niklas Söderlund (2022-10-09 15:41:46)
> > When moving the input selection to adv748x_reset() it was missed that
> > during probe the device is reset _before_ the initialization and parsing
> > of DT by the AFE subdevice. This can lead to the wrong input port (in
> > case it's not port 0) being selected until the device is reset for the
> > first time.
> >
> > Fix this by restoring the call to adv748x_afe_s_input() in the AFE
> > initialization while also keeping it in the adv748x_reset().
> >
> > Fixes: c30ed81afe89 ("media: adv748x: afe: Select input port when device is reset")
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/adv748x/adv748x-afe.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
> > index 02eabe10ab97..00095c7762c2 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> > @@ -521,6 +521,10 @@ int adv748x_afe_init(struct adv748x_afe *afe)
> > }
> > }
> >
> > + adv748x_afe_s_input(afe, afe->input);
> > +
> > + adv_dbg(state, "AFE Default input set to %d\n", afe->input);
>
> That's now two places with this message. But we likely can't put it in
> adv748x_afe_s_input(), as that isn't always setting the default, but the
> current...
>
> And it's probably not worth an extra function just to dedup a single
> debug line :S
>
> So - it seems reasonable to me.
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>
> Though does this now mean that a normal boot will show (with debug
> enabled)
>
> AFE Default input set to 0
> ...
> AFE Default input set to 0
>
> ?
Yes, but if the default input is not 0 after paring DT it will be,
AFE Default input set to 0
...
AFE Default input set to 2
Where the first one is form the reset and the second one from the probe
after we have parsed DT. Not sure how much effort we should put in to
resolving this, I'm happy with anything, keeping both or dropping any or
both of them.
Let me know what you think. And if you wish me to spin a v2. I do like
to get his fixed without to much delay as it kills my new automatic test
setup without an ugly quirk :-)
> --
> Kieran
>
>
>
>
> > +
> > /* Entity pads and sinks are 0-indexed to match the pads */
> > for (i = ADV748X_AFE_SINK_AIN0; i <= ADV748X_AFE_SINK_AIN7; i++)
> > afe->pads[i].flags = MEDIA_PAD_FL_SINK;
> > --
> > 2.37.3
> >
--
Kind Regards,
Niklas Söderlund
