On Tue, Mar 04, 2025 at 07:47:50PM +0800, Zhang Yi wrote:
> The driver is for codec es8389 of everest which is different from ES8388
> @@ -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.
> + 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...
> + 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.
> + 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?
Attachment:
signature.asc
Description: PGP signature
