On Sun, Jul 28, 2024 at 05:38:37AM GMT, Sui Jingfeng wrote: > Hi, > > On 7/28/24 04:28, Dmitry Baryshkov wrote: > > On Sun, Jul 28, 2024 at 03:10:21AM GMT, Sui Jingfeng wrote: > > > Hi, > > > > > > On 7/28/24 00:39, Dmitry Baryshkov wrote: > > > > > Hi, > > > > > > > > > > This patch series aims to add Freescale i.MX8qxp Display Controller support. > > > > > > > > > > The controller is comprised of three main components that include a blit > > > > > engine for 2D graphics accelerations, display controller for display output > > > > > processing, as well as a command sequencer. > > > > > > > > > > Previous patch series attempts to do that can be found at: > > > > > https://patchwork.freedesktop.org/series/84524/ > > > > > > > > > > This series addresses Maxime's comments on the previous one: > > > > > a. Split the display controller into multiple internal devices. > > > > > 1) List display engine, pixel engine, interrupt controller and more as the > > > > > controller's child devices. > > > > > 2) List display engine and pixel engine's processing units as their child > > > > > devices. > > > > > > > > > > b. Add minimal feature support. > > > > > Only support two display pipelines with primary planes with XR24 fb, > > > > > backed by two fetchunits. No fetchunit dynamic allocation logic(to be done > > > > > when necessary). > > > > > > > > > > c. Use drm_dev_{enter, exit}(). > > > > > > > > > > Since this series changes a lot comparing to the previous one, I choose to > > > > > send it with a new patch series, not a new version. > > > > I'm sorry, I have started reviewing v2 without noticing that there is a > > > > v3 already. > > > > > > > > Let me summarize my comments: > > > > > > > > - You are using OF aliases. Are they documented and acked by DT > > > > maintainers? > > > > > > > > - I generally feel that the use of so many small devices to declare > > > > functional blocks is an abuse of the DT. Please consider creating > > > > _small_ units from the driver code directly rather than going throught > > > > the components. > > > > > > Well, I really don't think so. I don't agree. > > > > > > I have checked the DTSpec[1] before type, the spec isn't define how > > > many is considered to be "many", and the spec isn't define to what > > > extent is think to be "small" as well. > > > > Yeah. However _usually_ we are not defining DT devices for sub-device > > components. > > I guess, this depended on their hardware (i.MX8qxp) layout, reflecting > exactly what their hardware's layout is perfectly valid. It also depend > on if specific part of those sub-device will be re-visioned or not. If > only a small part of the whole is re-versioned in the future, we can still > re-using this same driver with slightly modify(update) the DTS. > > The point is to controll the granularity and forward compatibility. That's why I asked for the comments or explanations. The cover letter is a usual place, as it is an introduction to the series. The reviewers don't start the series from 8/N, they start from 00/NN. > > > So at least such decisions ought to be described and > > explained in the cover letter. > > Agree, but I see 08/19 patch has a beautiful schematic. I have learned > a lot when reading it. I can't see any abuse of the DT through this > bulk series anyway. > > > Comments below are not revelant to Ying's patch series itself. > > /*----------------------------------------------------------------*/ > > By the way, the last time that I have ever seen and feel abuse of the > DT is the aux-bridge.c[1] and aux-hpd-bridge.c[2]. I strongly feel that > those two *small* programs are abuses to the DT and possibily abuse to > the auxiliary bus framework. Off-topic should be directed as an answer to the original series. And if you tried, you'd have seen the reason and the explanation. And a quick glance around would have shown you the ti-sn65dsi86 bridge, which also has similar structure for exactly the same reasons. Nevertheless, the aux-bridge and aux-hpd-bridge provide a way to link two different directions of the probe dependency chains: from the SoC to the connector (DRM bridge chains) and from the connector to the SoC (USB-C chains). > 1) It's so *small* that it don't even have a hardware entity (physical > device) to corresponding with. As far as I can see, all hardware > units in this patch series are bigger than yours. Because your HPD > bridge is basically a "virtual wire". > > An non-physical-exist wire hold reference to several device node, > this is the most awful abuse to the DT I have ever seen. In other > words, despite you want to solve some software problems, but then, > you could put a device not in the DTS, and let the 'OF' system > create a device for you. Just like what this series do. Thank you for your opinion. However you have clearly provided the reason why these devices do not have a separate DT node: they are internal parts of the existing device. > > 2) I feel your HPD fake bridge driver abuse to the philosophy of > auxiliary bus [3]. The document of auxiliary bus tell us that > > "These individual devices split from the core cannot live on > the platform bus as they are not physical devices that are > controlled by DT/ACPI" This is correct and two mentioned bridges follow this approach. This phrase mostly provides a boundary between platform-based MFD devices which usually have MMIO-like addressing for cells and non-MMIO, non-platform devices living on the aux bus. > Which is nearly equivalent to say that devices that are controlled > by DT/ACPI have better ways to enforce the control. When using > auxiliary bus, we *generally* should not messed with DT. See > golden examples[4][5]. At least, their code are able to run on > X86, while the code you write just can't. > > [0] https://patchwork.freedesktop.org/patch/605555/?series=135786&rev=3 > [1] https://elixir.bootlin.com/linux/v6.10.2/source/drivers/gpu/drm/bridge/aux-bridge.c > [2] https://elixir.bootlin.com/linux/v6.10.2/source/drivers/gpu/drm/bridge/aux-hpd-bridge.c > [3] https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html > > [4] https://patchwork.freedesktop.org/series/136431/ > [5] https://patchwork.freedesktop.org/series/134837/ > > > Best regards > Sui > > > > > > > [1] > > > https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.4 > > > > -- > Best regards > Sui > -- With best wishes Dmitry
