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
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 <[email protected]>
---
.../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 = <¶llel_from_gc2145>;
+ bus-width = <8>;
+ hsync-active = <1>;
+ vsync-active = <1>;
+ data-active = <1>;
+ pclk-sample = <1>;
+ };
+ };
+ };
+ };
+
...
--
2.43.0
Tested on PinePhone with libcamera-based GNOME screenshot.
Signed-off-by: Andrey Skvortsov <[email protected]>
---
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
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 <[email protected]>
> ---
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> .../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 = <¶llel_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
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 <[email protected]>
> > ---
> > .../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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > ---
> > > .../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
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 <[email protected]>
> > ---
> > .../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 = <¶llel_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