Messages in this thread | | | Date | Tue, 15 May 2012 20:43:30 +0200 | From | Johan Hovold <> | Subject | Re: [PATCH 1/1 v1] leds: Add LED driver for lm3556 chip |
| |
On Mon, Apr 09, 2012 at 11:00:55AM +0900, Geon Si Jeong wrote: > It is a simple driver for LM3556 Chip(Texas Instruments) > LM3556 : > The LM3556 is a 4 MHz fixed-frequency synchronous boost > converter plus 1.5A constant current driver for a high-current white LED. > Datasheet: www.national.com/ds/LM/LM3556.pdf > > Tested on OMAP4430 > > Signed-off-by: Geon Si Jeong <gshark.jeong@gmail.com>
[...]
> +/* i2c access*/ > +static int lm3556_read_reg(struct i2c_client *client, u8 reg, u8 * val) > +{ > + int ret; > + > + ret = i2c_smbus_read_byte_data(client, reg); > + if (ret < 0) { > + dev_err(&client->dev, "i2c reading fail at 0x%02x error %d\n", > + reg, ret); > + return ret; > + } > + *val = ret & 0xff; > + return ret; > +} > + > +static int lm3556_write_reg(struct i2c_client *client, u8 reg, u8 val) > +{ > + int ret = 0; > + > + ret = i2c_smbus_write_byte_data(client, reg, val); > + > + if (ret < 0) > + dev_err(&client->dev, "i2c writting fail at 0x%02x\n", reg); > + return ret; > +} > + > +static int lm3556_write_bits(struct i2c_client *client, > + u8 reg, u8 val, u8 mask, u8 shift) > +{ > + int ret; > + u8 reg_val; > + struct lm3556_chip_data *chip = i2c_get_clientdata(client); > + > + mutex_lock(&chip->lock); > + ret = lm3556_read_reg(client, reg, ®_val); > + if (ret < 0) > + goto out; > + reg_val &= (~(mask << shift)); > + reg_val |= ((val & mask) << shift); > + ret = lm3556_write_reg(client, reg, reg_val); > +out: > + mutex_unlock(&chip->lock); > + return ret; > +}
The register io-locking above is broken. You need to protect both write_reg and write_bits using the mutex (but you must restructure your code because write_bits currently calls write_reg). This is required to avoid register corruption due to concurrent write_reg and write_bits.
As has been suggested elsewhere, you could consider using regmap.
Thanks, Johan
| |