Re: [PATCH 11/14] net: ks8851: Implement register and FIFO accessor callbacks

On 3/24/20 3:29 PM, Lukas Wunner wrote:

On Tue, Mar 24, 2020 at 03:10:59PM +0100, Marek Vasut wrote:

quoted

On 3/24/20 2:45 PM, Lukas Wunner wrote:

quoted

On Tue, Mar 24, 2020 at 12:43:00AM +0100, Marek Vasut wrote:

quoted

The register and FIFO accessors are bus specific. Implement callbacks so
that each variant of the KS8851 can implement matching accessors and use
the rest of the common code.

[...]

quoted

+	unsigned int		(*rdreg16)(struct ks8851_net *ks,
+					   unsigned int reg);
+	void			(*wrreg16)(struct ks8851_net *ks,
+					   unsigned int reg, unsigned int val);
+	void			(*rdfifo)(struct ks8851_net *ks, u8 *buff,
+					  unsigned int len);
+	void			(*wrfifo)(struct ks8851_net *ks,
+					  struct sk_buff *txp, bool irq);

Using callbacks entails a dereference for each invocation.

Yes indeed, the SPI stack which you use to talk to the KS8851 SPI is
also full of those.

Apples and oranges.  Low-level SPI drivers provide callbacks to the
SPI core because it would be too expensive (space-wise) to link the
SPI core into every low-level driver.  Whereas in this case, you're
generating two separate modules anyway, so there's no need at all
to use callbacks.

quoted

quoted

A cheaper approach is to just declare the function signatures
in ks8851.h and provide non-static implementations in
ks8851_spi.c and ks8851_mll.c, so I'd strongly prefer that.

Even better, since this only concerns the register accessors
(which are inlined anyway by the compiler), it would be best
to have them in header files (e.g. ks8851_spi.h / ks8851_par.h)
which are included by the common ks8851.c based on the target
which is being compiled.  That involves a bit of kbuild magic
though to generate two different .o from the same .c file,
each with specific "-include ..." CFLAGS.

Before we go down the complex and ugly path, can you check whether this
actually has performance impact ? I would expect that since this is an
SPI-connected device, this here shouldn't have that much impact. But I
might be wrong, I don't have the hardware.

I can test it, but the devices are in the office, I won't return there
before Thursday.  That said, I don't think it's a proper approach to
make the code more expensive even though it's perfectly possible to
avoid any performance impact, and shrug off concerns with the argument
that the impact should be measured first.

I cannot measure the impact on the SPI device, but I would like to know
the numbers to see whether it's worth it all, before I start creating a
more complex solution.

Since the SPI bus is limited to 40 MHz per datasheet, I don't think the
pointer dereference is gonna introduce any performance problem.

I can at least try skipping the dereference on the parallel bus option
and see if it makes a difference.

quoted

Also note that having this dereference in place, it permits me to easily
implement accessors for both LE and BE variant of the parallel bus device.

My understanding is that you're supposed to configure the chip to use
the native endianness of your architecture on ->probe() such that
conversions become unnecessary and the same accessor can be used for
LE and BE.  So why do you need two accessors?

Because I have a device here which is configured the "wrong" way thus far.