Re: [PATCH 1/1] aat2870: Adding backlight, regulator and mfd driver

Samuel Ortiz
2011-06-20T10:29:49Z

Hi Jin,

On Mon, May 30, 2011 at 05:17:32PM +0900, Jin Park wrote:
> Adding backlight, regulator and mfd driver for AnalogicTech AAT2870.
Before reviewing this patch, could you please do the following:

1) Split it into 3 actual patches: the MFD one, the regulator one and the
backlight one.
2) Add the relevant maintainers (See MAINTAINERS) if you want to get a proper
regulator and backlight driver review.

Now, some MFD specific comments:

> +static int aat2870_read(struct aat2870_data *aat2870, u8 addr, u8 *val)
> +{
> + struct i2c_client *client = aat2870->client;
> + int ret = 0;
> +
> + if (addr >= AAT2870_REG_NUM) {
> + dev_err(aat2870->dev, "Invalid address, 0x%02x\n", addr);
> + return -EINVAL;
> + }
> +
> + mutex_lock(&aat2870->io_lock);
> +
> + if (!aat2870->reg_cache[addr].readable) {
> + *val = aat2870->reg_cache[addr].value;
> + goto out_unlock;
> + }
> +
> + ret = i2c_master_send(client, &addr, 1);
> + if (ret < 0)
> + goto out_unlock;
> + if (ret != 1) {
> + ret = -EIO;
> + goto out_unlock;
> + }
> +
> + ret = i2c_master_recv(client, val, 1);
> + if (ret < 0)
> + goto out_unlock;
> + if (ret != 1) {
> + ret = -EIO;
> + goto out_unlock;
> + }
> +
> + ret = 0;
> +
> +out_unlock:
> + mutex_unlock(&aat2870->io_lock);
> + dev_dbg(&client->dev, "read: addr=0x%02x, val=0x%02x\n", addr, *val);
> +
> + return ret;
> +}
> +
> +static int aat2870_write(struct aat2870_data *aat2870, u8 addr, u8 val)
> +{
> + struct i2c_client *client = aat2870->client;
> + u8 msg[2];
> + int ret = 0;
> +
> + if (addr >= AAT2870_REG_NUM) {
> + dev_err(aat2870->dev, "Invalid address, 0x%02x\n", addr);
> + return -EINVAL;
> + }
> +
> + if (!aat2870->reg_cache[addr].writeable) {
> + dev_err(aat2870->dev, "Address 0x%02x is not writeable\n",
> + addr);
> + return -EINVAL;
> + }
> +
> + mutex_lock(&aat2870->io_lock);
> +
> + msg[0] = addr;
> + msg[1] = val;
> + ret = i2c_master_send(client, msg, 2);
> + if (ret < 0)
> + goto out_unlock;
> + if (ret != 2) {
> + ret = -EIO;
> + goto out_unlock;
> + }
> +
> + aat2870->reg_cache[addr].value = val;
> + ret = 0;
> +
> +out_unlock:
> + mutex_unlock(&aat2870->io_lock);
> + dev_dbg(&client->dev, "write: addr=0x%02x, val=0x%02x\n", addr, val);
> +
> + return ret;
> +}
> +
> +static int aat2870_update_bits(struct aat2870_data *aat2870, u8 addr,
> + u8 mask, u8 val)
> +{
> + int change;
> + u8 old_val, new_val;
> + int ret;
> +
> + ret = aat2870->read(aat2870, addr, &old_val);
> + if (ret)
> + return ret;
> +
> + new_val = (old_val & ~mask) | val;
> + change = old_val != new_val;
> + if (change)
> + ret = aat2870->write(aat2870, addr, new_val);
> +
> + dev_dbg(&aat2870->client->dev,
> + "update_bits: change=%d, addr=0x%02x, old=0x%02x, new=0x%02x\n",
> + change, addr, old_val, new_val);
> +
> + return ret;
> +}
You also need to take the io_lock mutex here, to prevent someone else to write
a different value to your register between your read and write.
So what you typically want is an unlocked version of aat2870_[read|write]
(let's say we call it __aat2870_[read|write]). Then your aat2870_[read|write]
become wrappers around the __aat2870_[read|write] with the lock taken. And
your update_bits routine can use __aat2870_[read|write] with the lock taken
from the beginning.

The rest looks pretty fine to me.

Cheers,
Samuel.

--
Intel Open Source Technology Centre