Re: [PATCH v3 2/5] spi: sun6i: change OF match data to a struct

[Andre Przywara <andre.przywara@arm.com>]

On Wed, 10 May 2023 08:55:27 +0000
David Laight <David.Laight@ACULAB.COM> wrote:

Hi David,

> From: Maxim Kiselev
> > Sent: 10 May 2023 09:34
> > 
> > Hi, David
> >   
> > > Is it worth doing a structure copy at this point?
> > > The 'cfg' data is short and constant and it would make

Sorry, I don't really get the reason for this. Since the data is constant,
wouldn't it make much more sense to keep it there in the const section,
which we need anyway? What would a second copy bring us?

> > > the code that uses it smaller and faster.  

Smaller: Do you mean the generated code? Not sure that really matters, but
your sketch below hints that the C code would get larger, more error prone
(you mention yourself that you skipped over the error checking) and most
importantly harder to read.

Faster: Do you have numbers that back that up, or does that solve a
particular problem of yours?
This is programming a SPI controller transfer, which runs in the vicinity
of a few Mbits/s. I doubt that saving a few memory accesses (once
per transfer, not per word) matters even the slightest?
The actual MMIO access to program the controller registers already takes
a few dozen to a few hundred cycles, so I doubt that doing a struct copy
saves us anything here, in practice.

Besides: Copying the pointer is the most common pattern in the kernel, I
believe. I just sampled 21 SPI drivers in the tree, 17 out of them do
this. The other either copy the members of the struct into the driver data
(which would be an option for us, too), or immediately consume the data in
the probe() routine.

If you have some good reason to optimise this, please send a patch (on
top).

Cheers,
Andre.

> > 
> > I'm sorry but I don't fully understand what you are suggesting.
> > In patch 3\5 we extend the sun6i_spi_cfg structure with the has_clk_ctl field.  
> 
> You are still only adding a second integer.
> 
> I'm suggesting that instead of sspi->cfg being a pointer to the
> config data it is a copy of the config data.
> So the assignment below becomes (ignoring error checks)
> 	memcpy(&sspi->cfg, of_device_get_match_data(&pdev->dev), sizeof sspi->cfg);
> and the code that needs the values is:
> 	sspi->cfg.fifo_depth
> (etc)
> 
> 	David
> 
> > 
> > пн, 8 мая 2023 г. в 12:47, David Laight <David.Laight@aculab.com>:  
> > >
> > > From: Maksim Kiselev  
> > > > Sent: 07 May 2023 00:26
> > > >
> > > > As we're adding more properties to the OF match data, convert it to a
> > > > struct now.
> > > >  
> > > ...  
> > > > -     sspi->fifo_depth = (unsigned long)of_device_get_match_data(&pdev->dev);
> > > > +     sspi->cfg = of_device_get_match_data(&pdev->dev);  
> > >
> > > Is it worth doing a structure copy at this point?
> > > The 'cfg' data is short and constant and it would make
> > > the code that uses it smaller and faster.
> > >
> > >         David
> > >
> > > -
> > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > > Registration No: 1397386 (Wales)
> > >  
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)