On 04/03/2021 1:45 pm, Takashi Iwai wrote:

On Wed, 03 Mar 2021 19:29:58 +0100, Vitaly Rodionov wrote:

...

@@ -1243,6 +1258,8 @@ static int patch_cs4213(struct hda_codec *codec) #define CIR_I2C_QWRITE 0x005D #define CIR_I2C_QREAD 0x005E

+static struct mutex cs8409_i2c_mux;

Any reason that this must be the global mutex? I suppose it can fit in own spec->i2c_mutex instead?

No,  there is no reason to have global mutex, will move it out to spec.

...

+static void cs8409_cs42l42_cap_sync_hook(struct hda_codec *codec,

• 			 struct snd_kcontrol *kcontrol,
  

• 			 struct snd_ctl_elem_value *ucontrol)
  

+{

• struct cs_spec *spec = codec->spec;
• unsigned int curval, expval;
• /* CS8409 DMIC Pin only allows the setting of the Stream Parameters in

• * Power State D0. When a headset is unplugged, and the path is switched to
  

• * the DMIC, the Stream is restarted with the new ADC, but this is done in
  

• * Power State D3. Restart the Stream now DMIC is in D0.
  

• */
  

• if (spec->gen.cur_adc == CS8409_CS42L42_DMIC_ADC_PIN_NID) {

• curval = snd_hda_codec_read(codec, spec->gen.cur_adc,
  

• 	0, AC_VERB_GET_CONV, 0);
  

• expval = (spec->gen.cur_adc_stream_tag << 4) | 0;
  

• if (curval != expval) {
  

• 	codec_dbg(codec, "%s Restarting Stream after DMIC switch\n", __func__);
  

• 	__snd_hda_codec_cleanup_stream(codec, spec->gen.cur_adc, 1);
  

• 	snd_hda_codec_setup_stream(codec, spec->gen.cur_adc,
  

• 			   spec->gen.cur_adc_stream_tag, 0,
  

• 			   spec->gen.cur_adc_format);
  

Hrm, this looks a big scary. We may need to reconsider how to handle this better later, but it's OK as long as you've tested with this code...

We have been thinking about this code, and we have some ideas , it was tested by us, DELL and Canonical and works.

But we would like to change it a bit later, and handle it in a more generic way.

...

+static int cs8409_cs42l42_init(struct hda_codec *codec) +{

• int ret = 0;
• ret = snd_hda_gen_init(codec);
• if (!ret) {

• /* On Dell platforms with suspend D3 mode support we
  

•  * have to re-initialise cs8409 bridge and companion
  

•  * cs42l42 codec
  

•  */
  

• snd_hda_sequence_write(codec, cs8409_cs42l42_init_verbs);
  

• snd_hda_sequence_write(codec, cs8409_cs42l42_add_verbs);
  

• cs8409_cs42l42_hw_init(codec);
  

Ah... the init stuff at resume appears finally here. This part should be in the second patch instead.

Yes, moving this into second patch.

...

+static int cs8409_cs42l42_exec_verb(struct hdac_device *dev,

• unsigned int cmd, unsigned int flags, unsigned int *res)
  

+{

• struct hda_codec *codec = container_of(dev, struct hda_codec, core);
• struct cs_spec *spec = codec->spec;
• unsigned int nid = 0;
• unsigned int verb = 0;

The blank line above should be removed.

...

• case CS8409_CS42L42_HP_PIN_NID:

• if (verb == AC_VERB_GET_PIN_SENSE) {
  

• 	*res = (spec->cs42l42_hp_jack_in)?AC_PINSENSE_PRESENCE:0;
  

The spaces are needed around operators. Similar coding-style issues are seen other places. Please try to run scripts/checkpatch.pl.

Fixed, and checked with scripts/checkpatch.pl. Base has 19 warnings. This patch will not introduce any new.

thanks,

Takashi

Thanks,

Vitaly