[PATCH 00/13] drm/ast: Convert to managed initialization

[PATCH 00/13] drm/ast: Convert to managed initialization

[Thomas Zimmermann]

This is the final patchset for converting ast to managed initialization.

Patches #1 to #4 address I2C helpers. The structures are being stored
in struct ast_connector. The initialization and cleanups is being converted
to managed release helpers.

Patches #5 to #10 address modesetting and device structures. All are
being embedded into struct ast_private. With struct ast_private being
a subclass of struct drm_device, patch #10 switches ast to DRM's managed-
allocation helpers.

Patches #11 and #12 address firmware memory that ast allocates
internally.

Finally, patch #13 removes ast's destroy function in favor of managed
release helpers.

Tested on AST 2100 HW.

Thomas Zimmermann (13):
  drm/ast: Move I2C code within ast_mode.c
  drm/ast: Test if I2C support has been initialized
  drm/ast: Embed I2C fields in struct ast_connector
  drm/ast: Managed release of I2C adapter
  drm/ast: Embed CRTC and connector in struct ast_private
  drm/ast: Separate DRM driver from PCI code
  drm/ast: Replace driver load/unload functions with device
    create/destroy
  drm/ast: Replace struct_drm_device.dev_private with to_ast_private()
  drm/ast: Don't use ast->dev if dev is available
  drm/ast: Embed struct drm_device in struct ast_private
  drm/ast: Managed release of ast firmware
  drm/ast: Manage release of firmware backup memory
  drm/ast: Managed device release

 drivers/gpu/drm/ast/ast_cursor.c |   8 +-
 drivers/gpu/drm/ast/ast_dp501.c  |  23 ++-
 drivers/gpu/drm/ast/ast_drv.c    |  82 ++++----
 drivers/gpu/drm/ast/ast_drv.h    |  43 +++--
 drivers/gpu/drm/ast/ast_main.c   |  74 ++++----
 drivers/gpu/drm/ast/ast_mm.c     |   2 +-
 drivers/gpu/drm/ast/ast_mode.c   | 310 ++++++++++++++-----------------
 drivers/gpu/drm/ast/ast_post.c   |   6 +-
 8 files changed, 263 insertions(+), 285 deletions(-)

--
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/13] drm/ast: Move I2C code within ast_mode.c

[Thomas Zimmermann]

The I2C support feels slammed down to the end of ast_mode.c. Improve
this by moving the code before it's callers, remove the declarations,
rename the callbacks to match I2C's get/set sda/scl convention, and
prefix all functions with ast_. No functional changes have been made.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 249 +++++++++++++++++----------------
 1 file changed, 125 insertions(+), 124 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 154cd877d9d1..19f1dfc8e9e0 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -46,9 +46,6 @@
 #include "ast_drv.h"
 #include "ast_tables.h"
 
-static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
-static void ast_i2c_destroy(struct ast_i2c_chan *i2c);
-
 static inline void ast_load_palette_index(struct ast_private *ast,
 				     u8 index, u8 red, u8 green,
 				     u8 blue)
@@ -514,6 +511,131 @@ static void ast_set_start_address_crt1(struct ast_private *ast,
 
 }
 
+/*
+ * I2C
+ */
+
+static int ast_i2c_getscl(void *i2c_priv)
+{
+	struct ast_i2c_chan *i2c = i2c_priv;
+	struct ast_private *ast = to_ast_private(i2c->dev);
+	uint32_t val, val2, count, pass;
+
+	count = 0;
+	pass = 0;
+	val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
+	do {
+		val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
+		if (val == val2) {
+			pass++;
+		} else {
+			pass = 0;
+			val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
+		}
+	} while ((pass < 5) && (count++ < 0x10000));
+
+	return val & 1 ? 1 : 0;
+}
+
+static int ast_i2c_getsda(void *i2c_priv)
+{
+	struct ast_i2c_chan *i2c = i2c_priv;
+	struct ast_private *ast = to_ast_private(i2c->dev);
+	uint32_t val, val2, count, pass;
+
+	count = 0;
+	pass = 0;
+	val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
+	do {
+		val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
+		if (val == val2) {
+			pass++;
+		} else {
+			pass = 0;
+			val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
+		}
+	} while ((pass < 5) && (count++ < 0x10000));
+
+	return val & 1 ? 1 : 0;
+}
+
+static void ast_i2c_setscl(void *i2c_priv, int clock)
+{
+	struct ast_i2c_chan *i2c = i2c_priv;
+	struct ast_private *ast = to_ast_private(i2c->dev);
+	int i;
+	u8 ujcrb7, jtemp;
+
+	for (i = 0; i < 0x10000; i++) {
+		ujcrb7 = ((clock & 0x01) ? 0 : 1);
+		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7);
+		jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01);
+		if (ujcrb7 == jtemp)
+			break;
+	}
+}
+
+static void ast_i2c_setsda(void *i2c_priv, int data)
+{
+	struct ast_i2c_chan *i2c = i2c_priv;
+	struct ast_private *ast = to_ast_private(i2c->dev);
+	int i;
+	u8 ujcrb7, jtemp;
+
+	for (i = 0; i < 0x10000; i++) {
+		ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;
+		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7);
+		jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04);
+		if (ujcrb7 == jtemp)
+			break;
+	}
+}
+
+static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
+{
+	struct ast_i2c_chan *i2c;
+	int ret;
+
+	i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
+	if (!i2c)
+		return NULL;
+
+	i2c->adapter.owner = THIS_MODULE;
+	i2c->adapter.class = I2C_CLASS_DDC;
+	i2c->adapter.dev.parent = &dev->pdev->dev;
+	i2c->dev = dev;
+	i2c_set_adapdata(&i2c->adapter, i2c);
+	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
+		 "AST i2c bit bus");
+	i2c->adapter.algo_data = &i2c->bit;
+
+	i2c->bit.udelay = 20;
+	i2c->bit.timeout = 2;
+	i2c->bit.data = i2c;
+	i2c->bit.setsda = ast_i2c_setsda;
+	i2c->bit.setscl = ast_i2c_setscl;
+	i2c->bit.getsda = ast_i2c_getsda;
+	i2c->bit.getscl = ast_i2c_getscl;
+	ret = i2c_bit_add_bus(&i2c->adapter);
+	if (ret) {
+		drm_err(dev, "Failed to register bit i2c\n");
+		goto out_free;
+	}
+
+	return i2c;
+out_free:
+	kfree(i2c);
+	return NULL;
+}
+
+static void ast_i2c_destroy(struct ast_i2c_chan *i2c)
+{
+	if (!i2c)
+		return;
+	i2c_del_adapter(&i2c->adapter);
+	kfree(i2c);
+}
+
 /*
  * Primary plane
  */
@@ -1146,124 +1268,3 @@ int ast_mode_config_init(struct ast_private *ast)
 
 	return 0;
 }
-
-static int get_clock(void *i2c_priv)
-{
-	struct ast_i2c_chan *i2c = i2c_priv;
-	struct ast_private *ast = to_ast_private(i2c->dev);
-	uint32_t val, val2, count, pass;
-
-	count = 0;
-	pass = 0;
-	val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
-	do {
-		val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
-		if (val == val2) {
-			pass++;
-		} else {
-			pass = 0;
-			val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
-		}
-	} while ((pass < 5) && (count++ < 0x10000));
-
-	return val & 1 ? 1 : 0;
-}
-
-static int get_data(void *i2c_priv)
-{
-	struct ast_i2c_chan *i2c = i2c_priv;
-	struct ast_private *ast = to_ast_private(i2c->dev);
-	uint32_t val, val2, count, pass;
-
-	count = 0;
-	pass = 0;
-	val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
-	do {
-		val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
-		if (val == val2) {
-			pass++;
-		} else {
-			pass = 0;
-			val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
-		}
-	} while ((pass < 5) && (count++ < 0x10000));
-
-	return val & 1 ? 1 : 0;
-}
-
-static void set_clock(void *i2c_priv, int clock)
-{
-	struct ast_i2c_chan *i2c = i2c_priv;
-	struct ast_private *ast = to_ast_private(i2c->dev);
-	int i;
-	u8 ujcrb7, jtemp;
-
-	for (i = 0; i < 0x10000; i++) {
-		ujcrb7 = ((clock & 0x01) ? 0 : 1);
-		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7);
-		jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01);
-		if (ujcrb7 == jtemp)
-			break;
-	}
-}
-
-static void set_data(void *i2c_priv, int data)
-{
-	struct ast_i2c_chan *i2c = i2c_priv;
-	struct ast_private *ast = to_ast_private(i2c->dev);
-	int i;
-	u8 ujcrb7, jtemp;
-
-	for (i = 0; i < 0x10000; i++) {
-		ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;
-		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7);
-		jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04);
-		if (ujcrb7 == jtemp)
-			break;
-	}
-}
-
-static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
-{
-	struct ast_i2c_chan *i2c;
-	int ret;
-
-	i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
-	if (!i2c)
-		return NULL;
-
-	i2c->adapter.owner = THIS_MODULE;
-	i2c->adapter.class = I2C_CLASS_DDC;
-	i2c->adapter.dev.parent = &dev->pdev->dev;
-	i2c->dev = dev;
-	i2c_set_adapdata(&i2c->adapter, i2c);
-	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
-		 "AST i2c bit bus");
-	i2c->adapter.algo_data = &i2c->bit;
-
-	i2c->bit.udelay = 20;
-	i2c->bit.timeout = 2;
-	i2c->bit.data = i2c;
-	i2c->bit.setsda = set_data;
-	i2c->bit.setscl = set_clock;
-	i2c->bit.getsda = get_data;
-	i2c->bit.getscl = get_clock;
-	ret = i2c_bit_add_bus(&i2c->adapter);
-	if (ret) {
-		drm_err(dev, "Failed to register bit i2c\n");
-		goto out_free;
-	}
-
-	return i2c;
-out_free:
-	kfree(i2c);
-	return NULL;
-}
-
-static void ast_i2c_destroy(struct ast_i2c_chan *i2c)
-{
-	if (!i2c)
-		return;
-	i2c_del_adapter(&i2c->adapter);
-	kfree(i2c);
-}
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/13] drm/ast: Move I2C code within ast_mode.c

[Sam Ravnborg]

Hi Thomas.

A few comments related to the code - but not to this patch and
not to this patch-set.
But I noticed so here goes.

	Sam

On Tue, Jul 28, 2020 at 09:44:13AM +0200, Thomas Zimmermann wrote:
> The I2C support feels slammed down to the end of ast_mode.c. Improve
> this by moving the code before it's callers, remove the declarations,
> rename the callbacks to match I2C's get/set sda/scl convention, and
> prefix all functions with ast_. No functional changes have been made.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/ast/ast_mode.c | 249 +++++++++++++++++----------------
>  1 file changed, 125 insertions(+), 124 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 154cd877d9d1..19f1dfc8e9e0 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -46,9 +46,6 @@
>  #include "ast_drv.h"
>  #include "ast_tables.h"
>  
> -static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
> -static void ast_i2c_destroy(struct ast_i2c_chan *i2c);
> -
>  static inline void ast_load_palette_index(struct ast_private *ast,
>  				     u8 index, u8 red, u8 green,
>  				     u8 blue)
> @@ -514,6 +511,131 @@ static void ast_set_start_address_crt1(struct ast_private *ast,
>  
>  }
>  
> +/*
> + * I2C
> + */
> +
> +static int ast_i2c_getscl(void *i2c_priv)
> +{
> +	struct ast_i2c_chan *i2c = i2c_priv;
> +	struct ast_private *ast = to_ast_private(i2c->dev);
> +	uint32_t val, val2, count, pass;
s/uint32_t/u32

> +
> +	count = 0;
> +	pass = 0;
> +	val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
So val is a bool - but anyway an int is used.

> +	do {
> +		val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
Likewise for val2.

> +		if (val == val2) {
> +			pass++;
> +		} else {
> +			pass = 0;
> +			val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
> +		}
> +	} while ((pass < 5) && (count++ < 0x10000));
> +
> +	return val & 1 ? 1 : 0;
bool to int conversion could do the trick here.

> +}
> +
> +static int ast_i2c_getsda(void *i2c_priv)
> +{
> +	struct ast_i2c_chan *i2c = i2c_priv;
> +	struct ast_private *ast = to_ast_private(i2c->dev);
> +	uint32_t val, val2, count, pass;
> +
> +	count = 0;
> +	pass = 0;
> +	val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
> +	do {
> +		val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
> +		if (val == val2) {
> +			pass++;
> +		} else {
> +			pass = 0;
> +			val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
> +		}
> +	} while ((pass < 5) && (count++ < 0x10000));
> +
> +	return val & 1 ? 1 : 0;
> +}
> +
> +static void ast_i2c_setscl(void *i2c_priv, int clock)
> +{
> +	struct ast_i2c_chan *i2c = i2c_priv;
> +	struct ast_private *ast = to_ast_private(i2c->dev);
> +	int i;
> +	u8 ujcrb7, jtemp;
And now u8 is used for registers.
Maybe because ast_get_index_reg_mask() returns uint8_t.

So for consistentcy do the uint8_t => u8 etc. conversion first.

> +
> +	for (i = 0; i < 0x10000; i++) {
> +		ujcrb7 = ((clock & 0x01) ? 0 : 1);
> +		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7);
> +		jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01);
> +		if (ujcrb7 == jtemp)
> +			break;
> +	}
> +}
> +
> +static void ast_i2c_setsda(void *i2c_priv, int data)
> +{
> +	struct ast_i2c_chan *i2c = i2c_priv;
> +	struct ast_private *ast = to_ast_private(i2c->dev);
> +	int i;
> +	u8 ujcrb7, jtemp;
> +
> +	for (i = 0; i < 0x10000; i++) {
> +		ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;
> +		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7);
> +		jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04);
> +		if (ujcrb7 == jtemp)
> +			break;
> +	}
> +}
> +
> +static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
> +{
> +	struct ast_i2c_chan *i2c;
> +	int ret;
> +
> +	i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
> +	if (!i2c)
> +		return NULL;
> +
> +	i2c->adapter.owner = THIS_MODULE;
> +	i2c->adapter.class = I2C_CLASS_DDC;
> +	i2c->adapter.dev.parent = &dev->pdev->dev;
> +	i2c->dev = dev;
> +	i2c_set_adapdata(&i2c->adapter, i2c);
If ast_private * was passed here and not i2c.
Then the implementation of ast_i2c_* could be a tad simpler - no
upclassing needed.
And then you could drop the drm_device field.
(And would need to invent another way to check if i2c is initialized).


> +	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
> +		 "AST i2c bit bus");
> +	i2c->adapter.algo_data = &i2c->bit;
> +
> +	i2c->bit.udelay = 20;
> +	i2c->bit.timeout = 2;
> +	i2c->bit.data = i2c;
> +	i2c->bit.setsda = ast_i2c_setsda;
> +	i2c->bit.setscl = ast_i2c_setscl;
> +	i2c->bit.getsda = ast_i2c_getsda;
> +	i2c->bit.getscl = ast_i2c_getscl;
> +	ret = i2c_bit_add_bus(&i2c->adapter);
> +	if (ret) {
> +		drm_err(dev, "Failed to register bit i2c\n");
> +		goto out_free;
> +	}
> +
> +	return i2c;
> +out_free:
> +	kfree(i2c);
> +	return NULL;
> +}
> +
> +static void ast_i2c_destroy(struct ast_i2c_chan *i2c)
> +{
> +	if (!i2c)
> +		return;
> +	i2c_del_adapter(&i2c->adapter);
> +	kfree(i2c);
> +}
> +
>  /*
>   * Primary plane
>   */
> @@ -1146,124 +1268,3 @@ int ast_mode_config_init(struct ast_private *ast)
>  
>  	return 0;
>  }
> -
> -static int get_clock(void *i2c_priv)
> -{
> -	struct ast_i2c_chan *i2c = i2c_priv;
> -	struct ast_private *ast = to_ast_private(i2c->dev);
> -	uint32_t val, val2, count, pass;
> -
> -	count = 0;
> -	pass = 0;
> -	val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
> -	do {
> -		val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
> -		if (val == val2) {
> -			pass++;
> -		} else {
> -			pass = 0;
> -			val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
> -		}
> -	} while ((pass < 5) && (count++ < 0x10000));
> -
> -	return val & 1 ? 1 : 0;
> -}
> -
> -static int get_data(void *i2c_priv)
> -{
> -	struct ast_i2c_chan *i2c = i2c_priv;
> -	struct ast_private *ast = to_ast_private(i2c->dev);
> -	uint32_t val, val2, count, pass;
> -
> -	count = 0;
> -	pass = 0;
> -	val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
> -	do {
> -		val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
> -		if (val == val2) {
> -			pass++;
> -		} else {
> -			pass = 0;
> -			val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
> -		}
> -	} while ((pass < 5) && (count++ < 0x10000));
> -
> -	return val & 1 ? 1 : 0;
> -}
> -
> -static void set_clock(void *i2c_priv, int clock)
> -{
> -	struct ast_i2c_chan *i2c = i2c_priv;
> -	struct ast_private *ast = to_ast_private(i2c->dev);
> -	int i;
> -	u8 ujcrb7, jtemp;
> -
> -	for (i = 0; i < 0x10000; i++) {
> -		ujcrb7 = ((clock & 0x01) ? 0 : 1);
> -		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7);
> -		jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01);
> -		if (ujcrb7 == jtemp)
> -			break;
> -	}
> -}
> -
> -static void set_data(void *i2c_priv, int data)
> -{
> -	struct ast_i2c_chan *i2c = i2c_priv;
> -	struct ast_private *ast = to_ast_private(i2c->dev);
> -	int i;
> -	u8 ujcrb7, jtemp;
> -
> -	for (i = 0; i < 0x10000; i++) {
> -		ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;
> -		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7);
> -		jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04);
> -		if (ujcrb7 == jtemp)
> -			break;
> -	}
> -}
> -
> -static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
> -{
> -	struct ast_i2c_chan *i2c;
> -	int ret;
> -
> -	i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
> -	if (!i2c)
> -		return NULL;
> -
> -	i2c->adapter.owner = THIS_MODULE;
> -	i2c->adapter.class = I2C_CLASS_DDC;
> -	i2c->adapter.dev.parent = &dev->pdev->dev;
> -	i2c->dev = dev;
> -	i2c_set_adapdata(&i2c->adapter, i2c);
> -	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
> -		 "AST i2c bit bus");
> -	i2c->adapter.algo_data = &i2c->bit;
> -
> -	i2c->bit.udelay = 20;
> -	i2c->bit.timeout = 2;
> -	i2c->bit.data = i2c;
> -	i2c->bit.setsda = set_data;
> -	i2c->bit.setscl = set_clock;
> -	i2c->bit.getsda = get_data;
> -	i2c->bit.getscl = get_clock;
> -	ret = i2c_bit_add_bus(&i2c->adapter);
> -	if (ret) {
> -		drm_err(dev, "Failed to register bit i2c\n");
> -		goto out_free;
> -	}
> -
> -	return i2c;
> -out_free:
> -	kfree(i2c);
> -	return NULL;
> -}
> -
> -static void ast_i2c_destroy(struct ast_i2c_chan *i2c)
> -{
> -	if (!i2c)
> -		return;
> -	i2c_del_adapter(&i2c->adapter);
> -	kfree(i2c);
> -}
> -- 
> 2.27.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/13] drm/ast: Move I2C code within ast_mode.c

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 10085 bytes --]

Hi

Am 28.07.20 um 20:04 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> A few comments related to the code - but not to this patch and
> not to this patch-set.
> But I noticed so here goes.

You're right on these points. But it's worth a separate patchset. I
really just move code around here.

Best regards
Thomas

> 
> 	Sam
> 
> On Tue, Jul 28, 2020 at 09:44:13AM +0200, Thomas Zimmermann wrote:
>> The I2C support feels slammed down to the end of ast_mode.c. Improve
>> this by moving the code before it's callers, remove the declarations,
>> rename the callbacks to match I2C's get/set sda/scl convention, and
>> prefix all functions with ast_. No functional changes have been made.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/ast/ast_mode.c | 249 +++++++++++++++++----------------
>>  1 file changed, 125 insertions(+), 124 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index 154cd877d9d1..19f1dfc8e9e0 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -46,9 +46,6 @@
>>  #include "ast_drv.h"
>>  #include "ast_tables.h"
>>  
>> -static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
>> -static void ast_i2c_destroy(struct ast_i2c_chan *i2c);
>> -
>>  static inline void ast_load_palette_index(struct ast_private *ast,
>>  				     u8 index, u8 red, u8 green,
>>  				     u8 blue)
>> @@ -514,6 +511,131 @@ static void ast_set_start_address_crt1(struct ast_private *ast,
>>  
>>  }
>>  
>> +/*
>> + * I2C
>> + */
>> +
>> +static int ast_i2c_getscl(void *i2c_priv)
>> +{
>> +	struct ast_i2c_chan *i2c = i2c_priv;
>> +	struct ast_private *ast = to_ast_private(i2c->dev);
>> +	uint32_t val, val2, count, pass;
> s/uint32_t/u32
> 
>> +
>> +	count = 0;
>> +	pass = 0;
>> +	val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
> So val is a bool - but anyway an int is used.
> 
>> +	do {
>> +		val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
> Likewise for val2.
> 
>> +		if (val == val2) {
>> +			pass++;
>> +		} else {
>> +			pass = 0;
>> +			val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
>> +		}
>> +	} while ((pass < 5) && (count++ < 0x10000));
>> +
>> +	return val & 1 ? 1 : 0;
> bool to int conversion could do the trick here.
> 
>> +}
>> +
>> +static int ast_i2c_getsda(void *i2c_priv)
>> +{
>> +	struct ast_i2c_chan *i2c = i2c_priv;
>> +	struct ast_private *ast = to_ast_private(i2c->dev);
>> +	uint32_t val, val2, count, pass;
>> +
>> +	count = 0;
>> +	pass = 0;
>> +	val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
>> +	do {
>> +		val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
>> +		if (val == val2) {
>> +			pass++;
>> +		} else {
>> +			pass = 0;
>> +			val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
>> +		}
>> +	} while ((pass < 5) && (count++ < 0x10000));
>> +
>> +	return val & 1 ? 1 : 0;
>> +}
>> +
>> +static void ast_i2c_setscl(void *i2c_priv, int clock)
>> +{
>> +	struct ast_i2c_chan *i2c = i2c_priv;
>> +	struct ast_private *ast = to_ast_private(i2c->dev);
>> +	int i;
>> +	u8 ujcrb7, jtemp;
> And now u8 is used for registers.
> Maybe because ast_get_index_reg_mask() returns uint8_t.
> 
> So for consistentcy do the uint8_t => u8 etc. conversion first.
> 
>> +
>> +	for (i = 0; i < 0x10000; i++) {
>> +		ujcrb7 = ((clock & 0x01) ? 0 : 1);
>> +		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7);
>> +		jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01);
>> +		if (ujcrb7 == jtemp)
>> +			break;
>> +	}
>> +}
>> +
>> +static void ast_i2c_setsda(void *i2c_priv, int data)
>> +{
>> +	struct ast_i2c_chan *i2c = i2c_priv;
>> +	struct ast_private *ast = to_ast_private(i2c->dev);
>> +	int i;
>> +	u8 ujcrb7, jtemp;
>> +
>> +	for (i = 0; i < 0x10000; i++) {
>> +		ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;
>> +		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7);
>> +		jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04);
>> +		if (ujcrb7 == jtemp)
>> +			break;
>> +	}
>> +}
>> +
>> +static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
>> +{
>> +	struct ast_i2c_chan *i2c;
>> +	int ret;
>> +
>> +	i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
>> +	if (!i2c)
>> +		return NULL;
>> +
>> +	i2c->adapter.owner = THIS_MODULE;
>> +	i2c->adapter.class = I2C_CLASS_DDC;
>> +	i2c->adapter.dev.parent = &dev->pdev->dev;
>> +	i2c->dev = dev;
>> +	i2c_set_adapdata(&i2c->adapter, i2c);
> If ast_private * was passed here and not i2c.
> Then the implementation of ast_i2c_* could be a tad simpler - no
> upclassing needed.
> And then you could drop the drm_device field.
> (And would need to invent another way to check if i2c is initialized).
> 
> 
>> +	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
>> +		 "AST i2c bit bus");
>> +	i2c->adapter.algo_data = &i2c->bit;
>> +
>> +	i2c->bit.udelay = 20;
>> +	i2c->bit.timeout = 2;
>> +	i2c->bit.data = i2c;
>> +	i2c->bit.setsda = ast_i2c_setsda;
>> +	i2c->bit.setscl = ast_i2c_setscl;
>> +	i2c->bit.getsda = ast_i2c_getsda;
>> +	i2c->bit.getscl = ast_i2c_getscl;
>> +	ret = i2c_bit_add_bus(&i2c->adapter);
>> +	if (ret) {
>> +		drm_err(dev, "Failed to register bit i2c\n");
>> +		goto out_free;
>> +	}
>> +
>> +	return i2c;
>> +out_free:
>> +	kfree(i2c);
>> +	return NULL;
>> +}
>> +
>> +static void ast_i2c_destroy(struct ast_i2c_chan *i2c)
>> +{
>> +	if (!i2c)
>> +		return;
>> +	i2c_del_adapter(&i2c->adapter);
>> +	kfree(i2c);
>> +}
>> +
>>  /*
>>   * Primary plane
>>   */
>> @@ -1146,124 +1268,3 @@ int ast_mode_config_init(struct ast_private *ast)
>>  
>>  	return 0;
>>  }
>> -
>> -static int get_clock(void *i2c_priv)
>> -{
>> -	struct ast_i2c_chan *i2c = i2c_priv;
>> -	struct ast_private *ast = to_ast_private(i2c->dev);
>> -	uint32_t val, val2, count, pass;
>> -
>> -	count = 0;
>> -	pass = 0;
>> -	val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
>> -	do {
>> -		val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
>> -		if (val == val2) {
>> -			pass++;
>> -		} else {
>> -			pass = 0;
>> -			val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x10) >> 4) & 0x01;
>> -		}
>> -	} while ((pass < 5) && (count++ < 0x10000));
>> -
>> -	return val & 1 ? 1 : 0;
>> -}
>> -
>> -static int get_data(void *i2c_priv)
>> -{
>> -	struct ast_i2c_chan *i2c = i2c_priv;
>> -	struct ast_private *ast = to_ast_private(i2c->dev);
>> -	uint32_t val, val2, count, pass;
>> -
>> -	count = 0;
>> -	pass = 0;
>> -	val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
>> -	do {
>> -		val2 = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
>> -		if (val == val2) {
>> -			pass++;
>> -		} else {
>> -			pass = 0;
>> -			val = (ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x20) >> 5) & 0x01;
>> -		}
>> -	} while ((pass < 5) && (count++ < 0x10000));
>> -
>> -	return val & 1 ? 1 : 0;
>> -}
>> -
>> -static void set_clock(void *i2c_priv, int clock)
>> -{
>> -	struct ast_i2c_chan *i2c = i2c_priv;
>> -	struct ast_private *ast = to_ast_private(i2c->dev);
>> -	int i;
>> -	u8 ujcrb7, jtemp;
>> -
>> -	for (i = 0; i < 0x10000; i++) {
>> -		ujcrb7 = ((clock & 0x01) ? 0 : 1);
>> -		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf4, ujcrb7);
>> -		jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x01);
>> -		if (ujcrb7 == jtemp)
>> -			break;
>> -	}
>> -}
>> -
>> -static void set_data(void *i2c_priv, int data)
>> -{
>> -	struct ast_i2c_chan *i2c = i2c_priv;
>> -	struct ast_private *ast = to_ast_private(i2c->dev);
>> -	int i;
>> -	u8 ujcrb7, jtemp;
>> -
>> -	for (i = 0; i < 0x10000; i++) {
>> -		ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;
>> -		ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0xf1, ujcrb7);
>> -		jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xb7, 0x04);
>> -		if (ujcrb7 == jtemp)
>> -			break;
>> -	}
>> -}
>> -
>> -static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
>> -{
>> -	struct ast_i2c_chan *i2c;
>> -	int ret;
>> -
>> -	i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
>> -	if (!i2c)
>> -		return NULL;
>> -
>> -	i2c->adapter.owner = THIS_MODULE;
>> -	i2c->adapter.class = I2C_CLASS_DDC;
>> -	i2c->adapter.dev.parent = &dev->pdev->dev;
>> -	i2c->dev = dev;
>> -	i2c_set_adapdata(&i2c->adapter, i2c);
>> -	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
>> -		 "AST i2c bit bus");
>> -	i2c->adapter.algo_data = &i2c->bit;
>> -
>> -	i2c->bit.udelay = 20;
>> -	i2c->bit.timeout = 2;
>> -	i2c->bit.data = i2c;
>> -	i2c->bit.setsda = set_data;
>> -	i2c->bit.setscl = set_clock;
>> -	i2c->bit.getsda = get_data;
>> -	i2c->bit.getscl = get_clock;
>> -	ret = i2c_bit_add_bus(&i2c->adapter);
>> -	if (ret) {
>> -		drm_err(dev, "Failed to register bit i2c\n");
>> -		goto out_free;
>> -	}
>> -
>> -	return i2c;
>> -out_free:
>> -	kfree(i2c);
>> -	return NULL;
>> -}
>> -
>> -static void ast_i2c_destroy(struct ast_i2c_chan *i2c)
>> -{
>> -	if (!i2c)
>> -		return;
>> -	i2c_del_adapter(&i2c->adapter);
>> -	kfree(i2c);
>> -}
>> -- 
>> 2.27.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/13] drm/ast: Test if I2C support has been initialized

[Thomas Zimmermann]

The ast driver is supposed to work without I2C support. This is
tested by looking at the connector's i2c field being non-NULL.

After embedding the I2C structure in the connector, the i2c field
will not be a pointer. So change the test to look at the dev field
in struct ast_i2c_chan.

ast_get_modes() did not really test if I2C has been initialized, so
the patch adds this test as well.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 19f1dfc8e9e0..45be020afcad 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -603,7 +603,6 @@ static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 	i2c->adapter.owner = THIS_MODULE;
 	i2c->adapter.class = I2C_CLASS_DDC;
 	i2c->adapter.dev.parent = &dev->pdev->dev;
-	i2c->dev = dev;
 	i2c_set_adapdata(&i2c->adapter, i2c);
 	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
 		 "AST i2c bit bus");
@@ -622,17 +621,30 @@ static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 		goto out_free;
 	}
 
+	i2c->dev = dev; /* signals presence of I2C support */
+
 	return i2c;
 out_free:
 	kfree(i2c);
 	return NULL;
 }
 
-static void ast_i2c_destroy(struct ast_i2c_chan *i2c)
+static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
 {
-	if (!i2c)
+	return i2c && !!i2c->dev;
+}
+
+static void ast_i2c_fini(struct ast_i2c_chan *i2c)
+{
+	if (!ast_i2c_is_initialized(i2c))
 		return;
 	i2c_del_adapter(&i2c->adapter);
+	i2c->dev = NULL; /* clear to signal absence of I2C support */
+}
+
+static void ast_i2c_destroy(struct ast_i2c_chan *i2c)
+{
+	ast_i2c_fini(i2c);
 	kfree(i2c);
 }
 
@@ -1054,6 +1066,7 @@ static int ast_get_modes(struct drm_connector *connector)
 {
 	struct ast_connector *ast_connector = to_ast_connector(connector);
 	struct ast_private *ast = to_ast_private(connector->dev);
+	struct ast_i2c_chan *i2c = ast_connector->i2c;
 	struct edid *edid;
 	int ret;
 	bool flags = false;
@@ -1069,8 +1082,8 @@ static int ast_get_modes(struct drm_connector *connector)
 		else
 			kfree(edid);
 	}
-	if (!flags)
-		edid = drm_get_edid(connector, &ast_connector->i2c->adapter);
+	if (!flags && ast_i2c_is_initialized(i2c))
+		edid = drm_get_edid(connector, &i2c->adapter);
 	if (edid) {
 		drm_connector_update_edid_property(&ast_connector->base, edid);
 		ret = drm_add_edid_modes(connector, edid);
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/13] drm/ast: Test if I2C support has been initialized

[Sam Ravnborg]

Hi Thomas.

On Tue, Jul 28, 2020 at 09:44:14AM +0200, Thomas Zimmermann wrote:
> The ast driver is supposed to work without I2C support. This is
> tested by looking at the connector's i2c field being non-NULL.
> 
> After embedding the I2C structure in the connector, the i2c field
> will not be a pointer. So change the test to look at the dev field
> in struct ast_i2c_chan.
> 
> ast_get_modes() did not really test if I2C has been initialized, so
> the patch adds this test as well.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/ast/ast_mode.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 19f1dfc8e9e0..45be020afcad 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -603,7 +603,6 @@ static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
>  	i2c->adapter.owner = THIS_MODULE;
>  	i2c->adapter.class = I2C_CLASS_DDC;
>  	i2c->adapter.dev.parent = &dev->pdev->dev;
> -	i2c->dev = dev;
>  	i2c_set_adapdata(&i2c->adapter, i2c);
>  	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
>  		 "AST i2c bit bus");
> @@ -622,17 +621,30 @@ static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
>  		goto out_free;
>  	}
>  
> +	i2c->dev = dev; /* signals presence of I2C support */
> +
>  	return i2c;
>  out_free:
>  	kfree(i2c);
>  	return NULL;
>  }
>  
> -static void ast_i2c_destroy(struct ast_i2c_chan *i2c)
> +static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
>  {
> -	if (!i2c)
> +	return i2c && !!i2c->dev;
> +}
It seems pointless to convert the pointer to bool here ("!!").

> +
> +static void ast_i2c_fini(struct ast_i2c_chan *i2c)
> +{
> +	if (!ast_i2c_is_initialized(i2c))
>  		return;
Empty line after return?
>  	i2c_del_adapter(&i2c->adapter);
> +	i2c->dev = NULL; /* clear to signal absence of I2C support */
> +}
> +
> +static void ast_i2c_destroy(struct ast_i2c_chan *i2c)
> +{
> +	ast_i2c_fini(i2c);
>  	kfree(i2c);
>  }
>  
> @@ -1054,6 +1066,7 @@ static int ast_get_modes(struct drm_connector *connector)
>  {
>  	struct ast_connector *ast_connector = to_ast_connector(connector);
>  	struct ast_private *ast = to_ast_private(connector->dev);
> +	struct ast_i2c_chan *i2c = ast_connector->i2c;
>  	struct edid *edid;
>  	int ret;
>  	bool flags = false;
> @@ -1069,8 +1082,8 @@ static int ast_get_modes(struct drm_connector *connector)
>  		else
>  			kfree(edid);
>  	}
> -	if (!flags)
> -		edid = drm_get_edid(connector, &ast_connector->i2c->adapter);
> +	if (!flags && ast_i2c_is_initialized(i2c))
> +		edid = drm_get_edid(connector, &i2c->adapter);
>  	if (edid) {
>  		drm_connector_update_edid_property(&ast_connector->base, edid);
>  		ret = drm_add_edid_modes(connector, edid);
> -- 
> 2.27.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/13] drm/ast: Embed I2C fields in struct ast_connector

[Thomas Zimmermann]

With the I2C fields embedded in struct ast_connector, the related call
to kzalloc() can be removed.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  |  3 +--
 drivers/gpu/drm/ast/ast_mode.c | 33 ++++++++++-----------------------
 2 files changed, 11 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index e3a264ac7ee2..d303df568099 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -98,7 +98,6 @@ enum ast_tx_chip {
 #define AST_HWC_SIGNATURE_HOTSPOTX	0x14
 #define AST_HWC_SIGNATURE_HOTSPOTY	0x18
 
-
 struct ast_private {
 	struct drm_device *dev;
 
@@ -234,7 +233,7 @@ struct ast_i2c_chan {
 
 struct ast_connector {
 	struct drm_connector base;
-	struct ast_i2c_chan *i2c;
+	struct ast_i2c_chan i2c;
 };
 
 #define to_ast_connector(x) container_of(x, struct ast_connector, base)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 45be020afcad..f421a60d8a96 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -591,15 +591,10 @@ static void ast_i2c_setsda(void *i2c_priv, int data)
 	}
 }
 
-static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
+static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
 {
-	struct ast_i2c_chan *i2c;
 	int ret;
 
-	i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
-	if (!i2c)
-		return NULL;
-
 	i2c->adapter.owner = THIS_MODULE;
 	i2c->adapter.class = I2C_CLASS_DDC;
 	i2c->adapter.dev.parent = &dev->pdev->dev;
@@ -618,20 +613,17 @@ static struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 	ret = i2c_bit_add_bus(&i2c->adapter);
 	if (ret) {
 		drm_err(dev, "Failed to register bit i2c\n");
-		goto out_free;
+		return ret;
 	}
 
 	i2c->dev = dev; /* signals presence of I2C support */
 
-	return i2c;
-out_free:
-	kfree(i2c);
-	return NULL;
+	return 0;
 }
 
 static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
 {
-	return i2c && !!i2c->dev;
+	return !!i2c->dev;
 }
 
 static void ast_i2c_fini(struct ast_i2c_chan *i2c)
@@ -642,12 +634,6 @@ static void ast_i2c_fini(struct ast_i2c_chan *i2c)
 	i2c->dev = NULL; /* clear to signal absence of I2C support */
 }
 
-static void ast_i2c_destroy(struct ast_i2c_chan *i2c)
-{
-	ast_i2c_fini(i2c);
-	kfree(i2c);
-}
-
 /*
  * Primary plane
  */
@@ -1066,7 +1052,7 @@ static int ast_get_modes(struct drm_connector *connector)
 {
 	struct ast_connector *ast_connector = to_ast_connector(connector);
 	struct ast_private *ast = to_ast_private(connector->dev);
-	struct ast_i2c_chan *i2c = ast_connector->i2c;
+	struct ast_i2c_chan *i2c = &ast_connector->i2c;
 	struct edid *edid;
 	int ret;
 	bool flags = false;
@@ -1154,7 +1140,7 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
 static void ast_connector_destroy(struct drm_connector *connector)
 {
 	struct ast_connector *ast_connector = to_ast_connector(connector);
-	ast_i2c_destroy(ast_connector->i2c);
+	ast_i2c_fini(&ast_connector->i2c);
 	drm_connector_cleanup(connector);
 	kfree(connector);
 }
@@ -1177,20 +1163,21 @@ static int ast_connector_init(struct drm_device *dev)
 	struct ast_connector *ast_connector;
 	struct drm_connector *connector;
 	struct drm_encoder *encoder;
+	int ret;
 
 	ast_connector = kzalloc(sizeof(struct ast_connector), GFP_KERNEL);
 	if (!ast_connector)
 		return -ENOMEM;
 
 	connector = &ast_connector->base;
-	ast_connector->i2c = ast_i2c_create(dev);
-	if (!ast_connector->i2c)
+	ret = ast_i2c_init(&ast_connector->i2c, dev);
+	if (ret)
 		drm_err(dev, "failed to add ddc bus for connector\n");
 
 	drm_connector_init_with_ddc(dev, connector,
 				    &ast_connector_funcs,
 				    DRM_MODE_CONNECTOR_VGA,
-				    &ast_connector->i2c->adapter);
+				    &ast_connector->i2c.adapter);
 
 	drm_connector_helper_add(connector, &ast_connector_helper_funcs);
 
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/13] drm/ast: Managed release of I2C adapter

[Thomas Zimmermann]

Managed releases of the device's I2C adapter simplify the connector's
release.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index f421a60d8a96..27eb49bd12b3 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -39,6 +39,7 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_vram_helper.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
@@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data)
 	}
 }
 
+static void ast_i2c_release(struct drm_device *dev, void *data)
+{
+	struct ast_i2c_chan *i2c = data;
+
+	i2c_del_adapter(&i2c->adapter);
+	i2c->dev = NULL; /* clear to signal absence of I2C support */
+}
+
 static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
 {
 	int ret;
@@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
 
 	i2c->dev = dev; /* signals presence of I2C support */
 
-	return 0;
+	return drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
 }
 
 static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
@@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
 	return !!i2c->dev;
 }
 
-static void ast_i2c_fini(struct ast_i2c_chan *i2c)
-{
-	if (!ast_i2c_is_initialized(i2c))
-		return;
-	i2c_del_adapter(&i2c->adapter);
-	i2c->dev = NULL; /* clear to signal absence of I2C support */
-}
-
 /*
  * Primary plane
  */
@@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
 
 static void ast_connector_destroy(struct drm_connector *connector)
 {
-	struct ast_connector *ast_connector = to_ast_connector(connector);
-	ast_i2c_fini(&ast_connector->i2c);
 	drm_connector_cleanup(connector);
 	kfree(connector);
 }
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/13] drm/ast: Managed release of I2C adapter

[daniel]

On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote:
> Managed releases of the device's I2C adapter simplify the connector's
> release.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

I think this breaks bisect, since at this point the release callback is
called when the connector is already gone. At the end of the series it's
fine again though.

I've done a very cursory reading of your series to look for high-level
issues, I think overall reasonable. On the series:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

But maybe you want to polish a bit more, up to you.
-Daniel

> ---
>  drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index f421a60d8a96..27eb49bd12b3 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -39,6 +39,7 @@
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_gem_vram_helper.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_simple_kms_helper.h>
> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data)
>  	}
>  }
>  
> +static void ast_i2c_release(struct drm_device *dev, void *data)
> +{
> +	struct ast_i2c_chan *i2c = data;
> +
> +	i2c_del_adapter(&i2c->adapter);
> +	i2c->dev = NULL; /* clear to signal absence of I2C support */
> +}
> +
>  static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
>  {
>  	int ret;
> @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
>  
>  	i2c->dev = dev; /* signals presence of I2C support */
>  
> -	return 0;
> +	return drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
>  }
>  
>  static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
> @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
>  	return !!i2c->dev;
>  }
>  
> -static void ast_i2c_fini(struct ast_i2c_chan *i2c)
> -{
> -	if (!ast_i2c_is_initialized(i2c))
> -		return;
> -	i2c_del_adapter(&i2c->adapter);
> -	i2c->dev = NULL; /* clear to signal absence of I2C support */
> -}
> -
>  /*
>   * Primary plane
>   */
> @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
>  
>  static void ast_connector_destroy(struct drm_connector *connector)
>  {
> -	struct ast_connector *ast_connector = to_ast_connector(connector);
> -	ast_i2c_fini(&ast_connector->i2c);
>  	drm_connector_cleanup(connector);
>  	kfree(connector);
>  }
> -- 
> 2.27.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/13] drm/ast: Managed release of I2C adapter

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3258 bytes --]

Hi

Am 28.07.20 um 11:23 schrieb daniel@ffwll.ch:
> On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote:
>> Managed releases of the device's I2C adapter simplify the connector's
>> release.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> I think this breaks bisect, since at this point the release callback is
> called when the connector is already gone. At the end of the series it's
> fine again though.
> 
> I've done a very cursory reading of your series to look for high-level
> issues, I think overall reasonable. On the series:
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> But maybe you want to polish a bit more, up to you.

Thanks. I'll address your points and wait a bit longer. Usually Sam has
a number of good comments as well.

Best regards
Thomas

> -Daniel
> 
>> ---
>>  drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++-----------
>>  1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index f421a60d8a96..27eb49bd12b3 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -39,6 +39,7 @@
>>  #include <drm/drm_fourcc.h>
>>  #include <drm/drm_gem_framebuffer_helper.h>
>>  #include <drm/drm_gem_vram_helper.h>
>> +#include <drm/drm_managed.h>
>>  #include <drm/drm_plane_helper.h>
>>  #include <drm/drm_probe_helper.h>
>>  #include <drm/drm_simple_kms_helper.h>
>> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data)
>>  	}
>>  }
>>  
>> +static void ast_i2c_release(struct drm_device *dev, void *data)
>> +{
>> +	struct ast_i2c_chan *i2c = data;
>> +
>> +	i2c_del_adapter(&i2c->adapter);
>> +	i2c->dev = NULL; /* clear to signal absence of I2C support */
>> +}
>> +
>>  static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
>>  {
>>  	int ret;
>> @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
>>  
>>  	i2c->dev = dev; /* signals presence of I2C support */
>>  
>> -	return 0;
>> +	return drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
>>  }
>>  
>>  static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
>> @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
>>  	return !!i2c->dev;
>>  }
>>  
>> -static void ast_i2c_fini(struct ast_i2c_chan *i2c)
>> -{
>> -	if (!ast_i2c_is_initialized(i2c))
>> -		return;
>> -	i2c_del_adapter(&i2c->adapter);
>> -	i2c->dev = NULL; /* clear to signal absence of I2C support */
>> -}
>> -
>>  /*
>>   * Primary plane
>>   */
>> @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
>>  
>>  static void ast_connector_destroy(struct drm_connector *connector)
>>  {
>> -	struct ast_connector *ast_connector = to_ast_connector(connector);
>> -	ast_i2c_fini(&ast_connector->i2c);
>>  	drm_connector_cleanup(connector);
>>  	kfree(connector);
>>  }
>> -- 
>> 2.27.0
>>
> 

-- 
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/13] drm/ast: Managed release of I2C adapter

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3232 bytes --]

Hi

Am 28.07.20 um 11:23 schrieb daniel@ffwll.ch:
> On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote:
>> Managed releases of the device's I2C adapter simplify the connector's
>> release.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> I think this breaks bisect, since at this point the release callback is
> called when the connector is already gone. At the end of the series it's
> fine again though.

I'll move the auto-release of I2C to the end of the series. It should
work then.

Best regards
Thomas

> 
> I've done a very cursory reading of your series to look for high-level
> issues, I think overall reasonable. On the series:
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> But maybe you want to polish a bit more, up to you.
> -Daniel
> 
>> ---
>>  drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++-----------
>>  1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index f421a60d8a96..27eb49bd12b3 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -39,6 +39,7 @@
>>  #include <drm/drm_fourcc.h>
>>  #include <drm/drm_gem_framebuffer_helper.h>
>>  #include <drm/drm_gem_vram_helper.h>
>> +#include <drm/drm_managed.h>
>>  #include <drm/drm_plane_helper.h>
>>  #include <drm/drm_probe_helper.h>
>>  #include <drm/drm_simple_kms_helper.h>
>> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data)
>>  	}
>>  }
>>  
>> +static void ast_i2c_release(struct drm_device *dev, void *data)
>> +{
>> +	struct ast_i2c_chan *i2c = data;
>> +
>> +	i2c_del_adapter(&i2c->adapter);
>> +	i2c->dev = NULL; /* clear to signal absence of I2C support */
>> +}
>> +
>>  static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
>>  {
>>  	int ret;
>> @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
>>  
>>  	i2c->dev = dev; /* signals presence of I2C support */
>>  
>> -	return 0;
>> +	return drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
>>  }
>>  
>>  static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
>> @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
>>  	return !!i2c->dev;
>>  }
>>  
>> -static void ast_i2c_fini(struct ast_i2c_chan *i2c)
>> -{
>> -	if (!ast_i2c_is_initialized(i2c))
>> -		return;
>> -	i2c_del_adapter(&i2c->adapter);
>> -	i2c->dev = NULL; /* clear to signal absence of I2C support */
>> -}
>> -
>>  /*
>>   * Primary plane
>>   */
>> @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
>>  
>>  static void ast_connector_destroy(struct drm_connector *connector)
>>  {
>> -	struct ast_connector *ast_connector = to_ast_connector(connector);
>> -	ast_i2c_fini(&ast_connector->i2c);
>>  	drm_connector_cleanup(connector);
>>  	kfree(connector);
>>  }
>> -- 
>> 2.27.0
>>
> 

-- 
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/13] drm/ast: Managed release of I2C adapter

[Sam Ravnborg]

On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote:
> Managed releases of the device's I2C adapter simplify the connector's
> release.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index f421a60d8a96..27eb49bd12b3 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -39,6 +39,7 @@
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_gem_vram_helper.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_simple_kms_helper.h>
> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data)
>  	}
>  }
>  
> +static void ast_i2c_release(struct drm_device *dev, void *data)
> +{
> +	struct ast_i2c_chan *i2c = data;
> +
> +	i2c_del_adapter(&i2c->adapter);
> +	i2c->dev = NULL; /* clear to signal absence of I2C support */
> +}
> +
>  static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
>  {
>  	int ret;
> @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
>  
>  	i2c->dev = dev; /* signals presence of I2C support */
>  
> -	return 0;
> +	return drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
>  }
>  
>  static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
> @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
>  	return !!i2c->dev;
>  }
>  
> -static void ast_i2c_fini(struct ast_i2c_chan *i2c)
> -{
> -	if (!ast_i2c_is_initialized(i2c))
> -		return;
> -	i2c_del_adapter(&i2c->adapter);
> -	i2c->dev = NULL; /* clear to signal absence of I2C support */
> -}
The intro of ast_i2c_fini() and then removal again confuses me a little.
But end result looks simple so I guess thats what counts.

	Sam

> -
>  /*
>   * Primary plane
>   */
> @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
>  
>  static void ast_connector_destroy(struct drm_connector *connector)
>  {
> -	struct ast_connector *ast_connector = to_ast_connector(connector);
> -	ast_i2c_fini(&ast_connector->i2c);
>  	drm_connector_cleanup(connector);
>  	kfree(connector);
>  }
> -- 
> 2.27.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/13] drm/ast: Managed release of I2C adapter

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3382 bytes --]

Hi

Am 28.07.20 um 20:06 schrieb Sam Ravnborg:
> On Tue, Jul 28, 2020 at 09:44:16AM +0200, Thomas Zimmermann wrote:
>> Managed releases of the device's I2C adapter simplify the connector's
>> release.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/ast/ast_mode.c | 21 ++++++++++-----------
>>  1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index f421a60d8a96..27eb49bd12b3 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -39,6 +39,7 @@
>>  #include <drm/drm_fourcc.h>
>>  #include <drm/drm_gem_framebuffer_helper.h>
>>  #include <drm/drm_gem_vram_helper.h>
>> +#include <drm/drm_managed.h>
>>  #include <drm/drm_plane_helper.h>
>>  #include <drm/drm_probe_helper.h>
>>  #include <drm/drm_simple_kms_helper.h>
>> @@ -591,6 +592,14 @@ static void ast_i2c_setsda(void *i2c_priv, int data)
>>  	}
>>  }
>>  
>> +static void ast_i2c_release(struct drm_device *dev, void *data)
>> +{
>> +	struct ast_i2c_chan *i2c = data;
>> +
>> +	i2c_del_adapter(&i2c->adapter);
>> +	i2c->dev = NULL; /* clear to signal absence of I2C support */
>> +}
>> +
>>  static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
>>  {
>>  	int ret;
>> @@ -618,7 +627,7 @@ static int ast_i2c_init(struct ast_i2c_chan *i2c, struct drm_device *dev)
>>  
>>  	i2c->dev = dev; /* signals presence of I2C support */
>>  
>> -	return 0;
>> +	return drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
>>  }
>>  
>>  static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
>> @@ -626,14 +635,6 @@ static bool ast_i2c_is_initialized(struct ast_i2c_chan *i2c)
>>  	return !!i2c->dev;
>>  }
>>  
>> -static void ast_i2c_fini(struct ast_i2c_chan *i2c)
>> -{
>> -	if (!ast_i2c_is_initialized(i2c))
>> -		return;
>> -	i2c_del_adapter(&i2c->adapter);
>> -	i2c->dev = NULL; /* clear to signal absence of I2C support */
>> -}
> The intro of ast_i2c_fini() and then removal again confuses me a little.
> But end result looks simple so I guess thats what counts.

The intention was to separate _fini from _destroy and make the patch
series more readable. But looking at it now, this idea did not really
work. I guess, I'll drop _fini.

Best regards
Thomas

> 
> 	Sam
> 
>> -
>>  /*
>>   * Primary plane
>>   */
>> @@ -1139,8 +1140,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
>>  
>>  static void ast_connector_destroy(struct drm_connector *connector)
>>  {
>> -	struct ast_connector *ast_connector = to_ast_connector(connector);
>> -	ast_i2c_fini(&ast_connector->i2c);
>>  	drm_connector_cleanup(connector);
>>  	kfree(connector);
>>  }
>> -- 
>> 2.27.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/13] drm/ast: Embed CRTC and connector in struct ast_private

[Thomas Zimmermann]

Only single instances of CRTC and connector are supported per
device. Embed both in ast's structure and remove the individual
memory allocations. DRM's CRTC/connector cleanup helpers replace
the rsp. destroy functions in ast.

While at it, also convert to_ast_connector() to a function.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  | 34 ++++++++++++++-----------
 drivers/gpu/drm/ast/ast_mode.c | 46 ++++++++--------------------------
 2 files changed, 31 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index d303df568099..b401560e4e8f 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -98,6 +98,23 @@ enum ast_tx_chip {
 #define AST_HWC_SIGNATURE_HOTSPOTX	0x14
 #define AST_HWC_SIGNATURE_HOTSPOTY	0x18
 
+struct ast_i2c_chan {
+	struct i2c_adapter adapter;
+	struct drm_device *dev;
+	struct i2c_algo_bit_data bit;
+};
+
+struct ast_connector {
+	struct drm_connector base;
+	struct ast_i2c_chan i2c;
+};
+
+static inline struct ast_connector *
+to_ast_connector(struct drm_connector *connector)
+{
+	return container_of(connector, struct ast_connector, base);
+}
+
 struct ast_private {
 	struct drm_device *dev;
 
@@ -118,9 +135,11 @@ struct ast_private {
 		unsigned int next_index;
 	} cursor;
 
-	struct drm_encoder encoder;
 	struct drm_plane primary_plane;
 	struct drm_plane cursor_plane;
+	struct drm_crtc crtc;
+	struct drm_encoder encoder;
+	struct ast_connector connector;
 
 	bool support_wide_screen;
 	enum {
@@ -225,19 +244,6 @@ static inline void ast_open_key(struct ast_private *ast)
 
 #define AST_VIDMEM_DEFAULT_SIZE AST_VIDMEM_SIZE_8M
 
-struct ast_i2c_chan {
-	struct i2c_adapter adapter;
-	struct drm_device *dev;
-	struct i2c_algo_bit_data bit;
-};
-
-struct ast_connector {
-	struct drm_connector base;
-	struct ast_i2c_chan i2c;
-};
-
-#define to_ast_connector(x) container_of(x, struct ast_connector, base)
-
 struct ast_vbios_stdtable {
 	u8 misc;
 	u8 seq[4];
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 27eb49bd12b3..201313ab3e71 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -952,12 +952,6 @@ static void ast_crtc_reset(struct drm_crtc *crtc)
 	__drm_atomic_helper_crtc_reset(crtc, &ast_state->base);
 }
 
-static void ast_crtc_destroy(struct drm_crtc *crtc)
-{
-	drm_crtc_cleanup(crtc);
-	kfree(crtc);
-}
-
 static struct drm_crtc_state *
 ast_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
 {
@@ -993,7 +987,7 @@ static void ast_crtc_atomic_destroy_state(struct drm_crtc *crtc,
 static const struct drm_crtc_funcs ast_crtc_funcs = {
 	.reset = ast_crtc_reset,
 	.gamma_set = drm_atomic_helper_legacy_gamma_set,
-	.destroy = ast_crtc_destroy,
+	.destroy = drm_crtc_cleanup,
 	.set_config = drm_atomic_helper_set_config,
 	.page_flip = drm_atomic_helper_page_flip,
 	.atomic_duplicate_state = ast_crtc_atomic_duplicate_state,
@@ -1003,27 +997,19 @@ static const struct drm_crtc_funcs ast_crtc_funcs = {
 static int ast_crtc_init(struct drm_device *dev)
 {
 	struct ast_private *ast = to_ast_private(dev);
-	struct drm_crtc *crtc;
+	struct drm_crtc *crtc = &ast->crtc;
 	int ret;
 
-	crtc = kzalloc(sizeof(*crtc), GFP_KERNEL);
-	if (!crtc)
-		return -ENOMEM;
-
 	ret = drm_crtc_init_with_planes(dev, crtc, &ast->primary_plane,
 					&ast->cursor_plane, &ast_crtc_funcs,
 					NULL);
 	if (ret)
-		goto err_kfree;
+		return ret;
 
 	drm_mode_crtc_set_gamma_size(crtc, 256);
 	drm_crtc_helper_add(crtc, &ast_crtc_helper_funcs);
 
 	return 0;
-
-err_kfree:
-	kfree(crtc);
-	return ret;
 }
 
 /*
@@ -1138,12 +1124,6 @@ static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
 	return flags;
 }
 
-static void ast_connector_destroy(struct drm_connector *connector)
-{
-	drm_connector_cleanup(connector);
-	kfree(connector);
-}
-
 static const struct drm_connector_helper_funcs ast_connector_helper_funcs = {
 	.get_modes = ast_get_modes,
 	.mode_valid = ast_mode_valid,
@@ -1152,31 +1132,28 @@ static const struct drm_connector_helper_funcs ast_connector_helper_funcs = {
 static const struct drm_connector_funcs ast_connector_funcs = {
 	.reset = drm_atomic_helper_connector_reset,
 	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = ast_connector_destroy,
+	.destroy = drm_connector_cleanup,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static int ast_connector_init(struct drm_device *dev)
 {
-	struct ast_connector *ast_connector;
-	struct drm_connector *connector;
-	struct drm_encoder *encoder;
+	struct ast_private *ast = to_ast_private(dev);
+	struct ast_connector *ast_connector = &ast->connector;
+	struct drm_connector *connector = &ast_connector->base;
+	struct drm_encoder *encoder = &ast->encoder;
+	struct ast_i2c_chan *i2c = &ast_connector->i2c;
 	int ret;
 
-	ast_connector = kzalloc(sizeof(struct ast_connector), GFP_KERNEL);
-	if (!ast_connector)
-		return -ENOMEM;
-
-	connector = &ast_connector->base;
-	ret = ast_i2c_init(&ast_connector->i2c, dev);
+	ret = ast_i2c_init(i2c, dev);
 	if (ret)
 		drm_err(dev, "failed to add ddc bus for connector\n");
 
 	drm_connector_init_with_ddc(dev, connector,
 				    &ast_connector_funcs,
 				    DRM_MODE_CONNECTOR_VGA,
-				    &ast_connector->i2c.adapter);
+				    &i2c->adapter);
 
 	drm_connector_helper_add(connector, &ast_connector_helper_funcs);
 
@@ -1185,7 +1162,6 @@ static int ast_connector_init(struct drm_device *dev)
 
 	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
 
-	encoder = list_first_entry(&dev->mode_config.encoder_list, struct drm_encoder, head);
 	drm_connector_attach_encoder(connector, encoder);
 
 	return 0;
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/13] drm/ast: Separate DRM driver from PCI code

[Thomas Zimmermann]

Putting the DRM driver to the top of the file and the PCI code to the
bottom makes ast_drv.c more readable. While at it, the patch prefixes
file-scope variables with ast_.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.c | 59 ++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 0b58f7aee6b0..9d04f2b5225c 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -43,9 +43,33 @@ int ast_modeset = -1;
 MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
 module_param_named(modeset, ast_modeset, int, 0400);
 
-#define PCI_VENDOR_ASPEED 0x1a03
+/*
+ * DRM driver
+ */
+
+DEFINE_DRM_GEM_FOPS(ast_fops);
+
+static struct drm_driver ast_driver = {
+	.driver_features = DRIVER_ATOMIC |
+			   DRIVER_GEM |
+			   DRIVER_MODESET,
+
+	.fops = &ast_fops,
+	.name = DRIVER_NAME,
+	.desc = DRIVER_DESC,
+	.date = DRIVER_DATE,
+	.major = DRIVER_MAJOR,
+	.minor = DRIVER_MINOR,
+	.patchlevel = DRIVER_PATCHLEVEL,
 
-static struct drm_driver driver;
+	DRM_GEM_VRAM_DRIVER
+};
+
+/*
+ * PCI driver
+ */
+
+#define PCI_VENDOR_ASPEED 0x1a03
 
 #define AST_VGA_DEVICE(id, info) {		\
 	.class = PCI_BASE_CLASS_DISPLAY << 16,	\
@@ -56,13 +80,13 @@ static struct drm_driver driver;
 	.subdevice = PCI_ANY_ID,		\
 	.driver_data = (unsigned long) info }
 
-static const struct pci_device_id pciidlist[] = {
+static const struct pci_device_id ast_pciidlist[] = {
 	AST_VGA_DEVICE(PCI_CHIP_AST2000, NULL),
 	AST_VGA_DEVICE(PCI_CHIP_AST2100, NULL),
 	{0, 0, 0},
 };
 
-MODULE_DEVICE_TABLE(pci, pciidlist);
+MODULE_DEVICE_TABLE(pci, ast_pciidlist);
 
 static void ast_kick_out_firmware_fb(struct pci_dev *pdev)
 {
@@ -94,7 +118,7 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		return ret;
 
-	dev = drm_dev_alloc(&driver, &pdev->dev);
+	dev = drm_dev_alloc(&ast_driver, &pdev->dev);
 	if (IS_ERR(dev))
 		return  PTR_ERR(dev);
 
@@ -118,11 +142,9 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 err_drm_dev_put:
 	drm_dev_put(dev);
 	return ret;
-
 }
 
-static void
-ast_pci_remove(struct pci_dev *pdev)
+static void ast_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
@@ -217,30 +239,12 @@ static const struct dev_pm_ops ast_pm_ops = {
 
 static struct pci_driver ast_pci_driver = {
 	.name = DRIVER_NAME,
-	.id_table = pciidlist,
+	.id_table = ast_pciidlist,
 	.probe = ast_pci_probe,
 	.remove = ast_pci_remove,
 	.driver.pm = &ast_pm_ops,
 };
 
-DEFINE_DRM_GEM_FOPS(ast_fops);
-
-static struct drm_driver driver = {
-	.driver_features = DRIVER_ATOMIC |
-			   DRIVER_GEM |
-			   DRIVER_MODESET,
-
-	.fops = &ast_fops,
-	.name = DRIVER_NAME,
-	.desc = DRIVER_DESC,
-	.date = DRIVER_DATE,
-	.major = DRIVER_MAJOR,
-	.minor = DRIVER_MINOR,
-	.patchlevel = DRIVER_PATCHLEVEL,
-
-	DRM_GEM_VRAM_DRIVER
-};
-
 static int __init ast_init(void)
 {
 	if (vgacon_text_force() && ast_modeset == -1)
@@ -261,4 +265,3 @@ module_exit(ast_exit);
 MODULE_AUTHOR(DRIVER_AUTHOR);
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("GPL and additional rights");
-
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/13] drm/ast: Replace driver load/unload functions with device create/destroy

[Thomas Zimmermann]

The ast driver's load and unload functions are left-overs from when
struct drm_driver.load/unload was still in use. The PCI probe helper
allocated the DRM device and ran load to initialize it.

This patch replaces this code with device create and destroy. The
main difference is that the device's create function allocates the
DRM device and ast structures in the same place. This will be required
for switching ast to managed allocations.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.c  | 24 +++++++++++-------------
 drivers/gpu/drm/ast/ast_drv.h  |  6 ++++--
 drivers/gpu/drm/ast/ast_main.c | 24 ++++++++++++++++++------
 3 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 9d04f2b5225c..ad93c35b4cf7 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -109,6 +109,7 @@ static void ast_kick_out_firmware_fb(struct pci_dev *pdev)
 
 static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
+	struct ast_private *ast;
 	struct drm_device *dev;
 	int ret;
 
@@ -118,27 +119,23 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		return ret;
 
-	dev = drm_dev_alloc(&ast_driver, &pdev->dev);
-	if (IS_ERR(dev))
-		return  PTR_ERR(dev);
-
-	dev->pdev = pdev;
-	pci_set_drvdata(pdev, dev);
-
-	ret = ast_driver_load(dev, ent->driver_data);
-	if (ret)
+	ast = ast_device_create(&ast_driver, pdev, ent->driver_data);
+	if (IS_ERR(ast)) {
+		ret = PTR_ERR(ast);
 		goto err_drm_dev_put;
+	}
+	dev = ast->dev;
 
 	ret = drm_dev_register(dev, ent->driver_data);
 	if (ret)
-		goto err_ast_driver_unload;
+		goto err_ast_device_destroy;
 
 	drm_fbdev_generic_setup(dev, 32);
 
 	return 0;
 
-err_ast_driver_unload:
-	ast_driver_unload(dev);
+err_ast_device_destroy:
+	ast_device_destroy(ast);
 err_drm_dev_put:
 	drm_dev_put(dev);
 	return ret;
@@ -147,9 +144,10 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 static void ast_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
+	struct ast_private *ast = to_ast_private(dev);
 
 	drm_dev_unregister(dev);
-	ast_driver_unload(dev);
+	ast_device_destroy(ast);
 	drm_dev_put(dev);
 }
 
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index b401560e4e8f..f1aebc719d9e 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -159,8 +159,10 @@ static inline struct ast_private *to_ast_private(struct drm_device *dev)
 	return dev->dev_private;
 }
 
-int ast_driver_load(struct drm_device *dev, unsigned long flags);
-void ast_driver_unload(struct drm_device *dev);
+struct ast_private *ast_device_create(struct drm_driver *drv,
+				      struct pci_dev *pdev,
+				      unsigned long flags);
+void ast_device_destroy(struct ast_private *ast);
 
 #define AST_IO_AR_PORT_WRITE		(0x40)
 #define AST_IO_MISC_PORT_WRITE		(0x42)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index dd12b55d57a2..8d46166f8462 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -30,6 +30,7 @@
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_vram_helper.h>
 
@@ -378,15 +379,25 @@ static int ast_get_dram_info(struct drm_device *dev)
 	return 0;
 }
 
-int ast_driver_load(struct drm_device *dev, unsigned long flags)
+struct ast_private *ast_device_create(struct drm_driver *drv,
+				      struct pci_dev *pdev,
+				      unsigned long flags)
 {
+	struct drm_device *dev;
 	struct ast_private *ast;
 	bool need_post;
 	int ret = 0;
 
+	dev = drm_dev_alloc(drv, &pdev->dev);
+	if (IS_ERR(dev))
+		return ERR_CAST(dev);
+
+	dev->pdev = pdev;
+	pci_set_drvdata(pdev, dev);
+
 	ast = kzalloc(sizeof(struct ast_private), GFP_KERNEL);
 	if (!ast)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	dev->dev_private = ast;
 	ast->dev = dev;
@@ -435,16 +446,17 @@ int ast_driver_load(struct drm_device *dev, unsigned long flags)
 	if (ret)
 		goto out_free;
 
-	return 0;
+	return ast;
+
 out_free:
 	kfree(ast);
 	dev->dev_private = NULL;
-	return ret;
+	return ERR_PTR(ret);
 }
 
-void ast_driver_unload(struct drm_device *dev)
+void ast_device_destroy(struct ast_private *ast)
 {
-	struct ast_private *ast = to_ast_private(dev);
+	struct drm_device *dev = ast->dev;
 
 	/* enable standard VGA decode */
 	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 08/13] drm/ast: Replace struct_drm_device.dev_private with to_ast_private()

[Thomas Zimmermann]

The ast code still references dev_private in several place when looking
up the ast device structure. Convert the remaining locations to use
to_ast_private().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_cursor.c | 2 +-
 drivers/gpu/drm/ast/ast_mode.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
index acf0d23514e8..8d693c8b346f 100644
--- a/drivers/gpu/drm/ast/ast_cursor.c
+++ b/drivers/gpu/drm/ast/ast_cursor.c
@@ -47,7 +47,7 @@ static void ast_cursor_fini(struct ast_private *ast)
 
 static void ast_cursor_release(struct drm_device *dev, void *ptr)
 {
-	struct ast_private *ast = dev->dev_private;
+	struct ast_private *ast = to_ast_private(dev);
 
 	ast_cursor_fini(ast);
 }
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 201313ab3e71..01340b0e40f8 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -784,7 +784,7 @@ ast_cursor_plane_helper_atomic_update(struct drm_plane *plane,
 {
 	struct drm_plane_state *state = plane->state;
 	struct drm_framebuffer *fb = state->fb;
-	struct ast_private *ast = plane->dev->dev_private;
+	struct ast_private *ast = to_ast_private(plane->dev);
 	unsigned int offset_x, offset_y;
 
 	offset_x = AST_MAX_HWC_WIDTH - fb->width;
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/13] drm/ast: Don't use ast->dev if dev is available

[Thomas Zimmermann]

Several places in ast use ast->dev, when a dev pointer is already
available within the function. Remove the extra indirection. No
functional changes made.

This is just a small cleanup before embedding the DRM device instance
in struct ast_private.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 2 +-
 drivers/gpu/drm/ast/ast_post.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 01340b0e40f8..dceb11a320b2 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1196,7 +1196,7 @@ int ast_mode_config_init(struct ast_private *ast)
 	dev->mode_config.min_height = 0;
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.prefer_shadow = 1;
-	dev->mode_config.fb_base = pci_resource_start(ast->dev->pdev, 0);
+	dev->mode_config.fb_base = pci_resource_start(dev->pdev, 0);
 
 	if (ast->chip == AST2100 ||
 	    ast->chip == AST2200 ||
diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
index c043fe717553..b1d42a639ece 100644
--- a/drivers/gpu/drm/ast/ast_post.c
+++ b/drivers/gpu/drm/ast/ast_post.c
@@ -368,9 +368,9 @@ void ast_post_gpu(struct drm_device *dev)
 	u32 reg;
 	struct ast_private *ast = to_ast_private(dev);
 
-	pci_read_config_dword(ast->dev->pdev, 0x04, &reg);
+	pci_read_config_dword(dev->pdev, 0x04, &reg);
 	reg |= 0x3;
-	pci_write_config_dword(ast->dev->pdev, 0x04, reg);
+	pci_write_config_dword(dev->pdev, 0x04, reg);
 
 	ast_enable_vga(dev);
 	ast_open_key(ast);
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/13] drm/ast: Embed struct drm_device in struct ast_private

[Thomas Zimmermann]

Turns struct ast_private into a subclass of struct drm_device by
embedding the latter. This allows for using DRM's managed device
allocation.

The use of struct drm_device.dev_private is deprecated. The patch
converts the last remaining users to to_ast_private().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_cursor.c |  6 ++---
 drivers/gpu/drm/ast/ast_drv.c    |  2 +-
 drivers/gpu/drm/ast/ast_drv.h    |  4 +--
 drivers/gpu/drm/ast/ast_main.c   | 42 ++++++++++----------------------
 drivers/gpu/drm/ast/ast_mm.c     |  2 +-
 drivers/gpu/drm/ast/ast_mode.c   |  2 +-
 drivers/gpu/drm/ast/ast_post.c   |  2 +-
 7 files changed, 22 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
index 8d693c8b346f..6c96f74cdb9e 100644
--- a/drivers/gpu/drm/ast/ast_cursor.c
+++ b/drivers/gpu/drm/ast/ast_cursor.c
@@ -57,7 +57,7 @@ static void ast_cursor_release(struct drm_device *dev, void *ptr)
  */
 int ast_cursor_init(struct ast_private *ast)
 {
-	struct drm_device *dev = ast->dev;
+	struct drm_device *dev = &ast->base;
 	size_t size, i;
 	struct drm_gem_vram_object *gbo;
 	void __iomem *vaddr;
@@ -168,7 +168,7 @@ static void update_cursor_image(u8 __iomem *dst, const u8 *src, int width, int h
 
 int ast_cursor_blit(struct ast_private *ast, struct drm_framebuffer *fb)
 {
-	struct drm_device *dev = ast->dev;
+	struct drm_device *dev = &ast->base;
 	struct drm_gem_vram_object *gbo;
 	int ret;
 	void *src;
@@ -217,7 +217,7 @@ static void ast_cursor_set_base(struct ast_private *ast, u64 address)
 
 void ast_cursor_page_flip(struct ast_private *ast)
 {
-	struct drm_device *dev = ast->dev;
+	struct drm_device *dev = &ast->base;
 	struct drm_gem_vram_object *gbo;
 	s64 off;
 
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index ad93c35b4cf7..c394383a7979 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -124,7 +124,7 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		ret = PTR_ERR(ast);
 		goto err_drm_dev_put;
 	}
-	dev = ast->dev;
+	dev = &ast->base;
 
 	ret = drm_dev_register(dev, ent->driver_data);
 	if (ret)
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index f1aebc719d9e..86c9a7ac712b 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -116,7 +116,7 @@ to_ast_connector(struct drm_connector *connector)
 }
 
 struct ast_private {
-	struct drm_device *dev;
+	struct drm_device base;
 
 	void __iomem *regs;
 	void __iomem *ioregs;
@@ -156,7 +156,7 @@ struct ast_private {
 
 static inline struct ast_private *to_ast_private(struct drm_device *dev)
 {
-	return dev->dev_private;
+	return container_of(dev, struct ast_private, base);
 }
 
 struct ast_private *ast_device_create(struct drm_driver *drv,
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 8d46166f8462..792fb7f616ec 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -388,25 +388,17 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
 	bool need_post;
 	int ret = 0;
 
-	dev = drm_dev_alloc(drv, &pdev->dev);
-	if (IS_ERR(dev))
-		return ERR_CAST(dev);
+	ast = devm_drm_dev_alloc(&pdev->dev, drv, struct ast_private, base);
+	if (IS_ERR(ast))
+		return ast;
+	dev = &ast->base;
 
 	dev->pdev = pdev;
 	pci_set_drvdata(pdev, dev);
 
-	ast = kzalloc(sizeof(struct ast_private), GFP_KERNEL);
-	if (!ast)
-		return ERR_PTR(-ENOMEM);
-
-	dev->dev_private = ast;
-	ast->dev = dev;
-
 	ast->regs = pci_iomap(dev->pdev, 1, 0);
-	if (!ast->regs) {
-		ret = -EIO;
-		goto out_free;
-	}
+	if (!ast->regs)
+		return ERR_PTR(-EIO);
 
 	/*
 	 * If we don't have IO space at all, use MMIO now and
@@ -421,17 +413,16 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
 	/* "map" IO regs if the above hasn't done so already */
 	if (!ast->ioregs) {
 		ast->ioregs = pci_iomap(dev->pdev, 2, 0);
-		if (!ast->ioregs) {
-			ret = -EIO;
-			goto out_free;
-		}
+		if (!ast->ioregs)
+			return ERR_PTR(-EIO);
 	}
 
 	ast_detect_chip(dev, &need_post);
 
 	ret = ast_get_dram_info(dev);
 	if (ret)
-		goto out_free;
+		return ERR_PTR(ret);
+
 	drm_info(dev, "dram MCLK=%u Mhz type=%d bus_width=%d\n",
 		 ast->mclk, ast->dram_type, ast->dram_bus_width);
 
@@ -440,29 +431,22 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
 
 	ret = ast_mm_init(ast);
 	if (ret)
-		goto out_free;
+		return ERR_PTR(ret);
 
 	ret = ast_mode_config_init(ast);
 	if (ret)
-		goto out_free;
+		return ERR_PTR(ret);
 
 	return ast;
-
-out_free:
-	kfree(ast);
-	dev->dev_private = NULL;
-	return ERR_PTR(ret);
 }
 
 void ast_device_destroy(struct ast_private *ast)
 {
-	struct drm_device *dev = ast->dev;
+	struct drm_device *dev = &ast->base;
 
 	/* enable standard VGA decode */
 	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
 
 	ast_release_firmware(dev);
 	kfree(ast->dp501_fw_addr);
-
-	kfree(ast);
 }
diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
index 9186ec3ebbe0..8392ebde504b 100644
--- a/drivers/gpu/drm/ast/ast_mm.c
+++ b/drivers/gpu/drm/ast/ast_mm.c
@@ -85,9 +85,9 @@ static void ast_mm_release(struct drm_device *dev, void *ptr)
 
 int ast_mm_init(struct ast_private *ast)
 {
+	struct drm_device *dev = &ast->base;
 	u32 vram_size;
 	int ret;
-	struct drm_device *dev = ast->dev;
 
 	vram_size = ast_get_vram_size(ast);
 
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index dceb11a320b2..4a4010fdfd2d 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1180,7 +1180,7 @@ static const struct drm_mode_config_funcs ast_mode_config_funcs = {
 
 int ast_mode_config_init(struct ast_private *ast)
 {
-	struct drm_device *dev = ast->dev;
+	struct drm_device *dev = &ast->base;
 	int ret;
 
 	ret = ast_cursor_init(ast);
diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
index b1d42a639ece..8902c2f84bf9 100644
--- a/drivers/gpu/drm/ast/ast_post.c
+++ b/drivers/gpu/drm/ast/ast_post.c
@@ -365,8 +365,8 @@ static void ast_init_dram_reg(struct drm_device *dev)
 
 void ast_post_gpu(struct drm_device *dev)
 {
-	u32 reg;
 	struct ast_private *ast = to_ast_private(dev);
+	u32 reg;
 
 	pci_read_config_dword(dev->pdev, 0x04, &reg);
 	reg |= 0x3;
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 11/13] drm/ast: Managed release of ast firmware

[Thomas Zimmermann]

The ast driver loads firmware for the DP501 display encoder. The
patch replaces the removal code with a managed release function.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_dp501.c | 23 ++++++++++++++---------
 drivers/gpu/drm/ast/ast_drv.h   |  1 -
 drivers/gpu/drm/ast/ast_main.c  |  3 ---
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
index 4b85a504825a..88121c0e0d05 100644
--- a/drivers/gpu/drm/ast/ast_dp501.c
+++ b/drivers/gpu/drm/ast/ast_dp501.c
@@ -8,11 +8,24 @@
 
 MODULE_FIRMWARE("ast_dp501_fw.bin");
 
+static void ast_release_firmware(void *data)
+{
+	struct ast_private *ast = data;
+
+	release_firmware(ast->dp501_fw);
+	ast->dp501_fw = NULL;
+}
+
 static int ast_load_dp501_microcode(struct drm_device *dev)
 {
 	struct ast_private *ast = to_ast_private(dev);
+	int ret;
+
+	ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
+	if (ret)
+		return ret;
 
-	return request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
+	return devm_add_action_or_reset(dev->dev, ast_release_firmware, ast);
 }
 
 static void send_ack(struct ast_private *ast)
@@ -435,11 +448,3 @@ void ast_init_3rdtx(struct drm_device *dev)
 		}
 	}
 }
-
-void ast_release_firmware(struct drm_device *dev)
-{
-	struct ast_private *ast = to_ast_private(dev);
-
-	release_firmware(ast->dp501_fw);
-	ast->dp501_fw = NULL;
-}
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 86c9a7ac712b..02908d005b99 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -312,7 +312,6 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size);
 bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata);
 u8 ast_get_dp501_max_clk(struct drm_device *dev);
 void ast_init_3rdtx(struct drm_device *dev);
-void ast_release_firmware(struct drm_device *dev);
 
 /* ast_cursor.c */
 int ast_cursor_init(struct ast_private *ast);
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 792fb7f616ec..e3b7748335a3 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -442,11 +442,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
 
 void ast_device_destroy(struct ast_private *ast)
 {
-	struct drm_device *dev = &ast->base;
-
 	/* enable standard VGA decode */
 	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
 
-	ast_release_firmware(dev);
 	kfree(ast->dp501_fw_addr);
 }
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/13] drm/ast: Managed release of ast firmware

[daniel]

On Tue, Jul 28, 2020 at 09:44:23AM +0200, Thomas Zimmermann wrote:
> The ast driver loads firmware for the DP501 display encoder. The
> patch replaces the removal code with a managed release function.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Hm a devm_request_firmware which does exactly this would be nice I think.
Maybe as a follow-up refactor?
-Daniel

> ---
>  drivers/gpu/drm/ast/ast_dp501.c | 23 ++++++++++++++---------
>  drivers/gpu/drm/ast/ast_drv.h   |  1 -
>  drivers/gpu/drm/ast/ast_main.c  |  3 ---
>  3 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
> index 4b85a504825a..88121c0e0d05 100644
> --- a/drivers/gpu/drm/ast/ast_dp501.c
> +++ b/drivers/gpu/drm/ast/ast_dp501.c
> @@ -8,11 +8,24 @@
>  
>  MODULE_FIRMWARE("ast_dp501_fw.bin");
>  
> +static void ast_release_firmware(void *data)
> +{
> +	struct ast_private *ast = data;
> +
> +	release_firmware(ast->dp501_fw);
> +	ast->dp501_fw = NULL;
> +}
> +
>  static int ast_load_dp501_microcode(struct drm_device *dev)
>  {
>  	struct ast_private *ast = to_ast_private(dev);
> +	int ret;
> +
> +	ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
> +	if (ret)
> +		return ret;
>  
> -	return request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
> +	return devm_add_action_or_reset(dev->dev, ast_release_firmware, ast);
>  }
>  
>  static void send_ack(struct ast_private *ast)
> @@ -435,11 +448,3 @@ void ast_init_3rdtx(struct drm_device *dev)
>  		}
>  	}
>  }
> -
> -void ast_release_firmware(struct drm_device *dev)
> -{
> -	struct ast_private *ast = to_ast_private(dev);
> -
> -	release_firmware(ast->dp501_fw);
> -	ast->dp501_fw = NULL;
> -}
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 86c9a7ac712b..02908d005b99 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -312,7 +312,6 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size);
>  bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata);
>  u8 ast_get_dp501_max_clk(struct drm_device *dev);
>  void ast_init_3rdtx(struct drm_device *dev);
> -void ast_release_firmware(struct drm_device *dev);
>  
>  /* ast_cursor.c */
>  int ast_cursor_init(struct ast_private *ast);
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index 792fb7f616ec..e3b7748335a3 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -442,11 +442,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
>  
>  void ast_device_destroy(struct ast_private *ast)
>  {
> -	struct drm_device *dev = &ast->base;
> -
>  	/* enable standard VGA decode */
>  	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
>  
> -	ast_release_firmware(dev);
>  	kfree(ast->dp501_fw_addr);
>  }
> -- 
> 2.27.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/13] drm/ast: Managed release of ast firmware

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3643 bytes --]

Hi

Am 28.07.20 um 11:17 schrieb daniel@ffwll.ch:
> On Tue, Jul 28, 2020 at 09:44:23AM +0200, Thomas Zimmermann wrote:
>> The ast driver loads firmware for the DP501 display encoder. The
>> patch replaces the removal code with a managed release function.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Hm a devm_request_firmware which does exactly this would be nice I think.
> Maybe as a follow-up refactor?

There are so many ideas for follow-up patches wrt. devres and drmres, we
should add a todo item to collect them. Especially, devres is much more
over head in terms of reviews and kernel building/testing tha tit makes
sense to collect ideas and address them in larger chunks.

Best regards
Thomas

> -Daniel
> 
>> ---
>>  drivers/gpu/drm/ast/ast_dp501.c | 23 ++++++++++++++---------
>>  drivers/gpu/drm/ast/ast_drv.h   |  1 -
>>  drivers/gpu/drm/ast/ast_main.c  |  3 ---
>>  3 files changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
>> index 4b85a504825a..88121c0e0d05 100644
>> --- a/drivers/gpu/drm/ast/ast_dp501.c
>> +++ b/drivers/gpu/drm/ast/ast_dp501.c
>> @@ -8,11 +8,24 @@
>>  
>>  MODULE_FIRMWARE("ast_dp501_fw.bin");
>>  
>> +static void ast_release_firmware(void *data)
>> +{
>> +	struct ast_private *ast = data;
>> +
>> +	release_firmware(ast->dp501_fw);
>> +	ast->dp501_fw = NULL;
>> +}
>> +
>>  static int ast_load_dp501_microcode(struct drm_device *dev)
>>  {
>>  	struct ast_private *ast = to_ast_private(dev);
>> +	int ret;
>> +
>> +	ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
>> +	if (ret)
>> +		return ret;
>>  
>> -	return request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
>> +	return devm_add_action_or_reset(dev->dev, ast_release_firmware, ast);
>>  }
>>  
>>  static void send_ack(struct ast_private *ast)
>> @@ -435,11 +448,3 @@ void ast_init_3rdtx(struct drm_device *dev)
>>  		}
>>  	}
>>  }
>> -
>> -void ast_release_firmware(struct drm_device *dev)
>> -{
>> -	struct ast_private *ast = to_ast_private(dev);
>> -
>> -	release_firmware(ast->dp501_fw);
>> -	ast->dp501_fw = NULL;
>> -}
>> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
>> index 86c9a7ac712b..02908d005b99 100644
>> --- a/drivers/gpu/drm/ast/ast_drv.h
>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>> @@ -312,7 +312,6 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size);
>>  bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata);
>>  u8 ast_get_dp501_max_clk(struct drm_device *dev);
>>  void ast_init_3rdtx(struct drm_device *dev);
>> -void ast_release_firmware(struct drm_device *dev);
>>  
>>  /* ast_cursor.c */
>>  int ast_cursor_init(struct ast_private *ast);
>> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
>> index 792fb7f616ec..e3b7748335a3 100644
>> --- a/drivers/gpu/drm/ast/ast_main.c
>> +++ b/drivers/gpu/drm/ast/ast_main.c
>> @@ -442,11 +442,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
>>  
>>  void ast_device_destroy(struct ast_private *ast)
>>  {
>> -	struct drm_device *dev = &ast->base;
>> -
>>  	/* enable standard VGA decode */
>>  	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
>>  
>> -	ast_release_firmware(dev);
>>  	kfree(ast->dp501_fw_addr);
>>  }
>> -- 
>> 2.27.0
>>
> 

-- 
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/13] drm/ast: Managed release of ast firmware

[daniel]

On Tue, Jul 28, 2020 at 11:32:04AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 28.07.20 um 11:17 schrieb daniel@ffwll.ch:
> > On Tue, Jul 28, 2020 at 09:44:23AM +0200, Thomas Zimmermann wrote:
> >> The ast driver loads firmware for the DP501 display encoder. The
> >> patch replaces the removal code with a managed release function.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > 
> > Hm a devm_request_firmware which does exactly this would be nice I think.
> > Maybe as a follow-up refactor?
> 
> There are so many ideas for follow-up patches wrt. devres and drmres, we
> should add a todo item to collect them. Especially, devres is much more
> over head in terms of reviews and kernel building/testing tha tit makes
> sense to collect ideas and address them in larger chunks.

Yeah maybe a section with wanted devres functions in todo.rst makes sense.
For devres it depends which subsystem you're dealing with I guess, and how
much they want to see before it lands.
-Daniel

> 
> Best regards
> Thomas
> 
> > -Daniel
> > 
> >> ---
> >>  drivers/gpu/drm/ast/ast_dp501.c | 23 ++++++++++++++---------
> >>  drivers/gpu/drm/ast/ast_drv.h   |  1 -
> >>  drivers/gpu/drm/ast/ast_main.c  |  3 ---
> >>  3 files changed, 14 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
> >> index 4b85a504825a..88121c0e0d05 100644
> >> --- a/drivers/gpu/drm/ast/ast_dp501.c
> >> +++ b/drivers/gpu/drm/ast/ast_dp501.c
> >> @@ -8,11 +8,24 @@
> >>  
> >>  MODULE_FIRMWARE("ast_dp501_fw.bin");
> >>  
> >> +static void ast_release_firmware(void *data)
> >> +{
> >> +	struct ast_private *ast = data;
> >> +
> >> +	release_firmware(ast->dp501_fw);
> >> +	ast->dp501_fw = NULL;
> >> +}
> >> +
> >>  static int ast_load_dp501_microcode(struct drm_device *dev)
> >>  {
> >>  	struct ast_private *ast = to_ast_private(dev);
> >> +	int ret;
> >> +
> >> +	ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
> >> +	if (ret)
> >> +		return ret;
> >>  
> >> -	return request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
> >> +	return devm_add_action_or_reset(dev->dev, ast_release_firmware, ast);
> >>  }
> >>  
> >>  static void send_ack(struct ast_private *ast)
> >> @@ -435,11 +448,3 @@ void ast_init_3rdtx(struct drm_device *dev)
> >>  		}
> >>  	}
> >>  }
> >> -
> >> -void ast_release_firmware(struct drm_device *dev)
> >> -{
> >> -	struct ast_private *ast = to_ast_private(dev);
> >> -
> >> -	release_firmware(ast->dp501_fw);
> >> -	ast->dp501_fw = NULL;
> >> -}
> >> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> >> index 86c9a7ac712b..02908d005b99 100644
> >> --- a/drivers/gpu/drm/ast/ast_drv.h
> >> +++ b/drivers/gpu/drm/ast/ast_drv.h
> >> @@ -312,7 +312,6 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size);
> >>  bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata);
> >>  u8 ast_get_dp501_max_clk(struct drm_device *dev);
> >>  void ast_init_3rdtx(struct drm_device *dev);
> >> -void ast_release_firmware(struct drm_device *dev);
> >>  
> >>  /* ast_cursor.c */
> >>  int ast_cursor_init(struct ast_private *ast);
> >> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> >> index 792fb7f616ec..e3b7748335a3 100644
> >> --- a/drivers/gpu/drm/ast/ast_main.c
> >> +++ b/drivers/gpu/drm/ast/ast_main.c
> >> @@ -442,11 +442,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
> >>  
> >>  void ast_device_destroy(struct ast_private *ast)
> >>  {
> >> -	struct drm_device *dev = &ast->base;
> >> -
> >>  	/* enable standard VGA decode */
> >>  	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
> >>  
> >> -	ast_release_firmware(dev);
> >>  	kfree(ast->dp501_fw_addr);
> >>  }
> >> -- 
> >> 2.27.0
> >>
> > 
> 
> -- 
> 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
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 12/13] drm/ast: Manage release of firmware backup memory

[Thomas Zimmermann]

The ast driver keeps a backup copy of the DP501 encoder's firmware. This
patch adds managed release of the allocated memory.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_main.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index e3b7748335a3..67e20727d82d 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -33,6 +33,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_gem_vram_helper.h>
+#include <drm/drm_managed.h>
 
 #include "ast_drv.h"
 
@@ -231,11 +232,11 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
 			ast->tx_chip_type = AST_TX_SIL164;
 			break;
 		case 0x08:
-			ast->dp501_fw_addr = kzalloc(32*1024, GFP_KERNEL);
+			ast->dp501_fw_addr = drmm_kzalloc(dev, 32*1024, GFP_KERNEL);
 			if (ast->dp501_fw_addr) {
 				/* backup firmware */
 				if (ast_backup_fw(dev, ast->dp501_fw_addr, 32*1024)) {
-					kfree(ast->dp501_fw_addr);
+					drmm_kfree(dev, ast->dp501_fw_addr);
 					ast->dp501_fw_addr = NULL;
 				}
 			}
@@ -444,6 +445,4 @@ void ast_device_destroy(struct ast_private *ast)
 {
 	/* enable standard VGA decode */
 	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
-
-	kfree(ast->dp501_fw_addr);
 }
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 12/13] drm/ast: Manage release of firmware backup memory

[daniel]

On Tue, Jul 28, 2020 at 09:44:24AM +0200, Thomas Zimmermann wrote:
> The ast driver keeps a backup copy of the DP501 encoder's firmware. This
> patch adds managed release of the allocated memory.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

We can't really do anything with the firmware after the device is gone, so
I think this is one of the very rare exceptions where devm_kzalloc is the
right thing to do! Totally minor nit though, since either way it gets
cleaned up. But I think conceptually cleaner.
-Daniel
> ---
>  drivers/gpu/drm/ast/ast_main.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index e3b7748335a3..67e20727d82d 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -33,6 +33,7 @@
>  #include <drm/drm_drv.h>
>  #include <drm/drm_gem.h>
>  #include <drm/drm_gem_vram_helper.h>
> +#include <drm/drm_managed.h>
>  
>  #include "ast_drv.h"
>  
> @@ -231,11 +232,11 @@ static int ast_detect_chip(struct drm_device *dev, bool *need_post)
>  			ast->tx_chip_type = AST_TX_SIL164;
>  			break;
>  		case 0x08:
> -			ast->dp501_fw_addr = kzalloc(32*1024, GFP_KERNEL);
> +			ast->dp501_fw_addr = drmm_kzalloc(dev, 32*1024, GFP_KERNEL);
>  			if (ast->dp501_fw_addr) {
>  				/* backup firmware */
>  				if (ast_backup_fw(dev, ast->dp501_fw_addr, 32*1024)) {
> -					kfree(ast->dp501_fw_addr);
> +					drmm_kfree(dev, ast->dp501_fw_addr);
>  					ast->dp501_fw_addr = NULL;
>  				}
>  			}
> @@ -444,6 +445,4 @@ void ast_device_destroy(struct ast_private *ast)
>  {
>  	/* enable standard VGA decode */
>  	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
> -
> -	kfree(ast->dp501_fw_addr);
>  }
> -- 
> 2.27.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 13/13] drm/ast: Managed device release

[Thomas Zimmermann]

This turns the ast's device cleanup code into a managed release helper
function. Note that the code uses devres helpers. The release function
switches the device back to VGA mode and therefore runs during HW device
cleanup; not at DRM device cleanup.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.c  | 17 +++--------------
 drivers/gpu/drm/ast/ast_drv.h  |  1 -
 drivers/gpu/drm/ast/ast_main.c | 22 ++++++++++++++++------
 3 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index c394383a7979..f0b4af1c390a 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -120,35 +120,24 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		return ret;
 
 	ast = ast_device_create(&ast_driver, pdev, ent->driver_data);
-	if (IS_ERR(ast)) {
-		ret = PTR_ERR(ast);
-		goto err_drm_dev_put;
-	}
+	if (IS_ERR(ast))
+		return PTR_ERR(ast);
 	dev = &ast->base;
 
 	ret = drm_dev_register(dev, ent->driver_data);
 	if (ret)
-		goto err_ast_device_destroy;
+		return ret;
 
 	drm_fbdev_generic_setup(dev, 32);
 
 	return 0;
-
-err_ast_device_destroy:
-	ast_device_destroy(ast);
-err_drm_dev_put:
-	drm_dev_put(dev);
-	return ret;
 }
 
 static void ast_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
-	struct ast_private *ast = to_ast_private(dev);
 
 	drm_dev_unregister(dev);
-	ast_device_destroy(ast);
-	drm_dev_put(dev);
 }
 
 static int ast_drm_freeze(struct drm_device *dev)
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 02908d005b99..0911136e0842 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -162,7 +162,6 @@ static inline struct ast_private *to_ast_private(struct drm_device *dev)
 struct ast_private *ast_device_create(struct drm_driver *drv,
 				      struct pci_dev *pdev,
 				      unsigned long flags);
-void ast_device_destroy(struct ast_private *ast);
 
 #define AST_IO_AR_PORT_WRITE		(0x40)
 #define AST_IO_MISC_PORT_WRITE		(0x42)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 67e20727d82d..d62749a10cdd 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -380,6 +380,18 @@ static int ast_get_dram_info(struct drm_device *dev)
 	return 0;
 }
 
+/*
+ * Run this function as part of the HW device cleanup; not
+ * when the DRM device gets released.
+ */
+static void ast_device_release(void *data)
+{
+	struct ast_private *ast = data;
+
+	/* enable standard VGA decode */
+	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
+}
+
 struct ast_private *ast_device_create(struct drm_driver *drv,
 				      struct pci_dev *pdev,
 				      unsigned long flags)
@@ -438,11 +450,9 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
 	if (ret)
 		return ERR_PTR(ret);
 
-	return ast;
-}
+	ret = devm_add_action_or_reset(dev->dev, ast_device_release, ast);
+	if (ret)
+		return ERR_PTR(ret);
 
-void ast_device_destroy(struct ast_private *ast)
-{
-	/* enable standard VGA decode */
-	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
+	return ast;
 }
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/13] drm/ast: Convert to managed initialization

[Sam Ravnborg]

Hi Thomas.

On Tue, Jul 28, 2020 at 09:44:12AM +0200, Thomas Zimmermann wrote:
> This is the final patchset for converting ast to managed initialization.
> 
> Patches #1 to #4 address I2C helpers. The structures are being stored
> in struct ast_connector. The initialization and cleanups is being converted
> to managed release helpers.
> 
> Patches #5 to #10 address modesetting and device structures. All are
> being embedded into struct ast_private. With struct ast_private being
> a subclass of struct drm_device, patch #10 switches ast to DRM's managed-
> allocation helpers.
> 
> Patches #11 and #12 address firmware memory that ast allocates
> internally.
> 
> Finally, patch #13 removes ast's destroy function in favor of managed
> release helpers.
> 
> Tested on AST 2100 HW.
> 
> Thomas Zimmermann (13):
>   drm/ast: Move I2C code within ast_mode.c
>   drm/ast: Test if I2C support has been initialized
>   drm/ast: Embed I2C fields in struct ast_connector
>   drm/ast: Managed release of I2C adapter
>   drm/ast: Embed CRTC and connector in struct ast_private
>   drm/ast: Separate DRM driver from PCI code
>   drm/ast: Replace driver load/unload functions with device
>     create/destroy
>   drm/ast: Replace struct_drm_device.dev_private with to_ast_private()
>   drm/ast: Don't use ast->dev if dev is available
>   drm/ast: Embed struct drm_device in struct ast_private
>   drm/ast: Managed release of ast firmware
>   drm/ast: Manage release of firmware backup memory
>   drm/ast: Managed device release

A few nits posted to a few patches.
Patch 1-11 are all:
Acked-by: Sam Ravnborg <sam@ravnborg.org>

I did not look at 12 and did not follow all the changes in 13.
Not that I found 13 faulty - just lost track and -ENOTIME

	Sam

> 
>  drivers/gpu/drm/ast/ast_cursor.c |   8 +-
>  drivers/gpu/drm/ast/ast_dp501.c  |  23 ++-
>  drivers/gpu/drm/ast/ast_drv.c    |  82 ++++----
>  drivers/gpu/drm/ast/ast_drv.h    |  43 +++--
>  drivers/gpu/drm/ast/ast_main.c   |  74 ++++----
>  drivers/gpu/drm/ast/ast_mm.c     |   2 +-
>  drivers/gpu/drm/ast/ast_mode.c   | 310 ++++++++++++++-----------------
>  drivers/gpu/drm/ast/ast_post.c   |   6 +-
>  8 files changed, 263 insertions(+), 285 deletions(-)
> 
> --
> 2.27.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel