From: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>
To: Zhang Yi <zhangyi@everest-semi.com>,
broonie@kernel.org, tiwai@suse.com, linux-sound@vger.kernel.org
Cc: peter.ujfalusi@linux.intel.com, yung-chuan.liao@linux.intel.com,
ranjani.sridharan@linux.intel.com, kai.vehmanen@linux.intel.com
Subject: Re: [PATCH v2 4/6] ASoC: es9356-sdca: Add ES9356 SDCA driver
Date: Wed, 11 Mar 2026 21:14:59 -0700 [thread overview]
Message-ID: <a36ce413-5aff-4117-bc76-95801ce1fd3b@linux.dev> (raw)
In-Reply-To: <20260226073534.3347-5-zhangyi@everest-semi.com>
> +static void es9356_pde_transition_delay(struct es9356_sdw_priv *es9356, unsigned char func,
> + unsigned char entity, unsigned char ps)
> +{
> + unsigned int delay = 1000, val;
> +
> + pm_runtime_mark_last_busy(&es9356->slave->dev);
> +
> + /* waiting for Actual PDE becomes to PS0/PS3 */
> + while (delay) {
> + regmap_read(es9356->regmap,
> + SDW_SDCA_HCTL(func, entity, ES9356_SDCA_CTL_ACTUAL_POWER_STATE, 0), &val);
> + if (val == ps)
> + break;
> +
> + usleep_range(1000, 1500);
this is a delay
> + delay--;
and this isn't a delay, it's a loop/iteration counter.
> + }
> + if (!delay) {
> + dev_warn(&es9356->slave->dev, "%s PDE to %s is NOT ready", __func__, ps?"PS3":"PS0");
it's a constant discussion on this list to figure out if dev_warn() is really useful. I don't think it is.
You also have a number of dev_err() that will likely pollute the logs without giving the user much information.
dev_dbg() is more useful for developers IMHO.
> +static int es9356_sdw_pcm_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
> + struct snd_soc_component *component = dai->component;
> + struct es9356_sdw_priv *es9356 = snd_soc_component_get_drvdata(component);
> + struct sdw_stream_config stream_config = {0};
> + struct sdw_port_config port_config = {0};
> + struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
> + enum sdw_data_direction direction;
> + int ret, num_channels;
> + unsigned int rate;
> +
> + if (!sdw_stream)
> + return -EINVAL;
> +
> + if (!es9356->slave)
> + return -EINVAL;
> +
> + /* SoundWire specific configuration */
> + snd_sdw_params_to_config(substream, params, &stream_config, &port_config);
> +
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> + port_config.num = dai->id;
> + } else {
> + port_config.num = dai->id;
> + }
looks like a useless test to me if both branches do the same thing?
Consider using a static checker for stuff like this...
> +
> + ret = sdw_stream_add_slave(es9356->slave, &stream_config,
> + &port_config, 1, sdw_stream);
> + if (ret)
> + dev_err(dai->dev, "Unable to configure port\n");
> +
> + switch (params_rate(params)) {
> + case 16000:
> + rate = ES9356_SDCA_RATE_16000HZ;
> + break;
> + case 44100:
> + rate = ES9356_SDCA_RATE_44100HZ;
> + break;
> + case 48000:
> + rate = ES9356_SDCA_RATE_48000HZ;
> + break;
> + case 96000:
> + rate = ES9356_SDCA_RATE_96000HZ;
> + break;
> + default:
> + dev_err(component->dev, "%s: Rate %d is not supported\n",
> + __func__, params_rate(params));
> + return -EINVAL;
> + }
> +
> + if (dai->id == ES9356_DMIC)
> + regmap_write(es9356->regmap,
> + SDW_SDCA_HCTL(FUNC_NUM_MIC, ES9356_SDCA_ENT_CS113, ES9356_SDCA_CTL_SAMPLE_FREQ_INDEX, 0), rate);
> + if (dai->id == ES9356_AMP)
> + regmap_write(es9356->regmap,
> + SDW_SDCA_HCTL(FUNC_NUM_AMP, ES9356_SDCA_ENT_CS21, ES9356_SDCA_CTL_SAMPLE_FREQ_INDEX, 0), rate);
> + if ((dai->id == ES9356_JACK_IN) | (dai->id == ES9356_JACK_OUT)) {
> + regmap_write(es9356->regmap,
> + SDW_SDCA_HCTL(FUNC_NUM_UAJ, ES9356_SDCA_ENT_CS41, ES9356_SDCA_CTL_SAMPLE_FREQ_INDEX, 0), rate);
> + regmap_write(es9356->regmap,
> + SDW_SDCA_HCTL(FUNC_NUM_UAJ, ES9356_SDCA_ENT_CS36, ES9356_SDCA_CTL_SAMPLE_FREQ_INDEX, 0), rate);
> + }
> + return ret;
> +}
> +static void es9356_register_init(struct es9356_sdw_priv *es9356)
> +{
> + regmap_write(es9356->regmap, ES9356_STATE, 0x02);
> + regmap_write(es9356->regmap, ES9356_ENDPOINT_MODE, 0x24);
> + regmap_write(es9356->regmap, ES9356_PRE_DIV_CTL, 0x00);
> + regmap_write(es9356->regmap, ES9356_ADC_OSR, 0x18);
> + regmap_write(es9356->regmap, ES9356_ADC_OSRGAIN, 0x13);
> + regmap_write(es9356->regmap, ES9356_DAC_OSR, 0x16);
> + regmap_write(es9356->regmap, ES9356_CLK_CTL, 0x0f);
> + regmap_write(es9356->regmap, ES9356_CSM_RESET, 0x01);
> + regmap_write(es9356->regmap, ES9356_CLK_SEL, 0x30);
> +
> + regmap_write(es9356->regmap, ES9356_DETCLK_CTL, 0x51);
> + regmap_write(es9356->regmap, ES9356_HP_TYPE, 0x10);
> + regmap_write(es9356->regmap, ES9356_MICBIAS_CTL, 0x10);
> + regmap_write(es9356->regmap, ES9356_HPDETECT_CTL, 0x07);
> + regmap_write(es9356->regmap, ES9356_ADC_ANA, 0x30);
> + regmap_write(es9356->regmap, ES9356_PGA_CTL, 0xa8);
> + regmap_write(es9356->regmap, ES9356_ADC_INT, 0xaa);
> + regmap_write(es9356->regmap, ES9356_ADC_LP, 0x19);
> + regmap_write(es9356->regmap, ES9356_VMID1SEL, 0xbc);
> + regmap_write(es9356->regmap, ES9356_VMID_TIME, 0x0b);
> + regmap_write(es9356->regmap, ES9356_STATE_TIME, 0xbb);
> + regmap_write(es9356->regmap, ES9356_HP_SPK_TIME, 0x77);
> + regmap_write(es9356->regmap, ES9356_HP_DETECTTIME, 0xa4);
> + regmap_write(es9356->regmap, ES9356_MICBIAS_SEL, 0x15);
> + regmap_write(es9356->regmap, ES9356_KEY_PRESS_TIME, 0xff);
> + regmap_write(es9356->regmap, ES9356_KEY_RELEASE_TIME, 0xff);
> + regmap_write(es9356->regmap, ES9356_KEY_HOLD_TIME, 0x0f);
> + regmap_write(es9356->regmap, ES9356_BTSEL_REF, 0x00);
> + regmap_write(es9356->regmap, ES9356_KEYD_DETECT, 0x18);
> + regmap_write(es9356->regmap, ES9356_MICBIAS_RES, 0x03);
> + regmap_write(es9356->regmap, ES9356_BUTTON_CHARGE, 0x00);
> + regmap_write(es9356->regmap, ES9356_CALIBRATION_TIME, 0x13);
> + regmap_write(es9356->regmap, ES9356_CALIBRATION_SETTING, 0xf4);
> +
> + regmap_write(es9356->regmap, ES9356_SPK_VOLUME, 0x33);
> + regmap_write(es9356->regmap, ES9356_DAC_VROI, 0x01);
> + regmap_write(es9356->regmap, ES9356_DAC_LP, 0x00);
> + regmap_write(es9356->regmap, ES9356_HP_IBIAS, 0x04);
> + regmap_write(es9356->regmap, ES9356_HP_LP, 0x03);
> + regmap_write(es9356->regmap, ES9356_SPKLDO_CTL, 0x65);
> + regmap_write(es9356->regmap, ES9356_SPKBIAS_COMP, 0x09);
> + regmap_write(es9356->regmap, ES9356_VMID1STL, 0x00);
> + regmap_write(es9356->regmap, ES9356_VMID2STL, 0x00);
> + regmap_write(es9356->regmap, ES9356_VSEL, 0xfc);
> +
> + regmap_write(es9356->regmap, ES9356_IBIASGEN, 0x10);
> + regmap_write(es9356->regmap, ES9356_ADC_AMIC_CTL, 0x0d);
> + regmap_write(es9356->regmap, SDW_SCP_BUS_CLOCK_BASE, 0x01);
this is wrong, all standard codec registers should be handled by the core, not by the codec driver with regmap.
please remove this regmap definition
next prev parent reply other threads:[~2026-03-12 4:44 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-02-26 7:35 [PATCH v2 0/6] Add es9356 focused SoundWire CODEC Zhang Yi
2026-02-26 7:35 ` [PATCH v2 1/6] ASoC: sdw_utils: Add ES9356 support functions Zhang Yi
2026-02-26 7:35 ` [PATCH v2 2/6] ASoC: sdw_utils: add ES9356 in codec_info_list Zhang Yi
2026-02-26 7:35 ` [PATCH v2 3/6] ASoC: sdw_utils: add soc_sdw_es9356 Zhang Yi
2026-03-12 3:55 ` Pierre-Louis Bossart
2026-02-26 7:35 ` [PATCH v2 4/6] ASoC: es9356-sdca: Add ES9356 SDCA driver Zhang Yi
2026-03-12 4:14 ` Pierre-Louis Bossart [this message]
2026-03-12 13:55 ` Mark Brown
2026-02-26 7:35 ` [PATCH v2 5/6] ASoC: Intel: soc-acpi: arl: Add es9356 support Zhang Yi
2026-02-26 7:35 ` [PATCH v2 6/6] ASoC: Intel: sof_sdw: add " Zhang Yi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a36ce413-5aff-4117-bc76-95801ce1fd3b@linux.dev \
--to=pierre-louis.bossart@linux.dev \
--cc=broonie@kernel.org \
--cc=kai.vehmanen@linux.intel.com \
--cc=linux-sound@vger.kernel.org \
--cc=peter.ujfalusi@linux.intel.com \
--cc=ranjani.sridharan@linux.intel.com \
--cc=tiwai@suse.com \
--cc=yung-chuan.liao@linux.intel.com \
--cc=zhangyi@everest-semi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox