RE: [PATCH iwl-next,v1 2/3] igc: Add default Rx queue configuration via sysfs

[Song, Yoong Siang]

On Tuesday, July 30, 2024 5:59 PM, Kurt Kanzenbach <kurt@xxxxxxxxxxxxx> wrote:
>> --- a/drivers/net/ethernet/intel/igc/igc_regs.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_regs.h
>> @@ -63,6 +63,12 @@
>>  /* RSS registers */
>>  #define IGC_MRQC		0x05818 /* Multiple Receive Control - RW */
>>
>> +/* MRQC register bit definitions */
>
>Nit: Now, the MRQC register definitions are scattered over two files:
>igc_regs.h and igc.h. igc.h has
>
>#define IGC_MRQC_ENABLE_RSS_MQ		0x00000002
>#define IGC_MRQC_RSS_FIELD_IPV4_UDP	0x00400000
>#define IGC_MRQC_RSS_FIELD_IPV6_UDP	0x00800000
>
>Maybe combine them into a single location?
>

Hi Kurt Kanzenbach,

Thanks for your review comment.
Sure, I will try to combine them into single location.

>> +#define IGC_MRQC_ENABLE_MQ		0x00000000
>> +#define IGC_MRQC_ENABLE_MASK		GENMASK(2, 0)
>> +#define IGC_MRQC_DEFAULT_QUEUE_MASK	GENMASK(5, 3)
>> +#define IGC_MRQC_DEFAULT_QUEUE_SHIFT	3
>
>Nit: FIELD_GET() and FIELD_PREP() can help to get rid of the manual
>shifting. See below.
>

Noted.
[...]

>> +	return (mrqc & IGC_MRQC_DEFAULT_QUEUE_MASK) >>
>> +		IGC_MRQC_DEFAULT_QUEUE_SHIFT;
>
>Nit: return FIELD_GET(IGC_MRQC_DEFAULT_QUEUE_MASK, mrqc);
>

Noted.
[...]

>> +	mrqc &= ~IGC_MRQC_DEFAULT_QUEUE_MASK;
>> +	mrqc |= queue << IGC_MRQC_DEFAULT_QUEUE_SHIFT;
>
>Nit: mrqc |= FIELD_PREP(IGC_MRQC_DEFAULT_QUEUE_MASK, queue);
>

Noted.
[...]

>Thanks,
>Kurt

Thanks & Regards
Siang