Re: [PATCH v3 4/4] clk: microchip: core: allow driver to be compiled with COMPILE_TEST

[claudiu beznea <claudiu.beznea@tuxon.dev>]

Hi, Brian,

Sorry for the delay, I was off for a while.

On 12/19/25 21:32, Brian Masney wrote:
> Hi Claudiu,
> 
> Sorry about the delay writing back. I was at Linux Plumbers in Tokyo.
> 
> On Sat, Dec 06, 2025 at 04:28:31PM +0200, Claudiu Beznea wrote:
>> On 12/5/25 21:46, Brian Masney wrote:
>>> This driver currently only supports builds against a PIC32 target. To
>>> avoid future breakage in the future, let's update the Kconfig and the
>>> driver so that it can be built with CONFIG_COMPILE_TEST enabled.
>>>
>>> Note that with the existing asm calls is not how I'd want to do this
>>> today if this was a new driver, however I don't have access to this
>>> hardware. To avoid any breakage, let's keep the existing behavior.
>>>
>>> Signed-off-by: Brian Masney <bmasney@redhat.com>
>>> ---
>>>   drivers/clk/microchip/Kconfig    |  2 +-
>>>   drivers/clk/microchip/clk-core.c | 32 +++++++++++++++++++++++---------
>>>   2 files changed, 24 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/clk/microchip/Kconfig b/drivers/clk/microchip/Kconfig
>>> index 1b9e43eb54976b219a0277cc971f353fd6af226a..1e56a057319d97e20440fe4e107d26fa85c95ab1 100644
>>> --- a/drivers/clk/microchip/Kconfig
>>> +++ b/drivers/clk/microchip/Kconfig
>>> @@ -1,7 +1,7 @@
>>>   # SPDX-License-Identifier: GPL-2.0
>>>   
>>>   config COMMON_CLK_PIC32
>>> -	def_bool COMMON_CLK && MACH_PIC32
>>> +	def_bool (COMMON_CLK && MACH_PIC32) || COMPILE_TEST
>>>   
>>>   config MCHP_CLK_MPFS
>>>   	bool "Clk driver for PolarFire SoC"
>>> diff --git a/drivers/clk/microchip/clk-core.c b/drivers/clk/microchip/clk-core.c
>>> index f467d7bc28c87a50fb18dc527574f973c4b7e615..fad4b45d908310ffb59e4ed57c55ae4266253444 100644
>>> --- a/drivers/clk/microchip/clk-core.c
>>> +++ b/drivers/clk/microchip/clk-core.c
>>> @@ -9,7 +9,15 @@
>>>   #include <linux/interrupt.h>
>>>   #include <linux/io.h>
>>>   #include <linux/iopoll.h>
>>> +
>>> +#if !defined(CONFIG_MACH_PIC32) && defined(CONFIG_COMPILE_TEST)
>>
>> Can't we have something like:
>>
>> #if defined(CONFIG_MATCH_PIC32)
>> #include <asm/match-pic32/pic32.h>
>> #else
>> #define PIC32_CLR(_reg)		((_reg) + 0x04)
>> #define PIC32_SET(_reg)		((_reg) + 0x08)
>> #define PIC32_INV(_reg)		((_reg) + 0x0C)
>> #define pic32_syskey_unlock()
>> #endif
> 
> Yes, I agree that would be simpler.
> 
>>> +#define PIC32_CLR(_reg)		((_reg) + 0x04)
>>> +#define PIC32_SET(_reg)		((_reg) + 0x08)
>>> +#define PIC32_INV(_reg)		((_reg) + 0x0C)
>>> +#define pic32_syskey_unlock()
>>
>> On the other side, there are other drivers using these defines, maybe a
>> unified approach would fit better? Maybe moving these to
>> include/linux/platform_data ?
> 
> I agree that would be the better approach. Specifically:
> 
> - Move arch/mips/include/asm/mach-pic32/pic32.h to
>    include/linux/platform_data
> - Drop the unused include linux/io.h in pic32.h
> - Check for CONFIG_MATCH_PIC32 for the pic32_syskey_unlock define. Make
>    it a noop for all other architectures.
> 
> That would allow us to have the following in the drivers, with no #if's:
> 
> #include <linux/platform_data/pic32.h>
> 
> I initially wanted to go this route, and I feel that's the best
> technical decision, however my only hesitation is that this is going to
> touch at least 8 different subsystems. I could probably get the MIPS
> folks to take all of this, however it's going to be a pain to collect
> all of the ACKs from the different subsystems.
> 
> x1:~/src/linux/linus (master %)$ git grep pic32.h
> arch/mips/pic32/common/reset.c:#include <asm/mach-pic32/pic32.h>
> arch/mips/pic32/common/reset.c:static void pic32_halt(void)
> arch/mips/pic32/common/reset.c: pic32_halt();
> arch/mips/pic32/common/reset.c: pic32_halt();
> arch/mips/pic32/pic32mzda/config.c:#include <asm/mach-pic32/pic32.h>
> arch/mips/pic32/pic32mzda/early_clk.c:#include <asm/mach-pic32/pic32.h>
> arch/mips/pic32/pic32mzda/early_console.c:#include <asm/mach-pic32/pic32.h>
> arch/mips/pic32/pic32mzda/init.c:#include <linux/platform_data/sdhci-pic32.h>
> drivers/clk/microchip/clk-core.c:#include <asm/mach-pic32/pic32.h>
> drivers/irqchip/irq-pic32-evic.c:#include <asm/mach-pic32/pic32.h>
> drivers/mmc/host/sdhci-pic32.c:#include <linux/platform_data/sdhci-pic32.h>
> drivers/pinctrl/pinctrl-pic32.c:#include <asm/mach-pic32/pic32.h>
> drivers/pinctrl/pinctrl-pic32.c:#include "pinctrl-pic32.h"
> drivers/rtc/rtc-pic32.c:#include <asm/mach-pic32/pic32.h>
> drivers/tty/serial/pic32_uart.c:#include <asm/mach-pic32/pic32.h>
> drivers/watchdog/pic32-dmt.c:#include <asm/mach-pic32/pic32.h>
> drivers/watchdog/pic32-wdt.c:#include <asm/mach-pic32/pic32.h>
> 
> Thoughts?

Sounds good to me.

Thank you,
Claudiu