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

It was <2020-11-04 śro 03:42>, when Andrew Lunn wrote:

quoted

+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

Done.

quoted

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

changed _at_ run time...

Done.

quoted

+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?

ax_local->capabilities |= ((*(u32 *)data) & SPICR_RCEN) ? AX_CAP_COMP : 0;

or rather

ax_local->capabilities |= ((*(u32 *)data) & (SPICR_RCEN | SPICR_QCEN)) ? AX_CAP_COMP : 0;

and possibly in the future (or now) split it into 

ax_local->capabilities |= ((*(u32 *)data) & SPICR_RCEN) ? AX_CAP_COMP_R : 0;
ax_local->capabilities |= ((*(u32 *)data) & SPICR_QCEN) ? AX_CAP_COMP_Q : 0;

(and appropriate masking abve and proper handling in
ax88796c_soft_reset()).

Kind regards
-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics