Quoting Philip Chen (2021-10-26 12:11:09) > 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. > > Second, 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 delay to fit into this driver change. > > In addition, 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@xxxxxxxxxxxx> > Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx> > --- > > Changes in v3: > - Fix typo/wording in the commit message. > - Add ps8640_aux_transfer_msg() for AUX operation. In > ps8640_aux_transfer(), wrap around ps8640_aux_transfer_msg() > with PM operations and HPD check. > - Document why autosuspend_delay is set to 500ms. > > drivers/gpu/drm/bridge/parade-ps8640.c | 186 +++++++++++++++---------- > 1 file changed, 115 insertions(+), 71 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c > index 3aaa90913bf8..ac42a3473770 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,8 +149,29 @@ static inline struct ps8640 *aux_to_ps8640(struct drm_dp_aux *aux) > return container_of(aux, struct ps8640, aux); > } > > -static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux, > - struct drm_dp_aux_msg *msg) > +static void ps8640_ensure_hpd(struct ps8640 *ps_bridge) static int ps8640_ensure_hpd? > +{ > + 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\n", ret); > + > + return ret; > +} > + > +static ssize_t ps8640_aux_transfer_msg(struct drm_dp_aux *aux, > + struct drm_dp_aux_msg *msg) > { > struct ps8640 *ps_bridge = aux_to_ps8640(aux); > struct regmap *map = ps_bridge->regmap[PAGE0_DP_CNTL]; Otherwise Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
