Claude review: drm/bridge: add support for lontium lt8713sx bridge driver

Claude review: drm/bridge: add support for lontium lt8713sx bridge driver

[Claude Code Review Bot]

Patch Review

**`bridge.funcs` assigned redundantly:**
`devm_drm_bridge_alloc()` already sets `bridge->funcs = funcs` internally. The manual assignment at line is unnecessary and misleading:

```c
+	lt8713sx->bridge.funcs = &lt8713sx_bridge_funcs;
```

Remove this line.

**`enable_gpio` acquired but never used:**
The driver requests an "enable" GPIO and stores it, but never reads or toggles it after acquisition:

```c
+	lt8713sx->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
```

If the purpose is just to drive it high at probe time, that side effect relies on the `GPIOD_OUT_HIGH` flag, which is subtle and undocumented. This should either be used explicitly (e.g., in enable/disable callbacks) or removed. It's also not documented in the DT binding.

**`bridge_attach` signature may be wrong:**
On the current drm-next tree, the attach callback signature includes the encoder parameter:
```c
int (*attach)(struct drm_bridge *bridge, struct drm_encoder *encoder,
              enum drm_bridge_attach_flags flags);
```
The patch matches this, but the driver only passes through to `drm_bridge_attach`. This is fine but should be verified against the exact tree it's targeting.

**No bridge `ops` set — driver does nothing as a bridge:**
The bridge has no `ops` flags set (no `DRM_BRIDGE_OP_DETECT`, `DRM_BRIDGE_OP_EDID`, etc.) and the only bridge function is `attach`. This means the bridge does nothing in the display pipeline — it just chains to `next_bridge`. This raises the question of whether this needs to be a `drm_bridge` driver at all, or whether a simpler i2c driver would suffice for firmware loading.

**Sysfs attribute naming and approach:**
```c
+static DEVICE_ATTR_WO(lt8713sx_firmware);
```

This creates a sysfs attribute named `lt8713sx_firmware`. The naming convention is unusual — sysfs attributes typically use shorter generic names (e.g., `firmware_update` or `update_fw`). More importantly, using a raw sysfs write-triggered firmware flash is not the preferred approach. The kernel has the firmware upload API (`firmware_upload`) in `include/linux/firmware.h` designed exactly for this use case, or firmware can be loaded automatically at probe time.

The `store` function ignores the buffer content entirely:
```c
+static ssize_t lt8713sx_firmware_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t len)
+{
+	struct lt8713sx *lt8713sx = dev_get_drvdata(dev);
+	int ret;
+
+	ret = lt8713sx_firmware_update(lt8713sx);
```

Writing any value triggers a firmware flash. This is fragile — there's no validation, no idempotency check.

**Integer type issues with `sz_12k`:**
```c
+	u64 sz_12k = 12 * SZ_1K;
```

This is `u64` for a value of 12288 — completely unnecessary. Just use a `#define` or plain `unsigned int`. The subsequent division by `sz_12k` then does a 64-bit divide which is expensive on 32-bit architectures (and may need `do_div`):

```c
+	lt8713sx->bank_num = (lt8713sx->fw->size - SZ_64K + sz_12k - 1) / sz_12k;
```

This will break on 32-bit builds because it does a 64-bit divide. Use `DIV_ROUND_UP()` with appropriate types.

**`lt8713sx_write_data` always returns 0:**
```c
+static int lt8713sx_write_data(struct lt8713sx *lt8713sx, const u8 *data, u64 filesize)
+{
+	...
+	return 0;
+}
```

The function never checks `regmap_write()` return values and always returns success. If any register write fails during firmware flashing, the failure is silently ignored. All the `regmap_write` calls in this driver are unchecked — this is a pervasive issue throughout the driver.

**`lt8713sx_block_erase` has unbounded-ish busy wait:**
```c
+		while (1) {
+			flash_status = lt8713sx_read_flash_status(lt8713sx);
+			if ((flash_status & 0x01) == 0)
+				break;
+			if (i > 50)
+				break;
+			i++;
+			msleep(50);
+		}
```

The loop breaks after 50 iterations but doesn't report an error if the flash never becomes ready. A block erase that fails silently will lead to corrupted firmware.

**Firmware size check is off-by-one:**
```c
+	if (lt8713sx->fw->size > SZ_256K - 1) {
```

This allows firmware of size exactly `SZ_256K - 1` (262143 bytes). If the intent is to allow up to 256KB, use `>= SZ_256K` or `> SZ_256K`. The `- 1` suggests confusion.

**Main firmware copy is also off-by-one:**
```c
+	memcpy(lt8713sx->fw_buffer, lt8713sx->fw->data, SZ_64K - 1);
```

This copies 65535 bytes instead of 65536. This means the last byte of the first 64K is not copied from the firmware file — it's left as 0xFF (from the memset). The CRC is then computed over 65535 bytes and stored at offset `SZ_64K - 1`. This seems intentional (the CRC replaces the last byte), but it means the firmware format requires the last byte of the first 64K block to be reserved. This should be documented.

**`bank_crc_value` array size is 17 but not bounds-checked:**
```c
+	u32 bank_crc_value[17];
+	...
+	lt8713sx->bank_num = (lt8713sx->fw->size - SZ_64K + sz_12k - 1) / sz_12k;
+	...
+	for (int i = 0; i < lt8713sx->bank_num; i++) {
+		lt8713sx->bank_crc_value[i] = ...
```

The maximum number of 12K banks in a 192KB region (256K - 64K) is `(192*1024) / (12*1024) = 16`, so `bank_crc_value[17]` is sufficient. However, there is no explicit bounds check on `bank_num` before indexing into the array. If the firmware size calculation yields a `bank_num > 17`, this is a buffer overflow.

**`i2c_device_id` should be `const`:**
```c
+static struct i2c_device_id lt8713sx_id[] = {
```

Should be:
```c
+static const struct i2c_device_id lt8713sx_id[] = {
```

**`i2c_device_id` entry should not include the vendor prefix:**
```c
+	{ "lontium,lt8713sx", 0 },
```

The `i2c_device_id` `.name` field should be just `"lt8713sx"`, not `"lontium,lt8713sx"`. The compatible string in `of_device_id` uses the vendor prefix, but the I2C ID table does not. Also, the trailing `, 0` is unnecessary with modern kernel conventions.

**`crc8_populate_msb` called on a global table in probe:**
```c
+DECLARE_CRC8_TABLE(lt8713sx_crc_table);
+...
+	crc8_populate_msb(lt8713sx_crc_table, 0x31);
```

This is a module-level static array populated in `probe()`. If multiple devices probe (unlikely but possible), there's a race on table initialization. This should be done in `module_init` or with a `once` guard. Alternatively, use a `CRC8_INIT_TABLE` or populate it at compile time.

**`drm_bridge_remove` in remove but no unplug support:**
```c
+static void lt8713sx_remove(struct i2c_client *client)
+{
+	struct lt8713sx *lt8713sx = i2c_get_clientdata(client);
+	drm_bridge_remove(&lt8713sx->bridge);
+}
```

Modern bridge drivers using `devm_drm_bridge_alloc` should use `devm_drm_bridge_add` instead of the manual `drm_bridge_add`/`drm_bridge_remove` pair, which would eliminate the need for a `remove` callback entirely.

**`lt8713sx_load_bank_fw_to_sram` takes `u64 addr` unnecessarily:**
```c
+static void lt8713sx_load_bank_fw_to_sram(struct lt8713sx *lt8713sx, u64 addr)
```

The address fits in 24 bits (max 0x040000). Using `u64` is misleading and wasteful. Use `u32`.

**No `DRM_BRIDGE_ATTACH_NO_CONNECTOR` handling:**
The `attach` callback doesn't check for or pass through `DRM_BRIDGE_ATTACH_NO_CONNECTOR` flags. Modern bridge drivers should support this flag.

---
Generated by Claude Code Patch Reviewer

[PATCH v5 0/2] Add lontium lt8713sx bridge driver

[Vishnu Saini]

The lt8713sx is a Type-C/DP1.4 to Type-C/DP1.4/HDMI2.0 converter,
with three configurable DP1.4/HDMI2.0/DP++ output interfaces and
audio output interface.

This series provides bridge driver and dt bindings for lt8713sx.
The driver is required for firmware upgrade and enabling the bridge chip.

Signed-off-by: Vishnu Saini <vishnu.saini@oss.qualcomm.com>
---
Changes in v5:
- Commit msg changed a bit
- Added Author Maintainer after confirmation from Lontium
- Link to v4: https://lore.kernel.org/r/20260224-lt8713sx-bridge-driver-v4-0-b5603f5458d8@oss.qualcomm.com

Changes in v4:
- Improved crc calculation, calculated on padded buffer instead of bit
  by bit.
- Fixed brm bridge chain, using single drm_bridge as bridge chip itself will
  take care of providing all edp outputs from single input.
- Used guard mutex where needed.
- Link to v3: https://lore.kernel.org/r/20251228-lt8713sx-bridge-driver-v3-0-9169fbef0e5b@oss.qualcomm.com

Changes in v3:
- Used linux/sizes.h header for size definations.
- Used linux/crc8.h for CRC calculation
- Added Basic drm_bridge changes to support corresponding ports handeling in dt
- Ran coccinelle, smatch and sparse checkpatch.pl tools to improve code quality.
- Link to v2: https://lore.kernel.org/r/20251118-lt8713sx-bridge-driver-v2-0-25ad49280a11@oss.qualcomm.com

Changes in v2:
- Addressed review comments from V1, majorly:
- Fixed DCO chain.
- Added supply in bindings.
- Handeled deferred probe in lt8713sx driver probe.
- Link to v1: https://lore.kernel.org/r/20251115-lt8713sx-bridge-driver-v1-0-bd5a1c1c730a@oss.qualcomm.com

---
Vishnu Saini (2):
      dt-bindings: bridge: lt8713sx: Add bindings
      drm/bridge: add support for lontium lt8713sx bridge driver

 .../bindings/display/bridge/lontium,lt8713sx.yaml  | 113 ++++
 drivers/gpu/drm/bridge/Kconfig                     |  10 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/lontium-lt8713sx.c          | 598 +++++++++++++++++++++
 4 files changed, 722 insertions(+)
---
base-commit: de0d6e19d2ef33ba34be2467ffdf3595da5f5326
change-id: 20251115-lt8713sx-bridge-driver-32704455bc9a

Best regards,
-- 
Vishnu Saini <vishnu.saini@oss.qualcomm.com>

[PATCH v5 1/2] dt-bindings: bridge: lt8713sx: Add bindings

[Vishnu Saini]

Add bindings for lt8713sx.

Co-developed-by: Prahlad Valluru <vvalluru@qti.qualcomm.com>
Signed-off-by: Prahlad Valluru <vvalluru@qti.qualcomm.com>
Signed-off-by: Vishnu Saini <vishnu.saini@oss.qualcomm.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Cc: Simon Zhu <xmzhu@lontium.corp-partner.google.com>
---
 .../bindings/display/bridge/lontium,lt8713sx.yaml  | 113 +++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/lontium,lt8713sx.yaml b/Documentation/devicetree/bindings/display/bridge/lontium,lt8713sx.yaml
new file mode 100644
index 000000000000..a5ba4db11a7c
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/lontium,lt8713sx.yaml
@@ -0,0 +1,113 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/lontium,lt8713sx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Lontium LT8713SX Type-C/DP1.4 to Type-C/DP1.4/HDMI2.0/DP++ bridge-hub
+
+maintainers:
+  - Vishnu Saini <vishnu.saini@oss.qualcomm.com>
+
+description:
+  The Lontium LT8713SX is a Type-C/DP1.4 to Type-C/DP1.4/HDMI2.0 converter
+  that integrates one DP input and up to three configurable output interfaces
+  (DP1.4 / HDMI2.0 / DP++), with SST/MST functionality and audio support.
+
+properties:
+  compatible:
+    enum:
+      - lontium,lt8713sx
+
+  reg:
+    maxItems: 1
+
+  vcc-supply:
+    description: Regulator for 3.3V vcc.
+
+  vdd-supply:
+    description: Regulator for 1.1V vdd.
+
+  reset-gpios:
+    description: GPIO connected to active low RESET pin.
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          DP port for DP input from soc to bridge chip
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          DP port for DP output from bridge
+
+      port@2:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          Additional DP port for DP output from bridge
+
+      port@3:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          Additional DP port for DP output from bridge
+
+    required:
+      - port@0
+
+required:
+  - compatible
+  - reg
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        bridge@4f {
+            compatible = "lontium,lt8713sx";
+            reg = <0x4f>;
+            reset-gpios = <&tlmm 6 GPIO_ACTIVE_LOW>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+                    lt8713sx_dp_in: endpoint {
+                        remote-endpoint = <&mdss_dp0_out>;
+                    };
+                };
+
+                port@1 {
+                    reg = <1>;
+                    lt8713sx_dp0_out: endpoint {
+                        remote-endpoint = <&dp0_connector_in>;
+                    };
+                };
+
+                port@2 {
+                    reg = <2>;
+                    lt8713sx_dp1_out: endpoint {
+                        remote-endpoint = <&dp1_connector_in>;
+                    };
+                };
+
+                port@3 {
+                    reg = <3>;
+                    lt8713sx_dp2_out: endpoint {
+                        remote-endpoint = <&dp2_connector_in>;
+                    };
+                };
+            };
+        };
+    };

-- 
2.34.1

[PATCH v5 2/2] drm/bridge: add support for lontium lt8713sx bridge driver

[Vishnu Saini]

The lt8713sx is a Type-C/DP1.4 to DP1.4/HDMI2.0 converter,
with three configurable DP1.4/HDMI2.0/DP++ output interfaces and
audio output interface.

Driver is required for firmware upgrade and enabling the bridge chip.

Co-developed-by: Prahlad Valluru <vvalluru@qti.qualcomm.com>
Signed-off-by: Prahlad Valluru <vvalluru@qti.qualcomm.com>
Signed-off-by: Vishnu Saini <vishnu.saini@oss.qualcomm.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Cc: Simon Zhu <xmzhu@lontium.corp-partner.google.com>
---
 drivers/gpu/drm/bridge/Kconfig            |  10 +
 drivers/gpu/drm/bridge/Makefile           |   1 +
 drivers/gpu/drm/bridge/lontium-lt8713sx.c | 598 ++++++++++++++++++++++++++++++
 3 files changed, 609 insertions(+)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index a250afd8d662..7fef383ed7cb 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -184,6 +184,16 @@ config DRM_LONTIUM_LT9611UXC
 	  HDMI signals
 	  Please say Y if you have such hardware.
 
+config DRM_LONTIUM_LT8713SX
+	tristate "Lontium LT8713SX DP MST bridge"
+	depends on OF
+	select REGMAP_I2C
+	help
+	  Driver for Lontium LT8713SX DP MST bridge
+	  chip firmware upgrade, which converts Type-C/DP1.4
+	  to 3 configurable Type-C/DP1.4/HDMI2.0 outputs
+	  Please say Y if you have such hardware.
+
 config DRM_ITE_IT66121
 	tristate "ITE IT66121 HDMI bridge"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index c7dc03182e59..07eeb13fa497 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_DRM_LONTIUM_LT8912B) += lontium-lt8912b.o
 obj-$(CONFIG_DRM_LONTIUM_LT9211) += lontium-lt9211.o
 obj-$(CONFIG_DRM_LONTIUM_LT9611) += lontium-lt9611.o
 obj-$(CONFIG_DRM_LONTIUM_LT9611UXC) += lontium-lt9611uxc.o
+obj-$(CONFIG_DRM_LONTIUM_LT8713SX) += lontium-lt8713sx.o
 obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
 obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
 obj-$(CONFIG_DRM_MICROCHIP_LVDS_SERIALIZER) += microchip-lvds.o
diff --git a/drivers/gpu/drm/bridge/lontium-lt8713sx.c b/drivers/gpu/drm/bridge/lontium-lt8713sx.c
new file mode 100644
index 000000000000..a8ae38c84719
--- /dev/null
+++ b/drivers/gpu/drm/bridge/lontium-lt8713sx.c
@@ -0,0 +1,598 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ */
+
+#include <linux/crc8.h>
+#include <linux/firmware.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/sizes.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+
+#include <drm/drm_bridge.h>
+#include <drm/drm_of.h>
+
+#define FW_FILE "lt8713sx_fw.bin"
+
+#define REG_PAGE_CONTROL	0xff
+
+#define LT8713SX_PAGE_SIZE	256
+
+DECLARE_CRC8_TABLE(lt8713sx_crc_table);
+
+struct lt8713sx {
+	struct device *dev;
+	struct drm_bridge bridge;
+	struct drm_bridge *next_bridge;
+
+	struct regmap *regmap;
+	/* Protects all accesses to registers by stopping the on-chip MCU */
+	struct mutex ocm_lock;
+
+	struct gpio_desc *reset_gpio;
+	struct gpio_desc *enable_gpio;
+
+	struct i2c_client *client;
+	const struct firmware *fw;
+
+	u8 *fw_buffer;
+
+	u32 main_crc_value;
+	u32 bank_crc_value[17];
+
+	int bank_num;
+};
+
+static void lt8713sx_reset(struct lt8713sx *lt8713sx);
+
+static const struct regmap_range lt8713sx_ranges[] = {
+	{
+		.range_min = 0x0000,
+		.range_max = 0xffff
+	},
+};
+
+static const struct regmap_access_table lt8713sx_table = {
+	.yes_ranges = lt8713sx_ranges,
+	.n_yes_ranges = ARRAY_SIZE(lt8713sx_ranges),
+};
+
+static const struct regmap_range_cfg lt8713sx_range_cfg = {
+	.name = "lt8713sx",
+	.range_min = 0x0000,
+	.range_max = 0xffff,
+	.selector_reg = REG_PAGE_CONTROL,
+	.selector_mask = 0xff,
+	.selector_shift = 0,
+	.window_start = 0,
+	.window_len = 0x100,
+};
+
+static const struct regmap_config lt8713sx_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &lt8713sx_table,
+	.ranges = &lt8713sx_range_cfg,
+	.num_ranges = 1,
+	.cache_type = REGCACHE_NONE,
+	.max_register = 0xffff,
+};
+
+static void lt8713sx_i2c_enable(struct lt8713sx *lt8713sx)
+{
+	regmap_write(lt8713sx->regmap, 0xe0ee, 0x01);
+}
+
+static void lt8713sx_i2c_disable(struct lt8713sx *lt8713sx)
+{
+	regmap_write(lt8713sx->regmap, 0xe0ee, 0x00);
+}
+
+static int lt8713sx_prepare_firmware_data(struct lt8713sx *lt8713sx)
+{
+	int ret = 0;
+	u64 sz_12k = 12 * SZ_1K;
+
+	ret = request_firmware(&lt8713sx->fw, FW_FILE, lt8713sx->dev);
+	if (ret < 0) {
+		dev_err(lt8713sx->dev, "request firmware failed\n");
+		return ret;
+	}
+
+	dev_dbg(lt8713sx->dev, "Firmware size: %zu bytes\n", lt8713sx->fw->size);
+
+	if (lt8713sx->fw->size > SZ_256K - 1) {
+		dev_err(lt8713sx->dev, "Firmware size exceeds 256KB limit\n");
+		release_firmware(lt8713sx->fw);
+		return -EINVAL;
+	}
+
+	lt8713sx->fw_buffer = kvmalloc(SZ_256K, GFP_KERNEL);
+	if (!lt8713sx->fw_buffer) {
+		release_firmware(lt8713sx->fw);
+		return -ENOMEM;
+	}
+
+	memset(lt8713sx->fw_buffer, 0xff, SZ_256K);
+
+	/* main firmware */
+	memcpy(lt8713sx->fw_buffer, lt8713sx->fw->data, SZ_64K - 1);
+
+	lt8713sx->fw_buffer[SZ_64K - 1] =
+		crc8(lt8713sx_crc_table, lt8713sx->fw_buffer, SZ_64K - 1, 0);
+	lt8713sx->main_crc_value = lt8713sx->fw_buffer[SZ_64K - 1];
+	dev_dbg(lt8713sx->dev,
+		"Main Firmware Data  Crc = 0x%02X\n", lt8713sx->main_crc_value);
+
+	/* bank firmware */
+	memcpy(lt8713sx->fw_buffer + SZ_64K,
+	       lt8713sx->fw->data + SZ_64K,
+	       lt8713sx->fw->size - SZ_64K);
+
+	lt8713sx->bank_num = (lt8713sx->fw->size - SZ_64K + sz_12k - 1) / sz_12k;
+	dev_dbg(lt8713sx->dev, "Bank Number Total is %d.\n", lt8713sx->bank_num);
+
+	for (int i = 0; i < lt8713sx->bank_num; i++) {
+		lt8713sx->bank_crc_value[i] =
+			crc8(lt8713sx_crc_table, lt8713sx->fw_buffer + SZ_64K + i * sz_12k,
+			     sz_12k, 0);
+		dev_dbg(lt8713sx->dev, "Bank number:%d; Firmware Data  Crc:0x%02X\n",
+			i, lt8713sx->bank_crc_value[i]);
+	}
+	return 0;
+}
+
+static void lt8713sx_config_parameters(struct lt8713sx *lt8713sx)
+{
+	regmap_write(lt8713sx->regmap, 0xe05e, 0xc1);
+	regmap_write(lt8713sx->regmap, 0xe058, 0x00);
+	regmap_write(lt8713sx->regmap, 0xe059, 0x50);
+	regmap_write(lt8713sx->regmap, 0xe05a, 0x10);
+	regmap_write(lt8713sx->regmap, 0xe05a, 0x00);
+	regmap_write(lt8713sx->regmap, 0xe058, 0x21);
+}
+
+static void lt8713sx_wren(struct lt8713sx *lt8713sx)
+{
+	regmap_write(lt8713sx->regmap, 0xe103, 0xbf);
+	regmap_write(lt8713sx->regmap, 0xe103, 0xff);
+	regmap_write(lt8713sx->regmap, 0xe05a, 0x04);
+	regmap_write(lt8713sx->regmap, 0xe05a, 0x00);
+}
+
+static void lt8713sx_wrdi(struct lt8713sx *lt8713sx)
+{
+	regmap_write(lt8713sx->regmap, 0xe05a, 0x08);
+	regmap_write(lt8713sx->regmap, 0xe05a, 0x00);
+}
+
+static void lt8713sx_fifo_reset(struct lt8713sx *lt8713sx)
+{
+	regmap_write(lt8713sx->regmap, 0xe103, 0xbf);
+	regmap_write(lt8713sx->regmap, 0xe103, 0xff);
+}
+
+static void lt8713sx_disable_sram_write(struct lt8713sx *lt8713sx)
+{
+	regmap_write(lt8713sx->regmap, 0xe055, 0x00);
+}
+
+static void lt8713sx_sram_to_flash(struct lt8713sx *lt8713sx)
+{
+	regmap_write(lt8713sx->regmap, 0xe05a, 0x30);
+	regmap_write(lt8713sx->regmap, 0xe05a, 0x00);
+}
+
+static void lt8713sx_i2c_to_sram(struct lt8713sx *lt8713sx)
+{
+	regmap_write(lt8713sx->regmap, 0xe055, 0x80);
+	regmap_write(lt8713sx->regmap, 0xe05e, 0xc0);
+	regmap_write(lt8713sx->regmap, 0xe058, 0x21);
+}
+
+static u8 lt8713sx_read_flash_status(struct lt8713sx *lt8713sx)
+{
+	u32 flash_status = 0;
+
+	regmap_write(lt8713sx->regmap,  0xe103, 0x3f);
+	regmap_write(lt8713sx->regmap,  0xe103, 0xff);
+
+	regmap_write(lt8713sx->regmap,  0xe05e, 0x40);
+	regmap_write(lt8713sx->regmap,  0xe056, 0x05); /* opcode=read status register */
+	regmap_write(lt8713sx->regmap,  0xe055, 0x25);
+	regmap_write(lt8713sx->regmap,  0xe055, 0x01);
+	regmap_write(lt8713sx->regmap,  0xe058, 0x21);
+
+	regmap_read(lt8713sx->regmap, 0xe05f, &flash_status);
+	dev_dbg(lt8713sx->dev, "flash_status:%x\n", flash_status);
+
+	return flash_status;
+}
+
+static void lt8713sx_block_erase(struct lt8713sx *lt8713sx)
+{
+	u32 i = 0;
+	u8 flash_status = 0;
+	u8 blocknum = 0x00;
+	u32 flashaddr = 0x00;
+
+	for (blocknum = 0; blocknum < 8; blocknum++) {
+		flashaddr = blocknum * SZ_32K;
+		regmap_write(lt8713sx->regmap,  0xe05a, 0x04);
+		regmap_write(lt8713sx->regmap,  0xe05a, 0x00);
+		regmap_write(lt8713sx->regmap,  0xe05b, flashaddr >> 16);
+		regmap_write(lt8713sx->regmap,  0xe05c, flashaddr >> 8);
+		regmap_write(lt8713sx->regmap,  0xe05d, flashaddr);
+		regmap_write(lt8713sx->regmap,  0xe05a, 0x01);
+		regmap_write(lt8713sx->regmap,  0xe05a, 0x00);
+		msleep(100);
+		i = 0;
+		while (1) {
+			flash_status = lt8713sx_read_flash_status(lt8713sx);
+			if ((flash_status & 0x01) == 0)
+				break;
+
+			if (i > 50)
+				break;
+
+			i++;
+			msleep(50);
+		}
+	}
+	dev_dbg(lt8713sx->dev, "erase flash done.\n");
+}
+
+static void lt8713sx_load_main_fw_to_sram(struct lt8713sx *lt8713sx)
+{
+	regmap_write(lt8713sx->regmap, 0xe068, 0x00);
+	regmap_write(lt8713sx->regmap, 0xe069, 0x00);
+	regmap_write(lt8713sx->regmap, 0xe06a, 0x00);
+	regmap_write(lt8713sx->regmap, 0xe065, 0x00);
+	regmap_write(lt8713sx->regmap, 0xe066, 0xff);
+	regmap_write(lt8713sx->regmap, 0xe067, 0xff);
+	regmap_write(lt8713sx->regmap, 0xe06b, 0x00);
+	regmap_write(lt8713sx->regmap, 0xe06c, 0x00);
+	regmap_write(lt8713sx->regmap, 0xe060, 0x01);
+	msleep(200);
+	regmap_write(lt8713sx->regmap, 0xe060, 0x00);
+}
+
+static void lt8713sx_load_bank_fw_to_sram(struct lt8713sx *lt8713sx, u64 addr)
+{
+	regmap_write(lt8713sx->regmap, 0xe068, ((addr & 0xff0000) >> 16));
+	regmap_write(lt8713sx->regmap, 0xe069, ((addr & 0x00ff00) >> 8));
+	regmap_write(lt8713sx->regmap, 0xe06a, (addr & 0x0000ff));
+	regmap_write(lt8713sx->regmap, 0xe065, 0x00);
+	regmap_write(lt8713sx->regmap, 0xe066, 0x30);
+	regmap_write(lt8713sx->regmap, 0xe067, 0x00);
+	regmap_write(lt8713sx->regmap, 0xe06b, 0x00);
+	regmap_write(lt8713sx->regmap, 0xe06c, 0x00);
+	regmap_write(lt8713sx->regmap, 0xe060, 0x01);
+	msleep(50);
+	regmap_write(lt8713sx->regmap, 0xe060, 0x00);
+}
+
+static int lt8713sx_write_data(struct lt8713sx *lt8713sx, const u8 *data, u64 filesize)
+{
+	int page = 0, num = 0, i = 0, val;
+
+	page = (filesize % LT8713SX_PAGE_SIZE) ?
+			((filesize / LT8713SX_PAGE_SIZE) + 1) : (filesize / LT8713SX_PAGE_SIZE);
+
+	dev_dbg(lt8713sx->dev,
+		"Writing to Sram=%u pages, total size = %llu bytes\n", page, filesize);
+
+	for (num = 0; num < page; num++) {
+		dev_dbg(lt8713sx->dev, "page[%d]\n", num);
+		lt8713sx_i2c_to_sram(lt8713sx);
+
+		for (i = 0; i < LT8713SX_PAGE_SIZE; i++) {
+			if ((num * LT8713SX_PAGE_SIZE + i) < filesize)
+				val = *(data + (num * LT8713SX_PAGE_SIZE + i));
+			else
+				val = 0xff;
+			regmap_write(lt8713sx->regmap, 0xe059, val);
+		}
+
+		lt8713sx_wren(lt8713sx);
+		lt8713sx_sram_to_flash(lt8713sx);
+	}
+
+	lt8713sx_wrdi(lt8713sx);
+	lt8713sx_disable_sram_write(lt8713sx);
+
+	return 0;
+}
+
+static void lt8713sx_main_upgrade_result(struct lt8713sx *lt8713sx)
+{
+	u32 main_crc_result;
+
+	regmap_read(lt8713sx->regmap, 0xe023, &main_crc_result);
+
+	dev_dbg(lt8713sx->dev, "Main CRC HW: 0x%02X\n", main_crc_result);
+	dev_dbg(lt8713sx->dev, "Main CRC FW: 0x%02X\n", lt8713sx->main_crc_value);
+
+	if (main_crc_result == lt8713sx->main_crc_value)
+		dev_info(lt8713sx->dev, "Main Firmware Upgrade Success.\n");
+	else
+		dev_err(lt8713sx->dev, "Main Firmware Upgrade Failed.\n");
+}
+
+static void lt8713sx_bank_upgrade_result(struct lt8713sx *lt8713sx, u8 banknum)
+{
+	u32 bank_crc_result;
+
+	regmap_read(lt8713sx->regmap, 0xe023, &bank_crc_result);
+
+	dev_dbg(lt8713sx->dev, "Bank %d CRC Result: 0x%02X\n", banknum, bank_crc_result);
+
+	if (bank_crc_result == lt8713sx->bank_crc_value[banknum])
+		dev_info(lt8713sx->dev, "Bank %d Firmware Upgrade Success.\n", banknum);
+	else
+		dev_err(lt8713sx->dev, "Bank %d Firmware Upgrade Failed.\n", banknum);
+}
+
+static void lt8713sx_bank_result_check(struct lt8713sx *lt8713sx)
+{
+	int i;
+	u64 addr = 0x010000;
+
+	for (i = 0; i < lt8713sx->bank_num; i++) {
+		lt8713sx_load_bank_fw_to_sram(lt8713sx, addr);
+		lt8713sx_bank_upgrade_result(lt8713sx, i);
+		addr += 0x3000;
+	}
+}
+
+static int lt8713sx_firmware_upgrade(struct lt8713sx *lt8713sx)
+{
+	int ret;
+
+	lt8713sx_config_parameters(lt8713sx);
+
+	lt8713sx_block_erase(lt8713sx);
+
+	if (lt8713sx->fw->size < SZ_64K) {
+		ret = lt8713sx_write_data(lt8713sx, lt8713sx->fw_buffer, SZ_64K);
+		if (ret < 0) {
+			dev_err(lt8713sx->dev, "Failed to write firmware data: %d\n", ret);
+			return ret;
+		}
+	} else {
+		ret = lt8713sx_write_data(lt8713sx, lt8713sx->fw_buffer, lt8713sx->fw->size);
+		if (ret < 0) {
+			dev_err(lt8713sx->dev, "Failed to write firmware data: %d\n", ret);
+			return ret;
+		}
+	}
+	dev_dbg(lt8713sx->dev, "Write Data done.\n");
+
+	return 0;
+}
+
+static int lt8713sx_firmware_update(struct lt8713sx *lt8713sx)
+{
+	int ret = 0;
+
+	guard(mutex)(&lt8713sx->ocm_lock);
+	lt8713sx_i2c_enable(lt8713sx);
+
+	ret = lt8713sx_prepare_firmware_data(lt8713sx);
+	if (ret < 0) {
+		dev_err(lt8713sx->dev, "Failed to prepare firmware data: %d\n", ret);
+		goto error;
+	}
+
+	ret = lt8713sx_firmware_upgrade(lt8713sx);
+	if (ret < 0) {
+		dev_err(lt8713sx->dev, "Upgrade failure.\n");
+		goto error;
+	}
+
+	/* Validate CRC */
+	lt8713sx_load_main_fw_to_sram(lt8713sx);
+	lt8713sx_main_upgrade_result(lt8713sx);
+	lt8713sx_wrdi(lt8713sx);
+	lt8713sx_fifo_reset(lt8713sx);
+	lt8713sx_bank_result_check(lt8713sx);
+	lt8713sx_wrdi(lt8713sx);
+
+error:
+	lt8713sx_i2c_disable(lt8713sx);
+	if (!ret)
+		lt8713sx_reset(lt8713sx);
+
+	kvfree(lt8713sx->fw_buffer);
+	lt8713sx->fw_buffer = NULL;
+
+	if (lt8713sx->fw) {
+		release_firmware(lt8713sx->fw);
+		lt8713sx->fw = NULL;
+	}
+
+	return ret;
+}
+
+static void lt8713sx_reset(struct lt8713sx *lt8713sx)
+{
+	dev_dbg(lt8713sx->dev, "reset bridge.\n");
+	gpiod_set_value_cansleep(lt8713sx->reset_gpio, 1);
+	msleep(20);
+
+	gpiod_set_value_cansleep(lt8713sx->reset_gpio, 0);
+	msleep(20);
+
+	dev_dbg(lt8713sx->dev, "reset done.\n");
+}
+
+static int lt8713sx_regulator_enable(struct lt8713sx *lt8713sx)
+{
+	int ret;
+
+	ret = devm_regulator_get_enable(lt8713sx->dev, "vdd");
+	if (ret < 0)
+		return dev_err_probe(lt8713sx->dev, ret, "failed to enable vdd regulator\n");
+
+	usleep_range(1000, 10000);
+
+	ret = devm_regulator_get_enable(lt8713sx->dev, "vcc");
+	if (ret < 0)
+		return dev_err_probe(lt8713sx->dev, ret, "failed to enable vcc regulator\n");
+	return 0;
+}
+
+static int lt8713sx_bridge_attach(struct drm_bridge *bridge,
+				  struct drm_encoder *encoder,
+				  enum drm_bridge_attach_flags flags)
+{
+	struct lt8713sx *lt8713sx = container_of(bridge, struct lt8713sx, bridge);
+
+	return drm_bridge_attach(encoder,
+				lt8713sx->next_bridge,
+				bridge, flags);
+}
+
+static int lt8713sx_gpio_init(struct lt8713sx *lt8713sx)
+{
+	struct device *dev = lt8713sx->dev;
+
+	lt8713sx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(lt8713sx->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(lt8713sx->reset_gpio),
+				     "failed to acquire reset gpio\n");
+
+	/* power enable gpio */
+	lt8713sx->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
+	if (IS_ERR(lt8713sx->enable_gpio))
+		return dev_err_probe(dev, PTR_ERR(lt8713sx->enable_gpio),
+				     "failed to acquire enable gpio\n");
+	return 0;
+}
+
+static ssize_t lt8713sx_firmware_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t len)
+{
+	struct lt8713sx *lt8713sx = dev_get_drvdata(dev);
+	int ret;
+
+	ret = lt8713sx_firmware_update(lt8713sx);
+	if (ret < 0)
+		return ret;
+	return len;
+}
+
+static DEVICE_ATTR_WO(lt8713sx_firmware);
+
+static struct attribute *lt8713sx_attrs[] = {
+	&dev_attr_lt8713sx_firmware.attr,
+	NULL,
+};
+
+static const struct attribute_group lt8713sx_attr_group = {
+	.attrs = lt8713sx_attrs,
+};
+
+static const struct attribute_group *lt8713sx_attr_groups[] = {
+	&lt8713sx_attr_group,
+	NULL,
+};
+
+static const struct drm_bridge_funcs lt8713sx_bridge_funcs = {
+	.attach = lt8713sx_bridge_attach,
+};
+
+static int lt8713sx_probe(struct i2c_client *client)
+{
+	struct lt8713sx *lt8713sx;
+	struct device *dev = &client->dev;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return dev_err_probe(dev, -ENODEV, "device doesn't support I2C\n");
+
+	lt8713sx = devm_drm_bridge_alloc(dev, struct lt8713sx, bridge, &lt8713sx_bridge_funcs);
+	if (IS_ERR(lt8713sx))
+		return PTR_ERR(lt8713sx);
+
+	lt8713sx->dev = dev;
+	lt8713sx->client = client;
+	i2c_set_clientdata(client, lt8713sx);
+
+	ret = devm_mutex_init(lt8713sx->dev, &lt8713sx->ocm_lock);
+	if (ret)
+		return ret;
+
+	lt8713sx->regmap = devm_regmap_init_i2c(client, &lt8713sx_regmap_config);
+	if (IS_ERR(lt8713sx->regmap))
+		return dev_err_probe(dev, PTR_ERR(lt8713sx->regmap), "regmap i2c init failed\n");
+
+	ret = drm_of_find_panel_or_bridge(lt8713sx->dev->of_node, 1, -1, NULL,
+					  &lt8713sx->next_bridge);
+	if (ret < 0)
+		return ret;
+
+	ret = lt8713sx_gpio_init(lt8713sx);
+	if (ret < 0)
+		return ret;
+
+	ret = lt8713sx_regulator_enable(lt8713sx);
+	if (ret)
+		return ret;
+
+	lt8713sx_reset(lt8713sx);
+
+	lt8713sx->bridge.funcs = &lt8713sx_bridge_funcs;
+	lt8713sx->bridge.of_node = dev->of_node;
+	lt8713sx->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
+	drm_bridge_add(&lt8713sx->bridge);
+
+	crc8_populate_msb(lt8713sx_crc_table, 0x31);
+
+	return 0;
+}
+
+static void lt8713sx_remove(struct i2c_client *client)
+{
+	struct lt8713sx *lt8713sx = i2c_get_clientdata(client);
+
+	drm_bridge_remove(&lt8713sx->bridge);
+}
+
+static struct i2c_device_id lt8713sx_id[] = {
+	{ "lontium,lt8713sx", 0 },
+	{ /* sentinel */ }
+};
+
+static const struct of_device_id lt8713sx_match_table[] = {
+	{ .compatible = "lontium,lt8713sx" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, lt8713sx_match_table);
+
+static struct i2c_driver lt8713sx_driver = {
+	.driver = {
+		.name = "lt8713sx",
+		.of_match_table = lt8713sx_match_table,
+		.dev_groups = lt8713sx_attr_groups,
+	},
+	.probe = lt8713sx_probe,
+	.remove = lt8713sx_remove,
+	.id_table = lt8713sx_id,
+};
+
+module_i2c_driver(lt8713sx_driver);
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("lt8713sx drm bridge driver");
+MODULE_AUTHOR("Vishnu Saini <vishnu.saini@oss.qualcomm.com>");
+MODULE_FIRMWARE(FW_FILE);

-- 
2.34.1

Claude review: Add lontium lt8713sx bridge driver

[Claude Code Review Bot]

Overall Series Review

Subject: Add lontium lt8713sx bridge driver
Author: Vishnu Saini <vishnu.saini@oss.qualcomm.com>
Patches: 3
Reviewed: 2026-03-04T06:57:55.228488

---

This is a v5 series adding a DRM bridge driver for the Lontium LT8713SX, a Type-C/DP1.4 to DP1.4/HDMI2.0 converter bridge chip. The series consists of a DT binding (already reviewed-by Krzysztof Kozlowski) and a driver (reviewed-by Dmitry Baryshkov). The driver is primarily a firmware upgrade mechanism exposed via sysfs, with a minimal `drm_bridge` implementation that only forwards attach to the next bridge in the chain.

The overall design is reasonable for a firmware-upgrade-focused driver, but there are several issues ranging from a real bug to minor style/correctness concerns that should be addressed.

---

---
Generated by Claude Code Patch Reviewer

Claude review: dt-bindings: bridge: lt8713sx: Add bindings

[Claude Code Review Bot]

Patch Review

This patch has a `Reviewed-by: Krzysztof Kozlowski` so the DT binding itself is likely in good shape. A few observations:

1. **Missing `enable-gpios` in the binding**: The driver in patch 2 acquires an optional `"enable"` GPIO via `devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH)`, but the binding YAML has no `enable-gpios` property. If the hardware can use an enable GPIO, it should be documented in the binding. If it isn't actually needed, the driver shouldn't be requesting it.

2. **Output port descriptions are vague**: All three output ports (port@1, port@2, port@3) have nearly identical descriptions:
   ```yaml
   port@1:
     description:
       DP port for DP output from bridge
   port@2:
     description:
       Additional DP port for DP output from bridge
   ```
   It would be helpful to clarify if these correspond to specific physical connectors on the chip or are functionally interchangeable, and whether they can be HDMI outputs as well (the title says "DP1.4/HDMI2.0").

3. **No `allOf` with `i2c-controller.yaml`**: Minor — some bindings for I2C devices include an explicit `$ref` to `i2c-controller.yaml` or the `unevaluatedProperties: false` pattern, though this isn't strictly required with `additionalProperties: false`.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: add support for lontium lt8713sx bridge driver

[Claude Code Review Bot]

Patch Review

**Issues (in descending severity):**

#### 1. Bug: Redundant assignment of `bridge.funcs` (line 555)

```c
lt8713sx = devm_drm_bridge_alloc(dev, struct lt8713sx, bridge, &lt8713sx_bridge_funcs);
...
lt8713sx->bridge.funcs = &lt8713sx_bridge_funcs;
```

`devm_drm_bridge_alloc()` already sets `bridge->funcs = funcs` (confirmed in `drm_bridge.c:349`). The manual assignment at line 555 is redundant. While not harmful, it suggests a misunderstanding of the API and should be removed to match convention — no other in-tree user of `devm_drm_bridge_alloc()` does this.

#### 2. Firmware data corruption when `fw->size < SZ_64K` (line 128)

```c
memcpy(lt8713sx->fw_buffer, lt8713sx->fw->data, SZ_64K - 1);
```

This unconditionally copies `SZ_64K - 1` bytes from firmware data, but the firmware size is only validated as `<= SZ_256K - 1`. If the firmware is smaller than 64K (which is explicitly handled in `lt8713sx_firmware_upgrade` with `if (lt8713sx->fw->size < SZ_64K)`), this `memcpy` reads past the end of `lt8713sx->fw->data`, causing an out-of-bounds read. The copy size should be `min(lt8713sx->fw->size, (size_t)(SZ_64K - 1))`.

Similarly, when `fw->size < SZ_64K`, the bank firmware section on line 137-139:
```c
memcpy(lt8713sx->fw_buffer + SZ_64K,
       lt8713sx->fw->data + SZ_64K,
       lt8713sx->fw->size - SZ_64K);
```
would produce an underflow in the size argument (`fw->size - SZ_64K` wraps to a huge number since `size_t` is unsigned). This would be catastrophic. The bank firmware copy and bank_num calculation should be guarded by a check that `fw->size > SZ_64K`.

#### 3. `bank_crc_value` array bounds (lines 50, 143-149)

```c
u32 bank_crc_value[17];
...
lt8713sx->bank_num = (lt8713sx->fw->size - SZ_64K + sz_12k - 1) / sz_12k;
```

The maximum firmware size is `SZ_256K - 1` (262143 bytes). The bank region starts at `SZ_64K` (65536), so the max bank data is `262143 - 65536 = 196607` bytes. With 12K banks: `ceil(196607 / 12288) = 16`. The array has 17 entries (indices 0-16), so this is just barely sufficient, but the magic number 17 is fragile. A `#define` or computed constant would be safer and clearer. More importantly, there is no bounds check on `bank_num` before using it to index `bank_crc_value[]`.

#### 4. `lt8713sx_block_erase` silently ignores timeout (lines 238-249)

```c
while (1) {
    flash_status = lt8713sx_read_flash_status(lt8713sx);
    if ((flash_status & 0x01) == 0)
        break;
    if (i > 50)
        break;
    i++;
    msleep(50);
}
```

When `i > 50`, the loop breaks silently — the erase timeout is not reported. The function returns `void` so the caller (`lt8713sx_firmware_upgrade`) has no idea the erase failed and proceeds to write firmware to a potentially not-erased flash. This could result in a corrupted firmware on the bridge chip. This function should return an error code on timeout.

#### 5. `lt8713sx_write_data` always returns 0 (line 313)

```c
static int lt8713sx_write_data(struct lt8713sx *lt8713sx, const u8 *data, u64 filesize)
{
    ...
    return 0;
}
```

Despite returning `int`, `lt8713sx_write_data` ignores all `regmap_write()` return values and always returns 0. The callers check `ret < 0` but this can never happen. Either the function should check regmap errors, or the return type should be `void` (and callers adjusted).

#### 6. `i2c_device_id` should not contain commas (line 573)

```c
static struct i2c_device_id lt8713sx_id[] = {
    { "lontium,lt8713sx", 0 },
```

The I2C device ID string should not contain a comma — that's the OF compatible format, not the I2C device name. Compare with other lontium drivers which use e.g. `"lontium,lt9611"`. Wait — I see the other lontium drivers also use the comma format. This appears to be a pre-existing convention in the lontium drivers, though technically it's not correct for I2C device names (they should match the `driver.name` or be plain device names). Additionally, the `0` second field is unnecessary for modern kernels and can be omitted (as `lt9611.c` does).

Also this should be `const`:
```c
static struct i2c_device_id lt8713sx_id[] = {
```
should be:
```c
static const struct i2c_device_id lt8713sx_id[] = {
```

#### 7. `enable_gpio` acquired but not in DT binding (line 475)

```c
lt8713sx->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
```

The DT binding in patch 1 has no `enable-gpios` property. If this GPIO is needed, add it to the binding. If it's not needed, remove the code. Having driver code that references undocumented DT properties will cause `dt_binding_check` warnings.

#### 8. Unused includes (lines 10, 13, 18, 19)

Several headers appear unused:
- `<linux/interrupt.h>` — no interrupt handling in this driver
- `<linux/platform_device.h>` — this is an I2C driver
- `<linux/wait.h>` — no wait queues used
- `<linux/workqueue.h>` — no workqueues used

#### 9. `usleep_range(1000, 10000)` has a very wide range (line 446)

```c
usleep_range(1000, 10000);
```

A 10x spread between min and max is unusually large. Typically the range should be tighter, e.g., `usleep_range(1000, 1500)` or `usleep_range(1000, 2000)`, unless the hardware truly doesn't care. This warrants a comment or tightening.

#### 10. Global CRC table (line 30, 560)

```c
DECLARE_CRC8_TABLE(lt8713sx_crc_table);
...
crc8_populate_msb(lt8713sx_crc_table, 0x31);
```

The CRC table is module-global (file-scoped `static`), not per-device. If two instances of this bridge exist, `crc8_populate_msb` would be called twice from two different `probe` calls, which is harmless but wasteful. More importantly, the table population happens in `probe()` but could theoretically be used via the sysfs attribute before a second `probe()` completes, leading to a race. Consider using `once` semantics or making it `__initdata` with a module init function.

#### 11. sysfs attribute naming (line 495)

```c
static DEVICE_ATTR_WO(lt8713sx_firmware);
```

This creates a sysfs attribute named `lt8713sx_firmware`. The convention for sysfs attributes is to not prefix with the driver name — since the attribute is already scoped to the device, just `firmware_update` or `update_firmware` would be clearer. Also, a write-only sysfs attribute that triggers firmware update regardless of what is written is somewhat unusual — the content of `buf` is completely ignored. Consider documenting this in the ABI or at minimum adding a comment.

#### 12. `lt8713sx_firmware_update` cleanup path (lines 410-421)

The `guard(mutex)` at line 387 means the mutex is held until function return. The error label at line 410 always calls `lt8713sx_i2c_disable()`, which writes to regmap — this is fine. However, `kvfree(lt8713sx->fw_buffer)` is called unconditionally. If `lt8713sx_prepare_firmware_data` fails at `request_firmware`, `fw_buffer` is NULL (never allocated), and `kvfree(NULL)` is safe. If it fails at `kvmalloc`, `fw_buffer` is also NULL. So the cleanup is correct, but the `if (lt8713sx->fw)` check on line 418 suggests `fw` might not always be set — the asymmetry between `fw` (conditional free) and `fw_buffer` (unconditional free) is slightly confusing but functionally correct.

#### 13. `u64` types where `u32` would suffice

Several variables use `u64` where `u32` is more than adequate:
- `sz_12k` (line 103): `12 * SZ_1K = 12288`, easily fits in `u32`
- `addr` in `lt8713sx_bank_result_check` (line 348): max value is `0x010000 + 16*0x3000 = 0x40000`, fits in `u32`
- `filesize` parameter in `lt8713sx_write_data` (line 284): max is `SZ_256K`, fits in `u32`

Using `u64` for these is misleading and generates slightly worse code on 32-bit architectures.

---
Generated by Claude Code Patch Reviewer