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
proxy by GPIO9) (3) the other configs. Runtime _resume() can be called
before panel is powered, so we only add (1) to _resume() and do (2)(3)
in _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 it 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 <[email protected]>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 177 ++++++++++++++-----------
1 file changed, 103 insertions(+), 74 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 3aaa90913bf8..acfe1bf0f936 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);
@@ -319,81 +345,70 @@ static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
*/
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 +489,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 +527,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 +608,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 +641,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
Conventionally, panel is listed under the root in 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 the 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 <[email protected]>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 52 ++++++++++++++++++++------
1 file changed, 40 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index acfe1bf0f936..98884f799ea8 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,24 @@ 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;
+
+ if (!dev->of_node)
+ return false;
+
+ bus = of_get_child_by_name(dev->of_node, "aux-bus");
+ if (!bus)
+ return false;
+
+ panel = of_get_child_by_name(bus, "panel");
+ if (!panel)
+ return false;
+
+ return true;
+}
+
static void ps8640_ensure_hpd(struct ps8640 *ps_bridge)
{
struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
@@ -546,17 +565,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),
@@ -579,9 +587,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);
@@ -615,6 +630,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
Hi Philip,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-tip/drm-tip next-20211015]
[cannot apply to linux/master linus/master v5.15-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Philip-Chen/drm-bridge-parade-ps8640-Enable-runtime-power-management/20211017-005837
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: hexagon-buildonly-randconfig-r001-20211017 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 8ca4b3ef19fe82d7ad6a6e1515317dcc01b41515)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/3224ea81dd1cf1d744bee07def5edb1e98c269a6
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Philip-Chen/drm-bridge-parade-ps8640-Enable-runtime-power-management/20211017-005837
git checkout 3224ea81dd1cf1d744bee07def5edb1e98c269a6
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=hexagon
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>, old ones prefixed by <<):
ERROR: modpost: "__raw_readsl" [drivers/i3c/master/svc-i3c-master.ko] undefined!
ERROR: modpost: "__raw_writesl" [drivers/i3c/master/dw-i3c-master.ko] undefined!
ERROR: modpost: "__raw_readsl" [drivers/i3c/master/dw-i3c-master.ko] undefined!
ERROR: modpost: "__raw_writesl" [drivers/i3c/master/i3c-master-cdns.ko] undefined!
ERROR: modpost: "__raw_readsl" [drivers/i3c/master/i3c-master-cdns.ko] undefined!
>> ERROR: modpost: "devm_of_dp_aux_populate_ep_devices" [drivers/gpu/drm/bridge/parade-ps8640.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Philip,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-tip/drm-tip next-20211015]
[cannot apply to linux/master linus/master v5.15-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Philip-Chen/drm-bridge-parade-ps8640-Enable-runtime-power-management/20211017-005837
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: hexagon-randconfig-r041-20211018 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d245f2e8597bfb52c34810a328d42b990e4af1a4)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/3224ea81dd1cf1d744bee07def5edb1e98c269a6
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Philip-Chen/drm-bridge-parade-ps8640-Enable-runtime-power-management/20211017-005837
git checkout 3224ea81dd1cf1d744bee07def5edb1e98c269a6
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> ld.lld: error: undefined symbol: devm_of_dp_aux_populate_ep_devices
>>> referenced by parade-ps8640.c
>>> gpu/drm/bridge/parade-ps8640.o:(ps8640_probe) in archive drivers/built-in.a
>>> referenced by parade-ps8640.c
>>> gpu/drm/bridge/parade-ps8640.o:(ps8640_probe) in archive drivers/built-in.a
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi,
On Sat, Oct 16, 2021 at 9:57 AM Philip Chen <[email protected]> wrote:
>
> Conventionally, panel is listed under the root in 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 the 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 <[email protected]>
> ---
>
> drivers/gpu/drm/bridge/parade-ps8640.c | 52 ++++++++++++++++++++------
> 1 file changed, 40 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index acfe1bf0f936..98884f799ea8 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>
You need a `select DRM_DP_AUX_BUS` in the Kconfig to avoid the errors
that the build robot found for you.
> #include <drm/drm_dp_helper.h>
> #include <drm/drm_mipi_dsi.h>
> #include <drm/drm_of.h>
> @@ -149,6 +150,24 @@ 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;
> +
> + if (!dev->of_node)
> + return false;
You probably don't need the above check. I think things would be
pretty broken if we didn't have an "of_node".
> + bus = of_get_child_by_name(dev->of_node, "aux-bus");
> + if (!bus)
> + return false;
> +
> + panel = of_get_child_by_name(bus, "panel");
of_node_put(bus);
> + if (!panel)
> + return false;
of_node_put(panel);
Other than the above, this looks reasonable to me.
-Doug
Hi,
On Sat, Oct 16, 2021 at 9:57 AM Philip Chen <[email protected]> wrote:
>
> @@ -319,81 +345,70 @@ static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
> */
> 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;
> - }
Above the "msleep(200)" I see a comment that says "and then check the
MCU ready flag every 20ms". That probably refers to the code that
you're moving here. Maybe change the comment above the "msleep(200);"
to something like this if you like it:
/*
* 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.
*/
Other than that this looks good to me, which isn't really a surprise
since I was involved in helping with / reviewing early versions of
this change. In any case, I'm happy with:
Reviewed-by: Douglas Anderson <[email protected]>
Hi Doug,
On Mon, Oct 18, 2021 at 1:43 PM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Sat, Oct 16, 2021 at 9:57 AM Philip Chen <[email protected]> wrote:
> >
> > @@ -319,81 +345,70 @@ static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
> > */
> > 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;
> > - }
>
> Above the "msleep(200)" I see a comment that says "and then check the
> MCU ready flag every 20ms". That probably refers to the code that
> you're moving here. Maybe change the comment above the "msleep(200);"
> to something like this if you like it:
>
> /*
> * 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.
> */
>
Thanks for the review.
Added the comment in v2.
PTAL.
> Other than that this looks good to me, which isn't really a surprise
> since I was involved in helping with / reviewing early versions of
> this change. In any case, I'm happy with:
>
> Reviewed-by: Douglas Anderson <[email protected]>
Hi Doug,
On Mon, Oct 18, 2021 at 1:43 PM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Sat, Oct 16, 2021 at 9:57 AM Philip Chen <[email protected]> wrote:
> >
> > Conventionally, panel is listed under the root in 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 the 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 <[email protected]>
> > ---
> >
> > drivers/gpu/drm/bridge/parade-ps8640.c | 52 ++++++++++++++++++++------
> > 1 file changed, 40 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > index acfe1bf0f936..98884f799ea8 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>
>
> You need a `select DRM_DP_AUX_BUS` in the Kconfig to avoid the errors
> that the build robot found for you.
Thanks for the tip!
I also found "select REGMAP_I2C" seems to be missing for ps8640.
although the build robot didn't complain.
Should I post a fix-up?
>
>
> > #include <drm/drm_dp_helper.h>
> > #include <drm/drm_mipi_dsi.h>
> > #include <drm/drm_of.h>
> > @@ -149,6 +150,24 @@ 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;
> > +
> > + if (!dev->of_node)
> > + return false;
>
> You probably don't need the above check. I think things would be
> pretty broken if we didn't have an "of_node".
Removed in v2.
PTAL.
>
>
> > + bus = of_get_child_by_name(dev->of_node, "aux-bus");
> > + if (!bus)
> > + return false;
> > +
> > + panel = of_get_child_by_name(bus, "panel");
>
> of_node_put(bus);
Added in v2.
PTAL.
>
>
> > + if (!panel)
> > + return false;
>
> of_node_put(panel);
Added in v2.
PTAL.
>
>
> Other than the above, this looks reasonable to me.
>
> -Doug