On Mon, 10 Jul 2023 06:12:16 +0200,
Shenghao Ding wrote:
>
> Create tas2781 side codec HDA driver for Lenovo Laptops. The quantity
> of the speakers has been define in ACPI. All of the tas2781s in the
> laptop will be aggregated as one audio speaker. The code supports
> realtek codec as the primary codec. Code offers several controls for
> digtial/analog gain setting during playback, and other for eq params
> setting in case of different audio profiles, such as music, voice,
> movie, etc.
>
> Signed-off-by: Shenghao Ding <13916275206@xxxxxxx>
>
> ---
> Changes in v2:
> - All the controls set as const static
> - Add descriptions for tas2781_save_calibration
> - remove global addr handling in the code
> - checking subid in switch statement in function tas2781_hda_bind
> - add force firmware load Kcontrol
> - rename the kcontrol name to be more undertandable
> - remove Superfluous cast in tasdevice_fw_ready
> - correct weird line break in function tas2781_acpi_get_i2c_resource
> - correct Referencing adev after acpi_dev_put() in tas2781_hda_read_acpi
> - As to checking the given value in tasdevice_set_profile_id, it seems done
> by the tasdevice_info_profile
> - replace strcpy with strscpy in tas2781_hda_read_acpi
> - rewrite the subid judgement
> - Add tiwai@xxxxxxx into Cc list
> - remove the cast in tas2781_acpi_get_i2c_resource
> - remove else in tas2781_acpi_get_i2c_resource
> - fix the return value in tasdevice_set_profile_id
> - remove unneeded NL in tasdevice_config_get
> - Unifiy the comment style
> - remove ret = 0 in tasdevice_fw_ready
> - remove ret in tas2781_save_calibration
> - remove unused ret in tas2781_hda_playback
> - add force firmware load Kcontrol
The new version looks much better. Another few minor comments:
> +static int tas2781_acpi_get_i2c_res(struct acpi_resource *ares,
> + void *data)
The indentation could be improved in this patch, too.
Not only this line but in many places.
> +static int tas2781_hda_read_acpi(struct tasdevice_priv *tas_priv,
> + const char *hid)
> +{
...
> +err:
> + dev_err(tas_priv->dev, "Failed acpi ret: %d\n", ret);
Too ambiguous error message. It's hard to spot what the error is only
from this text.
> +static int tasdevice_set_profile_id(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
> + int ret = 0;
> +
> + if (tas_priv->rcabin.profile_cfg_id !=
> + ucontrol->value.integer.value[0]) {
> + tas_priv->rcabin.profile_cfg_id =
> + ucontrol->value.integer.value[0];
You should have a sanity check of the given values. User-space may
pass any arbitrary value, and ALSA core doesn't always filter invalid
values. Ditto for other *_put() callbacks.
> +static void tas2781_apply_calib(struct tasdevice_priv *tas_priv)
> +{
> + const unsigned char page_array[CALIB_MAX] = {
> + 0x17, 0x18, 0x18, 0x0d, 0x18
> + };
Missing static.
> + const unsigned char rgno_array[CALIB_MAX] = {
> + 0x74, 0x0c, 0x14, 0x3c, 0x7c
> + };
Ditto.
> +static void tas2781_hda_remove(struct device *dev)
> +{
> + struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> +
> + pm_runtime_get_sync(tas_priv->dev);
> + pm_runtime_disable(tas_priv->dev);
> +
> +
Too many blank lines.
thanks,
Takashi
