| Message ID | 1459331485-28376-4-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 30, 2016 at 11:51:18AM +0200, Daniel Vetter wrote: > The fb helper private gamma_set/get functions are only required when > the driver supports paletted 8bit mode with fbdev. Armada uses 32bpp > unconditionally, so this is just dead code. It also doesn't do > anything really. Let's just remove it. This comment is misleading: Armada supports 16bpp formats as well as 32bpp, and the hardware does have 8bpp modes to, but I've chosen not to support the 8bpp modes.
On Wed, Mar 30, 2016 at 1:09 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Wed, Mar 30, 2016 at 11:51:18AM +0200, Daniel Vetter wrote: >> The fb helper private gamma_set/get functions are only required when >> the driver supports paletted 8bit mode with fbdev. Armada uses 32bpp >> unconditionally, so this is just dead code. It also doesn't do >> anything really. Let's just remove it. > > This comment is misleading: Armada supports 16bpp formats as well as > 32bpp, and the hardware does have 8bpp modes to, but I've chosen not > to support the 8bpp modes. This is purely about the fbdev emulation (and yeah need to clarify that), not about kms support in general. And these two gamma_set/get hooks are _only_ used by the fbdev emulation, and only needed if you do 8bit paletted mode. Ok if I change the commit message to "Armada used 32bpp unconditionally for fbdev emulation, ..."? -Daniel
On Wed, Mar 30, 2016 at 01:19:21PM +0200, Daniel Vetter wrote: > On Wed, Mar 30, 2016 at 1:09 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Wed, Mar 30, 2016 at 11:51:18AM +0200, Daniel Vetter wrote: > >> The fb helper private gamma_set/get functions are only required when > >> the driver supports paletted 8bit mode with fbdev. Armada uses 32bpp > >> unconditionally, so this is just dead code. It also doesn't do > >> anything really. Let's just remove it. > > > > This comment is misleading: Armada supports 16bpp formats as well as > > 32bpp, and the hardware does have 8bpp modes to, but I've chosen not > > to support the 8bpp modes. > > This is purely about the fbdev emulation (and yeah need to clarify > that), not about kms support in general. And these two gamma_set/get > hooks are _only_ used by the fbdev emulation, and only needed if you > do 8bit paletted mode. Ok if I change the commit message to "Armada > used 32bpp unconditionally for fbdev emulation, ..."? I still don't know where you get that from - the armada fbdev code supports more than just 32bpp. Please explain.
On Wed, Mar 30, 2016 at 01:56:06PM +0100, Russell King - ARM Linux wrote: > On Wed, Mar 30, 2016 at 01:19:21PM +0200, Daniel Vetter wrote: > > On Wed, Mar 30, 2016 at 1:09 PM, Russell King - ARM Linux > > <linux@arm.linux.org.uk> wrote: > > > On Wed, Mar 30, 2016 at 11:51:18AM +0200, Daniel Vetter wrote: > > >> The fb helper private gamma_set/get functions are only required when > > >> the driver supports paletted 8bit mode with fbdev. Armada uses 32bpp > > >> unconditionally, so this is just dead code. It also doesn't do > > >> anything really. Let's just remove it. > > > > > > This comment is misleading: Armada supports 16bpp formats as well as > > > 32bpp, and the hardware does have 8bpp modes to, but I've chosen not > > > to support the 8bpp modes. > > > > This is purely about the fbdev emulation (and yeah need to clarify > > that), not about kms support in general. And these two gamma_set/get > > hooks are _only_ used by the fbdev emulation, and only needed if you > > do 8bit paletted mode. Ok if I change the commit message to "Armada > > used 32bpp unconditionally for fbdev emulation, ..."? > > I still don't know where you get that from - the armada fbdev code > supports more than just 32bpp. Please explain. Ok, I lost myself in the code and missed the cmdline parsing of fbdev depth. So seems indeed possible to create a 8bit fbdev on armada (since armada_framebuffer_create won't reject it, and I didn't see anything else). I'll drop this patch. Thanks for the feedback. -Daniel
diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index 0293eb74d777..f2979655e100 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -317,16 +317,6 @@ static void armada_drm_vblank_off(struct armada_crtc *dcrtc) armada_drm_plane_work_run(dcrtc, plane); } -void armada_drm_crtc_gamma_set(struct drm_crtc *crtc, u16 r, u16 g, u16 b, - int idx) -{ -} - -void armada_drm_crtc_gamma_get(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b, - int idx) -{ -} - /* The mode_config.mutex will be held for this call */ static void armada_drm_crtc_dpms(struct drm_crtc *crtc, int dpms) { diff --git a/drivers/gpu/drm/armada/armada_crtc.h b/drivers/gpu/drm/armada/armada_crtc.h index 04fdd22d483b..6e2770343856 100644 --- a/drivers/gpu/drm/armada/armada_crtc.h +++ b/drivers/gpu/drm/armada/armada_crtc.h @@ -92,8 +92,6 @@ struct armada_crtc { }; #define drm_to_armada_crtc(c) container_of(c, struct armada_crtc, crtc) -void armada_drm_crtc_gamma_set(struct drm_crtc *, u16, u16, u16, int); -void armada_drm_crtc_gamma_get(struct drm_crtc *, u16 *, u16 *, u16 *, int); void armada_drm_crtc_disable_irq(struct armada_crtc *, u32); void armada_drm_crtc_enable_irq(struct armada_crtc *, u32); void armada_drm_crtc_update_regs(struct armada_crtc *, struct armada_regs *); diff --git a/drivers/gpu/drm/armada/armada_fbdev.c b/drivers/gpu/drm/armada/armada_fbdev.c index 7d03c51abcb9..7781bac2ada2 100644 --- a/drivers/gpu/drm/armada/armada_fbdev.c +++ b/drivers/gpu/drm/armada/armada_fbdev.c @@ -124,8 +124,6 @@ static int armada_fb_probe(struct drm_fb_helper *fbh, } static const struct drm_fb_helper_funcs armada_fb_helper_funcs = { - .gamma_set = armada_drm_crtc_gamma_set, - .gamma_get = armada_drm_crtc_gamma_get, .fb_probe = armada_fb_probe, };
The fb helper private gamma_set/get functions are only required when the driver supports paletted 8bit mode with fbdev. Armada uses 32bpp unconditionally, so this is just dead code. It also doesn't do anything really. Let's just remove it. Cc: Russell King <rmk+kernel@arm.linux.org.uk> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/armada/armada_crtc.c | 10 ---------- drivers/gpu/drm/armada/armada_crtc.h | 2 -- drivers/gpu/drm/armada/armada_fbdev.c | 2 -- 3 files changed, 14 deletions(-)