[v6,0/3] watchdog: mt7621-wdt: avoid globals and arch dependencies

Message ID 20230213200519.889503-1-sergio.paracuellos@gmail.com
Headers
Series watchdog: mt7621-wdt: avoid globals and arch dependencies |

Message

Sergio Paracuellos Feb. 13, 2023, 8:05 p.m. UTC
  Hi all,

This series make an update in the MT7621 SoC's watchdog driver code. This
SoC already provides a system controller node to access reset status
register needed for the watchdog. Instead of using MIPS architecture
dependent operations in header 'asm/mach-ralink/ralink_regs.h' get
node syscon regmap using 'syscon_regmap_lookup_by_compatible()' and use.
regmap APIS. Driver is also using some globals that are not needed at
all if a driver data structure is used along the code. Hence, add all
new needed stuff inside a new driver data structure. With this changes
driver can be properly compile tested.

Thanks in advance for reviewing this!

v1 of this series here [0].
v2 of this series here [1].
v3 of this series here [2].
v4 of this series here [3].
v5 of this series here [4].

Changes in v6:
    - Get regmap syscon using 'syscon_regmap_lookup_by_compatible()'
    - Add COMPILE_TEST and dependencies to Kconfig.
    - Collect Philippe Mathieu-Daudé 'Reviewed-by' tag for watchdog node
      warning fix.

Changes in v5:
    - Drop patches related with device tree ABI breakage and only
      maintain the rest.
    - Collect Arinc 'Reviewed-by' tag for watchdog node warning fix.

Changes in v4:
    - Add a patch to fix a watchdog node warning with 'make dtbs_check'
      because of a wrong node name.
    - Collect Guenter 'Reviewed-by' tags for watchdog driver code.
    - Add a missing 'COMPILE_TEST' to Kconfig which was lost when driver
      code was split in two patches in v2.

Changes in v3:
    - rename phandler from 'ralink,sysctl' into 'mediatek,sysctl'.
    - Drop error message added in PATCH 3 that modifies functionality
      and we only want to maintain current functionaloty by now.

Changes in v2:
    - Remove no needed compatible 'syscon' from bindings.
    - Rewrite new syscon phandle description in bindings.
    - Remove 'syscon' from compatible in 'mt7621.dtsi'.
    - Split PATCH 3 into two different patches:
        - PATCH 3: removes static globals using a driver data structure.
        - PATCH 4: remove ralink architecture dependent includes and code.

Best regards,
    Sergio Paracuellos

[0]: https://lore.kernel.org/linux-watchdog/20230210065621.598120-1-sergio.paracuellos@gmail.com/T/#t
[1]: https://lore.kernel.org/linux-watchdog/20230210121735.639089-1-sergio.paracuellos@gmail.com/T/#t
[2]: https://lore.kernel.org/linux-watchdog/20230210173841.705783-1-sergio.paracuellos@gmail.com/T/#t
[3]: https://lore.kernel.org/linux-watchdog/20230211073357.755893-1-sergio.paracuellos@gmail.com/T/#t
[4]: https://lore.kernel.org/linux-watchdog/20230213120638.850612-1-sergio.paracuellos@gmail.com/T/#t

Sergio Paracuellos (2):

Sergio Paracuellos (3):
  mips: dts: ralink: mt7621: rename watchdog node from 'wdt' into
    'watchdog'
  watchdog: mt7621-wdt: avoid static global declarations
  watchdog: mt7621-wdt: avoid ralink architecture dependent code

 arch/mips/boot/dts/ralink/mt7621.dtsi |   2 +-
 drivers/watchdog/Kconfig              |   4 +-
 drivers/watchdog/mt7621_wdt.c         | 118 +++++++++++++++++---------
 3 files changed, 81 insertions(+), 43 deletions(-)
  

Comments

Arınç ÜNAL Feb. 14, 2023, 8:12 a.m. UTC | #1
On 13.02.2023 23:05, Sergio Paracuellos wrote:
> Hi all,
> 
> This series make an update in the MT7621 SoC's watchdog driver code. This
> SoC already provides a system controller node to access reset status
> register needed for the watchdog. Instead of using MIPS architecture
> dependent operations in header 'asm/mach-ralink/ralink_regs.h' get
> node syscon regmap using 'syscon_regmap_lookup_by_compatible()' and use.
> regmap APIS. Driver is also using some globals that are not needed at
> all if a driver data structure is used along the code. Hence, add all
> new needed stuff inside a new driver data structure. With this changes
> driver can be properly compile tested.
> 
> Thanks in advance for reviewing this!
> 
> v1 of this series here [0].
> v2 of this series here [1].
> v3 of this series here [2].
> v4 of this series here [3].
> v5 of this series here [4].
> 
> Changes in v6:
>      - Get regmap syscon using 'syscon_regmap_lookup_by_compatible()'
>      - Add COMPILE_TEST and dependencies to Kconfig.
>      - Collect Philippe Mathieu-Daudé 'Reviewed-by' tag for watchdog node
>        warning fix.
> 
> Changes in v5:
>      - Drop patches related with device tree ABI breakage and only
>        maintain the rest.

This makes me regret taking mt7621-dts out of drivers/staging. Clearly 
there were improvements to be made on the MT7621 drivers that would 
affect the devicetree binding. I don't think one would bat an eye to 
make dt-binding changes on a devicetree on drivers/staging.

I also want to make a similar change that would break the ABI. I want to 
split the MediaTek MIPS (currently called Ralink) pinctrl subdrivers 
further, namely mt7620.c, to split MT7628/MT7688 pinmux data from 
MT7620, so I can properly document which function can be given to which 
group(s) for the MT7628/MT7688 SoCs.

This means I've got to introduce a new compatible string for the MT76X8 
SoCs which would cause the pinctrl driver to stop working with an old DT 
for the MT76X8 SoCs. By the nature of the change, I can't make a way 
around with the code like Sergio did in v6.

Arınç
  
Arınç ÜNAL Feb. 14, 2023, 8:31 a.m. UTC | #2
On 14.02.2023 11:12, Arınç ÜNAL wrote:
> On 13.02.2023 23:05, Sergio Paracuellos wrote:
>> Hi all,
>>
>> This series make an update in the MT7621 SoC's watchdog driver code. This
>> SoC already provides a system controller node to access reset status
>> register needed for the watchdog. Instead of using MIPS architecture
>> dependent operations in header 'asm/mach-ralink/ralink_regs.h' get
>> node syscon regmap using 'syscon_regmap_lookup_by_compatible()' and use.
>> regmap APIS. Driver is also using some globals that are not needed at
>> all if a driver data structure is used along the code. Hence, add all
>> new needed stuff inside a new driver data structure. With this changes
>> driver can be properly compile tested.
>>
>> Thanks in advance for reviewing this!
>>
>> v1 of this series here [0].
>> v2 of this series here [1].
>> v3 of this series here [2].
>> v4 of this series here [3].
>> v5 of this series here [4].
>>
>> Changes in v6:
>>      - Get regmap syscon using 'syscon_regmap_lookup_by_compatible()'
>>      - Add COMPILE_TEST and dependencies to Kconfig.
>>      - Collect Philippe Mathieu-Daudé 'Reviewed-by' tag for watchdog node
>>        warning fix.
>>
>> Changes in v5:
>>      - Drop patches related with device tree ABI breakage and only
>>        maintain the rest.
> 
> This makes me regret taking mt7621-dts out of drivers/staging. Clearly 
> there were improvements to be made on the MT7621 drivers that would 
> affect the devicetree binding. I don't think one would bat an eye to 
> make dt-binding changes on a devicetree on drivers/staging.
> 
> I also want to make a similar change that would break the ABI. I want to 
> split the MediaTek MIPS (currently called Ralink) pinctrl subdrivers 
> further, namely mt7620.c, to split MT7628/MT7688 pinmux data from 
> MT7620, so I can properly document which function can be given to which 
> group(s) for the MT7628/MT7688 SoCs.
> 
> This means I've got to introduce a new compatible string for the MT76X8 
> SoCs which would cause the pinctrl driver to stop working with an old DT 
> for the MT76X8 SoCs. By the nature of the change, I can't make a way 
> around with the code like Sergio did in v6.

Now that I think about it, I think there's a way I can introduce a new 
compatible string for the sake of better documentation, and keep the 
driver working with old DTs. I'd not like to convolute this series with 
this conversation. I'll send a mail to the related mailing list when the 
time is right.

Arınç