[RFC PATCH 0/2] DT match helpers for initcalls and platform devices

[RFC PATCH 0/2] DT match helpers for initcalls and platform devices

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

This series adds a couple of boilerplate helpers to match with DT for
initcalls and platform device creation and probe. The goal here is to 
remove more platform code out of arch/arm and eventually the machine
descriptors.

Rob

Rob Herring (2):
  driver core: introduce module_platform_driver_match_and_probe
  of: add initcall with match boilerplate helpers

 include/linux/of.h              | 12 ++++++++++++
 include/linux/platform_device.h | 23 +++++++++++++++++++++++
 2 files changed, 35 insertions(+)

-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 0/2] DT match helpers for initcalls and platform devices

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

This series adds a couple of boilerplate helpers to match with DT for
initcalls and platform device creation and probe. The goal here is to 
remove more platform code out of arch/arm and eventually the machine
descriptors.

Rob

Rob Herring (2):
  driver core: introduce module_platform_driver_match_and_probe
  of: add initcall with match boilerplate helpers

 include/linux/of.h              | 12 ++++++++++++
 include/linux/platform_device.h | 23 +++++++++++++++++++++++
 2 files changed, 35 insertions(+)

-- 
1.8.1.2

[RFC PATCH 1/2] driver core: introduce module_platform_driver_match_and_probe

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

Introduce a helper to match, create and probe a platform device. This
is for drivers such as cpuidle or cpufreq that typically don't have a
bus device node and need to match on a system-level compatible property.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 include/linux/platform_device.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index ce8e4ff..0b4b1c9 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -12,6 +12,7 @@
 #define _PLATFORM_DEVICE_H_
 
 #include <linux/device.h>
+#include <linux/err.h>
 #include <linux/mod_devicetable.h>
 
 #define PLATFORM_DEVID_NONE	(-1)
@@ -241,6 +242,28 @@ extern struct platform_device *platform_create_bundle(
 	struct resource *res, unsigned int n_res,
 	const void *data, size_t size);
 
+/*
+ * module_platform_driver_match_and_probe() - Helper macro for drivers without
+ * a bus device node and need to match on an arbitrary compatible property.
+ * This eliminates a lot of boilerplate.  Each module may only use this macro
+ * once, and calling it replaces module_init() and module_exit()
+ */
+#define module_platform_driver_match_and_probe(__platform_driver, __platform_probe) \
+static int __init __platform_driver##_init(void) \
+{ \
+	if (of_find_matching_node(NULL, (__platform_driver).driver.of_match_table)) \
+		return PTR_ERR_OR_ZERO(platform_create_bundle(&(__platform_driver), \
+			__platform_probe, NULL, 0, NULL, 0)); \
+	else \
+		return -ENODEV;	\
+} \
+module_init(__platform_driver##_init); \
+static void __exit __platform_driver##_exit(void) \
+{ \
+	platform_driver_unregister(&(__platform_driver)); \
+} \
+module_exit(__platform_driver##_exit);
+
 /* early platform driver interface */
 struct early_platform_driver {
 	const char *class_str;
-- 
1.8.1.2

Re: [RFC PATCH 1/2] driver core: introduce module_platform_driver_match_and_probe

[Grant Likely]

On Wed, 30 Oct 2013 01:12:50 -0500, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> Introduce a helper to match, create and probe a platform device. This
> is for drivers such as cpuidle or cpufreq that typically don't have a
> bus device node and need to match on a system-level compatible property.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Grant Likely <grant.likely@linaro.org>
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>

Oh, ick. Please no. If a platform_device isn't getting created for a
device tree node, then we should be asking why it isn't getting created
and fix the core logic rather than trying to bodge it in the driver init
code.

We should never be creating and registering devices in module init code.
We've spent the last 4 years trying to get away from that.

g.

> ---
>  include/linux/platform_device.h | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index ce8e4ff..0b4b1c9 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -12,6 +12,7 @@
>  #define _PLATFORM_DEVICE_H_
>  
>  #include <linux/device.h>
> +#include <linux/err.h>
>  #include <linux/mod_devicetable.h>
>  
>  #define PLATFORM_DEVID_NONE	(-1)
> @@ -241,6 +242,28 @@ extern struct platform_device *platform_create_bundle(
>  	struct resource *res, unsigned int n_res,
>  	const void *data, size_t size);
>  
> +/*
> + * module_platform_driver_match_and_probe() - Helper macro for drivers without
> + * a bus device node and need to match on an arbitrary compatible property.
> + * This eliminates a lot of boilerplate.  Each module may only use this macro
> + * once, and calling it replaces module_init() and module_exit()
> + */
> +#define module_platform_driver_match_and_probe(__platform_driver, __platform_probe) \
> +static int __init __platform_driver##_init(void) \
> +{ \
> +	if (of_find_matching_node(NULL, (__platform_driver).driver.of_match_table)) \
> +		return PTR_ERR_OR_ZERO(platform_create_bundle(&(__platform_driver), \
> +			__platform_probe, NULL, 0, NULL, 0)); \
> +	else \
> +		return -ENODEV;	\
> +} \
> +module_init(__platform_driver##_init); \
> +static void __exit __platform_driver##_exit(void) \
> +{ \
> +	platform_driver_unregister(&(__platform_driver)); \
> +} \
> +module_exit(__platform_driver##_exit);
> +
>  /* early platform driver interface */
>  struct early_platform_driver {
>  	const char *class_str;
> -- 
> 1.8.1.2
> 

Re: [RFC PATCH 1/2] driver core: introduce module_platform_driver_match_and_probe

[Rob Herring]

On Thu, Nov 21, 2013 at 6:47 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Wed, 30 Oct 2013 01:12:50 -0500, Rob Herring <robherring2@gmail.com> wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> Introduce a helper to match, create and probe a platform device. This
>> is for drivers such as cpuidle or cpufreq that typically don't have a
>> bus device node and need to match on a system-level compatible property.
>>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Grant Likely <grant.likely@linaro.org>
>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>
> Oh, ick. Please no. If a platform_device isn't getting created for a
> device tree node, then we should be asking why it isn't getting created
> and fix the core logic rather than trying to bodge it in the driver init
> code.
>
> We should never be creating and registering devices in module init code.
> We've spent the last 4 years trying to get away from that.

This is for devices that have no DT device node to be associated with
and therefore will never have a device created by the core DT code.
Instead the devices are created based off of the root compatible
property. cpuidle drivers are one such example [1]. We already do this
today by putting the platform device creation in the
machine_desc.init_machine function which is a conditional initcall.
The motivation for changing this is how to support drivers like this
on arm64 which doesn't want any platform code or machine_desc. At
least historically, we didn't want DT nodes of Linux specific devices
in the DT. So, how would you propose to solve this problem?

Rob

[1] https://git.kernel.org/cgit/linux/kernel/git/robh/linux.git/commit/?h=highbank-rm-mach-desc&id=f4c00839748688c480c56952cfc06d49aebe1162

[RFC PATCH 2/2] of: add initcall with match boilerplate helpers

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

Add boilerplate helpers to create initcalls which are conditional on
matching on devicetree properties.

Cc: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 include/linux/of.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index f95aee3..a1327c9 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -592,6 +592,18 @@ static inline int of_property_read_u32(const struct device_node *np,
 		s;						\
 		s = of_prop_next_string(prop, s))
 
+#define of_initcall_match(__func, __lvl, __match) \
+static int __init __func##_init(void) \
+{ \
+	if (of_find_matching_node(NULL, __match)) \
+		return __func(); \
+	else \
+		return -ENODEV;	\
+} \
+__lvl(__func##_init);
+
+#define of_module_init_match(__func, __match) of_initcall_match(__func, module_init, __match)
+
 #if defined(CONFIG_PROC_FS) && defined(CONFIG_PROC_DEVICETREE)
 extern void proc_device_tree_add_node(struct device_node *, struct proc_dir_entry *);
 extern void proc_device_tree_add_prop(struct proc_dir_entry *pde, struct property *prop);
-- 
1.8.1.2

Re: [RFC PATCH 2/2] of: add initcall with match boilerplate helpers

[Grant Likely]

On Wed, 30 Oct 2013 01:12:51 -0500, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> Add boilerplate helpers to create initcalls which are conditional on
> matching on devicetree properties.
> 
> Cc: Grant Likely <grant.likely@linaro.org>
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> ---
>  include/linux/of.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index f95aee3..a1327c9 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -592,6 +592,18 @@ static inline int of_property_read_u32(const struct device_node *np,
>  		s;						\
>  		s = of_prop_next_string(prop, s))
>  
> +#define of_initcall_match(__func, __lvl, __match) \
> +static int __init __func##_init(void) \
> +{ \
> +	if (of_find_matching_node(NULL, __match)) \
> +		return __func(); \
> +	else \
> +		return -ENODEV;	\
> +} \
> +__lvl(__func##_init);
> +
> +#define of_module_init_match(__func, __match) of_initcall_match(__func, module_init, __match)
> +

I'm not sure why this is necessary. I don't particularly have a problem
with it, but I wouldn't normally try to filter out device drivers when
the probe code simply won't get called.

Considering it's paired with the previous patch that creates devices in
the module init call, I'm assuming it is to support that use case. If so
then I don't think it is a good idea. If there is another use case then
maybe.

g.

Re: [RFC PATCH 2/2] of: add initcall with match boilerplate helpers

[Rob Herring]

On Thu, Nov 21, 2013 at 6:50 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Wed, 30 Oct 2013 01:12:51 -0500, Rob Herring <robherring2@gmail.com> wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> Add boilerplate helpers to create initcalls which are conditional on
>> matching on devicetree properties.
>>
>> Cc: Grant Likely <grant.likely@linaro.org>
>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>> ---
>>  include/linux/of.h | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index f95aee3..a1327c9 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -592,6 +592,18 @@ static inline int of_property_read_u32(const struct device_node *np,
>>               s;                                              \
>>               s = of_prop_next_string(prop, s))
>>
>> +#define of_initcall_match(__func, __lvl, __match) \
>> +static int __init __func##_init(void) \
>> +{ \
>> +     if (of_find_matching_node(NULL, __match)) \
>> +             return __func(); \
>> +     else \
>> +             return -ENODEV; \
>> +} \
>> +__lvl(__func##_init);
>> +
>> +#define of_module_init_match(__func, __match) of_initcall_match(__func, module_init, __match)
>> +
>
> I'm not sure why this is necessary. I don't particularly have a problem
> with it, but I wouldn't normally try to filter out device drivers when
> the probe code simply won't get called.
>
> Considering it's paired with the previous patch that creates devices in
> the module init call, I'm assuming it is to support that use case. If so
> then I don't think it is a good idea. If there is another use case then
> maybe.

This is for cases that are not drivers. Again, the motivation is how
do I do initialization on arm64 where there is not already some
conditional initialization hook. My approach to solve this is by
eliminating the machine_desc for highbank. While I don't really have a
need to do this on highbank itself, I do have a need for some of the
same functions present in mach-highbank for arm64.

I realized quickly going down this path that I would have a repeated
pattern and that there's already a few other examples in the kernel of
something like this:

static int __init some_initcall(void)
{
  if (of_machine_is_compatible("my-awesume-board"))
    return -ENODEV;

  /* do some init stuff... */
}

There are some examples using this here [1][2].

Rob

[1] https://git.kernel.org/cgit/linux/kernel/git/robh/linux.git/commit/?h=highbank-rm-mach-desc&id=2920f2082f51128dc28348756a1727fe0d13b5a8
[2] https://git.kernel.org/cgit/linux/kernel/git/robh/linux.git/commit/?h=highbank-rm-mach-desc&id=1acc8c4d023506e22d0abaad84a74a34c36fecfb

Re: [RFC PATCH 0/2] DT match helpers for initcalls and platform devices

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

On Wed, Oct 30, 2013 at 01:12:49AM -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> 
> This series adds a couple of boilerplate helpers to match with DT for
> initcalls and platform device creation and probe. The goal here is to 
> remove more platform code out of arch/arm and eventually the machine
> descriptors.

I fear that this is a step backwards because it makes it easier for
people to do the wrong thing. We've been doing the same with interrupt
controllers and clocks. With those there's at least the argument that
they need to be available really early and therefore cannot use the
regular driver model. But for everything else, shouldn't we enforce
proper drivers to be written?

Perhaps if you can point me to some examples I could get more context
why and where this would be useful.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 0/2] DT match helpers for initcalls and platform devices

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

On Wed, Oct 30, 2013 at 01:12:49AM -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> This series adds a couple of boilerplate helpers to match with DT for
> initcalls and platform device creation and probe. The goal here is to 
> remove more platform code out of arch/arm and eventually the machine
> descriptors.

I fear that this is a step backwards because it makes it easier for
people to do the wrong thing. We've been doing the same with interrupt
controllers and clocks. With those there's at least the argument that
they need to be available really early and therefore cannot use the
regular driver model. But for everything else, shouldn't we enforce
proper drivers to be written?

Perhaps if you can point me to some examples I could get more context
why and where this would be useful.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 0/2] DT match helpers for initcalls and platform devices

[Rob Herring]

On Wed, Oct 30, 2013 at 3:26 AM, Thierry Reding
<thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Oct 30, 2013 at 01:12:49AM -0500, Rob Herring wrote:
>> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>>
>> This series adds a couple of boilerplate helpers to match with DT for
>> initcalls and platform device creation and probe. The goal here is to
>> remove more platform code out of arch/arm and eventually the machine
>> descriptors.
>
> I fear that this is a step backwards because it makes it easier for
> people to do the wrong thing. We've been doing the same with interrupt
> controllers and clocks. With those there's at least the argument that
> they need to be available really early and therefore cannot use the
> regular driver model. But for everything else, shouldn't we enforce
> proper drivers to be written?

You think both are a step backwards or just the initcall one? For
initcalls, there are things which are not drivers, but just one time
init.  The example on highbank is highbank_pm_init. In some cases like
cpuidle, they have been converted to platform drivers, but then we
have platform code to create devices if we are on the relevant
platform. There is not really a real device node that exists for some
of these drivers and we need to create the device rather than the
device getting created by scanning the buses in the DT.

I think this is less error prone because we've had cases of
unconditional initcalls which break multi-platform kernels.

> Perhaps if you can point me to some examples I could get more context
> why and where this would be useful.

I'll push out a WIP branch later which has the above examples and
removes the machine_desc struct for highbank. This all is more for
arm64 which doesn't have or want to have a machine_desc.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/2] DT match helpers for initcalls and platform devices

[Rob Herring]

On Wed, Oct 30, 2013 at 3:26 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Oct 30, 2013 at 01:12:49AM -0500, Rob Herring wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> This series adds a couple of boilerplate helpers to match with DT for
>> initcalls and platform device creation and probe. The goal here is to
>> remove more platform code out of arch/arm and eventually the machine
>> descriptors.
>
> I fear that this is a step backwards because it makes it easier for
> people to do the wrong thing. We've been doing the same with interrupt
> controllers and clocks. With those there's at least the argument that
> they need to be available really early and therefore cannot use the
> regular driver model. But for everything else, shouldn't we enforce
> proper drivers to be written?

You think both are a step backwards or just the initcall one? For
initcalls, there are things which are not drivers, but just one time
init.  The example on highbank is highbank_pm_init. In some cases like
cpuidle, they have been converted to platform drivers, but then we
have platform code to create devices if we are on the relevant
platform. There is not really a real device node that exists for some
of these drivers and we need to create the device rather than the
device getting created by scanning the buses in the DT.

I think this is less error prone because we've had cases of
unconditional initcalls which break multi-platform kernels.

> Perhaps if you can point me to some examples I could get more context
> why and where this would be useful.

I'll push out a WIP branch later which has the above examples and
removes the machine_desc struct for highbank. This all is more for
arm64 which doesn't have or want to have a machine_desc.

Rob

Re: [RFC PATCH 0/2] DT match helpers for initcalls and platform devices

[Rob Herring]

On Wed, Oct 30, 2013 at 3:02 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Oct 30, 2013 at 3:26 AM, Thierry Reding
> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Wed, Oct 30, 2013 at 01:12:49AM -0500, Rob Herring wrote:
>>> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>>>
>>> This series adds a couple of boilerplate helpers to match with DT for
>>> initcalls and platform device creation and probe. The goal here is to
>>> remove more platform code out of arch/arm and eventually the machine
>>> descriptors.
>>
>> I fear that this is a step backwards because it makes it easier for
>> people to do the wrong thing. We've been doing the same with interrupt
>> controllers and clocks. With those there's at least the argument that
>> they need to be available really early and therefore cannot use the
>> regular driver model. But for everything else, shouldn't we enforce
>> proper drivers to be written?
>
> You think both are a step backwards or just the initcall one? For
> initcalls, there are things which are not drivers, but just one time
> init.  The example on highbank is highbank_pm_init. In some cases like
> cpuidle, they have been converted to platform drivers, but then we
> have platform code to create devices if we are on the relevant
> platform. There is not really a real device node that exists for some
> of these drivers and we need to create the device rather than the
> device getting created by scanning the buses in the DT.
>
> I think this is less error prone because we've had cases of
> unconditional initcalls which break multi-platform kernels.
>
>> Perhaps if you can point me to some examples I could get more context
>> why and where this would be useful.
>
> I'll push out a WIP branch later which has the above examples and
> removes the machine_desc struct for highbank. This all is more for
> arm64 which doesn't have or want to have a machine_desc.

Here is a branch with example users:

git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
highbank-rm-mach-desc

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/2] DT match helpers for initcalls and platform devices

[Rob Herring]

On Wed, Oct 30, 2013 at 3:02 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Wed, Oct 30, 2013 at 3:26 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>> On Wed, Oct 30, 2013 at 01:12:49AM -0500, Rob Herring wrote:
>>> From: Rob Herring <rob.herring@calxeda.com>
>>>
>>> This series adds a couple of boilerplate helpers to match with DT for
>>> initcalls and platform device creation and probe. The goal here is to
>>> remove more platform code out of arch/arm and eventually the machine
>>> descriptors.
>>
>> I fear that this is a step backwards because it makes it easier for
>> people to do the wrong thing. We've been doing the same with interrupt
>> controllers and clocks. With those there's at least the argument that
>> they need to be available really early and therefore cannot use the
>> regular driver model. But for everything else, shouldn't we enforce
>> proper drivers to be written?
>
> You think both are a step backwards or just the initcall one? For
> initcalls, there are things which are not drivers, but just one time
> init.  The example on highbank is highbank_pm_init. In some cases like
> cpuidle, they have been converted to platform drivers, but then we
> have platform code to create devices if we are on the relevant
> platform. There is not really a real device node that exists for some
> of these drivers and we need to create the device rather than the
> device getting created by scanning the buses in the DT.
>
> I think this is less error prone because we've had cases of
> unconditional initcalls which break multi-platform kernels.
>
>> Perhaps if you can point me to some examples I could get more context
>> why and where this would be useful.
>
> I'll push out a WIP branch later which has the above examples and
> removes the machine_desc struct for highbank. This all is more for
> arm64 which doesn't have or want to have a machine_desc.

Here is a branch with example users:

git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
highbank-rm-mach-desc

Rob

Re: [RFC PATCH 0/2] DT match helpers for initcalls and platform devices

[Grant Likely]

On Wed, 30 Oct 2013 15:02:40 -0500, Rob Herring <robherring2@gmail.com> wrote:
> On Wed, Oct 30, 2013 at 3:26 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Wed, Oct 30, 2013 at 01:12:49AM -0500, Rob Herring wrote:
> >> From: Rob Herring <rob.herring@calxeda.com>
> >>
> >> This series adds a couple of boilerplate helpers to match with DT for
> >> initcalls and platform device creation and probe. The goal here is to
> >> remove more platform code out of arch/arm and eventually the machine
> >> descriptors.
> >
> > I fear that this is a step backwards because it makes it easier for
> > people to do the wrong thing. We've been doing the same with interrupt
> > controllers and clocks. With those there's at least the argument that
> > they need to be available really early and therefore cannot use the
> > regular driver model. But for everything else, shouldn't we enforce
> > proper drivers to be written?
> 
> You think both are a step backwards or just the initcall one? For
> initcalls, there are things which are not drivers, but just one time
> init.  The example on highbank is highbank_pm_init. In some cases like
> cpuidle, they have been converted to platform drivers, but then we
> have platform code to create devices if we are on the relevant
> platform. There is not really a real device node that exists for some
> of these drivers and we need to create the device rather than the
> device getting created by scanning the buses in the DT.
> 
> I think this is less error prone because we've had cases of
> unconditional initcalls which break multi-platform kernels.

In that situation, it would probably be better to simply do what you
need to do at initcall level and not register a struct device at all.

If the situation is you absolutely need to create a device because the
DT does not provide what you need (and the DT cannot be changed) then I
strongly recommend separating the driver code and the registration code
into separate blocks because it enforces separation between device
enumaration and device driver probing.

g.

Re: [RFC PATCH 1/2] driver core: introduce module_platform_driver_match_and_probe

[Grant Likely]

On Thu, 21 Nov 2013 16:33:13 -0600, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Nov 21, 2013 at 6:47 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, 30 Oct 2013 01:12:50 -0500, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> >>
> >> Introduce a helper to match, create and probe a platform device. This
> >> is for drivers such as cpuidle or cpufreq that typically don't have a
> >> bus device node and need to match on a system-level compatible property.
> >>
> >> Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> >> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> >
> > Oh, ick. Please no. If a platform_device isn't getting created for a
> > device tree node, then we should be asking why it isn't getting created
> > and fix the core logic rather than trying to bodge it in the driver init
> > code.
> >
> > We should never be creating and registering devices in module init code.
> > We've spent the last 4 years trying to get away from that.
> 
> This is for devices that have no DT device node to be associated with
> and therefore will never have a device created by the core DT code.
> Instead the devices are created based off of the root compatible
> property. cpuidle drivers are one such example [1]. We already do this
> today by putting the platform device creation in the
> machine_desc.init_machine function which is a conditional initcall.
> The motivation for changing this is how to support drivers like this
> on arm64 which doesn't want any platform code or machine_desc. At
> least historically, we didn't want DT nodes of Linux specific devices
> in the DT. So, how would you propose to solve this problem?

Honestly? With a machine_desc.  :-)

Actually, I'm totally serious here. That is why machine_desc is still a
good idea. There are always going to be a few per-platform bits that
need to be present, although the hope is the per-platform will be at the
SoC family level instead of the board level.

g.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/2] driver core: introduce module_platform_driver_match_and_probe

[Grant Likely]

On Thu, 21 Nov 2013 16:33:13 -0600, Rob Herring <robherring2@gmail.com> wrote:
> On Thu, Nov 21, 2013 at 6:47 AM, Grant Likely <grant.likely@linaro.org> wrote:
> > On Wed, 30 Oct 2013 01:12:50 -0500, Rob Herring <robherring2@gmail.com> wrote:
> >> From: Rob Herring <rob.herring@calxeda.com>
> >>
> >> Introduce a helper to match, create and probe a platform device. This
> >> is for drivers such as cpuidle or cpufreq that typically don't have a
> >> bus device node and need to match on a system-level compatible property.
> >>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Cc: Grant Likely <grant.likely@linaro.org>
> >> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> >
> > Oh, ick. Please no. If a platform_device isn't getting created for a
> > device tree node, then we should be asking why it isn't getting created
> > and fix the core logic rather than trying to bodge it in the driver init
> > code.
> >
> > We should never be creating and registering devices in module init code.
> > We've spent the last 4 years trying to get away from that.
> 
> This is for devices that have no DT device node to be associated with
> and therefore will never have a device created by the core DT code.
> Instead the devices are created based off of the root compatible
> property. cpuidle drivers are one such example [1]. We already do this
> today by putting the platform device creation in the
> machine_desc.init_machine function which is a conditional initcall.
> The motivation for changing this is how to support drivers like this
> on arm64 which doesn't want any platform code or machine_desc. At
> least historically, we didn't want DT nodes of Linux specific devices
> in the DT. So, how would you propose to solve this problem?

Honestly? With a machine_desc.  :-)

Actually, I'm totally serious here. That is why machine_desc is still a
good idea. There are always going to be a few per-platform bits that
need to be present, although the hope is the per-platform will be at the
SoC family level instead of the board level.

g.

Re: [RFC PATCH 2/2] of: add initcall with match boilerplate helpers

[Grant Likely]

On Thu, 21 Nov 2013 17:23:14 -0600, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Nov 21, 2013 at 6:50 AM, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, 30 Oct 2013 01:12:51 -0500, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> >>
> >> Add boilerplate helpers to create initcalls which are conditional on
> >> matching on devicetree properties.
> >>
> >> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> >> ---
> >>  include/linux/of.h | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/include/linux/of.h b/include/linux/of.h
> >> index f95aee3..a1327c9 100644
> >> --- a/include/linux/of.h
> >> +++ b/include/linux/of.h
> >> @@ -592,6 +592,18 @@ static inline int of_property_read_u32(const struct device_node *np,
> >>               s;                                              \
> >>               s = of_prop_next_string(prop, s))
> >>
> >> +#define of_initcall_match(__func, __lvl, __match) \
> >> +static int __init __func##_init(void) \
> >> +{ \
> >> +     if (of_find_matching_node(NULL, __match)) \
> >> +             return __func(); \
> >> +     else \
> >> +             return -ENODEV; \
> >> +} \
> >> +__lvl(__func##_init);
> >> +
> >> +#define of_module_init_match(__func, __match) of_initcall_match(__func, module_init, __match)
> >> +
> >
> > I'm not sure why this is necessary. I don't particularly have a problem
> > with it, but I wouldn't normally try to filter out device drivers when
> > the probe code simply won't get called.
> >
> > Considering it's paired with the previous patch that creates devices in
> > the module init call, I'm assuming it is to support that use case. If so
> > then I don't think it is a good idea. If there is another use case then
> > maybe.
> 
> This is for cases that are not drivers. Again, the motivation is how
> do I do initialization on arm64 where there is not already some
> conditional initialization hook. My approach to solve this is by
> eliminating the machine_desc for highbank. While I don't really have a
> need to do this on highbank itself, I do have a need for some of the
> same functions present in mach-highbank for arm64.
> 
> I realized quickly going down this path that I would have a repeated
> pattern and that there's already a few other examples in the kernel of
> something like this:
> 
> static int __init some_initcall(void)
> {
>   if (of_machine_is_compatible("my-awesume-board"))
>     return -ENODEV;
> 
>   /* do some init stuff... */
> }
> 
> There are some examples using this here [1][2].

It's still kind of a nasty pattern. I'd like to limit the use of this as
much as possible and I'd like to make it as easy as possible to discover
where things are coming from at boot. That's one of the things I like
about machine_desc. It is a common entry point for SoC family specific
behaviour.

As for device drivers, the preferred model is to bind against something
already registered. If it is not present, then it does nothing. To me
there is a difference between making the behavour conditional on
something that was registered vs. an arbitrary value in the device tree.
The triggers for the former are exported in /dev/devices. The latter is
'magic'.  :-)

The problem with "if (of_machine_is_compatible())" initcalls is they are
an exception to the normal flow of things. You have to go searching for
specific strings to find out if they are getting called.

I'm possibly splitting hairs here, but a big part of migration to the
Linux device model is getting rid of modules that both register a device
and then bind to it.

g.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 2/2] of: add initcall with match boilerplate helpers

[Grant Likely]

On Thu, 21 Nov 2013 17:23:14 -0600, Rob Herring <robherring2@gmail.com> wrote:
> On Thu, Nov 21, 2013 at 6:50 AM, Grant Likely <grant.likely@linaro.org> wrote:
> > On Wed, 30 Oct 2013 01:12:51 -0500, Rob Herring <robherring2@gmail.com> wrote:
> >> From: Rob Herring <rob.herring@calxeda.com>
> >>
> >> Add boilerplate helpers to create initcalls which are conditional on
> >> matching on devicetree properties.
> >>
> >> Cc: Grant Likely <grant.likely@linaro.org>
> >> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> >> ---
> >>  include/linux/of.h | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/include/linux/of.h b/include/linux/of.h
> >> index f95aee3..a1327c9 100644
> >> --- a/include/linux/of.h
> >> +++ b/include/linux/of.h
> >> @@ -592,6 +592,18 @@ static inline int of_property_read_u32(const struct device_node *np,
> >>               s;                                              \
> >>               s = of_prop_next_string(prop, s))
> >>
> >> +#define of_initcall_match(__func, __lvl, __match) \
> >> +static int __init __func##_init(void) \
> >> +{ \
> >> +     if (of_find_matching_node(NULL, __match)) \
> >> +             return __func(); \
> >> +     else \
> >> +             return -ENODEV; \
> >> +} \
> >> +__lvl(__func##_init);
> >> +
> >> +#define of_module_init_match(__func, __match) of_initcall_match(__func, module_init, __match)
> >> +
> >
> > I'm not sure why this is necessary. I don't particularly have a problem
> > with it, but I wouldn't normally try to filter out device drivers when
> > the probe code simply won't get called.
> >
> > Considering it's paired with the previous patch that creates devices in
> > the module init call, I'm assuming it is to support that use case. If so
> > then I don't think it is a good idea. If there is another use case then
> > maybe.
> 
> This is for cases that are not drivers. Again, the motivation is how
> do I do initialization on arm64 where there is not already some
> conditional initialization hook. My approach to solve this is by
> eliminating the machine_desc for highbank. While I don't really have a
> need to do this on highbank itself, I do have a need for some of the
> same functions present in mach-highbank for arm64.
> 
> I realized quickly going down this path that I would have a repeated
> pattern and that there's already a few other examples in the kernel of
> something like this:
> 
> static int __init some_initcall(void)
> {
>   if (of_machine_is_compatible("my-awesume-board"))
>     return -ENODEV;
> 
>   /* do some init stuff... */
> }
> 
> There are some examples using this here [1][2].

It's still kind of a nasty pattern. I'd like to limit the use of this as
much as possible and I'd like to make it as easy as possible to discover
where things are coming from at boot. That's one of the things I like
about machine_desc. It is a common entry point for SoC family specific
behaviour.

As for device drivers, the preferred model is to bind against something
already registered. If it is not present, then it does nothing. To me
there is a difference between making the behavour conditional on
something that was registered vs. an arbitrary value in the device tree.
The triggers for the former are exported in /dev/devices. The latter is
'magic'.  :-)

The problem with "if (of_machine_is_compatible())" initcalls is they are
an exception to the normal flow of things. You have to go searching for
specific strings to find out if they are getting called.

I'm possibly splitting hairs here, but a big part of migration to the
Linux device model is getting rid of modules that both register a device
and then bind to it.

g.

Re: [RFC PATCH 2/2] of: add initcall with match boilerplate helpers

[Rob Herring]

On Wed, Nov 27, 2013 at 9:22 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Thu, 21 Nov 2013 17:23:14 -0600, Rob Herring <robherring2@gmail.com> wrote:
>> On Thu, Nov 21, 2013 at 6:50 AM, Grant Likely <grant.likely@linaro.org> wrote:
>> > On Wed, 30 Oct 2013 01:12:51 -0500, Rob Herring <robherring2@gmail.com> wrote:
>> >> From: Rob Herring <rob.herring@calxeda.com>
>> >>
>> >> Add boilerplate helpers to create initcalls which are conditional on
>> >> matching on devicetree properties.

[snip]

>> > I'm not sure why this is necessary. I don't particularly have a problem
>> > with it, but I wouldn't normally try to filter out device drivers when
>> > the probe code simply won't get called.
>> >
>> > Considering it's paired with the previous patch that creates devices in
>> > the module init call, I'm assuming it is to support that use case. If so
>> > then I don't think it is a good idea. If there is another use case then
>> > maybe.
>>
>> This is for cases that are not drivers. Again, the motivation is how
>> do I do initialization on arm64 where there is not already some
>> conditional initialization hook. My approach to solve this is by
>> eliminating the machine_desc for highbank. While I don't really have a
>> need to do this on highbank itself, I do have a need for some of the
>> same functions present in mach-highbank for arm64.
>>
>> I realized quickly going down this path that I would have a repeated
>> pattern and that there's already a few other examples in the kernel of
>> something like this:
>>
>> static int __init some_initcall(void)
>> {
>>   if (of_machine_is_compatible("my-awesume-board"))
>>     return -ENODEV;
>>
>>   /* do some init stuff... */
>> }
>>
>> There are some examples using this here [1][2].
>
> It's still kind of a nasty pattern. I'd like to limit the use of this as
> much as possible and I'd like to make it as easy as possible to discover
> where things are coming from at boot. That's one of the things I like
> about machine_desc. It is a common entry point for SoC family specific
> behaviour.

But that is really no longer true now that all the machine_desc
function ptrs are optional. A given platform may or may not even be
calling some machine_desc functions. So the boot flow is already very
much a function of what is in the DTB rather than what machine is
selected.

> As for device drivers, the preferred model is to bind against something
> already registered. If it is not present, then it does nothing. To me
> there is a difference between making the behavour conditional on
> something that was registered vs. an arbitrary value in the device tree.
> The triggers for the former are exported in /dev/devices. The latter is
> 'magic'.  :-)

Again, this patch is for not device driver stuff.

> The problem with "if (of_machine_is_compatible())" initcalls is they are
> an exception to the normal flow of things. You have to go searching for
> specific strings to find out if they are getting called.

Umm, initcall_debug is your friend here.

A standard boilerplate here would actually help in finding these
cases. If arm64 continues to hold the line of no machine_desc, then
this won't be the exception. Are there currently enough exceptions to
justify factoring out this pattern? Obviously, I think there are and
think they will grow, not shrink.

Since none of the proponents of removing/avoiding machine_desc have
weighed in, we'll never reach any conclusion on this. I'll just
continue to open code this as I need to.

> I'm possibly splitting hairs here, but a big part of migration to the
> Linux device model is getting rid of modules that both register a device
> and then bind to it.

There will always be bits that don't fit into the driver model.

Rob