Re: [PATCH 1/4 v2] ASoC: add a WM8978 codec driver

Liam Girdwood
2010-01-22T17:39:20Z

On Fri, 2010-01-22 at 17:27 +0100, Guennadi Liakhovetski wrote:
> The WM8978 codec from Wolfson Microelectronics is very similar to wm8974, but
> is stereo and also has some differences in pin configuration and internal
> signal routing. This driver is based on wm8974 and takes the differences into
> account.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>

Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>

>
> In short - addressed all comments (thanks a lot again!) - except one... I
> still would not like to use symbolic names for register bits. Instead I
> thoroughly commented all multi-bit register manipulations. Reasons:
>
> 1. thanks for the header, Mark, but unfortunately it contains errors
> (duplicate register names, duplicate and wrong bitfield names)
> 2. the header also uses spaces for indentation, which would have to be
> manually replaced with TABs
> 3. I am still not convinced, that macro names like WM8978_WL or
> WM8978_DLRSWAP or WM8978_MS or... better describe the meaning of the
> bitfield than respective comment in the source. Here's an example of a
> comment from this patch:
>
> /* bit 3: enable bias, bit 2: enable I/O tie off buffer */
> power1 |= 0xc;
>
> Where bit-field names do make sense, IMHO, is in drivers, where the
> same bitfields have to be written / evaluated multiple times at
> different locations. Than indeed giving those bits symbolic names helps
> finding them. So, I'd like to request a permission to preserve the
> present style of the driver.
>

I think we will have to agree to disagree on this one, but it can be
fixed at a later stage.

Liam