[v6,7/7] drm/bridge: it6505: Register Type C mode switches
Commit Message
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.
Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---
Changes in v6:
- Changed it6505_typec_mux_set callback function to accommodate with
the latest drm-misc patches
- Changed the driver implementation to accommodate with the new binding
- Squashed to a single patch
drivers/gpu/drm/bridge/Kconfig | 1 +
drivers/gpu/drm/bridge/ite-it6505.c | 205 +++++++++++++++++++++++++++-
2 files changed, 202 insertions(+), 4 deletions(-)
Comments
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?
...
> +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?
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
>
>
@@ -87,6 +87,7 @@ config DRM_FSL_LDB
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
@@ -17,6 +17,8 @@
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
#include <linux/types.h>
+#include <linux/usb/typec_dp.h>
+#include <linux/usb/typec_mux.h>
#include <linux/wait.h>
#include <crypto/hash.h>
@@ -30,6 +32,7 @@
#include <drm/drm_edid.h>
#include <drm/drm_print.h>
#include <drm/drm_probe_helper.h>
+#include <drm/drm_of.h>
#include <sound/hdmi-codec.h>
@@ -402,6 +405,12 @@ struct debugfs_entries {
const struct file_operations *fops;
};
+struct it6505_port_data {
+ bool dp_connected;
+ struct typec_mux_dev *typec_mux;
+ struct it6505 *it6505;
+};
+
struct it6505 {
struct drm_dp_aux aux;
struct drm_bridge bridge;
@@ -454,6 +463,8 @@ struct it6505 {
struct delayed_work delayed_audio;
struct it6505_audio_data audio;
struct dentry *debugfs;
+ int num_typec_switches;
+ struct it6505_port_data *typec_ports;
/* it6505 driver hold option */
bool enable_drv_hold;
@@ -3265,13 +3276,185 @@ static void it6505_shutdown(struct i2c_client *client)
it6505_lane_off(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);
+}
+
+static int it6505_typec_mux_set(struct typec_mux_dev *mux,
+ struct typec_mux_state *state)
+{
+ struct it6505_port_data *data = typec_mux_get_drvdata(mux);
+ struct it6505 *it6505 = data->it6505;
+ struct device *dev = &it6505->client->dev;
+ bool old_dp_connected, new_dp_connected;
+
+ if (it6505->num_typec_switches == 1)
+ return 0;
+
+ mutex_lock(&it6505->extcon_lock);
+
+ old_dp_connected = it6505->typec_ports[0].dp_connected ||
+ it6505->typec_ports[1].dp_connected;
+
+ data->dp_connected = (state->alt && state->alt->svid == USB_TYPEC_DP_SID &&
+ state->alt->mode == USB_TYPEC_DP_MODE);
+
+ dev_dbg(dev, "mux_set dp_connected: c0=%d, c1=%d\n",
+ it6505->typec_ports[0].dp_connected, it6505->typec_ports[1].dp_connected);
+
+ new_dp_connected = it6505->typec_ports[0].dp_connected ||
+ it6505->typec_ports[1].dp_connected;
+
+ if (it6505->enable_drv_hold) {
+ dev_dbg(dev, "enable driver hold\n");
+ goto unlock;
+ }
+
+ it6505_typec_ports_update(it6505);
+
+ if (!old_dp_connected && new_dp_connected) {
+ 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.
+ * Handling the error here to make sure the bridge is powered on.
+ */
+ if (ret < 0)
+ it6505_poweron(it6505);
+
+ complete_all(&it6505->extcon_completion);
+ }
+
+ if (old_dp_connected && !new_dp_connected) {
+ reinit_completion(&it6505->extcon_completion);
+ pm_runtime_put_sync(dev);
+ if (it6505->bridge.dev)
+ drm_helper_hpd_irq_event(it6505->bridge.dev);
+ memset(it6505->dpcd, 0, sizeof(it6505->dpcd));
+ }
+
+unlock:
+ mutex_unlock(&it6505->extcon_lock);
+ return 0;
+}
+
+static int it6505_register_mode_switch(struct device *dev, struct device_node *node,
+ struct it6505 *it6505)
+{
+ struct it6505_port_data *port_data;
+ struct typec_mux_desc mux_desc = {};
+ char name[32];
+ u32 dp_lanes[2];
+ int ret, num_lanes, i, port_num = -1;
+
+ 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;
+ }
+ port_num = dp_lanes[i] / 2;
+ }
+
+ port_data = &it6505->typec_ports[port_num];
+ port_data->it6505 = it6505;
+ mux_desc.fwnode = &node->fwnode;
+ mux_desc.drvdata = port_data;
+ snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
+ mux_desc.name = name;
+ mux_desc.set = it6505_typec_mux_set;
+
+ port_data->typec_mux = typec_mux_register(dev, &mux_desc);
+ if (IS_ERR(port_data->typec_mux)) {
+ ret = PTR_ERR(port_data->typec_mux);
+ dev_err(dev, "Mode switch register for port %d failed: %d\n",
+ port_num, ret);
+ }
+
+ return ret;
+}
+
+static void it6505_unregister_typec_switches(struct it6505 *it6505)
+{
+ int i;
+
+ for (i = 0; i < it6505->num_typec_switches; i++)
+ typec_mux_unregister(it6505->typec_ports[i].typec_mux);
+}
+
+static int it6505_register_typec_switches(struct device *dev, struct it6505 *it6505)
+{
+ struct device_node *port, *sw;
+ int ret = 0;
+
+ port = of_graph_get_port_by_id(dev->of_node, 1);
+
+ for_each_child_of_node(port, sw) {
+ if (of_property_read_bool(sw, "mode-switch"))
+ it6505->num_typec_switches++;
+ }
+
+ if (!it6505->num_typec_switches) {
+ dev_warn(dev, "No Type-C switches node found\n");
+ return ret;
+ }
+
+ it6505->typec_ports = devm_kzalloc(
+ dev, it6505->num_typec_switches * sizeof(struct it6505_port_data),
+ GFP_KERNEL);
+ if (!it6505->typec_ports)
+ return -ENOMEM;
+
+ /* Register switches for each connector. */
+ for_each_child_of_node(port, sw) {
+ if (!of_property_read_bool(sw, "mode-switch"))
+ continue;
+ ret = it6505_register_mode_switch(dev, sw, it6505);
+ if (ret) {
+ dev_err(dev, "Failed to register mode switch: %d\n", ret);
+ of_node_put(sw);
+ break;
+ }
+ }
+
+ if (ret)
+ it6505_unregister_typec_switches(it6505);
+
+ return ret;
+}
+
static int it6505_i2c_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
struct it6505 *it6505;
struct device *dev = &client->dev;
struct extcon_dev *extcon;
- int err, intp_irq;
+ int err, intp_irq, ret;
it6505 = devm_kzalloc(&client->dev, sizeof(*it6505), GFP_KERNEL);
if (!it6505)
@@ -3292,11 +3475,24 @@ static int it6505_i2c_probe(struct i2c_client *client,
if (PTR_ERR(extcon) == -EPROBE_DEFER)
return -EPROBE_DEFER;
if (IS_ERR(extcon)) {
- dev_err(dev, "can not get extcon device!");
- return PTR_ERR(extcon);
+ if (PTR_ERR(extcon) != -ENODEV)
+ dev_warn(dev, "Cannot get extcon device: %ld\n",
+ PTR_ERR(extcon));
+ it6505->extcon = NULL;
+ } else {
+ it6505->extcon = extcon;
}
- it6505->extcon = extcon;
+ ret = it6505_register_typec_switches(dev, it6505);
+ if (ret) {
+ if (ret != -ENODEV)
+ dev_warn(dev, "Didn't register Type-C switches, err: %d\n",
+ ret);
+ if (!it6505->extcon) {
+ dev_err(dev, "Both extcon and typec-switch are not registered.\n");
+ return -EINVAL;
+ }
+ }
it6505->regmap = devm_regmap_init_i2c(client, &it6505_regmap_config);
if (IS_ERR(it6505->regmap)) {
@@ -3367,6 +3563,7 @@ static void it6505_i2c_remove(struct i2c_client *client)
drm_dp_aux_unregister(&it6505->aux);
it6505_debugfs_remove(it6505);
it6505_poweroff(it6505);
+ it6505_unregister_typec_switches(it6505);
}
static const struct i2c_device_id it6505_id[] = {