Re: [alsa-devel] [PATCH v3] ASoC: rt1308: Add RT1308 amplifier driver

On 2019-06-24 15:08, derek.fang@realtek.com wrote:

+static int rt1308_reg_init(struct snd_soc_component *component) +{

• struct rt1308_priv *rt1308 = snd_soc_component_get_drvdata(component);
• regmap_multi_reg_write(rt1308->regmap, init_list, RT1308_INIT_REG_LEN);
• return 0;

s/return 0/return regmap_multi_reg_write/ perhaps?

+static int rt1308_classd_event(struct snd_soc_dapm_widget *w,

• struct snd_kcontrol *kcontrol, int event)

+{

• struct snd_soc_component *component =

• snd_soc_dapm_to_component(w->dapm);
  

• switch (event) {
• case SND_SOC_DAPM_POST_PMU:

• msleep(30);
  

• snd_soc_component_update_bits(component, RT1308_POWER_STATUS,
  

• 	RT1308_POW_PDB_REG_BIT, RT1308_POW_PDB_REG_BIT);
  

• msleep(40);
  

• break;
  

• case SND_SOC_DAPM_PRE_PMD:

• snd_soc_component_update_bits(component, RT1308_POWER_STATUS,
  

• 	RT1308_POW_PDB_REG_BIT, 0);
  

• usleep_range(150000, 200000);
  

• break;
  

• default:

• return 0;
  

• }
• return 0;

Redundant return.

+static int rt1308_hw_params(struct snd_pcm_substream *substream,

• struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)

+{

• struct snd_soc_component *component = dai->component;
• struct rt1308_priv *rt1308 = snd_soc_component_get_drvdata(component);
• unsigned int val_len = 0, val_clk, mask_clk;
• int pre_div, bclk_ms, frame_size;
• rt1308->lrck = params_rate(params);
• pre_div = rt1308_get_clk_info(rt1308->sysclk, rt1308->lrck);
• if (pre_div < 0) {

• dev_err(component->dev,
  

• 	"Unsupported clock setting %d\n", rt1308->lrck);
  

• return -EINVAL;
  

• }
• frame_size = snd_soc_params_to_frame_size(params);
• if (frame_size < 0) {

• dev_err(component->dev, "Unsupported frame size: %d\n",
  

• 	frame_size);
  

• return -EINVAL;
  

• }
• bclk_ms = frame_size > 32;
• rt1308->bclk = rt1308->lrck * (32 << bclk_ms);
• dev_dbg(component->dev, "bclk_ms is %d and pre_div is %d for iis %d\n",

• 		bclk_ms, pre_div, dai->id);
  

• dev_dbg(component->dev, "lrck is %dHz and pre_div is %d for iis %d\n",

• 		rt1308->lrck, pre_div, dai->id);
  

• switch (params_width(params)) {
• case 16:

• val_len |= RT1308_I2S_DL_SEL_16B;
  

• break;
  

• case 20:

• val_len |= RT1308_I2S_DL_SEL_20B;
  

• break;
  

• case 24:

• val_len |= RT1308_I2S_DL_SEL_24B;
  

• break;
  

• case 8:

• val_len |= RT1308_I2S_DL_SEL_8B;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• switch (dai->id) {
• case RT1308_AIF1:

• mask_clk = RT1308_DIV_FS_SYS_MASK;
  

• val_clk = pre_div << RT1308_DIV_FS_SYS_SFT;
  

• snd_soc_component_update_bits(component,
  

• 	RT1308_I2S_SET_2, RT1308_I2S_DL_SEL_MASK,
  

• 	val_len);
  

• break;
  

• default:

• dev_err(component->dev, "Invalid dai->id: %d\n", dai->id);
  

• return -EINVAL;
  

• }

So, either id == RT1308_AIF1 -or- func collapses. I'd suggest a refactor: if (dai->id != RT1308_AIF1) // collapse // do the stuff

Moreover, this block (if-statement) should probably be moved to the top of the func. Why bother with any ops if dai->id does not match.

• snd_soc_component_update_bits(component, RT1308_CLK_1,

• mask_clk, val_clk);
  

is mask_clk local even needed? It could be simply inlined..

• return 0;

+}

+static int rt1308_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt) +{

• struct snd_soc_component *component = dai->component;
• struct rt1308_priv *rt1308 = snd_soc_component_get_drvdata(component);
• unsigned int reg_val = 0, reg1_val = 0;
• switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
• case SND_SOC_DAIFMT_CBS_CFS:

• rt1308->master = 0;
  

• break;
  

• default:

• return -EINVAL;
  

• } > +
• switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
• case SND_SOC_DAIFMT_I2S:

• break;
  

• case SND_SOC_DAIFMT_LEFT_J:

• reg_val |= RT1308_I2S_DF_SEL_LEFT;
  

• break;
  

• case SND_SOC_DAIFMT_DSP_A:

• reg_val |= RT1308_I2S_DF_SEL_PCM_A;
  

• break;
  

• case SND_SOC_DAIFMT_DSP_B:

• reg_val |= RT1308_I2S_DF_SEL_PCM_B;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
• case SND_SOC_DAIFMT_NB_NF:

• break;
  

• case SND_SOC_DAIFMT_IB_NF:

• reg1_val |= RT1308_I2S_BCLK_INV;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• switch (dai->id) {
• case RT1308_AIF1:

• snd_soc_component_update_bits(component,
  

• 	RT1308_I2S_SET_1, RT1308_I2S_DF_SEL_MASK,
  

• 	reg_val);
  

• snd_soc_component_update_bits(component,
  

• 	RT1308_I2S_SET_2, RT1308_I2S_BCLK_MASK,
  

• 	reg1_val);
  

• break;
  

• default:

• dev_err(component->dev, "Invalid dai->id: %d\n", dai->id);
  

• return -EINVAL;
  

• }

Same treatment as for rt1308_hw_params could be applied.

+static int rt1308_probe(struct snd_soc_component *component) +{

• struct rt1308_priv *rt1308 = snd_soc_component_get_drvdata(component);
• rt1308->component = component;
• rt1308_reg_init(component);
• return 0;

s/return 0/return rt1308_reg_init/ perhaps?

+#if defined(CONFIG_OF) +static const struct of_device_id rt1308_of_match[] = {

• { .compatible = "realtek,rt1308", },
• {},

+}; +MODULE_DEVICE_TABLE(of, rt1308_of_match); +#endif

+#ifdef CONFIG_ACPI +static struct acpi_device_id rt1308_acpi_match[] = {

• {"10EC1308", 0,},
• {},

+}; +MODULE_DEVICE_TABLE(acpi, rt1308_acpi_match); +#endif

+static const struct i2c_device_id rt1308_i2c_id[] = {

• { "rt1308", 0 },
• { }

+};

Each of these 3 const arrays above has different spacing of their elements. It would look better if same style was chosen for all of em.