diff mbox series

[2/3] ASoC: tlv320aic3x: use BCLK instead of MCLK if not in master mode

Message ID 20230705190324.355282-3-andreas@kemnade.info
State New
Headers
Series ARM: omap4: embt2ws: Add audio support |

Commit Message

Andreas Kemnade July 5, 2023, 7:03 p.m. UTC 
  Required to have audio output on Epson Moverio BT-200.
Audio chip there is marked with AC31051. Audio output is silent there
without that clock register set.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 sound/soc/codecs/tlv320aic3x.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Andreas Kemnade July 5, 2023, 7:56 p.m. UTC | #1 
On Wed, 5 Jul 2023 20:21:58 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Wed, Jul 05, 2023 at 09:03:23PM +0200, Andreas Kemnade wrote:
> 
> > +	/* probably no mclk if not master, so rely on bitclk */
> > +	if (!aic3x->master)
> > +		clk_id = 2;
> > +  
> 
> This is fairly clearly a massive hack, we're just silently ignoring the
> clock we were asked to configure and choosing another one which is
> likely at a different rate to that we were expecting and sadly the
> driver didn't provide an automatic mode due to how old it is.  We also
> appear to try to use the configured clock rate during PLL setup which
> still happens in hw_params() even with this change which is a bit of a
> concern here.  Are you sure hw_params ends up doing the right thing, and
> that there are no other systems that get broken by this (perhaps ones
> sending a lower BCLK for example)?

Yes, I am not that happy myself with that one. Possible victim is
keystone-k2g-evm.dts. 
I looked for if (master) things and found the pll enable/disable connected to it.
But that is not the whole story...
> 
> It would be nicer to set the clock via the DT bindings, ideally with the
> clock bindings...

I found no path from these simple-audio-card things to provide a clk_id 
to set_dai_sysclk. I would of course prefer such a thing. Do I have overlooked
something?

Regards,
Andreas
Mark Brown July 6, 2023, 12:02 p.m. UTC | #2 
On Wed, Jul 05, 2023 at 09:56:11PM +0200, Andreas Kemnade wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > It would be nicer to set the clock via the DT bindings, ideally with the
> > clock bindings...

> I found no path from these simple-audio-card things to provide a clk_id 
> to set_dai_sysclk. I would of course prefer such a thing. Do I have overlooked
> something?

Since we already have clock bindings we should use those to configure
the clocks, there's several drivers that have added this support already
- look for clock providers.
Andreas Kemnade July 8, 2023, 1:03 p.m. UTC | #3 
Hi,

On Thu, 6 Jul 2023 13:02:36 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Wed, Jul 05, 2023 at 09:56:11PM +0200, Andreas Kemnade wrote:
> > Mark Brown <broonie@kernel.org> wrote:  
> 
> > > It would be nicer to set the clock via the DT bindings, ideally with the
> > > clock bindings...  
> 
> > I found no path from these simple-audio-card things to provide a clk_id 
> > to set_dai_sysclk. I would of course prefer such a thing. Do I have overlooked
> > something?  
> 
> Since we already have clock bindings we should use those to configure
> the clocks, there's several drivers that have added this support already
> - look for clock providers.

ok, looking around:
Just to make sure I am not running in a bad direction: Do you think
tlv320aic32x4{,-clk}.c is a good example? It is ignoring clk_id. 
I was mentally bound to have to use clk_id there, so I did not found a good
solution.

So I guess I have to configure the chip as a master and using mclk and compare
register dumps with the state we have now and the state after the changes,
additionally to check bclk functionality directly.

Regards,
Andreas
Mark Brown July 10, 2023, 4:36 p.m. UTC | #4 
On Sat, Jul 08, 2023 at 03:03:19PM +0200, Andreas Kemnade wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > Since we already have clock bindings we should use those to configure
> > the clocks, there's several drivers that have added this support already
> > - look for clock providers.

> ok, looking around:
> Just to make sure I am not running in a bad direction: Do you think
> tlv320aic32x4{,-clk}.c is a good example? It is ignoring clk_id. 
> I was mentally bound to have to use clk_id there, so I did not found a good
> solution.

Yes, that looks like a good example - the whole thing here is to move
away from using clk_id and to using the clock API to specify clocks.

> So I guess I have to configure the chip as a master and using mclk and compare
> register dumps with the state we have now and the state after the changes,
> additionally to check bclk functionality directly.

You can probably get away with less but it's goot to be thorough.
diff mbox series

Patch

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index 56e795a00e22f..87929e210b55e 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -1234,6 +1234,10 @@  static int aic3x_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 	struct snd_soc_component *component = codec_dai->component;
 	struct aic3x_priv *aic3x = snd_soc_component_get_drvdata(component);
 
+	/* probably no mclk if not master, so rely on bitclk */
+	if (!aic3x->master)
+		clk_id = 2;
+
 	/* set clock on MCLK or GPIO2 or BCLK */
 	snd_soc_component_update_bits(component, AIC3X_CLKGEN_CTRL_REG, PLLCLK_IN_MASK,
 				clk_id << PLLCLK_IN_SHIFT);