Hello,
This series adds a ssd130x-spi driver that provides a 4-wire SPI transport support for SSD130x OLED controllers that can be accessed through a SPI.
The driver is quite similar to existing ssd130x-i2c driver that is used by I2C controllers, but there is a difference in the protocol used by SSD130x depending on the transport used. The details are in patch #4 description.
Patch #1 just makes the current ssd130x-i2c compatible strings in the DT binding to be deprecated, and add new ones that don't have an -fb suffix.
Patch #2 extends the DT binding with the compatible string and properties needed to support the ssd130x-spi devices.
Patch #3 adds the new compatible strings to the OF device ID table in the ssd130x-i2c DRM driver.
Patch #4 moves the device info for the different SSD130x variants from the ssd130x-i2c transport driver to the ssd130x core driver.
Finally patch #5 adds the ssd130x-spi DRM driver for the OLED controllers that come with a 4-wire SPI interface, instead of an I2C interface.
Best regards, Javier
Javier Martinez Canillas (5): dt-bindings: display: ssd1307fb: Deprecate fbdev compatible strings dt-bindings: display: ssd1307fb: Extend schema for SPI controllers drm/solomon: Add ssd130x-i2c compatible strings without an -fb suffix drm/solomon: Move device info from ssd130x-i2c to the core driver drm/solomon: Add SSD130x OLED displays SPI support
.../bindings/display/solomon,ssd1307fb.yaml | 117 ++++++++--- drivers/gpu/drm/solomon/Kconfig | 9 + drivers/gpu/drm/solomon/Makefile | 1 + drivers/gpu/drm/solomon/ssd130x-i2c.c | 60 +++--- drivers/gpu/drm/solomon/ssd130x-spi.c | 184 ++++++++++++++++++ drivers/gpu/drm/solomon/ssd130x.c | 60 +++++- drivers/gpu/drm/solomon/ssd130x.h | 13 ++ 7 files changed, 376 insertions(+), 68 deletions(-) create mode 100644 drivers/gpu/drm/solomon/ssd130x-spi.c
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 javierm@redhat.com ---
.../bindings/display/solomon,ssd1307fb.yaml | 36 ++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml index ade61d502edd..46207f2c12b8 100644 --- a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml +++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml @@ -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 + deprecated: true + + # SSD130x I2C controllers + - items: + - enum: + - sinowealth,sh1106-i2c + - solomon,ssd1305-i2c + - solomon,ssd1306-i2c + - solomon,ssd1307-i2c + - solomon,ssd1309-i2c
reg: maxItems: 1 @@ -148,7 +160,7 @@ allOf: properties: compatible: contains: - const: solomon,ssd1305fb-i2c + const: solomon,ssd1305-i2c then: properties: solomon,dclk-div: @@ -160,7 +172,7 @@ allOf: properties: compatible: contains: - const: solomon,ssd1306fb-i2c + const: solomon,ssd1306-i2c then: properties: solomon,dclk-div: @@ -172,7 +184,7 @@ allOf: properties: compatible: contains: - const: solomon,ssd1307fb-i2c + const: solomon,ssd1307-i2c then: properties: solomon,dclk-div: @@ -186,7 +198,7 @@ allOf: properties: compatible: contains: - const: solomon,ssd1309fb-i2c + const: solomon,ssd1309-i2c then: properties: solomon,dclk-div: @@ -203,14 +215,14 @@ examples: #size-cells = <0>;
ssd1307: oled@3c { - compatible = "solomon,ssd1307fb-i2c"; + compatible = "solomon,ssd1307-i2c"; reg = <0x3c>; pwms = <&pwm 4 3000>; reset-gpios = <&gpio2 7>; };
ssd1306: oled@3d { - compatible = "solomon,ssd1306fb-i2c"; + compatible = "solomon,ssd1306-i2c"; reg = <0x3c>; pwms = <&pwm 4 3000>; reset-gpios = <&gpio2 7>;
On Thu, Apr 07, 2022 at 10:02:00PM +0200, Javier Martinez Canillas wrote:
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.
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 javierm@redhat.com
.../bindings/display/solomon,ssd1307fb.yaml | 36 ++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml index ade61d502edd..46207f2c12b8 100644 --- a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml +++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml @@ -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-i2cdeprecated: true# 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
Hello Rob,
On 4/8/22 20:22, Rob Herring wrote:
On Thu, Apr 07, 2022 at 10:02:00PM +0200, Javier Martinez Canillas wrote:
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.
No, that's not what I meant. In fact, we currently have two drivers that match against the same set of compatible strings. These drivers are:
* drivers/video/fbdev/ssd1307fb.c * drivers/gpu/drm/solomon/ssd130x-i2c.c
what I don't personally like about the current compatible strings is that the *driver* name was encoded on those, rather than the IC names. So for instance there's a "solomon,ssd1307fb-i2c" (notice the fb suffix) instead of just "solomon,ssd1307-i2c" or "solomon,ssd1307".
When I ported the fbdev driver to DRM, I considered using different values for the compatible strings but decided to just use the same for backward compatibility.
But now I want to add compatible strings for OLED controllers that use a SPI interface instead, and I don't really want to add a compatible string "solomon,ssd1307fb-spi" but just without the "fb".
I want the SPI compatible strings to be consistent with the I2C ones though, hence the deprecation so new DTS could just use the ones without a "fb".
Now, if you say that I can't do add new ones for I2C, then I will just add "solomon,ssd1307fb-spi" and similar even though I don't like the "fb" there.
And just to make clear, the DRM driver will continue matching against both compatible strings, but the fbdev will only match the old "fb" ones.
[snip]
# SSD130x I2C controllers- items:- enum:- sinowealth,sh1106-i2c- solomon,ssd1305-i2c- solomon,ssd1306-i2c- solomon,ssd1307-i2c- solomon,ssd1309-i2cThere'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.
For example,
$ cat /sys/devices/platform/soc/fe804000.i2c/i2c-1/1-003c/modalias of:NoledT(null)Csolomon,ssd1306fb-i2c
$ cat /sys/devices/platform/soc/fe804000.i2c/i2c-1/1-003c/uevent DRIVER=ssd130x-i2c OF_NAME=oled OF_FULLNAME=/soc/i2c@7e804000/oled@3c OF_COMPATIBLE_0=solomon,ssd1306fb-i2c OF_COMPATIBLE_N=1 MODALIAS=of:NoledT(null)Csolomon,ssd1306fb-i2c
and
$ modinfo ssd130x-i2c | grep alias alias: of:N*T*Csolomon,ssd1309fb-i2cC* alias: of:N*T*Csolomon,ssd1309fb-i2c alias: of:N*T*Csolomon,ssd1307fb-i2cC* alias: of:N*T*Csolomon,ssd1307fb-i2c alias: of:N*T*Csolomon,ssd1306fb-i2cC* alias: of:N*T*Csolomon,ssd1306fb-i2c alias: of:N*T*Csolomon,ssd1305fb-i2cC* alias: of:N*T*Csolomon,ssd1305fb-i2c alias: of:N*T*Csinowealth,sh1106-i2cC* alias: of:N*T*Csinowealth,sh1106-i2c
this module will match against any MODALIAS uevent that has one of the listed compatible strings in "C" and any node name in "N". But also for any type "T".
And even if the module alias was more restrictive and say only matched against 'of:N*Ti2cCsolomon,ssd1307fb-i2c', the type information is not filled by the bus drivers.
So, if we just had a "solomon,ssd1307" compatible string, then a device registered through OF could lead to the wrong kernel module to be loaded.
In other words, it's true that having a single compatible strings for all bus drivers will work for device -> driver matching but may not work for module auto-loading.
(You should also support the 'fb-i2c' variant in DRM IMO, but doubtful that I'll review that.)
As mentioned above, it does even after adding support for the new strings, for backward compatibility.
Rob
On 4/8/22 21:19, Javier Martinez Canillas wrote:
[snip]
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.
Forgot to mention that in this particular case it will work but just because the SPI subsystem always report a module alias of the form "spi:device" even for devices that are registered through OF.
So having a single "solomon,ssd1306" would work because for I2C the module alias will be "of:NoledT(null)Csolomon,ssd1306" and for SPI it will be "spi:ssd1306".
But if ever the SPI subsystem is fixed to report proper OF module aliases things will break. And since the DT bindings is an ABI, it's safer to have "-i2c" and "-spi" compatible strings variants.
Hi Javier,
On Thu, Apr 7, 2022 at 10:03 PM Javier Martinez Canillas javierm@redhat.com wrote:
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 javierm@redhat.com
--- a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml +++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml @@ -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
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"'
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hello Geert,
On 4/11/22 15:47, Geert Uytterhoeven wrote:
Hi Javier,
On Thu, Apr 7, 2022 at 10:03 PM Javier Martinez Canillas javierm@redhat.com wrote:
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 javierm@redhat.com
--- a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml +++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml @@ -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-i2cPlease 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"'
Yes, I know but was worried about the potential issues that mentioned in a previous email in this thread. But after the discussion we had over IRC, I think that is safe to assume that the SPI subsystem won't change how the modaliases are reported, so there won't be conflict between I2C and SPI.
And if that is ever changed, there's a plan to add the bus type to the data reported by the modalias uevent so user-space could figure out what to load.
So I'll go ahead with Rob and yours suggestion, and just deprecate the old ones and drop both the "fb" and "-i2c" part of the compatible strings, to use the same compatible strings for both the I2C and SPI drivers.
After all, that's the correct way to describe the hardware and not encode any Linux implementation details in the DT binding.
The Solomon SSD130x OLED displays can either have an I2C or SPI interface, add to the schema the compatible strings, properties and examples for SPI.
Signed-off-by: Javier Martinez Canillas javierm@redhat.com ---
.../bindings/display/solomon,ssd1307fb.yaml | 89 +++++++++++++++---- 1 file changed, 71 insertions(+), 18 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml index 46207f2c12b8..05e7975296a7 100644 --- a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml +++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml @@ -31,6 +31,15 @@ properties: - solomon,ssd1307-i2c - solomon,ssd1309-i2c
+ # SSD130x SPI controllers + - items: + - enum: + - sinowealth,sh1106-spi + - solomon,ssd1305-spi + - solomon,ssd1306-spi + - solomon,ssd1307-spi + - solomon,ssd1309-spi + reg: maxItems: 1
@@ -40,9 +49,14 @@ properties: reset-gpios: maxItems: 1
+ dc-gpios: + maxItems: 1 + vbat-supply: description: The supply for VBAT
+ spi-max-frequency: true + solomon,height: $ref: /schemas/types.yaml#/definitions/uint32 default: 16 @@ -148,19 +162,10 @@ allOf: properties: compatible: contains: - const: sinowealth,sh1106-i2c - then: - properties: - solomon,dclk-div: - default: 1 - solomon,dclk-frq: - default: 5 - - - if: - properties: - compatible: - contains: - const: solomon,ssd1305-i2c + enum: + - sinowealth,sh1106-i2c + - solomon,ssd1305-i2c + - solomon,ssd1305-spi then: properties: solomon,dclk-div: @@ -172,7 +177,9 @@ allOf: properties: compatible: contains: - const: solomon,ssd1306-i2c + enum: + - solomon,ssd1306-i2c + - solomon,ssd1306-spi then: properties: solomon,dclk-div: @@ -184,7 +191,9 @@ allOf: properties: compatible: contains: - const: solomon,ssd1307-i2c + enum: + - solomon,ssd1307-i2c + - solomon,ssd1307-spi then: properties: solomon,dclk-div: @@ -198,7 +207,9 @@ allOf: properties: compatible: contains: - const: solomon,ssd1309-i2c + enum: + - solomon,ssd1309-i2c + - solomon,ssd1309-spi then: properties: solomon,dclk-div: @@ -206,6 +217,21 @@ allOf: solomon,dclk-frq: default: 10
+ - 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 + additionalProperties: false
examples: @@ -214,14 +240,14 @@ examples: #address-cells = <1>; #size-cells = <0>;
- ssd1307: oled@3c { + ssd1307_i2c: oled@3c { compatible = "solomon,ssd1307-i2c"; reg = <0x3c>; pwms = <&pwm 4 3000>; reset-gpios = <&gpio2 7>; };
- ssd1306: oled@3d { + ssd1306_i2c: oled@3d { compatible = "solomon,ssd1306-i2c"; reg = <0x3c>; pwms = <&pwm 4 3000>; @@ -232,3 +258,30 @@ examples: solomon,lookup-table = /bits/ 8 <0x3f 0x3f 0x3f 0x3f>; }; }; + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + + ssd1307_spi: oled@0 { + compatible = "solomon,ssd1307-spi"; + reg = <0x0>; + pwms = <&pwm 4 3000>; + reset-gpios = <&gpio2 7>; + dc-gpios = <&gpio2 8>; + spi-max-frequency = <10000000>; + }; + + ssd1306_spi: oled@1 { + compatible = "solomon,ssd1306-spi"; + reg = <0x1>; + pwms = <&pwm 4 3000>; + reset-gpios = <&gpio2 7>; + dc-gpios = <&gpio2 8>; + spi-max-frequency = <10000000>; + solomon,com-lrremap; + solomon,com-invdir; + solomon,com-offset = <32>; + solomon,lookup-table = /bits/ 8 <0x3f 0x3f 0x3f 0x3f>; + }; + };
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 new ones that don't have the -fb suffix. The old entries are still kept for backward compatibility.
Signed-off-by: Javier Martinez Canillas javierm@redhat.com ---
drivers/gpu/drm/solomon/ssd130x-i2c.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/solomon/ssd130x-i2c.c b/drivers/gpu/drm/solomon/ssd130x-i2c.c index d099b241dd3f..a469679548f8 100644 --- a/drivers/gpu/drm/solomon/ssd130x-i2c.c +++ b/drivers/gpu/drm/solomon/ssd130x-i2c.c @@ -91,6 +91,23 @@ static const struct of_device_id ssd130x_of_match[] = { .compatible = "sinowealth,sh1106-i2c", .data = &ssd130x_sh1106_deviceinfo, }, + { + .compatible = "solomon,ssd1305-i2c", + .data = &ssd130x_ssd1305_deviceinfo, + }, + { + .compatible = "solomon,ssd1306-i2c", + .data = &ssd130x_ssd1306_deviceinfo, + }, + { + .compatible = "solomon,ssd1307-i2c", + .data = &ssd130x_ssd1307_deviceinfo, + }, + { + .compatible = "solomon,ssd1309-i2c", + .data = &ssd130x_ssd1309_deviceinfo, + }, + /* Deprecated but remain for backward compatibility */ { .compatible = "solomon,ssd1305fb-i2c", .data = &ssd130x_ssd1305_deviceinfo,
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 wens@kernel.org Signed-off-by: Javier Martinez Canillas javierm@redhat.com ---
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(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x-i2c.c b/drivers/gpu/drm/solomon/ssd130x-i2c.c index a469679548f8..aa6cc2cb54f9 100644 --- a/drivers/gpu/drm/solomon/ssd130x-i2c.c +++ b/drivers/gpu/drm/solomon/ssd130x-i2c.c @@ -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 */ } }; diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index a7e784518c69..1f00fd3c0023 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -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 @@ -94,6 +92,55 @@
#define MAX_CONTRAST 255
+static struct ssd130x_deviceinfo ssd130x_variants[] = { + { + .default_vcomh = 0x40, + .default_dclk_div = 1, + .default_dclk_frq = 5, + .page_mode_only = 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; diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h index f5b062576fdf..4e0b62a41aa3 100644 --- a/drivers/gpu/drm/solomon/ssd130x.h +++ b/drivers/gpu/drm/solomon/ssd130x.h @@ -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, + SSD1307_ID, + SSD1309_ID, + NR_SSD130X_VARIANTS +}; + struct ssd130x_deviceinfo { + enum ssd130x_variants variant; u32 default_vcomh; u32 default_dclk_div; u32 default_dclk_frq;
Hi,
On 07/04/2022 22:02, Javier Martinez Canillas wrote:
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 wens@kernel.org Signed-off-by: Javier Martinez Canillas javierm@redhat.com
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(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x-i2c.c b/drivers/gpu/drm/solomon/ssd130x-i2c.c index a469679548f8..aa6cc2cb54f9 100644 --- a/drivers/gpu/drm/solomon/ssd130x-i2c.c +++ b/drivers/gpu/drm/solomon/ssd130x-i2c.c @@ -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,
.data = &ssd130x_ssd1305_deviceinfo,
.data = (void *)SSD1305_ID,
.data = &ssd130x_ssd1306_deviceinfo,
.data = (void *)SSD1306_ID,
.data = &ssd130x_ssd1307_deviceinfo,
.data = (void *)SSD1307_ID,
.data = &ssd130x_ssd1309_deviceinfo,
.data = (void *)SSD1309_ID,
.data = &ssd130x_ssd1305_deviceinfo,
.data = (void *)SSD1305_ID,
.data = &ssd130x_ssd1306_deviceinfo,
.data = (void *)SSD1306_ID,
.data = &ssd130x_ssd1307_deviceinfo,
.data = (void *)SSD1307_ID,
.data = &ssd130x_ssd1309_deviceinfo,
.data = (void *)SSD1309_ID,diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index a7e784518c69..1f00fd3c0023 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -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
@@ -94,6 +92,55 @@
#define MAX_CONTRAST 255
+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 ?
- {
.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;
diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h index f5b062576fdf..4e0b62a41aa3 100644 --- a/drivers/gpu/drm/solomon/ssd130x.h +++ b/drivers/gpu/drm/solomon/ssd130x.h @@ -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,
- SSD1307_ID,
- SSD1309_ID,
- NR_SSD130X_VARIANTS
+};
- struct ssd130x_deviceinfo {
- enum ssd130x_variants variant; u32 default_vcomh; u32 default_dclk_div; u32 default_dclk_frq;
Neil
Hello Neil,
Thanks for your feedback.
On 4/8/22 09:49, Neil Armstrong wrote:
[snip]
+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 ?
Indeed. I'll change that in v2.
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 javierm@redhat.com ---
drivers/gpu/drm/solomon/Kconfig | 9 ++ drivers/gpu/drm/solomon/Makefile | 1 + drivers/gpu/drm/solomon/ssd130x-spi.c | 184 ++++++++++++++++++++++++++ 3 files changed, 194 insertions(+) create mode 100644 drivers/gpu/drm/solomon/ssd130x-spi.c
diff --git a/drivers/gpu/drm/solomon/Kconfig b/drivers/gpu/drm/solomon/Kconfig index 8c0a0c788385..e170716d976b 100644 --- a/drivers/gpu/drm/solomon/Kconfig +++ b/drivers/gpu/drm/solomon/Kconfig @@ -20,3 +20,12 @@ config DRM_SSD130X_I2C I2C bus.
If M is selected the module will be called ssd130x-i2c. + +config DRM_SSD130X_SPI + tristate "DRM support for Solomon SSD130X OLED displays (SPI bus)" + depends on DRM_SSD130X && SPI + select REGMAP + help + Say Y here if the SSD130x OLED display is connected via SPI bus. + + If M is selected the module will be called ssd130x-spi. diff --git a/drivers/gpu/drm/solomon/Makefile b/drivers/gpu/drm/solomon/Makefile index 4bfc5acb0447..b5fc792257d7 100644 --- a/drivers/gpu/drm/solomon/Makefile +++ b/drivers/gpu/drm/solomon/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_DRM_SSD130X) += ssd130x.o obj-$(CONFIG_DRM_SSD130X_I2C) += ssd130x-i2c.o +obj-$(CONFIG_DRM_SSD130X_SPI) += ssd130x-spi.o diff --git a/drivers/gpu/drm/solomon/ssd130x-spi.c b/drivers/gpu/drm/solomon/ssd130x-spi.c new file mode 100644 index 000000000000..c3a2e7ba67a1 --- /dev/null +++ b/drivers/gpu/drm/solomon/ssd130x-spi.c @@ -0,0 +1,184 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * DRM driver for Solomon SSD130X OLED displays (SPI bus) + * + * Copyright 2022 Red Hat Inc. + * Authors: Javier Martinez Canillas javierm@redhat.com + */ +#include <linux/spi/spi.h> +#include <linux/module.h> + +#include "ssd130x.h" + +#define DRIVER_NAME "ssd130x-spi" +#define DRIVER_DESC "DRM driver for Solomon SSD130X OLED displays (SPI)" + +struct ssd130x_spi_transport { + struct spi_device *spi; + struct gpio_desc *dc; +}; + +static const struct regmap_config ssd130x_spi_regmap_config = { + .reg_bits = 8, + .val_bits = 8, +}; + +/* + * The regmap bus .write handler, it is just a wrapper around spi_write() + * but toggling the Data/Command control pin (D/C#). Since for 4-wire SPI + * a D/C# pin is used, in contrast with I2C where a control byte is sent, + * prior to every data byte, that contains a bit with the D/C# value. + * + * These control bytes are considered registers by the ssd130x core driver + * and can be used by the ssd130x SPI driver to determine if the data sent + * is for a command register or for the Graphic Display Data RAM (GDDRAM). + */ +static int ssd130x_spi_write(void *context, const void *data, size_t count) +{ + struct ssd130x_spi_transport *t = context; + struct spi_device *spi = t->spi; + const u8 *reg = data; + + if (*reg == SSD130X_COMMAND) + gpiod_set_value_cansleep(t->dc, 0); + + if (*reg == SSD130X_DATA) + gpiod_set_value_cansleep(t->dc, 1); + + /* Remove the control byte since is not used by the 4-wire SPI */ + return spi_write(spi, ((u8 *)data) + 1, count - 1); +} + +/* The ssd130x driver does not read registers but regmap expects a .read */ +static int ssd130x_spi_read(void *context, const void *reg, size_t reg_size, + void *val, size_t val_size) +{ + return 0; +} + +/* + * A custom bus is needed due the special write that toggles a D/C# pin, + * another option could be to just have a .reg_write() callback but that + * will prevent to do data writes in bulk. + * + * Once the regmap API is extended to support defining a bulk write handler + * in the struct regmap_config, this can be simplified and the bus dropped. + */ +static struct regmap_bus regmap_ssd130x_spi_bus = { + .write = ssd130x_spi_write, + .read = ssd130x_spi_read, +}; + +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; + + t = devm_kzalloc(dev, sizeof(*t), GFP_KERNEL); + if (!t) + return dev_err_probe(dev, -ENOMEM, + "Failed to allocate SPI transport data\n"); + + t->spi = spi; + + t->dc = ssd130x_spi_get_dc(&spi->dev); + if (IS_ERR(t->dc)) + return PTR_ERR(t->dc); + + regmap = devm_regmap_init(dev, ®map_ssd130x_spi_bus, t, + &ssd130x_spi_regmap_config); + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + + ssd130x = ssd130x_probe(dev, regmap); + if (IS_ERR(ssd130x)) + return PTR_ERR(ssd130x); + + spi_set_drvdata(spi, ssd130x); + + return 0; +} + +static void ssd130x_spi_remove(struct spi_device *spi) +{ + struct ssd130x_device *ssd130x = spi_get_drvdata(spi); + + ssd130x_remove(ssd130x); +} + +static void ssd130x_spi_shutdown(struct spi_device *spi) +{ + struct ssd130x_device *ssd130x = spi_get_drvdata(spi); + + ssd130x_shutdown(ssd130x); +} + +static const struct of_device_id ssd130x_of_match[] = { + { + .compatible = "sinowealth,sh1106-spi", + .data = (void *)SH1106_ID, + }, + { + .compatible = "solomon,ssd1305-spi", + .data = (void *)SSD1305_ID, + }, + { + .compatible = "solomon,ssd1306-spi", + .data = (void *)SSD1306_ID, + }, + { + .compatible = "solomon,ssd1307-spi", + .data = (void *)SSD1307_ID, + }, + { + .compatible = "solomon,ssd1309-spi", + .data = (void *)SSD1309_ID, + }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, ssd130x_of_match); + +/* + * The SPI core always reports a MODALIAS uevent of the form "spi:<dev>", even + * if the device was registered via OF. This means that the module will not be + * auto loaded, unless it contains an alias that matches the MODALIAS reported. + * + * To workaround this issue, add a SPI device ID table. Even when this should + * not be needed for this driver to match the registered SPI devices. + */ +static const struct spi_device_id ssd130x_spi_table[] = { + { "sh1106-spi", SH1106_ID }, + { "ssd1305-spi", SSD1305_ID }, + { "ssd1306-spi", SSD1306_ID }, + { "ssd1307-spi", SSD1307_ID }, + { "ssd1309-spi", SSD1309_ID }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(spi, ssd130x_spi_table); + +static struct spi_driver ssd130x_spi_driver = { + .driver = { + .name = DRIVER_NAME, + .of_match_table = ssd130x_of_match, + }, + .probe = ssd130x_spi_probe, + .remove = ssd130x_spi_remove, + .shutdown = ssd130x_spi_shutdown, +}; +module_spi_driver(ssd130x_spi_driver); + +MODULE_DESCRIPTION(DRIVER_DESC); +MODULE_AUTHOR("Javier Martinez Canillas javierm@redhat.com"); +MODULE_LICENSE("GPL");
On Thu, Apr 07, 2022 at 10:02:04PM +0200, Javier Martinez Canillas wrote:
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.
Acked-by: Mark Brown broonie@kernel.org
dri-devel@lists.freedesktop.org
-
Geert Uytterhoeven -
Javier Martinez Canillas -
Mark Brown -
Neil Armstrong -
Rob Herring