[PATCH v3 2/2] renesas: Renesas R-Car Gen4 watchdog driver

[Marek Vasut]

On 6/3/25 5:06 AM, Shmuel Leib Melamud via B4 Relay wrote:

[...]

> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright 2025 Red Hat, Inc., Shmuel Leib Melamud <smelamud at redhat.com>
> +
> +#include <clk.h>
> +#include <dm.h>
> +#include <wdt.h>
> +#include <asm/io.h>
> +#include <dm/device_compat.h>
> +#include <linux/delay.h>
> +#include <linux/iopoll.h>

Nitpick, keep the list sorted.

> +#define usleep_range(a, b) udelay((b))

Shouldn't this be udelay((a)) , the shorter (a) delay should be 
sufficient in all cases, shouldn't it ?

[...]

> +static int rwdt_probe(struct udevice *dev)
> +{
> +	struct rwdt_priv *priv = dev_get_priv(dev);
> +	unsigned long clks_per_sec;
> +	int ret, i;
> +
> +	priv->wdt = dev_remap_addr(dev);
> +	if (!priv->wdt)
> +		return -EINVAL;
> +
> +	ret = clk_get_by_index(dev, 0, &priv->clk);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = clk_enable(&priv->clk);
> +	if (ret)
> +		return ret;
> +
> +	priv->clk_rate = clk_get_rate(&priv->clk);
> +	if (!priv->clk_rate) {
> +		ret = -ENOENT;
> +		goto err_clk_disable;
> +	}

This loop below could use a code comment clarifying what this does.

> +	for (i = ARRAY_SIZE(clk_divs) - 1; i >= 0; i--) {
> +		clks_per_sec = priv->clk_rate / clk_divs[i];
> +		if (clks_per_sec && clks_per_sec < 65536) {
> +			priv->cks = i;
> +			break;
> +		}
> +	}

A few basic nitpicks, very nice otherwise, thanks !