| Message ID | 1386258219-26437-2-git-send-email-denis@eukrea.com |
|---|---|
| State | New |
| Headers | show |
> 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 ... ---
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
> 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? ---
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
> 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.... ---
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
> > > 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." ---
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;