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=-11.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable 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 B9E7CC8301F for ; Tue, 13 Oct 2020 02:42:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7806C20678 for ; Tue, 13 Oct 2020 02:42:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728059AbgJMBPf (ORCPT ); Mon, 12 Oct 2020 21:15:35 -0400 Received: from new2-smtp.messagingengine.com ([66.111.4.224]:51969 "EHLO new2-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727950AbgJMBPf (ORCPT ); Mon, 12 Oct 2020 21:15:35 -0400 Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailnew.nyi.internal (Postfix) with ESMTP id 1E8F058034E; Mon, 12 Oct 2020 21:15:34 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute5.internal (MEProxy); Mon, 12 Oct 2020 21:15:34 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sholland.org; h= subject:to:cc:references:from:message-id:date:mime-version :in-reply-to:content-type:content-transfer-encoding; s=fm3; bh=/ EI01dJNkjAjzVrpLDHdoBNLG11c1Wr2zSgmdsOeUQk=; b=jCD0vosBiM2Fi73lK qq7aEC7Hb+KiedGsgfnXGak+S2OdSPM+00OpPo4aKVqHuKGsYz0d/4VAwNuq99ut Us3X0KrIVparkDtgMhxFwQG4sjdLlENjS+gZVSQDMDduzFx6u3lfZkjnuBR/yC5+ jkmKZnNQWhPktwEBHoyn9IwmIMCvRMwHUqfo+GZBQ3091EwroTB/Bd9k9hXfj0Kf kOMMXSNu0uRl2oRKccfZK2gYZMSs8n70ZxNa+4yM3X/uwhEZcImbDBCLGz8U8k4o +BklZbIkN6gEa0u9V0EXLiS0uOCNVe+DCfj3JTKHnHCsUwqOlbhxnj9w4InMzi3/ N1paQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=/EI01dJNkjAjzVrpLDHdoBNLG11c1Wr2zSgmdsOeU Qk=; b=olq7BEih38ZaP7P0P6a0o82OUMIVpyveU4fH9PnYNYW2NC6P1Qm9LmPpf 4w3aOxuLOTDTYqlaJG6FnsXnACSHL/s0n78f5owEm/nGMgdPbLJ4lGNZD2B05L7c uanGpd3Dsw52BPMqdPcTKwenO0n8OqrFQooCXsTE4+TUJMb1XXXtS4cAUF9A22g4 3zAJCvfKB0Kkt/FZS5ZWMQhI2J5DPdG0ljF39p05odHga0K4w6F4AEmMtfKyrkDs ZfKxGMYlDOqyLSMFMs+TpvEvfFxwZlyWRwg05mQDzGHspmvf1mb/108FfJB4zriv o4Jmv2lOhDKFaG4JONdYVjEVuMMIA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrheekgdeghecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefuvfhfhffkffgfgggjtgfgsehtkeertddtfeehnecuhfhrohhmpefurghmuhgv lhcujfholhhlrghnugcuoehsrghmuhgvlhesshhhohhllhgrnhgurdhorhhgqeenucggtf frrghtthgvrhhnpefhueeuueetteeiuddvveetieekiedvkeejjeetkeejfeejuddvvedu geejteelueenucfkphepjedtrddufeehrddugeekrdduhedunecuvehluhhsthgvrhfuih iivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepshgrmhhuvghlsehshhholhhlrghn ugdrohhrgh X-ME-Proxy: Received: from [70.135.148.151] (70-135-148-151.lightspeed.stlsmo.sbcglobal.net [70.135.148.151]) by mail.messagingengine.com (Postfix) with ESMTPA id E0A99328005E; Mon, 12 Oct 2020 21:15:30 -0400 (EDT) Subject: Re: [PATCH v6 02/14] ASoC: sun4i-i2s: Change set_chan_cfg() params To: Maxime Ripard , =?UTF-8?B?Q2zDqW1lbnQgUMOpcm9u?= Cc: Chen-Yu Tsai , Rob Herring , Mark Brown , Liam Girdwood , Jaroslav Kysela , Takashi Iwai , Marcus Cooper , Jernej Skrabec , Linux-ALSA , devicetree , linux-arm-kernel , linux-kernel , linux-sunxi References: <20201003141950.455829-1-peron.clem@gmail.com> <20201003141950.455829-3-peron.clem@gmail.com> <20201005121307.v6jpyeyfi4kxc2cl@gilmour.lan> <20201012121536.z5d7kecdxaabw35n@gilmour.lan> From: Samuel Holland Message-ID: <0d6f0693-5ca9-9b48-4d33-a969bd5b1b1b@sholland.org> Date: Mon, 12 Oct 2020 20:15:30 -0500 User-Agent: Mozilla/5.0 (X11; Linux ppc64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1 MIME-Version: 1.0 In-Reply-To: <20201012121536.z5d7kecdxaabw35n@gilmour.lan> Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On 10/12/20 7:15 AM, Maxime Ripard wrote: > Hi, > > On Mon, Oct 05, 2020 at 03:23:12PM +0200, Clément Péron wrote: >> On Mon, 5 Oct 2020 at 14:13, Maxime Ripard wrote: >>> >>> On Sat, Oct 03, 2020 at 04:19:38PM +0200, Clément Péron wrote: >>>> As slots and slot_width can be set manually using set_tdm(). >>>> These values are then kept in sun4i_i2s struct. >>>> So we need to check if these values are setted or not >>>> in the struct. >>>> >>>> Avoid to check for this logic in set_chan_cfg(). This will >>>> duplicate the same check instead pass the required values >>>> as params to set_chan_cfg(). >>>> >>>> This will also avoid a bug when we will enable 20/24bits support, >>>> i2s->slot_width is not actually used in the lrck_period computation. >>>> >>>> Suggested-by: Samuel Holland >>>> Signed-off-by: Clément Péron >>>> --- >>>> sound/soc/sunxi/sun4i-i2s.c | 36 ++++++++++++++---------------------- >>>> 1 file changed, 14 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c >>>> index c5ccd423e6d3..1f577dbc20a6 100644 >>>> --- a/sound/soc/sunxi/sun4i-i2s.c >>>> +++ b/sound/soc/sunxi/sun4i-i2s.c >>>> @@ -177,8 +177,9 @@ struct sun4i_i2s_quirks { >>>> unsigned long (*get_bclk_parent_rate)(const struct sun4i_i2s *); >>>> s8 (*get_sr)(const struct sun4i_i2s *, int); >>>> s8 (*get_wss)(const struct sun4i_i2s *, int); >>>> - int (*set_chan_cfg)(const struct sun4i_i2s *, >>>> - const struct snd_pcm_hw_params *); >>>> + int (*set_chan_cfg)(const struct sun4i_i2s *i2s, >>>> + unsigned int channels, unsigned int slots, >>>> + unsigned int slot_width); >>>> int (*set_fmt)(const struct sun4i_i2s *, unsigned int); >>>> }; >>>> >>>> @@ -414,10 +415,9 @@ static s8 sun8i_i2s_get_sr_wss(const struct sun4i_i2s *i2s, int width) >>>> } >>>> >>>> static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> - const struct snd_pcm_hw_params *params) >>>> + unsigned int channels, unsigned int slots, >>>> + unsigned int slot_width) >>>> { >>>> - unsigned int channels = params_channels(params); >>>> - >>>> /* Map the channels for playback and capture */ >>>> regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG, 0x76543210); >>>> regmap_write(i2s->regmap, SUN4I_I2S_RX_CHAN_MAP_REG, 0x00003210); >>>> @@ -434,15 +434,11 @@ static int sun4i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> } >>>> >>>> static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> - const struct snd_pcm_hw_params *params) >>>> + unsigned int channels, unsigned int slots, >>>> + unsigned int slot_width) >>>> { >>>> - unsigned int channels = params_channels(params); >>>> - unsigned int slots = channels; >>>> unsigned int lrck_period; >>>> >>>> - if (i2s->slots) >>>> - slots = i2s->slots; >>>> - >>>> /* Map the channels for playback and capture */ >>>> regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210); >>>> regmap_write(i2s->regmap, SUN8I_I2S_RX_CHAN_MAP_REG, 0x76543210); >>>> @@ -467,11 +463,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> case SND_SOC_DAIFMT_DSP_B: >>>> case SND_SOC_DAIFMT_LEFT_J: >>>> case SND_SOC_DAIFMT_RIGHT_J: >>>> - lrck_period = params_physical_width(params) * slots; >>>> + lrck_period = slot_width * slots; >>>> break; >>>> >>>> case SND_SOC_DAIFMT_I2S: >>>> - lrck_period = params_physical_width(params); >>>> + lrck_period = slot_width; >>>> break; >>>> >>>> default: >>>> @@ -490,15 +486,11 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> } >>>> >>>> static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> - const struct snd_pcm_hw_params *params) >>>> + unsigned int channels, unsigned int slots, >>>> + unsigned int slot_width) >>>> { >>>> - unsigned int channels = params_channels(params); >>>> - unsigned int slots = channels; >>>> unsigned int lrck_period; >>>> >>>> - if (i2s->slots) >>>> - slots = i2s->slots; >>>> - >>>> /* Map the channels for playback and capture */ >>>> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0xFEDCBA98); >>>> regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210); >>>> @@ -525,11 +517,11 @@ static int sun50i_h6_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, >>>> case SND_SOC_DAIFMT_DSP_B: >>>> case SND_SOC_DAIFMT_LEFT_J: >>>> case SND_SOC_DAIFMT_RIGHT_J: >>>> - lrck_period = params_physical_width(params) * slots; >>>> + lrck_period = slot_width * slots; >>>> break; >>>> >>>> case SND_SOC_DAIFMT_I2S: >>>> - lrck_period = params_physical_width(params); >>>> + lrck_period = slot_width; >>>> break; >>>> >>>> default: >>>> @@ -565,7 +557,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, >>>> if (i2s->slot_width) >>>> slot_width = i2s->slot_width; >>>> >>>> - ret = i2s->variant->set_chan_cfg(i2s, params); >>>> + ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); >>> >>> Isn't slots and slot_width set to 0 here ? >> >> No, there is still a check before calling the set_cfg function. >> >> unsigned int slot_width = params_physical_width(params); >> unsigned int channels = params_channels(params); >> unsigned int slots = channels; >> >> if (i2s->slots) >> slots = i2s->slots; >> >> if (i2s->slot_width) >> slot_width = i2s->slot_width; >> >> ret = i2s->variant->set_chan_cfg(i2s, channels, slots, slot_width); >> >> So slot_width will be equal to params_physical_width(params); >> like before > > Still, it's not obvious what slots and slot_width are going to be set to > in a non-TDM setup where that doesn't really make much sense. My understanding is: "slots" is channels per frame + padding slots, regardless of format. "slot_width" is bits per sample + padding bits, regardless of format. Some formats may require or prohibit certain padding, but that has no effect on the definitions. That seems clear to me? At least that's what I implemented (and you acked) in sun8i-codec. > I assume you want to reduce the boilerplate, then we can make an helper > to get the frame size and the number of channels / slots if needed What would you name the return values, if not "slots" and "slot_width"? "channels" and "word_size" would be inaccurate, because those terms refer to the values without padding. > Maxime Cheers, Samuel