| 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: <DC1OBXZ7UUTZ.2V473MOKABPKJ@linaro.org>
Date: Thu, 14 Aug 2025 00:04:13 +0100
From: "Alexey Klimov" <alexey.klimov@...aro.org>
To: "Christophe JAILLET" <christophe.jaillet@...adoo.fr>
Cc: "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>, "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
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
Powered by blists - more mailing lists