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@139.com
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@suse.de 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