From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757838AbcBJKTn (ORCPT ); Wed, 10 Feb 2016 05:19:43 -0500 Received: from us01smtprelay-2.synopsys.com ([198.182.47.9]:40153 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757814AbcBJKTg (ORCPT ); Wed, 10 Feb 2016 05:19:36 -0500 Subject: Re: [PATCH] adv7511: Added mode_fixup function. To: Lars-Peter Clausen , Carlos Palminha , Laurent Pinchart , David Airlie , Laurent Pinchart , , 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> <20160209092444.GD11240@phenom.ffwll.local> From: Carlos Palminha Message-ID: <56BB0EA9.1040301@synopsys.com> Date: Wed, 10 Feb 2016 10:19:21 +0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160209092444.GD11240@phenom.ffwll.local> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.107.25.139] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi guys, I agree that this should be fixed in the helper library. There are already a lot of drivers that copy/paste code and several helper functions that do not avoid it. I will start sending some patches to fix this type of issue in several places of DRM helper functions. Regards, C.Palminha On 09-02-2016 09:24, Daniel Vetter wrote: > 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 >