Thank you for your review.
We will proceed to remove unnecessary initializations and defensive code as suggested. Additionally, we will take into account the other comments provided and rework the content accordingly before submitting it again for further review.
-----Original Message----- From: Christophe JAILLET christophe.jaillet@wanadoo.fr Sent: Thursday, July 10, 2025 3:10 AM 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
Le 09/07/2025 à 02:10, ew.kim@samsung.com a écrit :
From: ew kim ew.kim@samsung.com
Implemet basic abox generic drivers.
Implement.
This driver is a management driver for the generic drivers used in Automotive Abox, connecting them to SOC drivers. It supports various Exynos Automotive SOCs.
...
+int abox_generic_attach_soc_callback(struct device *soc_dev,
- SOC_IOCTL soc_ioctl)
+{
- struct abox_generic_data *generic_data = ABOX_GENERIC_DATA;
- dev_info(soc_dev, "%s(%d) Attach SoC IOCTL\n", __func__, __LINE__);
__LINE__ is only used in this function. Maybe it is a bit too much?
- if (!generic_data) {
dev_err(soc_dev, "%s Generic Drv is not ready\n", __func__);return -ENODATA;- }
- generic_data->soc_dev = soc_dev;
- generic_data->soc_ioctl = soc_ioctl;
- generic_data->num_of_rdma = generic_data->soc_ioctl(generic_data-
soc_dev,
ABOX_SOC_IOCTL_GET_NUM_OF_RDMA, NULL);- generic_data->num_of_wdma = generic_data->soc_ioctl(generic_data-
soc_dev,
ABOX_SOC_IOCTL_GET_NUM_OF_WDMA, NULL);- generic_data->num_of_uaif = generic_data->soc_ioctl(generic_data-
soc_dev,
ABOX_SOC_IOCTL_GET_NUM_OF_UAIF, NULL);- dev_info(soc_dev, "%s(%d) num_of_rdma:%d\n", __func__, __LINE__,
generic_data->num_of_rdma);
- dev_info(soc_dev, "%s(%d) num_of_wdma:%d\n", __func__, __LINE__,
generic_data->num_of_wdma);
- dev_info(soc_dev, "%s(%d) num_of_uaif:%d\n", __func__, __LINE__,
+generic_data->num_of_uaif);
- return 0;
+}
...
+struct device *abox_generic_find_fe_dev_from_rtd(struct +snd_soc_pcm_runtime *be) {
- struct abox_generic_data *generic_data = ABOX_GENERIC_DATA;
- struct snd_soc_dpcm *dpcm = NULL;
- struct snd_soc_pcm_runtime *fe = NULL;
- int stream_type = 0;
Unneeded and unusual init
- if (!generic_data)
return NULL;- for (stream_type = 0; stream_type <= SNDRV_PCM_STREAM_LAST;
stream_type++) {
int cmpnt_index = 0;struct snd_soc_component *component = NULL;for_each_dpcm_fe(be, stream_type, dpcm) {fe = dpcm->fe;if (fe)break;}if (!fe)continue;for_each_rtd_components(fe, cmpnt_index, component) {struct platform_device **pdev = NULL;int num_of_pcm_dev = 0;int i = 0;Unneeded and unusual init
if (stream_type == SNDRV_PCM_STREAM_PLAYBACK) {num_of_pcm_dev = generic_data-num_of_pcm_playback;
pdev = generic_data->pdev_pcm_playback;} else {num_of_pcm_dev = generic_data-num_of_pcm_capture;
pdev = generic_data->pdev_pcm_capture;}for (i = 0; i < num_of_pcm_dev; i++)if (pdev[i] && component->dev == &pdev[i]->dev)return component->dev;}- }
- return NULL;
+}
...
+static int abox_generic_resume(struct device *dev) {
- struct abox_generic_data *data = dev_get_drvdata(dev);
- int ret = 0;
- dev_info(dev, "%s start\n", __func__);
- if (!data) {
I don't think this can happen. (same for the suspend function)
dev_err(dev, "%s: Invalid abox generic data\n", __func__);return -ENODATA;- }
- dev_info(dev, "%s end\n", __func__);
- return ret;
return 0; to be more explicit?
+}
...
+static int abox_generic_read_property_from_dt(struct device *dev, +struct abox_generic_data *data) {
- struct device_node *np = dev->of_node;
- int ret = 0;
- ret = of_property_read_u32(np, "samsung,num-of-pcm_playback",
&data->num_of_pcm_playback);
- if (ret < 0) {
dev_err(dev, "%s property reading fail\n", "samsung,num-of-pcm_playback");
return ret;- }
- ret = of_property_read_u32(np, "samsung,num-of-pcm_capture", &data-
num_of_pcm_capture);
- if (ret < 0) {
dev_err(dev, "%s property reading fail\n", "samsung,num-of-pcm_capture");
return ret;- }
- ret = of_property_read_u32(np, "samsung,num-of-i2s-dummy-backend",
&data->num_of_i2s_dummy);
- if (ret < 0) {
dev_err(dev, "%s property reading fail\n", "samsung,num-of-i2s-dummy-backend");
return ret;- }
- return ret;
return 0; to be more explicit?
+}
+/**
- @cnotice
- @prdcode
- @Sub_SW_Component{abox generic}
- @ALM_Link {work item url}
- @purpose "Allocate memory for abox generic"
- @logic
- \image html
- @params
- @param{in, dev, struct:: device *, !NULL}
- @param{in, data, struct:: abox_gneric_data, !NULL}
- @param{out, data->pdev_pcm_playback, struct:: platform_device,
+!NULL}
- @param{out, data->pdev_pcm_capture, struct:: platform_device,
+!NULL}
- @endparam
- @retval{ret, int, 0, 0, > 0}
- */
+static int abox_generic_allocate_memory(struct device *dev, struct +abox_generic_data *data) {
- int ret = 0;
Unneeded (see below)
- data->pdev_pcm_playback = devm_kzalloc(dev,
devm_kcalloc()?
sizeof(struct platform_device *) * data->num_of_pcm_playback,GFP_KERNEL);
- if (!data->pdev_pcm_playback) {
dev_err(dev, "%s Can't allocate memory forpdev_pcm_playback\n", __func__);
ret = -ENOMEM;return ret;Not need for ret. return -ENOMEM; is less verbose.
- }
- data->pdev_pcm_capture = devm_kzalloc(dev,
devm_kcalloc()?
sizeof(struct platform_device *) * data->num_of_pcm_capture,GFP_KERNEL);
- if (!data->pdev_pcm_capture) {
dev_err(dev, "%s Can't allocate memory forpdev_pcm_capture\n", __func__);
ret = -ENOMEM;return ret;Not need for ret. return -ENOMEM; is less verbose.
- }
- return ret;
return 0; to be more explicit?
+}
+/**
- @cnotice
- @prdcode
- @Sub_SW_Component{abox generic}
- @ALM_Link {work item url}
- @purpose "Probing the abox generic"
- @logic
- \image html
- @params
- @param{in, pdev, struct platform_device *, !NULL}
- @param{in, pdev->dev, struct:: device, !NULL}
- @endparam
- @retval{ret, int, 0, 0, > 0}
- */
+static int samsung_abox_generic_probe(struct platform_device *pdev) {
- struct device *dev = &pdev->dev;
- struct device_node *np = dev->of_node;
- struct abox_generic_data *data;
- int ret = 0;
- dev_info(dev, "%s\n", __func__);
- data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
- if (!data) {
return -ENOMEM;- }
Extra { } not needed. checkpatch.pl or maybe checkpatch.pl --strict should catech it.
- data->pdev = pdev;
- ret = abox_generic_read_property_from_dt(dev, data);
- if (ret < 0) {
dev_err(dev, "%s Failed to read property. ret:%d\n",__func__,
+ret);
Using dev_err_probe() here and below would be less verbose.
return ret;- }
- ret = abox_generic_allocate_memory(dev, data);
- if (ret < 0) {
dev_err(dev, "%s Failed to allocate memory. ret:%d\n",__func__,
+ret);
dev_err() is already called in abox_generic_allocate_memory().
return ret;- }
- g_abox_generic_data = data;
- platform_set_drvdata(pdev, data);
- platform_register_drivers(abox_generic_sub_drivers,
ARRAY_SIZE(abox_generic_sub_drivers));
- ret = of_platform_populate(np, NULL, NULL, dev);
- if (ret < 0) {
dev_err(dev, "Failed to populate sub-platform_devices.ret:%d\n", ret);
return ret;- }
- return ret;
return 0; to be more explicit?
+}
+/**
- @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__);
- if (!data) {
This can not happen. data is set if the probe succeeds.
dev_err(dev, "%s: Invalid abox generic data\n", __func__);return;- }
- return;
Not needed.
+}
+/**
- @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;
Not needed.
+}
+static const struct of_device_id samsung_abox_generic_match[] = {
- {
.compatible = "samsung,abox_generic",- },
- {},
Trailing comma can be removed after a terminator.
+}; +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)
+};
...
CJ