ASoC: rt5645: Add the HW EQ for the customized speaker output of Google Celes
Subsystems (3)
Oder Chiou
maintainerPATCH v4 1/1 ASoC: rt5645: Add the HW EQ for the customized speaker output of Google Celes
--- include/sound/rt5645.h | 2 + sound/soc/codecs/rt5645.c | 103 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 104 insertions(+), 1 deletion(-)
include/sound/rt5645.h +2 -0
@@ -23,6 +23,8 @@ struct rt5645_platform_data {unsigned int jd_mode;/* Invert JD when jack insert */ bool jd_invert; +/* Speaker HWEQ */ + bool hweq; };#endif
sound/soc/codecs/rt5645.c +102 -1
172 lines not on this page · raw message
Signature · 1 line
1.8.1.1.439.g50a6b54
Mark Brown
maintainerQuoted 1 line · Oder Chiou
Signed-off-by Oder Chiou <[email protected]>
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...
sound/soc/codecs/rt5645.c static const DECLARE_TLV_DB_RANGE(spk_clsd_tlv, · line 508
Quoted 12 lines
sound/soc/codecs/rt5645.c +12
-0+static int rt5645_hweq_get (struct snd_kcontrol *kcontrol, +struct snd_ctl_elem_value *ucontrol) +{ +struct snd_soc_component *component =snd_kcontrol_chip (kcontrol); +struct rt5645_priv *rt5645 =snd_soc_component_get_drvdata (component); +char *p = ucontrol->value.bytes.data; + +if (rt5645->pdata.hweq) +memcpy (p, rt5645->eq_param, +RT5645_HWEQ_NUM *sizeof (struct rt5645_eq_param_s )); + +return 0 ;
There's a validation function below - we should be using it when the user supplies data so they can tell if their settings took effect.
sound/soc/codecs/rt5645.c static int strago_quirk_cb(const struct dmi_system_id *id) · line 3304
Quoted 6 lines
sound/soc/codecs/rt5645.c +6
-0+static struct rt5645_platform_data celes_platform_data = { + .dmic1_data_pin =RT5645_DMIC1_DISABLE , + .dmic2_data_pin =RT5645_DMIC_DATA_IN2P , + .jd_mode =3 , + .hweq = true, +};
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.
Please, one change per patch and describe changes in the changelog.
Oder Chiou
maintainerQuoted 6 lines
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.
Quoted 3 lines
There's a validation function below - we should be using it when the user supplies data so they can tell if their settings took effect.
We will add the validation function in the function "rt5645_hweq_put", if the settings are validated, it will be copied to the private data "eq_param".
Quoted 5 lines
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.
Mark Brown
maintainerQuoted 7 lines · Oder Chiou
Quoted 5 lines
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.
Quoted 8 lines
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.
Quoted 8 lines
Quoted 4 lines
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.