Hi Kuninori,
On Mon, 20 Oct 2008 18:06:29 +0900, Kuninori Morimoto wrote:
> This patch adds ov772x driver that use soc_camera framework.
> It was tested on SH Migo-r board.
>
> Signed-off-by: Kuninori Morimoto <[EMAIL PROTECTED]>
> ---
> PATCH v5 -> PATCH v6
> fix i2c smbus byte/word order miss
> fix checking reg->val from > 0xff to < 0
> fix i2c adapter on i2c_check_functionality
A couple more things...
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +static int ov772x_get_register(struct soc_camera_device *icd,
> + struct v4l2_register *reg)
> +{
> + struct ov772x_priv *priv = container_of(icd, struct ov772x_priv, icd);
> +
> + if (reg->reg > 0xff)
> + return -EINVAL;
> +
> + 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.
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.
> +
> + return 0;
> +}
> +
> +static int ov772x_set_register(struct soc_camera_device *icd,
> + struct v4l2_register *reg)
> +{
> + struct ov772x_priv *priv = container_of(icd, struct ov772x_priv, icd);
> +
> + if (reg->reg > 0xff ||
> + reg->val > 0xff)
> + return -EINVAL;
> +
> + 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.
> +
> + return 0;
> +}
> +#endif
> (...)
> +static int ov772x_probe(struct i2c_client *client,
> + const struct i2c_device_id *did)
> +
> +{
> + struct ov772x_priv *priv;
> + struct ov772x_camera_info *info;
> + struct soc_camera_device *icd;
> + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> + int ret;
> +
> + info = client->dev.platform_data;
> + if (!info)
> + return -EINVAL;
> +
> + 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.
> + return -EIO;
> + }
--
Jean Delvare
_______________________________________________
i2c mailing list
i2c@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/i2c
