| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <DB16S3FI8AXX.1LA99XCPAW1UF@linaro.org>
Date: Wed, 02 Jul 2025 02:43:01 +0100
From: "Alexey Klimov" <alexey.klimov@...aro.org>
To: "Srinivas Kandagatla" <srinivas.kandagatla@....qualcomm.com>, "Srinivas
Kandagatla" <srini@...nel.org>, "Liam Girdwood" <lgirdwood@...il.com>,
"Mark Brown" <broonie@...nel.org>, "Rob Herring" <robh@...nel.org>,
"Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor Dooley"
<conor+dt@...nel.org>, "Stephen Boyd" <sboyd@...nel.org>
Cc: "Lee Jones" <lee@...nel.org>, "Jaroslav Kysela" <perex@...ex.cz>,
"Takashi Iwai" <tiwai@...e.com>, <linux-arm-msm@...r.kernel.org>,
<linux-sound@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, "Dmitry Baryshkov"
<dmitry.baryshkov@....qualcomm.com>
Subject: Re: [PATCH 3/3] ASoC: codecs: add new pm4125 audio codec driver
On Sat Jun 28, 2025 at 9:24 PM BST, Srinivas Kandagatla wrote:
> On 6/26/25 12:50 AM, Alexey Klimov wrote:
>> The audio codec is found in Qualcomm PM2250/PM4125 PMICs and is used on
>> platforms like Qualcomm QCM2290. It has soundwire interface and
>> corresponding RX and TX slave devices.
>>
>> It has only two input channels: HPH left and right. The line output (LO)
>> is linked to HPHL so the hardware has some limitations regarding concurrent
>> playback via HPH and LO for instance.
>>
>> Not all functionality is implemented and there are some number of
>> placeholders. Mostly output functionality like analog and digital mics is
>> missing at this point and implemented in a possible minimal way.
>> The codec driver also uses WCD MBCH framework. The MBHC functionality is
>> also implemented in a minimalistic way to enable IRQs.
>
> Lets try to fill these gaps in next version, I have tested Headset,
> Lineout and MIC on my side with additional changes to this.
Yep.
>> Despite all limitations in its current state the line out and HPH playback
>> work.
>
> Thanks Alexey for all the work porting the driver from downstream audio
> kernel.
>
> my comments below.
It is a bit difficult to follow comments so if I miss smth please let me know.
>> Cc: Srinivas Kandagatla <srinivas.kandagatla@....qualcomm.com>
>> Signed-off-by: Alexey Klimov <alexey.klimov@...aro.org>
>> ---
>> sound/soc/codecs/Kconfig | 19 +
>> sound/soc/codecs/Makefile | 8 +
>> sound/soc/codecs/pm4125-sdw.c | 485 +++++++++++
>> sound/soc/codecs/pm4125.c | 1848 +++++++++++++++++++++++++++++++++++++++++
>> sound/soc/codecs/pm4125.h | 375 +++++++++
>> 5 files changed, 2735 insertions(+)
>>
>> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
>> index 6d7e4725d89cd33647770e4f2c4e81445b8335ce..69b08021d165f83f4a7ca18e476cfc7e2473f490 100644
>> --- a/sound/soc/codecs/Kconfig
>> +++ b/sound/soc/codecs/Kconfig
>> @@ -297,6 +297,7 @@ config SND_SOC_ALL_CODECS
>> imply SND_SOC_WCD937X_SDW
>> imply SND_SOC_WCD938X_SDW
>> imply SND_SOC_WCD939X_SDW
>> + imply SND_SOC_PM4125_SDW
>> imply SND_SOC_LPASS_MACRO_COMMON
>> imply SND_SOC_LPASS_RX_MACRO
>> imply SND_SOC_LPASS_TX_MACRO
>> @@ -2316,6 +2317,24 @@ config SND_SOC_WCD939X_SDW
>> The WCD9390/9395 is a audio codec IC Integrated in
>> Qualcomm SoCs like SM8650.
>>
>> +config SND_SOC_PM4125
>> + depends on SND_SOC_PM4125_SDW
>> + tristate
>> + depends on SOUNDWIRE || !SOUNDWIRE
>> + select SND_SOC_WCD_CLASSH
>> +
>> +config SND_SOC_PM4125_SDW
>> + tristate "PM4125 audio codec - SDW"
>> + select SND_SOC_PM4125
>> + select SND_SOC_WCD_MBHC
>> + select REGMAP_IRQ
>> + depends on SOUNDWIRE
>> + select REGMAP_SOUNDWIRE
>> + help
>> + The PMIC PM4125 has an in-built audio codec IC used with SoCs
>> + like QCM2290, and it is connected via soundwire and SPMI.
>> + To compile this codec driver say Y or m.
>> +
>> config SND_SOC_WL1273
>> tristate
>>
>> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
>> index a68c3d192a1b6ccec513c6bc447c29be532ea70c..e993cc9803c4da60daf230a8181673b45c06aa5b 100644
>> --- a/sound/soc/codecs/Makefile
>> +++ b/sound/soc/codecs/Makefile
>> @@ -348,6 +348,8 @@ snd-soc-wcd938x-y := wcd938x.o
>> snd-soc-wcd938x-sdw-y := wcd938x-sdw.o
>> snd-soc-wcd939x-y := wcd939x.o
>> snd-soc-wcd939x-sdw-y := wcd939x-sdw.o
>> +snd-soc-pm4125-y := pm4125.o
>> +snd-soc-pm4125-sdw-y := pm4125-sdw.o
>> snd-soc-wl1273-y := wl1273.o
>> snd-soc-wm-adsp-y := wm_adsp.o
>> snd-soc-wm0010-y := wm0010.o
>> @@ -779,6 +781,12 @@ ifdef CONFIG_SND_SOC_WCD939X_SDW
>> # avoid link failure by forcing sdw code built-in when needed
>> obj-$(CONFIG_SND_SOC_WCD939X) += snd-soc-wcd939x-sdw.o
>> endif
>> +obj-$(CONFIG_SND_SOC_PM4125_SDW) += snd-soc-pm4125-sdw.o
>> +obj-$(CONFIG_SND_SOC_PM4125) += snd-soc-pm4125.o
>> +ifdef CONFIG_SND_SOC_PM4125_SDW
>> +# avoid link failure by forcing sdw code built-in when needed
>> +obj-$(CONFIG_SND_SOC_PM4125) += snd-soc-pm4125-sdw.o
>> +endif
>> 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/pm4125-sdw.c b/sound/soc/codecs/pm4125-sdw.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..d09c4f5fb3b7b7918a1a0c25750046b212f1063f
>> --- /dev/null
>> +++ b/sound/soc/codecs/pm4125-sdw.c
>> @@ -0,0 +1,485 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +// Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> +// Copyright, 2025 Linaro Ltd
>> +
>> +#include <linux/component.h>
>> +#include <linux/device.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/soundwire/sdw.h>
>> +#include <linux/soundwire/sdw_registers.h>
>> +#include <linux/soundwire/sdw_type.h>
>> +#include <sound/soc-dapm.h>
>> +#include <sound/soc.h>
>> +#include "pm4125.h"
>> +
>> +static struct pm4125_sdw_ch_info pm4125_sdw_rx_ch_info[] = {
>> + WCD_SDW_CH(PM4125_HPH_L, PM4125_HPH_PORT, BIT(0)),
>> + WCD_SDW_CH(PM4125_HPH_R, PM4125_HPH_PORT, BIT(1)),
>> + WCD_SDW_CH(PM4125_COMP_L, PM4125_COMP_PORT, BIT(0)),
>> + WCD_SDW_CH(PM4125_COMP_R, PM4125_COMP_PORT, BIT(1)),
>> +};
>> +
>> +static struct pm4125_sdw_ch_info pm4125_sdw_tx_ch_info[] = {
>> + WCD_SDW_CH(PM4125_ADC1, PM4125_ADC_1_PORT, BIT(0)),
>> + WCD_SDW_CH(PM4125_DMIC0, PM4125_DMIC_0_3_MBHC_PORT, BIT(0)),
>> + WCD_SDW_CH(PM4125_DMIC1, PM4125_DMIC_0_3_MBHC_PORT, BIT(1)),
>> + WCD_SDW_CH(PM4125_DMIC2, PM4125_DMIC_0_3_MBHC_PORT, BIT(2)),
> these does not look correct. there are 2 ADCs .
Ok, this needs to be fixed.
[...]
>> +static bool pm4125_rdwr_register(struct device *dev, unsigned int reg)
>> +{
>> + if (reg > PM4125_ANA_BASE_ADDR &&
>> + reg < PM4125_ANALOG_REGISTERS_MAX_SIZE)
>> + return pm4125_reg_access_analog[PM4125_REG(reg)] & WR_REG;
> Lets not bring in some additional layer here from downstream, pl use the
> registers directly here as we do for other codecs.
Ok, let's fix this.
[...]
>> +static int pm4125_probe(struct sdw_slave *pdev,
>> + const struct sdw_device_id *id)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct pm4125_sdw_priv *priv;
>> + u8 master_ch_mask[PM4125_MAX_SWR_CH_IDS];
>> + int master_ch_mask_size = 0;
>> + int ret, i;
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + /*
>> + * Port map index starts at 0,
>> + * however the data port for this codec start at index 1
>> + */
>> + if (of_property_present(dev->of_node, "qcom,tx-port-mapping")) {
>> + priv->is_tx = true;
>> + ret = of_property_read_u32_array(dev->of_node,
>> + "qcom,tx-port-mapping",
>> + &pdev->m_port_map[1],
>> + PM4125_MAX_TX_SWR_PORTS);
>> + } else {
>> + ret = of_property_read_u32_array(dev->of_node,
>> + "qcom,rx-port-mapping",
>> + &pdev->m_port_map[1],
>> + PM4125_MAX_SWR_PORTS);
>> + }
>> + if (ret < 0)
>> + dev_info(dev, "Error getting static port mapping for %s (%d)\n",
>> + priv->is_tx ? "TX" : "RX", ret);
>> +
>> + priv->sdev = pdev;
>> + dev_set_drvdata(dev, priv);
>> +
>> + 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;
>> + pdev->prop.simple_clk_stop_capable = true;
>> +
>> + memset(master_ch_mask, 0, PM4125_MAX_SWR_CH_IDS);
>> +
>> + if (priv->is_tx) {
>> + master_ch_mask_size =
>> + of_property_count_u8_elems(dev->of_node,
>> + "qcom,tx-channel-mapping");
>> +
>> + if (master_ch_mask_size)
>> + ret = of_property_read_u8_array(dev->of_node,
>> + "qcom,tx-channel-mapping",
>> + master_ch_mask,
>> + master_ch_mask_size);
>> + } else {
>> + master_ch_mask_size =
>> + of_property_count_u8_elems(dev->of_node,
>> + "qcom,rx-channel-mapping");
>> +
>> + if (master_ch_mask_size)
>> + ret = of_property_read_u8_array(dev->of_node,
>> + "qcom,rx-channel-mapping",
>> + master_ch_mask,
>> + master_ch_mask_size);
>> + }
>> +
>> + if (ret < 0)
>> + dev_info(dev, "Static channel mapping not specified using device channel maps\n");
>> +
>> + if (priv->is_tx) {
>> + pdev->prop.source_ports = GENMASK(PM4125_MAX_TX_SWR_PORTS, 0);
>> + pdev->prop.src_dpn_prop = pm4125_dpn_prop;
>> + priv->ch_info = &pm4125_sdw_tx_ch_info[0];
>> +
>> + for (i = 0; i < master_ch_mask_size; i++)
>> + priv->ch_info[i].master_ch_mask =
>> + PM4125_SWRM_CH_MASK(master_ch_mask[i]);
>> +
>> + pdev->prop.wake_capable = true;
>> +
>> + priv->regmap = devm_regmap_init_sdw(pdev,
>> + &pm4125_regmap_config);
> we do have 100 chars per line, do not split the lines unless required.
Heh, I see, thanks. Checkpatch seems to check for 100 chars now.
[..]
>> diff --git a/sound/soc/codecs/pm4125.c b/sound/soc/codecs/pm4125.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..58435b9c75865b06b344c6d79800aa9b4bac3abd
>> --- /dev/null
>> +++ b/sound/soc/codecs/pm4125.c
>> @@ -0,0 +1,1848 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +// Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> +// Copyright (c) 2025, Linaro Ltd
>> +
>> +#include <linux/component.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/slab.h>
>> +#include <sound/jack.h>
>> +#include <sound/pcm_params.h>
>> +#include <sound/pcm.h>
>> +#include <sound/soc-dapm.h>
>> +#include <sound/soc.h>
>> +#include <sound/tlv.h>
>> +
>> +#include "wcd-mbhc-v2.h"
>> +#include "pm4125.h"
>> +
>> +#define WCD_MBHC_HS_V_MAX 1600
>> +#define PM4125_MBHC_MAX_BUTTONS 8
>> +
>> +#define PM4125_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |\
>> + SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_48000 |\
>> + SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_192000 |\
>> + SNDRV_PCM_RATE_384000)
>> +
>> +/* Fractional Rates */
>> +#define PM4125_FRAC_RATES (SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_88200 |\
>> + SNDRV_PCM_RATE_176400 | SNDRV_PCM_RATE_352800)
>> +
>> +#define PM4125_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE |\
>> + SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)
>> +
>> +/* Registers in SPMI addr space */
>> +#define PM4125_CODEC_RESET_REG 0xF3DB
>> +#define PM4125_CODEC_OFF 0x1
>> +#define PM4125_CODEC_ON 0x0
>> +#define PM4125_CODEC_FOUNDRY_ID_REG 0x7
>> +
>> +enum {
>> + HPH_COMP_DELAY,
>> + HPH_PA_DELAY,
>> +};
>> +
>> +enum {
>> + AIF1_PB = 0,
>> + AIF1_CAP,
>> + NUM_CODEC_DAIS,
>> +};
>> +
>> +struct pm4125_priv {
>> + struct sdw_slave *tx_sdw_dev;
>> + struct pm4125_sdw_priv *sdw_priv[NUM_CODEC_DAIS];
>> + struct device *txdev;
>> + struct device *rxdev;
>> + struct device_node *rxnode;
>> + struct device_node *txnode;
>> + struct regmap *regmap;
>> + struct regmap *spmi_regmap;
>> + /* mbhc module */
>> + struct wcd_mbhc *wcd_mbhc;
>> + struct wcd_mbhc_config mbhc_cfg;
>> + struct wcd_mbhc_intr intr_ids;
>> + struct irq_domain *virq;
>> + const struct regmap_irq_chip *pm4125_regmap_irq_chip;
>> + struct regmap_irq_chip_data *irq_chip;
>> + struct regulator_bulk_data supplies[PM4125_MAX_BULK_SUPPLY];
>> + struct regulator *buck_supply;
>
> unused.
Needs to be removed, yep.
>> + struct snd_soc_jack *jack;
>> + unsigned long status_mask;
>> + s32 micb_ref[PM4125_MAX_MICBIAS];
>> + s32 pullup_ref[PM4125_MAX_MICBIAS];
>> + u32 hph_mode;
>> + int ear_rx_path;
>> + u32 micb1_mv;
>> + u32 micb2_mv;
>> + u32 micb3_mv;
>> +
>> + int hphr_pdm_wd_int;
>> + int hphl_pdm_wd_int;
>> + bool comp1_enable;
>> + bool comp2_enable;
>> +
>> + atomic_t rx_clk_cnt;
>> + atomic_t ana_clk_count;
> unused
Ok.
[..]
>> +static int pm4125_handle_post_irq(void *data)
>> +{
>> + struct pm4125_priv *pm4125;
>> +
>> + if (data)
>> + pm4125 = (struct pm4125_priv *)data;
>> + else
>> + return IRQ_HANDLED;
> This will result in interrupt storm, as you are not clearning the source.
>
>> +
>> + regmap_write(pm4125->regmap, PM4125_DIG_SWR_INTR_CLEAR_0, 0);
>> + regmap_write(pm4125->regmap, PM4125_DIG_SWR_INTR_CLEAR_1, 0);
>> + regmap_write(pm4125->regmap, PM4125_DIG_SWR_INTR_CLEAR_2, 0);
>> +
>> + return IRQ_HANDLED;
>> +}
Do you mean that it should be:
static int pm4125_handle_post_irq(void *data)
{
struct pm4125_priv *pm4125 = (struct pm4125_priv *)data;
regmap_write(pm4125->regmap, PM4125_DIG_SWR_INTR_CLEAR_0, 0);
regmap_write(pm4125->regmap, PM4125_DIG_SWR_INTR_CLEAR_1, 0);
regmap_write(pm4125->regmap, PM4125_DIG_SWR_INTR_CLEAR_2, 0);
return IRQ_HANDLED;
}
I need to fix irq_drv_data = NULL in pm4125_regmap_irq_chip then.
IIRC it is always NULL.
The same behaviour as in wcd937x.c
>> +static const u32 pm4125_config_regs[] = {
>> + PM4125_DIG_SWR_INTR_LEVEL_0,
>> +};
>> +
>> +static const struct regmap_irq_chip pm4125_regmap_irq_chip = {
>> + .name = "pm4125",
>> + .irqs = pm4125_irqs,
>> + .num_irqs = ARRAY_SIZE(pm4125_irqs),
>> + .num_regs = 3,
>> + .status_base = PM4125_DIG_SWR_INTR_STATUS_0,
>> + .mask_base = PM4125_DIG_SWR_INTR_MASK_0,
>> + .ack_base = PM4125_DIG_SWR_INTR_CLEAR_0,
>> + .use_ack = 1,
>> + .clear_ack = 1,
>> + .config_base = pm4125_config_regs,
>> + .num_config_bases = ARRAY_SIZE(pm4125_config_regs),
>> + .num_config_regs = 1,
>> + .runtime_pm = true,
>> + .handle_post_irq = pm4125_handle_post_irq,
>> + .irq_drv_data = NULL,
>> +};
>> +
>> +static void pm4125_reset(struct pm4125_priv *pm4125)
>> +{
>> + regmap_write(pm4125->spmi_regmap,
>> + PM4125_CODEC_RESET_REG, PM4125_CODEC_OFF);
>> + usleep_range(20, 30);
>> +
>> + regmap_write(pm4125->spmi_regmap,
>> + PM4125_CODEC_RESET_REG, PM4125_CODEC_ON);
>> + usleep_range(5000, 5010);
>> +}
>> +
>> +static void pm4125_io_init(struct regmap *regmap)
>> +{
>> + /* Disable HPH OCP */
>> + regmap_update_bits(regmap, PM4125_ANA_HPHPA_CNP_CTL_2, 0x03, 0x00);
> pl use defines instead of using magic values.
> applies to most part of the driver.
> also pl take a look at
> snd_soc_component_read/write_field() functions which are handy
Ok, this will be some rework in order to do this.
>> + /* Enable surge protection */
>> + regmap_update_bits(regmap, PM4125_ANA_SURGE_EN, 0xC0, 0xC0);
>> +
>> + /* Disable mic bias pull down */
>> + regmap_update_bits(regmap, PM4125_ANA_MICBIAS_MICB_1_2_EN, 0x01, 0x00);
>> +}
>> +
>> +static int pm4125_global_mbias_disable(struct snd_soc_component *component)
>> +{
>> + snd_soc_component_update_bits(component, PM4125_ANA_MBIAS_EN,
>> + 0x10, 0x00);
>> + snd_soc_component_update_bits(component, PM4125_ANA_MBIAS_EN,
>> + 0x20, 0x00);
>> + return 0;
>> +}
>> +
>> +static int pm4125_global_mbias_enable(struct snd_soc_component *component)
>> +{
>> + snd_soc_component_update_bits(component, PM4125_ANA_MBIAS_EN,
>> + 0x20, 0x20);
>> + snd_soc_component_update_bits(component, PM4125_ANA_MBIAS_EN,
>> + 0x10, 0x10);
>> + usleep_range(1000, 1100);
>> + return 0;
>> +}
>> +
>> +static int pm4125_rx_clk_enable(struct snd_soc_component *component)
>
> looks like the rxclk can be converted into a proper dapm supply widget.
I'll check that. Thank you.
[..]
>> +static int pm4125_codec_hphl_dac_event(struct snd_soc_dapm_widget *w,
>> + struct snd_kcontrol *kcontrol, int event)
>> +{
>> + struct snd_soc_component *component =
>> + snd_soc_dapm_to_component(w->dapm);
>> + struct pm4125_priv *pm4125 = snd_soc_component_get_drvdata(component);
>> +
>> + switch (event) {
>> + case SND_SOC_DAPM_PRE_PMU:
>> + pm4125_rx_clk_enable(component);
>> + snd_soc_component_update_bits(component,
>> + PM4125_ANA_HPHPA_CNP_CTL_1,
>> + 0x02, 0x02);
>> +
>> + snd_soc_component_update_bits(component,
>> + PM4125_SWR_HPHPA_HD2,
>> + 0x38, 0x38);
>> +
>> + set_bit(HPH_COMP_DELAY, &pm4125->status_mask);
>> + break;
>> + case SND_SOC_DAPM_POST_PMU:
>> +
> drop space.
Ok.
>> + if (pm4125->comp1_enable) {
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_CDC_COMP_CTL_0,
>> + 0x02, 0x02);
>> +
>> + if (pm4125->comp2_enable)
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_CDC_COMP_CTL_0,
>> + 0x01, 0x01);
>> + /*
>> + * 5ms sleep is required after COMP is enabled as per
>> + * HW requirement
>> + */
>> + if (test_bit(HPH_COMP_DELAY, &pm4125->status_mask)) {
>> + usleep_range(5000, 5100);
>> + clear_bit(HPH_COMP_DELAY, &pm4125->status_mask);
>> + }
>> + } else {
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_CDC_COMP_CTL_0,
>> + 0x02, 0x00);
>> + }
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_CDC_RX0_CTL,
>> + 0x80, 0x00);
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_CDC_RX_GAIN_CTL,
>> + 0x04, 0x04);
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_CDC_RX_CLK_CTL,
>> + 0x01, 0x01);
>> + break;
>> + case SND_SOC_DAPM_POST_PMD:
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_CDC_RX_CLK_CTL,
>> + 0x01, 0x00);
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_CDC_RX_GAIN_CTL,
>> + 0x04, 0x00);
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_CDC_RX0_CTL,
>> + 0x80, 0x80);
>> + if (pm4125->comp1_enable)
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_CDC_COMP_CTL_0,
>> + 0x02, 0x00);
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int pm4125_codec_hphr_dac_event(struct snd_soc_dapm_widget *w,
>> + struct snd_kcontrol *kcontrol, int event)
>> +{
>> + struct snd_soc_component *component =
>> + snd_soc_dapm_to_component(w->dapm);
> this can fit in 100 chars.
Ok.
[...]
>> +static int pm4125_codec_enable_hphl_pa(struct snd_soc_dapm_widget *w,
>> + struct snd_kcontrol *kcontrol, int event)
>> +{
>> + struct snd_soc_component *component =
>> + snd_soc_dapm_to_component(w->dapm);
>> + struct pm4125_priv *pm4125 = snd_soc_component_get_drvdata(component);
>> +
>> + switch (event) {
>> + case SND_SOC_DAPM_PRE_PMU:
>> + usleep_range(200, 210);
>> + set_bit(HPH_PA_DELAY, &pm4125->status_mask);
> Am really not following the logic here, we set the bit here and check
> the bit in POST_PMU and then we set BIT in PRE_PMD and clear it int
> POST_PMD.
>
> So this bit will be set in all the cases.
>
> why do we even need this bit to be set, can we not use the dealy by default?
I guess it makes a lot of sense to remove it.
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_PDM_WD_CTL0,
>> + 0x03, 0x03);
>> + break;
>> + case SND_SOC_DAPM_POST_PMU:
>> + if (test_bit(HPH_PA_DELAY, &pm4125->status_mask)) {
>> + usleep_range(5000, 5100);
>> + clear_bit(HPH_PA_DELAY, &pm4125->status_mask);
>> + }
>> +
>> + enable_irq(pm4125->hphl_pdm_wd_int);
>> + break;
>> + case SND_SOC_DAPM_PRE_PMD:
>> + disable_irq_nosync(pm4125->hphl_pdm_wd_int);
>> + set_bit(HPH_PA_DELAY, &pm4125->status_mask);
>> + break;
>> + case SND_SOC_DAPM_POST_PMD:
>> + if (test_bit(HPH_PA_DELAY, &pm4125->status_mask)) {
>> + usleep_range(5000, 5100);
>> + clear_bit(HPH_PA_DELAY, &pm4125->status_mask);
>> + }
>> +
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_PDM_WD_CTL0,
>> + 0x03, 0x00);
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int pm4125_codec_enable_lo_pa(struct snd_soc_dapm_widget *w,
>> + struct snd_kcontrol *kcontrol, int event)
>> +{
>> + struct snd_soc_component *component =
>> + snd_soc_dapm_to_component(w->dapm);
>> + struct pm4125_priv *pm4125 = snd_soc_component_get_drvdata(component);
>> +
>> + switch (event) {
>> + case SND_SOC_DAPM_PRE_PMU:
>> + snd_soc_component_update_bits(component,
>> + PM4125_ANA_COMBOPA_CTL_5,
>> + 0x04, 0x00);
>> + usleep_range(1000, 1010);
>> + snd_soc_component_update_bits(component,
>> + PM4125_ANA_COMBOPA_CTL_4,
>> + 0x0F, 0x0F);
>> + usleep_range(1000, 1010);
>> + snd_soc_component_update_bits(component,
>> + PM4125_ANA_COMBOPA_CTL,
>> + 0x40, 0x40);
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_PDM_WD_CTL0,
>> + 0x03, 0x03);
>> + break;
>> + case SND_SOC_DAPM_POST_PMU:
>> + usleep_range(5000, 5010);
>> +
>> + snd_soc_component_update_bits(component,
>> + PM4125_ANA_COMBOPA_CTL_4,
>> + 0x0F, 0x04);
>> + enable_irq(pm4125->hphl_pdm_wd_int);
>> + break;
>> + case SND_SOC_DAPM_PRE_PMD:
>> + disable_irq_nosync(pm4125->hphl_pdm_wd_int);
>> + break;
>> + case SND_SOC_DAPM_POST_PMD:
>> + usleep_range(2000, 2010);
>> +
> Empty lining is really not consistent across the code, either use it
> consistently or not use it at all.
Ok.
[...]
>> +static int pm4125_enable_rx1(struct snd_soc_dapm_widget *w,
>> + struct snd_kcontrol *kcontrol, int event)
>> +{
>> + struct snd_soc_component *component =
>> + snd_soc_dapm_to_component(w->dapm);
>> +
>> + if (event == SND_SOC_DAPM_POST_PMD)
>> + pm4125_rx_clk_disable(component);
>
> if we convert the rx clk into dapm, then things will be inplace
> automatically and you do not do this kinda handling.
Thanks. I need to check this.
>> +
>> + return 0;
>> +}
>> +
>> +static int pm4125_enable_rx2(struct snd_soc_dapm_widget *w,
>> + struct snd_kcontrol *kcontrol, int event)
>> +{
>> + struct snd_soc_component *component =
>> + snd_soc_dapm_to_component(w->dapm);
>> +
>> + if (event == SND_SOC_DAPM_POST_PMD)
>> + pm4125_rx_clk_disable(component);
>> +
>> + return 0;
>> +}
>
> This function is duplicate of pm4125_enable_rx1()...
Heh. Ok.
>> +static int pm4125_get_micb_vout_ctl_val(u32 micb_mv)
>> +{
>> + if (micb_mv < 1600 || micb_mv > 2850) {
>> + pr_err("Unsupported micbias voltage (%u mV)\n", micb_mv);
>> + return -EINVAL;
>> + }
>> +
>> + return (micb_mv - 1600) / 50;
>> +}
>> +
>> +static int pm4125_codec_enable_adc(struct snd_soc_dapm_widget *w,
>> + struct snd_kcontrol *kcontrol, int event)
>> +{
> Lets add the code needed for ADC in next spin.
Yes.
>> +static int pm4125_codec_enable_dmic(struct snd_soc_dapm_widget *w,
>> + struct snd_kcontrol *kcontrol, int event)
>> +{
>> + struct snd_soc_component *component =
>> + snd_soc_dapm_to_component(w->dapm);
>> + u16 dmic_clk_reg;
>> +
>> + switch (w->shift) {
>> + case 0:
>> + case 1:
> Can we use proper names here, instead of 1 and 0, can we use
> PM4125_DMIC0, PM4125_DMIC1 ?
Okay.
>> + dmic_clk_reg = PM4125_DIG_SWR_CDC_DMIC1_CTL;
> Also why can not we pass this as reg to the widget? then you can avoid
> all this switch caseing.
No idea. Let me check if I'll be able to fix that.
>> + break;
>> + default:
>> + dev_err(component->dev, "Invalid DMIC selection\n");
>> + return -EINVAL;
>> + }
>> +
>> + switch (event) {
>> + case SND_SOC_DAPM_PRE_PMU:
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_CDC_AMIC_CTL,
>> + 0x02, 0x00);
>> + snd_soc_component_update_bits(component,
>> + dmic_clk_reg,
>> + 0x08, 0x08);
>> + break;
>> + case SND_SOC_DAPM_POST_PMD:
>> + snd_soc_component_update_bits(component,
>> + dmic_clk_reg,
>> + 0x08, 0x00);
>> + snd_soc_component_update_bits(component,
>> + PM4125_DIG_SWR_CDC_AMIC_CTL,
>> + 0x02, 0x02);
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int pm4125_micbias_control(struct snd_soc_component *component,
>> + int micb_num, int req, bool is_dapm)
>> +{
> Lets implement this in v2.
Yes, agree.
>> +static int __pm4125_codec_enable_micbias(struct snd_soc_dapm_widget *w,
>> + int event)
>> +{
>> + struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
>> + int micb_num = w->shift;
>> +
>> + switch (event) {
>> + case SND_SOC_DAPM_PRE_PMU:
>> + pm4125_micbias_control(component, micb_num,
>> + MICB_ENABLE, true);
> MICBIAS 3 needs internal LDO pull up, as we can not connect it to
> vairable voltage.
Are we missing some code here? Maybe let's see if we can add it then?
>> + break;
>> + case SND_SOC_DAPM_POST_PMU:
>> + usleep_range(1000, 1100);
>> + break;
>> + case SND_SOC_DAPM_POST_PMD:
>> + pm4125_micbias_control(component, micb_num,
>> + MICB_DISABLE, true);
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int pm4125_codec_enable_micbias(struct snd_soc_dapm_widget *w,
>> + struct snd_kcontrol *kcontrol, int event)
>> +{
>> + return __pm4125_codec_enable_micbias(w, event);
> Why do we need this boiler plate, same comment applies to other parts of
> the code too.
I tried to make a minimal working version (that doesn't horribly fail) that
we're going to update with more patches during next submission.
[..]
>> +static const struct snd_kcontrol_new hph_type_detect_controls[] = {
>> + SOC_SINGLE_EXT("HPH Type", 0, 0, WCD_MBHC_HPH_STEREO, 0, NULL, NULL),
>> +};
>> +
>> +static const struct snd_kcontrol_new impedance_detect_controls[] = {
>> + SOC_SINGLE_EXT("HPHL Impedance", 0, 0, INT_MAX, 0, NULL, NULL),
>> + SOC_SINGLE_EXT("HPHR Impedance", 0, 1, INT_MAX, 0, NULL, NULL),
>
> What are these supposed to read?
Erm, nothing at this point? It is a stub for future changes.
>> +static void pm4125_mbhc_mbhc_bias_control(struct snd_soc_component *component,
>> + bool enable)
>> +{
>> + snd_soc_component_update_bits(component, PM4125_ANA_MBHC_ELECT, 0x01,
>> + enable ? 0x01 : 0x00);
>> +}
>> +
>> +static void pm4125_mbhc_program_btn_thr(struct snd_soc_component *component,
>> + int *btn_low, int *btn_high,
>> + int num_btn, bool is_micbias)
>> +{
>> + int i, vth;
>> +
>> + if (num_btn > WCD_MBHC_DEF_BUTTONS) {
>> + dev_err(component->dev, "%s: invalid number of buttons: %d\n",
>> + __func__, num_btn);
>> + return;
>> + }
>> +
>> + for (i = 0; i < num_btn; i++) {
>> + vth = ((btn_high[i] * 2) / 25) & 0x3F;
>> + snd_soc_component_write_field(component,
>> + PM4125_ANA_MBHC_BTN0_ZDET_VREF1 + i,
>> + 0xfc, vth << 2);
>> + }
>> +}
>> +
>> +static const struct wcd_mbhc_cb mbhc_cb = {
>> + .clk_setup = NULL,
>> + .mbhc_bias = pm4125_mbhc_mbhc_bias_control,
>> + .set_btn_thr = pm4125_mbhc_program_btn_thr,
>> + .micbias_enable_status = NULL,
>> + .hph_pull_up_control_v2 = NULL,
>> + .mbhc_micbias_control = NULL,
>> + .mbhc_micb_ramp_control = NULL,
>> + .mbhc_micb_ctrl_thr_mic = NULL,
>> + .compute_impedance = NULL,
>> + .mbhc_gnd_det_ctrl = NULL,
>> + .hph_pull_down_ctrl = NULL,
>> + .mbhc_moisture_config = NULL,
>> + .mbhc_get_moisture_status = NULL,
>> + .mbhc_moisture_polling_ctrl = NULL,
>> + .mbhc_moisture_detect_en = NULL,
> Either we add mbhc or not, having these dummy functions is not really
> helping.
Okay, then I can remove it then.
[...]
>> +static bool pm4125_swap_gnd_mic(struct snd_soc_component *component)
>> +{
>> + return true;
>
> why true all the time? Isn't this suppose to control a mux or a gpio?
I can see if I can remove it or let me know if you have code that implements
this. It was supposed to be a stub.
[...]
>> diff --git a/sound/soc/codecs/pm4125.h b/sound/soc/codecs/pm4125.h
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..2c5e8218202d92a0adc493413368991a406471b0
>> --- /dev/null
>> +++ b/sound/soc/codecs/pm4125.h
>> @@ -0,0 +1,375 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only
>> + * Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#ifndef _PM4125_REGISTERS_H
>> +#define _PM4125_REGISTERS_H
>> +
>> +#include <linux/soundwire/sdw.h>
>> +#include <linux/soundwire/sdw_type.h>
>> +
>> +#define PM4125_ANA_BASE_ADDR 0x3000
>> +#define PM4125_DIG_BASE_ADDR 0x3400
>> +
>> +#define PM4125_REG(reg) ((reg > PM4125_DIG_BASE_ADDR) ? \
>> + (reg - PM4125_DIG_BASE_ADDR) : \
>> + (reg - PM4125_ANA_BASE_ADDR))
>> +
>> +enum {
>> + REG_NO_ACCESS,
>> + RD_REG,
>> + WR_REG,
>> + RD_WR_REG
>> +};
>> +
> Both PM4125_REG(), and RD/WR_REG does not make sense, please use the
> registers directly in the regmap config callbacks.
Yes, okay.
> ...
>> +#define PM4125_MAX_MICBIAS 3
>> +#define PM4125_MAX_BULK_SUPPLY 4
>> +#define PM4125_MAX_SWR_CH_IDS 15
>> +#define PM4125_SWRM_CH_MASK(ch_idx) BIT(ch_idx - 1)
>> +
>> +enum pm4125_tx_sdw_ports {
>> + PM4125_ADC_1_PORT = 1,
>> + PM4125_DMIC_0_3_MBHC_PORT,
>> + PM4125_MAX_TX_SWR_PORTS = PM4125_DMIC_0_3_MBHC_PORT,
>> +};
>> +
>> +enum pm4125_rx_sdw_ports {
>> + PM4125_HPH_PORT = 1,
>> + PM4125_COMP_PORT,
>> + PM4125_MAX_SWR_PORTS = PM4125_COMP_PORT,
>> +};
>> +
>> +struct pm4125_sdw_ch_info {
>> + int port_num;
>> + unsigned int ch_mask;
>> + unsigned int master_ch_mask;
>> +};
>> +
>> +#define WCD_SDW_CH(id, pn, cmask) \
>> + [id] = { \
>> + .port_num = pn, \
>> + .ch_mask = cmask, \
>> + .master_ch_mask = cmask, \
>> + }
>> +
>> +struct pm4125_priv;
>> +struct pm4125_sdw_priv {
>> + struct sdw_slave *sdev;
>> + struct sdw_stream_config sconfig;
>> + struct sdw_stream_runtime *sruntime;
>> + struct sdw_port_config port_config[PM4125_MAX_SWR_PORTS];
>> + struct pm4125_sdw_ch_info *ch_info;
>> + bool port_enable[PM4125_MAX_SWR_CH_IDS];
>> + unsigned int master_channel_map[SDW_MAX_PORTS];
>> + int active_ports;
>> + int num_ports;
>> + bool is_tx;
>> + struct pm4125_priv *pm4125;
>> + struct irq_domain *slave_irq;
>> + struct regmap *regmap;
>> +};
>> +
>> +#if IS_ENABLED(CONFIG_SND_SOC_PM4125_SDW)
>> +int pm4125_sdw_free(struct pm4125_sdw_priv *pm4125,
>> + struct snd_pcm_substream *substream,
>> + struct snd_soc_dai *dai);
>> +int pm4125_sdw_set_sdw_stream(struct pm4125_sdw_priv *pm4125,
>> + struct snd_soc_dai *dai,
>> + void *stream, int direction);
>> +int pm4125_sdw_hw_params(struct pm4125_sdw_priv *pm4125,
>> + struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params,
>> + struct snd_soc_dai *dai);
>> +
>> +struct device *pm4125_sdw_device_get(struct device_node *np);
>> +
>> +#else
>> +int pm4125_sdw_free(struct pm4125_sdw_priv *pm4125,
> Should this be static inline, if not you will endup with multiple
> defination of the function.
> same for other stubs too.
Thanks, I'll check that.
>> + struct snd_pcm_substream *substream,
>> + struct snd_soc_dai *dai)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +
>> +int pm4125_sdw_set_sdw_stream(struct pm4125_sdw_priv *pm4125,
>> + struct snd_soc_dai *dai,
>> + void *stream, int direction)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +
>> +int pm4125_sdw_hw_params(struct pm4125_sdw_priv *pm4125,
>> + struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params,
>> + struct snd_soc_dai *dai)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +#endif
>> +
>> +enum {
>> + /* INTR_CTRL_INT_MASK_0 */
>> + PM4125_IRQ_MBHC_BUTTON_PRESS_DET = 0,
>> + PM4125_IRQ_MBHC_BUTTON_RELEASE_DET,
>> + PM4125_IRQ_MBHC_ELECT_INS_REM_DET,
>> + PM4125_IRQ_MBHC_ELECT_INS_REM_LEG_DET,
>> + PM4125_IRQ_MBHC_SW_DET,
>> + PM4125_IRQ_HPHR_OCP_INT,
>> + PM4125_IRQ_HPHR_CNP_INT,
>> + PM4125_IRQ_HPHL_OCP_INT,
>> +
>> + /* INTR_CTRL_INT_MASK_1 */
>> + PM4125_IRQ_HPHL_CNP_INT,
>> + PM4125_IRQ_EAR_CNP_INT,
>> + PM4125_IRQ_EAR_SCD_INT,
>> + PM4125_IRQ_AUX_CNP_INT,
>> + PM4125_IRQ_AUX_SCD_INT,
>> + PM4125_IRQ_HPHL_PDM_WD_INT,
>> + PM4125_IRQ_HPHR_PDM_WD_INT,
>> + PM4125_IRQ_AUX_PDM_WD_INT,
>> +
>> + /* INTR_CTRL_INT_MASK_2 */
>> + PM4125_IRQ_LDORT_SCD_INT,
>> + PM4125_IRQ_MBHC_MOISTURE_INT,
>> + PM4125_IRQ_HPHL_SURGE_DET_INT,
>> + PM4125_IRQ_HPHR_SURGE_DET_INT,
>> + PM4125_NUM_IRQS,
>> +};
>> +
>> +enum pm4125_tx_sdw_channels {
>> + PM4125_ADC1,
>> + PM4125_ADC2,
>> + PM4125_ADC3,
>> + PM4125_DMIC0,
>> + PM4125_DMIC1,
>> + PM4125_MBHC,
>> + PM4125_DMIC2,
>> + PM4125_DMIC3,
>> + PM4125_DMIC4,
>> + PM4125_DMIC5,
>> + PM4125_DMIC6,
> do we really have so many channels on TX, AFAIU, its only 2 channels and
> two lanes.
>> +};
>> +
>> +enum pm4125_rx_sdw_channels {
>> + PM4125_HPH_L,
>> + PM4125_HPH_R,
>> + PM4125_CLSH,
>> + PM4125_COMP_L,
>> + PM4125_COMP_R,
>> + PM4125_LO,
>> + PM4125_DSD_R,
>> + PM4125_DSD_L,
> Do we have so many channes? AFAIU, this is only 4 channels and 1 lane.
I thought it is 2 or 4 channels and 2 lanes for rx.
But for tx it is 2 channels and 1 lane.
Anyways, this needs to be fixed.
Thank you for your comments.
Best regards,
Alexey
Powered by blists - more mailing lists