Re: [PATCH 1/3] drm/ast: Remove unused code paths for AST 1180

2020-06-17 Thread Thomas Zimmermann 
Hi Emil

Am 16.06.20 um 01:21 schrieb Emil Velikov:
> Hi Thomas,
> 
> On Thu, 11 Jun 2020 at 09:28, Thomas Zimmermann  wrote:
> 
>> --- a/drivers/gpu/drm/ast/ast_drv.c
>> +++ b/drivers/gpu/drm/ast/ast_drv.c
>> @@ -59,7 +59,6 @@ static struct drm_driver driver;
>>  static const struct pci_device_id pciidlist[] = {
>> AST_VGA_DEVICE(PCI_CHIP_AST2000, NULL),
>> AST_VGA_DEVICE(PCI_CHIP_AST2100, NULL),
>> -   /*  AST_VGA_DEVICE(PCI_CHIP_AST1180, NULL), - don't bind to 1180 
>> for now */
> 
> Since we don't bind to this pciid, the (removed) code is never
> used/dead. For the series:
> Reviewed-by: Emil Velikov 
> 
> Small idea below: feel free to ignore or if you agree - follow-up at a
> random point in the future.
> 
> 
>> +   if (dev->pdev->revision >= 0x40) {
>> +   ast->chip = AST2500;
>> +   DRM_INFO("AST 2500 detected\n");
>> +   } else if (dev->pdev->revision >= 0x30) {
>> +   ast->chip = AST2400;
>> +   DRM_INFO("AST 2400 detected\n");
>> +   } else if (dev->pdev->revision >= 0x30) {
>> +   ast->chip = AST2400;
>> +   DRM_INFO("AST 2400 detected\n");
>> +   } else if (dev->pdev->revision >= 0x20) {
>> +   ast->chip = AST2300;
>> +   DRM_INFO("AST 2300 detected\n");
>> +   } else if (dev->pdev->revision >= 0x10) {
>> +   switch (scu_rev & 0x0300) {
>> +   case 0x0200:
>> +   ast->chip = AST1100;
>> +   DRM_INFO("AST 1100 detected\n");
>> +   break;
>> +   case 0x0100:
>> +   ast->chip = AST2200;
>> +   DRM_INFO("AST 2200 detected\n");
>> +   break;
>> +   case 0x:
>> +   ast->chip = AST2150;
>> +   DRM_INFO("AST 2150 detected\n");
>> +   break;
>> +   default:
>> +   ast->chip = AST2100;
>> +   DRM_INFO("AST 2100 detected\n");
>> +   break;
>> }
>> +   ast->vga2_clone = false;
>> +   } else {
>> +   ast->chip = AST2000;
>> +   DRM_INFO("AST 2000 detected\n");
>> }
>>
> Instead of the above if/else spaghetti, one can use something alike
> 
> static const struct foo {
> u8 rev_maj; // revision & 0xf0 >> 4
> u8 rev_scu; // scu_rev & 0x0300 >> 8, ignored if table has 0xf
> enum ast_chip;
> const char *name;
> } bar {
>{ 0x3, 0xf, AST2400, "2400" },
>{ 0x2, 0xf, AST2300, "2300" },
>{ 0x1, 0x3, AST2100, "2100" },
>{ 0x1, 0x2, AST1100, "1100" },
>{ 0x1, 0x1, AST2200, "2200" },
>{ 0x1, 0x0, AST2150, "2150" },
>{ 0x0, 0xf, AST2000, "2000" },
> };
> 
> + trivial loop.

I do like the idea, but there's plenty of similar code throughout the
driver. I think a separate patchset could introduce an info structure
with per-chip constants and flags, and fix the entire driver.

Best regards
Thomas

> 
> -Emil
> ___
> 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



signature.asc
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/ast: Remove unused code paths for AST 1180

2020-06-15 Thread Emil Velikov 
Hi Thomas,

On Thu, 11 Jun 2020 at 09:28, Thomas Zimmermann  wrote:

> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -59,7 +59,6 @@ static struct drm_driver driver;
>  static const struct pci_device_id pciidlist[] = {
> AST_VGA_DEVICE(PCI_CHIP_AST2000, NULL),
> AST_VGA_DEVICE(PCI_CHIP_AST2100, NULL),
> -   /*  AST_VGA_DEVICE(PCI_CHIP_AST1180, NULL), - don't bind to 1180 
> for now */

Since we don't bind to this pciid, the (removed) code is never
used/dead. For the series:
Reviewed-by: Emil Velikov 

Small idea below: feel free to ignore or if you agree - follow-up at a
random point in the future.


> +   if (dev->pdev->revision >= 0x40) {
> +   ast->chip = AST2500;
> +   DRM_INFO("AST 2500 detected\n");
> +   } else if (dev->pdev->revision >= 0x30) {
> +   ast->chip = AST2400;
> +   DRM_INFO("AST 2400 detected\n");
> +   } else if (dev->pdev->revision >= 0x30) {
> +   ast->chip = AST2400;
> +   DRM_INFO("AST 2400 detected\n");
> +   } else if (dev->pdev->revision >= 0x20) {
> +   ast->chip = AST2300;
> +   DRM_INFO("AST 2300 detected\n");
> +   } else if (dev->pdev->revision >= 0x10) {
> +   switch (scu_rev & 0x0300) {
> +   case 0x0200:
> +   ast->chip = AST1100;
> +   DRM_INFO("AST 1100 detected\n");
> +   break;
> +   case 0x0100:
> +   ast->chip = AST2200;
> +   DRM_INFO("AST 2200 detected\n");
> +   break;
> +   case 0x:
> +   ast->chip = AST2150;
> +   DRM_INFO("AST 2150 detected\n");
> +   break;
> +   default:
> +   ast->chip = AST2100;
> +   DRM_INFO("AST 2100 detected\n");
> +   break;
> }
> +   ast->vga2_clone = false;
> +   } else {
> +   ast->chip = AST2000;
> +   DRM_INFO("AST 2000 detected\n");
> }
>
Instead of the above if/else spaghetti, one can use something alike

static const struct foo {
u8 rev_maj; // revision & 0xf0 >> 4
u8 rev_scu; // scu_rev & 0x0300 >> 8, ignored if table has 0xf
enum ast_chip;
const char *name;
} bar {
   { 0x3, 0xf, AST2400, "2400" },
   { 0x2, 0xf, AST2300, "2300" },
   { 0x1, 0x3, AST2100, "2100" },
   { 0x1, 0x2, AST1100, "1100" },
   { 0x1, 0x1, AST2200, "2200" },
   { 0x1, 0x0, AST2150, "2150" },
   { 0x0, 0xf, AST2000, "2000" },
};

+ trivial loop.

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

Re: [PATCH 1/3] drm/ast: Remove unused code paths for AST 1180

2020-06-11 Thread Daniel Vetter 
On Thu, Jun 11, 2020 at 10:28:07AM +0200, Thomas Zimmermann wrote:
> The ast driver contains code paths for AST 1180 chips. The chip is not
> supported and the rsp code has never been tested. Simplify the driver by
> removing the AST 1180 code.
> 
> Signed-off-by: Thomas Zimmermann 

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/ast/ast_drv.c  |  1 -
>  drivers/gpu/drm/ast/ast_drv.h  |  2 -
>  drivers/gpu/drm/ast/ast_main.c | 89 +++---
>  drivers/gpu/drm/ast/ast_mode.c | 10 +---
>  drivers/gpu/drm/ast/ast_post.c | 10 ++--
>  5 files changed, 43 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index b7ba22dddcad9..83509106f3ba9 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -59,7 +59,6 @@ static struct drm_driver driver;
>  static const struct pci_device_id pciidlist[] = {
>   AST_VGA_DEVICE(PCI_CHIP_AST2000, NULL),
>   AST_VGA_DEVICE(PCI_CHIP_AST2100, NULL),
> - /*  AST_VGA_DEVICE(PCI_CHIP_AST1180, NULL), - don't bind to 1180 
> for now */
>   {0, 0, 0},
>  };
>  
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 656d591b154b3..09f2659e29118 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -52,7 +52,6 @@
>  
>  #define PCI_CHIP_AST2000 0x2000
>  #define PCI_CHIP_AST2100 0x2010
> -#define PCI_CHIP_AST1180 0x1180
>  
>  
>  enum ast_chip {
> @@ -64,7 +63,6 @@ enum ast_chip {
>   AST2300,
>   AST2400,
>   AST2500,
> - AST1180,
>  };
>  
>  enum ast_tx_chip {
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index e5398e3dabe70..f48a9f62368c0 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -142,50 +142,42 @@ static int ast_detect_chip(struct drm_device *dev, bool 
> *need_post)
>   ast_detect_config_mode(dev, _rev);
>  
>   /* Identify chipset */
> - if (dev->pdev->device == PCI_CHIP_AST1180) {
> - ast->chip = AST1100;
> - DRM_INFO("AST 1180 detected\n");
> - } else {
> - if (dev->pdev->revision >= 0x40) {
> - ast->chip = AST2500;
> - DRM_INFO("AST 2500 detected\n");
> - } else if (dev->pdev->revision >= 0x30) {
> - ast->chip = AST2400;
> - DRM_INFO("AST 2400 detected\n");
> - } else if (dev->pdev->revision >= 0x20) {
> - ast->chip = AST2300;
> - DRM_INFO("AST 2300 detected\n");
> - } else if (dev->pdev->revision >= 0x10) {
> - switch (scu_rev & 0x0300) {
> - case 0x0200:
> - ast->chip = AST1100;
> - DRM_INFO("AST 1100 detected\n");
> - break;
> - case 0x0100:
> - ast->chip = AST2200;
> - DRM_INFO("AST 2200 detected\n");
> - break;
> - case 0x:
> - ast->chip = AST2150;
> - DRM_INFO("AST 2150 detected\n");
> - break;
> - default:
> - ast->chip = AST2100;
> - DRM_INFO("AST 2100 detected\n");
> - break;
> - }
> - ast->vga2_clone = false;
> - } else {
> - ast->chip = AST2000;
> - DRM_INFO("AST 2000 detected\n");
> + if (dev->pdev->revision >= 0x40) {
> + ast->chip = AST2500;
> + DRM_INFO("AST 2500 detected\n");
> + } else if (dev->pdev->revision >= 0x30) {
> + ast->chip = AST2400;
> + DRM_INFO("AST 2400 detected\n");
> + } else if (dev->pdev->revision >= 0x20) {
> + ast->chip = AST2300;
> + DRM_INFO("AST 2300 detected\n");
> + } else if (dev->pdev->revision >= 0x10) {
> + switch (scu_rev & 0x0300) {
> + case 0x0200:
> + ast->chip = AST1100;
> + DRM_INFO("AST 1100 detected\n");
> + break;
> + case 0x0100:
> + ast->chip = AST2200;
> + DRM_INFO("AST 2200 detected\n");
> + break;
> + case 0x:
> + ast->chip = AST2150;
> + DRM_INFO("AST 2150 detected\n");
> + break;
> + default:
> + ast->chip = AST2100;
> + DRM_INFO("AST 2100 detected\n");
> + break;
>   }
> + ast->vga2_clone = false;
> + } else {
> + ast->chip = AST2000;
> + DRM_INFO("AST

[PATCH 1/3] drm/ast: Remove unused code paths for AST 1180

2020-06-11 Thread Thomas Zimmermann 
The ast driver contains code paths for AST 1180 chips. The chip is not
supported and the rsp code has never been tested. Simplify the driver by
removing the AST 1180 code.

Signed-off-by: Thomas Zimmermann 
---
 drivers/gpu/drm/ast/ast_drv.c  |  1 -
 drivers/gpu/drm/ast/ast_drv.h  |  2 -
 drivers/gpu/drm/ast/ast_main.c | 89 +++---
 drivers/gpu/drm/ast/ast_mode.c | 10 +---
 drivers/gpu/drm/ast/ast_post.c | 10 ++--
 5 files changed, 43 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index b7ba22dddcad9..83509106f3ba9 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -59,7 +59,6 @@ static struct drm_driver driver;
 static const struct pci_device_id pciidlist[] = {
AST_VGA_DEVICE(PCI_CHIP_AST2000, NULL),
AST_VGA_DEVICE(PCI_CHIP_AST2100, NULL),
-   /*  AST_VGA_DEVICE(PCI_CHIP_AST1180, NULL), - don't bind to 1180 
for now */
{0, 0, 0},
 };
 
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 656d591b154b3..09f2659e29118 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -52,7 +52,6 @@
 
 #define PCI_CHIP_AST2000 0x2000
 #define PCI_CHIP_AST2100 0x2010
-#define PCI_CHIP_AST1180 0x1180
 
 
 enum ast_chip {
@@ -64,7 +63,6 @@ enum ast_chip {
AST2300,
AST2400,
AST2500,
-   AST1180,
 };
 
 enum ast_tx_chip {
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index e5398e3dabe70..f48a9f62368c0 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -142,50 +142,42 @@ static int ast_detect_chip(struct drm_device *dev, bool 
*need_post)
ast_detect_config_mode(dev, _rev);
 
/* Identify chipset */
-   if (dev->pdev->device == PCI_CHIP_AST1180) {
-   ast->chip = AST1100;
-   DRM_INFO("AST 1180 detected\n");
-   } else {
-   if (dev->pdev->revision >= 0x40) {
-   ast->chip = AST2500;
-   DRM_INFO("AST 2500 detected\n");
-   } else if (dev->pdev->revision >= 0x30) {
-   ast->chip = AST2400;
-   DRM_INFO("AST 2400 detected\n");
-   } else if (dev->pdev->revision >= 0x20) {
-   ast->chip = AST2300;
-   DRM_INFO("AST 2300 detected\n");
-   } else if (dev->pdev->revision >= 0x10) {
-   switch (scu_rev & 0x0300) {
-   case 0x0200:
-   ast->chip = AST1100;
-   DRM_INFO("AST 1100 detected\n");
-   break;
-   case 0x0100:
-   ast->chip = AST2200;
-   DRM_INFO("AST 2200 detected\n");
-   break;
-   case 0x:
-   ast->chip = AST2150;
-   DRM_INFO("AST 2150 detected\n");
-   break;
-   default:
-   ast->chip = AST2100;
-   DRM_INFO("AST 2100 detected\n");
-   break;
-   }
-   ast->vga2_clone = false;
-   } else {
-   ast->chip = AST2000;
-   DRM_INFO("AST 2000 detected\n");
+   if (dev->pdev->revision >= 0x40) {
+   ast->chip = AST2500;
+   DRM_INFO("AST 2500 detected\n");
+   } else if (dev->pdev->revision >= 0x30) {
+   ast->chip = AST2400;
+   DRM_INFO("AST 2400 detected\n");
+   } else if (dev->pdev->revision >= 0x20) {
+   ast->chip = AST2300;
+   DRM_INFO("AST 2300 detected\n");
+   } else if (dev->pdev->revision >= 0x10) {
+   switch (scu_rev & 0x0300) {
+   case 0x0200:
+   ast->chip = AST1100;
+   DRM_INFO("AST 1100 detected\n");
+   break;
+   case 0x0100:
+   ast->chip = AST2200;
+   DRM_INFO("AST 2200 detected\n");
+   break;
+   case 0x:
+   ast->chip = AST2150;
+   DRM_INFO("AST 2150 detected\n");
+   break;
+   default:
+   ast->chip = AST2100;
+   DRM_INFO("AST 2100 detected\n");
+   break;
}
+   ast->vga2_clone = false;
+   } else {
+   ast->chip = AST2000;
+   DRM_INFO("AST 2000 detected\n");
}
 
/* Check if we support wide screen */
switch (ast->chip) {
-   case AST1180:
-   ast->support_wide_screen = true;
-