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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yjx1V1Lx0bAtgsCp@robh.at.kernel.org>
Date:   Thu, 24 Mar 2022 08:42:47 -0500
From:   Rob Herring <robh@...nel.org>
To:     Sui Jingfeng <15330273260@....cn>
Cc:     Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Roland Scheidegger <sroland@...are.com>,
        Zack Rusin <zackr@...are.com>,
        Christian Gmeiner <christian.gmeiner@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Andrey Zhizhikin <andrey.zhizhikin@...ca-geosystems.com>,
        Sam Ravnborg <sam@...nborg.org>,
        "David S . Miller" <davem@...emloft.net>,
        Jiaxun Yang <jiaxun.yang@...goat.com>,
        Lucas Stach <l.stach@...gutronix.de>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Ilia Mirkin <imirkin@...m.mit.edu>,
        Qing Zhang <zhangqing@...ngson.cn>,
        suijingfeng <suijingfeng@...ngson.cn>,
        linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v11 7/7] drm/lsdc: add drm driver for loongson display
 controller

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