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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 91FBFC433FE for ; Thu, 24 Nov 2022 12:23:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229463AbiKXMXs (ORCPT ); Thu, 24 Nov 2022 07:23:48 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37852 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229379AbiKXMXs (ORCPT ); Thu, 24 Nov 2022 07:23:48 -0500 Received: from mga06.intel.com (mga06b.intel.com [134.134.136.31]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 116D363149; Thu, 24 Nov 2022 04:23:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1669292627; x=1700828627; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=ObxTC5S376qcP+jc5Vqug+j/70Vb8bRa53QKgt/sNI0=; b=TAkd87MI3g9IIv6jKxilbUyRn7Xtf91ySoQVrSKBa76amUvI/djlpyzA Qp59Yye0ah75bHWg9ssGDdK3+8JSeEUzRFGK0YLvTvvePRiME0WX5fbL7 IlIBZTfO80x1cHOaoboFtTMDpQdBkHAwhKutyBk58JvqjJVJoqV4i5ag2 6f/342glZbcNdrdhVE4dDbaNnot7T0UaMja7bgJqoELxsJ5JVMN7irWlz 7wCE23nVKBxk6/mPyJgCuQobHXkkooTF1i6Ubp6LZczIZ/x5pZvk++dSn tl4XoIuPSXBJvB2tIMein+e1b7cEH8yqNrUoMlRKU+C27D+9w5rJ3VMQ7 Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10540"; a="376434111" X-IronPort-AV: E=Sophos;i="5.96,190,1665471600"; d="scan'208";a="376434111" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Nov 2022 04:23:46 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10540"; a="642336159" X-IronPort-AV: E=Sophos;i="5.96,190,1665471600"; d="scan'208";a="642336159" Received: from smile.fi.intel.com ([10.237.72.54]) by orsmga002.jf.intel.com with ESMTP; 24 Nov 2022 04:23:37 -0800 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1oyBGQ-00GjwQ-0y; Thu, 24 Nov 2022 14:23:34 +0200 Date: Thu, 24 Nov 2022 14:23:34 +0200 From: Andy Shevchenko To: Pin-yen Lin Cc: Andrzej Hajda , Neil Armstrong , Robert Foss , Laurent Pinchart , Jonas Karlman , Jernej Skrabec , David Airlie , Daniel Vetter , Rob Herring , Krzysztof Kozlowski , Daniel Scally , Heikki Krogerus , Sakari Ailus , Greg Kroah-Hartman , "Rafael J . Wysocki" , Prashant Malani , Benson Leung , Guenter Roeck , Javier Martinez Canillas , Stephen Boyd , dri-devel@lists.freedesktop.org, Hsin-Yi Wang , Thomas Zimmermann , devicetree@vger.kernel.org, chrome-platform@lists.linux.dev, linux-acpi@vger.kernel.org, Marek Vasut , Xin Ji , Lyude Paul , =?iso-8859-1?Q?N=EDcolas_F_=2E_R_=2E_A_=2E?= Prado , AngeloGioacchino Del Regno , linux-kernel@vger.kernel.org, Allen Chen Subject: Re: [PATCH v6 7/7] drm/bridge: it6505: Register Type C mode switches Message-ID: References: <20221124102056.393220-1-treapking@chromium.org> <20221124102056.393220-8-treapking@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221124102056.393220-8-treapking@chromium.org> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org On Thu, Nov 24, 2022 at 06:20:56PM +0800, Pin-yen Lin wrote: > Register USB Type-C mode switches when the "mode-switch" property and > relevant port are available in Device Tree. Configure the "lane_swap" > state based on the entered alternate mode for a specific Type-C > connector, which ends up updating the lane swap registers of the it6505 > chip. ... > config DRM_ITE_IT6505 > tristate "ITE IT6505 DisplayPort bridge" > depends on OF > + depends on TYPEC || TYPEC=n > select DRM_DISPLAY_DP_HELPER > select DRM_DISPLAY_HDCP_HELPER > select DRM_DISPLAY_HELPER Something went wrong with the indentation. Perhaps you need to fix it first. ... > #include > #include > #include > +#include Make it ordered? ... > +struct it6505_port_data { > + bool dp_connected; Perhaps make it last? > + struct typec_mux_dev *typec_mux; > + struct it6505 *it6505; > +}; ... > +static void it6505_typec_ports_update(struct it6505 *it6505) > +{ > + usleep_range(3000, 4000); > + > + if (it6505->typec_ports[0].dp_connected && it6505->typec_ports[1].dp_connected) > + /* Both ports available, do nothing to retain the current one. */ > + return; > + else if (it6505->typec_ports[0].dp_connected) > + it6505->lane_swap = false; > + else if (it6505->typec_ports[1].dp_connected) > + it6505->lane_swap = true; > + > + usleep_range(3000, 4000); > +} As per previous patch comments. Also, comment out these long sleeps. Why they are needed. ... > + int ret = pm_runtime_get_sync(dev); > + > + /* > + * On system resume, mux_set can be triggered before > + * pm_runtime_force_resume re-enables runtime power management. We refer to the functions as func(). > + * Handling the error here to make sure the bridge is powered on. > + */ > + if (ret < 0) > + it6505_poweron(it6505); This seems needed a bit more of explanation, esp. why you leave PM runtime reference count bumped up. ... > + num_lanes = drm_of_get_data_lanes_count(node, 0, 2); > + if (num_lanes <= 0) { > + dev_err(dev, "Error on getting data lanes count: %d\n", > + num_lanes); > + return num_lanes; > + } > + > + ret = of_property_read_u32_array(node, "data-lanes", dp_lanes, num_lanes); > + if (ret) { > + dev_err(dev, "Failed to read the data-lanes variable: %d\n", > + ret); > + return ret; > + } > + > + for (i = 0; i < num_lanes; i++) { > + if (port_num != -1 && port_num != dp_lanes[i] / 2) { > + dev_err(dev, "Invalid data lane numbers\n"); > + return -EINVAL; > + } As per previous patch comments. > + port_num = dp_lanes[i] / 2; > + } The above seems like tons of duplicating code that drivers need to implement. Can we shrink that burden by adding some library functions? -- With Best Regards, Andy Shevchenko