Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 03 Jul 2023 17:41:18 +0200
Message-ID: <87mt0d3sv5.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Shenghao Ding <13916275206@139.com>
Cc:     broonie@kernel.org, devicetree@vger.kernel.org, robh+dt@kernel.org,
        lgirdwood@gmail.com, perex@perex.cz,
        pierre-louis.bossart@linux.intel.com, kevin-lu@ti.com,
        shenghao-ding@ti.com, alsa-devel@alsa-project.org,
        linux-kernel@vger.kernel.org, x1077012@ti.com, peeyush@ti.com,
        navada@ti.com
Subject: Re: [PATCH v1 2/3] ALSA: hda/tas2781: Add tas2781 HDA driver
In-Reply-To: <20230702081857.799693-2-13916275206@139.com>
References: <20230702081857.799693-1-13916275206@139.com>
        <20230702081857.799693-2-13916275206@139.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Sun, 02 Jul 2023 10:18:56 +0200,
Shenghao Ding wrote:
> 
> Create tas2781 side codec HDA driver for Lenovo Laptops. All of the
> tas2781s in the laptop will be aggregated as one audio speaker. The
> code supports realtek as the primary codec.

First of all, again, it's too short description.  You added the code
to bind, but also add lots of own control elements.  There is *NO*
description / explanation about those at all.  User have no idea
what is it and how can be used.

So, please make the patch descriptive.


> @@ -0,0 +1,874 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// TAS2781 HDA I2C driver
> +//
> +// Copyright 2023 Texas Instruments, Inc.
> +//
> +// Author: Shenghao Ding <shenghao-ding@ti.com>
> +
> +#include <linux/acpi.h>
> +#include <linux/crc8.h>
> +#include <linux/crc32.h>
> +#include <linux/efi.h>
> +#include <linux/firmware.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <sound/hda_codec.h>
> +#include <sound/soc.h>
> +#include <sound/tas2781.h>
> +#include <sound/tlv.h>
> +#include <sound/tas2781-tlv.h>
> +
> +#include "hda_local.h"
> +#include "hda_auto_parser.h"
> +#include "hda_component.h"
> +#include "hda_jack.h"
> +#include "hda_generic.h"
> +
> +#define TASDEVICE_SPEAKER_CALIBRATION_SIZE	20
> +
> +#define ACARD_SINGLE_RANGE_EXT_TLV(xname, xreg, xshift, xmin, xmax, xinvert, \
> +	xhandler_get, xhandler_put, tlv_array) \
> +{	.iface = SNDRV_CTL_ELEM_IFACE_CARD, .name = (xname),\
> +	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |\
> +		 SNDRV_CTL_ELEM_ACCESS_READWRITE,\
> +	.tlv.p = (tlv_array), \
> +	.info = snd_soc_info_volsw_range, \
> +	.get = xhandler_get, .put = xhandler_put, \
> +	.private_value = (unsigned long)&(struct soc_mixer_control) \
> +		{.reg = xreg, .rreg = xreg, .shift = xshift, \
> +		 .rshift = xshift, .min = xmin, .max = xmax, \
> +		 .invert = xinvert} }

Is the only difference from the standard macro is the iface type?
Comment it so.

> +static int tas2781_acpi_get_i2c_res(struct acpi_resource *ares,
> +	void *data)
> +{
> +	struct tasdevice_priv *tas_priv = data;
> +	struct acpi_resource_i2c_serialbus *sb;
> +
> +	if (i2c_acpi_get_i2c_resource(ares, &sb)) {
> +		if (sb->slave_address != TAS2781_GLOBAL_ADDR) {
> +			tas_priv->tasdevice[tas_priv->ndev].dev_addr =
> +				(unsigned int)sb->slave_address;
> +			tas_priv->ndev++;
> +			goto out;
> +		}
> +	}
> +out:
> +	return 1;
> +}

With the code above, tas_priv->ndev might overflow
tas_priv->tasdevice[] if a buggy ACPI data is given.
Add the range check.

> +static int tas2781_hda_read_acpi(struct tasdevice_priv *tas_priv,
> +	const char *hid)
> +{
> +	struct acpi_device *adev;
> +	struct device *physdev;
> +	LIST_HEAD(resources);
> +	const char *sub;
> +	int ret;
> +
> +	adev = acpi_dev_get_first_match_dev(hid, NULL, -1);
> +	if (!adev) {
> +		dev_err(tas_priv->dev,
> +			"Failed to find an ACPI device for %s\n", hid);
> +		return -ENODEV;
> +	}
> +
> +	ret = acpi_dev_get_resources(adev, &resources,
> +		tas2781_acpi_get_i2c_res, tas_priv);
> +	if (ret < 0)
> +		goto err;
> +
> +	acpi_dev_free_resource_list(&resources);
> +	strscpy(tas_priv->dev_name, hid, sizeof(tas_priv->dev_name));
> +	physdev = get_device(acpi_get_first_physical_node(adev));
> +	acpi_dev_put(adev);
> +
> +	sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev));
> +	if (IS_ERR(sub))
> +		sub = NULL;
> +
> +	tas_priv->acpi_subsystem_id = sub;
> +
> +	put_device(physdev);
> +
> +	return 0;

Isn't NULL sub treated as an error?

> +static int tasdevice_create_control(struct tasdevice_priv *tas_priv)
> +{
> +	char prof_ctrl_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> +	struct hda_codec *codec = tas_priv->codec;
> +	struct snd_kcontrol_new prof_ctrl = {
> +		.name = prof_ctrl_name,
> +		.iface = SNDRV_CTL_ELEM_IFACE_CARD,
> +		.info = tasdevice_info_profile,
> +		.get = tasdevice_get_profile_id,
> +		.put = tasdevice_set_profile_id,
> +	};
> +	int ret;
> +
> +	/* Create a mixer item for selecting the active profile */
> +	scnprintf(prof_ctrl_name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN,
> +		"Speaker Profile Id");

It's a constant name, so we can drop the prof_ctrl_name temp buffer
and pass the name string directly, no?
Then prof_ctrl can be a const static, too.

> +static int tasdevice_dsp_create_ctrls(struct tasdevice_priv
> +	*tas_priv)
> +{
> +	char prog_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> +	char conf_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> +	struct hda_codec *codec = tas_priv->codec;
> +	struct snd_kcontrol_new prog_ctl = {
> +		.name = prog_name,
> +		.iface = SNDRV_CTL_ELEM_IFACE_CARD,
> +		.info = tasdevice_info_programs,
> +		.get = tasdevice_program_get,
> +		.put = tasdevice_program_put,
> +	};
> +	struct snd_kcontrol_new conf_ctl = {
> +		.name = conf_name,
> +		.iface = SNDRV_CTL_ELEM_IFACE_CARD,
> +		.info = tasdevice_info_configurations,
> +		.get = tasdevice_config_get,
> +		.put = tasdevice_config_put,
> +	};
> +	int ret;
> +
> +	scnprintf(prog_name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN,
> +		"Speaker Program Id");
> +	scnprintf(conf_name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN,
> +		"Speaker Config Id");

Ditto.

> +static int tas2781_save_calibration(struct tasdevice_priv *tas_priv)
> +{
> +	efi_guid_t efi_guid = EFI_GUID(0x02f9af02, 0x7734, 0x4233, 0xb4, 0x3d,
> +		0x93, 0xfe, 0x5a, 0xa3, 0x5d, 0xb3);
> +	static efi_char16_t efi_name[] = L"CALI_DATA";
> +	struct tm *tm = &tas_priv->tm;
> +	unsigned int attr, crc;
> +	unsigned int *tmp_val;
> +	efi_status_t status;
> +
> +	/* Lenovo devices */
> +	if (tas_priv->catlog_id == LENOVO)
> +		efi_guid = EFI_GUID(0x1f52d2a1, 0xbb3a, 0x457d, 0xbc, 0x09,
> +			0x43, 0xa3, 0xf4, 0x31, 0x0a, 0x92);
> +
> +	tas_priv->cali_data.total_sz = 0;
> +	/* Get real size of UEFI variable */
> +	status = efi.get_variable(efi_name, &efi_guid, &attr,
> +		&tas_priv->cali_data.total_sz, tas_priv->cali_data.data);
> +	if (status == EFI_BUFFER_TOO_SMALL) {
> +		/* Allocate data buffer of data_size bytes */
> +		tas_priv->cali_data.data = devm_kzalloc(tas_priv->dev,
> +			tas_priv->cali_data.total_sz, GFP_KERNEL);
> +		if (!tas_priv->cali_data.data)
> +			return -ENOMEM;
> +		/* Get variable contents into buffer */
> +		status = efi.get_variable(efi_name, &efi_guid, &attr,
> +			&tas_priv->cali_data.total_sz,
> +			tas_priv->cali_data.data);
> +		if (status != EFI_SUCCESS)
> +			return -EINVAL;
> +	}
> +
> +	tmp_val = (unsigned int *)tas_priv->cali_data.data;
> +
> +	crc = crc32(~0, tas_priv->cali_data.data, 84) ^ ~0;
> +	dev_dbg(tas_priv->dev, "cali crc 0x%08x PK tmp_val 0x%08x\n",
> +		crc, tmp_val[21]);
> +
> +	if (crc == tmp_val[21]) {
> +		time64_to_tm(tmp_val[20], 0, tm);
> +		dev_dbg(tas_priv->dev, "%4ld-%2d-%2d, %2d:%2d:%2d\n",
> +			tm->tm_year, tm->tm_mon, tm->tm_mday,
> +			tm->tm_hour, tm->tm_min, tm->tm_sec);
> +		tas2781_apply_calib(tas_priv);
> +	} else
> +		tas_priv->cali_data.total_sz = 0;
> +
> +	return 0;
> +}

What does the function do actually?  I can guess, but it's all too
much black magic there.  Please describe more.


> +static int tas2781_hda_bind(struct device *dev, struct device *master,
> +	void *master_data)
> +{
> +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> +	struct hda_component *comps = master_data;
> +	struct hda_codec *codec;
> +	unsigned int subid;
> +	int ret;
> +
> +	if (!comps || tas_priv->index < 0 ||
> +		tas_priv->index >= HDA_MAX_COMPONENTS)
> +		return -EINVAL;
> +
> +	comps = &comps[tas_priv->index];
> +	if (comps->dev)
> +		return -EBUSY;
> +
> +	codec = comps->codec;
> +	subid = codec->core.subsystem_id & 0xFFFF;
> +
> +	//Lenovo devices
> +	switch (subid) {
> +	case 0x387d:
> +	case 0x387e:
> +	case 0x3881:
> +	case 0x3884:
> +	case 0x3886:
> +	case 0x38a7:
> +	case 0x38a8:
> +	case 0x38ba:
> +	case 0x38bb:
> +	case 0x38be:
> +	case 0x38bf:
> +	case 0x38c3:
> +	case 0x38cb:
> +	case 0x38cd:
> +		tas_priv->catlog_id = LENOVO;
> +		break;
> +	default:
> +		tas_priv->catlog_id = OTHERS;
> +		break;
> +	}

Listing up the explicit SSID here *again* looks bad.  It means that we
have those SSIDs in two distinct places, and that's very error-prone.
Can we put the info in the quirk side (patch_realtek.c) somehow?  Or
can it be simply a check of vendor id with Lenovo?


thanks,

Takashi