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

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

Thank you for your detailed review.
We will proceed to remove unnecessary logs and code, as well as make
changes to some APIs accordingly.

Based on the feedback provided during your review, we will revise our work
and submit it again upon completion.

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Wednesday, July 9, 2025 5:47 PM
> To: ew.kim@samsung.com
> Cc: s.nawrocki@samsung.com; lgirdwood@gmail.com; perex@perex.cz;
> tiwai@suse.com; 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 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.