Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP

[Mahadevan, Girish]

Hi Mark, Stephen

On 5/22/2018 11:30 AM, Mark Brown wrote:
>> That's good. The problem I see is that we have to specify the max
>> frequency in the controller/bus node, and also in the child/slave node.
>> It should only need to be specified in the slave node, so making the
>> cur_speed_hz equal the max_speed_hz is problematic. The current speed of
>> the master should be determined by calling clk_get_rate().
>
> We don't require that the slaves all individually set a speed since it
> gets a bit redundant, it should be enough to just use the default the
> controller provides.  A bigger problem with this is that the driver will
> never see a transfer which doesn't explicitly have a speed set as the
> core will ensure something is set, open coding this logic in every
> driver would obviously be tiresome.

Sorry , I need some more clarification.

When I register the master, I specify the max rate the core can support
(50 Mhz). So any transaction that comes to the driver is going to be
requesting at-most 50 Mhz.

The reason I have the cur_speed_hz is that there is a max_speed_hz which
is the max frequency the slave can do; but every transfer can also
specify a speed_hz and override this.

So my point is we can do upto 4 slaves on a given controller, each slave
can have a different max speed and each slave's transfer can override
the max_frequency of that slave device.
(the default frequency is the master's max frequency)

>> Do you mean spi-rx-delay-us and spi-tx-delay-us properties? Those are
>> documented but don't seem to be used. There's also the delay_usecs part
>> of the spi_transfer structure, which may be what you're talking about.
> 
> delay_usecs is for inter-transfer delays within a message rather than
> after the initial chip select assert (it can be used to keep chip select
> asserted for longer after the final transfer too).  Obviously this is
> also something that shouldn't be configured in a driver specific
> fashion.
> 

Hmmm ok, so you mean don't send these as controller_data, rather add new
members to the spi_device struct ?

Best Regards
Girish

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora
Forum, a Linux Foundation Collaborative Project.