Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1283638 > unrolled thread
| Started by | Enric Balletbo i Serra <eballetbo@gmail.com> |
|---|---|
| First post | 2015-12-04 09:40 +0100 |
| Last post | 2015-12-09 13:00 +0100 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
[PATCHv6 0/5] Add initial support for slimport anx78xx Enric Balletbo i Serra <eballetbo@gmail.com> - 2015-12-04 09:40 +0100
[PATCHv6 2/5] hdmi: added functions for MPEG InfoFrames Enric Balletbo i Serra <eballetbo@gmail.com> - 2015-12-04 09:40 +0100
Re: [PATCHv6 5/5] drm: bridge: anx78xx: Add anx78xx driver support by analogix. Dan Carpenter <dan.carpenter@oracle.com> - 2015-12-07 09:20 +0100
Re: [PATCHv6 5/5] drm: bridge: anx78xx: Add anx78xx driver support by analogix. Enric Balletbo Serra <eballetbo@gmail.com> - 2015-12-09 13:00 +0100
| From | Enric Balletbo i Serra <eballetbo@gmail.com> |
|---|---|
| Date | 2015-12-04 09:40 +0100 |
| Subject | [PATCHv6 0/5] Add initial support for slimport anx78xx |
| Message-ID | <qBU4i-h7-15@gated-at.bofh.it> |
Hi all, This is another version of the patch set to introduce the anx7814 transmitter. Any comments are welcome. The following series add initial support for the Slimport ANX7814 transmitter, a ultra-low power Full-HD (1080p60) transmitter designed for portable device. The driver was originally created and based from the work of Junhua Xia from Analogix. This driver is a refactor of the original driver and fixes different coding style lines, and different errors/warnings reported by checkpatch. Also there were things that I noticed that we need to change like: - Convert the numbered GPIO API to the new descriptor based GPIO API. - Review the DT binding - Add missing MODULE_DEVICE_TABLE(of, ...); - Fix Makefiles and Kconfig to build conditionally. - Use SIMPLE_DEV_PM_OPS() instead of the deprecated i2c .suspend and .resume callbacks. - Move to use managed device resources. - Remove dead/unused code. - And others ... Changes since last version: - Add the revision history within the respective patches (requested by Emil) - Add two new patches 01 and 02 as part of the series (requested by Thierry) Enric Balletbo i Serra (5): drm/dp: add DPCD definitions from DP 1.1 hdmi: added functions for MPEG InfoFrames of: Add vendor prefix for Analogix Semiconductor, Inc. devicetree: Add new ANX7814 SlimPort transmitter binding. drm: bridge: anx78xx: Add anx78xx driver support by analogix. .../devicetree/bindings/vendor-prefixes.txt | 1 + .../devicetree/bindings/video/bridge/anx7814.txt | 39 + 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 +++++ drivers/video/hdmi.c | 156 + include/drm/drm_dp_helper.h | 10 + include/linux/hdmi.h | 24 + 14 files changed, 4677 insertions(+) create mode 100644 Documentation/devicetree/bindings/video/bridge/anx7814.txt 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 -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Enric Balletbo i Serra <eballetbo@gmail.com> |
|---|---|
| Date | 2015-12-04 09:40 +0100 |
| Subject | [PATCHv6 2/5] hdmi: added functions for MPEG InfoFrames |
| Message-ID | <qBU4j-h7-35@gated-at.bofh.it> |
| In reply to | #1283638 |
The MPEG Source (MS) InfoFrame is in EIA/CEA-861B. It describes aspects of
the compressed video stream that were used to produce the uncompressed
video.
The patch adds functions to work with MPEG InfoFrames.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Changes since last version (requested by Thierry Redding)
- hdmi_infoframe_pack: Fix missing break
- hdmi_mpeg_picture_get_name: return NULL instead of "Reserved"
- hdmi_mpeg_picture_get_name: use more canonical names "I-Frame", "P-Frame", etc
- hdmi_mpeg_infoframe_unpack: remove braces that aren't needed
- hdmi_vendor_any_infoframe: s/mpeg/MPEG/
drivers/video/hdmi.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/hdmi.h | 24 ++++++++
2 files changed, 180 insertions(+)
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 1626892..47121a6 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -388,6 +388,81 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
}
EXPORT_SYMBOL(hdmi_vendor_infoframe_pack);
+/**
+ * hdmi_mpeg_infoframe_init() - initialize an HDMI MPEG infoframe
+ * @frame: HDMI MPEG infoframe
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int hdmi_mpeg_infoframe_init(struct hdmi_mpeg_infoframe *frame)
+{
+ memset(frame, 0, sizeof(*frame));
+
+ frame->type = HDMI_INFOFRAME_TYPE_MPEG;
+ frame->version = 1;
+ frame->length = HDMI_MPEG_INFOFRAME_SIZE;
+
+ return 0;
+}
+EXPORT_SYMBOL(hdmi_mpeg_infoframe_init);
+
+/**
+ * hdmi_mpeg_infoframe_pack() - write HDMI MPEG infoframe to binary buffer
+ * @frame: HDMI MPEG infoframe
+ * @buffer: destination buffer
+ * @size: size of buffer
+ *
+ * Packs the information contained in the @frame structure into a binary
+ * representation that can be written into the corresponding controller
+ * registers. Also computes the checksum as required by section 5.3.5 of
+ * the HDMI 1.4 specification.
+ *
+ * Returns the number of bytes packed into the binary buffer or a negative
+ * error code on failure.
+ */
+ssize_t hdmi_mpeg_infoframe_pack(struct hdmi_mpeg_infoframe *frame,
+ void *buffer, size_t size)
+{
+ u8 *ptr = buffer;
+ size_t length;
+
+ length = HDMI_INFOFRAME_HEADER_SIZE + frame->length;
+
+ if (size < length)
+ return -ENOSPC;
+
+ memset(buffer, 0, size);
+
+ ptr[0] = frame->type;
+ ptr[1] = frame->version;
+ ptr[2] = frame->length;
+ ptr[3] = 0; /* checksum */
+
+ /* start infoframe payload */
+ ptr += HDMI_INFOFRAME_HEADER_SIZE;
+
+ /*
+ * The MPEG Bit Rate is stored as a 32-bit number and is expressed in
+ * Hertz. MB#0 contains the least significant byte while MB#3 contains
+ * the most significant byte. If the MPEG Bit Rate is unknown or this
+ * field doesn’t apply, then all of the bits in Data Bytes 1-4 shall
+ * be set to 0.
+ */
+ ptr[0] = frame->bitrate & 0x000000ff;
+ ptr[1] = (frame->bitrate & 0x0000ff00) >> 8;
+ ptr[2] = (frame->bitrate & 0x00ff0000) >> 16;
+ ptr[3] = (frame->bitrate & 0xff000000) >> 24;
+
+ ptr[4] = frame->frame_type;
+ if (frame->repeated)
+ ptr[4] |= BIT(4);
+
+ hdmi_infoframe_set_checksum(buffer, length);
+
+ return length;
+}
+EXPORT_SYMBOL(hdmi_mpeg_infoframe_pack);
+
/*
* hdmi_vendor_any_infoframe_pack() - write a vendor infoframe to binary buffer
*/
@@ -435,6 +510,9 @@ hdmi_infoframe_pack(union hdmi_infoframe *frame, void *buffer, size_t size)
length = hdmi_vendor_any_infoframe_pack(&frame->vendor,
buffer, size);
break;
+ case HDMI_INFOFRAME_TYPE_MPEG:
+ length = hdmi_mpeg_infoframe_pack(&frame->mpeg, buffer, size);
+ break;
default:
WARN(1, "Bad infoframe type %d\n", frame->any.type);
length = -EINVAL;
@@ -457,6 +535,8 @@ static const char *hdmi_infoframe_type_get_name(enum hdmi_infoframe_type type)
return "Source Product Description (SPD)";
case HDMI_INFOFRAME_TYPE_AUDIO:
return "Audio";
+ case HDMI_INFOFRAME_TYPE_MPEG:
+ return "MPEG";
}
return "Reserved";
}
@@ -899,6 +979,41 @@ static void hdmi_audio_infoframe_log(const char *level,
frame->downmix_inhibit ? "Yes" : "No");
}
+static const char *hdmi_mpeg_picture_get_name(enum hdmi_mpeg_frame_type type)
+{
+ switch (type) {
+ case HDMI_MPEG_UNKNOWN_FRAME:
+ return "Unknown";
+ case HDMI_MPEG_I_FRAME:
+ return "Intra-coded picture";
+ case HDMI_MPEG_B_FRAME:
+ return "Bi-predictive picture";
+ case HDMI_MPEG_P_FRAME:
+ return "Predicted picture";
+ }
+ return NULL;
+}
+
+/**
+ * hdmi_mpeg_infoframe_log() - log info of HDMI MPEG infoframe
+ * @level: logging level
+ * @dev: device
+ * @frame: HDMI MPEG infoframe
+ */
+static void hdmi_mpeg_infoframe_log(const char *level,
+ struct device *dev,
+ struct hdmi_mpeg_infoframe *frame)
+{
+ hdmi_infoframe_log_header(level, dev,
+ (struct hdmi_any_infoframe *)frame);
+
+ hdmi_log(" bit rate: %d Hz\n", frame->bitrate);
+ hdmi_log(" frame type: %s\n",
+ hdmi_mpeg_picture_get_name(frame->frame_type));
+ hdmi_log(" repeated frame: %s\n",
+ frame->repeated ? "Yes" : "No");
+}
+
static const char *
hdmi_3d_structure_get_name(enum hdmi_3d_structure s3d_struct)
{
@@ -987,6 +1102,9 @@ void hdmi_infoframe_log(const char *level,
case HDMI_INFOFRAME_TYPE_VENDOR:
hdmi_vendor_any_infoframe_log(level, dev, &frame->vendor);
break;
+ case HDMI_INFOFRAME_TYPE_MPEG:
+ hdmi_mpeg_infoframe_log(level, dev, &frame->mpeg);
+ break;
}
}
EXPORT_SYMBOL(hdmi_infoframe_log);
@@ -1138,6 +1256,41 @@ static int hdmi_audio_infoframe_unpack(struct hdmi_audio_infoframe *frame,
}
/**
+ * hdmi_mpeg_infoframe_unpack() - unpack binary buffer to a HDMI MPEG infoframe
+ * @buffer: source buffer
+ * @frame: HDMI MPEG infoframe
+ *
+ * Unpacks the information contained in binary @buffer into a structured
+ * @frame of the HDMI MPEG information frame. Also verifies the checksum as
+ * required by section 5.3.5 of the HDMI 1.4 specification.
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+static int hdmi_mpeg_infoframe_unpack(struct hdmi_mpeg_infoframe *frame,
+ void *buffer)
+{
+ u8 *ptr = buffer;
+
+ if (ptr[0] != HDMI_INFOFRAME_TYPE_MPEG ||
+ ptr[1] != 1 ||
+ ptr[2] != HDMI_MPEG_INFOFRAME_SIZE)
+ return -EINVAL;
+
+ if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(MPEG)) != 0)
+ return -EINVAL;
+
+ ptr += HDMI_INFOFRAME_HEADER_SIZE;
+
+ frame->bitrate = (ptr[3] << 24) | (ptr[2] << 16) |
+ (ptr[1] << 8) | ptr[0];
+
+ frame->frame_type = ptr[4] & 0x03;
+ frame->repeated = ptr[4] & BIT(4) ? true : false;
+
+ return 0;
+}
+
+/**
* hdmi_vendor_infoframe_unpack() - unpack binary buffer to a HDMI vendor infoframe
* @buffer: source buffer
* @frame: HDMI Vendor infoframe
@@ -1234,6 +1387,9 @@ int hdmi_infoframe_unpack(union hdmi_infoframe *frame, void *buffer)
case HDMI_INFOFRAME_TYPE_VENDOR:
ret = hdmi_vendor_any_infoframe_unpack(&frame->vendor, buffer);
break;
+ case HDMI_INFOFRAME_TYPE_MPEG:
+ ret = hdmi_mpeg_infoframe_unpack(&frame->mpeg, buffer);
+ break;
default:
ret = -EINVAL;
break;
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index e974420..c033554 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -32,11 +32,13 @@ enum hdmi_infoframe_type {
HDMI_INFOFRAME_TYPE_AVI = 0x82,
HDMI_INFOFRAME_TYPE_SPD = 0x83,
HDMI_INFOFRAME_TYPE_AUDIO = 0x84,
+ HDMI_INFOFRAME_TYPE_MPEG = 0x85,
};
#define HDMI_IEEE_OUI 0x000c03
#define HDMI_INFOFRAME_HEADER_SIZE 4
#define HDMI_AVI_INFOFRAME_SIZE 13
+#define HDMI_MPEG_INFOFRAME_SIZE 10
#define HDMI_SPD_INFOFRAME_SIZE 25
#define HDMI_AUDIO_INFOFRAME_SIZE 10
@@ -297,6 +299,26 @@ int hdmi_vendor_infoframe_init(struct hdmi_vendor_infoframe *frame);
ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
void *buffer, size_t size);
+enum hdmi_mpeg_frame_type {
+ HDMI_MPEG_UNKNOWN_FRAME = 0x00,
+ HDMI_MPEG_I_FRAME = 0x01,
+ HDMI_MPEG_B_FRAME = 0x02,
+ HDMI_MPEG_P_FRAME = 0x03,
+};
+
+struct hdmi_mpeg_infoframe {
+ enum hdmi_infoframe_type type;
+ unsigned char version;
+ unsigned char length;
+ u32 bitrate;
+ enum hdmi_mpeg_frame_type frame_type;
+ bool repeated;
+};
+
+int hdmi_mpeg_infoframe_init(struct hdmi_mpeg_infoframe *frame);
+ssize_t hdmi_mpeg_infoframe_pack(struct hdmi_mpeg_infoframe *frame,
+ void *buffer, size_t size);
+
union hdmi_vendor_any_infoframe {
struct {
enum hdmi_infoframe_type type;
@@ -314,6 +336,7 @@ union hdmi_vendor_any_infoframe {
* @spd: spd infoframe
* @vendor: union of all vendor infoframes
* @audio: audio infoframe
+ * @mpeg: MPEG infoframe
*
* This is used by the generic pack function. This works since all infoframes
* have the same header which also indicates which type of infoframe should be
@@ -325,6 +348,7 @@ union hdmi_infoframe {
struct hdmi_spd_infoframe spd;
union hdmi_vendor_any_infoframe vendor;
struct hdmi_audio_infoframe audio;
+ struct hdmi_mpeg_infoframe mpeg;
};
ssize_t
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dan Carpenter <dan.carpenter@oracle.com> |
|---|---|
| Date | 2015-12-07 09:20 +0100 |
| Subject | Re: [PATCHv6 5/5] drm: bridge: anx78xx: Add anx78xx driver support by analogix. |
| Message-ID | <qCZbA-1Xo-3@gated-at.bofh.it> |
| In reply to | #1283638 |
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 = &anx78xx->client->dev;
> +
> + errcnt = 150;
> + while (errcnt--) {
> + sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, &val);
> + 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, &val);
> + 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,
> + &val);
> + 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, &val);
> + 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 = &anx78xx->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, &last_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++) {
> + if (sp_seg_edid_read(anx78xx, count / 2,
> + offset))
> + return -1;
> + offset = offset + 0x10;
> + }
> + break;
> + default:
> + break;
Is there something which complains if you leave out the default case
statement? It's not reachable.
> + }
> + }
> +
> + sp_reset_aux(anx78xx);
> +
> + if (!drm_edid_block_valid(sp.edid_blocks, 0, true, NULL)) {
> + dev_err(dev, "EDID block is invalid\n");
> + return -1;
> + }
> +
> + sp_dp_read_bytes_from_dpcd(anx78xx, DP_TEST_REQUEST, 1, &val);
> + if (val & DP_TEST_LINK_EDID_READ) {
> + dev_dbg(dev, "EDID test requested\n");
> + val = sp_edid_block_checksum(sp.edid_blocks);
> + dev_dbg(dev, "EDID checksum is %d\n", val);
> + sp_dp_write_bytes_to_dpcd(anx78xx, DP_TEST_EDID_CHECKSUM, 1,
> + &val);
> + sp.tx_test_edid = true;
> + val = DP_TEST_EDID_CHECKSUM_WRITE;
> + sp_dp_write_bytes_to_dpcd(anx78xx, DP_TEST_RESPONSE, 1, &val);
> + }
> +
> + return 0;
> +}
> +
> +static bool sp_check_with_pre_edid(struct anx78xx *anx78xx)
> +{
> + struct device *dev = &anx78xx->client->dev;
> + u8 i;
> + u8 buf[16];
> + bool ret = false;
> +
> + 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, 0x03);
> +
> + if (sp_wait_aux_op_finish(anx78xx))
> + goto return_point;
> +
> + sp_addronly_set(anx78xx, false);
> +
> + if (sp_edid_read(anx78xx, 0x70, buf))
> + goto return_point;
> +
> + for (i = 0; i < 16; i++) {
> + if (sp.edid_blocks[0x70 + i] != buf[i]) {
> + dev_dbg(dev, "%s\n",
> + "different checksum and blocks num\n");
> + goto return_point;
> + }
> + }
Can you just say:
if (memcmp(&sp.edid_blocks[0x70], buf, 16) != 0) {
dev_dbg(dev, "different checksum and blocks num\n");
goto return_point;
}
> +
> + if (sp_edid_read(anx78xx, 0x08, buf))
> + goto return_point;
> +
> + for (i = 0; i < 16; i++) {
> + if (sp.edid_blocks[i + 8] != buf[i]) {
> + dev_dbg(dev, "different edid information\n");
> + goto return_point;
> + }
> + }
> +
> + ret = true;
> +return_point:
> + sp_reset_aux(anx78xx);
> +
> + return ret;
> +}
> +
[ snip ]
> +static bool sp_config_video_output(struct anx78xx *anx78xx)
> +{
> + struct device *dev = &anx78xx->client->dev;
> + u8 val;
> +
> + switch (sp.tx_vo_state) {
> + default:
> + case VO_WAIT_VIDEO_STABLE:
> + sp_reg_read(anx78xx, RX_P0, SP_SYSTEM_STATUS_REG, &val);
> + if ((val & SP_TMDS_DE_DET) && (val & SP_TMDS_CLOCK_DET)) {
> + if (sp_tx_bw_lc_sel(anx78xx))
> + return false;
> + sp_enable_video_input(anx78xx, false);
> + sp_hdmi_new_avi_int(anx78xx);
> + sp_reg_read(anx78xx, RX_P0,
> + SP_PACKET_RECEIVING_STATUS_REG, &val);
> + if (val & SP_VSI_RCVD)
> + sp_hdmi_new_vsi_int(anx78xx);
> + sp_enable_video_input(anx78xx, true);
> + sp.tx_vo_state = VO_WAIT_TX_VIDEO_STABLE;
> + } else {
> + dev_dbg(dev, "HDMI input video not stable!\n");
> + break;
> + }
> + /* fallthrough */
> + case VO_WAIT_TX_VIDEO_STABLE:
> + /*
> + * The flag is write clear and can be latched from last
> + * status. So the first read and write is to clear the
> + * previous status.
> + */
> + sp_reg_read(anx78xx, TX_P0, SP_DP_SYSTEM_CTRL_BASE + 2, &val);
> + sp_reg_write(anx78xx, TX_P0, SP_DP_SYSTEM_CTRL_BASE + 2, val);
> +
> + sp_reg_read(anx78xx, TX_P0, SP_DP_SYSTEM_CTRL_BASE + 2, &val);
> + if (val & SP_CHA_STA) {
> + dev_dbg(dev, "stream clock not stable!\n");
> + break;
> + } else {
No need for the else statement. Pull it in one indent level.
> + /*
> + * The flag is write clear and can be latched from
> + * last status. So the first read and write is to
> + * clear the previous status.
> + */
> + sp_reg_read(anx78xx, TX_P0,
> + SP_DP_SYSTEM_CTRL_BASE + 3,
> + &val);
> + sp_reg_write(anx78xx, TX_P0,
> + SP_DP_SYSTEM_CTRL_BASE + 3,
> + val);
> +
> + sp_reg_read(anx78xx, TX_P0,
> + SP_DP_SYSTEM_CTRL_BASE + 3,
> + &val);
> + if (val & SP_STRM_VALID) {
> + if (sp.tx_test_lt)
> + sp.tx_test_lt = false;
> + sp.tx_vo_state = VO_FINISH;
> + } else {
> + dev_err(dev, "video stream not valid!\n");
> + break;
> + }
> + }
> + /* fallthrough */
> + case VO_FINISH:
> + sp_block_power_ctrl(anx78xx, SP_TX_PWR_AUDIO, false);
> + sp_hdmi_mute_video(anx78xx, false);
> + sp_video_mute(anx78xx, false);
> + sp_show_information(anx78xx);
> + return true;
> + }
> +
> + return false;
> +}
> +
[ snip ]
> +static void sp_config_audio(struct anx78xx *anx78xx)
> +{
> + int i;
> + u8 val;
> +
> + sp_block_power_ctrl(anx78xx, SP_TX_PWR_AUDIO, true);
> +
> + sp_reg_read(anx78xx, TX_P0, SP_DP_MAIN_LINK_BW_SET_REG, &val);
> + if (val & SP_INITIAL_SLIM_M_AUD_SEL)
> + if (sp_calculate_audio_m_value(anx78xx))
> + return;
Combine these:
if ((val & SP_INITIAL_SLIM_M_AUD_SEL) &&
sp_calculate_audio_m_value(anx78xx))
return;
> +
> + sp_reg_clear_bits(anx78xx, TX_P1, SP_AUD_INTERFACE_CTRL0_REG,
> + SP_AUD_INTERFACE_DISABLE);
> +
> + sp_reg_set_bits(anx78xx, TX_P1, SP_AUD_INTERFACE_CTRL2_REG,
> + SP_M_AUD_ADJUST_ST);
> +
> + sp_reg_read(anx78xx, RX_P0, SP_HDMI_STATUS_REG, &val);
> + if (val & SP_HDMI_AUD_LAYOUT)
> + sp_reg_set_bits(anx78xx, TX_P2, SP_AUD_CH_STATUS_BASE + 5,
> + SP_I2S_CH_NUM_8 | SP_AUDIO_LAYOUT);
> + else
> + sp_reg_clear_bits(anx78xx, TX_P2, SP_AUD_CH_STATUS_BASE + 5,
> + SP_I2S_CHANNEL_NUM_MASK | SP_AUDIO_LAYOUT);
> +
> + /* transfer audio channel status from HDMI Rx to Slimport Tx */
> + for (i = 1; i <= SP_AUD_CH_STATUS_REG_NUM; i++) {
> + sp_reg_read(anx78xx, RX_P0, SP_AUD_SPDIF_CH_STATUS_BASE + i,
> + &val);
> + sp_reg_write(anx78xx, TX_P2, SP_AUD_CH_STATUS_BASE + i,
> + val);
> + }
Either this loop is off by one or the loop in sp_hdmi_audio_samplechg_int()
is off by one. Also just call that function instead of re-implimenting
it here.
> +
> + /* enable audio */
> + sp_enable_audio_output(anx78xx, true);
> +}
> +
> +static bool sp_config_audio_output(struct anx78xx *anx78xx)
> +{
> + u8 val;
> +
> + switch (sp.tx_ao_state) {
> + default:
> + case AO_INIT:
> + case AO_CTS_RCV_INT:
> + case AO_AUDIO_RCV_INT:
> + sp_reg_read(anx78xx, RX_P0, SP_HDMI_STATUS_REG, &val);
> + if (!val & SP_HDMI_MODE) {
This is a precendence error. It should be:
if (!(val & SP_HDMI_MODE)) {
> + sp.tx_ao_state = AO_INIT;
> + return true;
> + }
> + break;
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Enric Balletbo Serra <eballetbo@gmail.com> |
|---|---|
| Date | 2015-12-09 13:00 +0100 |
| Subject | Re: [PATCHv6 5/5] drm: bridge: anx78xx: Add anx78xx driver support by analogix. |
| Message-ID | <qDLzz-7Ue-5@gated-at.bofh.it> |
| In reply to | #1285073 |
Hi Dan,
Many thanks for your comments.
2015-12-07 9:09 GMT+01:00 Dan Carpenter <dan.carpenter@oracle.com>:
> 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 = &anx78xx->client->dev;
>> +
>> + errcnt = 150;
>> + while (errcnt--) {
>> + sp_reg_read(anx78xx, TX_P0, SP_DP_AUX_CH_CTRL2_REG, &val);
>> + 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, &val);
>> + 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,
>> + &val);
>> + 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, &val);
>> + 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 = &anx78xx->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, &last_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++) {
>> + if (sp_seg_edid_read(anx78xx, count / 2,
>> + offset))
>> + return -1;
>> + offset = offset + 0x10;
>> + }
>> + break;
>> + default:
>> + break;
>
> Is there something which complains if you leave out the default case
> statement? It's not reachable.
>
I left out the default case as is not reachable.
>> + }
>> + }
>> +
>> + sp_reset_aux(anx78xx);
>> +
>> + if (!drm_edid_block_valid(sp.edid_blocks, 0, true, NULL)) {
>> + dev_err(dev, "EDID block is invalid\n");
>> + return -1;
>> + }
>> +
>> + sp_dp_read_bytes_from_dpcd(anx78xx, DP_TEST_REQUEST, 1, &val);
>> + if (val & DP_TEST_LINK_EDID_READ) {
>> + dev_dbg(dev, "EDID test requested\n");
>> + val = sp_edid_block_checksum(sp.edid_blocks);
>> + dev_dbg(dev, "EDID checksum is %d\n", val);
>> + sp_dp_write_bytes_to_dpcd(anx78xx, DP_TEST_EDID_CHECKSUM, 1,
>> + &val);
>> + sp.tx_test_edid = true;
>> + val = DP_TEST_EDID_CHECKSUM_WRITE;
>> + sp_dp_write_bytes_to_dpcd(anx78xx, DP_TEST_RESPONSE, 1, &val);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static bool sp_check_with_pre_edid(struct anx78xx *anx78xx)
>> +{
>> + struct device *dev = &anx78xx->client->dev;
>> + u8 i;
>> + u8 buf[16];
>> + bool ret = false;
>> +
>> + 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, 0x03);
>> +
>> + if (sp_wait_aux_op_finish(anx78xx))
>> + goto return_point;
>> +
>> + sp_addronly_set(anx78xx, false);
>> +
>> + if (sp_edid_read(anx78xx, 0x70, buf))
>> + goto return_point;
>> +
>> + for (i = 0; i < 16; i++) {
>> + if (sp.edid_blocks[0x70 + i] != buf[i]) {
>> + dev_dbg(dev, "%s\n",
>> + "different checksum and blocks num\n");
>> + goto return_point;
>> + }
>> + }
>
> Can you just say:
>
> if (memcmp(&sp.edid_blocks[0x70], buf, 16) != 0) {
> dev_dbg(dev, "different checksum and blocks num\n");
> goto return_point;
> }
>
I will change
>
>> +
>> + if (sp_edid_read(anx78xx, 0x08, buf))
>> + goto return_point;
>> +
>> + for (i = 0; i < 16; i++) {
>> + if (sp.edid_blocks[i + 8] != buf[i]) {
>> + dev_dbg(dev, "different edid information\n");
>> + goto return_point;
>> + }
>> + }
>> +
>> + ret = true;
>> +return_point:
>> + sp_reset_aux(anx78xx);
>> +
>> + return ret;
>> +}
>> +
>
> [ snip ]
>
>> +static bool sp_config_video_output(struct anx78xx *anx78xx)
>> +{
>> + struct device *dev = &anx78xx->client->dev;
>> + u8 val;
>> +
>> + switch (sp.tx_vo_state) {
>> + default:
>> + case VO_WAIT_VIDEO_STABLE:
>> + sp_reg_read(anx78xx, RX_P0, SP_SYSTEM_STATUS_REG, &val);
>> + if ((val & SP_TMDS_DE_DET) && (val & SP_TMDS_CLOCK_DET)) {
>> + if (sp_tx_bw_lc_sel(anx78xx))
>> + return false;
>> + sp_enable_video_input(anx78xx, false);
>> + sp_hdmi_new_avi_int(anx78xx);
>> + sp_reg_read(anx78xx, RX_P0,
>> + SP_PACKET_RECEIVING_STATUS_REG, &val);
>> + if (val & SP_VSI_RCVD)
>> + sp_hdmi_new_vsi_int(anx78xx);
>> + sp_enable_video_input(anx78xx, true);
>> + sp.tx_vo_state = VO_WAIT_TX_VIDEO_STABLE;
>> + } else {
>> + dev_dbg(dev, "HDMI input video not stable!\n");
>> + break;
>> + }
>> + /* fallthrough */
>> + case VO_WAIT_TX_VIDEO_STABLE:
>> + /*
>> + * The flag is write clear and can be latched from last
>> + * status. So the first read and write is to clear the
>> + * previous status.
>> + */
>> + sp_reg_read(anx78xx, TX_P0, SP_DP_SYSTEM_CTRL_BASE + 2, &val);
>> + sp_reg_write(anx78xx, TX_P0, SP_DP_SYSTEM_CTRL_BASE + 2, val);
>> +
>> + sp_reg_read(anx78xx, TX_P0, SP_DP_SYSTEM_CTRL_BASE + 2, &val);
>> + if (val & SP_CHA_STA) {
>> + dev_dbg(dev, "stream clock not stable!\n");
>> + break;
>> + } else {
>
>
> No need for the else statement. Pull it in one indent level.
>
Ack
>> + /*
>> + * The flag is write clear and can be latched from
>> + * last status. So the first read and write is to
>> + * clear the previous status.
>> + */
>> + sp_reg_read(anx78xx, TX_P0,
>> + SP_DP_SYSTEM_CTRL_BASE + 3,
>> + &val);
>> + sp_reg_write(anx78xx, TX_P0,
>> + SP_DP_SYSTEM_CTRL_BASE + 3,
>> + val);
>> +
>> + sp_reg_read(anx78xx, TX_P0,
>> + SP_DP_SYSTEM_CTRL_BASE + 3,
>> + &val);
>> + if (val & SP_STRM_VALID) {
>> + if (sp.tx_test_lt)
>> + sp.tx_test_lt = false;
>> + sp.tx_vo_state = VO_FINISH;
>> + } else {
>> + dev_err(dev, "video stream not valid!\n");
>> + break;
>> + }
>> + }
>> + /* fallthrough */
>> + case VO_FINISH:
>> + sp_block_power_ctrl(anx78xx, SP_TX_PWR_AUDIO, false);
>> + sp_hdmi_mute_video(anx78xx, false);
>> + sp_video_mute(anx78xx, false);
>> + sp_show_information(anx78xx);
>> + return true;
>> + }
Changed
>> +
>> + return false;
>> +}
>> +
>
> [ snip ]
>
>> +static void sp_config_audio(struct anx78xx *anx78xx)
>> +{
>> + int i;
>> + u8 val;
>> +
>> + sp_block_power_ctrl(anx78xx, SP_TX_PWR_AUDIO, true);
>> +
>> + sp_reg_read(anx78xx, TX_P0, SP_DP_MAIN_LINK_BW_SET_REG, &val);
>> + if (val & SP_INITIAL_SLIM_M_AUD_SEL)
>> + if (sp_calculate_audio_m_value(anx78xx))
>> + return;
>
> Combine these:
>
> if ((val & SP_INITIAL_SLIM_M_AUD_SEL) &&
> sp_calculate_audio_m_value(anx78xx))
> return;
>
Ok
>> +
>> + sp_reg_clear_bits(anx78xx, TX_P1, SP_AUD_INTERFACE_CTRL0_REG,
>> + SP_AUD_INTERFACE_DISABLE);
>> +
>> + sp_reg_set_bits(anx78xx, TX_P1, SP_AUD_INTERFACE_CTRL2_REG,
>> + SP_M_AUD_ADJUST_ST);
>> +
>> + sp_reg_read(anx78xx, RX_P0, SP_HDMI_STATUS_REG, &val);
>> + if (val & SP_HDMI_AUD_LAYOUT)
>> + sp_reg_set_bits(anx78xx, TX_P2, SP_AUD_CH_STATUS_BASE + 5,
>> + SP_I2S_CH_NUM_8 | SP_AUDIO_LAYOUT);
>> + else
>> + sp_reg_clear_bits(anx78xx, TX_P2, SP_AUD_CH_STATUS_BASE + 5,
>> + SP_I2S_CHANNEL_NUM_MASK | SP_AUDIO_LAYOUT);
>> +
>> + /* transfer audio channel status from HDMI Rx to Slimport Tx */
>> + for (i = 1; i <= SP_AUD_CH_STATUS_REG_NUM; i++) {
>> + sp_reg_read(anx78xx, RX_P0, SP_AUD_SPDIF_CH_STATUS_BASE + i,
>> + &val);
>> + sp_reg_write(anx78xx, TX_P2, SP_AUD_CH_STATUS_BASE + i,
>> + val);
>> + }
>
> Either this loop is off by one or the loop in sp_hdmi_audio_samplechg_int()
> is off by one. Also just call that function instead of re-implimenting
> it here.
>
Right, I'll fix that
>> +
>> + /* enable audio */
>> + sp_enable_audio_output(anx78xx, true);
>> +}
>> +
>> +static bool sp_config_audio_output(struct anx78xx *anx78xx)
>> +{
>> + u8 val;
>> +
>> + switch (sp.tx_ao_state) {
>> + default:
>> + case AO_INIT:
>> + case AO_CTS_RCV_INT:
>> + case AO_AUDIO_RCV_INT:
>> + sp_reg_read(anx78xx, RX_P0, SP_HDMI_STATUS_REG, &val);
>> + if (!val & SP_HDMI_MODE) {
>
> This is a precendence error. It should be:
>
Ack
> if (!(val & SP_HDMI_MODE)) {
>
>> + sp.tx_ao_state = AO_INIT;
>> + return true;
>> + }
>> + break;
>
> regards,
> dan carpenter
I'll wait a bit more to see if anyone else does more comments and then
I'll send another version with the changes you suggested. Thanks.
Regards,
Enric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web