All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sascha Hauer <s.hauer@pengutronix.de>
To: Liu Ying <liu.y.victor@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
	Zhang Lily-R58066 <r58066@freescale.com>,
	Arnaud Patard <arnaud.patard@rtp-net.org>
Subject: Re: [PATCH 5/9] Add i.MX5 framebuffer driver
Date: Mon, 13 Dec 2010 12:38:30 +0100	[thread overview]
Message-ID: <20101213113830.GE6017@pengutronix.de> (raw)
In-Reply-To: <AANLkTikZowDCK4JPiCyEou1ekRuYA1qGnKrBPT5dXB+8@mail.gmail.com>

On Sun, Dec 12, 2010 at 02:13:40PM +0800, Liu Ying wrote:
> Hello, Sascha,
> 
> I have following comments to this patch:
> 1) Please modify the commit message, as IPUv3 is not embedded in i.MX50 SoC.

ok.

> 2) ADC is not supported yet in the framebuffer driver, so please
> modify this comment:
>    > + * Framebuffer Framebuffer Driver for SDC and ADC.

ok.

> 3) 'ipu_dp_set_window_pos()' is called only once in
> imx_ipu_fb_set_par_overlay(). So, the framebuffer driver doesn't
> support to change the overlay framebuffer position. Need a
> mechanism/interface for users to change the overlay framebuffer
> position.

Yes. The overlay support is currently only present in the driver because
I didn't want to break it in general. There is no generic overlay API in
the kernel and I didn't want to invent the n+1 API. Currently the
possible use cases of the overlay support are not clear to me. Number
one use case would probably be to display video output, but the
corresponding driver would be a v4l2 driver, so in this case we would
only need an in-kernel API.
Anyway, I have not the resources to implement complete overlay support.
So either we keep it like it is and it more has an example character
or we remove it completely from the driver for now.

> 4) Need to make sure the framebuffer on DP-FG is blanked before the
> framebuffer on DP-BG is blanked. Meanwhile, the framebuffer on DP-FG
> should be unblanked after the framebuffer on DP-BG is unblanked
> 5) Need to check the framebuffer on DP-FG doesn't run out of the range
> of the framebuffer on DP-BG.

I tend to remove the overlay support.

> 6) I prefer to find the video mode in modedb first, and if we cannot
> find the video mode in common video mode data base, we can find a
> video mode in custom video mode data base which is defined in platform
> data. In this way, we don't need to export common modefb.

I exported the modedb to be able to show the different modes under
/sys/class/graphics/fb0/modes and to set it with
/sys/class/graphics/fb0/mode. If you want this there is no way around
exporting it. The ioctl interface for mode setting is independent of the
specific modes anyway.

BTW please make comments specific to specific code inline.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

WARNING: multiple messages have this Message-ID (diff)
From: Sascha Hauer <s.hauer@pengutronix.de>
To: linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 5/9] Add i.MX5 framebuffer driver
Date: Mon, 13 Dec 2010 11:38:30 +0000	[thread overview]
Message-ID: <20101213113830.GE6017@pengutronix.de> (raw)
In-Reply-To: <AANLkTikZowDCK4JPiCyEou1ekRuYA1qGnKrBPT5dXB+8@mail.gmail.com>

On Sun, Dec 12, 2010 at 02:13:40PM +0800, Liu Ying wrote:
> Hello, Sascha,
> 
> I have following comments to this patch:
> 1) Please modify the commit message, as IPUv3 is not embedded in i.MX50 SoC.

ok.

> 2) ADC is not supported yet in the framebuffer driver, so please
> modify this comment:
>    > + * Framebuffer Framebuffer Driver for SDC and ADC.

ok.

> 3) 'ipu_dp_set_window_pos()' is called only once in
> imx_ipu_fb_set_par_overlay(). So, the framebuffer driver doesn't
> support to change the overlay framebuffer position. Need a
> mechanism/interface for users to change the overlay framebuffer
> position.

Yes. The overlay support is currently only present in the driver because
I didn't want to break it in general. There is no generic overlay API in
the kernel and I didn't want to invent the n+1 API. Currently the
possible use cases of the overlay support are not clear to me. Number
one use case would probably be to display video output, but the
corresponding driver would be a v4l2 driver, so in this case we would
only need an in-kernel API.
Anyway, I have not the resources to implement complete overlay support.
So either we keep it like it is and it more has an example character
or we remove it completely from the driver for now.

> 4) Need to make sure the framebuffer on DP-FG is blanked before the
> framebuffer on DP-BG is blanked. Meanwhile, the framebuffer on DP-FG
> should be unblanked after the framebuffer on DP-BG is unblanked
> 5) Need to check the framebuffer on DP-FG doesn't run out of the range
> of the framebuffer on DP-BG.

I tend to remove the overlay support.

> 6) I prefer to find the video mode in modedb first, and if we cannot
> find the video mode in common video mode data base, we can find a
> video mode in custom video mode data base which is defined in platform
> data. In this way, we don't need to export common modefb.

I exported the modedb to be able to show the different modes under
/sys/class/graphics/fb0/modes and to set it with
/sys/class/graphics/fb0/mode. If you want this there is no way around
exporting it. The ioctl interface for mode setting is independent of the
specific modes anyway.

BTW please make comments specific to specific code inline.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

WARNING: multiple messages have this Message-ID (diff)
From: s.hauer@pengutronix.de (Sascha Hauer)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 5/9] Add i.MX5 framebuffer driver
Date: Mon, 13 Dec 2010 12:38:30 +0100	[thread overview]
Message-ID: <20101213113830.GE6017@pengutronix.de> (raw)
In-Reply-To: <AANLkTikZowDCK4JPiCyEou1ekRuYA1qGnKrBPT5dXB+8@mail.gmail.com>

On Sun, Dec 12, 2010 at 02:13:40PM +0800, Liu Ying wrote:
> Hello, Sascha,
> 
> I have following comments to this patch:
> 1) Please modify the commit message, as IPUv3 is not embedded in i.MX50 SoC.

ok.

> 2) ADC is not supported yet in the framebuffer driver, so please
> modify this comment:
>    > + * Framebuffer Framebuffer Driver for SDC and ADC.

ok.

> 3) 'ipu_dp_set_window_pos()' is called only once in
> imx_ipu_fb_set_par_overlay(). So, the framebuffer driver doesn't
> support to change the overlay framebuffer position. Need a
> mechanism/interface for users to change the overlay framebuffer
> position.

Yes. The overlay support is currently only present in the driver because
I didn't want to break it in general. There is no generic overlay API in
the kernel and I didn't want to invent the n+1 API. Currently the
possible use cases of the overlay support are not clear to me. Number
one use case would probably be to display video output, but the
corresponding driver would be a v4l2 driver, so in this case we would
only need an in-kernel API.
Anyway, I have not the resources to implement complete overlay support.
So either we keep it like it is and it more has an example character
or we remove it completely from the driver for now.

> 4) Need to make sure the framebuffer on DP-FG is blanked before the
> framebuffer on DP-BG is blanked. Meanwhile, the framebuffer on DP-FG
> should be unblanked after the framebuffer on DP-BG is unblanked
> 5) Need to check the framebuffer on DP-FG doesn't run out of the range
> of the framebuffer on DP-BG.

I tend to remove the overlay support.

> 6) I prefer to find the video mode in modedb first, and if we cannot
> find the video mode in common video mode data base, we can find a
> video mode in custom video mode data base which is defined in platform
> data. In this way, we don't need to export common modefb.

I exported the modedb to be able to show the different modes under
/sys/class/graphics/fb0/modes and to set it with
/sys/class/graphics/fb0/mode. If you want this there is no way around
exporting it. The ioctl interface for mode setting is independent of the
specific modes anyway.

BTW please make comments specific to specific code inline.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

  parent reply	other threads:[~2010-12-13 11:38 UTC|newest]

Thread overview: 118+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-12-09 13:47 [PATCH RFC] i.MX51 Framebuffer support Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` [PATCH 1/9] ARM i.MX51: Add ipu clock support Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-15 15:40   ` Arnd Bergmann
2010-12-15 15:40     ` Arnd Bergmann
2010-12-15 15:40     ` Arnd Bergmann
2010-12-15 16:34     ` Russell King - ARM Linux
2010-12-15 16:34       ` Russell King - ARM Linux
2010-12-15 16:34       ` Russell King - ARM Linux
2010-12-15 16:49       ` Arnd Bergmann
2010-12-15 16:49         ` Arnd Bergmann
2010-12-15 16:49         ` Arnd Bergmann
2010-12-15 17:12         ` Russell King - ARM Linux
2010-12-15 17:12           ` Russell King - ARM Linux
2010-12-15 17:12           ` Russell King - ARM Linux
2010-12-09 13:47 ` [PATCH 2/9] ARM i.MX51: rename IPU irqs Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-09 14:34   ` Uwe Kleine-König
2010-12-09 14:34     ` Uwe Kleine-König
2010-12-09 14:34     ` 
2010-12-09 13:47 ` [PATCH 3/9] Add a mfd IPUv3 driver Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-12  5:21   ` Liu Ying
2010-12-12  5:21     ` Liu Ying
2010-12-13 11:23     ` Sascha Hauer
2010-12-13 11:23       ` Sascha Hauer
2010-12-13 11:23       ` Sascha Hauer
2010-12-14  4:05       ` Liu Ying
2010-12-14  4:05         ` Liu Ying
2010-12-14  4:05         ` Liu Ying
2010-12-14  8:40         ` Sascha Hauer
2010-12-14  8:40           ` Sascha Hauer
2010-12-14  8:40           ` Sascha Hauer
2010-12-14 13:13           ` Liu Ying
2010-12-14 13:13             ` Liu Ying
2010-12-14 13:13             ` Liu Ying
2010-12-09 13:47 ` [PATCH 4/9] fb: export fb mode db table Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2011-01-06  7:26   ` Paul Mundt
2011-01-06  7:26     ` Paul Mundt
2011-01-06  7:26     ` Paul Mundt
2011-01-06 10:04     ` Sascha Hauer
2011-01-06 10:04       ` Sascha Hauer
2011-01-06 10:04       ` Sascha Hauer
2010-12-09 13:47 ` [PATCH 5/9] Add i.MX5 framebuffer driver Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-12  6:13   ` Liu Ying
2010-12-12  6:13     ` Liu Ying
2010-12-12  6:13     ` Liu Ying
2010-12-13  7:23     ` Lothar Waßmann
2010-12-13  7:23       ` Lothar Waßmann
2010-12-13  7:23       ` Lothar Waßmann
2010-12-13 11:35       ` Liu Ying
2010-12-13 11:35         ` Liu Ying
2010-12-13 11:35         ` Liu Ying
2010-12-13 11:38     ` Sascha Hauer [this message]
2010-12-13 11:38       ` Sascha Hauer
2010-12-13 11:38       ` Sascha Hauer
2010-12-14  6:40       ` Liu Ying
2010-12-14  6:40         ` Liu Ying
2010-12-14  6:40         ` Liu Ying
2010-12-14  8:45         ` Sascha Hauer
2010-12-14  8:45           ` Sascha Hauer
2010-12-14  8:45           ` Sascha Hauer
2010-12-14 13:23           ` Liu Ying
2010-12-14 13:23             ` Liu Ying
2010-12-14 13:23             ` Liu Ying
2010-12-15 11:17             ` Sascha Hauer
2010-12-15 11:17               ` Sascha Hauer
2010-12-15 11:17               ` Sascha Hauer
2010-12-09 13:47 ` [PATCH 6/9] ARM i.MX51: Add IPU device support Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-15 15:49   ` Arnd Bergmann
2010-12-15 15:49     ` Arnd Bergmann
2010-12-15 15:49     ` Arnd Bergmann
2010-12-15 16:26     ` Arnaud Patard
2010-12-15 16:26       ` Arnaud Patard (Rtp)
2010-12-15 16:26       ` Arnaud Patard
2010-12-15 16:29       ` Arnd Bergmann
2010-12-15 16:29         ` Arnd Bergmann
2010-12-15 16:29         ` Arnd Bergmann
2010-12-09 13:47 ` [PATCH 7/9] ARM i.MX5: Allow to increase max zone order Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-09 13:47 ` [PATCH 8/9] ARM i.MX5: increase dma consistent size for IPU support Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-09 13:47 ` [PATCH 9/9] ARM i.MX51 babbage: Add framebuffer support Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-09 13:47   ` Sascha Hauer
2010-12-12  1:37   ` Liu Ying
2010-12-12  1:37     ` Liu Ying
2010-12-12  1:37     ` Liu Ying
2010-12-13 11:43     ` Sascha Hauer
2010-12-13 11:43       ` Sascha Hauer
2010-12-13 11:43       ` Sascha Hauer
2010-12-14  6:47       ` Liu Ying
2010-12-14  6:47         ` Liu Ying
2010-12-14  6:47         ` Liu Ying
2010-12-15  9:35 ` [PATCH RFC] i.MX51 Framebuffer support Peter Horton
2010-12-15 11:24   ` Sascha Hauer
     [not found] <33F32152BE7EC740BC2C838D2836AC8704A957@039-SN1MPN1-002.039d.mgd.msft.net>
2010-12-14 12:38 ` [PATCH 5/9] Add i.MX5 framebuffer driver Chen Jie-B02280
2010-12-14 12:38   ` Chen Jie-B02280
2010-12-15 14:38   ` s.hauer
2010-12-15 14:38     ` s.hauer at pengutronix.de
2010-12-15 14:38     ` s.hauer
2010-12-16  2:07     ` Chen Jie-B02280
2010-12-16  2:07       ` Chen Jie-B02280
2010-12-16  2:07       ` Chen Jie-B02280
2010-12-20 10:48 [PATCH v2] i.MX51 Framebuffer support Sascha Hauer
2010-12-20 10:48 ` [PATCH 5/9] Add i.MX5 framebuffer driver Sascha Hauer
2010-12-20 10:48   ` Sascha Hauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20101213113830.GE6017@pengutronix.de \
    --to=s.hauer@pengutronix.de \
    --cc=arnaud.patard@rtp-net.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=liu.y.victor@gmail.com \
    --cc=r58066@freescale.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.