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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Hi Emil

Am 16.06.20 um 01:21 schrieb Emil Velikov:
> Hi Thomas,
> 
> On Thu, 11 Jun 2020 at 09:28, Thomas Zimmermann <tzimmermann@suse.de> 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 <emil.l.velikov@gmail.com>
> 
> 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 0x0000:
>> +                       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


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