Hi Andrew,

On 24/10/23 8:03 am, Andrew Lunn wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

quoted

+static int lan865x_set_hw_macaddr(struct net_device *netdev)
+{
+     u32 regval;
+     bool ret;
+     struct lan865x_priv *priv = netdev_priv(netdev);
+     const u8 *mac = netdev->dev_addr;
+
+     ret = oa_tc6_read_register(priv->tc6, LAN865X_MAC_NCR, &regval);
+     if (ret)
+             goto error_mac;
+     if ((regval & LAN865X_TXEN) | (regval & LAN865X_RXEN)) {

Would testing netif_running(netdev) be enough? That is a more common
test you see. In fact, you already have that in
lan865x_set_mac_address(). And in lan865x_probe() why would the
hardware be enabled?

Ah yes, this check is not needed at all. Will correct it.

quoted

+             if (netif_msg_drv(priv))
+                     netdev_warn(netdev, "Hardware must be disabled for MAC setting\n");
+             return -EBUSY;
+     }
+     /* MAC address setting */
+     regval = (mac[3] << 24) | (mac[2] << 16) | (mac[1] << 8) | mac[0];
+     ret = oa_tc6_write_register(priv->tc6, LAN865X_MAC_SAB1, regval);
+     if (ret)
+             goto error_mac;
+
+     regval = (mac[5] << 8) | mac[4];
+     ret = oa_tc6_write_register(priv->tc6, LAN865X_MAC_SAT1, regval);
+     if (ret)
+             goto error_mac;
+
+     return 0;
+
+error_mac:
+     return -ENODEV;

oa_tc6_write_register() should return an error code, so return it.

Yes, noted. Will do it.

quoted

+static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
+{
+     struct sockaddr *address = addr;
+
+     if (netif_running(netdev))
+             return -EBUSY;
+
+     eth_hw_addr_set(netdev, address->sa_data);
+
+     return lan865x_set_hw_macaddr(netdev);

You should ideally only update the netdev MAC address, if you managed
to change the value in the hardware.

Ah ok, then it is supposed to be like below, isn't it?

static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
{
	struct sockaddr *address = addr;
	int ret;

	if (netif_running(netdev))
		return -EBUSY;

	ret = lan865x_set_hw_macaddr(netdev);
	if (ret)
		return ret;

	eth_hw_addr_set(netdev, address->sa_data);

	return 0;
}

quoted

+static void lan865x_set_multicast_list(struct net_device *netdev)
+{
+     struct lan865x_priv *priv = netdev_priv(netdev);
+     u32 regval = 0;
+
+     if (netdev->flags & IFF_PROMISC) {
+             /* Enabling promiscuous mode */
+             regval |= MAC_PROMISCUOUS_MODE;
+             regval &= (~MAC_MULTICAST_MODE);
+             regval &= (~MAC_UNICAST_MODE);
+     } else if (netdev->flags & IFF_ALLMULTI) {
+             /* Enabling all multicast mode */
+             regval &= (~MAC_PROMISCUOUS_MODE);
+             regval |= MAC_MULTICAST_MODE;
+             regval &= (~MAC_UNICAST_MODE);
+     } else if (!netdev_mc_empty(netdev)) {
+             /* Enabling specific multicast addresses */
+             struct netdev_hw_addr *ha;
+             u32 hash_lo = 0;
+             u32 hash_hi = 0;
+
+             netdev_for_each_mc_addr(ha, netdev) {
+                     u32 bit_num = lan865x_hash(ha->addr);
+                     u32 mask = 1 << (bit_num & 0x1f);
+
+                     if (bit_num & 0x20)
+                             hash_hi |= mask;
+                     else
+                             hash_lo |= mask;
+             }
+             if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRT, hash_hi)) {
+                     if (netif_msg_timer(priv))
+                             netdev_err(netdev, "Failed to write reg_hashh");
+                     return;
+             }
+             if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, hash_lo)) {
+                     if (netif_msg_timer(priv))
+                             netdev_err(netdev, "Failed to write reg_hashl");
+                     return;
+             }
+             regval &= (~MAC_PROMISCUOUS_MODE);
+             regval &= (~MAC_MULTICAST_MODE);
+             regval |= MAC_UNICAST_MODE;
+     } else {
+             /* enabling local mac address only */
+             if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRT, regval)) {
+                     if (netif_msg_timer(priv))
+                             netdev_err(netdev, "Failed to write reg_hashh");
+                     return;
+             }
+             if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, regval)) {
+                     if (netif_msg_timer(priv))
+                             netdev_err(netdev, "Failed to write reg_hashl");
+                     return;
+             }
+     }
+     if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_NCFGR, regval)) {
+             if (netif_msg_timer(priv))
+                     netdev_err(netdev, "Failed to enable promiscuous mode");
+     }
+}

Another of those big functions which could be lots of simple functions
each doing one thing.

Sure, will split into multiple functions.

quoted

+/* Configures the number of bytes allocated to each buffer in the
+ * transmit/receive queue. LAN865x supports only 64 and 32 bytes cps and also 64
+ * is the default value. So it is enough to configure the queue buffer size only
+ * for 32 bytes. Generally cps can't be changed during run time and also it is
+ * configured in the device tree. The values for the Tx/Rx queue buffer size are
+ * taken from the LAN865x datasheet.
+ */

Its unclear why this needs to be a callback. Why not just set it after
oa_tc6_init() returns?

oa_tc6_init() function internally calls oa_tc6_configure() function to 
configure different settings. At the end it writes SYNC bit to CONFIG0 
register which enables the MAC-PHY to start the data transfer. So the 
buffer configuration should be done prior to that. Since this is part of 
the configuration this callback is implemented.

quoted

+static void lan865x_remove(struct spi_device *spi)
+{
+     struct lan865x_priv *priv = spi_get_drvdata(spi);
+
+     oa_tc6_exit(priv->tc6);
+     unregister_netdev(priv->netdev);

Is that the correct order? Seems like you should unregister the netdev
first.

Ah yes, will correct it.

quoted

+#ifdef CONFIG_ACPI
+static const struct acpi_device_id lan865x_acpi_ids[] = {
+     { .id = "LAN865X",
+     },
+     {},

Does this work? You don't have access to the device tree properties.

Going to remove all the OA specific configurations in the next revisions.

Best Regards,
Parthiban V

      Andrew