From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Stuebner Subject: Re: [PATCH v2 4/7] drm/rockchip: dw_hdmi: add inno hdmi phy ops Date: Sat, 09 Dec 2017 18:09:52 +0100 Message-ID: <1710749.RVigaV1YNW@phil> References: <1506735886-151761-1-git-send-email-algea.cao@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1506735886-151761-1-git-send-email-algea.cao@rock-chips.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: dri-devel@lists.freedesktop.org Cc: Algea Cao , airlied@linux.ie, linux-kernel@vger.kernel.org, kever.yang@rock-chips.com, linux-rockchip@lists.infradead.org, daniel.vetter@intel.com, yang.zheng@rock-chips.com List-Id: linux-rockchip.vger.kernel.org SGkgQWxnZWEsCgpBbSBTYW1zdGFnLCAzMC4gU2VwdGVtYmVyIDIwMTcsIDA5OjQ0OjQ2IENFVCBz Y2hyaWViIEFsZ2VhIENhbzoKPiBCZWNhdXNlIHNvbWUgUksgY2hpcHMgdXNlIGlubm8gaGRtaSBw aHksIHN1Y2ggYXMgUkszMzI4LCB3ZSBhZGQKPiBpbm5vIGhkbWkgcGh5IG9wcy4KPiAKPiBTaWdu ZWQtb2ZmLWJ5OiBBbGdlYSBDYW8gPGFsZ2VhLmNhb0Byb2NrLWNoaXBzLmNvbT4KPiAtLS0KPiAg ZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL2R3X2hkbWktcm9ja2NoaXAuYyB8IDExMCArKysrKysr KysrKysrKysrKysrKysrKysrKystCj4gIDEgZmlsZSBjaGFuZ2VkLCAxMDcgaW5zZXJ0aW9ucygr KSwgMyBkZWxldGlvbnMoLSkKPiAKPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL3JvY2tj aGlwL2R3X2hkbWktcm9ja2NoaXAuYyBiL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9kd19oZG1p LXJvY2tjaGlwLmMKPiBpbmRleCAwOWM3N2Y5Li43NjU4YjJmIDEwMDY0NAo+IC0tLSBhL2RyaXZl cnMvZ3B1L2RybS9yb2NrY2hpcC9kd19oZG1pLXJvY2tjaGlwLmMKPiArKysgYi9kcml2ZXJzL2dw dS9kcm0vcm9ja2NoaXAvZHdfaGRtaS1yb2NrY2hpcC5jCj4gQEAgLTEyLDcgKzEyLDcgQEAKPiAg I2luY2x1ZGUgPGxpbnV4L21vZHVsZS5oPgo+ICAjaW5jbHVkZSA8bGludXgvcGxhdGZvcm1fZGV2 aWNlLmg+Cj4gICNpbmNsdWRlIDxsaW51eC9yZWdtYXAuaD4KPiAtCj4gKyNpbmNsdWRlIDxsaW51 eC9waHkvcGh5Lmg+Cj4gICNpbmNsdWRlIDxkcm0vZHJtX29mLmg+Cj4gICNpbmNsdWRlIDxkcm0v ZHJtUC5oPgo+ICAjaW5jbHVkZSA8ZHJtL2RybV9jcnRjX2hlbHBlci5oPgo+IEBAIC0yNiw2ICsy NiwxOCBAQAo+ICAjZGVmaW5lIFJLMzI4OF9IRE1JX0xDRENfU0VMCQlCSVQoNCkKPiAgI2RlZmlu ZSBSSzMzOTlfR1JGX1NPQ19DT04yMAkJMHg2MjUwCj4gICNkZWZpbmUgUkszMzk5X0hETUlfTENE Q19TRUwJCUJJVCg2KQo+ICsjZGVmaW5lIFJLMzMyOF9HUkZfU09DX0NPTjIJCTB4MDQwOAo+ICsj ZGVmaW5lIFJLMzMyOF9ERENfTUFTS19FTgkJKCgzIDw8IDEwKSB8ICgzIDw8ICgxMCArIDE2KSkp Cj4gKyNkZWZpbmUgUkszMzI4X0dSRl9TT0NfQ09OMwkJMHgwNDBjCj4gKyNkZWZpbmUgUkszMzI4 X0lPX0NUUkxfQllfSERNSQkJKDB4ZjAwMDAwMDAgfCBCSVQoMTMpIHwgQklUKDEyKSkKPiArI2Rl ZmluZSBSSzMzMjhfR1JGX1NPQ19DT040CQkweDA0MTAKPiArI2RlZmluZSBSSzMzMjhfSU9fM1Zf RE9NQUlOCQkoNyA8PCAoOSArIDE2KSkKPiArI2RlZmluZSBSSzMzMjhfSU9fNVZfRE9NQUlOCQko KDcgPDwgOSkgfCAoMyA8PCAoOSArIDE2KSkpCgpUaGlzIGlzIHNsaWdodGx5IGNvbmZ1c2luZy4g KDkrMTYpIGlzIG9idmlvdXNseSB0aGUgd3JpdGUtZW5hYmxlLW1hc2ssIHNvCnNob3VsZG4ndCB0 aGUgd3JpdGUtZW5hYmxlLW1hc2sgbm90IGF0IGxlYXN0IGNvdmVyIHRoZSBjaGFuZ2VkIGJpdHM/ CgoKPiArc3RhdGljIGVudW0gZHJtX2Nvbm5lY3Rvcl9zdGF0dXMKPiAraW5ub19kd19oZG1pX3Bo eV9yZWFkX2hwZChzdHJ1Y3QgZHdfaGRtaSAqZHdfaGRtaSwgdm9pZCAqZGF0YSkKPiArewo+ICsJ c3RydWN0IHJvY2tjaGlwX2hkbWkgKmhkbWkgPSAoc3RydWN0IHJvY2tjaGlwX2hkbWkgKilkYXRh Owo+ICsJZW51bSBkcm1fY29ubmVjdG9yX3N0YXR1cyBzdGF0dXM7Cj4gKwo+ICsJc3RhdHVzID0g ZHdfaGRtaV9waHlfcmVhZF9ocGQoZHdfaGRtaSwgZGF0YSk7Cj4gKwo+ICsJaWYgKGhkbWktPmRl dl90eXBlID09IFJLMzIyOF9IRE1JKQo+ICsJCXJldHVybiBzdGF0dXM7CgpUaGVyZSBpcyBubyBu ZWVkIHRvIGVuY29kZSB0aGUgZnVuY3Rpb25hbGl0eSBmb3IgYm90aCB2YXJpYW50cwooYW5kIHBv c3NpYmx5IGxhdGVyIG9uZXMpIGludG8gb25lIGZ1bmN0aW9uLgoKQXMgTmVpbCBzYWlkIGluIGhp cyByZXBseSB0byBwYXRjaDIsIHdlIGRvbid0IHJlYWxseSB3YW50IHRvIGNhcnJ5CnRoYXQgZGV2 X3R5cGUgcHJvcGVydHksIHNvIHdoeSBkb24ndCB5b3UganVzdCBkZWZpbmUgdGhpcyBhcwpyazMz MjhfaW5ub19waHlfcmVhZF9ocGQgYW5kIHRoZW4gdXNlIGl0IGxpa2UgCgpzdGF0aWMgY29uc3Qg c3RydWN0IGR3X2hkbWlfcGh5X29wcyByazMyMjhfZHdfaGRtaV9waHlfb3BzID0gewoJLmluaXQJ CT0gaW5ub19kd19oZG1pX3BoeV9pbml0LAoJLmRpc2FibGUJPSBpbm5vX2R3X2hkbWlfcGh5X2Rp c2FibGUsCgkucmVhZF9ocGQJPSBkd19oZG1pX3BoeV9yZWFkX2hwZCwKfTsKCnN0YXRpYyBjb25z dCBzdHJ1Y3QgZHdfaGRtaV9waHlfb3BzIHJrMzMyOF9kd19oZG1pX3BoeV9vcHMgPSB7CgkuaW5p dAkJPSBpbm5vX2R3X2hkbWlfcGh5X2luaXQsCgkuZGlzYWJsZQk9IGlubm9fZHdfaGRtaV9waHlf ZGlzYWJsZSwKCS5yZWFkX2hwZAk9IHJrMzMyOF9pbm5vX3BoeV9yZWFkX2hwZCwKfTsKCj4gKwlp ZiAoc3RhdHVzID09IGNvbm5lY3Rvcl9zdGF0dXNfY29ubmVjdGVkKQo+ICsJCXJlZ21hcF93cml0 ZShoZG1pLT5yZWdtYXAsCj4gKwkJCSAgICAgUkszMzI4X0dSRl9TT0NfQ09ONCwKPiArCQkJICAg ICBSSzMzMjhfSU9fNVZfRE9NQUlOKTsKPiArCWVsc2UKPiArCQlyZWdtYXBfd3JpdGUoaGRtaS0+ cmVnbWFwLAo+ICsJCQkgICAgIFJLMzMyOF9HUkZfU09DX0NPTjQsCj4gKwkJCSAgICAgUkszMzI4 X0lPXzNWX0RPTUFJTik7CgpUaGF0IHNob3VsZCBkZWZpbml0ZWx5IGdldCBhIGNvbW1lbnQgZXhw bGFpbmluZyB3aGF0IHRoaXMgaXMgZG9pbmcgYW5kCndoeSA6LSkgLiBFc3BlY2lhbGx5IGFzIGl0 IHNlZW1zIHJlbGF0ZWQgdG8gdGhlIHBsdWcvdW5wbHVnIGV2ZW50cy4KCkFjY29yZGluZyB0byB0 aGUgVFJNLCB0aGUgaGRtaXBoeSBJUCBibG9jayBoYXMgaXRzIG93biBpbnRlcnJ1cHQuIENhbgp5 b3UgdGVsbCBtZSBpZiBpdCBpcyBmaXJpbmcgb24gaG90cGx1ZyBldmVudHM/IEJlY2F1c2UgSSdt IHdvbmRlcmluZyBpZgp0aGVzZSBHUkYgc2V0dGluZ3Mgd291bGRuJ3QgYmUgYmV0dGVyIGxpdmUg aW4gdGhlIGhkbWlwaHkgZHJpdmVyCltpZiBpdCBnZXRzIGFuIGlycSBvbiBwbHVnL3VucGx1Z10s IHdoaWNoIGluIHR1cm4gd291bGQgbWFrZSB0aGUgcGh5CmhhbmRsaW5nIGluIGR3X2hkbWkgYSBs b3QgZWFzaWVyLgoKPiArCXJldHVybiBzdGF0dXM7Cj4gK30KPiArCgpbLi4uXQoKPiBAQCAtMzYx LDcgKzQ1Myw3IEBAIHN0YXRpYyBpbnQgZHdfaGRtaV9yb2NrY2hpcF9iaW5kKHN0cnVjdCBkZXZp Y2UgKmRldiwgc3RydWN0IGRldmljZSAqbWFzdGVyLAo+ICAJCXJldHVybiAtRU5PTUVNOwo+ICAK PiAgCW1hdGNoID0gb2ZfbWF0Y2hfbm9kZShkd19oZG1pX3JvY2tjaGlwX2R0X2lkcywgcGRldi0+ ZGV2Lm9mX25vZGUpOwo+IC0JcGxhdF9kYXRhID0gbWF0Y2gtPmRhdGE7Cj4gKwlwbGF0X2RhdGEg PSAoc3RydWN0IGR3X2hkbWlfcGxhdF9kYXRhICopbWF0Y2gtPmRhdGE7Cj4gIAloZG1pLT5kZXYg PSAmcGRldi0+ZGV2Owo+ICAJaGRtaS0+Y2hpcF9kYXRhID0gcGxhdF9kYXRhLT5waHlfZGF0YTsK PiAgCWhkbWktPmRldl90eXBlID0gcGxhdF9kYXRhLT5kZXZfdHlwZTsKPiBAQCAtMzgzLDYgKzQ3 NSwxOCBAQCBzdGF0aWMgaW50IGR3X2hkbWlfcm9ja2NoaXBfYmluZChzdHJ1Y3QgZGV2aWNlICpk ZXYsIHN0cnVjdCBkZXZpY2UgKm1hc3RlciwKPiAgCQlyZXR1cm4gcmV0Owo+ICAJfQo+ICAKPiAr CWlmIChoZG1pLT5kZXZfdHlwZSA9PSBSSzMzMjhfSERNSSB8fCBoZG1pLT5kZXZfdHlwZSA9PSBS SzMyMjhfSERNSSkgewo+ICsJCWhkbWktPnBoeSA9IGRldm1fcGh5X2dldChkZXYsICJoZG1pX3Bo eSIpOwo+ICsJCWlmIChJU19FUlIoaGRtaS0+cGh5KSkgewo+ICsJCQlyZXQgPSBQVFJfRVJSKGhk bWktPnBoeSk7Cj4gKwkJCWRldl9lcnIoZGV2LCAiZmFpbGVkIHRvIGdldCBwaHk6ICVkXG4iLCBy ZXQpOwo+ICsJCQlyZXR1cm4gcmV0Owo+ICsJCX0KPiArCQlwbGF0X2RhdGEtPnBoeV9kYXRhID0g aGRtaTsKPiArCQlyZXQgPSBpbm5vX2R3X2hkbWlfaW5pdChoZG1pKTsKPiArCQlpZiAocmV0IDwg MCkKPiArCQkJcmV0dXJuIHJldDsKPiArCX0KCllvdSBjb3VsZCBhbHNvIGp1c3QgZGVjbGFyZSBp dCBvcHRpb25hbCBpbiB0aGUgYmluZGluZywgdHJ5IHRvIGdldCB0aGUgcGh5CnZpYSBkZXZtX3Bo eV9vcHRpb25hbF9nZXQgYW5kIGlmIGl0J3MgTlVMTCBqdXN0IGFzc3VtZSB0aGF0IGl0J3MgdGhl CnJlZ3VsYXIgaW50ZXJuYWwgcGh5IGFzIHdpdGggcHJldmlvdXMgc29jcy4gVGhhdCB3YXkgeW91 IGNhbiBkcm9wIHRoZQpkZXYtdHlwZS1jaGVjay4KCkFrYSwgaWYgYSBwaHkgaXMgZGVmaW5lZCB3 ZSBhcmUgcHJldHR5IHN1cmUgd2Ugd2FudCB0byB1c2UgdGhhdCBvbmUgOi0pIC4KCgpIZWlrbwpf X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwg bWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751374AbdLIRKX (ORCPT ); Sat, 9 Dec 2017 12:10:23 -0500 Received: from gloria.sntech.de ([95.129.55.99]:56724 "EHLO gloria.sntech.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751100AbdLIRKW (ORCPT ); Sat, 9 Dec 2017 12:10:22 -0500 From: Heiko Stuebner To: dri-devel@lists.freedesktop.org Cc: Algea Cao , daniel.vetter@intel.com, jani.nikula@linux.intel.com, seanpaul@chromium.org, airlied@linux.ie, linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, yang.zheng@rock-chips.com, kever.yang@rock-chips.com Subject: Re: [PATCH v2 4/7] drm/rockchip: dw_hdmi: add inno hdmi phy ops Date: Sat, 09 Dec 2017 18:09:52 +0100 Message-ID: <1710749.RVigaV1YNW@phil> User-Agent: KMail/5.2.3 (Linux/4.13.0-1-amd64; KDE/5.37.0; x86_64; ; ) In-Reply-To: <1506735886-151761-1-git-send-email-algea.cao@rock-chips.com> References: <1506735886-151761-1-git-send-email-algea.cao@rock-chips.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 Algea, Am Samstag, 30. September 2017, 09:44:46 CET schrieb Algea Cao: > Because some RK chips use inno hdmi phy, such as RK3328, we add > inno hdmi phy ops. > > Signed-off-by: Algea Cao > --- > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 110 +++++++++++++++++++++++++++- > 1 file changed, 107 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > index 09c77f9..7658b2f 100644 > --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > @@ -12,7 +12,7 @@ > #include > #include > #include > - > +#include > #include > #include > #include > @@ -26,6 +26,18 @@ > #define RK3288_HDMI_LCDC_SEL BIT(4) > #define RK3399_GRF_SOC_CON20 0x6250 > #define RK3399_HDMI_LCDC_SEL BIT(6) > +#define RK3328_GRF_SOC_CON2 0x0408 > +#define RK3328_DDC_MASK_EN ((3 << 10) | (3 << (10 + 16))) > +#define RK3328_GRF_SOC_CON3 0x040c > +#define RK3328_IO_CTRL_BY_HDMI (0xf0000000 | BIT(13) | BIT(12)) > +#define RK3328_GRF_SOC_CON4 0x0410 > +#define RK3328_IO_3V_DOMAIN (7 << (9 + 16)) > +#define RK3328_IO_5V_DOMAIN ((7 << 9) | (3 << (9 + 16))) This is slightly confusing. (9+16) is obviously the write-enable-mask, so shouldn't the write-enable-mask not at least cover the changed bits? > +static enum drm_connector_status > +inno_dw_hdmi_phy_read_hpd(struct dw_hdmi *dw_hdmi, void *data) > +{ > + struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; > + enum drm_connector_status status; > + > + status = dw_hdmi_phy_read_hpd(dw_hdmi, data); > + > + if (hdmi->dev_type == RK3228_HDMI) > + return status; There is no need to encode the functionality for both variants (and possibly later ones) into one function. As Neil said in his reply to patch2, we don't really want to carry that dev_type property, so why don't you just define this as rk3328_inno_phy_read_hpd and then use it like static const struct dw_hdmi_phy_ops rk3228_dw_hdmi_phy_ops = { .init = inno_dw_hdmi_phy_init, .disable = inno_dw_hdmi_phy_disable, .read_hpd = dw_hdmi_phy_read_hpd, }; static const struct dw_hdmi_phy_ops rk3328_dw_hdmi_phy_ops = { .init = inno_dw_hdmi_phy_init, .disable = inno_dw_hdmi_phy_disable, .read_hpd = rk3328_inno_phy_read_hpd, }; > + if (status == connector_status_connected) > + regmap_write(hdmi->regmap, > + RK3328_GRF_SOC_CON4, > + RK3328_IO_5V_DOMAIN); > + else > + regmap_write(hdmi->regmap, > + RK3328_GRF_SOC_CON4, > + RK3328_IO_3V_DOMAIN); That should definitely get a comment explaining what this is doing and why :-) . Especially as it seems related to the plug/unplug events. According to the TRM, the hdmiphy IP block has its own interrupt. Can you tell me if it is firing on hotplug events? Because I'm wondering if these GRF settings wouldn't be better live in the hdmiphy driver [if it gets an irq on plug/unplug], which in turn would make the phy handling in dw_hdmi a lot easier. > + return status; > +} > + [...] > @@ -361,7 +453,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master, > return -ENOMEM; > > match = of_match_node(dw_hdmi_rockchip_dt_ids, pdev->dev.of_node); > - plat_data = match->data; > + plat_data = (struct dw_hdmi_plat_data *)match->data; > hdmi->dev = &pdev->dev; > hdmi->chip_data = plat_data->phy_data; > hdmi->dev_type = plat_data->dev_type; > @@ -383,6 +475,18 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master, > return ret; > } > > + if (hdmi->dev_type == RK3328_HDMI || hdmi->dev_type == RK3228_HDMI) { > + hdmi->phy = devm_phy_get(dev, "hdmi_phy"); > + if (IS_ERR(hdmi->phy)) { > + ret = PTR_ERR(hdmi->phy); > + dev_err(dev, "failed to get phy: %d\n", ret); > + return ret; > + } > + plat_data->phy_data = hdmi; > + ret = inno_dw_hdmi_init(hdmi); > + if (ret < 0) > + return ret; > + } You could also just declare it optional in the binding, try to get the phy via devm_phy_optional_get and if it's NULL just assume that it's the regular internal phy as with previous socs. That way you can drop the dev-type-check. Aka, if a phy is defined we are pretty sure we want to use that one :-) . Heiko