Message ID | 20230714015059.18775-1-yuji2.ishikawa@toshiba.co.jp |
---|---|
Headers | show |
Series | Add Toshiba Visconti Video Input Interface driver | expand |
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.
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 >
Hello Hans, > -----Original Message----- > From: Hans Verkuil <hverkuil@xs4all.nl> > Sent: Monday, August 21, 2023 9:59 PM > To: ishikawa yuji(石川 悠司 ○RDC□AITC○EA開) > <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(岩松 信洋 ○DITC□DIT○ > OST) <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 > >
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(石川 悠司 ○RDC□AITC○EA開) > <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(岩松 信洋 ○DITC□DIT○ > OST) <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