On Fri, Sep 4, 2020 at 7:52 PM Vadym Kochan <vadym.koc...@plvision.eu> wrote:
>
> The ethtool API provides support for the configuration of the following
> features: speed and duplex, auto-negotiation, MDI-x, forward error
> correction, port media type. The API also provides information about the
> port status, hardware and software statistic. The following limitation
> exists:
>
> - port media type should be configured before speed setting
> - ethtool -m option is not supported
> - ethtool -p option is not supported
> - ethtool -r option is supported for RJ45 port only
> - the following combination of parameters is not supported:
>
> ethtool -s sw1pX port XX autoneg on
>
> - forward error correction feature is supported only on SFP ports, 10G
> speed
>
> - auto-negotiation and MDI-x features are not supported on
> Copper-to-Fiber SFP module
...
> +#include <linux/kernel.h>
> +#include <linux/netdevice.h>
> +#include <linux/ethtool.h>
Sorted?
...
> + if (new_mode < PRESTERA_LINK_MODE_MAX)
> + err = prestera_hw_port_link_mode_set(port, new_mode);
> + else
> + err = -EINVAL;
> +
> + if (!err) {
> + port->caps.type = type;
> + port->autoneg = false;
> + }
> +
> + return err;
Traditional pattern?
if (err)
return err;
...
return 0;
...
> + ecmd->base.speed = !err ? speed : SPEED_UNKNOWN;
Why not err : SPEED_... : speed; ?
...
> +static int
> +prestera_ethtool_get_link_ksettings(struct net_device *dev,
One line?
> + struct ethtool_link_ksettings *ecmd)
...
> + err = prestera_hw_port_link_mode_get(port, &curr_mode);
> + if (err || curr_mode >= PRESTERA_LINK_MODE_MAX)
> + return -EINVAL;
Why shadowing error code?
...
> +/*
> + * Copyright (c) 2019-2020 Marvell International Ltd. All rights reserved.
> + *
> + */
One line.
...
> +#include <linux/netdevice.h>
Is this being used?
> +#include <linux/ethtool.h>
> +
> +extern const struct ethtool_ops prestera_ethtool_ops;
...
> +enum {
> + PRESTERA_FC_NONE,
> + PRESTERA_FC_SYMMETRIC,
> + PRESTERA_FC_ASYMMETRIC,
> + PRESTERA_FC_SYMM_ASYMM
Comma?
> +};
...
> + struct prestera_msg_port_attr_req req = {
> + .attr = PRESTERA_CMD_PORT_ATTR_REMOTE_CAPABILITY,
> + .port = port->hw_id,
> + .dev = port->dev_id
Comma?
> + };
...
> + err = prestera_cmd_ret(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_GET,
> + &req.cmd, sizeof(req), &resp.ret,
> sizeof(resp));
> + if (err)
> + return err;
> +
> + *link_mode_bitmap = resp.param.cap.link_mode;
> +
> + return err;
return 0;
I think I have talked that any of the given comments applies to *all*
similar code pieces!
This file is full of repetitions of the above.
...
> +static u8 prestera_hw_mdix_to_eth(u8 mode)
> +{
> + switch (mode) {
> + case PRESTERA_PORT_TP_MDI:
> + return ETH_TP_MDI;
> + case PRESTERA_PORT_TP_MDIX:
> + return ETH_TP_MDI_X;
> + case PRESTERA_PORT_TP_AUTO:
> + return ETH_TP_MDI_AUTO;
> + }
> +
> + return ETH_TP_MDI_INVALID;
default.
Also I have a deja vu about such.
> +}
> +
> +static u8 prestera_hw_mdix_from_eth(u8 mode)
> +{
> + switch (mode) {
> + case ETH_TP_MDI:
> + return PRESTERA_PORT_TP_MDI;
> + case ETH_TP_MDI_X:
> + return PRESTERA_PORT_TP_MDIX;
> + case ETH_TP_MDI_AUTO:
> + return PRESTERA_PORT_TP_AUTO;
> + }
> +
> + return PRESTERA_PORT_TP_NA;
> +}
Ditto.
...
> +enum {
> + PRESTERA_PORT_DUPLEX_HALF,
> + PRESTERA_PORT_DUPLEX_FULL
Comma ?
> +};
--
With Best Regards,
Andy Shevchenko
