Re: [PATCH] ALSA: hda: Add a new CM9825 standard driver.

[Takashi Iwai]

On Mon, 09 Sep 2024 04:23:40 +0200,
Leo Tsai wrote:
> 
> The CM9825 is a High Definition Audio Codec.
> There are 2 independent stereo outputs, one of the stereo
> outputs is cap-less with HP AMP, and the other is line out to
> connect the active speaker. The inputs can be Line-in and MIC-in.
> 
> Signed-off-by: Leo Tsai <antivirus621@xxxxxxxxx>
> ---
>  sound/pci/hda/patch_cmedia.c | 268 +++++++++++++++++++++++++++++++++++
>  1 file changed, 268 insertions(+)
>  mode change 100644 => 100755 sound/pci/hda/patch_cmedia.c
> 
> diff --git a/sound/pci/hda/patch_cmedia.c b/sound/pci/hda/patch_cmedia.c
> old mode 100644
> new mode 100755

You changed the file permission mistakenly here?

> index 2ddd33f8dd6c..3195261a7d2c
> --- a/sound/pci/hda/patch_cmedia.c
> +++ b/sound/pci/hda/patch_cmedia.c
(snip)
> +	{0x3C, AC_VERB_SET_AMP_GAIN_MUTE | 0x0a0, 0x2d},	/* Gain set */
> +	{0x3C, AC_VERB_SET_AMP_GAIN_MUTE | 0x090, 0x2d},	/* Gain set */

FWIW, those are equivalent with:
	{0x3C, AC_VERB_SET_AMP_GAIN_MUTE, AC_AMP_SET_OUTPUT | AC_AMP_SET_RIGHT | 0x2d},	/* Gain set */
	{0x3C, AC_VERB_SET_AMP_GAIN_MUTE, AC_AMP_SET_OUTPUT | AC_AMP_SET_LEFT | 0x2d},	/* Gain set */

> +static void cm9825_unsol_hp_delayed(struct work_struct *work)
> +{
> +	struct cmi_spec *spec =
> +	    container_of(to_delayed_work(work), struct cmi_spec, unsol_hp_work);
> +	struct hda_jack_tbl *jack;
> +	hda_nid_t hp_pin = spec->gen.autocfg.hp_pins[0];
> +	bool hp_jack_plugin = false;
> +	int err = 0;

Both are unnecessary initializations.

> +static int cm9825_resume(struct hda_codec *codec)
> +{
> +	struct cmi_spec *spec = codec->spec;
> +	hda_nid_t hp_pin = 0;
> +	bool hp_jack_plugin = false;
> +	int err;
> +
> +	err =
> +	    snd_hda_codec_write(spec->codec, 0x42, 0,
> +				AC_VERB_SET_PIN_WIDGET_CONTROL, 0x00);
> +	if (err)
> +		codec_dbg(codec, "codec_write err %d\n", err);
> +
> +	/* hibernation resume needs the full chip initialization */
> +	if (cmi_is_s4_resume(codec))
> +		codec_dbg(spec->codec, "resume from S4\n");

Is the comment above correct?
It only shows a debug print and does nothing else, no?

> @@ -32,6 +252,53 @@ static const struct hda_codec_ops cmi_auto_patch_ops = {
>  	.unsol_event = snd_hda_jack_unsol_event,
>  };
>  
> +static int patch_cm9825(struct hda_codec *codec)
> +{
(snip)
> +	INIT_DELAYED_WORK(&spec->unsol_hp_work, cm9825_unsol_hp_delayed);

If you use the delayed work, it has to be cleared at the destructor
via cancel_delayed_work().  That is, you'd need the own ops.free
callback implementation.

And if doing so, it's better to put this initialization earlier (right
after the alloc of spec and its field initializations), so that you
can call cancel_delayed_work() unconditionally.


thanks,

Takashi