Re: [PATCH v3 3/7] ASoC: codecs: wcd938x: add basic driver
On 19/03/2021 15:59, Pierre-Louis Bossart wrote:
On 3/19/21 4:29 AM, Srinivas Kandagatla wrote:
This patch adds basic SoundWire codec driver to support for
WCD938X TX and RX devices.
It took me a while to figure out that you are adding support for a codec
that has 2 Slave interfaces internally, one for TX and one for RX dais.
Each of the interfaces will appear as a separate Linux device, even
though they are physically in the same hardware component.
That perfectly legit from a SoundWire standpoint, but I wonder how you
are going to handle pm_runtime and regmap access if the two parts are
joined at the hip for imp-def register access (described in the cover
letter as "Even though this device has two SoundWire devices, only tx
device has access to main codec Control/Status Registers!").
I was clearly unable to figure out how regmap/gpios/regulator were
handled between the two TX and TX parts.
tx regmap is shared between both the devices.
Regulators are also handled in common code for tx and rx device.
I added more detailed reply in cover letter reply.
See more below.
diff --git a/sound/soc/codecs/wcd938x-sdw.c
b/sound/soc/codecs/wcd938x-sdw.c
new file mode 100644
index ..ca29793b0972
--- /dev/null
+++ b/sound/soc/codecs/wcd938x-sdw.c
@@ -0,0 +1,291 @@
+// SPDX-License-Identifier: GPL-2.0
GPL-2.0-only for consistency with the other additions below?
Sure will fix that!
+static int wcd9380_probe(struct sdw_slave *pdev,
+ const struct sdw_device_id *id)
+{
+ struct device *dev = >dev;
+ struct wcd938x_sdw_priv *wcd;
+ const char *dir = "rx";
+ int ret;
+
+ wcd = devm_kzalloc(dev, sizeof(*wcd), GFP_KERNEL);
+ if (!wcd)
+ return -ENOMEM;
+
+ of_property_read_string(dev->of_node, "direction", );
+ if (!strcmp(dir, "tx"))
+ wcd->is_tx = true;
+ else
+ wcd->is_tx = false;
+
+
extra line
+ ret = of_property_read_variable_u32_array(dev->of_node,
"qcom,port-mapping",
+ wcd->port_map,
+ WCD938X_MAX_TX_SWR_PORTS,
+ WCD938X_MAX_SWR_PORTS);
+ if (ret)
+ dev_info(dev, "Static Port mapping not specified\n");
+
+ wcd->sdev = pdev;
+ dev_set_drvdata(dev, wcd);
+ ret = wcd938x_init(wcd);
+ if (ret)
+ return ret;
+
+ pdev->prop.scp_int1_mask = SDW_SCP_INT1_IMPL_DEF |
+ SDW_SCP_INT1_BUS_CLASH |
+ SDW_SCP_INT1_PARITY;
+ pdev->prop.lane_control_support = true;
+ if (wcd->is_tx) {
+ pdev->prop.source_ports = GENMASK(WCD938X_MAX_SWR_PORTS, 0);
+ pdev->prop.src_dpn_prop = wcd938x_dpn_prop;
+ wcd->ch_info = _sdw_tx_ch_info[0];
+ pdev->prop.wake_capable = true;
+ } else {
+ pdev->prop.sink_ports = GENMASK(WCD938X_MAX_SWR_PORTS, 0);
+ pdev->prop.sink_dpn_prop = wcd938x_dpn_prop;
+ wcd->ch_info = _sdw_rx_ch_info[0];
+ }
+
+ if (wcd->is_tx)
+ return wcd938x_register_component(wcd, dev, _tx_dai);
+ else
+ return wcd938x_register_component(wcd, dev, _rx_dai);
+
+}
+
+static const struct sdw_device_id wcd9380_slave_id[] = {
+ SDW_SLAVE_ENTRY(0x0217, 0x10d, 0),
does this mean you cannot determine the functionality of the device by
looking at the devId registers?
No, we can not, they have same device id.
I would have expected each interface to have a different part ID to show
that they are different...such tricks would not work in the ACPI world
at the moment, the expectation was really that different part numbers
are unique enough to know what to expect.
+ {},
+};
+MODULE_DEVICE_TABLE(sdw, wcd9380_slave_id);
+
+static struct sdw_driver wcd9380_codec_driver = {
+ .probe = wcd9380_probe,
+ .ops = _slave_ops,
+ .id_table = wcd9380_slave_id,
+ .driver = {
+ .name = "wcd9380-codec",
+ }
+};
+module_sdw_driver(wcd9380_codec_driver);
[...]
+static bool wcd938x_readable_register(struct device *dev, unsigned
int reg)
+{
+ bool ret;
+
+ ret = wcd938x_readonly_register(dev, reg);
+ if (!ret)
+ return wcd938x_rdwr_register(dev, reg);
+
+ return ret;
+}
+
+static bool wcd938x_writeable_register(struct device *dev, unsigned
int reg)
+{
+ return wcd938x_rdwr_register(dev, reg);
+}
+
+static bool wcd938x_volatile_register(struct device *dev, unsigned
int reg)
+{
+ if (reg <= WCD938X_BASE_ADDRESS)
+ return 0;
+
+ if (reg == WCD938X_DIGITAL_SWR_TX_CLK_RATE)
+ return true;
+
+ if (wcd938x_readonly_register(dev, reg))
+ return true;
+
+ return false;
+}
this part is quite confusing, you mentioned that only the TX interface
has access to registers, but here you seem to expose regmap registers
for both TX and RX?
No, this regmap is only for tx.
Am happy to prefix them with tx if it helps.
+
+static struct regmap_config wcd938x_regmap_config = {
+ .name = "wcd938x_csr",
+ .reg_bits = 32,
+
Re: [PATCH v3 3/7] ASoC: codecs: wcd938x: add basic driver
On 3/19/21 4:29 AM, Srinivas Kandagatla wrote:
This patch adds basic SoundWire codec driver to support for
WCD938X TX and RX devices.
It took me a while to figure out that you are adding support for a codec
that has 2 Slave interfaces internally, one for TX and one for RX dais.
Each of the interfaces will appear as a separate Linux device, even
though they are physically in the same hardware component.
That perfectly legit from a SoundWire standpoint, but I wonder how you
are going to handle pm_runtime and regmap access if the two parts are
joined at the hip for imp-def register access (described in the cover
letter as "Even though this device has two SoundWire devices, only tx
device has access to main codec Control/Status Registers!").
I was clearly unable to figure out how regmap/gpios/regulator were
handled between the two TX and TX parts.
See more below.
diff --git a/sound/soc/codecs/wcd938x-sdw.c b/sound/soc/codecs/wcd938x-sdw.c
new file mode 100644
index ..ca29793b0972
--- /dev/null
+++ b/sound/soc/codecs/wcd938x-sdw.c
@@ -0,0 +1,291 @@
+// SPDX-License-Identifier: GPL-2.0
GPL-2.0-only for consistency with the other additions below?
+static int wcd9380_probe(struct sdw_slave *pdev,
+const struct sdw_device_id *id)
+{
+ struct device *dev = >dev;
+ struct wcd938x_sdw_priv *wcd;
+ const char *dir = "rx";
+ int ret;
+
+ wcd = devm_kzalloc(dev, sizeof(*wcd), GFP_KERNEL);
+ if (!wcd)
+ return -ENOMEM;
+
+ of_property_read_string(dev->of_node, "direction", );
+ if (!strcmp(dir, "tx"))
+ wcd->is_tx = true;
+ else
+ wcd->is_tx = false;
+
+
extra line
+ ret = of_property_read_variable_u32_array(dev->of_node,
"qcom,port-mapping",
+ wcd->port_map,
+ WCD938X_MAX_TX_SWR_PORTS,
+ WCD938X_MAX_SWR_PORTS);
+ if (ret)
+ dev_info(dev, "Static Port mapping not specified\n");
+
+ wcd->sdev = pdev;
+ dev_set_drvdata(dev, wcd);
+ ret = wcd938x_init(wcd);
+ if (ret)
+ return ret;
+
+ pdev->prop.scp_int1_mask = SDW_SCP_INT1_IMPL_DEF |
+ SDW_SCP_INT1_BUS_CLASH |
+ SDW_SCP_INT1_PARITY;
+ pdev->prop.lane_control_support = true;
+ if (wcd->is_tx) {
+ pdev->prop.source_ports = GENMASK(WCD938X_MAX_SWR_PORTS, 0);
+ pdev->prop.src_dpn_prop = wcd938x_dpn_prop;
+ wcd->ch_info = _sdw_tx_ch_info[0];
+ pdev->prop.wake_capable = true;
+ } else {
+ pdev->prop.sink_ports = GENMASK(WCD938X_MAX_SWR_PORTS, 0);
+ pdev->prop.sink_dpn_prop = wcd938x_dpn_prop;
+ wcd->ch_info = _sdw_rx_ch_info[0];
+ }
+
+ if (wcd->is_tx)
+ return wcd938x_register_component(wcd, dev, _tx_dai);
+ else
+ return wcd938x_register_component(wcd, dev, _rx_dai);
+
+}
+
+static const struct sdw_device_id wcd9380_slave_id[] = {
+ SDW_SLAVE_ENTRY(0x0217, 0x10d, 0),
does this mean you cannot determine the functionality of the device by
looking at the devId registers?
I would have expected each interface to have a different part ID to show
that they are different...such tricks would not work in the ACPI world
at the moment, the expectation was really that different part numbers
are unique enough to know what to expect.
+ {},
+};
+MODULE_DEVICE_TABLE(sdw, wcd9380_slave_id);
+
+static struct sdw_driver wcd9380_codec_driver = {
+ .probe = wcd9380_probe,
+ .ops = _slave_ops,
+ .id_table = wcd9380_slave_id,
+ .driver = {
+ .name = "wcd9380-codec",
+ }
+};
+module_sdw_driver(wcd9380_codec_driver);
[...]
+static bool wcd938x_readable_register(struct device *dev, unsigned int reg)
+{
+ bool ret;
+
+ ret = wcd938x_readonly_register(dev, reg);
+ if (!ret)
+ return wcd938x_rdwr_register(dev, reg);
+
+ return ret;
+}
+
+static bool wcd938x_writeable_register(struct device *dev, unsigned int reg)
+{
+ return wcd938x_rdwr_register(dev, reg);
+}
+
+static bool wcd938x_volatile_register(struct device *dev, unsigned int reg)
+{
+ if (reg <= WCD938X_BASE_ADDRESS)
+ return 0;
+
+ if (reg == WCD938X_DIGITAL_SWR_TX_CLK_RATE)
+ return true;
+
+ if (wcd938x_readonly_register(dev, reg))
+ return true;
+
+ return false;
+}
this part is quite confusing, you mentioned that only the TX interface
has access to registers, but here you seem to expose regmap registers
for both TX and RX?
+
+static struct regmap_config wcd938x_regmap_config = {
+ .name = "wcd938x_csr",
+ .reg_bits = 32,
+
[PATCH v3 3/7] ASoC: codecs: wcd938x: add basic driver
This patch adds basic SoundWire codec driver to support for
WCD938X TX and RX devices.
Signed-off-by: Srinivas Kandagatla
---
sound/soc/codecs/Kconfig |9 +
sound/soc/codecs/Makefile |2 +
sound/soc/codecs/wcd938x-sdw.c | 291 ++
sound/soc/codecs/wcd938x.c | 1615
sound/soc/codecs/wcd938x.h | 676 +
5 files changed, 2593 insertions(+)
create mode 100644 sound/soc/codecs/wcd938x-sdw.c
create mode 100644 sound/soc/codecs/wcd938x.c
create mode 100644 sound/soc/codecs/wcd938x.h
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 4ab34bca71aa..17fe1f690d22 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -231,6 +231,7 @@ config SND_SOC_ALL_CODECS
imply SND_SOC_UDA1380
imply SND_SOC_WCD9335
imply SND_SOC_WCD934X
+ imply SND_SOC_WCD938X
imply SND_SOC_LPASS_RX_MACRO
imply SND_SOC_LPASS_TX_MACRO
imply SND_SOC_WL1273
@@ -1521,6 +1522,14 @@ config SND_SOC_WCD934X
The WCD9340/9341 is a audio codec IC Integrated in
Qualcomm SoCs like SDM845.
+config SND_SOC_WCD938X
+ tristate "WCD9380/WCD9385 Codec"
+ depends on SOUNDWIRE
+ select REGMAP_SOUNDWIRE
+ help
+ The WCD9380/9385 is a audio codec IC Integrated in
+ Qualcomm SoCs like SM8250.
+
config SND_SOC_WL1273
tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index edff5c5b92d3..c7dbe84d939e 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -249,6 +249,7 @@ snd-soc-uda134x-objs := uda134x.o
snd-soc-uda1380-objs := uda1380.o
snd-soc-wcd9335-objs := wcd-clsh-v2.o wcd9335.o
snd-soc-wcd934x-objs := wcd-clsh-v2.o wcd934x.o
+snd-soc-wcd938x-objs := wcd938x.o wcd938x-sdw.o wcd-clsh-v2.o wcd938x-sdw.o
snd-soc-wl1273-objs := wl1273.o
snd-soc-wm-adsp-objs := wm_adsp.o
snd-soc-wm0010-objs := wm0010.o
@@ -568,6 +569,7 @@ obj-$(CONFIG_SND_SOC_UDA134X) += snd-soc-uda134x.o
obj-$(CONFIG_SND_SOC_UDA1380) += snd-soc-uda1380.o
obj-$(CONFIG_SND_SOC_WCD9335) += snd-soc-wcd9335.o
obj-$(CONFIG_SND_SOC_WCD934X) += snd-soc-wcd934x.o
+obj-$(CONFIG_SND_SOC_WCD938X) += snd-soc-wcd938x.o
obj-$(CONFIG_SND_SOC_WL1273) += snd-soc-wl1273.o
obj-$(CONFIG_SND_SOC_WM0010) += snd-soc-wm0010.o
obj-$(CONFIG_SND_SOC_WM1250_EV1) += snd-soc-wm1250-ev1.o
diff --git a/sound/soc/codecs/wcd938x-sdw.c b/sound/soc/codecs/wcd938x-sdw.c
new file mode 100644
index ..ca29793b0972
--- /dev/null
+++ b/sound/soc/codecs/wcd938x-sdw.c
@@ -0,0 +1,291 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2021, Linaro Limited
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "wcd938x.h"
+
+#define WCD938X_RATES_MASK (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |\
+ SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_48000 |\
+ SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_192000)
+/* Fractional Rates */
+#define WCD938X_FRAC_RATES_MASK (SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_88200 |\
+SNDRV_PCM_RATE_176400)
+#define WCD938X_FORMATS_S16_S24_LE (SNDRV_PCM_FMTBIT_S16_LE | \
+ SNDRV_PCM_FMTBIT_S24_LE)
+#define SWRS_SCP_HOST_CLK_DIV2_CTL_BANK(m) (0xE0 + 0x10 * (m))
+
+static struct wcd938x_sdw_ch_info wcd938x_sdw_rx_ch_info[] = {
+ WCD_SDW_CH(WCD938X_HPH_L, WCD938X_HPH_PORT, BIT(0)),
+ WCD_SDW_CH(WCD938X_HPH_R, WCD938X_HPH_PORT, BIT(1)),
+ WCD_SDW_CH(WCD938X_CLSH, WCD938X_CLSH_PORT, BIT(0)),
+ WCD_SDW_CH(WCD938X_COMP_L, WCD938X_COMP_PORT, BIT(0)),
+ WCD_SDW_CH(WCD938X_COMP_R, WCD938X_COMP_PORT, BIT(1)),
+ WCD_SDW_CH(WCD938X_LO, WCD938X_LO_PORT, BIT(0)),
+ WCD_SDW_CH(WCD938X_DSD_L, WCD938X_DSD_PORT, BIT(0)),
+ WCD_SDW_CH(WCD938X_DSD_R, WCD938X_DSD_PORT, BIT(1)),
+};
+
+static struct wcd938x_sdw_ch_info wcd938x_sdw_tx_ch_info[] = {
+ WCD_SDW_CH(WCD938X_ADC1, WCD938X_ADC_1_2_PORT, BIT(0)),
+ WCD_SDW_CH(WCD938X_ADC2, WCD938X_ADC_1_2_PORT, BIT(1)),
+ WCD_SDW_CH(WCD938X_ADC3, WCD938X_ADC_3_4_PORT, BIT(0)),
+ WCD_SDW_CH(WCD938X_ADC4, WCD938X_ADC_3_4_PORT, BIT(1)),
+ WCD_SDW_CH(WCD938X_DMIC0, WCD938X_DMIC_0_3_MBHC_PORT, BIT(0)),
+ WCD_SDW_CH(WCD938X_DMIC1, WCD938X_DMIC_0_3_MBHC_PORT, BIT(1)),
+ WCD_SDW_CH(WCD938X_MBHC, WCD938X_DMIC_0_3_MBHC_PORT, BIT(2)),
+ WCD_SDW_CH(WCD938X_DMIC2, WCD938X_DMIC_0_3_MBHC_PORT, BIT(2)),
+ WCD_SDW_CH(WCD938X_DMIC3, WCD938X_DMIC_0_3_MBHC_PORT, BIT(3)),
+ WCD_SDW_CH(WCD938X_DMIC4, WCD938X_DMIC_4_7_PORT, BIT(0)),
+ WCD_SDW_CH(WCD938X_DMIC5, WCD938X_DMIC_4_7_PORT, BIT(1)),
+ WCD_SDW_CH(WCD938X_DMIC6, WCD938X_DMIC_4_7_PORT, BIT(2)),
+ WCD_SDW_CH(WCD938X_DMIC7, WCD938X_DMIC_4_7_PORT, BIT(3)),
+};
+
