On Wed, 24 Apr 2024 at 08:09, Sui Jingfeng <sui.jingfeng@xxxxxxxxx> 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
