* [PATCH 0/4] regmap: add SoundWire 1.2 MBQ support @ 2020-08-25 17:16 Pierre-Louis Bossart 2020-08-25 17:16 ` [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP Pierre-Louis Bossart ` (3 more replies) 0 siblings, 4 replies; 26+ messages in thread From: Pierre-Louis Bossart @ 2020-08-25 17:16 UTC (permalink / raw) To: alsa-devel Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Pierre-Louis Bossart In preparation of the upstream contribution of SDCA (SoundWire Device Class for Audio) ASoC codec drivers [1] [2], add regmap support SoundWire 1.2 MBQ support. The MBQ (Multi-Byte Quantity) registers need to be handled in a different way from regular 8-bit SoundWire registers, their main application is going to be for volume/gain controls. The 3rd patch was initially suggested for inclusion in the SoundWire tree, and was modified to add more background information in the commit message as requested by Vinod Koul. Pierre-Louis Bossart (4): regmap: sdw: move to -EOPNOTSUPP regmap: sdw: add required header files soundwire: SDCA: add helper macro to access controls regmap: sdw: add support for SoundWire 1.2 MBQ drivers/base/regmap/Kconfig | 6 +- drivers/base/regmap/Makefile | 1 + drivers/base/regmap/regmap-sdw-mbq.c | 102 ++++++++++++++++++++++++ drivers/base/regmap/regmap-sdw.c | 8 +- include/linux/regmap.h | 20 +++++ include/linux/soundwire/sdw_registers.h | 13 +++ 6 files changed, 146 insertions(+), 4 deletions(-) create mode 100644 drivers/base/regmap/regmap-sdw-mbq.c base-commit: 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5 -- 2.25.1 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP 2020-08-25 17:16 [PATCH 0/4] regmap: add SoundWire 1.2 MBQ support Pierre-Louis Bossart @ 2020-08-25 17:16 ` Pierre-Louis Bossart 2020-08-25 21:48 ` Mark Brown 2020-08-25 17:16 ` [PATCH 2/4] regmap: sdw: add required header files Pierre-Louis Bossart ` (2 subsequent siblings) 3 siblings, 1 reply; 26+ messages in thread From: Pierre-Louis Bossart @ 2020-08-25 17:16 UTC (permalink / raw) To: alsa-devel Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Pierre-Louis Bossart, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki -ENOTSUPP is not a valid error code, use recommended value instead. Reviewed-by: Rander Wang <rander.wang@linux.intel.com> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/base/regmap/regmap-sdw.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/base/regmap/regmap-sdw.c b/drivers/base/regmap/regmap-sdw.c index 50a66382d87d..89d3856f5890 100644 --- a/drivers/base/regmap/regmap-sdw.c +++ b/drivers/base/regmap/regmap-sdw.c @@ -40,14 +40,14 @@ static int regmap_sdw_config_check(const struct regmap_config *config) { /* All register are 8-bits wide as per MIPI Soundwire 1.0 Spec */ if (config->val_bits != 8) - return -ENOTSUPP; + return -EOPNOTSUPP; /* Registers are 32 bits wide */ if (config->reg_bits != 32) - return -ENOTSUPP; + return -EOPNOTSUPP; if (config->pad_bits != 0) - return -ENOTSUPP; + return -EOPNOTSUPP; return 0; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP 2020-08-25 17:16 ` [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP Pierre-Louis Bossart @ 2020-08-25 21:48 ` Mark Brown 2020-08-25 22:08 ` Pierre-Louis Bossart 0 siblings, 1 reply; 26+ messages in thread From: Mark Brown @ 2020-08-25 21:48 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: alsa-devel, linux-kernel, tiwai, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki [-- Attachment #1: Type: text/plain, Size: 228 bytes --] On Tue, Aug 25, 2020 at 12:16:53PM -0500, Pierre-Louis Bossart wrote: > -ENOTSUPP is not a valid error code, use recommended value instead. What makes you say this - it's what regmap uses internally for unsupported operations? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP 2020-08-25 21:48 ` Mark Brown @ 2020-08-25 22:08 ` Pierre-Louis Bossart 2020-08-26 9:56 ` Mark Brown 0 siblings, 1 reply; 26+ messages in thread From: Pierre-Louis Bossart @ 2020-08-25 22:08 UTC (permalink / raw) To: Mark Brown Cc: alsa-devel, linux-kernel, tiwai, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki >> -ENOTSUPP is not a valid error code, use recommended value instead. > > What makes you say this - it's what regmap uses internally for > unsupported operations? This was flagged by scripts/checkpatch.pl (must be a new addition). # ENOTSUPP is not a standard error code and should be avoided in new patches. # Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP. # Similarly to ENOSYS warning a small number of false positives is expected. if (!$file && $line =~ /\bENOTSUPP\b/) { if (WARN("ENOTSUPP", "ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP\n" . $herecurr) && $fix) { $fixed[$fixlinenr] =~ s/\bENOTSUPP\b/EOPNOTSUPP/; } } I was just blindly making checkpatch happy and tried to keep regmap-sdw.c and regmap-sdw-mbq aligned. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP 2020-08-25 22:08 ` Pierre-Louis Bossart @ 2020-08-26 9:56 ` Mark Brown 2020-08-26 10:09 ` Takashi Iwai 0 siblings, 1 reply; 26+ messages in thread From: Mark Brown @ 2020-08-26 9:56 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: alsa-devel, linux-kernel, tiwai, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki [-- Attachment #1: Type: text/plain, Size: 335 bytes --] On Tue, Aug 25, 2020 at 05:08:55PM -0500, Pierre-Louis Bossart wrote: > > > -ENOTSUPP is not a valid error code, use recommended value instead. > > What makes you say this - it's what regmap uses internally for > > unsupported operations? > This was flagged by scripts/checkpatch.pl (must be a new addition). checkpatch is broken. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP 2020-08-26 9:56 ` Mark Brown @ 2020-08-26 10:09 ` Takashi Iwai 2020-08-26 10:13 ` Mark Brown 0 siblings, 1 reply; 26+ messages in thread From: Takashi Iwai @ 2020-08-26 10:09 UTC (permalink / raw) To: Mark Brown Cc: Pierre-Louis Bossart, alsa-devel, linux-kernel, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki On Wed, 26 Aug 2020 11:56:01 +0200, Mark Brown wrote: > > On Tue, Aug 25, 2020 at 05:08:55PM -0500, Pierre-Louis Bossart wrote: > > > > > -ENOTSUPP is not a valid error code, use recommended value instead. > > > > What makes you say this - it's what regmap uses internally for > > > unsupported operations? > > > This was flagged by scripts/checkpatch.pl (must be a new addition). > > checkpatch is broken. Heh, I'm not objecting it :) OTOH, it's also true that ENOTSUPP is no good error code if returned to user-space. Unfortunately many codes (including what I wrote) use this code mistakenly, and they can't be changed any longer... Takashi ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP 2020-08-26 10:09 ` Takashi Iwai @ 2020-08-26 10:13 ` Mark Brown 2020-08-26 10:22 ` Takashi Iwai 0 siblings, 1 reply; 26+ messages in thread From: Mark Brown @ 2020-08-26 10:13 UTC (permalink / raw) To: Takashi Iwai Cc: Pierre-Louis Bossart, alsa-devel, linux-kernel, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki [-- Attachment #1: Type: text/plain, Size: 517 bytes --] On Wed, Aug 26, 2020 at 12:09:28PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > checkpatch is broken. > Heh, I'm not objecting it :) > OTOH, it's also true that ENOTSUPP is no good error code if returned > to user-space. Unfortunately many codes (including what I wrote) use > this code mistakenly, and they can't be changed any longer... It's also used internally in various places without being returned to userspace, that's what's going on here - the regmap core has some specific checks for -ENOTSUPP. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP 2020-08-26 10:13 ` Mark Brown @ 2020-08-26 10:22 ` Takashi Iwai 2020-08-26 12:03 ` Vinod Koul 2020-08-26 15:05 ` Pierre-Louis Bossart 0 siblings, 2 replies; 26+ messages in thread From: Takashi Iwai @ 2020-08-26 10:22 UTC (permalink / raw) To: Mark Brown Cc: Pierre-Louis Bossart, alsa-devel, linux-kernel, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki On Wed, 26 Aug 2020 12:13:01 +0200, Mark Brown wrote: > > On Wed, Aug 26, 2020 at 12:09:28PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > checkpatch is broken. > > > Heh, I'm not objecting it :) > > > OTOH, it's also true that ENOTSUPP is no good error code if returned > > to user-space. Unfortunately many codes (including what I wrote) use > > this code mistakenly, and they can't be changed any longer... > > It's also used internally in various places without being returned to > userspace, that's what's going on here - the regmap core has some > specific checks for -ENOTSUPP. Sure, for such an internal usage any code can be used. The question is a case like this -- where the return code might be carried to outside. Though, looking through the grep output, all callers simply return -EINVAL for any errors, so it doesn't matter much for now. BTW, there are a few callers of devm_regmap_init_sdw() checking the return value with NULL. This will crash as the function returns the error pointer, and they must be checked with IS_ERR() instead. Takashi ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP 2020-08-26 10:22 ` Takashi Iwai @ 2020-08-26 12:03 ` Vinod Koul 2020-08-26 15:05 ` Pierre-Louis Bossart 1 sibling, 0 replies; 26+ messages in thread From: Vinod Koul @ 2020-08-26 12:03 UTC (permalink / raw) To: Takashi Iwai Cc: Mark Brown, Pierre-Louis Bossart, alsa-devel, linux-kernel, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki On 26-08-20, 12:22, Takashi Iwai wrote: > On Wed, 26 Aug 2020 12:13:01 +0200, > Mark Brown wrote: > > > > On Wed, Aug 26, 2020 at 12:09:28PM +0200, Takashi Iwai wrote: > > > Mark Brown wrote: > > > > > > checkpatch is broken. > > > > > Heh, I'm not objecting it :) > > > > > OTOH, it's also true that ENOTSUPP is no good error code if returned > > > to user-space. Unfortunately many codes (including what I wrote) use > > > this code mistakenly, and they can't be changed any longer... > > > > It's also used internally in various places without being returned to > > userspace, that's what's going on here - the regmap core has some > > specific checks for -ENOTSUPP. > > Sure, for such an internal usage any code can be used. > The question is a case like this -- where the return code might be > carried to outside. Though, looking through the grep output, all > callers simply return -EINVAL for any errors, so it doesn't matter > much for now. > > > BTW, there are a few callers of devm_regmap_init_sdw() checking the > return value with NULL. This will crash as the function returns the > error pointer, and they must be checked with IS_ERR() instead. Yes that is correct, all expect wsa codec do the incorrect thing. Will send patches for these shortly Thanks -- ~Vinod ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP 2020-08-26 10:22 ` Takashi Iwai 2020-08-26 12:03 ` Vinod Koul @ 2020-08-26 15:05 ` Pierre-Louis Bossart 2020-08-26 17:25 ` Mark Brown 1 sibling, 1 reply; 26+ messages in thread From: Pierre-Louis Bossart @ 2020-08-26 15:05 UTC (permalink / raw) To: Takashi Iwai, Mark Brown Cc: Guennadi Liakhovetski, alsa-devel, Kai Vehmanen, Rafael J. Wysocki, gregkh, linux-kernel, Hui Wang, vkoul, srinivas.kandagatla, Ranjani Sridharan, jank, slawomir.blauciak, Bard liao, Rander Wang >>>> checkpatch is broken. >> >>> Heh, I'm not objecting it :) >> >>> OTOH, it's also true that ENOTSUPP is no good error code if returned >>> to user-space. Unfortunately many codes (including what I wrote) use >>> this code mistakenly, and they can't be changed any longer... >> >> It's also used internally in various places without being returned to >> userspace, that's what's going on here - the regmap core has some >> specific checks for -ENOTSUPP. > > Sure, for such an internal usage any code can be used. > The question is a case like this -- where the return code might be > carried to outside. Though, looking through the grep output, all > callers simply return -EINVAL for any errors, so it doesn't matter > much for now. I assumed this change to -EOPNOTSUPP reflected a consensus in kernel-land, it's obviously not the case. This patch was supposed to be a trivial clean-up... So to be clear, what is the direction for existing code a) keep -ENOTSUPP as is? b) move to -EOPNOTSUPP? And what is the preference for new code? ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP 2020-08-26 15:05 ` Pierre-Louis Bossart @ 2020-08-26 17:25 ` Mark Brown 2020-08-26 18:08 ` Pierre-Louis Bossart 0 siblings, 1 reply; 26+ messages in thread From: Mark Brown @ 2020-08-26 17:25 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Takashi Iwai, Guennadi Liakhovetski, alsa-devel, Kai Vehmanen, Rafael J. Wysocki, gregkh, linux-kernel, Hui Wang, vkoul, srinivas.kandagatla, Ranjani Sridharan, jank, slawomir.blauciak, Bard liao, Rander Wang [-- Attachment #1: Type: text/plain, Size: 669 bytes --] On Wed, Aug 26, 2020 at 10:05:50AM -0500, Pierre-Louis Bossart wrote: > I assumed this change to -EOPNOTSUPP reflected a consensus in kernel-land, > it's obviously not the case. This patch was supposed to be a trivial > clean-up... No, it's just some random guy sent a patch. They've not made any perceptible effort to interact with any of the existing users. > So to be clear, what is the direction for existing code > a) keep -ENOTSUPP as is? > b) move to -EOPNOTSUPP? > And what is the preference for new code? If you want to change this you'd need to change it over the whole subsystem (if not other subsystems), including the places where the value is used. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP 2020-08-26 17:25 ` Mark Brown @ 2020-08-26 18:08 ` Pierre-Louis Bossart 0 siblings, 0 replies; 26+ messages in thread From: Pierre-Louis Bossart @ 2020-08-26 18:08 UTC (permalink / raw) To: Mark Brown Cc: Guennadi Liakhovetski, alsa-devel, Kai Vehmanen, Rafael J. Wysocki, Takashi Iwai, gregkh, linux-kernel, Ranjani Sridharan, Hui Wang, vkoul, srinivas.kandagatla, jank, slawomir.blauciak, Bard liao, Rander Wang > If you want to change this you'd need to change it over the whole > subsystem (if not other subsystems), including the places where the > value is used. ok, I'll drop this patch for now, keep -ENOTSUPP and deal with this later. The only thing I really care about for now is SoundWire MBQ support, this is the only thing gating SDCA contributions. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/4] regmap: sdw: add required header files 2020-08-25 17:16 [PATCH 0/4] regmap: add SoundWire 1.2 MBQ support Pierre-Louis Bossart 2020-08-25 17:16 ` [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP Pierre-Louis Bossart @ 2020-08-25 17:16 ` Pierre-Louis Bossart 2020-08-25 17:16 ` [PATCH 3/4] soundwire: SDCA: add helper macro to access controls Pierre-Louis Bossart 2020-08-25 17:16 ` [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ Pierre-Louis Bossart 3 siblings, 0 replies; 26+ messages in thread From: Pierre-Louis Bossart @ 2020-08-25 17:16 UTC (permalink / raw) To: alsa-devel Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Pierre-Louis Bossart, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki Explicitly add header files used by regmap SoundWire support. Suggested-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> Reviewed-by: Rander Wang <rander.wang@linux.intel.com> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/base/regmap/regmap-sdw.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/base/regmap/regmap-sdw.c b/drivers/base/regmap/regmap-sdw.c index 89d3856f5890..29edbb6da48f 100644 --- a/drivers/base/regmap/regmap-sdw.c +++ b/drivers/base/regmap/regmap-sdw.c @@ -2,8 +2,10 @@ // Copyright(c) 2015-17 Intel Corporation. #include <linux/device.h> +#include <linux/errno.h> #include <linux/mod_devicetable.h> #include <linux/module.h> +#include <linux/regmap.h> #include <linux/soundwire/sdw.h> #include "internal.h" -- 2.25.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/4] soundwire: SDCA: add helper macro to access controls 2020-08-25 17:16 [PATCH 0/4] regmap: add SoundWire 1.2 MBQ support Pierre-Louis Bossart 2020-08-25 17:16 ` [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP Pierre-Louis Bossart 2020-08-25 17:16 ` [PATCH 2/4] regmap: sdw: add required header files Pierre-Louis Bossart @ 2020-08-25 17:16 ` Pierre-Louis Bossart 2020-08-26 1:04 ` Bard liao 2020-08-26 8:55 ` Vinod Koul 2020-08-25 17:16 ` [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ Pierre-Louis Bossart 3 siblings, 2 replies; 26+ messages in thread From: Pierre-Louis Bossart @ 2020-08-25 17:16 UTC (permalink / raw) To: alsa-devel Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Pierre-Louis Bossart, Guennadi Liakhovetski, Kai Vehmanen, Sanyog Kale 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 are 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 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. Reviewed-by: Rander Wang <rander.wang@linux.intel.com> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- include/linux/soundwire/sdw_registers.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/include/linux/soundwire/sdw_registers.h b/include/linux/soundwire/sdw_registers.h index 5d3c271af7d1..906dadda7387 100644 --- a/include/linux/soundwire/sdw_registers.h +++ b/include/linux/soundwire/sdw_registers.h @@ -305,4 +305,17 @@ #define SDW_CASC_PORT_MASK_INTSTAT3 1 #define SDW_CASC_PORT_REG_OFFSET_INTSTAT3 2 +/* v1.2 device - SDCA address mapping */ +#define SDW_SDCA_CTL(fun, ent, ctl, ch) (BIT(30) | \ + (((fun) & 0x7) << 22) | \ + (((ent) & 0x40) << 15) | \ + (((ent) & 0x3f) << 7) | \ + (((ctl) & 0x30) << 15) | \ + (((ctl) & 0x0f) << 3) | \ + (((ch) & 0x38) << 12) | \ + ((ch) & 0x07)) + +#define SDW_SDCA_MBQ_CTL(reg) ((reg) | BIT(13)) +#define SDW_SDCA_NEXT_CTL(reg) ((reg) | BIT(14)) + #endif /* __SDW_REGISTERS_H */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 3/4] soundwire: SDCA: add helper macro to access controls 2020-08-25 17:16 ` [PATCH 3/4] soundwire: SDCA: add helper macro to access controls Pierre-Louis Bossart @ 2020-08-26 1:04 ` Bard liao 2020-08-26 8:55 ` Vinod Koul 1 sibling, 0 replies; 26+ messages in thread From: Bard liao @ 2020-08-26 1:04 UTC (permalink / raw) To: Pierre-Louis Bossart, alsa-devel Cc: Kai Vehmanen, Guennadi Liakhovetski, tiwai, gregkh, linux-kernel, Ranjani Sridharan, Hui Wang, vkoul, broonie, srinivas.kandagatla, jank, slawomir.blauciak, Sanyog Kale, Rander Wang On 8/26/2020 1:16 AM, 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 are 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 > 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. > > Reviewed-by: Rander Wang <rander.wang@linux.intel.com> > Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> > Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Acked-by: Bard Liao <yung-chuan.liao@linux.intel.com> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/4] soundwire: SDCA: add helper macro to access controls 2020-08-25 17:16 ` [PATCH 3/4] soundwire: SDCA: add helper macro to access controls Pierre-Louis Bossart 2020-08-26 1:04 ` Bard liao @ 2020-08-26 8:55 ` Vinod Koul 2020-08-26 15:00 ` Pierre-Louis Bossart 1 sibling, 1 reply; 26+ messages in thread From: Vinod Koul @ 2020-08-26 8:55 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: alsa-devel, linux-kernel, tiwai, broonie, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Sanyog Kale On 25-08-20, 12:16, 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 are 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 > 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. > > Reviewed-by: Rander Wang <rander.wang@linux.intel.com> > Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> > Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > include/linux/soundwire/sdw_registers.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/linux/soundwire/sdw_registers.h b/include/linux/soundwire/sdw_registers.h > index 5d3c271af7d1..906dadda7387 100644 > --- a/include/linux/soundwire/sdw_registers.h > +++ b/include/linux/soundwire/sdw_registers.h > @@ -305,4 +305,17 @@ > #define SDW_CASC_PORT_MASK_INTSTAT3 1 > #define SDW_CASC_PORT_REG_OFFSET_INTSTAT3 2 > > +/* v1.2 device - SDCA address mapping */ Can you please add description of bits used by each field here, something like we have done for DevId > +#define SDW_SDCA_CTL(fun, ent, ctl, ch) (BIT(30) | \ > + (((fun) & 0x7) << 22) | \ > + (((ent) & 0x40) << 15) | \ > + (((ent) & 0x3f) << 7) | \ > + (((ctl) & 0x30) << 15) | \ > + (((ctl) & 0x0f) << 3) | \ > + (((ch) & 0x38) << 12) | \ > + ((ch) & 0x07)) GENMASK() for the bitmaps here please. Also it would look very neat by using FIELD_PREP() here, you can skip the bit shifts and they would be done by FIELD_PREP() for you. > -- > 2.25.1 -- ~Vinod ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/4] soundwire: SDCA: add helper macro to access controls 2020-08-26 8:55 ` Vinod Koul @ 2020-08-26 15:00 ` Pierre-Louis Bossart 2020-08-26 16:40 ` Vinod Koul 0 siblings, 1 reply; 26+ messages in thread From: Pierre-Louis Bossart @ 2020-08-26 15:00 UTC (permalink / raw) To: Vinod Koul Cc: Guennadi Liakhovetski, alsa-devel, Kai Vehmanen, tiwai, gregkh, linux-kernel, Ranjani Sridharan, Hui Wang, broonie, srinivas.kandagatla, jank, slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang >> +/* v1.2 device - SDCA address mapping */ > > Can you please add description of bits used by each field here, > something like we have done for DevId were you referring to something like this? * Spec definition * Register Bit Contents * DevId_0 [7:4] 47:44 sdw_version * DevId_0 [3:0] 43:40 unique_id * DevId_1 39:32 mfg_id [15:8] * DevId_2 31:24 mfg_id [7:0] * DevId_3 23:16 part_id [15:8] * DevId_4 15:08 part_id [7:0] * DevId_5 07:00 class_id > >> +#define SDW_SDCA_CTL(fun, ent, ctl, ch) (BIT(30) | \ >> + (((fun) & 0x7) << 22) | \ >> + (((ent) & 0x40) << 15) | \ >> + (((ent) & 0x3f) << 7) | \ >> + (((ctl) & 0x30) << 15) | \ >> + (((ctl) & 0x0f) << 3) | \ >> + (((ch) & 0x38) << 12) | \ >> + ((ch) & 0x07)) > > GENMASK() for the bitmaps here please. Also it would look very neat by > using FIELD_PREP() here, you can skip the bit shifts and they would be > done by FIELD_PREP() for you. ok. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/4] soundwire: SDCA: add helper macro to access controls 2020-08-26 15:00 ` Pierre-Louis Bossart @ 2020-08-26 16:40 ` Vinod Koul 0 siblings, 0 replies; 26+ messages in thread From: Vinod Koul @ 2020-08-26 16:40 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Guennadi Liakhovetski, alsa-devel, Kai Vehmanen, tiwai, gregkh, linux-kernel, Ranjani Sridharan, Hui Wang, broonie, srinivas.kandagatla, jank, slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang On 26-08-20, 10:00, Pierre-Louis Bossart wrote: > > > > > +/* v1.2 device - SDCA address mapping */ > > > > Can you please add description of bits used by each field here, > > something like we have done for DevId > > were you referring to something like this? > > * Spec definition > * Register Bit Contents > * DevId_0 [7:4] 47:44 sdw_version > * DevId_0 [3:0] 43:40 unique_id > * DevId_1 39:32 mfg_id [15:8] > * DevId_2 31:24 mfg_id [7:0] > * DevId_3 23:16 part_id [15:8] > * DevId_4 15:08 part_id [7:0] > * DevId_5 07:00 class_id Correct > > > > > +#define SDW_SDCA_CTL(fun, ent, ctl, ch) (BIT(30) | \ > > > + (((fun) & 0x7) << 22) | \ > > > + (((ent) & 0x40) << 15) | \ > > > + (((ent) & 0x3f) << 7) | \ > > > + (((ctl) & 0x30) << 15) | \ > > > + (((ctl) & 0x0f) << 3) | \ > > > + (((ch) & 0x38) << 12) | \ > > > + ((ch) & 0x07)) > > > > GENMASK() for the bitmaps here please. Also it would look very neat by > > using FIELD_PREP() here, you can skip the bit shifts and they would be > > done by FIELD_PREP() for you. > > ok. FWIW I am testing changes to do the conversion for subsystem to use nice stuff in bitfield.h -- ~Vinod ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ 2020-08-25 17:16 [PATCH 0/4] regmap: add SoundWire 1.2 MBQ support Pierre-Louis Bossart ` (2 preceding siblings ...) 2020-08-25 17:16 ` [PATCH 3/4] soundwire: SDCA: add helper macro to access controls Pierre-Louis Bossart @ 2020-08-25 17:16 ` Pierre-Louis Bossart 2020-08-26 0:59 ` Bard liao ` (2 more replies) 3 siblings, 3 replies; 26+ messages in thread From: Pierre-Louis Bossart @ 2020-08-25 17:16 UTC (permalink / raw) To: alsa-devel Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Pierre-Louis Bossart, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki The SoundWire 1.1 specification only allowed for reads and writes of bytes. The SoundWire 1.2 specification adds a new capability to transfer "Multi-Byte Quantities" (MBQ) across the bus. The transfers still happens one-byte-at-a-time, but the update is atomic. For example when writing a 16-bit volume, the first byte transferred is only taken into account when the second byte is successfully transferred. The mechanism is symmetrical for read and writes: - On a read, the address of the last byte to be read is modified by setting the MBQ bit - On a write, the address of all but the last byte to be written are modified by setting the MBQ bit. The address for the last byte relies on the MBQ bit being cleared. The current definitions for MBQ-based controls in the SDCA draft standard are limited to 16 bits for volumes, so for now this is the only supported format. An update will be provided if and when support for 24-bit and 32-bit values is specified by the SDCA standard. One possible objection is that this code could have been handled with regmap-sdw.c. However this is a new spec addition not handled by every SoundWire 1.1 and non-SDCA device, so there's no reason to load code that will never be used. Also in practice it's extremely unlikely that CONFIG_REGMAP would not be selected with CONFIG_REGMAP_MBQ selected. However there's no functional dependency between the two modules so they can be selected separately. Reviewed-by: Rander Wang <rander.wang@linux.intel.com> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/base/regmap/Kconfig | 6 +- drivers/base/regmap/Makefile | 1 + drivers/base/regmap/regmap-sdw-mbq.c | 102 +++++++++++++++++++++++++++ include/linux/regmap.h | 20 ++++++ 4 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 drivers/base/regmap/regmap-sdw-mbq.c diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig index 1d1d26b0d279..4d14355e9930 100644 --- a/drivers/base/regmap/Kconfig +++ b/drivers/base/regmap/Kconfig @@ -4,7 +4,7 @@ # subsystems should select the appropriate symbols. config REGMAP - default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_SOUNDWIRE || REGMAP_SCCB || REGMAP_I3C) + default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_SOUNDWIRE || REGMAP_SOUNDWIRE_MBQ || REGMAP_SCCB || REGMAP_I3C) select IRQ_DOMAIN if REGMAP_IRQ bool @@ -46,6 +46,10 @@ config REGMAP_SOUNDWIRE tristate depends on SOUNDWIRE +config REGMAP_SOUNDWIRE_MBQ + tristate + depends on SOUNDWIRE + config REGMAP_SCCB tristate depends on I2C diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile index ff6c7d8ec1cd..23841535b250 100644 --- a/drivers/base/regmap/Makefile +++ b/drivers/base/regmap/Makefile @@ -15,5 +15,6 @@ obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o obj-$(CONFIG_REGMAP_IRQ) += regmap-irq.o obj-$(CONFIG_REGMAP_W1) += regmap-w1.o obj-$(CONFIG_REGMAP_SOUNDWIRE) += regmap-sdw.o +obj-$(CONFIG_REGMAP_SOUNDWIRE_MBQ) += regmap-sdw-mbq.o obj-$(CONFIG_REGMAP_SCCB) += regmap-sccb.o obj-$(CONFIG_REGMAP_I3C) += regmap-i3c.o diff --git a/drivers/base/regmap/regmap-sdw-mbq.c b/drivers/base/regmap/regmap-sdw-mbq.c new file mode 100644 index 000000000000..8f4aa691887c --- /dev/null +++ b/drivers/base/regmap/regmap-sdw-mbq.c @@ -0,0 +1,102 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright(c) 2020 Intel Corporation. + +#include <linux/device.h> +#include <linux/errno.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/soundwire/sdw.h> +#include <linux/soundwire/sdw_registers.h> +#include "internal.h" + +static int regmap_sdw_mbq_write(void *context, unsigned int reg, unsigned int val) +{ + struct device *dev = context; + struct sdw_slave *slave = dev_to_sdw_dev(dev); + int ret; + + ret = sdw_write(slave, SDW_SDCA_MBQ_CTL(reg), (val >> 8) & 0xff); + if (ret < 0) + return ret; + + return sdw_write(slave, reg, val & 0xff); +} + +static int regmap_sdw_mbq_read(void *context, unsigned int reg, unsigned int *val) +{ + struct device *dev = context; + struct sdw_slave *slave = dev_to_sdw_dev(dev); + int read0; + int read1; + + read0 = sdw_read(slave, reg); + if (read0 < 0) + return read0; + + read1 = sdw_read(slave, SDW_SDCA_MBQ_CTL(reg)); + if (read1 < 0) + return read1; + + *val = (read1 << 8) | read0; + + return 0; +} + +static struct regmap_bus regmap_sdw_mbq = { + .reg_read = regmap_sdw_mbq_read, + .reg_write = regmap_sdw_mbq_write, + .reg_format_endian_default = REGMAP_ENDIAN_LITTLE, + .val_format_endian_default = REGMAP_ENDIAN_LITTLE, +}; + +static int regmap_sdw_mbq_config_check(const struct regmap_config *config) +{ + /* MBQ-based controls are only 16-bits for now */ + if (config->val_bits != 16) + return -EOPNOTSUPP; + + /* Registers are 32 bits wide */ + if (config->reg_bits != 32) + return -EOPNOTSUPP; + + if (config->pad_bits != 0) + return -EOPNOTSUPP; + + return 0; +} + +struct regmap *__regmap_init_sdw_mbq(struct sdw_slave *sdw, + const struct regmap_config *config, + struct lock_class_key *lock_key, + const char *lock_name) +{ + int ret; + + ret = regmap_sdw_mbq_config_check(config); + if (ret) + return ERR_PTR(ret); + + return __regmap_init(&sdw->dev, ®map_sdw_mbq, + &sdw->dev, config, lock_key, lock_name); +} +EXPORT_SYMBOL_GPL(__regmap_init_sdw_mbq); + +struct regmap *__devm_regmap_init_sdw_mbq(struct sdw_slave *sdw, + const struct regmap_config *config, + struct lock_class_key *lock_key, + const char *lock_name) +{ + int ret; + + ret = regmap_sdw_mbq_config_check(config); + if (ret) + return ERR_PTR(ret); + + return __devm_regmap_init(&sdw->dev, ®map_sdw_mbq, + &sdw->dev, config, lock_key, lock_name); +} +EXPORT_SYMBOL_GPL(__devm_regmap_init_sdw_mbq); + +MODULE_DESCRIPTION("Regmap SoundWire Module"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 1970ed59d49f..506e2a97d381 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -567,6 +567,10 @@ struct regmap *__regmap_init_sdw(struct sdw_slave *sdw, const struct regmap_config *config, struct lock_class_key *lock_key, const char *lock_name); +struct regmap *__regmap_init_sdw_mbq(struct sdw_slave *sdw, + const struct regmap_config *config, + struct lock_class_key *lock_key, + const char *lock_name); struct regmap *__devm_regmap_init(struct device *dev, const struct regmap_bus *bus, @@ -612,6 +616,10 @@ struct regmap *__devm_regmap_init_sdw(struct sdw_slave *sdw, const struct regmap_config *config, struct lock_class_key *lock_key, const char *lock_name); +struct regmap *__devm_regmap_init_sdw_mbq(struct sdw_slave *sdw, + const struct regmap_config *config, + struct lock_class_key *lock_key, + const char *lock_name); struct regmap *__devm_regmap_init_slimbus(struct slim_device *slimbus, const struct regmap_config *config, struct lock_class_key *lock_key, @@ -806,6 +814,18 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg); __regmap_lockdep_wrapper(__regmap_init_sdw, #config, \ sdw, config) +/** + * regmap_init_sdw_mbq() - Initialise register map + * + * @sdw: Device that will be interacted with + * @config: Configuration for register map + * + * The return value will be an ERR_PTR() on error or a valid pointer to + * a struct regmap. + */ +#define regmap_init_sdw_mbq(sdw, config) \ + __regmap_lockdep_wrapper(__regmap_init_sdw_mbq, #config, \ + sdw, config) /** * devm_regmap_init() - Initialise managed register map -- 2.25.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ 2020-08-25 17:16 ` [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ Pierre-Louis Bossart @ 2020-08-26 0:59 ` Bard liao 2020-08-26 9:05 ` Vinod Koul 2020-08-26 10:16 ` Mark Brown 2 siblings, 0 replies; 26+ messages in thread From: Bard liao @ 2020-08-26 0:59 UTC (permalink / raw) To: Pierre-Louis Bossart, alsa-devel Cc: linux-kernel, tiwai, broonie, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki On 8/26/2020 1:16 AM, Pierre-Louis Bossart wrote: > The SoundWire 1.1 specification only allowed for reads and writes of > bytes. The SoundWire 1.2 specification adds a new capability to > transfer "Multi-Byte Quantities" (MBQ) across the bus. The transfers > still happens one-byte-at-a-time, but the update is atomic. > > For example when writing a 16-bit volume, the first byte transferred > is only taken into account when the second byte is successfully > transferred. > > The mechanism is symmetrical for read and writes: > - On a read, the address of the last byte to be read is modified by > setting the MBQ bit > - On a write, the address of all but the last byte to be written are > modified by setting the MBQ bit. The address for the last byte relies > on the MBQ bit being cleared. > > The current definitions for MBQ-based controls in the SDCA draft > standard are limited to 16 bits for volumes, so for now this is the > only supported format. An update will be provided if and when support > for 24-bit and 32-bit values is specified by the SDCA standard. > > One possible objection is that this code could have been handled with > regmap-sdw.c. However this is a new spec addition not handled by every > SoundWire 1.1 and non-SDCA device, so there's no reason to load code > that will never be used. > > Also in practice it's extremely unlikely that CONFIG_REGMAP would not > be selected with CONFIG_REGMAP_MBQ selected. However there's no > functional dependency between the two modules so they can be selected > separately. > > Reviewed-by: Rander Wang <rander.wang@linux.intel.com> > Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> > Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Acked-by: Bard Liao <yung-chuan.liao@linux.intel.com> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ 2020-08-25 17:16 ` [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ Pierre-Louis Bossart 2020-08-26 0:59 ` Bard liao @ 2020-08-26 9:05 ` Vinod Koul 2020-08-26 16:57 ` Pierre-Louis Bossart 2020-08-26 10:16 ` Mark Brown 2 siblings, 1 reply; 26+ messages in thread From: Vinod Koul @ 2020-08-26 9:05 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: alsa-devel, linux-kernel, tiwai, broonie, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki On 25-08-20, 12:16, Pierre-Louis Bossart wrote: > The SoundWire 1.1 specification only allowed for reads and writes of > bytes. The SoundWire 1.2 specification adds a new capability to > transfer "Multi-Byte Quantities" (MBQ) across the bus. The transfers > still happens one-byte-at-a-time, but the update is atomic. > > For example when writing a 16-bit volume, the first byte transferred > is only taken into account when the second byte is successfully > transferred. > > The mechanism is symmetrical for read and writes: > - On a read, the address of the last byte to be read is modified by > setting the MBQ bit > - On a write, the address of all but the last byte to be written are > modified by setting the MBQ bit. The address for the last byte relies > on the MBQ bit being cleared. > > The current definitions for MBQ-based controls in the SDCA draft > standard are limited to 16 bits for volumes, so for now this is the > only supported format. An update will be provided if and when support > for 24-bit and 32-bit values is specified by the SDCA standard. > > One possible objection is that this code could have been handled with > regmap-sdw.c. However this is a new spec addition not handled by every > SoundWire 1.1 and non-SDCA device, so there's no reason to load code > that will never be used. > > Also in practice it's extremely unlikely that CONFIG_REGMAP would not > be selected with CONFIG_REGMAP_MBQ selected. However there's no > functional dependency between the two modules so they can be selected > separately. Is there a reason for a new module for mbq writes, cant we do this as part of sdw module? Driver can invoke either regmap_init_sdw() or regmap_init_sdw_mbq()? > +++ b/drivers/base/regmap/regmap-sdw-mbq.c > @@ -0,0 +1,102 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright(c) 2020 Intel Corporation. > + > +#include <linux/device.h> > +#include <linux/errno.h> > +#include <linux/mod_devicetable.h> Curious why do you need this header? > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/soundwire/sdw.h> > +#include <linux/soundwire/sdw_registers.h> > +#include "internal.h" > + > +static int regmap_sdw_mbq_write(void *context, unsigned int reg, unsigned int val) > +{ > + struct device *dev = context; > + struct sdw_slave *slave = dev_to_sdw_dev(dev); > + int ret; > + > + ret = sdw_write(slave, SDW_SDCA_MBQ_CTL(reg), (val >> 8) & 0xff); > + if (ret < 0) > + return ret; > + > + return sdw_write(slave, reg, val & 0xff); > +} > + > +static int regmap_sdw_mbq_read(void *context, unsigned int reg, unsigned int *val) > +{ > + struct device *dev = context; > + struct sdw_slave *slave = dev_to_sdw_dev(dev); > + int read0; > + int read1; > + > + read0 = sdw_read(slave, reg); > + if (read0 < 0) > + return read0; > + > + read1 = sdw_read(slave, SDW_SDCA_MBQ_CTL(reg)); > + if (read1 < 0) > + return read1; > + > + *val = (read1 << 8) | read0; > + > + return 0; > +} > + > +static struct regmap_bus regmap_sdw_mbq = { > + .reg_read = regmap_sdw_mbq_read, > + .reg_write = regmap_sdw_mbq_write, > + .reg_format_endian_default = REGMAP_ENDIAN_LITTLE, > + .val_format_endian_default = REGMAP_ENDIAN_LITTLE, > +}; > + > +static int regmap_sdw_mbq_config_check(const struct regmap_config *config) > +{ > + /* MBQ-based controls are only 16-bits for now */ > + if (config->val_bits != 16) > + return -EOPNOTSUPP; > + > + /* Registers are 32 bits wide */ > + if (config->reg_bits != 32) > + return -EOPNOTSUPP; > + > + if (config->pad_bits != 0) > + return -EOPNOTSUPP; > + > + return 0; > +} > + > +struct regmap *__regmap_init_sdw_mbq(struct sdw_slave *sdw, > + const struct regmap_config *config, > + struct lock_class_key *lock_key, > + const char *lock_name) > +{ > + int ret; > + > + ret = regmap_sdw_mbq_config_check(config); > + if (ret) > + return ERR_PTR(ret); > + > + return __regmap_init(&sdw->dev, ®map_sdw_mbq, > + &sdw->dev, config, lock_key, lock_name); > +} > +EXPORT_SYMBOL_GPL(__regmap_init_sdw_mbq); > + > +struct regmap *__devm_regmap_init_sdw_mbq(struct sdw_slave *sdw, > + const struct regmap_config *config, > + struct lock_class_key *lock_key, > + const char *lock_name) > +{ > + int ret; > + > + ret = regmap_sdw_mbq_config_check(config); > + if (ret) > + return ERR_PTR(ret); > + > + return __devm_regmap_init(&sdw->dev, ®map_sdw_mbq, > + &sdw->dev, config, lock_key, lock_name); > +} > +EXPORT_SYMBOL_GPL(__devm_regmap_init_sdw_mbq); > + > +MODULE_DESCRIPTION("Regmap SoundWire Module"); This is same of sdw module, pls make this one a bit different. -- ~Vinod ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ 2020-08-26 9:05 ` Vinod Koul @ 2020-08-26 16:57 ` Pierre-Louis Bossart 2020-08-28 7:23 ` Vinod Koul 0 siblings, 1 reply; 26+ messages in thread From: Pierre-Louis Bossart @ 2020-08-26 16:57 UTC (permalink / raw) To: Vinod Koul Cc: alsa-devel, linux-kernel, tiwai, broonie, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki >> +#include <linux/device.h> >> +#include <linux/errno.h> >> +#include <linux/mod_devicetable.h> > > Curious why do you need this header? I'll return the question back to you, since you added this header for regmap-sdw.c: 7c22ce6e21840 (Vinod Koul 2018-01-08 15:50:59 +0530 6) #include <linux/mod_devicetable.h> so I assumed it was needed here as well. >> +MODULE_DESCRIPTION("Regmap SoundWire Module"); > > This is same of sdw module, pls make this one a bit different. yes, fixed. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ 2020-08-26 16:57 ` Pierre-Louis Bossart @ 2020-08-28 7:23 ` Vinod Koul 2020-08-28 14:49 ` Pierre-Louis Bossart 0 siblings, 1 reply; 26+ messages in thread From: Vinod Koul @ 2020-08-28 7:23 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: alsa-devel, linux-kernel, tiwai, broonie, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki On 26-08-20, 11:57, Pierre-Louis Bossart wrote: > > > > +#include <linux/device.h> > > > +#include <linux/errno.h> > > > +#include <linux/mod_devicetable.h> > > > > Curious why do you need this header? > > I'll return the question back to you, since you added this header for > regmap-sdw.c: > > 7c22ce6e21840 (Vinod Koul 2018-01-08 15:50:59 +0530 6) #include > <linux/mod_devicetable.h> Looks like it should be removed :) I could compile it without any issues on few archs I do. let me push the patch on my tree and see if bots are happy, will send the patch > > so I assumed it was needed here as well. > > > > +MODULE_DESCRIPTION("Regmap SoundWire Module"); > > > > This is same of sdw module, pls make this one a bit different. > > yes, fixed. -- ~Vinod ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ 2020-08-28 7:23 ` Vinod Koul @ 2020-08-28 14:49 ` Pierre-Louis Bossart 0 siblings, 0 replies; 26+ messages in thread From: Pierre-Louis Bossart @ 2020-08-28 14:49 UTC (permalink / raw) To: Vinod Koul Cc: Guennadi Liakhovetski, alsa-devel, Kai Vehmanen, Rafael J. Wysocki, tiwai, gregkh, linux-kernel, Ranjani Sridharan, Hui Wang, broonie, srinivas.kandagatla, jank, slawomir.blauciak, Bard liao, Rander Wang >>>> +#include <linux/mod_devicetable.h> >>> >>> Curious why do you need this header? >> >> I'll return the question back to you, since you added this header for >> regmap-sdw.c: >> >> 7c22ce6e21840 (Vinod Koul 2018-01-08 15:50:59 +0530 6) #include >> <linux/mod_devicetable.h> > > Looks like it should be removed :) > > I could compile it without any issues on few archs I do. let me push the > patch on my tree and see if bots are happy, will send the patch I already fixed this on my side, will submit next week. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ 2020-08-25 17:16 ` [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ Pierre-Louis Bossart 2020-08-26 0:59 ` Bard liao 2020-08-26 9:05 ` Vinod Koul @ 2020-08-26 10:16 ` Mark Brown 2020-08-26 14:54 ` Pierre-Louis Bossart 2 siblings, 1 reply; 26+ messages in thread From: Mark Brown @ 2020-08-26 10:16 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: alsa-devel, linux-kernel, tiwai, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki [-- Attachment #1: Type: text/plain, Size: 924 bytes --] On Tue, Aug 25, 2020 at 12:16:56PM -0500, Pierre-Louis Bossart wrote: > One possible objection is that this code could have been handled with > regmap-sdw.c. However this is a new spec addition not handled by every > SoundWire 1.1 and non-SDCA device, so there's no reason to load code > that will never be used. > Also in practice it's extremely unlikely that CONFIG_REGMAP would not > be selected with CONFIG_REGMAP_MBQ selected. However there's no > functional dependency between the two modules so they can be selected > separately. The other thing I'm wondering here is about compatibility - is this something we can enumerate at runtime and if so couldn't this be done more like how we handle the various I2C and SMBus variants so the driver just says it wants a SoundWire regmap and then based on the capabilities of the device and the controller the regmap decides if it can use MBQ or not on the current system? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ 2020-08-26 10:16 ` Mark Brown @ 2020-08-26 14:54 ` Pierre-Louis Bossart 0 siblings, 0 replies; 26+ messages in thread From: Pierre-Louis Bossart @ 2020-08-26 14:54 UTC (permalink / raw) To: Mark Brown Cc: alsa-devel, linux-kernel, tiwai, vkoul, gregkh, jank, srinivas.kandagatla, slawomir.blauciak, Bard liao, Rander Wang, Ranjani Sridharan, Hui Wang, Guennadi Liakhovetski, Kai Vehmanen, Rafael J. Wysocki >> One possible objection is that this code could have been handled with >> regmap-sdw.c. However this is a new spec addition not handled by every >> SoundWire 1.1 and non-SDCA device, so there's no reason to load code >> that will never be used. > >> Also in practice it's extremely unlikely that CONFIG_REGMAP would not >> be selected with CONFIG_REGMAP_MBQ selected. However there's no >> functional dependency between the two modules so they can be selected >> separately. > > The other thing I'm wondering here is about compatibility - is this > something we can enumerate at runtime and if so couldn't this be done > more like how we handle the various I2C and SMBus variants so the driver > just says it wants a SoundWire regmap and then based on the capabilities > of the device and the controller the regmap decides if it can use MBQ or > not on the current system? An SDCA device will have two regmaps, one for 'regular' registers and one for MBQ-based ones. There is no known case where a codec can use ONLY an MBQ-based regmap. It's different from I2C/SMB since the bus is really identical, the interface is the same, the difference is really the sequence by which you access registers allocated to SDCA and how the address is constructed. Each SDCA control will be described with a firmware property, and based on their range and purpose you would know how if the control is a regular one or an MBQ-based one. Alternatively, the driver might hard-code things and define addresses for each. Does this answer to your question? ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2020-08-28 15:18 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-25 17:16 [PATCH 0/4] regmap: add SoundWire 1.2 MBQ support Pierre-Louis Bossart 2020-08-25 17:16 ` [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP Pierre-Louis Bossart 2020-08-25 21:48 ` Mark Brown 2020-08-25 22:08 ` Pierre-Louis Bossart 2020-08-26 9:56 ` Mark Brown 2020-08-26 10:09 ` Takashi Iwai 2020-08-26 10:13 ` Mark Brown 2020-08-26 10:22 ` Takashi Iwai 2020-08-26 12:03 ` Vinod Koul 2020-08-26 15:05 ` Pierre-Louis Bossart 2020-08-26 17:25 ` Mark Brown 2020-08-26 18:08 ` Pierre-Louis Bossart 2020-08-25 17:16 ` [PATCH 2/4] regmap: sdw: add required header files Pierre-Louis Bossart 2020-08-25 17:16 ` [PATCH 3/4] soundwire: SDCA: add helper macro to access controls Pierre-Louis Bossart 2020-08-26 1:04 ` Bard liao 2020-08-26 8:55 ` Vinod Koul 2020-08-26 15:00 ` Pierre-Louis Bossart 2020-08-26 16:40 ` Vinod Koul 2020-08-25 17:16 ` [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ Pierre-Louis Bossart 2020-08-26 0:59 ` Bard liao 2020-08-26 9:05 ` Vinod Koul 2020-08-26 16:57 ` Pierre-Louis Bossart 2020-08-28 7:23 ` Vinod Koul 2020-08-28 14:49 ` Pierre-Louis Bossart 2020-08-26 10:16 ` Mark Brown 2020-08-26 14:54 ` Pierre-Louis Bossart
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).