Re: [PATCH 2/8] fbdev: ssd1307fb: Unify init code and make controller configurable from device tree

On Sat, Feb 07, 2015 at 03:59:33PM +0100, Thomas Niederprüm wrote:

Am Sat, 7 Feb 2015 11:42:25 +0100
schrieb Maxime Ripard [off-list ref]:

quoted

Hi,

On Fri, Feb 06, 2015 at 11:28:08PM +0100, niederp@physik.uni-kl.de
wrote:

quoted

From: Thomas Niederprüm <redacted>

This patches unifies the init code for the ssd130X chips and
adds device tree bindings to describe the hardware configuration
of the used controller. This gets rid of the magic bit values
used in the init code so far.

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

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

b/Documentation/devicetree/bindings/video/ssd1307fb.txt index
7a12542..1230f68 100644 ---
a/Documentation/devicetree/bindings/video/ssd1307fb.txt +++
b/Documentation/devicetree/bindings/video/ssd1307fb.txt @@ -15,6
+15,17 @@ Required properties: 
 Optional properties:
   - reset-active-low: Is the reset gpio is active on physical low?
+  - solomon,segment-remap: Invert the order of data column to
segment mapping
+  - solomon,offset: Map the display start line to one of COM0 -
COM63
+  - solomon,contrast: Set initial contrast of the display
+  - solomon,prechargep1: Set the duration of the precharge period
phase1
+  - solomon,prechargep2: Set the duration of the precharge period
phase2
+  - solomon,com-alt: Enable/disable alternate COM pin configuration
+  - solomon,com-lrremap: Enable/disable left-right remap of COM
pins
+  - solomon,com-invdir: Invert COM scan direction
+  - solomon,vcomh: Set VCOMH regulator voltage
+  - solomon,dclk-div: Set display clock divider
+  - solomon,dclk-frq: Set display clock frequency

I'm sorry, but this is the wrong approach, for at least two reasons:
you broke all existing users of that driver, which is a clear no-go,

Unfortunately this is true. The problem is that the SSD130X
controllers allow for a very versatile wiring of the display to the
controller.  It's over to the manufacturer of the OLED module
(disp+controller) to decide how it's actually wired and during
device initialization the driver has to take care to configure the
SSD130X controller according to that wiring. If the driver fails to
do so you will end up having your display showing
garbage.

How so?

Does it depend on the X, or can it change from one same controller to
another? to what extent?

The 1306 for example seems to not be using these values at all, while
the 1307 does.

Unfortunately the current sate of the initialization code of the
ssd1307fb driver is not very flexible in that respect. Taking a look
at the initialization code for the ssd1306 shows that it was written
with one very special display module in mind. Most of the magic bit
values set there are non-default values according to the
datasheet. The result is that the driver works with that one
particular display module but many other (differently wired) display
modules using a ssd1306 controller won't work without changing the
hardcoded magic bit values.

My idea here was to set all configuration to the default values (as
given in the datasheet) unless it is overwritten by DT. Of course,
without a change in DT, this breaks the driver for all existing users.
The only alternative would be to set the current values as default.
Somehow this feels wrong to me as these values look arbitrary when you
don't know what exact display module they were set for. But if you
insist, I will change the default values.

Unfortunately, the DT bindings are to be considered an ABI, and we
should support booting with older DTs (not that I personally care
about it, but that's another issue). So we really don't have much
choice here.

Moreover, that issue left aside, modifying bindings like this without
fixing up the in-tree users is considered quite rude :)

quoted

and the DT itself should not contain any direct mapping of the
registers.

I think I don't get what you mean here. Is it because I do no sanity
checks of the numbers set in DT? I was just looking for a way to hand
over the information about the wiring of display to the driver. How
would you propose to solve this?

What I meant was that replicating direct registers value is usually a
recipe for a later failure, especially if we can have the information
under a generic and easy to understand manner.

For example, replacing the solomon,dclk-div and solomon,dclk-frq
properties by a clock-frequency property in Hz, and computing the
divider and that register in your driver is usually better, also
because it allows to have different requirements / algorithms to
compute that if some other chip needs it.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com