[PATCH v2 0/2] media: gc2145: add basic dvp bus support

[PATCH v2 0/2] media: gc2145: add basic dvp bus support

[Andrey Skvortsov]

Tested on PinePhone with libcamera-based GNOME screenshot.

Andrey Skvortsov (2):
  dt-bindings: media: i2c: add galaxycore,gc2145 DVP bus support
  media: gc2145: implement basic dvp bus support

v2:
 - bindings:
   - add required bus-type property
   - conditionally require link-frequency property based on bus-type
   - add DVP properties with their default values

 - driver:
  - fix fwnode parsing
  - remove gc2145_is_csi2
  - fix error message for unsupported bus-type

 .../bindings/media/i2c/galaxycore,gc2145.yaml |  64 ++++++++-
 drivers/media/i2c/gc2145.c                    | 122 ++++++++++++++----
 2 files changed, 159 insertions(+), 27 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] dt-bindings: media: i2c: add galaxycore,gc2145 DVP bus support

[Andrey Skvortsov]

Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/media/i2c/galaxycore,gc2145.yaml | 65 ++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
index 1726ecca4c77..3ca5bb94502d 100644
--- a/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
@@ -61,8 +61,38 @@ properties:
         properties:
           link-frequencies: true
 
+          bus-type:
+            enum:
+              - 4 # CSI-2 D-PHY
+              - 5 # Parallel
+            default: 4
+
+          bus-width:
+            const: 8
+
+          hsync-active:
+            enum: [0, 1]
+            default: 1
+
+          vsync-active:
+            enum: [0, 1]
+            default: 1
+
+          pclk-sample:
+            enum: [0, 1]
+            default: 1
+
         required:
-          - link-frequencies
+          - bus-type
+
+        allOf:
+          - if:
+              properties:
+                bus-type:
+                  const: 4
+            then:
+              required:
+                - link-frequencies
 
     required:
       - endpoint
@@ -84,6 +114,7 @@ additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/media/video-interfaces.h>
     #include <dt-bindings/gpio/gpio.h>
 
     i2c {
@@ -103,6 +134,7 @@ examples:
             port {
                 endpoint {
                     remote-endpoint = <&mipid02_0>;
+                    bus-type = <MEDIA_BUS_TYPE_CSI2_DPHY>;
                     data-lanes = <1 2>;
                     link-frequencies = /bits/ 64 <120000000 192000000 240000000>;
                 };
@@ -110,4 +142,35 @@ examples:
         };
     };
 
+  - |
+    #include <dt-bindings/media/video-interfaces.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        camera@3c {
+            compatible = "galaxycore,gc2145";
+            reg = <0x3c>;
+            clocks = <&clk_ext_camera>;
+            iovdd-supply = <&scmi_v3v3_sw>;
+            avdd-supply = <&scmi_v3v3_sw>;
+            dvdd-supply = <&scmi_v3v3_sw>;
+            powerdown-gpios = <&mcp23017 3 (GPIO_ACTIVE_LOW | GPIO_PUSH_PULL)>;
+            reset-gpios = <&mcp23017 4 (GPIO_ACTIVE_LOW | GPIO_PUSH_PULL)>;
+
+            port {
+                endpoint {
+                    remote-endpoint = <&parallel_from_gc2145>;
+                    bus-type = <MEDIA_BUS_TYPE_PARALLEL>;
+                    bus-width = <8>;
+                    hsync-active = <1>;
+                    vsync-active = <1>;
+                    pclk-sample = <1>;
+                };
+            };
+        };
+    };
+
 ...
-- 
2.43.0

[PATCH v2 2/2] media: gc2145: implement basic dvp bus support

[Andrey Skvortsov]

Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
---
 drivers/media/i2c/gc2145.c | 122 +++++++++++++++++++++++++++++--------
 1 file changed, 96 insertions(+), 26 deletions(-)

diff --git a/drivers/media/i2c/gc2145.c b/drivers/media/i2c/gc2145.c
index bef7b0e056a8..2c5c9f3de5ff 100644
--- a/drivers/media/i2c/gc2145.c
+++ b/drivers/media/i2c/gc2145.c
@@ -39,6 +39,10 @@
 #define GC2145_REG_ANALOG_MODE1	CCI_REG8(0x17)
 #define GC2145_REG_OUTPUT_FMT	CCI_REG8(0x84)
 #define GC2145_REG_SYNC_MODE	CCI_REG8(0x86)
+#define GC2145_SYNC_MODE_VSYNC_POL	BIT(0)
+#define GC2145_SYNC_MODE_HSYNC_POL	BIT(1)
+#define GC2145_SYNC_MODE_OPCLK_POL	BIT(2)
+#define GC2145_SYNC_MODE_OPCLK_GATE	BIT(3)
 #define GC2145_SYNC_MODE_COL_SWITCH	BIT(4)
 #define GC2145_SYNC_MODE_ROW_SWITCH	BIT(5)
 #define GC2145_REG_BYPASS_MODE	CCI_REG8(0x89)
@@ -53,6 +57,12 @@
 #define GC2145_REG_GLOBAL_GAIN	CCI_REG8(0xb0)
 #define GC2145_REG_CHIP_ID	CCI_REG16(0xf0)
 #define GC2145_REG_PAD_IO	CCI_REG8(0xf2)
+#define GC2145_REG_PLL_MODE1	CCI_REG8(0xf7)
+#define GC2145_REG_PLL_MODE2	CCI_REG8(0xf8)
+#define GC2145_REG_CM_MODE	CCI_REG8(0xf9)
+#define GC2145_REG_CLK_DIV_MODE	CCI_REG8(0xfa)
+#define GC2145_REG_ANALOG_PWC	CCI_REG8(0xfc)
+#define GC2145_REG_PAD_IO	CCI_REG8(0xf2)
 #define GC2145_REG_PAGE_SELECT	CCI_REG8(0xfe)
 /* Page 3 */
 #define GC2145_REG_DPHY_ANALOG_MODE1	CCI_REG8(0x01)
@@ -598,6 +608,7 @@ struct gc2145 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 
+	struct v4l2_fwnode_endpoint ep; /* the parsed DT endpoint info */
 	struct regmap *regmap;
 	struct clk *xclk;
 
@@ -773,6 +784,38 @@ static int gc2145_set_pad_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int gc2145_config_dvp_mode(struct gc2145 *gc2145,
+				   const struct gc2145_format *gc2145_format)
+{
+	int ret = 0;
+	u64 sync_mode;
+	int flags;
+
+	flags = gc2145->ep.bus.parallel.flags;
+
+	ret = cci_read(gc2145->regmap, GC2145_REG_SYNC_MODE, &sync_mode, NULL);
+	if (ret)
+		return ret;
+
+	sync_mode &= ~(GC2145_SYNC_MODE_VSYNC_POL |
+		       GC2145_SYNC_MODE_HSYNC_POL |
+		       GC2145_SYNC_MODE_OPCLK_POL);
+
+	if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
+		sync_mode |= GC2145_SYNC_MODE_VSYNC_POL;
+
+	if (flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
+		sync_mode |= GC2145_SYNC_MODE_HSYNC_POL;
+
+	if (flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
+		sync_mode |= GC2145_SYNC_MODE_OPCLK_POL;
+
+	cci_write(gc2145->regmap, GC2145_REG_SYNC_MODE, sync_mode, &ret);
+	cci_write(gc2145->regmap, GC2145_REG_PAD_IO, 0x0f, &ret);
+
+	return ret;
+}
+
 static const struct cci_reg_sequence gc2145_common_mipi_regs[] = {
 	{GC2145_REG_PAGE_SELECT, 0x03},
 	{GC2145_REG_DPHY_ANALOG_MODE1, GC2145_DPHY_MODE_PHY_CLK_EN |
@@ -895,10 +938,13 @@ static int gc2145_start_streaming(struct gc2145 *gc2145,
 		goto err_rpm_put;
 	}
 
-	/* Perform MIPI specific configuration */
-	ret = gc2145_config_mipi_mode(gc2145, gc2145_format);
+	/* Perform interface specific configuration */
+	if (gc2145->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
+		ret = gc2145_config_mipi_mode(gc2145, gc2145_format);
+	else
+		ret = gc2145_config_dvp_mode(gc2145, gc2145_format);
 	if (ret) {
-		dev_err(&client->dev, "%s failed to write mipi conf\n",
+		dev_err(&client->dev, "%s failed to write interface conf\n",
 			__func__);
 		goto err_rpm_put;
 	}
@@ -924,6 +970,9 @@ static void gc2145_stop_streaming(struct gc2145 *gc2145)
 			GC2145_CSI2_MODE_EN | GC2145_CSI2_MODE_MIPI_EN, 0,
 			&ret);
 	cci_write(gc2145->regmap, GC2145_REG_PAGE_SELECT, 0x00, &ret);
+
+	/* Disable dvp streaming */
+	cci_write(gc2145->regmap, GC2145_REG_PAD_IO, 0x00, &ret);
 	if (ret)
 		dev_err(&client->dev, "%s failed to write regs\n", __func__);
 
@@ -1233,9 +1282,8 @@ static int gc2145_init_controls(struct gc2145 *gc2145)
 static int gc2145_check_hwcfg(struct device *dev)
 {
 	struct fwnode_handle *endpoint;
-	struct v4l2_fwnode_endpoint ep_cfg = {
-		.bus_type = V4L2_MBUS_CSI2_DPHY
-	};
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct gc2145 *gc2145 = to_gc2145(sd);
 	int ret;
 
 	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
@@ -1244,36 +1292,57 @@ static int gc2145_check_hwcfg(struct device *dev)
 		return -EINVAL;
 	}
 
-	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg);
+
+	gc2145->ep.bus_type = V4L2_MBUS_CSI2_DPHY;
+	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &gc2145->ep);
+	if (ret) {
+		gc2145->ep.bus_type = V4L2_MBUS_PARALLEL;
+		ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &gc2145->ep);
+	}
 	fwnode_handle_put(endpoint);
-	if (ret)
+	if (ret) {
+		dev_err(dev, "could not parse endpoint\n");
 		return ret;
-
-	/* Check the number of MIPI CSI2 data lanes */
-	if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
-		dev_err(dev, "only 2 data lanes are currently supported\n");
-		ret = -EINVAL;
-		goto out;
 	}
 
-	/* Check the link frequency set in device tree */
-	if (!ep_cfg.nr_of_link_frequencies) {
-		dev_err(dev, "link-frequency property not found in DT\n");
+	switch (gc2145->ep.bus_type) {
+	case V4L2_MBUS_CSI2_DPHY:
+		/* Check the link frequency set in device tree */
+		if (!gc2145->ep.nr_of_link_frequencies) {
+			dev_err(dev, "link-frequencies property not found in DT\n");
+			ret = -EINVAL;
+			goto out;
+		}
+
+		/* Check the number of MIPI CSI2 data lanes */
+		if (gc2145->ep.bus.mipi_csi2.num_data_lanes != 2) {
+			dev_err(dev, "only 2 data lanes are currently supported\n");
+			ret = -EINVAL;
+			goto out;
+		}
+
+		if (gc2145->ep.nr_of_link_frequencies != 3 ||
+			gc2145->ep.link_frequencies[0] != GC2145_640_480_LINKFREQ ||
+			gc2145->ep.link_frequencies[1] != GC2145_1280_720_LINKFREQ ||
+			gc2145->ep.link_frequencies[2] != GC2145_1600_1200_LINKFREQ) {
+			dev_err(dev, "Invalid link-frequencies provided\n");
+			ret = -EINVAL;
+			goto out;
+		}
+		break;
+
+	case V4L2_MBUS_PARALLEL:
+		break;
+	default:
+		dev_err(dev, "unsupported bus type %u\n", gc2145->ep.bus_type);
 		ret = -EINVAL;
 		goto out;
 	}
 
-	if (ep_cfg.nr_of_link_frequencies != 3 ||
-	    ep_cfg.link_frequencies[0] != GC2145_640_480_LINKFREQ ||
-	    ep_cfg.link_frequencies[1] != GC2145_1280_720_LINKFREQ ||
-	    ep_cfg.link_frequencies[2] != GC2145_1600_1200_LINKFREQ) {
-		dev_err(dev, "Invalid link-frequencies provided\n");
-		ret = -EINVAL;
-	}
+	return 0;
 
 out:
-	v4l2_fwnode_endpoint_free(&ep_cfg);
-
+	v4l2_fwnode_endpoint_free(&gc2145->ep);
 	return ret;
 }
 
@@ -1421,6 +1490,7 @@ static void gc2145_remove(struct i2c_client *client)
 	if (!pm_runtime_status_suspended(&client->dev))
 		gc2145_power_off(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
+	v4l2_fwnode_endpoint_free(&gc2145->ep);
 }
 
 static const struct of_device_id gc2145_dt_ids[] = {
-- 
2.43.0

Re: [PATCH v2 2/2] media: gc2145: implement basic dvp bus support

[Alain Volmat]

Hi Andrey,

sorry for the very long delay in reviewing / testing that patch.

I don't have an GC2145 parallel setup however I can confirm that
GC2145 CSI behaves properly with this patch applied on stm32mp13.

See below few comment

On Thu, Feb 22, 2024 at 09:22:14AM +0300, Andrey Skvortsov wrote:

> Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>

Maybe you could write a bit more within the commit log.

> ---
>  drivers/media/i2c/gc2145.c | 122 +++++++++++++++++++++++++++++--------
>  1 file changed, 96 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/i2c/gc2145.c b/drivers/media/i2c/gc2145.c
> index bef7b0e056a8..2c5c9f3de5ff 100644
> --- a/drivers/media/i2c/gc2145.c
> +++ b/drivers/media/i2c/gc2145.c
> @@ -39,6 +39,10 @@
>  #define GC2145_REG_ANALOG_MODE1	CCI_REG8(0x17)
>  #define GC2145_REG_OUTPUT_FMT	CCI_REG8(0x84)
>  #define GC2145_REG_SYNC_MODE	CCI_REG8(0x86)
> +#define GC2145_SYNC_MODE_VSYNC_POL	BIT(0)
> +#define GC2145_SYNC_MODE_HSYNC_POL	BIT(1)
> +#define GC2145_SYNC_MODE_OPCLK_POL	BIT(2)
> +#define GC2145_SYNC_MODE_OPCLK_GATE	BIT(3)

OPCLK_GATE added but never used.

>  #define GC2145_SYNC_MODE_COL_SWITCH	BIT(4)
>  #define GC2145_SYNC_MODE_ROW_SWITCH	BIT(5)
>  #define GC2145_REG_BYPASS_MODE	CCI_REG8(0x89)
> @@ -53,6 +57,12 @@
>  #define GC2145_REG_GLOBAL_GAIN	CCI_REG8(0xb0)
>  #define GC2145_REG_CHIP_ID	CCI_REG16(0xf0)
>  #define GC2145_REG_PAD_IO	CCI_REG8(0xf2)
> +#define GC2145_REG_PLL_MODE1	CCI_REG8(0xf7)
> +#define GC2145_REG_PLL_MODE2	CCI_REG8(0xf8)
> +#define GC2145_REG_CM_MODE	CCI_REG8(0xf9)
> +#define GC2145_REG_CLK_DIV_MODE	CCI_REG8(0xfa)
> +#define GC2145_REG_ANALOG_PWC	CCI_REG8(0xfc)

All 5 define added but never used, those settings are part of the cci_sequences
table. Maybe either keep the define and update the tables or drop the
new define ?

> +#define GC2145_REG_PAD_IO	CCI_REG8(0xf2)

Was already defined in the existing code, see above.

>  #define GC2145_REG_PAGE_SELECT	CCI_REG8(0xfe)
>  /* Page 3 */
>  #define GC2145_REG_DPHY_ANALOG_MODE1	CCI_REG8(0x01)
> @@ -598,6 +608,7 @@ struct gc2145 {
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
>  
> +	struct v4l2_fwnode_endpoint ep; /* the parsed DT endpoint info */
>  	struct regmap *regmap;
>  	struct clk *xclk;
>  
> @@ -773,6 +784,38 @@ static int gc2145_set_pad_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int gc2145_config_dvp_mode(struct gc2145 *gc2145,
> +				   const struct gc2145_format *gc2145_format)

alignment ?

> +{
> +	int ret = 0;
> +	u64 sync_mode;
> +	int flags;
> +
> +	flags = gc2145->ep.bus.parallel.flags;

int flags = gc2145->ep.bus.parallel.flags;
?

> +
> +	ret = cci_read(gc2145->regmap, GC2145_REG_SYNC_MODE, &sync_mode, NULL);
> +	if (ret)
> +		return ret;
> +
> +	sync_mode &= ~(GC2145_SYNC_MODE_VSYNC_POL |
> +		       GC2145_SYNC_MODE_HSYNC_POL |
> +		       GC2145_SYNC_MODE_OPCLK_POL);
> +
> +	if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> +		sync_mode |= GC2145_SYNC_MODE_VSYNC_POL;
> +
> +	if (flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> +		sync_mode |= GC2145_SYNC_MODE_HSYNC_POL;
> +
> +	if (flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> +		sync_mode |= GC2145_SYNC_MODE_OPCLK_POL;
> +
> +	cci_write(gc2145->regmap, GC2145_REG_SYNC_MODE, sync_mode, &ret);
> +	cci_write(gc2145->regmap, GC2145_REG_PAD_IO, 0x0f, &ret);
> +
> +	return ret;
> +}
> +
>  static const struct cci_reg_sequence gc2145_common_mipi_regs[] = {
>  	{GC2145_REG_PAGE_SELECT, 0x03},
>  	{GC2145_REG_DPHY_ANALOG_MODE1, GC2145_DPHY_MODE_PHY_CLK_EN |
> @@ -895,10 +938,13 @@ static int gc2145_start_streaming(struct gc2145 *gc2145,
>  		goto err_rpm_put;
>  	}
>  
> -	/* Perform MIPI specific configuration */
> -	ret = gc2145_config_mipi_mode(gc2145, gc2145_format);
> +	/* Perform interface specific configuration */
> +	if (gc2145->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
> +		ret = gc2145_config_mipi_mode(gc2145, gc2145_format);
> +	else
> +		ret = gc2145_config_dvp_mode(gc2145, gc2145_format);
>  	if (ret) {
> -		dev_err(&client->dev, "%s failed to write mipi conf\n",
> +		dev_err(&client->dev, "%s failed to write interface conf\n",
>  			__func__);
>  		goto err_rpm_put;
>  	}
> @@ -924,6 +970,9 @@ static void gc2145_stop_streaming(struct gc2145 *gc2145)
>  			GC2145_CSI2_MODE_EN | GC2145_CSI2_MODE_MIPI_EN, 0,
>  			&ret);
>  	cci_write(gc2145->regmap, GC2145_REG_PAGE_SELECT, 0x00, &ret);
> +
> +	/* Disable dvp streaming */
> +	cci_write(gc2145->regmap, GC2145_REG_PAD_IO, 0x00, &ret);
>  	if (ret)
>  		dev_err(&client->dev, "%s failed to write regs\n", __func__);
>  
> @@ -1233,9 +1282,8 @@ static int gc2145_init_controls(struct gc2145 *gc2145)
>  static int gc2145_check_hwcfg(struct device *dev)
>  {
>  	struct fwnode_handle *endpoint;
> -	struct v4l2_fwnode_endpoint ep_cfg = {
> -		.bus_type = V4L2_MBUS_CSI2_DPHY
> -	};
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct gc2145 *gc2145 = to_gc2145(sd);
>  	int ret;
>  
>  	endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> @@ -1244,36 +1292,57 @@ static int gc2145_check_hwcfg(struct device *dev)
>  		return -EINVAL;
>  	}
>  
> -	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg);
> +

no new line here.

> +	gc2145->ep.bus_type = V4L2_MBUS_CSI2_DPHY;
> +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &gc2145->ep);
> +	if (ret) {
> +		gc2145->ep.bus_type = V4L2_MBUS_PARALLEL;
> +		ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &gc2145->ep);
> +	}
>  	fwnode_handle_put(endpoint);
> -	if (ret)
> +	if (ret) {
> +		dev_err(dev, "could not parse endpoint\n");
>  		return ret;
> -
> -	/* Check the number of MIPI CSI2 data lanes */
> -	if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> -		dev_err(dev, "only 2 data lanes are currently supported\n");
> -		ret = -EINVAL;
> -		goto out;
>  	}
>  
> -	/* Check the link frequency set in device tree */
> -	if (!ep_cfg.nr_of_link_frequencies) {
> -		dev_err(dev, "link-frequency property not found in DT\n");
> +	switch (gc2145->ep.bus_type) {
> +	case V4L2_MBUS_CSI2_DPHY:
> +		/* Check the link frequency set in device tree */
> +		if (!gc2145->ep.nr_of_link_frequencies) {
> +			dev_err(dev, "link-frequencies property not found in DT\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}

any reason for change of the if order ? before num_data_lanes was
checked first, then nr_of_link_frequencies, then 3 link_frequencies.

> +
> +		/* Check the number of MIPI CSI2 data lanes */
> +		if (gc2145->ep.bus.mipi_csi2.num_data_lanes != 2) {
> +			dev_err(dev, "only 2 data lanes are currently supported\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		if (gc2145->ep.nr_of_link_frequencies != 3 ||
> +			gc2145->ep.link_frequencies[0] != GC2145_640_480_LINKFREQ ||
> +			gc2145->ep.link_frequencies[1] != GC2145_1280_720_LINKFREQ ||
> +			gc2145->ep.link_frequencies[2] != GC2145_1600_1200_LINKFREQ) {

alignment

> +			dev_err(dev, "Invalid link-frequencies provided\n");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		break;
> +

no newline here

> +	case V4L2_MBUS_PARALLEL:
> +		break;
> +	default:
> +		dev_err(dev, "unsupported bus type %u\n", gc2145->ep.bus_type);
>  		ret = -EINVAL;
>  		goto out;
>  	}
>  
> -	if (ep_cfg.nr_of_link_frequencies != 3 ||
> -	    ep_cfg.link_frequencies[0] != GC2145_640_480_LINKFREQ ||
> -	    ep_cfg.link_frequencies[1] != GC2145_1280_720_LINKFREQ ||
> -	    ep_cfg.link_frequencies[2] != GC2145_1600_1200_LINKFREQ) {
> -		dev_err(dev, "Invalid link-frequencies provided\n");
> -		ret = -EINVAL;
> -	}
> +	return 0;
>  
>  out:
> -	v4l2_fwnode_endpoint_free(&ep_cfg);
> -
> +	v4l2_fwnode_endpoint_free(&gc2145->ep);
>  	return ret;
>  }
>  
> @@ -1421,6 +1490,7 @@ static void gc2145_remove(struct i2c_client *client)
>  	if (!pm_runtime_status_suspended(&client->dev))
>  		gc2145_power_off(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> +	v4l2_fwnode_endpoint_free(&gc2145->ep);
>  }
>  
>  static const struct of_device_id gc2145_dt_ids[] = {
> -- 
> 2.43.0
> 

Alain

Re: [PATCH v2 2/2] media: gc2145: implement basic dvp bus support

[Andrey Skvortsov]

Hi Alain,

thanks for the review.

On 24-04-26 18:38, Alain Volmat wrote:
> Hi Andrey,
> 
> > --- a/drivers/media/i2c/gc2145.c
> > +++ b/drivers/media/i2c/gc2145.c
> > @@ -39,6 +39,10 @@
> >  #define GC2145_REG_ANALOG_MODE1	CCI_REG8(0x17)
> >  #define GC2145_REG_OUTPUT_FMT	CCI_REG8(0x84)
> >  #define GC2145_REG_SYNC_MODE	CCI_REG8(0x86)
> > +#define GC2145_SYNC_MODE_VSYNC_POL	BIT(0)
> > +#define GC2145_SYNC_MODE_HSYNC_POL	BIT(1)
> > +#define GC2145_SYNC_MODE_OPCLK_POL	BIT(2)
> > +#define GC2145_SYNC_MODE_OPCLK_GATE	BIT(3)
> 
> OPCLK_GATE added but never used.
All near bits were described. I decided to put description for this bit
as well, so all bits 0-5 are described in source code.

> 
> >  #define GC2145_SYNC_MODE_COL_SWITCH	BIT(4)
> >  #define GC2145_SYNC_MODE_ROW_SWITCH	BIT(5)
> >  #define GC2145_REG_BYPASS_MODE	CCI_REG8(0x89)
> > @@ -53,6 +57,12 @@
> >  #define GC2145_REG_GLOBAL_GAIN	CCI_REG8(0xb0)
> >  #define GC2145_REG_CHIP_ID	CCI_REG16(0xf0)
> >  #define GC2145_REG_PAD_IO	CCI_REG8(0xf2)
> > +#define GC2145_REG_PLL_MODE1	CCI_REG8(0xf7)
> > +#define GC2145_REG_PLL_MODE2	CCI_REG8(0xf8)
> > +#define GC2145_REG_CM_MODE	CCI_REG8(0xf9)
> > +#define GC2145_REG_CLK_DIV_MODE	CCI_REG8(0xfa)
> > +#define GC2145_REG_ANALOG_PWC	CCI_REG8(0xfc)
> 
> All 5 define added but never used, those settings are part of the cci_sequences
> table. Maybe either keep the define and update the tables or drop the
> new define ?

> > +#define GC2145_REG_PAD_IO	CCI_REG8(0xf2)
> 
> Was already defined in the existing code, see above.
Thanks, I'll drop all unneeded defines in v3.


> > @@ -773,6 +784,38 @@ static int gc2145_set_pad_format(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > +static int gc2145_config_dvp_mode(struct gc2145 *gc2145,
> > +				   const struct gc2145_format *gc2145_format)
> 
> alignment ?
Thanks.

> > +{
> > +	int ret = 0;
> > +	u64 sync_mode;
> > +	int flags;
> > +
> > +	flags = gc2145->ep.bus.parallel.flags;
> 
> int flags = gc2145->ep.bus.parallel.flags;
> ?
Fix in v3.

> > @@ -1244,36 +1292,57 @@ static int gc2145_check_hwcfg(struct device *dev)
> >  		return -EINVAL;
> >  	}
> >  
> > -	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg);
> > +
> 
> no new line here.
ok.

> > +	gc2145->ep.bus_type = V4L2_MBUS_CSI2_DPHY;
> > +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &gc2145->ep);
> > +	if (ret) {
> > +		gc2145->ep.bus_type = V4L2_MBUS_PARALLEL;
> > +		ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &gc2145->ep);
> > +	}
> >  	fwnode_handle_put(endpoint);
> > -	if (ret)
> > +	if (ret) {
> > +		dev_err(dev, "could not parse endpoint\n");
> >  		return ret;
> > -
> > -	/* Check the number of MIPI CSI2 data lanes */
> > -	if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> > -		dev_err(dev, "only 2 data lanes are currently supported\n");
> > -		ret = -EINVAL;
> > -		goto out;
> >  	}
> >  
> > -	/* Check the link frequency set in device tree */
> > -	if (!ep_cfg.nr_of_link_frequencies) {
> > -		dev_err(dev, "link-frequency property not found in DT\n");
> > +	switch (gc2145->ep.bus_type) {
> > +	case V4L2_MBUS_CSI2_DPHY:
> > +		/* Check the link frequency set in device tree */
> > +		if (!gc2145->ep.nr_of_link_frequencies) {
> > +			dev_err(dev, "link-frequencies property not found in DT\n");
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> 
> any reason for change of the if order ? before num_data_lanes was
> checked first, then nr_of_link_frequencies, then 3 link_frequencies.

No reason. I'll change order like it was before in v3.

> > +
> > +		/* Check the number of MIPI CSI2 data lanes */
> > +		if (gc2145->ep.bus.mipi_csi2.num_data_lanes != 2) {
> > +			dev_err(dev, "only 2 data lanes are currently supported\n");
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		if (gc2145->ep.nr_of_link_frequencies != 3 ||
> > +			gc2145->ep.link_frequencies[0] != GC2145_640_480_LINKFREQ ||
> > +			gc2145->ep.link_frequencies[1] != GC2145_1280_720_LINKFREQ ||
> > +			gc2145->ep.link_frequencies[2] != GC2145_1600_1200_LINKFREQ) {
> 
> alignment
Thanks.

> 
> > +			dev_err(dev, "Invalid link-frequencies provided\n");
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +		break;
> > +
> 
> no newline here
ok

-- 
Best regards,
Andrey Skvortsov