On Fri, Nov 08, 2024 at 02:55:18PM +0200, Roger Quadros wrote: > Hi Siddharth, > > On 08/11/2024 14:30, Siddharth Vadapalli wrote: [...] > >> +#define AM65_CPSW_PORTN_REG_CTL 0x004 > > > > nitpick: indentation needs to be fixed here to align with the macros > > below. > > It is fine in the code and in my editor in this reply email. That's strange. But it appears the same to me as seen at: https://lore.kernel.org/r/20241105-am65-cpsw-multi-rx-dscp-v1-2-38db85333c88@xxxxxxxxxx/ where the indentation looks incorrect. [...] > > >> + > >> + if (dscp > AM65_CPSW_DSCP_MAX) > >> + return -EINVAL; > > > > am65_cpsw_port_set_dscp_map() seems to be invoked by > > am65_cpsw_port_enable_dscp_map() below, where the above check is guaranteed > > to be satisfied. Is the check added for future-proofing this function? > > > > Right, future callers can't be guaranteed to do the check so I'd prefer > to have the check here. Thank you for the confirmation. > > >> + > >> + if (pri > AM65_CPSW_PRI_MAX) > >> + return -EINVAL; > >> + > >> + reg_ofs = (dscp / 8) * 4; /* reg offset to this dscp */ > >> + bit_ofs = 4 * (dscp % 8); /* bit offset to this dscp */ > > > > Maybe a macro can be used for the "4" since it is not clear what it > > First 4 was for 4 bytes per register. Not sure if we need a macro for this. > The comment already mentions register offset and we know each register is > 32-bits wide. > > We could add a macro for the 8 though > #define AM65_CPSW_DSCP_PRI_PER_REG 8 > > The second 4 is actually 4 bits per DSCP field. I could add a macro for this. > #define AM65_CPSW_DSCP_PRI_FIELD_WIDTH 4 This looks good to me, but I am fine either way, in case you prefer to drop the macros. > > > > corresponds to. Or maybe two macros can be used for "reg_ofs" and > > "bit_ofs". > > > >> + val = readl(slave->port_base + AM65_CPSW_PORTN_REG_DSCP_MAP + reg_ofs); > >> + val &= ~(AM65_CPSW_PRI_MAX << bit_ofs); /* clear */ > >> + val |= pri << bit_ofs; /* set */ > >> + writel(val, slave->port_base + AM65_CPSW_PORTN_REG_DSCP_MAP + reg_ofs); > >> + val = readl(slave->port_base + AM65_CPSW_PORTN_REG_DSCP_MAP + reg_ofs); > > > > The above readback seems to be just to flush the writel(). A comment of > > the form: > > /* flush */ > > might help, considering that other drivers do the same. Also, assigning > > the returned value to "val" might not be required unless it is intended to > > be checked. > > This was actually left over debug code. I'll drop the readl. Ok. Regards, Siddharth.
