[PATCH 0/4] regmap: add SoundWire 1.2 MBQ support
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
-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; }
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?
-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.
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.
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
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.
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
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
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?
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.
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.
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"
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 */
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
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
+/* 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.
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
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
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
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.
+#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.
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.
+#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.
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?
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?
participants (5)
-
Bard liao -
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai -
Vinod Koul