| 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: <20250310030357.120390-1-zhangyi@everest-semi.com>
Date: Mon, 10 Mar 2025 11:03:57 +0800
From: Zhang Yi <zhangyi@...rest-semi.com>
To: krzk@...nel.org
Cc: tiwai@...e.com,
robh@...nel.org,
conor+dt@...nel.org,
broonie@...nel.org,
devicetree@...r.kernel.org,
lgirdwood@...il.com,
linux-kernel@...r.kernel.org,
linux-sound@...r.kernel.org,
perex@...ex.cz
Subject: RE: [PATCH v4 1/2] ASoC: codecs: add support for ES8389
I apologize for not responding to this review comment.
But I did view these review comments and fixed the error.
In the meantime I will modify my cc list, do I need to resend a new version
of the patch to correct my error in my cc list?
> > +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__.
Ok,I'll fix it
> > +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).
Ok,I'll fix it
> > + { }
> > +};
> > +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.
I'll drop these.
> > +
> > + 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.
Ok,I'll fix it
>> + 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.
Ok,I'll fix it
> > + 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.
Ok,I'll fix it
> > + .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@...rest-semi.com>");
> > +MODULE_LICENSE("GPL");
> > +
> > +
>
> No need for blank lines at the end.
Ok,I'll fix it
Powered by blists - more mailing lists