[PATCH v6 2/5] media: hi846: Fix link frequency handling
Sakari Ailus
sakari.ailus at linux.intel.com
Wed Sep 2 02:24:39 PDT 2026
Hi Pengyu,
Thanks for the update.
On Mon, Aug 31, 2026 at 12:00:22AM +0800, Pengyu Luo wrote:
> Link frequency is tied to PLL configuration, lane count, and external
> and configurable clock, so use runtime here instead of hardcoding for
> specific configuration. To implement this, we do
>
> 1. Drop fixed link freqs, we calculate the driver supported values and
> use v4l2_link_freq_to_bitmap() to get the intersection with the DT
> supported values.
>
> 2. Attach mipi_clk_div_{2,4}lane to current mode, and use the div with
> mclk clock, lane count to calculate link frequency.
>
> 3. Drop mclk clock rate check.
>
> Fixes: e8c0882685f9 ("media: i2c: add driver for the SK Hynix Hi-846 8M pixel camera")
> Signed-off-by: Pengyu Luo <mitltlatltl at gmail.com>
> ---
> v6:
> - Add link freq ctrl back (Sakari)
> - Use v4l2_link_freq_to_bitmap() to get matched link freqs (Sakari)
> - Move clk_get() before than hi846_parse_dt(), since we use clock in hi846_parse_dt()
> v5:
> - Use separated fields instead of raw register values for PLL cfg (Sakari)
> - Use mul_u64_u32_div() to avoid loss of pricision and u64/u32 issues (Sakari)
> - Drop line break (Sakari)
> ---
> drivers/media/i2c/hi846.c | 151 ++++++++++++++++++++++++--------------
> 1 file changed, 94 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 7f069aca0fce..2f8624f9bdf3 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
> @@ -1,7 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> // Copyright (c) 2021 Purism SPC
>
> -#include <linux/unaligned.h>
> +#include <linux/bitfield.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> #include <linux/gpio/consumer.h>
> @@ -11,6 +11,7 @@
> #include <linux/pm.h>
> #include <linux/property.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/unaligned.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-fwnode.h>
> @@ -219,8 +220,8 @@ struct hi846_mode {
> /* Horizontal timing size */
> u32 llp;
>
> - /* Link frequency needed for this resolution */
> - u8 link_freq_index;
> + u8 mipi_clk_div_2lane;
> + u8 mipi_clk_div_4lane;
>
> u16 fps;
>
> @@ -1040,13 +1041,6 @@ static const char * const hi846_test_pattern_menu[] = {
> "Resolution Pattern",
> };
>
> -#define FREQ_INDEX_640 0
> -#define FREQ_INDEX_1280 1
> -static const s64 hi846_link_freqs[] = {
> - [FREQ_INDEX_640] = 80000000,
> - [FREQ_INDEX_1280] = 200000000,
> -};
> -
> static const struct hi846_reg_list hi846_init_regs_list_2lane = {
> .num_of_regs = ARRAY_SIZE(hi846_init_2lane),
> .regs = hi846_init_2lane,
> @@ -1061,7 +1055,13 @@ static const struct hi846_mode supported_modes[] = {
> {
> .width = 640,
> .height = 480,
> - .link_freq_index = FREQ_INDEX_640,
> + .mipi_clk_div_2lane = 4,
> + /*
> + * Dummy but necessary if we set this mode default, otherwise
> + * hi846_calc_pixel_rate() will be broken in
> + * hi846_init_controls()
> + */
> + .mipi_clk_div_4lane = 8,
The divider of the 4-lane case appears to be always two times that of the
2-lane case. Could you calculate the value instead?
I think it'd be better to keep the link frequencies and modes at separate
indices; this is the way it used to be, too.
> .fps = 120,
> .frame_len = 631,
> .llp = HI846_LINE_LENGTH,
> @@ -1086,7 +1086,8 @@ static const struct hi846_mode supported_modes[] = {
> {
> .width = 1280,
> .height = 720,
> - .link_freq_index = FREQ_INDEX_1280,
> + .mipi_clk_div_2lane = 2,
> + .mipi_clk_div_4lane = 4,
> .fps = 90,
> .frame_len = 842,
> .llp = HI846_LINE_LENGTH,
> @@ -1112,7 +1113,8 @@ static const struct hi846_mode supported_modes[] = {
> {
> .width = 1632,
> .height = 1224,
> - .link_freq_index = FREQ_INDEX_1280,
> + .mipi_clk_div_2lane = 2,
> + .mipi_clk_div_4lane = 4,
> .fps = 30,
> .frame_len = 2526,
> .llp = HI846_LINE_LENGTH,
> @@ -1167,6 +1169,9 @@ struct hi846 {
> struct v4l2_ctrl *hblank;
> struct v4l2_ctrl *exposure;
>
> + s64 link_freqs[ARRAY_SIZE(supported_modes)];
> + int num_link_freqs;
> +
> struct mutex mutex; /* protect cur_mode, streaming and chip access */
> const struct hi846_mode *cur_mode;
> bool streaming;
> @@ -1192,21 +1197,41 @@ static const struct hi846_datafmt *hi846_find_datafmt(u32 code)
> return NULL;
> }
>
> -static inline u8 hi846_get_link_freq_index(struct hi846 *hi846)
> +static u64
> +hi846_get_link_freq(const struct hi846 *hi846, const struct hi846_mode *mode)
> {
> - return hi846->cur_mode->link_freq_index;
> + u64 mclk = clk_get_rate(hi846->clock);
> + u8 mipi_clk_div;
> +
> + if (hi846->nr_lanes == 2)
> + mipi_clk_div = mode->mipi_clk_div_2lane;
> + else
> + mipi_clk_div = mode->mipi_clk_div_4lane;
> +
> + /*
> + * HI846_REG_PLL_CFG_MIPI1_H = 0x025a, it is fixed in listed modes
> + * [11:8]: 0x02 => pre_div = 3
> + * [7:0]: 0x5a => multiplier = 90
> + */
> + return mul_u64_u32_div(mclk, 90, 3 * mipi_clk_div);
> }
>
> -static u64 hi846_get_link_freq(struct hi846 *hi846)
> +static int hi846_get_link_freq_index(const struct hi846 *hi846,
> + const struct hi846_mode *mode)
> {
> - u8 index = hi846_get_link_freq_index(hi846);
> + u64 link_freq = hi846_get_link_freq(hi846, mode);
> + int i;
> +
> + for (i = 0; i < hi846->num_link_freqs; i++)
> + if (hi846->link_freqs[i] == link_freq)
> + return i;
>
> - return hi846_link_freqs[index];
> + return -EINVAL;
> }
>
> static u64 hi846_calc_pixel_rate(struct hi846 *hi846)
> {
> - u64 link_freq = hi846_get_link_freq(hi846);
> + u64 link_freq = hi846_get_link_freq(hi846, hi846->cur_mode);
> u64 pixel_rate = link_freq * 2 * hi846->nr_lanes;
>
> do_div(pixel_rate, HI846_RGB_DEPTH);
> @@ -1429,8 +1454,8 @@ static int hi846_init_controls(struct hi846 *hi846)
> hi846->link_freq =
> v4l2_ctrl_new_int_menu(ctrl_hdlr, &hi846_ctrl_ops,
> V4L2_CID_LINK_FREQ,
> - ARRAY_SIZE(hi846_link_freqs) - 1,
> - 0, hi846_link_freqs);
> + hi846->num_link_freqs - 1,
> + 0, hi846->link_freqs);
> if (hi846->link_freq)
> hi846->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> @@ -1503,10 +1528,9 @@ static int hi846_set_video_mode(struct hi846 *hi846, int fps)
> u64 frame_length;
> int ret = 0;
> int dummy_lines;
> - u64 link_freq = hi846_get_link_freq(hi846);
> + u64 link_freq = hi846_get_link_freq(hi846, hi846->cur_mode);
>
> - dev_dbg(&client->dev, "%s: link freq: %llu\n", __func__,
> - hi846_get_link_freq(hi846));
> + dev_dbg(&client->dev, "%s: link freq: %llu\n", __func__, link_freq);
>
> do_div(link_freq, fps);
> frame_length = link_freq;
> @@ -1699,6 +1723,7 @@ static int hi846_set_format(struct v4l2_subdev *sd,
> const struct hi846_datafmt *fmt = hi846_find_datafmt(mf->code);
> u32 tgt_fps;
> s32 vblank_def, h_blank;
> + int idx;
>
> if (!fmt) {
> mf->code = hi846_colour_fmts[0].code;
> @@ -1749,7 +1774,14 @@ static int hi846_set_format(struct v4l2_subdev *sd,
> mf->code = HI846_MEDIA_BUS_FORMAT;
> mf->field = V4L2_FIELD_NONE;
>
> - __v4l2_ctrl_s_ctrl(hi846->link_freq, hi846_get_link_freq_index(hi846));
> + idx = hi846_get_link_freq_index(hi846, hi846->cur_mode);
> + if (idx < 0) {
> + dev_err(&client->dev,
> + "failed to get link freq index: %d\n", idx);
> + return -EINVAL;
> + }
> +
> + __v4l2_ctrl_s_ctrl(hi846->link_freq, idx);
> __v4l2_ctrl_s_ctrl_int64(hi846->pixel_rate,
> hi846_calc_pixel_rate(hi846));
>
> @@ -1947,20 +1979,33 @@ static int hi846_identify_module(struct hi846 *hi846)
> return 0;
> }
>
> -static s64 hi846_check_link_freqs(struct hi846 *hi846,
> - struct v4l2_fwnode_endpoint *ep)
> +static int hi846_add_link_freqs(struct hi846 *hi846, struct device *dev,
> + struct v4l2_fwnode_endpoint *ep)
> {
> - const s64 *freqs = hi846_link_freqs;
> - int freqs_count = ARRAY_SIZE(hi846_link_freqs);
> - int i, j;
> -
> - for (i = 0; i < freqs_count; i++) {
> - for (j = 0; j < ep->nr_of_link_frequencies; j++)
> - if (freqs[i] == ep->link_frequencies[j])
> - break;
> - if (j == ep->nr_of_link_frequencies)
> - return freqs[i];
> - }
> + s64 hi846_link_freqs[ARRAY_SIZE(supported_modes)];
> + unsigned long freq_bitmap;
> + int ret, i;
> +
> + /*
> + * Since the MCLK freq varies between platforms, calculating driver
> + * supported link freqs here.
> + */
> + for (i = 0; i < ARRAY_SIZE(supported_modes); i++)
> + hi846_link_freqs[i] = hi846_get_link_freq(hi846, &supported_modes[i]);
> +
> + ret = v4l2_link_freq_to_bitmap(dev, ep->link_frequencies,
> + ep->nr_of_link_frequencies,
> + hi846_link_freqs,
> + ARRAY_SIZE(hi846_link_freqs),
> + &freq_bitmap);
> + if (ret || !freq_bitmap)
> + return ret;
> +
> + for (i = 0; i < ARRAY_SIZE(hi846_link_freqs); i++)
Could you use the hi846_link_freqs array as-is for the control?
The selectable modes are expected to depend on the chose link frequency,
which is not affected by setting the format, for instance. I wonder if it'd
be useful to squash the next patch into this one.
> + if (BIT(i) & freq_bitmap) {
> + hi846->link_freqs[hi846->num_link_freqs++] = hi846_link_freqs[i];
> + dev_dbg(dev, "Add supported link frequency %lld\n", hi846_link_freqs[i]);
> + }
>
> return 0;
> }
> @@ -1973,7 +2018,6 @@ static int hi846_parse_dt(struct hi846 *hi846, struct device *dev)
> .bus_type = V4L2_MBUS_CSI2_DPHY
> };
> int ret;
> - s64 fq;
>
> ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> if (!ep) {
> @@ -2004,11 +2048,10 @@ static int hi846_parse_dt(struct hi846 *hi846, struct device *dev)
> goto check_hwcfg_error;
> }
>
> - /* Check that link frequences for all the modes are in device tree */
> - fq = hi846_check_link_freqs(hi846, &bus_cfg);
> - if (fq) {
> - dev_err(dev, "Link frequency of %lld is not supported\n", fq);
> - ret = -EINVAL;
> + /* Add link frequencies which are supported by both DT and the driver */
> + ret = hi846_add_link_freqs(hi846, dev, &bus_cfg);
> + if (ret) {
> + dev_err(dev, "failed to add link frequency %d\n", ret);
> goto check_hwcfg_error;
> }
>
> @@ -2041,30 +2084,24 @@ static int hi846_probe(struct i2c_client *client)
> struct hi846 *hi846;
> int ret;
> int i;
> - u32 mclk_freq;
>
> hi846 = devm_kzalloc(&client->dev, sizeof(*hi846), GFP_KERNEL);
> if (!hi846)
> return -ENOMEM;
>
> - ret = hi846_parse_dt(hi846, &client->dev);
> - if (ret) {
> - dev_err(&client->dev, "failed to check HW configuration: %d",
> - ret);
> - return ret;
> - }
> -
> + /* Get the MCLK first, since we need it to calculate link freqs */
> hi846->clock = devm_v4l2_sensor_clk_get(&client->dev, NULL);
> if (IS_ERR(hi846->clock))
> return dev_err_probe(&client->dev, PTR_ERR(hi846->clock),
> "failed to get clock: %pe\n",
> hi846->clock);
>
> - mclk_freq = clk_get_rate(hi846->clock);
> - if (mclk_freq != 25000000)
> - dev_warn(&client->dev,
> - "External clock freq should be 25000000, not %u.\n",
> - mclk_freq);
> + ret = hi846_parse_dt(hi846, &client->dev);
> + if (ret) {
> + dev_err(&client->dev, "failed to check HW configuration: %d",
> + ret);
> + return ret;
> + }
>
> for (i = 0; i < HI846_NUM_SUPPLIES; i++)
> hi846->supplies[i].supply = hi846_supply_names[i];
--
Regards,
Sakari Ailus
More information about the linux-arm-kernel
mailing list