Hi,
On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
<quic_sbillaka@xxxxxxxxxxx> wrote:
>
> The panel-edp enables the eDP panel power during probe, get_modes
> and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
> controller are directly dependent on panel power. As eDP display can be
> assumed as always connected, the controller driver can skip the eDP
> connect and disconnect interrupts. Any disruption in the link status
> will be indicated via the IRQ_HPD interrupts.
>
> So, the eDP controller driver can just enable the IRQ_HPD and replug
> interrupts. The DP controller driver still needs to enable all the
> interrupts.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@xxxxxxxxxxx>
> ---
> Changes in v8:
> - add comment explaining the interrupt status return
>
> Changes in v7:
> - reordered the patch in the series
> - modified the return statement for isr
> - connector check modified to just check for eDP
>
> drivers/gpu/drm/msm/dp/dp_catalog.c | 18 ++++++++++++------
> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
> 2 files changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index fac815f..3a298e9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
>
> u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>
> - /* enable HPD plug and unplug interrupts */
> - dp_catalog_hpd_config_intr(dp_catalog,
> - DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
> -
> /* Configure REFTIMER and enable it */
> reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> @@ -599,13 +595,23 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
> {
> struct dp_catalog_private *catalog = container_of(dp_catalog,
> struct dp_catalog_private, dp_catalog);
> - int isr = 0;
> + int isr, mask;
>
> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
> (isr & DP_DP_HPD_INT_MASK));
> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
>
> - return isr;
> + /*
> + * REG_DP_DP_HPD_INT_STATUS reports the status of all interrupts
> + * irrespective of their masked status. The HW interrupt will not be
> + * generated if an interrupt is masked. However, the interrupt status
> + * bit in the register will still be set. The eDP controller driver
> + * masks the plug and unplug interrupts unlike DP controller which
> + * unmasks all the interrupts. So, do not report the status of the
> + * masked interrupts.
> + */
> + return isr & (mask | ~DP_DP_HPD_INT_MASK);
What's still missing in your comments is why you aren't just doing
"return isr & mask;". In other words, why is the API for HPD bits
different from the API for non-HPD bits? What code out there wants to
know about non-HPD interrupts even if they are masked?
Actually, thinking about this more, my preference would be this:
a) Rename the existing function to dp_catalog_hpd_get_intr_status_raw()
b) Create a new function called dp_catalog_hpd_get_intr_status() whose
implementation is:
return dp_catalog_hpd_get_intr_status_raw() & mask;
Then any callers who care about the raw status can be changed to call
dp_catalog_hpd_get_intr_status_raw(). If no callers need
dp_catalog_hpd_get_intr_status_raw() then you don't actually need to
create this function.
If you make that change then all of a sudden the API isn't weird/wonky
and you can just get rid of the comment I asked you to add.
-Doug
