On 21/10/2022 08:47, Ping-Ke Shih wrote: > > >> -----Original Message----- >> From: Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> >> Sent: Monday, October 17, 2022 1:27 AM [...] >> + >> + val32 &= ~0x007ff800; >> + val32 |= (crystal_cap | (crystal_cap << 6)) << 11; >> + rtl8xxxu_write32(priv, REG_AFE_XTAL_CTRL, val32); > > This could be clear: > > #define XTAL1 GENMASK(22, 17) > #define XTAL0 GENMASK(16, 11) > > val32 &= ~(XTAL1 | XTAL2) > val32 |= FIELD_PREP(XTAL1, crystal_cap) | > FIELD_PREP(XTAL0, crystal_cap); > Ah, so that's what FIELD_PREP does. >> + if (priv->cfo_tracking.packet_count == 0xffffffff) >> + priv->cfo_tracking.packet_count = 0; >> + else >> + priv->cfo_tracking.packet_count++; > > 'packet_count' is u32, so 0xffffffff + 1 will become 0. Then, just > > priv->cfo_tracking.packet_count++; > I thought so, but I'm not *that* familiar with the C standard, so I left it how I found it in one of the Realtek drivers. >> + >> + if (crystal_cap > 0x3f) >> + crystal_cap = 0x3f; >> + else if (crystal_cap < 0) >> + crystal_cap = 0; > > crystal_cap = clamp(crystal_cap, 0, 0x3f); > Right, that's better. Thanks for the reviews. I'll implement your suggestions.