| Message ID | 20240422164658.217037-1-sui.jingfeng@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] software node: Implement device_get_match_data fwnode callback | expand |
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. > 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. > 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? > 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). ... > +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? > + if (ret < 0 || !val) > + return NULL; > + while (matches && matches->compatible[0]) { First part of the conditional is invariant to the loop. Can be simply matches = dev->driver->of_match_table; if (!matches) return NULL; while (...) > + if (!strcmp(matches->compatible, val)) > + return matches->data; > + > + matches++; > + } > + > + return NULL; > +}
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; >> +}
On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: > 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? No, this is not fully correct. The drivers anyway have to adopted for 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. > 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. I don't think upstream developers care about the downstream kernels. 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 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. This doesn't constitute a fix. 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" 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. The implementation is still incorrect. The swnode code shouldn't look into the OF data. Please use non-DT match IDs. > > > > > + 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; > > > +} > > -- > Best regards, > Sui >
Hi, On 2024/4/24 05:37, Dmitry Baryshkov wrote: > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: >> 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? > No, this is not fully correct. The drivers anyway have to adopted for > 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. >> 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. > I don't think upstream developers care about the downstream kernels. Theupstream kernels are facing the same problem,by default drm-misc-x86_defconfigdon't has the CONFIG_OF and CONFIG_OF_OVERLAY selected. See [1] for an example. [1] https://cgit.freedesktop.org/drm/drm-tip/tree/drm-misc-x86_defconfig?h=rerere-cache > But let me throw an argument why this patch (or something similar) looks > to be necessary. Agreed till to here. > 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. Yeah, still sounds good. > Currently handling the data coming from such device_ids requires using > special bits of code, It get started to deviate from here, as you are going to rash onto a narrow way. Because you made the wrong assumption, it can be platform devices, it can *also* be of platform device created by the of_platform_device_create(). The patch itself won't put strong restrictions about its users. > e.g. platform_get_device_id(pdev)->driver_data to > get the data from the platform_device_id. Right, but you run into a narrow area and stuck yourself. The so called non-DT, non-ACPI platform devices are all you basis of you argument, right? There have plenty i2c device and SPI device associated with software note properties. After applied this patch, it means that device_get_match_data() can also works for those device. And the approach you provide already generate a lot of *boilerplate*... > Having such codepaths goes > against the goal of unifying DT and non-DT paths via generic property / > fwnode code. Who's goal? your goal or community's goal? is it documented somewhere? Andy's goal is just to make those two drivers truely DT independent, and I agree with Andy. I'm going to cooperate with Andy to achieve this small goal. However, apparently, our goal is *different* with your goal, your goal is a big goal. If you have such a ambitious goal, you can definitely do something on behalf of yourself. For example, improving DT overlay support for the FPGA device, Or making the of_device_id stuff truly platform neutral before telling people that "XXXX doesn't depend on DT". I guess task of removing the of_node member from the struct device already in you job list, as you want to unify the DT and non-DT code paths... All I want is just be able to contribute, do something useful and do the right thing. So please don't throw your personal goal or taste onto the body of other people. Thanks. > As such, I support Sui's idea OK so far. But, > of being able to use device_get_match_data > for non-DT, non-ACPI platform devices. Please *stop* the making biased assumptions! Please stop the making *biased* assumptions! Please stop the making biased *assumptions*! Currently, the various display drivers don't have the acpi_device_id associated. This means that those drivers won't probed even in ACPI enabled systems either. Adding acpi_device_id to those drivers is another topic. If you have that ambitious, you can take the job. But this again is another problem. Back to the concern itself, I didn't mention what device or what drivers will be benefits in my commit message. In fact, after applied this patch, device_get_match_data() will works for the i2c device and SPI device associated with software note. Hence, "non-DT, non-ACPI platform devices" are just an imaginary of yourself. So please stop bring you own confusion to us. > Sui, if that fits your purpose, That doesn't fits my purpose, please stop the recommendation, thanks. > please make sure that with your patch > (or the next iteration of it) you can get driver_data from the matched > platform_device_id. No, that's a another problem. The 'platform_get_device_id(pdev)->driver_data' you mentioned is completely off the domain of fwnode API framework. You are completely deviate what we are currently talking about. What we are talking about is something within the fwnode API framework. You can hack the device_get_match_data() function to call platform_get_device_id() as a fallback code path when the fwnode subsystem couldn't return a match data to you. But this is another problem. >> >>>> 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. > This doesn't constitute a fix. No, it does. > It's not that there is a bug that you are > fixing. You are adding new feature ('support for non-DT platforms'). Yes, it's a bit of farfetched. But as our goal is to make driver OF-independent, as mentioned in the commit title. when the needed feature is missing, the goal can not be achieved. Fix the missing. >> 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. > The implementation is still incorrect. No, it is correct. > The swnode code shouldn't look > into the OF data. Please use non-DT match IDs. Please stop the misleading, the software_node_get_match_data() is a mimic to (subset of) acpi_fwnode_device_get_match_data(), Software node is kind of complement to ACPI, it's definitely need to follow the code style of ACPI counterpart. The initial implementation choose to take a look at the dev->driver->of_match_table, which is to avoid ugly duplication. This introducing no *boilerplate*, and partly reflect what you goal: "Unifying". So, please don't go against with yourself and Please read the implement of acpi_fwnode_device_get_match_data() before objects, thanks. >> >>>> + 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; >>>> +} >> -- >> Best regards, >> Sui >>
On Wed, 24 Apr 2024 at 08:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > Hi, > > > On 2024/4/24 05:37, Dmitry Baryshkov wrote: > > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: > >> 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? > > No, this is not fully correct. The drivers anyway have to adopted for > > 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. > >> 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. > > I don't think upstream developers care about the downstream kernels. > > > Theupstream kernels are facing the same problem,by default drm-misc-x86_defconfigdon't has the CONFIG_OF and CONFIG_OF_OVERLAY selected. > See [1] for an example. > > [1] https://cgit.freedesktop.org/drm/drm-tip/tree/drm-misc-x86_defconfig?h=rerere-cache > > > > But let me throw an argument why this patch (or something similar) looks > > to be necessary. > > Agreed till to here. > > > > 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. > > > Yeah, still sounds good. > > > > Currently handling the data coming from such device_ids requires using > > special bits of code, > > > It get started to deviate from here, as you are going to rash onto a narrow way. > Because you made the wrong assumption, it can be platform devices, it can *also* > be of platform device created by the of_platform_device_create(). The patch itself > won't put strong restrictions about its users. Devices created via of_platform_device_create() have associated device_node, so they won't have such an issue. > > > > e.g. platform_get_device_id(pdev)->driver_data to > > get the data from the platform_device_id. > > Right, but you run into a narrow area and stuck yourself. > The so called non-DT, non-ACPI platform devices are all you basis of you argument, right? > > There have plenty i2c device and SPI device associated with software note properties. > After applied this patch, it means that device_get_match_data() can also works for > those device. > > And the approach you provide already generate a lot of *boilerplate*... Ok, so here you are making an assumption that mentioned i2c and spi devices should use the same match data for OF and non-OF cases. This is not correct. These devices are matched against i2c_device_id and spi_device_id. These structures have their own driver_data fields. It doesn't seem logical to return match data from the structure that wasn't used for matching and ignore the data from the device_id that actually matched the device. So yes, a proper solution from my POV requires teaching subsystems to populate data in a generic way that later can be used by device_get_match_data(). This way we can also deprecate i2c_get_match_data() and spi_get_match_data() and always use device_get_match_data() instead. > > > Having such codepaths goes > > against the goal of unifying DT and non-DT paths via generic property / > > fwnode code. > > > Who's goal? your goal or community's goal? is it documented somewhere? > > Andy's goal is just to make those two drivers truely DT independent, > and I agree with Andy. I'm going to cooperate with Andy to achieve this > small goal. > > However, apparently, our goal is *different* with your goal, your goal > is a big goal. If you have such a ambitious goal, you can definitely do > something on behalf of yourself. > > For example, improving DT overlay support for the FPGA device, Or making > the of_device_id stuff truly platform neutral before telling people that > "XXXX doesn't depend on DT". I guess task of removing the of_node member > from the struct device already in you job list, as you want to unify > the DT and non-DT code paths... > > All I want is just be able to contribute, do something useful and do the > right thing. So please don't throw your personal goal or taste onto the > body of other people. Thanks. I'm not throwing my goals onto anybody. But using taste is actually a part of reviewing the patches. Surely you can disagree here. > > As such, I support Sui's idea > > > OK so far. But, > > > > of being able to use device_get_match_data > > for non-DT, non-ACPI platform devices. > > Please *stop* the making biased assumptions! > Please stop the making *biased* assumptions! > Please stop the making biased *assumptions*! > > > Currently, the various display drivers don't have the acpi_device_id associated. > This means that those drivers won't probed even in ACPI enabled systems either. > Adding acpi_device_id to those drivers is another topic. If you have that ambitious, > you can take the job. But this again is another problem. acpi_device_id is required if those devices are matched against ACPI nodes. > Back to the concern itself, I didn't mention what device or what drivers will > be benefits in my commit message. In fact, after applied this patch, > device_get_match_data() will works for the i2c device and SPI device associated > with software note. Hence, "non-DT, non-ACPI platform devices" are just an imaginary > of yourself. So please stop bring you own confusion to us. Ok, excuse me here. > > > Sui, if that fits your purpose, > > > That doesn't fits my purpose, please stop the recommendation, thanks. > > > > please make sure that with your patch > > (or the next iteration of it) you can get driver_data from the matched > > platform_device_id. > > > No, that's a another problem. > > The 'platform_get_device_id(pdev)->driver_data' you mentioned is completely > off the domain of fwnode API framework. You are completely deviate what we > are currently talking about. > > What we are talking about is something within the fwnode API framework. > > You can hack the device_get_match_data() function to call platform_get_device_id() > as a fallback code path when the fwnode subsystem couldn't return a match data to > you. But this is another problem. No. I was using this as a pointer for having non-DT driver data. As I wrote several paragraphs above, other subsystems use their own driver-specific match structures. Reworking subsystems one-by-one to be able to use generic codepath sounds to me like a way to go. Adding "compatible" property doesn't. > >>>> 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. > > This doesn't constitute a fix. > > > No, it does. > > > It's not that there is a bug that you are > > fixing. You are adding new feature ('support for non-DT platforms'). > > > Yes, it's a bit of farfetched. > > But as our goal is to make driver OF-independent, as mentioned in the commit title. > when the needed feature is missing, the goal can not be achieved. Fix the missing. Ok, what is the _bug_ that is being fixed by this patch? If you check the 'submitting-patches.rst', you'll find this phrase as a description of the Fixes: tag. > >> 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. As far as I can see, repaper correctly handles the case by falling back to the spi_id. So does st7735r.c. -- With best wishes Dmitry
Hi, On 2024/4/24 16:39, Dmitry Baryshkov wrote: > Ok, what is the_bug_ that is being fixed by this patch? I didn't have a way to use that driver under non DT environment, this is the bug. Note: I demanding full features. Both st7735r.c and repaper.c requires additional device property. - For st7735r.c: device_property_read_u32(dev, "rotation", &rotation); - For repaper.c: device_property_read_string(dev, "pervasive,thermal-zone", &thermal_zone) Under non DT environment, those device properties can not be read. Software node backend provided a way. Otherwise, the net results of the patch doesn't meet the description in the commit message. > If you check > the 'submitting-patches.rst', you'll find this phrase as a description > of the Fixes: tag. I have readthe 'submitting-patches.rst', the first sentence tell us that "A Fixes: tag indicates that the patch fixes an issue in a previous commit." So what's the problem? >>>> 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. > As far as I can see, repaper correctly handles the case by falling > back to the spi_id. So does st7735r.c. Yeah, this explicitly is the issue. Falling back to other means is robust design, but it explicitly says that the freshly addeddevice_get_match_data() don't works at all under non DT environment. This is the bug, so what's you concern? According to the commit message, the purpose of the introduction of thedevice_get_match_data() is to achieve OF independent. But as you said, it will fall back, then how does the goal "Make driver OF-independent" can be achieved by the patch itself?
Hi, On 2024/4/24 16:39, Dmitry Baryshkov wrote: >>> Sui, if that fits your purpose, >> That doesn't fits my purpose, please stop the recommendation, thanks. >> >> >>> please make sure that with your patch >>> (or the next iteration of it) you can get driver_data from the matched >>> platform_device_id. >> No, that's a another problem. >> >> The 'platform_get_device_id(pdev)->driver_data' you mentioned is completely >> off the domain of fwnode API framework. You are completely deviate what we >> are currently talking about. >> >> What we are talking about is something within the fwnode API framework. >> >> You can hack the device_get_match_data() function to call platform_get_device_id() >> as a fallback code path when the fwnode subsystem couldn't return a match data to >> you. But this is another problem. > No. I was using this as a pointer for having non-DT driver data. Whatever. Whatever how does it going to be used by you, or whatever data the pointer you use to point to. Just remember one thing, it is not relevant to my patch itself. > As I > wrote several paragraphs above, other subsystems use their own > driver-specific match structures. Fine, but on the DT systems, they mostly probed via DT. Thus, the so-called driver-specific match structures won't be used. > Reworking subsystems one-by-one to > be able to use generic codepath sounds to me like a way to go. Fine, you are encouraged to do whatever you like, you don't have to told me. > Adding > "compatible" property doesn't. As I have told you several times, software node is kind of complement to ACPI, it's definitely need to follow the style of ACPI counterpart. Software node can be secondary node of the primary ACPI device node. With this understood, please read the implementation of acpi_of_match_device() before express objection. Or you can read several relevant commit such as 4222f38ca3b7a ('ACPI / bus: Do not traverse through non-existed device table') for knowing some extra background. Beside, users of the software node backend itself can do whatever they like. The value of the "compatible" property is *just* string, programmers are free to name their string property in their driver. It is not you to say no though. No offensive here, I means that both of us are not good at this domain. Especially me, but at the very least, I'm willing to listen to what expects in ACPI/swnode domain say. One day, you become the top maintainer of specific domain, and when I go to contribute then, I will also read to you reviews message carefully. Back to the technical itself, the "compatible" property is a standard property of ACPI _DSD object. This is written into the ACPI Spec. The value of the "compatible" property is just string, store it at 'platform_get_device_id(pdev)->driver_data' or in 'of_device_id->compatible' or some other places doesn't really changes much in semantic. A device driver can be platform probed, DT probed, ACPI probed, I2C probed, SPI probed... Take the driver of I2C slave display bridge as an example, it can platform probed, DT probed, I2C probed, in the future, there may have programmers want to add acpi_device_id, then, it will gain the 'ACPI probed'. If each of them introduce a driver-specific match structure. Then, that going to introduce very heavy maintain overhead to the maintainers, not to mention to achieve your cheap slogan: "Unifying DT and non-DT paths via generic property / fwnode code".
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.
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().
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.
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.
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.
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?
On Thu, Apr 25, 2024 at 09:42:53PM +0800, Sui Jingfeng wrote: > 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'. Hmm... This is very well established term meaning the hard coded platform description (you may consider that as "device tree" written in C inside the Linux kernel). There are plenty of legacy platforms still exist in the Linux kernel source tree, you may find examples, like (first comes to mind) arch/arm/mach-pxa/spitz.c. > 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? Most of (I though 100% before this message) the Linux kernel developers _know_ this term, sorry that you maybe young enough :-)
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(+)