Re: [PATCH 3/5] spi: core: allow defining time that cs is deasserted as a multiple of SCK

[Lukas Wunner <lukas@xxxxxxxxx>]

On Sat, Feb 23, 2019 at 02:15:13PM +0100, kernel@xxxxxxxxxxxxxxxx wrote:
> On 23.02.2019, at 13:40, Lukas Wunner <lukas@xxxxxxxxx> wrote:
> > On Sat, Feb 23, 2019 at 08:49:50AM +0000, kernel@xxxxxxxxxxxxxxxx wrote:
> > > Support setting a delay between cs assert and deassert as
> > > a multiple of spi clock length.
> > 
> > The ability to define a unit seems somewhat over-engineered to me.
> > You're introducing lots of branching and integer divisions (both
> > comparatively expensive operations) which are executed for every
> > single transfer (i.e. in a hot path).  I'd recommend standardizing
> > on a single unit.  If 1 usec is too coarse-grained for your use case,
> > convert everything to nsec.
> 
> Well - a spi_device driver does not actually know what is the
> actual spi clock configured/used by the spi-bus-driver.

Which slave device and driver are we talking about anyway?
Is the driver actually making use of the ability to change the
speed per-transfer?  Most slaves I've dealt with so far use a
constant speed.  In that case the master driver could set the
slave's ->speed_hz member to the effective speed on ->setup()
and the slave's driver could then calculate the delay in nsec
or usec based on that speed.


> All it knows is that the device requires X clock cycles of cs high.

That's what I was driving at with my "to what end?" question.  You've
apparently got a slave device that requires a delay of a specific number
of clock cycles.  You should make that explicit in the commit messages,
ideally by naming the slave that requires it.  Right now your commit
messages only explain the "what", not the "why".  For an uninitiated
reader it's helpful to understand the specific motivation of your code
changes.


> From a CPU utilization perspective: for most situations we will
> (most likely) be busy waiting to pass the required time anyway.
> So those divides are just taking time from those busy cycles???

Okay.


> As for the rpi-mailing-list: if this is an issue, then this is a
> separate issue that needs to get addressed separately as it is not
> really code-related.

Yes, obviously that issue is tangential to the present series and
not a review comment, I raised it in the hope that someone reading it
may have an idea how the situation can be improved.  Thanks for
resending the missing patch.

Lukas