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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx> 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@xxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxxxx
> 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: signature.asc
Description: OpenPGP digital signature

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

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux