On 2/1/21 5:33 PM, Boris Lysov wrote: > В Sun, 31 Jan 2021 16:31:09 -0800 > Guenter Roeck <linux@xxxxxxxxxxxx> пишет: > >> 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 So you are saying that the existing mediatek watchdog driver is not good ? Or that it is good for setting the number of supported reset pins, but not for setting the register width ? > 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. > I don't like it at all, I don't see your problem, and mediatek,dma-33bits seems to be a completely different scope (all it does is to trigger a couple of additional writes, not control register width). On the other side, you would have to sell it to dt maintainers, not to me. Guenter > References: > [1] Mediatek UART APDMA driver uses similar flag > called `mediatek,dma-33bits` > Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt >
