| Message ID | 20240123143026.v1.3.Idf7d373c3cbb54058403cb951d644f1f09973d15@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: Adding support for UCSI 3.0 | expand |
Hi Abhishek, Few nitpicks for this one. On Tue, Jan 23, 2024 at 02:30:36PM -0800, Abhishek Pandit-Subedi wrote: > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > PD major revision for the port partner is described in > GET_CONNECTOR_CAPABILITY and is only valid on UCSI 2.0 and newer. Update > the pd_revision on the partner if the UCSI version is 2.0 or newer. > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > --- > $ cat /sys/class/typec/port2-partner/usb_power_delivery_revision > 3.0 > > drivers/usb/typec/ucsi/ucsi.c | 25 +++++++++++++++++++++++++ > drivers/usb/typec/ucsi/ucsi.h | 3 +++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index 4edf785d203b..8e0a512853ba 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -782,6 +782,8 @@ static int ucsi_register_partner(struct ucsi_connector *con) > } > > desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD; > + desc.pd_revision = > + UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags); desc.pd_revision = UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags); > partner = typec_register_partner(con->port, &desc); > if (IS_ERR(partner)) { > @@ -856,6 +858,28 @@ static void ucsi_partner_change(struct ucsi_connector *con) > con->num, u_role); > } > > +static int ucsi_check_connector_capability(struct ucsi_connector *con) > +{ > + u64 command; > + int ret; > + > + if (!con->partner && !IS_MIN_VERSION_2_0(con->ucsi)) > + return 0; > + > + command = UCSI_GET_CONNECTOR_CAPABILITY | UCSI_CONNECTOR_NUMBER(con->num); > + ret = ucsi_send_command(con->ucsi, command, &con->cap, sizeof(con->cap)); > + if (ret < 0) { > + dev_err(con->ucsi->dev, "GET_CONNECTOR_CAPABILITY failed (%d)\n", ret); > + return ret; > + } > + > + typec_partner_set_pd_revision( > + con->partner, > + UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags)); typec_partner_set_pd_revision(con->partner, UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags)); > + > + return ret; > +} > + > static int ucsi_check_connection(struct ucsi_connector *con) > { > u8 prev_flags = con->status.flags; > @@ -925,6 +949,7 @@ static void ucsi_handle_connector_change(struct work_struct *work) > if (con->status.flags & UCSI_CONSTAT_CONNECTED) { > ucsi_register_partner(con); > ucsi_partner_task(con, ucsi_check_connection, 1, HZ); > + ucsi_partner_task(con, ucsi_check_connector_capability, 1, HZ); > > if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == > UCSI_CONSTAT_PWR_OPMODE_PD) > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > index 94b373378f63..5e60328f398e 100644 > --- a/drivers/usb/typec/ucsi/ucsi.h > +++ b/drivers/usb/typec/ucsi/ucsi.h > @@ -36,6 +36,9 @@ struct dentry; > #define UCSI_BCD_GET_MINOR(_v_) (((_v_) >> 4) & 0x0F) > #define UCSI_BCD_GET_SUBMINOR(_v_) ((_v_) & 0x0F) > > +#define IS_MIN_VERSION(ucsi, min_ver) ((ucsi)->version >= min_ver) Probable better to use brackets also with that min_ver: #define IS_MIN_VERSION(ucsi, min_ver) ((ucsi)->version >= (min_ver)) > +#define IS_MIN_VERSION_2_0(ucsi) IS_MIN_VERSION(ucsi, UCSI_VERSION_2_0) > + > /* Command Status and Connector Change Indication (CCI) bits */ > #define UCSI_CCI_CONNECTOR(_c_) (((_c_) & GENMASK(7, 1)) >> 1) > #define UCSI_CCI_LENGTH(_c_) (((_c_) & GENMASK(15, 8)) >> 8) > -- > 2.43.0.429.g432eaa2c6b-goog thanks,
Hi Abhishek, On Tue, Jan 23, 2024 at 2:30 PM Abhishek Pandit-Subedi <abhishekpandit@google.com> wrote: > > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > PD major revision for the port partner is described in > GET_CONNECTOR_CAPABILITY and is only valid on UCSI 2.0 and newer. Update > the pd_revision on the partner if the UCSI version is 2.0 or newer. > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > --- > $ cat /sys/class/typec/port2-partner/usb_power_delivery_revision > 3.0 > > drivers/usb/typec/ucsi/ucsi.c | 25 +++++++++++++++++++++++++ > drivers/usb/typec/ucsi/ucsi.h | 3 +++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index 4edf785d203b..8e0a512853ba 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -782,6 +782,8 @@ static int ucsi_register_partner(struct ucsi_connector *con) > } > > desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD; > + desc.pd_revision = > + UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags); > > partner = typec_register_partner(con->port, &desc); > if (IS_ERR(partner)) { > @@ -856,6 +858,28 @@ static void ucsi_partner_change(struct ucsi_connector *con) > con->num, u_role); > } > > +static int ucsi_check_connector_capability(struct ucsi_connector *con) > +{ > + u64 command; > + int ret; > + > + if (!con->partner && !IS_MIN_VERSION_2_0(con->ucsi)) (Mentioned side-band but reproducing here for consistency) This macro is unnecessary. It's just doing a comparison, which can be inlined without any perceptible change in readability (actually, I'd argue adding the ! to an english idiom makes things *less* readable): if (!con->partner && con->ucsi->version < UCSI_VERSION_2_0) return 0; Besides that, I think you want an || operator instead of the && operator, right? > + return 0; > + > + command = UCSI_GET_CONNECTOR_CAPABILITY | UCSI_CONNECTOR_NUMBER(con->num); > + ret = ucsi_send_command(con->ucsi, command, &con->cap, sizeof(con->cap)); > + if (ret < 0) { > + dev_err(con->ucsi->dev, "GET_CONNECTOR_CAPABILITY failed (%d)\n", ret); > + return ret; > + } > + > + typec_partner_set_pd_revision( > + con->partner, > + UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags)); > + > + return ret; > +} > + > static int ucsi_check_connection(struct ucsi_connector *con) > { > u8 prev_flags = con->status.flags; Thanks,
On Wed, Jan 24, 2024 at 10:49 AM Prashant Malani <pmalani@chromium.org> wrote: > > Hi Abhishek, > > On Tue, Jan 23, 2024 at 2:30 PM Abhishek Pandit-Subedi > <abhishekpandit@google.com> wrote: > > > > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > > PD major revision for the port partner is described in > > GET_CONNECTOR_CAPABILITY and is only valid on UCSI 2.0 and newer. Update > > the pd_revision on the partner if the UCSI version is 2.0 or newer. > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > --- > > $ cat /sys/class/typec/port2-partner/usb_power_delivery_revision > > 3.0 > > > > drivers/usb/typec/ucsi/ucsi.c | 25 +++++++++++++++++++++++++ > > drivers/usb/typec/ucsi/ucsi.h | 3 +++ > > 2 files changed, 28 insertions(+) > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > > index 4edf785d203b..8e0a512853ba 100644 > > --- a/drivers/usb/typec/ucsi/ucsi.c > > +++ b/drivers/usb/typec/ucsi/ucsi.c > > @@ -782,6 +782,8 @@ static int ucsi_register_partner(struct ucsi_connector *con) > > } > > > > desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD; > > + desc.pd_revision = > > + UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags); > > > > partner = typec_register_partner(con->port, &desc); > > if (IS_ERR(partner)) { > > @@ -856,6 +858,28 @@ static void ucsi_partner_change(struct ucsi_connector *con) > > con->num, u_role); > > } > > > > +static int ucsi_check_connector_capability(struct ucsi_connector *con) > > +{ > > + u64 command; > > + int ret; > > + > > + if (!con->partner && !IS_MIN_VERSION_2_0(con->ucsi)) > > (Mentioned side-band but reproducing here for consistency) > This macro is unnecessary. It's just doing a comparison, which can be inlined > without any perceptible change in readability (actually, I'd argue adding the ! > to an english idiom makes things *less* readable): I prefer the macro because it makes it easier to search where version checks are being done and it keeps the `<` vs `<=` consistent. UCSI only has a few published revisions: 1.2, 2.0, 2.1 and 3.0 and major changes seem to have happened in 2.0 and 3.0 so there should be very few of these macros created/used. > > if (!con->partner && con->ucsi->version < UCSI_VERSION_2_0) > return 0; > > Besides that, I think you want an || operator instead of the && operator, right? Good catch on that. It should be OR. i.e. if (!con->partner || !IS_MIN_VERSION_2_0(con->ucsi)) > > > + return 0; > > + > > + command = UCSI_GET_CONNECTOR_CAPABILITY | UCSI_CONNECTOR_NUMBER(con->num); > > + ret = ucsi_send_command(con->ucsi, command, &con->cap, sizeof(con->cap)); > > + if (ret < 0) { > > + dev_err(con->ucsi->dev, "GET_CONNECTOR_CAPABILITY failed (%d)\n", ret); > > + return ret; > > + } > > + > > + typec_partner_set_pd_revision( > > + con->partner, > > + UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags)); > > + > > + return ret; > > +} > > + > > static int ucsi_check_connection(struct ucsi_connector *con) > > { > > u8 prev_flags = con->status.flags; > > Thanks,
On Wed, Jan 24, 2024 at 11:18 AM Abhishek Pandit-Subedi <abhishekpandit@chromium.org> wrote: > > On Wed, Jan 24, 2024 at 10:49 AM Prashant Malani <pmalani@chromium.org> wrote: > > > > Hi Abhishek, > > > > On Tue, Jan 23, 2024 at 2:30 PM Abhishek Pandit-Subedi > > <abhishekpandit@google.com> wrote: > > > > > > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > > > > PD major revision for the port partner is described in > > > GET_CONNECTOR_CAPABILITY and is only valid on UCSI 2.0 and newer. Update > > > the pd_revision on the partner if the UCSI version is 2.0 or newer. > > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > --- > > > $ cat /sys/class/typec/port2-partner/usb_power_delivery_revision > > > 3.0 > > > > > > drivers/usb/typec/ucsi/ucsi.c | 25 +++++++++++++++++++++++++ > > > drivers/usb/typec/ucsi/ucsi.h | 3 +++ > > > 2 files changed, 28 insertions(+) > > > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > > > index 4edf785d203b..8e0a512853ba 100644 > > > --- a/drivers/usb/typec/ucsi/ucsi.c > > > +++ b/drivers/usb/typec/ucsi/ucsi.c > > > @@ -782,6 +782,8 @@ static int ucsi_register_partner(struct ucsi_connector *con) > > > } > > > > > > desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD; > > > + desc.pd_revision = > > > + UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags); > > > > > > partner = typec_register_partner(con->port, &desc); > > > if (IS_ERR(partner)) { > > > @@ -856,6 +858,28 @@ static void ucsi_partner_change(struct ucsi_connector *con) > > > con->num, u_role); > > > } > > > > > > +static int ucsi_check_connector_capability(struct ucsi_connector *con) > > > +{ > > > + u64 command; > > > + int ret; > > > + > > > + if (!con->partner && !IS_MIN_VERSION_2_0(con->ucsi)) > > > > (Mentioned side-band but reproducing here for consistency) > > This macro is unnecessary. It's just doing a comparison, which can be inlined > > without any perceptible change in readability (actually, I'd argue adding the ! > > to an english idiom makes things *less* readable): > > I prefer the macro because it makes it easier to search where version > checks are being done. I don't see how searching for "IS_MIN_VERSION_2_0" is easier than just searching for "UCSI_VERSION_2_0". I didn't quite understand what you meant by > it keeps the `<` vs `<=` consistent. Perhaps I'm missing something... (are these comparisons being used elsewhere/in some other fashion?). In any case, I don't want to bike-shed so I'll defer to the maintainer's call on this. BR, -Prashant
On Wed, Jan 24, 2024 at 11:34 AM Prashant Malani <pmalani@chromium.org> wrote: > > On Wed, Jan 24, 2024 at 11:18 AM Abhishek Pandit-Subedi > <abhishekpandit@chromium.org> wrote: > > > > On Wed, Jan 24, 2024 at 10:49 AM Prashant Malani <pmalani@chromium.org> wrote: > > > > > > Hi Abhishek, > > > > > > On Tue, Jan 23, 2024 at 2:30 PM Abhishek Pandit-Subedi > > > <abhishekpandit@google.com> wrote: > > > > > > > > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > > > > > > PD major revision for the port partner is described in > > > > GET_CONNECTOR_CAPABILITY and is only valid on UCSI 2.0 and newer. Update > > > > the pd_revision on the partner if the UCSI version is 2.0 or newer. > > > > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > > --- > > > > $ cat /sys/class/typec/port2-partner/usb_power_delivery_revision > > > > 3.0 > > > > > > > > drivers/usb/typec/ucsi/ucsi.c | 25 +++++++++++++++++++++++++ > > > > drivers/usb/typec/ucsi/ucsi.h | 3 +++ > > > > 2 files changed, 28 insertions(+) > > > > > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > > > > index 4edf785d203b..8e0a512853ba 100644 > > > > --- a/drivers/usb/typec/ucsi/ucsi.c > > > > +++ b/drivers/usb/typec/ucsi/ucsi.c > > > > @@ -782,6 +782,8 @@ static int ucsi_register_partner(struct ucsi_connector *con) > > > > } > > > > > > > > desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD; > > > > + desc.pd_revision = > > > > + UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags); > > > > > > > > partner = typec_register_partner(con->port, &desc); > > > > if (IS_ERR(partner)) { > > > > @@ -856,6 +858,28 @@ static void ucsi_partner_change(struct ucsi_connector *con) > > > > con->num, u_role); > > > > } > > > > > > > > +static int ucsi_check_connector_capability(struct ucsi_connector *con) > > > > +{ > > > > + u64 command; > > > > + int ret; > > > > + > > > > + if (!con->partner && !IS_MIN_VERSION_2_0(con->ucsi)) > > > > > > (Mentioned side-band but reproducing here for consistency) > > > This macro is unnecessary. It's just doing a comparison, which can be inlined > > > without any perceptible change in readability (actually, I'd argue adding the ! > > > to an english idiom makes things *less* readable): > > > > I prefer the macro because it makes it easier to search where version > > checks are being done. > > I don't see how searching for "IS_MIN_VERSION_2_0" is easier > than just searching for "UCSI_VERSION_2_0". > > I didn't quite understand what you meant by > > > it keeps the `<` vs `<=` consistent. > > Perhaps I'm missing something... (are these comparisons being > used elsewhere/in some other fashion?). Let's say someone wants to guard code for UCSI 2.0. Should they use: // Guard against older versions. if (ucsi->version < UCSI_VERSION_2_0) return; // This also guards since the version jumps from 1.2 to 2.0. if (ucsi->version <= UCSI_VERSION_1_2) return; // Only do something on newer versions. if (ucsi->version >= UCSI_VERSION_2_0) { // Fill out something available in newer spec. } I'd rather everyone just use a macro that normalizes comparisons. It's always IS_MIN_VERSION and its inverse !IS_MIN_VERSION. It's personal preference so deferring to the maintainer is IMO the right call here. > > In any case, I don't want to bike-shed so I'll defer to the > maintainer's call on this. > > BR, > > -Prashant
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index 4edf785d203b..8e0a512853ba 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -782,6 +782,8 @@ static int ucsi_register_partner(struct ucsi_connector *con) } desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD; + desc.pd_revision = + UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags); partner = typec_register_partner(con->port, &desc); if (IS_ERR(partner)) { @@ -856,6 +858,28 @@ static void ucsi_partner_change(struct ucsi_connector *con) con->num, u_role); } +static int ucsi_check_connector_capability(struct ucsi_connector *con) +{ + u64 command; + int ret; + + if (!con->partner && !IS_MIN_VERSION_2_0(con->ucsi)) + return 0; + + command = UCSI_GET_CONNECTOR_CAPABILITY | UCSI_CONNECTOR_NUMBER(con->num); + ret = ucsi_send_command(con->ucsi, command, &con->cap, sizeof(con->cap)); + if (ret < 0) { + dev_err(con->ucsi->dev, "GET_CONNECTOR_CAPABILITY failed (%d)\n", ret); + return ret; + } + + typec_partner_set_pd_revision( + con->partner, + UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags)); + + return ret; +} + static int ucsi_check_connection(struct ucsi_connector *con) { u8 prev_flags = con->status.flags; @@ -925,6 +949,7 @@ static void ucsi_handle_connector_change(struct work_struct *work) if (con->status.flags & UCSI_CONSTAT_CONNECTED) { ucsi_register_partner(con); ucsi_partner_task(con, ucsi_check_connection, 1, HZ); + ucsi_partner_task(con, ucsi_check_connector_capability, 1, HZ); if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == UCSI_CONSTAT_PWR_OPMODE_PD) diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h index 94b373378f63..5e60328f398e 100644 --- a/drivers/usb/typec/ucsi/ucsi.h +++ b/drivers/usb/typec/ucsi/ucsi.h @@ -36,6 +36,9 @@ struct dentry; #define UCSI_BCD_GET_MINOR(_v_) (((_v_) >> 4) & 0x0F) #define UCSI_BCD_GET_SUBMINOR(_v_) ((_v_) & 0x0F) +#define IS_MIN_VERSION(ucsi, min_ver) ((ucsi)->version >= min_ver) +#define IS_MIN_VERSION_2_0(ucsi) IS_MIN_VERSION(ucsi, UCSI_VERSION_2_0) + /* Command Status and Connector Change Indication (CCI) bits */ #define UCSI_CCI_CONNECTOR(_c_) (((_c_) & GENMASK(7, 1)) >> 1) #define UCSI_CCI_LENGTH(_c_) (((_c_) & GENMASK(15, 8)) >> 8)