Hi Andrew, On 24/04/24 5:57 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> +/* OPEN Alliance Configuration Register #0 */ >> +#define OA_TC6_REG_CONFIG0 0x0004 >> +#define CONFIG0_ZARFE_ENABLE BIT(12) > > If this is a standard register, you should put these defined where > other drivers can use them. OK. Will move it to oa_tc6.c in the next version as you have also suggested to use this as a helper function in the below comment. > >> +static int lan865x_set_mac_address(struct net_device *netdev, void *addr) >> +{ >> + struct lan865x_priv *priv = netdev_priv(netdev); >> + struct sockaddr *address = addr; >> + int ret; >> + >> + ret = eth_prepare_mac_addr_change(netdev, addr); >> + if (ret < 0) >> + return ret; >> + >> + if (ether_addr_equal(address->sa_data, netdev->dev_addr)) >> + return 0; >> + >> + ret = lan865x_set_hw_macaddr(priv, address->sa_data); >> + if (ret) >> + return ret; >> + >> + eth_hw_addr_set(netdev, address->sa_data); > > It seems more normal to call eth_commit_mac_addr_change(), which > better pairs with eth_prepare_mac_addr_change(). OK, so I will replace eth_hw_addr_set() with eth_prepare_mac_addr_change() in the next version. > >> +static int lan865x_set_zarfe(struct lan865x_priv *priv) >> +{ >> + u32 regval; >> + int ret; >> + >> + ret = oa_tc6_read_register(priv->tc6, OA_TC6_REG_CONFIG0, ®val); >> + if (ret) >> + return ret; >> + >> + /* Set Zero-Align Receive Frame Enable */ >> + regval |= CONFIG0_ZARFE_ENABLE; >> + >> + return oa_tc6_write_register(priv->tc6, OA_TC6_REG_CONFIG0, regval); >> +} > > There does not appear to be anything specific to your device here. So > please make this a helper in the shared code, so any driver can use > it. OK, I will implement this helper function as oa_tc6_enable_zarfe() in the oa_tc6.c. Also do you want me to move this helper function implementation to a new patch? Best regards, Parthiban V > > Andrew >