Re: [PATCH 2/8] fbdev: ssd1307fb: Unify init code and make controller configurable from device tree
From: Maxime Ripard
Date: Mon Feb 23 2015 - 04:45:13 EST
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@xxxxxxxxxxxxxxxxxx>:
> > 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@xxxxxxxxxxxxxxxxxx>:
> > >
> > > > Hi,
> > > >
> > > > On Fri, Feb 06, 2015 at 11:28:08PM +0100, niederp@xxxxxxxxxxxxxxxx
> > > > wrote:
> > > > > From: Thomas Niederprüm <niederp@xxxxxxxxxxxxxxxx>
> > > > >
> > > > > 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@xxxxxxxxxxxxxxxx>
> > > > > ---
> > > > > .../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 , 10.1.8 in , 9.1.8 in ). 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 , 10.1.18 in , 9.1.18 in )
> and readout direction of the video memory (Section 10.1.21 of ,
> 10.1.14 in , 9.1.14 in ). 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
> . 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.
> > > 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 ).
> 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 |
> 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
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
Description: Digital signature