On Mon, Jan 30, 2017 at 2:54 PM, Eric Anholt <eric@xxxxxxxxxx> wrote: > Rob Clark <robdclark@xxxxxxxxx> writes: > >> On Mon, Jan 30, 2017 at 1:09 PM, Eric Anholt <eric@xxxxxxxxxx> wrote: >>> Rob Clark <robdclark@xxxxxxxxx> 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@xxxxxxxxxx> thanks BR, -R -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
