From: Thomas Niederprüm <redacted>
This patch series is the result of making the ssd1307fb driver work with
a Newhaven OLED display using the Solomon SSD1305 controller. To achieve
this the intialization code for the SSD1306 and the SSD1307 is merged
and based on device tree configuration. This gets rid of the magic bit
values that were used so far.
Based on these changes it was straight forward to add support for the
SSD1305 controller.
While working with the driver I realized that it was not possible to
correctly mmap the video memory from userspace since the memory
reserved by kzalloc is not page aligned. This problem is fixed by
using vmalloc as it is done inthe vfb driver.
Furthermore module parameters are added to set the bits per pixel
and the delay for the deferred io update. It makes sense to set
the bits per pixel for the video memory to 8 bits since there is
only very poor userspace support for 1 bit framebuffers.
Also sysfs handles are added to make the contrast settings and dim
mode setting available in userspace.
Thomas Niederprüm (8):
Documentation: dts: add missing Solomon Systech vendor prefix.
fbdev: ssd1307fb: Unify init code and make controller configurable
from device tree
fbdev: ssd1307fb: Add support for SSD1305
fbdev: ssd1307fb: Use vmalloc to allocate video memory.
fbdev: ssd1307fb: Add module parameter bitsperpixel.
fbdev: ssd1307fb: Add module parameter to set update delay of the
deffered io.
fbdev: ssd1307fb: Add sysfs handles to expose contrast and dim setting
to userspace.
fbdev: ssd1307fb: Turn off display on driver unload.
.../devicetree/bindings/vendor-prefixes.txt | 1 +
.../devicetree/bindings/video/ssd1307fb.txt | 13 +-
drivers/video/fbdev/ssd1307fb.c | 426 ++++++++++++++++-----
3 files changed, 346 insertions(+), 94 deletions(-)
--
2.1.1
@@ -143,6 +143,7 @@ sitronix Sitronix Technology Corporation smsc Standard Microsystems Corporation snps Synopsys, Inc. solidrun SolidRun+solomon Solomon Systech Limited sony Sony Corporation spansion Spansion Inc. st STMicroelectronics
From: Thomas Niederprüm <redacted>
It makes sense to use vmalloc to allocate the video buffer since it has to be page aligned memory for using it with
mmap. Also deffered io seems buggy in combination with kmalloc'ed memory (crash on unloading the module).
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 43 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 41 insertions(+), 2 deletions(-)
@@ -93,6 +94,43 @@ static struct fb_var_screeninfo ssd1307fb_var = {.bits_per_pixel=1,};+staticvoid*rvmalloc(unsignedlongsize)+{+void*mem;+unsignedlongadr;++size=PAGE_ALIGN(size);+mem=vmalloc_32(size);+if(!mem)+returnNULL;++memset(mem,0,size);/* Clear the ram out, no junk to the user */+adr=(unsignedlong)mem;+while(size>0){+SetPageReserved(vmalloc_to_page((void*)adr));+adr+=PAGE_SIZE;+size-=PAGE_SIZE;+}++returnmem;+}++staticvoidrvfree(void*mem,unsignedlongsize)+{+unsignedlongadr;++if(!mem)+return;++adr=(unsignedlong)mem;+while((long)size>0){+ClearPageReserved(vmalloc_to_page((void*)adr));+adr+=PAGE_SIZE;+size-=PAGE_SIZE;+}+vfree(mem);+}+staticstructssd1307fb_array*ssd1307fb_alloc_array(u32len,u8type){structssd1307fb_array*array;
From: Thomas Niederprüm <redacted>
This patch adds sysfs handles to enable userspace control over the display
contrast as well as the dim mode. The handles are available as "contrast"
and "dim" in the framebuffers sysfs domain.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 88 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 87 insertions(+), 1 deletion(-)
@@ -523,7 +601,7 @@ static int ssd1307fb_probe(struct i2c_client *client,u32vmem_size;structssd1307fb_par*par;u8*vmem;-intret;+intret,i;if(!node){dev_err(&client->dev,"No device tree data found!\n");
@@ -650,6 +728,14 @@ static int ssd1307fb_probe(struct i2c_client *client,gotoreset_oled_error;ret=register_framebuffer(info);++for(i=0;i<ARRAY_SIZE(device_attrs);i++)+ret=device_create_file(info->dev,&device_attrs[i]);++if(ret){+dev_err(&client->dev,"Couldn't register sysfs nodes\n");+}+if(ret){dev_err(&client->dev,"Couldn't register the framebuffer\n");gotopanel_init_error;
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(-)
@@ -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 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
@@ -254,127 +270,151 @@ static struct fb_deferred_io ssd1307fb_defio = {.deferred_io=ssd1307fb_deferred_io,};-staticintssd1307fb_ssd1307_init(structssd1307fb_par*par)+staticintssd1307fb_init(structssd1307fb_par*par){intret;+u32precharge,dclk,com_invdir,compins;-par->pwm=pwm_get(&par->client->dev,NULL);-if(IS_ERR(par->pwm)){-dev_err(&par->client->dev,"Could not get PWM from device tree!\n");-returnPTR_ERR(par->pwm);-}--par->pwm_period=pwm_get_period(par->pwm);-/* Enable the PWM */-pwm_config(par->pwm,par->pwm_period/2,par->pwm_period);-pwm_enable(par->pwm);--dev_dbg(&par->client->dev,"Using PWM%d with a %dns period.\n",-par->pwm->pwm,par->pwm_period);--/* Map column 127 of the OLED to segment 0 */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SEG_REMAP_ON);-if(ret<0)-returnret;--/* Turn on the display */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_DISPLAY_ON);-if(ret<0)-returnret;--return0;-}--staticintssd1307fb_ssd1307_remove(structssd1307fb_par*par)-{-pwm_disable(par->pwm);-pwm_put(par->pwm);-return0;-}+if(par->device_info->device_id=DEVID_SSD1307){+par->pwm=pwm_get(&par->client->dev,NULL);+if(IS_ERR(par->pwm)){+dev_err(&par->client->dev,"Could not get PWM from device tree!\n");+returnPTR_ERR(par->pwm);+}-staticstructssd1307fb_opsssd1307fb_ssd1307_ops={-.init=ssd1307fb_ssd1307_init,-.remove=ssd1307fb_ssd1307_remove,-};+par->pwm_period=pwm_get_period(par->pwm);+/* Enable the PWM */+pwm_config(par->pwm,par->pwm_period/2,par->pwm_period);+pwm_enable(par->pwm);-staticintssd1307fb_ssd1306_init(structssd1307fb_par*par)-{-intret;+dev_dbg(&par->client->dev,"Using PWM%d with a %dns period.\n",+par->pwm->pwm,par->pwm_period);+};/* Set initial contrast */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_CONTRAST);-ret=ret&ssd1307fb_write_cmd(par->client,0x7f);if(ret<0)returnret;-/* Set COM direction */-ret=ssd1307fb_write_cmd(par->client,0xc8);+ret=ssd1307fb_write_cmd(par->client,par->contrast);if(ret<0)returnret;/* Set segment re-map */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SEG_REMAP_ON);+if(par->seg_remap){+ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SEG_REMAP_ON);+if(ret<0)+returnret;+};++/* Set COM direction */+com_invdir=0xc0|(par->com_invdir&0xf)<<3;+ret=ssd1307fb_write_cmd(par->client,com_invdir);if(ret<0)returnret;/* Set multiplex ratio value */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_MULTIPLEX_RATIO);-ret=ret&ssd1307fb_write_cmd(par->client,par->height-1);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,par->height-1);if(ret<0)returnret;/* set display offset value */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_DISPLAY_OFFSET);-ret=ssd1307fb_write_cmd(par->client,0x20);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,par->offset);if(ret<0)returnret;/* Set clock frequency */+dclk=(par->dclk_div&0xf)|(par->dclk_frq&0xf)<<4;ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_CLOCK_FREQ);-ret=ret&ssd1307fb_write_cmd(par->client,0xf0);if(ret<0)returnret;-/* Set precharge period in number of ticks from the internal clock */+ret=ssd1307fb_write_cmd(par->client,dclk);+if(ret<0)+returnret;++/* Set precharge period in number of ticks from the internal clock*/+precharge=(par->prechargep1&0xf)|(par->prechargep2&0xf)<<4;ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_PRECHARGE_PERIOD);-ret=ret&ssd1307fb_write_cmd(par->client,0x22);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,precharge);if(ret<0)returnret;/* Set COM pins configuration */+compins=0x02|(par->com_alt&0x1)<<4+|(par->com_lrremap&0x1)<<5;ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_COM_PINS_CONFIG);-ret=ret&ssd1307fb_write_cmd(par->client,0x22);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,compins);if(ret<0)returnret;/* Set VCOMH */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_VCOMH);-ret=ret&ssd1307fb_write_cmd(par->client,0x49);if(ret<0)returnret;-/* Turn on the DC-DC Charge Pump */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_CHARGE_PUMP);-ret=ret&ssd1307fb_write_cmd(par->client,0x14);+ret=ssd1307fb_write_cmd(par->client,par->vcomh);if(ret<0)returnret;+if(par->device_info->device_id=DEVID_SSD1306){+/* Turn on the DC-DC Charge Pump */+ret=ssd1307fb_write_cmd(par->client,SSD1307FB_CHARGE_PUMP);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,0x14);+if(ret<0)+returnret;+};+/* Switch to horizontal addressing mode */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_ADDRESS_MODE);-ret=ret&ssd1307fb_write_cmd(par->client,+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_ADDRESS_MODE_HORIZONTAL);if(ret<0)returnret;+/* Set column range */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_COL_RANGE);-ret=ret&ssd1307fb_write_cmd(par->client,0x0);-ret=ret&ssd1307fb_write_cmd(par->client,par->width-1);if(ret<0)returnret;+ret=ssd1307fb_write_cmd(par->client,0x0);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,par->width-1);+if(ret<0)+returnret;++/* Set page range */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_PAGE_RANGE);-ret=ret&ssd1307fb_write_cmd(par->client,0x0);-ret=ret&ssd1307fb_write_cmd(par->client,+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,0x0);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,par->page_offset+(par->height/8)-1);if(ret<0)returnret;
@@ -387,18 +427,28 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)return0;}-staticstructssd1307fb_opsssd1307fb_ssd1306_ops={-.init=ssd1307fb_ssd1306_init,+staticstructssd1307fb_deviceinfossd1307fb_ssd1306_deviceinfo={+.device_id=DEVID_SSD1306,+.default_vcomh=0x20,+.default_dclk_div=0,+.default_dclk_frq=8,+};++staticstructssd1307fb_deviceinfossd1307fb_ssd1307_deviceinfo={+.device_id=DEVID_SSD1307,+.default_vcomh=0x20,+.default_dclk_div=1,+.default_dclk_frq=12,};staticconststructof_device_idssd1307fb_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,},{},};
@@ -429,8 +479,8 @@ static int ssd1307fb_probe(struct i2c_client *client,par->info=info;par->client=client;-par->ops=(structssd1307fb_ops*)of_match_device(ssd1307fb_of_match,-&client->dev)->data;+par->device_info=(structssd1307fb_deviceinfo*)of_match_device(+ssd1307fb_of_match,&client->dev)->data;par->reset=of_get_named_gpio(client->dev.of_node,"reset-gpios",0);
From: Thomas Niederprüm <redacted>
This patch adds a module parameter 'bitsperpixel' to adjust the colordepth
of the framebuffer. All values >1 will result in memory map of the requested
color depth. However only the MSB of each pixel will be sent to the device.
The framebuffer identifies itself as a grayscale display with the specified
depth.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
@@ -2,7 +2,7 @@ Required properties: - compatible: Should be "solomon,<chip>fb-<bus>". The only supported bus for- now is i2c, and the supported chips are ssd1306 and ssd1307.+ now is i2c, and the supported chips are ssd1305, ssd1306 and ssd1307. - reg: Should contain address of the controller on the I2C bus. Most likely 0x3c or 0x3d - pwm: Should contain the pwm to use according to the OF device tree PWM
From: Thomas Niederprüm <redacted>
This patch adds the module parameter "delaydivider" to set delay for the
deferred io. Effectively this is setting the refresh rate for mmap access
to the framebuffer. The delay for the deferred io is HZ/delaydivider.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
From: Maxime Ripard <hidden> Date: 2015-02-07 10:45:08
Hi,
On Fri, Feb 06, 2015 at 11:28:08PM +0100, niederp@physik.uni-kl.de wrote:
quoted hunk
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(-)
@@ -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,
and the DT itself should not contain any direct mapping of the
registers.
@@ -254,127 +270,151 @@ static struct fb_deferred_io ssd1307fb_defio = {.deferred_io=ssd1307fb_deferred_io,};-staticintssd1307fb_ssd1307_init(structssd1307fb_par*par)+staticintssd1307fb_init(structssd1307fb_par*par){intret;+u32precharge,dclk,com_invdir,compins;-par->pwm=pwm_get(&par->client->dev,NULL);-if(IS_ERR(par->pwm)){-dev_err(&par->client->dev,"Could not get PWM from device tree!\n");-returnPTR_ERR(par->pwm);-}--par->pwm_period=pwm_get_period(par->pwm);-/* Enable the PWM */-pwm_config(par->pwm,par->pwm_period/2,par->pwm_period);-pwm_enable(par->pwm);--dev_dbg(&par->client->dev,"Using PWM%d with a %dns period.\n",-par->pwm->pwm,par->pwm_period);--/* Map column 127 of the OLED to segment 0 */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SEG_REMAP_ON);-if(ret<0)-returnret;--/* Turn on the display */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_DISPLAY_ON);-if(ret<0)-returnret;--return0;-}--staticintssd1307fb_ssd1307_remove(structssd1307fb_par*par)-{-pwm_disable(par->pwm);-pwm_put(par->pwm);-return0;-}+if(par->device_info->device_id==DEVID_SSD1307){+par->pwm=pwm_get(&par->client->dev,NULL);+if(IS_ERR(par->pwm)){+dev_err(&par->client->dev,"Could not get PWM from device tree!\n");+returnPTR_ERR(par->pwm);+}-staticstructssd1307fb_opsssd1307fb_ssd1307_ops={-.init=ssd1307fb_ssd1307_init,-.remove=ssd1307fb_ssd1307_remove,-};+par->pwm_period=pwm_get_period(par->pwm);+/* Enable the PWM */+pwm_config(par->pwm,par->pwm_period/2,par->pwm_period);+pwm_enable(par->pwm);-staticintssd1307fb_ssd1306_init(structssd1307fb_par*par)-{-intret;+dev_dbg(&par->client->dev,"Using PWM%d with a %dns period.\n",+par->pwm->pwm,par->pwm_period);+};/* Set initial contrast */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_CONTRAST);-ret=ret&ssd1307fb_write_cmd(par->client,0x7f);if(ret<0)returnret;-/* Set COM direction */-ret=ssd1307fb_write_cmd(par->client,0xc8);+ret=ssd1307fb_write_cmd(par->client,par->contrast);if(ret<0)returnret;/* Set segment re-map */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SEG_REMAP_ON);+if(par->seg_remap){+ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SEG_REMAP_ON);+if(ret<0)+returnret;+};++/* Set COM direction */+com_invdir=0xc0|(par->com_invdir&0xf)<<3;+ret=ssd1307fb_write_cmd(par->client,com_invdir);if(ret<0)returnret;/* Set multiplex ratio value */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_MULTIPLEX_RATIO);-ret=ret&ssd1307fb_write_cmd(par->client,par->height-1);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,par->height-1);if(ret<0)returnret;/* set display offset value */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_DISPLAY_OFFSET);-ret=ssd1307fb_write_cmd(par->client,0x20);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,par->offset);if(ret<0)returnret;/* Set clock frequency */+dclk=(par->dclk_div&0xf)|(par->dclk_frq&0xf)<<4;ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_CLOCK_FREQ);-ret=ret&ssd1307fb_write_cmd(par->client,0xf0);if(ret<0)returnret;-/* Set precharge period in number of ticks from the internal clock */+ret=ssd1307fb_write_cmd(par->client,dclk);+if(ret<0)+returnret;++/* Set precharge period in number of ticks from the internal clock*/+precharge=(par->prechargep1&0xf)|(par->prechargep2&0xf)<<4;ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_PRECHARGE_PERIOD);-ret=ret&ssd1307fb_write_cmd(par->client,0x22);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,precharge);if(ret<0)returnret;/* Set COM pins configuration */+compins=0x02|(par->com_alt&0x1)<<4+|(par->com_lrremap&0x1)<<5;ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_COM_PINS_CONFIG);-ret=ret&ssd1307fb_write_cmd(par->client,0x22);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,compins);if(ret<0)returnret;/* Set VCOMH */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_VCOMH);-ret=ret&ssd1307fb_write_cmd(par->client,0x49);if(ret<0)returnret;-/* Turn on the DC-DC Charge Pump */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_CHARGE_PUMP);-ret=ret&ssd1307fb_write_cmd(par->client,0x14);+ret=ssd1307fb_write_cmd(par->client,par->vcomh);if(ret<0)returnret;+if(par->device_info->device_id==DEVID_SSD1306){+/* Turn on the DC-DC Charge Pump */+ret=ssd1307fb_write_cmd(par->client,SSD1307FB_CHARGE_PUMP);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,0x14);+if(ret<0)+returnret;+};+/* Switch to horizontal addressing mode */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_ADDRESS_MODE);-ret=ret&ssd1307fb_write_cmd(par->client,+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_ADDRESS_MODE_HORIZONTAL);if(ret<0)returnret;+/* Set column range */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_COL_RANGE);-ret=ret&ssd1307fb_write_cmd(par->client,0x0);-ret=ret&ssd1307fb_write_cmd(par->client,par->width-1);if(ret<0)returnret;+ret=ssd1307fb_write_cmd(par->client,0x0);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,par->width-1);+if(ret<0)+returnret;++/* Set page range */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_PAGE_RANGE);-ret=ret&ssd1307fb_write_cmd(par->client,0x0);-ret=ret&ssd1307fb_write_cmd(par->client,+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,0x0);+if(ret<0)+returnret;++ret=ssd1307fb_write_cmd(par->client,par->page_offset+(par->height/8)-1);if(ret<0)returnret;
@@ -387,18 +427,28 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)return0;}-staticstructssd1307fb_opsssd1307fb_ssd1306_ops={-.init=ssd1307fb_ssd1306_init,+staticstructssd1307fb_deviceinfossd1307fb_ssd1306_deviceinfo={+.device_id=DEVID_SSD1306,+.default_vcomh=0x20,+.default_dclk_div=0,+.default_dclk_frq=8,+};++staticstructssd1307fb_deviceinfossd1307fb_ssd1307_deviceinfo={+.device_id=DEVID_SSD1307,+.default_vcomh=0x20,+.default_dclk_div=1,+.default_dclk_frq=12,};staticconststructof_device_idssd1307fb_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,},{},};
@@ -429,8 +479,8 @@ static int ssd1307fb_probe(struct i2c_client *client,par->info=info;par->client=client;-par->ops=(structssd1307fb_ops*)of_match_device(ssd1307fb_of_match,-&client->dev)->data;+par->device_info=(structssd1307fb_deviceinfo*)of_match_device(+ssd1307fb_of_match,&client->dev)->data;par->reset=of_get_named_gpio(client->dev.of_node,"reset-gpios",0);
From: Maxime Ripard <hidden> Date: 2015-02-07 11:20:18
Hi,
On Fri, Feb 06, 2015 at 11:28:10PM +0100, niederp@physik.uni-kl.de wrote:
From: Thomas Niederprüm <redacted>
It makes sense to use vmalloc to allocate the video buffer since it
has to be page aligned memory for using it with mmap.
Please wrap your commit log at 80 chars.
It looks like there's numerous fbdev drivers using this (especially
since you copy pasted that code, without mentionning it).
That should be turned into an allocator so that drivers all get this
right.
Also deffered io seems buggy in combination with kmalloc'ed memory
(crash on unloading the module).
+ if (!mem)
+ return NULL;
+
+ memset(mem, 0, size); /* Clear the ram out, no junk to the user */
+ adr = (unsigned long) mem;
+ while (size > 0) {
+ SetPageReserved(vmalloc_to_page((void *)adr));
I'm not really sure it makes sense to mark all pages reserved if we're
not even sure we're going to use mmap.
And why do you need to mark these pages reserved in the first place?
Why are you changing from virtual to physical address here?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
From: Maxime Ripard <hidden> Date: 2015-02-07 11:25:15
On Fri, Feb 06, 2015 at 11:28:11PM +0100, niederp@physik.uni-kl.de wrote:
From: Thomas Niederprüm <redacted>
This patch adds a module parameter 'bitsperpixel' to adjust the colordepth
of the framebuffer. All values >1 will result in memory map of the requested
color depth. However only the MSB of each pixel will be sent to the device.
The framebuffer identifies itself as a grayscale display with the specified
depth.
I'm not sure this is the right thing to do.
The bits per pixel for this display is rightfully defined, used and
reported to the userspace, why would you want to change that?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
From: Maxime Ripard <hidden> Date: 2015-02-07 11:30:20
On Fri, Feb 06, 2015 at 11:28:12PM +0100, niederp@physik.uni-kl.de wrote:
From: Thomas Niederprüm <redacted>
This patch adds the module parameter "delaydivider" to set delay for the
deferred io. Effectively this is setting the refresh rate for mmap access
to the framebuffer. The delay for the deferred io is HZ/delaydivider.
So this is actually a refresh rate?
Maybe you could expose it as such, and pass a frequency in Hz as an
argument.
Exposing the divider directly has some issues, since the bootloader
that set the parameter won't know the HZ value, you'll end up with
different rates for different configurations, without any way to do
something about it.
From: Maxime Ripard <hidden> Date: 2015-02-07 11:45:20
On Fri, Feb 06, 2015 at 11:28:13PM +0100, niederp@physik.uni-kl.de wrote:
quoted hunk
From: Thomas Niederprüm <redacted>
This patch adds sysfs handles to enable userspace control over the display
contrast as well as the dim mode. The handles are available as "contrast"
and "dim" in the framebuffers sysfs domain.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 88 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 87 insertions(+), 1 deletion(-)
I would have thought this was something accessible through the
framebuffer ioctl.
Apparently it's not, at least for the contrast, so maybe it should be
added there, instead of doing it for a single driver?
(oh, and btw, every sysfs file should be documented in
Documentation/ABI)
quoted hunk
static int ssd1307fb_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -523,7 +601,7 @@ static int ssd1307fb_probe(struct i2c_client *client, u32 vmem_size; struct ssd1307fb_par *par; u8 *vmem;- int ret;+ int ret, i; if (!node) { dev_err(&client->dev, "No device tree data found!\n");
@@ -650,6 +728,14 @@ static int ssd1307fb_probe(struct i2c_client *client, goto reset_oled_error; ret = register_framebuffer(info);++ for (i = 0; i < ARRAY_SIZE(device_attrs); i++)+ ret = device_create_file(info->dev, &device_attrs[i]);++ if (ret) {+ dev_err(&client->dev, "Couldn't register sysfs nodes\n");+ }+
sysfs_create_groups does pretty much that already.
And don't forget to remove these files in the .remove()
if (ret) {
dev_err(&client->dev, "Couldn't register the framebuffer\n");
goto panel_init_error;
--
2.1.1
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
@@ -762,6 +762,11 @@ static int ssd1307fb_remove(struct i2c_client *client){structfb_info*info=i2c_get_clientdata(client);structssd1307fb_par*par=info->par;+intret=0;++ret=ssd1307fb_write_cmd(par->client,SSD1307FB_DISPLAY_OFF);+if(ret<0)+returnret;
I don't think we really care about the return value here.
It might be even worse actually, since you'll end up in a intermediate
state, where you won't have freed everything, but your remove method
has been called still.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
From: Thomas Niederprüm <hidden> Date: 2015-02-07 15:00:15
Am Sat, 7 Feb 2015 11:42:25 +0100
schrieb Maxime Ripard [off-list ref]:
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(-)
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. 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.
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?
Hi,
Den 07.02.2015 15:59, skrev Thomas Niederprüm:
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(-)
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. 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.
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?
I have the exact same challenge with the staging/fbtft drivers.
I have asked about this on the DT list a couple of days ago, but no
answer yet:
Can I do register initialization from Device Tree?
http://www.spinics.net/lists/devicetree/msg68174.html
Regards,
Noralf Trønnes
From: Thomas Niederprüm <hidden> Date: 2015-02-07 15:33:09
Am Sat, 7 Feb 2015 12:18:21 +0100
schrieb Maxime Ripard [off-list ref]:
Hi,
On Fri, Feb 06, 2015 at 11:28:10PM +0100, niederp@physik.uni-kl.de
wrote:
quoted
From: Thomas Niederprüm <redacted>
It makes sense to use vmalloc to allocate the video buffer since it
has to be page aligned memory for using it with mmap.
Please wrap your commit log at 80 chars.
I'll try to do so in future, sorry for that.
It looks like there's numerous fbdev drivers using this (especially
since you copy pasted that code, without mentionning it).
Yes, I should have mentioned that in the commit message. As
implicitly indicated in the cover letter the rvmalloc() and rvfree() are
copy pasted from the vfb driver. Honestly, I didn't give this one too
much thought. It seemed a viable solution to the mmap problem. For a
bit more history on that, see my comment below.
That should be turned into an allocator so that drivers all get this
right.
quoted
Also deffered io seems buggy in combination with kmalloc'ed memory
(crash on unloading the module).
+ if (!mem)
+ return NULL;
+
+ memset(mem, 0, size); /* Clear the ram out, no junk to the
user */
+ adr = (unsigned long) mem;
+ while (size > 0) {
+ SetPageReserved(vmalloc_to_page((void *)adr));
I'm not really sure it makes sense to mark all pages reserved if we're
not even sure we're going to use mmap.
And why do you need to mark these pages reserved in the first place?
Why are you changing from virtual to physical address here?
Oh, good catch! This is still a residual of my attempts to get this
working with kmalloc'ed memory. In the current state the driver is
presenting a completely wrong memory address upon mmap. As reported in
[0] info->fix.smem_start has to hold the physical address of the video
memory if it was allocated using kmalloc. Correcting this let me run
into the problem that the kmalloc'ed memory was not page aligned but,
the memory address handed to userspace mmap was aligned to the next
full page, resulting in an inaccessable display region. At that point I
just copied the vmalloc approach from the vfb driver.
[0] http://stackoverflow.com/questions/22285151/kernel-panic-using-deferred-io-on-kmalloced-buffer
From: Thomas Niederprüm <hidden> Date: 2015-02-07 16:02:31
Am Sat, 7 Feb 2015 12:20:43 +0100
schrieb Maxime Ripard [off-list ref]:
On Fri, Feb 06, 2015 at 11:28:11PM +0100, niederp@physik.uni-kl.de
wrote:
quoted
From: Thomas Niederprüm <redacted>
This patch adds a module parameter 'bitsperpixel' to adjust the
colordepth of the framebuffer. All values >1 will result in memory
map of the requested color depth. However only the MSB of each
pixel will be sent to the device. The framebuffer identifies itself
as a grayscale display with the specified depth.
I'm not sure this is the right thing to do.
The bits per pixel for this display is rightfully defined, used and
reported to the userspace, why would you want to change that?
You are right of course. The display is 1bpp and it reports to be 1
bpp. The problem is that there is almost no userspace library that can
handle 1 bit framebuffers correctly. So it is nice if the framebuffer
(optionally) can expose itself as 8 bits per pixel grayscale to the
userspace program. As an example this allows to run DirectFB on the
framebuffer, which is not possible out of the box for 1bpp.
Also note that if do not set the module parameter at load time
the framebuffer will be 1bpp. So you have to actively set that module
parameter to make the framebuffer pretend to be more than 1bpp.
In any case I don't cling to that patch, I just thought it was a nice
feature.
From: Thomas Niederprüm <hidden> Date: 2015-02-07 16:09:39
Am Sat, 7 Feb 2015 12:26:27 +0100
schrieb Maxime Ripard [off-list ref]:
On Fri, Feb 06, 2015 at 11:28:12PM +0100, niederp@physik.uni-kl.de
wrote:
quoted
From: Thomas Niederprüm <redacted>
This patch adds the module parameter "delaydivider" to set delay
for the deferred io. Effectively this is setting the refresh rate
for mmap access to the framebuffer. The delay for the deferred io
is HZ/delaydivider.
So this is actually a refresh rate?
Maybe you could expose it as such, and pass a frequency in Hz as an
argument.
Good idea! I'll try to do it that way.
Exposing the divider directly has some issues, since the bootloader
that set the parameter won't know the HZ value, you'll end up with
different rates for different configurations, without any way to do
something about it.
From: Thomas Niederprüm <hidden> Date: 2015-02-07 16:40:12
Am Sat, 7 Feb 2015 12:43:29 +0100
schrieb Maxime Ripard [off-list ref]:
On Fri, Feb 06, 2015 at 11:28:13PM +0100, niederp@physik.uni-kl.de
wrote:
quoted
From: Thomas Niederprüm <redacted>
This patch adds sysfs handles to enable userspace control over the
display contrast as well as the dim mode. The handles are available
as "contrast" and "dim" in the framebuffers sysfs domain.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 88
++++++++++++++++++++++++++++++++++++++++- 1 file changed, 87
insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/ssd1307fb.c
b/drivers/video/fbdev/ssd1307fb.c index b38315d..02931c7 100644
I would have thought this was something accessible through the
framebuffer ioctl.
Apparently it's not, at least for the contrast, so maybe it should be
added there, instead of doing it for a single driver?
I think the contrast setting for an OLED display is much like the
backlight setting for LCD panel. Since there is also no ioctl to set
the backlight of an LCD I wonder if the contrast of an OLED should have
one.
(oh, and btw, every sysfs file should be documented in
Documentation/ABI)
quoted
static int ssd1307fb_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -523,7 +601,7 @@ static int ssd1307fb_probe(struct i2c_client
*client, u32 vmem_size;
struct ssd1307fb_par *par;
u8 *vmem;
- int ret;
+ int ret, i;
if (!node) {
dev_err(&client->dev, "No device tree data
found!\n"); @@ -650,6 +728,14 @@ static int ssd1307fb_probe(struct
i2c_client *client, goto reset_oled_error;
ret = register_framebuffer(info);
+
+ for (i = 0; i < ARRAY_SIZE(device_attrs); i++)
+ ret = device_create_file(info->dev,
&device_attrs[i]); +
+ if (ret) {
+ dev_err(&client->dev, "Couldn't register sysfs
nodes\n");
+ }
+
sysfs_create_groups does pretty much that already.
I'll have a look at it.
And don't forget to remove these files in the .remove()
Good point! :)
quoted
if (ret) {
dev_err(&client->dev, "Couldn't register the
framebuffer\n"); goto panel_init_error;
--
2.1.1
@@ -762,6 +762,11 @@ static int ssd1307fb_remove(struct i2c_client
*client) {
struct fb_info *info = i2c_get_clientdata(client);
struct ssd1307fb_par *par = info->par;
+ int ret = 0;
+
+ ret = ssd1307fb_write_cmd(par->client,
SSD1307FB_DISPLAY_OFF);
+ if (ret < 0)
+ return ret;
I don't think we really care about the return value here.
It might be even worse actually, since you'll end up in a intermediate
state, where you won't have freed everything, but your remove method
has been called still.
I agree. I will remove the check for the return statement.
I would have thought this was something accessible through the
framebuffer ioctl.
Apparently it's not, at least for the contrast, so maybe it should be
added there, instead of doing it for a single driver?
I think the contrast setting for an OLED display is much like the
backlight setting for LCD panel. Since there is also no ioctl to set
the backlight of an LCD I wonder if the contrast of an OLED should have
one.
It's too much of framebuffer interface debate for me here. Tomi?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
From: Maxime Ripard <hidden> Date: 2015-02-09 09:06:16
On Sat, Feb 07, 2015 at 05:12:11PM +0100, Thomas Niederprüm wrote:
Am Sat, 7 Feb 2015 12:26:27 +0100
schrieb Maxime Ripard [off-list ref]:
quoted
On Fri, Feb 06, 2015 at 11:28:12PM +0100, niederp@physik.uni-kl.de
wrote:
quoted
From: Thomas Niederprüm <redacted>
This patch adds the module parameter "delaydivider" to set delay
for the deferred io. Effectively this is setting the refresh rate
for mmap access to the framebuffer. The delay for the deferred io
is HZ/delaydivider.
So this is actually a refresh rate?
Maybe you could expose it as such, and pass a frequency in Hz as an
argument.
Good idea! I'll try to do it that way.
quoted
Exposing the divider directly has some issues, since the bootloader
that set the parameter won't know the HZ value, you'll end up with
different rates for different configurations, without any way to do
something about it.
That won't work with multiple instances of the same driver
unfortunately.
Could you please elaborate why? I'm not seeing it...
On a general basis, because the structure is shared by all the
instances of the driver, so it's usually not such a good idea to mix
the static declaration of the structure and the dynamic one.
From a more fundamental point of view, because this parameter will
most likely be different from one instance to another?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
From: Thomas Niederprüm <hidden> Date: 2015-02-09 10:40:13
Am Samstag, den 07.02.2015, 16:19 +0100 schrieb Noralf Trønnes:
Hi,
Den 07.02.2015 15:59, skrev Thomas Niederprüm:
quoted
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(-)
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. 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.
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?
I have the exact same challenge with the staging/fbtft drivers.
Even though we are facing the same problem here, yours is much harder
than mine, since I have much more knowledge about the controller.
Therefor I have no need to completely expose the registers for
initialization. I just define all configurable options that the
controller has in DT and let the driver take care to write the
corresponding register values during initialization. Of course this
needs the exact knowledge of the configuration options of the controller
as well as register addresses of these options in the driver. So I have
the fear that this approach does not scale for a driver handling
different controllers.
From: Maxime Ripard <hidden> Date: 2015-02-12 15:15:07
On Sat, Feb 07, 2015 at 04:35:41PM +0100, Thomas Niederprüm wrote:
Am Sat, 7 Feb 2015 12:18:21 +0100
schrieb Maxime Ripard [off-list ref]:
quoted
Hi,
On Fri, Feb 06, 2015 at 11:28:10PM +0100, niederp@physik.uni-kl.de
wrote:
quoted
From: Thomas Niederprüm <redacted>
It makes sense to use vmalloc to allocate the video buffer since it
has to be page aligned memory for using it with mmap.
Please wrap your commit log at 80 chars.
I'll try to do so in future, sorry for that.
quoted
It looks like there's numerous fbdev drivers using this (especially
since you copy pasted that code, without mentionning it).
Yes, I should have mentioned that in the commit message. As
implicitly indicated in the cover letter the rvmalloc() and rvfree() are
copy pasted from the vfb driver. Honestly, I didn't give this one too
much thought. It seemed a viable solution to the mmap problem. For a
bit more history on that, see my comment below.
quoted
That should be turned into an allocator so that drivers all get this
right.
quoted
Also deffered io seems buggy in combination with kmalloc'ed memory
(crash on unloading the module).
And maybe that's the real issue to fix.
The problem is solved by using vmalloc ;)
Yep, but why do you need to mark the reserved pages?
...
quoted
quoted
@@ -570,7 +608,7 @@ static int ssd1307fb_probe(struct i2c_client
Why are you changing from virtual to physical address here?
Oh, good catch! This is still a residual of my attempts to get this
working with kmalloc'ed memory. In the current state the driver is
presenting a completely wrong memory address upon mmap. As reported in
[0] info->fix.smem_start has to hold the physical address of the video
memory if it was allocated using kmalloc. Correcting this let me run
into the problem that the kmalloc'ed memory was not page aligned but,
the memory address handed to userspace mmap was aligned to the next
full page, resulting in an inaccessable display region. At that point I
just copied the vmalloc approach from the vfb driver.
[0] http://stackoverflow.com/questions/22285151/kernel-panic-using-deferred-io-on-kmalloced-buffer
From: Maxime Ripard <hidden> Date: 2015-02-12 16:45:20
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(-)
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
From: Maxime Ripard <hidden> Date: 2015-02-12 17:00:22
On Sat, Feb 07, 2015 at 04:19:37PM +0100, Noralf Trønnes wrote:
quoted
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?
I have the exact same challenge with the staging/fbtft drivers.
I have asked about this on the DT list a couple of days ago, but no answer
yet:
Can I do register initialization from Device Tree?
http://www.spinics.net/lists/devicetree/msg68174.html
The DT is just an hardware representation, and should not contain any
logic. Any attempts at doing so have been failures, mostly because
that kind of construct are really fragile.
What would happen if at some point some new controller pops up and
need to poke a GPIO before every write?
Every driver should contain all the needed code to initialise properly
its hardware. The only exception being stuff that are volatile by
essence, like bus addresses, GPIOs, etc.
In your case, using (and maybe extending) the generic panel bindings
look like a better way forward.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
From: Thomas Niederprüm <hidden> Date: 2015-02-14 14:19:31
Am Thu, 12 Feb 2015 16:11:21 +0100
schrieb Maxime Ripard [off-list ref]:
On Sat, Feb 07, 2015 at 04:35:41PM +0100, Thomas Niederprüm wrote:
quoted
Am Sat, 7 Feb 2015 12:18:21 +0100
schrieb Maxime Ripard [off-list ref]:
quoted
Hi,
On Fri, Feb 06, 2015 at 11:28:10PM +0100, niederp@physik.uni-kl.de
wrote:
quoted
From: Thomas Niederprüm <redacted>
It makes sense to use vmalloc to allocate the video buffer
since it has to be page aligned memory for using it with mmap.
Please wrap your commit log at 80 chars.
I'll try to do so in future, sorry for that.
quoted
It looks like there's numerous fbdev drivers using this
(especially since you copy pasted that code, without mentionning
it).
Yes, I should have mentioned that in the commit message. As
implicitly indicated in the cover letter the rvmalloc() and
rvfree() are copy pasted from the vfb driver. Honestly, I didn't
give this one too much thought. It seemed a viable solution to the
mmap problem. For a bit more history on that, see my comment below.
quoted
That should be turned into an allocator so that drivers all get
this right.
quoted
Also deffered io seems buggy in combination with kmalloc'ed
memory (crash on unloading the module).
And maybe that's the real issue to fix.
The problem is solved by using vmalloc ;)
Yep, but why do you need to mark the reserved pages?
...
Why are you changing from virtual to physical address here?
Oh, good catch! This is still a residual of my attempts to get this
working with kmalloc'ed memory. In the current state the driver is
presenting a completely wrong memory address upon mmap. As reported
in [0] info->fix.smem_start has to hold the physical address of the
video memory if it was allocated using kmalloc. Correcting this let
me run into the problem that the kmalloc'ed memory was not page
aligned but, the memory address handed to userspace mmap was
aligned to the next full page, resulting in an inaccessable display
region. At that point I just copied the vmalloc approach from the
vfb driver.
[0]
http://stackoverflow.com/questions/22285151/kernel-panic-using-deferred-io-on-kmalloced-buffer
From: Maxime Ripard <hidden> Date: 2015-02-14 15:40:10
On Sat, Feb 14, 2015 at 03:22:12PM +0100, Thomas Niederprüm wrote:
Am Thu, 12 Feb 2015 16:11:21 +0100
schrieb Maxime Ripard [off-list ref]:
quoted
On Sat, Feb 07, 2015 at 04:35:41PM +0100, Thomas Niederprüm wrote:
quoted
Am Sat, 7 Feb 2015 12:18:21 +0100
schrieb Maxime Ripard [off-list ref]:
quoted
Hi,
On Fri, Feb 06, 2015 at 11:28:10PM +0100, niederp@physik.uni-kl.de
wrote:
quoted
From: Thomas Niederprüm <redacted>
It makes sense to use vmalloc to allocate the video buffer
since it has to be page aligned memory for using it with mmap.
Please wrap your commit log at 80 chars.
I'll try to do so in future, sorry for that.
quoted
It looks like there's numerous fbdev drivers using this
(especially since you copy pasted that code, without mentionning
it).
Yes, I should have mentioned that in the commit message. As
implicitly indicated in the cover letter the rvmalloc() and
rvfree() are copy pasted from the vfb driver. Honestly, I didn't
give this one too much thought. It seemed a viable solution to the
mmap problem. For a bit more history on that, see my comment below.
quoted
That should be turned into an allocator so that drivers all get
this right.
quoted
Also deffered io seems buggy in combination with kmalloc'ed
memory (crash on unloading the module).
And maybe that's the real issue to fix.
The problem is solved by using vmalloc ;)
Yep, but why do you need to mark the reserved pages?
...
From: Maxime Ripard <hidden> Date: 2015-02-14 15:55:06
On Sat, Feb 07, 2015 at 05:05:03PM +0100, Thomas Niederprüm wrote:
Am Sat, 7 Feb 2015 12:20:43 +0100
schrieb Maxime Ripard [off-list ref]:
quoted
On Fri, Feb 06, 2015 at 11:28:11PM +0100, niederp@physik.uni-kl.de
wrote:
quoted
From: Thomas Niederprüm <redacted>
This patch adds a module parameter 'bitsperpixel' to adjust the
colordepth of the framebuffer. All values >1 will result in memory
map of the requested color depth. However only the MSB of each
pixel will be sent to the device. The framebuffer identifies itself
as a grayscale display with the specified depth.
I'm not sure this is the right thing to do.
The bits per pixel for this display is rightfully defined, used and
reported to the userspace, why would you want to change that?
You are right of course. The display is 1bpp and it reports to be 1
bpp. The problem is that there is almost no userspace library that can
handle 1 bit framebuffers correctly. So it is nice if the framebuffer
(optionally) can expose itself as 8 bits per pixel grayscale to the
userspace program. As an example this allows to run DirectFB on the
framebuffer, which is not possible out of the box for 1bpp.
Also note that if do not set the module parameter at load time
the framebuffer will be 1bpp. So you have to actively set that module
parameter to make the framebuffer pretend to be more than 1bpp.
In any case I don't cling to that patch, I just thought it was a nice
feature.
I'd say that the right fix would be to patch DirectFB, instead of
faking that in the kernel.
But again, that's probably Tomi's call, not mine.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
From: Thomas Niederprüm <hidden> Date: 2015-02-14 16:09:51
Am Thu, 12 Feb 2015 17:41:47 +0100
schrieb Maxime Ripard [off-list ref]:
On Sat, Feb 07, 2015 at 03:59:33PM +0100, Thomas Niederprüm wrote:
quoted
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?
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.
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 :/
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.
quoted
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.
quoted
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.
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.
From: Maxime Ripard <hidden> Date: 2015-02-23 09:45:07
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 [off-list ref]:
quoted
On Sat, Feb 07, 2015 at 03:59:33PM +0100, Thomas Niederprüm wrote:
quoted
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?
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.
quoted
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.
quoted
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.
quoted
quoted
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).
quoted
quoted
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.
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
From: Thomas Niederprüm <hidden> Date: 2015-03-01 22:28:37
It makes sense to use vmalloc to allocate the video buffer since it has to be
page aligned memory for using it with mmap. Also deffered io seems buggy in
combination with kmalloc'ed memory (crash on unloading the module).
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
@@ -2,7 +2,7 @@ Required properties: - compatible: Should be "solomon,<chip>fb-<bus>". The only supported bus for- now is i2c, and the supported chips are ssd1306 and ssd1307.+ now is i2c, and the supported chips are ssd1305, ssd1306 and ssd1307. - reg: Should contain address of the controller on the I2C bus. Most likely 0x3c or 0x3d - pwm: Should contain the pwm to use according to the OF device tree PWM
From: Thomas Niederprüm <hidden> Date: 2015-03-01 22:28:45
This patch adds the module parameter "refreshrate" to set delay for the
deferred io. The refresh rate is given in units of Hertz. The default
refresh rate is 1 Hz.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
From: Thomas Niederprüm <hidden> Date: 2015-03-01 22:28:48
introducing the new DT properties the in tree users of the SSD1306
controller are updated to be up to date.
Signed-off-by: Thomas Niederprüm <redacted>
---
arch/arm/boot/dts/imx28-cfa10036.dts | 4 ++++
1 file changed, 4 insertions(+)
From: Thomas Niederprüm <hidden> Date: 2015-03-01 22:28:51
This patch adds a module parameter 'bitsperpixel' to adjust the colordepth
of the framebuffer. All values >1 will result in memory map of the requested
color depth. However only the MSB of each pixel will be sent to the device.
The framebuffer identifies itself as a grayscale display with the specified
depth.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
From: Thomas Niederprüm <hidden> Date: 2015-03-01 22:28:54
This patch turns off the display when the driver is unloaded.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 2 ++
1 file changed, 2 insertions(+)
From: Thomas Niederprüm <hidden> Date: 2015-03-01 22:29:43
This patch adds sysfs handles to enable userspace control over the display
contrast as well as the dim mode. The handles are available as "contrast"
and "dim" in the brightness group of the framebuffers sysfs domain.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 90 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 90 insertions(+)
From: Thomas Niederprüm <hidden> Date: 2015-03-01 22:30:15
The SSD130X 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-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 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 the values are not set in DT the default values
according to the controllers datasheet are assumed. This implies that this
patch changes the existing behaviour with respect to the segment remap for
the SSD1307 when the corresponding property is not present in DT. The example
in the DT bindings documentation is updated to reflect this change.
Note that the SSD1306 does not seem to be using the configuration written to
the registers at all. Nevertheless an example is added to the DT bindings
documentation that would lead to the same configuration as the current init
code.
Signed-off-by: Thomas Niederprüm <redacted>
---
.../devicetree/bindings/video/ssd1307fb.txt | 22 +++
drivers/video/fbdev/ssd1307fb.c | 195 ++++++++++++---------
2 files changed, 138 insertions(+), 79 deletions(-)
@@ -15,6 +15,15 @@ Required properties: Optional properties: - reset-active-low: Is the reset gpio is active on physical low?+ - solomon,segment-remap: Display needs 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: Offset of the first COM pin wired to the panel+ - 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 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
@@ -255,69 +274,46 @@ static struct fb_deferred_io ssd1307fb_defio = {.deferred_io=ssd1307fb_deferred_io,};-staticintssd1307fb_ssd1307_init(structssd1307fb_par*par)+staticintssd1307fb_init(structssd1307fb_par*par){intret;+u32precharge,dclk,com_invdir,compins;-par->pwm=pwm_get(&par->client->dev,NULL);-if(IS_ERR(par->pwm)){-dev_err(&par->client->dev,"Could not get PWM from device tree!\n");-returnPTR_ERR(par->pwm);-}--par->pwm_period=pwm_get_period(par->pwm);-/* Enable the PWM */-pwm_config(par->pwm,par->pwm_period/2,par->pwm_period);-pwm_enable(par->pwm);--dev_dbg(&par->client->dev,"Using PWM%d with a %dns period.\n",-par->pwm->pwm,par->pwm_period);--/* Map column 127 of the OLED to segment 0 */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SEG_REMAP_ON);-if(ret<0)-returnret;--/* Turn on the display */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_DISPLAY_ON);-if(ret<0)-returnret;--return0;-}--staticintssd1307fb_ssd1307_remove(structssd1307fb_par*par)-{-pwm_disable(par->pwm);-pwm_put(par->pwm);-return0;-}+if(par->device_info->device_id=DEVID_SSD1307){+par->pwm=pwm_get(&par->client->dev,NULL);+if(IS_ERR(par->pwm)){+dev_err(&par->client->dev,"Could not get PWM from device tree!\n");+returnPTR_ERR(par->pwm);+}-staticstructssd1307fb_opsssd1307fb_ssd1307_ops={-.init=ssd1307fb_ssd1307_init,-.remove=ssd1307fb_ssd1307_remove,-};+par->pwm_period=pwm_get_period(par->pwm);+/* Enable the PWM */+pwm_config(par->pwm,par->pwm_period/2,par->pwm_period);+pwm_enable(par->pwm);-staticintssd1307fb_ssd1306_init(structssd1307fb_par*par)-{-intret;+dev_dbg(&par->client->dev,"Using PWM%d with a %dns period.\n",+par->pwm->pwm,par->pwm_period);+};/* Set initial contrast */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_CONTRAST);if(ret<0)returnret;-ret=ssd1307fb_write_cmd(par->client,0x7f);-if(ret<0)-returnret;--/* Set COM direction */-ret=ssd1307fb_write_cmd(par->client,0xc8);+ret=ssd1307fb_write_cmd(par->client,par->contrast);if(ret<0)returnret;/* Set segment re-map */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SEG_REMAP_ON);+if(par->seg_remap){+ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SEG_REMAP_ON);+if(ret<0)+returnret;+};++/* Set COM direction */+com_invdir=0xc0|(par->com_invdir&0xf)<<3;+ret=ssd1307fb_write_cmd(par->client,com_invdir);if(ret<0)returnret;
@@ -335,34 +331,38 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)if(ret<0)returnret;-ret=ssd1307fb_write_cmd(par->client,0x20);+ret=ssd1307fb_write_cmd(par->client,par->com_offset);if(ret<0)returnret;/* Set clock frequency */+dclk=(par->dclk_div&0xf)|(par->dclk_frq&0xf)<<4;ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_CLOCK_FREQ);if(ret<0)returnret;-ret=ssd1307fb_write_cmd(par->client,0xf0);+ret=ssd1307fb_write_cmd(par->client,dclk);if(ret<0)returnret;/* Set precharge period in number of ticks from the internal clock */+precharge=(par->prechargep1&0xf)|(par->prechargep2&0xf)<<4;ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_PRECHARGE_PERIOD);if(ret<0)returnret;-ret=ssd1307fb_write_cmd(par->client,0x22);+ret=ssd1307fb_write_cmd(par->client,precharge);if(ret<0)returnret;/* Set COM pins configuration */+compins=0x02|(!par->com_seq&0x1)<<4+|(par->com_lrremap&0x1)<<5;ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_COM_PINS_CONFIG);if(ret<0)returnret;-ret=ssd1307fb_write_cmd(par->client,0x22);+ret=ssd1307fb_write_cmd(par->client,compins);if(ret<0)returnret;
@@ -371,18 +371,20 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)if(ret<0)returnret;-ret=ssd1307fb_write_cmd(par->client,0x49);+ret=ssd1307fb_write_cmd(par->client,par->vcomh);if(ret<0)returnret;-/* Turn on the DC-DC Charge Pump */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_CHARGE_PUMP);-if(ret<0)-returnret;+if(par->device_info->device_id=DEVID_SSD1306){+/* Turn on the DC-DC Charge Pump */+ret=ssd1307fb_write_cmd(par->client,SSD1307FB_CHARGE_PUMP);+if(ret<0)+returnret;-ret=ssd1307fb_write_cmd(par->client,0x14);-if(ret<0)-returnret;+ret=ssd1307fb_write_cmd(par->client,0x14);+if(ret<0)+returnret;+};/* Switch to horizontal addressing mode */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_ADDRESS_MODE);
@@ -394,6 +396,7 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)if(ret<0)returnret;+/* Set column range */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_COL_RANGE);if(ret<0)returnret;
@@ -406,6 +409,7 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)if(ret<0)returnret;+/* Set page range */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_PAGE_RANGE);if(ret<0)returnret;
@@ -427,18 +431,28 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)return0;}-staticstructssd1307fb_opsssd1307fb_ssd1306_ops={-.init=ssd1307fb_ssd1306_init,+staticstructssd1307fb_deviceinfossd1307fb_ssd1306_deviceinfo={+.device_id=DEVID_SSD1306,+.default_vcomh=0x20,+.default_dclk_div=0,+.default_dclk_frq=8,+};++staticstructssd1307fb_deviceinfossd1307fb_ssd1307_deviceinfo={+.device_id=DEVID_SSD1307,+.default_vcomh=0x20,+.default_dclk_div=1,+.default_dclk_frq=12,};staticconststructof_device_idssd1307fb_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,},{},};
@@ -469,8 +483,8 @@ static int ssd1307fb_probe(struct i2c_client *client,par->info=info;par->client=client;-par->ops=(structssd1307fb_ops*)of_match_device(ssd1307fb_of_match,-&client->dev)->data;+par->device_info=(structssd1307fb_deviceinfo*)of_match_device(+ssd1307fb_of_match,&client->dev)->data;par->reset=of_get_named_gpio(client->dev.of_node,"reset-gpios",0);
From: Thomas Niederprüm <hidden> Date: 2015-03-01 22:30:32
This patch series is the result of making the ssd1307fb driver work with
a Newhaven OLED display using the Solomon SSD1305 controller. To achieve
this the intialization code for the SSD1306 and the SSD1307 is merged
and based on DT configuration to reflect to various possible wirings
of the SSD130X controller (04/10). Based on these changes it is straight
forward to add support for the SSD1305 controller (05/10).
While working on the driver I realized that it was not possible to
correctly mmap the video memory from userspace since the address handed
to the userspace app is a virtual one where it should be a physical one.
Patch 01/10 fixes this. Furthermore the memory reserved by kzalloc is
not page aligned while the address handed to userspace is aligned to the
next page frame. This problem is fixed by using vmalloc in 02/10.
Furthermore module parameters are added to set the bits per pixel
and the delay for the deferred io update. It makes sense to set
the bits per pixel for the video memory to 8 bits since there is
only very poor userspace support for 1 bit framebuffers.
Also sysfs handles are added to make the contrast settings and dim
mode setting available in userspace.
changes since v1 (thanks to Maxime for the feedback):
- dedicated patch for fixing smem_start address
- remove page reserve upon vmalloc
- remove return value check upon display turn-off at module unload
- use a module parameter refreshrate rather than delaydivider
- allocate fbdefio dynamically
- use sysfs_create_groups to create sysfs entries
- remove contrast, vhcom and dclk properties from DT since they are
not part of hw description. The contrast module parameter was added
to set contrast at load time. vhcom and dclk stays at it's default
values for now.
- add new DT properties to in tree users of ssd130X
- rebased to apply on top of linux-nextThis patch series is the result of making the ssd1307fb driver work with
Thomas Niederprüm (10):
fbdev: ssd1307fb: fix memory address smem_start.
fbdev: ssd1307fb: Use vmalloc to allocate video memory.
Documentation: dts: add missing Solomon Systech vendor prefix.
fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT
fbdev: ssd1307fb: fix in tree users of ssd1306
fbdev: ssd1307fb: Add support for SSD1305
fbdev: ssd1307fb: Add module parameter to set refresh rate of the
display
fbdev: ssd1307fb: Add module parameter bitsperpixel.
fbdev: ssd1307fb: Add sysfs handles to expose contrast and dim setting
to userspace.
fbdev: ssd1307fb: Turn off display on driver unload.
.../devicetree/bindings/vendor-prefixes.txt | 1 +
.../devicetree/bindings/video/ssd1307fb.txt | 24 +-
arch/arm/boot/dts/imx28-cfa10036.dts | 4 +
drivers/video/fbdev/ssd1307fb.c | 354 +++++++++++++++------
4 files changed, 286 insertions(+), 97 deletions(-)
--
2.3.0
From: Thomas Niederprüm <hidden> Date: 2015-03-01 22:30:35
the smem_start pointer of the framebuffer info struct needs to hold the
physical address rather than the virtual address. This patch fixes a
driver crash on mmaping the framebuffer memory due to an access to the
memory address.
Note however that the memory allocated by kzalloc is not page aligned,
while the address presented on a mmap call is aligned to the next page
boudary.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -169,6 +169,7 @@ sitronix Sitronix Technology Corporation smsc Standard Microsystems Corporation snps Synopsys, Inc. solidrun SolidRun+solomon Solomon Systech Limited sony Sony Corporation spansion Spansion Inc. sprd Spreadtrum Communications Inc.
From: Maxime Ripard <hidden> Date: 2015-03-03 07:20:07
On Sun, Mar 01, 2015 at 11:27:54PM +0100, Thomas Niederprüm wrote:
the smem_start pointer of the framebuffer info struct needs to hold the
physical address rather than the virtual address. This patch fixes a
driver crash on mmaping the framebuffer memory due to an access to the
memory address.
Note however that the memory allocated by kzalloc is not page aligned,
while the address presented on a mmap call is aligned to the next page
boudary.
Signed-off-by: Thomas Niederprüm <redacted>
Acked-by: Maxime Ripard <redacted>
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
From: Maxime Ripard <hidden> Date: 2015-03-03 08:55:30
Hi,
On Sun, Mar 01, 2015 at 11:27:55PM +0100, Thomas Niederprüm wrote:
quoted hunk
It makes sense to use vmalloc to allocate the video buffer since it has to be
page aligned memory for using it with mmap. Also deffered io seems buggy in
combination with kmalloc'ed memory (crash on unloading the module).
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Don't you need the vfree in the remove too?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
From: Maxime Ripard <hidden> Date: 2015-03-03 09:35:06
On Sun, Mar 01, 2015 at 11:27:58PM +0100, Thomas Niederprüm wrote:
introducing the new DT properties the in tree users of the SSD1306
controller are updated to be up to date.
Signed-off-by: Thomas Niederprüm <redacted>
This should be prefixed by "ARM: mxs:", and sent to the MXS
maintainers.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
From: Thomas Niederprüm <hidden> Date: 2015-03-03 19:03:16
Am Tue, 3 Mar 2015 09:52:55 +0100
schrieb Maxime Ripard [off-list ref]:
Hi,
On Sun, Mar 01, 2015 at 11:27:55PM +0100, Thomas Niederprüm wrote:
quoted
It makes sense to use vmalloc to allocate the video buffer since it
has to be page aligned memory for using it with mmap. Also deffered
io seems buggy in combination with kmalloc'ed memory (crash on
unloading the module).
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/ssd1307fb.c
b/drivers/video/fbdev/ssd1307fb.c index 61e0ce8..25dd08d 100644
From: Maxime Ripard <hidden> Date: 2015-03-05 21:50:18
On Sun, Mar 01, 2015 at 11:27:57PM +0100, Thomas Niederprüm wrote:
The SSD130X 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-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 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 the values are not set in DT the default values
according to the controllers datasheet are assumed.
Unfortunately, this is not a reasonable thing to do, even if you fix
the existing user, there's still the case where you have an older DT
with a newer kernel.
Keeping (and documenting) the previous defaults is the only easy way
to support this.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
From: Maxime Ripard <hidden> Date: 2015-03-05 22:15:07
On Sun, Mar 01, 2015 at 11:28:00PM +0100, Thomas Niederprüm wrote:
quoted hunk
This patch adds the module parameter "refreshrate" to set delay for the
deferred io. The refresh rate is given in units of Hertz. The default
refresh rate is 1 Hz.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
a space around the operator.
I'm not so sure this is a good solution, since you might perfectly
want to have an SSD1305 with a refreshrate of 1Hz, and an SSD1306 with
a refreshrate of 20Hz.
Unfortunately, beside sysfs, I don't really have a better suggestion.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
From: Thomas Niederprüm <hidden> Date: 2015-03-09 22:19:26
the smem_start pointer of the framebuffer info struct needs to hold the
physical address rather than the virtual address. This patch fixes a
driver crash on mmaping the framebuffer memory due to an access to the
memory address.
Note however that the memory allocated by kzalloc is not page aligned,
while the address presented on a mmap call is aligned to the next page
boudary.
Signed-off-by: Thomas Niederprüm <redacted>
Acked-by: Maxime Ripard <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Thomas Niederprüm <hidden> Date: 2015-03-09 22:19:28
It makes sense to use vmalloc to allocate the video buffer since it has to be
page aligned memory for using it with mmap. Also deffered io seems buggy in
combination with kmalloc'ed memory (crash on unloading the module).
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
@@ -169,6 +169,7 @@ sitronix Sitronix Technology Corporation smsc Standard Microsystems Corporation snps Synopsys, Inc. solidrun SolidRun+solomon Solomon Systech Limited sony Sony Corporation spansion Spansion Inc. sprd Spreadtrum Communications Inc.
From: Thomas Niederprüm <hidden> Date: 2015-03-09 22:19:37
This patch updates the in tree-users of the SSD1306 controller for using
the newly introduced DT properties.
Signed-off-by: Thomas Niederprüm <redacted>
---
arch/arm/boot/dts/imx28-cfa10036.dts | 4 ++++
1 file changed, 4 insertions(+)
From: Thomas Niederprüm <hidden> Date: 2015-03-09 22:19:41
This patch turns off the display when the driver is unloaded.
Signed-off-by: Thomas Niederprüm <redacted>
Acked-by: Maxime Ripard <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 2 ++
1 file changed, 2 insertions(+)
From: Thomas Niederprüm <hidden> Date: 2015-03-09 22:19:44
This patch adds the module parameter "refreshrate" to set delay for the
deferred io. The refresh rate is given in units of Hertz. The default
refresh rate is 1 Hz.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
From: Thomas Niederprüm <hidden> Date: 2015-03-09 22:20:34
This patch adds sysfs handles to enable userspace control over the display
contrast as well as the dim mode. The handles are available as "contrast"
and "dim" in the framebuffers sysfs domain.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 90 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 90 insertions(+)
From: Thomas Niederprüm <hidden> Date: 2015-03-09 22:20:59
This patch adds a module parameter 'bitsperpixel' to adjust the colordepth
of the framebuffer. All values >1 will result in memory map of the requested
color depth. However only the MSB of each pixel will be sent to the device.
The framebuffer identifies itself as a grayscale display with the specified
depth.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
@@ -2,7 +2,7 @@ Required properties: - compatible: Should be "solomon,<chip>fb-<bus>". The only supported bus for- now is i2c, and the supported chips are ssd1306 and ssd1307.+ now is i2c, and the supported chips are ssd1305, ssd1306 and ssd1307. - reg: Should contain address of the controller on the I2C bus. Most likely 0x3c or 0x3d - pwm: Should contain the pwm to use according to the OF device tree PWM
From: Thomas Niederprüm <hidden> Date: 2015-03-09 22:21:48
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 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 <redacted>
---
.../devicetree/bindings/video/ssd1307fb.txt | 21 +++
drivers/video/fbdev/ssd1307fb.c | 195 ++++++++++++---------
2 files changed, 137 insertions(+), 79 deletions(-)
@@ -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: Offset of the first COM pin wired to the panel+ - 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 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
@@ -255,69 +274,46 @@ static struct fb_deferred_io ssd1307fb_defio = {.deferred_io=ssd1307fb_deferred_io,};-staticintssd1307fb_ssd1307_init(structssd1307fb_par*par)+staticintssd1307fb_init(structssd1307fb_par*par){intret;+u32precharge,dclk,com_invdir,compins;-par->pwm=pwm_get(&par->client->dev,NULL);-if(IS_ERR(par->pwm)){-dev_err(&par->client->dev,"Could not get PWM from device tree!\n");-returnPTR_ERR(par->pwm);-}--par->pwm_period=pwm_get_period(par->pwm);-/* Enable the PWM */-pwm_config(par->pwm,par->pwm_period/2,par->pwm_period);-pwm_enable(par->pwm);--dev_dbg(&par->client->dev,"Using PWM%d with a %dns period.\n",-par->pwm->pwm,par->pwm_period);--/* Map column 127 of the OLED to segment 0 */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SEG_REMAP_ON);-if(ret<0)-returnret;--/* Turn on the display */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_DISPLAY_ON);-if(ret<0)-returnret;--return0;-}--staticintssd1307fb_ssd1307_remove(structssd1307fb_par*par)-{-pwm_disable(par->pwm);-pwm_put(par->pwm);-return0;-}+if(par->device_info->device_id=DEVID_SSD1307){+par->pwm=pwm_get(&par->client->dev,NULL);+if(IS_ERR(par->pwm)){+dev_err(&par->client->dev,"Could not get PWM from device tree!\n");+returnPTR_ERR(par->pwm);+}-staticstructssd1307fb_opsssd1307fb_ssd1307_ops={-.init=ssd1307fb_ssd1307_init,-.remove=ssd1307fb_ssd1307_remove,-};+par->pwm_period=pwm_get_period(par->pwm);+/* Enable the PWM */+pwm_config(par->pwm,par->pwm_period/2,par->pwm_period);+pwm_enable(par->pwm);-staticintssd1307fb_ssd1306_init(structssd1307fb_par*par)-{-intret;+dev_dbg(&par->client->dev,"Using PWM%d with a %dns period.\n",+par->pwm->pwm,par->pwm_period);+};/* Set initial contrast */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_CONTRAST);if(ret<0)returnret;-ret=ssd1307fb_write_cmd(par->client,0x7f);-if(ret<0)-returnret;--/* Set COM direction */-ret=ssd1307fb_write_cmd(par->client,0xc8);+ret=ssd1307fb_write_cmd(par->client,par->contrast);if(ret<0)returnret;/* Set segment re-map */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SEG_REMAP_ON);+if(par->seg_remap){+ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SEG_REMAP_ON);+if(ret<0)+returnret;+};++/* Set COM direction */+com_invdir=0xc0|(par->com_invdir&0xf)<<3;+ret=ssd1307fb_write_cmd(par->client,com_invdir);if(ret<0)returnret;
@@ -335,34 +331,38 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)if(ret<0)returnret;-ret=ssd1307fb_write_cmd(par->client,0x20);+ret=ssd1307fb_write_cmd(par->client,par->com_offset);if(ret<0)returnret;/* Set clock frequency */+dclk=(par->dclk_div&0xf)|(par->dclk_frq&0xf)<<4;ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_CLOCK_FREQ);if(ret<0)returnret;-ret=ssd1307fb_write_cmd(par->client,0xf0);+ret=ssd1307fb_write_cmd(par->client,dclk);if(ret<0)returnret;/* Set precharge period in number of ticks from the internal clock */+precharge=(par->prechargep1&0xf)|(par->prechargep2&0xf)<<4;ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_PRECHARGE_PERIOD);if(ret<0)returnret;-ret=ssd1307fb_write_cmd(par->client,0x22);+ret=ssd1307fb_write_cmd(par->client,precharge);if(ret<0)returnret;/* Set COM pins configuration */+compins=0x02|(!par->com_seq&0x1)<<4+|(par->com_lrremap&0x1)<<5;ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_COM_PINS_CONFIG);if(ret<0)returnret;-ret=ssd1307fb_write_cmd(par->client,0x22);+ret=ssd1307fb_write_cmd(par->client,compins);if(ret<0)returnret;
@@ -371,18 +371,20 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)if(ret<0)returnret;-ret=ssd1307fb_write_cmd(par->client,0x49);+ret=ssd1307fb_write_cmd(par->client,par->vcomh);if(ret<0)returnret;-/* Turn on the DC-DC Charge Pump */-ret=ssd1307fb_write_cmd(par->client,SSD1307FB_CHARGE_PUMP);-if(ret<0)-returnret;+if(par->device_info->device_id=DEVID_SSD1306){+/* Turn on the DC-DC Charge Pump */+ret=ssd1307fb_write_cmd(par->client,SSD1307FB_CHARGE_PUMP);+if(ret<0)+returnret;-ret=ssd1307fb_write_cmd(par->client,0x14);-if(ret<0)-returnret;+ret=ssd1307fb_write_cmd(par->client,0x14);+if(ret<0)+returnret;+};/* Switch to horizontal addressing mode */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_ADDRESS_MODE);
@@ -394,6 +396,7 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)if(ret<0)returnret;+/* Set column range */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_COL_RANGE);if(ret<0)returnret;
@@ -406,6 +409,7 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)if(ret<0)returnret;+/* Set page range */ret=ssd1307fb_write_cmd(par->client,SSD1307FB_SET_PAGE_RANGE);if(ret<0)returnret;
@@ -427,18 +431,28 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)return0;}-staticstructssd1307fb_opsssd1307fb_ssd1306_ops={-.init=ssd1307fb_ssd1306_init,+staticstructssd1307fb_deviceinfossd1307fb_ssd1306_deviceinfo={+.device_id=DEVID_SSD1306,+.default_vcomh=0x20,+.default_dclk_div=0,+.default_dclk_frq=8,+};++staticstructssd1307fb_deviceinfossd1307fb_ssd1307_deviceinfo={+.device_id=DEVID_SSD1307,+.default_vcomh=0x20,+.default_dclk_div=1,+.default_dclk_frq=12,};staticconststructof_device_idssd1307fb_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,},{},};
@@ -469,8 +483,8 @@ static int ssd1307fb_probe(struct i2c_client *client,par->info=info;par->client=client;-par->ops=(structssd1307fb_ops*)of_match_device(ssd1307fb_of_match,-&client->dev)->data;+par->device_info=(structssd1307fb_deviceinfo*)of_match_device(+ssd1307fb_of_match,&client->dev)->data;par->reset=of_get_named_gpio(client->dev.of_node,"reset-gpios",0);
From: Thomas Niederprüm <hidden> Date: 2015-03-09 22:21:55
This patch series is the result of making the ssd1307fb driver work with
a Newhaven OLED display using the Solomon SSD1305 controller. To achieve
this the intialization code for the SSD1306 and the SSD1307 is merged
and based on DT configuration to reflect the various possible wirings
of the SSD130X controller (04/10). Based on these changes it is straight
forward to add support for the SSD1305 controller (06/10).
While working on the driver I realized that it was not possible to
correctly mmap the video memory from userspace since the address handed
to the userspace app is a virtual one where it should be a physical one.
Patch 01/10 fixes this. Furthermore the memory reserved by kzalloc is
not page aligned while the address handed to userspace is aligned to the
next page frame. This problem is fixed by using vmalloc in 02/10.
Furthermore module parameters are added to set the bits per pixel
and the delay for the deferred io update. It makes sense to set
the bits per pixel for the video memory to 8 bits since there is
only very poor userspace support for 1 bit framebuffers.
Also sysfs handles are added to make the contrast settings and dim
mode setting available in userspace.
changes since v1 (thanks to Maxime for the feedback):
- dedicated patch for fixing smem_start address
- remove page reserve upon vmalloc
- remove return value check upon display turn-off at module unload
- use a module parameter refreshrate rather than delaydivider
- allocate fbdefio dynamically
- use sysfs_create_groups to create sysfs entries
- remove contrast, vhcom and dclk properties from DT since they are
not part of hw description. The contrast module parameter was added
to set contrast at load time. vhcom and dclk stays at it's default
values for now.
- add new DT properties to in tree users of ssd130X
- rebased to apply on top of linux-next
changes since v2 (thanks to Maxime again):
- free memory allocated by vmalloc on driver unload
- set default values in the init code to the ones of the existing ssd1307
init code
- added two ACKs (Maxime Ripard)
Thomas Niederprüm (10):
fbdev: ssd1307fb: fix memory address smem_start.
fbdev: ssd1307fb: Use vmalloc to allocate video memory.
of: Add Solomon Systech vendor prefix.
fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT
ARM: mxs: fix in tree users of ssd1306
fbdev: ssd1307fb: Add support for SSD1305
fbdev: ssd1307fb: Add module parameter to set refresh rate of the
display
fbdev: ssd1307fb: Add module parameter bitsperpixel.
fbdev: ssd1307fb: Add sysfs handles to expose contrast and dim setting
to userspace.
fbdev: ssd1307fb: Turn off display on driver unload.
.../devicetree/bindings/vendor-prefixes.txt | 1 +
.../devicetree/bindings/video/ssd1307fb.txt | 23 +-
arch/arm/boot/dts/imx28-cfa10036.dts | 4 +
drivers/video/fbdev/ssd1307fb.c | 355 +++++++++++++++------
4 files changed, 286 insertions(+), 97 deletions(-)
--
2.3.0
From: Tomi Valkeinen <hidden> Date: 2015-03-10 10:46:08
On 14/02/15 17:54, Maxime Ripard wrote:
On Sat, Feb 07, 2015 at 05:05:03PM +0100, Thomas Niederprüm wrote:
quoted
Am Sat, 7 Feb 2015 12:20:43 +0100
schrieb Maxime Ripard [off-list ref]:
quoted
On Fri, Feb 06, 2015 at 11:28:11PM +0100, niederp@physik.uni-kl.de
wrote:
quoted
From: Thomas Niederprüm <redacted>
This patch adds a module parameter 'bitsperpixel' to adjust the
colordepth of the framebuffer. All values >1 will result in memory
map of the requested color depth. However only the MSB of each
pixel will be sent to the device. The framebuffer identifies itself
as a grayscale display with the specified depth.
I'm not sure this is the right thing to do.
The bits per pixel for this display is rightfully defined, used and
reported to the userspace, why would you want to change that?
You are right of course. The display is 1bpp and it reports to be 1
bpp. The problem is that there is almost no userspace library that can
handle 1 bit framebuffers correctly. So it is nice if the framebuffer
(optionally) can expose itself as 8 bits per pixel grayscale to the
userspace program. As an example this allows to run DirectFB on the
framebuffer, which is not possible out of the box for 1bpp.
Also note that if do not set the module parameter at load time
the framebuffer will be 1bpp. So you have to actively set that module
parameter to make the framebuffer pretend to be more than 1bpp.
In any case I don't cling to that patch, I just thought it was a nice
feature.
I'd say that the right fix would be to patch DirectFB, instead of
faking that in the kernel.
But again, that's probably Tomi's call, not mine.
Right, I'm not thrilled =). I don't think it's a good idea to lie to the
userspace (except when fixing regressions).
Tomi
I would have thought this was something accessible through the
framebuffer ioctl.
Apparently it's not, at least for the contrast, so maybe it should be
added there, instead of doing it for a single driver?
I think the contrast setting for an OLED display is much like the
backlight setting for LCD panel. Since there is also no ioctl to set
the backlight of an LCD I wonder if the contrast of an OLED should have
one.
It's too much of framebuffer interface debate for me here. Tomi?
We have backlight and contrast already in backlight-class and lcd-class
(drivers/video/backlight/backlight.c and drivers/video/backlight/lcd.c).
Are those something that could be used here instead of custom sysfs files?
Tomi
From: Tomi Valkeinen <hidden> Date: 2015-03-10 11:28:44
On 14/02/15 16:22, Thomas Niederprüm wrote:
Am Thu, 12 Feb 2015 16:11:21 +0100
schrieb Maxime Ripard [off-list ref]:
quoted
On Sat, Feb 07, 2015 at 04:35:41PM +0100, Thomas Niederprüm wrote:
quoted
Am Sat, 7 Feb 2015 12:18:21 +0100
schrieb Maxime Ripard [off-list ref]:
quoted
Hi,
On Fri, Feb 06, 2015 at 11:28:10PM +0100, niederp@physik.uni-kl.de
wrote:
quoted
From: Thomas Niederprüm <redacted>
It makes sense to use vmalloc to allocate the video buffer
since it has to be page aligned memory for using it with mmap.
Please wrap your commit log at 80 chars.
I'll try to do so in future, sorry for that.
quoted
It looks like there's numerous fbdev drivers using this
(especially since you copy pasted that code, without mentionning
it).
Yes, I should have mentioned that in the commit message. As
implicitly indicated in the cover letter the rvmalloc() and
rvfree() are copy pasted from the vfb driver. Honestly, I didn't
give this one too much thought. It seemed a viable solution to the
mmap problem. For a bit more history on that, see my comment below.
quoted
That should be turned into an allocator so that drivers all get
this right.
quoted
Also deffered io seems buggy in combination with kmalloc'ed
memory (crash on unloading the module).
And maybe that's the real issue to fix.
The problem is solved by using vmalloc ;)
Yep, but why do you need to mark the reserved pages?
...
As far as I understood mmaped memory is marked as userspace memory in
the page table and is therefore subject to swapping. The pages are
marked reserved to make clear that this memory can not be swapped and
thus lock the pages in memory. See discussions [0,1,2].
Why is it a problem if it is swapped? Only CPU uses the memory, as far
as I can see.
Also, isn't doing __pa() for the memory returned by vmalloc plain wrong?
What was the crash about when using kmalloc? It would be good to fix
defio, as I don't see why it should not work with kmalloced memory.
Tomi
From: Thomas Niederprüm <hidden> Date: 2015-03-13 21:36:32
Am Tue, 10 Mar 2015 12:45:49 +0200
schrieb Tomi Valkeinen [off-list ref]:
On 14/02/15 17:54, Maxime Ripard wrote:
quoted
On Sat, Feb 07, 2015 at 05:05:03PM +0100, Thomas Niederprüm wrote:
quoted
Am Sat, 7 Feb 2015 12:20:43 +0100
schrieb Maxime Ripard [off-list ref]:
quoted
On Fri, Feb 06, 2015 at 11:28:11PM +0100, niederp@physik.uni-kl.de
wrote:
quoted
From: Thomas Niederprüm <redacted>
This patch adds a module parameter 'bitsperpixel' to adjust the
colordepth of the framebuffer. All values >1 will result in
memory map of the requested color depth. However only the MSB of
each pixel will be sent to the device. The framebuffer
identifies itself as a grayscale display with the specified
depth.
I'm not sure this is the right thing to do.
The bits per pixel for this display is rightfully defined, used
and reported to the userspace, why would you want to change that?
You are right of course. The display is 1bpp and it reports to be 1
bpp. The problem is that there is almost no userspace library that
can handle 1 bit framebuffers correctly. So it is nice if the
framebuffer (optionally) can expose itself as 8 bits per pixel
grayscale to the userspace program. As an example this allows to
run DirectFB on the framebuffer, which is not possible out of the
box for 1bpp.
Also note that if do not set the module parameter at load time
the framebuffer will be 1bpp. So you have to actively set that
module parameter to make the framebuffer pretend to be more than
1bpp.
In any case I don't cling to that patch, I just thought it was a
nice feature.
I'd say that the right fix would be to patch DirectFB, instead of
faking that in the kernel.
But again, that's probably Tomi's call, not mine.
Right, I'm not thrilled =). I don't think it's a good idea to lie to
the userspace (except when fixing regressions).
Ok, since Maxime and you agree that this is not desirable I will drop
that patch in v4.
Thomas
I would have thought this was something accessible through the
framebuffer ioctl.
Apparently it's not, at least for the contrast, so maybe it
should be added there, instead of doing it for a single driver?
I think the contrast setting for an OLED display is much like the
backlight setting for LCD panel. Since there is also no ioctl to
set the backlight of an LCD I wonder if the contrast of an OLED
should have one.
It's too much of framebuffer interface debate for me here. Tomi?
We have backlight and contrast already in backlight-class and
lcd-class (drivers/video/backlight/backlight.c and
drivers/video/backlight/lcd.c). Are those something that could be
used here instead of custom sysfs files?
I just gave the backlight-class a try and it works like a charm. I
will include it in v4 and drop the sysfs handles instead. Thanks for the
hint!
Thomas
From: Thomas Niederprüm <hidden> Date: 2015-03-13 21:36:39
Am Tue, 10 Mar 2015 13:28:25 +0200
schrieb Tomi Valkeinen [off-list ref]:
On 14/02/15 16:22, Thomas Niederprüm wrote:
quoted
Am Thu, 12 Feb 2015 16:11:21 +0100
schrieb Maxime Ripard [off-list ref]:
quoted
On Sat, Feb 07, 2015 at 04:35:41PM +0100, Thomas Niederprüm wrote:
quoted
Am Sat, 7 Feb 2015 12:18:21 +0100
schrieb Maxime Ripard [off-list ref]:
quoted
Hi,
On Fri, Feb 06, 2015 at 11:28:10PM +0100,
niederp@physik.uni-kl.de wrote:
quoted
From: Thomas Niederprüm <redacted>
It makes sense to use vmalloc to allocate the video buffer
since it has to be page aligned memory for using it with mmap.
Please wrap your commit log at 80 chars.
I'll try to do so in future, sorry for that.
quoted
It looks like there's numerous fbdev drivers using this
(especially since you copy pasted that code, without mentionning
it).
Yes, I should have mentioned that in the commit message. As
implicitly indicated in the cover letter the rvmalloc() and
rvfree() are copy pasted from the vfb driver. Honestly, I didn't
give this one too much thought. It seemed a viable solution to the
mmap problem. For a bit more history on that, see my comment
below.
quoted
That should be turned into an allocator so that drivers all get
this right.
quoted
Also deffered io seems buggy in combination with kmalloc'ed
memory (crash on unloading the module).
And maybe that's the real issue to fix.
The problem is solved by using vmalloc ;)
Yep, but why do you need to mark the reserved pages?
...
As far as I understood mmaped memory is marked as userspace memory
in the page table and is therefore subject to swapping. The pages
are marked reserved to make clear that this memory can not be
swapped and thus lock the pages in memory. See discussions [0,1,2].
Why is it a problem if it is swapped? Only CPU uses the memory, as far
as I can see.
It seems to be no problem at all. I was copying the allocation code
from the vfb driver. The memory is no longer marked as reserved from
v2 on.
Also, isn't doing __pa() for the memory returned by vmalloc plain
wrong?
What was the crash about when using kmalloc? It would be good to fix
defio, as I don't see why it should not work with kmalloced memory.
The main challenge here is that the memory handed to userspace upon
mmap call needs to be page aligned. The memory returned by kmalloc has
no such alignment, but the pointer presented to the userspace program
gets aligned to next page boundary. It's not clear to me whether there
is an easy way to obtain page aligned kmalloc memory. Memory
allocated by vmalloc on the other hand is always aligned to page
boundaries. This is why I chose to go for vmalloc.
Thomas
On Fri, Mar 13, 2015 at 10:31 PM, Thomas Niederprüm
[off-list ref] wrote:
Am Tue, 10 Mar 2015 13:28:25 +0200
schrieb Tomi Valkeinen [off-list ref]:
quoted
Also, isn't doing __pa() for the memory returned by vmalloc plain
wrong?
quoted
What was the crash about when using kmalloc? It would be good to fix
defio, as I don't see why it should not work with kmalloced memory.
The main challenge here is that the memory handed to userspace upon
mmap call needs to be page aligned. The memory returned by kmalloc has
no such alignment, but the pointer presented to the userspace program
gets aligned to next page boundary. It's not clear to me whether there
is an easy way to obtain page aligned kmalloc memory. Memory
allocated by vmalloc on the other hand is always aligned to page
boundaries. This is why I chose to go for vmalloc.
__get_free_pages()?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
@@ -169,6 +169,7 @@ sitronix Sitronix Technology Corporation smsc Standard Microsystems Corporation snps Synopsys, Inc. solidrun SolidRun+solomon Solomon Systech Limited sony Sony Corporation spansion Spansion Inc. sprd Spreadtrum Communications Inc.
From: Thomas Niederprüm <hidden> Date: 2015-03-16 17:09:02
This patch series is the result of making the ssd1307fb driver work with
a Newhaven OLED display using the Solomon SSD1305 controller. To achieve
this the intialization code for the SSD1306 and the SSD1307 is merged
and based on DT configuration to reflect the various possible wirings
of the SSD130X controller (04/10). Based on these changes it is straight
forward to add support for the SSD1305 controller (06/10).
While working on the driver I realized that it was not possible to
correctly mmap the video memory from userspace since the address handed
to the userspace app is a logical one where it should be a physical one.
Patch 01/10 fixes this. Furthermore the memory reserved by kzalloc is
not page aligned while the address handed to userspace is aligned to the
next page frame. This problem is fixed by using __get_free_pages() in 02/10.
Furthermore a module parameter is added to set the delay for the
deferred io update (07/10). Also the backlight class is implemented to make
the contrast setting available in userspace (09/10).
changes since v1 (thanks to Maxime for the feedback):
- dedicated patch for fixing smem_start address
- remove page reserve upon vmalloc
- remove return value check upon display turn-off at module unload
- use a module parameter refreshrate rather than delaydivider
- allocate fbdefio dynamically
- use sysfs_create_groups to create sysfs entries
- remove contrast, vhcom and dclk properties from DT since they are
not part of hw description. The contrast module parameter was added
to set contrast at load time. vhcom and dclk stays at it's default
values for now.
- add new DT properties to in tree users of ssd130X
- rebased to apply on top of linux-next
changes since v2 (thanks to Maxime again):
- free memory allocated by vmalloc on driver unload
- set default values in the init code to the ones of the existing ssd1307
init code
- added two ACKs (Maxime Ripard)
changes since v3:
- use backlight class rather than dedicated sysfs files to set the
contrast (Thanks to Tomi Valkeinen)
- remove [PATCHv3 08/10] fbdev: ssd1307fb: Add module parameter bitsperpixel
- add new patch to blank the display (unreviewed)
- allocate video memory through __get_free_pages() rather than vmalloc
(Thanks to Geert Uytterhoeven)
- minor rewordings of the commit messages
Thomas Niederprüm (10):
fbdev: ssd1307fb: fix memory address smem_start.
fbdev: ssd1307fb: Allocate page aligned video memory.
of: Add Solomon Systech vendor prefix.
fbdev: ssd1307fb: Unify init code and obtain hw specific bits from DT
ARM: mxs: fix in tree users of ssd1306
fbdev: ssd1307fb: Add support for SSD1305
fbdev: ssd1307fb: Add a module parameter to set the refresh rate
fbdev: ssd1307fb: Turn off display on driver unload.
fbdev: ssd1307fb: add backlight controls for setting the contrast
fbdev: ssd1307fb: Add blank mode
.../devicetree/bindings/vendor-prefixes.txt | 1 +
.../devicetree/bindings/video/ssd1307fb.txt | 23 +-
arch/arm/boot/dts/imx28-cfa10036.dts | 3 +
drivers/video/fbdev/Kconfig | 1 +
drivers/video/fbdev/ssd1307fb.c | 310 +++++++++++++++------
5 files changed, 250 insertions(+), 88 deletions(-)
--
2.3.0
From: Thomas Niederprüm <hidden> Date: 2015-03-16 17:09:04
Currently the videomemory is allocated by kmalloc, making it a memory
region that is not necessarily page aligend. This leads to problems
upon mmap call, where the video memory's address gets aligned to the
next page boundary. The result is that the userspace program that issued
the mmap call is not able to access the video memory from the start to
the next page boundary.
This patch changes the allocation of the video memory to use
__get_free_pages() in order to obtain memory that is aligned
to page boundaries.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Thomas Niederprüm <hidden> Date: 2015-03-16 17:09:06
the smem_start pointer of the framebuffer info struct needs to hold the
physical address rather than the logical address. Right now the logical
address returned by kmalloc is stored. This patch converts this address
to a physical address and thus fixes a driver crash on mmaping the
framebuffer memory due to an access to the wrong memory address.
Signed-off-by: Thomas Niederprüm <redacted>
Acked-by: Maxime Ripard <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Thomas Niederprüm <hidden> Date: 2015-03-16 17:09:11
This patch adds ssd1307fb_blank() to make the framebuffer capable
of blanking.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/ssd1307fb.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
From: Thomas Niederprüm <hidden> Date: 2015-03-16 17:09:41
The backlight class is used to create userspace handles for
setting the OLED contrast.
Signed-off-by: Thomas Niederprüm <redacted>
---
drivers/video/fbdev/Kconfig | 1 +
drivers/video/fbdev/ssd1307fb.c | 58 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 59 insertions(+)
@@ -607,10 +649,24 @@ static int ssd1307fb_probe(struct i2c_client *client,gotopanel_init_error;}+snprintf(bl_name,sizeof(bl_name),"ssd1307fb%d",info->node);+bl=backlight_device_register(bl_name,&client->dev,par,+&ssd1307fb_bl_ops,NULL);+bl->props.brightness=contrast;+bl->props.max_brightness=MAX_CONTRAST;+info->bl_dev=bl;++if(IS_ERR(bl)){+dev_err(&client->dev,"unable to register backlight device: %ld\n",+PTR_ERR(bl));+gotobl_init_error;+}dev_info(&client->dev,"fb%d: %s framebuffer device registered, using %d bytes of video memory\n",info->node,info->fix.id,vmem_size);return0;+bl_init_error:+unregister_framebuffer(info);panel_init_error:if(par->device_info->device_id=DEVID_SSD1307){pwm_disable(par->pwm);
@@ -630,6 +686,8 @@ static int ssd1307fb_remove(struct i2c_client *client)ssd1307fb_write_cmd(par->client,SSD1307FB_DISPLAY_OFF);+backlight_device_unregister(info->bl_dev);+unregister_framebuffer(info);if(par->device_info->device_id=DEVID_SSD1307){pwm_disable(par->pwm);