Re: [PATCH 1/2] nvme: Cleanup the definition of the controller config register fields

[Damien Le Moal <dlemoal@kernel.org>]

On 2025/02/13 15:00, Christoph Hellwig wrote:
>> +++ b/include/linux/nvme.h
>> @@ -202,24 +202,35 @@ enum {
>>  enum {
>>  	NVME_CC_ENABLE		= 1 << 0,
>>  	NVME_CC_EN_SHIFT	= 0,
>> +
>>  	NVME_CC_CSS_SHIFT	= 4,
>> -	NVME_CC_MPS_SHIFT	= 7,
>> -	NVME_CC_AMS_SHIFT	= 11,
>> -	NVME_CC_SHN_SHIFT	= 14,
>> -	NVME_CC_IOSQES_SHIFT	= 16,
>> -	NVME_CC_IOCQES_SHIFT	= 20,
>> +	NVME_CC_CSS_MASK	= 7 << NVME_CC_CSS_SHIFT,
> 
> While you're at it, maybe use a separate anonymous enum for each
> field and a comment about which field it describes?

I find the single enum for all the fields of the same u32 register to be a nice
grouping of things. But I agree that more comments would indeed be nice.
I can add that. But do we really need separate enums for each field ?


-- 
Damien Le Moal
Western Digital Research