Re: [PATCH 2/8] drm/loongson: Introduce a drm bridge driver for it66121 HDMI transmitter

[Dmitry Baryshkov]

On Mon, 30 Oct 2023 at 15:26, Sui Jingfeng <suijingfeng@xxxxxxxxxxx> wrote:
>
> Hi,
>
>
> On 2023/10/30 18:01, Dmitry Baryshkov wrote:
> > On Mon, 30 Oct 2023 at 11:42, Sui Jingfeng <suijingfeng@xxxxxxxxxxx> wrote:
> >> Hi,
> >>
> >>
> >> On 2023/10/30 06:53, Dmitry Baryshkov wrote:
> >>> On Sun, 29 Oct 2023 at 21:46, Sui Jingfeng <suijingfeng@xxxxxxxxxxx> wrote:
> >>>> The IT66121 is a DVO to HDMI converter, LS3A5000+LS7A1000 ML5A_MB use this
> >>>> chip to support HDMI output. Thus add a drm bridge based driver for it.
> >>>> This patch is developed with drivers/gpu/drm/bridge/ite-it66121.c as base.
> >>> Please use the original bridge driver instead of adding a new one.
> >> I'm agree with the spirit of code sharing, but this is nearly impossible for non-DT system.
> >>
> >> Because the original bridge driver(say it66121.ko) is fully dependent on the DT.
> > I can not agree here. It doesn't depend on DT. It has fully populated
> > i2c_device_id structures, so it will work with bare I2C buses.
> > Most likely you will have to change of calls into fwnode calls, that's it.
> >
> >> UEFI+ACPI based system can not use with it.
> >>
> >> Our I2C adapter is created by the drm/loongson.ko on the runtime.
> >> The potential problem is that *cyclic dependency* !
> >>
> >> I2C adapter driver is depend on drm/loongson
> >> drm/loongson depend on drm bridge driver (say it66121.ko)
> >> drm bridge driver (say it66121.ko) depend on I2C adapter to setup.
> >>
> >> This plus the defer probe mechanism is totally a trap,
> >> incurring troubles and don't work.
> > Welcome. We had this kind of issue for DP AUX buses.
> >
> > I can suggest the following approach:
> > - In the root probe function you can create an i2c bus and populate it
> > with the i2c devices.
> > - I have not checked whether you use components or not. If not, please
> > use an auxiliary or a platform device for the main DRM functionality.
> > - In the subdevice probe / bind function you check for the next
> > bridge. Then you get one of the following:drm_bridge pointer,
> > -EPROBE_DEFER, or any other error case. Your driver can react
> > accordingly.
>
> I have similar way to solve this problem, and I have solved it one and a half years ago.
> See [1] for a reference.
>
> [1] https://patchwork.freedesktop.org/patch/478998/?series=99512&rev=11
>
> When the PCI device get probed, we create the I2C bus first.
> This ensure that when drm/lsdc.ko get loaded, the I2C bus is presence
> and ready to be get by the drm_bridge driver.
> This is basically a PCI-to-GPIO-emulated-I2C adapter,
> then wait the display bridges driver get loaded and set up.
>
> I also need to create a virtual platform device for the display controller.
> which allow the drm drivers instance for this virtual platform device
> be able to probed due to defer probe mechanism.
>
> This solution made the framework of my driver distortion severely,

I don't think I could catch this phrase. Did you see distortions on the screen?

> and in the end we still solve a easy problem by workaround.

No workarounds for the kernel subsystems are allowed.

>
> I know how to use the component framework also, but the component framework just
> a wrapper. Similar with above approach, it brings no gains in the end.
> It does not make this driver better. I got trapped one years ago,
> and I don't want to got trapped another time.
> And I know how solve such a problem by workaround, but that's not worthy for the effort.
>
> I think my approach provide a solution, while still keep the bridges drivers
> to a modular at the same time. Despite simple, it indeed solve the problem.
> It simple because of explicit control of the loading order by myself, not by
> rely on the framework or something else (say component)

PCI media drivers have had this issue for ages. And all of them found
a way to work.

> It is not totally duplicating, I have rewrite part of them.

This is even worse. Now one can not apply fixes to the second one.

> You can compare
> to see what I'm changed. It is just that it66162 was upstream-ed earlier than
> our solution. But I also have write display drivers for lt8618 and lt8619
> completely by myself.
>
>
> Even though our local drm bridges driver will not be able to enjoy the updates.
> We will accept such a results(or pain). I can maintain our local drm bridges
> drivers by myself.

What happens if anybody wants to reuse your bridge driver for their
own platform?
Linux kernel uses driver model and frameworks to improve code sharing,
not to reduce it.

> Sorry, on this technique point, we will not follow your idea.
> I'm sure that my approach is toward to right direction for our device at now.
> If someone invent a better solution to handle this problem, which make the
> various drm bridges drivers usable out of box, then I will follow and cooperate
> to test.
>
>
> > Basically duplicating the existing driver code is not really a way to
> > go. Consider somebody adding a new feature or fixing a bug in your
> > driver copy. Then they have to check if the fix applies to the driver
> > at drivers/gpu/drm/bridge/ite-it66121.c. And vice versa. After fixing
> > an issue in the standard driver one has to keep in mind to check your
> > private copy.
> >
> > So, please, use the OF code as an inspiration and register all your
> > devices in the device tree. Yes, this requires some effort from your
> > side. Yes, this pays off in the longer distance.
> >
> >>>    If
> >>> it needs to be changed in any way, please help everyone else by
> >>> improving it instead of introducing new driver.
> >>>
> >>>> Signed-off-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> >>>> ---
> >>>>    drivers/gpu/drm/loongson/Kconfig            |   1 +
> >>>>    drivers/gpu/drm/loongson/Makefile           |   2 +
> >>>>    drivers/gpu/drm/loongson/ite_it66121.c      | 749 ++++++++++++++++++++
> >>>>    drivers/gpu/drm/loongson/ite_it66121.h      |  19 +
> >>>>    drivers/gpu/drm/loongson/ite_it66121_regs.h | 268 +++++++
> >>>>    5 files changed, 1039 insertions(+)
> >>>>    create mode 100644 drivers/gpu/drm/loongson/ite_it66121.c
> >>>>    create mode 100644 drivers/gpu/drm/loongson/ite_it66121.h
> >>>>    create mode 100644 drivers/gpu/drm/loongson/ite_it66121_regs.h
> >
>


-- 
With best wishes
Dmitry