Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1209861 > unrolled thread

[PATCH v3 0/14] Add Analogix Core Display Port Driver

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/14] Add Analogix Core Display Port Driver Yakir Yang <ykk@rock-chips.com> - 2015-08-19 16:50 +0200
    [PATCH v3 07/14] drm: rockchip/dp: add rockchip platform dp driver Yakir Yang <ykk@rock-chips.com> - 2015-08-19 17:00 +0200
    [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-08-19 17:00 +0200
      Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp Rob Herring <robherring2@gmail.com> - 2015-08-24 01:30 +0200
        Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-08-24 02:50 +0200
          Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-08-24 04:50 +0200
            Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-08-24 06:30 +0200
              Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-08-24 14:50 +0200
                Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp Heiko Stuebner <heiko@sntech.de> - 2015-08-24 15:10 +0200
                  Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-08-25 03:40 +0200
                Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-08-25 02:00 +0200
                  Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-08-25 03:40 +0200
                  Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-08-25 03:40 +0200
          Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp Jingoo Han <jingoohan1@gmail.com> - 2015-08-24 09:50 +0200
            Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-08-24 15:00 +0200
        Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-08-24 04:30 +0200
        Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-08-24 15:00 +0200
          Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp Rob Herring <robherring2@gmail.com> - 2015-08-24 16:50 +0200
            Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp Heiko Stuebner <heiko@sntech.de> - 2015-08-24 18:20 +0200
            Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-08-25 03:30 +0200
            Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Thierry Reding <treding@nvidia.com> - 2015-08-25 11:20 +0200
              Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-08-25 11:40 +0200
                Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Thierry Reding <treding@nvidia.com> - 2015-08-25 12:50 +0200
                  Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-08-25 13:00 +0200
              Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-08-25 11:50 +0200
                Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Thierry Reding <treding@nvidia.com> - 2015-08-25 12:10 +0200
                  Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-08-25 16:10 +0200
        Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Thierry Reding <treding@nvidia.com> - 2015-08-25 11:20 +0200
          Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-08-25 11:40 +0200
          Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp Rob Herring <robherring2@gmail.com> - 2015-08-25 15:30 +0200
            Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Thierry Reding <treding@nvidia.com> - 2015-08-25 16:20 +0200
              Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-08-25 16:30 +0200
      Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Thierry Reding <treding@nvidia.com> - 2015-08-25 12:00 +0200
        Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-08-25 16:10 +0200
          Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Thierry Reding <treding@nvidia.com> - 2015-08-25 16:30 +0200
            Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for  analogix_dp Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-08-25 18:00 +0200
    [PATCH v3 14/14] drm: bridge/analogix_dp: add edid modes parse in get_modes method Yakir Yang <ykk@rock-chips.com> - 2015-08-19 17:00 +0200
    [PATCH v3 08/14] phy: Add driver for rockchip Display Port PHY Yakir Yang <ykk@rock-chips.com> - 2015-08-19 17:00 +0200
      Re: [PATCH v3 08/14] phy: Add driver for rockchip Display Port PHY Kishon Vijay Abraham I <kishon@ti.com> - 2015-08-20 06:50 +0200
    [PATCH v3 11/14] drm: bridge: analogix_dp: try force hpd after plug in lookup failed Yakir Yang <ykk@rock-chips.com> - 2015-08-19 17:00 +0200
    [PATCH v3 05/14] drm: bridge/analogix_dp: fix link_rate & lane_count bug Yakir Yang <ykk@rock-chips.com> - 2015-08-19 17:00 +0200
      Re: [PATCH v3 05/14] drm: bridge/analogix_dp: fix link_rate & lane_count bug Jingoo Han <jingoohan1@gmail.com> - 2015-08-20 09:30 +0200
        Re: [PATCH v3 05/14] drm: bridge/analogix_dp: fix link_rate & lane_count  bug Yakir Yang <ykk@rock-chips.com> - 2015-08-20 10:10 +0200
    [PATCH v3 12/14] drm: bridge/analogix_dp: expand the delay time for hpd detect Yakir Yang <ykk@rock-chips.com> - 2015-08-19 17:00 +0200
      Re: [PATCH v3 12/14] drm: bridge/analogix_dp: expand the delay time for hpd detect Jingoo Han <jingoohan1@gmail.com> - 2015-08-20 08:20 +0200
        Re: [PATCH v3 12/14] drm: bridge/analogix_dp: expand the delay time  for hpd detect Yakir Yang <ykk@rock-chips.com> - 2015-08-20 10:10 +0200
    [PATCH v3 04/14] drm: bridge/analogix_dp: dynamic parse sync_pol & interlace & colorimetry Yakir Yang <ykk@rock-chips.com> - 2015-08-19 17:00 +0200
    [PATCH v3 02/14] drm: exynos/dp: convert to drm bridge mode Yakir Yang <ykk@rock-chips.com> - 2015-08-19 17:00 +0200
    [PATCH v3 13/14] drm: bridge/analogix_dp: move hpd detect to connector detect function Yakir Yang <ykk@rock-chips.com> - 2015-08-19 17:00 +0200
      Re: [PATCH v3 13/14] drm: bridge/analogix_dp: move hpd detect to connector detect function Jingoo Han <jingoohan1@gmail.com> - 2015-08-20 09:50 +0200
        Re: [PATCH v3 13/14] drm: bridge/analogix_dp: move hpd detect to  connector detect function Yakir Yang <ykk@rock-chips.com> - 2015-08-20 10:20 +0200
    [PATCH v3 10/14] drm: bridge: analogix_dp: add some rk3288 special registers setting Yakir Yang <ykk@rock-chips.com> - 2015-08-19 17:00 +0200
    [PATCH v3 09/14] drm: bridge/analogix_dp: add platform device type support Yakir Yang <ykk@rock-chips.com> - 2015-08-19 17:00 +0200
    [PATCH v3 01/14] drm: exynos/dp: fix code style Yakir Yang <ykk@rock-chips.com> - 2015-08-19 17:00 +0200
    Re: [PATCH v3 0/14] Add Analogix Core Display Port Driver Dave Airlie <airlied@gmail.com> - 2015-08-20 02:00 +0200
      Re: [PATCH v3 0/14] Add Analogix Core Display Port Driver Yakir Yang <ykk@rock-chips.com> - 2015-08-20 03:10 +0200
    Re: [PATCH v3 0/14] Add Analogix Core Display Port Driver Archit Taneja <architt@codeaurora.org> - 2015-08-20 06:40 +0200
      Re: [PATCH v3 0/14] Add Analogix Core Display Port Driver Jingoo Han <jingoohan1@gmail.com> - 2015-08-20 08:00 +0200
        Re: [PATCH v3 0/14] Add Analogix Core Display Port Driver Yakir Yang <ykk@rock-chips.com> - 2015-08-20 08:30 +0200
          Re: [PATCH v3 0/14] Add Analogix Core Display Port Driver Jingoo Han <jingoohan1@gmail.com> - 2015-08-20 09:00 +0200
            Re: [PATCH v3 0/14] Add Analogix Core Display Port Driver Yakir Yang <ykk@rock-chips.com> - 2015-08-20 10:10 +0200
    Re: [PATCH v3 0/14] Add Analogix Core Display Port Driver Jingoo Han <jingoohan1@gmail.com> - 2015-08-21 10:30 +0200
      Re: [PATCH v3 0/14] Add Analogix Core Display Port Driver Thierry Reding <treding@nvidia.com> - 2015-08-21 15:20 +0200
        Re: [PATCH v3 0/14] Add Analogix Core Display Port Driver Romain Perier <romain.perier@gmail.com> - 2015-08-30 14:20 +0200
          Re: [PATCH v3 0/14] Add Analogix Core Display Port Driver Yakir Yang <ykk@rock-chips.com> - 2015-08-31 04:50 +0200

Page 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

#1212871 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Multipart message — attachments visible in raw view] — view raw

On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
> On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
> >> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> >> > +       -analogix,color-depth:
> >> > +               number of bits per colour component.
> >> > +                       COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 = 3
> >>
> >> This seems pretty generic. Just use 6, 8, 10, or 12 for values. And
> >> drop the vendor prefix.
> >
> > Please think about this some more.  What does "color-depth" mean?  Does it
> > mean the number of bits per colour _component_, or does it mean the total
> > number of bits to represent a particular colour.  It's confusing as it
> > stands.
> 
> Then "component-color-bpp" perhaps?

There should be no need to have this in DT at all. The BPC is a property
of the attached panel and it should come from the panel (either the
panel driver or parsed from EDID if available).

> > When we adopted the graph bindings for iMX DRM, I thought exactly at that
> > time "it would be nice if this could become the standard for binding DRM
> > components together" but I don't have the authority from either the DT
> > perspective or the DRM perspective to mandate that.  Neither does anyone
> > else.  That's the _real_ problem here.
> >
> > I've seen several DRM bindings go by which don't use the of-graph stuff,
> > which means that they'll never be compatible with generic components
> > which do use the of-graph stuff.
> 
> It goes beyond bindings IMO. The use of the component framework or not
> has been at the whim of driver writers as well. It is either used or
> private APIs are created. I'm using components and my need for it
> boils down to passing the struct drm_device pointer to the encoder.
> Other components like panels and bridges have different ways to attach
> to the DRM driver.

I certainly support unification, but it needs to be reasonable. There
are cases where a different structure for the binding work better than
another and I think this always needs to be evaluated on a case by case
basis.

Because of that I think it makes sense to make all these framework bits
opt-in, otherwise we could easily end up in a situation where drivers
have to be rearchitected (or even DT bindings altered!) in order to be
able to reuse code.

Thierry

[toc] | [prev] | [next] | [standalone]

#1212910 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

On Tue, Aug 25, 2015 at 11:12:48AM +0200, Thierry Reding wrote:
> On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
> > It goes beyond bindings IMO. The use of the component framework or not
> > has been at the whim of driver writers as well. It is either used or
> > private APIs are created. I'm using components and my need for it
> > boils down to passing the struct drm_device pointer to the encoder.
> > Other components like panels and bridges have different ways to attach
> > to the DRM driver.
> 
> I certainly support unification, but it needs to be reasonable. There
> are cases where a different structure for the binding work better than
> another and I think this always needs to be evaluated on a case by case
> basis.

It can't be a case-by-case basis.

The TDA998x encoder/connector is going to be component only.  This is
a generic chip, which can be attached to the output of any parallel
RGB+sync+clock bus.  In other words, it could appear anywhere.

Are you really saying that we need to support multiple schemes of
attaching the driver to DRM?  That's totally insane IMHO.

The problem with the drm_encoder_slave stuff is that you can't sanely
attach of-nodes to the drm-created i2c device.  Yes, you can parse
them from the DT file as a sub-node of the upper device, but that
then goes against the principle of the I2C bindings, which is to
list the I2C devices as a child below the I2C adapter node.  If you
try and put the DT node there, then the OF code will create the I2C
device for you, and the drm_encoder_slave stuff won't have the
control it needs to communicate through the wrapped i2c_driver
stuff.

So, tda998x is going component-only, as that's the _only_ sane solution
for it.

Now, what happens when some other DRM driver wants to use the tda998x
driver, and its bindings are not compatible with the component helpers?
They're pretty much stuck up the creek without a paddle.

Case by case doesn't work unless you're talking about truely isolated
hardware where no one shares anything.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212952 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Multipart message — attachments visible in raw view] — view raw

On Tue, Aug 25, 2015 at 10:29:39AM +0100, Russell King - ARM Linux wrote:
> On Tue, Aug 25, 2015 at 11:12:48AM +0200, Thierry Reding wrote:
> > On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
> > > It goes beyond bindings IMO. The use of the component framework or not
> > > has been at the whim of driver writers as well. It is either used or
> > > private APIs are created. I'm using components and my need for it
> > > boils down to passing the struct drm_device pointer to the encoder.
> > > Other components like panels and bridges have different ways to attach
> > > to the DRM driver.
> > 
> > I certainly support unification, but it needs to be reasonable. There
> > are cases where a different structure for the binding work better than
> > another and I think this always needs to be evaluated on a case by case
> > basis.
> 
> It can't be a case-by-case basis.
> 
> The TDA998x encoder/connector is going to be component only.  This is
> a generic chip, which can be attached to the output of any parallel
> RGB+sync+clock bus.  In other words, it could appear anywhere.
> 
> Are you really saying that we need to support multiple schemes of
> attaching the driver to DRM?  That's totally insane IMHO.

No, what I'm saying is that we should have a single scheme, but one
that doesn't put any restrictions on what kind of DT binding you use or
how your driver is architected.

> The problem with the drm_encoder_slave stuff is that you can't sanely
> attach of-nodes to the drm-created i2c device.  Yes, you can parse
> them from the DT file as a sub-node of the upper device, but that
> then goes against the principle of the I2C bindings, which is to
> list the I2C devices as a child below the I2C adapter node.  If you
> try and put the DT node there, then the OF code will create the I2C
> device for you, and the drm_encoder_slave stuff won't have the
> control it needs to communicate through the wrapped i2c_driver
> stuff.
> 
> So, tda998x is going component-only, as that's the _only_ sane solution
> for it.

Has anyone ever considered turning it into a DRM bridge driver? I had
always envisioned component/master to be primarily useful to glue
together various SoC components to form one componentized device. Now
if tda998x is an I2C slave it is external to the SoC (auxiliary), so
in my opinion much better off as a bridge driver.

Bridge drivers don't come with any of the disadvantages that the
drm_encoder_slave stuff has. They are regular drivers that are probed
via their parent busses (I2C, platform, SPI, ...) and hook into DRM via
an abstract interface. The DT aspect is taken care of automatically
because they get instantiated by their parent bus like any other device.

> Now, what happens when some other DRM driver wants to use the tda998x
> driver, and its bindings are not compatible with the component helpers?
> They're pretty much stuck up the creek without a paddle.

I'm sure that will be very helpful response for whoever's going to end
up having to deal with that situation.

> Case by case doesn't work unless you're talking about truely isolated
> hardware where no one shares anything.

There are two different things here. The inter-driver interface, which,
in my opinion, it makes a lot of sense to standardize. Like I mentioned
above I think it unwise to make this interface depend upon a framework
or the firmware description such as DT in order to avoid unnecessary
restrictions. The second, orthogonal, issue, is the DT bindings. Those
I think should absolutely be designed case by case and select whatever
most accurately describes the hardware.

Thierry

[toc] | [prev] | [next] | [standalone]

#1212953 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

On Tue, Aug 25, 2015 at 12:40:01PM +0200, Thierry Reding wrote:
> On Tue, Aug 25, 2015 at 10:29:39AM +0100, Russell King - ARM Linux wrote:
> > Now, what happens when some other DRM driver wants to use the tda998x
> > driver, and its bindings are not compatible with the component helpers?
> > They're pretty much stuck up the creek without a paddle.
> 
> I'm sure that will be very helpful response for whoever's going to end
> up having to deal with that situation.

Thank you for that comment, it's very constructive and much appreciated.
I can see it's well worth me continuing to spend time on this thread.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212915 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

Hi Thierry,

在 2015/8/25 17:12, Thierry Reding 写道:
> On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
>> On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>>>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>>> +       -analogix,color-depth:
>>>>> +               number of bits per colour component.
>>>>> +                       COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 = 3
>>>> This seems pretty generic. Just use 6, 8, 10, or 12 for values. And
>>>> drop the vendor prefix.
>>> Please think about this some more.  What does "color-depth" mean?  Does it
>>> mean the number of bits per colour _component_, or does it mean the total
>>> number of bits to represent a particular colour.  It's confusing as it
>>> stands.
>> Then "component-color-bpp" perhaps?
> There should be no need to have this in DT at all. The BPC is a property
> of the attached panel and it should come from the panel (either the
> panel driver or parsed from EDID if available).

Actually I have send an email about this one to you in version 2, just 
past from that email:

"samsung,color_space" and "samsung,color-depth"

The drm_display_info's color_formats and bpc indicate the monitor 
display ability, but
the edp driver could not take it as input video format directly.

For example, with my DP TV I would found "RGB444 & YCRCB422 & & YCRCB444"
support in drm_display_info.color_formats and 16bit bpc support, but 
RK3288 crtc
driver could only output RGB & ITU formats, so finally 
analogix_dp-rockchip driver
config crtc to RGBaaa 10bpc mode.

In this sutiation, the analogix_dp core driver would pazzled by the 
drm_display_info,
can't chose the right color_space and bpc.

And this is the place that confused me, wish you could give some ideas 
about this one :-)

- Yakir
>>> When we adopted the graph bindings for iMX DRM, I thought exactly at that
>>> time "it would be nice if this could become the standard for binding DRM
>>> components together" but I don't have the authority from either the DT
>>> perspective or the DRM perspective to mandate that.  Neither does anyone
>>> else.  That's the _real_ problem here.
>>>
>>> I've seen several DRM bindings go by which don't use the of-graph stuff,
>>> which means that they'll never be compatible with generic components
>>> which do use the of-graph stuff.
>> It goes beyond bindings IMO. The use of the component framework or not
>> has been at the whim of driver writers as well. It is either used or
>> private APIs are created. I'm using components and my need for it
>> boils down to passing the struct drm_device pointer to the encoder.
>> Other components like panels and bridges have different ways to attach
>> to the DRM driver.
> I certainly support unification, but it needs to be reasonable. There
> are cases where a different structure for the binding work better than
> another and I think this always needs to be evaluated on a case by case
> basis.
>
> Because of that I think it makes sense to make all these framework bits
> opt-in, otherwise we could easily end up in a situation where drivers
> have to be rearchitected (or even DT bindings altered!) in order to be
> able to reuse code.
>
> Thierry


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212923 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Multipart message — attachments visible in raw view] — view raw

On Tue, Aug 25, 2015 at 05:41:19PM +0800, Yakir Yang wrote:
> Hi Thierry,
> 
> 在 2015/8/25 17:12, Thierry Reding 写道:
> >On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
> >>On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
> >><linux@arm.linux.org.uk> wrote:
> >>>On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
> >>>>On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> >>>>>+       -analogix,color-depth:
> >>>>>+               number of bits per colour component.
> >>>>>+                       COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 = 3
> >>>>This seems pretty generic. Just use 6, 8, 10, or 12 for values. And
> >>>>drop the vendor prefix.
> >>>Please think about this some more.  What does "color-depth" mean?  Does it
> >>>mean the number of bits per colour _component_, or does it mean the total
> >>>number of bits to represent a particular colour.  It's confusing as it
> >>>stands.
> >>Then "component-color-bpp" perhaps?
> >There should be no need to have this in DT at all. The BPC is a property
> >of the attached panel and it should come from the panel (either the
> >panel driver or parsed from EDID if available).
> 
> Actually I have send an email about this one to you in version 2, just past
> from that email:
> 
> "samsung,color_space" and "samsung,color-depth"
> 
> The drm_display_info's color_formats and bpc indicate the monitor display
> ability, but
> the edp driver could not take it as input video format directly.
> 
> For example, with my DP TV I would found "RGB444 & YCRCB422 & & YCRCB444"
> support in drm_display_info.color_formats and 16bit bpc support, but RK3288
> crtc
> driver could only output RGB & ITU formats, so finally analogix_dp-rockchip
> driver
> config crtc to RGBaaa 10bpc mode.
> 
> In this sutiation, the analogix_dp core driver would pazzled by the
> drm_display_info,
> can't chose the right color_space and bpc.
> 
> And this is the place that confused me, wish you could give some ideas about
> this one :-)

Your display driver should choose whatever it is capable of outputting.
If the display reports that it can do 16 bits-per-color, but your
display driver can't do it, then it should choose a configuration that
it supports. Similarily for the color encodings. If you can't generate
YCrCb444 with your hardware, then it's the driver's job to know about
that and select the next appropriate configuration.

But hard-coding this is not the right solution because the value in DT
may end up conflicting with what the display reports.

Thierry

[toc] | [prev] | [next] | [standalone]

#1213058 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

Hi Thierry,

在 2015/8/25 18:06, Thierry Reding 写道:
> On Tue, Aug 25, 2015 at 05:41:19PM +0800, Yakir Yang wrote:
>> Hi Thierry,
>>
>> 在 2015/8/25 17:12, Thierry Reding 写道:
>>> On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
>>>> On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
>>>> <linux@arm.linux.org.uk> wrote:
>>>>> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>>>>>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>>>>> +       -analogix,color-depth:
>>>>>>> +               number of bits per colour component.
>>>>>>> +                       COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 = 3
>>>>>> This seems pretty generic. Just use 6, 8, 10, or 12 for values. And
>>>>>> drop the vendor prefix.
>>>>> Please think about this some more.  What does "color-depth" mean?  Does it
>>>>> mean the number of bits per colour _component_, or does it mean the total
>>>>> number of bits to represent a particular colour.  It's confusing as it
>>>>> stands.
>>>> Then "component-color-bpp" perhaps?
>>> There should be no need to have this in DT at all. The BPC is a property
>>> of the attached panel and it should come from the panel (either the
>>> panel driver or parsed from EDID if available).
>> Actually I have send an email about this one to you in version 2, just past
>> from that email:
>>
>> "samsung,color_space" and "samsung,color-depth"
>>
>> The drm_display_info's color_formats and bpc indicate the monitor display
>> ability, but
>> the edp driver could not take it as input video format directly.
>>
>> For example, with my DP TV I would found "RGB444 & YCRCB422 & & YCRCB444"
>> support in drm_display_info.color_formats and 16bit bpc support, but RK3288
>> crtc
>> driver could only output RGB & ITU formats, so finally analogix_dp-rockchip
>> driver
>> config crtc to RGBaaa 10bpc mode.
>>
>> In this sutiation, the analogix_dp core driver would pazzled by the
>> drm_display_info,
>> can't chose the right color_space and bpc.
>>
>> And this is the place that confused me, wish you could give some ideas about
>> this one :-)
> Your display driver should choose whatever it is capable of outputting.
> If the display reports that it can do 16 bits-per-color, but your
> display driver can't do it, then it should choose a configuration that
> it supports. Similarily for the color encodings. If you can't generate
> YCrCb444 with your hardware, then it's the driver's job to know about
> that and select the next appropriate configuration.
>
> But hard-coding this is not the right solution because the value in DT
> may end up conflicting with what the display reports.

Yeah, thanks for your explain, you are right. It's the best way to get 
"color-depth"
and "color-space" from display driver, not to hard-code in DT prop.

But if the common analogix-dp driver want to get those values, then 
those values
should come from the common drm struct data. Personally I think "struct 
drm_crtc"
is the best place that should indicate the output ability of SoC vop/lcdc.

But I haven't find out there are some place to store those message for 
now (I don't
think it's good to modify the original color-space and color-bpc which 
parsed from
monitor edid).

So could you share sme ideas about this, and I would rather to talk with 
Mark (Author
of rockchip drm driver) to find out the better way to fix this one.

Besides, I would appreciate very much if you can share some ideas about 
how Exynos
handler with this problem ;)

Thanks,
- Yakir
> Thierry


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212867 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Multipart message — attachments visible in raw view] — view raw

On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
[...]
> > +       -analogix,link-rate:
> > +               max link rate supported by the eDP controller.
> > +                       LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A,
> > +                       LINK_RATE_5_40GBPS = 0x14
> 
> Same here. I'd rather see something like "link-rate-mbps" and use the
> actual rate.

There is no need whatsoever to hard-code this in DT. (e)DP provides the
means to detect what rate the link supports and the specification
provides guidance on how to select an appropriate one.

> 
> > +       -analogix,lane-count:
> > +               max number of lanes supported by the eDP contoller.
> > +                       LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
> 
> And drop the vendor prefix here.

Same as for the link rate.

Thierry

[toc] | [prev] | [next] | [standalone]

#1212913 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

Hi Thierry,

在 2015/8/25 17:15, Thierry Reding 写道:
> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> [...]
>>> +       -analogix,link-rate:
>>> +               max link rate supported by the eDP controller.
>>> +                       LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A,
>>> +                       LINK_RATE_5_40GBPS = 0x14
>> Same here. I'd rather see something like "link-rate-mbps" and use the
>> actual rate.
> There is no need whatsoever to hard-code this in DT. (e)DP provides the
> means to detect what rate the link supports and the specification
> provides guidance on how to select an appropriate one.

Hmm... could you share more about this :-)

I only find that drm_dp_link_probe() could get the panel link-rate and
num-lanes by reading the DP_DPCD_REV messag.

I don't found there are some guidance to help select the approriate one.
Beside this DT prop just indicate the max eDP controller link-rate & lanes
support, how could eDP detect them automatically?

- Yakir

>>> +       -analogix,lane-count:
>>> +               max number of lanes supported by the eDP contoller.
>>> +                       LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
>> And drop the vendor prefix here.
> Same as for the link rate.
>
> Thierry


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1213036 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

On Tue, Aug 25, 2015 at 4:15 AM, Thierry Reding <treding@nvidia.com> wrote:
> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> [...]
>> > +       -analogix,link-rate:
>> > +               max link rate supported by the eDP controller.
>> > +                       LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A,
>> > +                       LINK_RATE_5_40GBPS = 0x14
>>
>> Same here. I'd rather see something like "link-rate-mbps" and use the
>> actual rate.
>
> There is no need whatsoever to hard-code this in DT. (e)DP provides the
> means to detect what rate the link supports and the specification
> provides guidance on how to select an appropriate one.

Good, even better.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1213063 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Multipart message — attachments visible in raw view] — view raw

On Tue, Aug 25, 2015 at 09:48:01PM +0800, Yakir Yang wrote:
> Hi Thierry & Rob,
> 
> 在 2015/8/25 21:27, Rob Herring 写道:
> >On Tue, Aug 25, 2015 at 4:15 AM, Thierry Reding <treding@nvidia.com> wrote:
> >>On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
> >>>On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> >>[...]
> >>>>+       -analogix,link-rate:
> >>>>+               max link rate supported by the eDP controller.
> >>>>+                       LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A,
> >>>>+                       LINK_RATE_5_40GBPS = 0x14
> >>>Same here. I'd rather see something like "link-rate-mbps" and use the
> >>>actual rate.
> >>There is no need whatsoever to hard-code this in DT. (e)DP provides the
> >>means to detect what rate the link supports and the specification
> >>provides guidance on how to select an appropriate one.
> >Good, even better.
> 
> I do think we still need keep this DT prop yet.
> 
> I think drm_dp_help.c could get the "panel" max link-rate and lane-count,
> but it's not enough, we still need knew the "eDP controller" max link-rate
> and lane-count.
> 
> Let me show the exact example that happened in my side. When I connect
> my board to my 2K DP-1.2 TV. Analogix dp driver would get the max link-rate
> from dpcd, and the max link-rate is 5.4Gbps. So if I just set eDP controller
> link-rate
> to 5.4Gbps, the DP TV just broken, do not light up normally.
> 
> This reason why TV broken is the max link-rate which support by RK3288 eDP
> controller is 2.7Gbps. Here are the exact words that RK3288 eDP TRM said:
> 
> *Compliant with DisplayPortTM Specification, Version 1.2.
> Compliant with eDPTM Specification, Version 1.3.
> HDCP v1.3 amendment for DisplayPortTM Revision 1.0.
> Main link containing 4 physical lanes of 2.7/1.62 Gbps/lane
> *
> **
> 
> 
> Beside I haven't found there are some registers would indicate the eDP
> controller
> max link-rate and lane-count, so this is why I still instance that we need
> this DT
> prop to indicata "Max rate controller support".
> 
> So, I wish you could agree with me on this point.

Your driver should know what link rates it supports and restrict itself
to use those. This is implied by the compatible string and doesn't need
to be duplicated into device tree.

Thierry

[toc] | [prev] | [next] | [standalone]

#1213080 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

Hi Thierry,

在 2015/8/25 22:16, Thierry Reding 写道:
> On Tue, Aug 25, 2015 at 09:48:01PM +0800, Yakir Yang wrote:
>> Hi Thierry & Rob,
>>
>> 在 2015/8/25 21:27, Rob Herring 写道:
>>> On Tue, Aug 25, 2015 at 4:15 AM, Thierry Reding <treding@nvidia.com> wrote:
>>>> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>>>>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>>> [...]
>>>>>> +       -analogix,link-rate:
>>>>>> +               max link rate supported by the eDP controller.
>>>>>> +                       LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A,
>>>>>> +                       LINK_RATE_5_40GBPS = 0x14
>>>>> Same here. I'd rather see something like "link-rate-mbps" and use the
>>>>> actual rate.
>>>> There is no need whatsoever to hard-code this in DT. (e)DP provides the
>>>> means to detect what rate the link supports and the specification
>>>> provides guidance on how to select an appropriate one.
>>> Good, even better.
>> I do think we still need keep this DT prop yet.
>>
>> I think drm_dp_help.c could get the "panel" max link-rate and lane-count,
>> but it's not enough, we still need knew the "eDP controller" max link-rate
>> and lane-count.
>>
>> Let me show the exact example that happened in my side. When I connect
>> my board to my 2K DP-1.2 TV. Analogix dp driver would get the max link-rate
>> from dpcd, and the max link-rate is 5.4Gbps. So if I just set eDP controller
>> link-rate
>> to 5.4Gbps, the DP TV just broken, do not light up normally.
>>
>> This reason why TV broken is the max link-rate which support by RK3288 eDP
>> controller is 2.7Gbps. Here are the exact words that RK3288 eDP TRM said:
>>
>> *Compliant with DisplayPortTM Specification, Version 1.2.
>> Compliant with eDPTM Specification, Version 1.3.
>> HDCP v1.3 amendment for DisplayPortTM Revision 1.0.
>> Main link containing 4 physical lanes of 2.7/1.62 Gbps/lane
>> *
>> **
>>
>>
>> Beside I haven't found there are some registers would indicate the eDP
>> controller
>> max link-rate and lane-count, so this is why I still instance that we need
>> this DT
>> prop to indicata "Max rate controller support".
>>
>> So, I wish you could agree with me on this point.
> Your driver should know what link rates it supports and restrict itself
> to use those. This is implied by the compatible string and doesn't need
> to be duplicated into device tree.

Oh, yeah, good idea   :-D
Thanks for your point out.

- Yakir

> Thierry


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212920 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Multipart message — attachments visible in raw view] — view raw

On Wed, Aug 19, 2015 at 09:50:34AM -0500, Yakir Yang wrote:
[...]
> +	-analogix,color-space:
> +		input video data format.
> +			COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2

I don't think DT is an appropriate place to set this. To my knowledge
this depends on the display and/or mode, so I don't think hard-coding
it here is the right thing to do.

Thierry

[toc] | [prev] | [next] | [standalone]

#1213059 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

Hi Thierry,

在 2015/8/25 17:58, Thierry Reding 写道:
> On Wed, Aug 19, 2015 at 09:50:34AM -0500, Yakir Yang wrote:
> [...]
>> +	-analogix,color-space:
>> +		input video data format.
>> +			COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2
> I don't think DT is an appropriate place to set this. To my knowledge
> this depends on the display and/or mode, so I don't think hard-coding
> it here is the right thing to do.

Yeah, same question with my previous reply ;)

Thanks,
- Yakir
>
> Thierry


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1213075 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Multipart message — attachments visible in raw view] — view raw

On Tue, Aug 25, 2015 at 10:03:52PM +0800, Yakir Yang wrote:
> Hi Thierry,
> 
> 在 2015/8/25 17:58, Thierry Reding 写道:
> >On Wed, Aug 19, 2015 at 09:50:34AM -0500, Yakir Yang wrote:
> >[...]
> >>+	-analogix,color-space:
> >>+		input video data format.
> >>+			COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2
> >I don't think DT is an appropriate place to set this. To my knowledge
> >this depends on the display and/or mode, so I don't think hard-coding
> >it here is the right thing to do.
> 
> Yeah, same question with my previous reply ;)

I don't have an answer for you, unfortunately. But like I said,
hard-coding isn't going to work. What if, for example, you set this to a
fixed value and then you connect a monitor that doesn't support the
specific one you set?

You cited code from dw_hdmi.c earlier, it looks like it might be correct
even though it doesn't cite a reference for why this was done. Perhaps
someone on this thread, or someone involved with dw_hdmi can answer
where that code came from.

Thierry

[toc] | [prev] | [next] | [standalone]

#1213131 — Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

On Tue, Aug 25, 2015 at 04:21:51PM +0200, Thierry Reding wrote:
> You cited code from dw_hdmi.c earlier, it looks like it might be correct
> even though it doesn't cite a reference for why this was done. Perhaps
> someone on this thread, or someone involved with dw_hdmi can answer
> where that code came from.

dw_hdmi doesn't do any format conversion - it's hard coded to RGB, 8
bits per colour component.  That's a requirement for all HDMI sinks.

The reason it's hard-coded in dw_hdmi is that (a) no one has yet decided
its worth the effort to get the dw_hdmi hardware to do the colourspace
conversion to the YUV spaces and verify that it works, and (b) I really
don't see the point when we're talking about computer like devices which
work primerily with RGB and RGB is always supported by the sink.

As far as greater colour depths go, the driver came from the Freescale
iMX6 code base, and the hardware which feeds dw_hdmi can't do more than
8 bits per component - so going to 10, 12 or 16 bits per component is
beyond what iMX6 can cope with.  Hence, no one on the iMX6 side has a
need for the deep colour stuff.

In any case, I view this as a very low priority issue - it would be nice
to have audio support on HDMI for iMX6 at some point in the next 20 years,
preferably before the hardware becomes obsolete.  I've been maintaining
patches for this for 1.5 years now... how much longer is it going to take?
My pull request to David from 15th July was ignored.  My re-send of that
after he returned was ignored.  My reminder of it has been ignored.  What's
going on in DRM land?  It would be nice to get _some_ kind of feedback so
I know why they're not being taken, so I can fix whatever the issue is.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1209868 — [PATCH v3 14/14] drm: bridge/analogix_dp: add edid modes parse in get_modes method

Display Port monitor could support kinds of mode which indicate
in monitor edid, not just one single display resolution which
defined in panel or devivetree property display timing.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v3:
- Add edid modes parse support

Changes in v2: None

 drivers/gpu/drm/bridge/analogix_dp_core.c | 17 +++++++-----
 drivers/gpu/drm/bridge/analogix_dp_core.h | 45 ++++++++++++++++---------------
 2 files changed, 33 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix_dp_core.c
index 052b9b3..83698c6 100644
--- a/drivers/gpu/drm/bridge/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix_dp_core.c
@@ -111,7 +111,7 @@ static unsigned char analogix_dp_calc_edid_check_sum(unsigned char *edid_data)
 
 static int analogix_dp_read_edid(struct analogix_dp_device *dp)
 {
-	unsigned char edid[EDID_BLOCK_LENGTH * 2];
+	unsigned char *edid = dp->edid;
 	unsigned int extend_block = 0;
 	unsigned char sum;
 	unsigned char test_vector;
@@ -915,12 +915,6 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
 			DRM_ERROR("failed to disable the panel\n");
 	}
 
-	ret = analogix_dp_handle_edid(dp);
-	if (ret) {
-		dev_err(dp->dev, "unable to handle edid\n");
-		return;
-	}
-
 	ret = analogix_dp_set_link_train(dp, dp->video_info->max_lane_count,
 					 dp->video_info->max_link_rate);
 	if (ret) {
@@ -976,8 +970,17 @@ static int analogix_dp_get_modes(struct drm_connector *connector)
 {
 	struct analogix_dp_device *dp = connector_to_dp(connector);
 	struct analogix_dp_plat_data *plat_data = dp->plat_data;
+	struct edid *edid = (struct edid *)dp->edid;
 	int num_modes = 0;
 
+	if (analogix_dp_handle_edid(dp)) {
+		dev_err(dp->dev, "unable to handle edid\n");
+		return -EINVAL;
+	}
+
+	drm_mode_connector_update_edid_property(connector, edid);
+	num_modes += drm_add_edid_modes(connector, edid);
+
 	if (plat_data && plat_data->panel)
 		num_modes += drm_panel_get_modes(plat_data->panel);
 
diff --git a/drivers/gpu/drm/bridge/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix_dp_core.h
index 2405155..950b6ca 100644
--- a/drivers/gpu/drm/bridge/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix_dp_core.h
@@ -20,6 +20,28 @@
 #define MAX_CR_LOOP 5
 #define MAX_EQ_LOOP 5
 
+/* I2C EDID Chip ID, Slave Address */
+#define I2C_EDID_DEVICE_ADDR			0x50
+#define I2C_E_EDID_DEVICE_ADDR			0x30
+
+#define EDID_BLOCK_LENGTH			0x80
+#define EDID_HEADER_PATTERN			0x00
+#define EDID_EXTENSION_FLAG			0x7e
+#define EDID_CHECKSUM				0x7f
+
+/* DP_MAX_LANE_COUNT */
+#define DPCD_ENHANCED_FRAME_CAP(x)		(((x) >> 7) & 0x1)
+#define DPCD_MAX_LANE_COUNT(x)			((x) & 0x1f)
+
+/* DP_LANE_COUNT_SET */
+#define DPCD_LANE_COUNT_SET(x)			((x) & 0x1f)
+
+/* DP_TRAINING_LANE0_SET */
+#define DPCD_PRE_EMPHASIS_SET(x)		(((x) & 0x3) << 3)
+#define DPCD_PRE_EMPHASIS_GET(x)		(((x) >> 3) & 0x3)
+#define DPCD_VOLTAGE_SWING_SET(x)		(((x) & 0x3) << 0)
+#define DPCD_VOLTAGE_SWING_GET(x)		(((x) >> 0) & 0x3)
+
 enum link_rate_type {
 	LINK_RATE_1_62GBPS = DP_LINK_BW_1_62,
 	LINK_RATE_2_70GBPS = DP_LINK_BW_2_7,
@@ -161,6 +183,7 @@ struct analogix_dp_device {
 	int			dpms_mode;
 	int			hpd_gpio;
 	bool                    need_force_hpd;
+	unsigned char           edid[EDID_BLOCK_LENGTH * 2];
 
 	struct analogix_dp_plat_data *plat_data;
 };
@@ -260,26 +283,4 @@ void analogix_dp_config_video_slave_mode(struct analogix_dp_device *dp);
 void analogix_dp_enable_scrambling(struct analogix_dp_device *dp);
 void analogix_dp_disable_scrambling(struct analogix_dp_device *dp);
 
-/* I2C EDID Chip ID, Slave Address */
-#define I2C_EDID_DEVICE_ADDR			0x50
-#define I2C_E_EDID_DEVICE_ADDR			0x30
-
-#define EDID_BLOCK_LENGTH			0x80
-#define EDID_HEADER_PATTERN			0x00
-#define EDID_EXTENSION_FLAG			0x7e
-#define EDID_CHECKSUM				0x7f
-
-/* DP_MAX_LANE_COUNT */
-#define DPCD_ENHANCED_FRAME_CAP(x)		(((x) >> 7) & 0x1)
-#define DPCD_MAX_LANE_COUNT(x)			((x) & 0x1f)
-
-/* DP_LANE_COUNT_SET */
-#define DPCD_LANE_COUNT_SET(x)			((x) & 0x1f)
-
-/* DP_TRAINING_LANE0_SET */
-#define DPCD_PRE_EMPHASIS_SET(x)		(((x) & 0x3) << 3)
-#define DPCD_PRE_EMPHASIS_GET(x)		(((x) >> 3) & 0x3)
-#define DPCD_VOLTAGE_SWING_SET(x)		(((x) & 0x3) << 0)
-#define DPCD_VOLTAGE_SWING_GET(x)		(((x) >> 0) & 0x3)
-
 #endif /* _ANALOGIX_DP_CORE_H */
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1209870 — [PATCH v3 08/14] phy: Add driver for rockchip Display Port PHY

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v3:
- Take Heiko suggest, add rockchip dp phy driver,
  collect the phy clocks and power control.

Changes in v2: None

 .../devicetree/bindings/phy/rockchip-dp-phy.txt    |  26 +++
 drivers/phy/Kconfig                                |   7 +
 drivers/phy/Makefile                               |   1 +
 drivers/phy/phy-rockchip-dp.c                      | 185 +++++++++++++++++++++
 4 files changed, 219 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/rockchip-dp-phy.txt
 create mode 100644 drivers/phy/phy-rockchip-dp.c

diff --git a/Documentation/devicetree/bindings/phy/rockchip-dp-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-dp-phy.txt
new file mode 100644
index 0000000..5de1088
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/rockchip-dp-phy.txt
@@ -0,0 +1,26 @@
+Rockchip Soc Seroes Display Port PHY
+------------------------------------
+
+Required properties:
+- compatible : should be one of the following supported values:
+	 - "rockchip.rk3288-dp-phy"
+
+- reg : a list of registers used by phy driver
+- clocks: from common clock binding: handle to dp clock.
+	of memory mapped region.
+- clock-names: from common clock binding:
+	Required elements: "sclk_dp" "sclk_dp_24m"
+
+- rockchip,grf: this soc should set GRF regs, so need get grf here.
+- #phy-cells : from the generic PHY bindings, must be 0;
+
+Example:
+
+edp_phy: phy@ff770274 {
+	compatilble = "rockchip,rk3288-dp-phy";
+	reg = <0xff770274 4>;
+	rockchip,grf = <&grf>;
+	clocks = <&cru SCLK_EDP_24M>;
+	clock-names = "24m";
+	#phy-cells = <0>;
+}
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 6b8dd16..da00440 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -297,6 +297,13 @@ config PHY_ROCKCHIP_USB
 	help
 	  Enable this to support the Rockchip USB 2.0 PHY.
 
+config PHY_ROCKCHIP_DP
+	tristate "Rockchip Display Port PHY Driver"
+	depends on ARCH_ROCKCHIP && OF
+	select GENERIC_PHY
+	help
+	  Enable this to support the Rockchip Display Port PHY.
+
 config PHY_ST_SPEAR1310_MIPHY
 	tristate "ST SPEAR1310-MIPHY driver"
 	select GENERIC_PHY
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index f344e1b..35e3ce6 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -33,6 +33,7 @@ phy-exynos-usb2-$(CONFIG_PHY_S5PV210_USB2)	+= phy-s5pv210-usb2.o
 obj-$(CONFIG_PHY_EXYNOS5_USBDRD)	+= phy-exynos5-usbdrd.o
 obj-$(CONFIG_PHY_QCOM_APQ8064_SATA)	+= phy-qcom-apq8064-sata.o
 obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
+obj-$(CONFIG_PHY_ROCKCHIP_DP)		+= phy-rockchip-dp.o
 obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)	+= phy-qcom-ipq806x-sata.o
 obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)	+= phy-spear1310-miphy.o
 obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY)	+= phy-spear1340-miphy.o
diff --git a/drivers/phy/phy-rockchip-dp.c b/drivers/phy/phy-rockchip-dp.c
new file mode 100644
index 0000000..4759111
--- /dev/null
+++ b/drivers/phy/phy-rockchip-dp.c
@@ -0,0 +1,185 @@
+/*
+ * Rockchip DP PHY driver
+ *
+ * Copyright (C) 2015 FuZhou Rockchip Co., Ltd.
+ * Author: Yakir Yang <ykk@@rock-chips.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ */
+
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/clk.h>
+#include <linux/phy/phy.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+
+#define GRF_SOC_CON12                   0x0274
+#define GRF_EDP_REF_CLK_SEL_INTER       BIT(4)
+
+#define DP_PHY_SIDDQ_WRITE_EN           BIT(21)
+#define DP_PHY_SIDDQ_ON                 0
+#define DP_PHY_SIDDQ_OFF                BIT(5)
+
+struct rockchip_dp_phy {
+	struct device  *dev;
+	struct regmap  *grf;
+	void __iomem   *regs;
+	struct clk     *phy_24m;
+};
+
+static int rockchip_dp_phy_clk_enable(struct rockchip_dp_phy *dp)
+{
+	int ret = 0;
+
+	ret = clk_set_rate(dp->phy_24m, 24000000);
+	if (ret < 0) {
+		dev_err(dp->dev, "cannot set clock phy_24m %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(dp->phy_24m);
+	if (ret < 0) {
+		dev_err(dp->dev, "cannot enable clock phy_24m %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int rockchip_dp_phy_clk_disable(struct rockchip_dp_phy *dp)
+{
+	clk_disable_unprepare(dp->phy_24m);
+
+	return 0;
+}
+
+static int rockchip_set_phy_state(struct phy *phy, bool enable)
+{
+	struct rockchip_dp_phy *dp = phy_get_drvdata(phy);
+
+	if (enable) {
+		rockchip_dp_phy_clk_enable(dp);
+		writel(DP_PHY_SIDDQ_WRITE_EN | DP_PHY_SIDDQ_ON, dp->regs);
+	} else {
+		rockchip_dp_phy_clk_disable(dp);
+		writel(DP_PHY_SIDDQ_WRITE_EN | DP_PHY_SIDDQ_OFF, dp->regs);
+	}
+
+	return 0;
+}
+
+static int rockchip_dp_phy_power_on(struct phy *phy)
+{
+	return rockchip_set_phy_state(phy, true);
+}
+
+static int rockchip_dp_phy_power_off(struct phy *phy)
+{
+	return rockchip_set_phy_state(phy, false);
+}
+
+static struct phy_ops rockchip_dp_phy_ops = {
+	.power_on	= rockchip_dp_phy_power_on,
+	.power_off	= rockchip_dp_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int rockchip_dp_phy_init(struct rockchip_dp_phy *dp)
+{
+	struct device *dev = dp->dev;
+	struct device_node *np = dev->of_node;
+	int ret;
+
+	dp->phy_24m = devm_clk_get(dev, "24m");
+	if (IS_ERR(dp->phy_24m)) {
+		dev_err(dev, "cannot get clock 24m\n");
+		return PTR_ERR(dp->phy_24m);
+	}
+
+	ret = rockchip_dp_phy_clk_enable(dp);
+	if (ret < 0) {
+		dev_err(dp->dev, "cannot enable dp phy clk %d\n", ret);
+		return ret;
+	}
+
+	dp->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
+	if (IS_ERR(dp->grf)) {
+		dev_err(dev, "rk3288-dp needs rockchip,grf property\n");
+		return PTR_ERR(dp->grf);
+	}
+
+	ret = regmap_write(dp->grf, GRF_SOC_CON12,
+			   GRF_EDP_REF_CLK_SEL_INTER |
+			   (GRF_EDP_REF_CLK_SEL_INTER << 16));
+	if (ret != 0) {
+		dev_err(dp->dev, "Could not config GRF edp ref clk: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int rockchip_dp_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy_provider *phy_provider;
+	struct rockchip_dp_phy *dp;
+	struct resource *res;
+	struct phy *phy;
+	int ret;
+
+	dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);
+	if (IS_ERR(dp))
+		return -ENOMEM;
+
+	dp->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	dp->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(dp->regs))
+		return PTR_ERR(dp->regs);
+
+	ret = rockchip_dp_phy_init(dp);
+	if (ret)
+		return ret;
+
+	phy = devm_phy_create(dev, NULL, &rockchip_dp_phy_ops, NULL);
+	if (IS_ERR(phy)) {
+		dev_err(dev, "failed to create phy\n");
+		return PTR_ERR(phy);
+	}
+	phy_set_drvdata(phy, dp);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id rockchip_dp_phy_dt_ids[] = {
+	{ .compatible = "rockchip,rk3288-dp-phy" },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, rockchip_dp_phy_dt_ids);
+
+static struct platform_driver rockchip_dp_phy_driver = {
+	.probe		= rockchip_dp_phy_probe,
+	.driver		= {
+		.name	= "rockchip-dp-phy",
+		.owner	= THIS_MODULE,
+		.of_match_table = rockchip_dp_phy_dt_ids,
+	},
+};
+
+module_platform_driver(rockchip_dp_phy_driver);
+
+MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>");
+MODULE_DESCRIPTION("Rockchip DP PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1210141 — Re: [PATCH v3 08/14] phy: Add driver for rockchip Display Port PHY

Hi,

On Wednesday 19 August 2015 08:21 PM, Yakir Yang wrote:
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>

where's the commit message?
> ---
> Changes in v3:
> - Take Heiko suggest, add rockchip dp phy driver,
>   collect the phy clocks and power control.
> 
> Changes in v2: None
> 
>  .../devicetree/bindings/phy/rockchip-dp-phy.txt    |  26 +++
>  drivers/phy/Kconfig                                |   7 +
>  drivers/phy/Makefile                               |   1 +
>  drivers/phy/phy-rockchip-dp.c                      | 185 +++++++++++++++++++++
>  4 files changed, 219 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/rockchip-dp-phy.txt
>  create mode 100644 drivers/phy/phy-rockchip-dp.c
> 
> diff --git a/Documentation/devicetree/bindings/phy/rockchip-dp-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-dp-phy.txt
> new file mode 100644
> index 0000000..5de1088
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/rockchip-dp-phy.txt
> @@ -0,0 +1,26 @@
> +Rockchip Soc Seroes Display Port PHY
> +------------------------------------
> +
> +Required properties:
> +- compatible : should be one of the following supported values:
> +	 - "rockchip.rk3288-dp-phy"
> +
> +- reg : a list of registers used by phy driver
> +- clocks: from common clock binding: handle to dp clock.
> +	of memory mapped region.
> +- clock-names: from common clock binding:
> +	Required elements: "sclk_dp" "sclk_dp_24m"
> +
> +- rockchip,grf: this soc should set GRF regs, so need get grf here.
> +- #phy-cells : from the generic PHY bindings, must be 0;
> +
> +Example:
> +
> +edp_phy: phy@ff770274 {
> +	compatilble = "rockchip,rk3288-dp-phy";
> +	reg = <0xff770274 4>;
> +	rockchip,grf = <&grf>;
> +	clocks = <&cru SCLK_EDP_24M>;
> +	clock-names = "24m";
> +	#phy-cells = <0>;
> +}
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 6b8dd16..da00440 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -297,6 +297,13 @@ config PHY_ROCKCHIP_USB
>  	help
>  	  Enable this to support the Rockchip USB 2.0 PHY.
>  
> +config PHY_ROCKCHIP_DP
> +	tristate "Rockchip Display Port PHY Driver"
> +	depends on ARCH_ROCKCHIP && OF
> +	select GENERIC_PHY
> +	help
> +	  Enable this to support the Rockchip Display Port PHY.
> +
>  config PHY_ST_SPEAR1310_MIPHY
>  	tristate "ST SPEAR1310-MIPHY driver"
>  	select GENERIC_PHY
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index f344e1b..35e3ce6 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -33,6 +33,7 @@ phy-exynos-usb2-$(CONFIG_PHY_S5PV210_USB2)	+= phy-s5pv210-usb2.o
>  obj-$(CONFIG_PHY_EXYNOS5_USBDRD)	+= phy-exynos5-usbdrd.o
>  obj-$(CONFIG_PHY_QCOM_APQ8064_SATA)	+= phy-qcom-apq8064-sata.o
>  obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
> +obj-$(CONFIG_PHY_ROCKCHIP_DP)		+= phy-rockchip-dp.o
>  obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA)	+= phy-qcom-ipq806x-sata.o
>  obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)	+= phy-spear1310-miphy.o
>  obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY)	+= phy-spear1340-miphy.o
> diff --git a/drivers/phy/phy-rockchip-dp.c b/drivers/phy/phy-rockchip-dp.c
> new file mode 100644
> index 0000000..4759111
> --- /dev/null
> +++ b/drivers/phy/phy-rockchip-dp.c
> @@ -0,0 +1,185 @@
> +/*
> + * Rockchip DP PHY driver
> + *
> + * Copyright (C) 2015 FuZhou Rockchip Co., Ltd.
> + * Author: Yakir Yang <ykk@@rock-chips.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/clk.h>
> +#include <linux/phy/phy.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +
> +#define GRF_SOC_CON12                   0x0274
> +#define GRF_EDP_REF_CLK_SEL_INTER       BIT(4)
> +
> +#define DP_PHY_SIDDQ_WRITE_EN           BIT(21)
> +#define DP_PHY_SIDDQ_ON                 0
> +#define DP_PHY_SIDDQ_OFF                BIT(5)
> +
> +struct rockchip_dp_phy {
> +	struct device  *dev;
> +	struct regmap  *grf;
> +	void __iomem   *regs;
> +	struct clk     *phy_24m;
> +};
> +
> +static int rockchip_dp_phy_clk_enable(struct rockchip_dp_phy *dp)
> +{
> +	int ret = 0;
> +
> +	ret = clk_set_rate(dp->phy_24m, 24000000);
> +	if (ret < 0) {
> +		dev_err(dp->dev, "cannot set clock phy_24m %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(dp->phy_24m);
> +	if (ret < 0) {
> +		dev_err(dp->dev, "cannot enable clock phy_24m %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rockchip_dp_phy_clk_disable(struct rockchip_dp_phy *dp)
> +{
> +	clk_disable_unprepare(dp->phy_24m);
> +
> +	return 0;
> +}
> +
> +static int rockchip_set_phy_state(struct phy *phy, bool enable)
> +{
> +	struct rockchip_dp_phy *dp = phy_get_drvdata(phy);
> +
> +	if (enable) {
> +		rockchip_dp_phy_clk_enable(dp);
> +		writel(DP_PHY_SIDDQ_WRITE_EN | DP_PHY_SIDDQ_ON, dp->regs);
> +	} else {
> +		rockchip_dp_phy_clk_disable(dp);
> +		writel(DP_PHY_SIDDQ_WRITE_EN | DP_PHY_SIDDQ_OFF, dp->regs);
> +	}
> +
> +	return 0;
> +}
> +
> +static int rockchip_dp_phy_power_on(struct phy *phy)
> +{
> +	return rockchip_set_phy_state(phy, true);
> +}
> +
> +static int rockchip_dp_phy_power_off(struct phy *phy)
> +{
> +	return rockchip_set_phy_state(phy, false);
> +}
> +
> +static struct phy_ops rockchip_dp_phy_ops = {
> +	.power_on	= rockchip_dp_phy_power_on,
> +	.power_off	= rockchip_dp_phy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static int rockchip_dp_phy_init(struct rockchip_dp_phy *dp)
> +{
> +	struct device *dev = dp->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +
> +	dp->phy_24m = devm_clk_get(dev, "24m");
> +	if (IS_ERR(dp->phy_24m)) {
> +		dev_err(dev, "cannot get clock 24m\n");
> +		return PTR_ERR(dp->phy_24m);
> +	}
> +
> +	ret = rockchip_dp_phy_clk_enable(dp);
> +	if (ret < 0) {
> +		dev_err(dp->dev, "cannot enable dp phy clk %d\n", ret);
> +		return ret;
> +	}
> +
> +	dp->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> +	if (IS_ERR(dp->grf)) {
> +		dev_err(dev, "rk3288-dp needs rockchip,grf property\n");
> +		return PTR_ERR(dp->grf);
> +	}
> +
> +	ret = regmap_write(dp->grf, GRF_SOC_CON12,
> +			   GRF_EDP_REF_CLK_SEL_INTER |
> +			   (GRF_EDP_REF_CLK_SEL_INTER << 16));
> +	if (ret != 0) {
> +		dev_err(dp->dev, "Could not config GRF edp ref clk: %d\n", ret);
> +		return ret;
> +	}

This function can be split to do only clk_get and syscon_regmap_lookup in
probe, clk_enable and regmap_write in phy_init?
> +
> +	return 0;
> +}
> +
> +static int rockchip_dp_phy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct phy_provider *phy_provider;
> +	struct rockchip_dp_phy *dp;
> +	struct resource *res;
> +	struct phy *phy;
> +	int ret;
> +
> +	dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);
> +	if (IS_ERR(dp))
> +		return -ENOMEM;
> +
> +	dp->dev = dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	dp->regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(dp->regs))
> +		return PTR_ERR(dp->regs);
> +
> +	ret = rockchip_dp_phy_init(dp);
> +	if (ret)
> +		return ret;
> +
> +	phy = devm_phy_create(dev, NULL, &rockchip_dp_phy_ops, NULL);
> +	if (IS_ERR(phy)) {
> +		dev_err(dev, "failed to create phy\n");
> +		return PTR_ERR(phy);
> +	}
> +	phy_set_drvdata(phy, dp);
> +
> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +
> +	return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static const struct of_device_id rockchip_dp_phy_dt_ids[] = {
> +	{ .compatible = "rockchip,rk3288-dp-phy" },
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, rockchip_dp_phy_dt_ids);
> +
> +static struct platform_driver rockchip_dp_phy_driver = {
> +	.probe		= rockchip_dp_phy_probe,
> +	.driver		= {
> +		.name	= "rockchip-dp-phy",
> +		.owner	= THIS_MODULE,

owner is not required here.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1209871 — [PATCH v3 11/14] drm: bridge: analogix_dp: try force hpd after plug in lookup failed

Some edp screen do not have hpd signal, so we can't just return
failed when hpd plug in detect failed.

This is an hardware property, so we need add a devicetree property
"analogix,need-force-hpd" to indicate this sutiation.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v3:
- Add "analogix,need-force-hpd" to indicate whether driver need foce
  hpd when hpd detect failed.

Changes in v2: None

 .../devicetree/bindings/drm/bridge/analogix_dp.txt |  3 ++
 .../bindings/video/analogix_dp-rockchip.txt        |  1 +
 .../devicetree/bindings/video/exynos_dp.txt        |  1 +
 drivers/gpu/drm/bridge/analogix_dp_core.c          | 36 +++++++++++++++++++---
 drivers/gpu/drm/bridge/analogix_dp_core.h          |  2 ++
 drivers/gpu/drm/bridge/analogix_dp_reg.c           |  9 ++++++
 6 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt b/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
index 6127018..b043200 100644
--- a/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
+++ b/Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
@@ -40,6 +40,9 @@ Required properties for dp-controller:
 		* Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
 
 Optional properties for dp-controller:
+	-analogix,need-force-hpd:
+		Indicate driver need force hpd when hpd detect failed, this
+		is used for some eDP screen which don't have hpd signal.
 	-analogix,hpd-gpio:
 		Hotplug detect GPIO.
 			Indicates which GPIO should be used for hotplug
diff --git a/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt b/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
index 99fd421..752005e 100644
--- a/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
+++ b/Documentation/devicetree/bindings/video/analogix_dp-rockchip.txt
@@ -27,6 +27,7 @@ For the below properties, please refer to Analogix DP binding document:
 - analogix,color-depth (required)
 - analogix,link-rate (required)
 - analogix,lane-count (required)
+- analogix,need-force-hpd (optional)
 - analogix,hpd-gpio (optional)
 - video interfaces (optional)
 -------------------------------------------------------------------------------
diff --git a/Documentation/devicetree/bindings/video/exynos_dp.txt b/Documentation/devicetree/bindings/video/exynos_dp.txt
index 177506f..ba20416 100644
--- a/Documentation/devicetree/bindings/video/exynos_dp.txt
+++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
@@ -53,6 +53,7 @@ For the below properties, please refer to Analogix DP binding document:
 	-analogix,color-depth (required)
 	-analogix,link-rate (required)
 	-analogix,lane-count (required)
+	-analogix,need-force-hpd (optional)
 	-analogix,hpd-gpio (optional)
 	-video interfaces (optional)
 -------------------------------------------------------------------------------
diff --git a/drivers/gpu/drm/bridge/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix_dp_core.c
index 1778e0a..99870f7 100644
--- a/drivers/gpu/drm/bridge/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix_dp_core.c
@@ -63,15 +63,38 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
 {
 	int timeout_loop = 0;
 
-	while (analogix_dp_get_plug_in_status(dp) != 0) {
+	while (timeout_loop < DP_TIMEOUT_LOOP_COUNT) {
+		if (analogix_dp_get_plug_in_status(dp) == 0)
+			return 0;
+
 		timeout_loop++;
-		if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
-			dev_err(dp->dev, "failed to get hpd plug status\n");
-			return -ETIMEDOUT;
-		}
 		usleep_range(10, 11);
 	}
 
+	/*
+	 * Some edp screen do not have hpd signal, so we can't just
+	 * return failed when hpd plug in detect failed, DT property
+	 * "need-force-hpd" would indicate whether driver need this.
+	 */
+	if (!dp->need_force_hpd)
+		return -ETIMEDOUT;
+
+	/*
+	 * The eDP TRM indicate that if HPD_STATUS(RO) is 0, AUX CH
+	 * will not work, so we need to give a force hpd action to
+	 * set HPD_STATUS manually.
+	 */
+	dev_dbg(dp->dev, "failed to get hpd plug status, try to force hpd\n");
+
+	analogix_dp_force_hpd(dp);
+
+	if (analogix_dp_get_plug_in_status(dp) != 0) {
+		dev_err(dp->dev, "failed to get hpd plug in status\n");
+		return -EINVAL;
+	}
+
+	dev_dbg(dp->dev, "success to get plug in status after force hpd\n");
+
 	return 0;
 }
 
@@ -1272,6 +1295,9 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
 	if (IS_ERR(dp->reg_base))
 		return PTR_ERR(dp->reg_base);
 
+	dp->need_force_hpd =
+		of_property_read_bool(dev->of_node, "analogix,need-force-hpd");
+
 	dp->hpd_gpio = of_get_named_gpio(dev->of_node, "analogix,hpd-gpio", 0);
 
 	if (gpio_is_valid(dp->hpd_gpio)) {
diff --git a/drivers/gpu/drm/bridge/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix_dp_core.h
index 941b34f..2405155 100644
--- a/drivers/gpu/drm/bridge/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix_dp_core.h
@@ -160,6 +160,7 @@ struct analogix_dp_device {
 	struct phy		*phy;
 	int			dpms_mode;
 	int			hpd_gpio;
+	bool                    need_force_hpd;
 
 	struct analogix_dp_plat_data *plat_data;
 };
@@ -180,6 +181,7 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
 				       bool enable);
 void analogix_dp_init_analog_func(struct analogix_dp_device *dp);
 void analogix_dp_init_hpd(struct analogix_dp_device *dp);
+void analogix_dp_force_hpd(struct analogix_dp_device *dp);
 enum dp_irq_type analogix_dp_get_irq_type(struct analogix_dp_device *dp);
 void analogix_dp_clear_hotplug_interrupts(struct analogix_dp_device *dp);
 void analogix_dp_reset_aux(struct analogix_dp_device *dp);
diff --git a/drivers/gpu/drm/bridge/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix_dp_reg.c
index a0fc1fb..bf7cee1 100644
--- a/drivers/gpu/drm/bridge/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix_dp_reg.c
@@ -365,6 +365,15 @@ void analogix_dp_init_hpd(struct analogix_dp_device *dp)
 	writel(reg, dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
 }
 
+void analogix_dp_force_hpd(struct analogix_dp_device *dp)
+{
+	u32 reg;
+
+	reg = readl(dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
+	reg = (F_HPD | HPD_CTRL);
+	writel(reg, dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
+}
+
 enum dp_irq_type analogix_dp_get_irq_type(struct analogix_dp_device *dp)
 {
 	u32 reg;
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

Back to top | Article view | linux.kernel

csiph-web