From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756051AbcBIJYZ (ORCPT ); Tue, 9 Feb 2016 04:24:25 -0500 Received: from mail-wm0-f43.google.com ([74.125.82.43]:34474 "EHLO mail-wm0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751734AbcBIJYW (ORCPT ); Tue, 9 Feb 2016 04:24:22 -0500 Date: Tue, 9 Feb 2016 10:24:44 +0100 From: Daniel Vetter To: Lars-Peter Clausen Cc: Carlos Palminha , Laurent Pinchart , David Airlie , Laurent Pinchart , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH] adv7511: Added mode_fixup function. Message-ID: <20160209092444.GD11240@phenom.ffwll.local> Mail-Followup-To: Lars-Peter Clausen , Carlos Palminha , Laurent Pinchart , David Airlie , Laurent Pinchart , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org References: <1454063627-12219-1-git-send-email-palminha@synopsys.com> <1948106.WC7MId72vp@avalon> <56AF5176.80603@synopsys.com> <56B36CB1.80000@synopsys.com> <56B3B42C.7020804@metafoo.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56B3B42C.7020804@metafoo.de> X-Operating-System: Linux phenom 4.3.0-1-amd64 User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Feb 04, 2016 at 09:27:24PM +0100, Lars-Peter Clausen wrote: > On 02/04/2016 04:22 PM, Carlos Palminha wrote: > > Hi guys, > > > > any feedback? patch will be accepted for adv7511 driver? > > Hi, > > Thanks for the patch, but please try to find and fix the call site that is > trying to invoke the callback even though it does not exist. > > This is most likely drm_i2c_encoder_mode_fixup(). Agreed, this should be fixed in the helper library, not in drivers by copypasting piles more dummy functions. -Daniel > > - Lars > > > > > Regards, > > C.Palminha > > > > On 01-02-2016 12:37, Carlos Palminha wrote: > >> Hi Laurent > >> > >> On 29-01-2016 17:48, Laurent Pinchart wrote: > >>> Hi Carlos, > >>> > >>> Thank you for the patch. > >>> > >>> On Friday 29 January 2016 10:33:47 Carlos Palminha wrote: > >>>> The mode_fixup is necessary when using it in a DRM FB driver pipeline. > >>> > >>> Instead of implementing stubs in encoder drivers, wouldn't it be better to > >>> make mode_fixup optional ? > >> Probably you are right but i don't have enough knowledge or time to do that for the DRM framework. :( > >> I limited myself to do what the other drivers already implement. > >> > >> The patch is mandatory to have to the ADV working or else will get some NULL pointer crash. > >> > >> Regards, > >> C.Palminha > >> > >>> > >>>> Signed-off-by: Carlos Palminha > >>>> --- > >>>> drivers/gpu/drm/i2c/adv7511.c | 8 ++++++++ > >>>> 1 file changed, 8 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c > >>>> index 533d1e3..90082d2 100644 > >>>> --- a/drivers/gpu/drm/i2c/adv7511.c > >>>> +++ b/drivers/gpu/drm/i2c/adv7511.c > >>>> @@ -648,6 +648,13 @@ adv7511_encoder_detect(struct drm_encoder *encoder, > >>>> return status; > >>>> } > >>>> > >>>> +static bool adv7511_encoder_mode_fixup(struct drm_encoder *encoder, > >>>> + const struct drm_display_mode *mode, > >>>> + struct drm_display_mode *adjusted_mode) > >>>> +{ > >>>> + return true; > >>>> +} > >>>> + > >>>> static int adv7511_encoder_mode_valid(struct drm_encoder *encoder, > >>>> struct drm_display_mode *mode) > >>>> { > >>>> @@ -754,6 +761,7 @@ static void adv7511_encoder_mode_set(struct drm_encoder > >>>> *encoder, > >>>> > >>>> static const struct drm_encoder_slave_funcs adv7511_encoder_funcs = { > >>>> .dpms = adv7511_encoder_dpms, > >>>> + .mode_fixup = adv7511_encoder_mode_fixup, > >>>> .mode_valid = adv7511_encoder_mode_valid, > >>>> .mode_set = adv7511_encoder_mode_set, > >>>> .detect = adv7511_encoder_detect, > >>> > >> > >> > >> On 29-01-2016 17:48, Laurent Pinchart wrote: > >>> Hi Carlos, > >>> > >>> Thank you for the patch. > >>> > >>> On Friday 29 January 2016 10:33:47 Carlos Palminha wrote: > >>>> The mode_fixup is necessary when using it in a DRM FB driver pipeline. > >>> > >>> Instead of implementing stubs in encoder drivers, wouldn't it be better to > >>> make mode_fixup optional ? > >>> > >>>> Signed-off-by: Carlos Palminha > >>>> --- > >>>> drivers/gpu/drm/i2c/adv7511.c | 8 ++++++++ > >>>> 1 file changed, 8 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c > >>>> index 533d1e3..90082d2 100644 > >>>> --- a/drivers/gpu/drm/i2c/adv7511.c > >>>> +++ b/drivers/gpu/drm/i2c/adv7511.c > >>>> @@ -648,6 +648,13 @@ adv7511_encoder_detect(struct drm_encoder *encoder, > >>>> return status; > >>>> } > >>>> > >>>> +static bool adv7511_encoder_mode_fixup(struct drm_encoder *encoder, > >>>> + const struct drm_display_mode *mode, > >>>> + struct drm_display_mode *adjusted_mode) > >>>> +{ > >>>> + return true; > >>>> +} > >>>> + > >>>> static int adv7511_encoder_mode_valid(struct drm_encoder *encoder, > >>>> struct drm_display_mode *mode) > >>>> { > >>>> @@ -754,6 +761,7 @@ static void adv7511_encoder_mode_set(struct drm_encoder > >>>> *encoder, > >>>> > >>>> static const struct drm_encoder_slave_funcs adv7511_encoder_funcs = { > >>>> .dpms = adv7511_encoder_dpms, > >>>> + .mode_fixup = adv7511_encoder_mode_fixup, > >>>> .mode_valid = adv7511_encoder_mode_valid, > >>>> .mode_set = adv7511_encoder_mode_set, > >>>> .detect = adv7511_encoder_detect, > >>> > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch