On 22/11/2023 15:34, Jerome Brunet wrote: > > On Wed 22 Nov 2023 at 09:37, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote: > >> On 20/11/2023 11:04, Jerome Brunet wrote: >>>>>>>> .../devicetree/bindings/pwm/pwm-amlogic.yaml | 36 +++++++++++++++++-- >>>>>>>> 1 file changed, 34 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>> Reviewed-by: Rob Herring <robh@xxxxxxxxxx> >>>>>>> >>>>>> >>>>>> I'm puzzled, isn't it recommended to have a per-soc compatible now ? >> >> Yes, it is. >> >>>>> I have specifically addressed this matter in the description, >>>>> haven't I ? What good would it do in this case ? >> >> There is nothing about compatible naming in commit msg. > > Krzysztof, the whole commit desciption is explanation about why a new > compatible is introduced. I don't understand this comment. > >> >>>> >>>> Yes you did but I was asked for the last year+ that all new compatible >>>> should be soc specific (while imprecise, in our care soc family should be ok), >>>> with a possible semi-generic callback with an IP version or a first soc >>>> implementing the IP. >>>> >>>>> Plus the definition of a SoC is very vague. One could argue that >>>>> the content of the list bellow are vaguely defined families. Should we >>>>> add meson8b, gxl, gxm, sm1 ? ... or even the actual SoC reference ? >>>>> This list gets huge for no reason. >>>> >>>> I think in our case soc family is reasonable since they share same silicon >>>> design. >>>> >>>>> We know all existing PWM of this type are the same. We have been using >>>>> them for years. It is not a new support we know nothing about. >>>>> >>>>>> >>>>>> I thought something like: >>>>>> - items: >>>>>> - enum: >>>>>> - amlogic,gxbb-pwm >>>>>> - amlogic,axg-pwm >>>>>> - amlogic,g12a-pwm >>>>>> - const: amlogic,pwm-v1 >>>>> I'm not sure I understand what you are suggesting here. >>>>> Adding a "amlogic,pwm-v1" for the obsolete compatible ? No amlogic DT >>>>> has that and I'm working to remove this type, so I don't get the point. >>>>> >>>>>> >>>>>> should be preferred instead of a single amlogic,meson8-pwm-v2 ? >>>>> This is named after the first SoC supporting the type. >>>>> Naming it amlogic,pwm-v2 would feel weird with the s4 coming after. >>>>> Plus the doc specifically advise against this type of names. >>>> >>>> The -v2 refers to a pure software/dt implementation versioning and not >>>> an HW version, so I'm puzzled and I requires DT maintainers advice here. >>>> >>>> Yes meson8b is the first "known" platform, even if I'm pretty sure meson6 has >> >> Yes, this should be SoC-based compatible, unless you have clear >> versioning scheme by SoC/IP block vendor. You named it not a HW version, >> which kind of answers to the "unless" case - that's not hardware version. >> > > This is specifically the point of the comment in commit description. > We know all the PWMs compatible are the same HW (version) as one found > in the meson8b. > > It is certain that adding more compatible, listing all the SoC, will be > useless. I can do it if you insist. The docs you references insist on that, so yeah, I insist as well. > >>> >>> This is not my point. I picked this name because I have to pick a >>> specific device based one. Not because it is actually the first or >>> not. I don't see a problem with meson6 being compatible with >>> meson8-pwm-v2, if that ever comes along. >> >> No, the point is not to use "v2". Use SoC compatibles. > > It is a SoC compatible. The second one. "v2" is not the soc. I assume meson8 is one specific SoC, right? Because elinux says it is a *family*: https://elinux.org/Amlogic/SoCs > > The first one, as explained in the description was describing the driver > more that the HW. > > Changing the way clock are passed from DT to the driver would be break > user of the old compatible. So a new compatible is introduced. I believe > this is recommended way to introduce incompatible binding changes. The way is not to introduce incompatible changes. Your way is also not good as it breaks other users of DTS. > > v2 here denote a new interface version, nothing to do with HW > versioning. I happy to pick something else to denote this. Sorry, then it is not a SoC-based compatible. > >> >>> >>> I think the binding here satisfy the rule that it should be specific, >>> and the intent that goes with it: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/writing-bindings.rst?h=v6.7-rc2#n42 >>> >>>> the same pwm architecture, this is why "amlogic,pwm-v1" as fallback seems more >>>> reasonable and s4 and later pwm could use the "amlogic,pwm-v2" >>>> fallback. >>> >>> That is not how understand this: >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/writing-bindings.rst?h=v6.7-rc2#n82 >>> >> >> Again, where the "v2" is defined? Where is any document explaining the >> mapping between version blocks and SoC parts? Why do you list here only >> major version? Blocks almost always have also minor (e.g. v2.0). > > Again, v2 does has nothing to do with the HW. Never wrote it was. > The HW remains the same. Don't add compatibles which are not related to HW, but represent software versioning. Software does not matter for the bindings. That's a clear NAK. Best regards, Krzysztof
