Re: [PATCH 2/2] ASoC: codecs: cmx655: Add CML's CMX655D codec

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3003 bytes --]

On Mon, Feb 03, 2025 at 06:01:17PM +0100, Nikola Jelic wrote:

This looks like a new version of:

   https://lore.kernel.org/linux-sound/20250121230903.89808-2-nikola.jelic83@gmail.com/

but it's not identified as v2 (nor does it have a changelog of any
kind, inter-version or otherwise).

> +static int cmx655_i2c_probe(struct i2c_client *client)
> +{
> +	int ret;
> +
> +	ret =
> +	    cmx655_common_register_component(&client->dev,
> +					     devm_regmap_init_i2c(client,
> +								  &cmx655_regmap),
> +					     client->irq);

This would be more legible if you used a temporary variable to store the
regmap.

> +	cmx655_common_unregister_component(&client->dev);
> +	gpiod_set_value_cansleep(cmx655_data->reset_gpio, 1);

Why isn't the GPIO set in the common unregister function?

> +	*ndiv = 0;
> +	*pll_ctrl = 0;
> +	switch (clk_id) {
> +	case (CMX655_SYSCLK_RCLK):
> +	case (CMX655_SYSCLK_LPO):
> +	case (CMX655_SYSCLK_LRCLK):

The brackets around the constants here are weird.

> +	ret = cmx655_get_sys_clk_config(cmx655_dai_data->sys_clk,
> +					primary_mode, srate_setting,
> +					&clk_src, &rdiv, &ndiv, &pll_ctrl);
> +	if (ret < 0) {
> +		dev_err(component->dev,
> +			"Failed to get system clock settings %i\n", ret);
> +	}

We then proceed to use the configuration?

> +	} else {
> +		cmx655_dai_data->best_clk_running = true;
> +	}
> +	if (snd_soc_component_test_bits(component, CMX655_CLKCTRL,

Some blank lines between blocks would help a lot with legibility
throughout the driver.

> +		/* Wait for filters to settle */
> +		if (snd_soc_component_test_bits
> +		    (component, CMX655_RVF, CMX655_VF_DCBLOCK,
> +		     CMX655_VF_DCBLOCK) == 0) {

Please try to follow the kernel coding style more, here just putting one
parameter per line with normal indentation is probably better.

> +static const unsigned int cmx655_rate_vals[] = {
> +	8000, 16000, 32000, 48000
> +};
> +
> +static const struct snd_pcm_hw_constraint_list cmx655_rate_constraint = {
> +	.count = ARRAY_SIZE(cmx655_rate_vals),
> +	.list = cmx655_rate_vals,
> +};
> +
> +static int cmx655_dai_startup(struct snd_pcm_substream *stream,
> +			      struct snd_soc_dai *dai)
> +{
> +	int ret = 0;
> +
> +	ret = snd_pcm_hw_constraint_list(stream->runtime, 0,
> +					 SNDRV_PCM_HW_PARAM_RATE,
> +					 &cmx655_rate_constraint);
> +	return ret;
> +}

If the driver just supports a constant set of constraints why set them
dynamically - set them in the rates for the DAI.

> +	SOC_SINGLE_TLV("ALC Gain Playback Volume", CMX655_ALCGAIN, 0, 12, 0,
> +		       cmx655_alc_gain),

A gain is a volume.

> +	SOC_SINGLE("Companding Switch", CMX655_SAIMUX, 4, 1, 0),
> +	SOC_ENUM("Companding Type Enum", cmx655_companding_enum),

No Enum, the control is for Companding Type.

> +	/* Custom widgets for Mics to get them to turn on before switches */
> +	{.id = snd_soc_dapm_mic,
> +	 .name = "Left Mic",
1
> +	{.id = snd_soc_dapm_mic,
> +	 .name = "Right Mic",

Define a macro for the repated pattern if one is really needed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]