Re: [PATCH 3/4] ALSA: hda/cirrus: Add jack detect interrupt support from CS42L42 companion codec.

[Vitaly Rodionov <vitalyr@opensource.cirrus.com>]

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