Re: [PATCH] gma500:Remove functions that are now deprecated and move to the newer functions in drm_dp_helper.c

Re: [PATCH] gma500:Remove functions that are now deprecated and move to the newer functions in drm_dp_helper.c

[Alan Cox]

On Mon, 2015-05-04 at 18:29 -0400, Nicholas Krause 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.

Which devices have you tested this on ?

Alan


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] gma500:Remove functions that are now deprecated and move to the newer functions in drm_dp_helper.c

[Alan Cox]

On Mon, 2015-05-04 at 18:29 -0400, Nicholas Krause 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.

Which devices have you tested this on ?

Alan

Re: [PATCH] gma500:Remove functions that are now deprecated and move to the newer functions in drm_dp_helper.c

[Patrik Jakobsson]

On Tue, May 5, 2015 at 12:29 AM, Nicholas Krause <xerofoify@gmail.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

> Signed-off-by: Nicholas Krause <xerofoify@gmail.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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] gma500:Remove functions that are now deprecated and move to the newer functions in drm_dp_helper.c

[Patrik Jakobsson]

On Tue, May 5, 2015 at 12:29 AM, Nicholas Krause <xerofoify@gmail.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

> Signed-off-by: Nicholas Krause <xerofoify@gmail.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
>

Re: [PATCH] gma500:Remove functions that are now deprecated and move to the newer functions in drm_dp_helper.c

[patrik.r.jakobsson]

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@gmail.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

> >> Signed-off-by: Nicholas Krause <xerofoify@gmail.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
> >>

Re: [PATCH] gma500:Remove functions that are now deprecated and move to the newer functions in drm_dp_helper.c

[patrik.r.jakobsson]

On Sun, May 10, 2015 at 01:48:14PM -0400, nick wrote:
> 
> 
> On 2015-05-10 01:04 PM, patrik.r.jakobsson@gmail.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@gmail.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

> 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@gmail.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
> >>>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] gma500:Remove functions that are now deprecated and move to the newer functions in drm_dp_helper.c

[patrik.r.jakobsson]

On Sun, May 10, 2015 at 01:48:14PM -0400, nick wrote:
> 
> 
> On 2015-05-10 01:04 PM, patrik.r.jakobsson@gmail.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@gmail.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

> 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@gmail.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
> >>>>

Re: [PATCH] gma500:Remove functions that are now deprecated and move to the newer functions in drm_dp_helper.c

[patrik.r.jakobsson]

On Sun, May 10, 2015 at 02:40:40PM -0400, nick wrote:
> 
> 
> On 2015-05-10 02:35 PM, patrik.r.jakobsson@gmail.com wrote:
> > On Sun, May 10, 2015 at 01:48:14PM -0400, nick wrote:
> >>
> >>
> >> On 2015-05-10 01:04 PM, patrik.r.jakobsson@gmail.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@gmail.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?

> >> 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@gmail.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
> >>>>>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] gma500:Remove functions that are now deprecated and move to the newer functions in drm_dp_helper.c

[patrik.r.jakobsson]

On Sun, May 10, 2015 at 02:40:40PM -0400, nick wrote:
> 
> 
> On 2015-05-10 02:35 PM, patrik.r.jakobsson@gmail.com wrote:
> > On Sun, May 10, 2015 at 01:48:14PM -0400, nick wrote:
> >>
> >>
> >> On 2015-05-10 01:04 PM, patrik.r.jakobsson@gmail.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@gmail.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?

> >> 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@gmail.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
> >>>>>>

Re: [PATCH] gma500:Remove functions that are now deprecated and move to the newer functions in drm_dp_helper.c

[Patrik Jakobsson]

On Sun, May 10, 2015 at 9:50 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
>
>
> On May 10, 2015 3:45:36 PM EDT, patrik.r.jakobsson@gmail.com wrote:
>>On Sun, May 10, 2015 at 02:40:40PM -0400, nick wrote:
>>>
>>>
>>> On 2015-05-10 02:35 PM, patrik.r.jakobsson@gmail.com wrote:
>>> > On Sun, May 10, 2015 at 01:48:14PM -0400, nick wrote:
>>> >>
>>> >>
>>> >> On 2015-05-10 01:04 PM, patrik.r.jakobsson@gmail.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@gmail.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@gmail.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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] gma500:Remove functions that are now deprecated and move to the newer functions in drm_dp_helper.c

[Patrik Jakobsson]

On Sun, May 10, 2015 at 9:50 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
>
>
> On May 10, 2015 3:45:36 PM EDT, patrik.r.jakobsson@gmail.com wrote:
>>On Sun, May 10, 2015 at 02:40:40PM -0400, nick wrote:
>>>
>>>
>>> On 2015-05-10 02:35 PM, patrik.r.jakobsson@gmail.com wrote:
>>> > On Sun, May 10, 2015 at 01:48:14PM -0400, nick wrote:
>>> >>
>>> >>
>>> >> On 2015-05-10 01:04 PM, patrik.r.jakobsson@gmail.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@gmail.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@gmail.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.

Re: [PATCH] gma500:Remove functions that are now deprecated and move to the newer functions in drm_dp_helper.c

[Patrik Jakobsson]

On Mon, May 11, 2015 at 5:54 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
>
>
> On May 11, 2015 4:24:01 AM EDT, Patrik Jakobsson <patrik.r.jakobsson@gmail.com> wrote:
>>On Sun, May 10, 2015 at 9:50 PM, Nicholas Krause <xerofoify@gmail.com>
>>wrote:
>>>
>>>
>>> On May 10, 2015 3:45:36 PM EDT, patrik.r.jakobsson@gmail.com wrote:
>>>>On Sun, May 10, 2015 at 02:40:40PM -0400, nick wrote:
>>>>>
>>>>>
>>>>> On 2015-05-10 02:35 PM, patrik.r.jakobsson@gmail.com wrote:
>>>>> > On Sun, May 10, 2015 at 01:48:14PM -0400, nick wrote:
>>>>> >>
>>>>> >>
>>>>> >> On 2015-05-10 01:04 PM, patrik.r.jakobsson@gmail.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@gmail.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
>>
> That's not what I meant in terms of teaching me.  I asked if you want help solving this issue with your machine.
> Nick

Sure. What info do you need?