Welcome!
Patch set 1:Code-Review +1
21 comments:
Patch Set #1, Line 7: generic/genesyslogic
I don't think the GL9763E is a "generic" device. I'd put it inside `drivers/genesyslogic/` instead.
Patch Set #1, Line 7: GL9763E
Could you please mention in the commit message which kind of device the GL9763E is? I couldn't find a datasheet anywhere, only some Linux patches that hint at it being a PCIe to eMMC controller: https://lkml.org/lkml/2020/5/8/160
File src/drivers/generic/genesyslogic/Kconfig:
Patch Set #1, Line 1: config DRIVERS_GENERIC_GLI9763E
Note that, since nothing currently selects this option, the code hasn't been build-tested by Jenkins. If this driver will be used by some board (I guess a chromebook?), it would be good to make a follow-up change (a new commit right after this one) that selects this option.
File src/drivers/generic/genesyslogic/gli9763e.h:
nit: is it GL9763E or GL*I*9763?
Patch Set #1, Line 3: Driver
Definitions (the driver is the .c file)
#define PCI_VENDOR_ID_GLI 0x17a0
#define PCI_DEVICE_ID_GLI_9763E 0xe763
I'd suggest adding these to `src/include/device/pci_ids.h`
What does `VHS` mean? I don't think it's one of these, right? 😄 https://en.wikipedia.org/wiki/VHS
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. The repeated prefix only makes the definitions long and easy to mix up 😕
Patch Set #1, Line 11: 0xF00
(0xf << 16)
Patch Set #1, Line 11: GLI_9763E_VHS_REV
I'd add a `_MASK` suffix for clarity
#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:
#define GLI_9763E_VHS_REV_R (0x0 << 16)
#define GLI_9763E_VHS_REV_M (0x1 << 16)
#define GLI_9763E_VHS_REV_W (0x2 << 16)
Also, please add an extra space after `#define` to make it clear that these are fields within a register
Patch Set #1, Line 26: GLI_9763E_PLL_CTL_2_MAX_SSC
I'd add a `_MASK` suffix for clarity
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?
Patch Set #1, Line 27: gli_set_gl9763e
I'd prefer a more descriptive name for this function. `gl9763e_init`, for instance?
We use `dev` nearly everywhere, even if it's a pointer.
Patch Set #1, Line 33: (u32)
These casts aren't needed
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:
pci_or_config32(dev, PCIE_GLI_9763E_SCR, GLI_9763E_SCR_AXI_REQ);
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:
pci_and_config32(dev, PCIE_GLI_9763E_CFG_REG_2, ~GLI_9763E_CFG_REG_2_L0S);
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
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:
pci_or_config32(dev, PCIE_GLI_9763E_PLL_CTL, GLI_9763E_PLL_CTL_SSC);
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. It would be good to know why this is needed.
To view, visit change 43751. To unsubscribe, or for help writing mail filters, visit settings.