Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[Maxime Ripard]

On Fri, Feb 04, 2022 at 12:04:19AM +0800, Sui Jingfeng wrote:
> > > +/* Get the simple EDID data from the device tree
> > > + * the length must be EDID_LENGTH, since it is simple.
> > > + *
> > > + * @np: device node contain edid data
> > > + * @edid_data: where the edid data to store to
> > > + */
> > > +static bool lsdc_get_edid_from_dtb(struct device_node *np,
> > > +				   unsigned char *edid_data)
> > > +{
> > > +	int length;
> > > +	const void *prop;
> > > +
> > > +	if (np == NULL)
> > > +		return false;
> > > +
> > > +	prop = of_get_property(np, "edid", &length);
> > > +	if (prop && (length == EDID_LENGTH)) {
> > > +		memcpy(edid_data, prop, EDID_LENGTH);
> > > +		return true;
> > > +	}
> > > +
> > > +	return false;
> > > +}
> >
> > You don't have a device tree binding for that driver, this is something
> > that is required. And it's not clear to me why you'd want EDID in the
> > DTB?
> 
> 1) It is left to the end user of this driver.
> 
> The downstream motherboard maker may use a dpi(XRGB888) or LVDS panel
> which don't have DDC support either, doing this way allow them put a
> EDID property into the dc device node in the DTS. Then the entire system works.
> Note those panel usually support only one display mode.

I guess it depends on what we mean exactly by the user, but the DTB
usually isn't under the (end) user control. And the drm.edid_firmware is
here already to address exactly this issue.

On the other end, if the board has a static panel without any DDC lines,
then just put the timings in the device tree, there's no need for an
EDID blob.

> 2) That is for the display controller in ls2k1000 SoC.
> 
> Currently, the upstream kernel still don't have GPIO, PWM and I2C driver support
> for LS2K1000 SoC.
> 
> How dose you read EDID from the monitor without a I2C driver?
> 
> without reading EDID the device tree support, the screen just black,
> the lsdc driver just stall. With reading EDID from device tree support
> we do not need a i2c driver to light up the monitor.
> 
> This make lsdc drm driver work on various ls2k1000 development board
> before I2C driver and GPIO driver and PWM backlight driver is upstream.
> 
> I have many local private dts with the bindings, those local change just can not
> upstream at this time, below is an example.

The device tree is a platform description language. It's there to let
the OS know what the hardware is, but the state of hardware support in
the said OS isn't a parameter we have to take into account for a new
binding.

If you don't have any DDC support at the moment, use the firmware
mechanism above, or add fixed modes using drm_add_modes_noedid in the
driver, and leave the DT out of it. Once you'll gain support for the
EDID readout in the driver, then it'll just work and you won't need to
change the DT again.

> 3) Again, doing this way is for graphic environment bring up.
> 
> &lsdc {
> 
>     output-ports = <&dvo0 &dvo1>;
>     #address-cells = <1>;
>     #size-cells = <0>;
>     dvo0: dvo@0 {
>         reg = <0>;
> 
>         connector = "dpi-connector";
>         encoder = "none";
>         status = "ok";
> 
>         display-timings {
>             native-mode = <&mode_0_1024x600_60>;
> 
>             mode_0_1024x600_60: panel-timing@0 {
>                 clock-frequency = <51200000>;
>                 hactive = <1024>;
>                 vactive = <600>;
>                 hsync-len = <4>;
>                 hfront-porch = <160>;
>                 hback-porch = <156>;
>                 vfront-porch = <11>;
>                 vback-porch = <23>;
>                 vsync-len = <1>;
>             };
> 
>             mode_1_800x480_60: panel-timing@1 {
>                 clock-frequency = <30066000>;
>                 hactive = <800>;
>                 vactive = <480>;
>                 hfront-porch = <50>;
>                 hback-porch = <70>;
>                 hsync-len = <50>;
>                 vback-porch = <0>;
>                 vfront-porch = <0>;
>                 vsync-len = <50>;
>             };
>         };
>     };
> 
>     dvo1: dvo@1 {
>         reg = <1>;
> 
>         connector = "hdmi-connector";
>         type = "a";
>         encoder = "sil9022";
> 
>         edid = [ 00 ff ff ff ff ff ff 00 1e 6d 54 5b 0b cc 04 00
>              02 1c 01 03 6c 30 1b 78 ea 31 35 a5 55 4e a1 26
>              0c 50 54 a5 4b 00 71 4f 81 80 95 00 b3 00 a9 c0
>              81 00 81 c0 90 40 02 3a 80 18 71 38 2d 40 58 2c
>              45 00 e0 0e 11 00 00 1e 00 00 00 fd 00 38 4b 1e
>              53 0f 00 0a 20 20 20 20 20 20 00 00 00 fc 00 4c
>              47 20 46 55 4c 4c 20 48 44 0a 20 20 00 00 00 ff
>              00 38 30 32 4e 54 43 5a 39 38 33 37 39 0a 00 35 ];
> 
>         status = "ok";
>     };
> };

Yeah, this needs to be documented with a YAML schema

Maxime

Attachment: signature.asc
Description: PGP signature