[PATCH 0/3] staging: media: max96712: a few, mainly cosmetic, changes

[PATCH 0/3] staging: media: max96712: a few, mainly cosmetic, changes

[Laurentiu Palcu]

Hi,

This is in preparation for bringing our downstream max96724 work into
the max96712 driver. The changes in this patch-set are mainly cosmetic
to increase readability, with one exception related to the PCLK debug
register which appears to exist in MAX96724 as well(see VPG section in
users guide [1]).

Unfortunately, I couldn't find any max96712 reference manual anywhere to
make sure the registers match between the 2 chips, nor I have a max96712
to test with.

However, since the max96724 compatible was added here, according to the
commit

6c872b782ae ("media: staging: max96712: Add support for MAX96724")

the chips are "almost identical". So, I used max96724 register and
field descriptions instead. Someone with a max96712 RM may want to
double-check though.

Thanks,
Laurentiu

[1] https://www.analog.com/media/en/technical-documentation/user-guides/max96724fr-user-guide.pdf

Laurentiu Palcu (3):
  staging: media: max96712: fix typo
  staging: media: max96712: enable extra debug register for max96724
  staging: media: max96712: add some register and field definitions

 drivers/staging/media/max96712/max96712.c | 210 +++++++++++++++++-----
 1 file changed, 165 insertions(+), 45 deletions(-)

-- 
2.44.1

[PATCH 1/3] staging: media: max96712: fix typo

[Laurentiu Palcu]

Fix a typo in VS_LOW expression.

Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
---
 drivers/staging/media/max96712/max96712.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
index 0751b2e048958..5228f9ec60859 100644
--- a/drivers/staging/media/max96712/max96712.c
+++ b/drivers/staging/media/max96712/max96712.c
@@ -182,7 +182,7 @@ static void max96712_pattern_enable(struct max96712_priv *priv, bool enable)
 	max96712_write_bulk_value(priv, 0x1052, 0, 3);
 	max96712_write_bulk_value(priv, 0x1055, v_sw * h_tot, 3);
 	max96712_write_bulk_value(priv, 0x1058,
-				  (v_active + v_fp + + v_bp) * h_tot, 3);
+				  (v_active + v_fp + v_bp) * h_tot, 3);
 	max96712_write_bulk_value(priv, 0x105b, 0, 3);
 	max96712_write_bulk_value(priv, 0x105e, h_sw, 2);
 	max96712_write_bulk_value(priv, 0x1060, h_active + h_fp + h_bp, 2);
-- 
2.44.1

[PATCH 2/3] staging: media: max96712: enable extra debug register for max96724

[Laurentiu Palcu]

The current implementation has the register disabled since it is not
documented in the MAX96724 RM. However, in the chip's user's guide [1],
in the Video Pattern Generator section, the register is documented and
it appears to work just fine. Before the change I was experiencing
approx 10fps when streaming, after: 30.3fps.

[1] https://www.analog.com/media/en/technical-documentation/user-guides/max96724fr-user-guide.pdf

Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
---
 drivers/staging/media/max96712/max96712.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
index 5228f9ec60859..682ebd20851f7 100644
--- a/drivers/staging/media/max96712/max96712.c
+++ b/drivers/staging/media/max96712/max96712.c
@@ -462,6 +462,7 @@ static const struct max96712_info max96712_info_max96712 = {
 
 static const struct max96712_info max96712_info_max96724 = {
 	.dpllfreq = 1200,
+	.have_debug_extra = true,
 };
 
 static const struct of_device_id max96712_of_table[] = {
-- 
2.44.1

[PATCH 3/3] staging: media: max96712: add some register and field definitions

[Laurentiu Palcu]

Most of the registers in the driver, and their values, are hard-coded
which makes the code rather difficult to read and maintain. Add register
and field definitions and use them to replace the hard-coded values
where possible.

This patch does not change the driver functionality or its structure.

Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
---
 drivers/staging/media/max96712/max96712.c | 207 +++++++++++++++++-----
 1 file changed, 163 insertions(+), 44 deletions(-)

diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
index 682ebd20851f7..a3e4b51dc5048 100644
--- a/drivers/staging/media/max96712/max96712.c
+++ b/drivers/staging/media/max96712/max96712.c
@@ -16,9 +16,118 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
-#define DEBUG_EXTRA_REG			0x09
-#define DEBUG_EXTRA_PCLK_25MHZ		0x00
-#define DEBUG_EXTRA_PCLK_75MHZ		0x01
+/* TOP_CTRL */
+#define MAX96712_DEBUG_EXTRA_REG			0x0009
+#define   DEBUG_EXTRA_PCLK_25MHZ			0x00
+#define   DEBUG_EXTRA_PCLK_75MHZ			0x01
+#define MAX96724_TOP_CTRL_PWR1				0x0013
+#define   RESET_ALL					BIT(6)
+
+/* BACKTOP0 */
+#define MAX96712_BACKTOP0_12				0x040b
+#define   CSI_OUT_EN					BIT(1)
+#define   SOFT_BPP_0_MASK				GENMASK(7, 3)
+#define   SOFT_BPP_0_SHIFT				3
+#define MAX96712_BACKTOP0_22				0x0415
+#define MAX96712_BACKTOP0_25				0x0418
+#define   PHY_CSI_TX_DPLL_PREDEF_FREQ_MASK		GENMASK(4, 0)
+#define   PHY_CSI_TX_DPLL_PREDEF_FREQ_SHIFT		0
+#define   PHY_CSI_TX_DPLL_FB_FRACTION_PREDEF_EN		BIT(5)
+#define   OVERRIDE_BPP_VC_DT_0_2			BIT(6)
+#define   OVERRIDE_BPP_VC_DT_1_3			BIT(7)
+
+/* MIPI_PHY */
+#define MAX96712_MIPI_PHY_0				0x08a0
+#define   PHY_4X2					BIT(0)
+#define   PHY_2X4					BIT(2)
+#define   PHY_1X4A_22					BIT(3)
+#define   PHY_1X4B_22					BIT(4)
+#define   FORCE_CLK0_EN					BIT(5)
+#define   FORCE_CLK3_EN					BIT(6)
+#define   FORCE_CSI_OUT_EN				BIT(7)
+#define MAX96712_MIPI_PHY_2				0x08a2
+#define   T_HS_TRAIL_MASK				GENMASK(1, 0)
+#define   T_HS_TRAIL_SHIFT				0
+#define   T_LPX_MASK					GENMASK(3, 2)
+#define   T_LPX_SHIFT					2
+#define   PHY_STDBY_N_MASK				GENMASK(7, 4)
+#define   PHY_STDBY_N_SHIFT				4
+#define   PHY0_EN					BIT(4)
+#define   PHY1_EN					BIT(5)
+#define   PHY2_EN					BIT(6)
+#define   PHY3_EN					BIT(7)
+#define MAX96712_MIPI_PHY_3				0x08a3
+#define   PHY0_LANE_MAP_MASK				GENMASK(3, 0)
+#define   PHY0_LANE_MAP_SHIFT				0
+#define   PHY1_LANE_MAP_MASK				GENMASK(7, 4)
+#define   PHY1_LANE_MAP_SHIFT				4
+#define MAX96712_MIPI_PHY_5				0x08a5
+#define   PHY0_POL_MAP_MASK				GENMASK(2, 0)
+#define   PHY0_POL_MAP_SHIFT				0
+#define   PHY1_POL_MAP_MASK				GENMASK(5, 3)
+#define   PHY1_POL_MAP_SHIFT				3
+#define   T_CLK_PREP_MASK				GENMASK(7, 6)
+#define   T_CLK_PREP_SHIFT				6
+#define MAX96712_MIPI_PHY_13				0x08ad
+#define   T_T3_PREBEGIN_MASK				GENMASK(5, 0)
+#define   T_T3_PREBEGIN_SHIFT				0
+#define MAX96712_MIPI_PHY_14				0x08ae
+#define   T_T3_PREP_MASK				GENMASK(1, 0)
+#define   T_T3_PREP_SHIFT				0
+#define   T_T3_PREP_40NS				0
+#define   T_T3_PREP_55NS				1
+#define   T_T3_PREP_66_7NS				2
+#define   T_T3_PREP_86_7NS				3
+#define   T_T3_POST_MASK				GENMASK(6, 2)
+#define   T_T3_POST_SHIFT				2
+
+/* MIPI_TX: 0 <= phy < 4 */
+#define MAX96712_MIPI_TX_10(phy)			(0x090a + (phy) * 0x40)
+#define   CSI2_TWAKEUP_H_MASK				GENMASK(2, 0)
+#define   CSI2_TWAKEUP_H_SHIFT				0
+#define   CSI2_VCX_EN					BIT(4)
+#define   CSI2_CPHY_EN					BIT(5)
+#define   CSI2_LANE_CNT_MASK				GENMASK(7, 6)
+#define   CSI2_LANE_CNT_SHIFT				6
+
+/* VRX_PATGEN */
+#define MAX96712_VRX_PATGEN_0				0x1050
+#define   VTG_MODE_MASK					GENMASK(1, 0)
+#define   VTG_MODE_SHIFT				0
+#define   VTG_MODE_VS_TRACKING				0
+#define   VTG_MODE_VS_TRIGGER				1
+#define   VTG_MODE_AUTO_REPEAT				2
+#define   VTG_MODE_FREE_RUNNING				3
+#define   DE_INV					BIT(2)
+#define   HS_INV					BIT(3)
+#define   VS_INV					BIT(4)
+#define   GEN_DE					BIT(5)
+#define   GEN_HS					BIT(6)
+#define   GEN_VS					BIT(7)
+#define MAX96712_VRX_PATGEN_1				0x1051
+#define   VS_TRIG					BIT(0)
+#define   PATGEN_MODE_MASK				GENMASK(5, 4)
+#define   PATGEN_MODE_SHIFT				4
+#define   PATGEN_MODE_CHECKERBOARD			(1 << PATGEN_MODE_SHIFT)
+#define   PATGEN_MODE_GRADIENT				(2 << PATGEN_MODE_SHIFT)
+#define   GRAD_MODE					BIT(7)
+#define MAX96712_VRX_PATGEN_VS_DLY			0x1052
+#define MAX96712_VRX_PATGEN_VS_HIGH			0x1055
+#define MAX96712_VRX_PATGEN_VS_LOW			0x1058
+#define MAX96712_VRX_PATGEN_V2H				0x105b
+#define MAX96712_VRX_PATGEN_HS_HIGH			0x105e
+#define MAX96712_VRX_PATGEN_HS_LOW			0x1060
+#define MAX96712_VRX_PATGEN_HS_CNT			0x1062
+#define MAX96712_VRX_PATGEN_V2D				0x1064
+#define MAX96712_VRX_PATGEN_DE_HIGH			0x1067
+#define MAX96712_VRX_PATGEN_DE_LOW			0x1069
+#define MAX96712_VRX_PATGEN_DE_CNT			0x106b
+#define MAX96712_VRX_PATGEN_GRAD_INCR			0x106d
+#define MAX96712_VRX_PATGEN_CHKR_COLOR_A		0x106e
+#define MAX96712_VRX_PATGEN_CHKR_COLOR_B		0x1071
+#define MAX96712_VRX_PATGEN_CHKR_RPT_A			0x1074
+#define MAX96712_VRX_PATGEN_CHKR_RPT_B			0x1075
+#define MAX96712_VRX_PATGEN_CHKR_ALT			0x1076
 
 enum max96712_pattern {
 	MAX96712_PATTERN_CHECKERBOARD = 0,
@@ -97,18 +206,18 @@ static int max96712_write_bulk_value(struct max96712_priv *priv,
 
 static void max96712_reset(struct max96712_priv *priv)
 {
-	max96712_update_bits(priv, 0x13, 0x40, 0x40);
+	max96712_update_bits(priv, MAX96724_TOP_CTRL_PWR1, RESET_ALL, RESET_ALL);
 	msleep(20);
 }
 
 static void max96712_mipi_enable(struct max96712_priv *priv, bool enable)
 {
 	if (enable) {
-		max96712_update_bits(priv, 0x40b, 0x02, 0x02);
-		max96712_update_bits(priv, 0x8a0, 0x80, 0x80);
+		max96712_update_bits(priv, MAX96712_BACKTOP0_12, CSI_OUT_EN, CSI_OUT_EN);
+		max96712_update_bits(priv, MAX96712_MIPI_PHY_0, FORCE_CSI_OUT_EN, FORCE_CSI_OUT_EN);
 	} else {
-		max96712_update_bits(priv, 0x8a0, 0x80, 0x00);
-		max96712_update_bits(priv, 0x40b, 0x02, 0x00);
+		max96712_update_bits(priv, MAX96712_MIPI_PHY_0, FORCE_CSI_OUT_EN, 0x00);
+		max96712_update_bits(priv, MAX96712_BACKTOP0_12, CSI_OUT_EN, 0x00);
 	}
 }
 
@@ -120,39 +229,47 @@ static void max96712_mipi_configure(struct max96712_priv *priv)
 	max96712_mipi_enable(priv, false);
 
 	/* Select 2x4 mode. */
-	max96712_write(priv, 0x8a0, 0x04);
+	max96712_write(priv, MAX96712_MIPI_PHY_0, PHY_2X4);
 
 	/* TODO: Add support for 2-lane and 1-lane configurations. */
 	if (priv->cphy) {
-		/* Configure a 3-lane C-PHY using PHY0 and PHY1. */
-		max96712_write(priv, 0x94a, 0xa0);
+		/* Configure a 3-lane C-PHY using PHY1. */
+		max96712_write(priv, MAX96712_MIPI_TX_10(1),
+			       (2 << CSI2_LANE_CNT_SHIFT) | CSI2_CPHY_EN);
 
 		/* Configure C-PHY timings. */
-		max96712_write(priv, 0x8ad, 0x3f);
-		max96712_write(priv, 0x8ae, 0x7d);
+		max96712_write(priv, MAX96712_MIPI_PHY_13, 0x3f);
+		max96712_write(priv, MAX96712_MIPI_PHY_14,
+			       (0x1f << T_T3_POST_SHIFT) | T_T3_PREP_55NS);
 	} else {
-		/* Configure a 4-lane D-PHY using PHY0 and PHY1. */
-		max96712_write(priv, 0x94a, 0xc0);
+		/* Configure a 4-lane D-PHY using PHY1. */
+		max96712_write(priv, MAX96712_MIPI_TX_10(1), 3 << CSI2_LANE_CNT_SHIFT);
 	}
 
 	/* Configure lane mapping for PHY0 and PHY1. */
 	/* TODO: Add support for lane swapping. */
-	max96712_write(priv, 0x8a3, 0xe4);
+	max96712_write(priv, MAX96712_MIPI_PHY_3, 0xe4);
 
 	/* Configure lane polarity for PHY0 and PHY1. */
 	for (i = 0; i < priv->mipi.num_data_lanes + 1; i++)
 		if (priv->mipi.lane_polarities[i])
 			phy5 |= BIT(i == 0 ? 5 : i < 3 ? i - 1 : i);
-	max96712_write(priv, 0x8a5, phy5);
+	max96712_write(priv, MAX96712_MIPI_PHY_5, phy5);
 
 	/* Set link frequency for PHY0 and PHY1. */
-	max96712_update_bits(priv, 0x415, 0x3f,
-			     ((priv->info->dpllfreq / 100) & 0x1f) | BIT(5));
-	max96712_update_bits(priv, 0x418, 0x3f,
-			     ((priv->info->dpllfreq / 100) & 0x1f) | BIT(5));
+	max96712_update_bits(priv, MAX96712_BACKTOP0_22,
+			     PHY_CSI_TX_DPLL_FB_FRACTION_PREDEF_EN |
+			     PHY_CSI_TX_DPLL_PREDEF_FREQ_MASK,
+			     PHY_CSI_TX_DPLL_FB_FRACTION_PREDEF_EN |
+			     ((priv->info->dpllfreq / 100) & 0x1f));
+	max96712_update_bits(priv, MAX96712_BACKTOP0_25,
+			     PHY_CSI_TX_DPLL_FB_FRACTION_PREDEF_EN |
+			     PHY_CSI_TX_DPLL_PREDEF_FREQ_MASK,
+			     PHY_CSI_TX_DPLL_FB_FRACTION_PREDEF_EN |
+			     ((priv->info->dpllfreq / 100) & 0x1f));
 
 	/* Enable PHY0 and PHY1 */
-	max96712_update_bits(priv, 0x8a2, 0xf0, 0x30);
+	max96712_update_bits(priv, MAX96712_MIPI_PHY_2, PHY_STDBY_N_MASK, PHY0_EN | PHY1_EN);
 }
 
 static void max96712_pattern_enable(struct max96712_priv *priv, bool enable)
@@ -170,51 +287,53 @@ static void max96712_pattern_enable(struct max96712_priv *priv, bool enable)
 	const u32 v_tot = v_active + v_fp + v_sw + v_bp;
 
 	if (!enable) {
-		max96712_write(priv, 0x1051, 0x00);
+		max96712_write(priv, MAX96712_VRX_PATGEN_1, 0x00);
 		return;
 	}
 
 	/* Set PCLK to 75MHz if device have DEBUG_EXTRA register. */
 	if (priv->info->have_debug_extra)
-		max96712_write(priv, DEBUG_EXTRA_REG, DEBUG_EXTRA_PCLK_75MHZ);
+		max96712_write(priv, MAX96712_DEBUG_EXTRA_REG, DEBUG_EXTRA_PCLK_75MHZ);
 
 	/* Configure Video Timing Generator for 1920x1080 @ 30 fps. */
-	max96712_write_bulk_value(priv, 0x1052, 0, 3);
-	max96712_write_bulk_value(priv, 0x1055, v_sw * h_tot, 3);
-	max96712_write_bulk_value(priv, 0x1058,
+	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_VS_DLY, 0, 3);
+	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_VS_HIGH, v_sw * h_tot, 3);
+	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_VS_LOW,
 				  (v_active + v_fp + v_bp) * h_tot, 3);
-	max96712_write_bulk_value(priv, 0x105b, 0, 3);
-	max96712_write_bulk_value(priv, 0x105e, h_sw, 2);
-	max96712_write_bulk_value(priv, 0x1060, h_active + h_fp + h_bp, 2);
-	max96712_write_bulk_value(priv, 0x1062, v_tot, 2);
-	max96712_write_bulk_value(priv, 0x1064,
+	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_V2H, 0, 3);
+	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_HS_HIGH, h_sw, 2);
+	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_HS_LOW, h_active + h_fp + h_bp, 2);
+	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_HS_CNT, v_tot, 2);
+	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_V2D,
 				  h_tot * (v_sw + v_bp) + (h_sw + h_bp), 3);
-	max96712_write_bulk_value(priv, 0x1067, h_active, 2);
-	max96712_write_bulk_value(priv, 0x1069, h_fp + h_sw + h_bp, 2);
-	max96712_write_bulk_value(priv, 0x106b, v_active, 2);
+	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_DE_HIGH, h_active, 2);
+	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_DE_LOW, h_fp + h_sw + h_bp, 2);
+	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_DE_CNT, v_active, 2);
 
 	/* Generate VS, HS and DE in free-running mode. */
-	max96712_write(priv, 0x1050, 0xfb);
+	max96712_write(priv, MAX96712_VRX_PATGEN_0,
+		       GEN_VS | GEN_HS | GEN_DE | VS_INV | HS_INV |
+		       (VTG_MODE_FREE_RUNNING << VTG_MODE_SHIFT));
 
 	/* Configure Video Pattern Generator. */
 	if (priv->pattern == MAX96712_PATTERN_CHECKERBOARD) {
 		/* Set checkerboard pattern size. */
-		max96712_write(priv, 0x1074, 0x3c);
-		max96712_write(priv, 0x1075, 0x3c);
-		max96712_write(priv, 0x1076, 0x3c);
+		max96712_write(priv, MAX96712_VRX_PATGEN_CHKR_RPT_A, 0x3c);
+		max96712_write(priv, MAX96712_VRX_PATGEN_CHKR_RPT_B, 0x3c);
+		max96712_write(priv, MAX96712_VRX_PATGEN_CHKR_ALT, 0x3c);
 
 		/* Set checkerboard pattern colors. */
-		max96712_write_bulk_value(priv, 0x106e, 0xfecc00, 3);
-		max96712_write_bulk_value(priv, 0x1071, 0x006aa7, 3);
+		max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_CHKR_COLOR_A, 0xfecc00, 3);
+		max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_CHKR_COLOR_B, 0x006aa7, 3);
 
 		/* Generate checkerboard pattern. */
-		max96712_write(priv, 0x1051, 0x10);
+		max96712_write(priv, MAX96712_VRX_PATGEN_1, PATGEN_MODE_CHECKERBOARD);
 	} else {
 		/* Set gradient increment. */
-		max96712_write(priv, 0x106d, 0x10);
+		max96712_write(priv, MAX96712_VRX_PATGEN_GRAD_INCR, 0x10);
 
 		/* Generate gradient pattern. */
-		max96712_write(priv, 0x1051, 0x20);
+		max96712_write(priv, MAX96712_VRX_PATGEN_1, PATGEN_MODE_GRADIENT);
 	}
 }
 
-- 
2.44.1

Re: [PATCH 1/3] staging: media: max96712: fix typo

[Niklas Söderlund]

Hello Laurentiu,

Thanks for your work.

On 2024-12-19 17:06:41 +0200, Laurentiu Palcu wrote:
> Fix a typo in VS_LOW expression.
> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/staging/media/max96712/max96712.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
> index 0751b2e048958..5228f9ec60859 100644
> --- a/drivers/staging/media/max96712/max96712.c
> +++ b/drivers/staging/media/max96712/max96712.c
> @@ -182,7 +182,7 @@ static void max96712_pattern_enable(struct max96712_priv *priv, bool enable)
>  	max96712_write_bulk_value(priv, 0x1052, 0, 3);
>  	max96712_write_bulk_value(priv, 0x1055, v_sw * h_tot, 3);
>  	max96712_write_bulk_value(priv, 0x1058,
> -				  (v_active + v_fp + + v_bp) * h_tot, 3);
> +				  (v_active + v_fp + v_bp) * h_tot, 3);
>  	max96712_write_bulk_value(priv, 0x105b, 0, 3);
>  	max96712_write_bulk_value(priv, 0x105e, h_sw, 2);
>  	max96712_write_bulk_value(priv, 0x1060, h_active + h_fp + h_bp, 2);
> -- 
> 2.44.1
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH 2/3] staging: media: max96712: enable extra debug register for max96724

[Niklas Söderlund]

Hello Laurentiu,

Thanks for your work.

On 2024-12-19 17:06:42 +0200, Laurentiu Palcu wrote:
> The current implementation has the register disabled since it is not
> documented in the MAX96724 RM. However, in the chip's user's guide [1],
> in the Video Pattern Generator section, the register is documented and
> it appears to work just fine. Before the change I was experiencing
> approx 10fps when streaming, after: 30.3fps.
> 
> [1] https://www.analog.com/media/en/technical-documentation/user-guides/max96724fr-user-guide.pdf
> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> ---
>  drivers/staging/media/max96712/max96712.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
> index 5228f9ec60859..682ebd20851f7 100644
> --- a/drivers/staging/media/max96712/max96712.c
> +++ b/drivers/staging/media/max96712/max96712.c
> @@ -462,6 +462,7 @@ static const struct max96712_info max96712_info_max96712 = {
>  
>  static const struct max96712_info max96712_info_max96724 = {
>  	.dpllfreq = 1200,
> +	.have_debug_extra = true,

This flag was added as I could not find the register in the MAX96724 
datasheet I had access to at the time. As you now prove the register 
exists in the user's guide and that it works I think a better solution 
is to remove the 'have_debug_extra' flag all together then set it for 
both supported devices.

>  };
>  
>  static const struct of_device_id max96712_of_table[] = {
> -- 
> 2.44.1
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH 3/3] staging: media: max96712: add some register and field definitions

[Niklas Söderlund]

Hi Laurentiu,

Thanks for your patch.

On 2024-12-19 17:06:43 +0200, Laurentiu Palcu wrote:
> Most of the registers in the driver, and their values, are hard-coded
> which makes the code rather difficult to read and maintain. Add register
> and field definitions and use them to replace the hard-coded values
> where possible.
> 
> This patch does not change the driver functionality or its structure.
> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>

Nice clean up, thanks for this!

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/staging/media/max96712/max96712.c | 207 +++++++++++++++++-----
>  1 file changed, 163 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
> index 682ebd20851f7..a3e4b51dc5048 100644
> --- a/drivers/staging/media/max96712/max96712.c
> +++ b/drivers/staging/media/max96712/max96712.c
> @@ -16,9 +16,118 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> -#define DEBUG_EXTRA_REG			0x09
> -#define DEBUG_EXTRA_PCLK_25MHZ		0x00
> -#define DEBUG_EXTRA_PCLK_75MHZ		0x01
> +/* TOP_CTRL */
> +#define MAX96712_DEBUG_EXTRA_REG			0x0009
> +#define   DEBUG_EXTRA_PCLK_25MHZ			0x00
> +#define   DEBUG_EXTRA_PCLK_75MHZ			0x01
> +#define MAX96724_TOP_CTRL_PWR1				0x0013
> +#define   RESET_ALL					BIT(6)
> +
> +/* BACKTOP0 */
> +#define MAX96712_BACKTOP0_12				0x040b
> +#define   CSI_OUT_EN					BIT(1)
> +#define   SOFT_BPP_0_MASK				GENMASK(7, 3)
> +#define   SOFT_BPP_0_SHIFT				3
> +#define MAX96712_BACKTOP0_22				0x0415
> +#define MAX96712_BACKTOP0_25				0x0418
> +#define   PHY_CSI_TX_DPLL_PREDEF_FREQ_MASK		GENMASK(4, 0)
> +#define   PHY_CSI_TX_DPLL_PREDEF_FREQ_SHIFT		0
> +#define   PHY_CSI_TX_DPLL_FB_FRACTION_PREDEF_EN		BIT(5)
> +#define   OVERRIDE_BPP_VC_DT_0_2			BIT(6)
> +#define   OVERRIDE_BPP_VC_DT_1_3			BIT(7)
> +
> +/* MIPI_PHY */
> +#define MAX96712_MIPI_PHY_0				0x08a0
> +#define   PHY_4X2					BIT(0)
> +#define   PHY_2X4					BIT(2)
> +#define   PHY_1X4A_22					BIT(3)
> +#define   PHY_1X4B_22					BIT(4)
> +#define   FORCE_CLK0_EN					BIT(5)
> +#define   FORCE_CLK3_EN					BIT(6)
> +#define   FORCE_CSI_OUT_EN				BIT(7)
> +#define MAX96712_MIPI_PHY_2				0x08a2
> +#define   T_HS_TRAIL_MASK				GENMASK(1, 0)
> +#define   T_HS_TRAIL_SHIFT				0
> +#define   T_LPX_MASK					GENMASK(3, 2)
> +#define   T_LPX_SHIFT					2
> +#define   PHY_STDBY_N_MASK				GENMASK(7, 4)
> +#define   PHY_STDBY_N_SHIFT				4
> +#define   PHY0_EN					BIT(4)
> +#define   PHY1_EN					BIT(5)
> +#define   PHY2_EN					BIT(6)
> +#define   PHY3_EN					BIT(7)
> +#define MAX96712_MIPI_PHY_3				0x08a3
> +#define   PHY0_LANE_MAP_MASK				GENMASK(3, 0)
> +#define   PHY0_LANE_MAP_SHIFT				0
> +#define   PHY1_LANE_MAP_MASK				GENMASK(7, 4)
> +#define   PHY1_LANE_MAP_SHIFT				4
> +#define MAX96712_MIPI_PHY_5				0x08a5
> +#define   PHY0_POL_MAP_MASK				GENMASK(2, 0)
> +#define   PHY0_POL_MAP_SHIFT				0
> +#define   PHY1_POL_MAP_MASK				GENMASK(5, 3)
> +#define   PHY1_POL_MAP_SHIFT				3
> +#define   T_CLK_PREP_MASK				GENMASK(7, 6)
> +#define   T_CLK_PREP_SHIFT				6
> +#define MAX96712_MIPI_PHY_13				0x08ad
> +#define   T_T3_PREBEGIN_MASK				GENMASK(5, 0)
> +#define   T_T3_PREBEGIN_SHIFT				0
> +#define MAX96712_MIPI_PHY_14				0x08ae
> +#define   T_T3_PREP_MASK				GENMASK(1, 0)
> +#define   T_T3_PREP_SHIFT				0
> +#define   T_T3_PREP_40NS				0
> +#define   T_T3_PREP_55NS				1
> +#define   T_T3_PREP_66_7NS				2
> +#define   T_T3_PREP_86_7NS				3
> +#define   T_T3_POST_MASK				GENMASK(6, 2)
> +#define   T_T3_POST_SHIFT				2
> +
> +/* MIPI_TX: 0 <= phy < 4 */
> +#define MAX96712_MIPI_TX_10(phy)			(0x090a + (phy) * 0x40)
> +#define   CSI2_TWAKEUP_H_MASK				GENMASK(2, 0)
> +#define   CSI2_TWAKEUP_H_SHIFT				0
> +#define   CSI2_VCX_EN					BIT(4)
> +#define   CSI2_CPHY_EN					BIT(5)
> +#define   CSI2_LANE_CNT_MASK				GENMASK(7, 6)
> +#define   CSI2_LANE_CNT_SHIFT				6
> +
> +/* VRX_PATGEN */
> +#define MAX96712_VRX_PATGEN_0				0x1050
> +#define   VTG_MODE_MASK					GENMASK(1, 0)
> +#define   VTG_MODE_SHIFT				0
> +#define   VTG_MODE_VS_TRACKING				0
> +#define   VTG_MODE_VS_TRIGGER				1
> +#define   VTG_MODE_AUTO_REPEAT				2
> +#define   VTG_MODE_FREE_RUNNING				3
> +#define   DE_INV					BIT(2)
> +#define   HS_INV					BIT(3)
> +#define   VS_INV					BIT(4)
> +#define   GEN_DE					BIT(5)
> +#define   GEN_HS					BIT(6)
> +#define   GEN_VS					BIT(7)
> +#define MAX96712_VRX_PATGEN_1				0x1051
> +#define   VS_TRIG					BIT(0)
> +#define   PATGEN_MODE_MASK				GENMASK(5, 4)
> +#define   PATGEN_MODE_SHIFT				4
> +#define   PATGEN_MODE_CHECKERBOARD			(1 << PATGEN_MODE_SHIFT)
> +#define   PATGEN_MODE_GRADIENT				(2 << PATGEN_MODE_SHIFT)
> +#define   GRAD_MODE					BIT(7)
> +#define MAX96712_VRX_PATGEN_VS_DLY			0x1052
> +#define MAX96712_VRX_PATGEN_VS_HIGH			0x1055
> +#define MAX96712_VRX_PATGEN_VS_LOW			0x1058
> +#define MAX96712_VRX_PATGEN_V2H				0x105b
> +#define MAX96712_VRX_PATGEN_HS_HIGH			0x105e
> +#define MAX96712_VRX_PATGEN_HS_LOW			0x1060
> +#define MAX96712_VRX_PATGEN_HS_CNT			0x1062
> +#define MAX96712_VRX_PATGEN_V2D				0x1064
> +#define MAX96712_VRX_PATGEN_DE_HIGH			0x1067
> +#define MAX96712_VRX_PATGEN_DE_LOW			0x1069
> +#define MAX96712_VRX_PATGEN_DE_CNT			0x106b
> +#define MAX96712_VRX_PATGEN_GRAD_INCR			0x106d
> +#define MAX96712_VRX_PATGEN_CHKR_COLOR_A		0x106e
> +#define MAX96712_VRX_PATGEN_CHKR_COLOR_B		0x1071
> +#define MAX96712_VRX_PATGEN_CHKR_RPT_A			0x1074
> +#define MAX96712_VRX_PATGEN_CHKR_RPT_B			0x1075
> +#define MAX96712_VRX_PATGEN_CHKR_ALT			0x1076
>  
>  enum max96712_pattern {
>  	MAX96712_PATTERN_CHECKERBOARD = 0,
> @@ -97,18 +206,18 @@ static int max96712_write_bulk_value(struct max96712_priv *priv,
>  
>  static void max96712_reset(struct max96712_priv *priv)
>  {
> -	max96712_update_bits(priv, 0x13, 0x40, 0x40);
> +	max96712_update_bits(priv, MAX96724_TOP_CTRL_PWR1, RESET_ALL, RESET_ALL);
>  	msleep(20);
>  }
>  
>  static void max96712_mipi_enable(struct max96712_priv *priv, bool enable)
>  {
>  	if (enable) {
> -		max96712_update_bits(priv, 0x40b, 0x02, 0x02);
> -		max96712_update_bits(priv, 0x8a0, 0x80, 0x80);
> +		max96712_update_bits(priv, MAX96712_BACKTOP0_12, CSI_OUT_EN, CSI_OUT_EN);
> +		max96712_update_bits(priv, MAX96712_MIPI_PHY_0, FORCE_CSI_OUT_EN, FORCE_CSI_OUT_EN);
>  	} else {
> -		max96712_update_bits(priv, 0x8a0, 0x80, 0x00);
> -		max96712_update_bits(priv, 0x40b, 0x02, 0x00);
> +		max96712_update_bits(priv, MAX96712_MIPI_PHY_0, FORCE_CSI_OUT_EN, 0x00);
> +		max96712_update_bits(priv, MAX96712_BACKTOP0_12, CSI_OUT_EN, 0x00);
>  	}
>  }
>  
> @@ -120,39 +229,47 @@ static void max96712_mipi_configure(struct max96712_priv *priv)
>  	max96712_mipi_enable(priv, false);
>  
>  	/* Select 2x4 mode. */
> -	max96712_write(priv, 0x8a0, 0x04);
> +	max96712_write(priv, MAX96712_MIPI_PHY_0, PHY_2X4);
>  
>  	/* TODO: Add support for 2-lane and 1-lane configurations. */
>  	if (priv->cphy) {
> -		/* Configure a 3-lane C-PHY using PHY0 and PHY1. */
> -		max96712_write(priv, 0x94a, 0xa0);
> +		/* Configure a 3-lane C-PHY using PHY1. */
> +		max96712_write(priv, MAX96712_MIPI_TX_10(1),
> +			       (2 << CSI2_LANE_CNT_SHIFT) | CSI2_CPHY_EN);
>  
>  		/* Configure C-PHY timings. */
> -		max96712_write(priv, 0x8ad, 0x3f);
> -		max96712_write(priv, 0x8ae, 0x7d);
> +		max96712_write(priv, MAX96712_MIPI_PHY_13, 0x3f);
> +		max96712_write(priv, MAX96712_MIPI_PHY_14,
> +			       (0x1f << T_T3_POST_SHIFT) | T_T3_PREP_55NS);
>  	} else {
> -		/* Configure a 4-lane D-PHY using PHY0 and PHY1. */
> -		max96712_write(priv, 0x94a, 0xc0);
> +		/* Configure a 4-lane D-PHY using PHY1. */
> +		max96712_write(priv, MAX96712_MIPI_TX_10(1), 3 << CSI2_LANE_CNT_SHIFT);
>  	}
>  
>  	/* Configure lane mapping for PHY0 and PHY1. */
>  	/* TODO: Add support for lane swapping. */
> -	max96712_write(priv, 0x8a3, 0xe4);
> +	max96712_write(priv, MAX96712_MIPI_PHY_3, 0xe4);
>  
>  	/* Configure lane polarity for PHY0 and PHY1. */
>  	for (i = 0; i < priv->mipi.num_data_lanes + 1; i++)
>  		if (priv->mipi.lane_polarities[i])
>  			phy5 |= BIT(i == 0 ? 5 : i < 3 ? i - 1 : i);
> -	max96712_write(priv, 0x8a5, phy5);
> +	max96712_write(priv, MAX96712_MIPI_PHY_5, phy5);
>  
>  	/* Set link frequency for PHY0 and PHY1. */
> -	max96712_update_bits(priv, 0x415, 0x3f,
> -			     ((priv->info->dpllfreq / 100) & 0x1f) | BIT(5));
> -	max96712_update_bits(priv, 0x418, 0x3f,
> -			     ((priv->info->dpllfreq / 100) & 0x1f) | BIT(5));
> +	max96712_update_bits(priv, MAX96712_BACKTOP0_22,
> +			     PHY_CSI_TX_DPLL_FB_FRACTION_PREDEF_EN |
> +			     PHY_CSI_TX_DPLL_PREDEF_FREQ_MASK,
> +			     PHY_CSI_TX_DPLL_FB_FRACTION_PREDEF_EN |
> +			     ((priv->info->dpllfreq / 100) & 0x1f));
> +	max96712_update_bits(priv, MAX96712_BACKTOP0_25,
> +			     PHY_CSI_TX_DPLL_FB_FRACTION_PREDEF_EN |
> +			     PHY_CSI_TX_DPLL_PREDEF_FREQ_MASK,
> +			     PHY_CSI_TX_DPLL_FB_FRACTION_PREDEF_EN |
> +			     ((priv->info->dpllfreq / 100) & 0x1f));
>  
>  	/* Enable PHY0 and PHY1 */
> -	max96712_update_bits(priv, 0x8a2, 0xf0, 0x30);
> +	max96712_update_bits(priv, MAX96712_MIPI_PHY_2, PHY_STDBY_N_MASK, PHY0_EN | PHY1_EN);
>  }
>  
>  static void max96712_pattern_enable(struct max96712_priv *priv, bool enable)
> @@ -170,51 +287,53 @@ static void max96712_pattern_enable(struct max96712_priv *priv, bool enable)
>  	const u32 v_tot = v_active + v_fp + v_sw + v_bp;
>  
>  	if (!enable) {
> -		max96712_write(priv, 0x1051, 0x00);
> +		max96712_write(priv, MAX96712_VRX_PATGEN_1, 0x00);
>  		return;
>  	}
>  
>  	/* Set PCLK to 75MHz if device have DEBUG_EXTRA register. */
>  	if (priv->info->have_debug_extra)
> -		max96712_write(priv, DEBUG_EXTRA_REG, DEBUG_EXTRA_PCLK_75MHZ);
> +		max96712_write(priv, MAX96712_DEBUG_EXTRA_REG, DEBUG_EXTRA_PCLK_75MHZ);
>  
>  	/* Configure Video Timing Generator for 1920x1080 @ 30 fps. */
> -	max96712_write_bulk_value(priv, 0x1052, 0, 3);
> -	max96712_write_bulk_value(priv, 0x1055, v_sw * h_tot, 3);
> -	max96712_write_bulk_value(priv, 0x1058,
> +	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_VS_DLY, 0, 3);
> +	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_VS_HIGH, v_sw * h_tot, 3);
> +	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_VS_LOW,
>  				  (v_active + v_fp + v_bp) * h_tot, 3);
> -	max96712_write_bulk_value(priv, 0x105b, 0, 3);
> -	max96712_write_bulk_value(priv, 0x105e, h_sw, 2);
> -	max96712_write_bulk_value(priv, 0x1060, h_active + h_fp + h_bp, 2);
> -	max96712_write_bulk_value(priv, 0x1062, v_tot, 2);
> -	max96712_write_bulk_value(priv, 0x1064,
> +	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_V2H, 0, 3);
> +	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_HS_HIGH, h_sw, 2);
> +	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_HS_LOW, h_active + h_fp + h_bp, 2);
> +	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_HS_CNT, v_tot, 2);
> +	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_V2D,
>  				  h_tot * (v_sw + v_bp) + (h_sw + h_bp), 3);
> -	max96712_write_bulk_value(priv, 0x1067, h_active, 2);
> -	max96712_write_bulk_value(priv, 0x1069, h_fp + h_sw + h_bp, 2);
> -	max96712_write_bulk_value(priv, 0x106b, v_active, 2);
> +	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_DE_HIGH, h_active, 2);
> +	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_DE_LOW, h_fp + h_sw + h_bp, 2);
> +	max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_DE_CNT, v_active, 2);
>  
>  	/* Generate VS, HS and DE in free-running mode. */
> -	max96712_write(priv, 0x1050, 0xfb);
> +	max96712_write(priv, MAX96712_VRX_PATGEN_0,
> +		       GEN_VS | GEN_HS | GEN_DE | VS_INV | HS_INV |
> +		       (VTG_MODE_FREE_RUNNING << VTG_MODE_SHIFT));
>  
>  	/* Configure Video Pattern Generator. */
>  	if (priv->pattern == MAX96712_PATTERN_CHECKERBOARD) {
>  		/* Set checkerboard pattern size. */
> -		max96712_write(priv, 0x1074, 0x3c);
> -		max96712_write(priv, 0x1075, 0x3c);
> -		max96712_write(priv, 0x1076, 0x3c);
> +		max96712_write(priv, MAX96712_VRX_PATGEN_CHKR_RPT_A, 0x3c);
> +		max96712_write(priv, MAX96712_VRX_PATGEN_CHKR_RPT_B, 0x3c);
> +		max96712_write(priv, MAX96712_VRX_PATGEN_CHKR_ALT, 0x3c);
>  
>  		/* Set checkerboard pattern colors. */
> -		max96712_write_bulk_value(priv, 0x106e, 0xfecc00, 3);
> -		max96712_write_bulk_value(priv, 0x1071, 0x006aa7, 3);
> +		max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_CHKR_COLOR_A, 0xfecc00, 3);
> +		max96712_write_bulk_value(priv, MAX96712_VRX_PATGEN_CHKR_COLOR_B, 0x006aa7, 3);
>  
>  		/* Generate checkerboard pattern. */
> -		max96712_write(priv, 0x1051, 0x10);
> +		max96712_write(priv, MAX96712_VRX_PATGEN_1, PATGEN_MODE_CHECKERBOARD);
>  	} else {
>  		/* Set gradient increment. */
> -		max96712_write(priv, 0x106d, 0x10);
> +		max96712_write(priv, MAX96712_VRX_PATGEN_GRAD_INCR, 0x10);
>  
>  		/* Generate gradient pattern. */
> -		max96712_write(priv, 0x1051, 0x20);
> +		max96712_write(priv, MAX96712_VRX_PATGEN_1, PATGEN_MODE_GRADIENT);
>  	}
>  }
>  
> -- 
> 2.44.1
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH 0/3] staging: media: max96712: a few, mainly cosmetic, changes

[Niklas Söderlund]

Hello Laurentiu,

On 2024-12-19 17:06:40 +0200, Laurentiu Palcu wrote:
> Hi,
> 
> This is in preparation for bringing our downstream max96724 work into
> the max96712 driver. The changes in this patch-set are mainly cosmetic
> to increase readability, with one exception related to the PCLK debug
> register which appears to exist in MAX96724 as well(see VPG section in
> users guide [1]).
> 
> Unfortunately, I couldn't find any max96712 reference manual anywhere to
> make sure the registers match between the 2 chips, nor I have a max96712
> to test with.

I have tested this series with both MAX96712 and MAX96724 and I can 
capture the test patterns correctly. For the series,

Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> 
> However, since the max96724 compatible was added here, according to the
> commit
> 
> 6c872b782ae ("media: staging: max96712: Add support for MAX96724")
> 
> the chips are "almost identical". So, I used max96724 register and
> field descriptions instead. Someone with a max96712 RM may want to
> double-check though.
> 
> Thanks,
> Laurentiu
> 
> [1] https://www.analog.com/media/en/technical-documentation/user-guides/max96724fr-user-guide.pdf
> 
> Laurentiu Palcu (3):
>   staging: media: max96712: fix typo
>   staging: media: max96712: enable extra debug register for max96724
>   staging: media: max96712: add some register and field definitions
> 
>  drivers/staging/media/max96712/max96712.c | 210 +++++++++++++++++-----
>  1 file changed, 165 insertions(+), 45 deletions(-)
> 
> -- 
> 2.44.1
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH 2/3] staging: media: max96712: enable extra debug register for max96724

[Laurentiu Palcu]

Hi Niklas,

On Thu, Dec 19, 2024 at 06:09:23PM +0100, Niklas Söderlund wrote:
> Hello Laurentiu,
> 
> Thanks for your work.
> 
> On 2024-12-19 17:06:42 +0200, Laurentiu Palcu wrote:
> > The current implementation has the register disabled since it is not
> > documented in the MAX96724 RM. However, in the chip's user's guide [1],
> > in the Video Pattern Generator section, the register is documented and
> > it appears to work just fine. Before the change I was experiencing
> > approx 10fps when streaming, after: 30.3fps.
> > 
> > [1] https://www.analog.com/media/en/technical-documentation/user-guides/max96724fr-user-guide.pdf
> > 
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> > ---
> >  drivers/staging/media/max96712/max96712.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
> > index 5228f9ec60859..682ebd20851f7 100644
> > --- a/drivers/staging/media/max96712/max96712.c
> > +++ b/drivers/staging/media/max96712/max96712.c
> > @@ -462,6 +462,7 @@ static const struct max96712_info max96712_info_max96712 = {
> >  
> >  static const struct max96712_info max96712_info_max96724 = {
> >  	.dpllfreq = 1200,
> > +	.have_debug_extra = true,
> 
> This flag was added as I could not find the register in the MAX96724 
> datasheet I had access to at the time. As you now prove the register 
> exists in the user's guide and that it works I think a better solution 
> is to remove the 'have_debug_extra' flag all together then set it for 
> both supported devices.

Ok, makes sense. I guess I was thinking of having this driver support
other chips as well in the future and that's why I left it there, for
chip variants that don't actually have it. But, on the other hand, it
could always be added later if needed.

I'll send a v2 with the flag removed.

Thanks,
Laurentiu

> 
> >  };
> >  
> >  static const struct of_device_id max96712_of_table[] = {
> > -- 
> > 2.44.1
> > 
> 
> -- 
> Kind Regards,
> Niklas Söderlund

Re: [PATCH 1/3] staging: media: max96712: fix typo

[Dan Carpenter]

On Thu, Dec 19, 2024 at 05:06:41PM +0200, Laurentiu Palcu wrote:
> Fix a typo in VS_LOW expression.
> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> ---
>  drivers/staging/media/max96712/max96712.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
> index 0751b2e048958..5228f9ec60859 100644
> --- a/drivers/staging/media/max96712/max96712.c
> +++ b/drivers/staging/media/max96712/max96712.c
> @@ -182,7 +182,7 @@ static void max96712_pattern_enable(struct max96712_priv *priv, bool enable)
>  	max96712_write_bulk_value(priv, 0x1052, 0, 3);
>  	max96712_write_bulk_value(priv, 0x1055, v_sw * h_tot, 3);
>  	max96712_write_bulk_value(priv, 0x1058,
> -				  (v_active + v_fp + + v_bp) * h_tot, 3);
> +				  (v_active + v_fp + v_bp) * h_tot, 3);

Huh.  I had expected this would be a bug in some way, but it actually
works fine.  #MysteriesOfC

regards,
dan carpenter