Updated.
18 comments:
File src/drivers/generic/genesyslogic/gli9763e.h:
nit: is it GL9763E or GL*I*9763?
Change the name to GL9763E.
Patch Set #1, Line 3: Driver
Definitions (the driver is the . […]
Done
#define PCI_VENDOR_ID_GLI 0x17a0
#define PCI_DEVICE_ID_GLI_9763E 0xe763
I'd suggest adding these to `src/include/device/pci_ids. […]
Done
Patch Set #1, Line 10: PCIE_GLI_9763E
I think one would expect these definitions to be for GL9763E, given the file they're in. […]
Remove prefix XX_GLI_9763E_XX. Done
What does `VHS` mean? I don't think it's one of these, right? 😄 https://en.wikipedia. […]
Ha 😄, you can call it this if you want.
When it is called "VHS", it will work. Therefore, it is called VHS.
(VHS should be Vendor Header Space.)
Patch Set #1, Line 11: GLI_9763E_VHS_REV
I'd add a `_MASK` suffix for clarity
Done
Patch Set #1, Line 11: 0xF00
(0xf << 16)
Done
#define GLI_9763E_VHS_REV_R 0x0
#define GLI_9763E_VHS_REV_M 0x1
#define GLI_9763E_VHS_REV_W 0x2
I would define these with the value already shifted: […]
Done
Patch Set #1, Line 26: GLI_9763E_PLL_CTL_2_MAX_SSC
I'd add a `_MASK` suffix for clarity
Done
File src/drivers/generic/genesyslogic/gli9763e.c:
static inline int pci_read_config_dword(struct device *dev, int where,
u32 *val)
{
*val = pci_read_config32(dev, where);
return 0;
}
static inline int pci_write_config_dword(struct device *dev, int where,
u32 val)
{
pci_write_config32(dev, where, val);
return 0;
}
Why add these wrappers?
Removed.
Patch Set #1, Line 27: gli_set_gl9763e
I'd prefer a more descriptive name for this function. […]
Done
We use `dev` nearly everywhere, even if it's a pointer.
Done
Patch Set #1, Line 33: (u32)
These casts aren't needed
Done
pci_read_config_dword(pdev, PCIE_GLI_9763E_SCR, &value);
value |= GLI_9763E_SCR_AXI_REQ;
pci_write_config_dword(pdev, PCIE_GLI_9763E_SCR, value);
Or, in a single line: […]
Done
pci_read_config_dword(pdev, PCIE_GLI_9763E_CFG_REG_2, &value);
value &= ~GLI_9763E_CFG_REG_2_L0S;
pci_write_config_dword(pdev, PCIE_GLI_9763E_CFG_REG_2, value);
Or, in a single line: […]
Done
Patch Set #1, Line 47: pci_write_config_dword(pdev, PCIE_GLI_9763E_PLL_CTL_2, value);
These could be done with `pci_update_config32` but it would be a very long line
Modified.
pci_read_config_dword(pdev, PCIE_GLI_9763E_PLL_CTL, &value);
value |= GLI_9763E_PLL_CTL_SSC;
pci_write_config_dword(pdev, PCIE_GLI_9763E_PLL_CTL, value);
Or, in a single line: […]
Done
Patch Set #1, Line 56: pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
I see this register is programmed twice with different values. […]
VHS has write protection. Enable writing and disable writing.
Thanks for your comments.
To view, visit change 43751. To unsubscribe, or for help writing mail filters, visit settings.