On Fri, Jul 26, 2024 at 04:47:55PM +0800, Shenghao Ding wrote:
Add calibration related kcontrol for speaker impedance calibration and speaker leakage check for Chromebook
This is pretty hard to understand, it feels like there's a bunch of cleanup work in here along with the actual control addition and there's nothing really saying anything concrete about the actual controls to check that the controls do the right thing. It's hard to do anything but the most superficial review here since I don't really understand what the changes are trying to accomplish.
-/* pow(10, db/20) * pow(2,30) */ -static const unsigned char tas2563_dvc_table[][4] = {
- { 0X00, 0X00, 0X00, 0X00 }, /* -121.5db */
For example moving this to the header could be done separately (though perhaps it should just be exported rather than placed in the header)?
@@ -64,8 +64,8 @@ static int tasdevice_change_chn_book(struct tasdevice_priv *tas_priv, */ ret = regmap_write(map, TASDEVICE_PAGE_SELECT, 0); if (ret < 0) {
dev_err(tas_priv->dev, "%s, E=%d\n",__func__, ret);
dev_err(tas_priv->dev, "%s, E=%d channel:%d\n",__func__, ret, chn); goto out; }
This is another example of a random cleanup.
static void tasdev_load_calibrated_data(struct tasdevice_priv *priv, int i) {
- struct tasdevice_fw *cal_fmw = priv->tasdevice[i].cali_data_fmw;
- struct calidata *cali_data = &priv->cali_data;
- unsigned char *data = &cali_data->data[1]; struct tasdevice_calibration *cal;
- struct tasdevice_fw *cal_fmw;
- int k = i * (cali_data->cali_dat_sz + 1);
- int j, rc;
- cal_fmw = priv->tasdevice[i].cali_data_fmw;
- /* Load the calibrated data from cal bin file */
- if (!priv->is_user_space_calidata && cal_fmw) {
cal = cal_fmw->calibrations;
- /* No calibrated data for current devices, playback will go ahead. */
- if (!cal_fmw)
if (cal)load_calib_data(priv, &cal->dev_data);
It feels like there's an abstraction problem with the different ways to get calibration data. Perhaps each way of loading calibration data should parse the data into a standard format on load and then the rest of the code doesn't need to worry about where it came from?
@@ -67,8 +215,13 @@ static int tas2781_digital_getvol(struct snd_kcontrol *kcontrol, struct tasdevice_priv *tas_priv = snd_soc_component_get_drvdata(codec); struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value;
- int rc;
- mutex_lock(&tas_priv->codec_lock);
- rc = tasdevice_digital_getvol(tas_priv, ucontrol, mc);
- mutex_unlock(&tas_priv->codec_lock);
- return tasdevice_digital_getvol(tas_priv, ucontrol, mc);
- return rc;
}
Why all these locking changes? Could they be split out into a praparatory change?
+static int tasdevice_get_chip_id(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct snd_soc_component *codec = snd_soc_kcontrol_component(kcontrol);
- struct tasdevice_priv *tas_priv = snd_soc_component_get_drvdata(codec);
- ucontrol->value.integer.value[0] = tas_priv->chip_id;
- return 0;
+}
Should these chip ID controls be done separately? They don't seem super related.