[PATCH 0/4] Add link frequency to bitmap helper

[PATCH 0/4] Add link frequency to bitmap helper

[Sakari Ailus]

Hi folks,

This set adds a little helper for sensor drivers to match link frequencies
from system firmware with those supported the driver and generate a bitmap
suitable for passing to the control framework.

Imx319, imx334 and imx355 drivers are converted to use the helper, too.

I've tested imx319 and imx355 drivers.

Sakari Ailus (4):
  media: v4l: Add a helper for setting up link-frequencies control
  media: imx334: Use v4l2_link_frequencies_to_bitmap helper
  media: imx319: Use v4l2_link_frequencies_to_bitmap helper
  media: imx355: Use v4l2_link_frequencies_to_bitmap helper

 drivers/media/i2c/imx319.c            | 53 +++++++--------------------
 drivers/media/i2c/imx334.c            | 41 +++++++--------------
 drivers/media/i2c/imx355.c            | 53 +++++++--------------------
 drivers/media/v4l2-core/v4l2-common.c | 48 ++++++++++++++++++++++++
 include/media/v4l2-common.h           | 27 ++++++++++++++
 5 files changed, 116 insertions(+), 106 deletions(-)

-- 
2.39.2

[PATCH 1/4] media: v4l: Add a helper for setting up link-frequencies control

[Sakari Ailus]

Add a helper for obtaining supported link frequencies in form most drivers
need them. The result is a bitmap of supported controls.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-common.c | 48 +++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 27 +++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index e9e7e70fa24e..63302009db5f 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -585,3 +585,51 @@ u32 v4l2_fraction_to_interval(u32 numerator, u32 denominator)
 	return denominator ? numerator * multiplier / denominator : 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_fraction_to_interval);
+
+int v4l2_link_frequencies_to_bitmap(struct device *dev,
+				    const u64 *fw_link_freqs,
+				    unsigned int num_of_fw_link_freqs,
+				    const s64 *driver_link_freqs,
+				    unsigned int num_of_driver_link_freqs,
+				    unsigned long *bitmap)
+{
+	unsigned int i;
+
+	*bitmap = 0;
+
+	if (!num_of_fw_link_freqs) {
+		dev_err(dev, "no link frequencies in firmware\n");
+		return -ENODATA;
+	}
+
+	for (i = 0; i < num_of_fw_link_freqs; i++) {
+		unsigned int j;
+
+		for (j = 0; j < num_of_driver_link_freqs; j++) {
+			if (fw_link_freqs[i] != driver_link_freqs[j])
+				continue;
+
+			dev_dbg(dev, "enabling link frequency %lld Hz\n",
+				driver_link_freqs[j]);
+			*bitmap |= BIT(j);
+			break;
+		}
+	}
+
+	if (!*bitmap) {
+		dev_err(dev, "no matching link frequencies found\n");
+
+		dev_dbg(dev, "specified in firmware:\n");
+		for (i = 0; i < num_of_fw_link_freqs; i++)
+			dev_dbg(dev, "\t%llu Hz\n", fw_link_freqs[i]);
+
+		dev_dbg(dev, "driver supported:\n");
+		for (i = 0; i < num_of_driver_link_freqs; i++)
+			dev_dbg(dev, "\t%lld Hz\n", driver_link_freqs[i]);
+
+		return -ENOENT;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_link_frequencies_to_bitmap);
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index d278836fd9cb..adf997baf659 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -547,6 +547,33 @@ void v4l2_simplify_fraction(u32 *numerator, u32 *denominator,
 		unsigned int n_terms, unsigned int threshold);
 u32 v4l2_fraction_to_interval(u32 numerator, u32 denominator);
 
+/**
+ * v4l2_link_frequencies_to_bitmap - Figure out platform-supported link
+ *				     frequencies
+ * @dev: The struct device
+ * @fw_link_freqs: Array of link frequencies from firmware
+ * @num_of_fw_link_freqs: Number of entries in @fw_link_freqs
+ * @driver_link_freqs: Array of link frequencies supported by the driver
+ * @num_of_driver_link_freqs: Number of entries in @driver_link_freqs
+ * @bitmap: Bitmap of driver-supported link frequencies found in @fw_link_freqs
+ *
+ * This function checks which driver-supported link frequencies are enabled in
+ * system firmware and sets the corresponding bits in @bitmap (after first
+ * zeroing it).
+ *
+ * Return values:
+ *	0: Success
+ *	-ENOENT: No match found between driver-supported link frequencies and
+ *		 those available in firmware.
+ *	-ENODATA: No link frequencies were specified in firmware.
+ */
+int v4l2_link_frequencies_to_bitmap(struct device *dev,
+				    const u64 *fw_link_freqs,
+				    unsigned int num_of_fw_link_freqs,
+				    const s64 *driver_link_freqs,
+				    unsigned int num_of_driver_link_freqs,
+				    unsigned long *bitmap);
+
 static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
 {
 	/*
-- 
2.39.2

[PATCH 2/4] media: imx334: Use v4l2_link_frequencies_to_bitmap helper

[Sakari Ailus]

Use the v4l2_link_frequencies_to_bitmap() helper to figure out which
driver-supported link frequencies can be used on a given system.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/imx334.c | 41 ++++++++++++--------------------------
 1 file changed, 13 insertions(+), 28 deletions(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 6725b3e2a73e..76b88bbcb40a 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -136,7 +136,7 @@ struct imx334_mode {
  * @vblank: Vertical blanking in lines
  * @cur_mode: Pointer to current selected sensor mode
  * @mutex: Mutex for serializing sensor controls
- * @menu_skip_mask: Menu skip mask for link_freq_ctrl
+ * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
  * @cur_code: current selected format code
  */
 struct imx334 {
@@ -158,7 +158,7 @@ struct imx334 {
 	u32 vblank;
 	const struct imx334_mode *cur_mode;
 	struct mutex mutex;
-	unsigned long menu_skip_mask;
+	unsigned long link_freq_bitmap;
 	u32 cur_code;
 };
 
@@ -954,9 +954,9 @@ static int imx334_init_state(struct v4l2_subdev *sd,
 	imx334_fill_pad_format(imx334, imx334->cur_mode, &fmt);
 
 	__v4l2_ctrl_modify_range(imx334->link_freq_ctrl, 0,
-				 __fls(imx334->menu_skip_mask),
-				 ~(imx334->menu_skip_mask),
-				 __ffs(imx334->menu_skip_mask));
+				 __fls(imx334->link_freq_bitmap),
+				 ~(imx334->link_freq_bitmap),
+				 __ffs(imx334->link_freq_bitmap));
 
 	mutex_unlock(&imx334->mutex);
 
@@ -1157,26 +1157,11 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
 		goto done_endpoint_free;
 	}
 
-	if (!bus_cfg.nr_of_link_frequencies) {
-		dev_err(imx334->dev, "no link frequencies defined");
-		ret = -EINVAL;
-		goto done_endpoint_free;
-	}
-
-	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
-		for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
-			if (bus_cfg.link_frequencies[i] == link_freq[j]) {
-				set_bit(j, &imx334->menu_skip_mask);
-				break;
-			}
-		}
-
-		if (j == ARRAY_SIZE(link_freq)) {
-			ret = dev_err_probe(imx334->dev, -EINVAL,
-					    "no supported link freq found\n");
-			goto done_endpoint_free;
-		}
-	}
+	ret = v4l2_link_frequencies_to_bitmap(imx334->dev,
+					      bus_cfg.link_frequencies,
+					      bus_cfg.nr_of_link_frequencies,
+					      link_freq, ARRAY_SIZE(link_freq),
+					      &imx334->link_freq_bitmap);
 
 done_endpoint_free:
 	v4l2_fwnode_endpoint_free(&bus_cfg);
@@ -1310,8 +1295,8 @@ static int imx334_init_controls(struct imx334 *imx334)
 	imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
 							&imx334_ctrl_ops,
 							V4L2_CID_LINK_FREQ,
-							__fls(imx334->menu_skip_mask),
-							__ffs(imx334->menu_skip_mask),
+							__fls(imx334->link_freq_bitmap),
+							__ffs(imx334->link_freq_bitmap),
 							link_freq);
 
 	if (imx334->link_freq_ctrl)
@@ -1386,7 +1371,7 @@ static int imx334_probe(struct i2c_client *client)
 	}
 
 	/* Set default mode to max resolution */
-	imx334->cur_mode = &supported_modes[__ffs(imx334->menu_skip_mask)];
+	imx334->cur_mode = &supported_modes[__ffs(imx334->link_freq_bitmap)];
 	imx334->cur_code = imx334_mbus_codes[0];
 	imx334->vblank = imx334->cur_mode->vblank;
 
-- 
2.39.2

[PATCH 3/4] media: imx319: Use v4l2_link_frequencies_to_bitmap helper

[Sakari Ailus]

Use the v4l2_link_frequencies_to_bitmap() helper to figure out which
driver-supported link frequencies can be used on a given system.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/imx319.c | 53 ++++++++++----------------------------
 1 file changed, 14 insertions(+), 39 deletions(-)

diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index e47eff672e0c..3fdda6f7d01a 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -70,7 +70,7 @@
 #define IMX319_REG_ORIENTATION		0x0101
 
 /* default link frequency and external clock */
-#define IMX319_LINK_FREQ_DEFAULT	482400000
+#define IMX319_LINK_FREQ_DEFAULT	482400000LL
 #define IMX319_EXT_CLK			19200000
 #define IMX319_LINK_FREQ_INDEX		0
 
@@ -107,8 +107,7 @@ struct imx319_mode {
 
 struct imx319_hwcfg {
 	u32 ext_clk;			/* sensor external clk */
-	s64 *link_freqs;		/* CSI-2 link frequencies */
-	unsigned int nr_of_link_freqs;
+	unsigned long link_freq_bitmap;
 };
 
 struct imx319 {
@@ -129,7 +128,6 @@ struct imx319 {
 	const struct imx319_mode *cur_mode;
 
 	struct imx319_hwcfg *hwcfg;
-	s64 link_def_freq;	/* CSI-2 link default frequency */
 
 	/*
 	 * Mutex for serialized access:
@@ -1654,7 +1652,10 @@ static const char * const imx319_test_pattern_menu[] = {
 	"Pseudorandom Sequence (PN9)",
 };
 
-/* supported link frequencies */
+/*
+ * When adding more than the one below, make sure the disallowed ones will
+ * actually be disabled in the LINK_FREQ control.
+ */
 static const s64 link_freq_menu_items[] = {
 	IMX319_LINK_FREQ_DEFAULT,
 };
@@ -2058,7 +2059,7 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
 		*framefmt = fmt->format;
 	} else {
 		imx319->cur_mode = mode;
-		pixel_rate = imx319->link_def_freq * 2 * 4;
+		pixel_rate = IMX319_LINK_FREQ_DEFAULT * 2 * 4;
 		do_div(pixel_rate, 10);
 		__v4l2_ctrl_s_ctrl_int64(imx319->pixel_rate, pixel_rate);
 		/* Update limits and set FPS to default */
@@ -2255,7 +2256,7 @@ static int imx319_init_controls(struct imx319 *imx319)
 		imx319->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
 	/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
-	pixel_rate = imx319->link_def_freq * 2 * 4;
+	pixel_rate = IMX319_LINK_FREQ_DEFAULT * 2 * 4;
 	do_div(pixel_rate, 10);
 	/* By default, PIXEL_RATE is read only */
 	imx319->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops,
@@ -2332,7 +2333,6 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct device *dev)
 	};
 	struct fwnode_handle *ep;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
-	unsigned int i;
 	int ret;
 
 	if (!fwnode)
@@ -2364,24 +2364,14 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct device *dev)
 		goto out_err;
 	}
 
-	dev_dbg(dev, "num of link freqs: %d", bus_cfg.nr_of_link_frequencies);
-	if (!bus_cfg.nr_of_link_frequencies) {
-		dev_warn(dev, "no link frequencies defined");
-		goto out_err;
-	}
-
-	cfg->nr_of_link_freqs = bus_cfg.nr_of_link_frequencies;
-	cfg->link_freqs = devm_kcalloc(dev,
-				       bus_cfg.nr_of_link_frequencies + 1,
-				       sizeof(*cfg->link_freqs), GFP_KERNEL);
-	if (!cfg->link_freqs)
+	ret = v4l2_link_frequencies_to_bitmap(dev, bus_cfg.link_frequencies,
+					      bus_cfg.nr_of_link_frequencies,
+					      link_freq_menu_items,
+					      ARRAY_SIZE(link_freq_menu_items),
+					      &cfg->link_freq_bitmap);
+	if (ret)
 		goto out_err;
 
-	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
-		cfg->link_freqs[i] = bus_cfg.link_frequencies[i];
-		dev_dbg(dev, "link_freq[%d] = %lld", i, cfg->link_freqs[i]);
-	}
-
 	v4l2_fwnode_endpoint_free(&bus_cfg);
 	fwnode_handle_put(ep);
 	return cfg;
@@ -2397,7 +2387,6 @@ static int imx319_probe(struct i2c_client *client)
 	struct imx319 *imx319;
 	bool full_power;
 	int ret;
-	u32 i;
 
 	imx319 = devm_kzalloc(&client->dev, sizeof(*imx319), GFP_KERNEL);
 	if (!imx319)
@@ -2425,20 +2414,6 @@ static int imx319_probe(struct i2c_client *client)
 		goto error_probe;
 	}
 
-	imx319->link_def_freq = link_freq_menu_items[IMX319_LINK_FREQ_INDEX];
-	for (i = 0; i < imx319->hwcfg->nr_of_link_freqs; i++) {
-		if (imx319->hwcfg->link_freqs[i] == imx319->link_def_freq) {
-			dev_dbg(&client->dev, "link freq index %d matched", i);
-			break;
-		}
-	}
-
-	if (i == imx319->hwcfg->nr_of_link_freqs) {
-		dev_err(&client->dev, "no link frequency supported");
-		ret = -EINVAL;
-		goto error_probe;
-	}
-
 	/* Set default mode to max resolution */
 	imx319->cur_mode = &supported_modes[0];
 
-- 
2.39.2

[PATCH 4/4] media: imx355: Use v4l2_link_frequencies_to_bitmap helper

[Sakari Ailus]

Use the v4l2_link_frequencies_to_bitmap() helper to figure out which
driver-supported link frequencies can be used on a given system.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/imx355.c | 53 ++++++++++----------------------------
 1 file changed, 14 insertions(+), 39 deletions(-)

diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index 8c995c58743a..51e3f3ae53da 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -56,7 +56,7 @@
 #define IMX355_REG_ORIENTATION		0x0101
 
 /* default link frequency and external clock */
-#define IMX355_LINK_FREQ_DEFAULT	360000000
+#define IMX355_LINK_FREQ_DEFAULT	360000000LL
 #define IMX355_EXT_CLK			19200000
 #define IMX355_LINK_FREQ_INDEX		0
 
@@ -93,8 +93,7 @@ struct imx355_mode {
 
 struct imx355_hwcfg {
 	u32 ext_clk;			/* sensor external clk */
-	s64 *link_freqs;		/* CSI-2 link frequencies */
-	unsigned int nr_of_link_freqs;
+	unsigned long link_freq_bitmap;
 };
 
 struct imx355 {
@@ -115,7 +114,6 @@ struct imx355 {
 	const struct imx355_mode *cur_mode;
 
 	struct imx355_hwcfg *hwcfg;
-	s64 link_def_freq;	/* CSI-2 link default frequency */
 
 	/*
 	 * Mutex for serialized access:
@@ -879,7 +877,10 @@ static const char * const imx355_test_pattern_menu[] = {
 	"Pseudorandom Sequence (PN9)",
 };
 
-/* supported link frequencies */
+/*
+ * When adding more than the one below, make sure the disallowed ones will
+ * actually be disabled in the LINK_FREQ control.
+ */
 static const s64 link_freq_menu_items[] = {
 	IMX355_LINK_FREQ_DEFAULT,
 };
@@ -1356,7 +1357,7 @@ imx355_set_pad_format(struct v4l2_subdev *sd,
 		*framefmt = fmt->format;
 	} else {
 		imx355->cur_mode = mode;
-		pixel_rate = imx355->link_def_freq * 2 * 4;
+		pixel_rate = IMX355_LINK_FREQ_DEFAULT * 2 * 4;
 		do_div(pixel_rate, 10);
 		__v4l2_ctrl_s_ctrl_int64(imx355->pixel_rate, pixel_rate);
 		/* Update limits and set FPS to default */
@@ -1543,7 +1544,7 @@ static int imx355_init_controls(struct imx355 *imx355)
 		imx355->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
 	/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
-	pixel_rate = imx355->link_def_freq * 2 * 4;
+	pixel_rate = IMX355_LINK_FREQ_DEFAULT * 2 * 4;
 	do_div(pixel_rate, 10);
 	/* By default, PIXEL_RATE is read only */
 	imx355->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx355_ctrl_ops,
@@ -1620,7 +1621,6 @@ static struct imx355_hwcfg *imx355_get_hwcfg(struct device *dev)
 	};
 	struct fwnode_handle *ep;
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
-	unsigned int i;
 	int ret;
 
 	if (!fwnode)
@@ -1652,24 +1652,14 @@ static struct imx355_hwcfg *imx355_get_hwcfg(struct device *dev)
 		goto out_err;
 	}
 
-	dev_dbg(dev, "num of link freqs: %d", bus_cfg.nr_of_link_frequencies);
-	if (!bus_cfg.nr_of_link_frequencies) {
-		dev_warn(dev, "no link frequencies defined");
-		goto out_err;
-	}
-
-	cfg->nr_of_link_freqs = bus_cfg.nr_of_link_frequencies;
-	cfg->link_freqs = devm_kcalloc(dev,
-				       bus_cfg.nr_of_link_frequencies + 1,
-				       sizeof(*cfg->link_freqs), GFP_KERNEL);
-	if (!cfg->link_freqs)
+	ret = v4l2_link_frequencies_to_bitmap(dev, bus_cfg.link_frequencies,
+					      bus_cfg.nr_of_link_frequencies,
+					      link_freq_menu_items,
+					      ARRAY_SIZE(link_freq_menu_items),
+					      &cfg->link_freq_bitmap);
+	if (ret)
 		goto out_err;
 
-	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
-		cfg->link_freqs[i] = bus_cfg.link_frequencies[i];
-		dev_dbg(dev, "link_freq[%d] = %lld", i, cfg->link_freqs[i]);
-	}
-
 	v4l2_fwnode_endpoint_free(&bus_cfg);
 	fwnode_handle_put(ep);
 	return cfg;
@@ -1684,7 +1674,6 @@ static int imx355_probe(struct i2c_client *client)
 {
 	struct imx355 *imx355;
 	int ret;
-	u32 i;
 
 	imx355 = devm_kzalloc(&client->dev, sizeof(*imx355), GFP_KERNEL);
 	if (!imx355)
@@ -1709,20 +1698,6 @@ static int imx355_probe(struct i2c_client *client)
 		goto error_probe;
 	}
 
-	imx355->link_def_freq = link_freq_menu_items[IMX355_LINK_FREQ_INDEX];
-	for (i = 0; i < imx355->hwcfg->nr_of_link_freqs; i++) {
-		if (imx355->hwcfg->link_freqs[i] == imx355->link_def_freq) {
-			dev_dbg(&client->dev, "link freq index %d matched", i);
-			break;
-		}
-	}
-
-	if (i == imx355->hwcfg->nr_of_link_freqs) {
-		dev_err(&client->dev, "no link frequency supported");
-		ret = -EINVAL;
-		goto error_probe;
-	}
-
 	/* Set default mode to max resolution */
 	imx355->cur_mode = &supported_modes[0];
 
-- 
2.39.2

Re: [PATCH 2/4] media: imx334: Use v4l2_link_frequencies_to_bitmap helper

[kernel test robot]

Hi Sakari,

kernel test robot noticed the following build warnings:

[auto build test WARNING on media-tree/master]
[also build test WARNING on linuxtv-media-stage/master linus/master v6.7-rc5 next-20231211]
[cannot apply to sailus-media-tree/streams]
[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/Sakari-Ailus/media-v4l-Add-a-helper-for-setting-up-link-frequencies-control/20231211-220844
base:   git://linuxtv.org/media_tree.git master
patch link:    https://lore.kernel.org/r/20231211140658.366268-3-sakari.ailus%40linux.intel.com
patch subject: [PATCH 2/4] media: imx334: Use v4l2_link_frequencies_to_bitmap helper
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20231212/202312120434.56RvVcwQ-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231212/202312120434.56RvVcwQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312120434.56RvVcwQ-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/media/i2c/imx334.c:1115:15: warning: unused variable 'i' [-Wunused-variable]
           unsigned int i, j;
                        ^
>> drivers/media/i2c/imx334.c:1115:18: warning: unused variable 'j' [-Wunused-variable]
           unsigned int i, j;
                           ^
   2 warnings generated.


vim +/i +1115 drivers/media/i2c/imx334.c

9746b11715c394 Martina Krasteva 2021-02-03  1100  
9746b11715c394 Martina Krasteva 2021-02-03  1101  /**
9746b11715c394 Martina Krasteva 2021-02-03  1102   * imx334_parse_hw_config() - Parse HW configuration and check if supported
9746b11715c394 Martina Krasteva 2021-02-03  1103   * @imx334: pointer to imx334 device
9746b11715c394 Martina Krasteva 2021-02-03  1104   *
9746b11715c394 Martina Krasteva 2021-02-03  1105   * Return: 0 if successful, error code otherwise.
9746b11715c394 Martina Krasteva 2021-02-03  1106   */
9746b11715c394 Martina Krasteva 2021-02-03  1107  static int imx334_parse_hw_config(struct imx334 *imx334)
9746b11715c394 Martina Krasteva 2021-02-03  1108  {
9746b11715c394 Martina Krasteva 2021-02-03  1109  	struct fwnode_handle *fwnode = dev_fwnode(imx334->dev);
9746b11715c394 Martina Krasteva 2021-02-03  1110  	struct v4l2_fwnode_endpoint bus_cfg = {
9746b11715c394 Martina Krasteva 2021-02-03  1111  		.bus_type = V4L2_MBUS_CSI2_DPHY
9746b11715c394 Martina Krasteva 2021-02-03  1112  	};
9746b11715c394 Martina Krasteva 2021-02-03  1113  	struct fwnode_handle *ep;
9746b11715c394 Martina Krasteva 2021-02-03  1114  	unsigned long rate;
e3269ea5148d39 Shravan Chippa   2023-04-14 @1115  	unsigned int i, j;
9746b11715c394 Martina Krasteva 2021-02-03  1116  	int ret;
9746b11715c394 Martina Krasteva 2021-02-03  1117  
9746b11715c394 Martina Krasteva 2021-02-03  1118  	if (!fwnode)
9746b11715c394 Martina Krasteva 2021-02-03  1119  		return -ENXIO;
9746b11715c394 Martina Krasteva 2021-02-03  1120  
9746b11715c394 Martina Krasteva 2021-02-03  1121  	/* Request optional reset pin */
9746b11715c394 Martina Krasteva 2021-02-03  1122  	imx334->reset_gpio = devm_gpiod_get_optional(imx334->dev, "reset",
9746b11715c394 Martina Krasteva 2021-02-03  1123  						     GPIOD_OUT_LOW);
9746b11715c394 Martina Krasteva 2021-02-03  1124  	if (IS_ERR(imx334->reset_gpio)) {
c702e2f70275db Hans Verkuil     2021-02-08  1125  		dev_err(imx334->dev, "failed to get reset gpio %ld",
c702e2f70275db Hans Verkuil     2021-02-08  1126  			PTR_ERR(imx334->reset_gpio));
9746b11715c394 Martina Krasteva 2021-02-03  1127  		return PTR_ERR(imx334->reset_gpio);
9746b11715c394 Martina Krasteva 2021-02-03  1128  	}
9746b11715c394 Martina Krasteva 2021-02-03  1129  
9746b11715c394 Martina Krasteva 2021-02-03  1130  	/* Get sensor input clock */
9746b11715c394 Martina Krasteva 2021-02-03  1131  	imx334->inclk = devm_clk_get(imx334->dev, NULL);
9746b11715c394 Martina Krasteva 2021-02-03  1132  	if (IS_ERR(imx334->inclk)) {
9746b11715c394 Martina Krasteva 2021-02-03  1133  		dev_err(imx334->dev, "could not get inclk");
9746b11715c394 Martina Krasteva 2021-02-03  1134  		return PTR_ERR(imx334->inclk);
9746b11715c394 Martina Krasteva 2021-02-03  1135  	}
9746b11715c394 Martina Krasteva 2021-02-03  1136  
9746b11715c394 Martina Krasteva 2021-02-03  1137  	rate = clk_get_rate(imx334->inclk);
9746b11715c394 Martina Krasteva 2021-02-03  1138  	if (rate != IMX334_INCLK_RATE) {
9746b11715c394 Martina Krasteva 2021-02-03  1139  		dev_err(imx334->dev, "inclk frequency mismatch");
9746b11715c394 Martina Krasteva 2021-02-03  1140  		return -EINVAL;
9746b11715c394 Martina Krasteva 2021-02-03  1141  	}
9746b11715c394 Martina Krasteva 2021-02-03  1142  
9746b11715c394 Martina Krasteva 2021-02-03  1143  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
9746b11715c394 Martina Krasteva 2021-02-03  1144  	if (!ep)
9746b11715c394 Martina Krasteva 2021-02-03  1145  		return -ENXIO;
9746b11715c394 Martina Krasteva 2021-02-03  1146  
9746b11715c394 Martina Krasteva 2021-02-03  1147  	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
9746b11715c394 Martina Krasteva 2021-02-03  1148  	fwnode_handle_put(ep);
9746b11715c394 Martina Krasteva 2021-02-03  1149  	if (ret)
9746b11715c394 Martina Krasteva 2021-02-03  1150  		return ret;
9746b11715c394 Martina Krasteva 2021-02-03  1151  
9746b11715c394 Martina Krasteva 2021-02-03  1152  	if (bus_cfg.bus.mipi_csi2.num_data_lanes != IMX334_NUM_DATA_LANES) {
9746b11715c394 Martina Krasteva 2021-02-03  1153  		dev_err(imx334->dev,
9746b11715c394 Martina Krasteva 2021-02-03  1154  			"number of CSI2 data lanes %d is not supported",
9746b11715c394 Martina Krasteva 2021-02-03  1155  			bus_cfg.bus.mipi_csi2.num_data_lanes);
9746b11715c394 Martina Krasteva 2021-02-03  1156  		ret = -EINVAL;
9746b11715c394 Martina Krasteva 2021-02-03  1157  		goto done_endpoint_free;
9746b11715c394 Martina Krasteva 2021-02-03  1158  	}
9746b11715c394 Martina Krasteva 2021-02-03  1159  
4b895656c4c2b2 Sakari Ailus     2023-12-11  1160  	ret = v4l2_link_frequencies_to_bitmap(imx334->dev,
4b895656c4c2b2 Sakari Ailus     2023-12-11  1161  					      bus_cfg.link_frequencies,
4b895656c4c2b2 Sakari Ailus     2023-12-11  1162  					      bus_cfg.nr_of_link_frequencies,
4b895656c4c2b2 Sakari Ailus     2023-12-11  1163  					      link_freq, ARRAY_SIZE(link_freq),
4b895656c4c2b2 Sakari Ailus     2023-12-11  1164  					      &imx334->link_freq_bitmap);
9746b11715c394 Martina Krasteva 2021-02-03  1165  
9746b11715c394 Martina Krasteva 2021-02-03  1166  done_endpoint_free:
9746b11715c394 Martina Krasteva 2021-02-03  1167  	v4l2_fwnode_endpoint_free(&bus_cfg);
9746b11715c394 Martina Krasteva 2021-02-03  1168  
9746b11715c394 Martina Krasteva 2021-02-03  1169  	return ret;
9746b11715c394 Martina Krasteva 2021-02-03  1170  }
9746b11715c394 Martina Krasteva 2021-02-03  1171  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/4] media: v4l: Add a helper for setting up link-frequencies control

[kernel test robot]

Hi Sakari,

kernel test robot noticed the following build warnings:

[auto build test WARNING on media-tree/master]
[also build test WARNING on linuxtv-media-stage/master sailus-media-tree/streams linus/master v6.7-rc5 next-20231212]
[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/Sakari-Ailus/media-v4l-Add-a-helper-for-setting-up-link-frequencies-control/20231211-220844
base:   git://linuxtv.org/media_tree.git master
patch link:    https://lore.kernel.org/r/20231211140658.366268-2-sakari.ailus%40linux.intel.com
patch subject: [PATCH 1/4] media: v4l: Add a helper for setting up link-frequencies control
reproduce: (https://download.01.org/0day-ci/archive/20231212/202312122344.1l0j6bV6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312122344.1l0j6bV6-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Documentation/driver-api/media/v4l2-common:6: ./include/media/v4l2-common.h:567: WARNING: Unexpected indentation.
>> Documentation/driver-api/media/v4l2-common:6: ./include/media/v4l2-common.h:568: WARNING: Block quote ends without a blank line; unexpected unindent.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

RE: [PATCH 3/4] media: imx319: Use v4l2_link_frequencies_to_bitmap helper

[Cao, Bingbu]

Sakari, 

Thanks for the patch.

------------------------------------------------------------------------
BRs,  
Bingbu Cao 

>-----Original Message-----
>From: Sakari Ailus <sakari.ailus@linux.intel.com>
>Sent: Monday, December 11, 2023 10:07 PM
>To: linux-media@vger.kernel.org
>Cc: laurent.pinchart@ideasonboard.com; jacopo.mondi@ideasonboard.com; Cao,
>Bingbu <bingbu.cao@intel.com>; Qiu, Tian Shu <tian.shu.qiu@intel.com>; Hans
>Verkuil <hverkuil-cisco@xs4all.nl>
>Subject: [PATCH 3/4] media: imx319: Use v4l2_link_frequencies_to_bitmap
>helper
>
>Use the v4l2_link_frequencies_to_bitmap() helper to figure out which
>driver-supported link frequencies can be used on a given system.
>
>Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>---
> drivers/media/i2c/imx319.c | 53 ++++++++++----------------------------
> 1 file changed, 14 insertions(+), 39 deletions(-)
>
>diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c index
>e47eff672e0c..3fdda6f7d01a 100644
>--- a/drivers/media/i2c/imx319.c
>+++ b/drivers/media/i2c/imx319.c
>@@ -70,7 +70,7 @@
> #define IMX319_REG_ORIENTATION		0x0101
>
> /* default link frequency and external clock */
>-#define IMX319_LINK_FREQ_DEFAULT	482400000
>+#define IMX319_LINK_FREQ_DEFAULT	482400000LL
> #define IMX319_EXT_CLK			19200000
> #define IMX319_LINK_FREQ_INDEX		0
>
>@@ -107,8 +107,7 @@ struct imx319_mode {
>
> struct imx319_hwcfg {
> 	u32 ext_clk;			/* sensor external clk */
>-	s64 *link_freqs;		/* CSI-2 link frequencies */
>-	unsigned int nr_of_link_freqs;
>+	unsigned long link_freq_bitmap;
> };
>
> struct imx319 {
>@@ -129,7 +128,6 @@ struct imx319 {
> 	const struct imx319_mode *cur_mode;
>
> 	struct imx319_hwcfg *hwcfg;
>-	s64 link_def_freq;	/* CSI-2 link default frequency */
>
> 	/*
> 	 * Mutex for serialized access:
>@@ -1654,7 +1652,10 @@ static const char * const imx319_test_pattern_menu[]
>= {
> 	"Pseudorandom Sequence (PN9)",
> };
>
>-/* supported link frequencies */
>+/*
>+ * When adding more than the one below, make sure the disallowed ones
>+will
>+ * actually be disabled in the LINK_FREQ control.
>+ */
> static const s64 link_freq_menu_items[] = {
> 	IMX319_LINK_FREQ_DEFAULT,
> };
>@@ -2058,7 +2059,7 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
> 		*framefmt = fmt->format;
> 	} else {
> 		imx319->cur_mode = mode;
>-		pixel_rate = imx319->link_def_freq * 2 * 4;
>+		pixel_rate = IMX319_LINK_FREQ_DEFAULT * 2 * 4;
> 		do_div(pixel_rate, 10);
> 		__v4l2_ctrl_s_ctrl_int64(imx319->pixel_rate, pixel_rate);
> 		/* Update limits and set FPS to default */ @@ -2255,7 +2256,7
>@@ static int imx319_init_controls(struct imx319 *imx319)
> 		imx319->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> 	/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
>-	pixel_rate = imx319->link_def_freq * 2 * 4;
>+	pixel_rate = IMX319_LINK_FREQ_DEFAULT * 2 * 4;
> 	do_div(pixel_rate, 10);
> 	/* By default, PIXEL_RATE is read only */
> 	imx319->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops,
>@@ -2332,7 +2333,6 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct
>device *dev)
> 	};
> 	struct fwnode_handle *ep;
> 	struct fwnode_handle *fwnode = dev_fwnode(dev);
>-	unsigned int i;
> 	int ret;
>
> 	if (!fwnode)
>@@ -2364,24 +2364,14 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct
>device *dev)
> 		goto out_err;
> 	}
>
>-	dev_dbg(dev, "num of link freqs: %d",
>bus_cfg.nr_of_link_frequencies);
>-	if (!bus_cfg.nr_of_link_frequencies) {
>-		dev_warn(dev, "no link frequencies defined");
>-		goto out_err;
>-	}
>-
>-	cfg->nr_of_link_freqs = bus_cfg.nr_of_link_frequencies;
>-	cfg->link_freqs = devm_kcalloc(dev,
>-				       bus_cfg.nr_of_link_frequencies + 1,
>-				       sizeof(*cfg->link_freqs), GFP_KERNEL);
>-	if (!cfg->link_freqs)
>+	ret = v4l2_link_frequencies_to_bitmap(dev, bus_cfg.link_frequencies,
>+					      bus_cfg.nr_of_link_frequencies,
>+					      link_freq_menu_items,
>+					      ARRAY_SIZE(link_freq_menu_items),
>+					      &cfg->link_freq_bitmap);
>+	if (ret)
> 		goto out_err;
>
>-	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
>-		cfg->link_freqs[i] = bus_cfg.link_frequencies[i];
>-		dev_dbg(dev, "link_freq[%d] = %lld", i, cfg->link_freqs[i]);
>-	}
>-
> 	v4l2_fwnode_endpoint_free(&bus_cfg);
> 	fwnode_handle_put(ep);
> 	return cfg;
>@@ -2397,7 +2387,6 @@ static int imx319_probe(struct i2c_client *client)
> 	struct imx319 *imx319;
> 	bool full_power;
> 	int ret;
>-	u32 i;
>
> 	imx319 = devm_kzalloc(&client->dev, sizeof(*imx319), GFP_KERNEL);
> 	if (!imx319)
>@@ -2425,20 +2414,6 @@ static int imx319_probe(struct i2c_client *client)
> 		goto error_probe;
> 	}
>
>-	imx319->link_def_freq = link_freq_menu_items[IMX319_LINK_FREQ_INDEX];
>-	for (i = 0; i < imx319->hwcfg->nr_of_link_freqs; i++) {
>-		if (imx319->hwcfg->link_freqs[i] == imx319->link_def_freq) {
>-			dev_dbg(&client->dev, "link freq index %d matched", i);
>-			break;
>-		}
>-	}
>-
>-	if (i == imx319->hwcfg->nr_of_link_freqs) {
>-		dev_err(&client->dev, "no link frequency supported");
>-		ret = -EINVAL;
>-		goto error_probe;
>-	}
>-

Do we also need change the link frequency ctrl *max* value once we get
The bitmap?

> 	/* Set default mode to max resolution */
> 	imx319->cur_mode = &supported_modes[0];
>
>--
>2.39.2

Re: [PATCH 3/4] media: imx319: Use v4l2_link_frequencies_to_bitmap helper

[Sakari Ailus]

Hi Bingbu,

On Wed, Dec 13, 2023 at 02:16:31AM +0000, Cao, Bingbu wrote:
> Sakari, 
> 
> Thanks for the patch.
> 
> ------------------------------------------------------------------------
> BRs,  
> Bingbu Cao 
> 
> >-----Original Message-----
> >From: Sakari Ailus <sakari.ailus@linux.intel.com>
> >Sent: Monday, December 11, 2023 10:07 PM
> >To: linux-media@vger.kernel.org
> >Cc: laurent.pinchart@ideasonboard.com; jacopo.mondi@ideasonboard.com; Cao,
> >Bingbu <bingbu.cao@intel.com>; Qiu, Tian Shu <tian.shu.qiu@intel.com>; Hans
> >Verkuil <hverkuil-cisco@xs4all.nl>
> >Subject: [PATCH 3/4] media: imx319: Use v4l2_link_frequencies_to_bitmap
> >helper
> >
> >Use the v4l2_link_frequencies_to_bitmap() helper to figure out which
> >driver-supported link frequencies can be used on a given system.
> >
> >Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >---
> > drivers/media/i2c/imx319.c | 53 ++++++++++----------------------------
> > 1 file changed, 14 insertions(+), 39 deletions(-)
> >
> >diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c index
> >e47eff672e0c..3fdda6f7d01a 100644
> >--- a/drivers/media/i2c/imx319.c
> >+++ b/drivers/media/i2c/imx319.c
> >@@ -70,7 +70,7 @@
> > #define IMX319_REG_ORIENTATION		0x0101
> >
> > /* default link frequency and external clock */
> >-#define IMX319_LINK_FREQ_DEFAULT	482400000
> >+#define IMX319_LINK_FREQ_DEFAULT	482400000LL
> > #define IMX319_EXT_CLK			19200000
> > #define IMX319_LINK_FREQ_INDEX		0
> >
> >@@ -107,8 +107,7 @@ struct imx319_mode {
> >
> > struct imx319_hwcfg {
> > 	u32 ext_clk;			/* sensor external clk */
> >-	s64 *link_freqs;		/* CSI-2 link frequencies */
> >-	unsigned int nr_of_link_freqs;
> >+	unsigned long link_freq_bitmap;
> > };
> >
> > struct imx319 {
> >@@ -129,7 +128,6 @@ struct imx319 {
> > 	const struct imx319_mode *cur_mode;
> >
> > 	struct imx319_hwcfg *hwcfg;
> >-	s64 link_def_freq;	/* CSI-2 link default frequency */
> >
> > 	/*
> > 	 * Mutex for serialized access:
> >@@ -1654,7 +1652,10 @@ static const char * const imx319_test_pattern_menu[]
> >= {
> > 	"Pseudorandom Sequence (PN9)",
> > };
> >
> >-/* supported link frequencies */
> >+/*
> >+ * When adding more than the one below, make sure the disallowed ones
> >+will
> >+ * actually be disabled in the LINK_FREQ control.
> >+ */
> > static const s64 link_freq_menu_items[] = {
> > 	IMX319_LINK_FREQ_DEFAULT,
> > };
> >@@ -2058,7 +2059,7 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
> > 		*framefmt = fmt->format;
> > 	} else {
> > 		imx319->cur_mode = mode;
> >-		pixel_rate = imx319->link_def_freq * 2 * 4;
> >+		pixel_rate = IMX319_LINK_FREQ_DEFAULT * 2 * 4;
> > 		do_div(pixel_rate, 10);
> > 		__v4l2_ctrl_s_ctrl_int64(imx319->pixel_rate, pixel_rate);
> > 		/* Update limits and set FPS to default */ @@ -2255,7 +2256,7
> >@@ static int imx319_init_controls(struct imx319 *imx319)
> > 		imx319->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >
> > 	/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> >-	pixel_rate = imx319->link_def_freq * 2 * 4;
> >+	pixel_rate = IMX319_LINK_FREQ_DEFAULT * 2 * 4;
> > 	do_div(pixel_rate, 10);
> > 	/* By default, PIXEL_RATE is read only */
> > 	imx319->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops,
> >@@ -2332,7 +2333,6 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct
> >device *dev)
> > 	};
> > 	struct fwnode_handle *ep;
> > 	struct fwnode_handle *fwnode = dev_fwnode(dev);
> >-	unsigned int i;
> > 	int ret;
> >
> > 	if (!fwnode)
> >@@ -2364,24 +2364,14 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct
> >device *dev)
> > 		goto out_err;
> > 	}
> >
> >-	dev_dbg(dev, "num of link freqs: %d",
> >bus_cfg.nr_of_link_frequencies);
> >-	if (!bus_cfg.nr_of_link_frequencies) {
> >-		dev_warn(dev, "no link frequencies defined");
> >-		goto out_err;
> >-	}
> >-
> >-	cfg->nr_of_link_freqs = bus_cfg.nr_of_link_frequencies;
> >-	cfg->link_freqs = devm_kcalloc(dev,
> >-				       bus_cfg.nr_of_link_frequencies + 1,
> >-				       sizeof(*cfg->link_freqs), GFP_KERNEL);
> >-	if (!cfg->link_freqs)
> >+	ret = v4l2_link_frequencies_to_bitmap(dev, bus_cfg.link_frequencies,
> >+					      bus_cfg.nr_of_link_frequencies,
> >+					      link_freq_menu_items,
> >+					      ARRAY_SIZE(link_freq_menu_items),
> >+					      &cfg->link_freq_bitmap);
> >+	if (ret)
> > 		goto out_err;
> >
> >-	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> >-		cfg->link_freqs[i] = bus_cfg.link_frequencies[i];
> >-		dev_dbg(dev, "link_freq[%d] = %lld", i, cfg->link_freqs[i]);
> >-	}
> >-
> > 	v4l2_fwnode_endpoint_free(&bus_cfg);
> > 	fwnode_handle_put(ep);
> > 	return cfg;
> >@@ -2397,7 +2387,6 @@ static int imx319_probe(struct i2c_client *client)
> > 	struct imx319 *imx319;
> > 	bool full_power;
> > 	int ret;
> >-	u32 i;
> >
> > 	imx319 = devm_kzalloc(&client->dev, sizeof(*imx319), GFP_KERNEL);
> > 	if (!imx319)
> >@@ -2425,20 +2414,6 @@ static int imx319_probe(struct i2c_client *client)
> > 		goto error_probe;
> > 	}
> >
> >-	imx319->link_def_freq = link_freq_menu_items[IMX319_LINK_FREQ_INDEX];
> >-	for (i = 0; i < imx319->hwcfg->nr_of_link_freqs; i++) {
> >-		if (imx319->hwcfg->link_freqs[i] == imx319->link_def_freq) {
> >-			dev_dbg(&client->dev, "link freq index %d matched", i);
> >-			break;
> >-		}
> >-	}
> >-
> >-	if (i == imx319->hwcfg->nr_of_link_freqs) {
> >-		dev_err(&client->dev, "no link frequency supported");
> >-		ret = -EINVAL;
> >-		goto error_probe;
> >-	}
> >-
> 
> Do we also need change the link frequency ctrl *max* value once we get
> The bitmap?

The driver supports a single value, so in this case not. We also have a
mask of supported values so min and max are less critical. It would
actually be good to have a helper to set up the control correctly, too.
Unfortunately the function creating the control doesn't take a mask as
argument.

-- 
Sakari Ailus