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

[김은우 <ew.kim@samsung.com>]

Thank you for your review.
We will proceed to remove unnecessary Doxygen comments and logs as suggested.

Based on the feedback provided, we will revise the work accordingly and resubmit it for further review.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Wednesday, July 9, 2025 10:58 PM
> To: ew.kim@samsung.com; s.nawrocki@samsung.com; lgirdwood@gmail.com;
> broonie@kernel.org; perex@perex.cz; tiwai@suse.com
> Cc: linux-sound@vger.kernel.org; alsa-devel@alsa-project.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] ASoC: samsung: Implement abox generic structure
> 
> 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