Received-SPF: pass (google.com: domain of linux-kernel+bounces-213162-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 13 Jun 2024 14:01:44 +0200
Message-ID: <87ikydnj9z.wl-tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: bo liu <bo.liu@senarytech.com>
Cc: perex@perex.cz,
	tiwai@suse.com,
	linux-sound@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2] ALSA: hda/senarytech: add senarytech codec support
In-Reply-To: <20240606015703.37118-1-bo.liu@senarytech.com>
References: <20240606015703.37118-1-bo.liu@senarytech.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
Precedence: bulk
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII

On Thu, 06 Jun 2024 03:57:03 +0200,
bo liu wrote:
> 
> Add initial Senarytech codec support for SN6186. Note that this hda
> patch relies on the configuration default registers to be set correctly
> (normally by BIOS/firmware) in order for it to set up pin widgets
> properly.
> 
> Signed-off-by: bo liu <bo.liu@senarytech.com>

The code looks almost fine, just a few nitpicking:

> --- a/sound/pci/hda/Makefile
> +++ b/sound/pci/hda/Makefile
> @@ -24,6 +24,7 @@ snd-hda-codec-cs8409-y :=	patch_cs8409.o patch_cs8409-tables.o
>  snd-hda-codec-ca0110-y :=	patch_ca0110.o
>  snd-hda-codec-ca0132-y :=	patch_ca0132.o
>  snd-hda-codec-conexant-y :=	patch_conexant.o
> +snd-hda-codec-senarytech-objs :=patch_senarytech.o

Use *-y instead of *-objs.  The other entries have been corrected
recently.

> +/* parse EAPDs */
> +static void senary_auto_parse_eapd(struct hda_codec *codec)
> +{
> +	struct senary_spec *spec = codec->spec;
> +	hda_nid_t nid;
> +
> +	for_each_hda_codec_node(nid, codec) {
> +		if (get_wcaps_type(get_wcaps(codec, nid)) != AC_WID_PIN)
> +			continue;
> +		if (!(snd_hda_query_pin_caps(codec, nid) & AC_PINCAP_EAPD))
> +			continue;
> +		spec->eapds[spec->num_eapds++] = nid;
> +		if (spec->num_eapds >= ARRAY_SIZE(spec->eapds))
> +			break;
> +	}
> +
> +	/* NOTE: below is a wild guess; if we have more than two EAPDs,
> +	 * it's a new chip, where EAPDs are supposed to be associated to
> +	 * pins, and we can control EAPD per pin.
> +	 * OTOH, if only one or two EAPDs are found, it's an old chip,
> +	 * thus it might control over all pins.
> +	 */
> +	if (spec->num_eapds > 2)
> +		spec->dynamic_eapd = 1;

Do you still need to verify this?  This is copied from Conexant
driver, and they needed this workaround due to different handling per
chip model.  Unless needed, better to drop (or assuming always true)
this condition for simplicity.

> +#ifdef CONFIG_PM
> +static int senary_auto_suspend(struct hda_codec *codec)
> +{
> +	senary_auto_shutdown(codec);
> +	return 0;
> +}
> +#endif

The latest code dropped the dependency on CONFIG_PM.  Remove ifdef.

> +static const struct hda_codec_ops senary_auto_patch_ops = {
> +	.build_controls = snd_hda_gen_build_controls,
> +	.build_pcms = snd_hda_gen_build_pcms,
> +	.init = senary_auto_init,
> +	.free = senary_auto_free,
> +	.unsol_event = snd_hda_jack_unsol_event,
> +#ifdef CONFIG_PM
> +	.suspend = senary_auto_suspend,
> +	.check_power_status = snd_hda_gen_check_power_status,
> +#endif

Ditto.


thanks,

Takashi