[PATCH v2 0/5] Visconti: Add Toshiba Visconti Video Input Interface driver
yuji2.ishikawa at toshiba.co.jp
yuji2.ishikawa at toshiba.co.jp
Wed Apr 20 06:22:06 PDT 2022
Hi, Hans
Thank you for your review and comment.
> -----Original Message-----
> From: Hans Verkuil <hverkuil at xs4all.nl>
> Sent: Wednesday, April 20, 2022 4:55 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa at toshiba.co.jp>; Mauro Carvalho Chehab
> <mchehab at kernel.org>; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> <nobuhiro1.iwamatsu at toshiba.co.jp>; Laurent Pinchart
> <laurent.pinchart at ideasonboard.com>
> Cc: linux-media at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linux-kernel at vger.kernel.org
> Subject: Re: [PATCH v2 0/5] Visconti: Add Toshiba Visconti Video Input
> Interface driver
>
> Hi Yuji,
>
> On 14/04/2022 07:35, Yuji Ishikawa wrote:
> > This series is the Video Input Interface driver for Toshiba's ARM SoC,
> Visconti[0].
> > This provides DT binding documentation, device driver, MAINTAINER fiels.
> >
> > Best regards,
> > Yuji
> >
> > [0]:
> >
> https://toshiba.semicon-storage.com/ap-en/semiconductor/product/image-
> > recognition-processors-visconti.html
> >
> >
> > dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input
> Interface bindings
> > v1 -> v2:
> > - No update
> >
> > media: platform: visconti: Add Toshiba Visconti Video Input Interface driver
> headers
> > v1 -> v2:
> > - moved driver headers to an individual patch
> >
> > media: platform: visconti: Add Toshiba Visconti Video Input Interface driver
> body
> > v1 -> v2:
> > - moved driver sources to an individual patch
> >
> > media: platform: visconti: Add Toshiba VIIF image signal processor driver
> > v1 -> v2:
> > - moved image signal processor driver to an individual patch
> >
> > MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface
> > v1 -> v2:
> > - No update
> >
> > Change in V2:
> > - moved files into individual patches to decrease patch size
>
> Thank you for your patch series.
>
> However, there are quite a few things that need more work. I'll make some high
> level guidelines here, and go into a bit more detail in some of the patches.
>
> First of all, run your patches through 'scripts/checkpatch.pl --strict' and fix the
> many warnings, errors and checks. Use common sense, sometimes a check or
> warning isn't actually valid, but the vast majority of what checkpatch spits out
> appears reasonable.
I'll execute checkpatch.pl with --strict option.
>
> Another thing I noticed is code like this:
>
> + if (param->r_cr_in_offset > HWD_VIIF_CSC_MAX_OFFSET)
> + return -EINVAL;
> +
> + if (param->g_y_in_offset > HWD_VIIF_CSC_MAX_OFFSET)
> + return -EINVAL;
> +
> + if (param->b_cb_in_offset > HWD_VIIF_CSC_MAX_OFFSET)
> + return -EINVAL;
> +
> + if (param->r_cr_out_offset > HWD_VIIF_CSC_MAX_OFFSET)
> + return -EINVAL;
> +
> + if (param->g_y_out_offset > HWD_VIIF_CSC_MAX_OFFSET)
> + return -EINVAL;
> +
> + if (param->b_cb_out_offset > HWD_VIIF_CSC_MAX_OFFSET)
> + return -EINVAL;
>
> This can easily be combined into a single if:
>
> if (param->r_cr_in_offset > HWD_VIIF_CSC_MAX_OFFSET ||
> param->g_y_in_offset > HWD_VIIF_CSC_MAX_OFFSET ||
> param->b_cb_in_offset > HWD_VIIF_CSC_MAX_OFFSET
> ||
> param->r_cr_out_offset > HWD_VIIF_CSC_MAX_OFFSET
> ||
> param->g_y_out_offset > HWD_VIIF_CSC_MAX_OFFSET
> ||
> param->b_cb_out_offset >
> HWD_VIIF_CSC_MAX_OFFSET)
> return -EINVAL;
>
> Easier to read and a lot shorter.
I'll fix them
>
> Another thing to avoid is mixing lower and upper case in function names.
> A lot of functions have this prefix: 'hwd_VIIF_'. Just change that to
> 'hwd_viif_': that's much easier on the eyes.
I'll fix them.
I'm also wondering if the common prefix hwd_viif_ is acceptable for the identifiers in Visconti VIIIF driver.
> I also see a fair amount of code that is indented very far to the right.
> Often due to constructs like this:
>
> if (test) {
> // lots of code
> }
> return ret;
>
> Which can be changed to:
>
> if (!test)
> return ret;
> // lots of code
> return ret;
>
> The same can also happen in a for/while loop where you can just 'continue'
> instead of 'return'.
>
> This makes the code easier to read and review.
Yes, the indents are too deep.
I'll try fix them.
> It doesn't look like this driver uses the media controller API. This is probably
> something you want to look into, esp. in combination with libcamera support
> (https://libcamera.org/). I've added Laurent to this, since he's the expert on
> this.
Thank you for great advice. I wanted to know how to control/aggregate functions of VIIF.
As I'm completely new to media controller API, I'll check the documents first.
Regards,
Yuji
> Regards,
>
> Hans
>
> >
> > Yuji Ishikawa (5):
> > dt-bindings: media: platform: visconti: Add Toshiba Visconti Video
> > Input Interface bindings
> > media: platform: visconti: Add Toshiba Visconti Video Input Interface
> > driver headers
> > media: platform: visconti: Add Toshiba Visconti Video Input Interface
> > driver body
> > media: platform: visconti: Add Toshiba VIIF image signal processor
> > driver
> > MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface
> >
> > .../bindings/media/toshiba,visconti-viif.yaml | 103 +
> > MAINTAINERS | 2 +
> > drivers/media/platform/Kconfig | 2 +
> > drivers/media/platform/Makefile | 4 +
> > drivers/media/platform/visconti/Kconfig | 9 +
> > drivers/media/platform/visconti/Makefile | 9 +
> > drivers/media/platform/visconti/hwd_viif.c | 2233 ++++++++++
> > drivers/media/platform/visconti/hwd_viif.h | 1776 ++++++++
> > .../media/platform/visconti/hwd_viif_csi2rx.c | 767 ++++
> > .../platform/visconti/hwd_viif_internal.h | 361 ++
> > .../media/platform/visconti/hwd_viif_l1isp.c | 3769
> +++++++++++++++++
> > .../media/platform/visconti/hwd_viif_reg.h | 2802 ++++++++++++
> > drivers/media/platform/visconti/viif.c | 2384 +++++++++++
> > drivers/media/platform/visconti/viif.h | 134 +
> > include/uapi/linux/visconti_viif.h | 1683 ++++++++
> > 15 files changed, 16038 insertions(+) create mode 100644
> > Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
> > create mode 100644 drivers/media/platform/visconti/Kconfig
> > create mode 100644 drivers/media/platform/visconti/Makefile
> > create mode 100644 drivers/media/platform/visconti/hwd_viif.c
> > create mode 100644 drivers/media/platform/visconti/hwd_viif.h
> > create mode 100644 drivers/media/platform/visconti/hwd_viif_csi2rx.c
> > create mode 100644
> > drivers/media/platform/visconti/hwd_viif_internal.h
> > create mode 100644 drivers/media/platform/visconti/hwd_viif_l1isp.c
> > create mode 100644 drivers/media/platform/visconti/hwd_viif_reg.h
> > create mode 100644 drivers/media/platform/visconti/viif.c
> > create mode 100644 drivers/media/platform/visconti/viif.h
> > create mode 100644 include/uapi/linux/visconti_viif.h
> >
More information about the linux-arm-kernel
mailing list