Re: [PATCH v3 1/4] usb: typec: Add attribute file showing the supported USB modes of the port

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, Oct 11, 2024 at 03:43:59PM +0300, Heikki Krogerus wrote:
> +What:		/sys/class/typec/<port>/usb_capability
> +Date:		May 2024

It's a bit past May 2024 :)

> +static ssize_t
> +usb_capability_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	enum usb_mode mode = to_typec_port(dev)->usb_mode;
> +	u8 cap = to_typec_port(dev)->cap->usb_capability;
> +	int len = 0;
> +	int i;
> +
> +	for (i = USB_MODE_USB2; i < USB_MODE_USB4 + 1; i++) {
> +		if (!(BIT(i - 1) & cap))
> +			continue;
> +
> +		if (i == mode)
> +			len += sysfs_emit_at(buf, len, "[%s] ", usb_modes[i]);
> +		else
> +			len += sysfs_emit_at(buf, len, "%s ", usb_modes[i]);
> +	}
> +
> +	buf[len - 1] = '\n';

Nit, shouldn't this be another call to sysfs_emit_at()?

> @@ -240,6 +251,7 @@ struct typec_partner_desc {
>   * @port_type_set: Set port type
>   * @pd_get: Get available USB Power Delivery Capabilities.
>   * @pd_set: Set USB Power Delivery Capabilities.
> + * @default_usb_mode_set: USB Mode to be used by default with Enter_USB Message
>   */
>  struct typec_operations {
>  	int (*try_role)(struct typec_port *port, int role);
> @@ -250,6 +262,7 @@ struct typec_operations {
>  			     enum typec_port_type type);
>  	struct usb_power_delivery **(*pd_get)(struct typec_port *port);
>  	int (*pd_set)(struct typec_port *port, struct usb_power_delivery *pd);
> +	int (*default_usb_mode_set)(struct typec_port *port, enum usb_mode mode);

Naming is hard, but usually it's "noun_verb" so wouldn't this be just
"mode_set_default"?  We know it's usb :)

But why default, why not just "mode_set"?  or "set_mode" given you have
"try_role" here, but then you have "pd_set".  Ick, I don't know, it's
your code, so your call, nevermind...

thanks,

greg k-h