Re: [PATCH RFC] timer: of: Create a platform_device before the framework is initialized

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On Tue, Jul 01, 2025 at 03:28:50PM -0700, Will McVicker wrote:
> On 07/01/2025, Arnd Bergmann wrote:
> > On Tue, Jul 1, 2025, at 20:21, William McVicker wrote:
> > > On 07/01/2025, Arnd Bergmann wrote:
> > >> On Tue, Jul 1, 2025, at 01:53, William McVicker wrote:
> > >> >> @@ -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.
> > >
> > > Probing with TIMER_OF_DECLARE() just consists of running the match table's data
> > > function pointer. So that is covered by Daniel's patch AFAICT. However, it's
> > > not clear if this implementation allows you to load the kernel module after the
> > > device boots? For example, will the Exynos MCT timer probe if I load the
> > > exynos_mct driver after the device boots? My guess is you'd need to register
> > > the device as a platform device with a dedicated probe function to handle that.
> > 
> > Yes, that's what I meant: the loadable module needs a module_init()
> > function that registers the actual platform driver in order for the
> > probe function to be called. module_platform_driver_probe()
> > is the way I would suggest to arrange it, though that is just a
> > convenience helper around the registration.
> > 
> > The platform device at that point is created by the normal DT scan,
> > so there is no need to create an extra one. On the contrary, in case
> > we successfully call the probe function from timer_init(), we end
> > up with two separate 'struct platform_device' instances 
> > 
> >      Arnd
> 
> So then does it even make sense to have `timer_pdev_of_probe()` if it's very
> unlikely that the module will even be loaded by the time `timer_probe()` runs?
> Would it make sense for TIMER_OF_DECLARE_PDEV() to just have a special else case
> with the module boiler plate stuff for when the driver is built as a module?
> Something like:
> 
> --->o---
> 
> #if !defined(MODULE)
> #define TIMER_OF_DECLARE_PDEV(...) TIMER_OF_DECLARE(...)
> #else
> static int timer_pdev_probe(struct platform_device *pdev)
> {
> 	struct device *dev = &pdev->dev;
> 	int (*timer_init)(struct device_node *np);
> 
> 	timer_init = of_device_get_match_data(dev);
> 	if (!timer_init)
> 		return -EINVAL;
> 
> 	return timer_init(dev->of_node);
> }
> 
> #define TIMER_OF_DECLARE_PDEV(...) \
> 	OF_DECLARE_PDEV(timer_pdev, name, compat, fn) \ # make this define MODULE_DEVICE_TABLE() as well
> 	<create struct platform_driver instance> \
> 	<call module_platform_driver_probe(..., timer_pdev_probe)
> #endif
> 
> --->o---

Yes, that is exactly the goal :)

The RFC was just for the timer-probe function, I was unsure about its
validity. Let me resend with all the changes for the [no ] module
support.

--

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog