Re: [PATCHv6 5/5] drm: bridge: anx78xx: Add anx78xx driver support by analogix.

[Enric Balletbo Serra]

Hi Dan,

Many thanks for your comments.

2015-12-07 9:09 GMT+01:00 Dan Carpenter :
> On Fri, Dec 04, 2015 at 09:35:07AM +0100, Enric Balletbo i Serra wrote:
>> +static int sp_wait_aux_op_finish(struct anx78xx *anx78xx)
>> +{
>> + u8 errcnt;
>> + u8 val;
>> + struct device *dev = >client->dev;
>> +
>> + errcnt = 150;
>> + while (errcnt--) {
>> + sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, );
>> + if (!(val & SP_AUX_EN))
>> + break;
>> + usleep_range(2000, 4000);
>> + }
>> +
>> + if (!errcnt) {
>
> This is off by one.  It should be:
>
> while (--errcnt) {
> ...
> }
> if (errcnt == 0)
> return -EWHATEVER;
>
> Or:
>
> while (errcnt--) {
> ...
> }
> if (errcnt == -1)
> return -EWHATEVER;
>
> Also "errcnt" is a bad name, it should be retry_cnt or something (or
> maybe it actually is counting errors?).  Also -1 is never a correct
> error code, please change all the -1 returns to something better.
>

Ok, I renamed to retry_cnt and changed all the -1 values to something better.


>> + /* Buffer size of AUX CH is 16 */
>> + if (count > 16)
>> + return -1;
>
> Just make a define so that you don't need to add comments about why 16
> is correct.
>
> if (count > SIZE_AUX_CH)
> return -EINVAL;
>

Added a define

>> + errcnt = 10;
>> + while (errcnt--) {
>> + sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, );
>> + if (!(val & SP_AUX_EN))
>> + break;
>> + usleep_range(1000, 2000);
>> + }
>> +
>> + if (!errcnt) {
>> + dev_err(dev,
>> + "failed to read DP AUX Channel Control Register 2\n");
>> + sp_reset_aux(anx78xx);
>> + return -1;
>> + }
>
> Off by one again.
>

Ack

>
>> +
>> + sp_reg_write(anx78xx, TX_P0, SP_AUX_ADDR_7_0_REG, SP_I2C_EXTRA_ADDR);
>> + sp_tx_aux_wr(anx78xx, offset);
>> + /* read 16 bytes (MOT = 1) */
>> + sp_tx_aux_rd(anx78xx, 0xf0 | DP_AUX_I2C_MOT | DP_AUX_I2C_READ);
>> +
>> + for (i = 0; i < 16; i++) {
>> + errcnt = 10;
>> + while (errcnt--) {
>> + sp_reg_read(anx78xx, TX_P0, SP_BUF_DATA_COUNT_REG,
>> + );
>> + if (val & SP_BUF_DATA_COUNT_MASK)
>> + break;
>> + usleep_range(2000, 4000);
>> + }
>> +
>> + if (!errcnt) {
>> + dev_err(dev,
>> + "failed to read DP Buffer Data Count 
>> Register\n");
>> + sp_reset_aux(anx78xx);
>> + return -1;
>> + }
>
> And here.
>

Ack

>> + errcnt = 10;
>> + while (errcnt--) {
>> + sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, );
>> + if (!(val & SP_AUX_EN))
>> + break;
>> + usleep_range(1000, 2000);
>> + }
>> +
>> + if (!errcnt) {
>> + dev_err(dev,
>> + "failed to read DP AUX Channel Control Register 2\n");
>> + sp_reset_aux(anx78xx);
>> + return -1;
>> + }
>
> Here.
>

Ack

>
>> +
>> + return 0;
>> +}
>> +
>> +static int sp_edid_block_checksum(const u8 *raw_edid)
>> +{
>> + int i;
>> + u8 csum = 0;
>> +
>> + for (i = 0; i < EDID_LENGTH; i++)
>> + csum += raw_edid[i];
>> +
>> + return csum;
>> +}
>> +
>> +static int sp_tx_edid_read(struct anx78xx *anx78xx)
>> +{
>> + struct device *dev = >client->dev;
>> + u8 val, last_block, offset = 0;
>> + u8 buf[16];
>> + int i, j, count;
>> +
>> + sp_tx_edid_read_initial(anx78xx);
>> + sp_reg_write(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL1_REG, 0x04);
>> + sp_reg_set_bits(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG,
>> + SP_AUX_EN | SP_ADDR_ONLY);
>> +
>> + if (sp_wait_aux_op_finish(anx78xx))
>> + return -1;
>> +
>> + sp_addronly_set(anx78xx, false);
>> +
>> + /* Read the number of blocks */
>> + sp_tx_aux_wr(anx78xx, 0x7e);
>> + sp_tx_aux_rd(anx78xx, DP_AUX_I2C_READ);
>> + sp_reg_read(anx78xx, TX_P0, SP_DP_BUF_DATA0_REG, _block);
>> + dev_dbg(dev, "last EDID block is %d\n", last_block);
>> +
>> + /* FIXME: Why not just cap to 3 if the reported value is >3 */
>> + if (last_block > 3)
>> + last_block = 1;
>> +
>> + /* for every block */
>> + for (count = 0; count <= last_block; count++) {
>> + switch (count) {
>> + case 0:
>> + case 1:
>> + for (i = 0; i < 8; i++) {
>> + offset = (i + count * 8) * 16;
>> + if (sp_edid_read(anx78xx, offset, buf))
>> +  

Re: [PATCHv6 5/5] drm: bridge: anx78xx: Add anx78xx driver support by analogix.

[Dan Carpenter]

On Fri, Dec 04, 2015 at 09:35:07AM +0100, Enric Balletbo i Serra wrote:
> +static int sp_wait_aux_op_finish(struct anx78xx *anx78xx)
> +{
> + u8 errcnt;
> + u8 val;
> + struct device *dev = >client->dev;
> +
> + errcnt = 150;
> + while (errcnt--) {
> + sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, );
> + if (!(val & SP_AUX_EN))
> + break;
> + usleep_range(2000, 4000);
> + }
> +
> + if (!errcnt) {

This is off by one.  It should be:

while (--errcnt) {
...
}
if (errcnt == 0)
return -EWHATEVER;

Or:

while (errcnt--) {
...
}
if (errcnt == -1)
return -EWHATEVER;

Also "errcnt" is a bad name, it should be retry_cnt or something (or
maybe it actually is counting errors?).  Also -1 is never a correct
error code, please change all the -1 returns to something better.

> + /* Buffer size of AUX CH is 16 */
> + if (count > 16)
> + return -1;

Just make a define so that you don't need to add comments about why 16
is correct.

if (count > SIZE_AUX_CH)
return -EINVAL;

> + errcnt = 10;
> + while (errcnt--) {
> + sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, );
> + if (!(val & SP_AUX_EN))
> + break;
> + usleep_range(1000, 2000);
> + }
> +
> + if (!errcnt) {
> + dev_err(dev,
> + "failed to read DP AUX Channel Control Register 2\n");
> + sp_reset_aux(anx78xx);
> + return -1;
> + }

Off by one again.


> +
> + sp_reg_write(anx78xx, TX_P0, SP_AUX_ADDR_7_0_REG, SP_I2C_EXTRA_ADDR);
> + sp_tx_aux_wr(anx78xx, offset);
> + /* read 16 bytes (MOT = 1) */
> + sp_tx_aux_rd(anx78xx, 0xf0 | DP_AUX_I2C_MOT | DP_AUX_I2C_READ);
> +
> + for (i = 0; i < 16; i++) {
> + errcnt = 10;
> + while (errcnt--) {
> + sp_reg_read(anx78xx, TX_P0, SP_BUF_DATA_COUNT_REG,
> + );
> + if (val & SP_BUF_DATA_COUNT_MASK)
> + break;
> + usleep_range(2000, 4000);
> + }
> +
> + if (!errcnt) {
> + dev_err(dev,
> + "failed to read DP Buffer Data Count 
> Register\n");
> + sp_reset_aux(anx78xx);
> + return -1;
> + }

And here.

> + errcnt = 10;
> + while (errcnt--) {
> + sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, );
> + if (!(val & SP_AUX_EN))
> + break;
> + usleep_range(1000, 2000);
> + }
> +
> + if (!errcnt) {
> + dev_err(dev,
> + "failed to read DP AUX Channel Control Register 2\n");
> + sp_reset_aux(anx78xx);
> + return -1;
> + }

Here.


> +
> + return 0;
> +}
> +
> +static int sp_edid_block_checksum(const u8 *raw_edid)
> +{
> + int i;
> + u8 csum = 0;
> +
> + for (i = 0; i < EDID_LENGTH; i++)
> + csum += raw_edid[i];
> +
> + return csum;
> +}
> +
> +static int sp_tx_edid_read(struct anx78xx *anx78xx)
> +{
> + struct device *dev = >client->dev;
> + u8 val, last_block, offset = 0;
> + u8 buf[16];
> + int i, j, count;
> +
> + sp_tx_edid_read_initial(anx78xx);
> + sp_reg_write(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL1_REG, 0x04);
> + sp_reg_set_bits(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG,
> + SP_AUX_EN | SP_ADDR_ONLY);
> +
> + if (sp_wait_aux_op_finish(anx78xx))
> + return -1;
> +
> + sp_addronly_set(anx78xx, false);
> +
> + /* Read the number of blocks */
> + sp_tx_aux_wr(anx78xx, 0x7e);
> + sp_tx_aux_rd(anx78xx, DP_AUX_I2C_READ);
> + sp_reg_read(anx78xx, TX_P0, SP_DP_BUF_DATA0_REG, _block);
> + dev_dbg(dev, "last EDID block is %d\n", last_block);
> +
> + /* FIXME: Why not just cap to 3 if the reported value is >3 */
> + if (last_block > 3)
> + last_block = 1;
> +
> + /* for every block */
> + for (count = 0; count <= last_block; count++) {
> + switch (count) {
> + case 0:
> + case 1:
> + for (i = 0; i < 8; i++) {
> + offset = (i + count * 8) * 16;
> + if (sp_edid_read(anx78xx, offset, buf))
> + return -1;
> + for (j = 0; j < 16; j++)
> + sp.edid_blocks[offset + j] = buf[j];
> + }
> + break;
> + case 2:
> + case 3:
> + offset = (count == 2) ? 0x00 : 0x80;
> + for (j = 0; j < 8; j++) {
> + 

[PATCHv6 5/5] drm: bridge: anx78xx: Add anx78xx driver support by analogix.

[Enric Balletbo i Serra]

At the moment it only supports ANX7814.

The ANX7814 is an ultra-low power Full-HD (1080p60) SlimPort transmitter
designed for portable devices. The ANX7814 transforms the HDMI output of
an application processor to MyDP or DisplayPort.

The driver supports HDMI to DP pass-through mode and works using external
adapters that converts MyDP or DisplayPort to HDMI or DVI.

Signed-off-by: Enric Balletbo i Serra 
---

Changes since last version:
- Fix auto build test ERROR (anx78xx->bridge.of_node = client->dev.of_node)
- Remove more magic numbers and use DP_ defines from hdmi.h
- Use common dp/hdmi defines instead of redefine it.
- Improve a bit the documentation of the driver.
- Improve debug messages.
- Use devm to request the irq.

 drivers/gpu/drm/bridge/Kconfig   |2 +
 drivers/gpu/drm/bridge/Makefile  |1 +
 drivers/gpu/drm/bridge/anx78xx/Kconfig   |5 +
 drivers/gpu/drm/bridge/anx78xx/Makefile  |4 +
 drivers/gpu/drm/bridge/anx78xx/anx78xx.h |   44 +
 drivers/gpu/drm/bridge/anx78xx/anx78xx_main.c|  334 +++
 drivers/gpu/drm/bridge/anx78xx/slimport_tx_drv.c | 3210 ++
 drivers/gpu/drm/bridge/anx78xx/slimport_tx_drv.h |  110 +
 drivers/gpu/drm/bridge/anx78xx/slimport_tx_reg.h |  737 +
 9 files changed, 4447 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/anx78xx/Kconfig
 create mode 100644 drivers/gpu/drm/bridge/anx78xx/Makefile
 create mode 100644 drivers/gpu/drm/bridge/anx78xx/anx78xx.h
 create mode 100644 drivers/gpu/drm/bridge/anx78xx/anx78xx_main.c
 create mode 100644 drivers/gpu/drm/bridge/anx78xx/slimport_tx_drv.c
 create mode 100644 drivers/gpu/drm/bridge/anx78xx/slimport_tx_drv.h
 create mode 100644 drivers/gpu/drm/bridge/anx78xx/slimport_tx_reg.h

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 639..1d92bc1 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -41,4 +41,6 @@ config DRM_PARADE_PS8622
---help---
  Parade eDP-LVDS bridge chip driver.
 
+source "drivers/gpu/drm/bridge/anx78xx/Kconfig"
+
 endmenu
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index d4e28be..0e9fdb4 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_DRM_DW_HDMI) += dw_hdmi.o
 obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw_hdmi-ahb-audio.o
 obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
 obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
+obj-$(CONFIG_DRM_ANX78XX) += anx78xx/
diff --git a/drivers/gpu/drm/bridge/anx78xx/Kconfig 
b/drivers/gpu/drm/bridge/anx78xx/Kconfig
new file mode 100644
index 000..5be362d
--- /dev/null
+++ b/drivers/gpu/drm/bridge/anx78xx/Kconfig
@@ -0,0 +1,5 @@
+config DRM_ANX78XX
+   tristate "Analogix ANX78XX bridge"
+   help
+ ANX78XX is a HD video transmitter chip over micro-USB
+ connector for smartphone device.
diff --git a/drivers/gpu/drm/bridge/anx78xx/Makefile 
b/drivers/gpu/drm/bridge/anx78xx/Makefile
new file mode 100644
index 000..a843733
--- /dev/null
+++ b/drivers/gpu/drm/bridge/anx78xx/Makefile
@@ -0,0 +1,4 @@
+obj-${CONFIG_DRM_ANX78XX} :=  anx78xx.o
+
+anx78xx-y += anx78xx_main.o
+anx78xx-y += slimport_tx_drv.o
diff --git a/drivers/gpu/drm/bridge/anx78xx/anx78xx.h 
b/drivers/gpu/drm/bridge/anx78xx/anx78xx.h
new file mode 100644
index 000..6548918
--- /dev/null
+++ b/drivers/gpu/drm/bridge/anx78xx/anx78xx.h
@@ -0,0 +1,44 @@
+/*
+ * Copyright(c) 2015, Analogix Semiconductor. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __ANX78xx_H
+#define __ANX78xx_H
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+struct anx78xx_platform_data {
+   struct gpio_desc *gpiod_pd;
+   struct gpio_desc *gpiod_reset;
+   struct gpio_desc *gpiod_v10;
+};
+
+struct anx78xx {
+   struct drm_bridge bridge;
+   struct i2c_client *client;
+   struct anx78xx_platform_data *pdata;
+   struct delayed_work work;
+   struct workqueue_struct *workqueue;
+};
+
+void anx78xx_poweron(struct anx78xx *data);
+void anx78xx_poweroff(struct anx78xx *data);
+bool anx78xx_cable_is_detected(struct anx78xx *anx78xx);
+
+#endif
diff --git a/drivers/gpu/drm/bridge/anx78xx/anx78xx_main.c 
b/drivers/gpu/drm/bridge/anx78xx/anx78xx_main.c
new file mode 100644
index 000..0101c23
--- /dev/null
+++ b/drivers/gpu/drm/bridge/anx78xx/anx78xx_main.c
@@ -0,0 +1,334 @@
+/*
+ * Copyright(c) 2015,