Message-ID: <20150325231423.6d067023@maestro.intranet>
Date:	Wed, 25 Mar 2015 23:14:23 +0100
From:	Thomas Niederprüm <niederp@...sik.uni-kl.de>
To:	Olliver Schinagl <oliver@...inagl.nl>
Cc:	plagnioj@...osoft.com, tomi.valkeinen@...com,
	maxime.ripard@...e-electrons.com, kernel@...gutronix.de,
	shawn.guo@...aro.org, robh+dt@...nel.org,
	linux-fbdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: 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 <oliver@...inagl.nl>:

> 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:
> > 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 <niederp@...sik.uni-kl.de>
> > ---
> >   .../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?
> 
> >
> >   [0]: Documentation/devicetree/bindings/pwm/pwm.txt
> >
> <snip>
> > +
> > +	/* 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.

> 
> > +	ret = ssd1307fb_write_cmd(par->client,  com_invdir);
> >   	if (ret < 0)
> >   		return ret;
> <snip>
> >
> > -	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.

> >
> >   	/* 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>
> >
> > -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.

> > +
> > +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. 

> >
> > +	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.

> > +
> > +	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 
> <o.schinagl@...imaker.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists