Am 24.09.2012 00:23, schrieb Guennadi Liakhovetski:
> On Sun, 23 Sep 2012, Frank Schäfer wrote:
>
>> 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.
> Why won't you be able to just use i2c_smbus_write_byte_data() directly
> with those your sequences? Ok, if you just dislike the long name, and if
> you have a number of them, I might buy that as a valid reason:-)
The suggest helper function also prints a debugging message, so we are
talking about two lines for each single write.
> And yes,
> it'd be good to add such a helper function in a separate patch, preceding
> the actual functional changes. But then I'd probably suggest to name, that
> offers an even greater saving of your monitor real estate and is more
> similar to what other drivers use, something like ov2640_reg_write() and
> also add an ov2640_reg_read() for symmetry.
Ok, thats a matter of taste ;). ov2640_write_single seemed to be the
logocal counterpart to ov2640_write_array, but I don't care.
I will come back to this when the next feature patch(es) are ready.
Regards,
Frank
>
> Thanks
> Guennadi
>
>> 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
> ---
> 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
