From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/sitronix/st7586: fix bad pixel data due to byte swap Date: Tue, 03 Mar 2026 14:04:23 +1000 Message-ID: In-Reply-To: <20260228-drm-mipi-dbi-fix-st7586-byte-swap-v1-1-e78f6c24cd28@baylibre.com> References: <20260228-drm-mipi-dbi-fix-st7586-byte-swap-v1-1-e78f6c24cd28@baylibre.com> <20260228-drm-mipi-dbi-fix-st7586-byte-swap-v1-1-e78f6c24cd28@baylibre.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness: Good** The fix correctly sets `dbi->write_memory_bpw =3D 8` immediately after `mip= i_dbi_spi_init()`, which defaults `write_memory_bpw` to 16. This is the rig= ht value for the ST7586 since it uses 8-bit data (3 grayscale pixels packed= per byte). The placement is correct: 1. `mipi_dbi_spi_init()` sets `write_memory_bpw =3D 16` (default) 2. **Patch adds** `dbi->write_memory_bpw =3D 8` =E2=80=94 overrides before = anything else uses the value 3. `mipi_dbi_dev_init_with_formats()` only overrides `write_memory_bpw` for= `DRM_FORMAT_RGB888`, but st7586 uses `DRM_FORMAT_XRGB8888`, so no conflict ```c + /* + * Override value set by mipi_dbi_spi_init(). This driver is a bit + * non-standard, so best to set it explicitly here. + */ + dbi->write_memory_bpw =3D 8; ``` **Removal of old workaround: Correct** The old `swap_bytes =3D true` block is properly removed. The old comment ex= plained that `swap_bytes` was being used not for actual byte swapping, but = to trick `mipi_dbi_typec3_command()` into using 8-bit SPI transfers. That m= echanism is now replaced by the direct `write_memory_bpw` control: ```c - /* - * we are using 8-bit data, so we are not actually swapping anything, - * but setting mipi->swap_bytes makes mipi_dbi_typec3_command() do the - * right thing and not use 16-bit transfers (which results in swapped - * bytes on little-endian systems and causes out of order data to be - * sent to the display). - */ - dbi->swap_bytes =3D true; ``` **Minor style observation (not blocking):** The new comment is slightly gen= eric =E2=80=94 "a bit non-standard" doesn't explain *why* 8 bpw is needed. = The old comment was more informative about the root cause (3 pixels per byt= e, 8-bit data). A one-liner like "ST7586 uses 8-bit data (3 pixels per byte= )" would be more helpful to future readers. But this is a nit. **Commit message: Good.** Clear explanation of the problem (every 3 columns= swapped), root cause (default 16-bit transfers after refactoring), and fix= . Proper `Fixes:` tag referencing the breaking commit. **No other concerns.** The patch is minimal, focused, and correct. --- Generated by Claude Code Patch Reviewer