Thread (16 messages) 16 messages, 4 authors, 2021-05-10

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help