Re: [PATCH] ASoC: samsung: Implement abox generic structure

[Krzysztof Kozlowski <krzk@kernel.org>]

On 09/07/2025 02:10, ew.kim@samsung.com wrote:
> +/**
> + * @cnotice
> + * @prdcode
> + * @Sub_SW_Component{abox generic}
> + * @ALM_Link {work item url}
> + * @purpose "Disbaling the abox generic"
> + * @logic "Disbale the abox generic"
> + * \image html
> + * @params
> + * @param{in, pdev->dev, struct::device, !NULL}
> + * @endparam
> + * @noret
> + */
> +static void samsung_abox_generic_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct abox_generic_data *data = dev_get_drvdata(dev);
> +
> +	dev_info(dev, "%s\n", __func__);

This is just poor code. Clean it up from all such oddities popular in
downstream. Look at upstream code. Do you see such code there? No.

> +
> +	if (!data) {
> +		dev_err(dev, "%s: Invalid abox generic data\n", __func__);
> +		return;
> +	}
> +	return;
> +}
> +
> +/**
> + * @cnotice
> + * @prdcode
> + * @Sub_SW_Component{abox generic}
> + * @ALM_Link {work item url}
> + * @purpose "shutdown of the abox generic"
> + * @logic "Disbale the abox hardware by calling the following function
> + * pm_runtime_disable(dev)"
> + * \image html
> + * @params
> + * @param{in, pdev->dev, struct:: device, !NULL}
> + * @endparam
> + * @noret
> + */
> +static void samsung_abox_generic_shutdown(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct abox_generic_data *data = dev_get_drvdata(dev);
> +
> +	if (!data) {
> +		dev_err(dev, "%s: Invalid abox generic data\n", __func__);
> +		return;
> +	}
> +	return;
> +}
> +
> +static const struct of_device_id samsung_abox_generic_match[] = {
> +	{
> +		.compatible = "samsung,abox_generic",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, samsung_abox_generic_match);
> +
> +static const struct dev_pm_ops samsung_abox_generic_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(abox_generic_suspend, abox_generic_resume)
> +};
> +
> +struct platform_driver samsung_abox_generic_driver = {
> +	.probe  = samsung_abox_generic_probe,
> +	.remove = samsung_abox_generic_remove,
> +	.shutdown = samsung_abox_generic_shutdown,
> +	.driver = {
> +		.name = "samsung-abox-generic",
> +		.owner = THIS_MODULE,

So that's indeed 2013 code you upstream. We really want you to clean it
up before you post some ancient stuff like that.


> +		.of_match_table = of_match_ptr(samsung_abox_generic_match),
> +		.pm = &samsung_abox_generic_pm,
> +	},
> +};
> +
> +module_platform_driver(samsung_abox_generic_driver);
> +/* Module information */

Useless comment.

> +MODULE_AUTHOR("Eunwoo Kim, <ew.kim@samsung.com>");
> +MODULE_DESCRIPTION("Samsung ASoC A-Box Generic Driver");
> +MODULE_ALIAS("platform:samsung-abox-generic");

No, drop. This was raised so many times already...

> +MODULE_LICENSE("GPL v2");
> +
> diff --git a/sound/soc/samsung/auto_abox/generic/include/abox_generic.h b/sound/soc/samsung/auto_abox/generic/include/abox_generic.h
> new file mode 100644
> index 000000000000..1c954272e2b5
> --- /dev/null
> +++ b/sound/soc/samsung/auto_abox/generic/include/abox_generic.h
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * ALSA SoC - Samsung ABOX Share Function and Data structure
> + * for Exynos specific extensions
> + *
> + * Copyright (C) 2013-2020 Samsung Electronics Co., Ltd.
> + *
> + * EXYNOS - sound/soc/samsung/abox/include/abox_generic.h

Same with paths. Do you see them anywhere in the kernel?

> + */
> +
> +#ifndef __SND_SOC_ABOX_GENERIC_BASE_H
> +#define __SND_SOC_ABOX_GENERIC_BASE_H
> +
> +#define ABOX_GENERIC_DATA		abox_generic_get_abox_generic_data();
> +
> +struct snd_soc_pcm_runtime;
> +
> +enum abox_soc_ioctl_cmd {
> +	ABOX_SOC_IOCTL_GET_NUM_OF_RDMA,
> +	ABOX_SOC_IOCTL_GET_NUM_OF_WDMA,
> +	ABOX_SOC_IOCTL_GET_NUM_OF_UAIF,
> +	ABOX_SOC_IOCTL_GET_SOC_TIMER,
> +	ABOX_SOC_IOCTL_SET_DMA_BUFFER,
> +	ABOX_SOC_IOCTL_SET_PP_POINTER,
> +	ABOX_SOC_IOCTL_SET_PERF_PERIOD,
> +	ABOX_SOC_IOCTL_CHECK_TIME_MUTEX,
> +	ABOX_SOC_IOCTL_CHECK_TIME_NO_MUTEX,
> +	ABOX_SOC_IOCTL_PCM_DUMP_INTR,
> +	ABOX_SOC_IOCTL_PCM_DUMP_CLOSE,
> +	ABOX_SOC_IOCTL_PCM_DUMP_ADD_CONTROL,
> +	ABOX_SOC_IOCTL_MAX
> +};
> +
> +typedef int (*SOC_IOCTL)(struct device *soc_dev, enum abox_soc_ioctl_cmd cmd, void *data);

Follow coding style.

> +
> +struct abox_generic_data {
> +	struct platform_device *pdev;
> +	struct platform_device **pdev_pcm_playback;
> +	struct platform_device **pdev_pcm_capture;
> +	unsigned int num_of_pcm_playback;
> +	unsigned int num_of_pcm_capture;
> +	unsigned int num_of_i2s_dummy;
> +	unsigned int num_of_rdma;
> +	unsigned int num_of_wdma;
> +	unsigned int num_of_uaif;
> +	struct device *soc_dev;
> +	SOC_IOCTL soc_ioctl;
> +};
> +
> +
> +/************ Internal API ************/

Then why do you expose it via header?

> +
> +struct abox_generic_data *abox_generic_get_abox_generic_data(void);
> +
> +int abox_generic_set_dma_buffer(struct device *pcm_dev);
> +
> +int abox_generic_request_soc_ioctl(struct device *generic_dev, enum abox_soc_ioctl_cmd cmd,
> +	void *data);
> +
> +int abox_generic_set_pp_pointer(struct device *pcm_dev);
> +
> +
> +
> +
> +/************ External API ************/
> +
> +extern struct device *abox_generic_find_fe_dev_from_rtd(struct snd_soc_pcm_runtime *be);

You cannot have external API. All API is internal first.

> +
> +extern struct platform_device *abox_generic_get_pcm_platform_dev(int pcm_id,
> +	int stream_type);
> +
Best regards,
Krzysztof