Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 04 Mar 2021 14:45:51 +0100
Message-ID: <s5h7dmn9gmo.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Vitaly Rodionov <vitalyr@opensource.cirrus.com>
Cc:     Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
        <alsa-devel@alsa-project.org>, <patches@opensource.cirrus.com>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/4] ALSA: hda/cirrus: Add jack detect interrupt support from CS42L42 companion codec.
In-Reply-To: <20210303182959.5322-4-vitalyr@opensource.cirrus.com>
References: <20210303182959.5322-1-vitalyr@opensource.cirrus.com>
        <20210303182959.5322-4-vitalyr@opensource.cirrus.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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?


> +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...

> +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.

> +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.


thanks,

Takashi