| Message ID | 20240422164658.217037-1-sui.jingfeng@linux.dev |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] software node: Implement device_get_match_data fwnode callback | expand |
Hi, 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: >> Because the software node backend of the fwnode API framework lacks an >> implementation for the .device_get_match_data function callback. This >> makes it difficult to use(and/or test) a few drivers that originates > Missing space before opening parenthesis. OK, will be fixed at the next version. >> from DT world on the non-DT platform. >> >> 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. > Yep and again, how is this related? If you want to test a driver originating > from DT, you would probably want to have a DT (overlay) to be provided. There are a few reasons, please fixed me if I'm wrong. 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? I means that Linux kernel is intended to be used by both in-tree drivers and out-of-tree drivers. 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. >> This also helps to keep the three backends of the fwnode API aligned as >> 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") > How is it a fix? 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. Hence, before my patch is applied, the two "Make driver OF-independent" patch 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. >> Closes: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/ > Yes, and then Reported-by, which is missing here. > >> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Cc: Daniel Scally <djrscally@gmail.com> >> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> >> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Please, move these after the cutter '---' line (note you may have that line in > your local repo). > > ... > OK, thanks a lot for teaching me. >> +static const void * >> +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); > And if there are more than one compatible provided? Nope, I think this is kind of limitation of the software node, 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. >> + if (ret < 0 || !val) >> + return NULL; >> + while (matches && matches->compatible[0]) { > First part of the conditional is invariant to the loop. Can be simply Right, thanks. > matches = dev->driver->of_match_table; > if (!matches) > return NULL; > > while (...) > >> + if (!strcmp(matches->compatible, val)) >> + return matches->data; >> + >> + matches++; >> + } >> + >> + return NULL; >> +}
Hi, On 2024/4/25 00:34, Dmitry Baryshkov wrote: > On Wed, Apr 24, 2024 at 05:52:03PM +0300, Andy Shevchenko wrote: >> On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote: >>> On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko >>> <andriy.shevchenko@linux.intel.com> wrote: >>>> On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote: >>>>> On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: >>>>>> On 2024/4/23 21:28, Andy Shevchenko wrote: >>>>>>> On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote: >> ... >> >>>>> 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. >>>> I'm not sure I buy this. We have a special helpers based on the bus type to >>>> combine device_get_match_data() with the respective ID table crawling, see >>>> the SPI and I²C cases as the examples. >>> I was thinking that we might be able to deprecate these helpers and >>> always use device_get_match_data(). >> True, but that is orthogonal to swnode match_data support, right? >> There even was (still is?) a patch series to do something like a new >> member to struct device_driver (? don't remember) to achieve that. > Maybe the scenario was not properly described in the commit message, No thecommit message is very clear, just you don't clear. Can't you see that only you are out of scope here and complaining with wrong hypothesis? > or > maybe I missed something. No, you miss and mess everything. > The usecase that I understood from the commit > message was to use instatiated i2c / spi devices, which means > i2c_device_id / spi_device_id. It can also be platform device with manually assigned software node. > The commit message should describe why > the usecase requires using 'compatible' property and swnode. Ideally it > should describe how these devices are instantiated at the first place. > Yes, I admit it, its not the first time you do such a thing. I know you might good at debating and directing blindly. But those skills are not really helpful. As it brings ZERO benefits to the developers and end user of Linux kernel. What the end user need is a good DRM driver and infrastructure like i915, amdgpu, radeon, vc4, ast, X server or wayland etc. Its fine to keep quite if you don't understand something very well, especially, the for the things that not directly maintained by you. As you don't have to responsible for it and you don't need to pay for the obligation. You might deceive yourself to believe that you are reviewing, actually, you are just wasting your time as well as other people's time. If the the commit message is really matters to you, then do you thing about the following series [1][2][3] ? [1] https://patchwork.freedesktop.org/series/123812/ [2] https://patchwork.freedesktop.org/patch/579730/?series=130021&rev=2 [3] https://patchwork.freedesktop.org/series/123737/ How about the witting of its commit message, very well, right? Think before you type, and type with the brain not with the emotion.
On Wed, 24 Apr 2024 at 19:45, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Apr 24, 2024 at 07:34:54PM +0300, Dmitry Baryshkov wrote: > > On Wed, Apr 24, 2024 at 05:52:03PM +0300, Andy Shevchenko wrote: > > > On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote: > > > > On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote: > > > > > > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: > > > > > > > On 2024/4/23 21:28, Andy Shevchenko wrote: > > > > > > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote: > > ... > > > > > > > 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. > > > > > > > > > > I'm not sure I buy this. We have a special helpers based on the bus type to > > > > > combine device_get_match_data() with the respective ID table crawling, see > > > > > the SPI and I²C cases as the examples. > > > > > > > > I was thinking that we might be able to deprecate these helpers and > > > > always use device_get_match_data(). > > > > > > True, but that is orthogonal to swnode match_data support, right? > > > There even was (still is?) a patch series to do something like a new > > > member to struct device_driver (? don't remember) to achieve that. > > > > Maybe the scenario was not properly described in the commit message, or > > maybe I missed something. The usecase that I understood from the commit > > message was to use instatiated i2c / spi devices, which means > > i2c_device_id / spi_device_id. The commit message should describe why > > the usecase requires using 'compatible' property and swnode. Ideally it > > should describe how these devices are instantiated at the first place. > > Yep. I also do not clearly understand the use case and why we need to have > a board file, because the swnodes all are about board files that we must not > use for the new platforms. Not necessarily board files. In some cases it also allows creating device nodes to patch devices, e.g. when the ACPI description is incomplete. But my main concern is about using the "compatible" property here. I suppose that it should be avoided if not absolutely required, instead the driver should use native foo_device_id matching. Here is a list of existing "compatible" properties and their usecases. arch/arm/mach-omap1/board-nokia770.c: PROPERTY_ENTRY_STRING("compatible", "ti,ads7846"), This one looks like a way to overcome shortcomings of the ads7846 driver. The driver should add spi_device_id, use spi_get_device_match_data(), then the property can be dropped. drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c: nodes->i2c_props[0] = PROPERTY_ENTRY_STRING("compatible", "snps,designware-i2c"); drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c: nodes->sfp_props[0] = PROPERTY_ENTRY_STRING("compatible", "sff,sfp"); I think these two can also be dropped if the corresponding drivers get platform_device_id entries. drivers/platform/x86/x86-android-tablets/asus.c: PROPERTY_ENTRY_STRING("compatible", "simple-battery"), drivers/platform/x86/x86-android-tablets/shared-psy-info.c: PROPERTY_ENTRY_STRING("compatible", "simple-battery"), These are required by the power_supply core to identify the "battery stub" case. There is no corresponding device being created and/or matched. drivers/platform/chrome/chromeos_laptop.c: PROPERTY_ENTRY_STRING("compatible", "atmel,maxtouch"), drivers/platform/chrome/chromeos_laptop.c: PROPERTY_ENTRY_STRING("compatible", "atmel,maxtouch"), drivers/platform/chrome/chromeos_laptop.c: PROPERTY_ENTRY_STRING("compatible", "atmel,maxtouch"), drivers/platform/x86/x86-android-tablets/asus.c: PROPERTY_ENTRY_STRING("compatible", "atmel,atmel_mxt_ts"), The driver checks for the presence of the "compatible" property to check that the device has properties at all. I think it was added as a safegap against treating incomplete trackpad nodes (which should have linux,gpio-keymap) as touchscreens (which have none).
On 2024/4/25 00:44, Andy Shevchenko wrote: > On Wed, Apr 24, 2024 at 07:34:54PM +0300, Dmitry Baryshkov wrote: >> On Wed, Apr 24, 2024 at 05:52:03PM +0300, Andy Shevchenko wrote: >>> On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote: >>>> On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko >>>> <andriy.shevchenko@linux.intel.com> wrote: >>>>> On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote: >>>>>> On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: >>>>>>> On 2024/4/23 21:28, Andy Shevchenko wrote: >>>>>>>> On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote: > ... > >>>>>> 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. >>>>> I'm not sure I buy this. We have a special helpers based on the bus type to >>>>> combine device_get_match_data() with the respective ID table crawling, see >>>>> the SPI and I²C cases as the examples. >>>> I was thinking that we might be able to deprecate these helpers and >>>> always use device_get_match_data(). >>> True, but that is orthogonal to swnode match_data support, right? >>> There even was (still is?) a patch series to do something like a new >>> member to struct device_driver (? don't remember) to achieve that. >> Maybe the scenario was not properly described in the commit message, or >> maybe I missed something. The usecase that I understood from the commit >> message was to use instatiated i2c / spi devices, which means >> i2c_device_id / spi_device_id. The commit message should describe why >> the usecase requires using 'compatible' property and swnode. Ideally it >> should describe how these devices are instantiated at the first place. > Yep. I also do not clearly understand the use case and why we need to have > a board file, because the swnodes all are about board files that we must not > use for the new platforms. > Would you like to tell us what's the 'board file'? I am asking because I can not understand those two words at all. I'm really don't know what's the meanings of 'board file'. Do you means that board file is something like the dts, or somethings describe the stuff on the motherboard but outside the CPU? Does the hardware IP core belong to the "board file"? Can we using more concrete vocabulary instead of the vague vocabulary to communicate?
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index eb6eb25b343b..48d18a90b97b 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -14,6 +14,7 @@ #include <linux/init.h> #include <linux/kobject.h> #include <linux/kstrtox.h> +#include <linux/mod_devicetable.h> #include <linux/list.h> #include <linux/property.h> #include <linux/slab.h> @@ -390,6 +391,30 @@ static void software_node_put(struct fwnode_handle *fwnode) kobject_put(&swnode->kobj); } +static const void * +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); + if (ret < 0 || !val) + return NULL; + + while (matches && matches->compatible[0]) { + if (!strcmp(matches->compatible, val)) + return matches->data; + + matches++; + } + + return NULL; +} + static bool software_node_property_present(const struct fwnode_handle *fwnode, const char *propname) { @@ -676,6 +701,7 @@ software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode, static const struct fwnode_operations software_node_ops = { .get = software_node_get, .put = software_node_put, + .device_get_match_data = software_node_get_match_data, .property_present = software_node_property_present, .property_read_int_array = software_node_read_int_array, .property_read_string_array = software_node_read_string_array,
Because the software node backend of the fwnode API framework lacks an implementation for the .device_get_match_data function callback. This makes it difficult to use(and/or test) a few drivers that originates from DT world on the non-DT platform. 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. This also helps to keep the three backends of the fwnode API aligned as 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") Closes: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/ Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Daniel Scally <djrscally@gmail.com> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev> --- V2: Update commit message drivers/base/swnode.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)