| Message ID | 20240318234222.1278882-1-sui.jingfeng@linux.dev |
|---|---|
| State | Superseded |
| Headers | show |
| Series | software node: Implement device_get_match_data fwnode callback | expand |
+Cc: Vladimir On Tue, Mar 19, 2024 at 07:42:22AM +0800, Sui Jingfeng wrote: > This makes it possible to support (and/or test) a few drivers that > originates from DT World on the x86-64 platform. Originally, those > drivers using the of_device_get_match_data() function to get match > data. For example, drivers/gpu/drm/bridge/simple-bridge.c and > drivers/gpu/drm/bridge/display-connector.c. Those drivers works very > well in the DT world, however, there is no counterpart to > of_device_get_match_data() when porting them to the x86 platform, > because x86 CPUs lack DT support. This is not true. First of all, there is counter part that called device_get_match_data(). Second, there *is* DT support for the _selected_ x86 based platforms. > By replacing it with device_get_match_data() and creating a software > graph that mimics the OF graph, everything else works fine, except that > there isn't an out-of-box replacement for the of_device_get_match_data() > function. Because the software node backend of the fwnode framework lacks > an implementation for the device_get_match_data callback. .device_get_match_data > Implement device_get_match_data fwnode callback fwnode callback to fill .device_get_match_data > this gap. Device drivers or platform setup codes are expected to provide > a "compatible" string property. The value of this string property is used > to match against the compatible entries in the of_device_id table. Which > is consistent with the original usage style. Why do you need to implement the graph in the board file? ... Have you seen this discussion? https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/
Hi, On 2024/3/20 18:39, Andy Shevchenko wrote: > +Cc: Vladimir > > On Tue, Mar 19, 2024 at 07:42:22AM +0800, Sui Jingfeng wrote: >> This makes it possible to support (and/or test) a few drivers that >> originates from DT World on the x86-64 platform. Originally, those >> drivers using the of_device_get_match_data() function to get match >> data. For example, drivers/gpu/drm/bridge/simple-bridge.c and >> drivers/gpu/drm/bridge/display-connector.c. Those drivers works very >> well in the DT world, however, there is no counterpart to >> of_device_get_match_data() when porting them to the x86 platform, >> because x86 CPUs lack DT support. > This is not true. > > First of all, there is counter part that called device_get_match_data(). Are you means that the acpi_fwnode_device_get_match_data() implementation? As the fwnode API framework has three backend: OF, ACPI, and software node. If you are hinting me that the acpi backend has the .device_get_match_data implemented. Then you are right. > Second, there *is* DT support for the _selected_ x86 based platforms. Yeah, you maybe right again here. I guess you means that some special hardware or platform may have a *limited* support? Can you pointed it out for study of learning purpose? To speak precisely, there are some drm display bridges drivers are lack of the DT support on X86. Those display bridges belong to the device drivers catalogs. OK, I will update my commit message at the next version if possible, and try my best to describe the problem precisely. >> By replacing it with device_get_match_data() and creating a software >> graph that mimics the OF graph, everything else works fine, except that >> there isn't an out-of-box replacement for the of_device_get_match_data() >> function. Because the software node backend of the fwnode framework lacks >> an implementation for the device_get_match_data callback. > .device_get_match_data > >> Implement device_get_match_data fwnode callback fwnode callback to fill > .device_get_match_data OK, thanks a lot. >> this gap. Device drivers or platform setup codes are expected to provide >> a "compatible" string property. The value of this string property is used >> to match against the compatible entries in the of_device_id table. Which >> is consistent with the original usage style. > Why do you need to implement the graph in the board file? It can be inside the chip, there is no clear cut. I means that the graph(including fwnode graph, OF graph or swnode graph) can be used at anywhere. The examples given here may lead you to think it is board specific, but it is not limited to board specific. fwnode graph, OF graph and swnode graph, all of them are implements of the graph. Its common that different hardware vendors bought the some IP and has been integrated it into their SoC. So it can be inside of the chip if you want *code sharing*. Back to the patch itself, we want to keep the three backends aligned as much as possible. Is this reasonable enough? > ... > > Have you seen this discussion? > https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/ > I really didn't have seen that thread before this patch is sent, I'm a graphic developer, I'm mainly focus on graphics domain. Previously, I have implemented similar functionality at the drivers layer [1][2]. But as the instances grows, I realized there is a risk to introducing *boilerplate*. So I send this patch. [1][2] can be drop if this patch could be merged. [1] https://patchwork.freedesktop.org/patch/575414/?series=129040&rev=1 [2] https://patchwork.freedesktop.org/patch/575411/?series=129040&rev=1 After a brief skim, I guess we encounter similar problems. Oops! In a nutshell, there is a need to *emulation* on X86 platform, to suit the need of device-driver coding style of DT world. Besides, at the swnode backend layer, we should not call fwnode_property_read_string(), instead, we should usethe property_entry_read_string_array() function. Because the fwnode_property_read_string() is belong to upper layer. While backend implementations should call functions from bottom layer only.
Hi, On 2024/3/21 04:28, Andy Shevchenko wrote: >>>> By replacing it with device_get_match_data() and creating a software >>>> graph that mimics the OF graph, everything else works fine, except that >>>> there isn't an out-of-box replacement for the of_device_get_match_data() >>>> function. Because the software node backend of the fwnode framework lacks >>>> an implementation for the device_get_match_data callback. >>> .device_get_match_data >>> >>>> Implement device_get_match_data fwnode callback fwnode callback to fill >>> .device_get_match_data >> OK, thanks a lot. >> >>>> this gap. Device drivers or platform setup codes are expected to provide >>>> a "compatible" string property. The value of this string property is used >>>> to match against the compatible entries in the of_device_id table. Which >>>> is consistent with the original usage style. >>> Why do you need to implement the graph in the board file? >> It can be inside the chip, there is no clear cut.\ > Which chip? Flash memory / ROM or you meant something like FPGA here? > For the latter there is another discussion on how to use DT overlays > in ACPI-enabled environments for the FPGA configurations. There are some hardware resource or software entity is created on the driver runtime. But DT or DT overlays are compiled before device driver get loaded. GPIO-emulated-I2C is just an example, this is kind of driver level knowledge on the runtime. With the GPIO or programmable some hardware IP unit, device driver authors can change the connection relationship at their will at the runtime. While with DT, every thing has to be sure before the compile time. DT overlays can be a alternative solution, but this doesn't conflict with this patch. This patch won't assume how device drives go to use it, and allow device driver creating device instead enumerating by DT. In one word: "flexibility".
Hi, On 2024/3/23 00:14, Andy Shevchenko wrote: > On Fri, Mar 22, 2024 at 05:00:05PM +0800, Sui Jingfeng wrote: >> On 2024/3/21 04:28, Andy Shevchenko wrote: > ... > >>>>>> By replacing it with device_get_match_data() and creating a software >>>>>> graph that mimics the OF graph, everything else works fine, except that >>>>>> there isn't an out-of-box replacement for the of_device_get_match_data() >>>>>> function. Because the software node backend of the fwnode framework lacks >>>>>> an implementation for the device_get_match_data callback. >>>>> .device_get_match_data >>>>> >>>>>> Implement device_get_match_data fwnode callback fwnode callback to fill >>>>> .device_get_match_data >>>> OK, thanks a lot. >>>> >>>>>> this gap. Device drivers or platform setup codes are expected to provide >>>>>> a "compatible" string property. The value of this string property is used >>>>>> to match against the compatible entries in the of_device_id table. Which >>>>>> is consistent with the original usage style. >>>>> Why do you need to implement the graph in the board file? >>>> It can be inside the chip, there is no clear cut.\ >>> Which chip? Flash memory / ROM or you meant something like FPGA here? >>> For the latter there is another discussion on how to use DT overlays >>> in ACPI-enabled environments for the FPGA configurations. >> There are some hardware resource or software entity is created on the >> driver runtime. But DT or DT overlays are compiled before device driver >> get loaded. GPIO-emulated-I2C is just an example, this is kind of driver >> level knowledge on the runtime. With the GPIO or programmable some >> hardware IP unit, device driver authors can change the connection relationship >> at their will at the runtime. While with DT, every thing has to be sure >> before the compile time. >> >> DT overlays can be a alternative solution, but this doesn't conflict with >> this patch. This patch won't assume how device drives go to use it, and >> allow device driver creating device instead enumerating by DT. In one >> word: "flexibility". > Software nodes in general for the device driver / platform quirks. The real problem is that we probably shouldn't make an assumption how does the user is going to use the infrastructure, right? You could say it is *mostly* for quirks or whatever, Like the ./drivers/i2c/busses/i2c-cht-wc.c. But software nodes *can* also be something else. Can we stop restricting its usage by limited understanding or someone personal judgement? A workaround or quirk may be enough for some corner usage. Vladimir is also encounter similar problem, right? > They are not designed for what you are talking about here. I have never hint anything about any real applications, the materials and/or talk given here is just for example purpose. What we are doing here is to keep the three back-ends aligned. > Consider using SSDT / DT overlays instead. > NAK, When developers are doing task 'A' , reviewers ask them to do task 'B'. And when developers doing task 'B', reviewers then recommend that the tool 'C' is a better alternative. ... ... This is not good. As I have read the lengthy thread in link [1] as you pointed to me. The boring coding review is just as the following scheme: 1) Asking details about what they do with software nodes impolitely. 2) Wasting time to talk about irreverent things by brute force. 3) Tell everybody that software nodes are not designed for what you application. 4) Recommending DT overlays or something else. Again, this is non-technical discussion, the time being wasting is not worthwhile. And the judgements being given is irrelevant to the *patch itself*. [1] https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/
Hi, On 2024/3/23 02:05, Andy Shevchenko wrote: > Besides that, the kernel project rule is "we do not add > the dead (unused) code". This rule is good an correct and I admit. But the problem is that it is chicken-and-egg problem, it probably have at least two user now. it's possible that it will gain more users in the future. But if you reject everybody from now, then it is zero.
On Sat, Mar 23, 2024 at 02:12:14AM +0800, Sui Jingfeng wrote: > On 2024/3/23 02:05, Andy Shevchenko wrote: > > Besides that, the kernel project rule is "we do not add > > the dead (unused) code". > > This rule is good an correct and I admit. > > But the problem is that it is chicken-and-egg problem, > it probably have at least two user now. Then show them! Convert in the same series and show that. > it's possible that it will gain more users in the future. > > But if you reject everybody from now, then it is zero. As a no-user patch, yes, I reject this.
Hi, On 2024/3/23 02:16, Andy Shevchenko wrote: > On Sat, Mar 23, 2024 at 02:12:14AM +0800, Sui Jingfeng wrote: >> On 2024/3/23 02:05, Andy Shevchenko wrote: >>> Besides that, the kernel project rule is "we do not add >>> the dead (unused) code". >> This rule is good an correct and I admit. >> >> But the problem is that it is chicken-and-egg problem, >> it probably have at least two user now. > Then show them! Convert in the same series and show that. I believe that Vladimir has show enough to you. I have read that thread, I think Vladimit have explained very well.
On Sat, Mar 23, 2024 at 02:30:08AM +0800, Sui Jingfeng wrote: > On 2024/3/23 02:16, Andy Shevchenko wrote: > > On Sat, Mar 23, 2024 at 02:12:14AM +0800, Sui Jingfeng wrote: > > > On 2024/3/23 02:05, Andy Shevchenko wrote: > > > > Besides that, the kernel project rule is "we do not add > > > > the dead (unused) code". > > > This rule is good an correct and I admit. > > > > > > But the problem is that it is chicken-and-egg problem, > > > it probably have at least two user now. > > Then show them! Convert in the same series and show that. > > I believe that Vladimir has show enough to you. I have read that thread, > I think Vladimit have explained very well. Let's continue there. I replied there just now.
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index 36512fb75a20..3670094592f2 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -8,6 +8,7 @@ #include <linux/device.h> #include <linux/kernel.h> +#include <linux/mod_devicetable.h> #include <linux/property.h> #include <linux/slab.h> @@ -379,6 +380,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) { @@ -665,6 +690,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,
This makes it possible to support (and/or test) a few drivers that originates from DT World on the x86-64 platform. Originally, those drivers using the of_device_get_match_data() function to get match data. For example, drivers/gpu/drm/bridge/simple-bridge.c and drivers/gpu/drm/bridge/display-connector.c. Those drivers works very well in the DT world, however, there is no counterpart to of_device_get_match_data() when porting them to the x86 platform, because x86 CPUs lack DT support. By replacing it with device_get_match_data() and creating a software graph that mimics the OF graph, everything else works fine, except that there isn't an out-of-box replacement for the of_device_get_match_data() function. Because the software node backend of the fwnode framework lacks an implementation for the device_get_match_data callback. Implement device_get_match_data fwnode callback fwnode callback to fill this gap. Device drivers or platform setup codes are expected to provide a "compatible" string property. The value of this string property is used to match against the compatible entries in the of_device_id table. Which is consistent with the original usage style. Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev> --- drivers/base/swnode.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)