Re: [PATCH] ASoC: Codec driver for Texas Instruments tlv320dac33 codec

[Peter Ujfalusi <peter.ujfalusi@nokia.com>]

On Monday 12 October 2009 18:18:00 ext Mark Brown wrote:
> On Mon, Oct 12, 2009 at 02:48:58PM +0300, Peter Ujfalusi wrote:
> > +static struct i2c_client *tlv320dac33_client;
> >
> > +static struct workqueue_struct *dac33_wq;
> 
> I'm surprised to see these done as static variables - is there any great
> reason for doing that?

Hmm, I'm surprised as well, will be fixed.

> 
> > +static int dac33_read(struct snd_soc_codec *codec, unsigned int reg,
> > +		      u8 *value)
> > +{
> > +	struct tlv320dac33_priv *dac33 = codec->private_data;
> > +	int val;
> > +
> > +	*value = reg & 0xff;
> > +
> > +	/* If powered off, return the cached value */
> > +	mutex_lock(&dac33->mutex);
> > +	if (dac33->power_state) {
> > +		val = i2c_smbus_read_byte_data(codec->control_data, value[0]);
> > +		if (val < 0) {
> 
> This is happening a lot - it should probably be factored into the shared
> register cache code, there's a lot of devices which could use something
> like this especially as regulator support is added to more and more
> drivers.

Actually this does not happen that much, since the reg_cache is used for most of 
the read operation, but the write through I2C might be happening more.

> 
> > +			dev_err(codec->dev, "Read failed\n");
> 
> It'd be useful to print val here.

Sure.

> 
> > +	 * data is
> > +	 *   D23..D16 dac33 register offset
> > +	 *   D15..D8  register data MSB
> > +	 *   D7...D0  register data LSB
> > +	 */
> > +	data[0] = reg & 0xff;
> > +	data[1] = (value >> 8) & 0xff;
> > +	data[2] = value & 0xff;
> 
> This looks just like a 16 bit register write - would it make sense to
> just treat the CODEC as having 16 bit registers and deal with the
> autoincrement fun by latching the top bit of each 16 bit register in the
> cache?  That seems to be equivalent to what the current code is doing
> but it's a little more direct.

Most of the registers are 8 bit, there are few exception to this, where 2 
registers holds the MSB and LSB of the value, that's justify the existence of 
this function. I could have used two writes for the registers, but in this way 
the code looks cleaner.

> 
> > +static void dac33_set_power(struct snd_soc_codec *codec, int power)
> > +{
> > +	struct tlv320dac33_priv *dac33 = codec->private_data;
> > +
> > +	if (power) {
> > +		if (dac33->power_gpio >= 0) {
> > +			mutex_lock(&dac33->mutex);
> > +			gpio_set_value(dac33->power_gpio, 1);
> > +			dac33->power_state = 1;
> > +			mutex_unlock(&dac33->mutex);
> > +		}
> > +		dac33_soft_power(codec, 1);
> 
> What exactly is the mutex protecting here?  I'd expect it to cover the
> soft power function too.

It suppose to protect the dac33->power_state, which is checked when accessing to 
the chip (via I2C). Probably there would be no problems, if I don't use the 
mutex for protecting the dac33->power_state...
Basically nothing will brake, but the I2C access would fail and the driver would 
print an error to the log.
I'm not sure how to make sure that we can access to the chip, and at the same 
time do not use extensive mutex lock/unlock for the I2C accesses.
Ideas?

> 
> > +static int dac33_set_nsample(struct snd_kcontrol *kcontrol,
> > +			 struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> > +	struct tlv320dac33_priv *dac33 = codec->private_data;
> > +	int ret = -EINVAL;
> > +
> > +	if (dac33->nsample == ucontrol->value.integer.value[0])
> > +		return 0;
> > +	if (ucontrol->value.integer.value[0] < dac33->nsample_min)
> > +		dac33->nsample = dac33->nsample_min;
> > +	else if (ucontrol->value.integer.value[0] > dac33->nsample_max)
> > +		dac33->nsample = dac33->nsample_max;
> 
> I'd expect these cases to just return the error and not update the
> driver state?

Will do that.

> 
> > +static int dac33_set_nsample_switch(struct snd_kcontrol *kcontrol,
> > +			 struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> > +	struct tlv320dac33_priv *dac33 = codec->private_data;
> > +	int ret = -EINVAL;
> > +
> > +	if (dac33->nsample_switch == ucontrol->value.integer.value[0])
> > +		return 0;
> > +	if (ucontrol->value.integer.value[0] < 0)
> > +		dac33->nsample_switch = 0;
> > +	else if (ucontrol->value.integer.value[0] > 1)
> > +		dac33->nsample_switch = 1;
> > +	else {
> > +		dac33->nsample_switch = ucontrol->value.integer.value[0];
> > +		ret = 0;
> > +	}
> 
> Similarly here.

I'll fix this as well. 

> 
> > +static int dac33_set_bias_level(struct snd_soc_codec *codec,
> > +				enum snd_soc_bias_level level)
> > +{
> > +	switch (level) {
> > +	case SND_SOC_BIAS_ON:
> > +		dac33_soft_power(codec, 1);
> > +		break;
> > +	case SND_SOC_BIAS_PREPARE:
> > +		break;
> > +	case SND_SOC_BIAS_STANDBY:
> > +		dac33_soft_power(codec, 0);
> > +		break;
> > +	case SND_SOC_BIAS_OFF:
> > +		dac33_set_power(codec, 0);
> > +		break;
> > +	}
> 
> Hrm.  I'm finding the set_power/soft_power thing a little abstruse here,
> partly due to the very close naming and partly due to the asymmetry in
> the calls - I can see what they do but it's not entirely obvious.

Hmm, yes you are right, it is kind of a mess...
I can think of the following:
dac33_set_power -> dac33_hard_reset
dac33_soft_power -> dac33_soft_reset

In dac33_set_bias_level only toggle the dac33_soft_reset.
In dac33_soc_suspend/dac33_soc_resume I can use the dac33_hard_reset with 
register restore.

Does it makes it a bit cleaner?

> I'd also expect to see one of these functions doing a register cache restore
> which none of these do.

After dac33_set_power(1) there is a place for restoring the registers, yes.
At the moment the only place where this is happening is in dac33_soc_resume,
there I can add the register restore.

> 
> I think some refactoring would clarify things here but I don't have a
> sufficiently clear understanding of how all the pieces fit together to
> offer any concrete suggestions.
> 
> > +static int dac33_startup(struct snd_pcm_substream *substream,
> > +			   struct snd_soc_dai *dai)
> > +{
> > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +	struct snd_soc_device *socdev = rtd->socdev;
> > +	struct snd_soc_codec *codec = socdev->card->codec;
> > +	struct tlv320dac33_priv *dac33 = codec->private_data;
> > +	int err;
> > +
> > +	/* Save the nsample mode to be used until the stream terminates */
> > +	dac33->nsmaple_cmode = dac33->nsample_switch;
> 
> If you're doing this I'd suggest making the control unwritable while
> playback is active in order to avoid confusion.

Yes, than I don't need the nsample_cmode.

> 
> > +	/* We only need the interrupt in nSample mode */
> > +	if (dac33->nsmaple_cmode) {
> > +		err = request_irq(dac33->irq, dac33_interrupt_handler,
> > +				  IRQF_TRIGGER_RISING | IRQF_DISABLED,
> > +				  codec->name, codec);
> > +		if (err < 0) {
> > +			dev_err(codec->dev, "Could not request IRQ\n");
> > +			return err;
> > +		}
> > +	}
> 
> It'd be better to request the IRQ at startup and refuse to offer nsample
> mode if you don't have it rather than requesting at the start of each
> stream - that's more the expected usage from an IRQ point of view and it
> would improve usability since users wouldn't be able to set the mode if
> it can't be used.

Good point.

> 
> > +	pwr_ctrl = dac33_read_reg_cache(codec, DAC33_PWR_CTRL);
> > +	pwr_ctrl &= ~(OSCPDNB | DACRPDNB | DACLPDNB);
> > +	dac33_write(codec, DAC33_PWR_CTRL, pwr_ctrl);
> 
> This looks like a case for DAPM?

Well, yes. I have tried that. The problem is that tlv320dac33 is really picky on 
the sequence of the register writes. what I mean by 'picky' is that if the 
register accesses are not in certain order, than dac33 would be not operational.
This particular shutdown is in place, because otherwise dac33 would be dead in 
certain cases...
There might be a possibility to find another sequence, which is working, but 
this is really time consuming process.
I'd also like to move these to DAPM domain, but for now, I'd like to leave this 
as it is and fix it later, if it is possible at all.

> 
> > +       switch (cmd) {
> > +       case SNDRV_PCM_TRIGGER_START:
> > +       case SNDRV_PCM_TRIGGER_RESUME:
> > +       case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > +               if (dac33->nsmaple_cmode) {
> > +                       dac33->state = DAC33_PREFILL;
> > +                       queue_work(dac33_wq, &dac33->work);
> > +               }
> 
> I don't see anything that cancels this work or synchronises with it on
> shutdown?

Good point, also the dac33->state is not really protected, which can cause some 
interesting behavior, I think.
I'll look at this as well..

> 
> > +static void dac33_init_chip(struct snd_soc_codec *codec)
> > +{
> > +	/* 44-46: DAC Control Registers */
> > +	/* A : DAC sample rate Fsref/1.5 */
> > +	dac33_write(codec, DAC33_DAC_CTRL_A, DACRATE(1));
> > +	/* B : DAC src=normal, not muted */
> > +	dac33_write(codec, DAC33_DAC_CTRL_B, DACSRCR_RIGHT | DACSRCL_LEFT);
> > +	/* C : (defaults) */
> > +	dac33_write(codec, DAC33_DAC_CTRL_C, 0x00);
> 
> Is there any great reason for setting all of this stuff rather than
> using the hardware defaults?

Yes and no. I can wipe out the unneeded things, but most of the settings here is 
needed to get the dac33 working. If we use the HW defaults, than dac33 would not 
work.
The DAC_CTRL_B register for example controls the digital routing to the DACs.
I will try to find a way to use user controls for most of the settings in a 
future, if it is OK.

> 
> > +/* Bit definitions */
> 
> Pretty much all the bit definitions need to be namespaced - a lot of
> them are very generic and likely to generate clashes.

I'll go through all of them.


Thanks,
Péter