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

[Takashi Iwai <tiwai@xxxxxxx>]

On Mon, 09 Sep 2024 12:20:12 +0200,
Takashi Iwai wrote:
> 
> 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.

And last but not least: don't forget to put v2 prefix to the subject
at the next submission, e.g.
  Subject: [PATCH v2] ALSA: hda: ....


Takashi