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

[Eero Nurkkala]

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() ?
> +}
> +