From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750894AbcBJMR1 (ORCPT ); Wed, 10 Feb 2016 07:17:27 -0500 Received: from mail-wm0-f41.google.com ([74.125.82.41]:37608 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750760AbcBJMRZ (ORCPT ); Wed, 10 Feb 2016 07:17:25 -0500 Date: Wed, 10 Feb 2016 13:17:47 +0100 From: Daniel Vetter To: Carlos Palminha Cc: Lars-Peter Clausen , 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: <20160210121747.GS11240@phenom.ffwll.local> Mail-Followup-To: Carlos Palminha , Lars-Peter Clausen , 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> <20160209092444.GD11240@phenom.ffwll.local> <56BB0EA9.1040301@synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56BB0EA9.1040301@synopsys.com> 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 Wed, Feb 10, 2016 at 10:19:21AM +0000, Carlos Palminha wrote: > 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. Awesome, thanks a lot for doing this. -Daniel > > 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 > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch