This patch series is mostly Freescale's SAI SoC Digital Audio Interface driver implementation. And the implementation is only compatible with device tree definition.
This patch series is based on linux-next and has been tested on Vybrid VF610 Tower board using device tree.
Added in v1:
- Add SAI SoC Digital Audio Interface driver.
- Add Freescale SAI ALSA SoC Digital Audio Interface node for VF610.
- Enables SAI ALSA SoC DAI device for Vybrid VF610 TOWER board.
- Add device tree bindings for Freescale SAI.
- Revise the bugs about the sgt15000 codec.
- Add SGT15000 based audio machine driver.
- Enable SGT15000 codec based audio driver node for VF610.
- Add device tree bindings for Freescale VF610 sound.
This adds the Document for Freescale SAI driver under
Documentation/devicetree/bindings/sound/.
Signed-off-by: Xiubo Li <redacted>
---
.../devicetree/bindings/sound/fsl-sai.txt | 32 ++++++++++++++++++++++
1 file changed, 32 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/fsl-sai.txt
@@ -0,0 +1,32 @@+Freescale Synchronous Audio Interface (SAI).++The SAI is based on I2S module that used communicating with audio codecs,+which provides a synchronous audio interface that supports fullduplex+serial interfaces with frame synchronization such as I2S, AC97, TDM, and+codec/DSP interfaces.+++Required properties:+- compatible: Compatible list, contains "fsl,vf610-sai".+- reg: Offset and length of the register set for the device.+- clocks: Must contain an entry for each entry in clock-names.+- clock-names : Must include the "sai" entry.+- dmas : Generic dma devicetree binding as described in+ Documentation/devicetree/bindings/dma/dma.txt.+- dma-names : Two dmas have to be defined, "tx" and "rx".+- pinctrl-names: Must contain a "default" entry.+- pinctrl-NNN: One property must exist for each entry in pinctrl-names.+ See ../pinctrl/pinctrl-bindings.txt for details of the property values.++Example:+sai2: sai@40031000 {+ compatible = "fsl,vf610-sai";+ reg = <0x40031000 0x1000>;+ pinctrl-names = "default";+ pinctrl-0 = <&pinctrl_sai2_1>;+ clocks = <&clks VF610_CLK_SAI2>;+ clock-names = "sai";+ dma-names = "tx", "rx";+ dmas = <&edma0 0 VF610_EDMA_MUXID0_SAI2_TX>,+ <&edma0 0 VF610_EDMA_MUXID0_SAI2_RX>;+};
When the CONFIG_REGULATOR is disabled there will be some warnings
printed out.
Signed-off-by: Xiubo Li <redacted>
---
sound/soc/codecs/sgtl5000.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
@@ -883,14 +883,19 @@ static int ldo_regulator_register(struct snd_soc_codec *codec,structregulator_init_data*init_data,intvoltage){+#ifdef CONFIG_SND_SOC_FSL_SGTL5000+return0;+#elsedev_err(codec->dev,"this setup needs regulator support in the kernel\n");return-EINVAL;+#endif}staticintldo_regulator_remove(structsnd_soc_codec*codec){return0;}+#endif/*
@@ -1391,11 +1399,12 @@ static int sgtl5000_probe(struct snd_soc_codec *codec)if(ret)returnret;+#ifdef CONFIG_REGULATOR/* power up sgtl5000 */ret=sgtl5000_set_power_regs(codec);if(ret)gotoerr;-+#endif/* enable small pop, introduce 400ms delay in turning off */snd_soc_update_bits(codec,SGTL5000_CHIP_REF_CTRL,SGTL5000_SMALL_POP,
This is the SGTl5000 codec based audio driver supported with both
playback and capture dai link implemention.
This implementation is only compatible with device tree definition.
Signed-off-by: Alison Wang <b18965@freescale.com
Signed-off-by: Xiubo Li <redacted>
---
sound/soc/fsl/Kconfig | 10 +++
sound/soc/fsl/Makefile | 2 +
sound/soc/fsl/fsl-sgtl5000.c | 208 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 220 insertions(+)
create mode 100644 sound/soc/fsl/fsl-sgtl5000.c
@@ -220,4 +220,14 @@ config SND_SOC_FSL_PCMtristateselectSND_SOC_GENERIC_DMAENGINE_PCM+configSND_SOC_FSL_SGTL5000+tristate"SoC Audio support for FSL boards with sgtl5000"+depends onOF&&I2C+selectSND_SOC_FSL_SAI+selectSND_SOC_FSL_PCM+selectSND_SOC_SGTL5000+help+SayYifyouwanttoaddsupportforSoCaudioonanFSLboardwith+asgtl5000codec.+endif# SND_FSL_SOC
@@ -0,0 +1,208 @@+/*+*FreeacaleALSASoCAudiousingSGT1500ascodec.+*+*Copyright2012-2013FreescaleSemiconductor,Inc.+*+*ThecodecontainedhereinislicensedundertheGNUGeneralPublic+*License.YoumayobtainacopyoftheGNUGeneralPublicLicense+*Version2orlateratthefollowinglocations:+*+*/++#include<linux/module.h>+#include<linux/of.h>+#include<linux/of_platform.h>+#include<linux/i2c.h>+#include<linux/clk.h>++#include"../codecs/sgtl5000.h"+#include"fsl-sai.h"++staticunsignedintsysclk_rate;++staticintfsl_sgtl5000_dai_init(structsnd_soc_pcm_runtime*rtd)+{+intret;+structdevice*dev=rtd->card->dev;++ret=snd_soc_dai_set_sysclk(rtd->codec_dai,SGTL5000_SYSCLK,+sysclk_rate,SND_SOC_CLOCK_IN);+if(ret){+dev_err(dev,"could not set codec driver clock params :%d\n",+ret);+returnret;+}++ret=snd_soc_dai_set_sysclk(rtd->cpu_dai,FSL_SAI_CLK_BUS,+sysclk_rate,SND_SOC_CLOCK_OUT);+if(ret){+dev_err(dev,"could not set cpu dai driver clock params :%d\n",+ret);+returnret;+}++return0;+}++staticintsgtl5000_params(structsnd_pcm_substream*substream,+structsnd_pcm_hw_params*params)+{+structsnd_soc_pcm_runtime*rtd=substream->private_data;+structsnd_soc_dai*cpu_dai=rtd->cpu_dai;+unsignedintchannels=params_channels(params);++/* TODO: The SAI driver should figure this out for us */+switch(channels){+case2:+snd_soc_dai_set_tdm_slot(cpu_dai,0xfffffffc,0xfffffffc,2,0);+break;+case1:+snd_soc_dai_set_tdm_slot(cpu_dai,0xfffffffe,0xfffffffe,1,0);+break;+default:+return-EINVAL;+}++return0;+}++staticstructsnd_soc_opsfsl_sgtl5000_hifi_ops={+.hw_params=sgtl5000_params,+};++staticstructsnd_soc_dai_linkfsl_sgtl5000_dai={+.name="HiFi",+.stream_name="HiFi",+.codec_dai_name="sgtl5000",+.init=&fsl_sgtl5000_dai_init,+.dai_fmt=SND_SOC_DAIFMT_I2S|SND_SOC_DAIFMT_NB_NF|+SND_SOC_DAIFMT_CBM_CFM,+.ops=&fsl_sgtl5000_hifi_ops,+};++staticconststructsnd_soc_dapm_widgetfsl_sgtl5000_dapm_widgets[]={+SND_SOC_DAPM_MIC("Mic Jack",NULL),+SND_SOC_DAPM_LINE("Line In Jack",NULL),+SND_SOC_DAPM_HP("Headphone Jack",NULL),+SND_SOC_DAPM_SPK("Line Out Jack",NULL),+SND_SOC_DAPM_SPK("Ext Spk",NULL),+};++staticstructsnd_soc_cardfsl_sgt1500_card={+.owner=THIS_MODULE,+.num_links=1,+.dai_link=&fsl_sgtl5000_dai,+.dapm_widgets=fsl_sgtl5000_dapm_widgets,+.num_dapm_widgets=ARRAY_SIZE(fsl_sgtl5000_dapm_widgets),+};++staticintfsl_sgtl5000_parse_dt(structplatform_device*pdev)+{+intret;+structdevice_node*sai_np,*codec_np;+structclk*codec_clk;+structi2c_client*codec_dev;+structdevice_node*np=pdev->dev.of_node;++ret=snd_soc_of_parse_card_name(&fsl_sgt1500_card,"model");+if(ret)+returnret;++ret=snd_soc_of_parse_audio_routing(&fsl_sgt1500_card,+"audio-routing");+if(ret)+returnret;++sai_np=of_parse_phandle(np,"saif-controller",0);+if(!sai_np){+dev_err(&pdev->dev,"\"saif-controller\" phandle missing or "+"invalid\n");+return-EINVAL;+}+fsl_sgtl5000_dai.cpu_of_node=sai_np;+fsl_sgtl5000_dai.platform_of_node=sai_np;++codec_np=of_parse_phandle(np,"audio-codec",0);+if(!codec_np){+dev_err(&pdev->dev,"\"audio-codec\" phandle missing or "+"invalid\n");+ret=-EINVAL;+gotosai_np_fail;+}+fsl_sgtl5000_dai.codec_of_node=codec_np;++codec_dev=of_find_i2c_device_by_node(codec_np);+if(!codec_dev){+dev_err(&pdev->dev,"failed to find codec platform device\n");+ret=PTR_ERR(codec_dev);+gotocodec_np_fail;+}++codec_clk=devm_clk_get(&codec_dev->dev,NULL);+if(IS_ERR(codec_clk)){+dev_err(&pdev->dev,"failed to get codec clock\n");+ret=PTR_ERR(codec_clk);+gotocodec_np_fail;+}++sysclk_rate=clk_get_rate(codec_clk);++codec_np_fail:+of_node_put(codec_np);+sai_np_fail:+of_node_put(sai_np);++returnret;+}++staticintfsl_sgtl5000_probe(structplatform_device*pdev)+{+intret;++fsl_sgt1500_card.dev=&pdev->dev;++ret=fsl_sgtl5000_parse_dt(pdev);+if(ret){+dev_err(&pdev->dev,+"parse sgtl5000 device tree failed :%d\n",+ret);+returnret;+}++ret=snd_soc_register_card(&fsl_sgt1500_card);+if(ret){+dev_err(&pdev->dev,"register soc sound card failed :%d\n",+ret);+returnret;+}++return0;+}++staticintfsl_sgtl5000_remove(structplatform_device*pdev)+{+snd_soc_unregister_card(&fsl_sgt1500_card);++return0;+}++staticconststructof_device_idfsl_sgtl5000_dt_ids[]={+{.compatible="fsl,vf610-sgtl5000",},+{/* sentinel */}+};+MODULE_DEVICE_TABLE(of,fsl_sgtl5000_dt_ids);++staticstructplatform_driverfsl_sgtl5000_driver={+.driver={+.name="fsl-sgtl5000",+.owner=THIS_MODULE,+.of_match_table=fsl_sgtl5000_dt_ids,+},+.probe=fsl_sgtl5000_probe,+.remove=fsl_sgtl5000_remove,+};+module_platform_driver(fsl_sgtl5000_driver);++MODULE_AUTHOR("Xiubo Li <Li.Xiubo@freescale.com>");+MODULE_DESCRIPTION("Freescale SGTL5000 ASoC driver");+MODULE_LICENSE("GPL");
This patch add and enable SGT15000 codec support, and also specified
the corresponding SAI node.
Signed-off-by: Xiubo Li <redacted>
Signed-off-by: Alison Wang <b18965@freescale.com
---
arch/arm/boot/dts/vf610-twr.dts | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
@@ -0,0 +1,52 @@+Freescale VF610 audio complex with SGTL5000 codec++Required properties:+- compatible: "fsl,vf610-sgtl5000"+- model: The user-visible name of this sound complex.+- saif-controllers: The phandle list of the SAI controller.+- audio-codec: The phandle of the SGTL5000 audio codec.+- audio-routing : A list of the connections between audio components.+ Each entry is a pair of strings, the first being the connection's sink,+ the second being the connection's source. Valid names could be power+ supplies, SGTL5000 pins, and the jacks on the board:++ -- Power supplies:+ * Mic Bias++ -- SGTL5000 pins:+ * MIC_IN+ * LINE_IN+ * HP_OUT+ * LINE_OUT++ -- Board connectors:+ * Mic Jack+ * Line In Jack+ * Headphone Jack+ * Line Out Jack+ * Ext Spk++Example:++sound {+ compatible = "fsl,vf610-sgtl5000";+ model = "vf610-sgtl5000";+ saif-controller = <&sai2>;+ audio-codec = <&codec>;+ audio-routing =+ "MIC_IN", "Mic Jack",+ "Mic Jack", "Mic Bias",+ "LINE_IN", "Line In Jack",+ "Headphone Jack", "HP_OUT",+ "Ext Spk", "LINE_OUT";+};++&i2c0 {+ ...++ codec: sgtl5000@0a {+ compatible = "fsl,sgtl5000";+ reg = <0x0a>;+ clocks = <&clks VF610_CLK_SAI2>;+ };+};
The comma after the last entry in a struct initializer is there to make
patches that append another entry cleaner. Since this entry is and
always must be the last entry, the comma is useless here.
This document name is overly generic, there are more than one FSL
platforms with SGTL5000 codecs. Please include the vf610 here.
quoted hunk
@@ -0,0 +1,52 @@+Freescale VF610 audio complex with SGTL5000 codec++Required properties:+- compatible: "fsl,vf610-sgtl5000"+- model: The user-visible name of this sound complex.+- saif-controllers: The phandle list of the SAI controller.+- audio-codec: The phandle of the SGTL5000 audio codec.+- audio-routing : A list of the connections between audio components.+ Each entry is a pair of strings, the first being the connection's sink,+ the second being the connection's source. Valid names could be power+ supplies, SGTL5000 pins, and the jacks on the board:++ -- Power supplies:+ * Mic Bias++ -- SGTL5000 pins:+ * MIC_IN+ * LINE_IN+ * HP_OUT+ * LINE_OUT++ -- Board connectors:+ * Mic Jack+ * Line In Jack+ * Headphone Jack+ * Line Out Jack+ * Ext Spk++Example:++sound {+ compatible = "fsl,vf610-sgtl5000";+ model = "vf610-sgtl5000";+ saif-controller = <&sai2>;+ audio-codec = <&codec>;+ audio-routing =+ "MIC_IN", "Mic Jack",+ "Mic Jack", "Mic Bias",+ "LINE_IN", "Line In Jack",+ "Headphone Jack", "HP_OUT",+ "Ext Spk", "LINE_OUT";+};++&i2c0 {+ ...++ codec: sgtl5000@0a {+ compatible = "fsl,sgtl5000";+ reg = <0x0a>;+ clocks = <&clks VF610_CLK_SAI2>;+ };+};
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi,
On Thu, Oct 17, 2013 at 05:01:14PM +0800, Xiubo Li wrote:
When the CONFIG_REGULATOR is disabled there will be some warnings
printed out.
A little confused by the title. But after looking at the comments,
is the patch just gonna add some debug info for the case when the
CONFIG_REGULATOR's been un-selected?
Well first, I think at least the title should be more explicit.
And second, the necessity of this patch might just a little...
if CONFIG_REGULATOR is required to power it up, why not turn it on.
The inline regulator-related functions are already have REGULATOR dependency.
Is that necessary to put an additional one here?
quoted hunk
/*
* sgtl5000 has 3 internal power supplies:
* 1. VAG, normally set to vdda/2
@@ -1269,6 +1275,7 @@ static int sgtl5000_set_power_regs(struct snd_soc_codec *codec) return 0; }+#endif static int sgtl5000_replace_vddd_with_ldo(struct snd_soc_codec *codec) {
@@ -1370,6 +1377,7 @@ err_regulator_free: sgtl5000->supplies); if (external_vddd) ldo_regulator_remove(codec);+
Pls drop this.
quoted hunk
return ret;
}
@@ -1391,11 +1399,12 @@ static int sgtl5000_probe(struct snd_soc_codec *codec) if (ret) return ret;+#ifdef CONFIG_REGULATOR /* power up sgtl5000 */ ret = sgtl5000_set_power_regs(codec); if (ret) goto err;-+#endif /* enable small pop, introduce 400ms delay in turning off */ snd_soc_update_bits(codec, SGTL5000_CHIP_REF_CTRL, SGTL5000_SMALL_POP,
When the CONFIG_REGULATOR is disabled there will be some warnings
printed out.
=20
Signed-off-by: Xiubo Li <redacted>
---
sound/soc/codecs/sgtl5000.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
=20
Hi,
Xiubo Li [off-list ref] wrote:
The subject has a wrong name for the codec "SGT1..." instead of
"SGTL...", which will make it difficult to search for this thread in
mail archives or in commit messages once this patches should be applied!
Lothar Wa=C3=9Fmann
--=20
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstra=C3=9Fe 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch=C3=A4ftsf=C3=BChrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________
Because it won't check for conflicting resource regions.
Maybe I've been out of the loop for too long, but why is that a particular
problem with this driver?
It is usually something you'd want to check in general to make sure that you
don't have multiple device that access the same iomem region at the same time.
- Lars
From: Timur Tabi <hidden> Date: 2013-10-17 13:37:30
Lars-Peter Clausen wrote:
quoted
quoted
Maybe I've been out of the loop for too long, but why is that a particular
problem with this driver?
It is usually something you'd want to check in general to make sure that you
don't have multiple device that access the same iomem region at the same time.
I understand that, but I'm trying to figure out why of_iomap() is okay
for hundreds of other drivers, but not this one. I've used it dozens of
times myself, without ever worrying about overlapping regions.
Maybe I've been out of the loop for too long, but why is that a particular
problem with this driver?
quoted
It is usually something you'd want to check in general to make sure that you
don't have multiple device that access the same iomem region at the same
time.
I understand that, but I'm trying to figure out why of_iomap() is okay for
hundreds of other drivers, but not this one. I've used it dozens of times
myself, without ever worrying about overlapping regions.
The driver would work fine with just of_iomap(). But the resource range
check comes basically for free and it does help to catch errors, so I'd
recommend on using it rather than not using it.
- Lars
From: Mark Brown <broonie@kernel.org> Date: 2013-10-17 14:11:22
On Thu, Oct 17, 2013 at 03:51:54PM +0200, Lars-Peter Clausen wrote:
On 10/17/2013 03:37 PM, Timur Tabi wrote:
quoted
I understand that, but I'm trying to figure out why of_iomap() is okay for
hundreds of other drivers, but not this one. I've used it dozens of times
myself, without ever worrying about overlapping regions.
The driver would work fine with just of_iomap(). But the resource range
check comes basically for free and it does help to catch errors, so I'd
recommend on using it rather than not using it.
There's also the fact that it's a devm_ function which means less error
handling code that we can break which is nice. There's probably a case
for an improved OF helper here...
I understand that, but I'm trying to figure out why of_iomap() is
okay for hundreds of other drivers, but not this one. I've used it
dozens of times myself, without ever worrying about overlapping
regions.
=20
quoted
The driver would work fine with just of_iomap(). But the resource
range check comes basically for free and it does help to catch errors,
so I'd recommend on using it rather than not using it.
=20
There's also the fact that it's a devm_ function which means less error
handling code that we can break which is nice. There's probably a case
for an improved OF helper here...
Using this instead of of_iomap() is because "devm_" and resource range chec=
k
as Lars and Mark said, and there are more than one SAI device here which wi=
ll
be added later, maybe the resource range check is needed.
Thanks.
--
BRS
Xiubo
From: Mark Brown <broonie@kernel.org> Date: 2013-10-18 17:28:55
On Thu, Oct 17, 2013 at 05:01:14PM +0800, Xiubo Li wrote:
quoted hunk
@@ -883,14 +883,19 @@ static int ldo_regulator_register(struct snd_soc_codec *codec, struct regulator_init_data *init_data, int voltage) {+#ifdef CONFIG_SND_SOC_FSL_SGTL5000+ return 0;+#else dev_err(codec->dev, "this setup needs regulator support in the kernel\n"); return -EINVAL;+#endif }
If these systems don't actually need the internal regulator then should
they not be trying to enable it? Alternatively if it's OK to ignore
this then why is this conditional in the board?
If this is something that it's safe to ignore then it should either be
ignored all the time or should be controlled by platform data not by a
compile time #define.
Things that are part of the CODEC should be part of the CODEC binding
and this binding should reference that - this way the information
doesn't have to be replicated by all boards using the CODEC and if new
devices are supported by the CODEC driver then only that needs updating
hopefully.
When the CONFIG_REGULATOR is disabled there will be some warnings
printed out.
=20
A little confused by the title. But after looking at the comments, is the
patch just gonna add some debug info for the case when the
CONFIG_REGULATOR's been un-selected?
=20
Well first, I think at least the title should be more explicit.
And second, the necessity of this patch might just a little...
if CONFIG_REGULATOR is required to power it up, why not turn it on.
=20
Sorry, I will add some more detail and explicit description about this patc=
h.
In VF610 board there has not Power Manager module. So if the CONFIG_REGULAT=
OR is
turned on the SGTL5000 cannot be brought up correctly.
If it's turned off there will also some other errors for the SGTL5000 codec=
driver=20
using the CONFIG_REGULATOR mirco not very correctly.
=20
Same here, shouldn't tx and rx be started independently depending on the
substream direction?
=20
But this couldn't, from the SAI's spec we can see that:
------
The SAI transmitter and receiver can be configured to operate with synchron=
ous bit clock
and frame sync.
1),=20
If the transmitter bit clock and frame sync are to be used by both the tran=
smitter and
receiver:
* The transmitter must be configured for asynchronous operation and the rec=
eiver for
synchronous operation.
* In synchronous mode, the receiver is enabled only when both the transmitt=
er and
receiver are enabled.
* It is recommended that the transmitter is the last enabled and the first =
disabled.
2),
If the receiver bit clock and frame sync are to be used by both the transmi=
tter and
receiver:
* The receiver must be configured for asynchronous operation and the transm=
itter for
synchronous operation.
* In synchronous mode, the transmitter is enabled only when both the receiv=
er and
transmitter are both enabled.
* It is recommended that the receiver is the last enabled and the first dis=
abled.
------
The receiver and transmitter should be both enabled at the same time if any=
of them is alive.
=20
Things that are part of the CODEC should be part of the CODEC binding and
this binding should reference that - this way the information doesn't
have to be replicated by all boards using the CODEC and if new devices
are supported by the CODEC driver then only that needs updating hopefully=
.
Yes, the "-- SGTL5000 pins:" should be in the CODEC binding.
But, actually the CODEC binding hasn't any reference about this.
So I added it here, but not very sure.
@@ -883,14 +883,19 @@ static int ldo_regulator_register(struct
snd_soc_codec *codec,
quoted
struct regulator_init_data *init_data,
int voltage)
{
+#ifdef CONFIG_SND_SOC_FSL_SGTL5000
+ return 0;
+#else
dev_err(codec->dev, "this setup needs regulator support in the
kernel\n");
quoted
return -EINVAL;
+#endif
This looks wrong to me, as this will disable the error for unsolicited
platforms in a multi arch kernel!
=20
=20
The CONFIG_SND_SOC_FSL_SGTL5000 micro will be renamed to CONFIG_SND_SOC_F=
SL_SGTL5000_VF610.
In VF610, there has not Power Manager Module, so whether the CONFIG_REGUL=
ATOR is enable or=20
Disabled, there will always some errors booting...
=20
Yes, but you are altering code that may be run on a different machine
than VF610 in a multiarch kernel! You should have a RUNTIME check for
the machine type if you need to do machine type specific stuff.
Lothar Wa=C3=9Fmann
--=20
___________________________________________________________
Ka-Ro electronics GmbH | Pascalstra=C3=9Fe 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch=C3=A4ftsf=C3=BChrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________
From: Timur Tabi <hidden> Date: 2013-10-21 11:21:03
Xiubo Li-B47053 wrote:
The CONFIG_SND_SOC_FSL_SGTL5000 micro will be renamed to CONFIG_SND_SOC_FSL_SGTL5000_VF610.
In VF610, there has not Power Manager Module, so whether the CONFIG_REGULATOR is enable or
Disabled, there will always some errors booting...
That's just not acceptable. You have to fix the code so that it works
with CONFIG_REGULATOR both set and not set.
=20
The driver should not have to manually parse the dma devicetree
properties, this is something that should be handled by the dma engine
driver.
=20
Yes, the dma engine interface has already parsed the slave_id while
the dma customer requesting one dma channel.
Though this also could be a way to pass the slave_id to dma driver, but the=
=20
dma driver uses the way while requesting dma channels.
So I'll drop this code later.
There's a patch in -next that lets the generic dmaengine code figure out
some settings from the dmacontroller rather than requiring the driver to
explicitly provide configuration - it's "ASoC: dmaengine-pcm: Provide
default config". Please update your driver to use this, or let's work
out what it doesn't do any try to fix it.
+ ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq,
+ FSL_FMT_TRANSMITTER);
+ if (ret) {
+ dev_err(cpu_dai->dev,
+ "Cannot set sai's transmitter sysclk: %d\n",
+ ret);
+ return ret;
+ }
+
+ ret = fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq,
+ FSL_FMT_RECEIVER);
As other people have commented these should be exposed as separate
clocks rather than set in sync, unless there's some hardware reason they
need to be identical. If that is the case then a comment explaining the
limitation would be good.
Similarly with several of the other functions.
It'd be a bit nicer to only enable the clock while the driver is
actively being used rather than all the time the system is powered up
but it's not a blocker for merge.
+ ret = snd_soc_register_component(&pdev->dev, &fsl_component,
+ &fsl_sai_dai, 1);
+ if (ret)
+ return ret;
There's a devm_snd_soc_register_component() in -next, please use that.
+
+ ret = fsl_pcm_dma_init(pdev);
+ if (ret)
+ goto out;
+
+ platform_set_drvdata(pdev, sai);
These should go before the driver is registered with the subsystem
otherwise you've got a race where something might try to use the driver
before init is finished.
@@ -883,14 +883,19 @@ static int ldo_regulator_register(struct
snd_soc_codec *codec,
quoted
struct regulator_init_data *init_data,
int voltage)
{
+#ifdef CONFIG_SND_SOC_FSL_SGTL5000
+ return 0;
+#else
dev_err(codec->dev, "this setup needs regulator support in the
kernel\n");
quoted
return -EINVAL;
+#endif
}
=20
If these systems don't actually need the internal regulator then should
they not be trying to enable it? =20
Yes, I think do not trying to enable the regulator is much better.
Alternatively if it's OK to ignore this then why is this conditional in th=
e board?
=20
The CONFIG_SND_SOC_FSL_SGTL5000 micro maybe confuse you and others.
And it should be CONFIG_SND_SOC_FSL_SGTL5000_VF610....
=20
There's a patch in -next that lets the generic dmaengine code figure out
some settings from the dmacontroller rather than requiring the driver to
explicitly provide configuration - it's "ASoC: dmaengine-pcm: Provide
default config". Please update your driver to use this, or let's work
out what it doesn't do any try to fix it.
I will do a research.
=20
quoted
+ ret =3D fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq,
+ FSL_FMT_TRANSMITTER);
+ if (ret) {
+ dev_err(cpu_dai->dev,
+ "Cannot set sai's transmitter sysclk: %d\n",
+ ret);
+ return ret;
+ }
+
+ ret =3D fsl_sai_set_dai_sysclk_tr(cpu_dai, clk_id, freq,
+ FSL_FMT_RECEIVER);
=20
As other people have commented these should be exposed as separate clocks
rather than set in sync, unless there's some hardware reason they need to
be identical. If that is the case then a comment explaining the
limitation would be good.
=20
Similarly with several of the other functions.
=20
As I have replied before, there is one function couldn't be separated for t=
he hardware limitation.
=20
It'd be a bit nicer to only enable the clock while the driver is actively
being used rather than all the time the system is powered up but it's not
a blocker for merge.
=20
Actully there are to "XXX_probe" functions and two "XXX_remove" functions:
fsl_sai_dai_probe() and fsl_sai_dai_remove() are callbacks of the ASoC subs=
ystem.
And in fsl_sai_dai_probe() needs to read/write the SAI controller's registe=
rs, so
the clk_enable_prepare() must be here and clk_disable_unprepare() in fsl_sa=
i_dai_remove().
fsl_sai_probe() and fsl_sai_remove() are the driver's probe and remove inte=
rfaces.
So the "+ clk_disable_unprepare(sai->clk);" sentence in fsl_sai_remove() wi=
ll be removed later.
quoted
+ ret =3D snd_soc_register_component(&pdev->dev, &fsl_component,
+ &fsl_sai_dai, 1);
+ if (ret)
+ return ret;
=20
There's a devm_snd_soc_register_component() in -next, please use that.
=20
See the next version.
quoted
+
+ ret =3D fsl_pcm_dma_init(pdev);
+ if (ret)
+ goto out;
+
+ platform_set_drvdata(pdev, sai);
=20
These should go before the driver is registered with the subsystem
otherwise you've got a race where something might try to use the driver
before init is finished.
=20
=20
There's a patch in -next that lets the generic dmaengine code figure out
some settings from the dmacontroller rather than requiring the driver to
explicitly provide configuration - it's "ASoC: dmaengine-pcm: Provide
default config". Please update your driver to use this, or let's work
out what it doesn't do any try to fix it.
=20
I couldn't find the patch in the next and other trees.
Does this patch has been submitted to the -next tree ?
Or could you tell me how to find the patch please?
Thanks very much.
On Tue, Oct 29, 2013 at 12:00:57PM +0800, Xiubo Li-B47053 wrote:
quoted
There's a patch in -next that lets the generic dmaengine code figure out
some settings from the dmacontroller rather than requiring the driver to
explicitly provide configuration - it's "ASoC: dmaengine-pcm: Provide
default config". Please update your driver to use this, or let's work
out what it doesn't do any try to fix it.
I couldn't find the patch in the next and other trees.
Does this patch has been submitted to the -next tree ?
Or could you tell me how to find the patch please?
Are you using broonie's repository?
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
If you searched the title in for-next branch, you should have found it.
Best regards,
Nicolin Chen
-----Original Message-----
From: Chen Guangyu-B42378
Sent: Tuesday, October 29, 2013 12:02 PM
To: Xiubo Li-B47053
Cc: Mark Brown; Guo Shawn-R65073; timur@tabi.org; lgirdwood@gmail.com;
Jin Zhengxiong-R64188; rob.herring@calxeda.com; pawel.moll@arm.com;
mark.rutland@arm.com; swarren@wwwdotorg.org; ian.campbell@citrix.com;
rob@landley.net; linux@arm.linux.org.uk; perex@perex.cz; tiwai@suse.de;
grant.likely@linaro.org; Estevam Fabio-R49496; LW@KARO-electronics.de;
oskar@scara.com; shawn.guo@linaro.org; Wang Huan-B18965;
devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; alsa-
devel@alsa-project.org; linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCHv1 1/8] ALSA: Add SAI SoC Digital Audio Interface
driver.
=20
On Tue, Oct 29, 2013 at 12:00:57PM +0800, Xiubo Li-B47053 wrote:
quoted
quoted
There's a patch in -next that lets the generic dmaengine code figure
out some settings from the dmacontroller rather than requiring the
driver to explicitly provide configuration - it's "ASoC:
dmaengine-pcm: Provide default config". Please update your driver
to use this, or let's work out what it doesn't do any try to fix it.
I couldn't find the patch in the next and other trees.
Does this patch has been submitted to the -next tree ?
Or could you tell me how to find the patch please?
=20
Are you using broonie's repository?
NO.
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
=20
If you searched the title in for-next branch, you should have found it.
=20
The driver should not have to manually parse the dma devicetree
properties, this is something that should be handled by the dma
engine driver.
What do you think about the DMA slave_id ?
I have been noticed by one colleague that this should be parsed here,
which is from your opinions ?
Sure slave_id can be parsed here, but IMO it should be programmed via the
dma_slave_confog into the respective channel
=20
Actually, these are parsed for cpu_dai->playback_dma_data and cpu_dai->capt=
ure_dma_data dynamically, whose type is struct dma_slave_config.
And now I must parse them here, because the platform eDMA driver's newest v=
ersion will check and use the slave_ids to select and configure the eDMA ch=
annels via dma_device->device_control().=20
--
Xiubo
The driver should not have to manually parse the dma devicetree
properties, this is something that should be handled by the dma
engine driver.
What do you think about the DMA slave_id ?
I have been noticed by one colleague that this should be parsed here,
which is from your opinions ?
Sure slave_id can be parsed here, but IMO it should be programmed via the
dma_slave_confog into the respective channel
Actually, these are parsed for cpu_dai->playback_dma_data and cpu_dai->capture_dma_data dynamically, whose type is struct dma_slave_config.
And now I must parse them here, because the platform eDMA driver's newest version will check and use the slave_ids to select and configure the eDMA channels via dma_device->device_control().
Parsing them here is a layering violation. The format of the DMA specifier
depends on the DMA controller. A DMA slave should not make any assumptions
about how the specifier looks like, it should not even look at them. You should
fix the DMA controller driver to work without slave_id in the devicetree case.
- Lars