Re: [PATCH] ASoC: Add initial WM8995 driver

[Liam Girdwood <lrg@slimlogic.co.uk>]

On Wed, 2010-12-22 at 10:26 +0000, Dimitris Papastamos wrote:
> On Tue, 2010-12-21 at 20:38 +0000, Liam Girdwood wrote:
> > On Tue, 2010-12-21 at 17:16 +0000, Dimitris Papastamos wrote:
> > > The WM8995 is a digital audio hub CODEC designed for smartphones.
> > > The current driver supports most of the basic functionality of the
> > > WM8995.
> > > 
> > > Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
> > > ---
> > 
> > > +
> > > +static int wm8995_volatile(unsigned int reg)
> > > +{
> > > +	if (reg >= WM8995_MAX_REGISTER + 1)
> > > +		return 1;
> > > +
> > 
> > return -EINVAL here.
> 
> The logic behind this is that out of bounds registers are considered
> volatile. 

It does look a little odd since out of bounds generally means invalid. 

>  If I change it to -EINVAL, the behaviour will essentially be
> the same because in the soc-cache code we use
> snd_soc_codec_volatile_register() and we only test it for being
> non-zero.  We either need clearer semantics for this function or
> otherwise it should be fine.
> 

Clearer comments are probably better in this case since it does look
wrong, however if you have time please feel free to update the function
semantics. 

> > > +	switch (reg) {
> > > +	case WM8995_SOFTWARE_RESET:
> > > +	case WM8995_DC_SERVO_READBACK_0:
> > > +	case WM8995_INTERRUPT_STATUS_1:
> > > +	case WM8995_INTERRUPT_STATUS_2:
> > > +	case WM8995_INTERRUPT_STATUS_1_MASK:
> > > +	case WM8995_INTERRUPT_STATUS_2_MASK:
> > > +	case WM8995_INTERRUPT_CONTROL:
> > > +	case WM8995_ACCESSORY_DETECT_MODE1:
> > > +	case WM8995_ACCESSORY_DETECT_MODE2:
> > > +	case WM8995_HEADPHONE_DETECT1:
> > > +	case WM8995_HEADPHONE_DETECT2:
> > > +		return 1;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > 
> > > +
> > > +/*
> > > + * R1 (0x01) - Power Management (1)
> > > + */
> > > +#define WM8995_MICB2_ENA                        0x0200	/* MICB2_ENA */
> > > +#define WM8995_MICB2_ENA_MASK                   0x0200	/* MICB2_ENA */
> > > +#define WM8995_MICB2_ENA_SHIFT                       9	/* MICB2_ENA */
> > > +#define WM8995_MICB2_ENA_WIDTH                       1	/* MICB2_ENA */
> > > +#define WM8995_MICB1_ENA                        0x0100	/* MICB1_ENA */
> > > +#define WM8995_MICB1_ENA_MASK                   0x0100	/* MICB1_ENA */
> > > +#define WM8995_MICB1_ENA_SHIFT                       8	/* MICB1_ENA */
> > > +#define WM8995_MICB1_ENA_WIDTH                       1	/* MICB1_ENA */
> > > +#define WM8995_HPOUT2L_ENA                      0x0080	/* HPOUT2L_ENA */
> > > +#define WM8995_HPOUT2L_ENA_MASK                 0x0080	/* HPOUT2L_ENA */
> > > +#define WM8995_HPOUT2L_ENA_SHIFT                     7	/* HPOUT2L_ENA */
> > > +#define WM8995_HPOUT2L_ENA_WIDTH                     1	/* HPOUT2L_ENA */
> > > +#define WM8995_HPOUT2R_ENA                      0x0040	/* HPOUT2R_ENA */
> > > +#define WM8995_HPOUT2R_ENA_MASK                 0x0040	/* HPOUT2R_ENA */
> > > +#define WM8995_HPOUT2R_ENA_SHIFT                     6	/* HPOUT2R_ENA */
> > > +#define WM8995_HPOUT2R_ENA_WIDTH                     1	/* HPOUT2R_ENA */
> > > +#define WM8995_HPOUT1L_ENA                      0x0020	/* HPOUT1L_ENA */
> > > +#define WM8995_HPOUT1L_ENA_MASK                 0x0020	/* HPOUT1L_ENA */
> > > +#define WM8995_HPOUT1L_ENA_SHIFT                     5	/* HPOUT1L_ENA */
> > > +#define WM8995_HPOUT1L_ENA_WIDTH                     1	/* HPOUT1L_ENA */
> > > +#define WM8995_HPOUT1R_ENA                      0x0010	/* HPOUT1R_ENA */
> > > +#define WM8995_HPOUT1R_ENA_MASK                 0x0010	/* HPOUT1R_ENA */
> > > +#define WM8995_HPOUT1R_ENA_SHIFT                     4	/* HPOUT1R_ENA */
> > > +#define WM8995_HPOUT1R_ENA_WIDTH                     1	/* HPOUT1R_ENA */
> > > +#define WM8995_BG_ENA                           0x0001	/* BG_ENA */
> > > +#define WM8995_BG_ENA_MASK                      0x0001	/* BG_ENA */
> > > +#define WM8995_BG_ENA_SHIFT                          0	/* BG_ENA */
> > > +#define WM8995_BG_ENA_WIDTH                          1	/* BG_ENA */
> > > +
> > 
> > It looks like all the register bits have macros but some driver
> > functions are still using magic numbers for bits instead of the macros.
> 
> I've found one call to dc_servo_cmd() using a magic number.  The dapm
> widgets and the kcontrols don't generally use the macros since the lines
> become very long.
> 

There are also some magic numbers in configure_aif_clock,
wm8995_set_dai_fmt, wm8995_hw_params. It's possible there are no macros
defined for these numbers though...

Liam

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk