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