Message-ID: <0acde3b4-a437-4fa1-b5bd-fe1810309bb8@wanadoo.fr>
Date: Fri, 11 Jul 2025 17:15:56 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Alexey Klimov <alexey.klimov@...aro.org>,
 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>,
 Srinivas Kandagatla <srinivas.kandagatla@....qualcomm.com>
Subject: Re: [PATCH v2 2/3] ASoC: codecs: add new pm4125 audio codec driver

Le 11/07/2025 à 05:00, Alexey Klimov a écrit :
> 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.
> 
> The codec driver also uses WCD MBCH framework. The MBHC functionality is
> implemented in a minimalistic way to enable IRQs and avoid different
> issues with IRQs.

Hi,
...

> +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 starts 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);

Nitpick: If a branch of an if needs { }, I think that both should have.

> +
> +	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);

...

> +static const struct sdw_device_id pm4125_slave_id[] = {
> +	SDW_SLAVE_ENTRY(0x0217, 0x10c, 0), /* Soundwire pm4125 RX/TX Device ID */
> +	{ },

No need for a trailing comma after a terminator

> +};
> +MODULE_DEVICE_TABLE(sdw, pm4125_slave_id);

...

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

Maybe, keep alphabetical order?

> +#include <sound/pcm_params.h>
> +#include <sound/pcm.h>
> +#include <sound/soc-dapm.h>
> +#include <sound/soc.h>
> +#include <sound/tlv.h>

...

> +static int pm4125_bind(struct device *dev)

If an error occures at some point, should things be undone before returning?

> +{
> +	struct pm4125_priv *pm4125 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	/* Give the soundwire subdevices some more time to settle */
> +	usleep_range(15000, 15010);
> +
> +	ret = component_bind_all(dev, pm4125);
> +	if (ret) {
> +		dev_err(dev, "Slave bind failed, ret = %d\n", ret);
> +		return ret;
> +	}
> +
> +	pm4125->rxdev = pm4125_sdw_device_get(pm4125->rxnode);
> +	if (!pm4125->rxdev) {
> +		dev_err(dev, "could not find rxslave with matching of node\n");
> +		return -EINVAL;
> +	}
> +
> +	pm4125->sdw_priv[AIF1_PB] = dev_get_drvdata(pm4125->rxdev);
> +	pm4125->sdw_priv[AIF1_PB]->pm4125 = pm4125;
> +
> +	pm4125->txdev = pm4125_sdw_device_get(pm4125->txnode);
> +	if (!pm4125->txdev) {
> +		dev_err(dev, "could not find txslave with matching of node\n");
> +		return -EINVAL;
> +	}
> +
> +	pm4125->sdw_priv[AIF1_CAP] = dev_get_drvdata(pm4125->txdev);
> +	pm4125->sdw_priv[AIF1_CAP]->pm4125 = pm4125;
> +
> +	pm4125->tx_sdw_dev = dev_to_sdw_dev(pm4125->txdev);
> +	if (!pm4125->tx_sdw_dev) {
> +		dev_err(dev, "could not get txslave with matching of dev\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * As TX is the main CSR reg interface, which should not be suspended first.
> +	 * expicilty add the dependency link
> +	 */
> +	if (!device_link_add(pm4125->rxdev, pm4125->txdev,
> +			     DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME)) {
> +		dev_err(dev, "Could not devlink TX and RX\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!device_link_add(dev, pm4125->txdev,
> +			     DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME)) {
> +		dev_err(dev, "Could not devlink PM4125 and TX\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!device_link_add(dev, pm4125->rxdev,
> +			     DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME)) {
> +		dev_err(dev, "Could not devlink PM4125 and RX\n");
> +		return -EINVAL;
> +	}
> +
> +	pm4125->regmap = dev_get_regmap(&pm4125->tx_sdw_dev->dev, NULL);
> +	if (!pm4125->regmap) {
> +		dev_err(dev, "could not get TX device regmap\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = pm4125_irq_init(pm4125, dev);
> +	if (ret) {
> +		dev_err(dev, "IRQ init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	pm4125->sdw_priv[AIF1_PB]->slave_irq = pm4125->virq;
> +	pm4125->sdw_priv[AIF1_CAP]->slave_irq = pm4125->virq;
> +
> +	ret = pm4125_set_micbias_data(pm4125);
> +	if (ret < 0) {
> +		dev_err(dev, "Bad micbias pdata\n");
> +		return ret;
> +	}
> +
> +	ret = snd_soc_register_component(dev, &soc_codec_dev_pm4125,
> +					 pm4125_dais, ARRAY_SIZE(pm4125_dais));
> +	if (ret)
> +		dev_err(dev, "Codec registration failed\n");
> +
> +	return ret;
> +}
> +
> +static void pm4125_unbind(struct device *dev)
> +{
> +	struct pm4125_priv *pm4125 = dev_get_drvdata(dev);
> +
> +	snd_soc_unregister_component(dev);
> +	device_link_remove(dev, pm4125->txdev);
> +	device_link_remove(dev, pm4125->rxdev);
> +	device_link_remove(pm4125->rxdev, pm4125->txdev);
> +	component_unbind_all(dev, pm4125);
> +}
> +
> +static const struct component_master_ops pm4125_comp_ops = {
> +	.bind = pm4125_bind,
> +	.unbind = pm4125_unbind,
> +};
> +
> +static int pm4125_add_slave_components(struct pm4125_priv *pm4125, struct device *dev,
> +				       struct component_match **matchptr)
> +{
> +	struct device_node *np = dev->of_node;
> +
> +	pm4125->rxnode = of_parse_phandle(np, "qcom,rx-device", 0);
> +	if (!pm4125->rxnode)
> +		return dev_err_probe(dev, -ENODEV, "Couldn't parse phandle to qcom,rx-device\n");
> +	component_match_add_release(dev, matchptr, component_release_of, component_compare_of,
> +				    pm4125->rxnode);
> +	of_node_put(pm4125->rxnode);
> +
> +	pm4125->txnode = of_parse_phandle(np, "qcom,tx-device", 0);
> +	if (!pm4125->txnode)
> +		return dev_err_probe(dev, -ENODEV, "Couldn't parse phandle to qcom,tx-device\n");
> +	component_match_add_release(dev, matchptr, component_release_of, component_compare_of,
> +				    pm4125->txnode);
> +	of_node_put(pm4125->txnode);
> +
> +	return 0;
> +}
> +
> +static int pm4125_probe(struct platform_device *pdev)
> +{
> +	struct component_match *match = NULL;
> +	struct device *dev = &pdev->dev;
> +	struct pm4125_priv *pm4125;
> +	struct wcd_mbhc_config *cfg;
> +	int ret;
> +
> +	pm4125 = devm_kzalloc(dev, sizeof(*pm4125), GFP_KERNEL);
> +	if (!pm4125)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, pm4125);
> +
> +	pm4125->supplies[0].supply = "vdd-io";
> +	pm4125->supplies[1].supply = "vdd-cp";
> +	pm4125->supplies[2].supply = "vdd-mic-bias";
> +	pm4125->supplies[3].supply = "vdd-pa-vpos";
> +
> +	ret = devm_regulator_bulk_get(dev, PM4125_MAX_BULK_SUPPLY, pm4125->supplies);

devm_regulator_bulk_get_enable() could certainly be used to save a few 
lines of code after fix the missing regulator_bulk_disable() in the 
error handling of the probe.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get supplies\n");
> +
> +	ret = regulator_bulk_enable(PM4125_MAX_BULK_SUPPLY, pm4125->supplies);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable supplies\n");
> +
> +	pm4125->spmi_regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!pm4125->spmi_regmap)
> +		return -ENXIO;
> +
> +	pm4125_reset(pm4125);
> +
> +	pm4125_dt_parse_micbias_info(dev, pm4125);
> +
> +	cfg = &pm4125->mbhc_cfg;
> +	cfg->mbhc_micbias = MIC_BIAS_2;
> +	cfg->anc_micbias = MIC_BIAS_2;
> +	cfg->v_hs_max = WCD_MBHC_HS_V_MAX;
> +	cfg->num_btn = PM4125_MBHC_MAX_BUTTONS;
> +	cfg->micb_mv = pm4125->micb2_mv;
> +	cfg->linein_th = 5000;
> +	cfg->hs_thr = 1700;
> +	cfg->hph_thr = 50;
> +
> +	wcd_dt_parse_mbhc_data(dev, &pm4125->mbhc_cfg);
> +
> +	ret = pm4125_add_slave_components(pm4125, dev, &match);
> +	if (ret)
> +		return ret;
> +
> +	ret = component_master_add_with_match(dev, &pm4125_comp_ops, match);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_idle(dev);
> +
> +	return 0;
> +}
> +
> +static void pm4125_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pm4125_priv *pm4125 = dev_get_drvdata(dev);
> +
> +	component_master_del(&pdev->dev, &pm4125_comp_ops);
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_dont_use_autosuspend(dev);
> +
> +	regulator_bulk_disable(PM4125_MAX_BULK_SUPPLY, pm4125->supplies);
> +	regulator_bulk_free(PM4125_MAX_BULK_SUPPLY, pm4125->supplies);

Is it correct? (it looks related to devm_regulator_bulk_get())

> +}

CJ

Powered by blists - more mailing lists