Re: [PATCH v4 09/14] usb: hub: Power on connected M.2 E-key connectors with power sequencing API

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Jul 09, 2026 at 05:57:14PM +0800, Chen-Yu Tsai wrote:
> The new M.2 E-key connector can have a USB connection. For the USB device
> on this connector to work, its power must be enabled and the W_DISABLE2#
> signal deasserted. The connector driver handles this and provides a
> toggle over the power sequencing API.
> 
> This feature currently only supports a directly connected (no mux in
> between) M.2 E-key connector. Existing USB connector types are not
> covered. The USB A connector was recently added to the onboard devices
> driver. USB B connectors have historically been managed by the USB
> gadget or dual-role device controller drivers. USB C connectors are
> handled by TCPM drivers.
> 
> The power sequencing API does not know whether a power sequence provider
> is not needed or not available yet, so we only request it for connectors
> that we know need it, which at this time is just the E-key connector.
> 
> On the USB side, the port firmware node (if present) is tied to the
> usb_port device. This device is used to acquire the power sequencing
> descriptor. This allows the provider to tell the different ports on one
> hub apart.
> 
> This feature is not implemented in the onboard USB devices driver. The
> power sequencing API expects the consumer device to make the request,
> but there is no device node to instantiate a platform device to tie
> the driver to. The connector is not a child node of the USB host or
> hub, and the graph connection is from a USB port to the connector.
> And the connector itself already has a driver.
> 
> Power sequencing is not directly enabled in the connector driver as
> that would completely decouple the timing of it from the USB subsystem.
> It would not be possible for the USB subsystem to toggle the power
> for a power cycle or to disable the port.
> 
> Also rewrite the existing set_bit() and clear_bit() branches with
> assign_bit() to make it cleaner.

...

>  int usb_hub_set_port_power(struct usb_device *hdev, struct usb_hub *hub,
>  			   int port1, bool set)
>  {
> +	struct usb_port *pwrseq_port = hub->ports[port1 - 1];
>  	int ret;
>  
> +	ret = usb_hub_set_port_pwrseq(pwrseq_port, set);
> +	if (ret)
> +		return ret;
> +
>  	if (set)
>  		ret = set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
>  	else
>  		ret = usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
>  
> -	if (ret)
> +	if (ret) {
> +		usb_hub_set_port_pwrseq(pwrseq_port, !set);
>  		return ret;
> +	}

> -	if (set)
> -		set_bit(port1, hub->power_bits);
> -	else
> -		clear_bit(port1, hub->power_bits);
> +	assign_bit(port1, hub->power_bits, set);

I just realised that this change doesn't affect any functionality neither
before nor after the core of this patch. With that being said, I think you
may split this small piece to a separate patch to avoid unneeded churn here.
I will give a tag to that patch immediately I see it.

>  	return 0;
>  }

...

> -	return ret;
> +	/* stub functions return error */
> +	if (!IS_ENABLED(CONFIG_POWER_SEQUENCING))
> +		return ret;
> +
> +	if (!port->pwrseq)
> +		return ret;
> +
> +	return ret && pwrseq_power_is_on(port->pwrseq);

Perhaps

	/* stub functions return error */
	if (IS_ENABLED(CONFIG_POWER_SEQUENCING) && port->pwrseq)
		return ret && pwrseq_power_is_on(port->pwrseq);
	return ret;

? This will also make better visibility of what you meant in the comment
above.

>  }

...

> +static bool port_pwrseq_is_supported(struct usb_port *port_dev)
> +{
> +	struct device *dev = &port_dev->dev;
> +	struct fwnode_handle *port = dev->fwnode;

Use dev_fwnode().

Also it make sense to add a blank line here to keep __free()-defined variables
coupled with the checks (and hence scope).

> +	struct fwnode_handle *ep __free(fwnode_handle) =
> +			fwnode_graph_get_next_port_endpoint(port, NULL);
> +	if (!ep)
> +		return false;
> +
> +	struct fwnode_handle *remote __free(fwnode_handle) =
> +			fwnode_graph_get_remote_port_parent(ep);
> +	if (!remote)
> +		return false;
> +
> +	if (!fwnode_device_is_compatible(remote, "pcie-m2-e-connector")) {
> +		dev_dbg(dev, "remote endpoint %pfw is not a supported connector", remote);
> +		return false;
> +	}
> +
> +	return true;
> +}

-- 
With Best Regards,
Andy Shevchenko