Re: [PATCH v7 2/8] ASoC: mediatek: mt8186: add platform driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2022-06-10 at 12:02 +0200, AngeloGioacchino Del Regno wrote:
> Il 10/06/22 10:27, Jiaxin Yu ha scritto:
> > Add mt8186 platform and affiliated driver.
> > 
> > Signed-off-by: Jiaxin Yu <jiaxin.yu@xxxxxxxxxxxx>
> > ---
> >   sound/soc/mediatek/Kconfig                    |   12 +
> >   sound/soc/mediatek/Makefile                   |    1 +
> >   sound/soc/mediatek/mt8186/Makefile            |   19 +
> >   sound/soc/mediatek/mt8186/mt8186-afe-common.h |  235 ++
> >   .../soc/mediatek/mt8186/mt8186-afe-control.c  |  261 ++
> >   sound/soc/mediatek/mt8186/mt8186-afe-pcm.c    | 3009
> > +++++++++++++++++
> >   6 files changed, 3537 insertions(+)
> >   create mode 100644 sound/soc/mediatek/mt8186/Makefile
> >   create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-common.h
> >   create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-control.c
> >   create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-pcm.c
..snip..
> > diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-pcm.c
> > b/sound/soc/mediatek/mt8186/mt8186-afe-pcm.c
> > new file mode 100644
> > index 000000000000..aaba8627d9e1
> > --- /dev/null
> > +++ b/sound/soc/mediatek/mt8186/mt8186-afe-pcm.c
> > @@ -0,0 +1,3009 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Mediatek ALSA SoC AFE platform driver for 8186
> > +//
> > +// Copyright (c) 2022 MediaTek Inc.
> > +// Author: Jiaxin Yu <jiaxin.yu@xxxxxxxxxxxx>
> > +
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/reset.h>
> > +#include <sound/soc.h>
> > +
> > +#include "../common/mtk-afe-platform-driver.h"
> > +#include "../common/mtk-afe-fe-dai.h"
> > +
> > +#include "mt8186-afe-common.h"
> > +#include "mt8186-afe-clk.h"
> > +#include "mt8186-afe-gpio.h"
> > +#include "mt8186-interconnection.h"
> > +
> 
> ..snip..
> 
> > +
> > +static int mt8186_fe_hw_free(struct snd_pcm_substream *substream,
> > +			     struct snd_soc_dai *dai)
> > +{
> > +	int ret;
> > +
> > +	ret = mtk_afe_fe_hw_free(substream, dai);
> > +	if (ret)
> > +		goto exit;
> > +
> > +	/* wait for some platform related operation */
> 
> This comment says that we should wait for "some platform related
> operation":
> it's not describing what kind of operation we would wait for... and
> we are
> not waiting for anything at all and just returning.
> 
> Are you implementing this wait mechanism?
> ...otherwise, there's no need for that goto at all.
> 

This is a reserved place for development, but there is no need to do
anything in the end. So we can remove this line.

> > +exit:
> > +	return ret;
> > +}
> > +
> > +static int mt8186_fe_trigger(struct snd_pcm_substream *substream,
> > int cmd,
> > +			     struct snd_soc_dai *dai)
> > +{
> > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +	struct snd_pcm_runtime * const runtime = substream->runtime;
> > +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int id = asoc_rtd_to_cpu(rtd, 0)->id;
> > +	struct mtk_base_afe_memif *memif = &afe->memif[id];
> > +	int irq_id = memif->irq_usage;
> > +	struct mtk_base_afe_irq *irqs = &afe->irqs[irq_id];
> > +	const struct mtk_base_irq_data *irq_data = irqs->irq_data;
> > +	unsigned int counter = runtime->period_size;
> 
> You're assigning a value to 'counter' here and you may or may not use
> it,
> because if we look down there...
> 
> > +	unsigned int rate = runtime->rate;
> > +	int fs;
> > +	int ret;
> > +
> > +	dev_dbg(afe->dev, "%s(), %s cmd %d, irq_id %d\n",
> > +		__func__, memif->data->name, cmd, irq_id);
> > +
> > +	switch (cmd) {
> > +	case SNDRV_PCM_TRIGGER_START:
> > +	case SNDRV_PCM_TRIGGER_RESUME:
> > +		ret = mtk_memif_set_enable(afe, id);
> > +		if (ret) {
> > +			dev_err(afe->dev, "%s(), error, id %d, memif
> > enable, ret %d\n",
> > +				__func__, id, ret);
> > +			return ret;
> > +		}
> > +
> > +		/*
> > +		 * for small latency record
> > +		 * ul memif need read some data before irq enable
> > +		 */
> > +		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE &&
> > +		    ((runtime->period_size * 1000) / rate <= 10))
> > +			udelay(300);
> > +
> > +		/* set irq counter */
> > +		if (afe_priv->irq_cnt[id] > 0)
> > +			counter = afe_priv->irq_cnt[id];
> 
> ...you're reassigning the counter if irq_cnt > 0, so you can as well
> avoid a
> reassignment if you simply do it like
> 
> 		if (afe_priv->irq_cnt[id] > 0)
> 			counter = afe_priv->irq_cnt[id];
> 		else
> 			counter = runtime->period_size;
> 

Agree with you.

> > +
> > +		regmap_update_bits(afe->regmap, irq_data->irq_cnt_reg,
> > +				   irq_data->irq_cnt_maskbit
> > +				   << irq_data->irq_cnt_shift,
> > +				   counter << irq_data->irq_cnt_shift);
> > +
> > +		/* set irq fs */
> > +		fs = afe->irq_fs(substream, runtime->rate);
> > +		if (fs < 0)
> > +			return -EINVAL;
> > +
> > +		regmap_update_bits(afe->regmap, irq_data->irq_fs_reg,
> > +				   irq_data->irq_fs_maskbit
> > +				   << irq_data->irq_fs_shift,
> > +				   fs << irq_data->irq_fs_shift);
> > +
> > +		/* enable interrupt */
> > +		if (runtime->stop_threshold != ~(0U))
> > +			regmap_update_bits(afe->regmap,
> > +					   irq_data->irq_en_reg,
> > +					   1 << irq_data->irq_en_shift,
> > +					   1 << irq_data-
> > >irq_en_shift);
> > +		return 0;
> > +	case SNDRV_PCM_TRIGGER_STOP:
> > +	case SNDRV_PCM_TRIGGER_SUSPEND:
> > +		if (afe_priv->xrun_assert[id] > 0) {
> > +			if (substream->stream ==
> > SNDRV_PCM_STREAM_CAPTURE) {
> > +				int avail =
> > snd_pcm_capture_avail(runtime);
> > +
> > +				if (avail >= runtime->buffer_size) {
> > +					dev_err(afe->dev, "%s(), id %d,
> > xrun assert\n",
> > +						__func__, id);
> 
> You're printing an error here, but not returning an error state
> related to
> this one at the end of this case... why?
> 
> (if this is intended, add a comment in the code explaining the
> reason)
> 

Here is a debug log about xrun when capture. There is no processing
here because ALSA will help with it. We just want to know the id
information.

> > +				}
> > +			}
> > +		} > +
> > +		ret = mtk_memif_set_disable(afe, id);
> > +		if (ret)
> > +			dev_err(afe->dev, "%s(), error, id %d, memif
> > enable, ret %d\n",
> > +				__func__, id, ret);
> > +
> > +		/* disable interrupt */
> > +		if (runtime->stop_threshold != ~(0U))
> > +			regmap_update_bits(afe->regmap,
> > +					   irq_data->irq_en_reg,
> > +					   1 << irq_data->irq_en_shift,
> > +					   0 << irq_data-
> > >irq_en_shift);
> > +
> > +		/* clear pending IRQ */
> > +		regmap_write(afe->regmap, irq_data->irq_clr_reg,
> > +			     1 << irq_data->irq_clr_shift);
> > +		return ret;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> 
> ..snip..
> 
> > +
> > +static int mt8186_irq_cnt1_set(struct snd_kcontrol *kcontrol,
> > +			       struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_component *cmpnt =
> > snd_soc_kcontrol_component(kcontrol);
> > +	struct mtk_base_afe *afe =
> > snd_soc_component_get_drvdata(cmpnt);
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int memif_num = MT8186_PRIMARY_MEMIF;
> > +	struct mtk_base_afe_memif *memif = &afe->memif[memif_num];
> > +	int irq_id = memif->irq_usage;
> > +	int irq_cnt = afe_priv->irq_cnt[memif_num];
> > +
> > +	dev_dbg(afe->dev, "%s(), irq_id %d, irq_cnt = %d, value =
> > %ld\n",
> > +		__func__, irq_id, irq_cnt, ucontrol-
> > >value.integer.value[0]);
> > +
> > +	if (irq_cnt == ucontrol->value.integer.value[0])
> > +		return 0;
> > +
> > +	irq_cnt = ucontrol->value.integer.value[0];
> > +	afe_priv->irq_cnt[memif_num] = irq_cnt;
> > +
> > +	if (pm_runtime_status_suspended(afe->dev) || irq_id < 0) {
> > +		dev_info(afe->dev, "%s(), suspended || irq_id %d, not
> > set\n",
> > +			 __func__, irq_id);
> 
> This should probably be a dev_dbg, as that's not supposed to normally
> happen.
> Besides, since the expected flow for this function is to write to
> registers,
> I think that for readability purposes it's best to invert this
> conditional:
> 
> 	if (!pm_runtime_status_suspended(afe->dev) && irq_id >= 0) {
> 		.....
> 	} else {
> 		dev_dbg(.....)
> 	}
> 

Got it.

> > +	} else {
> > +		struct mtk_base_afe_irq *irqs = &afe->irqs[irq_id];
> > +		const struct mtk_base_irq_data *irq_data = irqs-
> > >irq_data;
> > +
> > +		regmap_update_bits(afe->regmap, irq_data->irq_cnt_reg,
> > +				   irq_data->irq_cnt_maskbit
> > +				   << irq_data->irq_cnt_shift,
> > +				   irq_cnt << irq_data->irq_cnt_shift);
> > +	}
> > +
> > +	return 1;
> > +}
> > +
> > +static int mt8186_irq_cnt2_get(struct snd_kcontrol *kcontrol,
> > +			       struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_component *cmpnt =
> > snd_soc_kcontrol_component(kcontrol);
> > +	struct mtk_base_afe *afe =
> > snd_soc_component_get_drvdata(cmpnt);
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +
> > +	ucontrol->value.integer.value[0] =
> > +		afe_priv->irq_cnt[MT8186_RECORD_MEMIF];
> > +
> > +	return 0;
> > +}
> > +
> > +static int mt8186_irq_cnt2_set(struct snd_kcontrol *kcontrol,
> > +			       struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_component *cmpnt =
> > snd_soc_kcontrol_component(kcontrol);
> > +	struct mtk_base_afe *afe =
> > snd_soc_component_get_drvdata(cmpnt);
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int memif_num = MT8186_RECORD_MEMIF;
> > +	struct mtk_base_afe_memif *memif = &afe->memif[memif_num];
> > +	int irq_id = memif->irq_usage;
> > +	int irq_cnt = afe_priv->irq_cnt[memif_num];
> > +
> > +	dev_dbg(afe->dev, "%s(), irq_id %d, irq_cnt = %d, value =
> > %ld\n",
> > +		__func__, irq_id, irq_cnt, ucontrol-
> > >value.integer.value[0]);
> > +
> > +	if (irq_cnt == ucontrol->value.integer.value[0])
> > +		return 0;
> > +
> > +	irq_cnt = ucontrol->value.integer.value[0];
> > +	afe_priv->irq_cnt[memif_num] = irq_cnt;
> > +
> > +	if (pm_runtime_status_suspended(afe->dev) || irq_id < 0) {
> > +		dev_info(afe->dev, "%s(), suspended || irq_id %d, not
> > set\n",
> > +			 __func__, irq_id);
> 
> same here.
> 

Got it.

> > +static int mt8186_afe_pcm_dev_probe(struct platform_device *pdev)
> > +{
> > +	struct mtk_base_afe *afe;
> > +	struct mt8186_afe_private *afe_priv;
> > +	struct resource *res;
> > +	struct reset_control *rstc;
> > +	struct device *dev = &pdev->dev;
> > +	int i, ret, irq_id;
> > +
> > +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(34));
> > +	if (ret)
> > +		return ret;
> > +
> > +	afe = devm_kzalloc(dev, sizeof(*afe), GFP_KERNEL);
> > +	if (!afe)
> > +		return -ENOMEM;
> > +	platform_set_drvdata(pdev, afe);
> > +
> > +	afe->platform_priv = devm_kzalloc(dev, sizeof(*afe_priv),
> > GFP_KERNEL);
> > +	if (!afe->platform_priv)
> > +		return -ENOMEM;
> > +
> > +	afe_priv = afe->platform_priv;
> > +	afe->dev = &pdev->dev;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	afe->base_addr = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(afe->base_addr))
> > +		return PTR_ERR(afe->base_addr);
> > +
> > +	/* init audio related clock */
> > +	ret = mt8186_init_clock(afe);
> > +	if (ret) {
> > +		dev_err(dev, "init clock error, ret %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* init memif */
> > +	afe->memif_32bit_supported = 0;
> > +	afe->memif_size = MT8186_MEMIF_NUM;
> > +	afe->memif = devm_kcalloc(dev, afe->memif_size, sizeof(*afe-
> > >memif),
> > +				  GFP_KERNEL);
> > +
> > +	if (!afe->memif)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < afe->memif_size; i++) {
> > +		afe->memif[i].data = &memif_data[i];
> > +		afe->memif[i].irq_usage = memif_irq_usage[i];
> > +		afe->memif[i].const_irq = 1;
> > +	}
> > +
> > +	mutex_init(&afe->irq_alloc_lock);	/* needed when dynamic irq
> > */
> > +
> > +	/* init irq */
> > +	afe->irqs_size = MT8186_IRQ_NUM;
> > +	afe->irqs = devm_kcalloc(dev, afe->irqs_size, sizeof(*afe-
> > >irqs),
> > +				 GFP_KERNEL);
> > +
> > +	if (!afe->irqs)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < afe->irqs_size; i++)
> > +		afe->irqs[i].irq_data = &irq_data[i];
> > +
> > +	/* request irq */
> > +	irq_id = platform_get_irq(pdev, 0);
> > +	if (irq_id <= 0)
> > +		return dev_err_probe(dev, irq_id < 0 ? irq_id : -ENXIO,
> > +				     "no irq found");
> > +
> > +	ret = devm_request_irq(dev, irq_id, mt8186_afe_irq_handler,
> > +			       IRQF_TRIGGER_NONE,
> > +			       "Afe_ISR_Handle", (void *)afe);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "could not request_irq
> > for Afe_ISR_Handle\n");
> > +
> > +	ret = enable_irq_wake(irq_id);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "enable_irq_wake %d\n",
> > irq_id);
> > +
> > +	/* init sub_dais */
> > +	INIT_LIST_HEAD(&afe->sub_dais);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(dai_register_cbs); i++) {
> > +		ret = dai_register_cbs[i](afe);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret, "dai register i
> > %d fail\n", i);
> > +	}
> > +
> > +	/* init dai_driver and component_driver */
> > +	ret = mtk_afe_combine_sub_dai(afe);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "mtk_afe_combine_sub_dai
> > fail\n");
> > +
> > +	/* reset controller to reset audio regs before regmap cache */
> > +	rstc = devm_reset_control_get_exclusive(dev, "audiosys");
> > +	if (IS_ERR(rstc))
> > +		return dev_err_probe(dev, PTR_ERR(rstc), "could not get
> > audiosys reset\n");
> > +
> > +	ret = reset_control_reset(rstc);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to trigger audio
> > reset\n");
> > +
> > +	/* enable clock for regcache get default value from hw */
> > +	afe_priv->pm_runtime_bypass_reg_ctl = true;
> 
> 
> > +	pm_runtime_enable(dev);
> 
> What about....
> 
> 	ret = devm_pm_runtime_enable(dev);
> 	if (ret)
> 		return ret;
> 
> 	ret = pm_runtime_resume_and_get(dev);
> 	if (ret)
> 		return dev_err_probe(dev, ret, "failed to resume
> device\n");
> 

Yes, pm_runime_resume_and_get() will do error when
pm_runtime_get_sync().

> > +	ret = pm_runtime_get_sync(dev);
> > +	if (ret) {
> > +		dev_err(dev, "failed to resume device: %d\n", ret);
> > +		goto err_pm_disable;
> > +	}
> > +
> > +	afe->regmap = devm_regmap_init_mmio(dev, afe->base_addr,
> > +					    &mt8186_afe_regmap_config);
> > +	if (IS_ERR(afe->regmap)) {
> > +		ret = PTR_ERR(afe->regmap);
> > +		goto err_pm_disable;
> > +	}
> > +
> > +	/* others */
> > +	afe->mtk_afe_hardware = &mt8186_afe_hardware;
> > +	afe->memif_fs = mt8186_memif_fs;
> > +	afe->irq_fs = mt8186_irq_fs;
> > +	afe->get_dai_fs = mt8186_get_dai_fs;
> > +	afe->get_memif_pbuf_size = mt8186_get_memif_pbuf_size;
> > +
> > +	afe->runtime_resume = mt8186_afe_runtime_resume;
> > +	afe->runtime_suspend = mt8186_afe_runtime_suspend;
> > +
> > +	/* register platform */
> > +	dev_info(dev, "%s(), devm_snd_soc_register_component\n",
> > __func__);
> 
> This should be dev_dbg().
> 
Got it.
> > +
> > +	ret = devm_snd_soc_register_component(dev,
> > +					      &mt8186_afe_component,
> > +					      afe->dai_drivers,
> > +					      afe->num_dai_drivers);
> > +	if (ret) {
> > +		dev_err(dev, "err_dai_component\n");
> > +		goto err_pm_disable;
> > +	}
> > +
> > +	ret = pm_runtime_put_sync(dev);
> > +	if (ret) {
> > +		pm_runtime_get_noresume(dev);
> > +		dev_err(dev, "failed to suspend device: %d\n", ret);
> > +		goto err_pm_disable;
> > +	}
> > +	afe_priv->pm_runtime_bypass_reg_ctl = false;
> > +
> > +	regcache_cache_only(afe->regmap, true);
> > +	regcache_mark_dirty(afe->regmap);
> > +
> > +	return 0;
> > +
> > +err_pm_disable:
> > +	pm_runtime_put_noidle(dev);
> > +	pm_runtime_set_suspended(dev);
> > +	pm_runtime_disable(dev);
> 
> ...and then, since we're using devm_pm_runtime_enable, this call to
> pm_runtime_disable() can be removed.
> 

Yes, we can remove this 4 lines if we use devm_pm_runtime_enable() and
pm_runime_resume_and_get().

> > +
> > +	return ret;
> > +}
> > +
> > +static int mt8186_afe_pcm_dev_remove(struct platform_device *pdev)
> > +{
> > +	struct mtk_base_afe *afe = platform_get_drvdata(pdev);
> > +
> > +	pm_runtime_disable(&pdev->dev);
> 
> Here too.
> 

Got it.
> 
> Regards,
> Angelo




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux