Hi Heikki,
Thanks a lot for reviewing the patch.
On Wed, Jul 01, 2020 at 11:22:30AM +0300, Heikki Krogerus wrote:
> Hi Prashant,
>
> On Tue, Jun 30, 2020 at 05:31:48PM -0700, Prashant Malani wrote:
> > Add a user-visible attribute for the number of alt modes available in a
> > partner. This allows userspace to determine whether there are any
> > remaining alt modes left to be registered by the kernel driver. It can
> > begin implementing any policy state machine after all available alt
> > modes have been registered with the connector class framework.
> >
> > This value is set to "-1" initially, which is an invalid value,
> > signifying that a valid number of alt modes haven't been set for the
> > partner.
> >
> > Signed-off-by: Prashant Malani <pmalani@xxxxxxxxxxxx>
> > Cc: Benson Leung <bleung@xxxxxxxxxxxx>
> > ---
> > drivers/usb/typec/class.c | 34 ++++++++++++++++++++++++++++++++++
> > include/linux/usb/typec.h | 1 +
> > 2 files changed, 35 insertions(+)
>
> You need to add documentation for the file. Add an entry for it to
> Documentation/ABI/testing/sysfs-class-typec
>
Ack.
Will add this in v2.
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index c9234748537a..680cbcfbd427 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -33,6 +33,7 @@ struct typec_partner {
> > struct usb_pd_identity *identity;
> > enum typec_accessory accessory;
> > struct ida mode_ids;
> > + int num_altmodes;
> > };
> >
> > struct typec_port {
> > @@ -532,9 +533,18 @@ static ssize_t supports_usb_power_delivery_show(struct device *dev,
> > }
> > static DEVICE_ATTR_RO(supports_usb_power_delivery);
> >
> > +static ssize_t num_altmodes_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > + struct typec_partner *p = to_typec_partner(dev);
> > +
> > + return sprintf(buf, "%d\n", p->num_altmodes);
> > +}
> > +static DEVICE_ATTR_RO(num_altmodes);
>
> This is up to you, but please consider naming the file
> "number_of_alternate_modes".
>
Ack. Will update the name in v2.
> So now user space needs to understand that value -1 means "unknown",
> right?
>
> If so, then I think it would probable be better to just hide the file
> until the actual number of alternate modes is known.
>
SGTM.
> > static struct attribute *typec_partner_attrs[] = {
> > &dev_attr_accessory_mode.attr,
> > &dev_attr_supports_usb_power_delivery.attr,
> > + &dev_attr_num_altmodes.attr,
> > NULL
> > };
> > ATTRIBUTE_GROUPS(typec_partner);
>
> -ATTRIBUTE_GROUPS(typec_partner);
>
> static umode_t typec_partner_attr_is_visible(struct kobject *kobj,
> struct attribute *attr, int n)
> {
> struct typec_partner *partner = to_typec_partner(kobj_to_dev(kobj));
>
> if (attr == &dev_attr_num_altmodes.attr) {
> if (partner->num_altmodes < 0)
> return 0;
> }
>
> return attr->mode;
> }
>
> static struct attribute_group typec_partner_group = {
> .is_visible = typec_partner_attr_is_visible,
> .attrs = typec_partner_attrs
> };
>
> static const struct attribute_group *typec_partner_groups[] = {
> &typec_partner_group,
> NULL
> };
>
Nice, this is cool. Will make the modification in v2.
Best regards,
> > @@ -570,6 +580,29 @@ int typec_partner_set_identity(struct typec_partner *partner)
> > }
> > EXPORT_SYMBOL_GPL(typec_partner_set_identity);
> >
> > +/**
> > + * typec_partner_set_num_altmodes - Update number of available altmodes
> > + * @partner: The partner to be updated
> > + * @num_alt_modes: The number of altmodes we want to specify as available
> > + *
> > + * This routine is used to report the number of alternate modes supported by the
> > + * partner. This value is *not* enforced in alt mode registration routines.
> > + *
> > + * @partner.num_altmodes is set to -1 on partner registration, denoting that
> > + * a valid value has not been set for it yet.
> > + */
> > +int typec_partner_set_num_altmodes(struct typec_partner *partner, int num_altmodes)
> > +{
> > + if (num_altmodes < 0)
> > + return -EINVAL;
> > +
> > + partner->num_altmodes = num_altmodes;
> > + sysfs_notify(&partner->dev.kobj, NULL, "num_altmodes");
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(typec_partner_set_num_altmodes);
> > +
> > /**
> > * typec_partner_register_altmode - Register USB Type-C Partner Alternate Mode
> > * @partner: USB Type-C Partner that supports the alternate mode
> > @@ -612,6 +645,7 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
> > ida_init(&partner->mode_ids);
> > partner->usb_pd = desc->usb_pd;
> > partner->accessory = desc->accessory;
> > + partner->num_altmodes = -1;
> >
> > if (desc->identity) {
> > /*
> > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> > index 5daa1c49761c..ab523caa23a4 100644
> > --- a/include/linux/usb/typec.h
> > +++ b/include/linux/usb/typec.h
> > @@ -112,6 +112,7 @@ struct typec_altmode_desc {
> > enum typec_port_data roles;
> > };
> >
> > +int typec_partner_set_num_altmodes(struct typec_partner *partner, int num_altmodes);
> > struct typec_altmode
> > *typec_partner_register_altmode(struct typec_partner *partner,
> > const struct typec_altmode_desc *desc);
>
> thanks,
>
> --
> heikki
