RE: [PATCH v2 0/5] Visconti: Add Toshiba Visconti Video Input Interface driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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(石川 悠司 ○RDC□AITC○EA開)
> <yuji2.ishikawa@xxxxxxxxxxxxx>; Mauro Carvalho Chehab
> <mchehab@xxxxxxxxxx>; iwamatsu nobuhiro(岩松 信洋 □SWC◯ACT)
> <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
> >




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux