[PATCH 2/4] drm/msm: drop qcom,chipid

[Rob Clark]

On Mon, Jan 30, 2017 at 2:54 PM, Eric Anholt <eric at anholt.net> wrote:
> Rob Clark <robdclark at gmail.com> writes:
>
>> On Mon, Jan 30, 2017 at 1:09 PM, Eric Anholt <eric at anholt.net> wrote:
>>> Rob Clark <robdclark at gmail.com> writes:
>>>
>>>> The original way we determined the gpu version was based on downstream
>>>> bindings from android kernel.  A cleaner way is to get the version from
>>>> the compatible string.
>>>>
>>>> Note that no upstream dtb uses these bindings.  But the code still
>>>> supports falling back to the legacy bindings (with a warning), so that
>>>> we are still compatible with the gpu dt node from android device
>>>> kernels.
>>>
>>> The gpulist[] seems pretty short, like you could just have 7 compatible
>>> strings in dt_match[] and point them directly at corresponding the
>>> gpulist[] entry.  Or are there lots of patch levels, with the same
>>> struct adreno_info values?
>>
>> The list currently is rather incomplete (which may or may not matter,
>> I guess it comes down to how many different phones/tablets out there
>> people try to get an upstream kernel working on).  And it has those
>> ANY_ID wildcard entries.
>>
>> A full list of all the gpu's including all their patchlevels would be
>> quite long.
>>
>> We might end up wanting to split the quirks out differently, since
>> those tend to apply to specific patchlevel's.. I had to change the
>> existing entry for a530 from a530.* to a530.2 for this reason.  But
>> that won't effect the bindings so that is an implementation detail we
>> can worry about later.
>
> I think that using the of_match_device() mechanism from the specific
> strings listed as the compatible would make more sense than this string
> parsing.  You have to write a gpulist[] entry anyway for the module to
> load.  So that means adding about 7 values today to the compatible
> string dt_match, and the code to use of_match_device() to get your
> struct adreno_info.  Then the current version number lookup loop would
> be just for the old DT compatibility and there's no string parsing.

well, the problem is still that we would end up needing entries for
each gpu version + patchlevel, which I was trying to avoid.. I think
otherwise, if we started adding more adreno variants the table would
get quite large.  The ANY_ID entries keep the table size down
currently.

> However, it's your driver.  The code seems correct, and using specific
> compatible strings is an obviously good step.

I may possibly re-work this in the future, but was leaning more
towards perhaps introducing some sort of of_match_device_wildcard(id,
dev, ...) type function, and allowing a compat string match like
"qcom,adreno-%1u%1u%1u.%u"

I guess maybe re-arranging this so multiple compat table entries point
back to the same 'struct adreno_info' might be workable, but that list
could still grow quite long.  At any rate, that doesn't change how the
bindings look so that can come later.

> Reviewed-by: Eric Anholt <eric at anholt.net>

thanks

BR,
-R