Messages in this thread | | | Date | Mon, 13 Feb 2023 12:09:45 -0800 | Subject | Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to access system controller registers | From | Guenter Roeck <> |
| |
On 2/13/23 11:57, Sergio Paracuellos wrote: > On Mon, Feb 13, 2023 at 8:36 PM Guenter Roeck <linux@roeck-us.net> wrote: >> >> 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@roeck-us.net> wrote: >>>> >>>> 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. >>> >>> 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. > > Thanks for the hint. I didn't know about > 'syscon_regmap_lookup_by_compatible()'. I will use this to avoid DTB > ABI breakage and allow the driver to be selected for COMPILE_TEST.. >
I didn't know about that one either. I thought about of_find_compatible_node() or of_find_node_by_name(). syscon_regmap_lookup_by_compatible() is widely used, though, so it seems to be a much better option.
Thanks, Guenter
| |