[PATCH] drm/modes: Fix outdated drm_mode_vrefresh return value documentation

[Jonathan Liu]

The vrefresh field in drm_display_mode struct was removed so the
function no longer checks if it is set before calculating it.

Fixes: 0425662fdf05 ("drm: Nuke mode->vrefresh")
Signed-off-by: Jonathan Liu 
---
 drivers/gpu/drm/drm_modes.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 3c8034a8c27b..2d51ab2734a0 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -803,8 +803,7 @@ EXPORT_SYMBOL(drm_mode_set_name);
  * @mode: mode
  *
  * Returns:
- * @modes's vrefresh rate in Hz, rounded to the nearest integer. Calculates the
- * value first if it is not yet set.
+ * @modes's vrefresh rate in Hz, rounded to the nearest integer.
  */
 int drm_mode_vrefresh(const struct drm_display_mode *mode)
 {
-- 
2.38.1

Re: [PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1

[Jonathan Liu]

Hi Marek,

On Mon, 23 May 2022 at 23:15, Marek Vasut  wrote:
>
> On 5/23/22 15:01, Jonathan Liu wrote:
> > The code from [1] sets SYS_CTRL_1 to different values depending on the
> > desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the
> > positive edge of the clock with the pixel data while other values delay
> > the clock by a fraction of the clock period. A clock phase of 1/2 aligns
> > the negative edge of the clock with the pixel data.
> >
> > The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to
> > aligning the positive edge of the clock with the pixel data. This won't
> > work correctly for panels that require aligning the negative edge of the
> > clock with the pixel data.
> >
> > Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is
> > present in bus_flags, otherwise adjust the clock phase to 1/2 as
> > appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.
> >
> > [1] https://github.com/tdjastrzebski/ICN6211-Configurator
> >
> > Signed-off-by: Jonathan Liu 
> > ---
> > V2: Use GENMASK and FIELD_PREP macros
> > ---
> >   drivers/gpu/drm/bridge/chipone-icn6211.c | 18 --
> >   1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c 
> > b/drivers/gpu/drm/bridge/chipone-icn6211.c
> > index 47dea657a752..f1538fb5f8a9 100644
> > --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> > +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> > @@ -9,6 +9,8 @@
> >   #include 
> >   #include 
> >
> > +#include 
> > +#include 
> >   #include 
> >   #include 
> >   #include 
> > @@ -26,6 +28,11 @@
> >   #define PD_CTRL(n)  (0x0a + ((n) & 0x3)) /* 0..3 */
> >   #define RST_CTRL(n) (0x0e + ((n) & 0x1)) /* 0..1 */
> >   #define SYS_CTRL(n) (0x10 + ((n) & 0x7)) /* 0..4 */
> > +#define SYS_CTRL_1_CLK_PHASE_MSK GENMASK(5, 4)
>
> This should be GENMASK(7, 6) , no ?

Clock phase 0 = 0b_1000_1000 = 0x88
Clock phase 1/4 = 0b_1001_1000 = 0x98
Clock phase 1/2 = 0b_1010_1000 = 0xA8
Clock phase 3/4 = 0b_1011_1000 = 0xB8

The clock phase bits are 5:4 not 7:6. The upper 2 bits and lower 4
bits are unknown.

Regards,
Jonathan

[PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1

[Jonathan Liu]

The code from [1] sets SYS_CTRL_1 to different values depending on the
desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the
positive edge of the clock with the pixel data while other values delay
the clock by a fraction of the clock period. A clock phase of 1/2 aligns
the negative edge of the clock with the pixel data.

The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to
aligning the positive edge of the clock with the pixel data. This won't
work correctly for panels that require aligning the negative edge of the
clock with the pixel data.

Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is
present in bus_flags, otherwise adjust the clock phase to 1/2 as
appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.

[1] https://github.com/tdjastrzebski/ICN6211-Configurator

Signed-off-by: Jonathan Liu 
---
V2: Use GENMASK and FIELD_PREP macros
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c 
b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 47dea657a752..f1538fb5f8a9 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -9,6 +9,8 @@
 #include 
 #include 
 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -26,6 +28,11 @@
 #define PD_CTRL(n) (0x0a + ((n) & 0x3)) /* 0..3 */
 #define RST_CTRL(n)(0x0e + ((n) & 0x1)) /* 0..1 */
 #define SYS_CTRL(n)(0x10 + ((n) & 0x7)) /* 0..4 */
+#define SYS_CTRL_1_CLK_PHASE_MSK   GENMASK(5, 4)
+#define CLK_PHASE_00
+#define CLK_PHASE_1_4  1
+#define CLK_PHASE_1_2  2
+#define CLK_PHASE_3_4  3
 #define RGB_DRV(n) (0x18 + ((n) & 0x3)) /* 0..3 */
 #define RGB_DLY(n) (0x1c + ((n) & 0x1)) /* 0..1 */
 #define RGB_TEST_CTRL  0x1e
@@ -336,7 +343,7 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
const struct drm_bridge_state *bridge_state;
u16 hfp, hbp, hsync;
u32 bus_flags;
-   u8 pol, id[4];
+   u8 pol, sys_ctrl_1, id[4];
 
chipone_readb(icn, VENDOR_ID, id);
chipone_readb(icn, DEVICE_ID_H, id + 1);
@@ -414,7 +421,14 @@ static void chipone_atomic_enable(struct drm_bridge 
*bridge,
chipone_configure_pll(icn, mode);
 
chipone_writeb(icn, SYS_CTRL(0), 0x40);
-   chipone_writeb(icn, SYS_CTRL(1), 0x88);
+   sys_ctrl_1 = 0x88;
+
+   if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
+   sys_ctrl_1 |= FIELD_PREP(SYS_CTRL_1_CLK_PHASE_MSK, CLK_PHASE_0);
+   else
+   sys_ctrl_1 |= FIELD_PREP(SYS_CTRL_1_CLK_PHASE_MSK, 
CLK_PHASE_1_2);
+
+   chipone_writeb(icn, SYS_CTRL(1), sys_ctrl_1);
 
/* icn6211 specific sequence */
chipone_writeb(icn, MIPI_FORCE_0, 0x20);
-- 
2.36.1

[PATCH] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1

[Jonathan Liu]

The code from [1] sets SYS_CTRL_1 to different values depending on the
desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the
positive edge of the clock with the pixel data while other values delay
the clock by a fraction of the clock period. A clock phase of 1/2 aligns
the negative edge of the clock with the pixel data.

The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to
aligning the positive edge of the clock with the pixel data. This won't
work correctly for panels that require aligning the negative edge of the
clock with the pixel data.

Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is
present in bus_flags, otherwise adjust the clock phase to 1/2 as
appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE.

[1] https://github.com/tdjastrzebski/ICN6211-Configurator

Signed-off-by: Jonathan Liu 
---
 drivers/gpu/drm/bridge/chipone-icn6211.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c 
b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 47dea657a752..df0290059aa3 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -57,6 +57,10 @@
 #define BIST_CHESS_XY_H0x30
 #define BIST_FRAME_TIME_L  0x31
 #define BIST_FRAME_TIME_H  0x32
+#define CLK_PHASE_00x88
+#define CLK_PHASE_1_4  0x98
+#define CLK_PHASE_1_2  0xa8
+#define CLK_PHASE_3_4  0xb8
 #define FIFO_MAX_ADDR_LOW  0x33
 #define SYNC_EVENT_DLY 0x34
 #define HSW_MIN0x35
@@ -414,7 +418,11 @@ static void chipone_atomic_enable(struct drm_bridge 
*bridge,
chipone_configure_pll(icn, mode);
 
chipone_writeb(icn, SYS_CTRL(0), 0x40);
-   chipone_writeb(icn, SYS_CTRL(1), 0x88);
+
+   if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE)
+   chipone_writeb(icn, SYS_CTRL(1), CLK_PHASE_0);
+   else
+   chipone_writeb(icn, SYS_CTRL(1), CLK_PHASE_1_2);
 
/* icn6211 specific sequence */
chipone_writeb(icn, MIPI_FORCE_0, 0x20);
-- 
2.36.1

Re: [PATCH] drm/bridge: dw-mipi-dsi: Move drm_bridge_add into probe

[Jonathan Liu]

Hi Jagan,

On Thu, 24 Jun 2021 at 22:34, Jagan Teki  wrote:
>
> Hi Jonathan,
>
> On Fri, Jun 18, 2021 at 6:40 PM Jonathan Liu  wrote:
> >
> > Hi Jagan,
> >
> > On Wed, 3 Feb 2021 at 09:13, Jagan Teki  wrote:
> > > @@ -1167,6 +1151,20 @@ __dw_mipi_dsi_probe(struct platform_device *pdev,
> > > dw_mipi_dsi_debugfs_init(dsi);
> > > pm_runtime_enable(dev);
> > >
> > > +   ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0,
> > > + , );
> > > +   if (ret)
> > > +   return ERR_PTR(ret);
> >
> > On RK3399 if the error is EPROBE_DEFER, __dw_mipi_dsi_probe can be
> > called again and result in the following errors:
> > [0.717589] debugfs: Directory 'ff96.mipi' with parent '/'
> > already present!
> > [0.717601] dw-mipi-dsi-rockchip ff96.mipi: failed to create debugfs 
> > root
> > [0.717606] dw-mipi-dsi-rockchip ff96.mipi: Unbalanced 
> > pm_runtime_enable!
>
> Is this when you test bridge on rk3399 or panel?

MIPI-DSI to LVDS bridge.

Regards,
Jonathan

Re: [PATCH] drm/bridge: dw-mipi-dsi: Move drm_bridge_add into probe

[Jonathan Liu]

Hi Jagan,

On Wed, 3 Feb 2021 at 09:13, Jagan Teki  wrote:
> @@ -1167,6 +1151,20 @@ __dw_mipi_dsi_probe(struct platform_device *pdev,
> dw_mipi_dsi_debugfs_init(dsi);
> pm_runtime_enable(dev);
>
> +   ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0,
> + , );
> +   if (ret)
> +   return ERR_PTR(ret);

On RK3399 if the error is EPROBE_DEFER, __dw_mipi_dsi_probe can be
called again and result in the following errors:
[0.717589] debugfs: Directory 'ff96.mipi' with parent '/'
already present!
[0.717601] dw-mipi-dsi-rockchip ff96.mipi: failed to create debugfs root
[0.717606] dw-mipi-dsi-rockchip ff96.mipi: Unbalanced pm_runtime_enable!

Regards,
Jonathan

Re: [PATCH] drm/bridge: ti-sn65dsi83: Fix null pointer dereference in remove callback

[Jonathan Liu]

Hi Marek,

On Fri, 18 Jun 2021 at 00:14, Laurent Pinchart
 wrote:
>
> Hi Jonathan,
>
> Thank you for the patch.
>
> On Thu, Jun 17, 2021 at 09:19:25PM +1000, Jonathan Liu wrote:
> > If attach has not been called, unloading the driver can result in a null
> > pointer dereference in mipi_dsi_detach as ctx->dsi has not been assigned
> > yet.
>
> Shouldn't this be done in a brige .detach() operation instead ?
>

Could you please take a look?
I don't have a working setup to test moving the code to detach.

> > Fixes: ceb515ba29ba6b ("drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and 
> > SN65DSI84 driver")
> > Signed-off-by: Jonathan Liu 
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 7 +--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c 
> > b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > index 750f2172ef08..8e9f45c5c7c1 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > @@ -671,8 +671,11 @@ static int sn65dsi83_remove(struct i2c_client *client)
> >  {
> >   struct sn65dsi83 *ctx = i2c_get_clientdata(client);
> >
> > - mipi_dsi_detach(ctx->dsi);
> > - mipi_dsi_device_unregister(ctx->dsi);
> > + if (ctx->dsi) {
> > + mipi_dsi_detach(ctx->dsi);
> > + mipi_dsi_device_unregister(ctx->dsi);
> > + }
> > +
> >   drm_bridge_remove(>bridge);
> >   of_node_put(ctx->host_node);
> >

Thanks.

Regards,
Jonathan

[PATCH] drm/bridge: ti-sn65dsi83: Fix null pointer dereference in remove callback

[Jonathan Liu]

If attach has not been called, unloading the driver can result in a null
pointer dereference in mipi_dsi_detach as ctx->dsi has not been assigned
yet.

Fixes: ceb515ba29ba6b ("drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and 
SN65DSI84 driver")
Signed-off-by: Jonathan Liu 
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c 
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 750f2172ef08..8e9f45c5c7c1 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -671,8 +671,11 @@ static int sn65dsi83_remove(struct i2c_client *client)
 {
struct sn65dsi83 *ctx = i2c_get_clientdata(client);
 
-   mipi_dsi_detach(ctx->dsi);
-   mipi_dsi_device_unregister(ctx->dsi);
+   if (ctx->dsi) {
+   mipi_dsi_detach(ctx->dsi);
+   mipi_dsi_device_unregister(ctx->dsi);
+   }
+
drm_bridge_remove(>bridge);
of_node_put(ctx->host_node);
 
-- 
2.32.0

Re: [RESEND PATCH] drm/rockchip: dsi: move all lane config except LCDC mux to bind()

[Jonathan Liu]

On Mon, 19 Apr 2021 at 02:04, Thomas Hebb  wrote:
>
> When we first enable the DSI encoder, we currently program some per-chip
> configuration that we look up in rk3399_chip_data based on the device
> tree compatible we match. This data configures various parameters of the
> MIPI lanes, including on RK3399 whether DSI1 is slaved to DSI0 in a
> dual-mode configuration. It also selects which LCDC (i.e. VOP) to scan
> out from.
>
> This causes a problem in RK3399 dual-mode configurations, though: panel
> prepare() callbacks run before the encoder gets enabled and expect to be
> able to write commands to the DSI bus, but the bus isn't fully
> functional until the lane and master/slave configuration have been
> programmed. As a result, dual-mode panels (and possibly others too) fail
> to turn on when the rockchipdrm driver is initially loaded.
>
> Because the LCDC mux is the only thing we don't know until enable time
> (and is the only thing that can ever change), we can actually move most
> of the initialization to bind() and get it out of the way early. That's
> what this change does. (Rockchip's 4.4 BSP kernel does it in mode_set(),
> which also avoids the issue, but bind() seems like the more correct
> place to me.)
>
> Tested on a Google Scarlet board (Acer Chromebook Tab 10), which has a
> Kingdisplay KD097D04 dual-mode panel. Prior to this change, the panel's
> backlight would turn on but no image would appear when initially loading
> rockchipdrm. If I kept rockchipdrm loaded and reloaded the panel driver,
> it would come on. With this change, the panel successfully turns on
> during initial rockchipdrm load as expected.
>
> Fixes: 2d4f7bdafd70 ("drm/rockchip: dsi: migrate to use dw-mipi-dsi bridge 
> driver")
> Signed-off-by: Thomas Hebb 

Tested-by: Jonathan Liu 

Fixes MIPI-DSI panel initialization for me on RK3399 too.

Regards,
Jonathan

Re: [PATCH] drm: bridge: dw-hdmi: Avoid resetting force in the detect function

[Jonathan Liu]

Hi Sam,

On Sun, 8 Nov 2020 at 9:47 pm, Sam Ravnborg  wrote:

> Hi Russell,
>
> On Sun, Nov 08, 2020 at 09:57:25AM +, Russell King - ARM Linux admin
> wrote:
> > On Sun, Nov 08, 2020 at 10:53:22AM +0100, Sam Ravnborg wrote:
> > > Russell,
> > >
> > > On Sat, Oct 31, 2020 at 07:17:47PM +1100, Jonathan Liu wrote:
> > > > It has been observed that resetting force in the detect function can
> > > > result in the PHY being powered down in response to hot-plug detect
> > > > being asserted, even when the HDMI connector is forced on.
> > > >
> > > > Enabling debug messages and adding a call to dump_stack() in
> > > > dw_hdmi_phy_power_off() shows the following in dmesg:
> > > > [  160.637413] dwhdmi-rockchip ff94.hdmi: EVENT=plugin
> > > > [  160.637433] dwhdmi-rockchip ff94.hdmi: PHY powered down in 0
> iterations
> > > >
> > > > Call trace:
> > > > dw_hdmi_phy_power_off
> > > > dw_hdmi_phy_disable
> > > > dw_hdmi_update_power
> > > > dw_hdmi_detect
> > > > dw_hdmi_connector_detect
> > > > drm_helper_probe_detect_ctx
> > > > drm_helper_hpd_irq_event
> > > > dw_hdmi_irq
> > > > irq_thread_fn
> > > > irq_thread
> > > > kthread
> > > > ret_from_fork
> > > >
> > > > Fixes: 381f05a7a842 ("drm: bridge/dw_hdmi: add connector mode
> forcing")
> > > > Signed-off-by: Jonathan Liu 
> > >
> > > you are the original author of this code - any comments on this patch?
> >
> > No further comments beyond what has already been discussed, and the
> > long and short of it is it's been so long that I don't remember why
> > that code was there. Given that, I'm not even in a position to ack
> > the change. Sorry.
> Thanks for the quick reply.
>
> Given that this fixes a problem for Jonathan I will apply this to -fixes
> if there is no other feedback the next couple of days.
> If it introduces regression we can take it from there.
>
> Jonathan - please ping me if I forget.
>
> Sam


> Ping.

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: bridge: dw-hdmi: Avoid resetting force in the detect function

[Jonathan Liu]

Hi Sam,

On Sun, 8 Nov 2020 at 21:47, Sam Ravnborg  wrote:
>
> Hi Russell,
>
> On Sun, Nov 08, 2020 at 09:57:25AM +, Russell King - ARM Linux admin 
> wrote:
> > On Sun, Nov 08, 2020 at 10:53:22AM +0100, Sam Ravnborg wrote:
> > > Russell,
> > >
> > > On Sat, Oct 31, 2020 at 07:17:47PM +1100, Jonathan Liu wrote:
> > > > It has been observed that resetting force in the detect function can
> > > > result in the PHY being powered down in response to hot-plug detect
> > > > being asserted, even when the HDMI connector is forced on.
> > > >
> > > > Enabling debug messages and adding a call to dump_stack() in
> > > > dw_hdmi_phy_power_off() shows the following in dmesg:
> > > > [  160.637413] dwhdmi-rockchip ff94.hdmi: EVENT=plugin
> > > > [  160.637433] dwhdmi-rockchip ff94.hdmi: PHY powered down in 0 
> > > > iterations
> > > >
> > > > Call trace:
> > > > dw_hdmi_phy_power_off
> > > > dw_hdmi_phy_disable
> > > > dw_hdmi_update_power
> > > > dw_hdmi_detect
> > > > dw_hdmi_connector_detect
> > > > drm_helper_probe_detect_ctx
> > > > drm_helper_hpd_irq_event
> > > > dw_hdmi_irq
> > > > irq_thread_fn
> > > > irq_thread
> > > > kthread
> > > > ret_from_fork
> > > >
> > > > Fixes: 381f05a7a842 ("drm: bridge/dw_hdmi: add connector mode forcing")
> > > > Signed-off-by: Jonathan Liu 
> > >
> > > you are the original author of this code - any comments on this patch?
> >
> > No further comments beyond what has already been discussed, and the
> > long and short of it is it's been so long that I don't remember why
> > that code was there. Given that, I'm not even in a position to ack
> > the change. Sorry.
> Thanks for the quick reply.
>
> Given that this fixes a problem for Jonathan I will apply this to -fixes
> if there is no other feedback the next couple of days.
> If it introduces regression we can take it from there.
>
> Jonathan - please ping me if I forget.
>
> Sam

Ping.

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: bridge: dw-hdmi: Avoid resetting force in the detect function

[Jonathan Liu]

It has been observed that resetting force in the detect function can
result in the PHY being powered down in response to hot-plug detect
being asserted, even when the HDMI connector is forced on.

Enabling debug messages and adding a call to dump_stack() in
dw_hdmi_phy_power_off() shows the following in dmesg:
[  160.637413] dwhdmi-rockchip ff94.hdmi: EVENT=plugin
[  160.637433] dwhdmi-rockchip ff94.hdmi: PHY powered down in 0 iterations

Call trace:
dw_hdmi_phy_power_off
dw_hdmi_phy_disable
dw_hdmi_update_power
dw_hdmi_detect
dw_hdmi_connector_detect
drm_helper_probe_detect_ctx
drm_helper_hpd_irq_event
dw_hdmi_irq
irq_thread_fn
irq_thread
kthread
ret_from_fork

Fixes: 381f05a7a842 ("drm: bridge/dw_hdmi: add connector mode forcing")
Signed-off-by: Jonathan Liu 
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 748df1cacd2b..0c79a9ba48bb 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2327,12 +2327,6 @@ static enum drm_connector_status dw_hdmi_detect(struct 
dw_hdmi *hdmi)
 {
enum drm_connector_status result;
 
-   mutex_lock(>mutex);
-   hdmi->force = DRM_FORCE_UNSPECIFIED;
-   dw_hdmi_update_power(hdmi);
-   dw_hdmi_update_phy_mask(hdmi);
-   mutex_unlock(>mutex);
-
result = hdmi->phy.ops->read_hpd(hdmi, hdmi->phy.data);
 
mutex_lock(>mutex);
-- 
2.29.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/rockchip: dw_hdmi: fix incorrect clock in vpll clock error message

[Jonathan Liu]

Error message incorrectly refers to grf clock instead of vpll clock.

Signed-off-by: Jonathan Liu 
---
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c 
b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 23de359a1dec..830bdd5e9b7c 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -202,7 +202,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi 
*hdmi)
} else if (PTR_ERR(hdmi->vpll_clk) == -EPROBE_DEFER) {
return -EPROBE_DEFER;
} else if (IS_ERR(hdmi->vpll_clk)) {
-   DRM_DEV_ERROR(hdmi->dev, "failed to get grf clock\n");
+   DRM_DEV_ERROR(hdmi->dev, "failed to get vpll clock\n");
return PTR_ERR(hdmi->vpll_clk);
}
 
-- 
2.29.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Jonathan Liu]

Hi Giulio,

On Wed, 12 Dec 2018 at 04:20, Giulio Benetti
 wrote:
>
> Hi Jonathan,
>
> Il 11/12/2018 11:49, Jonathan Liu ha scritto:
> > Hi Giulio,
> >
> > On Thu, 6 Dec 2018 at 22:00, Giulio Benetti
> >  wrote:
> >>
> >> Hi Jonathan,
> >>
> >> Il 06/12/2018 08:29, Jonathan Liu ha scritto:
> >>> Hi Giulio,
> >>>
> >>> On Thu, 15 Feb 2018 at 17:54, Giulio Benetti
> >>>  wrote:
> >>>>
> >>>> Differently from other Lcd signals, HSYNC and VSYNC signals
> >>>> result inverted if their bits are cleared to 0.
> >>>>
> >>>> Invert their settings of IO_POL register.
> >>>>
> >>>> Signed-off-by: Giulio Benetti 
> >>>> ---
> >>>>drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
> >>>>1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c 
> >>>> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >>>> index 3c15cf2..aaf911a 100644
> >>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >>>> @@ -389,10 +389,10 @@ static void sun4i_tcon0_mode_set_rgb(struct 
> >>>> sun4i_tcon *tcon,
> >>>>SUN4I_TCON0_BASIC3_H_SYNC(hsync));
> >>>>
> >>>>   /* Setup the polarity of the various signals */
> >>>> -   if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
> >>>> +   if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> >>>>   val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
> >>>>
> >>>> -   if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
> >>>> +   if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> >>>>   val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> >>>>
> >>>>   regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
> >>>
> >>> I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7"
> >>> LCD touchscreen (Innolux AT070TN92) connected to Olimex
> >>> A20-OLinuXino-MICRO that the image does not display correctly after
> >>> this change.
> >>> The image is shifted to the right.
> >>>
> >>> Reverting the change results in the image being displayed correctly on
> >>> the screen.
> >>>
> >>> I have in the device tree a "panel" node with compatible =
> >>> "innolux,at070tn92" which uses the timings in
> >>> drivers/gpu/drm/panel/panel-simple.c.
> >>>
> >>> Any ideas?
> >
> >>
> >> Checking Display Datasheet:
> >> https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf
> >>
> >> Page 13 section 3.3.2 you can see it needs active low VS and HS.
> >>
> >> You can refer to this Thread and check scope captures about VS and HS
> >> versus TCON0_IOPOL register:
> >> https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html
> >>
> >> There should be something that wrongly sets one of these or both:
> >> mode->flags |= DRM_MODE_FLAG_PHSYNC;
> >> and/or
> >> mode->flags |= DRM_MODE_FLAG_PVSYNC;
> >>
> >> Checked in panel-simple.c but it's not there.

> >
> > flags is 0 because it is not assigned in the struct definition for the 
> > panel.
>
> I don't think it is 0, because otherwise IO_POL_REG wouldn't be set to
> 0x0300 but to 0.
> What is checked is exactly mode->flags, so the problem seems to be upstream.
>
> This is my doubt, it seems mode->flags is not initialized or overriden
> at a certain point, this is why I want to debug it with Jtag tomorrow.
>

If you look at the change made by your patch:
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -389,10 +389,10 @@ static void sun4i_tcon0_mode_set_rgb(struct
sun4i_tcon *tcon,
 SUN4I_TCON0_BASIC3_H_SYNC(hsync));

/* Setup the polarity of the various signals */
-   if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
+   if (mode->flags & DRM_MODE_FLAG_PHSYNC)
val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;

-   if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
+   if (mode->flags & DRM_MODE_FLAG_PVSYNC)
val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;

regmap_update_bits(tcon->regs, SUN4I_TCO

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Jonathan Liu]

Hi Giulio,

On Wed, 12 Dec 2018 at 04:39, Giulio Benetti
 wrote:
>
> Forgot to ask you,
>
> Il 11/12/2018 18:20, Giulio Benetti ha scritto:
> > Hi Jonathan,
> >
> > Il 11/12/2018 11:49, Jonathan Liu ha scritto:
> >> Hi Giulio,
> >>
> >> On Thu, 6 Dec 2018 at 22:00, Giulio Benetti
> >>  wrote:
> >>>
> >>> Hi Jonathan,
> >>>
> >>> Il 06/12/2018 08:29, Jonathan Liu ha scritto:
> >>>> Hi Giulio,
> >>>>
> >>>> On Thu, 15 Feb 2018 at 17:54, Giulio Benetti
> >>>>  wrote:
> >>>>>
> >>>>> Differently from other Lcd signals, HSYNC and VSYNC signals
> >>>>> result inverted if their bits are cleared to 0.
> >>>>>
> >>>>> Invert their settings of IO_POL register.
> >>>>>
> >>>>> Signed-off-by: Giulio Benetti 
> >>>>> ---
> >>>>>drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
> >>>>>1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >>>>> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >>>>> index 3c15cf2..aaf911a 100644
> >>>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >>>>> @@ -389,10 +389,10 @@ static void sun4i_tcon0_mode_set_rgb(struct
> >>>>> sun4i_tcon *tcon,
> >>>>>SUN4I_TCON0_BASIC3_H_SYNC(hsync));
> >>>>>
> >>>>>   /* Setup the polarity of the various signals */
> >>>>> -   if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
> >>>>> +   if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> >>>>>   val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
> >>>>>
> >>>>> -   if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
> >>>>> +   if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> >>>>>   val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> >>>>>
> >>>>>   regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
> >>>>
> >>>> I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7"
> >>>> LCD touchscreen (Innolux AT070TN92) connected to Olimex
> >>>> A20-OLinuXino-MICRO that the image does not display correctly after
> >>>> this change.
> >>>> The image is shifted to the right.
> >>>>
> >>>> Reverting the change results in the image being displayed correctly on
> >>>> the screen.
> >>>>
> >>>> I have in the device tree a "panel" node with compatible =
> >>>> "innolux,at070tn92" which uses the timings in
> >>>> drivers/gpu/drm/panel/panel-simple.c.
> >>>>
> >>>> Any ideas?
> >>
> >>>
> >>> Checking Display Datasheet:
> >>> https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf
> >>>
> >>>
> >>> Page 13 section 3.3.2 you can see it needs active low VS and HS.
> >>>
> >>> You can refer to this Thread and check scope captures about VS and HS
> >>> versus TCON0_IOPOL register:
> >>> https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html
> >>>
> >>>
> >>> There should be something that wrongly sets one of these or both:
> >>> mode->flags |= DRM_MODE_FLAG_PHSYNC;
> >>> and/or
> >>> mode->flags |= DRM_MODE_FLAG_PVSYNC;
> >>>
> >>> Checked in panel-simple.c but it's not there.
> >>
> >> flags is 0 because it is not assigned in the struct definition for the
> >> panel.
> >
> > I don't think it is 0, because otherwise IO_POL_REG wouldn't be set to
> > 0x0300 but to 0.
> > What is checked is exactly mode->flags, so the problem seems to be
> > upstream.
> >
> > This is my doubt, it seems mode->flags is not initialized or overriden
> > at a certain point, this is why I want to debug it with Jtag tomorrow.
> >
> >> Before this change, TCON0_IO_POL_REG would be 0x0300 (both bits
> >> set to 1) and image displays correctly > After this change,
> >> TCON0_IO_POL_REG is 0x (both bits set to 0)
> >> and image doesn't display correctly.
> >>
> >> Checked using "devmem2 0x01c0c088" on A20-OLinuXino-MICRO Rev J.
> >
> > 0x0300 as I've triple checked with scope means Positive H/Vsync,
> > and 0x Negative H/VSync.
> >
> > Please check on the Thread I've pointed you above where there are all
> > the links to the scope captures.
> >
> > Are you completely sure you're using the correct panel?
> > This is because if with 0x0300 it works correctly, it means that
> > you're using Positive VS and HS but on datasheet on Figure 3.2 it shows
> > that they must be negative.
> >
> > Do you have any chance to measure those signals with a scope?
> >
> > Tomorrow, while debugging, I'll re-check H/Vsync signals again.
> >
> > Kind regards

>
> Can you precisely point me the sources of:
> - u-boot
> - kernel
> - dts
>
> you're using?
>
> Thanks

U-Boot - git tag v2017.03
Linux - git tag v4.19.6
DTS - see device tree changes in
https://github.com/net147/linux/tree/sun7i-drm-wip but change the
compatible to "innolux,at070tn92"

Thanks.

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Jonathan Liu]

Hi Giulio,

On Thu, 6 Dec 2018 at 22:00, Giulio Benetti
 wrote:
>
> Hi Jonathan,
>
> Il 06/12/2018 08:29, Jonathan Liu ha scritto:
> > Hi Giulio,
> >
> > On Thu, 15 Feb 2018 at 17:54, Giulio Benetti
> >  wrote:
> >>
> >> Differently from other Lcd signals, HSYNC and VSYNC signals
> >> result inverted if their bits are cleared to 0.
> >>
> >> Invert their settings of IO_POL register.
> >>
> >> Signed-off-by: Giulio Benetti 
> >> ---
> >>   drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c 
> >> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> index 3c15cf2..aaf911a 100644
> >> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> >> @@ -389,10 +389,10 @@ static void sun4i_tcon0_mode_set_rgb(struct 
> >> sun4i_tcon *tcon,
> >>   SUN4I_TCON0_BASIC3_H_SYNC(hsync));
> >>
> >>  /* Setup the polarity of the various signals */
> >> -   if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
> >> +   if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> >>  val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
> >>
> >> -   if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
> >> +   if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> >>  val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> >>
> >>  regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,
> >
> > I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7"
> > LCD touchscreen (Innolux AT070TN92) connected to Olimex
> > A20-OLinuXino-MICRO that the image does not display correctly after
> > this change.
> > The image is shifted to the right.
> >
> > Reverting the change results in the image being displayed correctly on
> > the screen.
> >
> > I have in the device tree a "panel" node with compatible =
> > "innolux,at070tn92" which uses the timings in
> > drivers/gpu/drm/panel/panel-simple.c.
> >
> > Any ideas?

>
> Checking Display Datasheet:
> https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf
>
> Page 13 section 3.3.2 you can see it needs active low VS and HS.
>
> You can refer to this Thread and check scope captures about VS and HS
> versus TCON0_IOPOL register:
> https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html
>
> There should be something that wrongly sets one of these or both:
> mode->flags |= DRM_MODE_FLAG_PHSYNC;
> and/or
> mode->flags |= DRM_MODE_FLAG_PVSYNC;
>
> Checked in panel-simple.c but it's not there.

flags is 0 because it is not assigned in the struct definition for the panel.
Before this change, TCON0_IO_POL_REG would be 0x0300 (both bits
set to 1) and image displays correctly.
After this change, TCON0_IO_POL_REG is 0x (both bits set to 0)
and image doesn't display correctly.

Checked using "devmem2 0x01c0c088" on A20-OLinuXino-MICRO Rev J.

>
> At the moment I don't have a board to check it with me, I'll try to do
> it soon, but I'm full with other work at the moment.
>
> If you have the chance to debug you could try to find in which point
> mode->flags gets changed with those 2 flags.
>
> When the problem came out I've noticed the same thing for u-boot too but
> it's been decided to not patch it because in that case every single
> sunxi defconfig had to be changed from:
> CONFIG_VIDEO_LCD_MODE="...,sync:3,..."
> to:
> CONFIG_VIDEO_LCD_MODE="...,sync:0,..."
> and it would have been a great mess, probably not worth it:
> https://lists.denx.de/pipermail/u-boot/2018-February/321405.html
>
> So keep in mind that TCON driver works differently for HS and
> VS(inverted) polarity in u-boot and linux.
>
> Hope to work this out soon.

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity

[Jonathan Liu]

Hi Giulio,

On Thu, 15 Feb 2018 at 17:54, Giulio Benetti
 wrote:
>
> Differently from other Lcd signals, HSYNC and VSYNC signals
> result inverted if their bits are cleared to 0.
>
> Invert their settings of IO_POL register.
>
> Signed-off-by: Giulio Benetti 
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c 
> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 3c15cf2..aaf911a 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -389,10 +389,10 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon 
> *tcon,
>  SUN4I_TCON0_BASIC3_H_SYNC(hsync));
>
> /* Setup the polarity of the various signals */
> -   if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
> +   if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
>
> -   if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
> +   if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
>
> regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG,

I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7"
LCD touchscreen (Innolux AT070TN92) connected to Olimex
A20-OLinuXino-MICRO that the image does not display correctly after
this change.
The image is shifted to the right.

Reverting the change results in the image being displayed correctly on
the screen.

I have in the device tree a "panel" node with compatible =
"innolux,at070tn92" which uses the timings in
drivers/gpu/drm/panel/panel-simple.c.

Any ideas?

Thanks.

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm] tests/util: Add support for sun4i-drm module

[Jonathan Liu]

Add support for sun4i DRM driver merged for Linux 4.7.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 tests/util/kms.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/util/kms.c b/tests/util/kms.c
index 8b3e7878..ffae2531 100644
--- a/tests/util/kms.c
+++ b/tests/util/kms.c
@@ -144,6 +144,7 @@ static const char * const modules[] = {
"mediatek",
"meson",
"pl111",
+   "sun4i-drm",
 };
 
 int util_open(const char *device, const char *module)
-- 
2.16.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 0/3] drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate

[Jonathan Liu]

This patchset fixes several issues in sun4i_tmds_determine_rate that I
discovered while trying to get a projector connected to an Olimex
A20-OLinuXino-LIME using HDMI with a native resolution of 1280x800 and
pixel clock of 83.5 MHz to display at its native resolution.

Changes for v3:
- Improve commit message for unset best_parent

Changes for v2:
- Split into separate patches for each issue
- Add details to commit message for reproducing issue

Jonathan Liu (3):
  drm/sun4i: hdmi: Check for unset best_parent in
sun4i_tmds_determine_rate
  drm/sun4i: hdmi: Fix incorrect assignment in sun4i_tmds_determine_rate
  drm/sun4i: hdmi: Add missing rate halving check in
sun4i_tmds_determine_rate

 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.15.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 2/3] drm/sun4i: hdmi: Fix incorrect assignment in sun4i_tmds_determine_rate

[Jonathan Liu]

best_div is set to i which corresponds to rate halving when it should be
set to j which corresponds to the divider.

Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c 
b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
index 4d235e5ea31c..88eeeaf34638 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
@@ -105,7 +105,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
if (!best_parent || abs(rate - rounded / i) <
abs(rate - best_parent / best_div)) {
best_parent = rounded;
-   best_div = i;
+   best_div = j;
}
}
}
-- 
2.15.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 3/3] drm/sun4i: hdmi: Add missing rate halving check in sun4i_tmds_determine_rate

[Jonathan Liu]

It was only checking the divider when determing the closest match if
it could not match the requested rate exactly.

For a projector connected to an Olimex A20-OLinuXino-LIME using HDMI
with a native resolution of 1280x800 and pixel clock of 83.5 MHz, this
resulted in 1280x800 mode not being available and the following in dmesg
when the kernel is booted with drm.debug=0x3e:
[drm:drm_mode_debug_printmodeline] Modeline 37:"1280x800" 60 83500 1280 1352 
1480 1680 800 810 816 831 0x48 0x5
[drm:drm_mode_prune_invalid] Not using 1280x800 mode: NOCLOCK

Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c 
b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
index 88eeeaf34638..3ecffa52c814 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
@@ -102,9 +102,12 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
goto out;
}
 
-   if (!best_parent || abs(rate - rounded / i) <
-   abs(rate - best_parent / best_div)) {
+   if (!best_parent ||
+   abs(rate - rounded / i / j) <
+   abs(rate - best_parent / best_half /
+   best_div)) {
best_parent = rounded;
+   best_half = i;
best_div = j;
}
}
-- 
2.15.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate

[Jonathan Liu]

It is possible that if there is no exact rate match and
"rounded = clk_hw_round_rate(parent, ideal)" gives high enough values
(e.g. if rounded is 2 * ideal) that the condition
"abs(rate - rounded / i) < abs(rate - best_parent / best_div)" is never
met and best_parent is never set. This results in req->rate and
req->best_parent_rate being assigned 0.

To avoid this, we set best_parent to the first calculated rate if it is
unset. The sun4i_tmds_calc_divider function already has a similar check.

Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c 
b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
index dc332ea56f6c..4d235e5ea31c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
@@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
goto out;
}
 
-   if (abs(rate - rounded / i) <
+   if (!best_parent || abs(rate - rounded / i) <
abs(rate - best_parent / best_div)) {
best_parent = rounded;
best_div = i;
-- 
2.15.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate

[Jonathan Liu]

Hi Maxime,

On 5 January 2018 at 21:03, Maxime Ripard
<maxime.rip...@free-electrons.com> wrote:
> On Fri, Jan 05, 2018 at 09:44:39AM +1100, Jonathan Liu wrote:
>> On 5 January 2018 at 06:56, Maxime Ripard
>> <maxime.rip...@free-electrons.com> wrote:
>> > On Tue, Dec 26, 2017 at 10:12:25PM +1100, Jonathan Liu wrote:
>> >> We should check if the best match has been set before comparing it.
>> >>
>> >> Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support")
>> >> Signed-off-by: Jonathan Liu <net...@gmail.com>
>> >> ---
>> >>  drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c 
>> >> b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
>> >> index dc332ea56f6c..4d235e5ea31c 100644
>> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
>> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
>> >> @@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw 
>> >> *hw,
>> >>   goto out;
>> >>   }
>> >>
>> >> - if (abs(rate - rounded / i) <
>> >> + if (!best_parent || abs(rate - rounded / i) 
>> >> <
>> >
>> > Why is that causing any issue?
>> >
>> > If best_parent is set to 0...
>> >
>> >>   abs(rate - best_parent / best_div)) {
>> >
>> > ... the value returned here is going to be rate, which is going to be
>> > higher than the first part of the comparison meaning ...
>> >
>> >>   best_parent = rounded;
>> >
>> > ... that best_parent is going to be set there.
>>
>> Consider the following:
>> rate = 8350
>> rounded = ideal * 2
>>
>> It is possible that if "rounded = clk_hw_round_rate(parent, ideal)"
>> gives high enough values that the condition "abs(rate - rounded / i) <
>> abs(rate - best_parent / best_div)" is never met.
>>
>> Then you can end up with:
>> req->rate = 0
>> req->best_parent_rate = 0
>> req->best_parent_hw = ...
>>
>> Also, the sun4i_tmds_calc_divider function has a similar check.
>
> Ok. That explanation must be part of your commit log, or at least
> which problem you're trying to address and in which situation it will
> trigger.

Ok. I have added amended the commit message with explanation for v3.

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate

[Jonathan Liu]

Hi Maxime,

On 5 January 2018 at 06:56, Maxime Ripard
<maxime.rip...@free-electrons.com> wrote:
> On Tue, Dec 26, 2017 at 10:12:25PM +1100, Jonathan Liu wrote:
>> We should check if the best match has been set before comparing it.
>>
>> Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support")
>> Signed-off-by: Jonathan Liu <net...@gmail.com>
>> ---
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c 
>> b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
>> index dc332ea56f6c..4d235e5ea31c 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
>> @@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
>>   goto out;
>>   }
>>
>> - if (abs(rate - rounded / i) <
>> + if (!best_parent || abs(rate - rounded / i) <
>
> Why is that causing any issue?
>
> If best_parent is set to 0...
>
>>   abs(rate - best_parent / best_div)) {
>
> ... the value returned here is going to be rate, which is going to be
> higher than the first part of the comparison meaning ...
>
>>   best_parent = rounded;
>
> ... that best_parent is going to be set there.

Consider the following:
rate = 8350
rounded = ideal * 2

It is possible that if "rounded = clk_hw_round_rate(parent, ideal)"
gives high enough values that the condition "abs(rate - rounded / i) <
abs(rate - best_parent / best_div)" is never met.

Then you can end up with:
req->rate = 0
req->best_parent_rate = 0
req->best_parent_hw = ...

Also, the sun4i_tmds_calc_divider function has a similar check.

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/3] drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate

[Jonathan Liu]

This patchset fixes several issues in sun4i_tmds_determine_rate that I
discovered while trying to get a projector connected to an Olimex
A20-OLinuXino-LIME using HDMI with a native resolution of 1280x800 and
pixel clock of 83.5 MHz to display at its native resolution.

Changes for v2:
- Split into separate patches for each issue
- Add details to commit message for reproducing issue

Jonathan Liu (3):
  drm/sun4i: hdmi: Check for unset best_parent in
sun4i_tmds_determine_rate
  drm/sun4i: hdmi: Fix incorrect assignment in sun4i_tmds_determine_rate
  drm/sun4i: hdmi: Add missing rate halving check in
sun4i_tmds_determine_rate

 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.15.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/3] drm/sun4i: hdmi: Fix incorrect assignment in sun4i_tmds_determine_rate

[Jonathan Liu]

best_div is set to i which corresponds to rate halving when it should be
set to j which corresponds to the divider.

Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c 
b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
index 4d235e5ea31c..88eeeaf34638 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
@@ -105,7 +105,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
if (!best_parent || abs(rate - rounded / i) <
abs(rate - best_parent / best_div)) {
best_parent = rounded;
-   best_div = i;
+   best_div = j;
}
}
}
-- 
2.15.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/3] drm/sun4i: hdmi: Add missing rate halving check in sun4i_tmds_determine_rate

[Jonathan Liu]

It was only checking the divider when determing the closest match if
it could not match the requested rate exactly.

For a projector connected to an Olimex A20-OLinuXino-LIME using HDMI
with a native resolution of 1280x800 and pixel clock of 83.5 MHz, this
resulted in 1280x800 mode not being available and the following in dmesg
when the kernel is booted with drm.debug=0x3e:
[drm:drm_mode_debug_printmodeline] Modeline 37:"1280x800" 60 83500 1280 1352 
1480 1680 800 810 816 831 0x48 0x5
[drm:drm_mode_prune_invalid] Not using 1280x800 mode: NOCLOCK

Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c 
b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
index 88eeeaf34638..3ecffa52c814 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
@@ -102,9 +102,12 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
goto out;
}
 
-   if (!best_parent || abs(rate - rounded / i) <
-   abs(rate - best_parent / best_div)) {
+   if (!best_parent ||
+   abs(rate - rounded / i / j) <
+   abs(rate - best_parent / best_half /
+   best_div)) {
best_parent = rounded;
+   best_half = i;
best_div = j;
}
}
-- 
2.15.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate

[Jonathan Liu]

We should check if the best match has been set before comparing it.

Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c 
b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
index dc332ea56f6c..4d235e5ea31c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
@@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
goto out;
}
 
-   if (abs(rate - rounded / i) <
+   if (!best_parent || abs(rate - rounded / i) <
abs(rate - best_parent / best_div)) {
best_parent = rounded;
best_div = i;
-- 
2.15.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate

[Jonathan Liu]

There are several issues in sun4i_tmds_determine_rate:
- doesn't check if the best match was already set before comparing it
  with the enumerated parameters which could result in integer divide
  by zero
- doesn't consider rate halving when determining closest match if it
  can't match the requested rate exactly
- sets best_div to i which corresponds to rate halving when it should be
  set to j which corresponds to the divider

Fix these issues.

Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support")
Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c 
b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
index dc332ea56f6c..3ecffa52c814 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
@@ -102,10 +102,13 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw,
goto out;
}
 
-   if (abs(rate - rounded / i) <
-   abs(rate - best_parent / best_div)) {
+   if (!best_parent ||
+   abs(rate - rounded / i / j) <
+   abs(rate - best_parent / best_half /
+   best_div)) {
best_parent = rounded;
-   best_div = i;
+   best_half = i;
+   best_div = j;
}
}
}
-- 
2.15.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [14/23] drm/sun4i: Create minimal multipliers and dividers

[Jonathan Liu]

Hi Maxime,

On 17 October 2017 at 20:06, Maxime Ripard
 wrote:
> The various outputs the TCON can provide have different constraints on the
> dotclock divider. Let's make them configurable by the various mode_set
> functions.
>
> Signed-off-by: Maxime Ripard 
> Reviewed-by: Chen-Yu Tsai 
> ---
>  drivers/gpu/drm/sun4i/sun4i_dotclock.c | 10 +++---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c |  2 ++
>  drivers/gpu/drm/sun4i/sun4i_tcon.h |  2 ++
>  3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c 
> b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> index d401156490f3..023f39bda633 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> @@ -17,8 +17,9 @@
>  #include "sun4i_dotclock.h"
>
>  struct sun4i_dclk {
> -   struct clk_hw   hw;
> -   struct regmap   *regmap;
> +   struct clk_hw   hw;
> +   struct regmap   *regmap;
> +   struct sun4i_tcon   *tcon;
>  };
>
>  static inline struct sun4i_dclk *hw_to_dclk(struct clk_hw *hw)
> @@ -73,11 +74,13 @@ static unsigned long sun4i_dclk_recalc_rate(struct clk_hw 
> *hw,
>  static long sun4i_dclk_round_rate(struct clk_hw *hw, unsigned long rate,
>   unsigned long *parent_rate)
>  {
> +   struct sun4i_dclk *dclk = hw_to_dclk(hw);
> +   struct sun4i_tcon *tcon = dclk->tcon;
> unsigned long best_parent = 0;
> u8 best_div = 1;
> int i;
>
> -   for (i = 6; i <= 127; i++) {
> +   for (i = tcon->dclk_min_div; i <= tcon->dclk_max_div; i++) {
> unsigned long ideal = rate * i;
> unsigned long rounded;
>
> @@ -167,6 +170,7 @@ int sun4i_dclk_create(struct device *dev, struct 
> sun4i_tcon *tcon)
> dclk = devm_kzalloc(dev, sizeof(*dclk), GFP_KERNEL);
> if (!dclk)
> return -ENOMEM;
> +   dclk->tcon = tcon;
>
> init.name = clk_name;
> init.ops = _dclk_ops;
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c 
> b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index f69bcdf11cb8..3efa1ab045cd 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -177,6 +177,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon 
> *tcon,
> u8 clk_delay;
> u32 val = 0;
>
> +   tcon->dclk_min_div = 6;
> +   tcon->dclk_max_div = 127;
> sun4i_tcon0_mode_set_common(tcon, mode);
>
> /* Adjust clock delay */
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h 
> b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> index f61bf6d83b4a..4141fbd97ddf 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> @@ -169,6 +169,8 @@ struct sun4i_tcon {
>
> /* Pixel clock */
> struct clk  *dclk;
> +   u8  dclk_max_div;
> +   u8  dclk_min_div;
>
> /* Reset control */
> struct reset_control*lcd_rst;

I have 4.3" RGB LCD enabled on sun7i-a20-olinuxino-lime and hdmi
disabled. After applying this patch the LCD no longer turns on.
If I add some debug statements to sun4i_dclk_recalc_rate, it shows
tcon->dclk_min_div and tcon->dclk_max_div are both 0.

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [15/23] drm/sun4i: Add LVDS support

[Jonathan Liu]

Hi Maxime,

On 17 October 2017 at 20:06, Maxime Ripard
 wrote:
> The TCON supports the LVDS interface to output to a panel or a bridge.
> Let's add support for it.
>
> Signed-off-by: Maxime Ripard 
> ---
>  drivers/gpu/drm/sun4i/Makefile |   1 +-
>  drivers/gpu/drm/sun4i/sun4i_lvds.c | 183 -
>  drivers/gpu/drm/sun4i/sun4i_lvds.h |  18 +++-
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 193 +-
>  drivers/gpu/drm/sun4i/sun4i_tcon.h |  25 -
>  5 files changed, 419 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_lvds.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_lvds.h
>
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index cfba2c07519c..6fee15d016ef 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -10,6 +10,7 @@ sun4i-drm-hdmi-y  += sun4i_hdmi_tmds_clk.o
>
>  sun4i-tcon-y   += sun4i_tcon.o
>  sun4i-tcon-y   += sun4i_rgb.o
> +sun4i-tcon-y   += sun4i_lvds.o
>  sun4i-tcon-y   += sun4i_dotclock.o
>  sun4i-tcon-y   += sun4i_crtc.o
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c 
> b/drivers/gpu/drm/sun4i/sun4i_lvds.c
> new file mode 100644
> index ..635a3f505ecb
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c
> @@ -0,0 +1,183 @@
> +/*
> + * Copyright (C) 2015 NextThing Co
> + * Copyright (C) 2015-2017 Free Electrons
> + *
> + * Maxime Ripard 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "sun4i_crtc.h"
> +#include "sun4i_tcon.h"
> +#include "sun4i_lvds.h"
> +
> +struct sun4i_lvds {
> +   struct drm_connectorconnector;
> +   struct drm_encoder  encoder;
> +
> +   struct sun4i_tcon   *tcon;
> +};
> +
> +static inline struct sun4i_lvds *
> +drm_connector_to_sun4i_lvds(struct drm_connector *connector)
> +{
> +   return container_of(connector, struct sun4i_lvds,
> +   connector);
> +}
> +
> +static inline struct sun4i_lvds *
> +drm_encoder_to_sun4i_lvds(struct drm_encoder *encoder)
> +{
> +   return container_of(encoder, struct sun4i_lvds,
> +   encoder);
> +}
> +
> +static int sun4i_lvds_get_modes(struct drm_connector *connector)
> +{
> +   struct sun4i_lvds *lvds =
> +   drm_connector_to_sun4i_lvds(connector);
> +   struct sun4i_tcon *tcon = lvds->tcon;
> +
> +   return drm_panel_get_modes(tcon->panel);
> +}
> +
> +static struct drm_connector_helper_funcs sun4i_lvds_con_helper_funcs = {
> +   .get_modes  = sun4i_lvds_get_modes,
> +};
> +
> +static void
> +sun4i_lvds_connector_destroy(struct drm_connector *connector)
> +{
> +   struct sun4i_lvds *lvds = drm_connector_to_sun4i_lvds(connector);
> +   struct sun4i_tcon *tcon = lvds->tcon;
> +
> +   drm_panel_detach(tcon->panel);
> +   drm_connector_cleanup(connector);
> +}
> +
> +static const struct drm_connector_funcs sun4i_lvds_con_funcs = {
> +   .fill_modes = drm_helper_probe_single_connector_modes,
> +   .destroy= sun4i_lvds_connector_destroy,
> +   .reset  = drm_atomic_helper_connector_reset,
> +   .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +   .atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static void sun4i_lvds_encoder_enable(struct drm_encoder *encoder)
> +{
> +   struct sun4i_lvds *lvds = drm_encoder_to_sun4i_lvds(encoder);
> +   struct sun4i_tcon *tcon = lvds->tcon;
> +
> +   DRM_DEBUG_DRIVER("Enabling LVDS output\n");
> +
> +   if (!IS_ERR(tcon->panel)) {
> +   drm_panel_prepare(tcon->panel);
> +   drm_panel_enable(tcon->panel);
> +   }
> +}
> +
> +static void sun4i_lvds_encoder_disable(struct drm_encoder *encoder)
> +{
> +   struct sun4i_lvds *lvds = drm_encoder_to_sun4i_lvds(encoder);
> +   struct sun4i_tcon *tcon = lvds->tcon;
> +
> +   DRM_DEBUG_DRIVER("Disabling LVDS output\n");
> +
> +   if (!IS_ERR(tcon->panel)) {
> +   drm_panel_disable(tcon->panel);
> +   drm_panel_unprepare(tcon->panel);
> +   }
> +}
> +
> +static const struct drm_encoder_helper_funcs sun4i_lvds_enc_helper_funcs = {
> +   .disable= sun4i_lvds_encoder_disable,
> +   .enable = sun4i_lvds_encoder_enable,
> +};
> +
> +static const struct drm_encoder_funcs sun4i_lvds_enc_funcs = {
> +   .destroy= 

[PATCH] drm/sun4i: tcon: Add dithering support for RGB565/RGB666 LCD panels

[Jonathan Liu]

Dithering is supported on TCON channel 0 which is used for LCD panels.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 37 +
 drivers/gpu/drm/sun4i/sun4i_tcon.h | 18 +-
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c 
b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 68751b999877..cf313ca858b3 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -12,11 +12,13 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -168,7 +170,9 @@ void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon,
  struct drm_display_mode *mode)
 {
unsigned int bp, hsync, vsync;
+   u32 bus_format = 0;
u8 clk_delay;
+   struct drm_connector *connector = tcon->panel->connector;
u32 val = 0;
 
/* Configure the dot clock */
@@ -230,6 +234,39 @@ void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon,
   SUN4I_TCON0_IO_POL_HSYNC_POSITIVE | 
SUN4I_TCON0_IO_POL_VSYNC_POSITIVE,
   val);
 
+   if (connector->display_info.num_bus_formats)
+   bus_format = connector->display_info.bus_formats[0];
+
+   switch (bus_format) {
+   case MEDIA_BUS_FMT_RGB565_1X16:
+   case MEDIA_BUS_FMT_RGB666_1X18:
+   /* Enable dithering */
+   /* FIXME: Undocumented bits */
+   regmap_write(tcon->regs, SUN4I_TCON0_FRM_SEED0_REG, 0x);
+   regmap_write(tcon->regs, SUN4I_TCON0_FRM_SEED1_REG, 0x);
+   regmap_write(tcon->regs, SUN4I_TCON0_FRM_SEED2_REG, 0x);
+   regmap_write(tcon->regs, SUN4I_TCON0_FRM_SEED3_REG, 0x);
+   regmap_write(tcon->regs, SUN4I_TCON0_FRM_SEED4_REG, 0x);
+   regmap_write(tcon->regs, SUN4I_TCON0_FRM_SEED5_REG, 0x);
+   regmap_write(tcon->regs, SUN4I_TCON0_FRM_TAB0_REG, 0x0101);
+   regmap_write(tcon->regs, SUN4I_TCON0_FRM_TAB1_REG, 0x1515);
+   regmap_write(tcon->regs, SUN4I_TCON0_FRM_TAB2_REG, 0x5757);
+   regmap_write(tcon->regs, SUN4I_TCON0_FRM_TAB3_REG, 0x7f7f);
+   val = SUN4I_TCON0_FRM_CTL_ENABLE;
+
+   if (bus_format == MEDIA_BUS_FMT_RGB565_1X16) {
+   val |= SUN4I_TCON0_FRM_CTL_MODE_R;
+   val |= SUN4I_TCON0_FRM_CTL_MODE_B;
+   }
+
+   regmap_write(tcon->regs, SUN4I_TCON0_FRM_CTL_REG, val);
+   break;
+   default:
+   /* Disable dithering */
+   regmap_write(tcon->regs, SUN4I_TCON0_FRM_CTL_REG, 0);
+   break;
+   }
+
/* Map output pins to channel 0 */
regmap_update_bits(tcon->regs, SUN4I_TCON_GCTL_REG,
   SUN4I_TCON_GCTL_IOMAP_MASK,
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h 
b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index d9e1357cc8ae..d64d45144c91 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -31,7 +31,23 @@
 #define SUN4I_TCON_GINT0_VBLANK_INT(pipe)  BIT(15 - (pipe))
 
 #define SUN4I_TCON_GINT1_REG   0x8
-#define SUN4I_TCON_FRM_CTL_REG 0x10
+
+#define SUN4I_TCON0_FRM_CTL_REG0x10
+#define SUN4I_TCON0_FRM_CTL_ENABLE BIT(31)
+#define SUN4I_TCON0_FRM_CTL_MODE_R BIT(6)
+#define SUN4I_TCON0_FRM_CTL_MODE_G BIT(5)
+#define SUN4I_TCON0_FRM_CTL_MODE_B BIT(4)
+
+#define SUN4I_TCON0_FRM_SEED0_REG  0x14
+#define SUN4I_TCON0_FRM_SEED1_REG  0x18
+#define SUN4I_TCON0_FRM_SEED2_REG  0x1c
+#define SUN4I_TCON0_FRM_SEED3_REG  0x20
+#define SUN4I_TCON0_FRM_SEED4_REG  0x24
+#define SUN4I_TCON0_FRM_SEED5_REG  0x28
+#define SUN4I_TCON0_FRM_TAB0_REG   0x2c
+#define SUN4I_TCON0_FRM_TAB1_REG   0x30
+#define SUN4I_TCON0_FRM_TAB2_REG   0x34
+#define SUN4I_TCON0_FRM_TAB3_REG   0x38
 
 #define SUN4I_TCON0_CTL_REG0x40
 #define SUN4I_TCON0_CTL_TCON_ENABLEBIT(31)
-- 
2.14.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/panel: simple: Add missing panel_simple_unprepare calls

[Jonathan Liu]

During panel removal or system shutdown panel_simple_disable is called
which disables the panel backlight but the panel is still powered due to
missing calls to panel_simple_unprepare.

Fixes: d02fd93e2cd8 ("drm/panel: simple - Disable panel on shutdown")
Cc: sta...@vger.kernel.org # v3.16+
Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/gpu/drm/panel/panel-simple.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-simple.c 
b/drivers/gpu/drm/panel/panel-simple.c
index 474fa759e06e..234af81fb3d0 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -369,6 +369,7 @@ static int panel_simple_remove(struct device *dev)
drm_panel_remove(>base);
 
panel_simple_disable(>base);
+   panel_simple_unprepare(>base);
 
if (panel->ddc)
put_device(>ddc->dev);
@@ -384,6 +385,7 @@ static void panel_simple_shutdown(struct device *dev)
struct panel_simple *panel = dev_get_drvdata(dev);
 
panel_simple_disable(>base);
+   panel_simple_unprepare(>base);
 }
 
 static const struct drm_display_mode ampire_am_480272h3tmqw_t01h_mode = {
-- 
2.13.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/panel: simple: Fix width and height for Olimex LCD-OLinuXino-4.3TS

[Jonathan Liu]

The physical size of the panel is 105.5 (W) x 67.2 (H) x 4.05 (D) mm
but the active display area is 95.04 (W) x 53.856 (H) mm.

The width and height should be set to the active display area.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/gpu/drm/panel/panel-simple.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c 
b/drivers/gpu/drm/panel/panel-simple.c
index 474fa759e06e..39a622a547e7 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1522,8 +1522,8 @@ static const struct panel_desc olimex_lcd_olinuxino_43ts 
= {
.modes = _lcd_olinuxino_43ts_mode,
.num_modes = 1,
.size = {
-   .width = 105,
-   .height = 67,
+   .width = 95,
+   .height = 54,
},
.bus_format = MEDIA_BUS_FMT_RGB888_1X24,
 };
-- 
2.13.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/sun4i: Implement drm_driver lastclose to restore fbdev console

[Jonathan Liu]

Hi Maxime,

On 10 July 2017 at 16:44, Maxime Ripard
<maxime.rip...@free-electrons.com> wrote:
> On Sun, Jul 09, 2017 at 11:11:07PM +0800, Chen-Yu Tsai wrote:
>> On Sun, Jul 9, 2017 at 3:59 PM, Jonathan Liu <net...@gmail.com> wrote:
>> > The drm_driver lastclose callback is called when the last userspace
>> > DRM client has closed. Call drm_fbdev_cma_restore_mode to restore
>> > the fbdev console otherwise the fbdev console will stop working.
>> >
>> > Signed-off-by: Jonathan Liu <net...@gmail.com>
>>
>> This should have
>>
>> Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
>>
>> Otherwise,
>>
>> Reviewed-by: Chen-Yu Tsai <w...@csie.org>
>
> And it should be sent to stable too.
>
> Can you resend it?

Will do.

>
> Thanks!
> Maxime
>
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3] drm/sun4i: Implement drm_driver lastclose to restore fbdev console

[Jonathan Liu]

The drm_driver lastclose callback is called when the last userspace
DRM client has closed. Call drm_fbdev_cma_restore_mode to restore
the fbdev console otherwise the fbdev console will stop working.

Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
Cc: sta...@vger.kernel.org
Signed-off-by: Jonathan Liu <net...@gmail.com>
Reviewed-by: Chen-Yu Tsai <w...@csie.org>
---
Changes for v3:
 - Add 'Fixes:' tag
 - Add CC to stable
 - Add 'Reviewed-by: Chen-Yu Tsai <w...@csie.org>'

Changes for v2:
 - Rename sun4i_drm_lastclose to sun4i_drv_lastclose

 drivers/gpu/drm/sun4i/sun4i_drv.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c 
b/drivers/gpu/drm/sun4i/sun4i_drv.c
index abc7d8fe06b4..a45a627283a1 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -25,12 +25,20 @@
 #include "sun4i_framebuffer.h"
 #include "sun4i_tcon.h"
 
+static void sun4i_drv_lastclose(struct drm_device *dev)
+{
+   struct sun4i_drv *drv = dev->dev_private;
+
+   drm_fbdev_cma_restore_mode(drv->fbdev);
+}
+
 DEFINE_DRM_GEM_CMA_FOPS(sun4i_drv_fops);
 
 static struct drm_driver sun4i_drv_driver = {
.driver_features= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | 
DRIVER_ATOMIC,
 
/* Generic Operations */
+   .lastclose  = sun4i_drv_lastclose,
.fops   = _drv_fops,
.name   = "sun4i-drm",
.desc   = "Allwinner sun4i Display Engine",
-- 
2.13.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/sun4i: Implement drm_driver lastclose to restore fbdev console

[Jonathan Liu]

The drm_driver lastclose callback is called when the last userspace
DRM client has closed. Call drm_fbdev_cma_restore_mode to restore
the fbdev console otherwise the fbdev console will stop working.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/gpu/drm/sun4i/sun4i_drv.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c 
b/drivers/gpu/drm/sun4i/sun4i_drv.c
index abc7d8fe06b4..c434540d409c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -25,12 +25,20 @@
 #include "sun4i_framebuffer.h"
 #include "sun4i_tcon.h"
 
+static void sun4i_drm_lastclose(struct drm_device *dev)
+{
+   struct sun4i_drv *drv = dev->dev_private;
+
+   drm_fbdev_cma_restore_mode(drv->fbdev);
+}
+
 DEFINE_DRM_GEM_CMA_FOPS(sun4i_drv_fops);
 
 static struct drm_driver sun4i_drv_driver = {
.driver_features= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | 
DRIVER_ATOMIC,
 
/* Generic Operations */
+   .lastclose  = sun4i_drm_lastclose,
.fops   = _drv_fops,
.name   = "sun4i-drm",
.desc   = "Allwinner sun4i Display Engine",
-- 
2.13.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm/sun4i: Implement drm_driver lastclose to restore fbdev console

[Jonathan Liu]

The drm_driver lastclose callback is called when the last userspace
DRM client has closed. Call drm_fbdev_cma_restore_mode to restore
the fbdev console otherwise the fbdev console will stop working.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
Changes for v2:
 - Rename sun4i_drm_lastclose to sun4i_drv_lastclose

 drivers/gpu/drm/sun4i/sun4i_drv.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c 
b/drivers/gpu/drm/sun4i/sun4i_drv.c
index abc7d8fe06b4..a45a627283a1 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -25,12 +25,20 @@
 #include "sun4i_framebuffer.h"
 #include "sun4i_tcon.h"
 
+static void sun4i_drv_lastclose(struct drm_device *dev)
+{
+   struct sun4i_drv *drv = dev->dev_private;
+
+   drm_fbdev_cma_restore_mode(drv->fbdev);
+}
+
 DEFINE_DRM_GEM_CMA_FOPS(sun4i_drv_fops);
 
 static struct drm_driver sun4i_drv_driver = {
.driver_features= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | 
DRIVER_ATOMIC,
 
/* Generic Operations */
+   .lastclose  = sun4i_drv_lastclose,
.fops   = _drv_fops,
.name   = "sun4i-drm",
.desc   = "Allwinner sun4i Display Engine",
-- 
2.13.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

Hi Chen-Yu and Maxime,

On 30 June 2017 at 13:16, Chen-Yu Tsai <w...@csie.org> wrote:
> On Fri, Jun 30, 2017 at 6:22 AM, Jonathan Liu <net...@gmail.com> wrote:
>> Hi Maxime,
>>
>> On 30 June 2017 at 01:56, Maxime Ripard
>> <maxime.rip...@free-electrons.com> wrote:
>>> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>>>> >> + u32 int_status;
>>>> >> + u32 fifo_status;
>>>> >> + /* Read needs empty flag unset, write needs full flag unset */
>>>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>>>> >> +   SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>>>> >> + int ret;
>>>> >> +
>>>> >> + /* Wait until error or FIFO ready */
>>>> >> + ret = readl_poll_timeout(hdmi->base + 
>>>> >> SUN4I_HDMI_DDC_INT_STATUS_REG,
>>>> >> +  int_status,
>>>> >> +  is_err_status(int_status) ||
>>>> >> +  is_fifo_flag_unset(hdmi, _status, 
>>>> >> flag),
>>>> >> +  min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * 
>>>> >> byte_time,
>>>> >> +  10);
>>>> >> +
>>>> >> + if (is_err_status(int_status))
>>>> >> + return -EIO;
>>>> >> + if (ret)
>>>> >> + return -ETIMEDOUT;
>>>> >
>>>> > Why not just have
>>>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, 
>>>> > reg,
>>>> >  !(reg & flag), 100, 10);
>>>> >
>>>> > if (ret < 0)
>>>> > if (is_err_status())
>>>> > return -EIO;
>>>> > return ret;
>>>> >
>>>> >
>>>>
>>>> If I check error status after readl_poll_timeout and there is an error
>>>> (e.g. the I2C address does not have a corresponding device connected
>>>> or nothing connected to HDMI port) it will keep checking the fifo
>>>> status even though error bit is set in the int status and then timeout
>>>> after 100 ms. If it checks the int status register at the same time,
>>>> it will error after 100 nanoseconds. I don't want to introduce
>>>> unnecessary delays considering part of the reason for adding this
>>>> driver to make it more usable for non-standard use cases.
>>>
>>> Well, polling for 100ms doesn't seem great either. What was the
>>> rationale behind that timeout?
>>>
>>
>> When an error occurs one of the error bits will be set in the
>> INT_STATUS register so this is detected very quickly if I check the
>> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
>> case the I2C slave does clock stretching in which case the transfer
>> may take longer than the predicted time.
>>
>>> And we can also reverse the check and look at the INT_STATUS
>>> register. The errors will be there, and we can program the threshold
>>> we want in both directions and use the
>>> DDC_FIFO_Request_Interrupt_Status bit.
>>>
>>
>> I did try that when I was doing the v3 patch but I couldn't get it to
>> work as mentioned previously in the v3 patch discussion. I programmed
>> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
>> register at the same time as setting FIFO_Address_Clear but the
>> request interrupt status bit did not get updated to the appropriate
>> state that is consistent with the FIFO level and the thresholds. I did
>> try this several times for subsequent patch versions without success.
>
> The manual says "When FIFO level is above this value in read mode, DMA
> request and FIFO request interrupt are asserted if relative enable is on."
>
> Perhaps try enabling the interrupts? But if that were the case, wouldn't
> using interrupts instead of polling be better?
>
> ChenYu
>

I managed to get the thresholds working so switching to using
interrupts instead of polling will be my next goal.

>>
>>> Maxime
>>>
>>> --
>>> Maxime Ripard, Free Electrons
>>> Embedded Linux and Kernel engineering
>>> http://free-electrons.com
>>
>> Regards,
>> Jonathan

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

Hi Maxime,

On 30 June 2017 at 19:58, Jonathan Liu <net...@gmail.com> wrote:
> Hi Maxime,
>
> On 30 June 2017 at 19:34, Maxime Ripard
> <maxime.rip...@free-electrons.com> wrote:
>> Hi,
>>
>> On Fri, Jun 30, 2017 at 08:22:13AM +1000, Jonathan Liu wrote:
>>> Hi Maxime,
>>>
>>> On 30 June 2017 at 01:56, Maxime Ripard
>>> <maxime.rip...@free-electrons.com> wrote:
>>> > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>>> >> >> + u32 int_status;
>>> >> >> + u32 fifo_status;
>>> >> >> + /* Read needs empty flag unset, write needs full flag unset */
>>> >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>>> >> >> +   SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>>> >> >> + int ret;
>>> >> >> +
>>> >> >> + /* Wait until error or FIFO ready */
>>> >> >> + ret = readl_poll_timeout(hdmi->base + 
>>> >> >> SUN4I_HDMI_DDC_INT_STATUS_REG,
>>> >> >> +  int_status,
>>> >> >> +  is_err_status(int_status) ||
>>> >> >> +  is_fifo_flag_unset(hdmi, _status, 
>>> >> >> flag),
>>> >> >> +  min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * 
>>> >> >> byte_time,
>>> >> >> +  10);
>>> >> >> +
>>> >> >> + if (is_err_status(int_status))
>>> >> >> + return -EIO;
>>> >> >> + if (ret)
>>> >> >> + return -ETIMEDOUT;
>>> >> >
>>> >> > Why not just have
>>> >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, 
>>> >> > reg,
>>> >> >  !(reg & flag), 100, 10);
>>> >> >
>>> >> > if (ret < 0)
>>> >> > if (is_err_status())
>>> >> > return -EIO;
>>> >> > return ret;
>>> >> >
>>> >> >
>>> >>
>>> >> If I check error status after readl_poll_timeout and there is an error
>>> >> (e.g. the I2C address does not have a corresponding device connected
>>> >> or nothing connected to HDMI port) it will keep checking the fifo
>>> >> status even though error bit is set in the int status and then timeout
>>> >> after 100 ms. If it checks the int status register at the same time,
>>> >> it will error after 100 nanoseconds. I don't want to introduce
>>> >> unnecessary delays considering part of the reason for adding this
>>> >> driver to make it more usable for non-standard use cases.
>>> >
>>> > Well, polling for 100ms doesn't seem great either. What was the
>>> > rationale behind that timeout?
>>> >
>>>
>>> When an error occurs one of the error bits will be set in the
>>> INT_STATUS register so this is detected very quickly if I check the
>>> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
>>> case the I2C slave does clock stretching in which case the transfer
>>> may take longer than the predicted time.
>>
>> 100ms isn't stretching anymore, it's worse than that.
>>
>
> What would you suggest?
>

You need to consider the fact that there are I2C devices such as
sensors that may do I2C clock stretching up to several tens of
milliseconds. For example the HTU21D humidity sensor takes up to 50 ms
max to perform measurement of temperature during which it is holding
down the clock to do I2C clock stretching.

>>> > And we can also reverse the check and look at the INT_STATUS
>>> > register. The errors will be there, and we can program the threshold
>>> > we want in both directions and use the
>>> > DDC_FIFO_Request_Interrupt_Status bit.
>>> >
>>>
>>> I did try that when I was doing the v3 patch but I couldn't get it to
>>> work as mentioned previously in the v3 patch discussion. I programmed
>>> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
>>> register at the same time as setting FIFO_Address_Clear but the
>>> request interrupt status bit did not get updated to the appropriate
>>> state that is consistent with the FIFO level and the thresholds. I did
>>> try this several times for subsequent patch versions without success.
>>
>> What values did you set it to?
>>
>
> FIFO_RX_TRIGGER_THRES: 0
> FIFO_TX_TRIGGER_THRES: 15
>
>> Maxime
>>
>> --
>> Maxime Ripard, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com
>
> Regards,
> Jonathan

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

Hi Maxime,

On 30 June 2017 at 19:34, Maxime Ripard
<maxime.rip...@free-electrons.com> wrote:
> Hi,
>
> On Fri, Jun 30, 2017 at 08:22:13AM +1000, Jonathan Liu wrote:
>> Hi Maxime,
>>
>> On 30 June 2017 at 01:56, Maxime Ripard
>> <maxime.rip...@free-electrons.com> wrote:
>> > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>> >> >> + u32 int_status;
>> >> >> + u32 fifo_status;
>> >> >> + /* Read needs empty flag unset, write needs full flag unset */
>> >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>> >> >> +   SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>> >> >> + int ret;
>> >> >> +
>> >> >> + /* Wait until error or FIFO ready */
>> >> >> + ret = readl_poll_timeout(hdmi->base + 
>> >> >> SUN4I_HDMI_DDC_INT_STATUS_REG,
>> >> >> +  int_status,
>> >> >> +  is_err_status(int_status) ||
>> >> >> +  is_fifo_flag_unset(hdmi, _status, 
>> >> >> flag),
>> >> >> +  min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * 
>> >> >> byte_time,
>> >> >> +  10);
>> >> >> +
>> >> >> + if (is_err_status(int_status))
>> >> >> + return -EIO;
>> >> >> + if (ret)
>> >> >> + return -ETIMEDOUT;
>> >> >
>> >> > Why not just have
>> >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, 
>> >> > reg,
>> >> >  !(reg & flag), 100, 10);
>> >> >
>> >> > if (ret < 0)
>> >> > if (is_err_status())
>> >> > return -EIO;
>> >> > return ret;
>> >> >
>> >> >
>> >>
>> >> If I check error status after readl_poll_timeout and there is an error
>> >> (e.g. the I2C address does not have a corresponding device connected
>> >> or nothing connected to HDMI port) it will keep checking the fifo
>> >> status even though error bit is set in the int status and then timeout
>> >> after 100 ms. If it checks the int status register at the same time,
>> >> it will error after 100 nanoseconds. I don't want to introduce
>> >> unnecessary delays considering part of the reason for adding this
>> >> driver to make it more usable for non-standard use cases.
>> >
>> > Well, polling for 100ms doesn't seem great either. What was the
>> > rationale behind that timeout?
>> >
>>
>> When an error occurs one of the error bits will be set in the
>> INT_STATUS register so this is detected very quickly if I check the
>> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
>> case the I2C slave does clock stretching in which case the transfer
>> may take longer than the predicted time.
>
> 100ms isn't stretching anymore, it's worse than that.
>

What would you suggest?

>> > And we can also reverse the check and look at the INT_STATUS
>> > register. The errors will be there, and we can program the threshold
>> > we want in both directions and use the
>> > DDC_FIFO_Request_Interrupt_Status bit.
>> >
>>
>> I did try that when I was doing the v3 patch but I couldn't get it to
>> work as mentioned previously in the v3 patch discussion. I programmed
>> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
>> register at the same time as setting FIFO_Address_Clear but the
>> request interrupt status bit did not get updated to the appropriate
>> state that is consistent with the FIFO level and the thresholds. I did
>> try this several times for subsequent patch versions without success.
>
> What values did you set it to?
>

FIFO_RX_TRIGGER_THRES: 0
FIFO_TX_TRIGGER_THRES: 15

> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v8] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
"As in the general case the DDC bus is accessible by the kernel at the I2C
level, drivers must make all reasonable efforts to expose it as an I2C
adapter and use drm_get_edid() instead of abusing this function."

Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
for purposes other than reading the EDID such as modifying the EDID or
using the HDMI DDC pins as an I2C bus through the I2C dev interface from
userspace (e.g. i2c-tools).

Implement this for A10s.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
Changes for v8:
 - Fix clock rate in comment being 100 MHz when it is actually 100 kHz
 - Fix clearing of bits in interrupt status register as they are cleared by
   writing 1 to the bit rather than 0
 - Set FIFO RX/TX thresholds so interrupt status register can be used to
   check if FIFO is ready instead of having to additionally check the FIFO
   status register
 - Remove unused linux/ktime.h include in sun4i_hdmi_i2c.c
 - Reduce timeout for clearing FIFO from 100 ms to 2 ms
 - Use BIT macro instead of GENMASK for SUN4I_HDMI_DDC_BYTE_COUNT_MAX to
   make it clearer that the maximum is derived from the field bit width rather
   than its position in the register

Changes for v7:
 - Fix mixed declarations and code compiler warning for level variable

Changes for v6:
 - Use fixed byte time of 100 us instead of dynamically calculating from DDC
   clock that is set to a fixed 100 MHz rate anyway
 - Change is_fifo_flag_unset to not read the status register as well to be
   more consistent with is_err_status

Changes for v5:
 - Use devm_kzalloc instead of devm_kmemdup and remove const struct i2c_adapter
 - Rework to use readl_poll_timeout for checking FIFO status

Changes for v4:
 - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c
 - Clean up indentation in sun4i_hdmi.h
 - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX
   and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the
   value to use the GENMASK macro to make it clear that it is derived from
   the width of the field in the register
 - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be
   SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW
 - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register
 - Change struct i2c_adapter to be const by using devm_kmemdup on creation
 - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an
   I2C message
 - Instead of waiting for 1-2 bytes to transfer, wait for the time it would
   take for remaining bytes to transfer (limited by FIFO size)
 - Add additional comments

Changes for v3:
 - Explain why drm_do_get_edid should be used and why it's better to expose it
   as an I2C adapter in commit message
 - Reorder bit defines in descending order for consistency
 - Keep old unused macros instead of removing them
 - The v2 algorithm split large transfers into 16 byte transfers but this may
   cause a large single write to be treated as multiple writes causing data
   corruption. The algorithm has been reworked to not split larger transfers
   and make more use of the FIFO to avoid this.
 - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
   sun4i_hdmi_i2c.c
 - Reformatted code
 - Instead of masking bits that we don't want to check for errors, explicitly
   check the error bits
 - Clear error bits at start of transfer in case of error from previous transfer
 - Poll for completion of FIFO clear after setting FIFO clear bit

Changes for v2:
 - Rebased against Maxime's sunxi-drm/for-next branch
 - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if
   any of the calls after the I2C adapter is created fails
 - Remove unnecessary includes in sun4i_hdmi_i2c.c

 drivers/gpu/drm/sun4i/Makefile |   1 +
 drivers/gpu/drm/sun4i/sun4i_hdmi.h |  24 
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++-
 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 220 +
 4 files changed, 256 insertions(+), 90 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c

diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index e29fd3a2ba9c..43c753cafc88 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
 sun4i-drm-y += sun4i_framebuffer.o
 
 sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
+sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
 sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
 sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h 
b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 2f2f2ff1ea63..0957ff2076ac 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -96,6 +96,7 @@
 #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31)
 #define SUN4I_HDMI_DDC_CTRL_START_CMD  

Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

Hi Maxime,

On 30 June 2017 at 01:56, Maxime Ripard
<maxime.rip...@free-electrons.com> wrote:
> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote:
>> >> + u32 int_status;
>> >> + u32 fifo_status;
>> >> + /* Read needs empty flag unset, write needs full flag unset */
>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>> >> +   SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>> >> + int ret;
>> >> +
>> >> + /* Wait until error or FIFO ready */
>> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>> >> +  int_status,
>> >> +  is_err_status(int_status) ||
>> >> +  is_fifo_flag_unset(hdmi, _status, 
>> >> flag),
>> >> +  min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * 
>> >> byte_time,
>> >> +  10);
>> >> +
>> >> + if (is_err_status(int_status))
>> >> + return -EIO;
>> >> + if (ret)
>> >> + return -ETIMEDOUT;
>> >
>> > Why not just have
>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>> >  !(reg & flag), 100, 10);
>> >
>> > if (ret < 0)
>> > if (is_err_status())
>> > return -EIO;
>> > return ret;
>> >
>> >
>>
>> If I check error status after readl_poll_timeout and there is an error
>> (e.g. the I2C address does not have a corresponding device connected
>> or nothing connected to HDMI port) it will keep checking the fifo
>> status even though error bit is set in the int status and then timeout
>> after 100 ms. If it checks the int status register at the same time,
>> it will error after 100 nanoseconds. I don't want to introduce
>> unnecessary delays considering part of the reason for adding this
>> driver to make it more usable for non-standard use cases.
>
> Well, polling for 100ms doesn't seem great either. What was the
> rationale behind that timeout?
>

When an error occurs one of the error bits will be set in the
INT_STATUS register so this is detected very quickly if I check the
INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in
case the I2C slave does clock stretching in which case the transfer
may take longer than the predicted time.

> And we can also reverse the check and look at the INT_STATUS
> register. The errors will be there, and we can program the threshold
> we want in both directions and use the
> DDC_FIFO_Request_Interrupt_Status bit.
>

I did try that when I was doing the v3 patch but I couldn't get it to
work as mentioned previously in the v3 patch discussion. I programmed
the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl
register at the same time as setting FIFO_Address_Clear but the
request interrupt status bit did not get updated to the appropriate
state that is consistent with the FIFO level and the thresholds. I did
try this several times for subsequent patch versions without success.

> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v7] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

Hi Chen-Yu,

On 29 June 2017 at 12:47, Chen-Yu Tsai <w...@csie.org> wrote:
> Hi,
>
> On Wed, Jun 28, 2017 at 6:52 PM, Jonathan Liu <net...@gmail.com> wrote:
>> The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
>> "As in the general case the DDC bus is accessible by the kernel at the I2C
>> level, drivers must make all reasonable efforts to expose it as an I2C
>> adapter and use drm_get_edid() instead of abusing this function."
>>
>> Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
>> for purposes other than reading the EDID such as modifying the EDID or
>> using the HDMI DDC pins as an I2C bus through the I2C dev interface from
>> userspace (e.g. i2c-tools).
>>
>> Implement this for A10s.
>>
>> Signed-off-by: Jonathan Liu <net...@gmail.com>
>> ---
>> Changes for v7:
>>  - Fix mixed declarations and code compiler warning for level variable
>>
>> Changes for v6:
>>  - Use fixed byte time of 100 us instead of dynamically calculating from DDC
>>clock that is set to a fixed 100 MHz rate anyway
>>  - Change is_fifo_flag_unset to not read the status register as well to be
>>more consistent with is_err_status
>>
>> Changes for v5:
>>  - Use devm_kzalloc instead of devm_kmemdup and remove const struct 
>> i2c_adapter
>>  - Rework to use readl_poll_timeout for checking FIFO status
>>
>> Changes for v4:
>>  - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c
>>  - Clean up indentation in sun4i_hdmi.h
>>  - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX
>>and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the
>>value to use the GENMASK macro to make it clear that it is derived from
>>the width of the field in the register
>>  - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be
>>SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW
>>  - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register
>>  - Change struct i2c_adapter to be const by using devm_kmemdup on creation
>>  - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring 
>> an
>>I2C message
>>  - Instead of waiting for 1-2 bytes to transfer, wait for the time it would
>>take for remaining bytes to transfer (limited by FIFO size)
>>  - Add additional comments
>>
>> Changes for v3:
>>  - Explain why drm_do_get_edid should be used and why it's better to expose 
>> it
>>as an I2C adapter in commit message
>>  - Reorder bit defines in descending order for consistency
>>  - Keep old unused macros instead of removing them
>>  - The v2 algorithm split large transfers into 16 byte transfers but this may
>>cause a large single write to be treated as multiple writes causing data
>>corruption. The algorithm has been reworked to not split larger transfers
>>and make more use of the FIFO to avoid this.
>>  - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
>>sun4i_hdmi_i2c.c
>>  - Reformatted code
>>  - Instead of masking bits that we don't want to check for errors, explicitly
>>check the error bits
>>  - Clear error bits at start of transfer in case of error from previous 
>> transfer
>>  - Poll for completion of FIFO clear after setting FIFO clear bit
>>
>> Changes for v2:
>>  - Rebased against Maxime's sunxi-drm/for-next branch
>>  - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted 
>> if
>>any of the calls after the I2C adapter is created fails
>>  - Remove unnecessary includes in sun4i_hdmi_i2c.c
>>
>>  drivers/gpu/drm/sun4i/Makefile |   1 +
>>  drivers/gpu/drm/sun4i/sun4i_hdmi.h |  23 
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 234 
>> +
>>  4 files changed, 269 insertions(+), 90 deletions(-)
>>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
>>
>
> [...]
>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c 
>> b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
>> new file mode 100644
>> index ..ce954ee25ae4
>> --- /dev/null
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
>> @@ -0,0 +1,234 @@
>
> [...]
>
>> +static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool 
>> read)
>> +{
>> +   /*
>> +* 1 byte takes 9 clock cycles (8 bits + 1 ACK) = 90 us for 100 MHz
>

Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

Hi Maxime,

On 28 June 2017 at 19:20, Maxime Ripard
<maxime.rip...@free-electrons.com> wrote:
> On Wed, Jun 28, 2017 at 12:36:52AM +1000, Jonathan Liu wrote:
>> +#define SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK ( \
>> + SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION | \
>> + SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW | \
>> + SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW | \
>> + SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \
>> + SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \
>> + SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \
>> +)
>> +
>> +static bool is_err_status(u32 int_status)
>> +{
>> + return !!(int_status & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK);
>> +}
>> +
>> +static bool is_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 *fifo_status,
>> +u32 flag)
>> +{
>> + *fifo_status = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
>> + return !(*fifo_status & flag);
>> +}
>> +
>> +static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool 
>> read)
>> +{
>> + /* 1 byte takes 9 clock cycles (8 bits + 1 ACK) */
>> + unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC,
>> +clk_get_rate(hdmi->ddc_clk)) * 
>> 9;
>
> There's no real need for it to be dynamic. The clock rate will not
> change, and the order of magnitude is roughly 100us, so let's just use
> that (and make a comment).
>

Ok.

>> + u32 int_status;
>> + u32 fifo_status;
>> + /* Read needs empty flag unset, write needs full flag unset */
>> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY :
>> +   SUN4I_HDMI_DDC_FIFO_STATUS_FULL;
>> + int ret;
>> +
>> + /* Wait until error or FIFO ready */
>> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG,
>> +  int_status,
>> +  is_err_status(int_status) ||
>> +  is_fifo_flag_unset(hdmi, _status, flag),
>> +  min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * 
>> byte_time,
>> +  10);
>> +
>> + if (is_err_status(int_status))
>> + return -EIO;
>> + if (ret)
>> + return -ETIMEDOUT;
>
> Why not just have
> ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg,
>  !(reg & flag), 100, 10);
>
> if (ret < 0)
> if (is_err_status())
> return -EIO;
> return ret;
>
>

If I check error status after readl_poll_timeout and there is an error
(e.g. the I2C address does not have a corresponding device connected
or nothing connected to HDMI port) it will keep checking the fifo
status even though error bit is set in the int status and then timeout
after 100 ms. If it checks the int status register at the same time,
it will error after 100 nanoseconds. I don't want to introduce
unnecessary delays considering part of the reason for adding this
driver to make it more usable for non-standard use cases.

>> +
>> + /* Read FIFO level */
>> + int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK);
>
> and explicitly read the fifo status here. That will make you remove
> that function that does two things while claiming that it does only
> one, and it will be more obvious.
>

I will fix the is_fifo_flag_unset function so it only does one thing.

> You can also just use reg at this point, instead of reading it once
> again.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
"As in the general case the DDC bus is accessible by the kernel at the I2C
level, drivers must make all reasonable efforts to expose it as an I2C
adapter and use drm_get_edid() instead of abusing this function."

Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
for purposes other than reading the EDID such as modifying the EDID or
using the HDMI DDC pins as an I2C bus through the I2C dev interface from
userspace (e.g. i2c-tools).

Implement this for A10s.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
Changes for v5:
 - Use devm_kzalloc instead of devm_kmemdup and remove const struct i2c_adapter
 - Rework to use readl_poll_timeout for checking FIFO status

Changes for v4:
 - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c
 - Clean up indentation in sun4i_hdmi.h
 - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX
   and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the
   value to use the GENMASK macro to make it clear that it is derived from
   the width of the field in the register
 - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be
   SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW
 - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register
 - Change struct i2c_adapter to be const by using devm_kmemdup on creation
 - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an
   I2C message
 - Instead of waiting for 1-2 bytes to transfer, wait for the time it would
   take for remaining bytes to transfer (limited by FIFO size)
 - Add additional comments

Changes for v3:
 - Explain why drm_do_get_edid should be used and why it's better to expose it
   as an I2C adapter in commit message
 - Reorder bit defines in descending order for consistency
 - Keep old unused macros instead of removing them
 - The v2 algorithm split large transfers into 16 byte transfers but this may
   cause a large single write to be treated as multiple writes causing data
   corruption. The algorithm has been reworked to not split larger transfers
   and make more use of the FIFO to avoid this.
 - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
   sun4i_hdmi_i2c.c
 - Reformatted code
 - Instead of masking bits that we don't want to check for errors, explicitly
   check the error bits
 - Clear error bits at start of transfer in case of error from previous transfer
 - Poll for completion of FIFO clear after setting FIFO clear bit

Changes for v2:
 - Rebased against Maxime's sunxi-drm/for-next branch
 - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if
   any of the calls after the I2C adapter is created fails
 - Remove unnecessary includes in sun4i_hdmi_i2c.c

 drivers/gpu/drm/sun4i/Makefile |   1 +
 drivers/gpu/drm/sun4i/sun4i_hdmi.h |  23 
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++-
 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 226 +
 4 files changed, 261 insertions(+), 90 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c

diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index e29fd3a2ba9c..43c753cafc88 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
 sun4i-drm-y += sun4i_framebuffer.o
 
 sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
+sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
 sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
 sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h 
b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 2f2f2ff1ea63..eaff92fe236c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -96,6 +96,7 @@
 #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31)
 #define SUN4I_HDMI_DDC_CTRL_START_CMD  BIT(30)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK  BIT(8)
+#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ  (0 << 8)
 #define SUN4I_HDMI_DDC_CTRL_RESET  BIT(0)
 
@@ -105,14 +106,33 @@
 #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8)
 #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff)
 
+#define SUN4I_HDMI_DDC_INT_STATUS_REG  0x50c
+#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION   BIT(7)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOWBIT(6)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW BIT(5)
+#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST BIT(4)
+#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERRORBIT(3)
+#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERRORBIT(2)
+#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERRORBIT(1)
+#define SUN4I_HDMI_DDC_INT

[PATCH v6] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
"As in the general case the DDC bus is accessible by the kernel at the I2C
level, drivers must make all reasonable efforts to expose it as an I2C
adapter and use drm_get_edid() instead of abusing this function."

Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
for purposes other than reading the EDID such as modifying the EDID or
using the HDMI DDC pins as an I2C bus through the I2C dev interface from
userspace (e.g. i2c-tools).

Implement this for A10s.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
Changes for v6:
 - Use fixed byte time of 100 us instead of dynamically calculating from DDC
   clock that is set to a fixed 100 MHz rate anyway
 - Change is_fifo_flag_unset to not read the status register as well to be
   more consistent with is_err_status

Changes for v5:
 - Use devm_kzalloc instead of devm_kmemdup and remove const struct i2c_adapter
 - Rework to use readl_poll_timeout for checking FIFO status

Changes for v4:
 - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c
 - Clean up indentation in sun4i_hdmi.h
 - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX
   and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the
   value to use the GENMASK macro to make it clear that it is derived from
   the width of the field in the register
 - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be
   SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW
 - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register
 - Change struct i2c_adapter to be const by using devm_kmemdup on creation
 - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an
   I2C message
 - Instead of waiting for 1-2 bytes to transfer, wait for the time it would
   take for remaining bytes to transfer (limited by FIFO size)
 - Add additional comments

Changes for v3:
 - Explain why drm_do_get_edid should be used and why it's better to expose it
   as an I2C adapter in commit message
 - Reorder bit defines in descending order for consistency
 - Keep old unused macros instead of removing them
 - The v2 algorithm split large transfers into 16 byte transfers but this may
   cause a large single write to be treated as multiple writes causing data
   corruption. The algorithm has been reworked to not split larger transfers
   and make more use of the FIFO to avoid this.
 - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
   sun4i_hdmi_i2c.c
 - Reformatted code
 - Instead of masking bits that we don't want to check for errors, explicitly
   check the error bits
 - Clear error bits at start of transfer in case of error from previous transfer
 - Poll for completion of FIFO clear after setting FIFO clear bit

Changes for v2:
 - Rebased against Maxime's sunxi-drm/for-next branch
 - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if
   any of the calls after the I2C adapter is created fails
 - Remove unnecessary includes in sun4i_hdmi_i2c.c

 drivers/gpu/drm/sun4i/Makefile |   1 +
 drivers/gpu/drm/sun4i/sun4i_hdmi.h |  23 
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++
 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 233 +
 4 files changed, 268 insertions(+), 90 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c

diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index e29fd3a2ba9c..43c753cafc88 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
 sun4i-drm-y += sun4i_framebuffer.o
 
 sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
+sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
 sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
 sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h 
b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 2f2f2ff1ea63..eaff92fe236c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -96,6 +96,7 @@
 #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31)
 #define SUN4I_HDMI_DDC_CTRL_START_CMD  BIT(30)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK  BIT(8)
+#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ  (0 << 8)
 #define SUN4I_HDMI_DDC_CTRL_RESET  BIT(0)
 
@@ -105,14 +106,33 @@
 #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8)
 #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff)
 
+#define SUN4I_HDMI_DDC_INT_STATUS_REG  0x50c
+#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION   BIT(7)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOWBIT(6)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW BIT(5)
+#define SUN4I_HDMI_DDC_INT_STATUS_FI

[PATCH v7] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
"As in the general case the DDC bus is accessible by the kernel at the I2C
level, drivers must make all reasonable efforts to expose it as an I2C
adapter and use drm_get_edid() instead of abusing this function."

Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
for purposes other than reading the EDID such as modifying the EDID or
using the HDMI DDC pins as an I2C bus through the I2C dev interface from
userspace (e.g. i2c-tools).

Implement this for A10s.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
Changes for v7:
 - Fix mixed declarations and code compiler warning for level variable

Changes for v6:
 - Use fixed byte time of 100 us instead of dynamically calculating from DDC
   clock that is set to a fixed 100 MHz rate anyway
 - Change is_fifo_flag_unset to not read the status register as well to be
   more consistent with is_err_status

Changes for v5:
 - Use devm_kzalloc instead of devm_kmemdup and remove const struct i2c_adapter
 - Rework to use readl_poll_timeout for checking FIFO status

Changes for v4:
 - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c
 - Clean up indentation in sun4i_hdmi.h
 - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX
   and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the
   value to use the GENMASK macro to make it clear that it is derived from
   the width of the field in the register
 - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be
   SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW
 - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register
 - Change struct i2c_adapter to be const by using devm_kmemdup on creation
 - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an
   I2C message
 - Instead of waiting for 1-2 bytes to transfer, wait for the time it would
   take for remaining bytes to transfer (limited by FIFO size)
 - Add additional comments

Changes for v3:
 - Explain why drm_do_get_edid should be used and why it's better to expose it
   as an I2C adapter in commit message
 - Reorder bit defines in descending order for consistency
 - Keep old unused macros instead of removing them
 - The v2 algorithm split large transfers into 16 byte transfers but this may
   cause a large single write to be treated as multiple writes causing data
   corruption. The algorithm has been reworked to not split larger transfers
   and make more use of the FIFO to avoid this.
 - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
   sun4i_hdmi_i2c.c
 - Reformatted code
 - Instead of masking bits that we don't want to check for errors, explicitly
   check the error bits
 - Clear error bits at start of transfer in case of error from previous transfer
 - Poll for completion of FIFO clear after setting FIFO clear bit

Changes for v2:
 - Rebased against Maxime's sunxi-drm/for-next branch
 - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if
   any of the calls after the I2C adapter is created fails
 - Remove unnecessary includes in sun4i_hdmi_i2c.c

 drivers/gpu/drm/sun4i/Makefile |   1 +
 drivers/gpu/drm/sun4i/sun4i_hdmi.h |  23 
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++
 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 234 +
 4 files changed, 269 insertions(+), 90 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c

diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index e29fd3a2ba9c..43c753cafc88 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
 sun4i-drm-y += sun4i_framebuffer.o
 
 sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
+sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
 sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
 sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h 
b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 2f2f2ff1ea63..eaff92fe236c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -96,6 +96,7 @@
 #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31)
 #define SUN4I_HDMI_DDC_CTRL_START_CMD  BIT(30)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK  BIT(8)
+#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ  (0 << 8)
 #define SUN4I_HDMI_DDC_CTRL_RESET  BIT(0)
 
@@ -105,14 +106,33 @@
 #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8)
 #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff)
 
+#define SUN4I_HDMI_DDC_INT_STATUS_REG  0x50c
+#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION   BIT(7)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOWBIT(6)
+#define SUN4I_HDMI_DDC_INT_STATUS_DD

Re: [PATCH v4] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

Hi Maxime,

On 27 June 2017 at 05:05, Maxime Ripard
<maxime.rip...@free-electrons.com> wrote:
> On Sat, Jun 24, 2017 at 04:10:54PM +1000, Jonathan Liu wrote:
>> The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
>> "As in the general case the DDC bus is accessible by the kernel at the I2C
>> level, drivers must make all reasonable efforts to expose it as an I2C
>> adapter and use drm_get_edid() instead of abusing this function."
>>
>> Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
>> for purposes other than reading the EDID such as modifying the EDID or
>> using the HDMI DDC pins as an I2C bus through the I2C dev interface from
>> userspace (e.g. i2c-tools).
>>
>> Implement this for A10s.
>>
>> Signed-off-by: Jonathan Liu <net...@gmail.com>
>> ---
>> Changes for v4:
>>  - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c
>>  - Clean up indentation in sun4i_hdmi.h
>>  - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX
>>and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the
>>value to use the GENMASK macro to make it clear that it is derived from
>>the width of the field in the register
>>  - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be
>>SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW
>>  - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register
>>  - Change struct i2c_adapter to be const by using devm_kmemdup on creation
>>  - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring 
>> an
>>I2C message
>>  - Instead of waiting for 1-2 bytes to transfer, wait for the time it would
>>take for remaining bytes to transfer (limited by FIFO size)
>>  - Add additional comments
>>
>> Changes for v3:
>>  - Explain why drm_do_get_edid should be used and why it's better to expose 
>> it
>>as an I2C adapter in commit message
>>  - Reorder bit defines in descending order for consistency
>>  - Keep old unused macros instead of removing them
>>  - The v2 algorithm split large transfers into 16 byte transfers but this may
>>cause a large single write to be treated as multiple writes causing data
>>corruption. The algorithm has been reworked to not split larger transfers
>>and make more use of the FIFO to avoid this.
>>  - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
>>sun4i_hdmi_i2c.c
>>  - Reformatted code
>>  - Instead of masking bits that we don't want to check for errors, explicitly
>>check the error bits
>>  - Clear error bits at start of transfer in case of error from previous 
>> transfer
>>  - Poll for completion of FIFO clear after setting FIFO clear bit
>>
>> Changes for v2:
>>  - Rebased against Maxime's sunxi-drm/for-next branch
>>  - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted 
>> if
>>any of the calls after the I2C adapter is created fails
>>  - Remove unnecessary includes in sun4i_hdmi_i2c.c
>>
>>  drivers/gpu/drm/sun4i/Makefile |   1 +
>>  drivers/gpu/drm/sun4i/sun4i_hdmi.h |  23 
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 242 
>> +
>>  4 files changed, 277 insertions(+), 90 deletions(-)
>>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
>>
>> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
>> index e29fd3a2ba9c..43c753cafc88 100644
>> --- a/drivers/gpu/drm/sun4i/Makefile
>> +++ b/drivers/gpu/drm/sun4i/Makefile
>> @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
>>  sun4i-drm-y += sun4i_framebuffer.o
>>
>>  sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
>> +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
>>  sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
>>  sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h 
>> b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> index 2f2f2ff1ea63..eaff92fe236c 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> @@ -96,6 +96,7 @@
>>  #define SUN4I_HDMI_DDC_CTRL_ENABLE   BIT(31)
>>  #define SUN4I_HDMI_DDC_CTRL_START_CMDBIT(30)
>>  #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASKBIT(8)
>> +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE   (1 << 8)
>>  #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ(0 << 8)
>>  #de

[PATCH v4] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
"As in the general case the DDC bus is accessible by the kernel at the I2C
level, drivers must make all reasonable efforts to expose it as an I2C
adapter and use drm_get_edid() instead of abusing this function."

Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
for purposes other than reading the EDID such as modifying the EDID or
using the HDMI DDC pins as an I2C bus through the I2C dev interface from
userspace (e.g. i2c-tools).

Implement this for A10s.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
Changes for v4:
 - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c
 - Clean up indentation in sun4i_hdmi.h
 - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX
   and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the
   value to use the GENMASK macro to make it clear that it is derived from
   the width of the field in the register
 - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be
   SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW
 - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register
 - Change struct i2c_adapter to be const by using devm_kmemdup on creation
 - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an
   I2C message
 - Instead of waiting for 1-2 bytes to transfer, wait for the time it would
   take for remaining bytes to transfer (limited by FIFO size)
 - Add additional comments

Changes for v3:
 - Explain why drm_do_get_edid should be used and why it's better to expose it
   as an I2C adapter in commit message
 - Reorder bit defines in descending order for consistency
 - Keep old unused macros instead of removing them
 - The v2 algorithm split large transfers into 16 byte transfers but this may
   cause a large single write to be treated as multiple writes causing data
   corruption. The algorithm has been reworked to not split larger transfers
   and make more use of the FIFO to avoid this.
 - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
   sun4i_hdmi_i2c.c
 - Reformatted code
 - Instead of masking bits that we don't want to check for errors, explicitly
   check the error bits
 - Clear error bits at start of transfer in case of error from previous transfer
 - Poll for completion of FIFO clear after setting FIFO clear bit

Changes for v2:
 - Rebased against Maxime's sunxi-drm/for-next branch
 - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if
   any of the calls after the I2C adapter is created fails
 - Remove unnecessary includes in sun4i_hdmi_i2c.c

 drivers/gpu/drm/sun4i/Makefile |   1 +
 drivers/gpu/drm/sun4i/sun4i_hdmi.h |  23 
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++
 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 242 +
 4 files changed, 277 insertions(+), 90 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c

diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index e29fd3a2ba9c..43c753cafc88 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
 sun4i-drm-y += sun4i_framebuffer.o
 
 sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
+sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
 sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
 sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h 
b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 2f2f2ff1ea63..eaff92fe236c 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -96,6 +96,7 @@
 #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31)
 #define SUN4I_HDMI_DDC_CTRL_START_CMD  BIT(30)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK  BIT(8)
+#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ  (0 << 8)
 #define SUN4I_HDMI_DDC_CTRL_RESET  BIT(0)
 
@@ -105,14 +106,33 @@
 #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8)
 #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff)
 
+#define SUN4I_HDMI_DDC_INT_STATUS_REG  0x50c
+#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION   BIT(7)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOWBIT(6)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW BIT(5)
+#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST BIT(4)
+#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERRORBIT(3)
+#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERRORBIT(2)
+#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERRORBIT(1)
+#define SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETEBIT(0)
+
 #define SUN4I_HDMI_DDC_FIFO_CTRL_REG   0x510
 #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31)
 
+#define SUN

Re: [PATCH v3] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

Hi Maxime,

On 22 June 2017 at 07:26, Maxime Ripard
<maxime.rip...@free-electrons.com> wrote:
> On Wed, Jun 21, 2017 at 07:42:47PM +1000, Jonathan Liu wrote:
>> >> +static int wait_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 flag)
>> >> +{
>> >> + /* 1 byte takes 9 clock cycles (8 bits + 1 ack) */
>> >> + unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC,
>> >> +clk_get_rate(hdmi->ddc_clk)) 
>> >> * 9;
>> >> + ktime_t wait_timeout = ktime_add_us(ktime_get(), 10);
>> >> + u32 reg;
>> >> +
>> >> + for (;;) {
>> >> + /* Check for errors */
>> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
>> >> + if (reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) {
>> >> + writel(reg, hdmi->base + 
>> >> SUN4I_HDMI_DDC_INT_STATUS_REG);
>> >> + return -EIO;
>> >> + }
>> >> +
>> >> + /* Check if requested FIFO flag is unset */
>> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG);
>> >> + if (!(reg & flag))
>> >> + return 0;
>> >> +
>> >> + /* Timeout */
>> >> + if (ktime_compare(ktime_get(), wait_timeout) > 0)
>> >> + return -EIO;
>> >> +
>> >> + /* Wait for 1-2 bytes to transfer */
>> >> + usleep_range(byte_time, 2 * byte_time);
>> >> + }
>> >> +
>> >> + return -EIO;
>> >> +}
>> >> +
>> >> +static int wait_fifo_read_ready(struct sun4i_hdmi *hdmi)
>> >> +{
>> >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY);
>> >> +}
>> >> +
>> >> +static int wait_fifo_write_ready(struct sun4i_hdmi *hdmi)
>> >> +{
>> >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_FULL);
>> >> +}
>> >
>> > Both of these can use readl_poll_timeout, and I'm not sure you need to
>> > be that aggressive in your timings.
>> >
>> I have set my timings to minimize communication delays - e.g. when
>> userspace is reading from I2C one byte at a time (like i2cdump from
>> i2c-tools).
>
> I wouldn't try to optimise that too much. There's already a lot of
> overhead, it's way inefficient, and it's not really a significant use
> case anyway.
>

Ok.

>> readl_poll_timeout can't be used to check 2 registers at
>> once and I couldn't get DDC_FIFO_Request_Interrupt_Status_Bit in
>> DDC_Int_Status register to behave properly.
>
> Can't you just use readl_poll_timeout, and then if it timed out check
> for errors?
>

I think it is more flexible this way as I can adjust the usleep_range min.

>> I also wanted to minimize the chance of FIFO underrun/overrun.
>>
>> >> +
>> >> +static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg)
>> >> +{
>> >> + u32 reg;
>> >> + int i;
>> >> +
>> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
>> >> + reg &= ~SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK;
>> >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG);
>> >> +
>> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> >> + reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK;
>> >> + reg |= (msg->flags & I2C_M_RD) ?
>> >> +SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ :
>> >> +SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE;
>> >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> >> +
>> >> + writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->addr),
>> >> +hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
>> >> +
>> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
>> >> + writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
>> >> +hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
>> >> +
>> >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG,
>> >> +reg,
>> >> +!(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR),
>> >> +100, 10))
>> >

Re: [PATCH v2] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

Hi Maxime,

On 21 June 2017 at 18:41, Maxime Ripard
<maxime.rip...@free-electrons.com> wrote:
> On Tue, Jun 13, 2017 at 09:53:31PM +1000, Jonathan Liu wrote:
>> >> --- /dev/null
>> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
>> >> @@ -0,0 +1,163 @@
>> >> +/*
>> >> + * Copyright (C) 2017 Jonathan Liu
>> >> + *
>> >> + * Jonathan Liu <net...@gmail.com>
>> >
>> > Is it?
>>
>> I could add you to the copyright since you did the old one. But the
>> implementation is different.
>> I intend to rework this I2C driver to use the FIFO more to avoid
>> splitting larger transfers > 16 bytes and do the transfer in a single
>> command. Would you like to be added to the copyright?
>
> You took some code that you didn't create, and added some more
> stuff. The copyright on the initial code remains, and it has nothing
> to do with whether the author wants it or not, (s)he should be
> mentionned, along with you of course.
>
I will update the copyright header accordingly.

>> >> +? SUN4I_HDMI_DDC_CMD_IMPLICIT_READ
>> >> +: SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE,
>> >> +hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
>> >> +
>> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> >> + writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
>> >> +hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
>> >> +
>> >> + if (!(msg->flags & I2C_M_RD)) {
>> >> + for (i = 0; i < count; i++) {
>> >> + writeb(*msg->buf++, hdmi->base
>> >> ++ SUN4I_HDMI_DDC_FIFO_DATA_REG);
>> >
>> > writesb?
>> I intend to rework the FIFO handling so will need to continue using writeb.
>
> Then you'll change it when you'll rework it. Before then, you can use
> writesb.
>
I have already reworked it in V3.

>> >
>> >> + --msg->len;
>> >> + }
>> >> + }
>> >> +
>> >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG,
>> >> +reg,
>> >> +!(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
>> >> +100, 10))
>> >> + return -EIO;
>> >> +
>> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INTERRUPT_STATUS_REG);
>> >> + reg &= ~SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST;
>> I want to check all the other bits that are set if there are errors.
>
> Same thing here: you'll change it when that happens.
I have already reworked it in V3.

>
>> >
>> > You don't need to use that mask.
>> >
>> >> + if (reg != SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE)
>> >> + return -EIO;
>> >
>> > !(reg & SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE) would be enough.
>> I want to check all the other bits that are set if there are errors.
>
> Same thing here.
I have already reworked it in V3.

>
>> >
>> >> +
>> >> + if (msg->flags & I2C_M_RD) {
>> >> + for (i = 0; i < count; i++) {
>> >> + *msg->buf++ = readb(hdmi->base
>> >> + + SUN4I_HDMI_DDC_FIFO_DATA_REG);
>> >
>> > readsb ?
>> I am reworking the FIFO handling so I will need to continue to use readb.
>
> Same thing here.
I have already reworked it in V3.

>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Regards,
Jonathan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

Hi Maxime,

On 21 June 2017 at 18:51, Maxime Ripard
<maxime.rip...@free-electrons.com> wrote:
> On Thu, Jun 15, 2017 at 01:29:33AM +1000, Jonathan Liu wrote:
>> The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
>> "As in the general case the DDC bus is accessible by the kernel at the I2C
>> level, drivers must make all reasonable efforts to expose it as an I2C
>> adapter and use drm_get_edid() instead of abusing this function."
>>
>> Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
>> for purposes other than reading the EDID such as modifying the EDID or
>> using the HDMI DDC pins as an I2C bus through the I2C dev interface from
>> userspace (e.g. i2c-tools).
>>
>> Implement this for A10s.
>>
>> Signed-off-by: Jonathan Liu <net...@gmail.com>
>> ---
>> Changes for v3:
>>  - Explain why drm_do_get_edid should be used and why it's better to expose 
>> it
>>as an I2C adapter in commit message
>>  - Reorder bit defines in descending order for consistency
>>  - Keep old unused macros instead of removing them
>>  - The v2 algorithm split large transfers into 16 byte transfers but this may
>>cause a large single write to be treated as multiple writes causing data
>>corruption. The algorithm has been reworked to not split larger transfers
>>and make more use of the FIFO to avoid this.
>>  - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
>>sun4i_hdmi_i2c.c
>>  - Reformatted code
>>  - Instead of masking bits that we don't want to check for errors, explicitly
>>check the error bits
>>  - Clear error bits at start of transfer in case of error from previous 
>> transfer
>>  - Poll for completion of FIFO clear after setting FIFO clear bit
>>
>> Changes for v2:
>>  - Rebased against Maxime's sunxi-drm/for-next branch
>>  - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted 
>> if
>>any of the calls after the I2C adapter is created fails
>>  - Remove unnecessary includes in sun4i_hdmi_i2c.c
>>
>>  drivers/gpu/drm/sun4i/Makefile |   1 +
>>  drivers/gpu/drm/sun4i/sun4i_hdmi.h |  21 
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++-
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 220 
>> +
>>  4 files changed, 253 insertions(+), 90 deletions(-)
>>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
>>
>> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
>> index e29fd3a2ba9c..43c753cafc88 100644
>> --- a/drivers/gpu/drm/sun4i/Makefile
>> +++ b/drivers/gpu/drm/sun4i/Makefile
>> @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
>>  sun4i-drm-y += sun4i_framebuffer.o
>>
>>  sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
>> +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
>>  sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
>>  sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h 
>> b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> index 2f2f2ff1ea63..018af9cbb593 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> @@ -96,6 +96,7 @@
>>  #define SUN4I_HDMI_DDC_CTRL_ENABLE   BIT(31)
>>  #define SUN4I_HDMI_DDC_CTRL_START_CMDBIT(30)
>>  #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASKBIT(8)
>> +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE   (1 << 8)
>>  #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ(0 << 8)
>>  #define SUN4I_HDMI_DDC_CTRL_RESETBIT(0)
>>
>> @@ -105,14 +106,30 @@
>>  #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)  (((off) & 0xff) << 8)
>>  #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)  ((addr) & 0xff)
>>
>> +#define SUN4I_HDMI_DDC_INT_STATUS_REG0x50c
>> +#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION BIT(7)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW  BIT(6)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW  BIT(5)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST   BIT(4)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR  BIT(3)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR  BIT(2)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR  BIT(1)
>> +#define SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE  BIT(0)
>> +
>>  #define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510
>>  #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR   BIT(31)
>>
>> +#define SUN4I_HDMI_DDC

[PATCH] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
"As in the general case the DDC bus is accessible by the kernel at the I2C
level, drivers must make all reasonable efforts to expose it as an I2C
adapter and use drm_get_edid() instead of abusing this function."

Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
for purposes other than reading the EDID such as modifying the EDID or
using the HDMI DDC pins as an I2C bus through the I2C dev interface from
userspace (e.g. i2c-tools).

Implement this for A10s.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
Changes for v3:
 - Explain why drm_do_get_edid should be used and why it's better to expose it
   as an I2C adapter in commit message
 - Reorder bit defines in descending order for consistency
 - Keep old unused macros instead of removing them
 - The v2 algorithm split large transfers into 16 byte transfers but this may
   cause a large single write to be treated as multiple writes causing data
   corruption. The algorithm has been reworked to not split larger transfers
   and make more use of the FIFO to avoid this.
 - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
   sun4i_hdmi_i2c.c
 - Reformatted code
 - Instead of masking bits that we don't want to check for errors, explicitly
   check the error bits
 - Clear error bits at start of transfer in case of error from previous transfer
 - Poll for completion of FIFO clear after setting FIFO clear bit

Changes for v2:
 - Rebased against Maxime's sunxi-drm/for-next branch
 - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if
   any of the calls after the I2C adapter is created fails
 - Remove unnecessary includes in sun4i_hdmi_i2c.c

 drivers/gpu/drm/sun4i/Makefile |   1 +
 drivers/gpu/drm/sun4i/sun4i_hdmi.h |  21 
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++-
 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 220 +
 4 files changed, 253 insertions(+), 90 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c

diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index e29fd3a2ba9c..43c753cafc88 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
 sun4i-drm-y += sun4i_framebuffer.o
 
 sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
+sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
 sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
 sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h 
b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 2f2f2ff1ea63..018af9cbb593 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -96,6 +96,7 @@
 #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31)
 #define SUN4I_HDMI_DDC_CTRL_START_CMD  BIT(30)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK  BIT(8)
+#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ  (0 << 8)
 #define SUN4I_HDMI_DDC_CTRL_RESET  BIT(0)
 
@@ -105,14 +106,30 @@
 #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8)
 #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff)
 
+#define SUN4I_HDMI_DDC_INT_STATUS_REG  0x50c
+#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION   BIT(7)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOWBIT(6)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOWBIT(5)
+#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST BIT(4)
+#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERRORBIT(3)
+#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERRORBIT(2)
+#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERRORBIT(1)
+#define SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETEBIT(0)
+
 #define SUN4I_HDMI_DDC_FIFO_CTRL_REG   0x510
 #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31)
 
+#define SUN4I_HDMI_DDC_FIFO_STATUS_REG 0x514
+#define SUN4I_HDMI_DDC_FIFO_STATUS_FULLBIT(6)
+#define SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY   BIT(5)
+
 #define SUN4I_HDMI_DDC_FIFO_DATA_REG   0x518
 #define SUN4I_HDMI_DDC_BYTE_COUNT_REG  0x51c
 
 #define SUN4I_HDMI_DDC_CMD_REG 0x520
 #define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ  6
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ   5
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE  3
 
 #define SUN4I_HDMI_DDC_CLK_REG 0x528
 #define SUN4I_HDMI_DDC_CLK_M(m)(((m) & 0x7) << 3)
@@ -123,6 +140,7 @@
 #define SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLEBIT(8)
 
 #define SUN4I_HDMI_DDC_FIFO_SIZE   16
+#define SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE   1023
 
 enum sun4i_hdmi_pkt_type {
SUN4I_HDMI_PKT_AVI = 2,
@@ -146,6 +164,8 @@ struct sun4i_hdmi {
struct clk  *ddc_clk;
struct clk  *tmds_clk;
 
+   struct i2c_adapter  *i2c;
+
struct sun4i_drv

[PATCH v3] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states:
"As in the general case the DDC bus is accessible by the kernel at the I2C
level, drivers must make all reasonable efforts to expose it as an I2C
adapter and use drm_get_edid() instead of abusing this function."

Exposing the DDC bus as an I2C adapter is more beneficial as it can be used
for purposes other than reading the EDID such as modifying the EDID or
using the HDMI DDC pins as an I2C bus through the I2C dev interface from
userspace (e.g. i2c-tools).

Implement this for A10s.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
Changes for v3:
 - Explain why drm_do_get_edid should be used and why it's better to expose it
   as an I2C adapter in commit message
 - Reorder bit defines in descending order for consistency
 - Keep old unused macros instead of removing them
 - The v2 algorithm split large transfers into 16 byte transfers but this may
   cause a large single write to be treated as multiple writes causing data
   corruption. The algorithm has been reworked to not split larger transfers
   and make more use of the FIFO to avoid this.
 - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to
   sun4i_hdmi_i2c.c
 - Reformatted code
 - Instead of masking bits that we don't want to check for errors, explicitly
   check the error bits
 - Clear error bits at start of transfer in case of error from previous transfer
 - Poll for completion of FIFO clear after setting FIFO clear bit

Changes for v2:
 - Rebased against Maxime's sunxi-drm/for-next branch
 - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if
   any of the calls after the I2C adapter is created fails
 - Remove unnecessary includes in sun4i_hdmi_i2c.c

 drivers/gpu/drm/sun4i/Makefile |   1 +
 drivers/gpu/drm/sun4i/sun4i_hdmi.h |  21 
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++-
 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 220 +
 4 files changed, 253 insertions(+), 90 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c

diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index e29fd3a2ba9c..43c753cafc88 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
 sun4i-drm-y += sun4i_framebuffer.o
 
 sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
+sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
 sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
 sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h 
b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 2f2f2ff1ea63..018af9cbb593 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -96,6 +96,7 @@
 #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31)
 #define SUN4I_HDMI_DDC_CTRL_START_CMD  BIT(30)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK  BIT(8)
+#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ  (0 << 8)
 #define SUN4I_HDMI_DDC_CTRL_RESET  BIT(0)
 
@@ -105,14 +106,30 @@
 #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8)
 #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff)
 
+#define SUN4I_HDMI_DDC_INT_STATUS_REG  0x50c
+#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION   BIT(7)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOWBIT(6)
+#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOWBIT(5)
+#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST BIT(4)
+#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERRORBIT(3)
+#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERRORBIT(2)
+#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERRORBIT(1)
+#define SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETEBIT(0)
+
 #define SUN4I_HDMI_DDC_FIFO_CTRL_REG   0x510
 #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31)
 
+#define SUN4I_HDMI_DDC_FIFO_STATUS_REG 0x514
+#define SUN4I_HDMI_DDC_FIFO_STATUS_FULLBIT(6)
+#define SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY   BIT(5)
+
 #define SUN4I_HDMI_DDC_FIFO_DATA_REG   0x518
 #define SUN4I_HDMI_DDC_BYTE_COUNT_REG  0x51c
 
 #define SUN4I_HDMI_DDC_CMD_REG 0x520
 #define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ  6
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ   5
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE  3
 
 #define SUN4I_HDMI_DDC_CLK_REG 0x528
 #define SUN4I_HDMI_DDC_CLK_M(m)(((m) & 0x7) << 3)
@@ -123,6 +140,7 @@
 #define SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLEBIT(8)
 
 #define SUN4I_HDMI_DDC_FIFO_SIZE   16
+#define SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE   1023
 
 enum sun4i_hdmi_pkt_type {
SUN4I_HDMI_PKT_AVI = 2,
@@ -146,6 +164,8 @@ struct sun4i_hdmi {
struct clk  *ddc_clk;
struct clk  *tmds_clk;
 
+   struct i2c_adapter  *i2c;
+
struct sun4i_drv

Re: [PATCH v2] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

Hi Maxime,

On 13 June 2017 at 21:15, Maxime Ripard
<maxime.rip...@free-electrons.com> wrote:
> On Mon, Jun 12, 2017 at 03:52:35PM +1000, Jonathan Liu wrote:
>> The drm_get_edid function should be used instead of drm_do_get_edid by
>> exposing the DDC bus as an I2C adapter. Implement this for A10s.
>>
>> Signed-off-by: Jonathan Liu <net...@gmail.com>
>
> Your commit log should explain *why* that function should be used
> instead, and why it's better to expose it as an i2c adadpter.
Ok.

>
>> ---
>> Changes for v2:
>>  - Rebased against Maxime's sunxi-drm/for-next branch
>>  - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted 
>> if
>>any of the calls after the I2C adapter is created fails
>>  - Remove unnecessary includes in sun4i_hdmi_i2c.c
>>
>>  drivers/gpu/drm/sun4i/Makefile |   1 +
>>  drivers/gpu/drm/sun4i/sun4i_hdmi.h |  11 ++-
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 103 +++--
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 163 
>> +
>>  4 files changed, 189 insertions(+), 89 deletions(-)
>>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
>>
>> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
>> index e29fd3a2ba9c..43c753cafc88 100644
>> --- a/drivers/gpu/drm/sun4i/Makefile
>> +++ b/drivers/gpu/drm/sun4i/Makefile
>> @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
>>  sun4i-drm-y += sun4i_framebuffer.o
>>
>>  sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
>> +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
>>  sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
>>  sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
>
> You probably also need to depend on the I2C framework in order to
> work, right?
In Kconfig, DRM_SUN4I depends on DRM which depends on I2C already.
Is there anything else I need to do here?

>
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h 
>> b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> index 2f2f2ff1ea63..4c01dbe89cd9 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> @@ -97,6 +97,7 @@
>>  #define SUN4I_HDMI_DDC_CTRL_START_CMDBIT(30)
>>  #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASKBIT(8)
>>  #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ(0 << 8)
>> +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE   (1 << 8)
>
> All the other bit defines are ordered by descending orders, please
> keep it consistent.
Ok.

>
>>  #define SUN4I_HDMI_DDC_CTRL_RESETBIT(0)
>>
>>  #define SUN4I_HDMI_DDC_ADDR_REG  0x504
>> @@ -105,6 +106,10 @@
>>  #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)  (((off) & 0xff) << 8)
>>  #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)  ((addr) & 0xff)
>>
>> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_REG  0x50c
>
> It is called INT_STATUS in the datasheet
>
>> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST BIT(4)
>> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETEBIT(0)
>> +
>>  #define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510
>>  #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR   BIT(31)
>>
>> @@ -112,7 +117,8 @@
>>  #define SUN4I_HDMI_DDC_BYTE_COUNT_REG0x51c
>>
>>  #define SUN4I_HDMI_DDC_CMD_REG   0x520
>> -#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ6
>> +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE3
>> +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ 5
>
> Same remark here, and why do you remove the old one?
Ok, I will not remove any old macros.

>
>>
>>  #define SUN4I_HDMI_DDC_CLK_REG   0x528
>>  #define SUN4I_HDMI_DDC_CLK_M(m)  (((m) & 0x7) << 3)
>> @@ -146,6 +152,8 @@ struct sun4i_hdmi {
>>   struct clk  *ddc_clk;
>>   struct clk  *tmds_clk;
>>
>> + struct i2c_adapter  *i2c;
>> +
>>   struct sun4i_drv*drv;
>>
>>   boolhdmi_monitor;
>> @@ -153,5 +161,6 @@ struct sun4i_hdmi {
>>
>>  int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
>>  int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
>> +int sun4i_hdmi_i2c_create(struct sun4i_hdmi *hdmi);
>>
>>  #endif /* _SUN4I_HDMI_H_ */
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c 
>> b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> index d3398f6250ef..2a8c0b14eabc 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>

[PATCH] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

The drm_get_edid function should be used instead of drm_do_get_edid by
exposing the DDC bus as an I2C adapter. Implement this for A10s.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
 drivers/gpu/drm/sun4i/Makefile |   1 +
 drivers/gpu/drm/sun4i/sun4i_hdmi.h |  11 ++-
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c |  96 +++
 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 166 +
 4 files changed, 190 insertions(+), 84 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c

diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index e29fd3a2ba9c..43c753cafc88 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
 sun4i-drm-y += sun4i_framebuffer.o
 
 sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
+sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
 sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
 sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h 
b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 2589bc92be59..8e7a70f67f04 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -100,6 +100,7 @@
 #define SUN4I_HDMI_DDC_CTRL_START_CMD  BIT(30)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK  BIT(8)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ  (0 << 8)
+#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8)
 #define SUN4I_HDMI_DDC_CTRL_RESET  BIT(0)
 
 #define SUN4I_HDMI_DDC_ADDR_REG0x504
@@ -108,6 +109,10 @@
 #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8)
 #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff)
 
+#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_REG0x50c
+#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST   BIT(4)
+#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE  BIT(0)
+
 #define SUN4I_HDMI_DDC_FIFO_CTRL_REG   0x510
 #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31)
 
@@ -115,7 +120,8 @@
 #define SUN4I_HDMI_DDC_BYTE_COUNT_REG  0x51c
 
 #define SUN4I_HDMI_DDC_CMD_REG 0x520
-#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ  6
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE  3
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ   5
 
 #define SUN4I_HDMI_DDC_CLK_REG 0x528
 #define SUN4I_HDMI_DDC_CLK_M(m)(((m) & 0x7) << 3)
@@ -181,6 +187,8 @@ struct sun4i_hdmi {
struct clk  *ddc_clk;
struct clk  *tmds_clk;
 
+   struct i2c_adapter  *i2c;
+
struct sun4i_drv*drv;
 
boolhdmi_monitor;
@@ -192,5 +200,6 @@ int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk 
*clk);
 int sun6i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
 int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
 int sun6i_tmds_create(struct sun4i_hdmi *hdmi);
+int sun4i_hdmi_i2c_create(struct sun4i_hdmi *hdmi);
 
 #endif /* _SUN4I_HDMI_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c 
b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index e9abf93eb41c..6c9b11c4cf68 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -186,93 +186,13 @@ static const struct drm_encoder_funcs sun4i_hdmi_funcs = {
.destroy= drm_encoder_cleanup,
 };
 
-static int sun4i_hdmi_read_sub_block(struct sun4i_hdmi *hdmi,
-unsigned int blk, unsigned int offset,
-u8 *buf, unsigned int count)
-{
-   unsigned long reg;
-   int i;
-
-   reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
-   reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK;
-   writel(reg | SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ,
-  hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
-
-   writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) |
-  SUN4I_HDMI_DDC_ADDR_EDDC(DDC_SEGMENT_ADDR << 1) |
-  SUN4I_HDMI_DDC_ADDR_OFFSET(offset) |
-  SUN4I_HDMI_DDC_ADDR_SLAVE(DDC_ADDR),
-  hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
-
-   reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
-   writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
-  hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
-
-   writel(count, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
-   writel(SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ,
-  hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
-
-   reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
-   writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
-  hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
-
-   if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
-  !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
-  100, 10))
-   return -EIO;
-
-   for (i = 0; i < count; i++)
-   

[PATCH v2] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

The drm_get_edid function should be used instead of drm_do_get_edid by
exposing the DDC bus as an I2C adapter. Implement this for A10s.

Signed-off-by: Jonathan Liu <net...@gmail.com>
---
Changes for v2:
 - Rebased against Maxime's sunxi-drm/for-next branch
 - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if
   any of the calls after the I2C adapter is created fails
 - Remove unnecessary includes in sun4i_hdmi_i2c.c

 drivers/gpu/drm/sun4i/Makefile |   1 +
 drivers/gpu/drm/sun4i/sun4i_hdmi.h |  11 ++-
 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 103 +++--
 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 163 +
 4 files changed, 189 insertions(+), 89 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c

diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index e29fd3a2ba9c..43c753cafc88 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
 sun4i-drm-y += sun4i_framebuffer.o
 
 sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
+sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
 sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
 sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
 
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h 
b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
index 2f2f2ff1ea63..4c01dbe89cd9 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
@@ -97,6 +97,7 @@
 #define SUN4I_HDMI_DDC_CTRL_START_CMD  BIT(30)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK  BIT(8)
 #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ  (0 << 8)
+#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8)
 #define SUN4I_HDMI_DDC_CTRL_RESET  BIT(0)
 
 #define SUN4I_HDMI_DDC_ADDR_REG0x504
@@ -105,6 +106,10 @@
 #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8)
 #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff)
 
+#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_REG0x50c
+#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST   BIT(4)
+#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE  BIT(0)
+
 #define SUN4I_HDMI_DDC_FIFO_CTRL_REG   0x510
 #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31)
 
@@ -112,7 +117,8 @@
 #define SUN4I_HDMI_DDC_BYTE_COUNT_REG  0x51c
 
 #define SUN4I_HDMI_DDC_CMD_REG 0x520
-#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ  6
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE  3
+#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ   5
 
 #define SUN4I_HDMI_DDC_CLK_REG 0x528
 #define SUN4I_HDMI_DDC_CLK_M(m)(((m) & 0x7) << 3)
@@ -146,6 +152,8 @@ struct sun4i_hdmi {
struct clk  *ddc_clk;
struct clk  *tmds_clk;
 
+   struct i2c_adapter  *i2c;
+
struct sun4i_drv*drv;
 
boolhdmi_monitor;
@@ -153,5 +161,6 @@ struct sun4i_hdmi {
 
 int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
 int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
+int sun4i_hdmi_i2c_create(struct sun4i_hdmi *hdmi);
 
 #endif /* _SUN4I_HDMI_H_ */
diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c 
b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
index d3398f6250ef..2a8c0b14eabc 100644
--- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
@@ -29,8 +29,6 @@
 #include "sun4i_hdmi.h"
 #include "sun4i_tcon.h"
 
-#define DDC_SEGMENT_ADDR   0x30
-
 static inline struct sun4i_hdmi *
 drm_encoder_to_sun4i_hdmi(struct drm_encoder *encoder)
 {
@@ -184,93 +182,13 @@ static const struct drm_encoder_funcs sun4i_hdmi_funcs = {
.destroy= drm_encoder_cleanup,
 };
 
-static int sun4i_hdmi_read_sub_block(struct sun4i_hdmi *hdmi,
-unsigned int blk, unsigned int offset,
-u8 *buf, unsigned int count)
-{
-   unsigned long reg;
-   int i;
-
-   reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
-   reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK;
-   writel(reg | SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ,
-  hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
-
-   writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) |
-  SUN4I_HDMI_DDC_ADDR_EDDC(DDC_SEGMENT_ADDR << 1) |
-  SUN4I_HDMI_DDC_ADDR_OFFSET(offset) |
-  SUN4I_HDMI_DDC_ADDR_SLAVE(DDC_ADDR),
-  hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
-
-   reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
-   writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
-  hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
-
-   writel(count, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
-   writel(SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ,
-  hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
-
-   reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
-

Re: [PATCH] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

[Jonathan Liu]

Hi Chen-Yu,

On 12 June 2017 at 13:28, Chen-Yu Tsai <w...@csie.org> wrote:
> On Mon, Jun 12, 2017 at 10:12 AM, Jonathan Liu <net...@gmail.com> wrote:
>> The drm_get_edid function should be used instead of drm_do_get_edid by
>> exposing the DDC bus as an I2C adapter. Implement this for A10s.
>
> Nice!

It is my first Linux kernel driver so there may be other things I have
overlooked. I am not sure about the behaviour of I2C writes larger
than 16 bytes but the EEPROMs I have tested only have a write buffer
of 8 bytes so writes are limited to a maximum of 8 bytes at a time
anyway.

>
>>
>> Signed-off-by: Jonathan Liu <net...@gmail.com>
>> ---
>>  drivers/gpu/drm/sun4i/Makefile |   1 +
>>  drivers/gpu/drm/sun4i/sun4i_hdmi.h |  11 ++-
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c |  96 +++
>>  drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 166 
>> +
>>  4 files changed, 190 insertions(+), 84 deletions(-)
>>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
>>
>> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
>> index e29fd3a2ba9c..43c753cafc88 100644
>> --- a/drivers/gpu/drm/sun4i/Makefile
>> +++ b/drivers/gpu/drm/sun4i/Makefile
>> @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o
>>  sun4i-drm-y += sun4i_framebuffer.o
>>
>>  sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
>> +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o
>>  sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
>>  sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h 
>> b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> index 2589bc92be59..8e7a70f67f04 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
>> @@ -100,6 +100,7 @@
>>  #define SUN4I_HDMI_DDC_CTRL_START_CMD  BIT(30)
>>  #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK  BIT(8)
>>  #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ  (0 << 8)
>> +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8)
>>  #define SUN4I_HDMI_DDC_CTRL_RESET  BIT(0)
>>
>>  #define SUN4I_HDMI_DDC_ADDR_REG0x504
>> @@ -108,6 +109,10 @@
>>  #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8)
>>  #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff)
>>
>> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_REG0x50c
>> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST   BIT(4)
>> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE  BIT(0)
>> +
>>  #define SUN4I_HDMI_DDC_FIFO_CTRL_REG   0x510
>>  #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31)
>>
>> @@ -115,7 +120,8 @@
>>  #define SUN4I_HDMI_DDC_BYTE_COUNT_REG  0x51c
>>
>>  #define SUN4I_HDMI_DDC_CMD_REG 0x520
>> -#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ  6
>> +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE  3
>> +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ   5
>>
>>  #define SUN4I_HDMI_DDC_CLK_REG 0x528
>>  #define SUN4I_HDMI_DDC_CLK_M(m)(((m) & 0x7) << 3)
>> @@ -181,6 +187,8 @@ struct sun4i_hdmi {
>> struct clk  *ddc_clk;
>> struct clk  *tmds_clk;
>>
>> +   struct i2c_adapter  *i2c;
>> +
>> struct sun4i_drv*drv;
>>
>> boolhdmi_monitor;
>> @@ -192,5 +200,6 @@ int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk 
>> *clk);
>>  int sun6i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
>>  int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
>>  int sun6i_tmds_create(struct sun4i_hdmi *hdmi);
>> +int sun4i_hdmi_i2c_create(struct sun4i_hdmi *hdmi);
>
> However you seem to have based it on the wrong branch/patches.
> You based them on my A31 HDMI patches, instead of what Maxime has
> in his sunxi-drm/for-next branch.
>
> I could add this to my A31 patches, though it probably won't make
> 4.13. Alternatively you could rebase them on top of Maxime's branch.

Yes, I forgot about that. I will rebase against sunxi-drm/for-next for V2.

>
>>
>>  #endif /* _SUN4I_HDMI_H_ */
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c 
>> b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> index e9abf93eb41c..6c9b11c4cf68 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>> @@ -186,93 +186,13 @@ static const struct drm_encoder_funcs sun4i_hdmi_funcs 
>> = {
>> .destroy= drm_

[PATCH][v2] drm/sun4i: rgb: Enable panel after controller

[Jonathan Liu]

The panel should be enabled after the controller so that we do not have
visual glitches on the panel while the controller is setup. Similarly,
the panel should be disabled before the controller.

Signed-off-by: Jonathan Liu 
---
Changes in v2:
 - Changed the commit message to be clearer

 drivers/gpu/drm/sun4i/sun4i_rgb.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c 
b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index c3ff10f..4e4bea6 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -152,15 +152,16 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder 
*encoder)

DRM_DEBUG_DRIVER("Enabling RGB output\n");

-   if (!IS_ERR(tcon->panel)) {
+   if (!IS_ERR(tcon->panel))
drm_panel_prepare(tcon->panel);
-   drm_panel_enable(tcon->panel);
-   }

/* encoder->bridge can be NULL; drm_bridge_enable checks for it */
drm_bridge_enable(encoder->bridge);

sun4i_tcon_channel_enable(tcon, 0);
+
+   if (!IS_ERR(tcon->panel))
+   drm_panel_enable(tcon->panel);
 }

 static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
@@ -171,15 +172,16 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder 
*encoder)

DRM_DEBUG_DRIVER("Disabling RGB output\n");

+   if (!IS_ERR(tcon->panel))
+   drm_panel_disable(tcon->panel);
+
sun4i_tcon_channel_disable(tcon, 0);

/* encoder->bridge can be NULL; drm_bridge_disable checks for it */
drm_bridge_disable(encoder->bridge);

-   if (!IS_ERR(tcon->panel)) {
-   drm_panel_disable(tcon->panel);
+   if (!IS_ERR(tcon->panel))
drm_panel_unprepare(tcon->panel);
-   }
 }

 static void sun4i_rgb_encoder_mode_set(struct drm_encoder *encoder,
-- 
2.10.0

[PATCH] drm/sun4i: rgb: Enable panel after controller

[Jonathan Liu]

Hi Maxime,

On 23 September 2016 at 23:16, Maxime Ripard
 wrote:
> On Thu, Sep 22, 2016 at 08:03:31AM +1000, Jonathan Liu wrote:
>> Hi Maxime,
>>
>> On Thursday, 22 September 2016, Maxime Ripard > free-electrons.
>> com> wrote:
>>
>> > On Wed, Sep 21, 2016 at 11:03:04PM +1000, Jonathan Liu wrote:
>> > > The panel should be enabled after the controller so that the panel
>> > > prepare/enable delays are properly taken into account. Similarly, the
>> > > panel should be disabled before the controller so that the panel
>> > > unprepare/disable delays are properly taken into account.
>> > >
>> > > This is useful for avoiding visual glitches.
>> >
>> > This is not really taking any delays into account, especially since
>> > drm_panel_enable and prepare are suppose to block until their
>> > operation is complete.
>>
>>
>> drm_panel_prepare turns on power to the LCD using enable-gpios property of
>> the panel and then blocks for prepare delay. The prepare delay for panel
>> can be set to how long it takes between the time the panel is powered to
>> when it is ready to receive images. If backlight property is specified the
>> backlight will be off while the panel is powered on.
>>
>> drm_panel_enable blocks for enable delay and then turns on the backlight.
>> The enable delay can be set to how long it takes for panel to start making
>> the image visible after receiving the first valid frame. For example if the
>> panel starts off as white and the TFT takes some time to initialize to
>> black before it shows the image being received.
>>
>> Refer to drivers/gpu/drm/panel-panel.simple.c for details.
>
> From drm_panel.h:
>
> """
> * drm_panel_enable - enable a panel
> * @panel: DRM panel
> *
> * Calling this function will cause the panel display drivers to be turned on
> * and the backlight to be enabled. Content will be visible on screen after
> * this call completes.
> """
>
> """
> * drm_panel_prepare - power on a panel
> * @panel: DRM panel
> *
> * Calling this function will enable power and deassert any reset signals to
> * the panel. After this has completed it is possible to communicate with any
> * integrated circuitry via a command bus.
> """
>
> Those comments clearly says that the caller should not have to deal
> with the delays, even more so by just moving calls around and hoping
> that the code running in between is adding enough delay for the panel
> to behave properly.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/drm/drm_panel.h#n34

In comment for struct drm_panel_funcs:
/**
 * struct drm_panel_funcs - perform operations on a given panel
 * @disable: disable panel (turn off back light, etc.)
 * @unprepare: turn off panel
 * @prepare: turn on panel and perform set up
 * @enable: enable panel (turn on back light, etc.)
 * @get_modes: add modes to the connector that the panel is attached to and
 * return the number of modes added
 * @get_timings: copy display timings into the provided array and return
 * the number of display timings available
 *
 * The .prepare() function is typically called before the display controller
 * starts to transmit video data. Panel drivers can use this to turn the panel
 * on and wait for it to become ready. If additional configuration is required
 * (via a control bus such as I2C, SPI or DSI for example) this is a good time
 * to do that.
 *
 * After the display controller has started transmitting video data, it's safe
 * to call the .enable() function. This will typically enable the backlight to
 * make the image on screen visible. Some panels require a certain amount of
 * time or frames before the image is displayed. This function is responsible
 * for taking this into account before enabling the backlight to avoid visual
 * glitches.
 *
 * Before stopping video transmission from the display controller it can be
 * necessary to turn off the panel to avoid visual glitches. This is done in
 * the .disable() function. Analogously to .enable() this typically involves
 * turning off the backlight and waiting for some time to make sure no image
 * is visible on the panel. It is then safe for the display controller to
 * cease transmission of video data.
 *
 * To save power when no video data is transmitted, a driver can power down
 * the panel. This is the job of the .unprepare() function.
 */

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/panel/panel-simple.c?id=refs/tags/v4.8-rc7#n39

I

[PATCH] drm/sun4i: rgb: Enable panel after controller

[Jonathan Liu]

Hi Maxime,

On 22 September 2016 at 07:03, Maxime Ripard
 wrote:
> On Wed, Sep 21, 2016 at 11:03:04PM +1000, Jonathan Liu wrote:
>> The panel should be enabled after the controller so that the panel
>> prepare/enable delays are properly taken into account. Similarly, the
>> panel should be disabled before the controller so that the panel
>> unprepare/disable delays are properly taken into account.
>>
>> This is useful for avoiding visual glitches.
>
> This is not really taking any delays into account, especially since
> drm_panel_enable and prepare are suppose to block until their
> operation is complete.

drm_panel_prepare turns on power to the LCD using enable-gpios
property of the panel and then blocks for prepare delay. The prepare
delay for panel can be set to how long it takes between the time the
panel is powered to when it is ready to receive images. If backlight
property is specified the backlight will be off while the panel is
powered on.

drm_panel_enable blocks for enable delay and then turns on the
backlight. The enable delay can be set to how long it takes for panel
to start making the image visible after receiving the first valid
frame. For example if the panel starts off as white and the TFT takes
some time to initialize to black before it shows the image being
received.

Refer to drivers/gpu/drm/panel-panel.simple.c for details.

Regards,
Jonathan

[PATCH] drm/sun4i: rgb: Enable panel after controller

[Jonathan Liu]

Hi Maxime,

On Thursday, 22 September 2016, Maxime Ripard  wrote:

> On Wed, Sep 21, 2016 at 11:03:04PM +1000, Jonathan Liu wrote:
> > The panel should be enabled after the controller so that the panel
> > prepare/enable delays are properly taken into account. Similarly, the
> > panel should be disabled before the controller so that the panel
> > unprepare/disable delays are properly taken into account.
> >
> > This is useful for avoiding visual glitches.
>
> This is not really taking any delays into account, especially since
> drm_panel_enable and prepare are suppose to block until their
> operation is complete.


drm_panel_prepare turns on power to the LCD using enable-gpios property of
the panel and then blocks for prepare delay. The prepare delay for panel
can be set to how long it takes between the time the panel is powered to
when it is ready to receive images. If backlight property is specified the
backlight will be off while the panel is powered on.

drm_panel_enable blocks for enable delay and then turns on the backlight.
The enable delay can be set to how long it takes for panel to start making
the image visible after receiving the first valid frame. For example if the
panel starts off as white and the TFT takes some time to initialize to
black before it shows the image being received.

Refer to drivers/gpu/drm/panel-panel.simple.c for details.

Regards,
Jonathan
-- next part --
An HTML attachment was scrubbed...
URL: 
<https://lists.freedesktop.org/archives/dri-devel/attachments/20160922/28e3d06f/attachment.html>

[PATCH] drm/sun4i: rgb: Enable panel after controller

[Jonathan Liu]

The panel should be enabled after the controller so that the panel
prepare/enable delays are properly taken into account. Similarly, the
panel should be disabled before the controller so that the panel
unprepare/disable delays are properly taken into account.

This is useful for avoiding visual glitches.

Signed-off-by: Jonathan Liu 
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c 
b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index c3ff10f..4e4bea6 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -152,15 +152,16 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder 
*encoder)

DRM_DEBUG_DRIVER("Enabling RGB output\n");

-   if (!IS_ERR(tcon->panel)) {
+   if (!IS_ERR(tcon->panel))
drm_panel_prepare(tcon->panel);
-   drm_panel_enable(tcon->panel);
-   }

/* encoder->bridge can be NULL; drm_bridge_enable checks for it */
drm_bridge_enable(encoder->bridge);

sun4i_tcon_channel_enable(tcon, 0);
+
+   if (!IS_ERR(tcon->panel))
+   drm_panel_enable(tcon->panel);
 }

 static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
@@ -171,15 +172,16 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder 
*encoder)

DRM_DEBUG_DRIVER("Disabling RGB output\n");

+   if (!IS_ERR(tcon->panel))
+   drm_panel_disable(tcon->panel);
+
sun4i_tcon_channel_disable(tcon, 0);

/* encoder->bridge can be NULL; drm_bridge_disable checks for it */
drm_bridge_disable(encoder->bridge);

-   if (!IS_ERR(tcon->panel)) {
-   drm_panel_disable(tcon->panel);
+   if (!IS_ERR(tcon->panel))
drm_panel_unprepare(tcon->panel);
-   }
 }

 static void sun4i_rgb_encoder_mode_set(struct drm_encoder *encoder,
-- 
2.10.0

[PATCH] drm/panel: simple: Fix bus_format for the Olimex LCD-OLinuXino-4.3TS

[Jonathan Liu]

The format is RGB888 not RGB666.

Signed-off-by: Jonathan Liu 
---
 drivers/gpu/drm/panel/panel-simple.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c 
b/drivers/gpu/drm/panel/panel-simple.c
index 85143d1..d4aa68e 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1186,7 +1186,7 @@ static const struct panel_desc olimex_lcd_olinuxino_43ts 
= {
.width = 105,
.height = 67,
},
-   .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+   .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
 };

 /*
-- 
2.9.3

[PATCH] drm/sun4i: rgb: add missing calls to drm_panel_{prepare, unprepare}

[Jonathan Liu]

If the enable-gpios property of a simple panel in device tree is set,
the GPIO is not toggled on/off because of missing calls to
drm_panel_prepare and drm_panel_unprepare.

Signed-off-by: Jonathan Liu 
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c 
b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index f5bbac6..d6943e9 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -151,6 +151,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder 
*encoder)

DRM_DEBUG_DRIVER("Enabling RGB output\n");

+   drm_panel_prepare(tcon->panel);
drm_panel_enable(tcon->panel);
sun4i_tcon_channel_enable(tcon, 0);
 }
@@ -165,6 +166,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder 
*encoder)

sun4i_tcon_channel_disable(tcon, 0);
drm_panel_disable(tcon->panel);
+   drm_panel_unprepare(tcon->panel);
 }

 static void sun4i_rgb_encoder_mode_set(struct drm_encoder *encoder,
-- 
2.9.3

[linux-sunxi] [PATCH 11/19] drm: sun4i: Add composite output

[Jonathan Liu]

On 31 October 2015 at 01:20, Maxime Ripard
 wrote:
> Some Allwinner SoCs have an IP called the TV encoder that is used to output
> composite and VGA signals. In such a case, we need to use the second TCON
> channel.
>
> Add support for that TV encoder.
>
> Signed-off-by: Maxime Ripard 
> ---
>  drivers/gpu/drm/sun4i/Makefile|   1 +
>  drivers/gpu/drm/sun4i/sun4i_drv.c |  10 +-
>  drivers/gpu/drm/sun4i/sun4i_tv.c  | 532 
> ++
>  drivers/gpu/drm/sun4i/sun4i_tv.h  |  18 ++
>  4 files changed, 560 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_tv.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_tv.h
>
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 4d230b658a05..fc0dc8be82d9 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -6,5 +6,6 @@ sun4i-drm-y += sun4i_layer.o
>  sun4i-drm-y += sun4i_tcon.o
>
>  sun4i-drm-y += sun4i_rgb.o
> +sun4i-drm-y += sun4i_tv.o
>
>  obj-$(CONFIG_DRM_SUN4I)+= sun4i-drm.o
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c 
> b/drivers/gpu/drm/sun4i/sun4i_drv.c
> index f2c9c8a2eb75..3cf7a9e89afa 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> @@ -22,6 +22,7 @@
>  #include "sun4i_layer.h"
>  #include "sun4i_rgb.h"
>  #include "sun4i_tcon.h"
> +#include "sun4i_tv.h"
>
>  static void sun4i_drv_preclose(struct drm_device *drm,
>struct drm_file *file_priv)
> @@ -134,12 +135,18 @@ static int sun4i_drv_load(struct drm_device *drm, 
> unsigned long flags)
> goto err_free_crtc;
> }
>
> +   ret = sun4i_tv_init(drm);
> +   if (ret) {
> +   dev_err(drm->dev, "Couldn't create our RGB output\n");

RGB should be composite. Seems like a copy-paste error.

> +   goto err_free_rgb;
> +   }
> +
> /* Create our framebuffer */
> drv->fbdev = sun4i_framebuffer_init(drm);
> if (IS_ERR(drv->fbdev)) {
> dev_err(drm->dev, "Couldn't create our framebuffer\n");
> ret = PTR_ERR(drv->fbdev);
> -   goto err_free_rgb;
> +   goto err_free_tv;
> }
>
> /* Enable connectors polling */

Regards,
Jonathan