From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 1/2] ASoC: Add support for Conexant CX2072X CODEC Date: Fri, 03 May 2019 09:18:26 +0200 Message-ID: References: <20190423141336.12568-1-tiwai@suse.de> <20190423141336.12568-2-tiwai@suse.de> <20190427175938.GJ14916@sirena.org.uk> <20190503064729.GF14916@sirena.org.uk> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 92488F8075A for ; Fri, 3 May 2019 09:18:28 +0200 (CEST) In-Reply-To: <20190503064729.GF14916@sirena.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" To: Mark Brown Cc: alsa-devel@alsa-project.org, Pierre-Louis Bossart List-Id: alsa-devel@alsa-project.org On Fri, 03 May 2019 08:47:29 +0200, Mark Brown wrote: > > On Thu, May 02, 2019 at 09:04:06AM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > On Tue, Apr 23, 2019 at 04:13:35PM +0200, Takashi Iwai wrote: > > > > This looks *very* much like board configuration rather than a patch - > > > there's no kind of test bit and the comments talk specifically about > > > things like gain settings and pad configuration which look very board > > > specific. Register patches are supposed to be for things like early > > > revisions of the chip which have different register defaults or magic > > > sequences that vendors tell you to run on startup, usually to tune test > > > registers. > > > OK, will replace with the straight regmap_multi_reg_write(). > > That's probably not addressing the issue, a lot of that stuff just > doesn't seem like it should be in some fixed configuration table at all. So what's your alternative suggestion? > > > > +#define cx2072x_plbk_eq_en_info snd_ctl_boolean_mono_info > > > > Why not just use the function directly rather than hiding it? > > > Just a standard idiom. Can be replaced if preferred. > > Please. > > > > > +int snd_soc_cx2072x_enable_jack_detect(struct snd_soc_component *codec) > > > > +{ > > > > + struct cx2072x_priv *cx2072x = snd_soc_component_get_drvdata(codec); > > > > + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(codec); > > > > + > > > > + /* No-sticky input type */ > > > > + regmap_write(cx2072x->regmap, CX2072X_GPIO_STICKY_MASK, 0x1f); > > > > + > > > > + /* Use GPOI0 as interrupt pin */ > > > > + regmap_write(cx2072x->regmap, CX2072X_UM_INTERRUPT_CRTL_E, 0x12 << 24); > > > > This isn't board specific is it? > > > I have no idea. It's been so from the original code, and there > > doesn't seem any other hardware implementations. > > Oh, joy. What's the story here? Do you have a datasheet for the part? Not at all. I just refreshed the already submitted patches since I have a laptop with the codec. I tried to contact Conexant, but in vain, so I decided to submit the renewed one. > > The jack detection in ASoC is anyway a bit funky, especially when > > involved with PM... > > What do you mean here? I'm not aware of any issues and the systems I've > worked with seemed robust... There are tons of different ways of implementation for jack controls, with different API usages. IOW, no consistency. > > > > + dev_dbg(codec->dev, "CX2072X_HSDETECT type=0x%X,Jack state = %x\n", > > > > + type, state); > > > > + return state; > > > > +} > > > > +EXPORT_SYMBOL_GPL(snd_soc_cx2072x_get_jack_state); > > > > Why is this symbol exported? > > > It's called from the machine driver. > > snd_soc_jack_add_gpios() is called in the machine driver side, and it > > needs the jack_status_check callback that calls this function. > > That code shouldn't be in the machine driver, the CODEC driver should > request any interrupts it needs itself. The similar things are done on many other Intel SST board drivers. The current patch just follows the pattern. > > > > + /* use flat eq by default */ > > > > + for (ch = 0 ; ch < 2 ; ch++) { > > > > + for (band = 0; band < CX2072X_PLBK_EQ_BAND_NUM; band++) { > > > > + cx2072x->plbk_eq[ch][band][1] = 64; > > > > + cx2072x->plbk_eq[ch][band][10] = 3; > > > > + } > > > > + } > > > > Why not use the register defaults? > > > Because it'll become too messy for put flatten array values? > > The initialization using loop makes more sense in such a case, IMO. > > No, that's not the question. The question is why there is any > initialization at all? Again, no idea. These are likely no default values of the hardware registers, and we need to set up some. I *guess* ditto for the initial register table in the above, too. thanks, Takashi