From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DF3B5C433E6 for ; Thu, 4 Mar 2021 13:47:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BAD2964F43 for ; Thu, 4 Mar 2021 13:47:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239116AbhCDNrA (ORCPT ); Thu, 4 Mar 2021 08:47:00 -0500 Received: from mx2.suse.de ([195.135.220.15]:40002 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233480AbhCDNqe (ORCPT ); Thu, 4 Mar 2021 08:46:34 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 0AAB7AE47; Thu, 4 Mar 2021 13:45:53 +0000 (UTC) Date: Thu, 04 Mar 2021 14:45:51 +0100 Message-ID: From: Takashi Iwai To: Vitaly Rodionov Cc: Jaroslav Kysela , Takashi Iwai , , , 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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