drm/solomon: Add SSD130x OLED displays SPI support

The current compatible strings for SSD130x I2C controllers contain both an "fb" and "-i2c" suffixes. It seems to indicate that are for a fbdev driver and also that are for devices that can be accessed over an I2C bus.

But a DT is supposed to describe the hardware and not Linux implementation details. So let's deprecate those compatible strings and add new ones that only contain the vendor and device name, without any of these suffixes.

These will just describe the device and can be matched by both I2C and SPI DRM drivers.

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

Acked-by Mark Brown <[email protected]>

The current compatible strings for SSD130x I2C controllers contain both an "fb" and "-i2c" suffixes. It seems to indicate that are for a fbdev driver and also that are for devices that can be accessed over an I2C bus.

But a DT is supposed to describe the hardware and not Linux implementation details. So let's deprecate those compatible strings and add new ones that only contain the vendor and device name, without any of these suffixes.

These will just describe the device and can be matched by both I2C and SPI DRM drivers.

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

Acked-by Mark Brown <[email protected]>

---

Changes in v2:
- Drop the -i2c suffixes from the compatible strings too (Geert Uytterhoeven).

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

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

index ade61d502edd..6b9d0c72739a 100644
@@ -12,12 +12,23 @@ 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:
+              - sinowealth,sh1106-i2c
+              - solomon,ssd1305fb-i2c
+              - solomon,ssd1306fb-i2c
+              - solomon,ssd1307fb-i2c
+              - solomon,ssd1309fb-i2c
+        deprecated: true
+      - items:
+          - enum:
+              - sinowealth,sh1106
+              - solomon,ssd1305
+              - solomon,ssd1306
+              - solomon,ssd1307
+              - solomon,ssd1309
 
   reg:
     maxItems: 1

On Mon, Apr 11, 2022 at 11:12:39PM +0200, Javier Martinez Canillas wrote:

Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml allOf: · line 150

Quoted 9 lines

reg:
maxItems: 1

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

@@ -136,7 +147,7 @@ allOf:
       properties:
         compatible:
           contains:
-            const: sinowealth,sh1106-i2c
+            const: sinowealth,sh1106

I think we should have both in the condition here, but it looks good otherwise.

Maxime

The current compatible strings for SSD130x I2C controllers contain both an "fb" and "-i2c" suffixes. It seems to indicate that are for a fbdev driver and also that are for devices that can be accessed over an I2C bus.

But a DT is supposed to describe the hardware and not Linux implementation details. So let's deprecate those compatible strings and add new ones that only contain the vendor and device name, without any of these suffixes.

These will just describe the device and can be matched by both I2C and SPI DRM drivers.

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

Acked-by Mark Brown <[email protected]>

---

Changes in v2:
- Drop the -i2c suffixes from the compatible strings too (Geert Uytterhoeven).

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

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

index ade61d502edd..6b9d0c72739a 100644
@@ -12,12 +12,23 @@ maintainers:
 
 properties:
   compatible:
-    enum:

On Tue, Apr 12, 2022 at 5:12 AM Javier Martinez Canillas <[email protected]> wrote:

I think you can just drop this one, since it was just merged and isn't part of any release yet. It's not even in -rc.

ChenYu

The Solomon SSD130x OLED displays can either have an I2C or SPI interface, add to the schema the properties and examples for OLED devices under SPI.

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

Acked-by Mark Brown <[email protected]>

---

Changes in v2:
- Don't add compatible strings with an "-spi" suffix (Geert Uytterhoeven).

Hi Javier,

Quoted 3 lines · Javier Martinez Canillas

The Solomon SSD130x OLED displays can either have an I2C or SPI interface, add to the schema the properties and examples for OLED devices under SPI.

Quoted 6 lines

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

Acked-by Mark Brown <[email protected]>

---

Changes in v2:
- Don't add compatible strings with an "-spi" suffix (Geert Uytterhoeven).

Thanks for the update!

Quoted 6 lines

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

@@ -39,9 +39,14 @@ properties:
   reset-gpios:
     maxItems: 1
 

Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml properties: · line 44

Quoted 3 lines

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

+  dc-gpios:
+    maxItems: 1
+

Perhaps add a description, and clarify this is for SPI only?

Reviewed-by Geert Uytterhoeven <[email protected]>

Quoted 13 lines · Geert Uytterhoeven

Quoted 11 lines · Javier Martinez Canillas

The Solomon SSD130x OLED displays can either have an I2C or SPI interface, add to the schema the properties and examples for OLED devices under SPI.

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

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

@@ -39,9 +39,14 @@ properties:
   reset-gpios:
     maxItems: 1
 

Quoted 5 lines

Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml properties: · line 44

Quoted 3 lines

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

+  dc-gpios:
+    maxItems: 1
+

Perhaps add a description, and clarify this is for SPI only?

I wondered how to make it required for SPI but couldn't find another binding that did the same and I'm not that familiar with DT schemas to figure it out.

Before, when I had compatible strings just for SPI I could do the following:

- if:
properties:
compatible:
contains:
enum:
- sinowealth,sh1106-spi
- solomon,ssd1305-spi
- solomon,ssd1306-spi
- solomon,ssd1307-spi
- solomon,ssd1309-spi
then:
required:
- spi-max-frequency
- dc-gpios

but now that we are using the same compatible strings for I2C and SPI, the compatible string can't be used anymore as an indication to make required.

Do you have any hints here on how I should enforce this in the schema ?

Or if you think that a comment is enough, then I will add it in v3.

Hi Javier,

Quoted 4 lines

Do you have any hints here on how I should enforce this in the schema ?

Or if you think that a comment is enough, then I will add it in v3.

I don't know how to make it required for SPI, if possible at all.

Quoted 1 line · Geert Uytterhoeven

Hi Javier,

[snip]

Quoted 3 lines

Quoted 3 lines

Do you have any hints here on how I should enforce this in the schema ?

Quoted 4 lines

Quoted 1 line

Or if you think that a comment is enough, then I will add it in v3.

I don't know how to make it required for SPI, if possible at all.

I see. Let's go with a comment then and we can later enforce it, if someone knows if is possible / how to do it.

The current compatible strings for SSD130x I2C controllers contain an "fb" and "-i2c" suffixes. These have been deprecated and more correct ones were added, that don't encode a subsystem or bus used to interface the devices.

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

Acked-by Mark Brown <[email protected]>

---

(no changes since v1)

Hi Javier,

Quoted 4 lines · Javier Martinez Canillas

The current compatible strings for SSD130x I2C controllers contain an "fb" and "-i2c" suffixes. These have been deprecated and more correct ones were added, that don't encode a subsystem or bus used to interface the devices.

Quoted 5 lines

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

Acked-by Mark Brown <[email protected]>

---

(no changes since v1)

That's not really true ;-)

Reviewed-by Geert Uytterhoeven <[email protected]>

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 info 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]>

Hi Javier,

Thanks for your patch!

Quoted 3 lines · Javier Martinez Canillas

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.

Quoted 5 lines

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 info 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

though they are ... they could

Quoted 10 lines

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.c +0 -0

@@ -860,7 +890,14 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
 

drivers/gpu/drm/solomon/ssd130x.c struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap) · line 894

Quoted 5 lines

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

        ssd130x->dev = dev;
        ssd130x->regmap = regmap;
-       ssd130x->device_info = device_get_match_data(dev);
+
+       variant = (enum ssd130x_variants)device_get_match_data(dev);

(uintptr_t), to avoid a cast from pointer to integer of different size warning.

drivers/gpu/drm/solomon/ssd130x.c struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap) · line 895

Quoted 1 line

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

+

Please drop the blank line.

Quoted 6 lines

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

+       if (variant >= NR_SSD130X_VARIANTS)
+               return ERR_PTR(dev_err_probe(dev, -EINVAL,
+                                            "Invalid SSD130x variant\n"));
+
+       ssd130x->device_info = &ssd130x_variants[variant];
 

drivers/gpu/drm/solomon/ssd130x.c struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap) · line 903

Quoted 2 lines

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

        if (ssd130x->device_info->page_mode_only)
                ssd130x->page_address_mode = 1;

With the above fixed:

Reviewed-by Geert Uytterhoeven <[email protected]>

Quoted 2 lines · Geert Uytterhoeven

On Mon, Apr 11, 2022 at 11:12 PM Javier Martinez Canillas <[email protected]> wrote:

Quoted 7 lines

drivers/gpu/drm/solomon/ssd130x.c struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap) · line 894

Quoted 3 lines

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

-       ssd130x->device_info = device_get_match_data(dev);
+
+       variant = (enum ssd130x_variants)device_get_match_data(dev);

(uintptr_t), to avoid a cast from pointer to integer of different size warning.

Indeed. The kernel test robot reported the same.

Quoted 3 lines · Javier Martinez Canillas

Quoted 2 lines · Geert Uytterhoeven

On Mon, Apr 11, 2022 at 11:12 PM Javier Martinez Canillas <[email protected]> wrote:

Quoted 8 lines

Quoted 7 lines

drivers/gpu/drm/solomon/ssd130x.c struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap) · line 894

Quoted 3 lines

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

-       ssd130x->device_info = device_get_match_data(dev);
+
+       variant = (enum ssd130x_variants)device_get_match_data(dev);

(uintptr_t), to avoid a cast from pointer to integer of different size warning.

Quoted 1 line

Indeed. The kernel test robot reported the same.

Not only because of this, but also with the non-NULL pointers I prefer the old style without ugly castings.

Instead, you may export the array (in the driver's namespace) and use &info[ID] pointer for the specific device info.

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Javier Martinez Canillas

Quoted 2 lines · Geert Uytterhoeven

On Mon, Apr 11, 2022 at 11:12 PM Javier Martinez Canillas <[email protected]> wrote:

...

Quoted 10 lines

Quoted 9 lines

Quoted 7 lines

drivers/gpu/drm/solomon/ssd130x.c struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap) · line 894

Quoted 3 lines

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

-       ssd130x->device_info = device_get_match_data(dev);
+
+       variant = (enum ssd130x_variants)device_get_match_data(dev);

(uintptr_t), to avoid a cast from pointer to integer of different size warning.

Indeed. The kernel test robot reported the same.

Quoted 5 lines

Not only because of this, but also with the non-NULL pointers I prefer the old style without ugly castings.

Instead, you may export the array (in the driver's namespace) and use &info[ID] pointer for the specific device info.

Note that device_get_match_data() has no clue if the data is absent or data == (void *)0.

Quoted 3 lines · Javier Martinez Canillas

Quoted 2 lines · Geert Uytterhoeven

On Mon, Apr 11, 2022 at 11:12 PM Javier Martinez Canillas <[email protected]> wrote:

...

Quoted 8 lines

Quoted 7 lines

drivers/gpu/drm/solomon/ssd130x.c struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap) · line 894

Quoted 3 lines

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

-       ssd130x->device_info = device_get_match_data(dev);
+
+       variant = (enum ssd130x_variants)device_get_match_data(dev);

(uintptr_t), to avoid a cast from pointer to integer of different size warning.

Quoted 1 line

Indeed. The kernel test robot reported the same.

Not only because of this, but also with the non-NULL pointers I prefer the old style without ugly castings.

Instead, you may export the array (in the driver's namespace) and use &info[ID] pointer for the specific device 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.

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

Acked-by Mark Brown <[email protected]>

drivers/gpu/drm/solomon/ssd130x-spi.c new file +17 -0

 
+static struct gpio_desc *ssd130x_spi_get_dc(struct device *dev)
+{
+       struct gpio_desc *dc = devm_gpiod_get(dev, "dc", GPIOD_OUT_LOW);
+
+       if (IS_ERR(dc))
+               return ERR_PTR(dev_err_probe(dev, PTR_ERR(dc), "Failed to get dc gpio\n"));
+
+       return dc;
+}
+
+static int ssd130x_spi_probe(struct spi_device *spi)
+{
+       struct ssd130x_spi_transport *t;
+       struct ssd130x_device *ssd130x;
+       struct regmap *regmap;
+       struct device *dev = &spi->dev;
+

drivers/gpu/drm/solomon/ssd130x-spi.c line 98

Quoted 6 lines

drivers/gpu/drm/solomon/ssd130x-spi.c new file +6 -0

+
+       t->spi = spi;
+
+       t->dc = ssd130x_spi_get_dc(&spi->dev);
+       if (IS_ERR(t->dc))
+               return PTR_ERR(t->dc);

This can be simplified (no need for the PTR_ERR(ERR_PTR(...) dance) by open-coding ssd130x_spi_get_dc() here.