Hi Guenter,
Thanks again for the review :) I agree with all of your comments, thanks!
On 4/30/24 19:24, Guenter Roeck wrote:
On 4/30/24 05:01, Matti Vaittinen wrote:
Introduce driver for WDG block on ROHM BD96801 scalable PMIC.
This driver only supports watchdog with I2C feeding and delayed
response detection. Whether the watchdog toggles PRSTB pin or
just causes an interrupt can be configured via device-tree.
The BD96801 PMIC HW supports also window watchdog (too early
feeding detection) and Q&A mode. These are not supported by
this driver.
Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
---
Revision history:
RFCv2 => v1:
- Fix watchdog time-outs to match DS4
- Fix target timeout overflow
- Improve dbg prints
RFCv1 => RFCv2:
- remove always running
- add IRQ handling
- call emergency_restart()
- drop MODULE_ALIAS and add MODULE_DEVICE_TABLE
---
...
+
+static int bd96801_set_wdt_mode(struct wdtbd96801 *w, unsigned int
hw_margin,
+ unsigned int hw_margin_min)
+{
+ int fastng, slowng, type, ret, reg, mask;
+ struct device *dev = w->dev;
+
+ /*
+ * Convert to 100uS to guarantee reasonable timeouts fit in
+ * 32bit maintaining also a decent accuracy.
+ */
+ hw_margin *= 10;
+ hw_margin_min *= 10;
+
+ if (hw_margin_min) {
+ unsigned int min;
+
+ type = BD96801_WD_TYPE_WIN;
+ dev_dbg(dev, "Setting type WINDOW 0x%x\n", type);
+ ret = find_closest_fast(hw_margin_min, &fastng, &min);
+ if (ret) {
+ dev_err(dev, "bad WDT window for fast timeout\n");
+ return ret;
+ }
+
+ ret = find_closest_slow_by_fast(min, &hw_margin, &slowng);
+ if (ret) {
+ dev_err(dev, "bad WDT window\n");
+ return ret;
+ }
+ w->wdt.min_hw_heartbeat_ms = min / 10;
+ } else {
+ type = BD96801_WD_TYPE_SLOW;
+ dev_dbg(dev, "Setting type SLOW 0x%x\n", type);
+ ret = find_closest_slow(&hw_margin, &slowng, &fastng);
+ if (ret) {
+ dev_err(dev, "bad WDT window\n");
What is the value of those error messages ? To me they only leave big
question marks. One would see "bad WDT window" or "bad WDT window for
fast timeout" and then what ? If the cause is bad values for hw_margin
and/or hw_margin_min, the message should include the offending values
and not leave the user in the dark.
Well, at least one can grep the error from module which printed it :)
But yes, you have a very valid point. I will see how to improve, thanks!
+ return ret;
+ }
+ }
+
+ w->wdt.max_hw_heartbeat_ms = hw_margin / 10;
+
+ fastng <<= ffs(BD96801_WD_TMO_SHORT_MASK) - 1;
Any reason fore not using standard functionality such as FIELD_PREP here ?
A reason, yes. A good reason, no :) Reason is that I wrote this before I
learned about the FIELD_PREP() and FIELD_GET(). And actually, I have
always found those two more confusing than the open-coded shift, '|' and
'&'. Still, I believe the FIELD_GET() and FIELD_PREP() make it obvious
for a reader that the bit magic is just a standard register field stuff.
So, no good reason. I'll fix this for the next version.
...
+extern void emergency_restart(void);
No. include linux/reboot.h instead.
Hmm.. I am sure I had checked the headers for prototypes before doing
something like this... I think I must've missed something. I'll fix this
for the next version, or add a good explanation if there indeed was some
problem finding the prototype.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
