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

From: Maxime Ripard
Date: Wed Mar 18 2015 - 15:30:10 EST

Hi Thomas,

On Mon, Mar 16, 2015 at 06:11:52PM +0100, Thomas Niederprüm wrote:
> The 130X controllers are very similar from the configuration point of view.
> The configuration registers for the SSD1305/6/7 are bit identical (except the
> the VHCOM register and the the default values for clock setup register). This
> patch unifies the init code of the controller and adds hardware specific
> properties to DT that are needed to correctly initialize the device.
>
> The SSD130X can be wired to the OLED panel in various ways. Even for the
> same controller this wiring can differ from one display module to another
> and can not be probed by software. The added DT properties reflect these
> hardware decisions of the display module manufacturer.
> The 'com-sequential', 'com-lrremap' and 'com-invdir' values define different
> possibilities for the COM signals pin configuration and readout direction
> of the video memory. The 'segment-no-remap' allows the inversion of the
> memory-to-pin mapping ultimately inverting the order of the controllers
> output pins. The 'prechargepX' values need to be adapted according the
> capacitance of the OLEDs pixel cells.
>
> So far these hardware specific bits are hard coded in the init code, making
> the driver usable only for one certain wiring of the controller. This patch
> makes the driver usable with all possible hardware setups, given a valid hw
> description in DT. If these values are not set in DT the default values,
> as they are set in the ssd1307 init code right now, are used. This implies
> that without the corresponding DT property "segment-no-remap" the segment
> remap of the ssd130X controller gets activated. Even though this is not the
> default behaviour according to the datasheet it maintains backward
> compatibility with older DTBs.
>
> Note that the SSD1306 does not seem to be using the configuration written to
> the registers at all. Therefore this patch does not try to maintain these
> values without changes in DT. For reference an example is added to the DT
> bindings documentation that reproduces the configuration that is set in the
> current init code.
>
> Signed-off-by: Thomas Niederprüm <niederp@xxxxxxxxxxxxxxxx>

This looks mostly fine, thanks for your effort on this.

> ---
> .../devicetree/bindings/video/ssd1307fb.txt | 21 +++
> drivers/video/fbdev/ssd1307fb.c | 195 ++++++++++++---------
> 2 files changed, 137 insertions(+), 79 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/video/ssd1307fb.txt b/Documentation/devicetree/bindings/video/ssd1307fb.txt
> index 7a12542..be27562 100644
> --- a/Documentation/devicetree/bindings/video/ssd1307fb.txt
> +++ b/Documentation/devicetree/bindings/video/ssd1307fb.txt
> @@ -15,6 +15,16 @@ Required properties:
>
> Optional properties:
> - reset-active-low: Is the reset gpio is active on physical low?
> + - solomon,segment-no-remap: Display needs normal (non-inverted) data column
> + to segment mapping
> + - solomon,com-sequential: Display uses sequential COM pin configuration
> + - solomon,com-lrremap: Display uses left-right COM pin remap
> + - solomon,com-invdir: Display uses inverted COM pin scan direction
> + - solomon,com-offset: Offset of the first COM pin wired to the panel

Documenting the offset unit would be fine.

> + - 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
>
> @@ -26,3 +36,14 @@ ssd1307: oled@3c {
> reset-gpios = <&gpio2 7>;
> reset-active-low;
> };
> +
> +ssd1306: oled@3c {
> + compatible = "solomon,ssd1306fb-i2c";
> + reg = <0x3c>;
> + pwms = <&pwm 4 3000>;
> + reset-gpios = <&gpio2 7>;
> + reset-active-low;
> + solomon,com-lrremap;
> + solomon,com-invdir;
> + solomon,com-offset = <32>;
> +};
> diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
> index 8d34c56..9a66118 100644
> --- a/drivers/video/fbdev/ssd1307fb.c
> +++ b/drivers/video/fbdev/ssd1307fb.c
> @@ -16,6 +16,9 @@
> #include <linux/pwm.h>
> #include <linux/delay.h>
>
> +#define DEVID_SSD1306 6
> +#define DEVID_SSD1307 7
> +
> #define SSD1307FB_DATA 0x40
> #define SSD1307FB_COMMAND 0x80
>
> @@ -38,22 +41,38 @@
> #define SSD1307FB_SET_COM_PINS_CONFIG 0xda
> #define SSD1307FB_SET_VCOMH 0xdb
>
> +static u_int contrast = 128;
> +module_param(contrast, uint, S_IRUGO);
> +

Why is this patch adding a contrast parameter? You don't even mention
it in your commit log. This should be a separate patch.

> struct ssd1307fb_par;
>
> -struct ssd1307fb_ops {
> - int (*init)(struct ssd1307fb_par *);
> - int (*remove)(struct ssd1307fb_par *);
> +struct ssd1307fb_deviceinfo {
> + int device_id;
> + u32 default_vcomh;
> + u32 default_dclk_div;
> + u32 default_dclk_frq;
> };
>
> struct ssd1307fb_par {
> + u32 com_invdir;
> + u32 com_lrremap;
> + u32 com_offset;
> + u32 com_seq;
> + u32 contrast;
> + u32 dclk_div;
> + u32 dclk_frq;
> + struct ssd1307fb_deviceinfo *device_info;
> struct i2c_client *client;
> u32 height;
> struct fb_info *info;
> - struct ssd1307fb_ops *ops;
> u32 page_offset;
> + u32 prechargep1;
> + u32 prechargep2;
> struct pwm_device *pwm;
> u32 pwm_period;
> int reset;
> + u32 seg_remap;
> + u32 vcomh;
> u32 width;
> };
>
> @@ -254,69 +273,46 @@ static struct fb_deferred_io ssd1307fb_defio = {
> .deferred_io = ssd1307fb_deferred_io,
> };
>
> -static int ssd1307fb_ssd1307_init(struct ssd1307fb_par *par)
> +static int ssd1307fb_init(struct ssd1307fb_par *par)
> {
> int ret;
> + u32 precharge, 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");
> - return PTR_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)
> - return ret;
> -
> - /* Turn on the display */
> - ret = ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_ON);
> - if (ret < 0)
> - return ret;
> -
> - return 0;
> -}
> -
> -static int ssd1307fb_ssd1307_remove(struct ssd1307fb_par *par)
> -{
> - pwm_disable(par->pwm);
> - pwm_put(par->pwm);
> - return 0;
> -}
> + 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");
> + return PTR_ERR(par->pwm);
> + }
>
> -static struct ssd1307fb_ops ssd1307fb_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);
>
> -static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
> -{
> - int ret;
> + 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)
> return ret;
>
> - ret = ssd1307fb_write_cmd(par->client, 0x7f);
> - if (ret < 0)
> - return ret;
> -
> - /* Set COM direction */
> - ret = ssd1307fb_write_cmd(par->client, 0xc8);
> + ret = ssd1307fb_write_cmd(par->client, par->contrast);
> if (ret < 0)
> return ret;
>
> /* 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)
> + return ret;
> + };
> +
> + /* Set COM direction */
> + com_invdir = 0xc0 | (par->com_invdir & 0xf) << 3;
> + ret = ssd1307fb_write_cmd(par->client, com_invdir);
> if (ret < 0)
> return ret;
>
> @@ -334,34 +330,38 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
> if (ret < 0)
> return ret;
>
> - ret = ssd1307fb_write_cmd(par->client, 0x20);
> + ret = ssd1307fb_write_cmd(par->client, par->com_offset);
> if (ret < 0)
> return ret;
>
> /* 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)
> return ret;
>
> - ret = ssd1307fb_write_cmd(par->client, 0xf0);
> + ret = ssd1307fb_write_cmd(par->client, dclk);

It would make more sense to assign the variable where you use it.

> if (ret < 0)
> return ret;
>
> /* 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)
> return ret;
>
> - ret = ssd1307fb_write_cmd(par->client, 0x22);
> + ret = ssd1307fb_write_cmd(par->client, precharge);

Here too.

> if (ret < 0)
> return ret;
>
> /* 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)
> return ret;
>
> - ret = ssd1307fb_write_cmd(par->client, 0x22);
> + ret = ssd1307fb_write_cmd(par->client, compins);

And here.

> if (ret < 0)
> return ret;
>
> @@ -370,18 +370,20 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
> if (ret < 0)
> return ret;
>
> - ret = ssd1307fb_write_cmd(par->client, 0x49);
> + ret = ssd1307fb_write_cmd(par->client, par->vcomh);
> if (ret < 0)
> return ret;
>
> - /* Turn on the DC-DC Charge Pump */
> - ret = ssd1307fb_write_cmd(par->client, SSD1307FB_CHARGE_PUMP);
> - if (ret < 0)
> - return ret;
> + 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)
> + return ret;
>
> - ret = ssd1307fb_write_cmd(par->client, 0x14);
> - if (ret < 0)
> - return ret;
> + ret = ssd1307fb_write_cmd(par->client, 0x14);
> + if (ret < 0)
> + return ret;
> + };
>
> /* Switch to horizontal addressing mode */
> ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_ADDRESS_MODE);
> @@ -393,6 +395,7 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
> if (ret < 0)
> return ret;
>
> + /* Set column range */

The indentation is wrong.

> ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_COL_RANGE);
> if (ret < 0)
> return ret;
> @@ -405,6 +408,7 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
> if (ret < 0)
> return ret;
>
> + /* Set page range */

Here too.

> ret = ssd1307fb_write_cmd(par->client, SSD1307FB_SET_PAGE_RANGE);
> if (ret < 0)
> return ret;
> @@ -426,18 +430,28 @@ static int ssd1307fb_ssd1306_init(struct ssd1307fb_par *par)
> return 0;
> }
>
> -static struct ssd1307fb_ops ssd1307fb_ssd1306_ops = {
> - .init = ssd1307fb_ssd1306_init,
> +static struct ssd1307fb_deviceinfo ssd1307fb_ssd1306_deviceinfo = {
> + .device_id = DEVID_SSD1306,
> + .default_vcomh = 0x20,
> + .default_dclk_div = 0,
> + .default_dclk_frq = 8,
> +};
> +
> +static struct ssd1307fb_deviceinfo ssd1307fb_ssd1307_deviceinfo = {
> + .device_id = DEVID_SSD1307,
> + .default_vcomh = 0x20,
> + .default_dclk_div = 1,
> + .default_dclk_frq = 12,
> };
>
> static const struct of_device_id ssd1307fb_of_match[] = {
> {
> .compatible = "solomon,ssd1306fb-i2c",
> - .data = (void *)&ssd1307fb_ssd1306_ops,
> + .data = (void *)&ssd1307fb_ssd1306_deviceinfo,
> },
> {
> .compatible = "solomon,ssd1307fb-i2c",
> - .data = (void *)&ssd1307fb_ssd1307_ops,
> + .data = (void *)&ssd1307fb_ssd1307_deviceinfo,

Do we need this ID? Wouldn't it make more sense to pass the pointer to
the struct we need to use?

> },
> {},
> };
> @@ -468,8 +482,8 @@ static int ssd1307fb_probe(struct i2c_client *client,
> par->info = info;
> par->client = client;
>
> - par->ops = (struct ssd1307fb_ops *)of_match_device(ssd1307fb_of_match,
> - &client->dev)->data;
> + par->device_info = (struct ssd1307fb_deviceinfo *)of_match_device(
> + ssd1307fb_of_match, &client->dev)->data;
>
> par->reset = of_get_named_gpio(client->dev.of_node,
> "reset-gpios", 0);
> @@ -487,6 +501,27 @@ static int ssd1307fb_probe(struct i2c_client *client,
> if (of_property_read_u32(node, "solomon,page-offset", &par->page_offset))
> par->page_offset = 1;
>
> + if (of_property_read_u32(node, "solomon,com-offset", &par->com_offset))
> + par->com_offset = 0;
> +
> + if (of_property_read_u32(node, "solomon,prechargep1", &par->prechargep1))
> + par->prechargep1 = 2;
> +
> + if (of_property_read_u32(node, "solomon,prechargep2", &par->prechargep2))
> + par->prechargep2 = 0;
> +
> + par->seg_remap = !of_property_read_bool(node, "solomon,segment-no-remap");
> + par->com_seq = of_property_read_bool(node, "solomon,com-sequential");
> + par->com_lrremap = of_property_read_bool(node, "solomon,com-lrremap");
> + par->com_invdir = of_property_read_bool(node, "solomon,com-invdir");
> +
> + par->contrast = contrast;
> + par->vcomh = par->device_info->default_vcomh;
> +
> + /* Setup display timing */
> + par->dclk_div = par->device_info->default_dclk_div;
> + par->dclk_frq = par->device_info->default_dclk_frq;
> +
> vmem_size = par->width * par->height / 8;
>
> vmem = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> @@ -539,11 +574,9 @@ static int ssd1307fb_probe(struct i2c_client *client,
> gpio_set_value(par->reset, 1);
> udelay(4);
>
> - if (par->ops->init) {
> - ret = par->ops->init(par);
> - if (ret)
> - goto reset_oled_error;
> - }
> + ret = ssd1307fb_init(par);
> + if (ret)
> + goto reset_oled_error;
>
> ret = register_framebuffer(info);
> if (ret) {
> @@ -556,8 +589,10 @@ static int ssd1307fb_probe(struct i2c_client *client,
> return 0;
>
> panel_init_error:
> - if (par->ops->remove)
> - par->ops->remove(par);
> + if (par->device_info->device_id == DEVID_SSD1307) {
> + pwm_disable(par->pwm);
> + pwm_put(par->pwm);
> + };
> reset_oled_error:
> fb_deferred_io_cleanup(info);
> fb_alloc_error:
> @@ -571,8 +606,10 @@ static int ssd1307fb_remove(struct i2c_client *client)
> struct ssd1307fb_par *par = info->par;
>
> unregister_framebuffer(info);
> - if (par->ops->remove)
> - par->ops->remove(par);
> + if (par->device_info->device_id == DEVID_SSD1307) {
> + pwm_disable(par->pwm);
> + pwm_put(par->pwm);
> + };
> fb_deferred_io_cleanup(info);
> __free_pages(__va(info->fix.smem_start), get_order(info->fix.smem_len));
> framebuffer_release(info);
> --
> 2.3.0
>

Thanks,
Maxime

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

Attachment: signature.asc
Description: Digital signature