On 08/12/2014 02:56 AM, Markus Pargmann wrote:

From: Krystian Garbaciak <redacted>

This driver supports watchdog device inside DA906x PMIC.

Signed-off-by: Krystian Garbaciak <redacted>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Markus Pargmann <redacted>

What is different to v1 ?

It might be better to return -EINVAL and use it in the calling code.

+}
+
+static int da9063_wdt_disable(struct da9063 *da9063)
+{
+	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
+			DA9063_TWDSCALE_MASK,
+			DA9063_TWDSCALE_DISABLE << DA9063_TWDSCALE_SHIFT);
+}
+
+static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
+{
+	if (regval < DA9063_TWDSCALE_MIN || regval > DA9063_TWDSCALE_MAX)
+		return -EINVAL;
+
+	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
+			DA9063_TWDSCALE_MASK, regval << DA9063_TWDSCALE_SHIFT);
+}
+
+static int _da9063_wdt_kick(struct da9063 *da9063)
+{
+	return regmap_write(da9063->regmap, DA9063_REG_CONTROL_F,
+			DA9063_WATCHDOG);
+}
+
+static int da9063_wdt_start(struct watchdog_device *wdd)
+{
+	struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd);
+	unsigned int selector;
+	int ret = 0;
+
+	mutex_lock(&wdt->lock);
+	selector = da9063_wdt_timeout_to_sel(wdt->wdtdev->timeout);
+	ret = _da9063_wdt_set_timeout(wdt->da9063, selector);
+	if (ret) {
+		dev_err(wdt->da9063->dev, "Watchdog failed to start (err = %d)\n",
+				ret);
+	}
+	mutex_unlock(&wdt->lock);
+
+	return ret;
+}
+
+static int da9063_wdt_stop(struct watchdog_device *wdd)
+{
+	struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd);
+	int ret = 0;
+
+	mutex_lock(&wdt->lock);
+	ret = da9063_wdt_disable(wdt->da9063);
+	if (ret)
+		dev_alert(wdt->da9063->dev,
+			  "Watchdog failed to stop (err = %d)\n", ret);
+	mutex_unlock(&wdt->lock);
+
+	return ret;
+}
+
+static int da9063_wdt_ping(struct watchdog_device *wdd)
+{
+	struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd);
+	int ret = 0;
+
+	mutex_lock(&wdt->lock);
+	ret = _da9063_wdt_kick(wdt->da9063);
+	if (ret)
+		dev_alert(wdt->da9063->dev, "Failed to ping the watchdog (err = %d)\n",
+				ret);
+	mutex_unlock(&wdt->lock);
+
+	return ret;
+}
+
+static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
+							unsigned int timeout)
+{
+	struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd);
+	unsigned int selector;
+	int ret;
+
+	selector = da9063_wdt_timeout_to_sel(timeout);
+	if (selector == 0) {
+		dev_err(wdt->da9063->dev, "Unsupported watchdog timeout (select from: 2, 4, 8, 16, 32, 65, 131)\n");

This error case should never happen, since the range is checked by the calling code.
Besides, the error message is wrong. You do accept the entire range of [1 .. 131],
not just selected values.

Overall the error checking is inconsistent. Above you don't check the return code
but depend on _da9063_wdt_set_timeout to detect a range error.

+		return -EINVAL;
+	}
+
+	mutex_lock(&wdt->lock);
+
+	ret = _da9063_wdt_set_timeout(wdt->da9063, selector);
+	if (ret)
+		dev_err(wdt->da9063->dev, "Failed to set watchdog timeout (err = %d)\n",
+				ret);
+	else
+		wdd->timeout = wdt_timeout[selector];
+
+	mutex_unlock(&wdt->lock);
+
+	return ret;
+}
+
+static const struct watchdog_info da9063_watchdog_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = "DA9063 Watchdog",
+};
+
+static const struct watchdog_ops da9063_watchdog_ops = {
+	.owner = THIS_MODULE,
+	.start = da9063_wdt_start,
+	.stop = da9063_wdt_stop,
+	.ping = da9063_wdt_ping,
+	.set_timeout = da9063_wdt_set_timeout,
+};
+
+static struct watchdog_device da9063_watchdog_device = {
+	.info = &da9063_watchdog_info,
+	.ops = &da9063_watchdog_ops,
+};
+
+static int da9063_wdt_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct da9063 *da9063 = dev_get_drvdata(pdev->dev.parent);
+	struct da9063_watchdog *wdt;
+
+	if (!da9063)
+		return -EPROBE_DEFER;
+
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	wdt->da9063 = da9063;
+	wdt->wdtdev = &da9063_watchdog_device;
+	mutex_init(&wdt->lock);
+
+	if (IS_ENABLED(CONFIG_WATCHDOG_NOWAYOUT))
+		set_bit(WDOG_NO_WAY_OUT, &wdt->wdtdev->status);
+

Why not use watchdog_set_nowayout, or initialize .status with
WATCHDOG_NOWAYOUT_INIT_STATUS, as suggested in the API documentation ?

+	wdt->wdtdev->min_timeout = DA9063_WDT_MIN_TIMEOUT;
+	wdt->wdtdev->max_timeout = DA9063_WDT_MAX_TIMEOUT;
+	wdt->wdtdev->timeout = DA9063_WDG_TIMEOUT;
+

Why don't you set those variables directly in da9063_watchdog_device ?
If you want to set it here, you might as well set ops and info
and get rid of the static variable (by including the entire structure
in wdt, not just a pointer to it).

+	ret = watchdog_register_device(wdt->wdtdev);
+	if (ret) {
+		dev_err(da9063->dev, "da9063-watchdog registration failed (err = %d)",
+				ret);
+		return ret;
+	}
+
+	watchdog_set_drvdata(wdt->wdtdev, wdt);
+

This creates a race condition, because at least in theory an access could
already occur between the watchdog registration above and this code.

That is unnecessary. Why shift if there is nothing to shift ?
It just adds unnecessary complexity to the code.

  #define	DA9063_BLINK_FRQ_MASK			0x38
  #define		DA9063_BLINK_FRQ_OFF		0x00
  #define		DA9063_BLINK_FRQ_1S0		0x08