linux-kernel - Re: [alsa-devel] [PATCH] ASoC: da7210: Add support for PLL and SRM

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>] [day] [month] [year] [list]

Message-ID: <20120112185257.GB9661@sirena.org.uk>
Date:	Thu, 12 Jan 2012 18:52:57 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Ashish Chavan <ashish.chavan@...tcummins.com>
Cc:	lrg <lrg@...com>, alsa-devel <alsa-devel@...a-project.org>,
	David Dajun Chen <David@...a-project.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	"kuninori.morimoto.gx" <kuninori.morimoto.gx@...esas.com>
Subject: Re: [alsa-devel]  [PATCH] ASoC: da7210: Add support for PLL and SRM

On Thu, Jan 12, 2012 at 05:54:21PM +0530, Ashish Chavan wrote:

> Hi,

Remember this is a changelog...

> +/**
> + * da7210_set_dai_clkdiv: Set codec input clock divider
> + * @param codec_dai	: pointer to codec DAI
> + * @param div_id	: DA7210 has only one clk_div, so div_id is always zero
> + * @param div		: Divider value (configured via control reg MCLK_RANGE)
> + * @return int		: Zero for success, negative error code for error
> + *
> + */
> +static int da7210_set_dai_clkdiv(struct snd_soc_dai *codec_dai,
> +				 int div_id, int div)

Why does the driver need the machine driver to manually configure clock
dividers?

> +	switch (fref) {
> +	case 12000000:
> +		mclk_offset = 0;
> +		break;

> +	switch (fout) {
> +	case 2822400:
> +		fsdm_offset = 0;
> +		break;

> +	if (da7210->master) {
> +		/* In PLL master mode, use Master mode PLL dividers */
> +		pll_div_offset = (mclk_offset * 2 + fsdm_offset) * 3;
> +		pll_div1 = da7210_pll_master_div[pll_div_offset];
> +		pll_div2 = da7210_pll_master_div[pll_div_offset + 1];
> +		pll_div3 = da7210_pll_master_div[pll_div_offset + 2];

This isn't great, you're indexing into a table of divisors using raw
numeric constants in a totally separate part of the code.  Worse, these
constants aren't even directly used but have a calculation applied to 
them apparently because this is really a multi dimensional array.  This
isn't great for either legibility or robustness.

Fix this to remove the use of magic numbers, for example by putting
fout and fref into the table and searching for them.
--
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/