[PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers

[Guenter Roeck]

On 2/1/21 5:33 PM, Boris Lysov wrote:
> В Sun, 31 Jan 2021 16:31:09 -0800
> Guenter Roeck <linux at roeck-us.net> пишет:
> 
>> 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
>