Am 23.09.2012 23:43, schrieb Guennadi Liakhovetski:
> On Sun, 23 Sep 2012, Frank Schäfer wrote:
>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx>
>> ---
>> drivers/media/i2c/soc_camera/ov2640.c | 17 ++++++++---------
>> 1 Datei geändert, 8 Zeilen hinzugefügt(+), 9 Zeilen entfernt(-)
>>
>> diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c
>> index 182d5a1..e71bf4c 100644
>> --- a/drivers/media/i2c/soc_camera/ov2640.c
>> +++ b/drivers/media/i2c/soc_camera/ov2640.c
>> @@ -639,17 +639,19 @@ static struct ov2640_priv *to_ov2640(const struct i2c_client *client)
>> subdev);
>> }
>>
>> +static int ov2640_write_single(struct i2c_client *client, u8 reg, u8 val)
>> +{
>> + dev_vdbg(&client->dev, "write: 0x%02x, 0x%02x", reg, val);
>> + return i2c_smbus_write_byte_data(client, reg, val);
>> +}
> Well, I'm not convinced. I don't necessarily see it as a simplification.
> You replace one perfectly ok function with another one with exactly the
> same parameters. Ok, you also hide a debug printk() in your wrapper, but
> that's not too useful either, IMHO.
Sure, at the moment this is not really needed. But that will change in
the future, when we need to do more single writes / can't use static
register sequences.
A good example is the powerline frequency filter control, which I'm
currently experimenting with.
But if you don't want to take it at the moment, it's ok for me.
> Besides, you're missing more calls to
> i2c_smbus_write_byte_data() in ov2640_mask_set(), ov2640_s_register() and
> ov2640_video_probe(). So, I'd just drop it.
I skipped that because of the different debug output (which could of
course be improved).
Regrads,
Frank
> Thanks
> Guennadi
>
>> +
>> static int ov2640_write_array(struct i2c_client *client,
>> const struct regval_list *vals)
>> {
>> int ret;
>>
>> while ((vals->reg_num != 0xff) || (vals->value != 0xff)) {
>> - ret = i2c_smbus_write_byte_data(client,
>> - vals->reg_num, vals->value);
>> - dev_vdbg(&client->dev, "array: 0x%02x, 0x%02x",
>> - vals->reg_num, vals->value);
>> -
>> + ret = ov2640_write_single(client, vals->reg_num, vals->value);
>> if (ret < 0)
>> return ret;
>> vals++;
>> @@ -704,13 +706,10 @@ static int ov2640_s_ctrl(struct v4l2_ctrl *ctrl)
>> struct v4l2_subdev *sd =
>> &container_of(ctrl->handler, struct ov2640_priv, hdl)->subdev;
>> struct i2c_client *client = v4l2_get_subdevdata(sd);
>> - struct regval_list regval;
>> int ret;
>> u8 val;
>>
>> - regval.reg_num = BANK_SEL;
>> - regval.value = BANK_SEL_SENS;
>> - ret = ov2640_write_array(client, ®val);
>> + ret = ov2640_write_single(client, BANK_SEL, BANK_SEL_SENS);
>> if (ret < 0)
>> return ret;
>>
>> --
>> 1.7.10.4
>>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
