[PATCH 0/4] regmap: add SoundWire 1.2 MBQ support

[PATCH 0/4] regmap: add SoundWire 1.2 MBQ support

[Pierre-Louis Bossart]

In preparation of the upstream contribution of SDCA (SoundWire Device
Class for Audio) ASoC codec drivers [1] [2], add regmap support
SoundWire 1.2 MBQ support. The MBQ (Multi-Byte Quantity) registers
need to be handled in a different way from regular 8-bit SoundWire
registers, their main application is going to be for volume/gain
controls.

The 3rd patch was initially suggested for inclusion in the SoundWire
tree, and was modified to add more background information in the
commit message as requested by Vinod Koul.

Pierre-Louis Bossart (4):
  regmap: sdw: move to -EOPNOTSUPP
  regmap: sdw: add required header files
  soundwire: SDCA: add helper macro to access controls
  regmap: sdw: add support for SoundWire 1.2 MBQ

 drivers/base/regmap/Kconfig             |   6 +-
 drivers/base/regmap/Makefile            |   1 +
 drivers/base/regmap/regmap-sdw-mbq.c    | 102 ++++++++++++++++++++++++
 drivers/base/regmap/regmap-sdw.c        |   8 +-
 include/linux/regmap.h                  |  20 +++++
 include/linux/soundwire/sdw_registers.h |  13 +++
 6 files changed, 146 insertions(+), 4 deletions(-)
 create mode 100644 drivers/base/regmap/regmap-sdw-mbq.c


base-commit: 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5
-- 
2.25.1

[PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP

[Pierre-Louis Bossart]

-ENOTSUPP is not a valid error code, use recommended value instead.

Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/base/regmap/regmap-sdw.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/regmap/regmap-sdw.c b/drivers/base/regmap/regmap-sdw.c
index 50a66382d87d..89d3856f5890 100644
--- a/drivers/base/regmap/regmap-sdw.c
+++ b/drivers/base/regmap/regmap-sdw.c
@@ -40,14 +40,14 @@ static int regmap_sdw_config_check(const struct regmap_config *config)
 {
 	/* All register are 8-bits wide as per MIPI Soundwire 1.0 Spec */
 	if (config->val_bits != 8)
-		return -ENOTSUPP;
+		return -EOPNOTSUPP;
 
 	/* Registers are 32 bits wide */
 	if (config->reg_bits != 32)
-		return -ENOTSUPP;
+		return -EOPNOTSUPP;
 
 	if (config->pad_bits != 0)
-		return -ENOTSUPP;
+		return -EOPNOTSUPP;
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 228 bytes --]

On Tue, Aug 25, 2020 at 12:16:53PM -0500, Pierre-Louis Bossart wrote:
> -ENOTSUPP is not a valid error code, use recommended value instead.

What makes you say this - it's what regmap uses internally for
unsupported operations?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP

[Pierre-Louis Bossart]

>> -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.

Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

On Tue, Aug 25, 2020 at 05:08:55PM -0500, Pierre-Louis Bossart wrote:

> > > -ENOTSUPP is not a valid error code, use recommended value instead.

> > What makes you say this - it's what regmap uses internally for
> > unsupported operations?

> This was flagged by scripts/checkpatch.pl (must be a new addition).

checkpatch is broken.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP

[Takashi Iwai]

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

Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]

On Wed, Aug 26, 2020 at 12:09:28PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > checkpatch is broken.

> Heh, I'm not objecting it :)

> OTOH, it's also true that ENOTSUPP is no good error code if returned
> to user-space.  Unfortunately many codes (including what I wrote) use
> this code mistakenly, and they can't be changed any longer...

It's also used internally in various places without being returned to
userspace, that's what's going on here - the regmap core has some
specific checks for -ENOTSUPP.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP

[Takashi Iwai]

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

Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP

[Vinod Koul]

On 26-08-20, 12:22, Takashi Iwai wrote:
> On Wed, 26 Aug 2020 12:13:01 +0200,
> Mark Brown wrote:
> > 
> > On Wed, Aug 26, 2020 at 12:09:28PM +0200, Takashi Iwai wrote:
> > > Mark Brown wrote:
> > 
> > > > checkpatch is broken.
> > 
> > > Heh, I'm not objecting it :)
> > 
> > > OTOH, it's also true that ENOTSUPP is no good error code if returned
> > > to user-space.  Unfortunately many codes (including what I wrote) use
> > > this code mistakenly, and they can't be changed any longer...
> > 
> > It's also used internally in various places without being returned to
> > userspace, that's what's going on here - the regmap core has some
> > specific checks for -ENOTSUPP.
> 
> Sure, for such an internal usage any code can be used.
> The question is a case like this -- where the return code might be
> carried to outside.  Though, looking through the grep output, all
> callers simply return -EINVAL for any errors, so it doesn't matter
> much for now.
> 
> 
> BTW, there are a few callers of devm_regmap_init_sdw() checking the
> return value with NULL.  This will crash as the function returns the
> error pointer, and they must be checked with IS_ERR() instead.

Yes that is correct, all expect wsa codec do the incorrect thing. Will
send patches for these shortly

Thanks
-- 
~Vinod

Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP

[Pierre-Louis Bossart]

>>>> 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?

Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 669 bytes --]

On Wed, Aug 26, 2020 at 10:05:50AM -0500, Pierre-Louis Bossart wrote:

> I assumed this change to -EOPNOTSUPP reflected a consensus in kernel-land,
> it's obviously not the case. This patch was supposed to be a trivial
> clean-up...

No, it's just some random guy sent a patch.  They've not made any
perceptible effort to interact with any of the existing users.

> So to be clear, what is the direction for existing code
> a) keep -ENOTSUPP as is?
> b) move to -EOPNOTSUPP?

> And what is the preference for new code?

If you want to change this you'd need to change it over the whole
subsystem (if not other subsystems), including the places where the
value is used.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP

[Pierre-Louis Bossart]

> 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.

[PATCH 2/4] regmap: sdw: add required header files

[Pierre-Louis Bossart]

Explicitly add header files used by regmap SoundWire support.

Suggested-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/base/regmap/regmap-sdw.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/regmap/regmap-sdw.c b/drivers/base/regmap/regmap-sdw.c
index 89d3856f5890..29edbb6da48f 100644
--- a/drivers/base/regmap/regmap-sdw.c
+++ b/drivers/base/regmap/regmap-sdw.c
@@ -2,8 +2,10 @@
 // Copyright(c) 2015-17 Intel Corporation.
 
 #include <linux/device.h>
+#include <linux/errno.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/soundwire/sdw.h>
 #include "internal.h"
 
-- 
2.25.1

[PATCH 3/4] soundwire: SDCA: add helper macro to access controls

[Pierre-Louis Bossart]

The upcoming SDCA (SoundWire Device Class Audio) specification defines
a hierarchical encoding to interface with Class-defined capabilities.

The specification is not yet accessible to the general public but this
information is released with explicit permission from the MIPI Board
to avoid delays with SDCA support on Linux platforms.

A block of 64 MBytes of register addresses are allocated to SDCA
controls, starting at address 0x40000000. The 26 LSBs which identify
individual controls are set based on the following variables:

- Function Number. An SCDA device can be split in up to 8 independent
  Functions. Each of these Functions is described in the SDCA
  specification, e.g. Smart Amplifier, Smart Microphone, Simple
  Microphone, Jack codec, HID, etc.

- Entity Number.  Within each Function,  an Entity is  an identifiable
  block.  Up   to  127  Entities   are  connected  in   a  pre-defined
  graph  (similar to  USB), with  Entity0 reserved  for Function-level
  configurations.  In  contrast  to  USB, the  SDCA  spec  pre-defines
  Function Types, topologies, and allowed  options, i.e. the degree of
  freedom  is not  unlimited to  limit  the possibility  of errors  in
  descriptors leading to software quirks.

- Control Selector. Within each Entity, the SDCA specification defines
  48 controls such as Mute, Gain, AGC, etc, and 16 implementation
  defined ones. Some Control Selectors might be used for low-level
  platform setup, and other exposed to applications and users. Note
  that the same Control Selector capability, e.g. Latency control,
  might be located at different offsets in different entities, the
  Control Selector mapping is Entity-specific.

- Control Number. Some Control Selectors allow channel-specific values
  to be set, with up to 64 channels allowed. This is mostly used for
  volume control.

- Current/Next values. Some Control Selectors are
  'Dual-Ranked'. Software may either update the Current value directly
  for immediate effect. Alternatively, software may write into the
  'Next' values and update the SoundWire 1.2 'Commit Groups' register
  to copy 'Next' values into 'Current' ones in a synchronized
  manner. This is different from bank switching which is typically
  used to change the bus configuration only.

- MBQ. the Multi-Byte Quantity bit is used to provide atomic updates
  when accessing more that one byte, for example a 16-bit volume
  control would be updated consistently, the intermediate values
  mixing old MSB with new LSB are not applied.

These 6 parameters are used to build a 32-bit address to access the
desired Controls. Because of address range, paging is required, but
the most often used parameter values are placed in the lower 16 bits
of the address. This helps to keep the paging registers constant while
updating Controls for a specific Device/Function.

Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/linux/soundwire/sdw_registers.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/soundwire/sdw_registers.h b/include/linux/soundwire/sdw_registers.h
index 5d3c271af7d1..906dadda7387 100644
--- a/include/linux/soundwire/sdw_registers.h
+++ b/include/linux/soundwire/sdw_registers.h
@@ -305,4 +305,17 @@
 #define SDW_CASC_PORT_MASK_INTSTAT3		1
 #define SDW_CASC_PORT_REG_OFFSET_INTSTAT3	2
 
+/* v1.2 device - SDCA address mapping */
+#define SDW_SDCA_CTL(fun, ent, ctl, ch)		(BIT(30) |			\
+						 (((fun) & 0x7) << 22) |	\
+						 (((ent) & 0x40) << 15) |	\
+						 (((ent) & 0x3f) << 7) |	\
+						 (((ctl) & 0x30) << 15) |	\
+						 (((ctl) & 0x0f) << 3) |	\
+						 (((ch) & 0x38) << 12) |	\
+						 ((ch) & 0x07))
+
+#define SDW_SDCA_MBQ_CTL(reg)			((reg) | BIT(13))
+#define SDW_SDCA_NEXT_CTL(reg)			((reg) | BIT(14))
+
 #endif /* __SDW_REGISTERS_H */
-- 
2.25.1

Re: [PATCH 3/4] soundwire: SDCA: add helper macro to access controls

[Bard liao]

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>

Re: [PATCH 3/4] soundwire: SDCA: add helper macro to access controls

[Vinod Koul]

On 25-08-20, 12:16, Pierre-Louis Bossart wrote:
> The upcoming SDCA (SoundWire Device Class Audio) specification defines
> a hierarchical encoding to interface with Class-defined capabilities.
> 
> The specification is not yet accessible to the general public but this
> information is released with explicit permission from the MIPI Board
> to avoid delays with SDCA support on Linux platforms.
> 
> A block of 64 MBytes of register addresses are allocated to SDCA
> controls, starting at address 0x40000000. The 26 LSBs which identify
> individual controls are set based on the following variables:
> 
> - Function Number. An SCDA device can be split in up to 8 independent
>   Functions. Each of these Functions is described in the SDCA
>   specification, e.g. Smart Amplifier, Smart Microphone, Simple
>   Microphone, Jack codec, HID, etc.
> 
> - Entity Number.  Within each Function,  an Entity is  an identifiable
>   block.  Up   to  127  Entities   are  connected  in   a  pre-defined
>   graph  (similar to  USB), with  Entity0 reserved  for Function-level
>   configurations.  In  contrast  to  USB, the  SDCA  spec  pre-defines
>   Function Types, topologies, and allowed  options, i.e. the degree of
>   freedom  is not  unlimited to  limit  the possibility  of errors  in
>   descriptors leading to software quirks.
> 
> - Control Selector. Within each Entity, the SDCA specification defines
>   48 controls such as Mute, Gain, AGC, etc, and 16 implementation
>   defined ones. Some Control Selectors might be used for low-level
>   platform setup, and other exposed to applications and users. Note
>   that the same Control Selector capability, e.g. Latency control,
>   might be located at different offsets in different entities, the
>   Control Selector mapping is Entity-specific.
> 
> - Control Number. Some Control Selectors allow channel-specific values
>   to be set, with up to 64 channels allowed. This is mostly used for
>   volume control.
> 
> - Current/Next values. Some Control Selectors are
>   'Dual-Ranked'. Software may either update the Current value directly
>   for immediate effect. Alternatively, software may write into the
>   'Next' values and update the SoundWire 1.2 'Commit Groups' register
>   to copy 'Next' values into 'Current' ones in a synchronized
>   manner. This is different from bank switching which is typically
>   used to change the bus configuration only.
> 
> - MBQ. the Multi-Byte Quantity bit is used to provide atomic updates
>   when accessing more that one byte, for example a 16-bit volume
>   control would be updated consistently, the intermediate values
>   mixing old MSB with new LSB are not applied.
> 
> These 6 parameters are used to build a 32-bit address to access the
> desired Controls. Because of address range, paging is required, but
> the most often used parameter values are placed in the lower 16 bits
> of the address. This helps to keep the paging registers constant while
> updating Controls for a specific Device/Function.
> 
> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  include/linux/soundwire/sdw_registers.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/linux/soundwire/sdw_registers.h b/include/linux/soundwire/sdw_registers.h
> index 5d3c271af7d1..906dadda7387 100644
> --- a/include/linux/soundwire/sdw_registers.h
> +++ b/include/linux/soundwire/sdw_registers.h
> @@ -305,4 +305,17 @@
>  #define SDW_CASC_PORT_MASK_INTSTAT3		1
>  #define SDW_CASC_PORT_REG_OFFSET_INTSTAT3	2
>  
> +/* v1.2 device - SDCA address mapping */

Can you please add description of bits used by each field here,
something like we have done for DevId

> +#define SDW_SDCA_CTL(fun, ent, ctl, ch)		(BIT(30) |			\
> +						 (((fun) & 0x7) << 22) |	\
> +						 (((ent) & 0x40) << 15) |	\
> +						 (((ent) & 0x3f) << 7) |	\
> +						 (((ctl) & 0x30) << 15) |	\
> +						 (((ctl) & 0x0f) << 3) |	\
> +						 (((ch) & 0x38) << 12) |	\
> +						 ((ch) & 0x07))

GENMASK() for the bitmaps here please. Also it would look very neat by
using FIELD_PREP() here, you can skip the bit shifts and they would be
done by FIELD_PREP() for you.

> -- 
> 2.25.1

-- 
~Vinod

Re: [PATCH 3/4] soundwire: SDCA: add helper macro to access controls

[Pierre-Louis Bossart]

>> +/* 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.

Re: [PATCH 3/4] soundwire: SDCA: add helper macro to access controls

[Vinod Koul]

On 26-08-20, 10:00, Pierre-Louis Bossart wrote:
> 
> 
> > > +/* v1.2 device - SDCA address mapping */
> > 
> > Can you please add description of bits used by each field here,
> > something like we have done for DevId
> 
> were you referring to something like this?
> 
>  * Spec definition
>  *   Register		Bit	Contents
>  *   DevId_0 [7:4]	47:44	sdw_version
>  *   DevId_0 [3:0]	43:40	unique_id
>  *   DevId_1		39:32	mfg_id [15:8]
>  *   DevId_2		31:24	mfg_id [7:0]
>  *   DevId_3		23:16	part_id [15:8]
>  *   DevId_4		15:08	part_id [7:0]
>  *   DevId_5		07:00	class_id

Correct

> > 
> > > +#define SDW_SDCA_CTL(fun, ent, ctl, ch)		(BIT(30) |			\
> > > +						 (((fun) & 0x7) << 22) |	\
> > > +						 (((ent) & 0x40) << 15) |	\
> > > +						 (((ent) & 0x3f) << 7) |	\
> > > +						 (((ctl) & 0x30) << 15) |	\
> > > +						 (((ctl) & 0x0f) << 3) |	\
> > > +						 (((ch) & 0x38) << 12) |	\
> > > +						 ((ch) & 0x07))
> > 
> > GENMASK() for the bitmaps here please. Also it would look very neat by
> > using FIELD_PREP() here, you can skip the bit shifts and they would be
> > done by FIELD_PREP() for you.
> 
> ok.

FWIW I am testing changes to do the conversion for subsystem to use nice
stuff in bitfield.h


-- 
~Vinod

[PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ

[Pierre-Louis Bossart]

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, &regmap_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, &regmap_sdw_mbq,
+			&sdw->dev, config, lock_key, lock_name);
+}
+EXPORT_SYMBOL_GPL(__devm_regmap_init_sdw_mbq);
+
+MODULE_DESCRIPTION("Regmap SoundWire Module");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 1970ed59d49f..506e2a97d381 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -567,6 +567,10 @@ struct regmap *__regmap_init_sdw(struct sdw_slave *sdw,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
 				 const char *lock_name);
+struct regmap *__regmap_init_sdw_mbq(struct sdw_slave *sdw,
+				     const struct regmap_config *config,
+				     struct lock_class_key *lock_key,
+				     const char *lock_name);
 
 struct regmap *__devm_regmap_init(struct device *dev,
 				  const struct regmap_bus *bus,
@@ -612,6 +616,10 @@ struct regmap *__devm_regmap_init_sdw(struct sdw_slave *sdw,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
 				 const char *lock_name);
+struct regmap *__devm_regmap_init_sdw_mbq(struct sdw_slave *sdw,
+					  const struct regmap_config *config,
+					  struct lock_class_key *lock_key,
+					  const char *lock_name);
 struct regmap *__devm_regmap_init_slimbus(struct slim_device *slimbus,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
@@ -806,6 +814,18 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 	__regmap_lockdep_wrapper(__regmap_init_sdw, #config,		\
 				sdw, config)
 
+/**
+ * regmap_init_sdw_mbq() - Initialise register map
+ *
+ * @sdw: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer to
+ * a struct regmap.
+ */
+#define regmap_init_sdw_mbq(sdw, config)					\
+	__regmap_lockdep_wrapper(__regmap_init_sdw_mbq, #config,		\
+				sdw, config)
 
 /**
  * devm_regmap_init() - Initialise managed register map
-- 
2.25.1

Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ

[Bard liao]

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>

Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ

[Vinod Koul]

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, &regmap_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, &regmap_sdw_mbq,
> +			&sdw->dev, config, lock_key, lock_name);
> +}
> +EXPORT_SYMBOL_GPL(__devm_regmap_init_sdw_mbq);
> +
> +MODULE_DESCRIPTION("Regmap SoundWire Module");

This is same of sdw module, pls make this one a bit different.
-- 
~Vinod

Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ

[Pierre-Louis Bossart]

>> +#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.

Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ

[Vinod Koul]

On 26-08-20, 11:57, Pierre-Louis Bossart wrote:
> 
> > > +#include <linux/device.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/mod_devicetable.h>
> > 
> > Curious why do you need this header?
> 
> I'll return the question back to you, since you added this header for
> regmap-sdw.c:
> 
> 7c22ce6e21840 (Vinod Koul           2018-01-08 15:50:59 +0530  6) #include
> <linux/mod_devicetable.h>

Looks like it should be removed :)

I could compile it without any issues on few archs I do. let me push the
patch on my tree and see if bots are happy, will send the patch

> 
> so I assumed it was needed here as well.
> 
> > > +MODULE_DESCRIPTION("Regmap SoundWire Module");
> > 
> > This is same of sdw module, pls make this one a bit different.
> 
> yes, fixed.

-- 
~Vinod

Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ

[Pierre-Louis Bossart]

>>>> +#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.

Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

On Tue, Aug 25, 2020 at 12:16:56PM -0500, Pierre-Louis Bossart wrote:

> One possible objection is that this code could have been handled with
> regmap-sdw.c. However this is a new spec addition not handled by every
> SoundWire 1.1 and non-SDCA device, so there's no reason to load code
> that will never be used.

> Also in practice it's extremely unlikely that CONFIG_REGMAP would not
> be selected with CONFIG_REGMAP_MBQ selected. However there's no
> functional dependency between the two modules so they can be selected
> separately.

The other thing I'm wondering here is about compatibility - is this
something we can enumerate at runtime and if so couldn't this be done
more like how we handle the various I2C and SMBus variants so the driver
just says it wants a SoundWire regmap and then based on the capabilities
of the device and the controller the regmap decides if it can use MBQ or
not on the current system?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ

[Pierre-Louis Bossart]

>> 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?