RE: [PATCH v4 1/2] ASoC: codecs: add support for ES8389

RE: [PATCH v4 1/2] ASoC: codecs: add support for ES8389

[Zhang Yi]

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@everest-semi.com>");
> > +MODULE_LICENSE("GPL");
> > +
> > +
> 
> No need for blank lines at the end.

Ok,I'll fix it

Re: [PATCH v4 1/2] ASoC: codecs: add support for ES8389

[Krzysztof Kozlowski]

On 10/03/2025 04:03, Zhang Yi wrote:
> 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?

Why this list uses different addresses in the first place?

None of the patches had changelog, so you put additional effort on us to
actually check what changed (see submitting patches).


Best regards,
Krzysztof

RE: [PATCH v4 1/2] ASoC: codecs: add support for ES8389

[Zhang Yi]

> > 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?
> 
> Why this list uses different addresses in the first place?

I was trying to show that I have changed my cc list, I apologize if it bothered you

> None of the patches had changelog, so you put additional effort on us to actually check what changed (see submitting patches).

Can you please check out my new v5 patch, I'll add changlog to the following patch