On Mon, Feb 13, 2023 at 09:59:35AM +0100, Sergio Paracuellos wrote: > On Sun, Feb 12, 2023 at 4:27 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > > > > On 2/12/23 00:13, Sergio Paracuellos wrote: > > > On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski > > > <krzysztof.kozlowski@xxxxxxxxxx> wrote: > > >> > > >> On 11/02/2023 12:01, Sergio Paracuellos wrote: > > >>> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@xxxxxxxxxx> wrote: > > >>>> > > >>>> On 11.02.2023 13:41, Sergio Paracuellos wrote: > > >>>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@xxxxxxxxxx> 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. > > Ok, I will maintain only the PATCH that changes the driver to not use > globals and send v5. > Other options might be to search for the "syscon" node name or to search for the "mediatek,mt7621-sysc" compatible. Guenter > > > > Guenter > > > > Thanks, > Sergio Paracuellos
