Messages in this thread | | | Subject | Re: [PATCH 2/2] ASoC: tlv320adcx140: Add the tlv320adcx140 codec driver family | From | Dan Murphy <> | Date | Tue, 11 Feb 2020 10:24:36 -0600 |
| |
Mark
Thanks for the review
On 2/10/20 7:52 AM, Mark Brown wrote: > On Fri, Feb 07, 2020 at 01:45:33PM -0600, Dan Murphy wrote: > >> + /* interface format */ >> + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { >> + case SND_SOC_DAIFMT_I2S: >> + iface_reg1 |= ADCX140_I2S_MODE_BIT; >> + break; >> + case SND_SOC_DAIFMT_LEFT_J: >> + iface_reg1 |= ADCX140_LEFT_JUST_BIT; >> + break; >> + case SND_SOC_DAIFMT_DSP_A: >> + case SND_SOC_DAIFMT_DSP_B: >> + break; > _DSP_A and _DSP_B are two different format so I'd expect the device to > be configured differently for them, or for only one to be supported. Ack. I will need to adjust the TX offset for these. I will do add a prepare call back and set the offset there. These can fall through since TDM is default. > >> +static int adcx140_mute(struct snd_soc_dai *codec_dai, int mute) >> +{ >> + struct snd_soc_component *component = codec_dai->component; >> + int config_reg; >> + int mic_enable; >> + int i; >> + >> + /* There is not a single register to mute. Each enabled path has to be >> + * muted individually. Read which path is enabled and mute it. >> + */ >> + snd_soc_component_read(component, ADCX140_IN_CH_EN, &mic_enable); >> + if (!mic_enable) >> + return 0; > You could also just offer this control to userspace, it's not > *essential* to have this operation though it can help with glitching > during stream startup. > >> + >> + for (i = 0; i < ADCX140_MAX_CHANNELS; i++) { >> + config_reg = ADCX140_CH8_CFG2 - (5 * i); >> + if (!(mic_enable & BIT(i))) >> + continue; >> + >> + if (mute) >> + snd_soc_component_write(component, config_reg, 0); >> + } > How does the unmute work? This is unmuted through volume control. I can remove this as the device is muted when the volume register is set to 0. That will force the user to have to set a volume. > >> + internal_reg = device_property_present(adcx140->dev, >> + "ti,use-internal-areg"); >> + >> + if (internal_reg) >> + sleep_cfg_val |= ADCX140_AREG_INTERNAL; > Does this actually need a specific property or could you support the > regulator API and then use regulator_get_optional() to figure out if an > external AVDD is attached?
Yes we could set internal AREG bit if no supply is given.
> >> +static int adcx140_codec_probe(struct snd_soc_component *component) >> +{ >> + struct adcx140_priv *adcx140 = snd_soc_component_get_drvdata(component); >> + >> + return adc5410_init(adcx140); >> +} > Does the separate init function buy us anything?
No that is an artifact of my development I can move everything to codec_probe
Dan
| |