Re: [PATCH v3 07/10] media: staging: rkisp1: rsz: enumerate the formats on the src pad according to the capture

[Tomasz Figa <tfiga@chromium.org>]

Hi Dafna,

On Sat, Aug 29, 2020 at 7:53 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
>
>
> Am 04.08.20 um 01:50 schrieb Helen Koike:
> > Hi Dafna,
> >
> > On 7/23/20 10:20 AM, Dafna Hirschfeld wrote:
> >> Currently the resizer outputs the same media bus format
> >> as the input. This is wrong since the resizer is also used
> >> to downscale YUV formats. This patch changes the enumeration
> >> of the supported formats. The supported formats on the sink pad
> >> should be taken from the isp entity and the supported formats on
> >> the source pad should be taken from the capture entity.
> >>
> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >> ---
> >>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 41 ++++++++++++-------
> >>   1 file changed, 26 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >> index 066d22096a7d..4e87c6f3f732 100644
> >> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >> @@ -433,24 +433,35 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
> >>   {
> >>      struct rkisp1_resizer *rsz =
> >>              container_of(sd, struct rkisp1_resizer, sd);
> >> -    struct v4l2_subdev_pad_config dummy_cfg;
> >> -    u32 pad = code->pad;
> >>      int ret;
> >>
> >> -    if (rsz->id == RKISP1_SELFPATH) {
> >> -            if (code->index > 0)
> >> -                    return -EINVAL;
> >> -            code->code = MEDIA_BUS_FMT_YUYV8_2X8;
> >> -            return 0;
> >> +    /* supported mbus codes on the sink pad are the same as isp src pad */
> >> +    if (code->pad == RKISP1_RSZ_PAD_SINK) {
> >> +            struct v4l2_subdev_pad_config dummy_cfg;
> >> +            u32 pad = code->pad;
> >> +
> >> +            /*
> >> +             * the sp capture doesn't support bayer formats so the sp resizer
> >> +             * supports only yuv422
> >> +             */
> >> +            if (rsz->id == RKISP1_SELFPATH) {
> >> +                    if (code->index > 0)
> >> +                            return -EINVAL;
> >> +                    code->code = MEDIA_BUS_FMT_YUYV8_2X8;
> >> +                    return 0;
> >> +            }
> >> +            code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
> >> +            ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
> >> +                                   &dummy_cfg, code);
> >> +
> >> +            /* restore pad */
> >> +            code->pad = pad;
> >> +    } else {
> >> +            /* supported mbus codes on the src are the same as in the capture */
> >> +            struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
> >> +
> >> +            ret = rkisp1_cap_enum_mbus_codes(cap, code);
> >>      }
> >> -
> >> -    /* 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);
> >> -
> >> -    /* restore pad */
> >> -    code->pad = pad;
> >>      return ret;
> >>   }
> >>
> >>
> >
> > I think you can reduce indentation by doing:
> >
> >       /* supported mbus codes on the src are the same as in the capture */
> >       if (code->pad == RKISP1_RSZ_PAD_SRC)
> >               return kisp1_cap_enum_mbus_codes(
> >                       &rsz->rkisp1->capture_devs[rsz->id], code);
> >
> >       // ... rest of the code for sink pad without an else statement
>
> I think it will make the code a bit less clear, since one should note that there is a return statement inside the first 'if'
> and conclude that the rest is the 'else' case. With 'if-else' it is more clear what code run under which condition.

I guess it might be a bit subjective, but it is a common kernel coding
pattern and usually preferred over if/else blocks. It allows quickly
pruning various edge cases and leaving the rest (and usually most
complex part) of the function easier to follow, because of more
visible assumptions. After all, one usually reads a function from the
top to bottom.

Best regards,
Tomasz