* [PATCH v2 1/2] drm/bridge: parade-ps8640: Enable runtime power management
@ 2021-10-21 21:05 Philip Chen
2021-10-21 21:06 ` [PATCH v2 2/2] drm/bridge: parade-ps8640: Populate devices on aux-bus Philip Chen
2021-10-25 20:05 ` [PATCH v2 1/2] drm/bridge: parade-ps8640: Enable runtime power management Stephen Boyd
0 siblings, 2 replies; 6+ messages in thread
From: Philip Chen @ 2021-10-21 21:05 UTC (permalink / raw)
To: LKML
Cc: dianders, swboyd, Philip Chen, Andrzej Hajda, Daniel Vetter,
David Airlie, Jernej Skrabec, Jonas Karlman, Laurent Pinchart,
Neil Armstrong, Robert Foss, dri-devel
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
to fit into this driver change.
Besides, rename "powered" to "pre_enabled" and don't check for it in
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>
---
(no changes since v1)
drivers/gpu/drm/bridge/parade-ps8640.c | 184 +++++++++++++++----------
1 file changed, 108 insertions(+), 76 deletions(-)
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
@@ -9,6 +9,7 @@
#include <linux/i2c.h>
#include <linux/module.h>
#include <linux/of_graph.h>
+#include <linux/pm_runtime.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
@@ -100,7 +101,7 @@ struct ps8640 {
struct regulator_bulk_data supplies[2];
struct gpio_desc *gpio_reset;
struct gpio_desc *gpio_powerdown;
- bool powered;
+ bool pre_enabled;
};
static const struct regmap_config ps8640_regmap_config[] = {
@@ -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);
+}
+
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);
+
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 ||
@@ -264,48 +290,48 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux,
DRM_DEV_ERROR(dev,
"failed to read RDATA: %d\n",
ret);
- return ret;
+ goto exit;
}
buf[i] = data;
}
}
+exit:
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ if (ret)
+ return ret;
return len;
}
-static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
- const enum ps8640_vdo_control ctrl)
+static void ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
+ const enum ps8640_vdo_control ctrl)
{
struct regmap *map = ps_bridge->regmap[PAGE3_DSI_CNTL1];
+ struct device *dev = &ps_bridge->page[PAGE3_DSI_CNTL1]->dev;
u8 vdo_ctrl_buf[] = { VDO_CTL_ADD, ctrl };
int ret;
ret = regmap_bulk_write(map, PAGE3_SET_ADD,
vdo_ctrl_buf, sizeof(vdo_ctrl_buf));
- if (ret < 0) {
- DRM_ERROR("failed to %sable VDO: %d\n",
- ctrl == ENABLE ? "en" : "dis", ret);
- return ret;
- }
-
- return 0;
+ if (ret < 0)
+ dev_err(dev, "failed to %sable VDO: %d\n",
+ ctrl == ENABLE ? "en" : "dis", ret);
}
-static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
+static int __maybe_unused ps8640_resume(struct device *dev)
{
- struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
- int ret, status;
-
- if (ps_bridge->powered)
- return;
+ struct ps8640 *ps_bridge = dev_get_drvdata(dev);
+ int ret;
ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
ps_bridge->supplies);
if (ret < 0) {
- DRM_ERROR("cannot enable regulators %d\n", ret);
- return;
+ dev_err(dev, "cannot enable regulators %d\n", ret);
+ return ret;
}
gpiod_set_value(ps_bridge->gpio_powerdown, 0);
@@ -314,86 +340,78 @@ static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
gpiod_set_value(ps_bridge->gpio_reset, 0);
/*
- * Wait for the ps8640 embedded MCU to be ready
- * First wait 200ms and then check the MCU ready flag every 20ms
+ * Mystery 200 ms delay for the "MCU to be ready". It's unclear if
+ * this is truly necessary since the MCU will already signal that
+ * things are "good to go" by signaling HPD on "gpio 9". See
+ * ps8640_ensure_hpd(). For now we'll keep this mystery delay just in
+ * case.
*/
msleep(200);
- ret = regmap_read_poll_timeout(map, PAGE2_GPIO_H, status,
- status & PS_GPIO9, 20 * 1000, 200 * 1000);
-
- if (ret < 0) {
- DRM_ERROR("failed read PAGE2_GPIO_H: %d\n", ret);
- goto err_regulators_disable;
- }
-
- msleep(50);
-
- /*
- * The Manufacturer Command Set (MCS) is a device dependent interface
- * intended for factory programming of the display module default
- * parameters. Once the display module is configured, the MCS shall be
- * disabled by the manufacturer. Once disabled, all MCS commands are
- * ignored by the display interface.
- */
-
- ret = regmap_update_bits(map, PAGE2_MCS_EN, MCS_EN, 0);
- if (ret < 0) {
- DRM_ERROR("failed write PAGE2_MCS_EN: %d\n", ret);
- goto err_regulators_disable;
- }
-
- /* Switch access edp panel's edid through i2c */
- ret = regmap_write(map, PAGE2_I2C_BYPASS, I2C_BYPASS_EN);
- if (ret < 0) {
- DRM_ERROR("failed write PAGE2_I2C_BYPASS: %d\n", ret);
- goto err_regulators_disable;
- }
-
- ps_bridge->powered = true;
-
- return;
-
-err_regulators_disable:
- regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
- ps_bridge->supplies);
+ return 0;
}
-static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
+static int __maybe_unused ps8640_suspend(struct device *dev)
{
+ struct ps8640 *ps_bridge = dev_get_drvdata(dev);
int ret;
- if (!ps_bridge->powered)
- return;
-
gpiod_set_value(ps_bridge->gpio_reset, 1);
gpiod_set_value(ps_bridge->gpio_powerdown, 1);
ret = regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
ps_bridge->supplies);
if (ret < 0)
- DRM_ERROR("cannot disable regulators %d\n", ret);
+ dev_err(dev, "cannot disable regulators %d\n", ret);
- ps_bridge->powered = false;
+ return ret;
}
+static const struct dev_pm_ops ps8640_pm_ops = {
+ SET_RUNTIME_PM_OPS(ps8640_suspend, ps8640_resume, NULL)
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+};
+
static void ps8640_pre_enable(struct drm_bridge *bridge)
{
struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
+ struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
+ struct device *dev = &ps_bridge->page[PAGE0_DP_CNTL]->dev;
int ret;
- ps8640_bridge_poweron(ps_bridge);
+ pm_runtime_get_sync(dev);
+ ps8640_ensure_hpd(ps_bridge);
- ret = ps8640_bridge_vdo_control(ps_bridge, ENABLE);
+ /*
+ * The Manufacturer Command Set (MCS) is a device dependent interface
+ * intended for factory programming of the display module default
+ * parameters. Once the display module is configured, the MCS shall be
+ * disabled by the manufacturer. Once disabled, all MCS commands are
+ * ignored by the display interface.
+ */
+
+ ret = regmap_update_bits(map, PAGE2_MCS_EN, MCS_EN, 0);
+ if (ret < 0)
+ dev_warn(dev, "failed write PAGE2_MCS_EN: %d\n", ret);
+
+ /* Switch access edp panel's edid through i2c */
+ ret = regmap_write(map, PAGE2_I2C_BYPASS, I2C_BYPASS_EN);
if (ret < 0)
- ps8640_bridge_poweroff(ps_bridge);
+ dev_warn(dev, "failed write PAGE2_MCS_EN: %d\n", ret);
+
+ ps8640_bridge_vdo_control(ps_bridge, ENABLE);
+
+ ps_bridge->pre_enabled = true;
}
static void ps8640_post_disable(struct drm_bridge *bridge)
{
struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
+ ps_bridge->pre_enabled = false;
+
ps8640_bridge_vdo_control(ps_bridge, DISABLE);
- ps8640_bridge_poweroff(ps_bridge);
+ pm_runtime_put_sync(&ps_bridge->page[PAGE0_DP_CNTL]->dev);
}
static int ps8640_bridge_attach(struct drm_bridge *bridge,
@@ -474,7 +492,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
struct drm_connector *connector)
{
struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
- bool poweroff = !ps_bridge->powered;
+ bool poweroff = !ps_bridge->pre_enabled;
struct edid *edid;
/*
@@ -512,6 +530,12 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = {
.pre_enable = ps8640_pre_enable,
};
+static void ps8640_runtime_disable(void *data)
+{
+ pm_runtime_dont_use_autosuspend(data);
+ pm_runtime_disable(data);
+}
+
static int ps8640_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
@@ -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);
+ 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;
@@ -613,6 +644,7 @@ static struct i2c_driver ps8640_driver = {
.driver = {
.name = "ps8640",
.of_match_table = ps8640_match,
+ .pm = &ps8640_pm_ops,
},
};
module_i2c_driver(ps8640_driver);
--
2.33.0.1079.g6e70778dc9-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] drm/bridge: parade-ps8640: Populate devices on aux-bus
2021-10-21 21:05 [PATCH v2 1/2] drm/bridge: parade-ps8640: Enable runtime power management Philip Chen
@ 2021-10-21 21:06 ` Philip Chen
2021-10-25 20:10 ` Stephen Boyd
2021-10-25 20:05 ` [PATCH v2 1/2] drm/bridge: parade-ps8640: Enable runtime power management Stephen Boyd
1 sibling, 1 reply; 6+ messages in thread
From: Philip Chen @ 2021-10-21 21:06 UTC (permalink / raw)
To: LKML
Cc: dianders, swboyd, Philip Chen, Andrzej Hajda, Daniel Vetter,
David Airlie, Jernej Skrabec, Jonas Karlman, Laurent Pinchart,
Neil Armstrong, Robert Foss, dri-devel
Conventionally, panel is listed under the root of the device tree.
When userland asks for display mode, ps8640 bridge is responsible
for returning EDID when ps8640_bridge_get_edid() is called.
Now enable a new option of listing panel under "aux-bus" of ps8640
bridge node in the device tree. In this case, panel driver can retrieve
EDID by triggering AUX transactions, without ps8640_bridge_get_edid()
calls at all.
To prevent the "old" and "new" options from interfering with each
other's logic flow, disable DRM_BRIDGE_OP_EDID when the new option
is taken.
Signed-off-by: Philip Chen <philipchen@chromium.org>
---
Changes in v2:
- Add of_node_put() calls in ps8640_of_panel_on_aux_bus()
- Select DRM_DP_AUX_BUS for PS8640 driver in Kconfig
- Replace _put_sync() with _put_sync_suspend() in ps8640_post_disable()
drivers/gpu/drm/bridge/Kconfig | 1 +
drivers/gpu/drm/bridge/parade-ps8640.c | 53 +++++++++++++++++++-------
2 files changed, 41 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 431b6e12a81f..61db5a66b493 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -182,6 +182,7 @@ config DRM_PARADE_PS8622
config DRM_PARADE_PS8640
tristate "Parade PS8640 MIPI DSI to eDP Converter"
depends on OF
+ select DRM_DP_AUX_BUS
select DRM_KMS_HELPER
select DRM_MIPI_DSI
select DRM_PANEL
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 220ca3b03d24..f99a2e0808b7 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -14,6 +14,7 @@
#include <linux/regulator/consumer.h>
#include <drm/drm_bridge.h>
+#include <drm/drm_dp_aux_bus.h>
#include <drm/drm_dp_helper.h>
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_of.h>
@@ -149,6 +150,23 @@ static inline struct ps8640 *aux_to_ps8640(struct drm_dp_aux *aux)
return container_of(aux, struct ps8640, aux);
}
+static bool ps8640_of_panel_on_aux_bus(struct device *dev)
+{
+ struct device_node *bus, *panel;
+
+ bus = of_get_child_by_name(dev->of_node, "aux-bus");
+ if (!bus)
+ return false;
+ of_node_put(bus);
+
+ panel = of_get_child_by_name(bus, "panel");
+ if (!panel)
+ return false;
+ of_node_put(panel);
+
+ return true;
+}
+
static void ps8640_ensure_hpd(struct ps8640 *ps_bridge)
{
struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
@@ -411,7 +429,7 @@ static void ps8640_post_disable(struct drm_bridge *bridge)
ps_bridge->pre_enabled = false;
ps8640_bridge_vdo_control(ps_bridge, DISABLE);
- pm_runtime_put_sync(&ps_bridge->page[PAGE0_DP_CNTL]->dev);
+ pm_runtime_put_sync_suspend(&ps_bridge->page[PAGE0_DP_CNTL]->dev);
}
static int ps8640_bridge_attach(struct drm_bridge *bridge,
@@ -549,17 +567,6 @@ static int ps8640_probe(struct i2c_client *client)
if (!ps_bridge)
return -ENOMEM;
- /* port@1 is ps8640 output port */
- ret = drm_of_find_panel_or_bridge(np, 1, 0, &panel, NULL);
- if (ret < 0)
- return ret;
- if (!panel)
- return -ENODEV;
-
- ps_bridge->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
- if (IS_ERR(ps_bridge->panel_bridge))
- return PTR_ERR(ps_bridge->panel_bridge);
-
ps_bridge->supplies[0].supply = "vdd33";
ps_bridge->supplies[1].supply = "vdd12";
ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ps_bridge->supplies),
@@ -582,9 +589,16 @@ static int ps8640_probe(struct i2c_client *client)
ps_bridge->bridge.funcs = &ps8640_bridge_funcs;
ps_bridge->bridge.of_node = dev->of_node;
- ps_bridge->bridge.ops = DRM_BRIDGE_OP_EDID;
ps_bridge->bridge.type = DRM_MODE_CONNECTOR_eDP;
+ /*
+ * In the device tree, if panel is listed under aux-bus of the bridge
+ * node, panel driver should be able to retrieve EDID by itself using
+ * aux-bus. So let's not set DRM_BRIDGE_OP_EDID here.
+ */
+ if (!ps8640_of_panel_on_aux_bus(&client->dev))
+ ps_bridge->bridge.ops = DRM_BRIDGE_OP_EDID;
+
ps_bridge->page[PAGE0_DP_CNTL] = client;
ps_bridge->regmap[PAGE0_DP_CNTL] = devm_regmap_init_i2c(client, ps8640_regmap_config);
@@ -618,6 +632,19 @@ static int ps8640_probe(struct i2c_client *client)
if (ret)
return ret;
+ devm_of_dp_aux_populate_ep_devices(&ps_bridge->aux);
+
+ /* port@1 is ps8640 output port */
+ ret = drm_of_find_panel_or_bridge(np, 1, 0, &panel, NULL);
+ if (ret < 0)
+ return ret;
+ if (!panel)
+ return -ENODEV;
+
+ ps_bridge->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+ if (IS_ERR(ps_bridge->panel_bridge))
+ return PTR_ERR(ps_bridge->panel_bridge);
+
drm_bridge_add(&ps_bridge->bridge);
return 0;
--
2.33.0.1079.g6e70778dc9-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] drm/bridge: parade-ps8640: Enable runtime power management
2021-10-21 21:05 [PATCH v2 1/2] drm/bridge: parade-ps8640: Enable runtime power management Philip Chen
2021-10-21 21:06 ` [PATCH v2 2/2] drm/bridge: parade-ps8640: Populate devices on aux-bus Philip Chen
@ 2021-10-25 20:05 ` Stephen Boyd
2021-10-26 19:14 ` Philip Chen
1 sibling, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2021-10-25 20:05 UTC (permalink / raw)
To: LKML, Philip Chen
Cc: dianders, Andrzej Hajda, Daniel Vetter, David Airlie,
Jernej Skrabec, Jonas Karlman, Laurent Pinchart, Neil Armstrong,
Robert Foss, dri-devel
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] drm/bridge: parade-ps8640: Populate devices on aux-bus
2021-10-21 21:06 ` [PATCH v2 2/2] drm/bridge: parade-ps8640: Populate devices on aux-bus Philip Chen
@ 2021-10-25 20:10 ` Stephen Boyd
2021-10-26 19:02 ` Philip Chen
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2021-10-25 20:10 UTC (permalink / raw)
To: LKML, Philip Chen
Cc: dianders, Andrzej Hajda, Daniel Vetter, David Airlie,
Jernej Skrabec, Jonas Karlman, Laurent Pinchart, Neil Armstrong,
Robert Foss, dri-devel
Quoting Philip Chen (2021-10-21 14:06:00)
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 220ca3b03d24..f99a2e0808b7 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -149,6 +150,23 @@ static inline struct ps8640 *aux_to_ps8640(struct drm_dp_aux *aux)
> return container_of(aux, struct ps8640, aux);
> }
>
> +static bool ps8640_of_panel_on_aux_bus(struct device *dev)
> +{
> + struct device_node *bus, *panel;
> +
> + bus = of_get_child_by_name(dev->of_node, "aux-bus");
> + if (!bus)
> + return false;
> + of_node_put(bus);
This should come after the next line...
> +
> + panel = of_get_child_by_name(bus, "panel");
here, so that 'bus' can't go away before getting children nodes. It
doesn't actually matter in this case because 'device' holds the aux-bus,
but we shouldn't add anti-patterns to the code lest someone copies it
where it actually matters.
> + if (!panel)
> + return false;
> + of_node_put(panel);
> +
> + return true;
> +}
> +
> static void ps8640_ensure_hpd(struct ps8640 *ps_bridge)
> {
> struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
Otherwise
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] drm/bridge: parade-ps8640: Populate devices on aux-bus
2021-10-25 20:10 ` Stephen Boyd
@ 2021-10-26 19:02 ` Philip Chen
0 siblings, 0 replies; 6+ messages in thread
From: Philip Chen @ 2021-10-26 19:02 UTC (permalink / raw)
To: Stephen Boyd
Cc: LKML, dianders, Andrzej Hajda, Daniel Vetter, David Airlie,
Jernej Skrabec, Jonas Karlman, Laurent Pinchart, Neil Armstrong,
Robert Foss, dri-devel
Hi,
On Mon, Oct 25, 2021 at 1:10 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Philip Chen (2021-10-21 14:06:00)
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index 220ca3b03d24..f99a2e0808b7 100644
> > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > @@ -149,6 +150,23 @@ static inline struct ps8640 *aux_to_ps8640(struct drm_dp_aux *aux)
> > return container_of(aux, struct ps8640, aux);
> > }
> >
> > +static bool ps8640_of_panel_on_aux_bus(struct device *dev)
> > +{
> > + struct device_node *bus, *panel;
> > +
> > + bus = of_get_child_by_name(dev->of_node, "aux-bus");
> > + if (!bus)
> > + return false;
> > + of_node_put(bus);
>
> This should come after the next line...
>
> > +
> > + panel = of_get_child_by_name(bus, "panel");
>
> here, so that 'bus' can't go away before getting children nodes. It
> doesn't actually matter in this case because 'device' holds the aux-bus,
> but we shouldn't add anti-patterns to the code lest someone copies it
> where it actually matters.
Thanks for pointing it out.
I will fix it in v3.
>
> > + if (!panel)
> > + return false;
> > + of_node_put(panel);
> > +
> > + return true;
> > +}
> > +
> > static void ps8640_ensure_hpd(struct ps8640 *ps_bridge)
> > {
> > struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
>
> Otherwise
>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] drm/bridge: parade-ps8640: Enable runtime power management
2021-10-25 20:05 ` [PATCH v2 1/2] drm/bridge: parade-ps8640: Enable runtime power management Stephen Boyd
@ 2021-10-26 19:14 ` Philip Chen
0 siblings, 0 replies; 6+ messages in thread
From: Philip Chen @ 2021-10-26 19:14 UTC (permalink / raw)
To: Stephen Boyd
Cc: LKML, dianders, Andrzej Hajda, Daniel Vetter, David Airlie,
Jernej Skrabec, Jonas Karlman, Laurent Pinchart, Neil Armstrong,
Robert Foss, dri-devel
Hi
On Mon, Oct 25, 2021 at 1:05 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> 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/
Thanks.
I've fixed it in v3.
>
> > 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"?
Thanks.
I've fixed it in v3.
>
> > 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.
Thanks.
I've fixed it in v3.
>
> > +}
> > +
> > 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?
Sounds about right.
I fixed this in v3.
PTAL.
>
> > +
> > 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.
Thanks for the suggestion.
I've posted v3 following this route.
PTAL.
>
>
> 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?
Added a comment in v3.
PTAL.
>
> > + 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;
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-26 19:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-21 21:05 [PATCH v2 1/2] drm/bridge: parade-ps8640: Enable runtime power management Philip Chen
2021-10-21 21:06 ` [PATCH v2 2/2] drm/bridge: parade-ps8640: Populate devices on aux-bus Philip Chen
2021-10-25 20:10 ` Stephen Boyd
2021-10-26 19:02 ` Philip Chen
2021-10-25 20:05 ` [PATCH v2 1/2] drm/bridge: parade-ps8640: Enable runtime power management Stephen Boyd
2021-10-26 19:14 ` Philip Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).