From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 211C5C4360F for ; Tue, 19 Mar 2019 11:44:27 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E231220857 for ; Tue, 19 Mar 2019 11:44:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="C9xjXnph" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E231220857 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=sntech.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=3IgKQeuqWBo3smJ14/QoFnyN3a188wi8FXRT6JWZmAU=; b=C9xjXnph0iEWNb r7fFiFqqUJR3hWPLBCcJpMWV/PhuqLQPiwV7bEcdKNgrkSxivJIl8zlxvyqX3Zk+1fxruFk/ARbsL EILY/rvykLcV0MOh6npsEupcPaFuM3zi91hu2FXJyXydHe2w3EOdBAXJaNf81zFlSlbmUMixGGGjM dZkDBiyXRF0kTa1TT1XCUxb3YhBR1Sxl2f1Bf9oUhufv7pMGU0i5RPXkAZwlC+kin4wHU6y2XN/9g QKFjD7j5QcR+hfilc0PJHWQBnkFUnCicIjXdYlVQiNmsEw2/mobQJ+oyGyJbkEuCc4+omNAAHT3nw KODjl+1tCK5op8L186Hg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h6DAK-0001hh-85; Tue, 19 Mar 2019 11:44:20 +0000 Received: from gloria.sntech.de ([185.11.138.130]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h6DAG-0001hH-S7; Tue, 19 Mar 2019 11:44:18 +0000 Received: from ip5f5a6320.dynamic.kabel-deutschland.de ([95.90.99.32] helo=diego.localnet) by gloria.sntech.de with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1h6DA4-0006B0-Ec; Tue, 19 Mar 2019 12:44:04 +0100 From: Heiko =?ISO-8859-1?Q?St=FCbner?= To: Johan Jonker Subject: Re: [PATCH v4 1/4] drm: rockchip: introduce rk3066 hdmi Date: Tue, 19 Mar 2019 12:44:03 +0100 Message-ID: <4125149.ebfS1HlHkS@diego> In-Reply-To: <20190306224113.24853-2-jbx6244@gmail.com> References: <20190306224113.24853-1-jbx6244@gmail.com> <20190306224113.24853-2-jbx6244@gmail.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190319_044417_060576_015F984C X-CRM114-Status: GOOD ( 25.09 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, airlied@linux.ie, hjc@rock-chips.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, robh+dt@kernel.org, daniel@ffwll.ch, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Johan, Am Mittwoch, 6. M=E4rz 2019, 23:41:10 CET schrieb Johan Jonker: > From: Zheng Yang > = > The RK3066 HDMI TX serves as interface between a LCD Controller and > a HDMI bus. A HDMI TX consists of one HDMI transmitter controller and > one HDMI transmitter PHY. The interface has three (3) 8-bit data channels > which can be configured for a number of bus widths (8/10/12/16/20/24-bit) > and different video formats (RGB, YCbCr). > = > Features: > HDMI version 1.4a, HDCP revision 1.4 and > DVI version 1.0 compliant transmitter. > Supports DTV resolutions from 480i to 1080i/p HD. > Master I2C interface for a DDC connection. > HDMI TX supports multiple power save modes. > The HDMI TX input can switch between LCDC0 and LCDC1. > (Sound support is not included in this patch) > = > Signed-off-by: Zheng Yang > Signed-off-by: Johan Jonker Looks good in general, but there are some minor things to improve yet, please see below for specifics. [...] > +static void rk3066_hdmi_config_phy(struct rk3066_hdmi *hdmi) > +{ > + /* TMDS uses the same frequency as dclk. */ > + hdmi_writeb(hdmi, HDMI_DEEP_COLOR_MODE, 0x22); These magic values below are no fault of yours, but in any case this could use a comment that the semi-public documentation does not describe hdmi-registers at all, so we're stuck with these magic-values for now. > + if (hdmi->tmdsclk > 100000000) { > + rk3066_hdmi_phy_write(hdmi, 0x158, 0x0E); > + rk3066_hdmi_phy_write(hdmi, 0x15c, 0x00); > + rk3066_hdmi_phy_write(hdmi, 0x160, 0x60); > + rk3066_hdmi_phy_write(hdmi, 0x164, 0x00); > + rk3066_hdmi_phy_write(hdmi, 0x168, 0xDA); > + rk3066_hdmi_phy_write(hdmi, 0x16c, 0xA1); > + rk3066_hdmi_phy_write(hdmi, 0x170, 0x0e); > + rk3066_hdmi_phy_write(hdmi, 0x174, 0x22); > + rk3066_hdmi_phy_write(hdmi, 0x178, 0x00); > + } else if (hdmi->tmdsclk > 50000000) { > + rk3066_hdmi_phy_write(hdmi, 0x158, 0x06); > + rk3066_hdmi_phy_write(hdmi, 0x15c, 0x00); > + rk3066_hdmi_phy_write(hdmi, 0x160, 0x60); > + rk3066_hdmi_phy_write(hdmi, 0x164, 0x00); > + rk3066_hdmi_phy_write(hdmi, 0x168, 0xCA); > + rk3066_hdmi_phy_write(hdmi, 0x16c, 0xA3); > + rk3066_hdmi_phy_write(hdmi, 0x170, 0x0e); > + rk3066_hdmi_phy_write(hdmi, 0x174, 0x20); > + rk3066_hdmi_phy_write(hdmi, 0x178, 0x00); > + } else { > + rk3066_hdmi_phy_write(hdmi, 0x158, 0x02); > + rk3066_hdmi_phy_write(hdmi, 0x15c, 0x00); > + rk3066_hdmi_phy_write(hdmi, 0x160, 0x60); > + rk3066_hdmi_phy_write(hdmi, 0x164, 0x00); > + rk3066_hdmi_phy_write(hdmi, 0x168, 0xC2); > + rk3066_hdmi_phy_write(hdmi, 0x16c, 0xA2); > + rk3066_hdmi_phy_write(hdmi, 0x170, 0x0e); > + rk3066_hdmi_phy_write(hdmi, 0x174, 0x20); > + rk3066_hdmi_phy_write(hdmi, 0x178, 0x00); > + } > +} > +static void rk3066_hdmi_encoder_enable(struct drm_encoder *encoder) > +{ > + struct rk3066_hdmi *hdmi =3D to_rk3066_hdmi(encoder); > + int mux, val; > + > + mux =3D drm_of_encoder_active_endpoint_id(hdmi->dev->of_node, encoder); > + if (mux) > + val =3D BIT(30) | BIT(14); > + else > + val =3D BIT(30); > + > + regmap_write(hdmi->grf, 0x150, val); Please define constants for both BIT(14) and the 0x150 GRF register, which is GRF_SOC_CON0, so in the header #define GRF_SOC_CON0 0x150 #define HDMI_VIDEO_SEL BIT(14) and then do if (mux) val =3D (HDMI_VIDEO_SEL << 16) | HDMI_VIDEO_SEL; else val =3D HDMI_VIDEO_SEL << 16; regmap_write(hdmi->grf, GRF_SOC_CON0, val); > + > + dev_dbg(hdmi->dev, "hdmi encoder enable select: vop%s\n", > + (mux) ? "1" : "0"); > + > + rk3066_hdmi_setup(hdmi, &hdmi->previous_mode); > +} > + > +static const struct drm_display_mode edid_cea_modes[] =3D { > + /* 4 - 1280x720@60Hz 16:9 */ > + { DRM_MODE("1280x720", DRM_MODE_TYPE_DRIVER, 74250, 1280, 1390, > + 1430, 1650, 0, 720, 725, 730, 750, 0, > + DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC), > + .vrefresh =3D 60, .picture_aspect_ratio =3D HDMI_PICTURE_ASPECT_16_9,= }, > +}; please drop that, see below > +static int rk3066_hdmi_connector_get_modes(struct drm_connector *connect= or) > +{ > + struct rk3066_hdmi *hdmi =3D to_rk3066_hdmi(connector); > + struct drm_display_mode *mode =3D NULL; > + struct edid *edid; > + int ret =3D 0; > + > + if (!hdmi->ddc) > + return 0; > + > + hdmi->hdmi_data.sink_is_hdmi =3D false; > + > + edid =3D drm_get_edid(connector, hdmi->ddc); > + if (edid) { > + hdmi->hdmi_data.sink_is_hdmi =3D drm_detect_hdmi_monitor(edid); > + > + dev_info(hdmi->dev, "monitor type : %s : %dx%d cm\n", > + (hdmi->hdmi_data.sink_is_hdmi ? "HDMI" : "DVI"), > + edid->width_cm, edid->height_cm); > + > + drm_connector_update_edid_property(connector, edid); > + ret =3D drm_add_edid_modes(connector, edid); > + kfree(edid); > + } > + > + if ((ret =3D=3D 0) || (hdmi->hdmi_data.sink_is_hdmi =3D=3D false)) { > + hdmi->hdmi_data.sink_is_hdmi =3D false; > + > + mode =3D drm_mode_duplicate(hdmi->drm_dev, &edid_cea_modes[0]); > + if (!mode) > + return ret; > + mode->type |=3D DRM_MODE_TYPE_PREFERRED; > + drm_mode_probed_add(connector, mode); > + ret++; > + > + dev_info(hdmi->dev, "no CEA mode found, use default\n"); This is a hack from the vendor-kernel. If EDID reading fails, it is some sort of issue with the driver, so we shouldn't assume any specific mode at all. See the somewhat similar inno_hdmi driver for comparison. > + } > + > + return ret; > +} > + > +static int > +rk3066_hdmi_register(struct drm_device *drm, struct rk3066_hdmi *hdmi) > +{ > + struct drm_encoder *encoder =3D &hdmi->encoder; > + struct device *dev =3D hdmi->dev; > + > + encoder->possible_crtcs =3D > + drm_of_find_possible_crtcs(drm, dev->of_node); > + > + /* > + * If we failed to find the VOP(s) which this encoder is > + * supposed to be connected to, it's because the VOP has > + * not been registered yet. Defer probing, and hope that > + * the required VOP is added later. > + */ > + if (encoder->possible_crtcs =3D=3D 0) > + return -EPROBE_DEFER; > + > + dev_info(hdmi->dev, "VOP found\n"); unnecessary output which will clutter up the kernel log, please drop. > + > + drm_encoder_helper_add(encoder, &rk3066_hdmi_encoder_helper_funcs); > + drm_encoder_init(drm, encoder, &rk3066_hdmi_encoder_funcs, > + DRM_MODE_ENCODER_TMDS, NULL); > + > + hdmi->connector.polled =3D DRM_CONNECTOR_POLL_HPD; > + > + drm_connector_helper_add(&hdmi->connector, > + &rk3066_hdmi_connector_helper_funcs); > + drm_connector_init(drm, &hdmi->connector, > + &rk3066_hdmi_connector_funcs, > + DRM_MODE_CONNECTOR_HDMIA); > + > + drm_connector_attach_encoder(&hdmi->connector, encoder); > + > + return 0; > +} > + > +static irqreturn_t rk3066_hdmi_i2c_irq(struct rk3066_hdmi *hdmi, u8 stat) > +{ > + struct rk3066_hdmi_i2c *i2c =3D hdmi->i2c; > + > + if (!(stat & HDMI_INTR_EDID_MASK)) > + return IRQ_NONE; > + > + i2c->stat =3D stat; > + > + complete(&i2c->compl); > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t rk3066_hdmi_hardirq(int irq, void *dev_id) > +{ > + struct rk3066_hdmi *hdmi =3D dev_id; > + irqreturn_t ret =3D IRQ_NONE; > + u8 interrupt; > + > + if (rk3066_hdmi_get_power_mode(hdmi) =3D=3D HDMI_SYS_POWER_MODE_A) > + hdmi_writeb(hdmi, HDMI_SYS_CTRL, HDMI_SYS_POWER_MODE_B); > + > + interrupt =3D hdmi_readb(hdmi, HDMI_INTR_STATUS1); > + if (interrupt) > + hdmi_writeb(hdmi, HDMI_INTR_STATUS1, interrupt); > + > + if (hdmi->i2c) > + ret =3D rk3066_hdmi_i2c_irq(hdmi, interrupt); I think you don't really need that rk3066_hdmi_i2c_irq function above, this can just become if (interrupt & HDMI_INTR_EDID_MASK) { hdmi->i2c->stat =3D stat; complete(&hdmi->i2c->compl); } hdmi->i2c is set through rk3066_hdmi_i2c_adapter() which is always called and needs to be sucessful before registering the interrupt, so there is no need to check for hdmi->i2c here at all. > + > + if (interrupt & (HDMI_INTR_HOTPLUG | HDMI_INTR_MSENS)) > + ret =3D IRQ_WAKE_THREAD; > + > + return ret; > +} > + > +static struct i2c_adapter *rk3066_hdmi_i2c_adapter(struct rk3066_hdmi *h= dmi) > +{ > + struct i2c_adapter *adap; > + struct rk3066_hdmi_i2c *i2c; > + int ret; > + > + i2c =3D devm_kzalloc(hdmi->dev, sizeof(*i2c), GFP_KERNEL); > + if (!i2c) > + return ERR_PTR(-ENOMEM); > + > + mutex_init(&i2c->i2c_lock); > + init_completion(&i2c->compl); > + > + adap =3D &i2c->adap; > + adap->class =3D I2C_CLASS_DDC; > + adap->owner =3D THIS_MODULE; > + adap->dev.parent =3D hdmi->dev; > + adap->dev.of_node =3D hdmi->dev->of_node; > + adap->algo =3D &rk3066_hdmi_algorithm; > + strlcpy(adap->name, "RK3066 HDMI", sizeof(adap->name)); > + i2c_set_adapdata(adap, hdmi); > + > + ret =3D i2c_add_adapter(adap); > + if (ret) { > + dev_warn(hdmi->dev, "cannot add %s I2C adapter\n", adap->name); > + devm_kfree(hdmi->dev, i2c); > + return ERR_PTR(ret); > + } > + > + hdmi->i2c =3D i2c; > + > + dev_info(hdmi->dev, "registered %s I2C bus driver\n", adap->name); Please drop or make it a DRM_DEV_DEBUG. Also, please convert all the general dev_foobar calls to their DRM_* equivalents, so dev_warn becomes DRM_DEV_ERROR, dev_info DRM_DEV_INFO and so on. > + return adap; > +} > + > +static int rk3066_hdmi_bind(struct device *dev, struct device *master, > + void *data) > +{ > + struct platform_device *pdev =3D to_platform_device(dev); > + struct drm_device *drm =3D data; > + struct rk3066_hdmi *hdmi; > + struct resource *iores; > + int irq; > + int ret; > + > + hdmi =3D devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL); > + if (!hdmi) > + return -ENOMEM; > + > + hdmi->dev =3D dev; > + hdmi->drm_dev =3D drm; > + > + iores =3D platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!iores) > + return -ENXIO; > + > + hdmi->regs =3D devm_ioremap_resource(dev, iores); > + if (IS_ERR(hdmi->regs)) > + return PTR_ERR(hdmi->regs); > + > + irq =3D platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > + > + hdmi->hclk =3D devm_clk_get(dev, "hclk"); > + if (IS_ERR(hdmi->hclk)) { > + dev_err(dev, "unable to get HDMI hclk clock\n"); > + return PTR_ERR(hdmi->hclk); > + } > + > + ret =3D clk_prepare_enable(hdmi->hclk); > + if (ret) { > + dev_err(dev, "cannot enable HDMI hclk clock: %d\n", ret); > + return ret; > + } > + > + hdmi->grf =3D syscon_regmap_lookup_by_phandle(dev->of_node, > + "rockchip,grf"); > + if (IS_ERR(hdmi->grf)) { > + dev_err(dev, "unable to get rockchip,grf\n"); > + ret =3D PTR_ERR(hdmi->grf); > + goto err_disable_hclk; > + } > + > + /* internal hclk =3D hdmi_hclk / 25 */ > + hdmi_writeb(hdmi, HDMI_INTERNAL_CLK_DIVIDER, 25); > + > + hdmi->ddc =3D rk3066_hdmi_i2c_adapter(hdmi); > + if (IS_ERR(hdmi->ddc)) { > + ret =3D PTR_ERR(hdmi->ddc); > + hdmi->ddc =3D NULL; > + goto err_disable_hclk; > + } > + > + rk3066_hdmi_set_power_mode(hdmi, HDMI_SYS_POWER_MODE_B); > + usleep_range(999, 1000); > + hdmi_writeb(hdmi, HDMI_INTR_MASK1, HDMI_INTR_HOTPLUG); > + hdmi_writeb(hdmi, HDMI_INTR_MASK2, 0); > + hdmi_writeb(hdmi, HDMI_INTR_MASK3, 0); > + hdmi_writeb(hdmi, HDMI_INTR_MASK4, 0); > + rk3066_hdmi_set_power_mode(hdmi, HDMI_SYS_POWER_MODE_A); > + > + ret =3D rk3066_hdmi_register(drm, hdmi); > + if (ret) > + goto err_disable_hclk; goto err_disable_i2c; So that the i2c-adapter also gets disabled on error. > + dev_set_drvdata(dev, hdmi); > + > + ret =3D devm_request_threaded_irq(dev, irq, rk3066_hdmi_hardirq, > + rk3066_hdmi_irq, IRQF_SHARED, > + dev_name(dev), hdmi); > + if (ret) { > + dev_err(dev, "failed to request hdmi irq: %d\n", ret); > + goto err_disable_hclk; goto err_disable_i2c; > + } > + > + return 0; > + err_disable_i2c: i2c_put_adapter(hdmi->ddc); > +err_disable_hclk: > + clk_disable_unprepare(hdmi->hclk); > + > + return ret; > +} > + > +static void rk3066_hdmi_unbind(struct device *dev, struct device *master, > + void *data) > +{ > + struct rk3066_hdmi *hdmi =3D dev_get_drvdata(dev); > + > + hdmi->connector.funcs->destroy(&hdmi->connector); > + hdmi->encoder.funcs->destroy(&hdmi->encoder); > + > + clk_disable_unprepare(hdmi->hclk); > + i2c_put_adapter(hdmi->ddc); you should probably invert the calling order here, first remove the i2c adapter and only then disable the clock. Thanks Heiko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel