Re: [PATCH v3 2/2] watchdog: Add Toshiba Visconti watchdog driver

From: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
To: Punit Agrawal <punit1.agrawal@toshiba.co.jp>
Cc: Rob Herring <robh+dt@kernel.org>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	Guenter Roeck <linux@roeck-us.net>,
	devicetree@vger.kernel.org, linux-watchdog@vger.kernel.org,
	yuji2.ishikawa@toshiba.co.jp,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 2/2] watchdog: Add Toshiba Visconti watchdog driver
Date: Mon, 21 Sep 2020 17:46:46 +0900	[thread overview]
Message-ID: <20200921084646.w4cps33nnxnqxefx@toshiba.co.jp> (raw)
In-Reply-To: <87d02flhfr.fsf@kokedama.swc.toshiba.co.jp>

Hi,

Thanks for your review.

On Mon, Sep 21, 2020 at 04:25:44PM +0900, Punit Agrawal wrote:
> Iwamatsu-san,
> 
> Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> writes:
> 
> > Add the watchdog driver for Toshiba Visconti series.
> >
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> > ---
> >  drivers/watchdog/Kconfig        |   8 ++
> >  drivers/watchdog/Makefile       |   1 +
> >  drivers/watchdog/visconti_wdt.c | 191 ++++++++++++++++++++++++++++++++
> >  3 files changed, 200 insertions(+)
> >  create mode 100644 drivers/watchdog/visconti_wdt.c
> >
> 
> [...]
> 
> > diff --git a/drivers/watchdog/visconti_wdt.c b/drivers/watchdog/visconti_wdt.c
> > new file mode 100644
> > index 000000000000..4a67b9fe9220
> > --- /dev/null
> > +++ b/drivers/watchdog/visconti_wdt.c
> > @@ -0,0 +1,191 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2020 TOSHIBA CORPORATION
> > + * Copyright (c) 2020 Toshiba Electronic Devices & Storage Corporation
> > + * Copyright (c) 2020 Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> 
> interrupt.h doesn't seem to be used. It can be dropped.
> 

Right. I will remove this.

> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/watchdog.h>
> > +
> > +#define WDT_CNT			0x00
> > +#define WDT_MIN			0x04
> > +#define WDT_MAX			0x08
> > +#define WDT_CTL			0x0c
> > +#define WDT_CMD			0x10
> > +#define WDT_CMD_CLEAR		0x4352
> > +#define WDT_CMD_START_STOP	0x5354
> > +#define WDT_DIV			0x30
> > +
> > +#define VISCONTI_WDT_FREQ	2000000 /* 2MHz */
> > +#define WDT_DEFAULT_TIMEOUT	10U /* in seconds */
> > +
> > +static bool nowayout = WATCHDOG_NOWAYOUT;
> > +module_param(nowayout, bool, 0);
> > +MODULE_PARM_DESC(
> > +	nowayout,
> > +	"Watchdog cannot be stopped once started (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT)")");

<snip>

> > +module_platform_driver(visconti_wdt_driver);
> > +
> > +MODULE_DESCRIPTION("TOSHIBA Visconti Watchdog Driver");
> > +MODULE_AUTHOR("TOSHIBA ELECTRONIC DEVICES & STORAGE CORPORATION");
> 
> The MODULE_AUTHOR() macro is usually used to represent the driver
> authors. Not sure about using it for the organization name which is
> already included in the copyright notice. Please drop it or replace it
> with the author name / email.

Well, it's unnecessary for MODULE_AUTHOR.
I will drop this.

> 
> > +MODULE_AUTHOR("Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp");
> > +MODULE_LICENSE("GPL v2");
> 
> Reviewed-by: Punit Agrawal <punit1.agrawal@toshiba.co.jp>
> 

Thanks!

> Thanks,
> Punit
> 

Best regards,
  Nobuhiro