Am Sat, 7 Feb 2015 12:45:34 +0100
schrieb Maxime Ripard <[email protected]>:

> On Fri, Feb 06, 2015 at 11:28:14PM +0100, [email protected]
> wrote:
> > From: Thomas Niederprüm <[email protected]>
> >
>
> A commit log is always nice :)

Will be added.

>
> > Signed-off-by: Thomas Niederprüm <[email protected]>
> > ---
> > drivers/video/fbdev/ssd1307fb.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/video/fbdev/ssd1307fb.c
> > b/drivers/video/fbdev/ssd1307fb.c index 02931c7..be91dfc 100644
> > --- a/drivers/video/fbdev/ssd1307fb.c
> > +++ b/drivers/video/fbdev/ssd1307fb.c
> > @@ -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.

>
> Maxime
>

2015-02-09 08:55:40

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH 7/8] fbdev: ssd1307fb: Add sysfs handles to expose contrast and dim setting to userspace.

On Sat, Feb 07, 2015 at 05:42:44PM +0100, Thomas Niederpr?m wrote:
> > > +static struct device_attribute device_attrs[] = {
> > > + __ATTR(contrast, S_IRUGO|S_IWUSR, show_contrast,
> > > store_contrast),
> > > + __ATTR(dim, S_IRUGO|S_IWUSR, show_dim, store_dim),
> > > +
> > > +};
> > > +
> >
> > 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

Attachments:

(No filename) (926.00 B)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-02-09 09:06:16

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 <[email protected]>:
>
> > Hi,
> >
> > On Fri, Feb 06, 2015 at 11:28:10PM +0100, [email protected]
> > wrote:
> > > From: Thomas Niederpr?m <[email protected]>
> > >
> > > 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.
> >
> > > 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?

...

> > > @@ -570,7 +608,7 @@ static int ssd1307fb_probe(struct i2c_client
> > > *client, info->var.blue.offset = 0;
> > >
> > > info->screen_base = (u8 __force __iomem *)vmem;
> > > - info->fix.smem_start = (unsigned long)vmem;
> > > + info->fix.smem_start = __pa(vmem);
> >
> > 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

And the documentation of fb_fix_screeninfo indeed state that this
should be the physical address:
http://lxr.free-electrons.com/source/include/uapi/linux/fb.h#L158

Could you make that a separate patch?

Thanks,
Maxime

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

Attachments:

(No filename) (2.74 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-02-12 16:45:20

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH 2/8] fbdev: ssd1307fb: Unify init code and make controller configurable from device tree

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 <[email protected]>:
>
> > Hi,
> >
> > On Fri, Feb 06, 2015 at 11:28:08PM +0100, [email protected]
> > wrote:
> > > From: Thomas Niederpr?m <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > .../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?

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 :)

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

Attachments:

(No filename) (5.13 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-02-12 17:00:22

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 <[email protected]>:
>
> > 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 <[email protected]>:
> > >
> > > > Hi,
> > > >
> > > > On Fri, Feb 06, 2015 at 11:28:10PM +0100, [email protected]
> > > > wrote:
> > > > > From: Thomas Niederpr?m <[email protected]>
> > > > >
> > > > > 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.
> > > >
> > > > > 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].
>
> [0]http://stackoverflow.com/questions/10760479/mmap-kernel-buffer-to-user-space
> [1]http://www.linuxquestions.org/questions/linux-kernel-70/why-why-setpagereserved-is-needed-when-map-a-kernel-space-to-user-space-885176/
> [2]https://sites.google.com/site/skartikeyan/mmap.html

Hmmm, both https://lwn.net/Articles/28746/ and
http://linux-mm.org/DeviceDriverMmap tell otherwise :)

Maxime

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

Attachments:

(No filename) (2.46 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2015-02-14 15:55:06

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 <[email protected]>:
>
> > 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 <[email protected]>:
> > >
> > > > Hi,
> > > >
> > > > On Fri, Feb 06, 2015 at 11:28:08PM +0100, [email protected]
> > > > wrote:
> > > > > From: Thomas Niederpr?m <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > .../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.

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

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

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

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

Attachments:

(No filename) (9.15 kB)
signature.asc (819.00 B)
Digital signature Download all attachments