Re: [PATCH v5 3/5] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

[Andrew Lunn <andrew@lunn.ch>]

> +config SPI_AX88796C_COMPRESSION
> +	bool "SPI transfer compression"
> +	default n
> +	depends on SPI_AX88796C
> +	help
> +	  Say Y here to enable SPI transfer compression. It saves up
> +	  to 24 dummy cycles during each transfer which may noticably
> +	  speed up short transfers. This sets the default value that is
> +	  inherited by network interfecase during probe. It can be

interface


> +	  changed in run time via spi-compression ethtool tunable.

changed _at_ run time...

> +static int
> +ax88796c_set_tunable(struct net_device *ndev, const struct ethtool_tunable *tuna,
> +		     const void *data)
> +{
> +	struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
> +
> +	switch (tuna->id) {
> +	case ETHTOOL_SPI_COMPRESSION:
> +		if (netif_running(ndev))
> +			return -EBUSY;
> +		ax_local->capabilities &= ~AX_CAP_COMP;
> +		ax_local->capabilities |= *(u32 *)data ? AX_CAP_COMP : 0;

You should probably validate here that data is 0 or 1. That is what
ax88796c_get_tunable() will return.

It seems like this controls two hardware bits:

SPICR_RCEN | SPICR_QCEN

Maybe at some point it would make sense to allow these bits to be set
individually? If you never validate the tunable, you cannot make use
of other values to control the bits individually.

   Andrew