Re: [PATCH 2/4] Add Wolfson Microelectronics WM8766 codec ALSA driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]