Re: [PATCH v1] add tas2780

[Mark Brown <broonie@kernel.org>]

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

On Mon, Jul 04, 2022 at 06:47:59PM +0800, Raphael-Xu wrote:

> 1.update Kconfig and Makefile 2.add tas2780.c and tas2780.h

This looks pretty good, there's some mostly stylistic things below but
they're all fairly minor and you also need to provide documentation for
the DT binding.

>  snd-soc-tas2562-objs := tas2562.o
>  snd-soc-tas2764-objs := tas2764.o
> +snd-soc-tas2780-objs := tas2780.o
>  # Mux

Please preserve these blank lines here.

> +	ret = snd_soc_component_update_bits(component, TAS2780_PWR_CTRL,
> +					    TAS2780_PWR_CTRL_MASK,
> +					    TAS2780_PWR_CTRL_SHUTDOWN);
> +	if (ret < 0) {
> +		pr_err("%s:errCode:0x%0x:power down error\n",
> +			__func__, ret);

dev_err(), and the style used in the log message doesn't really match
the typical kernel style at all.

> +		goto EXIT;

Labels should generally be lower case.

> +static int tas2780_dac_event(struct snd_soc_dapm_widget *w,
> +			     struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component =
> +		snd_soc_dapm_to_component(w->dapm);
> +	struct tas2780_priv *tas2780 =
> +		snd_soc_component_get_drvdata(component);
> +	int ret = 0;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		ret = snd_soc_component_update_bits(component,
> +						TAS2780_PWR_CTRL,
> +						TAS2780_PWR_CTRL_MASK,
> +						TAS2780_PWR_CTRL_MUTE);
> +		break;
> +	case SND_SOC_DAPM_PRE_PMD:
> +		ret = snd_soc_component_update_bits(component,
> +						TAS2780_PWR_CTRL,
> +						TAS2780_PWR_CTRL_MASK,
> +						TAS2780_PWR_CTRL_SHUTDOWN);
> +		break;

This looks like it should perhaps be a mute_stream operation while...

> +static int tas2780_mute(struct snd_soc_dai *dai, int mute, int direction)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct tas2780_priv *tas2780 =
> +		snd_soc_component_get_drvdata(component);
> +	int ret = 0;
> +
> +	if (!mute) {
> +		ret = snd_soc_component_update_bits(component,
> +			TAS2780_CLK_CFG, TAS2780_CLK_CFG_MASK,
> +			TAS2780_CLK_CFG_ENABLE);
> +
> +		if (ret < 0) {
> +			dev_err(tas2780->dev,
> +				"%s: Failed to CLK_CFG_ENABLE\n",
> +				__func__);
> +			goto EXIT;
> +		}
> +	}
> +	ret = snd_soc_component_update_bits(component, TAS2780_PWR_CTRL,
> +		TAS2780_PWR_CTRL_MASK,
> +		mute ? TAS2780_PWR_CTRL_MUTE : 0);

...this is managing clocks which doesn't look like what I'd expect for a
mute operation, that should probably be part of the power management
(either a DAPM supply or in the bias level handling)?

> +
> +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +	case SND_SOC_DAIFMT_I2S:
> +	case SND_SOC_DAIFMT_DSP_A:
> +		iface = TAS2780_TDM_CFG2_SCFG_I2S;
> +		tdm_rx_start_slot = 1;
> +		break;
> +	case SND_SOC_DAIFMT_DSP_B:
> +	case SND_SOC_DAIFMT_LEFT_J:
> +		iface = TAS2780_TDM_CFG2_SCFG_LEFT_J;
> +		tdm_rx_start_slot = 0;
> +		break;

This doesn't seem right - it's using exactly the same configuration for
multiple DAI formats.

> +static bool tas2780_volatile(struct device *dev,
> +	unsigned int reg)
> +{
> +			return true;
> +}

Just don't specify a cache.

> +static int tas2780_parse_dt(struct device *dev, struct tas2780_priv *tas2780)
> +{
> +	int ret = 0;
> +
> +	tas2780->reset_gpio = devm_gpiod_get_optional(tas2780->dev, "reset",
> +		GPIOD_OUT_HIGH);
> +	if (IS_ERR(tas2780->reset_gpio)) {
> +		if (PTR_ERR(tas2780->reset_gpio) == -EPROBE_DEFER) {
> +			tas2780->reset_gpio = NULL;
> +			return -EPROBE_DEFER;
> +		}
> +	}

This has a DT binding but there's no DT binding document, any new DT
bindings need to be documented.

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