Re: [alsa-devel] [PATCH - try2] ASoC: TPA6130A2 amplifier driver

On Fri, 2009-10-09 at 14:55 +0200, Ujfalusi Peter (Nokia-D/Tampere) wrote:

+static int tpa6130a2_i2c_write(int reg, u8 value) +{

•   struct tpa6130a2_data *data;
  

•   int val = 0;
  

•   BUG_ON(tpa6130a2_client == NULL);
  

•   data = i2c_get_clientdata(tpa6130a2_client);
  

•   if (data->power_state) {
  

•           val = i2c_smbus_write_byte_data(tpa6130a2_client, reg, value);
  

•           if (val < 0)
  

•                   dev_err(&tpa6130a2_client->dev, "Write failed\n");
  

•   }
  

•   /* Either powered on or off, we save the context */
  

•   data->regs[reg] = value;
  

•   return val;
  

+}

<snip>

+static void tpa6130a2_channel_enable(u8 channel, int enable) +{

•   struct  tpa6130a2_data *data;
  

•   u8      val;
  

•   BUG_ON(tpa6130a2_client == NULL);
  

•   data = i2c_get_clientdata(tpa6130a2_client);
  

mutex_lock()

•   if (enable) {
  

•           /* Enable channel */
  

•           /* Enable amplifier */
  

•           val = tpa6130a2_read(TPA6130A2_REG_CONTROL);
  

•           val |= channel;
  

•           tpa6130a2_i2c_write(TPA6130A2_REG_CONTROL, val);
  

During the tpa6130a2_i2c_write, you read the data->power_state, that may change it's state meanwhile (preempted). Thus, I suggest you using the mutex to cover all i2c writes. (and all tpa read <-> write cycles, so that things are consistent?)

A call to tpa6130a2_power() -> preemted over somewhere here, you may have i2c accesses to a chip that's not powered up?

•           /* Unmute channel */
  

•           val = tpa6130a2_read(TPA6130A2_REG_VOL_MUTE);
  

•           val &= ~channel;
  

•           tpa6130a2_i2c_write(TPA6130A2_REG_VOL_MUTE, val);
  

•   } else {
  

•           /* Disable channel */
  

•           /* Mute channel */
  

•           val = tpa6130a2_read(TPA6130A2_REG_VOL_MUTE);
  

•           val |= channel;
  

•           tpa6130a2_i2c_write(TPA6130A2_REG_VOL_MUTE, val);
  

•           /* Disable amplifier */
  

•           val = tpa6130a2_read(TPA6130A2_REG_CONTROL);
  

•           val &= ~channel;
  

•           tpa6130a2_i2c_write(TPA6130A2_REG_CONTROL, val);
  

•   }
  

mutex_unlock() ?

+}