Re: [PATCH 1/1 v1] leds: Add LED driver for lm3556 chip

[Johan Hovold <jhovold@gmail.com>]

On Tue, May 15, 2012 at 08:43:30PM +0200, Johan Hovold wrote:
> 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, &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.

Andrew, just wanted to remind you that this driver is still in your
tree. Do you want to drop it or should someone else fix the locking?

Johan