On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote:
Hi,No, this is not fully correct. The drivers anyway have to adopted for
Thanks a for you reviewing my patch.
On 2024/4/23 21:28, Andy Shevchenko wrote:
On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:OK, will be fixed at the next version.
Because the software node backend of the fwnode API framework lacks anMissing space before opening parenthesis.
implementation for the .device_get_match_data function callback. This
makes it difficult to use(and/or test) a few drivers that originates
There are a few reasons, please fixed me if I'm wrong.from DT world on the non-DT platform.Yep and again, how is this related? If you want to test a driver originating
Implement the .device_get_match_data fwnode callback, device drivers or
platform setup codes are expected to provide a string property, named as
"compatible", the value of this software node string property is used to
match against the compatible entries in the of_device_id table.
from DT, you would probably want to have a DT (overlay) to be provided.
DT (overlay) can be possible solution, but DT (overlay) still depend on DT.
For example, one of my x86 computer with Ubuntu 22.04 Linux/x86 6.5.0-28-generic
kernel configuration do not has the DT enabled. This means that the default kernel
configuration is decided by the downstream OS distribution. It is not decided by
usual programmers. This means that out-of-tree device drivers can never utilize
DT or DT overlay, right?
the platforms they are used with. It is perfectly fine to have a driver
that supports both DT and ACPI at the same time.
I means that Linux kernel is intended to be used by both in-tree drivers and out-of-tree drivers.I don't think upstream developers care about the downstream kernels.
Out-of-tree device drivers don't have a chance to alter kernel config, they can only managed to
get their source code compiled against the Linux kernel the host in-using.
Some out-of-tree device drivers using DKMS to get their source code compiled,
with the kernel configuration already *fixed*. So they don't have a opportunity
to use DT overlay.
Relying on DT overlay is *still* *DT* *dependent*, and I not seeing matured solution
get merged into upstream kernel yet. However, software node has *already* been merged
into Linux kernel. It can be used on both DT systems and non-DT systems. Software node
has the least requirement, it is *handy* for interact with drivers who only need a small
set properties.
In short, I still think my patch maybe useful for some peoples. DT overlay support on
X86 is not matured yet, need some extra work. For out-of-tree kernel module on
downstream kernel. Select DT and DT overlay on X86 is out-of-control. And I don't want
to restrict the freedom of developers.
But let me throw an argument why this patch (or something similar) looks
to be necessary.
Both on DT and non-DT systems the kernel allows using the non-OF based
matching. For the platform devices there is platform_device_id-based
matching.
Currently handling the data coming from such device_ids requires using
special bits of code,
e.g. platform_get_device_id(pdev)->driver_data to
get the data from the platform_device_id.
Having such codepaths goes
against the goal of unifying DT and non-DT paths via generic property /
fwnode code.
As such, I support Sui's idea
of being able to use device_get_match_data
for non-DT, non-ACPI platform devices.
Sui, if that fits your purpose,
please make sure that with your patch
(or the next iteration of it) you can get driver_data from the matched
platform_device_id.
This doesn't constitute a fix.
This also helps to keep the three backends of the fwnode API aligned asHow is it a fix?
much as possible, which is a fundamential step to make device driver
OF-independent truely possible.
Fixes: ffb42e64561e ("drm/tiny/repaper: Make driver OF-independent")
Fixes: 5703d6ae9573 ("drm/tiny/st7735r: Make driver OF-independent")
Because the drm/tiny/repaper driver and drm/tiny/st7735r driver requires extra
device properties. We can not make them OF-independent simply by switching to
device_get_match_data(). As the device_get_match_data() is a *no-op* on non-DT
environment.
It's not that there is a bug that you are
fixing. You are adding new feature ('support for non-DT platforms').
Hence, before my patch is applied, the two "Make driver OF-independent" patchThe implementation is still incorrect.
have no effect. Using device_get_match_data() itself is exactly *same* with
using of_device_get_match_data() as long as the .device_get_match_data hook is
not implemented.
See my analysis below:
When the .device_get_match_data hook is not implemented:
1) On DT systems, device_get_match_data() just redirect to of_fwnode_device_get_match_data(),
which is just a wrapper of of_device_get_match_data().
2) On Non-DT system, device_get_match_data() has *ZERO* effect, it just return NULL.
Therefore, device_get_match_data() adds *ZERO* benefits to the mentioned drivers if
the .device_get_match_data is not implemented.
Only when the .device_get_match_data hook get implemented, device_get_match_data()
can redirect tosoftware_node_get_match_data() function in this patch.
Therefore, the two driver has a way to get a proper driver match data on
non-DT environment. Beside, the users of those two driver can provide
additional software node property at platform setup code. as long as at
somewhere before the driver is probed.
So the two driver really became OF-independent after applied my patch.
OK, thanks a lot for teaching me.Closes: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/Yes, and then Reported-by, which is missing here.
Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>Please, move these after the cutter '---' line (note you may have that line in
Cc: Daniel Scally <djrscally@xxxxxxxxx>
Cc: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
your local repo).
...
Nope, I think this is kind of limitation of the software node,+static const void *And if there are more than one compatible provided?
+software_node_get_match_data(const struct fwnode_handle *fwnode,
+ const struct device *dev)
+{
+ struct swnode *swnode = to_swnode(fwnode);
+ const struct of_device_id *matches = dev->driver->of_match_table;
+ const char *val = NULL;
+ int ret;
+ ret = property_entry_read_string_array(swnode->node->properties,
+ "compatible", &val, 1);
platform setup code generally could provide a compatible property.
No duplicate name is allowed. But we the best explanation would be
platform setup code should provide the "best" or "default" compatible
property.
The swnode code shouldn't look
into the OF data. Please use non-DT match IDs.
--
+ if (ret < 0 || !val)First part of the conditional is invariant to the loop. Can be simply
+ return NULL;
+ while (matches && matches->compatible[0]) {
Right, thanks.
matches = dev->driver->of_match_table;--
if (!matches)
return NULL;
while (...)
+ if (!strcmp(matches->compatible, val))
+ return matches->data;
+
+ matches++;
+ }
+
+ return NULL;
+}
Best regards,
Sui