| Message ID | 20220414053528.31460-1-yuji2.ishikawa@toshiba.co.jp (mailing list archive) |
|---|---|
| Headers | show |
| Series | Visconti: Add Toshiba Visconti Video Input Interface driver | expand |
Hi Yuji, On 14/04/2022 07:35, Yuji Ishikawa wrote: > This series is the Video Input Interface driver for Toshiba's ARM SoC, Visconti[0]. > This provides DT binding documentation, device driver, MAINTAINER fiels. > > Best regards, > Yuji > > [0]: https://toshiba.semicon-storage.com/ap-en/semiconductor/product/image-recognition-processors-visconti.html > > > dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface bindings > v1 -> v2: > - No update > > media: platform: visconti: Add Toshiba Visconti Video Input Interface driver headers > v1 -> v2: > - moved driver headers to an individual patch > > media: platform: visconti: Add Toshiba Visconti Video Input Interface driver body > v1 -> v2: > - moved driver sources to an individual patch > > media: platform: visconti: Add Toshiba VIIF image signal processor driver > v1 -> v2: > - moved image signal processor driver to an individual patch > > MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface > v1 -> v2: > - No update > > Change in V2: > - moved files into individual patches to decrease patch size Thank you for your patch series. However, there are quite a few things that need more work. I'll make some high level guidelines here, and go into a bit more detail in some of the patches. First of all, run your patches through 'scripts/checkpatch.pl --strict' and fix the many warnings, errors and checks. Use common sense, sometimes a check or warning isn't actually valid, but the vast majority of what checkpatch spits out appears reasonable. Another thing I noticed is code like this: + if (param->r_cr_in_offset > HWD_VIIF_CSC_MAX_OFFSET) + return -EINVAL; + + if (param->g_y_in_offset > HWD_VIIF_CSC_MAX_OFFSET) + return -EINVAL; + + if (param->b_cb_in_offset > HWD_VIIF_CSC_MAX_OFFSET) + return -EINVAL; + + if (param->r_cr_out_offset > HWD_VIIF_CSC_MAX_OFFSET) + return -EINVAL; + + if (param->g_y_out_offset > HWD_VIIF_CSC_MAX_OFFSET) + return -EINVAL; + + if (param->b_cb_out_offset > HWD_VIIF_CSC_MAX_OFFSET) + return -EINVAL; This can easily be combined into a single if: if (param->r_cr_in_offset > HWD_VIIF_CSC_MAX_OFFSET || param->g_y_in_offset > HWD_VIIF_CSC_MAX_OFFSET || param->b_cb_in_offset > HWD_VIIF_CSC_MAX_OFFSET || param->r_cr_out_offset > HWD_VIIF_CSC_MAX_OFFSET || param->g_y_out_offset > HWD_VIIF_CSC_MAX_OFFSET || param->b_cb_out_offset > HWD_VIIF_CSC_MAX_OFFSET) return -EINVAL; Easier to read and a lot shorter. Another thing to avoid is mixing lower and upper case in function names. A lot of functions have this prefix: 'hwd_VIIF_'. Just change that to 'hwd_viif_': that's much easier on the eyes. I also see a fair amount of code that is indented very far to the right. Often due to constructs like this: if (test) { // lots of code } return ret; Which can be changed to: if (!test) return ret; // lots of code return ret; The same can also happen in a for/while loop where you can just 'continue' instead of 'return'. This makes the code easier to read and review. It doesn't look like this driver uses the media controller API. This is probably something you want to look into, esp. in combination with libcamera support (https://libcamera.org/). I've added Laurent to this, since he's the expert on this. Regards, Hans > > Yuji Ishikawa (5): > dt-bindings: media: platform: visconti: Add Toshiba Visconti Video > Input Interface bindings > media: platform: visconti: Add Toshiba Visconti Video Input Interface > driver headers > media: platform: visconti: Add Toshiba Visconti Video Input Interface > driver body > media: platform: visconti: Add Toshiba VIIF image signal processor > driver > MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface > > .../bindings/media/toshiba,visconti-viif.yaml | 103 + > MAINTAINERS | 2 + > drivers/media/platform/Kconfig | 2 + > drivers/media/platform/Makefile | 4 + > drivers/media/platform/visconti/Kconfig | 9 + > drivers/media/platform/visconti/Makefile | 9 + > drivers/media/platform/visconti/hwd_viif.c | 2233 ++++++++++ > drivers/media/platform/visconti/hwd_viif.h | 1776 ++++++++ > .../media/platform/visconti/hwd_viif_csi2rx.c | 767 ++++ > .../platform/visconti/hwd_viif_internal.h | 361 ++ > .../media/platform/visconti/hwd_viif_l1isp.c | 3769 +++++++++++++++++ > .../media/platform/visconti/hwd_viif_reg.h | 2802 ++++++++++++ > drivers/media/platform/visconti/viif.c | 2384 +++++++++++ > drivers/media/platform/visconti/viif.h | 134 + > include/uapi/linux/visconti_viif.h | 1683 ++++++++ > 15 files changed, 16038 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml > create mode 100644 drivers/media/platform/visconti/Kconfig > create mode 100644 drivers/media/platform/visconti/Makefile > create mode 100644 drivers/media/platform/visconti/hwd_viif.c > create mode 100644 drivers/media/platform/visconti/hwd_viif.h > create mode 100644 drivers/media/platform/visconti/hwd_viif_csi2rx.c > create mode 100644 drivers/media/platform/visconti/hwd_viif_internal.h > create mode 100644 drivers/media/platform/visconti/hwd_viif_l1isp.c > create mode 100644 drivers/media/platform/visconti/hwd_viif_reg.h > create mode 100644 drivers/media/platform/visconti/viif.c > create mode 100644 drivers/media/platform/visconti/viif.h > create mode 100644 include/uapi/linux/visconti_viif.h >
On 14/04/2022 07:35, Yuji Ishikawa wrote: > Add support to Video Input Interface on Toshiba Visconti Video Input Interface driver. > The Video Input Interface includes CSI2 receiver, frame grabber and image signal processor. > Headers in this commit provide definitions of data-structure and hardware registers. > > Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp> > Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > --- > v1 -> v2: > - moved driver headers to this patch; to decrease patch size > --- > drivers/media/platform/visconti/hwd_viif.h | 834 +++++ > .../platform/visconti/hwd_viif_internal.h | 361 +++ > .../media/platform/visconti/hwd_viif_reg.h | 2802 +++++++++++++++++ > drivers/media/platform/visconti/viif.h | 134 + > include/uapi/linux/visconti_viif.h | 356 +++ > 5 files changed, 4487 insertions(+) > create mode 100644 drivers/media/platform/visconti/hwd_viif.h > create mode 100644 drivers/media/platform/visconti/hwd_viif_internal.h > create mode 100644 drivers/media/platform/visconti/hwd_viif_reg.h > create mode 100644 drivers/media/platform/visconti/viif.h > create mode 100644 include/uapi/linux/visconti_viif.h <snip> > diff --git a/include/uapi/linux/visconti_viif.h b/include/uapi/linux/visconti_viif.h > new file mode 100644 > index 000000000..a235b4d7c > --- /dev/null > +++ b/include/uapi/linux/visconti_viif.h > @@ -0,0 +1,356 @@ > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > +/* Toshiba Visconti Video Capture Support > + * > + * (C) Copyright 2022 TOSHIBA CORPORATION > + * (C) Copyright 2022 Toshiba Electronic Devices & Storage Corporation > + */ > + > +#ifndef __UAPI_VISCONTI_VIIF_H_ > +#define __UAPI_VISCONTI_VIIF_H_ > + > +#include <linux/types.h> > +#include <linux/videodev2.h> > + > +/* Private IPCTLs */ Typo: IPCTLs -> IOCTLs > +#define VIDIOC_VIIF_MAIN_SET_RAWPACK_MODE \ > + _IOW('V', BASE_VIDIOC_PRIVATE + 1, uint32_t) > +#define VIDIOC_VIIF_L2_SET_UNDIST \ > + _IOW('V', BASE_VIDIOC_PRIVATE + 21, struct viif_l2_undist_config) > +#define VIDIOC_VIIF_L2_SET_ROI \ > + _IOW('V', BASE_VIDIOC_PRIVATE + 22, struct viif_l2_roi_config) > +#define VIDIOC_VIIF_L2_SET_GAMMA \ > + _IOW('V', BASE_VIDIOC_PRIVATE + 23, struct viif_l2_gamma_config) > +#define VIDIOC_VIIF_L2_SET_CROP \ > + _IOW('V', BASE_VIDIOC_PRIVATE + 24, struct viif_l2_crop_config) > +#define VIDIOC_VIIF_CSI2RX_SET_MBUS_FMT \ > + _IOW('V', BASE_VIDIOC_PRIVATE + 25, uint32_t) > +#define VIDIOC_VIIF_CSI2RX_GET_CALIBRATION_STATUS \ > + _IOR('V', BASE_VIDIOC_PRIVATE + 26, \ > + struct viif_csi2rx_dphy_calibration_status) > +#define VIDIOC_VIIF_CSI2RX_GET_ERR_STATUS \ > + _IOR('V', BASE_VIDIOC_PRIVATE + 27, struct viif_csi2rx_err_status) > +#define VIDIOC_VIIF_ISP_GET_LAST_CAPTURE_STATUS \ > + _IOR('V', BASE_VIDIOC_PRIVATE + 28, struct viif_isp_capture_status) We really don't want to introduce private ioctls in a public API. It's hard to maintain, and you would need very good reasons to go this route. A better choice is either using compound controls, as is used by stateless codecs: https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/ext-ctrls-codec-stateless.html or streaming metadata, as is done by the rkisp1 driver: https://linuxtv.org/downloads/v4l-dvb-apis-new/admin-guide/rkisp1.html In both cases you have to make sure that the data layout is the same regardless of whether you run on a 32 bit or 64 bit OS. I.e. if the kernel is 64 bit (arm64) but the application is compiled for 32 bit, then you don't want to have to convert between the two layouts. The pahole utility is very helpful for checking this. Actually, the same issue is present when using private ioctls. > + > +/* Enable/Disable flag */ > +#define VIIF_DISABLE (0U) > +#define VIIF_ENABLE (1U) > + > +/** > + * enum viif_rawpack_mode - RAW pack mode for ioctl(VIDIOC_VIIF_MAIN_SET_RAWPACK_MODE) > + * > + * @VIIF_RAWPACK_DISABLE: RAW pack disable > + * @VIIF_RAWPACK_MSBFIRST: RAW pack enable (MSB First) > + * @VIIF_RAWPACK_LSBFIRST: RAW pack enable (LSB First) > + */ > +enum viif_rawpack_mode { > + VIIF_RAWPACK_DISABLE = 0, > + VIIF_RAWPACK_MSBFIRST = 2, > + VIIF_RAWPACK_LSBFIRST = 3, > +}; > + > +/* L2ISP undistortion mode */ > +enum viif_l2_undist_mode { > + VIIF_L2_UNDIST_POLY = 0, /* polynomial mode */ > + VIIF_L2_UNDIST_GRID = 1, /* grid table mode */ > + VIIF_L2_UNDIST_POLY_TO_GRID = 2, /* polynomial, then grid table mode */ > + VIIF_L2_UNDIST_GRID_TO_POLY = 3, /* grid table, then polynomial mode */ > +}; > + > +/** > + * struct viif_l2_undist - L2ISP UNDIST parameters > + * for &struct viif_l2_undist_config > + * @through_mode: 1:enable or 0:disable through mode of undistortion > + * @roi_mode: :ref:`L2ISP undistortion mode <L2ISP_undistortion_mode>` > + * @sensor_crop_ofs_h: Horizontal start position of sensor crop area[pixel] > + * [-4296..4296], accuracy: 1/2 > + * @sensor_crop_ofs_v: Vertical start position of sensor crop area[line] > + * [-2360..2360], accuracy: 1/2 > + * @norm_scale: Normalization coefficient for distance from center > + * [0..1677721], accuracy: 1/33554432 > + * @valid_r_norm2_poly: Setting target area for polynomial correction > + * [0..0x3FFFFFF], accuracy: 1/33554432 > + * @valid_r_norm2_grid: Setting target area for grid table correction > + * [0..0x3FFFFFF], accuracy: 1/33554432 > + * @roi_write_area_delta: Error adjustment value of forward function and > + * inverse function for pixel position calculation > + * [0..0x7FF], accuracy: 1/1024 > + * @poly_write_g_coef: 10th-order polynomial coefficient for G write pixel position calculation > + * [-2147352576..2147352576], accuracy: 1/131072 > + * @poly_read_b_coef: 10th-order polynomial coefficient for B read pixel position calculation > + * [-2147352576..2147352576], accuracy: 1/131072 > + * @poly_read_g_coef: 10th-order polynomial coefficient for G read pixel position calculation > + * [-2147352576..2147352576], accuracy: 1/131072 > + * @poly_read_r_coef: 10th-order polynomial coefficient for R read pixel position calculation > + * [-2147352576..2147352576], accuracy: 1/131072 > + * @grid_node_num_h: Number of horizontal grids [16..64] > + * @grid_node_num_v: Number of vertical grids [16..64] > + * @grid_patch_hsize_inv: Inverse pixel size between horizontal grids > + * [0..0x7FFFFF], accuracy: 1/8388608 > + * @grid_patch_vsize_inv: Inverse pixel size between vertical grids > + * [0..0x7FFFFF], accuracy: 1/8388608 > + */ > +struct viif_l2_undist { > + uint32_t through_mode; > + uint32_t roi_mode; > + int32_t sensor_crop_ofs_h; > + int32_t sensor_crop_ofs_v; > + uint32_t norm_scale; > + uint32_t valid_r_norm2_poly; > + uint32_t valid_r_norm2_grid; > + uint32_t roi_write_area_delta; > + int32_t poly_write_g_coef[11]; > + int32_t poly_read_b_coef[11]; > + int32_t poly_read_g_coef[11]; > + int32_t poly_read_r_coef[11]; > + uint32_t grid_node_num_h; > + uint32_t grid_node_num_v; > + uint32_t grid_patch_hsize_inv; > + uint32_t grid_patch_vsize_inv; > +}; > +/** > + * struct viif_l2_undist_config - L2ISP UNDIST parameters > + * for :ref:`VIDIOC_VIIF_L2_SET_UNDIST` > + * @param: &struct viif_l2_undist > + * @write_g: Write for G Grid table address. > + * Table is not transferred if a NULL pointer is set > + * @read_b: Read for B Grid table address. > + * Table is not transferred if a NULL pointer is set > + * @read_g: Read for G Grid table address. > + * Table is not transferred if a NULL pointer is set > + * @read_r: Read for R Grid table address. > + * Table is not transferred if a NULL pointer is set > + * @size: Table size [byte]. Range: [1024..8192] or 0. > + * Should be set to "grid_node_num_h * grid_node_num_v * 4". > + * Refer to &struct viif_l2_undist. > + * Should set 0 in case NULL is set for all tables. > + * Should set size other than 0 in case If other is set in more than one table. > + * > + * Application should make sure that the table data is based on HW specification > + * since this driver does not check the contents of specified grid table. > + */ > +struct viif_l2_undist_config { > + struct viif_l2_undist param; > + uint32_t *write_g; > + uint32_t *read_b; > + uint32_t *read_g; > + uint32_t *read_r; Pointers in a public API are possibly but it really complicates the code. When using controls this can be done by placing these tables in separate controls using the upcoming 'Dynamic Array' support. Patches adding support for that are part of this series: https://lore.kernel.org/linux-media/20220407152940.738159-1-benjamin.gaignard@collabora.com/T/#t > + uint32_t size; > +}; > + > +/** > + * struct viif_l2_roi_config - L2ISP ROI parameters > + * for :ref:`VIDIOC_VIIF_L2_SET_ROI` > + * @roi_scale: Scale value for each ROI [32768..131072], accuracy: 1/65536 > + * @roi_scale_inv: Inverse scale value for each ROI [32768..131072], accuracy: 1/65536 > + * @corrected_wo_scale_hsize: Corrected image width for each ROI [pixel] [128..8190] > + * @corrected_wo_scale_vsize: Corrected image height for each ROI [line] [128..4094] > + * @corrected_hsize: Corrected and scaled image width for each ROI [pixel] [128..8190] > + * @corrected_vsize: Corrected and scaled image height for each ROI [line] [128..4094] > + */ > +struct viif_l2_roi_config { > + uint32_t roi_scale; > + uint32_t roi_scale_inv; > + uint32_t corrected_wo_scale_hsize; > + uint32_t corrected_wo_scale_vsize; > + uint32_t corrected_hsize; > + uint32_t corrected_vsize; > +}; > + > +/** enum viif_gamma_mode - Gamma correction mode > + * > + * @VIIF_GAMMA_COMPRESSED: compressed table mode > + * @VIIF_GAMMA_LINEAR: liner table mode liner -> linear > + */ > +enum viif_gamma_mode { > + VIIF_GAMMA_COMPRESSED = 0, > + VIIF_GAMMA_LINEAR = 1, > +}; > + > +/** > + * struct viif_l2_gamma_config - L2ISP gamma correction parameters > + * for :ref:`VIDIOC_VIIF_L2_SET_GAMMA` > + * @enable: 1:Enable, 0:Disable settings of L2ISP gamma correction control > + * @vsplit: Line switching position of first table and second table [line] [0..4094]. > + * Should set 0 in case 0 is set to @enable > + * @mode: :ref:`Gamma correction mode <Gamma_correction_mode>`. > + * Should set VIIF_GAMMA_COMPRESSED in case 0 is set to @enable > + * @table: Table address. > + * Gamma table is not transferred if a NULL pointer is set to table. > + * The size of each table is fixed to 512 bytes. > + * [0]: G/Y(1st table), [1]: G/Y(2nd table), [2]: B/U(1st table) > + * [3]: B/U(2nd table), [4]: R/V(1st table), [5]: R/V(2nd table) > + */ > +struct viif_l2_gamma_config { > + uint32_t enable; > + uint32_t vsplit; > + uint32_t mode; > + uint16_t *table[6]; > +}; > + > +/** > + * struct viif_l2_crop_config - L2ISP Cropping parameters > + * for :ref:`VIDIOC_VIIF_L2_SET_CROP` > + * @x: X coordinate position > + * (with the upper left corner of the image as the origin)[pixel] [0..8062] > + * @y: Y coordinate position > + * (with the upper left corner of the image as the origin)[Line] [0..3966] > + * @w: Image width[pixel] [128..8190] > + * @h: Image height[pixel] [128..4094] > + */ > +struct viif_l2_crop_config { > + uint32_t x; > + uint32_t y; > + uint32_t w; > + uint32_t h; > +}; > + > +/** > + * enum viif_csi2_cal_status - CSI2RX calibration status > + * > + * @VIIF_CSI2_CAL_NOT_DONE: Calibration not complete > + * @VIIF_CSI2_CAL_SUCCESS: Calibration success > + * @VIIF_CSI2_CAL_FAIL: Calibration failed > + */ > +enum viif_csi2_cal_status { > + VIIF_CSI2_CAL_NOT_DONE = 0, > + VIIF_CSI2_CAL_SUCCESS = 1, > + VIIF_CSI2_CAL_FAIL = 2, > +}; > + > +/** > + * struct viif_csi2rx_dphy_calibration_status - CSI2-RX D-PHY Calibration > + * information for :ref:`VIDIOC_VIIF_CSI2RX_GET_CALIBRATION_STATUS` > + * @term_cal_with_rext: Result of termination calibration with rext > + * @clock_lane_offset_cal: Result of offset calibration of clock lane > + * @data_lane0_offset_cal: Result of offset calibration of data lane0 > + * @data_lane1_offset_cal: Result of offset calibration of data lane1 > + * @data_lane2_offset_cal: Result of offset calibration of data lane2 > + * @data_lane3_offset_cal: Result of offset calibration of data lane3 > + * @data_lane0_ddl_tuning_cal: Result of digital delay line tuning calibration of data lane0 > + * @data_lane1_ddl_tuning_cal: Result of digital delay line tuning calibration of data lane1 > + * @data_lane2_ddl_tuning_cal: Result of digital delay line tuning calibration of data lane2 > + * @data_lane3_ddl_tuning_cal: Result of digital delay line tuning calibration of data lane3 > + * > + * Refer to :ref:`CSI2-RX calibration status <CSI2RX_calibration_status>` > + * for the definitions of each member > + */ > +struct viif_csi2rx_dphy_calibration_status { > + uint32_t term_cal_with_rext; > + uint32_t clock_lane_offset_cal; > + uint32_t data_lane0_offset_cal; > + uint32_t data_lane1_offset_cal; > + uint32_t data_lane2_offset_cal; > + uint32_t data_lane3_offset_cal; > + uint32_t data_lane0_ddl_tuning_cal; > + uint32_t data_lane1_ddl_tuning_cal; > + uint32_t data_lane2_ddl_tuning_cal; > + uint32_t data_lane3_ddl_tuning_cal; > +}; > + > +/** > + * struct viif_csi2rx_err_status - CSI2RX Error status parameters > + * for :ref:`VIDIOC_VIIF_CSI2RX_GET_ERR_STATUS` > + * @err_phy_fatal: D-PHY FATAL error. > + * bit[3]: Start of transmission error on DATA Lane3. > + * bit[2]: Start of transmission error on DATA Lane2. > + * bit[1]: Start of transmission error on DATA Lane1. > + * bit[0]: Start of transmission error on DATA Lane0. > + * @err_pkt_fatal: Packet FATAL error. > + * bit[16]: Header ECC contains 2 errors, unrecoverable. > + * bit[3]: Checksum error detected on virtual channel 3. > + * bit[2]: Checksum error detected on virtual channel 2. > + * bit[1]: Checksum error detected on virtual channel 1. > + * bit[0]: Checksum error detected on virtual channel 0. > + * @err_frame_fatal: Frame FATAL error. > + * bit[19]: Last received Frame, in virtual channel 3, has at least one CRC error. > + * bit[18]: Last received Frame, in virtual channel 2, has at least one CRC error. > + * bit[17]: Last received Frame, in virtual channel 1, has at least one CRC error. > + * bit[16]: Last received Frame, in virtual channel 0, has at least one CRC error. > + * bit[11]: Incorrect Frame Sequence detected in virtual channel 3. > + * bit[10]: Incorrect Frame Sequence detected in virtual channel 2. > + * bit[9]: Incorrect Frame Sequence detected in virtual channel 1. > + * bit[8]: Incorrect Frame Sequence detected in virtual channel 0. > + * bit[3]: Error matching Frame Start with Frame End for virtual channel 3. > + * bit[2]: Error matching Frame Start with Frame End for virtual channel 2. > + * bit[1]: Error matching Frame Start with Frame End for virtual channel 1. > + * bit[0]: Error matching Frame Start with Frame End for virtual channel 0. > + * @err_phy: D-PHY error. > + * bit[19]: Escape Entry Error on Data Lane 3. > + * bit[18]: Escape Entry Error on Data Lane 2. > + * bit[17]: Escape Entry Error on Data Lane 1. > + * bit[16]: Escape Entry Error on Data Lane 0. > + * bit[3]: Start of Transmission Error on Data Lane 3 (synchronization can still be achieved). > + * bit[2]: Start of Transmission Error on Data Lane 2 (synchronization can still be achieved). > + * bit[1]: Start of Transmission Error on Data Lane 1 (synchronization can still be achieved). > + * bit[0]: Start of Transmission Error on Data Lane 0 (synchronization can still be achieved). > + * @err_pkt: Packet error. > + * bit[19]: Header Error detected and corrected on virtual channel 3. > + * bit[18]: Header Error detected and corrected on virtual channel 2. > + * bit[17]: Header Error detected and corrected on virtual channel 1. > + * bit[16]: Header Error detected and corrected on virtual channel 0. > + * bit[3]: Unrecognized or unimplemented data type detected in virtual channel 3. > + * bit[2]: Unrecognized or unimplemented data type detected in virtual channel 2. > + * bit[1]: Unrecognized or unimplemented data type detected in virtual channel 1. > + * bit[0]: Unrecognized or unimplemented data type detected in virtual channel 0. > + * @err_line: Line error. > + * bit[23]: Error in the sequence of lines for vc7 and dt7. > + * bit[22]: Error in the sequence of lines for vc6 and dt6. > + * bit[21]: Error in the sequence of lines for vc5 and dt5. > + * bit[20]: Error in the sequence of lines for vc4 and dt4. > + * bit[19]: Error in the sequence of lines for vc3 and dt3. > + * bit[18]: Error in the sequence of lines for vc2 and dt2. > + * bit[17]: Error in the sequence of lines for vc1 and dt1. > + * bit[16]: Error in the sequence of lines for vc0 and dt0. > + * bit[7]: Error matching Line Start with Line End for vc7 and dt7. > + * bit[6]: Error matching Line Start with Line End for vc6 and dt6. > + * bit[5]: Error matching Line Start with Line End for vc5 and dt5. > + * bit[4]: Error matching Line Start with Line End for vc4 and dt4. > + * bit[3]: Error matching Line Start with Line End for vc3 and dt3. > + * bit[2]: Error matching Line Start with Line End for vc2 and dt2. > + * bit[1]: Error matching Line Start with Line End for vc1 and dt1. > + * bit[0]: Error matching Line Start with Line End for vc0 and dt0. > + */ > +struct viif_csi2rx_err_status { > + uint32_t err_phy_fatal; > + uint32_t err_pkt_fatal; > + uint32_t err_frame_fatal; > + uint32_t err_phy; > + uint32_t err_pkt; > + uint32_t err_line; > +}; > + > +/** > + * struct viif_l1_info - L1ISP AWB information > + * for &struct viif_isp_capture_status > + * @awb_ave_u: U average value of AWB adjustment [pixel] > + * @awb_ave_v: V average value of AWB adjustment [pixel] > + * @awb_accumulated_pixel: Accumulated pixel count of AWB adjustment > + * @awb_gain_r: R gain used in the next frame of AWB adjustment > + * @awb_gain_g: G gain used in the next frame of AWB adjustment > + * @awb_gain_b: B gain used in the next frame of AWB adjustment > + * @awb_status_u: U convergence state of AWB adjustment > + * (true: converged, false: not-converged) > + * @awb_status_v: V convergence state of AWB adjustment > + * (true: converged, false: not-converged) > + */ > +struct viif_l1_info { > + uint32_t awb_ave_u; > + uint32_t awb_ave_v; > + uint32_t awb_accumulated_pixel; > + uint32_t awb_gain_r; > + uint32_t awb_gain_g; > + uint32_t awb_gain_b; > + bool awb_status_u; > + bool awb_status_v; bool is not allowed in a userspace API. Use __u32 or something like that instead. Regards, Hans > +}; > +/** > + * struct viif_isp_capture_status - L1ISP capture information > + * for :ref:`VIDIOC_VIIF_ISP_GET_LAST_CAPTURE_STATUS` > + * @l1_info: L1ISP AWB information. Refer to &struct viif_l1_info > + */ > +struct viif_isp_capture_status { > + struct viif_l1_info l1_info; > +}; > + > +#endif /* __UAPI_VISCONTI_VIIF_H_ */
Hi, Hans Thank you for your review and comment. > -----Original Message----- > From: Hans Verkuil <hverkuil@xs4all.nl> > Sent: Wednesday, April 20, 2022 4:55 PM > To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) > <yuji2.ishikawa@toshiba.co.jp>; Mauro Carvalho Chehab > <mchehab@kernel.org>; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ) > <nobuhiro1.iwamatsu@toshiba.co.jp>; Laurent Pinchart > <laurent.pinchart@ideasonboard.com> > Cc: linux-media@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2 0/5] Visconti: Add Toshiba Visconti Video Input > Interface driver > > Hi Yuji, > > On 14/04/2022 07:35, Yuji Ishikawa wrote: > > This series is the Video Input Interface driver for Toshiba's ARM SoC, > Visconti[0]. > > This provides DT binding documentation, device driver, MAINTAINER fiels. > > > > Best regards, > > Yuji > > > > [0]: > > > https://toshiba.semicon-storage.com/ap-en/semiconductor/product/image- > > recognition-processors-visconti.html > > > > > > dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input > Interface bindings > > v1 -> v2: > > - No update > > > > media: platform: visconti: Add Toshiba Visconti Video Input Interface driver > headers > > v1 -> v2: > > - moved driver headers to an individual patch > > > > media: platform: visconti: Add Toshiba Visconti Video Input Interface driver > body > > v1 -> v2: > > - moved driver sources to an individual patch > > > > media: platform: visconti: Add Toshiba VIIF image signal processor driver > > v1 -> v2: > > - moved image signal processor driver to an individual patch > > > > MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface > > v1 -> v2: > > - No update > > > > Change in V2: > > - moved files into individual patches to decrease patch size > > Thank you for your patch series. > > However, there are quite a few things that need more work. I'll make some high > level guidelines here, and go into a bit more detail in some of the patches. > > First of all, run your patches through 'scripts/checkpatch.pl --strict' and fix the > many warnings, errors and checks. Use common sense, sometimes a check or > warning isn't actually valid, but the vast majority of what checkpatch spits out > appears reasonable. I'll execute checkpatch.pl with --strict option. > > Another thing I noticed is code like this: > > + if (param->r_cr_in_offset > HWD_VIIF_CSC_MAX_OFFSET) > + return -EINVAL; > + > + if (param->g_y_in_offset > HWD_VIIF_CSC_MAX_OFFSET) > + return -EINVAL; > + > + if (param->b_cb_in_offset > HWD_VIIF_CSC_MAX_OFFSET) > + return -EINVAL; > + > + if (param->r_cr_out_offset > HWD_VIIF_CSC_MAX_OFFSET) > + return -EINVAL; > + > + if (param->g_y_out_offset > HWD_VIIF_CSC_MAX_OFFSET) > + return -EINVAL; > + > + if (param->b_cb_out_offset > HWD_VIIF_CSC_MAX_OFFSET) > + return -EINVAL; > > This can easily be combined into a single if: > > if (param->r_cr_in_offset > HWD_VIIF_CSC_MAX_OFFSET || > param->g_y_in_offset > HWD_VIIF_CSC_MAX_OFFSET || > param->b_cb_in_offset > HWD_VIIF_CSC_MAX_OFFSET > || > param->r_cr_out_offset > HWD_VIIF_CSC_MAX_OFFSET > || > param->g_y_out_offset > HWD_VIIF_CSC_MAX_OFFSET > || > param->b_cb_out_offset > > HWD_VIIF_CSC_MAX_OFFSET) > return -EINVAL; > > Easier to read and a lot shorter. I'll fix them > > Another thing to avoid is mixing lower and upper case in function names. > A lot of functions have this prefix: 'hwd_VIIF_'. Just change that to > 'hwd_viif_': that's much easier on the eyes. I'll fix them. I'm also wondering if the common prefix hwd_viif_ is acceptable for the identifiers in Visconti VIIIF driver. > I also see a fair amount of code that is indented very far to the right. > Often due to constructs like this: > > if (test) { > // lots of code > } > return ret; > > Which can be changed to: > > if (!test) > return ret; > // lots of code > return ret; > > The same can also happen in a for/while loop where you can just 'continue' > instead of 'return'. > > This makes the code easier to read and review. Yes, the indents are too deep. I'll try fix them. > It doesn't look like this driver uses the media controller API. This is probably > something you want to look into, esp. in combination with libcamera support > (https://libcamera.org/). I've added Laurent to this, since he's the expert on > this. Thank you for great advice. I wanted to know how to control/aggregate functions of VIIF. As I'm completely new to media controller API, I'll check the documents first. Regards, Yuji > Regards, > > Hans > > > > > Yuji Ishikawa (5): > > dt-bindings: media: platform: visconti: Add Toshiba Visconti Video > > Input Interface bindings > > media: platform: visconti: Add Toshiba Visconti Video Input Interface > > driver headers > > media: platform: visconti: Add Toshiba Visconti Video Input Interface > > driver body > > media: platform: visconti: Add Toshiba VIIF image signal processor > > driver > > MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface > > > > .../bindings/media/toshiba,visconti-viif.yaml | 103 + > > MAINTAINERS | 2 + > > drivers/media/platform/Kconfig | 2 + > > drivers/media/platform/Makefile | 4 + > > drivers/media/platform/visconti/Kconfig | 9 + > > drivers/media/platform/visconti/Makefile | 9 + > > drivers/media/platform/visconti/hwd_viif.c | 2233 ++++++++++ > > drivers/media/platform/visconti/hwd_viif.h | 1776 ++++++++ > > .../media/platform/visconti/hwd_viif_csi2rx.c | 767 ++++ > > .../platform/visconti/hwd_viif_internal.h | 361 ++ > > .../media/platform/visconti/hwd_viif_l1isp.c | 3769 > +++++++++++++++++ > > .../media/platform/visconti/hwd_viif_reg.h | 2802 ++++++++++++ > > drivers/media/platform/visconti/viif.c | 2384 +++++++++++ > > drivers/media/platform/visconti/viif.h | 134 + > > include/uapi/linux/visconti_viif.h | 1683 ++++++++ > > 15 files changed, 16038 insertions(+) create mode 100644 > > Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml > > create mode 100644 drivers/media/platform/visconti/Kconfig > > create mode 100644 drivers/media/platform/visconti/Makefile > > create mode 100644 drivers/media/platform/visconti/hwd_viif.c > > create mode 100644 drivers/media/platform/visconti/hwd_viif.h > > create mode 100644 drivers/media/platform/visconti/hwd_viif_csi2rx.c > > create mode 100644 > > drivers/media/platform/visconti/hwd_viif_internal.h > > create mode 100644 drivers/media/platform/visconti/hwd_viif_l1isp.c > > create mode 100644 drivers/media/platform/visconti/hwd_viif_reg.h > > create mode 100644 drivers/media/platform/visconti/viif.c > > create mode 100644 drivers/media/platform/visconti/viif.h > > create mode 100644 include/uapi/linux/visconti_viif.h > >
This series is the Video Input Interface driver for Toshiba's ARM SoC, Visconti[0]. This provides DT binding documentation, device driver, MAINTAINER fiels. Best regards, Yuji [0]: https://toshiba.semicon-storage.com/ap-en/semiconductor/product/image-recognition-processors-visconti.html dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface bindings v1 -> v2: - No update media: platform: visconti: Add Toshiba Visconti Video Input Interface driver headers v1 -> v2: - moved driver headers to an individual patch media: platform: visconti: Add Toshiba Visconti Video Input Interface driver body v1 -> v2: - moved driver sources to an individual patch media: platform: visconti: Add Toshiba VIIF image signal processor driver v1 -> v2: - moved image signal processor driver to an individual patch MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface v1 -> v2: - No update Change in V2: - moved files into individual patches to decrease patch size Yuji Ishikawa (5): dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface bindings media: platform: visconti: Add Toshiba Visconti Video Input Interface driver headers media: platform: visconti: Add Toshiba Visconti Video Input Interface driver body media: platform: visconti: Add Toshiba VIIF image signal processor driver MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface .../bindings/media/toshiba,visconti-viif.yaml | 103 + MAINTAINERS | 2 + drivers/media/platform/Kconfig | 2 + drivers/media/platform/Makefile | 4 + drivers/media/platform/visconti/Kconfig | 9 + drivers/media/platform/visconti/Makefile | 9 + drivers/media/platform/visconti/hwd_viif.c | 2233 ++++++++++ drivers/media/platform/visconti/hwd_viif.h | 1776 ++++++++ .../media/platform/visconti/hwd_viif_csi2rx.c | 767 ++++ .../platform/visconti/hwd_viif_internal.h | 361 ++ .../media/platform/visconti/hwd_viif_l1isp.c | 3769 +++++++++++++++++ .../media/platform/visconti/hwd_viif_reg.h | 2802 ++++++++++++ drivers/media/platform/visconti/viif.c | 2384 +++++++++++ drivers/media/platform/visconti/viif.h | 134 + include/uapi/linux/visconti_viif.h | 1683 ++++++++ 15 files changed, 16038 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml create mode 100644 drivers/media/platform/visconti/Kconfig create mode 100644 drivers/media/platform/visconti/Makefile create mode 100644 drivers/media/platform/visconti/hwd_viif.c create mode 100644 drivers/media/platform/visconti/hwd_viif.h create mode 100644 drivers/media/platform/visconti/hwd_viif_csi2rx.c create mode 100644 drivers/media/platform/visconti/hwd_viif_internal.h create mode 100644 drivers/media/platform/visconti/hwd_viif_l1isp.c create mode 100644 drivers/media/platform/visconti/hwd_viif_reg.h create mode 100644 drivers/media/platform/visconti/viif.c create mode 100644 drivers/media/platform/visconti/viif.h create mode 100644 include/uapi/linux/visconti_viif.h