Re: [PATCH 2/2] sound: asoc: Adding support for STA529 Audio Codec

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

On Fri, Mar 18, 2011 at 11:42:33AM +0530, rajeev wrote:
> On 3/17/2011 8:47 PM, Mark Brown wrote:
> > On Thu, Mar 17, 2011 at 04:53:36PM +0530, Rajeev Kumar wrote:

> >> +/* reading from register cache: sta529 register value */
> >> +static inline unsigned int
> >> +sta529_read_reg_cache(struct snd_soc_codec *codec, u32 reg)

> > Use the generic register cache code - this looks to be a very 8 bit data
> > 8 bit address map.

> Could you please explain little bit more

See sound/soc/soc-cache.c.  You should use snd_soc_set_cache_io() and
the relevant fields in the CODEC driver rather than open coding all
this.

> >> +	cache = codec->reg_cache;
> >> +	for (i = 0; i < ARRAY_SIZE(sta529_reg); i++) {
> >> +		data[0] = i;
> >> +		data[1] = cache[i];
> >> +		codec->hw_write(codec->control_data, data, NUM_OF_MSG);
> >> +	}

> > You should be using the chip defaults for the most part and therefore
> > should have no reason to write back to hardware in probe()

> Actually the default values at INIT are not working for all possible
> play and record combination. I have kept the STA529 register combinations that
> work well here and write the same to HW. 

> However if you insist, I would try to change them at run-time, rather
> than writing to HW in probe()

The main problem here is that you are unconditionally overwriting the
entire register map with hard coded magic numbers.  This is bad for
documentation and is going to mean you're setting values which it is
more appropriate to customise per system - for example, you will be
updating all volume and routing controls in the device but the settings
that make sense for one system are likely to not work for all systems.

Your register defaults should reflect the default hardware state and you
should update anything you need to change with explicit changes that
show exactly what's being altered rather than blanket writing every
register.

Doing some updates in probe is fine if that is appropriate for those
updates but they should be specific changes where the intended meaning
is clear.

> 
> >> +		.name = "sta529-codec",

> > Drop the -codec.

> I am using stream_name  as "STA529" in  machine driver,so it will create confusion. 

It won't the CODEC name doesn't get used for a stream name.

> >> +#define SPEAR_PCM_RATES		SNDRV_PCM_RATE_48000
> >> +#define SPEAR_PCM_FORMAT	SNDRV_PCM_FMTBIT_S16_LE

> > These are for your CPU not for your CODEC, even if they're the same they
> > should have different names.

> Ok, will be replace with SPEAR_RATES

That's still a problem - these should be STA529_RATES or similar.