This patchset is trying to fix problems seen on S905X boards when interfacing
with an ILI9486 equipped SPI panel.
To: Kamlesh Gurudasani <[email protected]>
To: David Airlie <[email protected]>
To: Daniel Vetter <[email protected]>
To: Mark Brown <[email protected]>
To: Neil Armstrong <[email protected]>
To: Kevin Hilman <[email protected]>
To: Jerome Brunet <[email protected]>
To: Martin Blumenstingl <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Carlo Caione <[email protected]>
---
Changes in v2:
- Removed SPICC patch
- Reworked commit message
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Carlo Caione (2):
drm/tiny: rpi-lcd-35: Enable driver module autoloading
drm/tiny: ili9486: Do not assume 8-bit only SPI controllers
drivers/gpu/drm/tiny/ili9486.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
---
base-commit: 147307c69ba4441ee90c1f8ce8edf5df4ea60f67
change-id: 20221116-s905x_spi_ili9486-aed54ff3cb21
Best regards,
--
Carlo Caione
SPI devices use the spi_device_id for module autoloading even on
systems using device tree.
Add the spi_device_id entry to enable autoloading for the 3.5inch RPi
Display (rpi-lcd-35).
Signed-off-by: Carlo Caione <[email protected]>
---
drivers/gpu/drm/tiny/ili9486.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
index 1bb847466b10..bd37dfe8dd05 100644
--- a/drivers/gpu/drm/tiny/ili9486.c
+++ b/drivers/gpu/drm/tiny/ili9486.c
@@ -183,6 +183,7 @@ MODULE_DEVICE_TABLE(of, ili9486_of_match);
static const struct spi_device_id ili9486_id[] = {
{ "ili9486", 0 },
+ { "rpi-lcd-35", 0 },
{ }
};
MODULE_DEVICE_TABLE(spi, ili9486_id);
--
b4 0.10.1
The pixel data for the ILI9486 is always 16-bits wide and it must be
sent over the SPI bus. When the controller is only able to deal with
8-bit transfers, this 16-bits data needs to be swapped before the
sending to account for the big endian bus, this is on the contrary not
needed when the SPI controller already supports 16-bits transfers.
The decision about swapping the pixel data or not is taken in the MIPI
DBI code by probing the controller capabilities: if the controller only
suppors 8-bit transfers the data is swapped, otherwise it is not.
This swapping/non-swapping is relying on the assumption that when the
controller does support 16-bit transactions then the data is sent
unswapped in 16-bits-per-word over SPI.
The problem with the ILI9486 driver is that it is forcing 8-bit
transactions also for controllers supporting 16-bits, violating the
assumption and corrupting the pixel data.
Align the driver to what is done in the MIPI DBI code by adjusting the
tranfer size to the maximum allowed by the SPI controller.
Signed-off-by: Carlo Caione <[email protected]>
---
drivers/gpu/drm/tiny/ili9486.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
index bd37dfe8dd05..4d80a413338f 100644
--- a/drivers/gpu/drm/tiny/ili9486.c
+++ b/drivers/gpu/drm/tiny/ili9486.c
@@ -43,6 +43,7 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
size_t num)
{
struct spi_device *spi = mipi->spi;
+ unsigned int bpw = 8;
void *data = par;
u32 speed_hz;
int i, ret;
@@ -56,8 +57,6 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
* The displays are Raspberry Pi HATs and connected to the 8-bit only
* SPI controller, so 16-bit command and parameters need byte swapping
* before being transferred as 8-bit on the big endian SPI bus.
- * Pixel data bytes have already been swapped before this function is
- * called.
*/
buf[0] = cpu_to_be16(*cmd);
gpiod_set_value_cansleep(mipi->dc, 0);
@@ -71,12 +70,18 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
for (i = 0; i < num; i++)
buf[i] = cpu_to_be16(par[i]);
num *= 2;
- speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num);
data = buf;
}
+ /*
+ * Check whether pixel data bytes needs to be swapped or not
+ */
+ if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
+ bpw = 16;
+
gpiod_set_value_cansleep(mipi->dc, 1);
- ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, data, num);
+ speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num);
+ ret = mipi_dbi_spi_transfer(spi, speed_hz, bpw, data, num);
free:
kfree(buf);
--
b4 0.10.1
On 21/11/2022 10:42, Carlo Caione wrote:
> This patchset is trying to fix problems seen on S905X boards when interfacing
> with an ILI9486 equipped SPI panel.
Gentle ping on this 2-patch set.
Cheers,
--
Carlo Caione
On 21/11/2022 10:42, Carlo Caione wrote:
> SPI devices use the spi_device_id for module autoloading even on
> systems using device tree.
>
> Add the spi_device_id entry to enable autoloading for the 3.5inch RPi
> Display (rpi-lcd-35).
>
> Signed-off-by: Carlo Caione <[email protected]>
> ---
> drivers/gpu/drm/tiny/ili9486.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
> index 1bb847466b10..bd37dfe8dd05 100644
> --- a/drivers/gpu/drm/tiny/ili9486.c
> +++ b/drivers/gpu/drm/tiny/ili9486.c
> @@ -183,6 +183,7 @@ MODULE_DEVICE_TABLE(of, ili9486_of_match);
>
> static const struct spi_device_id ili9486_id[] = {
> { "ili9486", 0 },
> + { "rpi-lcd-35", 0 },
It should also contain "piscreen" then.
Anyway:
Reviewed-by: Neil Armstrong <[email protected]>
> { }
> };
> MODULE_DEVICE_TABLE(spi, ili9486_id);
>
On 21/11/2022 10:42, Carlo Caione wrote:
> The pixel data for the ILI9486 is always 16-bits wide and it must be
> sent over the SPI bus. When the controller is only able to deal with
> 8-bit transfers, this 16-bits data needs to be swapped before the
> sending to account for the big endian bus, this is on the contrary not
> needed when the SPI controller already supports 16-bits transfers.
>
> The decision about swapping the pixel data or not is taken in the MIPI
> DBI code by probing the controller capabilities: if the controller only
> suppors 8-bit transfers the data is swapped, otherwise it is not.
>
> This swapping/non-swapping is relying on the assumption that when the
> controller does support 16-bit transactions then the data is sent
> unswapped in 16-bits-per-word over SPI.
>
> The problem with the ILI9486 driver is that it is forcing 8-bit
> transactions also for controllers supporting 16-bits, violating the
> assumption and corrupting the pixel data.
>
> Align the driver to what is done in the MIPI DBI code by adjusting the
> tranfer size to the maximum allowed by the SPI controller.
>
> Signed-off-by: Carlo Caione <[email protected]>
> ---
> drivers/gpu/drm/tiny/ili9486.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
> index bd37dfe8dd05..4d80a413338f 100644
> --- a/drivers/gpu/drm/tiny/ili9486.c
> +++ b/drivers/gpu/drm/tiny/ili9486.c
> @@ -43,6 +43,7 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
> size_t num)
> {
> struct spi_device *spi = mipi->spi;
> + unsigned int bpw = 8;
> void *data = par;
> u32 speed_hz;
> int i, ret;
> @@ -56,8 +57,6 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
> * The displays are Raspberry Pi HATs and connected to the 8-bit only
> * SPI controller, so 16-bit command and parameters need byte swapping
> * before being transferred as 8-bit on the big endian SPI bus.
> - * Pixel data bytes have already been swapped before this function is
> - * called.
> */
> buf[0] = cpu_to_be16(*cmd);
> gpiod_set_value_cansleep(mipi->dc, 0);
> @@ -71,12 +70,18 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
> for (i = 0; i < num; i++)
> buf[i] = cpu_to_be16(par[i]);
> num *= 2;
> - speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num);
> data = buf;
> }
>
> + /*
> + * Check whether pixel data bytes needs to be swapped or not
> + */
> + if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
> + bpw = 16;
> +
> gpiod_set_value_cansleep(mipi->dc, 1);
> - ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, data, num);
> + speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num);
> + ret = mipi_dbi_spi_transfer(spi, speed_hz, bpw, data, num);
> free:
> kfree(buf);
>
> Looks fine, but should somehow be tested on an RPi first
to check if the 8bit fallback still works.
Reviewed-by: Neil Armstrong <[email protected]>