[PATCH RESEND v10 0/5] media: i2c: imx334: support lower bandwidth mode

[PATCH RESEND v10 0/5] 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

V10 -> V10
PATCH RESEND

V9 -> V10
Added new dt-binding patch
added support for handling multiple link-frequncy
minor changes on coding style

V8 -> V9
-Updated all array values with samall later to get unifamity
in mode array values
-corrected hblank_min, hbalank, pix_clk for 1920x1080@30 updated 
according to link frequency
-corrected mutex use for imx334_get_format_code function 
-corrected the fmt->format.code value assinment
-in function imx334_get_format_code variable "i" value comparision
corrected

V7 -> V8
-patch drop "mimx334-odify-link-frequency" as per the commnets
linkfrquncy will be half of the line bandwidth

-changed 1920x1080@30 mode link frequency from (891000000Mbps) 
to (445500000Mbps). linkfrquncy will be half of the line bandwidth

V6 -> V7
Reloved: kernel test robot warning
"drivers/media/i2c/imx334.c:767:15: warning: unused variable 'i' "

V5 -> V6
-Drop the dt-binding patch
-Optimize the code to avoid duplicating the lines
-Added proper mutex while imx334_mbus_codes array
-Modified Function __v4l2_ctrl_modify_range arguments as per the review
commants
-Added hblank dummy set ctrl
-Removed Redundant comment
-corrected code alignment 
-All commit msgs are re-written

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

Shravan Chippa (5):
  media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to
    __v4l2_ctrl_modify_range
  media: i2c: imx334: add missing reset values for mode 3840x2160_regs[]
  media: i2c: imx334: support lower bandwidth mode
  dt-bindings: media: i2c: imx334 add new link_freq
  media: i2c: imx334: update pixel and link frequency

 .../bindings/media/i2c/sony,imx334.yaml       |   2 +-
 drivers/media/i2c/imx334.c                    | 361 ++++++++++++++++--
 2 files changed, 324 insertions(+), 39 deletions(-)

-- 
2.34.1

[PATCH RESEND v10 0/5] 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

V10 -> V10
PATCH RESEND

V9 -> V10
Added new dt-binding patch
added support for handling multiple link-frequncy
minor changes on coding style

V8 -> V9
-Updated all array values with samall later to get unifamity
in mode array values
-corrected hblank_min, hbalank, pix_clk for 1920x1080@30 updated 
according to link frequency
-corrected mutex use for imx334_get_format_code function 
-corrected the fmt->format.code value assinment
-in function imx334_get_format_code variable "i" value comparision
corrected

V7 -> V8
-patch drop "mimx334-odify-link-frequency" as per the commnets
linkfrquncy will be half of the line bandwidth

-changed 1920x1080@30 mode link frequency from (891000000Mbps) 
to (445500000Mbps). linkfrquncy will be half of the line bandwidth

V6 -> V7
Reloved: kernel test robot warning
"drivers/media/i2c/imx334.c:767:15: warning: unused variable 'i' "

V5 -> V6
-Drop the dt-binding patch
-Optimize the code to avoid duplicating the lines
-Added proper mutex while imx334_mbus_codes array
-Modified Function __v4l2_ctrl_modify_range arguments as per the review
commants
-Added hblank dummy set ctrl
-Removed Redundant comment
-corrected code alignment 
-All commit msgs are re-written

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

Shravan Chippa (5):
  media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to
    __v4l2_ctrl_modify_range
  media: i2c: imx334: add missing reset values for mode 3840x2160_regs[]
  media: i2c: imx334: support lower bandwidth mode
  dt-bindings: media: i2c: imx334 add new link_freq
  media: i2c: imx334: update pixel and link frequency

 .../bindings/media/i2c/sony,imx334.yaml       |   2 +-
 drivers/media/i2c/imx334.c                    | 361 ++++++++++++++++--
 2 files changed, 324 insertions(+), 39 deletions(-)

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RESEND v10 1/5] media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to __v4l2_ctrl_modify_range

[shravan kumar]

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

For evry mode we will get new set of values for hbalnk so use
__v4l2_ctrl_modify_range() to support multi modes for hblank.

The hblank value is readonly in the driver. because of this the function
returns error if we try to change. so added dumy return case in
imx334_set_ctrl function.

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

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 7b0a9086447d..ebacba3059b3 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, mode->hblank,
+				       mode->hblank, 1, mode->hblank);
 	if (ret)
 		return ret;
 
@@ -480,6 +481,9 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
 
 		pm_runtime_put(imx334->dev);
 
+		break;
+	case V4L2_CID_HBLANK:
+		ret = 0;
 		break;
 	default:
 		dev_err(imx334->dev, "Invalid control %d", ctrl->id);
-- 
2.34.1

[PATCH RESEND v10 1/5] media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to __v4l2_ctrl_modify_range

[shravan kumar]

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

For evry mode we will get new set of values for hbalnk so use
__v4l2_ctrl_modify_range() to support multi modes for hblank.

The hblank value is readonly in the driver. because of this the function
returns error if we try to change. so added dumy return case in
imx334_set_ctrl function.

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

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 7b0a9086447d..ebacba3059b3 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, mode->hblank,
+				       mode->hblank, 1, mode->hblank);
 	if (ret)
 		return ret;
 
@@ -480,6 +481,9 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
 
 		pm_runtime_put(imx334->dev);
 
+		break;
+	case V4L2_CID_HBLANK:
+		ret = 0;
 		break;
 	default:
 		dev_err(imx334->dev, "Invalid control %d", ctrl->id);
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RESEND v10 2/5] media: i2c: imx334: add missing reset values for mode 3840x2160_regs[]

[shravan kumar]

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

There are some missing reset reg_mode values for the 3840x2160@60
resolution. The camera sensor still works in 3840x2160@60 resolution mode
because of the register reset values. This is an issue when we change the
modes dynamically. As an example, when we change the mode from 1920x1080@30
 resolution to 3840x2160@60 resoultion then the mode values will be written
to the registers from the array mode_3840x2160_regs[] which gives the wrong
output which is incorrect resolution.

So add the missing reset values to the mode_3840x2160_regs[].

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
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 ebacba3059b3..40ece08f20f5 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

[PATCH RESEND v10 2/5] media: i2c: imx334: add missing reset values for mode 3840x2160_regs[]

[shravan kumar]

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

There are some missing reset reg_mode values for the 3840x2160@60
resolution. The camera sensor still works in 3840x2160@60 resolution mode
because of the register reset values. This is an issue when we change the
modes dynamically. As an example, when we change the mode from 1920x1080@30
 resolution to 3840x2160@60 resoultion then the mode values will be written
to the registers from the array mode_3840x2160_regs[] which gives the wrong
output which is incorrect resolution.

So add the missing reset values to the mode_3840x2160_regs[].

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
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 ebacba3059b3..40ece08f20f5 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RESEND v10 3/5] 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.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
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 | 296 +++++++++++++++++++++++++++++++++----
 1 file changed, 269 insertions(+), 27 deletions(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 40ece08f20f5..309c706114d2 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,7 +147,170 @@ static const s64 link_freq[] = {
 	IMX334_LINK_FREQ,
 };
 
-/* Sensor mode registers */
+/* Sensor mode registers for 1920x1080@30fps */
+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 for 3840x2160@30fps */
 static const struct imx334_reg mode_3840x2160_regs[] = {
 	{0x3000, 0x01},
 	{0x3002, 0x00},
@@ -263,20 +426,53 @@ 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},
+};
+
+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 = 2480,
+		.vblank = 1170,
+		.vblank_min = 45,
+		.vblank_max = 132840,
+		.pclk = 297000000,
+		.link_freq_idx = 0,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
+			.regs = mode_1920x1080_regs,
+		},
 	},
 };
 
@@ -518,6 +714,18 @@ static const struct v4l2_ctrl_ops imx334_ctrl_ops = {
 	.s_ctrl = imx334_set_ctrl,
 };
 
+static int imx334_get_format_code(struct imx334 *imx334, u32 code)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(imx334_mbus_codes); i++) {
+		if (imx334_mbus_codes[i] == code)
+			return imx334_mbus_codes[i];
+	}
+
+	return imx334_mbus_codes[0];
+}
+
 /**
  * imx334_enum_mbus_code() - Enumerate V4L2 sub-device mbus codes
  * @sd: pointer to imx334 V4L2 sub-device structure
@@ -530,10 +738,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;
 }
@@ -550,15 +758,20 @@ 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)
+	struct imx334 *imx334 = to_imx334(sd);
+	u32 code;
+
+	if (fsize->index >= ARRAY_SIZE(supported_modes))
 		return -EINVAL;
 
-	if (fsize->code != supported_mode.code)
+	code = imx334_get_format_code(imx334, fsize->code);
+
+	if (fsize->code != code)
 		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;
@@ -577,7 +790,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;
@@ -607,6 +819,7 @@ static int imx334_get_pad_format(struct v4l2_subdev *sd,
 		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
 		fmt->format = *framefmt;
 	} else {
+		fmt->format.code = imx334->cur_code;
 		imx334_fill_pad_format(imx334, imx334->cur_mode, fmt);
 	}
 
@@ -633,15 +846,21 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
 
 	mutex_lock(&imx334->mutex);
 
-	mode = &supported_mode;
+	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);
+	fmt->format.code = imx334_get_format_code(imx334, fmt->format.code);
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
 		struct v4l2_mbus_framefmt *framefmt;
 
 		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;
@@ -666,11 +885,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
@@ -691,6 +925,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) {
@@ -1061,7 +1302,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

[PATCH RESEND v10 3/5] 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.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
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 | 296 +++++++++++++++++++++++++++++++++----
 1 file changed, 269 insertions(+), 27 deletions(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 40ece08f20f5..309c706114d2 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,7 +147,170 @@ static const s64 link_freq[] = {
 	IMX334_LINK_FREQ,
 };
 
-/* Sensor mode registers */
+/* Sensor mode registers for 1920x1080@30fps */
+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 for 3840x2160@30fps */
 static const struct imx334_reg mode_3840x2160_regs[] = {
 	{0x3000, 0x01},
 	{0x3002, 0x00},
@@ -263,20 +426,53 @@ 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},
+};
+
+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 = 2480,
+		.vblank = 1170,
+		.vblank_min = 45,
+		.vblank_max = 132840,
+		.pclk = 297000000,
+		.link_freq_idx = 0,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
+			.regs = mode_1920x1080_regs,
+		},
 	},
 };
 
@@ -518,6 +714,18 @@ static const struct v4l2_ctrl_ops imx334_ctrl_ops = {
 	.s_ctrl = imx334_set_ctrl,
 };
 
+static int imx334_get_format_code(struct imx334 *imx334, u32 code)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(imx334_mbus_codes); i++) {
+		if (imx334_mbus_codes[i] == code)
+			return imx334_mbus_codes[i];
+	}
+
+	return imx334_mbus_codes[0];
+}
+
 /**
  * imx334_enum_mbus_code() - Enumerate V4L2 sub-device mbus codes
  * @sd: pointer to imx334 V4L2 sub-device structure
@@ -530,10 +738,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;
 }
@@ -550,15 +758,20 @@ 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)
+	struct imx334 *imx334 = to_imx334(sd);
+	u32 code;
+
+	if (fsize->index >= ARRAY_SIZE(supported_modes))
 		return -EINVAL;
 
-	if (fsize->code != supported_mode.code)
+	code = imx334_get_format_code(imx334, fsize->code);
+
+	if (fsize->code != code)
 		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;
@@ -577,7 +790,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;
@@ -607,6 +819,7 @@ static int imx334_get_pad_format(struct v4l2_subdev *sd,
 		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
 		fmt->format = *framefmt;
 	} else {
+		fmt->format.code = imx334->cur_code;
 		imx334_fill_pad_format(imx334, imx334->cur_mode, fmt);
 	}
 
@@ -633,15 +846,21 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
 
 	mutex_lock(&imx334->mutex);
 
-	mode = &supported_mode;
+	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);
+	fmt->format.code = imx334_get_format_code(imx334, fmt->format.code);
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
 		struct v4l2_mbus_framefmt *framefmt;
 
 		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;
@@ -666,11 +885,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
@@ -691,6 +925,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) {
@@ -1061,7 +1302,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add new link_freq

[shravan kumar]

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

Add new supported link frequency in dt example.

Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
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..09533496b20c 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 <891000000 445500000>;
                 };
             };
         };
-- 
2.34.1

[PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add new link_freq

[shravan kumar]

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

Add new supported link frequency in dt example.

Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
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..09533496b20c 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 <891000000 445500000>;
                 };
             };
         };
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add new link_freq

[Krzysztof Kozlowski]

On 21/01/2023 04:37, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
> 
> Add new supported link frequency in dt example.

You got comment to fix you CC list. Why not following my feedback?

Best regards,
Krzysztof

Re: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add new link_freq

[Krzysztof Kozlowski]

On 21/01/2023 04:37, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
> 
> Add new supported link frequency in dt example.

You got comment to fix you CC list. Why not following my feedback?

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add new link_freq

[Shravan.Chippa]

Hi,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: 22 January 2023 07:37 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>;
> paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de
> Cc: festevam@gmail.com; kernel@pengutronix.de; linux-imx@nxp.com;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Sakari
> Ailus <sakari.ailus@iki.fi>
> Subject: Re: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add
> new link_freq
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On 21/01/2023 04:37, shravan kumar wrote:
> > From: Shravan Chippa <shravan.chippa@microchip.com>
> >
> > Add new supported link frequency in dt example.
> 
> You got a comment to fix you CC list. Why not follow my feedback?

Based on your previous comment I ran the below script, rebased to the latest code and based on that output I have added a CC list
But I missed adding one name to the to-list which is  "Rob Herring <robh+dt@kernel.org>", I will add it.
Did I miss any other entry in the CC list?

*************************************
./scripts/get_maintainer.pl Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
"Paul J. Murphy" <paul.j.murphy@intel.com> (maintainer:SONY IMX334 SENSOR DRIVER,in file)
Daniele Alessandrelli <daniele.alessandrelli@intel.com> (maintainer:SONY IMX334 SENSOR DRIVER,in file)
Mauro Carvalho Chehab <mchehab@kernel.org> (maintainer:MEDIA INPUT INFRASTRUCTURE (V4L/DVB))
Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Shawn Guo <shawnguo@kernel.org> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
Sascha Hauer <s.hauer@pengutronix.de> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
Pengutronix Kernel Team <kernel@pengutronix.de> (reviewer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
Fabio Estevam <festevam@gmail.com> (reviewer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
NXP Linux Team <linux-imx@nxp.com> (reviewer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
linux-media@vger.kernel.org (open list:SONY IMX334 SENSOR DRIVER)
devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
linux-arm-kernel@lists.infradead.org (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
linux-kernel@vger.kernel.org (open list)

./scripts/get_maintainer.pl drivers/media/i2c/imx334.c
"Paul J. Murphy" <paul.j.murphy@intel.com> (maintainer:SONY IMX334 SENSOR DRIVER)
Daniele Alessandrelli <daniele.alessandrelli@intel.com> (maintainer:SONY IMX334 SENSOR DRIVER)
Mauro Carvalho Chehab <mchehab@kernel.org> (maintainer:MEDIA INPUT INFRASTRUCTURE (V4L/DVB))
linux-media@vger.kernel.org (open list:SONY IMX334 SENSOR DRIVER)
linux-kernel@vger.kernel.org (open list)
**************************
> 
> Best regards,
> Krzysztof

RE: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add new link_freq

[Shravan.Chippa]

Hi,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: 22 January 2023 07:37 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>;
> paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de
> Cc: festevam@gmail.com; kernel@pengutronix.de; linux-imx@nxp.com;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Sakari
> Ailus <sakari.ailus@iki.fi>
> Subject: Re: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add
> new link_freq
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On 21/01/2023 04:37, shravan kumar wrote:
> > From: Shravan Chippa <shravan.chippa@microchip.com>
> >
> > Add new supported link frequency in dt example.
> 
> You got a comment to fix you CC list. Why not follow my feedback?

Based on your previous comment I ran the below script, rebased to the latest code and based on that output I have added a CC list
But I missed adding one name to the to-list which is  "Rob Herring <robh+dt@kernel.org>", I will add it.
Did I miss any other entry in the CC list?

*************************************
./scripts/get_maintainer.pl Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml
"Paul J. Murphy" <paul.j.murphy@intel.com> (maintainer:SONY IMX334 SENSOR DRIVER,in file)
Daniele Alessandrelli <daniele.alessandrelli@intel.com> (maintainer:SONY IMX334 SENSOR DRIVER,in file)
Mauro Carvalho Chehab <mchehab@kernel.org> (maintainer:MEDIA INPUT INFRASTRUCTURE (V4L/DVB))
Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Shawn Guo <shawnguo@kernel.org> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
Sascha Hauer <s.hauer@pengutronix.de> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
Pengutronix Kernel Team <kernel@pengutronix.de> (reviewer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
Fabio Estevam <festevam@gmail.com> (reviewer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
NXP Linux Team <linux-imx@nxp.com> (reviewer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
linux-media@vger.kernel.org (open list:SONY IMX334 SENSOR DRIVER)
devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
linux-arm-kernel@lists.infradead.org (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE)
linux-kernel@vger.kernel.org (open list)

./scripts/get_maintainer.pl drivers/media/i2c/imx334.c
"Paul J. Murphy" <paul.j.murphy@intel.com> (maintainer:SONY IMX334 SENSOR DRIVER)
Daniele Alessandrelli <daniele.alessandrelli@intel.com> (maintainer:SONY IMX334 SENSOR DRIVER)
Mauro Carvalho Chehab <mchehab@kernel.org> (maintainer:MEDIA INPUT INFRASTRUCTURE (V4L/DVB))
linux-media@vger.kernel.org (open list:SONY IMX334 SENSOR DRIVER)
linux-kernel@vger.kernel.org (open list)
**************************
> 
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add new link_freq

[Krzysztof Kozlowski]

On 23/01/2023 07:28, Shravan.Chippa@microchip.com wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: 22 January 2023 07:37 PM
>> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>;
>> paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
>> mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
>> shawnguo@kernel.org; s.hauer@pengutronix.de
>> Cc: festevam@gmail.com; kernel@pengutronix.de; linux-imx@nxp.com;
>> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Sakari
>> Ailus <sakari.ailus@iki.fi>
>> Subject: Re: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add
>> new link_freq
>>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>
>> On 21/01/2023 04:37, shravan kumar wrote:
>>> From: Shravan Chippa <shravan.chippa@microchip.com>
>>>
>>> Add new supported link frequency in dt example.
>>
>> You got a comment to fix you CC list. Why not follow my feedback?
> 
> Based on your previous comment I ran the below script, rebased to the latest code and based on that output I have added a CC list
> But I missed adding one name to the to-list which is  "Rob Herring <robh+dt@kernel.org>", I will add it.

Which is quite important...

Best regards,
Krzysztof

Re: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add new link_freq

[Krzysztof Kozlowski]

On 23/01/2023 07:28, Shravan.Chippa@microchip.com wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: 22 January 2023 07:37 PM
>> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>;
>> paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
>> mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
>> shawnguo@kernel.org; s.hauer@pengutronix.de
>> Cc: festevam@gmail.com; kernel@pengutronix.de; linux-imx@nxp.com;
>> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Sakari
>> Ailus <sakari.ailus@iki.fi>
>> Subject: Re: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add
>> new link_freq
>>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>
>> On 21/01/2023 04:37, shravan kumar wrote:
>>> From: Shravan Chippa <shravan.chippa@microchip.com>
>>>
>>> Add new supported link frequency in dt example.
>>
>> You got a comment to fix you CC list. Why not follow my feedback?
> 
> Based on your previous comment I ran the below script, rebased to the latest code and based on that output I have added a CC list
> But I missed adding one name to the to-list which is  "Rob Herring <robh+dt@kernel.org>", I will add it.

Which is quite important...

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add new link_freq

[Rob Herring]

On Mon, Jan 23, 2023 at 09:00:30AM +0100, Krzysztof Kozlowski wrote:
> On 23/01/2023 07:28, Shravan.Chippa@microchip.com wrote:
> > Hi,
> > 
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: 22 January 2023 07:37 PM
> >> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>;
> >> paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> >> mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> >> shawnguo@kernel.org; s.hauer@pengutronix.de
> >> Cc: festevam@gmail.com; kernel@pengutronix.de; linux-imx@nxp.com;
> >> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Sakari
> >> Ailus <sakari.ailus@iki.fi>
> >> Subject: Re: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add
> >> new link_freq
> >>
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> >> content is safe
> >>
> >> On 21/01/2023 04:37, shravan kumar wrote:
> >>> From: Shravan Chippa <shravan.chippa@microchip.com>
> >>>
> >>> Add new supported link frequency in dt example.
> >>
> >> You got a comment to fix you CC list. Why not follow my feedback?
> > 
> > Based on your previous comment I ran the below script, rebased to the latest code and based on that output I have added a CC list
> > But I missed adding one name to the to-list which is  "Rob Herring <robh+dt@kernel.org>", I will add it.
> 
> Which is quite important...

Not really. I've long since given up trying to filter based on that. The 
DT list vs. to me is about the same firehose. Expecting senders to get 
it right or figure out what's important to you or not will never happen. 
With lore now, there's little need as I control what I get or not.

Rob

Re: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add new link_freq

[Rob Herring]

On Mon, Jan 23, 2023 at 09:00:30AM +0100, Krzysztof Kozlowski wrote:
> On 23/01/2023 07:28, Shravan.Chippa@microchip.com wrote:
> > Hi,
> > 
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: 22 January 2023 07:37 PM
> >> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>;
> >> paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> >> mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> >> shawnguo@kernel.org; s.hauer@pengutronix.de
> >> Cc: festevam@gmail.com; kernel@pengutronix.de; linux-imx@nxp.com;
> >> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Sakari
> >> Ailus <sakari.ailus@iki.fi>
> >> Subject: Re: [PATCH RESEND v10 4/5] dt-bindings: media: i2c: imx334 add
> >> new link_freq
> >>
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> >> content is safe
> >>
> >> On 21/01/2023 04:37, shravan kumar wrote:
> >>> From: Shravan Chippa <shravan.chippa@microchip.com>
> >>>
> >>> Add new supported link frequency in dt example.
> >>
> >> You got a comment to fix you CC list. Why not follow my feedback?
> > 
> > Based on your previous comment I ran the below script, rebased to the latest code and based on that output I have added a CC list
> > But I missed adding one name to the to-list which is  "Rob Herring <robh+dt@kernel.org>", I will add it.
> 
> Which is quite important...

Not really. I've long since given up trying to filter based on that. The 
DT list vs. to me is about the same firehose. Expecting senders to get 
it right or figure out what's important to you or not will never happen. 
With lore now, there's little need as I control what I get or not.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[shravan kumar]

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

Update pixel_rate and link frequency for 1920x1080@30
while changing mode.

Add dummy ctrl cases for pixel_rate and link frequency
to avoid error while changing the modes dynamically.

Add support to handle multiple link frequencies.

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

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 309c706114d2..62b104eaa437 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -49,7 +49,8 @@
 #define IMX334_INCLK_RATE	24000000
 
 /* CSI2 HW configuration */
-#define IMX334_LINK_FREQ	891000000
+#define IMX334_LINK_FREQ_891M	891000000
+#define IMX334_LINK_FREQ_445M	445500000
 #define IMX334_NUM_DATA_LANES	4
 
 #define IMX334_REG_MIN		0x00
@@ -139,12 +140,14 @@ struct imx334 {
 	u32 vblank;
 	const struct imx334_mode *cur_mode;
 	struct mutex mutex;
+	unsigned long menu_skip_mask;
 	u32 cur_code;
 	bool streaming;
 };
 
 static const s64 link_freq[] = {
-	IMX334_LINK_FREQ,
+	IMX334_LINK_FREQ_891M,
+	IMX334_LINK_FREQ_445M,
 };
 
 /* Sensor mode registers for 1920x1080@30fps */
@@ -468,7 +471,7 @@ static const struct imx334_mode supported_modes[] = {
 		.vblank_min = 45,
 		.vblank_max = 132840,
 		.pclk = 297000000,
-		.link_freq_idx = 0,
+		.link_freq_idx = 1,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
 			.regs = mode_1920x1080_regs,
@@ -598,6 +601,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, mode->hblank,
 				       mode->hblank, 1, mode->hblank);
 	if (ret)
@@ -698,6 +706,8 @@ 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:
 	case V4L2_CID_HBLANK:
 		ret = 0;
 		break;
@@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
 	struct fwnode_handle *ep;
 	unsigned long rate;
 	int ret;
-	int i;
+	int i, j;
 
 	if (!fwnode)
 		return -ENXIO;
@@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
 		goto done_endpoint_free;
 	}
 
-	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
-		if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
+	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
+		for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
+			if (bus_cfg.link_frequencies[i] == link_freq[j]) {
+				set_bit(j, &imx334->menu_skip_mask);
+				break;
+			}
+		}
+
+		if (j == ARRAY_SIZE(link_freq)) {
+			ret = dev_err_probe(imx334->dev, -EINVAL,
+					    "no supported link freq found\n");
 			goto done_endpoint_free;
-
-	ret = -EINVAL;
+		}
+	}
 
 done_endpoint_free:
 	v4l2_fwnode_endpoint_free(&bus_cfg);
@@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct imx334 *imx334)
 	imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
 							&imx334_ctrl_ops,
 							V4L2_CID_LINK_FREQ,
-							ARRAY_SIZE(link_freq) -
-							1,
-							mode->link_freq_idx,
+							__fls(imx334->menu_skip_mask),
+							__ffs(imx334->menu_skip_mask),
 							link_freq);
+
 	if (imx334->link_freq_ctrl)
 		imx334->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
-- 
2.34.1

[PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[shravan kumar]

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

Update pixel_rate and link frequency for 1920x1080@30
while changing mode.

Add dummy ctrl cases for pixel_rate and link frequency
to avoid error while changing the modes dynamically.

Add support to handle multiple link frequencies.

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

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 309c706114d2..62b104eaa437 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -49,7 +49,8 @@
 #define IMX334_INCLK_RATE	24000000
 
 /* CSI2 HW configuration */
-#define IMX334_LINK_FREQ	891000000
+#define IMX334_LINK_FREQ_891M	891000000
+#define IMX334_LINK_FREQ_445M	445500000
 #define IMX334_NUM_DATA_LANES	4
 
 #define IMX334_REG_MIN		0x00
@@ -139,12 +140,14 @@ struct imx334 {
 	u32 vblank;
 	const struct imx334_mode *cur_mode;
 	struct mutex mutex;
+	unsigned long menu_skip_mask;
 	u32 cur_code;
 	bool streaming;
 };
 
 static const s64 link_freq[] = {
-	IMX334_LINK_FREQ,
+	IMX334_LINK_FREQ_891M,
+	IMX334_LINK_FREQ_445M,
 };
 
 /* Sensor mode registers for 1920x1080@30fps */
@@ -468,7 +471,7 @@ static const struct imx334_mode supported_modes[] = {
 		.vblank_min = 45,
 		.vblank_max = 132840,
 		.pclk = 297000000,
-		.link_freq_idx = 0,
+		.link_freq_idx = 1,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
 			.regs = mode_1920x1080_regs,
@@ -598,6 +601,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, mode->hblank,
 				       mode->hblank, 1, mode->hblank);
 	if (ret)
@@ -698,6 +706,8 @@ 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:
 	case V4L2_CID_HBLANK:
 		ret = 0;
 		break;
@@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
 	struct fwnode_handle *ep;
 	unsigned long rate;
 	int ret;
-	int i;
+	int i, j;
 
 	if (!fwnode)
 		return -ENXIO;
@@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
 		goto done_endpoint_free;
 	}
 
-	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
-		if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
+	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
+		for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
+			if (bus_cfg.link_frequencies[i] == link_freq[j]) {
+				set_bit(j, &imx334->menu_skip_mask);
+				break;
+			}
+		}
+
+		if (j == ARRAY_SIZE(link_freq)) {
+			ret = dev_err_probe(imx334->dev, -EINVAL,
+					    "no supported link freq found\n");
 			goto done_endpoint_free;
-
-	ret = -EINVAL;
+		}
+	}
 
 done_endpoint_free:
 	v4l2_fwnode_endpoint_free(&bus_cfg);
@@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct imx334 *imx334)
 	imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
 							&imx334_ctrl_ops,
 							V4L2_CID_LINK_FREQ,
-							ARRAY_SIZE(link_freq) -
-							1,
-							mode->link_freq_idx,
+							__fls(imx334->menu_skip_mask),
+							__ffs(imx334->menu_skip_mask),
 							link_freq);
+
 	if (imx334->link_freq_ctrl)
 		imx334->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[Sakari Ailus]

Hi Shravan,

On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
> 
> Update pixel_rate and link frequency for 1920x1080@30
> while changing mode.
> 
> Add dummy ctrl cases for pixel_rate and link frequency
> to avoid error while changing the modes dynamically.
> 
> Add support to handle multiple link frequencies.
> 
> Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  drivers/media/i2c/imx334.c | 41 ++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 309c706114d2..62b104eaa437 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -49,7 +49,8 @@
>  #define IMX334_INCLK_RATE	24000000
>  
>  /* CSI2 HW configuration */
> -#define IMX334_LINK_FREQ	891000000
> +#define IMX334_LINK_FREQ_891M	891000000
> +#define IMX334_LINK_FREQ_445M	445500000
>  #define IMX334_NUM_DATA_LANES	4
>  
>  #define IMX334_REG_MIN		0x00
> @@ -139,12 +140,14 @@ struct imx334 {
>  	u32 vblank;
>  	const struct imx334_mode *cur_mode;
>  	struct mutex mutex;
> +	unsigned long menu_skip_mask;
>  	u32 cur_code;
>  	bool streaming;
>  };
>  
>  static const s64 link_freq[] = {
> -	IMX334_LINK_FREQ,
> +	IMX334_LINK_FREQ_891M,
> +	IMX334_LINK_FREQ_445M,
>  };
>  
>  /* Sensor mode registers for 1920x1080@30fps */
> @@ -468,7 +471,7 @@ static const struct imx334_mode supported_modes[] = {
>  		.vblank_min = 45,
>  		.vblank_max = 132840,
>  		.pclk = 297000000,
> -		.link_freq_idx = 0,
> +		.link_freq_idx = 1,
>  		.reg_list = {
>  			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
>  			.regs = mode_1920x1080_regs,
> @@ -598,6 +601,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, mode->hblank,
>  				       mode->hblank, 1, mode->hblank);
>  	if (ret)
> @@ -698,6 +706,8 @@ 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:
>  	case V4L2_CID_HBLANK:
>  		ret = 0;
>  		break;
> @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
>  	struct fwnode_handle *ep;
>  	unsigned long rate;
>  	int ret;
> -	int i;
> +	int i, j;

unsigned int would be nicer.

>  
>  	if (!fwnode)
>  		return -ENXIO;
> @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
>  		goto done_endpoint_free;
>  	}
>  
> -	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> -		if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> +	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> +		for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> +			if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> +				set_bit(j, &imx334->menu_skip_mask);

Is there a guarantee that you'll only be using the modes with the listed
frequencies? I don't see one but I might have missed it.

> +				break;
> +			}
> +		}
> +
> +		if (j == ARRAY_SIZE(link_freq)) {
> +			ret = dev_err_probe(imx334->dev, -EINVAL,
> +					    "no supported link freq found\n");
>  			goto done_endpoint_free;
> -
> -	ret = -EINVAL;
> +		}
> +	}
>  
>  done_endpoint_free:
>  	v4l2_fwnode_endpoint_free(&bus_cfg);
> @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct imx334 *imx334)
>  	imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>  							&imx334_ctrl_ops,
>  							V4L2_CID_LINK_FREQ,
> -							ARRAY_SIZE(link_freq) -
> -							1,
> -							mode->link_freq_idx,
> +							__fls(imx334->menu_skip_mask),
> +							__ffs(imx334->menu_skip_mask),
>  							link_freq);
> +
>  	if (imx334->link_freq_ctrl)
>  		imx334->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  

-- 
Kind regards,

Sakari Ailus

Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[Sakari Ailus]

Hi Shravan,

On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
> 
> Update pixel_rate and link frequency for 1920x1080@30
> while changing mode.
> 
> Add dummy ctrl cases for pixel_rate and link frequency
> to avoid error while changing the modes dynamically.
> 
> Add support to handle multiple link frequencies.
> 
> Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  drivers/media/i2c/imx334.c | 41 ++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 309c706114d2..62b104eaa437 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -49,7 +49,8 @@
>  #define IMX334_INCLK_RATE	24000000
>  
>  /* CSI2 HW configuration */
> -#define IMX334_LINK_FREQ	891000000
> +#define IMX334_LINK_FREQ_891M	891000000
> +#define IMX334_LINK_FREQ_445M	445500000
>  #define IMX334_NUM_DATA_LANES	4
>  
>  #define IMX334_REG_MIN		0x00
> @@ -139,12 +140,14 @@ struct imx334 {
>  	u32 vblank;
>  	const struct imx334_mode *cur_mode;
>  	struct mutex mutex;
> +	unsigned long menu_skip_mask;
>  	u32 cur_code;
>  	bool streaming;
>  };
>  
>  static const s64 link_freq[] = {
> -	IMX334_LINK_FREQ,
> +	IMX334_LINK_FREQ_891M,
> +	IMX334_LINK_FREQ_445M,
>  };
>  
>  /* Sensor mode registers for 1920x1080@30fps */
> @@ -468,7 +471,7 @@ static const struct imx334_mode supported_modes[] = {
>  		.vblank_min = 45,
>  		.vblank_max = 132840,
>  		.pclk = 297000000,
> -		.link_freq_idx = 0,
> +		.link_freq_idx = 1,
>  		.reg_list = {
>  			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
>  			.regs = mode_1920x1080_regs,
> @@ -598,6 +601,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, mode->hblank,
>  				       mode->hblank, 1, mode->hblank);
>  	if (ret)
> @@ -698,6 +706,8 @@ 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:
>  	case V4L2_CID_HBLANK:
>  		ret = 0;
>  		break;
> @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
>  	struct fwnode_handle *ep;
>  	unsigned long rate;
>  	int ret;
> -	int i;
> +	int i, j;

unsigned int would be nicer.

>  
>  	if (!fwnode)
>  		return -ENXIO;
> @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
>  		goto done_endpoint_free;
>  	}
>  
> -	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> -		if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> +	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> +		for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> +			if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> +				set_bit(j, &imx334->menu_skip_mask);

Is there a guarantee that you'll only be using the modes with the listed
frequencies? I don't see one but I might have missed it.

> +				break;
> +			}
> +		}
> +
> +		if (j == ARRAY_SIZE(link_freq)) {
> +			ret = dev_err_probe(imx334->dev, -EINVAL,
> +					    "no supported link freq found\n");
>  			goto done_endpoint_free;
> -
> -	ret = -EINVAL;
> +		}
> +	}
>  
>  done_endpoint_free:
>  	v4l2_fwnode_endpoint_free(&bus_cfg);
> @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct imx334 *imx334)
>  	imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>  							&imx334_ctrl_ops,
>  							V4L2_CID_LINK_FREQ,
> -							ARRAY_SIZE(link_freq) -
> -							1,
> -							mode->link_freq_idx,
> +							__fls(imx334->menu_skip_mask),
> +							__ffs(imx334->menu_skip_mask),
>  							link_freq);
> +
>  	if (imx334->link_freq_ctrl)
>  		imx334->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  

-- 
Kind regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[Shravan.Chippa]

Hi Sakari,

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@iki.fi>
> Sent: 24 January 2023 04:33 AM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link
> frequency
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Shravan,
> 
> On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> > From: Shravan Chippa <shravan.chippa@microchip.com>
> >
> > Update pixel_rate and link frequency for 1920x1080@30 while changing
> > mode.
> >
> > Add dummy ctrl cases for pixel_rate and link frequency to avoid error
> > while changing the modes dynamically.
> >
> > Add support to handle multiple link frequencies.
> >
> > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > ---
> >  drivers/media/i2c/imx334.c | 41
> > ++++++++++++++++++++++++++++----------
> >  1 file changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index 309c706114d2..62b104eaa437 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -49,7 +49,8 @@
> >  #define IMX334_INCLK_RATE    24000000
> >
> >  /* CSI2 HW configuration */
> > -#define IMX334_LINK_FREQ     891000000
> > +#define IMX334_LINK_FREQ_891M        891000000
> > +#define IMX334_LINK_FREQ_445M        445500000
> >  #define IMX334_NUM_DATA_LANES        4
> >
> >  #define IMX334_REG_MIN               0x00
> > @@ -139,12 +140,14 @@ struct imx334 {
> >       u32 vblank;
> >       const struct imx334_mode *cur_mode;
> >       struct mutex mutex;
> > +     unsigned long menu_skip_mask;
> >       u32 cur_code;
> >       bool streaming;
> >  };
> >
> >  static const s64 link_freq[] = {
> > -     IMX334_LINK_FREQ,
> > +     IMX334_LINK_FREQ_891M,
> > +     IMX334_LINK_FREQ_445M,
> >  };
> >
> >  /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7 +471,7 @@
> > static const struct imx334_mode supported_modes[] = {
> >               .vblank_min = 45,
> >               .vblank_max = 132840,
> >               .pclk = 297000000,
> > -             .link_freq_idx = 0,
> > +             .link_freq_idx = 1,
> >               .reg_list = {
> >                       .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> >                       .regs = mode_1920x1080_regs, @@ -598,6 +601,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, mode->hblank,
> >                                      mode->hblank, 1, mode->hblank);
> >       if (ret)
> > @@ -698,6 +706,8 @@ 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:
> >       case V4L2_CID_HBLANK:
> >               ret = 0;
> >               break;
> > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct imx334
> *imx334)
> >       struct fwnode_handle *ep;
> >       unsigned long rate;
> >       int ret;
> > -     int i;
> > +     int i, j;
> 
> unsigned int would be nicer.
I will change.
> 
> >
> >       if (!fwnode)
> >               return -ENXIO;
> > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct imx334
> *imx334)
> >               goto done_endpoint_free;
> >       }
> >
> > -     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > -             if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> > +     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > +             for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> > +                     if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> > +                             set_bit(j, &imx334->menu_skip_mask);
> 
> Is there a guarantee that you'll only be using the modes with the listed
> frequencies? I don't see one but I might have missed it.

If I understand it correctly, the question here is, the listed freqeunceis and modes are one to one mapped?  Then yes.

Thanks.
shravan
> 
> > +                             break;
> > +                     }
> > +             }
> > +
> > +             if (j == ARRAY_SIZE(link_freq)) {
> > +                     ret = dev_err_probe(imx334->dev, -EINVAL,
> > +                                         "no supported link freq
> > + found\n");
> >                       goto done_endpoint_free;
> > -
> > -     ret = -EINVAL;
> > +             }
> > +     }
> >
> >  done_endpoint_free:
> >       v4l2_fwnode_endpoint_free(&bus_cfg);
> > @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct imx334
> *imx334)
> >       imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> >                                                       &imx334_ctrl_ops,
> >                                                       V4L2_CID_LINK_FREQ,
> > -                                                     ARRAY_SIZE(link_freq) -
> > -                                                     1,
> > -                                                     mode->link_freq_idx,
> > +                                                     __fls(imx334->menu_skip_mask),
> > +
> > + __ffs(imx334->menu_skip_mask),
> >                                                       link_freq);
> > +
> >       if (imx334->link_freq_ctrl)
> >               imx334->link_freq_ctrl->flags |=
> > V4L2_CTRL_FLAG_READ_ONLY;
> >
> 
> --
> Kind regards,
> 
> Sakari Ailus

RE: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[Shravan.Chippa]

Hi Sakari,

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@iki.fi>
> Sent: 24 January 2023 04:33 AM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link
> frequency
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Shravan,
> 
> On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> > From: Shravan Chippa <shravan.chippa@microchip.com>
> >
> > Update pixel_rate and link frequency for 1920x1080@30 while changing
> > mode.
> >
> > Add dummy ctrl cases for pixel_rate and link frequency to avoid error
> > while changing the modes dynamically.
> >
> > Add support to handle multiple link frequencies.
> >
> > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > ---
> >  drivers/media/i2c/imx334.c | 41
> > ++++++++++++++++++++++++++++----------
> >  1 file changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index 309c706114d2..62b104eaa437 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -49,7 +49,8 @@
> >  #define IMX334_INCLK_RATE    24000000
> >
> >  /* CSI2 HW configuration */
> > -#define IMX334_LINK_FREQ     891000000
> > +#define IMX334_LINK_FREQ_891M        891000000
> > +#define IMX334_LINK_FREQ_445M        445500000
> >  #define IMX334_NUM_DATA_LANES        4
> >
> >  #define IMX334_REG_MIN               0x00
> > @@ -139,12 +140,14 @@ struct imx334 {
> >       u32 vblank;
> >       const struct imx334_mode *cur_mode;
> >       struct mutex mutex;
> > +     unsigned long menu_skip_mask;
> >       u32 cur_code;
> >       bool streaming;
> >  };
> >
> >  static const s64 link_freq[] = {
> > -     IMX334_LINK_FREQ,
> > +     IMX334_LINK_FREQ_891M,
> > +     IMX334_LINK_FREQ_445M,
> >  };
> >
> >  /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7 +471,7 @@
> > static const struct imx334_mode supported_modes[] = {
> >               .vblank_min = 45,
> >               .vblank_max = 132840,
> >               .pclk = 297000000,
> > -             .link_freq_idx = 0,
> > +             .link_freq_idx = 1,
> >               .reg_list = {
> >                       .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> >                       .regs = mode_1920x1080_regs, @@ -598,6 +601,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, mode->hblank,
> >                                      mode->hblank, 1, mode->hblank);
> >       if (ret)
> > @@ -698,6 +706,8 @@ 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:
> >       case V4L2_CID_HBLANK:
> >               ret = 0;
> >               break;
> > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct imx334
> *imx334)
> >       struct fwnode_handle *ep;
> >       unsigned long rate;
> >       int ret;
> > -     int i;
> > +     int i, j;
> 
> unsigned int would be nicer.
I will change.
> 
> >
> >       if (!fwnode)
> >               return -ENXIO;
> > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct imx334
> *imx334)
> >               goto done_endpoint_free;
> >       }
> >
> > -     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > -             if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> > +     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > +             for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> > +                     if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> > +                             set_bit(j, &imx334->menu_skip_mask);
> 
> Is there a guarantee that you'll only be using the modes with the listed
> frequencies? I don't see one but I might have missed it.

If I understand it correctly, the question here is, the listed freqeunceis and modes are one to one mapped?  Then yes.

Thanks.
shravan
> 
> > +                             break;
> > +                     }
> > +             }
> > +
> > +             if (j == ARRAY_SIZE(link_freq)) {
> > +                     ret = dev_err_probe(imx334->dev, -EINVAL,
> > +                                         "no supported link freq
> > + found\n");
> >                       goto done_endpoint_free;
> > -
> > -     ret = -EINVAL;
> > +             }
> > +     }
> >
> >  done_endpoint_free:
> >       v4l2_fwnode_endpoint_free(&bus_cfg);
> > @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct imx334
> *imx334)
> >       imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> >                                                       &imx334_ctrl_ops,
> >                                                       V4L2_CID_LINK_FREQ,
> > -                                                     ARRAY_SIZE(link_freq) -
> > -                                                     1,
> > -                                                     mode->link_freq_idx,
> > +                                                     __fls(imx334->menu_skip_mask),
> > +
> > + __ffs(imx334->menu_skip_mask),
> >                                                       link_freq);
> > +
> >       if (imx334->link_freq_ctrl)
> >               imx334->link_freq_ctrl->flags |=
> > V4L2_CTRL_FLAG_READ_ONLY;
> >
> 
> --
> Kind regards,
> 
> Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[Sakari Ailus]

Hi Shravan,

On Tue, Jan 24, 2023 at 05:34:02AM +0000, Shravan.Chippa@microchip.com wrote:
> Hi Sakari,
> 
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> > Sent: 24 January 2023 04:33 AM
> > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org;
> > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org
> > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link
> > frequency
> > 
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> > 
> > Hi Shravan,
> > 
> > On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > >
> > > Update pixel_rate and link frequency for 1920x1080@30 while changing
> > > mode.
> > >
> > > Add dummy ctrl cases for pixel_rate and link frequency to avoid error
> > > while changing the modes dynamically.
> > >
> > > Add support to handle multiple link frequencies.
> > >
> > > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > ---
> > >  drivers/media/i2c/imx334.c | 41
> > > ++++++++++++++++++++++++++++----------
> > >  1 file changed, 30 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > index 309c706114d2..62b104eaa437 100644
> > > --- a/drivers/media/i2c/imx334.c
> > > +++ b/drivers/media/i2c/imx334.c
> > > @@ -49,7 +49,8 @@
> > >  #define IMX334_INCLK_RATE    24000000
> > >
> > >  /* CSI2 HW configuration */
> > > -#define IMX334_LINK_FREQ     891000000
> > > +#define IMX334_LINK_FREQ_891M        891000000
> > > +#define IMX334_LINK_FREQ_445M        445500000
> > >  #define IMX334_NUM_DATA_LANES        4
> > >
> > >  #define IMX334_REG_MIN               0x00
> > > @@ -139,12 +140,14 @@ struct imx334 {
> > >       u32 vblank;
> > >       const struct imx334_mode *cur_mode;
> > >       struct mutex mutex;
> > > +     unsigned long menu_skip_mask;
> > >       u32 cur_code;
> > >       bool streaming;
> > >  };
> > >
> > >  static const s64 link_freq[] = {
> > > -     IMX334_LINK_FREQ,
> > > +     IMX334_LINK_FREQ_891M,
> > > +     IMX334_LINK_FREQ_445M,
> > >  };
> > >
> > >  /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7 +471,7 @@
> > > static const struct imx334_mode supported_modes[] = {
> > >               .vblank_min = 45,
> > >               .vblank_max = 132840,
> > >               .pclk = 297000000,
> > > -             .link_freq_idx = 0,
> > > +             .link_freq_idx = 1,
> > >               .reg_list = {
> > >                       .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > >                       .regs = mode_1920x1080_regs, @@ -598,6 +601,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, mode->hblank,
> > >                                      mode->hblank, 1, mode->hblank);
> > >       if (ret)
> > > @@ -698,6 +706,8 @@ 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:
> > >       case V4L2_CID_HBLANK:
> > >               ret = 0;
> > >               break;
> > > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct imx334
> > *imx334)
> > >       struct fwnode_handle *ep;
> > >       unsigned long rate;
> > >       int ret;
> > > -     int i;
> > > +     int i, j;
> > 
> > unsigned int would be nicer.
> I will change.
> > 
> > >
> > >       if (!fwnode)
> > >               return -ENXIO;
> > > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct imx334
> > *imx334)
> > >               goto done_endpoint_free;
> > >       }
> > >
> > > -     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > > -             if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> > > +     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > > +             for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> > > +                     if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> > > +                             set_bit(j, &imx334->menu_skip_mask);
> > 
> > Is there a guarantee that you'll only be using the modes with the listed
> > frequencies? I don't see one but I might have missed it.
> 
> If I understand it correctly, the question here is, the listed
> freqeunceis and modes are one to one mapped? Then yes.

I don't see this being checked in imx334_set_pad_format(), for instance.

If a frequency isn't in DT, the driver isn't supposed to be using it
either.

> 
> Thanks.
> shravan
> > 
> > > +                             break;
> > > +                     }
> > > +             }
> > > +
> > > +             if (j == ARRAY_SIZE(link_freq)) {
> > > +                     ret = dev_err_probe(imx334->dev, -EINVAL,
> > > +                                         "no supported link freq
> > > + found\n");
> > >                       goto done_endpoint_free;
> > > -
> > > -     ret = -EINVAL;
> > > +             }
> > > +     }
> > >
> > >  done_endpoint_free:
> > >       v4l2_fwnode_endpoint_free(&bus_cfg);
> > > @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct imx334
> > *imx334)
> > >       imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > >                                                       &imx334_ctrl_ops,
> > >                                                       V4L2_CID_LINK_FREQ,
> > > -                                                     ARRAY_SIZE(link_freq) -
> > > -                                                     1,
> > > -                                                     mode->link_freq_idx,
> > > +                                                     __fls(imx334->menu_skip_mask),
> > > +
> > > + __ffs(imx334->menu_skip_mask),
> > >                                                       link_freq);
> > > +
> > >       if (imx334->link_freq_ctrl)
> > >               imx334->link_freq_ctrl->flags |=
> > > V4L2_CTRL_FLAG_READ_ONLY;

-- 
Kind regards,

Sakari Ailus

Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[Sakari Ailus]

Hi Shravan,

On Tue, Jan 24, 2023 at 05:34:02AM +0000, Shravan.Chippa@microchip.com wrote:
> Hi Sakari,
> 
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> > Sent: 24 January 2023 04:33 AM
> > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org;
> > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org
> > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link
> > frequency
> > 
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> > 
> > Hi Shravan,
> > 
> > On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > >
> > > Update pixel_rate and link frequency for 1920x1080@30 while changing
> > > mode.
> > >
> > > Add dummy ctrl cases for pixel_rate and link frequency to avoid error
> > > while changing the modes dynamically.
> > >
> > > Add support to handle multiple link frequencies.
> > >
> > > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > ---
> > >  drivers/media/i2c/imx334.c | 41
> > > ++++++++++++++++++++++++++++----------
> > >  1 file changed, 30 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > index 309c706114d2..62b104eaa437 100644
> > > --- a/drivers/media/i2c/imx334.c
> > > +++ b/drivers/media/i2c/imx334.c
> > > @@ -49,7 +49,8 @@
> > >  #define IMX334_INCLK_RATE    24000000
> > >
> > >  /* CSI2 HW configuration */
> > > -#define IMX334_LINK_FREQ     891000000
> > > +#define IMX334_LINK_FREQ_891M        891000000
> > > +#define IMX334_LINK_FREQ_445M        445500000
> > >  #define IMX334_NUM_DATA_LANES        4
> > >
> > >  #define IMX334_REG_MIN               0x00
> > > @@ -139,12 +140,14 @@ struct imx334 {
> > >       u32 vblank;
> > >       const struct imx334_mode *cur_mode;
> > >       struct mutex mutex;
> > > +     unsigned long menu_skip_mask;
> > >       u32 cur_code;
> > >       bool streaming;
> > >  };
> > >
> > >  static const s64 link_freq[] = {
> > > -     IMX334_LINK_FREQ,
> > > +     IMX334_LINK_FREQ_891M,
> > > +     IMX334_LINK_FREQ_445M,
> > >  };
> > >
> > >  /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7 +471,7 @@
> > > static const struct imx334_mode supported_modes[] = {
> > >               .vblank_min = 45,
> > >               .vblank_max = 132840,
> > >               .pclk = 297000000,
> > > -             .link_freq_idx = 0,
> > > +             .link_freq_idx = 1,
> > >               .reg_list = {
> > >                       .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > >                       .regs = mode_1920x1080_regs, @@ -598,6 +601,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, mode->hblank,
> > >                                      mode->hblank, 1, mode->hblank);
> > >       if (ret)
> > > @@ -698,6 +706,8 @@ 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:
> > >       case V4L2_CID_HBLANK:
> > >               ret = 0;
> > >               break;
> > > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct imx334
> > *imx334)
> > >       struct fwnode_handle *ep;
> > >       unsigned long rate;
> > >       int ret;
> > > -     int i;
> > > +     int i, j;
> > 
> > unsigned int would be nicer.
> I will change.
> > 
> > >
> > >       if (!fwnode)
> > >               return -ENXIO;
> > > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct imx334
> > *imx334)
> > >               goto done_endpoint_free;
> > >       }
> > >
> > > -     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > > -             if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> > > +     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > > +             for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> > > +                     if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> > > +                             set_bit(j, &imx334->menu_skip_mask);
> > 
> > Is there a guarantee that you'll only be using the modes with the listed
> > frequencies? I don't see one but I might have missed it.
> 
> If I understand it correctly, the question here is, the listed
> freqeunceis and modes are one to one mapped? Then yes.

I don't see this being checked in imx334_set_pad_format(), for instance.

If a frequency isn't in DT, the driver isn't supposed to be using it
either.

> 
> Thanks.
> shravan
> > 
> > > +                             break;
> > > +                     }
> > > +             }
> > > +
> > > +             if (j == ARRAY_SIZE(link_freq)) {
> > > +                     ret = dev_err_probe(imx334->dev, -EINVAL,
> > > +                                         "no supported link freq
> > > + found\n");
> > >                       goto done_endpoint_free;
> > > -
> > > -     ret = -EINVAL;
> > > +             }
> > > +     }
> > >
> > >  done_endpoint_free:
> > >       v4l2_fwnode_endpoint_free(&bus_cfg);
> > > @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct imx334
> > *imx334)
> > >       imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > >                                                       &imx334_ctrl_ops,
> > >                                                       V4L2_CID_LINK_FREQ,
> > > -                                                     ARRAY_SIZE(link_freq) -
> > > -                                                     1,
> > > -                                                     mode->link_freq_idx,
> > > +                                                     __fls(imx334->menu_skip_mask),
> > > +
> > > + __ffs(imx334->menu_skip_mask),
> > >                                                       link_freq);
> > > +
> > >       if (imx334->link_freq_ctrl)
> > >               imx334->link_freq_ctrl->flags |=
> > > V4L2_CTRL_FLAG_READ_ONLY;

-- 
Kind regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[Shravan.Chippa]

Hi Sakari,

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@iki.fi>
> Sent: 25 January 2023 03:12 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link
> frequency
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Shravan,
> 
> On Tue, Jan 24, 2023 at 05:34:02AM +0000, Shravan.Chippa@microchip.com
> wrote:
> > Hi Sakari,
> >
> > > -----Original Message-----
> > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > Sent: 24 January 2023 04:33 AM
> > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > kernel@pengutronix.de; linux-imx@nxp.com;
> > > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-arm- kernel@lists.infradead.org
> > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel
> > > and link frequency
> > >
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > >
> > > Hi Shravan,
> > >
> > > On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > >
> > > > Update pixel_rate and link frequency for 1920x1080@30 while
> > > > changing mode.
> > > >
> > > > Add dummy ctrl cases for pixel_rate and link frequency to avoid
> > > > error while changing the modes dynamically.
> > > >
> > > > Add support to handle multiple link frequencies.
> > > >
> > > > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > ---
> > > >  drivers/media/i2c/imx334.c | 41
> > > > ++++++++++++++++++++++++++++----------
> > > >  1 file changed, 30 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx334.c
> > > > b/drivers/media/i2c/imx334.c index 309c706114d2..62b104eaa437
> > > > 100644
> > > > --- a/drivers/media/i2c/imx334.c
> > > > +++ b/drivers/media/i2c/imx334.c
> > > > @@ -49,7 +49,8 @@
> > > >  #define IMX334_INCLK_RATE    24000000
> > > >
> > > >  /* CSI2 HW configuration */
> > > > -#define IMX334_LINK_FREQ     891000000
> > > > +#define IMX334_LINK_FREQ_891M        891000000
> > > > +#define IMX334_LINK_FREQ_445M        445500000
> > > >  #define IMX334_NUM_DATA_LANES        4
> > > >
> > > >  #define IMX334_REG_MIN               0x00
> > > > @@ -139,12 +140,14 @@ struct imx334 {
> > > >       u32 vblank;
> > > >       const struct imx334_mode *cur_mode;
> > > >       struct mutex mutex;
> > > > +     unsigned long menu_skip_mask;
> > > >       u32 cur_code;
> > > >       bool streaming;
> > > >  };
> > > >
> > > >  static const s64 link_freq[] = {
> > > > -     IMX334_LINK_FREQ,
> > > > +     IMX334_LINK_FREQ_891M,
> > > > +     IMX334_LINK_FREQ_445M,
> > > >  };
> > > >
> > > >  /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7 +471,7
> > > > @@ static const struct imx334_mode supported_modes[] = {
> > > >               .vblank_min = 45,
> > > >               .vblank_max = 132840,
> > > >               .pclk = 297000000,
> > > > -             .link_freq_idx = 0,
> > > > +             .link_freq_idx = 1,
> > > >               .reg_list = {
> > > >                       .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > > >                       .regs = mode_1920x1080_regs, @@ -598,6
> > > > +601,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, mode->hblank,
> > > >                                      mode->hblank, 1, mode->hblank);
> > > >       if (ret)
> > > > @@ -698,6 +706,8 @@ 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:
> > > >       case V4L2_CID_HBLANK:
> > > >               ret = 0;
> > > >               break;
> > > > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct
> > > > imx334
> > > *imx334)
> > > >       struct fwnode_handle *ep;
> > > >       unsigned long rate;
> > > >       int ret;
> > > > -     int i;
> > > > +     int i, j;
> > >
> > > unsigned int would be nicer.
> > I will change.
> > >
> > > >
> > > >       if (!fwnode)
> > > >               return -ENXIO;
> > > > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct
> > > > imx334
> > > *imx334)
> > > >               goto done_endpoint_free;
> > > >       }
> > > >
> > > > -     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > > > -             if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> > > > +     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > > > +             for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> > > > +                     if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> > > > +                             set_bit(j, &imx334->menu_skip_mask);
> > >
> > > Is there a guarantee that you'll only be using the modes with the
> > > listed frequencies? I don't see one but I might have missed it.
> >
> > If I understand it correctly, the question here is, the listed
> > freqeunceis and modes are one to one mapped? Then yes.
> 
> I don't see this being checked in imx334_set_pad_format(), for instance.
> 
> If a frequency isn't in DT, the driver isn't supposed to be using it either.

Yes, there is no check.

But, if a frequency is not in DT, the driver will not add in menu items.
So, the function imx334_set_pad_format() -> imx334_update_controls() fails, if we set the frequencies which are not there in the DT or menu items.


Thanks,
Shravan

> 
> >
> > Thanks.
> > shravan
> > >
> > > > +                             break;
> > > > +                     }
> > > > +             }
> > > > +
> > > > +             if (j == ARRAY_SIZE(link_freq)) {
> > > > +                     ret = dev_err_probe(imx334->dev, -EINVAL,
> > > > +                                         "no supported link freq
> > > > + found\n");
> > > >                       goto done_endpoint_free;
> > > > -
> > > > -     ret = -EINVAL;
> > > > +             }
> > > > +     }
> > > >
> > > >  done_endpoint_free:
> > > >       v4l2_fwnode_endpoint_free(&bus_cfg);
> > > > @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct
> > > > imx334
> > > *imx334)
> > > >       imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > > >                                                       &imx334_ctrl_ops,
> > > >                                                       V4L2_CID_LINK_FREQ,
> > > > -                                                     ARRAY_SIZE(link_freq) -
> > > > -                                                     1,
> > > > -                                                     mode->link_freq_idx,
> > > > +
> > > > + __fls(imx334->menu_skip_mask),
> > > > +
> > > > + __ffs(imx334->menu_skip_mask),
> > > >                                                       link_freq);
> > > > +
> > > >       if (imx334->link_freq_ctrl)
> > > >               imx334->link_freq_ctrl->flags |=
> > > > V4L2_CTRL_FLAG_READ_ONLY;
> 
> --
> Kind regards,
> 
> Sakari Ailus

RE: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[Shravan.Chippa]

Hi Sakari,

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@iki.fi>
> Sent: 25 January 2023 03:12 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link
> frequency
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Shravan,
> 
> On Tue, Jan 24, 2023 at 05:34:02AM +0000, Shravan.Chippa@microchip.com
> wrote:
> > Hi Sakari,
> >
> > > -----Original Message-----
> > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > Sent: 24 January 2023 04:33 AM
> > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > kernel@pengutronix.de; linux-imx@nxp.com;
> > > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-arm- kernel@lists.infradead.org
> > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel
> > > and link frequency
> > >
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > >
> > > Hi Shravan,
> > >
> > > On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > >
> > > > Update pixel_rate and link frequency for 1920x1080@30 while
> > > > changing mode.
> > > >
> > > > Add dummy ctrl cases for pixel_rate and link frequency to avoid
> > > > error while changing the modes dynamically.
> > > >
> > > > Add support to handle multiple link frequencies.
> > > >
> > > > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > ---
> > > >  drivers/media/i2c/imx334.c | 41
> > > > ++++++++++++++++++++++++++++----------
> > > >  1 file changed, 30 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx334.c
> > > > b/drivers/media/i2c/imx334.c index 309c706114d2..62b104eaa437
> > > > 100644
> > > > --- a/drivers/media/i2c/imx334.c
> > > > +++ b/drivers/media/i2c/imx334.c
> > > > @@ -49,7 +49,8 @@
> > > >  #define IMX334_INCLK_RATE    24000000
> > > >
> > > >  /* CSI2 HW configuration */
> > > > -#define IMX334_LINK_FREQ     891000000
> > > > +#define IMX334_LINK_FREQ_891M        891000000
> > > > +#define IMX334_LINK_FREQ_445M        445500000
> > > >  #define IMX334_NUM_DATA_LANES        4
> > > >
> > > >  #define IMX334_REG_MIN               0x00
> > > > @@ -139,12 +140,14 @@ struct imx334 {
> > > >       u32 vblank;
> > > >       const struct imx334_mode *cur_mode;
> > > >       struct mutex mutex;
> > > > +     unsigned long menu_skip_mask;
> > > >       u32 cur_code;
> > > >       bool streaming;
> > > >  };
> > > >
> > > >  static const s64 link_freq[] = {
> > > > -     IMX334_LINK_FREQ,
> > > > +     IMX334_LINK_FREQ_891M,
> > > > +     IMX334_LINK_FREQ_445M,
> > > >  };
> > > >
> > > >  /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7 +471,7
> > > > @@ static const struct imx334_mode supported_modes[] = {
> > > >               .vblank_min = 45,
> > > >               .vblank_max = 132840,
> > > >               .pclk = 297000000,
> > > > -             .link_freq_idx = 0,
> > > > +             .link_freq_idx = 1,
> > > >               .reg_list = {
> > > >                       .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > > >                       .regs = mode_1920x1080_regs, @@ -598,6
> > > > +601,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, mode->hblank,
> > > >                                      mode->hblank, 1, mode->hblank);
> > > >       if (ret)
> > > > @@ -698,6 +706,8 @@ 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:
> > > >       case V4L2_CID_HBLANK:
> > > >               ret = 0;
> > > >               break;
> > > > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct
> > > > imx334
> > > *imx334)
> > > >       struct fwnode_handle *ep;
> > > >       unsigned long rate;
> > > >       int ret;
> > > > -     int i;
> > > > +     int i, j;
> > >
> > > unsigned int would be nicer.
> > I will change.
> > >
> > > >
> > > >       if (!fwnode)
> > > >               return -ENXIO;
> > > > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct
> > > > imx334
> > > *imx334)
> > > >               goto done_endpoint_free;
> > > >       }
> > > >
> > > > -     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > > > -             if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> > > > +     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > > > +             for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> > > > +                     if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> > > > +                             set_bit(j, &imx334->menu_skip_mask);
> > >
> > > Is there a guarantee that you'll only be using the modes with the
> > > listed frequencies? I don't see one but I might have missed it.
> >
> > If I understand it correctly, the question here is, the listed
> > freqeunceis and modes are one to one mapped? Then yes.
> 
> I don't see this being checked in imx334_set_pad_format(), for instance.
> 
> If a frequency isn't in DT, the driver isn't supposed to be using it either.

Yes, there is no check.

But, if a frequency is not in DT, the driver will not add in menu items.
So, the function imx334_set_pad_format() -> imx334_update_controls() fails, if we set the frequencies which are not there in the DT or menu items.


Thanks,
Shravan

> 
> >
> > Thanks.
> > shravan
> > >
> > > > +                             break;
> > > > +                     }
> > > > +             }
> > > > +
> > > > +             if (j == ARRAY_SIZE(link_freq)) {
> > > > +                     ret = dev_err_probe(imx334->dev, -EINVAL,
> > > > +                                         "no supported link freq
> > > > + found\n");
> > > >                       goto done_endpoint_free;
> > > > -
> > > > -     ret = -EINVAL;
> > > > +             }
> > > > +     }
> > > >
> > > >  done_endpoint_free:
> > > >       v4l2_fwnode_endpoint_free(&bus_cfg);
> > > > @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct
> > > > imx334
> > > *imx334)
> > > >       imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > > >                                                       &imx334_ctrl_ops,
> > > >                                                       V4L2_CID_LINK_FREQ,
> > > > -                                                     ARRAY_SIZE(link_freq) -
> > > > -                                                     1,
> > > > -                                                     mode->link_freq_idx,
> > > > +
> > > > + __fls(imx334->menu_skip_mask),
> > > > +
> > > > + __ffs(imx334->menu_skip_mask),
> > > >                                                       link_freq);
> > > > +
> > > >       if (imx334->link_freq_ctrl)
> > > >               imx334->link_freq_ctrl->flags |=
> > > > V4L2_CTRL_FLAG_READ_ONLY;
> 
> --
> Kind regards,
> 
> Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[Shravan.Chippa]

Hi Sakari,


> -----Original Message-----
> From: shravan Chippa - I35088
> Sent: 27 January 2023 05:40 AM
> To: Sakari Ailus <sakari.ailus@iki.fi>
> Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: RE: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and
> link frequency
> 
> Hi Sakari,
> 
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> > Sent: 25 January 2023 03:12 PM
> > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > kernel@pengutronix.de; linux-imx@nxp.com; linux-
> media@vger.kernel.org;
> > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org
> > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel
> > and link frequency
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > the content is safe
> >
> > Hi Shravan,
> >
> > On Tue, Jan 24, 2023 at 05:34:02AM +0000, Shravan.Chippa@microchip.com
> > wrote:
> > > Hi Sakari,
> > >
> > > > -----Original Message-----
> > > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > > Sent: 24 January 2023 04:33 AM
> > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > > shawnguo@kernel.org; s.hauer@pengutronix.de;
> festevam@gmail.com;
> > > > kernel@pengutronix.de; linux-imx@nxp.com;
> > > > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > devicetree@vger.kernel.org; linux-arm- kernel@lists.infradead.org
> > > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update
> > > > pixel and link frequency
> > > >
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > > know the content is safe
> > > >
> > > > Hi Shravan,
> > > >
> > > > On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > >
> > > > > Update pixel_rate and link frequency for 1920x1080@30 while
> > > > > changing mode.
> > > > >
> > > > > Add dummy ctrl cases for pixel_rate and link frequency to avoid
> > > > > error while changing the modes dynamically.
> > > > >
> > > > > Add support to handle multiple link frequencies.
> > > > >
> > > > > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > ---
> > > > >  drivers/media/i2c/imx334.c | 41
> > > > > ++++++++++++++++++++++++++++----------
> > > > >  1 file changed, 30 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx334.c
> > > > > b/drivers/media/i2c/imx334.c index 309c706114d2..62b104eaa437
> > > > > 100644
> > > > > --- a/drivers/media/i2c/imx334.c
> > > > > +++ b/drivers/media/i2c/imx334.c
> > > > > @@ -49,7 +49,8 @@
> > > > >  #define IMX334_INCLK_RATE    24000000
> > > > >
> > > > >  /* CSI2 HW configuration */
> > > > > -#define IMX334_LINK_FREQ     891000000
> > > > > +#define IMX334_LINK_FREQ_891M        891000000
> > > > > +#define IMX334_LINK_FREQ_445M        445500000
> > > > >  #define IMX334_NUM_DATA_LANES        4
> > > > >
> > > > >  #define IMX334_REG_MIN               0x00
> > > > > @@ -139,12 +140,14 @@ struct imx334 {
> > > > >       u32 vblank;
> > > > >       const struct imx334_mode *cur_mode;
> > > > >       struct mutex mutex;
> > > > > +     unsigned long menu_skip_mask;
> > > > >       u32 cur_code;
> > > > >       bool streaming;
> > > > >  };
> > > > >
> > > > >  static const s64 link_freq[] = {
> > > > > -     IMX334_LINK_FREQ,
> > > > > +     IMX334_LINK_FREQ_891M,
> > > > > +     IMX334_LINK_FREQ_445M,
> > > > >  };
> > > > >
> > > > >  /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7
> > > > > +471,7 @@ static const struct imx334_mode supported_modes[] = {
> > > > >               .vblank_min = 45,
> > > > >               .vblank_max = 132840,
> > > > >               .pclk = 297000000,
> > > > > -             .link_freq_idx = 0,
> > > > > +             .link_freq_idx = 1,
> > > > >               .reg_list = {
> > > > >                       .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > > > >                       .regs = mode_1920x1080_regs, @@ -598,6
> > > > > +601,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, mode-
> >hblank,
> > > > >                                      mode->hblank, 1, mode->hblank);
> > > > >       if (ret)
> > > > > @@ -698,6 +706,8 @@ 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:
> > > > >       case V4L2_CID_HBLANK:
> > > > >               ret = 0;
> > > > >               break;
> > > > > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct
> > > > > imx334
> > > > *imx334)
> > > > >       struct fwnode_handle *ep;
> > > > >       unsigned long rate;
> > > > >       int ret;
> > > > > -     int i;
> > > > > +     int i, j;
> > > >
> > > > unsigned int would be nicer.
> > > I will change.
> > > >
> > > > >
> > > > >       if (!fwnode)
> > > > >               return -ENXIO;
> > > > > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct
> > > > > imx334
> > > > *imx334)
> > > > >               goto done_endpoint_free;
> > > > >       }
> > > > >
> > > > > -     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > > > > -             if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> > > > > +     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > > > > +             for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> > > > > +                     if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> > > > > +                             set_bit(j,
> > > > > + &imx334->menu_skip_mask);
> > > >
> > > > Is there a guarantee that you'll only be using the modes with the
> > > > listed frequencies? I don't see one but I might have missed it.
> > >
> > > If I understand it correctly, the question here is, the listed
> > > freqeunceis and modes are one to one mapped? Then yes.
> >
> > I don't see this being checked in imx334_set_pad_format(), for instance.
> >
> > If a frequency isn't in DT, the driver isn't supposed to be using it either.
> 
> Yes, there is no check.
> 
> But, if a frequency is not in DT, the driver will not add in menu items.
> So, the function imx334_set_pad_format() -> imx334_update_controls()
> fails, if we set the frequencies which are not there in the DT or menu items.
> 

Are you ok with the above explanation or any changes you are expecting?
Please do let me know if there are any changes needed.
I am planning to send the next version.

Thanks,
Shravan

> 
> Thanks,
> Shravan
> 
> >
> > >
> > > Thanks.
> > > shravan
> > > >
> > > > > +                             break;
> > > > > +                     }
> > > > > +             }
> > > > > +
> > > > > +             if (j == ARRAY_SIZE(link_freq)) {
> > > > > +                     ret = dev_err_probe(imx334->dev, -EINVAL,
> > > > > +                                         "no supported link
> > > > > + freq found\n");
> > > > >                       goto done_endpoint_free;
> > > > > -
> > > > > -     ret = -EINVAL;
> > > > > +             }
> > > > > +     }
> > > > >
> > > > >  done_endpoint_free:
> > > > >       v4l2_fwnode_endpoint_free(&bus_cfg);
> > > > > @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct
> > > > > imx334
> > > > *imx334)
> > > > >       imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > > > >                                                       &imx334_ctrl_ops,
> > > > >                                                       V4L2_CID_LINK_FREQ,
> > > > > -                                                     ARRAY_SIZE(link_freq) -
> > > > > -                                                     1,
> > > > > -                                                     mode->link_freq_idx,
> > > > > +
> > > > > + __fls(imx334->menu_skip_mask),
> > > > > +
> > > > > + __ffs(imx334->menu_skip_mask),
> > > > >
> > > > > link_freq);
> > > > > +
> > > > >       if (imx334->link_freq_ctrl)
> > > > >               imx334->link_freq_ctrl->flags |=
> > > > > V4L2_CTRL_FLAG_READ_ONLY;
> >
> > --
> > Kind regards,
> >
> > Sakari Ailus

RE: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[Shravan.Chippa]

Hi Sakari,


> -----Original Message-----
> From: shravan Chippa - I35088
> Sent: 27 January 2023 05:40 AM
> To: Sakari Ailus <sakari.ailus@iki.fi>
> Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: RE: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and
> link frequency
> 
> Hi Sakari,
> 
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@iki.fi>
> > Sent: 25 January 2023 03:12 PM
> > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > kernel@pengutronix.de; linux-imx@nxp.com; linux-
> media@vger.kernel.org;
> > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org
> > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel
> > and link frequency
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > the content is safe
> >
> > Hi Shravan,
> >
> > On Tue, Jan 24, 2023 at 05:34:02AM +0000, Shravan.Chippa@microchip.com
> > wrote:
> > > Hi Sakari,
> > >
> > > > -----Original Message-----
> > > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > > Sent: 24 January 2023 04:33 AM
> > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > > shawnguo@kernel.org; s.hauer@pengutronix.de;
> festevam@gmail.com;
> > > > kernel@pengutronix.de; linux-imx@nxp.com;
> > > > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > devicetree@vger.kernel.org; linux-arm- kernel@lists.infradead.org
> > > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update
> > > > pixel and link frequency
> > > >
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > > know the content is safe
> > > >
> > > > Hi Shravan,
> > > >
> > > > On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > >
> > > > > Update pixel_rate and link frequency for 1920x1080@30 while
> > > > > changing mode.
> > > > >
> > > > > Add dummy ctrl cases for pixel_rate and link frequency to avoid
> > > > > error while changing the modes dynamically.
> > > > >
> > > > > Add support to handle multiple link frequencies.
> > > > >
> > > > > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > ---
> > > > >  drivers/media/i2c/imx334.c | 41
> > > > > ++++++++++++++++++++++++++++----------
> > > > >  1 file changed, 30 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx334.c
> > > > > b/drivers/media/i2c/imx334.c index 309c706114d2..62b104eaa437
> > > > > 100644
> > > > > --- a/drivers/media/i2c/imx334.c
> > > > > +++ b/drivers/media/i2c/imx334.c
> > > > > @@ -49,7 +49,8 @@
> > > > >  #define IMX334_INCLK_RATE    24000000
> > > > >
> > > > >  /* CSI2 HW configuration */
> > > > > -#define IMX334_LINK_FREQ     891000000
> > > > > +#define IMX334_LINK_FREQ_891M        891000000
> > > > > +#define IMX334_LINK_FREQ_445M        445500000
> > > > >  #define IMX334_NUM_DATA_LANES        4
> > > > >
> > > > >  #define IMX334_REG_MIN               0x00
> > > > > @@ -139,12 +140,14 @@ struct imx334 {
> > > > >       u32 vblank;
> > > > >       const struct imx334_mode *cur_mode;
> > > > >       struct mutex mutex;
> > > > > +     unsigned long menu_skip_mask;
> > > > >       u32 cur_code;
> > > > >       bool streaming;
> > > > >  };
> > > > >
> > > > >  static const s64 link_freq[] = {
> > > > > -     IMX334_LINK_FREQ,
> > > > > +     IMX334_LINK_FREQ_891M,
> > > > > +     IMX334_LINK_FREQ_445M,
> > > > >  };
> > > > >
> > > > >  /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7
> > > > > +471,7 @@ static const struct imx334_mode supported_modes[] = {
> > > > >               .vblank_min = 45,
> > > > >               .vblank_max = 132840,
> > > > >               .pclk = 297000000,
> > > > > -             .link_freq_idx = 0,
> > > > > +             .link_freq_idx = 1,
> > > > >               .reg_list = {
> > > > >                       .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > > > >                       .regs = mode_1920x1080_regs, @@ -598,6
> > > > > +601,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, mode-
> >hblank,
> > > > >                                      mode->hblank, 1, mode->hblank);
> > > > >       if (ret)
> > > > > @@ -698,6 +706,8 @@ 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:
> > > > >       case V4L2_CID_HBLANK:
> > > > >               ret = 0;
> > > > >               break;
> > > > > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct
> > > > > imx334
> > > > *imx334)
> > > > >       struct fwnode_handle *ep;
> > > > >       unsigned long rate;
> > > > >       int ret;
> > > > > -     int i;
> > > > > +     int i, j;
> > > >
> > > > unsigned int would be nicer.
> > > I will change.
> > > >
> > > > >
> > > > >       if (!fwnode)
> > > > >               return -ENXIO;
> > > > > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct
> > > > > imx334
> > > > *imx334)
> > > > >               goto done_endpoint_free;
> > > > >       }
> > > > >
> > > > > -     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > > > > -             if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> > > > > +     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > > > > +             for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> > > > > +                     if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> > > > > +                             set_bit(j,
> > > > > + &imx334->menu_skip_mask);
> > > >
> > > > Is there a guarantee that you'll only be using the modes with the
> > > > listed frequencies? I don't see one but I might have missed it.
> > >
> > > If I understand it correctly, the question here is, the listed
> > > freqeunceis and modes are one to one mapped? Then yes.
> >
> > I don't see this being checked in imx334_set_pad_format(), for instance.
> >
> > If a frequency isn't in DT, the driver isn't supposed to be using it either.
> 
> Yes, there is no check.
> 
> But, if a frequency is not in DT, the driver will not add in menu items.
> So, the function imx334_set_pad_format() -> imx334_update_controls()
> fails, if we set the frequencies which are not there in the DT or menu items.
> 

Are you ok with the above explanation or any changes you are expecting?
Please do let me know if there are any changes needed.
I am planning to send the next version.

Thanks,
Shravan

> 
> Thanks,
> Shravan
> 
> >
> > >
> > > Thanks.
> > > shravan
> > > >
> > > > > +                             break;
> > > > > +                     }
> > > > > +             }
> > > > > +
> > > > > +             if (j == ARRAY_SIZE(link_freq)) {
> > > > > +                     ret = dev_err_probe(imx334->dev, -EINVAL,
> > > > > +                                         "no supported link
> > > > > + freq found\n");
> > > > >                       goto done_endpoint_free;
> > > > > -
> > > > > -     ret = -EINVAL;
> > > > > +             }
> > > > > +     }
> > > > >
> > > > >  done_endpoint_free:
> > > > >       v4l2_fwnode_endpoint_free(&bus_cfg);
> > > > > @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct
> > > > > imx334
> > > > *imx334)
> > > > >       imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > > > >                                                       &imx334_ctrl_ops,
> > > > >                                                       V4L2_CID_LINK_FREQ,
> > > > > -                                                     ARRAY_SIZE(link_freq) -
> > > > > -                                                     1,
> > > > > -                                                     mode->link_freq_idx,
> > > > > +
> > > > > + __fls(imx334->menu_skip_mask),
> > > > > +
> > > > > + __ffs(imx334->menu_skip_mask),
> > > > >
> > > > > link_freq);
> > > > > +
> > > > >       if (imx334->link_freq_ctrl)
> > > > >               imx334->link_freq_ctrl->flags |=
> > > > > V4L2_CTRL_FLAG_READ_ONLY;
> >
> > --
> > Kind regards,
> >
> > Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[Sakari Ailus]

Hi Shravan,

On Mon, Feb 06, 2023 at 04:43:42AM +0000, Shravan.Chippa@microchip.com wrote:
> Hi Sakari,
> 
> 
> > -----Original Message-----
> > From: shravan Chippa - I35088
> > Sent: 27 January 2023 05:40 AM
> > To: Sakari Ailus <sakari.ailus@iki.fi>
> > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org;
> > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org
> > Subject: RE: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and
> > link frequency
> > 
> > Hi Sakari,
> > 
> > > -----Original Message-----
> > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > Sent: 25 January 2023 03:12 PM
> > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > kernel@pengutronix.de; linux-imx@nxp.com; linux-
> > media@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org
> > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel
> > > and link frequency
> > >
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > > the content is safe
> > >
> > > Hi Shravan,
> > >
> > > On Tue, Jan 24, 2023 at 05:34:02AM +0000, Shravan.Chippa@microchip.com
> > > wrote:
> > > > Hi Sakari,
> > > >
> > > > > -----Original Message-----
> > > > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > Sent: 24 January 2023 04:33 AM
> > > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > > > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > > > shawnguo@kernel.org; s.hauer@pengutronix.de;
> > festevam@gmail.com;
> > > > > kernel@pengutronix.de; linux-imx@nxp.com;
> > > > > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > > devicetree@vger.kernel.org; linux-arm- kernel@lists.infradead.org
> > > > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update
> > > > > pixel and link frequency
> > > > >
> > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > > > know the content is safe
> > > > >
> > > > > Hi Shravan,
> > > > >
> > > > > On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> > > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > >
> > > > > > Update pixel_rate and link frequency for 1920x1080@30 while
> > > > > > changing mode.
> > > > > >
> > > > > > Add dummy ctrl cases for pixel_rate and link frequency to avoid
> > > > > > error while changing the modes dynamically.
> > > > > >
> > > > > > Add support to handle multiple link frequencies.
> > > > > >
> > > > > > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > > ---
> > > > > >  drivers/media/i2c/imx334.c | 41
> > > > > > ++++++++++++++++++++++++++++----------
> > > > > >  1 file changed, 30 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/imx334.c
> > > > > > b/drivers/media/i2c/imx334.c index 309c706114d2..62b104eaa437
> > > > > > 100644
> > > > > > --- a/drivers/media/i2c/imx334.c
> > > > > > +++ b/drivers/media/i2c/imx334.c
> > > > > > @@ -49,7 +49,8 @@
> > > > > >  #define IMX334_INCLK_RATE    24000000
> > > > > >
> > > > > >  /* CSI2 HW configuration */
> > > > > > -#define IMX334_LINK_FREQ     891000000
> > > > > > +#define IMX334_LINK_FREQ_891M        891000000
> > > > > > +#define IMX334_LINK_FREQ_445M        445500000
> > > > > >  #define IMX334_NUM_DATA_LANES        4
> > > > > >
> > > > > >  #define IMX334_REG_MIN               0x00
> > > > > > @@ -139,12 +140,14 @@ struct imx334 {
> > > > > >       u32 vblank;
> > > > > >       const struct imx334_mode *cur_mode;
> > > > > >       struct mutex mutex;
> > > > > > +     unsigned long menu_skip_mask;
> > > > > >       u32 cur_code;
> > > > > >       bool streaming;
> > > > > >  };
> > > > > >
> > > > > >  static const s64 link_freq[] = {
> > > > > > -     IMX334_LINK_FREQ,
> > > > > > +     IMX334_LINK_FREQ_891M,
> > > > > > +     IMX334_LINK_FREQ_445M,
> > > > > >  };
> > > > > >
> > > > > >  /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7
> > > > > > +471,7 @@ static const struct imx334_mode supported_modes[] = {
> > > > > >               .vblank_min = 45,
> > > > > >               .vblank_max = 132840,
> > > > > >               .pclk = 297000000,
> > > > > > -             .link_freq_idx = 0,
> > > > > > +             .link_freq_idx = 1,
> > > > > >               .reg_list = {
> > > > > >                       .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > > > > >                       .regs = mode_1920x1080_regs, @@ -598,6
> > > > > > +601,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, mode-
> > >hblank,
> > > > > >                                      mode->hblank, 1, mode->hblank);
> > > > > >       if (ret)
> > > > > > @@ -698,6 +706,8 @@ 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:
> > > > > >       case V4L2_CID_HBLANK:
> > > > > >               ret = 0;
> > > > > >               break;
> > > > > > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct
> > > > > > imx334
> > > > > *imx334)
> > > > > >       struct fwnode_handle *ep;
> > > > > >       unsigned long rate;
> > > > > >       int ret;
> > > > > > -     int i;
> > > > > > +     int i, j;
> > > > >
> > > > > unsigned int would be nicer.
> > > > I will change.
> > > > >
> > > > > >
> > > > > >       if (!fwnode)
> > > > > >               return -ENXIO;
> > > > > > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct
> > > > > > imx334
> > > > > *imx334)
> > > > > >               goto done_endpoint_free;
> > > > > >       }
> > > > > >
> > > > > > -     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > > > > > -             if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> > > > > > +     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > > > > > +             for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> > > > > > +                     if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> > > > > > +                             set_bit(j,
> > > > > > + &imx334->menu_skip_mask);
> > > > >
> > > > > Is there a guarantee that you'll only be using the modes with the
> > > > > listed frequencies? I don't see one but I might have missed it.
> > > >
> > > > If I understand it correctly, the question here is, the listed
> > > > freqeunceis and modes are one to one mapped? Then yes.
> > >
> > > I don't see this being checked in imx334_set_pad_format(), for instance.
> > >
> > > If a frequency isn't in DT, the driver isn't supposed to be using it either.
> > 
> > Yes, there is no check.
> > 
> > But, if a frequency is not in DT, the driver will not add in menu items.
> > So, the function imx334_set_pad_format() -> imx334_update_controls()
> > fails, if we set the frequencies which are not there in the DT or menu items.
> > 
> 
> Are you ok with the above explanation or any changes you are expecting?
> Please do let me know if there are any changes needed.
> I am planning to send the next version.

There doesn't seem to be anything that would prevent selecting a format
with a wrong link frequency in these patches. Could you address that, or
point me to where this is done? The control can't be changed by the user,
but that's not enough.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link frequency

[Sakari Ailus]

Hi Shravan,

On Mon, Feb 06, 2023 at 04:43:42AM +0000, Shravan.Chippa@microchip.com wrote:
> Hi Sakari,
> 
> 
> > -----Original Message-----
> > From: shravan Chippa - I35088
> > Sent: 27 January 2023 05:40 AM
> > To: Sakari Ailus <sakari.ailus@iki.fi>
> > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org;
> > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org
> > Subject: RE: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and
> > link frequency
> > 
> > Hi Sakari,
> > 
> > > -----Original Message-----
> > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > Sent: 25 January 2023 03:12 PM
> > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > kernel@pengutronix.de; linux-imx@nxp.com; linux-
> > media@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org
> > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel
> > > and link frequency
> > >
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > > the content is safe
> > >
> > > Hi Shravan,
> > >
> > > On Tue, Jan 24, 2023 at 05:34:02AM +0000, Shravan.Chippa@microchip.com
> > > wrote:
> > > > Hi Sakari,
> > > >
> > > > > -----Original Message-----
> > > > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > Sent: 24 January 2023 04:33 AM
> > > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > > > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > > > shawnguo@kernel.org; s.hauer@pengutronix.de;
> > festevam@gmail.com;
> > > > > kernel@pengutronix.de; linux-imx@nxp.com;
> > > > > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > > devicetree@vger.kernel.org; linux-arm- kernel@lists.infradead.org
> > > > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update
> > > > > pixel and link frequency
> > > > >
> > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > > > know the content is safe
> > > > >
> > > > > Hi Shravan,
> > > > >
> > > > > On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> > > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > >
> > > > > > Update pixel_rate and link frequency for 1920x1080@30 while
> > > > > > changing mode.
> > > > > >
> > > > > > Add dummy ctrl cases for pixel_rate and link frequency to avoid
> > > > > > error while changing the modes dynamically.
> > > > > >
> > > > > > Add support to handle multiple link frequencies.
> > > > > >
> > > > > > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > > ---
> > > > > >  drivers/media/i2c/imx334.c | 41
> > > > > > ++++++++++++++++++++++++++++----------
> > > > > >  1 file changed, 30 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/imx334.c
> > > > > > b/drivers/media/i2c/imx334.c index 309c706114d2..62b104eaa437
> > > > > > 100644
> > > > > > --- a/drivers/media/i2c/imx334.c
> > > > > > +++ b/drivers/media/i2c/imx334.c
> > > > > > @@ -49,7 +49,8 @@
> > > > > >  #define IMX334_INCLK_RATE    24000000
> > > > > >
> > > > > >  /* CSI2 HW configuration */
> > > > > > -#define IMX334_LINK_FREQ     891000000
> > > > > > +#define IMX334_LINK_FREQ_891M        891000000
> > > > > > +#define IMX334_LINK_FREQ_445M        445500000
> > > > > >  #define IMX334_NUM_DATA_LANES        4
> > > > > >
> > > > > >  #define IMX334_REG_MIN               0x00
> > > > > > @@ -139,12 +140,14 @@ struct imx334 {
> > > > > >       u32 vblank;
> > > > > >       const struct imx334_mode *cur_mode;
> > > > > >       struct mutex mutex;
> > > > > > +     unsigned long menu_skip_mask;
> > > > > >       u32 cur_code;
> > > > > >       bool streaming;
> > > > > >  };
> > > > > >
> > > > > >  static const s64 link_freq[] = {
> > > > > > -     IMX334_LINK_FREQ,
> > > > > > +     IMX334_LINK_FREQ_891M,
> > > > > > +     IMX334_LINK_FREQ_445M,
> > > > > >  };
> > > > > >
> > > > > >  /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7
> > > > > > +471,7 @@ static const struct imx334_mode supported_modes[] = {
> > > > > >               .vblank_min = 45,
> > > > > >               .vblank_max = 132840,
> > > > > >               .pclk = 297000000,
> > > > > > -             .link_freq_idx = 0,
> > > > > > +             .link_freq_idx = 1,
> > > > > >               .reg_list = {
> > > > > >                       .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > > > > >                       .regs = mode_1920x1080_regs, @@ -598,6
> > > > > > +601,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, mode-
> > >hblank,
> > > > > >                                      mode->hblank, 1, mode->hblank);
> > > > > >       if (ret)
> > > > > > @@ -698,6 +706,8 @@ 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:
> > > > > >       case V4L2_CID_HBLANK:
> > > > > >               ret = 0;
> > > > > >               break;
> > > > > > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct
> > > > > > imx334
> > > > > *imx334)
> > > > > >       struct fwnode_handle *ep;
> > > > > >       unsigned long rate;
> > > > > >       int ret;
> > > > > > -     int i;
> > > > > > +     int i, j;
> > > > >
> > > > > unsigned int would be nicer.
> > > > I will change.
> > > > >
> > > > > >
> > > > > >       if (!fwnode)
> > > > > >               return -ENXIO;
> > > > > > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct
> > > > > > imx334
> > > > > *imx334)
> > > > > >               goto done_endpoint_free;
> > > > > >       }
> > > > > >
> > > > > > -     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > > > > > -             if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> > > > > > +     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > > > > > +             for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> > > > > > +                     if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> > > > > > +                             set_bit(j,
> > > > > > + &imx334->menu_skip_mask);
> > > > >
> > > > > Is there a guarantee that you'll only be using the modes with the
> > > > > listed frequencies? I don't see one but I might have missed it.
> > > >
> > > > If I understand it correctly, the question here is, the listed
> > > > freqeunceis and modes are one to one mapped? Then yes.
> > >
> > > I don't see this being checked in imx334_set_pad_format(), for instance.
> > >
> > > If a frequency isn't in DT, the driver isn't supposed to be using it either.
> > 
> > Yes, there is no check.
> > 
> > But, if a frequency is not in DT, the driver will not add in menu items.
> > So, the function imx334_set_pad_format() -> imx334_update_controls()
> > fails, if we set the frequencies which are not there in the DT or menu items.
> > 
> 
> Are you ok with the above explanation or any changes you are expecting?
> Please do let me know if there are any changes needed.
> I am planning to send the next version.

There doesn't seem to be anything that would prevent selecting a format
with a wrong link frequency in these patches. Could you address that, or
point me to where this is done? The control can't be changed by the user,
but that's not enough.

-- 
Kind regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel