Re: [PATCH v11 7/7] drm/lsdc: add drm driver for loongson display controller

[Rob Herring <robh@kernel.org>]

On Thu, Mar 24, 2022 at 09:39:49AM +0800, Sui Jingfeng wrote:
> 
> On 2022/3/23 04:49, Rob Herring wrote:
> > On Tue, Mar 22, 2022 at 12:29:16AM +0800, Sui Jingfeng wrote:
> > > From: suijingfeng <suijingfeng@loongson.cn>
> > > 
> > > There is a display controller in loongson's LS2K1000 SoC and LS7A1000
> > > bridge chip, the display controller is a PCI device in those chips. It
> > > has two display pipes but with only one hardware cursor. Each way has
> > > a DVO interface which provide RGB888 signals, vertical & horizontal
> > > synchronisations, data enable and the pixel clock. Each CRTC is able to
> > > scanout from 1920x1080 resolution at 60Hz, the maxmium resolution is
> > > 2048x2048 according to the hardware spec. Loongson display controllers
> > > are simple which require scanout buffers to be physically contiguous.

[...]

> > > +			val |= mask;
> > > +		else
> > > +			val &= ~mask;
> > > +		writeb(val, li2c->dat_reg);
> > Shouldn't you set the data register low first and then change the
> > direction? Otherwise, you may be driving high for a moment. However, if
> > high is always done by setting the direction as input, why write the
> > data register each time? I'm assuming whatever is written to the dat_reg
> > is maintained regardless of pin state.
> > 
> When the pin is input, i am not sure value written to it will be preserved.
> 
> I'm worry about it get flushed by the external input value.
> 
> Because the output data register is same with the input data register(
> offset is  0x1650).
> 
> The hardware designer do not provided a  separation.

Usually for GPIO data registers the read value is current pin state 
regardless of direction and the written value is what to drive as an 
output. But your h/w could be different.


> > > +
> > > +		/* Optional properties which made the driver more flexible */
> > > +		of_property_read_u32(i2c_np, "udelay", &udelay);
> > > +		of_property_read_u32(i2c_np, "timeout", &timeout);
> > These aren't documented. Do you really need them in DT?
> 
> Yes, in very rare case:
> 
> When debugging, sometimes one way I2C works, another way I2C not on specific
> board.

This is not specific to you, so why do you solve it in a way that only 
works for you? If you want to add tuning parameters to the i2c bit 
algorithm, why don't you do so in a way that works for all users? I'm 
sure the I2C maintainer and others have some opinion on this, but 
they'll never see it hidden away in some display driver.


> and you want to see what will happen if you change it from 5 to 2.
> 
> modify device tree is enough, have to recompile the kernel and driver
> modules every time.

Modifying the DT is not the easiest way to debug either.


> It is optional through.

Lots of properties are optional, what's your point?


> Please do not ask me to document such a easy thing,

Everything must be documented. There's nothing more to discuss.


> DT itself is a documention, human readable,  it already speak for itself.

It is machine readable too. Undocumented properties generate warnings 
now.

Rob