devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* RE: [PATCH v4 1/2] ASoC: codecs: add support for ES8389
@ 2025-03-10  3:03 Zhang Yi
  2025-03-10  7:10 ` Krzysztof Kozlowski
  0 siblings, 1 reply; 11+ messages in thread
From: Zhang Yi @ 2025-03-10  3:03 UTC (permalink / raw)
  To: krzk
  Cc: tiwai, robh, conor+dt, broonie, devicetree, lgirdwood,
	linux-kernel, linux-sound, perex

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

^ permalink raw reply	[flat|nested] 11+ messages in thread
* RE: [PATCH v4 1/2] ASoC: codecs: add support for ES8389
@ 2025-03-10  7:43 Zhang Yi
  0 siblings, 0 replies; 11+ messages in thread
From: Zhang Yi @ 2025-03-10  7:43 UTC (permalink / raw)
  To: krzk
  Cc: tiwai, robh, conor+dt, broonie, devicetree, lgirdwood,
	linux-kernel, linux-sound, perex

> > 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

^ permalink raw reply	[flat|nested] 11+ messages in thread
* RE: [PATCH v4 1/2] ASoC: codecs: add support for ES8389
@ 2025-03-07  8:00 Zhang Yi
  0 siblings, 0 replies; 11+ messages in thread
From: Zhang Yi @ 2025-03-07  8:00 UTC (permalink / raw)
  To: alsa-devel, broonie, devicetree
  Cc: tiwai, amadeuszx.slawinski, robh+dt, krzysztof.kozlowski+dt,
	conor+dt

> > > No, the machine driver should be configuring different TDM slots for
> > > each device - that's the whole point of the API.
> 
> > We are using multiple codecs as a single device.So we can't use set_tdm_slot to configure
> > different slots for multiple codecs under one device.
> 
> What do you mean by using it "as a single device"?  Multiple CODECs on
> the same link is the main use case for set_tdm_slot().

Thanks for the advice.The method you mentioned could work.
I will modify my driver.

^ permalink raw reply	[flat|nested] 11+ messages in thread
* RE: [PATCH v4 1/2] ASoC: codecs: add support for ES8389
@ 2025-03-06  7:23 Zhang Yi
  2025-03-06 13:24 ` Mark Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Zhang Yi @ 2025-03-06  7:23 UTC (permalink / raw)
  To: alsa-devel, broonie, devicetree
  Cc: tiwai, amadeuszx.slawinski, robh+dt, krzysztof.kozlowski+dt,
	conor+dt

> > > set_tdm_slot()
> 
> > We will register multiple codecs inside a single dai_link and differentiate these
> > codecs by of_node. And the adc_slot and the dac_slot may be different, they can 
> > be decided by the user.If we use set_tdm_slot,the adc_slot and the dac_slot will
> > be same.
> 
> No, the machine driver should be configuring different TDM slots for
> each device - that's the whole point of the API.

We are using multiple codecs as a single device.So we can't use set_tdm_slot to configure
different slots for multiple codecs under one device.

> > > > +		ret = clk_prepare_enable(es8389->mclk);
> > > > +		if (ret) {
> > > > +			dev_err(codec->dev, "%s, unable to enable mclk\n", __func__);
> > > > +			return ret;
> > > > +		}
> > > > +	}
> 
> > > Making the use of a MCLK depend on the configuration of a TDM slot for
> > > the ADC seems *very* unusual, what's going on there?
> 
> > Because we are associating multiple codecs under a single dai_link, we will be
> > executing probe and init many times during initialization.But MCLK only needs
> > to be used once.So we decided making the use of a MCLK depend on the configuration
> > of a TDM slot for the ADC 
> 
> No, each device should just get and enable the MCLK itself - the clock
> API does reference counting so there's no problem with this, it's normal
> for a clok to have multiple consumers.

ok,I'll fix it

^ permalink raw reply	[flat|nested] 11+ messages in thread
* RE: [PATCH v4 1/2] ASoC: codecs: add support for ES8389
@ 2025-03-05  9:18 Zhang Yi
  0 siblings, 0 replies; 11+ messages in thread
From: Zhang Yi @ 2025-03-05  9:18 UTC (permalink / raw)
  To: alsa-devel, broonie, devicetree
  Cc: tiwai, amadeuszx.slawinski, robh+dt, krzysztof.kozlowski+dt,
	conor+dt

> > @@ -0,0 +1,961 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * es8389.c  --  ES8389/ES8390 ALSA SoC Audio Codec
> > + *
> > + * Copyright (C) 2025 Everest Semiconductor Co., Ltd
> 
> Please make the entire comment block a C++ one so things look more
> consistent.

I will fix it

> > +	if (es8389->dmic == true) {
> > +		regmap_update_bits(es8389->regmap, ES8389_DMIC_EN, 0xC0, 0xC0);
> > +		regmap_update_bits(es8389->regmap, ES8389_ADC_MODE, 0x03, 0x03);
> > +	} else {
> > +		regmap_update_bits(es8389->regmap, ES8389_DMIC_EN, 0xC0, 0x00);
> > +		regmap_update_bits(es8389->regmap, ES8389_ADC_MODE, 0x03, 0x00);
> > +	}
> 
> We also had the DMIC mux, is that useful as a runtime control when we
> have firmware data telling us if there's a DMIC?  Can both a DMIC and
> analog input be present in the same system?
> 
> It does still look like a lot of these settings might be things that
> should be user controllable...

I'm going to remove es8389->dmic and everest,dmic-enabled and use only DMIC_MUX

^ permalink raw reply	[flat|nested] 11+ messages in thread
* RE: [PATCH v4 1/2] ASoC: codecs: add support for ES8389
@ 2025-03-05  2:56 Zhang Yi
  2025-03-05 13:06 ` Mark Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Zhang Yi @ 2025-03-05  2:56 UTC (permalink / raw)
  To: alsa-devel, broonie, devicetree
  Cc: tiwai, amadeuszx.slawinski, robh+dt, krzysztof.kozlowski+dt,
	conor+dt

> > +	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;
> > +	}
> 
> set_tdm_slot()
> 
> Please don't ignore review comments, people are generally making them
> for a reason and are likely to have the same concerns if issues remain
> unaddressed.  Having to repeat the same comments can get repetitive and
> make people question the value of time spent reviewing.  If you disagree
> with the review comments that's fine but you need to reply and discuss
> your concerns so that the reviewer can understand your decisions.

We will register multiple codecs inside a single dai_link and differentiate these
codecs by of_node. And the adc_slot and the dac_slot may be different, they can 
be decided by the user.If we use set_tdm_slot,the adc_slot and the dac_slot will
be same.

> > +	if (!es8389->adc_slot) {
> > +		es8389->mclk = devm_clk_get(codec->dev, "mclk");
> > +		if (IS_ERR(es8389->mclk))
> > +			return dev_err_probe(codec->dev, PTR_ERR(es8389->mclk),
> > +				"ES8389 is unable to get mclk\n");
> > +
> > +		if (!es8389->mclk)
> > +			dev_err(codec->dev, "%s, assuming static mclk\n", __func__);
> > +
> > +		ret = clk_prepare_enable(es8389->mclk);
> > +		if (ret) {
> > +			dev_err(codec->dev, "%s, unable to enable mclk\n", __func__);
> > +			return ret;
> > +		}
> > +	}
> 
> Making the use of a MCLK depend on the configuration of a TDM slot for
> the ADC seems *very* unusual, what's going on there?

Because we are associating multiple codecs under a single dai_link, we will be
executing probe and init many times during initialization.But MCLK only needs
to be used once.So we decided making the use of a MCLK depend on the configuration
of a TDM slot for the ADC 

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH v4 0/2] ASoC: codecs: add support for ES8389
@ 2025-03-04 11:47 Zhang Yi
  2025-03-04 11:47 ` [PATCH v4 1/2] " Zhang Yi
  0 siblings, 1 reply; 11+ messages in thread
From: Zhang Yi @ 2025-03-04 11:47 UTC (permalink / raw)
  To: alsa-devel, broonie, devicetree
  Cc: tiwai, amadeuszx.slawinski, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, Zhang Yi

The driver is for codec ES8389 of everest-semi.

Zhang Yi (2):
  ASoC: codecs: add support for ES8389
  ASoC: dt-bindings: Add Everest ES8389 audio CODEC

 .../bindings/sound/everest,es8389.yaml        |  83 ++
 sound/soc/codecs/Kconfig                      |   6 +-
 sound/soc/codecs/Makefile                     |   2 +
 sound/soc/codecs/es8389.c                     | 961 ++++++++++++++++++
 sound/soc/codecs/es8389.h                     | 139 +++
 5 files changed, 1190 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/sound/everest,es8389.yaml
 create mode 100644 sound/soc/codecs/es8389.c
 create mode 100644 sound/soc/codecs/es8389.h

-- 
2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-03-10  7:44 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-10  3:03 [PATCH v4 1/2] ASoC: codecs: add support for ES8389 Zhang Yi
2025-03-10  7:10 ` Krzysztof Kozlowski
  -- strict thread matches above, loose matches on Subject: below --
2025-03-10  7:43 Zhang Yi
2025-03-07  8:00 Zhang Yi
2025-03-06  7:23 Zhang Yi
2025-03-06 13:24 ` Mark Brown
2025-03-05  9:18 Zhang Yi
2025-03-05  2:56 Zhang Yi
2025-03-05 13:06 ` Mark Brown
2025-03-04 11:47 [PATCH v4 0/2] " Zhang Yi
2025-03-04 11:47 ` [PATCH v4 1/2] " Zhang Yi
2025-03-04 19:08   ` Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).