On Tue, Mar 04, 2025 at 02:27:36PM +0800, Zhang Yi wrote:
> +static int es8389_probe(struct snd_soc_component *codec)
> +{
> + int ret = 0;
> + struct es8389_private *es8389 = snd_soc_component_get_drvdata(codec);
> +
> + ret = device_property_read_u8(codec->dev, "everest,mclk-src", &es8389->mclk_src);
> + if (ret != 0) {
> + dev_dbg(codec->dev, "mclk-src return %d", ret);
> + es8389->mclk_src = ES8389_MCLK_SOURCE;
> + }
> +
> + ret = device_property_read_u8(codec->dev, "everest,adc-slot", &es8389->adc_slot);
> + if (ret != 0) {
> + dev_dbg(codec->dev, "adc-slot return %d", ret);
> + es8389->adc_slot = 0x00;
> + }
> +
> + ret = device_property_read_u8(codec->dev, "everest,dac-slot", &es8389->dac_slot);
> + if (ret != 0) {
> + dev_dbg(codec->dev, "dac-slot return %d", ret);
> + es8389->dac_slot = 0x00;
> + }
> +
> + es8389->dmic = device_property_read_bool(codec->dev,
> + "everest,dmic-enabled");
> + dev_dbg(codec->dev, "dmic-enabled %x", es8389->dmic);
> +
> + if (!es8389->adc_slot) {
> + es8389->mclk = devm_clk_get(codec->dev, "mclk");
> + if (IS_ERR(es8389->mclk)) {
> + dev_err(codec->dev, "%s,unable to get mclk\n", __func__);
Syntax is return dev_err_probe. Also, drop __func__.
> + return PTR_ERR(es8389->mclk);
> + }
...
> +static struct snd_soc_component_driver soc_codec_dev_es8389 = {
> + .probe = es8389_probe,
> + .remove = es8389_remove,
> + .suspend = es8389_suspend,
> + .resume = es8389_resume,
> + .set_bias_level = es8389_set_bias_level,
> +
> + .controls = es8389_snd_controls,
> + .num_controls = ARRAY_SIZE(es8389_snd_controls),
> + .dapm_widgets = es8389_dapm_widgets,
> + .num_dapm_widgets = ARRAY_SIZE(es8389_dapm_widgets),
> + .dapm_routes = es8389_dapm_routes,
> + .num_dapm_routes = ARRAY_SIZE(es8389_dapm_routes),
> + .idle_bias_on = 1,
> + .use_pmdown_time = 1,
> +};
> +
> +static struct regmap_config es8389_regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +
> + .max_register = ES8389_MAX_REGISTER,
> +
> + .volatile_reg = es8389_volatile_register,
> + .cache_type = REGCACHE_MAPLE,
> +};
> +
> +#ifdef CONFIG_OF
> +static struct of_device_id es8389_if_dt_ids[] = {
> + { .compatible = "everest,es8389", },
Bindings are before the user (see DT submitting patches).
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, es8389_if_dt_ids);
> +#endif
> +
> +static void es8389_i2c_shutdown(struct i2c_client *i2c)
> +{
> + struct snd_soc_component *component;
> + struct es8389_private *es8389;
> +
> + es8389 = i2c_get_clientdata(i2c);
> + component = es8389->component;
> + dev_dbg(component->dev, "Enter into %s\n", __func__);
Please drop simple probe enter/exit debugs. Tracing is for that in
general.
> +
> + regmap_write(es8389->regmap, ES8389_MASTER_MODE, 0x28);
> + regmap_write(es8389->regmap, ES8389_HPSW, 0x00);
> + regmap_write(es8389->regmap, ES8389_VMID, 0x00);
> + regmap_write(es8389->regmap, ES8389_RESET, 0x00);
> + regmap_write(es8389->regmap, ES8389_CSM_JUMP, 0xCC);
> + usleep_range(500000, 550000);//500MS
> + regmap_write(es8389->regmap, ES8389_CSM_JUMP, 0x00);
> + regmap_write(es8389->regmap, ES8389_ANA_CTL1, 0x08);
> + regmap_write(es8389->regmap, ES8389_ISO_CTL, 0xC1);
> + regmap_write(es8389->regmap, ES8389_PULL_DOWN, 0x00);
> +}
> +
> +static int es8389_i2c_probe(struct i2c_client *i2c_client)
> +{
> + struct es8389_private *es8389;
> + int ret = -1;
> +
> + es8389 = devm_kzalloc(&i2c_client->dev,
> + sizeof(*es8389), GFP_KERNEL);
You wrapping is odd: unnecessary and not matching coding style.
Run checkpatch.pl --strict
> + if (es8389 == NULL)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(i2c_client, es8389);
> + es8389->regmap = devm_regmap_init_i2c(i2c_client, &es8389_regmap);
> + if (IS_ERR(es8389->regmap))
> + return dev_err_probe(&i2c_client->dev, PTR_ERR(es8389->regmap),
> + "regmap_init() failed\n");
> +
> + ret = devm_snd_soc_register_component(&i2c_client->dev,
> + &soc_codec_dev_es8389,
> + &es8389_dai,
> + 1);
> + if (ret < 0) {
> + kfree(es8389);
You have a bug here - double free. You can easily trigger this and see
the result with KASAN.
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static const struct i2c_device_id es8389_i2c_id[] = {
> + {"es8389"},
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, es8389_i2c_id);
> +
> +static struct i2c_driver es8389_i2c_driver = {
> + .driver = {
> + .name = "es8389",
> + .owner = THIS_MODULE,
So you sent us an old code, probably based on downstream, duplicating
issues we already fixed in upstream.
That's really suboptimal choice.
First, you have the issues here which we already fixed. Second, you ask
us to review and find the same problems we already pointed out and
fixed.
Instead, please take the newest reviewed driver and use it as base.
> + .of_match_table = of_match_ptr(es8389_if_dt_ids),
> + },
> + .shutdown = es8389_i2c_shutdown,
> + .probe = es8389_i2c_probe,
> + .id_table = es8389_i2c_id,
> +};
> +module_i2c_driver(es8389_i2c_driver);
> +
> +MODULE_DESCRIPTION("ASoC es8389 driver");
> +MODULE_AUTHOR("Michael Zhang <zhangyi@xxxxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +
> +
No need for blank lines at the end.
Best regards,
Krzysztof
