lkml.org 
[lkml]   [2023]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers
    From
    On 2/12/23 00:13, Sergio Paracuellos wrote:
    > On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski
    > <krzysztof.kozlowski@linaro.org> wrote:
    >>
    >> On 11/02/2023 12:01, Sergio Paracuellos wrote:
    >>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
    >>>>
    >>>> On 11.02.2023 13:41, Sergio Paracuellos wrote:
    >>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
    >>>>>>
    >>>>>> Is this mediatek,sysctl property required after your changes on the
    >>>>>> watchdog code?
    >>>>>
    >>>>> I don't really understand the question :-) Yes, it is. Since we have
    >>>>> introduced a new phandle in the watchdog node to be able to access the
    >>>>> reset status register through the 'sysc' syscon node.
    >>>>> We need the bindings to be aligned with the mt7621.dtsi file and we
    >>>>> are getting the syscon regmap handler via
    >>>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.
    >>>>
    >>>> I believe you need to put mediatek,sysctl under "required:".
    >>>
    >>> Ah, I understood your question now :-). You meant 'required' property.
    >>> I need more coffee, I guess :-). I am not sure if you can add
    >>> properties as required after bindings are already mainlined for
    >>> compatibility issues. The problem with this SoC is that drivers become
    >>> mainlined before the device tree was so if things are properly fixed
    >>> now this kind of issues appear. Let's see Krzysztof and Rob comments
    >>> for this.
    >>
    >> If your driver fails to probe without mediatek,sysctl, you already made
    >> it required (thus broke the ABI) regardless what dt-binding is saying.
    >> In such case you should update dt-binding to reflect reality.
    >>
    >> Now ABI break is different case. Usually you should not break it without
    >> valid reasons (e.g. it was never working before). Your commit msg
    >> suggests that you only improve the code, thus ABI break is not really
    >> justified. In such case - binding is correct, driver should be reworked
    >> to accept DTS without the new property.
    >
    > Thanks for clarification, Krzysztof. Ok, so if this is the case I need
    > to add this property required (as Arinc was properly pointing out in
    > previous mail) since without it the driver is going to fail on probe
    > (PATCH 5 of the series). I understand the "it was never working
    > before" argument reason for ABI breaks. What happens if the old driver
    > code was not ideal and totally dependent on architecture specific
    > operations when this could be totally avoided and properly make arch
    > independent agnostic drivers? This driver was added in 2016 [0]. There
    > was not a device tree file in the kernel for this SoC mainlined until
    > 2022 [1]. I also personally migrated this watchdog binding in 2022
    > from text to YAML and maintained it without changes [2]. When this was
    > mainlined not all drivers were properly reviewed and the current code
    > was just maintained as it is. Most users of this SoC are in the
    > openWRT community where the dtsi of the mainline is not used yet and
    > they maintain their own mt7621.dtsi files. Also, when a new version of
    > the openWRT selected kernel is added they also modify and align with
    > its mt7621.dtsi file without maintaining previous dtb's. If "make the
    > driver arch independent to be able to be compile tested" and this kind
    > of arguments are not valid at all I need to know because I have
    > started to review driver code for this SoC and other drivers also have
    > the same arch dependency that ideally should be avoided in the same
    > way. This at the end means to break the ABI again in the future for
    > those drivers / bindings. So I can just let them be as it is and not
    > provide any change at all and continue without being compile tested
    > and other beneficial features to detect future driver breakage.
    >

    Problem is that there are (presumably) shipped systems out there with
    the old devicetree file. The watchdog driver would no longer instantiate
    on those systems.

    Guenter

    \
     
     \ /
      Last update: 2023-03-27 00:19    [W:4.383 / U:0.212 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site