On Wed, Feb 26, 2014 at 11:14:25AM +0200, Jyri Sarha wrote:
> This commit adds a bare bones driver support for TLV320AIC31XX family
> audio codecs. The driver adds basic stereo playback trough headphone
> and speaker outputs and mono capture trough microphone inputs.
Always CC maintainers on patches please.
> +static bool aic31xx_volatile(struct device *dev, unsigned int reg)
> +{
> + if (aic31xx_precious(dev, reg) || aic31xx_real_volatile(dev, reg))
> + return true;
> +
> + switch (reg) {
> + case AIC31XX_PAGECTL: /* regmap implementation requires this */
> + case AIC31XX_RESET: /* always clears after write */
> + return true;
> + }
> + return false;
> +}
This is more complex than you need, just write a standalone volatile
function with some comments in it.
> +static const struct regmap_range_cfg aic31xx_ranges[] = {
> + {
> + .name = "codec-regmap",
Make this meaningful or omit it.
> +#define AIC31XX_NUM_SUPPLIES 6
> +static const char * const aic31xx_supply_names[] = {
Use the define in the array size to help check everything lines up.
> +static const char * const ldac_in_text[] = {
> + "off", "Left Data", "Right Data", "Mono"
> +};
Off not off - be consistent both internally and with the general style.
> +static const struct snd_kcontrol_new aic311x_snd_controls[] = {
> + SOC_DOUBLE_R("SP Driver Playback Switch", AIC31XX_SPLGAIN,
> + AIC31XX_SPRGAIN, 2, 1, 0),
SP?
> + if (!strcmp(w->name, "DAC Left")) {
> + mask = AIC31XX_LDACPWRSTATUS_MASK;
There's no overlap with the enable bits? In general it would seem nicer
to have a switch based on the register bits used for the enable rather
than the tree of string comparisions but it's probably not that big a
deal overall.
> + dev_err(w->codec->dev, "Unknown widget '%s' calling %s/n",
> + w->name, __func__);
> + return -1;
Return real error codes.
> + if (event == SND_SOC_DAPM_POST_PMU)
> + return aic31xx_wait_bits(aic31xx, reg, mask, mask, 5000, 100);
> + else if (event == SND_SOC_DAPM_POST_PMD)
> + return aic31xx_wait_bits(aic31xx, reg, mask, 0, 5000, 100);
switch.
> + SND_SOC_DAPM_DAC_E("DAC Right", "DAC Right Input",
> + AIC31XX_DACSETUP, 6, 0, aic31xx_dapm_power_event,
> + SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
Don't use stream names, use DAPM to route from the AIF to the widgets.
> + switch (params_format(params)) {
> + case SNDRV_PCM_FORMAT_S16_LE:
> + break;
params_width().
> + AIC31XX_IFACE1_DATALEN_MASK,
> + data);
> +
> + return aic31xx_setup_pll(codec, params);
This is going to mean that you have a symmetry constraint I think, if
you try to set up different rates for playback and capture presumably
things will break. Setting symmetric_rates will tell applications about
that.
> + case SND_SOC_DAIFMT_CBS_CFM:
> + case SND_SOC_DAIFMT_CBM_CFS:
> + case SND_SOC_DAIFMT_CBS_CFS:
> + dev_err(codec->dev, "Unsupported DAI master/slave interface\n");
> + return -EINVAL;
> + default:
> + dev_alert(codec->dev, "Invalid DAI master/slave interface\n");
> + return -EINVAL;
Just have a default.
> + for (i = 0; aic31xx_divs[i].mclk != freq; i++)
> + if (i == ARRAY_SIZE(aic31xx_divs)) {
> + dev_err(aic31xx->dev, "%s: Unsupported frequency %d\n",
> + __func__, freq);
> + return -EINVAL;
> + }
Braces around the for () too please.
> +static int aic31xx_set_power(struct snd_soc_codec *codec, int power)
> +{
> + struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
> + int ret;
> +
> + dev_dbg(codec->dev, "## %s: %d -> %d\n", __func__,
> + aic31xx->power, power);
> + if (aic31xx->power == power)
> + return 0;
This looks like you need sensible refcounting somewhere? Implementing
this as the standard PM operations may be sensible.
> + if (gpio_is_valid(aic31xx->pdata.gpio_reset)) {
> + gpio_set_value(aic31xx->pdata.gpio_reset, 1);
> + udelay(100);
> + }
> + snd_soc_write(codec, AIC31XX_RESET, 0x01);
> + udelay(100);
> + regcache_cache_only(aic31xx->regmap, false);
You should be coming out of cache only mode before you issue the reset
write. Is it not sensible to skip that if you have a physical reset
line?
> + snd_soc_write(codec, AIC31XX_RESET, 0x01);
> + regcache_cache_only(aic31xx->regmap, true);
> +
> + if (gpio_is_valid(aic31xx->pdata.gpio_reset))
> + gpio_set_value(aic31xx->pdata.gpio_reset, 0);
Likewise here. Also if you do reset the CODEC then the dance you did
with the regulator notifiers becomes pointless - the goal with that is
to avoid the need to resync the cache if the CODEC wasn't actually reset
by a power cycle but since the CODEC is going to be explicitly reset
before it's powered off there's no benefit.
> + switch (level) {
> + /* full On */
> + case SND_SOC_BIAS_ON:
> + /* All power is driven by DAPM system*/
> + break;
> + /* partial On */
Coding style, please.
Attachment:
signature.asc
Description: Digital signature
