Hi Heikki, Friendly ping to review this patch (I see you added Reviewed-by to the other two in this series). Thanks, Abhishek On Fri, Jan 26, 2024 at 12:25 PM Prashant Malani <pmalani@xxxxxxxxxxxx> wrote: > > Hi Abhishek, > > On Fri, Jan 26, 2024 at 10:39 AM Abhishek Pandit-Subedi > <abhishekpandit@xxxxxxxxxxxx> wrote: > > > > 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@xxxxxxxxxxxx> > > --- > > $ cat /sys/class/typec/port2-partner/usb_power_delivery_revision > > 3.0 > > > > (no changes since v2) > > > > Changes in v2: > > - Formatting changes and update macro to use brackets. > > - Fix incorrect guard condition when checking connector capability. > > > > drivers/usb/typec/ucsi/ucsi.c | 23 +++++++++++++++++++++++ > > drivers/usb/typec/ucsi/ucsi.h | 3 +++ > > 2 files changed, 26 insertions(+) > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > > index a35056ee3e96..2b7983d2fdae 100644 > > --- a/drivers/usb/typec/ucsi/ucsi.c > > +++ b/drivers/usb/typec/ucsi/ucsi.c > > @@ -782,6 +782,7 @@ 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 +857,27 @@ 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)) > > I'll reiterate my comment from a previous version, since this series > has been revv-ed a few > times since and it may have gotten lost; no need to respond to it if > you don't want to, > since I believe we left it to the maintainer(s) to decide [1]: > > This macro is unnecessary. Since the version is in BCD format and we > already have the > macros for versions, just a simple comparison is enough: > if (!con-partner || con->ucsi->version < UCSI_VERSION_2_0) > return 0; > > I'll add that Patch 1 of this series [2] is also using the same style > for comparing version numbers. > > > + 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); > > nit: I know this is being done elsewhere in this file, but we should > avoid putting error > numbers in parentheses [3]. Perhaps something for a separate cleanup patch. > > [1] https://lore.kernel.org/linux-usb/CANFp7mXP=aN8bQi4akKKcoMZE8RaCBuFnwTa5hbp0MZvZe0hYQ@xxxxxxxxxxxxxx/ > [2] https://lore.kernel.org/linux-usb/20240126103859.v3.1.Iacf5570a66b82b73ef03daa6557e2fc0db10266a@changeid/ > [3] https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages