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

[Maxime Ripard <maxime.ripard@free-electrons.com>]

[-- Attachment #1: Type: text/plain, Size: 9563 bytes --]

On Sat, Feb 14, 2015 at 05:12:32PM +0100, Thomas Niederprüm wrote:
> Am Thu, 12 Feb 2015 17:41:47 +0100
> schrieb Maxime Ripard <maxime.ripard@free-electrons.com>:
> 
> > 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 <maxime.ripard@free-electrons.com>:
> > > 
> > > > Hi,
> > > > 
> > > > On Fri, Feb 06, 2015 at 11:28:08PM +0100, niederp@physik.uni-kl.de
> > > > wrote:
> > > > > From: Thomas Niederprüm <niederp@physik.uni-kl.de>
> > > > > 
> > > > > 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 <niederp@physik.uni-kl.de>
> > > > > ---
> > > > >  .../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?
> 
> One good example is the segment remap. It basically allows to invert the
> order of the output pins connecting to the oled panel. This gives the
> manufacturer of the module the freedom wire it the one way or the
> other, depending on the PCB restrictions/panel layout (Section 10.1.12
> of [0], 10.1.8 in [1], 9.1.8 in [2]). However, once the panel is
> connected to the controller it's determined whether the segment remap
> is needed or not. Setting the segment remap as done in the current
> initialization code for ssd1306 and ssd1307 makes my display module
> show it's contents mirrored left to right, probably since the
> manufacturer decided not to connect the panel in an inverted order.
> 
> The same applies to the com-alt, com-lrremap and com-invdir values,
> which define different possibilities for the COM signals pin
> configuration (Section 10.1.26 of [0], 10.1.18 in [1], 9.1.18 in [2])
> and readout direction of the video memory (Section 10.1.21 of [0],
> 10.1.14 in [1], 9.1.14 in [2]). Setting com-alt incorrectly leaves
> every other line of the display blank. Setting com-lrremap incorrectly
> produces a very distorted image. Setting com-invdir incorrectly flips
> the image upside down.
> 
> IMHO at least these four hardware-specific properties need to be known
> to the driver in order to initialize the hardware correctly.

I'd agree then.

> > Does it depend on the X, or can it change from one same controller to
> > another? to what extent?
> 
> Unfortunately I do not posses any hardware utilizing a ssd1306 or
> ssd1307 controller. My primary and only target device is a Newhaven
> NHD-3.12-25664UCB2 OLED display module using an SSD1305 controller. I
> just inferred from the datasheets of ssd1306/7 [1,2] that they should
> behave the same since the registers are bit to bit identical (except
> for the VHCOM register). Maybe that was a bit too naive :/

I would guess it's a rather safe assumption.

> > The 1306 for example seems to not be using these values at all, while
> > the 1307 does.
> 
> That is surprising. In that case I would like to ask the guys from
> Solomon why they describe all these options in the SSD1306 datasheet
> [1]. But in any case, isn't that good news for the problem of setting
> the default values. When the 1306 isn't using these values anyway we can
> not break the initialization by setting different default values. In
> this case the problem of the default values boils down to the segment
> remap only since this is set in the init code of the 1307, while the
> default would be to leave it off.

Indeed.

> > > 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 :)
> 
> I didn't intend to be rude, sorry. A quick search revealed that there
> is luckily only one in-tree user, which is imx28-cfa10036.dts. In case
> it will be necessary I will include a patch to fix this.

Please do (and fix the bindings Documentation too).

> > > > 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.
> 
> I'll give that a try, even though that particular one is not trivial
> since the documentation on the actual frequency that is set by the
> dclk-freq is very poor (not present for 1306/1307 [1,2], just a graph
> for 1305 [0]).
> 
> For the properties describing the hardware pin configuration (see above)
> I see no real alternative. Maybe they can all be covered by one DT
> property like:
> solomon,com-cfg = PINCFG_SEGREMAP | PINCFG_COMALT | PINCFG_COMINV |
> PINCFG_COMLRRM
> each PINCFG_* setting one bit. The driver will then translate this into
> the correct settings for the 130X registers. The only problem here is
> that this implicitly assumes the default values of each bit to be 0.

A property that would be here or not is better. You can have all the
defaults you want, it's more clear in the DT, and you don't need the
macros.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]