Re: [PATCH v2 2/3] ALSA: hda/tas2781: Add tas2781 HDA driver

[Takashi Iwai <tiwai@suse.de>]

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