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.
Date: Thu, 04 Mar 2021 14:45:51 +0100 [thread overview]
Message-ID: <s5h7dmn9gmo.wl-tiwai@suse.de> (raw)
In-Reply-To: <20210303182959.5322-4-vitalyr@opensource.cirrus.com>
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
next prev parent reply other threads:[~2021-03-04 13:47 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-03 18:29 [PATCH 0/4] ALSA: hda/cirrus: Add support for CS8409 HDA bridge and CS42L42 companion codec Vitaly Rodionov
2021-03-03 18:29 ` [PATCH 1/4] ALSA: hda/cirrus: Increase AUTO_CFG_MAX_INS from 8 to 18 Vitaly Rodionov
2021-03-03 18:29 ` [PATCH 2/4] ALSA: hda/cirrus: Add support for CS8409 HDA bridge and CS42L42 companion codec Vitaly Rodionov
2021-03-04 13:39 ` Takashi Iwai
2021-03-04 18:36 ` Vitaly Rodionov
2021-03-03 18:29 ` [PATCH 3/4] ALSA: hda/cirrus: Add jack detect interrupt support from " Vitaly Rodionov
2021-03-04 13:45 ` Takashi Iwai [this message]
2021-03-04 18:44 ` Vitaly Rodionov
2021-03-03 18:29 ` [PATCH 4/4] ALSA: hda/cirrus: Add Headphone and Headset MIC Volume Control Vitaly Rodionov
2021-03-04 13:49 ` Takashi Iwai
2021-03-04 18:45 ` Vitaly Rodionov
2021-03-03 18:53 [PATCH 0/4] ALSA: hda/cirrus: Add support for CS8409 HDA bridge and CS42L42 companion codec Vitaly Rodionov
2021-03-03 18:53 ` [PATCH 3/4] ALSA: hda/cirrus: Add jack detect interrupt support from " Vitaly Rodionov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=s5h7dmn9gmo.wl-tiwai@suse.de \
--to=tiwai@suse.de \
--cc=alsa-devel@alsa-project.org \
--cc=linux-kernel@vger.kernel.org \
--cc=patches@opensource.cirrus.com \
--cc=perex@perex.cz \
--cc=tiwai@suse.com \
--cc=vitalyr@opensource.cirrus.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).