Re: [PATCH v3 03/10] drivers: wdt: Introduce HPE GXP SoC Watchdog

[Guenter Roeck <linux@roeck-us.net>]

On 4/4/22 09:25, Hawkins, Nick wrote:
> 
> 
> -----Original Message-----
> From: Guenter Roeck [mailto:linux@roeck-us.net]
> Sent: Monday, April 4, 2022 9:29 AM
> To: Hawkins, Nick <nick.hawkins@hpe.com>
> Cc: Verdun, Jean-Marie <verdun@hpe.com>; Wim Van Sebroeck <wim@linux-watchdog.org>; linux-kernel@vger.kernel.org; linux-watchdog@vger.kernel.org
> Subject: Re: [PATCH v3 03/10] drivers: wdt: Introduce HPE GXP SoC Watchdog
> 
> On Thu, Mar 10, 2022 at 01:52:22PM -0600, nick.hawkins@hpe.com wrote:
>>> From: Nick Hawkins <nick.hawkins@hpe.com>
>>>
>>> Adding support for the HPE GXP Watchdog. It is new to the linux
>>> community and this along with several other patches is the first
>>> support for it. The GXP asic contains a full compliment of timers one
>>> of which is the watchdog timer. The watchdog timer is 16 bit and has
>>> 10ms resolution.
>>>
>>> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
>>> ---
>>>   drivers/watchdog/Kconfig   |   8 ++
>>>   drivers/watchdog/Makefile  |   1 +
>>>   drivers/watchdog/gxp-wdt.c | 191
>>> +++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 200 insertions(+)
>>>   create mode 100644 drivers/watchdog/gxp-wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
>>> c8fa79da23b3..cb210d2978d2 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -1820,6 +1820,14 @@ config RALINK_WDT
>>>   	help
>>>   	  Hardware driver for the Ralink SoC Watchdog Timer.
>>>   
>>> +config GXP_WATCHDOG
>>> +	tristate "HPE GXP watchdog support"
>>> +	depends on ARCH_HPE_GXP
>>> +	select WATCHDOG_CORE
>>> +	help
>>> +	  Say Y here to include support for the watchdog timer
>>> +	  in HPE GXP SoCs.
>>> +
>>>   config MT7621_WDT
>>>   	tristate "Mediatek SoC watchdog"
>>>   	select WATCHDOG_CORE
>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>> index f7da867e8782..e2acf3a0d0fc 100644
>>> --- a/drivers/watchdog/Makefile
>>> +++ b/drivers/watchdog/Makefile
>>> @@ -92,6 +92,7 @@ obj-$(CONFIG_RTD119X_WATCHDOG) += rtd119x_wdt.o
>>>   obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
>>>   obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o
>>>   obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o
>>> +obj-$(CONFIG_GXP_WATCHDOG) += gxp-wdt.o
>>>   obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
>>>   obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
>>>   obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o diff --git
>>> a/drivers/watchdog/gxp-wdt.c b/drivers/watchdog/gxp-wdt.c new file
>>> mode 100644 index 000000000000..d2b489cb4774
>>> --- /dev/null
>>> +++ b/drivers/watchdog/gxp-wdt.c
>>> @@ -0,0 +1,191 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P.
>>> + *
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> +modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include <linux/delay.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/types.h>
>>> +#include <linux/watchdog.h>
>>> +
>>> +#define MASK_WDGCS_ENABLE	0x01
>>> +#define MASK_WDGCS_RELOAD	0x04
>>> +#define MASK_WDGCS_NMIEN	0x08
>>> +#define MASK_WDGCS_WARN		0x80
>>> +
>>> +#define WDT_MAX_TIMEOUT_MS	655000
>>> +#define WDT_DEFAULT_TIMEOUT	30
>>> +#define SECS_TO_WDOG_TICKS(x) ((x) * 100) #define
>>> +WDOG_TICKS_TO_SECS(x) ((x) / 100)
>>> +
>>> +struct gxp_wdt {
>>> +	void __iomem	*counter;
>>> +	void __iomem	*control;
>>> +	struct watchdog_device	wdd;
> 
>> Odd variable alignment. Might as well just use spaces before the variable names.
> 
> Fixed
> 
>>> +};
>>> +
>>> +static void gxp_wdt_enable_reload(struct gxp_wdt *drvdata) {
>>> +	uint8_t val;
>>> +
>>> +	val = readb(drvdata->control);
>>> +	val |= (MASK_WDGCS_ENABLE | MASK_WDGCS_RELOAD);
>>> +	writeb(val, drvdata->control);
>>> +}
>>> +
>>> +static int gxp_wdt_start(struct watchdog_device *wdd) {
>>> +	struct gxp_wdt *drvdata = watchdog_get_drvdata(wdd);
>>> +
>>> +	writew((SECS_TO_WDOG_TICKS(wdd->timeout)), drvdata->counter);
> 
>> Unnecessary iand confusing () around SECS_TO_WDOG_TICKS().
> 
> Fixed
> 
>>> +	gxp_wdt_enable_reload(drvdata);
>>> +	return 0;
>>> +}
>>> +
>>> +static int gxp_wdt_stop(struct watchdog_device *wdd) {
>>> +	struct gxp_wdt *drvdata = watchdog_get_drvdata(wdd);
>>> +	uint8_t val;
>>> +
>>> +	val = readb_relaxed(drvdata->control);
>>> +	val &= ~MASK_WDGCS_ENABLE;
>>> +	writeb(val, drvdata->control);
>>> +	return 0;
>>> +}
>>> +
>>> +static int gxp_wdt_set_timeout(struct watchdog_device *wdd,
>>> +			       unsigned int timeout)
>>> +{
>>> +	struct gxp_wdt *drvdata = watchdog_get_drvdata(wdd);
>>> +	uint32_t actual;
> 
>> Please use u32 as suggested by checkpatch. Same everywhere.
> 
> Fixed, checkpatch did not flag this, is there an option I should be using with checkpatch.pl?

--strict

Guenter