Re: [PATCH 2/2] ASoC: tas2764: Add the driver for the TAS2764

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Mark

Thanks for the review

On 10/1/20 11:25 AM, Mark Brown wrote:
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
OK

+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?

I will fix it up


+	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.

Ah it does do LEFT J and Right J so I will fix this


+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?

I will eliminate this completely and move to i2c_probe

Dan




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux