Re: [RFC PATCH net-next 08/10] net: hibmcge: Implement workqueue and some ethtool_ops functions

[Andrew Lunn]

> +static void hbg_ethtool_get_drvinfo(struct net_device *netdev,
> +				    struct ethtool_drvinfo *drvinfo)
> +{
> +	strscpy(drvinfo->version, HBG_MOD_VERSION, sizeof(drvinfo->version));
> +	drvinfo->version[sizeof(drvinfo->version) - 1] = '\0';

A version is pointless, it tells you nothing useful. If you don't
touch version, the core will fill it with the uname, so you know
exactly what kernel this is, which is useful.

> +static u32 hbg_ethtool_get_link(struct net_device *netdev)
> +{
> +	struct hbg_priv *priv = netdev_priv(netdev);
> +
> +	return priv->mac.link_status;
> +}
> +
> +static int hbg_ethtool_get_ksettings(struct net_device *netdev,
> +				     struct ethtool_link_ksettings *ksettings)
> +{
> +	struct hbg_priv *priv = netdev_priv(netdev);
> +
> +	phy_ethtool_ksettings_get(priv->mac.phydev, ksettings);

You can avoid this wrapper since phy_attach_direct sets netdev->phydev
to phydev. You can then call phy_ethtool_get_link_ksettings() etc.

> +static int hbg_ethtool_set_ksettings(struct net_device *netdev,
> +				     const struct ethtool_link_ksettings *cmd)
> +{
> +	struct hbg_priv *priv = netdev_priv(netdev);
> +
> +	if (cmd->base.speed == SPEED_1000 && cmd->base.duplex == DUPLEX_HALF)
> +		return -EINVAL;

So long as you have told phylib about not supporting 1000Base/Half,
phy_ethtool_set_link_ksettings() will return an error for you.

> +static const struct ethtool_ops hbg_ethtool_ops = {
> +	.get_drvinfo		= hbg_ethtool_get_drvinfo,
> +	.get_link		= hbg_ethtool_get_link,

Why not ethtool_op_get_link() ?

> +	.get_link_ksettings	= hbg_ethtool_get_ksettings,
> +	.set_link_ksettings	= hbg_ethtool_set_ksettings,
> +};
> +static void hbg_update_link_status(struct hbg_priv *priv)
> +{
> +	u32 link;
> +
> +	link = hbg_get_link_status(priv);
> +	if (link == priv->mac.link_status)
> +		return;
> +
> +	priv->mac.link_status = link;
> +	if (link == HBG_LINK_DOWN) {
> +		netif_carrier_off(priv->netdev);
> +		netif_tx_stop_all_queues(priv->netdev);
> +		dev_info(&priv->pdev->dev, "link down!");
> +	} else {
> +		netif_tx_wake_all_queues(priv->netdev);
> +		netif_carrier_on(priv->netdev);
> +		dev_info(&priv->pdev->dev, "link up!");
> +	}
> +}

Why do you need this? phylib will poll the PHY once per second and
call the adjust_link callback whenever the link changes state.

> @@ -177,12 +226,17 @@ static int hbg_init(struct net_device *netdev)
>  	ret = hbg_irq_init(priv);
>  	if (ret)
>  		return ret;
> -
>  	ret = devm_add_action_or_reset(&priv->pdev->dev, hbg_irq_uninit, priv);

This white space change does not belong here.

     Andrew