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

From: Mark Brown <broonie@kernel.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Dilip Kota <dkota@codeaurora.org>,
	Stephen Boyd <swboyd@chromium.org>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-spi <linux-spi@vger.kernel.org>,
	Sagar Dharia <sdharia@codeaurora.org>,
	Karthikeyan Ramasubramanian <kramasub@codeaurora.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	"Mahadevan, Girish" <girishm@codeaurora.org>
Subject: Re: [PATCH] spi: spi-geni-qcom: Add SPI driver support for GENI based QUP
Date: Fri, 10 Aug 2018 17:43:45 +0100	[thread overview]
Message-ID: <20180810164345.GH20971@sirena.org.uk> (raw)
In-Reply-To: <CAD=FV=Xu986JWnwC_D6OdUbers4fGf=ExS_z-J_t=QqHOhs1PA@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

On Fri, Aug 10, 2018 at 09:27:05AM -0700, Doug Anderson wrote:
> On Fri, Aug 10, 2018 at 9:13 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Aug 10, 2018 at 08:40:17AM -0700, Doug Anderson wrote:

> >> 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.

> Agreed.  If the clock is massively off and that causes problems then
> someone will need to debug it and find a solution.  I'm not aware of
> us being in that case in the driver in question.

For the logic in the SPI core it's relatively common, lots of SPI
controllers are limited to something in the region of 10-12.5MHz but it's
common for devices to be able to run up to the region of 30MHz.

> >> >> 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:

> >> 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.
> 
> We're talking past each other I think.  Maybe a concrete example helps?

...

> IMO the line marked "/* UNNEEDED */" below should be removed:

>     ...
>     spi-max-frequency = <50000000>;  /* UNNEEDED */

This is a line in the device tree (which I agree shouldn't be there),
not code in the SPI driver?

> ...and then the driver should say "oh, I have a compatible string of
> "qcom,geni-spi-sdm845" so I know my controller's max frequency must be
> 50 MHz.  It can get that information using of_device_get_match_data().

> Hopefully that's clearer?

That's just a data table, when you talk about code in the driver
(particularly given the wider discussion of what the maximum rate might
be) it sounds rather more involved than that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]