Message-ID: <6dce1ca0-78bf-4a4a-bdd6-ff6647eae6b6@xs4all.nl>
Date: Fri, 31 May 2024 13:01:52 +0200
From: Hans Verkuil <hverkuil@...all.nl>
To: Yuji Ishikawa <yuji2.ishikawa@...hiba.co.jp>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
 Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.co.jp>
Cc: linux-media@...r.kernel.org, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 0/6] Add Toshiba Visconti Video Input Interface driver

Hi Yuji,

On 24/04/2024 04:42, Yuji Ishikawa wrote:
> This series is the Video Input Interface driver
> for Toshiba's ARM SoC, Visconti.
> This provides DT binding documentation,
> device driver, documentation and MAINTAINER files.
> 
> A visconti VIIF driver instance exposes
> 1 media control device file, 3 video device files for capture
> and 2 video device files for controlling image signal processor.
> Detailed HW/SW are described in documentation directory.
> The VIIF hardware has CSI2 receiver,
> image signal processor and DMAC inside.
> The subdevice for image signal processor provides
> vendor specific V4L2 controls.
> 
> The device driver depends on two other drivers under development;
> clock framework driver and IOMMU driver.
> Corresponding features will be added later.
> 
> Best regards,
> Yuji

I commented on patches 3 and 4.

I also ran this series through my build scripts, and it did found a few small
warnings:

kerneldoc: WARNINGS:

drivers/media/platform/toshiba/visconti/viif.h:386: warning: Excess struct member 'subdevs' description in 'viif_device'
drivers/media/platform/toshiba/visconti/viif.h:386: warning: Excess struct member 'asds' description in 'viif_device'
drivers/media/platform/toshiba/visconti/viif.h:386: warning: Excess struct member 'sd' description in 'viif_device'
drivers/media/platform/toshiba/visconti/viif_common.h:30: warning: Function parameter or struct member 'bayer_pattern' not described in 'viif_mbus_format'
drivers/media/platform/toshiba/visconti/viif_common.h:30: warning: Function parameter or struct member 'is_bayer' not described in 'viif_mbus_format'

Should be trivial to fix.

Regards,

	Hans

> 
> Changelog v2:
> - Resend v1 because a patch exceeds size limit.
> 
> Changelog v3:
> - Add documentation to describe SW and HW
> - Adapted to media control framework
> - Introduced ISP subdevice, capture device
> - Remove private IOCTLs and add vendor specific V4L2 controls
> - Change function name avoiding camelcase and uppercase letters
> 
> Changelog v4:
> - Split patches because a patch exceeds size limit
> - fix dt-bindings document
> - stop specifying ID numbers for driver instance explicitly at device tree
> - use pm_runtime to trigger initialization of HW
>   along with open/close of device files.
> - add a entry for a header file at MAINTAINERS file
> 
> Changelog v5:
> - Fix coding style problem in viif.c (patch 2/6)
> 
> Changelog v6:
> - add register definition of BUS-IF and MPU in dt-bindings
> - add CSI2RX subdevice (separeted from ISP subdevice)
> - change directory layout (moved to media/platform/toshiba/visconti)
> - change source file layout (removed hwd_xxxx.c)
> - pointer to userland memory is removed from uAPI parameters
> - change register access (from struct style to macro style)
> - remove unused macros
> 
> Changelog v7:
> - remove redundant "bindings" from header and description text
> - fix multiline text of "description"
> - change "compatible" to "visconti5-viif"
> - explicitly define allowed properties for port::endpoint
> - remove unused variables
> - update kerneldoc comments
> - update references to headers
> 
> Changelog v8:
> - rename bindings description file
> - remove/simplify items in bindings
> - update operations around v4l2_async_notifier
> - use v4l2_async_connection instead of v4l2_async_subdev
> - use dev_err_probe()
> - better error handling at probe
> - remove redundant mutex
> - add V4L2_CTRL_TYPE_VISCONTI_ISP constant
> 
> Changelog v9:
> - dictionary ordering of dt-bindings properties
> - applied sparce checker
> - call div64_u64 for 64bit division
> - rebase to media_staging tree
> - fix warning for cast between ptr and dma_addr_t
> 
> Changelog v10:
> - add an independent entry in MAINTAINERS
> - add paddings to uAPI structs
> - use parameter buffer to control ISP (instead of vendor specific controls)
> 
> Yuji Ishikawa (6):
>   dt-bindings: media: platform: visconti: Add Toshiba Visconti Video
>     Input Interface
>   media: videodev2.h: add visconti viif meta buffer format
>   media: platform: visconti: Add Toshiba Visconti Video Input Interface
>     driver
>   media: platform: visconti: add streaming interface for ISP parameters
>     and status
>   documentation: media: add documentation for Toshiba Visconti Video
>     Input Interface driver
>   MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface
> 
>  .../admin-guide/media/v4l-drivers.rst         |    1 +
>  .../admin-guide/media/visconti-viif.dot       |   18 +
>  .../admin-guide/media/visconti-viif.rst       |  252 ++
>  .../media/toshiba,visconti5-viif.yaml         |  105 +
>  .../userspace-api/media/v4l/meta-formats.rst  |    1 +
>  .../media/v4l/metafmt-visconti-viif.rst       |   48 +
>  MAINTAINERS                                   |   11 +
>  drivers/media/platform/Kconfig                |    1 +
>  drivers/media/platform/Makefile               |    1 +
>  drivers/media/platform/toshiba/Kconfig        |    6 +
>  drivers/media/platform/toshiba/Makefile       |    2 +
>  .../media/platform/toshiba/visconti/Kconfig   |   19 +
>  .../media/platform/toshiba/visconti/Makefile  |    8 +
>  .../media/platform/toshiba/visconti/viif.c    |  664 ++++++
>  .../media/platform/toshiba/visconti/viif.h    |  398 ++++
>  .../platform/toshiba/visconti/viif_capture.c  | 1472 ++++++++++++
>  .../platform/toshiba/visconti/viif_capture.h  |   22 +
>  .../platform/toshiba/visconti/viif_common.c   |  239 ++
>  .../platform/toshiba/visconti/viif_common.h   |   40 +
>  .../platform/toshiba/visconti/viif_csi2rx.c   |  657 ++++++
>  .../platform/toshiba/visconti/viif_csi2rx.h   |   24 +
>  .../toshiba/visconti/viif_csi2rx_regs.h       |  102 +
>  .../platform/toshiba/visconti/viif_isp.c      | 1191 ++++++++++
>  .../platform/toshiba/visconti/viif_isp.h      |   24 +
>  .../platform/toshiba/visconti/viif_params.c   | 2026 +++++++++++++++++
>  .../platform/toshiba/visconti/viif_params.h   |   19 +
>  .../platform/toshiba/visconti/viif_regs.h     |  721 ++++++
>  .../platform/toshiba/visconti/viif_stats.c    |  334 +++
>  .../platform/toshiba/visconti/viif_stats.h    |   14 +
>  include/uapi/linux/videodev2.h                |    4 +
>  include/uapi/linux/visconti_viif.h            | 1921 ++++++++++++++++
>  31 files changed, 10345 insertions(+)
>  create mode 100644 Documentation/admin-guide/media/visconti-viif.dot
>  create mode 100644 Documentation/admin-guide/media/visconti-viif.rst
>  create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti5-viif.yaml
>  create mode 100644 Documentation/userspace-api/media/v4l/metafmt-visconti-viif.rst
>  create mode 100644 drivers/media/platform/toshiba/Kconfig
>  create mode 100644 drivers/media/platform/toshiba/Makefile
>  create mode 100644 drivers/media/platform/toshiba/visconti/Kconfig
>  create mode 100644 drivers/media/platform/toshiba/visconti/Makefile
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif.c
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif.h
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_capture.c
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_capture.h
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_common.c
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_common.h
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_csi2rx.c
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_csi2rx.h
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_csi2rx_regs.h
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_isp.c
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_isp.h
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_params.c
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_params.h
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_regs.h
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_stats.c
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_stats.h
>  create mode 100644 include/uapi/linux/visconti_viif.h
> 

Powered by blists - more mailing lists