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=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,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 A5718C43381 for ; Tue, 19 Mar 2019 12:48:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6EA612146E for ; Tue, 19 Mar 2019 12:48:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727270AbfCSMsV convert rfc822-to-8bit (ORCPT ); Tue, 19 Mar 2019 08:48:21 -0400 Received: from unicorn.mansr.com ([81.2.72.234]:38298 "EHLO unicorn.mansr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726067AbfCSMsV (ORCPT ); Tue, 19 Mar 2019 08:48:21 -0400 Received: by unicorn.mansr.com (Postfix, from userid 51770) id 0726914CEB; Tue, 19 Mar 2019 12:48:19 +0000 (GMT) From: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= To: Maxime Ripard Cc: David Airlie , Chen-Yu Tsai , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property References: <20190311134713.25876-1-mans@mansr.com> <20190311154702.eslw5ccol44vxcmy@flea> <20190314154105.o6r7hzeuiyajxh7v@flea> <20190318155013.lge2x2cp5hvyz52f@flea> <20190319123445.w6q7vzr3qtgkejwj@flea> Date: Tue, 19 Mar 2019 12:48:19 +0000 In-Reply-To: <20190319123445.w6q7vzr3qtgkejwj@flea> (Maxime Ripard's message of "Tue, 19 Mar 2019 13:34:45 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Maxime Ripard writes: > On Mon, Mar 18, 2019 at 04:23:56PM +0000, Måns Rullgård wrote: >> Maxime Ripard writes: >> >> > On Thu, Mar 14, 2019 at 04:09:13PM +0000, Måns Rullgård wrote: >> >> Maxime Ripard writes: >> >> >> >> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, Måns Rullgård wrote: >> >> >> Maxime Ripard writes: >> >> >> >> >> >> > Hi! >> >> >> > >> >> >> > On Mon, Mar 11, 2019 at 01:47:13PM +0000, Mans Rullgard wrote: >> >> >> >> Sometimes it is desirabled to use a separate i2c controller for ddc >> >> >> >> access. This adds support for the ddc-i2c-bus property of the >> >> >> >> hdmi-connector node, using the specified controller if provided. >> >> >> >> >> >> >> >> Signed-off-by: Mans Rullgard >> >> >> >> --- >> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 + >> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 +++++++++++++++++++++++--- >> >> >> >> 2 files changed, 35 insertions(+), 3 deletions(-) >> >> >> >> >> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> >> >> >> index b685ee11623d..b08c4453d47c 100644 >> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> >> >> >> @@ -269,6 +269,7 @@ struct sun4i_hdmi { >> >> >> >> struct clk *tmds_clk; >> >> >> >> >> >> >> >> struct i2c_adapter *i2c; >> >> >> >> + struct i2c_adapter *ddc_i2c; >> >> >> >> >> >> >> >> /* Regmap fields for I2C adapter */ >> >> >> >> struct regmap_field *field_ddc_en; >> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c >> >> >> >> index 061d2e0d9011..5b2fac79f5d6 100644 >> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c >> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c >> >> >> >> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector) >> >> >> >> struct edid *edid; >> >> >> >> int ret; >> >> >> >> >> >> >> >> - edid = drm_get_edid(connector, hdmi->i2c); >> >> >> >> + edid = drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c); >> >> >> > >> >> >> > You can't test whether ddc_i2c is NULL or not... >> >> >> > >> >> >> >> if (!edid) >> >> >> >> return 0; >> >> >> >> >> >> >> >> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm_connector *connector) >> >> >> >> return ret; >> >> >> >> } >> >> >> >> >> >> >> >> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *dev) >> >> >> >> +{ >> >> >> >> + struct device_node *phandle, *remote; >> >> >> >> + struct i2c_adapter *ddc; >> >> >> >> + >> >> >> >> + remote = of_graph_get_remote_node(dev->of_node, 1, -1); >> >> >> >> + if (!remote) >> >> >> >> + return ERR_PTR(-EINVAL); >> >> >> >> + >> >> >> >> + phandle = of_parse_phandle(remote, "ddc-i2c-bus", 0); >> >> >> >> + of_node_put(remote); >> >> >> >> + if (!phandle) >> >> >> >> + return NULL; >> >> >> >> + >> >> >> >> + ddc = of_get_i2c_adapter_by_node(phandle); >> >> >> >> + of_node_put(phandle); >> >> >> >> + if (!ddc) >> >> >> >> + return ERR_PTR(-EPROBE_DEFER); >> >> >> >> + >> >> >> >> + return ddc; >> >> >> > >> >> >> > ... Since even in (most) error cases you're returning a !NULL pointer. >> >> >> > >> >> >> >> +} >> >> >> >> + >> >> >> >> static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = { >> >> >> >> .get_modes = sun4i_hdmi_get_modes, >> >> >> >> }; >> >> >> >> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *dev, struct device *master, >> >> >> >> goto err_disable_mod_clk; >> >> >> >> } >> >> >> >> >> >> >> >> + hdmi->ddc_i2c = sun4i_hdmi_get_ddc(dev); >> >> >> >> + if (IS_ERR(hdmi->ddc_i2c)) { >> >> >> >> >> >> ... which is checked here. >> >> >> >> >> >> The property is optional, so the idea was to return null in that case >> >> >> and use the built-in controller. If the property exists but some error >> >> >> occurs, we want to abort rather than proceed with the fallback which >> >> >> almost certainly won't work. >> >> >> >> >> >> Maybe I got something wrong in that logic. >> >> > >> >> > Indeed, I just got confused. I guess returning ENODEV in such a case, >> >> > and testing for that, would make things more obvious. >> >> >> >> There's also a case I hadn't thought of: property exists but isn't a >> >> valid phandle. What do you think is the correct action in that case? >> > >> > I think we would have that one covered. of_parse_phandle will return >> > !NULL, but then of_get_i2c_adapter_by_node will return NULL since we >> > wouldn't have an associated i2c adapter to the bogus phandle, and you >> > are checking for that already. >> >> of_parse_phandle() doesn't differentiate between a missing property and >> an existing non-phandle value. The following cases are possible with >> this patch: >> >> - ddc-i2c-bus points to valid i2c controller node: use this for ddc >> - no ddc-i2c-bus property: return NULL, use internal i2c >> - ddc-i2c-bus exists but isn't a phandle: likewise >> - ddc-i2c-bus points to a non-i2c-controller node: EPROBE_DEFER >> >> The last two cases obviously mean the devicetree is invalid, so perhaps >> it doesn't matter much what happens then. I don't think it's possible >> to distinguish between a well-formed phandle pointing to some bogus node >> and a good one where the i2c driver hasn't been probed yet. > > Ah, I see what you mean now. Yeah, there's not much we can do against > a wrong / corrupted DT. The DT validation would help prevent the third > case, and possibly the fourth, but that's basically the only thing we > can do. We need to return -EPROBE_DEFER in the case that everything is fine but the i2c driver hasn't been probed yet. From here, that is indistinguishable from of_parse_phandle() returning a completely bogus node. If the ddc-i2c-bus property doesn't contain a phandle at all, we could either return an error or fall back to the internal i2c. The patch does the latter because that's less code. I don't think that's any worse than aborting entirely in terms of user experience. The choice is up to you. I'm happy to update the patch, but you'll have to tell me what behaviour you prefer. -- Måns Rullgård 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=-7.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,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 C5B1CC43381 for ; Tue, 19 Mar 2019 12:48:30 +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 9808A20685 for ; Tue, 19 Mar 2019 12:48:30 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="jYsexWiE" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9808A20685 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=mansr.com 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:Message-ID:In-Reply-To: Date:References: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=+goVxgpUUeR+midF8/y1eqylN6ukvWPjg1lMrPG2lv8=; b=jYsexWiEySwPer ZM/yvkoPd1hHBatkyUHHpStLka/IuG4JoQCfWS1sPLWKBBGEMUsfamanElBShWqBtx//JKW6uOiYl SEL7shITc6z2yh8/eV2QFBqYHzUyB0OjS3Thh3N+ouPCeWGm+adlyiEK5OyPNo9grBgH/0MB04TXJ c7Bszapidx8jK3QMUFG35O3JQNhvWNiKCy494csJV6oaJ7W/eh3DfTkzKRtOmtyfzMC/98lF9Xw85 xCwAAjbxM9DF6v9DvfT5Fo/N/ge0md9Sz89qCB08Cp0Qde9leB88o3lSHgR6z301IXc6EDt+iQAj+ 372rBwhbnGvHLPcwzXkA==; 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 1h6EAM-0000vP-NG; Tue, 19 Mar 2019 12:48:26 +0000 Received: from unicorn.mansr.com ([81.2.72.234]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h6EAJ-0000uL-6d for linux-arm-kernel@lists.infradead.org; Tue, 19 Mar 2019 12:48:24 +0000 Received: by unicorn.mansr.com (Postfix, from userid 51770) id 0726914CEB; Tue, 19 Mar 2019 12:48:19 +0000 (GMT) From: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= To: Maxime Ripard Subject: Re: [PATCH] drm/sun4i: hdmi: add support for ddc-i2c-bus property References: <20190311134713.25876-1-mans@mansr.com> <20190311154702.eslw5ccol44vxcmy@flea> <20190314154105.o6r7hzeuiyajxh7v@flea> <20190318155013.lge2x2cp5hvyz52f@flea> <20190319123445.w6q7vzr3qtgkejwj@flea> Date: Tue, 19 Mar 2019 12:48:19 +0000 In-Reply-To: <20190319123445.w6q7vzr3qtgkejwj@flea> (Maxime Ripard's message of "Tue, 19 Mar 2019 13:34:45 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190319_054823_560465_0B95A70D X-CRM114-Status: GOOD ( 26.48 ) 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: David Airlie , Chen-Yu Tsai , linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.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 Maxime Ripard writes: > On Mon, Mar 18, 2019 at 04:23:56PM +0000, M=E5ns Rullg=E5rd wrote: >> Maxime Ripard writes: >> >> > On Thu, Mar 14, 2019 at 04:09:13PM +0000, M=E5ns Rullg=E5rd wrote: >> >> Maxime Ripard writes: >> >> >> >> > On Mon, Mar 11, 2019 at 04:11:06PM +0000, M=E5ns Rullg=E5rd wrote: >> >> >> Maxime Ripard writes: >> >> >> >> >> >> > Hi! >> >> >> > >> >> >> > On Mon, Mar 11, 2019 at 01:47:13PM +0000, Mans Rullgard wrote: >> >> >> >> Sometimes it is desirabled to use a separate i2c controller for= ddc >> >> >> >> access. This adds support for the ddc-i2c-bus property of the >> >> >> >> hdmi-connector node, using the specified controller if provided. >> >> >> >> >> >> >> >> Signed-off-by: Mans Rullgard >> >> >> >> --- >> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 1 + >> >> >> >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 37 ++++++++++++++++++= +++++--- >> >> >> >> 2 files changed, 35 insertions(+), 3 deletions(-) >> >> >> >> >> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/d= rm/sun4i/sun4i_hdmi.h >> >> >> >> index b685ee11623d..b08c4453d47c 100644 >> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> >> >> >> @@ -269,6 +269,7 @@ struct sun4i_hdmi { >> >> >> >> struct clk *tmds_clk; >> >> >> >> >> >> >> >> struct i2c_adapter *i2c; >> >> >> >> + struct i2c_adapter *ddc_i2c; >> >> >> >> >> >> >> >> /* Regmap fields for I2C adapter */ >> >> >> >> struct regmap_field *field_ddc_en; >> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/g= pu/drm/sun4i/sun4i_hdmi_enc.c >> >> >> >> index 061d2e0d9011..5b2fac79f5d6 100644 >> >> >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c >> >> >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c >> >> >> >> @@ -212,7 +212,7 @@ static int sun4i_hdmi_get_modes(struct drm_= connector *connector) >> >> >> >> struct edid *edid; >> >> >> >> int ret; >> >> >> >> >> >> >> >> - edid =3D drm_get_edid(connector, hdmi->i2c); >> >> >> >> + edid =3D drm_get_edid(connector, hdmi->ddc_i2c ?: hdmi->i2c); >> >> >> > >> >> >> > You can't test whether ddc_i2c is NULL or not... >> >> >> > >> >> >> >> if (!edid) >> >> >> >> return 0; >> >> >> >> >> >> >> >> @@ -228,6 +228,28 @@ static int sun4i_hdmi_get_modes(struct drm= _connector *connector) >> >> >> >> return ret; >> >> >> >> } >> >> >> >> >> >> >> >> +static struct i2c_adapter *sun4i_hdmi_get_ddc(struct device *d= ev) >> >> >> >> +{ >> >> >> >> + struct device_node *phandle, *remote; >> >> >> >> + struct i2c_adapter *ddc; >> >> >> >> + >> >> >> >> + remote =3D of_graph_get_remote_node(dev->of_node, 1, -1); >> >> >> >> + if (!remote) >> >> >> >> + return ERR_PTR(-EINVAL); >> >> >> >> + >> >> >> >> + phandle =3D of_parse_phandle(remote, "ddc-i2c-bus", 0); >> >> >> >> + of_node_put(remote); >> >> >> >> + if (!phandle) >> >> >> >> + return NULL; >> >> >> >> + >> >> >> >> + ddc =3D of_get_i2c_adapter_by_node(phandle); >> >> >> >> + of_node_put(phandle); >> >> >> >> + if (!ddc) >> >> >> >> + return ERR_PTR(-EPROBE_DEFER); >> >> >> >> + >> >> >> >> + return ddc; >> >> >> > >> >> >> > ... Since even in (most) error cases you're returning a !NULL po= inter. >> >> >> > >> >> >> >> +} >> >> >> >> + >> >> >> >> static const struct drm_connector_helper_funcs sun4i_hdmi_conn= ector_helper_funcs =3D { >> >> >> >> .get_modes =3D sun4i_hdmi_get_modes, >> >> >> >> }; >> >> >> >> @@ -575,6 +597,12 @@ static int sun4i_hdmi_bind(struct device *= dev, struct device *master, >> >> >> >> goto err_disable_mod_clk; >> >> >> >> } >> >> >> >> >> >> >> >> + hdmi->ddc_i2c =3D sun4i_hdmi_get_ddc(dev); >> >> >> >> + if (IS_ERR(hdmi->ddc_i2c)) { >> >> >> >> >> >> ... which is checked here. >> >> >> >> >> >> The property is optional, so the idea was to return null in that c= ase >> >> >> and use the built-in controller. If the property exists but some = error >> >> >> occurs, we want to abort rather than proceed with the fallback whi= ch >> >> >> almost certainly won't work. >> >> >> >> >> >> Maybe I got something wrong in that logic. >> >> > >> >> > Indeed, I just got confused. I guess returning ENODEV in such a cas= e, >> >> > and testing for that, would make things more obvious. >> >> >> >> There's also a case I hadn't thought of: property exists but isn't a >> >> valid phandle. What do you think is the correct action in that case? >> > >> > I think we would have that one covered. of_parse_phandle will return >> > !NULL, but then of_get_i2c_adapter_by_node will return NULL since we >> > wouldn't have an associated i2c adapter to the bogus phandle, and you >> > are checking for that already. >> >> of_parse_phandle() doesn't differentiate between a missing property and >> an existing non-phandle value. The following cases are possible with >> this patch: >> >> - ddc-i2c-bus points to valid i2c controller node: use this for ddc >> - no ddc-i2c-bus property: return NULL, use internal i2c >> - ddc-i2c-bus exists but isn't a phandle: likewise >> - ddc-i2c-bus points to a non-i2c-controller node: EPROBE_DEFER >> >> The last two cases obviously mean the devicetree is invalid, so perhaps >> it doesn't matter much what happens then. I don't think it's possible >> to distinguish between a well-formed phandle pointing to some bogus node >> and a good one where the i2c driver hasn't been probed yet. > > Ah, I see what you mean now. Yeah, there's not much we can do against > a wrong / corrupted DT. The DT validation would help prevent the third > case, and possibly the fourth, but that's basically the only thing we > can do. We need to return -EPROBE_DEFER in the case that everything is fine but the i2c driver hasn't been probed yet. From here, that is indistinguishable from of_parse_phandle() returning a completely bogus node. If the ddc-i2c-bus property doesn't contain a phandle at all, we could either return an error or fall back to the internal i2c. The patch does the latter because that's less code. I don't think that's any worse than aborting entirely in terms of user experience. The choice is up to you. I'm happy to update the patch, but you'll have to tell me what behaviour you prefer. -- = M=E5ns Rullg=E5rd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel