[v10,3/9] drm/display: Add Type-C switch helpers

Add helpers to register and unregister Type-C "switches" for bridges
capable of switching their output between two downstream devices.

The helper registers USB Type-C mode switches when the "mode-switch"
and the "data-lanes" properties are available in Device Tree.

Signed-off-by: Pin-yen Lin <treapking@chromium.org>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

---

Changes in v10:
- Collected Reviewed-by and Tested-by tags
- Replaced "void *" with "typec_mux_set_fn_t" for mux_set callbacks
- Print out the node name when errors on parsing DT
- Use dev_dbg instead of dev_warn when no Type-C switch nodes available
- Made the return path of drm_dp_register_mode_switch clearer

Changes in v8:
- Fixed the build issue when CONFIG_TYPEC=m
- Fixed some style issues

Changes in v7:
- Extracted the common codes to a helper function
- New in v7

 drivers/gpu/drm/display/drm_dp_helper.c | 134 ++++++++++++++++++++++++
 include/drm/display/drm_dp_helper.h     |  17 +++
 2 files changed, 151 insertions(+)

Hi Dmitry,

Thanks for the review.

On Thu, Jan 12, 2023 at 12:40 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 12/01/2023 06:20, Pin-yen Lin wrote:
> > Add helpers to register and unregister Type-C "switches" for bridges
> > capable of switching their output between two downstream devices.
> >
> > The helper registers USB Type-C mode switches when the "mode-switch"
> > and the "data-lanes" properties are available in Device Tree.
> >
> > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >
> > ---
> >
> > Changes in v10:
> > - Collected Reviewed-by and Tested-by tags
> > - Replaced "void *" with "typec_mux_set_fn_t" for mux_set callbacks
> > - Print out the node name when errors on parsing DT
> > - Use dev_dbg instead of dev_warn when no Type-C switch nodes available
> > - Made the return path of drm_dp_register_mode_switch clearer
> >
> > Changes in v8:
> > - Fixed the build issue when CONFIG_TYPEC=m
> > - Fixed some style issues
> >
> > Changes in v7:
> > - Extracted the common codes to a helper function
> > - New in v7
> >
> >   drivers/gpu/drm/display/drm_dp_helper.c | 134 ++++++++++++++++++++++++
> >   include/drm/display/drm_dp_helper.h     |  17 +++
> >   2 files changed, 151 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> > index 16565a0a5da6..a2ec40a621cb 100644
> > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > @@ -30,11 +30,13 @@
> >   #include <linux/sched.h>
> >   #include <linux/seq_file.h>
> >   #include <linux/string_helpers.h>
> > +#include <linux/usb/typec_mux.h>
> >   #include <linux/dynamic_debug.h>
> >
> >   #include <drm/display/drm_dp_helper.h>
> >   #include <drm/display/drm_dp_mst_helper.h>
> >   #include <drm/drm_edid.h>
> > +#include <drm/drm_of.h>
> >   #include <drm/drm_print.h>
> >   #include <drm/drm_vblank.h>
> >   #include <drm/drm_panel.h>
> > @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
> >   EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
> >
> >   #endif
> > +
> > +#if IS_REACHABLE(CONFIG_TYPEC)
> > +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
> > +                                    struct drm_dp_typec_switch_desc *switch_desc,
> > +                                    void *data, typec_mux_set_fn_t mux_set)
> > +{
> > +     struct drm_dp_typec_port_data *port_data;
> > +     struct typec_mux_desc mux_desc = {};
> > +     char name[32];
> > +     u32 dp_lanes[2];
> > +     int ret, num_lanes, port_num = -1;
> > +
> > +     num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
>
> 2 looks incorrect. IIRC DP altmode can support up to 4 lanes.

This function is implemented for 4-lane DP bridges to switch its
outputs between 2 downstreams. So, I assume that there will only be at
most 2 lanes for each downstream. I don't think a 4-lane downstream
makes sense for mode switches unless we want to support bridges with
more than 4 lanes.
>
> > +     if (num_lanes <= 0) {
> > +             dev_err(dev, "Error on getting data lanes count from %s: %d\n",
> > +                     node->name, 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 from %s: %d\n",
> > +                     node->name, ret);
> > +             return ret;
> > +     }
> > +
> > +     port_num = dp_lanes[0] / 2;
> > +
> > +     port_data = &switch_desc->typec_ports[port_num];
> > +     port_data->data = data;
> > +     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 = 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;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +/**
> > + * drm_dp_register_typec_switches() - register Type-C switches
> > + * @dev: Device that registers Type-C switches
> > + * @port: Device node for the switch
> > + * @switch_desc: A Type-C switch descriptor
> > + * @data: Private data for the switches
> > + * @mux_set: Callback function for typec_mux_set
> > + *
> > + * This function registers USB Type-C switches for DP bridges that can switch
> > + * the output signal between their output pins.
> > + *
> > + * Currently only mode switches are implemented, and the function assumes the
> > + * given @port device node has endpoints with "mode-switch" property.
> > + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
> > + * and register it as port 1 if "data-lanes" falls in 2/3.
> > + */
> > +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> > +                                struct drm_dp_typec_switch_desc *switch_desc,
> > +                                void *data, typec_mux_set_fn_t mux_set)
> > +{
> > +     struct device_node *sw;
> > +     int ret;
> > +
> > +     for_each_child_of_node(port, sw) {
> > +             if (of_property_read_bool(sw, "mode-switch"))
> > +                     switch_desc->num_typec_switches++;
> > +     }
> > +
> > +     if (!switch_desc->num_typec_switches) {
> > +             dev_dbg(dev, "No Type-C switches node found\n");
> > +             return 0;
> > +     }
> > +
> > +     switch_desc->typec_ports = devm_kcalloc(
> > +             dev, switch_desc->num_typec_switches,
> > +             sizeof(struct drm_dp_typec_port_data), GFP_KERNEL);
> > +
> > +     if (!switch_desc->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 = drm_dp_register_mode_switch(dev, sw, switch_desc, data, mux_set);
> > +             if (ret)
> > +                     goto err_unregister_typec_switches;
> > +     }
> > +
> > +     return 0;
> > +
> > +err_unregister_typec_switches:
> > +     of_node_put(sw);
> > +     drm_dp_unregister_typec_switches(switch_desc);
> > +     dev_err(dev, "Failed to register mode switch: %d\n", ret);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> > +
> > +/**
> > + * drm_dp_unregister_typec_switches() - unregister Type-C switches
> > + * @switch_desc: A Type-C switch descriptor
> > + */
> > +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < switch_desc->num_typec_switches; i++)
> > +             typec_mux_unregister(switch_desc->typec_ports[i].typec_mux);
> > +}
> > +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> > +#else
> > +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> > +{
> > +}
> > +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> > +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> > +                                struct drm_dp_typec_switch_desc *switch_desc,
> > +                                void *data, typec_mux_set_fn_t mux_set)
> > +{
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> > +#endif
> > diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> > index ab55453f2d2c..5a3824f13b4e 100644
> > --- a/include/drm/display/drm_dp_helper.h
> > +++ b/include/drm/display/drm_dp_helper.h
> > @@ -25,6 +25,7 @@
> >
> >   #include <linux/delay.h>
> >   #include <linux/i2c.h>
> > +#include <linux/usb/typec_mux.h>
> >
> >   #include <drm/display/drm_dp.h>
> >   #include <drm/drm_connector.h>
> > @@ -763,4 +764,20 @@ bool drm_dp_downstream_rgb_to_ycbcr_conversion(const u8 dpcd[DP_RECEIVER_CAP_SIZ
> >                                              const u8 port_cap[4], u8 color_spc);
> >   int drm_dp_pcon_convert_rgb_to_ycbcr(struct drm_dp_aux *aux, u8 color_spc);
> >
> > +struct drm_dp_typec_port_data {
> > +     struct typec_mux_dev *typec_mux;
> > +     void *data;
> > +     bool dp_connected;
> > +};
> > +
> > +struct drm_dp_typec_switch_desc {
> > +     int num_typec_switches;
> > +     struct drm_dp_typec_port_data *typec_ports;
> > +};
> > +
> > +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
> > +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> > +                                struct drm_dp_typec_switch_desc *switch_desc,
> > +                                void *data, typec_mux_set_fn_t mux_set);
> > +
> >   #endif /* _DRM_DP_HELPER_H_ */
>
> --
> With best wishes
> Dmitry
>

Best regards,
Pin-yen

On Thu, Jan 12, 2023 at 1:24 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 12/01/2023 07:19, Pin-yen Lin wrote:
> > Hi Dmitry,
> >
> > Thanks for the review.
> >
> > On Thu, Jan 12, 2023 at 12:40 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> >>
> >> On 12/01/2023 06:20, Pin-yen Lin wrote:
> >>> Add helpers to register and unregister Type-C "switches" for bridges
> >>> capable of switching their output between two downstream devices.
> >>>
> >>> The helper registers USB Type-C mode switches when the "mode-switch"
> >>> and the "data-lanes" properties are available in Device Tree.
> >>>
> >>> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> >>> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> >>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
> >>> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >>>
> >>> ---
> >>>
> >>> Changes in v10:
> >>> - Collected Reviewed-by and Tested-by tags
> >>> - Replaced "void *" with "typec_mux_set_fn_t" for mux_set callbacks
> >>> - Print out the node name when errors on parsing DT
> >>> - Use dev_dbg instead of dev_warn when no Type-C switch nodes available
> >>> - Made the return path of drm_dp_register_mode_switch clearer
> >>>
> >>> Changes in v8:
> >>> - Fixed the build issue when CONFIG_TYPEC=m
> >>> - Fixed some style issues
> >>>
> >>> Changes in v7:
> >>> - Extracted the common codes to a helper function
> >>> - New in v7
> >>>
> >>>    drivers/gpu/drm/display/drm_dp_helper.c | 134 ++++++++++++++++++++++++
> >>>    include/drm/display/drm_dp_helper.h     |  17 +++
> >>>    2 files changed, 151 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> >>> index 16565a0a5da6..a2ec40a621cb 100644
> >>> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> >>> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> >>> @@ -30,11 +30,13 @@
> >>>    #include <linux/sched.h>
> >>>    #include <linux/seq_file.h>
> >>>    #include <linux/string_helpers.h>
> >>> +#include <linux/usb/typec_mux.h>
> >>>    #include <linux/dynamic_debug.h>
> >>>
> >>>    #include <drm/display/drm_dp_helper.h>
> >>>    #include <drm/display/drm_dp_mst_helper.h>
> >>>    #include <drm/drm_edid.h>
> >>> +#include <drm/drm_of.h>
> >>>    #include <drm/drm_print.h>
> >>>    #include <drm/drm_vblank.h>
> >>>    #include <drm/drm_panel.h>
> >>> @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
> >>>    EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
> >>>
> >>>    #endif
> >>> +
> >>> +#if IS_REACHABLE(CONFIG_TYPEC)
> >>> +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
> >>> +                                    struct drm_dp_typec_switch_desc *switch_desc,
> >>> +                                    void *data, typec_mux_set_fn_t mux_set)
> >>> +{
> >>> +     struct drm_dp_typec_port_data *port_data;
> >>> +     struct typec_mux_desc mux_desc = {};
> >>> +     char name[32];
> >>> +     u32 dp_lanes[2];
> >>> +     int ret, num_lanes, port_num = -1;
> >>> +
> >>> +     num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
> >>
> >> 2 looks incorrect. IIRC DP altmode can support up to 4 lanes.
> >
> > This function is implemented for 4-lane DP bridges to switch its
> > outputs between 2 downstreams. So, I assume that there will only be at
> > most 2 lanes for each downstream. I don't think a 4-lane downstream
> > makes sense for mode switches unless we want to support bridges with
> > more than 4 lanes.
>
> Yes. However by using 4 here you'd make the helper generic and cover
> both your case and the generic case. We don't need this for the msm case
> (since the mux is handled by the PHY). But if not for the PHY, I'd have
> used such helper (with max_lanes = 4).
>
I wonder if simply using 4 here really makes it more generic here.
This function assumes the mapping between "data-lanes" and the port
number (e.g., 0/1 --> port 0) and hard-coded the way to parse the
property.

Is it better to use "reg" instead of "data-lanes" to determine the
port number? The drivers can still read the DT node to get the
"data-lanes" property if they want to do some fancy stuffs around
that.
> >>
> >>> +     if (num_lanes <= 0) {
> >>> +             dev_err(dev, "Error on getting data lanes count from %s: %d\n",
> >>> +                     node->name, 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 from %s: %d\n",
> >>> +                     node->name, ret);
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     port_num = dp_lanes[0] / 2;
> >>> +
> >>> +     port_data = &switch_desc->typec_ports[port_num];
> >>> +     port_data->data = data;
> >>> +     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 = 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;
> >>> +     }
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +/**
> >>> + * drm_dp_register_typec_switches() - register Type-C switches
> >>> + * @dev: Device that registers Type-C switches
> >>> + * @port: Device node for the switch
> >>> + * @switch_desc: A Type-C switch descriptor
> >>> + * @data: Private data for the switches
> >>> + * @mux_set: Callback function for typec_mux_set
> >>> + *
> >>> + * This function registers USB Type-C switches for DP bridges that can switch
> >>> + * the output signal between their output pins.
> >>> + *
> >>> + * Currently only mode switches are implemented, and the function assumes the
> >>> + * given @port device node has endpoints with "mode-switch" property.
> >>> + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
> >>> + * and register it as port 1 if "data-lanes" falls in 2/3.
> >>> + */
> >>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> >>> +                                struct drm_dp_typec_switch_desc *switch_desc,
> >>> +                                void *data, typec_mux_set_fn_t mux_set)
> >>> +{
> >>> +     struct device_node *sw;
> >>> +     int ret;
> >>> +
> >>> +     for_each_child_of_node(port, sw) {
> >>> +             if (of_property_read_bool(sw, "mode-switch"))
> >>> +                     switch_desc->num_typec_switches++;
> >>> +     }
> >>> +
> >>> +     if (!switch_desc->num_typec_switches) {
> >>> +             dev_dbg(dev, "No Type-C switches node found\n");
> >>> +             return 0;
> >>> +     }
> >>> +
> >>> +     switch_desc->typec_ports = devm_kcalloc(
> >>> +             dev, switch_desc->num_typec_switches,
> >>> +             sizeof(struct drm_dp_typec_port_data), GFP_KERNEL);
> >>> +
> >>> +     if (!switch_desc->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 = drm_dp_register_mode_switch(dev, sw, switch_desc, data, mux_set);
> >>> +             if (ret)
> >>> +                     goto err_unregister_typec_switches;
> >>> +     }
> >>> +
> >>> +     return 0;
> >>> +
> >>> +err_unregister_typec_switches:
> >>> +     of_node_put(sw);
> >>> +     drm_dp_unregister_typec_switches(switch_desc);
> >>> +     dev_err(dev, "Failed to register mode switch: %d\n", ret);
> >>> +     return ret;
> >>> +}
> >>> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> >>> +
> >>> +/**
> >>> + * drm_dp_unregister_typec_switches() - unregister Type-C switches
> >>> + * @switch_desc: A Type-C switch descriptor
> >>> + */
> >>> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> >>> +{
> >>> +     int i;
> >>> +
> >>> +     for (i = 0; i < switch_desc->num_typec_switches; i++)
> >>> +             typec_mux_unregister(switch_desc->typec_ports[i].typec_mux);
> >>> +}
> >>> +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> >>> +#else
> >>> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> >>> +{
> >>> +}
> >>> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> >>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> >>> +                                struct drm_dp_typec_switch_desc *switch_desc,
> >>> +                                void *data, typec_mux_set_fn_t mux_set)
> >>> +{
> >>> +     return 0;
> >>> +}
> >>> +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> >>> +#endif
> >>> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> >>> index ab55453f2d2c..5a3824f13b4e 100644
> >>> --- a/include/drm/display/drm_dp_helper.h
> >>> +++ b/include/drm/display/drm_dp_helper.h
> >>> @@ -25,6 +25,7 @@
> >>>
> >>>    #include <linux/delay.h>
> >>>    #include <linux/i2c.h>
> >>> +#include <linux/usb/typec_mux.h>
> >>>
> >>>    #include <drm/display/drm_dp.h>
> >>>    #include <drm/drm_connector.h>
> >>> @@ -763,4 +764,20 @@ bool drm_dp_downstream_rgb_to_ycbcr_conversion(const u8 dpcd[DP_RECEIVER_CAP_SIZ
> >>>                                               const u8 port_cap[4], u8 color_spc);
> >>>    int drm_dp_pcon_convert_rgb_to_ycbcr(struct drm_dp_aux *aux, u8 color_spc);
> >>>
> >>> +struct drm_dp_typec_port_data {
> >>> +     struct typec_mux_dev *typec_mux;
> >>> +     void *data;
> >>> +     bool dp_connected;
> >>> +};
> >>> +
> >>> +struct drm_dp_typec_switch_desc {
> >>> +     int num_typec_switches;
> >>> +     struct drm_dp_typec_port_data *typec_ports;
> >>> +};
> >>> +
> >>> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
> >>> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> >>> +                                struct drm_dp_typec_switch_desc *switch_desc,
> >>> +                                void *data, typec_mux_set_fn_t mux_set);
> >>> +
> >>>    #endif /* _DRM_DP_HELPER_H_ */
> >>
> >> --
> >> With best wishes
> >> Dmitry
> >>
> >
> > Best regards,
> > Pin-yen
>
> --
> With best wishes
> Dmitry
>
Best regards,
Pin-yen

On Thu, 12 Jan 2023, Pin-yen Lin <treapking@chromium.org> wrote:
> Add helpers to register and unregister Type-C "switches" for bridges
> capable of switching their output between two downstream devices.
>
> The helper registers USB Type-C mode switches when the "mode-switch"
> and the "data-lanes" properties are available in Device Tree.
>
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>
> ---
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
> - Replaced "void *" with "typec_mux_set_fn_t" for mux_set callbacks
> - Print out the node name when errors on parsing DT
> - Use dev_dbg instead of dev_warn when no Type-C switch nodes available
> - Made the return path of drm_dp_register_mode_switch clearer
>
> Changes in v8:
> - Fixed the build issue when CONFIG_TYPEC=m
> - Fixed some style issues
>
> Changes in v7:
> - Extracted the common codes to a helper function
> - New in v7
>
>  drivers/gpu/drm/display/drm_dp_helper.c | 134 ++++++++++++++++++++++++
>  include/drm/display/drm_dp_helper.h     |  17 +++
>  2 files changed, 151 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 16565a0a5da6..a2ec40a621cb 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -30,11 +30,13 @@
>  #include <linux/sched.h>
>  #include <linux/seq_file.h>
>  #include <linux/string_helpers.h>
> +#include <linux/usb/typec_mux.h>
>  #include <linux/dynamic_debug.h>
>  
>  #include <drm/display/drm_dp_helper.h>
>  #include <drm/display/drm_dp_mst_helper.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_vblank.h>
>  #include <drm/drm_panel.h>
> @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
>  EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
>  
>  #endif
> +
> +#if IS_REACHABLE(CONFIG_TYPEC)

I think IS_REACHABLE() is a workaround for not getting the Kconfig
dependencies right. It allows configurations that silently just don't
work, instead of warning about it at config time. It fixes a build
issue, but trades it for an end user configuration issue that you don't
get any feedback about, and is hard to figure out. It's for people who
deal with build issues, but don't need to deal with user issues.

BR,
Jani.



> +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
> +				       struct drm_dp_typec_switch_desc *switch_desc,
> +				       void *data, typec_mux_set_fn_t mux_set)
> +{
> +	struct drm_dp_typec_port_data *port_data;
> +	struct typec_mux_desc mux_desc = {};
> +	char name[32];
> +	u32 dp_lanes[2];
> +	int ret, num_lanes, 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 from %s: %d\n",
> +			node->name, 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 from %s: %d\n",
> +			node->name, ret);
> +		return ret;
> +	}
> +
> +	port_num = dp_lanes[0] / 2;
> +
> +	port_data = &switch_desc->typec_ports[port_num];
> +	port_data->data = data;
> +	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 = 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;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * drm_dp_register_typec_switches() - register Type-C switches
> + * @dev: Device that registers Type-C switches
> + * @port: Device node for the switch
> + * @switch_desc: A Type-C switch descriptor
> + * @data: Private data for the switches
> + * @mux_set: Callback function for typec_mux_set
> + *
> + * This function registers USB Type-C switches for DP bridges that can switch
> + * the output signal between their output pins.
> + *
> + * Currently only mode switches are implemented, and the function assumes the
> + * given @port device node has endpoints with "mode-switch" property.
> + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
> + * and register it as port 1 if "data-lanes" falls in 2/3.
> + */
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> +				   struct drm_dp_typec_switch_desc *switch_desc,
> +				   void *data, typec_mux_set_fn_t mux_set)
> +{
> +	struct device_node *sw;
> +	int ret;
> +
> +	for_each_child_of_node(port, sw) {
> +		if (of_property_read_bool(sw, "mode-switch"))
> +			switch_desc->num_typec_switches++;
> +	}
> +
> +	if (!switch_desc->num_typec_switches) {
> +		dev_dbg(dev, "No Type-C switches node found\n");
> +		return 0;
> +	}
> +
> +	switch_desc->typec_ports = devm_kcalloc(
> +		dev, switch_desc->num_typec_switches,
> +		sizeof(struct drm_dp_typec_port_data), GFP_KERNEL);
> +
> +	if (!switch_desc->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 = drm_dp_register_mode_switch(dev, sw, switch_desc, data, mux_set);
> +		if (ret)
> +			goto err_unregister_typec_switches;
> +	}
> +
> +	return 0;
> +
> +err_unregister_typec_switches:
> +	of_node_put(sw);
> +	drm_dp_unregister_typec_switches(switch_desc);
> +	dev_err(dev, "Failed to register mode switch: %d\n", ret);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> +
> +/**
> + * drm_dp_unregister_typec_switches() - unregister Type-C switches
> + * @switch_desc: A Type-C switch descriptor
> + */
> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> +{
> +	int i;
> +
> +	for (i = 0; i < switch_desc->num_typec_switches; i++)
> +		typec_mux_unregister(switch_desc->typec_ports[i].typec_mux);
> +}
> +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> +#else
> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> +{
> +}
> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> +				   struct drm_dp_typec_switch_desc *switch_desc,
> +				   void *data, typec_mux_set_fn_t mux_set)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> +#endif
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index ab55453f2d2c..5a3824f13b4e 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -25,6 +25,7 @@
>  
>  #include <linux/delay.h>
>  #include <linux/i2c.h>
> +#include <linux/usb/typec_mux.h>
>  
>  #include <drm/display/drm_dp.h>
>  #include <drm/drm_connector.h>
> @@ -763,4 +764,20 @@ bool drm_dp_downstream_rgb_to_ycbcr_conversion(const u8 dpcd[DP_RECEIVER_CAP_SIZ
>  					       const u8 port_cap[4], u8 color_spc);
>  int drm_dp_pcon_convert_rgb_to_ycbcr(struct drm_dp_aux *aux, u8 color_spc);
>  
> +struct drm_dp_typec_port_data {
> +	struct typec_mux_dev *typec_mux;
> +	void *data;
> +	bool dp_connected;
> +};
> +
> +struct drm_dp_typec_switch_desc {
> +	int num_typec_switches;
> +	struct drm_dp_typec_port_data *typec_ports;
> +};
> +
> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> +				   struct drm_dp_typec_switch_desc *switch_desc,
> +				   void *data, typec_mux_set_fn_t mux_set);
> +
>  #endif /* _DRM_DP_HELPER_H_ */

Hi Jani,

Thanks for the review.

On Thu, Jan 12, 2023 at 4:37 PM Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Thu, 12 Jan 2023, Pin-yen Lin <treapking@chromium.org> wrote:
> > Add helpers to register and unregister Type-C "switches" for bridges
> > capable of switching their output between two downstream devices.
> >
> > The helper registers USB Type-C mode switches when the "mode-switch"
> > and the "data-lanes" properties are available in Device Tree.
> >
> > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >
> > ---
> >
> > Changes in v10:
> > - Collected Reviewed-by and Tested-by tags
> > - Replaced "void *" with "typec_mux_set_fn_t" for mux_set callbacks
> > - Print out the node name when errors on parsing DT
> > - Use dev_dbg instead of dev_warn when no Type-C switch nodes available
> > - Made the return path of drm_dp_register_mode_switch clearer
> >
> > Changes in v8:
> > - Fixed the build issue when CONFIG_TYPEC=m
> > - Fixed some style issues
> >
> > Changes in v7:
> > - Extracted the common codes to a helper function
> > - New in v7
> >
> >  drivers/gpu/drm/display/drm_dp_helper.c | 134 ++++++++++++++++++++++++
> >  include/drm/display/drm_dp_helper.h     |  17 +++
> >  2 files changed, 151 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> > index 16565a0a5da6..a2ec40a621cb 100644
> > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > @@ -30,11 +30,13 @@
> >  #include <linux/sched.h>
> >  #include <linux/seq_file.h>
> >  #include <linux/string_helpers.h>
> > +#include <linux/usb/typec_mux.h>
> >  #include <linux/dynamic_debug.h>
> >
> >  #include <drm/display/drm_dp_helper.h>
> >  #include <drm/display/drm_dp_mst_helper.h>
> >  #include <drm/drm_edid.h>
> > +#include <drm/drm_of.h>
> >  #include <drm/drm_print.h>
> >  #include <drm/drm_vblank.h>
> >  #include <drm/drm_panel.h>
> > @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
> >  EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
> >
> >  #endif
> > +
> > +#if IS_REACHABLE(CONFIG_TYPEC)
>
> I think IS_REACHABLE() is a workaround for not getting the Kconfig
> dependencies right. It allows configurations that silently just don't
> work, instead of warning about it at config time. It fixes a build
> issue, but trades it for an end user configuration issue that you don't
> get any feedback about, and is hard to figure out. It's for people who
> deal with build issues, but don't need to deal with user issues.
>
> BR,
> Jani.
>
I've added "depends on TYPEC || TYPEC=n" on the Kconfigs of its users
(i.e., anx7625 and it6505). I didn't do this on DRM_DISPLAY_DP_HELPER
because that stops all users of DRM_DISPLAY_DP_HELPER, which is a
quite generic helper file, from building TYPEC as a module.

Or, do you have any other suggestions on this? Move these functions to
a separate file?

>
>
> > +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,
> > +                                    struct drm_dp_typec_switch_desc *switch_desc,
> > +                                    void *data, typec_mux_set_fn_t mux_set)
> > +{
> > +     struct drm_dp_typec_port_data *port_data;
> > +     struct typec_mux_desc mux_desc = {};
> > +     char name[32];
> > +     u32 dp_lanes[2];
> > +     int ret, num_lanes, 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 from %s: %d\n",
> > +                     node->name, 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 from %s: %d\n",
> > +                     node->name, ret);
> > +             return ret;
> > +     }
> > +
> > +     port_num = dp_lanes[0] / 2;
> > +
> > +     port_data = &switch_desc->typec_ports[port_num];
> > +     port_data->data = data;
> > +     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 = 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;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +/**
> > + * drm_dp_register_typec_switches() - register Type-C switches
> > + * @dev: Device that registers Type-C switches
> > + * @port: Device node for the switch
> > + * @switch_desc: A Type-C switch descriptor
> > + * @data: Private data for the switches
> > + * @mux_set: Callback function for typec_mux_set
> > + *
> > + * This function registers USB Type-C switches for DP bridges that can switch
> > + * the output signal between their output pins.
> > + *
> > + * Currently only mode switches are implemented, and the function assumes the
> > + * given @port device node has endpoints with "mode-switch" property.
> > + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
> > + * and register it as port 1 if "data-lanes" falls in 2/3.
> > + */
> > +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> > +                                struct drm_dp_typec_switch_desc *switch_desc,
> > +                                void *data, typec_mux_set_fn_t mux_set)
> > +{
> > +     struct device_node *sw;
> > +     int ret;
> > +
> > +     for_each_child_of_node(port, sw) {
> > +             if (of_property_read_bool(sw, "mode-switch"))
> > +                     switch_desc->num_typec_switches++;
> > +     }
> > +
> > +     if (!switch_desc->num_typec_switches) {
> > +             dev_dbg(dev, "No Type-C switches node found\n");
> > +             return 0;
> > +     }
> > +
> > +     switch_desc->typec_ports = devm_kcalloc(
> > +             dev, switch_desc->num_typec_switches,
> > +             sizeof(struct drm_dp_typec_port_data), GFP_KERNEL);
> > +
> > +     if (!switch_desc->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 = drm_dp_register_mode_switch(dev, sw, switch_desc, data, mux_set);
> > +             if (ret)
> > +                     goto err_unregister_typec_switches;
> > +     }
> > +
> > +     return 0;
> > +
> > +err_unregister_typec_switches:
> > +     of_node_put(sw);
> > +     drm_dp_unregister_typec_switches(switch_desc);
> > +     dev_err(dev, "Failed to register mode switch: %d\n", ret);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> > +
> > +/**
> > + * drm_dp_unregister_typec_switches() - unregister Type-C switches
> > + * @switch_desc: A Type-C switch descriptor
> > + */
> > +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < switch_desc->num_typec_switches; i++)
> > +             typec_mux_unregister(switch_desc->typec_ports[i].typec_mux);
> > +}
> > +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> > +#else
> > +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> > +{
> > +}
> > +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> > +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> > +                                struct drm_dp_typec_switch_desc *switch_desc,
> > +                                void *data, typec_mux_set_fn_t mux_set)
> > +{
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> > +#endif
> > diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> > index ab55453f2d2c..5a3824f13b4e 100644
> > --- a/include/drm/display/drm_dp_helper.h
> > +++ b/include/drm/display/drm_dp_helper.h
> > @@ -25,6 +25,7 @@
> >
> >  #include <linux/delay.h>
> >  #include <linux/i2c.h>
> > +#include <linux/usb/typec_mux.h>
> >
> >  #include <drm/display/drm_dp.h>
> >  #include <drm/drm_connector.h>
> > @@ -763,4 +764,20 @@ bool drm_dp_downstream_rgb_to_ycbcr_conversion(const u8 dpcd[DP_RECEIVER_CAP_SIZ
> >                                              const u8 port_cap[4], u8 color_spc);
> >  int drm_dp_pcon_convert_rgb_to_ycbcr(struct drm_dp_aux *aux, u8 color_spc);
> >
> > +struct drm_dp_typec_port_data {
> > +     struct typec_mux_dev *typec_mux;
> > +     void *data;
> > +     bool dp_connected;
> > +};
> > +
> > +struct drm_dp_typec_switch_desc {
> > +     int num_typec_switches;
> > +     struct drm_dp_typec_port_data *typec_ports;
> > +};
> > +
> > +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
> > +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> > +                                struct drm_dp_typec_switch_desc *switch_desc,
> > +                                void *data, typec_mux_set_fn_t mux_set);
> > +
> >  #endif /* _DRM_DP_HELPER_H_ */
>
> --
> Jani Nikula, Intel Open Source Graphics Center

Best regards,
Pin-yen

Hi,

On Thu, Jan 12, 2023 at 12:20:58PM +0800, Pin-yen Lin wrote:
> Add helpers to register and unregister Type-C "switches" for bridges
> capable of switching their output between two downstream devices.
> 
> The helper registers USB Type-C mode switches when the "mode-switch"
> and the "data-lanes" properties are available in Device Tree.

Let's not make this kind of helpers DT only, please. See below ...

> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 
> ---
> 
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
> - Replaced "void *" with "typec_mux_set_fn_t" for mux_set callbacks
> - Print out the node name when errors on parsing DT
> - Use dev_dbg instead of dev_warn when no Type-C switch nodes available
> - Made the return path of drm_dp_register_mode_switch clearer
> 
> Changes in v8:
> - Fixed the build issue when CONFIG_TYPEC=m
> - Fixed some style issues
> 
> Changes in v7:
> - Extracted the common codes to a helper function
> - New in v7
> 
>  drivers/gpu/drm/display/drm_dp_helper.c | 134 ++++++++++++++++++++++++
>  include/drm/display/drm_dp_helper.h     |  17 +++
>  2 files changed, 151 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 16565a0a5da6..a2ec40a621cb 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -30,11 +30,13 @@
>  #include <linux/sched.h>
>  #include <linux/seq_file.h>
>  #include <linux/string_helpers.h>
> +#include <linux/usb/typec_mux.h>
>  #include <linux/dynamic_debug.h>
>  
>  #include <drm/display/drm_dp_helper.h>
>  #include <drm/display/drm_dp_mst_helper.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_vblank.h>
>  #include <drm/drm_panel.h>
> @@ -3891,3 +3893,135 @@ int drm_panel_dp_aux_backlight(struct drm_panel *panel, struct drm_dp_aux *aux)
>  EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
>  
>  #endif
> +
> +#if IS_REACHABLE(CONFIG_TYPEC)

I think Jani already pointed out that that is wrong. Just move these
into a separate file and enable them silently in the Makefile when
TYPEC is enabled - so no separate Kconfig option.

> +static int drm_dp_register_mode_switch(struct device *dev, struct device_node *node,

static int drm_dp_register_mode_switch(struct device *dev, struct fwnode_handle *fwnode,

> +				       struct drm_dp_typec_switch_desc *switch_desc,
> +				       void *data, typec_mux_set_fn_t mux_set)
> +{
> +	struct drm_dp_typec_port_data *port_data;
> +	struct typec_mux_desc mux_desc = {};
> +	char name[32];
> +	u32 dp_lanes[2];
> +	int ret, num_lanes, port_num = -1;
> +
> +	num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
> +	if (num_lanes <= 0) {

        num_lanes = fwnode_property_read_u32_array(fwnode, "data-lanes", NULL, 0);
        if (num_lanes <= 0 || num_lanes > 2)

> +		dev_err(dev, "Error on getting data lanes count from %s: %d\n",
> +			node->name, num_lanes);
> +		return num_lanes;
> +	}
> +
> +	ret = of_property_read_u32_array(node, "data-lanes", dp_lanes, num_lanes);

        ret = fwnode_property_read_u32_array(fwnode, "data-lanes", dp_lanes, num_lanes);

> +	if (ret) {
> +		dev_err(dev, "Failed to read the data-lanes variable from %s: %d\n",
> +			node->name, ret);

			fwnode_get_name(fwnode), ret);

> +		return ret;
> +	}
> +
> +	port_num = dp_lanes[0] / 2;
> +
> +	port_data = &switch_desc->typec_ports[port_num];
> +	port_data->data = data;
> +	mux_desc.fwnode = &node->fwnode;

        mux_desc.fwnode = fwnode;

> +	mux_desc.drvdata = port_data;
> +	snprintf(name, sizeof(name), "%s-%u", node->name, port_num);

	snprintf(name, sizeof(name), "%s-%u", fwnode_get_name(fwnode), port_num);

> +	mux_desc.name = name;
> +	mux_desc.set = 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;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * drm_dp_register_typec_switches() - register Type-C switches
> + * @dev: Device that registers Type-C switches
> + * @port: Device node for the switch
> + * @switch_desc: A Type-C switch descriptor
> + * @data: Private data for the switches
> + * @mux_set: Callback function for typec_mux_set
> + *
> + * This function registers USB Type-C switches for DP bridges that can switch
> + * the output signal between their output pins.
> + *
> + * Currently only mode switches are implemented, and the function assumes the
> + * given @port device node has endpoints with "mode-switch" property.
> + * Register the endpoint as port 0 if the "data-lanes" property falls in 0/1,
> + * and register it as port 1 if "data-lanes" falls in 2/3.
> + */
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,

int drm_dp_register_typec_switches(struct device *dev, struct fwnode_handle *port,

> +				   struct drm_dp_typec_switch_desc *switch_desc,
> +				   void *data, typec_mux_set_fn_t mux_set)
> +{
> +	struct device_node *sw;

        struct fwnode_handle *sw;

> +	int ret;
> +
> +	for_each_child_of_node(port, sw) {
> +		if (of_property_read_bool(sw, "mode-switch"))
> +			switch_desc->num_typec_switches++;
> +	}

        fwnode_for_each_child_node(port, sw)
                if (fwnode_property_present(sw, "mode-switch"))
			switch_desc->num_typec_switches++;

> +	if (!switch_desc->num_typec_switches) {
> +		dev_dbg(dev, "No Type-C switches node found\n");
> +		return 0;
> +	}
> +
> +	switch_desc->typec_ports = devm_kcalloc(
> +		dev, switch_desc->num_typec_switches,
> +		sizeof(struct drm_dp_typec_port_data), GFP_KERNEL);
> +
> +	if (!switch_desc->typec_ports)
> +		return -ENOMEM;
> +
> +	/* Register switches for each connector. */
> +	for_each_child_of_node(port, sw) {
> +		if (!of_property_read_bool(sw, "mode-switch"))

        fwnode_for_each_child_node(port, sw) {
                if (!fwnode_property_present(sw, "mode-switch"))

> +			continue;
> +		ret = drm_dp_register_mode_switch(dev, sw, switch_desc, data, mux_set);
> +		if (ret)
> +			goto err_unregister_typec_switches;
> +	}
> +
> +	return 0;
> +
> +err_unregister_typec_switches:
> +	of_node_put(sw);
> +	drm_dp_unregister_typec_switches(switch_desc);
> +	dev_err(dev, "Failed to register mode switch: %d\n", ret);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> +
> +/**
> + * drm_dp_unregister_typec_switches() - unregister Type-C switches
> + * @switch_desc: A Type-C switch descriptor
> + */
> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> +{
> +	int i;
> +
> +	for (i = 0; i < switch_desc->num_typec_switches; i++)
> +		typec_mux_unregister(switch_desc->typec_ports[i].typec_mux);
> +}
> +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> +#else
> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc)
> +{
> +}
> +EXPORT_SYMBOL(drm_dp_register_typec_switches);
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> +				   struct drm_dp_typec_switch_desc *switch_desc,
> +				   void *data, typec_mux_set_fn_t mux_set)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_dp_unregister_typec_switches);
> +#endif
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index ab55453f2d2c..5a3824f13b4e 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -25,6 +25,7 @@
>  
>  #include <linux/delay.h>
>  #include <linux/i2c.h>
> +#include <linux/usb/typec_mux.h>
>  
>  #include <drm/display/drm_dp.h>
>  #include <drm/drm_connector.h>
> @@ -763,4 +764,20 @@ bool drm_dp_downstream_rgb_to_ycbcr_conversion(const u8 dpcd[DP_RECEIVER_CAP_SIZ
>  					       const u8 port_cap[4], u8 color_spc);
>  int drm_dp_pcon_convert_rgb_to_ycbcr(struct drm_dp_aux *aux, u8 color_spc);
>  
> +struct drm_dp_typec_port_data {
> +	struct typec_mux_dev *typec_mux;
> +	void *data;
> +	bool dp_connected;
> +};
> +
> +struct drm_dp_typec_switch_desc {
> +	int num_typec_switches;
> +	struct drm_dp_typec_port_data *typec_ports;
> +};
> +
> +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc *switch_desc);
> +int drm_dp_register_typec_switches(struct device *dev, struct device_node *port,
> +				   struct drm_dp_typec_switch_desc *switch_desc,
> +				   void *data, typec_mux_set_fn_t mux_set);
> +
>  #endif /* _DRM_DP_HELPER_H_ */

The function stubs go here if they are needed.


thanks,

On Fri, Jan 13, 2023 at 11:23:44AM +0200, Heikki Krogerus wrote:
> On Thu, Jan 12, 2023 at 12:20:58PM +0800, Pin-yen Lin wrote:

...

> > +		dev_err(dev, "Failed to read the data-lanes variable from %s: %d\n",
> > +			node->name, ret);
> 
> 			fwnode_get_name(fwnode), ret);

Or even %pfwP ?

...

> > +	snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
> 
> 	snprintf(name, sizeof(name), "%s-%u", fwnode_get_name(fwnode), port_num);

Ditto.