| Message ID | 20220318184755.113152-1-marex@denx.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: bridge: icn6211: Fix hard-coded panel settings and add I2C support | expand |
Hey Marek, On Fri, 18 Mar 2022 at 19:48, Marek Vasut <marex@denx.de> wrote: > > This series fixes multiple problems with the ICN6211 driver and adds > support for configuration of the chip via I2C bus. > > First, in the current state, the ICN6211 driver hard-codes DPI timing > and clock settings specific to some unknown panel. The settings provided > by panel driver are ignored. Using any other panel than the one for which > this driver is currently hard-coded can lead to permanent damage of the > panel (per display supplier warning, and it sure did in my case. The > damage looks like multiple rows of dead pixels at the bottom of the > panel, and this is not going away even after long power off time). > > Much of this series thus fixes incorrect register layout, DPI timing > programming, clock generation by adding actual PLL configuration code. > This series no longer adds lane count decoding and retains current > hard-coded lane count 4 due to disagreement over lane count parsing > from DT. The lane count support will come later. The series also fills > in a couple of registers with likely correct default values. > > Second, this series adds support for I2C configuration of the ICN6211. > The device can be configured either via DSI command mode or via I2C, > the register layout is the same in both cases. > > Since the datasheet for this device is very hard to come by, a lot of > information has been salvaged from [1] and [2]. > > [1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c > [2] https://github.com/tdjastrzebski/ICN6211-Configurator > > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Maxime Ripard <maxime@cerno.tech> > Cc: Robert Foss <robert.foss@linaro.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > To: dri-devel@lists.freedesktop.org > > Marek Vasut (11): > drm: bridge: icn6211: Fix register layout > drm: bridge: icn6211: Fix HFP_HSW_HBP_HI and HFP_MIN handling > drm: bridge: icn6211: Add HS/VS/DE polarity handling > drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration > drm: bridge: icn6211: Use DSI burst mode without EoT and with LP > command mode > drm: bridge: icn6211: Disable DPI color swap > drm: bridge: icn6211: Set SYS_CTRL_1 to value used in examples > drm: bridge: icn6211: Implement atomic_get_input_bus_fmts > drm: bridge: icn6211: Add I2C configuration support > drm: bridge: icn6211: Rework ICN6211_DSI to chipone_writeb() > drm: bridge: icn6211: Read and validate chip IDs before configuration > > drivers/gpu/drm/bridge/chipone-icn6211.c | 486 ++++++++++++++++++++--- > 1 file changed, 434 insertions(+), 52 deletions(-) > > -- > 2.35.1 > This series looks ready to be merged, could you fix the remaining 'checkpatch --strict' warnings that are applicable? Ideally the line removal suggested by Maxime would be included too.
On 3/31/22 14:02, Robert Foss wrote: > Hey Marek, Hi, > On Fri, 18 Mar 2022 at 19:48, Marek Vasut <marex@denx.de> wrote: >> >> This series fixes multiple problems with the ICN6211 driver and adds >> support for configuration of the chip via I2C bus. >> >> First, in the current state, the ICN6211 driver hard-codes DPI timing >> and clock settings specific to some unknown panel. The settings provided >> by panel driver are ignored. Using any other panel than the one for which >> this driver is currently hard-coded can lead to permanent damage of the >> panel (per display supplier warning, and it sure did in my case. The >> damage looks like multiple rows of dead pixels at the bottom of the >> panel, and this is not going away even after long power off time). >> >> Much of this series thus fixes incorrect register layout, DPI timing >> programming, clock generation by adding actual PLL configuration code. >> This series no longer adds lane count decoding and retains current >> hard-coded lane count 4 due to disagreement over lane count parsing >> from DT. The lane count support will come later. The series also fills >> in a couple of registers with likely correct default values. >> >> Second, this series adds support for I2C configuration of the ICN6211. >> The device can be configured either via DSI command mode or via I2C, >> the register layout is the same in both cases. >> >> Since the datasheet for this device is very hard to come by, a lot of >> information has been salvaged from [1] and [2]. >> >> [1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c >> [2] https://github.com/tdjastrzebski/ICN6211-Configurator >> >> Cc: Jagan Teki <jagan@amarulasolutions.com> >> Cc: Maxime Ripard <maxime@cerno.tech> >> Cc: Robert Foss <robert.foss@linaro.org> >> Cc: Sam Ravnborg <sam@ravnborg.org> >> Cc: Thomas Zimmermann <tzimmermann@suse.de> >> To: dri-devel@lists.freedesktop.org >> >> Marek Vasut (11): >> drm: bridge: icn6211: Fix register layout >> drm: bridge: icn6211: Fix HFP_HSW_HBP_HI and HFP_MIN handling >> drm: bridge: icn6211: Add HS/VS/DE polarity handling >> drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration >> drm: bridge: icn6211: Use DSI burst mode without EoT and with LP >> command mode >> drm: bridge: icn6211: Disable DPI color swap >> drm: bridge: icn6211: Set SYS_CTRL_1 to value used in examples >> drm: bridge: icn6211: Implement atomic_get_input_bus_fmts >> drm: bridge: icn6211: Add I2C configuration support >> drm: bridge: icn6211: Rework ICN6211_DSI to chipone_writeb() >> drm: bridge: icn6211: Read and validate chip IDs before configuration >> >> drivers/gpu/drm/bridge/chipone-icn6211.c | 486 ++++++++++++++++++++--- >> 1 file changed, 434 insertions(+), 52 deletions(-) >> >> -- >> 2.35.1 >> > > This series looks ready to be merged I was waiting for 5.18-rc1 to be out and MW closed before picking it into drm-misc-next . Maybe I can pick it up now already ? > , could you fix the remaining > 'checkpatch --strict' warnings that are applicable? There are only these left, which I think is OK: WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) And then this one strict CHECK, but if I change that the formatting looks even uglier: 0010-drm-bridge-icn6211-Rework-ICN6211_DSI-to-chipone_wri.patch CHECK: Alignment should match open parenthesis #68: FILE: drivers/gpu/drm/bridge/chipone-icn6211.c:238: + chipone_writeb(icn, PLL_REF_DIV, (best_p ? PLL_REF_DIV_Pe : 0) | /* Prefer /2 pre-divider */ CHECK: Alignment should match open parenthesis #97: FILE: drivers/gpu/drm/bridge/chipone-icn6211.c:272: + chipone_writeb(icn, VACTIVE_HACTIVE_HI, ((mode->hdisplay >> 8) & 0xf) | CHECK: Alignment should match open parenthesis #113: FILE: drivers/gpu/drm/bridge/chipone-icn6211.c:284: + chipone_writeb(icn, HFP_HSW_HBP_HI, HFP_HSW_HBP_HI_HFP(hfp) | > Ideally the line > removal suggested by Maxime would be included too. This line removal comment has nothing to do with changes in this series, it is about the following patch, which is no longer part of this series because there is ongoing disagreement about that part and OF graph, so that patch will be resubmitted separately later: [PATCH v4 05/13] drm: bridge: icn6211: Add DSI lane count DT property parsing The continuation of that discussion is already in: [PATCH] dt-bindings: display: bridge: Drop requirement on input port for DSI devices So this series itself has no outstanding changes pending, unless you really want the uglier formatting above.
On Thu, 31 Mar 2022 at 15:42, Marek Vasut <marex@denx.de> wrote: > > On 3/31/22 14:02, Robert Foss wrote: > > Hey Marek, > > Hi, > > > On Fri, 18 Mar 2022 at 19:48, Marek Vasut <marex@denx.de> wrote: > >> > >> This series fixes multiple problems with the ICN6211 driver and adds > >> support for configuration of the chip via I2C bus. > >> > >> First, in the current state, the ICN6211 driver hard-codes DPI timing > >> and clock settings specific to some unknown panel. The settings provided > >> by panel driver are ignored. Using any other panel than the one for which > >> this driver is currently hard-coded can lead to permanent damage of the > >> panel (per display supplier warning, and it sure did in my case. The > >> damage looks like multiple rows of dead pixels at the bottom of the > >> panel, and this is not going away even after long power off time). > >> > >> Much of this series thus fixes incorrect register layout, DPI timing > >> programming, clock generation by adding actual PLL configuration code. > >> This series no longer adds lane count decoding and retains current > >> hard-coded lane count 4 due to disagreement over lane count parsing > >> from DT. The lane count support will come later. The series also fills > >> in a couple of registers with likely correct default values. > >> > >> Second, this series adds support for I2C configuration of the ICN6211. > >> The device can be configured either via DSI command mode or via I2C, > >> the register layout is the same in both cases. > >> > >> Since the datasheet for this device is very hard to come by, a lot of > >> information has been salvaged from [1] and [2]. > >> > >> [1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c > >> [2] https://github.com/tdjastrzebski/ICN6211-Configurator > >> > >> Cc: Jagan Teki <jagan@amarulasolutions.com> > >> Cc: Maxime Ripard <maxime@cerno.tech> > >> Cc: Robert Foss <robert.foss@linaro.org> > >> Cc: Sam Ravnborg <sam@ravnborg.org> > >> Cc: Thomas Zimmermann <tzimmermann@suse.de> > >> To: dri-devel@lists.freedesktop.org > >> > >> Marek Vasut (11): > >> drm: bridge: icn6211: Fix register layout > >> drm: bridge: icn6211: Fix HFP_HSW_HBP_HI and HFP_MIN handling > >> drm: bridge: icn6211: Add HS/VS/DE polarity handling > >> drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration > >> drm: bridge: icn6211: Use DSI burst mode without EoT and with LP > >> command mode > >> drm: bridge: icn6211: Disable DPI color swap > >> drm: bridge: icn6211: Set SYS_CTRL_1 to value used in examples > >> drm: bridge: icn6211: Implement atomic_get_input_bus_fmts > >> drm: bridge: icn6211: Add I2C configuration support > >> drm: bridge: icn6211: Rework ICN6211_DSI to chipone_writeb() > >> drm: bridge: icn6211: Read and validate chip IDs before configuration > >> > >> drivers/gpu/drm/bridge/chipone-icn6211.c | 486 ++++++++++++++++++++--- > >> 1 file changed, 434 insertions(+), 52 deletions(-) > >> > >> -- > >> 2.35.1 > >> > > > > This series looks ready to be merged > > I was waiting for 5.18-rc1 to be out and MW closed before picking it > into drm-misc-next . Maybe I can pick it up now already ? > > > , could you fix the remaining > > 'checkpatch --strict' warnings that are applicable? > > There are only these left, which I think is OK: > WARNING: Possible unwrapped commit description (prefer a maximum 75 > chars per line) > > And then this one strict CHECK, but if I change that the formatting > looks even uglier: > 0010-drm-bridge-icn6211-Rework-ICN6211_DSI-to-chipone_wri.patch > > CHECK: Alignment should match open parenthesis > #68: FILE: drivers/gpu/drm/bridge/chipone-icn6211.c:238: > + chipone_writeb(icn, PLL_REF_DIV, > (best_p ? PLL_REF_DIV_Pe : 0) | /* Prefer /2 > pre-divider */ > > CHECK: Alignment should match open parenthesis > #97: FILE: drivers/gpu/drm/bridge/chipone-icn6211.c:272: > + chipone_writeb(icn, VACTIVE_HACTIVE_HI, > ((mode->hdisplay >> 8) & 0xf) | > > CHECK: Alignment should match open parenthesis > #113: FILE: drivers/gpu/drm/bridge/chipone-icn6211.c:284: > + chipone_writeb(icn, HFP_HSW_HBP_HI, > HFP_HSW_HBP_HI_HFP(hfp) | > I'd like to at least strive for uniformity, so checkpatch is king, and whatever formatting it preferes is what should be used. > > Ideally the line > > removal suggested by Maxime would be included too. > > This line removal comment has nothing to do with changes in this series, > it is about the following patch, which is no longer part of this series > because there is ongoing disagreement about that part and OF graph, so > that patch will be resubmitted separately later: Ack, thanks for explaining. > > [PATCH v4 05/13] drm: bridge: icn6211: Add DSI lane count DT property > parsing > > The continuation of that discussion is already in: > > [PATCH] dt-bindings: display: bridge: Drop requirement on input port for > DSI devices > > So this series itself has no outstanding changes pending, unless you > really want the uglier formatting above. Yes, please resend with this fixed, and I'll merge it.
This series fixes multiple problems with the ICN6211 driver and adds support for configuration of the chip via I2C bus. First, in the current state, the ICN6211 driver hard-codes DPI timing and clock settings specific to some unknown panel. The settings provided by panel driver are ignored. Using any other panel than the one for which this driver is currently hard-coded can lead to permanent damage of the panel (per display supplier warning, and it sure did in my case. The damage looks like multiple rows of dead pixels at the bottom of the panel, and this is not going away even after long power off time). Much of this series thus fixes incorrect register layout, DPI timing programming, clock generation by adding actual PLL configuration code. This series no longer adds lane count decoding and retains current hard-coded lane count 4 due to disagreement over lane count parsing from DT. The lane count support will come later. The series also fills in a couple of registers with likely correct default values. Second, this series adds support for I2C configuration of the ICN6211. The device can be configured either via DSI command mode or via I2C, the register layout is the same in both cases. Since the datasheet for this device is very hard to come by, a lot of information has been salvaged from [1] and [2]. [1] https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/gpu/drm/bridge/icn6211.c [2] https://github.com/tdjastrzebski/ICN6211-Configurator Cc: Jagan Teki <jagan@amarulasolutions.com> Cc: Maxime Ripard <maxime@cerno.tech> Cc: Robert Foss <robert.foss@linaro.org> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> To: dri-devel@lists.freedesktop.org Marek Vasut (11): drm: bridge: icn6211: Fix register layout drm: bridge: icn6211: Fix HFP_HSW_HBP_HI and HFP_MIN handling drm: bridge: icn6211: Add HS/VS/DE polarity handling drm: bridge: icn6211: Add generic DSI-to-DPI PLL configuration drm: bridge: icn6211: Use DSI burst mode without EoT and with LP command mode drm: bridge: icn6211: Disable DPI color swap drm: bridge: icn6211: Set SYS_CTRL_1 to value used in examples drm: bridge: icn6211: Implement atomic_get_input_bus_fmts drm: bridge: icn6211: Add I2C configuration support drm: bridge: icn6211: Rework ICN6211_DSI to chipone_writeb() drm: bridge: icn6211: Read and validate chip IDs before configuration drivers/gpu/drm/bridge/chipone-icn6211.c | 486 ++++++++++++++++++++--- 1 file changed, 434 insertions(+), 52 deletions(-)