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

Guenter Roeck
2022-04-04T21:51:13Z

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