| Message ID | 20220120150024.646714-3-paul.kocialkowski@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: LogiCVC display controller support | expand |
On Thu, Jan 20, 2022 at 4:00 PM Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > There are lots of different versions of the logicvc block and it > makes little sense to list them all in compatibles since all versions > with the same major are found to be register-compatible. The reason we try to be precise is because sometime, long after the driver has been merged and maintained for a few years, a bug is discovered in a specific version of the silicon. What happens is that a fix is applied on all silicon whether it is needed or not. If you have the precise silicon compatible, you can avoid this and target only a specific version. Yours, Linus Walleij
On 2022-01-30 00:46, Linus Walleij wrote: > On Thu, Jan 20, 2022 at 4:00 PM Paul Kocialkowski > <paul.kocialkowski@bootlin.com> wrote: > >> There are lots of different versions of the logicvc block and it >> makes little sense to list them all in compatibles since all versions >> with the same major are found to be register-compatible. > > The reason we try to be precise is because sometime, long after the driver > has been merged and maintained for a few years, a bug is discovered > in a specific version of the silicon. > > What happens is that a fix is applied on all silicon whether it is needed > or not. > > If you have the precise silicon compatible, you can avoid this and target > only a specific version. Indeed, the better approach would be something like: compatible: oneOf: - items: - enum: - foo,bar-v1.0 - foo,bar,v1.1 - const: foo,bar-v1 - items: - enum: - foo,bar-v2.0 - const: foo,bar-v2 That way the DTs are future-proof, while drivers can still match on only the less-specific strings until a need arises. Plus it avoids the problem that if an existing OS that only understands "foo,bar-v1.0" is given a new DT with only "foo,bar-v1" for v1.0 hardware it won't be able to use the device, even though it's *functionally* capable of doing so. However, from skimming patch #5, it looks possible that none of these changes are needed at all. If LOGICVC_IP_VERSION_REG tells you the exact revision, and is always present (as the unconditional reading of it implies), then the only reason for adding new compatibles would be if, say, v5 has more clocks from v4 and you want the binding to enforce that; otherwise, newer versions are literally compatible with the currently-defined binding and therefore should continue to bind against the existing string(s) to maximise forward- and backward-compatibility. Sure, it's not the prettiest thing for a "generic" compatible to be based on an oddly-specific version number that doesn't necessarily match the actual software-discoverable version, but what's done is done and that's the cost of ABI. Cheers, Robin. (also, nitpick for that part of patch #5 since I'm here: please include linux/bitfield.h rather than reinventing FIELD_GET() locally)
Hi, On Wed 02 Feb 22, 14:26, Robin Murphy wrote: > On 2022-01-30 00:46, Linus Walleij wrote: > > On Thu, Jan 20, 2022 at 4:00 PM Paul Kocialkowski > > <paul.kocialkowski@bootlin.com> wrote: > > > > > There are lots of different versions of the logicvc block and it > > > makes little sense to list them all in compatibles since all versions > > > with the same major are found to be register-compatible. > > > > The reason we try to be precise is because sometime, long after the driver > > has been merged and maintained for a few years, a bug is discovered > > in a specific version of the silicon. > > > > What happens is that a fix is applied on all silicon whether it is needed > > or not. > > > > If you have the precise silicon compatible, you can avoid this and target > > only a specific version. > > Indeed, the better approach would be something like: > > compatible: > oneOf: > - items: > - enum: > - foo,bar-v1.0 > - foo,bar,v1.1 > - const: foo,bar-v1 > - items: > - enum: > - foo,bar-v2.0 > - const: foo,bar-v2 > > That way the DTs are future-proof, while drivers can still match on only the > less-specific strings until a need arises. Plus it avoids the problem that > if an existing OS that only understands "foo,bar-v1.0" is given a new DT > with only "foo,bar-v1" for v1.0 hardware it won't be able to use the device, > even though it's *functionally* capable of doing so. Yes I understand that we need to keep compatibility with the already-defined compatible. > However, from skimming patch #5, it looks possible that none of these > changes are needed at all. If LOGICVC_IP_VERSION_REG tells you the exact > revision, and is always present (as the unconditional reading of it > implies), then the only reason for adding new compatibles would be if, say, > v5 has more clocks from v4 and you want the binding to enforce that; > otherwise, newer versions are literally compatible with the > currently-defined binding and therefore should continue to bind against the > existing string(s) to maximise forward- and backward-compatibility. Sure, > it's not the prettiest thing for a "generic" compatible to be based on an > oddly-specific version number that doesn't necessarily match the actual > software-discoverable version, but what's done is done and that's the cost > of ABI. Indeed it's true that hardware quirks can be applied based on the precise version read from the register, so I don't think there is a need for overly precise compatibles. Since the device-tree binding is currently the same for all versions, I understand that it makes sense to keep a single compatible (the already defined one), so I guess I will make another iteration without introducing new compatibles. But I will probably update the binding document to reflect which versions are currently known to work with its current state. > (also, nitpick for that part of patch #5 since I'm here: please include > linux/bitfield.h rather than reinventing FIELD_GET() locally) Ah good to know thanks, first time hearing about those. Paul
diff --git a/Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.yaml b/Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.yaml index a36aec27069c..94dee736d986 100644 --- a/Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.yaml +++ b/Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.yaml @@ -30,6 +30,7 @@ properties: compatible: enum: - xylon,logicvc-3.02.a-gpio + - xylon,logicvc-3-gpio reg: maxItems: 1
There are lots of different versions of the logicvc block and it makes little sense to list them all in compatibles since all versions with the same major are found to be register-compatible. Introduce a new compatible with the major version only. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> --- Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.yaml | 1 + 1 file changed, 1 insertion(+)