[PATCH v2 0/3] 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 second patch was initially suggested for inclusion in the SoundWire tree, and was modified to add more background information on SDCA in the commit message as requested by Vinod Koul.
Changes since v1: Rebased on regmap tree (conflict with SPI stuff). Removed mod_devicetable.h header Removed -EOPNOTSUPP error codes, use -ENOTSUPP Added long description of SDCA Used FIELD_PREP/GET as suggested by Vinod Koul Added Bard Liao's Acked-by tag.
Pierre-Louis Bossart (3): 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 | 101 ++++++++++++++++++++++++ drivers/base/regmap/regmap-sdw.c | 2 + include/linux/regmap.h | 21 +++++ include/linux/soundwire/sdw_registers.h | 33 ++++++++ 6 files changed, 163 insertions(+), 1 deletion(-) create mode 100644 drivers/base/regmap/regmap-sdw-mbq.c
base-commit: d17343b87da422a59d99a3ed130573dbeb96c582
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 c92d614b4943..c83be26434e7 100644 --- a/drivers/base/regmap/regmap-sdw.c +++ b/drivers/base/regmap/regmap-sdw.c @@ -2,7 +2,9 @@ // Copyright(c) 2015-17 Intel Corporation.
#include <linux/device.h> +#include <linux/errno.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 is allocated to SDCA controls, starting at address 0x40000000. The 26 LSBs which identify individual controls are set based on the following variables:
- Function Number. An SCDA device can be split in up to 8 independent Functions. Each of these Functions is described in the SDCA specification, e.g. Smart Amplifier, Smart Microphone, Simple Microphone, Jack codec, HID, etc.
- Entity Number. Within each Function, an Entity is an identifiable block. Up to 127 Entities are connected in a pre-defined graph (similar to USB), with Entity0 reserved for Function-level configurations. In contrast to USB, the SDCA spec pre-defines Function Types, topologies, and allowed options, i.e. the degree of freedom is not unlimited to limit the possibility of errors in descriptors leading to software quirks.
- Control Selector. Within each Entity, the SDCA specification defines up-to 48 controls such as Mute, Gain, AGC, etc, and 16 implementation defined ones. Some Control Selectors might be used for low-level platform setup, and other exposed to applications and users. Note that the same Control Selector capability, e.g. Latency control, might be located at different offsets in different entities - the Control Selector mapping is Entity-specific.
- Control Number. Some Control Selectors allow channel-specific values to be set, with up to 64 channels allowed. This is mostly used for volume control.
- Current/Next values. Some Control Selectors are 'Dual-Ranked'. Software may either update the Current value directly for immediate effect. Alternatively, software may write into the 'Next' values and update the SoundWire 1.2 'Commit Groups' register to copy 'Next' values into 'Current' ones in a synchronized manner. This is different from bank switching which is typically used to change the bus configuration only.
- MBQ. the Multi-Byte Quantity bit is used to provide atomic updates when accessing more that one byte, for example a 16-bit volume control would be updated consistently, the intermediate values mixing old MSB with new LSB are not applied.
These 6 parameters are used to build a 32-bit address to access the desired Controls. Because of address range, paging is required, but the most often used parameter values are placed in the lower 16 bits of the address. This helps to keep the paging registers constant while updating Controls for a specific Device/Function.
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 Acked-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- include/linux/soundwire/sdw_registers.h | 33 +++++++++++++++++++++++++ 1 file changed, 33 insertions(+)
diff --git a/include/linux/soundwire/sdw_registers.h b/include/linux/soundwire/sdw_registers.h index 5d3c271af7d1..99ff7afc27a2 100644 --- a/include/linux/soundwire/sdw_registers.h +++ b/include/linux/soundwire/sdw_registers.h @@ -305,4 +305,37 @@ #define SDW_CASC_PORT_MASK_INTSTAT3 1 #define SDW_CASC_PORT_REG_OFFSET_INTSTAT3 2
+/* + * v1.2 device - SDCA address mapping + * + * Spec definition + * Bits Contents + * 31 0 (required by addressing range) + * 30:26 0b10000 (Control Prefix) + * 25 0 (Reserved) + * 24:22 Function Number [2:0] + * 21 Entity[6] + * 20:19 Control Selector[5:4] + * 18 0 (Reserved) + * 17:15 Control Number[5:3] + * 14 Next + * 13 MBQ + * 12:7 Entity[5:0] + * 6:3 Control Selector[3:0] + * 2:0 Control Number[2:0] + */ + +#define SDW_SDCA_CTL(fun, ent, ctl, ch) \ + (BIT(30) | \ + FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun))) | \ + FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent))) | \ + FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) | \ + FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch))) | \ + FIELD_PREP(GENMASK(12, 7), FIELD_GET(GENMASK(5, 0), (ent))) | \ + FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) | \ + FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch)))) + +#define SDW_SDCA_MBQ_CTL(reg) ((reg) | BIT(13)) +#define SDW_SDCA_NEXT_CTL(reg) ((reg) | BIT(14)) + #endif /* __SDW_REGISTERS_H */
On 01-09-20, 11:22, Pierre-Louis Bossart wrote:
The upcoming SDCA (SoundWire Device Class Audio) specification defines a hierarchical encoding to interface with Class-defined capabilities.
The specification is not yet accessible to the general public but this information is released with explicit permission from the MIPI Board to avoid delays with SDCA support on Linux platforms.
A block of 64 MBytes of register addresses is allocated to SDCA controls, starting at address 0x40000000. The 26 LSBs which identify individual controls are set based on the following variables:
Function Number. An SCDA device can be split in up to 8 independent Functions. Each of these Functions is described in the SDCA specification, e.g. Smart Amplifier, Smart Microphone, Simple Microphone, Jack codec, HID, etc.
Entity Number. Within each Function, an Entity is an identifiable block. Up to 127 Entities are connected in a pre-defined graph (similar to USB), with Entity0 reserved for Function-level configurations. In contrast to USB, the SDCA spec pre-defines Function Types, topologies, and allowed options, i.e. the degree of freedom is not unlimited to limit the possibility of errors in descriptors leading to software quirks.
Control Selector. Within each Entity, the SDCA specification defines up-to 48 controls such as Mute, Gain, AGC, etc, and 16 implementation defined ones. Some Control Selectors might be used for low-level platform setup, and other exposed to applications and users. Note that the same Control Selector capability, e.g. Latency control, might be located at different offsets in different entities - the Control Selector mapping is Entity-specific.
Control Number. Some Control Selectors allow channel-specific values to be set, with up to 64 channels allowed. This is mostly used for volume control.
Current/Next values. Some Control Selectors are 'Dual-Ranked'. Software may either update the Current value directly for immediate effect. Alternatively, software may write into the 'Next' values and update the SoundWire 1.2 'Commit Groups' register to copy 'Next' values into 'Current' ones in a synchronized manner. This is different from bank switching which is typically used to change the bus configuration only.
MBQ. the Multi-Byte Quantity bit is used to provide atomic updates when accessing more that one byte, for example a 16-bit volume control would be updated consistently, the intermediate values mixing old MSB with new LSB are not applied.
These 6 parameters are used to build a 32-bit address to access the desired Controls. Because of address range, paging is required, but the most often used parameter values are placed in the lower 16 bits of the address. This helps to keep the paging registers constant while updating Controls for a specific Device/Function.
This is good, thanks for adding it in changelog. Can you also add this description to Documentation (that can come as an individual patch),
Reviewed-by: Rander Wang rander.wang@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Acked-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
include/linux/soundwire/sdw_registers.h | 33 +++++++++++++++++++++++++ 1 file changed, 33 insertions(+)
diff --git a/include/linux/soundwire/sdw_registers.h b/include/linux/soundwire/sdw_registers.h index 5d3c271af7d1..99ff7afc27a2 100644 --- a/include/linux/soundwire/sdw_registers.h +++ b/include/linux/soundwire/sdw_registers.h @@ -305,4 +305,37 @@ #define SDW_CASC_PORT_MASK_INTSTAT3 1 #define SDW_CASC_PORT_REG_OFFSET_INTSTAT3 2
+/*
- v1.2 device - SDCA address mapping
- Spec definition
- Bits Contents
- 31 0 (required by addressing range)
- 30:26 0b10000 (Control Prefix)
So this is for 30:26
- 25 0 (Reserved)
- 24:22 Function Number [2:0]
- 21 Entity[6]
- 20:19 Control Selector[5:4]
- 18 0 (Reserved)
- 17:15 Control Number[5:3]
- 14 Next
- 13 MBQ
- 12:7 Entity[5:0]
- 6:3 Control Selector[3:0]
- 2:0 Control Number[2:0]
- */
+#define SDW_SDCA_CTL(fun, ent, ctl, ch) \
- (BIT(30) | \
Programmatically this is fine, but then since we are defining for the description above, IMO it would actually make sense for this to be defined as FIELD_PREP:
FIELD_PREP(GENMASK(30, 26), 1)
or better
u32_encode_bits(GENMASK(30, 26), 1)
- FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun))) | \
Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and below?
- FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent))) | \
- FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) | \
- FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch))) | \
- FIELD_PREP(GENMASK(12, 7), FIELD_GET(GENMASK(5, 0), (ent))) | \
- FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) | \
- FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch))))
Also, can we rather have a nice function for this, that would look much cleaner
And while at it, consider defining masks for various fields rather than using numbers in GENMASK() above, that would look better, be more readable and people can reuse it.
Thanks for the review Vinod,
This is good, thanks for adding it in changelog. Can you also add this description to Documentation (that can come as an individual patch),
ok
+/*
- v1.2 device - SDCA address mapping
- Spec definition
- Bits Contents
- 31 0 (required by addressing range)
- 30:26 0b10000 (Control Prefix)
So this is for 30:26
I don't get the comment, sorry.
- 25 0 (Reserved)
- 24:22 Function Number [2:0]
- 21 Entity[6]
- 20:19 Control Selector[5:4]
- 18 0 (Reserved)
- 17:15 Control Number[5:3]
- 14 Next
- 13 MBQ
- 12:7 Entity[5:0]
- 6:3 Control Selector[3:0]
- 2:0 Control Number[2:0]
- */
+#define SDW_SDCA_CTL(fun, ent, ctl, ch) \
- (BIT(30) | \
Programmatically this is fine, but then since we are defining for the description above, IMO it would actually make sense for this to be defined as FIELD_PREP:
FIELD_PREP(GENMASK(30, 26), 1)or better
u32_encode_bits(GENMASK(30, 26), 1)
- FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun))) | \
Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and below?
Because your comment for the v1 review was to use FIELD_PREP/FIELD_GET, and your other patches for bitfield access only use FIELD_PREP/FIELD_GET.
I really don't care about which macro is used but it wouldn't hurt to have some level of consistency between different parts of the code? Why not use FIELD_PREP/GET everywhere?
- FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent))) | \
- FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) | \
- FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch))) | \
- FIELD_PREP(GENMASK(12, 7), FIELD_GET(GENMASK(5, 0), (ent))) | \
- FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) | \
- FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch))))
Also, can we rather have a nice function for this, that would look much cleaner
I am not sure what would be cleaner but fine.
And while at it, consider defining masks for various fields rather than using numbers in GENMASK() above, that would look better, be more readable and people can reuse it.
Actually on this one I disagree. These fields are not intended to be used by anyone, the goal is precisely to hide them behind regmap, and the use of raw numbers makes it easier to cross-check the documentation and the code. Adding a separate set of definitions would not increase readability.
On 08-09-20, 08:33, Pierre-Louis Bossart wrote:
Thanks for the review Vinod,
This is good, thanks for adding it in changelog. Can you also add this description to Documentation (that can come as an individual patch),
ok
+/*
- v1.2 device - SDCA address mapping
- Spec definition
- Bits Contents
- 31 0 (required by addressing range)
- 30:26 0b10000 (Control Prefix)
So this is for 30:26
I don't get the comment, sorry.
I should have added see below.
- 25 0 (Reserved)
- 24:22 Function Number [2:0]
- 21 Entity[6]
- 20:19 Control Selector[5:4]
- 18 0 (Reserved)
- 17:15 Control Number[5:3]
- 14 Next
- 13 MBQ
- 12:7 Entity[5:0]
- 6:3 Control Selector[3:0]
- 2:0 Control Number[2:0]
- */
+#define SDW_SDCA_CTL(fun, ent, ctl, ch) \
- (BIT(30) | \
Programmatically this is fine, but then since we are defining for the description above, IMO it would actually make sense for this to be defined as FIELD_PREP:
FIELD_PREP(GENMASK(30, 26), 1)or better
u32_encode_bits(GENMASK(30, 26), 1)
- FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun))) | \
Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and below?
Because your comment for the v1 review was to use FIELD_PREP/FIELD_GET, and your other patches for bitfield access only use FIELD_PREP/FIELD_GET.
yes and looking at this, I feel u32_encode_bits(GENMASK(24, 22), (fun)) would look better than FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))
Do you agree?
I really don't care about which macro is used but it wouldn't hurt to have some level of consistency between different parts of the code? Why not use FIELD_PREP/GET everywhere?
- FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent))) | \
- FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) | \
- FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch))) | \
- FIELD_PREP(GENMASK(12, 7), FIELD_GET(GENMASK(5, 0), (ent))) | \
- FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) | \
- FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch))))
Also, can we rather have a nice function for this, that would look much cleaner
I am not sure what would be cleaner but fine.
Ok
And while at it, consider defining masks for various fields rather than using numbers in GENMASK() above, that would look better, be more readable and people can reuse it.
Actually on this one I disagree. These fields are not intended to be used by anyone, the goal is precisely to hide them behind regmap, and the use of raw numbers makes it easier to cross-check the documentation and the code. Adding a separate set of definitions would not increase readability.
Which one would you prefer:
#define SDCA_FUN_MASK GENMASK(24, 22)
foo |= u32_encode_bits(SDCA_FUN_MASK, fun)
Or the one proposed...?
- 25 0 (Reserved)
- 24:22 Function Number [2:0]
- 21 Entity[6]
- 20:19 Control Selector[5:4]
- 18 0 (Reserved)
- 17:15 Control Number[5:3]
- 14 Next
- 13 MBQ
- 12:7 Entity[5:0]
- 6:3 Control Selector[3:0]
- 2:0 Control Number[2:0]
- */
+#define SDW_SDCA_CTL(fun, ent, ctl, ch) \
- (BIT(30) | \
Programmatically this is fine, but then since we are defining for the description above, IMO it would actually make sense for this to be defined as FIELD_PREP:
FIELD_PREP(GENMASK(30, 26), 1)or better
u32_encode_bits(GENMASK(30, 26), 1)
- FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun))) | \
Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and below?
Because your comment for the v1 review was to use FIELD_PREP/FIELD_GET, and your other patches for bitfield access only use FIELD_PREP/FIELD_GET.
yes and looking at this, I feel u32_encode_bits(GENMASK(24, 22), (fun)) would look better than FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))
Do you agree?
The Function (fun) case is the easy one: the value is not split in two.
But look at the entity case, it's split in two:
FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent))) FIELD_PREP(GENMASK(12, 7), FIELD_GET(GENMASK(5, 0), (ent)))
same for control
FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) | FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) |
and same for channel number
FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch))) | FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch))))
I don't see how we can avoid using the FIELD_GET to extract the relevant bits from entity, control, channel number values.
Or I am missing your point completely.
And while at it, consider defining masks for various fields rather than using numbers in GENMASK() above, that would look better, be more readable and people can reuse it.
Actually on this one I disagree. These fields are not intended to be used by anyone, the goal is precisely to hide them behind regmap, and the use of raw numbers makes it easier to cross-check the documentation and the code. Adding a separate set of definitions would not increase readability.
Which one would you prefer:
#define SDCA_FUN_MASK GENMASK(24, 22) foo |= u32_encode_bits(SDCA_FUN_MASK, fun)Or the one proposed...?
Same as above, let's see what this does with the control case where we'd need to have four definitions:
#define SDCA_CONTROL_DEST_MASK1 GENMASK(20, 19) #define SDCA_CONTROL_ORIG_MASK1 GENMASK(5, 4) #define SDCA_CONTROL_DEST_MASK2 GENMASK(6, 3) #define SDCA_CONTROL_ORIG_MASK2 GENMASK(3, 0)
And the code would look like
foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK1, FIELD_GET(SDCA_CONTROL_ORIG_MASK1, fun)); foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK2, FIELD_GET(SDCA_CONTROL_ORIG_MASK2, fun));
The original suggestion was:
FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) | FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) |
I prefer the original... Adding these defines doesn't really add value because a) the values will not be reused anywhere else. b) we need 12 of those defines b) we need a prefix for those defines which makes the code heavier
On 09-09-20, 08:48, Pierre-Louis Bossart wrote:
- 25 0 (Reserved)
- 24:22 Function Number [2:0]
- 21 Entity[6]
- 20:19 Control Selector[5:4]
- 18 0 (Reserved)
- 17:15 Control Number[5:3]
- 14 Next
- 13 MBQ
- 12:7 Entity[5:0]
- 6:3 Control Selector[3:0]
- 2:0 Control Number[2:0]
- */
+#define SDW_SDCA_CTL(fun, ent, ctl, ch) \
- (BIT(30) | \
Programmatically this is fine, but then since we are defining for the description above, IMO it would actually make sense for this to be defined as FIELD_PREP:
FIELD_PREP(GENMASK(30, 26), 1)or better
u32_encode_bits(GENMASK(30, 26), 1)
- FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun))) | \
Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and below?
Because your comment for the v1 review was to use FIELD_PREP/FIELD_GET, and your other patches for bitfield access only use FIELD_PREP/FIELD_GET.
yes and looking at this, I feel u32_encode_bits(GENMASK(24, 22), (fun)) would look better than FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))
Do you agree?
The Function (fun) case is the easy one: the value is not split in two.
But look at the entity case, it's split in two:
FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent))) FIELD_PREP(GENMASK(12, 7), FIELD_GET(GENMASK(5, 0), (ent)))
same for control
FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) | FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) |
and same for channel number
FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch))) | FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch))))
I don't see how we can avoid using the FIELD_GET to extract the relevant bits from entity, control, channel number values.
No, you dont need FIELD_GET, that would be pointless for this helper if that was the case
Or I am missing your point completely.
Correct
It should be:
foo |= u32_encode_bits(val, FOO_MASK_A);
which would write val into bits represented by FOO_MASK_A by appropriately shifting val and masking it with FOO_MASK_A
So net result is bits in FOO_MASK_A are modified with val, rest of the bits are not touched
And while at it, consider defining masks for various fields rather than using numbers in GENMASK() above, that would look better, be more readable and people can reuse it.
Actually on this one I disagree. These fields are not intended to be used by anyone, the goal is precisely to hide them behind regmap, and the use of raw numbers makes it easier to cross-check the documentation and the code. Adding a separate set of definitions would not increase readability.
Which one would you prefer:
#define SDCA_FUN_MASK GENMASK(24, 22) foo |= u32_encode_bits(SDCA_FUN_MASK, fun)Or the one proposed...?
Same as above, let's see what this does with the control case where we'd need to have four definitions:
#define SDCA_CONTROL_DEST_MASK1 GENMASK(20, 19) #define SDCA_CONTROL_ORIG_MASK1 GENMASK(5, 4) #define SDCA_CONTROL_DEST_MASK2 GENMASK(6, 3) #define SDCA_CONTROL_ORIG_MASK2 GENMASK(3, 0)
And the code would look like
foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK1, FIELD_GET(SDCA_CONTROL_ORIG_MASK1, fun)); foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK2, FIELD_GET(SDCA_CONTROL_ORIG_MASK2, fun));
The original suggestion was:
FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) | FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) |
I prefer the original... Adding these defines doesn't really add value because a) the values will not be reused anywhere else. b) we need 12 of those defines b) we need a prefix for those defines which makes the code heavier
On 9/10/20 1:22 AM, Vinod Koul wrote:
On 09-09-20, 08:48, Pierre-Louis Bossart wrote:
- 25 0 (Reserved)
- 24:22 Function Number [2:0]
- 21 Entity[6]
- 20:19 Control Selector[5:4]
- 18 0 (Reserved)
- 17:15 Control Number[5:3]
- 14 Next
- 13 MBQ
- 12:7 Entity[5:0]
- 6:3 Control Selector[3:0]
- 2:0 Control Number[2:0]
- */
+#define SDW_SDCA_CTL(fun, ent, ctl, ch) \
- (BIT(30) | \
Programmatically this is fine, but then since we are defining for the description above, IMO it would actually make sense for this to be defined as FIELD_PREP:
FIELD_PREP(GENMASK(30, 26), 1)or better
u32_encode_bits(GENMASK(30, 26), 1)
- FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun))) | \
Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and below?
Because your comment for the v1 review was to use FIELD_PREP/FIELD_GET, and your other patches for bitfield access only use FIELD_PREP/FIELD_GET.
yes and looking at this, I feel u32_encode_bits(GENMASK(24, 22), (fun)) would look better than FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))
Do you agree?
The Function (fun) case is the easy one: the value is not split in two.
But look at the entity case, it's split in two:
FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent))) FIELD_PREP(GENMASK(12, 7), FIELD_GET(GENMASK(5, 0), (ent)))
same for control
FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) | FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) |
and same for channel number
FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch))) | FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch))))
I don't see how we can avoid using the FIELD_GET to extract the relevant bits from entity, control, channel number values.
No, you dont need FIELD_GET, that would be pointless for this helper if that was the case
I don't get how one would specify which parts of the original value are extracted?
Or I am missing your point completely.
Correct
It should be:
foo |= u32_encode_bits(val, FOO_MASK_A);which would write val into bits represented by FOO_MASK_A by appropriately shifting val and masking it with FOO_MASK_A
So net result is bits in FOO_MASK_A are modified with val, rest of the bits are not touched
Vinod, please see the explanation below [1], we need to split the original value in two and insert the bits in two separate locations.
You only considered the simple case for the functions, your proposal will not work for entities, controls and channel numbers.
And while at it, consider defining masks for various fields rather than using numbers in GENMASK() above, that would look better, be more readable and people can reuse it.
Actually on this one I disagree. These fields are not intended to be used by anyone, the goal is precisely to hide them behind regmap, and the use of raw numbers makes it easier to cross-check the documentation and the code. Adding a separate set of definitions would not increase readability.
Which one would you prefer:
#define SDCA_FUN_MASK GENMASK(24, 22) foo |= u32_encode_bits(SDCA_FUN_MASK, fun)Or the one proposed...?
Same as above, let's see what this does with the control case where we'd need to have four definitions:
[1]
#define SDCA_CONTROL_DEST_MASK1 GENMASK(20, 19) #define SDCA_CONTROL_ORIG_MASK1 GENMASK(5, 4) #define SDCA_CONTROL_DEST_MASK2 GENMASK(6, 3) #define SDCA_CONTROL_ORIG_MASK2 GENMASK(3, 0)
And the code would look like
foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK1, FIELD_GET(SDCA_CONTROL_ORIG_MASK1, fun)); foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK2, FIELD_GET(SDCA_CONTROL_ORIG_MASK2, fun));
The original suggestion was:
FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) | FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) |
I prefer the original... Adding these defines doesn't really add value because a) the values will not be reused anywhere else. b) we need 12 of those defines b) we need a prefix for those defines which makes the code heavier
On 10-09-20, 08:53, Pierre-Louis Bossart wrote:
On 9/10/20 1:22 AM, Vinod Koul wrote:
On 09-09-20, 08:48, Pierre-Louis Bossart wrote:
> + * 25 0 (Reserved) > + * 24:22 Function Number [2:0] > + * 21 Entity[6] > + * 20:19 Control Selector[5:4] > + * 18 0 (Reserved) > + * 17:15 Control Number[5:3] > + * 14 Next > + * 13 MBQ > + * 12:7 Entity[5:0] > + * 6:3 Control Selector[3:0] > + * 2:0 Control Number[2:0] > + */ > + > +#define SDW_SDCA_CTL(fun, ent, ctl, ch) \ > + (BIT(30) | \
Programmatically this is fine, but then since we are defining for the description above, IMO it would actually make sense for this to be defined as FIELD_PREP:
FIELD_PREP(GENMASK(30, 26), 1)or better
u32_encode_bits(GENMASK(30, 26), 1)> + FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun))) | \
Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and below?
Because your comment for the v1 review was to use FIELD_PREP/FIELD_GET, and your other patches for bitfield access only use FIELD_PREP/FIELD_GET.
yes and looking at this, I feel u32_encode_bits(GENMASK(24, 22), (fun)) would look better than FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))
Do you agree?
The Function (fun) case is the easy one: the value is not split in two.
But look at the entity case, it's split in two:
FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent))) FIELD_PREP(GENMASK(12, 7), FIELD_GET(GENMASK(5, 0), (ent)))
same for control
FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) | FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) |
and same for channel number
FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch))) | FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch))))
I don't see how we can avoid using the FIELD_GET to extract the relevant bits from entity, control, channel number values.
No, you dont need FIELD_GET, that would be pointless for this helper if that was the case
I don't get how one would specify which parts of the original value are extracted?
Or I am missing your point completely.
Correct
It should be:
foo |= u32_encode_bits(val, FOO_MASK_A);which would write val into bits represented by FOO_MASK_A by appropriately shifting val and masking it with FOO_MASK_A
So net result is bits in FOO_MASK_A are modified with val, rest of the bits are not touched
Vinod, please see the explanation below [1], we need to split the original value in two and insert the bits in two separate locations.
You only considered the simple case for the functions, your proposal will not work for entities, controls and channel numbers.
And while at it, consider defining masks for various fields rather than using numbers in GENMASK() above, that would look better, be more readable and people can reuse it.
Actually on this one I disagree. These fields are not intended to be used by anyone, the goal is precisely to hide them behind regmap, and the use of raw numbers makes it easier to cross-check the documentation and the code. Adding a separate set of definitions would not increase readability.
Which one would you prefer:
#define SDCA_FUN_MASK GENMASK(24, 22) foo |= u32_encode_bits(SDCA_FUN_MASK, fun)Or the one proposed...?
Same as above, let's see what this does with the control case where we'd need to have four definitions:
[1]
#define SDCA_CONTROL_DEST_MASK1 GENMASK(20, 19) #define SDCA_CONTROL_ORIG_MASK1 GENMASK(5, 4) #define SDCA_CONTROL_DEST_MASK2 GENMASK(6, 3) #define SDCA_CONTROL_ORIG_MASK2 GENMASK(3, 0)
I think I missed ORIG and DEST stuff, what does this mean here?
Relooking at the bit definition, for example 'Control Number' is defined in both 17:15 as well as 2:0, why is that. Is it split?
How does one program a control number into this?
And the code would look like
foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK1, FIELD_GET(SDCA_CONTROL_ORIG_MASK1, fun)); foo |= u32_encode_bits(SDCA_CONTROL_DEST_MASK2, FIELD_GET(SDCA_CONTROL_ORIG_MASK2, fun));
The original suggestion was:
FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl))) | FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl))) |
I prefer the original... Adding these defines doesn't really add value because a) the values will not be reused anywhere else. b) we need 12 of those defines b) we need a prefix for those defines which makes the code heavier
Hi Vinod,
>> + * 25 0 (Reserved) >> + * 24:22 Function Number [2:0] >> + * 21 Entity[6] >> + * 20:19 Control Selector[5:4] >> + * 18 0 (Reserved) >> + * 17:15 Control Number[5:3] >> + * 14 Next >> + * 13 MBQ >> + * 12:7 Entity[5:0] >> + * 6:3 Control Selector[3:0] >> + * 2:0 Control Number[2:0]
[...]
#define SDCA_CONTROL_DEST_MASK1 GENMASK(20, 19) #define SDCA_CONTROL_ORIG_MASK1 GENMASK(5, 4) #define SDCA_CONTROL_DEST_MASK2 GENMASK(6, 3) #define SDCA_CONTROL_ORIG_MASK2 GENMASK(3, 0)
I think I missed ORIG and DEST stuff, what does this mean here?
If you missed this, it means my explanations are not good enough and I need to make it clearer in the commit log/documentation. Point taken, I'll improve this for the next version.
Relooking at the bit definition, for example 'Control Number' is defined in both 17:15 as well as 2:0, why is that. Is it split?
How does one program a control number into this?
A Control Number is represented on 6 bits.
See the documentation above.
17:15 Control Selector[5:3] 2:0 Control Selector[2:0]
The 3 MSBs for into bits 17:15 of the address, and the 3 LSBs into bits 2:0 of the address. The second part is simpler for Control Number but for entities and control selectors the LSB positions don't match.
Yes it's convoluted but it was well-intended: in most cases, there is a limited number of entities, control selectors, channel numbers, and putting the LSBs together in the 16-LSB of the address helps avoid reprogramming paging registers: all the addresses for a given function typically map into the same page.
That said, I am not sure the optimization is that great in the end, because we end-up having to play with bits for each address. Fewer changes of the paging registers but tons of operations in the core.
I wasn't around when this mapping was defined, and it is what is is now. There's hardware built based on this formula so we have to make it work.
Does this clarify the usage?
If you have a better suggestion that the FIELD_PREP/FIELD_GET use, I am all ears. At the end of the day, the mapping is pre-defined and we don't have any degree of freedom. What I do want is that this macro/inline function is shared by all codec drivers so that we don't have different interpretations of how the address is constructed.
Thanks, -Pierre
Hi Pierre,
On 11-09-20, 09:50, Pierre-Louis Bossart wrote:
> > > + * 25 0 (Reserved) > > > + * 24:22 Function Number [2:0] > > > + * 21 Entity[6] > > > + * 20:19 Control Selector[5:4] > > > + * 18 0 (Reserved) > > > + * 17:15 Control Number[5:3] > > > + * 14 Next > > > + * 13 MBQ > > > + * 12:7 Entity[5:0] > > > + * 6:3 Control Selector[3:0] > > > + * 2:0 Control Number[2:0]
[...]
#define SDCA_CONTROL_DEST_MASK1 GENMASK(20, 19) #define SDCA_CONTROL_ORIG_MASK1 GENMASK(5, 4) #define SDCA_CONTROL_DEST_MASK2 GENMASK(6, 3) #define SDCA_CONTROL_ORIG_MASK2 GENMASK(3, 0)
I think I missed ORIG and DEST stuff, what does this mean here?
If you missed this, it means my explanations are not good enough and I need to make it clearer in the commit log/documentation. Point taken, I'll improve this for the next version.
Relooking at the bit definition, for example 'Control Number' is defined in both 17:15 as well as 2:0, why is that. Is it split?
How does one program a control number into this?
A Control Number is represented on 6 bits.
See the documentation above.
17:15 Control Selector[5:3] 2:0 Control Selector[2:0]
The 3 MSBs for into bits 17:15 of the address, and the 3 LSBs into bits 2:0 of the address. The second part is simpler for Control Number but for entities and control selectors the LSB positions don't match.
Yes it's convoluted but it was well-intended: in most cases, there is a limited number of entities, control selectors, channel numbers, and putting the LSBs together in the 16-LSB of the address helps avoid reprogramming paging registers: all the addresses for a given function typically map into the same page.
That said, I am not sure the optimization is that great in the end, because we end-up having to play with bits for each address. Fewer changes of the paging registers but tons of operations in the core.
I wasn't around when this mapping was defined, and it is what is is now. There's hardware built based on this formula so we have to make it work.
Does this clarify the usage?
Thanks, that is very helpful. I have overlooked this bit.
For LSB bits, I dont think this is an issue. I expect it to work, for example: #define CONTROL_LSB_MASK GENMASK(2, 0) foo |= u32_encode_bits(control, CONTROL_LSB_MASK);
would mask the control value and program that in specific bitfeild.
But for MSB bits, I am not sure above will work so, you may need to extract the bits and then use, for example: #define CONTROL_MSB_BITS GENMASK(5, 3) #define CONTROL_MSB_MASK GENMASK(17, 15)
control = FIELD_GET(CONTROL_MSB_BITS, control); foo |= u32_encode_bits(control, CONTROL_MSB_MASK);
If you have a better suggestion that the FIELD_PREP/FIELD_GET use, I am all ears. At the end of the day, the mapping is pre-defined and we don't have any degree of freedom. What I do want is that this macro/inline function is shared by all codec drivers so that we don't have different interpretations of how the address is constructed.
Absolutely, this need to be defined here and used by everyone else.
For LSB bits, I dont think this is an issue. I expect it to work, for example: #define CONTROL_LSB_MASK GENMASK(2, 0) foo |= u32_encode_bits(control, CONTROL_LSB_MASK);
would mask the control value and program that in specific bitfeild.
But for MSB bits, I am not sure above will work so, you may need to extract the bits and then use, for example: #define CONTROL_MSB_BITS GENMASK(5, 3) #define CONTROL_MSB_MASK GENMASK(17, 15)
control = FIELD_GET(CONTROL_MSB_BITS, control); foo |= u32_encode_bits(control, CONTROL_MSB_MASK);If you have a better suggestion that the FIELD_PREP/FIELD_GET use, I am all ears. At the end of the day, the mapping is pre-defined and we don't have any degree of freedom. What I do want is that this macro/inline function is shared by all codec drivers so that we don't have different interpretations of how the address is constructed.
Absolutely, this need to be defined here and used by everyone else.
Compare:
#define SDCA_CONTROL_MSB_BITS GENMASK(5, 3) #define SDCA_CONTROL_MSB_MASK GENMASK(17, 15) #define SDCA_CONTROL_LSB_MASK GENMASK(2, 0)
foo |= u32_encode_bits(control, SDCA_CONTROL_LSB_MASK); control = FIELD_GET(SDCA_CONTROL_MSB_BITS, control); foo |= u32_encode_bits(control, SDCA_CONTROL_MSB_MASK);
with the original proposal:
foo |= FIELD_GET(GENMASK(2, 0), control)) foo |= FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), control))
it gets worse when the LSB positions don't match, you need another variable and an additional mask.
I don't see how this improves readability? I get that hard-coding magic numbers is a bad thing in general, but in this case there are limited benefits to the use of additional defines.
On 14-09-20, 09:44, Pierre-Louis Bossart wrote:
For LSB bits, I dont think this is an issue. I expect it to work, for example: #define CONTROL_LSB_MASK GENMASK(2, 0) foo |= u32_encode_bits(control, CONTROL_LSB_MASK);
would mask the control value and program that in specific bitfeild.
But for MSB bits, I am not sure above will work so, you may need to extract the bits and then use, for example: #define CONTROL_MSB_BITS GENMASK(5, 3) #define CONTROL_MSB_MASK GENMASK(17, 15)
control = FIELD_GET(CONTROL_MSB_BITS, control); foo |= u32_encode_bits(control, CONTROL_MSB_MASK);If you have a better suggestion that the FIELD_PREP/FIELD_GET use, I am all ears. At the end of the day, the mapping is pre-defined and we don't have any degree of freedom. What I do want is that this macro/inline function is shared by all codec drivers so that we don't have different interpretations of how the address is constructed.
Absolutely, this need to be defined here and used by everyone else.
Compare:
#define SDCA_CONTROL_MSB_BITS GENMASK(5, 3) #define SDCA_CONTROL_MSB_MASK GENMASK(17, 15) #define SDCA_CONTROL_LSB_MASK GENMASK(2, 0)
foo |= u32_encode_bits(control, SDCA_CONTROL_LSB_MASK); control = FIELD_GET(SDCA_CONTROL_MSB_BITS, control); foo |= u32_encode_bits(control, SDCA_CONTROL_MSB_MASK);
with the original proposal:
foo |= FIELD_GET(GENMASK(2, 0), control)) foo |= FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), control))
it gets worse when the LSB positions don't match, you need another variable and an additional mask.
I don't see how this improves readability? I get that hard-coding magic numbers is a bad thing in general, but in this case there are limited benefits to the use of additional defines.
I think it would be prudent to define the masks and use them rather than magic values. Also it makes it future proof
Thanks
On 9/16/20 7:35 AM, Vinod Koul wrote:
On 14-09-20, 09:44, Pierre-Louis Bossart wrote:
For LSB bits, I dont think this is an issue. I expect it to work, for example: #define CONTROL_LSB_MASK GENMASK(2, 0) foo |= u32_encode_bits(control, CONTROL_LSB_MASK);
would mask the control value and program that in specific bitfeild.
But for MSB bits, I am not sure above will work so, you may need to extract the bits and then use, for example: #define CONTROL_MSB_BITS GENMASK(5, 3) #define CONTROL_MSB_MASK GENMASK(17, 15)
control = FIELD_GET(CONTROL_MSB_BITS, control); foo |= u32_encode_bits(control, CONTROL_MSB_MASK);If you have a better suggestion that the FIELD_PREP/FIELD_GET use, I am all ears. At the end of the day, the mapping is pre-defined and we don't have any degree of freedom. What I do want is that this macro/inline function is shared by all codec drivers so that we don't have different interpretations of how the address is constructed.
Absolutely, this need to be defined here and used by everyone else.
Compare:
#define SDCA_CONTROL_MSB_BITS GENMASK(5, 3) #define SDCA_CONTROL_MSB_MASK GENMASK(17, 15) #define SDCA_CONTROL_LSB_MASK GENMASK(2, 0)
foo |= u32_encode_bits(control, SDCA_CONTROL_LSB_MASK); control = FIELD_GET(SDCA_CONTROL_MSB_BITS, control); foo |= u32_encode_bits(control, SDCA_CONTROL_MSB_MASK);
with the original proposal:
foo |= FIELD_GET(GENMASK(2, 0), control)) foo |= FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), control))
it gets worse when the LSB positions don't match, you need another variable and an additional mask.
I don't see how this improves readability? I get that hard-coding magic numbers is a bad thing in general, but in this case there are limited benefits to the use of additional defines.
I think it would be prudent to define the masks and use them rather than magic values. Also it makes it future proof
I don't see your point at all. The values cannot be modified, a different macro would be needed for a standard change.
Anyways, I am not going to argue further, I'll use your code example as is and move on.
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 Acked-by: Bard Liao yung-chuan.liao@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 | 101 +++++++++++++++++++++++++++ include/linux/regmap.h | 21 ++++++ 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 bcb90d8c3960..50b1e2d06a25 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 || REGMAP_SPI_AVMM) + 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 || REGMAP_SPI_AVMM) 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 ac1b69ee4051..33f63adb5b3d 100644 --- a/drivers/base/regmap/Makefile +++ b/drivers/base/regmap/Makefile @@ -15,6 +15,7 @@ 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 obj-$(CONFIG_REGMAP_SPI_AVMM) += regmap-spi-avmm.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..8ce30650b97c --- /dev/null +++ b/drivers/base/regmap/regmap-sdw-mbq.c @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright(c) 2020 Intel Corporation. + +#include <linux/device.h> +#include <linux/errno.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 -ENOTSUPP; + + /* Registers are 32 bits wide */ + if (config->reg_bits != 32) + return -ENOTSUPP; + + if (config->pad_bits != 0) + return -ENOTSUPP; + + 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 MBQ Module"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/regmap.h b/include/linux/regmap.h index d865d8fea535..a031ace22b6b 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 *__regmap_init_spi_avmm(struct spi_device *spi, const struct regmap_config *config, struct lock_class_key *lock_key, @@ -616,6 +620,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, @@ -814,6 +822,19 @@ 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) + /** * regmap_init_spi_avmm() - Initialize register map for Intel SPI Slave * to AVMM Bus Bridge
On 01-09-20, 11:22, Pierre-Louis Bossart wrote:
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 second patch was initially suggested for inclusion in the SoundWire tree, and was modified to add more background information on SDCA in the commit message as requested by Vinod Koul.
Is this targetted towards the sdw tree or regmap tree, I think it may have dependencies so sdw can be used.
It would be nice to mention these things here in cover
Changes since v1: Rebased on regmap tree (conflict with SPI stuff). Removed mod_devicetable.h header Removed -EOPNOTSUPP error codes, use -ENOTSUPP Added long description of SDCA Used FIELD_PREP/GET as suggested by Vinod Koul Added Bard Liao's Acked-by tag.
Pierre-Louis Bossart (3): 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 | 101 ++++++++++++++++++++++++ drivers/base/regmap/regmap-sdw.c | 2 + include/linux/regmap.h | 21 +++++ include/linux/soundwire/sdw_registers.h | 33 ++++++++ 6 files changed, 163 insertions(+), 1 deletion(-) create mode 100644 drivers/base/regmap/regmap-sdw-mbq.c
base-commit: d17343b87da422a59d99a3ed130573dbeb96c582
2.25.1
On 9/3/20 5:36 AM, Vinod Koul wrote:
On 01-09-20, 11:22, Pierre-Louis Bossart wrote:
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 second patch was initially suggested for inclusion in the SoundWire tree, and was modified to add more background information on SDCA in the commit message as requested by Vinod Koul.
Is this targetted towards the sdw tree or regmap tree, I think it may have dependencies so sdw can be used.
It would be nice to mention these things here in cover
It was intended for the regmap tree (the cover letter does not include the soundwire: prefix).
Patch2 would need the ack of SoundWire maintainers (Bard already provide his tag).
The dependencies are regmap->ASoC for codec integration. There will be no dependencies for the SoundWire core proper, SDCA is really about the device side of things beyond what the bus provides.
Thanks!
Changes since v1: Rebased on regmap tree (conflict with SPI stuff). Removed mod_devicetable.h header Removed -EOPNOTSUPP error codes, use -ENOTSUPP Added long description of SDCA Used FIELD_PREP/GET as suggested by Vinod Koul Added Bard Liao's Acked-by tag.
Pierre-Louis Bossart (3): 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 | 101 ++++++++++++++++++++++++ drivers/base/regmap/regmap-sdw.c | 2 + include/linux/regmap.h | 21 +++++ include/linux/soundwire/sdw_registers.h | 33 ++++++++ 6 files changed, 163 insertions(+), 1 deletion(-) create mode 100644 drivers/base/regmap/regmap-sdw-mbq.c
base-commit: d17343b87da422a59d99a3ed130573dbeb96c582
2.25.1
participants (2)
-
Pierre-Louis Bossart -
Vinod Koul