| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160614155938.GS2282@sirena.org.uk>
Date: Tue, 14 Jun 2016 16:59:38 +0100
From: Mark Brown <broonie@...nel.org>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc: alsa-devel@...a-project.org, Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Liam Girdwood <lgirdwood@...il.com>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, kwestfie@...eaurora.org,
plai@...eaurora.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v3 2/2] ASoC: msm8916: Add msm8916-wcd codec driver
On Fri, Jun 10, 2016 at 07:18:45PM +0100, Srinivas Kandagatla wrote:
> +config SND_SOC_MSM8916_WCD
> + tristate "Qualcomm MSM8916 WCD"
> + depends on SPMI && MFD_SYSCON
> +
Normally users select MFD_SYSCON.
> @@ -208,7 +209,6 @@ snd-soc-wm9705-objs := wm9705.o
> snd-soc-wm9712-objs := wm9712.o
> snd-soc-wm9713-objs := wm9713.o
> snd-soc-wm-hubs-objs := wm_hubs.o
> -
> # Amp
> snd-soc-max9877-objs := max9877.o
> snd-soc-tpa6130a2-objs := tpa6130a2.o
Spurious whitespace change.
> +#include "msm8916-wcd-registers.h"
> +#include "msm8916-wcd.h"
> +#include "dt-bindings/sound/msm8916-wcd.h"
What's in here? There weren't any constants in the bindings.
> +struct msm8916_wcd_chip {
> + struct regmap *analog_map;
> + struct regmap *digital_map;
> + unsigned int analog_offset;
> + u16 pmic_rev;
> + u16 codec_version;
Why is this one device and not two devices? The description indicated
that this was two separate bits of silicon.
> +static int msm8916_wcd_write(struct snd_soc_codec *codec, unsigned int reg,
> + unsigned int val)
> +{
> + int ret = -EINVAL;
> + struct msm8916_wcd_chip *chip = dev_get_drvdata(codec->dev);
> + u8 *cache = codec->reg_cache;
> +
> + if (!msm8916_wcd_reg_readonly[reg])
> + cache[reg] = val;
Why is the driver open coding a cache? Don't do that!
> + case SND_SOC_DAPM_POST_PMU:
> + if (w->shift == 5)
> + snd_soc_update_bits(codec, LPASS_CDC_RX1_B6_CTL,
> + RXn_B6_CTL_MUTE_MASK, 0);
> + else if (w->shift == 4)
> + snd_soc_update_bits(codec, LPASS_CDC_RX2_B6_CTL,
> + RXn_B6_CTL_MUTE_MASK, 0);
Switch statement.
> + widget_name = kstrndup(w->name, 15, GFP_KERNEL);
> + if (!widget_name)
> + return -ENOMEM;
> + temp = widget_name;
> +
> + dec_name = strsep(&widget_name, " ");
> + widget_name = temp;
> + if (!dec_name) {
> + dev_err(codec->dev, "Invalid decimator = %s\n", w->name);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + dec_num = strpbrk(dec_name, "12");
> + if (dec_num == NULL) {
> + dev_err(codec->dev, "Invalid Decimator\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = kstrtouint(dec_num, 10, &decimator);
> + if (ret < 0) {
> + dev_err(codec->dev, "Invalid decimator = %s\n", dec_name);
> + ret = -EINVAL;
> + goto out;
> + }
I'm not terribly clear what this is doing, it probably needs some
comments explaining what's going on at the very least.
> + /*RX stuff */
> + SND_SOC_DAPM_AIF_IN("I2S RX1", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0),
> + SND_SOC_DAPM_AIF_IN("I2S RX2", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0),
> + SND_SOC_DAPM_AIF_IN("I2S RX3", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0),
Use DAPM routes to connect the widgets in, don't name the DAI in the
widget.
> + mclk_rate = clk_get_rate(msm8916_wcd->mclk);
> +
> + if (mclk_rate == 12288000)
> + snd_soc_update_bits(codec, LPASS_CDC_TOP_CTL,
> + TOP_CTL_DIG_MCLK_FREQ_MASK,
> + TOP_CTL_DIG_MCLK_FREQ_F_12_288MHZ);
> +
> + else if (mclk_rate == 9600000)
> + snd_soc_update_bits(codec, LPASS_CDC_TOP_CTL,
> + TOP_CTL_DIG_MCLK_FREQ_MASK,
> + TOP_CTL_DIG_MCLK_FREQ_F_9_6MHZ);
Switch statement, and this should also handle unexpected rates.
> +static int msm8916_wcd_codec_probe(struct snd_soc_codec *codec)
> +{
> + struct msm8916_wcd_chip *chip = dev_get_drvdata(codec->dev);
> + int err, reg;
> +
> + err = regulator_enable(chip->vddio);
> + if (err < 0) {
> + dev_err(codec->dev,
> + "failed to enable VDDIO regulator (%d)\n", err);
> + return err;
> + }
> +
> + err = regulator_enable(chip->vdd_tx_rx);
> + if (err < 0) {
> + dev_err(codec->dev,
> + "failed to enable VDD_TX_RX regulator (%d)\n", err);
> + regulator_disable(chip->vddio);
> + return err;
> + }
Why is this not using regulator_bulk_enable()? I'd also expect to see
most if not all of this initial setup stuff in the main device probe.
> + if (TOMBAK_IS_1_0(chip->pmic_rev)) {
> + for (reg = 0; reg < ARRAY_SIZE(wcd_reg_defaults); reg++)
> + snd_soc_write(codec, wcd_reg_defaults[reg].reg,
> + wcd_reg_defaults[reg].val);
> + } else {
> + for (reg = 0; reg < ARRAY_SIZE(wcd_reg_defaults_2_0); reg++)
> + snd_soc_write(codec, wcd_reg_defaults_2_0[reg].reg,
> + wcd_reg_defaults_2_0[reg].val);
> + }
Please reset the chip properly.
> + ret = clk_prepare_enable(chip->mclk);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable mclk %d\n", ret);
> + return ret;
> + }
Runtime PM?
> +static const struct of_device_id msm8916_wcd_match_table[] = {
> + {.compatible = "qcom,msm8916-pmic-wcd-codec"},
> + {}
> +};
We were peering inside the parent for the register map, why does this
appear in the device tree as a separate device? Both the patch
description and that code suggest that it doesn't really have a separate
existence independent of the broader IP.
Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)
Powered by blists - more mailing lists