Messages in this thread |  | | Date | Thu, 1 Oct 2020 17:25:05 +0100 | From | Mark Brown <> | Subject | Re: [PATCH 2/2] ASoC: tas2764: Add the driver for the TAS2764 |
| |
On Wed, Sep 30, 2020 at 11:38:09AM -0500, Dan Murphy wrote:
This all looks good - a few very minor things below but nothing substantial:
> + default: > + dev_err(tas2764->dev, "Not supported evevt\n"); > + return -EINVAL;
evevt -> event
> +static int tas2764_mute(struct snd_soc_dai *dai, int mute, int direction) > +{ > + struct snd_soc_component *component = dai->component; > + int ret = snd_soc_component_update_bits(component, TAS2764_PWR_CTRL, > + TAS2764_PWR_CTRL_MASK, > + mute ? TAS2764_PWR_CTRL_MUTE : 0); > + > + if (ret < 0) > + return ret;
This looks weird with the ternery operator and extreme indentation - could you please at least split the declaration of ret from the call to make the line length a bit extreme?
> + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { > + case SND_SOC_DAIFMT_I2S: > + case SND_SOC_DAIFMT_DSP_A: > + tdm_rx_start_slot = 1; > + break; > + case SND_SOC_DAIFMT_DSP_B: > + case SND_SOC_DAIFMT_LEFT_J: > + tdm_rx_start_slot = 0; > + break;
I'm not seeing any other handling that distinguishes between the I2S and DSP modes anywhere - I'm guessing this is because the device is really only implementing the DSP modes but because it's mono this is compatible with the I2S modes? It'd be worth having a comment saying this since while that would be OK not distinguishing between modes properly is a common error in drivers so it'd help avoid cut'n'paste issues if someone uses this code as a reference.
> +static int tas2764_register_codec(struct tas2764_priv *tas2764) > +{ > + return devm_snd_soc_register_component(tas2764->dev, > + &soc_component_driver_tas2764, > + tas2764_dai_driver, > + ARRAY_SIZE(tas2764_dai_driver)); > +}
This is a bit odd - can we not just inline the component registration rather than having this function? [unhandled content-type:application/pgp-signature] |  |