Re: [PATCH v2 2/3] ASoC: codecs: add new pm4125 audio codec driver

["Alexey Klimov" <alexey.klimov@linaro.org>]

On Fri Jul 11, 2025 at 4:15 PM BST, Christophe JAILLET wrote:
> Hi,

Hi Christophe,

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

Yes. Thanks.

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

Ok.


>> +};
>> +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?

It looks like the headers were sorted in pm4125-sdw.c.
However, in pm4125.c I changed the order a bit for the next submission.

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

Thanks! This probably inherited from other codecs.
I reimplemented this for the next version.

>> +{
>> +	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 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.

Thanks! I'll reimplement this.
For me it looks like after staring at devm_regulator_bulk_get_enable()
that regulator_bulk_disable() is not needed.

[..]

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

I'll take a look at that.

Thank you,
Alexey