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, 10 Jul 2023 16:30:53 +0200
Message-ID: <87351vzvmq.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Shenghao Ding <13916275206@139.com>
Cc:     broonie@kernel.org, devicetree@vger.kernel.org,
        krzysztof.kozlowski+dt@linaro.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, liam.r.girdwood@intel.com,
        mengdong.lin@intel.com, x1077012@ti.com, peeyush@ti.com,
        navada@ti.com
Subject: Re: [PATCH v2 1/3] ALSA: hda/tas2781: Add tas2781 HDA driver
In-Reply-To: <20230710041217.151099-1-13916275206@139.com>
References: <20230710041217.151099-1-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 Mon, 10 Jul 2023 06:12:15 +0200,
Shenghao Ding wrote:
> 
> Integrate tas2781 configs for Lenovo Laptops. All of the tas2781s in the
> laptop will be aggregated as one audio device. The code support realtek
> as the primary codec. Rename "struct cs35l41_dev_name" to
> "struct scodec_dev_name" for all other side codecs instead of the certain
> one.
> 
> Signed-off-by: Shenghao Ding <13916275206@139.com>
> 
> ---
> Changes in v2:
>  -  simplify the check of vendor id with Lenovo
>  - ThinkPad is one of Lenovo's brands, they suggested me to use
>    ALC269_FIXUP_THINKPAD_ACPI.
>  - Add comments on ACARD_SINGLE_RANGE_EXT_TLV
>  - Add the range check for tas_priv->tasdevice[] in tas2781_acpi_get_i2c_res.
>  - remove acpi_subsystem_id
>  - Issue in Laptop 0x17aa38be ACPI talbe caused codec->bus->pci->subsystem_device
>    is not equal to (codec->core.subsystem_id & 0xffff) in snd_hda_pick_fixup.
>    The former is 0x3802 and the latter is 0x38be leads to getting the wrong
>    fixup_id and enter into the wrong entry. Although, this issue has been raised
>    to the laptop manufacturer, but the ACPI table is locked, cannot be changed
>    any more. Correct the wrong entry in the code.
>  - Rename "struct cs35l41_dev_name" to "struct scodec_dev_name" for all
>    other side codecs instead of one certain codec.
>  - Ignore the checkpatch complaints in alc269_fixup_tbl
>  - Drop the hunk which is irrelevant with my code in
>    alc_fixup_headset_mode_alc255_no_hp_mic
>  - Add tiwai@suse.de into Cc list
>  - remove useless index
>  - combine ALC287_FIXUP_TAS2781_I2C_2 and ALC287_FIXUP_TAS2781_I2C_4 together as
>    ALC287_FIXUP_TAS2781_I2C, The code view all the tas2781s in the laptop as one instance.
>  - delete the white space at the end of the line in alc_fixup_headset_mode_alc255_no_hp_mic

The patch looks almost fine.  Just a few things:

> @@ -6730,12 +6730,32 @@ static int comp_match_cs35l41_dev_name(struct device *dev, void *data)
>  	return !strcmp(d + n, tmp);
>  }
>  
> +static int comp_match_tas2781_dev_name(struct device *dev,
> +	void *data)

The indentation in the patch doesn't look good.
I don't want to be too strict about it, but this inconsistent
indentation makes reading quite difficult, so better to be fixed.
In general, it's recommended to align the opened parenthesis.
(But, in the case above, you don't need to break the line at all.)

> @@ -10761,6 +10843,22 @@ static int patch_alc269(struct hda_codec *codec)
>  		codec->fixup_id = HDA_FIXUP_ID_NOT_SET;
>  	}
>  
> +	/* FIXME: Issue in Laptop 0x17aa38be ACPI talbe caused
> +	 * codec->bus->pci->subsystem_device is not equal to
> +	 * (codec->core.subsystem_id & 0xffff) in snd_hda_pick_fixup.
> +	 * The former is 0x3802 and the latter is 0x38be leads to getting the
> +	 * wrong fixup_id and enter into the wrong entry. Although, this issue
> +	 * has been raised to the laptop manufacturer, but the ACPI table is
> +	 * locked, cannot be changed any more. Correct the wrong entry in the
> +	 * code.
> +	 */
> +	if (codec->fixup_id == ALC287_FIXUP_YOGA7_14ITL_SPEAKERS &&
> +		codec->core.vendor_id == 0x10ec0287 &&
> +		codec->core.subsystem_id == 0x17aa38be) {
> +		codec_dbg(codec, "Clear wrong fixup for 17aa38be\n");
> +		codec->fixup_id = ALC287_FIXUP_TAS2781_I2C;
> +	}

This should be rather applied differently.  A similar workaround was
found in the commit 56ec3e755bd1041d35bdec020a99b327697ee470
    ALSA: hda/realtek: Apply fixup for Lenovo Yoga Duet 7 properly


thanks,

Takashi