[PATCH v2 0/2] media: staging: max96712: Add support for MAX96724

[PATCH v2 0/2] media: staging: max96712: Add support for MAX96724

[Niklas Söderlund]

Hello,

This small series extends the max96712 driver in staging to also support 
the MAX96724 device.

The devices are similar but not identical. As the staging driver only 
supports the video pattern generator the changes in the driver are 
small, but needed, to generate a stable test pattern.

Patch 1/2 extends the bindings with a new compatible for MAX96724, while 
patch 2/2 takes care of updating the driver to support generating a test 
pattern without changing the test pattern clock (which is not supported 
on MAX96724).

These two patches where previously posted separately but have now been 
collected together in a series. See individual patches for changelog.

Niklas Söderlund (2):
  dt-bindings: i2c: maxim,max96712: Add compatible for MAX96724
  media: staging: max96712: Add support for MAX96724

 .../bindings/media/i2c/maxim,max96712.yaml    |  5 +++-
 drivers/staging/media/max96712/max96712.c     | 26 +++++++++++++++----
 2 files changed, 25 insertions(+), 6 deletions(-)

-- 
2.46.0

[PATCH v2 1/2] dt-bindings: i2c: maxim,max96712: Add compatible for MAX96724

[Niklas Söderlund]

The MAX96712 and MAX96724 are almost identical and can be supported by
the same driver, add a compatible for MAX96724.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
* Changes since v1
- Group in series together with driver change.
---
 .../devicetree/bindings/media/i2c/maxim,max96712.yaml        | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
index 6c72e77b927c..26f85151afbd 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
@@ -25,7 +25,10 @@ description: |
 
 properties:
   compatible:
-    const: maxim,max96712
+    items:
+      - enum:
+          - maxim,max96712
+          - maxim,max96724
 
   reg:
     description: I2C device address
-- 
2.46.0

[PATCH v2 2/2] media: staging: max96712: Add support for MAX96724

[Niklas Söderlund]

The MAX96724 is almost identical to the MAX96712 and can be supported by
the same driver, add support for it.

For the staging driver which only supports patter generation the big
difference is that the datasheet (rev 4) for MAX96724 do not describe
the DEBUG_EXTRA register, which is at offset 0x0009 on MAX96712. It's
not clear if this register is removed or moved to a different offset.
What is known is writing to register 0x0009 have no effect on MAX96724.

This makes it impossible to increase the test pattern clock frequency
from 25 MHz to 75Mhz on MAX96724. To be able to get a stable test
pattern the DPLL frequency have to be increase instead to compensate for
this. The frequency selected is found by experimentation as the MAX96724
datasheet is much sparser then what's available for MAX96712.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v1
- Group in series together with binding.
---
 drivers/staging/media/max96712/max96712.c | 26 ++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
index 6bdbccbee05a..6bd02276c413 100644
--- a/drivers/staging/media/max96712/max96712.c
+++ b/drivers/staging/media/max96712/max96712.c
@@ -17,8 +17,10 @@
 #include <media/v4l2-subdev.h>
 
 #define MAX96712_ID 0x20
+#define MAX96724_ID 0xA7
 
 #define MAX96712_DPLL_FREQ 1000
+#define MAX96724_DPLL_FREQ 1200
 
 enum max96712_pattern {
 	MAX96712_PATTERN_CHECKERBOARD = 0,
@@ -31,6 +33,7 @@ struct max96712_priv {
 	struct gpio_desc *gpiod_pwdn;
 
 	bool cphy;
+	bool max96724;
 	struct v4l2_mbus_config_mipi_csi2 mipi;
 
 	struct v4l2_subdev sd;
@@ -120,6 +123,7 @@ static void max96712_mipi_enable(struct max96712_priv *priv, bool enable)
 
 static void max96712_mipi_configure(struct max96712_priv *priv)
 {
+	unsigned int dpll_freq;
 	unsigned int i;
 	u8 phy5 = 0;
 
@@ -152,10 +156,11 @@ static void max96712_mipi_configure(struct max96712_priv *priv)
 	max96712_write(priv, 0x8a5, phy5);
 
 	/* Set link frequency for PHY0 and PHY1. */
+	dpll_freq = priv->max96724 ? MAX96724_DPLL_FREQ : MAX96712_DPLL_FREQ;
 	max96712_update_bits(priv, 0x415, 0x3f,
-			     ((MAX96712_DPLL_FREQ / 100) & 0x1f) | BIT(5));
+			     ((dpll_freq / 100) & 0x1f) | BIT(5));
 	max96712_update_bits(priv, 0x418, 0x3f,
-			     ((MAX96712_DPLL_FREQ / 100) & 0x1f) | BIT(5));
+			     ((dpll_freq / 100) & 0x1f) | BIT(5));
 
 	/* Enable PHY0 and PHY1 */
 	max96712_update_bits(priv, 0x8a2, 0xf0, 0x30);
@@ -181,7 +186,8 @@ static void max96712_pattern_enable(struct max96712_priv *priv, bool enable)
 	}
 
 	/* PCLK 75MHz. */
-	max96712_write(priv, 0x0009, 0x01);
+	if (!priv->max96724)
+		max96712_write(priv, 0x0009, 0x01);
 
 	/* Configure Video Timing Generator for 1920x1080 @ 30 fps. */
 	max96712_write_bulk_value(priv, 0x1052, 0, 3);
@@ -303,6 +309,7 @@ static const struct v4l2_ctrl_ops max96712_ctrl_ops = {
 
 static int max96712_v4l2_register(struct max96712_priv *priv)
 {
+	unsigned int dpll_freq;
 	long pixel_rate;
 	int ret;
 
@@ -317,7 +324,8 @@ static int max96712_v4l2_register(struct max96712_priv *priv)
 	 * TODO: Once V4L2_CID_LINK_FREQ is changed from a menu control to an
 	 * INT64 control it should be used here instead of V4L2_CID_PIXEL_RATE.
 	 */
-	pixel_rate = MAX96712_DPLL_FREQ / priv->mipi.num_data_lanes * 1000000;
+	dpll_freq = priv->max96724 ? MAX96724_DPLL_FREQ : MAX96712_DPLL_FREQ;
+	pixel_rate = dpll_freq / priv->mipi.num_data_lanes * 1000000;
 	v4l2_ctrl_new_std(&priv->ctrl_handler, NULL, V4L2_CID_PIXEL_RATE,
 			  pixel_rate, pixel_rate, 1, pixel_rate);
 
@@ -438,8 +446,15 @@ static int max96712_probe(struct i2c_client *client)
 	if (priv->gpiod_pwdn)
 		usleep_range(4000, 5000);
 
-	if (max96712_read(priv, 0x4a) != MAX96712_ID)
+	switch (max96712_read(priv, 0x4a)) {
+	case MAX96712_ID:
+		break;
+	case MAX96724_ID:
+		priv->max96724 = true;
+		break;
+	default:
 		return -ENODEV;
+	}
 
 	max96712_reset(priv);
 
@@ -463,6 +478,7 @@ static void max96712_remove(struct i2c_client *client)
 
 static const struct of_device_id max96712_of_table[] = {
 	{ .compatible = "maxim,max96712" },
+	{ .compatible = "maxim,max96724" },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, max96712_of_table);
-- 
2.46.0

Re: [PATCH v2 2/2] media: staging: max96712: Add support for MAX96724

[Sakari Ailus]

Hejssan,

Tack för lappan!

On Tue, Aug 27, 2024 at 03:18:41PM +0200, Niklas Söderlund wrote:
> The MAX96724 is almost identical to the MAX96712 and can be supported by
> the same driver, add support for it.
> 
> For the staging driver which only supports patter generation the big
> difference is that the datasheet (rev 4) for MAX96724 do not describe
> the DEBUG_EXTRA register, which is at offset 0x0009 on MAX96712. It's
> not clear if this register is removed or moved to a different offset.
> What is known is writing to register 0x0009 have no effect on MAX96724.
> 
> This makes it impossible to increase the test pattern clock frequency
> from 25 MHz to 75Mhz on MAX96724. To be able to get a stable test
> pattern the DPLL frequency have to be increase instead to compensate for
> this. The frequency selected is found by experimentation as the MAX96724
> datasheet is much sparser then what's available for MAX96712.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v1
> - Group in series together with binding.
> ---
>  drivers/staging/media/max96712/max96712.c | 26 ++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
> index 6bdbccbee05a..6bd02276c413 100644
> --- a/drivers/staging/media/max96712/max96712.c
> +++ b/drivers/staging/media/max96712/max96712.c
> @@ -17,8 +17,10 @@
>  #include <media/v4l2-subdev.h>
>  
>  #define MAX96712_ID 0x20
> +#define MAX96724_ID 0xA7
>  
>  #define MAX96712_DPLL_FREQ 1000
> +#define MAX96724_DPLL_FREQ 1200
>  
>  enum max96712_pattern {
>  	MAX96712_PATTERN_CHECKERBOARD = 0,
> @@ -31,6 +33,7 @@ struct max96712_priv {
>  	struct gpio_desc *gpiod_pwdn;
>  
>  	bool cphy;
> +	bool max96724;
>  	struct v4l2_mbus_config_mipi_csi2 mipi;
>  
>  	struct v4l2_subdev sd;
> @@ -120,6 +123,7 @@ static void max96712_mipi_enable(struct max96712_priv *priv, bool enable)
>  
>  static void max96712_mipi_configure(struct max96712_priv *priv)
>  {
> +	unsigned int dpll_freq;
>  	unsigned int i;
>  	u8 phy5 = 0;
>  
> @@ -152,10 +156,11 @@ static void max96712_mipi_configure(struct max96712_priv *priv)
>  	max96712_write(priv, 0x8a5, phy5);
>  
>  	/* Set link frequency for PHY0 and PHY1. */
> +	dpll_freq = priv->max96724 ? MAX96724_DPLL_FREQ : MAX96712_DPLL_FREQ;
>  	max96712_update_bits(priv, 0x415, 0x3f,
> -			     ((MAX96712_DPLL_FREQ / 100) & 0x1f) | BIT(5));
> +			     ((dpll_freq / 100) & 0x1f) | BIT(5));
>  	max96712_update_bits(priv, 0x418, 0x3f,
> -			     ((MAX96712_DPLL_FREQ / 100) & 0x1f) | BIT(5));
> +			     ((dpll_freq / 100) & 0x1f) | BIT(5));
>  
>  	/* Enable PHY0 and PHY1 */
>  	max96712_update_bits(priv, 0x8a2, 0xf0, 0x30);
> @@ -181,7 +186,8 @@ static void max96712_pattern_enable(struct max96712_priv *priv, bool enable)
>  	}
>  
>  	/* PCLK 75MHz. */
> -	max96712_write(priv, 0x0009, 0x01);
> +	if (!priv->max96724)
> +		max96712_write(priv, 0x0009, 0x01);

It'd be nice to have a macro for this, espeically now that the driver
supports more than one chip.

>  
>  	/* Configure Video Timing Generator for 1920x1080 @ 30 fps. */
>  	max96712_write_bulk_value(priv, 0x1052, 0, 3);
> @@ -303,6 +309,7 @@ static const struct v4l2_ctrl_ops max96712_ctrl_ops = {
>  
>  static int max96712_v4l2_register(struct max96712_priv *priv)
>  {
> +	unsigned int dpll_freq;
>  	long pixel_rate;
>  	int ret;
>  
> @@ -317,7 +324,8 @@ static int max96712_v4l2_register(struct max96712_priv *priv)
>  	 * TODO: Once V4L2_CID_LINK_FREQ is changed from a menu control to an
>  	 * INT64 control it should be used here instead of V4L2_CID_PIXEL_RATE.
>  	 */
> -	pixel_rate = MAX96712_DPLL_FREQ / priv->mipi.num_data_lanes * 1000000;
> +	dpll_freq = priv->max96724 ? MAX96724_DPLL_FREQ : MAX96712_DPLL_FREQ;
> +	pixel_rate = dpll_freq / priv->mipi.num_data_lanes * 1000000;
>  	v4l2_ctrl_new_std(&priv->ctrl_handler, NULL, V4L2_CID_PIXEL_RATE,
>  			  pixel_rate, pixel_rate, 1, pixel_rate);
>  
> @@ -438,8 +446,15 @@ static int max96712_probe(struct i2c_client *client)
>  	if (priv->gpiod_pwdn)
>  		usleep_range(4000, 5000);
>  
> -	if (max96712_read(priv, 0x4a) != MAX96712_ID)
> +	switch (max96712_read(priv, 0x4a)) {
> +	case MAX96712_ID:
> +		break;
> +	case MAX96724_ID:
> +		priv->max96724 = true;
> +		break;
> +	default:
>  		return -ENODEV;
> +	}
>  
>  	max96712_reset(priv);
>  
> @@ -463,6 +478,7 @@ static void max96712_remove(struct i2c_client *client)
>  
>  static const struct of_device_id max96712_of_table[] = {
>  	{ .compatible = "maxim,max96712" },
> +	{ .compatible = "maxim,max96724" },

How about adding compatible specific data to convey the model, instead of a
switch? See e.g. drivers/media/i2c/ccs/ccs-core.c for an example.

You could store the DPLL frequency there, too.

>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, max96712_of_table);
> 

-- 
Trevliga hälsningar,

Sakari Ailus

Re: [PATCH v2 2/2] media: staging: max96712: Add support for MAX96724

[Tommaso Merciai]

Hi Niklas,
Thanks for your work.

Hi Julien,
I think we can adopt a similar approach for the max96716 deserializer using your work
on max96714 driver. What do you think?

Thanks in advance.

On Tue, Aug 27, 2024 at 03:18:41PM +0200, Niklas Söderlund wrote:
> The MAX96724 is almost identical to the MAX96712 and can be supported by
> the same driver, add support for it.
> 
> For the staging driver which only supports patter generation the big
> difference is that the datasheet (rev 4) for MAX96724 do not describe
> the DEBUG_EXTRA register, which is at offset 0x0009 on MAX96712. It's
> not clear if this register is removed or moved to a different offset.
> What is known is writing to register 0x0009 have no effect on MAX96724.
> 
> This makes it impossible to increase the test pattern clock frequency
> from 25 MHz to 75Mhz on MAX96724. To be able to get a stable test
> pattern the DPLL frequency have to be increase instead to compensate for
> this. The frequency selected is found by experimentation as the MAX96724
> datasheet is much sparser then what's available for MAX96712.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v1
> - Group in series together with binding.
> ---
>  drivers/staging/media/max96712/max96712.c | 26 ++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
> index 6bdbccbee05a..6bd02276c413 100644
> --- a/drivers/staging/media/max96712/max96712.c
> +++ b/drivers/staging/media/max96712/max96712.c
> @@ -17,8 +17,10 @@
>  #include <media/v4l2-subdev.h>
>  
>  #define MAX96712_ID 0x20
> +#define MAX96724_ID 0xA7
>  
>  #define MAX96712_DPLL_FREQ 1000
> +#define MAX96724_DPLL_FREQ 1200
>  
>  enum max96712_pattern {
>  	MAX96712_PATTERN_CHECKERBOARD = 0,
> @@ -31,6 +33,7 @@ struct max96712_priv {
>  	struct gpio_desc *gpiod_pwdn;
>  
>  	bool cphy;
> +	bool max96724;
>  	struct v4l2_mbus_config_mipi_csi2 mipi;
>  
>  	struct v4l2_subdev sd;
> @@ -120,6 +123,7 @@ static void max96712_mipi_enable(struct max96712_priv *priv, bool enable)
>  
>  static void max96712_mipi_configure(struct max96712_priv *priv)
>  {
> +	unsigned int dpll_freq;
>  	unsigned int i;
>  	u8 phy5 = 0;
>  
> @@ -152,10 +156,11 @@ static void max96712_mipi_configure(struct max96712_priv *priv)
>  	max96712_write(priv, 0x8a5, phy5);
>  
>  	/* Set link frequency for PHY0 and PHY1. */
> +	dpll_freq = priv->max96724 ? MAX96724_DPLL_FREQ : MAX96712_DPLL_FREQ;
>  	max96712_update_bits(priv, 0x415, 0x3f,
> -			     ((MAX96712_DPLL_FREQ / 100) & 0x1f) | BIT(5));
> +			     ((dpll_freq / 100) & 0x1f) | BIT(5));
>  	max96712_update_bits(priv, 0x418, 0x3f,
> -			     ((MAX96712_DPLL_FREQ / 100) & 0x1f) | BIT(5));
> +			     ((dpll_freq / 100) & 0x1f) | BIT(5));
>  
>  	/* Enable PHY0 and PHY1 */
>  	max96712_update_bits(priv, 0x8a2, 0xf0, 0x30);
> @@ -181,7 +186,8 @@ static void max96712_pattern_enable(struct max96712_priv *priv, bool enable)
>  	}
>  
>  	/* PCLK 75MHz. */
> -	max96712_write(priv, 0x0009, 0x01);
> +	if (!priv->max96724)
> +		max96712_write(priv, 0x0009, 0x01);
>  
>  	/* Configure Video Timing Generator for 1920x1080 @ 30 fps. */
>  	max96712_write_bulk_value(priv, 0x1052, 0, 3);
> @@ -303,6 +309,7 @@ static const struct v4l2_ctrl_ops max96712_ctrl_ops = {
>  
>  static int max96712_v4l2_register(struct max96712_priv *priv)
>  {
> +	unsigned int dpll_freq;
>  	long pixel_rate;
>  	int ret;
>  
> @@ -317,7 +324,8 @@ static int max96712_v4l2_register(struct max96712_priv *priv)
>  	 * TODO: Once V4L2_CID_LINK_FREQ is changed from a menu control to an
>  	 * INT64 control it should be used here instead of V4L2_CID_PIXEL_RATE.
>  	 */
> -	pixel_rate = MAX96712_DPLL_FREQ / priv->mipi.num_data_lanes * 1000000;
> +	dpll_freq = priv->max96724 ? MAX96724_DPLL_FREQ : MAX96712_DPLL_FREQ;
> +	pixel_rate = dpll_freq / priv->mipi.num_data_lanes * 1000000;
>  	v4l2_ctrl_new_std(&priv->ctrl_handler, NULL, V4L2_CID_PIXEL_RATE,
>  			  pixel_rate, pixel_rate, 1, pixel_rate);
>  
> @@ -438,8 +446,15 @@ static int max96712_probe(struct i2c_client *client)
>  	if (priv->gpiod_pwdn)
>  		usleep_range(4000, 5000);
>  
> -	if (max96712_read(priv, 0x4a) != MAX96712_ID)
> +	switch (max96712_read(priv, 0x4a)) {
> +	case MAX96712_ID:
> +		break;
> +	case MAX96724_ID:
> +		priv->max96724 = true;
> +		break;
> +	default:
>  		return -ENODEV;
> +	}
>  
>  	max96712_reset(priv);
>  
> @@ -463,6 +478,7 @@ static void max96712_remove(struct i2c_client *client)
>  
>  static const struct of_device_id max96712_of_table[] = {
>  	{ .compatible = "maxim,max96712" },
> +	{ .compatible = "maxim,max96724" },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, max96712_of_table);

Thanks & Regards,
Tommaso

> -- 
> 2.46.0
> 
> 

Re: [PATCH v2 1/2] dt-bindings: i2c: maxim,max96712: Add compatible for MAX96724

[Krzysztof Kozlowski]

On 27/08/2024 15:18, Niklas Söderlund wrote:
> The MAX96712 and MAX96724 are almost identical and can be supported by
> the same driver, add a compatible for MAX96724.

The driver statement in this context is meaningless. You did not make
them compatible so what does it matter?

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> * Changes since v1
> - Group in series together with driver change.
> ---
>  .../devicetree/bindings/media/i2c/maxim,max96712.yaml        | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
> index 6c72e77b927c..26f85151afbd 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
> @@ -25,7 +25,10 @@ description: |
>  
>  properties:
>    compatible:
> -    const: maxim,max96712
> +    items:
> +      - enum:
> +          - maxim,max96712
> +          - maxim,max96724

Driver change tells these are compatible and version is detectable.
Express it in the binding with fallback or explain in commit msg why
they are not compatible.


Best regards,
Krzysztof

Re: [PATCH v2 2/2] media: staging: max96712: Add support for MAX96724

[Niklas Söderlund]

Hej Sakari,

Tack för feedback.

On 2024-08-27 13:32:43 +0000, Sakari Ailus wrote:
> Hejssan,
> 
> Tack för lappan!
> 
> On Tue, Aug 27, 2024 at 03:18:41PM +0200, Niklas Söderlund wrote:
> > The MAX96724 is almost identical to the MAX96712 and can be supported by
> > the same driver, add support for it.
> > 
> > For the staging driver which only supports patter generation the big
> > difference is that the datasheet (rev 4) for MAX96724 do not describe
> > the DEBUG_EXTRA register, which is at offset 0x0009 on MAX96712. It's
> > not clear if this register is removed or moved to a different offset.
> > What is known is writing to register 0x0009 have no effect on MAX96724.
> > 
> > This makes it impossible to increase the test pattern clock frequency
> > from 25 MHz to 75Mhz on MAX96724. To be able to get a stable test
> > pattern the DPLL frequency have to be increase instead to compensate for
> > this. The frequency selected is found by experimentation as the MAX96724
> > datasheet is much sparser then what's available for MAX96712.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> > * Changes since v1
> > - Group in series together with binding.
> > ---
> >  drivers/staging/media/max96712/max96712.c | 26 ++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
> > index 6bdbccbee05a..6bd02276c413 100644
> > --- a/drivers/staging/media/max96712/max96712.c
> > +++ b/drivers/staging/media/max96712/max96712.c
> > @@ -17,8 +17,10 @@
> >  #include <media/v4l2-subdev.h>
> >  
> >  #define MAX96712_ID 0x20
> > +#define MAX96724_ID 0xA7
> >  
> >  #define MAX96712_DPLL_FREQ 1000
> > +#define MAX96724_DPLL_FREQ 1200
> >  
> >  enum max96712_pattern {
> >  	MAX96712_PATTERN_CHECKERBOARD = 0,
> > @@ -31,6 +33,7 @@ struct max96712_priv {
> >  	struct gpio_desc *gpiod_pwdn;
> >  
> >  	bool cphy;
> > +	bool max96724;
> >  	struct v4l2_mbus_config_mipi_csi2 mipi;
> >  
> >  	struct v4l2_subdev sd;
> > @@ -120,6 +123,7 @@ static void max96712_mipi_enable(struct max96712_priv *priv, bool enable)
> >  
> >  static void max96712_mipi_configure(struct max96712_priv *priv)
> >  {
> > +	unsigned int dpll_freq;
> >  	unsigned int i;
> >  	u8 phy5 = 0;
> >  
> > @@ -152,10 +156,11 @@ static void max96712_mipi_configure(struct max96712_priv *priv)
> >  	max96712_write(priv, 0x8a5, phy5);
> >  
> >  	/* Set link frequency for PHY0 and PHY1. */
> > +	dpll_freq = priv->max96724 ? MAX96724_DPLL_FREQ : MAX96712_DPLL_FREQ;
> >  	max96712_update_bits(priv, 0x415, 0x3f,
> > -			     ((MAX96712_DPLL_FREQ / 100) & 0x1f) | BIT(5));
> > +			     ((dpll_freq / 100) & 0x1f) | BIT(5));
> >  	max96712_update_bits(priv, 0x418, 0x3f,
> > -			     ((MAX96712_DPLL_FREQ / 100) & 0x1f) | BIT(5));
> > +			     ((dpll_freq / 100) & 0x1f) | BIT(5));
> >  
> >  	/* Enable PHY0 and PHY1 */
> >  	max96712_update_bits(priv, 0x8a2, 0xf0, 0x30);
> > @@ -181,7 +186,8 @@ static void max96712_pattern_enable(struct max96712_priv *priv, bool enable)
> >  	}
> >  
> >  	/* PCLK 75MHz. */
> > -	max96712_write(priv, 0x0009, 0x01);
> > +	if (!priv->max96724)
> > +		max96712_write(priv, 0x0009, 0x01);
> 
> It'd be nice to have a macro for this, espeically now that the driver
> supports more than one chip.

What do you mean by macro? To test for priv->max96724, a define for the 
register name or something else?

> 
> >  
> >  	/* Configure Video Timing Generator for 1920x1080 @ 30 fps. */
> >  	max96712_write_bulk_value(priv, 0x1052, 0, 3);
> > @@ -303,6 +309,7 @@ static const struct v4l2_ctrl_ops max96712_ctrl_ops = {
> >  
> >  static int max96712_v4l2_register(struct max96712_priv *priv)
> >  {
> > +	unsigned int dpll_freq;
> >  	long pixel_rate;
> >  	int ret;
> >  
> > @@ -317,7 +324,8 @@ static int max96712_v4l2_register(struct max96712_priv *priv)
> >  	 * TODO: Once V4L2_CID_LINK_FREQ is changed from a menu control to an
> >  	 * INT64 control it should be used here instead of V4L2_CID_PIXEL_RATE.
> >  	 */
> > -	pixel_rate = MAX96712_DPLL_FREQ / priv->mipi.num_data_lanes * 1000000;
> > +	dpll_freq = priv->max96724 ? MAX96724_DPLL_FREQ : MAX96712_DPLL_FREQ;
> > +	pixel_rate = dpll_freq / priv->mipi.num_data_lanes * 1000000;
> >  	v4l2_ctrl_new_std(&priv->ctrl_handler, NULL, V4L2_CID_PIXEL_RATE,
> >  			  pixel_rate, pixel_rate, 1, pixel_rate);
> >  
> > @@ -438,8 +446,15 @@ static int max96712_probe(struct i2c_client *client)
> >  	if (priv->gpiod_pwdn)
> >  		usleep_range(4000, 5000);
> >  
> > -	if (max96712_read(priv, 0x4a) != MAX96712_ID)
> > +	switch (max96712_read(priv, 0x4a)) {
> > +	case MAX96712_ID:
> > +		break;
> > +	case MAX96724_ID:
> > +		priv->max96724 = true;
> > +		break;
> > +	default:
> >  		return -ENODEV;
> > +	}
> >  
> >  	max96712_reset(priv);
> >  
> > @@ -463,6 +478,7 @@ static void max96712_remove(struct i2c_client *client)
> >  
> >  static const struct of_device_id max96712_of_table[] = {
> >  	{ .compatible = "maxim,max96712" },
> > +	{ .compatible = "maxim,max96724" },
> 
> How about adding compatible specific data to convey the model, instead of a
> switch? See e.g. drivers/media/i2c/ccs/ccs-core.c for an example.
> 
> You could store the DPLL frequency there, too.

Good idea, will do so in next version.

> 
> >  	{ /* sentinel */ },
> >  };
> >  MODULE_DEVICE_TABLE(of, max96712_of_table);
> > 
> 
> -- 
> Trevliga hälsningar,
> 
> Sakari Ailus

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2 1/2] dt-bindings: i2c: maxim,max96712: Add compatible for MAX96724

[Niklas Söderlund]

Hi Krzysztof,

Thanks for your feedback.

On 2024-08-27 16:53:20 +0200, Krzysztof Kozlowski wrote:
> On 27/08/2024 15:18, Niklas Söderlund wrote:
> > The MAX96712 and MAX96724 are almost identical and can be supported by
> > the same driver, add a compatible for MAX96724.
> 
> The driver statement in this context is meaningless. You did not make
> them compatible so what does it matter?

Agreed, this commit message can be improved.

> 
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> > * Changes since v1
> > - Group in series together with driver change.
> > ---
> >  .../devicetree/bindings/media/i2c/maxim,max96712.yaml        | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
> > index 6c72e77b927c..26f85151afbd 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
> > @@ -25,7 +25,10 @@ description: |
> >  
> >  properties:
> >    compatible:
> > -    const: maxim,max96712
> > +    items:
> > +      - enum:
> > +          - maxim,max96712
> > +          - maxim,max96724
> 
> Driver change tells these are compatible and version is detectable.
> Express it in the binding with fallback or explain in commit msg why
> they are not compatible.

It is, and as you point out the commit message can be improved. It 
should have made it clear that what is supported by the staging driver 
(test pattern generator) is similar for the two devices, the full device 
have larger differences.

Before this driver can move out of staging the full GMSL side of things 
needs to be added and there they differ more and will need different 
bindings as one device have more PHYs then the other for example. Run 
time detecting is not enough to be able to verify that in DTS files.

I will fix this for the next version.

> 
> 
> Best regards,
> Krzysztof
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2 2/2] media: staging: max96712: Add support for MAX96724

[Dan Carpenter]

On Tue, Aug 27, 2024 at 03:18:41PM +0200, Niklas Söderlund wrote:
> @@ -181,7 +186,8 @@ static void max96712_pattern_enable(struct max96712_priv *priv, bool enable)
>  	}
>  
>  	/* PCLK 75MHz. */
> -	max96712_write(priv, 0x0009, 0x01);
> +	if (!priv->max96724)
> +		max96712_write(priv, 0x0009, 0x01);
>  

I don't like this either.  The comment should move.  I didn't see the ! the
first couple times I read this.  I don't like the MAX96712_ID and
MAX96724_ID defines because when humans read they only see the general shape of
the words.

https://www.dictionary.com/e/typoglycemia/
https://en.wikipedia.org/wiki/Hamming_distance

I guess the best we can do is remove the _ID so at least the last characters are
different.

It should be something like:

	if (priv->id == MAX96712) {
		/* PCLK 75MHz. */
		max96712_write(priv, 0x0009, 0x01);
	}

Or maybe put in function.

regards,
dan carpenter

Re: [PATCH v2 2/2] media: staging: max96712: Add support for MAX96724

[Sakari Ailus]

Hejssan!

On Tue, Aug 27, 2024 at 07:57:35PM +0200, Niklas Söderlund wrote:
> Hej Sakari,
> 
> Tack för feedback.

Var så god!

> > > @@ -181,7 +186,8 @@ static void max96712_pattern_enable(struct max96712_priv *priv, bool enable)
> > >  	}
> > >  
> > >  	/* PCLK 75MHz. */
> > > -	max96712_write(priv, 0x0009, 0x01);
> > > +	if (!priv->max96724)
> > > +		max96712_write(priv, 0x0009, 0x01);
> > 
> > It'd be nice to have a macro for this, espeically now that the driver
> > supports more than one chip.
> 
> What do you mean by macro? To test for priv->max96724, a define for the 
> register name or something else?

Ah, I meant 0x0009 and preferrably 0x01 as well.

-- 
Hälsningar,

Sakari Ailus

Re: [PATCH v2 1/2] dt-bindings: i2c: maxim,max96712: Add compatible for MAX96724

[Krzysztof Kozlowski]

On 27/08/2024 20:03, Niklas Söderlund wrote:
> Hi Krzysztof,
> 
> Thanks for your feedback.
> 
> On 2024-08-27 16:53:20 +0200, Krzysztof Kozlowski wrote:
>> On 27/08/2024 15:18, Niklas Söderlund wrote:
>>> The MAX96712 and MAX96724 are almost identical and can be supported by
>>> the same driver, add a compatible for MAX96724.
>>
>> The driver statement in this context is meaningless. You did not make
>> them compatible so what does it matter?
> 
> Agreed, this commit message can be improved.
> 
>>
>>>
>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>>> ---
>>> * Changes since v1
>>> - Group in series together with driver change.
>>> ---
>>>  .../devicetree/bindings/media/i2c/maxim,max96712.yaml        | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
>>> index 6c72e77b927c..26f85151afbd 100644
>>> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
>>> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96712.yaml
>>> @@ -25,7 +25,10 @@ description: |
>>>  
>>>  properties:
>>>    compatible:
>>> -    const: maxim,max96712
>>> +    items:
>>> +      - enum:
>>> +          - maxim,max96712
>>> +          - maxim,max96724
>>
>> Driver change tells these are compatible and version is detectable.
>> Express it in the binding with fallback or explain in commit msg why
>> they are not compatible.
> 
> It is, and as you point out the commit message can be improved. It 
> should have made it clear that what is supported by the staging driver 
> (test pattern generator) is similar for the two devices, the full device 
> have larger differences.

OK.



Best regards,
Krzysztof

RE: [PATCH v2 2/2] media: staging: max96712: Add support for MAX96724

[Biju Das]

Hi Niklas,

Thanks for the patch

> -----Original Message-----
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Sent: Tuesday, August 27, 2024 2:19 PM
> Subject: [PATCH v2 2/2] media: staging: max96712: Add support for MAX96724
> 
> The MAX96724 is almost identical to the MAX96712 and can be supported by the same driver, add support
> for it.
> 
> For the staging driver which only supports patter generation the big difference is that the datasheet
> (rev 4) for MAX96724 do not describe the DEBUG_EXTRA register, which is at offset 0x0009 on MAX96712.
> It's not clear if this register is removed or moved to a different offset.
> What is known is writing to register 0x0009 have no effect on MAX96724.
> 
> This makes it impossible to increase the test pattern clock frequency from 25 MHz to 75Mhz on
> MAX96724. To be able to get a stable test pattern the DPLL frequency have to be increase instead to
> compensate for this. The frequency selected is found by experimentation as the MAX96724 datasheet is
> much sparser then what's available for MAX96712.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v1
> - Group in series together with binding.
> ---
>  drivers/staging/media/max96712/max96712.c | 26 ++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
> index 6bdbccbee05a..6bd02276c413 100644
> --- a/drivers/staging/media/max96712/max96712.c
> +++ b/drivers/staging/media/max96712/max96712.c
> @@ -17,8 +17,10 @@
>  #include <media/v4l2-subdev.h>
> 
>  #define MAX96712_ID 0x20
> +#define MAX96724_ID 0xA7

This should also go in device data, for device differences.

> 
>  #define MAX96712_DPLL_FREQ 1000
> +#define MAX96724_DPLL_FREQ 1200

This should also go in device data, for device differences.

> 
>  enum max96712_pattern {
>  	MAX96712_PATTERN_CHECKERBOARD = 0,
> @@ -31,6 +33,7 @@ struct max96712_priv {
>  	struct gpio_desc *gpiod_pwdn;
> 
>  	bool cphy;
> +	bool max96724;

You should drop this and add a single bit capability device variable in device data.

>  	struct v4l2_mbus_config_mipi_csi2 mipi;
> 
>  	struct v4l2_subdev sd;
> @@ -120,6 +123,7 @@ static void max96712_mipi_enable(struct max96712_priv *priv, bool enable)
> 
>  static void max96712_mipi_configure(struct max96712_priv *priv)  {
> +	unsigned int dpll_freq;
>  	unsigned int i;
>  	u8 phy5 = 0;
> 
> @@ -152,10 +156,11 @@ static void max96712_mipi_configure(struct max96712_priv *priv)
>  	max96712_write(priv, 0x8a5, phy5);
> 
>  	/* Set link frequency for PHY0 and PHY1. */
> +	dpll_freq = priv->max96724 ? MAX96724_DPLL_FREQ : MAX96712_DPLL_FREQ;

This should be device data assignment.

>  	max96712_update_bits(priv, 0x415, 0x3f,
> -			     ((MAX96712_DPLL_FREQ / 100) & 0x1f) | BIT(5));
> +			     ((dpll_freq / 100) & 0x1f) | BIT(5));
>  	max96712_update_bits(priv, 0x418, 0x3f,
> -			     ((MAX96712_DPLL_FREQ / 100) & 0x1f) | BIT(5));
> +			     ((dpll_freq / 100) & 0x1f) | BIT(5));
> 
>  	/* Enable PHY0 and PHY1 */
>  	max96712_update_bits(priv, 0x8a2, 0xf0, 0x30); @@ -181,7 +186,8 @@ static void
> max96712_pattern_enable(struct max96712_priv *priv, bool enable)
>  	}
> 
>  	/* PCLK 75MHz. */
> -	max96712_write(priv, 0x0009, 0x01);
> +	if (!priv->max96724)

This should be device capability variable check.

> +		max96712_write(priv, 0x0009, 0x01);
> 
>  	/* Configure Video Timing Generator for 1920x1080 @ 30 fps. */
>  	max96712_write_bulk_value(priv, 0x1052, 0, 3); @@ -303,6 +309,7 @@ static const struct
> v4l2_ctrl_ops max96712_ctrl_ops = {
> 
>  static int max96712_v4l2_register(struct max96712_priv *priv)  {
> +	unsigned int dpll_freq;
>  	long pixel_rate;
>  	int ret;
> 
> @@ -317,7 +324,8 @@ static int max96712_v4l2_register(struct max96712_priv *priv)
>  	 * TODO: Once V4L2_CID_LINK_FREQ is changed from a menu control to an
>  	 * INT64 control it should be used here instead of V4L2_CID_PIXEL_RATE.
>  	 */
> -	pixel_rate = MAX96712_DPLL_FREQ / priv->mipi.num_data_lanes * 1000000;
> +	dpll_freq = priv->max96724 ? MAX96724_DPLL_FREQ : MAX96712_DPLL_FREQ;
> +	pixel_rate = dpll_freq / priv->mipi.num_data_lanes * 1000000;
>  	v4l2_ctrl_new_std(&priv->ctrl_handler, NULL, V4L2_CID_PIXEL_RATE,
>  			  pixel_rate, pixel_rate, 1, pixel_rate);
> 
> @@ -438,8 +446,15 @@ static int max96712_probe(struct i2c_client *client)
>  	if (priv->gpiod_pwdn)
>  		usleep_range(4000, 5000);
> 
> -	if (max96712_read(priv, 0x4a) != MAX96712_ID)

ID got from device_match_data should be compared with this ID and
If there is a mismatch, you should throw error.

> +	switch (max96712_read(priv, 0x4a)) {
> +	case MAX96712_ID:
> +		break;
> +	case MAX96724_ID:
> +		priv->max96724 = true;
> +		break;
> +	default:
>  		return -ENODEV;
> +	}
> 
>  	max96712_reset(priv);
> 
> @@ -463,6 +478,7 @@ static void max96712_remove(struct i2c_client *client)
> 
>  static const struct of_device_id max96712_of_table[] = {
>  	{ .compatible = "maxim,max96712" },
> +	{ .compatible = "maxim,max96724" },
>  	{ /* sentinel */ },

Drop comma in separate patch.


Cheers,
Biju

>  };
>  MODULE_DEVICE_TABLE(of, max96712_of_table);
> --
> 2.46.0
> 

Re: [PATCH v2 2/2] media: staging: max96712: Add support for MAX96724

[Julien Massot]

Hi Niklas,

On 8/27/24 3:18 PM, Niklas Söderlund wrote:
> The MAX96724 is almost identical to the MAX96712 and can be supported by
> the same driver, add support for it.
> 
> For the staging driver which only supports patter generation the big
> difference is that the datasheet (rev 4) for MAX96724 do not describe
> the DEBUG_EXTRA register, which is at offset 0x0009 on MAX96712. It's
> not clear if this register is removed or moved to a different offset.
> What is known is writing to register 0x0009 have no effect on MAX96724.
> 
> This makes it impossible to increase the test pattern clock frequency
> from 25 MHz to 75Mhz on MAX96724. To be able to get a stable test
> pattern the DPLL frequency have to be increase instead to compensate for
> this. The frequency selected is found by experimentation as the MAX96724
> datasheet is much sparser then what's available for MAX96712.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v1
> - Group in series together with binding.
> ---
>   drivers/staging/media/max96712/max96712.c | 26 ++++++++++++++++++-----
>   1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
> index 6bdbccbee05a..6bd02276c413 100644
> --- a/drivers/staging/media/max96712/max96712.c
> +++ b/drivers/staging/media/max96712/max96712.c
> @@ -17,8 +17,10 @@
>   #include <media/v4l2-subdev.h>
>   
>   #define MAX96712_ID 0x20
> +#define MAX96724_ID 0xA7
I sent some comments on the first serie about the device revision register.

https://patchwork.kernel.org/project/linux-media/patch/20240527133410.1690169-1-niklas.soderlund+renesas@ragnatech.se/

Can you fix them in your next patch version ?

Regards,
-- 
Julien

Re: [PATCH v2 2/2] media: staging: max96712: Add support for MAX96724

[Niklas Söderlund]

Hi Julien,

On 2024-08-28 11:41:51 +0200, Julien Massot wrote:
> Hi Niklas,
> 
> On 8/27/24 3:18 PM, Niklas Söderlund wrote:
> > The MAX96724 is almost identical to the MAX96712 and can be supported by
> > the same driver, add support for it.
> > 
> > For the staging driver which only supports patter generation the big
> > difference is that the datasheet (rev 4) for MAX96724 do not describe
> > the DEBUG_EXTRA register, which is at offset 0x0009 on MAX96712. It's
> > not clear if this register is removed or moved to a different offset.
> > What is known is writing to register 0x0009 have no effect on MAX96724.
> > 
> > This makes it impossible to increase the test pattern clock frequency
> > from 25 MHz to 75Mhz on MAX96724. To be able to get a stable test
> > pattern the DPLL frequency have to be increase instead to compensate for
> > this. The frequency selected is found by experimentation as the MAX96724
> > datasheet is much sparser then what's available for MAX96712.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> > * Changes since v1
> > - Group in series together with binding.
> > ---
> >   drivers/staging/media/max96712/max96712.c | 26 ++++++++++++++++++-----
> >   1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/media/max96712/max96712.c b/drivers/staging/media/max96712/max96712.c
> > index 6bdbccbee05a..6bd02276c413 100644
> > --- a/drivers/staging/media/max96712/max96712.c
> > +++ b/drivers/staging/media/max96712/max96712.c
> > @@ -17,8 +17,10 @@
> >   #include <media/v4l2-subdev.h>
> >   #define MAX96712_ID 0x20
> > +#define MAX96724_ID 0xA7
> I sent some comments on the first serie about the device revision register.
> 
> https://patchwork.kernel.org/project/linux-media/patch/20240527133410.1690169-1-niklas.soderlund+renesas@ragnatech.se/
> 
> Can you fix them in your next patch version ?

Thanks for pointing that out. I missed them as they where in a reply 
where we where discussing the Debug Extra register, my bad will fix.

> 
> Regards,
> -- 
> Julien

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2 2/2] media: staging: max96712: Add support for MAX96724

[Julien Massot]

Hi Tommaso,

On 8/27/24 3:55 PM, Tommaso Merciai wrote:
> Hi Niklas,
> Thanks for your work.
> 
> Hi Julien,
> I think we can adopt a similar approach for the max96716 deserializer using your work
> on max96714 driver. What do you think?
> 
> Thanks in advance.


I don't think that supporting MAX96716 and MAX96714 in the same driver 
is the best option

MAX96714 is a very simple device with one input one output one video pipe.

While the MAX96716 is a two inputs/outputs with four different pipes.

IMHO we should have one driver for the 1 port GMSL2 devices, another 
driver for the dual deserializers which will introduce more complex 
routing, and another one for the quad deserializers since the register 
layout is too much different.

But that's only my opinion let's see when we will implement the dual 
deserializer support.

We can of course share some functions for those drivers like GPIO 
handling or pattern generation.

Regards,
Julien

Re: [PATCH v2 2/2] media: staging: max96712: Add support for MAX96724

[Tommaso Merciai]

On Wed, Aug 28, 2024 at 12:02:24PM +0200, Julien Massot wrote:

Hi Julien,

> Hi Tommaso,
> 
> On 8/27/24 3:55 PM, Tommaso Merciai wrote:
> > Hi Niklas,
> > Thanks for your work.
> > 
> > Hi Julien,
> > I think we can adopt a similar approach for the max96716 deserializer using your work
> > on max96714 driver. What do you think?
> > 
> > Thanks in advance.
> 
> 
> I don't think that supporting MAX96716 and MAX96714 in the same driver is
> the best option
> 
> MAX96714 is a very simple device with one input one output one video pipe.
> 
> While the MAX96716 is a two inputs/outputs with four different pipes.
> 
> IMHO we should have one driver for the 1 port GMSL2 devices, another driver
> for the dual deserializers which will introduce more complex routing, and
> another one for the quad deserializers since the register layout is too much
> different.
> 
> But that's only my opinion let's see when we will implement the dual
> deserializer support.
> 
> We can of course share some functions for those drivers like GPIO handling
> or pattern generation.

Understood. Thanks for sharing your idea. :)

Thanks & Regards,
Tommaso

> 
> Regards,
> Julien
> 
>