Re: [PATCH v3 2/2] ASoC: tlv320adcx140: Add the tlv320adcx140 codec driver family

[Dan Murphy <dmurphy@ti.com>]

Richard

On 2/26/20 5:18 AM, Ricard Wanderlof wrote:
> On Wed, 19 Feb 2020, Dan Murphy wrote:
>
>> Add the tlv320adcx140 codec driver family.
>>
> ...
>
>> +
>> +static int adcx140_set_dai_fmt(struct snd_soc_dai *codec_dai,
>> +                              unsigned int fmt)
>> +{
>> +       struct snd_soc_component *component = codec_dai->component;
>> +       struct adcx140_priv *adcx140 =
>> snd_soc_component_get_drvdata(component);
>> +       u8 iface_reg1 = 0;
>> +       u8 iface_reg2 = 0;
>> +
>> +       /* set master/slave audio interface */
>> +       switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>> +       case SND_SOC_DAIFMT_CBM_CFM:
>> +               iface_reg2 |= ADCX140_BCLK_FSYNC_MASTER;
> Although this sets up the codec to be I2S master, there doesn't seem to be
> a way of actually configuring the master clock frequency in master mode,
> as there is no .set_sysclk member in adcx140_dai_ops (and the
> corresponding register field appears never to be touched by the driver).

Thanks for the comments.  The driver was written for the device to be in 
slave mode with auto clock configuration set.

I will look into adding the master mode clock settings.

>
>> +               break;
>> +       case SND_SOC_DAIFMT_CBS_CFS:
>> +               break;
>> +       case SND_SOC_DAIFMT_CBS_CFM:
>> +       case SND_SOC_DAIFMT_CBM_CFS:
>> +       default:
>> +               dev_err(component->dev, "Invalid DAI master/slave interface\n");
>> +               return -EINVAL;
>> +       }
> ...
>
>> +
>> +static int adcx140_codec_probe(struct snd_soc_component *component)
>> +{
>> +       struct adcx140_priv *adcx140 =
>> snd_soc_component_get_drvdata(component);
>> +       int sleep_cfg_val = ADCX140_WAKE_DEV;
>> +       u8 bias_source;
>> +       u8 vref_source;
>> +       int ret;
>> +
>> +       ret = device_property_read_u8(adcx140->dev, "ti,mic-bias-source",
>> +                                     &bias_source);
>> +       if (ret)
>> +               bias_source = ADCX140_MIC_BIAS_VAL_VREF;
>> +
>> +       if (bias_source != ADCX140_MIC_BIAS_VAL_VREF &&
>> +           bias_source != ADCX140_MIC_BIAS_VAL_VREF_1096 &&
>> +           bias_source != ADCX140_MIC_BIAS_VAL_AVDD) {
>> +               dev_err(adcx140->dev, "Mic Bias source value is invalid\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       ret = device_property_read_u8(adcx140->dev, "ti,vref-source",
>> +                                     &vref_source);
>> +       if (ret)
>> +               vref_source = ADCX140_MIC_BIAS_VREF_275V;
>> +
>> +       if (vref_source != ADCX140_MIC_BIAS_VREF_275V &&
>> +           vref_source != ADCX140_MIC_BIAS_VREF_25V &&
>> +           vref_source != ADCX140_MIC_BIAS_VREF_1375V) {
> According to the data sheet, this setting controls the ADC full scale
> setting and has nothing to do with the mic bias voltage, hence using
> MIC_BIAS as part of the macro name is misleaading.
>
These are not macros they are #defines but in any event I can change the 
names as they are all local to the c and h files.
>> +               dev_err(adcx140->dev, "Mic Bias source value is invalid\n");
> Error text does not reflect the actual error, suggest "VREF value is
> invalid".

Ack

Dan