All of lore.kernel.org
 help / color / mirror / Atom feed
From: Boris Lysov <arzamas-16@mail.ee>
To: Guenter Roeck <linux@roeck-us.net>, devicetree@vger.kernel.org
Cc: matthias.bgg@gmail.com, robh+dt@kernel.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers
Date: Tue, 2 Feb 2021 04:33:55 +0300	[thread overview]
Message-ID: <20210202043355.18080818@pc> (raw)
In-Reply-To: <050f2f8e-9c3c-10e3-05ef-cd84e949b98f@roeck-us.net>

В Sun, 31 Jan 2021 16:31:09 -0800
Guenter Roeck <linux@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
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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

WARNING: multiple messages have this Message-ID (diff)
From: Boris Lysov <arzamas-16@mail.ee>
To: Guenter Roeck <linux@roeck-us.net>, devicetree@vger.kernel.org
Cc: matthias.bgg@gmail.com, robh+dt@kernel.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers
Date: Tue, 2 Feb 2021 04:33:55 +0300	[thread overview]
Message-ID: <20210202043355.18080818@pc> (raw)
In-Reply-To: <050f2f8e-9c3c-10e3-05ef-cd84e949b98f@roeck-us.net>

В Sun, 31 Jan 2021 16:31:09 -0800
Guenter Roeck <linux@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
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

  reply	other threads:[~2021-02-02  1:34 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-31 23:44 [PATCH 0/3] watchdog: mtk_wdt: add support for mt6577 Boris Lysov
2021-01-31 23:44 ` Boris Lysov
2021-01-31 23:44 ` [PATCH 1/3] dt-bindings: watchdog: mediatek: add support for mt6577 SoC Boris Lysov
2021-01-31 23:44   ` Boris Lysov
2021-02-09 20:41   ` Rob Herring
2021-02-09 20:41     ` Rob Herring
2021-01-31 23:44 ` [PATCH 2/3] watchdog: mtk_wdt: add support for 16-bit control registers Boris Lysov
2021-01-31 23:44   ` Boris Lysov
2021-02-01  0:31   ` Guenter Roeck
2021-02-01  0:31     ` Guenter Roeck
2021-02-02  1:33     ` Boris Lysov [this message]
2021-02-02  1:33       ` Boris Lysov
2021-02-02  1:55       ` Guenter Roeck
2021-02-02  1:55         ` Guenter Roeck
2021-02-10 11:37       ` Matthias Brugger
2021-02-10 11:37         ` Matthias Brugger
2021-01-31 23:44 ` [PATCH 3/3] watchdog: mtk_wdt: declare compatibility with mt6577 Boris Lysov
2021-01-31 23:44   ` Boris Lysov
2021-05-09 21:16 ` [PATCH v2 0/3] watchdog: mtk_wdt: refactor code to support more watchdogs Boris Lysov
2021-05-09 21:16   ` Boris Lysov
2021-05-09 21:17   ` [PATCH v2 1/3] watchdog: mtk_wdt: Refactor code to support more SoCs Boris Lysov
2021-05-09 21:17     ` Boris Lysov
2021-05-10 13:21     ` Guenter Roeck
2021-05-10 13:21       ` Guenter Roeck
2021-05-10 22:27       ` Boris Lysov
2021-05-10 22:27         ` Boris Lysov
2021-05-09 21:17   ` [PATCH v2 2/3] dt-bindings: watchdog: mediatek: add support for mt6577 SoC Boris Lysov
2021-05-09 21:17     ` Boris Lysov
2021-05-10 16:17     ` Rob Herring
2021-05-10 16:17       ` Rob Herring
2021-05-09 21:17   ` [PATCH v2 3/3] watchdog: mtk_wdt: add support for mt6577 Boris Lysov
2021-05-09 21:17     ` Boris Lysov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210202043355.18080818@pc \
    --to=arzamas-16@mail.ee \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux@roeck-us.net \
    --cc=matthias.bgg@gmail.com \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.