[PATCH v2 0/4] media: staging: rkisp1: bugs fixes and vars renames

Linux-Rockchip Archive on lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH v2 0/4] media: staging: rkisp1: bugs fixes and vars renames
@ 2020-06-09 15:28 Dafna Hirschfeld
  2020-06-09 15:28 ` [PATCH v2 1/4] media: staging: rkisp1: rsz: supported formats are the isp's src formats, not sink formats Dafna Hirschfeld
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Dafna Hirschfeld @ 2020-06-09 15:28 UTC (permalink / raw)
  To: linux-media, laurent.pinchart
  Cc: dafna.hirschfeld, helen.koike, ezequiel, hverkuil, kernel, dafna3,
	sakari.ailus, linux-rockchip, mchehab, tfiga

The first two patches in this patchset are two bug fixes related to the enumeration and
settings of the sink format of the resizer entity.
The two other patches are renaming of macros and the variables.

changes from v1:
- added "Fixes: 56e3b29f9f6b "media: staging: rkisp1: add streaming paths"
to the commit log of the first two patches.
- added two patches. One patch rename the macros "RKISP1_DIR_*"
to "RKISP1_ISP_SD_*", another that rename the field 'direction'
in 'struct rkisp1_isp_mbus_info' to 'isp_pads_flags'

Dafna Hirschfeld (4):
  media: staging: rkisp1: rsz: supported formats are the isp's src
    formats, not sink formats
  media: staging: rkisp1: rsz: set default format if the given format is
    not RKISP1_DIR_SRC
  media: staging: rkisp1: rename macros 'RKISP1_DIR_*' to
    'RKISP1_ISP_SD_*'
  media: staging: rkisp1: rename the field 'direction' in
    'rkisp1_isp_mbus_info' to 'isp_pads_flags'

 drivers/staging/media/rkisp1/rkisp1-common.h  |  6 ++-
 drivers/staging/media/rkisp1/rkisp1-isp.c     | 50 +++++++++----------
 drivers/staging/media/rkisp1/rkisp1-resizer.c |  6 +--
 3 files changed, 31 insertions(+), 31 deletions(-)

-- 
2.17.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/4] media: staging: rkisp1: rsz: supported formats are the isp's src formats, not sink formats
  2020-06-09 15:28 [PATCH v2 0/4] media: staging: rkisp1: bugs fixes and vars renames Dafna Hirschfeld
@ 2020-06-09 15:28 ` Dafna Hirschfeld
  2020-06-10 17:15   ` Tomasz Figa
  2020-06-09 15:28 ` [PATCH v2 2/4] media: staging: rkisp1: rsz: set default format if the given format is not RKISP1_DIR_SRC Dafna Hirschfeld
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Dafna Hirschfeld @ 2020-06-09 15:28 UTC (permalink / raw)
  To: linux-media, laurent.pinchart
  Cc: dafna.hirschfeld, helen.koike, ezequiel, hverkuil, kernel, dafna3,
	sakari.ailus, linux-rockchip, mchehab, tfiga

The rkisp1_resizer's enum callback 'rkisp1_rsz_enum_mbus_code'
calls the enum callback of the 'rkisp1_isp' on it's video sink pad.
This is a bug, the resizer should support the same formats
supported by the 'rkisp1_isp' on the source pad (not the sink pad).

Fixes: 56e3b29f9f6b "media: staging: rkisp1: add streaming paths"

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-resizer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index d049374413dc..d64c064bdb1d 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -437,8 +437,8 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
 	u32 pad = code->pad;
 	int ret;
 
-	/* supported mbus codes are the same in isp sink pad */
-	code->pad = RKISP1_ISP_PAD_SINK_VIDEO;
+	/* supported mbus codes are the same in isp video src pad */
+	code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
 	ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
 			       &dummy_cfg, code);
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] media: staging: rkisp1: rsz: supported formats are the isp's src formats, not sink formats
  2020-06-09 15:28 ` [PATCH v2 1/4] media: staging: rkisp1: rsz: supported formats are the isp's src formats, not sink formats Dafna Hirschfeld
@ 2020-06-10 17:15   ` Tomasz Figa
  2020-06-10 18:36     ` Dafna Hirschfeld
  0 siblings, 1 reply; 12+ messages in thread
From: Tomasz Figa @ 2020-06-10 17:15 UTC (permalink / raw)
  To: Dafna Hirschfeld
  Cc: linux-media, laurent.pinchart, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, sakari.ailus, linux-rockchip, mchehab

Hi Dafna,

On Tue, Jun 09, 2020 at 05:28:22PM +0200, Dafna Hirschfeld wrote:
> The rkisp1_resizer's enum callback 'rkisp1_rsz_enum_mbus_code'
> calls the enum callback of the 'rkisp1_isp' on it's video sink pad.
> This is a bug, the resizer should support the same formats
> supported by the 'rkisp1_isp' on the source pad (not the sink pad).
> 
> Fixes: 56e3b29f9f6b "media: staging: rkisp1: add streaming paths"
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Acked-by: Helen Koike <helen.koike@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-resizer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> index d049374413dc..d64c064bdb1d 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> @@ -437,8 +437,8 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
>  	u32 pad = code->pad;
>  	int ret;
>  
> -	/* supported mbus codes are the same in isp sink pad */
> -	code->pad = RKISP1_ISP_PAD_SINK_VIDEO;
> +	/* supported mbus codes are the same in isp video src pad */
> +	code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
>  	ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
>  			       &dummy_cfg, code);

Actually, is this really the true? AFAIR the ISP itself can only output
either Bayer or YUV 4:2:2. The resizer can take YUV 4:2:2 at its input
and output YUV 4:4:4, 4:2:2 and 4:2:0. Bayer capture is handled with
resizer bypass mode. We haven't tested that, but if implemented, it
should probably be done by exposing a link between the ISP entity and a
video node directly, without the resizer involved.

WDYT?

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] media: staging: rkisp1: rsz: supported formats are the isp's src formats, not sink formats
  2020-06-10 17:15   ` Tomasz Figa
@ 2020-06-10 18:36     ` Dafna Hirschfeld
  2020-06-10 19:00       ` Dafna Hirschfeld
  0 siblings, 1 reply; 12+ messages in thread
From: Dafna Hirschfeld @ 2020-06-10 18:36 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: linux-media, laurent.pinchart, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, sakari.ailus, linux-rockchip, mchehab



On 10.06.20 19:15, Tomasz Figa wrote:
> Hi Dafna,
> 
> On Tue, Jun 09, 2020 at 05:28:22PM +0200, Dafna Hirschfeld wrote:
>> The rkisp1_resizer's enum callback 'rkisp1_rsz_enum_mbus_code'
>> calls the enum callback of the 'rkisp1_isp' on it's video sink pad.
>> This is a bug, the resizer should support the same formats
>> supported by the 'rkisp1_isp' on the source pad (not the sink pad).
>>
>> Fixes: 56e3b29f9f6b "media: staging: rkisp1: add streaming paths"
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Acked-by: Helen Koike <helen.koike@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
> 
> Thank you for the patch. Please see my comments inline.
> 
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> index d049374413dc..d64c064bdb1d 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> @@ -437,8 +437,8 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
>>   	u32 pad = code->pad;
>>   	int ret;
>>   
>> -	/* supported mbus codes are the same in isp sink pad */
>> -	code->pad = RKISP1_ISP_PAD_SINK_VIDEO;
>> +	/* supported mbus codes are the same in isp video src pad */
>> +	code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
>>   	ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
>>   			       &dummy_cfg, code);
> 
> Actually, is this really the true? AFAIR the ISP itself can only output
> either Bayer or YUV 4:2:2. The resizer can take YUV 4:2:2 at its input
> and output YUV 4:4:4, 4:2:2 and 4:2:0. Bayer capture is handled with
> resizer bypass mode. We haven't tested that, but if implemented, it
> should probably be done by exposing a link between the ISP entity and a
> video node directly, without the resizer involved.
> 
> WDYT?

We can also implement it that way. Only the mainpath needs
a direct link from the isp since selfpath does not support bayer formats.
It makes it easier on userspace for bayer formats since it does not have to
configure the resizer.
On the other hand if the format is YUV it has the option
to either use the the resizer or not.

Thanks,
Dafna

> 
> Best regards,
> Tomasz
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] media: staging: rkisp1: rsz: supported formats are the isp's src formats, not sink formats
  2020-06-10 18:36     ` Dafna Hirschfeld
@ 2020-06-10 19:00       ` Dafna Hirschfeld
       [not found]         ` <bb5576b1-7bfb-7911-004c-8fa1b4e1603c-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
  0 siblings, 1 reply; 12+ messages in thread
From: Dafna Hirschfeld @ 2020-06-10 19:00 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: linux-media, laurent.pinchart, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, sakari.ailus, linux-rockchip, mchehab



On 10.06.20 20:36, Dafna Hirschfeld wrote:
> 
> 
> On 10.06.20 19:15, Tomasz Figa wrote:
>> Hi Dafna,
>>
>> On Tue, Jun 09, 2020 at 05:28:22PM +0200, Dafna Hirschfeld wrote:
>>> The rkisp1_resizer's enum callback 'rkisp1_rsz_enum_mbus_code'
>>> calls the enum callback of the 'rkisp1_isp' on it's video sink pad.
>>> This is a bug, the resizer should support the same formats
>>> supported by the 'rkisp1_isp' on the source pad (not the sink pad).
>>>
>>> Fixes: 56e3b29f9f6b "media: staging: rkisp1: add streaming paths"
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>> ---
>>>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>
>> Thank you for the patch. Please see my comments inline.
>>
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>> index d049374413dc..d64c064bdb1d 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>> @@ -437,8 +437,8 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
>>>       u32 pad = code->pad;
>>>       int ret;
>>> -    /* supported mbus codes are the same in isp sink pad */
>>> -    code->pad = RKISP1_ISP_PAD_SINK_VIDEO;
>>> +    /* supported mbus codes are the same in isp video src pad */
>>> +    code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
>>>       ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
>>>                      &dummy_cfg, code);
>>
>> Actually, is this really the true? AFAIR the ISP itself can only output
>> either Bayer or YUV 4:2:2. The resizer can take YUV 4:2:2 at its input
>> and output YUV 4:4:4, 4:2:2 and 4:2:0. Bayer capture is handled with
>> resizer bypass mode. We haven't tested that, but if implemented, it
>> should probably be done by exposing a link between the ISP entity and a
>> video node directly, without the resizer involved.
>>
>> WDYT?
> 
> We can also implement it that way. Only the mainpath needs
> a direct link from the isp since selfpath does not support bayer formats.
> It makes it easier on userspace for bayer formats since it does not have to
> configure the resizer.
> On the other hand if the format is YUV it has the option
> to either use the the resizer or not.
> 
> Thanks,
> Dafna

Anyway, this is a two line bug fix, so I think this patch can first be
accepted and then if we choose to change the topology this can be done
in a separate patchset.

Thanks,
Dafna



> 
>>
>> Best regards,
>> Tomasz
>>

^ permalink raw reply	[flat|nested] 12+ messages in thread

[parent not found: <bb5576b1-7bfb-7911-004c-8fa1b4e1603c-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>]

* Re: [PATCH v2 1/4] media: staging: rkisp1: rsz: supported formats are the isp's src formats, not sink formats
       [not found]         ` <bb5576b1-7bfb-7911-004c-8fa1b4e1603c-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
@ 2020-06-10 19:28           ` Tomasz Figa
  0 siblings, 0 replies; 12+ messages in thread
From: Tomasz Figa @ 2020-06-10 19:28 UTC (permalink / raw)
  To: Dafna Hirschfeld
  Cc: Mauro Carvalho Chehab, Dafna Hirschfeld, Hans Verkuil,
	open list:ARM/Rockchip SoC..., Helen Koike, Laurent Pinchart,
	Sakari Ailus, kernel-ZGY8ohtN/8qB+jHODAdFcQ, Ezequiel Garcia,
	Linux Media Mailing List

On Wed, Jun 10, 2020 at 9:00 PM Dafna Hirschfeld
<dafna.hirschfeld-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> wrote:
>
>
>
> On 10.06.20 20:36, Dafna Hirschfeld wrote:
> >
> >
> > On 10.06.20 19:15, Tomasz Figa wrote:
> >> Hi Dafna,
> >>
> >> On Tue, Jun 09, 2020 at 05:28:22PM +0200, Dafna Hirschfeld wrote:
> >>> The rkisp1_resizer's enum callback 'rkisp1_rsz_enum_mbus_code'
> >>> calls the enum callback of the 'rkisp1_isp' on it's video sink pad.
> >>> This is a bug, the resizer should support the same formats
> >>> supported by the 'rkisp1_isp' on the source pad (not the sink pad).
> >>>
> >>> Fixes: 56e3b29f9f6b "media: staging: rkisp1: add streaming paths"
> >>>
> >>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> >>> Acked-by: Helen Koike <helen.koike-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> >>> ---
> >>>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>
> >> Thank you for the patch. Please see my comments inline.
> >>
> >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >>> index d049374413dc..d64c064bdb1d 100644
> >>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >>> @@ -437,8 +437,8 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
> >>>       u32 pad = code->pad;
> >>>       int ret;
> >>> -    /* supported mbus codes are the same in isp sink pad */
> >>> -    code->pad = RKISP1_ISP_PAD_SINK_VIDEO;
> >>> +    /* supported mbus codes are the same in isp video src pad */
> >>> +    code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
> >>>       ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
> >>>                      &dummy_cfg, code);
> >>
> >> Actually, is this really the true? AFAIR the ISP itself can only output
> >> either Bayer or YUV 4:2:2. The resizer can take YUV 4:2:2 at its input
> >> and output YUV 4:4:4, 4:2:2 and 4:2:0. Bayer capture is handled with
> >> resizer bypass mode. We haven't tested that, but if implemented, it
> >> should probably be done by exposing a link between the ISP entity and a
> >> video node directly, without the resizer involved.
> >>
> >> WDYT?
> >
> > We can also implement it that way. Only the mainpath needs
> > a direct link from the isp since selfpath does not support bayer formats.
> > It makes it easier on userspace for bayer formats since it does not have to
> > configure the resizer.
> > On the other hand if the format is YUV it has the option
> > to either use the the resizer or not.
> >
> > Thanks,
> > Dafna
>
> Anyway, this is a two line bug fix, so I think this patch can first be
> accepted and then if we choose to change the topology this can be done
> in a separate patchset.

Makes sense. Feel free to add my Reviewed-by.

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 2/4] media: staging: rkisp1: rsz: set default format if the given format is not RKISP1_DIR_SRC
  2020-06-09 15:28 [PATCH v2 0/4] media: staging: rkisp1: bugs fixes and vars renames Dafna Hirschfeld
  2020-06-09 15:28 ` [PATCH v2 1/4] media: staging: rkisp1: rsz: supported formats are the isp's src formats, not sink formats Dafna Hirschfeld
@ 2020-06-09 15:28 ` Dafna Hirschfeld
  2020-06-10 17:19   ` Tomasz Figa
  2020-06-09 15:28 ` [PATCH v2 3/4] media: staging: rkisp1: rename macros 'RKISP1_DIR_*' to 'RKISP1_ISP_SD_*' Dafna Hirschfeld
  2020-06-09 15:28 ` [PATCH v2 4/4] media: staging: rkisp1: rename the field 'direction' in 'rkisp1_isp_mbus_info' to 'isp_pads_flags' Dafna Hirschfeld
  3 siblings, 1 reply; 12+ messages in thread
From: Dafna Hirschfeld @ 2020-06-09 15:28 UTC (permalink / raw)
  To: linux-media, laurent.pinchart
  Cc: dafna.hirschfeld, helen.koike, ezequiel, hverkuil, kernel, dafna3,
	sakari.ailus, linux-rockchip, mchehab, tfiga

When setting the sink format of the 'rkisp1_resizer'
the format should be supported by 'rkisp1_isp' on
the video source pad. This patch checks this condition
and set the format to default if the condition is false.

Fixes: 56e3b29f9f6b "media: staging: rkisp1: add streaming paths"

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h  | 4 ++++
 drivers/staging/media/rkisp1/rkisp1-isp.c     | 4 ----
 drivers/staging/media/rkisp1/rkisp1-resizer.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 0c4fe503adc9..39d8e46d8d8a 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -22,6 +22,10 @@
 #include "rkisp1-regs.h"
 #include "uapi/rkisp1-config.h"
 
+#define RKISP1_DIR_SRC BIT(0)
+#define RKISP1_DIR_SINK BIT(1)
+#define RKISP1_DIR_SINK_SRC (RKISP1_DIR_SINK | RKISP1_DIR_SRC)
+
 #define RKISP1_ISP_MAX_WIDTH		4032
 #define RKISP1_ISP_MAX_HEIGHT		3024
 #define RKISP1_ISP_MIN_WIDTH		32
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index dc2b59a0160a..e66e87d6ea8b 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -23,10 +23,6 @@
 
 #define RKISP1_ISP_DEV_NAME	RKISP1_DRIVER_NAME "_isp"
 
-#define RKISP1_DIR_SRC BIT(0)
-#define RKISP1_DIR_SINK BIT(1)
-#define RKISP1_DIR_SINK_SRC (RKISP1_DIR_SINK | RKISP1_DIR_SRC)
-
 /*
  * NOTE: MIPI controller and input MUX are also configured in this file.
  * This is because ISP Subdev describes not only ISP submodule (input size,
diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index d64c064bdb1d..fa28f4bd65c0 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -542,7 +542,7 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
 					    which);
 	sink_fmt->code = format->code;
 	mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
-	if (!mbus_info) {
+	if (!mbus_info || !(mbus_info->direction & RKISP1_DIR_SRC)) {
 		sink_fmt->code = RKISP1_DEF_FMT;
 		mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
 	}
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: set default format if the given format is not RKISP1_DIR_SRC
  2020-06-09 15:28 ` [PATCH v2 2/4] media: staging: rkisp1: rsz: set default format if the given format is not RKISP1_DIR_SRC Dafna Hirschfeld
@ 2020-06-10 17:19   ` Tomasz Figa
  0 siblings, 0 replies; 12+ messages in thread
From: Tomasz Figa @ 2020-06-10 17:19 UTC (permalink / raw)
  To: Dafna Hirschfeld
  Cc: linux-media, laurent.pinchart, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, sakari.ailus, linux-rockchip, mchehab

Hi Dafna,

On Tue, Jun 09, 2020 at 05:28:23PM +0200, Dafna Hirschfeld wrote:
> When setting the sink format of the 'rkisp1_resizer'
> the format should be supported by 'rkisp1_isp' on
> the video source pad. This patch checks this condition
> and set the format to default if the condition is false.
> 
> Fixes: 56e3b29f9f6b "media: staging: rkisp1: add streaming paths"
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h  | 4 ++++
>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 4 ----
>  drivers/staging/media/rkisp1/rkisp1-resizer.c | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 3/4] media: staging: rkisp1: rename macros 'RKISP1_DIR_*' to 'RKISP1_ISP_SD_*'
  2020-06-09 15:28 [PATCH v2 0/4] media: staging: rkisp1: bugs fixes and vars renames Dafna Hirschfeld
  2020-06-09 15:28 ` [PATCH v2 1/4] media: staging: rkisp1: rsz: supported formats are the isp's src formats, not sink formats Dafna Hirschfeld
  2020-06-09 15:28 ` [PATCH v2 2/4] media: staging: rkisp1: rsz: set default format if the given format is not RKISP1_DIR_SRC Dafna Hirschfeld
@ 2020-06-09 15:28 ` Dafna Hirschfeld
  2020-06-10 17:24   ` Tomasz Figa
  2020-06-09 15:28 ` [PATCH v2 4/4] media: staging: rkisp1: rename the field 'direction' in 'rkisp1_isp_mbus_info' to 'isp_pads_flags' Dafna Hirschfeld
  3 siblings, 1 reply; 12+ messages in thread
From: Dafna Hirschfeld @ 2020-06-09 15:28 UTC (permalink / raw)
  To: linux-media, laurent.pinchart
  Cc: dafna.hirschfeld, helen.koike, ezequiel, hverkuil, kernel, dafna3,
	sakari.ailus, linux-rockchip, mchehab, tfiga

The macros 'RKISP1_DIR_*' are flags that indicate on which
pads of the isp subdevice the media bus code is supported. so the
prefix RKISP1_ISP_SD_ is better.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h  |  6 +--
 drivers/staging/media/rkisp1/rkisp1-isp.c     | 42 +++++++++----------
 drivers/staging/media/rkisp1/rkisp1-resizer.c |  2 +-
 3 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 39d8e46d8d8a..a6cd9fc13b3d 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -22,9 +22,9 @@
 #include "rkisp1-regs.h"
 #include "uapi/rkisp1-config.h"
 
-#define RKISP1_DIR_SRC BIT(0)
-#define RKISP1_DIR_SINK BIT(1)
-#define RKISP1_DIR_SINK_SRC (RKISP1_DIR_SINK | RKISP1_DIR_SRC)
+#define RKISP1_ISP_SD_SRC BIT(0)
+#define RKISP1_ISP_SD_SINK BIT(1)
+#define RKISP1_ISP_SD_SINK_SRC (RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC)
 
 #define RKISP1_ISP_MAX_WIDTH		4032
 #define RKISP1_ISP_MAX_HEIGHT		3024
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index e66e87d6ea8b..b63f193a3ac2 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -58,119 +58,119 @@ static const struct rkisp1_isp_mbus_info rkisp1_isp_formats[] = {
 	{
 		.mbus_code	= MEDIA_BUS_FMT_YUYV8_2X8,
 		.pixel_enc	= V4L2_PIXEL_ENC_YUV,
-		.direction	= RKISP1_DIR_SRC,
+		.direction	= RKISP1_ISP_SD_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SRGGB10_1X10,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW10,
 		.bayer_pat	= RKISP1_RAW_RGGB,
 		.bus_width	= 10,
-		.direction	= RKISP1_DIR_SINK_SRC,
+		.direction	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SBGGR10_1X10,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW10,
 		.bayer_pat	= RKISP1_RAW_BGGR,
 		.bus_width	= 10,
-		.direction	= RKISP1_DIR_SINK_SRC,
+		.direction	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SGBRG10_1X10,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW10,
 		.bayer_pat	= RKISP1_RAW_GBRG,
 		.bus_width	= 10,
-		.direction	= RKISP1_DIR_SINK_SRC,
+		.direction	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SGRBG10_1X10,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW10,
 		.bayer_pat	= RKISP1_RAW_GRBG,
 		.bus_width	= 10,
-		.direction	= RKISP1_DIR_SINK_SRC,
+		.direction	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SRGGB12_1X12,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW12,
 		.bayer_pat	= RKISP1_RAW_RGGB,
 		.bus_width	= 12,
-		.direction	= RKISP1_DIR_SINK_SRC,
+		.direction	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SBGGR12_1X12,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW12,
 		.bayer_pat	= RKISP1_RAW_BGGR,
 		.bus_width	= 12,
-		.direction	= RKISP1_DIR_SINK_SRC,
+		.direction	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SGBRG12_1X12,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW12,
 		.bayer_pat	= RKISP1_RAW_GBRG,
 		.bus_width	= 12,
-		.direction	= RKISP1_DIR_SINK_SRC,
+		.direction	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SGRBG12_1X12,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW12,
 		.bayer_pat	= RKISP1_RAW_GRBG,
 		.bus_width	= 12,
-		.direction	= RKISP1_DIR_SINK_SRC,
+		.direction	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SRGGB8_1X8,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW8,
 		.bayer_pat	= RKISP1_RAW_RGGB,
 		.bus_width	= 8,
-		.direction	= RKISP1_DIR_SINK_SRC,
+		.direction	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SBGGR8_1X8,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW8,
 		.bayer_pat	= RKISP1_RAW_BGGR,
 		.bus_width	= 8,
-		.direction	= RKISP1_DIR_SINK_SRC,
+		.direction	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SGBRG8_1X8,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW8,
 		.bayer_pat	= RKISP1_RAW_GBRG,
 		.bus_width	= 8,
-		.direction	= RKISP1_DIR_SINK_SRC,
+		.direction	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SGRBG8_1X8,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW8,
 		.bayer_pat	= RKISP1_RAW_GRBG,
 		.bus_width	= 8,
-		.direction	= RKISP1_DIR_SINK_SRC,
+		.direction	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_YUYV8_1X16,
 		.pixel_enc	= V4L2_PIXEL_ENC_YUV,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_YUV422_8b,
 		.yuv_seq	= RKISP1_CIF_ISP_ACQ_PROP_YCBYCR,
 		.bus_width	= 16,
-		.direction	= RKISP1_DIR_SINK,
+		.direction	= RKISP1_ISP_SD_SINK,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_YVYU8_1X16,
 		.pixel_enc	= V4L2_PIXEL_ENC_YUV,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_YUV422_8b,
 		.yuv_seq	= RKISP1_CIF_ISP_ACQ_PROP_YCRYCB,
 		.bus_width	= 16,
-		.direction	= RKISP1_DIR_SINK,
+		.direction	= RKISP1_ISP_SD_SINK,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_UYVY8_1X16,
 		.pixel_enc	= V4L2_PIXEL_ENC_YUV,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_YUV422_8b,
 		.yuv_seq	= RKISP1_CIF_ISP_ACQ_PROP_CBYCRY,
 		.bus_width	= 16,
-		.direction	= RKISP1_DIR_SINK,
+		.direction	= RKISP1_ISP_SD_SINK,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_VYUY8_1X16,
 		.pixel_enc	= V4L2_PIXEL_ENC_YUV,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_YUV422_8b,
 		.yuv_seq	= RKISP1_CIF_ISP_ACQ_PROP_CRYCBY,
 		.bus_width	= 16,
-		.direction	= RKISP1_DIR_SINK,
+		.direction	= RKISP1_ISP_SD_SINK,
 	},
 };
 
@@ -570,9 +570,9 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
 	int pos = 0;
 
 	if (code->pad == RKISP1_ISP_PAD_SINK_VIDEO) {
-		dir = RKISP1_DIR_SINK;
+		dir = RKISP1_ISP_SD_SINK;
 	} else if (code->pad == RKISP1_ISP_PAD_SOURCE_VIDEO) {
-		dir = RKISP1_DIR_SRC;
+		dir = RKISP1_ISP_SD_SRC;
 	} else {
 		if (code->index > 0)
 			return -EINVAL;
@@ -657,7 +657,7 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
 
 	src_fmt->code = format->code;
 	mbus_info = rkisp1_isp_mbus_info_get(src_fmt->code);
-	if (!mbus_info || !(mbus_info->direction & RKISP1_DIR_SRC)) {
+	if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SRC)) {
 		src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
 		mbus_info = rkisp1_isp_mbus_info_get(src_fmt->code);
 	}
@@ -741,7 +741,7 @@ static void rkisp1_isp_set_sink_fmt(struct rkisp1_isp *isp,
 					  which);
 	sink_fmt->code = format->code;
 	mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
-	if (!mbus_info || !(mbus_info->direction & RKISP1_DIR_SINK)) {
+	if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SINK)) {
 		sink_fmt->code = RKISP1_DEF_SINK_PAD_FMT;
 		mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
 	}
diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index fa28f4bd65c0..137298b77341 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -542,7 +542,7 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
 					    which);
 	sink_fmt->code = format->code;
 	mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
-	if (!mbus_info || !(mbus_info->direction & RKISP1_DIR_SRC)) {
+	if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SRC)) {
 		sink_fmt->code = RKISP1_DEF_FMT;
 		mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
 	}
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 3/4] media: staging: rkisp1: rename macros 'RKISP1_DIR_*' to 'RKISP1_ISP_SD_*'
  2020-06-09 15:28 ` [PATCH v2 3/4] media: staging: rkisp1: rename macros 'RKISP1_DIR_*' to 'RKISP1_ISP_SD_*' Dafna Hirschfeld
@ 2020-06-10 17:24   ` Tomasz Figa
  0 siblings, 0 replies; 12+ messages in thread
From: Tomasz Figa @ 2020-06-10 17:24 UTC (permalink / raw)
  To: Dafna Hirschfeld
  Cc: linux-media, laurent.pinchart, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, sakari.ailus, linux-rockchip, mchehab

Hi Dafna,

On Tue, Jun 09, 2020 at 05:28:24PM +0200, Dafna Hirschfeld wrote:
> The macros 'RKISP1_DIR_*' are flags that indicate on which
> pads of the isp subdevice the media bus code is supported. so the
> prefix RKISP1_ISP_SD_ is better.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h  |  6 +--
>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 42 +++++++++----------
>  drivers/staging/media/rkisp1/rkisp1-resizer.c |  2 +-
>  3 files changed, 25 insertions(+), 25 deletions(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 39d8e46d8d8a..a6cd9fc13b3d 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -22,9 +22,9 @@
>  #include "rkisp1-regs.h"
>  #include "uapi/rkisp1-config.h"
>  
> -#define RKISP1_DIR_SRC BIT(0)
> -#define RKISP1_DIR_SINK BIT(1)
> -#define RKISP1_DIR_SINK_SRC (RKISP1_DIR_SINK | RKISP1_DIR_SRC)
> +#define RKISP1_ISP_SD_SRC BIT(0)
> +#define RKISP1_ISP_SD_SINK BIT(1)
> +#define RKISP1_ISP_SD_SINK_SRC (RKISP1_ISP_SD_SINK | RKISP1_ISP_SD_SRC)

nit: It might be just me, but this feels to me like obfuscating the
code, because it hides the fact that it's a mask. If changing this
already, could we remove this one and just OR the two bits explicitly?

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 4/4] media: staging: rkisp1: rename the field 'direction' in 'rkisp1_isp_mbus_info' to 'isp_pads_flags'
  2020-06-09 15:28 [PATCH v2 0/4] media: staging: rkisp1: bugs fixes and vars renames Dafna Hirschfeld
                   ` (2 preceding siblings ...)
  2020-06-09 15:28 ` [PATCH v2 3/4] media: staging: rkisp1: rename macros 'RKISP1_DIR_*' to 'RKISP1_ISP_SD_*' Dafna Hirschfeld
@ 2020-06-09 15:28 ` Dafna Hirschfeld
  2020-06-10 17:26   ` Tomasz Figa
  3 siblings, 1 reply; 12+ messages in thread
From: Dafna Hirschfeld @ 2020-06-09 15:28 UTC (permalink / raw)
  To: linux-media, laurent.pinchart
  Cc: dafna.hirschfeld, helen.koike, ezequiel, hverkuil, kernel, dafna3,
	sakari.ailus, linux-rockchip, mchehab, tfiga

The field 'direction' in 'struct rkisp1_isp_mbus_info' holds
the flags of the supported pads of the mbus code. Therefore
the name 'isp_pads_flags' is better.
The patch also rename a local variable 'dir' that holds such flag
to 'pad'.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
 drivers/staging/media/rkisp1/rkisp1-isp.c     | 46 +++++++++----------
 drivers/staging/media/rkisp1/rkisp1-resizer.c |  2 +-
 3 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index a6cd9fc13b3d..1dda6d53adea 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -283,7 +283,7 @@ struct rkisp1_isp_mbus_info {
 	u32 yuv_seq;
 	u8 bus_width;
 	enum rkisp1_fmt_raw_pat_type bayer_pat;
-	unsigned int direction;
+	unsigned int isp_pads_flags;
 };
 
 static inline void
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index b63f193a3ac2..c9ba7c2d820e 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -58,119 +58,119 @@ static const struct rkisp1_isp_mbus_info rkisp1_isp_formats[] = {
 	{
 		.mbus_code	= MEDIA_BUS_FMT_YUYV8_2X8,
 		.pixel_enc	= V4L2_PIXEL_ENC_YUV,
-		.direction	= RKISP1_ISP_SD_SRC,
+		.isp_pads_flags	= RKISP1_ISP_SD_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SRGGB10_1X10,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW10,
 		.bayer_pat	= RKISP1_RAW_RGGB,
 		.bus_width	= 10,
-		.direction	= RKISP1_ISP_SD_SINK_SRC,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SBGGR10_1X10,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW10,
 		.bayer_pat	= RKISP1_RAW_BGGR,
 		.bus_width	= 10,
-		.direction	= RKISP1_ISP_SD_SINK_SRC,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SGBRG10_1X10,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW10,
 		.bayer_pat	= RKISP1_RAW_GBRG,
 		.bus_width	= 10,
-		.direction	= RKISP1_ISP_SD_SINK_SRC,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SGRBG10_1X10,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW10,
 		.bayer_pat	= RKISP1_RAW_GRBG,
 		.bus_width	= 10,
-		.direction	= RKISP1_ISP_SD_SINK_SRC,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SRGGB12_1X12,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW12,
 		.bayer_pat	= RKISP1_RAW_RGGB,
 		.bus_width	= 12,
-		.direction	= RKISP1_ISP_SD_SINK_SRC,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SBGGR12_1X12,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW12,
 		.bayer_pat	= RKISP1_RAW_BGGR,
 		.bus_width	= 12,
-		.direction	= RKISP1_ISP_SD_SINK_SRC,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SGBRG12_1X12,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW12,
 		.bayer_pat	= RKISP1_RAW_GBRG,
 		.bus_width	= 12,
-		.direction	= RKISP1_ISP_SD_SINK_SRC,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SGRBG12_1X12,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW12,
 		.bayer_pat	= RKISP1_RAW_GRBG,
 		.bus_width	= 12,
-		.direction	= RKISP1_ISP_SD_SINK_SRC,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SRGGB8_1X8,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW8,
 		.bayer_pat	= RKISP1_RAW_RGGB,
 		.bus_width	= 8,
-		.direction	= RKISP1_ISP_SD_SINK_SRC,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SBGGR8_1X8,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW8,
 		.bayer_pat	= RKISP1_RAW_BGGR,
 		.bus_width	= 8,
-		.direction	= RKISP1_ISP_SD_SINK_SRC,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SGBRG8_1X8,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW8,
 		.bayer_pat	= RKISP1_RAW_GBRG,
 		.bus_width	= 8,
-		.direction	= RKISP1_ISP_SD_SINK_SRC,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_SGRBG8_1X8,
 		.pixel_enc	= V4L2_PIXEL_ENC_BAYER,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_RAW8,
 		.bayer_pat	= RKISP1_RAW_GRBG,
 		.bus_width	= 8,
-		.direction	= RKISP1_ISP_SD_SINK_SRC,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK_SRC,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_YUYV8_1X16,
 		.pixel_enc	= V4L2_PIXEL_ENC_YUV,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_YUV422_8b,
 		.yuv_seq	= RKISP1_CIF_ISP_ACQ_PROP_YCBYCR,
 		.bus_width	= 16,
-		.direction	= RKISP1_ISP_SD_SINK,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_YVYU8_1X16,
 		.pixel_enc	= V4L2_PIXEL_ENC_YUV,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_YUV422_8b,
 		.yuv_seq	= RKISP1_CIF_ISP_ACQ_PROP_YCRYCB,
 		.bus_width	= 16,
-		.direction	= RKISP1_ISP_SD_SINK,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_UYVY8_1X16,
 		.pixel_enc	= V4L2_PIXEL_ENC_YUV,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_YUV422_8b,
 		.yuv_seq	= RKISP1_CIF_ISP_ACQ_PROP_CBYCRY,
 		.bus_width	= 16,
-		.direction	= RKISP1_ISP_SD_SINK,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK,
 	}, {
 		.mbus_code	= MEDIA_BUS_FMT_VYUY8_1X16,
 		.pixel_enc	= V4L2_PIXEL_ENC_YUV,
 		.mipi_dt	= RKISP1_CIF_CSI2_DT_YUV422_8b,
 		.yuv_seq	= RKISP1_CIF_ISP_ACQ_PROP_CRYCBY,
 		.bus_width	= 16,
-		.direction	= RKISP1_ISP_SD_SINK,
+		.isp_pads_flags	= RKISP1_ISP_SD_SINK,
 	},
 };
 
@@ -566,13 +566,13 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
 				     struct v4l2_subdev_pad_config *cfg,
 				     struct v4l2_subdev_mbus_code_enum *code)
 {
-	unsigned int i, dir;
+	unsigned int i, pad;
 	int pos = 0;
 
 	if (code->pad == RKISP1_ISP_PAD_SINK_VIDEO) {
-		dir = RKISP1_ISP_SD_SINK;
+		pad = RKISP1_ISP_SD_SINK;
 	} else if (code->pad == RKISP1_ISP_PAD_SOURCE_VIDEO) {
-		dir = RKISP1_ISP_SD_SRC;
+		pad = RKISP1_ISP_SD_SRC;
 	} else {
 		if (code->index > 0)
 			return -EINVAL;
@@ -586,7 +586,7 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
 	for (i = 0; i < ARRAY_SIZE(rkisp1_isp_formats); i++) {
 		const struct rkisp1_isp_mbus_info *fmt = &rkisp1_isp_formats[i];
 
-		if (fmt->direction & dir)
+		if (fmt->isp_pads_flags & pad)
 			pos++;
 
 		if (code->index == pos - 1) {
@@ -657,7 +657,7 @@ static void rkisp1_isp_set_src_fmt(struct rkisp1_isp *isp,
 
 	src_fmt->code = format->code;
 	mbus_info = rkisp1_isp_mbus_info_get(src_fmt->code);
-	if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SRC)) {
+	if (!mbus_info || !(mbus_info->isp_pads_flags & RKISP1_ISP_SD_SRC)) {
 		src_fmt->code = RKISP1_DEF_SRC_PAD_FMT;
 		mbus_info = rkisp1_isp_mbus_info_get(src_fmt->code);
 	}
@@ -741,7 +741,7 @@ static void rkisp1_isp_set_sink_fmt(struct rkisp1_isp *isp,
 					  which);
 	sink_fmt->code = format->code;
 	mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
-	if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SINK)) {
+	if (!mbus_info || !(mbus_info->isp_pads_flags & RKISP1_ISP_SD_SINK)) {
 		sink_fmt->code = RKISP1_DEF_SINK_PAD_FMT;
 		mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
 	}
diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index 137298b77341..bb06887648a1 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -542,7 +542,7 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
 					    which);
 	sink_fmt->code = format->code;
 	mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
-	if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SRC)) {
+	if (!mbus_info || !(mbus_info->isp_pads_flags & RKISP1_ISP_SD_SRC)) {
 		sink_fmt->code = RKISP1_DEF_FMT;
 		mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
 	}
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 4/4] media: staging: rkisp1: rename the field 'direction' in 'rkisp1_isp_mbus_info' to 'isp_pads_flags'
  2020-06-09 15:28 ` [PATCH v2 4/4] media: staging: rkisp1: rename the field 'direction' in 'rkisp1_isp_mbus_info' to 'isp_pads_flags' Dafna Hirschfeld
@ 2020-06-10 17:26   ` Tomasz Figa
  0 siblings, 0 replies; 12+ messages in thread
From: Tomasz Figa @ 2020-06-10 17:26 UTC (permalink / raw)
  To: Dafna Hirschfeld
  Cc: linux-media, laurent.pinchart, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, sakari.ailus, linux-rockchip, mchehab

Hi Dafna,

On Tue, Jun 09, 2020 at 05:28:25PM +0200, Dafna Hirschfeld wrote:
> The field 'direction' in 'struct rkisp1_isp_mbus_info' holds
> the flags of the supported pads of the mbus code. Therefore
> the name 'isp_pads_flags' is better.
> The patch also rename a local variable 'dir' that holds such flag
> to 'pad'.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h  |  2 +-
>  drivers/staging/media/rkisp1/rkisp1-isp.c     | 46 +++++++++----------
>  drivers/staging/media/rkisp1/rkisp1-resizer.c |  2 +-
>  3 files changed, 25 insertions(+), 25 deletions(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index a6cd9fc13b3d..1dda6d53adea 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -283,7 +283,7 @@ struct rkisp1_isp_mbus_info {

FYI, there is some missing documentation of the fields above. If
changing this field, perhaps its documentation could be added as well?

>  	u32 yuv_seq;
>  	u8 bus_width;
>  	enum rkisp1_fmt_raw_pat_type bayer_pat;
> -	unsigned int direction;
> +	unsigned int isp_pads_flags;

nit: Wouldn't "isp_pads_mask" represent the usage more precisely?

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-06-10 19:28 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-09 15:28 [PATCH v2 0/4] media: staging: rkisp1: bugs fixes and vars renames Dafna Hirschfeld
2020-06-09 15:28 ` [PATCH v2 1/4] media: staging: rkisp1: rsz: supported formats are the isp's src formats, not sink formats Dafna Hirschfeld
2020-06-10 17:15   ` Tomasz Figa
2020-06-10 18:36     ` Dafna Hirschfeld
2020-06-10 19:00       ` Dafna Hirschfeld
     [not found]         ` <bb5576b1-7bfb-7911-004c-8fa1b4e1603c-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
2020-06-10 19:28           ` Tomasz Figa
2020-06-09 15:28 ` [PATCH v2 2/4] media: staging: rkisp1: rsz: set default format if the given format is not RKISP1_DIR_SRC Dafna Hirschfeld
2020-06-10 17:19   ` Tomasz Figa
2020-06-09 15:28 ` [PATCH v2 3/4] media: staging: rkisp1: rename macros 'RKISP1_DIR_*' to 'RKISP1_ISP_SD_*' Dafna Hirschfeld
2020-06-10 17:24   ` Tomasz Figa
2020-06-09 15:28 ` [PATCH v2 4/4] media: staging: rkisp1: rename the field 'direction' in 'rkisp1_isp_mbus_info' to 'isp_pads_flags' Dafna Hirschfeld
2020-06-10 17:26   ` Tomasz Figa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox