linux-kernel - Re: [PATCH 3/3] sound/soc/lapis: add platform driver

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]

Date:	Mon, 21 Nov 2011 11:45:24 +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 3/3] sound/soc/lapis: add platform driver

On Mon, Nov 21, 2011 at 01:08:52PM +0900, Tomoya MORINAGA wrote:

> +/* =================== I2S CH0 config =================== */
> +#define	I2S_CH0_MCLK		(12288000) /* Master Clock Frequency[Hz] */

This looks like it should be board specific?

> +  #if IOH_I2S_USE_PARAM
> +	#define	I2S_CH0_FS_16000	16000
> +	#define	I2S_CH0_FS_32000	32000
> +	#define	I2S_CH0_FS_48000	48000
> +  #else
> +	#define	I2S_CH0_FS_8000		8000
> +	#define	I2S_CH0_FS_11025	11025
> +	#define	I2S_CH0_FS_22050	22050
> +	#define	I2S_CH0_FS_44100	44100
> +  #endif

What are these ifdefs for?  This should be configured at runtime.

> +/* select master or slave. The value is ioh_mssel_t */
> +#define	I2S_CH0_MSSEL			(ioh_mssel_master)
> +/* select MCLK or MLBCLK into Master Clock. The value is enum ioh_masterclk_t */
> +#define	I2S_CH0_MASTERCLKSEL		(ioh_masterclksel_mclk)

This and most of the following defines also look like compile time
configuration for things that should be runtime configured.  As with
the previous patches in the series the big picture issue here is that
you need to update your code to reflect the practices of modern
mainline Linux code - here the major issue is that you appear to be
doing all your configuration with #defines rather than by using runtime
configuration from the machine driver as is normal for ASoC code.

I got through a bunch more defines but stopped reviewing at some point
as there was so much of this issue.
--
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/