On Wed, Jul 09, 2025 at 09:10:02AM +0900, ew.kim@samsung.com wrote:
From: ew kim ew.kim@samsung.com
Implemet basic abox generic drivers. 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.
I can't really tell what the driver is supposed to do from this - what is abox? Looking at the code I'm not really much clearer, to a large extent because it doesn't seem to integrate with the subsystem (or other kernel subsystems) at all. It looks like this might be intended to control a DSP offload system, but it's not 100% clear, and it seems like there's an ioctl() interface which it looks like it's exposing internal abstractions to userspace. This needs to be some combination of much more clearly explained and better integrated with the existing kernel subsystems, right now I can't really review it effectively because it's hard for me to tell what the code is trying to do.
I've got a few very supreficial comments below but really there's a structural problem that needs to be addressed first.
+++ b/sound/soc/samsung/auto_abox/generic/abox_generic.c @@ -0,0 +1,568 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (c) 2020 Samsung Electronics Co., Ltd.
http://www.samsung.com/
It's now 2025...
Please also make the entire comment a C++ one so things look more intentional.
+//#define DEBUG
Just delete this, it can be added if needed.
+#include <linux/clk.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> +#include <linux/delay.h> +#include <linux/suspend.h> +#include <sound/soc.h> +#include <sound/soc-dapm.h> +#include <sound/pcm_params.h> +#include <linux/of_reserved_mem.h> +#include <linux/types.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/sched/clock.h> +#include <linux/ktime.h> +#include <linux/iommu.h> +#include <linux/clk-provider.h> +#include <linux/kmod.h> +#include <linux/umh.h> +#include <linux/string.h>
Do you really need all these headers?
+static struct abox_generic_data *g_abox_generic_data;
This isn't really the kernel style - neither the g_ naming, nor the concept of having a global for a driver.
+/**
- @cnotice
- @prdcode
- @Sub_SW_Component{abox_generic}
- @ALM_Link {work item url}
- @purpose "get value from virtual address"
- @logic "return global abox_generic_data"
- \image html
- @params
- @param{in, -, -, -}
- @endparam
- @retval {-, struct *abox_generic_data, !NULL, NULL}
- */
This is not the style for kernel comments, and doesn't seem to make terribly much sense.