Re: [PATCH v10 07/14] media: tvp5150: fix set_selection rectangle handling
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
>> ---
>>
>> 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
>>
>>
>
Re: [PATCH v10 07/14] media: tvp5150: fix set_selection rectangle handling
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
> ---
>
> 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?
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
>
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH v10 07/14] media: tvp5150: fix set_selection rectangle handling
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
---
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,
+ 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
