* [PATCH v6 0/4] media: Add ST VGXY61 camera sensor driver
@ 2022-09-27 8:36 Benjamin Mugnier
2022-09-27 8:36 ` [PATCH v6 1/4] media: v4l: Add 1X16 16-bit greyscale media bus code definition Benjamin Mugnier
` (3 more replies)
0 siblings, 4 replies; 26+ messages in thread
From: Benjamin Mugnier @ 2022-09-27 8:36 UTC (permalink / raw)
To: linux-media
Cc: alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson,
sakari.ailus, laurent.pinchart, kieran.bingham, nicolas,
hverkuil, Benjamin Mugnier
Hi,
This series adds a driver for the ST VGXY61 camera sensor. This camera sensor is
using the i2c bus for control and the csi-2 bus for data.
These commits introduce the MEDIA_BUS_FMT_Y16_1X16 as a media bus format (1/4),
a standard control in V4L2 for the hdr mode (2/4), dt bindings (3/4), and the
driver (4/4).
Tested on DragonBoard 410c, Raspberry Pi 4, and Qualcomm Robotics RB5.
Thanks,
Benjamin
v5->v6:
- Remove slave mode for now
- Comment mipi_margin
v4->v5:
- Fix kernel test robot errors
v3->v4:
- Standardize V4L2_CID_HDR_MODE
- Refactor to 80 characters limit
- Better types handling
- Various others
- Rebase on master
v2->v3:
- Fix errors in dt bindings
- Use alphabetical order where applicable
- Remove US_PER_MS macro
- Rename device specific macros
- Fix some control grabs not releasing
- Move user controls to public header and add documentation
- Rename set_framerate to set_framelength
- Rework i2c access
- Remove branchings that should never happen
- Rework pattern generator setting
- Update information on sensor rules of thumb
- Remove temperature control
- Remove suspend and resume
- Move mutex init to probe
- Move detect to probe
- Fix async_subdev_register order
- Support autosuspend
- Add init_cfg
- Fix controls not updating when sensor was off
- Various cleaning
- Rebase on master
v1->v2:
- Move temperature control to v4l2-controls.h as a standard control
- Reserve controls in v4l2-controls.h for HDR control
- Remove GPIO strobbing controls
- Remove references to unused controls in sensor struct
- Use v4l2_find_nearest_size
- Use V4L2_COLORSPACE_RAW instead of V4L2_COLORSPACE_SRGB
- Fill ycbcr_enc, quantization, and xfer_func in v4l2_mbus_framefmt
- Change gain from formula to a register RAW write
- Change temperature control value directly
- Remove clock-name and clock-lanes from device tree bindings
- Declare crop zones in mode_info structure
- Remove frame_interval functions and add hblank and vblank instead
- Update controls dependencies on change
- Fix first exposure rule of thumb being too restrictive
- Disable hflip and vflip controls while streaming
- Change RGB media bus code to Y as the sensor is monochrome
- Add Y16 format to v4l2 media bus formats
- Add get_selection API
- Fix timeout errors while disabling streaming on high framerates
- Support pm_runtime
- Rebase on master
Benjamin Mugnier (4):
media: v4l: Add 1X16 16-bit greyscale media bus code definition
media: v4l: ctrls: Add a control for HDR mode
media: dt-bindings: media: i2c: Add ST VGXY61 camera sensor binding
media: i2c: Add driver for ST VGXY61 camera sensor
.../bindings/media/i2c/st,st-vgxy61.yaml | 112 +
.../media/v4l/ext-ctrls-camera.rst | 8 +
.../media/v4l/subdev-formats.rst | 37 +
MAINTAINERS | 9 +
drivers/media/i2c/Kconfig | 10 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/st-vgxy61.c | 1969 +++++++++++++++++
drivers/media/v4l2-core/v4l2-ctrls-defs.c | 2 +
include/uapi/linux/media-bus-format.h | 3 +-
include/uapi/linux/v4l2-controls.h | 2 +
10 files changed, 2152 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml
create mode 100644 drivers/media/i2c/st-vgxy61.c
--
2.25.1
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH v6 1/4] media: v4l: Add 1X16 16-bit greyscale media bus code definition 2022-09-27 8:36 [PATCH v6 0/4] media: Add ST VGXY61 camera sensor driver Benjamin Mugnier @ 2022-09-27 8:36 ` Benjamin Mugnier 2022-10-06 21:26 ` Laurent Pinchart 2022-09-27 8:37 ` [PATCH v6 2/4] media: v4l: ctrls: Add a control for HDR mode Benjamin Mugnier ` (2 subsequent siblings) 3 siblings, 1 reply; 26+ messages in thread From: Benjamin Mugnier @ 2022-09-27 8:36 UTC (permalink / raw) To: linux-media Cc: alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, sakari.ailus, laurent.pinchart, kieran.bingham, nicolas, hverkuil, Benjamin Mugnier This extends the greyscale media bus family originally from MEDIA_BUS_FMT_Y8_1X8 up to MEDIA_BUS_FMT_Y14_1X14 by adding MEDIA_BUS_FMT_Y16_1X16, and behaves the same way with 16 bits. Add its documentation in subdev-formats.rst Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com> --- .../media/v4l/subdev-formats.rst | 37 +++++++++++++++++++ include/uapi/linux/media-bus-format.h | 3 +- 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst index d21d532eee15..19025e9ba447 100644 --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst @@ -7355,6 +7355,43 @@ the following codes. - v\ :sub:`2` - v\ :sub:`1` - v\ :sub:`0` + * .. _MEDIA-BUS-FMT-Y16-1X16: + + - MEDIA_BUS_FMT_Y16_1X16 + - 0x202e + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - y\ :sub:`15` + - y\ :sub:`14` + - y\ :sub:`13` + - y\ :sub:`12` + - y\ :sub:`11` + - y\ :sub:`10` + - y\ :sub:`9` + - y\ :sub:`8` + - y\ :sub:`7` + - y\ :sub:`6` + - y\ :sub:`5` + - y\ :sub:`4` + - y\ :sub:`3` + - y\ :sub:`2` + - y\ :sub:`1` + - y\ :sub:`0` .. raw:: latex diff --git a/include/uapi/linux/media-bus-format.h b/include/uapi/linux/media-bus-format.h index ec3323dbb927..b078dd125e57 100644 --- a/include/uapi/linux/media-bus-format.h +++ b/include/uapi/linux/media-bus-format.h @@ -69,7 +69,7 @@ #define MEDIA_BUS_FMT_RGB121212_1X36 0x1019 #define MEDIA_BUS_FMT_RGB161616_1X48 0x101a -/* YUV (including grey) - next is 0x202e */ +/* YUV (including grey) - next is 0x202f */ #define MEDIA_BUS_FMT_Y8_1X8 0x2001 #define MEDIA_BUS_FMT_UV8_1X8 0x2015 #define MEDIA_BUS_FMT_UYVY8_1_5X8 0x2002 @@ -115,6 +115,7 @@ #define MEDIA_BUS_FMT_YUV12_1X36 0x2029 #define MEDIA_BUS_FMT_YUV16_1X48 0x202a #define MEDIA_BUS_FMT_UYYVYY16_0_5X48 0x202b +#define MEDIA_BUS_FMT_Y16_1X16 0x202e /* Bayer - next is 0x3021 */ #define MEDIA_BUS_FMT_SBGGR8_1X8 0x3001 -- 2.25.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/4] media: v4l: Add 1X16 16-bit greyscale media bus code definition 2022-09-27 8:36 ` [PATCH v6 1/4] media: v4l: Add 1X16 16-bit greyscale media bus code definition Benjamin Mugnier @ 2022-10-06 21:26 ` Laurent Pinchart 2022-10-07 11:56 ` Benjamin MUGNIER 0 siblings, 1 reply; 26+ messages in thread From: Laurent Pinchart @ 2022-10-06 21:26 UTC (permalink / raw) To: Benjamin Mugnier Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, sakari.ailus, kieran.bingham, nicolas, hverkuil Hi Benjamin, Thank you for the patch. On Tue, Sep 27, 2022 at 10:36:59AM +0200, Benjamin Mugnier wrote: > This extends the greyscale media bus family originally from > MEDIA_BUS_FMT_Y8_1X8 up to MEDIA_BUS_FMT_Y14_1X14 by adding > MEDIA_BUS_FMT_Y16_1X16, and behaves the same way with 16 bits. > Add its documentation in subdev-formats.rst > > Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com> > --- > .../media/v4l/subdev-formats.rst | 37 +++++++++++++++++++ > include/uapi/linux/media-bus-format.h | 3 +- > 2 files changed, 39 insertions(+), 1 deletion(-) > > diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst > index d21d532eee15..19025e9ba447 100644 > --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst > +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst > @@ -7355,6 +7355,43 @@ the following codes. > - v\ :sub:`2` > - v\ :sub:`1` > - v\ :sub:`0` > + * .. _MEDIA-BUS-FMT-Y16-1X16: > + > + - MEDIA_BUS_FMT_Y16_1X16 > + - 0x202e > + - > + - > + - > + - > + - > + - > + - > + - > + - > + - > + - > + - > + - > + - > + - > + - > + - > + - y\ :sub:`15` > + - y\ :sub:`14` > + - y\ :sub:`13` > + - y\ :sub:`12` > + - y\ :sub:`11` > + - y\ :sub:`10` > + - y\ :sub:`9` > + - y\ :sub:`8` > + - y\ :sub:`7` > + - y\ :sub:`6` > + - y\ :sub:`5` > + - y\ :sub:`4` > + - y\ :sub:`3` > + - y\ :sub:`2` > + - y\ :sub:`1` > + - y\ :sub:`0` Could you please add this right after MEDIA_BUS_FMT_Y14_1X14 ? Same in include/uapi/linux/media-bus-format.h. With this change, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > .. raw:: latex > diff --git a/include/uapi/linux/media-bus-format.h b/include/uapi/linux/media-bus-format.h > index ec3323dbb927..b078dd125e57 100644 > --- a/include/uapi/linux/media-bus-format.h > +++ b/include/uapi/linux/media-bus-format.h > @@ -69,7 +69,7 @@ > #define MEDIA_BUS_FMT_RGB121212_1X36 0x1019 > #define MEDIA_BUS_FMT_RGB161616_1X48 0x101a > > -/* YUV (including grey) - next is 0x202e */ > +/* YUV (including grey) - next is 0x202f */ > #define MEDIA_BUS_FMT_Y8_1X8 0x2001 > #define MEDIA_BUS_FMT_UV8_1X8 0x2015 > #define MEDIA_BUS_FMT_UYVY8_1_5X8 0x2002 > @@ -115,6 +115,7 @@ > #define MEDIA_BUS_FMT_YUV12_1X36 0x2029 > #define MEDIA_BUS_FMT_YUV16_1X48 0x202a > #define MEDIA_BUS_FMT_UYYVYY16_0_5X48 0x202b > +#define MEDIA_BUS_FMT_Y16_1X16 0x202e > > /* Bayer - next is 0x3021 */ > #define MEDIA_BUS_FMT_SBGGR8_1X8 0x3001 -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/4] media: v4l: Add 1X16 16-bit greyscale media bus code definition 2022-10-06 21:26 ` Laurent Pinchart @ 2022-10-07 11:56 ` Benjamin MUGNIER 0 siblings, 0 replies; 26+ messages in thread From: Benjamin MUGNIER @ 2022-10-07 11:56 UTC (permalink / raw) To: Laurent Pinchart Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, sakari.ailus, kieran.bingham, nicolas, hverkuil Hi Laurent, Thank you for your review. On 10/6/22 23:26, Laurent Pinchart wrote: > Hi Benjamin, > > Thank you for the patch. > > On Tue, Sep 27, 2022 at 10:36:59AM +0200, Benjamin Mugnier wrote: >> This extends the greyscale media bus family originally from >> MEDIA_BUS_FMT_Y8_1X8 up to MEDIA_BUS_FMT_Y14_1X14 by adding >> MEDIA_BUS_FMT_Y16_1X16, and behaves the same way with 16 bits. >> Add its documentation in subdev-formats.rst >> >> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com> >> --- >> .../media/v4l/subdev-formats.rst | 37 +++++++++++++++++++ >> include/uapi/linux/media-bus-format.h | 3 +- >> 2 files changed, 39 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst >> index d21d532eee15..19025e9ba447 100644 >> --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst >> +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst >> @@ -7355,6 +7355,43 @@ the following codes. >> - v\ :sub:`2` >> - v\ :sub:`1` >> - v\ :sub:`0` >> + * .. _MEDIA-BUS-FMT-Y16-1X16: >> + >> + - MEDIA_BUS_FMT_Y16_1X16 >> + - 0x202e >> + - >> + - >> + - >> + - >> + - >> + - >> + - >> + - >> + - >> + - >> + - >> + - >> + - >> + - >> + - >> + - >> + - >> + - y\ :sub:`15` >> + - y\ :sub:`14` >> + - y\ :sub:`13` >> + - y\ :sub:`12` >> + - y\ :sub:`11` >> + - y\ :sub:`10` >> + - y\ :sub:`9` >> + - y\ :sub:`8` >> + - y\ :sub:`7` >> + - y\ :sub:`6` >> + - y\ :sub:`5` >> + - y\ :sub:`4` >> + - y\ :sub:`3` >> + - y\ :sub:`2` >> + - y\ :sub:`1` >> + - y\ :sub:`0` > > Could you please add this right after MEDIA_BUS_FMT_Y14_1X14 ? Same in include/uapi/linux/media-bus-format.h. With this change, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sure no problem. Regards, Benjamin >> >> >> .. raw:: latex >> diff --git a/include/uapi/linux/media-bus-format.h b/include/uapi/linux/media-bus-format.h >> index ec3323dbb927..b078dd125e57 100644 >> --- a/include/uapi/linux/media-bus-format.h >> +++ b/include/uapi/linux/media-bus-format.h >> @@ -69,7 +69,7 @@ >> #define MEDIA_BUS_FMT_RGB121212_1X36 0x1019 >> #define MEDIA_BUS_FMT_RGB161616_1X48 0x101a >> >> -/* YUV (including grey) - next is 0x202e */ >> +/* YUV (including grey) - next is 0x202f */ >> #define MEDIA_BUS_FMT_Y8_1X8 0x2001 >> #define MEDIA_BUS_FMT_UV8_1X8 0x2015 >> #define MEDIA_BUS_FMT_UYVY8_1_5X8 0x2002 >> @@ -115,6 +115,7 @@ >> #define MEDIA_BUS_FMT_YUV12_1X36 0x2029 >> #define MEDIA_BUS_FMT_YUV16_1X48 0x202a >> #define MEDIA_BUS_FMT_UYYVYY16_0_5X48 0x202b >> +#define MEDIA_BUS_FMT_Y16_1X16 0x202e >> >> /* Bayer - next is 0x3021 */ >> #define MEDIA_BUS_FMT_SBGGR8_1X8 0x3001 > ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v6 2/4] media: v4l: ctrls: Add a control for HDR mode 2022-09-27 8:36 [PATCH v6 0/4] media: Add ST VGXY61 camera sensor driver Benjamin Mugnier 2022-09-27 8:36 ` [PATCH v6 1/4] media: v4l: Add 1X16 16-bit greyscale media bus code definition Benjamin Mugnier @ 2022-09-27 8:37 ` Benjamin Mugnier 2022-09-27 8:37 ` [PATCH v6 3/4] media: dt-bindings: media: i2c: Add ST VGXY61 camera sensor binding Benjamin Mugnier 2022-09-27 8:37 ` [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor Benjamin Mugnier 3 siblings, 0 replies; 26+ messages in thread From: Benjamin Mugnier @ 2022-09-27 8:37 UTC (permalink / raw) To: linux-media Cc: alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, sakari.ailus, laurent.pinchart, kieran.bingham, nicolas, hverkuil, Benjamin Mugnier Add V4L2_CID_HDR_MODE as a menu item control to set the HDR mode of the sensor, and its documentation. Menu items are not standardized as they differ for each sensors. Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com> --- .../userspace-api/media/v4l/ext-ctrls-camera.rst | 8 ++++++++ drivers/media/v4l2-core/v4l2-ctrls-defs.c | 2 ++ include/uapi/linux/v4l2-controls.h | 2 ++ 3 files changed, 12 insertions(+) diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst index 4c5061aa9cd4..0ee09ff250e7 100644 --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst @@ -661,3 +661,11 @@ enum v4l2_scene_mode - .. [#f1] This control may be changed to a menu control in the future, if more options are required. + +``V4L2_CID_HDR_MODE (menu)`` + Change the sensor HDR mode. A HDR picture is obtained by merging two + captures of the same scene using two different exposure periods. HDR mode + describes the way these two captures are merged in the sensor. + + As modes differ for each sensor, menu items are not standardized by this + control and are left to the programmer. diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c index e22921e7ea61..0854de1ef1a5 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c @@ -1043,6 +1043,7 @@ const char *v4l2_ctrl_get_name(u32 id) case V4L2_CID_UNIT_CELL_SIZE: return "Unit Cell Size"; case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation"; case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation"; + case V4L2_CID_HDR_MODE: return "HDR mode"; /* FM Radio Modulator controls */ /* Keep the order of the 'case's the same as in v4l2-controls.h! */ @@ -1370,6 +1371,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, case V4L2_CID_STATELESS_H264_START_CODE: case V4L2_CID_CAMERA_ORIENTATION: case V4L2_CID_MPEG_VIDEO_INTRA_REFRESH_PERIOD_TYPE: + case V4L2_CID_HDR_MODE: *type = V4L2_CTRL_TYPE_MENU; break; case V4L2_CID_LINK_FREQ: diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h index b5e7d082b8ad..a27fdb93626e 100644 --- a/include/uapi/linux/v4l2-controls.h +++ b/include/uapi/linux/v4l2-controls.h @@ -1019,6 +1019,8 @@ enum v4l2_auto_focus_range { #define V4L2_CID_CAMERA_SENSOR_ROTATION (V4L2_CID_CAMERA_CLASS_BASE+35) +#define V4L2_CID_HDR_MODE (V4L2_CID_CAMERA_CLASS_BASE+36) + /* FM Modulator class control IDs */ #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900) -- 2.25.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v6 3/4] media: dt-bindings: media: i2c: Add ST VGXY61 camera sensor binding 2022-09-27 8:36 [PATCH v6 0/4] media: Add ST VGXY61 camera sensor driver Benjamin Mugnier 2022-09-27 8:36 ` [PATCH v6 1/4] media: v4l: Add 1X16 16-bit greyscale media bus code definition Benjamin Mugnier 2022-09-27 8:37 ` [PATCH v6 2/4] media: v4l: ctrls: Add a control for HDR mode Benjamin Mugnier @ 2022-09-27 8:37 ` Benjamin Mugnier 2022-10-06 19:15 ` Sakari Ailus 2022-09-27 8:37 ` [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor Benjamin Mugnier 3 siblings, 1 reply; 26+ messages in thread From: Benjamin Mugnier @ 2022-09-27 8:37 UTC (permalink / raw) To: linux-media Cc: alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, sakari.ailus, laurent.pinchart, kieran.bingham, nicolas, hverkuil, Benjamin Mugnier Add device tree binding for the ST VGXY61 camera sensor, and update MAINTAINERS file. Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com> --- .../bindings/media/i2c/st,st-vgxy61.yaml | 112 ++++++++++++++++++ MAINTAINERS | 9 ++ 2 files changed, 121 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml new file mode 100644 index 000000000000..652170367675 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml @@ -0,0 +1,112 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright (c) 2022 STMicroelectronics SA. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/st,st-vgxy61.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: STMicroelectronics VGxy61 HDR Global Shutter Sensor Family Device Tree Bindings + +maintainers: + - Benjamin Mugnier <benjamin.mugnier@foss.st.com> + - Sylvain Petinot <sylvain.petinot@foss.st.com> + +description: |- + STMicroelectronics VGxy61 family has a CSI-2 output port. CSI-2 output is a + quad lanes 800Mbps per lane. + Supported formats are RAW8, RAW10, RAW12, RAW14 and RAW16. + Following part number are supported + - VG5661 and VG6661 are 1.6 Mpx (1464 x 1104) monochrome and color sensors. + Maximum frame rate is 75 fps. + - VG5761 and VG6761 are 2.3 Mpx (1944 x 1204) monochrome and color sensors. + Maximum frame rate is 60 fps. + +properties: + compatible: + const: st,st-vgxy61 + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + VCORE-supply: + description: + Sensor digital core supply. Must be 1.2 volts. + + VDDIO-supply: + description: + Sensor digital IO supply. Must be 1.8 volts. + + VANA-supply: + description: + Sensor analog supply. Must be 2.8 volts. + + reset-gpios: + description: + Reference to the GPIO connected to the reset pin, if any. + This is an active low signal to the vgxy61. + + st,strobe-gpios-polarity: + description: + Invert polarity of strobe GPIOs. + type: boolean + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + additionalProperties: false + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + description: + CSI lanes to use + items: + - const: 1 + - const: 2 + - const: 3 + - const: 4 + + remote-endpoint: true + + required: + - data-lanes + +required: + - compatible + - clocks + - VCORE-supply + - VDDIO-supply + - VANA-supply + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + vgxy61: csi2tx@10 { + compatible = "st,st-vgxy61"; + reg = <0x10>; + clocks = <&clk_ext_camera>; + VCORE-supply = <&v1v2>; + VDDIO-supply = <&v1v8>; + VANA-supply = <&v2v8>; + reset-gpios = <&mfxgpio 18 GPIO_ACTIVE_LOW>; + port { + ep0: endpoint { + data-lanes = <1 2 3 4>; + remote-endpoint = <&mipi_csi2_out>; + }; + }; + }; + }; +... diff --git a/MAINTAINERS b/MAINTAINERS index a58f1fc6dd47..2e855bfdfeb7 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19278,6 +19278,15 @@ S: Maintained F: Documentation/hwmon/stpddc60.rst F: drivers/hwmon/pmbus/stpddc60.c +ST VGXY61 DRIVER +M: Benjamin Mugnier <benjamin.mugnier@foss.st.com> +M: Sylvain Petinot <sylvain.petinot@foss.st.com> +L: linux-media@vger.kernel.org +S: Maintained +T: git git://linuxtv.org/media_tree.git +F: Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml +F: drivers/media/i2c/st-vgxy61.c + ST VL53L0X ToF RANGER(I2C) IIO DRIVER M: Song Qiang <songqiang1304521@gmail.com> L: linux-iio@vger.kernel.org -- 2.25.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v6 3/4] media: dt-bindings: media: i2c: Add ST VGXY61 camera sensor binding 2022-09-27 8:37 ` [PATCH v6 3/4] media: dt-bindings: media: i2c: Add ST VGXY61 camera sensor binding Benjamin Mugnier @ 2022-10-06 19:15 ` Sakari Ailus 2022-10-07 11:57 ` Benjamin MUGNIER 0 siblings, 1 reply; 26+ messages in thread From: Sakari Ailus @ 2022-10-06 19:15 UTC (permalink / raw) To: Benjamin Mugnier Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Benjamin, On Tue, Sep 27, 2022 at 10:37:01AM +0200, Benjamin Mugnier wrote: > Add device tree binding for the ST VGXY61 camera sensor, and update > MAINTAINERS file. > > Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com> > --- > .../bindings/media/i2c/st,st-vgxy61.yaml | 112 ++++++++++++++++++ > MAINTAINERS | 9 ++ > 2 files changed, 121 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml > new file mode 100644 > index 000000000000..652170367675 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml > @@ -0,0 +1,112 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright (c) 2022 STMicroelectronics SA. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/st,st-vgxy61.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: STMicroelectronics VGxy61 HDR Global Shutter Sensor Family Device Tree Bindings > + > +maintainers: > + - Benjamin Mugnier <benjamin.mugnier@foss.st.com> > + - Sylvain Petinot <sylvain.petinot@foss.st.com> > + > +description: |- > + STMicroelectronics VGxy61 family has a CSI-2 output port. CSI-2 output is a > + quad lanes 800Mbps per lane. > + Supported formats are RAW8, RAW10, RAW12, RAW14 and RAW16. > + Following part number are supported > + - VG5661 and VG6661 are 1.6 Mpx (1464 x 1104) monochrome and color sensors. > + Maximum frame rate is 75 fps. > + - VG5761 and VG6761 are 2.3 Mpx (1944 x 1204) monochrome and color sensors. > + Maximum frame rate is 60 fps. > + > +properties: > + compatible: > + const: st,st-vgxy61 > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + VCORE-supply: > + description: > + Sensor digital core supply. Must be 1.2 volts. > + > + VDDIO-supply: > + description: > + Sensor digital IO supply. Must be 1.8 volts. > + > + VANA-supply: > + description: > + Sensor analog supply. Must be 2.8 volts. > + > + reset-gpios: > + description: > + Reference to the GPIO connected to the reset pin, if any. > + This is an active low signal to the vgxy61. > + > + st,strobe-gpios-polarity: > + description: > + Invert polarity of strobe GPIOs. > + type: boolean This was probably intended to be left out of this version? > + > + port: > + $ref: /schemas/graph.yaml#/$defs/port-base > + additionalProperties: false > + > + properties: > + endpoint: > + $ref: /schemas/media/video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + data-lanes: > + description: > + CSI lanes to use > + items: > + - const: 1 > + - const: 2 > + - const: 3 > + - const: 4 > + > + remote-endpoint: true > + > + required: > + - data-lanes > + > +required: > + - compatible > + - clocks > + - VCORE-supply > + - VDDIO-supply > + - VANA-supply > + - port > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + vgxy61: csi2tx@10 { > + compatible = "st,st-vgxy61"; > + reg = <0x10>; > + clocks = <&clk_ext_camera>; > + VCORE-supply = <&v1v2>; > + VDDIO-supply = <&v1v8>; > + VANA-supply = <&v2v8>; > + reset-gpios = <&mfxgpio 18 GPIO_ACTIVE_LOW>; > + port { > + ep0: endpoint { > + data-lanes = <1 2 3 4>; > + remote-endpoint = <&mipi_csi2_out>; > + }; > + }; > + }; > + }; > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index a58f1fc6dd47..2e855bfdfeb7 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -19278,6 +19278,15 @@ S: Maintained > F: Documentation/hwmon/stpddc60.rst > F: drivers/hwmon/pmbus/stpddc60.c > > +ST VGXY61 DRIVER > +M: Benjamin Mugnier <benjamin.mugnier@foss.st.com> > +M: Sylvain Petinot <sylvain.petinot@foss.st.com> > +L: linux-media@vger.kernel.org > +S: Maintained > +T: git git://linuxtv.org/media_tree.git > +F: Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml > +F: drivers/media/i2c/st-vgxy61.c > + > ST VL53L0X ToF RANGER(I2C) IIO DRIVER > M: Song Qiang <songqiang1304521@gmail.com> > L: linux-iio@vger.kernel.org -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 3/4] media: dt-bindings: media: i2c: Add ST VGXY61 camera sensor binding 2022-10-06 19:15 ` Sakari Ailus @ 2022-10-07 11:57 ` Benjamin MUGNIER 2022-10-07 12:28 ` Benjamin MUGNIER 0 siblings, 1 reply; 26+ messages in thread From: Benjamin MUGNIER @ 2022-10-07 11:57 UTC (permalink / raw) To: Sakari Ailus Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Sakari, Thank you for your review. On 10/6/22 21:15, Sakari Ailus wrote: > Hi Benjamin, > > On Tue, Sep 27, 2022 at 10:37:01AM +0200, Benjamin Mugnier wrote: >> Add device tree binding for the ST VGXY61 camera sensor, and update >> MAINTAINERS file. >> >> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com> >> --- >> .../bindings/media/i2c/st,st-vgxy61.yaml | 112 ++++++++++++++++++ >> MAINTAINERS | 9 ++ >> 2 files changed, 121 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml >> new file mode 100644 >> index 000000000000..652170367675 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml >> @@ -0,0 +1,112 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +# Copyright (c) 2022 STMicroelectronics SA. >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/i2c/st,st-vgxy61.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: STMicroelectronics VGxy61 HDR Global Shutter Sensor Family Device Tree Bindings >> + >> +maintainers: >> + - Benjamin Mugnier <benjamin.mugnier@foss.st.com> >> + - Sylvain Petinot <sylvain.petinot@foss.st.com> >> + >> +description: |- >> + STMicroelectronics VGxy61 family has a CSI-2 output port. CSI-2 output is a >> + quad lanes 800Mbps per lane. >> + Supported formats are RAW8, RAW10, RAW12, RAW14 and RAW16. >> + Following part number are supported >> + - VG5661 and VG6661 are 1.6 Mpx (1464 x 1104) monochrome and color sensors. >> + Maximum frame rate is 75 fps. >> + - VG5761 and VG6761 are 2.3 Mpx (1944 x 1204) monochrome and color sensors. >> + Maximum frame rate is 60 fps. >> + >> +properties: >> + compatible: >> + const: st,st-vgxy61 >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 1 >> + >> + VCORE-supply: >> + description: >> + Sensor digital core supply. Must be 1.2 volts. >> + >> + VDDIO-supply: >> + description: >> + Sensor digital IO supply. Must be 1.8 volts. >> + >> + VANA-supply: >> + description: >> + Sensor analog supply. Must be 2.8 volts. >> + >> + reset-gpios: >> + description: >> + Reference to the GPIO connected to the reset pin, if any. >> + This is an active low signal to the vgxy61. >> + >> + st,strobe-gpios-polarity: >> + description: >> + Invert polarity of strobe GPIOs. >> + type: boolean > > This was probably intended to be left out of this version? > Indeed, I forgot to update this file. Thanks a lot for double checking. Regards, Benjamin >> + >> + port: >> + $ref: /schemas/graph.yaml#/$defs/port-base >> + additionalProperties: false >> + >> + properties: >> + endpoint: >> + $ref: /schemas/media/video-interfaces.yaml# >> + unevaluatedProperties: false >> + >> + properties: >> + data-lanes: >> + description: >> + CSI lanes to use >> + items: >> + - const: 1 >> + - const: 2 >> + - const: 3 >> + - const: 4 >> + >> + remote-endpoint: true >> + >> + required: >> + - data-lanes >> + >> +required: >> + - compatible >> + - clocks >> + - VCORE-supply >> + - VDDIO-supply >> + - VANA-supply >> + - port >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/gpio/gpio.h> >> + i2c { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + vgxy61: csi2tx@10 { >> + compatible = "st,st-vgxy61"; >> + reg = <0x10>; >> + clocks = <&clk_ext_camera>; >> + VCORE-supply = <&v1v2>; >> + VDDIO-supply = <&v1v8>; >> + VANA-supply = <&v2v8>; >> + reset-gpios = <&mfxgpio 18 GPIO_ACTIVE_LOW>; >> + port { >> + ep0: endpoint { >> + data-lanes = <1 2 3 4>; >> + remote-endpoint = <&mipi_csi2_out>; >> + }; >> + }; >> + }; >> + }; >> +... >> diff --git a/MAINTAINERS b/MAINTAINERS >> index a58f1fc6dd47..2e855bfdfeb7 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -19278,6 +19278,15 @@ S: Maintained >> F: Documentation/hwmon/stpddc60.rst >> F: drivers/hwmon/pmbus/stpddc60.c >> >> +ST VGXY61 DRIVER >> +M: Benjamin Mugnier <benjamin.mugnier@foss.st.com> >> +M: Sylvain Petinot <sylvain.petinot@foss.st.com> >> +L: linux-media@vger.kernel.org >> +S: Maintained >> +T: git git://linuxtv.org/media_tree.git >> +F: Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml >> +F: drivers/media/i2c/st-vgxy61.c >> + >> ST VL53L0X ToF RANGER(I2C) IIO DRIVER >> M: Song Qiang <songqiang1304521@gmail.com> >> L: linux-iio@vger.kernel.org > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 3/4] media: dt-bindings: media: i2c: Add ST VGXY61 camera sensor binding 2022-10-07 11:57 ` Benjamin MUGNIER @ 2022-10-07 12:28 ` Benjamin MUGNIER 0 siblings, 0 replies; 26+ messages in thread From: Benjamin MUGNIER @ 2022-10-07 12:28 UTC (permalink / raw) To: Sakari Ailus Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Actually please ignore my previous answer. I just answered in the driver thread this is for gpio strobing and not sensor synchronization. Sorry, even I got confused. I will rename it as this is, indeed, very misleading ;) On 10/7/22 13:57, Benjamin MUGNIER wrote: > Hi Sakari, > > Thank you for your review. > > On 10/6/22 21:15, Sakari Ailus wrote: >> Hi Benjamin, >> >> On Tue, Sep 27, 2022 at 10:37:01AM +0200, Benjamin Mugnier wrote: >>> Add device tree binding for the ST VGXY61 camera sensor, and update >>> MAINTAINERS file. >>> >>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com> >>> --- >>> .../bindings/media/i2c/st,st-vgxy61.yaml | 112 ++++++++++++++++++ >>> MAINTAINERS | 9 ++ >>> 2 files changed, 121 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml >>> new file mode 100644 >>> index 000000000000..652170367675 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml >>> @@ -0,0 +1,112 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +# Copyright (c) 2022 STMicroelectronics SA. >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/media/i2c/st,st-vgxy61.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: STMicroelectronics VGxy61 HDR Global Shutter Sensor Family Device Tree Bindings >>> + >>> +maintainers: >>> + - Benjamin Mugnier <benjamin.mugnier@foss.st.com> >>> + - Sylvain Petinot <sylvain.petinot@foss.st.com> >>> + >>> +description: |- >>> + STMicroelectronics VGxy61 family has a CSI-2 output port. CSI-2 output is a >>> + quad lanes 800Mbps per lane. >>> + Supported formats are RAW8, RAW10, RAW12, RAW14 and RAW16. >>> + Following part number are supported >>> + - VG5661 and VG6661 are 1.6 Mpx (1464 x 1104) monochrome and color sensors. >>> + Maximum frame rate is 75 fps. >>> + - VG5761 and VG6761 are 2.3 Mpx (1944 x 1204) monochrome and color sensors. >>> + Maximum frame rate is 60 fps. >>> + >>> +properties: >>> + compatible: >>> + const: st,st-vgxy61 >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + clocks: >>> + maxItems: 1 >>> + >>> + VCORE-supply: >>> + description: >>> + Sensor digital core supply. Must be 1.2 volts. >>> + >>> + VDDIO-supply: >>> + description: >>> + Sensor digital IO supply. Must be 1.8 volts. >>> + >>> + VANA-supply: >>> + description: >>> + Sensor analog supply. Must be 2.8 volts. >>> + >>> + reset-gpios: >>> + description: >>> + Reference to the GPIO connected to the reset pin, if any. >>> + This is an active low signal to the vgxy61. >>> + >>> + st,strobe-gpios-polarity: >>> + description: >>> + Invert polarity of strobe GPIOs. >>> + type: boolean >> >> This was probably intended to be left out of this version? >> > > Indeed, I forgot to update this file. Thanks a lot for double checking. > > > Regards, > > Benjamin > >>> + >>> + port: >>> + $ref: /schemas/graph.yaml#/$defs/port-base >>> + additionalProperties: false >>> + >>> + properties: >>> + endpoint: >>> + $ref: /schemas/media/video-interfaces.yaml# >>> + unevaluatedProperties: false >>> + >>> + properties: >>> + data-lanes: >>> + description: >>> + CSI lanes to use >>> + items: >>> + - const: 1 >>> + - const: 2 >>> + - const: 3 >>> + - const: 4 >>> + >>> + remote-endpoint: true >>> + >>> + required: >>> + - data-lanes >>> + >>> +required: >>> + - compatible >>> + - clocks >>> + - VCORE-supply >>> + - VDDIO-supply >>> + - VANA-supply >>> + - port >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/gpio/gpio.h> >>> + i2c { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + vgxy61: csi2tx@10 { >>> + compatible = "st,st-vgxy61"; >>> + reg = <0x10>; >>> + clocks = <&clk_ext_camera>; >>> + VCORE-supply = <&v1v2>; >>> + VDDIO-supply = <&v1v8>; >>> + VANA-supply = <&v2v8>; >>> + reset-gpios = <&mfxgpio 18 GPIO_ACTIVE_LOW>; >>> + port { >>> + ep0: endpoint { >>> + data-lanes = <1 2 3 4>; >>> + remote-endpoint = <&mipi_csi2_out>; >>> + }; >>> + }; >>> + }; >>> + }; >>> +... >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index a58f1fc6dd47..2e855bfdfeb7 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -19278,6 +19278,15 @@ S: Maintained >>> F: Documentation/hwmon/stpddc60.rst >>> F: drivers/hwmon/pmbus/stpddc60.c >>> >>> +ST VGXY61 DRIVER >>> +M: Benjamin Mugnier <benjamin.mugnier@foss.st.com> >>> +M: Sylvain Petinot <sylvain.petinot@foss.st.com> >>> +L: linux-media@vger.kernel.org >>> +S: Maintained >>> +T: git git://linuxtv.org/media_tree.git >>> +F: Documentation/devicetree/bindings/media/i2c/st,st-vgxy61.yaml >>> +F: drivers/media/i2c/st-vgxy61.c >>> + >>> ST VL53L0X ToF RANGER(I2C) IIO DRIVER >>> M: Song Qiang <songqiang1304521@gmail.com> >>> L: linux-iio@vger.kernel.org >> ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-09-27 8:36 [PATCH v6 0/4] media: Add ST VGXY61 camera sensor driver Benjamin Mugnier ` (2 preceding siblings ...) 2022-09-27 8:37 ` [PATCH v6 3/4] media: dt-bindings: media: i2c: Add ST VGXY61 camera sensor binding Benjamin Mugnier @ 2022-09-27 8:37 ` Benjamin Mugnier 2022-10-06 19:14 ` Sakari Ailus 3 siblings, 1 reply; 26+ messages in thread From: Benjamin Mugnier @ 2022-09-27 8:37 UTC (permalink / raw) To: linux-media Cc: alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, sakari.ailus, laurent.pinchart, kieran.bingham, nicolas, hverkuil, Benjamin Mugnier The VGXY61 has a quad lanes CSI-2 output port running at 800mbps per lane, and supports RAW8, RAW10, RAW12, RAW14 and RAW16 formats. The driver handles both sensor types: - VG5661 and VG6661: 1.6 Mpx (1464 x 1104) 75fps. - VG5761 and VG6761: 2.3 Mpx (1944 x 1204) 60 fps. The driver supports: - HDR linearize mode, HDR substraction mode, and no HDR - GPIOs LEDs strobing - Digital binning and analog subsampling - Horizontal and vertical flip - Manual exposure - Analog and digital gains - Test patterns Signed-off-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com> --- drivers/media/i2c/Kconfig | 10 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/st-vgxy61.c | 1969 +++++++++++++++++++++++++++++++++ 3 files changed, 1980 insertions(+) create mode 100644 drivers/media/i2c/st-vgxy61.c diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 7806d4b81716..065c05f6adbf 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -769,6 +769,16 @@ config VIDEO_SR030PC30 help This driver supports SR030PC30 VGA camera from Siliconfile +config VIDEO_ST_VGXY61 + tristate "ST VGXY61 sensor support" + depends on OF && GPIOLIB && VIDEO_DEV && I2C + select MEDIA_CONTROLLER + select VIDEO_V4L2_SUBDEV_API + select V4L2_FWNODE + help + This is a Video4Linux2 sensor driver for the ST VGXY61 + camera sensor. + config VIDEO_VS6624 tristate "ST VS6624 sensor support" depends on VIDEO_DEV && I2C diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 0a2933103dd9..891c6268f59f 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -117,6 +117,7 @@ obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o +obj-$(CONFIG_VIDEO_ST_VGXY61) += st-vgxy61.o obj-$(CONFIG_VIDEO_TC358743) += tc358743.o obj-$(CONFIG_VIDEO_TDA1997X) += tda1997x.o obj-$(CONFIG_VIDEO_TDA7432) += tda7432.o diff --git a/drivers/media/i2c/st-vgxy61.c b/drivers/media/i2c/st-vgxy61.c new file mode 100644 index 000000000000..0c4fba7103da --- /dev/null +++ b/drivers/media/i2c/st-vgxy61.c @@ -0,0 +1,1969 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Driver for VGXY61 global shutter sensor family driver + * + * Copyright (C) 2022 STMicroelectronics SA + */ + +#include <asm-generic/unaligned.h> +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/iopoll.h> +#include <linux/module.h> +#include <linux/pm_runtime.h> +#include <linux/regulator/consumer.h> +#include <linux/units.h> +#include <media/mipi-csi2.h> +#include <media/v4l2-async.h> +#include <media/v4l2-ctrls.h> +#include <media/v4l2-device.h> +#include <media/v4l2-fwnode.h> +#include <media/v4l2-subdev.h> + +#define VGXY61_REG_8BIT(n) ((1 << 16) | (n)) +#define VGXY61_REG_16BIT(n) ((2 << 16) | (n)) +#define VGXY61_REG_32BIT(n) ((4 << 16) | (n)) +#define VGXY61_REG_SIZE_SHIFT 16 +#define VGXY61_REG_ADDR_MASK 0xffff + +#define VGXY61_REG_MODEL_ID VGXY61_REG_16BIT(0x0000) +#define VG5661_MODEL_ID 0x5661 +#define VG5761_MODEL_ID 0x5761 +#define VGXY61_REG_REVISION VGXY61_REG_16BIT(0x0002) +#define VGXY61_REG_FWPATCH_REVISION VGXY61_REG_16BIT(0x0014) +#define VGXY61_REG_FWPATCH_START_ADDR VGXY61_REG_8BIT(0x2000) +#define VGXY61_REG_SYSTEM_FSM VGXY61_REG_8BIT(0x0020) +#define VGXY61_SYSTEM_FSM_SW_STBY 0x03 +#define VGXY61_SYSTEM_FSM_STREAMING 0x04 +#define VGXY61_REG_NVM VGXY61_REG_8BIT(0x0023) +#define VGXY61_NVM_OK 0x04 +#define VGXY61_REG_STBY VGXY61_REG_8BIT(0x0201) +#define VGXY61_STBY_NO_REQ 0 +#define VGXY61_STBY_REQ_TMP_READ BIT(2) +#define VGXY61_REG_STREAMING VGXY61_REG_8BIT(0x0202) +#define VGXY61_STREAMING_NO_REQ 0 +#define VGXY61_STREAMING_REQ_STOP BIT(0) +#define VGXY61_STREAMING_REQ_START BIT(1) +#define VGXY61_REG_EXT_CLOCK VGXY61_REG_32BIT(0x0220) +#define VGXY61_REG_CLK_PLL_PREDIV VGXY61_REG_8BIT(0x0224) +#define VGXY61_REG_CLK_SYS_PLL_MULT VGXY61_REG_8BIT(0x0225) +#define VGXY61_REG_GPIO_0_CTRL VGXY61_REG_8BIT(0x0236) +#define VGXY61_REG_GPIO_1_CTRL VGXY61_REG_8BIT(0x0237) +#define VGXY61_REG_GPIO_2_CTRL VGXY61_REG_8BIT(0x0238) +#define VGXY61_REG_GPIO_3_CTRL VGXY61_REG_8BIT(0x0239) +#define VGXY61_REG_SIGNALS_POLARITY_CTRL VGXY61_REG_8BIT(0x023b) +#define VGXY61_REG_LINE_LENGTH VGXY61_REG_16BIT(0x0300) +#define VGXY61_REG_ORIENTATION VGXY61_REG_8BIT(0x0302) +#define VGXY61_REG_VT_CTRL VGXY61_REG_8BIT(0x0304) +#define VGXY61_REG_FORMAT_CTRL VGXY61_REG_8BIT(0x0305) +#define VGXY61_REG_OIF_CTRL VGXY61_REG_16BIT(0x0306) +#define VGXY61_REG_OIF_ROI0_CTRL VGXY61_REG_8BIT(0x030a) +#define VGXY61_REG_ROI0_START_H VGXY61_REG_16BIT(0x0400) +#define VGXY61_REG_ROI0_START_V VGXY61_REG_16BIT(0x0402) +#define VGXY61_REG_ROI0_END_H VGXY61_REG_16BIT(0x0404) +#define VGXY61_REG_ROI0_END_V VGXY61_REG_16BIT(0x0406) +#define VGXY61_REG_PATGEN_CTRL VGXY61_REG_32BIT(0x0440) +#define VGXY61_PATGEN_LONG_ENABLE BIT(16) +#define VGXY61_PATGEN_SHORT_ENABLE BIT(0) +#define VGXY61_PATGEN_LONG_TYPE_SHIFT 18 +#define VGXY61_PATGEN_SHORT_TYPE_SHIFT 4 +#define VGXY61_REG_FRAME_CONTENT_CTRL VGXY61_REG_8BIT(0x0478) +#define VGXY61_REG_COARSE_EXPOSURE_LONG VGXY61_REG_16BIT(0x0500) +#define VGXY61_REG_COARSE_EXPOSURE_SHORT VGXY61_REG_16BIT(0x0504) +#define VGXY61_REG_ANALOG_GAIN VGXY61_REG_8BIT(0x0508) +#define VGXY61_REG_DIGITAL_GAIN_LONG VGXY61_REG_16BIT(0x050a) +#define VGXY61_REG_DIGITAL_GAIN_SHORT VGXY61_REG_16BIT(0x0512) +#define VGXY61_REG_FRAME_LENGTH VGXY61_REG_16BIT(0x051a) +#define VGXY61_REG_SIGNALS_CTRL VGXY61_REG_16BIT(0x0522) +#define VGXY61_SIGNALS_GPIO_ID_SHIFT 4 +#define VGXY61_REG_READOUT_CTRL VGXY61_REG_8BIT(0x0530) +#define VGXY61_REG_HDR_CTRL VGXY61_REG_8BIT(0x0532) +#define VGXY61_REG_PATGEN_LONG_DATA_GR VGXY61_REG_16BIT(0x092c) +#define VGXY61_REG_PATGEN_LONG_DATA_R VGXY61_REG_16BIT(0x092e) +#define VGXY61_REG_PATGEN_LONG_DATA_B VGXY61_REG_16BIT(0x0930) +#define VGXY61_REG_PATGEN_LONG_DATA_GB VGXY61_REG_16BIT(0x0932) +#define VGXY61_REG_PATGEN_SHORT_DATA_GR VGXY61_REG_16BIT(0x0950) +#define VGXY61_REG_PATGEN_SHORT_DATA_R VGXY61_REG_16BIT(0x0952) +#define VGXY61_REG_PATGEN_SHORT_DATA_B VGXY61_REG_16BIT(0x0954) +#define VGXY61_REG_PATGEN_SHORT_DATA_GB VGXY61_REG_16BIT(0x0956) +#define VGXY61_REG_BYPASS_CTRL VGXY61_REG_8BIT(0x0a60) + +#define VGX661_WIDTH 1464 +#define VGX661_HEIGHT 1104 +#define VGX761_WIDTH 1944 +#define VGX761_HEIGHT 1204 +#define VGX661_DEFAULT_MODE 1 +#define VGX761_DEFAULT_MODE 1 +#define VGX661_SHORT_ROT_TERM 93 +#define VGX761_SHORT_ROT_TERM 90 +#define VGXY61_EXPOS_ROT_TERM 66 +#define VGXY61_WRITE_MULTIPLE_CHUNK_MAX 16 +#define VGXY61_NB_GPIOS 4 +#define VGXY61_NB_POLARITIES 5 +#define VGXY61_FRAME_LENGTH_DEF 1313 +#define VGXY61_MIN_FRAME_LENGTH 1288 +#define VGXY61_MIN_EXPOSURE 10 +#define VGXY61_HDR_LINEAR_RATIO 10 +#define VGXY61_TIMEOUT_MS 500 +#define VGXY61_MEDIA_BUS_FMT_DEF MEDIA_BUS_FMT_Y8_1X8 + +#define VGXY61_FWPATCH_REVISION_MAJOR 2 +#define VGXY61_FWPATCH_REVISION_MINOR 0 +#define VGXY61_FWPATCH_REVISION_MICRO 5 + +static const u8 patch_array[] = { + 0xbf, 0x00, 0x05, 0x20, 0x06, 0x01, 0xe0, 0xe0, 0x04, 0x80, 0xe6, 0x45, + 0xed, 0x6f, 0xfe, 0xff, 0x14, 0x80, 0x1f, 0x84, 0x10, 0x42, 0x05, 0x7c, + 0x01, 0xc4, 0x1e, 0x80, 0xb6, 0x42, 0x00, 0xe0, 0x1e, 0x82, 0x1e, 0xc0, + 0x93, 0xdd, 0xc3, 0xc1, 0x0c, 0x04, 0x00, 0xfa, 0x86, 0x0d, 0x70, 0xe1, + 0x04, 0x98, 0x15, 0x00, 0x28, 0xe0, 0x14, 0x02, 0x08, 0xfc, 0x15, 0x40, + 0x28, 0xe0, 0x98, 0x58, 0xe0, 0xef, 0x04, 0x98, 0x0e, 0x04, 0x00, 0xf0, + 0x15, 0x00, 0x28, 0xe0, 0x19, 0xc8, 0x15, 0x40, 0x28, 0xe0, 0xc6, 0x41, + 0xfc, 0xe0, 0x14, 0x80, 0x1f, 0x84, 0x14, 0x02, 0xa0, 0xfc, 0x1e, 0x80, + 0x14, 0x80, 0x14, 0x02, 0x80, 0xfb, 0x14, 0x02, 0xe0, 0xfc, 0x1e, 0x80, + 0x14, 0xc0, 0x1f, 0x84, 0x14, 0x02, 0xa4, 0xfc, 0x1e, 0xc0, 0x14, 0xc0, + 0x14, 0x02, 0x80, 0xfb, 0x14, 0x02, 0xe4, 0xfc, 0x1e, 0xc0, 0x0c, 0x0c, + 0x00, 0xf2, 0x93, 0xdd, 0x86, 0x00, 0xf8, 0xe0, 0x04, 0x80, 0xc6, 0x03, + 0x70, 0xe1, 0x0e, 0x84, 0x93, 0xdd, 0xc3, 0xc1, 0x0c, 0x04, 0x00, 0xfa, + 0x6b, 0x80, 0x06, 0x40, 0x6c, 0xe1, 0x04, 0x80, 0x09, 0x00, 0xe0, 0xe0, + 0x0b, 0xa1, 0x95, 0x84, 0x05, 0x0c, 0x1c, 0xe0, 0x86, 0x02, 0xf9, 0x60, + 0xe0, 0xcf, 0x78, 0x6e, 0x80, 0xef, 0x25, 0x0c, 0x18, 0xe0, 0x05, 0x4c, + 0x1c, 0xe0, 0x86, 0x02, 0xf9, 0x60, 0xe0, 0xcf, 0x0b, 0x84, 0xd8, 0x6d, + 0x80, 0xef, 0x05, 0x4c, 0x18, 0xe0, 0x04, 0xd8, 0x0b, 0xa5, 0x95, 0x84, + 0x05, 0x0c, 0x2c, 0xe0, 0x06, 0x02, 0x01, 0x60, 0xe0, 0xce, 0x18, 0x6d, + 0x80, 0xef, 0x25, 0x0c, 0x30, 0xe0, 0x05, 0x4c, 0x2c, 0xe0, 0x06, 0x02, + 0x01, 0x60, 0xe0, 0xce, 0x0b, 0x84, 0x78, 0x6c, 0x80, 0xef, 0x05, 0x4c, + 0x30, 0xe0, 0x0c, 0x0c, 0x00, 0xf2, 0x93, 0xdd, 0x46, 0x01, 0x70, 0xe1, + 0x08, 0x80, 0x0b, 0xa1, 0x08, 0x5c, 0x00, 0xda, 0x06, 0x01, 0x68, 0xe1, + 0x04, 0x80, 0x4a, 0x40, 0x84, 0xe0, 0x08, 0x5c, 0x00, 0x9a, 0x06, 0x01, + 0xe0, 0xe0, 0x04, 0x80, 0x15, 0x00, 0x60, 0xe0, 0x19, 0xc4, 0x15, 0x40, + 0x60, 0xe0, 0x15, 0x00, 0x78, 0xe0, 0x19, 0xc4, 0x15, 0x40, 0x78, 0xe0, + 0x93, 0xdd, 0xc3, 0xc1, 0x46, 0x01, 0x70, 0xe1, 0x08, 0x80, 0x0b, 0xa1, + 0x08, 0x5c, 0x00, 0xda, 0x06, 0x01, 0x68, 0xe1, 0x04, 0x80, 0x4a, 0x40, + 0x84, 0xe0, 0x08, 0x5c, 0x00, 0x9a, 0x06, 0x01, 0xe0, 0xe0, 0x14, 0x80, + 0x25, 0x02, 0x54, 0xe0, 0x29, 0xc4, 0x25, 0x42, 0x54, 0xe0, 0x24, 0x80, + 0x35, 0x04, 0x6c, 0xe0, 0x39, 0xc4, 0x35, 0x44, 0x6c, 0xe0, 0x25, 0x02, + 0x64, 0xe0, 0x29, 0xc4, 0x25, 0x42, 0x64, 0xe0, 0x04, 0x80, 0x15, 0x00, + 0x7c, 0xe0, 0x19, 0xc4, 0x15, 0x40, 0x7c, 0xe0, 0x93, 0xdd, 0xc3, 0xc1, + 0x4c, 0x04, 0x7c, 0xfa, 0x86, 0x40, 0x98, 0xe0, 0x14, 0x80, 0x1b, 0xa1, + 0x06, 0x00, 0x00, 0xc0, 0x08, 0x42, 0x38, 0xdc, 0x08, 0x64, 0xa0, 0xef, + 0x86, 0x42, 0x3c, 0xe0, 0x68, 0x49, 0x80, 0xef, 0x6b, 0x80, 0x78, 0x53, + 0xc8, 0xef, 0xc6, 0x54, 0x6c, 0xe1, 0x7b, 0x80, 0xb5, 0x14, 0x0c, 0xf8, + 0x05, 0x14, 0x14, 0xf8, 0x1a, 0xac, 0x8a, 0x80, 0x0b, 0x90, 0x38, 0x55, + 0x80, 0xef, 0x1a, 0xae, 0x17, 0xc2, 0x03, 0x82, 0x88, 0x65, 0x80, 0xef, + 0x1b, 0x80, 0x0b, 0x8e, 0x68, 0x65, 0x80, 0xef, 0x9b, 0x80, 0x0b, 0x8c, + 0x08, 0x65, 0x80, 0xef, 0x6b, 0x80, 0x0b, 0x92, 0x1b, 0x8c, 0x98, 0x64, + 0x80, 0xef, 0x1a, 0xec, 0x9b, 0x80, 0x0b, 0x90, 0x95, 0x54, 0x10, 0xe0, + 0xa8, 0x53, 0x80, 0xef, 0x1a, 0xee, 0x17, 0xc2, 0x03, 0x82, 0xf8, 0x63, + 0x80, 0xef, 0x1b, 0x80, 0x0b, 0x8e, 0xd8, 0x63, 0x80, 0xef, 0x1b, 0x8c, + 0x68, 0x63, 0x80, 0xef, 0x6b, 0x80, 0x0b, 0x92, 0x65, 0x54, 0x14, 0xe0, + 0x08, 0x65, 0x84, 0xef, 0x68, 0x63, 0x80, 0xef, 0x7b, 0x80, 0x0b, 0x8c, + 0xa8, 0x64, 0x84, 0xef, 0x08, 0x63, 0x80, 0xef, 0x14, 0xe8, 0x46, 0x44, + 0x94, 0xe1, 0x24, 0x88, 0x4a, 0x4e, 0x04, 0xe0, 0x14, 0xea, 0x1a, 0x04, + 0x08, 0xe0, 0x0a, 0x40, 0x84, 0xed, 0x0c, 0x04, 0x00, 0xe2, 0x4a, 0x40, + 0x04, 0xe0, 0x19, 0x16, 0xc0, 0xe0, 0x0a, 0x40, 0x84, 0xed, 0x21, 0x54, + 0x60, 0xe0, 0x0c, 0x04, 0x00, 0xe2, 0x1b, 0xa5, 0x0e, 0xea, 0x01, 0x89, + 0x21, 0x54, 0x64, 0xe0, 0x7e, 0xe8, 0x65, 0x82, 0x1b, 0xa7, 0x26, 0x00, + 0x00, 0x80, 0xa5, 0x82, 0x1b, 0xa9, 0x65, 0x82, 0x1b, 0xa3, 0x01, 0x85, + 0x16, 0x00, 0x00, 0xc0, 0x01, 0x54, 0x04, 0xf8, 0x06, 0xaa, 0x01, 0x83, + 0x06, 0xa8, 0x65, 0x81, 0x06, 0xa8, 0x01, 0x54, 0x04, 0xf8, 0x01, 0x83, + 0x06, 0xaa, 0x09, 0x14, 0x18, 0xf8, 0x0b, 0xa1, 0x05, 0x84, 0xc6, 0x42, + 0xd4, 0xe0, 0x14, 0x84, 0x01, 0x83, 0x01, 0x54, 0x60, 0xe0, 0x01, 0x54, + 0x64, 0xe0, 0x0b, 0x02, 0x90, 0xe0, 0x10, 0x02, 0x90, 0xe5, 0x01, 0x54, + 0x88, 0xe0, 0xb5, 0x81, 0xc6, 0x40, 0xd4, 0xe0, 0x14, 0x80, 0x0b, 0x02, + 0xe0, 0xe4, 0x10, 0x02, 0x31, 0x66, 0x02, 0xc0, 0x01, 0x54, 0x88, 0xe0, + 0x1a, 0x84, 0x29, 0x14, 0x10, 0xe0, 0x1c, 0xaa, 0x2b, 0xa1, 0xf5, 0x82, + 0x25, 0x14, 0x10, 0xf8, 0x2b, 0x04, 0xa8, 0xe0, 0x20, 0x44, 0x0d, 0x70, + 0x03, 0xc0, 0x2b, 0xa1, 0x04, 0x00, 0x80, 0x9a, 0x02, 0x40, 0x84, 0x90, + 0x03, 0x54, 0x04, 0x80, 0x4c, 0x0c, 0x7c, 0xf2, 0x93, 0xdd, 0x00, 0x00, + 0x02, 0xa9, 0x00, 0x00, 0x64, 0x4a, 0x40, 0x00, 0x08, 0x2d, 0x58, 0xe0, + 0xa8, 0x98, 0x40, 0x00, 0x28, 0x07, 0x34, 0xe0, 0x05, 0xb9, 0x00, 0x00, + 0x28, 0x00, 0x41, 0x05, 0x88, 0x00, 0x41, 0x3c, 0x98, 0x00, 0x41, 0x52, + 0x04, 0x01, 0x41, 0x79, 0x3c, 0x01, 0x41, 0x6a, 0x3d, 0xfe, 0x00, 0x00, +}; + +static const char * const vgxy61_test_pattern_menu[] = { + "Disabled", + "Solid", + "Colorbar", + "Gradbar", + "Hgrey", + "Vgrey", + "Dgrey", + "PN28", +}; + +static const char * const vgxy61_hdr_mode_menu[] = { + "HDR linearize", + "HDR substraction", + "No HDR", +}; + +static const char * const vgxy61_supply_name[] = { + "VCORE", + "VDDIO", + "VANA", +}; + +#define VGXY61_NUM_SUPPLIES ARRAY_SIZE(vgxy61_supply_name) + +static const s64 link_freq[] = { + /* + * MIPI output freq is 804Mhz / 2, as it uses both rising edge and + * falling edges to send data + */ + 402000000ULL +}; + +enum vgxy61_bin_mode { + VGXY61_BIN_MODE_NORMAL, + VGXY61_BIN_MODE_DIGITAL_X2, + VGXY61_BIN_MODE_DIGITAL_X4, +}; + +enum vgxy61_hdr_mode { + VGXY61_HDR_LINEAR, + VGXY61_HDR_SUB, + VGXY61_NO_HDR, +}; + +enum vgxy61_strobe_mode { + VGXY61_STROBE_DISABLED, + VGXY61_STROBE_LONG, + VGXY61_STROBE_ENABLED, +}; + +struct vgxy61_mode_info { + u32 width; + u32 height; + enum vgxy61_bin_mode bin_mode; + struct v4l2_rect crop; +}; + +struct vgxy61_fmt_desc { + u32 code; + u8 bpp; + u8 data_type; +}; + +static const struct vgxy61_fmt_desc vgxy61_supported_codes[] = { + { + .code = MEDIA_BUS_FMT_Y8_1X8, + .bpp = 8, + .data_type = MIPI_CSI2_DT_RAW8, + }, + { + .code = MEDIA_BUS_FMT_Y10_1X10, + .bpp = 10, + .data_type = MIPI_CSI2_DT_RAW10, + }, + { + .code = MEDIA_BUS_FMT_Y12_1X12, + .bpp = 12, + .data_type = MIPI_CSI2_DT_RAW12, + }, + { + .code = MEDIA_BUS_FMT_Y14_1X14, + .bpp = 14, + .data_type = MIPI_CSI2_DT_RAW14, + }, + { + .code = MEDIA_BUS_FMT_Y16_1X16, + .bpp = 16, + .data_type = MIPI_CSI2_DT_RAW16, + }, +}; + +static const struct vgxy61_mode_info vgx661_mode_data[] = { + { + .width = VGX661_WIDTH, + .height = VGX661_HEIGHT, + .bin_mode = VGXY61_BIN_MODE_NORMAL, + .crop = { + .left = 0, + .top = 0, + .width = VGX661_WIDTH, + .height = VGX661_HEIGHT, + }, + }, + { + .width = 1280, + .height = 720, + .bin_mode = VGXY61_BIN_MODE_NORMAL, + .crop = { + .left = 92, + .top = 192, + .width = 1280, + .height = 720, + }, + }, + { + .width = 640, + .height = 480, + .bin_mode = VGXY61_BIN_MODE_DIGITAL_X2, + .crop = { + .left = 92, + .top = 72, + .width = 1280, + .height = 960, + }, + }, + { + .width = 320, + .height = 240, + .bin_mode = VGXY61_BIN_MODE_DIGITAL_X4, + .crop = { + .left = 92, + .top = 72, + .width = 1280, + .height = 960, + }, + }, +}; + +static const struct vgxy61_mode_info vgx761_mode_data[] = { + { + .width = VGX761_WIDTH, + .height = VGX761_HEIGHT, + .bin_mode = VGXY61_BIN_MODE_NORMAL, + .crop = { + .left = 0, + .top = 0, + .width = VGX761_WIDTH, + .height = VGX761_HEIGHT, + }, + }, + { + .width = 1920, + .height = 1080, + .bin_mode = VGXY61_BIN_MODE_NORMAL, + .crop = { + .left = 12, + .top = 62, + .width = 1920, + .height = 1080, + }, + }, + { + .width = 1280, + .height = 720, + .bin_mode = VGXY61_BIN_MODE_NORMAL, + .crop = { + .left = 332, + .top = 242, + .width = 1280, + .height = 720, + }, + }, + { + .width = 640, + .height = 480, + .bin_mode = VGXY61_BIN_MODE_DIGITAL_X2, + .crop = { + .left = 332, + .top = 122, + .width = 1280, + .height = 960, + }, + }, + { + .width = 320, + .height = 240, + .bin_mode = VGXY61_BIN_MODE_DIGITAL_X4, + .crop = { + .left = 332, + .top = 122, + .width = 1280, + .height = 960, + }, + }, +}; + +struct vgxy61_dev { + struct i2c_client *i2c_client; + struct v4l2_subdev sd; + struct media_pad pad; + struct regulator_bulk_data supplies[VGXY61_NUM_SUPPLIES]; + struct gpio_desc *reset_gpio; + struct clk *xclk; + u32 clk_freq; + u16 id; + u16 sensor_width; + u16 sensor_height; + u16 oif_ctrl; + unsigned int nb_of_lane; + u32 data_rate_in_mbps; + u32 pclk; + u16 line_length; + u16 rot_term; + bool gpios_polarity; + /* Lock to protect all members below */ + struct mutex lock; + struct v4l2_ctrl_handler ctrl_handler; + struct v4l2_ctrl *pixel_rate_ctrl; + struct v4l2_ctrl *expo_ctrl; + struct v4l2_ctrl *vblank_ctrl; + struct v4l2_ctrl *vflip_ctrl; + struct v4l2_ctrl *hflip_ctrl; + bool streaming; + struct v4l2_mbus_framefmt fmt; + const struct vgxy61_mode_info *sensor_modes; + unsigned int sensor_modes_nb; + const struct vgxy61_mode_info *default_mode; + const struct vgxy61_mode_info *current_mode; + bool hflip; + bool vflip; + enum vgxy61_hdr_mode hdr; + u16 expo_long; + u16 expo_short; + u16 expo_max; + u16 expo_min; + u16 vblank; + u16 vblank_min; + u16 frame_length; + u16 digital_gain; + u8 analog_gain; + enum vgxy61_strobe_mode strobe_mode; + u32 pattern; +}; + +static u8 get_bpp_by_code(__u32 code) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(vgxy61_supported_codes); i++) { + if (vgxy61_supported_codes[i].code == code) + return vgxy61_supported_codes[i].bpp; + } + /* Should never happen */ + WARN(1, "Unsupported code %d. default to 8 bpp", code); + return 8; +} + +static u8 get_data_type_by_code(__u32 code) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(vgxy61_supported_codes); i++) { + if (vgxy61_supported_codes[i].code == code) + return vgxy61_supported_codes[i].data_type; + } + /* Should never happen */ + WARN(1, "Unsupported code %d. default to MIPI_CSI2_DT_RAW8 data type", + code); + return MIPI_CSI2_DT_RAW8; +} + +static void compute_pll_parameters_by_freq(u32 freq, u8 *prediv, u8 *mult) +{ + const unsigned int predivs[] = {1, 2, 4}; + unsigned int i; + + /* + * Freq range is [6Mhz-27Mhz] already checked. + * Output of divider should be in [6Mhz-12Mhz[. + */ + for (i = 0; i < ARRAY_SIZE(predivs); i++) { + *prediv = predivs[i]; + if (freq / *prediv < 12 * HZ_PER_MHZ) + break; + } + WARN_ON(i == ARRAY_SIZE(predivs)); + + /* + * Target freq is 804Mhz. Don't change this as it will impact image + * quality. + */ + *mult = ((804 * HZ_PER_MHZ) * (*prediv) + freq / 2) / freq; +} + +static s32 get_pixel_rate(struct vgxy61_dev *sensor) +{ + return div64_u64((u64)sensor->data_rate_in_mbps * sensor->nb_of_lane, + get_bpp_by_code(sensor->fmt.code)); +} + +static inline struct vgxy61_dev *to_vgxy61_dev(struct v4l2_subdev *sd) +{ + return container_of(sd, struct vgxy61_dev, sd); +} + +static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl) +{ + return &container_of(ctrl->handler, struct vgxy61_dev, + ctrl_handler)->sd; +} + +static unsigned int get_chunk_size(struct vgxy61_dev *sensor) +{ + struct i2c_adapter *adapter = sensor->i2c_client->adapter; + int max_write_len = VGXY61_WRITE_MULTIPLE_CHUNK_MAX; + + if (adapter->quirks && adapter->quirks->max_write_len) + max_write_len = adapter->quirks->max_write_len - 2; + + max_write_len = min(max_write_len, VGXY61_WRITE_MULTIPLE_CHUNK_MAX); + + return max(max_write_len, 1); +} + +static int vgxy61_read_multiple(struct vgxy61_dev *sensor, u32 reg, + unsigned int len) +{ + struct i2c_client *client = sensor->i2c_client; + struct i2c_msg msg[2]; + u8 buf[2]; + u8 val[sizeof(u32)] = {0}; + int ret; + + if (len > sizeof(u32)) + return -EINVAL; + buf[0] = reg >> 8; + buf[1] = reg & 0xff; + + msg[0].addr = client->addr; + msg[0].flags = client->flags; + msg[0].buf = buf; + msg[0].len = sizeof(buf); + + msg[1].addr = client->addr; + msg[1].flags = client->flags | I2C_M_RD; + msg[1].buf = val; + msg[1].len = len; + + ret = i2c_transfer(client->adapter, msg, 2); + if (ret < 0) { + dev_dbg(&client->dev, "%s: %x i2c_transfer, reg: %x => %d\n", + __func__, client->addr, reg, ret); + return ret; + } + + return get_unaligned_le32(val); +} + +static inline int vgxy61_read_reg(struct vgxy61_dev *sensor, u32 reg) +{ + return vgxy61_read_multiple(sensor, reg & VGXY61_REG_ADDR_MASK, + (reg >> VGXY61_REG_SIZE_SHIFT) & 7); +} + +static int vgxy61_write_multiple(struct vgxy61_dev *sensor, u32 reg, + const u8 *data, unsigned int len, int *err) +{ + struct i2c_client *client = sensor->i2c_client; + struct i2c_msg msg; + u8 buf[VGXY61_WRITE_MULTIPLE_CHUNK_MAX + 2]; + unsigned int i; + int ret; + + if (err && *err) + return *err; + + if (len > VGXY61_WRITE_MULTIPLE_CHUNK_MAX) + return -EINVAL; + buf[0] = reg >> 8; + buf[1] = reg & 0xff; + for (i = 0; i < len; i++) + buf[i + 2] = data[i]; + + msg.addr = client->addr; + msg.flags = client->flags; + msg.buf = buf; + msg.len = len + 2; + + ret = i2c_transfer(client->adapter, &msg, 1); + if (ret < 0) { + dev_dbg(&client->dev, "%s: i2c_transfer, reg: %x => %d\n", + __func__, reg, ret); + if (err) + *err = ret; + return ret; + } + + return 0; +} + +static int vgxy61_write_array(struct vgxy61_dev *sensor, u32 reg, + unsigned int nb, const u8 *array) +{ + const unsigned int chunk_size = get_chunk_size(sensor); + int ret; + unsigned int sz; + + while (nb) { + sz = min(nb, chunk_size); + ret = vgxy61_write_multiple(sensor, reg, array, sz, NULL); + if (ret < 0) + return ret; + nb -= sz; + reg += sz; + array += sz; + } + + return 0; +} + +static inline int vgxy61_write_reg(struct vgxy61_dev *sensor, u32 reg, u32 val, + int *err) +{ + return vgxy61_write_multiple(sensor, reg & VGXY61_REG_ADDR_MASK, + (u8 *)&val, + (reg >> VGXY61_REG_SIZE_SHIFT) & 7, err); +} + +static int vgxy61_poll_reg(struct vgxy61_dev *sensor, u32 reg, u8 poll_val, + unsigned int timeout_ms) +{ + const unsigned int loop_delay_ms = 10; + int ret, timeout; + + timeout = read_poll_timeout(vgxy61_read_reg, ret, + ((ret < 0) || (ret == poll_val)), + loop_delay_ms * 1000, timeout_ms * 1000, + false, sensor, reg); + if (timeout) + return timeout; + + return 0; +} + +static int vgxy61_wait_state(struct vgxy61_dev *sensor, int state, + unsigned int timeout_ms) +{ + return vgxy61_poll_reg(sensor, VGXY61_REG_SYSTEM_FSM, state, + timeout_ms); +} + +static int vgxy61_check_bw(struct vgxy61_dev *sensor) +{ + /* + * Simplification of time needed to send short packets and for the MIPI + * to add transition times (EoT, LPS, and SoT packet delimiters) needed + * by the protocol to go in low power between 2 packets of data. This + * is a mipi IP constant for the sensor. + */ + const unsigned int mipi_margin = 1056; + unsigned int binning_scale = sensor->current_mode->crop.height / + sensor->current_mode->height; + u8 bpp = get_bpp_by_code(sensor->fmt.code); + unsigned int max_bit_per_line; + unsigned int bit_per_line; + u64 line_rate; + + line_rate = sensor->nb_of_lane * (u64)sensor->data_rate_in_mbps * + sensor->line_length; + max_bit_per_line = div64_u64(line_rate, sensor->pclk) - mipi_margin; + bit_per_line = (bpp * sensor->current_mode->width) / binning_scale; + + return bit_per_line > max_bit_per_line ? -EINVAL : 0; +} + +static int vgxy61_apply_exposure(struct vgxy61_dev *sensor) +{ + int ret = 0; + + /* We first set expo to zero to avoid forbidden parameters couple */ + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, + 0, &ret); + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_LONG, + sensor->expo_long, &ret); + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, + sensor->expo_short, &ret); + if (ret) + return ret; + + return 0; +} + +static int vgxy61_get_regulators(struct vgxy61_dev *sensor) +{ + unsigned int i; + + for (i = 0; i < VGXY61_NUM_SUPPLIES; i++) + sensor->supplies[i].supply = vgxy61_supply_name[i]; + + return devm_regulator_bulk_get(&sensor->i2c_client->dev, + VGXY61_NUM_SUPPLIES, sensor->supplies); +} + +static int vgxy61_apply_reset(struct vgxy61_dev *sensor) +{ + gpiod_set_value_cansleep(sensor->reset_gpio, 0); + usleep_range(5000, 10000); + gpiod_set_value_cansleep(sensor->reset_gpio, 1); + usleep_range(5000, 10000); + gpiod_set_value_cansleep(sensor->reset_gpio, 0); + usleep_range(40000, 100000); + return vgxy61_wait_state(sensor, VGXY61_SYSTEM_FSM_SW_STBY, + VGXY61_TIMEOUT_MS); +} + +static void vgxy61_fill_framefmt(struct vgxy61_dev *sensor, + const struct vgxy61_mode_info *mode, + struct v4l2_mbus_framefmt *fmt, u32 code) +{ + fmt->code = code; + fmt->width = mode->width; + fmt->height = mode->height; + fmt->colorspace = V4L2_COLORSPACE_RAW; + fmt->field = V4L2_FIELD_NONE; + fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; + fmt->quantization = V4L2_QUANTIZATION_DEFAULT; + fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT; +} + +static int vgxy61_try_fmt_internal(struct v4l2_subdev *sd, + struct v4l2_mbus_framefmt *fmt, + const struct vgxy61_mode_info **new_mode) +{ + struct vgxy61_dev *sensor = to_vgxy61_dev(sd); + const struct vgxy61_mode_info *mode = sensor->sensor_modes; + unsigned int index; + + for (index = 0; index < ARRAY_SIZE(vgxy61_supported_codes); index++) { + if (vgxy61_supported_codes[index].code == fmt->code) + break; + } + if (index == ARRAY_SIZE(vgxy61_supported_codes)) + index = 0; + + mode = v4l2_find_nearest_size(sensor->sensor_modes, + sensor->sensor_modes_nb, width, height, + fmt->width, fmt->height); + if (new_mode) + *new_mode = mode; + + vgxy61_fill_framefmt(sensor, mode, fmt, + vgxy61_supported_codes[index].code); + + return 0; +} + +static int vgxy61_get_selection(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_selection *sel) +{ + struct vgxy61_dev *sensor = to_vgxy61_dev(sd); + + switch (sel->target) { + case V4L2_SEL_TGT_CROP: + sel->r = sensor->current_mode->crop; + return 0; + case V4L2_SEL_TGT_NATIVE_SIZE: + case V4L2_SEL_TGT_CROP_DEFAULT: + case V4L2_SEL_TGT_CROP_BOUNDS: + sel->r.top = 0; + sel->r.left = 0; + sel->r.width = sensor->sensor_width; + sel->r.height = sensor->sensor_height; + return 0; + } + + return -EINVAL; +} + +static int vgxy61_enum_mbus_code(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_mbus_code_enum *code) +{ + if (code->index >= ARRAY_SIZE(vgxy61_supported_codes)) + return -EINVAL; + + code->code = vgxy61_supported_codes[code->index].code; + + return 0; +} + +static int vgxy61_get_fmt(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_format *format) +{ + struct vgxy61_dev *sensor = to_vgxy61_dev(sd); + struct v4l2_mbus_framefmt *fmt; + + mutex_lock(&sensor->lock); + + if (format->which == V4L2_SUBDEV_FORMAT_TRY) + fmt = v4l2_subdev_get_try_format(&sensor->sd, sd_state, + format->pad); + else + fmt = &sensor->fmt; + + format->format = *fmt; + + mutex_unlock(&sensor->lock); + + return 0; +} + +static u16 vgxy61_get_vblank_min(struct vgxy61_dev *sensor, + enum vgxy61_hdr_mode hdr) +{ + u16 min_vblank = VGXY61_MIN_FRAME_LENGTH - + sensor->current_mode->crop.height; + /* Ensure the first rule of thumb can't be negative */ + u16 min_vblank_hdr = VGXY61_MIN_EXPOSURE + sensor->rot_term + 1; + + if (hdr != VGXY61_NO_HDR) + return max(min_vblank, min_vblank_hdr); + return min_vblank; +} + +static int vgxy61_enum_frame_size(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_frame_size_enum *fse) +{ + struct vgxy61_dev *sensor = to_vgxy61_dev(sd); + + if (fse->index >= sensor->sensor_modes_nb) + return -EINVAL; + + fse->min_width = sensor->sensor_modes[fse->index].width; + fse->max_width = fse->min_width; + fse->min_height = sensor->sensor_modes[fse->index].height; + fse->max_height = fse->min_height; + + return 0; +} + +static int vgxy61_update_analog_gain(struct vgxy61_dev *sensor, u32 target) +{ + sensor->analog_gain = target; + + if (sensor->streaming) + return vgxy61_write_reg(sensor, VGXY61_REG_ANALOG_GAIN, target, + NULL); + return 0; +} + +static int vgxy61_apply_digital_gain(struct vgxy61_dev *sensor, + u32 digital_gain) +{ + int ret = 0; + + /* + * For a monochrome version, configuring DIGITAL_GAIN_LONG_CH0 and + * DIGITAL_GAIN_SHORT_CH0 is enough to configure the gain of all + * four sub pixels. + */ + vgxy61_write_reg(sensor, VGXY61_REG_DIGITAL_GAIN_LONG, digital_gain, + &ret); + vgxy61_write_reg(sensor, VGXY61_REG_DIGITAL_GAIN_SHORT, digital_gain, + &ret); + + return ret; +} + +static int vgxy61_update_digital_gain(struct vgxy61_dev *sensor, u32 target) +{ + sensor->digital_gain = target; + + if (sensor->streaming) + return vgxy61_apply_digital_gain(sensor, sensor->digital_gain); + return 0; +} + +static int vgxy61_apply_patgen(struct vgxy61_dev *sensor, u32 index) +{ + static const u8 index2val[] = { + 0x0, 0x1, 0x2, 0x3, 0x10, 0x11, 0x12, 0x13 + }; + u32 pattern = index2val[index]; + u32 reg = (pattern << VGXY61_PATGEN_LONG_TYPE_SHIFT) | + (pattern << VGXY61_PATGEN_SHORT_TYPE_SHIFT); + + if (pattern) + reg |= VGXY61_PATGEN_LONG_ENABLE | VGXY61_PATGEN_SHORT_ENABLE; + return vgxy61_write_reg(sensor, VGXY61_REG_PATGEN_CTRL, reg, NULL); +} + +static int vgxy61_update_patgen(struct vgxy61_dev *sensor, u32 pattern) +{ + sensor->pattern = pattern; + + if (sensor->streaming) + return vgxy61_apply_patgen(sensor, sensor->pattern); + return 0; +} + +static int vgxy61_apply_gpiox_strobe_mode(struct vgxy61_dev *sensor, + enum vgxy61_strobe_mode mode, + unsigned int idx) +{ + static const u8 index2val[] = {0x0, 0x1, 0x3}; + u16 reg; + + reg = vgxy61_read_reg(sensor, VGXY61_REG_SIGNALS_CTRL); + if (reg < 0) + return reg; + reg &= ~(0xf << (idx * VGXY61_SIGNALS_GPIO_ID_SHIFT)); + reg |= index2val[mode] << (idx * VGXY61_SIGNALS_GPIO_ID_SHIFT); + + return vgxy61_write_reg(sensor, VGXY61_REG_SIGNALS_CTRL, reg, NULL); +} + +static int vgxy61_update_gpios_strobe_mode(struct vgxy61_dev *sensor, + enum vgxy61_hdr_mode hdr) +{ + unsigned int i; + int ret; + + switch (hdr) { + case VGXY61_HDR_LINEAR: + sensor->strobe_mode = VGXY61_STROBE_ENABLED; + break; + case VGXY61_HDR_SUB: + case VGXY61_NO_HDR: + sensor->strobe_mode = VGXY61_STROBE_LONG; + break; + default: + /* Should never happen */ + WARN_ON(true); + break; + } + + if (!sensor->streaming) + return 0; + + for (i = 0; i < VGXY61_NB_GPIOS; i++) { + ret = vgxy61_apply_gpiox_strobe_mode(sensor, + sensor->strobe_mode, + i); + if (ret) + return ret; + } + + return 0; +} + +static int vgxy61_update_gpios_strobe_polarity(struct vgxy61_dev *sensor, + bool polarity) +{ + int ret = 0; + + if (sensor->streaming) + return -EBUSY; + + vgxy61_write_reg(sensor, VGXY61_REG_GPIO_0_CTRL, polarity << 1, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_GPIO_1_CTRL, polarity << 1, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_GPIO_2_CTRL, polarity << 1, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_GPIO_3_CTRL, polarity << 1, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_SIGNALS_POLARITY_CTRL, polarity, + &ret); + + return ret; +} + +static u32 vgxy61_get_expo_long_max(struct vgxy61_dev *sensor, + unsigned int short_expo_ratio) +{ + u32 first_rot_max_expo, second_rot_max_expo, third_rot_max_expo; + + /* Apply sensor's rules of thumb */ + /* + * Short exposure + height must be less than frame length to avoid bad + * pixel line at the botom of the image + */ + first_rot_max_expo = + ((sensor->frame_length - sensor->current_mode->crop.height - + sensor->rot_term) * short_expo_ratio) - 1; + + /* + * Total exposition time must be less than frame length to avoid sensor + * crash + */ + second_rot_max_expo = + (((sensor->frame_length - VGXY61_EXPOS_ROT_TERM) * + short_expo_ratio) / (short_expo_ratio + 1)) - 1; + + /* + * Short exposure times 71 must be less than frame length to avoid + * sensor crash + */ + third_rot_max_expo = (sensor->frame_length / 71) * short_expo_ratio; + + /* Take the minimum from all rules */ + return min(min(first_rot_max_expo, second_rot_max_expo), + third_rot_max_expo); +} + +static int vgxy61_update_exposure(struct vgxy61_dev *sensor, u16 new_expo_long, + enum vgxy61_hdr_mode hdr) +{ + struct i2c_client *client = sensor->i2c_client; + u16 new_expo_short = 0; + u16 expo_short_max = 0; + u16 expo_long_min = VGXY61_MIN_EXPOSURE; + u16 expo_long_max; + + /* Compute short exposure according to hdr mode and long exposure */ + switch (hdr) { + case VGXY61_HDR_LINEAR: + /* + * Take ratio into account for minimal exposures in + * VGXY61_HDR_LINEAR + */ + expo_long_min = VGXY61_MIN_EXPOSURE * VGXY61_HDR_LINEAR_RATIO; + new_expo_long = max(expo_long_min, new_expo_long); + + expo_long_max = + vgxy61_get_expo_long_max(sensor, + VGXY61_HDR_LINEAR_RATIO); + expo_short_max = (expo_long_max + + (VGXY61_HDR_LINEAR_RATIO / 2)) / + VGXY61_HDR_LINEAR_RATIO; + new_expo_short = (new_expo_long + + (VGXY61_HDR_LINEAR_RATIO / 2)) / + VGXY61_HDR_LINEAR_RATIO; + break; + case VGXY61_HDR_SUB: + new_expo_long = max(expo_long_min, new_expo_long); + + expo_long_max = vgxy61_get_expo_long_max(sensor, 1); + /* Short and long are the same in VGXY61_HDR_SUB */ + expo_short_max = expo_long_max; + new_expo_short = new_expo_long; + break; + case VGXY61_NO_HDR: + new_expo_long = max(expo_long_min, new_expo_long); + + /* + * As short expo is 0 here, only the second rule of thumb + * applies, see vgxy61_get_expo_long_max for more + */ + expo_long_max = sensor->frame_length - VGXY61_EXPOS_ROT_TERM; + break; + default: + /* Should never happen */ + WARN_ON(true); + break; + } + + /* If this happens, something is wrong with formulas */ + WARN_ON(expo_long_min > expo_long_max); + + if (new_expo_long > expo_long_max) { + dev_warn(&client->dev, "Exposure %d too high, clamping to %d\n", + new_expo_long, expo_long_max); + new_expo_long = expo_long_max; + new_expo_short = expo_short_max; + } + + sensor->expo_long = new_expo_long; + sensor->expo_short = new_expo_short; + sensor->expo_max = expo_long_max; + sensor->expo_min = expo_long_min; + + if (sensor->streaming) + return vgxy61_apply_exposure(sensor); + return 0; +} + +static int vgxy61_apply_framelength(struct vgxy61_dev *sensor) +{ + return vgxy61_write_reg(sensor, VGXY61_REG_FRAME_LENGTH, + sensor->frame_length, NULL); +} + +static int vgxy61_update_vblank(struct vgxy61_dev *sensor, u16 vblank, + enum vgxy61_hdr_mode hdr) +{ + int ret; + + sensor->vblank_min = vgxy61_get_vblank_min(sensor, hdr); + sensor->vblank = max(sensor->vblank_min, vblank); + sensor->frame_length = sensor->current_mode->crop.height + + sensor->vblank; + + /* Update exposure according to vblank */ + ret = vgxy61_update_exposure(sensor, sensor->expo_long, hdr); + if (ret) + return ret; + + if (sensor->streaming) + return vgxy61_apply_framelength(sensor); + return 0; +} + +static int vgxy61_apply_hdr(struct vgxy61_dev *sensor, + enum vgxy61_hdr_mode index) +{ + static const u8 index2val[] = {0x1, 0x4, 0xa}; + + return vgxy61_write_reg(sensor, VGXY61_REG_HDR_CTRL, index2val[index], + NULL); +} + +static int vgxy61_update_hdr(struct vgxy61_dev *sensor, + enum vgxy61_hdr_mode index) +{ + int ret; + + /* + * vblank and short exposure change according to HDR mode, do it first + * as it can violate sensors 'rule of thumbs' and therefore will require + * to change the long exposure. + */ + ret = vgxy61_update_vblank(sensor, sensor->vblank, index); + if (ret) + return ret; + + /* Update strobe mode according to HDR */ + ret = vgxy61_update_gpios_strobe_mode(sensor, index); + if (ret) + return ret; + + sensor->hdr = index; + + if (sensor->streaming) + return vgxy61_apply_hdr(sensor, sensor->hdr); + return 0; +} + +static int vgxy61_apply_settings(struct vgxy61_dev *sensor) +{ + int ret; + unsigned int i; + + ret = vgxy61_apply_hdr(sensor, sensor->hdr); + if (ret) + return ret; + + ret = vgxy61_apply_framelength(sensor); + if (ret) + return ret; + + ret = vgxy61_apply_exposure(sensor); + if (ret) + return ret; + + ret = vgxy61_write_reg(sensor, VGXY61_REG_ANALOG_GAIN, + sensor->analog_gain, NULL); + if (ret) + return ret; + ret = vgxy61_apply_digital_gain(sensor, sensor->digital_gain); + if (ret) + return ret; + + ret = vgxy61_write_reg(sensor, VGXY61_REG_ORIENTATION, + sensor->hflip | (sensor->vflip << 1), NULL); + if (ret) + return ret; + + ret = vgxy61_apply_patgen(sensor, sensor->pattern); + if (ret) + return ret; + + for (i = 0; i < VGXY61_NB_GPIOS; i++) { + ret = vgxy61_apply_gpiox_strobe_mode(sensor, + sensor->strobe_mode, i); + if (ret) + return ret; + } + + return 0; +} + +static int vgxy61_stream_enable(struct vgxy61_dev *sensor) +{ + struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd); + const struct v4l2_rect *crop = &sensor->current_mode->crop; + int ret = 0; + + ret = vgxy61_check_bw(sensor); + if (ret) + return ret; + + ret = pm_runtime_get_sync(&client->dev); + if (ret < 0) { + pm_runtime_put_autosuspend(&client->dev); + return ret; + } + + vgxy61_write_reg(sensor, VGXY61_REG_FORMAT_CTRL, + get_bpp_by_code(sensor->fmt.code), &ret); + vgxy61_write_reg(sensor, VGXY61_REG_OIF_ROI0_CTRL, + get_data_type_by_code(sensor->fmt.code), &ret); + + vgxy61_write_reg(sensor, VGXY61_REG_READOUT_CTRL, + sensor->current_mode->bin_mode, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_ROI0_START_H, crop->left, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_ROI0_END_H, + crop->left + crop->width - 1, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_ROI0_START_V, crop->top, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_ROI0_END_V, + crop->top + crop->height - 1, &ret); + if (ret) + goto err_rpm_put; + + ret = vgxy61_apply_settings(sensor); + if (ret) + goto err_rpm_put; + + ret = vgxy61_write_reg(sensor, VGXY61_REG_STREAMING, + VGXY61_STREAMING_REQ_START, NULL); + if (ret) + goto err_rpm_put; + + ret = vgxy61_poll_reg(sensor, VGXY61_REG_STREAMING, + VGXY61_STREAMING_NO_REQ, VGXY61_TIMEOUT_MS); + if (ret) + goto err_rpm_put; + + ret = vgxy61_wait_state(sensor, VGXY61_SYSTEM_FSM_STREAMING, + VGXY61_TIMEOUT_MS); + if (ret) + goto err_rpm_put; + + /* vflip and hflip cannot change during streaming */ + __v4l2_ctrl_grab(sensor->vflip_ctrl, true); + __v4l2_ctrl_grab(sensor->hflip_ctrl, true); + + return 0; + +err_rpm_put: + pm_runtime_put(&client->dev); + return ret; +} + +static int vgxy61_stream_disable(struct vgxy61_dev *sensor) +{ + struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd); + int ret; + + ret = vgxy61_write_reg(sensor, VGXY61_REG_STREAMING, + VGXY61_STREAMING_REQ_STOP, NULL); + if (ret) + goto err_str_dis; + + ret = vgxy61_poll_reg(sensor, VGXY61_REG_STREAMING, + VGXY61_STREAMING_NO_REQ, 2000); + if (ret) + goto err_str_dis; + + ret = vgxy61_wait_state(sensor, VGXY61_SYSTEM_FSM_SW_STBY, + VGXY61_TIMEOUT_MS); + if (ret) + goto err_str_dis; + + __v4l2_ctrl_grab(sensor->vflip_ctrl, false); + __v4l2_ctrl_grab(sensor->hflip_ctrl, false); + +err_str_dis: + if (ret) + WARN(1, "Can't disable stream"); + pm_runtime_put(&client->dev); + + return ret; +} + +static int vgxy61_s_stream(struct v4l2_subdev *sd, int enable) +{ + struct vgxy61_dev *sensor = to_vgxy61_dev(sd); + int ret = 0; + + mutex_lock(&sensor->lock); + + ret = enable ? vgxy61_stream_enable(sensor) : + vgxy61_stream_disable(sensor); + if (!ret) + sensor->streaming = enable; + + mutex_unlock(&sensor->lock); + + return ret; +} + +static int vgxy61_set_fmt(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_format *format) +{ + struct vgxy61_dev *sensor = to_vgxy61_dev(sd); + const struct vgxy61_mode_info *new_mode; + struct v4l2_mbus_framefmt *fmt; + int ret; + + mutex_lock(&sensor->lock); + + if (sensor->streaming) { + ret = -EBUSY; + goto out; + } + + ret = vgxy61_try_fmt_internal(sd, &format->format, &new_mode); + if (ret) + goto out; + + if (format->which == V4L2_SUBDEV_FORMAT_TRY) { + fmt = v4l2_subdev_get_try_format(sd, sd_state, 0); + *fmt = format->format; + } else if (sensor->current_mode != new_mode || + sensor->fmt.code != format->format.code) { + fmt = &sensor->fmt; + *fmt = format->format; + + sensor->current_mode = new_mode; + + /* Reset vblank and framelength to default */ + ret = vgxy61_update_vblank(sensor, + VGXY61_FRAME_LENGTH_DEF - + new_mode->crop.height, + sensor->hdr); + + /* Update controls to reflect new mode */ + __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate_ctrl, + get_pixel_rate(sensor)); + __v4l2_ctrl_modify_range(sensor->vblank_ctrl, + sensor->vblank_min, + 0xffff - new_mode->crop.height, + 1, sensor->vblank); + __v4l2_ctrl_s_ctrl(sensor->vblank_ctrl, sensor->vblank); + __v4l2_ctrl_modify_range(sensor->expo_ctrl, sensor->expo_min, + sensor->expo_max, 1, + sensor->expo_long); + } + +out: + mutex_unlock(&sensor->lock); + + return ret; +} + +static int vgxy61_init_cfg(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state) +{ + struct vgxy61_dev *sensor = to_vgxy61_dev(sd); + struct v4l2_subdev_format fmt = { 0 }; + + sensor->current_mode = sensor->default_mode; + vgxy61_fill_framefmt(sensor, sensor->current_mode, &fmt.format, + VGXY61_MEDIA_BUS_FMT_DEF); + + return vgxy61_set_fmt(sd, sd_state, &fmt); +} + +static int vgxy61_s_ctrl(struct v4l2_ctrl *ctrl) +{ + struct v4l2_subdev *sd = ctrl_to_sd(ctrl); + struct vgxy61_dev *sensor = to_vgxy61_dev(sd); + const struct vgxy61_mode_info *cur_mode = sensor->current_mode; + int ret; + + switch (ctrl->id) { + case V4L2_CID_EXPOSURE: + ret = vgxy61_update_exposure(sensor, ctrl->val, sensor->hdr); + ctrl->val = sensor->expo_long; + break; + case V4L2_CID_ANALOGUE_GAIN: + ret = vgxy61_update_analog_gain(sensor, ctrl->val); + break; + case V4L2_CID_DIGITAL_GAIN: + ret = vgxy61_update_digital_gain(sensor, ctrl->val); + break; + case V4L2_CID_VFLIP: + case V4L2_CID_HFLIP: + if (sensor->streaming) { + ret = -EBUSY; + break; + } + if (ctrl->id == V4L2_CID_VFLIP) + sensor->vflip = ctrl->val; + if (ctrl->id == V4L2_CID_HFLIP) + sensor->hflip = ctrl->val; + ret = 0; + break; + case V4L2_CID_TEST_PATTERN: + ret = vgxy61_update_patgen(sensor, ctrl->val); + break; + case V4L2_CID_HDR_MODE: + ret = vgxy61_update_hdr(sensor, ctrl->val); + /* Update vblank and exposure controls to match new hdr */ + __v4l2_ctrl_modify_range(sensor->vblank_ctrl, + sensor->vblank_min, + 0xffff - cur_mode->crop.height, + 1, sensor->vblank); + __v4l2_ctrl_modify_range(sensor->expo_ctrl, sensor->expo_min, + sensor->expo_max, 1, + sensor->expo_long); + break; + case V4L2_CID_VBLANK: + ret = vgxy61_update_vblank(sensor, ctrl->val, sensor->hdr); + /* Update exposure control to match new vblank */ + __v4l2_ctrl_modify_range(sensor->expo_ctrl, sensor->expo_min, + sensor->expo_max, 1, + sensor->expo_long); + break; + default: + ret = -EINVAL; + break; + } + + return ret; +} + +static const struct v4l2_ctrl_ops vgxy61_ctrl_ops = { + .s_ctrl = vgxy61_s_ctrl, +}; + +static int vgxy61_init_controls(struct vgxy61_dev *sensor) +{ + const struct v4l2_ctrl_ops *ops = &vgxy61_ctrl_ops; + struct v4l2_ctrl_handler *hdl = &sensor->ctrl_handler; + const struct vgxy61_mode_info *cur_mode = sensor->current_mode; + struct v4l2_ctrl *ctrl; + int ret; + + v4l2_ctrl_handler_init(hdl, 16); + /* We can use our own mutex for the ctrl lock */ + hdl->lock = &sensor->lock; + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN, 0, 0x1c, 1, + sensor->analog_gain); + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN, 0, 0xfff, 1, + sensor->digital_gain); + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN, + ARRAY_SIZE(vgxy61_test_pattern_menu) - 1, + 0, 0, vgxy61_test_pattern_menu); + ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, 0, + sensor->line_length, 1, + sensor->line_length - cur_mode->width); + if (ctrl) + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; + ctrl = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, + ARRAY_SIZE(link_freq) - 1, 0, link_freq); + if (ctrl) + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_HDR_MODE, + ARRAY_SIZE(vgxy61_hdr_mode_menu) - 1, 0, + VGXY61_NO_HDR, vgxy61_hdr_mode_menu); + + /* + * Keep a pointer to these controls as we need to update them when + * setting the format + */ + sensor->pixel_rate_ctrl = v4l2_ctrl_new_std(hdl, ops, + V4L2_CID_PIXEL_RATE, 1, + INT_MAX, 1, + get_pixel_rate(sensor)); + sensor->pixel_rate_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; + sensor->expo_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, + sensor->expo_min, + sensor->expo_max, 1, + sensor->expo_long); + sensor->vblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, + sensor->vblank_min, + 0xffff - cur_mode->crop.height, + 1, sensor->vblank); + sensor->vflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, + 0, 1, 1, sensor->vflip); + sensor->hflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, + 0, 1, 1, sensor->hflip); + + if (hdl->error) { + ret = hdl->error; + goto free_ctrls; + } + + sensor->sd.ctrl_handler = hdl; + return 0; + +free_ctrls: + v4l2_ctrl_handler_free(hdl); + return ret; +} + +static const struct v4l2_subdev_video_ops vgxy61_video_ops = { + .s_stream = vgxy61_s_stream, +}; + +static const struct v4l2_subdev_pad_ops vgxy61_pad_ops = { + .init_cfg = vgxy61_init_cfg, + .enum_mbus_code = vgxy61_enum_mbus_code, + .get_fmt = vgxy61_get_fmt, + .set_fmt = vgxy61_set_fmt, + .get_selection = vgxy61_get_selection, + .enum_frame_size = vgxy61_enum_frame_size, +}; + +static const struct v4l2_subdev_ops vgxy61_subdev_ops = { + .video = &vgxy61_video_ops, + .pad = &vgxy61_pad_ops, +}; + +static const struct media_entity_operations vgxy61_subdev_entity_ops = { + .link_validate = v4l2_subdev_link_validate, +}; + +static int vgxy61_tx_from_ep(struct vgxy61_dev *sensor, + struct fwnode_handle *handle) +{ + struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_CSI2_DPHY }; + struct i2c_client *client = sensor->i2c_client; + u32 log2phy[VGXY61_NB_POLARITIES] = {~0, ~0, ~0, ~0, ~0}; + u32 phy2log[VGXY61_NB_POLARITIES] = {~0, ~0, ~0, ~0, ~0}; + int polarities[VGXY61_NB_POLARITIES] = {0, 0, 0, 0, 0}; + int l_nb; + unsigned int p, l, i; + int ret; + + ret = v4l2_fwnode_endpoint_alloc_parse(handle, &ep); + if (ret) + return -EINVAL; + + l_nb = ep.bus.mipi_csi2.num_data_lanes; + if (l_nb != 1 && l_nb != 2 && l_nb != 4) { + dev_err(&client->dev, "invalid data lane number %d\n", l_nb); + goto error_ep; + } + + /* Build log2phy, phy2log and polarities from ep info */ + log2phy[0] = ep.bus.mipi_csi2.clock_lane; + phy2log[log2phy[0]] = 0; + for (l = 1; l < l_nb + 1; l++) { + log2phy[l] = ep.bus.mipi_csi2.data_lanes[l - 1]; + phy2log[log2phy[l]] = l; + } + /* + * Then fill remaining slots for every physical slot to have something + * valid for hardware stuff. + */ + for (p = 0; p < VGXY61_NB_POLARITIES; p++) { + if (phy2log[p] != ~0) + continue; + phy2log[p] = l; + log2phy[l] = p; + l++; + } + for (l = 0; l < l_nb + 1; l++) + polarities[l] = ep.bus.mipi_csi2.lane_polarities[l]; + + if (log2phy[0] != 0) { + dev_err(&client->dev, "clk lane must be map to physical lane 0\n"); + goto error_ep; + } + sensor->oif_ctrl = (polarities[4] << 15) + ((phy2log[4] - 1) << 13) + + (polarities[3] << 12) + ((phy2log[3] - 1) << 10) + + (polarities[2] << 9) + ((phy2log[2] - 1) << 7) + + (polarities[1] << 6) + ((phy2log[1] - 1) << 4) + + (polarities[0] << 3) + + l_nb; + sensor->nb_of_lane = l_nb; + + dev_dbg(&client->dev, "tx uses %d lanes", l_nb); + for (i = 0; i < 5; i++) { + dev_dbg(&client->dev, "log2phy[%d] = %d\n", i, log2phy[i]); + dev_dbg(&client->dev, "phy2log[%d] = %d\n", i, phy2log[i]); + dev_dbg(&client->dev, "polarity[%d] = %d\n", i, polarities[i]); + } + dev_dbg(&client->dev, "oif_ctrl = 0x%04x\n", sensor->oif_ctrl); + + v4l2_fwnode_endpoint_free(&ep); + + return 0; + +error_ep: + v4l2_fwnode_endpoint_free(&ep); + + return -EINVAL; +} + +static int vgxy61_configure(struct vgxy61_dev *sensor) +{ + u32 sensor_freq; + u8 prediv, mult; + u16 line_length; + int ret = 0; + + compute_pll_parameters_by_freq(sensor->clk_freq, &prediv, &mult); + sensor_freq = (mult * sensor->clk_freq) / prediv; + /* Frequency to data rate is 1:1 ratio for MIPI */ + sensor->data_rate_in_mbps = sensor_freq; + /* Video timing ISP path (pixel clock) requires 804/5 mhz = 160 mhz */ + sensor->pclk = sensor_freq / 5; + + line_length = vgxy61_read_reg(sensor, VGXY61_REG_LINE_LENGTH); + if (line_length < 0) + return line_length; + sensor->line_length = line_length; + vgxy61_write_reg(sensor, VGXY61_REG_EXT_CLOCK, sensor->clk_freq, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_CLK_PLL_PREDIV, prediv, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_CLK_SYS_PLL_MULT, mult, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_OIF_CTRL, sensor->oif_ctrl, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_FRAME_CONTENT_CTRL, 0, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_BYPASS_CTRL, 4, &ret); + if (ret) + return ret; + vgxy61_update_gpios_strobe_polarity(sensor, sensor->gpios_polarity); + /* Set pattern generator solid to middle value */ + vgxy61_write_reg(sensor, VGXY61_REG_PATGEN_LONG_DATA_GR, 0x800, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_PATGEN_LONG_DATA_R, 0x800, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_PATGEN_LONG_DATA_B, 0x800, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_PATGEN_LONG_DATA_GB, 0x800, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_PATGEN_SHORT_DATA_GR, 0x800, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_PATGEN_SHORT_DATA_R, 0x800, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_PATGEN_SHORT_DATA_B, 0x800, &ret); + vgxy61_write_reg(sensor, VGXY61_REG_PATGEN_SHORT_DATA_GB, 0x800, &ret); + if (ret) + return ret; + + return 0; +} + +static int vgxy61_patch(struct vgxy61_dev *sensor) +{ + struct i2c_client *client = sensor->i2c_client; + u16 patch; + int ret; + + ret = vgxy61_write_array(sensor, VGXY61_REG_FWPATCH_START_ADDR, + sizeof(patch_array), patch_array); + if (ret) + return ret; + + ret = vgxy61_write_reg(sensor, VGXY61_REG_STBY, 0x10, NULL); + if (ret) + return ret; + + ret = vgxy61_poll_reg(sensor, VGXY61_REG_STBY, 0, VGXY61_TIMEOUT_MS); + if (ret) + return ret; + + patch = vgxy61_read_reg(sensor, VGXY61_REG_FWPATCH_REVISION); + if (patch < 0) + return patch; + + if (patch != (VGXY61_FWPATCH_REVISION_MAJOR << 12) + + (VGXY61_FWPATCH_REVISION_MINOR << 8) + + VGXY61_FWPATCH_REVISION_MICRO) { + dev_err(&client->dev, "bad patch version expected %d.%d.%d got %d.%d.%d\n", + VGXY61_FWPATCH_REVISION_MAJOR, + VGXY61_FWPATCH_REVISION_MINOR, + VGXY61_FWPATCH_REVISION_MICRO, + patch >> 12, (patch >> 8) & 0x0f, patch & 0xff); + return -ENODEV; + } + dev_dbg(&client->dev, "patch %d.%d.%d applied\n", + patch >> 12, (patch >> 8) & 0x0f, patch & 0xff); + + return 0; +} + +static int vgxy61_detect_cut_version(struct vgxy61_dev *sensor) +{ + struct i2c_client *client = sensor->i2c_client; + u16 device_rev; + + device_rev = vgxy61_read_reg(sensor, VGXY61_REG_REVISION); + if (device_rev < 0) + return device_rev; + + switch (device_rev >> 8) { + case 0xA: + dev_dbg(&client->dev, "Cut1 detected\n"); + dev_err(&client->dev, "Cut1 not supported by this driver\n"); + return -ENODEV; + case 0xB: + dev_dbg(&client->dev, "Cut2 detected\n"); + return 0; + case 0xC: + dev_dbg(&client->dev, "Cut3 detected\n"); + return 0; + default: + dev_err(&client->dev, "Unable to detect cut version\n"); + return -ENODEV; + } +} + +static int vgxy61_detect(struct vgxy61_dev *sensor) +{ + struct i2c_client *client = sensor->i2c_client; + u16 id = 0; + int ret; + u8 st; + + id = vgxy61_read_reg(sensor, VGXY61_REG_MODEL_ID); + if (id < 0) + return id; + if (id != VG5661_MODEL_ID && id != VG5761_MODEL_ID) { + dev_warn(&client->dev, "Unsupported sensor id %x\n", id); + return -ENODEV; + } + dev_dbg(&client->dev, "detected sensor id = 0x%04x\n", id); + sensor->id = id; + + ret = vgxy61_wait_state(sensor, VGXY61_SYSTEM_FSM_SW_STBY, + VGXY61_TIMEOUT_MS); + if (ret) + return ret; + + st = vgxy61_read_reg(sensor, VGXY61_REG_NVM); + if (st < 0) + return st; + if (st != VGXY61_NVM_OK) + dev_warn(&client->dev, "Bad nvm state got %d\n", st); + + ret = vgxy61_detect_cut_version(sensor); + if (ret) + return ret; + + return 0; +} + +/* Power/clock management functions */ +static int vgxy61_power_on(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct vgxy61_dev *sensor = to_vgxy61_dev(sd); + int ret; + + ret = clk_prepare_enable(sensor->xclk); + if (ret) { + dev_err(&client->dev, "failed to enable clock %d\n", ret); + goto disable_bulk; + } + + if (sensor->reset_gpio) { + ret = vgxy61_apply_reset(sensor); + if (ret) { + dev_err(&client->dev, "sensor reset failed %d\n", ret); + goto disable_clock; + } + } + + ret = vgxy61_patch(sensor); + if (ret) { + dev_err(&client->dev, "sensor patch failed %d\n", ret); + goto disable_clock; + } + + ret = vgxy61_configure(sensor); + if (ret) { + dev_err(&client->dev, "sensor configuration failed %d\n", ret); + goto disable_clock; + } + + return 0; + +disable_clock: + clk_disable_unprepare(sensor->xclk); +disable_bulk: + regulator_bulk_disable(VGXY61_NUM_SUPPLIES, sensor->supplies); + + return ret; +} + +static int vgxy61_power_off(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct vgxy61_dev *sensor = to_vgxy61_dev(sd); + + clk_disable_unprepare(sensor->xclk); + regulator_bulk_disable(VGXY61_NUM_SUPPLIES, sensor->supplies); + return 0; +} + +static void vgxy61_fill_sensor_param(struct vgxy61_dev *sensor) +{ + if (sensor->id == VG5761_MODEL_ID) { + sensor->sensor_width = VGX761_WIDTH; + sensor->sensor_height = VGX761_HEIGHT; + sensor->sensor_modes = vgx761_mode_data; + sensor->sensor_modes_nb = ARRAY_SIZE(vgx761_mode_data); + sensor->default_mode = &vgx761_mode_data[VGX761_DEFAULT_MODE]; + sensor->rot_term = VGX761_SHORT_ROT_TERM; + } else if (sensor->id == VG5661_MODEL_ID) { + sensor->sensor_width = VGX661_WIDTH; + sensor->sensor_height = VGX661_HEIGHT; + sensor->sensor_modes = vgx661_mode_data; + sensor->sensor_modes_nb = ARRAY_SIZE(vgx661_mode_data); + sensor->default_mode = &vgx661_mode_data[VGX661_DEFAULT_MODE]; + sensor->rot_term = VGX661_SHORT_ROT_TERM; + } else { + /* Should never happen */ + WARN_ON(true); + } + sensor->current_mode = sensor->default_mode; +} + +static int vgxy61_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct fwnode_handle *handle; + struct vgxy61_dev *sensor; + int ret; + + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); + if (!sensor) + return -ENOMEM; + + sensor->i2c_client = client; + sensor->streaming = false; + sensor->hdr = VGXY61_NO_HDR; + sensor->expo_long = 200; + sensor->expo_short = 0; + sensor->hflip = false; + sensor->vflip = false; + sensor->analog_gain = 0; + sensor->digital_gain = 256; + + handle = fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), + NULL); + if (!handle) { + dev_err(dev, "handle node not found\n"); + return -EINVAL; + } + + ret = vgxy61_tx_from_ep(sensor, handle); + fwnode_handle_put(handle); + if (ret) { + dev_err(dev, "Failed to parse handle %d\n", ret); + return ret; + } + + sensor->xclk = devm_clk_get(dev, NULL); + if (IS_ERR(sensor->xclk)) { + dev_err(dev, "failed to get xclk\n"); + return PTR_ERR(sensor->xclk); + } + sensor->clk_freq = clk_get_rate(sensor->xclk); + if (sensor->clk_freq < 6 * HZ_PER_MHZ || + sensor->clk_freq > 27 * HZ_PER_MHZ) { + dev_err(dev, "Only 6Mhz-27Mhz clock range supported. provide %lu MHz\n", + sensor->clk_freq / HZ_PER_MHZ); + return -EINVAL; + } + sensor->gpios_polarity = of_property_read_bool(dev->of_node, + "invert-gpios-polarity"); + + v4l2_i2c_subdev_init(&sensor->sd, client, &vgxy61_subdev_ops); + sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; + sensor->pad.flags = MEDIA_PAD_FL_SOURCE; + sensor->sd.entity.ops = &vgxy61_subdev_entity_ops; + sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; + + sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset", + GPIOD_OUT_HIGH); + + ret = vgxy61_get_regulators(sensor); + if (ret) { + dev_err(&client->dev, "failed to get regulators %d\n", ret); + return ret; + } + + ret = regulator_bulk_enable(VGXY61_NUM_SUPPLIES, sensor->supplies); + if (ret) { + dev_err(&client->dev, "failed to enable regulators %d\n", ret); + return ret; + } + + ret = vgxy61_power_on(dev); + if (ret) + return ret; + + ret = vgxy61_detect(sensor); + if (ret) { + dev_err(&client->dev, "sensor detect failed %d\n", ret); + return ret; + } + + vgxy61_fill_sensor_param(sensor); + vgxy61_fill_framefmt(sensor, sensor->current_mode, &sensor->fmt, + VGXY61_MEDIA_BUS_FMT_DEF); + + ret = vgxy61_update_hdr(sensor, sensor->hdr); + if (ret) + return ret; + + mutex_init(&sensor->lock); + + ret = vgxy61_init_controls(sensor); + if (ret) { + dev_err(&client->dev, "controls initialization failed %d\n", + ret); + goto error_power_off; + } + + ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad); + if (ret) { + dev_err(&client->dev, "pads init failed %d\n", ret); + goto error_handler_free; + } + + /* Enable runtime PM and turn off the device */ + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + pm_runtime_idle(dev); + + ret = v4l2_async_register_subdev(&sensor->sd); + if (ret) { + dev_err(&client->dev, "async subdev register failed %d\n", ret); + goto error_pm_runtime; + } + + pm_runtime_set_autosuspend_delay(&client->dev, 1000); + pm_runtime_use_autosuspend(&client->dev); + + dev_dbg(&client->dev, "vgxy61 probe successfully\n"); + + return 0; + +error_pm_runtime: + pm_runtime_disable(&client->dev); + pm_runtime_set_suspended(&client->dev); + media_entity_cleanup(&sensor->sd.entity); +error_handler_free: + v4l2_ctrl_handler_free(sensor->sd.ctrl_handler); + mutex_destroy(&sensor->lock); +error_power_off: + vgxy61_power_off(dev); + + return ret; +} + +static int vgxy61_remove(struct i2c_client *client) +{ + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct vgxy61_dev *sensor = to_vgxy61_dev(sd); + + v4l2_async_unregister_subdev(&sensor->sd); + mutex_destroy(&sensor->lock); + media_entity_cleanup(&sensor->sd.entity); + + pm_runtime_disable(&client->dev); + if (!pm_runtime_status_suspended(&client->dev)) + vgxy61_power_off(&client->dev); + pm_runtime_set_suspended(&client->dev); + + return 0; +} + +static const struct of_device_id vgxy61_dt_ids[] = { + { .compatible = "st,st-vgxy61" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, vgxy61_dt_ids); + +static const struct dev_pm_ops vgxy61_pm_ops = { + SET_RUNTIME_PM_OPS(vgxy61_power_off, vgxy61_power_on, NULL) +}; + +static struct i2c_driver vgxy61_i2c_driver = { + .driver = { + .name = "st-vgxy61", + .of_match_table = vgxy61_dt_ids, + .pm = &vgxy61_pm_ops, + }, + .probe_new = vgxy61_probe, + .remove = vgxy61_remove, +}; + +module_i2c_driver(vgxy61_i2c_driver); + +MODULE_AUTHOR("Benjamin Mugnier <benjamin.mugnier@foss.st.com>"); +MODULE_AUTHOR("Mickael Guene <mickael.guene@st.com>"); +MODULE_AUTHOR("Sylvain Petinot <sylvain.petinot@foss.st.com>"); +MODULE_DESCRIPTION("VGXY61 camera subdev driver"); +MODULE_LICENSE("GPL"); -- 2.25.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-09-27 8:37 ` [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor Benjamin Mugnier @ 2022-10-06 19:14 ` Sakari Ailus 2022-10-07 12:24 ` Benjamin MUGNIER 0 siblings, 1 reply; 26+ messages in thread From: Sakari Ailus @ 2022-10-06 19:14 UTC (permalink / raw) To: Benjamin Mugnier Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Benjamin, Thanks for the update. A few more comments below... On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote: > +static const char * const vgxy61_hdr_mode_menu[] = { > + "HDR linearize", > + "HDR substraction", > + "No HDR", > +}; I think more documentation is needed on the HDR modes. What do these mean? For instance, are they something that requires further processing or is the result e.g. a ready HDR image? This should probably make it to driver specific documentation. > + > +static const char * const vgxy61_supply_name[] = { > + "VCORE", > + "VDDIO", > + "VANA", > +}; > + > +#define VGXY61_NUM_SUPPLIES ARRAY_SIZE(vgxy61_supply_name) Please use plain ARRAY_SIZE() instead. ... > +static int vgxy61_poll_reg(struct vgxy61_dev *sensor, u32 reg, u8 poll_val, > + unsigned int timeout_ms) > +{ > + const unsigned int loop_delay_ms = 10; > + int ret, timeout; > + > + timeout = read_poll_timeout(vgxy61_read_reg, ret, > + ((ret < 0) || (ret == poll_val)), > + loop_delay_ms * 1000, timeout_ms * 1000, > + false, sensor, reg); > + if (timeout) > + return timeout; > + > + return 0; "timeout" here is entirely pointless. > +} ... > +static int vgxy61_apply_exposure(struct vgxy61_dev *sensor) > +{ > + int ret = 0; > + > + /* We first set expo to zero to avoid forbidden parameters couple */ > + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, > + 0, &ret); > + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_LONG, > + sensor->expo_long, &ret); > + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, > + sensor->expo_short, &ret); return vgxy61_write_reg(...); > + if (ret) > + return ret; > + > + return 0; > +} ... > +static int vgxy61_init_controls(struct vgxy61_dev *sensor) > +{ > + const struct v4l2_ctrl_ops *ops = &vgxy61_ctrl_ops; > + struct v4l2_ctrl_handler *hdl = &sensor->ctrl_handler; > + const struct vgxy61_mode_info *cur_mode = sensor->current_mode; > + struct v4l2_ctrl *ctrl; > + int ret; > + > + v4l2_ctrl_handler_init(hdl, 16); > + /* We can use our own mutex for the ctrl lock */ > + hdl->lock = &sensor->lock; > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN, 0, 0x1c, 1, > + sensor->analog_gain); > + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN, 0, 0xfff, 1, > + sensor->digital_gain); > + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(vgxy61_test_pattern_menu) - 1, > + 0, 0, vgxy61_test_pattern_menu); > + ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, 0, > + sensor->line_length, 1, > + sensor->line_length - cur_mode->width); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + ctrl = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, > + ARRAY_SIZE(link_freq) - 1, 0, link_freq); > + if (ctrl) > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_HDR_MODE, > + ARRAY_SIZE(vgxy61_hdr_mode_menu) - 1, 0, > + VGXY61_NO_HDR, vgxy61_hdr_mode_menu); > + > + /* > + * Keep a pointer to these controls as we need to update them when > + * setting the format > + */ > + sensor->pixel_rate_ctrl = v4l2_ctrl_new_std(hdl, ops, > + V4L2_CID_PIXEL_RATE, 1, > + INT_MAX, 1, > + get_pixel_rate(sensor)); > + sensor->pixel_rate_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; Also sensor->pixel_rate_ctrl may be NULL here. > + sensor->expo_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, > + sensor->expo_min, > + sensor->expo_max, 1, > + sensor->expo_long); > + sensor->vblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, > + sensor->vblank_min, > + 0xffff - cur_mode->crop.height, > + 1, sensor->vblank); > + sensor->vflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, > + 0, 1, 1, sensor->vflip); > + sensor->hflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, > + 0, 1, 1, sensor->hflip); > + > + if (hdl->error) { > + ret = hdl->error; > + goto free_ctrls; > + } > + > + sensor->sd.ctrl_handler = hdl; > + return 0; > + > +free_ctrls: > + v4l2_ctrl_handler_free(hdl); > + return ret; > +} ... > +static int vgxy61_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct fwnode_handle *handle; > + struct vgxy61_dev *sensor; > + int ret; > + > + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); > + if (!sensor) > + return -ENOMEM; > + > + sensor->i2c_client = client; > + sensor->streaming = false; > + sensor->hdr = VGXY61_NO_HDR; > + sensor->expo_long = 200; > + sensor->expo_short = 0; > + sensor->hflip = false; > + sensor->vflip = false; > + sensor->analog_gain = 0; > + sensor->digital_gain = 256; > + > + handle = fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), > + NULL); handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0); > + if (!handle) { > + dev_err(dev, "handle node not found\n"); > + return -EINVAL; > + } > + > + ret = vgxy61_tx_from_ep(sensor, handle); > + fwnode_handle_put(handle); > + if (ret) { > + dev_err(dev, "Failed to parse handle %d\n", ret); > + return ret; > + } > + > + sensor->xclk = devm_clk_get(dev, NULL); > + if (IS_ERR(sensor->xclk)) { > + dev_err(dev, "failed to get xclk\n"); > + return PTR_ERR(sensor->xclk); > + } > + sensor->clk_freq = clk_get_rate(sensor->xclk); > + if (sensor->clk_freq < 6 * HZ_PER_MHZ || > + sensor->clk_freq > 27 * HZ_PER_MHZ) { > + dev_err(dev, "Only 6Mhz-27Mhz clock range supported. provide %lu MHz\n", > + sensor->clk_freq / HZ_PER_MHZ); > + return -EINVAL; > + } > + sensor->gpios_polarity = of_property_read_bool(dev->of_node, > + "invert-gpios-polarity"); I thought we did discuss dropping support for sensor synchronisation in this version? > + > + v4l2_i2c_subdev_init(&sensor->sd, client, &vgxy61_subdev_ops); > + sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + sensor->pad.flags = MEDIA_PAD_FL_SOURCE; > + sensor->sd.entity.ops = &vgxy61_subdev_entity_ops; > + sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > + > + sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset", > + GPIOD_OUT_HIGH); > + > + ret = vgxy61_get_regulators(sensor); > + if (ret) { > + dev_err(&client->dev, "failed to get regulators %d\n", ret); > + return ret; > + } > + > + ret = regulator_bulk_enable(VGXY61_NUM_SUPPLIES, sensor->supplies); > + if (ret) { > + dev_err(&client->dev, "failed to enable regulators %d\n", ret); > + return ret; > + } > + > + ret = vgxy61_power_on(dev); > + if (ret) > + return ret; > + > + ret = vgxy61_detect(sensor); > + if (ret) { > + dev_err(&client->dev, "sensor detect failed %d\n", ret); > + return ret; > + } > + > + vgxy61_fill_sensor_param(sensor); > + vgxy61_fill_framefmt(sensor, sensor->current_mode, &sensor->fmt, > + VGXY61_MEDIA_BUS_FMT_DEF); > + > + ret = vgxy61_update_hdr(sensor, sensor->hdr); > + if (ret) > + return ret; > + > + mutex_init(&sensor->lock); > + > + ret = vgxy61_init_controls(sensor); > + if (ret) { > + dev_err(&client->dev, "controls initialization failed %d\n", > + ret); > + goto error_power_off; > + } > + > + ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad); > + if (ret) { > + dev_err(&client->dev, "pads init failed %d\n", ret); > + goto error_handler_free; > + } > + > + /* Enable runtime PM and turn off the device */ > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + pm_runtime_idle(dev); > + > + ret = v4l2_async_register_subdev(&sensor->sd); > + if (ret) { > + dev_err(&client->dev, "async subdev register failed %d\n", ret); > + goto error_pm_runtime; > + } > + > + pm_runtime_set_autosuspend_delay(&client->dev, 1000); > + pm_runtime_use_autosuspend(&client->dev); > + > + dev_dbg(&client->dev, "vgxy61 probe successfully\n"); > + > + return 0; > + > +error_pm_runtime: > + pm_runtime_disable(&client->dev); > + pm_runtime_set_suspended(&client->dev); > + media_entity_cleanup(&sensor->sd.entity); > +error_handler_free: > + v4l2_ctrl_handler_free(sensor->sd.ctrl_handler); > + mutex_destroy(&sensor->lock); > +error_power_off: > + vgxy61_power_off(dev); > + > + return ret; > +} -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-06 19:14 ` Sakari Ailus @ 2022-10-07 12:24 ` Benjamin MUGNIER 2022-10-07 12:32 ` Laurent Pinchart 2022-10-10 8:29 ` Sakari Ailus 0 siblings, 2 replies; 26+ messages in thread From: Benjamin MUGNIER @ 2022-10-07 12:24 UTC (permalink / raw) To: Sakari Ailus Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Sakari, Thank you for your review. Please consider everything not commented as queued for v7. On 10/6/22 21:14, Sakari Ailus wrote: > Hi Benjamin, > > Thanks for the update. A few more comments below... > > On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote: >> +static const char * const vgxy61_hdr_mode_menu[] = { >> + "HDR linearize", >> + "HDR substraction", >> + "No HDR", >> +}; > > I think more documentation is needed on the HDR modes. What do these mean? > For instance, are they something that requires further processing or is the > result e.g. a ready HDR image? > > This should probably make it to driver specific documentation. > Sure, in fact I did something like this in v3: https://lore.kernel.org/linux-media/20220512074037.3829926-4-benjamin.mugnier@foss.st.com/ Since I standardized the control I was unsure what to do with this documentation, and dropped it. I will add back the Documentation/userspace-api/media/drivers/st-vgxy61.rst file from this commit to the patchset, while changing the control name to the new one. >> + >> +static const char * const vgxy61_supply_name[] = { >> + "VCORE", >> + "VDDIO", >> + "VANA", >> +}; >> + >> +#define VGXY61_NUM_SUPPLIES ARRAY_SIZE(vgxy61_supply_name) > > Please use plain ARRAY_SIZE() instead. > > ... > >> +static int vgxy61_poll_reg(struct vgxy61_dev *sensor, u32 reg, u8 poll_val, >> + unsigned int timeout_ms) >> +{ >> + const unsigned int loop_delay_ms = 10; >> + int ret, timeout; >> + >> + timeout = read_poll_timeout(vgxy61_read_reg, ret, >> + ((ret < 0) || (ret == poll_val)), >> + loop_delay_ms * 1000, timeout_ms * 1000, >> + false, sensor, reg); >> + if (timeout) >> + return timeout; >> + >> + return 0; > > "timeout" here is entirely pointless. > >> +} > > ... > >> +static int vgxy61_apply_exposure(struct vgxy61_dev *sensor) >> +{ >> + int ret = 0; >> + >> + /* We first set expo to zero to avoid forbidden parameters couple */ >> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, >> + 0, &ret); >> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_LONG, >> + sensor->expo_long, &ret); >> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, >> + sensor->expo_short, &ret); > > return vgxy61_write_reg(...); > >> + if (ret) >> + return ret; >> + >> + return 0; >> +} > > ... > >> +static int vgxy61_init_controls(struct vgxy61_dev *sensor) >> +{ >> + const struct v4l2_ctrl_ops *ops = &vgxy61_ctrl_ops; >> + struct v4l2_ctrl_handler *hdl = &sensor->ctrl_handler; >> + const struct vgxy61_mode_info *cur_mode = sensor->current_mode; >> + struct v4l2_ctrl *ctrl; >> + int ret; >> + >> + v4l2_ctrl_handler_init(hdl, 16); >> + /* We can use our own mutex for the ctrl lock */ >> + hdl->lock = &sensor->lock; >> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN, 0, 0x1c, 1, >> + sensor->analog_gain); >> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN, 0, 0xfff, 1, >> + sensor->digital_gain); >> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN, >> + ARRAY_SIZE(vgxy61_test_pattern_menu) - 1, >> + 0, 0, vgxy61_test_pattern_menu); >> + ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, 0, >> + sensor->line_length, 1, >> + sensor->line_length - cur_mode->width); >> + if (ctrl) >> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >> + ctrl = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, >> + ARRAY_SIZE(link_freq) - 1, 0, link_freq); >> + if (ctrl) >> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_HDR_MODE, >> + ARRAY_SIZE(vgxy61_hdr_mode_menu) - 1, 0, >> + VGXY61_NO_HDR, vgxy61_hdr_mode_menu); >> + >> + /* >> + * Keep a pointer to these controls as we need to update them when >> + * setting the format >> + */ >> + sensor->pixel_rate_ctrl = v4l2_ctrl_new_std(hdl, ops, >> + V4L2_CID_PIXEL_RATE, 1, >> + INT_MAX, 1, >> + get_pixel_rate(sensor)); >> + sensor->pixel_rate_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > Also sensor->pixel_rate_ctrl may be NULL here. > >> + sensor->expo_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, >> + sensor->expo_min, >> + sensor->expo_max, 1, >> + sensor->expo_long); >> + sensor->vblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, >> + sensor->vblank_min, >> + 0xffff - cur_mode->crop.height, >> + 1, sensor->vblank); >> + sensor->vflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, >> + 0, 1, 1, sensor->vflip); >> + sensor->hflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, >> + 0, 1, 1, sensor->hflip); >> + >> + if (hdl->error) { >> + ret = hdl->error; >> + goto free_ctrls; >> + } >> + >> + sensor->sd.ctrl_handler = hdl; >> + return 0; >> + >> +free_ctrls: >> + v4l2_ctrl_handler_free(hdl); >> + return ret; >> +} > > ... > >> +static int vgxy61_probe(struct i2c_client *client) >> +{ >> + struct device *dev = &client->dev; >> + struct fwnode_handle *handle; >> + struct vgxy61_dev *sensor; >> + int ret; >> + >> + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); >> + if (!sensor) >> + return -ENOMEM; >> + >> + sensor->i2c_client = client; >> + sensor->streaming = false; >> + sensor->hdr = VGXY61_NO_HDR; >> + sensor->expo_long = 200; >> + sensor->expo_short = 0; >> + sensor->hflip = false; >> + sensor->vflip = false; >> + sensor->analog_gain = 0; >> + sensor->digital_gain = 256; >> + >> + handle = fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), >> + NULL); > > handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0); > >> + if (!handle) { >> + dev_err(dev, "handle node not found\n"); >> + return -EINVAL; >> + } >> + >> + ret = vgxy61_tx_from_ep(sensor, handle); >> + fwnode_handle_put(handle); >> + if (ret) { >> + dev_err(dev, "Failed to parse handle %d\n", ret); >> + return ret; >> + } >> + >> + sensor->xclk = devm_clk_get(dev, NULL); >> + if (IS_ERR(sensor->xclk)) { >> + dev_err(dev, "failed to get xclk\n"); >> + return PTR_ERR(sensor->xclk); >> + } >> + sensor->clk_freq = clk_get_rate(sensor->xclk); >> + if (sensor->clk_freq < 6 * HZ_PER_MHZ || >> + sensor->clk_freq > 27 * HZ_PER_MHZ) { >> + dev_err(dev, "Only 6Mhz-27Mhz clock range supported. provide %lu MHz\n", >> + sensor->clk_freq / HZ_PER_MHZ); >> + return -EINVAL; >> + } >> + sensor->gpios_polarity = of_property_read_bool(dev->of_node, >> + "invert-gpios-polarity"); > > I thought we did discuss dropping support for sensor synchronisation in > this version? > This properties affects strobing lights gpios polarities as you can see in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing gpios are inverted. This has nothing to do with the sensor synchronization. Now I realize this is poorly named, and I even forgot to document it in the device tree bindings file. I apologize. I would like to rename it to 'st,strobe-polarity' since this is vendor specific and to better reflect that it affects strobing gpios. I'll make this change for v7 and document this in the bindings file too. Tell me if there is any issues with that. Regards, Benjamin >> + >> + v4l2_i2c_subdev_init(&sensor->sd, client, &vgxy61_subdev_ops); >> + sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >> + sensor->pad.flags = MEDIA_PAD_FL_SOURCE; >> + sensor->sd.entity.ops = &vgxy61_subdev_entity_ops; >> + sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; >> + >> + sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset", >> + GPIOD_OUT_HIGH); >> + >> + ret = vgxy61_get_regulators(sensor); >> + if (ret) { >> + dev_err(&client->dev, "failed to get regulators %d\n", ret); >> + return ret; >> + } >> + >> + ret = regulator_bulk_enable(VGXY61_NUM_SUPPLIES, sensor->supplies); >> + if (ret) { >> + dev_err(&client->dev, "failed to enable regulators %d\n", ret); >> + return ret; >> + } >> + >> + ret = vgxy61_power_on(dev); >> + if (ret) >> + return ret; >> + >> + ret = vgxy61_detect(sensor); >> + if (ret) { >> + dev_err(&client->dev, "sensor detect failed %d\n", ret); >> + return ret; >> + } >> + >> + vgxy61_fill_sensor_param(sensor); >> + vgxy61_fill_framefmt(sensor, sensor->current_mode, &sensor->fmt, >> + VGXY61_MEDIA_BUS_FMT_DEF); >> + >> + ret = vgxy61_update_hdr(sensor, sensor->hdr); >> + if (ret) >> + return ret; >> + >> + mutex_init(&sensor->lock); >> + >> + ret = vgxy61_init_controls(sensor); >> + if (ret) { >> + dev_err(&client->dev, "controls initialization failed %d\n", >> + ret); >> + goto error_power_off; >> + } >> + >> + ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad); >> + if (ret) { >> + dev_err(&client->dev, "pads init failed %d\n", ret); >> + goto error_handler_free; >> + } >> + >> + /* Enable runtime PM and turn off the device */ >> + pm_runtime_set_active(dev); >> + pm_runtime_enable(dev); >> + pm_runtime_idle(dev); >> + >> + ret = v4l2_async_register_subdev(&sensor->sd); >> + if (ret) { >> + dev_err(&client->dev, "async subdev register failed %d\n", ret); >> + goto error_pm_runtime; >> + } >> + >> + pm_runtime_set_autosuspend_delay(&client->dev, 1000); >> + pm_runtime_use_autosuspend(&client->dev); >> + >> + dev_dbg(&client->dev, "vgxy61 probe successfully\n"); >> + >> + return 0; >> + >> +error_pm_runtime: >> + pm_runtime_disable(&client->dev); >> + pm_runtime_set_suspended(&client->dev); >> + media_entity_cleanup(&sensor->sd.entity); >> +error_handler_free: >> + v4l2_ctrl_handler_free(sensor->sd.ctrl_handler); >> + mutex_destroy(&sensor->lock); >> +error_power_off: >> + vgxy61_power_off(dev); >> + >> + return ret; >> +} > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-07 12:24 ` Benjamin MUGNIER @ 2022-10-07 12:32 ` Laurent Pinchart 2022-10-07 12:38 ` Benjamin MUGNIER 2022-10-10 8:29 ` Sakari Ailus 1 sibling, 1 reply; 26+ messages in thread From: Laurent Pinchart @ 2022-10-07 12:32 UTC (permalink / raw) To: Benjamin MUGNIER Cc: Sakari Ailus, linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, kieran.bingham, nicolas, hverkuil Hi Benjamin, On Fri, Oct 07, 2022 at 02:24:16PM +0200, Benjamin MUGNIER wrote: > Hi Sakari, > > Thank you for your review. > Please consider everything not commented as queued for v7. > > On 10/6/22 21:14, Sakari Ailus wrote: > > Hi Benjamin, > > > > Thanks for the update. A few more comments below... > > > > On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote: > >> +static const char * const vgxy61_hdr_mode_menu[] = { > >> + "HDR linearize", > >> + "HDR substraction", > >> + "No HDR", > >> +}; > > > > I think more documentation is needed on the HDR modes. What do these mean? > > For instance, are they something that requires further processing or is the > > result e.g. a ready HDR image? > > > > This should probably make it to driver specific documentation. > > Sure, in fact I did something like this in v3: > https://lore.kernel.org/linux-media/20220512074037.3829926-4-benjamin.mugnier@foss.st.com/ > > Since I standardized the control I was unsure what to do with this > documentation, and dropped it. > > I will add back the > Documentation/userspace-api/media/drivers/st-vgxy61.rst file from this > commit to the patchset, while changing the control name to the new > one. The documentation there is about modes for HDR merge on the sensor side. Can the sensor also output the unmerged images, for instance line-interleaved ? > >> + > >> +static const char * const vgxy61_supply_name[] = { > >> + "VCORE", > >> + "VDDIO", > >> + "VANA", > >> +}; > >> + > >> +#define VGXY61_NUM_SUPPLIES ARRAY_SIZE(vgxy61_supply_name) > > > > Please use plain ARRAY_SIZE() instead. > > > > ... > > > >> +static int vgxy61_poll_reg(struct vgxy61_dev *sensor, u32 reg, u8 poll_val, > >> + unsigned int timeout_ms) > >> +{ > >> + const unsigned int loop_delay_ms = 10; > >> + int ret, timeout; > >> + > >> + timeout = read_poll_timeout(vgxy61_read_reg, ret, > >> + ((ret < 0) || (ret == poll_val)), > >> + loop_delay_ms * 1000, timeout_ms * 1000, > >> + false, sensor, reg); > >> + if (timeout) > >> + return timeout; > >> + > >> + return 0; > > > > "timeout" here is entirely pointless. > > > >> +} > > > > ... > > > >> +static int vgxy61_apply_exposure(struct vgxy61_dev *sensor) > >> +{ > >> + int ret = 0; > >> + > >> + /* We first set expo to zero to avoid forbidden parameters couple */ > >> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, > >> + 0, &ret); > >> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_LONG, > >> + sensor->expo_long, &ret); > >> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, > >> + sensor->expo_short, &ret); > > > > return vgxy61_write_reg(...); > > > >> + if (ret) > >> + return ret; > >> + > >> + return 0; > >> +} > > > > ... > > > >> +static int vgxy61_init_controls(struct vgxy61_dev *sensor) > >> +{ > >> + const struct v4l2_ctrl_ops *ops = &vgxy61_ctrl_ops; > >> + struct v4l2_ctrl_handler *hdl = &sensor->ctrl_handler; > >> + const struct vgxy61_mode_info *cur_mode = sensor->current_mode; > >> + struct v4l2_ctrl *ctrl; > >> + int ret; > >> + > >> + v4l2_ctrl_handler_init(hdl, 16); > >> + /* We can use our own mutex for the ctrl lock */ > >> + hdl->lock = &sensor->lock; > >> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN, 0, 0x1c, 1, > >> + sensor->analog_gain); > >> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN, 0, 0xfff, 1, > >> + sensor->digital_gain); > >> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN, > >> + ARRAY_SIZE(vgxy61_test_pattern_menu) - 1, > >> + 0, 0, vgxy61_test_pattern_menu); > >> + ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, 0, > >> + sensor->line_length, 1, > >> + sensor->line_length - cur_mode->width); > >> + if (ctrl) > >> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >> + ctrl = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, > >> + ARRAY_SIZE(link_freq) - 1, 0, link_freq); > >> + if (ctrl) > >> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_HDR_MODE, > >> + ARRAY_SIZE(vgxy61_hdr_mode_menu) - 1, 0, > >> + VGXY61_NO_HDR, vgxy61_hdr_mode_menu); > >> + > >> + /* > >> + * Keep a pointer to these controls as we need to update them when > >> + * setting the format > >> + */ > >> + sensor->pixel_rate_ctrl = v4l2_ctrl_new_std(hdl, ops, > >> + V4L2_CID_PIXEL_RATE, 1, > >> + INT_MAX, 1, > >> + get_pixel_rate(sensor)); > >> + sensor->pixel_rate_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > Also sensor->pixel_rate_ctrl may be NULL here. > > > >> + sensor->expo_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, > >> + sensor->expo_min, > >> + sensor->expo_max, 1, > >> + sensor->expo_long); > >> + sensor->vblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, > >> + sensor->vblank_min, > >> + 0xffff - cur_mode->crop.height, > >> + 1, sensor->vblank); > >> + sensor->vflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, > >> + 0, 1, 1, sensor->vflip); > >> + sensor->hflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, > >> + 0, 1, 1, sensor->hflip); > >> + > >> + if (hdl->error) { > >> + ret = hdl->error; > >> + goto free_ctrls; > >> + } > >> + > >> + sensor->sd.ctrl_handler = hdl; > >> + return 0; > >> + > >> +free_ctrls: > >> + v4l2_ctrl_handler_free(hdl); > >> + return ret; > >> +} > > > > ... > > > >> +static int vgxy61_probe(struct i2c_client *client) > >> +{ > >> + struct device *dev = &client->dev; > >> + struct fwnode_handle *handle; > >> + struct vgxy61_dev *sensor; > >> + int ret; > >> + > >> + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); > >> + if (!sensor) > >> + return -ENOMEM; > >> + > >> + sensor->i2c_client = client; > >> + sensor->streaming = false; > >> + sensor->hdr = VGXY61_NO_HDR; > >> + sensor->expo_long = 200; > >> + sensor->expo_short = 0; > >> + sensor->hflip = false; > >> + sensor->vflip = false; > >> + sensor->analog_gain = 0; > >> + sensor->digital_gain = 256; > >> + > >> + handle = fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), > >> + NULL); > > > > handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0); > > > >> + if (!handle) { > >> + dev_err(dev, "handle node not found\n"); > >> + return -EINVAL; > >> + } > >> + > >> + ret = vgxy61_tx_from_ep(sensor, handle); > >> + fwnode_handle_put(handle); > >> + if (ret) { > >> + dev_err(dev, "Failed to parse handle %d\n", ret); > >> + return ret; > >> + } > >> + > >> + sensor->xclk = devm_clk_get(dev, NULL); > >> + if (IS_ERR(sensor->xclk)) { > >> + dev_err(dev, "failed to get xclk\n"); > >> + return PTR_ERR(sensor->xclk); > >> + } > >> + sensor->clk_freq = clk_get_rate(sensor->xclk); > >> + if (sensor->clk_freq < 6 * HZ_PER_MHZ || > >> + sensor->clk_freq > 27 * HZ_PER_MHZ) { > >> + dev_err(dev, "Only 6Mhz-27Mhz clock range supported. provide %lu MHz\n", > >> + sensor->clk_freq / HZ_PER_MHZ); > >> + return -EINVAL; > >> + } > >> + sensor->gpios_polarity = of_property_read_bool(dev->of_node, > >> + "invert-gpios-polarity"); > > > > I thought we did discuss dropping support for sensor synchronisation in > > this version? > > This properties affects strobing lights gpios polarities as you can > see in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing > gpios are inverted. This has nothing to do with the sensor > synchronization. > > Now I realize this is poorly named, and I even forgot to document it > in the device tree bindings file. I apologize. > > I would like to rename it to 'st,strobe-polarity' since this is vendor > specific and to better reflect that it affects strobing gpios. > I'll make this change for v7 and document this in the bindings file > too. Tell me if there is any issues with that. > > >> + > >> + v4l2_i2c_subdev_init(&sensor->sd, client, &vgxy61_subdev_ops); > >> + sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > >> + sensor->pad.flags = MEDIA_PAD_FL_SOURCE; > >> + sensor->sd.entity.ops = &vgxy61_subdev_entity_ops; > >> + sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > >> + > >> + sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset", > >> + GPIOD_OUT_HIGH); > >> + > >> + ret = vgxy61_get_regulators(sensor); > >> + if (ret) { > >> + dev_err(&client->dev, "failed to get regulators %d\n", ret); > >> + return ret; > >> + } > >> + > >> + ret = regulator_bulk_enable(VGXY61_NUM_SUPPLIES, sensor->supplies); > >> + if (ret) { > >> + dev_err(&client->dev, "failed to enable regulators %d\n", ret); > >> + return ret; > >> + } > >> + > >> + ret = vgxy61_power_on(dev); > >> + if (ret) > >> + return ret; > >> + > >> + ret = vgxy61_detect(sensor); > >> + if (ret) { > >> + dev_err(&client->dev, "sensor detect failed %d\n", ret); > >> + return ret; > >> + } > >> + > >> + vgxy61_fill_sensor_param(sensor); > >> + vgxy61_fill_framefmt(sensor, sensor->current_mode, &sensor->fmt, > >> + VGXY61_MEDIA_BUS_FMT_DEF); > >> + > >> + ret = vgxy61_update_hdr(sensor, sensor->hdr); > >> + if (ret) > >> + return ret; > >> + > >> + mutex_init(&sensor->lock); > >> + > >> + ret = vgxy61_init_controls(sensor); > >> + if (ret) { > >> + dev_err(&client->dev, "controls initialization failed %d\n", > >> + ret); > >> + goto error_power_off; > >> + } > >> + > >> + ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad); > >> + if (ret) { > >> + dev_err(&client->dev, "pads init failed %d\n", ret); > >> + goto error_handler_free; > >> + } > >> + > >> + /* Enable runtime PM and turn off the device */ > >> + pm_runtime_set_active(dev); > >> + pm_runtime_enable(dev); > >> + pm_runtime_idle(dev); > >> + > >> + ret = v4l2_async_register_subdev(&sensor->sd); > >> + if (ret) { > >> + dev_err(&client->dev, "async subdev register failed %d\n", ret); > >> + goto error_pm_runtime; > >> + } > >> + > >> + pm_runtime_set_autosuspend_delay(&client->dev, 1000); > >> + pm_runtime_use_autosuspend(&client->dev); > >> + > >> + dev_dbg(&client->dev, "vgxy61 probe successfully\n"); > >> + > >> + return 0; > >> + > >> +error_pm_runtime: > >> + pm_runtime_disable(&client->dev); > >> + pm_runtime_set_suspended(&client->dev); > >> + media_entity_cleanup(&sensor->sd.entity); > >> +error_handler_free: > >> + v4l2_ctrl_handler_free(sensor->sd.ctrl_handler); > >> + mutex_destroy(&sensor->lock); > >> +error_power_off: > >> + vgxy61_power_off(dev); > >> + > >> + return ret; > >> +} -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-07 12:32 ` Laurent Pinchart @ 2022-10-07 12:38 ` Benjamin MUGNIER 2022-10-07 13:19 ` Laurent Pinchart 0 siblings, 1 reply; 26+ messages in thread From: Benjamin MUGNIER @ 2022-10-07 12:38 UTC (permalink / raw) To: Laurent Pinchart Cc: Sakari Ailus, linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, kieran.bingham, nicolas, hverkuil Hi Laurent, On 10/7/22 14:32, Laurent Pinchart wrote: > Hi Benjamin, > > On Fri, Oct 07, 2022 at 02:24:16PM +0200, Benjamin MUGNIER wrote: >> Hi Sakari, >> >> Thank you for your review. >> Please consider everything not commented as queued for v7. >> >> On 10/6/22 21:14, Sakari Ailus wrote: >>> Hi Benjamin, >>> >>> Thanks for the update. A few more comments below... >>> >>> On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote: >>>> +static const char * const vgxy61_hdr_mode_menu[] = { >>>> + "HDR linearize", >>>> + "HDR substraction", >>>> + "No HDR", >>>> +}; >>> >>> I think more documentation is needed on the HDR modes. What do these mean? >>> For instance, are they something that requires further processing or is the >>> result e.g. a ready HDR image? >>> >>> This should probably make it to driver specific documentation. >> >> Sure, in fact I did something like this in v3: >> https://lore.kernel.org/linux-media/20220512074037.3829926-4-benjamin.mugnier@foss.st.com/ >> >> Since I standardized the control I was unsure what to do with this >> documentation, and dropped it. >> >> I will add back the >> Documentation/userspace-api/media/drivers/st-vgxy61.rst file from this >> commit to the patchset, while changing the control name to the new >> one. > > The documentation there is about modes for HDR merge on the sensor side. > Can the sensor also output the unmerged images, for instance > line-interleaved ? > The sensor can not output unmerged images. It can only output a single image and frame merging as to be done sensor side. >>>> + >>>> +static const char * const vgxy61_supply_name[] = { >>>> + "VCORE", >>>> + "VDDIO", >>>> + "VANA", >>>> +}; >>>> + >>>> +#define VGXY61_NUM_SUPPLIES ARRAY_SIZE(vgxy61_supply_name) >>> >>> Please use plain ARRAY_SIZE() instead. >>> >>> ... >>> >>>> +static int vgxy61_poll_reg(struct vgxy61_dev *sensor, u32 reg, u8 poll_val, >>>> + unsigned int timeout_ms) >>>> +{ >>>> + const unsigned int loop_delay_ms = 10; >>>> + int ret, timeout; >>>> + >>>> + timeout = read_poll_timeout(vgxy61_read_reg, ret, >>>> + ((ret < 0) || (ret == poll_val)), >>>> + loop_delay_ms * 1000, timeout_ms * 1000, >>>> + false, sensor, reg); >>>> + if (timeout) >>>> + return timeout; >>>> + >>>> + return 0; >>> >>> "timeout" here is entirely pointless. >>> >>>> +} >>> >>> ... >>> >>>> +static int vgxy61_apply_exposure(struct vgxy61_dev *sensor) >>>> +{ >>>> + int ret = 0; >>>> + >>>> + /* We first set expo to zero to avoid forbidden parameters couple */ >>>> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, >>>> + 0, &ret); >>>> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_LONG, >>>> + sensor->expo_long, &ret); >>>> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, >>>> + sensor->expo_short, &ret); >>> >>> return vgxy61_write_reg(...); >>> >>>> + if (ret) >>>> + return ret; >>>> + >>>> + return 0; >>>> +} >>> >>> ... >>> >>>> +static int vgxy61_init_controls(struct vgxy61_dev *sensor) >>>> +{ >>>> + const struct v4l2_ctrl_ops *ops = &vgxy61_ctrl_ops; >>>> + struct v4l2_ctrl_handler *hdl = &sensor->ctrl_handler; >>>> + const struct vgxy61_mode_info *cur_mode = sensor->current_mode; >>>> + struct v4l2_ctrl *ctrl; >>>> + int ret; >>>> + >>>> + v4l2_ctrl_handler_init(hdl, 16); >>>> + /* We can use our own mutex for the ctrl lock */ >>>> + hdl->lock = &sensor->lock; >>>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN, 0, 0x1c, 1, >>>> + sensor->analog_gain); >>>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN, 0, 0xfff, 1, >>>> + sensor->digital_gain); >>>> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN, >>>> + ARRAY_SIZE(vgxy61_test_pattern_menu) - 1, >>>> + 0, 0, vgxy61_test_pattern_menu); >>>> + ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, 0, >>>> + sensor->line_length, 1, >>>> + sensor->line_length - cur_mode->width); >>>> + if (ctrl) >>>> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>> + ctrl = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, >>>> + ARRAY_SIZE(link_freq) - 1, 0, link_freq); >>>> + if (ctrl) >>>> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_HDR_MODE, >>>> + ARRAY_SIZE(vgxy61_hdr_mode_menu) - 1, 0, >>>> + VGXY61_NO_HDR, vgxy61_hdr_mode_menu); >>>> + >>>> + /* >>>> + * Keep a pointer to these controls as we need to update them when >>>> + * setting the format >>>> + */ >>>> + sensor->pixel_rate_ctrl = v4l2_ctrl_new_std(hdl, ops, >>>> + V4L2_CID_PIXEL_RATE, 1, >>>> + INT_MAX, 1, >>>> + get_pixel_rate(sensor)); >>>> + sensor->pixel_rate_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>> >>> Also sensor->pixel_rate_ctrl may be NULL here. >>> >>>> + sensor->expo_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, >>>> + sensor->expo_min, >>>> + sensor->expo_max, 1, >>>> + sensor->expo_long); >>>> + sensor->vblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, >>>> + sensor->vblank_min, >>>> + 0xffff - cur_mode->crop.height, >>>> + 1, sensor->vblank); >>>> + sensor->vflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, >>>> + 0, 1, 1, sensor->vflip); >>>> + sensor->hflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, >>>> + 0, 1, 1, sensor->hflip); >>>> + >>>> + if (hdl->error) { >>>> + ret = hdl->error; >>>> + goto free_ctrls; >>>> + } >>>> + >>>> + sensor->sd.ctrl_handler = hdl; >>>> + return 0; >>>> + >>>> +free_ctrls: >>>> + v4l2_ctrl_handler_free(hdl); >>>> + return ret; >>>> +} >>> >>> ... >>> >>>> +static int vgxy61_probe(struct i2c_client *client) >>>> +{ >>>> + struct device *dev = &client->dev; >>>> + struct fwnode_handle *handle; >>>> + struct vgxy61_dev *sensor; >>>> + int ret; >>>> + >>>> + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); >>>> + if (!sensor) >>>> + return -ENOMEM; >>>> + >>>> + sensor->i2c_client = client; >>>> + sensor->streaming = false; >>>> + sensor->hdr = VGXY61_NO_HDR; >>>> + sensor->expo_long = 200; >>>> + sensor->expo_short = 0; >>>> + sensor->hflip = false; >>>> + sensor->vflip = false; >>>> + sensor->analog_gain = 0; >>>> + sensor->digital_gain = 256; >>>> + >>>> + handle = fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), >>>> + NULL); >>> >>> handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0); >>> >>>> + if (!handle) { >>>> + dev_err(dev, "handle node not found\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + ret = vgxy61_tx_from_ep(sensor, handle); >>>> + fwnode_handle_put(handle); >>>> + if (ret) { >>>> + dev_err(dev, "Failed to parse handle %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + sensor->xclk = devm_clk_get(dev, NULL); >>>> + if (IS_ERR(sensor->xclk)) { >>>> + dev_err(dev, "failed to get xclk\n"); >>>> + return PTR_ERR(sensor->xclk); >>>> + } >>>> + sensor->clk_freq = clk_get_rate(sensor->xclk); >>>> + if (sensor->clk_freq < 6 * HZ_PER_MHZ || >>>> + sensor->clk_freq > 27 * HZ_PER_MHZ) { >>>> + dev_err(dev, "Only 6Mhz-27Mhz clock range supported. provide %lu MHz\n", >>>> + sensor->clk_freq / HZ_PER_MHZ); >>>> + return -EINVAL; >>>> + } >>>> + sensor->gpios_polarity = of_property_read_bool(dev->of_node, >>>> + "invert-gpios-polarity"); >>> >>> I thought we did discuss dropping support for sensor synchronisation in >>> this version? >> >> This properties affects strobing lights gpios polarities as you can >> see in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing >> gpios are inverted. This has nothing to do with the sensor >> synchronization. >> >> Now I realize this is poorly named, and I even forgot to document it >> in the device tree bindings file. I apologize. >> >> I would like to rename it to 'st,strobe-polarity' since this is vendor >> specific and to better reflect that it affects strobing gpios. >> I'll make this change for v7 and document this in the bindings file >> too. Tell me if there is any issues with that. >> >>>> + >>>> + v4l2_i2c_subdev_init(&sensor->sd, client, &vgxy61_subdev_ops); >>>> + sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >>>> + sensor->pad.flags = MEDIA_PAD_FL_SOURCE; >>>> + sensor->sd.entity.ops = &vgxy61_subdev_entity_ops; >>>> + sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; >>>> + >>>> + sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset", >>>> + GPIOD_OUT_HIGH); >>>> + >>>> + ret = vgxy61_get_regulators(sensor); >>>> + if (ret) { >>>> + dev_err(&client->dev, "failed to get regulators %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + ret = regulator_bulk_enable(VGXY61_NUM_SUPPLIES, sensor->supplies); >>>> + if (ret) { >>>> + dev_err(&client->dev, "failed to enable regulators %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + ret = vgxy61_power_on(dev); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = vgxy61_detect(sensor); >>>> + if (ret) { >>>> + dev_err(&client->dev, "sensor detect failed %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + vgxy61_fill_sensor_param(sensor); >>>> + vgxy61_fill_framefmt(sensor, sensor->current_mode, &sensor->fmt, >>>> + VGXY61_MEDIA_BUS_FMT_DEF); >>>> + >>>> + ret = vgxy61_update_hdr(sensor, sensor->hdr); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + mutex_init(&sensor->lock); >>>> + >>>> + ret = vgxy61_init_controls(sensor); >>>> + if (ret) { >>>> + dev_err(&client->dev, "controls initialization failed %d\n", >>>> + ret); >>>> + goto error_power_off; >>>> + } >>>> + >>>> + ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad); >>>> + if (ret) { >>>> + dev_err(&client->dev, "pads init failed %d\n", ret); >>>> + goto error_handler_free; >>>> + } >>>> + >>>> + /* Enable runtime PM and turn off the device */ >>>> + pm_runtime_set_active(dev); >>>> + pm_runtime_enable(dev); >>>> + pm_runtime_idle(dev); >>>> + >>>> + ret = v4l2_async_register_subdev(&sensor->sd); >>>> + if (ret) { >>>> + dev_err(&client->dev, "async subdev register failed %d\n", ret); >>>> + goto error_pm_runtime; >>>> + } >>>> + >>>> + pm_runtime_set_autosuspend_delay(&client->dev, 1000); >>>> + pm_runtime_use_autosuspend(&client->dev); >>>> + >>>> + dev_dbg(&client->dev, "vgxy61 probe successfully\n"); >>>> + >>>> + return 0; >>>> + >>>> +error_pm_runtime: >>>> + pm_runtime_disable(&client->dev); >>>> + pm_runtime_set_suspended(&client->dev); >>>> + media_entity_cleanup(&sensor->sd.entity); >>>> +error_handler_free: >>>> + v4l2_ctrl_handler_free(sensor->sd.ctrl_handler); >>>> + mutex_destroy(&sensor->lock); >>>> +error_power_off: >>>> + vgxy61_power_off(dev); >>>> + >>>> + return ret; >>>> +} > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-07 12:38 ` Benjamin MUGNIER @ 2022-10-07 13:19 ` Laurent Pinchart 2022-10-07 14:06 ` Benjamin MUGNIER 0 siblings, 1 reply; 26+ messages in thread From: Laurent Pinchart @ 2022-10-07 13:19 UTC (permalink / raw) To: Benjamin MUGNIER Cc: Sakari Ailus, linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, kieran.bingham, nicolas, hverkuil Hi Benjamin, On Fri, Oct 07, 2022 at 02:38:33PM +0200, Benjamin MUGNIER wrote: > On 10/7/22 14:32, Laurent Pinchart wrote: > > On Fri, Oct 07, 2022 at 02:24:16PM +0200, Benjamin MUGNIER wrote: > >> Hi Sakari, > >> > >> Thank you for your review. > >> Please consider everything not commented as queued for v7. > >> > >> On 10/6/22 21:14, Sakari Ailus wrote: > >>> Hi Benjamin, > >>> > >>> Thanks for the update. A few more comments below... > >>> > >>> On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote: > >>>> +static const char * const vgxy61_hdr_mode_menu[] = { > >>>> + "HDR linearize", > >>>> + "HDR substraction", > >>>> + "No HDR", > >>>> +}; > >>> > >>> I think more documentation is needed on the HDR modes. What do these mean? > >>> For instance, are they something that requires further processing or is the > >>> result e.g. a ready HDR image? > >>> > >>> This should probably make it to driver specific documentation. > >> > >> Sure, in fact I did something like this in v3: > >> https://lore.kernel.org/linux-media/20220512074037.3829926-4-benjamin.mugnier@foss.st.com/ > >> > >> Since I standardized the control I was unsure what to do with this > >> documentation, and dropped it. > >> > >> I will add back the > >> Documentation/userspace-api/media/drivers/st-vgxy61.rst file from this > >> commit to the patchset, while changing the control name to the new > >> one. > > > > The documentation there is about modes for HDR merge on the sensor side. > > Can the sensor also output the unmerged images, for instance > > line-interleaved ? > > The sensor can not output unmerged images. It can only output a single > image and frame merging as to be done sensor side. I wonder then, should we have two HDR mode controls, one for sensor-side HDR, and one to control the interleaving of images for host-side HDR ? The latter would need some standardization I think, as the ISP configuration needs to match, so there must be some industry de-facto standards. > >>>> + > >>>> +static const char * const vgxy61_supply_name[] = { > >>>> + "VCORE", > >>>> + "VDDIO", > >>>> + "VANA", > >>>> +}; > >>>> + > >>>> +#define VGXY61_NUM_SUPPLIES ARRAY_SIZE(vgxy61_supply_name) > >>> > >>> Please use plain ARRAY_SIZE() instead. > >>> > >>> ... > >>> > >>>> +static int vgxy61_poll_reg(struct vgxy61_dev *sensor, u32 reg, u8 poll_val, > >>>> + unsigned int timeout_ms) > >>>> +{ > >>>> + const unsigned int loop_delay_ms = 10; > >>>> + int ret, timeout; > >>>> + > >>>> + timeout = read_poll_timeout(vgxy61_read_reg, ret, > >>>> + ((ret < 0) || (ret == poll_val)), > >>>> + loop_delay_ms * 1000, timeout_ms * 1000, > >>>> + false, sensor, reg); > >>>> + if (timeout) > >>>> + return timeout; > >>>> + > >>>> + return 0; > >>> > >>> "timeout" here is entirely pointless. > >>> > >>>> +} > >>> > >>> ... > >>> > >>>> +static int vgxy61_apply_exposure(struct vgxy61_dev *sensor) > >>>> +{ > >>>> + int ret = 0; > >>>> + > >>>> + /* We first set expo to zero to avoid forbidden parameters couple */ > >>>> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, > >>>> + 0, &ret); > >>>> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_LONG, > >>>> + sensor->expo_long, &ret); > >>>> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, > >>>> + sensor->expo_short, &ret); > >>> > >>> return vgxy61_write_reg(...); > >>> > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + return 0; > >>>> +} > >>> > >>> ... > >>> > >>>> +static int vgxy61_init_controls(struct vgxy61_dev *sensor) > >>>> +{ > >>>> + const struct v4l2_ctrl_ops *ops = &vgxy61_ctrl_ops; > >>>> + struct v4l2_ctrl_handler *hdl = &sensor->ctrl_handler; > >>>> + const struct vgxy61_mode_info *cur_mode = sensor->current_mode; > >>>> + struct v4l2_ctrl *ctrl; > >>>> + int ret; > >>>> + > >>>> + v4l2_ctrl_handler_init(hdl, 16); > >>>> + /* We can use our own mutex for the ctrl lock */ > >>>> + hdl->lock = &sensor->lock; > >>>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN, 0, 0x1c, 1, > >>>> + sensor->analog_gain); > >>>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN, 0, 0xfff, 1, > >>>> + sensor->digital_gain); > >>>> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN, > >>>> + ARRAY_SIZE(vgxy61_test_pattern_menu) - 1, > >>>> + 0, 0, vgxy61_test_pattern_menu); > >>>> + ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, 0, > >>>> + sensor->line_length, 1, > >>>> + sensor->line_length - cur_mode->width); > >>>> + if (ctrl) > >>>> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >>>> + ctrl = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, > >>>> + ARRAY_SIZE(link_freq) - 1, 0, link_freq); > >>>> + if (ctrl) > >>>> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >>>> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_HDR_MODE, > >>>> + ARRAY_SIZE(vgxy61_hdr_mode_menu) - 1, 0, > >>>> + VGXY61_NO_HDR, vgxy61_hdr_mode_menu); > >>>> + > >>>> + /* > >>>> + * Keep a pointer to these controls as we need to update them when > >>>> + * setting the format > >>>> + */ > >>>> + sensor->pixel_rate_ctrl = v4l2_ctrl_new_std(hdl, ops, > >>>> + V4L2_CID_PIXEL_RATE, 1, > >>>> + INT_MAX, 1, > >>>> + get_pixel_rate(sensor)); > >>>> + sensor->pixel_rate_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >>> > >>> Also sensor->pixel_rate_ctrl may be NULL here. > >>> > >>>> + sensor->expo_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, > >>>> + sensor->expo_min, > >>>> + sensor->expo_max, 1, > >>>> + sensor->expo_long); > >>>> + sensor->vblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, > >>>> + sensor->vblank_min, > >>>> + 0xffff - cur_mode->crop.height, > >>>> + 1, sensor->vblank); > >>>> + sensor->vflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, > >>>> + 0, 1, 1, sensor->vflip); > >>>> + sensor->hflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, > >>>> + 0, 1, 1, sensor->hflip); > >>>> + > >>>> + if (hdl->error) { > >>>> + ret = hdl->error; > >>>> + goto free_ctrls; > >>>> + } > >>>> + > >>>> + sensor->sd.ctrl_handler = hdl; > >>>> + return 0; > >>>> + > >>>> +free_ctrls: > >>>> + v4l2_ctrl_handler_free(hdl); > >>>> + return ret; > >>>> +} > >>> > >>> ... > >>> > >>>> +static int vgxy61_probe(struct i2c_client *client) > >>>> +{ > >>>> + struct device *dev = &client->dev; > >>>> + struct fwnode_handle *handle; > >>>> + struct vgxy61_dev *sensor; > >>>> + int ret; > >>>> + > >>>> + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); > >>>> + if (!sensor) > >>>> + return -ENOMEM; > >>>> + > >>>> + sensor->i2c_client = client; > >>>> + sensor->streaming = false; > >>>> + sensor->hdr = VGXY61_NO_HDR; > >>>> + sensor->expo_long = 200; > >>>> + sensor->expo_short = 0; > >>>> + sensor->hflip = false; > >>>> + sensor->vflip = false; > >>>> + sensor->analog_gain = 0; > >>>> + sensor->digital_gain = 256; > >>>> + > >>>> + handle = fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), > >>>> + NULL); > >>> > >>> handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0); > >>> > >>>> + if (!handle) { > >>>> + dev_err(dev, "handle node not found\n"); > >>>> + return -EINVAL; > >>>> + } > >>>> + > >>>> + ret = vgxy61_tx_from_ep(sensor, handle); > >>>> + fwnode_handle_put(handle); > >>>> + if (ret) { > >>>> + dev_err(dev, "Failed to parse handle %d\n", ret); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + sensor->xclk = devm_clk_get(dev, NULL); > >>>> + if (IS_ERR(sensor->xclk)) { > >>>> + dev_err(dev, "failed to get xclk\n"); > >>>> + return PTR_ERR(sensor->xclk); > >>>> + } > >>>> + sensor->clk_freq = clk_get_rate(sensor->xclk); > >>>> + if (sensor->clk_freq < 6 * HZ_PER_MHZ || > >>>> + sensor->clk_freq > 27 * HZ_PER_MHZ) { > >>>> + dev_err(dev, "Only 6Mhz-27Mhz clock range supported. provide %lu MHz\n", > >>>> + sensor->clk_freq / HZ_PER_MHZ); > >>>> + return -EINVAL; > >>>> + } > >>>> + sensor->gpios_polarity = of_property_read_bool(dev->of_node, > >>>> + "invert-gpios-polarity"); > >>> > >>> I thought we did discuss dropping support for sensor synchronisation in > >>> this version? > >> > >> This properties affects strobing lights gpios polarities as you can > >> see in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing > >> gpios are inverted. This has nothing to do with the sensor > >> synchronization. > >> > >> Now I realize this is poorly named, and I even forgot to document it > >> in the device tree bindings file. I apologize. > >> > >> I would like to rename it to 'st,strobe-polarity' since this is vendor > >> specific and to better reflect that it affects strobing gpios. > >> I'll make this change for v7 and document this in the bindings file > >> too. Tell me if there is any issues with that. > >> > >>>> + > >>>> + v4l2_i2c_subdev_init(&sensor->sd, client, &vgxy61_subdev_ops); > >>>> + sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > >>>> + sensor->pad.flags = MEDIA_PAD_FL_SOURCE; > >>>> + sensor->sd.entity.ops = &vgxy61_subdev_entity_ops; > >>>> + sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > >>>> + > >>>> + sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset", > >>>> + GPIOD_OUT_HIGH); > >>>> + > >>>> + ret = vgxy61_get_regulators(sensor); > >>>> + if (ret) { > >>>> + dev_err(&client->dev, "failed to get regulators %d\n", ret); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + ret = regulator_bulk_enable(VGXY61_NUM_SUPPLIES, sensor->supplies); > >>>> + if (ret) { > >>>> + dev_err(&client->dev, "failed to enable regulators %d\n", ret); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + ret = vgxy61_power_on(dev); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + ret = vgxy61_detect(sensor); > >>>> + if (ret) { > >>>> + dev_err(&client->dev, "sensor detect failed %d\n", ret); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + vgxy61_fill_sensor_param(sensor); > >>>> + vgxy61_fill_framefmt(sensor, sensor->current_mode, &sensor->fmt, > >>>> + VGXY61_MEDIA_BUS_FMT_DEF); > >>>> + > >>>> + ret = vgxy61_update_hdr(sensor, sensor->hdr); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + mutex_init(&sensor->lock); > >>>> + > >>>> + ret = vgxy61_init_controls(sensor); > >>>> + if (ret) { > >>>> + dev_err(&client->dev, "controls initialization failed %d\n", > >>>> + ret); > >>>> + goto error_power_off; > >>>> + } > >>>> + > >>>> + ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad); > >>>> + if (ret) { > >>>> + dev_err(&client->dev, "pads init failed %d\n", ret); > >>>> + goto error_handler_free; > >>>> + } > >>>> + > >>>> + /* Enable runtime PM and turn off the device */ > >>>> + pm_runtime_set_active(dev); > >>>> + pm_runtime_enable(dev); > >>>> + pm_runtime_idle(dev); > >>>> + > >>>> + ret = v4l2_async_register_subdev(&sensor->sd); > >>>> + if (ret) { > >>>> + dev_err(&client->dev, "async subdev register failed %d\n", ret); > >>>> + goto error_pm_runtime; > >>>> + } > >>>> + > >>>> + pm_runtime_set_autosuspend_delay(&client->dev, 1000); > >>>> + pm_runtime_use_autosuspend(&client->dev); > >>>> + > >>>> + dev_dbg(&client->dev, "vgxy61 probe successfully\n"); > >>>> + > >>>> + return 0; > >>>> + > >>>> +error_pm_runtime: > >>>> + pm_runtime_disable(&client->dev); > >>>> + pm_runtime_set_suspended(&client->dev); > >>>> + media_entity_cleanup(&sensor->sd.entity); > >>>> +error_handler_free: > >>>> + v4l2_ctrl_handler_free(sensor->sd.ctrl_handler); > >>>> + mutex_destroy(&sensor->lock); > >>>> +error_power_off: > >>>> + vgxy61_power_off(dev); > >>>> + > >>>> + return ret; > >>>> +} > > -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-07 13:19 ` Laurent Pinchart @ 2022-10-07 14:06 ` Benjamin MUGNIER 0 siblings, 0 replies; 26+ messages in thread From: Benjamin MUGNIER @ 2022-10-07 14:06 UTC (permalink / raw) To: Laurent Pinchart Cc: Sakari Ailus, linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, kieran.bingham, nicolas, hverkuil Hi Laurent, On 10/7/22 15:19, Laurent Pinchart wrote: > Hi Benjamin, > > On Fri, Oct 07, 2022 at 02:38:33PM +0200, Benjamin MUGNIER wrote: >> On 10/7/22 14:32, Laurent Pinchart wrote: >>> On Fri, Oct 07, 2022 at 02:24:16PM +0200, Benjamin MUGNIER wrote: >>>> Hi Sakari, >>>> >>>> Thank you for your review. >>>> Please consider everything not commented as queued for v7. >>>> >>>> On 10/6/22 21:14, Sakari Ailus wrote: >>>>> Hi Benjamin, >>>>> >>>>> Thanks for the update. A few more comments below... >>>>> >>>>> On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote: >>>>>> +static const char * const vgxy61_hdr_mode_menu[] = { >>>>>> + "HDR linearize", >>>>>> + "HDR substraction", >>>>>> + "No HDR", >>>>>> +}; >>>>> >>>>> I think more documentation is needed on the HDR modes. What do these mean? >>>>> For instance, are they something that requires further processing or is the >>>>> result e.g. a ready HDR image? >>>>> >>>>> This should probably make it to driver specific documentation. >>>> >>>> Sure, in fact I did something like this in v3: >>>> https://lore.kernel.org/linux-media/20220512074037.3829926-4-benjamin.mugnier@foss.st.com/ >>>> >>>> Since I standardized the control I was unsure what to do with this >>>> documentation, and dropped it. >>>> >>>> I will add back the >>>> Documentation/userspace-api/media/drivers/st-vgxy61.rst file from this >>>> commit to the patchset, while changing the control name to the new >>>> one. >>> >>> The documentation there is about modes for HDR merge on the sensor side. >>> Can the sensor also output the unmerged images, for instance >>> line-interleaved ? >> >> The sensor can not output unmerged images. It can only output a single >> image and frame merging as to be done sensor side. > > I wonder then, should we have two HDR mode controls, one for sensor-side > HDR, and one to control the interleaving of images for host-side HDR ? > The latter would need some standardization I think, as the ISP > configuration needs to match, so there must be some industry de-facto > standards. > I forgot to change it for this version, but Nicolas advised in v5 ircc to change the sensor side control from 'V4L2_CID_HDR_MODE' to 'V4L2_CID_HDR_SENSOR_MODE'. I'll do it for v7. That will give space for the host side HDR control later on. >>>>>> + >>>>>> +static const char * const vgxy61_supply_name[] = { >>>>>> + "VCORE", >>>>>> + "VDDIO", >>>>>> + "VANA", >>>>>> +}; >>>>>> + >>>>>> +#define VGXY61_NUM_SUPPLIES ARRAY_SIZE(vgxy61_supply_name) >>>>> >>>>> Please use plain ARRAY_SIZE() instead. >>>>> >>>>> ... >>>>> >>>>>> +static int vgxy61_poll_reg(struct vgxy61_dev *sensor, u32 reg, u8 poll_val, >>>>>> + unsigned int timeout_ms) >>>>>> +{ >>>>>> + const unsigned int loop_delay_ms = 10; >>>>>> + int ret, timeout; >>>>>> + >>>>>> + timeout = read_poll_timeout(vgxy61_read_reg, ret, >>>>>> + ((ret < 0) || (ret == poll_val)), >>>>>> + loop_delay_ms * 1000, timeout_ms * 1000, >>>>>> + false, sensor, reg); >>>>>> + if (timeout) >>>>>> + return timeout; >>>>>> + >>>>>> + return 0; >>>>> >>>>> "timeout" here is entirely pointless. >>>>> >>>>>> +} >>>>> >>>>> ... >>>>> >>>>>> +static int vgxy61_apply_exposure(struct vgxy61_dev *sensor) >>>>>> +{ >>>>>> + int ret = 0; >>>>>> + >>>>>> + /* We first set expo to zero to avoid forbidden parameters couple */ >>>>>> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, >>>>>> + 0, &ret); >>>>>> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_LONG, >>>>>> + sensor->expo_long, &ret); >>>>>> + ret = vgxy61_write_reg(sensor, VGXY61_REG_COARSE_EXPOSURE_SHORT, >>>>>> + sensor->expo_short, &ret); >>>>> >>>>> return vgxy61_write_reg(...); >>>>> >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>> >>>>> ... >>>>> >>>>>> +static int vgxy61_init_controls(struct vgxy61_dev *sensor) >>>>>> +{ >>>>>> + const struct v4l2_ctrl_ops *ops = &vgxy61_ctrl_ops; >>>>>> + struct v4l2_ctrl_handler *hdl = &sensor->ctrl_handler; >>>>>> + const struct vgxy61_mode_info *cur_mode = sensor->current_mode; >>>>>> + struct v4l2_ctrl *ctrl; >>>>>> + int ret; >>>>>> + >>>>>> + v4l2_ctrl_handler_init(hdl, 16); >>>>>> + /* We can use our own mutex for the ctrl lock */ >>>>>> + hdl->lock = &sensor->lock; >>>>>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN, 0, 0x1c, 1, >>>>>> + sensor->analog_gain); >>>>>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN, 0, 0xfff, 1, >>>>>> + sensor->digital_gain); >>>>>> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN, >>>>>> + ARRAY_SIZE(vgxy61_test_pattern_menu) - 1, >>>>>> + 0, 0, vgxy61_test_pattern_menu); >>>>>> + ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, 0, >>>>>> + sensor->line_length, 1, >>>>>> + sensor->line_length - cur_mode->width); >>>>>> + if (ctrl) >>>>>> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>>>> + ctrl = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, >>>>>> + ARRAY_SIZE(link_freq) - 1, 0, link_freq); >>>>>> + if (ctrl) >>>>>> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>>>> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_HDR_MODE, >>>>>> + ARRAY_SIZE(vgxy61_hdr_mode_menu) - 1, 0, >>>>>> + VGXY61_NO_HDR, vgxy61_hdr_mode_menu); >>>>>> + >>>>>> + /* >>>>>> + * Keep a pointer to these controls as we need to update them when >>>>>> + * setting the format >>>>>> + */ >>>>>> + sensor->pixel_rate_ctrl = v4l2_ctrl_new_std(hdl, ops, >>>>>> + V4L2_CID_PIXEL_RATE, 1, >>>>>> + INT_MAX, 1, >>>>>> + get_pixel_rate(sensor)); >>>>>> + sensor->pixel_rate_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>>> >>>>> Also sensor->pixel_rate_ctrl may be NULL here. >>>>> >>>>>> + sensor->expo_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, >>>>>> + sensor->expo_min, >>>>>> + sensor->expo_max, 1, >>>>>> + sensor->expo_long); >>>>>> + sensor->vblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, >>>>>> + sensor->vblank_min, >>>>>> + 0xffff - cur_mode->crop.height, >>>>>> + 1, sensor->vblank); >>>>>> + sensor->vflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, >>>>>> + 0, 1, 1, sensor->vflip); >>>>>> + sensor->hflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, >>>>>> + 0, 1, 1, sensor->hflip); >>>>>> + >>>>>> + if (hdl->error) { >>>>>> + ret = hdl->error; >>>>>> + goto free_ctrls; >>>>>> + } >>>>>> + >>>>>> + sensor->sd.ctrl_handler = hdl; >>>>>> + return 0; >>>>>> + >>>>>> +free_ctrls: >>>>>> + v4l2_ctrl_handler_free(hdl); >>>>>> + return ret; >>>>>> +} >>>>> >>>>> ... >>>>> >>>>>> +static int vgxy61_probe(struct i2c_client *client) >>>>>> +{ >>>>>> + struct device *dev = &client->dev; >>>>>> + struct fwnode_handle *handle; >>>>>> + struct vgxy61_dev *sensor; >>>>>> + int ret; >>>>>> + >>>>>> + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); >>>>>> + if (!sensor) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + sensor->i2c_client = client; >>>>>> + sensor->streaming = false; >>>>>> + sensor->hdr = VGXY61_NO_HDR; >>>>>> + sensor->expo_long = 200; >>>>>> + sensor->expo_short = 0; >>>>>> + sensor->hflip = false; >>>>>> + sensor->vflip = false; >>>>>> + sensor->analog_gain = 0; >>>>>> + sensor->digital_gain = 256; >>>>>> + >>>>>> + handle = fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), >>>>>> + NULL); >>>>> >>>>> handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0); >>>>> >>>>>> + if (!handle) { >>>>>> + dev_err(dev, "handle node not found\n"); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + ret = vgxy61_tx_from_ep(sensor, handle); >>>>>> + fwnode_handle_put(handle); >>>>>> + if (ret) { >>>>>> + dev_err(dev, "Failed to parse handle %d\n", ret); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + sensor->xclk = devm_clk_get(dev, NULL); >>>>>> + if (IS_ERR(sensor->xclk)) { >>>>>> + dev_err(dev, "failed to get xclk\n"); >>>>>> + return PTR_ERR(sensor->xclk); >>>>>> + } >>>>>> + sensor->clk_freq = clk_get_rate(sensor->xclk); >>>>>> + if (sensor->clk_freq < 6 * HZ_PER_MHZ || >>>>>> + sensor->clk_freq > 27 * HZ_PER_MHZ) { >>>>>> + dev_err(dev, "Only 6Mhz-27Mhz clock range supported. provide %lu MHz\n", >>>>>> + sensor->clk_freq / HZ_PER_MHZ); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + sensor->gpios_polarity = of_property_read_bool(dev->of_node, >>>>>> + "invert-gpios-polarity"); >>>>> >>>>> I thought we did discuss dropping support for sensor synchronisation in >>>>> this version? >>>> >>>> This properties affects strobing lights gpios polarities as you can >>>> see in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing >>>> gpios are inverted. This has nothing to do with the sensor >>>> synchronization. >>>> >>>> Now I realize this is poorly named, and I even forgot to document it >>>> in the device tree bindings file. I apologize. >>>> >>>> I would like to rename it to 'st,strobe-polarity' since this is vendor >>>> specific and to better reflect that it affects strobing gpios. >>>> I'll make this change for v7 and document this in the bindings file >>>> too. Tell me if there is any issues with that. >>>> >>>>>> + >>>>>> + v4l2_i2c_subdev_init(&sensor->sd, client, &vgxy61_subdev_ops); >>>>>> + sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >>>>>> + sensor->pad.flags = MEDIA_PAD_FL_SOURCE; >>>>>> + sensor->sd.entity.ops = &vgxy61_subdev_entity_ops; >>>>>> + sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; >>>>>> + >>>>>> + sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset", >>>>>> + GPIOD_OUT_HIGH); >>>>>> + >>>>>> + ret = vgxy61_get_regulators(sensor); >>>>>> + if (ret) { >>>>>> + dev_err(&client->dev, "failed to get regulators %d\n", ret); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + ret = regulator_bulk_enable(VGXY61_NUM_SUPPLIES, sensor->supplies); >>>>>> + if (ret) { >>>>>> + dev_err(&client->dev, "failed to enable regulators %d\n", ret); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + ret = vgxy61_power_on(dev); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>>> + ret = vgxy61_detect(sensor); >>>>>> + if (ret) { >>>>>> + dev_err(&client->dev, "sensor detect failed %d\n", ret); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + vgxy61_fill_sensor_param(sensor); >>>>>> + vgxy61_fill_framefmt(sensor, sensor->current_mode, &sensor->fmt, >>>>>> + VGXY61_MEDIA_BUS_FMT_DEF); >>>>>> + >>>>>> + ret = vgxy61_update_hdr(sensor, sensor->hdr); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>>> + mutex_init(&sensor->lock); >>>>>> + >>>>>> + ret = vgxy61_init_controls(sensor); >>>>>> + if (ret) { >>>>>> + dev_err(&client->dev, "controls initialization failed %d\n", >>>>>> + ret); >>>>>> + goto error_power_off; >>>>>> + } >>>>>> + >>>>>> + ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad); >>>>>> + if (ret) { >>>>>> + dev_err(&client->dev, "pads init failed %d\n", ret); >>>>>> + goto error_handler_free; >>>>>> + } >>>>>> + >>>>>> + /* Enable runtime PM and turn off the device */ >>>>>> + pm_runtime_set_active(dev); >>>>>> + pm_runtime_enable(dev); >>>>>> + pm_runtime_idle(dev); >>>>>> + >>>>>> + ret = v4l2_async_register_subdev(&sensor->sd); >>>>>> + if (ret) { >>>>>> + dev_err(&client->dev, "async subdev register failed %d\n", ret); >>>>>> + goto error_pm_runtime; >>>>>> + } >>>>>> + >>>>>> + pm_runtime_set_autosuspend_delay(&client->dev, 1000); >>>>>> + pm_runtime_use_autosuspend(&client->dev); >>>>>> + >>>>>> + dev_dbg(&client->dev, "vgxy61 probe successfully\n"); >>>>>> + >>>>>> + return 0; >>>>>> + >>>>>> +error_pm_runtime: >>>>>> + pm_runtime_disable(&client->dev); >>>>>> + pm_runtime_set_suspended(&client->dev); >>>>>> + media_entity_cleanup(&sensor->sd.entity); >>>>>> +error_handler_free: >>>>>> + v4l2_ctrl_handler_free(sensor->sd.ctrl_handler); >>>>>> + mutex_destroy(&sensor->lock); >>>>>> +error_power_off: >>>>>> + vgxy61_power_off(dev); >>>>>> + >>>>>> + return ret; >>>>>> +} >>> > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-07 12:24 ` Benjamin MUGNIER 2022-10-07 12:32 ` Laurent Pinchart @ 2022-10-10 8:29 ` Sakari Ailus 2022-10-10 9:19 ` Benjamin MUGNIER 1 sibling, 1 reply; 26+ messages in thread From: Sakari Ailus @ 2022-10-10 8:29 UTC (permalink / raw) To: Benjamin MUGNIER Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Benjamin, On Fri, Oct 07, 2022 at 02:24:16PM +0200, Benjamin MUGNIER wrote: > Hi Sakari, > > Thank you for your review. > Please consider everything not commented as queued for v7. > > On 10/6/22 21:14, Sakari Ailus wrote: > > Hi Benjamin, > > > > Thanks for the update. A few more comments below... > > > > On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote: > >> +static const char * const vgxy61_hdr_mode_menu[] = { > >> + "HDR linearize", > >> + "HDR substraction", > >> + "No HDR", > >> +}; > > > > I think more documentation is needed on the HDR modes. What do these mean? > > For instance, are they something that requires further processing or is the > > result e.g. a ready HDR image? > > > > This should probably make it to driver specific documentation. > > > > Sure, in fact I did something like this in v3: > https://lore.kernel.org/linux-media/20220512074037.3829926-4-benjamin.mugnier@foss.st.com/ > > Since I standardized the control I was unsure what to do with this documentation, and dropped it. > I will add back the Documentation/userspace-api/media/drivers/st-vgxy61.rst file from this commit to the patchset, while changing the control name to the new one. Yes, please. That seems reasonably good. I think Laurent's proposal on a HDR control for sensor-implemented HDR is a good one. Further controls can be added later on. > >> + sensor->gpios_polarity = of_property_read_bool(dev->of_node, > >> + "invert-gpios-polarity"); How about: device_property_read_bool(dev, ...); > > > > I thought we did discuss dropping support for sensor synchronisation in > > this version? > > > > This properties affects strobing lights gpios polarities as you can see > in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing gpios > are inverted. This has nothing to do with the sensor synchronization. So this is for strobing a LED flash? It would be good to mention this in DT bindings. > > Now I realize this is poorly named, and I even forgot to document it in > the device tree bindings file. I apologize. No problem. > > I would like to rename it to 'st,strobe-polarity' since this is vendor > specific and to better reflect that it affects strobing gpios. I'll make > this change for v7 and document this in the bindings file too. Tell me if > there is any issues with that. That name seems reasonable to me. Although, *if* this is actually usable as a GPIO as the bindings suggest, then the GPIO flags would probably be a better alternative. -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-10 8:29 ` Sakari Ailus @ 2022-10-10 9:19 ` Benjamin MUGNIER 2022-10-10 9:33 ` Sakari Ailus 0 siblings, 1 reply; 26+ messages in thread From: Benjamin MUGNIER @ 2022-10-10 9:19 UTC (permalink / raw) To: Sakari Ailus Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Sakari, On 10/10/22 10:29, Sakari Ailus wrote: > Hi Benjamin, > > On Fri, Oct 07, 2022 at 02:24:16PM +0200, Benjamin MUGNIER wrote: >> Hi Sakari, >> >> Thank you for your review. >> Please consider everything not commented as queued for v7. >> >> On 10/6/22 21:14, Sakari Ailus wrote: >>> Hi Benjamin, >>> >>> Thanks for the update. A few more comments below... >>> >>> On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote: >>>> +static const char * const vgxy61_hdr_mode_menu[] = { >>>> + "HDR linearize", >>>> + "HDR substraction", >>>> + "No HDR", >>>> +}; >>> >>> I think more documentation is needed on the HDR modes. What do these mean? >>> For instance, are they something that requires further processing or is the >>> result e.g. a ready HDR image? >>> >>> This should probably make it to driver specific documentation. >>> >> >> Sure, in fact I did something like this in v3: >> https://lore.kernel.org/linux-media/20220512074037.3829926-4-benjamin.mugnier@foss.st.com/ >> >> Since I standardized the control I was unsure what to do with this documentation, and dropped it. >> I will add back the Documentation/userspace-api/media/drivers/st-vgxy61.rst file from this commit to the patchset, while changing the control name to the new one. > > Yes, please. That seems reasonably good. > > I think Laurent's proposal on a HDR control for sensor-implemented HDR is a > good one. Further controls can be added later on. > >>>> + sensor->gpios_polarity = of_property_read_bool(dev->of_node, >>>> + "invert-gpios-polarity"); > > How about: > > device_property_read_bool(dev, ...); > Sounds great. >>> >>> I thought we did discuss dropping support for sensor synchronisation in >>> this version? >>> >> >> This properties affects strobing lights gpios polarities as you can see >> in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing gpios >> are inverted. This has nothing to do with the sensor synchronization. > > So this is for strobing a LED flash? It would be good to mention this in > DT bindings. > >> >> Now I realize this is poorly named, and I even forgot to document it in >> the device tree bindings file. I apologize. > > No problem. > >> >> I would like to rename it to 'st,strobe-polarity' since this is vendor >> specific and to better reflect that it affects strobing gpios. I'll make >> this change for v7 and document this in the bindings file too. Tell me if >> there is any issues with that. > > That name seems reasonable to me. Although, *if* this is actually usable as > a GPIO as the bindings suggest, then the GPIO flags would probably be a > better alternative. > If by GPIO flag you mean adding 'gpios' to the property, We could go with 'st,strobe-gpios-polarity', which in the end this leads to the same property name as it was in the dt bindings :) I'll add a bit of comments on the bindings. It seems to be the best choice. Thank you. -- Regards, Benjamin ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-10 9:19 ` Benjamin MUGNIER @ 2022-10-10 9:33 ` Sakari Ailus 2022-10-10 9:50 ` Benjamin MUGNIER 0 siblings, 1 reply; 26+ messages in thread From: Sakari Ailus @ 2022-10-10 9:33 UTC (permalink / raw) To: Benjamin MUGNIER Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Benjamin, On Mon, Oct 10, 2022 at 11:19:01AM +0200, Benjamin MUGNIER wrote: > Hi Sakari, > > On 10/10/22 10:29, Sakari Ailus wrote: > > Hi Benjamin, > > > > On Fri, Oct 07, 2022 at 02:24:16PM +0200, Benjamin MUGNIER wrote: > >> Hi Sakari, > >> > >> Thank you for your review. > >> Please consider everything not commented as queued for v7. > >> > >> On 10/6/22 21:14, Sakari Ailus wrote: > >>> Hi Benjamin, > >>> > >>> Thanks for the update. A few more comments below... > >>> > >>> On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote: > >>>> +static const char * const vgxy61_hdr_mode_menu[] = { > >>>> + "HDR linearize", > >>>> + "HDR substraction", > >>>> + "No HDR", > >>>> +}; > >>> > >>> I think more documentation is needed on the HDR modes. What do these mean? > >>> For instance, are they something that requires further processing or is the > >>> result e.g. a ready HDR image? > >>> > >>> This should probably make it to driver specific documentation. > >>> > >> > >> Sure, in fact I did something like this in v3: > >> https://lore.kernel.org/linux-media/20220512074037.3829926-4-benjamin.mugnier@foss.st.com/ > >> > >> Since I standardized the control I was unsure what to do with this documentation, and dropped it. > >> I will add back the Documentation/userspace-api/media/drivers/st-vgxy61.rst file from this commit to the patchset, while changing the control name to the new one. > > > > Yes, please. That seems reasonably good. > > > > I think Laurent's proposal on a HDR control for sensor-implemented HDR is a > > good one. Further controls can be added later on. > > > >>>> + sensor->gpios_polarity = of_property_read_bool(dev->of_node, > >>>> + "invert-gpios-polarity"); > > > > How about: > > > > device_property_read_bool(dev, ...); > > > > Sounds great. > > >>> > >>> I thought we did discuss dropping support for sensor synchronisation in > >>> this version? > >>> > >> > >> This properties affects strobing lights gpios polarities as you can see > >> in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing gpios > >> are inverted. This has nothing to do with the sensor synchronization. > > > > So this is for strobing a LED flash? It would be good to mention this in > > DT bindings. > > > >> > >> Now I realize this is poorly named, and I even forgot to document it in > >> the device tree bindings file. I apologize. > > > > No problem. > > > >> > >> I would like to rename it to 'st,strobe-polarity' since this is vendor > >> specific and to better reflect that it affects strobing gpios. I'll make > >> this change for v7 and document this in the bindings file too. Tell me if > >> there is any issues with that. > > > > That name seems reasonable to me. Although, *if* this is actually usable as > > a GPIO as the bindings suggest, then the GPIO flags would probably be a > > better alternative. > > > > If by GPIO flag you mean adding 'gpios' to the property, We could go with 'st,strobe-gpios-polarity', which in the end this leads to the same property name as it was in the dt bindings :) > I'll add a bit of comments on the bindings. It seems to be the best choice. Is this a GPIO or is it not (e.g. strobe signal only)? For the latter the this should be fine. And "flash-leds" property should be there as well I guess? -- Sakari Ailus ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-10 9:33 ` Sakari Ailus @ 2022-10-10 9:50 ` Benjamin MUGNIER 2022-10-10 11:55 ` Sakari Ailus 0 siblings, 1 reply; 26+ messages in thread From: Benjamin MUGNIER @ 2022-10-10 9:50 UTC (permalink / raw) To: Sakari Ailus Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Sakari, On 10/10/22 11:33, Sakari Ailus wrote: > Hi Benjamin, > > On Mon, Oct 10, 2022 at 11:19:01AM +0200, Benjamin MUGNIER wrote: >> Hi Sakari, >> >> On 10/10/22 10:29, Sakari Ailus wrote: >>> Hi Benjamin, >>> >>> On Fri, Oct 07, 2022 at 02:24:16PM +0200, Benjamin MUGNIER wrote: >>>> Hi Sakari, >>>> >>>> Thank you for your review. >>>> Please consider everything not commented as queued for v7. >>>> >>>> On 10/6/22 21:14, Sakari Ailus wrote: >>>>> Hi Benjamin, >>>>> >>>>> Thanks for the update. A few more comments below... >>>>> >>>>> On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote: >>>>>> +static const char * const vgxy61_hdr_mode_menu[] = { >>>>>> + "HDR linearize", >>>>>> + "HDR substraction", >>>>>> + "No HDR", >>>>>> +}; >>>>> >>>>> I think more documentation is needed on the HDR modes. What do these mean? >>>>> For instance, are they something that requires further processing or is the >>>>> result e.g. a ready HDR image? >>>>> >>>>> This should probably make it to driver specific documentation. >>>>> >>>> >>>> Sure, in fact I did something like this in v3: >>>> https://lore.kernel.org/linux-media/20220512074037.3829926-4-benjamin.mugnier@foss.st.com/ >>>> >>>> Since I standardized the control I was unsure what to do with this documentation, and dropped it. >>>> I will add back the Documentation/userspace-api/media/drivers/st-vgxy61.rst file from this commit to the patchset, while changing the control name to the new one. >>> >>> Yes, please. That seems reasonably good. >>> >>> I think Laurent's proposal on a HDR control for sensor-implemented HDR is a >>> good one. Further controls can be added later on. >>> >>>>>> + sensor->gpios_polarity = of_property_read_bool(dev->of_node, >>>>>> + "invert-gpios-polarity"); >>> >>> How about: >>> >>> device_property_read_bool(dev, ...); >>> >> >> Sounds great. >> >>>>> >>>>> I thought we did discuss dropping support for sensor synchronisation in >>>>> this version? >>>>> >>>> >>>> This properties affects strobing lights gpios polarities as you can see >>>> in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing gpios >>>> are inverted. This has nothing to do with the sensor synchronization. >>> >>> So this is for strobing a LED flash? It would be good to mention this in >>> DT bindings. >>> >>>> >>>> Now I realize this is poorly named, and I even forgot to document it in >>>> the device tree bindings file. I apologize. >>> >>> No problem. >>> >>>> >>>> I would like to rename it to 'st,strobe-polarity' since this is vendor >>>> specific and to better reflect that it affects strobing gpios. I'll make >>>> this change for v7 and document this in the bindings file too. Tell me if >>>> there is any issues with that. >>> >>> That name seems reasonable to me. Although, *if* this is actually usable as >>> a GPIO as the bindings suggest, then the GPIO flags would probably be a >>> better alternative. >>> >> >> If by GPIO flag you mean adding 'gpios' to the property, We could go with 'st,strobe-gpios-polarity', which in the end this leads to the same property name as it was in the dt bindings :) >> I'll add a bit of comments on the bindings. It seems to be the best choice. > > Is this a GPIO or is it not (e.g. strobe signal only)? > > For the latter the this should be fine. And "flash-leds" property should be > there as well I guess? > This property controls the polarity of and output GPIO connected to the sensor. This output GPIO is driven by the sensor firmware in order to illuminate the scene whenever necessary. I'm not sure this goes under the "flash-leds" category, as it only provides a signal with either "0" (don't illuminate) or a 1 (illuminate) ? The sensor controls the signal following the programmed "strobe-mode" as you can see in vgxy61_strobe_mode according to the HDR mode. It does not have a max-microamp or timeout values as a flash I suppose, it is really a simple signal. Practically we have what we call "illuminators" that are either built in the sensor'smodule, are externally connected to the sensor's module. Hope this is clearer. -- Regards, Benjamin ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-10 9:50 ` Benjamin MUGNIER @ 2022-10-10 11:55 ` Sakari Ailus 2022-10-10 12:11 ` Benjamin MUGNIER 0 siblings, 1 reply; 26+ messages in thread From: Sakari Ailus @ 2022-10-10 11:55 UTC (permalink / raw) To: Benjamin MUGNIER Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Benjamin, On Mon, Oct 10, 2022 at 11:50:25AM +0200, Benjamin MUGNIER wrote: > Hi Sakari, > > On 10/10/22 11:33, Sakari Ailus wrote: > > Hi Benjamin, > > > > On Mon, Oct 10, 2022 at 11:19:01AM +0200, Benjamin MUGNIER wrote: > >> Hi Sakari, > >> > >> On 10/10/22 10:29, Sakari Ailus wrote: > >>> Hi Benjamin, > >>> > >>> On Fri, Oct 07, 2022 at 02:24:16PM +0200, Benjamin MUGNIER wrote: > >>>> Hi Sakari, > >>>> > >>>> Thank you for your review. > >>>> Please consider everything not commented as queued for v7. > >>>> > >>>> On 10/6/22 21:14, Sakari Ailus wrote: > >>>>> Hi Benjamin, > >>>>> > >>>>> Thanks for the update. A few more comments below... > >>>>> > >>>>> On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote: > >>>>>> +static const char * const vgxy61_hdr_mode_menu[] = { > >>>>>> + "HDR linearize", > >>>>>> + "HDR substraction", > >>>>>> + "No HDR", > >>>>>> +}; > >>>>> > >>>>> I think more documentation is needed on the HDR modes. What do these mean? > >>>>> For instance, are they something that requires further processing or is the > >>>>> result e.g. a ready HDR image? > >>>>> > >>>>> This should probably make it to driver specific documentation. > >>>>> > >>>> > >>>> Sure, in fact I did something like this in v3: > >>>> https://lore.kernel.org/linux-media/20220512074037.3829926-4-benjamin.mugnier@foss.st.com/ > >>>> > >>>> Since I standardized the control I was unsure what to do with this documentation, and dropped it. > >>>> I will add back the Documentation/userspace-api/media/drivers/st-vgxy61.rst file from this commit to the patchset, while changing the control name to the new one. > >>> > >>> Yes, please. That seems reasonably good. > >>> > >>> I think Laurent's proposal on a HDR control for sensor-implemented HDR is a > >>> good one. Further controls can be added later on. > >>> > >>>>>> + sensor->gpios_polarity = of_property_read_bool(dev->of_node, > >>>>>> + "invert-gpios-polarity"); > >>> > >>> How about: > >>> > >>> device_property_read_bool(dev, ...); > >>> > >> > >> Sounds great. > >> > >>>>> > >>>>> I thought we did discuss dropping support for sensor synchronisation in > >>>>> this version? > >>>>> > >>>> > >>>> This properties affects strobing lights gpios polarities as you can see > >>>> in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing gpios > >>>> are inverted. This has nothing to do with the sensor synchronization. > >>> > >>> So this is for strobing a LED flash? It would be good to mention this in > >>> DT bindings. > >>> > >>>> > >>>> Now I realize this is poorly named, and I even forgot to document it in > >>>> the device tree bindings file. I apologize. > >>> > >>> No problem. > >>> > >>>> > >>>> I would like to rename it to 'st,strobe-polarity' since this is vendor > >>>> specific and to better reflect that it affects strobing gpios. I'll make > >>>> this change for v7 and document this in the bindings file too. Tell me if > >>>> there is any issues with that. > >>> > >>> That name seems reasonable to me. Although, *if* this is actually usable as > >>> a GPIO as the bindings suggest, then the GPIO flags would probably be a > >>> better alternative. > >>> > >> > >> If by GPIO flag you mean adding 'gpios' to the property, We could go with 'st,strobe-gpios-polarity', which in the end this leads to the same property name as it was in the dt bindings :) > >> I'll add a bit of comments on the bindings. It seems to be the best choice. > > > > Is this a GPIO or is it not (e.g. strobe signal only)? > > > > For the latter the this should be fine. And "flash-leds" property should be > > there as well I guess? > > > Please wrap the lines at around 74. Rewrapped now... > This property controls the polarity of and output GPIO connected to the > sensor. This output GPIO is driven by the sensor firmware in order to > illuminate the scene whenever necessary. I'm not sure this goes under the > "flash-leds" category, as it only provides a signal with either "0" > (don't illuminate) or a 1 (illuminate) ? The sensor controls the signal This is what sensors generally do. > following the programmed "strobe-mode" as you can see in > vgxy61_strobe_mode according to the HDR mode. It does not have a > max-microamp or timeout values as a flash I suppose, it is really a > simple signal. Those are usually configured for the flash driver, not on the sensor. > > Practically we have what we call "illuminators" that are either built in > the sensor'smodule, are externally connected to the sensor's module. > > Hope this is clearer. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-10 11:55 ` Sakari Ailus @ 2022-10-10 12:11 ` Benjamin MUGNIER 2022-10-10 12:52 ` Sakari Ailus 0 siblings, 1 reply; 26+ messages in thread From: Benjamin MUGNIER @ 2022-10-10 12:11 UTC (permalink / raw) To: Sakari Ailus Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Sakari, On 10/10/22 13:55, Sakari Ailus wrote: > Hi Benjamin, > > On Mon, Oct 10, 2022 at 11:50:25AM +0200, Benjamin MUGNIER wrote: >> Hi Sakari, >> >> On 10/10/22 11:33, Sakari Ailus wrote: >>> Hi Benjamin, >>> >>> On Mon, Oct 10, 2022 at 11:19:01AM +0200, Benjamin MUGNIER wrote: >>>> Hi Sakari, >>>> >>>> On 10/10/22 10:29, Sakari Ailus wrote: >>>>> Hi Benjamin, >>>>> >>>>> On Fri, Oct 07, 2022 at 02:24:16PM +0200, Benjamin MUGNIER wrote: >>>>>> Hi Sakari, >>>>>> >>>>>> Thank you for your review. >>>>>> Please consider everything not commented as queued for v7. >>>>>> >>>>>> On 10/6/22 21:14, Sakari Ailus wrote: >>>>>>> Hi Benjamin, >>>>>>> >>>>>>> Thanks for the update. A few more comments below... >>>>>>> >>>>>>> On Tue, Sep 27, 2022 at 10:37:02AM +0200, Benjamin Mugnier wrote: >>>>>>>> +static const char * const vgxy61_hdr_mode_menu[] = { >>>>>>>> + "HDR linearize", >>>>>>>> + "HDR substraction", >>>>>>>> + "No HDR", >>>>>>>> +}; >>>>>>> >>>>>>> I think more documentation is needed on the HDR modes. What do these mean? >>>>>>> For instance, are they something that requires further processing or is the >>>>>>> result e.g. a ready HDR image? >>>>>>> >>>>>>> This should probably make it to driver specific documentation. >>>>>>> >>>>>> >>>>>> Sure, in fact I did something like this in v3: >>>>>> https://lore.kernel.org/linux-media/20220512074037.3829926-4-benjamin.mugnier@foss.st.com/ >>>>>> >>>>>> Since I standardized the control I was unsure what to do with this documentation, and dropped it. >>>>>> I will add back the Documentation/userspace-api/media/drivers/st-vgxy61.rst file from this commit to the patchset, while changing the control name to the new one. >>>>> >>>>> Yes, please. That seems reasonably good. >>>>> >>>>> I think Laurent's proposal on a HDR control for sensor-implemented HDR is a >>>>> good one. Further controls can be added later on. >>>>> >>>>>>>> + sensor->gpios_polarity = of_property_read_bool(dev->of_node, >>>>>>>> + "invert-gpios-polarity"); >>>>> >>>>> How about: >>>>> >>>>> device_property_read_bool(dev, ...); >>>>> >>>> >>>> Sounds great. >>>> >>>>>>> >>>>>>> I thought we did discuss dropping support for sensor synchronisation in >>>>>>> this version? >>>>>>> >>>>>> >>>>>> This properties affects strobing lights gpios polarities as you can see >>>>>> in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing gpios >>>>>> are inverted. This has nothing to do with the sensor synchronization. >>>>> >>>>> So this is for strobing a LED flash? It would be good to mention this in >>>>> DT bindings. >>>>> >>>>>> >>>>>> Now I realize this is poorly named, and I even forgot to document it in >>>>>> the device tree bindings file. I apologize. >>>>> >>>>> No problem. >>>>> >>>>>> >>>>>> I would like to rename it to 'st,strobe-polarity' since this is vendor >>>>>> specific and to better reflect that it affects strobing gpios. I'll make >>>>>> this change for v7 and document this in the bindings file too. Tell me if >>>>>> there is any issues with that. >>>>> >>>>> That name seems reasonable to me. Although, *if* this is actually usable as >>>>> a GPIO as the bindings suggest, then the GPIO flags would probably be a >>>>> better alternative. >>>>> >>>> >>>> If by GPIO flag you mean adding 'gpios' to the property, We could go with 'st,strobe-gpios-polarity', which in the end this leads to the same property name as it was in the dt bindings :) >>>> I'll add a bit of comments on the bindings. It seems to be the best choice. >>> >>> Is this a GPIO or is it not (e.g. strobe signal only)? >>> >>> For the latter the this should be fine. And "flash-leds" property should be >>> there as well I guess? >>> >> > > Please wrap the lines at around 74. Rewrapped now... > Done. Thank you. >> This property controls the polarity of and output GPIO connected to the >> sensor. This output GPIO is driven by the sensor firmware in order to >> illuminate the scene whenever necessary. I'm not sure this goes under the >> "flash-leds" category, as it only provides a signal with either "0" >> (don't illuminate) or a 1 (illuminate) ? The sensor controls the signal > > This is what sensors generally do. > >> following the programmed "strobe-mode" as you can see in >> vgxy61_strobe_mode according to the HDR mode. It does not have a >> max-microamp or timeout values as a flash I suppose, it is really a >> simple signal. > > Those are usually configured for the flash driver, not on the sensor. > Ok, I guess in this case there is no flash driver. Should I keep the 'st,strobe-gpios-polarity' property or are you aware of an already defined property for this behavior? >> >> Practically we have what we call "illuminators" that are either built in >> the sensor'smodule, are externally connected to the sensor's module. >> >> Hope this is clearer. > -- Regards, Benjamin ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-10 12:11 ` Benjamin MUGNIER @ 2022-10-10 12:52 ` Sakari Ailus 2022-10-10 13:12 ` Benjamin MUGNIER 0 siblings, 1 reply; 26+ messages in thread From: Sakari Ailus @ 2022-10-10 12:52 UTC (permalink / raw) To: Benjamin MUGNIER Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Benjamin, On Mon, Oct 10, 2022 at 02:11:46PM +0200, Benjamin MUGNIER wrote: ... > >>>>>>> I thought we did discuss dropping support for sensor synchronisation in > >>>>>>> this version? > >>>>>>> > >>>>>> > >>>>>> This properties affects strobing lights gpios polarities as you can see > >>>>>> in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing gpios > >>>>>> are inverted. This has nothing to do with the sensor synchronization. > >>>>> > >>>>> So this is for strobing a LED flash? It would be good to mention this in > >>>>> DT bindings. > >>>>> > >>>>>> > >>>>>> Now I realize this is poorly named, and I even forgot to document it in > >>>>>> the device tree bindings file. I apologize. > >>>>> > >>>>> No problem. > >>>>> > >>>>>> > >>>>>> I would like to rename it to 'st,strobe-polarity' since this is vendor > >>>>>> specific and to better reflect that it affects strobing gpios. I'll make > >>>>>> this change for v7 and document this in the bindings file too. Tell me if > >>>>>> there is any issues with that. > >>>>> > >>>>> That name seems reasonable to me. Although, *if* this is actually usable as > >>>>> a GPIO as the bindings suggest, then the GPIO flags would probably be a > >>>>> better alternative. > >>>>> > >>>> > >>>> If by GPIO flag you mean adding 'gpios' to the property, We could go with 'st,strobe-gpios-polarity', which in the end this leads to the same property name as it was in the dt bindings :) > >>>> I'll add a bit of comments on the bindings. It seems to be the best choice. > >>> > >>> Is this a GPIO or is it not (e.g. strobe signal only)? > >>> > >>> For the latter the this should be fine. And "flash-leds" property should be > >>> there as well I guess? > >>> > >> > > > > Please wrap the lines at around 74. Rewrapped now... > > > > Done. Thank you. > > >> This property controls the polarity of and output GPIO connected to the > >> sensor. This output GPIO is driven by the sensor firmware in order to > >> illuminate the scene whenever necessary. I'm not sure this goes under the > >> "flash-leds" category, as it only provides a signal with either "0" > >> (don't illuminate) or a 1 (illuminate) ? The sensor controls the signal > > > > This is what sensors generally do. > > > >> following the programmed "strobe-mode" as you can see in > >> vgxy61_strobe_mode according to the HDR mode. It does not have a > >> max-microamp or timeout values as a flash I suppose, it is really a > >> simple signal. > > > > Those are usually configured for the flash driver, not on the sensor. > > > > Ok, I guess in this case there is no flash driver. Should I keep the > 'st,strobe-gpios-polarity' property or are you aware of an already > defined property for this behavior? So the LED is directly connected to this pin (perhaps in series with a resistor)? That is an unusual solution. -- Sakari Ailus ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-10 12:52 ` Sakari Ailus @ 2022-10-10 13:12 ` Benjamin MUGNIER 2022-10-13 8:00 ` Sakari Ailus 0 siblings, 1 reply; 26+ messages in thread From: Benjamin MUGNIER @ 2022-10-10 13:12 UTC (permalink / raw) To: Sakari Ailus Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Sakari, On 10/10/22 14:52, Sakari Ailus wrote: > Hi Benjamin, > > On Mon, Oct 10, 2022 at 02:11:46PM +0200, Benjamin MUGNIER wrote: > > ... > >>>>>>>>> I thought we did discuss dropping support for sensor synchronisation in >>>>>>>>> this version? >>>>>>>>> >>>>>>>> >>>>>>>> This properties affects strobing lights gpios polarities as you can see >>>>>>>> in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing gpios >>>>>>>> are inverted. This has nothing to do with the sensor synchronization. >>>>>>> >>>>>>> So this is for strobing a LED flash? It would be good to mention this in >>>>>>> DT bindings. >>>>>>> >>>>>>>> >>>>>>>> Now I realize this is poorly named, and I even forgot to document it in >>>>>>>> the device tree bindings file. I apologize. >>>>>>> >>>>>>> No problem. >>>>>>> >>>>>>>> >>>>>>>> I would like to rename it to 'st,strobe-polarity' since this is vendor >>>>>>>> specific and to better reflect that it affects strobing gpios. I'll make >>>>>>>> this change for v7 and document this in the bindings file too. Tell me if >>>>>>>> there is any issues with that. >>>>>>> >>>>>>> That name seems reasonable to me. Although, *if* this is actually usable as >>>>>>> a GPIO as the bindings suggest, then the GPIO flags would probably be a >>>>>>> better alternative. >>>>>>> >>>>>> >>>>>> If by GPIO flag you mean adding 'gpios' to the property, We could go with 'st,strobe-gpios-polarity', which in the end this leads to the same property name as it was in the dt bindings :) >>>>>> I'll add a bit of comments on the bindings. It seems to be the best choice. >>>>> >>>>> Is this a GPIO or is it not (e.g. strobe signal only)? >>>>> >>>>> For the latter the this should be fine. And "flash-leds" property should be >>>>> there as well I guess? >>>>> >>>> >>> >>> Please wrap the lines at around 74. Rewrapped now... >>> >> >> Done. Thank you. >> >>>> This property controls the polarity of and output GPIO connected to the >>>> sensor. This output GPIO is driven by the sensor firmware in order to >>>> illuminate the scene whenever necessary. I'm not sure this goes under the >>>> "flash-leds" category, as it only provides a signal with either "0" >>>> (don't illuminate) or a 1 (illuminate) ? The sensor controls the signal >>> >>> This is what sensors generally do. >>> >>>> following the programmed "strobe-mode" as you can see in >>>> vgxy61_strobe_mode according to the HDR mode. It does not have a >>>> max-microamp or timeout values as a flash I suppose, it is really a >>>> simple signal. >>> >>> Those are usually configured for the flash driver, not on the sensor. >>> >> >> Ok, I guess in this case there is no flash driver. Should I keep the >> 'st,strobe-gpios-polarity' property or are you aware of an already >> defined property for this behavior? > > So the LED is directly connected to this pin (perhaps in series with a > resistor)? That is an unusual solution. > Yes, the pin is connected to a transistor responsible of powering on and off the LEDs according to the pin value, so that they have correct voltage from another power supply. But yes that's basically it. Thanks a lot. -- Regards, Benjamin ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-10 13:12 ` Benjamin MUGNIER @ 2022-10-13 8:00 ` Sakari Ailus 2022-10-18 7:54 ` Benjamin MUGNIER 0 siblings, 1 reply; 26+ messages in thread From: Sakari Ailus @ 2022-10-13 8:00 UTC (permalink / raw) To: Benjamin MUGNIER Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Benjamin, On Mon, Oct 10, 2022 at 03:12:30PM +0200, Benjamin MUGNIER wrote: > Hi Sakari, > > On 10/10/22 14:52, Sakari Ailus wrote: > > Hi Benjamin, > > > > On Mon, Oct 10, 2022 at 02:11:46PM +0200, Benjamin MUGNIER wrote: > > > > ... > > > >>>>>>>>> I thought we did discuss dropping support for sensor synchronisation in > >>>>>>>>> this version? > >>>>>>>>> > >>>>>>>> > >>>>>>>> This properties affects strobing lights gpios polarities as you can see > >>>>>>>> in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing gpios > >>>>>>>> are inverted. This has nothing to do with the sensor synchronization. > >>>>>>> > >>>>>>> So this is for strobing a LED flash? It would be good to mention this in > >>>>>>> DT bindings. > >>>>>>> > >>>>>>>> > >>>>>>>> Now I realize this is poorly named, and I even forgot to document it in > >>>>>>>> the device tree bindings file. I apologize. > >>>>>>> > >>>>>>> No problem. > >>>>>>> > >>>>>>>> > >>>>>>>> I would like to rename it to 'st,strobe-polarity' since this is vendor > >>>>>>>> specific and to better reflect that it affects strobing gpios. I'll make > >>>>>>>> this change for v7 and document this in the bindings file too. Tell me if > >>>>>>>> there is any issues with that. > >>>>>>> > >>>>>>> That name seems reasonable to me. Although, *if* this is actually usable as > >>>>>>> a GPIO as the bindings suggest, then the GPIO flags would probably be a > >>>>>>> better alternative. > >>>>>>> > >>>>>> > >>>>>> If by GPIO flag you mean adding 'gpios' to the property, We could go with 'st,strobe-gpios-polarity', which in the end this leads to the same property name as it was in the dt bindings :) > >>>>>> I'll add a bit of comments on the bindings. It seems to be the best choice. > >>>>> > >>>>> Is this a GPIO or is it not (e.g. strobe signal only)? > >>>>> > >>>>> For the latter the this should be fine. And "flash-leds" property should be > >>>>> there as well I guess? > >>>>> > >>>> > >>> > >>> Please wrap the lines at around 74. Rewrapped now... > >>> > >> > >> Done. Thank you. > >> > >>>> This property controls the polarity of and output GPIO connected to the > >>>> sensor. This output GPIO is driven by the sensor firmware in order to > >>>> illuminate the scene whenever necessary. I'm not sure this goes under the > >>>> "flash-leds" category, as it only provides a signal with either "0" > >>>> (don't illuminate) or a 1 (illuminate) ? The sensor controls the signal > >>> > >>> This is what sensors generally do. > >>> > >>>> following the programmed "strobe-mode" as you can see in > >>>> vgxy61_strobe_mode according to the HDR mode. It does not have a > >>>> max-microamp or timeout values as a flash I suppose, it is really a > >>>> simple signal. > >>> > >>> Those are usually configured for the flash driver, not on the sensor. > >>> > >> > >> Ok, I guess in this case there is no flash driver. Should I keep the > >> 'st,strobe-gpios-polarity' property or are you aware of an already > >> defined property for this behavior? > > > > So the LED is directly connected to this pin (perhaps in series with a > > resistor)? That is an unusual solution. > > > > Yes, the pin is connected to a transistor responsible of powering on and > off the LEDs according to the pin value, so that they have correct > voltage from another power supply. But yes that's basically it. > > Thanks a lot. I think it's fine as-is. Maybe the property could be called "st,strobe-polarity" as this isn't a GPIO from software point of view? Virtually all other users have a flash driver chip. But the flash LED isn't part of the module in those cases either. -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor 2022-10-13 8:00 ` Sakari Ailus @ 2022-10-18 7:54 ` Benjamin MUGNIER 0 siblings, 0 replies; 26+ messages in thread From: Benjamin MUGNIER @ 2022-10-18 7:54 UTC (permalink / raw) To: Sakari Ailus Cc: linux-media, alain.volmat, hugues.fruchet, sylvain.petinot, dave.stevenson, laurent.pinchart, kieran.bingham, nicolas, hverkuil Hi Sakari, On 10/13/22 10:00, Sakari Ailus wrote: > Hi Benjamin, > > On Mon, Oct 10, 2022 at 03:12:30PM +0200, Benjamin MUGNIER wrote: >> Hi Sakari, >> >> On 10/10/22 14:52, Sakari Ailus wrote: >>> Hi Benjamin, >>> >>> On Mon, Oct 10, 2022 at 02:11:46PM +0200, Benjamin MUGNIER wrote: >>> >>> ... >>> >>>>>>>>>>> I thought we did discuss dropping support for sensor synchronisation in >>>>>>>>>>> this version? >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> This properties affects strobing lights gpios polarities as you can see >>>>>>>>>> in vgxy61_update_gpios_strobe_polarity. If set to '1' all strobing gpios >>>>>>>>>> are inverted. This has nothing to do with the sensor synchronization. >>>>>>>>> >>>>>>>>> So this is for strobing a LED flash? It would be good to mention this in >>>>>>>>> DT bindings. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> Now I realize this is poorly named, and I even forgot to document it in >>>>>>>>>> the device tree bindings file. I apologize. >>>>>>>>> >>>>>>>>> No problem. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> I would like to rename it to 'st,strobe-polarity' since this is vendor >>>>>>>>>> specific and to better reflect that it affects strobing gpios. I'll make >>>>>>>>>> this change for v7 and document this in the bindings file too. Tell me if >>>>>>>>>> there is any issues with that. >>>>>>>>> >>>>>>>>> That name seems reasonable to me. Although, *if* this is actually usable as >>>>>>>>> a GPIO as the bindings suggest, then the GPIO flags would probably be a >>>>>>>>> better alternative. >>>>>>>>> >>>>>>>> >>>>>>>> If by GPIO flag you mean adding 'gpios' to the property, We could go with 'st,strobe-gpios-polarity', which in the end this leads to the same property name as it was in the dt bindings :) >>>>>>>> I'll add a bit of comments on the bindings. It seems to be the best choice. >>>>>>> >>>>>>> Is this a GPIO or is it not (e.g. strobe signal only)? >>>>>>> >>>>>>> For the latter the this should be fine. And "flash-leds" property should be >>>>>>> there as well I guess? >>>>>>> >>>>>> >>>>> >>>>> Please wrap the lines at around 74. Rewrapped now... >>>>> >>>> >>>> Done. Thank you. >>>> >>>>>> This property controls the polarity of and output GPIO connected to the >>>>>> sensor. This output GPIO is driven by the sensor firmware in order to >>>>>> illuminate the scene whenever necessary. I'm not sure this goes under the >>>>>> "flash-leds" category, as it only provides a signal with either "0" >>>>>> (don't illuminate) or a 1 (illuminate) ? The sensor controls the signal >>>>> >>>>> This is what sensors generally do. >>>>> >>>>>> following the programmed "strobe-mode" as you can see in >>>>>> vgxy61_strobe_mode according to the HDR mode. It does not have a >>>>>> max-microamp or timeout values as a flash I suppose, it is really a >>>>>> simple signal. >>>>> >>>>> Those are usually configured for the flash driver, not on the sensor. >>>>> >>>> >>>> Ok, I guess in this case there is no flash driver. Should I keep the >>>> 'st,strobe-gpios-polarity' property or are you aware of an already >>>> defined property for this behavior? >>> >>> So the LED is directly connected to this pin (perhaps in series with a >>> resistor)? That is an unusual solution. >>> >> >> Yes, the pin is connected to a transistor responsible of powering on and >> off the LEDs according to the pin value, so that they have correct >> voltage from another power supply. But yes that's basically it. >> >> Thanks a lot. > > I think it's fine as-is. Maybe the property could be called > "st,strobe-polarity" as this isn't a GPIO from software point of view? You're right. As I already published v7 I queued this for v8 in order to reduce the noise on the mailing list, if you don't mind. Thanks a ton for your explanations. > > Virtually all other users have a flash driver chip. But the flash LED isn't > part of the module in those cases either. > -- Regards, Benjamin ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2022-10-18 7:54 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-27 8:36 [PATCH v6 0/4] media: Add ST VGXY61 camera sensor driver Benjamin Mugnier 2022-09-27 8:36 ` [PATCH v6 1/4] media: v4l: Add 1X16 16-bit greyscale media bus code definition Benjamin Mugnier 2022-10-06 21:26 ` Laurent Pinchart 2022-10-07 11:56 ` Benjamin MUGNIER 2022-09-27 8:37 ` [PATCH v6 2/4] media: v4l: ctrls: Add a control for HDR mode Benjamin Mugnier 2022-09-27 8:37 ` [PATCH v6 3/4] media: dt-bindings: media: i2c: Add ST VGXY61 camera sensor binding Benjamin Mugnier 2022-10-06 19:15 ` Sakari Ailus 2022-10-07 11:57 ` Benjamin MUGNIER 2022-10-07 12:28 ` Benjamin MUGNIER 2022-09-27 8:37 ` [PATCH v6 4/4] media: i2c: Add driver for ST VGXY61 camera sensor Benjamin Mugnier 2022-10-06 19:14 ` Sakari Ailus 2022-10-07 12:24 ` Benjamin MUGNIER 2022-10-07 12:32 ` Laurent Pinchart 2022-10-07 12:38 ` Benjamin MUGNIER 2022-10-07 13:19 ` Laurent Pinchart 2022-10-07 14:06 ` Benjamin MUGNIER 2022-10-10 8:29 ` Sakari Ailus 2022-10-10 9:19 ` Benjamin MUGNIER 2022-10-10 9:33 ` Sakari Ailus 2022-10-10 9:50 ` Benjamin MUGNIER 2022-10-10 11:55 ` Sakari Ailus 2022-10-10 12:11 ` Benjamin MUGNIER 2022-10-10 12:52 ` Sakari Ailus 2022-10-10 13:12 ` Benjamin MUGNIER 2022-10-13 8:00 ` Sakari Ailus 2022-10-18 7:54 ` Benjamin MUGNIER
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).