linux-kernel - Re: [PATCH v2] sound/soc/codecs: add LAPIS Semiconductor ML26124

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111125142858.GE5315@opensource.wolfsonmicro.com>
Date:	Fri, 25 Nov 2011 14:28:58 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Tomoya MORINAGA <tomoya.rohm@...il.com>
Cc:	Liam Girdwood <lrg@...com>, Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.de>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Dimitris Papastamos <dp@...nsource.wolfsonmicro.com>,
	Mike Frysinger <vapier@...too.org>,
	Daniel Mack <zonque@...il.com>, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org, qi.wang@...el.com,
	yong.y.wang@...el.com, joel.clark@...el.com, kok.howg.ewe@...el.com
Subject: Re: [PATCH v2] sound/soc/codecs: add LAPIS Semiconductor ML26124

On Fri, Nov 25, 2011 at 09:39:03PM +0900, Tomoya MORINAGA wrote:

This is better than the previous version but the major issue is that the
driver isn't really fitting in with the subsystem - it's still relying
on hard coded register write sequences for example.

> ML26124-01HB/ML26124-02GD is 16bit monaural audio CODEC which has high
> resistance to voltage noise. On chip regulator realizes power supply rejection
> ratio (PSRR) be over 90dB so more than 50dB is improved than ever. ML26124-01HB/

You should fix the word wrapping in your commit message to be well
within 80 columns.

> +struct ml26124_priv {
> +	enum snd_soc_control_type control_type;

Since the device only supports I2C you should just unconditionally use
I2C.  You should also use regmap for register access.

> +static const struct snd_kcontrol_new ml26124_snd_controls[] = {
> +	SOC_SINGLE_TLV("Capture Digital Volume", ML26124_RECORD_DIG_VOL, 0, 0xff, 1, rec_play_digi_vol),
> +	SOC_SINGLE_TLV("Playback Digital Volume", ML26124_PLBAK_DIG_VOL, 0, 0xff, 1, rec_play_digi_vol),

> +	SOC_SINGLE("Digital Volume Switch", ML26124_FILTER_EN, 4, 1, 0),

This needs either a Playback or Capture in there to match up with the
above unless it genuinely does mute both which would be really weird.

> +	SND_SOC_DAPM_VMID("VMID"),

Remove this unless it's actually used in your routing.

> +	SND_SOC_DAPM_MICBIAS("MICBIAS", ML26124_PW_REF_PW_MNG, 0, 0),

This should be a supply widget, MICBIAS widgets are legacy.

> +	SND_SOC_DAPM_MIXER("Output Mixer", ML26124_PW_SPAMP_PW_MNG, 0x1f, 0, 
> +			   &ml26124_output_mixer_controls[0],
> +			   ARRAY_SIZE(ml26124_output_mixer_controls)),

Do you *really* have a shift of 0x1f for the enable bit?

> +static int ml26124_snd_card_codec_set(int number, struct ml26124_priv *priv,
> +				      struct snd_soc_codec *codec)
> +{
> +	unsigned char data;
> +	unsigned int rate = priv->rate;
> +	unsigned int channels = priv->ch;
> +
> +	data = snd_soc_read(codec, ML26124_PW_MICBIAS_VOL);
> +	snd_soc_update_bits(codec, ML26124_SW_RST, 0x01, 1);	/* soft reset assert */
> +	snd_soc_update_bits(codec, ML26124_SW_RST, 0x01, 0);	/* soft reset negate */
> +
> +	snd_soc_update_bits(codec, 0x0c, 0x1f, 0);	/* Stop clock  */

The same issue as with your previous submission applies to this function
- you shouldn't have hard coded register write sequences in your code,
the driver should use the facilities of the subsystem to allow the user
to configure it at runtime.  Given how big an issue this is I've stopped
reading at this point.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/