Re: [PATCHv5 04/11] fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT

Am Wed, 25 Mar 2015 16:49:47 +0100
schrieb Olliver Schinagl [off-list ref]:

Hey Thomas,

awesome the time you put into this driver. It does what I was working
on the last few weeks, but with a much bigger bang ;)

I personally have a ssd1309 and will be submitting the patches for
that after testing your patch-set.

Yeah, it turned out to be quite some work. But I'm glad to see that it
might turn out useful for you and I'm looking forward to see support
added for even more controllers.

On 24-03-15 22:23, Thomas Niederprüm wrote:

quoted

The 130X controllers are very similar from the configuration point
of view. The configuration registers for the SSD1305/6/7 are bit
identical (except the the VHCOM register and the the default values
for clock setup register). This patch unifies the init code of the
controller and adds hardware specific properties to DT that are
needed to correctly initialize the device.

The SSD130X can be wired to the OLED panel in various ways. Even
for the same controller this wiring can differ from one display
module to another and can not be probed by software. The added DT
properties reflect these hardware decisions of the display module
manufacturer. The 'com-sequential', 'com-lrremap' and 'com-invdir'
values define different possibilities for the COM signals pin
configuration and readout direction of the video memory. The
'segment-no-remap' allows the inversion of the memory-to-pin
mapping ultimately inverting the order of the controllers output
pins. The 'prechargepX' values need to be adapted according to the
capacitance of the OLEDs pixel cells.

So far these hardware specific bits are hard coded in the init
code, making the driver usable only for one certain wiring of the
controller. This patch makes the driver usable with all possible
hardware setups, given a valid hw description in DT. If these
values are not set in DT the default values, as they are set in the
ssd1307 init code right now, are used. This implies that without
the corresponding DT property "segment-no-remap" the segment remap
of the ssd130X controller gets activated. Even though this is not
the default behaviour according to the datasheet it maintains
backward compatibility with older DTBs.

Note that the SSD1306 does not seem to be using the configuration
written to the registers at all. Therefore this patch does not try
to maintain these values without changes in DT. For reference an
example is added to the DT bindings documentation that reproduces
the configuration that is set in the current init code.

Signed-off-by: Thomas Niederprüm <redacted>
---
  .../devicetree/bindings/video/ssd1307fb.txt        |  21 +++
  drivers/video/fbdev/ssd1307fb.c                    | 192
++++++++++++--------- 2 files changed, 134 insertions(+), 79
deletions(-)

diff --git a/Documentation/devicetree/bindings/video/ssd1307fb.txt

b/Documentation/devicetree/bindings/video/ssd1307fb.txt index
7a12542..637690f 100644 ---
a/Documentation/devicetree/bindings/video/ssd1307fb.txt +++
b/Documentation/devicetree/bindings/video/ssd1307fb.txt @@ -15,6
+15,16 @@ Required properties:

  Optional properties:
    - reset-active-low: Is the reset gpio is active on physical low?
+  - solomon,segment-no-remap: Display needs normal (non-inverted)
data column
+                              to segment mapping
+  - solomon,com-sequential: Display uses sequential COM pin
configuration
+  - solomon,com-lrremap: Display uses left-right COM pin remap
+  - solomon,com-invdir: Display uses inverted COM pin scan
direction
+  - solomon,com-offset: Number of the COM pin wired to the first
display line
+  - solomon,prechargep1: Length of deselect period (phase 1) in
clock cycles.
+  - solomon,prechargep2: Length of precharge period (phase 2) in
clock cycles.
+                         This needs to be the higher, the higher
the capacitance
+                         of the OLED's pixels is

Aren't the height, width and page-offset also optional? They should
be set to sane defaults when not supplied. Though the default (which
is the max really) width/height depends on the actual attached
display to the chip (I mention this in the vcomh section too below,
the controller's width/height would be the absolute maximum values,
the display can always be smaller).

Same goes to say for the precharge, and probably other parameters. I 
don't think we have a proper framework to handle the attached
displays to a controller chip at the moment anyhow?

quoted

  [0]: Documentation/devicetree/bindings/pwm/pwm.txt

<snip>

quoted

+
+	/* Set COM direction */
+	com_invdir = 0xc0 | (par->com_invdir & 0xf) << 3;

Shouldn't this be par->com_invdir & 0x1 ?
The datasheet only mentions 1 bit change for the ssd1306, ssd1307 and 
ssd1309. I dont' know what the ssd1305 does here though.

Yes, you are right, it should be  par->com_invdir &0x1. The
register of the 1305 is identical to the 1306/7.

quoted

+	ret = ssd1307fb_write_cmd(par->client,  com_invdir);
  	if (ret < 0)
  		return ret;

<snip>

quoted

-	ret = ssd1307fb_write_cmd(par->client, 0x14);
-	if (ret < 0)
-		return ret;
+		ret = ssd1307fb_write_cmd(par->client, 0x14);
+		if (ret < 0)
+			return ret;
+	};

I had to look hard for this setting, as my old datasheet had omitted
the charge pump def.

but wouldn't you want something like (0x10 | 
par->device_info->need_chargepump & 0x1 << 2) & 0x14 to follow your 
previous styles?

Only bit 2 determines the state of the charge-pump. And I guess it 
should be safe to use on the other chips as well. My 1309 doesn't
seem to have a problem with it, I'm working a lot on this display so
it will be tested thoroughly in the next few weeks.

I didn't look into this so far, I just took that command from the
ssd1306 init code. I just checked the datasheet of the 1306 and I agree
that par->device_info->need_chargepump & 0x1 << 2) & 0x14 should be
what we want here, assuming that the other controllers make no use of
the register 0xBD. I can confirm that my 1305 based display module
seems not to use it. I'll include your suggestion for the next round.

quoted

  	/* Switch to horizontal addressing mode */
  	ret = ssd1307fb_write_cmd(par->client,
SSD1307FB_SET_ADDRESS_MODE); @@ -393,6 +390,7 @@ static int
ssd1307fb_ssd1306_init(struct ssd1307fb_par *par) if (ret < 0)
  		return ret;

<snip>

quoted

-static struct ssd1307fb_ops ssd1307fb_ssd1306_ops = {
-	.init	= ssd1307fb_ssd1306_init,
+static struct ssd1307fb_deviceinfo ssd1307fb_ssd1306_deviceinfo = {
+	.default_vcomh = 0x20,
+	.default_dclk_div = 0,
+	.default_dclk_frq = 8,
+	.need_pwm = 0,
+	.need_chargepump = 1,
+};

This device info struct, is this really always fixed data? We use a 
ssd1309 with a MI12864MO i think and the combination of these two
parts defines atleast the vcomh I would imagine? There are only a
very limited number of consumers of this driver so for now it won't
make much of a difference, just mentioning it as it is a configurable
so it may matter depending on the physical display.

Note that the default_vcomh just holds the default value for vcomh as
stated in the controller's datasheet. It becomes necessary to define
those on a per controller basis since this one of the very few
registers that actually differ between the different ssd130X. As this
is only the default value this is fixed data.
Once it becomes relevant to set this according to the hardware of the
display module (controller + panel) I guess one has to introduce a DT
property for this. For my particular device (see below) the default
value was working nicely, which is why I did not dive into this.

quoted

+
+static struct ssd1307fb_deviceinfo ssd1307fb_ssd1307_deviceinfo = {
+	.default_vcomh = 0x20,
+	.default_dclk_div = 1,
+	.default_dclk_frq = 12,
+	.need_pwm = 1,
+	.need_chargepump = 0,
  };

  static const struct of_device_id ssd1307fb_of_match[] = {
  	{
  		.compatible = "solomon,ssd1306fb-i2c",
-		.data = (void *)&ssd1307fb_ssd1306_ops,
+		.data = (void *)&ssd1307fb_ssd1306_deviceinfo,
  	},
  	{
  		.compatible = "solomon,ssd1307fb-i2c",
-		.data = (void *)&ssd1307fb_ssd1307_ops,
+		.data = (void *)&ssd1307fb_ssd1307_deviceinfo,
  	},
  	{},
  };

@@ -468,8 +479,8 @@ static int ssd1307fb_probe(struct i2c_client

*client, par->info = info;
  	par->client = client;

-	par->ops = (struct ssd1307fb_ops
*)of_match_device(ssd1307fb_of_match,
-
&client->dev)->data;
+	par->device_info = (struct ssd1307fb_deviceinfo
*)of_match_device(
+			ssd1307fb_of_match, &client->dev)->data;

  	par->reset = of_get_named_gpio(client->dev.of_node,
  					 "reset-gpios", 0);

@@ -487,6 +498,27 @@ static int ssd1307fb_probe(struct i2c_client

*client, if (of_property_read_u32(node, "solomon,page-offset",
&par->page_offset)) par->page_offset = 1;

While your re-doing the whole driver, why use a default page_offset
of 1? 0 makes far more sense (its the default on my board also) and 
logically, we'd start counting at 0 as programmers ;) Also, the 
datasheet uses 0 as a default preset.

the page_offset is not a quantity that appears in the datasheet. Don't
mix it up with the com-offset (that is referred to as offset in the
datsheet). the page_offset is used in the init code to set the upper
limit of the page address range. 

quoted

+	if (of_property_read_u32(node, "solomon,com-offset",
&par->com_offset))
+		par->com_offset = 0;
+
+	if (of_property_read_u32(node, "solomon,prechargep1",
&par->prechargep1))
+		par->prechargep1 = 2;
+
+	if (of_property_read_u32(node, "solomon,prechargep2",
&par->prechargep2))
+		par->prechargep2 = 0;

Why set the default to 0 here? The datasheet sets it to 0x2 by
default for example.

You are right, I will correct this.

quoted

+
+	par->seg_remap = !of_property_read_bool(node,
"solomon,segment-no-remap");
+	par->com_seq = of_property_read_bool(node,
"solomon,com-sequential");

all the other parameters are somewhat abbreviated, or the property
name matches the variable name. I'd just abbreviate it to com-seq
clear and short.

Ok, why not. I will abbreviate the name for v6

Overal, the driver works and can have a Tested-by: Olliver Schinagl 
[off-list ref] from me (for the 1309 which I'll submit
very soon.

Great!

Olliver

P.S. Do you have a link for your display? I'm curious as to how it
looks.

I'm using those two (very similar) displays:
http://www.newhavendisplay.com/nhd22312832ucy3-p-3787.html
http://www.newhavendisplay.com/nhd22312832umb3-p-6086.html

Thomas