On 6/26/23 20:29, Runa Guo-oc wrote: > On 2023/6/16 16:34, Damien Le Moal wrote: >> On 6/16/23 16:49, Runa Guo-oc wrote: >>> [PATCH] ata:sata_zhaoxin: Add support for ZhaoXin Serial ATA >> >> Broken patch: the email subject is your SoB instead of the above line, which >> should not be part of the message (it should be the subject). Please reformat >> and resend. >> > > Okay. > >>> >>> Add ZhaoXin Serial ATA support for ZhaoXin CUPs. >> >> What is "ZhaoXin" ? > > Zhaoxin is a Chinese company that has mastered the core technology > of independent general-purpose processors and its system platform chips, > and is committed to providing users with efficient, compatible and secure > independent general-purpose processors, chipsets and other products. > for more information, you can visit here: https://www.zhaoxin.com/. A company marketing message is not very informative, technically speaking. What is this chipset and on what board/machine can it be found ? That is the more relevant information we need in this commit message. > >> And what is "CUPs" ? Please spell this out. >> > > Yes, this is a spelling error. > >>> >>> Signed-off-by: Runa Guo-oc <RunaGuo-oc@xxxxxxxxxxx> >>> --- >>> drivers/ata/Kconfig | 8 + >>> drivers/ata/Makefile | 1 + >>> drivers/ata/sata_zhaoxin.c | 384 +++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 393 insertions(+) >>> create mode 100644 drivers/ata/sata_zhaoxin.c >>> >>> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig >>> index 42b51c9..ae327f3 100644 >>> --- a/drivers/ata/Kconfig >>> +++ b/drivers/ata/Kconfig >>> @@ -553,6 +553,14 @@ config SATA_VITESSE >>> >>> If unsure, say N. >>> >>> +config SATA_ZHAOXIN >>> + tristate "ZhaoXin SATA support" Same here. If ZhaoXin is only a company name, we need also a chipset model to be informative regarding which HW this driver serves. >>> + depends on PCI >>> + help >>> + This option enables support for ZhaoXin Serial ATA. >>> + >>> + If unsure, say N. >>> + >>> comment "PATA SFF controllers with BMDMA" >>> >>> config PATA_ALI >>> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile >>> index 20e6645..4b84669 100644 >>> --- a/drivers/ata/Makefile >>> +++ b/drivers/ata/Makefile >>> @@ -45,6 +45,7 @@ obj-$(CONFIG_SATA_SIL) += sata_sil.o >>> obj-$(CONFIG_SATA_SIS) += sata_sis.o >>> obj-$(CONFIG_SATA_SVW) += sata_svw.o >>> obj-$(CONFIG_SATA_ULI) += sata_uli.o >>> +obj-$(CONFIG_SATA_ZHAOXIN) += sata_zhaoxin.o >> >> Please sort this alphabetically. >> > > Like this? > obj-$(CONFIG_SATA_VITESSE) += sata_vsc.o > obj-$(CONFIG_SATA_ZHAOXIN) += sata_zhaoxin.o Yes. >>> + >>> + ata_link_err(link, "COMRESET success (errno=%d) ap=%d link %d\n", >>> + rc, link->ap->port_no, link->pmp); >>> + } else { >>> + ata_link_err(link, "COMRESET failed (errno=%d) ap=%d link %d\n", >>> + rc, link->ap->port_no, link->pmp); >> >> Reverse the if condition and exit early for this case. That will make the >> function code nicer. And you can drop the error message as well since >> sata_std_hardreset() prints one. >> > > Yes, I agree with your. I'll adjust the above codes like these? > > if(!rc || rc == -EAGAIN){ > struct ata_port *ap = link->ap; > int pmp = link->pmp; > int tmprc; > if(pmp){ > ap->ops->sff_dev_select(ap,pmp); > tmprc=ata_sff_wait_ready(&ap->link,deadline); > }else > tmprc=ata_sff_wait_ready(link,deadline); > > if(tmprc) > ata_link_err(link,"COMRESET failed for > wait(errno=%d)\n",rc); > > ata_link_err(link,"COMRESET success (errno=%d) ap=%d > link%d\n", > rc,link->ap->port_no,link->pmp); > You did not understand my point. Doing: rc = sata_std_hardreset(link, class, deadline); if (rc && rc != -EAGAIN) { ata_link_err(link, "COMRESET failed (errno=%d) ap=%d link %d\n", rc, link->ap->port_no, link->pmp); return rc; } /* rc == 0 || rc == -EAGAIN case */ ... Makes the code much nicer. [...] >>> +MODULE_AUTHOR("Yanchen:YanchenSun@xxxxxxxxxxx"); >>> +MODULE_DESCRIPTION("SCSI low-level driver for ZX SATA controllers"); >> >> This is not a scsi driver... >> > > I treat it as a scsi driver for the following reasons, which may be not > accurate. > 1, IO path: vfs->fs->block layer->scsi layer->this driver; > 2, Extracted from the following link: > "SCSI Lower level drivers (LLDs) are variously called host bus adapter > (HBA) drivers and host drivers (HD)." Again, this is not a scsi driver. Even if in Linux all ATA drives seat under the scsi layer, this is an ATA subsytem driver for an ATA drive. Not SCSI. -- Damien Le Moal Western Digital Research
