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

[dkota@codeaurora.org]

On 2018-08-10 21:43, Mark Brown wrote:
> On Fri, Aug 10, 2018 at 08:40:17AM -0700, Doug Anderson wrote:
>> On Fri, Aug 10, 2018 at 3:52 AM, Mark Brown <broonie@kernel.org> 
>> wrote:
> 
>> > This is more about matching the data rate between the two drivers - the
>> > clock framework could (and possibly should) reasonably return an error
>> > here, we're trying to ensure that drivers and controllers work well
>> > together here.
> 
>> The clock framework should be able to accomplish what you want.  If
>> you just request the rate it will do its best to make the rate
>> requested.  If we want to see what clock would be set before setting
> 
> The request could be massively off the deliverable rate - 50% or more.
> 
>> is "close enough" in this case?  I haven't gone and dug, but I've
>> always seen people only specify a max rate for SPI.  ...so as long as
>> the clock framework gives us something <= the clock we requested then
>> we should be OK, right?  If / when this becomes a problem then we
>> should add a min/max to "struct spi_transfer", no?
> 
> On the other hand if I ask for my audio to be played at 44.1kHz and the
> clock framework says "yes, sure" and gives me 8kHz then the user
> experience will be poor.
> 
>> Note that there are also clk_set_rate_range(), clk_set_min_rate(), and
>> clk_set_max_rate() though I'm told those might be a bit quirky.
> 
> They're certainly not widely used at any rate.
> 
>> ...but maybe we don't need to argue about this anyway since IMHO we
>> should just use the clk framework to figure out our maximum speed.
> 
> It looks like that's true in this case.
> 
>> >> 3. If you really truly need code in the SPI driver then make sure you
>> >> include a compatible string for the SoC and have a table in the driver
>> >> that's found with of_device_get_match_data().  AKA:
> 
>> >>   compatible = "qcom,geni-spi-sdm845", "qcom,geni-spi";
> 
>> > A controller driver really shouldn't need to be open coding anything.
> 
>> It wouldn't be open-coding, it would be a different way of specifying
>> things.  In my understanding it's always a judgement call about how
> 
> If you're saying we need clock rate selection logic (which is what it
> sounds like) rather than data then that seems like a problem.

Here are my couple of cents:
SPI controller maximum frequency can be lesser than or equal to Clock 
framework's maximum
frequency, so should not rely on the Clock framework.

SPI controller maximum frequency should not be derived from the SPI 
slave maximum frequency.
Consider the case where N number of slaves connected to SPI controller 
then there will
be N number of slave maximum frequency.
Also it does not look meaningful in communicating the SPI slave maximum 
frequency
as SPI controller maximum frequency to the SPI core framework.

Hence relying on SPI slave maximum frequency for SPI controller maximum 
frequency can be eliminated.

Now the need is, how to communicate the SPI controller maximum frequency 
to SPI core framework?
Is it by DTSI entry or hardcoding in the SPI controller driver?

My stand is for providing the DTSI entry.
Why because, this keeps SPI controller driver generic across the boards 
and portable.
Also it is not against to Device tree usage because maximum frequency
is describing the property of the hardware.

Please add your points.

--Dilip