Re: [PATCH v1 5/8] mfd: tac5x1x: Add TI TAC5x1x MFD core driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 12/03/2026 19:48, Niranjan H Y wrote:
> This patch adds the core Multi-Function Device (MFD) driver for the Texas

This patchset has multiple basic mistakes, including but not limited to:
1. Not following current kernel style. What does the submitting patches
say about such commit msg?

2. Repeating known old issues which we already fixed. Start from scratch
from recent drivers.


> Instruments TAC5x1x family of audio codecs. The MFD core handles device
> initialization, power management, and common resources for child devices.
> 


> +
> +static int tac5x1x_setup_regulators(struct device *dev,
> +				    struct tac5x1x *tac5x1x)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < TAC5X1X_NUM_SUPPLIES; i++)
> +		tac5x1x->supplies[i].supply = tac5x1x_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(dev, TAC5X1X_NUM_SUPPLIES,
> +				      tac5x1x->supplies);
> +	if (ret) {
> +		dev_err(dev, "Failed to get regulators: %d\n", ret);

Look how all drivers are doing this - dev_err_probe

> +		return ret;
> +	}
> +
> +	/* Regulators managed by PM runtime during probe */
> +	tac5x1x->regulators_enabled = false;
> +
> +	return 0;
> +}
> +
> +static int tac5x1x_probe(struct device *dev, struct tac5x1x *tac5x1x)
> +{
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +
> +	ret = tac5x1x_setup_regulators(dev, tac5x1x);
> +	if (ret)
> +		return ret;
> +
> +	/* Initialize PM runtime before adding child devices */
> +	pm_runtime_set_autosuspend_delay(dev, 3000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_mark_last_busy(dev);
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable regulators for device initialization */
> +	ret = regulator_bulk_enable(TAC5X1X_NUM_SUPPLIES, tac5x1x->supplies);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +	tac5x1x->regulators_enabled = true;
> +
> +	ret = tac5x1x_reset(tac5x1x);
> +	if (ret) {
> +		dev_err(dev, "Failed to reset device\n");
> +		goto err_disable_regulators;
> +	}
> +	tac5x1x_init(tac5x1x);
> +
> +	ret = tac5x1x_parse_dt(tac5x1x, np);
> +	if (ret) {
> +		dev_err(dev, "Failed to parse DT node: %d\n", ret);
> +		goto err_disable_regulators;
> +	}
> +
> +	/* update if vcom property is found */
> +	if (tac5x1x->out2x_vcom_cfg != -1) {
> +		regmap_update_bits(tac5x1x->regmap, TAC5X1X_OUT2CFG0,
> +				   TAC5X1X_OUT2CFG0_VCOM_MASK,
> +				   tac5x1x->out2x_vcom_cfg);
> +	}
> +
> +	dev_dbg(dev, "%s adding mfds\n", __func__);

Drop, useless debug.

> +
> +	/* Add child devices now PM runtime is initialized */
> +	ret = mfd_add_devices(dev, PLATFORM_DEVID_NONE, tac5x1x_mfd_devs,
> +			      ARRAY_SIZE(tac5x1x_mfd_devs), NULL, 0, NULL);
> +	if (ret) {
> +		dev_err(dev, "Failed to add mfd devices\n");
> +		goto err_remove_mfd;
> +	}
> +
> +	return 0;
> +
> +err_remove_mfd:
> +	mfd_remove_devices(dev);
> +err_disable_regulators:
> +	if (tac5x1x->regulators_enabled) {
> +		regulator_bulk_disable(TAC5X1X_NUM_SUPPLIES, tac5x1x->supplies);
> +		tac5x1x->regulators_enabled = false;
> +	}
> +	return ret;
> +}
> +
> +static void tac5x1x_remove(struct tac5x1x *tac5x1x)
> +{
> +	mfd_remove_devices(tac5x1x->dev);
> +	/* Only disable regulators if they are still enabled */
> +	if (tac5x1x->regulators_enabled) {
> +		regulator_bulk_disable(TAC5X1X_NUM_SUPPLIES, tac5x1x->supplies);
> +		tac5x1x->regulators_enabled = false;

Nope, you are not refcounting in parallel to regulator core. Drop.

> +	}

Why aren't you using devm interfaces?

> +}
> +
> +const struct of_device_id tac5x1x_of_match[] = {
> +	{ .compatible = "ti,taa5212", .data = (void *)TAA5212 },
> +	{ .compatible = "ti,taa5412", .data = (void *)TAA5412 },
> +	{ .compatible = "ti,tac5111", .data = (void *)TAC5111 },
> +	{ .compatible = "ti,tac5112", .data = (void *)TAC5112 },
> +	{ .compatible = "ti,tac5211", .data = (void *)TAC5211 },
> +	{ .compatible = "ti,tac5212", .data = (void *)TAC5212 },
> +	{ .compatible = "ti,tac5301", .data = (void *)TAC5301 },
> +	{ .compatible = "ti,tac5311", .data = (void *)TAC5311 },
> +	{ .compatible = "ti,tac5312", .data = (void *)TAC5312 },
> +	{ .compatible = "ti,tac5411", .data = (void *)TAC5411 },
> +	{ .compatible = "ti,tac5412", .data = (void *)TAC5412 },
> +	{ .compatible = "ti,tad5112", .data = (void *)TAD5112 },
> +	{ .compatible = "ti,tad5212", .data = (void *)TAD5212 },

What is all this? Your bindings said 5x1x?

> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, tac5x1x_of_match);
> +
> +static int tac5x1x_i2c_probe(struct i2c_client *i2c)
> +{
> +	enum tac5x1x_type type;
> +	struct tac5x1x *tac5x1x;
> +	struct regmap *regmap;
> +	const struct regmap_config *config = &tac5x1x_regmap;
> +
> +	tac5x1x = devm_kzalloc(&i2c->dev, sizeof(struct tac5x1x),
> +			       GFP_KERNEL);
> +	if (!tac5x1x)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, tac5x1x);
> +	regmap = devm_regmap_init_i2c(i2c, config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +	type = (uintptr_t)i2c_get_match_data(i2c);

No, don't use uintptr_t

> +
> +	tac5x1x->dev = &i2c->dev;
> +	tac5x1x->codec_type = type;
> +	tac5x1x->regmap = regmap;
> +
> +	return tac5x1x_probe(&i2c->dev, tac5x1x);
> +}
> +
> +static void tac5x1x_i2c_remove(struct i2c_client *client)
> +{
> +	tac5x1x_remove(i2c_get_clientdata(client));
> +}
> +
> +static const struct i2c_device_id tac5x1x_id[] = {
> +	{"taa5212", TAA5212},
> +	{"taa5412", TAA5412},
> +	{"tac5111", TAC5111},
> +	{"tac5112", TAC5112},
> +	{"tac5211", TAC5211},
> +	{"tac5212", TAC5212},
> +	{"tac5301", TAC5301},
> +	{"tac5311", TAC5311},
> +	{"tac5312", TAC5312},
> +	{"tac5411", TAC5411},
> +	{"tac5412", TAC5412},
> +	{"tad5112", TAD5112},
> +	{"tad5212", TAD5212},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tac5x1x_id);
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(tac5x1x_pm_ops, tac5x1x_suspend,
> +				 tac5x1x_resume, NULL);
> +
> +static struct i2c_driver tac5x1x_i2c_driver = {
> +	.driver = {
> +		.name = "tac5x1x-core",
> +		.pm = pm_ptr(&tac5x1x_pm_ops),
> +		.of_match_table = of_match_ptr(tac5x1x_of_match),

You have warnings here, drop of_match_ptr and compile test your code

> +	},
> +	.probe = tac5x1x_i2c_probe,
> +	.remove = tac5x1x_i2c_remove,
> +	.id_table = tac5x1x_id,
> +};
> +module_i2c_driver(tac5x1x_i2c_driver);
> +
> +MODULE_DESCRIPTION("Core support for the ASoC tac5x1x codec driver");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Niranjan H Y <niranjan.hy@ti.com>");
> +MODULE_SOFTDEP("pre: tac5x1x_pinctrl");

Drop



Best regards,
Krzysztof