Hi, Hans Thank you for your review and comment. > -----Original Message----- > From: Hans Verkuil <hverkuil@xxxxxxxxx> > Sent: Wednesday, April 20, 2022 4:55 PM > To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) > <yuji2.ishikawa@xxxxxxxxxxxxx>; Mauro Carvalho Chehab > <mchehab@xxxxxxxxxx>; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ) > <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>; Laurent Pinchart > <laurent.pinchart@xxxxxxxxxxxxxxxx> > Cc: linux-media@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx > 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 > >
