On Tue, Jul 8, 2025, at 11:50, Daniel Lezcano wrote:

On Tue, Jul 01, 2025 at 09:52:45AM +0200, Arnd Bergmann wrote:

quoted

On Tue, Jul 1, 2025, at 01:53, William McVicker wrote:

quoted

quoted

@@ -1550,6 +1553,8 @@ typedef void (*of_init_fn_1)(struct device_node *);
 		_OF_DECLARE(table, name, compat, fn, of_init_fn_1_ret)
 #define OF_DECLARE_2(table, name, compat, fn) \
 		_OF_DECLARE(table, name, compat, fn, of_init_fn_2)
+#define OF_DECLARE_PDEV(table, name, compat, fn) \
+		_OF_DECLARE(table, name, compat, fn, of_init_fn_pdev)

To support auto-module loading you'll need to also define the
MODULE_DEVICE_TABLE() as part of TIMER_OF_DECLARE_PDEV().

I haven't tested the patch yet, but aside from my comment above it LGTM.

The patch doesn't actually have a module_platform_driver_probe()
yet either, so loading the module wouldn't actually do anything.

I feel that this RFC by itself a good step in the direction we want, 
so Daniel should go ahead with prototyping the next two steps:
adding the platform_driver registration into OF_DECLARE_PDEV,
and converting a driver so it can be used either with the _OF_DECLARE()
or the platform_driver case.

I'm questioning the relevance of adding the macro when the driver is
not compiled as a module.

The first step of this macro is to allow the existing init functions
to be converted to the same signature as the module probe functions in
order to share the same code and take benefit of the devm_ variants
function which will considerably reduce the code size of the drivers.

Then we have the following situations:

 1. The driver has to be loaded very early TIMER_OF_DECLARE_PDEV
 (MODULE=no) the function timer-probe() is used

 2. The driver is a module_platform_driver() and MODULE=no, then it is
 built as a builtin_platform_driver(), we do not care about having it
 loaded by timer-probe()

 3. The driver is a module_platform_driver() and MODULE=yes

If we do the change to have the TIMER_OF_DECLARE_PDEV() adding the
platform_driver registration when MODULE=yes but using timer-probe
when MODULE=no, we change the initialization and we will have issues
with timers needing resources like SCMI clocks and where the
mechanisms rely on EPROBE_DEFER.

IMO, module_platform_driver and timer_probe must be separated.

I assumed that the entire point of your work was to simplify
the code when you have both a early and platform_driver based
probing in a single driver.

Let's assume the one possible future use case with the VF PIT. This
timer is only used on ARM but now it is also supported for the ARM64
s32g2. For the first platform we need it very early and in the second
case, we need it later because the architected timers are there.

We should endup with:

static const struct of_device_id pit_timer_of_match[] = {
	{ .compatible = "nxp,s32g2-pit", .data = &s32g2_data },
	{ .compatible = "nxp,s32g3-pit", .data = &s32g3_data },
	{ }
};
MODULE_DEVICE_TABLE(of, pit_timer_of_match);

static struct platform_driver nxp_pit_driver = {
	.driver = {
		.name = "nxp-pit",
		.of_match_table = pit_timer_of_match,
	},
	.probe = pit_timer_probe,
};
module_platform_driver(nxp_pit_driver);

TIMER_OF_DECLARE_PDEV(vf610, "fsl,vf610-pit", pit_timer_probe);

If you think we still need a module_platform_driver(), I would
not bother adding TIMER_OF_DECLARE_PDEV() either, as the
probe functions can easily be shared as well, the way that
drivers/clocksource/renesas-ostm.c does.

The only thing that would lose is the ability to call devm_
functions because the device pointer is unavailable here.

If we change the TIMER_OF_DECLARE_PDEV to a macro which relies on
timer_probe when MODULE=no, then the "nxp-pit" on the s32gX will fail
to initialize because of the SCMI clocks not ready and the routine
won't reprobe the function. This issue won't happen with
builtin_platform_driver

The way I had expected this to work was that TIMER_OF_DECLARE_PDEV()
always registers the platform_driver and just skips the
section magic when it's in a loadable module.

    Arnd