On 2022-01-10 9:45 AM, Joerg Schambacher wrote:
Adds a minimum component driver to run the amplifier in I2S master
mode only from standard audio clocks. Therefore, it only allows
44.1, 88.2, 176.4, 48, 96 and 192ksps with 16, 20, 24 and 32 bits
sample size. Digital volume control and the -6dB and +0.8dB switches
are supported.
Couple nitpicks and suggestions below.
(...)
+static int tas5754m_set_bias_level(struct snd_soc_component *component,
+ enum snd_soc_bias_level level)
+{
+ struct tas5754m_priv *tas5754m =
+ snd_soc_component_get_drvdata(component);
+ int ret;
+
+ switch (level) {
+ case SND_SOC_BIAS_ON:
+ case SND_SOC_BIAS_PREPARE:
+ break;
+
+ case SND_SOC_BIAS_STANDBY:
+ ret = regmap_update_bits(tas5754m->regmap,
+ TAS5754M_POWER, TAS5754M_RQST, 0);
+ if (ret != 0) {
I believe we are dealing here with standard API function i.e. 0 on
success and negative value on error. And thus, 'if (ret)' suffices.
+ dev_err(component->dev,
+ "Failed to remove standby: %d\n", ret);
+ return ret;
+ }
+ break;
+
+ case SND_SOC_BIAS_OFF:
+ ret = regmap_update_bits(tas5754m->regmap,
+ TAS5754M_POWER, TAS5754M_RQST, TAS5754M_RQST);
+ if (ret != 0) {
Ditto. This also goes for every single usage of regmap_xxx() in this file.
+ dev_err(component->dev,
+ "Failed to request standby: %d\n", ret);
+ return ret;
+ }
+ break;
+ }
+
+ return 0;
You could also drop the 'return ret' from the if-statements above -
granting you also ability to drop the brackets - and instead return
'ret' instead of '0' here. Of course that means 'ret' needs to be
initialized appropriately at the top of the function.
+}
+
+int tas5754m_set_clock_tree_master(struct snd_soc_dai *dai,
+ struct snd_pcm_hw_params *params)
Indentation seems off.
+{
+ struct snd_soc_component *component = dai->component;
+ struct tas5754m_priv *tas5754m =
+ snd_soc_component_get_drvdata(component);
+ static const struct reg_sequence pll_settings[] = {
+ { TAS5754M_PLL_COEFF_P, 0x01 }, // P=2
+ { TAS5754M_PLL_COEFF_J, 0x08 }, // J=8
+ { TAS5754M_PLL_COEFF_DL, 0x00 }, // D12-8 = 0
+ { TAS5754M_PLL_COEFF_DH, 0x00 }, // D7-0 = 0
+ { TAS5754M_PLL_COEFF_R, 0x00 }, // R=1
+ };
+ int ret;
+
+ /* disable PLL before any clock tree change */
+ ret = regmap_update_bits(tas5754m->regmap, TAS5754M_PLL_EN,
+ TAS5754M_PLLE, 0);
+ if (ret != 0) {
+ dev_err(component->dev, "Failed to disable PLL: %d\n", ret);
+ return ret;
+ }
+
+ /* set DAC clock source to MCLK */
+ ret = regmap_write(tas5754m->regmap, TAS5754M_DAC_REF, 0x30);
+ if (ret != 0) {
+ dev_err(component->dev, "Failed to set DAC ref\n");
+ return ret;
+ }
+
+ /* run PLL at fixed ratio to MCLK */
+ ret = regmap_multi_reg_write(tas5754m->regmap, pll_settings,
+ ARRAY_SIZE(pll_settings));
+ if (ret != 0) {
+ dev_err(component->dev, "Failed to set PLL ratio\n");
+ return ret;
+ }
+
+ /* set DSP divider to 2 => reg 0x01 */
+ ret = regmap_write(tas5754m->regmap, TAS5754M_DSP_CLKDIV, 1);
+ if (ret != 0) {
+ dev_err(component->dev, "Failed to set DSP divider\n");
+ return ret;
+ }
+ /* set DAC divider to 4 => reg 0x03*/
+ ret = regmap_write(tas5754m->regmap, TAS5754M_DAC_CLKDIV, 3);
+ if (ret != 0) {
+ dev_err(component->dev, "Failed to set OSDACR divider\n");
+ return ret;
+ }
+ /* set OSR divider to 1 */
+ ret = regmap_write(tas5754m->regmap, TAS5754M_OSR_CLKDIV, 0);
+ if (ret != 0) {
+ dev_err(component->dev, "Failed to set OSR divider\n");
+ return ret;
+ }
+ /* set CP divider to 4 => reg 0x03*/
+ ret = regmap_write(tas5754m->regmap, TAS5754M_NCP_CLKDIV, 3);
+ if (ret != 0) {
+ dev_err(component->dev, "Failed to set CP divider\n");
+ return ret;
+ }
+ /* finally enable PLL */
+ ret = regmap_update_bits(tas5754m->regmap, TAS5754M_PLL_EN,
+ TAS5754M_PLLE, 1);
+ if (ret != 0) {
+ dev_err(component->dev, "Failed to enable PLL: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
I'd suggest to keep the logical block organization cohesive. Especially
if there are several of them all located within a single function. Some
of the do/check/error-out blocks above are separated by a newline from
the following ones, and some are not.
Another point is the cohesiveness of the error-message format. Some of
the above print value of 'ret' i.e. carry additional value whereas other
skip that part. Is this intentional?
+
+int tas5754m_set_dai_mode(struct snd_soc_dai *dai)
+{
+ struct snd_soc_component *component = dai->component;
+ struct tas5754m_priv *tas5754m =
+ snd_soc_component_get_drvdata(component);
+ int fmt = tas5754m->fmt;
+
+ /* only I2S MASTER mode implemented */
+ if (((fmt & SND_SOC_DAIFMT_FORMAT_MASK) != SND_SOC_DAIFMT_I2S)) {
Maybe I'm missing something but the most outter pair of brackets is
redundant.
+ dev_err(component->dev,
+ "DAI format not supported (I2S master only)\n");
+ return -EINVAL;
+ }
+ /* TAS5754/6m do not support inverted clocks in MASTER mode */
A newline before the comment would make this more readabile - that's a
new logical block afterall.
+ if (((fmt & SND_SOC_DAIFMT_CLOCK_MASK) != SND_SOC_DAIFMT_NB_NF)) {
Again, I may be missing something, but this looks like outter brackets
are redundant.
+ dev_err(component->dev, "Inverted clocks not supported\n");
+ return -EINVAL;
+ }
+
+ switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
+ case SND_SOC_DAIFMT_CBM_CFM:
+ regmap_update_bits(tas5754m->regmap,
+ TAS5754M_BCLK_LRCLK_CFG,
+ TAS5754M_LRKO | TAS5754M_BCKO,
+ TAS5754M_LRKO | TAS5754M_BCKO);
+ /* reset CLK dividers */
+ regmap_update_bits(tas5754m->regmap,
+ TAS5754M_MASTER_MODE,
+ 0x00,
+ TAS5754M_RLRK | TAS5754M_RBCK);
+ /* ignore all clock error detection but MCLK */
+ regmap_update_bits(tas5754m->regmap,
+ TAS5754M_ERROR_DETECT,
+ TAS5754M_IPLK | TAS5754M_DCAS |
+ TAS5754M_IDCM | TAS5754M_IDSK |
+ TAS5754M_IDBK | TAS5754M_IDFS,
+ TAS5754M_IPLK | TAS5754M_DCAS |
+ TAS5754M_IDCM | TAS5754M_IDSK |
+ TAS5754M_IDBK | TAS5754M_IDFS);
+ break;
+ case SND_SOC_DAIFMT_CBS_CFS:
+ case SND_SOC_DAIFMT_CBM_CFS:
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+int tas5754m_set_dividers_master(struct snd_soc_dai *dai,
+ struct snd_pcm_hw_params *params)
+{
+ struct snd_soc_component *component = dai->component;
+ struct tas5754m_priv *tas5754m =
+ snd_soc_component_get_drvdata(component);
+ unsigned long bclk;
+ unsigned long mclk;
+ int bclk_div;
+ int lrclk_div;
+ int osr;
+ int ret;
+
+ mclk = clk_get_rate(tas5754m->sclk);
+ bclk = tas5754m->sample_len * 2 * params_rate(params);
+ bclk_div = mclk / bclk;
+ lrclk_div = tas5754m->sample_len * 2;
+ osr = mclk / 4 / params_rate(params) / 16;
Is there a specific reason as to why these magic numbers aren't
defines/constants?
+
+ // stop LR / SCLK clocks
Formatting of this comment looks odd. Please align with the recommended one.
(...)
Regards,
Czarek
