Re: [PATCH v3 00/19] Add Freescale i.MX8qxp Display Controller support

[Sui Jingfeng <sui.jingfeng@linux.dev>]

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.

> 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.

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.

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"

     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