Re: [PATCH v2 1/2] drm/bridge: parade-ps8640: Enable runtime power management

Quoting Philip Chen (2021-10-21 14:05:59)

Fit ps8640 driver into runtime power management framework:

First, break _poweron() to 3 parts: (1) turn on power and wait for ps8640's internal MCU to finish init (2) check panel HPD (which is proxied by GPIO9) (3) the other configs. As runtime_resume() can be called before panel is powered, we only add (1) to _resume() and leave (2)(3) to _pre_enable(). We also add (2) to _aux_transfer() as we want to ensure panel HPD is asserted before we start AUX CH transactions.

The original driver has a mysterious delay of 50 ms between (2) and (3). Since Parade's support can't explain what the delay is for, and we don't see removing the delay break any boards at hand, remove the dalay

s/dalay/delay/

to fit into this driver change.

Besides, rename "powered" to "pre_enabled" and don't check for it in

"Besides" doesn't make sense here. Probably "In addition" or "Also"?

the pm_runtime calls. The pm_runtime calls are already refcounted so there's no reason to check there. The other user of "powered", _get_edid(), only cares if pre_enable() has already been called.

Lastly, change some existing DRM_...() logging to dev_...() along the way, since DRM_...() seem to be deprecated in [1].

[1] https://patchwork.freedesktop.org/patch/454760/

Signed-off-by: Philip Chen philipchen@chromium.org Reviewed-by: Douglas Anderson dianders@chromium.org

---

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 3aaa90913bf8..220ca3b03d24 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -148,6 +149,25 @@ static inline struct ps8640 *aux_to_ps8640(struct drm_dp_aux *aux) return container_of(aux, struct ps8640, aux); }

+static void ps8640_ensure_hpd(struct ps8640 *ps_bridge) +{

•   struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
  

•   struct device *dev = &ps_bridge->page[PAGE2_TOP_CNTL]->dev;
  

•   int status;
  

•   int ret;
  

•   /*
  

•    * Apparently something about the firmware in the chip signals that
  

•    * HPD goes high by reporting GPIO9 as high (even though HPD isn't
  

•    * actually connected to GPIO9).
  

•    */
  

•   ret = regmap_read_poll_timeout(map, PAGE2_GPIO_H, status,
  

•                           status & PS_GPIO9, 20 * 1000, 200 * 1000);
  

•   if (ret < 0)
  

•           dev_warn(dev, "HPD didn't go high: %d", ret);
  

Missing newline on the print message.

+}

static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) { @@ -171,6 +191,9 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, if (msg->address & ~SWAUX_ADDR_MASK) return -EINVAL;

•   pm_runtime_get_sync(dev);
  

•   ps8640_ensure_hpd(ps_bridge);
  

Shouldn't we bail out of here with an error if we can't ensure hpd?

•   switch (request) {
    case DP_AUX_NATIVE_WRITE:
    case DP_AUX_NATIVE_READ:
  

@@ -180,14 +203,15 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, case DP_AUX_I2C_READ: break; default:

•           return -EINVAL;
  

•           ret = -EINVAL;
  

•           goto exit;
    }
  
    ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
    if (ret) {
            DRM_DEV_ERROR(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n",
                          ret);
  

•           return ret;
  

•           goto exit;
    }
  
    /* Assume it's good */
  

@@ -213,7 +237,7 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, DRM_DEV_ERROR(dev, "failed to write WDATA: %d\n", ret);

•                           return ret;
  

•                           goto exit;
                    }
            }
    }
  

@@ -228,7 +252,7 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, if (ret) { DRM_DEV_ERROR(dev, "failed to read PAGE0_SWAUX_STATUS: %d\n", ret);

•           return ret;
  

•           goto exit;
    }
  
    switch (data & SWAUX_STATUS_MASK) {
  

@@ -250,9 +274,11 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, len = data & SWAUX_M_MASK; break; case SWAUX_STATUS_INVALID:

•           return -EOPNOTSUPP;
  

•           ret = -EOPNOTSUPP;
  

•           goto exit;
    case SWAUX_STATUS_TIMEOUT:
  

•           return -ETIMEDOUT;
  

•           ret = -ETIMEDOUT;
  

•           goto exit;
    }
  
    if (len && (request == DP_AUX_NATIVE_READ ||
  

It may be simpler to understand the diff if the transfer function still exited the same way and a small wrapper function was put around this to do the runtime PM operations.

pm_runtime_get_sync(); if (ps8640_hpd_asserted()) ret = ps8640_aux_transfer_msg(); pm_runtime_mark_last_busy(); pm_runtime_put_autosuspend();

return ret;

@@ -587,6 +611,13 @@ static int ps8640_probe(struct i2c_client *client) ps_bridge->aux.transfer = ps8640_aux_transfer; drm_dp_aux_init(&ps_bridge->aux);

•   pm_runtime_enable(dev);
  

•   pm_runtime_set_autosuspend_delay(dev, 500);
  

Presumably 500 is chosen because the message transfer speed is faster than that? Can we get a comment in the code for that?

•   pm_runtime_use_autosuspend(dev);
  

•   ret = devm_add_action_or_reset(dev, ps8640_runtime_disable, dev);
  

•   if (ret)
  

•           return ret;
  

•   drm_bridge_add(&ps_bridge->bridge);
  
    return 0;