| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <TYAPR01MB62015A90154CDC75E6EF39D792F59@TYAPR01MB6201.jpnprd01.prod.outlook.com>
Date: Wed, 20 Apr 2022 13:22:06 +0000
From: <yuji2.ishikawa@...hiba.co.jp>
To: <hverkuil@...all.nl>, <mchehab@...nel.org>,
<nobuhiro1.iwamatsu@...hiba.co.jp>,
<laurent.pinchart@...asonboard.com>
CC: <linux-media@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 0/5] Visconti: Add Toshiba Visconti Video Input
Interface driver
Hi, Hans
Thank you for your review and comment.
> -----Original Message-----
> From: Hans Verkuil <hverkuil@...all.nl>
> Sent: Wednesday, April 20, 2022 4:55 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@...hiba.co.jp>; Mauro Carvalho Chehab
> <mchehab@...nel.org>; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> <nobuhiro1.iwamatsu@...hiba.co.jp>; Laurent Pinchart
> <laurent.pinchart@...asonboard.com>
> Cc: linux-media@...r.kernel.org; linux-arm-kernel@...ts.infradead.org;
> linux-kernel@...r.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
> >
Powered by blists - more mailing lists