| Message ID | cover.1725518229.git.zhoubinbin@loongson.cn |
|---|---|
| Headers | show |
| Series | ASoC: Some issues about loongson i2s | expand |
On 05/09/2024 09:02, Binbin Zhou wrote: > Hi all: > > This patch set is mainly about Loongson i2s related issues. > > Please allow me to briefly explain this patch set: > Patch 1-2: Add ES8323 codec required on Loongson-2K2000 > Patch 3-4: Add uda1342 codec required on Loongson-2K1000 > Patch 5: Improve code readability > Patch 6: Fix the problem of unable to detect codec under FDT system. > Patch 7-8: Add Loongson i2s platform device support > Patch 9-10: Related DTS support. This was based on some old tree... or you do not use get_maintainers.pl. Best regards, Krzysztof
On Thu, Sep 5, 2024 at 2:14 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 05/09/2024 09:02, Binbin Zhou wrote: > > Hi all: > > > > This patch set is mainly about Loongson i2s related issues. > > > > Please allow me to briefly explain this patch set: > > Patch 1-2: Add ES8323 codec required on Loongson-2K2000 > > Patch 3-4: Add uda1342 codec required on Loongson-2K1000 > > Patch 5: Improve code readability > > Patch 6: Fix the problem of unable to detect codec under FDT system. > > Patch 7-8: Add Loongson i2s platform device support > > Patch 9-10: Related DTS support. > > This was based on some old tree... or you do not use get_maintainers.pl. Hi Krzysztof: I used the following method to obtain the email address I need to copy: scripts/get_maintainer.pl sound/soc/ Liam Girdwood <lgirdwood@gmail.com> (supporter:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...) Mark Brown <broonie@kernel.org> (supporter:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...) Jaroslav Kysela <perex@perex.cz> (maintainer:SOUND) Takashi Iwai <tiwai@suse.com> (maintainer:SOUND) linux-sound@vger.kernel.org (open list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...) linux-kernel@vger.kernel.org (open list) The code repository from MAINTAINERS: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/?h=for-next base commit: 097a44db5747403b19d05a9664e8ec6adba27e3b Is there anything I'm missing, or where I'm going wrong? Thanks. Binbin > > Best regards, > Krzysztof >
On 05/09/2024 10:34, Binbin Zhou wrote: > On Thu, Sep 5, 2024 at 2:14 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 05/09/2024 09:02, Binbin Zhou wrote: >>> Hi all: >>> >>> This patch set is mainly about Loongson i2s related issues. >>> >>> Please allow me to briefly explain this patch set: >>> Patch 1-2: Add ES8323 codec required on Loongson-2K2000 >>> Patch 3-4: Add uda1342 codec required on Loongson-2K1000 >>> Patch 5: Improve code readability >>> Patch 6: Fix the problem of unable to detect codec under FDT system. >>> Patch 7-8: Add Loongson i2s platform device support >>> Patch 9-10: Related DTS support. >> >> This was based on some old tree... or you do not use get_maintainers.pl. > > Hi Krzysztof: > > I used the following method to obtain the email address I need to copy: > > scripts/get_maintainer.pl sound/soc/ That's not how you run get_maintainer.pl in typical process. You run it on the patches. > Liam Girdwood <lgirdwood@gmail.com> (supporter:SOUND - SOC LAYER / > DYNAMIC AUDIO POWER MANAGEM...) > Mark Brown <broonie@kernel.org> (supporter:SOUND - SOC LAYER / DYNAMIC > AUDIO POWER MANAGEM...) > Jaroslav Kysela <perex@perex.cz> (maintainer:SOUND) > Takashi Iwai <tiwai@suse.com> (maintainer:SOUND) > linux-sound@vger.kernel.org (open list:SOUND - SOC LAYER / DYNAMIC > AUDIO POWER MANAGEM...) > linux-kernel@vger.kernel.org (open list) > > The code repository from MAINTAINERS: > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/log/?h=for-next > base commit: 097a44db5747403b19d05a9664e8ec6adba27e3b > > Is there anything I'm missing, or where I'm going wrong? Yeah, the list is incomplete and wrong emails are used. Best regards, Krzysztof
On Thu, Sep 05, 2024 at 03:02:15PM +0800, Binbin Zhou wrote: This looks like it was based on some *extremely* old code and needs quite a few updates to bring it up to modern standards. > + * Author: Mark Brown <will@everset-semi.com> Oh? > +/* > + * es8323 register cache > + * We can't read the es8323 register space when we > + * are using 2 wire for device control, so we cache them instead. > + */ > +static u16 es8323_reg[] = { > + 0x06, 0x1C, 0xC3, 0xFC, /* 0 */ > + 0xC0, 0x00, 0x00, 0x7C, /* 4 */ There's a bunch of regmap reimplementation in the driver, the cache and I/O code all looks totally generic. Just use regmap. > +static const struct soc_enum es8323_enum[] = { > + SOC_VALUE_ENUM_SINGLE(ES8323_DACCONTROL16, 3, 7, ARRAY_SIZE(es8323_line_texts), > + es8323_line_texts, es8323_line_values), /* LLINE */ > + SOC_VALUE_ENUM_SINGLE(ES8323_DACCONTROL16, 0, 7, ARRAY_SIZE(es8323_line_texts), > + es8323_line_texts, es8323_line_values), /* RLINE */ Use named variables for the enums rather than putting them into an array that's not otherwise used... > +static const struct snd_kcontrol_new es8323_snd_controls[] = { > + SOC_ENUM("3D Mode", es8323_enum[4]), > + SOC_ENUM("ALC Capture Function", es8323_enum[5]), ...it's *vastly* more readable and maintainable. > + SOC_SINGLE("Capture Mute", ES8323_ADC_MUTE, 2, 1, 0), On/off switches should end in Switch, see control-names.rst. > + /* gModify.Cmmt Implement when suspend/startup */ > + SND_SOC_DAPM_DAC("Right DAC", "Right Playback", SND_SOC_NOPM, 6, 1), gModify.Cmmt? > +/* > + * Note that this should be called from init rather than from hw_params. > + */ > +static int es8323_set_dai_sysclk(struct snd_soc_dai *codec_dai, > + int clk_id, unsigned int freq, int dir) Why? > + /* set master/slave audio interface */ > + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { > + case SND_SOC_DAIFMT_CBM_CFM: /* MASTER MODE */ > + iface |= 0x80; > + break; > + case SND_SOC_DAIFMT_CBS_CFS: /* SLAVE MODE */ Please use the modern naming with _CBP_CFP and so on. > +static int es8323_mute(struct snd_soc_dai *dai, int mute, int stream) > +{ > + struct snd_soc_component *component = dai->component; > + struct es8323_priv *es8323 = snd_soc_component_get_drvdata(component); > + > + if (mute) { > + es8323_set_gpio(ES8323_CODEC_SET_SPK, > + !es8323->spk_gpio_level, es8323->spk_ctl_gpio); > + usleep_range(2000, 3000); > + snd_soc_component_write(component, ES8323_DAC_MUTE, 0x06); > + } else { > + snd_soc_component_write(component, ES8323_DAC_MUTE, 0x02); > + usleep_range(2000, 3000); > + if (!es8323->hp_inserted) > + es8323_set_gpio(ES8323_CODEC_SET_SPK, > + es8323->spk_gpio_level, es8323->spk_ctl_gpio); This appears to be controlling the speaker based on jack detection. Unless there is some hardware restriction this should be done via DAPM and the user allowed to manage when the speaker is used depending on their use case. > +static int es8323_suspend(struct snd_soc_component *component) > +{ > + snd_soc_component_write(component, ES8323_DAC_MUTE, 0x06); > + snd_soc_component_write(component, ES8323_LOUT1_VOL, 0x00); > + snd_soc_component_write(component, ES8323_ROUT1_VOL, 0x00); > + snd_soc_component_write(component, ES8323_LOUT2_VOL, 0x00); > + snd_soc_component_write(component, ES8323_ROUT2_VOL, 0x00); This will overwrite user settings when suspending, the controls should be unaffected by suspend. If the device needs this then use cache bypass to do the writes during suspend and resync the cache on resume. > + snd_soc_component_write(component, ES8323_CONTROL1, 0x06); > + snd_soc_component_write(component, ES8323_CONTROL2, 0x58); > + usleep_range(18000, 20000); Use fsleep(). > +static void es8323_init_component_regs(struct snd_soc_component *component) > +{ > + snd_soc_component_write(component, ES8323_ADCCONTROL1, 0x88); > + snd_soc_component_write(component, ES8323_ADCCONTROL2, 0xF0); > + snd_soc_component_write(component, ES8323_ADCCONTROL3, 0x02); > + snd_soc_component_write(component, ES8323_ADCCONTROL4, 0x0C); > + snd_soc_component_write(component, ES8323_ADCCONTROL5, 0x02); > + snd_soc_component_write(component, ES8323_LADC_VOL, 0x00); > + snd_soc_component_write(component, ES8323_RADC_VOL, 0x00); User visible controls should use the chip defaults as standard, userspace can configure what it needs and this avoids us worrying about individual use cases in the driver. > +static int es8323_resume(struct snd_soc_component *component) > +{ > + es8323_init_component_regs(component); > + /* open dac output */ > + snd_soc_component_write(component, ES8323_DACPOWER, 0x3c); > + > + return 0; > +} This doesn't restore any user settings. > +static int es8323_i2c_probe(struct i2c_client *i2c) > +{ > + struct es8323_priv *es8323; > + struct i2c_adapter *adapter = to_i2c_adapter(i2c->dev.parent); > + > + if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) { > + dev_warn(&adapter->dev, > + "I2C-Adapter doesn't support I2C_FUNC_I2C\n"); > + return -EIO; > + } Does the device really need this? It seems to be doing very basic I/O which should be SMBus compatible. In any case when you move to regmap this should be redundant.
On Thu, Sep 05, 2024 at 03:02:53PM +0800, Binbin Zhou wrote: > The UDA1342 is an NXP audio codec, support 2x Stereo audio ADC (4x PGA > mic inputs), stereo audio DAC, with basic audio processing. This looks basically fine overall, there's some issues below but they're mostly fairly small and stylistic rather than anything major. > --- /dev/null > +++ b/sound/soc/codecs/uda1342.c > @@ -0,0 +1,397 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * uda1342.c -- UDA1342 ALSA SoC Codec driver Please keep the entire comment a C++ one so things look more intentional. > +static int uda1342_mute(struct snd_soc_dai *dai, int mute, int direction) > +{ > + struct snd_soc_component *component = dai->component; > + struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component); > + unsigned int mask; > + unsigned int val = 0; > + > + dev_info(&uda1342->i2c->dev, "mute: %d\n", mute); This is far too noisy to be logged and will DoS the logs, please just remove it. > + > + /* Master mute */ > + mask = BIT(5); > + val = mute ? mask : 0; Please use normal conditional statements to improve legibility. > +static void uda1342_shutdown(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_soc_component *component = dai->component; > + struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component); > + > + if (uda1342->master_substream == substream) > + uda1342->master_substream = uda1342->slave_substream; Please avoid using master/slave terminology, we've tended to go for provider/consumer in ASoC. > +static int uda1342_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) > +{ > + struct snd_soc_component *component = dai->component; > + struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component); > + struct device *dev = &uda1342->i2c->dev; > + unsigned int hw_params = 0; > + > + if (substream == uda1342->slave_substream) { > + dev_info(dev, "ignoring hw_params for slave substream\n"); > + return 0; > + } This is also too noisy, it should be _dbg() at most. > + /* codec supports only full slave mode */ > + if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS) { > + dev_err(dev, "unsupported slave mode.\n"); > + return -EINVAL; > + } Use the more modern _CBC_CFC. > + /* > + * We can't setup DAI format here as it depends on the word bit num, > + * so let's just store the value for later > + */ > + uda1342->dai_fmt = fmt; No need to even store it if only one value is supported. > +static int uda1342_set_bias_level(struct snd_soc_component *component, > + enum snd_soc_bias_level level) > +{ > + switch (level) { > + case SND_SOC_BIAS_ON: > + break; > + case SND_SOC_BIAS_PREPARE: > + break; > + case SND_SOC_BIAS_STANDBY: > + break; > + case SND_SOC_BIAS_OFF: > + break; > + } > + > + return 0; > +} This does nothing so it can just be removed. > +static const struct soc_enum uda1342_mixer_enum[] = { > + SOC_ENUM_SINGLE(0x10, 3, 0x04, uda1342_deemph), > + SOC_ENUM_SINGLE(0x10, 0, 0x04, uda1342_mixmode), > +}; This doesn't seem to be referenced anywhere so can be removed, or should be added to the controls or DAPM? > +static int uda1342_soc_probe(struct snd_soc_component *component) > +{ > + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); > + > + snd_soc_add_component_controls(component, uda1342_snd_controls, > + ARRAY_SIZE(uda1342_snd_controls)); > + snd_soc_dapm_new_controls(dapm, uda1342_dapm_widgets, > + ARRAY_SIZE(uda1342_dapm_widgets)); > + snd_soc_dapm_add_routes(dapm, uda1342_dapm_routes, > + ARRAY_SIZE(uda1342_dapm_routes)); You can point to these arrays from the component struct and have the core register them for you. > +static const struct regmap_config uda1342_regmap = { > + .reg_bits = 8, > + .val_bits = 16, > + .max_register = 0x21, > + .reg_defaults = uda1342_reg_defaults, > + .num_reg_defaults = ARRAY_SIZE(uda1342_reg_defaults), > + .cache_type = REGCACHE_RBTREE, In general _MAPLE is preferred for new things unless you have a specific reason to use _RBTREE, it uses a more modern data structure and should generally do better.
On Thu, Sep 05, 2024 at 03:02:54PM +0800, Binbin Zhou wrote: > This patch attempts to clean up driver code formatting issues. > Mainly as follows: > 1. Use the BIT macro; > 2. Use dev_err_probe() in every error path in probe in loongson_card > driver; > 3. Introduce loongson_card_acpi_find_device() to streamlined code. Please split these out into three separate patches to make them easier to review. I see there's also some other code reordering and reformatting in there which should also be split out separately. I think the changes are probably good overall but there's too much different stuff going on in one patch to check properly.
On Thu, Sep 05, 2024 at 03:07:21PM +0800, Binbin Zhou wrote: > The Loongson I2S controller exists not only in PCI form (LS7A bridge > chip), but also in platform device form (Loongson-2K1000 SoC). > > This patch adds support for platform device I2S controller. Can some of this be shared with the PCI version - is it the same IP in a different wrapper, or is it a new IP?
On Thu, Sep 05, 2024 at 03:02:55PM +0800, Binbin Zhou wrote: > When the Codec is compiled into a module, we can't use > snd_soc_of_get_dlc() to get the codec dai_name, use > snd_soc_get_dai_name() instead. > > Also, for the cpu dailink, its dai_name is already defined as > "loongson-i2s", so just get the corresponding of_node attribute here. > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> > --- > sound/soc/loongson/loongson_card.c | 89 +++++++++++++++++++++--------- > 1 file changed, 63 insertions(+), 26 deletions(-) > > diff --git a/sound/soc/loongson/loongson_card.c b/sound/soc/loongson/loongson_card.c > index a25287efdd5c..d45a3e77cb90 100644 > --- a/sound/soc/loongson/loongson_card.c > +++ b/sound/soc/loongson/loongson_card.c > @@ -119,44 +119,81 @@ static int loongson_card_parse_acpi(struct loongson_card_data *data) > return 0; > } > > -static int loongson_card_parse_of(struct loongson_card_data *data) > +static int loongson_parse_cpu(struct device *dev, struct device_node **dai_node) > { > - struct snd_soc_card *card = &data->snd_card; > - struct device_node *cpu, *codec; > - struct device *dev = card->dev; > - int ret, i; > + struct device_node *cpu; > + int ret = 0; > > cpu = of_get_child_by_name(dev->of_node, "cpu"); > - if (!cpu) { > - dev_err(dev, "platform property missing or invalid\n"); > + if (!cpu) > return -EINVAL; > - } > + > + *dai_node = of_parse_phandle(cpu, "sound-dai", 0); > + if (!*dai_node) > + ret = -EINVAL; > + > + of_node_put(cpu); This goes just after of_parse_phandle, which simplifies your code. > + return ret; > +} > + > +static int loongson_parse_codec(struct device *dev, struct device_node **dai_node, > + const char **dai_name) > +{ > + struct of_phandle_args args; > + struct device_node *codec; > + int ret = 0; > > codec = of_get_child_by_name(dev->of_node, "codec"); > - if (!codec) { > - dev_err(dev, "audio-codec property missing or invalid\n"); > + if (!codec) Hm? So you exit here and then caller does of_node_put on stack value. This is buggy. > + return -EINVAL; > + > + ret = of_parse_phandle_with_args(codec, "sound-dai", "#sound-dai-cells", 0, &args); > + if (ret) > + goto free_codec; > + > + ret = snd_soc_get_dai_name(&args, dai_name); > + if (ret) Your error paths should clean here. Each function is responsible to clean up after itself in case of errors, not rely on caller. > + goto free_codec; > + > + *dai_node = of_parse_phandle(codec, "sound-dai", 0); > + if (!*dai_node) > ret = -EINVAL; > - goto free_cpu; > + > +free_codec: You are not freeing any resources here (at least not directly). You are dropping reference. Use matching label name. free is associated with kalloc().. > + of_node_put(codec); > + return ret; > +} > + > +static int loongson_card_parse_of(struct loongson_card_data *data) > +{ > + struct device_node *codec_dai_node, *cpu_dai_node; > + struct snd_soc_card *card = &data->snd_card; > + struct device *dev = card->dev; > + const char *codec_dai_name; > + int ret = 0, i; > + > + ret = loongson_parse_cpu(dev, &cpu_dai_node); > + if (ret) { > + dev_err(dev, "cpu property missing or invalid.\n"); > + goto out; > + } > + > + ret = loongson_parse_codec(dev, &codec_dai_node, &codec_dai_name); > + if (ret) { > + dev_err(dev, "audio-codec property missing or invalid.\n"); > + goto out; > } > > for (i = 0; i < card->num_links; i++) { > - ret = snd_soc_of_get_dlc(cpu, NULL, loongson_dai_links[i].cpus, 0); > - if (ret) { > - dev_err(dev, "getting cpu dlc error (%d)\n", ret); > - goto free_codec; > - } > - > - ret = snd_soc_of_get_dlc(codec, NULL, loongson_dai_links[i].codecs, 0); > - if (ret) { > - dev_err(dev, "getting codec dlc error (%d)\n", ret); > - goto free_codec; > - } > + loongson_dai_links[i].platforms->of_node = cpu_dai_node; > + loongson_dai_links[i].cpus->of_node = cpu_dai_node; > + loongson_dai_links[i].codecs->of_node = codec_dai_node; > + loongson_dai_links[i].codecs->dai_name = codec_dai_name; > } > > -free_codec: > - of_node_put(codec); > -free_cpu: > - of_node_put(cpu); > +out: > + of_node_put(codec_dai_node); Yeah, so here we see drop of reference from stack-based pointer... Best regards, Krzysztof
Hi Binbin, On Thu, Sep 5, 2024 at 9:07 AM Binbin Zhou <zhoubinbin@loongson.cn> wrote: > The Loongson I2S controller exists not only in PCI form (LS7A bridge > chip), but also in platform device form (Loongson-2K1000 SoC). > > This patch adds support for platform device I2S controller. > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> Thanks for your patch! > --- a/sound/soc/loongson/Kconfig > +++ b/sound/soc/loongson/Kconfig > @@ -13,10 +13,20 @@ config SND_SOC_LOONGSON_I2S_PCI > The controller is found in loongson bridge chips or SoCs, > and work as a PCI device. > > +config SND_SOC_LOONGSON_I2S_PLATFORM > + tristate "Loongson I2S controller as platform device" depends on LOONGARCH || COMPILE_TEST > + select SND_SOC_GENERIC_DMAENGINE_PCM > + help > + Say Y or M if you want to add support for I2S driver for > + Loongson I2S controller. > + > + The controller work as a platform device, found in loongson > + SoCs. > + > config SND_SOC_LOONGSON_CARD > tristate "Loongson Sound Card Driver" > select SND_SOC_LOONGSON_I2S_PCI > - depends on PCI "select SND_SOC_LOONGSON_I2S_PCI if PCI"? > + select SND_SOC_LOONGSON_I2S_PLATFORM Or perhaps do it the other way around, i,e. let SND_SOC_LOONGSON_I2S_{PCI,PLATFORM} select SND_SOC_LOONGSON_CARD? That would be similar to SPI_LOONGSON_{PCI,PLATFORM}, which select SPI_LOONGSON_CORE. > help > Say Y or M if you want to add support for SoC audio using > loongson I2S controller. Gr{oetje,eeting}s, Geert
On Thu, Sep 5, 2024 at 8:31 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Sep 05, 2024 at 03:02:54PM +0800, Binbin Zhou wrote: > > This patch attempts to clean up driver code formatting issues. > > Mainly as follows: > > 1. Use the BIT macro; > > 2. Use dev_err_probe() in every error path in probe in loongson_card > > driver; > > 3. Introduce loongson_card_acpi_find_device() to streamlined code. > > Please split these out into three separate patches to make them easier > to review. I see there's also some other code reordering and > reformatting in there which should also be split out separately. I > think the changes are probably good overall but there's too much > different stuff going on in one patch to check properly. Hi Mark: Thanks for your reply. That was my fault, indeed this patch is too cluttered. I will try to split them up and make them as a separate patch series. Thanks. Binbin
On Thu, Sep 5, 2024 at 8:36 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Sep 05, 2024 at 03:07:21PM +0800, Binbin Zhou wrote: > > The Loongson I2S controller exists not only in PCI form (LS7A bridge > > chip), but also in platform device form (Loongson-2K1000 SoC). > > > > This patch adds support for platform device I2S controller. > > Can some of this be shared with the PCI version - is it the same IP in a > different wrapper, or is it a new IP? Hi Mark: Thanks for your reply. To be exact, they are similar, such as the definition of the controller registers. But for example, DMA data processing is different. In the pci version of i2s, the DMA controller is built-in, while the DMA controller here is external, using ls2x-apbdma (drivers/dma/ls2x-apb-dma.c) Thanks. Binbin
Hi Geert: Thanks for your reply. On Fri, Sep 6, 2024 at 5:37 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Binbin, > > On Thu, Sep 5, 2024 at 9:07 AM Binbin Zhou <zhoubinbin@loongson.cn> wrote: > > The Loongson I2S controller exists not only in PCI form (LS7A bridge > > chip), but also in platform device form (Loongson-2K1000 SoC). > > > > This patch adds support for platform device I2S controller. > > > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> > > Thanks for your patch! > > > --- a/sound/soc/loongson/Kconfig > > +++ b/sound/soc/loongson/Kconfig > > @@ -13,10 +13,20 @@ config SND_SOC_LOONGSON_I2S_PCI > > The controller is found in loongson bridge chips or SoCs, > > and work as a PCI device. > > > > +config SND_SOC_LOONGSON_I2S_PLATFORM > > + tristate "Loongson I2S controller as platform device" > > depends on LOONGARCH || COMPILE_TEST This is will put under SND_SOC_LOONGSON_CARD. > > > + select SND_SOC_GENERIC_DMAENGINE_PCM > > + help > > + Say Y or M if you want to add support for I2S driver for > > + Loongson I2S controller. > > + > > + The controller work as a platform device, found in loongson > > + SoCs. > > + > > config SND_SOC_LOONGSON_CARD > > tristate "Loongson Sound Card Driver" > > select SND_SOC_LOONGSON_I2S_PCI > > - depends on PCI > > "select SND_SOC_LOONGSON_I2S_PCI if PCI"? > > > + select SND_SOC_LOONGSON_I2S_PLATFORM > > Or perhaps do it the other way around, i,e. let > SND_SOC_LOONGSON_I2S_{PCI,PLATFORM} select SND_SOC_LOONGSON_CARD? > That would be similar to SPI_LOONGSON_{PCI,PLATFORM}, which select > SPI_LOONGSON_CORE. Yes, it would be clearer. I'll modify it in the next version. Thanks. Binbin > > > help > > Say Y or M if you want to add support for SoC audio using > > loongson I2S controller. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
On Sat, Sep 07, 2024 at 02:08:08PM +0600, Binbin Zhou wrote: > On Thu, Sep 5, 2024 at 8:36 PM Mark Brown <broonie@kernel.org> wrote: > > Can some of this be shared with the PCI version - is it the same IP in a > > different wrapper, or is it a new IP? > To be exact, they are similar, such as the definition of the > controller registers. > But for example, DMA data processing is different. In the pci version > of i2s, the DMA controller is built-in, while the DMA controller here > is external, using ls2x-apbdma (drivers/dma/ls2x-apb-dma.c) OK, if all the registers are the same it might be worth trying to share that code but possibly not with the DMA being split like this.
Hi Krzysztof: Thanks for your detailed review. On Fri, Sep 6, 2024 at 4:29 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Thu, Sep 05, 2024 at 03:02:55PM +0800, Binbin Zhou wrote: > > When the Codec is compiled into a module, we can't use > > snd_soc_of_get_dlc() to get the codec dai_name, use > > snd_soc_get_dai_name() instead. > > > > Also, for the cpu dailink, its dai_name is already defined as > > "loongson-i2s", so just get the corresponding of_node attribute here. > > > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> > > --- > > sound/soc/loongson/loongson_card.c | 89 +++++++++++++++++++++--------- > > 1 file changed, 63 insertions(+), 26 deletions(-) > > > > diff --git a/sound/soc/loongson/loongson_card.c b/sound/soc/loongson/loongson_card.c > > index a25287efdd5c..d45a3e77cb90 100644 > > --- a/sound/soc/loongson/loongson_card.c > > +++ b/sound/soc/loongson/loongson_card.c > > @@ -119,44 +119,81 @@ static int loongson_card_parse_acpi(struct loongson_card_data *data) > > return 0; > > } > > > > -static int loongson_card_parse_of(struct loongson_card_data *data) > > +static int loongson_parse_cpu(struct device *dev, struct device_node **dai_node) > > { > > - struct snd_soc_card *card = &data->snd_card; > > - struct device_node *cpu, *codec; > > - struct device *dev = card->dev; > > - int ret, i; > > + struct device_node *cpu; > > + int ret = 0; > > > > cpu = of_get_child_by_name(dev->of_node, "cpu"); > > - if (!cpu) { > > - dev_err(dev, "platform property missing or invalid\n"); > > + if (!cpu) > > return -EINVAL; > > - } > > + > > + *dai_node = of_parse_phandle(cpu, "sound-dai", 0); > > + if (!*dai_node) > > + ret = -EINVAL; > > + > > + of_node_put(cpu); > > This goes just after of_parse_phandle, which simplifies your code. OK.. the ret param is unnecessary also. > > > + return ret; > > +} > > + > > +static int loongson_parse_codec(struct device *dev, struct device_node **dai_node, > > + const char **dai_name) > > +{ > > + struct of_phandle_args args; > > + struct device_node *codec; > > + int ret = 0; > > > > codec = of_get_child_by_name(dev->of_node, "codec"); > > - if (!codec) { > > - dev_err(dev, "audio-codec property missing or invalid\n"); > > + if (!codec) > > Hm? So you exit here and then caller does of_node_put on stack value. > This is buggy. Sorry, I can not get your point, I think there is nothing that should be put. > > > + return -EINVAL; > > + > > + ret = of_parse_phandle_with_args(codec, "sound-dai", "#sound-dai-cells", 0, &args); > > + if (ret) > > + goto free_codec; > > + > > + ret = snd_soc_get_dai_name(&args, dai_name); > > + if (ret) > > Your error paths should clean here. Each function is responsible to > clean up after itself in case of errors, not rely on caller. Yes, I should of_node_put(args.np); here > > > + goto free_codec; > > + > > + *dai_node = of_parse_phandle(codec, "sound-dai", 0); > > + if (!*dai_node) > > ret = -EINVAL; > > - goto free_cpu; > > + > > +free_codec: > > You are not freeing any resources here (at least not directly). You are > dropping reference. Use matching label name. free is associated with > kalloc().. > OK, I will rename the label name as codec_put. Thanks. Binbin > > > + of_node_put(codec); > > + return ret; > > +} > > + > > +static int loongson_card_parse_of(struct loongson_card_data *data) > > +{ > > + struct device_node *codec_dai_node, *cpu_dai_node; > > + struct snd_soc_card *card = &data->snd_card; > > + struct device *dev = card->dev; > > + const char *codec_dai_name; > > + int ret = 0, i; > > + > > + ret = loongson_parse_cpu(dev, &cpu_dai_node); > > + if (ret) { > > + dev_err(dev, "cpu property missing or invalid.\n"); > > + goto out; > > + } > > + > > + ret = loongson_parse_codec(dev, &codec_dai_node, &codec_dai_name); > > + if (ret) { > > + dev_err(dev, "audio-codec property missing or invalid.\n"); > > + goto out; > > } > > > > for (i = 0; i < card->num_links; i++) { > > - ret = snd_soc_of_get_dlc(cpu, NULL, loongson_dai_links[i].cpus, 0); > > - if (ret) { > > - dev_err(dev, "getting cpu dlc error (%d)\n", ret); > > - goto free_codec; > > - } > > - > > - ret = snd_soc_of_get_dlc(codec, NULL, loongson_dai_links[i].codecs, 0); > > - if (ret) { > > - dev_err(dev, "getting codec dlc error (%d)\n", ret); > > - goto free_codec; > > - } > > + loongson_dai_links[i].platforms->of_node = cpu_dai_node; > > + loongson_dai_links[i].cpus->of_node = cpu_dai_node; > > + loongson_dai_links[i].codecs->of_node = codec_dai_node; > > + loongson_dai_links[i].codecs->dai_name = codec_dai_name; > > } > > > > -free_codec: > > - of_node_put(codec); > > -free_cpu: > > - of_node_put(cpu); > > +out: > > + of_node_put(codec_dai_node); > > Yeah, so here we see drop of reference from stack-based pointer... > > Best regards, > Krzysztof >
On 09/09/2024 09:51, Binbin Zhou wrote: >> >>> + return ret; >>> +} >>> + >>> +static int loongson_parse_codec(struct device *dev, struct device_node **dai_node, >>> + const char **dai_name) >>> +{ >>> + struct of_phandle_args args; >>> + struct device_node *codec; >>> + int ret = 0; >>> >>> codec = of_get_child_by_name(dev->of_node, "codec"); >>> - if (!codec) { >>> - dev_err(dev, "audio-codec property missing or invalid\n"); >>> + if (!codec) >> >> Hm? So you exit here and then caller does of_node_put on stack value. >> This is buggy. > > Sorry, I can not get your point, I think there is nothing that should be put. You drop reference from a pointer which is a random stack value. Best regards, Krzysztof
Hi Mark: Thanks for your detailed review. On Thu, Sep 5, 2024 at 8:28 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Sep 05, 2024 at 03:02:53PM +0800, Binbin Zhou wrote: > > > The UDA1342 is an NXP audio codec, support 2x Stereo audio ADC (4x PGA > > mic inputs), stereo audio DAC, with basic audio processing. > > This looks basically fine overall, there's some issues below but they're > mostly fairly small and stylistic rather than anything major. > > > --- /dev/null > > +++ b/sound/soc/codecs/uda1342.c > > @@ -0,0 +1,397 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * uda1342.c -- UDA1342 ALSA SoC Codec driver > > Please keep the entire comment a C++ one so things look more > intentional. > > > +static int uda1342_mute(struct snd_soc_dai *dai, int mute, int direction) > > +{ > > + struct snd_soc_component *component = dai->component; > > + struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component); > > + unsigned int mask; > > + unsigned int val = 0; > > + > > + dev_info(&uda1342->i2c->dev, "mute: %d\n", mute); > > This is far too noisy to be logged and will DoS the logs, please just > remove it. OK, I will drop it. > > > + > > + /* Master mute */ > > + mask = BIT(5); > > + val = mute ? mask : 0; > > Please use normal conditional statements to improve legibility. OK, the code as following: if (mute) val = mask; > > > +static void uda1342_shutdown(struct snd_pcm_substream *substream, > > + struct snd_soc_dai *dai) > > +{ > > + struct snd_soc_component *component = dai->component; > > + struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component); > > + > > + if (uda1342->master_substream == substream) > > + uda1342->master_substream = uda1342->slave_substream; > > Please avoid using master/slave terminology, we've tended to go for > provider/consumer in ASoC. OK, I see, I will rename it. > > > +static int uda1342_hw_params(struct snd_pcm_substream *substream, > > + struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) > > +{ > > + struct snd_soc_component *component = dai->component; > > + struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component); > > + struct device *dev = &uda1342->i2c->dev; > > + unsigned int hw_params = 0; > > + > > + if (substream == uda1342->slave_substream) { > > + dev_info(dev, "ignoring hw_params for slave substream\n"); > > + return 0; > > + } > > This is also too noisy, it should be _dbg() at most. OK, I will drop it. > > > + /* codec supports only full slave mode */ > > + if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS) { > > + dev_err(dev, "unsupported slave mode.\n"); > > + return -EINVAL; > > + } > > Use the more modern _CBC_CFC. OK, it should be replaced by SND_SOC_DAIFMT_BC_FC. > > > + /* > > + * We can't setup DAI format here as it depends on the word bit num, > > + * so let's just store the value for later > > + */ > > + uda1342->dai_fmt = fmt; > > No need to even store it if only one value is supported. Emm, I will put the fmt setting here from the hw_param function. Also, the dai_fmt could be dropped. > > > +static int uda1342_set_bias_level(struct snd_soc_component *component, > > + enum snd_soc_bias_level level) > > +{ > > + switch (level) { > > + case SND_SOC_BIAS_ON: > > + break; > > + case SND_SOC_BIAS_PREPARE: > > + break; > > + case SND_SOC_BIAS_STANDBY: > > + break; > > + case SND_SOC_BIAS_OFF: > > + break; > > + } > > + > > + return 0; > > +} > > This does nothing so it can just be removed. I will drop it. > > > +static const struct soc_enum uda1342_mixer_enum[] = { > > + SOC_ENUM_SINGLE(0x10, 3, 0x04, uda1342_deemph), > > + SOC_ENUM_SINGLE(0x10, 0, 0x04, uda1342_mixmode), > > +}; > > This doesn't seem to be referenced anywhere so can be removed, or should > be added to the controls or DAPM? I will drop it. > > > +static int uda1342_soc_probe(struct snd_soc_component *component) > > +{ > > + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); > > + > > + snd_soc_add_component_controls(component, uda1342_snd_controls, > > + ARRAY_SIZE(uda1342_snd_controls)); > > + snd_soc_dapm_new_controls(dapm, uda1342_dapm_widgets, > > + ARRAY_SIZE(uda1342_dapm_widgets)); > > + snd_soc_dapm_add_routes(dapm, uda1342_dapm_routes, > > + ARRAY_SIZE(uda1342_dapm_routes)); > > You can point to these arrays from the component struct and have the > core register them for you. OK, I will do it. Thanks. Binbin > > > +static const struct regmap_config uda1342_regmap = { > > + .reg_bits = 8, > > + .val_bits = 16, > > + .max_register = 0x21, > > + .reg_defaults = uda1342_reg_defaults, > > + .num_reg_defaults = ARRAY_SIZE(uda1342_reg_defaults), > > + .cache_type = REGCACHE_RBTREE, > > In general _MAPLE is preferred for new things unless you have a specific > reason to use _RBTREE, it uses a more modern data structure and should > generally do better.
Hi Mark: Thanks for your detailed review. On Thu, Sep 5, 2024 at 8:05 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Sep 05, 2024 at 03:02:15PM +0800, Binbin Zhou wrote: > > This looks like it was based on some *extremely* old code and needs > quite a few updates to bring it up to modern standards. > > > + * Author: Mark Brown <will@everset-semi.com> > > Oh? > > > +/* > > + * es8323 register cache > > + * We can't read the es8323 register space when we > > + * are using 2 wire for device control, so we cache them instead. > > + */ > > +static u16 es8323_reg[] = { > > + 0x06, 0x1C, 0xC3, 0xFC, /* 0 */ > > + 0xC0, 0x00, 0x00, 0x7C, /* 4 */ > > There's a bunch of regmap reimplementation in the driver, the cache and > I/O code all looks totally generic. Just use regmap. Sorry, I don't really understand this point. Do you mean to use regmap_read()/regmap_write() instead of snd_soc_component_read()/snd_soc_component_write()? If so, are there any rules for using snd_soc_component_xxx()? > > > +static const struct soc_enum es8323_enum[] = { > > + SOC_VALUE_ENUM_SINGLE(ES8323_DACCONTROL16, 3, 7, ARRAY_SIZE(es8323_line_texts), > > + es8323_line_texts, es8323_line_values), /* LLINE */ > > + SOC_VALUE_ENUM_SINGLE(ES8323_DACCONTROL16, 0, 7, ARRAY_SIZE(es8323_line_texts), > > + es8323_line_texts, es8323_line_values), /* RLINE */ > > Use named variables for the enums rather than putting them into an array > that's not otherwise used... OK, I will use macro like SOC_ENUM_SINGLE_DECL() to define them. > > > +static const struct snd_kcontrol_new es8323_snd_controls[] = { > > + SOC_ENUM("3D Mode", es8323_enum[4]), > > + SOC_ENUM("ALC Capture Function", es8323_enum[5]), > > ...it's *vastly* more readable and maintainable. > > > + SOC_SINGLE("Capture Mute", ES8323_ADC_MUTE, 2, 1, 0), > > On/off switches should end in Switch, see control-names.rst. > > > + /* gModify.Cmmt Implement when suspend/startup */ > > + SND_SOC_DAPM_DAC("Right DAC", "Right Playback", SND_SOC_NOPM, 6, 1), > > gModify.Cmmt? > > > +/* > > + * Note that this should be called from init rather than from hw_params. > > + */ > > +static int es8323_set_dai_sysclk(struct snd_soc_dai *codec_dai, > > + int clk_id, unsigned int freq, int dir) > > Why? > > > + /* set master/slave audio interface */ > > + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { > > + case SND_SOC_DAIFMT_CBM_CFM: /* MASTER MODE */ > > + iface |= 0x80; > > + break; > > + case SND_SOC_DAIFMT_CBS_CFS: /* SLAVE MODE */ > > Please use the modern naming with _CBP_CFP and so on. Get. SND_SOC_DAIFMT_BC_FP/SND_SOC_DAIFMT_BC_FC will be used. > > > +static int es8323_mute(struct snd_soc_dai *dai, int mute, int stream) > > +{ > > + struct snd_soc_component *component = dai->component; > > + struct es8323_priv *es8323 = snd_soc_component_get_drvdata(component); > > + > > + if (mute) { > > + es8323_set_gpio(ES8323_CODEC_SET_SPK, > > + !es8323->spk_gpio_level, es8323->spk_ctl_gpio); > > + usleep_range(2000, 3000); > > + snd_soc_component_write(component, ES8323_DAC_MUTE, 0x06); > > + } else { > > + snd_soc_component_write(component, ES8323_DAC_MUTE, 0x02); > > + usleep_range(2000, 3000); > > + if (!es8323->hp_inserted) > > + es8323_set_gpio(ES8323_CODEC_SET_SPK, > > + es8323->spk_gpio_level, es8323->spk_ctl_gpio); > > This appears to be controlling the speaker based on jack detection. > Unless there is some hardware restriction this should be done via DAPM > and the user allowed to manage when the speaker is used depending on > their use case. In this version of the patch, I plan to remove the jack related functions, and I will add them later. > > > +static int es8323_suspend(struct snd_soc_component *component) > > +{ > > + snd_soc_component_write(component, ES8323_DAC_MUTE, 0x06); > > + snd_soc_component_write(component, ES8323_LOUT1_VOL, 0x00); > > + snd_soc_component_write(component, ES8323_ROUT1_VOL, 0x00); > > + snd_soc_component_write(component, ES8323_LOUT2_VOL, 0x00); > > + snd_soc_component_write(component, ES8323_ROUT2_VOL, 0x00); > > This will overwrite user settings when suspending, the controls should > be unaffected by suspend. If the device needs this then use cache > bypass to do the writes during suspend and resync the cache on resume. regcache_cache_only() and regcache_sync() will be used. > > > + snd_soc_component_write(component, ES8323_CONTROL1, 0x06); > > + snd_soc_component_write(component, ES8323_CONTROL2, 0x58); > > + usleep_range(18000, 20000); > > Use fsleep(). > > > +static void es8323_init_component_regs(struct snd_soc_component *component) > > +{ > > > + snd_soc_component_write(component, ES8323_ADCCONTROL1, 0x88); > > + snd_soc_component_write(component, ES8323_ADCCONTROL2, 0xF0); > > + snd_soc_component_write(component, ES8323_ADCCONTROL3, 0x02); > > + snd_soc_component_write(component, ES8323_ADCCONTROL4, 0x0C); > > + snd_soc_component_write(component, ES8323_ADCCONTROL5, 0x02); > > + snd_soc_component_write(component, ES8323_LADC_VOL, 0x00); > > + snd_soc_component_write(component, ES8323_RADC_VOL, 0x00); > > User visible controls should use the chip defaults as standard, > userspace can configure what it needs and this avoids us worrying about > individual use cases in the driver. > > > +static int es8323_resume(struct snd_soc_component *component) > > +{ > > + es8323_init_component_regs(component); > > + /* open dac output */ > > + snd_soc_component_write(component, ES8323_DACPOWER, 0x3c); > > + > > + return 0; > > +} > > This doesn't restore any user settings. > > > +static int es8323_i2c_probe(struct i2c_client *i2c) > > +{ > > + struct es8323_priv *es8323; > > + struct i2c_adapter *adapter = to_i2c_adapter(i2c->dev.parent); > > + > > + if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) { > > + dev_warn(&adapter->dev, > > + "I2C-Adapter doesn't support I2C_FUNC_I2C\n"); > > + return -EIO; > > + } > > Does the device really need this? It seems to be doing very basic I/O > which should be SMBus compatible. In any case when you move to regmap > this should be redundant. Emm, I will drop it. Thanks. Binbin
On Fri, Sep 13, 2024 at 08:02:02AM +0600, Binbin Zhou wrote: > On Thu, Sep 5, 2024 at 8:05 PM Mark Brown <broonie@kernel.org> wrote: > > > +/* > > > + * es8323 register cache > > > + * We can't read the es8323 register space when we > > > + * are using 2 wire for device control, so we cache them instead. > > > + */ > > > +static u16 es8323_reg[] = { > > > + 0x06, 0x1C, 0xC3, 0xFC, /* 0 */ > > > + 0xC0, 0x00, 0x00, 0x7C, /* 4 */ > > There's a bunch of regmap reimplementation in the driver, the cache and > > I/O code all looks totally generic. Just use regmap. > Sorry, I don't really understand this point. > Do you mean to use regmap_read()/regmap_write() instead of > snd_soc_component_read()/snd_soc_component_write()? > If so, are there any rules for using snd_soc_component_xxx()? No, it's fine to use the component wrappers if you happen to have a component to hand. What's an issue is things like writing your own register cache code (the above bit is part of an open coded cache implementation), or I2C read/write functions if there's not something unusual with how the device does I/O.
Hi Mark: Thanks for your explanation. On Fri, Sep 13, 2024 at 10:44 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Sep 13, 2024 at 08:02:02AM +0600, Binbin Zhou wrote: > > On Thu, Sep 5, 2024 at 8:05 PM Mark Brown <broonie@kernel.org> wrote: > > > > > +/* > > > > + * es8323 register cache > > > > + * We can't read the es8323 register space when we > > > > + * are using 2 wire for device control, so we cache them instead. > > > > + */ > > > > +static u16 es8323_reg[] = { > > > > + 0x06, 0x1C, 0xC3, 0xFC, /* 0 */ > > > > + 0xC0, 0x00, 0x00, 0x7C, /* 4 */ > > > > There's a bunch of regmap reimplementation in the driver, the cache and > > > I/O code all looks totally generic. Just use regmap. > > > Sorry, I don't really understand this point. > > Do you mean to use regmap_read()/regmap_write() instead of > > snd_soc_component_read()/snd_soc_component_write()? > > > If so, are there any rules for using snd_soc_component_xxx()? > > No, it's fine to use the component wrappers if you happen to have a > component to hand. What's an issue is things like writing your own > register cache code (the above bit is part of an open coded cache > implementation), or I2C read/write functions if there's not something > unusual with how the device does I/O. Indeed, I misunderstood it before. As I understand it, we should use regmap_config.reg_defaults instead of snd_soc_component_driver.{read/write}. Thanks. Binbin
On Wed, Sep 18, 2024 at 03:24:45PM +0600, Binbin Zhou wrote: > On Fri, Sep 13, 2024 at 10:44 PM Mark Brown <broonie@kernel.org> wrote: > > No, it's fine to use the component wrappers if you happen to have a > > component to hand. What's an issue is things like writing your own > > register cache code (the above bit is part of an open coded cache > > implementation), or I2C read/write functions if there's not something > > unusual with how the device does I/O. > Indeed, I misunderstood it before. As I understand it, we should use > regmap_config.reg_defaults instead of > snd_soc_component_driver.{read/write}. I think so - I'll need to see your actual patch to confirm but that sounds like the right direction.