Re: [PATCH] Signed-off-by: Runa Guo-oc <RunaGuo-oc@xxxxxxxxxxx>

[Damien Le Moal]

On 6/27/23 16:17, Runa Guo-oc wrote:
> 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.

That is fine. I do not need a reason for the name. what I would like to see is
information about which product/board/soc this driver will be needed for. So
something like the above is actually fine (may be a little more details if you
have).

> 
>>>
>>>> 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.

Fine. Just say so in the Kconfig entry then.
If in the future your company produces a different chipset model that needs a
different driver, then the entries can be clearly differentiated even if they
use the same company name (ZhaoXin). E.g. "ZhaoXin ZX-100S and ZX-200 chipset
support" vs "ZhaoXin XYZ-newgen chipset support". Adding entries should not
require modifying existing entries.



-- 
Damien Le Moal
Western Digital Research