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

[Andrew Lunn]

> >> +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.
> 
> Good point. What is your recommendation for the userland facing
> interface, so that future changes will be least disruptive?

I would KISS and just validate data is 0 or 1, and leave it at that.
That then later gives you the option to have other values, if that is
ever interesting.

     Andrew