On Sat, Aug 31, 2024 at 09:06:44PM -0700, Stephen Boyd wrote:
> Extend the aux-hpd bridge driver to support assigning DP lanes to USB
> type-c pins based on typec mux state entry. Existing users of this
> driver only need the HPD signaling support, so leave that in place and
> wrap the code with a variant that supports more features of USB type-c
Isn't the proper spelling "USB Type-C"?
> DP altmode, i.e. pin configurations. Prefix that code with
> 'drm_dp_typec_bridge' to differentiate it from the existing
> 'drm_aux_hpd_bridge' code.
>
> Parse the struct typec_mux_state members to determine if DP altmode has
> been entered and if HPD is asserted or not. Signal HPD to the drm bridge
> chain when HPD is asserted. Similarly, parse the pin assignment and map
> the DP lanes to the usb-c output lanes, taking into account any lane
> remapping from the data-lanes endpoint property. Pass that lane mapping
> to the previous drm_bridge in the bridge chain during the atomic check
> phase.
...
> +static inline struct drm_dp_typec_bridge_data *
> +hpd_bridge_to_typec_bridge_data(struct drm_aux_hpd_bridge_data *hpd_data)
> +{
> + return container_of(hpd_data, struct drm_dp_typec_bridge_data,
> hpd_bridge);
+ container_of.h ?
With that said, can the argument be const here?
> +}
...
Ditto for the two more helpers, added in this change.
...
> +static void drm_dp_typec_bridge_release(struct device *dev)
> +{
> + struct drm_dp_typec_bridge_dev *typec_bridge_dev;
> + struct auxiliary_device *adev;
> +
> + typec_bridge_dev = to_drm_dp_typec_bridge_dev(dev);
> + adev = &typec_bridge_dev->adev;
> +
> + ida_free(&drm_aux_hpd_bridge_ida, adev->id);
> + of_node_put(adev->dev.platform_data);
> + of_node_put(adev->dev.of_node);
I'm wondering why it's not made fwnode to begin with...
>From the file / function names it's not obvious that it's OF-only code. Neither
there is no explanations why this must be OF-only code (among all fwnode types
supported).
> + kfree(typec_bridge_dev);
> +}
...
> + return ERR_PTR(dev_err_probe(parent, -ENODEV, "Missing typec
> endpoint(s) port@0\n"));
We have a new helper for such cases.
...
> + adev->dev.of_node = of_node_get(parent->of_node);
device_set_node() ?
...
> + ret = auxiliary_device_init(adev);
> + if (ret) {
> + of_node_put(adev->dev.platform_data);
> + of_node_put(adev->dev.of_node);
> + ida_free(&drm_aux_hpd_bridge_ida, adev->id);
> + kfree(adev);
> + return ERR_PTR(ret);
Can cleanup.h be utilised here and in other error paths in this function?
> + }
> + ret = devm_add_action_or_reset(parent, drm_aux_hpd_bridge_free_adev,
> adev);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return typec_bridge_dev;
> +}
...
> +static int dp_lane_to_typec_lane(enum dp_lane lane)
> +{
> + switch (lane) {
> + case DP_ML0:
> + return USB_SSTX2;
> + case DP_ML1:
> + return USB_SSRX2;
> + case DP_ML2:
> + return USB_SSTX1;
> + case DP_ML3:
> + return USB_SSRX1;
> + }
> + return -EINVAL;
Hmm... This can be simply made as default case.
> +}
> +
> +static int typec_to_dp_lane(enum usb_ss_lane lane)
> +{
> + switch (lane) {
> + case USB_SSRX1:
> + return DP_ML3;
> + case USB_SSTX1:
> + return DP_ML2;
> + case USB_SSTX2:
> + return DP_ML0;
> + case USB_SSRX2:
> + return DP_ML1;
> + }
> +
> + return -EINVAL;
Ditto.
> +}
...
> + for (i = 0; i < num_lanes; i++) {
> + /* Get physical type-c lane for DP lane */
> + typec_lane = dp_lane_to_typec_lane(i);
> + if (typec_lane < 0) {
> + dev_err(&adev->dev, "Invalid type-c lane configuration
> at DP_ML%d\n", i);
> + return -EINVAL;
Most likely typec_lane contains an error code here. If yes, it would be rather
return typec_lane;
If no, why not make that happen?
> + }
> +
> + /* Map physical to logical type-c lane */
> + typec_lane = lane_mapping[typec_lane];
> +
> + /* Map logical type-c lane to logical DP lane */
> + dp_lanes[i] = typec_to_dp_lane(typec_lane);
> + }
...
> +static int drm_dp_typec_bridge_atomic_check(struct drm_bridge *bridge,
> + struct drm_bridge_state
> *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state
> *conn_state)
> +{
> + struct drm_dp_typec_bridge_data *data;
> + struct drm_lane_cfg *in_lanes;
> + u8 *dp_lanes;
> + size_t num_lanes;
> + int i;
Does it need to be signed? Can it theoretically overflow as num_lanes defined
as size_t?
> + data = to_drm_dp_typec_bridge_data(bridge);
> + num_lanes = data->num_lanes;
> + if (!num_lanes)
> + return 0;
> + dp_lanes = data->dp_lanes;
> +
> + in_lanes = kcalloc(num_lanes, sizeof(*in_lanes), GFP_KERNEL);
> + if (!in_lanes)
> + return -ENOMEM;
> +
> + bridge_state->input_bus_cfg.lanes = in_lanes;
> + bridge_state->input_bus_cfg.num_lanes = num_lanes;
> +
> + for (i = 0; i < num_lanes; i++)
> + in_lanes[i].logical = dp_lanes[i];
> +
> + return 0;
> +}
...
> + port->typec_data = typec_data;
> + if (of_property_read_u32_array(ep.local_node, "data-lanes",
> + port->lane_mapping,
> + ARRAY_SIZE(port->lane_mapping)))
> {
> + memcpy(port->lane_mapping, mapping, sizeof(mapping));
Hmm... I'm wondering if direct assignment will save a few .text bytes
port->lane_mapping = ...;
of_property_read_u32_array(ep.local_node, "data-lanes",
port->lane_mapping,
ARRAY_SIZE(port->lane_mapping));
Also note that conditional is not needed in this case.
(And again, why OF-centric code?)
> + }
--
With Best Regards,
Andy Shevchenko
