On 9/27/19 2:13 PM, Marco Felsch wrote:
> Hi Hans,
>
> since you review contained many checkpatch issues I wanna ask if it okay
> to keep this line as it before I will send a v11. Please check my inline
> comment.
>
> On 19-08-30 12:16, Marco Felsch wrote:
>> Currently a local copy of sel->r is made and adapted to the hardware
>> constraints. After the adaption the value is applied to the hardware but
>> the driver forgot to reflect the adapted value to the user space.
>>
>> Drop the local copy and work directly on the requested rectangle
>> instead to fix this.
>>
>> Signed-off-by: Marco Felsch <[email protected]>
>> ---
>>
>> v10:
>> - new patch
>>
>> drivers/media/i2c/tvp5150.c | 32 ++++++++++++++++----------------
>> 1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
>> index 477a929d4f89..c1542a89e8c8 100644
>> --- a/drivers/media/i2c/tvp5150.c
>> +++ b/drivers/media/i2c/tvp5150.c
>> @@ -1024,7 +1024,7 @@ static int tvp5150_set_selection(struct v4l2_subdev
>> *sd,
>> struct v4l2_subdev_selection *sel)
>> {
>> struct tvp5150 *decoder = to_tvp5150(sd);
>> - struct v4l2_rect rect = sel->r;
>> + struct v4l2_rect *rect = &sel->r;
>> v4l2_std_id std;
>> int hmax;
>>
>> @@ -1033,11 +1033,11 @@ static int tvp5150_set_selection(struct v4l2_subdev
>> *sd,
>> return -EINVAL;
>>
>> dev_dbg_lvl(sd->dev, 1, debug, "%s left=%d, top=%d, width=%d,
>> height=%d\n",
>> - __func__, rect.left, rect.top, rect.width, rect.height);
>> + __func__, rect->left, rect->top, rect->width, rect->height);
>>
>> /* tvp5150 has some special limits */
>> - rect.left = clamp(rect.left, 0, TVP5150_MAX_CROP_LEFT);
>> - rect.top = clamp(rect.top, 0, TVP5150_MAX_CROP_TOP);
>> + rect->left = clamp(rect->left, 0, TVP5150_MAX_CROP_LEFT);
>> + rect->top = clamp(rect->top, 0, TVP5150_MAX_CROP_TOP);
>>
>> /* Calculate height based on current standard */
>> if (decoder->norm == V4L2_STD_ALL)
>> @@ -1055,26 +1055,26 @@ static int tvp5150_set_selection(struct v4l2_subdev
>> *sd,
>> * - width = 2 due to UYVY colorspace
>> * - height, image = no special alignment
>> */
>> - v4l_bound_align_image(&rect.width,
>> - TVP5150_H_MAX - TVP5150_MAX_CROP_LEFT - rect.left,
>> - TVP5150_H_MAX - rect.left, 1, &rect.height,
>> - hmax - TVP5150_MAX_CROP_TOP - rect.top,
>> - hmax - rect.top, 0, 0);
>> + v4l_bound_align_image(&rect->width,
>> + TVP5150_H_MAX - TVP5150_MAX_CROP_LEFT -
>> rect->left,
>
> Now checkpatch complains about this line because it is 81 characters
> long. Is it okay to keep this as single line for readability?
Yes, in this case splitting it up just makes it harder to read, so keep it
as-is.
Regards,
Hans
>
> Regards,
> Marco
>
>
>> + TVP5150_H_MAX - rect->left, 1, &rect->height,
>> + hmax - TVP5150_MAX_CROP_TOP - rect->top,
>> + hmax - rect->top, 0, 0);
>>
>> - regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top);
>> + regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect->top);
>> regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP,
>> - rect.top + rect.height - hmax);
>> + rect->top + rect->height - hmax);
>> regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB,
>> - rect.left >> TVP5150_CROP_SHIFT);
>> + rect->left >> TVP5150_CROP_SHIFT);
>> regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB,
>> - rect.left | (1 << TVP5150_CROP_SHIFT));
>> + rect->left | (1 << TVP5150_CROP_SHIFT));
>> regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_MSB,
>> - (rect.left + rect.width - TVP5150_MAX_CROP_LEFT) >>
>> + (rect->left + rect->width - TVP5150_MAX_CROP_LEFT) >>
>> TVP5150_CROP_SHIFT);
>> regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_LSB,
>> - rect.left + rect.width - TVP5150_MAX_CROP_LEFT);
>> + rect->left + rect->width - TVP5150_MAX_CROP_LEFT);
>>
>> - decoder->rect = rect;
>> + decoder->rect = *rect;
>>
>> return 0;
>> }
>> --
>> 2.20.1
>>
>>
>
