Hello. On Mon, Jun 20, 2011 at 4:19 PM, Tejun Heo <tj@xxxxxxxxxx> wrote: > Hello, Yuan. > > Yeap, almost there, just few very minor things. :) > > On Mon, Jun 20, 2011 at 04:06:41PM +0800, Yuan-Hsin Chen wrote: >> static struct ata_port_operations ahci_sb600_ops = { >> .inherits = &ahci_ops, >> - .softreset = ahci_sb600_softreset, >> - .pmp_softreset = ahci_sb600_softreset, >> + .softreset = ahci_pmp_retry_srst_softreset, >> }; > > Why doesn't sb600 just use ahci_pmp_retry_srst_ops? > >> @@ -312,6 +312,7 @@ extern struct device_attribute *ahci_sdev_attrs[]; >> .sdev_attrs = ahci_sdev_attrs >> >> extern struct ata_port_operations ahci_ops; >> +extern struct ata_port_operations ahci_pmp_retry_srst_ops; > > Heh, I know I suggested that name but that is one ugly name. If > anyone has any better idea, please come forward. How about ahci_pmp_workaround_softreset or ahci_pmp_ipms_set_softreset? Also applys to ops, ahci_pmp_workaround_ops or ahci_pmp_ipms_set_ops. > >> @@ -82,6 +82,8 @@ static void ahci_pmp_attach(struct ata_port *ap); >> static void ahci_pmp_detach(struct ata_port *ap); >> static int ahci_softreset(struct ata_link *link, unsigned int *class, >> unsigned long deadline); >> +static int ahci_pmp_retry_srst_softreset(struct ata_link *link, unsigned int *class, >> + unsigned long deadline); > > srst equals softreset, so ahci_pmp_retry_softreset() should do. > >> +struct ata_port_operations ahci_pmp_retry_srst_ops = { >> + .inherits = &ahci_ops, >> + .softreset = ahci_pmp_retry_srst_softreset, >> +}; >> +EXPORT_SYMBOL_GPL(ahci_pmp_retry_srst_ops); >> + >> struct ata_port_operations ahci_ops = { >> .inherits = &sata_pmp_port_ops, > > Maybe it's better to place ahci_pmp_retry_srst_ops after ahci_ops? > >> +static int ahci_pmp_check_ready(struct ata_link *link) > > It would be better if the name reflects that it's not for specific > workaround. > > Thank you. > > -- > tejun > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html