Re: [PATCH v4 06/18] drm/bridge: aux-hpd: Support USB Type-C DP altmodes via DRM lane assignment

[Andy Shevchenko]

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