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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 12/01/2023 07:48, Pin-yen Lin wrote:
> 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.

Yes, I admit, this sounds more logical.

>>>>
>>>>> +     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

-- 
With best wishes
Dmitry