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=-10.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,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 C420FC43461 for ; Fri, 4 Sep 2020 05:02:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 804812074D for ; Fri, 4 Sep 2020 05:02:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1599195771; bh=JN0FZI582IJcgLwwiMOYbixgJHqCRoL83mENL/77ZfM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=tCbBZswNiSEuDy8uu7TQWdURFWcTaeRwZiclFmtjWHmADqWshWEZcUHuLRrdAnD6A iySXvYGNMATCD31P2VaE3W/l1g5Du5MDD7SGrvSHU8ERsXDTrOl0zLe5oszQ6OP6UV 0zRv5ueVgyUAIrPMH3J97IPs9enEW9jeWT+Gs3/w= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726245AbgIDFCu (ORCPT ); Fri, 4 Sep 2020 01:02:50 -0400 Received: from mail.kernel.org ([198.145.29.99]:50820 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725812AbgIDFCu (ORCPT ); Fri, 4 Sep 2020 01:02:50 -0400 Received: from localhost (unknown [122.182.253.87]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 5A64E206F2; Fri, 4 Sep 2020 05:02:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1599195769; bh=JN0FZI582IJcgLwwiMOYbixgJHqCRoL83mENL/77ZfM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=be/lZzo/VK2yulLqLcXmZhrVXOkDY4yNL96PCBPrXw6QDqyWmJ7jFG7c65t594kr5 tS0hdq9CrOrIhm+GxMb/C+R4VUqRGWdwFaz7lYJMmcLGzjEJVPLe107NxME1sFl7ok WwQoiwAaB7IwZ+5qUjIusbzw+QFZmKNZcZuw/iYc= Date: Fri, 4 Sep 2020 10:32:44 +0530 From: Vinod Koul To: Pierre-Louis Bossart Cc: alsa-devel@alsa-project.org, tiwai@suse.de, broonie@kernel.org, gregkh@linuxfoundation.org, Bard liao , Rander Wang , Guennadi Liakhovetski , Kai Vehmanen , Sanyog Kale , open list Subject: Re: [PATCH v2 2/3] soundwire: SDCA: add helper macro to access controls Message-ID: <20200904050244.GT2639@vkoul-mobl> References: <20200901162225.33343-1-pierre-louis.bossart@linux.intel.com> <20200901162225.33343-3-pierre-louis.bossart@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200901162225.33343-3-pierre-louis.bossart@linux.intel.com> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01-09-20, 11:22, Pierre-Louis Bossart wrote: > The upcoming SDCA (SoundWire Device Class Audio) specification defines > a hierarchical encoding to interface with Class-defined capabilities. > > The specification is not yet accessible to the general public but this > information is released with explicit permission from the MIPI Board > to avoid delays with SDCA support on Linux platforms. > > A block of 64 MBytes of register addresses is allocated to SDCA > controls, starting at address 0x40000000. The 26 LSBs which identify > individual controls are set based on the following variables: > > - Function Number. An SCDA device can be split in up to 8 independent > Functions. Each of these Functions is described in the SDCA > specification, e.g. Smart Amplifier, Smart Microphone, Simple > Microphone, Jack codec, HID, etc. > > - Entity Number. Within each Function, an Entity is an identifiable > block. Up to 127 Entities are connected in a pre-defined > graph (similar to USB), with Entity0 reserved for Function-level > configurations. In contrast to USB, the SDCA spec pre-defines > Function Types, topologies, and allowed options, i.e. the degree of > freedom is not unlimited to limit the possibility of errors in > descriptors leading to software quirks. > > - Control Selector. Within each Entity, the SDCA specification defines > up-to 48 controls such as Mute, Gain, AGC, etc, and 16 > implementation defined ones. Some Control Selectors might be used > for low-level platform setup, and other exposed to applications and > users. Note that the same Control Selector capability, e.g. Latency > control, might be located at different offsets in different > entities - the Control Selector mapping is Entity-specific. > > - Control Number. Some Control Selectors allow channel-specific values > to be set, with up to 64 channels allowed. This is mostly used for > volume control. > > - Current/Next values. Some Control Selectors are > 'Dual-Ranked'. Software may either update the Current value directly > for immediate effect. Alternatively, software may write into the > 'Next' values and update the SoundWire 1.2 'Commit Groups' register > to copy 'Next' values into 'Current' ones in a synchronized > manner. This is different from bank switching which is typically > used to change the bus configuration only. > > - MBQ. the Multi-Byte Quantity bit is used to provide atomic updates > when accessing more that one byte, for example a 16-bit volume > control would be updated consistently, the intermediate values > mixing old MSB with new LSB are not applied. > > These 6 parameters are used to build a 32-bit address to access the > desired Controls. Because of address range, paging is required, but > the most often used parameter values are placed in the lower 16 bits > of the address. This helps to keep the paging registers constant while > updating Controls for a specific Device/Function. This is good, thanks for adding it in changelog. Can you also add this description to Documentation (that can come as an individual patch), > > Reviewed-by: Rander Wang > Reviewed-by: Guennadi Liakhovetski > Reviewed-by: Kai Vehmanen > Acked-by: Bard Liao > Signed-off-by: Pierre-Louis Bossart > --- > include/linux/soundwire/sdw_registers.h | 33 +++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/include/linux/soundwire/sdw_registers.h b/include/linux/soundwire/sdw_registers.h > index 5d3c271af7d1..99ff7afc27a2 100644 > --- a/include/linux/soundwire/sdw_registers.h > +++ b/include/linux/soundwire/sdw_registers.h > @@ -305,4 +305,37 @@ > #define SDW_CASC_PORT_MASK_INTSTAT3 1 > #define SDW_CASC_PORT_REG_OFFSET_INTSTAT3 2 > > +/* > + * v1.2 device - SDCA address mapping > + * > + * Spec definition > + * Bits Contents > + * 31 0 (required by addressing range) > + * 30:26 0b10000 (Control Prefix) So this is for 30:26 > + * 25 0 (Reserved) > + * 24:22 Function Number [2:0] > + * 21 Entity[6] > + * 20:19 Control Selector[5:4] > + * 18 0 (Reserved) > + * 17:15 Control Number[5:3] > + * 14 Next > + * 13 MBQ > + * 12:7 Entity[5:0] > + * 6:3 Control Selector[3:0] > + * 2:0 Control Number[2:0] > + */ > + > +#define SDW_SDCA_CTL(fun, ent, ctl, ch) \ > + (BIT(30) | \ Programmatically this is fine, but then since we are defining for the description above, IMO it would actually make sense for this to be defined as FIELD_PREP: FIELD_PREP(GENMASK(30, 26), 1) or better u32_encode_bits(GENMASK(30, 26), 1) > + FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun))) | \ Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and below? > + FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent))) | \ > + FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) | \ > + FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch))) | \ > + FIELD_PREP(GENMASK(12, 7), FIELD_GET(GENMASK(5, 0), (ent))) | \ > + FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) | \ > + FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch)))) Also, can we rather have a nice function for this, that would look much cleaner And while at it, consider defining masks for various fields rather than using numbers in GENMASK() above, that would look better, be more readable and people can reuse it. -- ~Vinod