[PATCH v7 0/5] Add Toshiba Visconti Video Input Interface driver

[PATCH v7 0/5] Add Toshiba Visconti Video Input Interface driver

[Yuji Ishikawa]

This series is the Video Input Interface driver
for Toshiba's ARM SoC, Visconti[0].
This provides DT binding documentation,
device driver, documentation and MAINTAINER files.

A visconti VIIF driver instance exposes
1 media control device file and 3 video device files
for a VIIF hardware.
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

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

Yuji Ishikawa (5):
  dt-bindings: media: platform: visconti: Add Toshiba Visconti Video
    Input Interface
  media: platform: visconti: Add Toshiba Visconti Video Input Interface
    driver
  media: add V4L2 vendor specific control handlers
  documentation: media: add documentation for Toshiba Visconti Video
    Input Interface driver
  MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface

 .../bindings/media/toshiba,visconti-viif.yaml |  108 +
 .../driver-api/media/drivers/index.rst        |    1 +
 .../media/drivers/visconti-viif.rst           |  462 +++
 MAINTAINERS                                   |    4 +
 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   |   18 +
 .../media/platform/toshiba/visconti/Makefile  |    8 +
 .../media/platform/toshiba/visconti/viif.c    |  681 ++++
 .../media/platform/toshiba/visconti/viif.h    |  375 ++
 .../platform/toshiba/visconti/viif_capture.c  | 1485 +++++++
 .../platform/toshiba/visconti/viif_capture.h  |   22 +
 .../platform/toshiba/visconti/viif_common.c   |  199 +
 .../platform/toshiba/visconti/viif_common.h   |   38 +
 .../platform/toshiba/visconti/viif_controls.c | 3407 +++++++++++++++++
 .../platform/toshiba/visconti/viif_controls.h |   18 +
 .../platform/toshiba/visconti/viif_csi2rx.c   |  684 ++++
 .../platform/toshiba/visconti/viif_csi2rx.h   |   24 +
 .../toshiba/visconti/viif_csi2rx_regs.h       |  102 +
 .../platform/toshiba/visconti/viif_isp.c      | 1258 ++++++
 .../platform/toshiba/visconti/viif_isp.h      |   24 +
 .../platform/toshiba/visconti/viif_regs.h     |  716 ++++
 include/uapi/linux/v4l2-controls.h            |    6 +
 include/uapi/linux/visconti_viif.h            | 1800 +++++++++
 26 files changed, 11450 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
 create mode 100644 Documentation/driver-api/media/drivers/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_controls.c
 create mode 100644 drivers/media/platform/toshiba/visconti/viif_controls.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_regs.h
 create mode 100644 include/uapi/linux/visconti_viif.h

-- 
2.25.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 1/5] dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface

[Yuji Ishikawa]

Adds the Device Tree binding documentation that allows to describe
the Video Input Interface found in Toshiba Visconti SoCs.

Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>
Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
---
Changelog v2:
- no change

Changelog v3:
- no change

Changelog v4:
- fix style problems at the v3 patch
- remove "index" member
- update example

Changelog v5:
- no change

Changelog v6:
- add register definition of BUS-IF and MPU

Changelog v7:
- remove trailing "bindings" from commit header message
- remove trailing "Device Tree Bindings" from title
- fix text wrapping of description
- change compatible to visconti5-viif
- explicitly define allowed properties for port::endpoint

 .../bindings/media/toshiba,visconti-viif.yaml | 108 ++++++++++++++++++
 1 file changed, 108 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml

diff --git a/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml b/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
new file mode 100644
index 000000000..8377f75b3
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
@@ -0,0 +1,108 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/toshiba,visconti-viif.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Toshiba Visconti5 SoC Video Input Interface
+
+maintainers:
+  - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
+
+description: |-
+  Toshiba Visconti5 SoC Video Input Interface (VIIF) receives MIPI CSI2 video
+  stream, processes the stream with image signal processors (L1ISP, L2ISP),
+  then stores pictures to main memory.
+
+properties:
+  compatible:
+    const: toshiba,visconti5-viif
+
+  reg:
+    items:
+      - description: registers for capture control
+      - description: registers for CSI2 receiver control
+      - description: registers for bus interface unit control
+      - description: registers for Memory Protection Unit
+
+  interrupts:
+    items:
+      - description: Sync Interrupt
+      - description: Status (Error) Interrupt
+      - description: CSI2 Receiver Interrupt
+      - description: L1ISP Interrupt
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    unevaluatedProperties: false
+    description: Input port, single endpoint describing the CSI-2 transmitter.
+
+    properties:
+      endpoint:
+        $ref: video-interfaces.yaml#
+        additionalProperties: false
+
+        required: ["bus-type", "clock-noncontinuous", "link-frequencies", "remote-endpoint"]
+
+        properties:
+          data-lanes:
+            description: VIIF supports 2 or 4 data lanes
+            $ref: /schemas/types.yaml#/definitions/uint32-array
+            minItems: 1
+            maxItems: 4
+            items:
+              minimum: 1
+              maximum: 4
+
+          clock-lanes:
+            description: VIIF supports 1 clock lane
+            const: 0
+
+          bus-type: true
+          clock-noncontinuous: true
+          link-frequencies: true
+          remote-endpoint: true
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        viif@1c000000 {
+            compatible = "toshiba,visconti5-viif";
+            reg = <0 0x1c000000 0 0x6000>,
+                  <0 0x1c008000 0 0x400>,
+                  <0 0x1c00E000 0 0x1000>,
+                  <0 0x2417A000 0 0x1000>;
+            interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>;
+
+            port {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                csi_in0: endpoint {
+                    remote-endpoint = <&imx219_out0>;
+                    bus-type = <4>;
+                    data-lanes = <1 2>;
+                    clock-lanes = <0>;
+                    clock-noncontinuous;
+                    link-frequencies = /bits/ 64 <456000000>;
+                };
+            };
+        };
+    };
-- 
2.25.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 4/5] documentation: media: add documentation for Toshiba Visconti Video Input Interface driver

[Yuji Ishikawa]

Added basic description of Video Input Interface driver of
Toshiba Visconti architecture.
It includes hardware organization, structure of the driver
and description of vendor specific V4L2 controls
to configure the embedded image signal processor.

Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>
---
Changelog v3:
- Newly add documentation to describe SW and HW

Changelog v4:
- no change

Changelog v5:
- no change

Changelog v6:
- add description of CSI2RX subdevice
- add ordering of ioctl(S_FMT) and ioctl(S_EXT_CTRLS)

Changelog v7:
- no change

 .../driver-api/media/drivers/index.rst        |   1 +
 .../media/drivers/visconti-viif.rst           | 462 ++++++++++++++++++
 2 files changed, 463 insertions(+)
 create mode 100644 Documentation/driver-api/media/drivers/visconti-viif.rst

diff --git a/Documentation/driver-api/media/drivers/index.rst b/Documentation/driver-api/media/drivers/index.rst
index c4123a16b..5592bd99a 100644
--- a/Documentation/driver-api/media/drivers/index.rst
+++ b/Documentation/driver-api/media/drivers/index.rst
@@ -24,6 +24,7 @@ Video4Linux (V4L) drivers
 	sh_mobile_ceu_camera
 	tuners
 	vimc-devel
+	visconti-viif
 	zoran
 	ccs/ccs
 
diff --git a/Documentation/driver-api/media/drivers/visconti-viif.rst b/Documentation/driver-api/media/drivers/visconti-viif.rst
new file mode 100644
index 000000000..fd2480cbd
--- /dev/null
+++ b/Documentation/driver-api/media/drivers/visconti-viif.rst
@@ -0,0 +1,462 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+============================================
+Visconti Video Input Interface (VIIF) Driver
+============================================
+
+Overview
+========
+
+The Visconti VIIF Hardware
+--------------------------
+
+The Visconti Video Input Interface (VIIF) hardware is  a proprietary videocapture device of Toshiba.
+Following function modules are integrated:
+
+* MIPI CSI2 receiver (CSI2RX)
+* L1 Image Signal Processor (L1ISP)
+
+  * Correction, enhancement, adjustment on RAW pictures.
+
+* L2 Image Signal Processor (L2ISP)
+
+  * Lens distortion correction
+  * Scaling
+  * Cropping
+
+* Video DMAC
+
+  * format picture (RGB, YUV, Grayscale, ...)
+  * write picture into DRAM
+
+Visconti5 SoC has two VIIF hardware instances.
+
+software architecture
+---------------------
+
+The Visconti VIIF driver is composed of following components:
+
+* (image sensor driver)
+* MIPI CSI2 receiver subdevice driver
+
+  * corresponding to CSI2RX
+
+* Visconti ISP subdevice driver
+
+  * corresponding to L1ISP, L2ISP (Lens distortion correction, Scaling)
+
+* Visconti Capture V4L2 device driver
+
+  * corresponding to L2ISP (Cropping) and Video DMAC
+  * multiple output videobuf queues
+
+    * main path0 (RGB, YUV, Grayscale, ...)
+    * main path1 (RGB, YUV, Grayscale, ...)
+    * sub path (RAW picture)
+
+::
+
+  +-----------+      +-----------+     +----------------+       +-------------------------+
+  | Sensor    |      | CSI2RX    |     | ISP            |       | Capture MAIN PATH0      |
+  | subdevice | ---- | subdevice | --- | subdevice      | --+-- | V4L2 device             |
+  | (IMX219)  |      | (CSI2RX)  |     | (L1ISP, L2ISP) |   |   | (L2ISP crop, VideoDMAC) |
+  +-----------+      +-----------+     +----------------+   |   +-------------------------+
+                                                            |
+                                                            |   +-------------------------+
+                                                            |   | Capture MAIN PATH1      |
+                                                            +-- | V4L2 device             |
+                                                            |   | (L2ISP crop, VideoDMAC) |
+                                                            |   +-------------------------+
+                                                            |
+                                                            |   +-------------------------+
+                                                            |   | Capture SUB PATH        |
+                                                            +-- | V4L2 device             |
+                                                                | (VideoDMAC)             |
+                                                                +-------------------------+
+
+
+The VIIF driver provides following device nodes for Visconti5 SoC:
+
+* VIIF0
+
+  * /dev/media0
+  * /dev/video0 (main path0)
+  * /dev/video1 (main path1)
+  * /dev/video2 (sub path)
+
+* VIIF1
+
+  * /dev/media1
+  * /dev/video3 (main path0)
+  * /dev/video4 (main path1)
+  * /dev/video5 (sub path)
+
+Use of coherent memory
+----------------------
+
+Visconti5 SoC has two independent DDR SDRAM controllers.
+Each controller is mapped to 36bit address space.
+
+Accelerator bus masters have two paths to access memory;
+one is directly connected to SDRAM controller,
+the another is connected via a cache coherency bus
+which keeps coherency among CPUs.
+
+From acclerators and CPUs, the address map is following:
+
+* 0x0_8000_0000 DDR0 direct access
+* 0x4_8000_0000 DDR0 coherency bus
+* 0x8_8000_0000 DDR1 direct access
+* 0xC_8000_0000 DDR1 coherency bus
+
+The base address can be specified with "memory" and "reserved-memory" elements
+in a device tree description.
+It's not recommended to mix direct address and coherent address.
+
+The Visconti5 VIIF driver always use only direct address to configure Video DMACs of the hardware.
+This design is to avoid great performance loss at coherency bus caused by massive memory access.
+You should not put the dma_coherent attribute to viif element in device tree.
+Cache operations are done automatically by videobuf2 driver.
+
+Tested environment
+------------------
+
+The Visconti VIIF driver was tested with following items:
+
+* IMX219 image sensor
+* IMX335 image sensor
+
+IOCTLs
+======
+
+Following public IOCTLs are supported
+
+* VIDIOC_QUERYCAP
+* VIDIOC_ENUM_FMT
+* VIDIOC_TRY_FMT
+* VIDIOC_S_FMT
+* VIDIOC_G_FMT
+* VIDIOC_ENUM_FRAMESIZES
+* VIDIOC_G_EXT_CTRLS
+* VIDIOC_S_EXT_CTRLS
+* VIDIOC_REQBUFS
+* VIDIOC_QUERYBUF
+* VIDIOC_QBUF
+* VIDIOC_EXPBUF
+* VIDIOC_DQBUF
+* VIDIOC_CREATE_BUFS
+* VIDIOC_PREPARE_BUF
+* VIDIOC_STREAMON
+* VIDIOC_STREAMOFF
+
+Vendor specific v4l2 controls
+(except for V4L2_CID_VISCONTI_VIIF_MAIN_SET_RAWPACK_MODE and
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_INPUT_MODE) should be called
+after ioctl(S_FMT) because setting the frame format may affect
+valid range of parameters of the controls.
+
+Vendor specific v4l2 controls
+=============================
+
+.. _V4L2_CID_VISCONTI_VIIF_MAIN_SET_RAWPACK_MODE:
+
+V4L2_CID_VISCONTI_VIIF_MAIN_SET_RAWPACK_MODE
+--------------------------------------------
+
+This control sets the format to pack multiple RAW pixel values into a word.
+
+This control accepts a __u32 value defined as `enum viif_rawpack_mode`.
+
+This control should be set before ioctl(S_FMT) and should not be changed after that.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_INPUT_MODE:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_INPUT_MODE
+--------------------------------------------
+
+This control sets L1ISP preprocessing mode for RAW input images.
+
+This control accepts a `struct viif_l1_input_mode_config` instance.
+
+This control should be set before ioctl(S_FMT) and should not be changed after that.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_RGB_TO_Y_COEF:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_RGB_TO_Y_COEF
+-----------------------------------------------
+
+This control sets parameters to yield Y value from RGB pixel values.
+
+This control accepts a `struct viif_l1_rgb_to_y_coef_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AG_MODE:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AG_MODE
+-----------------------------------------
+
+This control sets rules of generating analog gains for each feature in L1ISP.
+Related features are:
+
+* Optical Black Clamp Correction (OBCC)
+* Defect Pixel Correction (DPC)
+* RAW Color Noise Reduction (RCNR)
+* Lens Shading Correction (LSC)
+* Color matrix correction (MPRO)
+* Image quality adjustment (VPRO)
+
+The base gain is set with V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AG control.
+
+This control accepts a `struct viif_l1_ag_mode_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AG:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AG
+------------------------------------
+
+This control sets base analog gain commonly used in L1ISP features.
+Analog gain for each L1ISP feature is generated
+from the base analog gain and a configuration via V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AG_MODE control.
+
+This control accepts a `struct viif_l1_ag_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRE:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRE
+--------------------------------------
+
+This controls sets parameters for HDR Expansion feature.
+
+This control accepts a `struct viif_l1_hdre_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_IMG_EXTRACTION:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_IMG_EXTRACTION
+------------------------------------------------
+
+This control sets black level parameters for L1ISP inputs.
+
+This control accepts a `struct viif_l1_img_extraction_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_DPC:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_DPC
+-------------------------------------
+
+This control sets parameters for Defect Pixel Correction.
+
+This control accepts a `struct viif_l1_dpc_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_PRESET_WHITE_BALANCE:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_PRESET_WHITE_BALANCE
+------------------------------------------------------
+
+This control sets parameters for white balance.
+
+This control accepts a `struct viif_l1_preset_white_balance_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_RAW_COLOR_NOISE_REDUCTION:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_RAW_COLOR_NOISE_REDUCTION
+-----------------------------------------------------------
+
+This control sets parameters for RAW color noise reduction (RCNR) feature.
+
+This control accepts a `struct viif_l1_raw_color_noise_reduction_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRS:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRS
+--------------------------------------
+
+This control sets parameters for HDR synthesis.
+
+This control accepts a `struct viif_l1_hdrs_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_BLACK_LEVEL_CORRECTION:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_BLACK_LEVEL_CORRECTION
+--------------------------------------------------------
+
+This control sets parameters for black level correction feature.
+
+This control accepts a `struct viif_l1_black_level_correction_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_LSC:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_LSC
+-------------------------------------
+
+This control sets parameters for Lens Shading Correction feature.
+L1ISP supports 2 correction methods:
+
+* parabola shading
+* grid shading
+
+This control accepts a `struct viif_l1_lsc_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_MAIN_PROCESS:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_MAIN_PROCESS
+----------------------------------------------
+
+This controls sets parameter for the MAIN PROCESS feature which is composed of:
+
+* demosaic
+* color matrix correction
+
+This control accepts a `struct viif_l1_main_process_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AWB:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AWB
+-------------------------------------
+
+This control sets parameter for auto white balance feature.
+
+This control accepts a `struct viif_l1_awb_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_LOCK_AWB_GAIN:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_LOCK_AWB_GAIN
+-------------------------------------------
+
+This control requests enable/disable of lock for AWB gain.
+
+This control accepts a u32 value; 0 for disable lock, 1 for enable lock.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRC:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRC
+--------------------------------------
+
+This control sets parameter for HDR Compression feature.
+
+This control accepts a `struct viif_l1_hdrc_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRC_LTM:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRC_LTM
+------------------------------------------
+
+This control sets parameter for HDR Compression Local Tone Mapping feature.
+
+This control accepts a `struct viif_l1_hdrc_ltm_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_GAMMA:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_GAMMA
+---------------------------------------
+
+This control sets parameter for gamma correction at L1ISP.
+
+This control accepts a `struct viif_l1_gamma_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_IMG_QUALITY_ADJUSTMENT:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_IMG_QUALITY_ADJUSTMENT
+--------------------------------------------------------
+
+This control sets parameter for VPRO feature which is composed of:
+
+* luminance adjustment:
+
+ * brightness adjustment
+ * linear contrast adjusment
+ * nonlinear contrast adjustment
+ * luminance noise reduction
+ * edge enhancement
+
+* chroma adjustment:
+
+ * chroma suppression
+ * color level adjustment
+ * chroma noise reduction
+ * coring suppression
+ * edge chroma suppression
+ * color noise reduction
+
+This control accepts a `struct viif_l1_img_quality_adjustment_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AVG_LUM_GENERATION:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AVG_LUM_GENERATION
+----------------------------------------------------
+
+This control sets parameter for average luminance statistics feature.
+
+This control accepts a `struct viif_l1_avg_lum_generation_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_UNDIST:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_UNDIST
+----------------------------------------
+
+This control sets parameter for the lens undistortion feature of L2ISP.
+Lens undistortion parameters are defined as either or combination of polinomial parameter and grid table.
+
+This control accepts a `struct viif_l2_undist_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_ROI:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_ROI
+-------------------------------------
+
+This control sets dimensions of intermediate images and scaling parameter of L2ISP.
+If you want to crop the output image,
+you should set crop parameter to the corresponding source pad of the ISP subdevice with media-ctl tool.
+
+This control accepts a `struct viif_l2_roi_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_GAMMA:
+
+V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_GAMMA
+---------------------------------------
+
+This control sets gamma parameter for L2ISP.
+
+This control accepts a `struct viif_l2_gamma_config` instance.
+
+.. _V4L2_CID_VISCONTI_VIIF_CSI2RX_GET_CALIBRATION_STATUS:
+
+V4L2_CID_VISCONTI_VIIF_CSI2RX_GET_CALIBRATION_STATUS
+----------------------------------------------------
+
+This control provides CSI2 receiver calibration status.
+
+This control fills a `struct viif_csi2rx_cal_status` instance with current status.
+
+.. _V4L2_CID_VISCONTI_VIIF_CSI2RX_GET_ERR_STATUS:
+
+V4L2_CID_VISCONTI_VIIF_CSI2RX_GET_ERR_STATUS
+--------------------------------------------
+
+This control provides CSI2 receiver error description.
+
+This control fills a `struct viif_csi2rx_err_status` instance with accumerated error status.
+Note that internal accumerated status is cleared after reading.
+
+.. _V4L2_CID_VISCONTI_VIIF_GET_LAST_CAPTURE_STATUS:
+
+V4L2_CID_VISCONTI_VIIF_GET_LAST_CAPTURE_STATUS
+----------------------------------------------
+
+This control provides status information for the last captured frame.
+
+This control fills a `struct viif_l1_info` instance with current status.
+
+.. _V4L2_CID_VISCONTI_VIIF_GET_REPORTED_ERRORS:
+
+V4L2_CID_VISCONTI_VIIF_GET_REPORTED_ERRORS
+------------------------------------------
+
+This control provides error information since the last read of this control.
+
+This control fills a `struct viif_reported_errors` instance with accumerated error status.
+Note that internal accumerated status is cleared after reading.
+
+Structures
+==========
+
+.. kernel-doc:: include/uapi/linux/visconti_viif.h
+
-- 
2.25.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 5/5] MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface

[Yuji Ishikawa]

Added entries for visconti Video Input Interface driver, including;
* device tree bindings
* source files
* documentation files

Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>
---
Changelog v2:
- no change

Changelog v3:
- added entry for driver API documentation

Changelog v4:
- added entry for header file

Changelog v5:
- no change

Changelog v6:
- update path to VIIF driver source files

Changelog v7:
- no change

 MAINTAINERS | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 35e195946..97a574325 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2957,17 +2957,21 @@ F:	Documentation/devicetree/bindings/arm/toshiba.yaml
 F:	Documentation/devicetree/bindings/clock/toshiba,tmpv770x-pipllct.yaml
 F:	Documentation/devicetree/bindings/clock/toshiba,tmpv770x-pismu.yaml
 F:	Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml
+F:	Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
 F:	Documentation/devicetree/bindings/net/toshiba,visconti-dwmac.yaml
 F:	Documentation/devicetree/bindings/pci/toshiba,visconti-pcie.yaml
 F:	Documentation/devicetree/bindings/pinctrl/toshiba,visconti-pinctrl.yaml
 F:	Documentation/devicetree/bindings/watchdog/toshiba,visconti-wdt.yaml
+F:	Documentation/driver-api/media/drivers/visconti-viif.rst
 F:	arch/arm64/boot/dts/toshiba/
 F:	drivers/clk/visconti/
 F:	drivers/gpio/gpio-visconti.c
+F:	drivers/media/platform/toshiba/visconti/
 F:	drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c
 F:	drivers/pci/controller/dwc/pcie-visconti.c
 F:	drivers/pinctrl/visconti/
 F:	drivers/watchdog/visconti_wdt.c
+F:	include/uapi/linux/visconti_viif.h
 N:	visconti
 
 ARM/UNIPHIER ARCHITECTURE
-- 
2.25.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 1/5] dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface

[Krzysztof Kozlowski]

On 14/07/2023 03:50, Yuji Ishikawa wrote:
> Adds the Device Tree binding documentation that allows to describe
> the Video Input Interface found in Toshiba Visconti SoCs.
> 
> Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>
> Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> ---
> Changelog v2:

Thank you for your patch. There is something to discuss/improve.

> - no change
> 
> Changelog v3:
> - no change
> 
> Changelog v4:
> - fix style problems at the v3 patch
> - remove "index" member
> - update example
> 
> Changelog v5:
> - no change
> 
> Changelog v6:
> - add register definition of BUS-IF and MPU
> 
> Changelog v7:
> - remove trailing "bindings" from commit header message
> - remove trailing "Device Tree Bindings" from title
> - fix text wrapping of description
> - change compatible to visconti5-viif

Then the filename should be updated to match it:
toshiba,visconti5-viif.yaml


> - explicitly define allowed properties for port::endpoint
> 
>  .../bindings/media/toshiba,visconti-viif.yaml | 108 ++++++++++++++++++
>  1 file changed, 108 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
> 

...

> +  port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    unevaluatedProperties: false
> +    description: Input port, single endpoint describing the CSI-2 transmitter.
> +
> +    properties:
> +      endpoint:
> +        $ref: video-interfaces.yaml#
> +        additionalProperties: false

This should be rather:
unevaluatedProperties: false
> +
> +        required: ["bus-type", "clock-noncontinuous", "link-frequencies", "remote-endpoint"]

That's not the syntax we try to keep in the bindings. See
renesas,rzg2l-csi2.yaml.

> +
> +        properties:
> +          data-lanes:
> +            description: VIIF supports 2 or 4 data lanes
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            minItems: 1
> +            maxItems: 4
> +            items:
> +              minimum: 1
> +              maximum: 4
> +
> +          clock-lanes:
> +            description: VIIF supports 1 clock lane
> +            const: 0

Are you sure it must be on position 0?

> +
> +          bus-type: true
> +          clock-noncontinuous: true
> +          link-frequencies: true
> +          remote-endpoint: true

Drop all of these "xxx: true", should not be needed after converting to
unevaluatedProperties: false

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        viif@1c000000 {

isp@
(or video or something else matching this type of the device, but this
should be a generic name)

> +            compatible = "toshiba,visconti5-viif";
> +            reg = <0 0x1c000000 0 0x6000>,
> +                  <0 0x1c008000 0 0x400>,
> +                  <0 0x1c00E000 0 0x1000>,
> +                  <0 0x2417A000 0 0x1000>;

Lowercase hex, please.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/5] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver

[Krzysztof Kozlowski]

On 14/07/2023 03:50, Yuji Ishikawa wrote:
> Add support to Video Input Interface on Toshiba Visconti ARM SoCs.
> The interface device includes CSI2 Receiver,
> frame grabber, video DMAC and image signal processor.
> 
> A driver instance provides three /dev/videoX device files;
> one for RGB image capture, another one for optional RGB capture
> with different parameters and the last one for RAW capture.
> 

...

> +static int visconti_viif_parse_dt(struct viif_device *viif_dev)
> +{
> +	struct device_node *of = viif_dev->dev->of_node;
> +	struct v4l2_fwnode_endpoint fw_ep;
> +	struct viif_subdev *viif_sd;
> +	struct device_node *ep;
> +	unsigned int i;
> +	int num_ep;
> +	int ret;
> +
> +	memset(&fw_ep, 0, sizeof(struct v4l2_fwnode_endpoint));

Why you cannot initialize it in declaration? = { 0 }?

> +
> +	num_ep = of_graph_get_endpoint_count(of);
> +	if (!num_ep)
> +		return -ENODEV;
> +
> +	ret = visconti_viif_init_async_subdevs(viif_dev, num_ep);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < num_ep; i++) {
> +		ep = of_graph_get_endpoint_by_regs(of, 0, i);
> +		if (!ep) {
> +			dev_err(viif_dev->dev, "No subdevice connected on endpoint %u.\n", i);
> +			ret = -ENODEV;
> +			goto error_put_node;
> +		}
> +
> +		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &fw_ep);
> +		if (ret) {
> +			dev_err(viif_dev->dev, "Unable to parse endpoint #%u.\n", i);
> +			goto error_put_node;
> +		}
> +
> +		if (fw_ep.bus_type != V4L2_MBUS_CSI2_DPHY ||
> +		    fw_ep.bus.mipi_csi2.num_data_lanes == 0) {
> +			dev_err(viif_dev->dev, "missing CSI-2 properties in endpoint\n");
> +			ret = -EINVAL;
> +			goto error_put_node;
> +		}
> +
> +		/* Setup the ceu subdevice and the async subdevice. */
> +		viif_sd = &viif_dev->subdevs[i];
> +		INIT_LIST_HEAD(&viif_sd->asd.list);
> +
> +		viif_sd->num_lane = fw_ep.bus.mipi_csi2.num_data_lanes;
> +		viif_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +		viif_sd->asd.match.fwnode =
> +			fwnode_graph_get_remote_port_parent(of_fwnode_handle(ep));
> +
> +		viif_dev->asds[i] = &viif_sd->asd;
> +		of_node_put(ep);
> +	}
> +
> +	return num_ep;
> +
> +error_put_node:
> +	of_node_put(ep);
> +	return ret;
> +}
> +
> +static const struct of_device_id visconti_viif_of_table[] = {
> +	{
> +		.compatible = "toshiba,visconti5-viif",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, visconti_viif_of_table);
> +
> +#define NUM_IRQS   3
> +#define IRQ_ID_STR "viif"
> +
> +static int visconti_viif_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct viif_device *viif_dev;
> +	dma_addr_t tables_dma;
> +	int ret, i, num_sd;
> +
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
> +	if (ret)
> +		return ret;
> +
> +	viif_dev = devm_kzalloc(dev, sizeof(*viif_dev), GFP_KERNEL);
> +	if (!viif_dev)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, viif_dev);
> +	viif_dev->dev = dev;
> +
> +	spin_lock_init(&viif_dev->regbuf_lock);
> +	mutex_init(&viif_dev->pow_lock);
> +	mutex_init(&viif_dev->stream_lock);
> +
> +	viif_dev->capture_reg = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(viif_dev->capture_reg))
> +		return PTR_ERR(viif_dev->capture_reg);
> +
> +	viif_dev->csi2host_reg = devm_platform_ioremap_resource(pdev, 1);
> +	if (IS_ERR(viif_dev->csi2host_reg))
> +		return PTR_ERR(viif_dev->csi2host_reg);
> +
> +	viif_dev->hwaif_reg = devm_platform_ioremap_resource(pdev, 2);
> +	if (IS_ERR(viif_dev->hwaif_reg))
> +		return PTR_ERR(viif_dev->hwaif_reg);
> +
> +	viif_dev->mpu_reg = devm_platform_ioremap_resource(pdev, 3);
> +	if (IS_ERR(viif_dev->mpu_reg))
> +		return PTR_ERR(viif_dev->mpu_reg);
> +
> +	viif_dev->run_flag_main = false;
> +
> +	for (i = 0; i < NUM_IRQS; i++) {
> +		ret = platform_get_irq(pdev, i);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to acquire irq resource\n");
> +			return ret;

return dev_err_probe()

> +		}
> +		viif_dev->irq[i] = ret;
> +		ret = devm_request_irq(dev, viif_dev->irq[i], visconti_viif_irq, 0, IRQ_ID_STR,
> +				       viif_dev);
> +		if (ret) {
> +			dev_err(dev, "irq request failed\n");

return dev_err_probe()

> +			return ret;
> +		}
> +	}
> +
> +	viif_dev->tables =
> +		dma_alloc_wc(dev, sizeof(struct viif_table_area), &tables_dma, GFP_KERNEL);
> +	if (!viif_dev->tables) {
> +		dev_err(dev, "dma_alloc_wc failed\n");

Are you sure DMA memory allocation errors shall be printed?

> +		return -ENOMEM;
> +	}
> +	viif_dev->tables_dma = (struct viif_table_area *)tables_dma;
> +
> +	/* power control */

Drop the comment, it is useless.

> +	pm_runtime_enable(dev);
> +
> +	/* build media_dev */
> +	viif_dev->media_dev.hw_revision = 0;
> +	strscpy(viif_dev->media_dev.model, VIIF_DRIVER_NAME, sizeof(viif_dev->media_dev.model));
> +	viif_dev->media_dev.dev = dev;
> +	/* TODO: platform:visconti-viif-0,1,2,3 for each VIIF driver instance */
> +	snprintf(viif_dev->media_dev.bus_info, sizeof(viif_dev->media_dev.bus_info), "%s-0",
> +		 VIIF_BUS_INFO_BASE);
> +	media_device_init(&viif_dev->media_dev);
> +
> +	/* build v4l2_dev */
> +	viif_dev->v4l2_dev.mdev = &viif_dev->media_dev;
> +	ret = v4l2_device_register(dev, &viif_dev->v4l2_dev);
> +	if (ret)
> +		goto error_dma_free;
> +
> +	ret = media_device_register(&viif_dev->media_dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to register media device: %d\n", ret);
> +		goto error_v4l2_unregister;

dev_err_probe

> +	}
> +
> +	ret = visconti_viif_csi2rx_register(viif_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to register csi2rx sub node: %d\n", ret);

dev_err_probe


> +		goto error_media_unregister;
> +	}
> +
> +	ret = visconti_viif_isp_register(viif_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to register isp sub node: %d\n", ret);

dev_err_probe


> +		goto error_media_unregister;
> +	}
> +	ret = visconti_viif_capture_register(viif_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to register capture node: %d\n", ret);

dev_err_probe

> +		goto error_media_unregister;
> +	}
> +
> +	/* handle subdevices in device tree */
> +	num_sd = visconti_viif_parse_dt(viif_dev);
> +	if (ret < 0) {
> +		ret = num_sd;

ret = dev_err_probe

> +		goto error_media_unregister;
> +	}
> +
> +	viif_dev->notifier.v4l2_dev = &viif_dev->v4l2_dev;
> +	v4l2_async_nf_init(&viif_dev->notifier);
> +	for (i = 0; i < num_sd; i++)
> +		__v4l2_async_nf_add_subdev(&viif_dev->notifier, viif_dev->asds[i]);
> +	viif_dev->notifier.ops = &viif_notify_ops;
> +	ret = v4l2_async_nf_register(&viif_dev->v4l2_dev, &viif_dev->notifier);
> +	if (ret)
> +		goto error_media_unregister;
> +
> +	viif_dev->wq = create_workqueue("visconti-viif");
> +	if (!viif_dev->wq)
> +		return -ENOMEM;

No error cleanup?

> +	INIT_WORK(&viif_dev->work, visconti_viif_wthread_l1info);
> +
> +	return 0;
> +
> +error_media_unregister:
> +	media_device_unregister(&viif_dev->media_dev);
> +error_v4l2_unregister:
> +	v4l2_device_unregister(&viif_dev->v4l2_dev);
> +error_dma_free:
> +	pm_runtime_disable(dev);
> +	dma_free_wc(&pdev->dev, sizeof(struct viif_table_area), viif_dev->tables,
> +		    (dma_addr_t)viif_dev->tables_dma);
> +	return ret;
> +}
> +
> +static int visconti_viif_remove(struct platform_device *pdev)
> +{
> +	struct viif_device *viif_dev = platform_get_drvdata(pdev);
> +
> +	destroy_workqueue(viif_dev->wq);
> +	visconti_viif_isp_unregister(viif_dev);
> +	visconti_viif_capture_unregister(viif_dev);
> +	v4l2_async_nf_unregister(&viif_dev->notifier);

Why these three are not called in probe error paths?

> +	media_device_unregister(&viif_dev->media_dev);
> +	v4l2_device_unregister(&viif_dev->v4l2_dev);
> +	pm_runtime_disable(&pdev->dev);
> +	dma_free_wc(&pdev->dev, sizeof(struct viif_table_area), viif_dev->tables,
> +		    (dma_addr_t)viif_dev->tables_dma);
> +
> +	return 0;
> +}
> +
> +static int visconti_viif_runtime_suspend(struct device *dev)
> +{
> +	/* This callback is kicked when the last device-file is closed */
> +	struct viif_device *viif_dev = dev_get_drvdata(dev);
> +
> +	mutex_lock(&viif_dev->pow_lock);

Why?

> +	visconti_viif_hw_off(viif_dev);
> +	mutex_unlock(&viif_dev->pow_lock);
> +
> +	return 0;
> +}
> +
> +static int visconti_viif_runtime_resume(struct device *dev)
> +{
> +	/* This callback is kicked when the first device-file is opened */
> +	struct viif_device *viif_dev = dev_get_drvdata(dev);
> +
> +	viif_dev->rawpack_mode = (u32)VIIF_RAWPACK_DISABLE;
> +
> +	mutex_lock(&viif_dev->pow_lock);

Why?

> +
> +	/* Initialize HWD driver */
> +	visconti_viif_hw_on(viif_dev);
> +
> +	/* VSYNC mask setting of MAIN unit */
> +	viif_main_vsync_set_irq_mask(viif_dev, MASK_INT_M_SYNC_MASK_SET);
> +
> +	/* STATUS error mask setting of MAIN unit */
> +	viif_main_status_err_set_irq_mask(viif_dev, MASK_INT_M_DELAY_INT_ERROR);
> +
> +	/* VSYNC mask settings of SUB unit */
> +	viif_sub_vsync_set_irq_mask(viif_dev, MASK_INT_S_SYNC_MASK_SET);
> +
> +	/* STATUS error mask setting(unmask) of SUB unit */
> +	viif_sub_status_err_set_irq_mask(viif_dev,
> +					 MASK_INT_S_RESERVED_SET | MASK_INT_S_DELAY_INT_ERROR);
> +
> +	mutex_unlock(&viif_dev->pow_lock);
> +
> +	return 0;
> +}
> +

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 3/5] media: add V4L2 vendor specific control handlers

[Hans Verkuil]

On 14/07/2023 03:50, Yuji Ishikawa wrote:
> Add support to Image Signal Processors of Visconti's Video Input Interface.
> This patch adds vendor specific compound controls
> to configure the image signal processor.
> 
> Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>
> ---
> Changelog v2:
> - Resend v1 because a patch exceeds size limit.
> 
> Changelog v3:
> - 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 the v3 patch exceeds size limit
> - Stop using ID number to identify driver instance:
>   - Use dynamically allocated structure to hold HW specific context,
>     instead of static one.
>   - Call HW layer functions with the context structure instead of ID number
> 
> Changelog v5:
> - no change
> 
> Changelog v6:
> - remove unused macros
> - removed hwd_ and HWD_ prefix
> - update source code documentation
> - Suggestion from Hans Verkuil
>   - pointer to userland memory is removed from uAPI arguments
>     - style of structure is now "nested" instead of "chained by pointer";
>   - use div64_u64 for 64bit division
>   - vendor specific controls support TRY_EXT_CTRLS
>   - add READ_ONLY flag to GET_CALIBRATION_STATUS control and similar ones
>   - human friendry control names for vendor specific controls
>   - add initial value to each vendor specific control
>   - GET_LAST_CAPTURE_STATUS control is updated asyncnously from workqueue
>   - remove EXECUTE_ON_WRITE flag of vendor specific control
>   - uAPI: return value of GET_CALIBRATION_STATUS follows common rules of error codes
>   - applied v4l2-compliance
> - Suggestion from Sakari Ailus
>   - use div64_u64 for 64bit division
>   - update copyright's year
>   - remove redandunt cast
>   - use bool instead of HWD_VIIF_ENABLE/DISABLE
>   - simplify comparison to 0
>   - simplify statements with trigram operator
>   - remove redundant local variables
>   - use general integer types instead of u32/s32
> - Suggestion from Laurent Pinchart
>   - moved VIIF driver to driver/platform/toshiba/visconti
>   - change register access: struct-style to macro-style
>   - remove unused type definitions
>   - define enums instead of successive macro constants
>   - remove redundant parenthesis of macro constant
>   - embed struct hwd_res into struct viif_device
>   - use xxx_dma instead of xxx_paddr for variable names of IOVA
>   - literal value: just 0 instead of 0x0
>   - use literal 1 or 0 instead of HWD_VIIF_ENABLE, DISABLE for register access
>   - use true or false instead of HWD_VIIF_ENABLE, DISABLE for function calls
>   - uAPI: return value of GET_CALIBRATION_STATUS follows common rules of error codes
> 
> Changelog v7:
> - remove unused variables
> - split long statements which have multiple logical-OR and trigram operators
> 
>  .../media/platform/toshiba/visconti/Makefile  |    2 +-
>  .../media/platform/toshiba/visconti/viif.c    |   10 +-
>  .../platform/toshiba/visconti/viif_controls.c | 3407 +++++++++++++++++
>  .../platform/toshiba/visconti/viif_controls.h |   18 +
>  .../platform/toshiba/visconti/viif_isp.c      |   15 +-
>  5 files changed, 3435 insertions(+), 17 deletions(-)
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_controls.c
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_controls.h
> 
> diff --git a/drivers/media/platform/toshiba/visconti/Makefile b/drivers/media/platform/toshiba/visconti/Makefile
> index 5f2f9199c..a28e6fa84 100644
> --- a/drivers/media/platform/toshiba/visconti/Makefile
> +++ b/drivers/media/platform/toshiba/visconti/Makefile
> @@ -3,6 +3,6 @@
>  # Makefile for the Visconti video input device driver
>  #
>  
> -visconti-viif-objs = viif.o viif_capture.o viif_isp.o viif_csi2rx.o viif_common.o
> +visconti-viif-objs = viif.o viif_capture.o viif_controls.o viif_isp.o viif_csi2rx.o viif_common.o
>  
>  obj-$(CONFIG_VIDEO_VISCONTI_VIIF) += visconti-viif.o
> diff --git a/drivers/media/platform/toshiba/visconti/viif.c b/drivers/media/platform/toshiba/visconti/viif.c
> index c07dc2626..1b3d61abf 100644
> --- a/drivers/media/platform/toshiba/visconti/viif.c
> +++ b/drivers/media/platform/toshiba/visconti/viif.c
> @@ -18,6 +18,7 @@
>  
>  #include "viif.h"
>  #include "viif_capture.h"
> +#include "viif_controls.h"
>  #include "viif_csi2rx.h"
>  #include "viif_common.h"
>  #include "viif_isp.h"
> @@ -178,12 +179,9 @@ static struct viif_subdev *to_viif_subdev(struct v4l2_async_subdev *asd)
>  /* before a userland capture application is trigered by vb2_buffer_done() */
>  static void visconti_viif_wthread_l1info(struct work_struct *work)
>  {
> -	/* called function is implemented by the next patch */
> -/*
> - *	struct viif_device *viif_dev = container_of(work, struct viif_device, work);
> - *
> - *	visconti_viif_save_l1_info(viif_dev);
> - */
> +	struct viif_device *viif_dev = container_of(work, struct viif_device, work);
> +
> +	visconti_viif_save_l1_info(viif_dev);
>  }
>  
>  static void viif_vsync_irq_handler_w_isp(struct viif_device *viif_dev)
> diff --git a/drivers/media/platform/toshiba/visconti/viif_controls.c b/drivers/media/platform/toshiba/visconti/viif_controls.c
> new file mode 100644
> index 000000000..3cf10e15c
> --- /dev/null
> +++ b/drivers/media/platform/toshiba/visconti/viif_controls.c
> @@ -0,0 +1,3407 @@

<snip>

> +static int visconti_viif_isp_try_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct viif_device *viif_dev = ctrl->priv;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_VISCONTI_VIIF_MAIN_SET_RAWPACK_MODE:
> +		return viif_main_set_rawpack_mode_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_INPUT_MODE:
> +		return viif_l1_set_input_mode_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_RGB_TO_Y_COEF:
> +		return viif_l1_set_rgb_to_y_coef_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AG_MODE:
> +		return viif_l1_set_ag_mode_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AG:
> +		return 0; //no need to check
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRE:
> +		return viif_l1_set_hdre_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_IMG_EXTRACTION:
> +		return viif_l1_set_img_extraction_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_DPC:
> +		return viif_l1_set_dpc_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_PRESET_WHITE_BALANCE:
> +		return viif_l1_set_preset_white_balance_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_RAW_COLOR_NOISE_REDUCTION:
> +		return viif_l1_set_raw_color_noise_reduction_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRS:
> +		return viif_l1_set_hdrs_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_BLACK_LEVEL_CORRECTION:
> +		return viif_l1_set_black_level_correction_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_LSC:
> +		return viif_l1_set_lsc_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_MAIN_PROCESS:
> +		return viif_l1_set_main_process_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AWB:
> +		return viif_l1_set_awb_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_LOCK_AWB_GAIN:
> +		return 0;
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRC:
> +		return viif_l1_set_hdrc_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRC_LTM:
> +		return viif_l1_set_hdrc_ltm_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_GAMMA:
> +		return viif_l1_set_gamma_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_IMG_QUALITY_ADJUSTMENT:
> +		return viif_l1_set_img_quality_adjustment_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AVG_LUM_GENERATION:
> +		return viif_l1_set_avg_lum_generation_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_UNDIST:
> +		return viif_l2_set_undist_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_ROI:
> +		return viif_l2_set_roi_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_GAMMA:
> +		return viif_l2_set_gamma_try(viif_dev, ctrl->p_new.p);
> +	case V4L2_CID_VISCONTI_VIIF_GET_LAST_CAPTURE_STATUS:
> +		return 0;
> +	default:
> +		pr_info("unknown_ctrl:t: id=%08X val=%d", ctrl->id, ctrl->val);
> +		break;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int visconti_viif_isp_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct viif_device *viif_dev = ctrl->priv;
> +	int ret;
> +
> +	pr_info("isp_set_ctrl: %s", ctrl->name);

Don't use pr_info for what is just a debug message! Either drop it, or
replace it with dev_dbg.

> +	if (pm_runtime_status_suspended(viif_dev->dev)) {
> +		pr_info("warning: visconti viif HW is not powered");

And here pr_info is used for a warning, so shouldn't this be dev_warn?

I see pr_info being used in a lot of places where it doesn't belong and
would just spam the kernel log.

Something to go through for v8.

Regards,

	Hans

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 3/5] media: add V4L2 vendor specific control handlers

[Laurent Pinchart]

On Mon, Aug 21, 2023 at 02:28:11PM +0200, Hans Verkuil wrote:
> On 14/07/2023 03:50, Yuji Ishikawa wrote:
> > Add support to Image Signal Processors of Visconti's Video Input Interface.
> > This patch adds vendor specific compound controls
> > to configure the image signal processor.
> > 
> > Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>
> > ---
> > Changelog v2:
> > - Resend v1 because a patch exceeds size limit.
> > 
> > Changelog v3:
> > - 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 the v3 patch exceeds size limit
> > - Stop using ID number to identify driver instance:
> >   - Use dynamically allocated structure to hold HW specific context,
> >     instead of static one.
> >   - Call HW layer functions with the context structure instead of ID number
> > 
> > Changelog v5:
> > - no change
> > 
> > Changelog v6:
> > - remove unused macros
> > - removed hwd_ and HWD_ prefix
> > - update source code documentation
> > - Suggestion from Hans Verkuil
> >   - pointer to userland memory is removed from uAPI arguments
> >     - style of structure is now "nested" instead of "chained by pointer";
> >   - use div64_u64 for 64bit division
> >   - vendor specific controls support TRY_EXT_CTRLS
> >   - add READ_ONLY flag to GET_CALIBRATION_STATUS control and similar ones
> >   - human friendry control names for vendor specific controls
> >   - add initial value to each vendor specific control
> >   - GET_LAST_CAPTURE_STATUS control is updated asyncnously from workqueue
> >   - remove EXECUTE_ON_WRITE flag of vendor specific control
> >   - uAPI: return value of GET_CALIBRATION_STATUS follows common rules of error codes
> >   - applied v4l2-compliance
> > - Suggestion from Sakari Ailus
> >   - use div64_u64 for 64bit division
> >   - update copyright's year
> >   - remove redandunt cast
> >   - use bool instead of HWD_VIIF_ENABLE/DISABLE
> >   - simplify comparison to 0
> >   - simplify statements with trigram operator
> >   - remove redundant local variables
> >   - use general integer types instead of u32/s32
> > - Suggestion from Laurent Pinchart
> >   - moved VIIF driver to driver/platform/toshiba/visconti
> >   - change register access: struct-style to macro-style
> >   - remove unused type definitions
> >   - define enums instead of successive macro constants
> >   - remove redundant parenthesis of macro constant
> >   - embed struct hwd_res into struct viif_device
> >   - use xxx_dma instead of xxx_paddr for variable names of IOVA
> >   - literal value: just 0 instead of 0x0
> >   - use literal 1 or 0 instead of HWD_VIIF_ENABLE, DISABLE for register access
> >   - use true or false instead of HWD_VIIF_ENABLE, DISABLE for function calls
> >   - uAPI: return value of GET_CALIBRATION_STATUS follows common rules of error codes
> > 
> > Changelog v7:
> > - remove unused variables
> > - split long statements which have multiple logical-OR and trigram operators
> > 
> >  .../media/platform/toshiba/visconti/Makefile  |    2 +-
> >  .../media/platform/toshiba/visconti/viif.c    |   10 +-
> >  .../platform/toshiba/visconti/viif_controls.c | 3407 +++++++++++++++++
> >  .../platform/toshiba/visconti/viif_controls.h |   18 +
> >  .../platform/toshiba/visconti/viif_isp.c      |   15 +-
> >  5 files changed, 3435 insertions(+), 17 deletions(-)
> >  create mode 100644 drivers/media/platform/toshiba/visconti/viif_controls.c
> >  create mode 100644 drivers/media/platform/toshiba/visconti/viif_controls.h
> > 
> > diff --git a/drivers/media/platform/toshiba/visconti/Makefile b/drivers/media/platform/toshiba/visconti/Makefile
> > index 5f2f9199c..a28e6fa84 100644
> > --- a/drivers/media/platform/toshiba/visconti/Makefile
> > +++ b/drivers/media/platform/toshiba/visconti/Makefile
> > @@ -3,6 +3,6 @@
> >  # Makefile for the Visconti video input device driver
> >  #
> >  
> > -visconti-viif-objs = viif.o viif_capture.o viif_isp.o viif_csi2rx.o viif_common.o
> > +visconti-viif-objs = viif.o viif_capture.o viif_controls.o viif_isp.o viif_csi2rx.o viif_common.o
> >  
> >  obj-$(CONFIG_VIDEO_VISCONTI_VIIF) += visconti-viif.o
> > diff --git a/drivers/media/platform/toshiba/visconti/viif.c b/drivers/media/platform/toshiba/visconti/viif.c
> > index c07dc2626..1b3d61abf 100644
> > --- a/drivers/media/platform/toshiba/visconti/viif.c
> > +++ b/drivers/media/platform/toshiba/visconti/viif.c
> > @@ -18,6 +18,7 @@
> >  
> >  #include "viif.h"
> >  #include "viif_capture.h"
> > +#include "viif_controls.h"
> >  #include "viif_csi2rx.h"
> >  #include "viif_common.h"
> >  #include "viif_isp.h"
> > @@ -178,12 +179,9 @@ static struct viif_subdev *to_viif_subdev(struct v4l2_async_subdev *asd)
> >  /* before a userland capture application is trigered by vb2_buffer_done() */
> >  static void visconti_viif_wthread_l1info(struct work_struct *work)
> >  {
> > -	/* called function is implemented by the next patch */
> > -/*
> > - *	struct viif_device *viif_dev = container_of(work, struct viif_device, work);
> > - *
> > - *	visconti_viif_save_l1_info(viif_dev);
> > - */
> > +	struct viif_device *viif_dev = container_of(work, struct viif_device, work);
> > +
> > +	visconti_viif_save_l1_info(viif_dev);
> >  }
> >  
> >  static void viif_vsync_irq_handler_w_isp(struct viif_device *viif_dev)
> > diff --git a/drivers/media/platform/toshiba/visconti/viif_controls.c b/drivers/media/platform/toshiba/visconti/viif_controls.c
> > new file mode 100644
> > index 000000000..3cf10e15c
> > --- /dev/null
> > +++ b/drivers/media/platform/toshiba/visconti/viif_controls.c
> > @@ -0,0 +1,3407 @@
> 
> <snip>
> 
> > +static int visconti_viif_isp_try_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct viif_device *viif_dev = ctrl->priv;
> > +
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_VISCONTI_VIIF_MAIN_SET_RAWPACK_MODE:
> > +		return viif_main_set_rawpack_mode_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_INPUT_MODE:
> > +		return viif_l1_set_input_mode_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_RGB_TO_Y_COEF:
> > +		return viif_l1_set_rgb_to_y_coef_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AG_MODE:
> > +		return viif_l1_set_ag_mode_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AG:
> > +		return 0; //no need to check
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRE:
> > +		return viif_l1_set_hdre_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_IMG_EXTRACTION:
> > +		return viif_l1_set_img_extraction_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_DPC:
> > +		return viif_l1_set_dpc_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_PRESET_WHITE_BALANCE:
> > +		return viif_l1_set_preset_white_balance_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_RAW_COLOR_NOISE_REDUCTION:
> > +		return viif_l1_set_raw_color_noise_reduction_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRS:
> > +		return viif_l1_set_hdrs_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_BLACK_LEVEL_CORRECTION:
> > +		return viif_l1_set_black_level_correction_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_LSC:
> > +		return viif_l1_set_lsc_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_MAIN_PROCESS:
> > +		return viif_l1_set_main_process_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AWB:
> > +		return viif_l1_set_awb_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_LOCK_AWB_GAIN:
> > +		return 0;
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRC:
> > +		return viif_l1_set_hdrc_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRC_LTM:
> > +		return viif_l1_set_hdrc_ltm_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_GAMMA:
> > +		return viif_l1_set_gamma_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_IMG_QUALITY_ADJUSTMENT:
> > +		return viif_l1_set_img_quality_adjustment_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AVG_LUM_GENERATION:
> > +		return viif_l1_set_avg_lum_generation_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_UNDIST:
> > +		return viif_l2_set_undist_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_ROI:
> > +		return viif_l2_set_roi_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_GAMMA:
> > +		return viif_l2_set_gamma_try(viif_dev, ctrl->p_new.p);
> > +	case V4L2_CID_VISCONTI_VIIF_GET_LAST_CAPTURE_STATUS:
> > +		return 0;
> > +	default:
> > +		pr_info("unknown_ctrl:t: id=%08X val=%d", ctrl->id, ctrl->val);
> > +		break;
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> > +static int visconti_viif_isp_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct viif_device *viif_dev = ctrl->priv;
> > +	int ret;
> > +
> > +	pr_info("isp_set_ctrl: %s", ctrl->name);
> 
> Don't use pr_info for what is just a debug message! Either drop it, or
> replace it with dev_dbg.

In drivers, almost all occurences of pr_*() should be replaced by
dev_*(). It's very rare that pr_*() would be the right API.

In this specific case, I'd just drop it.

> > +	if (pm_runtime_status_suspended(viif_dev->dev)) {
> > +		pr_info("warning: visconti viif HW is not powered");
> 
> And here pr_info is used for a warning, so shouldn't this be dev_warn?

I don't think there's a need to warn about this, it's a normal
situation.

The right runtime PM API here is pm_runtime_get_if_in_use() by the way,
not pm_runtime_status_suspended(). Don't forget to call pm_runtime_put()
at the end of the function.

> I see pr_info being used in a lot of places where it doesn't belong and
> would just spam the kernel log.
>
> Something to go through for v8.

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 0/5] Add Toshiba Visconti Video Input Interface driver

[Hans Verkuil]

Hi Yuji,

On 14/07/2023 03:50, 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, documentation and MAINTAINER files.
> 
> A visconti VIIF driver instance exposes
> 1 media control device file and 3 video device files
> for a VIIF hardware.
> 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.

Trying to compile this series on top of our latest staging tree fails
due to v4l2-async changes that have been merged. So for v8 please
rebase to the staging tree.

I also got a few kerneldoc warnings:

drivers/media/platform/toshiba/visconti/viif.h:217: warning: Function parameter or member 'ops_lock' not described in 'isp_subdev'
drivers/media/platform/toshiba/visconti/viif.h:233: warning: Function parameter or member 'ops_lock' not described in 'csi2rx_subdev'
drivers/media/platform/toshiba/visconti/viif.h:254: warning: Function parameter or member 'post_enable_flag' not described in 'viif_l2_roi_path_info'

Regards,

	Hans

> 
> Best regards,
> Yuji
> 
> 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
> 
> Yuji Ishikawa (5):
>   dt-bindings: media: platform: visconti: Add Toshiba Visconti Video
>     Input Interface
>   media: platform: visconti: Add Toshiba Visconti Video Input Interface
>     driver
>   media: add V4L2 vendor specific control handlers
>   documentation: media: add documentation for Toshiba Visconti Video
>     Input Interface driver
>   MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface
> 
>  .../bindings/media/toshiba,visconti-viif.yaml |  108 +
>  .../driver-api/media/drivers/index.rst        |    1 +
>  .../media/drivers/visconti-viif.rst           |  462 +++
>  MAINTAINERS                                   |    4 +
>  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   |   18 +
>  .../media/platform/toshiba/visconti/Makefile  |    8 +
>  .../media/platform/toshiba/visconti/viif.c    |  681 ++++
>  .../media/platform/toshiba/visconti/viif.h    |  375 ++
>  .../platform/toshiba/visconti/viif_capture.c  | 1485 +++++++
>  .../platform/toshiba/visconti/viif_capture.h  |   22 +
>  .../platform/toshiba/visconti/viif_common.c   |  199 +
>  .../platform/toshiba/visconti/viif_common.h   |   38 +
>  .../platform/toshiba/visconti/viif_controls.c | 3407 +++++++++++++++++
>  .../platform/toshiba/visconti/viif_controls.h |   18 +
>  .../platform/toshiba/visconti/viif_csi2rx.c   |  684 ++++
>  .../platform/toshiba/visconti/viif_csi2rx.h   |   24 +
>  .../toshiba/visconti/viif_csi2rx_regs.h       |  102 +
>  .../platform/toshiba/visconti/viif_isp.c      | 1258 ++++++
>  .../platform/toshiba/visconti/viif_isp.h      |   24 +
>  .../platform/toshiba/visconti/viif_regs.h     |  716 ++++
>  include/uapi/linux/v4l2-controls.h            |    6 +
>  include/uapi/linux/visconti_viif.h            | 1800 +++++++++
>  26 files changed, 11450 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
>  create mode 100644 Documentation/driver-api/media/drivers/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_controls.c
>  create mode 100644 drivers/media/platform/toshiba/visconti/viif_controls.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_regs.h
>  create mode 100644 include/uapi/linux/visconti_viif.h
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/5] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver

[Laurent Pinchart]

(CC'ing Christoph, Marek and Robin)

On Tue, Jul 25, 2023 at 06:10:03AM +0000, yuji2.ishikawa@toshiba.co.jp wrote:
> On Friday, July 14, 2023 5:00 PM, Krzysztof Kozlowski wrote:
> > On 14/07/2023 03:50, Yuji Ishikawa wrote:
> > > Add support to Video Input Interface on Toshiba Visconti ARM SoCs.
> > > The interface device includes CSI2 Receiver, frame grabber, video DMAC
> > > and image signal processor.
> > >
> > > A driver instance provides three /dev/videoX device files; one for RGB
> > > image capture, another one for optional RGB capture with different
> > > parameters and the last one for RAW capture.
> > 
> > ...

[snip]

> > > +static int visconti_viif_probe(struct platform_device *pdev) {
> > > +	struct device *dev = &pdev->dev;
> > > +	struct viif_device *viif_dev;
> > > +	dma_addr_t tables_dma;
> > > +	int ret, i, num_sd;
> > > +
> > > +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	viif_dev = devm_kzalloc(dev, sizeof(*viif_dev), GFP_KERNEL);
> > > +	if (!viif_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	platform_set_drvdata(pdev, viif_dev);
> > > +	viif_dev->dev = dev;
> > > +
> > > +	spin_lock_init(&viif_dev->regbuf_lock);
> > > +	mutex_init(&viif_dev->pow_lock);
> > > +	mutex_init(&viif_dev->stream_lock);
> > > +
> > > +	viif_dev->capture_reg = devm_platform_ioremap_resource(pdev, 0);
> > > +	if (IS_ERR(viif_dev->capture_reg))
> > > +		return PTR_ERR(viif_dev->capture_reg);
> > > +
> > > +	viif_dev->csi2host_reg = devm_platform_ioremap_resource(pdev, 1);
> > > +	if (IS_ERR(viif_dev->csi2host_reg))
> > > +		return PTR_ERR(viif_dev->csi2host_reg);
> > > +
> > > +	viif_dev->hwaif_reg = devm_platform_ioremap_resource(pdev, 2);
> > > +	if (IS_ERR(viif_dev->hwaif_reg))
> > > +		return PTR_ERR(viif_dev->hwaif_reg);
> > > +
> > > +	viif_dev->mpu_reg = devm_platform_ioremap_resource(pdev, 3);
> > > +	if (IS_ERR(viif_dev->mpu_reg))
> > > +		return PTR_ERR(viif_dev->mpu_reg);
> > > +
> > > +	viif_dev->run_flag_main = false;
> > > +
> > > +	for (i = 0; i < NUM_IRQS; i++) {
> > > +		ret = platform_get_irq(pdev, i);
> > > +		if (ret < 0) {
> > > +			dev_err(dev, "failed to acquire irq resource\n");
> > > +			return ret;
> > 
> > return dev_err_probe()
> 
> I'll use dev_err_probe().
> Same for other suggestions.
> 
> > > +		}
> > > +		viif_dev->irq[i] = ret;
> > > +		ret = devm_request_irq(dev, viif_dev->irq[i], visconti_viif_irq, 0, IRQ_ID_STR,
> > > +				       viif_dev);
> > > +		if (ret) {
> > > +			dev_err(dev, "irq request failed\n");
> > 
> > return dev_err_probe()
> 
> I'll use dev_err_probe().
> 
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	viif_dev->tables =
> > > +		dma_alloc_wc(dev, sizeof(struct viif_table_area), &tables_dma, GFP_KERNEL);
> > > +	if (!viif_dev->tables) {
> > > +		dev_err(dev, "dma_alloc_wc failed\n");
> > 
> > Are you sure DMA memory allocation errors shall be printed?
> 
> Printing this error is useless for users in general?
> If so, I'll drop this debug output.

Failures to allocate memory in the kernel generally result in warning
messages being printed by the allocation function, so there's no need to
do so manually in drivers. This being said, I check dma_alloc_wc()
(which is a wrapper around dma_alloc_attrs()), and unless I'm missing
something, it can return NULL without printing any error. I don't know
if this is an oversight in some code paths taken by dma_alloc_attrs() or
if it's on purpose. Maybe Christoph, Marek or Roben will known.

> > > +		return -ENOMEM;
> > > +	}
> > > +	viif_dev->tables_dma = (struct viif_table_area *)tables_dma;
> > > +
> > > +	/* power control */
> > 
> > Drop the comment, it is useless.
> 
> I'll drop the comment
> 
> > > +	pm_runtime_enable(dev);
> > > +
> > > +	/* build media_dev */
> > > +	viif_dev->media_dev.hw_revision = 0;
> > > +	strscpy(viif_dev->media_dev.model, VIIF_DRIVER_NAME, sizeof(viif_dev->media_dev.model));
> > > +	viif_dev->media_dev.dev = dev;
> > > +	/* TODO: platform:visconti-viif-0,1,2,3 for each VIIF driver instance */
> > > +	snprintf(viif_dev->media_dev.bus_info, sizeof(viif_dev->media_dev.bus_info), "%s-0",
> > > +		 VIIF_BUS_INFO_BASE);
> > > +	media_device_init(&viif_dev->media_dev);
> > > +
> > > +	/* build v4l2_dev */
> > > +	viif_dev->v4l2_dev.mdev = &viif_dev->media_dev;
> > > +	ret = v4l2_device_register(dev, &viif_dev->v4l2_dev);
> > > +	if (ret)
> > > +		goto error_dma_free;
> > > +
> > > +	ret = media_device_register(&viif_dev->media_dev);
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to register media device: %d\n", ret);
> > > +		goto error_v4l2_unregister;
> > 
> > dev_err_probe
> 
> I'll use dev_err_probe().
> 
> > > +	}
> > > +
> > > +	ret = visconti_viif_csi2rx_register(viif_dev);
> > > +	if (ret) {
> > > +		dev_err(dev, "failed to register csi2rx sub node: %d\n", ret);
> > 
> > dev_err_probe
> 
> I'll use dev_err_probe().
> 
> > > +		goto error_media_unregister;
> > > +	}
> > > +
> > > +	ret = visconti_viif_isp_register(viif_dev);
> > > +	if (ret) {
> > > +		dev_err(dev, "failed to register isp sub node: %d\n", ret);
> > 
> > dev_err_probe
> 
> I'll use dev_err_probe().
> 
> > > +		goto error_media_unregister;
> > > +	}
> > > +	ret = visconti_viif_capture_register(viif_dev);
> > > +	if (ret) {
> > > +		dev_err(dev, "failed to register capture node: %d\n", ret);
> > 
> > dev_err_probe
> 
> I'll use dev_err_probe().
> 
> > > +		goto error_media_unregister;
> > > +	}
> > > +
> > > +	/* handle subdevices in device tree */
> > > +	num_sd = visconti_viif_parse_dt(viif_dev);
> > > +	if (ret < 0) {
> > > +		ret = num_sd;
> > 
> > ret = dev_err_probe
> 
> I'll use dev_err_probe().
> 
> > > +		goto error_media_unregister;
> > > +	}
> > > +
> > > +	viif_dev->notifier.v4l2_dev = &viif_dev->v4l2_dev;
> > > +	v4l2_async_nf_init(&viif_dev->notifier);
> > > +	for (i = 0; i < num_sd; i++)
> > > +		__v4l2_async_nf_add_subdev(&viif_dev->notifier, viif_dev->asds[i]);
> > > +	viif_dev->notifier.ops = &viif_notify_ops;
> > > +	ret = v4l2_async_nf_register(&viif_dev->v4l2_dev, &viif_dev->notifier);
> > > +	if (ret)
> > > +		goto error_media_unregister;
> > > +
> > > +	viif_dev->wq = create_workqueue("visconti-viif");
> > > +	if (!viif_dev->wq)
> > > +		return -ENOMEM;
> > 
> > No error cleanup?
> 
> There should be. I'll add cleanup operations.
> 
> > > +	INIT_WORK(&viif_dev->work, visconti_viif_wthread_l1info);
> > > +
> > > +	return 0;
> > > +
> > > +error_media_unregister:
> > > +	media_device_unregister(&viif_dev->media_dev);
> > > +error_v4l2_unregister:
> > > +	v4l2_device_unregister(&viif_dev->v4l2_dev);
> > > +error_dma_free:
> > > +	pm_runtime_disable(dev);
> > > +	dma_free_wc(&pdev->dev, sizeof(struct viif_table_area), viif_dev->tables,
> > > +		    (dma_addr_t)viif_dev->tables_dma);
> > > +	return ret;
> > > +}

[snip]

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 2/5] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver

[Robin Murphy]

On 2023-08-21 14:19, Laurent Pinchart wrote:
[...]
>>>> +	viif_dev->tables =
>>>> +		dma_alloc_wc(dev, sizeof(struct viif_table_area), &tables_dma, GFP_KERNEL);
>>>> +	if (!viif_dev->tables) {
>>>> +		dev_err(dev, "dma_alloc_wc failed\n");
>>>
>>> Are you sure DMA memory allocation errors shall be printed?
>>
>> Printing this error is useless for users in general?
>> If so, I'll drop this debug output.
> 
> Failures to allocate memory in the kernel generally result in warning
> messages being printed by the allocation function, so there's no need to
> do so manually in drivers. This being said, I check dma_alloc_wc()
> (which is a wrapper around dma_alloc_attrs()), and unless I'm missing
> something, it can return NULL without printing any error. I don't know
> if this is an oversight in some code paths taken by dma_alloc_attrs() or
> if it's on purpose. Maybe Christoph, Marek or Roben will known.

Yeah, there might be a few edge cases, but in most cases 
dma_alloc_attrs() will end up falling back to the page allocator as a 
last resort if all the more preferred allocation options fail, and thus 
complete failure should eventually cause that to scream unless 
DMA_ATTR_NO_WARN was specified.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[yuji2.ishikawa]

Hello Hans,

> -----Original Message-----
> From: Hans Verkuil <hverkuil@xs4all.nl>
> Sent: Monday, August 21, 2023 9:59 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@toshiba.co.jp>; Sakari Ailus <sakari.ailus@iki.fi>; Laurent
> Pinchart <laurent.pinchart@ideasonboard.com>; Mauro Carvalho Chehab
> <mchehab@kernel.org>; Rob Herring <robh+dt@kernel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○
> ＯＳＴ) <nobuhiro1.iwamatsu@toshiba.co.jp>; Mark Brown
> <broonie@kernel.org>
> Cc: linux-media@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v7 0/5] Add Toshiba Visconti Video Input Interface driver
> 
> Hi Yuji,
> 
> On 14/07/2023 03:50, 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, documentation
> > and MAINTAINER files.
> >
> > A visconti VIIF driver instance exposes
> > 1 media control device file and 3 video device files for a VIIF
> > hardware.
> > 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.
> 
> Trying to compile this series on top of our latest staging tree fails due to
> v4l2-async changes that have been merged. So for v8 please rebase to the
> staging tree.

All right. The v8 patchset will be rebased to media_stage.git .

> I also got a few kerneldoc warnings:
> 
> drivers/media/platform/toshiba/visconti/viif.h:217: warning: Function
> parameter or member 'ops_lock' not described in 'isp_subdev'
> drivers/media/platform/toshiba/visconti/viif.h:233: warning: Function
> parameter or member 'ops_lock' not described in 'csi2rx_subdev'
> drivers/media/platform/toshiba/visconti/viif.h:254: warning: Function
> parameter or member 'post_enable_flag' not described in 'viif_l2_roi_path_info'

I'll check for kerneldoc warnings and fix them.

Regards,
Yuji

> Regards,
> 
> 	Hans
> 
> >
> > Best regards,
> > Yuji
> >
> > 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
> >
> > Yuji Ishikawa (5):
> >   dt-bindings: media: platform: visconti: Add Toshiba Visconti Video
> >     Input Interface
> >   media: platform: visconti: Add Toshiba Visconti Video Input Interface
> >     driver
> >   media: add V4L2 vendor specific control handlers
> >   documentation: media: add documentation for Toshiba Visconti Video
> >     Input Interface driver
> >   MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface
> >
> >  .../bindings/media/toshiba,visconti-viif.yaml |  108 +
> >  .../driver-api/media/drivers/index.rst        |    1 +
> >  .../media/drivers/visconti-viif.rst           |  462 +++
> >  MAINTAINERS                                   |    4 +
> >  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   |   18 +
> >  .../media/platform/toshiba/visconti/Makefile  |    8 +
> >  .../media/platform/toshiba/visconti/viif.c    |  681 ++++
> >  .../media/platform/toshiba/visconti/viif.h    |  375 ++
> >  .../platform/toshiba/visconti/viif_capture.c  | 1485 +++++++
> >  .../platform/toshiba/visconti/viif_capture.h  |   22 +
> >  .../platform/toshiba/visconti/viif_common.c   |  199 +
> >  .../platform/toshiba/visconti/viif_common.h   |   38 +
> >  .../platform/toshiba/visconti/viif_controls.c | 3407
> +++++++++++++++++
> >  .../platform/toshiba/visconti/viif_controls.h |   18 +
> >  .../platform/toshiba/visconti/viif_csi2rx.c   |  684 ++++
> >  .../platform/toshiba/visconti/viif_csi2rx.h   |   24 +
> >  .../toshiba/visconti/viif_csi2rx_regs.h       |  102 +
> >  .../platform/toshiba/visconti/viif_isp.c      | 1258 ++++++
> >  .../platform/toshiba/visconti/viif_isp.h      |   24 +
> >  .../platform/toshiba/visconti/viif_regs.h     |  716 ++++
> >  include/uapi/linux/v4l2-controls.h            |    6 +
> >  include/uapi/linux/visconti_viif.h            | 1800 +++++++++
> >  26 files changed, 11450 insertions(+)  create mode 100644
> > Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
> >  create mode 100644
> > Documentation/driver-api/media/drivers/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_controls.c
> >  create mode 100644
> > drivers/media/platform/toshiba/visconti/viif_controls.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_regs.h
> >  create mode 100644 include/uapi/linux/visconti_viif.h
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v7 3/5] media: add V4L2 vendor specific control handlers

[yuji2.ishikawa]

Hello Laurent, Hans,

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Monday, August 21, 2023 9:34 PM
> To: Hans Verkuil <hverkuil@xs4all.nl>
> Cc: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@toshiba.co.jp>; Sakari Ailus <sakari.ailus@iki.fi>; Mauro
> Carvalho Chehab <mchehab@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○
> ＯＳＴ) <nobuhiro1.iwamatsu@toshiba.co.jp>; Mark Brown
> <broonie@kernel.org>; linux-media@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v7 3/5] media: add V4L2 vendor specific control handlers
> 
> On Mon, Aug 21, 2023 at 02:28:11PM +0200, Hans Verkuil wrote:
> > On 14/07/2023 03:50, Yuji Ishikawa wrote:
> > > Add support to Image Signal Processors of Visconti's Video Input Interface.
> > > This patch adds vendor specific compound controls to configure the
> > > image signal processor.
> > >
> > > Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>
> > > ---
> > > Changelog v2:
> > > - Resend v1 because a patch exceeds size limit.
> > >
> > > Changelog v3:
> > > - 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 the v3 patch exceeds size limit
> > > - Stop using ID number to identify driver instance:
> > >   - Use dynamically allocated structure to hold HW specific context,
> > >     instead of static one.
> > >   - Call HW layer functions with the context structure instead of ID
> > > number
> > >
> > > Changelog v5:
> > > - no change
> > >
> > > Changelog v6:
> > > - remove unused macros
> > > - removed hwd_ and HWD_ prefix
> > > - update source code documentation
> > > - Suggestion from Hans Verkuil
> > >   - pointer to userland memory is removed from uAPI arguments
> > >     - style of structure is now "nested" instead of "chained by pointer";
> > >   - use div64_u64 for 64bit division
> > >   - vendor specific controls support TRY_EXT_CTRLS
> > >   - add READ_ONLY flag to GET_CALIBRATION_STATUS control and
> similar ones
> > >   - human friendry control names for vendor specific controls
> > >   - add initial value to each vendor specific control
> > >   - GET_LAST_CAPTURE_STATUS control is updated asyncnously from
> workqueue
> > >   - remove EXECUTE_ON_WRITE flag of vendor specific control
> > >   - uAPI: return value of GET_CALIBRATION_STATUS follows common
> rules of error codes
> > >   - applied v4l2-compliance
> > > - Suggestion from Sakari Ailus
> > >   - use div64_u64 for 64bit division
> > >   - update copyright's year
> > >   - remove redandunt cast
> > >   - use bool instead of HWD_VIIF_ENABLE/DISABLE
> > >   - simplify comparison to 0
> > >   - simplify statements with trigram operator
> > >   - remove redundant local variables
> > >   - use general integer types instead of u32/s32
> > > - Suggestion from Laurent Pinchart
> > >   - moved VIIF driver to driver/platform/toshiba/visconti
> > >   - change register access: struct-style to macro-style
> > >   - remove unused type definitions
> > >   - define enums instead of successive macro constants
> > >   - remove redundant parenthesis of macro constant
> > >   - embed struct hwd_res into struct viif_device
> > >   - use xxx_dma instead of xxx_paddr for variable names of IOVA
> > >   - literal value: just 0 instead of 0x0
> > >   - use literal 1 or 0 instead of HWD_VIIF_ENABLE, DISABLE for register
> access
> > >   - use true or false instead of HWD_VIIF_ENABLE, DISABLE for function
> calls
> > >   - uAPI: return value of GET_CALIBRATION_STATUS follows common
> > > rules of error codes
> > >
> > > Changelog v7:
> > > - remove unused variables
> > > - split long statements which have multiple logical-OR and trigram
> > > operators
> > >
> > >  .../media/platform/toshiba/visconti/Makefile  |    2 +-
> > >  .../media/platform/toshiba/visconti/viif.c    |   10 +-
> > >  .../platform/toshiba/visconti/viif_controls.c | 3407
> +++++++++++++++++
> > >  .../platform/toshiba/visconti/viif_controls.h |   18 +
> > >  .../platform/toshiba/visconti/viif_isp.c      |   15 +-
> > >  5 files changed, 3435 insertions(+), 17 deletions(-)  create mode
> > > 100644 drivers/media/platform/toshiba/visconti/viif_controls.c
> > >  create mode 100644
> > > drivers/media/platform/toshiba/visconti/viif_controls.h
> > >
> > > diff --git a/drivers/media/platform/toshiba/visconti/Makefile
> > > b/drivers/media/platform/toshiba/visconti/Makefile
> > > index 5f2f9199c..a28e6fa84 100644
> > > --- a/drivers/media/platform/toshiba/visconti/Makefile
> > > +++ b/drivers/media/platform/toshiba/visconti/Makefile
> > > @@ -3,6 +3,6 @@
> > >  # Makefile for the Visconti video input device driver  #
> > >
> > > -visconti-viif-objs = viif.o viif_capture.o viif_isp.o viif_csi2rx.o
> > > viif_common.o
> > > +visconti-viif-objs = viif.o viif_capture.o viif_controls.o
> > > +viif_isp.o viif_csi2rx.o viif_common.o
> > >
> > >  obj-$(CONFIG_VIDEO_VISCONTI_VIIF) += visconti-viif.o diff --git
> > > a/drivers/media/platform/toshiba/visconti/viif.c
> > > b/drivers/media/platform/toshiba/visconti/viif.c
> > > index c07dc2626..1b3d61abf 100644
> > > --- a/drivers/media/platform/toshiba/visconti/viif.c
> > > +++ b/drivers/media/platform/toshiba/visconti/viif.c
> > > @@ -18,6 +18,7 @@
> > >
> > >  #include "viif.h"
> > >  #include "viif_capture.h"
> > > +#include "viif_controls.h"
> > >  #include "viif_csi2rx.h"
> > >  #include "viif_common.h"
> > >  #include "viif_isp.h"
> > > @@ -178,12 +179,9 @@ static struct viif_subdev
> > > *to_viif_subdev(struct v4l2_async_subdev *asd)
> > >  /* before a userland capture application is trigered by
> > > vb2_buffer_done() */  static void
> > > visconti_viif_wthread_l1info(struct work_struct *work)  {
> > > -	/* called function is implemented by the next patch */
> > > -/*
> > > - *	struct viif_device *viif_dev = container_of(work, struct viif_device,
> work);
> > > - *
> > > - *	visconti_viif_save_l1_info(viif_dev);
> > > - */
> > > +	struct viif_device *viif_dev = container_of(work, struct
> > > +viif_device, work);
> > > +
> > > +	visconti_viif_save_l1_info(viif_dev);
> > >  }
> > >
> > >  static void viif_vsync_irq_handler_w_isp(struct viif_device
> > > *viif_dev) diff --git
> > > a/drivers/media/platform/toshiba/visconti/viif_controls.c
> > > b/drivers/media/platform/toshiba/visconti/viif_controls.c
> > > new file mode 100644
> > > index 000000000..3cf10e15c
> > > --- /dev/null
> > > +++ b/drivers/media/platform/toshiba/visconti/viif_controls.c
> > > @@ -0,0 +1,3407 @@
> >
> > <snip>
> >
> > > +static int visconti_viif_isp_try_ctrl(struct v4l2_ctrl *ctrl) {
> > > +	struct viif_device *viif_dev = ctrl->priv;
> > > +
> > > +	switch (ctrl->id) {
> > > +	case V4L2_CID_VISCONTI_VIIF_MAIN_SET_RAWPACK_MODE:
> > > +		return viif_main_set_rawpack_mode_try(viif_dev,
> ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_INPUT_MODE:
> > > +		return viif_l1_set_input_mode_try(viif_dev, ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_RGB_TO_Y_COEF:
> > > +		return viif_l1_set_rgb_to_y_coef_try(viif_dev, ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AG_MODE:
> > > +		return viif_l1_set_ag_mode_try(viif_dev, ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AG:
> > > +		return 0; //no need to check
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRE:
> > > +		return viif_l1_set_hdre_try(viif_dev, ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_IMG_EXTRACTION:
> > > +		return viif_l1_set_img_extraction_try(viif_dev, ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_DPC:
> > > +		return viif_l1_set_dpc_try(viif_dev, ctrl->p_new.p);
> > > +	case
> V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_PRESET_WHITE_BALANCE:
> > > +		return viif_l1_set_preset_white_balance_try(viif_dev,
> ctrl->p_new.p);
> > > +	case
> V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_RAW_COLOR_NOISE_REDUCTION:
> > > +		return viif_l1_set_raw_color_noise_reduction_try(viif_dev,
> ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRS:
> > > +		return viif_l1_set_hdrs_try(viif_dev, ctrl->p_new.p);
> > > +	case
> V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_BLACK_LEVEL_CORRECTION:
> > > +		return viif_l1_set_black_level_correction_try(viif_dev,
> ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_LSC:
> > > +		return viif_l1_set_lsc_try(viif_dev, ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_MAIN_PROCESS:
> > > +		return viif_l1_set_main_process_try(viif_dev, ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AWB:
> > > +		return viif_l1_set_awb_try(viif_dev, ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_LOCK_AWB_GAIN:
> > > +		return 0;
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRC:
> > > +		return viif_l1_set_hdrc_try(viif_dev, ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_HDRC_LTM:
> > > +		return viif_l1_set_hdrc_ltm_try(viif_dev, ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_GAMMA:
> > > +		return viif_l1_set_gamma_try(viif_dev, ctrl->p_new.p);
> > > +	case
> V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_IMG_QUALITY_ADJUSTMENT:
> > > +		return viif_l1_set_img_quality_adjustment_try(viif_dev,
> ctrl->p_new.p);
> > > +	case
> V4L2_CID_VISCONTI_VIIF_ISP_L1_SET_AVG_LUM_GENERATION:
> > > +		return viif_l1_set_avg_lum_generation_try(viif_dev,
> ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_UNDIST:
> > > +		return viif_l2_set_undist_try(viif_dev, ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_ROI:
> > > +		return viif_l2_set_roi_try(viif_dev, ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_ISP_L2_SET_GAMMA:
> > > +		return viif_l2_set_gamma_try(viif_dev, ctrl->p_new.p);
> > > +	case V4L2_CID_VISCONTI_VIIF_GET_LAST_CAPTURE_STATUS:
> > > +		return 0;
> > > +	default:
> > > +		pr_info("unknown_ctrl:t: id=%08X val=%d", ctrl->id, ctrl->val);
> > > +		break;
> > > +	}
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static int visconti_viif_isp_set_ctrl(struct v4l2_ctrl *ctrl) {
> > > +	struct viif_device *viif_dev = ctrl->priv;
> > > +	int ret;
> > > +
> > > +	pr_info("isp_set_ctrl: %s", ctrl->name);
> >
> > Don't use pr_info for what is just a debug message! Either drop it, or
> > replace it with dev_dbg.
> 
> In drivers, almost all occurences of pr_*() should be replaced by dev_*(). It's very
> rare that pr_*() would be the right API.
> 
> In this specific case, I'd just drop it.
> 

I'll remove pr_*() calls.

> > > +	if (pm_runtime_status_suspended(viif_dev->dev)) {
> > > +		pr_info("warning: visconti viif HW is not powered");
> >
> > And here pr_info is used for a warning, so shouldn't this be dev_warn?
> 
> I don't think there's a need to warn about this, it's a normal situation.
> 
> The right runtime PM API here is pm_runtime_get_if_in_use() by the way, not
> pm_runtime_status_suspended(). Don't forget to call pm_runtime_put() at the
> end of the function.
> 

I'll remove this pr_info() call.
Also, I'll use pm_runtime_get_if_in_use() instead of pm_runtime_status_suspended().

> > I see pr_info being used in a lot of places where it doesn't belong
> > and would just spam the kernel log.
> >
> > Something to go through for v8.
> 
> --
> Regards,
> 
> Laurent Pinchart

Regards,
Yuji
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v7 2/5] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver

[yuji2.ishikawa]

Hello Robin, Laurent,

> -----Original Message-----
> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Monday, August 21, 2023 10:36 PM
> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; ishikawa yuji(石川
> 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) <yuji2.ishikawa@toshiba.co.jp>
> Cc: krzysztof.kozlowski@linaro.org; hverkuil@xs4all.nl; sakari.ailus@iki.fi;
> mchehab@kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ＯＳ
> Ｔ) <nobuhiro1.iwamatsu@toshiba.co.jp>; broonie@kernel.org;
> linux-media@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Christoph
> Hellwig <hch@lst.de>; Marek Szyprowski <m.szyprowski@samsung.com>
> Subject: Re: [PATCH v7 2/5] media: platform: visconti: Add Toshiba Visconti
> Video Input Interface driver
> 
> On 2023-08-21 14:19, Laurent Pinchart wrote:
> [...]
> >>>> +	viif_dev->tables =
> >>>> +		dma_alloc_wc(dev, sizeof(struct viif_table_area),
> &tables_dma, GFP_KERNEL);
> >>>> +	if (!viif_dev->tables) {
> >>>> +		dev_err(dev, "dma_alloc_wc failed\n");
> >>>
> >>> Are you sure DMA memory allocation errors shall be printed?
> >>
> >> Printing this error is useless for users in general?
> >> If so, I'll drop this debug output.
> >
> > Failures to allocate memory in the kernel generally result in warning
> > messages being printed by the allocation function, so there's no need
> > to do so manually in drivers. This being said, I check dma_alloc_wc()
> > (which is a wrapper around dma_alloc_attrs()), and unless I'm missing
> > something, it can return NULL without printing any error. I don't know
> > if this is an oversight in some code paths taken by dma_alloc_attrs()
> > or if it's on purpose. Maybe Christoph, Marek or Roben will known.
> 
> Yeah, there might be a few edge cases, but in most cases
> dma_alloc_attrs() will end up falling back to the page allocator as a last resort if
> all the more preferred allocation options fail, and thus complete failure should
> eventually cause that to scream unless DMA_ATTR_NO_WARN was specified.

I understand there's no need for printing this error message at driver code.
I'll remove dev_err().

> Thanks,
> Robin.

Regards,
Yuji
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel