mbox series

[v2,0/2] ASoC: Add ak4619 codec support

Message ID 87frtb3x4k.wl-kuninori.morimoto.gx@renesas.com
Headers show
Series ASoC: Add ak4619 codec support | expand

Message

Kuninori Morimoto June 18, 2024, 12:36 a.m. UTC 
Hi Mark

This patch-set adds ak4619 driver.
It was created by Khanh, and I updated/adjusted to upstream.
It was tested on Renesas V4M GrayH

Link: https://lore.kernel.org/r/877ceotnrg.wl-kuninori.morimoto.gx@renesas.com

v1 -> v2
	- fixup git-log style
	- add vendor prefix for Doc
	- remove "ports" from Doc
	- add examples on Doc
	- add missing property on Doc
	- add const on driver
	- use sizeof(*) on driver

Khanh Le (1):
  ASoC: Add ak4619 codec support

Kuninori Morimoto (1):
  ASoC: dt-bindings: ak4619: Add initial DT binding

 .../bindings/sound/asahi-kasei,ak4619.yaml    |  62 ++
 sound/soc/codecs/Kconfig                      |   5 +
 sound/soc/codecs/Makefile                     |   2 +
 sound/soc/codecs/ak4619.c                     | 903 ++++++++++++++++++
 4 files changed, 972 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/asahi-kasei,ak4619.yaml
 create mode 100644 sound/soc/codecs/ak4619.c

Comments

Mark Brown June 18, 2024, 1:56 p.m. UTC | #1 
On Tue, Jun 18, 2024 at 12:36:43AM +0000, Kuninori Morimoto wrote:

Looks mostly good, a few small nits:

> +static int ak4619_put_deemph(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
> +	struct ak4619_priv *ak4619 = snd_soc_component_get_drvdata(component);
> +	int deemph_en = ucontrol->value.integer.value[0];
> +
> +	if (deemph_en > 1)
> +		return -EINVAL;
> +

We should also check for negative values here, those are also out of
range (many other drivers are buggy).

> +	ak4619->deemph_en = deemph_en;
> +	ak4619_set_deemph(component);
> +
> +	return 0;
> +}

This won't return 1 on change so will miss generating events confusing
some UIs, the mixer-test selftest should notice this and the range check
issue.

> +	/* Only slave mode is support */
> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBS_CFS:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Please update for the modern names.

> +static const struct regmap_config ak4619_regmap_cfg = {
> +	.reg_bits		= 8,
> +	.val_bits		= 8,
> +	.max_register		= 0x14,
> +	.reg_defaults		= ak4619_reg_defaults,
> +	.num_reg_defaults	= ARRAY_SIZE(ak4619_reg_defaults),
> +	.cache_type		= REGCACHE_RBTREE,
> +};

Unless there's a specific reason it's probably best to use _MAPLE for
the cache, not that it's likely to make a difference to a driver with
such a small regmap.