On Sat, Jul 26, 2014 at 12:52:04AM +0530, Ajay Kumar wrote:

Now that we have 2 new callbacks(prepare and unprepare) for drm_panel, make changes in all the drm drivers which use the drm_panel framework to support the new callbacks.

Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com

drivers/gpu/drm/exynos/exynos_drm_dpi.c | 8 +++-- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 7 +++++ drivers/gpu/drm/panel/panel-ld9040.c | 18 +++++++++-- drivers/gpu/drm/panel/panel-s6e8aa0.c | 18 +++++++++-- drivers/gpu/drm/panel/panel-simple.c | 51 ++++++++++++++++++++++++------- drivers/gpu/drm/tegra/output.c | 2 ++ 6 files changed, 85 insertions(+), 19 deletions(-)

I'd prefer for this to be split up into patches per panel and per display driver. The reason is that if this patch is merged as-is, then if it's ever determined to cause a regression it'll be difficult to find out which change exactly caused this.

But to not break bisectability you'll need to be careful in how you break up the patches. I think the following should work:

- for each panel driver, implement dummy .prepare() and .unprepare() that return 0 - for each display driver, make use of drm_panel_prepare() and drm_panel_unprepare() - for each panel driver, properly implement .prepare() and .unprepare() (presumably by moving code out of .enable() and .disable(), respectively)

I have a couple more comments below about the ordering of the .prepare() vs. .enable() and .disable() vs. .unprepare() calls.

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 5e78d45..2f58e45 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1333,6 +1333,12 @@ static int exynos_dsi_enable(struct exynos_dsi *dsi) if (ret < 0) return ret;

• ret = drm_panel_prepare(dsi->panel);
• if (ret < 0) {

• exynos_dsi_poweroff(dsi);
  

• return ret;
  

• }
• ret = drm_panel_enable(dsi->panel); if (ret < 0) { exynos_dsi_poweroff(dsi);

@@ -1354,6 +1360,7 @@ static void exynos_dsi_disable(struct exynos_dsi *dsi)

exynos_dsi_set_display_enable(dsi, false); drm_panel_disable(dsi->panel);

• drm_panel_unprepare(dsi->panel); exynos_dsi_poweroff(dsi);

I don't know Exynos very well, so this may be completely wrong, but should disable_panel_prepare() be called much earlier than drm_panel_enable() and drm_panel_unprepare() much later than drm_panel_disable()? With the above the result is still the same, so you'll get the same glitches as without their separation.

Without knowing exactly what Exynos does in the above, I'd expect the correct sequence to be something like this:

ret = exynos_dsi_power_on(dsi); if (ret < 0) return ret;

ret = drm_panel_prepare(dsi->panel); if (ret < 0) { exynos_dsi_poweroff(dsi); return ret; }

exynos_dsi_set_display_mode(dsi); exynos_dsi_set_display_enable(dsi, true);

ret = drm_panel_enable(dsi->panel); if (ret < 0) { /* perhaps undo exynos_dsi_set_display_enable() here? */ exynos_dsi_poweroff(dsi); return ret; }

And for disable:

drm_panel_disable(dsi->panel); exynos_dsi_set_display_enable(dsi, false); drm_panel_unprepare(dsi->panel); exynos_dsi_poweroff(dsi);

Perhaps I should quote the kerneldoc that I added for drm_panel_funcs to underline why I think this is important:

/** * struct drm_panel_funcs - perform operations on a given panel * @disable: disable panel (turn off back light, etc.) * @unprepare: turn off panel * @prepare: turn on panel and perform set up * @enable: enable panel (turn on back light, etc.) * @get_modes: add modes to the connector that the panel is attached to and * return the number of modes added * * The .prepare() function is typically called before the display controller * starts to transmit video data. Panel drivers can use this to turn the panel * on and wait for it to become ready. If additional configuration is required * (via a control bus such as I2C, SPI or DSI for example) this is a good time * to do that. * * After the display controller has started transmitting video data, it's safe * to call the .enable() function. This will typically enable the backlight to * make the image on screen visible. Some panels require a certain amount of * time or frames before the image is displayed. This function is responsible * for taking this into account before enabling the backlight to avoid visual * glitches. * * Before stopping video transmission from the display controller it can be * necessary to turn off the panel to avoid visual glitches. This is done in * the .disable() function. Analogously to .enable() this typically involves * turning off the backlight and waiting for some time to make sure no image * is visible on the panel. It is then safe for the display controller to * cease transmission of video data. * * To save power when no video data is transmitted, a driver can power down * the panel. This is the job of the .unprepare() function. */

As you see, .prepare() should do whatever is necessary to make the panel accept a stream of video data, then the display driver can start sending video data and call .enable() to make the transmitted data visible.

Analogously .disable() should turn off the display, so that the user can no longer see what's going on, then the display controller can cease transmission of video data (and since the panel is disabled this should no longer cause visual glitches) and then call .unprepare() to turn the panel off.

I know that this isn't immediately relevant just yet because currently the backlight will already turn on by default, but like we discussed earlier I have ideas on how to properly fix it. As a matter of fact I'll go and send out another mail about that when I'm through this series.

static const struct drm_panel_funcs ld9040_drm_funcs = {

• .unprepare = ld9040_unprepare, .disable = ld9040_disable,
• .prepare = ld9040_prepare, .enable = ld9040_enable, .get_modes = ld9040_get_modes,

};

The patch I applied for .prepare() and .unprepare() I've reordered the functions slightly to give a more natural sequence. .disable() is now first, while .unprepare() is next, since that's the sequence that they should be called in.

Patches for the panel drivers should follow this same ordering.

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index a251361..fb0cfe2 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -45,7 +45,8 @@ struct panel_desc {

struct panel_simple { struct drm_panel base;

• bool enabled;

• bool panel_enabled;
• bool backlight_enabled;

Perhaps this should rather be:

bool prepared; bool enabled;

?

diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c index 446837e..b574ee6 100644 --- a/drivers/gpu/drm/tegra/output.c +++ b/drivers/gpu/drm/tegra/output.c @@ -139,9 +139,11 @@ static void tegra_encoder_dpms(struct drm_encoder *encoder, int mode)

if (mode != DRM_MODE_DPMS_ON) { drm_panel_disable(panel);

• drm_panel_unprepare(panel);
  

  tegra_output_disable(output);

Similarly to my comments for the Exynos drivers, this should be:

drm_panel_disable(panel); tegra_output_disable(output); drm_panel_unprepare(panel);

} else { tegra_output_enable(output);

• drm_panel_prepare(panel);
  

  drm_panel_enable(panel); }

and

drm_panel_prepare(panel); tegra_output_enable(output); drm_panel_enable(panel);

Thierry

On Wed, Jul 30, 2014 at 4:21 PM, Thierry Reding thierry.reding@gmail.com wrote:

On Sat, Jul 26, 2014 at 12:52:05AM +0530, Ajay Kumar wrote:

...

Add panel_desc structure for auo_b133htn01 eDP panel.

Also, modify the panel_simple routines to support timing_parameter delays if mentioned in the panel_desc structure.

Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com

.../devicetree/bindings/panel/auo,b133htn01.txt | 7 +++ drivers/gpu/drm/panel/panel-simple.c | 47 ++++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/panel/auo,b133htn01.txt

I think this should be two patches, one which adds the delay parameters and another which adds support for the new panel.

Ok. Will split it.

...

diff --git a/Documentation/devicetree/bindings/panel/auo,b133htn01.txt b/Documentation/devicetree/bindings/panel/auo,b133htn01.txt new file mode 100644 index 0000000..302226b --- /dev/null +++ b/Documentation/devicetree/bindings/panel/auo,b133htn01.txt @@ -0,0 +1,7 @@ +AU Optronics Corporation 13.3" FHD (1920x1080) color TFT-LCD panel

+Required properties: +- compatible: should be "auo,b133htn01"

+This binding is compatible with the simple-panel binding, which is specified +in simple-panel.txt in this directory. diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index fb0cfe2..cbbb1b8 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -41,6 +41,13 @@ struct panel_desc { unsigned int width; unsigned int height; } size;

• struct {
  

•         unsigned int prepare_stage_delay;
  

•         unsigned int enable_stage_delay;
  

•         unsigned int disable_stage_delay;
  

•         unsigned int unprepare_stage_delay;
  

• } timing_parameter;
  

These are overly long in my opinion, how about:

    struct {
            unsigned int prepare;
            unsigned int enable;
            unsigned int disable;
            unsigned int unprepare;
    } delay;

Ok, will use this.

? Oh, and this should probably mention that it's in milliseconds. On that note, do you think we'll ever need microsecond resolution? I don't think I've ever seen a panel datasheet mentioning that kind of granularity.

Nope. All in milliseconds.

...

struct panel_simple { @@ -105,6 +112,8 @@ static int panel_simple_unprepare(struct drm_panel *panel) gpiod_set_value_cansleep(p->enable_gpio, 0);

  regulator_disable(p->supply);

• if (p->desc)
  

Should have a blank line between "regulator_disable(...)" and "if ...". Also it's not useful to check for p->desc, really, since it's a bug if that is NULL.

Well, I added this check because I used just the "simple-panel" compatible for panel node on snow. This check won't be needed anymore.

However I think it would be good to check for the delay being set, like so:

    if (p->desc->delay.unprepare)
            msleep(p->desc->delay.unprepare);

Ok, will change it.

I'm not sure about the placement of the delay here, see below for more.

...

@@ -142,6 +154,9 @@ static int panel_simple_prepare(struct drm_panel *panel) return err; }

• if (p->desc)
  

•         msleep(p->desc->timing_parameter.prepare_stage_delay);
  

• if (p->enable_gpio)
          gpiod_set_value_cansleep(p->enable_gpio, 1);
  

Should the delay not be *after* all steps in prepare have been performed? That way drivers can use it to specify the time that a panel needs to "internally" become ready after they've been power up (for example).

Right. I will move that delay down.

...

@@ -157,6 +172,8 @@ static int panel_simple_enable(struct drm_panel *panel) if (p->backlight_enabled) return 0;

• if (p->desc)
  

•         msleep(p->desc->timing_parameter.enable_stage_delay);
  if (p->backlight) {
          p->backlight->props.power = FB_BLANK_UNBLANK;
          backlight_update_status(p->backlight);
  

I think here the delay makes sense where it is because it allows the panel driver to wait for a given amount of time (after video data has been sent by the display controller) until the first "good" frame is visible.

Exactly!

In general I think it would be good to have a description of these in the struct panel_desc structure, something like this perhaps:

    /**
     * @prepare: the time (in milliseconds) that it takes for the panel
     *           to become ready and start receiving video data
     * @enable: the time (in milliseconds) that it takes for the panel
     *          to display the first valid frame after starting to
     *          receive video data
     * @disable: the time (in milliseconds) that it takes for the panel
     *           to turn the display off (no content is visible)
     * @unprepare: the time (in milliseconds) that it takes for the panel

to power down itself completely.

     */
    struct {
            unsigned int prepare;
            unsigned int enable;
            unsigned int disable;
            unsigned int unprepare;
    } delay;

For prepare and enable delays this would mean that they should take effect at the very end of the .prepare() and .enable() functions, respectively. For disable in means that it should be at the end (for example to take into account the time it takes for backlight to completely turn off). For unprepare I have no idea what we would need it for. And the new panel that you're adding in this patch doesn't use it either, so perhaps it can just be left out (for now)?

Actually, there was a typo. That should have been .unprepare_stage_delay = 50. This is needed because panels need some delay before powering them on again. As in, assume you are doing a test to turn on/off display continuously, Then, the delay between (N - 1)th cycle poweroff to Nth cycle poweron should be at least 500ms. That's what the datasheet says! And, somehow 50ms works fine for me.

Ajay

...

+static const struct panel_desc auo_b133htn01 = {

• .modes = &auo_b133htn01_mode,
  

• .num_modes = 1,
  

• .size = {
  

•         .width = 293,
  

•         .height = 165,
  

• },
  

• .timing_parameter = {
  

•         .prepare_stage_delay = 105,
  

•         .enable_stage_delay = 20,
  

•         .prepare_stage_delay = 50,
  

I take it that this last one was supposed to be .enable_stage_delay since you've already set up .prepare_stage_delay.

Thierry

On Wed, Jul 30, 2014 at 4:49 PM, Thierry Reding thierry.reding@gmail.com wrote:

On Sat, Jul 26, 2014 at 12:52:08AM +0530, Ajay Kumar wrote:

...

This patch tries to seperate drm_bridge implementation into 2 parts, a drm part and a non_drm part.

A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow.

The bridge devices register themselves on a lookup table when they get probed by calling "drm_bridge_add_for_lookup".

The parent encoder driver waits till the bridge is available in the lookup table(by calling "of_drm_find_bridge") and then continues with its initialization.

The encoder driver should call "drm_bridge_attach_encoder" to pass on the drm_device and the encoder pointers to the bridge object.

Now that the drm_device pointer is available, the encoder then calls "bridge->funcs->post_encoder_init" to allow the bridge to continue registering itself with the drm core.

Also, non driver model based ptn3460 driver is removed in this patch.

Why is it removed in this patch? Can't you do this incrementally rather than remove the driver in this patch and add it again later? If you do it this way then we'll always have this one commit where devices that have a ptn3460 don't work, so it becomes impossible to bisect across this commit.

Ok. I will try to modify ptn3460 to support driver model in this patch itself. And then, adding panel support, converting it to gpiod interface and other cleanups should follow.

...

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c

[...]

...

+int drm_bridge_add_for_lookup(struct drm_bridge *bridge) +{

• mutex_lock(&bridge_lock);
  

• list_add_tail(&bridge->head, &bridge_lookup);
  

• mutex_unlock(&bridge_lock);
  

• return 0;
  

+} +EXPORT_SYMBOL(drm_bridge_add_for_lookup);

+void drm_bridge_remove_from_lookup(struct drm_bridge *bridge) +{

• mutex_lock(&bridge_lock);
  

• list_del_init(&bridge->head);
  

• mutex_unlock(&bridge_lock);
  

+} +EXPORT_SYMBOL(drm_bridge_remove_from_lookup);

The "_for_lookup" and "_from_lookup" suffixes aren't useful in my opinion.

Ok. I will just remove the suffix.

...

+int drm_bridge_attach_encoder(struct drm_bridge *bridge,

•                         struct drm_encoder *encoder)
  

And this could simply be "drm_bridge_attach()" since we'll only ever want to attach it to encoders.

Right.

...

+{

• if (!bridge || !encoder)
  

•         return -EINVAL;
  

• if (bridge->encoder)
  

•         return -EBUSY;
  

• encoder->bridge = bridge;
  

• bridge->encoder = encoder;
  

• bridge->drm_dev = encoder->dev;
  

Should this function perhaps call the bridge's ->post_encoder_init()? And it should probably call drm_bridge_init() too, since the DRM device is now available.

This will cleanup some code in both the drivers. I will change it.

...

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c

[...]

Maybe since you're introducing a new drm_bridge.c file above already it would make sense to move out existing drm_bridge related code in a preparatory patch?

Maybe Sean or Rob can comment on whether there was a specific reason to include it in drm_crtc.c in the first place.

...

@@ -1012,8 +1010,7 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, if (ret) goto out;

• bridge->dev = dev;
  

• bridge->funcs = funcs;
  

• bridge->drm_dev = dev;
  

This sets ->drm_dev, but it was already set in drm_bridge_attach(), so I think that's one more argument to call this function when attaching.

Point accepted.

...

diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c

[...]

...

@@ -1370,7 +1361,7 @@ static const struct component_ops exynos_dp_ops = { static int exynos_dp_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev;

• struct device_node *panel_node;
  

• struct device_node *panel_node, *bridge_node;
  

Nit: I don't think you'll need two variables here, since once you've obtained the real panel or bridge objects you no longer need the OF nodes.

Right.

...

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e529b68..e5a41ad 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -619,6 +619,7 @@ struct drm_plane {

/**

• drm_bridge_funcs - drm_bridge control functions

• • @post_encoder_init: called by the parent encoder

Maybe rename this to "attach" to make it more obvious when exactly it's called?

"post_encoder_attach"?

...

• @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge
• @disable: Called right before encoder prepare, disables the bridge
• @post_disable: Called right after encoder prepare, for lockstepped disable

@@ -628,6 +629,7 @@ struct drm_plane {

• @destroy: make object go away

*/ struct drm_bridge_funcs {

• int (*post_encoder_init)(struct drm_bridge *bridge);
  bool (*mode_fixup)(struct drm_bridge *bridge,
                     const struct drm_display_mode *mode,
                     struct drm_display_mode *adjusted_mode);
  

@@ -648,15 +650,19 @@ struct drm_bridge_funcs {

• @base: base mode object
• @funcs: control functions
• @driver_private: pointer to the bridge driver's internal context

• • @connector_polled: polled flag needed for registering connector

Can you explain why this new field is needed? It seems like a completely unrelated change.

How do I select this flag for the bridge chip? Assume if I did select DRM_CONNECTOR_POLL_HPD, where to call drm_helper_hpd_irq_event in the driver? Is post_encoder_init a right place?

Without the polled flag, I get display very late. Please throw some light on this!

Ajay

On Wed, Jul 30, 2014 at 8:38 PM, Thierry Reding thierry.reding@gmail.com wrote:

On Wed, Jul 30, 2014 at 08:01:44PM +0530, Ajay kumar wrote:

...

On Wed, Jul 30, 2014 at 4:49 PM, Thierry Reding thierry.reding@gmail.com wrote:

...

On Sat, Jul 26, 2014 at 12:52:08AM +0530, Ajay Kumar wrote:

...

This patch tries to seperate drm_bridge implementation into 2 parts, a drm part and a non_drm part.

A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow.

The bridge devices register themselves on a lookup table when they get probed by calling "drm_bridge_add_for_lookup".

The parent encoder driver waits till the bridge is available in the lookup table(by calling "of_drm_find_bridge") and then continues with its initialization.

The encoder driver should call "drm_bridge_attach_encoder" to pass on the drm_device and the encoder pointers to the bridge object.

Now that the drm_device pointer is available, the encoder then calls "bridge->funcs->post_encoder_init" to allow the bridge to continue registering itself with the drm core.

Also, non driver model based ptn3460 driver is removed in this patch.

Why is it removed in this patch? Can't you do this incrementally rather than remove the driver in this patch and add it again later? If you do it this way then we'll always have this one commit where devices that have a ptn3460 don't work, so it becomes impossible to bisect across this commit.

Ok. I will try to modify ptn3460 to support driver model in this patch itself. And then, adding panel support, converting it to gpiod interface and other cleanups should follow.

I think it should even be possible to do this in more separate steps. For example you could add the new bridge infrastructure without touching any of the existing drivers (so that they are completely unaffected by the changes) and then start converting one by one.

For some of the changes this may be difficult (such as the dev -> drm_dev rename to make room for the new struct device *dev). But that could for example be done in a preparatory patch that first renames the field, so that the "infrastructure" patch can add the new field without renaming any fields and therefore needing changes to drivers directly.

The goal of that whole exercise is to allow display drivers to keep working with the existing API (ptn3460_init()) while we convert the bridge drivers to register with the new framework. Then we can more safely convert each display driver individually to make use of the new framework and once all drivers have been converted the old API can simply be removed.

That way there should be no impact on existing functionality at any point.

As of now only exynos_dp uses ptn3460_init. And, also only 2 drivers use drm_bridge_init. It should be really easy to bisect if something goes wrong. Still, I will try to divide it so that each patch contains minimal change.

...

...

...

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c

[...]

Maybe since you're introducing a new drm_bridge.c file above already it would make sense to move out existing drm_bridge related code in a preparatory patch?

Maybe Sean or Rob can comment on whether there was a specific reason to include it in drm_crtc.c in the first place.

...

@@ -1012,8 +1010,7 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, if (ret) goto out;

• bridge->dev = dev;
  

• bridge->funcs = funcs;
  

• bridge->drm_dev = dev;
  

This sets ->drm_dev, but it was already set in drm_bridge_attach(), so I think that's one more argument to call this function when attaching.

Point accepted.

I forgot to mention earlier. drm_dev seems redundant to me. I'd go with just "drm".

Ok.

...

...

...

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e529b68..e5a41ad 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -619,6 +619,7 @@ struct drm_plane {

/**

• drm_bridge_funcs - drm_bridge control functions

• • @post_encoder_init: called by the parent encoder

Maybe rename this to "attach" to make it more obvious when exactly it's called?

"post_encoder_attach"?

"post_encoder_" doesn't contain much information, or even ambiguous information. What does "post" "encoder" mean? A bridge is always attached to an encoder, so "encoder" can be dropped. Now "post" has implications as to the time when it is called, but does it mean after the encoder has been initialized, or after the encoder has been removed? Simply "attach" means it's called by the parent encoder to initialize the bridge once it's been attached to an encoder. So obviously it's after the encoder has been initialized. "attach" has all he information required. Any prefix is redundant in my opinion and removing prefixes gives shorter names and reduces the number of keypresses.

Finally, what name it should have?

...

...

...

• @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge
• @disable: Called right before encoder prepare, disables the bridge
• @post_disable: Called right after encoder prepare, for lockstepped disable

@@ -628,6 +629,7 @@ struct drm_plane {

• @destroy: make object go away

*/ struct drm_bridge_funcs {

• int (*post_encoder_init)(struct drm_bridge *bridge);
  bool (*mode_fixup)(struct drm_bridge *bridge,
                     const struct drm_display_mode *mode,
                     struct drm_display_mode *adjusted_mode);
  

@@ -648,15 +650,19 @@ struct drm_bridge_funcs {

• @base: base mode object
• @funcs: control functions
• @driver_private: pointer to the bridge driver's internal context

• • @connector_polled: polled flag needed for registering connector

Can you explain why this new field is needed? It seems like a completely unrelated change.

How do I select this flag for the bridge chip? Assume if I did select DRM_CONNECTOR_POLL_HPD, where to call drm_helper_hpd_irq_event in the driver? Is post_encoder_init a right place?

Without the polled flag, I get display very late. Please throw some light on this!

I just don't understand why it's necessary to implement this field in the drm_bridge. Every bridge driver will already implement a connector, in which case it can simply set the connector's .polled field, can't it?

It seems like the only reason you have it in drm_bridge is so that the encoder driver can set it. But I don't see why it should be doing that. The polled state is a property of the connector, and the encoder driver doesn't know anything about it. So if the bridge has a way to detect HPD then it should be setting up the connector to properly report it. For example if the bridge has an input pin to detect it, then it could use a GPIO to receive interrupts and call drm_helper_hpd_irq_event() in the interrupt handler.

Hmm. Are we allowed to call drm_helper_hpd_irq_event() the way DSI panels use it? Like the last step in panel probe? For bridges, it will be in post_encoder_init!

Perhaps you can explain the exact setup where you need this (or point me at the code since I can't seem to find the relevant location) so that I can gain a better understanding.

I can see bridge getting detected only when I set polled member of bridge connector to DRM_CONNECTOR_POLL_HPD, because exynos_drm also calls drm_helper_hpd_irq_event() to force detect all connectors at the end of drm_load.

If I don't set the polled member, I see bridge getting detected after quite sometime.

Ajay

On Sat, Jul 26, 2014 at 12:52:09AM +0530, Ajay Kumar wrote: [...]

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 1e2f96c..0b12d16 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -6,4 +6,14 @@ config DRM_BRIDGE

menu "bridge chips" depends on DRM_BRIDGE

+config DRM_PTN3460

• tristate "NXP ptn3460 eDP/LVDS bridge"
• depends on DRM && DRM_BRIDGE

I don't think you need these two dependencies any longer since they are implicit in the "bridge chips" menu.

diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c

[...]

+#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_gpio.h> +#include <linux/i2c.h> +#include <linux/gpio.h> +#include <linux/delay.h> +#include <drm/drm_panel.h>

These should be ordered alphabetically, but they are already this way in the original driver, so the reordering can be a separate patch.

+struct ptn3460_bridge {

• struct drm_connector connector;
• struct i2c_client *client;
• struct drm_bridge *bridge;

I think it would be much more natural for ptn3460_bridge to embed struct drm_bridge rather than store a pointer to it.

• struct drm_panel *panel;
• struct edid *edid;
• struct gpio_desc *gpio_pd_n;
• struct gpio_desc *gpio_rst_n;
• u32 edid_emulation;
• bool enabled;

+};

+static int ptn3460_read_bytes(struct ptn3460_bridge *ptn_bridge, char addr,

• u8 *buf, int len)
  

+{

• int ret;
• ret = i2c_master_send(ptn_bridge->client, &addr, 1);
• if (ret <= 0) {

• DRM_ERROR("Failed to send i2c command, ret=%d\n", ret);
  

• return ret;
  

• }
• ret = i2c_master_recv(ptn_bridge->client, buf, len);
• if (ret <= 0) {

• DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret);
  

• return ret;
  

• }

This isn't introduced by this patch, but doesn't this require locking so that this is an atomic transaction?

Perhaps it could be rewritten using i2c_smbus_read_block_data()?

+static int ptn3460_write_byte(struct ptn3460_bridge *ptn_bridge, char addr,

• char val)
  

+{

• int ret;
• char buf[2];
• buf[0] = addr;
• buf[1] = val;
• ret = i2c_master_send(ptn_bridge->client, buf, ARRAY_SIZE(buf));
• if (ret <= 0) {

• DRM_ERROR("Failed to send i2c command, ret=%d\n", ret);
  

• return ret;
  

• }
• return 0;

+}

Same here, this looks like it could be i2c_smbus_write_byte_data().

+static int ptn3460_select_edid(struct ptn3460_bridge *ptn_bridge) +{

• int ret;
• char val;
• /* Load the selected edid into SRAM (accessed at PTN3460_EDID_ADDR) */
• ret = ptn3460_write_byte(ptn_bridge, PTN3460_EDID_SRAM_LOAD_ADDR,

• 	ptn_bridge->edid_emulation);
  

• if (ret) {

• DRM_ERROR("Failed to transfer edid to sram, ret=%d\n", ret);
  

• return ret;
  

• }
• /* Enable EDID emulation and select the desired EDID */
• val = 1 << PTN3460_EDID_ENABLE_EMULATION |

• ptn_bridge->edid_emulation << PTN3460_EDID_EMULATION_SELECTION;
  

• ret = ptn3460_write_byte(ptn_bridge, PTN3460_EDID_EMULATION_ADDR, val);
• if (ret) {

• DRM_ERROR("Failed to write edid value, ret=%d\n", ret);
  

• return ret;
  

• }
• return 0;

+}

s/edid/EDID/ in the above (and below, too), but again the original driver had this, so it can be a separate patch.

+static void ptn3460_pre_enable(struct drm_bridge *bridge) +{

• struct ptn3460_bridge *ptn_bridge = bridge->driver_private;

If you embed drm_bridge within ptn3460_bridge then you can use the much more canonical container_of() macro (or rather a driver-specific inline function that wraps it) and no longer need the drm_bridge.driver_private field.

• int ret;
• if (ptn_bridge->enabled)

• return;
  

• gpiod_set_value(ptn_bridge->gpio_pd_n, 1);
• gpiod_set_value(ptn_bridge->gpio_rst_n, 0);
• udelay(10);

This shouldn't be using udelay(), usleep_range(10, 20) (or similar) would be better. Again, can be a separate patch.

• gpiod_set_value(ptn_bridge->gpio_rst_n, 1);

It also seems like you've converted to using the gpiod_*() API, but the driver previously used gpio_is_valid() to check that both PD and RST pins had valid GPIOs associated with them. The device tree binding said that they are required, though.

So this patch actually does the right thing by making them non-optional but it also changes behaviour from the original. Like I said earlier, I would very much prefer that this conversion be split into separate patches rather than one patch that removes the old driver and a second patch that adds a new one. It makes it really difficult to tell what's really changing, breaks bisectability and generally makes our lives miserable.

• drm_panel_prepare(ptn_bridge->panel);

This should check for errors.

+static void ptn3460_enable(struct drm_bridge *bridge) +{

• struct ptn3460_bridge *ptn_bridge = bridge->driver_private;
• drm_panel_enable(ptn_bridge->panel);

Should check for errors as well.

+int ptn3460_get_modes(struct drm_connector *connector)

static? There seem to be quite a few functions that can be locally scoped. Again, this seems to be the case in the original driver as but it should definitely be fixed at some point.

+{

• struct ptn3460_bridge *ptn_bridge;
• u8 *edid;
• int ret, num_modes;
• bool power_off;
• ptn_bridge = container_of(connector, struct ptn3460_bridge, connector);
• if (ptn_bridge->edid)

• return drm_add_edid_modes(connector, ptn_bridge->edid);
  

• power_off = !ptn_bridge->enabled;
• ptn3460_pre_enable(ptn_bridge->bridge);
• edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
• if (!edid) {

• DRM_ERROR("Failed to allocate edid\n");
  

• return 0;
  

• }
• ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid,

• 	EDID_LENGTH);
  

• if (ret) {

• kfree(edid);
  

• num_modes = 0;
  

Maybe instead of doing this here you can initialize the variable when you declare it? It's always been that way, so can be a separate patch, too.

+struct drm_encoder *ptn3460_best_encoder(struct drm_connector *connector) +{

• struct ptn3460_bridge *ptn_bridge;
• ptn_bridge = container_of(connector, struct ptn3460_bridge, connector);

You use this long construct a couple of times, so it's useful to introduce a helper, such as this:

static inline struct ptn3460_bridge * connector_to_ptn3460(struct drm_connector *connector) { return container_of(connector, struct ptn3460_bridge, connector); }

+int ptn3460_post_encoder_init(struct drm_bridge *bridge) +{

• struct ptn3460_bridge *ptn_bridge = bridge->driver_private;
• int ret;
• /* bridge is attached to encoder.

• * safe to remove it from the bridge_lookup table.
  

• */
  

• drm_bridge_remove_from_lookup(bridge);

No, you should never do this. First, you're not adding it back to the registry when the bridge is detached, so unloading and reloading the display driver will fail. Second there should never be a need to remove it from the registry as long as the driver itself is loaded. If you're concerned about a single bridge being used multiple times, there's already code to handle that in your previous patch:

int drm_bridge_attach_encoder(...) { ...

if (bridge->encoder) return -EBUSY;

... }

Generally the registry should contain a list of bridges that have been registered, whether they're used or not is irrelevant.

• ret = drm_bridge_init(bridge->drm_dev, bridge);
• if (ret) {

• DRM_ERROR("Failed to initialize bridge with drm\n");
  

• return ret;
  

• }
• /* connector implementation */
• ptn_bridge->connector.polled = bridge->connector_polled;

Why does this need to be handed from bridge to connector? You implement both the connector and the bridge in this driver, so can't you directly set ptn_bridge->connector.polled as appropriate?

• ret = drm_connector_init(bridge->drm_dev, &ptn_bridge->connector,

• 	&ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
  

• if (ret) {

• DRM_ERROR("Failed to initialize connector with drm\n");
  

• return ret;
  

• }
• drm_connector_helper_add(&ptn_bridge->connector,

• 			&ptn3460_connector_helper_funcs);
  

• drm_connector_register(&ptn_bridge->connector);
• drm_mode_connector_attach_encoder(&ptn_bridge->connector,

• 					bridge->encoder);
  

• if (ptn_bridge->panel)

• drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector);
  

• return ret;

+}

I'm thinking that eventually we'll want to register the connector only when a panel is attached to the bridge. This will only become important when we implement bridge chaining because if you instantiate a connector for each bridge then you'll get a list of connectors for the DRM device representing the output of each bridge rather than just the final one that goes to the display.

+static int ptn3460_probe(struct i2c_client *client,

• 		const struct i2c_device_id *id)
  

+{

• struct device *dev = &(client->dev);

No need for the parentheses here.

• struct device_node *panel_node;
• struct drm_bridge *bridge;
• struct ptn3460_bridge *ptn_bridge;
• int ret;
• bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
• if (!bridge) {

• DRM_ERROR("Failed to allocate drm bridge\n");
  

• return -ENOMEM;
  

• }
• ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL);
• if (!ptn_bridge) {

• DRM_ERROR("Failed to allocate ptn bridge\n");
  

• return -ENOMEM;
  

• }

No need for error messages on allocation failures. The allocator will already complain itself.

Also I think it's usually better to use the dev_*() functions to print messages, especially given that at this stage we're not even hooked up to DRM in the first place.

So in general I try to use DRM_*() functions only from DRM-specific callbacks (or functions called from them) and dev_*() otherwise.

+static int ptn3460_remove(struct i2c_client *client) +{

• return 0;

+}

This function should remove the bridge from the lookup table by calling drm_bridge_remove().

+static const struct i2c_device_id ptn3460_i2c_table[] = {

• {"nxp,ptn3460", 0},
• {},

+}; +MODULE_DEVICE_TABLE(i2c, ptn3460_i2c_table);

+static const struct of_device_id ptn3460_match[] = {

• { .compatible = "nxp,ptn3460" },
• {},

+}; +MODULE_DEVICE_TABLE(of, ptn3460_match);

+struct i2c_driver ptn3460_driver = {

Is there a reason why this can't be static?

• .id_table = ptn3460_i2c_table,
• .probe = ptn3460_probe,
• .remove = ptn3460_remove,
• .driver = {

• .name	= "nxp,ptn3460",
  

• .owner	= THIS_MODULE,
  

• .of_match_table = of_match_ptr(ptn3460_match),
  

You don't need of_match_ptr() here since you already depend on OF in Kconfig, therefore of_match_ptr(x) will always evaluate to x.

• },

+}; +module_i2c_driver(ptn3460_driver);

+MODULE_AUTHOR("Sean Paul seanpaul@chromium.org"); +MODULE_DESCRIPTION("NXP ptn3460 eDP-LVDS converter driver"); +MODULE_LICENSE("GPL");

This should be "GPL v2".

Thierry

Am 25.07.2014 21:22, schrieb Ajay Kumar:

From: Vincent Palatin vpalatin@chromium.org

This patch adds drm_bridge driver for parade DisplayPort to LVDS bridge chip.

Signed-off-by: Vincent Palatin vpalatin@chromium.org Signed-off-by: Andrew Bresticker abrestic@chromium.org Signed-off-by: Sean Paul seanpaul@chromium.org Signed-off-by: Rahul Sharma rahul.sharma@samsung.com Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com

---

.../devicetree/bindings/vendor-prefixes.txt | 1 + .../devicetree/bindings/video/bridge/ps8622.txt | 19 + drivers/gpu/drm/bridge/Kconfig | 10 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/ps8622.c | 602 ++++++++++++++++++++ 5 files changed, 633 insertions(+) create mode 100644 Documentation/devicetree/bindings/video/bridge/ps8622.txt create mode 100644 drivers/gpu/drm/bridge/ps8622.c

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt index 46a311e..b4a99cc 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt @@ -96,6 +96,7 @@ nxp NXP Semiconductors onnn ON Semiconductor Corp. opencores OpenCores.org panasonic Panasonic Corporation +parade Parade Technologies Inc. phytec PHYTEC Messtechnik GmbH picochip Picochip Ltd plathome Plat'Home Co., Ltd. diff --git a/Documentation/devicetree/bindings/video/bridge/ps8622.txt b/Documentation/devicetree/bindings/video/bridge/ps8622.txt new file mode 100644 index 0000000..fdeafb2 --- /dev/null +++ b/Documentation/devicetree/bindings/video/bridge/ps8622.txt @@ -0,0 +1,19 @@ +ps8622-bridge bindings

+Required properties:

• • compatible: "parade,ps8622" or "parade,ps8625"
• • reg: first i2c address of the bridge
• • sleep-gpios: OF device-tree gpio specification
• • reset-gpios: OF device-tree gpio specification

+Optional properties:

• • lane-count: number of DP lanes to use

+Example:

• ps8622-bridge@48 {

Nit: Shouldn't this be lvds-bridge like in 7/8 or something else not derived from the specific model? Applies to the DT series as well.

• compatible = "parade,ps8622";
  

• reg = <0x48>;
  

• sleep-gpios = <&gpc3 6 1 0 0>;
  

• reset-gpios = <&gpc3 1 1 0 0>;
  

• lane-count = <1>
  

• };

[...]

diff --git a/drivers/gpu/drm/bridge/ps8622.c b/drivers/gpu/drm/bridge/ps8622.c new file mode 100644 index 0000000..ec60fcf --- /dev/null +++ b/drivers/gpu/drm/bridge/ps8622.c @@ -0,0 +1,602 @@ +/*

• • Parade PS8622 eDP/LVDS bridge driver
• • Copyright (C) 2014 Google, Inc.
• • This software is licensed under the terms of the GNU General Public
• • License version 2, as published by the Free Software Foundation, and
• • may be copied, distributed, and modified under those terms.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• */

[...]

+MODULE_AUTHOR("Vincent Palatin vpalatin@chromium.org"); +MODULE_DESCRIPTION("Parade ps8622 eDP-LVDS converter driver"); +MODULE_LICENSE("GPL");

"GPL v2"?

Regards, Andreas

On Sat, Jul 26, 2014 at 12:52:10AM +0530, Ajay Kumar wrote: [...]

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt index 46a311e..b4a99cc 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt @@ -96,6 +96,7 @@ nxp NXP Semiconductors onnn ON Semiconductor Corp. opencores OpenCores.org panasonic Panasonic Corporation +parade Parade Technologies Inc. phytec PHYTEC Messtechnik GmbH picochip Picochip Ltd plathome Plat'Home Co., Ltd.

I think it's a good idea to turn this into a separate patch to avoid conflicts.

diff --git a/Documentation/devicetree/bindings/video/bridge/ps8622.txt b/Documentation/devicetree/bindings/video/bridge/ps8622.txt new file mode 100644 index 0000000..fdeafb2 --- /dev/null +++ b/Documentation/devicetree/bindings/video/bridge/ps8622.txt @@ -0,0 +1,19 @@ +ps8622-bridge bindings

+Required properties:

• • compatible: "parade,ps8622" or "parade,ps8625"
• • reg: first i2c address of the bridge

I'm not sure what the deal is with devices that use more than one I2C addresses. It would be good to hear from the device tree maintainers on how to handle those. Technically each address is a separate device. This is also mirrored in the Linux kernel's implementation of the I2C bus, where it looks wrong to access more than a single address (since the core only marks one of them used). So technically it's valid for userspace to access any but the first "page" of this device.

• • sleep-gpios: OF device-tree gpio specification
• • reset-gpios: OF device-tree gpio specification

This should explain what these GPIOs are used for.

+Optional properties:

• • lane-count: number of DP lanes to use

+Example:

• ps8622-bridge@48 {

• compatible = "parade,ps8622";
  

• reg = <0x48>;
  

• sleep-gpios = <&gpc3 6 1 0 0>;
  

• reset-gpios = <&gpc3 1 1 0 0>;
  

• lane-count = <1>
  

• };

Same here. It's usually best to make this a patch separate from the driver. Not because of conflicts, but because it makes it easier for DT reviewers to find the relevant pieces to look at.

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 0b12d16..d73e474 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -16,4 +16,14 @@ config DRM_PTN3460 help ptn3460 eDP-LVDS bridge chip driver.

+config DRM_PS8622

• tristate "Parade eDP/LVDS bridge"
• depends on DRM && DRM_BRIDGE

Aagin, these are unnecessary.

diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index b4733e1..d1b5daa 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -1,3 +1,4 @@ ccflags-y := -Iinclude/drm

obj-$(CONFIG_DRM_PTN3460) += ptn3460.o +obj-$(CONFIG_DRM_PS8622) += ps8622.o

Please keep these sorted alphabetically.

diff --git a/drivers/gpu/drm/bridge/ps8622.c b/drivers/gpu/drm/bridge/ps8622.c

[...]

+#include <linux/module.h> +#include <linux/backlight.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/fb.h> +#include <linux/gpio.h> +#include <linux/i2c.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/pm.h> +#include <linux/regulator/consumer.h>

These too.

+struct ps8622_bridge {

• struct drm_connector connector;
• struct i2c_client *client;
• struct drm_bridge *bridge;

Should be embedded.

• struct drm_panel *panel;
• struct regulator *v12;
• struct backlight_device *bl;
• struct mutex enable_mutex;

I don't quite understand what this protects. Can you explain?

+struct ps8622_device_data {

• u8 max_lane_count;

+};

+static const struct ps8622_device_data ps8622_data = {

• .max_lane_count = 1,

+};

+static const struct ps8622_device_data ps8625_data = {

• .max_lane_count = 2,

+};

+/* Brightness scale on the Parade chip */ +#define PS8622_MAX_BRIGHTNESS 0xff

+/* Timings taken from the version 1.7 datasheet for the PS8622/PS8625 */ +#define PS8622_POWER_RISE_T1_MIN_US 10 +#define PS8622_POWER_RISE_T1_MAX_US 10000 +#define PS8622_RST_HIGH_T2_MIN_US 3000 +#define PS8622_RST_HIGH_T2_MAX_US 30000 +#define PS8622_PWMO_END_T12_MS 200 +#define PS8622_POWER_FALL_T16_MAX_US 10000 +#define PS8622_POWER_OFF_T17_MS 500

+#if ((PS8622_RST_HIGH_T2_MIN_US + PS8622_POWER_RISE_T1_MAX_US) > \

• (PS8622_RST_HIGH_T2_MAX_US + PS8622_POWER_RISE_T1_MIN_US))

+#error "T2.min + T1.max must be less than T2.max + T1.min" +#endif

+static int ps8622_set(struct i2c_client *client, u8 page, u8 reg, u8 val) +{

• int ret;
• struct i2c_adapter *adap = client->adapter;
• struct i2c_msg msg;
• u8 data[] = {reg, val};
• msg.addr = client->addr + page;
• msg.flags = 0;
• msg.len = sizeof(data);
• msg.buf = data;
• ret = i2c_transfer(adap, &msg, 1);
• if (ret != 1)

• pr_warn("PS8622 I2C write (0x%02x,0x%02x,0x%02x) failed: %d\n",
  

• 	client->addr + page, reg, val, ret);
  

• return !(ret == 1);

+}

+static int ps8622_send_config(struct ps8622_bridge *ps_bridge) +{

• struct i2c_client *cl = ps_bridge->client;
• int err = 0;
• /* wait 20ms after power ON */
• usleep_range(20000, 30000);

It's unusual to do this here. The function doesn't know when it's being called. It could theoretically be called at any point. Better to move the sleep to after where the device is powered on.

• err |= ps8622_set(cl, 0x02, 0xa1, 0x01); /* HPD low */
• /* SW setting */
• err |= ps8622_set(cl, 0x04, 0x14, 0x01); /* [1:0] SW output 1.2V voltage

• 				  * is lower to 96% */
  

I'm not at all a fan of this kind of error chaining because you loose all context about specific errors. Also if there's a systematic error you'll want to abort as early as possible. If the device doesn't respond to the first command sent, then it likely won't respond to the next either. No need to keep trying in that case.

• /* RCO SS setting */
• err |= ps8622_set(cl, 0x04, 0xe3, 0x20); /* [5:4] = b01 0.5%, b10 1%,

• 				  * b11 1.5% */
  

Also I find it very hard to follow these calls. Since you have enough information about what each of these registers does, can't you provide symbolic constants?

It would probably also help to separate these into logical groups (or individual function calls if there are no meaningful groups). That way you'll get automatic documentation via function names and this function would look a lot less cluttered.

+static int ps8622_backlight_get(struct backlight_device *bl) +{

• return bl->props.brightness;

+}

Backlight devices no longer need this kind of trivial implementation. It is what the backlight core will do by default if the driver doesn't implement .get_brightness().

+static void ps8622_pre_enable(struct drm_bridge *bridge) +{

• struct ps8622_bridge *ps_bridge = bridge->driver_private;
• int ret;
• mutex_lock(&ps_bridge->enable_mutex);
• if (ps_bridge->enabled)

• goto out;
  

• gpiod_set_value(ps_bridge->gpio_rst_n, 0);
• if (ps_bridge->v12) {

• ret = regulator_enable(ps_bridge->v12);
  

• if (ret)
  

• 	DRM_ERROR("fails to enable ps_bridge->v12");
  

• }
• drm_panel_prepare(ps_bridge->panel);
• gpiod_set_value(ps_bridge->gpio_slp_n, 1);

This uses high-active logic for the sleep pin, so shouldn't it rather be named "gpio_slp"?

• /*

• * T1 is the range of time that it takes for the power to rise after we
  

• * enable the lcd fet.
  

Is that the time it takes for the FET's power to rise? In that case it would be a property of the FET, not the bridge.

                         T2 is the range of time in which the data sheet

• * specifies we should deassert the reset pin.
  

• *
  

• * If it takes T1.max for the power to rise, we need to wait atleast
  

• * T2.min before deasserting the reset pin. If it takes T1.min for the
  

• * power to rise, we need to wait at most T2.max before deasserting the
  

• * reset pin.
  

• */
  

Note that to my knowledge none of the sleep functions give you any guarantees about the maximum amount of time they'll sleep, only the minimum. So there's no such thing as "wait at most" in Linux.

• usleep_range(PS8622_RST_HIGH_T2_MIN_US + PS8622_POWER_RISE_T1_MAX_US,

•      PS8622_RST_HIGH_T2_MAX_US + PS8622_POWER_RISE_T1_MIN_US);
  

• gpiod_set_value(ps_bridge->gpio_rst_n, 1);

This also uses high-active logic.

• ret = ps8622_send_config(ps_bridge);
• if (ret)

• DRM_ERROR("Failed to send config to bridge (%d)\n", ret);
  

Isn't this a fatal error? What will be the consequences if this doesn't succeed? I guess it doesn't matter much either way since the API can't propagate errors at this point anyway...

+static void ps8622_enable(struct drm_bridge *bridge) +{

• struct ps8622_bridge *ps_bridge = bridge->driver_private;
• mutex_lock(&ps_bridge->enable_mutex);
• drm_panel_enable(ps_bridge->panel);
• mutex_unlock(&ps_bridge->enable_mutex);

What's the enable_mutex protecting here?

+static void ps8622_disable(struct drm_bridge *bridge) +{

• struct ps8622_bridge *ps_bridge = bridge->driver_private;
• mutex_lock(&ps_bridge->enable_mutex);
• if (!ps_bridge->enabled)

• goto out;
  

• ps_bridge->enabled = false;
• drm_panel_disable(ps_bridge->panel);
• msleep(PS8622_PWMO_END_T12_MS);
• /*

• * This doesn't matter if the regulators are turned off, but something
  

• * else might keep them on. In that case, we want to assert the slp gpio
  

• * to lower power.
  

• */
  

• gpiod_set_value(ps_bridge->gpio_slp_n, 0);
• if (ps_bridge->v12)

• regulator_disable(ps_bridge->v12);
  

• /*

• * Sleep for at least the amount of time that it takes the power rail to
  

• * fall to prevent asserting the rst gpio from doing anything.
  

• */
  

• usleep_range(PS8622_POWER_FALL_T16_MAX_US,

•      2 * PS8622_POWER_FALL_T16_MAX_US);
  

• gpiod_set_value(ps_bridge->gpio_rst_n, 0);

Does this even matter? Why would it be bad if the reset was asserted before power went away? Isn't the end result that the chip will be off and reset to an initial state the next time it's power up anyway?

+static enum drm_connector_status ps8622_detect(struct drm_connector *connector,

• bool force)
  

Alignment here is slightly odd. We usually follow (in DRM and many other parts of the kernel) the convention of aligning arguments on subsequent lines with the first argument on the first line.

+int ps8622_post_encoder_init(struct drm_bridge *bridge) +{

• struct ps8622_bridge *ps_bridge = bridge->driver_private;
• int ret;
• /* bridge is attached to encoder.

• * safe to remove it from the bridge_lookup table.
  

• */
  

• drm_bridge_remove_from_lookup(bridge);

Like I said for the ptn3460 driver, this should not be here.

• ret = drm_bridge_init(bridge->drm_dev, bridge);
• if (ret) {

• DRM_ERROR("Failed to initialize bridge with drm\n");
  

• return ret;
  

• }

And this should probably happen somewhere else to avoid having to do it for every driver.

+static const struct of_device_id ps8622_devices[] = {

• {

• .compatible = "parade,ps8622",
  

• .data	= &ps8622_data,
  

• }, {

• .compatible = "parade,ps8625",
  

• .data	= &ps8625_data,
  

• }, {

The above uses a mix of spaces and tabs for padding (tabs between .data and =). Single spaces will do.

• /* end node */
  

Nit: it's not a node, it's an entry or element.

• }

+}; +MODULE_DEVICE_TABLE(of, ps8622_devices);

+static inline struct ps8622_device_data *get_device_data(struct device *dev) +{

• const struct of_device_id *match =

• 	of_match_device(ps8622_devices, dev);
  

• return (struct ps8622_device_data *)match->data;

+}

Please drop this. First of all, match->data is void *, so there's no need for the explicit casting. Second, match->data is also const, so you're effectively casting that away.

+static int ps8622_probe(struct i2c_client *client,

• 			const struct i2c_device_id *id)
  

+{

• struct device *dev = &(client->dev);

No need for the parentheses here.

• struct device_node *panel_node, *backlight_node;
• struct drm_bridge *bridge;
• struct backlight_device *backlight_dev;
• struct ps8622_bridge *ps_bridge;

Have you considered naming this simply "ps" to make it shorter? There's some weird formatting below just because this is a very long variable name.

• const struct ps8622_device_data *device_data;
• int ret;

It's just as easy (and shorter) to do this here:

const struct ps8622_device_data *data; const struct of_device_id *match;

match = of_match_device(ps8622_devices, dev); data = match->data;

• bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
• if (!bridge) {

• DRM_ERROR("Failed to allocate drm bridge\n");
  

• return -ENOMEM;
  

• }
• ps_bridge = devm_kzalloc(dev, sizeof(*ps_bridge), GFP_KERNEL);
• if (!ps_bridge) {

• DRM_ERROR("could not allocate ps bridge\n");
  

• return -ENOMEM;
  

• }
• panel_node = of_parse_phandle(dev->of_node, "panel", 0);
• if (panel_node) {

• ps_bridge->panel = of_drm_find_panel(panel_node);
  

• of_node_put(panel_node);
  

• if (!ps_bridge->panel)
  

• 	return -EPROBE_DEFER;
  

• }
• backlight_dev = NULL;

It's more idiomatic to initialize these when you declare them to avoid sprinkling them across the code like this.

• backlight_node = of_parse_phandle(dev->of_node, "backlight", 0);
• if (backlight_node) {

• backlight_dev = of_find_backlight_by_node(backlight_node);
  

• of_node_put(backlight_node);
  

• if (!backlight_dev)
  

• 	return -EPROBE_DEFER;
  

It seems like in this case you aren't storing the backlight anywhere. Is there a reason for that? Is it because the same backlight is used by the panel driver and it does the controlling from there? If so it might be better to use a boolean property here rather than a phandle to another backlight device. Using a phandle usually signal that you do want to use the device in some way.

• }
• mutex_init(&ps_bridge->enable_mutex);
• bridge->dev = dev;
• bridge->driver_private = ps_bridge;
• bridge->funcs = &ps8622_bridge_funcs;
• ps_bridge->client = client;
• ps_bridge->bridge = bridge;
• device_data = get_device_data(dev);
• ps_bridge->v12 = devm_regulator_get(&client->dev, "vdd_bridge");

vdd_bridge is not a valid name for a regulator. This will translate to vdd_bridge-supply for a DT property and you shouldn't be using underscores for them. Also this property isn't documented in the binding.

• if (IS_ERR(ps_bridge->v12)) {

• DRM_INFO("no 1.2v regulator found for PS8622\n");
  

• ps_bridge->v12 = NULL;
  

• }

Does this ever happen? As far as I know the regulator core will give you a dummy regulator when booting using DT, so errors returned here will always be real errors that you likely won't want to ignore.

• ps_bridge->gpio_slp_n = devm_gpiod_get(&client->dev, "sleep");
• if (IS_ERR(ps_bridge->gpio_slp_n)) {

• ret = PTR_ERR(ps_bridge->gpio_slp_n);
  

• DRM_ERROR("cannot get gpio_slp_n %d\n", ret);
  

• goto err_client;
  

• } else {

• ret = gpiod_direction_output(ps_bridge->gpio_slp_n, 1);
  

• if (ret) {
  

• 	DRM_ERROR("cannot configure gpio_slp_n\n");
  

• 	goto err_client;
  

• }
  

• }
• ps_bridge->gpio_rst_n = devm_gpiod_get(&client->dev, "reset");
• if (IS_ERR(ps_bridge->gpio_rst_n)) {

• ret = PTR_ERR(ps_bridge->gpio_rst_n);
  

• DRM_ERROR("cannot get gpio_rst_n %d\n", ret);
  

• goto err_client;
  

• } else {

• /*
  

•  * Assert the reset pin high to avoid the bridge being
  

•  * initialized prematurely
  

•  */
  

• ret = gpiod_direction_output(ps_bridge->gpio_rst_n, 1);
  

• if (ret) {
  

• 	DRM_ERROR("cannot configure gpio_slp_n\n");
  

• 	goto err_client;
  

• }
  

• }

Just as a side-note, there's an API under discussion that will allow gpio_desc's to be configured at the same time as they are requested. This driver should probably convert to that API.

• ps_bridge->max_lane_count = device_data->max_lane_count;
• if (of_property_read_u8(dev->of_node, "lane-count",

• 				&ps_bridge->lane_count))
  

• ps_bridge->lane_count = ps_bridge->max_lane_count;
  

• else if (ps_bridge->lane_count > ps_bridge->max_lane_count) {

• DRM_INFO("lane-count property is too high for DP bridge\n");
  

• ps_bridge->lane_count = ps_bridge->max_lane_count;
  

• }

Perhaps this should clarify that you're falling back to max_lane_count.

• if (!backlight_dev) {

• ps_bridge->bl = backlight_device_register("ps8622-backlight",
  

• 		dev, ps_bridge, &ps8622_backlight_ops,
  

• 		NULL);
  

• if (IS_ERR(ps_bridge->bl)) {
  

• 	DRM_ERROR("failed to register backlight\n");
  

• 	ret = PTR_ERR(ps_bridge->bl);
  

• 	ps_bridge->bl = NULL;
  

No need to set this to NULL since you'll return with an error and ps_bridge will be freed.

• 	goto err_client;
  

• }
  

• ps_bridge->bl->props.max_brightness = PS8622_MAX_BRIGHTNESS;
  

• ps_bridge->bl->props.brightness = PS8622_MAX_BRIGHTNESS;
  

Perhaps you want to initialize bl->props.power to FB_BLANK_POWERDOWN at this point. The reason is that if you don't then the backlight will be on by default (FB_BLANK_UNBLANK == 0). Many backlight drivers do call backlight_update_status() after registering with the core to set the initial state. It looks like you're not doing that here, but it might be better to still set this to reflect what the initial state should be.

I'm assuming you want it disabled to avoid visual glitches when you turn on the display. Of course if the bootloader already turned it on and initialized the display, then you'll get flickering... but I guess that's a problem to solve another day.

• }
• i2c_set_clientdata(client, ps_bridge);
• drm_bridge_add_for_lookup(bridge);
• return 0;

+err_client:

• DRM_ERROR("device probe failed : %d\n", ret);

No need for this. The driver core typically tells you already.

+static int ps8622_remove(struct i2c_client *client) +{

• struct ps8622_bridge *ps_bridge = i2c_get_clientdata(client);
• if (ps_bridge->bl)

• backlight_device_unregister(ps_bridge->bl);
  

• return 0;

+}

You'll also want to call drm_bridge_remove() here.

+static const struct i2c_device_id ps8622_i2c_table[] = {

• {"parade", 0},
• {},

+}; +MODULE_DEVICE_TABLE(i2c, ps8622_i2c_table);

This table doesn't look right. Shouldn't that be:

{ "ps8622", 0 }, { "ps8625", 0 },

? And perhaps use the .driver_data field to refer to the device data like for the of_device_id table?

+struct i2c_driver ps8622_driver = {

• .id_table = ps8622_i2c_table,
• .probe = ps8622_probe,
• .remove = ps8622_remove,
• .driver = {

• .name	= "parade",
  

That's an awfully generic name. I'd suggest "ps8622".

• .owner	= THIS_MODULE,
  

• .of_match_table = of_match_ptr(ps8622_devices),
  

As for ptn3460, this has a hard dependency on OF, so of_match_ptr() isn't useful.

• },

+}; +module_i2c_driver(ps8622_driver);

+MODULE_AUTHOR("Vincent Palatin vpalatin@chromium.org"); +MODULE_DESCRIPTION("Parade ps8622 eDP-LVDS converter driver"); +MODULE_LICENSE("GPL");

"GPL v2"

Thierry

On Tue, Jul 29, 2014 at 01:21:48PM +0200, Andreas Färber wrote:

Hi Ajay,

Am 28.07.2014 08:13, schrieb Ajay kumar:

...

On 7/27/14, Andreas Färber afaerber@suse.de wrote:

...

Am 25.07.2014 21:22, schrieb Ajay Kumar:

...

This series is based on exynos-drm-next branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git

I have tested this after adding few DT changes for exynos5250-snow, exynos5420-peach-pit and exynos5800-peach-pi boards.

I'm trying to test this with a modified exynos5250-spring DT based off kgene's for-next branch due to DT, and I run into the following:

CC drivers/gpu/drm/bridge/ptn3460.o drivers/gpu/drm/bridge/ptn3460.c: In function ‘ptn3460_post_encoder_init’: drivers/gpu/drm/bridge/ptn3460.c:275:2: error: implicit declaration of function ‘drm_connector_register’ [-Werror=implicit-function-declaration] drm_connector_register(&ptn_bridge->connector); ^

Hope this might help: http://www.spinics.net/lists/dri-devel/msg60578.html

That fixed my build, thanks.

Unfortunately the most I got on Spring with attached DT was a blank screen with a white horizontal line in the middle.

Do I need to specify a specific panel model for Spring?

For testing I re-applied your iommu patches (which btw fail now for 5420 due to disp_pd) but didn't know what to do about your panel-lvds regulator patch now that it's gone.

If I don't apply this series, then commenting out the dp-controller node gets me a working display with simplefb as before.

Regards, Andreas

-- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

From 9172a26a8f0d0f0d170bd27e1c150ad204d8086a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20F=C3=A4rber?= afaerber@suse.de Date: Sun, 27 Jul 2014 21:58:06 +0200 Subject: [PATCH] ARM: dts: exynos5250: Add eDP/LVDS bridge to Spring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit

Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com [AF: Redone for v6] Signed-off-by: Andreas F??rber afaerber@suse.de

---

arch/arm/boot/dts/exynos5250-spring.dts | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos5250-spring.dts b/arch/arm/boot/dts/exynos5250-spring.dts index 687dfab86bc8..517b1ff2bfdf 100644 --- a/arch/arm/boot/dts/exynos5250-spring.dts +++ b/arch/arm/boot/dts/exynos5250-spring.dts @@ -64,10 +64,14 @@ vdd_pll-supply = <&s5m_ldo8_reg>; };

• panel: panel {

• compatible = "simple-panel";
  

• };

You can't do this. "simple-panel" isn't a valid panel model. It should probably be removed from the platform_of_match table in the driver.

Thierry

On Tue, Jul 29, 2014 at 01:42:09PM +0200, Andreas Färber wrote:

Am 29.07.2014 13:36, schrieb Thierry Reding:

...

On Tue, Jul 29, 2014 at 01:21:48PM +0200, Andreas Färber wrote:

...

Hi Ajay,

Am 28.07.2014 08:13, schrieb Ajay kumar:

...

On 7/27/14, Andreas Färber afaerber@suse.de wrote:

...

Am 25.07.2014 21:22, schrieb Ajay Kumar:

...

This series is based on exynos-drm-next branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git

I have tested this after adding few DT changes for exynos5250-snow, exynos5420-peach-pit and exynos5800-peach-pi boards.

I'm trying to test this with a modified exynos5250-spring DT

[...]

...

...

Unfortunately the most I got on Spring with attached DT was a blank screen with a white horizontal line in the middle.

Do I need to specify a specific panel model for Spring?

[...]

...

...

From 9172a26a8f0d0f0d170bd27e1c150ad204d8086a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20F=C3=A4rber?= afaerber@suse.de Date: Sun, 27 Jul 2014 21:58:06 +0200 Subject: [PATCH] ARM: dts: exynos5250: Add eDP/LVDS bridge to Spring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit

Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com [AF: Redone for v6] Signed-off-by: Andreas F??rber afaerber@suse.de

---

arch/arm/boot/dts/exynos5250-spring.dts | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos5250-spring.dts b/arch/arm/boot/dts/exynos5250-spring.dts index 687dfab86bc8..517b1ff2bfdf 100644 --- a/arch/arm/boot/dts/exynos5250-spring.dts +++ b/arch/arm/boot/dts/exynos5250-spring.dts @@ -64,10 +64,14 @@ vdd_pll-supply = <&s5m_ldo8_reg>; };

• panel: panel {

• compatible = "simple-panel";
  

• };

You can't do this. "simple-panel" isn't a valid panel model. It should probably be removed from the platform_of_match table in the driver.

Okay, that means the Snow DT is wrong, too: https://patchwork.kernel.org/patch/4625441/

And the others specify it as fallback: https://patchwork.kernel.org/patch/4625461/ https://patchwork.kernel.org/patch/4625451/

A quick grep shows that many (all?) devices that use DRM panels provide simple-panel as fallback. That's probably fine as long as they also do provide the specific model. But given that simple-panel does not have a mode or physical size, I don't think even that makes sense.

Any of the DTS files in the tree I have that list simple-panel as a fallback are Tegra, so I'll go write a patch that removes the fallback. I can't think of a reason why it would ever be needed or meaningful.

Thierry

On Wed, Jul 30, 2014 at 11:54:00AM +0530, Ajay kumar wrote:

Hi Thierry,

On Tue, Jul 29, 2014 at 5:17 PM, Thierry Reding thierry.reding@gmail.com wrote:

...

On Tue, Jul 29, 2014 at 01:42:09PM +0200, Andreas Färber wrote:

...

Am 29.07.2014 13:36, schrieb Thierry Reding:

...

On Tue, Jul 29, 2014 at 01:21:48PM +0200, Andreas Färber wrote:

...

Hi Ajay,

Am 28.07.2014 08:13, schrieb Ajay kumar:

...

On 7/27/14, Andreas Färber afaerber@suse.de wrote: > Am 25.07.2014 21:22, schrieb Ajay Kumar: >> This series is based on exynos-drm-next branch of Inki Dae's tree at: >> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git >> >> I have tested this after adding few DT changes for exynos5250-snow, >> exynos5420-peach-pit and exynos5800-peach-pi boards. > > I'm trying to test this with a modified exynos5250-spring DT

[...]

...

...

Unfortunately the most I got on Spring with attached DT was a blank screen with a white horizontal line in the middle.

Do I need to specify a specific panel model for Spring?

[...]

...

...

From 9172a26a8f0d0f0d170bd27e1c150ad204d8086a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20F=C3=A4rber?= afaerber@suse.de Date: Sun, 27 Jul 2014 21:58:06 +0200 Subject: [PATCH] ARM: dts: exynos5250: Add eDP/LVDS bridge to Spring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit

Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com [AF: Redone for v6] Signed-off-by: Andreas F??rber afaerber@suse.de

---

arch/arm/boot/dts/exynos5250-spring.dts | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos5250-spring.dts b/arch/arm/boot/dts/exynos5250-spring.dts index 687dfab86bc8..517b1ff2bfdf 100644 --- a/arch/arm/boot/dts/exynos5250-spring.dts +++ b/arch/arm/boot/dts/exynos5250-spring.dts @@ -64,10 +64,14 @@ vdd_pll-supply = <&s5m_ldo8_reg>; };

• panel: panel {

•      compatible = "simple-panel";
  

• };

You can't do this. "simple-panel" isn't a valid panel model. It should probably be removed from the platform_of_match table in the driver.

Okay, that means the Snow DT is wrong, too: https://patchwork.kernel.org/patch/4625441/

And the others specify it as fallback: https://patchwork.kernel.org/patch/4625461/ https://patchwork.kernel.org/patch/4625451/

A quick grep shows that many (all?) devices that use DRM panels provide simple-panel as fallback. That's probably fine as long as they also do provide the specific model. But given that simple-panel does not have a mode or physical size, I don't think even that makes sense.

On snow, the bridge chip provides the display mode instead of the panel. That is why display was working for me.

Okay, I suppose under some circumstances that might make sense. Although it's still always the panel that dictates the display timings, so the panel node needs to have a panel model specific compatible value with a matching entry in the panel-simple driver so that it can even be used in setups without a bridge.

One other thing: how does the bridge know which mode to drive? I suspect that it can drive more than one mode? Can it freely be configured or does it have a predefined set of modes? If the latter, then according to what you said above there needs to be a way to configure the bridge (via DT?) so that it reports the mode matching the panel. I wonder if that should be handled completely in code, so that for example a bridge has a panel attached it can use the panel's .get_modes() and select a matching mode among the set that it supports.

Thierry

On Wed, Jul 30, 2014 at 03:54:13PM +0530, Ajay kumar wrote:

On Wed, Jul 30, 2014 at 3:10 PM, Thierry Reding thierry.reding@gmail.com wrote:

...

On Wed, Jul 30, 2014 at 11:54:00AM +0530, Ajay kumar wrote:

...

Hi Thierry,

On Tue, Jul 29, 2014 at 5:17 PM, Thierry Reding thierry.reding@gmail.com wrote:

...

On Tue, Jul 29, 2014 at 01:42:09PM +0200, Andreas Färber wrote:

...

Am 29.07.2014 13:36, schrieb Thierry Reding:

...

On Tue, Jul 29, 2014 at 01:21:48PM +0200, Andreas Färber wrote: > Hi Ajay, > > Am 28.07.2014 08:13, schrieb Ajay kumar: >> On 7/27/14, Andreas Färber afaerber@suse.de wrote: >>> Am 25.07.2014 21:22, schrieb Ajay Kumar: >>>> This series is based on exynos-drm-next branch of Inki Dae's tree at: >>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git >>>> >>>> I have tested this after adding few DT changes for exynos5250-snow, >>>> exynos5420-peach-pit and exynos5800-peach-pi boards. >>> >>> I'm trying to test this with a modified exynos5250-spring DT

[...]

...

> Unfortunately the most I got on Spring with attached DT was a blank > screen with a white horizontal line in the middle. > > Do I need to specify a specific panel model for Spring?

[...]

...

> From 9172a26a8f0d0f0d170bd27e1c150ad204d8086a Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Andreas=20F=C3=A4rber?= afaerber@suse.de > Date: Sun, 27 Jul 2014 21:58:06 +0200 > Subject: [PATCH] ARM: dts: exynos5250: Add eDP/LVDS bridge to Spring > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com > [AF: Redone for v6] > Signed-off-by: Andreas F??rber afaerber@suse.de > --- > arch/arm/boot/dts/exynos5250-spring.dts | 32 +++++++++++++++++++++++++++++++- > 1 file changed, 31 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/exynos5250-spring.dts b/arch/arm/boot/dts/exynos5250-spring.dts > index 687dfab86bc8..517b1ff2bfdf 100644 > --- a/arch/arm/boot/dts/exynos5250-spring.dts > +++ b/arch/arm/boot/dts/exynos5250-spring.dts > @@ -64,10 +64,14 @@ > vdd_pll-supply = <&s5m_ldo8_reg>; > }; > > + panel: panel { > + compatible = "simple-panel"; > + };

You can't do this. "simple-panel" isn't a valid panel model. It should probably be removed from the platform_of_match table in the driver.

Okay, that means the Snow DT is wrong, too: https://patchwork.kernel.org/patch/4625441/

And the others specify it as fallback: https://patchwork.kernel.org/patch/4625461/ https://patchwork.kernel.org/patch/4625451/

A quick grep shows that many (all?) devices that use DRM panels provide simple-panel as fallback. That's probably fine as long as they also do provide the specific model. But given that simple-panel does not have a mode or physical size, I don't think even that makes sense.

On snow, the bridge chip provides the display mode instead of the panel. That is why display was working for me.

Okay, I suppose under some circumstances that might make sense. Although it's still always the panel that dictates the display timings, so the panel node needs to have a panel model specific compatible value with a matching entry in the panel-simple driver so that it can even be used in setups without a bridge.

Well, I am okay with adding the compatible value for specific panel model because "simple-panel" alone cannot provide width/height of the panel.

...

One other thing: how does the bridge know which mode to drive? I suspect that it can drive more than one mode? Can it freely be configured or does it have a predefined set of modes? If the latter, then according to what you said above there needs to be a way to configure the bridge (via DT?) so that it reports the mode matching the panel. I wonder if that should be handled completely in code, so that for example a bridge has a panel attached it can use the panel's .get_modes() and select a matching mode among the set that it supports.

ptn3460 supports a standard list of "edid-emulation" ids. As of now, we receive that as a DT entry. And, these are the list of emulation ids supported:

            | Value | Resolution | Description
            |   0   |  1024x768  | NXP Generic
            |   1   |  1920x1080 | NXP Generic
            |   2   |  1920x1080 | NXP Generic
            |   3   |  1600x900  | Samsung LTM200KT
            |   4   |  1920x1080 | Samsung LTM230HT
            |   5   |  1366x768  | NXP Generic
            |   6   |  1600x900  | ChiMei M215HGE

As you can see, the same resolutions have different emulator ids. May be, it depends on panel vendor also. I am really not sure if we can do this. For snow(which has 1366x768 panel), we set edid-emulation as 5.

Well, modes 1, 2 and 4 as well as modes 3 and 6 must differ in some ways, otherwise there wouldn't be much point in using different IDs for them. You could try to match on more than just the active horizontal and vertical resolution.

The reason behind this is that it would allow us to keep the device tree content to a minimum and determine the proper emulation ID at runtime. But if it's too difficult to implement I won't object to keeping the edid-emulation property in DT. It just means that the device tree will contain some duplicate information that needs to be kept in sync.

Thierry

Hi Laurent,

Please find the latest series here: http://www.spinics.net/lists/dri-devel/msg66740.html

On Wed, Sep 17, 2014 at 3:23 PM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

Hi Thierry,

On Wednesday 30 July 2014 11:40:54 Thierry Reding wrote:

...

On Wed, Jul 30, 2014 at 11:54:00AM +0530, Ajay kumar wrote:

...

On Tue, Jul 29, 2014 at 5:17 PM, Thierry Reding wrote:

...

On Tue, Jul 29, 2014 at 01:42:09PM +0200, Andreas Färber wrote:

...

Am 29.07.2014 13:36, schrieb Thierry Reding:

...

On Tue, Jul 29, 2014 at 01:21:48PM +0200, Andreas Färber wrote: > Am 28.07.2014 08:13, schrieb Ajay kumar: >> On 7/27/14, Andreas Färber afaerber@suse.de wrote: >>> Am 25.07.2014 21:22, schrieb Ajay Kumar: >>>> This series is based on exynos-drm-next branch of Inki Dae's tree >>>> at: >>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos. >>>> git >>>> >>>> I have tested this after adding few DT changes for >>>> exynos5250-snow, >>>> exynos5420-peach-pit and exynos5800-peach-pi boards. >>> >>> I'm trying to test this with a modified exynos5250-spring DT

[...]

...

> Unfortunately the most I got on Spring with attached DT was a blank > screen with a white horizontal line in the middle. > > Do I need to specify a specific panel model for Spring?

[...]

...

> From 9172a26a8f0d0f0d170bd27e1c150ad204d8086a Mon Sep 17 00:00:00 > 2001 > From: =?UTF-8?q?Andreas=20F=C3=A4rber?= afaerber@suse.de > Date: Sun, 27 Jul 2014 21:58:06 +0200 > Subject: [PATCH] ARM: dts: exynos5250: Add eDP/LVDS bridge to Spring > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com > [AF: Redone for v6] > Signed-off-by: Andreas F??rber afaerber@suse.de > --- > > arch/arm/boot/dts/exynos5250-spring.dts | 32 +++++++++++++++++++++- > 1 file changed, 31 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/exynos5250-spring.dts > b/arch/arm/boot/dts/exynos5250-spring.dts index > 687dfab86bc8..517b1ff2bfdf 100644 > --- a/arch/arm/boot/dts/exynos5250-spring.dts > +++ b/arch/arm/boot/dts/exynos5250-spring.dts > @@ -64,10 +64,14 @@ > vdd_pll-supply = <&s5m_ldo8_reg>; > }; > > + panel: panel { > + compatible = "simple-panel"; > + };

You can't do this. "simple-panel" isn't a valid panel model. It should probably be removed from the platform_of_match table in the driver.

Okay, that means the Snow DT is wrong, too: https://patchwork.kernel.org/patch/4625441/

And the others specify it as fallback: https://patchwork.kernel.org/patch/4625461/ https://patchwork.kernel.org/patch/4625451/

A quick grep shows that many (all?) devices that use DRM panels provide simple-panel as fallback. That's probably fine as long as they also do provide the specific model. But given that simple-panel does not have a mode or physical size, I don't think even that makes sense.

On snow, the bridge chip provides the display mode instead of the panel. That is why display was working for me.

Okay, I suppose under some circumstances that might make sense. Although it's still always the panel that dictates the display timings, so the panel node needs to have a panel model specific compatible value with a matching entry in the panel-simple driver so that it can even be used in setups without a bridge.

One other thing: how does the bridge know which mode to drive? I suspect that it can drive more than one mode? Can it freely be configured or does it have a predefined set of modes? If the latter, then according to what you said above there needs to be a way to configure the bridge (via DT?) so that it reports the mode matching the panel. I wonder if that should be handled completely in code, so that for example a bridge has a panel attached it can use the panel's .get_modes() and select a matching mode among the set that it supports.

Yes, pretty please :-) I don't think it would be a good idea to duplicate mode information in the bridge DT node, as that's not a property of the bridge. Querying the mode at runtime is in my opinion a much better option, and would also allow switching between different modes at runtime when that makes sense.

Now, I'm not sure whether it should be the bridge driver querying the panel driver directly, or the display controller driver doing it and then configuring the bridge accordingly. The latter seems more generic to me and doesn't rely on the assumption that the bridge output will always be directly connected to a panel.

-- Regards,

Laurent Pinchart

On Tue, Jul 29, 2014 at 01:36:46PM +0200, Thierry Reding wrote:

On Tue, Jul 29, 2014 at 01:21:48PM +0200, Andreas Färber wrote:

...

Hi Ajay,

Am 28.07.2014 08:13, schrieb Ajay kumar:

...

On 7/27/14, Andreas Färber afaerber@suse.de wrote:

...

Am 25.07.2014 21:22, schrieb Ajay Kumar:

...

This series is based on exynos-drm-next branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git

I have tested this after adding few DT changes for exynos5250-snow, exynos5420-peach-pit and exynos5800-peach-pi boards.

I'm trying to test this with a modified exynos5250-spring DT based off kgene's for-next branch due to DT, and I run into the following:

CC drivers/gpu/drm/bridge/ptn3460.o drivers/gpu/drm/bridge/ptn3460.c: In function ‘ptn3460_post_encoder_init’: drivers/gpu/drm/bridge/ptn3460.c:275:2: error: implicit declaration of function ‘drm_connector_register’ [-Werror=implicit-function-declaration] drm_connector_register(&ptn_bridge->connector); ^

Hope this might help: http://www.spinics.net/lists/dri-devel/msg60578.html

That fixed my build, thanks.

Unfortunately the most I got on Spring with attached DT was a blank screen with a white horizontal line in the middle.

Do I need to specify a specific panel model for Spring?

For testing I re-applied your iommu patches (which btw fail now for 5420 due to disp_pd) but didn't know what to do about your panel-lvds regulator patch now that it's gone.

If I don't apply this series, then commenting out the dp-controller node gets me a working display with simplefb as before.

Regards, Andreas

-- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

...

From 9172a26a8f0d0f0d170bd27e1c150ad204d8086a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20F=C3=A4rber?= afaerber@suse.de Date: Sun, 27 Jul 2014 21:58:06 +0200 Subject: [PATCH] ARM: dts: exynos5250: Add eDP/LVDS bridge to Spring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit

Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com [AF: Redone for v6] Signed-off-by: Andreas F??rber afaerber@suse.de

---

arch/arm/boot/dts/exynos5250-spring.dts | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos5250-spring.dts b/arch/arm/boot/dts/exynos5250-spring.dts index 687dfab86bc8..517b1ff2bfdf 100644 --- a/arch/arm/boot/dts/exynos5250-spring.dts +++ b/arch/arm/boot/dts/exynos5250-spring.dts @@ -64,10 +64,14 @@ vdd_pll-supply = <&s5m_ldo8_reg>; };

• panel: panel {

• compatible = "simple-panel";
  

• };

You can't do this. "simple-panel" isn't a valid panel model. It should probably be removed from the platform_of_match table in the driver.

Done.

Thierry