Hello, Loc. On Sun, Jan 12, 2014 at 08:01:59PM -0800, Loc Ho wrote: > Yes but Let me summary what overrides are required for this X-Gene > SATA controller driver: > > 1. For Query ID, these two functions - ahci_read_id and ahci_qc_issue > requires override. But the comment in ahci_qc_issue() says it's for PMP. > 2. For PMP support, these two functions - ahci_qc_prep and > ahci_qc_fill_rtf requires override. But the only difference between xgene_ahci_do_softreset() and ahci_do_softreset() is that the former removes fbs handling from the latter. Why not just turn off HOST_CAP_FBS? > 3. For softreset, softreset requires override to add additional flags > in call to ahci_exec_polled_cmd. which is what? It's the same for the xgene and normal functions. > 4. For IRQ and ensure data consistent for read operation, function > ahci_interrupt and ahci_port_intr requires override. But it only > requires an flush call right after reading from the CI register. > 5. For hardreset, no need to explain this one as all SATA drivers > require its only method. No, they don't and the comments in your driver don't really explain what's going on. Why are we having retry loops inside hardreset itself? This can prolong recovery time significantly in corner cases. Why is this necessary? I'm having a lot of trouble understanding what you're trying to do with your driver. Maybe it'd help to submit as multiple patches where the first one implements bare minimum to function and then later patches adding more workarounds to achieve more functionalities? I can't apply the patch as it currently stands. Some general comments. * Let's use fully winged comments for multiline comments. * I really don't like wrapping basic functions like readl/writel() in low level drivers. * If your driver needs to append or prepent something to the existing one, export the existing one and then wrap that one with the extra logic around it. Do not copy function body unnecessarily. Even for the port_interrupt, it looks like you can just call the flush after invoking the normal ahci_handle_port_interrupt(), no? -- tejun -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
