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
