Message-ID: <20150320221254.2cc5f502@maestro.intranet>
Date:	Fri, 20 Mar 2015 22:12:54 +0100
From:	Thomas Niederprüm <niederp@...sik.uni-kl.de>
To:	Maxime Ripard <maxime.ripard@...e-electrons.com>
Cc:	plagnioj@...osoft.com, tomi.valkeinen@...com,
	kernel@...gutronix.de, shawn.guo@...aro.org, robh+dt@...nel.org,
	linux-fbdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv4 04/10] fbdev: ssd1307fb: Unify init code and obtain hw
 specific bits from DT

Am Wed, 18 Mar 2015 20:27:19 +0100
schrieb Maxime Ripard <maxime.ripard@...e-electrons.com>:

> 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@...sik.uni-kl.de>
> 
> 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.

The com-offset determines which line of the display panel should
receive the first line of video memory. When designing your display
module you could decide to do something crazy and connect the first
lines of your panel to  (for example) pin COM32 onward. By setting the
com-offset to 32 you could still recover the correct image on your
panel. So in some sense the unit of the com-offset is display lines or
number of pins, depending on whether you see it from the controller
side or from the panel side. I will update the documentation to reflect
this.

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

I agree, this should disappear or go into another patch. Since the
contrast can also be set through the backlight class, I wonder whether
it is useful to keep this module parameter. I'm using it right now to
set the initial contrast as in my use case I almost always want the
smallest possible contrast setting. In any case I will split this into
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.
I will move the assignment right in front of the write command.
 
> >  	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.
Im surprised that I didn't see that myself, because the way it
highlighted is hard to miss. However, thank you for catching this.

> >  	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?

Are you talking about the device_id inside the struct
ssd1307_deviceinfo? I need the device_id to serve special needs of the
individual controllers during initialization. For example the ssd1307
needs to set up the pwm in the init code. 


> >  	},
> >  	{},
> >  };
> > @@ -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
> > 

Thomas

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists