<wei_wang@xxxxxxxxxxxxxx> writes:
> +static bool msi_en = 1;
> +module_param(msi_en, bool, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(msi_en, "Enable MSI");
> +
> +static bool adma_mode = 1;
> +module_param(adma_mode, bool, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(adma_mode, "ADMA Mode");
Why would I want to disable these features? And what if I have two
devices and want different settings for them?
> +int rtsx_pci_read_register(struct rtsx_pdev *pdev, u16 addr, u8 *data)
> +{
> + u32 val = 2 << 30;
> + int i;
> +
> + if (data)
> + *data = 0;
Why would anyone want to call this function with a NULL pointer?
> +
> + val |= (u32)(addr & 0x3FFF) << 16;
> + rtsx_pci_writel(pdev, RTSX_HAIMR, val);
> +
> + for (i = 0; i < MAX_RW_REG_CNT; i++) {
> + val = rtsx_pci_readl(pdev, RTSX_HAIMR);
> + if ((val & (1 << 31)) == 0)
> + break;
> + }
> +
> + if (i >= MAX_RW_REG_CNT)
> + return -ETIMEDOUT;
> +
> + if (data)
> + *data = (u8)(val & 0xFF);
And even if they did, why do go through the read and then check again?
Register reading side effects? Would be nice if that was mentioned in a
comment.
> + pr_debug("SG table count = %d\n", pdev->sgi);
dev_dbg here and many other places, maybe? Always nice to see which
device is spitting out such messages.
> + BUG_ON(!buf || (buf_len <= 0));
OK? And then I do what? Give you a call?
> + pr_info("%s: pdev->msi_en = %d, pci->irq = %d\n",
> + __func__, pdev->msi_en, pdev->pci->irq);
Same as for the debugging: dev_info is nicer.
> + pr_err("rtsx_sdmmc: unable to grab IRQ %d, disabling device\n",
> + pdev->pci->irq);
Likewise for other levels.
> +static unsigned char get_card_type(u32 card_status)
> +{
> + unsigned char type = 0;
> +
> + switch (card_status) {
> + case XD_EXIST:
> + type = RTSX_TYPE_XD;
> + break;
> +
> + case MS_EXIST:
> + type = RTSX_TYPE_MS;
> + break;
> +
> + case SD_EXIST:
> + type = RTSX_TYPE_SD;
> + break;
> +
> + default:
> + type = 0;
> + break;
Seems a bit redundant given that you initialized it to 0.
> +static u32 get_card_status(unsigned char type)
> +{
> + u32 card_status = 0;
> +
> + switch (type) {
> + case RTSX_TYPE_XD:
> + card_status = XD_EXIST;
> + break;
> +
> + case RTSX_TYPE_MS:
> + card_status = MS_EXIST;
> + break;
> +
> + case RTSX_TYPE_SD:
> + card_status = SD_EXIST;
> + break;
> +
> + default:
> + card_status = 0;
> + break;
Same as above.
> +static int rtsx_pci_extra_init_hw(struct rtsx_pdev *pdev)
> +{
> + pr_warn("%s\n", __func__);
> + return 0;
> +}
> +
> +static int rtsx_pci_optimize_phy(struct rtsx_pdev *pdev)
> +{
> + pr_warn("%s\n", __func__);
> + return 0;
> +}
> +
> +static void rtsx_pci_turn_on_led(struct rtsx_pdev *pdev)
> +{
> + pr_warn("%s\n", __func__);
> +}
> +
> +static void rtsx_pci_turn_off_led(struct rtsx_pdev *pdev)
> +{
> + pr_warn("%s\n", __func__);
> +}
> +
> +static void rtsx_pci_enable_auto_blink(struct rtsx_pdev *pdev)
> +{
> + pr_warn("%s\n", __func__);
> +}
> +
> +static void rtsx_pci_disable_auto_blink(struct rtsx_pdev *pdev)
> +{
> + pr_warn("%s\n", __func__);
> +}
Can all these stubs really be necessary?
> +static void rtsx_pci_init_ops(struct rtsx_pdev *pdev)
> +{
> + switch (PCI_PID(pdev)) {
> + case 0x5209:
> + pr_info("Initialize 0x5209\n");
> + pdev->ops.extra_init_hw = rts5209_extra_init_hw;
> + pdev->ops.optimize_phy = rts5209_optimize_phy;
> + pdev->ops.turn_on_led = rts5209_turn_on_led;
> + pdev->ops.turn_off_led = rts5209_turn_off_led;
> + pdev->ops.enable_auto_blink = rts5209_enable_auto_blink;
> + pdev->ops.disable_auto_blink = rts5209_disable_auto_blink;
> + break;
> +
> + case 0x5229:
> + pr_info("Initialize 0x5229\n");
> + pdev->ops.extra_init_hw = rts5229_extra_init_hw;
> + pdev->ops.optimize_phy = rts5229_optimize_phy;
> + pdev->ops.turn_on_led = rts5229_turn_on_led;
> + pdev->ops.turn_off_led = rts5229_turn_off_led;
> + pdev->ops.enable_auto_blink = rts5229_enable_auto_blink;
> + pdev->ops.disable_auto_blink = rts5229_disable_auto_blink;
> + break;
> +
> + default:
> + pr_warn("Initialize dummy ops\n");
> + pdev->ops.extra_init_hw = rtsx_pci_extra_init_hw;
> + pdev->ops.optimize_phy = rtsx_pci_optimize_phy;
> + pdev->ops.turn_on_led = rtsx_pci_turn_on_led;
> + pdev->ops.turn_off_led = rtsx_pci_turn_off_led;
> + pdev->ops.enable_auto_blink = rtsx_pci_enable_auto_blink;
> + pdev->ops.disable_auto_blink = rtsx_pci_disable_auto_blink;
> + }
Maybe three static "pdev_ops" structs, and make pdev->ops a pointer?
> +static int rtsx_pci_init_hw(struct rtsx_pdev *pdev)
> +{
> + int err;
> +
> + rtsx_pci_writel(pdev, RTSX_HCBAR, pdev->host_cmds_addr);
> +
> + rtsx_pci_enable_bus_int(pdev);
> +
> + /* Power on SSC */
> + err = rtsx_pci_write_register(pdev, FPDCTL, SSC_POWER_DOWN, 0);
> + if (err < 0)
> + return err;
> +
> + udelay(200);
Why? Yes, I can guess but it's always nice to have a small comment
documenting why you insert such things. In particular how the timeout
was selected. Is this based on a datasheet recommendation, or just some
guesstimate?
> +
> + err = pdev->ops.optimize_phy(pdev);
> + if (err < 0)
> + return err;
Is there any chance this would fail because the poweron timeout was too
short? If so, then maybe you should wait and retry?
> +static int __init rtsx_pci_drv_init(void)
> +{
> + pr_info(DRV_NAME ": Realtek PCI-E Card Reader adapter driver\n");
This is unnecessary noise.
> + for (i = 0xFDA0; i <= 0xFDAE; i++)
> + rtsx_pci_add_cmd(pdev, READ_REG_CMD, i, 0, 0);
> + for (i = 0xFD52; i <= 0xFD69; i++)
> + rtsx_pci_add_cmd(pdev, READ_REG_CMD, i, 0, 0);
These constants look like magic to me. Maybe add a comment or use a
macro to give them a describing name?
> + rtsx_pci_send_cmd(pdev, 100);
> +
> + ptr = rtsx_pci_get_cmd_data(pdev);
> + for (i = 0xFDA0; i <= 0xFDAE; i++)
> + pr_debug("0x%04X: 0x%02x\n", i, *(ptr++));
> + for (i = 0xFD52; i <= 0xFD69; i++)
> + pr_debug("0x%04X: 0x%02x\n", i, *(ptr++));
And they are repeated, so macros would help avoiding errors in any case.
> +static int sd_wait_voltage_stable_1(struct rtsx_pdev *pdev)
> +{
> + int err;
> + u8 stat;
> +
> + mdelay(1);
Another timeout needing explanation.
> +
> + /* SD_CMD, SD_DAT3~0 should be drived to low by card;
> + * If either one of SD_CMD,SD_DAT3~0 is not low,
> + * abort the voltage switch sequence;
> + */
> + err = rtsx_pci_read_register(pdev, SD_BUS_STAT, &stat);
> + if (err < 0)
> + return err;
> +
> + if (stat & (SD_CMD_STATUS | SD_DAT3_STATUS | SD_DAT2_STATUS |
> + SD_DAT1_STATUS | SD_DAT0_STATUS))
> + return -EINVAL;
> +
> + /* Stop toggle SD clock */
> + err = rtsx_pci_write_register(pdev, SD_BUS_STAT,
> + 0xFF, SD_CLK_FORCE_STOP);
> + if (err < 0)
> + return err;
> +
> + return 0;
> +}
> +
> +static int sd_wait_voltage_stable_2(struct rtsx_pdev *pdev)
> +{
> + int err;
> + u8 stat, mask, val;
> +
> + wait_timeout(50);
And another one.
> +
> + /* Toggle SD clock again */
> + err = rtsx_pci_write_register(pdev, SD_BUS_STAT,
> + 0xFF, SD_CLK_TOGGLE_EN);
> + if (err < 0)
> + return err;
> +
> + wait_timeout(10);
And one more. I realize that this may be obviously correct to anyone
understanding what's going on here, but you need to write this so that
*I* can understand it :-)
> + if (CHK_PCI_PID(pdev, 0x5209))
> + ldo_off = 0x06;
> + else
> + ldo_off = 0x00;
Hmm, I didn't expect any pid checks here. Could this deserve a field in
the device struct so that it could be set up at init time, or would that
be a waste?
> +static int pci_sdmmc_send_cmd_get_rsp(struct rtsx_adapter *adapter, u8 cmd_idx,
> + u32 arg, unsigned int resp_type, u32 *resp)
> +{
> + struct rtsx_pdev *pdev = dev_get_drvdata(adapter->dev.parent);
> + int err = 0;
> + int timeout = 100;
> + int i;
> + u8 *ptr;
> + int stat_idx = 0;
> + u8 rsp_type;
> + int rsp_len = 5;
> +
> + BUG_ON(!resp);
Yuck!
Just a few random comments from your random reader. Use it as you
like. I didn't really read it all. It's a huge driver...
Bjørn
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
