Re: [PATCH v3 08/13] usb: hub: Power on connected M.2 E-key connectors with power sequencing API

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

On Fri, Jul 03, 2026 at 07:03:09PM +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.

...

> +static int usb_hub_set_port_pwrseq(struct usb_port *port, bool set)

Not sure 'set' in the name is a good choice as you have also parameter 'set',
can be confusing. So for the other one. I don't know enough about pwrseq to
suggest better naming of these parts, but I would like to see some consistency
and less oddity.

> +{
> +	int ret = 0;

Unneeded assignment.

> +	if (set)
> +		ret = pwrseq_power_on(port->pwrseq);
> +	else
> +		ret = pwrseq_power_off(port->pwrseq);
> +
> +	return ret;

Moreover, for now this can be written as

	if (set)
		return pwrseq_power_on(port->pwrseq);

	return pwrseq_power_off(port->pwrseq);

> +}
> +
> +static int usb_hub_restore_port_pwrseq(struct usb_port *port, bool set)
> +{
> +	int ret = 0;

Ditto.

> +	if (set)
> +		ret = pwrseq_power_off(port->pwrseq);
> +	else
> +		ret = pwrseq_power_on(port->pwrseq);
> +
> +	return ret;
> +}

...

> +	port_dev->pwrseq = usb_hub_port_pwrseq_get(port_dev);
> +	if (IS_ERR(port_dev->pwrseq)) {
> +		retval = PTR_ERR(port_dev->pwrseq);
> +		dev_err_probe(&port_dev->dev, retval,
> +			      "failed to get power sequencing descriptor\n");

		retval = dev_err_probe(&port_dev->dev, PTR_ERR(port_dev->pwrseq),
				       "failed to get power sequencing descriptor\n");

> +		goto err_put_kn;
> +	}

-- 
With Best Regards,
Andy Shevchenko