linux-kernel - Re: [PATCH 2/4] Add Wolfson Microelectronics WM8766 codec ALSA 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:	Tue, 17 Apr 2012 17:54:51 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Ondrej Zary <linux@...nbow-software.org>
Cc:	alsa-devel@...a-project.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] Add Wolfson Microelectronics WM8766 codec ALSA driver

On Tue, Apr 17, 2012 at 06:35:32PM +0200, Ondrej Zary wrote:
> On Tuesday 17 April 2012 16:59:29 Mark Brown wrote:

> > No, this should be supported in ASoC - anything adding new code in
> > sound/i2c is *deeply* suspicious.

> I agree that sound/i2c is wrong. I worked on tea575x-tuner before which lives 
> in this directory too - but it's neither an I2C device nor a sound chip...
> There should probably be something like sound/codecs instead that could be 
> used by any sound card drivers.

That's what sound/soc/codecs is - anything with sufficiently split up
hardware to have distinct CODECs ought to be within that framework.
"soc" in this case to a large extent just means "mix and match CODEC and
CPU interface".

> > > +struct snd_wm8766_ops {
> > > +	void (*write)(struct snd_wm8766 *wm, u16 addr, u16 data);
> > > +};

> > You should in general use regmap rather than open coding register I/O
> > for I2C devices.

> regmap seems like an overkill here. It requires the i2c bus to be registered 
> in the kernel i2c subsystem (which is not in the case of ice1712).

Oh, that's fail.  I did notice that patch 4 looked to be open coding
rather generic stuff - I guess this is why.

> > I've not yet looked at the rest of the code but this looks awfully like
> > you've just invented a minimal version of the ASoC interfaces...

> The _set functions are just simple register writes - the caller needs to know 
> what to write there (only _set_if is used by psc724).

You can obviously do stuff as hard coded register write sequences, many
of which will be very short.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)