ASoC: simple-card, fsl-sai, ak4458, imx-ak4458
Greetings,
late last year I worked on a project using $SUBJECT. That has been suspended at least for now, so I'm writing these notes to my future self, or anyone else who may want to tread the same path. I'm happy to expand on explanation, or work on patches if anyone is interested.
Background:
Normally, fsl-sai is used with the platform-specific imx-ak4458 (afaik only in vendor tree).
However, our project wished to be able use pcm1789 which has no imx specific driver so I started trying to use simple-card with the generic ak4458 driver. I encountered a some of problems doing this:
------------------------------------------------------------------------ 1) Reset polarity of ak4458.
When imx-ak4458 is used, the platform driver handles the codec reset specified in DT compatible = "fsl,imx-audio-ak4458"; ak4458,pdn-gpio = <&gpio4 20 GPIO_ACTIVE_LOW>;
Used here. Afaics gpio_set_value sets the raw value given, ignoring the polarity specified by the DT?
gpio_set_value_cansleep(priv->pdn_gpio, 0); usleep_range(1000, 2000); gpio_set_value_cansleep(priv->pdn_gpio, 1); usleep_range(1000, 2000);
The codec driver reset code is not used.
When simple-card is used, the codec driver handles reset, specified like: compatible = "asahi-kasei,ak4458"; reset-gpios = <&gpio4 20 GPIO_ACTIVE_LOW>;
and used here (inverse for power on) static void ak4458_power_off(struct ak4458_priv *ak4458) { if (ak4458->reset_gpiod) { gpiod_set_value_cansleep(ak4458->reset_gpiod, 0); usleep_range(1000, 2000); } }
My understanding of gpiod functions is that value being set is the *logical* value of the GPIO. I.e. setting an active low gpio to 0 will set the hardware pin high. So it appears that the power_off and power_on functions are doing the opposite of what is intended. This is borne out by my hardware working correctly when the
------------------------------------------------------------------------ 2) Clock rate setting with simple-card
When simple-card is used and DT specifies mclk fs: simple-audio-card,mclk-fs = <256>;
asoc_simple_hw_params() calls snd_soc_dai_set_sysclk(..., clk_id=0, ...)
The hard-coded clk_id=0 doesn't work with fsl-sai, which requires clk_id==1.
For my testing purposes I changed the hard-coded value, but I think the proper solution could be to add a DT property to specify the clk_id (default=0) ?
------------------------------------------------------------------------ 3) Memory mapped stream access by aplay does not work. This precludes use of alsa plugins e.g. dmix
I have found no reason or solution for this so far
------------------------------------------------------------------------ 4) Unable to get multiple source clocks working with fsl-sai
With a single assigned-clock, switching between 48k and 44k1 clock rate families is accomplished by reparenting the root clock to the appropriate audio pll clock.
&sai2 { assigned-clock-parents = <&clk IMX8MM_AUDIO_PLL1_OUT>; assigned-clocks = <&clk IMX8MM_CLK_SAI2>; assigned-clock-rates = <12288000>; ... };
However if two of the sai mclks could be set to 48k and 44k1 derived rates respectively, the clock reparenting would not be required, and fsl_sai_set_bclk() would search the mclks and choose the appropriate mclk for the requested rate.
DT would be something like this:
&sai2 { assigned-clock-parents = <&clk IMX8MM_AUDIO_PLL1_OUT>, <&clk IMX8MM_AUDIO_PLL2_OUT>; assigned-clocks = <&clk IMX8MM_CLK_SAI2>, <&clk IMX8MM_CLK_SAI1>; assigned-clock-rates = <12288000>, <11289600>; };
This setup doesn't work as I hoped it would, don't know why not.
regards
On Wed, Jan 20, 2021 at 12:41:18PM +1300, Eliot Blennerhassett wrote:
It would be a bit easier to have one discussion per mail rather than mixing several different topics in a single mail, you should also CC the maintainers for the relevant drivers so they can comment on any issues in their drivers. I've copied in a bunch of people for the Freescale drivers and gpiolib.
- Reset polarity of ak4458.
When imx-ak4458 is used, the platform driver handles the codec reset specified in DT compatible = "fsl,imx-audio-ak4458"; ak4458,pdn-gpio = <&gpio4 20 GPIO_ACTIVE_LOW>;
Used here. Afaics gpio_set_value sets the raw value given, ignoring the polarity specified by the DT?
This is supposed to be handled by gpiolib I thought, I don't know off the top of my head if you need to convert the driver to use descriptors rather than numbers for that to happen. It's obviously not ideal if we force all drivers to implement custom inversion logic.
- Clock rate setting with simple-card
When simple-card is used and DT specifies mclk fs: simple-audio-card,mclk-fs = <256>;
asoc_simple_hw_params() calls snd_soc_dai_set_sysclk(..., clk_id=0, ...)
The hard-coded clk_id=0 doesn't work with fsl-sai, which requires clk_id==1.
For my testing purposes I changed the hard-coded value, but I think the proper solution could be to add a DT property to specify the clk_id (default=0) ?
That would make the clock an ABI. If it's getting to the point of needing to specify multiple clocks then we really should be looking at using the clock bindings to specify which one to use, that's a bit involved though.
- Memory mapped stream access by aplay does not work.
This precludes use of alsa plugins e.g. dmix
I have found no reason or solution for this so far
In what way does it not work - what errors or other problems do you see?
- Unable to get multiple source clocks working with fsl-sai
With a single assigned-clock, switching between 48k and 44k1 clock rate families is accomplished by reparenting the root clock to the appropriate audio pll clock.
&sai2 { assigned-clock-parents = <&clk IMX8MM_AUDIO_PLL1_OUT>; assigned-clocks = <&clk IMX8MM_CLK_SAI2>; assigned-clock-rates = <12288000>; ... };
However if two of the sai mclks could be set to 48k and 44k1 derived rates respectively, the clock reparenting would not be required, and fsl_sai_set_bclk() would search the mclks and choose the appropriate mclk for the requested rate.
DT would be something like this:
&sai2 { assigned-clock-parents = <&clk IMX8MM_AUDIO_PLL1_OUT>, <&clk IMX8MM_AUDIO_PLL2_OUT>; assigned-clocks = <&clk IMX8MM_CLK_SAI2>, <&clk IMX8MM_CLK_SAI1>; assigned-clock-rates = <12288000>, <11289600>; };
This setup doesn't work as I hoped it would, don't know why not.
regards
-- Eliot Blennerhassett
On 21/01/21 8:07 am, Mark Brown wrote:
On Wed, Jan 20, 2021 at 12:41:18PM +1300, Eliot Blennerhassett wrote:
It would be a bit easier to have one discussion per mail
taking up the topic of ak4458 reset polarity alone
ak4458.txt documents:
Optional properties: - reset-gpios: A GPIO specifier for the power down & reset pin Example reset-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>
Existing code in ak4458.c: static void ak4458_power_off(struct ak4458_priv *ak4458) { if (ak4458->reset_gpiod) { gpiod_set_value_cansleep(ak4458->reset_gpiod, 0); usleep_range(1000, 2000); } }
I suspect the value 0 represents the raw value for an active-low gpio, but this is wrong when used with gpiod_set_value_cansleep() function whose doc says "Set the logical value of the GPIO, i.e. taking its ACTIVE_LOW status into account"
Setting the value to 0 makes the GPIO *inactive* i.e. high if it is specified in the DT as ACTIVE_LOW. This is the wrong way round for the power_off/on functions.
Because the DT property is optional, perhaps nobody has tried to use it until now?
Patch to follow
-- Eliot
Reset (aka power off) happens when the reset gpio is made active. Change function name to ak4458_reset to match devicetree property "reset-gpios"
Signed-off-by: Eliot Blennerhassett eliot@blennerhassett.gen.nz --- sound/soc/codecs/ak4458.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/sound/soc/codecs/ak4458.c b/sound/soc/codecs/ak4458.c index 1010c9ee2e8..472caad1701 100644 --- a/sound/soc/codecs/ak4458.c +++ b/sound/soc/codecs/ak4458.c @@ -595,18 +595,10 @@ static struct snd_soc_dai_driver ak4497_dai = { .ops = &ak4458_dai_ops, };
-static void ak4458_power_off(struct ak4458_priv *ak4458) +static void ak4458_reset(struct ak4458_priv *ak4458, bool active) { if (ak4458->reset_gpiod) { - gpiod_set_value_cansleep(ak4458->reset_gpiod, 0); - usleep_range(1000, 2000); - } -} - -static void ak4458_power_on(struct ak4458_priv *ak4458) -{ - if (ak4458->reset_gpiod) { - gpiod_set_value_cansleep(ak4458->reset_gpiod, 1); + gpiod_set_value_cansleep(ak4458->reset_gpiod, active); usleep_range(1000, 2000); } } @@ -620,7 +612,7 @@ static int ak4458_init(struct snd_soc_component *component) if (ak4458->mute_gpiod) gpiod_set_value_cansleep(ak4458->mute_gpiod, 1);
- ak4458_power_on(ak4458); + ak4458_reset(ak4458, false);
ret = snd_soc_component_update_bits(component, AK4458_00_CONTROL1, 0x80, 0x80); /* ACKS bit = 1; 10000000 */ @@ -650,7 +642,7 @@ static void ak4458_remove(struct snd_soc_component *component) { struct ak4458_priv *ak4458 = snd_soc_component_get_drvdata(component);
- ak4458_power_off(ak4458); + ak4458_reset(ak4458, true); }
#ifdef CONFIG_PM @@ -660,7 +652,7 @@ static int __maybe_unused ak4458_runtime_suspend(struct device *dev)
regcache_cache_only(ak4458->regmap, true);
- ak4458_power_off(ak4458); + ak4458_reset(ak4458, true);
if (ak4458->mute_gpiod) gpiod_set_value_cansleep(ak4458->mute_gpiod, 0); @@ -685,8 +677,8 @@ static int __maybe_unused ak4458_runtime_resume(struct device *dev) if (ak4458->mute_gpiod) gpiod_set_value_cansleep(ak4458->mute_gpiod, 1);
- ak4458_power_off(ak4458); - ak4458_power_on(ak4458); + ak4458_reset(ak4458, true); + ak4458_reset(ak4458, false);
regcache_cache_only(ak4458->regmap, false); regcache_mark_dirty(ak4458->regmap);
Hi Eliot,
thanks for your patch!
On Fri, Jan 22, 2021 at 9:27 AM Eliot Blennerhassett eliot@blennerhassett.gen.nz wrote:
Reset (aka power off) happens when the reset gpio is made active. Change function name to ak4458_reset to match devicetree property "reset-gpios"
Signed-off-by: Eliot Blennerhassett eliot@blennerhassett.gen.nz
(...)
-static void ak4458_power_off(struct ak4458_priv *ak4458) +static void ak4458_reset(struct ak4458_priv *ak4458, bool active)
I usually use the variable name "asserted" to be crystal clear as to what this is about.
With that change: Reviewed-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
On Fri, 22 Jan 2021 21:27:08 +1300, Eliot Blennerhassett wrote:
Reset (aka power off) happens when the reset gpio is made active. Change function name to ak4458_reset to match devicetree property "reset-gpios"
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/1] ASoC: ak4458: correct reset polarity commit: e953daeb68b1abd8a7d44902786349fdeef5c297
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (3)
-
Eliot Blennerhassett
-
Linus Walleij
-
Mark Brown