[alsa-devel] [PATCH v3 1/2] ASoC: cs35l35: Add support for Cirrus CS35L35 Amplifier

[Charles Keepax]

On Mon, Jan 23, 2017 at 08:02:34AM -0600, Brian Austin wrote:
> On Wed, 21 Dec 2016, Charles Keepax wrote:
> > > +		break;
> > > +	case SND_SOC_DAIFMT_PDM:
> > > +		cs35l35->pdm_mode = true;
> > > +		cs35l35->i2s_mode = false;
> > 
> > Feels a bit redundant to have both of these if they are only ever
> > a logical inversion of each other.
> Certain features are only available in these modes and once TDM is added 
> will make it easier to adjust settings based on mode
<snip>
> > > +			};
> > > +		} else {
> > > +			/* Only certain ratios supported in I2S MASTER Mode */
> > > +			switch (sp_sclks) {
> > > +			case CS35L35_SP_SCLKS_32FS:
> > > +			case CS35L35_SP_SCLKS_64FS:
> > > +			break;
> > > +			default:
> > > +				dev_err(codec->dev, "ratio not supported\n");
> > > +				return -EINVAL;
> > > +			};
> > > +		}
> > > +		ret = regmap_update_bits(cs35l35->regmap,
> > > +			CS35L35_CLK_CTL3, CS35L35_SP_SCLKS_MASK,
> > > +			sp_sclks << CS35L35_SP_SCLKS_SHIFT);
> > > +		if (ret != 0) {
> > > +			dev_err(codec->dev, "Failed to set fsclk %d\n", ret);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +	if (cs35l35->pdm_mode) {
> > > +		regmap_update_bits(cs35l35->regmap, CS35L35_AMP_INP_DRV_CTL,
> > > +			CS35L35_PDM_MODE_MASK, 1 << CS35L35_PDM_MODE_SHIFT);
> > > +	} else {
> > > +		regmap_update_bits(cs35l35->regmap, CS35L35_AMP_INP_DRV_CTL,
> > > +			CS35L35_PDM_MODE_MASK, 0 << CS35L35_PDM_MODE_SHIFT);
> > > +	}
> > 
> > This if could be combined with the one above since pdm_mode ==
> > !i2s_mode.
> 
> Got it thanks

Probably best to ignore that comment given you mention the
addition of more flags and the potential for these to no longer
just be an inversion of each other in the future.

> > > +
> > > +	if (cs35l35->pdata.bst_ipk)
> > > +		regmap_update_bits(cs35l35->regmap, CS35L35_BST_PEAK_I,
> > > +			CS35L35_BST_IPK_MASK,
> > > +			cs35l35->pdata.bst_ipk << CS35L35_BST_IPK_SHIFT);
> > 
> > I believe zero is a valid value for this field, but not the
> > default. Are we happy that the user can never set this value?
> > 
> So here I can just AND in a set high bit that way the check will show true 
> even if 0. Isn't that what we thought would be the easiest?

Yeah seems easiest to me.

> > > +	if (classh->classh_algo_enable) {
> > > +		if (classh->classh_bst_override)
> > > +			regmap_update_bits(cs35l35->regmap,
> > > +				CS35L35_CLASS_H_CTL, CS35L35_CH_BST_OVR_MASK,
> > > +				classh->classh_bst_override << CS35L35_CH_BST_OVR_SHIFT);
> > > +		if (classh->classh_bst_max_limit)
> > > +			regmap_update_bits(cs35l35->regmap,
> > > +				CS35L35_CLASS_H_CTL, CS35L35_CH_BST_LIM_MASK,
> > > +				classh->classh_bst_max_limit << CS35L35_CH_BST_LIM_SHIFT);
> > 
> > This is a single bit, but the default bit is 1, so this code can
> > never change the value of the field.
> This one and the rest apply to previous statement correct?
> 

Yeah would cover this one too.

Thanks,
Charles