Re: [PATCH net-next v12 06/15] ax88179_178a: Add HW support for AX179A-based chips

[Birger Koblitz <mail@birger-koblitz.de>]

On 20/09/2026 12:12 am, Jakub Kicinski wrote:
> On Wed, 16 Sep 2026 05:24:05 +0200 Birger Koblitz wrote:
> 
>> +	/* Stop RX/TX for link configuration */
>> +	tmp16 = AX_RX_CTL_STOP;
>> +	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, &tmp16);
> 
> don't you want to add a wrapper for the commands?
> To write a cmd from a literal/immediate value?
> Something like:
> 
> 	ax88179_cmd_imm(dev, AX_ACCESS_MAC, AX_RX_CTL, AX_RX_CTL_STOP,
> 	2);
> 
> It'd save you all the temp buffers, which TBH I'm not sure are fully
> correct since you cast u8 buf to a pointer which may need alignment.
> 
Will do, great suggestion!
I added ax88179_write_u16() and ax88179_write_u8() which indeed 
eliminate many of the temporary variables.


>> +	tmp8 = 0;
>> +	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_MAC_PATH, 1, 1, &tmp8);
>> +
>> +	tmp8 = 0xa5;
>> +	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_MAC_CDC_DELAY_TX, 1, 1, &tmp8);
> 
> Please wrap at 80 chars if it doesn't impact readability.
Doing so as part of the introduction of the new write functions.


> 
>> +	/* Check these packets */
>> +	hdr_off = (rx_hdr & AX179A_RX_DH_DESC_OFFSET_MASK) >> AX179A_RX_DH_DESC_OFFSET_SHIFT;
>> +	pkt_cnt = rx_hdr & AX179A_RX_DH_PKT_CNT_MASK;
> 
> Please define shifted masks and use FIELD_GET() FIELD_PREP()
> Sometimes your masks are shifted sometimes not, makes the code harder
> to follow. Not to mention that this is much shorter:
> 
> 	hdr_off = FILED_GET(AX179A_RX_DH_DESC_OFFSET, rx_hdr);
> 
> 
>> +	/* TSO MSS */
>> +	tx_desc |= ((u64)(skb_shinfo(skb)->gso_size & AX179A_TX_DESC_MSS_MASK)) <<
>> +		   AX179A_TX_DESC_MSS_SHIFT;
> 
> (eg. this mask is not shifted)
Done in next revision.