[PATCH] drm/sitronix/st7586: fix bad pixel data due to byte swap

[PATCH] drm/sitronix/st7586: fix bad pixel data due to byte swap

[David Lechner]

Correctly set dbi->write_memory_bpw for the ST7586 driver. This driver
is for a monochrome display that has an unusual data format, so the
default value set in mipi_dbi_spi_init() is not correct simply because
this controller is non-standard.

Previously, we were using dbi->swap_bytes to make the same sort of
workaround, but it was removed in the same commit that added
dbi->write_memory_bpw, so we need to use the latter now to have the
correct behavior.

This fixes every 3 columns of pixels being swapped on the display. There
are 3 pixels per byte, so the byte swap caused this effect.

Fixes: df3fb27a74a4 ("drm/mipi-dbi: Make bits per word configurable for pixel transfers")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/gpu/drm/sitronix/st7586.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/sitronix/st7586.c b/drivers/gpu/drm/sitronix/st7586.c
index b57ebf37a664..16b6b4e368af 100644
--- a/drivers/gpu/drm/sitronix/st7586.c
+++ b/drivers/gpu/drm/sitronix/st7586.c
@@ -347,6 +347,12 @@ static int st7586_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	/*
+	 * 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 = 8;
+
 	/* Cannot read from this controller via SPI */
 	dbi->read_commands = NULL;
 
@@ -356,15 +362,6 @@ static int st7586_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	/*
-	 * 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 = true;
-
 	drm_mode_config_reset(drm);
 
 	ret = drm_dev_register(drm, 0);

---
base-commit: 3fa5e5702a82d259897bd7e209469bc06368bf31
change-id: 20260228-drm-mipi-dbi-fix-st7586-byte-swap-f0d03c5f800c

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

Re: [PATCH] drm/sitronix/st7586: fix bad pixel data due to byte swap

[Thomas Zimmermann]

cc Javier

Am 01.03.26 um 05:30 schrieb David Lechner:
> Correctly set dbi->write_memory_bpw for the ST7586 driver. This driver
> is for a monochrome display that has an unusual data format, so the
> default value set in mipi_dbi_spi_init() is not correct simply because
> this controller is non-standard.
>
> Previously, we were using dbi->swap_bytes to make the same sort of
> workaround, but it was removed in the same commit that added
> dbi->write_memory_bpw, so we need to use the latter now to have the
> correct behavior.
>
> This fixes every 3 columns of pixels being swapped on the display. There
> are 3 pixels per byte, so the byte swap caused this effect.
>
> Fixes: df3fb27a74a4 ("drm/mipi-dbi: Make bits per word configurable for pixel transfers")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>   drivers/gpu/drm/sitronix/st7586.c | 15 ++++++---------
>   1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/sitronix/st7586.c b/drivers/gpu/drm/sitronix/st7586.c
> index b57ebf37a664..16b6b4e368af 100644
> --- a/drivers/gpu/drm/sitronix/st7586.c
> +++ b/drivers/gpu/drm/sitronix/st7586.c
> @@ -347,6 +347,12 @@ static int st7586_probe(struct spi_device *spi)
>   	if (ret)
>   		return ret;
>   
> +	/*
> +	 * 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 = 8;
> +
>   	/* Cannot read from this controller via SPI */
>   	dbi->read_commands = NULL;
>   
> @@ -356,15 +362,6 @@ static int st7586_probe(struct spi_device *spi)
>   	if (ret)
>   		return ret;
>   
> -	/*
> -	 * 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 = true;
> -
>   	drm_mode_config_reset(drm);
>   
>   	ret = drm_dev_register(drm, 0);
>
> ---
> base-commit: 3fa5e5702a82d259897bd7e209469bc06368bf31
> change-id: 20260228-drm-mipi-dbi-fix-st7586-byte-swap-f0d03c5f800c
>
> Best regards,

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Claude review: drm/sitronix/st7586: fix bad pixel data due to byte swap

[Claude Code Review Bot]

Overall Series Review

Subject: drm/sitronix/st7586: fix bad pixel data due to byte swap
Author: David Lechner <dlechner@baylibre.com>
Patches: 2
Reviewed: 2026-03-03T14:04:23.047090

---

This is a single-patch bug fix for the ST7586 Sitronix display driver. The ST7586 is a monochrome controller that packs 3 pixels per byte (8-bit data), which is non-standard compared to typical MIPI DBI displays that use 16-bit RGB565.

A prior commit (`df3fb27a74a4`) refactored how the MIPI DBI subsystem controls the SPI bits-per-word for pixel transfers, replacing the indirect `swap_bytes` workaround with an explicit `write_memory_bpw` field. That refactoring inadvertently broke the ST7586 by leaving it with the default 16-bit transfer width, causing every group of 3 pixel columns to be swapped on the display.

The fix is straightforward, correct, and well-placed. **Reviewed-by worthy.**

---
Generated by Claude Code Patch Reviewer

Claude review: drm/sitronix/st7586: fix bad pixel data due to byte swap

[Claude Code Review Bot]

Patch Review

**Correctness: Good**

The fix correctly sets `dbi->write_memory_bpw = 8` immediately after `mipi_dbi_spi_init()`, which defaults `write_memory_bpw` to 16. This is the right 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 = 16` (default)
2. **Patch adds** `dbi->write_memory_bpw = 8` — 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 = 8;
```

**Removal of old workaround: Correct**

The old `swap_bytes = true` block is properly removed. The old comment explained 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 mechanism 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 = true;
```

**Minor style observation (not blocking):** The new comment is slightly generic — "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 byte, 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