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

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

On Thu Jun 26, 2025 at 7:19 AM BST, Krzysztof Kozlowski wrote:
> On Thu, Jun 26, 2025 at 12:50:31AM +0100, Alexey Klimov wrote:
>> +
>> +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) {
>> +		dev_err(dev, "Couldn't parse phandle to qcom,rx-device!\n");
>> +		return -ENODEV;
>> +	}
>> +	of_node_get(pm4125->rxnode);
>
> Where  do you clean this up?

Please don't tell me that this is a bug that being copied from driver
to driver.

I changed it to such flow for the next version since it seems that reference
should be decremented after of_parse_phandle() returns with it incremented:

rxnode = of_parse_phandle();
if (!rxnode)
	return dev_err_probe(...);

component_match_add_release(..., rxnode);
of_node_put(rxnode);


>> +	component_match_add_release(dev, matchptr, component_release_of,
>> +				    component_compare_of, pm4125->rxnode);
>> +
>> +	pm4125->txnode = of_parse_phandle(np, "qcom,tx-device", 0);
>> +	if (!pm4125->txnode) {
>> +		dev_err(dev, "Couldn't parse phandle to qcom,tx-device\n");
>> +			return -ENODEV;
>
> Messed indent. This should be anyway just one line as always - return
> dev_err_probe.

I changed it for the next version as you suggested. Thanks.

>> +	}
>> +	of_node_get(pm4125->txnode);
>
> And this?
>
>> +	component_match_add_release(dev, matchptr, component_release_of,
>> +				    component_compare_of, 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);
>> +
>> +	cfg = &pm4125->mbhc_cfg;
>> +	cfg->swap_gnd_mic = pm4125_swap_gnd_mic;
>> +
>> +	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);
>> +	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) {
>> +		regulator_bulk_free(PM4125_MAX_BULK_SUPPLY, pm4125->supplies);
>
> Double free.

Thanks.

>> +		return dev_err_probe(dev, ret, "Failed to enable supplies\n");
>> +	}
>> +
>> +	pm4125_dt_parse_micbias_info(dev, pm4125);
>> +
>> +	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;

[..]

>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id pm4125_of_match[] = {
>> +	{ .compatible = "qcom,pm4125-codec" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, pm4125_of_match);
>> +#endif
>> +
>> +static struct platform_driver pm4125_codec_driver = {
>> +	.probe = pm4125_probe,
>> +	.remove = pm4125_remove,
>> +	.driver = {
>> +		.name = "pm4125_codec",
>> +		.of_match_table = of_match_ptr(pm4125_of_match),
>
> Drop of_match_ptr and #if. We just removed it (or trying to )
> everywhere, so why re-introducing it...

Will remove it. Thanks.

>> +		.suppress_bind_attrs = true,
>> +	},
>> +};
>> +
>> +module_platform_driver(pm4125_codec_driver);
>> +MODULE_DESCRIPTION("PM4125 audio codec driver");
>> +MODULE_LICENSE("GPL");
>> 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

[...]

>> +const u8 pm4125_reg_access_analog[
>
> No, you cannot have data defined in the header. This is neither style of
> C, nor Linux kernel, nor makes any sense. What if this will be included
> by some other unit? This is some terrible downstream style.
>
> Heh... you actually do include it twice, so you would see all the
> duplicated data for no reason at all.

I pulled in the change that fixes this for the next version.
Thank you.

Best regards,
Alexey