Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the ALC5575

[Krzysztof Kozlowski <krzk@kernel.org>]

On 01/12/2025 11:59, Oder Chiou wrote:
> +
> +static int rt5575_i2c_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct i2c_client *client = context;
> +	struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
> +
> +	regmap_read(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING, val);
> +
> +	return 0;
> +}
> +
> +static int rt5575_i2c_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct i2c_client *client = context;
> +	struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
> +
> +	regmap_write(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING, val);
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config rt5575_regmap = {
> +	.reg_bits = 16,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = 0xfffc,
> +	.readable_reg = rt5575_readable_register,
> +	.reg_read = rt5575_i2c_read,
> +	.reg_write = rt5575_i2c_write,
> +	.use_single_read = true,
> +	.use_single_write = true,
> +};

OF device ID table goes around here - together with I2C.
> +
> +static const struct i2c_device_id rt5575_i2c_id[] = {
> +	{ "rt5575" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, rt5575_i2c_id);
> +
> +static int rt5575_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct rt5575_priv *rt5575;
> +	struct device *dev = &i2c->dev;
> +	int ret, val;
> +
> +#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)

No ifdefs in driver code.

> +	if (!rt5575_spi && of_device_is_compatible(dev->of_node, rt5575_of_match[1].compatible))

No, use driver match data if ever, but this is just wrong. You said it
depends on SPI flash, not SPI interface.

> +		return -EPROBE_DEFER;
> +#endif
> +
> +	rt5575 = devm_kzalloc(dev, sizeof(struct rt5575_priv),
> +				GFP_KERNEL);
> +	if (rt5575 == NULL)

This is not Linux coding style. Open existing drivers. It's everywhere
written (!foo)

> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, rt5575);
> +
> +	rt5575->i2c = i2c;
> +
> +	rt5575->dsp_regmap = devm_regmap_init_i2c(i2c, &rt5575_dsp_regmap);
> +	if (IS_ERR(rt5575->dsp_regmap)) {
> +		ret = PTR_ERR(rt5575->dsp_regmap);
> +		dev_err(dev, "Failed to allocate register map: %d\n", ret);
> +		return ret;
> +	}
> +
> +	rt5575->regmap = devm_regmap_init(dev, NULL, i2c, &rt5575_regmap);
> +	if (IS_ERR(rt5575->regmap)) {
> +		ret = PTR_ERR(rt5575->regmap);
> +		dev_err(dev, "Failed to allocate register map: %d\n", ret);
> +		return ret;
> +	}
> +
> +	regmap_read(rt5575->regmap, RT5575_ID, &val);
> +	if (val != RT5575_DEVICE_ID) {
> +		dev_err(dev, "Device with ID register %08x is not rt5575\n", val);
> +		return -ENODEV;
> +	}
> +
> +	regmap_read(rt5575->regmap, RT5575_ID_1, &val);
> +	if (!val) {
> +		dev_err(dev, "This is not formal version\n");
> +		return -ENODEV;
> +	}
> +
> +	return devm_snd_soc_register_component(dev, &rt5575_soc_component_dev, rt5575_dai,
> +		ARRAY_SIZE(rt5575_dai));
> +}
> +
> +static struct i2c_driver rt5575_i2c_driver = {
> +	.driver = {
> +		.name = "rt5575",
> +		.owner = THIS_MODULE,

Please drop it.

Please run standard kernel tools for static analysis, like coccinelle,
smatch and sparse, and fix reported warnings. Also please check for
warnings when building with W=1 for gcc and clang. Most of these
commands (checks or W=1 build) can build specific targets, like some
directory, to narrow the scope to only your code. The code here looks
like it needs a fix. Feel free to get in touch if the warning is not clear.

> +		.of_match_table = of_match_ptr(rt5575_of_match),

You have warning here. Drop of_match_ptr.

> +	},
> +	.probe = rt5575_i2c_probe,
> +	.id_table = rt5575_i2c_id,
> +};
> +module_i2c_driver(rt5575_i2c_driver)
Best regards,
Krzysztof