Hallo and thanks for your comments
Guenter Roeck schrieb am 22.07.22 um 17:08:
On 7/22/22 02:51, David Müller wrote:
+/* Includes */
There are various pointless comments in this driver.
This is one of them.
Ok, will fix.
+/* WDT runtime registers */
+#define WDT_CTRL 0x00
+#define WDT_VAL 0x01
+/* the millisec feature is not supported */
And this is another one. Readers won't know that the unit to be written
into the chip includes two bits, not juust one, and that 00 reflects
a 10ms timeout units.
+#define WDT_UNITS_SEC BIT(1)
+#define WDT_UNITS_MIN BIT(2)
This is a bit misleading, since the two bits combined control the
interval. That should be explained. The comment above doesn't do this.
What do you purpose / is prefered?
#define WDT_UNITS_MS 0x0 /* not supported / used */
#define WDT_UNITS_SEC 0x2
#define WDT_UNITS_MIN 0x4
or
enum wdt_units {
WDT_UNITS_MS = 0, /* not supported / used */
WDT_UNITS_SEC = 2,
WDT_UNITS_MIN = 4,
};
or something else?
+/* Module parameters */
+#define WATCHDOG_TIMEOUT 60 /* 60 sec default timeout */
+static int timeout = WATCHDOG_TIMEOUT; /* in seconds */
Should be pre-initialized with 0 and the default should be set in struct
watchdog_device to reduce runtime conplexity.
Ok, will fix.
+ /* read the MSB */
+ val = exar_sio_read(config_port, reg);
+
+ /* read and merge in the LSB */
+ val = (val << 8) | exar_sio_read(config_port, reg + 1);
+
More pointless comments. Please limit comments to places
where they are useful.
Ok, will fix.
+static void exar_wdt_disarm(const struct wdt_priv *priv)
+{
+ outb(0xFF, priv->rt_base + WDT_VAL);
+ outb(0, priv->rt_base + WDT_VAL);
The datasheet says that writing a non-0 value stops the watchdog,
but it doesn't explain what happens when writing 0 into the register.
This does warrant an explanation.
The datasheet is a little bit unclear about how to correctly stop the
watchdog. In my tests with actual hardware, the sequence above worked.
But I will check again.
+static int exar_wdt_set_timeout(struct watchdog_device *wdog,
unsigned int t)
+{
+ struct wdt_priv *priv = watchdog_get_drvdata(wdog);
+ unsigned char timeout_unit = WDT_UNITS_SEC;
+
+ if (watchdog_timeout_invalid(wdog, t))
+ return -EINVAL;
+
Unnecessary check; done by framework.
Ok, will fix.
+ /*
+ * if new timeout is bigger then 255 seconds, change the
+ * unit to minutes and round the timeout up to the next whole minute
+ */
+ if (t > 255) {
+ timeout_unit = WDT_UNITS_MIN;
+ t = DIV_ROUND_UP(t, 60);
+ }
+
+ /* save for later use in exar_wdt_start() */
+ priv->unit = timeout_unit;
Using a bool for timeout_unit would make it easier for the compiler.
+ priv->timeout = t;
+
+ wdog->timeout = (timeout_unit == WDT_UNITS_MIN) ? t * 60 : t;
+
This doesn't update the watchdog registers when the watchdog is running.
This means that the watchdog may fire unexpectedly if the timeout is
changed from a small to a large value.
Is there a reason why the framework does not issue a stop(), update(),
start() sequence when the timeout value is changed?
+ ret = exar_sio_enter(config_port, key);
+ if (ret) {
+ pr_info("config space unavailable\n (err: %d", ret);
Please no such noise. Also, there is an unbalanced ( in the string.
Ok, will fix.
+ return 0;
+ }
+
+ vid = exar_sio_read16(config_port, EXAR_VID);
+ did = exar_sio_read16(config_port, EXAR_DID);
+
+ /* check for the vendor and device IDs we currently know about */
+ if (vid == 0x13A8 && /* vendor Exar/MaxLinar */
+ (did == 0x0382 || /* UART XR28V382 */
+ did == 0x0384)) { /* UART XR28V384 */
It might be useful to have defines for those values.
Ok, will fix.
+ /* set LD config reg to WDT device (8) */
+ exar_sio_write(config_port, EXAR_LDN, 8);
+ /* is device active? */
+ if (exar_sio_read(config_port, EXAR_ACT) == 0x01) {
+ /* get the WDT runtime registers base address*/
+ base = exar_sio_read16(config_port, EXAR_RTBASE);
+
+ /* set global WDT control to
+ * assert WDTOUT / rearm by read
+ */
Please use standard multi-line comments.
Ok, will fix.
+ exar_sio_write(config_port, EXAR_WDT, 0);
The detect function should really not do this. Configuration and detection
should be separate functions.
Ok, will fix.
+static int __init exar_wdt_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct wdt_priv *priv;
+ int ret, i, j;
+ unsigned short did = 0, rt_base = 0;
+
+ /* search for the first active Exar WDT on all possible locations */
+ for (i = 0; !did && (i < ARRAY_SIZE(sio_config_ports)); i++) {
+ for (j = 0; !did && (j < ARRAY_SIZE(sio_enter_keys)); j++)
+ did = exar_detect(sio_config_ports[i],
+ sio_enter_keys[j],
+ &rt_base);
+ }
+
+ if (!did)
+ return -ENODEV;
This should really be done in the init function, and the init function
should only instantiate the watchdog platform driver if a device was found.
Ok, will move the detection part to the init() function.
Also, this assumes either that only one supported device is in the system,
or that the watchdog is always connected to the first device. The init
function should really instantiate devices for each detected chip,
not just for the first one.
Yes, only one active Exar WDT device is supported.
Even if there are multiple Exar UART chips present in the system, it
doesn't make much sense to have more than one Exar WDT active and
supported at a time IMHO.
+ if (watchdog_timeout_invalid(&wdt_dev, timeout))
+ /* invalid timeout specified, reset to default */
+ timeout = WATCHDOG_TIMEOUT;
It would be easier to just initialize .timeout in struct watchdog_device
and let watchdog_init_timeout() validate the configured value.
Ok, will fix.
+ if (!devm_request_region(dev,
+ priv->rt_base + WDT_CTRL, 2, DRV_NAME)) {
+ dev_err(dev, "failed to request region 0x%04x-0x%04x.\n",
+ priv->rt_base + WDT_CTRL, priv->rt_base + WDT_VAL);
+ return -EBUSY;
This should be -ENOMEM.
Ok, will fix.
