[00/23] v2: imx258 improvement series

From: Luigi311 <git@luigi311.com>

Resend due to email message limits being exceeded.

v2 changes:
- Add use macros patch 
- Add support for powerdown gpio patch
- Add support for reset gpio patch
- Dropped Add support for long exposure modes patch
- Implemented feedback from Jacopo Mondi
  - media: i2c: imx258: Add regulator control
  - media: i2c: imx258: Add support for 24MHz clock
  - media: i2c: imx258: Add support for running on 2 CSI data lanes
  - media: i2c: imx258: Add get_selection for pixel array information
  - media: i2c: imx258: Issue reset before starting streaming
  - media: i2c: imx258: Set pixel_rate range to the same as the value
  - dt-bindings: media: imx258: Add alternate compatible strings
  - media: i2c: imx258: Change register settings for variants of the sensor
  - media: i2c: imx258: Make HFLIP and VFLIP controls writable

This adds a few more patches and drops one. The long exposure mode patch was
dropped due to the bug that Jacopo found. The powerdown and reset gpio patches
were added as that fixes support for the Pinephone Pro, without them the sensor
doesnt initialize correctly.

Tested on a Pinephone Pro by forcing 24 mhz clock and was able to access all 3
resolutions. The two lower resolutions had some artifacts but that is expected
as more changes are required to fix them for the Pinephone Pro specifically,
kept all registers the same as Dave's original patch since that works on
dedicated imx258 hardware and the artifacts are PPP specific so it shouldnt be
a regression.


v1

This is a set of patches for imx258 that allow it to work with alternate clock
frequencies, over either 2 or 4 lanes, and generally adding flexibility to the
driver.

Tested with an IMX258 module from Soho Enterprises that has a 24MHz oscillator.
Both 2 and 4 lane configurations work with correct link frequencies and pixel
rates.

Jacopo has tested on a PinePhone Pro which has an ~19.2MHz clock fed from the SoC,
He confirms that the two lower resolution modes work, but not the full res mode.
Comparing to the BSP it looks like they have some weird clock configuration in
the 4208x3120 mode (nominally 1224Mb/s/lane instead of 1267).
As it has never previously worked directly with the mainline driver this isn't a
regression but may indicate that there is a need for support of additional link
frequencies in the future.

The last patch that makes HFLIP and VFLIP configurable may be contentious as I've
retained the default configuration of inverted from the original driver. I know
this was discussed recently, but I can't recall the final outcome.

I am relying on someone from Intel testing this out, as correcting the cropping
and supporting flips has changed the Bayer order. Seeing as this is all above
board in V4L2 terms I really hope that the layers above it behave themselves.

Cheers
  Dave


Dave Stevenson (20):
  media: i2c: imx258: Remove unused defines
  media: i2c: imx258: Make image geometry meet sensor requirements
  media: i2c: imx258: Disable digital cropping on binned modes
  media: i2c: imx258: Remove redundant I2C writes.
  media: i2c: imx258: Add regulator control
  media: i2c: imx258: Make V4L2_CID_VBLANK configurable.
  media: i2c: imx258: Split out common registers from the mode based
    ones
  media: i2c: imx258: Add support for 24MHz clock
  media: i2c: imx258: Add support for running on 2 CSI data lanes
  media: i2c: imx258: Follow normal V4L2 behaviours for clipping
    exposure
  media: i2c: imx258: Add get_selection for pixel array information
  media: i2c: imx258: Allow configuration of clock lane behaviour
  media: i2c: imx258: Correct max FRM_LENGTH_LINES value
  media: i2c: imx258: Issue reset before starting streaming
  media: i2c: imx258: Set pixel_rate range to the same as the value
  media: i2c: imx258: Support faster pixel rate on binned modes
  dt-bindings: media: imx258: Rename to include vendor prefix
  dt-bindings: media: imx258: Add alternate compatible strings
  media: i2c: imx258: Change register settings for variants of the
    sensor
  media: i2c: imx258: Make HFLIP and VFLIP controls writable

Luigi311 (3):
  drivers: media: i2c: imx258: Use macros
  drivers: media: i2c: imx258: Add support for powerdown gpio
  drivers: media: i2c: imx258: Add support for reset gpio

 .../i2c/{imx258.yaml => sony,imx258.yaml}     |   12 +-
 MAINTAINERS                                   |    2 +-
 drivers/media/i2c/imx258.c                    | 1147 +++++++++++------
 3 files changed, 744 insertions(+), 417 deletions(-)
 rename Documentation/devicetree/bindings/media/i2c/{imx258.yaml => sony,imx258.yaml} (88%)

On 28/03/2024 00:16, git@luigi311.com wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> The IMX258_FLL_* defines are unused. Remove them.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Your SoB is missing.

Best regards,
Krzysztof

On 28/03/2024 00:16, git@luigi311.com wrote:
> From: Luigi311 <git@luigi311.com>
> 
> Resend due to email message limits being exceeded.
> 
> v2 changes:

Please mark your patches correctly. Use b4 or -v2 for format-patch.

Best regards,
Krzysztof

Hi Luigi311,

Thank you for the patchset.

On Wed, Mar 27, 2024 at 05:16:54PM -0600, git@luigi311.com wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> There's no reason why only a clock of 19.2MHz is supported.
> Indeed this isn't even a frequency listed in the datasheet.
> 
> Add support for 24MHz as well.
> The PLL settings result in slightly different link frequencies,
> so parameterise those.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Luigi311 <git@luigi311.com>

Is Luigi311 your real name? As per
Documentation/process/submitting-patches.rst, anonymous (or pseudonym I'd
say as well) contributions are not an option.

> ---
>  drivers/media/i2c/imx258.c | 133 +++++++++++++++++++++++++++++--------
>  1 file changed, 107 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 351add1bc5d5..6ee7de079454 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -76,9 +76,6 @@
>  #define REG_CONFIG_MIRROR_FLIP		0x03
>  #define REG_CONFIG_FLIP_TEST_PATTERN	0x02
>  
> -/* Input clock frequency in Hz */
> -#define IMX258_INPUT_CLOCK_FREQ		19200000
> -
>  struct imx258_reg {
>  	u16 address;
>  	u8 val;
> @@ -115,7 +112,9 @@ struct imx258_mode {
>  };
>  
>  /* 4208x3120 needs 1267Mbps/lane, 4 lanes */
> -static const struct imx258_reg mipi_data_rate_1267mbps[] = {
> +static const struct imx258_reg mipi_1267mbps_19_2mhz[] = {
> +	{ 0x0136, 0x13 },
> +	{ 0x0137, 0x33 },
>  	{ 0x0301, 0x05 },
>  	{ 0x0303, 0x02 },
>  	{ 0x0305, 0x03 },
> @@ -133,7 +132,29 @@ static const struct imx258_reg mipi_data_rate_1267mbps[] = {
>  	{ 0x0823, 0xCC },
>  };
>  
> -static const struct imx258_reg mipi_data_rate_640mbps[] = {
> +static const struct imx258_reg mipi_1272mbps_24mhz[] = {
> +	{ 0x0136, 0x18 },
> +	{ 0x0137, 0x00 },
> +	{ 0x0301, 0x05 },
> +	{ 0x0303, 0x02 },
> +	{ 0x0305, 0x04 },
> +	{ 0x0306, 0x00 },
> +	{ 0x0307, 0xD4 },
> +	{ 0x0309, 0x0A },
> +	{ 0x030B, 0x01 },
> +	{ 0x030D, 0x02 },
> +	{ 0x030E, 0x00 },
> +	{ 0x030F, 0xD8 },
> +	{ 0x0310, 0x00 },
> +	{ 0x0820, 0x13 },
> +	{ 0x0821, 0x4C },
> +	{ 0x0822, 0xCC },
> +	{ 0x0823, 0xCC },
> +};
> +
> +static const struct imx258_reg mipi_640mbps_19_2mhz[] = {
> +	{ 0x0136, 0x13 },
> +	{ 0x0137, 0x33 },
>  	{ 0x0301, 0x05 },
>  	{ 0x0303, 0x02 },
>  	{ 0x0305, 0x03 },
> @@ -151,9 +172,27 @@ static const struct imx258_reg mipi_data_rate_640mbps[] = {
>  	{ 0x0823, 0x00 },
>  };
>  
> +static const struct imx258_reg mipi_642mbps_24mhz[] = {
> +	{ 0x0136, 0x18 },
> +	{ 0x0137, 0x00 },
> +	{ 0x0301, 0x05 },
> +	{ 0x0303, 0x02 },
> +	{ 0x0305, 0x04 },
> +	{ 0x0306, 0x00 },
> +	{ 0x0307, 0x6B },
> +	{ 0x0309, 0x0A },
> +	{ 0x030B, 0x01 },
> +	{ 0x030D, 0x02 },
> +	{ 0x030E, 0x00 },
> +	{ 0x030F, 0xD8 },
> +	{ 0x0310, 0x00 },
> +	{ 0x0820, 0x0A },
> +	{ 0x0821, 0x00 },
> +	{ 0x0822, 0x00 },
> +	{ 0x0823, 0x00 },
> +};
> +
>  static const struct imx258_reg mode_common_regs[] = {
> -	{ 0x0136, 0x13 },
> -	{ 0x0137, 0x33 },
>  	{ 0x3051, 0x00 },
>  	{ 0x3052, 0x00 },
>  	{ 0x4E21, 0x14 },
> @@ -313,10 +352,6 @@ static const char * const imx258_supply_name[] = {
>  
>  #define IMX258_NUM_SUPPLIES ARRAY_SIZE(imx258_supply_name)
>  
> -/* Configurations for supported link frequencies */
> -#define IMX258_LINK_FREQ_634MHZ	633600000ULL
> -#define IMX258_LINK_FREQ_320MHZ	320000000ULL
> -
>  enum {
>  	IMX258_LINK_FREQ_1267MBPS,
>  	IMX258_LINK_FREQ_640MBPS,
> @@ -335,25 +370,55 @@ static u64 link_freq_to_pixel_rate(u64 f)
>  }
>  
>  /* Menu items for LINK_FREQ V4L2 control */
> -static const s64 link_freq_menu_items[] = {
> +/* Configurations for supported link frequencies */
> +#define IMX258_LINK_FREQ_634MHZ	633600000ULL
> +#define IMX258_LINK_FREQ_320MHZ	320000000ULL
> +
> +static const s64 link_freq_menu_items_19_2[] = {
>  	IMX258_LINK_FREQ_634MHZ,
>  	IMX258_LINK_FREQ_320MHZ,
>  };
>  
> +/* Configurations for supported link frequencies */
> +#define IMX258_LINK_FREQ_636MHZ	636000000ULL
> +#define IMX258_LINK_FREQ_321MHZ	321000000ULL

These values aren't used outside the array below and the macro names are
imprecise anyway. Could you put the numerical values to the array instead?

> +
> +static const s64 link_freq_menu_items_24[] = {
> +	IMX258_LINK_FREQ_636MHZ,
> +	IMX258_LINK_FREQ_321MHZ,
> +};
> +
>  /* Link frequency configs */
> -static const struct imx258_link_freq_config link_freq_configs[] = {
> +static const struct imx258_link_freq_config link_freq_configs_19_2[] = {
>  	[IMX258_LINK_FREQ_1267MBPS] = {
>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>  		.reg_list = {
> -			.num_of_regs = ARRAY_SIZE(mipi_data_rate_1267mbps),
> -			.regs = mipi_data_rate_1267mbps,
> +			.num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz),
> +			.regs = mipi_1267mbps_19_2mhz,
>  		}
>  	},
>  	[IMX258_LINK_FREQ_640MBPS] = {
>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>  		.reg_list = {
> -			.num_of_regs = ARRAY_SIZE(mipi_data_rate_640mbps),
> -			.regs = mipi_data_rate_640mbps,
> +			.num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz),
> +			.regs = mipi_640mbps_19_2mhz,
> +		}
> +	},
> +};
> +
> +static const struct imx258_link_freq_config link_freq_configs_24[] = {
> +	[IMX258_LINK_FREQ_1267MBPS] = {
> +		.pixels_per_line = IMX258_PPL_DEFAULT,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz),
> +			.regs = mipi_1272mbps_24mhz,
> +		}
> +	},
> +	[IMX258_LINK_FREQ_640MBPS] = {
> +		.pixels_per_line = IMX258_PPL_DEFAULT,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz),
> +			.regs = mipi_642mbps_24mhz,
>  		}
>  	},
>  };
> @@ -410,6 +475,9 @@ struct imx258 {
>  	/* Current mode */
>  	const struct imx258_mode *cur_mode;
>  
> +	const struct imx258_link_freq_config *link_freq_configs;
> +	const s64 *link_freq_menu_items;
> +
>  	/*
>  	 * Mutex for serialized access:
>  	 * Protect sensor module set pad format and start/stop streaming safely.
> @@ -713,7 +781,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
>  		imx258->cur_mode = mode;
>  		__v4l2_ctrl_s_ctrl(imx258->link_freq, mode->link_freq_index);
>  
> -		link_freq = link_freq_menu_items[mode->link_freq_index];
> +		link_freq = imx258->link_freq_menu_items[mode->link_freq_index];
>  		pixel_rate = link_freq_to_pixel_rate(link_freq);
>  		__v4l2_ctrl_s_ctrl_int64(imx258->pixel_rate, pixel_rate);
>  		/* Update limits and set FPS to default */
> @@ -727,7 +795,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
>  			vblank_def);
>  		__v4l2_ctrl_s_ctrl(imx258->vblank, vblank_def);
>  		h_blank =
> -			link_freq_configs[mode->link_freq_index].pixels_per_line
> +			imx258->link_freq_configs[mode->link_freq_index].pixels_per_line
>  			 - imx258->cur_mode->width;
>  		__v4l2_ctrl_modify_range(imx258->hblank, h_blank,
>  					 h_blank, 1, h_blank);
> @@ -747,7 +815,7 @@ static int imx258_start_streaming(struct imx258 *imx258)
>  
>  	/* Setup PLL */
>  	link_freq_index = imx258->cur_mode->link_freq_index;
> -	reg_list = &link_freq_configs[link_freq_index].reg_list;
> +	reg_list = &imx258->link_freq_configs[link_freq_index].reg_list;
>  	ret = imx258_write_regs(imx258, reg_list->regs, reg_list->num_of_regs);
>  	if (ret) {
>  		dev_err(&client->dev, "%s failed to set plls\n", __func__);
> @@ -946,9 +1014,9 @@ static int imx258_init_controls(struct imx258 *imx258)
>  	imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>  				&imx258_ctrl_ops,
>  				V4L2_CID_LINK_FREQ,
> -				ARRAY_SIZE(link_freq_menu_items) - 1,
> +				ARRAY_SIZE(link_freq_menu_items_19_2) - 1,
>  				0,
> -				link_freq_menu_items);
> +				imx258->link_freq_menu_items);
>  
>  	if (imx258->link_freq)
>  		imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> @@ -964,8 +1032,10 @@ static int imx258_init_controls(struct imx258 *imx258)
>  	if (vflip)
>  		vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  
> -	pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
> -	pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
> +	pixel_rate_max =
> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[0]);
> +	pixel_rate_min =
> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[1]);

The arrays currently have two entries so this works but it'd nice to have a
bit more robust way to handle differences between the two arrays. Could you
maintain e.g. the number of entries in the array in a struct field perhaps?

>  	/* By default, PIXEL_RATE is read only */
>  	imx258->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
>  				V4L2_CID_PIXEL_RATE,
> @@ -1086,8 +1156,19 @@ static int imx258_probe(struct i2c_client *client)
>  	} else {
>  		val = clk_get_rate(imx258->clk);
>  	}
> -	if (val != IMX258_INPUT_CLOCK_FREQ) {
> -		dev_err(&client->dev, "input clock frequency not supported\n");
> +
> +	switch (val) {
> +	case 19200000:
> +		imx258->link_freq_configs = link_freq_configs_19_2;
> +		imx258->link_freq_menu_items = link_freq_menu_items_19_2;
> +		break;
> +	case 24000000:
> +		imx258->link_freq_configs = link_freq_configs_24;
> +		imx258->link_freq_menu_items = link_freq_menu_items_24;
> +		break;
> +	default:
> +		dev_err(&client->dev, "input clock frequency of %u not supported\n",
> +			val);
>  		return -EINVAL;
>  	}
>

Hi Luigi311, Dave,

On Wed, Mar 27, 2024 at 05:16:55PM -0600, git@luigi311.com wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Extends the driver to also support 2 data lanes.
> Frame rates are obviously more restricted on 2 lanes, but some
> hardware simply hasn't wired more up.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Luigi311 <git@luigi311.com>
> ---
>  drivers/media/i2c/imx258.c | 214 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 190 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 6ee7de079454..c65b9aad3b0a 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -86,12 +86,18 @@ struct imx258_reg_list {
>  	const struct imx258_reg *regs;
>  };
>  
> +enum {
> +	IMX258_2_LANE_MODE,
> +	IMX258_4_LANE_MODE,
> +	IMX258_LANE_CONFIGS,
> +};
> +
>  /* Link frequency config */
>  struct imx258_link_freq_config {
>  	u32 pixels_per_line;
>  
>  	/* PLL registers for this link frequency */
> -	struct imx258_reg_list reg_list;
> +	struct imx258_reg_list reg_list[IMX258_LANE_CONFIGS];
>  };
>  
>  /* Mode : resolution and related config&values */
> @@ -111,8 +117,34 @@ struct imx258_mode {
>  	struct imx258_reg_list reg_list;
>  };
>  
> -/* 4208x3120 needs 1267Mbps/lane, 4 lanes */
> -static const struct imx258_reg mipi_1267mbps_19_2mhz[] = {
> +/*
> + * 4208x3120 @ 30 fps needs 1267Mbps/lane, 4 lanes.
> + * To avoid further computation of clock settings, adopt the same per
> + * lane data rate when using 2 lanes, thus allowing a maximum of 15fps.
> + */
> +static const struct imx258_reg mipi_1267mbps_19_2mhz_2l[] = {
> +	{ 0x0136, 0x13 },
> +	{ 0x0137, 0x33 },
> +	{ 0x0301, 0x0A },
> +	{ 0x0303, 0x02 },
> +	{ 0x0305, 0x03 },
> +	{ 0x0306, 0x00 },
> +	{ 0x0307, 0xC6 },
> +	{ 0x0309, 0x0A },
> +	{ 0x030B, 0x01 },
> +	{ 0x030D, 0x02 },
> +	{ 0x030E, 0x00 },
> +	{ 0x030F, 0xD8 },
> +	{ 0x0310, 0x00 },
> +
> +	{ 0x0114, 0x01 },
> +	{ 0x0820, 0x09 },
> +	{ 0x0821, 0xa6 },
> +	{ 0x0822, 0x66 },
> +	{ 0x0823, 0x66 },
> +};
> +
> +static const struct imx258_reg mipi_1267mbps_19_2mhz_4l[] = {
>  	{ 0x0136, 0x13 },
>  	{ 0x0137, 0x33 },
>  	{ 0x0301, 0x05 },
> @@ -126,16 +158,18 @@ static const struct imx258_reg mipi_1267mbps_19_2mhz[] = {
>  	{ 0x030E, 0x00 },
>  	{ 0x030F, 0xD8 },
>  	{ 0x0310, 0x00 },
> +
> +	{ 0x0114, 0x03 },
>  	{ 0x0820, 0x13 },
>  	{ 0x0821, 0x4C },
>  	{ 0x0822, 0xCC },
>  	{ 0x0823, 0xCC },
>  };
>  
> -static const struct imx258_reg mipi_1272mbps_24mhz[] = {
> +static const struct imx258_reg mipi_1272mbps_24mhz_2l[] = {
>  	{ 0x0136, 0x18 },
>  	{ 0x0137, 0x00 },
> -	{ 0x0301, 0x05 },
> +	{ 0x0301, 0x0a },
>  	{ 0x0303, 0x02 },
>  	{ 0x0305, 0x04 },
>  	{ 0x0306, 0x00 },
> @@ -146,13 +180,59 @@ static const struct imx258_reg mipi_1272mbps_24mhz[] = {
>  	{ 0x030E, 0x00 },
>  	{ 0x030F, 0xD8 },
>  	{ 0x0310, 0x00 },
> +
> +	{ 0x0114, 0x01 },
>  	{ 0x0820, 0x13 },
>  	{ 0x0821, 0x4C },
>  	{ 0x0822, 0xCC },
>  	{ 0x0823, 0xCC },
>  };
>  
> -static const struct imx258_reg mipi_640mbps_19_2mhz[] = {
> +static const struct imx258_reg mipi_1272mbps_24mhz_4l[] = {
> +	{ 0x0136, 0x18 },
> +	{ 0x0137, 0x00 },
> +	{ 0x0301, 0x05 },
> +	{ 0x0303, 0x02 },
> +	{ 0x0305, 0x04 },
> +	{ 0x0306, 0x00 },
> +	{ 0x0307, 0xD4 },
> +	{ 0x0309, 0x0A },
> +	{ 0x030B, 0x01 },
> +	{ 0x030D, 0x02 },
> +	{ 0x030E, 0x00 },
> +	{ 0x030F, 0xD8 },
> +	{ 0x0310, 0x00 },
> +
> +	{ 0x0114, 0x03 },
> +	{ 0x0820, 0x13 },
> +	{ 0x0821, 0xE0 },
> +	{ 0x0822, 0x00 },
> +	{ 0x0823, 0x00 },
> +};
> +
> +static const struct imx258_reg mipi_640mbps_19_2mhz_2l[] = {
> +	{ 0x0136, 0x13 },
> +	{ 0x0137, 0x33 },
> +	{ 0x0301, 0x05 },
> +	{ 0x0303, 0x02 },
> +	{ 0x0305, 0x03 },
> +	{ 0x0306, 0x00 },
> +	{ 0x0307, 0x64 },
> +	{ 0x0309, 0x0A },
> +	{ 0x030B, 0x01 },
> +	{ 0x030D, 0x02 },
> +	{ 0x030E, 0x00 },
> +	{ 0x030F, 0xD8 },
> +	{ 0x0310, 0x00 },
> +
> +	{ 0x0114, 0x01 },
> +	{ 0x0820, 0x05 },
> +	{ 0x0821, 0x00 },
> +	{ 0x0822, 0x00 },
> +	{ 0x0823, 0x00 },
> +};
> +
> +static const struct imx258_reg mipi_640mbps_19_2mhz_4l[] = {
>  	{ 0x0136, 0x13 },
>  	{ 0x0137, 0x33 },
>  	{ 0x0301, 0x05 },
> @@ -166,13 +246,37 @@ static const struct imx258_reg mipi_640mbps_19_2mhz[] = {
>  	{ 0x030E, 0x00 },
>  	{ 0x030F, 0xD8 },
>  	{ 0x0310, 0x00 },
> +
> +	{ 0x0114, 0x03 },
> +	{ 0x0820, 0x0A },
> +	{ 0x0821, 0x00 },
> +	{ 0x0822, 0x00 },
> +	{ 0x0823, 0x00 },
> +};
> +
> +static const struct imx258_reg mipi_642mbps_24mhz_2l[] = {
> +	{ 0x0136, 0x18 },
> +	{ 0x0137, 0x00 },
> +	{ 0x0301, 0x0A },
> +	{ 0x0303, 0x02 },
> +	{ 0x0305, 0x04 },
> +	{ 0x0306, 0x00 },
> +	{ 0x0307, 0x6B },
> +	{ 0x0309, 0x0A },
> +	{ 0x030B, 0x01 },
> +	{ 0x030D, 0x02 },
> +	{ 0x030E, 0x00 },
> +	{ 0x030F, 0xD8 },
> +	{ 0x0310, 0x00 },
> +
> +	{ 0x0114, 0x01 },
>  	{ 0x0820, 0x0A },
>  	{ 0x0821, 0x00 },
>  	{ 0x0822, 0x00 },
>  	{ 0x0823, 0x00 },
>  };
>  
> -static const struct imx258_reg mipi_642mbps_24mhz[] = {
> +static const struct imx258_reg mipi_642mbps_24mhz_4l[] = {
>  	{ 0x0136, 0x18 },
>  	{ 0x0137, 0x00 },
>  	{ 0x0301, 0x05 },
> @@ -186,6 +290,8 @@ static const struct imx258_reg mipi_642mbps_24mhz[] = {
>  	{ 0x030E, 0x00 },
>  	{ 0x030F, 0xD8 },
>  	{ 0x0310, 0x00 },
> +
> +	{ 0x0114, 0x03 },
>  	{ 0x0820, 0x0A },
>  	{ 0x0821, 0x00 },
>  	{ 0x0822, 0x00 },
> @@ -240,7 +346,6 @@ static const struct imx258_reg mode_common_regs[] = {
>  	{ 0x5F05, 0xED },
>  	{ 0x0112, 0x0A },
>  	{ 0x0113, 0x0A },
> -	{ 0x0114, 0x03 },
>  	{ 0x0342, 0x14 },
>  	{ 0x0343, 0xE8 },
>  	{ 0x0344, 0x00 },
> @@ -359,11 +464,13 @@ enum {
>  
>  /*
>   * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample
> - * data rate => double data rate; number of lanes => 4; bits per pixel => 10
> + * data rate => double data rate;
> + * number of lanes => (configurable 2 or 4);
> + * bits per pixel => 10
>   */
> -static u64 link_freq_to_pixel_rate(u64 f)
> +static u64 link_freq_to_pixel_rate(u64 f, unsigned int nlanes)
>  {
> -	f *= 2 * 4;
> +	f *= 2 * nlanes;
>  	do_div(f, 10);
>  
>  	return f;
> @@ -393,15 +500,27 @@ static const struct imx258_link_freq_config link_freq_configs_19_2[] = {
>  	[IMX258_LINK_FREQ_1267MBPS] = {
>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>  		.reg_list = {
> -			.num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz),
> -			.regs = mipi_1267mbps_19_2mhz,
> +			[IMX258_2_LANE_MODE] = {
> +				.num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz_2l),
> +				.regs = mipi_1267mbps_19_2mhz_2l,
> +			},
> +			[IMX258_4_LANE_MODE] = {
> +				.num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz_4l),
> +				.regs = mipi_1267mbps_19_2mhz_4l,
> +			},
>  		}
>  	},
>  	[IMX258_LINK_FREQ_640MBPS] = {
>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>  		.reg_list = {
> -			.num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz),
> -			.regs = mipi_640mbps_19_2mhz,
> +			[IMX258_2_LANE_MODE] = {
> +				.num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz_2l),
> +				.regs = mipi_640mbps_19_2mhz_2l,
> +			},
> +			[IMX258_4_LANE_MODE] = {
> +				.num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz_4l),
> +				.regs = mipi_640mbps_19_2mhz_4l,
> +			},
>  		}
>  	},
>  };
> @@ -410,15 +529,27 @@ static const struct imx258_link_freq_config link_freq_configs_24[] = {
>  	[IMX258_LINK_FREQ_1267MBPS] = {
>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>  		.reg_list = {
> -			.num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz),
> -			.regs = mipi_1272mbps_24mhz,
> +			[IMX258_2_LANE_MODE] = {
> +				.num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz_2l),
> +				.regs = mipi_1272mbps_24mhz_2l,
> +			},
> +			[IMX258_4_LANE_MODE] = {
> +				.num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz_4l),
> +				.regs = mipi_1272mbps_24mhz_4l,
> +			},
>  		}
>  	},
>  	[IMX258_LINK_FREQ_640MBPS] = {
>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>  		.reg_list = {
> -			.num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz),
> -			.regs = mipi_642mbps_24mhz,
> +			[IMX258_2_LANE_MODE] = {
> +				.num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz_2l),
> +				.regs = mipi_642mbps_24mhz_2l,
> +			},
> +			[IMX258_4_LANE_MODE] = {
> +				.num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz_4l),
> +				.regs = mipi_642mbps_24mhz_4l,
> +			},
>  		}
>  	},
>  };
> @@ -477,6 +608,7 @@ struct imx258 {
>  
>  	const struct imx258_link_freq_config *link_freq_configs;
>  	const s64 *link_freq_menu_items;
> +	unsigned int nlanes;
>  
>  	/*
>  	 * Mutex for serialized access:
> @@ -782,7 +914,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
>  		__v4l2_ctrl_s_ctrl(imx258->link_freq, mode->link_freq_index);
>  
>  		link_freq = imx258->link_freq_menu_items[mode->link_freq_index];
> -		pixel_rate = link_freq_to_pixel_rate(link_freq);
> +		pixel_rate = link_freq_to_pixel_rate(link_freq, imx258->nlanes);
>  		__v4l2_ctrl_s_ctrl_int64(imx258->pixel_rate, pixel_rate);
>  		/* Update limits and set FPS to default */
>  		vblank_def = imx258->cur_mode->vts_def -
> @@ -811,11 +943,13 @@ static int imx258_start_streaming(struct imx258 *imx258)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
>  	const struct imx258_reg_list *reg_list;
> +	const struct imx258_link_freq_config *link_freq_cfg;
>  	int ret, link_freq_index;
>  
>  	/* Setup PLL */
>  	link_freq_index = imx258->cur_mode->link_freq_index;
> -	reg_list = &imx258->link_freq_configs[link_freq_index].reg_list;
> +	link_freq_cfg = &imx258->link_freq_configs[link_freq_index];
> +	reg_list = &link_freq_cfg->reg_list[imx258->nlanes == 2 ? 0 : 1];
>  	ret = imx258_write_regs(imx258, reg_list->regs, reg_list->num_of_regs);
>  	if (ret) {
>  		dev_err(&client->dev, "%s failed to set plls\n", __func__);
> @@ -1033,9 +1167,11 @@ static int imx258_init_controls(struct imx258 *imx258)
>  		vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  
>  	pixel_rate_max =
> -		link_freq_to_pixel_rate(imx258->link_freq_menu_items[0]);
> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[0],
> +					imx258->nlanes);
>  	pixel_rate_min =
> -		link_freq_to_pixel_rate(imx258->link_freq_menu_items[1]);
> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[1],
> +					imx258->nlanes);
>  	/* By default, PIXEL_RATE is read only */
>  	imx258->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
>  				V4L2_CID_PIXEL_RATE,
> @@ -1132,6 +1268,10 @@ static int imx258_get_regulators(struct imx258 *imx258,
>  static int imx258_probe(struct i2c_client *client)
>  {
>  	struct imx258 *imx258;
> +	struct fwnode_handle *endpoint;
> +	struct v4l2_fwnode_endpoint ep = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY
> +	};
>  	int ret;
>  	u32 val = 0;
>  
> @@ -1172,13 +1312,35 @@ static int imx258_probe(struct i2c_client *client)
>  		return -EINVAL;
>  	}
>  
> +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
> +	if (!endpoint) {
> +		dev_err(&client->dev, "Endpoint node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);

Here you're obtaining the list of supported link frequencies from the
firmware but it is not validated (nor it was validated by the driver
previously). I'd regard that a driver bug but fixing it at this point could
introduce adverse effects elsewhere.

I think what I'd do here is that I'd ignore the issue if there are no
frequencies defined for the endpoint but if there are, then enable only
those that are listed in the endpoint.

Could you add a patch to do this, please? v4l2_link_freq_to_bitmap() has
been recently added to facilitate this.

> +	fwnode_handle_put(endpoint);
> +	if (ret) {
> +		dev_err(&client->dev, "Parsing endpoint node failed\n");
> +		return ret;
> +	}
> +
> +	/* Get number of data lanes */
> +	imx258->nlanes = ep.bus.mipi_csi2.num_data_lanes;
> +	if (imx258->nlanes != 2 && imx258->nlanes != 4) {
> +		dev_err(&client->dev, "Invalid data lanes: %u\n",
> +			imx258->nlanes);
> +		ret = -EINVAL;
> +		goto error_endpoint_free;
> +	}
> +
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
>  
>  	/* Will be powered off via pm_runtime_idle */
>  	ret = imx258_power_on(&client->dev);
>  	if (ret)
> -		return ret;
> +		goto error_endpoint_free;
>  
>  	/* Check module identity */
>  	ret = imx258_identify_module(imx258);
> @@ -1211,6 +1373,7 @@ static int imx258_probe(struct i2c_client *client)
>  	pm_runtime_set_active(&client->dev);
>  	pm_runtime_enable(&client->dev);
>  	pm_runtime_idle(&client->dev);
> +	v4l2_fwnode_endpoint_free(&ep);
>  
>  	return 0;
>  
> @@ -1223,6 +1386,9 @@ static int imx258_probe(struct i2c_client *client)
>  error_identify:
>  	imx258_power_off(&client->dev);
>  
> +error_endpoint_free:
> +	v4l2_fwnode_endpoint_free(&ep);
> +
>  	return ret;
>  }
>

On 3/28/24 02:09, Sakari Ailus wrote:
> Hi Luigi311,
> 
> Thank you for the patchset.
> 
> On Wed, Mar 27, 2024 at 05:16:54PM -0600, git@luigi311.com wrote:
>> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>
>> There's no reason why only a clock of 19.2MHz is supported.
>> Indeed this isn't even a frequency listed in the datasheet.
>>
>> Add support for 24MHz as well.
>> The PLL settings result in slightly different link frequencies,
>> so parameterise those.
>>
>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>> Signed-off-by: Luigi311 <git@luigi311.com>
> 
> Is Luigi311 your real name? As per
> Documentation/process/submitting-patches.rst, anonymous (or pseudonym I'd
> say as well) contributions are not an option.

Luigi311 is not my real name but it would be a lot easier to find me if
it was. My real name is Luis Garcia which is a super common name so its
actually way easier to find me and all my work using my online name of
Luigi311. I can go ahead and swap over to Luis Garcia if required but a
name like that would provide no value in contacting/finding me since I'm
not famous like all the other Luis Garcia's that appear on google.

> 
>> ---
>>  drivers/media/i2c/imx258.c | 133 +++++++++++++++++++++++++++++--------
>>  1 file changed, 107 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>> index 351add1bc5d5..6ee7de079454 100644
>> --- a/drivers/media/i2c/imx258.c
>> +++ b/drivers/media/i2c/imx258.c
>> @@ -76,9 +76,6 @@
>>  #define REG_CONFIG_MIRROR_FLIP		0x03
>>  #define REG_CONFIG_FLIP_TEST_PATTERN	0x02
>>  
>> -/* Input clock frequency in Hz */
>> -#define IMX258_INPUT_CLOCK_FREQ		19200000
>> -
>>  struct imx258_reg {
>>  	u16 address;
>>  	u8 val;
>> @@ -115,7 +112,9 @@ struct imx258_mode {
>>  };
>>  
>>  /* 4208x3120 needs 1267Mbps/lane, 4 lanes */
>> -static const struct imx258_reg mipi_data_rate_1267mbps[] = {
>> +static const struct imx258_reg mipi_1267mbps_19_2mhz[] = {
>> +	{ 0x0136, 0x13 },
>> +	{ 0x0137, 0x33 },
>>  	{ 0x0301, 0x05 },
>>  	{ 0x0303, 0x02 },
>>  	{ 0x0305, 0x03 },
>> @@ -133,7 +132,29 @@ static const struct imx258_reg mipi_data_rate_1267mbps[] = {
>>  	{ 0x0823, 0xCC },
>>  };
>>  
>> -static const struct imx258_reg mipi_data_rate_640mbps[] = {
>> +static const struct imx258_reg mipi_1272mbps_24mhz[] = {
>> +	{ 0x0136, 0x18 },
>> +	{ 0x0137, 0x00 },
>> +	{ 0x0301, 0x05 },
>> +	{ 0x0303, 0x02 },
>> +	{ 0x0305, 0x04 },
>> +	{ 0x0306, 0x00 },
>> +	{ 0x0307, 0xD4 },
>> +	{ 0x0309, 0x0A },
>> +	{ 0x030B, 0x01 },
>> +	{ 0x030D, 0x02 },
>> +	{ 0x030E, 0x00 },
>> +	{ 0x030F, 0xD8 },
>> +	{ 0x0310, 0x00 },
>> +	{ 0x0820, 0x13 },
>> +	{ 0x0821, 0x4C },
>> +	{ 0x0822, 0xCC },
>> +	{ 0x0823, 0xCC },
>> +};
>> +
>> +static const struct imx258_reg mipi_640mbps_19_2mhz[] = {
>> +	{ 0x0136, 0x13 },
>> +	{ 0x0137, 0x33 },
>>  	{ 0x0301, 0x05 },
>>  	{ 0x0303, 0x02 },
>>  	{ 0x0305, 0x03 },
>> @@ -151,9 +172,27 @@ static const struct imx258_reg mipi_data_rate_640mbps[] = {
>>  	{ 0x0823, 0x00 },
>>  };
>>  
>> +static const struct imx258_reg mipi_642mbps_24mhz[] = {
>> +	{ 0x0136, 0x18 },
>> +	{ 0x0137, 0x00 },
>> +	{ 0x0301, 0x05 },
>> +	{ 0x0303, 0x02 },
>> +	{ 0x0305, 0x04 },
>> +	{ 0x0306, 0x00 },
>> +	{ 0x0307, 0x6B },
>> +	{ 0x0309, 0x0A },
>> +	{ 0x030B, 0x01 },
>> +	{ 0x030D, 0x02 },
>> +	{ 0x030E, 0x00 },
>> +	{ 0x030F, 0xD8 },
>> +	{ 0x0310, 0x00 },
>> +	{ 0x0820, 0x0A },
>> +	{ 0x0821, 0x00 },
>> +	{ 0x0822, 0x00 },
>> +	{ 0x0823, 0x00 },
>> +};
>> +
>>  static const struct imx258_reg mode_common_regs[] = {
>> -	{ 0x0136, 0x13 },
>> -	{ 0x0137, 0x33 },
>>  	{ 0x3051, 0x00 },
>>  	{ 0x3052, 0x00 },
>>  	{ 0x4E21, 0x14 },
>> @@ -313,10 +352,6 @@ static const char * const imx258_supply_name[] = {
>>  
>>  #define IMX258_NUM_SUPPLIES ARRAY_SIZE(imx258_supply_name)
>>  
>> -/* Configurations for supported link frequencies */
>> -#define IMX258_LINK_FREQ_634MHZ	633600000ULL
>> -#define IMX258_LINK_FREQ_320MHZ	320000000ULL
>> -
>>  enum {
>>  	IMX258_LINK_FREQ_1267MBPS,
>>  	IMX258_LINK_FREQ_640MBPS,
>> @@ -335,25 +370,55 @@ static u64 link_freq_to_pixel_rate(u64 f)
>>  }
>>  
>>  /* Menu items for LINK_FREQ V4L2 control */
>> -static const s64 link_freq_menu_items[] = {
>> +/* Configurations for supported link frequencies */
>> +#define IMX258_LINK_FREQ_634MHZ	633600000ULL
>> +#define IMX258_LINK_FREQ_320MHZ	320000000ULL
>> +
>> +static const s64 link_freq_menu_items_19_2[] = {
>>  	IMX258_LINK_FREQ_634MHZ,
>>  	IMX258_LINK_FREQ_320MHZ,
>>  };
>>  
>> +/* Configurations for supported link frequencies */
>> +#define IMX258_LINK_FREQ_636MHZ	636000000ULL
>> +#define IMX258_LINK_FREQ_321MHZ	321000000ULL
> 
> These values aren't used outside the array below and the macro names are
> imprecise anyway. Could you put the numerical values to the array instead?

Ok I've removed the defines and just threw the values into the array instead.

> 
>> +
>> +static const s64 link_freq_menu_items_24[] = {
>> +	IMX258_LINK_FREQ_636MHZ,
>> +	IMX258_LINK_FREQ_321MHZ,
>> +};
>> +
>>  /* Link frequency configs */
>> -static const struct imx258_link_freq_config link_freq_configs[] = {
>> +static const struct imx258_link_freq_config link_freq_configs_19_2[] = {
>>  	[IMX258_LINK_FREQ_1267MBPS] = {
>>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>>  		.reg_list = {
>> -			.num_of_regs = ARRAY_SIZE(mipi_data_rate_1267mbps),
>> -			.regs = mipi_data_rate_1267mbps,
>> +			.num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz),
>> +			.regs = mipi_1267mbps_19_2mhz,
>>  		}
>>  	},
>>  	[IMX258_LINK_FREQ_640MBPS] = {
>>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>>  		.reg_list = {
>> -			.num_of_regs = ARRAY_SIZE(mipi_data_rate_640mbps),
>> -			.regs = mipi_data_rate_640mbps,
>> +			.num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz),
>> +			.regs = mipi_640mbps_19_2mhz,
>> +		}
>> +	},
>> +};
>> +
>> +static const struct imx258_link_freq_config link_freq_configs_24[] = {
>> +	[IMX258_LINK_FREQ_1267MBPS] = {
>> +		.pixels_per_line = IMX258_PPL_DEFAULT,
>> +		.reg_list = {
>> +			.num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz),
>> +			.regs = mipi_1272mbps_24mhz,
>> +		}
>> +	},
>> +	[IMX258_LINK_FREQ_640MBPS] = {
>> +		.pixels_per_line = IMX258_PPL_DEFAULT,
>> +		.reg_list = {
>> +			.num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz),
>> +			.regs = mipi_642mbps_24mhz,
>>  		}
>>  	},
>>  };
>> @@ -410,6 +475,9 @@ struct imx258 {
>>  	/* Current mode */
>>  	const struct imx258_mode *cur_mode;
>>  
>> +	const struct imx258_link_freq_config *link_freq_configs;
>> +	const s64 *link_freq_menu_items;
>> +
>>  	/*
>>  	 * Mutex for serialized access:
>>  	 * Protect sensor module set pad format and start/stop streaming safely.
>> @@ -713,7 +781,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
>>  		imx258->cur_mode = mode;
>>  		__v4l2_ctrl_s_ctrl(imx258->link_freq, mode->link_freq_index);
>>  
>> -		link_freq = link_freq_menu_items[mode->link_freq_index];
>> +		link_freq = imx258->link_freq_menu_items[mode->link_freq_index];
>>  		pixel_rate = link_freq_to_pixel_rate(link_freq);
>>  		__v4l2_ctrl_s_ctrl_int64(imx258->pixel_rate, pixel_rate);
>>  		/* Update limits and set FPS to default */
>> @@ -727,7 +795,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
>>  			vblank_def);
>>  		__v4l2_ctrl_s_ctrl(imx258->vblank, vblank_def);
>>  		h_blank =
>> -			link_freq_configs[mode->link_freq_index].pixels_per_line
>> +			imx258->link_freq_configs[mode->link_freq_index].pixels_per_line
>>  			 - imx258->cur_mode->width;
>>  		__v4l2_ctrl_modify_range(imx258->hblank, h_blank,
>>  					 h_blank, 1, h_blank);
>> @@ -747,7 +815,7 @@ static int imx258_start_streaming(struct imx258 *imx258)
>>  
>>  	/* Setup PLL */
>>  	link_freq_index = imx258->cur_mode->link_freq_index;
>> -	reg_list = &link_freq_configs[link_freq_index].reg_list;
>> +	reg_list = &imx258->link_freq_configs[link_freq_index].reg_list;
>>  	ret = imx258_write_regs(imx258, reg_list->regs, reg_list->num_of_regs);
>>  	if (ret) {
>>  		dev_err(&client->dev, "%s failed to set plls\n", __func__);
>> @@ -946,9 +1014,9 @@ static int imx258_init_controls(struct imx258 *imx258)
>>  	imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>>  				&imx258_ctrl_ops,
>>  				V4L2_CID_LINK_FREQ,
>> -				ARRAY_SIZE(link_freq_menu_items) - 1,
>> +				ARRAY_SIZE(link_freq_menu_items_19_2) - 1,
>>  				0,
>> -				link_freq_menu_items);
>> +				imx258->link_freq_menu_items);
>>  
>>  	if (imx258->link_freq)
>>  		imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>> @@ -964,8 +1032,10 @@ static int imx258_init_controls(struct imx258 *imx258)
>>  	if (vflip)
>>  		vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>  
>> -	pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
>> -	pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
>> +	pixel_rate_max =
>> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[0]);
>> +	pixel_rate_min =
>> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[1]);
> 
> The arrays currently have two entries so this works but it'd nice to have a
> bit more robust way to handle differences between the two arrays. Could you
> maintain e.g. the number of entries in the array in a struct field perhaps?

Would it make more sense to do something like default to index 0 and then use 
ARRAY_SIZE to iterate through the array and do a comparison to get the min and
max size so it would always choose the correct value no matter how many entries
there are?

> 
>>  	/* By default, PIXEL_RATE is read only */
>>  	imx258->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
>>  				V4L2_CID_PIXEL_RATE,
>> @@ -1086,8 +1156,19 @@ static int imx258_probe(struct i2c_client *client)
>>  	} else {
>>  		val = clk_get_rate(imx258->clk);
>>  	}
>> -	if (val != IMX258_INPUT_CLOCK_FREQ) {
>> -		dev_err(&client->dev, "input clock frequency not supported\n");
>> +
>> +	switch (val) {
>> +	case 19200000:
>> +		imx258->link_freq_configs = link_freq_configs_19_2;
>> +		imx258->link_freq_menu_items = link_freq_menu_items_19_2;
>> +		break;
>> +	case 24000000:
>> +		imx258->link_freq_configs = link_freq_configs_24;
>> +		imx258->link_freq_menu_items = link_freq_menu_items_24;
>> +		break;
>> +	default:
>> +		dev_err(&client->dev, "input clock frequency of %u not supported\n",
>> +			val);
>>  		return -EINVAL;
>>  	}
>>  
>

On 3/28/24 11:55, Luigi311 wrote:
> On 3/28/24 02:09, Sakari Ailus wrote:
>> Hi Luigi311,
>>
>> Thank you for the patchset.
>>
>> On Wed, Mar 27, 2024 at 05:16:54PM -0600, git@luigi311.com wrote:
>>> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>>
>>> There's no reason why only a clock of 19.2MHz is supported.
>>> Indeed this isn't even a frequency listed in the datasheet.
>>>
>>> Add support for 24MHz as well.
>>> The PLL settings result in slightly different link frequencies,
>>> so parameterise those.
>>>
>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> Signed-off-by: Luigi311 <git@luigi311.com>
>>
>> Is Luigi311 your real name? As per
>> Documentation/process/submitting-patches.rst, anonymous (or pseudonym I'd
>> say as well) contributions are not an option.
> 
> Luigi311 is not my real name but it would be a lot easier to find me if
> it was. My real name is Luis Garcia which is a super common name so its
> actually way easier to find me and all my work using my online name of
> Luigi311. I can go ahead and swap over to Luis Garcia if required but a
> name like that would provide no value in contacting/finding me since I'm
> not famous like all the other Luis Garcia's that appear on google.
> 
>>
>>> ---
>>>  drivers/media/i2c/imx258.c | 133 +++++++++++++++++++++++++++++--------
>>>  1 file changed, 107 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>>> index 351add1bc5d5..6ee7de079454 100644
>>> --- a/drivers/media/i2c/imx258.c
>>> +++ b/drivers/media/i2c/imx258.c
>>> @@ -76,9 +76,6 @@
>>>  #define REG_CONFIG_MIRROR_FLIP		0x03
>>>  #define REG_CONFIG_FLIP_TEST_PATTERN	0x02
>>>  
>>> -/* Input clock frequency in Hz */
>>> -#define IMX258_INPUT_CLOCK_FREQ		19200000
>>> -
>>>  struct imx258_reg {
>>>  	u16 address;
>>>  	u8 val;
>>> @@ -115,7 +112,9 @@ struct imx258_mode {
>>>  };
>>>  
>>>  /* 4208x3120 needs 1267Mbps/lane, 4 lanes */
>>> -static const struct imx258_reg mipi_data_rate_1267mbps[] = {
>>> +static const struct imx258_reg mipi_1267mbps_19_2mhz[] = {
>>> +	{ 0x0136, 0x13 },
>>> +	{ 0x0137, 0x33 },
>>>  	{ 0x0301, 0x05 },
>>>  	{ 0x0303, 0x02 },
>>>  	{ 0x0305, 0x03 },
>>> @@ -133,7 +132,29 @@ static const struct imx258_reg mipi_data_rate_1267mbps[] = {
>>>  	{ 0x0823, 0xCC },
>>>  };
>>>  
>>> -static const struct imx258_reg mipi_data_rate_640mbps[] = {
>>> +static const struct imx258_reg mipi_1272mbps_24mhz[] = {
>>> +	{ 0x0136, 0x18 },
>>> +	{ 0x0137, 0x00 },
>>> +	{ 0x0301, 0x05 },
>>> +	{ 0x0303, 0x02 },
>>> +	{ 0x0305, 0x04 },
>>> +	{ 0x0306, 0x00 },
>>> +	{ 0x0307, 0xD4 },
>>> +	{ 0x0309, 0x0A },
>>> +	{ 0x030B, 0x01 },
>>> +	{ 0x030D, 0x02 },
>>> +	{ 0x030E, 0x00 },
>>> +	{ 0x030F, 0xD8 },
>>> +	{ 0x0310, 0x00 },
>>> +	{ 0x0820, 0x13 },
>>> +	{ 0x0821, 0x4C },
>>> +	{ 0x0822, 0xCC },
>>> +	{ 0x0823, 0xCC },
>>> +};
>>> +
>>> +static const struct imx258_reg mipi_640mbps_19_2mhz[] = {
>>> +	{ 0x0136, 0x13 },
>>> +	{ 0x0137, 0x33 },
>>>  	{ 0x0301, 0x05 },
>>>  	{ 0x0303, 0x02 },
>>>  	{ 0x0305, 0x03 },
>>> @@ -151,9 +172,27 @@ static const struct imx258_reg mipi_data_rate_640mbps[] = {
>>>  	{ 0x0823, 0x00 },
>>>  };
>>>  
>>> +static const struct imx258_reg mipi_642mbps_24mhz[] = {
>>> +	{ 0x0136, 0x18 },
>>> +	{ 0x0137, 0x00 },
>>> +	{ 0x0301, 0x05 },
>>> +	{ 0x0303, 0x02 },
>>> +	{ 0x0305, 0x04 },
>>> +	{ 0x0306, 0x00 },
>>> +	{ 0x0307, 0x6B },
>>> +	{ 0x0309, 0x0A },
>>> +	{ 0x030B, 0x01 },
>>> +	{ 0x030D, 0x02 },
>>> +	{ 0x030E, 0x00 },
>>> +	{ 0x030F, 0xD8 },
>>> +	{ 0x0310, 0x00 },
>>> +	{ 0x0820, 0x0A },
>>> +	{ 0x0821, 0x00 },
>>> +	{ 0x0822, 0x00 },
>>> +	{ 0x0823, 0x00 },
>>> +};
>>> +
>>>  static const struct imx258_reg mode_common_regs[] = {
>>> -	{ 0x0136, 0x13 },
>>> -	{ 0x0137, 0x33 },
>>>  	{ 0x3051, 0x00 },
>>>  	{ 0x3052, 0x00 },
>>>  	{ 0x4E21, 0x14 },
>>> @@ -313,10 +352,6 @@ static const char * const imx258_supply_name[] = {
>>>  
>>>  #define IMX258_NUM_SUPPLIES ARRAY_SIZE(imx258_supply_name)
>>>  
>>> -/* Configurations for supported link frequencies */
>>> -#define IMX258_LINK_FREQ_634MHZ	633600000ULL
>>> -#define IMX258_LINK_FREQ_320MHZ	320000000ULL
>>> -
>>>  enum {
>>>  	IMX258_LINK_FREQ_1267MBPS,
>>>  	IMX258_LINK_FREQ_640MBPS,
>>> @@ -335,25 +370,55 @@ static u64 link_freq_to_pixel_rate(u64 f)
>>>  }
>>>  
>>>  /* Menu items for LINK_FREQ V4L2 control */
>>> -static const s64 link_freq_menu_items[] = {
>>> +/* Configurations for supported link frequencies */
>>> +#define IMX258_LINK_FREQ_634MHZ	633600000ULL
>>> +#define IMX258_LINK_FREQ_320MHZ	320000000ULL
>>> +
>>> +static const s64 link_freq_menu_items_19_2[] = {
>>>  	IMX258_LINK_FREQ_634MHZ,
>>>  	IMX258_LINK_FREQ_320MHZ,
>>>  };
>>>  
>>> +/* Configurations for supported link frequencies */
>>> +#define IMX258_LINK_FREQ_636MHZ	636000000ULL
>>> +#define IMX258_LINK_FREQ_321MHZ	321000000ULL
>>
>> These values aren't used outside the array below and the macro names are
>> imprecise anyway. Could you put the numerical values to the array instead?
> 
> Ok I've removed the defines and just threw the values into the array instead.
> 
>>
>>> +
>>> +static const s64 link_freq_menu_items_24[] = {
>>> +	IMX258_LINK_FREQ_636MHZ,
>>> +	IMX258_LINK_FREQ_321MHZ,
>>> +};
>>> +
>>>  /* Link frequency configs */
>>> -static const struct imx258_link_freq_config link_freq_configs[] = {
>>> +static const struct imx258_link_freq_config link_freq_configs_19_2[] = {
>>>  	[IMX258_LINK_FREQ_1267MBPS] = {
>>>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>>>  		.reg_list = {
>>> -			.num_of_regs = ARRAY_SIZE(mipi_data_rate_1267mbps),
>>> -			.regs = mipi_data_rate_1267mbps,
>>> +			.num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz),
>>> +			.regs = mipi_1267mbps_19_2mhz,
>>>  		}
>>>  	},
>>>  	[IMX258_LINK_FREQ_640MBPS] = {
>>>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>>>  		.reg_list = {
>>> -			.num_of_regs = ARRAY_SIZE(mipi_data_rate_640mbps),
>>> -			.regs = mipi_data_rate_640mbps,
>>> +			.num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz),
>>> +			.regs = mipi_640mbps_19_2mhz,
>>> +		}
>>> +	},
>>> +};
>>> +
>>> +static const struct imx258_link_freq_config link_freq_configs_24[] = {
>>> +	[IMX258_LINK_FREQ_1267MBPS] = {
>>> +		.pixels_per_line = IMX258_PPL_DEFAULT,
>>> +		.reg_list = {
>>> +			.num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz),
>>> +			.regs = mipi_1272mbps_24mhz,
>>> +		}
>>> +	},
>>> +	[IMX258_LINK_FREQ_640MBPS] = {
>>> +		.pixels_per_line = IMX258_PPL_DEFAULT,
>>> +		.reg_list = {
>>> +			.num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz),
>>> +			.regs = mipi_642mbps_24mhz,
>>>  		}
>>>  	},
>>>  };
>>> @@ -410,6 +475,9 @@ struct imx258 {
>>>  	/* Current mode */
>>>  	const struct imx258_mode *cur_mode;
>>>  
>>> +	const struct imx258_link_freq_config *link_freq_configs;
>>> +	const s64 *link_freq_menu_items;
>>> +
>>>  	/*
>>>  	 * Mutex for serialized access:
>>>  	 * Protect sensor module set pad format and start/stop streaming safely.
>>> @@ -713,7 +781,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
>>>  		imx258->cur_mode = mode;
>>>  		__v4l2_ctrl_s_ctrl(imx258->link_freq, mode->link_freq_index);
>>>  
>>> -		link_freq = link_freq_menu_items[mode->link_freq_index];
>>> +		link_freq = imx258->link_freq_menu_items[mode->link_freq_index];
>>>  		pixel_rate = link_freq_to_pixel_rate(link_freq);
>>>  		__v4l2_ctrl_s_ctrl_int64(imx258->pixel_rate, pixel_rate);
>>>  		/* Update limits and set FPS to default */
>>> @@ -727,7 +795,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
>>>  			vblank_def);
>>>  		__v4l2_ctrl_s_ctrl(imx258->vblank, vblank_def);
>>>  		h_blank =
>>> -			link_freq_configs[mode->link_freq_index].pixels_per_line
>>> +			imx258->link_freq_configs[mode->link_freq_index].pixels_per_line
>>>  			 - imx258->cur_mode->width;
>>>  		__v4l2_ctrl_modify_range(imx258->hblank, h_blank,
>>>  					 h_blank, 1, h_blank);
>>> @@ -747,7 +815,7 @@ static int imx258_start_streaming(struct imx258 *imx258)
>>>  
>>>  	/* Setup PLL */
>>>  	link_freq_index = imx258->cur_mode->link_freq_index;
>>> -	reg_list = &link_freq_configs[link_freq_index].reg_list;
>>> +	reg_list = &imx258->link_freq_configs[link_freq_index].reg_list;
>>>  	ret = imx258_write_regs(imx258, reg_list->regs, reg_list->num_of_regs);
>>>  	if (ret) {
>>>  		dev_err(&client->dev, "%s failed to set plls\n", __func__);
>>> @@ -946,9 +1014,9 @@ static int imx258_init_controls(struct imx258 *imx258)
>>>  	imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>>>  				&imx258_ctrl_ops,
>>>  				V4L2_CID_LINK_FREQ,
>>> -				ARRAY_SIZE(link_freq_menu_items) - 1,
>>> +				ARRAY_SIZE(link_freq_menu_items_19_2) - 1,
>>>  				0,
>>> -				link_freq_menu_items);
>>> +				imx258->link_freq_menu_items);
>>>  
>>>  	if (imx258->link_freq)
>>>  		imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>> @@ -964,8 +1032,10 @@ static int imx258_init_controls(struct imx258 *imx258)
>>>  	if (vflip)
>>>  		vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>>  
>>> -	pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
>>> -	pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
>>> +	pixel_rate_max =
>>> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[0]);
>>> +	pixel_rate_min =
>>> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[1]);
>>
>> The arrays currently have two entries so this works but it'd nice to have a
>> bit more robust way to handle differences between the two arrays. Could you
>> maintain e.g. the number of entries in the array in a struct field perhaps?
> 
> Would it make more sense to do something like default to index 0 and then use 
> ARRAY_SIZE to iterate through the array and do a comparison to get the min and
> max size so it would always choose the correct value no matter how many entries
> there are?
> 

Actually down the patch series 15/23 set pixel_rate range to the same as the value,
changes the logic and removes those two lines all together

>>
>>>  	/* By default, PIXEL_RATE is read only */
>>>  	imx258->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
>>>  				V4L2_CID_PIXEL_RATE,
>>> @@ -1086,8 +1156,19 @@ static int imx258_probe(struct i2c_client *client)
>>>  	} else {
>>>  		val = clk_get_rate(imx258->clk);
>>>  	}
>>> -	if (val != IMX258_INPUT_CLOCK_FREQ) {
>>> -		dev_err(&client->dev, "input clock frequency not supported\n");
>>> +
>>> +	switch (val) {
>>> +	case 19200000:
>>> +		imx258->link_freq_configs = link_freq_configs_19_2;
>>> +		imx258->link_freq_menu_items = link_freq_menu_items_19_2;
>>> +		break;
>>> +	case 24000000:
>>> +		imx258->link_freq_configs = link_freq_configs_24;
>>> +		imx258->link_freq_menu_items = link_freq_menu_items_24;
>>> +		break;
>>> +	default:
>>> +		dev_err(&client->dev, "input clock frequency of %u not supported\n",
>>> +			val);
>>>  		return -EINVAL;
>>>  	}
>>>  
>>
>

On 3/28/24 02:19, Sakari Ailus wrote:
> Hi Luigi311, Dave,
> 
> On Wed, Mar 27, 2024 at 05:16:55PM -0600, git@luigi311.com wrote:
>> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>
>> Extends the driver to also support 2 data lanes.
>> Frame rates are obviously more restricted on 2 lanes, but some
>> hardware simply hasn't wired more up.
>>
>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>> Signed-off-by: Luigi311 <git@luigi311.com>
>> ---
>>  drivers/media/i2c/imx258.c | 214 ++++++++++++++++++++++++++++++++-----
>>  1 file changed, 190 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>> index 6ee7de079454..c65b9aad3b0a 100644
>> --- a/drivers/media/i2c/imx258.c
>> +++ b/drivers/media/i2c/imx258.c
>> @@ -86,12 +86,18 @@ struct imx258_reg_list {
>>  	const struct imx258_reg *regs;
>>  };
>>  
>> +enum {
>> +	IMX258_2_LANE_MODE,
>> +	IMX258_4_LANE_MODE,
>> +	IMX258_LANE_CONFIGS,
>> +};
>> +
>>  /* Link frequency config */
>>  struct imx258_link_freq_config {
>>  	u32 pixels_per_line;
>>  
>>  	/* PLL registers for this link frequency */
>> -	struct imx258_reg_list reg_list;
>> +	struct imx258_reg_list reg_list[IMX258_LANE_CONFIGS];
>>  };
>>  
>>  /* Mode : resolution and related config&values */
>> @@ -111,8 +117,34 @@ struct imx258_mode {
>>  	struct imx258_reg_list reg_list;
>>  };
>>  
>> -/* 4208x3120 needs 1267Mbps/lane, 4 lanes */
>> -static const struct imx258_reg mipi_1267mbps_19_2mhz[] = {
>> +/*
>> + * 4208x3120 @ 30 fps needs 1267Mbps/lane, 4 lanes.
>> + * To avoid further computation of clock settings, adopt the same per
>> + * lane data rate when using 2 lanes, thus allowing a maximum of 15fps.
>> + */
>> +static const struct imx258_reg mipi_1267mbps_19_2mhz_2l[] = {
>> +	{ 0x0136, 0x13 },
>> +	{ 0x0137, 0x33 },
>> +	{ 0x0301, 0x0A },
>> +	{ 0x0303, 0x02 },
>> +	{ 0x0305, 0x03 },
>> +	{ 0x0306, 0x00 },
>> +	{ 0x0307, 0xC6 },
>> +	{ 0x0309, 0x0A },
>> +	{ 0x030B, 0x01 },
>> +	{ 0x030D, 0x02 },
>> +	{ 0x030E, 0x00 },
>> +	{ 0x030F, 0xD8 },
>> +	{ 0x0310, 0x00 },
>> +
>> +	{ 0x0114, 0x01 },
>> +	{ 0x0820, 0x09 },
>> +	{ 0x0821, 0xa6 },
>> +	{ 0x0822, 0x66 },
>> +	{ 0x0823, 0x66 },
>> +};
>> +
>> +static const struct imx258_reg mipi_1267mbps_19_2mhz_4l[] = {
>>  	{ 0x0136, 0x13 },
>>  	{ 0x0137, 0x33 },
>>  	{ 0x0301, 0x05 },
>> @@ -126,16 +158,18 @@ static const struct imx258_reg mipi_1267mbps_19_2mhz[] = {
>>  	{ 0x030E, 0x00 },
>>  	{ 0x030F, 0xD8 },
>>  	{ 0x0310, 0x00 },
>> +
>> +	{ 0x0114, 0x03 },
>>  	{ 0x0820, 0x13 },
>>  	{ 0x0821, 0x4C },
>>  	{ 0x0822, 0xCC },
>>  	{ 0x0823, 0xCC },
>>  };
>>  
>> -static const struct imx258_reg mipi_1272mbps_24mhz[] = {
>> +static const struct imx258_reg mipi_1272mbps_24mhz_2l[] = {
>>  	{ 0x0136, 0x18 },
>>  	{ 0x0137, 0x00 },
>> -	{ 0x0301, 0x05 },
>> +	{ 0x0301, 0x0a },
>>  	{ 0x0303, 0x02 },
>>  	{ 0x0305, 0x04 },
>>  	{ 0x0306, 0x00 },
>> @@ -146,13 +180,59 @@ static const struct imx258_reg mipi_1272mbps_24mhz[] = {
>>  	{ 0x030E, 0x00 },
>>  	{ 0x030F, 0xD8 },
>>  	{ 0x0310, 0x00 },
>> +
>> +	{ 0x0114, 0x01 },
>>  	{ 0x0820, 0x13 },
>>  	{ 0x0821, 0x4C },
>>  	{ 0x0822, 0xCC },
>>  	{ 0x0823, 0xCC },
>>  };
>>  
>> -static const struct imx258_reg mipi_640mbps_19_2mhz[] = {
>> +static const struct imx258_reg mipi_1272mbps_24mhz_4l[] = {
>> +	{ 0x0136, 0x18 },
>> +	{ 0x0137, 0x00 },
>> +	{ 0x0301, 0x05 },
>> +	{ 0x0303, 0x02 },
>> +	{ 0x0305, 0x04 },
>> +	{ 0x0306, 0x00 },
>> +	{ 0x0307, 0xD4 },
>> +	{ 0x0309, 0x0A },
>> +	{ 0x030B, 0x01 },
>> +	{ 0x030D, 0x02 },
>> +	{ 0x030E, 0x00 },
>> +	{ 0x030F, 0xD8 },
>> +	{ 0x0310, 0x00 },
>> +
>> +	{ 0x0114, 0x03 },
>> +	{ 0x0820, 0x13 },
>> +	{ 0x0821, 0xE0 },
>> +	{ 0x0822, 0x00 },
>> +	{ 0x0823, 0x00 },
>> +};
>> +
>> +static const struct imx258_reg mipi_640mbps_19_2mhz_2l[] = {
>> +	{ 0x0136, 0x13 },
>> +	{ 0x0137, 0x33 },
>> +	{ 0x0301, 0x05 },
>> +	{ 0x0303, 0x02 },
>> +	{ 0x0305, 0x03 },
>> +	{ 0x0306, 0x00 },
>> +	{ 0x0307, 0x64 },
>> +	{ 0x0309, 0x0A },
>> +	{ 0x030B, 0x01 },
>> +	{ 0x030D, 0x02 },
>> +	{ 0x030E, 0x00 },
>> +	{ 0x030F, 0xD8 },
>> +	{ 0x0310, 0x00 },
>> +
>> +	{ 0x0114, 0x01 },
>> +	{ 0x0820, 0x05 },
>> +	{ 0x0821, 0x00 },
>> +	{ 0x0822, 0x00 },
>> +	{ 0x0823, 0x00 },
>> +};
>> +
>> +static const struct imx258_reg mipi_640mbps_19_2mhz_4l[] = {
>>  	{ 0x0136, 0x13 },
>>  	{ 0x0137, 0x33 },
>>  	{ 0x0301, 0x05 },
>> @@ -166,13 +246,37 @@ static const struct imx258_reg mipi_640mbps_19_2mhz[] = {
>>  	{ 0x030E, 0x00 },
>>  	{ 0x030F, 0xD8 },
>>  	{ 0x0310, 0x00 },
>> +
>> +	{ 0x0114, 0x03 },
>> +	{ 0x0820, 0x0A },
>> +	{ 0x0821, 0x00 },
>> +	{ 0x0822, 0x00 },
>> +	{ 0x0823, 0x00 },
>> +};
>> +
>> +static const struct imx258_reg mipi_642mbps_24mhz_2l[] = {
>> +	{ 0x0136, 0x18 },
>> +	{ 0x0137, 0x00 },
>> +	{ 0x0301, 0x0A },
>> +	{ 0x0303, 0x02 },
>> +	{ 0x0305, 0x04 },
>> +	{ 0x0306, 0x00 },
>> +	{ 0x0307, 0x6B },
>> +	{ 0x0309, 0x0A },
>> +	{ 0x030B, 0x01 },
>> +	{ 0x030D, 0x02 },
>> +	{ 0x030E, 0x00 },
>> +	{ 0x030F, 0xD8 },
>> +	{ 0x0310, 0x00 },
>> +
>> +	{ 0x0114, 0x01 },
>>  	{ 0x0820, 0x0A },
>>  	{ 0x0821, 0x00 },
>>  	{ 0x0822, 0x00 },
>>  	{ 0x0823, 0x00 },
>>  };
>>  
>> -static const struct imx258_reg mipi_642mbps_24mhz[] = {
>> +static const struct imx258_reg mipi_642mbps_24mhz_4l[] = {
>>  	{ 0x0136, 0x18 },
>>  	{ 0x0137, 0x00 },
>>  	{ 0x0301, 0x05 },
>> @@ -186,6 +290,8 @@ static const struct imx258_reg mipi_642mbps_24mhz[] = {
>>  	{ 0x030E, 0x00 },
>>  	{ 0x030F, 0xD8 },
>>  	{ 0x0310, 0x00 },
>> +
>> +	{ 0x0114, 0x03 },
>>  	{ 0x0820, 0x0A },
>>  	{ 0x0821, 0x00 },
>>  	{ 0x0822, 0x00 },
>> @@ -240,7 +346,6 @@ static const struct imx258_reg mode_common_regs[] = {
>>  	{ 0x5F05, 0xED },
>>  	{ 0x0112, 0x0A },
>>  	{ 0x0113, 0x0A },
>> -	{ 0x0114, 0x03 },
>>  	{ 0x0342, 0x14 },
>>  	{ 0x0343, 0xE8 },
>>  	{ 0x0344, 0x00 },
>> @@ -359,11 +464,13 @@ enum {
>>  
>>  /*
>>   * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample
>> - * data rate => double data rate; number of lanes => 4; bits per pixel => 10
>> + * data rate => double data rate;
>> + * number of lanes => (configurable 2 or 4);
>> + * bits per pixel => 10
>>   */
>> -static u64 link_freq_to_pixel_rate(u64 f)
>> +static u64 link_freq_to_pixel_rate(u64 f, unsigned int nlanes)
>>  {
>> -	f *= 2 * 4;
>> +	f *= 2 * nlanes;
>>  	do_div(f, 10);
>>  
>>  	return f;
>> @@ -393,15 +500,27 @@ static const struct imx258_link_freq_config link_freq_configs_19_2[] = {
>>  	[IMX258_LINK_FREQ_1267MBPS] = {
>>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>>  		.reg_list = {
>> -			.num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz),
>> -			.regs = mipi_1267mbps_19_2mhz,
>> +			[IMX258_2_LANE_MODE] = {
>> +				.num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz_2l),
>> +				.regs = mipi_1267mbps_19_2mhz_2l,
>> +			},
>> +			[IMX258_4_LANE_MODE] = {
>> +				.num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz_4l),
>> +				.regs = mipi_1267mbps_19_2mhz_4l,
>> +			},
>>  		}
>>  	},
>>  	[IMX258_LINK_FREQ_640MBPS] = {
>>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>>  		.reg_list = {
>> -			.num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz),
>> -			.regs = mipi_640mbps_19_2mhz,
>> +			[IMX258_2_LANE_MODE] = {
>> +				.num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz_2l),
>> +				.regs = mipi_640mbps_19_2mhz_2l,
>> +			},
>> +			[IMX258_4_LANE_MODE] = {
>> +				.num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz_4l),
>> +				.regs = mipi_640mbps_19_2mhz_4l,
>> +			},
>>  		}
>>  	},
>>  };
>> @@ -410,15 +529,27 @@ static const struct imx258_link_freq_config link_freq_configs_24[] = {
>>  	[IMX258_LINK_FREQ_1267MBPS] = {
>>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>>  		.reg_list = {
>> -			.num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz),
>> -			.regs = mipi_1272mbps_24mhz,
>> +			[IMX258_2_LANE_MODE] = {
>> +				.num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz_2l),
>> +				.regs = mipi_1272mbps_24mhz_2l,
>> +			},
>> +			[IMX258_4_LANE_MODE] = {
>> +				.num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz_4l),
>> +				.regs = mipi_1272mbps_24mhz_4l,
>> +			},
>>  		}
>>  	},
>>  	[IMX258_LINK_FREQ_640MBPS] = {
>>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>>  		.reg_list = {
>> -			.num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz),
>> -			.regs = mipi_642mbps_24mhz,
>> +			[IMX258_2_LANE_MODE] = {
>> +				.num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz_2l),
>> +				.regs = mipi_642mbps_24mhz_2l,
>> +			},
>> +			[IMX258_4_LANE_MODE] = {
>> +				.num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz_4l),
>> +				.regs = mipi_642mbps_24mhz_4l,
>> +			},
>>  		}
>>  	},
>>  };
>> @@ -477,6 +608,7 @@ struct imx258 {
>>  
>>  	const struct imx258_link_freq_config *link_freq_configs;
>>  	const s64 *link_freq_menu_items;
>> +	unsigned int nlanes;
>>  
>>  	/*
>>  	 * Mutex for serialized access:
>> @@ -782,7 +914,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
>>  		__v4l2_ctrl_s_ctrl(imx258->link_freq, mode->link_freq_index);
>>  
>>  		link_freq = imx258->link_freq_menu_items[mode->link_freq_index];
>> -		pixel_rate = link_freq_to_pixel_rate(link_freq);
>> +		pixel_rate = link_freq_to_pixel_rate(link_freq, imx258->nlanes);
>>  		__v4l2_ctrl_s_ctrl_int64(imx258->pixel_rate, pixel_rate);
>>  		/* Update limits and set FPS to default */
>>  		vblank_def = imx258->cur_mode->vts_def -
>> @@ -811,11 +943,13 @@ static int imx258_start_streaming(struct imx258 *imx258)
>>  {
>>  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
>>  	const struct imx258_reg_list *reg_list;
>> +	const struct imx258_link_freq_config *link_freq_cfg;
>>  	int ret, link_freq_index;
>>  
>>  	/* Setup PLL */
>>  	link_freq_index = imx258->cur_mode->link_freq_index;
>> -	reg_list = &imx258->link_freq_configs[link_freq_index].reg_list;
>> +	link_freq_cfg = &imx258->link_freq_configs[link_freq_index];
>> +	reg_list = &link_freq_cfg->reg_list[imx258->nlanes == 2 ? 0 : 1];
>>  	ret = imx258_write_regs(imx258, reg_list->regs, reg_list->num_of_regs);
>>  	if (ret) {
>>  		dev_err(&client->dev, "%s failed to set plls\n", __func__);
>> @@ -1033,9 +1167,11 @@ static int imx258_init_controls(struct imx258 *imx258)
>>  		vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>  
>>  	pixel_rate_max =
>> -		link_freq_to_pixel_rate(imx258->link_freq_menu_items[0]);
>> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[0],
>> +					imx258->nlanes);
>>  	pixel_rate_min =
>> -		link_freq_to_pixel_rate(imx258->link_freq_menu_items[1]);
>> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[1],
>> +					imx258->nlanes);
>>  	/* By default, PIXEL_RATE is read only */
>>  	imx258->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
>>  				V4L2_CID_PIXEL_RATE,
>> @@ -1132,6 +1268,10 @@ static int imx258_get_regulators(struct imx258 *imx258,
>>  static int imx258_probe(struct i2c_client *client)
>>  {
>>  	struct imx258 *imx258;
>> +	struct fwnode_handle *endpoint;
>> +	struct v4l2_fwnode_endpoint ep = {
>> +		.bus_type = V4L2_MBUS_CSI2_DPHY
>> +	};
>>  	int ret;
>>  	u32 val = 0;
>>  
>> @@ -1172,13 +1312,35 @@ static int imx258_probe(struct i2c_client *client)
>>  		return -EINVAL;
>>  	}
>>  
>> +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
>> +	if (!endpoint) {
>> +		dev_err(&client->dev, "Endpoint node not found\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
> 
> Here you're obtaining the list of supported link frequencies from the
> firmware but it is not validated (nor it was validated by the driver
> previously). I'd regard that a driver bug but fixing it at this point could
> introduce adverse effects elsewhere.
> 
> I think what I'd do here is that I'd ignore the issue if there are no
> frequencies defined for the endpoint but if there are, then enable only
> those that are listed in the endpoint.
> 
> Could you add a patch to do this, please? v4l2_link_freq_to_bitmap() has
> been recently added to facilitate this.
> 

I can give this a try, it would be similar to this patch that you submitted
earlier for the imx319 here
https://github.com/torvalds/linux/commit/726a09c1b6890887b7388745a26c8e93867780ca
right? 

>> +	fwnode_handle_put(endpoint);
>> +	if (ret) {
>> +		dev_err(&client->dev, "Parsing endpoint node failed\n");
>> +		return ret;
>> +	}
>> +
>> +	/* Get number of data lanes */
>> +	imx258->nlanes = ep.bus.mipi_csi2.num_data_lanes;
>> +	if (imx258->nlanes != 2 && imx258->nlanes != 4) {
>> +		dev_err(&client->dev, "Invalid data lanes: %u\n",
>> +			imx258->nlanes);
>> +		ret = -EINVAL;
>> +		goto error_endpoint_free;
>> +	}
>> +
>>  	/* Initialize subdev */
>>  	v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
>>  
>>  	/* Will be powered off via pm_runtime_idle */
>>  	ret = imx258_power_on(&client->dev);
>>  	if (ret)
>> -		return ret;
>> +		goto error_endpoint_free;
>>  
>>  	/* Check module identity */
>>  	ret = imx258_identify_module(imx258);
>> @@ -1211,6 +1373,7 @@ static int imx258_probe(struct i2c_client *client)
>>  	pm_runtime_set_active(&client->dev);
>>  	pm_runtime_enable(&client->dev);
>>  	pm_runtime_idle(&client->dev);
>> +	v4l2_fwnode_endpoint_free(&ep);
>>  
>>  	return 0;
>>  
>> @@ -1223,6 +1386,9 @@ static int imx258_probe(struct i2c_client *client)
>>  error_identify:
>>  	imx258_power_off(&client->dev);
>>  
>> +error_endpoint_free:
>> +	v4l2_fwnode_endpoint_free(&ep);
>> +
>>  	return ret;
>>  }
>>  
>

On 3/28/24 17:42, Luigi311 wrote:
> On 3/28/24 02:19, Sakari Ailus wrote:
>> Hi Luigi311, Dave,
>>
>> On Wed, Mar 27, 2024 at 05:16:55PM -0600, git@luigi311.com wrote:
>>> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>>
>>> Extends the driver to also support 2 data lanes.
>>> Frame rates are obviously more restricted on 2 lanes, but some
>>> hardware simply hasn't wired more up.
>>>
>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> Signed-off-by: Luigi311 <git@luigi311.com>
>>> ---
>>>  drivers/media/i2c/imx258.c | 214 ++++++++++++++++++++++++++++++++-----
>>>  1 file changed, 190 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>>> index 6ee7de079454..c65b9aad3b0a 100644
>>> --- a/drivers/media/i2c/imx258.c
>>> +++ b/drivers/media/i2c/imx258.c
>>> @@ -86,12 +86,18 @@ struct imx258_reg_list {
>>>  	const struct imx258_reg *regs;
>>>  };
>>>  
>>> +enum {
>>> +	IMX258_2_LANE_MODE,
>>> +	IMX258_4_LANE_MODE,
>>> +	IMX258_LANE_CONFIGS,
>>> +};
>>> +
>>>  /* Link frequency config */
>>>  struct imx258_link_freq_config {
>>>  	u32 pixels_per_line;
>>>  
>>>  	/* PLL registers for this link frequency */
>>> -	struct imx258_reg_list reg_list;
>>> +	struct imx258_reg_list reg_list[IMX258_LANE_CONFIGS];
>>>  };
>>>  
>>>  /* Mode : resolution and related config&values */
>>> @@ -111,8 +117,34 @@ struct imx258_mode {
>>>  	struct imx258_reg_list reg_list;
>>>  };
>>>  
>>> -/* 4208x3120 needs 1267Mbps/lane, 4 lanes */
>>> -static const struct imx258_reg mipi_1267mbps_19_2mhz[] = {
>>> +/*
>>> + * 4208x3120 @ 30 fps needs 1267Mbps/lane, 4 lanes.
>>> + * To avoid further computation of clock settings, adopt the same per
>>> + * lane data rate when using 2 lanes, thus allowing a maximum of 15fps.
>>> + */
>>> +static const struct imx258_reg mipi_1267mbps_19_2mhz_2l[] = {
>>> +	{ 0x0136, 0x13 },
>>> +	{ 0x0137, 0x33 },
>>> +	{ 0x0301, 0x0A },
>>> +	{ 0x0303, 0x02 },
>>> +	{ 0x0305, 0x03 },
>>> +	{ 0x0306, 0x00 },
>>> +	{ 0x0307, 0xC6 },
>>> +	{ 0x0309, 0x0A },
>>> +	{ 0x030B, 0x01 },
>>> +	{ 0x030D, 0x02 },
>>> +	{ 0x030E, 0x00 },
>>> +	{ 0x030F, 0xD8 },
>>> +	{ 0x0310, 0x00 },
>>> +
>>> +	{ 0x0114, 0x01 },
>>> +	{ 0x0820, 0x09 },
>>> +	{ 0x0821, 0xa6 },
>>> +	{ 0x0822, 0x66 },
>>> +	{ 0x0823, 0x66 },
>>> +};
>>> +
>>> +static const struct imx258_reg mipi_1267mbps_19_2mhz_4l[] = {
>>>  	{ 0x0136, 0x13 },
>>>  	{ 0x0137, 0x33 },
>>>  	{ 0x0301, 0x05 },
>>> @@ -126,16 +158,18 @@ static const struct imx258_reg mipi_1267mbps_19_2mhz[] = {
>>>  	{ 0x030E, 0x00 },
>>>  	{ 0x030F, 0xD8 },
>>>  	{ 0x0310, 0x00 },
>>> +
>>> +	{ 0x0114, 0x03 },
>>>  	{ 0x0820, 0x13 },
>>>  	{ 0x0821, 0x4C },
>>>  	{ 0x0822, 0xCC },
>>>  	{ 0x0823, 0xCC },
>>>  };
>>>  
>>> -static const struct imx258_reg mipi_1272mbps_24mhz[] = {
>>> +static const struct imx258_reg mipi_1272mbps_24mhz_2l[] = {
>>>  	{ 0x0136, 0x18 },
>>>  	{ 0x0137, 0x00 },
>>> -	{ 0x0301, 0x05 },
>>> +	{ 0x0301, 0x0a },
>>>  	{ 0x0303, 0x02 },
>>>  	{ 0x0305, 0x04 },
>>>  	{ 0x0306, 0x00 },
>>> @@ -146,13 +180,59 @@ static const struct imx258_reg mipi_1272mbps_24mhz[] = {
>>>  	{ 0x030E, 0x00 },
>>>  	{ 0x030F, 0xD8 },
>>>  	{ 0x0310, 0x00 },
>>> +
>>> +	{ 0x0114, 0x01 },
>>>  	{ 0x0820, 0x13 },
>>>  	{ 0x0821, 0x4C },
>>>  	{ 0x0822, 0xCC },
>>>  	{ 0x0823, 0xCC },
>>>  };
>>>  
>>> -static const struct imx258_reg mipi_640mbps_19_2mhz[] = {
>>> +static const struct imx258_reg mipi_1272mbps_24mhz_4l[] = {
>>> +	{ 0x0136, 0x18 },
>>> +	{ 0x0137, 0x00 },
>>> +	{ 0x0301, 0x05 },
>>> +	{ 0x0303, 0x02 },
>>> +	{ 0x0305, 0x04 },
>>> +	{ 0x0306, 0x00 },
>>> +	{ 0x0307, 0xD4 },
>>> +	{ 0x0309, 0x0A },
>>> +	{ 0x030B, 0x01 },
>>> +	{ 0x030D, 0x02 },
>>> +	{ 0x030E, 0x00 },
>>> +	{ 0x030F, 0xD8 },
>>> +	{ 0x0310, 0x00 },
>>> +
>>> +	{ 0x0114, 0x03 },
>>> +	{ 0x0820, 0x13 },
>>> +	{ 0x0821, 0xE0 },
>>> +	{ 0x0822, 0x00 },
>>> +	{ 0x0823, 0x00 },
>>> +};
>>> +
>>> +static const struct imx258_reg mipi_640mbps_19_2mhz_2l[] = {
>>> +	{ 0x0136, 0x13 },
>>> +	{ 0x0137, 0x33 },
>>> +	{ 0x0301, 0x05 },
>>> +	{ 0x0303, 0x02 },
>>> +	{ 0x0305, 0x03 },
>>> +	{ 0x0306, 0x00 },
>>> +	{ 0x0307, 0x64 },
>>> +	{ 0x0309, 0x0A },
>>> +	{ 0x030B, 0x01 },
>>> +	{ 0x030D, 0x02 },
>>> +	{ 0x030E, 0x00 },
>>> +	{ 0x030F, 0xD8 },
>>> +	{ 0x0310, 0x00 },
>>> +
>>> +	{ 0x0114, 0x01 },
>>> +	{ 0x0820, 0x05 },
>>> +	{ 0x0821, 0x00 },
>>> +	{ 0x0822, 0x00 },
>>> +	{ 0x0823, 0x00 },
>>> +};
>>> +
>>> +static const struct imx258_reg mipi_640mbps_19_2mhz_4l[] = {
>>>  	{ 0x0136, 0x13 },
>>>  	{ 0x0137, 0x33 },
>>>  	{ 0x0301, 0x05 },
>>> @@ -166,13 +246,37 @@ static const struct imx258_reg mipi_640mbps_19_2mhz[] = {
>>>  	{ 0x030E, 0x00 },
>>>  	{ 0x030F, 0xD8 },
>>>  	{ 0x0310, 0x00 },
>>> +
>>> +	{ 0x0114, 0x03 },
>>> +	{ 0x0820, 0x0A },
>>> +	{ 0x0821, 0x00 },
>>> +	{ 0x0822, 0x00 },
>>> +	{ 0x0823, 0x00 },
>>> +};
>>> +
>>> +static const struct imx258_reg mipi_642mbps_24mhz_2l[] = {
>>> +	{ 0x0136, 0x18 },
>>> +	{ 0x0137, 0x00 },
>>> +	{ 0x0301, 0x0A },
>>> +	{ 0x0303, 0x02 },
>>> +	{ 0x0305, 0x04 },
>>> +	{ 0x0306, 0x00 },
>>> +	{ 0x0307, 0x6B },
>>> +	{ 0x0309, 0x0A },
>>> +	{ 0x030B, 0x01 },
>>> +	{ 0x030D, 0x02 },
>>> +	{ 0x030E, 0x00 },
>>> +	{ 0x030F, 0xD8 },
>>> +	{ 0x0310, 0x00 },
>>> +
>>> +	{ 0x0114, 0x01 },
>>>  	{ 0x0820, 0x0A },
>>>  	{ 0x0821, 0x00 },
>>>  	{ 0x0822, 0x00 },
>>>  	{ 0x0823, 0x00 },
>>>  };
>>>  
>>> -static const struct imx258_reg mipi_642mbps_24mhz[] = {
>>> +static const struct imx258_reg mipi_642mbps_24mhz_4l[] = {
>>>  	{ 0x0136, 0x18 },
>>>  	{ 0x0137, 0x00 },
>>>  	{ 0x0301, 0x05 },
>>> @@ -186,6 +290,8 @@ static const struct imx258_reg mipi_642mbps_24mhz[] = {
>>>  	{ 0x030E, 0x00 },
>>>  	{ 0x030F, 0xD8 },
>>>  	{ 0x0310, 0x00 },
>>> +
>>> +	{ 0x0114, 0x03 },
>>>  	{ 0x0820, 0x0A },
>>>  	{ 0x0821, 0x00 },
>>>  	{ 0x0822, 0x00 },
>>> @@ -240,7 +346,6 @@ static const struct imx258_reg mode_common_regs[] = {
>>>  	{ 0x5F05, 0xED },
>>>  	{ 0x0112, 0x0A },
>>>  	{ 0x0113, 0x0A },
>>> -	{ 0x0114, 0x03 },
>>>  	{ 0x0342, 0x14 },
>>>  	{ 0x0343, 0xE8 },
>>>  	{ 0x0344, 0x00 },
>>> @@ -359,11 +464,13 @@ enum {
>>>  
>>>  /*
>>>   * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample
>>> - * data rate => double data rate; number of lanes => 4; bits per pixel => 10
>>> + * data rate => double data rate;
>>> + * number of lanes => (configurable 2 or 4);
>>> + * bits per pixel => 10
>>>   */
>>> -static u64 link_freq_to_pixel_rate(u64 f)
>>> +static u64 link_freq_to_pixel_rate(u64 f, unsigned int nlanes)
>>>  {
>>> -	f *= 2 * 4;
>>> +	f *= 2 * nlanes;
>>>  	do_div(f, 10);
>>>  
>>>  	return f;
>>> @@ -393,15 +500,27 @@ static const struct imx258_link_freq_config link_freq_configs_19_2[] = {
>>>  	[IMX258_LINK_FREQ_1267MBPS] = {
>>>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>>>  		.reg_list = {
>>> -			.num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz),
>>> -			.regs = mipi_1267mbps_19_2mhz,
>>> +			[IMX258_2_LANE_MODE] = {
>>> +				.num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz_2l),
>>> +				.regs = mipi_1267mbps_19_2mhz_2l,
>>> +			},
>>> +			[IMX258_4_LANE_MODE] = {
>>> +				.num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz_4l),
>>> +				.regs = mipi_1267mbps_19_2mhz_4l,
>>> +			},
>>>  		}
>>>  	},
>>>  	[IMX258_LINK_FREQ_640MBPS] = {
>>>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>>>  		.reg_list = {
>>> -			.num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz),
>>> -			.regs = mipi_640mbps_19_2mhz,
>>> +			[IMX258_2_LANE_MODE] = {
>>> +				.num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz_2l),
>>> +				.regs = mipi_640mbps_19_2mhz_2l,
>>> +			},
>>> +			[IMX258_4_LANE_MODE] = {
>>> +				.num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz_4l),
>>> +				.regs = mipi_640mbps_19_2mhz_4l,
>>> +			},
>>>  		}
>>>  	},
>>>  };
>>> @@ -410,15 +529,27 @@ static const struct imx258_link_freq_config link_freq_configs_24[] = {
>>>  	[IMX258_LINK_FREQ_1267MBPS] = {
>>>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>>>  		.reg_list = {
>>> -			.num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz),
>>> -			.regs = mipi_1272mbps_24mhz,
>>> +			[IMX258_2_LANE_MODE] = {
>>> +				.num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz_2l),
>>> +				.regs = mipi_1272mbps_24mhz_2l,
>>> +			},
>>> +			[IMX258_4_LANE_MODE] = {
>>> +				.num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz_4l),
>>> +				.regs = mipi_1272mbps_24mhz_4l,
>>> +			},
>>>  		}
>>>  	},
>>>  	[IMX258_LINK_FREQ_640MBPS] = {
>>>  		.pixels_per_line = IMX258_PPL_DEFAULT,
>>>  		.reg_list = {
>>> -			.num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz),
>>> -			.regs = mipi_642mbps_24mhz,
>>> +			[IMX258_2_LANE_MODE] = {
>>> +				.num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz_2l),
>>> +				.regs = mipi_642mbps_24mhz_2l,
>>> +			},
>>> +			[IMX258_4_LANE_MODE] = {
>>> +				.num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz_4l),
>>> +				.regs = mipi_642mbps_24mhz_4l,
>>> +			},
>>>  		}
>>>  	},
>>>  };
>>> @@ -477,6 +608,7 @@ struct imx258 {
>>>  
>>>  	const struct imx258_link_freq_config *link_freq_configs;
>>>  	const s64 *link_freq_menu_items;
>>> +	unsigned int nlanes;
>>>  
>>>  	/*
>>>  	 * Mutex for serialized access:
>>> @@ -782,7 +914,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
>>>  		__v4l2_ctrl_s_ctrl(imx258->link_freq, mode->link_freq_index);
>>>  
>>>  		link_freq = imx258->link_freq_menu_items[mode->link_freq_index];
>>> -		pixel_rate = link_freq_to_pixel_rate(link_freq);
>>> +		pixel_rate = link_freq_to_pixel_rate(link_freq, imx258->nlanes);
>>>  		__v4l2_ctrl_s_ctrl_int64(imx258->pixel_rate, pixel_rate);
>>>  		/* Update limits and set FPS to default */
>>>  		vblank_def = imx258->cur_mode->vts_def -
>>> @@ -811,11 +943,13 @@ static int imx258_start_streaming(struct imx258 *imx258)
>>>  {
>>>  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
>>>  	const struct imx258_reg_list *reg_list;
>>> +	const struct imx258_link_freq_config *link_freq_cfg;
>>>  	int ret, link_freq_index;
>>>  
>>>  	/* Setup PLL */
>>>  	link_freq_index = imx258->cur_mode->link_freq_index;
>>> -	reg_list = &imx258->link_freq_configs[link_freq_index].reg_list;
>>> +	link_freq_cfg = &imx258->link_freq_configs[link_freq_index];
>>> +	reg_list = &link_freq_cfg->reg_list[imx258->nlanes == 2 ? 0 : 1];
>>>  	ret = imx258_write_regs(imx258, reg_list->regs, reg_list->num_of_regs);
>>>  	if (ret) {
>>>  		dev_err(&client->dev, "%s failed to set plls\n", __func__);
>>> @@ -1033,9 +1167,11 @@ static int imx258_init_controls(struct imx258 *imx258)
>>>  		vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>>  
>>>  	pixel_rate_max =
>>> -		link_freq_to_pixel_rate(imx258->link_freq_menu_items[0]);
>>> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[0],
>>> +					imx258->nlanes);
>>>  	pixel_rate_min =
>>> -		link_freq_to_pixel_rate(imx258->link_freq_menu_items[1]);
>>> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[1],
>>> +					imx258->nlanes);
>>>  	/* By default, PIXEL_RATE is read only */
>>>  	imx258->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
>>>  				V4L2_CID_PIXEL_RATE,
>>> @@ -1132,6 +1268,10 @@ static int imx258_get_regulators(struct imx258 *imx258,
>>>  static int imx258_probe(struct i2c_client *client)
>>>  {
>>>  	struct imx258 *imx258;
>>> +	struct fwnode_handle *endpoint;
>>> +	struct v4l2_fwnode_endpoint ep = {
>>> +		.bus_type = V4L2_MBUS_CSI2_DPHY
>>> +	};
>>>  	int ret;
>>>  	u32 val = 0;
>>>  
>>> @@ -1172,13 +1312,35 @@ static int imx258_probe(struct i2c_client *client)
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> +	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
>>> +	if (!endpoint) {
>>> +		dev_err(&client->dev, "Endpoint node not found\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
>>
>> Here you're obtaining the list of supported link frequencies from the
>> firmware but it is not validated (nor it was validated by the driver
>> previously). I'd regard that a driver bug but fixing it at this point could
>> introduce adverse effects elsewhere.
>>
>> I think what I'd do here is that I'd ignore the issue if there are no
>> frequencies defined for the endpoint but if there are, then enable only
>> those that are listed in the endpoint.
>>
>> Could you add a patch to do this, please? v4l2_link_freq_to_bitmap() has
>> been recently added to facilitate this.
>>
> 
> I can give this a try, it would be similar to this patch that you submitted
> earlier for the imx319 here
> https://github.com/torvalds/linux/commit/726a09c1b6890887b7388745a26c8e93867780ca
> right? 
> 

I believe I got this implemented, added in that v4l2_link_freq_to_bitmap()
and it failed to probe the imx258 device due to missing frequencies so I
checked the device dts and the imx258 had no link-frequencies specified so
added in 321000000 and 636000000 to match the 24mhz and it worked, I then
swapped it over to bogus numbers 311000000 and 626000000 and it complained
about no matching link frequencies found so it failed to probe. Looks
like its working with that new function now. Will include in next revision

>>> +	fwnode_handle_put(endpoint);
>>> +	if (ret) {
>>> +		dev_err(&client->dev, "Parsing endpoint node failed\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	/* Get number of data lanes */
>>> +	imx258->nlanes = ep.bus.mipi_csi2.num_data_lanes;
>>> +	if (imx258->nlanes != 2 && imx258->nlanes != 4) {
>>> +		dev_err(&client->dev, "Invalid data lanes: %u\n",
>>> +			imx258->nlanes);
>>> +		ret = -EINVAL;
>>> +		goto error_endpoint_free;
>>> +	}
>>> +
>>>  	/* Initialize subdev */
>>>  	v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
>>>  
>>>  	/* Will be powered off via pm_runtime_idle */
>>>  	ret = imx258_power_on(&client->dev);
>>>  	if (ret)
>>> -		return ret;
>>> +		goto error_endpoint_free;
>>>  
>>>  	/* Check module identity */
>>>  	ret = imx258_identify_module(imx258);
>>> @@ -1211,6 +1373,7 @@ static int imx258_probe(struct i2c_client *client)
>>>  	pm_runtime_set_active(&client->dev);
>>>  	pm_runtime_enable(&client->dev);
>>>  	pm_runtime_idle(&client->dev);
>>> +	v4l2_fwnode_endpoint_free(&ep);
>>>  
>>>  	return 0;
>>>  
>>> @@ -1223,6 +1386,9 @@ static int imx258_probe(struct i2c_client *client)
>>>  error_identify:
>>>  	imx258_power_off(&client->dev);
>>>  
>>> +error_endpoint_free:
>>> +	v4l2_fwnode_endpoint_free(&ep);
>>> +
>>>  	return ret;
>>>  }
>>>  
>>
>

On Wednesday, March 27, 2024 11:16:46 PM UTC git@luigi311.com wrote:
> From: Luigi311 <git@luigi311.com>

The Linux kernel does not allow anonymous (or pseudonymous) contributions. You 
should use your real first and last name.

See section 1.5 in "A guide to the Kernel Development Process":
https://www.kernel.org/doc/html/v6.8/process/1.Intro.html#licensing

On 3/29/24 21:51, Dang Huynh wrote:
> On Wednesday, March 27, 2024 11:16:46 PM UTC git@luigi311.com wrote:
>> From: Luigi311 <git@luigi311.com>
> 
> The Linux kernel does not allow anonymous (or pseudonymous) contributions. You 
> should use your real first and last name.
> 
> See section 1.5 in "A guide to the Kernel Development Process":
> https://www.kernel.org/doc/html/v6.8/process/1.Intro.html#licensing
> 
> 

Ok I've changed my sign off to my real first and last name for the
next revision

Hi Luigi311,

On Thu, Mar 28, 2024 at 11:55:24AM -0600, Luigi311 wrote:
> On 3/28/24 02:09, Sakari Ailus wrote:
> > Hi Luigi311,
> > 
> > Thank you for the patchset.
> > 
> > On Wed, Mar 27, 2024 at 05:16:54PM -0600, git@luigi311.com wrote:
> >> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >>
> >> There's no reason why only a clock of 19.2MHz is supported.
> >> Indeed this isn't even a frequency listed in the datasheet.
> >>
> >> Add support for 24MHz as well.
> >> The PLL settings result in slightly different link frequencies,
> >> so parameterise those.
> >>
> >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >> Signed-off-by: Luigi311 <git@luigi311.com>
> > 
> > Is Luigi311 your real name? As per
> > Documentation/process/submitting-patches.rst, anonymous (or pseudonym I'd
> > say as well) contributions are not an option.
> 
> Luigi311 is not my real name but it would be a lot easier to find me if
> it was. My real name is Luis Garcia which is a super common name so its
> actually way easier to find me and all my work using my online name of
> Luigi311. I can go ahead and swap over to Luis Garcia if required but a
> name like that would provide no value in contacting/finding me since I'm
> not famous like all the other Luis Garcia's that appear on google.

Thanks. E-mail addresses are still unique, presumably, so that helps.

> 
> > 
> >> ---
> >>  drivers/media/i2c/imx258.c | 133 +++++++++++++++++++++++++++++--------
> >>  1 file changed, 107 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> >> index 351add1bc5d5..6ee7de079454 100644
> >> --- a/drivers/media/i2c/imx258.c
> >> +++ b/drivers/media/i2c/imx258.c
> >> @@ -76,9 +76,6 @@
> >>  #define REG_CONFIG_MIRROR_FLIP		0x03
> >>  #define REG_CONFIG_FLIP_TEST_PATTERN	0x02
> >>  
> >> -/* Input clock frequency in Hz */
> >> -#define IMX258_INPUT_CLOCK_FREQ		19200000
> >> -
> >>  struct imx258_reg {
> >>  	u16 address;
> >>  	u8 val;
> >> @@ -115,7 +112,9 @@ struct imx258_mode {
> >>  };
> >>  
> >>  /* 4208x3120 needs 1267Mbps/lane, 4 lanes */
> >> -static const struct imx258_reg mipi_data_rate_1267mbps[] = {
> >> +static const struct imx258_reg mipi_1267mbps_19_2mhz[] = {
> >> +	{ 0x0136, 0x13 },
> >> +	{ 0x0137, 0x33 },
> >>  	{ 0x0301, 0x05 },
> >>  	{ 0x0303, 0x02 },
> >>  	{ 0x0305, 0x03 },
> >> @@ -133,7 +132,29 @@ static const struct imx258_reg mipi_data_rate_1267mbps[] = {
> >>  	{ 0x0823, 0xCC },
> >>  };
> >>  
> >> -static const struct imx258_reg mipi_data_rate_640mbps[] = {
> >> +static const struct imx258_reg mipi_1272mbps_24mhz[] = {
> >> +	{ 0x0136, 0x18 },
> >> +	{ 0x0137, 0x00 },
> >> +	{ 0x0301, 0x05 },
> >> +	{ 0x0303, 0x02 },
> >> +	{ 0x0305, 0x04 },
> >> +	{ 0x0306, 0x00 },
> >> +	{ 0x0307, 0xD4 },
> >> +	{ 0x0309, 0x0A },
> >> +	{ 0x030B, 0x01 },
> >> +	{ 0x030D, 0x02 },
> >> +	{ 0x030E, 0x00 },
> >> +	{ 0x030F, 0xD8 },
> >> +	{ 0x0310, 0x00 },
> >> +	{ 0x0820, 0x13 },
> >> +	{ 0x0821, 0x4C },
> >> +	{ 0x0822, 0xCC },
> >> +	{ 0x0823, 0xCC },
> >> +};
> >> +
> >> +static const struct imx258_reg mipi_640mbps_19_2mhz[] = {
> >> +	{ 0x0136, 0x13 },
> >> +	{ 0x0137, 0x33 },
> >>  	{ 0x0301, 0x05 },
> >>  	{ 0x0303, 0x02 },
> >>  	{ 0x0305, 0x03 },
> >> @@ -151,9 +172,27 @@ static const struct imx258_reg mipi_data_rate_640mbps[] = {
> >>  	{ 0x0823, 0x00 },
> >>  };
> >>  
> >> +static const struct imx258_reg mipi_642mbps_24mhz[] = {
> >> +	{ 0x0136, 0x18 },
> >> +	{ 0x0137, 0x00 },
> >> +	{ 0x0301, 0x05 },
> >> +	{ 0x0303, 0x02 },
> >> +	{ 0x0305, 0x04 },
> >> +	{ 0x0306, 0x00 },
> >> +	{ 0x0307, 0x6B },
> >> +	{ 0x0309, 0x0A },
> >> +	{ 0x030B, 0x01 },
> >> +	{ 0x030D, 0x02 },
> >> +	{ 0x030E, 0x00 },
> >> +	{ 0x030F, 0xD8 },
> >> +	{ 0x0310, 0x00 },
> >> +	{ 0x0820, 0x0A },
> >> +	{ 0x0821, 0x00 },
> >> +	{ 0x0822, 0x00 },
> >> +	{ 0x0823, 0x00 },
> >> +};
> >> +
> >>  static const struct imx258_reg mode_common_regs[] = {
> >> -	{ 0x0136, 0x13 },
> >> -	{ 0x0137, 0x33 },
> >>  	{ 0x3051, 0x00 },
> >>  	{ 0x3052, 0x00 },
> >>  	{ 0x4E21, 0x14 },
> >> @@ -313,10 +352,6 @@ static const char * const imx258_supply_name[] = {
> >>  
> >>  #define IMX258_NUM_SUPPLIES ARRAY_SIZE(imx258_supply_name)
> >>  
> >> -/* Configurations for supported link frequencies */
> >> -#define IMX258_LINK_FREQ_634MHZ	633600000ULL
> >> -#define IMX258_LINK_FREQ_320MHZ	320000000ULL
> >> -
> >>  enum {
> >>  	IMX258_LINK_FREQ_1267MBPS,
> >>  	IMX258_LINK_FREQ_640MBPS,
> >> @@ -335,25 +370,55 @@ static u64 link_freq_to_pixel_rate(u64 f)
> >>  }
> >>  
> >>  /* Menu items for LINK_FREQ V4L2 control */
> >> -static const s64 link_freq_menu_items[] = {
> >> +/* Configurations for supported link frequencies */
> >> +#define IMX258_LINK_FREQ_634MHZ	633600000ULL
> >> +#define IMX258_LINK_FREQ_320MHZ	320000000ULL
> >> +
> >> +static const s64 link_freq_menu_items_19_2[] = {
> >>  	IMX258_LINK_FREQ_634MHZ,
> >>  	IMX258_LINK_FREQ_320MHZ,
> >>  };
> >>  
> >> +/* Configurations for supported link frequencies */
> >> +#define IMX258_LINK_FREQ_636MHZ	636000000ULL
> >> +#define IMX258_LINK_FREQ_321MHZ	321000000ULL
> > 
> > These values aren't used outside the array below and the macro names are
> > imprecise anyway. Could you put the numerical values to the array instead?
> 
> Ok I've removed the defines and just threw the values into the array instead.
> 
> > 
> >> +
> >> +static const s64 link_freq_menu_items_24[] = {
> >> +	IMX258_LINK_FREQ_636MHZ,
> >> +	IMX258_LINK_FREQ_321MHZ,
> >> +};
> >> +
> >>  /* Link frequency configs */
> >> -static const struct imx258_link_freq_config link_freq_configs[] = {
> >> +static const struct imx258_link_freq_config link_freq_configs_19_2[] = {
> >>  	[IMX258_LINK_FREQ_1267MBPS] = {
> >>  		.pixels_per_line = IMX258_PPL_DEFAULT,
> >>  		.reg_list = {
> >> -			.num_of_regs = ARRAY_SIZE(mipi_data_rate_1267mbps),
> >> -			.regs = mipi_data_rate_1267mbps,
> >> +			.num_of_regs = ARRAY_SIZE(mipi_1267mbps_19_2mhz),
> >> +			.regs = mipi_1267mbps_19_2mhz,
> >>  		}
> >>  	},
> >>  	[IMX258_LINK_FREQ_640MBPS] = {
> >>  		.pixels_per_line = IMX258_PPL_DEFAULT,
> >>  		.reg_list = {
> >> -			.num_of_regs = ARRAY_SIZE(mipi_data_rate_640mbps),
> >> -			.regs = mipi_data_rate_640mbps,
> >> +			.num_of_regs = ARRAY_SIZE(mipi_640mbps_19_2mhz),
> >> +			.regs = mipi_640mbps_19_2mhz,
> >> +		}
> >> +	},
> >> +};
> >> +
> >> +static const struct imx258_link_freq_config link_freq_configs_24[] = {
> >> +	[IMX258_LINK_FREQ_1267MBPS] = {
> >> +		.pixels_per_line = IMX258_PPL_DEFAULT,
> >> +		.reg_list = {
> >> +			.num_of_regs = ARRAY_SIZE(mipi_1272mbps_24mhz),
> >> +			.regs = mipi_1272mbps_24mhz,
> >> +		}
> >> +	},
> >> +	[IMX258_LINK_FREQ_640MBPS] = {
> >> +		.pixels_per_line = IMX258_PPL_DEFAULT,
> >> +		.reg_list = {
> >> +			.num_of_regs = ARRAY_SIZE(mipi_642mbps_24mhz),
> >> +			.regs = mipi_642mbps_24mhz,
> >>  		}
> >>  	},
> >>  };
> >> @@ -410,6 +475,9 @@ struct imx258 {
> >>  	/* Current mode */
> >>  	const struct imx258_mode *cur_mode;
> >>  
> >> +	const struct imx258_link_freq_config *link_freq_configs;
> >> +	const s64 *link_freq_menu_items;
> >> +
> >>  	/*
> >>  	 * Mutex for serialized access:
> >>  	 * Protect sensor module set pad format and start/stop streaming safely.
> >> @@ -713,7 +781,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
> >>  		imx258->cur_mode = mode;
> >>  		__v4l2_ctrl_s_ctrl(imx258->link_freq, mode->link_freq_index);
> >>  
> >> -		link_freq = link_freq_menu_items[mode->link_freq_index];
> >> +		link_freq = imx258->link_freq_menu_items[mode->link_freq_index];
> >>  		pixel_rate = link_freq_to_pixel_rate(link_freq);
> >>  		__v4l2_ctrl_s_ctrl_int64(imx258->pixel_rate, pixel_rate);
> >>  		/* Update limits and set FPS to default */
> >> @@ -727,7 +795,7 @@ static int imx258_set_pad_format(struct v4l2_subdev *sd,
> >>  			vblank_def);
> >>  		__v4l2_ctrl_s_ctrl(imx258->vblank, vblank_def);
> >>  		h_blank =
> >> -			link_freq_configs[mode->link_freq_index].pixels_per_line
> >> +			imx258->link_freq_configs[mode->link_freq_index].pixels_per_line
> >>  			 - imx258->cur_mode->width;
> >>  		__v4l2_ctrl_modify_range(imx258->hblank, h_blank,
> >>  					 h_blank, 1, h_blank);
> >> @@ -747,7 +815,7 @@ static int imx258_start_streaming(struct imx258 *imx258)
> >>  
> >>  	/* Setup PLL */
> >>  	link_freq_index = imx258->cur_mode->link_freq_index;
> >> -	reg_list = &link_freq_configs[link_freq_index].reg_list;
> >> +	reg_list = &imx258->link_freq_configs[link_freq_index].reg_list;
> >>  	ret = imx258_write_regs(imx258, reg_list->regs, reg_list->num_of_regs);
> >>  	if (ret) {
> >>  		dev_err(&client->dev, "%s failed to set plls\n", __func__);
> >> @@ -946,9 +1014,9 @@ static int imx258_init_controls(struct imx258 *imx258)
> >>  	imx258->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> >>  				&imx258_ctrl_ops,
> >>  				V4L2_CID_LINK_FREQ,
> >> -				ARRAY_SIZE(link_freq_menu_items) - 1,
> >> +				ARRAY_SIZE(link_freq_menu_items_19_2) - 1,
> >>  				0,
> >> -				link_freq_menu_items);
> >> +				imx258->link_freq_menu_items);
> >>  
> >>  	if (imx258->link_freq)
> >>  		imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >> @@ -964,8 +1032,10 @@ static int imx258_init_controls(struct imx258 *imx258)
> >>  	if (vflip)
> >>  		vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >>  
> >> -	pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
> >> -	pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
> >> +	pixel_rate_max =
> >> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[0]);
> >> +	pixel_rate_min =
> >> +		link_freq_to_pixel_rate(imx258->link_freq_menu_items[1]);
> > 
> > The arrays currently have two entries so this works but it'd nice to have a
> > bit more robust way to handle differences between the two arrays. Could you
> > maintain e.g. the number of entries in the array in a struct field perhaps?
> 
> Would it make more sense to do something like default to index 0 and then use 
> ARRAY_SIZE to iterate through the array and do a comparison to get the min and
> max size so it would always choose the correct value no matter how many entries
> there are?

I'll check that later patch you mentioned in the follow-up separately.