Quoting Niklas Söderlund (2022-10-10 15:08:49)
> 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.
Doesn't bother me - That's up to whoever picks the patch I think ;-)
> > 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 :-)
Doesn't bother me here I don't think. Perhaps a bit odd that the default
changes, but I suspect that fixing it to only have one would require
refactoring the ordering of how probe/reset is occuring?
You could only change/report the default as changed if it differs too
... but even that's maybe an over-optimisation.
--
Kieran
>
> > --
> > 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
