[RFC PATCH 0/3] v4l2 api changes for imx378 driver

[RFC PATCH 0/3] v4l2 api changes for imx378 driver

[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

[RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum

[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

Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum

[Sakari Ailus]

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

Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum

[Laurent Pinchart]

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

Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum

[Sakari Ailus]

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

Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum

[Laurent Pinchart]

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

Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum

[Sakari Ailus]

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

Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum

[Laurent Pinchart]

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

Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum

[Sakari Ailus]

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

Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum

[Laurent Pinchart]

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

[RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature

[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

Re: [RFC PATCH 2/3] media: v4l2: Add v4l2 control IDs for temperature

[Sakari Ailus]

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

[RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver

[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

Re: [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver

[kbuild test robot]

[-- 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 --]

Re: [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver

[Sakari Ailus]

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

Re: [RFC PATCH 3/3] media: imx378: Add imx378 camera sensor driver

[kbuild test robot]

[-- 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