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

[Olliver Schinagl <oliver@schinagl.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.

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@physik.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.

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

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

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


Overal, the driver works and can have a Tested-by: Olliver Schinagl 
<o.schinagl@ultimaker.com> from me (for the 1309 which I'll submit very 
soon.

Olliver

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