[no subject]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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






[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux