Re: [PATCH 1/3] watchdog: at91sam9_wdt.h: Cleanup the header file

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 27/02/2026 13:01:14+0530, Balakrishnan Sambath wrote:
> From: Andrei Simion <andrei.simion@microchip.com>
> 
> This patch reorganizes the header file by renaming the registers using
> a general pattern also this patch simplifies the watchdog disable logic
> in the at91sam9_wdt.h header by differentiating between modern
> (SAM9X60, SAMA7G5, SAM9X7) and legacy (SAMA5, AT91SAM9261) chips based
> on the watchdog disable bit.
> For modern chips, the disable bit is at bit 12, while for legacy chips
> it is at bit 15.
> 
> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
> [Remove Kconfig-based WDDIS selection and define explicit legacy and
> modern masks]
> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
> ---
>  drivers/watchdog/at91sam9_wdt.h | 65 ++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h
> index 298d545df1a1..1e0aeecb489f 100644
> --- a/drivers/watchdog/at91sam9_wdt.h
> +++ b/drivers/watchdog/at91sam9_wdt.h
> @@ -3,59 +3,58 @@
>   * drivers/watchdog/at91sam9_wdt.h
>   *
>   * Copyright (C) 2007 Andrew Victor
>   * Copyright (C) 2007 Atmel Corporation.
>   * Copyright (C) 2019 Microchip Technology Inc. and its subsidiaries
>   *
>   * Watchdog Timer (WDT) - System peripherals regsters.
>   * Based on AT91SAM9261 datasheet revision D.
>   * Based on SAM9X60 datasheet.
> + * Based on SAMA7G5 datasheet.
> + * Based on SAM9X75 datasheet.
>   *
>   */
>  
>  #ifndef AT91_WDT_H
>  #define AT91_WDT_H
>  
>  #include <linux/bits.h>
>  
> -#define AT91_WDT_CR		0x00			/* Watchdog Control Register */
> -#define  AT91_WDT_WDRSTT	BIT(0)			/* Restart */
> -#define  AT91_WDT_KEY		(0xa5UL << 24)		/* KEY Password */
> -
> -#define AT91_WDT_MR		0x04			/* Watchdog Mode Register */
> -#define  AT91_WDT_WDV		(0xfffUL << 0)		/* Counter Value */
> -#define  AT91_WDT_SET_WDV(x)	((x) & AT91_WDT_WDV)
> -#define  AT91_SAM9X60_PERIODRST	BIT(4)		/* Period Reset */
> -#define  AT91_SAM9X60_RPTHRST	BIT(5)		/* Minimum Restart Period */
> +#define AT91_WDT_CR		0x00		/* Watchdog Control Register */
> +#define  AT91_WDT_WDRSTT	BIT(0)		/* Restart */
> +#define  AT91_WDT_KEY		(0xa5UL << 24)	/* KEY Password */
> +#define AT91_WDT_MR		0x04		/* Watchdog Mode Register */
> +#define  AT91_WDT_WDV		(0xfffUL << 0)	/* Counter Value */
>  #define  AT91_WDT_WDFIEN	BIT(12)		/* Fault Interrupt Enable */
> -#define  AT91_SAM9X60_WDDIS	BIT(12)		/* Watchdog Disable */
> -#define  AT91_WDT_WDRSTEN	BIT(13)		/* Reset Processor */
> -#define  AT91_WDT_WDRPROC	BIT(14)		/* Timer Restart */
> -#define  AT91_WDT_WDDIS		BIT(15)		/* Watchdog Disable */
> -#define  AT91_WDT_WDD		(0xfffUL << 16)		/* Delta Value */
> -#define  AT91_WDT_SET_WDD(x)	(((x) << 16) & AT91_WDT_WDD)
> -#define  AT91_WDT_WDDBGHLT	BIT(28)		/* Debug Halt */
> -#define  AT91_WDT_WDIDLEHLT	BIT(29)		/* Idle Halt */
> -
> +#define  AT91_WDT_WDRSTEN	BIT(13)
> +#define  AT91_WDT_WDRPROC	BIT(14)
> +#define  AT91_WDT_WDD		(0xfffUL << 16)
> +#define  AT91_WDT_WDDBGHLT	BIT(28)
> +#define  AT91_WDT_WDIDLEHLT	BIT(29)
>  #define AT91_WDT_SR		0x08		/* Watchdog Status Register */
>  #define  AT91_WDT_WDUNF		BIT(0)		/* Watchdog Underflow */
>  #define  AT91_WDT_WDERR		BIT(1)		/* Watchdog Error */
>  
> -/* Watchdog Timer Value Register */
> -#define AT91_SAM9X60_VR		0x08
> +#define  AT91_WDT_SET_WDV(x)	((x) & AT91_WDT_WDV)
> +#define  AT91_WDT_SET_WDD(x)	(((x) << 16) & AT91_WDT_WDD)
>  
> -/* Watchdog Window Level Register */
> -#define AT91_SAM9X60_WLR	0x0c
> -/* Watchdog Period Value */
> -#define  AT91_SAM9X60_COUNTER	(0xfffUL << 0)
> -#define  AT91_SAM9X60_SET_COUNTER(x)	((x) & AT91_SAM9X60_COUNTER)
> +#define AT91_WDT_VR		0x08	/* Watchdog Timer Value Register */
> +#define AT91_WDT_ISR		0x1c	/* Interrupt Status Register */
> +#define AT91_WDT_IER		0x14	/* Interrupt Enable Register */
> +#define AT91_WDT_IDR		0x18	/* Interrupt Disable Register */
> +#define AT91_WDT_WLR		0x0c	/* Watchdog Window Level Register */
> +#define AT91_WDT_PERIODRST	BIT(4)	/* Period Reset */
> +#define AT91_WDT_RPTHRST	BIT(5)		/* Minimum Restart Period */
> +#define  AT91_WDT_PERINT	BIT(0)	/* Period Interrupt Enable */
> +#define  AT91_WDT_COUNTER	(0xfffUL << 0)	/* Watchdog Period Value */
> +#define  AT91_WDT_SET_COUNTER(x)	((x) & AT91_WDT_COUNTER)
>  
> -/* Interrupt Enable Register */
> -#define AT91_SAM9X60_IER	0x14
> -/* Period Interrupt Enable */
> -#define  AT91_SAM9X60_PERINT	BIT(0)
> -/* Interrupt Disable Register */
> -#define AT91_SAM9X60_IDR	0x18
> -/* Interrupt Status Register */
> -#define AT91_SAM9X60_ISR	0x1c
> +/*
> + * WDDIS bit differs by SoC:
> + *   - SAMA5, AT91SAM9261: bit 15
> + *   - SAM9X60, SAMA7G5, SAM9X75: bit 12
> + * Select at runtime via compatible string.
> + */
> +#define AT91_WDT_WDDIS_LEGACY   BIT(15)
> +#define AT91_WDT_WDDIS_MODERN   BIT(12)

This is bad naming, we are going to end up with
AT91_WDT_WDDIS_LEGACY_LEGACY, AT91_WDT_WDDIS_MODERN_LEGACY and
AT91_WDT_WDDIS_MODERN next time. The proper name would use the name of
the SoC introducing the new position.

>  
>  #endif
> -- 
> 2.34.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com