linux-kernel - Re: [PATCH V2] ALSA: hda/senarytech: add senarytech codec support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87ikydnj9z.wl-tiwai@suse.de>
Date: Thu, 13 Jun 2024 14:01:44 +0200
From: Takashi Iwai <tiwai@...e.de>
To: bo liu <bo.liu@...arytech.com>
Cc: perex@...ex.cz,
	tiwai@...e.com,
	linux-sound@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] ALSA: hda/senarytech: add senarytech codec support

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@...arytech.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