Messages in this thread | | | Subject | Re: [PATCH 2/2] ASoC: tas2764: Add the driver for the TAS2764 | From | Dan Murphy <> | Date | Fri, 2 Oct 2020 06:49:07 -0500 |
| |
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
| |