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

[Dmitry Baryshkov]

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.

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