Re: [PATCH v3 4/4] drm/ssd130x: Add support for the SSD133x OLED controller family

2023-12-22 Thread Javier Martinez Canillas
Jocelyn Falempe  writes:

Hello Jocelyn,

Thanks a lot for your review!

> On 19/12/2023 21:34, Javier Martinez Canillas wrote:
>> The Solomon SSD133x controllers (such as the SSD1331) are used by RGB dot
>> matrix OLED panels, add a modesetting pipeline to support the chip family.
>> 
>> The SSD133x controllers support 256 (8-bit) and 65k (16-bit) color depths
>> but only the former is implemented for now. This is because the 256 color
>> depth format matches a fourcc code already present in DRM (RGB8), but the
>> 65k pixel format does not match the existing RG16 fourcc code format.
>> 
>> Instead of a R:G:B 5:6:5, the controller expects the 16-bit pixels to be
>> R:G:B 6:5:6, and so a new fourcc needs to be added to support this format.
>
> small typo here, R:G:B 6:5:6 => that's 17 bits
>

Oh, tanks for pointing that out.

It seems to be a typo in the SSD1331 controller datasheet itself:

https://cdn-shop.adafruit.com/datasheets/SSD1331_1.2.pdf

"Each pixel has 16-bit data. Three sub-pixels for color A, B and C have 6
bits, 5 bits and 6 bits respectively."

I blindly copied what the datasheet said without relizing that it was 17
bits indeed!

So looking again at "Table 9 - Data bus usage under different bus width
and color depth mode" in the datasheet shared above, it seems the format
has the same sub-pixel layout than DRM_FORMAT_RGB565. But I tested with
that format and the colors were off, and the same for DRM_FORMAT_BGR565.

Now, even when only using 256 colors the images are pretty decent as you
can see in https://fosstodon.org/@javierm/111591985174504541

I'll reword the commit message and drop the comment about that RGB format
and explain that only DRM_FORMAT_RGB332 is supported for now.

> other than that, it looks good to me, feel free to add:
> Reviewed-by: Jocelyn Falempe 
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat



Re: [PATCH v3 4/4] drm/ssd130x: Add support for the SSD133x OLED controller family

2023-12-22 Thread Jocelyn Falempe




On 19/12/2023 21:34, Javier Martinez Canillas wrote:

The Solomon SSD133x controllers (such as the SSD1331) are used by RGB dot
matrix OLED panels, add a modesetting pipeline to support the chip family.

The SSD133x controllers support 256 (8-bit) and 65k (16-bit) color depths
but only the former is implemented for now. This is because the 256 color
depth format matches a fourcc code already present in DRM (RGB8), but the
65k pixel format does not match the existing RG16 fourcc code format.

Instead of a R:G:B 5:6:5, the controller expects the 16-bit pixels to be
R:G:B 6:5:6, and so a new fourcc needs to be added to support this format.


small typo here, R:G:B 6:5:6 => that's 17 bits

other than that, it looks good to me, feel free to add:
Reviewed-by: Jocelyn Falempe 



Signed-off-by: Javier Martinez Canillas 
---

(no changes since v1)

  drivers/gpu/drm/solomon/ssd130x-i2c.c |   5 +
  drivers/gpu/drm/solomon/ssd130x-spi.c |   7 +
  drivers/gpu/drm/solomon/ssd130x.c | 370 ++
  drivers/gpu/drm/solomon/ssd130x.h |   5 +-
  4 files changed, 386 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x-i2c.c 
b/drivers/gpu/drm/solomon/ssd130x-i2c.c
index f2ccab9c06d9..a047dbec4e48 100644
--- a/drivers/gpu/drm/solomon/ssd130x-i2c.c
+++ b/drivers/gpu/drm/solomon/ssd130x-i2c.c
@@ -105,6 +105,11 @@ static const struct of_device_id ssd130x_of_match[] = {
.compatible = "solomon,ssd1327",
.data = _variants[SSD1327_ID],
},
+   /* ssd133x family */
+   {
+   .compatible = "solomon,ssd1331",
+   .data = _variants[SSD1331_ID],
+   },
{ /* sentinel */ }
  };
  MODULE_DEVICE_TABLE(of, ssd130x_of_match);
diff --git a/drivers/gpu/drm/solomon/ssd130x-spi.c 
b/drivers/gpu/drm/solomon/ssd130x-spi.c
index 84e035a7ab3f..84bfde31d172 100644
--- a/drivers/gpu/drm/solomon/ssd130x-spi.c
+++ b/drivers/gpu/drm/solomon/ssd130x-spi.c
@@ -142,6 +142,11 @@ static const struct of_device_id ssd130x_of_match[] = {
.compatible = "solomon,ssd1327",
.data = _variants[SSD1327_ID],
},
+   /* ssd133x family */
+   {
+   .compatible = "solomon,ssd1331",
+   .data = _variants[SSD1331_ID],
+   },
{ /* sentinel */ }
  };
  MODULE_DEVICE_TABLE(of, ssd130x_of_match);
@@ -166,6 +171,8 @@ static const struct spi_device_id ssd130x_spi_table[] = {
{ "ssd1322", SSD1322_ID },
{ "ssd1325", SSD1325_ID },
{ "ssd1327", SSD1327_ID },
+   /* ssd133x family */
+   { "ssd1331", SSD1331_ID },
{ /* sentinel */ }
  };
  MODULE_DEVICE_TABLE(spi, ssd130x_spi_table);
diff --git a/drivers/gpu/drm/solomon/ssd130x.c 
b/drivers/gpu/drm/solomon/ssd130x.c
index bef293922b98..447d0c7c88c6 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -119,6 +119,26 @@
  #define SSD130X_SET_VCOMH_VOLTAGE 0xbe
  #define SSD132X_SET_FUNCTION_SELECT_B 0xd5
  
+/* ssd133x commands */

+#define SSD133X_SET_COL_RANGE  0x15
+#define SSD133X_SET_ROW_RANGE  0x75
+#define SSD133X_CONTRAST_A 0x81
+#define SSD133X_CONTRAST_B 0x82
+#define SSD133X_CONTRAST_C 0x83
+#define SSD133X_SET_MASTER_CURRENT 0x87
+#define SSD132X_SET_PRECHARGE_A0x8a
+#define SSD132X_SET_PRECHARGE_B0x8b
+#define SSD132X_SET_PRECHARGE_C0x8c
+#define SSD133X_SET_DISPLAY_START  0xa1
+#define SSD133X_SET_DISPLAY_OFFSET 0xa2
+#define SSD133X_SET_DISPLAY_NORMAL 0xa4
+#define SSD133X_SET_MASTER_CONFIG  0xad
+#define SSD133X_POWER_SAVE_MODE0xb0
+#define SSD133X_PHASES_PERIOD  0xb1
+#define SSD133X_SET_CLOCK_FREQ 0xb3
+#define SSD133X_SET_PRECHARGE_VOLTAGE  0xbb
+#define SSD133X_SET_VCOMH_VOLTAGE  0xbe
+
  #define MAX_CONTRAST 255
  
  const struct ssd130x_deviceinfo ssd130x_variants[] = {

@@ -180,6 +200,12 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.default_width = 128,
.default_height = 128,
.family_id = SSD132X_FAMILY,
+   },
+   /* ssd133x family */
+   [SSD1331_ID] = {
+   .default_width = 96,
+   .default_height = 64,
+   .family_id = SSD133X_FAMILY,
}
  };
  EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X);
@@ -589,6 +615,117 @@ static int ssd132x_init(struct ssd130x_device *ssd130x)
return 0;
  }
  
+static int ssd133x_init(struct ssd130x_device *ssd130x)

+{
+   int ret;
+
+   /* Set color A contrast */
+   ret = ssd130x_write_cmd(ssd130x, 2, SSD133X_CONTRAST_A, 0x91);
+   if (ret < 0)
+   return ret;
+
+   /* Set color B contrast */
+   ret = 

[PATCH v3 4/4] drm/ssd130x: Add support for the SSD133x OLED controller family

2023-12-19 Thread Javier Martinez Canillas
The Solomon SSD133x controllers (such as the SSD1331) are used by RGB dot
matrix OLED panels, add a modesetting pipeline to support the chip family.

The SSD133x controllers support 256 (8-bit) and 65k (16-bit) color depths
but only the former is implemented for now. This is because the 256 color
depth format matches a fourcc code already present in DRM (RGB8), but the
65k pixel format does not match the existing RG16 fourcc code format.

Instead of a R:G:B 5:6:5, the controller expects the 16-bit pixels to be
R:G:B 6:5:6, and so a new fourcc needs to be added to support this format.

Signed-off-by: Javier Martinez Canillas 
---

(no changes since v1)

 drivers/gpu/drm/solomon/ssd130x-i2c.c |   5 +
 drivers/gpu/drm/solomon/ssd130x-spi.c |   7 +
 drivers/gpu/drm/solomon/ssd130x.c | 370 ++
 drivers/gpu/drm/solomon/ssd130x.h |   5 +-
 4 files changed, 386 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x-i2c.c 
b/drivers/gpu/drm/solomon/ssd130x-i2c.c
index f2ccab9c06d9..a047dbec4e48 100644
--- a/drivers/gpu/drm/solomon/ssd130x-i2c.c
+++ b/drivers/gpu/drm/solomon/ssd130x-i2c.c
@@ -105,6 +105,11 @@ static const struct of_device_id ssd130x_of_match[] = {
.compatible = "solomon,ssd1327",
.data = _variants[SSD1327_ID],
},
+   /* ssd133x family */
+   {
+   .compatible = "solomon,ssd1331",
+   .data = _variants[SSD1331_ID],
+   },
{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, ssd130x_of_match);
diff --git a/drivers/gpu/drm/solomon/ssd130x-spi.c 
b/drivers/gpu/drm/solomon/ssd130x-spi.c
index 84e035a7ab3f..84bfde31d172 100644
--- a/drivers/gpu/drm/solomon/ssd130x-spi.c
+++ b/drivers/gpu/drm/solomon/ssd130x-spi.c
@@ -142,6 +142,11 @@ static const struct of_device_id ssd130x_of_match[] = {
.compatible = "solomon,ssd1327",
.data = _variants[SSD1327_ID],
},
+   /* ssd133x family */
+   {
+   .compatible = "solomon,ssd1331",
+   .data = _variants[SSD1331_ID],
+   },
{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, ssd130x_of_match);
@@ -166,6 +171,8 @@ static const struct spi_device_id ssd130x_spi_table[] = {
{ "ssd1322", SSD1322_ID },
{ "ssd1325", SSD1325_ID },
{ "ssd1327", SSD1327_ID },
+   /* ssd133x family */
+   { "ssd1331", SSD1331_ID },
{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(spi, ssd130x_spi_table);
diff --git a/drivers/gpu/drm/solomon/ssd130x.c 
b/drivers/gpu/drm/solomon/ssd130x.c
index bef293922b98..447d0c7c88c6 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -119,6 +119,26 @@
 #define SSD130X_SET_VCOMH_VOLTAGE  0xbe
 #define SSD132X_SET_FUNCTION_SELECT_B  0xd5
 
+/* ssd133x commands */
+#define SSD133X_SET_COL_RANGE  0x15
+#define SSD133X_SET_ROW_RANGE  0x75
+#define SSD133X_CONTRAST_A 0x81
+#define SSD133X_CONTRAST_B 0x82
+#define SSD133X_CONTRAST_C 0x83
+#define SSD133X_SET_MASTER_CURRENT 0x87
+#define SSD132X_SET_PRECHARGE_A0x8a
+#define SSD132X_SET_PRECHARGE_B0x8b
+#define SSD132X_SET_PRECHARGE_C0x8c
+#define SSD133X_SET_DISPLAY_START  0xa1
+#define SSD133X_SET_DISPLAY_OFFSET 0xa2
+#define SSD133X_SET_DISPLAY_NORMAL 0xa4
+#define SSD133X_SET_MASTER_CONFIG  0xad
+#define SSD133X_POWER_SAVE_MODE0xb0
+#define SSD133X_PHASES_PERIOD  0xb1
+#define SSD133X_SET_CLOCK_FREQ 0xb3
+#define SSD133X_SET_PRECHARGE_VOLTAGE  0xbb
+#define SSD133X_SET_VCOMH_VOLTAGE  0xbe
+
 #define MAX_CONTRAST 255
 
 const struct ssd130x_deviceinfo ssd130x_variants[] = {
@@ -180,6 +200,12 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.default_width = 128,
.default_height = 128,
.family_id = SSD132X_FAMILY,
+   },
+   /* ssd133x family */
+   [SSD1331_ID] = {
+   .default_width = 96,
+   .default_height = 64,
+   .family_id = SSD133X_FAMILY,
}
 };
 EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X);
@@ -589,6 +615,117 @@ static int ssd132x_init(struct ssd130x_device *ssd130x)
return 0;
 }
 
+static int ssd133x_init(struct ssd130x_device *ssd130x)
+{
+   int ret;
+
+   /* Set color A contrast */
+   ret = ssd130x_write_cmd(ssd130x, 2, SSD133X_CONTRAST_A, 0x91);
+   if (ret < 0)
+   return ret;
+
+   /* Set color B contrast */
+   ret = ssd130x_write_cmd(ssd130x, 2, SSD133X_CONTRAST_B, 0x50);
+   if (ret < 0)
+   return ret;
+
+   /* Set color C contrast */
+   ret = ssd130x_write_cmd(ssd130x, 2, SSD133X_CONTRAST_C, 0x7d);
+   if (ret <