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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Fri, Oct 11, 2024 at 03:07:21PM +0200, Greg Kroah-Hartman wrote:
> 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 :)

Yes. I'm sorry about that.

> > +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()?

Yes.

> > @@ -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...

I think I'll just change it this back to the way it was in the last
version, and drop "default" from the name.

thanks,

-- 
heikki