From: Pin-yen Lin <treapking@chromium.org> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: "Andrzej Hajda" <andrzej.hajda@intel.com>, "Neil Armstrong" <neil.armstrong@linaro.org>, "Robert Foss" <robert.foss@linaro.org>, "Laurent Pinchart" <Laurent.pinchart@ideasonboard.com>, "Jonas Karlman" <jonas@kwiboo.se>, "Jernej Skrabec" <jernej.skrabec@gmail.com>, "David Airlie" <airlied@gmail.com>, "Daniel Vetter" <daniel@ffwll.ch>, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Daniel Scally" <djrscally@gmail.com>, "Heikki Krogerus" <heikki.krogerus@linux.intel.com>, "Sakari Ailus" <sakari.ailus@linux.intel.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Rafael J . Wysocki" <rafael@kernel.org>, "Prashant Malani" <pmalani@chromium.org>, "Benson Leung" <bleung@chromium.org>, "Guenter Roeck" <groeck@chromium.org>, "Javier Martinez Canillas" <javierm@redhat.com>, "Stephen Boyd" <swboyd@chromium.org>, dri-devel@lists.freedesktop.org, "Hsin-Yi Wang" <hsinyi@chromium.org>, "Thomas Zimmermann" <tzimmermann@suse.de>, devicetree@vger.kernel.org, chrome-platform@lists.linux.dev, linux-acpi@vger.kernel.org, "Marek Vasut" <marex@denx.de>, "Xin Ji" <xji@analogixsemi.com>, "Lyude Paul" <lyude@redhat.com>, "Nícolas F . R . A . Prado" <nfraprado@collabora.com>, "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>, linux-kernel@vger.kernel.org, "Allen Chen" <allen.chen@ite.com.tw> Subject: Re: [PATCH v6 7/7] drm/bridge: it6505: Register Type C mode switches Date: Fri, 25 Nov 2022 14:55:11 +0800 [thread overview] Message-ID: <CAEXTbpfU1EBD7QYZLeRFk9Kz7+J1wamzaVuwVpa8M9WxWtCe-g@mail.gmail.com> (raw) In-Reply-To: <Y39iRg2TZCljOyNN@smile.fi.intel.com> Hi Andy, Thanks for reviewing the patch. On Thu, Nov 24, 2022 at 8:23 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > 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 <drm/drm_edid.h> > > #include <drm/drm_print.h> > > #include <drm/drm_probe_helper.h> > > +#include <drm/drm_of.h> > > Make it ordered? Will fix it in v7. > > ... > > > +struct it6505_port_data { > > > + bool dp_connected; > > Perhaps make it last? Will fix it in v7. > > > + 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. Will update it in v7. > > Also, comment out these long sleeps. Why they are needed. Actually, it turns out that these sleeps are not needed. I'll remove it in v7. > > ... > > > + 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(). Will fix this in v7. > > > + * 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. pm_runtime_force_suspend() disables runtime PM when the device enters suspend, and sometime it6505_typec_mux_set() is called before pm_runtime_force_resume brings runtime PM back. We force power up the bridge in this case and leave the ref count incremented to make the future pm_runtime_(get|put)_sync() calls balanced. I'll include more explanations around this in v7. > > ... > > > + 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. I'll remove this part in v7 and try to figure out how to do similar checking with schemas. > > > + 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? Could you advise where this lib file should go, and what the namings can be? The "port-switching" logic is specific to some of the DP bridges, and I'm not sure what kinds of naming/structure fit into this case. Thanks and regards, Pin-yen > > -- > With Best Regards, > Andy Shevchenko > >
WARNING: multiple messages have this Message-ID (diff)
From: Pin-yen Lin <treapking@chromium.org> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: "Heikki Krogerus" <heikki.krogerus@linux.intel.com>, "Andrzej Hajda" <andrzej.hajda@intel.com>, "Rafael J . Wysocki" <rafael@kernel.org>, dri-devel@lists.freedesktop.org, "Laurent Pinchart" <Laurent.pinchart@ideasonboard.com>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Guenter Roeck" <groeck@chromium.org>, "Marek Vasut" <marex@denx.de>, chrome-platform@lists.linux.dev, "Javier Martinez Canillas" <javierm@redhat.com>, "Jernej Skrabec" <jernej.skrabec@gmail.com>, linux-acpi@vger.kernel.org, devicetree@vger.kernel.org, "Sakari Ailus" <sakari.ailus@linux.intel.com>, "Nícolas F . R . A . Prado" <nfraprado@collabora.com>, "Jonas Karlman" <jonas@kwiboo.se>, "Allen Chen" <allen.chen@ite.com.tw>, "Stephen Boyd" <swboyd@chromium.org>, "Rob Herring" <robh+dt@kernel.org>, "Hsin-Yi Wang" <hsinyi@chromium.org>, "Xin Ji" <xji@analogixsemi.com>, "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>, "Neil Armstrong" <neil.armstrong@linaro.org>, "Thomas Zimmermann" <tzimmermann@suse.de>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, linux-kernel@vger.kernel.org, "Robert Foss" <robert.foss@linaro.org>, "Daniel Scally" <djrscally@gmail.com>, "Prashant Malani" <pmalani@chromium.org> Subject: Re: [PATCH v6 7/7] drm/bridge: it6505: Register Type C mode switches Date: Fri, 25 Nov 2022 14:55:11 +0800 [thread overview] Message-ID: <CAEXTbpfU1EBD7QYZLeRFk9Kz7+J1wamzaVuwVpa8M9WxWtCe-g@mail.gmail.com> (raw) In-Reply-To: <Y39iRg2TZCljOyNN@smile.fi.intel.com> Hi Andy, Thanks for reviewing the patch. On Thu, Nov 24, 2022 at 8:23 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > 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 <drm/drm_edid.h> > > #include <drm/drm_print.h> > > #include <drm/drm_probe_helper.h> > > +#include <drm/drm_of.h> > > Make it ordered? Will fix it in v7. > > ... > > > +struct it6505_port_data { > > > + bool dp_connected; > > Perhaps make it last? Will fix it in v7. > > > + 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. Will update it in v7. > > Also, comment out these long sleeps. Why they are needed. Actually, it turns out that these sleeps are not needed. I'll remove it in v7. > > ... > > > + 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(). Will fix this in v7. > > > + * 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. pm_runtime_force_suspend() disables runtime PM when the device enters suspend, and sometime it6505_typec_mux_set() is called before pm_runtime_force_resume brings runtime PM back. We force power up the bridge in this case and leave the ref count incremented to make the future pm_runtime_(get|put)_sync() calls balanced. I'll include more explanations around this in v7. > > ... > > > + 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. I'll remove this part in v7 and try to figure out how to do similar checking with schemas. > > > + 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? Could you advise where this lib file should go, and what the namings can be? The "port-switching" logic is specific to some of the DP bridges, and I'm not sure what kinds of naming/structure fit into this case. Thanks and regards, Pin-yen > > -- > With Best Regards, > Andy Shevchenko > >
next prev parent reply other threads:[~2022-11-25 6:55 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-11-24 10:20 [PATCH v6 0/7] Register Type-C mode-switch in DP bridge endpoints Pin-yen Lin 2022-11-24 10:20 ` Pin-yen Lin 2022-11-24 10:20 ` [PATCH v6 1/7] device property: Add remote endpoint to devcon matcher Pin-yen Lin 2022-11-24 10:20 ` Pin-yen Lin 2022-11-24 10:20 ` [PATCH v6 2/7] platform/chrome: cros_ec_typec: Purge blocking switch devlinks Pin-yen Lin 2022-11-24 10:20 ` Pin-yen Lin 2022-11-24 12:24 ` Andy Shevchenko 2022-11-24 12:24 ` Andy Shevchenko 2022-11-25 5:52 ` Pin-yen Lin 2022-11-25 5:52 ` Pin-yen Lin 2022-11-25 5:53 ` Prashant Malani 2022-11-25 5:53 ` Prashant Malani 2022-11-24 10:20 ` [PATCH v6 3/7] dt-bindings: drm/bridge: anx7625: Add mode-switch support Pin-yen Lin 2022-11-24 10:20 ` Pin-yen Lin 2022-11-24 17:39 ` Rob Herring 2022-11-24 17:39 ` Rob Herring 2022-11-25 3:58 ` Pin-yen Lin 2022-11-25 3:58 ` Pin-yen Lin 2022-11-27 20:58 ` Krzysztof Kozlowski 2022-11-27 20:58 ` Krzysztof Kozlowski 2022-12-26 8:49 ` Pin-yen Lin 2022-12-26 8:49 ` Pin-yen Lin 2022-11-24 10:20 ` [PATCH v6 4/7] drm/bridge: anx7625: Check for Type-C during panel registration Pin-yen Lin 2022-11-24 10:20 ` Pin-yen Lin 2022-11-24 10:20 ` [PATCH v6 5/7] drm/bridge: anx7625: Register Type C mode switches Pin-yen Lin 2022-11-24 10:20 ` Pin-yen Lin 2022-11-24 12:18 ` Andy Shevchenko 2022-11-24 12:18 ` Andy Shevchenko 2022-11-25 6:58 ` Pin-yen Lin 2022-11-25 6:58 ` Pin-yen Lin 2022-11-24 10:20 ` [PATCH v6 6/7] dt/bindings: drm/bridge: it6505: Add mode-switch support Pin-yen Lin 2022-11-24 10:20 ` Pin-yen Lin 2022-11-24 17:39 ` Rob Herring 2022-11-24 17:39 ` Rob Herring 2022-11-25 3:58 ` Pin-yen Lin 2022-11-25 3:58 ` Pin-yen Lin 2022-11-27 21:02 ` Krzysztof Kozlowski 2022-11-27 21:02 ` Krzysztof Kozlowski 2022-12-26 8:57 ` Pin-yen Lin 2022-12-26 8:57 ` Pin-yen Lin 2022-11-24 10:20 ` [PATCH v6 7/7] drm/bridge: it6505: Register Type C mode switches Pin-yen Lin 2022-11-24 10:20 ` Pin-yen Lin 2022-11-24 12:23 ` Andy Shevchenko 2022-11-24 12:23 ` Andy Shevchenko 2022-11-25 6:55 ` Pin-yen Lin [this message] 2022-11-25 6:55 ` Pin-yen Lin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAEXTbpfU1EBD7QYZLeRFk9Kz7+J1wamzaVuwVpa8M9WxWtCe-g@mail.gmail.com \ --to=treapking@chromium.org \ --cc=Laurent.pinchart@ideasonboard.com \ --cc=airlied@gmail.com \ --cc=allen.chen@ite.com.tw \ --cc=andriy.shevchenko@linux.intel.com \ --cc=andrzej.hajda@intel.com \ --cc=angelogioacchino.delregno@collabora.com \ --cc=bleung@chromium.org \ --cc=chrome-platform@lists.linux.dev \ --cc=daniel@ffwll.ch \ --cc=devicetree@vger.kernel.org \ --cc=djrscally@gmail.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=gregkh@linuxfoundation.org \ --cc=groeck@chromium.org \ --cc=heikki.krogerus@linux.intel.com \ --cc=hsinyi@chromium.org \ --cc=javierm@redhat.com \ --cc=jernej.skrabec@gmail.com \ --cc=jonas@kwiboo.se \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lyude@redhat.com \ --cc=marex@denx.de \ --cc=neil.armstrong@linaro.org \ --cc=nfraprado@collabora.com \ --cc=pmalani@chromium.org \ --cc=rafael@kernel.org \ --cc=robert.foss@linaro.org \ --cc=robh+dt@kernel.org \ --cc=sakari.ailus@linux.intel.com \ --cc=swboyd@chromium.org \ --cc=tzimmermann@suse.de \ --cc=xji@analogixsemi.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.