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