From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Thu, 02 Mar 2017 18:18:24 +0200 Subject: [PATCH v2 2/2] drm: bridge: Move HPD handling to PHY operations In-Reply-To: <1488468572-31971-3-git-send-email-narmstrong@baylibre.com> References: <1488468572-31971-1-git-send-email-narmstrong@baylibre.com> <1488468572-31971-3-git-send-email-narmstrong@baylibre.com> Message-ID: <6652377.Pu8amSWD8H@avalon> To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org Hi Neil, Thank you for the patch. On Thursday 02 Mar 2017 16:29:32 Neil Armstrong wrote: > The HDMI TX controller support HPD and RXSENSE signaling from the PHY > via it's STAT0 PHY interface, but some vendor PHYs can manage these > signals independently from the controller, thus these STAT0 handling > should be moved to PHY specific operations and become optional. > > The existing STAT0 HPD and RXSENSE handling code is refactored into > a supplementaty set of default PHY operations that are used automatically > when the platform glue doesn't provide its own operations. > > Signed-off-by: Neil Armstrong > --- > drivers/gpu/drm/bridge/dw-hdmi.c | 134 +++++++++++++++++++++++++----------- > include/drm/bridge/dw_hdmi.h | 8 +++ > 2 files changed, 104 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c > b/drivers/gpu/drm/bridge/dw-hdmi.c index 653ecd7..58dcf7d 100644 > --- a/drivers/gpu/drm/bridge/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c > @@ -1098,10 +1098,50 @@ static enum drm_connector_status > dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, connector_status_connected : > connector_status_disconnected; > } > > +static void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > + bool force, bool disabled, bool rxsense) > +{ > + if (force || disabled || !rxsense) > + hdmi->phy_mask |= HDMI_PHY_RX_SENSE; > + else > + hdmi->phy_mask &= ~HDMI_PHY_RX_SENSE; > +} > + > +static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data) > +{ > + /* setup HPD and RXSENSE interrupt polarity */ > + hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); > +} > + > +static void dw_hdmi_phy_configure_hpd(struct dw_hdmi *hdmi, void *data) > +{ > + /* enable cable hot plug irq */ > + hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); > +} > + > +static void dw_hdmi_phy_clear_hpd(struct dw_hdmi *hdmi, void *data) > +{ > + /* Clear Hotplug interrupts */ > + hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, > + HDMI_IH_PHY_STAT0); > +} > + > +static void dw_hdmi_phy_unmute_hpd(struct dw_hdmi *hdmi, void *data) > +{ > + /* Unmute Hotplug interrupts */ > + hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), > + HDMI_IH_MUTE_PHY_STAT0); > +} Do we really need all those new operations ? It looks to me like a bit of refactoring could help lowering the number of operations. We essentially need - an init function called at probe time (or likely rather at runtime PM resume time when we'll implement runtime PM) - an interrupt enable/disable function roughly equivalent to dw_hdmi_update_phy_mask() - a read function to report the detection results called from dw_hdmi_connector_detect() > static const struct dw_hdmi_phy_ops dw_hdmi_synopsys_phy_ops = { > .init = dw_hdmi_phy_init, > .disable = dw_hdmi_phy_disable, > .read_hpd = dw_hdmi_phy_read_hpd, > + .update_hpd = dw_hdmi_phy_update_hpd, > + .setup_hpd = dw_hdmi_phy_setup_hpd, > + .configure_hpd = dw_hdmi_phy_configure_hpd, > + .clear_hpd = dw_hdmi_phy_clear_hpd, > + .unmute_hpd = dw_hdmi_phy_unmute_hpd, > }; > > /* ------------------------------------------------------------------------ > @@ -1507,11 +1547,12 @@ static int dw_hdmi_fb_registered(struct dw_hdmi > *hdmi) HDMI_PHY_I2CM_CTLINT_ADDR); > > /* enable cable hot plug irq */ > - hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); > + if (hdmi->phy.ops->configure_hpd) > + hdmi->phy.ops->configure_hpd(hdmi, hdmi->phy.data); > > /* Clear Hotplug interrupts */ > - hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, > - HDMI_IH_PHY_STAT0); > + if (hdmi->phy.ops->clear_hpd) > + hdmi->phy.ops->clear_hpd(hdmi, hdmi->phy.data); The probe function contains the same code. Let's inline this function into probe, group all HPD-related initialization together and extract that into a function to keep probe easy to read. I think you can do that in a separate patch. > return 0; > } > @@ -1622,13 +1663,14 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi > *hdmi) { > u8 old_mask = hdmi->phy_mask; > > - if (hdmi->force || hdmi->disabled || !hdmi->rxsense) > - hdmi->phy_mask |= HDMI_PHY_RX_SENSE; > - else > - hdmi->phy_mask &= ~HDMI_PHY_RX_SENSE; > + if (hdmi->phy.ops->update_hpd) > + hdmi->phy.ops->update_hpd(hdmi, hdmi->phy.data, > + hdmi->force, hdmi->disabled, > + hdmi->rxsense); > > - if (old_mask != hdmi->phy_mask) > - hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); > + if (old_mask != hdmi->phy_mask && phy_mask isn't accessible to glue code, so this test will never be true with vendor PHYs. > + hdmi->phy.ops->configure_hpd) > + hdmi->phy.ops->configure_hpd(hdmi, hdmi->phy.data); > } > > static enum drm_connector_status > @@ -1820,6 +1862,41 @@ static irqreturn_t dw_hdmi_hardirq(int irq, void > *dev_id) return ret; > } > > +void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool > rx_sense) > +{ > + mutex_lock(&hdmi->mutex); > + > + if (!hdmi->disabled && !hdmi->force) { > + /* > + * If the RX sense status indicates we're disconnected, > + * clear the software rxsense status. > + */ > + if (!rx_sense) > + hdmi->rxsense = false; > + > + /* > + * Only set the software rxsense status when both > + * rxsense and hpd indicates we're connected. > + * This avoids what seems to be bad behaviour in > + * at least iMX6S versions of the phy. > + */ > + if (hpd) > + hdmi->rxsense = true; This contradicts the above comment, hdmi->rxsense is set back to true solely based on the hpd parameter. > + > + dw_hdmi_update_power(hdmi); > + dw_hdmi_update_phy_mask(hdmi); > + } I'd add a blank line here. > + mutex_unlock(&hdmi->mutex); > +} > + > +void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense) > +{ > + struct dw_hdmi *hdmi = dev_get_drvdata(dev); > + > + __dw_hdmi_setup_rx_sense(hdmi, hpd, rx_sense); > +} > +EXPORT_SYMBOL_GPL(dw_hdmi_setup_rx_sense); > + > static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) > { > struct dw_hdmi *hdmi = dev_id; > @@ -1852,30 +1929,10 @@ static irqreturn_t dw_hdmi_irq(int irq, void > *dev_id) * ask the source to re-read the EDID. > */ > if (intr_stat & > - (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) { > - mutex_lock(&hdmi->mutex); > - if (!hdmi->disabled && !hdmi->force) { > - /* > - * If the RX sense status indicates we're disconnected, > - * clear the software rxsense status. > - */ > - if (!(phy_stat & HDMI_PHY_RX_SENSE)) > - hdmi->rxsense = false; > - > - /* > - * Only set the software rxsense status when both > - * rxsense and hpd indicates we're connected. > - * This avoids what seems to be bad behaviour in > - * at least iMX6S versions of the phy. > - */ > - if (phy_stat & HDMI_PHY_HPD) > - hdmi->rxsense = true; > - > - dw_hdmi_update_power(hdmi); > - dw_hdmi_update_phy_mask(hdmi); > - } > - mutex_unlock(&hdmi->mutex); > - } > + (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) Is this right ? If your SoC implements a custom HPD mechanism, does it still rely on the standard RX SENSE and HPD interrupts ? In particular, this function still writes the HDMI_IH_MUTE_PHY_STAT0 register directly, while you've extracted a write to the same register in the probe function into a PHY operation. > + __dw_hdmi_setup_rx_sense(hdmi, > + phy_stat & HDMI_PHY_HPD, > + phy_stat & HDMI_PHY_RX_SENSE); > > if (intr_stat & HDMI_IH_PHY_STAT0_HPD) { > dev_dbg(hdmi->dev, "EVENT=%s\n", > @@ -2146,11 +2203,12 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > * Configure registers related to HDMI interrupt > * generation before registering IRQ. > */ > - hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); > + if (hdmi->phy.ops->setup_hpd) > + hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); > > /* Clear Hotplug interrupts */ > - hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, > - HDMI_IH_PHY_STAT0); > + if (hdmi->phy.ops->clear_hpd) > + hdmi->phy.ops->clear_hpd(hdmi, hdmi->phy.data); > > hdmi->bridge.driver_private = hdmi; > hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; > @@ -2163,8 +2221,8 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > goto err_iahb; > > /* Unmute interrupts */ > - hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), > - HDMI_IH_MUTE_PHY_STAT0); > + if (hdmi->phy.ops->unmute_hpd) > + hdmi->phy.ops->unmute_hpd(hdmi, hdmi->phy.data); > > memset(&pdevinfo, 0, sizeof(pdevinfo)); > pdevinfo.parent = dev; > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > index 8c0cc13..d72403f 100644 > --- a/include/drm/bridge/dw_hdmi.h > +++ b/include/drm/bridge/dw_hdmi.h > @@ -64,6 +64,12 @@ struct dw_hdmi_phy_ops { > struct drm_display_mode *mode); > void (*disable)(struct dw_hdmi *hdmi, void *data); > enum drm_connector_status (*read_hpd)(struct dw_hdmi *hdmi, void *data); > + void (*update_hpd)(struct dw_hdmi *hdmi, void *data, > + bool force, bool disabled, bool rxsense); > + void (*setup_hpd)(struct dw_hdmi *hdmi, void *data); > + void (*configure_hpd)(struct dw_hdmi *hdmi, void *data); > + void (*clear_hpd)(struct dw_hdmi *hdmi, void *data); > + void (*unmute_hpd)(struct dw_hdmi *hdmi, void *data); That's quite a lot of new operations. I think we need documentation :-) > }; > > struct dw_hdmi_plat_data { > @@ -93,6 +99,8 @@ int dw_hdmi_probe(struct platform_device *pdev, > int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder, > const struct dw_hdmi_plat_data *plat_data); > > +void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense); > + > void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate); > void dw_hdmi_audio_enable(struct dw_hdmi *hdmi); > void dw_hdmi_audio_disable(struct dw_hdmi *hdmi); -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v2 2/2] drm: bridge: Move HPD handling to PHY operations Date: Thu, 02 Mar 2017 18:18:24 +0200 Message-ID: <6652377.Pu8amSWD8H@avalon> References: <1488468572-31971-1-git-send-email-narmstrong@baylibre.com> <1488468572-31971-3-git-send-email-narmstrong@baylibre.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from galahad.ideasonboard.com (galahad.ideasonboard.com [185.26.127.97]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2DF966EB95 for ; Thu, 2 Mar 2017 16:17:51 +0000 (UTC) In-Reply-To: <1488468572-31971-3-git-send-email-narmstrong@baylibre.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Neil Armstrong Cc: Jose.Abreu@synopsys.com, laurent.pinchart+renesas@ideasonboard.com, kieran.bingham@ideasonboard.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-amlogic@lists.infradead.org List-Id: dri-devel@lists.freedesktop.org SGkgTmVpbCwKClRoYW5rIHlvdSBmb3IgdGhlIHBhdGNoLgoKT24gVGh1cnNkYXkgMDIgTWFyIDIw MTcgMTY6Mjk6MzIgTmVpbCBBcm1zdHJvbmcgd3JvdGU6Cj4gVGhlIEhETUkgVFggY29udHJvbGxl ciBzdXBwb3J0IEhQRCBhbmQgUlhTRU5TRSBzaWduYWxpbmcgZnJvbSB0aGUgUEhZCj4gdmlhIGl0 J3MgU1RBVDAgUEhZIGludGVyZmFjZSwgYnV0IHNvbWUgdmVuZG9yIFBIWXMgY2FuIG1hbmFnZSB0 aGVzZQo+IHNpZ25hbHMgaW5kZXBlbmRlbnRseSBmcm9tIHRoZSBjb250cm9sbGVyLCB0aHVzIHRo ZXNlIFNUQVQwIGhhbmRsaW5nCj4gc2hvdWxkIGJlIG1vdmVkIHRvIFBIWSBzcGVjaWZpYyBvcGVy YXRpb25zIGFuZCBiZWNvbWUgb3B0aW9uYWwuCj4gCj4gVGhlIGV4aXN0aW5nIFNUQVQwIEhQRCBh bmQgUlhTRU5TRSBoYW5kbGluZyBjb2RlIGlzIHJlZmFjdG9yZWQgaW50bwo+IGEgc3VwcGxlbWVu dGF0eSBzZXQgb2YgZGVmYXVsdCBQSFkgb3BlcmF0aW9ucyB0aGF0IGFyZSB1c2VkIGF1dG9tYXRp Y2FsbHkKPiB3aGVuIHRoZSBwbGF0Zm9ybSBnbHVlIGRvZXNuJ3QgcHJvdmlkZSBpdHMgb3duIG9w ZXJhdGlvbnMuCj4gCj4gU2lnbmVkLW9mZi1ieTogTmVpbCBBcm1zdHJvbmcgPG5hcm1zdHJvbmdA YmF5bGlicmUuY29tPgo+IC0tLQo+ICBkcml2ZXJzL2dwdS9kcm0vYnJpZGdlL2R3LWhkbWkuYyB8 IDEzNCArKysrKysrKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tLS0KPiAgaW5jbHVkZS9kcm0v YnJpZGdlL2R3X2hkbWkuaCAgICAgfCAgIDggKysrCj4gIDIgZmlsZXMgY2hhbmdlZCwgMTA0IGlu c2VydGlvbnMoKyksIDM4IGRlbGV0aW9ucygtKQo+IAo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dw dS9kcm0vYnJpZGdlL2R3LWhkbWkuYwo+IGIvZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9kdy1oZG1p LmMgaW5kZXggNjUzZWNkNy4uNThkY2Y3ZCAxMDA2NDQKPiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0v YnJpZGdlL2R3LWhkbWkuYwo+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9icmlkZ2UvZHctaGRtaS5j Cj4gQEAgLTEwOTgsMTAgKzEwOTgsNTAgQEAgc3RhdGljIGVudW0gZHJtX2Nvbm5lY3Rvcl9zdGF0 dXMKPiBkd19oZG1pX3BoeV9yZWFkX2hwZChzdHJ1Y3QgZHdfaGRtaSAqaGRtaSwgY29ubmVjdG9y X3N0YXR1c19jb25uZWN0ZWQgOgo+IGNvbm5lY3Rvcl9zdGF0dXNfZGlzY29ubmVjdGVkOwo+ICB9 Cj4gCj4gK3N0YXRpYyB2b2lkIGR3X2hkbWlfcGh5X3VwZGF0ZV9ocGQoc3RydWN0IGR3X2hkbWkg KmhkbWksIHZvaWQgKmRhdGEsCj4gKwkJCQkgICBib29sIGZvcmNlLCBib29sIGRpc2FibGVkLCBi b29sIHJ4c2Vuc2UpCj4gK3sKPiArCWlmIChmb3JjZSB8fCBkaXNhYmxlZCB8fCAhcnhzZW5zZSkK PiArCQloZG1pLT5waHlfbWFzayB8PSBIRE1JX1BIWV9SWF9TRU5TRTsKPiArCWVsc2UKPiArCQlo ZG1pLT5waHlfbWFzayAmPSB+SERNSV9QSFlfUlhfU0VOU0U7Cj4gK30KPiArCj4gK3N0YXRpYyB2 b2lkIGR3X2hkbWlfcGh5X3NldHVwX2hwZChzdHJ1Y3QgZHdfaGRtaSAqaGRtaSwgdm9pZCAqZGF0 YSkKPiArewo+ICsJLyogc2V0dXAgSFBEIGFuZCBSWFNFTlNFIGludGVycnVwdCBwb2xhcml0eSAq Lwo+ICsJaGRtaV93cml0ZWIoaGRtaSwgSERNSV9QSFlfSFBEIHwgSERNSV9QSFlfUlhfU0VOU0Us IEhETUlfUEhZX1BPTDApOwo+ICt9Cj4gKwo+ICtzdGF0aWMgdm9pZCBkd19oZG1pX3BoeV9jb25m aWd1cmVfaHBkKHN0cnVjdCBkd19oZG1pICpoZG1pLCB2b2lkICpkYXRhKQo+ICt7Cj4gKwkvKiBl bmFibGUgY2FibGUgaG90IHBsdWcgaXJxICovCj4gKwloZG1pX3dyaXRlYihoZG1pLCBoZG1pLT5w aHlfbWFzaywgSERNSV9QSFlfTUFTSzApOwo+ICt9Cj4gKwo+ICtzdGF0aWMgdm9pZCBkd19oZG1p X3BoeV9jbGVhcl9ocGQoc3RydWN0IGR3X2hkbWkgKmhkbWksIHZvaWQgKmRhdGEpCj4gK3sKPiAr CS8qIENsZWFyIEhvdHBsdWcgaW50ZXJydXB0cyAqLwo+ICsJaGRtaV93cml0ZWIoaGRtaSwgSERN SV9JSF9QSFlfU1RBVDBfSFBEIHwgSERNSV9JSF9QSFlfU1RBVDBfUlhfU0VOU0UsCj4gKwkJICAg IEhETUlfSUhfUEhZX1NUQVQwKTsKPiArfQo+ICsKPiArc3RhdGljIHZvaWQgZHdfaGRtaV9waHlf dW5tdXRlX2hwZChzdHJ1Y3QgZHdfaGRtaSAqaGRtaSwgdm9pZCAqZGF0YSkKPiArewo+ICsJLyog VW5tdXRlIEhvdHBsdWcgaW50ZXJydXB0cyAqLwo+ICsJaGRtaV93cml0ZWIoaGRtaSwgfihIRE1J X0lIX1BIWV9TVEFUMF9IUEQgfCAKSERNSV9JSF9QSFlfU1RBVDBfUlhfU0VOU0UpLAo+ICsJCSAg ICBIRE1JX0lIX01VVEVfUEhZX1NUQVQwKTsKPiArfQoKRG8gd2UgcmVhbGx5IG5lZWQgYWxsIHRo b3NlIG5ldyBvcGVyYXRpb25zID8gSXQgbG9va3MgdG8gbWUgbGlrZSBhIGJpdCBvZiAKcmVmYWN0 b3JpbmcgY291bGQgaGVscCBsb3dlcmluZyB0aGUgbnVtYmVyIG9mIG9wZXJhdGlvbnMuIFdlIGVz c2VudGlhbGx5IG5lZWQKCi0gYW4gaW5pdCBmdW5jdGlvbiBjYWxsZWQgYXQgcHJvYmUgdGltZSAo b3IgbGlrZWx5IHJhdGhlciBhdCBydW50aW1lIFBNIHJlc3VtZSAKdGltZSB3aGVuIHdlJ2xsIGlt cGxlbWVudCBydW50aW1lIFBNKSAKCi0gYW4gaW50ZXJydXB0IGVuYWJsZS9kaXNhYmxlIGZ1bmN0 aW9uIHJvdWdobHkgZXF1aXZhbGVudCB0byAKZHdfaGRtaV91cGRhdGVfcGh5X21hc2soKQoKLSBh IHJlYWQgZnVuY3Rpb24gdG8gcmVwb3J0IHRoZSBkZXRlY3Rpb24gcmVzdWx0cyBjYWxsZWQgZnJv bSAKZHdfaGRtaV9jb25uZWN0b3JfZGV0ZWN0KCkKCj4gIHN0YXRpYyBjb25zdCBzdHJ1Y3QgZHdf aGRtaV9waHlfb3BzIGR3X2hkbWlfc3lub3BzeXNfcGh5X29wcyA9IHsKPiAgCS5pbml0ID0gZHdf aGRtaV9waHlfaW5pdCwKPiAgCS5kaXNhYmxlID0gZHdfaGRtaV9waHlfZGlzYWJsZSwKPiAgCS5y ZWFkX2hwZCA9IGR3X2hkbWlfcGh5X3JlYWRfaHBkLAo+ICsJLnVwZGF0ZV9ocGQgPSBkd19oZG1p X3BoeV91cGRhdGVfaHBkLAo+ICsJLnNldHVwX2hwZCA9IGR3X2hkbWlfcGh5X3NldHVwX2hwZCwK PiArCS5jb25maWd1cmVfaHBkID0gZHdfaGRtaV9waHlfY29uZmlndXJlX2hwZCwKPiArCS5jbGVh cl9ocGQgPSBkd19oZG1pX3BoeV9jbGVhcl9ocGQsCj4gKwkudW5tdXRlX2hwZCA9IGR3X2hkbWlf cGh5X3VubXV0ZV9ocGQsCj4gIH07Cj4gCj4gIC8qIC0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQo+IEBAIC0xNTA3 LDExICsxNTQ3LDEyIEBAIHN0YXRpYyBpbnQgZHdfaGRtaV9mYl9yZWdpc3RlcmVkKHN0cnVjdCBk d19oZG1pCj4gKmhkbWkpIEhETUlfUEhZX0kyQ01fQ1RMSU5UX0FERFIpOwo+IAo+ICAJLyogZW5h YmxlIGNhYmxlIGhvdCBwbHVnIGlycSAqLwo+IC0JaGRtaV93cml0ZWIoaGRtaSwgaGRtaS0+cGh5 X21hc2ssIEhETUlfUEhZX01BU0swKTsKPiArCWlmIChoZG1pLT5waHkub3BzLT5jb25maWd1cmVf aHBkKQo+ICsJCWhkbWktPnBoeS5vcHMtPmNvbmZpZ3VyZV9ocGQoaGRtaSwgaGRtaS0+cGh5LmRh dGEpOwo+IAo+ICAJLyogQ2xlYXIgSG90cGx1ZyBpbnRlcnJ1cHRzICovCj4gLQloZG1pX3dyaXRl YihoZG1pLCBIRE1JX0lIX1BIWV9TVEFUMF9IUEQgfCBIRE1JX0lIX1BIWV9TVEFUMF9SWF9TRU5T RSwKPiAtCQkgICAgSERNSV9JSF9QSFlfU1RBVDApOwo+ICsJaWYgKGhkbWktPnBoeS5vcHMtPmNs ZWFyX2hwZCkKPiArCQloZG1pLT5waHkub3BzLT5jbGVhcl9ocGQoaGRtaSwgaGRtaS0+cGh5LmRh dGEpOwoKVGhlIHByb2JlIGZ1bmN0aW9uIGNvbnRhaW5zIHRoZSBzYW1lIGNvZGUuIExldCdzIGlu bGluZSB0aGlzIGZ1bmN0aW9uIGludG8gCnByb2JlLCBncm91cCBhbGwgSFBELXJlbGF0ZWQgaW5p dGlhbGl6YXRpb24gdG9nZXRoZXIgYW5kIGV4dHJhY3QgdGhhdCBpbnRvIGEgCmZ1bmN0aW9uIHRv IGtlZXAgcHJvYmUgZWFzeSB0byByZWFkLiBJIHRoaW5rIHlvdSBjYW4gZG8gdGhhdCBpbiBhIHNl cGFyYXRlIApwYXRjaC4KCj4gIAlyZXR1cm4gMDsKPiAgfQo+IEBAIC0xNjIyLDEzICsxNjYzLDE0 IEBAIHN0YXRpYyB2b2lkIGR3X2hkbWlfdXBkYXRlX3BoeV9tYXNrKHN0cnVjdCBkd19oZG1pCj4g KmhkbWkpIHsKPiAgCXU4IG9sZF9tYXNrID0gaGRtaS0+cGh5X21hc2s7Cj4KPiAtCWlmIChoZG1p LT5mb3JjZSB8fCBoZG1pLT5kaXNhYmxlZCB8fCAhaGRtaS0+cnhzZW5zZSkKPiAtCQloZG1pLT5w aHlfbWFzayB8PSBIRE1JX1BIWV9SWF9TRU5TRTsKPiAtCWVsc2UKPiAtCQloZG1pLT5waHlfbWFz ayAmPSB+SERNSV9QSFlfUlhfU0VOU0U7Cj4gKwlpZiAoaGRtaS0+cGh5Lm9wcy0+dXBkYXRlX2hw ZCkKPiArCQloZG1pLT5waHkub3BzLT51cGRhdGVfaHBkKGhkbWksIGhkbWktPnBoeS5kYXRhLAo+ ICsJCQkJCSAgaGRtaS0+Zm9yY2UsIGhkbWktPmRpc2FibGVkLAo+ICsJCQkJCSAgaGRtaS0+cnhz ZW5zZSk7Cj4gCj4gLQlpZiAob2xkX21hc2sgIT0gaGRtaS0+cGh5X21hc2spCj4gLQkJaGRtaV93 cml0ZWIoaGRtaSwgaGRtaS0+cGh5X21hc2ssIEhETUlfUEhZX01BU0swKTsKPiArCWlmIChvbGRf bWFzayAhPSBoZG1pLT5waHlfbWFzayAmJgoKcGh5X21hc2sgaXNuJ3QgYWNjZXNzaWJsZSB0byBn bHVlIGNvZGUsIHNvIHRoaXMgdGVzdCB3aWxsIG5ldmVyIGJlIHRydWUgd2l0aCAKdmVuZG9yIFBI WXMuCgo+ICsJICAgIGhkbWktPnBoeS5vcHMtPmNvbmZpZ3VyZV9ocGQpCj4gKwkJaGRtaS0+cGh5 Lm9wcy0+Y29uZmlndXJlX2hwZChoZG1pLCBoZG1pLT5waHkuZGF0YSk7Cj4gIH0KPiAKPiAgc3Rh dGljIGVudW0gZHJtX2Nvbm5lY3Rvcl9zdGF0dXMKPiBAQCAtMTgyMCw2ICsxODYyLDQxIEBAIHN0 YXRpYyBpcnFyZXR1cm5fdCBkd19oZG1pX2hhcmRpcnEoaW50IGlycSwgdm9pZAo+ICpkZXZfaWQp IHJldHVybiByZXQ7Cj4gIH0KPiAKPiArdm9pZCBfX2R3X2hkbWlfc2V0dXBfcnhfc2Vuc2Uoc3Ry dWN0IGR3X2hkbWkgKmhkbWksIGJvb2wgaHBkLCBib29sCj4gcnhfc2Vuc2UpCj4gK3sKPiArCW11 dGV4X2xvY2soJmhkbWktPm11dGV4KTsKPiArCj4gKwlpZiAoIWhkbWktPmRpc2FibGVkICYmICFo ZG1pLT5mb3JjZSkgewo+ICsJCS8qCj4gKwkJICogSWYgdGhlIFJYIHNlbnNlIHN0YXR1cyBpbmRp Y2F0ZXMgd2UncmUgZGlzY29ubmVjdGVkLAo+ICsJCSAqIGNsZWFyIHRoZSBzb2Z0d2FyZSByeHNl bnNlIHN0YXR1cy4KPiArCQkgKi8KPiArCQlpZiAoIXJ4X3NlbnNlKQo+ICsJCQloZG1pLT5yeHNl bnNlID0gZmFsc2U7Cj4gKwo+ICsJCS8qCj4gKwkJICogT25seSBzZXQgdGhlIHNvZnR3YXJlIHJ4 c2Vuc2Ugc3RhdHVzIHdoZW4gYm90aAo+ICsJCSAqIHJ4c2Vuc2UgYW5kIGhwZCBpbmRpY2F0ZXMg d2UncmUgY29ubmVjdGVkLgo+ICsJCSAqIFRoaXMgYXZvaWRzIHdoYXQgc2VlbXMgdG8gYmUgYmFk IGJlaGF2aW91ciBpbgo+ICsJCSAqIGF0IGxlYXN0IGlNWDZTIHZlcnNpb25zIG9mIHRoZSBwaHku Cj4gKwkJICovCj4gKwkJaWYgKGhwZCkKPiArCQkJaGRtaS0+cnhzZW5zZSA9IHRydWU7CgpUaGlz IGNvbnRyYWRpY3RzIHRoZSBhYm92ZSBjb21tZW50LCBoZG1pLT5yeHNlbnNlIGlzIHNldCBiYWNr IHRvIHRydWUgc29sZWx5IApiYXNlZCBvbiB0aGUgaHBkIHBhcmFtZXRlci4KCj4gKwo+ICsJCWR3 X2hkbWlfdXBkYXRlX3Bvd2VyKGhkbWkpOwo+ICsJCWR3X2hkbWlfdXBkYXRlX3BoeV9tYXNrKGhk bWkpOwo+ICsJfQoKSSdkIGFkZCBhIGJsYW5rIGxpbmUgaGVyZS4KCj4gKwltdXRleF91bmxvY2so JmhkbWktPm11dGV4KTsKPiArfQo+ICsKPiArdm9pZCBkd19oZG1pX3NldHVwX3J4X3NlbnNlKHN0 cnVjdCBkZXZpY2UgKmRldiwgYm9vbCBocGQsIGJvb2wgcnhfc2Vuc2UpCj4gK3sKPiArCXN0cnVj dCBkd19oZG1pICpoZG1pID0gZGV2X2dldF9kcnZkYXRhKGRldik7Cj4gKwo+ICsJX19kd19oZG1p X3NldHVwX3J4X3NlbnNlKGhkbWksIGhwZCwgcnhfc2Vuc2UpOwo+ICt9Cj4gK0VYUE9SVF9TWU1C T0xfR1BMKGR3X2hkbWlfc2V0dXBfcnhfc2Vuc2UpOwo+ICsKPiAgc3RhdGljIGlycXJldHVybl90 IGR3X2hkbWlfaXJxKGludCBpcnEsIHZvaWQgKmRldl9pZCkKPiAgewo+ICAJc3RydWN0IGR3X2hk bWkgKmhkbWkgPSBkZXZfaWQ7Cj4gQEAgLTE4NTIsMzAgKzE5MjksMTAgQEAgc3RhdGljIGlycXJl dHVybl90IGR3X2hkbWlfaXJxKGludCBpcnEsIHZvaWQKPiAqZGV2X2lkKSAqIGFzayB0aGUgc291 cmNlIHRvIHJlLXJlYWQgdGhlIEVESUQuCj4gIAkgKi8KPiAgCWlmIChpbnRyX3N0YXQgJgo+IC0J ICAgIChIRE1JX0lIX1BIWV9TVEFUMF9SWF9TRU5TRSB8IEhETUlfSUhfUEhZX1NUQVQwX0hQRCkp IHsKPiAtCQltdXRleF9sb2NrKCZoZG1pLT5tdXRleCk7Cj4gLQkJaWYgKCFoZG1pLT5kaXNhYmxl ZCAmJiAhaGRtaS0+Zm9yY2UpIHsKPiAtCQkJLyoKPiAtCQkJICogSWYgdGhlIFJYIHNlbnNlIHN0 YXR1cyBpbmRpY2F0ZXMgd2UncmUgCmRpc2Nvbm5lY3RlZCwKPiAtCQkJICogY2xlYXIgdGhlIHNv ZnR3YXJlIHJ4c2Vuc2Ugc3RhdHVzLgo+IC0JCQkgKi8KPiAtCQkJaWYgKCEocGh5X3N0YXQgJiBI RE1JX1BIWV9SWF9TRU5TRSkpCj4gLQkJCQloZG1pLT5yeHNlbnNlID0gZmFsc2U7Cj4gLQo+IC0J CQkvKgo+IC0JCQkgKiBPbmx5IHNldCB0aGUgc29mdHdhcmUgcnhzZW5zZSBzdGF0dXMgd2hlbiBi b3RoCj4gLQkJCSAqIHJ4c2Vuc2UgYW5kIGhwZCBpbmRpY2F0ZXMgd2UncmUgY29ubmVjdGVkLgo+ IC0JCQkgKiBUaGlzIGF2b2lkcyB3aGF0IHNlZW1zIHRvIGJlIGJhZCBiZWhhdmlvdXIgaW4KPiAt CQkJICogYXQgbGVhc3QgaU1YNlMgdmVyc2lvbnMgb2YgdGhlIHBoeS4KPiAtCQkJICovCj4gLQkJ CWlmIChwaHlfc3RhdCAmIEhETUlfUEhZX0hQRCkKPiAtCQkJCWhkbWktPnJ4c2Vuc2UgPSB0cnVl Owo+IC0KPiAtCQkJZHdfaGRtaV91cGRhdGVfcG93ZXIoaGRtaSk7Cj4gLQkJCWR3X2hkbWlfdXBk YXRlX3BoeV9tYXNrKGhkbWkpOwo+IC0JCX0KPiAtCQltdXRleF91bmxvY2soJmhkbWktPm11dGV4 KTsKPiAtCX0KPiArCSAgICAoSERNSV9JSF9QSFlfU1RBVDBfUlhfU0VOU0UgfCBIRE1JX0lIX1BI WV9TVEFUMF9IUEQpKQoKSXMgdGhpcyByaWdodCA/IElmIHlvdXIgU29DIGltcGxlbWVudHMgYSBj dXN0b20gSFBEIG1lY2hhbmlzbSwgZG9lcyBpdCBzdGlsbCAKcmVseSBvbiB0aGUgc3RhbmRhcmQg UlggU0VOU0UgYW5kIEhQRCBpbnRlcnJ1cHRzID8gSW4gcGFydGljdWxhciwgdGhpcyAKZnVuY3Rp b24gc3RpbGwgd3JpdGVzIHRoZSBIRE1JX0lIX01VVEVfUEhZX1NUQVQwIHJlZ2lzdGVyIGRpcmVj dGx5LCB3aGlsZSAKeW91J3ZlIGV4dHJhY3RlZCBhIHdyaXRlIHRvIHRoZSBzYW1lIHJlZ2lzdGVy IGluIHRoZSBwcm9iZSBmdW5jdGlvbiBpbnRvIGEgUEhZIApvcGVyYXRpb24uCgo+ICsJCV9fZHdf aGRtaV9zZXR1cF9yeF9zZW5zZShoZG1pLAo+ICsJCQkJCSBwaHlfc3RhdCAmIEhETUlfUEhZX0hQ RCwKPiArCQkJCQkgcGh5X3N0YXQgJiBIRE1JX1BIWV9SWF9TRU5TRSk7Cj4gCj4gIAlpZiAoaW50 cl9zdGF0ICYgSERNSV9JSF9QSFlfU1RBVDBfSFBEKSB7Cj4gIAkJZGV2X2RiZyhoZG1pLT5kZXYs ICJFVkVOVD0lc1xuIiwKPiBAQCAtMjE0NiwxMSArMjIwMywxMiBAQCBzdGF0aWMgaW50IGR3X2hk bWlfZGV0ZWN0X3BoeShzdHJ1Y3QgZHdfaGRtaSAqaGRtaSkKPiAgCSAqIENvbmZpZ3VyZSByZWdp c3RlcnMgcmVsYXRlZCB0byBIRE1JIGludGVycnVwdAo+ICAJICogZ2VuZXJhdGlvbiBiZWZvcmUg cmVnaXN0ZXJpbmcgSVJRLgo+ICAJICovCj4gLQloZG1pX3dyaXRlYihoZG1pLCBIRE1JX1BIWV9I UEQgfCBIRE1JX1BIWV9SWF9TRU5TRSwgSERNSV9QSFlfUE9MMCk7Cj4gKwlpZiAoaGRtaS0+cGh5 Lm9wcy0+c2V0dXBfaHBkKQo+ICsJCWhkbWktPnBoeS5vcHMtPnNldHVwX2hwZChoZG1pLCBoZG1p LT5waHkuZGF0YSk7Cj4gCj4gIAkvKiBDbGVhciBIb3RwbHVnIGludGVycnVwdHMgKi8KPiAtCWhk bWlfd3JpdGViKGhkbWksIEhETUlfSUhfUEhZX1NUQVQwX0hQRCB8IEhETUlfSUhfUEhZX1NUQVQw X1JYX1NFTlNFLAo+IC0JCSAgICBIRE1JX0lIX1BIWV9TVEFUMCk7Cj4gKwlpZiAoaGRtaS0+cGh5 Lm9wcy0+Y2xlYXJfaHBkKQo+ICsJCWhkbWktPnBoeS5vcHMtPmNsZWFyX2hwZChoZG1pLCBoZG1p LT5waHkuZGF0YSk7Cj4gCj4gIAloZG1pLT5icmlkZ2UuZHJpdmVyX3ByaXZhdGUgPSBoZG1pOwo+ ICAJaGRtaS0+YnJpZGdlLmZ1bmNzID0gJmR3X2hkbWlfYnJpZGdlX2Z1bmNzOwo+IEBAIC0yMTYz LDggKzIyMjEsOCBAQCBzdGF0aWMgaW50IGR3X2hkbWlfZGV0ZWN0X3BoeShzdHJ1Y3QgZHdfaGRt aSAqaGRtaSkKPiAgCQlnb3RvIGVycl9pYWhiOwo+IAo+ICAJLyogVW5tdXRlIGludGVycnVwdHMg Ki8KPiAtCWhkbWlfd3JpdGViKGhkbWksIH4oSERNSV9JSF9QSFlfU1RBVDBfSFBEIHwgCkhETUlf SUhfUEhZX1NUQVQwX1JYX1NFTlNFKSwKPiAtCQkgICAgSERNSV9JSF9NVVRFX1BIWV9TVEFUMCk7 Cj4gKwlpZiAoaGRtaS0+cGh5Lm9wcy0+dW5tdXRlX2hwZCkKPiArCQloZG1pLT5waHkub3BzLT51 bm11dGVfaHBkKGhkbWksIGhkbWktPnBoeS5kYXRhKTsKPiAKPiAgCW1lbXNldCgmcGRldmluZm8s IDAsIHNpemVvZihwZGV2aW5mbykpOwo+ICAJcGRldmluZm8ucGFyZW50ID0gZGV2Owo+IGRpZmYg LS1naXQgYS9pbmNsdWRlL2RybS9icmlkZ2UvZHdfaGRtaS5oIGIvaW5jbHVkZS9kcm0vYnJpZGdl L2R3X2hkbWkuaAo+IGluZGV4IDhjMGNjMTMuLmQ3MjQwM2YgMTAwNjQ0Cj4gLS0tIGEvaW5jbHVk ZS9kcm0vYnJpZGdlL2R3X2hkbWkuaAo+ICsrKyBiL2luY2x1ZGUvZHJtL2JyaWRnZS9kd19oZG1p LmgKPiBAQCAtNjQsNiArNjQsMTIgQEAgc3RydWN0IGR3X2hkbWlfcGh5X29wcyB7Cj4gIAkJICAg IHN0cnVjdCBkcm1fZGlzcGxheV9tb2RlICptb2RlKTsKPiAgCXZvaWQgKCpkaXNhYmxlKShzdHJ1 Y3QgZHdfaGRtaSAqaGRtaSwgdm9pZCAqZGF0YSk7Cj4gIAllbnVtIGRybV9jb25uZWN0b3Jfc3Rh dHVzICgqcmVhZF9ocGQpKHN0cnVjdCBkd19oZG1pICpoZG1pLCB2b2lkIAoqZGF0YSk7Cj4gKwl2 b2lkICgqdXBkYXRlX2hwZCkoc3RydWN0IGR3X2hkbWkgKmhkbWksIHZvaWQgKmRhdGEsCj4gKwkJ CSAgIGJvb2wgZm9yY2UsIGJvb2wgZGlzYWJsZWQsIGJvb2wgcnhzZW5zZSk7Cj4gKwl2b2lkICgq c2V0dXBfaHBkKShzdHJ1Y3QgZHdfaGRtaSAqaGRtaSwgdm9pZCAqZGF0YSk7Cj4gKwl2b2lkICgq Y29uZmlndXJlX2hwZCkoc3RydWN0IGR3X2hkbWkgKmhkbWksIHZvaWQgKmRhdGEpOwo+ICsJdm9p ZCAoKmNsZWFyX2hwZCkoc3RydWN0IGR3X2hkbWkgKmhkbWksIHZvaWQgKmRhdGEpOwo+ICsJdm9p ZCAoKnVubXV0ZV9ocGQpKHN0cnVjdCBkd19oZG1pICpoZG1pLCB2b2lkICpkYXRhKTsKClRoYXQn cyBxdWl0ZSBhIGxvdCBvZiBuZXcgb3BlcmF0aW9ucy4gSSB0aGluayB3ZSBuZWVkIGRvY3VtZW50 YXRpb24gOi0pCgo+ICB9Owo+IAo+ICBzdHJ1Y3QgZHdfaGRtaV9wbGF0X2RhdGEgewo+IEBAIC05 Myw2ICs5OSw4IEBAIGludCBkd19oZG1pX3Byb2JlKHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2UgKnBk ZXYsCj4gIGludCBkd19oZG1pX2JpbmQoc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldiwgc3Ry dWN0IGRybV9lbmNvZGVyICplbmNvZGVyLAo+IGNvbnN0IHN0cnVjdCBkd19oZG1pX3BsYXRfZGF0 YSAqcGxhdF9kYXRhKTsKPiAKPiArdm9pZCBkd19oZG1pX3NldHVwX3J4X3NlbnNlKHN0cnVjdCBk ZXZpY2UgKmRldiwgYm9vbCBocGQsIGJvb2wgcnhfc2Vuc2UpOwo+ICsKPiAgdm9pZCBkd19oZG1p X3NldF9zYW1wbGVfcmF0ZShzdHJ1Y3QgZHdfaGRtaSAqaGRtaSwgdW5zaWduZWQgaW50IHJhdGUp Owo+ICB2b2lkIGR3X2hkbWlfYXVkaW9fZW5hYmxlKHN0cnVjdCBkd19oZG1pICpoZG1pKTsKPiAg dm9pZCBkd19oZG1pX2F1ZGlvX2Rpc2FibGUoc3RydWN0IGR3X2hkbWkgKmhkbWkpOwoKLS0gClJl Z2FyZHMsCgpMYXVyZW50IFBpbmNoYXJ0CgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5m cmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0 aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752810AbdCBQTs (ORCPT ); Thu, 2 Mar 2017 11:19:48 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:58676 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752009AbdCBQTr (ORCPT ); Thu, 2 Mar 2017 11:19:47 -0500 From: Laurent Pinchart To: Neil Armstrong Cc: dri-devel@lists.freedesktop.org, laurent.pinchart+renesas@ideasonboard.com, Jose.Abreu@synopsys.com, kieran.bingham@ideasonboard.com, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/2] drm: bridge: Move HPD handling to PHY operations Date: Thu, 02 Mar 2017 18:18:24 +0200 Message-ID: <6652377.Pu8amSWD8H@avalon> User-Agent: KMail/4.14.10 (Linux/4.9.6-gentoo-r1; KDE/4.14.28; x86_64; ; ) In-Reply-To: <1488468572-31971-3-git-send-email-narmstrong@baylibre.com> References: <1488468572-31971-1-git-send-email-narmstrong@baylibre.com> <1488468572-31971-3-git-send-email-narmstrong@baylibre.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Neil, Thank you for the patch. On Thursday 02 Mar 2017 16:29:32 Neil Armstrong wrote: > The HDMI TX controller support HPD and RXSENSE signaling from the PHY > via it's STAT0 PHY interface, but some vendor PHYs can manage these > signals independently from the controller, thus these STAT0 handling > should be moved to PHY specific operations and become optional. > > The existing STAT0 HPD and RXSENSE handling code is refactored into > a supplementaty set of default PHY operations that are used automatically > when the platform glue doesn't provide its own operations. > > Signed-off-by: Neil Armstrong > --- > drivers/gpu/drm/bridge/dw-hdmi.c | 134 +++++++++++++++++++++++++----------- > include/drm/bridge/dw_hdmi.h | 8 +++ > 2 files changed, 104 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c > b/drivers/gpu/drm/bridge/dw-hdmi.c index 653ecd7..58dcf7d 100644 > --- a/drivers/gpu/drm/bridge/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c > @@ -1098,10 +1098,50 @@ static enum drm_connector_status > dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, connector_status_connected : > connector_status_disconnected; > } > > +static void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > + bool force, bool disabled, bool rxsense) > +{ > + if (force || disabled || !rxsense) > + hdmi->phy_mask |= HDMI_PHY_RX_SENSE; > + else > + hdmi->phy_mask &= ~HDMI_PHY_RX_SENSE; > +} > + > +static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data) > +{ > + /* setup HPD and RXSENSE interrupt polarity */ > + hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); > +} > + > +static void dw_hdmi_phy_configure_hpd(struct dw_hdmi *hdmi, void *data) > +{ > + /* enable cable hot plug irq */ > + hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); > +} > + > +static void dw_hdmi_phy_clear_hpd(struct dw_hdmi *hdmi, void *data) > +{ > + /* Clear Hotplug interrupts */ > + hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, > + HDMI_IH_PHY_STAT0); > +} > + > +static void dw_hdmi_phy_unmute_hpd(struct dw_hdmi *hdmi, void *data) > +{ > + /* Unmute Hotplug interrupts */ > + hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), > + HDMI_IH_MUTE_PHY_STAT0); > +} Do we really need all those new operations ? It looks to me like a bit of refactoring could help lowering the number of operations. We essentially need - an init function called at probe time (or likely rather at runtime PM resume time when we'll implement runtime PM) - an interrupt enable/disable function roughly equivalent to dw_hdmi_update_phy_mask() - a read function to report the detection results called from dw_hdmi_connector_detect() > static const struct dw_hdmi_phy_ops dw_hdmi_synopsys_phy_ops = { > .init = dw_hdmi_phy_init, > .disable = dw_hdmi_phy_disable, > .read_hpd = dw_hdmi_phy_read_hpd, > + .update_hpd = dw_hdmi_phy_update_hpd, > + .setup_hpd = dw_hdmi_phy_setup_hpd, > + .configure_hpd = dw_hdmi_phy_configure_hpd, > + .clear_hpd = dw_hdmi_phy_clear_hpd, > + .unmute_hpd = dw_hdmi_phy_unmute_hpd, > }; > > /* ------------------------------------------------------------------------ > @@ -1507,11 +1547,12 @@ static int dw_hdmi_fb_registered(struct dw_hdmi > *hdmi) HDMI_PHY_I2CM_CTLINT_ADDR); > > /* enable cable hot plug irq */ > - hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); > + if (hdmi->phy.ops->configure_hpd) > + hdmi->phy.ops->configure_hpd(hdmi, hdmi->phy.data); > > /* Clear Hotplug interrupts */ > - hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, > - HDMI_IH_PHY_STAT0); > + if (hdmi->phy.ops->clear_hpd) > + hdmi->phy.ops->clear_hpd(hdmi, hdmi->phy.data); The probe function contains the same code. Let's inline this function into probe, group all HPD-related initialization together and extract that into a function to keep probe easy to read. I think you can do that in a separate patch. > return 0; > } > @@ -1622,13 +1663,14 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi > *hdmi) { > u8 old_mask = hdmi->phy_mask; > > - if (hdmi->force || hdmi->disabled || !hdmi->rxsense) > - hdmi->phy_mask |= HDMI_PHY_RX_SENSE; > - else > - hdmi->phy_mask &= ~HDMI_PHY_RX_SENSE; > + if (hdmi->phy.ops->update_hpd) > + hdmi->phy.ops->update_hpd(hdmi, hdmi->phy.data, > + hdmi->force, hdmi->disabled, > + hdmi->rxsense); > > - if (old_mask != hdmi->phy_mask) > - hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); > + if (old_mask != hdmi->phy_mask && phy_mask isn't accessible to glue code, so this test will never be true with vendor PHYs. > + hdmi->phy.ops->configure_hpd) > + hdmi->phy.ops->configure_hpd(hdmi, hdmi->phy.data); > } > > static enum drm_connector_status > @@ -1820,6 +1862,41 @@ static irqreturn_t dw_hdmi_hardirq(int irq, void > *dev_id) return ret; > } > > +void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool > rx_sense) > +{ > + mutex_lock(&hdmi->mutex); > + > + if (!hdmi->disabled && !hdmi->force) { > + /* > + * If the RX sense status indicates we're disconnected, > + * clear the software rxsense status. > + */ > + if (!rx_sense) > + hdmi->rxsense = false; > + > + /* > + * Only set the software rxsense status when both > + * rxsense and hpd indicates we're connected. > + * This avoids what seems to be bad behaviour in > + * at least iMX6S versions of the phy. > + */ > + if (hpd) > + hdmi->rxsense = true; This contradicts the above comment, hdmi->rxsense is set back to true solely based on the hpd parameter. > + > + dw_hdmi_update_power(hdmi); > + dw_hdmi_update_phy_mask(hdmi); > + } I'd add a blank line here. > + mutex_unlock(&hdmi->mutex); > +} > + > +void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense) > +{ > + struct dw_hdmi *hdmi = dev_get_drvdata(dev); > + > + __dw_hdmi_setup_rx_sense(hdmi, hpd, rx_sense); > +} > +EXPORT_SYMBOL_GPL(dw_hdmi_setup_rx_sense); > + > static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) > { > struct dw_hdmi *hdmi = dev_id; > @@ -1852,30 +1929,10 @@ static irqreturn_t dw_hdmi_irq(int irq, void > *dev_id) * ask the source to re-read the EDID. > */ > if (intr_stat & > - (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) { > - mutex_lock(&hdmi->mutex); > - if (!hdmi->disabled && !hdmi->force) { > - /* > - * If the RX sense status indicates we're disconnected, > - * clear the software rxsense status. > - */ > - if (!(phy_stat & HDMI_PHY_RX_SENSE)) > - hdmi->rxsense = false; > - > - /* > - * Only set the software rxsense status when both > - * rxsense and hpd indicates we're connected. > - * This avoids what seems to be bad behaviour in > - * at least iMX6S versions of the phy. > - */ > - if (phy_stat & HDMI_PHY_HPD) > - hdmi->rxsense = true; > - > - dw_hdmi_update_power(hdmi); > - dw_hdmi_update_phy_mask(hdmi); > - } > - mutex_unlock(&hdmi->mutex); > - } > + (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) Is this right ? If your SoC implements a custom HPD mechanism, does it still rely on the standard RX SENSE and HPD interrupts ? In particular, this function still writes the HDMI_IH_MUTE_PHY_STAT0 register directly, while you've extracted a write to the same register in the probe function into a PHY operation. > + __dw_hdmi_setup_rx_sense(hdmi, > + phy_stat & HDMI_PHY_HPD, > + phy_stat & HDMI_PHY_RX_SENSE); > > if (intr_stat & HDMI_IH_PHY_STAT0_HPD) { > dev_dbg(hdmi->dev, "EVENT=%s\n", > @@ -2146,11 +2203,12 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > * Configure registers related to HDMI interrupt > * generation before registering IRQ. > */ > - hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); > + if (hdmi->phy.ops->setup_hpd) > + hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); > > /* Clear Hotplug interrupts */ > - hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, > - HDMI_IH_PHY_STAT0); > + if (hdmi->phy.ops->clear_hpd) > + hdmi->phy.ops->clear_hpd(hdmi, hdmi->phy.data); > > hdmi->bridge.driver_private = hdmi; > hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; > @@ -2163,8 +2221,8 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > goto err_iahb; > > /* Unmute interrupts */ > - hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), > - HDMI_IH_MUTE_PHY_STAT0); > + if (hdmi->phy.ops->unmute_hpd) > + hdmi->phy.ops->unmute_hpd(hdmi, hdmi->phy.data); > > memset(&pdevinfo, 0, sizeof(pdevinfo)); > pdevinfo.parent = dev; > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > index 8c0cc13..d72403f 100644 > --- a/include/drm/bridge/dw_hdmi.h > +++ b/include/drm/bridge/dw_hdmi.h > @@ -64,6 +64,12 @@ struct dw_hdmi_phy_ops { > struct drm_display_mode *mode); > void (*disable)(struct dw_hdmi *hdmi, void *data); > enum drm_connector_status (*read_hpd)(struct dw_hdmi *hdmi, void *data); > + void (*update_hpd)(struct dw_hdmi *hdmi, void *data, > + bool force, bool disabled, bool rxsense); > + void (*setup_hpd)(struct dw_hdmi *hdmi, void *data); > + void (*configure_hpd)(struct dw_hdmi *hdmi, void *data); > + void (*clear_hpd)(struct dw_hdmi *hdmi, void *data); > + void (*unmute_hpd)(struct dw_hdmi *hdmi, void *data); That's quite a lot of new operations. I think we need documentation :-) > }; > > struct dw_hdmi_plat_data { > @@ -93,6 +99,8 @@ int dw_hdmi_probe(struct platform_device *pdev, > int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder, > const struct dw_hdmi_plat_data *plat_data); > > +void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense); > + > void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate); > void dw_hdmi_audio_enable(struct dw_hdmi *hdmi); > void dw_hdmi_audio_disable(struct dw_hdmi *hdmi); -- Regards, Laurent Pinchart