Re: [PATCH] nvme: use BIT_MASK and GENMASK for NVME definitions

[Tokunori Ikegami <ikegami.t@gmail.com>]

On 2024/11/12 13:26, Christoph Hellwig wrote:
>> -	NVME_CMBSZ_CQS		= 1 << 1,
>> -	NVME_CMBSZ_LISTS	= 1 << 2,
>> -	NVME_CMBSZ_RDS		= 1 << 3,
>> -	NVME_CMBSZ_WDS		= 1 << 4,
>> +	NVME_CMBSZ_SQS		= BIT_MASK(0),
>> +	NVME_CMBSZ_CQS		= BIT_MASK(1),
>> +	NVME_CMBSZ_LISTS	= BIT_MASK(2),
>> +	NVME_CMBSZ_RDS		= BIT_MASK(3),
>> +	NVME_CMBSZ_WDS		= BIT_MASK(4),
> Nothjing genmask here, and a lot less readable for no good reason at
> all.
Is it still no good to change BIT_MASK() to BIT()?
>>   	NVME_CMBSZ_SZ_SHIFT	= 12,
>> -	NVME_CMBSZ_SZ_MASK	= 0xfffff,
>> +	NVME_CMBSZ_SZ_MASK	= GENMASK(19, 0),
> This is using the GENMASK that you mentioned, and now I actually
> need to look up what GENMASK does to decipher the previously perfectly
> understandable code.
-       NVME_CMBSZ_SZ_SHIFT     = 12,
-       NVME_CMBSZ_SZ_MASK      = 0xfffff,
+       NVME_CMBSZ_SZ_MASK      = GENMASK(31, 12),
Is this also still not understandable?
> Could people please stop sending or suggesting cleanups that make
> the code much worse?

Sorry for bothering you. Also the changes were not considered enough but 
still seems better to use the existing bit operation macros instead of 
the current implementation. Also thinking to change to use FIELD_GET() 
and FIELD_PREP() macros. If possible to make sure let me reconfirm your 
opinion for above. Thank you.