| Message ID | 20240123223039.1471557-1-abhishekpandit@google.com |
|---|---|
| Headers | show |
| Series | usb: typec: ucsi: Adding support for UCSI 3.0 | expand |
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,
Ack. Will make dev_dbg on the next iteration. This seems like a good addition to the style guide too: https://www.kernel.org/doc/html/v6.7/process/coding-style.html#printing-kernel-messages. "When drivers are working properly, they are quiet. Prefer to use DEBUG messages unless something is wrong." What do you think Greg? On Wed, Jan 24, 2024 at 6:17 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Jan 24, 2024 at 03:51:58PM +0200, Heikki Krogerus wrote: > > On Wed, Jan 24, 2024 at 12:12:26AM -0800, Prashant Malani 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> > > > > > > > > Between UCSI 1.2 and UCSI 2.0, the size of the MESSAGE_IN region was > > > > increased from 16 to 256. In order to avoid overflowing reads for older > > > > systems, add a mechanism to use the read UCSI version to truncate read > > > > sizes on UCSI v1.2. > > > > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > I have one nit (mentioned in side-band but reproducing here for consistency), > > > but will defer to the maintainer on that. > > > > > > The above notwithstanding, FWIW: > > > Reviewed-by: Prashant Malani<pmalani@chromium.org> > > > > > > > @@ -1556,6 +1569,15 @@ int ucsi_register(struct ucsi *ucsi) > > > > if (!ucsi->version) > > > > return -ENODEV; > > > > > > > > + /* > > > > + * Version format is JJ.M.N (JJ = Major version, M = Minor version, > > > > + * N = sub-minor version). > > > > + */ > > > > + dev_info(ucsi->dev, "Registered UCSI interface with version %x.%x.%x", > > > > + UCSI_BCD_GET_MAJOR(ucsi->version), > > > > + UCSI_BCD_GET_MINOR(ucsi->version), > > > > + UCSI_BCD_GET_SUBMINOR(ucsi->version)); > > > > > > nit: I think this doesn't need to be dev_info() and can be just > > > dev_dbg(), but will > > > defer to the maintainer. > > > > I think that's okay. > > > > Reviewewd-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > No, when drivers are working properly they are quiet, this needs to be > dev_dbg(). > > thanks, > > greg k-h >
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
On Wed, Jan 24, 2024 at 10:59:28AM -0800, Abhishek Pandit-Subedi wrote: > Ack. Will make dev_dbg on the next iteration. > > This seems like a good addition to the style guide too: > https://www.kernel.org/doc/html/v6.7/process/coding-style.html#printing-kernel-messages. > "When drivers are working properly, they are quiet. Prefer to use > DEBUG messages unless something is wrong." > > What do you think Greg? I think you need to stop top-posting :) But yes, that would be nice, hopefully people actually notice it there. Would you have read this and seen it? thanks, greg k-h
On Thu, Jan 25, 2024 at 3:16 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Jan 24, 2024 at 10:59:28AM -0800, Abhishek Pandit-Subedi wrote: > > Ack. Will make dev_dbg on the next iteration. > > > > This seems like a good addition to the style guide too: > > https://www.kernel.org/doc/html/v6.7/process/coding-style.html#printing-kernel-messages. > > "When drivers are working properly, they are quiet. Prefer to use > > DEBUG messages unless something is wrong." > > > > What do you think Greg? > > I think you need to stop top-posting :) > > But yes, that would be nice, hopefully people actually notice it there. > Would you have read this and seen it? > > thanks, > > greg k-h I blame gmail web-interface for the top-posting :) Prashant also mentioned the dev_info when we were reviewing this so I did a quick search for "kernel coding style" (to see if there was guidance) before sending this up so I would definitely have noticed it there. In Bluetooth (where I've previously contributed), dev_info is used for printing version info (example: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/bluetooth/btintel.c?h=v6.8-rc1#n337) and it's very useful for debugging so I assumed it was acceptable. The added benefit of this guidance being in the coding style guide is it's a quick change to checkpatch.pl to add a warning for this (and point to the coding style as the source). I'm fairly sure Chromium actually has a lint that warns whenever you use LOG_INFO(...) for similar reasons (too many INFO messages that should be DEBUG). I will send up a patch with the change soon (both to coding style and checkpatch.pl). Thanks, Abhishek
From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> Hi Heikki, This series starts the work adding UCSI 3.0 support to the UCSI driver. There's a couple of pieces to start here: * Add version checks and limit read size on 1.2. * Update Connector Status and Connector Capability structures. * Expose Partner PD revision from Capability data. These were tested against on a 6.6 kernel running a usermode PPM against a Realtek Evaluation board. One additional note: there are a lot more unaligned fields in UCSI now and the struct definitions are getting a bit out of hand. We can discuss alternate mechanisms for defining these structs in the patch that changes these structures. Thanks, Abhishek Abhishek Pandit-Subedi (3): usb: typec: ucsi: Limit read size on v1.2 usb: typec: ucsi: Update connector cap and status usb: typec: ucsi: Get PD revision for partner drivers/usb/typec/ucsi/ucsi.c | 51 ++++++++++++++++++++++++++-- drivers/usb/typec/ucsi/ucsi.h | 64 ++++++++++++++++++++++++++++++++--- 2 files changed, 109 insertions(+), 6 deletions(-)