drm/solomon: Add SSD130x OLED displays SPI support

The current compatible strings for SSD130x I2C controllers contain an -fb suffix, this seems to indicate that are for a fbdev driver. But the DT is supposed to describe the hardware and not Linux implementation details.

Let's deprecate those compatible strings and add a new enum that contains compatible strings that don't have a -fb suffix. These will be matched by the ssd130x-i2c DRM driver.

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 .../bindings/display/solomon,ssd1307fb.yaml   | 36 ++++++++++++-------
 1 file changed, 24 insertions(+), 12 deletions(-)

Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml +8 -6

index ade61d502edd..46207f2c12b8 100644
@@ -12,12 +12,24 @@ maintainers:
 
 properties:
   compatible:
-    enum:
-      - sinowealth,sh1106-i2c
-      - solomon,ssd1305fb-i2c
-      - solomon,ssd1306fb-i2c
-      - solomon,ssd1307fb-i2c
-      - solomon,ssd1309fb-i2c
+    oneOf:
+      # Deprecated compatible strings
+      - items:
+          - enum:
+              - solomon,ssd1305fb-i2c
+              - solomon,ssd1306fb-i2c
+              - solomon,ssd1307fb-i2c
+              - solomon,ssd1309fb-i2c

Quoted 3 lines · Javier Martinez Canillas

The current compatible strings for SSD130x I2C controllers contain an -fb suffix, this seems to indicate that are for a fbdev driver. But the DT is supposed to describe the hardware and not Linux implementation details.

True, but compatible is just an identifier. There's no reason to deprecate unless the binding as a whole needs to be redone.

I imagine you also want 2 compatibles for 2 drivers. That's saying you should change your firmware to switch drivers. The fact that we have 2 drivers for the same h/w is a kernel problem. Don't bring DT into it.

Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml maintainers: · line 32

Quoted 9 lines

Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml +9 -0

+
+      # SSD130x I2C controllers
+      - items:
+          - enum:
+              - sinowealth,sh1106-i2c
+              - solomon,ssd1305-i2c
+              - solomon,ssd1306-i2c
+              - solomon,ssd1307-i2c
+              - solomon,ssd1309-i2c

There's also no reason to put the bus interface into the compatible as the same compatible will work on different buses. But since you want to add SPI, just using the 'i2c' one will confuse people. For that reason you could add 'solomon,ssd1305', etc. for both SPI support and I2C DRM.

(You should also support the 'fb-i2c' variant in DRM IMO, but doubtful that I'll review that.)

Rob

Quoted 5 lines

There's also no reason to put the bus interface into the compatible as the same compatible will work on different buses. But since you want to add SPI, just using the 'i2c' one will confuse people. For that reason you could add 'solomon,ssd1305', etc. for both SPI support and I2C DRM.

That's not really true. There's a reason to add per bus compatible strings at least in Linux. And is that there's no information about the bus types in module aliases that are reported to user-space for module auto-loading.

The current compatible strings for SSD130x I2C controllers contain an -fb suffix, this seems to indicate that are for a fbdev driver. But the DT is supposed to describe the hardware and not Linux implementation details.

Let's deprecate those compatible strings and add a new enum that contains compatible strings that don't have a -fb suffix. These will be matched by the ssd130x-i2c DRM driver.

Signed-off-by Javier Martinez Canillas <[email protected]>

Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml +0 -4

@@ -12,12 +12,24 @@ maintainers:
 
 properties:
   compatible:
-    enum:
-      - sinowealth,sh1106-i2c
-      - solomon,ssd1305fb-i2c
-      - solomon,ssd1306fb-i2c

Hi Javier,

Quoted 27 lines · Javier Martinez Canillas

The current compatible strings for SSD130x I2C controllers contain an -fb suffix, this seems to indicate that are for a fbdev driver. But the DT is supposed to describe the hardware and not Linux implementation details.

Let's deprecate those compatible strings and add a new enum that contains compatible strings that don't have a -fb suffix. These will be matched by the ssd130x-i2c DRM driver.

Signed-off-by Javier Martinez Canillas <[email protected]>

Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml +5 -6

@@ -12,12 +12,24 @@ maintainers:
 
 properties:
   compatible:
-    enum:
-      - sinowealth,sh1106-i2c
-      - solomon,ssd1305fb-i2c
-      - solomon,ssd1306fb-i2c
-      - solomon,ssd1307fb-i2c
-      - solomon,ssd1309fb-i2c
+    oneOf:
+      # Deprecated compatible strings
+      - items:
+          - enum:
+              - solomon,ssd1305fb-i2c

Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml maintainers: · line 22

Quoted 3 lines

Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml +3 -0

+              - solomon,ssd1306fb-i2c
+              - solomon,ssd1307fb-i2c
+              - solomon,ssd1309fb-i2c

Please drop the "-i2c" suffixes, too.
We already have plenty of IIO sensors and audio codecs using the same compatible value for spi and i2c, cfr.
'git grep compatible -- "*-[si][p2][ic].c"'

These are declared in the ssd130x-i2c transport driver but the information is not I2C specific and could be used by other SSD130x transport drivers.

Move them to the ssd130x core driver and just set the OF device entries to an ID that could be used to lookup the correct device into from an array.

While being there, also move the SSD130X_DATA and SSD130X_COMMAND control bytes. Since even though are used by the I2C interface, it could also be useful for other transport protocols such as SPI.

Suggested-by Chen-Yu Tsai <[email protected]>

Signed-off-by Javier Martinez Canillas <[email protected]>

---

  drivers/gpu/drm/solomon/ssd130x-i2c.c | 51 ++++-------------------
  drivers/gpu/drm/solomon/ssd130x.c     | 60 +++++++++++++++++++++++++--
  drivers/gpu/drm/solomon/ssd130x.h     | 13 ++++++
  3 files changed, 78 insertions(+), 46 deletions(-)

drivers/gpu/drm/solomon/ssd130x-i2c.c +9 -42

index a469679548f8..aa6cc2cb54f9 100644
@@ -53,76 +53,43 @@ static void ssd130x_i2c_shutdown(struct i2c_client *client)
  	ssd130x_shutdown(ssd130x);
  }
 
-static struct ssd130x_deviceinfo ssd130x_sh1106_deviceinfo = {
-	.default_vcomh = 0x40,
-	.default_dclk_div = 1,
-	.default_dclk_frq = 5,
-	.page_mode_only = 1,
-};
-
-static struct ssd130x_deviceinfo ssd130x_ssd1305_deviceinfo = {
-	.default_vcomh = 0x34,
-	.default_dclk_div = 1,
-	.default_dclk_frq = 7,
-};
-
-static struct ssd130x_deviceinfo ssd130x_ssd1306_deviceinfo = {
-	.default_vcomh = 0x20,
-	.default_dclk_div = 1,
-	.default_dclk_frq = 8,
-	.need_chargepump = 1,
-};
-
-static struct ssd130x_deviceinfo ssd130x_ssd1307_deviceinfo = {
-	.default_vcomh = 0x20,
-	.default_dclk_div = 2,
-	.default_dclk_frq = 12,
-	.need_pwm = 1,
-};
-
-static struct ssd130x_deviceinfo ssd130x_ssd1309_deviceinfo = {
-	.default_vcomh = 0x34,
-	.default_dclk_div = 1,
-	.default_dclk_frq = 10,
-};
-
  static const struct of_device_id ssd130x_of_match[] = {
  	{
  		.compatible = "sinowealth,sh1106-i2c",
-		.data = &ssd130x_sh1106_deviceinfo,
+		.data = SH1106_ID,
  	},
  	{
  		.compatible = "solomon,ssd1305-i2c",
-		.data = &ssd130x_ssd1305_deviceinfo,
+		.data = (void *)SSD1305_ID,
  	},
  	{
  		.compatible = "solomon,ssd1306-i2c",
-		.data = &ssd130x_ssd1306_deviceinfo,
+		.data = (void *)SSD1306_ID,
  	},
  	{
  		.compatible = "solomon,ssd1307-i2c",
-		.data = &ssd130x_ssd1307_deviceinfo,
+		.data = (void *)SSD1307_ID,
  	},
  	{
  		.compatible = "solomon,ssd1309-i2c",
-		.data = &ssd130x_ssd1309_deviceinfo,
+		.data = (void *)SSD1309_ID,
  	},
  	/* Deprecated but remain for backward compatibility */
  	{
  		.compatible = "solomon,ssd1305fb-i2c",
-		.data = &ssd130x_ssd1305_deviceinfo,
+		.data = (void *)SSD1305_ID,
  	},
  	{
  		.compatible = "solomon,ssd1306fb-i2c",
-		.data = &ssd130x_ssd1306_deviceinfo,
+		.data = (void *)SSD1306_ID,
  	},
  	{
  		.compatible = "solomon,ssd1307fb-i2c",
-		.data = &ssd130x_ssd1307_deviceinfo,
+		.data = (void *)SSD1307_ID,
  	},
  	{
  		.compatible = "solomon,ssd1309fb-i2c",
-		.data = &ssd130x_ssd1309_deviceinfo,
+		.data = (void *)SSD1309_ID,
  	},
  	{ /* sentinel */ }
  };

drivers/gpu/drm/solomon/ssd130x.c +1 -3

index a7e784518c69..1f00fd3c0023 100644
@@ -39,11 +39,9 @@
  #define DRIVER_MAJOR	1
  #define DRIVER_MINOR	0
 
-#define SSD130X_DATA				0x40
-#define SSD130X_COMMAND				0x80
-
  #define SSD130X_PAGE_COL_START_LOW		0x00
  #define SSD130X_PAGE_COL_START_HIGH		0x10
+
  #define SSD130X_SET_ADDRESS_MODE		0x20
  #define SSD130X_SET_COL_RANGE			0x21
  #define SSD130X_SET_PAGE_RANGE			0x22

drivers/gpu/drm/solomon/ssd130x.c +48 -1

+	{
+		.variant = SSD1305_ID,
+		.default_vcomh = 0x34,
+		.default_dclk_div = 1,
+		.default_dclk_frq = 7,
+	},
+	{
+		.variant = SSD1306_ID,
+		.default_vcomh = 0x20,
+		.default_dclk_div = 1,
+		.default_dclk_frq = 8,
+		.need_chargepump = 1,
+	},
+	{
+		.variant = SSD1307_ID,
+		.default_vcomh = 0x20,
+		.default_dclk_div = 2,
+		.default_dclk_frq = 12,
+		.need_pwm = 1,
+	},
+	{
+		.variant = SSD1309_ID,
+		.default_vcomh = 0x34,
+		.default_dclk_div = 1,
+		.default_dclk_frq = 10,
+	}
+};
+
+static const struct ssd130x_deviceinfo *ssd13x_variant_to_info(enum ssd130x_variants variant)
+{
+	int i;
+	const struct ssd130x_deviceinfo *info;
+
+	for (i = 0; i < ARRAY_SIZE(ssd130x_variants); i++) {
+		info = &ssd130x_variants[i];
+		if (info->variant == variant)
+			return info;
+	}
+
+	return NULL;
+}
+
  static inline struct ssd130x_device *drm_to_ssd130x(struct drm_device *drm)
  {
  	return container_of(drm, struct ssd130x_device, drm);

@@ -846,6 +893,7 @@ static int ssd130x_get_resources(struct ssd130x_device *ssd130x)
  struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
  {
  	struct ssd130x_device *ssd130x;
+	enum ssd130x_variants variant;
  	struct backlight_device *bl;
  	struct drm_device *drm;
  	int ret;

@@ -860,7 +908,11 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
 
  	ssd130x->dev = dev;
  	ssd130x->regmap = regmap;
-	ssd130x->device_info = device_get_match_data(dev);
+
+	variant = (enum ssd130x_variants)device_get_match_data(dev);
+	ssd130x->device_info = ssd13x_variant_to_info(variant);
+	if (!ssd130x->device_info)
+		return ERR_PTR(-EINVAL);
 
  	if (ssd130x->device_info->page_mode_only)
  		ssd130x->page_address_mode = 1;

drivers/gpu/drm/solomon/ssd130x.h +7 -0

index f5b062576fdf..4e0b62a41aa3 100644
@@ -18,7 +18,20 @@
 
  #include <linux/regmap.h>
 
+#define SSD130X_DATA				0x40
+#define SSD130X_COMMAND				0x80
+
+enum ssd130x_variants {
+	SH1106_ID,
+	SSD1305_ID,
+	SSD1306_ID,

drivers/gpu/drm/solomon/ssd130x.c line 101

Quoted 8 lines

drivers/gpu/drm/solomon/ssd130x.c +7 -0

+static struct ssd130x_deviceinfo ssd130x_variants[] =  {
+	{
+		.default_vcomh = 0x40,
+		.default_dclk_div = 1,
+		.default_dclk_frq = 5,
+		.page_mode_only = 1,
+	},

Why not [SH1106_ID] = {

and later:

if (variant < NR_SSD130X_VARIANTS)

	ssd130x->device_info = ssd130x_variants[variant];

instead of less efficient ssd13x_variant_to_info ?

The ssd130x driver only provides the core support for these devices but it does not have any bus transport logic. Add a driver to interface over SPI.

There is a difference in the communication protocol when using 4-wire SPI instead of I2C. For the latter, a control byte that contains a D/C# field has to be sent. This field tells the controller whether the data has to be written to the command register or to the graphics display data memory.

But for 4-wire SPI that control byte is not used, instead a real D/C# line must be pulled HIGH for commands data and LOW for graphics display data.

For this reason the standard SPI regmap can't be used and a custom .write bus handler is needed.