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

[PATCH 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

 .../bindings/media/i2c/galaxycore,gc2145.yaml |  33 ++++-
 drivers/media/i2c/gc2145.c                    | 117 ++++++++++++++----
 2 files changed, 120 insertions(+), 30 deletions(-)

-- 
2.43.0

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

[Andrey Skvortsov]

Don't require link-frequencies like it's done for ov5640, that
supports both CSI-2 and DVP. And v4l2_fwnode_endpoint_alloc_parse
ignores link-frequencies property for DVP endpoint. It's used only for
CSI-2 endpoints

Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
---
 .../bindings/media/i2c/galaxycore,gc2145.yaml | 33 +++++++++++++++++--
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
index 1726ecca4c77..fb376b9d0f2a 100644
--- a/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
@@ -61,9 +61,6 @@ properties:
         properties:
           link-frequencies: true
 
-        required:
-          - link-frequencies
-
     required:
       - endpoint
 
@@ -110,4 +107,34 @@ examples:
         };
     };
 
+  - |
+    #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-width = <8>;
+                    hsync-active = <1>;
+                    vsync-active = <1>;
+                    data-active = <1>;
+                    pclk-sample = <1>;
+                };
+            };
+        };
+    };
+
 ...
-- 
2.43.0

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

[Andrey Skvortsov]

Tested on PinePhone with libcamera-based GNOME screenshot.

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

diff --git a/drivers/media/i2c/gc2145.c b/drivers/media/i2c/gc2145.c
index bef7b0e056a8..9a70b8d504e1 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;
 
@@ -612,6 +623,11 @@ struct gc2145 {
 	const struct gc2145_mode *mode;
 };
 
+static inline bool gc2145_is_csi2(const struct gc2145 *gc2145)
+{
+	return gc2145->ep.bus_type == V4L2_MBUS_CSI2_DPHY;
+}
+
 static inline struct gc2145 *to_gc2145(struct v4l2_subdev *_sd)
 {
 	return container_of(_sd, struct gc2145, sd);
@@ -773,6 +789,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 +943,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_is_csi2(gc2145))
+		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 +975,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 +1287,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 +1297,46 @@ static int gc2145_check_hwcfg(struct device *dev)
 		return -EINVAL;
 	}
 
-	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg);
+	ret = v4l2_fwnode_endpoint_parse(endpoint, &gc2145->ep);
 	fwnode_handle_put(endpoint);
 	if (ret)
 		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;
-	}
+	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;
 
-	/* 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");
+	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;
-	}
-
 out:
-	v4l2_fwnode_endpoint_free(&ep_cfg);
-
 	return ret;
 }
 
-- 
2.43.0

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

[Krzysztof Kozlowski]

On 17/02/2024 23:03, Andrey Skvortsov wrote:
> Don't require link-frequencies like it's done for ov5640, that
> supports both CSI-2 and DVP. And v4l2_fwnode_endpoint_alloc_parse
> ignores link-frequencies property for DVP endpoint. It's used only for
> CSI-2 endpoints
> 
> Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> ---

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Sakari Ailus]

Hi Andrey,

Thanks for the patchset.

On Sun, Feb 18, 2024 at 01:03:07AM +0300, Andrey Skvortsov wrote:
> Don't require link-frequencies like it's done for ov5640, that
> supports both CSI-2 and DVP. And v4l2_fwnode_endpoint_alloc_parse
> ignores link-frequencies property for DVP endpoint. It's used only for
> CSI-2 endpoints
> 
> Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> ---
>  .../bindings/media/i2c/galaxycore,gc2145.yaml | 33 +++++++++++++++++--
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
> index 1726ecca4c77..fb376b9d0f2a 100644
> --- a/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
> @@ -61,9 +61,6 @@ properties:
>          properties:
>            link-frequencies: true
>  
> -        required:
> -          - link-frequencies

That seems like a bad idea to me.

While for parallel interface it may not be often important, for CSI-2 it
should stay.

> -
>      required:
>        - endpoint
>  
> @@ -110,4 +107,34 @@ examples:
>          };
>      };
>  
> +  - |
> +    #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-width = <8>;
> +                    hsync-active = <1>;
> +                    vsync-active = <1>;
> +                    data-active = <1>;
> +                    pclk-sample = <1>;

Are there defaults for these if there are no such properties?

> +                };
> +            };
> +        };
> +    };
> +
>  ...

-- 
Regards,

Sakari Ailus

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

[Sakari Ailus]

On Tue, Feb 20, 2024 at 10:46:23AM +0000, Sakari Ailus wrote:
> Hi Andrey,
> 
> Thanks for the patchset.
> 
> On Sun, Feb 18, 2024 at 01:03:07AM +0300, Andrey Skvortsov wrote:
> > Don't require link-frequencies like it's done for ov5640, that
> > supports both CSI-2 and DVP. And v4l2_fwnode_endpoint_alloc_parse
> > ignores link-frequencies property for DVP endpoint. It's used only for
> > CSI-2 endpoints
> > 
> > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> > ---
> >  .../bindings/media/i2c/galaxycore,gc2145.yaml | 33 +++++++++++++++++--
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
> > index 1726ecca4c77..fb376b9d0f2a 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
> > @@ -61,9 +61,6 @@ properties:
> >          properties:
> >            link-frequencies: true
> >  
> > -        required:
> > -          - link-frequencies
> 
> That seems like a bad idea to me.
> 
> While for parallel interface it may not be often important, for CSI-2 it
> should stay.

You're also missing the bus-type property which should default to CSI-2
D-PHY.

-- 
Sakari Ailus

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

[Sakari Ailus]

Hi Andrey,

On Sun, Feb 18, 2024 at 01:03:08AM +0300, Andrey Skvortsov wrote:
> Tested on PinePhone with libcamera-based GNOME screenshot.
> 
> Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> ---
>  drivers/media/i2c/gc2145.c | 117 ++++++++++++++++++++++++++++---------
>  1 file changed, 90 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/i2c/gc2145.c b/drivers/media/i2c/gc2145.c
> index bef7b0e056a8..9a70b8d504e1 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;
>  
> @@ -612,6 +623,11 @@ struct gc2145 {
>  	const struct gc2145_mode *mode;
>  };
>  
> +static inline bool gc2145_is_csi2(const struct gc2145 *gc2145)
> +{
> +	return gc2145->ep.bus_type == V4L2_MBUS_CSI2_DPHY;

This is used in a single place. Could you move this comparison there?

> +}
> +
>  static inline struct gc2145 *to_gc2145(struct v4l2_subdev *_sd)
>  {
>  	return container_of(_sd, struct gc2145, sd);
> @@ -773,6 +789,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 +943,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_is_csi2(gc2145))
> +		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 +975,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 +1287,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
> -	};

First try D-PHY and if that fails, then try PARALLEL.

> +	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 +1297,46 @@ static int gc2145_check_hwcfg(struct device *dev)
>  		return -EINVAL;
>  	}
>  
> -	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg);
> +	ret = v4l2_fwnode_endpoint_parse(endpoint, &gc2145->ep);

You won't have any link frequencies available with this change.

>  	fwnode_handle_put(endpoint);
>  	if (ret)
>  		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;
> -	}
> +	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;
>  
> -	/* 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");
> +	case V4L2_MBUS_PARALLEL:
> +		break;
> +	default:
> +		dev_err(dev, "unsupported bus type %u\n",
> +			gc2145->ep.bus_type);

Fits on the previous line.

>  		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;
> -	}
> -
>  out:
> -	v4l2_fwnode_endpoint_free(&ep_cfg);
> -
>  	return ret;
>  }
>  

-- 
Regards,

Sakari Ailus

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

[Andrey Skvortsov]

Hi Sakari,

On 24-02-20 10:55, Sakari Ailus wrote:
> Hi Andrey,
> 
> On Sun, Feb 18, 2024 at 01:03:08AM +0300, Andrey Skvortsov wrote:
> > Tested on PinePhone with libcamera-based GNOME screenshot.
> > 
> > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> > ---
> >  drivers/media/i2c/gc2145.c | 117 ++++++++++++++++++++++++++++---------
> >  1 file changed, 90 insertions(+), 27 deletions(-)
> > 
> >  
> > @@ -612,6 +623,11 @@ struct gc2145 {
> >  	const struct gc2145_mode *mode;
> >  };
> >  
> > +static inline bool gc2145_is_csi2(const struct gc2145 *gc2145)
> > +{
> > +	return gc2145->ep.bus_type == V4L2_MBUS_CSI2_DPHY;
> 
> This is used in a single place. Could you move this comparison there?
Yes, I'll do.

> 
> > +}
> > +

> >  	}
> > @@ -924,6 +975,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 +1287,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
> > -	};
> 
> First try D-PHY and if that fails, then try PARALLEL.
> 
> > +	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 +1297,46 @@ static int gc2145_check_hwcfg(struct device *dev)
> >  		return -EINVAL;
> >  	}
> >  
> > -	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg);
> > +	ret = v4l2_fwnode_endpoint_parse(endpoint, &gc2145->ep);
> 
> You won't have any link frequencies available with this change.
Thanks for catching this up. I'll fix that.

> 
> >  	fwnode_handle_put(endpoint);
> >  	if (ret)
> >  		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;
> > -	}
> > +	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;
> >  
> > -	/* 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");
> > +	case V4L2_MBUS_PARALLEL:
> > +		break;
> > +	default:
> > +		dev_err(dev, "unsupported bus type %u\n",
> > +			gc2145->ep.bus_type);
> 
> Fits on the previous line.
I'll change this in v2.


-- 
Best regards,
Andrey Skvortsov

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

[Andrey Skvortsov]

Hi Sakari,

On 24-02-20 10:47, Sakari Ailus wrote:
> On Tue, Feb 20, 2024 at 10:46:23AM +0000, Sakari Ailus wrote:
> > Hi Andrey,
> > 
> > Thanks for the patchset.
> > 
> > On Sun, Feb 18, 2024 at 01:03:07AM +0300, Andrey Skvortsov wrote:
> > > Don't require link-frequencies like it's done for ov5640, that
> > > supports both CSI-2 and DVP. And v4l2_fwnode_endpoint_alloc_parse
> > > ignores link-frequencies property for DVP endpoint. It's used only for
> > > CSI-2 endpoints
> > > 
> > > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> > > ---
> > >  .../bindings/media/i2c/galaxycore,gc2145.yaml | 33 +++++++++++++++++--
> > >  1 file changed, 30 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
> > > index 1726ecca4c77..fb376b9d0f2a 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
> > > @@ -61,9 +61,6 @@ properties:
> > >          properties:
> > >            link-frequencies: true
> > >  
> > > -        required:
> > > -          - link-frequencies
> > 
> > That seems like a bad idea to me.
> > 
> > While for parallel interface it may not be often important, for CSI-2 it
> > should stay.
> 
> You're also missing the bus-type property which should default to CSI-2
> D-PHY.

I'll add this type and make link-frequencies required based on that
for CSI-2 bus.

-- 
Best regards,
Andrey Skvortsov

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

[Andrey Skvortsov]

On 24-02-20 10:46, Sakari Ailus wrote:
> Hi Andrey,
> 
> Thanks for the patchset.
> 
> On Sun, Feb 18, 2024 at 01:03:07AM +0300, Andrey Skvortsov wrote:
> > Don't require link-frequencies like it's done for ov5640, that
> > supports both CSI-2 and DVP. And v4l2_fwnode_endpoint_alloc_parse
> > ignores link-frequencies property for DVP endpoint. It's used only for
> > CSI-2 endpoints
> > 
> > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> > ---
> >  .../bindings/media/i2c/galaxycore,gc2145.yaml | 33 +++++++++++++++++--
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
> > index 1726ecca4c77..fb376b9d0f2a 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml
> > @@ -61,9 +61,6 @@ properties:
> >          properties:
> >            link-frequencies: true
> >  
> > -        required:
> > -          - link-frequencies
> 
> That seems like a bad idea to me.
> 
> While for parallel interface it may not be often important, for CSI-2 it
> should stay.

I'll add this type and make link-frequencies required based on that
for CSI-2 bus.

> 
> > -
> >      required:
> >        - endpoint
> >  
> > @@ -110,4 +107,34 @@ examples:
> >          };
> >      };
> >  
> > +  - |
> > +    #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-width = <8>;
> > +                    hsync-active = <1>;
> > +                    vsync-active = <1>;
> > +                    data-active = <1>;
> > +                    pclk-sample = <1>;
> 
> Are there defaults for these if there are no such properties?

good point. I'll add them to the endpoint properties description then.

-- 
Best regards,
Andrey Skvortsov