| 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: <96b2cccc-6b71-e70f-ba2c-38554c65e2c5@linaro.org>
Date: Mon, 22 Mar 2021 10:27:17 +0000
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
broonie@...nel.org
Cc: robh@...nel.org, alsa-devel@...a-project.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
lgirdwood@...il.com
Subject: 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 000000000000..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 = &pdev->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", &dir);
>> + 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 = &wcd938x_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 = &wcd938x_sdw_rx_ch_info[0];
>> + }
>> +
>> + if (wcd->is_tx)
>> + return wcd938x_register_component(wcd, dev, &wcd938x_tx_dai);
>> + else
>> + return wcd938x_register_component(wcd, dev, &wcd938x_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 = &wcd9380_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,
>> + .val_bits = 8,
>> + .cache_type = REGCACHE_RBTREE,
>> + .reg_defaults = wcd938x_defaults,
>> + .num_reg_defaults = ARRAY_SIZE(wcd938x_defaults),
>> + .max_register = WCD938X_MAX_REGISTER,
>> + .readable_reg = wcd938x_readable_register,
>> + .writeable_reg = wcd938x_writeable_register,
>> + .volatile_reg = wcd938x_volatile_register,
>> + .can_multi_write = true,
>> +};
>> +
>> +static const struct regmap_irq wcd938x_irqs[WCD938X_NUM_IRQS] = {
>> + REGMAP_IRQ_REG(WCD938X_IRQ_MBHC_BUTTON_PRESS_DET, 0, 0x01),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_MBHC_BUTTON_RELEASE_DET, 0, 0x02),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_MBHC_ELECT_INS_REM_DET, 0, 0x04),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_MBHC_ELECT_INS_REM_LEG_DET, 0, 0x08),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_MBHC_SW_DET, 0, 0x10),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_HPHR_OCP_INT, 0, 0x20),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_HPHR_CNP_INT, 0, 0x40),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_HPHL_OCP_INT, 0, 0x80),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_HPHL_CNP_INT, 1, 0x01),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_EAR_CNP_INT, 1, 0x02),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_EAR_SCD_INT, 1, 0x04),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_AUX_CNP_INT, 1, 0x08),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_AUX_SCD_INT, 1, 0x10),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_HPHL_PDM_WD_INT, 1, 0x20),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_HPHR_PDM_WD_INT, 1, 0x40),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_AUX_PDM_WD_INT, 1, 0x80),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_LDORT_SCD_INT, 2, 0x01),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_MBHC_MOISTURE_INT, 2, 0x02),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_HPHL_SURGE_DET_INT, 2, 0x04),
>> + REGMAP_IRQ_REG(WCD938X_IRQ_HPHR_SURGE_DET_INT, 2, 0x08),
>> +};
>> +
>> +static struct regmap_irq_chip wcd938x_regmap_irq_chip = {
>> + .name = "wcd938x",
>> + .irqs = wcd938x_irqs,
>> + .num_irqs = ARRAY_SIZE(wcd938x_irqs),
>> + .num_regs = 3,
>> + .status_base = WCD938X_DIGITAL_INTR_STATUS_0,
>> + .mask_base = WCD938X_DIGITAL_INTR_MASK_0,
>> + .type_base = WCD938X_DIGITAL_INTR_LEVEL_0,
>> + .ack_base = WCD938X_DIGITAL_INTR_CLEAR_0,
>> + .use_ack = 1,
>> + .runtime_pm = true,
>> + .irq_drv_data = NULL,
>> +};
>> +
>> +int wcd938x_io_init(struct wcd938x_sdw_priv *wcd)
>> +{
>> + struct regmap *rm = wcd->wcd938x->regmap;
>> +
>> + if (!wcd->is_tx)
>> + return 0;
>
> and here you only initialize the registers for the tx case, so what
> happens you you dump the rx registers or try to set them with
> /sys/kernel/debug/regmap?
wcd->wcd938x->regmap is regmap for tx device.
There should be no regmap for rx device at all!
There will be only one entry for wcd938x in /sys/kernel/debug/regmap
which is
/sys/kernel/debug/regmap/sdw:0:217:10d:0:3-wcd938x_csr
>
>> +
>> + regmap_update_bits(rm, WCD938X_SLEEP_CTL, 0x0E, 0x0E);
>> + regmap_update_bits(rm, WCD938X_SLEEP_CTL, 0x80, 0x80);
>> + /* 1 msec delay as per HW requirement */
>> + usleep_range(1000, 1010);
>> + regmap_update_bits(rm, WCD938X_SLEEP_CTL, 0x40, 0x40);
>> + /* 1 msec delay as per HW requirement */
>> + usleep_range(1000, 1010);
>> + regmap_update_bits(rm, WCD938X_LDORXTX_CONFIG, 0x10, 0x00);
>> + regmap_update_bits(rm, WCD938X_BIAS_VBG_FINE_ADJ,
>> + 0xF0, 0x80);
>> + regmap_update_bits(rm, WCD938X_ANA_BIAS, 0x80, 0x80);
>> + regmap_update_bits(rm, WCD938X_ANA_BIAS, 0x40, 0x40);
>> + /* 10 msec delay as per HW requirement */
>> + usleep_range(10000, 10010);
>> +
>> + regmap_update_bits(rm, WCD938X_ANA_BIAS, 0x40, 0x00);
>> + regmap_update_bits(rm, WCD938X_HPH_NEW_INT_RDAC_GAIN_CTL,
>> + 0xF0, 0x00);
>> + regmap_update_bits(rm, WCD938X_HPH_NEW_INT_RDAC_HD2_CTL_L_NEW,
>> + 0x1F, 0x15);
>> + regmap_update_bits(rm, WCD938X_HPH_NEW_INT_RDAC_HD2_CTL_R_NEW,
>> + 0x1F, 0x15);
>> + regmap_update_bits(rm, WCD938X_HPH_REFBUFF_UHQA_CTL,
>> + 0xC0, 0x80);
>> + regmap_update_bits(rm, WCD938X_DIGITAL_CDC_DMIC_CTL,
>> + 0x02, 0x02);
>> +
>> + regmap_update_bits(rm,
>> WCD938X_TX_COM_NEW_INT_TXFE_ICTRL_STG2CASC_ULP,
>> + 0xFF, 0x14);
>> + regmap_update_bits(rm,
>> WCD938X_TX_COM_NEW_INT_TXFE_ICTRL_STG2MAIN_ULP,
>> + 0x1F, 0x08);
>> +
>> + regmap_update_bits(rm, WCD938X_DIGITAL_TX_REQ_FB_CTL_0, 0xFF, 0x55);
>> + regmap_update_bits(rm, WCD938X_DIGITAL_TX_REQ_FB_CTL_1, 0xFF, 0x44);
>> + regmap_update_bits(rm, WCD938X_DIGITAL_TX_REQ_FB_CTL_2, 0xFF, 0x11);
>> + regmap_update_bits(rm, WCD938X_DIGITAL_TX_REQ_FB_CTL_3, 0xFF, 0x00);
>> + regmap_update_bits(rm, WCD938X_DIGITAL_TX_REQ_FB_CTL_4, 0xFF, 0x00);
>> +
>> + /* Set Noise Filter Resistor value */
>> + regmap_update_bits(rm, WCD938X_MICB1_TEST_CTL_1, 0xE0, 0xE0);
>> + regmap_update_bits(rm, WCD938X_MICB2_TEST_CTL_1, 0xE0, 0xE0);
>> + regmap_update_bits(rm, WCD938X_MICB3_TEST_CTL_1, 0xE0, 0xE0);
>> + regmap_update_bits(rm, WCD938X_MICB4_TEST_CTL_1, 0xE0, 0xE0);
>> +
>> + regmap_update_bits(rm, WCD938X_TX_3_4_TEST_BLK_EN2, 0x01, 0x00);
>> + regmap_update_bits(rm, WCD938X_HPH_SURGE_HPHLR_SURGE_EN, 0xC0,
>> 0xC0);
>> +
>> + return 0;
>> +
>> +}
>> +
>> +static int wcd938x_get_micb_vout_ctl_val(u32 micb_mv)
>> +{
>> + /* min micbias voltage is 1V and maximum is 2.85V */
>> + if (micb_mv < 1000 || micb_mv > 2850)
>> + return -EINVAL;
>> +
>> + return (micb_mv - 1000) / 50;
>> +}
>> +
>> +static int wcd938x_set_micbias_data(struct wcd938x_priv *wcd938x)
>> +{
>> + int vout_ctl_1, vout_ctl_2, vout_ctl_3, vout_ctl_4;
>> +
>> + /* set micbias voltage */
>> + vout_ctl_1 = wcd938x_get_micb_vout_ctl_val(wcd938x->micb1_mv);
>> + vout_ctl_2 = wcd938x_get_micb_vout_ctl_val(wcd938x->micb2_mv);
>> + vout_ctl_3 = wcd938x_get_micb_vout_ctl_val(wcd938x->micb3_mv);
>> + vout_ctl_4 = wcd938x_get_micb_vout_ctl_val(wcd938x->micb4_mv);
>> + if (vout_ctl_1 < 0 || vout_ctl_2 < 0 || vout_ctl_3 < 0 ||
>> vout_ctl_4 < 0)
>> + return -EINVAL;
>> +
>> + regmap_update_bits(wcd938x->regmap, WCD938X_ANA_MICB1,
>> + WCD938X_MICB_VOUT_MASK, vout_ctl_1);
>> + regmap_update_bits(wcd938x->regmap, WCD938X_ANA_MICB2,
>> + WCD938X_MICB_VOUT_MASK, vout_ctl_2);
>> + regmap_update_bits(wcd938x->regmap, WCD938X_ANA_MICB3,
>> + WCD938X_MICB_VOUT_MASK, vout_ctl_3);
>> + regmap_update_bits(wcd938x->regmap, WCD938X_ANA_MICB4,
>> + WCD938X_MICB_VOUT_MASK, vout_ctl_4);
>> +
>> + return 0;
>> +}
>> +
>> +static int wcd938x_soc_codec_rx_probe(struct snd_soc_component
>> *component)
>> +{
>> + struct wcd938x_sdw_priv *wcd =
>> snd_soc_component_get_drvdata(component);
>> + struct wcd938x_priv *wcd938x = wcd->wcd938x;
>> +
>> + snd_soc_component_init_regmap(component, wcd938x->regmap);
>> +
>> + wcd938x->clsh_info = wcd_clsh_ctrl_alloc(component, WCD938X);
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t wcd938x_wd_handle_irq(int irq, void *data)
>> +{
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static struct irq_chip wcd_irq_chip = {
>> + .name = "WCD938x",
>> +};
>> +
>> +static int wcd_irq_chip_map(struct irq_domain *irqd, unsigned int virq,
>> + irq_hw_number_t hw)
>> +{
>> + irq_set_chip_and_handler(virq, &wcd_irq_chip, handle_simple_irq);
>> + irq_set_nested_thread(virq, 1);
>> + irq_set_noprobe(virq);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct irq_domain_ops wcd_domain_ops = {
>> + .map = wcd_irq_chip_map,
>> +};
>> +
>> +static int wcd938x_irq_init(struct wcd938x_sdw_priv *data, struct
>> device *dev)
>> +{
>> + struct wcd938x_priv *wcd = data->wcd938x;
>> +
>> + wcd->virq = irq_domain_add_linear(NULL, 1, &wcd_domain_ops, NULL);
>> + if (!(wcd->virq)) {
>> + dev_err(dev, "%s: Failed to add IRQ domain\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + return devm_regmap_add_irq_chip(dev, wcd->regmap,
>> + irq_create_mapping(wcd->virq, 0),
>> + IRQF_ONESHOT, 0, &wcd938x_regmap_irq_chip,
>> + &wcd->irq_chip);
>> +}
>> +
>> +static int wcd938x_soc_codec_probe(struct snd_soc_component *component)
>
> is this supposed to be soc_tx_codec_probe()?
>
Yes, I will rename such fuctions explictly in next version to avoid
confusion.
>> +{
>> + struct wcd938x_sdw_priv *wcd =
>> snd_soc_component_get_drvdata(component);
>> + struct wcd938x_priv *wcd938x = wcd->wcd938x;
>> + struct device *dev = component->dev;
>> + int ret, i;
>> +
>> + wait_for_completion_timeout(&wcd->sdev->initialization_complete,
>> + msecs_to_jiffies(3000));
>> +
>> + snd_soc_component_init_regmap(component, wcd938x->regmap);
>> +
>> + wcd938x->variant = snd_soc_component_read_field(component,
>> + WCD938X_DIGITAL_EFUSE_REG_0,
>> + WCD938X_ID_MASK);
>> +
>> + /* Set all interrupts as edge triggered */
>> + for (i = 0; i < wcd938x_regmap_irq_chip.num_regs; i++) {
>> + regmap_write(wcd938x->regmap,
>> + (WCD938X_DIGITAL_INTR_LEVEL_0 + i), 0);
>> + }
>> +
>> + ret = wcd938x_irq_init(wcd, component->dev);
>> + if (ret) {
>> + dev_err(component->dev, "%s: IRQ init failed: %d\n",
>> + __func__, ret);
>> + return ret;
>> + }
>> +
>> + wcd938x->hphr_pdm_wd_int = regmap_irq_get_virq(wcd938x->irq_chip,
>> + WCD938X_IRQ_HPHR_PDM_WD_INT);
>> + wcd938x->hphl_pdm_wd_int = regmap_irq_get_virq(wcd938x->irq_chip,
>> + WCD938X_IRQ_HPHL_PDM_WD_INT);
>> + wcd938x->aux_pdm_wd_int = regmap_irq_get_virq(wcd938x->irq_chip,
>> + WCD938X_IRQ_AUX_PDM_WD_INT);
>> +
>> + /* Request for watchdog interrupt */
>> + ret = request_threaded_irq(wcd938x->hphr_pdm_wd_int, NULL,
>> wcd938x_wd_handle_irq,
>> + IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>> + "HPHR PDM WD INT", wcd938x);
>> + if (ret)
>> + dev_err(dev, "Failed to request HPHR WD interrupt (%d)\n", ret);
>> +
>> + ret = request_threaded_irq(wcd938x->hphl_pdm_wd_int, NULL,
>> wcd938x_wd_handle_irq,
>> + IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>> + "HPHL PDM WD INT", wcd938x);
>> + if (ret)
>> + dev_err(dev, "Failed to request HPHL WD interrupt (%d)\n", ret);
>> +
>> + ret = request_threaded_irq(wcd938x->aux_pdm_wd_int, NULL,
>> wcd938x_wd_handle_irq,
>> + IRQF_ONESHOT | IRQF_TRIGGER_RISING,
>> + "AUX PDM WD INT", wcd938x);
>> + if (ret)
>> + dev_err(dev, "Failed to request Aux WD interrupt (%d)\n", ret);
>> +
>> + /* Disable watchdog interrupt for HPH and AUX */
>> + disable_irq_nosync(wcd938x->hphr_pdm_wd_int);
>> + disable_irq_nosync(wcd938x->hphl_pdm_wd_int);
>> + disable_irq_nosync(wcd938x->aux_pdm_wd_int);
>> +
>> + return ret;
>> +}
>> +
>> +static const struct snd_soc_component_driver
>> soc_codec_dev_wcd938x_sdw_rx = {
>> + .name = "wcd938x_codec_rx",
>> + .probe = wcd938x_soc_codec_rx_probe,
>> +};
>> +
>> +static const struct snd_soc_component_driver
>> soc_codec_dev_wcd938x_sdw_tx = {
>> + .name = "wcd938x_codec_tx",
>> + .probe = wcd938x_soc_codec_probe,
>> +};
>> +
>> +static void wcd938x_dt_parse_micbias_info(struct device *dev, struct
>> wcd938x_priv *wcd)
>> +{
>> + struct device_node *np = dev->of_node;
>> + u32 prop_val = 0;
>> + int rc = 0;
>> +
>> + rc = of_property_read_u32(np, "qcom,micbias1-microvolt",
>> &prop_val);
>> + if (!rc)
>> + wcd->micb1_mv = prop_val/1000;
>> + else
>> + dev_info(dev, "%s: Micbias1 DT property not found\n", __func__);
>> +
>> + rc = of_property_read_u32(np, "qcom,micbias2-microvolt",
>> &prop_val);
>> + if (!rc)
>> + wcd->micb2_mv = prop_val/1000;
>> + else
>> + dev_info(dev, "%s: Micbias2 DT property not found\n", __func__);
>> +
>> + rc = of_property_read_u32(np, "qcom,micbias3-microvolt", &prop_val);
>> + if (!rc)
>> + wcd->micb3_mv = prop_val/1000;
>> + else
>> + dev_info(dev, "%s: Micbias3 DT property not found\n", __func__);
>> +
>> + rc = of_property_read_u32(np, "qcom,micbias4-microvolt",
>> &prop_val);
>> + if (!rc)
>> + wcd->micb4_mv = prop_val/1000;
>> + else
>> + dev_info(dev, "%s: Micbias4 DT property not found\n", __func__);
>> +}
>> +
>> +static int wcd938x_populate_dt_data(struct wcd938x_sdw_priv *wcd,
>> struct device *dev)
>> +{
>> + struct wcd938x_priv *wcd938x = wcd->wcd938x;
>> + int ret;
>> +
>> +
>
> extra lines
>
>> + wcd938x->reset_gpio = of_get_named_gpio(dev->of_node,
>> "reset-gpios", 0);
>> + if (wcd938x->reset_gpio < 0) {
>> + dev_err(dev, "Failed to get reset gpio: err = %d\n",
>> + wcd938x->reset_gpio);
>> + return wcd938x->reset_gpio;
>> + }
>> +
>> + wcd938x->supplies[0].supply = "vdd-rxtx";
>> + wcd938x->supplies[1].supply = "vdd-io";
>> + wcd938x->supplies[2].supply = "vdd-buck";
>> + wcd938x->supplies[3].supply = "vdd-mic-bias";
>> +
>> + ret = regulator_bulk_get(dev, WCD938X_MAX_SUPPLY,
>> wcd938x->supplies);
>> + if (ret) {
>> + dev_err(dev, "Failed to get supplies: err = %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = regulator_bulk_enable(WCD938X_MAX_SUPPLY, wcd938x->supplies);
>> + if (ret) {
>> + dev_err(dev, "Failed to enable supplies: err = %d\n", ret);
>> + return ret;
>> + }
>> +
>> + wcd938x_dt_parse_micbias_info(dev, wcd938x);
>> + return 0;
>> +}
>> +
>> +static int wcd938x_reset(struct wcd938x_sdw_priv *wcd, struct device
>> *dev)
>> +{
>> + struct wcd938x_priv *wcd938x = wcd->wcd938x;
>> +
>> + gpio_direction_output(wcd938x->reset_gpio, 0);
>> + /* 20us sleep required after pulling the reset gpio to LOW */
>> + usleep_range(20, 30);
>> + gpio_set_value(wcd938x->reset_gpio, 1);
>> + /* 20us sleep required after pulling the reset gpio to HIGH */
>> + usleep_range(20, 30);
>
> so each Slave device has its own GPIOs and regulators?
>
unfortunately No, reset gpio and regulators are common for the whole codec.
>> +
>> + return 0;
>> +}
>> +
>> +static int __wcd938x_init(struct wcd938x_sdw_priv *wcd, struct device
>> *dev)
>> +{
>> + int ret;
>> + struct wcd938x_priv *wcd938x = wcd->wcd938x;
>> +
>> + wcd938x_reset(wcd, dev);
>> +
>> + wcd938x->regmap = devm_regmap_init_sdw(wcd->sdev,
>> &wcd938x_regmap_config);
>> + if (IS_ERR(wcd938x->regmap)) {
>> + dev_err(dev, "%s: Regmap init failed\n", __func__);
>> + return PTR_ERR(wcd938x->regmap);
>> + }
>> +
>> + ret = wcd938x_set_micbias_data(wcd938x);
>> + if (ret < 0) {
>> + dev_err(dev, "%s: bad micbias data\n", __func__);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int wcd938x_register_component(struct wcd938x_sdw_priv *wcd, struct
>> device *dev,
>> + struct snd_soc_dai_driver *dai_driver)
>> +{
>> + if (wcd->is_tx)
>> + return snd_soc_register_component(dev,
>> + &soc_codec_dev_wcd938x_sdw_tx,
>> + dai_driver, 1);
>> + else
>> + return snd_soc_register_component(dev,
>> + &soc_codec_dev_wcd938x_sdw_rx,
>> + dai_driver, 1);
>> +}
>> +
>> +int wcd938x_handle_sdw_irq(struct wcd938x_sdw_priv *wcd)
>> +{
>> + struct wcd938x_priv *wcd938x = wcd->wcd938x;
>> + struct irq_domain *slave_irq = wcd938x->virq;
>> + u32 sts1, sts2, sts3;
>> +
>> + do {
>> + handle_nested_irq(irq_find_mapping(slave_irq, 0));
>> + regmap_read(wcd938x->regmap, WCD938X_DIGITAL_INTR_STATUS_0,
>> &sts1);
>> + regmap_read(wcd938x->regmap, WCD938X_DIGITAL_INTR_STATUS_1,
>> &sts2);
>> + regmap_read(wcd938x->regmap, WCD938X_DIGITAL_INTR_STATUS_2,
>> &sts3);
>> +
>> + } while (sts1 || sts2 || sts3);
> i
> no timeout in case the hardware is stuck?
>
Even if you timeout here that means the interrupt source is not cleared
yet, so we can potentially re-enter to this function subsequently.
But I agree with you on the timeout for hardware stuck case, I will try
to add that in next version.
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +int wcd938x_init(struct wcd938x_sdw_priv *data)
>> +{
>> + struct wcd938x_priv *wcd938x = NULL;
>
> initialization doesn't seem needed
>
Ack!
>> + struct device *dev = &data->sdev->dev;
>> + bool is_tx = data->is_tx;
>> + int ret;
>> +
>> + if (!is_tx) {
>> + if (g_wcd938x) {
>> + data->wcd938x = g_wcd938x;
>> + return 0;
>> + } else {
>> + return -EPROBE_DEFER;
>> + }
>> + }
>> +
>> + wcd938x = devm_kzalloc(dev, sizeof(struct wcd938x_priv),
>> + GFP_KERNEL);
>> + if (!wcd938x)
>> + return -ENOMEM;
>> +
>> + data->wcd938x = wcd938x;
>> + g_wcd938x = wcd938x;
>> + wcd938x->dev = dev;
>> + ret = wcd938x_populate_dt_data(data, dev);
>> + if (ret) {
>> + dev_err(dev, "%s: Fail to obtain platform data\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + ret = __wcd938x_init(data, dev);
>> + if (ret) {
>> + dev_err(dev, "%s: Fail to init\n", __func__);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int wcd938x_deinit(struct wcd938x_sdw_priv *wcd)
>> +{
>> + struct wcd938x_priv *wcd938x = wcd->wcd938x;
>> +
>> + if (!wcd->is_tx)
>> + wcd_clsh_ctrl_free(wcd938x->clsh_info);
>> +
>> + g_wcd938x = NULL;
>> +
>> + return 0;
>> +}
>> diff --git a/sound/soc/codecs/wcd938x.h b/sound/soc/codecs/wcd938x.h
>> new file mode 100644
>> index 000000000000..c373850f209e
>> --- /dev/null
>> +++ b/sound/soc/codecs/wcd938x.h
>> @@ -0,0 +1,676 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>
> GPL-2.0-only
>
>> +#define WCD938X_MAX_SWR_CH_IDS 15
>
> what is this value? SoundWire supports up to 8 channels.
This is the virtual channel IDs given to group of channels on a port for
a particular function. ex:
Channel ID WCD938X_HPH_L is for Channel 0 on port WCD938X_HPH_PORT
Channel ID WCD938X_HPH_R is for Channel 1 on port WCD938X_HPH_PORT
These IDs are internally used by the codec for mixer settings.
they do not exactly reflect number of channels, they are just enums for
internal use.
--srini
Powered by blists - more mailing lists