* Re: [PATCH] media: i2c: imx334: support lower bandwidth mode
@ 2022-08-01 4:34 kernel test robot
0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2022-08-01 4:34 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 12861 bytes --]
CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220728063044.19276-1-shravan.chippa@microchip.com>
References: <20220728063044.19276-1-shravan.chippa@microchip.com>
TO: shravan kumar <shravan.chippa@microchip.com>
TO: paul.j.murphy(a)intel.com
TO: daniele.alessandrelli(a)intel.com
TO: mchehab(a)kernel.org
CC: linux-media(a)vger.kernel.org
CC: linux-kernel(a)vger.kernel.org
CC: Shravan Chippa <shravan.chippa@microchip.com>
CC: Conor Dooley <conor.dooley@microchip.com>
CC: Prakash Battu <Prakash.Battu@microchip.com>
Hi shravan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on media-tree/master]
[also build test WARNING on linus/master v5.19 next-20220728]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/shravan-kumar/media-i2c-imx334-support-lower-bandwidth-mode/20220728-143143
base: git://linuxtv.org/media_tree.git master
:::::: branch date: 4 days ago
:::::: commit date: 4 days ago
config: arc-randconfig-m041-20220731 (https://download.01.org/0day-ci/archive/20220801/202208011237.RWmAl1tP-lkp(a)intel.com/config)
compiler: arceb-elf-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/media/i2c/imx334.c:735 imx334_enum_frame_size() warn: ignoring unreachable code.
drivers/media/i2c/imx334.c:832 imx334_set_pad_format() warn: unsigned 'code' is never less than zero.
vim +735 drivers/media/i2c/imx334.c
9746b11715c3949 Martina Krasteva 2021-02-03 717
9746b11715c3949 Martina Krasteva 2021-02-03 718 /**
9746b11715c3949 Martina Krasteva 2021-02-03 719 * imx334_enum_frame_size() - Enumerate V4L2 sub-device frame sizes
9746b11715c3949 Martina Krasteva 2021-02-03 720 * @sd: pointer to imx334 V4L2 sub-device structure
0d346d2a6f54f06 Tomi Valkeinen 2021-06-10 721 * @sd_state: V4L2 sub-device state
9746b11715c3949 Martina Krasteva 2021-02-03 722 * @fsize: V4L2 sub-device size enumeration need to be filled
9746b11715c3949 Martina Krasteva 2021-02-03 723 *
9746b11715c3949 Martina Krasteva 2021-02-03 724 * Return: 0 if successful, error code otherwise.
9746b11715c3949 Martina Krasteva 2021-02-03 725 */
9746b11715c3949 Martina Krasteva 2021-02-03 726 static int imx334_enum_frame_size(struct v4l2_subdev *sd,
0d346d2a6f54f06 Tomi Valkeinen 2021-06-10 727 struct v4l2_subdev_state *sd_state,
9746b11715c3949 Martina Krasteva 2021-02-03 728 struct v4l2_subdev_frame_size_enum *fsize)
9746b11715c3949 Martina Krasteva 2021-02-03 729 {
99a7e6eda2d21df Shravan Chippa 2022-07-28 730 int i;
99a7e6eda2d21df Shravan Chippa 2022-07-28 731
99a7e6eda2d21df Shravan Chippa 2022-07-28 732 if (fsize->index >= ARRAY_SIZE(supported_modes))
9746b11715c3949 Martina Krasteva 2021-02-03 733 return -EINVAL;
9746b11715c3949 Martina Krasteva 2021-02-03 734
99a7e6eda2d21df Shravan Chippa 2022-07-28 @735 for (i = 0; i < ARRAY_SIZE(codes); i++) {
99a7e6eda2d21df Shravan Chippa 2022-07-28 736 if (codes[i] == fsize->code)
99a7e6eda2d21df Shravan Chippa 2022-07-28 737 break;
9746b11715c3949 Martina Krasteva 2021-02-03 738 return -EINVAL;
99a7e6eda2d21df Shravan Chippa 2022-07-28 739 }
9746b11715c3949 Martina Krasteva 2021-02-03 740
99a7e6eda2d21df Shravan Chippa 2022-07-28 741 fsize->min_width = supported_modes[fsize->index].width;
9746b11715c3949 Martina Krasteva 2021-02-03 742 fsize->max_width = fsize->min_width;
99a7e6eda2d21df Shravan Chippa 2022-07-28 743 fsize->min_height = supported_modes[fsize->index].height;
9746b11715c3949 Martina Krasteva 2021-02-03 744 fsize->max_height = fsize->min_height;
9746b11715c3949 Martina Krasteva 2021-02-03 745
9746b11715c3949 Martina Krasteva 2021-02-03 746 return 0;
9746b11715c3949 Martina Krasteva 2021-02-03 747 }
9746b11715c3949 Martina Krasteva 2021-02-03 748
9746b11715c3949 Martina Krasteva 2021-02-03 749 /**
9746b11715c3949 Martina Krasteva 2021-02-03 750 * imx334_fill_pad_format() - Fill subdevice pad format
9746b11715c3949 Martina Krasteva 2021-02-03 751 * from selected sensor mode
9746b11715c3949 Martina Krasteva 2021-02-03 752 * @imx334: pointer to imx334 device
9746b11715c3949 Martina Krasteva 2021-02-03 753 * @mode: pointer to imx334_mode sensor mode
9746b11715c3949 Martina Krasteva 2021-02-03 754 * @fmt: V4L2 sub-device format need to be filled
9746b11715c3949 Martina Krasteva 2021-02-03 755 */
9746b11715c3949 Martina Krasteva 2021-02-03 756 static void imx334_fill_pad_format(struct imx334 *imx334,
9746b11715c3949 Martina Krasteva 2021-02-03 757 const struct imx334_mode *mode,
9746b11715c3949 Martina Krasteva 2021-02-03 758 struct v4l2_subdev_format *fmt)
9746b11715c3949 Martina Krasteva 2021-02-03 759 {
9746b11715c3949 Martina Krasteva 2021-02-03 760 fmt->format.width = mode->width;
9746b11715c3949 Martina Krasteva 2021-02-03 761 fmt->format.height = mode->height;
99a7e6eda2d21df Shravan Chippa 2022-07-28 762 fmt->format.code = imx334->cur_code;
9746b11715c3949 Martina Krasteva 2021-02-03 763 fmt->format.field = V4L2_FIELD_NONE;
9746b11715c3949 Martina Krasteva 2021-02-03 764 fmt->format.colorspace = V4L2_COLORSPACE_RAW;
9746b11715c3949 Martina Krasteva 2021-02-03 765 fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
9746b11715c3949 Martina Krasteva 2021-02-03 766 fmt->format.quantization = V4L2_QUANTIZATION_DEFAULT;
9746b11715c3949 Martina Krasteva 2021-02-03 767 fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
9746b11715c3949 Martina Krasteva 2021-02-03 768 }
9746b11715c3949 Martina Krasteva 2021-02-03 769
9746b11715c3949 Martina Krasteva 2021-02-03 770 /**
9746b11715c3949 Martina Krasteva 2021-02-03 771 * imx334_get_pad_format() - Get subdevice pad format
9746b11715c3949 Martina Krasteva 2021-02-03 772 * @sd: pointer to imx334 V4L2 sub-device structure
0d346d2a6f54f06 Tomi Valkeinen 2021-06-10 773 * @sd_state: V4L2 sub-device state
9746b11715c3949 Martina Krasteva 2021-02-03 774 * @fmt: V4L2 sub-device format need to be set
9746b11715c3949 Martina Krasteva 2021-02-03 775 *
9746b11715c3949 Martina Krasteva 2021-02-03 776 * Return: 0 if successful, error code otherwise.
9746b11715c3949 Martina Krasteva 2021-02-03 777 */
9746b11715c3949 Martina Krasteva 2021-02-03 778 static int imx334_get_pad_format(struct v4l2_subdev *sd,
0d346d2a6f54f06 Tomi Valkeinen 2021-06-10 779 struct v4l2_subdev_state *sd_state,
9746b11715c3949 Martina Krasteva 2021-02-03 780 struct v4l2_subdev_format *fmt)
9746b11715c3949 Martina Krasteva 2021-02-03 781 {
9746b11715c3949 Martina Krasteva 2021-02-03 782 struct imx334 *imx334 = to_imx334(sd);
9746b11715c3949 Martina Krasteva 2021-02-03 783
9746b11715c3949 Martina Krasteva 2021-02-03 784 mutex_lock(&imx334->mutex);
9746b11715c3949 Martina Krasteva 2021-02-03 785
9746b11715c3949 Martina Krasteva 2021-02-03 786 if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
9746b11715c3949 Martina Krasteva 2021-02-03 787 struct v4l2_mbus_framefmt *framefmt;
9746b11715c3949 Martina Krasteva 2021-02-03 788
0d346d2a6f54f06 Tomi Valkeinen 2021-06-10 789 framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
9746b11715c3949 Martina Krasteva 2021-02-03 790 fmt->format = *framefmt;
9746b11715c3949 Martina Krasteva 2021-02-03 791 } else {
9746b11715c3949 Martina Krasteva 2021-02-03 792 imx334_fill_pad_format(imx334, imx334->cur_mode, fmt);
9746b11715c3949 Martina Krasteva 2021-02-03 793 }
9746b11715c3949 Martina Krasteva 2021-02-03 794
9746b11715c3949 Martina Krasteva 2021-02-03 795 mutex_unlock(&imx334->mutex);
9746b11715c3949 Martina Krasteva 2021-02-03 796
9746b11715c3949 Martina Krasteva 2021-02-03 797 return 0;
9746b11715c3949 Martina Krasteva 2021-02-03 798 }
9746b11715c3949 Martina Krasteva 2021-02-03 799
99a7e6eda2d21df Shravan Chippa 2022-07-28 800 static int imx219_get_format_code(struct imx334 *imx334, struct v4l2_subdev_format *fmt)
99a7e6eda2d21df Shravan Chippa 2022-07-28 801 {
99a7e6eda2d21df Shravan Chippa 2022-07-28 802 int i;
99a7e6eda2d21df Shravan Chippa 2022-07-28 803
99a7e6eda2d21df Shravan Chippa 2022-07-28 804 for (i = 0; i < ARRAY_SIZE(codes); i++) {
99a7e6eda2d21df Shravan Chippa 2022-07-28 805 if (codes[i] == fmt->format.code)
99a7e6eda2d21df Shravan Chippa 2022-07-28 806 return codes[i];
99a7e6eda2d21df Shravan Chippa 2022-07-28 807 }
99a7e6eda2d21df Shravan Chippa 2022-07-28 808
99a7e6eda2d21df Shravan Chippa 2022-07-28 809 return -EINVAL;
99a7e6eda2d21df Shravan Chippa 2022-07-28 810 }
99a7e6eda2d21df Shravan Chippa 2022-07-28 811
9746b11715c3949 Martina Krasteva 2021-02-03 812 /**
9746b11715c3949 Martina Krasteva 2021-02-03 813 * imx334_set_pad_format() - Set subdevice pad format
9746b11715c3949 Martina Krasteva 2021-02-03 814 * @sd: pointer to imx334 V4L2 sub-device structure
0d346d2a6f54f06 Tomi Valkeinen 2021-06-10 815 * @sd_state: V4L2 sub-device state
9746b11715c3949 Martina Krasteva 2021-02-03 816 * @fmt: V4L2 sub-device format need to be set
9746b11715c3949 Martina Krasteva 2021-02-03 817 *
9746b11715c3949 Martina Krasteva 2021-02-03 818 * Return: 0 if successful, error code otherwise.
9746b11715c3949 Martina Krasteva 2021-02-03 819 */
9746b11715c3949 Martina Krasteva 2021-02-03 820 static int imx334_set_pad_format(struct v4l2_subdev *sd,
0d346d2a6f54f06 Tomi Valkeinen 2021-06-10 821 struct v4l2_subdev_state *sd_state,
9746b11715c3949 Martina Krasteva 2021-02-03 822 struct v4l2_subdev_format *fmt)
9746b11715c3949 Martina Krasteva 2021-02-03 823 {
9746b11715c3949 Martina Krasteva 2021-02-03 824 struct imx334 *imx334 = to_imx334(sd);
9746b11715c3949 Martina Krasteva 2021-02-03 825 const struct imx334_mode *mode;
9746b11715c3949 Martina Krasteva 2021-02-03 826 int ret = 0;
99a7e6eda2d21df Shravan Chippa 2022-07-28 827 u32 code;
9746b11715c3949 Martina Krasteva 2021-02-03 828
9746b11715c3949 Martina Krasteva 2021-02-03 829 mutex_lock(&imx334->mutex);
9746b11715c3949 Martina Krasteva 2021-02-03 830
99a7e6eda2d21df Shravan Chippa 2022-07-28 831 code = imx219_get_format_code(imx334, fmt);
99a7e6eda2d21df Shravan Chippa 2022-07-28 @832 if (code < 0)
99a7e6eda2d21df Shravan Chippa 2022-07-28 833 return -EINVAL;
99a7e6eda2d21df Shravan Chippa 2022-07-28 834
99a7e6eda2d21df Shravan Chippa 2022-07-28 835 imx334->cur_code = code;
99a7e6eda2d21df Shravan Chippa 2022-07-28 836
99a7e6eda2d21df Shravan Chippa 2022-07-28 837 mode = v4l2_find_nearest_size(supported_modes,
99a7e6eda2d21df Shravan Chippa 2022-07-28 838 ARRAY_SIZE(supported_modes),
99a7e6eda2d21df Shravan Chippa 2022-07-28 839 width, height,
99a7e6eda2d21df Shravan Chippa 2022-07-28 840 fmt->format.width, fmt->format.height);
99a7e6eda2d21df Shravan Chippa 2022-07-28 841
9746b11715c3949 Martina Krasteva 2021-02-03 842 imx334_fill_pad_format(imx334, mode, fmt);
9746b11715c3949 Martina Krasteva 2021-02-03 843
9746b11715c3949 Martina Krasteva 2021-02-03 844 if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
9746b11715c3949 Martina Krasteva 2021-02-03 845 struct v4l2_mbus_framefmt *framefmt;
9746b11715c3949 Martina Krasteva 2021-02-03 846
0d346d2a6f54f06 Tomi Valkeinen 2021-06-10 847 framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
9746b11715c3949 Martina Krasteva 2021-02-03 848 *framefmt = fmt->format;
99a7e6eda2d21df Shravan Chippa 2022-07-28 849 } else if (imx334->cur_mode != mode) {
9746b11715c3949 Martina Krasteva 2021-02-03 850 ret = imx334_update_controls(imx334, mode);
9746b11715c3949 Martina Krasteva 2021-02-03 851 if (!ret)
9746b11715c3949 Martina Krasteva 2021-02-03 852 imx334->cur_mode = mode;
9746b11715c3949 Martina Krasteva 2021-02-03 853 }
9746b11715c3949 Martina Krasteva 2021-02-03 854
9746b11715c3949 Martina Krasteva 2021-02-03 855 mutex_unlock(&imx334->mutex);
9746b11715c3949 Martina Krasteva 2021-02-03 856
9746b11715c3949 Martina Krasteva 2021-02-03 857 return ret;
9746b11715c3949 Martina Krasteva 2021-02-03 858 }
9746b11715c3949 Martina Krasteva 2021-02-03 859
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH] media: i2c: imx334: support lower bandwidth mode
@ 2022-07-28 6:30 shravan kumar
2022-08-22 11:24 ` Shravan.Chippa
2022-08-22 15:53 ` Jacopo Mondi
0 siblings, 2 replies; 8+ messages in thread
From: shravan kumar @ 2022-07-28 6:30 UTC (permalink / raw)
To: paul.j.murphy, daniele.alessandrelli, mchehab
Cc: linux-media, linux-kernel, Shravan Chippa, Conor Dooley,
Prakash Battu
From: Shravan Chippa <shravan.chippa@microchip.com>
Some platforms may not be capable of supporting the bandwidth
required for 12 bit or 3840x2160 resolutions.
Add support for dynamically selecting 10 bit and 1920x1080
resolutions while leaving the existing configuration as default
CC: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Prakash Battu <Prakash.Battu@microchip.com>
Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
drivers/media/i2c/imx334.c | 306 +++++++++++++++++++++++++++++++++----
1 file changed, 279 insertions(+), 27 deletions(-)
diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 062125501788..d1fa4c4d4d9e 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -79,7 +79,6 @@ struct imx334_reg_list {
* struct imx334_mode - imx334 sensor mode structure
* @width: Frame width
* @height: Frame height
- * @code: Format code
* @hblank: Horizontal blanking in lines
* @vblank: Vertical blanking in lines
* @vblank_min: Minimal vertical blanking in lines
@@ -91,7 +90,6 @@ struct imx334_reg_list {
struct imx334_mode {
u32 width;
u32 height;
- u32 code;
u32 hblank;
u32 vblank;
u32 vblank_min;
@@ -119,6 +117,7 @@ struct imx334_mode {
* @vblank: Vertical blanking in lines
* @cur_mode: Pointer to current selected sensor mode
* @mutex: Mutex for serializing sensor controls
+ * @cur_code: current selected format code
* @streaming: Flag indicating streaming state
*/
struct imx334 {
@@ -140,6 +139,7 @@ struct imx334 {
u32 vblank;
const struct imx334_mode *cur_mode;
struct mutex mutex;
+ u32 cur_code;
bool streaming;
};
@@ -147,6 +147,169 @@ static const s64 link_freq[] = {
IMX334_LINK_FREQ,
};
+/* Sensor mode registers */
+static const struct imx334_reg mode_1920x1080_regs[] = {
+ {0x3000, 0x01},
+ {0x3018, 0x04},
+ {0x3030, 0xCA},
+ {0x3031, 0x08},
+ {0x3032, 0x00},
+ {0x3034, 0x4C},
+ {0x3035, 0x04},
+ {0x302C, 0xF0},
+ {0x302D, 0x03},
+ {0x302E, 0x80},
+ {0x302F, 0x07},
+ {0x3074, 0xCC},
+ {0x3075, 0x02},
+ {0x308E, 0xCD},
+ {0x308F, 0x02},
+ {0x3076, 0x38},
+ {0x3077, 0x04},
+ {0x3090, 0x38},
+ {0x3091, 0x04},
+ {0x3308, 0x38},
+ {0x3309, 0x04},
+ {0x30C6, 0x00},
+ {0x30C7, 0x00},
+ {0x30CE, 0x00},
+ {0x30CF, 0x00},
+ {0x30D8, 0x18},
+ {0x30D9, 0x0A},
+ {0x304C, 0x00},
+ {0x304E, 0x00},
+ {0x304F, 0x00},
+ {0x3050, 0x00},
+ {0x30B6, 0x00},
+ {0x30B7, 0x00},
+ {0x3116, 0x08},
+ {0x3117, 0x00},
+ {0x31A0, 0x20},
+ {0x31A1, 0x0F},
+ {0x300C, 0x3B},
+ {0x300D, 0x29},
+ {0x314C, 0x29},
+ {0x314D, 0x01},
+ {0x315A, 0x0A},
+ {0x3168, 0xA0},
+ {0x316A, 0x7E},
+ {0x319E, 0x02},
+ {0x3199, 0x00},
+ {0x319D, 0x00},
+ {0x31DD, 0x03},
+ {0x3300, 0x00},
+ {0x341C, 0xFF},
+ {0x341D, 0x01},
+ {0x3A01, 0x03},
+ {0x3A18, 0x7F},
+ {0x3A19, 0x00},
+ {0x3A1A, 0x37},
+ {0x3A1B, 0x00},
+ {0x3A1C, 0x37},
+ {0x3A1D, 0x00},
+ {0x3A1E, 0xF7},
+ {0x3A1F, 0x00},
+ {0x3A20, 0x3F},
+ {0x3A21, 0x00},
+ {0x3A20, 0x6F},
+ {0x3A21, 0x00},
+ {0x3A20, 0x3F},
+ {0x3A21, 0x00},
+ {0x3A20, 0x5F},
+ {0x3A21, 0x00},
+ {0x3A20, 0x2F},
+ {0x3A21, 0x00},
+ {0x3078, 0x02},
+ {0x3079, 0x00},
+ {0x307A, 0x00},
+ {0x307B, 0x00},
+ {0x3080, 0x02},
+ {0x3081, 0x00},
+ {0x3082, 0x00},
+ {0x3083, 0x00},
+ {0x3088, 0x02},
+ {0x3094, 0x00},
+ {0x3095, 0x00},
+ {0x3096, 0x00},
+ {0x309B, 0x02},
+ {0x309C, 0x00},
+ {0x309D, 0x00},
+ {0x309E, 0x00},
+ {0x30A4, 0x00},
+ {0x30A5, 0x00},
+ {0x3288, 0x21},
+ {0x328A, 0x02},
+ {0x3414, 0x05},
+ {0x3416, 0x18},
+ {0x35AC, 0x0E},
+ {0x3648, 0x01},
+ {0x364A, 0x04},
+ {0x364C, 0x04},
+ {0x3678, 0x01},
+ {0x367C, 0x31},
+ {0x367E, 0x31},
+ {0x3708, 0x02},
+ {0x3714, 0x01},
+ {0x3715, 0x02},
+ {0x3716, 0x02},
+ {0x3717, 0x02},
+ {0x371C, 0x3D},
+ {0x371D, 0x3F},
+ {0x372C, 0x00},
+ {0x372D, 0x00},
+ {0x372E, 0x46},
+ {0x372F, 0x00},
+ {0x3730, 0x89},
+ {0x3731, 0x00},
+ {0x3732, 0x08},
+ {0x3733, 0x01},
+ {0x3734, 0xFE},
+ {0x3735, 0x05},
+ {0x375D, 0x00},
+ {0x375E, 0x00},
+ {0x375F, 0x61},
+ {0x3760, 0x06},
+ {0x3768, 0x1B},
+ {0x3769, 0x1B},
+ {0x376A, 0x1A},
+ {0x376B, 0x19},
+ {0x376C, 0x18},
+ {0x376D, 0x14},
+ {0x376E, 0x0F},
+ {0x3776, 0x00},
+ {0x3777, 0x00},
+ {0x3778, 0x46},
+ {0x3779, 0x00},
+ {0x377A, 0x08},
+ {0x377B, 0x01},
+ {0x377C, 0x45},
+ {0x377D, 0x01},
+ {0x377E, 0x23},
+ {0x377F, 0x02},
+ {0x3780, 0xD9},
+ {0x3781, 0x03},
+ {0x3782, 0xF5},
+ {0x3783, 0x06},
+ {0x3784, 0xA5},
+ {0x3788, 0x0F},
+ {0x378A, 0xD9},
+ {0x378B, 0x03},
+ {0x378C, 0xEB},
+ {0x378D, 0x05},
+ {0x378E, 0x87},
+ {0x378F, 0x06},
+ {0x3790, 0xF5},
+ {0x3792, 0x43},
+ {0x3794, 0x7A},
+ {0x3796, 0xA1},
+ {0x37B0, 0x37},
+ {0x3E04, 0x0E},
+ {0x30E8, 0x50},
+ {0x30E9, 0x00},
+ {0x3E04, 0x0E},
+ {0x3002, 0x00},
+};
+
/* Sensor mode registers */
static const struct imx334_reg mode_3840x2160_regs[] = {
{0x3000, 0x01},
@@ -243,20 +406,57 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
{0x3a00, 0x01},
};
+static const struct imx334_reg raw10_framefmt_regs[] = {
+ {0x3050, 0x00},
+ {0x319D, 0x00},
+ {0x341C, 0xFF},
+ {0x341D, 0x01},
+};
+
+static const struct imx334_reg raw12_framefmt_regs[] = {
+ {0x3050, 0x01},
+ {0x319D, 0x01},
+ {0x341C, 0x47},
+ {0x341D, 0x00},
+};
+
+/*
+ * The supported BUS formats.
+ */
+static const u32 codes[] = {
+ MEDIA_BUS_FMT_SRGGB10_1X10,
+ MEDIA_BUS_FMT_SRGGB12_1X12,
+};
+
/* 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 = 1920,
+ .height = 1080,
+ .hblank = 280,
+ .vblank = 1170,
+ .vblank_min = 90,
+ .vblank_max = 132840,
+ .pclk = 594000000,
+ .link_freq_idx = 0,
+ .reg_list = {
+ .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
+ .regs = mode_1920x1080_regs,
+ },
+ },
+ {
+ .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,
+ },
},
};
@@ -382,7 +582,8 @@ static int imx334_update_controls(struct imx334 *imx334,
if (ret)
return ret;
- ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
+ ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, IMX334_REG_MIN,
+ IMX334_REG_MAX, 1, mode->hblank);
if (ret)
return ret;
@@ -506,10 +707,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(codes))
return -EINVAL;
- code->code = supported_mode.code;
+ code->code = codes[code->index];
return 0;
}
@@ -526,15 +727,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)
+ int i;
+
+ if (fsize->index >= ARRAY_SIZE(supported_modes))
return -EINVAL;
- if (fsize->code != supported_mode.code)
+ for (i = 0; i < ARRAY_SIZE(codes); i++) {
+ if (codes[i] == fsize->code)
+ break;
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;
@@ -553,7 +759,7 @@ 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.code = imx334->cur_code;
fmt->format.field = V4L2_FIELD_NONE;
fmt->format.colorspace = V4L2_COLORSPACE_RAW;
fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
@@ -591,6 +797,18 @@ static int imx334_get_pad_format(struct v4l2_subdev *sd,
return 0;
}
+static int imx219_get_format_code(struct imx334 *imx334, struct v4l2_subdev_format *fmt)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(codes); i++) {
+ if (codes[i] == fmt->format.code)
+ return codes[i];
+ }
+
+ return -EINVAL;
+}
+
/**
* imx334_set_pad_format() - Set subdevice pad format
* @sd: pointer to imx334 V4L2 sub-device structure
@@ -606,10 +824,21 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
struct imx334 *imx334 = to_imx334(sd);
const struct imx334_mode *mode;
int ret = 0;
+ u32 code;
mutex_lock(&imx334->mutex);
- mode = &supported_mode;
+ code = imx219_get_format_code(imx334, fmt);
+ if (code < 0)
+ return -EINVAL;
+
+ imx334->cur_code = code;
+
+ mode = v4l2_find_nearest_size(supported_modes,
+ ARRAY_SIZE(supported_modes),
+ width, height,
+ fmt->format.width, fmt->format.height);
+
imx334_fill_pad_format(imx334, mode, fmt);
if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
@@ -617,7 +846,7 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
*framefmt = fmt->format;
- } else {
+ } else if (imx334->cur_mode != mode) {
ret = imx334_update_controls(imx334, mode);
if (!ret)
imx334->cur_mode = mode;
@@ -642,11 +871,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[1], &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
@@ -667,6 +911,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) {
@@ -1037,7 +1288,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[1];
+ imx334->cur_code = codes[1];
imx334->vblank = imx334->cur_mode->vblank;
ret = imx334_init_controls(imx334);
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* RE: [PATCH] media: i2c: imx334: support lower bandwidth mode
2022-07-28 6:30 shravan kumar
@ 2022-08-22 11:24 ` Shravan.Chippa
2022-08-22 15:53 ` Jacopo Mondi
1 sibling, 0 replies; 8+ messages in thread
From: Shravan.Chippa @ 2022-08-22 11:24 UTC (permalink / raw)
To: paul.j.murphy, daniele.alessandrelli, mchehab
Cc: linux-media, linux-kernel, Conor.Dooley, Prakash.Battu,
Shravan.Chippa
A gentle ping!
Thanks,
Shravan.
> -----Original Message-----
> From: shravan kumar <shravan.chippa@microchip.com>
> Sent: 28 July 2022 12:01 PM
> To: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; shravan
> Chippa - I35088 <Shravan.Chippa@microchip.com>; Conor Dooley - M52691
> <Conor.Dooley@microchip.com>; Battu Prakash Reddy - I30399
> <Prakash.Battu@microchip.com>
> Subject: [PATCH] media: i2c: imx334: support lower bandwidth mode
>
> From: Shravan Chippa <shravan.chippa@microchip.com>
>
> Some platforms may not be capable of supporting the bandwidth required
> for 12 bit or 3840x2160 resolutions.
>
> Add support for dynamically selecting 10 bit and 1920x1080 resolutions while
> leaving the existing configuration as default
>
> CC: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Prakash Battu <Prakash.Battu@microchip.com>
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
> drivers/media/i2c/imx334.c | 306
> +++++++++++++++++++++++++++++++++----
> 1 file changed, 279 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c index
> 062125501788..d1fa4c4d4d9e 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -79,7 +79,6 @@ struct imx334_reg_list {
> * struct imx334_mode - imx334 sensor mode structure
> * @width: Frame width
> * @height: Frame height
> - * @code: Format code
> * @hblank: Horizontal blanking in lines
> * @vblank: Vertical blanking in lines
> * @vblank_min: Minimal vertical blanking in lines @@ -91,7 +90,6 @@ struct
> imx334_reg_list { struct imx334_mode {
> u32 width;
> u32 height;
> - u32 code;
> u32 hblank;
> u32 vblank;
> u32 vblank_min;
> @@ -119,6 +117,7 @@ struct imx334_mode {
> * @vblank: Vertical blanking in lines
> * @cur_mode: Pointer to current selected sensor mode
> * @mutex: Mutex for serializing sensor controls
> + * @cur_code: current selected format code
> * @streaming: Flag indicating streaming state
> */
> struct imx334 {
> @@ -140,6 +139,7 @@ struct imx334 {
> u32 vblank;
> const struct imx334_mode *cur_mode;
> struct mutex mutex;
> + u32 cur_code;
> bool streaming;
> };
>
> @@ -147,6 +147,169 @@ static const s64 link_freq[] = {
> IMX334_LINK_FREQ,
> };
>
> +/* Sensor mode registers */
> +static const struct imx334_reg mode_1920x1080_regs[] = {
> + {0x3000, 0x01},
> + {0x3018, 0x04},
> + {0x3030, 0xCA},
> + {0x3031, 0x08},
> + {0x3032, 0x00},
> + {0x3034, 0x4C},
> + {0x3035, 0x04},
> + {0x302C, 0xF0},
> + {0x302D, 0x03},
> + {0x302E, 0x80},
> + {0x302F, 0x07},
> + {0x3074, 0xCC},
> + {0x3075, 0x02},
> + {0x308E, 0xCD},
> + {0x308F, 0x02},
> + {0x3076, 0x38},
> + {0x3077, 0x04},
> + {0x3090, 0x38},
> + {0x3091, 0x04},
> + {0x3308, 0x38},
> + {0x3309, 0x04},
> + {0x30C6, 0x00},
> + {0x30C7, 0x00},
> + {0x30CE, 0x00},
> + {0x30CF, 0x00},
> + {0x30D8, 0x18},
> + {0x30D9, 0x0A},
> + {0x304C, 0x00},
> + {0x304E, 0x00},
> + {0x304F, 0x00},
> + {0x3050, 0x00},
> + {0x30B6, 0x00},
> + {0x30B7, 0x00},
> + {0x3116, 0x08},
> + {0x3117, 0x00},
> + {0x31A0, 0x20},
> + {0x31A1, 0x0F},
> + {0x300C, 0x3B},
> + {0x300D, 0x29},
> + {0x314C, 0x29},
> + {0x314D, 0x01},
> + {0x315A, 0x0A},
> + {0x3168, 0xA0},
> + {0x316A, 0x7E},
> + {0x319E, 0x02},
> + {0x3199, 0x00},
> + {0x319D, 0x00},
> + {0x31DD, 0x03},
> + {0x3300, 0x00},
> + {0x341C, 0xFF},
> + {0x341D, 0x01},
> + {0x3A01, 0x03},
> + {0x3A18, 0x7F},
> + {0x3A19, 0x00},
> + {0x3A1A, 0x37},
> + {0x3A1B, 0x00},
> + {0x3A1C, 0x37},
> + {0x3A1D, 0x00},
> + {0x3A1E, 0xF7},
> + {0x3A1F, 0x00},
> + {0x3A20, 0x3F},
> + {0x3A21, 0x00},
> + {0x3A20, 0x6F},
> + {0x3A21, 0x00},
> + {0x3A20, 0x3F},
> + {0x3A21, 0x00},
> + {0x3A20, 0x5F},
> + {0x3A21, 0x00},
> + {0x3A20, 0x2F},
> + {0x3A21, 0x00},
> + {0x3078, 0x02},
> + {0x3079, 0x00},
> + {0x307A, 0x00},
> + {0x307B, 0x00},
> + {0x3080, 0x02},
> + {0x3081, 0x00},
> + {0x3082, 0x00},
> + {0x3083, 0x00},
> + {0x3088, 0x02},
> + {0x3094, 0x00},
> + {0x3095, 0x00},
> + {0x3096, 0x00},
> + {0x309B, 0x02},
> + {0x309C, 0x00},
> + {0x309D, 0x00},
> + {0x309E, 0x00},
> + {0x30A4, 0x00},
> + {0x30A5, 0x00},
> + {0x3288, 0x21},
> + {0x328A, 0x02},
> + {0x3414, 0x05},
> + {0x3416, 0x18},
> + {0x35AC, 0x0E},
> + {0x3648, 0x01},
> + {0x364A, 0x04},
> + {0x364C, 0x04},
> + {0x3678, 0x01},
> + {0x367C, 0x31},
> + {0x367E, 0x31},
> + {0x3708, 0x02},
> + {0x3714, 0x01},
> + {0x3715, 0x02},
> + {0x3716, 0x02},
> + {0x3717, 0x02},
> + {0x371C, 0x3D},
> + {0x371D, 0x3F},
> + {0x372C, 0x00},
> + {0x372D, 0x00},
> + {0x372E, 0x46},
> + {0x372F, 0x00},
> + {0x3730, 0x89},
> + {0x3731, 0x00},
> + {0x3732, 0x08},
> + {0x3733, 0x01},
> + {0x3734, 0xFE},
> + {0x3735, 0x05},
> + {0x375D, 0x00},
> + {0x375E, 0x00},
> + {0x375F, 0x61},
> + {0x3760, 0x06},
> + {0x3768, 0x1B},
> + {0x3769, 0x1B},
> + {0x376A, 0x1A},
> + {0x376B, 0x19},
> + {0x376C, 0x18},
> + {0x376D, 0x14},
> + {0x376E, 0x0F},
> + {0x3776, 0x00},
> + {0x3777, 0x00},
> + {0x3778, 0x46},
> + {0x3779, 0x00},
> + {0x377A, 0x08},
> + {0x377B, 0x01},
> + {0x377C, 0x45},
> + {0x377D, 0x01},
> + {0x377E, 0x23},
> + {0x377F, 0x02},
> + {0x3780, 0xD9},
> + {0x3781, 0x03},
> + {0x3782, 0xF5},
> + {0x3783, 0x06},
> + {0x3784, 0xA5},
> + {0x3788, 0x0F},
> + {0x378A, 0xD9},
> + {0x378B, 0x03},
> + {0x378C, 0xEB},
> + {0x378D, 0x05},
> + {0x378E, 0x87},
> + {0x378F, 0x06},
> + {0x3790, 0xF5},
> + {0x3792, 0x43},
> + {0x3794, 0x7A},
> + {0x3796, 0xA1},
> + {0x37B0, 0x37},
> + {0x3E04, 0x0E},
> + {0x30E8, 0x50},
> + {0x30E9, 0x00},
> + {0x3E04, 0x0E},
> + {0x3002, 0x00},
> +};
> +
> /* Sensor mode registers */
> static const struct imx334_reg mode_3840x2160_regs[] = {
> {0x3000, 0x01},
> @@ -243,20 +406,57 @@ static const struct imx334_reg
> mode_3840x2160_regs[] = {
> {0x3a00, 0x01},
> };
>
> +static const struct imx334_reg raw10_framefmt_regs[] = {
> + {0x3050, 0x00},
> + {0x319D, 0x00},
> + {0x341C, 0xFF},
> + {0x341D, 0x01},
> +};
> +
> +static const struct imx334_reg raw12_framefmt_regs[] = {
> + {0x3050, 0x01},
> + {0x319D, 0x01},
> + {0x341C, 0x47},
> + {0x341D, 0x00},
> +};
> +
> +/*
> + * The supported BUS formats.
> + */
> +static const u32 codes[] = {
> + MEDIA_BUS_FMT_SRGGB10_1X10,
> + MEDIA_BUS_FMT_SRGGB12_1X12,
> +};
> +
> /* 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 = 1920,
> + .height = 1080,
> + .hblank = 280,
> + .vblank = 1170,
> + .vblank_min = 90,
> + .vblank_max = 132840,
> + .pclk = 594000000,
> + .link_freq_idx = 0,
> + .reg_list = {
> + .num_of_regs =
> ARRAY_SIZE(mode_1920x1080_regs),
> + .regs = mode_1920x1080_regs,
> + },
> + },
> + {
> + .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,
> + },
> },
> };
>
> @@ -382,7 +582,8 @@ static int imx334_update_controls(struct imx334
> *imx334,
> if (ret)
> return ret;
>
> - ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
> + ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl,
> IMX334_REG_MIN,
> + IMX334_REG_MAX, 1, mode->hblank);
> if (ret)
> return ret;
>
> @@ -506,10 +707,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(codes))
> return -EINVAL;
>
> - code->code = supported_mode.code;
> + code->code = codes[code->index];
>
> return 0;
> }
> @@ -526,15 +727,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)
> + int i;
> +
> + if (fsize->index >= ARRAY_SIZE(supported_modes))
> return -EINVAL;
>
> - if (fsize->code != supported_mode.code)
> + for (i = 0; i < ARRAY_SIZE(codes); i++) {
> + if (codes[i] == fsize->code)
> + break;
> 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;
> @@ -553,7 +759,7 @@ 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.code = imx334->cur_code;
> fmt->format.field = V4L2_FIELD_NONE;
> fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; @@ -591,6
> +797,18 @@ static int imx334_get_pad_format(struct v4l2_subdev *sd,
> return 0;
> }
>
> +static int imx219_get_format_code(struct imx334 *imx334, struct
> +v4l2_subdev_format *fmt) {
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(codes); i++) {
> + if (codes[i] == fmt->format.code)
> + return codes[i];
> + }
> +
> + return -EINVAL;
> +}
> +
> /**
> * imx334_set_pad_format() - Set subdevice pad format
> * @sd: pointer to imx334 V4L2 sub-device structure @@ -606,10 +824,21
> @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
> struct imx334 *imx334 = to_imx334(sd);
> const struct imx334_mode *mode;
> int ret = 0;
> + u32 code;
>
> mutex_lock(&imx334->mutex);
>
> - mode = &supported_mode;
> + code = imx219_get_format_code(imx334, fmt);
> + if (code < 0)
> + return -EINVAL;
> +
> + imx334->cur_code = code;
> +
> + mode = v4l2_find_nearest_size(supported_modes,
> + ARRAY_SIZE(supported_modes),
> + width, height,
> + fmt->format.width, fmt->format.height);
> +
> imx334_fill_pad_format(imx334, mode, fmt);
>
> if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { @@ -617,7 +846,7
> @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
>
> framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt-
> >pad);
> *framefmt = fmt->format;
> - } else {
> + } else if (imx334->cur_mode != mode) {
> ret = imx334_update_controls(imx334, mode);
> if (!ret)
> imx334->cur_mode = mode;
> @@ -642,11 +871,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[1], &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
> @@ -667,6 +911,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) {
> @@ -1037,7 +1288,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[1];
> + imx334->cur_code = codes[1];
> imx334->vblank = imx334->cur_mode->vblank;
>
> ret = imx334_init_controls(imx334);
> --
> 2.17.1
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] media: i2c: imx334: support lower bandwidth mode
2022-07-28 6:30 shravan kumar
2022-08-22 11:24 ` Shravan.Chippa
@ 2022-08-22 15:53 ` Jacopo Mondi
2022-08-25 7:16 ` Shravan.Chippa
1 sibling, 1 reply; 8+ messages in thread
From: Jacopo Mondi @ 2022-08-22 15:53 UTC (permalink / raw)
To: shravan kumar
Cc: paul.j.murphy, daniele.alessandrelli, mchehab, linux-media,
linux-kernel, Conor Dooley, Prakash Battu
Hello Shravan,
On Thu, Jul 28, 2022 at 12:00:44PM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
>
> Some platforms may not be capable of supporting the bandwidth
> required for 12 bit or 3840x2160 resolutions.
>
> Add support for dynamically selecting 10 bit and 1920x1080
> resolutions while leaving the existing configuration as default
>
> CC: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Prakash Battu <Prakash.Battu@microchip.com>
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
> drivers/media/i2c/imx334.c | 306 +++++++++++++++++++++++++++++++++----
> 1 file changed, 279 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 062125501788..d1fa4c4d4d9e 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -79,7 +79,6 @@ struct imx334_reg_list {
> * struct imx334_mode - imx334 sensor mode structure
> * @width: Frame width
> * @height: Frame height
> - * @code: Format code
> * @hblank: Horizontal blanking in lines
> * @vblank: Vertical blanking in lines
> * @vblank_min: Minimal vertical blanking in lines
> @@ -91,7 +90,6 @@ struct imx334_reg_list {
> struct imx334_mode {
> u32 width;
> u32 height;
> - u32 code;
> u32 hblank;
> u32 vblank;
> u32 vblank_min;
> @@ -119,6 +117,7 @@ struct imx334_mode {
> * @vblank: Vertical blanking in lines
> * @cur_mode: Pointer to current selected sensor mode
> * @mutex: Mutex for serializing sensor controls
> + * @cur_code: current selected format code
> * @streaming: Flag indicating streaming state
> */
> struct imx334 {
> @@ -140,6 +139,7 @@ struct imx334 {
> u32 vblank;
> const struct imx334_mode *cur_mode;
> struct mutex mutex;
> + u32 cur_code;
> bool streaming;
> };
>
> @@ -147,6 +147,169 @@ static const s64 link_freq[] = {
> IMX334_LINK_FREQ,
> };
>
> +/* Sensor mode registers */
> +static const struct imx334_reg mode_1920x1080_regs[] = {
> + {0x3000, 0x01},
> + {0x3018, 0x04},
> + {0x3030, 0xCA},
> + {0x3031, 0x08},
> + {0x3032, 0x00},
> + {0x3034, 0x4C},
> + {0x3035, 0x04},
> + {0x302C, 0xF0},
> + {0x302D, 0x03},
> + {0x302E, 0x80},
> + {0x302F, 0x07},
> + {0x3074, 0xCC},
> + {0x3075, 0x02},
> + {0x308E, 0xCD},
> + {0x308F, 0x02},
> + {0x3076, 0x38},
> + {0x3077, 0x04},
> + {0x3090, 0x38},
> + {0x3091, 0x04},
> + {0x3308, 0x38},
> + {0x3309, 0x04},
> + {0x30C6, 0x00},
> + {0x30C7, 0x00},
> + {0x30CE, 0x00},
> + {0x30CF, 0x00},
> + {0x30D8, 0x18},
> + {0x30D9, 0x0A},
> + {0x304C, 0x00},
> + {0x304E, 0x00},
> + {0x304F, 0x00},
> + {0x3050, 0x00},
> + {0x30B6, 0x00},
> + {0x30B7, 0x00},
> + {0x3116, 0x08},
> + {0x3117, 0x00},
> + {0x31A0, 0x20},
> + {0x31A1, 0x0F},
> + {0x300C, 0x3B},
> + {0x300D, 0x29},
> + {0x314C, 0x29},
> + {0x314D, 0x01},
> + {0x315A, 0x0A},
> + {0x3168, 0xA0},
> + {0x316A, 0x7E},
> + {0x319E, 0x02},
> + {0x3199, 0x00},
> + {0x319D, 0x00},
> + {0x31DD, 0x03},
> + {0x3300, 0x00},
> + {0x341C, 0xFF},
> + {0x341D, 0x01},
> + {0x3A01, 0x03},
> + {0x3A18, 0x7F},
> + {0x3A19, 0x00},
> + {0x3A1A, 0x37},
> + {0x3A1B, 0x00},
> + {0x3A1C, 0x37},
> + {0x3A1D, 0x00},
> + {0x3A1E, 0xF7},
> + {0x3A1F, 0x00},
> + {0x3A20, 0x3F},
> + {0x3A21, 0x00},
> + {0x3A20, 0x6F},
> + {0x3A21, 0x00},
> + {0x3A20, 0x3F},
> + {0x3A21, 0x00},
> + {0x3A20, 0x5F},
> + {0x3A21, 0x00},
> + {0x3A20, 0x2F},
> + {0x3A21, 0x00},
> + {0x3078, 0x02},
> + {0x3079, 0x00},
> + {0x307A, 0x00},
> + {0x307B, 0x00},
> + {0x3080, 0x02},
> + {0x3081, 0x00},
> + {0x3082, 0x00},
> + {0x3083, 0x00},
> + {0x3088, 0x02},
> + {0x3094, 0x00},
> + {0x3095, 0x00},
> + {0x3096, 0x00},
> + {0x309B, 0x02},
> + {0x309C, 0x00},
> + {0x309D, 0x00},
> + {0x309E, 0x00},
> + {0x30A4, 0x00},
> + {0x30A5, 0x00},
> + {0x3288, 0x21},
> + {0x328A, 0x02},
> + {0x3414, 0x05},
> + {0x3416, 0x18},
> + {0x35AC, 0x0E},
> + {0x3648, 0x01},
> + {0x364A, 0x04},
> + {0x364C, 0x04},
> + {0x3678, 0x01},
> + {0x367C, 0x31},
> + {0x367E, 0x31},
> + {0x3708, 0x02},
> + {0x3714, 0x01},
> + {0x3715, 0x02},
> + {0x3716, 0x02},
> + {0x3717, 0x02},
> + {0x371C, 0x3D},
> + {0x371D, 0x3F},
> + {0x372C, 0x00},
> + {0x372D, 0x00},
> + {0x372E, 0x46},
> + {0x372F, 0x00},
> + {0x3730, 0x89},
> + {0x3731, 0x00},
> + {0x3732, 0x08},
> + {0x3733, 0x01},
> + {0x3734, 0xFE},
> + {0x3735, 0x05},
> + {0x375D, 0x00},
> + {0x375E, 0x00},
> + {0x375F, 0x61},
> + {0x3760, 0x06},
> + {0x3768, 0x1B},
> + {0x3769, 0x1B},
> + {0x376A, 0x1A},
> + {0x376B, 0x19},
> + {0x376C, 0x18},
> + {0x376D, 0x14},
> + {0x376E, 0x0F},
> + {0x3776, 0x00},
> + {0x3777, 0x00},
> + {0x3778, 0x46},
> + {0x3779, 0x00},
> + {0x377A, 0x08},
> + {0x377B, 0x01},
> + {0x377C, 0x45},
> + {0x377D, 0x01},
> + {0x377E, 0x23},
> + {0x377F, 0x02},
> + {0x3780, 0xD9},
> + {0x3781, 0x03},
> + {0x3782, 0xF5},
> + {0x3783, 0x06},
> + {0x3784, 0xA5},
> + {0x3788, 0x0F},
> + {0x378A, 0xD9},
> + {0x378B, 0x03},
> + {0x378C, 0xEB},
> + {0x378D, 0x05},
> + {0x378E, 0x87},
> + {0x378F, 0x06},
> + {0x3790, 0xF5},
> + {0x3792, 0x43},
> + {0x3794, 0x7A},
> + {0x3796, 0xA1},
> + {0x37B0, 0x37},
> + {0x3E04, 0x0E},
> + {0x30E8, 0x50},
> + {0x30E9, 0x00},
> + {0x3E04, 0x0E},
> + {0x3002, 0x00},
> +};
> +
> /* Sensor mode registers */
> static const struct imx334_reg mode_3840x2160_regs[] = {
> {0x3000, 0x01},
> @@ -243,20 +406,57 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
> {0x3a00, 0x01},
> };
>
> +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 codes[] = {
Just "codes" is a little vague ? What about imx334_mbus_codes[] ?
> + MEDIA_BUS_FMT_SRGGB10_1X10,
> + MEDIA_BUS_FMT_SRGGB12_1X12,
> +};
> +
> /* 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 = 1920,
> + .height = 1080,
> + .hblank = 280,
> + .vblank = 1170,
> + .vblank_min = 90,
> + .vblank_max = 132840,
> + .pclk = 594000000,
> + .link_freq_idx = 0,
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> + .regs = mode_1920x1080_regs,
> + },
> + },
> + {
> + .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,
> + },
Nit: I would keep the existing formats and code in position [0] so
that if additional modes are added the defaults can be kept in [0]. Also
I usually see drivers listing larger modes first, but that might not
be a strict rule.
> },
> };
>
> @@ -382,7 +582,8 @@ static int imx334_update_controls(struct imx334 *imx334,
> if (ret)
> return ret;
>
> - ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
> + ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, IMX334_REG_MIN,
> + IMX334_REG_MAX, 1, mode->hblank);
I might have missed why is this change related
> if (ret)
> return ret;
>
> @@ -506,10 +707,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(codes))
> return -EINVAL;
>
> - code->code = supported_mode.code;
> + code->code = codes[code->index];
>
> return 0;
> }
> @@ -526,15 +727,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)
> + int i;
> +
> + if (fsize->index >= ARRAY_SIZE(supported_modes))
> return -EINVAL;
>
> - if (fsize->code != supported_mode.code)
> + for (i = 0; i < ARRAY_SIZE(codes); i++) {
Can we use for (unsigned int i = 0; ... ) now that C99 has been
adopted ?
> + if (codes[i] == fsize->code)
> + break;
> return -EINVAL;
Are you sure this is right ?
If the format code in fsize->code is == codes[1] you will return
-EINVAL ?
Does the sensor support more formats than MEDIA_BUS_FMT_SRGGB10_1X10
and MEDIA_BUS_FMT_SRGGB12_1X12 ? Because otherwise you could replace
the for loop with a simple check.
However support for flip might change the bayer pattern, so it's
better to keep the loop maybe.
What about centrlize it like you're doing in imx219_get_format_code()
and use it here ? You can place the helper just after the supported
mbus codes declaration maybe ?
> + }
>
> - 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;
> @@ -553,7 +759,7 @@ 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.code = imx334->cur_code;
> fmt->format.field = V4L2_FIELD_NONE;
> fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> @@ -591,6 +797,18 @@ static int imx334_get_pad_format(struct v4l2_subdev *sd,
> return 0;
> }
>
> +static int imx219_get_format_code(struct imx334 *imx334, struct v4l2_subdev_format *fmt)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(codes); i++) {
Ditto, unsigned int and possibily declared inside the for loop
> + if (codes[i] == fmt->format.code)
> + return codes[i];
> + }
> +
> + return -EINVAL;
> +}
> +
> /**
> * imx334_set_pad_format() - Set subdevice pad format
> * @sd: pointer to imx334 V4L2 sub-device structure
> @@ -606,10 +824,21 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
> struct imx334 *imx334 = to_imx334(sd);
> const struct imx334_mode *mode;
> int ret = 0;
> + u32 code;
>
> mutex_lock(&imx334->mutex);
>
> - mode = &supported_mode;
> + code = imx219_get_format_code(imx334, fmt);
> + if (code < 0)
> + return -EINVAL;
> +
> + imx334->cur_code = code;
> +
> + mode = v4l2_find_nearest_size(supported_modes,
> + ARRAY_SIZE(supported_modes),
> + width, height,
> + fmt->format.width, fmt->format.height);
> +
> imx334_fill_pad_format(imx334, mode, fmt);
>
> if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> @@ -617,7 +846,7 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
>
> framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> *framefmt = fmt->format;
> - } else {
> + } else if (imx334->cur_mode != mode) {
Is this change related ? I think it's good as it avoids an unecessary
update of the controls, but maybe it's not related to this patch ?
Thanks
j
> ret = imx334_update_controls(imx334, mode);
> if (!ret)
> imx334->cur_mode = mode;
> @@ -642,11 +871,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[1], &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
> @@ -667,6 +911,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) {
> @@ -1037,7 +1288,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[1];
> + imx334->cur_code = codes[1];
> imx334->vblank = imx334->cur_mode->vblank;
>
> ret = imx334_init_controls(imx334);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH] media: i2c: imx334: support lower bandwidth mode
2022-08-22 15:53 ` Jacopo Mondi
@ 2022-08-25 7:16 ` Shravan.Chippa
2022-08-25 7:29 ` Jacopo Mondi
0 siblings, 1 reply; 8+ messages in thread
From: Shravan.Chippa @ 2022-08-25 7:16 UTC (permalink / raw)
To: jacopo
Cc: paul.j.murphy, daniele.alessandrelli, mchehab, linux-media,
linux-kernel, Conor.Dooley, Prakash.Battu
> -----Original Message-----
> From: Jacopo Mondi <jacopo@jmondi.org>
> Sent: 22 August 2022 09:23 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; linux-media@vger.kernel.org; linux-
> kernel@vger.kernel.org; Conor Dooley - M52691
> <Conor.Dooley@microchip.com>; Battu Prakash Reddy - I30399
> <Prakash.Battu@microchip.com>
> Subject: Re: [PATCH] media: i2c: imx334: support lower bandwidth mode
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> Hello Shravan,
>
> On Thu, Jul 28, 2022 at 12:00:44PM +0530, shravan kumar wrote:
> > From: Shravan Chippa <shravan.chippa@microchip.com>
> >
> > Some platforms may not be capable of supporting the bandwidth required
> > for 12 bit or 3840x2160 resolutions.
> >
> > Add support for dynamically selecting 10 bit and 1920x1080 resolutions
> > while leaving the existing configuration as default
> >
> > CC: Conor Dooley <conor.dooley@microchip.com>
> > Signed-off-by: Prakash Battu <Prakash.Battu@microchip.com>
> > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > ---
> > drivers/media/i2c/imx334.c | 306
> > +++++++++++++++++++++++++++++++++----
> > 1 file changed, 279 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index 062125501788..d1fa4c4d4d9e 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -79,7 +79,6 @@ struct imx334_reg_list {
> > * struct imx334_mode - imx334 sensor mode structure
> > * @width: Frame width
> > * @height: Frame height
> > - * @code: Format code
> > * @hblank: Horizontal blanking in lines
> > * @vblank: Vertical blanking in lines
> > * @vblank_min: Minimal vertical blanking in lines @@ -91,7 +90,6 @@
> > struct imx334_reg_list { struct imx334_mode {
> > u32 width;
> > u32 height;
> > - u32 code;
> > u32 hblank;
> > u32 vblank;
> > u32 vblank_min;
> > @@ -119,6 +117,7 @@ struct imx334_mode {
> > * @vblank: Vertical blanking in lines
> > * @cur_mode: Pointer to current selected sensor mode
> > * @mutex: Mutex for serializing sensor controls
> > + * @cur_code: current selected format code
> > * @streaming: Flag indicating streaming state
> > */
> > struct imx334 {
> > @@ -140,6 +139,7 @@ struct imx334 {
> > u32 vblank;
> > const struct imx334_mode *cur_mode;
> > struct mutex mutex;
> > + u32 cur_code;
> > bool streaming;
> > };
> >
> > @@ -147,6 +147,169 @@ static const s64 link_freq[] = {
> > IMX334_LINK_FREQ,
> > };
> >
> > +/* Sensor mode registers */
> > +static const struct imx334_reg mode_1920x1080_regs[] = {
> > + {0x3000, 0x01},
> > + {0x3018, 0x04},
> > + {0x3030, 0xCA},
> > + {0x3031, 0x08},
> > + {0x3032, 0x00},
> > + {0x3034, 0x4C},
> > + {0x3035, 0x04},
> > + {0x302C, 0xF0},
> > + {0x302D, 0x03},
> > + {0x302E, 0x80},
> > + {0x302F, 0x07},
> > + {0x3074, 0xCC},
> > + {0x3075, 0x02},
> > + {0x308E, 0xCD},
> > + {0x308F, 0x02},
> > + {0x3076, 0x38},
> > + {0x3077, 0x04},
> > + {0x3090, 0x38},
> > + {0x3091, 0x04},
> > + {0x3308, 0x38},
> > + {0x3309, 0x04},
> > + {0x30C6, 0x00},
> > + {0x30C7, 0x00},
> > + {0x30CE, 0x00},
> > + {0x30CF, 0x00},
> > + {0x30D8, 0x18},
> > + {0x30D9, 0x0A},
> > + {0x304C, 0x00},
> > + {0x304E, 0x00},
> > + {0x304F, 0x00},
> > + {0x3050, 0x00},
> > + {0x30B6, 0x00},
> > + {0x30B7, 0x00},
> > + {0x3116, 0x08},
> > + {0x3117, 0x00},
> > + {0x31A0, 0x20},
> > + {0x31A1, 0x0F},
> > + {0x300C, 0x3B},
> > + {0x300D, 0x29},
> > + {0x314C, 0x29},
> > + {0x314D, 0x01},
> > + {0x315A, 0x0A},
> > + {0x3168, 0xA0},
> > + {0x316A, 0x7E},
> > + {0x319E, 0x02},
> > + {0x3199, 0x00},
> > + {0x319D, 0x00},
> > + {0x31DD, 0x03},
> > + {0x3300, 0x00},
> > + {0x341C, 0xFF},
> > + {0x341D, 0x01},
> > + {0x3A01, 0x03},
> > + {0x3A18, 0x7F},
> > + {0x3A19, 0x00},
> > + {0x3A1A, 0x37},
> > + {0x3A1B, 0x00},
> > + {0x3A1C, 0x37},
> > + {0x3A1D, 0x00},
> > + {0x3A1E, 0xF7},
> > + {0x3A1F, 0x00},
> > + {0x3A20, 0x3F},
> > + {0x3A21, 0x00},
> > + {0x3A20, 0x6F},
> > + {0x3A21, 0x00},
> > + {0x3A20, 0x3F},
> > + {0x3A21, 0x00},
> > + {0x3A20, 0x5F},
> > + {0x3A21, 0x00},
> > + {0x3A20, 0x2F},
> > + {0x3A21, 0x00},
> > + {0x3078, 0x02},
> > + {0x3079, 0x00},
> > + {0x307A, 0x00},
> > + {0x307B, 0x00},
> > + {0x3080, 0x02},
> > + {0x3081, 0x00},
> > + {0x3082, 0x00},
> > + {0x3083, 0x00},
> > + {0x3088, 0x02},
> > + {0x3094, 0x00},
> > + {0x3095, 0x00},
> > + {0x3096, 0x00},
> > + {0x309B, 0x02},
> > + {0x309C, 0x00},
> > + {0x309D, 0x00},
> > + {0x309E, 0x00},
> > + {0x30A4, 0x00},
> > + {0x30A5, 0x00},
> > + {0x3288, 0x21},
> > + {0x328A, 0x02},
> > + {0x3414, 0x05},
> > + {0x3416, 0x18},
> > + {0x35AC, 0x0E},
> > + {0x3648, 0x01},
> > + {0x364A, 0x04},
> > + {0x364C, 0x04},
> > + {0x3678, 0x01},
> > + {0x367C, 0x31},
> > + {0x367E, 0x31},
> > + {0x3708, 0x02},
> > + {0x3714, 0x01},
> > + {0x3715, 0x02},
> > + {0x3716, 0x02},
> > + {0x3717, 0x02},
> > + {0x371C, 0x3D},
> > + {0x371D, 0x3F},
> > + {0x372C, 0x00},
> > + {0x372D, 0x00},
> > + {0x372E, 0x46},
> > + {0x372F, 0x00},
> > + {0x3730, 0x89},
> > + {0x3731, 0x00},
> > + {0x3732, 0x08},
> > + {0x3733, 0x01},
> > + {0x3734, 0xFE},
> > + {0x3735, 0x05},
> > + {0x375D, 0x00},
> > + {0x375E, 0x00},
> > + {0x375F, 0x61},
> > + {0x3760, 0x06},
> > + {0x3768, 0x1B},
> > + {0x3769, 0x1B},
> > + {0x376A, 0x1A},
> > + {0x376B, 0x19},
> > + {0x376C, 0x18},
> > + {0x376D, 0x14},
> > + {0x376E, 0x0F},
> > + {0x3776, 0x00},
> > + {0x3777, 0x00},
> > + {0x3778, 0x46},
> > + {0x3779, 0x00},
> > + {0x377A, 0x08},
> > + {0x377B, 0x01},
> > + {0x377C, 0x45},
> > + {0x377D, 0x01},
> > + {0x377E, 0x23},
> > + {0x377F, 0x02},
> > + {0x3780, 0xD9},
> > + {0x3781, 0x03},
> > + {0x3782, 0xF5},
> > + {0x3783, 0x06},
> > + {0x3784, 0xA5},
> > + {0x3788, 0x0F},
> > + {0x378A, 0xD9},
> > + {0x378B, 0x03},
> > + {0x378C, 0xEB},
> > + {0x378D, 0x05},
> > + {0x378E, 0x87},
> > + {0x378F, 0x06},
> > + {0x3790, 0xF5},
> > + {0x3792, 0x43},
> > + {0x3794, 0x7A},
> > + {0x3796, 0xA1},
> > + {0x37B0, 0x37},
> > + {0x3E04, 0x0E},
> > + {0x30E8, 0x50},
> > + {0x30E9, 0x00},
> > + {0x3E04, 0x0E},
> > + {0x3002, 0x00},
> > +};
> > +
> > /* Sensor mode registers */
> > static const struct imx334_reg mode_3840x2160_regs[] = {
> > {0x3000, 0x01},
> > @@ -243,20 +406,57 @@ static const struct imx334_reg
> mode_3840x2160_regs[] = {
> > {0x3a00, 0x01},
> > };
> >
> > +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 codes[] = {
>
> Just "codes" is a little vague ? What about imx334_mbus_codes[] ?
>
> > + MEDIA_BUS_FMT_SRGGB10_1X10,
> > + MEDIA_BUS_FMT_SRGGB12_1X12,
> > +};
> > +
> > /* 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 = 1920,
> > + .height = 1080,
> > + .hblank = 280,
> > + .vblank = 1170,
> > + .vblank_min = 90,
> > + .vblank_max = 132840,
> > + .pclk = 594000000,
> > + .link_freq_idx = 0,
> > + .reg_list = {
> > + .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > + .regs = mode_1920x1080_regs,
> > + },
> > + },
> > + {
> > + .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,
> > + },
>
> Nit: I would keep the existing formats and code in position [0] so that if
> additional modes are added the defaults can be kept in [0]. Also I usually see
> drivers listing larger modes first, but that might not be a strict rule.
>
> > },
> > };
> >
> > @@ -382,7 +582,8 @@ static int imx334_update_controls(struct imx334
> *imx334,
> > if (ret)
> > return ret;
> >
> > - ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
> > + ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl,
> IMX334_REG_MIN,
> > + IMX334_REG_MAX, 1, mode->hblank);
>
> I might have missed why is this change related
Hblank is readonly variable.
If I use __v4l2_ctrl_s_ctrl function update return error.
>
> > if (ret)
> > return ret;
> >
> > @@ -506,10 +707,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(codes))
> > return -EINVAL;
> >
> > - code->code = supported_mode.code;
> > + code->code = codes[code->index];
> >
> > return 0;
> > }
> > @@ -526,15 +727,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)
> > + int i;
> > +
> > + if (fsize->index >= ARRAY_SIZE(supported_modes))
> > return -EINVAL;
> >
> > - if (fsize->code != supported_mode.code)
> > + for (i = 0; i < ARRAY_SIZE(codes); i++) {
>
> Can we use for (unsigned int i = 0; ... ) now that C99 has been adopted ?
C99 might be adopted.
But I have not observered no one is started using.
>
> > + if (codes[i] == fsize->code)
> > + break;
> > return -EINVAL;
>
> Are you sure this is right ?
> If the format code in fsize->code is == codes[1] you will return -EINVAL ?
I think this will break the for loop, if fsize->code is == codes[1]
>
> Does the sensor support more formats than
> MEDIA_BUS_FMT_SRGGB10_1X10 and MEDIA_BUS_FMT_SRGGB12_1X12 ?
> Because otherwise you could replace the for loop with a simple check.
>
> However support for flip might change the bayer pattern, so it's better to
> keep the loop maybe.
>
> What about centrlize it like you're doing in imx219_get_format_code() and
> use it here ? You can place the helper just after the supported mbus codes
> declaration maybe ?
>
Yes, it is good. But imx334 do not have direct register to do VFLIP.
>
> > + }
> >
> > - 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;
> > @@ -553,7 +759,7 @@ 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.code = imx334->cur_code;
> > fmt->format.field = V4L2_FIELD_NONE;
> > fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> > fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; @@ -591,6
> > +797,18 @@ static int imx334_get_pad_format(struct v4l2_subdev *sd,
> > return 0;
> > }
> >
> > +static int imx219_get_format_code(struct imx334 *imx334, struct
> > +v4l2_subdev_format *fmt) {
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(codes); i++) {
>
> Ditto, unsigned int and possibily declared inside the for loop
>
> > + if (codes[i] == fmt->format.code)
> > + return codes[i];
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > /**
> > * imx334_set_pad_format() - Set subdevice pad format
> > * @sd: pointer to imx334 V4L2 sub-device structure @@ -606,10
> > +824,21 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
> > struct imx334 *imx334 = to_imx334(sd);
> > const struct imx334_mode *mode;
> > int ret = 0;
> > + u32 code;
> >
> > mutex_lock(&imx334->mutex);
> >
> > - mode = &supported_mode;
> > + code = imx219_get_format_code(imx334, fmt);
> > + if (code < 0)
> > + return -EINVAL;
> > +
> > + imx334->cur_code = code;
> > +
> > + mode = v4l2_find_nearest_size(supported_modes,
> > + ARRAY_SIZE(supported_modes),
> > + width, height,
> > + fmt->format.width,
> > + fmt->format.height);
> > +
> > imx334_fill_pad_format(imx334, mode, fmt);
> >
> > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { @@ -617,7 +846,7
> @@
> > static int imx334_set_pad_format(struct v4l2_subdev *sd,
> >
> > framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> > *framefmt = fmt->format;
> > - } else {
> > + } else if (imx334->cur_mode != mode) {
>
> Is this change related ? I think it's good as it avoids an unecessary update of
> the controls, but maybe it's not related to this patch ?
I have updated mode[] array new set of value,
Insated of calling functions simply.
Just added a check the the mode changed then only we need to updated other parameters
Same as imx219 code
Thanks,
Shravan
>
> Thanks
> j
>
> > ret = imx334_update_controls(imx334, mode);
> > if (!ret)
> > imx334->cur_mode = mode; @@ -642,11 +871,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[1], &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
> > @@ -667,6 +911,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) {
> > @@ -1037,7 +1288,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[1];
> > + imx334->cur_code = codes[1];
> > imx334->vblank = imx334->cur_mode->vblank;
> >
> > ret = imx334_init_controls(imx334);
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] media: i2c: imx334: support lower bandwidth mode
2022-08-25 7:16 ` Shravan.Chippa
@ 2022-08-25 7:29 ` Jacopo Mondi
2022-08-25 11:09 ` Shravan.Chippa
0 siblings, 1 reply; 8+ messages in thread
From: Jacopo Mondi @ 2022-08-25 7:29 UTC (permalink / raw)
To: Shravan.Chippa
Cc: paul.j.murphy, daniele.alessandrelli, mchehab, linux-media,
linux-kernel, Conor.Dooley, Prakash.Battu
Hi Shravan
On Thu, Aug 25, 2022 at 07:16:22AM +0000, Shravan.Chippa@microchip.com wrote:
>
>
> > -----Original Message-----
> > From: Jacopo Mondi <jacopo@jmondi.org>
> > Sent: 22 August 2022 09:23 PM
> > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > mchehab@kernel.org; linux-media@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Conor Dooley - M52691
> > <Conor.Dooley@microchip.com>; Battu Prakash Reddy - I30399
> > <Prakash.Battu@microchip.com>
> > Subject: Re: [PATCH] media: i2c: imx334: support lower bandwidth mode
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > Hello Shravan,
> >
> > On Thu, Jul 28, 2022 at 12:00:44PM +0530, shravan kumar wrote:
> > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > >
> > > Some platforms may not be capable of supporting the bandwidth required
> > > for 12 bit or 3840x2160 resolutions.
> > >
> > > Add support for dynamically selecting 10 bit and 1920x1080 resolutions
> > > while leaving the existing configuration as default
> > >
> > > CC: Conor Dooley <conor.dooley@microchip.com>
> > > Signed-off-by: Prakash Battu <Prakash.Battu@microchip.com>
> > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > ---
> > > drivers/media/i2c/imx334.c | 306
> > > +++++++++++++++++++++++++++++++++----
> > > 1 file changed, 279 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > index 062125501788..d1fa4c4d4d9e 100644
> > > --- a/drivers/media/i2c/imx334.c
> > > +++ b/drivers/media/i2c/imx334.c
> > > @@ -79,7 +79,6 @@ struct imx334_reg_list {
> > > * struct imx334_mode - imx334 sensor mode structure
> > > * @width: Frame width
> > > * @height: Frame height
> > > - * @code: Format code
> > > * @hblank: Horizontal blanking in lines
> > > * @vblank: Vertical blanking in lines
> > > * @vblank_min: Minimal vertical blanking in lines @@ -91,7 +90,6 @@
> > > struct imx334_reg_list { struct imx334_mode {
> > > u32 width;
> > > u32 height;
> > > - u32 code;
> > > u32 hblank;
> > > u32 vblank;
> > > u32 vblank_min;
> > > @@ -119,6 +117,7 @@ struct imx334_mode {
> > > * @vblank: Vertical blanking in lines
> > > * @cur_mode: Pointer to current selected sensor mode
> > > * @mutex: Mutex for serializing sensor controls
> > > + * @cur_code: current selected format code
> > > * @streaming: Flag indicating streaming state
> > > */
> > > struct imx334 {
> > > @@ -140,6 +139,7 @@ struct imx334 {
> > > u32 vblank;
> > > const struct imx334_mode *cur_mode;
> > > struct mutex mutex;
> > > + u32 cur_code;
> > > bool streaming;
> > > };
> > >
> > > @@ -147,6 +147,169 @@ static const s64 link_freq[] = {
> > > IMX334_LINK_FREQ,
> > > };
> > >
> > > +/* Sensor mode registers */
> > > +static const struct imx334_reg mode_1920x1080_regs[] = {
> > > + {0x3000, 0x01},
> > > + {0x3018, 0x04},
> > > + {0x3030, 0xCA},
> > > + {0x3031, 0x08},
> > > + {0x3032, 0x00},
> > > + {0x3034, 0x4C},
> > > + {0x3035, 0x04},
> > > + {0x302C, 0xF0},
> > > + {0x302D, 0x03},
> > > + {0x302E, 0x80},
> > > + {0x302F, 0x07},
> > > + {0x3074, 0xCC},
> > > + {0x3075, 0x02},
> > > + {0x308E, 0xCD},
> > > + {0x308F, 0x02},
> > > + {0x3076, 0x38},
> > > + {0x3077, 0x04},
> > > + {0x3090, 0x38},
> > > + {0x3091, 0x04},
> > > + {0x3308, 0x38},
> > > + {0x3309, 0x04},
> > > + {0x30C6, 0x00},
> > > + {0x30C7, 0x00},
> > > + {0x30CE, 0x00},
> > > + {0x30CF, 0x00},
> > > + {0x30D8, 0x18},
> > > + {0x30D9, 0x0A},
> > > + {0x304C, 0x00},
> > > + {0x304E, 0x00},
> > > + {0x304F, 0x00},
> > > + {0x3050, 0x00},
> > > + {0x30B6, 0x00},
> > > + {0x30B7, 0x00},
> > > + {0x3116, 0x08},
> > > + {0x3117, 0x00},
> > > + {0x31A0, 0x20},
> > > + {0x31A1, 0x0F},
> > > + {0x300C, 0x3B},
> > > + {0x300D, 0x29},
> > > + {0x314C, 0x29},
> > > + {0x314D, 0x01},
> > > + {0x315A, 0x0A},
> > > + {0x3168, 0xA0},
> > > + {0x316A, 0x7E},
> > > + {0x319E, 0x02},
> > > + {0x3199, 0x00},
> > > + {0x319D, 0x00},
> > > + {0x31DD, 0x03},
> > > + {0x3300, 0x00},
> > > + {0x341C, 0xFF},
> > > + {0x341D, 0x01},
> > > + {0x3A01, 0x03},
> > > + {0x3A18, 0x7F},
> > > + {0x3A19, 0x00},
> > > + {0x3A1A, 0x37},
> > > + {0x3A1B, 0x00},
> > > + {0x3A1C, 0x37},
> > > + {0x3A1D, 0x00},
> > > + {0x3A1E, 0xF7},
> > > + {0x3A1F, 0x00},
> > > + {0x3A20, 0x3F},
> > > + {0x3A21, 0x00},
> > > + {0x3A20, 0x6F},
> > > + {0x3A21, 0x00},
> > > + {0x3A20, 0x3F},
> > > + {0x3A21, 0x00},
> > > + {0x3A20, 0x5F},
> > > + {0x3A21, 0x00},
> > > + {0x3A20, 0x2F},
> > > + {0x3A21, 0x00},
> > > + {0x3078, 0x02},
> > > + {0x3079, 0x00},
> > > + {0x307A, 0x00},
> > > + {0x307B, 0x00},
> > > + {0x3080, 0x02},
> > > + {0x3081, 0x00},
> > > + {0x3082, 0x00},
> > > + {0x3083, 0x00},
> > > + {0x3088, 0x02},
> > > + {0x3094, 0x00},
> > > + {0x3095, 0x00},
> > > + {0x3096, 0x00},
> > > + {0x309B, 0x02},
> > > + {0x309C, 0x00},
> > > + {0x309D, 0x00},
> > > + {0x309E, 0x00},
> > > + {0x30A4, 0x00},
> > > + {0x30A5, 0x00},
> > > + {0x3288, 0x21},
> > > + {0x328A, 0x02},
> > > + {0x3414, 0x05},
> > > + {0x3416, 0x18},
> > > + {0x35AC, 0x0E},
> > > + {0x3648, 0x01},
> > > + {0x364A, 0x04},
> > > + {0x364C, 0x04},
> > > + {0x3678, 0x01},
> > > + {0x367C, 0x31},
> > > + {0x367E, 0x31},
> > > + {0x3708, 0x02},
> > > + {0x3714, 0x01},
> > > + {0x3715, 0x02},
> > > + {0x3716, 0x02},
> > > + {0x3717, 0x02},
> > > + {0x371C, 0x3D},
> > > + {0x371D, 0x3F},
> > > + {0x372C, 0x00},
> > > + {0x372D, 0x00},
> > > + {0x372E, 0x46},
> > > + {0x372F, 0x00},
> > > + {0x3730, 0x89},
> > > + {0x3731, 0x00},
> > > + {0x3732, 0x08},
> > > + {0x3733, 0x01},
> > > + {0x3734, 0xFE},
> > > + {0x3735, 0x05},
> > > + {0x375D, 0x00},
> > > + {0x375E, 0x00},
> > > + {0x375F, 0x61},
> > > + {0x3760, 0x06},
> > > + {0x3768, 0x1B},
> > > + {0x3769, 0x1B},
> > > + {0x376A, 0x1A},
> > > + {0x376B, 0x19},
> > > + {0x376C, 0x18},
> > > + {0x376D, 0x14},
> > > + {0x376E, 0x0F},
> > > + {0x3776, 0x00},
> > > + {0x3777, 0x00},
> > > + {0x3778, 0x46},
> > > + {0x3779, 0x00},
> > > + {0x377A, 0x08},
> > > + {0x377B, 0x01},
> > > + {0x377C, 0x45},
> > > + {0x377D, 0x01},
> > > + {0x377E, 0x23},
> > > + {0x377F, 0x02},
> > > + {0x3780, 0xD9},
> > > + {0x3781, 0x03},
> > > + {0x3782, 0xF5},
> > > + {0x3783, 0x06},
> > > + {0x3784, 0xA5},
> > > + {0x3788, 0x0F},
> > > + {0x378A, 0xD9},
> > > + {0x378B, 0x03},
> > > + {0x378C, 0xEB},
> > > + {0x378D, 0x05},
> > > + {0x378E, 0x87},
> > > + {0x378F, 0x06},
> > > + {0x3790, 0xF5},
> > > + {0x3792, 0x43},
> > > + {0x3794, 0x7A},
> > > + {0x3796, 0xA1},
> > > + {0x37B0, 0x37},
> > > + {0x3E04, 0x0E},
> > > + {0x30E8, 0x50},
> > > + {0x30E9, 0x00},
> > > + {0x3E04, 0x0E},
> > > + {0x3002, 0x00},
> > > +};
> > > +
> > > /* Sensor mode registers */
> > > static const struct imx334_reg mode_3840x2160_regs[] = {
> > > {0x3000, 0x01},
> > > @@ -243,20 +406,57 @@ static const struct imx334_reg
> > mode_3840x2160_regs[] = {
> > > {0x3a00, 0x01},
> > > };
> > >
> > > +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 codes[] = {
> >
> > Just "codes" is a little vague ? What about imx334_mbus_codes[] ?
> >
> > > + MEDIA_BUS_FMT_SRGGB10_1X10,
> > > + MEDIA_BUS_FMT_SRGGB12_1X12,
> > > +};
> > > +
> > > /* 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 = 1920,
> > > + .height = 1080,
> > > + .hblank = 280,
> > > + .vblank = 1170,
> > > + .vblank_min = 90,
> > > + .vblank_max = 132840,
> > > + .pclk = 594000000,
> > > + .link_freq_idx = 0,
> > > + .reg_list = {
> > > + .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > > + .regs = mode_1920x1080_regs,
> > > + },
> > > + },
> > > + {
> > > + .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,
> > > + },
> >
> > Nit: I would keep the existing formats and code in position [0] so that if
> > additional modes are added the defaults can be kept in [0]. Also I usually see
> > drivers listing larger modes first, but that might not be a strict rule.
> >
> > > },
> > > };
> > >
> > > @@ -382,7 +582,8 @@ static int imx334_update_controls(struct imx334
> > *imx334,
> > > if (ret)
> > > return ret;
> > >
> > > - ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
> > > + ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl,
> > IMX334_REG_MIN,
> > > + IMX334_REG_MAX, 1, mode->hblank);
> >
> > I might have missed why is this change related
>
> Hblank is readonly variable.
> If I use __v4l2_ctrl_s_ctrl function update return error.
So it's a separate fix unrelated to the introduction of the new mode ?
Probably it never triggered before as we had a single mode so the
control was actually never updated, but it's a fix worth a patch of
his own maybe.
I would make a separate patch before this one or clearly mention in the
commit message that also this is changed and why.
> >
> > > if (ret)
> > > return ret;
> > >
> > > @@ -506,10 +707,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(codes))
> > > return -EINVAL;
> > >
> > > - code->code = supported_mode.code;
> > > + code->code = codes[code->index];
> > >
> > > return 0;
> > > }
> > > @@ -526,15 +727,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)
> > > + int i;
> > > +
> > > + if (fsize->index >= ARRAY_SIZE(supported_modes))
> > > return -EINVAL;
> > >
> > > - if (fsize->code != supported_mode.code)
> > > + for (i = 0; i < ARRAY_SIZE(codes); i++) {
> >
> > Can we use for (unsigned int i = 0; ... ) now that C99 has been adopted ?
>
> C99 might be adopted.
> But I have not observered no one is started using.
>
$ git grep -e "for (unsigned " -e "for (int " v6.0-rc2 | wc -l
186
> >
> > > + if (codes[i] == fsize->code)
> > > + break;
> > > return -EINVAL;
> >
> > Are you sure this is right ?
> > If the format code in fsize->code is == codes[1] you will return -EINVAL ?
>
> I think this will break the for loop, if fsize->code is == codes[1]
Yes but before getting to codes[1] you would test codes[0] and if
fsize->code != codes[0] you will return -EINVAL
Am I reading this wrong ?
>
> >
> > Does the sensor support more formats than
> > MEDIA_BUS_FMT_SRGGB10_1X10 and MEDIA_BUS_FMT_SRGGB12_1X12 ?
> > Because otherwise you could replace the for loop with a simple check.
> >
> > However support for flip might change the bayer pattern, so it's better to
> > keep the loop maybe.
> >
> > What about centrlize it like you're doing in imx219_get_format_code() and
> > use it here ? You can place the helper just after the supported mbus codes
> > declaration maybe ?
> >
> Yes, it is good. But imx334 do not have direct register to do VFLIP.
>
> >
> > > + }
> > >
> > > - 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;
> > > @@ -553,7 +759,7 @@ 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.code = imx334->cur_code;
> > > fmt->format.field = V4L2_FIELD_NONE;
> > > fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> > > fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; @@ -591,6
> > > +797,18 @@ static int imx334_get_pad_format(struct v4l2_subdev *sd,
> > > return 0;
> > > }
> > >
> > > +static int imx219_get_format_code(struct imx334 *imx334, struct
> > > +v4l2_subdev_format *fmt) {
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(codes); i++) {
> >
> > Ditto, unsigned int and possibily declared inside the for loop
> >
> > > + if (codes[i] == fmt->format.code)
> > > + return codes[i];
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > /**
> > > * imx334_set_pad_format() - Set subdevice pad format
> > > * @sd: pointer to imx334 V4L2 sub-device structure @@ -606,10
> > > +824,21 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
> > > struct imx334 *imx334 = to_imx334(sd);
> > > const struct imx334_mode *mode;
> > > int ret = 0;
> > > + u32 code;
> > >
> > > mutex_lock(&imx334->mutex);
> > >
> > > - mode = &supported_mode;
> > > + code = imx219_get_format_code(imx334, fmt);
> > > + if (code < 0)
> > > + return -EINVAL;
> > > +
> > > + imx334->cur_code = code;
> > > +
> > > + mode = v4l2_find_nearest_size(supported_modes,
> > > + ARRAY_SIZE(supported_modes),
> > > + width, height,
> > > + fmt->format.width,
> > > + fmt->format.height);
> > > +
> > > imx334_fill_pad_format(imx334, mode, fmt);
> > >
> > > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { @@ -617,7 +846,7
> > @@
> > > static int imx334_set_pad_format(struct v4l2_subdev *sd,
> > >
> > > framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> > > *framefmt = fmt->format;
> > > - } else {
> > > + } else if (imx334->cur_mode != mode) {
> >
> > Is this change related ? I think it's good as it avoids an unecessary update of
> > the controls, but maybe it's not related to this patch ?
>
> I have updated mode[] array new set of value,
> Insated of calling functions simply.
> Just added a check the the mode changed then only we need to updated other parameters
> Same as imx219 code
You're right, we had a single mode before this change :)
>
> Thanks,
> Shravan
>
> >
> > Thanks
> > j
> >
> > > ret = imx334_update_controls(imx334, mode);
> > > if (!ret)
> > > imx334->cur_mode = mode; @@ -642,11 +871,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[1], &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
> > > @@ -667,6 +911,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) {
> > > @@ -1037,7 +1288,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[1];
> > > + imx334->cur_code = codes[1];
> > > imx334->vblank = imx334->cur_mode->vblank;
> > >
> > > ret = imx334_init_controls(imx334);
> > > --
> > > 2.17.1
> > >
^ permalink raw reply [flat|nested] 8+ messages in thread* RE: [PATCH] media: i2c: imx334: support lower bandwidth mode
2022-08-25 7:29 ` Jacopo Mondi
@ 2022-08-25 11:09 ` Shravan.Chippa
2022-08-25 11:58 ` Jacopo Mondi
0 siblings, 1 reply; 8+ messages in thread
From: Shravan.Chippa @ 2022-08-25 11:09 UTC (permalink / raw)
To: jacopo
Cc: paul.j.murphy, daniele.alessandrelli, mchehab, linux-media,
linux-kernel, Conor.Dooley, Prakash.Battu
> -----Original Message-----
> From: Jacopo Mondi <jacopo@jmondi.org>
> Sent: 25 August 2022 01:00 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; linux-media@vger.kernel.org; linux-
> kernel@vger.kernel.org; Conor Dooley - M52691
> <Conor.Dooley@microchip.com>; Battu Prakash Reddy - I30399
> <Prakash.Battu@microchip.com>
> Subject: Re: [PATCH] media: i2c: imx334: support lower bandwidth mode
>
> [You don't often get email from jacopo@jmondi.org. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> Hi Shravan
>
> On Thu, Aug 25, 2022 at 07:16:22AM +0000, Shravan.Chippa@microchip.com
> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Jacopo Mondi <jacopo@jmondi.org>
> > > Sent: 22 August 2022 09:23 PM
> > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > > mchehab@kernel.org; linux-media@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; Conor Dooley - M52691
> > > <Conor.Dooley@microchip.com>; Battu Prakash Reddy - I30399
> > > <Prakash.Battu@microchip.com>
> > > Subject: Re: [PATCH] media: i2c: imx334: support lower bandwidth
> > > mode
> > >
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > >
> > > Hello Shravan,
> > >
> > > On Thu, Jul 28, 2022 at 12:00:44PM +0530, shravan kumar wrote:
> > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > >
> > > > Some platforms may not be capable of supporting the bandwidth
> > > > required for 12 bit or 3840x2160 resolutions.
> > > >
> > > > Add support for dynamically selecting 10 bit and 1920x1080
> > > > resolutions while leaving the existing configuration as default
> > > >
> > > > CC: Conor Dooley <conor.dooley@microchip.com>
> > > > Signed-off-by: Prakash Battu <Prakash.Battu@microchip.com>
> > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > ---
> > > > drivers/media/i2c/imx334.c | 306
> > > > +++++++++++++++++++++++++++++++++----
> > > > 1 file changed, 279 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx334.c
> > > > b/drivers/media/i2c/imx334.c index 062125501788..d1fa4c4d4d9e
> > > > 100644
> > > > --- a/drivers/media/i2c/imx334.c
> > > > +++ b/drivers/media/i2c/imx334.c
> > > > @@ -79,7 +79,6 @@ struct imx334_reg_list {
> > > > * struct imx334_mode - imx334 sensor mode structure
> > > > * @width: Frame width
> > > > * @height: Frame height
> > > > - * @code: Format code
> > > > * @hblank: Horizontal blanking in lines
> > > > * @vblank: Vertical blanking in lines
> > > > * @vblank_min: Minimal vertical blanking in lines @@ -91,7 +90,6
> > > > @@ struct imx334_reg_list { struct imx334_mode {
> > > > u32 width;
> > > > u32 height;
> > > > - u32 code;
> > > > u32 hblank;
> > > > u32 vblank;
> > > > u32 vblank_min;
> > > > @@ -119,6 +117,7 @@ struct imx334_mode {
> > > > * @vblank: Vertical blanking in lines
> > > > * @cur_mode: Pointer to current selected sensor mode
> > > > * @mutex: Mutex for serializing sensor controls
> > > > + * @cur_code: current selected format code
> > > > * @streaming: Flag indicating streaming state
> > > > */
> > > > struct imx334 {
> > > > @@ -140,6 +139,7 @@ struct imx334 {
> > > > u32 vblank;
> > > > const struct imx334_mode *cur_mode;
> > > > struct mutex mutex;
> > > > + u32 cur_code;
> > > > bool streaming;
> > > > };
> > > >
> > > > @@ -147,6 +147,169 @@ static const s64 link_freq[] = {
> > > > IMX334_LINK_FREQ,
> > > > };
> > > >
> > > > +/* Sensor mode registers */
> > > > +static const struct imx334_reg mode_1920x1080_regs[] = {
> > > > + {0x3000, 0x01},
> > > > + {0x3018, 0x04},
> > > > + {0x3030, 0xCA},
> > > > + {0x3031, 0x08},
> > > > + {0x3032, 0x00},
> > > > + {0x3034, 0x4C},
> > > > + {0x3035, 0x04},
> > > > + {0x302C, 0xF0},
> > > > + {0x302D, 0x03},
> > > > + {0x302E, 0x80},
> > > > + {0x302F, 0x07},
> > > > + {0x3074, 0xCC},
> > > > + {0x3075, 0x02},
> > > > + {0x308E, 0xCD},
> > > > + {0x308F, 0x02},
> > > > + {0x3076, 0x38},
> > > > + {0x3077, 0x04},
> > > > + {0x3090, 0x38},
> > > > + {0x3091, 0x04},
> > > > + {0x3308, 0x38},
> > > > + {0x3309, 0x04},
> > > > + {0x30C6, 0x00},
> > > > + {0x30C7, 0x00},
> > > > + {0x30CE, 0x00},
> > > > + {0x30CF, 0x00},
> > > > + {0x30D8, 0x18},
> > > > + {0x30D9, 0x0A},
> > > > + {0x304C, 0x00},
> > > > + {0x304E, 0x00},
> > > > + {0x304F, 0x00},
> > > > + {0x3050, 0x00},
> > > > + {0x30B6, 0x00},
> > > > + {0x30B7, 0x00},
> > > > + {0x3116, 0x08},
> > > > + {0x3117, 0x00},
> > > > + {0x31A0, 0x20},
> > > > + {0x31A1, 0x0F},
> > > > + {0x300C, 0x3B},
> > > > + {0x300D, 0x29},
> > > > + {0x314C, 0x29},
> > > > + {0x314D, 0x01},
> > > > + {0x315A, 0x0A},
> > > > + {0x3168, 0xA0},
> > > > + {0x316A, 0x7E},
> > > > + {0x319E, 0x02},
> > > > + {0x3199, 0x00},
> > > > + {0x319D, 0x00},
> > > > + {0x31DD, 0x03},
> > > > + {0x3300, 0x00},
> > > > + {0x341C, 0xFF},
> > > > + {0x341D, 0x01},
> > > > + {0x3A01, 0x03},
> > > > + {0x3A18, 0x7F},
> > > > + {0x3A19, 0x00},
> > > > + {0x3A1A, 0x37},
> > > > + {0x3A1B, 0x00},
> > > > + {0x3A1C, 0x37},
> > > > + {0x3A1D, 0x00},
> > > > + {0x3A1E, 0xF7},
> > > > + {0x3A1F, 0x00},
> > > > + {0x3A20, 0x3F},
> > > > + {0x3A21, 0x00},
> > > > + {0x3A20, 0x6F},
> > > > + {0x3A21, 0x00},
> > > > + {0x3A20, 0x3F},
> > > > + {0x3A21, 0x00},
> > > > + {0x3A20, 0x5F},
> > > > + {0x3A21, 0x00},
> > > > + {0x3A20, 0x2F},
> > > > + {0x3A21, 0x00},
> > > > + {0x3078, 0x02},
> > > > + {0x3079, 0x00},
> > > > + {0x307A, 0x00},
> > > > + {0x307B, 0x00},
> > > > + {0x3080, 0x02},
> > > > + {0x3081, 0x00},
> > > > + {0x3082, 0x00},
> > > > + {0x3083, 0x00},
> > > > + {0x3088, 0x02},
> > > > + {0x3094, 0x00},
> > > > + {0x3095, 0x00},
> > > > + {0x3096, 0x00},
> > > > + {0x309B, 0x02},
> > > > + {0x309C, 0x00},
> > > > + {0x309D, 0x00},
> > > > + {0x309E, 0x00},
> > > > + {0x30A4, 0x00},
> > > > + {0x30A5, 0x00},
> > > > + {0x3288, 0x21},
> > > > + {0x328A, 0x02},
> > > > + {0x3414, 0x05},
> > > > + {0x3416, 0x18},
> > > > + {0x35AC, 0x0E},
> > > > + {0x3648, 0x01},
> > > > + {0x364A, 0x04},
> > > > + {0x364C, 0x04},
> > > > + {0x3678, 0x01},
> > > > + {0x367C, 0x31},
> > > > + {0x367E, 0x31},
> > > > + {0x3708, 0x02},
> > > > + {0x3714, 0x01},
> > > > + {0x3715, 0x02},
> > > > + {0x3716, 0x02},
> > > > + {0x3717, 0x02},
> > > > + {0x371C, 0x3D},
> > > > + {0x371D, 0x3F},
> > > > + {0x372C, 0x00},
> > > > + {0x372D, 0x00},
> > > > + {0x372E, 0x46},
> > > > + {0x372F, 0x00},
> > > > + {0x3730, 0x89},
> > > > + {0x3731, 0x00},
> > > > + {0x3732, 0x08},
> > > > + {0x3733, 0x01},
> > > > + {0x3734, 0xFE},
> > > > + {0x3735, 0x05},
> > > > + {0x375D, 0x00},
> > > > + {0x375E, 0x00},
> > > > + {0x375F, 0x61},
> > > > + {0x3760, 0x06},
> > > > + {0x3768, 0x1B},
> > > > + {0x3769, 0x1B},
> > > > + {0x376A, 0x1A},
> > > > + {0x376B, 0x19},
> > > > + {0x376C, 0x18},
> > > > + {0x376D, 0x14},
> > > > + {0x376E, 0x0F},
> > > > + {0x3776, 0x00},
> > > > + {0x3777, 0x00},
> > > > + {0x3778, 0x46},
> > > > + {0x3779, 0x00},
> > > > + {0x377A, 0x08},
> > > > + {0x377B, 0x01},
> > > > + {0x377C, 0x45},
> > > > + {0x377D, 0x01},
> > > > + {0x377E, 0x23},
> > > > + {0x377F, 0x02},
> > > > + {0x3780, 0xD9},
> > > > + {0x3781, 0x03},
> > > > + {0x3782, 0xF5},
> > > > + {0x3783, 0x06},
> > > > + {0x3784, 0xA5},
> > > > + {0x3788, 0x0F},
> > > > + {0x378A, 0xD9},
> > > > + {0x378B, 0x03},
> > > > + {0x378C, 0xEB},
> > > > + {0x378D, 0x05},
> > > > + {0x378E, 0x87},
> > > > + {0x378F, 0x06},
> > > > + {0x3790, 0xF5},
> > > > + {0x3792, 0x43},
> > > > + {0x3794, 0x7A},
> > > > + {0x3796, 0xA1},
> > > > + {0x37B0, 0x37},
> > > > + {0x3E04, 0x0E},
> > > > + {0x30E8, 0x50},
> > > > + {0x30E9, 0x00},
> > > > + {0x3E04, 0x0E},
> > > > + {0x3002, 0x00},
> > > > +};
> > > > +
> > > > /* Sensor mode registers */
> > > > static const struct imx334_reg mode_3840x2160_regs[] = {
> > > > {0x3000, 0x01},
> > > > @@ -243,20 +406,57 @@ static const struct imx334_reg
> > > mode_3840x2160_regs[] = {
> > > > {0x3a00, 0x01},
> > > > };
> > > >
> > > > +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 codes[] = {
> > >
> > > Just "codes" is a little vague ? What about imx334_mbus_codes[] ?
> > >
> > > > + MEDIA_BUS_FMT_SRGGB10_1X10,
> > > > + MEDIA_BUS_FMT_SRGGB12_1X12,
> > > > +};
> > > > +
> > > > /* 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 = 1920,
> > > > + .height = 1080,
> > > > + .hblank = 280,
> > > > + .vblank = 1170,
> > > > + .vblank_min = 90,
> > > > + .vblank_max = 132840,
> > > > + .pclk = 594000000,
> > > > + .link_freq_idx = 0,
> > > > + .reg_list = {
> > > > + .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > > > + .regs = mode_1920x1080_regs,
> > > > + },
> > > > + },
> > > > + {
> > > > + .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,
> > > > + },
> > >
> > > Nit: I would keep the existing formats and code in position [0] so
> > > that if additional modes are added the defaults can be kept in [0].
> > > Also I usually see drivers listing larger modes first, but that might not be a
> strict rule.
> > >
> > > > },
> > > > };
> > > >
> > > > @@ -382,7 +582,8 @@ static int imx334_update_controls(struct
> > > > imx334
> > > *imx334,
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
> > > > + ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl,
> > > IMX334_REG_MIN,
> > > > + IMX334_REG_MAX, 1,
> > > > + mode->hblank);
> > >
> > > I might have missed why is this change related
> >
> > Hblank is readonly variable.
> > If I use __v4l2_ctrl_s_ctrl function update return error.
>
> So it's a separate fix unrelated to the introduction of the new mode ?
>
> Probably it never triggered before as we had a single mode so the control
> was actually never updated, but it's a fix worth a patch of his own maybe.
>
> I would make a separate patch before this one or clearly mention in the
> commit message that also this is changed and why.
>
> > >
> > > > if (ret)
> > > > return ret;
> > > >
> > > > @@ -506,10 +707,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(codes))
> > > > return -EINVAL;
> > > >
> > > > - code->code = supported_mode.code;
> > > > + code->code = codes[code->index];
> > > >
> > > > return 0;
> > > > }
> > > > @@ -526,15 +727,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)
> > > > + int i;
> > > > +
> > > > + if (fsize->index >= ARRAY_SIZE(supported_modes))
> > > > return -EINVAL;
> > > >
> > > > - if (fsize->code != supported_mode.code)
> > > > + for (i = 0; i < ARRAY_SIZE(codes); i++) {
> > >
> > > Can we use for (unsigned int i = 0; ... ) now that C99 has been adopted ?
> >
> > C99 might be adopted.
> > But I have not observered no one is started using.
> >
>
> $ git grep -e "for (unsigned " -e "for (int " v6.0-rc2 | wc -l
> 186
>
> > >
> > > > + if (codes[i] == fsize->code)
> > > > + break;
> > > > return -EINVAL;
> > >
> > > Are you sure this is right ?
> > > If the format code in fsize->code is == codes[1] you will return -EINVAL ?
> >
> > I think this will break the for loop, if fsize->code is == codes[1]
>
> Yes but before getting to codes[1] you would test codes[0] and if
> fsize->code != codes[0] you will return -EINVAL
>
> Am I reading this wrong ?
>
Yes, I am wrong
If (codes[0] != fsize->code && codes[1] != fsize->code )
return -EINVAL
> > >
> > > Does the sensor support more formats than
> > > MEDIA_BUS_FMT_SRGGB10_1X10 and
> MEDIA_BUS_FMT_SRGGB12_1X12 ?
> > > Because otherwise you could replace the for loop with a simple check.
> > >
> > > However support for flip might change the bayer pattern, so it's
> > > better to keep the loop maybe.
> > >
> > > What about centrlize it like you're doing in
> > > imx219_get_format_code() and use it here ? You can place the helper
> > > just after the supported mbus codes declaration maybe ?
> > >
> > Yes, it is good. But imx334 do not have direct register to do VFLIP.
> >
> > >
> > > > + }
> > > >
> > > > - 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;
> > > > @@ -553,7 +759,7 @@ 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.code = imx334->cur_code;
> > > > fmt->format.field = V4L2_FIELD_NONE;
> > > > fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> > > > fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; @@ -591,6
> > > > +797,18 @@ static int imx334_get_pad_format(struct v4l2_subdev
> > > > +*sd,
> > > > return 0;
> > > > }
> > > >
> > > > +static int imx219_get_format_code(struct imx334 *imx334, struct
> > > > +v4l2_subdev_format *fmt) {
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(codes); i++) {
> > >
> > > Ditto, unsigned int and possibily declared inside the for loop
> > >
> > > > + if (codes[i] == fmt->format.code)
> > > > + return codes[i];
> > > > + }
> > > > +
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > /**
> > > > * imx334_set_pad_format() - Set subdevice pad format
> > > > * @sd: pointer to imx334 V4L2 sub-device structure @@ -606,10
> > > > +824,21 @@ static int imx334_set_pad_format(struct v4l2_subdev
> > > > +*sd,
> > > > struct imx334 *imx334 = to_imx334(sd);
> > > > const struct imx334_mode *mode;
> > > > int ret = 0;
> > > > + u32 code;
> > > >
> > > > mutex_lock(&imx334->mutex);
> > > >
> > > > - mode = &supported_mode;
> > > > + code = imx219_get_format_code(imx334, fmt);
> > > > + if (code < 0)
> > > > + return -EINVAL;
> > > > +
> > > > + imx334->cur_code = code;
> > > > +
> > > > + mode = v4l2_find_nearest_size(supported_modes,
> > > > + ARRAY_SIZE(supported_modes),
> > > > + width, height,
> > > > + fmt->format.width,
> > > > + fmt->format.height);
> > > > +
> > > > imx334_fill_pad_format(imx334, mode, fmt);
> > > >
> > > > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { @@ -617,7 +846,7
> > > @@
> > > > static int imx334_set_pad_format(struct v4l2_subdev *sd,
> > > >
> > > > framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt-
> >pad);
> > > > *framefmt = fmt->format;
> > > > - } else {
> > > > + } else if (imx334->cur_mode != mode) {
> > >
> > > Is this change related ? I think it's good as it avoids an
> > > unecessary update of the controls, but maybe it's not related to this
> patch ?
> >
> > I have updated mode[] array new set of value, Insated of calling
> > functions simply.
> > Just added a check the the mode changed then only we need to updated
> > other parameters Same as imx219 code
>
> You're right, we had a single mode before this change :)
>
> >
> > Thanks,
> > Shravan
> >
> > >
> > > Thanks
> > > j
> > >
> > > > ret = imx334_update_controls(imx334, mode);
> > > > if (!ret)
> > > > imx334->cur_mode = mode; @@ -642,11 +871,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[1], &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 @@ -667,6 +911,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) {
> > > > @@ -1037,7 +1288,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[1];
> > > > + imx334->cur_code = codes[1];
> > > > imx334->vblank = imx334->cur_mode->vblank;
> > > >
> > > > ret = imx334_init_controls(imx334);
> > > > --
> > > > 2.17.1
> > > >
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] media: i2c: imx334: support lower bandwidth mode
2022-08-25 11:09 ` Shravan.Chippa
@ 2022-08-25 11:58 ` Jacopo Mondi
0 siblings, 0 replies; 8+ messages in thread
From: Jacopo Mondi @ 2022-08-25 11:58 UTC (permalink / raw)
To: Shravan.Chippa
Cc: paul.j.murphy, daniele.alessandrelli, mchehab, linux-media,
linux-kernel, Conor.Dooley, Prakash.Battu
Hi Shravan,
On Thu, Aug 25, 2022 at 11:09:03AM +0000, Shravan.Chippa@microchip.com wrote:
>
>
> > -----Original Message-----
> > From: Jacopo Mondi <jacopo@jmondi.org>
> > Sent: 25 August 2022 01:00 PM
> > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > mchehab@kernel.org; linux-media@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Conor Dooley - M52691
> > <Conor.Dooley@microchip.com>; Battu Prakash Reddy - I30399
> > <Prakash.Battu@microchip.com>
> > Subject: Re: [PATCH] media: i2c: imx334: support lower bandwidth mode
> >
> > [You don't often get email from jacopo@jmondi.org. Learn why this is
> > important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > Hi Shravan
> >
> > On Thu, Aug 25, 2022 at 07:16:22AM +0000, Shravan.Chippa@microchip.com
> > wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jacopo Mondi <jacopo@jmondi.org>
> > > > Sent: 22 August 2022 09:23 PM
> > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > > > mchehab@kernel.org; linux-media@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org; Conor Dooley - M52691
> > > > <Conor.Dooley@microchip.com>; Battu Prakash Reddy - I30399
> > > > <Prakash.Battu@microchip.com>
> > > > Subject: Re: [PATCH] media: i2c: imx334: support lower bandwidth
> > > > mode
> > > >
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > > know the content is safe
> > > >
> > > > Hello Shravan,
> > > >
> > > > On Thu, Jul 28, 2022 at 12:00:44PM +0530, shravan kumar wrote:
> > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > >
> > > > > Some platforms may not be capable of supporting the bandwidth
> > > > > required for 12 bit or 3840x2160 resolutions.
> > > > >
> > > > > Add support for dynamically selecting 10 bit and 1920x1080
> > > > > resolutions while leaving the existing configuration as default
> > > > >
> > > > > CC: Conor Dooley <conor.dooley@microchip.com>
> > > > > Signed-off-by: Prakash Battu <Prakash.Battu@microchip.com>
> > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > ---
> > > > > drivers/media/i2c/imx334.c | 306
> > > > > +++++++++++++++++++++++++++++++++----
> > > > > 1 file changed, 279 insertions(+), 27 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx334.c
> > > > > b/drivers/media/i2c/imx334.c index 062125501788..d1fa4c4d4d9e
> > > > > 100644
> > > > > --- a/drivers/media/i2c/imx334.c
> > > > > +++ b/drivers/media/i2c/imx334.c
> > > > > @@ -79,7 +79,6 @@ struct imx334_reg_list {
> > > > > * struct imx334_mode - imx334 sensor mode structure
> > > > > * @width: Frame width
> > > > > * @height: Frame height
> > > > > - * @code: Format code
> > > > > * @hblank: Horizontal blanking in lines
> > > > > * @vblank: Vertical blanking in lines
> > > > > * @vblank_min: Minimal vertical blanking in lines @@ -91,7 +90,6
> > > > > @@ struct imx334_reg_list { struct imx334_mode {
> > > > > u32 width;
> > > > > u32 height;
> > > > > - u32 code;
> > > > > u32 hblank;
> > > > > u32 vblank;
> > > > > u32 vblank_min;
> > > > > @@ -119,6 +117,7 @@ struct imx334_mode {
> > > > > * @vblank: Vertical blanking in lines
> > > > > * @cur_mode: Pointer to current selected sensor mode
> > > > > * @mutex: Mutex for serializing sensor controls
> > > > > + * @cur_code: current selected format code
> > > > > * @streaming: Flag indicating streaming state
> > > > > */
> > > > > struct imx334 {
> > > > > @@ -140,6 +139,7 @@ struct imx334 {
> > > > > u32 vblank;
> > > > > const struct imx334_mode *cur_mode;
> > > > > struct mutex mutex;
> > > > > + u32 cur_code;
> > > > > bool streaming;
> > > > > };
> > > > >
> > > > > @@ -147,6 +147,169 @@ static const s64 link_freq[] = {
> > > > > IMX334_LINK_FREQ,
> > > > > };
> > > > >
> > > > > +/* Sensor mode registers */
> > > > > +static const struct imx334_reg mode_1920x1080_regs[] = {
> > > > > + {0x3000, 0x01},
> > > > > + {0x3018, 0x04},
> > > > > + {0x3030, 0xCA},
> > > > > + {0x3031, 0x08},
> > > > > + {0x3032, 0x00},
> > > > > + {0x3034, 0x4C},
> > > > > + {0x3035, 0x04},
> > > > > + {0x302C, 0xF0},
> > > > > + {0x302D, 0x03},
> > > > > + {0x302E, 0x80},
> > > > > + {0x302F, 0x07},
> > > > > + {0x3074, 0xCC},
> > > > > + {0x3075, 0x02},
> > > > > + {0x308E, 0xCD},
> > > > > + {0x308F, 0x02},
> > > > > + {0x3076, 0x38},
> > > > > + {0x3077, 0x04},
> > > > > + {0x3090, 0x38},
> > > > > + {0x3091, 0x04},
> > > > > + {0x3308, 0x38},
> > > > > + {0x3309, 0x04},
> > > > > + {0x30C6, 0x00},
> > > > > + {0x30C7, 0x00},
> > > > > + {0x30CE, 0x00},
> > > > > + {0x30CF, 0x00},
> > > > > + {0x30D8, 0x18},
> > > > > + {0x30D9, 0x0A},
> > > > > + {0x304C, 0x00},
> > > > > + {0x304E, 0x00},
> > > > > + {0x304F, 0x00},
> > > > > + {0x3050, 0x00},
> > > > > + {0x30B6, 0x00},
> > > > > + {0x30B7, 0x00},
> > > > > + {0x3116, 0x08},
> > > > > + {0x3117, 0x00},
> > > > > + {0x31A0, 0x20},
> > > > > + {0x31A1, 0x0F},
> > > > > + {0x300C, 0x3B},
> > > > > + {0x300D, 0x29},
> > > > > + {0x314C, 0x29},
> > > > > + {0x314D, 0x01},
> > > > > + {0x315A, 0x0A},
> > > > > + {0x3168, 0xA0},
> > > > > + {0x316A, 0x7E},
> > > > > + {0x319E, 0x02},
> > > > > + {0x3199, 0x00},
> > > > > + {0x319D, 0x00},
> > > > > + {0x31DD, 0x03},
> > > > > + {0x3300, 0x00},
> > > > > + {0x341C, 0xFF},
> > > > > + {0x341D, 0x01},
> > > > > + {0x3A01, 0x03},
> > > > > + {0x3A18, 0x7F},
> > > > > + {0x3A19, 0x00},
> > > > > + {0x3A1A, 0x37},
> > > > > + {0x3A1B, 0x00},
> > > > > + {0x3A1C, 0x37},
> > > > > + {0x3A1D, 0x00},
> > > > > + {0x3A1E, 0xF7},
> > > > > + {0x3A1F, 0x00},
> > > > > + {0x3A20, 0x3F},
> > > > > + {0x3A21, 0x00},
> > > > > + {0x3A20, 0x6F},
> > > > > + {0x3A21, 0x00},
> > > > > + {0x3A20, 0x3F},
> > > > > + {0x3A21, 0x00},
> > > > > + {0x3A20, 0x5F},
> > > > > + {0x3A21, 0x00},
> > > > > + {0x3A20, 0x2F},
> > > > > + {0x3A21, 0x00},
> > > > > + {0x3078, 0x02},
> > > > > + {0x3079, 0x00},
> > > > > + {0x307A, 0x00},
> > > > > + {0x307B, 0x00},
> > > > > + {0x3080, 0x02},
> > > > > + {0x3081, 0x00},
> > > > > + {0x3082, 0x00},
> > > > > + {0x3083, 0x00},
> > > > > + {0x3088, 0x02},
> > > > > + {0x3094, 0x00},
> > > > > + {0x3095, 0x00},
> > > > > + {0x3096, 0x00},
> > > > > + {0x309B, 0x02},
> > > > > + {0x309C, 0x00},
> > > > > + {0x309D, 0x00},
> > > > > + {0x309E, 0x00},
> > > > > + {0x30A4, 0x00},
> > > > > + {0x30A5, 0x00},
> > > > > + {0x3288, 0x21},
> > > > > + {0x328A, 0x02},
> > > > > + {0x3414, 0x05},
> > > > > + {0x3416, 0x18},
> > > > > + {0x35AC, 0x0E},
> > > > > + {0x3648, 0x01},
> > > > > + {0x364A, 0x04},
> > > > > + {0x364C, 0x04},
> > > > > + {0x3678, 0x01},
> > > > > + {0x367C, 0x31},
> > > > > + {0x367E, 0x31},
> > > > > + {0x3708, 0x02},
> > > > > + {0x3714, 0x01},
> > > > > + {0x3715, 0x02},
> > > > > + {0x3716, 0x02},
> > > > > + {0x3717, 0x02},
> > > > > + {0x371C, 0x3D},
> > > > > + {0x371D, 0x3F},
> > > > > + {0x372C, 0x00},
> > > > > + {0x372D, 0x00},
> > > > > + {0x372E, 0x46},
> > > > > + {0x372F, 0x00},
> > > > > + {0x3730, 0x89},
> > > > > + {0x3731, 0x00},
> > > > > + {0x3732, 0x08},
> > > > > + {0x3733, 0x01},
> > > > > + {0x3734, 0xFE},
> > > > > + {0x3735, 0x05},
> > > > > + {0x375D, 0x00},
> > > > > + {0x375E, 0x00},
> > > > > + {0x375F, 0x61},
> > > > > + {0x3760, 0x06},
> > > > > + {0x3768, 0x1B},
> > > > > + {0x3769, 0x1B},
> > > > > + {0x376A, 0x1A},
> > > > > + {0x376B, 0x19},
> > > > > + {0x376C, 0x18},
> > > > > + {0x376D, 0x14},
> > > > > + {0x376E, 0x0F},
> > > > > + {0x3776, 0x00},
> > > > > + {0x3777, 0x00},
> > > > > + {0x3778, 0x46},
> > > > > + {0x3779, 0x00},
> > > > > + {0x377A, 0x08},
> > > > > + {0x377B, 0x01},
> > > > > + {0x377C, 0x45},
> > > > > + {0x377D, 0x01},
> > > > > + {0x377E, 0x23},
> > > > > + {0x377F, 0x02},
> > > > > + {0x3780, 0xD9},
> > > > > + {0x3781, 0x03},
> > > > > + {0x3782, 0xF5},
> > > > > + {0x3783, 0x06},
> > > > > + {0x3784, 0xA5},
> > > > > + {0x3788, 0x0F},
> > > > > + {0x378A, 0xD9},
> > > > > + {0x378B, 0x03},
> > > > > + {0x378C, 0xEB},
> > > > > + {0x378D, 0x05},
> > > > > + {0x378E, 0x87},
> > > > > + {0x378F, 0x06},
> > > > > + {0x3790, 0xF5},
> > > > > + {0x3792, 0x43},
> > > > > + {0x3794, 0x7A},
> > > > > + {0x3796, 0xA1},
> > > > > + {0x37B0, 0x37},
> > > > > + {0x3E04, 0x0E},
> > > > > + {0x30E8, 0x50},
> > > > > + {0x30E9, 0x00},
> > > > > + {0x3E04, 0x0E},
> > > > > + {0x3002, 0x00},
> > > > > +};
> > > > > +
> > > > > /* Sensor mode registers */
> > > > > static const struct imx334_reg mode_3840x2160_regs[] = {
> > > > > {0x3000, 0x01},
> > > > > @@ -243,20 +406,57 @@ static const struct imx334_reg
> > > > mode_3840x2160_regs[] = {
> > > > > {0x3a00, 0x01},
> > > > > };
> > > > >
> > > > > +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 codes[] = {
> > > >
> > > > Just "codes" is a little vague ? What about imx334_mbus_codes[] ?
> > > >
> > > > > + MEDIA_BUS_FMT_SRGGB10_1X10,
> > > > > + MEDIA_BUS_FMT_SRGGB12_1X12,
> > > > > +};
> > > > > +
> > > > > /* 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 = 1920,
> > > > > + .height = 1080,
> > > > > + .hblank = 280,
> > > > > + .vblank = 1170,
> > > > > + .vblank_min = 90,
> > > > > + .vblank_max = 132840,
> > > > > + .pclk = 594000000,
> > > > > + .link_freq_idx = 0,
> > > > > + .reg_list = {
> > > > > + .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > > > > + .regs = mode_1920x1080_regs,
> > > > > + },
> > > > > + },
> > > > > + {
> > > > > + .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,
> > > > > + },
> > > >
> > > > Nit: I would keep the existing formats and code in position [0] so
> > > > that if additional modes are added the defaults can be kept in [0].
> > > > Also I usually see drivers listing larger modes first, but that might not be a
> > strict rule.
> > > >
> > > > > },
> > > > > };
> > > > >
> > > > > @@ -382,7 +582,8 @@ static int imx334_update_controls(struct
> > > > > imx334
> > > > *imx334,
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > - ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
> > > > > + ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl,
> > > > IMX334_REG_MIN,
> > > > > + IMX334_REG_MAX, 1,
> > > > > + mode->hblank);
> > > >
> > > > I might have missed why is this change related
> > >
> > > Hblank is readonly variable.
> > > If I use __v4l2_ctrl_s_ctrl function update return error.
> >
> > So it's a separate fix unrelated to the introduction of the new mode ?
> >
> > Probably it never triggered before as we had a single mode so the control
> > was actually never updated, but it's a fix worth a patch of his own maybe.
> >
> > I would make a separate patch before this one or clearly mention in the
> > commit message that also this is changed and why.
> >
> > > >
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > @@ -506,10 +707,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(codes))
> > > > > return -EINVAL;
> > > > >
> > > > > - code->code = supported_mode.code;
> > > > > + code->code = codes[code->index];
> > > > >
> > > > > return 0;
> > > > > }
> > > > > @@ -526,15 +727,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)
> > > > > + int i;
> > > > > +
> > > > > + if (fsize->index >= ARRAY_SIZE(supported_modes))
> > > > > return -EINVAL;
> > > > >
> > > > > - if (fsize->code != supported_mode.code)
> > > > > + for (i = 0; i < ARRAY_SIZE(codes); i++) {
> > > >
> > > > Can we use for (unsigned int i = 0; ... ) now that C99 has been adopted ?
> > >
> > > C99 might be adopted.
> > > But I have not observered no one is started using.
> > >
> >
> > $ git grep -e "for (unsigned " -e "for (int " v6.0-rc2 | wc -l
> > 186
> >
> > > >
> > > > > + if (codes[i] == fsize->code)
> > > > > + break;
> > > > > return -EINVAL;
> > > >
> > > > Are you sure this is right ?
> > > > If the format code in fsize->code is == codes[1] you will return -EINVAL ?
> > >
> > > I think this will break the for loop, if fsize->code is == codes[1]
> >
> > Yes but before getting to codes[1] you would test codes[0] and if
> > fsize->code != codes[0] you will return -EINVAL
> >
> > Am I reading this wrong ?
> >
>
> Yes, I am wrong
>
> If (codes[0] != fsize->code && codes[1] != fsize->code )
> return -EINVAL
>
Or, if you want to generalize it for future modes:
unsigned int i;
for (i = 0; i < ARRAY_SIZE(codes); ++i) {
if (codes[i] == fsize->code)
break;
}
if (i == ARRAY_SIZE(codes))
return -EINVAL;
Thanks
j
> > > >
> > > > Does the sensor support more formats than
> > > > MEDIA_BUS_FMT_SRGGB10_1X10 and
> > MEDIA_BUS_FMT_SRGGB12_1X12 ?
> > > > Because otherwise you could replace the for loop with a simple check.
> > > >
> > > > However support for flip might change the bayer pattern, so it's
> > > > better to keep the loop maybe.
> > > >
> > > > What about centrlize it like you're doing in
> > > > imx219_get_format_code() and use it here ? You can place the helper
> > > > just after the supported mbus codes declaration maybe ?
> > > >
> > > Yes, it is good. But imx334 do not have direct register to do VFLIP.
> > >
> > > >
> > > > > + }
> > > > >
> > > > > - 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;
> > > > > @@ -553,7 +759,7 @@ 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.code = imx334->cur_code;
> > > > > fmt->format.field = V4L2_FIELD_NONE;
> > > > > fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> > > > > fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; @@ -591,6
> > > > > +797,18 @@ static int imx334_get_pad_format(struct v4l2_subdev
> > > > > +*sd,
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static int imx219_get_format_code(struct imx334 *imx334, struct
> > > > > +v4l2_subdev_format *fmt) {
> > > > > + int i;
> > > > > +
> > > > > + for (i = 0; i < ARRAY_SIZE(codes); i++) {
> > > >
> > > > Ditto, unsigned int and possibily declared inside the for loop
> > > >
> > > > > + if (codes[i] == fmt->format.code)
> > > > > + return codes[i];
> > > > > + }
> > > > > +
> > > > > + return -EINVAL;
> > > > > +}
> > > > > +
> > > > > /**
> > > > > * imx334_set_pad_format() - Set subdevice pad format
> > > > > * @sd: pointer to imx334 V4L2 sub-device structure @@ -606,10
> > > > > +824,21 @@ static int imx334_set_pad_format(struct v4l2_subdev
> > > > > +*sd,
> > > > > struct imx334 *imx334 = to_imx334(sd);
> > > > > const struct imx334_mode *mode;
> > > > > int ret = 0;
> > > > > + u32 code;
> > > > >
> > > > > mutex_lock(&imx334->mutex);
> > > > >
> > > > > - mode = &supported_mode;
> > > > > + code = imx219_get_format_code(imx334, fmt);
> > > > > + if (code < 0)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + imx334->cur_code = code;
> > > > > +
> > > > > + mode = v4l2_find_nearest_size(supported_modes,
> > > > > + ARRAY_SIZE(supported_modes),
> > > > > + width, height,
> > > > > + fmt->format.width,
> > > > > + fmt->format.height);
> > > > > +
> > > > > imx334_fill_pad_format(imx334, mode, fmt);
> > > > >
> > > > > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { @@ -617,7 +846,7
> > > > @@
> > > > > static int imx334_set_pad_format(struct v4l2_subdev *sd,
> > > > >
> > > > > framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt-
> > >pad);
> > > > > *framefmt = fmt->format;
> > > > > - } else {
> > > > > + } else if (imx334->cur_mode != mode) {
> > > >
> > > > Is this change related ? I think it's good as it avoids an
> > > > unecessary update of the controls, but maybe it's not related to this
> > patch ?
> > >
> > > I have updated mode[] array new set of value, Insated of calling
> > > functions simply.
> > > Just added a check the the mode changed then only we need to updated
> > > other parameters Same as imx219 code
> >
> > You're right, we had a single mode before this change :)
> >
> > >
> > > Thanks,
> > > Shravan
> > >
> > > >
> > > > Thanks
> > > > j
> > > >
> > > > > ret = imx334_update_controls(imx334, mode);
> > > > > if (!ret)
> > > > > imx334->cur_mode = mode; @@ -642,11 +871,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[1], &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 @@ -667,6 +911,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) {
> > > > > @@ -1037,7 +1288,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[1];
> > > > > + imx334->cur_code = codes[1];
> > > > > imx334->vblank = imx334->cur_mode->vblank;
> > > > >
> > > > > ret = imx334_init_controls(imx334);
> > > > > --
> > > > > 2.17.1
> > > > >
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-08-25 11:58 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-01 4:34 [PATCH] media: i2c: imx334: support lower bandwidth mode kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2022-07-28 6:30 shravan kumar
2022-08-22 11:24 ` Shravan.Chippa
2022-08-22 15:53 ` Jacopo Mondi
2022-08-25 7:16 ` Shravan.Chippa
2022-08-25 7:29 ` Jacopo Mondi
2022-08-25 11:09 ` Shravan.Chippa
2022-08-25 11:58 ` Jacopo Mondi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.