On 2023/6/26 20:40, Damien Le Moal wrote: > 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. > The reason why it is called Zhaoxin SATA is actually to express that it applies to all Zhaoxin support of its separate chipset/SOC, for example, ZX-100S/ZX-200 chipsets. >> >>> 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. > As mentioned before, the chipset models this driver serves are ZX-100S and ZX-200 currently. >>>> + 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. > > > [...] > Okay, I'll adjust it like this? ... struct ata_port *ap = link->ap; int pmp = link->pmp; int tmprc; 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; } 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); return rc; >>>> +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. > Okay, I'll remove this inaccurate description next time. Thank you.
