Messages in this thread | | | From | Lukasz Stelmach <> | Subject | Re: [PATCH v5 3/5] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver | Date | Wed, 04 Nov 2020 11:20:37 +0100 |
| |
It was <2020-11-04 śro 03:42>, when Andrew Lunn wrote: >> +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.
>> + changed in run time via spi-compression ethtool tunable. > > changed _at_ run time... >
Done.
>> +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 [unhandled content-type:application/pgp-signature] | |