* [RFC PATCH 0/3] v4l2 api changes for imx378 driver @ 2020-04-14 20:01 Daniel Gomez 2020-04-14 20:01 ` [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum Daniel Gomez ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Daniel Gomez @ 2020-04-14 20:01 UTC (permalink / raw) To: mchehab, hverkuil-cisco, linux-media; +Cc: linux-kernel, Daniel Gomez Hi all, I would like to discuss with you guys the next two following topics: * VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL: I'm working on a driver for the imx378 sensor but the current v4l2-subdev API (VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL) doesn't allow you to set up a range of frame intervals. However, this is supported in the v4l2 device API level. My idea is to follow the same approach as the VIDIOC_SUBDEV_ENUM_FRAME_SIZE by setting a min and max intervals in the VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL to solve this missing support. This is the current output of VIDIOC_ENUM_FRAMEINTERVALS in continous mode: v4l2-ctl --list-frameintervals width=1920,height=1080,pixelformat=pRAA \ -d /dev/video0 ioctl: VIDIOC_ENUM_FRAMEINTERVALS Interval: Continuous 0.029s - 0.607s (1.648-34.797 fps) This is the current output of VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL: v4l2-ctl --list-subdev-frameintervals code=0x300f,width=1920,height=1080 \ -d /dev/v4l-subdev19 ioctl: VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL (pad=0) Interval: 0.029s (34.797 fps) So, the idea would be to return the interval range from the v4l2-subdev level to the device level. Besides that, it would also be necessary to adapt the v4l-utils tools (compliance and ctl). What do you think guys? Please, follow the RFC patch series to see my suggestion. * V4L2_CID_TEMPERATURE: In addition to this, the driver is able to report as a v4l2 control the temperature of the sensor. Since is quite 'general' control I also included the v4l2 temperature control as part of the common v4l2 control list. Would it be also possible? In the RFC patch series you will find the the modified code for the VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL and V4L2_CID_TEMPERATURE topics as well as the imx378 driver using the above. Daniel Gomez (3): media: v4l2-subdev.h: Add min and max enum media: v4l2: Add v4l2 control IDs for temperature media: imx378: Add imx378 camera sensor driver drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/imx378.c | 1829 ++++++++++++++++++++++++++ drivers/media/v4l2-core/v4l2-ctrls.c | 5 + include/uapi/linux/v4l2-controls.h | 4 +- include/uapi/linux/v4l2-subdev.h | 6 +- 6 files changed, 1854 insertions(+), 2 deletions(-) create mode 100644 drivers/media/i2c/imx378.c -- 2.25.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum 2020-04-14 20:01 [RFC PATCH 0/3] v4l2 api changes for imx378 driver Daniel Gomez @ 2020-04-14 20:01 ` Daniel Gomez 2020-04-30 9:42 ` Sakari Ailus 2020-04-14 20:01 ` [RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature Daniel Gomez 2020-04-14 20:01 ` [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver Daniel Gomez 2 siblings, 1 reply; 16+ messages in thread From: Daniel Gomez @ 2020-04-14 20:01 UTC (permalink / raw) To: mchehab, hverkuil-cisco, linux-media; +Cc: linux-kernel, Daniel Gomez Add min and max structures to the v4l2-subdev callback in order to allow the subdev to return a range of valid frame intervals. This would operate similar to the struct v4l2_subdev_frame_size_enum and its max and min values for the width and the height. In this case, the possibility to return a frame interval range is added to the v4l2-subdev level whenever the v4l2 device operates in step-wise or continuous mode. Signed-off-by: Daniel Gomez <daniel@qtec.com> --- include/uapi/linux/v4l2-subdev.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h index 03970ce30741..ee15393c58cd 100644 --- a/include/uapi/linux/v4l2-subdev.h +++ b/include/uapi/linux/v4l2-subdev.h @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval { * @code: format code (MEDIA_BUS_FMT_ definitions) * @width: frame width in pixels * @height: frame height in pixels + * @min_interval: min frame interval in seconds + * @max_interval: max frame interval in seconds * @interval: frame interval in seconds * @which: format type (from enum v4l2_subdev_format_whence) */ @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum { __u32 code; __u32 width; __u32 height; + struct v4l2_fract min_interval; + struct v4l2_fract max_interval; struct v4l2_fract interval; __u32 which; - __u32 reserved[8]; + __u32 reserved[4]; }; /** -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum 2020-04-14 20:01 ` [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum Daniel Gomez @ 2020-04-30 9:42 ` Sakari Ailus 2020-04-30 11:10 ` Laurent Pinchart 0 siblings, 1 reply; 16+ messages in thread From: Sakari Ailus @ 2020-04-30 9:42 UTC (permalink / raw) To: Daniel Gomez Cc: mchehab, hverkuil-cisco, linux-media, linux-kernel, Laurent Pinchart Hi Daniel, Thanks for the patchset. I'm also cc'ing Laurent who I think could be interested in this one. On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote: > Add min and max structures to the v4l2-subdev callback in order to allow > the subdev to return a range of valid frame intervals. > > This would operate similar to the struct v4l2_subdev_frame_size_enum and > its max and min values for the width and the height. In this case, the > possibility to return a frame interval range is added to the v4l2-subdev level > whenever the v4l2 device operates in step-wise or continuous mode. The current API only allows providing a list of enumerated values. That is limiting indeed, especially on register list based sensor drivers where vertical blanking is configurable. I guess this could be extended to cover what V4L2, more or less. If we tell it's a range, is it assumed to be contiguous? We don't have try operation for the frame interval, but I guess set is good enough. The fraction is probably best for TV standards but it's not what camera sensors natively use. (But for a register list based driver, the established practice remains to use frame interval.) I'm also wondering the effect on existing user space; if a driver gives a range, how will the existing programs work with such a driver? I'd add an anonymous union with the interval field, the other field being min_interval. Then the current applications would get the minimum interval and still continue to function. I guess compilers are modern enough these days we can have an anonymous union in the uAPI? > > Signed-off-by: Daniel Gomez <daniel@qtec.com> > --- > include/uapi/linux/v4l2-subdev.h | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > index 03970ce30741..ee15393c58cd 100644 > --- a/include/uapi/linux/v4l2-subdev.h > +++ b/include/uapi/linux/v4l2-subdev.h > @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval { > * @code: format code (MEDIA_BUS_FMT_ definitions) > * @width: frame width in pixels > * @height: frame height in pixels > + * @min_interval: min frame interval in seconds > + * @max_interval: max frame interval in seconds > * @interval: frame interval in seconds > * @which: format type (from enum v4l2_subdev_format_whence) > */ > @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum { > __u32 code; > __u32 width; > __u32 height; > + struct v4l2_fract min_interval; > + struct v4l2_fract max_interval; This changes the memory layout of the struct and breaks the ABI. Any new fields in the struct may only replace reserved fields while the rest must stay unchanged. > struct v4l2_fract interval; > __u32 which; > - __u32 reserved[8]; > + __u32 reserved[4]; > }; > > /** -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum 2020-04-30 9:42 ` Sakari Ailus @ 2020-04-30 11:10 ` Laurent Pinchart 2020-04-30 13:31 ` Sakari Ailus 0 siblings, 1 reply; 16+ messages in thread From: Laurent Pinchart @ 2020-04-30 11:10 UTC (permalink / raw) To: Sakari Ailus Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel Hello, On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote: > Hi Daniel, > > Thanks for the patchset. > > I'm also cc'ing Laurent who I think could be interested in this one. > > On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote: > > Add min and max structures to the v4l2-subdev callback in order to allow > > the subdev to return a range of valid frame intervals. > > > > This would operate similar to the struct v4l2_subdev_frame_size_enum and > > its max and min values for the width and the height. In this case, the > > possibility to return a frame interval range is added to the v4l2-subdev level > > whenever the v4l2 device operates in step-wise or continuous mode. > > The current API only allows providing a list of enumerated values. That is > limiting indeed, especially on register list based sensor drivers where > vertical blanking is configurable. > > I guess this could be extended to cover what V4L2, more or less. If we tell > it's a range, is it assumed to be contiguous? We don't have try operation > for the frame interval, but I guess set is good enough. The fraction is > probably best for TV standards but it's not what camera sensors natively > use. (But for a register list based driver, the established practice > remains to use frame interval.) > > I'm also wondering the effect on existing user space; if a driver gives a > range, how will the existing programs work with such a driver? > > I'd add an anonymous union with the interval field, the other field being > min_interval. Then the current applications would get the minimum interval > and still continue to function. I guess compilers are modern enough these > days we can have an anonymous union in the uAPI? We can discuss all this, but given patch 3/3 in this series, I think this isn't the right API :-) The sensor driver should not expose the frame interval enumeration API. It should instead expose control of the frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and V4L2_CID_VBLANK. > > Signed-off-by: Daniel Gomez <daniel@qtec.com> > > --- > > include/uapi/linux/v4l2-subdev.h | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > index 03970ce30741..ee15393c58cd 100644 > > --- a/include/uapi/linux/v4l2-subdev.h > > +++ b/include/uapi/linux/v4l2-subdev.h > > @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval { > > * @code: format code (MEDIA_BUS_FMT_ definitions) > > * @width: frame width in pixels > > * @height: frame height in pixels > > + * @min_interval: min frame interval in seconds > > + * @max_interval: max frame interval in seconds > > * @interval: frame interval in seconds > > * @which: format type (from enum v4l2_subdev_format_whence) > > */ > > @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum { > > __u32 code; > > __u32 width; > > __u32 height; > > + struct v4l2_fract min_interval; > > + struct v4l2_fract max_interval; > > This changes the memory layout of the struct and breaks the ABI. Any new > fields in the struct may only replace reserved fields while the rest must > stay unchanged. > > > struct v4l2_fract interval; > > __u32 which; > > - __u32 reserved[8]; > > + __u32 reserved[4]; > > }; > > > > /** -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum 2020-04-30 11:10 ` Laurent Pinchart @ 2020-04-30 13:31 ` Sakari Ailus 2020-04-30 13:59 ` Laurent Pinchart 0 siblings, 1 reply; 16+ messages in thread From: Sakari Ailus @ 2020-04-30 13:31 UTC (permalink / raw) To: Laurent Pinchart Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel Hi Laurent, On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote: > Hello, > > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote: > > Hi Daniel, > > > > Thanks for the patchset. > > > > I'm also cc'ing Laurent who I think could be interested in this one. > > > > On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote: > > > Add min and max structures to the v4l2-subdev callback in order to allow > > > the subdev to return a range of valid frame intervals. > > > > > > This would operate similar to the struct v4l2_subdev_frame_size_enum and > > > its max and min values for the width and the height. In this case, the > > > possibility to return a frame interval range is added to the v4l2-subdev level > > > whenever the v4l2 device operates in step-wise or continuous mode. > > > > The current API only allows providing a list of enumerated values. That is > > limiting indeed, especially on register list based sensor drivers where > > vertical blanking is configurable. > > > > I guess this could be extended to cover what V4L2, more or less. If we tell > > it's a range, is it assumed to be contiguous? We don't have try operation > > for the frame interval, but I guess set is good enough. The fraction is > > probably best for TV standards but it's not what camera sensors natively > > use. (But for a register list based driver, the established practice > > remains to use frame interval.) > > > > I'm also wondering the effect on existing user space; if a driver gives a > > range, how will the existing programs work with such a driver? > > > > I'd add an anonymous union with the interval field, the other field being > > min_interval. Then the current applications would get the minimum interval > > and still continue to function. I guess compilers are modern enough these > > days we can have an anonymous union in the uAPI? > > We can discuss all this, but given patch 3/3 in this series, I think > this isn't the right API :-) The sensor driver should not expose the > frame interval enumeration API. It should instead expose control of the > frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and > V4L2_CID_VBLANK. > That would require also exposing the size of the pixel array (and the analogue crop), in order to provide all the necessary information to calculate the frame rate. No objections there; this is a new driver. There are however existing drivers that implement s_frame_interval subdev ioctl; those might benefit from this one. Or would you implement the pixel rate based control as well, and effectively deprecate the s_frame_interval on those? > > > Signed-off-by: Daniel Gomez <daniel@qtec.com> > > > --- > > > include/uapi/linux/v4l2-subdev.h | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > > index 03970ce30741..ee15393c58cd 100644 > > > --- a/include/uapi/linux/v4l2-subdev.h > > > +++ b/include/uapi/linux/v4l2-subdev.h > > > @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval { > > > * @code: format code (MEDIA_BUS_FMT_ definitions) > > > * @width: frame width in pixels > > > * @height: frame height in pixels > > > + * @min_interval: min frame interval in seconds > > > + * @max_interval: max frame interval in seconds > > > * @interval: frame interval in seconds > > > * @which: format type (from enum v4l2_subdev_format_whence) > > > */ > > > @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum { > > > __u32 code; > > > __u32 width; > > > __u32 height; > > > + struct v4l2_fract min_interval; > > > + struct v4l2_fract max_interval; > > > > This changes the memory layout of the struct and breaks the ABI. Any new > > fields in the struct may only replace reserved fields while the rest must > > stay unchanged. > > > > > struct v4l2_fract interval; > > > __u32 which; > > > - __u32 reserved[8]; > > > + __u32 reserved[4]; > > > }; > > > > > > /** > -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum 2020-04-30 13:31 ` Sakari Ailus @ 2020-04-30 13:59 ` Laurent Pinchart 2020-04-30 14:15 ` Sakari Ailus 0 siblings, 1 reply; 16+ messages in thread From: Laurent Pinchart @ 2020-04-30 13:59 UTC (permalink / raw) To: Sakari Ailus Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel Hi Sakari, On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote: > On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote: > > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote: > >> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote: > >>> Add min and max structures to the v4l2-subdev callback in order to allow > >>> the subdev to return a range of valid frame intervals. > >>> > >>> This would operate similar to the struct v4l2_subdev_frame_size_enum and > >>> its max and min values for the width and the height. In this case, the > >>> possibility to return a frame interval range is added to the v4l2-subdev level > >>> whenever the v4l2 device operates in step-wise or continuous mode. > >> > >> The current API only allows providing a list of enumerated values. That is > >> limiting indeed, especially on register list based sensor drivers where > >> vertical blanking is configurable. > >> > >> I guess this could be extended to cover what V4L2, more or less. If we tell > >> it's a range, is it assumed to be contiguous? We don't have try operation > >> for the frame interval, but I guess set is good enough. The fraction is > >> probably best for TV standards but it's not what camera sensors natively > >> use. (But for a register list based driver, the established practice > >> remains to use frame interval.) > >> > >> I'm also wondering the effect on existing user space; if a driver gives a > >> range, how will the existing programs work with such a driver? > >> > >> I'd add an anonymous union with the interval field, the other field being > >> min_interval. Then the current applications would get the minimum interval > >> and still continue to function. I guess compilers are modern enough these > >> days we can have an anonymous union in the uAPI? > > > > We can discuss all this, but given patch 3/3 in this series, I think > > this isn't the right API :-) The sensor driver should not expose the > > frame interval enumeration API. It should instead expose control of the > > frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and > > V4L2_CID_VBLANK. > > > > That would require also exposing the size of the pixel array (and the > analogue crop), in order to provide all the necessary information to > calculate the frame rate. No objections there; this is a new driver. > > There are however existing drivers that implement s_frame_interval subdev > ioctl; those might benefit from this one. Or would you implement the pixel > rate based control as well, and effectively deprecate the s_frame_interval > on those? That's what I would recommend, yes. I would only keep .s_frame_interval() for sensors that expose that concept at the hardware level (for instance with an integrated ISP whose firmware exposes a frame interval or frame rate control). > >>> Signed-off-by: Daniel Gomez <daniel@qtec.com> > >>> --- > >>> include/uapi/linux/v4l2-subdev.h | 6 +++++- > >>> 1 file changed, 5 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > >>> index 03970ce30741..ee15393c58cd 100644 > >>> --- a/include/uapi/linux/v4l2-subdev.h > >>> +++ b/include/uapi/linux/v4l2-subdev.h > >>> @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval { > >>> * @code: format code (MEDIA_BUS_FMT_ definitions) > >>> * @width: frame width in pixels > >>> * @height: frame height in pixels > >>> + * @min_interval: min frame interval in seconds > >>> + * @max_interval: max frame interval in seconds > >>> * @interval: frame interval in seconds > >>> * @which: format type (from enum v4l2_subdev_format_whence) > >>> */ > >>> @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum { > >>> __u32 code; > >>> __u32 width; > >>> __u32 height; > >>> + struct v4l2_fract min_interval; > >>> + struct v4l2_fract max_interval; > >> > >> This changes the memory layout of the struct and breaks the ABI. Any new > >> fields in the struct may only replace reserved fields while the rest must > >> stay unchanged. > >> > >>> struct v4l2_fract interval; > >>> __u32 which; > >>> - __u32 reserved[8]; > >>> + __u32 reserved[4]; > >>> }; > >>> > >>> /** -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum 2020-04-30 13:59 ` Laurent Pinchart @ 2020-04-30 14:15 ` Sakari Ailus 2020-04-30 14:17 ` Laurent Pinchart 0 siblings, 1 reply; 16+ messages in thread From: Sakari Ailus @ 2020-04-30 14:15 UTC (permalink / raw) To: Laurent Pinchart Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel Hi Laurent, On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote: > > On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote: > > > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote: > > >> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote: > > >>> Add min and max structures to the v4l2-subdev callback in order to allow > > >>> the subdev to return a range of valid frame intervals. > > >>> > > >>> This would operate similar to the struct v4l2_subdev_frame_size_enum and > > >>> its max and min values for the width and the height. In this case, the > > >>> possibility to return a frame interval range is added to the v4l2-subdev level > > >>> whenever the v4l2 device operates in step-wise or continuous mode. > > >> > > >> The current API only allows providing a list of enumerated values. That is > > >> limiting indeed, especially on register list based sensor drivers where > > >> vertical blanking is configurable. > > >> > > >> I guess this could be extended to cover what V4L2, more or less. If we tell > > >> it's a range, is it assumed to be contiguous? We don't have try operation > > >> for the frame interval, but I guess set is good enough. The fraction is > > >> probably best for TV standards but it's not what camera sensors natively > > >> use. (But for a register list based driver, the established practice > > >> remains to use frame interval.) > > >> > > >> I'm also wondering the effect on existing user space; if a driver gives a > > >> range, how will the existing programs work with such a driver? > > >> > > >> I'd add an anonymous union with the interval field, the other field being > > >> min_interval. Then the current applications would get the minimum interval > > >> and still continue to function. I guess compilers are modern enough these > > >> days we can have an anonymous union in the uAPI? > > > > > > We can discuss all this, but given patch 3/3 in this series, I think > > > this isn't the right API :-) The sensor driver should not expose the > > > frame interval enumeration API. It should instead expose control of the > > > frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and > > > V4L2_CID_VBLANK. > > > > > > > That would require also exposing the size of the pixel array (and the > > analogue crop), in order to provide all the necessary information to > > calculate the frame rate. No objections there; this is a new driver. > > > > There are however existing drivers that implement s_frame_interval subdev > > ioctl; those might benefit from this one. Or would you implement the pixel > > rate based control as well, and effectively deprecate the s_frame_interval > > on those? > > That's what I would recommend, yes. I would only keep > .s_frame_interval() for sensors that expose that concept at the hardware > level (for instance with an integrated ISP whose firmware exposes a > frame interval or frame rate control). Sounds good to me. Jacopo's set exposing read-only subdevs completes the puzzle so the user space should have all it needs, right? -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum 2020-04-30 14:15 ` Sakari Ailus @ 2020-04-30 14:17 ` Laurent Pinchart 2020-04-30 14:18 ` Sakari Ailus 0 siblings, 1 reply; 16+ messages in thread From: Laurent Pinchart @ 2020-04-30 14:17 UTC (permalink / raw) To: Sakari Ailus Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel Hi Sakari, On Thu, Apr 30, 2020 at 05:15:52PM +0300, Sakari Ailus wrote: > On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote: > > On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote: > > > On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote: > > > > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote: > > > >> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote: > > > >>> Add min and max structures to the v4l2-subdev callback in order to allow > > > >>> the subdev to return a range of valid frame intervals. > > > >>> > > > >>> This would operate similar to the struct v4l2_subdev_frame_size_enum and > > > >>> its max and min values for the width and the height. In this case, the > > > >>> possibility to return a frame interval range is added to the v4l2-subdev level > > > >>> whenever the v4l2 device operates in step-wise or continuous mode. > > > >> > > > >> The current API only allows providing a list of enumerated values. That is > > > >> limiting indeed, especially on register list based sensor drivers where > > > >> vertical blanking is configurable. > > > >> > > > >> I guess this could be extended to cover what V4L2, more or less. If we tell > > > >> it's a range, is it assumed to be contiguous? We don't have try operation > > > >> for the frame interval, but I guess set is good enough. The fraction is > > > >> probably best for TV standards but it's not what camera sensors natively > > > >> use. (But for a register list based driver, the established practice > > > >> remains to use frame interval.) > > > >> > > > >> I'm also wondering the effect on existing user space; if a driver gives a > > > >> range, how will the existing programs work with such a driver? > > > >> > > > >> I'd add an anonymous union with the interval field, the other field being > > > >> min_interval. Then the current applications would get the minimum interval > > > >> and still continue to function. I guess compilers are modern enough these > > > >> days we can have an anonymous union in the uAPI? > > > > > > > > We can discuss all this, but given patch 3/3 in this series, I think > > > > this isn't the right API :-) The sensor driver should not expose the > > > > frame interval enumeration API. It should instead expose control of the > > > > frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and > > > > V4L2_CID_VBLANK. > > > > > > > > > > That would require also exposing the size of the pixel array (and the > > > analogue crop), in order to provide all the necessary information to > > > calculate the frame rate. No objections there; this is a new driver. > > > > > > There are however existing drivers that implement s_frame_interval subdev > > > ioctl; those might benefit from this one. Or would you implement the pixel > > > rate based control as well, and effectively deprecate the s_frame_interval > > > on those? > > > > That's what I would recommend, yes. I would only keep > > .s_frame_interval() for sensors that expose that concept at the hardware > > level (for instance with an integrated ISP whose firmware exposes a > > frame interval or frame rate control). > > Sounds good to me. > > Jacopo's set exposing read-only subdevs completes the puzzle so the user > space should have all it needs, right? Until we run into the next missing piece :-) -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum 2020-04-30 14:17 ` Laurent Pinchart @ 2020-04-30 14:18 ` Sakari Ailus 2020-04-30 14:20 ` Laurent Pinchart 0 siblings, 1 reply; 16+ messages in thread From: Sakari Ailus @ 2020-04-30 14:18 UTC (permalink / raw) To: Laurent Pinchart Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel On Thu, Apr 30, 2020 at 05:17:53PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Thu, Apr 30, 2020 at 05:15:52PM +0300, Sakari Ailus wrote: > > On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote: > > > On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote: > > > > On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote: > > > > > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote: > > > > >> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote: > > > > >>> Add min and max structures to the v4l2-subdev callback in order to allow > > > > >>> the subdev to return a range of valid frame intervals. > > > > >>> > > > > >>> This would operate similar to the struct v4l2_subdev_frame_size_enum and > > > > >>> its max and min values for the width and the height. In this case, the > > > > >>> possibility to return a frame interval range is added to the v4l2-subdev level > > > > >>> whenever the v4l2 device operates in step-wise or continuous mode. > > > > >> > > > > >> The current API only allows providing a list of enumerated values. That is > > > > >> limiting indeed, especially on register list based sensor drivers where > > > > >> vertical blanking is configurable. > > > > >> > > > > >> I guess this could be extended to cover what V4L2, more or less. If we tell > > > > >> it's a range, is it assumed to be contiguous? We don't have try operation > > > > >> for the frame interval, but I guess set is good enough. The fraction is > > > > >> probably best for TV standards but it's not what camera sensors natively > > > > >> use. (But for a register list based driver, the established practice > > > > >> remains to use frame interval.) > > > > >> > > > > >> I'm also wondering the effect on existing user space; if a driver gives a > > > > >> range, how will the existing programs work with such a driver? > > > > >> > > > > >> I'd add an anonymous union with the interval field, the other field being > > > > >> min_interval. Then the current applications would get the minimum interval > > > > >> and still continue to function. I guess compilers are modern enough these > > > > >> days we can have an anonymous union in the uAPI? > > > > > > > > > > We can discuss all this, but given patch 3/3 in this series, I think > > > > > this isn't the right API :-) The sensor driver should not expose the > > > > > frame interval enumeration API. It should instead expose control of the > > > > > frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and > > > > > V4L2_CID_VBLANK. > > > > > > > > > > > > > That would require also exposing the size of the pixel array (and the > > > > analogue crop), in order to provide all the necessary information to > > > > calculate the frame rate. No objections there; this is a new driver. > > > > > > > > There are however existing drivers that implement s_frame_interval subdev > > > > ioctl; those might benefit from this one. Or would you implement the pixel > > > > rate based control as well, and effectively deprecate the s_frame_interval > > > > on those? > > > > > > That's what I would recommend, yes. I would only keep > > > .s_frame_interval() for sensors that expose that concept at the hardware > > > level (for instance with an integrated ISP whose firmware exposes a > > > frame interval or frame rate control). > > > > Sounds good to me. > > > > Jacopo's set exposing read-only subdevs completes the puzzle so the user > > space should have all it needs, right? > > Until we run into the next missing piece :-) I was thinking of the frame rate configuration. Can you confirm that? -- Sakari Ailus ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum 2020-04-30 14:18 ` Sakari Ailus @ 2020-04-30 14:20 ` Laurent Pinchart 0 siblings, 0 replies; 16+ messages in thread From: Laurent Pinchart @ 2020-04-30 14:20 UTC (permalink / raw) To: Sakari Ailus Cc: Daniel Gomez, mchehab, hverkuil-cisco, linux-media, linux-kernel Hi Sakari, On Thu, Apr 30, 2020 at 05:18:49PM +0300, Sakari Ailus wrote: > On Thu, Apr 30, 2020 at 05:17:53PM +0300, Laurent Pinchart wrote: > > On Thu, Apr 30, 2020 at 05:15:52PM +0300, Sakari Ailus wrote: > >> On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote: > >>> On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote: > >>>> On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote: > >>>>> On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote: > >>>>>> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote: > >>>>>>> Add min and max structures to the v4l2-subdev callback in order to allow > >>>>>>> the subdev to return a range of valid frame intervals. > >>>>>>> > >>>>>>> This would operate similar to the struct v4l2_subdev_frame_size_enum and > >>>>>>> its max and min values for the width and the height. In this case, the > >>>>>>> possibility to return a frame interval range is added to the v4l2-subdev level > >>>>>>> whenever the v4l2 device operates in step-wise or continuous mode. > >>>>>> > >>>>>> The current API only allows providing a list of enumerated values. That is > >>>>>> limiting indeed, especially on register list based sensor drivers where > >>>>>> vertical blanking is configurable. > >>>>>> > >>>>>> I guess this could be extended to cover what V4L2, more or less. If we tell > >>>>>> it's a range, is it assumed to be contiguous? We don't have try operation > >>>>>> for the frame interval, but I guess set is good enough. The fraction is > >>>>>> probably best for TV standards but it's not what camera sensors natively > >>>>>> use. (But for a register list based driver, the established practice > >>>>>> remains to use frame interval.) > >>>>>> > >>>>>> I'm also wondering the effect on existing user space; if a driver gives a > >>>>>> range, how will the existing programs work with such a driver? > >>>>>> > >>>>>> I'd add an anonymous union with the interval field, the other field being > >>>>>> min_interval. Then the current applications would get the minimum interval > >>>>>> and still continue to function. I guess compilers are modern enough these > >>>>>> days we can have an anonymous union in the uAPI? > >>>>> > >>>>> We can discuss all this, but given patch 3/3 in this series, I think > >>>>> this isn't the right API :-) The sensor driver should not expose the > >>>>> frame interval enumeration API. It should instead expose control of the > >>>>> frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and > >>>>> V4L2_CID_VBLANK. > >>>>> > >>>> > >>>> That would require also exposing the size of the pixel array (and the > >>>> analogue crop), in order to provide all the necessary information to > >>>> calculate the frame rate. No objections there; this is a new driver. > >>>> > >>>> There are however existing drivers that implement s_frame_interval subdev > >>>> ioctl; those might benefit from this one. Or would you implement the pixel > >>>> rate based control as well, and effectively deprecate the s_frame_interval > >>>> on those? > >>> > >>> That's what I would recommend, yes. I would only keep > >>> .s_frame_interval() for sensors that expose that concept at the hardware > >>> level (for instance with an integrated ISP whose firmware exposes a > >>> frame interval or frame rate control). > >> > >> Sounds good to me. > >> > >> Jacopo's set exposing read-only subdevs completes the puzzle so the user > >> space should have all it needs, right? > > > > Until we run into the next missing piece :-) > > I was thinking of the frame rate configuration. Can you confirm that? I believe so, yes. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature 2020-04-14 20:01 [RFC PATCH 0/3] v4l2 api changes for imx378 driver Daniel Gomez 2020-04-14 20:01 ` [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum Daniel Gomez @ 2020-04-14 20:01 ` Daniel Gomez 2020-04-28 20:15 ` Sakari Ailus 2020-04-14 20:01 ` [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver Daniel Gomez 2 siblings, 1 reply; 16+ messages in thread From: Daniel Gomez @ 2020-04-14 20:01 UTC (permalink / raw) To: mchehab, hverkuil-cisco, linux-media; +Cc: linux-kernel, Daniel Gomez Add a v4l2 control ID to handle the temperature. Signed-off-by: Daniel Gomez <daniel@qtec.com> --- drivers/media/v4l2-core/v4l2-ctrls.c | 5 +++++ include/uapi/linux/v4l2-controls.h | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index 93d33d1db4e8..17b93111baa8 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -783,6 +783,7 @@ const char *v4l2_ctrl_get_name(u32 id) case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT: return "Min Number of Output Buffers"; case V4L2_CID_ALPHA_COMPONENT: return "Alpha Component"; case V4L2_CID_COLORFX_CBCR: return "Color Effects, CbCr"; + case V4L2_CID_TEMPERATURE: return "Temperature"; /* Codec controls */ /* The MPEG controls are applicable to all codec controls @@ -1344,6 +1345,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, *type = V4L2_CTRL_TYPE_INTEGER; *flags |= V4L2_CTRL_FLAG_READ_ONLY; break; + case V4L2_CID_TEMPERATURE: + *type = V4L2_CTRL_TYPE_INTEGER; + *flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY; + break; case V4L2_CID_MPEG_VIDEO_DEC_PTS: *type = V4L2_CTRL_TYPE_INTEGER64; *flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY; diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h index 1a58d7cc4ccc..76ec0a6da8d5 100644 --- a/include/uapi/linux/v4l2-controls.h +++ b/include/uapi/linux/v4l2-controls.h @@ -143,8 +143,10 @@ enum v4l2_colorfx { #define V4L2_CID_ALPHA_COMPONENT (V4L2_CID_BASE+41) #define V4L2_CID_COLORFX_CBCR (V4L2_CID_BASE+42) +#define V4L2_CID_TEMPERATURE (V4L2_CID_BASE+43) + /* last CID + 1 */ -#define V4L2_CID_LASTP1 (V4L2_CID_BASE+43) +#define V4L2_CID_LASTP1 (V4L2_CID_BASE+44) /* USER-class private control IDs */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature 2020-04-14 20:01 ` [RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature Daniel Gomez @ 2020-04-28 20:15 ` Sakari Ailus 0 siblings, 0 replies; 16+ messages in thread From: Sakari Ailus @ 2020-04-28 20:15 UTC (permalink / raw) To: Daniel Gomez; +Cc: mchehab, hverkuil-cisco, linux-media, linux-kernel Hi Daniel, On Tue, Apr 14, 2020 at 10:01:50PM +0200, Daniel Gomez wrote: > Add a v4l2 control ID to handle the temperature. > > Signed-off-by: Daniel Gomez <daniel@qtec.com> > --- > drivers/media/v4l2-core/v4l2-ctrls.c | 5 +++++ > include/uapi/linux/v4l2-controls.h | 4 +++- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index 93d33d1db4e8..17b93111baa8 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -783,6 +783,7 @@ const char *v4l2_ctrl_get_name(u32 id) > case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT: return "Min Number of Output Buffers"; > case V4L2_CID_ALPHA_COMPONENT: return "Alpha Component"; > case V4L2_CID_COLORFX_CBCR: return "Color Effects, CbCr"; > + case V4L2_CID_TEMPERATURE: return "Temperature"; What's the unit of this control? I think it should have one. As Hans pointed out, documentation is needed. > > /* Codec controls */ > /* The MPEG controls are applicable to all codec controls > @@ -1344,6 +1345,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > *type = V4L2_CTRL_TYPE_INTEGER; > *flags |= V4L2_CTRL_FLAG_READ_ONLY; > break; > + case V4L2_CID_TEMPERATURE: > + *type = V4L2_CTRL_TYPE_INTEGER; > + *flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY; > + break; > case V4L2_CID_MPEG_VIDEO_DEC_PTS: > *type = V4L2_CTRL_TYPE_INTEGER64; > *flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY; > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > index 1a58d7cc4ccc..76ec0a6da8d5 100644 > --- a/include/uapi/linux/v4l2-controls.h > +++ b/include/uapi/linux/v4l2-controls.h > @@ -143,8 +143,10 @@ enum v4l2_colorfx { > #define V4L2_CID_ALPHA_COMPONENT (V4L2_CID_BASE+41) > #define V4L2_CID_COLORFX_CBCR (V4L2_CID_BASE+42) > > +#define V4L2_CID_TEMPERATURE (V4L2_CID_BASE+43) > + > /* last CID + 1 */ > -#define V4L2_CID_LASTP1 (V4L2_CID_BASE+43) > +#define V4L2_CID_LASTP1 (V4L2_CID_BASE+44) > > /* USER-class private control IDs */ > -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver 2020-04-14 20:01 [RFC PATCH 0/3] v4l2 api changes for imx378 driver Daniel Gomez 2020-04-14 20:01 ` [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum Daniel Gomez 2020-04-14 20:01 ` [RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature Daniel Gomez @ 2020-04-14 20:01 ` Daniel Gomez 2020-04-15 0:37 ` kbuild test robot 2020-04-28 21:02 ` Sakari Ailus 2 siblings, 2 replies; 16+ messages in thread From: Daniel Gomez @ 2020-04-14 20:01 UTC (permalink / raw) To: mchehab, hverkuil-cisco, linux-media; +Cc: linux-kernel, Daniel Gomez Add a V4L2 sub-device driver for the Sony IMX378 image sensor. This is a camera sensor using the I2C bus for control and the CSI-2 bus for data. Signed-off-by: Daniel Gomez <daniel@qtec.com> --- drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/imx378.c | 1829 ++++++++++++++++++++++++++++++++++++ 3 files changed, 1841 insertions(+) create mode 100644 drivers/media/i2c/imx378.c diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index efd12bf4f8eb..8ea630275faf 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -796,6 +796,17 @@ config VIDEO_IMX355 To compile this driver as a module, choose M here: the module will be called imx355. +config VIDEO_IMX378 + tristate "Sony IMX378 sensor support" + depends on GPIOLIB && I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API + depends on V4L2_FWNODE + help + This is a Video4Linux2 sensor driver for the Sony + IMX378 camera. + + To compile this driver as a module, choose M here: the + module will be called imx378. + config VIDEO_OV2640 tristate "OmniVision OV2640 sensor support" depends on VIDEO_V4L2 && I2C diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 77bf7d0b691f..dff1c9ec444f 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -117,6 +117,7 @@ obj-$(CONFIG_VIDEO_IMX274) += imx274.o obj-$(CONFIG_VIDEO_IMX290) += imx290.o obj-$(CONFIG_VIDEO_IMX319) += imx319.o obj-$(CONFIG_VIDEO_IMX355) += imx355.o +obj-$(CONFIG_VIDEO_IMX378) += imx378.o obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o obj-$(CONFIG_SDR_MAX2175) += max2175.o diff --git a/drivers/media/i2c/imx378.c b/drivers/media/i2c/imx378.c new file mode 100644 index 000000000000..6e22c3b45f72 --- /dev/null +++ b/drivers/media/i2c/imx378.c @@ -0,0 +1,1829 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * imx378.c - imx378 sensor driver + * + * Copyright 2019 Qtechnology A/S + * + * Daniel Gomez <daniel@qtec.com> + * + * Based on imx214.c and imx355.c + */ +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <media/media-entity.h> +#include <media/v4l2-ctrls.h> +#include <media/v4l2-fwnode.h> +#include <media/v4l2-subdev.h> + +#define IMX378_DEFAULT_CLK_FREQ 24000000 +#define IMX378_DEFAULT_LINK_FREQ 480000000 +#define IMX378_DEFAULT_LINK_MFREQ (IMX378_DEFAULT_LINK_FREQ / 1000000) +#define IMX378_DEFAULT_PIXEL_RATE ((IMX378_DEFAULT_LINK_FREQ * 8LL) / 10) +#define IMX378_DEFAULT_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10 +#define IMX378_DEFAULT_FIVAL {1, 30} +#define IMX378_LINE_LENGTH 4444 +#define IMX378_MIN_WIDTH 4 +#define IMX378_MIN_HEIGHT 4 +#define IMX378_MAX_DEFAULT_WIDTH 4032 +#define IMX378_MAX_DEFAULT_HEIGHT 3040 +#define IMX378_MAX_BOUNDS_WIDTH 4032 +#define IMX378_MAX_BOUNDS_HEIGHT 3104 +#define IMX378_CID_CUSTOM_BASE (V4L2_CID_USER_BASE | 0xf000) +#define IMX378_CID_GREEN_BALANCE (IMX378_CID_CUSTOM_BASE + 0) + +static const char * const imx378_supply_name[] = { + "vdda", + "vddd", + "vdddo", +}; + +#define IMX378_NUM_SUPPLIES ARRAY_SIZE(imx378_supply_name) + +struct imx378 { + struct device *dev; + struct clk *xclk; + struct regmap *regmap; + + struct v4l2_subdev sd; + struct media_pad pad; + struct v4l2_mbus_framefmt fmt; + struct v4l2_rect crop; + + struct v4l2_ctrl_handler ctrls; + struct v4l2_ctrl *pixel_rate; + struct v4l2_ctrl *link_freq; + struct v4l2_ctrl *exposure; + struct v4l2_ctrl *vflip; + struct v4l2_ctrl *hflip; + struct v4l2_ctrl *analogue_gain; + struct v4l2_ctrl *digital_gain; + struct v4l2_ctrl *red_balance; + struct v4l2_ctrl *blue_balance; + struct v4l2_ctrl *green_balance; + struct v4l2_ctrl *temperature; + + struct regulator_bulk_data supplies[IMX378_NUM_SUPPLIES]; + + struct gpio_desc *enable_gpio; + + struct mutex mutex; + + struct v4l2_fract fival; + + bool streaming; +}; + +struct reg_8 { + u16 addr; + u8 val; +}; + +enum { + IMX378_TABLE_WAIT_MS = 0, + IMX378_TABLE_END, + IMX378_MAX_RETRIES, + IMX378_WAIT_MS +}; + +static const struct reg_8 mode_table_common[] = { + /* software standby settings */ + {0x0100, 0x00}, + + /* software reset */ + {0x0103, 0x01}, + + /* CSI */ + {0x0112, 0x0A}, + {0x0113, 0x0A}, + {0x0114, 0x03}, + + /* global settings */ + {0x0136, 0x18}, + {0x0137, 0x00}, + + {0x0202, 0x20}, + {0x0203, 0xa0}, + {0x0204, 0x03}, + {0x0205, 0xd2}, + {0x020e, 0x01}, + {0x020f, 0x00}, + {0x0210, 0x02}, + {0x0211, 0x21}, + {0x0212, 0x01}, + {0x0213, 0xee}, + {0x0214, 0x01}, + {0x0215, 0x00}, + {0x0220, 0x00}, + {0x0221, 0x11}, + + {0x0301, 0x05}, + {0x0303, 0x02}, + {0x0305, 0x03}, + {0x0306, 0x00}, + {0x0307, 0x96}, + + {0x0309, 0x0a}, + {0x030b, 0x01}, + {0x030d, 0x02}, + {0x030e, 0x00}, + {0x030f, 0xf6}, + {0x0310, 0x00}, + {0x0340, 0x04}, + {0x0341, 0xb0}, + {0x0342, 0x20}, + {0x0343, 0x08}, + {0x0344, 0x00}, + {0x0345, 0x00}, + {0x0346, 0x01}, + {0x0347, 0x78}, + {0x0348, 0x0f}, + {0x0349, 0xd7}, + {0x0350, 0x01}, + {0x034a, 0x0a}, + {0x034b, 0x67}, + {0x034c, 0x07}, + {0x034d, 0x80}, + {0x034e, 0x04}, + {0x034f, 0x38}, + {0x0381, 0x01}, + {0x0383, 0x01}, + {0x0385, 0x01}, + {0x0387, 0x01}, + + {0x0401, 0x00}, + {0x0404, 0x00}, + {0x0405, 0x10}, + {0x0408, 0x00}, + {0x0409, 0x6c}, + {0x040a, 0x00}, + {0x040b, 0x40}, + {0x040c, 0x07}, + {0x040d, 0x80}, + {0x040e, 0x04}, + {0x040f, 0x38}, + + {0x0900, 0x00}, + {0x0901, 0x11}, + {0x0902, 0x02}, + + {0x0c1a, 0x01}, + {0x0c1b, 0x01}, + + {0x3030, 0x01}, + {0x3032, 0x00}, + {0x3033, 0x40}, + {0x3140, 0x02}, + {0x38a8, 0x1f}, + {0x38a9, 0xff}, + {0x38aa, 0x1f}, + {0x38ab, 0xff}, + {0x3c00, 0x00}, + {0x3c01, 0x03}, + {0x3c02, 0xdc}, + {0x3d8a, 0x01}, + {0x3e20, 0x01}, + {0x3e37, 0x01}, + {0x3f0d, 0x00}, + {0x3f50, 0x00}, + {0x3f56, 0x00}, + {0x3f57, 0x01}, + {0x3ff9, 0x00}, + + {0x4421, 0x08}, + {0x4ae9, 0x18}, + {0x4ae9, 0x21}, + {0x4aea, 0x08}, + {0x4aea, 0x80}, + + {0x55d4, 0x00}, + {0x55d5, 0x00}, + {0x55d6, 0x07}, + {0x55d7, 0xff}, + {0x55e8, 0x07}, + {0x55e9, 0xff}, + {0x55ea, 0x00}, + {0x55eb, 0x00}, + {0x5748, 0x07}, + {0x5749, 0xff}, + {0x574a, 0x00}, + {0x574b, 0x00}, + {0x574c, 0x07}, + {0x574d, 0xff}, + {0x574e, 0x00}, + {0x574f, 0x00}, + {0x5754, 0x00}, + {0x5755, 0x00}, + {0x5756, 0x07}, + {0x5757, 0xff}, + {0x5973, 0x04}, + {0x5974, 0x01}, + {0x5d13, 0xc3}, + {0x5d14, 0x58}, + {0x5d15, 0xa3}, + {0x5d16, 0x1d}, + {0x5d17, 0x65}, + {0x5d18, 0x8c}, + {0x5d1a, 0x06}, + {0x5d1b, 0xa9}, + {0x5d1c, 0x45}, + {0x5d1d, 0x3a}, + {0x5d1e, 0xab}, + {0x5d1f, 0x15}, + {0x5d21, 0x0e}, + {0x5d22, 0x52}, + {0x5d23, 0xaa}, + {0x5d24, 0x7d}, + {0x5d25, 0x57}, + {0x5d26, 0xa8}, + {0x5d37, 0x5a}, + {0x5d38, 0x5a}, + {0x5d77, 0x7f}, + + {0x7b3b, 0x01}, + {0x7b4c, 0x00}, + {0x7b53, 0x01}, + {0x7b75, 0x0e}, + {0x7b76, 0x0b}, + {0x7b77, 0x08}, + {0x7b78, 0x0a}, + {0x7b79, 0x47}, + {0x7b7c, 0x00}, + {0x7b7d, 0x00}, + + {0x8d1f, 0x00}, + {0x8d27, 0x00}, + + {0x9004, 0x03}, + {0x9200, 0x50}, + {0x9201, 0x6c}, + {0x9202, 0x71}, + {0x9203, 0x00}, + {0x9204, 0x71}, + {0x9205, 0x01}, + {0x9304, 0x03}, + {0x9305, 0x00}, + {0x9369, 0x5a}, + {0x936b, 0x55}, + {0x936d, 0x28}, + {0x9371, 0x6a}, + {0x9373, 0x6a}, + {0x9375, 0x64}, + {0x9905, 0x00}, + {0x9907, 0x00}, + {0x9909, 0x00}, + {0x990b, 0x00}, + {0x991a, 0x00}, + {0x9944, 0x3c}, + {0x9947, 0x3c}, + {0x994a, 0x8c}, + {0x994b, 0x50}, + {0x994c, 0x1b}, + {0x994d, 0x8c}, + {0x994e, 0x50}, + {0x994f, 0x1b}, + {0x9950, 0x8c}, + {0x9951, 0x1b}, + {0x9952, 0x0a}, + {0x9953, 0x8c}, + {0x9954, 0x1b}, + {0x9955, 0x0a}, + {0x996b, 0x8c}, + {0x996c, 0x64}, + {0x996d, 0x50}, + {0x9a13, 0x04}, + {0x9a14, 0x04}, + {0x9a19, 0x00}, + {0x9a1c, 0x04}, + {0x9a1d, 0x04}, + {0x9a26, 0x05}, + {0x9a27, 0x05}, + {0x9a2c, 0x01}, + {0x9a2d, 0x03}, + {0x9a2f, 0x05}, + {0x9a30, 0x05}, + {0x9a41, 0x00}, + {0x9a46, 0x00}, + {0x9a47, 0x00}, + {0x9a4c, 0x0d}, + {0x9a4d, 0x0d}, + {0x9c17, 0x35}, + {0x9c1d, 0x31}, + {0x9c29, 0x50}, + {0x9c3b, 0x2f}, + {0x9c41, 0x6b}, + {0x9c47, 0x2d}, + {0x9c4d, 0x40}, + {0x9c6b, 0x00}, + {0x9c71, 0xc8}, + {0x9c73, 0x32}, + {0x9c75, 0x04}, + {0x9c7d, 0x2d}, + {0x9c83, 0x40}, + {0x9c94, 0x3f}, + {0x9c95, 0x3f}, + {0x9c96, 0x3f}, + {0x9c97, 0x00}, + {0x9c98, 0x00}, + {0x9c99, 0x00}, + {0x9c9a, 0x3f}, + {0x9c9b, 0x3f}, + {0x9c9c, 0x3f}, + {0x9ca0, 0x0f}, + {0x9ca1, 0x0f}, + {0x9ca2, 0x0f}, + {0x9ca3, 0x00}, + {0x9ca4, 0x00}, + {0x9ca5, 0x00}, + {0x9ca6, 0x1e}, + {0x9ca7, 0x1e}, + {0x9ca8, 0x1e}, + {0x9ca9, 0x00}, + {0x9caa, 0x00}, + {0x9cab, 0x00}, + {0x9cac, 0x09}, + {0x9cad, 0x09}, + {0x9cae, 0x09}, + {0x9cbd, 0x50}, + {0x9cbf, 0x50}, + {0x9cc1, 0x50}, + {0x9cc3, 0x40}, + {0x9cc5, 0x40}, + {0x9cc7, 0x40}, + {0x9cc9, 0x0a}, + {0x9ccb, 0x0a}, + {0x9ccd, 0x0a}, + {0x9d17, 0x35}, + {0x9d1d, 0x31}, + {0x9d29, 0x50}, + {0x9d3b, 0x2f}, + {0x9d41, 0x6b}, + {0x9d47, 0x42}, + {0x9d4d, 0x5a}, + {0x9d6b, 0x00}, + {0x9d71, 0xc8}, + {0x9d73, 0x32}, + {0x9d75, 0x04}, + {0x9d7d, 0x42}, + {0x9d83, 0x5a}, + {0x9d94, 0x3f}, + {0x9d95, 0x3f}, + {0x9d96, 0x3f}, + {0x9d97, 0x00}, + {0x9d98, 0x00}, + {0x9d99, 0x00}, + {0x9d9a, 0x3f}, + {0x9d9b, 0x3f}, + {0x9d9c, 0x3f}, + {0x9d9d, 0x1f}, + {0x9d9e, 0x1f}, + {0x9d9f, 0x1f}, + {0x9da0, 0x0f}, + {0x9da1, 0x0f}, + {0x9da2, 0x0f}, + {0x9da3, 0x00}, + {0x9da4, 0x00}, + {0x9da5, 0x00}, + {0x9da6, 0x1e}, + {0x9da7, 0x1e}, + {0x9da8, 0x1e}, + {0x9da9, 0x00}, + {0x9daa, 0x00}, + {0x9dab, 0x00}, + {0x9dac, 0x09}, + {0x9dad, 0x09}, + {0x9dae, 0x09}, + {0x9dc9, 0x0a}, + {0x9dcb, 0x0a}, + {0x9dcd, 0x0a}, + {0x9e17, 0x35}, + {0x9e1d, 0x31}, + {0x9e29, 0x50}, + {0x9e3b, 0x2f}, + {0x9e41, 0x6b}, + {0x9e47, 0x2d}, + {0x9e4d, 0x40}, + {0x9e6b, 0x00}, + {0x9e71, 0xc8}, + {0x9e73, 0x32}, + {0x9e75, 0x04}, + {0x9e94, 0x0f}, + {0x9e95, 0x0f}, + {0x9e96, 0x0f}, + {0x9e97, 0x00}, + {0x9e98, 0x00}, + {0x9e99, 0x00}, + {0x9e9a, 0x2f}, + {0x9e9b, 0x2f}, + {0x9e9c, 0x2f}, + {0x9e9d, 0x00}, + {0x9e9e, 0x00}, + {0x9e9f, 0x00}, + {0x9ea0, 0x0f}, + {0x9ea1, 0x0f}, + {0x9ea2, 0x0f}, + {0x9ea3, 0x00}, + {0x9ea4, 0x00}, + {0x9ea5, 0x00}, + {0x9ea6, 0x3f}, + {0x9ea7, 0x3f}, + {0x9ea8, 0x3f}, + {0x9ea9, 0x00}, + {0x9eaa, 0x00}, + {0x9eab, 0x00}, + {0x9eac, 0x09}, + {0x9ead, 0x09}, + {0x9eae, 0x09}, + {0x9ec9, 0x0a}, + {0x9ecb, 0x0a}, + {0x9ecd, 0x0a}, + {0x9f17, 0x35}, + {0x9f1d, 0x31}, + {0x9f29, 0x50}, + {0x9f3b, 0x2f}, + {0x9f41, 0x6b}, + {0x9f47, 0x42}, + {0x9f4d, 0x5a}, + {0x9f6b, 0x00}, + {0x9f71, 0xc8}, + {0x9f73, 0x32}, + {0x9f75, 0x04}, + {0x9f94, 0x0f}, + {0x9f95, 0x0f}, + {0x9f96, 0x0f}, + {0x9f97, 0x00}, + {0x9f98, 0x00}, + {0x9f99, 0x00}, + {0x9f9a, 0x2f}, + {0x9f9b, 0x2f}, + {0x9f9c, 0x2f}, + {0x9f9d, 0x00}, + {0x9f9e, 0x00}, + {0x9f9f, 0x00}, + {0x9fa0, 0x0f}, + {0x9fa1, 0x0f}, + {0x9fa2, 0x0f}, + {0x9fa3, 0x00}, + {0x9fa4, 0x00}, + {0x9fa5, 0x00}, + {0x9fa6, 0x1e}, + {0x9fa7, 0x1e}, + {0x9fa8, 0x1e}, + {0x9fa9, 0x00}, + {0x9faa, 0x00}, + {0x9fab, 0x00}, + {0x9fac, 0x09}, + {0x9fad, 0x09}, + {0x9fae, 0x09}, + {0x9fc9, 0x0a}, + {0x9fcb, 0x0a}, + {0x9fcd, 0x0a}, + + {0xa001, 0x0a}, + {0xa003, 0x0a}, + {0xa005, 0x0a}, + {0xa006, 0x01}, + {0xa007, 0xc0}, + {0xa009, 0xc0}, + {0xa14b, 0xff}, + {0xa151, 0x0c}, + {0xa153, 0x50}, + {0xa155, 0x02}, + {0xa157, 0x00}, + {0xa1ad, 0xff}, + {0xa1b3, 0x0c}, + {0xa1b5, 0x50}, + {0xa1b9, 0x00}, + {0xa24b, 0xff}, + {0xa257, 0x00}, + {0xa2a9, 0x60}, + {0xa2ad, 0xff}, + {0xa2b7, 0x00}, + {0xa2b9, 0x00}, + + {0xb21f, 0x04}, + {0xb35c, 0x00}, + {0xb35e, 0x08}, + {0xbcf1, 0x02}, + {0xe000, 0x00}, + {0xf61c, 0x04}, + {0xf61e, 0x04}, + + {IMX378_TABLE_END, 0x00} +}; + +static inline struct imx378 *to_imx378(struct v4l2_subdev *sd) +{ + return container_of(sd, struct imx378, sd); +} + +static int imx378_set_flip_mode(struct imx378 *imx378, u32 code) +{ + /* WORKAROUND to update controls */ + imx378->vflip->flags &= ~V4L2_CTRL_FLAG_READ_ONLY; + imx378->hflip->flags &= ~V4L2_CTRL_FLAG_READ_ONLY; + + switch (code) { + case MEDIA_BUS_FMT_SBGGR10_1X10: + v4l2_ctrl_s_ctrl(imx378->vflip, 1); + v4l2_ctrl_s_ctrl(imx378->hflip, 1); + break; + case MEDIA_BUS_FMT_SGBRG10_1X10: + v4l2_ctrl_s_ctrl(imx378->vflip, 1); + v4l2_ctrl_s_ctrl(imx378->hflip, 0); + break; + case MEDIA_BUS_FMT_SGRBG10_1X10: + v4l2_ctrl_s_ctrl(imx378->vflip, 0); + v4l2_ctrl_s_ctrl(imx378->hflip, 1); + break; + case MEDIA_BUS_FMT_SRGGB10_1X10: + v4l2_ctrl_s_ctrl(imx378->vflip, 0); + v4l2_ctrl_s_ctrl(imx378->hflip, 0); + break; + default: + break; + } + + /* WORKAROUND to update controls */ + imx378->vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY; + imx378->hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + return 0; +} + +static u32 imx378_get_format_code(struct imx378 *imx378) +{ + /* + * Only one bayer order is supported. + * It depends on the flip settings. + */ + u32 code; + static const u32 codes[2][2] = { + { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, }, + { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, }, + }; + + lockdep_assert_held(&imx378->mutex); + code = codes[imx378->vflip->val][imx378->hflip->val]; + + return code; +} + +static int imx378_read_reg16(struct imx378 *imx378, u16 addr, u16 *val) +{ + int ret; + u32 h_val = 0, l_val = 0; + + ret = regmap_read(imx378->regmap, addr + 1, &l_val); + if (ret) { + pr_err("%s:i2c read failed, %x\n", __func__, addr + 1); + return ret; + } + + *val = l_val & 0xFF; + + ret = regmap_read(imx378->regmap, addr, &h_val); + if (ret) { + pr_err("%s:i2c read failed, %x\n", __func__, addr); + return ret; + } + + *val |= (h_val & 0xFF) << 8; + + return ret; +} + +static int imx378_write_reg16(struct imx378 *imx378, u16 addr, u16 val) +{ + int ret; + + ret = regmap_write(imx378->regmap, addr + 1, val); + if (ret) { + pr_err("%s:i2c write failed, %x = %x\n", + __func__, addr + 1, val); + return ret; + } + + ret = regmap_write(imx378->regmap, addr, val >> 8); + if (ret) { + pr_err("%s:i2c write failed, %x = %x\n", + __func__, addr, val >> 8); + return ret; + } + + return ret; +} + +static int __maybe_unused imx378_power_on(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct imx378 *imx378 = to_imx378(sd); + int ret; + + gpiod_set_value_cansleep(imx378->enable_gpio, 0); + + ret = regulator_bulk_enable(IMX378_NUM_SUPPLIES, imx378->supplies); + if (ret < 0) { + dev_err(imx378->dev, "failed to enable regulators: %d\n", ret); + return ret; + } + + usleep_range(2000, 3000); + + ret = clk_prepare_enable(imx378->xclk); + if (ret < 0) { + regulator_bulk_disable(IMX378_NUM_SUPPLIES, imx378->supplies); + dev_err(imx378->dev, "clk prepare enable failed\n"); + return ret; + } + + gpiod_set_value_cansleep(imx378->enable_gpio, 1); + usleep_range(12000, 15000); + + return 0; +} + +static int __maybe_unused imx378_power_off(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct imx378 *imx378 = to_imx378(sd); + + gpiod_set_value_cansleep(imx378->enable_gpio, 0); + + clk_disable_unprepare(imx378->xclk); + + regulator_bulk_disable(IMX378_NUM_SUPPLIES, imx378->supplies); + usleep_range(10, 20); + + return 0; +} + +static int imx378_enum_mbus_code(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_mbus_code_enum *code) +{ + struct imx378 *imx378 = container_of(sd, struct imx378, sd); + + if (code->index != 0 || code->pad != 0) + return -EINVAL; + + code->code = imx378_get_format_code(imx378); + + return 0; +} + +static int imx378_enum_frame_size(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_frame_size_enum *fse) +{ + struct imx378 *imx378 = container_of(sd, struct imx378, sd); + + if (fse->index != 0) + return -EINVAL; + + mutex_lock(&imx378->mutex); + if (fse->code != imx378_get_format_code(imx378)) { + mutex_unlock(&imx378->mutex); + return -EINVAL; + } + mutex_unlock(&imx378->mutex); + + fse->min_width = IMX378_MIN_WIDTH; + fse->max_width = IMX378_MAX_BOUNDS_WIDTH; + fse->min_height = IMX378_MIN_HEIGHT; + fse->max_height = IMX378_MAX_BOUNDS_HEIGHT; + + return 0; +} + +#ifdef CONFIG_VIDEO_ADV_DEBUG +static int imx378_s_register(struct v4l2_subdev *sd, + const struct v4l2_dbg_register *reg) +{ + struct imx378 *imx378 = container_of(sd, struct imx378, sd); + + return regmap_write(imx378->regmap, reg->reg, reg->val); +} + +static int imx378_g_register(struct v4l2_subdev *sd, + struct v4l2_dbg_register *reg) +{ + struct imx378 *imx378 = container_of(sd, struct imx378, sd); + unsigned int aux; + int ret; + + reg->size = 1; + ret = regmap_read(imx378->regmap, reg->reg, &aux); + reg->val = aux; + + return ret; +} +#endif + +static const struct v4l2_subdev_core_ops imx378_core_ops = { +#ifdef CONFIG_VIDEO_ADV_DEBUG + .g_register = imx378_g_register, + .s_register = imx378_s_register, +#endif +}; + +static int imx378_set_img_size(struct imx378 *imx378) +{ + u32 frame_length; + int ret; + + ret = imx378_write_reg16(imx378, 0x34c, imx378->fmt.width); + if (ret < 0) + goto error; + + ret = imx378_write_reg16(imx378, 0x34e, imx378->fmt.height); + if (ret < 0) + goto error; + + ret = imx378_write_reg16(imx378, 0x40c, imx378->fmt.width); + if (ret < 0) + goto error; + + ret = imx378_write_reg16(imx378, 0x40e, imx378->fmt.height); + if (ret < 0) + goto error; + + ret = imx378_write_reg16(imx378, 0x342, IMX378_LINE_LENGTH); + if (ret < 0) + goto error; + + frame_length = IMX378_DEFAULT_LINK_FREQ / + ((imx378->fival.denominator / imx378->fival.numerator) + * IMX378_LINE_LENGTH); + + ret = imx378_write_reg16(imx378, 0x340, frame_length); + if (ret < 0) + goto error; + + ret = imx378_write_reg16(imx378, 0x344, imx378->crop.left); + if (ret < 0) + goto error; + + ret = imx378_write_reg16(imx378, 0x348, imx378->crop.left + + imx378->fmt.width - 1); + if (ret < 0) + goto error; + + ret = imx378_write_reg16(imx378, 0x346, imx378->crop.top); + if (ret < 0) + goto error; + + ret = imx378_write_reg16(imx378, 0x34a, imx378->crop.top + + imx378->fmt.height - 1); + if (ret < 0) + goto error; + + ret = imx378_write_reg16(imx378, 0x408, 0); + if (ret < 0) + goto error; + + ret = imx378_write_reg16(imx378, 0x40a, 0); + if (ret < 0) + goto error; + + ret = regmap_write(imx378->regmap, 0xb04, 1); + if (ret < 0) + goto error; + + return ret; + +error: + dev_err(imx378->dev, "could not set image size %d\n", ret); + return ret; +} + +static struct v4l2_mbus_framefmt * +__imx378_get_pad_format(struct imx378 *imx378, + struct v4l2_subdev_pad_config *cfg, + unsigned int pad, + enum v4l2_subdev_format_whence which) +{ + switch (which) { + case V4L2_SUBDEV_FORMAT_TRY: + return v4l2_subdev_get_try_format(&imx378->sd, cfg, pad); + case V4L2_SUBDEV_FORMAT_ACTIVE: + return &imx378->fmt; + default: + return NULL; + } +} + +static int imx378_get_fmt(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_format *fmt) +{ + struct imx378 *imx378 = to_imx378(sd); + + mutex_lock(&imx378->mutex); + fmt->format = *__imx378_get_pad_format(imx378, cfg, fmt->pad, + fmt->which); + mutex_unlock(&imx378->mutex); + + return 0; +} + +static struct v4l2_rect * +__imx378_get_pad_crop(struct imx378 *imx378, struct v4l2_subdev_pad_config *cfg, + unsigned int pad, enum v4l2_subdev_format_whence which) +{ + switch (which) { + case V4L2_SUBDEV_FORMAT_TRY: + return v4l2_subdev_get_try_crop(&imx378->sd, cfg, pad); + case V4L2_SUBDEV_FORMAT_ACTIVE: + return &imx378->crop; + default: + return NULL; + } +} + +static int imx378_min_interval(struct imx378 *imx378, + struct v4l2_fract *fival) +{ + fival->numerator = IMX378_LINE_LENGTH * (imx378->fmt.height); + fival->denominator = IMX378_DEFAULT_LINK_FREQ; + + return 0; +} + +static int imx378_max_interval(struct v4l2_fract *fival) +{ + fival->numerator = IMX378_LINE_LENGTH * 0xffff; + fival->denominator = IMX378_DEFAULT_LINK_FREQ; + + return 0; +} + +#define FRACT_CMP(a, OP, b) \ + ((u64)(a).numerator * (b).denominator OP \ + (u64)(b).numerator * (a).denominator) +static int imx378_clamp_interval(struct imx378 *imx378, + struct v4l2_fract *fival) +{ + struct v4l2_fract interval; + int ret; + + ret = imx378_min_interval(imx378, &interval); + if (ret) + return ret; + if (FRACT_CMP(*fival, <, interval)) + *fival = interval; + + ret = imx378_max_interval(&interval); + if (ret) + return ret; + if (FRACT_CMP(*fival, >, interval)) + *fival = interval; + + return 0; +} + +static int imx378_g_frame_interval(struct v4l2_subdev *sd, + struct v4l2_subdev_frame_interval *fival) +{ + struct imx378 *imx378 = to_imx378(sd); + + fival->interval = imx378->fival; + + return 0; +} + +static int imx378_s_frame_interval(struct v4l2_subdev *sd, + struct v4l2_subdev_frame_interval *fival) +{ + struct imx378 *imx378 = to_imx378(sd); + u64 max_exposure; + + if (fival->interval.denominator == 0) + fival->interval = (struct v4l2_fract) IMX378_DEFAULT_FIVAL; + + imx378_clamp_interval(imx378, &fival->interval); + + imx378->fival = fival->interval; + + /* Update exposure limits */ + max_exposure = (IMX378_DEFAULT_LINK_FREQ / + ((imx378->fival.denominator / imx378->fival.numerator) + * IMX378_LINE_LENGTH)) - 20; + + __v4l2_ctrl_modify_range(imx378->exposure, imx378->exposure->minimum, + max_exposure, imx378->exposure->step, + max_exposure); + + return 0; +} + +static int +imx378_enum_frame_interval(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_frame_interval_enum *fie) +{ + struct imx378 *imx378 = to_imx378(sd); + int ret; + + if (fie->index != 0) + return -EINVAL; + + if (fie->width > IMX378_MAX_BOUNDS_WIDTH || + fie->width < IMX378_MIN_WIDTH) + return -EINVAL; + + if (fie->height > IMX378_MAX_BOUNDS_HEIGHT || + fie->height < IMX378_MIN_HEIGHT) + return -EINVAL; + + mutex_lock(&imx378->mutex); + if (fie->code != imx378_get_format_code(imx378)) { + mutex_unlock(&imx378->mutex); + return -EINVAL; + } + mutex_unlock(&imx378->mutex); + + ret = imx378_min_interval(imx378, &fie->min_interval); + if (ret) + return ret; + + ret = imx378_max_interval(&fie->max_interval); + if (ret) + return ret; + + fie->interval = (struct v4l2_fract) IMX378_DEFAULT_FIVAL; + + return 0; +} + +static int imx378_set_fmt(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_format *fmt) +{ + struct imx378 *imx378 = to_imx378(sd); + struct v4l2_mbus_framefmt *__fmt; + struct v4l2_rect *__crop; + struct v4l2_subdev_frame_interval fival = { }; + + mutex_lock(&imx378->mutex); + + if (imx378->streaming && fmt->which != V4L2_SUBDEV_FORMAT_TRY) + return -EBUSY; + + __crop = __imx378_get_pad_crop(imx378, cfg, fmt->pad, fmt->which); + + v4l_bound_align_image(&fmt->format.width, IMX378_MIN_WIDTH, + IMX378_MAX_BOUNDS_WIDTH, ilog2(4), + &fmt->format.height, IMX378_MIN_HEIGHT, + IMX378_MAX_BOUNDS_HEIGHT, ilog2(8), 0); + + __crop->width = fmt->format.width; + __crop->height = fmt->format.height; + + v4l_bound_align_image(&__crop->left, 0, + IMX378_MAX_BOUNDS_WIDTH - __crop->width, ilog2(4), + &__crop->top, 0, + IMX378_MAX_BOUNDS_HEIGHT - __crop->height, + ilog2(8), 0); + + __fmt = __imx378_get_pad_format(imx378, cfg, fmt->pad, fmt->which); + + __fmt->width = __crop->width; + __fmt->height = __crop->height; + + if (fmt->format.code && fmt->which != V4L2_SUBDEV_FORMAT_TRY) + imx378_set_flip_mode(imx378, fmt->format.code); + + __fmt->code = imx378_get_format_code(imx378); + + __fmt->field = V4L2_FIELD_NONE; + __fmt->colorspace = V4L2_COLORSPACE_SRGB; + __fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->format.colorspace); + __fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true, + __fmt->colorspace, __fmt->ycbcr_enc); + __fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(__fmt->colorspace); + + fmt->format = *__fmt; + + /* Frame interval depends on the format so, update it accordingly */ + if (fmt->which != V4L2_SUBDEV_FORMAT_TRY) { + fival.interval = imx378->fival; + imx378_s_frame_interval(sd, &fival); + } + + mutex_unlock(&imx378->mutex); + + return 0; +} + +static int imx378_get_selection(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_selection *s) +{ + struct imx378 *imx378 = to_imx378(sd); + + switch (s->target) { + case V4L2_SEL_TGT_CROP: + case V4L2_SEL_TGT_COMPOSE: + s->r = imx378->crop; + return 0; + case V4L2_SEL_TGT_COMPOSE_DEFAULT: + case V4L2_SEL_TGT_CROP_DEFAULT: + s->r.width = IMX378_MAX_DEFAULT_WIDTH; + s->r.height = IMX378_MAX_DEFAULT_HEIGHT; + s->r.left = 0; + s->r.top = 0; + return 0; + case V4L2_SEL_TGT_COMPOSE_BOUNDS: + case V4L2_SEL_TGT_CROP_BOUNDS: + s->r.width = IMX378_MAX_BOUNDS_WIDTH; + s->r.height = IMX378_MAX_BOUNDS_HEIGHT; + s->r.left = 0; + s->r.top = 0; + return 0; + default: + return -EINVAL; + } + + return 0; +} + +static int imx378_set_selection(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_selection *s) +{ + struct imx378 *imx378 = to_imx378(sd); + int ret; + + if (s->target == V4L2_SEL_TGT_COMPOSE) + return 0; + + if (s->target != V4L2_SEL_TGT_CROP) + return -EINVAL; + + mutex_lock(&imx378->mutex); + + s->r.width = imx378->crop.width; + s->r.height = imx378->crop.height; + + v4l_bound_align_image(&s->r.left, 0, + IMX378_MAX_BOUNDS_WIDTH - s->r.width, ilog2(4), + &s->r.top, 0, + IMX378_MAX_BOUNDS_HEIGHT - s->r.height, ilog2(8), + 0); + + imx378->crop.left = s->r.left; + imx378->crop.top = s->r.top; + + if (imx378->streaming) { + ret = imx378_set_img_size(imx378); + if (ret < 0) { + dev_err(imx378->dev, "could not set image size\n"); + mutex_unlock(&imx378->mutex); + return ret; + } + } + + mutex_unlock(&imx378->mutex); + return 0; +} + +static int imx378_set_exposure(struct imx378 *imx378) +{ + int ret; + + /* Set group hold */ + ret = regmap_write(imx378->regmap, 0x104, 1); + if (ret < 0) + return ret; + + ret = imx378_write_reg16(imx378, 0x202, imx378->exposure->val); + if (ret < 0) + return ret; + + /* Release group hold */ + return regmap_write(imx378->regmap, 0x104, 0); +} + +static int imx378_set_analogue_gain(struct imx378 *imx378) +{ + int ret; + + /* Set group hold */ + ret = regmap_write(imx378->regmap, 0x104, 1); + if (ret < 0) + return ret; + + ret = imx378_write_reg16(imx378, 0x204, imx378->analogue_gain->val); + if (ret < 0) + return ret; + + /* Release group hold */ + return regmap_write(imx378->regmap, 0x104, 0); +} + +static int imx378_set_digital_gain_global(struct imx378 *imx378) +{ + int ret; + + /* Set group hold */ + ret = regmap_write(imx378->regmap, 0x104, 1); + if (ret < 0) + return ret; + + /* Digital gain control mode all color */ + ret = regmap_write(imx378->regmap, 0x3ff9, 1); + if (ret < 0) + return ret; + + /* Set digital gain globally */ + ret = imx378_write_reg16(imx378, 0x20e, imx378->digital_gain->val); + if (ret < 0) + return ret; + + /* Release group hold */ + return regmap_write(imx378->regmap, 0x104, 0); +} + +static int imx378_set_digital_gain_by_color(struct imx378 *imx378) +{ + int ret; + + /* Set group hold */ + ret = regmap_write(imx378->regmap, 0x104, 1); + if (ret < 0) + return ret; + + /* Digital gain control mode by color */ + ret = regmap_write(imx378->regmap, 0x3ff9, 0); + if (ret < 0) + return ret; + + /* Digital gain R */ + ret = imx378_write_reg16(imx378, 0x210, imx378->red_balance->val); + if (ret < 0) + return ret; + + /* Digital gain B */ + ret = imx378_write_reg16(imx378, 0x212, imx378->blue_balance->val); + if (ret < 0) + return ret; + + /* Digital gain GR */ + ret = imx378_write_reg16(imx378, 0x20e, imx378->green_balance->val); + if (ret < 0) + return ret; + + /* Digital gain GB */ + ret = imx378_write_reg16(imx378, 0x214, imx378->green_balance->val); + if (ret < 0) + return ret; + + /* Release group hold */ + return regmap_write(imx378->regmap, 0x104, 0); +} + +static int imx378_g_temperature(struct imx378 *imx378, s32 *temperature) +{ + unsigned int aux; + int ret; + + if (imx378->streaming) { + ret = regmap_read(imx378->regmap, 0x13a, &aux); + if (ret < 0) + return ret; + + if (aux >= 0xec) + *temperature = (aux - 0xec - 20) * 1000; + else + *temperature = (aux * 1000); + } + + dev_err(imx378->dev, + "Sensor is powered off, returning invalid temperature\n"); + + return 0; +} + +static int imx378_g_volatile_ctrl(struct v4l2_ctrl *ctrl) +{ + struct imx378 *imx378 = container_of(ctrl->handler, + struct imx378, ctrls); + int ret; + + /* + * Applying V4L2 control value only happens + * when power is up for streaming + */ + if (!pm_runtime_get_if_in_use(imx378->dev)) + return 0; + + switch (ctrl->id) { + case V4L2_CID_TEMPERATURE: + ret = imx378_g_temperature(imx378, &ctrl->val); + break; + default: + ret = -EINVAL; + break; + } + + pm_runtime_put(imx378->dev); + + return ret; +} + +static int imx378_set_ctrl(struct v4l2_ctrl *ctrl) +{ + struct imx378 *imx378 = container_of(ctrl->handler, + struct imx378, ctrls); + int ret; + + /* + * Applying V4L2 control value only happens + * when power is up for streaming + */ + if (!pm_runtime_get_if_in_use(imx378->dev)) + return 0; + + switch (ctrl->id) { + case V4L2_CID_EXPOSURE_ABSOLUTE: + ret = imx378_set_exposure(imx378); + break; + case V4L2_CID_ANALOGUE_GAIN: + ret = imx378_set_analogue_gain(imx378); + break; + case V4L2_CID_DIGITAL_GAIN: + ret = imx378_set_digital_gain_global(imx378); + break; + case V4L2_CID_RED_BALANCE: + case V4L2_CID_BLUE_BALANCE: + case IMX378_CID_GREEN_BALANCE: + ret = imx378_set_digital_gain_by_color(imx378); + break; + default: + ret = -EINVAL; + break; + } + + pm_runtime_put(imx378->dev); + + return ret; +} + +static const struct v4l2_ctrl_ops imx378_ctrl_ops = { + .g_volatile_ctrl = imx378_g_volatile_ctrl, + .s_ctrl = imx378_set_ctrl, +}; + +#define MAX_CMD 4 +static int imx378_write_table(struct imx378 *imx378, + const struct reg_8 table[]) +{ + u8 vals[MAX_CMD]; + int i, ret; + + for (table = table; table->addr != IMX378_TABLE_END ; table++) { + if (table->addr == IMX378_TABLE_WAIT_MS) { + usleep_range(table->val * 1000, + table->val * 1000 + 500); + continue; + } + + for (i = 0; i < MAX_CMD; i++) { + if (table[i].addr != (table[0].addr + i)) + break; + vals[i] = table[i].val; + } + + ret = regmap_bulk_write(imx378->regmap, table->addr, vals, i); + usleep_range(80, 120); + + if (ret) { + dev_err(imx378->dev, "write_table error: %d\n", ret); + return ret; + } + + table += i - 1; + } + + return 0; +} + +static int imx378_start_streaming(struct imx378 *imx378) +{ + int ret; + + ret = imx378_write_table(imx378, mode_table_common); + if (ret < 0) { + dev_err(imx378->dev, "could not send common table %d\n", ret); + goto error; + } + + ret = __v4l2_ctrl_handler_setup(&imx378->ctrls); + if (ret < 0) { + dev_err(imx378->dev, "could not sync v4l2 controls\n"); + goto error; + } + + ret = imx378_set_img_size(imx378); + if (ret < 0) { + dev_err(imx378->dev, "could not set image size\n"); + goto error; + } + + /* set orientation */ + ret = regmap_write(imx378->regmap, 0x101, + imx378->hflip->val | imx378->vflip->val << 1); + if (ret < 0) + goto error; + + /* set temperature control */ + ret = regmap_write(imx378->regmap, 0x138, 1); + if (ret < 0) + goto error; + + ret = regmap_write(imx378->regmap, 0x100, 1); + if (ret < 0) { + dev_err(imx378->dev, "could not send start table %d\n", ret); + goto error; + } + + usleep_range(4000, 6000); + + return 0; + +error: + mutex_unlock(&imx378->mutex); + return ret; +} + +static int imx378_stop_streaming(struct imx378 *imx378) +{ + int ret; + + ret = regmap_write(imx378->regmap, 0x100, 0); + if (ret < 0) + dev_err(imx378->dev, "could not send stop table %d\n", ret); + + return ret; +} + +static int imx378_s_stream(struct v4l2_subdev *sd, int enable) +{ + struct imx378 *imx378 = to_imx378(sd); + int ret; + + mutex_lock(&imx378->mutex); + + if (imx378->streaming == enable) { + mutex_unlock(&imx378->mutex); + return 0; + } + + if (enable) { + ret = pm_runtime_get_sync(imx378->dev); + if (ret < 0) { + pm_runtime_put_noidle(imx378->dev); + goto err_unlock; + } + + ret = imx378_start_streaming(imx378); + if (ret < 0) + goto err_rpm_put; + } else { + ret = imx378_stop_streaming(imx378); + if (ret < 0) + goto err_rpm_put; + pm_runtime_put(imx378->dev); + } + + imx378->streaming = enable; + + mutex_unlock(&imx378->mutex); + + return 0; + +err_rpm_put: + pm_runtime_put(imx378->dev); +err_unlock: + mutex_unlock(&imx378->mutex); + + return ret; +} + +static int imx378_entity_init_cfg(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg) +{ + struct v4l2_subdev_format fmt = { }; + struct v4l2_subdev_frame_interval fival = { }; + + if (!cfg) { + fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : + V4L2_SUBDEV_FORMAT_ACTIVE; + fmt.format.width = IMX378_MAX_BOUNDS_WIDTH; + fmt.format.height = IMX378_MAX_BOUNDS_HEIGHT; + + imx378_set_fmt(sd, cfg, &fmt); + + fival.interval = (struct v4l2_fract) IMX378_DEFAULT_FIVAL; + imx378_s_frame_interval(sd, &fival); + } + + return 0; +} + +static const struct v4l2_subdev_video_ops imx378_video_ops = { + .s_stream = imx378_s_stream, + .g_frame_interval = imx378_g_frame_interval, + .s_frame_interval = imx378_s_frame_interval, +}; + +static const struct v4l2_subdev_pad_ops imx378_subdev_pad_ops = { + .enum_mbus_code = imx378_enum_mbus_code, + .enum_frame_size = imx378_enum_frame_size, + .enum_frame_interval = imx378_enum_frame_interval, + .get_fmt = imx378_get_fmt, + .set_fmt = imx378_set_fmt, + .get_selection = imx378_get_selection, + .set_selection = imx378_set_selection, + .init_cfg = imx378_entity_init_cfg, +}; + +static const struct v4l2_subdev_ops imx378_subdev_ops = { + .core = &imx378_core_ops, + .video = &imx378_video_ops, + .pad = &imx378_subdev_pad_ops, +}; + +static const struct regmap_config sensor_regmap_config = { + .reg_bits = 16, + .val_bits = 8, + .cache_type = REGCACHE_RBTREE, +}; + +static int imx378_get_regulators(struct device *dev, struct imx378 *imx378) +{ + unsigned int i; + + for (i = 0; i < IMX378_NUM_SUPPLIES; i++) + imx378->supplies[i].supply = imx378_supply_name[i]; + + return devm_regulator_bulk_get(dev, IMX378_NUM_SUPPLIES, + imx378->supplies); +} + +struct v4l2_ctrl +*imx378_ctrl_new_str_custom(struct v4l2_ctrl_handler *hdl, u32 id, char *name, + s64 min, s64 max, u64 step, s64 def) +{ + static struct v4l2_ctrl_config ctrl = { + .ops = &imx378_ctrl_ops, + .type = V4L2_CTRL_TYPE_STRING, + }; + + ctrl.id = id; + ctrl.name = name; + ctrl.min = min; + ctrl.max = max; + ctrl.step = step; + ctrl.def = def; + + return v4l2_ctrl_new_custom(hdl, &ctrl, NULL); +} + +struct v4l2_ctrl +*imx378_ctrl_new_int_custom(struct v4l2_ctrl_handler *hdl, u32 id, char *name, + s64 min, s64 max, u64 step, s64 def) +{ + static struct v4l2_ctrl_config ctrl = { + .ops = &imx378_ctrl_ops, + .type = V4L2_CTRL_TYPE_INTEGER, + }; + + ctrl.id = id; + ctrl.name = name; + ctrl.min = min; + ctrl.max = max; + ctrl.step = step; + ctrl.def = def; + + return v4l2_ctrl_new_custom(hdl, &ctrl, NULL); +} + +static int imx378_init_controls(struct imx378 *imx378) +{ + static const s64 link_freq[] = { + IMX378_DEFAULT_LINK_FREQ, + }; + int ret; + + mutex_init(&imx378->mutex); + imx378->ctrls.lock = &imx378->mutex; + + v4l2_ctrl_handler_init(&imx378->ctrls, 12); + + imx378->pixel_rate = v4l2_ctrl_new_std(&imx378->ctrls, NULL, + V4L2_CID_PIXEL_RATE, 0, + IMX378_DEFAULT_PIXEL_RATE, 1, + IMX378_DEFAULT_PIXEL_RATE); + + imx378->link_freq = v4l2_ctrl_new_int_menu(&imx378->ctrls, NULL, + V4L2_CID_LINK_FREQ, + ARRAY_SIZE(link_freq) - 1, + 0, link_freq); + if (imx378->link_freq) + imx378->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + imx378->exposure = v4l2_ctrl_new_std(&imx378->ctrls, &imx378_ctrl_ops, + V4L2_CID_EXPOSURE_ABSOLUTE, + 0x8, 0xffff, 1, 0xc70); + + imx378->hflip = v4l2_ctrl_new_std(&imx378->ctrls, &imx378_ctrl_ops, + V4L2_CID_HFLIP, 0, 1, 1, 0); + if (imx378->hflip) + imx378->hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + imx378->vflip = v4l2_ctrl_new_std(&imx378->ctrls, &imx378_ctrl_ops, + V4L2_CID_VFLIP, 0, 1, 1, 0); + if (imx378->vflip) + imx378->vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + imx378->analogue_gain = v4l2_ctrl_new_std(&imx378->ctrls, + &imx378_ctrl_ops, + V4L2_CID_ANALOGUE_GAIN, + 0, 978, 1, 0); + + imx378->digital_gain = v4l2_ctrl_new_std(&imx378->ctrls, + &imx378_ctrl_ops, V4L2_CID_DIGITAL_GAIN, + 0x100, 0xfff, 1, 0x100); + + imx378->red_balance = v4l2_ctrl_new_std(&imx378->ctrls, + &imx378_ctrl_ops, V4L2_CID_RED_BALANCE, + 0x100, 0xfff, 1, 0x221); + + imx378->blue_balance = v4l2_ctrl_new_std(&imx378->ctrls, + &imx378_ctrl_ops, V4L2_CID_BLUE_BALANCE, + 0x100, 0xfff, 1, 0x1ee); + + imx378->green_balance = imx378_ctrl_new_int_custom(&imx378->ctrls, + IMX378_CID_GREEN_BALANCE, + "Green Balance", + 0x100, 0xfff, 1, 0x100); + if (imx378->green_balance) + imx378->green_balance->flags = V4L2_CTRL_FLAG_SLIDER; + + imx378->temperature = v4l2_ctrl_new_std(&imx378->ctrls, + &imx378_ctrl_ops, V4L2_CID_TEMPERATURE, + -200000, 800000, 1, 250000); + if (imx378->temperature) + imx378->temperature->name = "Sensor Temperature"; + + ret = imx378->ctrls.error; + if (ret) { + dev_err(imx378->dev, "%s control init failed (%d)\n", + __func__, ret); + goto error; + } + + imx378->sd.ctrl_handler = &imx378->ctrls; + + return 0; + +error: + v4l2_ctrl_handler_free(&imx378->ctrls); + return ret; +} + +static int imx378_parse_fwnode(struct device *dev) +{ + struct fwnode_handle *endpoint; + struct v4l2_fwnode_endpoint bus_cfg = { + .bus_type = V4L2_MBUS_CSI2_DPHY, + }; + unsigned int i; + int ret; + + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); + if (!endpoint) { + dev_err(dev, "endpoint node not found\n"); + return -EINVAL; + } + + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg); + if (ret) { + dev_err(dev, "parsing endpoint node failed\n"); + goto done; + } + + for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) + if (bus_cfg.link_frequencies[i] == IMX378_DEFAULT_LINK_FREQ) + break; + + if (i == bus_cfg.nr_of_link_frequencies) { + dev_err(dev, "link-frequencies %d not supported, Please review your DT\n", + IMX378_DEFAULT_LINK_FREQ); + ret = -EINVAL; + goto done; + } + +done: + v4l2_fwnode_endpoint_free(&bus_cfg); + fwnode_handle_put(endpoint); + return ret; +} + +static int __maybe_unused imx378_suspend(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct imx378 *imx378 = to_imx378(sd); + + if (imx378->streaming) + imx378_stop_streaming(imx378); + + return 0; +} + +static int __maybe_unused imx378_resume(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct imx378 *imx378 = to_imx378(sd); + int ret; + + if (imx378->streaming) { + ret = imx378_start_streaming(imx378); + if (ret) + goto error; + } + + return 0; + +error: + imx378_stop_streaming(imx378); + imx378->streaming = 0; + return ret; +} + +static int imx378_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct imx378 *imx378; + int ret; + + ret = imx378_parse_fwnode(dev); + if (ret) + return ret; + + imx378 = devm_kzalloc(dev, sizeof(*imx378), GFP_KERNEL); + if (!imx378) + return -ENOMEM; + + imx378->dev = dev; + + imx378->xclk = devm_clk_get(dev, NULL); + if (IS_ERR(imx378->xclk)) { + dev_err(dev, "could not get xclk"); + return PTR_ERR(imx378->xclk); + } + + ret = clk_set_rate(imx378->xclk, IMX378_DEFAULT_CLK_FREQ); + if (ret) { + dev_err(dev, "could not set xclk frequency\n"); + return ret; + } + + ret = imx378_get_regulators(dev, imx378); + if (ret < 0) { + dev_err(dev, "could not get regulators\n"); + return ret; + } + + imx378->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); + if (IS_ERR(imx378->enable_gpio)) { + dev_err(dev, "could get enable gpio\n"); + return PTR_ERR(imx378->enable_gpio); + } + + imx378->regmap = devm_regmap_init_i2c(client, &sensor_regmap_config); + if (IS_ERR(imx378->regmap)) { + dev_err(dev, "regmap init failed\n"); + return PTR_ERR(imx378->regmap); + } + + v4l2_i2c_subdev_init(&imx378->sd, client, &imx378_subdev_ops); + + imx378_power_on(imx378->dev); + + pm_runtime_set_active(imx378->dev); + pm_runtime_enable(imx378->dev); + pm_runtime_idle(imx378->dev); + + ret = imx378_init_controls(imx378); + if (ret) { + dev_err(dev, "failed to init controls: %d", ret); + goto error_probe; + } + + imx378->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; + imx378->pad.flags = MEDIA_PAD_FL_SOURCE; + imx378->sd.dev = &client->dev; + imx378->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; + + ret = media_entity_pads_init(&imx378->sd.entity, 1, &imx378->pad); + if (ret < 0) { + dev_err(dev, "could not register media entity\n"); + goto free_ctrl; + } + + imx378_entity_init_cfg(&imx378->sd, NULL); + + ret = v4l2_async_register_subdev_sensor_common(&imx378->sd); + if (ret < 0) { + dev_err(dev, "could not register v4l2 device\n"); + goto free_entity; + } + + ret = v4l2_device_register_subdev(&imx378->v4l2_dev, &imx378->sd); + if (ret < 0) { + dev_err(dev, "could not register v4l2 sd\n"); + goto free_entity; + } + + ret = v4l2_device_register_subdev_nodes(&imx378->v4l2_dev); + if (ret) { + dev_err(dev, "imx378 subdev nodes registration failed (err=%d)\n", + ret); + return ret; + } + + return 0; + +free_entity: + media_entity_cleanup(&imx378->sd.entity); +free_ctrl: + v4l2_ctrl_handler_free(&imx378->ctrls); +error_probe: + mutex_destroy(&imx378->mutex); + pm_runtime_disable(imx378->dev); + + return ret; +} + +static int imx378_remove(struct i2c_client *client) +{ + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct imx378 *imx378 = to_imx378(sd); + + v4l2_async_unregister_subdev(&imx378->sd); + media_entity_cleanup(&imx378->sd.entity); + v4l2_ctrl_handler_free(&imx378->ctrls); + + pm_runtime_disable(&client->dev); + pm_runtime_set_suspended(&client->dev); + + mutex_destroy(&imx378->mutex); + + return 0; +} + +static const struct of_device_id imx378_of_match[] = { + { .compatible = "sony,imx378" }, + { } +}; +MODULE_DEVICE_TABLE(of, imx378_of_match); + +static const struct i2c_device_id imx378_id[] = { + {"imx378", 0}, + {} +}; +MODULE_DEVICE_TABLE(i2c, imx378_id); + +static const struct dev_pm_ops imx378_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(imx378_suspend, imx378_resume) + SET_RUNTIME_PM_OPS(imx378_power_off, imx378_power_on, NULL) +}; + +static struct i2c_driver imx378_i2c_driver = { + .driver = { + .of_match_table = imx378_of_match, + .pm = &imx378_pm_ops, + .name = "imx378", + }, + .probe_new = imx378_probe, + .remove = imx378_remove, + .id_table = imx378_id, +}; + +module_i2c_driver(imx378_i2c_driver); + +MODULE_DESCRIPTION("Sony IMX378 Camera driver"); +MODULE_AUTHOR("Daniel Gomez <daniel@qtec.com>"); +MODULE_LICENSE("GPL v2"); -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver 2020-04-14 20:01 ` [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver Daniel Gomez @ 2020-04-15 0:37 ` kbuild test robot 2020-04-28 21:02 ` Sakari Ailus 1 sibling, 0 replies; 16+ messages in thread From: kbuild test robot @ 2020-04-15 0:37 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 6122 bytes --] Hi Daniel, [FYI, it's a private test report for your RFC patch.] [auto build test ERROR on linuxtv-media/master] [also build test ERROR on v5.7-rc1 next-20200414] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Daniel-Gomez/v4l2-api-changes-for-imx378-driver/20200415-053427 base: git://linuxtv.org/media_tree.git master config: parisc-allyesconfig (attached as .config) compiler: hppa-linux-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=9.3.0 make.cross ARCH=parisc If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot <lkp@intel.com> All errors (new ones prefixed by >>): drivers/media/i2c/imx378.c: In function 'imx378_probe': >> drivers/media/i2c/imx378.c:1754:8: error: implicit declaration of function 'v4l2_device_register_subdev'; did you mean 'v4l2_async_register_subdev'? [-Werror=implicit-function-declaration] 1754 | ret = v4l2_device_register_subdev(&imx378->v4l2_dev, &imx378->sd); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ | v4l2_async_register_subdev >> drivers/media/i2c/imx378.c:1754:43: error: 'struct imx378' has no member named 'v4l2_dev' 1754 | ret = v4l2_device_register_subdev(&imx378->v4l2_dev, &imx378->sd); | ^~ >> drivers/media/i2c/imx378.c:1760:8: error: implicit declaration of function 'v4l2_device_register_subdev_nodes'; did you mean 'v4l2_async_register_subdev'? [-Werror=implicit-function-declaration] 1760 | ret = v4l2_device_register_subdev_nodes(&imx378->v4l2_dev); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | v4l2_async_register_subdev drivers/media/i2c/imx378.c:1760:49: error: 'struct imx378' has no member named 'v4l2_dev' 1760 | ret = v4l2_device_register_subdev_nodes(&imx378->v4l2_dev); | ^~ At top level: drivers/media/i2c/imx378.c:577:12: warning: 'imx378_read_reg16' defined but not used [-Wunused-function] 577 | static int imx378_read_reg16(struct imx378 *imx378, u16 addr, u16 *val) | ^~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +1754 drivers/media/i2c/imx378.c 1674 1675 static int imx378_probe(struct i2c_client *client) 1676 { 1677 struct device *dev = &client->dev; 1678 struct imx378 *imx378; 1679 int ret; 1680 1681 ret = imx378_parse_fwnode(dev); 1682 if (ret) 1683 return ret; 1684 1685 imx378 = devm_kzalloc(dev, sizeof(*imx378), GFP_KERNEL); 1686 if (!imx378) 1687 return -ENOMEM; 1688 1689 imx378->dev = dev; 1690 1691 imx378->xclk = devm_clk_get(dev, NULL); 1692 if (IS_ERR(imx378->xclk)) { 1693 dev_err(dev, "could not get xclk"); 1694 return PTR_ERR(imx378->xclk); 1695 } 1696 1697 ret = clk_set_rate(imx378->xclk, IMX378_DEFAULT_CLK_FREQ); 1698 if (ret) { 1699 dev_err(dev, "could not set xclk frequency\n"); 1700 return ret; 1701 } 1702 1703 ret = imx378_get_regulators(dev, imx378); 1704 if (ret < 0) { 1705 dev_err(dev, "could not get regulators\n"); 1706 return ret; 1707 } 1708 1709 imx378->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); 1710 if (IS_ERR(imx378->enable_gpio)) { 1711 dev_err(dev, "could get enable gpio\n"); 1712 return PTR_ERR(imx378->enable_gpio); 1713 } 1714 1715 imx378->regmap = devm_regmap_init_i2c(client, &sensor_regmap_config); 1716 if (IS_ERR(imx378->regmap)) { 1717 dev_err(dev, "regmap init failed\n"); 1718 return PTR_ERR(imx378->regmap); 1719 } 1720 1721 v4l2_i2c_subdev_init(&imx378->sd, client, &imx378_subdev_ops); 1722 1723 imx378_power_on(imx378->dev); 1724 1725 pm_runtime_set_active(imx378->dev); 1726 pm_runtime_enable(imx378->dev); 1727 pm_runtime_idle(imx378->dev); 1728 1729 ret = imx378_init_controls(imx378); 1730 if (ret) { 1731 dev_err(dev, "failed to init controls: %d", ret); 1732 goto error_probe; 1733 } 1734 1735 imx378->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; 1736 imx378->pad.flags = MEDIA_PAD_FL_SOURCE; 1737 imx378->sd.dev = &client->dev; 1738 imx378->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; 1739 1740 ret = media_entity_pads_init(&imx378->sd.entity, 1, &imx378->pad); 1741 if (ret < 0) { 1742 dev_err(dev, "could not register media entity\n"); 1743 goto free_ctrl; 1744 } 1745 1746 imx378_entity_init_cfg(&imx378->sd, NULL); 1747 1748 ret = v4l2_async_register_subdev_sensor_common(&imx378->sd); 1749 if (ret < 0) { 1750 dev_err(dev, "could not register v4l2 device\n"); 1751 goto free_entity; 1752 } 1753 > 1754 ret = v4l2_device_register_subdev(&imx378->v4l2_dev, &imx378->sd); 1755 if (ret < 0) { 1756 dev_err(dev, "could not register v4l2 sd\n"); 1757 goto free_entity; 1758 } 1759 > 1760 ret = v4l2_device_register_subdev_nodes(&imx378->v4l2_dev); 1761 if (ret) { 1762 dev_err(dev, "imx378 subdev nodes registration failed (err=%d)\n", 1763 ret); 1764 return ret; 1765 } 1766 1767 return 0; 1768 1769 free_entity: 1770 media_entity_cleanup(&imx378->sd.entity); 1771 free_ctrl: 1772 v4l2_ctrl_handler_free(&imx378->ctrls); 1773 error_probe: 1774 mutex_destroy(&imx378->mutex); 1775 pm_runtime_disable(imx378->dev); 1776 1777 return ret; 1778 } 1779 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 61726 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver 2020-04-14 20:01 ` [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver Daniel Gomez 2020-04-15 0:37 ` kbuild test robot @ 2020-04-28 21:02 ` Sakari Ailus 1 sibling, 0 replies; 16+ messages in thread From: Sakari Ailus @ 2020-04-28 21:02 UTC (permalink / raw) To: Daniel Gomez; +Cc: mchehab, hverkuil-cisco, linux-media, linux-kernel Hi Daniel, On Tue, Apr 14, 2020 at 10:01:51PM +0200, Daniel Gomez wrote: > Add a V4L2 sub-device driver for the Sony IMX378 image sensor. > This is a camera sensor using the I2C bus for control and the > CSI-2 bus for data. > > Signed-off-by: Daniel Gomez <daniel@qtec.com> > --- > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/imx378.c | 1829 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 1841 insertions(+) > create mode 100644 drivers/media/i2c/imx378.c > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index efd12bf4f8eb..8ea630275faf 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -796,6 +796,17 @@ config VIDEO_IMX355 > To compile this driver as a module, choose M here: the > module will be called imx355. > > +config VIDEO_IMX378 > + tristate "Sony IMX378 sensor support" > + depends on GPIOLIB && I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API > + depends on V4L2_FWNODE You'll need to use select on some of these things nowadays. Please see the current Kconfig in the media tree master. > + help > + This is a Video4Linux2 sensor driver for the Sony > + IMX378 camera. > + > + To compile this driver as a module, choose M here: the > + module will be called imx378. > + > config VIDEO_OV2640 > tristate "OmniVision OV2640 sensor support" > depends on VIDEO_V4L2 && I2C > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index 77bf7d0b691f..dff1c9ec444f 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -117,6 +117,7 @@ obj-$(CONFIG_VIDEO_IMX274) += imx274.o > obj-$(CONFIG_VIDEO_IMX290) += imx290.o > obj-$(CONFIG_VIDEO_IMX319) += imx319.o > obj-$(CONFIG_VIDEO_IMX355) += imx355.o > +obj-$(CONFIG_VIDEO_IMX378) += imx378.o > obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o > > obj-$(CONFIG_SDR_MAX2175) += max2175.o > diff --git a/drivers/media/i2c/imx378.c b/drivers/media/i2c/imx378.c > new file mode 100644 > index 000000000000..6e22c3b45f72 > --- /dev/null > +++ b/drivers/media/i2c/imx378.c > @@ -0,0 +1,1829 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * imx378.c - imx378 sensor driver > + * > + * Copyright 2019 Qtechnology A/S > + * > + * Daniel Gomez <daniel@qtec.com> > + * > + * Based on imx214.c and imx355.c > + */ > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/pm_runtime.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <media/media-entity.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-fwnode.h> > +#include <media/v4l2-subdev.h> > + > +#define IMX378_DEFAULT_CLK_FREQ 24000000 > +#define IMX378_DEFAULT_LINK_FREQ 480000000 > +#define IMX378_DEFAULT_LINK_MFREQ (IMX378_DEFAULT_LINK_FREQ / 1000000) > +#define IMX378_DEFAULT_PIXEL_RATE ((IMX378_DEFAULT_LINK_FREQ * 8LL) / 10) > +#define IMX378_DEFAULT_MBUS_CODE MEDIA_BUS_FMT_SRGGB10_1X10 > +#define IMX378_DEFAULT_FIVAL {1, 30} > +#define IMX378_LINE_LENGTH 4444 > +#define IMX378_MIN_WIDTH 4 > +#define IMX378_MIN_HEIGHT 4 > +#define IMX378_MAX_DEFAULT_WIDTH 4032 > +#define IMX378_MAX_DEFAULT_HEIGHT 3040 > +#define IMX378_MAX_BOUNDS_WIDTH 4032 > +#define IMX378_MAX_BOUNDS_HEIGHT 3104 > +#define IMX378_CID_CUSTOM_BASE (V4L2_CID_USER_BASE | 0xf000) > +#define IMX378_CID_GREEN_BALANCE (IMX378_CID_CUSTOM_BASE + 0) > + > +static const char * const imx378_supply_name[] = { > + "vdda", > + "vddd", > + "vdddo", > +}; > + > +#define IMX378_NUM_SUPPLIES ARRAY_SIZE(imx378_supply_name) > + > +struct imx378 { > + struct device *dev; > + struct clk *xclk; > + struct regmap *regmap; > + > + struct v4l2_subdev sd; > + struct media_pad pad; > + struct v4l2_mbus_framefmt fmt; > + struct v4l2_rect crop; > + > + struct v4l2_ctrl_handler ctrls; > + struct v4l2_ctrl *pixel_rate; > + struct v4l2_ctrl *link_freq; > + struct v4l2_ctrl *exposure; > + struct v4l2_ctrl *vflip; > + struct v4l2_ctrl *hflip; > + struct v4l2_ctrl *analogue_gain; > + struct v4l2_ctrl *digital_gain; > + struct v4l2_ctrl *red_balance; > + struct v4l2_ctrl *blue_balance; > + struct v4l2_ctrl *green_balance; > + struct v4l2_ctrl *temperature; > + > + struct regulator_bulk_data supplies[IMX378_NUM_SUPPLIES]; > + > + struct gpio_desc *enable_gpio; > + > + struct mutex mutex; > + > + struct v4l2_fract fival; > + > + bool streaming; > +}; > + > +struct reg_8 { > + u16 addr; > + u8 val; > +}; > + > +enum { > + IMX378_TABLE_WAIT_MS = 0, > + IMX378_TABLE_END, > + IMX378_MAX_RETRIES, > + IMX378_WAIT_MS > +}; > + > +static const struct reg_8 mode_table_common[] = { > + /* software standby settings */ > + {0x0100, 0x00}, > + > + /* software reset */ > + {0x0103, 0x01}, > + > + /* CSI */ > + {0x0112, 0x0A}, > + {0x0113, 0x0A}, > + {0x0114, 0x03}, > + > + /* global settings */ > + {0x0136, 0x18}, > + {0x0137, 0x00}, > + > + {0x0202, 0x20}, > + {0x0203, 0xa0}, > + {0x0204, 0x03}, > + {0x0205, 0xd2}, > + {0x020e, 0x01}, > + {0x020f, 0x00}, > + {0x0210, 0x02}, > + {0x0211, 0x21}, > + {0x0212, 0x01}, > + {0x0213, 0xee}, > + {0x0214, 0x01}, > + {0x0215, 0x00}, > + {0x0220, 0x00}, > + {0x0221, 0x11}, > + > + {0x0301, 0x05}, > + {0x0303, 0x02}, > + {0x0305, 0x03}, > + {0x0306, 0x00}, > + {0x0307, 0x96}, > + > + {0x0309, 0x0a}, > + {0x030b, 0x01}, > + {0x030d, 0x02}, > + {0x030e, 0x00}, > + {0x030f, 0xf6}, > + {0x0310, 0x00}, > + {0x0340, 0x04}, > + {0x0341, 0xb0}, > + {0x0342, 0x20}, > + {0x0343, 0x08}, > + {0x0344, 0x00}, > + {0x0345, 0x00}, > + {0x0346, 0x01}, > + {0x0347, 0x78}, > + {0x0348, 0x0f}, > + {0x0349, 0xd7}, > + {0x0350, 0x01}, > + {0x034a, 0x0a}, > + {0x034b, 0x67}, > + {0x034c, 0x07}, > + {0x034d, 0x80}, > + {0x034e, 0x04}, > + {0x034f, 0x38}, > + {0x0381, 0x01}, > + {0x0383, 0x01}, > + {0x0385, 0x01}, > + {0x0387, 0x01}, > + > + {0x0401, 0x00}, > + {0x0404, 0x00}, > + {0x0405, 0x10}, > + {0x0408, 0x00}, > + {0x0409, 0x6c}, > + {0x040a, 0x00}, > + {0x040b, 0x40}, > + {0x040c, 0x07}, > + {0x040d, 0x80}, > + {0x040e, 0x04}, > + {0x040f, 0x38}, > + > + {0x0900, 0x00}, > + {0x0901, 0x11}, > + {0x0902, 0x02}, > + > + {0x0c1a, 0x01}, > + {0x0c1b, 0x01}, > + > + {0x3030, 0x01}, > + {0x3032, 0x00}, > + {0x3033, 0x40}, > + {0x3140, 0x02}, > + {0x38a8, 0x1f}, > + {0x38a9, 0xff}, > + {0x38aa, 0x1f}, > + {0x38ab, 0xff}, > + {0x3c00, 0x00}, > + {0x3c01, 0x03}, > + {0x3c02, 0xdc}, > + {0x3d8a, 0x01}, > + {0x3e20, 0x01}, > + {0x3e37, 0x01}, > + {0x3f0d, 0x00}, > + {0x3f50, 0x00}, > + {0x3f56, 0x00}, > + {0x3f57, 0x01}, > + {0x3ff9, 0x00}, > + > + {0x4421, 0x08}, > + {0x4ae9, 0x18}, > + {0x4ae9, 0x21}, > + {0x4aea, 0x08}, > + {0x4aea, 0x80}, > + > + {0x55d4, 0x00}, > + {0x55d5, 0x00}, > + {0x55d6, 0x07}, > + {0x55d7, 0xff}, > + {0x55e8, 0x07}, > + {0x55e9, 0xff}, > + {0x55ea, 0x00}, > + {0x55eb, 0x00}, > + {0x5748, 0x07}, > + {0x5749, 0xff}, > + {0x574a, 0x00}, > + {0x574b, 0x00}, > + {0x574c, 0x07}, > + {0x574d, 0xff}, > + {0x574e, 0x00}, > + {0x574f, 0x00}, > + {0x5754, 0x00}, > + {0x5755, 0x00}, > + {0x5756, 0x07}, > + {0x5757, 0xff}, > + {0x5973, 0x04}, > + {0x5974, 0x01}, > + {0x5d13, 0xc3}, > + {0x5d14, 0x58}, > + {0x5d15, 0xa3}, > + {0x5d16, 0x1d}, > + {0x5d17, 0x65}, > + {0x5d18, 0x8c}, > + {0x5d1a, 0x06}, > + {0x5d1b, 0xa9}, > + {0x5d1c, 0x45}, > + {0x5d1d, 0x3a}, > + {0x5d1e, 0xab}, > + {0x5d1f, 0x15}, > + {0x5d21, 0x0e}, > + {0x5d22, 0x52}, > + {0x5d23, 0xaa}, > + {0x5d24, 0x7d}, > + {0x5d25, 0x57}, > + {0x5d26, 0xa8}, > + {0x5d37, 0x5a}, > + {0x5d38, 0x5a}, > + {0x5d77, 0x7f}, > + > + {0x7b3b, 0x01}, > + {0x7b4c, 0x00}, > + {0x7b53, 0x01}, > + {0x7b75, 0x0e}, > + {0x7b76, 0x0b}, > + {0x7b77, 0x08}, > + {0x7b78, 0x0a}, > + {0x7b79, 0x47}, > + {0x7b7c, 0x00}, > + {0x7b7d, 0x00}, > + > + {0x8d1f, 0x00}, > + {0x8d27, 0x00}, > + > + {0x9004, 0x03}, > + {0x9200, 0x50}, > + {0x9201, 0x6c}, > + {0x9202, 0x71}, > + {0x9203, 0x00}, > + {0x9204, 0x71}, > + {0x9205, 0x01}, > + {0x9304, 0x03}, > + {0x9305, 0x00}, > + {0x9369, 0x5a}, > + {0x936b, 0x55}, > + {0x936d, 0x28}, > + {0x9371, 0x6a}, > + {0x9373, 0x6a}, > + {0x9375, 0x64}, > + {0x9905, 0x00}, > + {0x9907, 0x00}, > + {0x9909, 0x00}, > + {0x990b, 0x00}, > + {0x991a, 0x00}, > + {0x9944, 0x3c}, > + {0x9947, 0x3c}, > + {0x994a, 0x8c}, > + {0x994b, 0x50}, > + {0x994c, 0x1b}, > + {0x994d, 0x8c}, > + {0x994e, 0x50}, > + {0x994f, 0x1b}, > + {0x9950, 0x8c}, > + {0x9951, 0x1b}, > + {0x9952, 0x0a}, > + {0x9953, 0x8c}, > + {0x9954, 0x1b}, > + {0x9955, 0x0a}, > + {0x996b, 0x8c}, > + {0x996c, 0x64}, > + {0x996d, 0x50}, > + {0x9a13, 0x04}, > + {0x9a14, 0x04}, > + {0x9a19, 0x00}, > + {0x9a1c, 0x04}, > + {0x9a1d, 0x04}, > + {0x9a26, 0x05}, > + {0x9a27, 0x05}, > + {0x9a2c, 0x01}, > + {0x9a2d, 0x03}, > + {0x9a2f, 0x05}, > + {0x9a30, 0x05}, > + {0x9a41, 0x00}, > + {0x9a46, 0x00}, > + {0x9a47, 0x00}, > + {0x9a4c, 0x0d}, > + {0x9a4d, 0x0d}, > + {0x9c17, 0x35}, > + {0x9c1d, 0x31}, > + {0x9c29, 0x50}, > + {0x9c3b, 0x2f}, > + {0x9c41, 0x6b}, > + {0x9c47, 0x2d}, > + {0x9c4d, 0x40}, > + {0x9c6b, 0x00}, > + {0x9c71, 0xc8}, > + {0x9c73, 0x32}, > + {0x9c75, 0x04}, > + {0x9c7d, 0x2d}, > + {0x9c83, 0x40}, > + {0x9c94, 0x3f}, > + {0x9c95, 0x3f}, > + {0x9c96, 0x3f}, > + {0x9c97, 0x00}, > + {0x9c98, 0x00}, > + {0x9c99, 0x00}, > + {0x9c9a, 0x3f}, > + {0x9c9b, 0x3f}, > + {0x9c9c, 0x3f}, > + {0x9ca0, 0x0f}, > + {0x9ca1, 0x0f}, > + {0x9ca2, 0x0f}, > + {0x9ca3, 0x00}, > + {0x9ca4, 0x00}, > + {0x9ca5, 0x00}, > + {0x9ca6, 0x1e}, > + {0x9ca7, 0x1e}, > + {0x9ca8, 0x1e}, > + {0x9ca9, 0x00}, > + {0x9caa, 0x00}, > + {0x9cab, 0x00}, > + {0x9cac, 0x09}, > + {0x9cad, 0x09}, > + {0x9cae, 0x09}, > + {0x9cbd, 0x50}, > + {0x9cbf, 0x50}, > + {0x9cc1, 0x50}, > + {0x9cc3, 0x40}, > + {0x9cc5, 0x40}, > + {0x9cc7, 0x40}, > + {0x9cc9, 0x0a}, > + {0x9ccb, 0x0a}, > + {0x9ccd, 0x0a}, > + {0x9d17, 0x35}, > + {0x9d1d, 0x31}, > + {0x9d29, 0x50}, > + {0x9d3b, 0x2f}, > + {0x9d41, 0x6b}, > + {0x9d47, 0x42}, > + {0x9d4d, 0x5a}, > + {0x9d6b, 0x00}, > + {0x9d71, 0xc8}, > + {0x9d73, 0x32}, > + {0x9d75, 0x04}, > + {0x9d7d, 0x42}, > + {0x9d83, 0x5a}, > + {0x9d94, 0x3f}, > + {0x9d95, 0x3f}, > + {0x9d96, 0x3f}, > + {0x9d97, 0x00}, > + {0x9d98, 0x00}, > + {0x9d99, 0x00}, > + {0x9d9a, 0x3f}, > + {0x9d9b, 0x3f}, > + {0x9d9c, 0x3f}, > + {0x9d9d, 0x1f}, > + {0x9d9e, 0x1f}, > + {0x9d9f, 0x1f}, > + {0x9da0, 0x0f}, > + {0x9da1, 0x0f}, > + {0x9da2, 0x0f}, > + {0x9da3, 0x00}, > + {0x9da4, 0x00}, > + {0x9da5, 0x00}, > + {0x9da6, 0x1e}, > + {0x9da7, 0x1e}, > + {0x9da8, 0x1e}, > + {0x9da9, 0x00}, > + {0x9daa, 0x00}, > + {0x9dab, 0x00}, > + {0x9dac, 0x09}, > + {0x9dad, 0x09}, > + {0x9dae, 0x09}, > + {0x9dc9, 0x0a}, > + {0x9dcb, 0x0a}, > + {0x9dcd, 0x0a}, > + {0x9e17, 0x35}, > + {0x9e1d, 0x31}, > + {0x9e29, 0x50}, > + {0x9e3b, 0x2f}, > + {0x9e41, 0x6b}, > + {0x9e47, 0x2d}, > + {0x9e4d, 0x40}, > + {0x9e6b, 0x00}, > + {0x9e71, 0xc8}, > + {0x9e73, 0x32}, > + {0x9e75, 0x04}, > + {0x9e94, 0x0f}, > + {0x9e95, 0x0f}, > + {0x9e96, 0x0f}, > + {0x9e97, 0x00}, > + {0x9e98, 0x00}, > + {0x9e99, 0x00}, > + {0x9e9a, 0x2f}, > + {0x9e9b, 0x2f}, > + {0x9e9c, 0x2f}, > + {0x9e9d, 0x00}, > + {0x9e9e, 0x00}, > + {0x9e9f, 0x00}, > + {0x9ea0, 0x0f}, > + {0x9ea1, 0x0f}, > + {0x9ea2, 0x0f}, > + {0x9ea3, 0x00}, > + {0x9ea4, 0x00}, > + {0x9ea5, 0x00}, > + {0x9ea6, 0x3f}, > + {0x9ea7, 0x3f}, > + {0x9ea8, 0x3f}, > + {0x9ea9, 0x00}, > + {0x9eaa, 0x00}, > + {0x9eab, 0x00}, > + {0x9eac, 0x09}, > + {0x9ead, 0x09}, > + {0x9eae, 0x09}, > + {0x9ec9, 0x0a}, > + {0x9ecb, 0x0a}, > + {0x9ecd, 0x0a}, > + {0x9f17, 0x35}, > + {0x9f1d, 0x31}, > + {0x9f29, 0x50}, > + {0x9f3b, 0x2f}, > + {0x9f41, 0x6b}, > + {0x9f47, 0x42}, > + {0x9f4d, 0x5a}, > + {0x9f6b, 0x00}, > + {0x9f71, 0xc8}, > + {0x9f73, 0x32}, > + {0x9f75, 0x04}, > + {0x9f94, 0x0f}, > + {0x9f95, 0x0f}, > + {0x9f96, 0x0f}, > + {0x9f97, 0x00}, > + {0x9f98, 0x00}, > + {0x9f99, 0x00}, > + {0x9f9a, 0x2f}, > + {0x9f9b, 0x2f}, > + {0x9f9c, 0x2f}, > + {0x9f9d, 0x00}, > + {0x9f9e, 0x00}, > + {0x9f9f, 0x00}, > + {0x9fa0, 0x0f}, > + {0x9fa1, 0x0f}, > + {0x9fa2, 0x0f}, > + {0x9fa3, 0x00}, > + {0x9fa4, 0x00}, > + {0x9fa5, 0x00}, > + {0x9fa6, 0x1e}, > + {0x9fa7, 0x1e}, > + {0x9fa8, 0x1e}, > + {0x9fa9, 0x00}, > + {0x9faa, 0x00}, > + {0x9fab, 0x00}, > + {0x9fac, 0x09}, > + {0x9fad, 0x09}, > + {0x9fae, 0x09}, > + {0x9fc9, 0x0a}, > + {0x9fcb, 0x0a}, > + {0x9fcd, 0x0a}, > + > + {0xa001, 0x0a}, > + {0xa003, 0x0a}, > + {0xa005, 0x0a}, > + {0xa006, 0x01}, > + {0xa007, 0xc0}, > + {0xa009, 0xc0}, > + {0xa14b, 0xff}, > + {0xa151, 0x0c}, > + {0xa153, 0x50}, > + {0xa155, 0x02}, > + {0xa157, 0x00}, > + {0xa1ad, 0xff}, > + {0xa1b3, 0x0c}, > + {0xa1b5, 0x50}, > + {0xa1b9, 0x00}, > + {0xa24b, 0xff}, > + {0xa257, 0x00}, > + {0xa2a9, 0x60}, > + {0xa2ad, 0xff}, > + {0xa2b7, 0x00}, > + {0xa2b9, 0x00}, > + > + {0xb21f, 0x04}, > + {0xb35c, 0x00}, > + {0xb35e, 0x08}, > + {0xbcf1, 0x02}, > + {0xe000, 0x00}, > + {0xf61c, 0x04}, > + {0xf61e, 0x04}, > + > + {IMX378_TABLE_END, 0x00} > +}; > + > +static inline struct imx378 *to_imx378(struct v4l2_subdev *sd) > +{ > + return container_of(sd, struct imx378, sd); > +} > + > +static int imx378_set_flip_mode(struct imx378 *imx378, u32 code) > +{ > + /* WORKAROUND to update controls */ > + imx378->vflip->flags &= ~V4L2_CTRL_FLAG_READ_ONLY; > + imx378->hflip->flags &= ~V4L2_CTRL_FLAG_READ_ONLY; > + > + switch (code) { > + case MEDIA_BUS_FMT_SBGGR10_1X10: > + v4l2_ctrl_s_ctrl(imx378->vflip, 1); > + v4l2_ctrl_s_ctrl(imx378->hflip, 1); Other raw sensor drivers which support flip controls change the format based on those controls, not the other way around. Could you do the same in this one, please? > + break; > + case MEDIA_BUS_FMT_SGBRG10_1X10: > + v4l2_ctrl_s_ctrl(imx378->vflip, 1); > + v4l2_ctrl_s_ctrl(imx378->hflip, 0); > + break; > + case MEDIA_BUS_FMT_SGRBG10_1X10: > + v4l2_ctrl_s_ctrl(imx378->vflip, 0); > + v4l2_ctrl_s_ctrl(imx378->hflip, 1); > + break; > + case MEDIA_BUS_FMT_SRGGB10_1X10: > + v4l2_ctrl_s_ctrl(imx378->vflip, 0); > + v4l2_ctrl_s_ctrl(imx378->hflip, 0); > + break; > + default: > + break; > + } > + > + /* WORKAROUND to update controls */ > + imx378->vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + imx378->hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + return 0; > +} > + > +static u32 imx378_get_format_code(struct imx378 *imx378) > +{ > + /* > + * Only one bayer order is supported. > + * It depends on the flip settings. > + */ > + u32 code; > + static const u32 codes[2][2] = { > + { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, }, > + { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, }, > + }; > + > + lockdep_assert_held(&imx378->mutex); > + code = codes[imx378->vflip->val][imx378->hflip->val]; > + > + return code; > +} > + > +static int imx378_read_reg16(struct imx378 *imx378, u16 addr, u16 *val) > +{ > + int ret; > + u32 h_val = 0, l_val = 0; > + > + ret = regmap_read(imx378->regmap, addr + 1, &l_val); Is there a reason to read just 8 bits at a time? Same for writing. > + if (ret) { > + pr_err("%s:i2c read failed, %x\n", __func__, addr + 1); > + return ret; > + } > + > + *val = l_val & 0xFF; > + > + ret = regmap_read(imx378->regmap, addr, &h_val); > + if (ret) { > + pr_err("%s:i2c read failed, %x\n", __func__, addr); > + return ret; > + } > + > + *val |= (h_val & 0xFF) << 8; > + > + return ret; > +} > + > +static int imx378_write_reg16(struct imx378 *imx378, u16 addr, u16 val) > +{ > + int ret; > + > + ret = regmap_write(imx378->regmap, addr + 1, val); > + if (ret) { > + pr_err("%s:i2c write failed, %x = %x\n", > + __func__, addr + 1, val); > + return ret; > + } > + > + ret = regmap_write(imx378->regmap, addr, val >> 8); > + if (ret) { > + pr_err("%s:i2c write failed, %x = %x\n", > + __func__, addr, val >> 8); > + return ret; > + } > + > + return ret; > +} > + > +static int __maybe_unused imx378_power_on(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx378 *imx378 = to_imx378(sd); > + int ret; > + > + gpiod_set_value_cansleep(imx378->enable_gpio, 0); > + > + ret = regulator_bulk_enable(IMX378_NUM_SUPPLIES, imx378->supplies); > + if (ret < 0) { > + dev_err(imx378->dev, "failed to enable regulators: %d\n", ret); > + return ret; > + } > + > + usleep_range(2000, 3000); > + > + ret = clk_prepare_enable(imx378->xclk); > + if (ret < 0) { > + regulator_bulk_disable(IMX378_NUM_SUPPLIES, imx378->supplies); > + dev_err(imx378->dev, "clk prepare enable failed\n"); > + return ret; > + } > + > + gpiod_set_value_cansleep(imx378->enable_gpio, 1); > + usleep_range(12000, 15000); > + > + return 0; > +} > + > +static int __maybe_unused imx378_power_off(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx378 *imx378 = to_imx378(sd); > + > + gpiod_set_value_cansleep(imx378->enable_gpio, 0); > + > + clk_disable_unprepare(imx378->xclk); > + > + regulator_bulk_disable(IMX378_NUM_SUPPLIES, imx378->supplies); > + usleep_range(10, 20); > + > + return 0; > +} > + > +static int imx378_enum_mbus_code(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + struct imx378 *imx378 = container_of(sd, struct imx378, sd); > + > + if (code->index != 0 || code->pad != 0) > + return -EINVAL; > + > + code->code = imx378_get_format_code(imx378); > + > + return 0; > +} > + > +static int imx378_enum_frame_size(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + struct imx378 *imx378 = container_of(sd, struct imx378, sd); > + > + if (fse->index != 0) > + return -EINVAL; > + > + mutex_lock(&imx378->mutex); > + if (fse->code != imx378_get_format_code(imx378)) { > + mutex_unlock(&imx378->mutex); > + return -EINVAL; > + } > + mutex_unlock(&imx378->mutex); > + > + fse->min_width = IMX378_MIN_WIDTH; > + fse->max_width = IMX378_MAX_BOUNDS_WIDTH; > + fse->min_height = IMX378_MIN_HEIGHT; > + fse->max_height = IMX378_MAX_BOUNDS_HEIGHT; > + > + return 0; > +} > + > +#ifdef CONFIG_VIDEO_ADV_DEBUG > +static int imx378_s_register(struct v4l2_subdev *sd, > + const struct v4l2_dbg_register *reg) > +{ > + struct imx378 *imx378 = container_of(sd, struct imx378, sd); > + > + return regmap_write(imx378->regmap, reg->reg, reg->val); > +} > + > +static int imx378_g_register(struct v4l2_subdev *sd, > + struct v4l2_dbg_register *reg) > +{ > + struct imx378 *imx378 = container_of(sd, struct imx378, sd); > + unsigned int aux; > + int ret; > + > + reg->size = 1; > + ret = regmap_read(imx378->regmap, reg->reg, &aux); > + reg->val = aux; > + > + return ret; > +} > +#endif > + > +static const struct v4l2_subdev_core_ops imx378_core_ops = { > +#ifdef CONFIG_VIDEO_ADV_DEBUG > + .g_register = imx378_g_register, > + .s_register = imx378_s_register, > +#endif > +}; > + > +static int imx378_set_img_size(struct imx378 *imx378) > +{ > + u32 frame_length; > + int ret; > + > + ret = imx378_write_reg16(imx378, 0x34c, imx378->fmt.width); > + if (ret < 0) > + goto error; > + > + ret = imx378_write_reg16(imx378, 0x34e, imx378->fmt.height); > + if (ret < 0) > + goto error; > + > + ret = imx378_write_reg16(imx378, 0x40c, imx378->fmt.width); > + if (ret < 0) > + goto error; > + > + ret = imx378_write_reg16(imx378, 0x40e, imx378->fmt.height); > + if (ret < 0) > + goto error; > + > + ret = imx378_write_reg16(imx378, 0x342, IMX378_LINE_LENGTH); > + if (ret < 0) > + goto error; > + > + frame_length = IMX378_DEFAULT_LINK_FREQ / > + ((imx378->fival.denominator / imx378->fival.numerator) > + * IMX378_LINE_LENGTH); Do you ensure somewhere you won't be dividing by zero here? > + > + ret = imx378_write_reg16(imx378, 0x340, frame_length); > + if (ret < 0) > + goto error; > + > + ret = imx378_write_reg16(imx378, 0x344, imx378->crop.left); > + if (ret < 0) > + goto error; > + > + ret = imx378_write_reg16(imx378, 0x348, imx378->crop.left + > + imx378->fmt.width - 1); > + if (ret < 0) > + goto error; > + > + ret = imx378_write_reg16(imx378, 0x346, imx378->crop.top); > + if (ret < 0) > + goto error; > + > + ret = imx378_write_reg16(imx378, 0x34a, imx378->crop.top + > + imx378->fmt.height - 1); > + if (ret < 0) > + goto error; > + > + ret = imx378_write_reg16(imx378, 0x408, 0); > + if (ret < 0) > + goto error; > + > + ret = imx378_write_reg16(imx378, 0x40a, 0); > + if (ret < 0) > + goto error; > + > + ret = regmap_write(imx378->regmap, 0xb04, 1); > + if (ret < 0) > + goto error; > + > + return ret; > + > +error: > + dev_err(imx378->dev, "could not set image size %d\n", ret); > + return ret; > +} > + > +static struct v4l2_mbus_framefmt * > +__imx378_get_pad_format(struct imx378 *imx378, > + struct v4l2_subdev_pad_config *cfg, > + unsigned int pad, > + enum v4l2_subdev_format_whence which) > +{ > + switch (which) { > + case V4L2_SUBDEV_FORMAT_TRY: > + return v4l2_subdev_get_try_format(&imx378->sd, cfg, pad); > + case V4L2_SUBDEV_FORMAT_ACTIVE: > + return &imx378->fmt; > + default: > + return NULL; > + } > +} > + > +static int imx378_get_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct imx378 *imx378 = to_imx378(sd); > + > + mutex_lock(&imx378->mutex); > + fmt->format = *__imx378_get_pad_format(imx378, cfg, fmt->pad, > + fmt->which); > + mutex_unlock(&imx378->mutex); > + > + return 0; > +} > + > +static struct v4l2_rect * > +__imx378_get_pad_crop(struct imx378 *imx378, struct v4l2_subdev_pad_config *cfg, > + unsigned int pad, enum v4l2_subdev_format_whence which) > +{ > + switch (which) { > + case V4L2_SUBDEV_FORMAT_TRY: > + return v4l2_subdev_get_try_crop(&imx378->sd, cfg, pad); > + case V4L2_SUBDEV_FORMAT_ACTIVE: > + return &imx378->crop; > + default: > + return NULL; > + } > +} > + > +static int imx378_min_interval(struct imx378 *imx378, > + struct v4l2_fract *fival) > +{ > + fival->numerator = IMX378_LINE_LENGTH * (imx378->fmt.height); > + fival->denominator = IMX378_DEFAULT_LINK_FREQ; > + > + return 0; > +} > + > +static int imx378_max_interval(struct v4l2_fract *fival) > +{ > + fival->numerator = IMX378_LINE_LENGTH * 0xffff; > + fival->denominator = IMX378_DEFAULT_LINK_FREQ; > + > + return 0; > +} > + > +#define FRACT_CMP(a, OP, b) \ > + ((u64)(a).numerator * (b).denominator OP \ > + (u64)(b).numerator * (a).denominator) > +static int imx378_clamp_interval(struct imx378 *imx378, > + struct v4l2_fract *fival) > +{ > + struct v4l2_fract interval; > + int ret; > + > + ret = imx378_min_interval(imx378, &interval); > + if (ret) > + return ret; > + if (FRACT_CMP(*fival, <, interval)) > + *fival = interval; > + > + ret = imx378_max_interval(&interval); > + if (ret) > + return ret; > + if (FRACT_CMP(*fival, >, interval)) > + *fival = interval; > + > + return 0; > +} > + > +static int imx378_g_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_frame_interval *fival) > +{ > + struct imx378 *imx378 = to_imx378(sd); > + > + fival->interval = imx378->fival; > + > + return 0; > +} > + > +static int imx378_s_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_frame_interval *fival) > +{ > + struct imx378 *imx378 = to_imx378(sd); > + u64 max_exposure; > + > + if (fival->interval.denominator == 0) > + fival->interval = (struct v4l2_fract) IMX378_DEFAULT_FIVAL; > + > + imx378_clamp_interval(imx378, &fival->interval); > + > + imx378->fival = fival->interval; > + > + /* Update exposure limits */ > + max_exposure = (IMX378_DEFAULT_LINK_FREQ / > + ((imx378->fival.denominator / imx378->fival.numerator) > + * IMX378_LINE_LENGTH)) - 20; Ditto. > + > + __v4l2_ctrl_modify_range(imx378->exposure, imx378->exposure->minimum, > + max_exposure, imx378->exposure->step, > + max_exposure); I believe you should be acquiring the control handler lock here, i.e. use the locked variant. This function is called by the V4L2 subdev framework, including through the IOCTL handler. > + > + return 0; > +} > + > +static int > +imx378_enum_frame_interval(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_frame_interval_enum *fie) > +{ > + struct imx378 *imx378 = to_imx378(sd); > + int ret; > + > + if (fie->index != 0) > + return -EINVAL; > + > + if (fie->width > IMX378_MAX_BOUNDS_WIDTH || > + fie->width < IMX378_MIN_WIDTH) > + return -EINVAL; > + > + if (fie->height > IMX378_MAX_BOUNDS_HEIGHT || > + fie->height < IMX378_MIN_HEIGHT) > + return -EINVAL; The frame interval should be specific to a given size, i.e. you can return an error if this is being enumerated for something else than an enumerated size. > + > + mutex_lock(&imx378->mutex); > + if (fie->code != imx378_get_format_code(imx378)) { > + mutex_unlock(&imx378->mutex); > + return -EINVAL; > + } > + mutex_unlock(&imx378->mutex); > + > + ret = imx378_min_interval(imx378, &fie->min_interval); > + if (ret) > + return ret; > + > + ret = imx378_max_interval(&fie->max_interval); > + if (ret) > + return ret; > + > + fie->interval = (struct v4l2_fract) IMX378_DEFAULT_FIVAL; > + > + return 0; > +} > + > +static int imx378_set_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct imx378 *imx378 = to_imx378(sd); > + struct v4l2_mbus_framefmt *__fmt; > + struct v4l2_rect *__crop; > + struct v4l2_subdev_frame_interval fival = { }; > + > + mutex_lock(&imx378->mutex); > + > + if (imx378->streaming && fmt->which != V4L2_SUBDEV_FORMAT_TRY) > + return -EBUSY; Oops! > + > + __crop = __imx378_get_pad_crop(imx378, cfg, fmt->pad, fmt->which); > + > + v4l_bound_align_image(&fmt->format.width, IMX378_MIN_WIDTH, > + IMX378_MAX_BOUNDS_WIDTH, ilog2(4), > + &fmt->format.height, IMX378_MIN_HEIGHT, > + IMX378_MAX_BOUNDS_HEIGHT, ilog2(8), 0); > + > + __crop->width = fmt->format.width; > + __crop->height = fmt->format.height; > + > + v4l_bound_align_image(&__crop->left, 0, > + IMX378_MAX_BOUNDS_WIDTH - __crop->width, ilog2(4), > + &__crop->top, 0, > + IMX378_MAX_BOUNDS_HEIGHT - __crop->height, > + ilog2(8), 0); > + > + __fmt = __imx378_get_pad_format(imx378, cfg, fmt->pad, fmt->which); > + > + __fmt->width = __crop->width; > + __fmt->height = __crop->height; > + > + if (fmt->format.code && fmt->which != V4L2_SUBDEV_FORMAT_TRY) > + imx378_set_flip_mode(imx378, fmt->format.code); > + > + __fmt->code = imx378_get_format_code(imx378); > + > + __fmt->field = V4L2_FIELD_NONE; > + __fmt->colorspace = V4L2_COLORSPACE_SRGB; > + __fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->format.colorspace); > + __fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true, > + __fmt->colorspace, __fmt->ycbcr_enc); > + __fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(__fmt->colorspace); > + > + fmt->format = *__fmt; > + > + /* Frame interval depends on the format so, update it accordingly */ > + if (fmt->which != V4L2_SUBDEV_FORMAT_TRY) { > + fival.interval = imx378->fival; > + imx378_s_frame_interval(sd, &fival); > + } > + > + mutex_unlock(&imx378->mutex); > + > + return 0; > +} > + > +static int imx378_get_selection(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_selection *s) > +{ > + struct imx378 *imx378 = to_imx378(sd); > + > + switch (s->target) { > + case V4L2_SEL_TGT_CROP: > + case V4L2_SEL_TGT_COMPOSE: > + s->r = imx378->crop; s->which should be taken into account here, just as in set_fmt above. > + return 0; > + case V4L2_SEL_TGT_COMPOSE_DEFAULT: > + case V4L2_SEL_TGT_CROP_DEFAULT: > + s->r.width = IMX378_MAX_DEFAULT_WIDTH; > + s->r.height = IMX378_MAX_DEFAULT_HEIGHT; > + s->r.left = 0; > + s->r.top = 0; > + return 0; > + case V4L2_SEL_TGT_COMPOSE_BOUNDS: > + case V4L2_SEL_TGT_CROP_BOUNDS: > + s->r.width = IMX378_MAX_BOUNDS_WIDTH; > + s->r.height = IMX378_MAX_BOUNDS_HEIGHT; > + s->r.left = 0; > + s->r.top = 0; > + return 0; > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int imx378_set_selection(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_selection *s) > +{ > + struct imx378 *imx378 = to_imx378(sd); > + int ret; > + > + if (s->target == V4L2_SEL_TGT_COMPOSE) > + return 0; > + > + if (s->target != V4L2_SEL_TGT_CROP) > + return -EINVAL; > + > + mutex_lock(&imx378->mutex); > + > + s->r.width = imx378->crop.width; > + s->r.height = imx378->crop.height; > + > + v4l_bound_align_image(&s->r.left, 0, > + IMX378_MAX_BOUNDS_WIDTH - s->r.width, ilog2(4), > + &s->r.top, 0, > + IMX378_MAX_BOUNDS_HEIGHT - s->r.height, ilog2(8), > + 0); > + > + imx378->crop.left = s->r.left; > + imx378->crop.top = s->r.top; > + > + if (imx378->streaming) { > + ret = imx378_set_img_size(imx378); > + if (ret < 0) { > + dev_err(imx378->dev, "could not set image size\n"); > + mutex_unlock(&imx378->mutex); > + return ret; > + } > + } > + > + mutex_unlock(&imx378->mutex); > + return 0; > +} > + > +static int imx378_set_exposure(struct imx378 *imx378) > +{ > + int ret; > + > + /* Set group hold */ > + ret = regmap_write(imx378->regmap, 0x104, 1); > + if (ret < 0) > + return ret; > + > + ret = imx378_write_reg16(imx378, 0x202, imx378->exposure->val); > + if (ret < 0) > + return ret; > + > + /* Release group hold */ > + return regmap_write(imx378->regmap, 0x104, 0); > +} > + > +static int imx378_set_analogue_gain(struct imx378 *imx378) > +{ > + int ret; > + > + /* Set group hold */ > + ret = regmap_write(imx378->regmap, 0x104, 1); > + if (ret < 0) > + return ret; > + > + ret = imx378_write_reg16(imx378, 0x204, imx378->analogue_gain->val); > + if (ret < 0) > + return ret; > + > + /* Release group hold */ > + return regmap_write(imx378->regmap, 0x104, 0); > +} > + > +static int imx378_set_digital_gain_global(struct imx378 *imx378) > +{ > + int ret; > + > + /* Set group hold */ > + ret = regmap_write(imx378->regmap, 0x104, 1); > + if (ret < 0) > + return ret; > + > + /* Digital gain control mode all color */ > + ret = regmap_write(imx378->regmap, 0x3ff9, 1); > + if (ret < 0) > + return ret; > + > + /* Set digital gain globally */ > + ret = imx378_write_reg16(imx378, 0x20e, imx378->digital_gain->val); > + if (ret < 0) > + return ret; > + > + /* Release group hold */ > + return regmap_write(imx378->regmap, 0x104, 0); > +} > + > +static int imx378_set_digital_gain_by_color(struct imx378 *imx378) > +{ > + int ret; > + > + /* Set group hold */ > + ret = regmap_write(imx378->regmap, 0x104, 1); > + if (ret < 0) > + return ret; > + > + /* Digital gain control mode by color */ > + ret = regmap_write(imx378->regmap, 0x3ff9, 0); > + if (ret < 0) > + return ret; > + > + /* Digital gain R */ > + ret = imx378_write_reg16(imx378, 0x210, imx378->red_balance->val); > + if (ret < 0) > + return ret; > + > + /* Digital gain B */ > + ret = imx378_write_reg16(imx378, 0x212, imx378->blue_balance->val); > + if (ret < 0) > + return ret; > + > + /* Digital gain GR */ > + ret = imx378_write_reg16(imx378, 0x20e, imx378->green_balance->val); > + if (ret < 0) > + return ret; > + > + /* Digital gain GB */ > + ret = imx378_write_reg16(imx378, 0x214, imx378->green_balance->val); > + if (ret < 0) > + return ret; > + > + /* Release group hold */ > + return regmap_write(imx378->regmap, 0x104, 0); > +} > + > +static int imx378_g_temperature(struct imx378 *imx378, s32 *temperature) > +{ > + unsigned int aux; > + int ret; > + > + if (imx378->streaming) { > + ret = regmap_read(imx378->regmap, 0x13a, &aux); > + if (ret < 0) > + return ret; > + > + if (aux >= 0xec) > + *temperature = (aux - 0xec - 20) * 1000; > + else > + *temperature = (aux * 1000); > + } > + > + dev_err(imx378->dev, > + "Sensor is powered off, returning invalid temperature\n"); You could simply return an error here. No kernel messages should be emitted unless there's a non-uAPI related problem. > + > + return 0; > +} > + > +static int imx378_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct imx378 *imx378 = container_of(ctrl->handler, > + struct imx378, ctrls); > + int ret; > + > + /* > + * Applying V4L2 control value only happens > + * when power is up for streaming > + */ > + if (!pm_runtime_get_if_in_use(imx378->dev)) > + return 0; > + > + switch (ctrl->id) { > + case V4L2_CID_TEMPERATURE: > + ret = imx378_g_temperature(imx378, &ctrl->val); > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + pm_runtime_put(imx378->dev); > + > + return ret; > +} > + > +static int imx378_set_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct imx378 *imx378 = container_of(ctrl->handler, > + struct imx378, ctrls); > + int ret; > + > + /* > + * Applying V4L2 control value only happens > + * when power is up for streaming > + */ > + if (!pm_runtime_get_if_in_use(imx378->dev)) > + return 0; > + > + switch (ctrl->id) { > + case V4L2_CID_EXPOSURE_ABSOLUTE: > + ret = imx378_set_exposure(imx378); > + break; > + case V4L2_CID_ANALOGUE_GAIN: > + ret = imx378_set_analogue_gain(imx378); > + break; > + case V4L2_CID_DIGITAL_GAIN: > + ret = imx378_set_digital_gain_global(imx378); > + break; > + case V4L2_CID_RED_BALANCE: > + case V4L2_CID_BLUE_BALANCE: > + case IMX378_CID_GREEN_BALANCE: Perhaps Hans can correct me, but my understanding is the red and blue balances are meant to control the balance of the two while green stays constant. The difference is that the gains that the sensor implements are independent. I wouldn't use the existing controls for them. Also, the green gain is different for those pixels that are adjacent to blue and red pixels horizontally. There's usually no need to set them separately though, but I think it could still make sense to have that in the control API for completeness. I'd add four standard controls for these if you need this feature. > + ret = imx378_set_digital_gain_by_color(imx378); > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + pm_runtime_put(imx378->dev); > + > + return ret; > +} > + > +static const struct v4l2_ctrl_ops imx378_ctrl_ops = { > + .g_volatile_ctrl = imx378_g_volatile_ctrl, > + .s_ctrl = imx378_set_ctrl, > +}; > + > +#define MAX_CMD 4 > +static int imx378_write_table(struct imx378 *imx378, > + const struct reg_8 table[]) > +{ > + u8 vals[MAX_CMD]; > + int i, ret; > + > + for (table = table; table->addr != IMX378_TABLE_END ; table++) { > + if (table->addr == IMX378_TABLE_WAIT_MS) { > + usleep_range(table->val * 1000, > + table->val * 1000 + 500); > + continue; > + } > + > + for (i = 0; i < MAX_CMD; i++) { > + if (table[i].addr != (table[0].addr + i)) > + break; > + vals[i] = table[i].val; > + } > + > + ret = regmap_bulk_write(imx378->regmap, table->addr, vals, i); > + usleep_range(80, 120); > + > + if (ret) { > + dev_err(imx378->dev, "write_table error: %d\n", ret); > + return ret; > + } > + > + table += i - 1; > + } > + > + return 0; > +} > + > +static int imx378_start_streaming(struct imx378 *imx378) > +{ > + int ret; > + > + ret = imx378_write_table(imx378, mode_table_common); > + if (ret < 0) { > + dev_err(imx378->dev, "could not send common table %d\n", ret); > + goto error; > + } > + > + ret = __v4l2_ctrl_handler_setup(&imx378->ctrls); > + if (ret < 0) { > + dev_err(imx378->dev, "could not sync v4l2 controls\n"); > + goto error; > + } > + > + ret = imx378_set_img_size(imx378); > + if (ret < 0) { > + dev_err(imx378->dev, "could not set image size\n"); > + goto error; > + } > + > + /* set orientation */ > + ret = regmap_write(imx378->regmap, 0x101, > + imx378->hflip->val | imx378->vflip->val << 1); > + if (ret < 0) > + goto error; > + > + /* set temperature control */ > + ret = regmap_write(imx378->regmap, 0x138, 1); > + if (ret < 0) > + goto error; Could you use v4l2_ctrl_handler_setup() for doing this instead? Also, in general, using descriptive macros instead of raw register addresses is preferred. > + > + ret = regmap_write(imx378->regmap, 0x100, 1); > + if (ret < 0) { > + dev_err(imx378->dev, "could not send start table %d\n", ret); > + goto error; > + } > + > + usleep_range(4000, 6000); > + > + return 0; > + > +error: > + mutex_unlock(&imx378->mutex); > + return ret; > +} > + > +static int imx378_stop_streaming(struct imx378 *imx378) > +{ > + int ret; > + > + ret = regmap_write(imx378->regmap, 0x100, 0); > + if (ret < 0) > + dev_err(imx378->dev, "could not send stop table %d\n", ret); > + > + return ret; > +} > + > +static int imx378_s_stream(struct v4l2_subdev *sd, int enable) > +{ > + struct imx378 *imx378 = to_imx378(sd); > + int ret; > + > + mutex_lock(&imx378->mutex); > + > + if (imx378->streaming == enable) { > + mutex_unlock(&imx378->mutex); > + return 0; > + } > + > + if (enable) { > + ret = pm_runtime_get_sync(imx378->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(imx378->dev); > + goto err_unlock; > + } > + > + ret = imx378_start_streaming(imx378); > + if (ret < 0) > + goto err_rpm_put; > + } else { > + ret = imx378_stop_streaming(imx378); > + if (ret < 0) > + goto err_rpm_put; > + pm_runtime_put(imx378->dev); > + } > + > + imx378->streaming = enable; > + > + mutex_unlock(&imx378->mutex); > + > + return 0; > + > +err_rpm_put: > + pm_runtime_put(imx378->dev); > +err_unlock: > + mutex_unlock(&imx378->mutex); > + > + return ret; > +} > + > +static int imx378_entity_init_cfg(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg) > +{ > + struct v4l2_subdev_format fmt = { }; > + struct v4l2_subdev_frame_interval fival = { }; > + > + if (!cfg) { This function is never called with cfg being NULL. > + fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : > + V4L2_SUBDEV_FORMAT_ACTIVE; cfg would be always NULL here. > + fmt.format.width = IMX378_MAX_BOUNDS_WIDTH; > + fmt.format.height = IMX378_MAX_BOUNDS_HEIGHT; > + > + imx378_set_fmt(sd, cfg, &fmt); > + > + fival.interval = (struct v4l2_fract) IMX378_DEFAULT_FIVAL; > + imx378_s_frame_interval(sd, &fival); > + } > + > + return 0; > +} > + > +static const struct v4l2_subdev_video_ops imx378_video_ops = { > + .s_stream = imx378_s_stream, > + .g_frame_interval = imx378_g_frame_interval, > + .s_frame_interval = imx378_s_frame_interval, > +}; > + > +static const struct v4l2_subdev_pad_ops imx378_subdev_pad_ops = { > + .enum_mbus_code = imx378_enum_mbus_code, > + .enum_frame_size = imx378_enum_frame_size, > + .enum_frame_interval = imx378_enum_frame_interval, > + .get_fmt = imx378_get_fmt, > + .set_fmt = imx378_set_fmt, > + .get_selection = imx378_get_selection, > + .set_selection = imx378_set_selection, > + .init_cfg = imx378_entity_init_cfg, > +}; > + > +static const struct v4l2_subdev_ops imx378_subdev_ops = { > + .core = &imx378_core_ops, > + .video = &imx378_video_ops, > + .pad = &imx378_subdev_pad_ops, > +}; > + > +static const struct regmap_config sensor_regmap_config = { > + .reg_bits = 16, > + .val_bits = 8, > + .cache_type = REGCACHE_RBTREE, > +}; > + > +static int imx378_get_regulators(struct device *dev, struct imx378 *imx378) > +{ > + unsigned int i; > + > + for (i = 0; i < IMX378_NUM_SUPPLIES; i++) > + imx378->supplies[i].supply = imx378_supply_name[i]; > + > + return devm_regulator_bulk_get(dev, IMX378_NUM_SUPPLIES, > + imx378->supplies); > +} > + > +struct v4l2_ctrl > +*imx378_ctrl_new_str_custom(struct v4l2_ctrl_handler *hdl, u32 id, char *name, > + s64 min, s64 max, u64 step, s64 def) > +{ > + static struct v4l2_ctrl_config ctrl = { > + .ops = &imx378_ctrl_ops, > + .type = V4L2_CTRL_TYPE_STRING, > + }; > + > + ctrl.id = id; > + ctrl.name = name; > + ctrl.min = min; > + ctrl.max = max; > + ctrl.step = step; > + ctrl.def = def; > + > + return v4l2_ctrl_new_custom(hdl, &ctrl, NULL); > +} > + > +struct v4l2_ctrl > +*imx378_ctrl_new_int_custom(struct v4l2_ctrl_handler *hdl, u32 id, char *name, > + s64 min, s64 max, u64 step, s64 def) > +{ > + static struct v4l2_ctrl_config ctrl = { > + .ops = &imx378_ctrl_ops, > + .type = V4L2_CTRL_TYPE_INTEGER, > + }; > + > + ctrl.id = id; > + ctrl.name = name; > + ctrl.min = min; > + ctrl.max = max; > + ctrl.step = step; > + ctrl.def = def; > + > + return v4l2_ctrl_new_custom(hdl, &ctrl, NULL); > +} > + > +static int imx378_init_controls(struct imx378 *imx378) > +{ > + static const s64 link_freq[] = { > + IMX378_DEFAULT_LINK_FREQ, > + }; > + int ret; > + > + mutex_init(&imx378->mutex); > + imx378->ctrls.lock = &imx378->mutex; > + > + v4l2_ctrl_handler_init(&imx378->ctrls, 12); > + > + imx378->pixel_rate = v4l2_ctrl_new_std(&imx378->ctrls, NULL, > + V4L2_CID_PIXEL_RATE, 0, > + IMX378_DEFAULT_PIXEL_RATE, 1, > + IMX378_DEFAULT_PIXEL_RATE); > + > + imx378->link_freq = v4l2_ctrl_new_int_menu(&imx378->ctrls, NULL, > + V4L2_CID_LINK_FREQ, > + ARRAY_SIZE(link_freq) - 1, > + 0, link_freq); > + if (imx378->link_freq) > + imx378->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + imx378->exposure = v4l2_ctrl_new_std(&imx378->ctrls, &imx378_ctrl_ops, > + V4L2_CID_EXPOSURE_ABSOLUTE, > + 0x8, 0xffff, 1, 0xc70); > + > + imx378->hflip = v4l2_ctrl_new_std(&imx378->ctrls, &imx378_ctrl_ops, > + V4L2_CID_HFLIP, 0, 1, 1, 0); > + if (imx378->hflip) > + imx378->hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + imx378->vflip = v4l2_ctrl_new_std(&imx378->ctrls, &imx378_ctrl_ops, > + V4L2_CID_VFLIP, 0, 1, 1, 0); > + if (imx378->vflip) > + imx378->vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + imx378->analogue_gain = v4l2_ctrl_new_std(&imx378->ctrls, > + &imx378_ctrl_ops, > + V4L2_CID_ANALOGUE_GAIN, > + 0, 978, 1, 0); > + > + imx378->digital_gain = v4l2_ctrl_new_std(&imx378->ctrls, > + &imx378_ctrl_ops, V4L2_CID_DIGITAL_GAIN, > + 0x100, 0xfff, 1, 0x100); > + > + imx378->red_balance = v4l2_ctrl_new_std(&imx378->ctrls, > + &imx378_ctrl_ops, V4L2_CID_RED_BALANCE, > + 0x100, 0xfff, 1, 0x221); > + > + imx378->blue_balance = v4l2_ctrl_new_std(&imx378->ctrls, > + &imx378_ctrl_ops, V4L2_CID_BLUE_BALANCE, > + 0x100, 0xfff, 1, 0x1ee); > + > + imx378->green_balance = imx378_ctrl_new_int_custom(&imx378->ctrls, > + IMX378_CID_GREEN_BALANCE, > + "Green Balance", > + 0x100, 0xfff, 1, 0x100); > + if (imx378->green_balance) > + imx378->green_balance->flags = V4L2_CTRL_FLAG_SLIDER; > + > + imx378->temperature = v4l2_ctrl_new_std(&imx378->ctrls, > + &imx378_ctrl_ops, V4L2_CID_TEMPERATURE, > + -200000, 800000, 1, 250000); Those are big numbers. > + if (imx378->temperature) > + imx378->temperature->name = "Sensor Temperature"; Standard controls do not need to be renamed by drivers. > + > + ret = imx378->ctrls.error; > + if (ret) { > + dev_err(imx378->dev, "%s control init failed (%d)\n", > + __func__, ret); > + goto error; > + } > + > + imx378->sd.ctrl_handler = &imx378->ctrls; > + > + return 0; > + > +error: > + v4l2_ctrl_handler_free(&imx378->ctrls); > + return ret; > +} > + > +static int imx378_parse_fwnode(struct device *dev) > +{ > + struct fwnode_handle *endpoint; > + struct v4l2_fwnode_endpoint bus_cfg = { > + .bus_type = V4L2_MBUS_CSI2_DPHY, > + }; > + unsigned int i; > + int ret; > + > + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); > + if (!endpoint) { > + dev_err(dev, "endpoint node not found\n"); > + return -EINVAL; > + } > + > + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg); > + if (ret) { > + dev_err(dev, "parsing endpoint node failed\n"); > + goto done; > + } > + > + for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) > + if (bus_cfg.link_frequencies[i] == IMX378_DEFAULT_LINK_FREQ) > + break; > + > + if (i == bus_cfg.nr_of_link_frequencies) { > + dev_err(dev, "link-frequencies %d not supported, Please review your DT\n", > + IMX378_DEFAULT_LINK_FREQ); > + ret = -EINVAL; > + goto done; > + } > + > +done: > + v4l2_fwnode_endpoint_free(&bus_cfg); > + fwnode_handle_put(endpoint); > + return ret; > +} > + > +static int __maybe_unused imx378_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx378 *imx378 = to_imx378(sd); > + > + if (imx378->streaming) > + imx378_stop_streaming(imx378); > + > + return 0; > +} > + > +static int __maybe_unused imx378_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx378 *imx378 = to_imx378(sd); > + int ret; > + > + if (imx378->streaming) { > + ret = imx378_start_streaming(imx378); > + if (ret) > + goto error; > + } > + > + return 0; > + > +error: > + imx378_stop_streaming(imx378); > + imx378->streaming = 0; > + return ret; > +} > + > +static int imx378_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct imx378 *imx378; > + int ret; > + > + ret = imx378_parse_fwnode(dev); > + if (ret) > + return ret; > + > + imx378 = devm_kzalloc(dev, sizeof(*imx378), GFP_KERNEL); > + if (!imx378) > + return -ENOMEM; > + > + imx378->dev = dev; > + > + imx378->xclk = devm_clk_get(dev, NULL); > + if (IS_ERR(imx378->xclk)) { > + dev_err(dev, "could not get xclk"); > + return PTR_ERR(imx378->xclk); > + } > + > + ret = clk_set_rate(imx378->xclk, IMX378_DEFAULT_CLK_FREQ); > + if (ret) { > + dev_err(dev, "could not set xclk frequency\n"); > + return ret; > + } > + > + ret = imx378_get_regulators(dev, imx378); > + if (ret < 0) { > + dev_err(dev, "could not get regulators\n"); > + return ret; > + } > + > + imx378->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); > + if (IS_ERR(imx378->enable_gpio)) { > + dev_err(dev, "could get enable gpio\n"); > + return PTR_ERR(imx378->enable_gpio); > + } > + > + imx378->regmap = devm_regmap_init_i2c(client, &sensor_regmap_config); > + if (IS_ERR(imx378->regmap)) { > + dev_err(dev, "regmap init failed\n"); > + return PTR_ERR(imx378->regmap); > + } > + > + v4l2_i2c_subdev_init(&imx378->sd, client, &imx378_subdev_ops); > + > + imx378_power_on(imx378->dev); This could fail. Please add error handling. > + > + pm_runtime_set_active(imx378->dev); > + pm_runtime_enable(imx378->dev); > + pm_runtime_idle(imx378->dev); Nice. But I'd make them the last operation in probe. You'll need to power off the sensor if the rest fails --- think what happens when CONFIG_PM disabled. > + > + ret = imx378_init_controls(imx378); > + if (ret) { > + dev_err(dev, "failed to init controls: %d", ret); > + goto error_probe; > + } > + > + imx378->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + imx378->pad.flags = MEDIA_PAD_FL_SOURCE; > + imx378->sd.dev = &client->dev; > + imx378->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > + > + ret = media_entity_pads_init(&imx378->sd.entity, 1, &imx378->pad); > + if (ret < 0) { > + dev_err(dev, "could not register media entity\n"); > + goto free_ctrl; > + } > + > + imx378_entity_init_cfg(&imx378->sd, NULL); > + > + ret = v4l2_async_register_subdev_sensor_common(&imx378->sd); > + if (ret < 0) { > + dev_err(dev, "could not register v4l2 device\n"); > + goto free_entity; > + } > + > + ret = v4l2_device_register_subdev(&imx378->v4l2_dev, &imx378->sd); This isn't needed here; it takes place when the async subdevice is bound. > + if (ret < 0) { > + dev_err(dev, "could not register v4l2 sd\n"); > + goto free_entity; > + } > + > + ret = v4l2_device_register_subdev_nodes(&imx378->v4l2_dev); This shouldn't be called here, but from the bridge driver. > + if (ret) { > + dev_err(dev, "imx378 subdev nodes registration failed (err=%d)\n", > + ret); > + return ret; > + } > + > + return 0; > + > +free_entity: > + media_entity_cleanup(&imx378->sd.entity); > +free_ctrl: > + v4l2_ctrl_handler_free(&imx378->ctrls); > +error_probe: > + mutex_destroy(&imx378->mutex); > + pm_runtime_disable(imx378->dev); > + > + return ret; > +} > + > +static int imx378_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx378 *imx378 = to_imx378(sd); > + > + v4l2_async_unregister_subdev(&imx378->sd); > + media_entity_cleanup(&imx378->sd.entity); > + v4l2_ctrl_handler_free(&imx378->ctrls); > + > + pm_runtime_disable(&client->dev); > + pm_runtime_set_suspended(&client->dev); imx378_power_off(imx378); > + > + mutex_destroy(&imx378->mutex); > + > + return 0; > +} > + > +static const struct of_device_id imx378_of_match[] = { > + { .compatible = "sony,imx378" }, Please add DT bindings for the device. > + { } > +}; > +MODULE_DEVICE_TABLE(of, imx378_of_match); > + > +static const struct i2c_device_id imx378_id[] = { > + {"imx378", 0}, Do you really need i2c device table? If not, please use probe_new and omit it. > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, imx378_id); > + > +static const struct dev_pm_ops imx378_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(imx378_suspend, imx378_resume) > + SET_RUNTIME_PM_OPS(imx378_power_off, imx378_power_on, NULL) > +}; > + > +static struct i2c_driver imx378_i2c_driver = { > + .driver = { > + .of_match_table = imx378_of_match, > + .pm = &imx378_pm_ops, > + .name = "imx378", > + }, > + .probe_new = imx378_probe, > + .remove = imx378_remove, > + .id_table = imx378_id, > +}; > + > +module_i2c_driver(imx378_i2c_driver); > + > +MODULE_DESCRIPTION("Sony IMX378 Camera driver"); > +MODULE_AUTHOR("Daniel Gomez <daniel@qtec.com>"); > +MODULE_LICENSE("GPL v2"); -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver
@ 2020-04-15 7:26 kbuild test robot
0 siblings, 0 replies; 16+ messages in thread
From: kbuild test robot @ 2020-04-15 7:26 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 6194 bytes --]
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20200414200151.80089-4-daniel@qtec.com>
References: <20200414200151.80089-4-daniel@qtec.com>
TO: Daniel Gomez <daniel@qtec.com>
Hi Daniel,
[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.7-rc1 next-20200414]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Daniel-Gomez/v4l2-api-changes-for-imx378-driver/20200415-053427
base: git://linuxtv.org/media_tree.git master
:::::: branch date: 10 hours ago
:::::: commit date: 10 hours ago
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Julia Lawall <julia.lawall@lip6.fr>
coccinelle warnings: (new ones prefixed by >>)
>> drivers/media/i2c/imx378.c:978:2-8: preceding lock on line 975
# https://github.com/0day-ci/linux/commit/f64d4e1406bfb5507f25b050f49c07d89711772d
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout f64d4e1406bfb5507f25b050f49c07d89711772d
vim +978 drivers/media/i2c/imx378.c
f64d4e1406bfb5 Daniel Gomez 2020-04-14 965
f64d4e1406bfb5 Daniel Gomez 2020-04-14 966 static int imx378_set_fmt(struct v4l2_subdev *sd,
f64d4e1406bfb5 Daniel Gomez 2020-04-14 967 struct v4l2_subdev_pad_config *cfg,
f64d4e1406bfb5 Daniel Gomez 2020-04-14 968 struct v4l2_subdev_format *fmt)
f64d4e1406bfb5 Daniel Gomez 2020-04-14 969 {
f64d4e1406bfb5 Daniel Gomez 2020-04-14 970 struct imx378 *imx378 = to_imx378(sd);
f64d4e1406bfb5 Daniel Gomez 2020-04-14 971 struct v4l2_mbus_framefmt *__fmt;
f64d4e1406bfb5 Daniel Gomez 2020-04-14 972 struct v4l2_rect *__crop;
f64d4e1406bfb5 Daniel Gomez 2020-04-14 973 struct v4l2_subdev_frame_interval fival = { };
f64d4e1406bfb5 Daniel Gomez 2020-04-14 974
f64d4e1406bfb5 Daniel Gomez 2020-04-14 @975 mutex_lock(&imx378->mutex);
f64d4e1406bfb5 Daniel Gomez 2020-04-14 976
f64d4e1406bfb5 Daniel Gomez 2020-04-14 977 if (imx378->streaming && fmt->which != V4L2_SUBDEV_FORMAT_TRY)
f64d4e1406bfb5 Daniel Gomez 2020-04-14 @978 return -EBUSY;
f64d4e1406bfb5 Daniel Gomez 2020-04-14 979
f64d4e1406bfb5 Daniel Gomez 2020-04-14 980 __crop = __imx378_get_pad_crop(imx378, cfg, fmt->pad, fmt->which);
f64d4e1406bfb5 Daniel Gomez 2020-04-14 981
f64d4e1406bfb5 Daniel Gomez 2020-04-14 982 v4l_bound_align_image(&fmt->format.width, IMX378_MIN_WIDTH,
f64d4e1406bfb5 Daniel Gomez 2020-04-14 983 IMX378_MAX_BOUNDS_WIDTH, ilog2(4),
f64d4e1406bfb5 Daniel Gomez 2020-04-14 984 &fmt->format.height, IMX378_MIN_HEIGHT,
f64d4e1406bfb5 Daniel Gomez 2020-04-14 985 IMX378_MAX_BOUNDS_HEIGHT, ilog2(8), 0);
f64d4e1406bfb5 Daniel Gomez 2020-04-14 986
f64d4e1406bfb5 Daniel Gomez 2020-04-14 987 __crop->width = fmt->format.width;
f64d4e1406bfb5 Daniel Gomez 2020-04-14 988 __crop->height = fmt->format.height;
f64d4e1406bfb5 Daniel Gomez 2020-04-14 989
f64d4e1406bfb5 Daniel Gomez 2020-04-14 990 v4l_bound_align_image(&__crop->left, 0,
f64d4e1406bfb5 Daniel Gomez 2020-04-14 991 IMX378_MAX_BOUNDS_WIDTH - __crop->width, ilog2(4),
f64d4e1406bfb5 Daniel Gomez 2020-04-14 992 &__crop->top, 0,
f64d4e1406bfb5 Daniel Gomez 2020-04-14 993 IMX378_MAX_BOUNDS_HEIGHT - __crop->height,
f64d4e1406bfb5 Daniel Gomez 2020-04-14 994 ilog2(8), 0);
f64d4e1406bfb5 Daniel Gomez 2020-04-14 995
f64d4e1406bfb5 Daniel Gomez 2020-04-14 996 __fmt = __imx378_get_pad_format(imx378, cfg, fmt->pad, fmt->which);
f64d4e1406bfb5 Daniel Gomez 2020-04-14 997
f64d4e1406bfb5 Daniel Gomez 2020-04-14 998 __fmt->width = __crop->width;
f64d4e1406bfb5 Daniel Gomez 2020-04-14 999 __fmt->height = __crop->height;
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1000
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1001 if (fmt->format.code && fmt->which != V4L2_SUBDEV_FORMAT_TRY)
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1002 imx378_set_flip_mode(imx378, fmt->format.code);
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1003
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1004 __fmt->code = imx378_get_format_code(imx378);
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1005
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1006 __fmt->field = V4L2_FIELD_NONE;
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1007 __fmt->colorspace = V4L2_COLORSPACE_SRGB;
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1008 __fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->format.colorspace);
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1009 __fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1010 __fmt->colorspace, __fmt->ycbcr_enc);
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1011 __fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(__fmt->colorspace);
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1012
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1013 fmt->format = *__fmt;
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1014
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1015 /* Frame interval depends on the format so, update it accordingly */
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1016 if (fmt->which != V4L2_SUBDEV_FORMAT_TRY) {
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1017 fival.interval = imx378->fival;
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1018 imx378_s_frame_interval(sd, &fival);
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1019 }
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1020
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1021 mutex_unlock(&imx378->mutex);
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1022
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1023 return 0;
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1024 }
f64d4e1406bfb5 Daniel Gomez 2020-04-14 1025
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-04-30 14:20 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-14 20:01 [RFC PATCH 0/3] v4l2 api changes for imx378 driver Daniel Gomez 2020-04-14 20:01 ` [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum Daniel Gomez 2020-04-30 9:42 ` Sakari Ailus 2020-04-30 11:10 ` Laurent Pinchart 2020-04-30 13:31 ` Sakari Ailus 2020-04-30 13:59 ` Laurent Pinchart 2020-04-30 14:15 ` Sakari Ailus 2020-04-30 14:17 ` Laurent Pinchart 2020-04-30 14:18 ` Sakari Ailus 2020-04-30 14:20 ` Laurent Pinchart 2020-04-14 20:01 ` [RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature Daniel Gomez 2020-04-28 20:15 ` Sakari Ailus 2020-04-14 20:01 ` [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver Daniel Gomez 2020-04-15 0:37 ` kbuild test robot 2020-04-28 21:02 ` Sakari Ailus 2020-04-15 7:26 kbuild test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.