| Message ID | 20240126103859.v3.3.Idf7d373c3cbb54058403cb951d644f1f09973d15@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: Adding support for UCSI 3.0 | expand |
Hi Abhishek, On Fri, Jan 26, 2024 at 10:39 AM Abhishek Pandit-Subedi <abhishekpandit@chromium.org> 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@chromium.org> > --- > $ 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@mail.gmail.com/ [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
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@chromium.org> wrote: > > Hi Abhishek, > > On Fri, Jan 26, 2024 at 10:39 AM Abhishek Pandit-Subedi > <abhishekpandit@chromium.org> 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@chromium.org> > > --- > > $ 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@mail.gmail.com/ > [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
Hi Abhishek, On Mon, Feb 05, 2024 at 02:05:38PM -0800, Abhishek Pandit-Subedi wrote: > Hi Heikki, > > Friendly ping to review this patch (I see you added Reviewed-by to the > other two in this series). I think Prashant said that he prefers macros with those version checks, and I kinda agree. But I'll leave this to you to decide. I think that's also something that can be improved later. > On Fri, Jan 26, 2024 at 12:25 PM Prashant Malani <pmalani@chromium.org> wrote: > > > > Hi Abhishek, > > > > On Fri, Jan 26, 2024 at 10:39 AM Abhishek Pandit-Subedi > > <abhishekpandit@chromium.org> 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@chromium.org> So this okay by me: Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > --- > > > $ 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@mail.gmail.com/ > > [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 thanks,
On Tue, Feb 6, 2024 at 2:18 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > Hi Abhishek, > > On Mon, Feb 05, 2024 at 02:05:38PM -0800, Abhishek Pandit-Subedi wrote: > > Hi Heikki, > > > > Friendly ping to review this patch (I see you added Reviewed-by to the > > other two in this series). > > I think Prashant said that he prefers macros with those version checks, > and I kinda agree. But I'll leave this to you to decide. I think > that's also something that can be improved later. Yeah, the macro strikes me as unnecessary here. Anyhow, for the rest of it: Reviewed-by: Prashant Malani <pmalani@chromium.org>
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)) + 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 +947,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 1bae4cf8ecdc..d1d0e11b0704 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)
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 (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(+)