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

  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
  

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
> >