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

[claudiu beznea]

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@xxxxxxxxxx>
> > > ---
> > >   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