On Fri, Oct 11, 2024 at 6:59 AM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
>
> 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.
What's being set underneath is what USB mode to enter on the next
reset or attach -- i.e. the default USB mode to enter. So appropriate
naming here may be one of usb_set_default, usb_set_next. Dropping
"usb" makes less sense vs dropping "mode", which could also refer to
alternate modes so I'd prefer we don't call it mode_set.
>
> thanks,
>
> --
> heikki
