Re: [PATCH v4] ASoC: rt5645: Add the HW EQ for the customized speaker output of Google Celes

From: Mark Brown <broonie@kernel.org>
To: Oder Chiou <oder_chiou@realtek.com>
Cc: Jack Yu <jack.yu@realtek.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	Gary_lin <gary_lin@realtek.com>, John Lin <john.lin@realtek.com>,
	"woojoo.lee@samsung.com" <woojoo.lee@samsung.com>,
	Bard Liao <bardliao@realtek.com>, Flove <flove@realtek.com>
Subject: Re: [PATCH v4] ASoC: rt5645: Add the HW EQ for the customized speaker output of Google Celes
Date: Wed, 21 Oct 2015 11:34:04 +0100	[thread overview]
Message-ID: <20151021103404.GK32054@sirena.org.uk> (raw)
In-Reply-To: <7EB0DE829A537248AF2ED30C97D12694F9DE17@RTITMBSV03.realtek.com.tw>

[-- Attachment #1.1: Type: text/plain, Size: 2055 bytes --]

On Mon, Oct 19, 2015 at 06:34:52AM +0000, Oder Chiou wrote:
> > The userspace interface is now what I'd expec tbut I'm still quite
> > confused by this.  Why is this not a normal bytes control?  There's
> > something going on with private data here but I'm not sure what it's
> > supposed to do over writing to the device - a changelog might've
> > helped...

> We want to set the register table to the following struct byte-by-byte.
> struct rt5645_eq_param_s {
> 	unsigned short reg;
> 	unsigned short val;
> };
> Due to the length and target registers of the table are variant, we allocate
> the maximum size of register settings to store the data, and they should be
> controlled by DAPM for following the sequence, so the settings will be
> applied to the hardware in the speaker event of DAPM.

This *really* needed to be called out in the changelog.  Why are we
specifying the parameters in this format rather than just letting people
set the coefficients like other drivers do?  This is extremely unusual
and basically lets userspace write anything they like anywhere in the
device as part of the coefficients which doesn't seem like a good idea.
Look at how other drivers do bytes controls.

> > Why is the hweq setting part of platform data (and why is the platform
> > data for a specific system being set as part of this patch)?  This is
> > just a setting that can be set, there's nothing system specific about it
> > and it's not like we're even passing in a system specific tuning here.

> In the default, we want to disable the HW EQ function, and it is only enabled
> by the customers' request, so we set it in the platform data. The parameters
> of the HW EQ only can be passed by ALSA binary control.

This doesn't make much sense in an open system like Linux - the user can
always change the code to enable this, it's just getting in the way of
providing the configurability to force them to change their DT or kernel
and if they need to provide the settings from userspace anyway the
switch in the kernel isn't really doing anything.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]