[PATCH] media: adv748x: afe: Select input port when initializing AFE

[PATCH] media: adv748x: afe: Select input port when initializing AFE

[Niklas Söderlund]

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@ragnatech.se>
---
 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);
+
 	/* 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

Re: [PATCH] media: adv748x: afe: Select input port when initializing AFE

[Kieran Bingham]

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.

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@ragnatech.se>
> ---
>  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@ideasonboard.com>

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

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

Re: [PATCH] media: adv748x: afe: Select input port when initializing AFE

[Niklas Söderlund]

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@ragnatech.se>
> > ---
> >  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@ideasonboard.com>
> 
> 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

Re: [PATCH] media: adv748x: afe: Select input port when initializing AFE

[Kieran Bingham]

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@ragnatech.se>
> > > ---
> > >  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@ideasonboard.com>
> > 
> > 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