Patchwork [PATCHv13,2/4] video: imxfb: Also add pwmr for the device tree.

login
register
mail settings
Submitter Denis Carikli
Date Dec. 5, 2013, 3:43 p.m.
Message ID <1386258219-26437-2-git-send-email-denis@eukrea.com>
Download mbox | patch
Permalink /patch/297173/
State New
Headers show

Comments

Denis Carikli - Dec. 5, 2013, 3:43 p.m.
pwmr has to be set to get the imxfb backlight work,
though pwmr was only configurable trough the platform data.

Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: devicetree@vger.kernel.org
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Eric Bénard <eric@eukrea.com>
Signed-off-by: Denis Carikli <denis@eukrea.com>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Acked-by: Grant Likely <grant.likely@linaro.org>
---
 .../devicetree/bindings/video/fsl,imx-fb.txt       |    3 +++
 drivers/video/imxfb.c                              |    2 ++
 2 files changed, 5 insertions(+)
Alexander Shiyan - Dec. 5, 2013, 3:53 p.m.
> pwmr has to be set to get the imxfb backlight work,
> though pwmr was only configurable trough the platform data.

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/185022.html

...
---
Sascha Hauer - Dec. 6, 2013, 7:05 a.m.
On Thu, Dec 05, 2013 at 04:43:37PM +0100, Denis Carikli wrote:
> pwmr has to be set to get the imxfb backlight work,
> though pwmr was only configurable trough the platform data.
> 
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: devicetree@vger.kernel.org
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: linux-fbdev@vger.kernel.org
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: Eric Bénard <eric@eukrea.com>
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Acked-by: Grant Likely <grant.likely@linaro.org>
> ---
>  .../devicetree/bindings/video/fsl,imx-fb.txt       |    3 +++
>  drivers/video/imxfb.c                              |    2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> index 46da08d..ac457ae 100644
> --- a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> +++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> @@ -17,6 +17,9 @@ Required nodes:
>  Optional properties:
>  - fsl,dmacr: DMA Control Register value. This is optional. By default, the
>  	register is not modified as recommended by the datasheet.
> +- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
> +	optional, but defining it is necessary to get the backlight working. If that
> +	property is ommited, the register is zeroed.

Why isn't this implemented as a backlight driver? Static devicetree
provided values is very limiting.

Sascha
Alexander Shiyan - Dec. 6, 2013, 8:03 a.m.
> On Thu, Dec 05, 2013 at 04:43:37PM +0100, Denis Carikli wrote:
> > pwmr has to be set to get the imxfb backlight work,
> > though pwmr was only configurable trough the platform data.
> > 
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Pawel Moll <pawel.moll@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Stephen Warren <swarren@wwwdotorg.org>
> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > Cc: devicetree@vger.kernel.org
> > Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > Cc: linux-fbdev@vger.kernel.org
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: Eric Bénard <eric@eukrea.com>
> > Signed-off-by: Denis Carikli <denis@eukrea.com>
> > Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > Acked-by: Grant Likely <grant.likely@linaro.org>
> > ---
> >  .../devicetree/bindings/video/fsl,imx-fb.txt       |    3 +++
> >  drivers/video/imxfb.c                              |    2 ++
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > index 46da08d..ac457ae 100644
> > --- a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > +++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > @@ -17,6 +17,9 @@ Required nodes:
> >  Optional properties:
> >  - fsl,dmacr: DMA Control Register value. This is optional. By default, the
> >  	register is not modified as recommended by the datasheet.
> > +- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
> > +	optional, but defining it is necessary to get the backlight working. If that
> > +	property is ommited, the register is zeroed.
> 
> Why isn't this implemented as a backlight driver? Static devicetree
> provided values is very limiting.

Let's understand the terminology.
This register should be renamed according to the datasheet, i.e. LPCCR.
As I pointed out earlier, it is NOT control the backlight, this is a contrast control.
Yes, it works as PWM, but nothing do with the backlight subsystem.
Yes, we can make a driver for this PWM, but how are we going to control it?
I misunderstood something?

---
Sascha Hauer - Dec. 6, 2013, 8:16 a.m.
On Fri, Dec 06, 2013 at 12:03:54PM +0400, Alexander Shiyan wrote:
> > >  .../devicetree/bindings/video/fsl,imx-fb.txt       |    3 +++
> > >  drivers/video/imxfb.c                              |    2 ++
> > >  2 files changed, 5 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > > index 46da08d..ac457ae 100644
> > > --- a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > > +++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > > @@ -17,6 +17,9 @@ Required nodes:
> > >  Optional properties:
> > >  - fsl,dmacr: DMA Control Register value. This is optional. By default, the
> > >  	register is not modified as recommended by the datasheet.
> > > +- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
> > > +	optional, but defining it is necessary to get the backlight working. If that
> > > +	property is ommited, the register is zeroed.
> > 
> > Why isn't this implemented as a backlight driver? Static devicetree
> > provided values is very limiting.
> 
> Let's understand the terminology.
> This register should be renamed according to the datasheet, i.e. LPCCR.
> As I pointed out earlier, it is NOT control the backlight, this is a contrast control.
> Yes, it works as PWM, but nothing do with the backlight subsystem.
> Yes, we can make a driver for this PWM, but how are we going to control it?
> I misunderstood something?

I stumbled upon 'get the backlight working' which implied for me that it
should be a backlight driver. But you're right and now I remember we
talked about this already.

I still think this should be something adjustable, not static data.
Maybe we could change the wording to something like "This property
provides the default value for the contrast control register" since even
if we add driver support for controlling the contrast we still want
to have a sane default.

BTW the contrast could be controlled with a lcd_device (see
lcd_device_register) which seems to be very easy to implement.

SaschaMaybe we could change the wording to something like "This property
provides the default value for the contrast control register" since even
if we add driver support for controlling the contrast we still want
to have a sane default.

BTW the contrast could be controlled with a lcd_device (see
lcd_device_register) which seems to be very easy to implement.

Sascha
Alexander Shiyan - Dec. 6, 2013, 8:35 a.m.
> On Fri, Dec 06, 2013 at 12:03:54PM +0400, Alexander Shiyan wrote:
...
> > > >  - fsl,dmacr: DMA Control Register value. This is optional. By default, the
> > > >  	register is not modified as recommended by the datasheet.
> > > > +- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
> > > > +	optional, but defining it is necessary to get the backlight working. If that
> > > > +	property is ommited, the register is zeroed.
> > > 
> > > Why isn't this implemented as a backlight driver? Static devicetree
> > > provided values is very limiting.
> > 
> > Let's understand the terminology.
> > This register should be renamed according to the datasheet, i.e. LPCCR.
> > As I pointed out earlier, it is NOT control the backlight, this is a contrast control.
> > Yes, it works as PWM, but nothing do with the backlight subsystem.
> > Yes, we can make a driver for this PWM, but how are we going to control it?
> > I misunderstood something?
> 
> I stumbled upon 'get the backlight working' which implied for me that it
> should be a backlight driver. But you're right and now I remember we
> talked about this already.

Hallelujah.

> I still think this should be something adjustable, not static data.
> Maybe we could change the wording to something like "This property
> provides the default value for the contrast control register" since even
> if we add driver support for controlling the contrast we still want
> to have a sane default.

Sounds good.

> BTW the contrast could be controlled with a lcd_device (see
> lcd_device_register) which seems to be very easy to implement.

Address of register is placed within LCD area, so we cannot use this
memory region, I think is no so easy as you say....

---
Sascha Hauer - Dec. 6, 2013, 9:30 a.m.
On Fri, Dec 06, 2013 at 12:35:10PM +0400, Alexander Shiyan wrote:
> > On Fri, Dec 06, 2013 at 12:03:54PM +0400, Alexander Shiyan wrote:
> ...
> > > > >  - fsl,dmacr: DMA Control Register value. This is optional. By default, the
> > > > >  	register is not modified as recommended by the datasheet.
> > > > > +- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
> > > > > +	optional, but defining it is necessary to get the backlight working. If that
> > > > > +	property is ommited, the register is zeroed.
> > > > 
> > > > Why isn't this implemented as a backlight driver? Static devicetree
> > > > provided values is very limiting.
> > > 
> > > Let's understand the terminology.
> > > This register should be renamed according to the datasheet, i.e. LPCCR.
> > > As I pointed out earlier, it is NOT control the backlight, this is a contrast control.
> > > Yes, it works as PWM, but nothing do with the backlight subsystem.
> > > Yes, we can make a driver for this PWM, but how are we going to control it?
> > > I misunderstood something?
> > 
> > I stumbled upon 'get the backlight working' which implied for me that it
> > should be a backlight driver. But you're right and now I remember we
> > talked about this already.
> 
> Hallelujah.
> 
> > I still think this should be something adjustable, not static data.
> > Maybe we could change the wording to something like "This property
> > provides the default value for the contrast control register" since even
> > if we add driver support for controlling the contrast we still want
> > to have a sane default.
> 
> Sounds good.
> 
> > BTW the contrast could be controlled with a lcd_device (see
> > lcd_device_register) which seems to be very easy to implement.
> 
> Address of register is placed within LCD area, so we cannot use this
> memory region, I think is no so easy as you say....

We do not need a separate driver for this. Look for example at
drivers/video/bf537-lq035.c, it just calls lcd_device_register()
in its probe function.

Sascha
Alexander Shiyan - Dec. 6, 2013, 9:40 a.m.
> > > On Fri, Dec 06, 2013 at 12:03:54PM +0400, Alexander Shiyan wrote:
> > ...
> > > > > >  - fsl,dmacr: DMA Control Register value. This is optional. By default, the
> > > > > >  	register is not modified as recommended by the datasheet.
> > > > > > +- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
> > > > > > +	optional, but defining it is necessary to get the backlight working. If that
> > > > > > +	property is ommited, the register is zeroed.
> > > > > 
> > > > > Why isn't this implemented as a backlight driver? Static devicetree
> > > > > provided values is very limiting.
> > > > 
> > > > Let's understand the terminology.
> > > > This register should be renamed according to the datasheet, i.e. LPCCR.
> > > > As I pointed out earlier, it is NOT control the backlight, this is a contrast control.
> > > > Yes, it works as PWM, but nothing do with the backlight subsystem.
> > > > Yes, we can make a driver for this PWM, but how are we going to control it?
> > > > I misunderstood something?
> > > 
> > > I stumbled upon 'get the backlight working' which implied for me that it
> > > should be a backlight driver. But you're right and now I remember we
> > > talked about this already.
> > 
> > Hallelujah.
> > 
> > > I still think this should be something adjustable, not static data.
> > > Maybe we could change the wording to something like "This property
> > > provides the default value for the contrast control register" since even
> > > if we add driver support for controlling the contrast we still want
> > > to have a sane default.
> > 
> > Sounds good.
> > 
> > > BTW the contrast could be controlled with a lcd_device (see
> > > lcd_device_register) which seems to be very easy to implement.
> > 
> > Address of register is placed within LCD area, so we cannot use this
> > memory region, I think is no so easy as you say....
> 
> We do not need a separate driver for this. Look for example at
> drivers/video/bf537-lq035.c, it just calls lcd_device_register()
> in its probe function.

Nice. Seems this example even can handle LCD power
regulator from "[1/4] video: imxfb: Introduce regulator support."

---

Patch

diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
index 46da08d..ac457ae 100644
--- a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
+++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
@@ -17,6 +17,9 @@  Required nodes:
 Optional properties:
 - fsl,dmacr: DMA Control Register value. This is optional. By default, the
 	register is not modified as recommended by the datasheet.
+- fsl,pwmr:  LCDC PWM Contrast Control Register value. That property is
+	optional, but defining it is necessary to get the backlight working. If that
+	property is ommited, the register is zeroed.
 - fsl,lscr1: LCDC Sharp Configuration Register value.
 
 Example:
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index 57d3b81..17a46a7 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -835,6 +835,8 @@  static int imxfb_init_fbinfo(struct platform_device *pdev)
 
 		of_property_read_u32(np, "fsl,dmacr", &fbi->dmacr);
 
+		of_property_read_u32(np, "fsl,pwmr", &fbi->pwmr);
+
 		/* These two function pointers could be used by some specific
 		 * platforms. */
 		fbi->lcd_power = NULL;