[PATCH v5 0/6] media: i2c: imx334: support lower bandwidth mode

[PATCH v5 0/6] media: i2c: imx334: support lower bandwidth mode

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

Hi

This patch series is for imx334 sensor driver support for lower bandwidth

Some platforms may not be capable of supporting the bandwidth
required for 12 bit or 3840x2160@60 resolutions.

Add support for dynamically selecting 10 bit and 1920x1080@30
resolutions while leaving the existing configuration as default

Sorry for taking dealy to send updated the series
as i need to check the impact of my patch on 3840x2160@60 resolutions

V4 -> V5
-Added 5 more patchs as per the review comments witch has below updates
-Updated 1782000000Mbps link frequency for 3840x2160@60 as per the mode
values
-Updated 1782000000Mbps link frequency in dt-bindings also
-Updated 3840x2160@60 mode array with default(reset) values

-Updated hblank __v4l2_ctrl_s_ctrl() to __v4l2_ctrl_modify_range()
Suggested-by: Jacopo Mondi <jacopo@jmondi.org>

-Current mode update only when we try to set V4L2_SUBDEV_FORMAT_ACTIVE
-Added link frequency (891000000Mbps) and pixel rate (74250000) to
1920x1080@30 mode
Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>

-Updated commit message

V3 -> V4
- Make the 12 bit and 3840x2160 as default
- Set bus code SRGGB12 if set format fails

V2 -> V3
- Fixed the warning reported by kernel test robot

V1 -> V2
- Addressed the review comment given by Jacopo Mondi,
  Which has bug in imx334_enum_frame_size() loop function,
- Renamed array codes[] to imx334_mbus_codes[]
- Modified supported_modes[] to get higher resolution first

Shravan Chippa (6):
  dt-bindings: imx334: modify link frequency in examples
  media: i2c: imx334: modify link frequency as for the configureation
  media: i2c: imx334: hblank set function modify
  media: i2c: imx334: add default values in 3840x2160@60 array
  media: i2c: imx334: support lower bandwidth mode
  media: i2c: imx334: updating pixel and link frequency

 .../bindings/media/i2c/sony,imx334.yaml       |   2 +-
 drivers/media/i2c/imx334.c                    | 338 ++++++++++++++++--
 2 files changed, 311 insertions(+), 29 deletions(-)

-- 
2.34.1

[PATCH v5 1/6] dt-bindings: imx334: modify link frequency in examples

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

-imx334 sensor is configured to 1782Mbps/lane for 3840x2160@60.
-But in device tree bindings exapmple we are passing 891Mbps/lane
so modified to 1782Mbps

Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
index f5055b9db693..ea3c93f97d65 100644
--- a/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
@@ -82,7 +82,7 @@ examples:
                 imx334: endpoint {
                     remote-endpoint = <&cam>;
                     data-lanes = <1 2 3 4>;
-                    link-frequencies = /bits/ 64 <891000000>;
+                    link-frequencies = /bits/ 64 <1782000000>;
                 };
             };
         };
-- 
2.34.1

Re: [PATCH v5 1/6] dt-bindings: imx334: modify link frequency in examples

[Sakari Ailus]

Hi Shravan,

On Fri, Nov 25, 2022 at 10:38:02AM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
> 
> -imx334 sensor is configured to 1782Mbps/lane for 3840x2160@60.
> -But in device tree bindings exapmple we are passing 891Mbps/lane
> so modified to 1782Mbps
> 
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
> index f5055b9db693..ea3c93f97d65 100644
> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
> @@ -82,7 +82,7 @@ examples:
>                  imx334: endpoint {
>                      remote-endpoint = <&cam>;
>                      data-lanes = <1 2 3 4>;
> -                    link-frequencies = /bits/ 64 <891000000>;
> +                    link-frequencies = /bits/ 64 <1782000000>;

My understanding is that the original frequency is correct --- 594 MHz
pixel clock, 12 bpp, four lanes (and DDR).

>                  };
>              };
>          };

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v5 1/6] dt-bindings: imx334: modify link frequency in examples

[shravan kumar]

On Fri, Nov 25, 2022 at 9:27 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Shravan,
>
> On Fri, Nov 25, 2022 at 10:38:02AM +0530, shravan kumar wrote:
> > From: Shravan Chippa <shravan.chippa@microchip.com>
> >
> > -imx334 sensor is configured to 1782Mbps/lane for 3840x2160@60.
> > -But in device tree bindings exapmple we are passing 891Mbps/lane
> > so modified to 1782Mbps
> >
> > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > ---
> >  Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
> > index f5055b9db693..ea3c93f97d65 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
> > @@ -82,7 +82,7 @@ examples:
> >                  imx334: endpoint {
> >                      remote-endpoint = <&cam>;
> >                      data-lanes = <1 2 3 4>;
> > -                    link-frequencies = /bits/ 64 <891000000>;
> > +                    link-frequencies = /bits/ 64 <1782000000>;
>
> My understanding is that the original frequency is correct --- 594 MHz
> pixel clock, 12 bpp, four lanes (and DDR).

As per my understanding, other than these three parameters there is
one more link frequency (communication frequency from imx334 sensor to
MIPI CSI2 controller).
thanks,

Thanks,
Shravan.

>
> >                  };
> >              };
> >          };
>
> --
> Kind regards,
>
> Sakari Ailus

Re: [PATCH v5 1/6] dt-bindings: imx334: modify link frequency in examples

[Sakari Ailus]

On Sat, Nov 26, 2022 at 04:21:57PM +0530, shravan kumar wrote:
> On Fri, Nov 25, 2022 at 9:27 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > Hi Shravan,
> >
> > On Fri, Nov 25, 2022 at 10:38:02AM +0530, shravan kumar wrote:
> > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > >
> > > -imx334 sensor is configured to 1782Mbps/lane for 3840x2160@60.
> > > -But in device tree bindings exapmple we are passing 891Mbps/lane
> > > so modified to 1782Mbps
> > >
> > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > ---
> > >  Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
> > > index f5055b9db693..ea3c93f97d65 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
> > > @@ -82,7 +82,7 @@ examples:
> > >                  imx334: endpoint {
> > >                      remote-endpoint = <&cam>;
> > >                      data-lanes = <1 2 3 4>;
> > > -                    link-frequencies = /bits/ 64 <891000000>;
> > > +                    link-frequencies = /bits/ 64 <1782000000>;
> >
> > My understanding is that the original frequency is correct --- 594 MHz
> > pixel clock, 12 bpp, four lanes (and DDR).
> 
> As per my understanding, other than these three parameters there is
> one more link frequency (communication frequency from imx334 sensor to
> MIPI CSI2 controller).
> thanks,

Feel free to add additional frequencies later on. But this patch appears to
be just wrong.

-- 
Sakari Ailus

[PATCH v5 2/6] media: i2c: imx334: modify link frequency as for the configureation

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

-Imx334 camera is configured for 1782Mbps/lane for 3840x2160@60
resolution. but the imx334_parse_hw_config() function comparing with
IMX334_LINK_FREQ(891Mbpa/lane) link frequency
-The 3840x2160@60 mode values for 1782Mbps/lane
-Updated Macro IMX334_LINK_FREQ to 1782Mbps/lane
-The same value is populated in the userspce also

Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 7b0a9086447d..acc9f9f15e47 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -49,7 +49,7 @@
 #define IMX334_INCLK_RATE	24000000
 
 /* CSI2 HW configuration */
-#define IMX334_LINK_FREQ	891000000
+#define IMX334_LINK_FREQ	1782000000
 #define IMX334_NUM_DATA_LANES	4
 
 #define IMX334_REG_MIN		0x00
-- 
2.34.1

[PATCH v5 3/6] media: i2c: imx334: hblank set function modify

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

-If we one modes hblank will not change and it is readonly
-If we have multipull modes and if we do mode switch, hblank value will
change and __v4l2_ctrl_s_ctrl() returns error, so modified
function from __v4l2_ctrl_s_ctrl() to __v4l2_ctrl_modify_range()
will updated all values max, min default it is not showing any error
while changing the mode and update value

Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index acc9f9f15e47..d3bb62c162b3 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -382,7 +382,8 @@ static int imx334_update_controls(struct imx334 *imx334,
 	if (ret)
 		return ret;
 
-	ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
+	ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, IMX334_REG_MIN,
+				       IMX334_REG_MAX, 1, mode->hblank);
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH v5 3/6] media: i2c: imx334: hblank set function modify

[Sakari Ailus]

Hi Shravan,

Thanks for the patch.

On Fri, Nov 25, 2022 at 10:38:04AM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
> 
> -If we one modes hblank will not change and it is readonly
> -If we have multipull modes and if we do mode switch, hblank value will
> change and __v4l2_ctrl_s_ctrl() returns error, so modified
> function from __v4l2_ctrl_s_ctrl() to __v4l2_ctrl_modify_range()
> will updated all values max, min default it is not showing any error
> while changing the mode and update value

I think the commit message needs some more work.

> 
> Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  drivers/media/i2c/imx334.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index acc9f9f15e47..d3bb62c162b3 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -382,7 +382,8 @@ static int imx334_update_controls(struct imx334 *imx334,
>  	if (ret)
>  		return ret;
>  
> -	ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
> +	ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, IMX334_REG_MIN,
> +				       IMX334_REG_MAX, 1, mode->hblank);

I'd use mode->hblank for all the three values. This won't be settable by
the user anyway.

I wonder if IMX334_REG_MAX is correct as it would appear to require 20
bits, not 16, to store the value.

>  	if (ret)
>  		return ret;
>  

-- 
Kind regards,

Sakari Ailus

[PATCH v5 4/6] media: i2c: imx334: add default values in 3840x2160@60 array

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

-If we have only one mode there is no need to update camera
reset(default) values when we initialize the camera

-If we have mutipull modes in this case we need all value to
write while camera initializing, so i will not effect other modes
while shifting dynamically

-All default values for 3840x2160@60 updated becouse if we change
the mode we will not be able to recover the values, so add in
mode_3840x2160_regs[] array

Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index d3bb62c162b3..cd41df56ab7d 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -166,6 +166,7 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
 	{0x3288, 0x21},
 	{0x328a, 0x02},
 	{0x302c, 0x3c},
+	{0x302d, 0x00},
 	{0x302e, 0x00},
 	{0x302f, 0x0f},
 	{0x3076, 0x70},
@@ -240,7 +241,26 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
 	{0x3794, 0x7a},
 	{0x3796, 0xa1},
 	{0x3e04, 0x0e},
+	{0x319e, 0x00},
 	{0x3a00, 0x01},
+	{0x3A18, 0xBF},
+	{0x3A19, 0x00},
+	{0x3A1A, 0x67},
+	{0x3A1B, 0x00},
+	{0x3A1C, 0x6F},
+	{0x3A1D, 0x00},
+	{0x3A1E, 0xD7},
+	{0x3A1F, 0x01},
+	{0x3A20, 0x6F},
+	{0x3A21, 0x00},
+	{0x3A22, 0xCF},
+	{0x3A23, 0x00},
+	{0x3A24, 0x6F},
+	{0x3A25, 0x00},
+	{0x3A26, 0xB7},
+	{0x3A27, 0x00},
+	{0x3A28, 0x5F},
+	{0x3A29, 0x00},
 };
 
 /* Supported sensor mode configurations */
-- 
2.34.1

Re: [PATCH v5 4/6] media: i2c: imx334: add default values in 3840x2160@60 array

[Sakari Ailus]

Hi Shravan,

On Fri, Nov 25, 2022 at 10:38:05AM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
> 
> -If we have only one mode there is no need to update camera
> reset(default) values when we initialize the camera

How is this related to the patch?

> 
> -If we have mutipull modes in this case we need all value to
> write while camera initializing, so i will not effect other modes
> while shifting dynamically
> 
> -All default values for 3840x2160@60 updated becouse if we change
> the mode we will not be able to recover the values, so add in
> mode_3840x2160_regs[] array

Please remove dashes in front of the paragrahs and use period in the end of
sentences.

Are these registers' values specific to this mode or are they different on
different modes?

> 
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  drivers/media/i2c/imx334.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index d3bb62c162b3..cd41df56ab7d 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -166,6 +166,7 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
>  	{0x3288, 0x21},
>  	{0x328a, 0x02},
>  	{0x302c, 0x3c},
> +	{0x302d, 0x00},
>  	{0x302e, 0x00},
>  	{0x302f, 0x0f},
>  	{0x3076, 0x70},
> @@ -240,7 +241,26 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
>  	{0x3794, 0x7a},
>  	{0x3796, 0xa1},
>  	{0x3e04, 0x0e},
> +	{0x319e, 0x00},
>  	{0x3a00, 0x01},
> +	{0x3A18, 0xBF},
> +	{0x3A19, 0x00},
> +	{0x3A1A, 0x67},
> +	{0x3A1B, 0x00},
> +	{0x3A1C, 0x6F},
> +	{0x3A1D, 0x00},
> +	{0x3A1E, 0xD7},
> +	{0x3A1F, 0x01},
> +	{0x3A20, 0x6F},
> +	{0x3A21, 0x00},
> +	{0x3A22, 0xCF},
> +	{0x3A23, 0x00},
> +	{0x3A24, 0x6F},
> +	{0x3A25, 0x00},
> +	{0x3A26, 0xB7},
> +	{0x3A27, 0x00},
> +	{0x3A28, 0x5F},
> +	{0x3A29, 0x00},
>  };
>  
>  /* Supported sensor mode configurations */

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v5 4/6] media: i2c: imx334: add default values in 3840x2160@60 array

[shravan kumar]

On Fri, Nov 25, 2022 at 9:50 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Shravan,
>
> On Fri, Nov 25, 2022 at 10:38:05AM +0530, shravan kumar wrote:
> > From: Shravan Chippa <shravan.chippa@microchip.com>
> >
> > -If we have only one mode there is no need to update camera
> > reset(default) values when we initialize the camera
>
> How is this related to the patch?
>
> >
> > -If we have mutipull modes in this case we need all value to
> > write while camera initializing, so i will not effect other modes
> > while shifting dynamically
> >
> > -All default values for 3840x2160@60 updated becouse if we change
> > the mode we will not be able to recover the values, so add in
> > mode_3840x2160_regs[] array
>
> Please remove dashes in front of the paragrahs and use period in the end of
> sentences.
>
> Are these registers' values specific to this mode or are they different on
> different modes?

Yes, These are different for different mods
default values are matching with 3840x2160@60 mode
if we change mode dynamically we need to take care. so added.

Thanks,
Shravan

>
> >
> > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > ---
> >  drivers/media/i2c/imx334.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index d3bb62c162b3..cd41df56ab7d 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -166,6 +166,7 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
> >       {0x3288, 0x21},
> >       {0x328a, 0x02},
> >       {0x302c, 0x3c},
> > +     {0x302d, 0x00},
> >       {0x302e, 0x00},
> >       {0x302f, 0x0f},
> >       {0x3076, 0x70},
> > @@ -240,7 +241,26 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
> >       {0x3794, 0x7a},
> >       {0x3796, 0xa1},
> >       {0x3e04, 0x0e},
> > +     {0x319e, 0x00},
> >       {0x3a00, 0x01},
> > +     {0x3A18, 0xBF},
> > +     {0x3A19, 0x00},
> > +     {0x3A1A, 0x67},
> > +     {0x3A1B, 0x00},
> > +     {0x3A1C, 0x6F},
> > +     {0x3A1D, 0x00},
> > +     {0x3A1E, 0xD7},
> > +     {0x3A1F, 0x01},
> > +     {0x3A20, 0x6F},
> > +     {0x3A21, 0x00},
> > +     {0x3A22, 0xCF},
> > +     {0x3A23, 0x00},
> > +     {0x3A24, 0x6F},
> > +     {0x3A25, 0x00},
> > +     {0x3A26, 0xB7},
> > +     {0x3A27, 0x00},
> > +     {0x3A28, 0x5F},
> > +     {0x3A29, 0x00},
> >  };
> >
> >  /* Supported sensor mode configurations */
>
> --
> Kind regards,
>
> Sakari Ailus

[PATCH v5 5/6] media: i2c: imx334: support lower bandwidth mode

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

Some platforms may not be capable of supporting the bandwidth
required for 12 bit or 3840x2160@60 resolutions.

Add support for dynamically selecting 10 bit and 1920x1080@30
resolutions while leaving the existing configuration as default

CC: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Prakash Battu <Prakash.Battu@microchip.com>
Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 302 +++++++++++++++++++++++++++++++++----
 1 file changed, 276 insertions(+), 26 deletions(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index cd41df56ab7d..0e5575604d87 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -79,7 +79,6 @@ struct imx334_reg_list {
  * struct imx334_mode - imx334 sensor mode structure
  * @width: Frame width
  * @height: Frame height
- * @code: Format code
  * @hblank: Horizontal blanking in lines
  * @vblank: Vertical blanking in lines
  * @vblank_min: Minimal vertical blanking in lines
@@ -91,7 +90,6 @@ struct imx334_reg_list {
 struct imx334_mode {
 	u32 width;
 	u32 height;
-	u32 code;
 	u32 hblank;
 	u32 vblank;
 	u32 vblank_min;
@@ -119,6 +117,7 @@ struct imx334_mode {
  * @vblank: Vertical blanking in lines
  * @cur_mode: Pointer to current selected sensor mode
  * @mutex: Mutex for serializing sensor controls
+ * @cur_code: current selected format code
  * @streaming: Flag indicating streaming state
  */
 struct imx334 {
@@ -140,6 +139,7 @@ struct imx334 {
 	u32 vblank;
 	const struct imx334_mode *cur_mode;
 	struct mutex mutex;
+	u32 cur_code;
 	bool streaming;
 };
 
@@ -147,6 +147,169 @@ static const s64 link_freq[] = {
 	IMX334_LINK_FREQ,
 };
 
+/* Sensor mode registers */
+static const struct imx334_reg mode_1920x1080_regs[] = {
+	{0x3000, 0x01},
+	{0x3018, 0x04},
+	{0x3030, 0xCA},
+	{0x3031, 0x08},
+	{0x3032, 0x00},
+	{0x3034, 0x4C},
+	{0x3035, 0x04},
+	{0x302C, 0xF0},
+	{0x302D, 0x03},
+	{0x302E, 0x80},
+	{0x302F, 0x07},
+	{0x3074, 0xCC},
+	{0x3075, 0x02},
+	{0x308E, 0xCD},
+	{0x308F, 0x02},
+	{0x3076, 0x38},
+	{0x3077, 0x04},
+	{0x3090, 0x38},
+	{0x3091, 0x04},
+	{0x3308, 0x38},
+	{0x3309, 0x04},
+	{0x30C6, 0x00},
+	{0x30C7, 0x00},
+	{0x30CE, 0x00},
+	{0x30CF, 0x00},
+	{0x30D8, 0x18},
+	{0x30D9, 0x0A},
+	{0x304C, 0x00},
+	{0x304E, 0x00},
+	{0x304F, 0x00},
+	{0x3050, 0x00},
+	{0x30B6, 0x00},
+	{0x30B7, 0x00},
+	{0x3116, 0x08},
+	{0x3117, 0x00},
+	{0x31A0, 0x20},
+	{0x31A1, 0x0F},
+	{0x300C, 0x3B},
+	{0x300D, 0x29},
+	{0x314C, 0x29},
+	{0x314D, 0x01},
+	{0x315A, 0x06},
+	{0x3168, 0xA0},
+	{0x316A, 0x7E},
+	{0x319E, 0x02},
+	{0x3199, 0x00},
+	{0x319D, 0x00},
+	{0x31DD, 0x03},
+	{0x3300, 0x00},
+	{0x341C, 0xFF},
+	{0x341D, 0x01},
+	{0x3A01, 0x03},
+	{0x3A18, 0x7F},
+	{0x3A19, 0x00},
+	{0x3A1A, 0x37},
+	{0x3A1B, 0x00},
+	{0x3A1C, 0x37},
+	{0x3A1D, 0x00},
+	{0x3A1E, 0xF7},
+	{0x3A1F, 0x00},
+	{0x3A20, 0x3F},
+	{0x3A21, 0x00},
+	{0x3A20, 0x6F},
+	{0x3A21, 0x00},
+	{0x3A20, 0x3F},
+	{0x3A21, 0x00},
+	{0x3A20, 0x5F},
+	{0x3A21, 0x00},
+	{0x3A20, 0x2F},
+	{0x3A21, 0x00},
+	{0x3078, 0x02},
+	{0x3079, 0x00},
+	{0x307A, 0x00},
+	{0x307B, 0x00},
+	{0x3080, 0x02},
+	{0x3081, 0x00},
+	{0x3082, 0x00},
+	{0x3083, 0x00},
+	{0x3088, 0x02},
+	{0x3094, 0x00},
+	{0x3095, 0x00},
+	{0x3096, 0x00},
+	{0x309B, 0x02},
+	{0x309C, 0x00},
+	{0x309D, 0x00},
+	{0x309E, 0x00},
+	{0x30A4, 0x00},
+	{0x30A5, 0x00},
+	{0x3288, 0x21},
+	{0x328A, 0x02},
+	{0x3414, 0x05},
+	{0x3416, 0x18},
+	{0x35AC, 0x0E},
+	{0x3648, 0x01},
+	{0x364A, 0x04},
+	{0x364C, 0x04},
+	{0x3678, 0x01},
+	{0x367C, 0x31},
+	{0x367E, 0x31},
+	{0x3708, 0x02},
+	{0x3714, 0x01},
+	{0x3715, 0x02},
+	{0x3716, 0x02},
+	{0x3717, 0x02},
+	{0x371C, 0x3D},
+	{0x371D, 0x3F},
+	{0x372C, 0x00},
+	{0x372D, 0x00},
+	{0x372E, 0x46},
+	{0x372F, 0x00},
+	{0x3730, 0x89},
+	{0x3731, 0x00},
+	{0x3732, 0x08},
+	{0x3733, 0x01},
+	{0x3734, 0xFE},
+	{0x3735, 0x05},
+	{0x375D, 0x00},
+	{0x375E, 0x00},
+	{0x375F, 0x61},
+	{0x3760, 0x06},
+	{0x3768, 0x1B},
+	{0x3769, 0x1B},
+	{0x376A, 0x1A},
+	{0x376B, 0x19},
+	{0x376C, 0x18},
+	{0x376D, 0x14},
+	{0x376E, 0x0F},
+	{0x3776, 0x00},
+	{0x3777, 0x00},
+	{0x3778, 0x46},
+	{0x3779, 0x00},
+	{0x377A, 0x08},
+	{0x377B, 0x01},
+	{0x377C, 0x45},
+	{0x377D, 0x01},
+	{0x377E, 0x23},
+	{0x377F, 0x02},
+	{0x3780, 0xD9},
+	{0x3781, 0x03},
+	{0x3782, 0xF5},
+	{0x3783, 0x06},
+	{0x3784, 0xA5},
+	{0x3788, 0x0F},
+	{0x378A, 0xD9},
+	{0x378B, 0x03},
+	{0x378C, 0xEB},
+	{0x378D, 0x05},
+	{0x378E, 0x87},
+	{0x378F, 0x06},
+	{0x3790, 0xF5},
+	{0x3792, 0x43},
+	{0x3794, 0x7A},
+	{0x3796, 0xA1},
+	{0x37B0, 0x37},
+	{0x3E04, 0x0E},
+	{0x30E8, 0x50},
+	{0x30E9, 0x00},
+	{0x3E04, 0x0E},
+	{0x3002, 0x00},
+};
+
 /* Sensor mode registers */
 static const struct imx334_reg mode_3840x2160_regs[] = {
 	{0x3000, 0x01},
@@ -263,20 +426,56 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
 	{0x3A29, 0x00},
 };
 
+static const struct imx334_reg raw10_framefmt_regs[] = {
+	{0x3050, 0x00},
+	{0x319D, 0x00},
+	{0x341C, 0xFF},
+	{0x341D, 0x01},
+};
+
+static const struct imx334_reg raw12_framefmt_regs[] = {
+	{0x3050, 0x01},
+	{0x319D, 0x01},
+	{0x341C, 0x47},
+	{0x341D, 0x00},
+};
+
+/*
+ * The supported BUS formats.
+ */
+static const u32 imx334_mbus_codes[] = {
+	MEDIA_BUS_FMT_SRGGB12_1X12,
+	MEDIA_BUS_FMT_SRGGB10_1X10,
+};
+
 /* Supported sensor mode configurations */
-static const struct imx334_mode supported_mode = {
-	.width = 3840,
-	.height = 2160,
-	.hblank = 560,
-	.vblank = 2340,
-	.vblank_min = 90,
-	.vblank_max = 132840,
-	.pclk = 594000000,
-	.link_freq_idx = 0,
-	.code = MEDIA_BUS_FMT_SRGGB12_1X12,
-	.reg_list = {
-		.num_of_regs = ARRAY_SIZE(mode_3840x2160_regs),
-		.regs = mode_3840x2160_regs,
+static const struct imx334_mode supported_modes[] = {
+	{
+		.width = 3840,
+		.height = 2160,
+		.hblank = 560,
+		.vblank = 2340,
+		.vblank_min = 90,
+		.vblank_max = 132840,
+		.pclk = 594000000,
+		.link_freq_idx = 0,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_3840x2160_regs),
+			.regs = mode_3840x2160_regs,
+		},
+	}, {
+		.width = 1920,
+		.height = 1080,
+		.hblank = 280,
+		.vblank = 1170,
+		.vblank_min = 90,
+		.vblank_max = 132840,
+		.pclk = 74250000,
+		.link_freq_idx = 0,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
+			.regs = mode_1920x1080_regs,
+		},
 	},
 };
 
@@ -527,10 +726,10 @@ static int imx334_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
 {
-	if (code->index > 0)
+	if (code->index >= ARRAY_SIZE(imx334_mbus_codes))
 		return -EINVAL;
 
-	code->code = supported_mode.code;
+	code->code = imx334_mbus_codes[code->index];
 
 	return 0;
 }
@@ -547,15 +746,22 @@ static int imx334_enum_frame_size(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_frame_size_enum *fsize)
 {
-	if (fsize->index > 0)
+	unsigned int i;
+
+	if (fsize->index >= ARRAY_SIZE(supported_modes))
 		return -EINVAL;
 
-	if (fsize->code != supported_mode.code)
+	for (i = 0; i < ARRAY_SIZE(imx334_mbus_codes); ++i) {
+		if (imx334_mbus_codes[i] == fsize->code)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(imx334_mbus_codes))
 		return -EINVAL;
 
-	fsize->min_width = supported_mode.width;
+	fsize->min_width = supported_modes[fsize->index].width;
 	fsize->max_width = fsize->min_width;
-	fsize->min_height = supported_mode.height;
+	fsize->min_height = supported_modes[fsize->index].height;
 	fsize->max_height = fsize->min_height;
 
 	return 0;
@@ -574,7 +780,6 @@ static void imx334_fill_pad_format(struct imx334 *imx334,
 {
 	fmt->format.width = mode->width;
 	fmt->format.height = mode->height;
-	fmt->format.code = mode->code;
 	fmt->format.field = V4L2_FIELD_NONE;
 	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
 	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
@@ -612,6 +817,21 @@ static int imx334_get_pad_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int imx334_get_format_code(struct imx334 *imx334, struct v4l2_subdev_format *fmt)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(imx334_mbus_codes); i++) {
+		if (imx334_mbus_codes[i] == fmt->format.code)
+			break;
+	}
+
+	if (i >= ARRAY_SIZE(imx334_mbus_codes))
+		i = 0;
+
+	return imx334_mbus_codes[i];
+}
+
 /**
  * imx334_set_pad_format() - Set subdevice pad format
  * @sd: pointer to imx334 V4L2 sub-device structure
@@ -630,7 +850,13 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
 
 	mutex_lock(&imx334->mutex);
 
-	mode = &supported_mode;
+	fmt->format.code = imx334_get_format_code(imx334, fmt);
+
+	mode = v4l2_find_nearest_size(supported_modes,
+				      ARRAY_SIZE(supported_modes),
+				      width, height,
+				      fmt->format.width, fmt->format.height);
+
 	imx334_fill_pad_format(imx334, mode, fmt);
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
@@ -638,7 +864,8 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
 
 		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
 		*framefmt = fmt->format;
-	} else {
+	} else if (imx334->cur_mode != mode || imx334->cur_code != fmt->format.code) {
+		imx334->cur_code = fmt->format.code;
 		ret = imx334_update_controls(imx334, mode);
 		if (!ret)
 			imx334->cur_mode = mode;
@@ -663,11 +890,26 @@ static int imx334_init_pad_cfg(struct v4l2_subdev *sd,
 	struct v4l2_subdev_format fmt = { 0 };
 
 	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
-	imx334_fill_pad_format(imx334, &supported_mode, &fmt);
+	imx334_fill_pad_format(imx334, &supported_modes[0], &fmt);
 
 	return imx334_set_pad_format(sd, sd_state, &fmt);
 }
 
+static int imx334_set_framefmt(struct imx334 *imx334)
+{
+	switch (imx334->cur_code) {
+	case MEDIA_BUS_FMT_SRGGB10_1X10:
+		return imx334_write_regs(imx334, raw10_framefmt_regs,
+					ARRAY_SIZE(raw10_framefmt_regs));
+
+	case MEDIA_BUS_FMT_SRGGB12_1X12:
+		return imx334_write_regs(imx334, raw12_framefmt_regs,
+					ARRAY_SIZE(raw12_framefmt_regs));
+	}
+
+	return -EINVAL;
+}
+
 /**
  * imx334_start_streaming() - Start sensor stream
  * @imx334: pointer to imx334 device
@@ -688,6 +930,13 @@ static int imx334_start_streaming(struct imx334 *imx334)
 		return ret;
 	}
 
+	ret = imx334_set_framefmt(imx334);
+	if (ret) {
+		dev_err(imx334->dev, "%s failed to set frame format: %d\n",
+			__func__, ret);
+		return ret;
+	}
+
 	/* Setup handler will write actual exposure and gain */
 	ret =  __v4l2_ctrl_handler_setup(imx334->sd.ctrl_handler);
 	if (ret) {
@@ -1058,7 +1307,8 @@ static int imx334_probe(struct i2c_client *client)
 	}
 
 	/* Set default mode to max resolution */
-	imx334->cur_mode = &supported_mode;
+	imx334->cur_mode = &supported_modes[0];
+	imx334->cur_code = imx334_mbus_codes[0];
 	imx334->vblank = imx334->cur_mode->vblank;
 
 	ret = imx334_init_controls(imx334);
-- 
2.34.1

Re: [PATCH v5 5/6] media: i2c: imx334: support lower bandwidth mode

[Sakari Ailus]

Hi Shravan,

On Fri, Nov 25, 2022 at 10:38:06AM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
> 
> Some platforms may not be capable of supporting the bandwidth
> required for 12 bit or 3840x2160@60 resolutions.
> 
> Add support for dynamically selecting 10 bit and 1920x1080@30
> resolutions while leaving the existing configuration as default
> 
> CC: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Prakash Battu <Prakash.Battu@microchip.com>
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  drivers/media/i2c/imx334.c | 302 +++++++++++++++++++++++++++++++++----
>  1 file changed, 276 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index cd41df56ab7d..0e5575604d87 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -79,7 +79,6 @@ struct imx334_reg_list {
>   * struct imx334_mode - imx334 sensor mode structure
>   * @width: Frame width
>   * @height: Frame height
> - * @code: Format code
>   * @hblank: Horizontal blanking in lines
>   * @vblank: Vertical blanking in lines
>   * @vblank_min: Minimal vertical blanking in lines
> @@ -91,7 +90,6 @@ struct imx334_reg_list {
>  struct imx334_mode {
>  	u32 width;
>  	u32 height;
> -	u32 code;
>  	u32 hblank;
>  	u32 vblank;
>  	u32 vblank_min;
> @@ -119,6 +117,7 @@ struct imx334_mode {
>   * @vblank: Vertical blanking in lines
>   * @cur_mode: Pointer to current selected sensor mode
>   * @mutex: Mutex for serializing sensor controls
> + * @cur_code: current selected format code
>   * @streaming: Flag indicating streaming state
>   */
>  struct imx334 {
> @@ -140,6 +139,7 @@ struct imx334 {
>  	u32 vblank;
>  	const struct imx334_mode *cur_mode;
>  	struct mutex mutex;
> +	u32 cur_code;
>  	bool streaming;
>  };
>  
> @@ -147,6 +147,169 @@ static const s64 link_freq[] = {
>  	IMX334_LINK_FREQ,
>  };
>  
> +/* Sensor mode registers */
> +static const struct imx334_reg mode_1920x1080_regs[] = {
> +	{0x3000, 0x01},
> +	{0x3018, 0x04},
> +	{0x3030, 0xCA},
> +	{0x3031, 0x08},
> +	{0x3032, 0x00},
> +	{0x3034, 0x4C},
> +	{0x3035, 0x04},
> +	{0x302C, 0xF0},
> +	{0x302D, 0x03},
> +	{0x302E, 0x80},
> +	{0x302F, 0x07},
> +	{0x3074, 0xCC},
> +	{0x3075, 0x02},
> +	{0x308E, 0xCD},
> +	{0x308F, 0x02},
> +	{0x3076, 0x38},
> +	{0x3077, 0x04},
> +	{0x3090, 0x38},
> +	{0x3091, 0x04},
> +	{0x3308, 0x38},
> +	{0x3309, 0x04},
> +	{0x30C6, 0x00},
> +	{0x30C7, 0x00},
> +	{0x30CE, 0x00},
> +	{0x30CF, 0x00},
> +	{0x30D8, 0x18},
> +	{0x30D9, 0x0A},
> +	{0x304C, 0x00},
> +	{0x304E, 0x00},
> +	{0x304F, 0x00},
> +	{0x3050, 0x00},
> +	{0x30B6, 0x00},
> +	{0x30B7, 0x00},
> +	{0x3116, 0x08},
> +	{0x3117, 0x00},
> +	{0x31A0, 0x20},
> +	{0x31A1, 0x0F},
> +	{0x300C, 0x3B},
> +	{0x300D, 0x29},
> +	{0x314C, 0x29},
> +	{0x314D, 0x01},
> +	{0x315A, 0x06},
> +	{0x3168, 0xA0},
> +	{0x316A, 0x7E},
> +	{0x319E, 0x02},
> +	{0x3199, 0x00},
> +	{0x319D, 0x00},
> +	{0x31DD, 0x03},
> +	{0x3300, 0x00},
> +	{0x341C, 0xFF},
> +	{0x341D, 0x01},
> +	{0x3A01, 0x03},
> +	{0x3A18, 0x7F},
> +	{0x3A19, 0x00},
> +	{0x3A1A, 0x37},
> +	{0x3A1B, 0x00},
> +	{0x3A1C, 0x37},
> +	{0x3A1D, 0x00},
> +	{0x3A1E, 0xF7},
> +	{0x3A1F, 0x00},
> +	{0x3A20, 0x3F},
> +	{0x3A21, 0x00},
> +	{0x3A20, 0x6F},
> +	{0x3A21, 0x00},
> +	{0x3A20, 0x3F},
> +	{0x3A21, 0x00},
> +	{0x3A20, 0x5F},
> +	{0x3A21, 0x00},
> +	{0x3A20, 0x2F},
> +	{0x3A21, 0x00},
> +	{0x3078, 0x02},
> +	{0x3079, 0x00},
> +	{0x307A, 0x00},
> +	{0x307B, 0x00},
> +	{0x3080, 0x02},
> +	{0x3081, 0x00},
> +	{0x3082, 0x00},
> +	{0x3083, 0x00},
> +	{0x3088, 0x02},
> +	{0x3094, 0x00},
> +	{0x3095, 0x00},
> +	{0x3096, 0x00},
> +	{0x309B, 0x02},
> +	{0x309C, 0x00},
> +	{0x309D, 0x00},
> +	{0x309E, 0x00},
> +	{0x30A4, 0x00},
> +	{0x30A5, 0x00},
> +	{0x3288, 0x21},
> +	{0x328A, 0x02},
> +	{0x3414, 0x05},
> +	{0x3416, 0x18},
> +	{0x35AC, 0x0E},
> +	{0x3648, 0x01},
> +	{0x364A, 0x04},
> +	{0x364C, 0x04},
> +	{0x3678, 0x01},
> +	{0x367C, 0x31},
> +	{0x367E, 0x31},
> +	{0x3708, 0x02},
> +	{0x3714, 0x01},
> +	{0x3715, 0x02},
> +	{0x3716, 0x02},
> +	{0x3717, 0x02},
> +	{0x371C, 0x3D},
> +	{0x371D, 0x3F},
> +	{0x372C, 0x00},
> +	{0x372D, 0x00},
> +	{0x372E, 0x46},
> +	{0x372F, 0x00},
> +	{0x3730, 0x89},
> +	{0x3731, 0x00},
> +	{0x3732, 0x08},
> +	{0x3733, 0x01},
> +	{0x3734, 0xFE},
> +	{0x3735, 0x05},
> +	{0x375D, 0x00},
> +	{0x375E, 0x00},
> +	{0x375F, 0x61},
> +	{0x3760, 0x06},
> +	{0x3768, 0x1B},
> +	{0x3769, 0x1B},
> +	{0x376A, 0x1A},
> +	{0x376B, 0x19},
> +	{0x376C, 0x18},
> +	{0x376D, 0x14},
> +	{0x376E, 0x0F},
> +	{0x3776, 0x00},
> +	{0x3777, 0x00},
> +	{0x3778, 0x46},
> +	{0x3779, 0x00},
> +	{0x377A, 0x08},
> +	{0x377B, 0x01},
> +	{0x377C, 0x45},
> +	{0x377D, 0x01},
> +	{0x377E, 0x23},
> +	{0x377F, 0x02},
> +	{0x3780, 0xD9},
> +	{0x3781, 0x03},
> +	{0x3782, 0xF5},
> +	{0x3783, 0x06},
> +	{0x3784, 0xA5},
> +	{0x3788, 0x0F},
> +	{0x378A, 0xD9},
> +	{0x378B, 0x03},
> +	{0x378C, 0xEB},
> +	{0x378D, 0x05},
> +	{0x378E, 0x87},
> +	{0x378F, 0x06},
> +	{0x3790, 0xF5},
> +	{0x3792, 0x43},
> +	{0x3794, 0x7A},
> +	{0x3796, 0xA1},
> +	{0x37B0, 0x37},
> +	{0x3E04, 0x0E},
> +	{0x30E8, 0x50},
> +	{0x30E9, 0x00},
> +	{0x3E04, 0x0E},
> +	{0x3002, 0x00},
> +};
> +
>  /* Sensor mode registers */
>  static const struct imx334_reg mode_3840x2160_regs[] = {
>  	{0x3000, 0x01},
> @@ -263,20 +426,56 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
>  	{0x3A29, 0x00},
>  };
>  
> +static const struct imx334_reg raw10_framefmt_regs[] = {
> +	{0x3050, 0x00},
> +	{0x319D, 0x00},
> +	{0x341C, 0xFF},
> +	{0x341D, 0x01},
> +};
> +
> +static const struct imx334_reg raw12_framefmt_regs[] = {
> +	{0x3050, 0x01},
> +	{0x319D, 0x01},
> +	{0x341C, 0x47},
> +	{0x341D, 0x00},
> +};
> +
> +/*
> + * The supported BUS formats.
> + */

Redundant comment.

> +static const u32 imx334_mbus_codes[] = {
> +	MEDIA_BUS_FMT_SRGGB12_1X12,
> +	MEDIA_BUS_FMT_SRGGB10_1X10,
> +};
> +
>  /* Supported sensor mode configurations */
> -static const struct imx334_mode supported_mode = {
> -	.width = 3840,
> -	.height = 2160,
> -	.hblank = 560,
> -	.vblank = 2340,
> -	.vblank_min = 90,
> -	.vblank_max = 132840,
> -	.pclk = 594000000,
> -	.link_freq_idx = 0,
> -	.code = MEDIA_BUS_FMT_SRGGB12_1X12,
> -	.reg_list = {
> -		.num_of_regs = ARRAY_SIZE(mode_3840x2160_regs),
> -		.regs = mode_3840x2160_regs,
> +static const struct imx334_mode supported_modes[] = {
> +	{
> +		.width = 3840,
> +		.height = 2160,
> +		.hblank = 560,
> +		.vblank = 2340,
> +		.vblank_min = 90,
> +		.vblank_max = 132840,
> +		.pclk = 594000000,
> +		.link_freq_idx = 0,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_3840x2160_regs),
> +			.regs = mode_3840x2160_regs,
> +		},
> +	}, {
> +		.width = 1920,
> +		.height = 1080,
> +		.hblank = 280,
> +		.vblank = 1170,
> +		.vblank_min = 90,
> +		.vblank_max = 132840,
> +		.pclk = 74250000,
> +		.link_freq_idx = 0,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> +			.regs = mode_1920x1080_regs,
> +		},
>  	},
>  };
>  
> @@ -527,10 +726,10 @@ static int imx334_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum *code)
>  {
> -	if (code->index > 0)
> +	if (code->index >= ARRAY_SIZE(imx334_mbus_codes))
>  		return -EINVAL;
>  
> -	code->code = supported_mode.code;
> +	code->code = imx334_mbus_codes[code->index];
>  
>  	return 0;
>  }
> @@ -547,15 +746,22 @@ static int imx334_enum_frame_size(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_state *sd_state,
>  				  struct v4l2_subdev_frame_size_enum *fsize)
>  {
> -	if (fsize->index > 0)
> +	unsigned int i;
> +
> +	if (fsize->index >= ARRAY_SIZE(supported_modes))
>  		return -EINVAL;
>  
> -	if (fsize->code != supported_mode.code)
> +	for (i = 0; i < ARRAY_SIZE(imx334_mbus_codes); ++i) {
> +		if (imx334_mbus_codes[i] == fsize->code)
> +			break;
> +	}
> +
> +	if (i == ARRAY_SIZE(imx334_mbus_codes))

You could use >= instead, as you do below. You could also refactor this a
little to avoid duplicating the these lines.

>  		return -EINVAL;
>  
> -	fsize->min_width = supported_mode.width;
> +	fsize->min_width = supported_modes[fsize->index].width;
>  	fsize->max_width = fsize->min_width;
> -	fsize->min_height = supported_mode.height;
> +	fsize->min_height = supported_modes[fsize->index].height;
>  	fsize->max_height = fsize->min_height;
>  
>  	return 0;
> @@ -574,7 +780,6 @@ static void imx334_fill_pad_format(struct imx334 *imx334,
>  {
>  	fmt->format.width = mode->width;
>  	fmt->format.height = mode->height;
> -	fmt->format.code = mode->code;
>  	fmt->format.field = V4L2_FIELD_NONE;
>  	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
>  	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> @@ -612,6 +817,21 @@ static int imx334_get_pad_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int imx334_get_format_code(struct imx334 *imx334, struct v4l2_subdev_format *fmt)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(imx334_mbus_codes); i++) {
> +		if (imx334_mbus_codes[i] == fmt->format.code)
> +			break;
> +	}
> +
> +	if (i >= ARRAY_SIZE(imx334_mbus_codes))
> +		i = 0;
> +
> +	return imx334_mbus_codes[i];
> +}
> +
>  /**
>   * imx334_set_pad_format() - Set subdevice pad format
>   * @sd: pointer to imx334 V4L2 sub-device structure
> @@ -630,7 +850,13 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
>  
>  	mutex_lock(&imx334->mutex);
>  
> -	mode = &supported_mode;
> +	fmt->format.code = imx334_get_format_code(imx334, fmt);
> +
> +	mode = v4l2_find_nearest_size(supported_modes,
> +				      ARRAY_SIZE(supported_modes),
> +				      width, height,
> +				      fmt->format.width, fmt->format.height);
> +
>  	imx334_fill_pad_format(imx334, mode, fmt);
>  
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> @@ -638,7 +864,8 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
>  
>  		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
>  		*framefmt = fmt->format;
> -	} else {
> +	} else if (imx334->cur_mode != mode || imx334->cur_code != fmt->format.code) {
> +		imx334->cur_code = fmt->format.code;
>  		ret = imx334_update_controls(imx334, mode);
>  		if (!ret)
>  			imx334->cur_mode = mode;
> @@ -663,11 +890,26 @@ static int imx334_init_pad_cfg(struct v4l2_subdev *sd,
>  	struct v4l2_subdev_format fmt = { 0 };
>  
>  	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> -	imx334_fill_pad_format(imx334, &supported_mode, &fmt);
> +	imx334_fill_pad_format(imx334, &supported_modes[0], &fmt);
>  
>  	return imx334_set_pad_format(sd, sd_state, &fmt);
>  }
>  
> +static int imx334_set_framefmt(struct imx334 *imx334)
> +{
> +	switch (imx334->cur_code) {
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +		return imx334_write_regs(imx334, raw10_framefmt_regs,
> +					ARRAY_SIZE(raw10_framefmt_regs));
> +
> +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> +		return imx334_write_regs(imx334, raw12_framefmt_regs,
> +					ARRAY_SIZE(raw12_framefmt_regs));

Please align just right of the opening parenthesis. The same above.

> +	}
> +
> +	return -EINVAL;
> +}
> +
>  /**
>   * imx334_start_streaming() - Start sensor stream
>   * @imx334: pointer to imx334 device
> @@ -688,6 +930,13 @@ static int imx334_start_streaming(struct imx334 *imx334)
>  		return ret;
>  	}
>  
> +	ret = imx334_set_framefmt(imx334);
> +	if (ret) {
> +		dev_err(imx334->dev, "%s failed to set frame format: %d\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
>  	/* Setup handler will write actual exposure and gain */
>  	ret =  __v4l2_ctrl_handler_setup(imx334->sd.ctrl_handler);
>  	if (ret) {
> @@ -1058,7 +1307,8 @@ static int imx334_probe(struct i2c_client *client)
>  	}
>  
>  	/* Set default mode to max resolution */
> -	imx334->cur_mode = &supported_mode;
> +	imx334->cur_mode = &supported_modes[0];
> +	imx334->cur_code = imx334_mbus_codes[0];
>  	imx334->vblank = imx334->cur_mode->vblank;
>  
>  	ret = imx334_init_controls(imx334);

-- 
Kind regards,

Sakari Ailus

[PATCH v5 6/6] media: i2c: imx334: updating pixel and link frequency

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

-Update pixel_rate and link frequency for 1920x1080@30
Add dummy ctrl cases for pixel_rate and link frequency
to avoid error while changing the modes dynamically

Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 0e5575604d87..ec77835d94a2 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -50,6 +50,7 @@
 
 /* CSI2 HW configuration */
 #define IMX334_LINK_FREQ	1782000000
+#define IMX334_LINK_FREQ_891M	891000000
 #define IMX334_NUM_DATA_LANES	4
 
 #define IMX334_REG_MIN		0x00
@@ -145,6 +146,7 @@ struct imx334 {
 
 static const s64 link_freq[] = {
 	IMX334_LINK_FREQ,
+	IMX334_LINK_FREQ_891M,
 };
 
 /* Sensor mode registers */
@@ -471,7 +473,7 @@ static const struct imx334_mode supported_modes[] = {
 		.vblank_min = 90,
 		.vblank_max = 132840,
 		.pclk = 74250000,
-		.link_freq_idx = 0,
+		.link_freq_idx = 1,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
 			.regs = mode_1920x1080_regs,
@@ -601,6 +603,11 @@ static int imx334_update_controls(struct imx334 *imx334,
 	if (ret)
 		return ret;
 
+	ret = __v4l2_ctrl_modify_range(imx334->pclk_ctrl, mode->pclk,
+				       mode->pclk, 1, mode->pclk);
+	if (ret)
+		return ret;
+
 	ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, IMX334_REG_MIN,
 				       IMX334_REG_MAX, 1, mode->hblank);
 	if (ret)
@@ -700,6 +707,10 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
 
 		pm_runtime_put(imx334->dev);
 
+		break;
+	case V4L2_CID_PIXEL_RATE:
+	case V4L2_CID_LINK_FREQ:
+		ret = 0;
 		break;
 	default:
 		dev_err(imx334->dev, "Invalid control %d", ctrl->id);
-- 
2.34.1