Message-ID: <CAMeQTsaFv3ppNMAAnEXAAnyaahdS8pTeTDZAAYGPyf-m8L+YMA@mail.gmail.com>
Date:	Mon, 11 May 2015 10:24:01 +0200
From:	Patrik Jakobsson <patrik.r.jakobsson@...il.com>
To:	Nicholas Krause <xerofoify@...il.com>
Cc:	dri-devel <dri-devel@...ts.freedesktop.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gma500:Remove functions that are now deprecated and move
 to the newer functions in drm_dp_helper.c

On Sun, May 10, 2015 at 9:50 PM, Nicholas Krause <xerofoify@...il.com> wrote:
>
>
> On May 10, 2015 3:45:36 PM EDT, patrik.r.jakobsson@...il.com wrote:
>>On Sun, May 10, 2015 at 02:40:40PM -0400, nick wrote:
>>>
>>>
>>> On 2015-05-10 02:35 PM, patrik.r.jakobsson@...il.com wrote:
>>> > On Sun, May 10, 2015 at 01:48:14PM -0400, nick wrote:
>>> >>
>>> >>
>>> >> On 2015-05-10 01:04 PM, patrik.r.jakobsson@...il.com wrote:
>>> >>> On Tue, May 05, 2015 at 11:17:07AM -0400, nick wrote:
>>> >>>>
>>> >>>>
>>> >>>> On 2015-05-05 09:06 AM, Patrik Jakobsson wrote:
>>> >>>>> On Tue, May 5, 2015 at 12:29 AM, Nicholas Krause
>><xerofoify@...il.com> wrote:
>>> >>>>>> This removes the deprecated functions,i2c_dp_aux_add_bus and
>>> >>>>>> i2c_dp_aux_prepare_bus and the only call in the function,
>>> >>>>>> cdv_intel_dp_i2c_init to i2c_dp_aux_add_bus respectfully.
>>> >>>>>> The call and use of these functions is now replaced alongside
>>> >>>>>> the removal of setting other values in the
>>function,cdv_intel_dp_i2c_init
>>> >>>>>> to use the helper function, drm_dp_aux_register that can
>>handle all this
>>> >>>>>> work internally.
>>> >>>>>
>>> >>>>> You need to fill in the drm_dp_aux structure and implement a
>>proper transfer
>>> >>>>> function. This patch would only break things. But the cdv dp
>>output is already
>>> >>>>> broken on my system so it needs fixing first.
>>> >>>>>
>>> >>>>> -Patrik
>>> >>>>>
>>> >>>> Patrik,
>>> >>>> Due to me not being an expert in this area of the kernel I based
>>my work off the similar function,
>>> >>>> intel_dp_aux_init for i915. I was unsure of which helper
>>functions for i915 need to be written
>>> >>>> differently for gma500 based solutions as I don't known the
>>differences between these two in the
>>> >>>> graphics specs area.
>>> >>>> Thanks,
>>> >>>> Nick
>>> >>>
>>> >>> Hi Nick
>>> >>> You are _required_ to know what your patches are doing before
>>sending them. If
>>> >>> you don't test your code and don't know what it's doing then we
>>will not accept
>>> >>> it. You're expected to read the available documentation and
>>relevant literature
>>> >>> before sending patches and asking questions. Otherwise someone
>>else is doing the
>>> >>> work for you, which I assume is not the purpose of you patch
>>submission.
>>> >>>
>>> >>> Thanks
>>> >>> Patrik
>>>
>>> >> Patrik,
>>> >> Sorry about that my question is actually this ,I am unsure of what
>>parts of the function,intel_dp_aux_init
>>> >> need to be ported for the gma500 gpu driver. Furthermore I will
>>list what parts for certain I known need to
>>> >> be ported over above the lines of code in the function definition
>>I have pasted below. Please let me known
>>> >> if there are any areas I am missing. This is more just of a double
>>check to make sure I don't miss something
>>> >> critical:)(I like to verify something before I code it).
>>> >> Sorry for the misunderstanding,
>>> >
>>> > You can figure this out by reading the drm dp helpers docs and
>>looking at
>>> > cdv_intel_dp.c. For instance, in gma500 we initialize DP_B and
>>DP_C.
>>> >
>>> > My point is that _you_ need to read the code and understand it. I
>>cannot help
>>> > you with that.
>>> >
>>> > But as I said in my first reply. DP on CDV is broken (at least on
>>my machine) so
>>> > that should be fixed first.
>>> >
>>> > Cheers
>>> > Patrik
>>> >
>>> Patrik,
>>> Sorry about that :). Your right it's a bad habit :(, I will start
>>working on it.
>>> Further more by "broken" you mean what. This is very vague and if you
>>would like
>>> some help with it, I am willing to if you can give me a detailed
>>response of what
>>> is wrong.
>>> Nick
>>
>>Vague? Both DP and LVDS displays are black after boot when DP is
>>connected. What
>>additional information do you need?
>>
>  That sounds broken to me. This seems like a good place to start helping a new with your permission.
> Nick

Sorry but I'm no teacher and cannot help you with that.

Patrik

>>> >> Nick
>>> >> static void
>>> >> 1002 intel_dp_aux_init(struct intel_dp *intel_dp, struct
>>intel_connector *connector)
>>> >> 1003 {
>>> >>   /*Unsure about how to port this over the functions in these lines
>>a.k.a any differences between i915 and gma500 related to these
>>functions?
>>> >>   I read through these function definitions but would like to
>>double check*/
>>> >> 1004         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>> >> 1005         struct intel_digital_port *intel_dig_port =
>>dp_to_dig_port(intel_dp);
>>> >> 1006         enum port port = intel_dig_port->port;
>>> >> /*Port these lines*/
>>> >> 1007         const char *name = NULL;
>>> >> 1008         int ret;
>>> >> /*Port this switch statement */
>>> >> 1009
>>> >> 1010         switch (port) {
>>> >> 1011         case PORT_A:
>>> >> 1012                 intel_dp->aux_ch_ctl_reg = DPA_AUX_CH_CTL;
>>> >> 1013                 name = "DPDDC-A";
>>> >> 1014                 break;
>>> >> 1015         case PORT_B:
>>> >> 1016                 intel_dp->aux_ch_ctl_reg =
>>PCH_DPB_AUX_CH_CTL;
>>> >> 1017                 name = "DPDDC-B";
>>> >> 1018                 break;
>>> >> 1019         case PORT_C:
>>> >> 1020                 intel_dp->aux_ch_ctl_reg =
>>PCH_DPC_AUX_CH_CTL;
>>> >> 1021                 name = "DPDDC-C";
>>> >> 1022                 break;
>>> >> 1023         case PORT_D:
>>> >> 1024                 intel_dp->aux_ch_ctl_reg =
>>PCH_DPD_AUX_CH_CTL;
>>> >> 1025                 name = "DPDDC-D";
>>> >> 1026                 break;
>>> >> 1027         default:
>>> >> 1028                 BUG();
>>> >> 1029         }
>>> >> 1030
>>> >> 1031         /*
>>> >> 1032          * The AUX_CTL register is usually DP_CTL + 0x10.
>>> >> 1033          *
>>> >> 1034          * On Haswell and Broadwell though:
>>> >> 1035          *   - Both port A DDI_BUF_CTL and DDI_AUX_CTL are on
>>the CPU
>>> >> 1036          *   - Port B/C/D AUX channels are on the PCH,
>>DDI_BUF_CTL on the CPU
>>> >> 1037          *
>>> >> 1038          * Skylake moves AUX_CTL back next to DDI_BUF_CTL, on
>>the CPU.
>>> >> 1039          */
>>> >> 1040         if (!IS_HASWELL(dev) && !IS_BROADWELL(dev))
>>> >> 1041                 intel_dp->aux_ch_ctl_reg =
>>intel_dp->output_reg + 0x10;
>>> >> 1042
>>> >> /*Port these lines*/
>>> >> 1043         intel_dp->aux.name = name;
>>> >> 1044         intel_dp->aux.dev = dev->dev;
>>> >> 1045         intel_dp->aux.transfer = intel_dp_aux_transfer;
>>> >> 1046
>>> >> /*Port this devugging message */
>>> >> 1047 DRM_DEBUG_KMS("registering %s bus for %s\n", name,
>>> >> 1048                       connector->base.kdev->kobj.name);
>>> >> 1049
>>> >> /*Port these lines*/
>>> >> 1050         ret = drm_dp_aux_register(&intel_dp->aux);
>>> >> 1051         if (ret < 0) {
>>> >> 1052                 DRM_ERROR("drm_dp_aux_register() for %s
>>failed (%d)\n",
>>> >> 1053                           name, ret);
>>> >> 1054                 return;
>>> >> 1055         }
>>> >> 1056
>>> >> /*Port the of the function */
>>> >> 1057         ret = sysfs_create_link(&connector->base.kdev->kobj,
>>> >> 1058                                 &intel_dp->aux.ddc.dev.kobj,
>>> >> 1059
>>intel_dp->aux.ddc.dev.kobj.name);
>>> >> 1060         if (ret < 0) {
>>> >> 1061                 DRM_ERROR("sysfs_create_link() for %s failed
>>(%d)\n", name, ret);
>>> >> 1062                 drm_dp_aux_unregister(&intel_dp->aux);
>>> >> 1063         }
>>> >> 1064 }
>>> >> 1065
>>> >>>
>>> >>>>>> Signed-off-by: Nicholas Krause <xerofoify@...il.com>
>>> >>>>>> ---
>>> >>>>>>  drivers/gpu/drm/gma500/cdv_intel_dp.c | 42
>>++---------------------------------
>>> >>>>>>  1 file changed, 2 insertions(+), 40 deletions(-)
>>> >>>>>>
>>> >>>>>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_dp.c
>>b/drivers/gpu/drm/gma500/cdv_intel_dp.c
>>> >>>>>> index 0fafb8e..c8c20fc 100644
>>> >>>>>> --- a/drivers/gpu/drm/gma500/cdv_intel_dp.c
>>> >>>>>> +++ b/drivers/gpu/drm/gma500/cdv_intel_dp.c
>>> >>>>>> @@ -200,38 +200,6 @@ static const struct i2c_algorithm
>>i2c_dp_aux_algo = {
>>> >>>>>>         .functionality  = i2c_algo_dp_aux_functionality,
>>> >>>>>>  };
>>> >>>>>>
>>> >>>>>> -static void
>>> >>>>>> -i2c_dp_aux_reset_bus(struct i2c_adapter *adapter)
>>> >>>>>> -{
>>> >>>>>> -       (void) i2c_algo_dp_aux_address(adapter, 0, false);
>>> >>>>>> -       (void) i2c_algo_dp_aux_stop(adapter, false);
>>> >>>>>> -}
>>> >>>>>> -
>>> >>>>>> -static int
>>> >>>>>> -i2c_dp_aux_prepare_bus(struct i2c_adapter *adapter)
>>> >>>>>> -{
>>> >>>>>> -       adapter->algo = &i2c_dp_aux_algo;
>>> >>>>>> -       adapter->retries = 3;
>>> >>>>>> -       i2c_dp_aux_reset_bus(adapter);
>>> >>>>>> -       return 0;
>>> >>>>>> -}
>>> >>>>>> -
>>> >>>>>> -/*
>>> >>>>>> - * FIXME: This is the old dp aux helper, gma500 is the last
>>driver that needs to
>>> >>>>>> - * be ported over to the new helper code in drm_dp_helper.c
>>like i915 or radeon.
>>> >>>>>> - */
>>> >>>>>> -static int __deprecated
>>> >>>>>> -i2c_dp_aux_add_bus(struct i2c_adapter *adapter)
>>> >>>>>> -{
>>> >>>>>> -       int error;
>>> >>>>>> -
>>> >>>>>> -       error = i2c_dp_aux_prepare_bus(adapter);
>>> >>>>>> -       if (error)
>>> >>>>>> -               return error;
>>> >>>>>> -       error = i2c_add_adapter(adapter);
>>> >>>>>> -       return error;
>>> >>>>>> -}
>>> >>>>>> -
>>> >>>>>>  #define _wait_for(COND, MS, W) ({ \
>>> >>>>>>          unsigned long timeout__ = jiffies +
>>msecs_to_jiffies(MS);       \
>>> >>>>>>          int ret__ = 0;
>>          \
>>> >>>>>> @@ -275,6 +243,7 @@ struct cdv_intel_dp {
>>> >>>>>>         int backlight_on_delay;
>>> >>>>>>         int backlight_off_delay;
>>> >>>>>>         struct drm_display_mode *panel_fixed_mode;  /* for eDP
>>*/
>>> >>>>>> +       struct drm_dp_aux aux;
>>> >>>>>>         bool panel_on;
>>> >>>>>>  };
>>> >>>>>>
>>> >>>>>> @@ -855,18 +824,11 @@ cdv_intel_dp_i2c_init(struct
>>gma_connector *connector,
>>> >>>>>>         intel_dp->algo.running = false;
>>> >>>>>>         intel_dp->algo.address = 0;
>>> >>>>>>         intel_dp->algo.aux_ch = cdv_intel_dp_i2c_aux_ch;
>>> >>>>>> -
>>> >>>>>>         memset(&intel_dp->adapter, '\0', sizeof
>>(intel_dp->adapter));
>>> >>>>>> -       intel_dp->adapter.owner = THIS_MODULE;
>>> >>>>>> -       intel_dp->adapter.class = I2C_CLASS_DDC;
>>> >>>>>> -       strncpy (intel_dp->adapter.name, name,
>>sizeof(intel_dp->adapter.name) - 1);
>>> >>>>>> -       intel_dp->adapter.name[sizeof(intel_dp->adapter.name)
>>- 1] = '\0';
>>> >>>>>> -       intel_dp->adapter.algo_data = &intel_dp->algo;
>>> >>>>>> -       intel_dp->adapter.dev.parent = connector->base.kdev;
>>> >>>>>>
>>> >>>>>>         if (is_edp(encoder))
>>> >>>>>>                 cdv_intel_edp_panel_vdd_on(encoder);
>>> >>>>>> -       ret = i2c_dp_aux_add_bus(&intel_dp->adapter);
>>> >>>>>> +       ret = drm_dp_aux_register(&intel_dp->aux);
>>> >>>>>>         if (is_edp(encoder))
>>> >>>>>>                 cdv_intel_edp_panel_vdd_off(encoder);
>>> >>>>>>
>>> >>>>>> --
>>> >>>>>> 2.1.4
>>> >>>>>>
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists