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

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, &scu_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("A

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information