Hi On Tue, Oct 26, 2021 at 12:36 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > 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? Oops, this change was mis-added to the patch on top of this one. I'll fix it in v4... > > > +{ > > + 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>
