* [PATCH RESEND 0/8] x86 platform framebuffers
From: David Herrmann @ 2013-08-02 12:05 UTC (permalink / raw)
To: linux-kernel
Cc: David Airlie, Geert Uytterhoeven, Stephen Warren, Peter Jones,
Tomi Valkeinen, Jean-Christophe Plagniol-Villard, H. Peter Anvin,
Ingo Molnar, Thomas Gleixner, x86, linux-fbdev, akpm,
David Herrmann
Hi
I cut down my previous series to no longer include the SimpleDRM driver. If
anyone is interested, you can find it here:
http://lwn.net/Articles/558104/
I will resend it once these preparation patches are in.
Changes since v2:
- added common x86 formats (reported by hpa) (patch #5)
This whole series (including simpledrm) is tested by Stephen and me. I would be
glad if maintainers could ack/nack this so I can continue my work.
This series is pretty small and just converts x86 to use platform-devices
instead of global objects to pass framebuffer data to drivers. The commit
messages explain everything in detail.
The idea is to create a "platform-framebuffer" device which drivers can bind to.
If x86 boot code detectes efi or vesa framebuffers, it creates efi-framebuffer
or vesa-framebuffer devices instead.
Additionally, if the modes are compatible, "simple-framebuffer" devices are
created so simplefb can be used on x86. This feature is only enabled if
CONFIG_X86_SYSFB is selected (off by default) so users without simplefb still
get boot logs.
@Stephen: I wasn't sure whether you tested the efi/vesa framebuffer changes,
too, so I didn't add your tested-by there. And I changed patch #5 so I dropped
it there, too. Thanks for testing!
Thanks
David
David Herrmann (8):
fbdev: simplefb: add init through platform_data
fbdev: simplefb: mark as fw and allocate apertures
x86: provide platform-devices for boot-framebuffers
x86: sysfb: move EFI quirks from efifb to sysfb
fbdev: simplefb: add common x86 RGB formats
fbdev: vesafb: bind to platform-framebuffer device
fbdev: efifb: bind to efi-framebuffer
fbdev: fbcon: select VT_HW_CONSOLE_BINDING
arch/x86/Kconfig | 26 +++
arch/x86/include/asm/sysfb.h | 98 +++++++++++
arch/x86/kernel/Makefile | 3 +
arch/x86/kernel/sysfb.c | 74 ++++++++
arch/x86/kernel/sysfb_efi.c | 214 +++++++++++++++++++++++
arch/x86/kernel/sysfb_simplefb.c | 95 +++++++++++
drivers/video/Kconfig | 5 +-
drivers/video/console/Kconfig | 3 +-
drivers/video/efifb.c | 302 ++++-----------------------------
drivers/video/simplefb.c | 58 +++++--
drivers/video/vesafb.c | 55 ++----
include/linux/platform_data/simplefb.h | 63 +++++++
12 files changed, 666 insertions(+), 330 deletions(-)
create mode 100644 arch/x86/include/asm/sysfb.h
create mode 100644 arch/x86/kernel/sysfb.c
create mode 100644 arch/x86/kernel/sysfb_efi.c
create mode 100644 arch/x86/kernel/sysfb_simplefb.c
create mode 100644 include/linux/platform_data/simplefb.h
--
1.8.3.4
^ permalink raw reply
* Re: [PATCHv3 1/3] video: mxsfb: fix color settings for 18bit data bus and 32bpp
From: Maxime Ripard @ 2013-08-02 9:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51FB6F51.7090500@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]
On Fri, Aug 02, 2013 at 11:35:29AM +0300, Tomi Valkeinen wrote:
> On 01/08/13 11:40, Maxime Ripard wrote:
> > From: Hector Palacios <hector.palacios@digi.com>
> >
> > For a combination of 18bit LCD data bus width and a color
> > mode of 32bpp, the driver was setting the color mapping to
> > rgb666, which is wrong, as the color in memory realy has an
> > rgb888 layout.
> >
> > This patch also removes the setting of flag CTRL_DF24 that
> > makes the driver dimiss the upper 2 bits when handling 32/24bpp
> > colors in a diplay with 18bit data bus width. This flag made
> > true color images display wrong in such configurations.
> >
> > Finally, the color mapping rgb666 has also been removed as nobody
> > is using it and high level applications like Qt5 cannot work
> > with it either.
> >
> > Reference: https://lkml.org/lkml/2013/5/23/220
> > Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> > Acked-by: Juergen Beisert <jbe@pengutronix.de>
> > Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> > drivers/video/mxsfb.c | 26 --------------------------
> > 1 file changed, 26 deletions(-)
>
> This one is labeled as "fix". Should it got to v3.11?
It would be better yes. Otherwise, 3.12 will do.
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCHv3 1/3] video: mxsfb: fix color settings for 18bit data bus and 32bpp
From: Tomi Valkeinen @ 2013-08-02 8:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1375346437-18991-2-git-send-email-maxime.ripard@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]
On 01/08/13 11:40, Maxime Ripard wrote:
> From: Hector Palacios <hector.palacios@digi.com>
>
> For a combination of 18bit LCD data bus width and a color
> mode of 32bpp, the driver was setting the color mapping to
> rgb666, which is wrong, as the color in memory realy has an
> rgb888 layout.
>
> This patch also removes the setting of flag CTRL_DF24 that
> makes the driver dimiss the upper 2 bits when handling 32/24bpp
> colors in a diplay with 18bit data bus width. This flag made
> true color images display wrong in such configurations.
>
> Finally, the color mapping rgb666 has also been removed as nobody
> is using it and high level applications like Qt5 cannot work
> with it either.
>
> Reference: https://lkml.org/lkml/2013/5/23/220
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> Acked-by: Juergen Beisert <jbe@pengutronix.de>
> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> drivers/video/mxsfb.c | 26 --------------------------
> 1 file changed, 26 deletions(-)
This one is labeled as "fix". Should it got to v3.11?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCHv3 3/3] fb: backlight: HX8357: Add HX8369 support
From: Maxime Ripard @ 2013-08-02 8:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51FA56FF.8090308@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]
Hi Tomi,
On Thu, Aug 01, 2013 at 03:39:27PM +0300, Tomi Valkeinen wrote:
> Hi,
>
> On 01/08/13 11:40, Maxime Ripard wrote:
> > From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> >
> > Add support for the Himax HX8369 controller as it is quite similar to the
> > hx8357.
> >
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > Acked-by: Jingoo Han <jg1.han@samsung.com>
> > ---
> > drivers/video/backlight/hx8357.c | 219 ++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 204 insertions(+), 15 deletions(-)
>
> I don't have comments about this patch as such, but hx8357.c in general:
>
> - The commands used look like MIPI DCS commands. We have defined those
> in include/video/mipi_display.h.
Aaah, yes indeed. Thanks for pointing that out.
> - The command arrays could be const
True.
> - I don't like command arrays. Rather than having array for
> "HX8357_SET_COLUMN_ADDRESS, 0x00, 0x00, 0x01, 0x3f,", I'd much more like
> a function hx8357_set_column_address(int x1, int x2) or such.
Hmm, Ok.
> - Looking at the driver makes me think it resembles very much the panel
> driver(s) we have for OMAP. If only CDF was ready... ;)
>
> Those said, I don't see a problem with applying this. We could clean up
> later those things mentioned above, but maybe it's better to do that
> when moving to CDF.
Yes, CDF would be really great for this :)
Alexandre and I will try to find some time to do the cleanups you
requested.
> For this and the two other patches:
>
> Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>
> I can apply these to my 3.12-fbdev branch some day soon, if
> Jean-Christophe hasn't come back yet (I'd normally rather deal only with
> trivial patches, and leave the rest to Jean-Christophe).
Great, thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 18/24] video: da8xx-fb: minimal dt support
From: Tomi Valkeinen @ 2013-08-02 6:40 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-19-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3553 bytes --]
On 01/08/13 23:21, Darren Etheridge wrote:
> Darren Etheridge <detheridge@ti.com> wrote on Thu [2013-Aug-01 08:36:02 -0500]:
>> Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Wed [2013-Jul-31 13:19:01 +0300]:
>>>> +Required properties:
>>>> +- compatible:
>>>> + DA830 - "ti,da830-lcdc"
>>>> + AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
>>>
>>> I'm not totally sure about this, but how I understand the compatible
>>> property, the above reads as: "this device is ti,am3352-lcdc and it's
>>> fully compatible with ti,da830-lcdc". I.e. if the kernel has a driver
>>> for da830-lcdc, it should work with AM335x also (without any of the new
>>> features in AM335x, obviously). Which I believe is not the case, as the
>>> point of this series is to add the AM335x support.
>>>
>>> Or should the current da830-lcdc work with AM335x also, but it just
>>> didn't because there were bugs in da830-lcdc?
>>>
>>> Tomi
>>>
>>>
>> OK I agree there is something wrong here, for one I don't think setting
>> ti,am3352-lcdc would do anything anyway given the driver only reports
>> .compatible with ti,da830-lcdc so at the very least the document is
>> wrong. I will look into this and decide what is the best way of
>> resolving this. I will go ahead and submit the series without the DT
>> support anyway and then I will look into this.
>>
> Reading the device tree documentation I think what is here in the
> patch is actually correct so ignore my first response.
>
> In answer to your original question in theory the da830 driver would
> be fully compatible with the am335x - am335x implements a version 2 of
> the lcd controller that adds some extra reg bits that support larger
> horizontal and vertical resolutions and a sync loss interrupt.
> However the driver takes care of those differences internally. I
> think the main thing that was needed from this patch set to support
> am335x were the dt changes, the Kconfig changes and the clock changes
> - the rest of it are enhancements and bug fixes that are not strictly
> needed for am335x support.
Ok. And the clock changes are also valid in da8xx context also? The need
for them just didn't realize with our current use of da8xx?
> Perhaps the correct thing to do and maybe what you are getting at is
> really am3352 is the superset driver which we have now added complete
> support for with these patches. However because we handle the
> differences between the two IP's directly in the driver we should have
> the driver provide both ti,am3352-lcdc and ti,da830-lcdc as
> .compatible entries because really the driver supports both version 1 and
> version 2 IP's.
Hmm, right, I believe the driver should provide "ti,am3352-lcdc" also,
because it does support the extra features offered in am3352.
A bit side tracking, but: I've been wondering about the device names we
use. Naming the lcdc driver by the SoC is not quite correct. I don't
know all the IPs TI has, but what I see is that TI has two types of
display controllers: the more basic LCDC, and the bigger DSS.
Could we just call the device lcdc or dss? So, for example, DA830 has
"ti,lcdc1", AM3352 has "ti,lcdc2", OMAP2 has "ti,dss2", OMAP4 has
"ti,dss4", etc.
Then again, I'm not sure if the HW development of LCDC or DSS goes quite
that linearly. There are multiple SoCs that have OMAP3's DSS, but are
they the same DSS, or does each of them have a slightly different
version, without there clearly being a DSS version 3.1, 3.2, 3.3, etc...
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH v2 10/24] video: da8xx-fb: fb_set_par support
From: Darren Etheridge @ 2013-08-01 20:30 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-11-git-send-email-detheridge@ti.com>
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Thu [2013-Aug-01 08:44:00 +0300]:
> On 31/07/13 21:56, Etheridge, Darren wrote:
> >>>
> >>> +static int da8xxfb_set_par(struct fb_info *info) {
> >>> + struct da8xx_fb_par *par = info->par;
> >>> + int ret;
> >>> + bool raster = da8xx_fb_is_raster_enabled();
> >>> +
> >>> + if (raster)
> >>> + lcd_disable_raster(true);
> >>> + else
> >>> + lcd_disable_raster(false);
> >>
> >> This looks odd. If raster is enabled, you disable it. And if raster is disabled,
> >> you disable it.
> >
> > I corrected this one in patch 0011 - I agree this code is very confusing.
>
> In patch 11 you add the enum. I wasn't referring to that. My point was
> that even if raster is already disabled,
> lcd_disable_raster(dont-wait-for-framedone) is called.
Agreed and removed, the lcd_disable_raster function does the same
check as da8xx_fb_is_raster_enabled() and the immediately exits if not
enabled so this path of the conditional appears completely redundant.
Darren
^ permalink raw reply
* Re: [PATCH v2 18/24] video: da8xx-fb: minimal dt support
From: Darren Etheridge @ 2013-08-01 20:21 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-19-git-send-email-detheridge@ti.com>
Darren Etheridge <detheridge@ti.com> wrote on Thu [2013-Aug-01 08:36:02 -0500]:
> Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Wed [2013-Jul-31 13:19:01 +0300]:
> > > +Required properties:
> > > +- compatible:
> > > + DA830 - "ti,da830-lcdc"
> > > + AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
> >
> > I'm not totally sure about this, but how I understand the compatible
> > property, the above reads as: "this device is ti,am3352-lcdc and it's
> > fully compatible with ti,da830-lcdc". I.e. if the kernel has a driver
> > for da830-lcdc, it should work with AM335x also (without any of the new
> > features in AM335x, obviously). Which I believe is not the case, as the
> > point of this series is to add the AM335x support.
> >
> > Or should the current da830-lcdc work with AM335x also, but it just
> > didn't because there were bugs in da830-lcdc?
> >
> > Tomi
> >
> >
> OK I agree there is something wrong here, for one I don't think setting
> ti,am3352-lcdc would do anything anyway given the driver only reports
> .compatible with ti,da830-lcdc so at the very least the document is
> wrong. I will look into this and decide what is the best way of
> resolving this. I will go ahead and submit the series without the DT
> support anyway and then I will look into this.
>
Reading the device tree documentation I think what is here in the
patch is actually correct so ignore my first response.
In answer to your original question in theory the da830 driver would
be fully compatible with the am335x - am335x implements a version 2 of
the lcd controller that adds some extra reg bits that support larger
horizontal and vertical resolutions and a sync loss interrupt.
However the driver takes care of those differences internally. I
think the main thing that was needed from this patch set to support
am335x were the dt changes, the Kconfig changes and the clock changes
- the rest of it are enhancements and bug fixes that are not strictly
needed for am335x support.
Perhaps the correct thing to do and maybe what you are getting at is
really am3352 is the superset driver which we have now added complete
support for with these patches. However because we handle the
differences between the two IP's directly in the driver we should have
the driver provide both ti,am3352-lcdc and ti,da830-lcdc as
.compatible entries because really the driver supports both version 1 and
version 2 IP's.
^ permalink raw reply
* Re: [PATCH v2 00/24] video/da8xx-fb fbdev driver enhance to support TI am335x SoC
From: Darren Etheridge @ 2013-08-01 14:06 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-1-git-send-email-detheridge@ti.com>
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Wed [2013-Jul-31 13:04:33 +0300]:
> Hi,
>
> On 30/07/13 21:26, Darren Etheridge wrote:
> > Changes in v2:
> > Addressing review comments from Tomi Valkeinen:
> > Dropped readl/writel patch
> > Many cosmetic changes to make code easier to understand
> >
> >
> > This is primarily a resend of a series of patches that were original
> > submitted to linux-fbdev back in January of 2013 for 3.8 by Afzal
> > Mohammed. I have rebased them on 3.10 and also made sure they
> > apply cleanly to the 'for-next' branch of linux-fbdev git.
> > The patches enable use of the current mainline da8xx-fb driver on the
> > TI AM335x SOC along with some bug fixes and cleanup.
> >
> > The original patch series can be found here:
> > https://patchwork.kernel.org/project/linux-fbdev/list/?submitter9101
> > if you want to see the history.
>
> Comments on the whole series:
>
> Most of the patches are originally from Afzal. I believe some of the
> patches are unchanged, but some are changed by you. In cases like this
> you should pick one of the following options for each patch:
>
> - If the patch is unchanged, send the patch as it is, having From: Afzal
> line there.
>
> - If you have changed the patch, send the patch having From: Afzal line,
> but marking in the description that you've changed it (and what you
> did). This should be done if the changes are small.
>
> - If you changed a lot in the patch, send the patch with yourself as the
> author, signed off by only you, but mention that it's based on Afzal's work.
> The point here is that if you change the patch, it's no longer Afzal's
> original patch. Afzal hasn't reviewed it, so signed-off-by Afzal is not
> correct. You could've introduced horrible bugs in the patch, and I'm
> sure Afzal doesn't want to see that a patch in the kernel introducing
> horrible bugs is from him (when it is not from him).
Understood, and I have made the changes accordingly in the updated
series I am preparing.
> Another thing are the DT related patches. They should be sent to
> devicetree@vger.kernel.org for review. And I think the DT patches should
> be squashed into one, as they are quite short and having them as a whole
> makes it easier to look at them. You could probably move the DT patches
> to a separate series, so that we can merge the rest of the improvements,
> and manage DT separately.
This is no problem to do, I have extracted the DT patches from the
series and will submit them separately and include the devicetree list.
Thanks for taking the time to review so thoroughly.
Darren
^ permalink raw reply
* Re: [PATCH v2 10/24] video: da8xx-fb: fb_set_par support
From: Darren Etheridge @ 2013-08-01 13:54 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-11-git-send-email-detheridge@ti.com>
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Wed [2013-Jul-31 14:38:52 +0300]:
> >
> > +static inline bool da8xx_fb_is_raster_enabled(void)
> > +{
> > + return !!(lcdc_read(LCD_RASTER_CTRL_REG) & LCD_RASTER_ENABLE);
> > +}
>
> See Documentation/CodingStyle about inline.
>
> I think, generally, it's better not to use inline at all in normal
> functions. Let the compiler decide. Even more so with funcs like
> da8xx_fb_is_raster_enabled(), which I guess is only used rarely.
>
> There are some inlines added in other patches in the series also.
>
I have added a new patch to the update series that removes the use of
inline from all offending places.
Darren
^ permalink raw reply
* Re: [PATCH v2 18/24] video: da8xx-fb: minimal dt support
From: Darren Etheridge @ 2013-08-01 13:36 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-19-git-send-email-detheridge@ti.com>
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Wed [2013-Jul-31 13:19:01 +0300]:
> > +Required properties:
> > +- compatible:
> > + DA830 - "ti,da830-lcdc"
> > + AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
>
> I'm not totally sure about this, but how I understand the compatible
> property, the above reads as: "this device is ti,am3352-lcdc and it's
> fully compatible with ti,da830-lcdc". I.e. if the kernel has a driver
> for da830-lcdc, it should work with AM335x also (without any of the new
> features in AM335x, obviously). Which I believe is not the case, as the
> point of this series is to add the AM335x support.
>
> Or should the current da830-lcdc work with AM335x also, but it just
> didn't because there were bugs in da830-lcdc?
>
> Tomi
>
>
OK I agree there is something wrong here, for one I don't think setting
ti,am3352-lcdc would do anything anyway given the driver only reports
.compatible with ti,da830-lcdc so at the very least the document is
wrong. I will look into this and decide what is the best way of
resolving this. I will go ahead and submit the series without the DT
support anyway and then I will look into this.
Darren
^ permalink raw reply
* Re: [PATCHv3 3/3] fb: backlight: HX8357: Add HX8369 support
From: Tomi Valkeinen @ 2013-08-01 12:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1375346437-18991-4-git-send-email-maxime.ripard@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]
Hi,
On 01/08/13 11:40, Maxime Ripard wrote:
> From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>
> Add support for the Himax HX8369 controller as it is quite similar to the
> hx8357.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> ---
> drivers/video/backlight/hx8357.c | 219 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 204 insertions(+), 15 deletions(-)
I don't have comments about this patch as such, but hx8357.c in general:
- The commands used look like MIPI DCS commands. We have defined those
in include/video/mipi_display.h.
- The command arrays could be const
- I don't like command arrays. Rather than having array for
"HX8357_SET_COLUMN_ADDRESS, 0x00, 0x00, 0x01, 0x3f,", I'd much more like
a function hx8357_set_column_address(int x1, int x2) or such.
- Looking at the driver makes me think it resembles very much the panel
driver(s) we have for OMAP. If only CDF was ready... ;)
Those said, I don't see a problem with applying this. We could clean up
later those things mentioned above, but maybe it's better to do that
when moving to CDF.
For this and the two other patches:
Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
I can apply these to my 3.12-fbdev branch some day soon, if
Jean-Christophe hasn't come back yet (I'd normally rather deal only with
trivial patches, and leave the rest to Jean-Christophe).
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [patch -next] fb: fix recent breakage in correct_chipset()
From: Dan Carpenter @ 2013-08-01 11:50 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20130702062821.GC24410@elgon.mountain>
On Thu, Aug 01, 2013 at 11:31:07AM +0200, Geert Uytterhoeven wrote:
> On Tue, Jul 2, 2013 at 8:28 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > The 6e36308a6f "fb: fix atyfb build warning" isn't right. It makes all
> > the indexes off by one. This patch reverts it and casts the
> > ARRAY_SIZE() to int to silence the build warning.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/video/aty/atyfb_base.c b/drivers/video/aty/atyfb_base.c
> > index a89c15d..9b0f12c 100644
> > --- a/drivers/video/aty/atyfb_base.c
> > +++ b/drivers/video/aty/atyfb_base.c
> > @@ -435,8 +435,8 @@ static int correct_chipset(struct atyfb_par *par)
> > const char *name;
> > int i;
> >
> > - for (i = ARRAY_SIZE(aty_chips); i > 0; i--)
> > - if (par->pci_id = aty_chips[i - 1].pci_id)
> > + for (i = (int)ARRAY_SIZE(aty_chips) - 1; i >= 0; i--)
> > + if (par->pci_id = aty_chips[i].pci_id)
> > break;
> >
> > if (i < 0)
>
> Sorry for chiming in late, but can't we just revert the order of the loop
> iteration and change i from int to size_t instead of adding a cast?
>
Nope. That would be a nearly-forever loop.
regards,
dan carpenter
^ permalink raw reply
* RE: [PATCH v2 10/24] video: da8xx-fb: fb_set_par support
From: Etheridge, Darren @ 2013-08-01 11:12 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-11-git-send-email-detheridge@ti.com>
>>> +static int da8xxfb_set_par(struct fb_info *info) {
>>> + struct da8xx_fb_par *par = info->par;
>>> + int ret;
>>> + bool raster = da8xx_fb_is_raster_enabled();
>>> +
>>> + if (raster)
>>> + lcd_disable_raster(true);
>>> + else
>>> + lcd_disable_raster(false);
>>
>> This looks odd. If raster is enabled, you disable it. And if raster is disabled,
>> you disable it.
>
> I corrected this one in patch 0011 - I agree this code is very confusing.
>
>In patch 11 you add the enum. I wasn't referring to that. My point was
>that even if raster is already disabled,
>lcd_disable_raster(dont-wait-for-framedone) is called.
Oh I see, yes you are absolutely right - I will check into what the intent of this code was and see if it is even necessary.
^ permalink raw reply
* Re: [patch -next] fb: fix recent breakage in correct_chipset()
From: Geert Uytterhoeven @ 2013-08-01 9:31 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20130702062821.GC24410@elgon.mountain>
On Tue, Jul 2, 2013 at 8:28 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> The 6e36308a6f "fb: fix atyfb build warning" isn't right. It makes all
> the indexes off by one. This patch reverts it and casts the
> ARRAY_SIZE() to int to silence the build warning.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/video/aty/atyfb_base.c b/drivers/video/aty/atyfb_base.c
> index a89c15d..9b0f12c 100644
> --- a/drivers/video/aty/atyfb_base.c
> +++ b/drivers/video/aty/atyfb_base.c
> @@ -435,8 +435,8 @@ static int correct_chipset(struct atyfb_par *par)
> const char *name;
> int i;
>
> - for (i = ARRAY_SIZE(aty_chips); i > 0; i--)
> - if (par->pci_id = aty_chips[i - 1].pci_id)
> + for (i = (int)ARRAY_SIZE(aty_chips) - 1; i >= 0; i--)
> + if (par->pci_id = aty_chips[i].pci_id)
> break;
>
> if (i < 0)
Sorry for chiming in late, but can't we just revert the order of the loop
iteration and change i from int to size_t instead of adding a cast?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCHv3 3/3] fb: backlight: HX8357: Add HX8369 support
From: Maxime Ripard @ 2013-08-01 8:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1375346437-18991-1-git-send-email-maxime.ripard@free-electrons.com>
From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Add support for the Himax HX8369 controller as it is quite similar to the
hx8357.
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/backlight/hx8357.c | 219 ++++++++++++++++++++++++++++++++++++---
1 file changed, 204 insertions(+), 15 deletions(-)
diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index 47237fa..c7af8c4 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -71,6 +71,18 @@
#define HX8357_SET_POWER_NORMAL 0xd2
#define HX8357_SET_PANEL_RELATED 0xe9
+#define HX8369_SET_DISPLAY_BRIGHTNESS 0x51
+#define HX8369_WRITE_CABC_DISPLAY_VALUE 0x53
+#define HX8369_WRITE_CABC_BRIGHT_CTRL 0x55
+#define HX8369_WRITE_CABC_MIN_BRIGHTNESS 0x5e
+#define HX8369_SET_POWER 0xb1
+#define HX8369_SET_DISPLAY_MODE 0xb2
+#define HX8369_SET_DISPLAY_WAVEFORM_CYC 0xb4
+#define HX8369_SET_VCOM 0xb6
+#define HX8369_SET_EXTENSION_COMMAND 0xb9
+#define HX8369_SET_GIP 0xd5
+#define HX8369_SET_GAMMA_CURVE_RELATED 0xe0
+
struct hx8357_data {
unsigned im_pins[HX8357_NUM_IM_PINS];
unsigned reset;
@@ -144,6 +156,61 @@ static u8 hx8357_seq_display_mode[] = {
HX8357_SET_DISPLAY_MODE_RGB_INTERFACE,
};
+static u8 hx8369_seq_write_CABC_min_brightness[] = {
+ HX8369_WRITE_CABC_MIN_BRIGHTNESS, 0x00,
+};
+
+static u8 hx8369_seq_write_CABC_control[] = {
+ HX8369_WRITE_CABC_DISPLAY_VALUE, 0x24,
+};
+
+static u8 hx8369_seq_set_display_brightness[] = {
+ HX8369_SET_DISPLAY_BRIGHTNESS, 0xFF,
+};
+
+static u8 hx8369_seq_write_CABC_control_setting[] = {
+ HX8369_WRITE_CABC_BRIGHT_CTRL, 0x02,
+};
+
+static u8 hx8369_seq_extension_command[] = {
+ HX8369_SET_EXTENSION_COMMAND, 0xff, 0x83, 0x69,
+};
+
+static u8 hx8369_seq_display_related[] = {
+ HX8369_SET_DISPLAY_MODE, 0x00, 0x2b, 0x03, 0x03, 0x70, 0x00,
+ 0xff, 0x00, 0x00, 0x00, 0x00, 0x03, 0x03, 0x00, 0x01,
+};
+
+static u8 hx8369_seq_panel_waveform_cycle[] = {
+ HX8369_SET_DISPLAY_WAVEFORM_CYC, 0x0a, 0x1d, 0x80, 0x06, 0x02,
+};
+
+static u8 hx8369_seq_set_address_mode[] = {
+ HX8357_SET_ADDRESS_MODE, 0x00,
+};
+
+static u8 hx8369_seq_vcom[] = {
+ HX8369_SET_VCOM, 0x3e, 0x3e,
+};
+
+static u8 hx8369_seq_gip[] = {
+ HX8369_SET_GIP, 0x00, 0x01, 0x03, 0x25, 0x01, 0x02, 0x28, 0x70,
+ 0x11, 0x13, 0x00, 0x00, 0x40, 0x26, 0x51, 0x37, 0x00, 0x00, 0x71,
+ 0x35, 0x60, 0x24, 0x07, 0x0f, 0x04, 0x04,
+};
+
+static u8 hx8369_seq_power[] = {
+ HX8369_SET_POWER, 0x01, 0x00, 0x34, 0x03, 0x00, 0x11, 0x11, 0x32,
+ 0x2f, 0x3f, 0x3f, 0x01, 0x3a, 0x01, 0xe6, 0xe6, 0xe6, 0xe6, 0xe6,
+};
+
+static u8 hx8369_seq_gamma_curve_related[] = {
+ HX8369_SET_GAMMA_CURVE_RELATED, 0x00, 0x0d, 0x19, 0x2f, 0x3b, 0x3d,
+ 0x2e, 0x4a, 0x08, 0x0e, 0x0f, 0x14, 0x16, 0x14, 0x14, 0x14, 0x1e,
+ 0x00, 0x0d, 0x19, 0x2f, 0x3b, 0x3d, 0x2e, 0x4a, 0x08, 0x0e, 0x0f,
+ 0x14, 0x16, 0x14, 0x14, 0x14, 0x1e,
+};
+
static int hx8357_spi_write_then_read(struct lcd_device *lcdev,
u8 *txbuf, u16 txlen,
u8 *rxbuf, u16 rxlen)
@@ -220,6 +287,10 @@ static int hx8357_enter_standby(struct lcd_device *lcdev)
if (ret < 0)
return ret;
+ /*
+ * The controller needs 120ms when entering in sleep mode before we can
+ * send the command to go off sleep mode
+ */
msleep(120);
return 0;
@@ -233,6 +304,10 @@ static int hx8357_exit_standby(struct lcd_device *lcdev)
if (ret < 0)
return ret;
+ /*
+ * The controller needs 120ms when exiting from sleep mode before we
+ * can send the command to enter in sleep mode
+ */
msleep(120);
ret = hx8357_spi_write_byte(lcdev, HX8357_SET_DISPLAY_ON);
@@ -242,6 +317,21 @@ static int hx8357_exit_standby(struct lcd_device *lcdev)
return 0;
}
+static void hx8357_lcd_reset(struct lcd_device *lcdev)
+{
+ struct hx8357_data *lcd = lcd_get_data(lcdev);
+
+ /* Reset the screen */
+ gpio_set_value(lcd->reset, 1);
+ usleep_range(10000, 12000);
+ gpio_set_value(lcd->reset, 0);
+ usleep_range(10000, 12000);
+ gpio_set_value(lcd->reset, 1);
+
+ /* The controller needs 120ms to recover from reset */
+ msleep(120);
+}
+
static int hx8357_lcd_init(struct lcd_device *lcdev)
{
struct hx8357_data *lcd = lcd_get_data(lcdev);
@@ -257,14 +347,6 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
gpio_set_value_cansleep(lcd->im_pins[2], 1);
}
- /* Reset the screen */
- gpio_set_value(lcd->reset, 1);
- usleep_range(10000, 12000);
- gpio_set_value(lcd->reset, 0);
- usleep_range(10000, 12000);
- gpio_set_value(lcd->reset, 1);
- msleep(120);
-
ret = hx8357_spi_write_array(lcdev, hx8357_seq_power,
ARRAY_SIZE(hx8357_seq_power));
if (ret < 0)
@@ -344,6 +426,9 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
if (ret < 0)
return ret;
+ /*
+ * The controller needs 120ms to fully recover from exiting sleep mode
+ */
msleep(120);
ret = hx8357_spi_write_byte(lcdev, HX8357_SET_DISPLAY_ON);
@@ -359,6 +444,96 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
return 0;
}
+static int hx8369_lcd_init(struct lcd_device *lcdev)
+{
+ int ret;
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_extension_command,
+ ARRAY_SIZE(hx8369_seq_extension_command));
+ if (ret < 0)
+ return ret;
+ usleep_range(10000, 12000);
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_display_related,
+ ARRAY_SIZE(hx8369_seq_display_related));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_panel_waveform_cycle,
+ ARRAY_SIZE(hx8369_seq_panel_waveform_cycle));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_set_address_mode,
+ ARRAY_SIZE(hx8369_seq_set_address_mode));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_vcom,
+ ARRAY_SIZE(hx8369_seq_vcom));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_gip,
+ ARRAY_SIZE(hx8369_seq_gip));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_power,
+ ARRAY_SIZE(hx8369_seq_power));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_byte(lcdev, HX8357_EXIT_SLEEP_MODE);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * The controller needs 120ms to fully recover from exiting sleep mode
+ */
+ msleep(120);
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_gamma_curve_related,
+ ARRAY_SIZE(hx8369_seq_gamma_curve_related));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_byte(lcdev, HX8357_EXIT_SLEEP_MODE);
+ if (ret < 0)
+ return ret;
+ usleep_range(1000, 1200);
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_write_CABC_control,
+ ARRAY_SIZE(hx8369_seq_write_CABC_control));
+ if (ret < 0)
+ return ret;
+ usleep_range(10000, 12000);
+
+ ret = hx8357_spi_write_array(lcdev,
+ hx8369_seq_write_CABC_control_setting,
+ ARRAY_SIZE(hx8369_seq_write_CABC_control_setting));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_array(lcdev,
+ hx8369_seq_write_CABC_min_brightness,
+ ARRAY_SIZE(hx8369_seq_write_CABC_min_brightness));
+ if (ret < 0)
+ return ret;
+ usleep_range(10000, 12000);
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_set_display_brightness,
+ ARRAY_SIZE(hx8369_seq_set_display_brightness));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_byte(lcdev, HX8357_SET_DISPLAY_ON);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
#define POWER_IS_ON(pwr) ((pwr) <= FB_BLANK_NORMAL)
static int hx8357_set_power(struct lcd_device *lcdev, int power)
@@ -391,10 +566,24 @@ static struct lcd_ops hx8357_ops = {
.get_power = hx8357_get_power,
};
+static const struct of_device_id hx8357_dt_ids[] = {
+ {
+ .compatible = "himax,hx8357",
+ .data = hx8357_lcd_init,
+ },
+ {
+ .compatible = "himax,hx8369",
+ .data = hx8369_lcd_init,
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
+
static int hx8357_probe(struct spi_device *spi)
{
struct lcd_device *lcdev;
struct hx8357_data *lcd;
+ const struct of_device_id *match;
int i, ret;
lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL);
@@ -411,6 +600,10 @@ static int hx8357_probe(struct spi_device *spi)
lcd->spi = spi;
+ match = of_match_device(hx8357_dt_ids, &spi->dev);
+ if (!match || !match->data)
+ return -EINVAL;
+
lcd->reset = of_get_named_gpio(spi->dev.of_node, "gpios-reset", 0);
if (!gpio_is_valid(lcd->reset)) {
dev_err(&spi->dev, "Missing dt property: gpios-reset\n");
@@ -462,7 +655,9 @@ static int hx8357_probe(struct spi_device *spi)
}
spi_set_drvdata(spi, lcdev);
- ret = hx8357_lcd_init(lcdev);
+ hx8357_lcd_reset(lcdev);
+
+ ret = ((int (*)(struct lcd_device *))match->data)(lcdev);
if (ret) {
dev_err(&spi->dev, "Couldn't initialize panel\n");
goto init_error;
@@ -485,12 +680,6 @@ static int hx8357_remove(struct spi_device *spi)
return 0;
}
-static const struct of_device_id hx8357_dt_ids[] = {
- { .compatible = "himax,hx8357" },
- {},
-};
-MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
-
static struct spi_driver hx8357_driver = {
.probe = hx8357_probe,
.remove = hx8357_remove,
--
1.8.3.4
^ permalink raw reply related
* [PATCHv3 2/3] video: hx8357: Make IM pins optional
From: Maxime Ripard @ 2013-08-01 8:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1375346437-18991-1-git-send-email-maxime.ripard@free-electrons.com>
The IM pins of the HX8357 controller are used to define the interface
used to feed pixel stream to the LCD panel.
Most of the time, these pins are directly routed to either the ground or
the VCC to set their values.
Remove the need to assign GPIOs to these pins when we are in such a case.
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/backlight/hx8357.c | 52 ++++++++++++++++++++++++----------------
1 file changed, 31 insertions(+), 21 deletions(-)
diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index a0482b5..47237fa 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -76,6 +76,7 @@ struct hx8357_data {
unsigned reset;
struct spi_device *spi;
int state;
+ bool use_im_pins;
};
static u8 hx8357_seq_power[] = {
@@ -250,9 +251,11 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
* Set the interface selection pins to SPI mode, with three
* wires
*/
- gpio_set_value_cansleep(lcd->im_pins[0], 1);
- gpio_set_value_cansleep(lcd->im_pins[1], 0);
- gpio_set_value_cansleep(lcd->im_pins[2], 1);
+ if (lcd->use_im_pins) {
+ gpio_set_value_cansleep(lcd->im_pins[0], 1);
+ gpio_set_value_cansleep(lcd->im_pins[1], 0);
+ gpio_set_value_cansleep(lcd->im_pins[2], 1);
+ }
/* Reset the screen */
gpio_set_value(lcd->reset, 1);
@@ -424,25 +427,32 @@ static int hx8357_probe(struct spi_device *spi)
return -EINVAL;
}
- for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
- lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
- "im-gpios", i);
- if (lcd->im_pins[i] = -EPROBE_DEFER) {
- dev_info(&spi->dev, "GPIO requested is not here yet, deferring the probe\n");
- return -EPROBE_DEFER;
- }
- if (!gpio_is_valid(lcd->im_pins[i])) {
- dev_err(&spi->dev, "Missing dt property: im-gpios\n");
- return -EINVAL;
- }
-
- ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
- GPIOF_OUT_INIT_LOW, "im_pins");
- if (ret) {
- dev_err(&spi->dev, "failed to request gpio %d: %d\n",
- lcd->im_pins[i], ret);
- return -EINVAL;
+ if (of_find_property(spi->dev.of_node, "im-gpios", NULL)) {
+ lcd->use_im_pins = 1;
+
+ for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
+ lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
+ "im-gpios", i);
+ if (lcd->im_pins[i] = -EPROBE_DEFER) {
+ dev_info(&spi->dev, "GPIO requested is not here yet, deferring the probe\n");
+ return -EPROBE_DEFER;
+ }
+ if (!gpio_is_valid(lcd->im_pins[i])) {
+ dev_err(&spi->dev, "Missing dt property: im-gpios\n");
+ return -EINVAL;
+ }
+
+ ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
+ GPIOF_OUT_INIT_LOW,
+ "im_pins");
+ if (ret) {
+ dev_err(&spi->dev, "failed to request gpio %d: %d\n",
+ lcd->im_pins[i], ret);
+ return -EINVAL;
+ }
}
+ } else {
+ lcd->use_im_pins = 0;
}
lcdev = lcd_device_register("mxsfb", &spi->dev, lcd, &hx8357_ops);
--
1.8.3.4
^ permalink raw reply related
* [PATCHv3 1/3] video: mxsfb: fix color settings for 18bit data bus and 32bpp
From: Maxime Ripard @ 2013-08-01 8:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1375346437-18991-1-git-send-email-maxime.ripard@free-electrons.com>
From: Hector Palacios <hector.palacios@digi.com>
For a combination of 18bit LCD data bus width and a color
mode of 32bpp, the driver was setting the color mapping to
rgb666, which is wrong, as the color in memory realy has an
rgb888 layout.
This patch also removes the setting of flag CTRL_DF24 that
makes the driver dimiss the upper 2 bits when handling 32/24bpp
colors in a diplay with 18bit data bus width. This flag made
true color images display wrong in such configurations.
Finally, the color mapping rgb666 has also been removed as nobody
is using it and high level applications like Qt5 cannot work
with it either.
Reference: https://lkml.org/lkml/2013/5/23/220
Signed-off-by: Hector Palacios <hector.palacios@digi.com>
Acked-by: Juergen Beisert <jbe@pengutronix.de>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/video/mxsfb.c | 26 --------------------------
1 file changed, 26 deletions(-)
diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
index 3ba3771..dc09ebe 100644
--- a/drivers/video/mxsfb.c
+++ b/drivers/video/mxsfb.c
@@ -239,24 +239,6 @@ static const struct fb_bitfield def_rgb565[] = {
}
};
-static const struct fb_bitfield def_rgb666[] = {
- [RED] = {
- .offset = 16,
- .length = 6,
- },
- [GREEN] = {
- .offset = 8,
- .length = 6,
- },
- [BLUE] = {
- .offset = 0,
- .length = 6,
- },
- [TRANSP] = { /* no support for transparency */
- .length = 0,
- }
-};
-
static const struct fb_bitfield def_rgb888[] = {
[RED] = {
.offset = 16,
@@ -309,9 +291,6 @@ static int mxsfb_check_var(struct fb_var_screeninfo *var,
break;
case STMLCDIF_16BIT:
case STMLCDIF_18BIT:
- /* 24 bit to 18 bit mapping */
- rgb = def_rgb666;
- break;
case STMLCDIF_24BIT:
/* real 24 bit */
rgb = def_rgb888;
@@ -453,11 +432,6 @@ static int mxsfb_set_par(struct fb_info *fb_info)
return -EINVAL;
case STMLCDIF_16BIT:
case STMLCDIF_18BIT:
- /* 24 bit to 18 bit mapping */
- ctrl |= CTRL_DF24; /* ignore the upper 2 bits in
- * each colour component
- */
- break;
case STMLCDIF_24BIT:
/* real 24 bit */
break;
--
1.8.3.4
^ permalink raw reply related
* [PATCHv3 0/3] Few ignored framebuffer fixes/additions
From: Maxime Ripard @ 2013-08-01 8:40 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
This is the v2 of a set of patches that got forgotten.
These have been around for a while without any functionnal comments recently,
so I guess we should be pretty close to merge them.
It would be great if we could have comments from the maintainer on these
patches though (three patches iterations and 41 days since last sign of life of
the maintainer).
Thanks,
Maxime
Changes from v2:
- Few minor coding style fixes
- Removed the useless msleep(100) in the hx8369 init function
Changes from v1:
- Fix line wrapping in patch 2
- Added some comments on the msleep in patch 3
Alexandre Belloni (1):
fb: backlight: HX8357: Add HX8369 support
Hector Palacios (1):
video: mxsfb: fix color settings for 18bit data bus and 32bpp
Maxime Ripard (1):
video: hx8357: Make IM pins optional
drivers/video/backlight/hx8357.c | 269 ++++++++++++++++++++++++++++++++++-----
drivers/video/mxsfb.c | 26 ----
2 files changed, 234 insertions(+), 61 deletions(-)
--
1.8.3.4
^ permalink raw reply
* Re: [PATCH v2 10/24] video: da8xx-fb: fb_set_par support
From: Tomi Valkeinen @ 2013-08-01 5:44 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-11-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 701 bytes --]
On 31/07/13 21:56, Etheridge, Darren wrote:
>>>
>>> +static int da8xxfb_set_par(struct fb_info *info) {
>>> + struct da8xx_fb_par *par = info->par;
>>> + int ret;
>>> + bool raster = da8xx_fb_is_raster_enabled();
>>> +
>>> + if (raster)
>>> + lcd_disable_raster(true);
>>> + else
>>> + lcd_disable_raster(false);
>>
>> This looks odd. If raster is enabled, you disable it. And if raster is disabled,
>> you disable it.
>
> I corrected this one in patch 0011 - I agree this code is very confusing.
In patch 11 you add the enum. I wasn't referring to that. My point was
that even if raster is already disabled,
lcd_disable_raster(dont-wait-for-framedone) is called.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [RESEND PATCH v10 1/8] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-08-01 1:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374842963-13545-2-git-send-email-kishon@ti.com>
On Fri, Jul 26, 2013 at 06:19:16PM +0530, Kishon Vijay Abraham I wrote:
> +static int phy_get_id(void)
> +{
> + int ret;
> + int id;
> +
> + ret = ida_pre_get(&phy_ida, GFP_KERNEL);
> + if (!ret)
> + return -ENOMEM;
> +
> + ret = ida_get_new(&phy_ida, &id);
> + if (ret < 0)
> + return ret;
> +
> + return id;
> +}
ida_simple_get() instead? And if you do that, you can get rid of this
function entirely.
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH v2 10/24] video: da8xx-fb: fb_set_par support
From: Etheridge, Darren @ 2013-07-31 18:56 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-11-git-send-email-detheridge@ti.com>
> >
> > +static int da8xxfb_set_par(struct fb_info *info) {
> > + struct da8xx_fb_par *par = info->par;
> > + int ret;
> > + bool raster = da8xx_fb_is_raster_enabled();
> > +
> > + if (raster)
> > + lcd_disable_raster(true);
> > + else
> > + lcd_disable_raster(false);
>
> This looks odd. If raster is enabled, you disable it. And if raster is disabled,
> you disable it.
I corrected this one in patch 0011 - I agree this code is very confusing.
^ permalink raw reply
* Re: [PATCHv2 3/3] fb: backlight: HX8357: Add HX8369 support
From: Alexandre Belloni @ 2013-07-31 18:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <002301ce8991$f9b82060$ed286120$@samsung.com>
Hi,
On 26/07/2013 01:52, Jingoo Han wrote:
> On Thursday, July 25, 2013 10:05 PM, Maxime Ripard wrote:
>> From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>
>> Add support for the Himax HX8369 controller as it is quite similar to the
>> hx8357.
>>
>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> Acked-by: Jingoo Han <jg1.han@samsung.com>
>> ---
>> drivers/video/backlight/hx8357.c | 221 ++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 206 insertions(+), 15 deletions(-)
>>
> [....]
>
>> + ret = hx8357_spi_write_byte(lcdev, HX8357_SET_DISPLAY_ON);
>> + if (ret < 0)
>> + return ret;
>> +
>> + msleep(100);
> Hi Maxime Ripard,
>
> Could you add comment on this huge delay, too?
That one can actually be removed.
Regards,
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v2 10/24] video: da8xx-fb: fb_set_par support
From: Tomi Valkeinen @ 2013-07-31 11:38 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-11-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2739 bytes --]
On 30/07/13 21:26, Darren Etheridge wrote:
> From: Afzal Mohammed <afzal@ti.com>
>
> fb_set_par helps in runtime configuration of lcd controller like
> changing resolution, pixel clock etc. (eg. using fbset utility)
>
> Reconfigure lcd controller based on information passed by framework.
> Enable raster back if it was already enabled.
>
> As fb_set_par would get invoked indirectly from probe via fb_set_var,
> remove existing lcdc initialization in probe and do lcdc reset in
> probe so that reset happens only at the begining.
>
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> Signed-off-by: Darren Etheridge <detheridge@ti.com>
> ---
> drivers/video/da8xx-fb.c | 60 +++++++++++++++++++++++++++++++++++++--------
> 1 files changed, 49 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index 8d73730..b25b810 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
> @@ -243,6 +243,11 @@ static struct fb_videomode known_lcd_panels[] = {
> },
> };
>
> +static inline bool da8xx_fb_is_raster_enabled(void)
> +{
> + return !!(lcdc_read(LCD_RASTER_CTRL_REG) & LCD_RASTER_ENABLE);
> +}
See Documentation/CodingStyle about inline.
I think, generally, it's better not to use inline at all in normal
functions. Let the compiler decide. Even more so with funcs like
da8xx_fb_is_raster_enabled(), which I guess is only used rarely.
There are some inlines added in other patches in the series also.
> /* Enable the Raster Engine of the LCD Controller */
> static inline void lcd_enable_raster(void)
> {
> @@ -665,9 +670,6 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
>
> static void da8xx_fb_lcd_reset(void)
> {
> - /* Disable the Raster if previously Enabled */
> - lcd_disable_raster(false);
> -
> /* DMA has to be disabled */
> lcdc_write(0, LCD_DMA_CTRL_REG);
> lcdc_write(0, LCD_RASTER_CTRL_REG);
> @@ -720,8 +722,6 @@ static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
> u32 bpp;
> int ret = 0;
>
> - da8xx_fb_lcd_reset();
> -
> da8xx_fb_calc_config_clk_divider(par, panel);
>
> if (panel->sync & FB_SYNC_CLK_INVERT)
> @@ -1201,9 +1201,52 @@ static int da8xx_pan_display(struct fb_var_screeninfo *var,
> return ret;
> }
>
> +static int da8xxfb_set_par(struct fb_info *info)
> +{
> + struct da8xx_fb_par *par = info->par;
> + int ret;
> + bool raster = da8xx_fb_is_raster_enabled();
> +
> + if (raster)
> + lcd_disable_raster(true);
> + else
> + lcd_disable_raster(false);
This looks odd. If raster is enabled, you disable it. And if raster is
disabled, you disable it.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH v2 18/24] video: da8xx-fb: minimal dt support
From: Tomi Valkeinen @ 2013-07-31 10:19 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-19-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]
On 30/07/13 21:26, Darren Etheridge wrote:
> From: Afzal Mohammed <afzal@ti.com>
>
> Driver is provided a means to have the probe triggered by DT.
>
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> Signed-off-by: Darren Etheridge <detheridge@ti.com>
> ---
> .../devicetree/bindings/video/fb-da8xx.txt | 16 ++++++++++++++++
> drivers/video/da8xx-fb.c | 7 +++++++
> 2 files changed, 23 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt
>
> diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
> new file mode 100644
> index 0000000..581e014
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
> @@ -0,0 +1,16 @@
> +TI LCD Controller on DA830/DA850/AM335x SoC's
> +
> +Required properties:
> +- compatible:
> + DA830 - "ti,da830-lcdc"
> + AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
I'm not totally sure about this, but how I understand the compatible
property, the above reads as: "this device is ti,am3352-lcdc and it's
fully compatible with ti,da830-lcdc". I.e. if the kernel has a driver
for da830-lcdc, it should work with AM335x also (without any of the new
features in AM335x, obviously). Which I believe is not the case, as the
point of this series is to add the AM335x support.
Or should the current da830-lcdc work with AM335x also, but it just
didn't because there were bugs in da830-lcdc?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH v2 00/24] video/da8xx-fb fbdev driver enhance to support TI am335x SoC
From: Tomi Valkeinen @ 2013-07-31 10:04 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-1-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]
Hi,
On 30/07/13 21:26, Darren Etheridge wrote:
> Changes in v2:
> Addressing review comments from Tomi Valkeinen:
> Dropped readl/writel patch
> Many cosmetic changes to make code easier to understand
>
>
> This is primarily a resend of a series of patches that were original
> submitted to linux-fbdev back in January of 2013 for 3.8 by Afzal
> Mohammed. I have rebased them on 3.10 and also made sure they
> apply cleanly to the 'for-next' branch of linux-fbdev git.
> The patches enable use of the current mainline da8xx-fb driver on the
> TI AM335x SOC along with some bug fixes and cleanup.
>
> The original patch series can be found here:
> https://patchwork.kernel.org/project/linux-fbdev/list/?submitter=39101
> if you want to see the history.
Comments on the whole series:
Most of the patches are originally from Afzal. I believe some of the
patches are unchanged, but some are changed by you. In cases like this
you should pick one of the following options for each patch:
- If the patch is unchanged, send the patch as it is, having From: Afzal
line there.
- If you have changed the patch, send the patch having From: Afzal line,
but marking in the description that you've changed it (and what you
did). This should be done if the changes are small.
- If you changed a lot in the patch, send the patch with yourself as the
author, signed off by only you, but mention that it's based on Afzal's work.
The point here is that if you change the patch, it's no longer Afzal's
original patch. Afzal hasn't reviewed it, so signed-off-by Afzal is not
correct. You could've introduced horrible bugs in the patch, and I'm
sure Afzal doesn't want to see that a patch in the kernel introducing
horrible bugs is from him (when it is not from him).
Of course, if you have actively discussed the patches with Afzal, and
he's okay with all the changes you've made, then the patches are fine.
Another thing are the DT related patches. They should be sent to
devicetree@vger.kernel.org for review. And I think the DT patches should
be squashed into one, as they are quite short and having them as a whole
makes it easier to look at them. You could probably move the DT patches
to a separate series, so that we can merge the rest of the improvements,
and manage DT separately.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox