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