| Message ID | 20220414053528.31460-1-yuji2.ishikawa@toshiba.co.jp (mailing list archive) |
|---|---|
| Headers | show |
| Series | Visconti: Add Toshiba Visconti Video Input Interface driver | expand |
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. 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. 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 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. 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. 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 >
Hi, Hans Thank you for your review and comment. > -----Original Message----- > From: Hans Verkuil <hverkuil@xs4all.nl> > Sent: Wednesday, April 20, 2022 4:55 PM > To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) > <yuji2.ishikawa@toshiba.co.jp>; Mauro Carvalho Chehab > <mchehab@kernel.org>; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ) > <nobuhiro1.iwamatsu@toshiba.co.jp>; Laurent Pinchart > <laurent.pinchart@ideasonboard.com> > Cc: linux-media@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@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 > >
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 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