linux-kernel - Re: [PATCH v8 2/8] ASoC: mediatek: mt8186: add platform driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c8b888fc-dff9-c278-da10-6883c4277289@wanadoo.fr>
Date:   Sun, 26 Jun 2022 10:33:12 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     jiaxin.yu@...iatek.com
Cc:     Project_Global_Chrome_Upstream_Group@...iatek.com,
        aaronyu@...gle.com, alsa-devel@...a-project.org,
        angelogioacchino.delregno@...labora.com, broonie@...nel.org,
        devicetree@...r.kernel.org, julianbraha@...il.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-mediatek@...ts.infradead.org, matthias.bgg@...il.com,
        robh+dt@...nel.org, trevor.wu@...iatek.com, tzungbi@...gle.com
Subject: Re: [PATCH v8 2/8] ASoC: mediatek: mt8186: add platform driver

Le 25/06/2022 à 21:08, Jiaxin Yu a écrit :
> Add mt8186 platform and affiliated driver.
> 
> Signed-off-by: Jiaxin Yu <jiaxin.yu-NuS5LvNUpcJWk0Htik3J/w@...lic.gmane.org>
> ---
>   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  |  255 ++
>   sound/soc/mediatek/mt8186/mt8186-afe-pcm.c    | 3011 +++++++++++++++++
>   6 files changed, 3533 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

[...]

> +	MT8186_DAI_HOSTLESS_SRC_AAUDIO,
> +	MT8186_DAI_HOSTLESS_SRC_1,	/* just an exmpale */

example?

> +	MT8186_DAI_HOSTLESS_SRC_BARGEIN,
> +	MT8186_DAI_HOSTLESS_UL1,

[...]

> +#define MTK_SPK_I2S_0_STR "MTK_SPK_I2S_0"
> +#define MTK_SPK_I2S_1_STR "MTK_SPK_I2S_1"
> +#define MTK_SPK_I2S_2_STR "MTK_SPK_I2S_2"
> +#define MTK_SPK_I2S_3_STR "MTK_SPK_I2S_3"

Out of curiosity, why no 4?
Or, if related to mtk_spk_i2s_type below, why  6, 7, 8 and 9?

> +#define MTK_SPK_I2S_5_STR "MTK_SPK_I2S_5"
> +#define MTK_SPK_I2S_6_STR "MTK_SPK_I2S_6"
> +#define MTK_SPK_I2S_7_STR "MTK_SPK_I2S_7"
> +#define MTK_SPK_I2S_8_STR "MTK_SPK_I2S_8"
> +#define MTK_SPK_I2S_9_STR "MTK_SPK_I2S_9"
> +

[...]

> +
> +enum mtk_spk_i2s_type {
> +	MTK_SPK_I2S_TYPE_INVALID = -1,
> +	MTK_SPK_I2S_0,
> +	MTK_SPK_I2S_1,
> +	MTK_SPK_I2S_2,
> +	MTK_SPK_I2S_3,
> +	MTK_SPK_I2S_5,
> +	MTK_SPK_I2S_TYPE_NUM
> +};

[...]

> +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;
> +	}

There is a mt8186_deinit_clock() call in the remove function.
Should this also be called in the error handling path below?
Or should a devm_add_action_or_reset() be used to ease error handling?

> +
> +	/* 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);
> +

Nit: no need for an empty line here.

> +	if (!afe->memif)
> +		return -ENOMEM;
> +

[...]

> +
> +	return 0;
> +
> +err_pm_disable:
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_set_suspended(dev);
> +
> +	return ret;
> +}
> +
> +static int mt8186_afe_pcm_dev_remove(struct platform_device *pdev)
> +{
> +	struct mtk_base_afe *afe = platform_get_drvdata(pdev);
> +
> +	if (!pm_runtime_status_suspended(&pdev->dev))
> +		mt8186_afe_runtime_suspend(&pdev->dev);

Out of curiosity, is it normal to have some pm_runtime related code here 
that does not look the same as the one in the error handling of the probe?
(I don't know much about pm, but usually, .remove() functions and error 
handling in the probe look quite close)

> +
> +	mt8186_deinit_clock(afe);
> +
> +	return 0;
> +}
> +

[...]