[i2c] [PATCH v6] Add ov772x driver

morimoto.kuninori at renesas.com morimoto.kuninori at renesas.com
Mon Oct 20 11:54:07 CEST 2008


Hi Jean Delvare again

Thank you for checking patch.

> > +	reg->val = i2c_smbus_read_byte_data(priv->client, reg->reg);
> > +
> > +	if (reg->val < 0)
> 
> Nitpicking: traditional coding style says no blank line between
> function call and its error checking.

Thank you.
I'll fix it.

> Also, I just noticed that reg->val is an __u64 so it can't be < 0.
> You'll have to use a local int to store the result of
> i2c_smbus_read_byte_data(). I'm surprised the compiler didn't warn
> you... Maybe you didn't test with CONFIG_VIDEO_ADV_DEBUG=y?
> 
> > +		return -EIO;
> 
> You might as well return the error value you got from
> i2c_smbus_read_byte_data(), it may be more detailed.

OK. I'll fix it.

> > +	if (i2c_smbus_write_byte_data(priv->client, reg->reg, reg->val) < 0)
> > +		return -EIO;
> 
> Here again it would be preferable to return the error value you got
> from i2c_smbus_write_byte_data() instead of hard-coding one.

OK. I'll fix it.


> > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> > +		dev_warn(&adapter->dev,
> > +			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_BYTE\n");
> 
> The warning message still doesn't match the failed check
> (I2C_FUNC_SMBUS_BYTE_DATA != I2C_FUNC_SMBUS_BYTE). Also, I believe you
> should use dev_err(), not dev_warn(), as this is a fatal error.

Is that so?
mt9m001.c used dev_warn...

I'm sorry about the content of the message. 
I thought what you say is "adapter".

OK. I'll fix it.

/Morimoto




More information about the i2c mailing list