Re: [PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers
From: Boris Lysov <hidden>
Date: 2021-02-02 01:34:54
Also in:
linux-mediatek
В Sun, 31 Jan 2021 16:31:09 -0800 Guenter Roeck [off-list ref] пишет:
We can't do this. With this flag enabled, the watchdog won't support other SoCs, and there is nothing that prevents the flag from being set for those SoCs. This has to be handled differently, without configuration flag. Maybe use regmap for register addresses, [snip], or use accessor functions in mtk_wdt_dev.
Thank you for reviewing my patch! I will consider suggested fixes, and I will come up with better solution in V2. I'm beginner developer, and am still learning.
use the compatible string to determine which regmap settings to use
I think relying on hardcoded "compatible string - settings" pairs in driver is not good. Whilst most Mediatek watchdogs I've seen use similar drivers, no one (except Mediatek itself, of course) knows for sure how many devices use 16-bit mode, and specifying each one in C code may _theoretically_ bloat it. (well, on the other hand, I've seen other watchdog drivers with many compatible devices listed in C code, and they didn't seem bloated at all) What do you think about implementing a simple boolean flag in dt-binding for enabling the 16-bit operation mode? Something like "mediatek,watchdog-16bits" [1] , which the driver would check for in the `mtk_wdt_probe` and set corresponding regmaps. As result, there won't be a need for kernel configuration flag, and other watchdogs would be supported. Most likely this idea doesn't sound good as I portray it, but I would still like to hear your opinion about it. Thanks. References: [1] Mediatek UART APDMA driver uses similar flag called `mediatek,dma-33bits` Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt