[PATCH drm/hisilicon 0/3] support reading resolutions from EDID

[PATCH drm/hisilicon 0/3] support reading resolutions from EDID

[Tian Tao]

patch #1 add a new file to implements i2c adapters, #2 read the
resolution from the edid, if that fails, set the resolution to fixed.
patch #3 update the destroy callback function to release the i2c adapters.

Tian Tao (3):
  drm/hisilicon: Support i2c driver algorithms for bit-shift adapters
  drm/hisilicon: Features to support reading resolutions from EDID
  drm/hisilicon: Releasing Resources in the Destroy callback Function

 drivers/gpu/drm/hisilicon/hibmc/Makefile         |  2 +-
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h  | 21 ++++-
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c  | 98 ++++++++++++++++++++++++
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 45 +++++++++--
 4 files changed, 157 insertions(+), 9 deletions(-)
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c

-- 
2.7.4

[PATCH drm/hisilicon 1/3] drm/hisilicon: Support i2c driver algorithms for bit-shift adapters

[Tian Tao]

Adding driver implementation to support i2c driver algorithms for
bit-shift adapters, so hibmc will using the interface provided by
drm to read edid.

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
 drivers/gpu/drm/hisilicon/hibmc/Makefile        |  2 +-
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 21 +++++-
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c | 98 +++++++++++++++++++++++++
 3 files changed, 119 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c

diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
index f991327..684ef79 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
+++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
@@ -1,4 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
-hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_ttm.o
+hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_ttm.o hibmc_drm_i2c.o
 
 obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
index 197485e..1b2edb3 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
@@ -14,11 +14,24 @@
 #ifndef HIBMC_DRM_DRV_H
 #define HIBMC_DRM_DRV_H
 
+#include <linux/gpio/consumer.h>
+#include <linux/i2c-algo-bit.h>
+#include <linux/i2c.h>
+
+#include <drm/drm_edid.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_framebuffer.h>
 
 struct drm_device;
 
+struct hibmc_connector {
+	struct drm_connector base;
+	struct drm_device  *dev;
+
+	struct i2c_adapter adapter;
+	struct i2c_algo_bit_data bit_data;
+};
+
 struct hibmc_drm_private {
 	/* hw */
 	void __iomem   *mmio;
@@ -31,10 +44,15 @@ struct hibmc_drm_private {
 	struct drm_plane primary_plane;
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;
-	struct drm_connector connector;
+	struct hibmc_connector connector;
 	bool mode_config_initialized;
 };
 
+static inline struct hibmc_connector *to_hibmc_connector(struct drm_connector *connector)
+{
+	return container_of(connector, struct hibmc_connector, base);
+}
+
 void hibmc_set_power_mode(struct hibmc_drm_private *priv,
 			  unsigned int power_mode);
 void hibmc_set_current_gate(struct hibmc_drm_private *priv,
@@ -47,6 +65,7 @@ int hibmc_mm_init(struct hibmc_drm_private *hibmc);
 void hibmc_mm_fini(struct hibmc_drm_private *hibmc);
 int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
 		      struct drm_mode_create_dumb *args);
+int hibmc_ddc_create(struct hibmc_connector *connector);
 
 extern const struct drm_mode_config_funcs hibmc_mode_funcs;
 
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
new file mode 100644
index 0000000..0506846
--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* Hisilicon Hibmc SoC drm driver
+ *
+ * Based on the bochs drm driver.
+ *
+ * Copyright (c) 2016 Huawei Limited.
+ *
+ * Author:
+ *      Tian Tao <tiantao6@hisilicon.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/pci.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_probe_helper.h>
+
+#include "hibmc_drm_drv.h"
+
+#define GPIO_DATA		0x0802A0
+#define GPIO_DATA_DIRECTION	0x0802A4
+
+#define GPIO_SCL_MASK 0x1
+#define GPIO_SDA_MASK  0x2
+
+static void hibmc_set_i2c_signal(void *data, u32 mask, int value)
+{
+	struct hibmc_connector *hibmc_connector = data;
+	struct hibmc_drm_private *priv = hibmc_connector->dev->dev_private;
+	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
+
+	if (value) {
+		tmp_dir &= ~mask;
+		writel(tmp_dir, priv->mmio + GPIO_DATA_DIRECTION);
+	} else {
+		u32 tmp_data = readl(priv->mmio + GPIO_DATA);
+
+		tmp_data &= ~mask;
+		writel(tmp_data, priv->mmio + GPIO_DATA);
+
+		tmp_dir |= mask;
+		writel(tmp_dir, priv->mmio + GPIO_DATA_DIRECTION);
+	}
+}
+
+static int hibmc_get_i2c_signal(void *data, u32 mask)
+{
+	struct hibmc_connector *hibmc_connector = data;
+	struct hibmc_drm_private *priv = hibmc_connector->dev->dev_private;
+	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
+
+	if ((tmp_dir & mask) != mask) {
+		tmp_dir &= ~mask;
+		writel(tmp_dir, priv->mmio + GPIO_DATA_DIRECTION);
+	}
+
+	return (readl(priv->mmio + GPIO_DATA) & mask) ? 1 : 0;
+}
+
+static void hibmc_ddc_setsda(void *data, int state)
+{
+	hibmc_set_i2c_signal(data, GPIO_SDA_MASK, state);
+}
+
+static void hibmc_ddc_setscl(void *data, int state)
+{
+	hibmc_set_i2c_signal(data, GPIO_SCL_MASK, state);
+}
+
+static int hibmc_ddc_getsda(void *data)
+{
+	return hibmc_get_i2c_signal(data, GPIO_SDA_MASK);
+}
+
+static int hibmc_ddc_getscl(void *data)
+{
+	return hibmc_get_i2c_signal(data, GPIO_SCL_MASK);
+}
+
+int hibmc_ddc_create(struct hibmc_connector *connector)
+{
+	connector->adapter.owner = THIS_MODULE;
+	connector->adapter.class = I2C_CLASS_DDC;
+	snprintf(connector->adapter.name, I2C_NAME_SIZE, "HIS i2c bit bus");
+	connector->adapter.dev.parent = &connector->dev->pdev->dev;
+	i2c_set_adapdata(&connector->adapter, connector);
+	connector->adapter.algo_data = &connector->bit_data;
+
+	connector->bit_data.udelay = 20;
+	connector->bit_data.timeout = usecs_to_jiffies(2000);
+	connector->bit_data.data = connector;
+	connector->bit_data.setsda = hibmc_ddc_setsda;
+	connector->bit_data.setscl = hibmc_ddc_setscl;
+	connector->bit_data.getsda = hibmc_ddc_getsda;
+	connector->bit_data.getscl = hibmc_ddc_getscl;
+
+	return i2c_bit_add_bus(&connector->adapter);
+}
-- 
2.7.4

Re: [PATCH drm/hisilicon 1/3] drm/hisilicon: Support i2c driver algorithms for bit-shift adapters

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 6885 bytes --]

Hi

Am 21.09.20 um 05:25 schrieb Tian Tao:
> Adding driver implementation to support i2c driver algorithms for
> bit-shift adapters, so hibmc will using the interface provided by
> drm to read edid.
> 
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/Makefile        |  2 +-
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 21 +++++-
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c | 98 +++++++++++++++++++++++++
>  3 files changed, 119 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> index f991327..684ef79 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> @@ -1,4 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_ttm.o
> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_ttm.o hibmc_drm_i2c.o
>  
>  obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index 197485e..1b2edb3 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -14,11 +14,24 @@
>  #ifndef HIBMC_DRM_DRV_H
>  #define HIBMC_DRM_DRV_H
>  
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c-algo-bit.h>
> +#include <linux/i2c.h>
> +
> +#include <drm/drm_edid.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_framebuffer.h>
>  
>  struct drm_device;
>  
> +struct hibmc_connector {
> +	struct drm_connector base;
> +	struct drm_device  *dev;

dev is not required. There's already a dev field under base.

> +
> +	struct i2c_adapter adapter;
> +	struct i2c_algo_bit_data bit_data;
> +};
> +
>  struct hibmc_drm_private {
>  	/* hw */
>  	void __iomem   *mmio;
> @@ -31,10 +44,15 @@ struct hibmc_drm_private {
>  	struct drm_plane primary_plane;
>  	struct drm_crtc crtc;
>  	struct drm_encoder encoder;
> -	struct drm_connector connector;
> +	struct hibmc_connector connector;
>  	bool mode_config_initialized;
>  };
>  
> +static inline struct hibmc_connector *to_hibmc_connector(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct hibmc_connector, base);
> +}
> +
>  void hibmc_set_power_mode(struct hibmc_drm_private *priv,
>  			  unsigned int power_mode);
>  void hibmc_set_current_gate(struct hibmc_drm_private *priv,
> @@ -47,6 +65,7 @@ int hibmc_mm_init(struct hibmc_drm_private *hibmc);
>  void hibmc_mm_fini(struct hibmc_drm_private *hibmc);
>  int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>  		      struct drm_mode_create_dumb *args);
> +int hibmc_ddc_create(struct hibmc_connector *connector);
>  
>  extern const struct drm_mode_config_funcs hibmc_mode_funcs;
>  
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
> new file mode 100644
> index 0000000..0506846
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_i2c.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Hisilicon Hibmc SoC drm driver
> + *
> + * Based on the bochs drm driver.
> + *
> + * Copyright (c) 2016 Huawei Limited.
> + *
> + * Author:
> + *      Tian Tao <tiantao6@hisilicon.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/pci.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_probe_helper.h>
> +
> +#include "hibmc_drm_drv.h"
> +
> +#define GPIO_DATA		0x0802A0
> +#define GPIO_DATA_DIRECTION	0x0802A4
> +
> +#define GPIO_SCL_MASK 0x1
> +#define GPIO_SDA_MASK  0x2

Weird indention.

> +
> +static void hibmc_set_i2c_signal(void *data, u32 mask, int value)
> +{
> +	struct hibmc_connector *hibmc_connector = data;
> +	struct hibmc_drm_private *priv = hibmc_connector->dev->dev_private;

Oh, hibmc is still using dev_private. dev_private is on it's way out.

The least thing to do is to make little function that wraps the
conversion from struct drm_device to hibmc_drm_private. And then convert
over all uses of dev_private over to the function. That's for another
patchset.


That's all just nits, so

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Best regards
Thomas

> +	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
> +
> +	if (value) {
> +		tmp_dir &= ~mask;
> +		writel(tmp_dir, priv->mmio + GPIO_DATA_DIRECTION);
> +	} else {
> +		u32 tmp_data = readl(priv->mmio + GPIO_DATA);
> +
> +		tmp_data &= ~mask;
> +		writel(tmp_data, priv->mmio + GPIO_DATA);
> +
> +		tmp_dir |= mask;
> +		writel(tmp_dir, priv->mmio + GPIO_DATA_DIRECTION);
> +	}
> +}
> +
> +static int hibmc_get_i2c_signal(void *data, u32 mask)
> +{
> +	struct hibmc_connector *hibmc_connector = data;
> +	struct hibmc_drm_private *priv = hibmc_connector->dev->dev_private;
> +	u32 tmp_dir = readl(priv->mmio + GPIO_DATA_DIRECTION);
> +
> +	if ((tmp_dir & mask) != mask) {
> +		tmp_dir &= ~mask;
> +		writel(tmp_dir, priv->mmio + GPIO_DATA_DIRECTION);
> +	}
> +
> +	return (readl(priv->mmio + GPIO_DATA) & mask) ? 1 : 0;
> +}
> +
> +static void hibmc_ddc_setsda(void *data, int state)
> +{
> +	hibmc_set_i2c_signal(data, GPIO_SDA_MASK, state);
> +}
> +
> +static void hibmc_ddc_setscl(void *data, int state)
> +{
> +	hibmc_set_i2c_signal(data, GPIO_SCL_MASK, state);
> +}
> +
> +static int hibmc_ddc_getsda(void *data)
> +{
> +	return hibmc_get_i2c_signal(data, GPIO_SDA_MASK);
> +}
> +
> +static int hibmc_ddc_getscl(void *data)
> +{
> +	return hibmc_get_i2c_signal(data, GPIO_SCL_MASK);
> +}
> +
> +int hibmc_ddc_create(struct hibmc_connector *connector)
> +{
> +	connector->adapter.owner = THIS_MODULE;
> +	connector->adapter.class = I2C_CLASS_DDC;
> +	snprintf(connector->adapter.name, I2C_NAME_SIZE, "HIS i2c bit bus");
> +	connector->adapter.dev.parent = &connector->dev->pdev->dev;
> +	i2c_set_adapdata(&connector->adapter, connector);
> +	connector->adapter.algo_data = &connector->bit_data;
> +
> +	connector->bit_data.udelay = 20;
> +	connector->bit_data.timeout = usecs_to_jiffies(2000);
> +	connector->bit_data.data = connector;
> +	connector->bit_data.setsda = hibmc_ddc_setsda;
> +	connector->bit_data.setscl = hibmc_ddc_setscl;
> +	connector->bit_data.getsda = hibmc_ddc_getsda;
> +	connector->bit_data.getscl = hibmc_ddc_getscl;
> +
> +	return i2c_bit_add_bus(&connector->adapter);
> +}
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[PATCH drm/hisilicon 2/3] drm/hisilicon: Features to support reading resolutions from EDID

[Tian Tao]

Use drm_get_edid to get the resolution, if that fails, set it to
a fixed resolution.

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 35 ++++++++++++++++++++----
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
index 376a05d..e84d381 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
@@ -20,13 +20,24 @@
 
 static int hibmc_connector_get_modes(struct drm_connector *connector)
 {
-	int count;
+	int count = 0;
+	void *edid;
+	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
+
+	edid = drm_get_edid(connector, &hibmc_connector->adapter);
+	if (edid) {
+		drm_connector_update_edid_property(connector, edid);
+		count = drm_add_edid_modes(connector, edid);
+	}
 
-	count = drm_add_modes_noedid(connector,
+	if (!edid || count == 0) {
+		count = drm_add_modes_noedid(connector,
 				     connector->dev->mode_config.max_width,
 				     connector->dev->mode_config.max_height);
-	drm_set_preferred_mode(connector, 1024, 768);
+		drm_set_preferred_mode(connector, 1024, 768);
+	}
 
+	kfree(edid);
 	return count;
 }
 
@@ -77,10 +88,19 @@ static const struct drm_encoder_funcs hibmc_encoder_funcs = {
 int hibmc_vdac_init(struct hibmc_drm_private *priv)
 {
 	struct drm_device *dev = priv->dev;
+	struct hibmc_connector *hibmc_connector = &priv->connector;
 	struct drm_encoder *encoder = &priv->encoder;
-	struct drm_connector *connector = &priv->connector;
+	struct drm_connector *connector = &hibmc_connector->base;
 	int ret;
 
+	hibmc_connector->dev = dev;
+
+	ret = hibmc_ddc_create(hibmc_connector);
+	if (ret) {
+		drm_err(dev, "failed to create connector: %d\n", ret);
+		return ret;
+	}
+
 	encoder->possible_crtcs = 0x1;
 	ret = drm_encoder_init(dev, encoder, &hibmc_encoder_funcs,
 			       DRM_MODE_ENCODER_DAC, NULL);
@@ -91,12 +111,15 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
 
 	drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
 
-	ret = drm_connector_init(dev, connector, &hibmc_connector_funcs,
-				 DRM_MODE_CONNECTOR_VGA);
+	ret = drm_connector_init_with_ddc(dev, connector,
+					  &hibmc_connector_funcs,
+					  DRM_MODE_CONNECTOR_VGA,
+					  &hibmc_connector->adapter);
 	if (ret) {
 		drm_err(dev, "failed to init connector: %d\n", ret);
 		return ret;
 	}
+
 	drm_connector_helper_add(connector, &hibmc_connector_helper_funcs);
 
 	drm_connector_attach_encoder(connector, encoder);
-- 
2.7.4

Re: [PATCH drm/hisilicon 2/3] drm/hisilicon: Features to support reading resolutions from EDID

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3449 bytes --]

Hi

Am 21.09.20 um 05:25 schrieb Tian Tao:
> Use drm_get_edid to get the resolution, if that fails, set it to
> a fixed resolution.
> 
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 35 ++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> index 376a05d..e84d381 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> @@ -20,13 +20,24 @@
>  
>  static int hibmc_connector_get_modes(struct drm_connector *connector)
>  {
> -	int count;
> +	int count = 0;
> +	void *edid;
> +	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
> +
> +	edid = drm_get_edid(connector, &hibmc_connector->adapter);
> +	if (edid) {
> +		drm_connector_update_edid_property(connector, edid);
> +		count = drm_add_edid_modes(connector, edid);

There's nothing wrong here, but i think it might be more readable if you do

  if (count)
    goto out;

within this branch. The out label would be just before the kfree. With
this change, the code with drm_add_modes_noedid() would remain the same.

> +	}
>  
> -	count = drm_add_modes_noedid(connector,
> +	if (!edid || count == 0) {
> +		count = drm_add_modes_noedid(connector,
>  				     connector->dev->mode_config.max_width,
>  				     connector->dev->mode_config.max_height);
> -	drm_set_preferred_mode(connector, 1024, 768);
> +		drm_set_preferred_mode(connector, 1024, 768);
> +	}
>  
> +	kfree(edid);
>  	return count;
>  }
>  
> @@ -77,10 +88,19 @@ static const struct drm_encoder_funcs hibmc_encoder_funcs = {
>  int hibmc_vdac_init(struct hibmc_drm_private *priv)
>  {
>  	struct drm_device *dev = priv->dev;
> +	struct hibmc_connector *hibmc_connector = &priv->connector;
>  	struct drm_encoder *encoder = &priv->encoder;
> -	struct drm_connector *connector = &priv->connector;
> +	struct drm_connector *connector = &hibmc_connector->base;
>  	int ret;
>  
> +	hibmc_connector->dev = dev;

As mentioned, this dev field should be removed.

> +
> +	ret = hibmc_ddc_create(hibmc_connector);
> +	if (ret) {
> +		drm_err(dev, "failed to create connector: %d\n", ret);

The error message appears to be incorrect. (?)

> +		return ret;
> +	}
> +
>  	encoder->possible_crtcs = 0x1;
>  	ret = drm_encoder_init(dev, encoder, &hibmc_encoder_funcs,
>  			       DRM_MODE_ENCODER_DAC, NULL);
> @@ -91,12 +111,15 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>  
>  	drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs);
>  
> -	ret = drm_connector_init(dev, connector, &hibmc_connector_funcs,
> -				 DRM_MODE_CONNECTOR_VGA);
> +	ret = drm_connector_init_with_ddc(dev, connector,
> +					  &hibmc_connector_funcs,
> +					  DRM_MODE_CONNECTOR_VGA,
> +					  &hibmc_connector->adapter);
>  	if (ret) {
>  		drm_err(dev, "failed to init connector: %d\n", ret);
>  		return ret;
>  	}
> +
>  	drm_connector_helper_add(connector, &hibmc_connector_helper_funcs);
>  
>  	drm_connector_attach_encoder(connector, encoder);
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[PATCH drm/hisilicon 3/3] drm/hisilicon: Releasing Resources in the Destroy callback Function

[Tian Tao]

Rewrite the desrtoy callback function to release resources.

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
index e84d381..f1541ff 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
@@ -47,6 +47,14 @@ static enum drm_mode_status hibmc_connector_mode_valid(struct drm_connector *con
 	return MODE_OK;
 }
 
+static void hibmc_connector_destroy(struct drm_connector *connector)
+{
+	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
+
+	i2c_del_adapter(&hibmc_connector->adapter);
+	drm_connector_cleanup(connector);
+}
+
 static const struct drm_connector_helper_funcs
 	hibmc_connector_helper_funcs = {
 	.get_modes = hibmc_connector_get_modes,
@@ -55,7 +63,7 @@ static const struct drm_connector_helper_funcs
 
 static const struct drm_connector_funcs hibmc_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = drm_connector_cleanup,
+	.destroy = hibmc_connector_destroy,
 	.reset = drm_atomic_helper_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-- 
2.7.4

Re: [PATCH drm/hisilicon 3/3] drm/hisilicon: Releasing Resources in the Destroy callback Function

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1965 bytes --]

Hi

The code looks correcet, but I think this patch should be merged into
patch 2. Anyone who initializes the i2c adapter certainly wants it
cleaned up as well :)

Best regards
Thomas


Am 21.09.20 um 05:25 schrieb Tian Tao:
> Rewrite the desrtoy callback function to release resources.
> 
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> index e84d381..f1541ff 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c
> @@ -47,6 +47,14 @@ static enum drm_mode_status hibmc_connector_mode_valid(struct drm_connector *con
>  	return MODE_OK;
>  }
>  
> +static void hibmc_connector_destroy(struct drm_connector *connector)
> +{
> +	struct hibmc_connector *hibmc_connector = to_hibmc_connector(connector);
> +
> +	i2c_del_adapter(&hibmc_connector->adapter);
> +	drm_connector_cleanup(connector);
> +}
> +
>  static const struct drm_connector_helper_funcs
>  	hibmc_connector_helper_funcs = {
>  	.get_modes = hibmc_connector_get_modes,
> @@ -55,7 +63,7 @@ static const struct drm_connector_helper_funcs
>  
>  static const struct drm_connector_funcs hibmc_connector_funcs = {
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.destroy = drm_connector_cleanup,
> +	.destroy = hibmc_connector_destroy,
>  	.reset = drm_atomic_helper_connector_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]