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

From: Balakrishnan.S

Date: Mon Mar 02 2026 - 02:12:31 EST

Hi,

Thanks for the comments.

On 27/02/26 1:22 pm, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 27/02/2026 13:01:14+0530, Balakrishnan Sambath wrote:
>> From: Andrei Simion <andrei.simion@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxx>
>> [Remove Kconfig-based WDDIS selection and define explicit legacy and
>> modern masks]
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@xxxxxxxxxxxxx>
>> ---
>> 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.

Okay got it. If that's the case I'll retain the soC specific namings
everywhere in v2.

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