* [RFC PATCH v2 0/2] usb: typec: ucsi: New way to handle GET_CONNECTOR_STATUS @ 2024-09-03 14:53 Heikki Krogerus 2024-09-03 14:53 ` [RFC PATCH v2 1/2] usb: typec: ucsi: Helper for Get Connector Status command Heikki Krogerus 2024-09-03 14:53 ` [RFC PATCH v2 2/2] usb: typec: ucsi: Convert connector specific commands to bitmaps Heikki Krogerus 0 siblings, 2 replies; 8+ messages in thread From: Heikki Krogerus @ 2024-09-03 14:53 UTC (permalink / raw) To: Abhishek Pandit-Subedi Cc: Jameson Thies, Benson Leung, Bartosz Szpila, Pilla, Siva sai kumar, linux-usb Hi, This is a bit better version of the idea. I'm now using the bitmap API, but there are actually not that many other changes. I converted GET_CONNECTOR_CAP and GET_CONNECTOR_STATUS. The original version: https://lore.kernel.org/linux-usb/20240828161502.2774996-1-heikki.krogerus@linux.intel.com/ thanks, Heikki Krogerus (2): usb: typec: ucsi: Helper for Get Connector Status command usb: typec: ucsi: Convert connector specific commands to bitmaps drivers/usb/typec/ucsi/psy.c | 28 ++--- drivers/usb/typec/ucsi/trace.h | 28 ++--- drivers/usb/typec/ucsi/ucsi.c | 135 ++++++++++---------- drivers/usb/typec/ucsi/ucsi.h | 195 ++++++++++++++++------------- drivers/usb/typec/ucsi/ucsi_acpi.c | 7 +- 5 files changed, 206 insertions(+), 187 deletions(-) -- 2.45.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC PATCH v2 1/2] usb: typec: ucsi: Helper for Get Connector Status command 2024-09-03 14:53 [RFC PATCH v2 0/2] usb: typec: ucsi: New way to handle GET_CONNECTOR_STATUS Heikki Krogerus @ 2024-09-03 14:53 ` Heikki Krogerus 2024-09-04 5:17 ` Abhishek Pandit-Subedi 2024-09-12 10:08 ` Dmitry Baryshkov 2024-09-03 14:53 ` [RFC PATCH v2 2/2] usb: typec: ucsi: Convert connector specific commands to bitmaps Heikki Krogerus 1 sibling, 2 replies; 8+ messages in thread From: Heikki Krogerus @ 2024-09-03 14:53 UTC (permalink / raw) To: Abhishek Pandit-Subedi Cc: Jameson Thies, Benson Leung, Bartosz Szpila, Pilla, Siva sai kumar, linux-usb That command is executed from several locations in the driver, so providing a dedicated helper function for it. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/usb/typec/ucsi/ucsi.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index 35dce4057c25..71cf64a424d9 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -646,6 +646,16 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient) } } +static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack) +{ + u64 command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num); + struct ucsi *ucsi = con->ucsi; + int ret; + + ret = ucsi_send_command_common(ucsi, command, &con->status, sizeof(con->status), conn_ack); + return ret < 0 ? ret : 0; +} + static int ucsi_read_pdos(struct ucsi_connector *con, enum typec_role role, int is_partner, u32 *pdos, int offset, int num_pdos) @@ -1123,12 +1133,10 @@ static void ucsi_partner_change(struct ucsi_connector *con) static int ucsi_check_connection(struct ucsi_connector *con) { u8 prev_flags = con->status.flags; - u64 command; int ret; - command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num); - ret = ucsi_send_command(con->ucsi, command, &con->status, sizeof(con->status)); - if (ret < 0) { + ret = ucsi_get_connector_status(con, false); + if (ret) { dev_err(con->ucsi->dev, "GET_CONNECTOR_STATUS failed (%d)\n", ret); return ret; } @@ -1192,16 +1200,12 @@ static void ucsi_handle_connector_change(struct work_struct *work) work); struct ucsi *ucsi = con->ucsi; enum typec_role role; - u64 command; int ret; mutex_lock(&con->lock); - command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num); - - ret = ucsi_send_command_common(ucsi, command, &con->status, - sizeof(con->status), true); - if (ret < 0) { + ret = ucsi_get_connector_status(con, true); + if (ret) { dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n", __func__, ret); clear_bit(EVENT_PENDING, &con->ucsi->flags); @@ -1611,14 +1615,11 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) } /* Get the status */ - command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num); - ret = ucsi_send_command(ucsi, command, &con->status, sizeof(con->status)); - if (ret < 0) { + ret = ucsi_get_connector_status(con, false); + if (ret) { dev_err(ucsi->dev, "con%d: failed to get status\n", con->num); - ret = 0; goto out; } - ret = 0; /* ucsi_send_command() returns length on success */ if (ucsi->ops->connector_status) ucsi->ops->connector_status(con); -- 2.45.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v2 1/2] usb: typec: ucsi: Helper for Get Connector Status command 2024-09-03 14:53 ` [RFC PATCH v2 1/2] usb: typec: ucsi: Helper for Get Connector Status command Heikki Krogerus @ 2024-09-04 5:17 ` Abhishek Pandit-Subedi 2024-09-12 10:08 ` Dmitry Baryshkov 1 sibling, 0 replies; 8+ messages in thread From: Abhishek Pandit-Subedi @ 2024-09-04 5:17 UTC (permalink / raw) To: Heikki Krogerus Cc: Jameson Thies, Benson Leung, Bartosz Szpila, Pilla, Siva sai kumar, linux-usb On Tue, Sep 3, 2024 at 7:53 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > That command is executed from several locations in the > driver, so providing a dedicated helper function for it. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/ucsi/ucsi.c | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index 35dce4057c25..71cf64a424d9 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -646,6 +646,16 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient) > } > } > > +static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack) > +{ > + u64 command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num); > + struct ucsi *ucsi = con->ucsi; > + int ret; > + > + ret = ucsi_send_command_common(ucsi, command, &con->status, sizeof(con->status), conn_ack); > + return ret < 0 ? ret : 0; > +} > + > static int ucsi_read_pdos(struct ucsi_connector *con, > enum typec_role role, int is_partner, > u32 *pdos, int offset, int num_pdos) > @@ -1123,12 +1133,10 @@ static void ucsi_partner_change(struct ucsi_connector *con) > static int ucsi_check_connection(struct ucsi_connector *con) > { > u8 prev_flags = con->status.flags; > - u64 command; > int ret; > > - command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num); > - ret = ucsi_send_command(con->ucsi, command, &con->status, sizeof(con->status)); > - if (ret < 0) { > + ret = ucsi_get_connector_status(con, false); > + if (ret) { > dev_err(con->ucsi->dev, "GET_CONNECTOR_STATUS failed (%d)\n", ret); > return ret; > } > @@ -1192,16 +1200,12 @@ static void ucsi_handle_connector_change(struct work_struct *work) > work); > struct ucsi *ucsi = con->ucsi; > enum typec_role role; > - u64 command; > int ret; > > mutex_lock(&con->lock); > > - command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num); > - > - ret = ucsi_send_command_common(ucsi, command, &con->status, > - sizeof(con->status), true); > - if (ret < 0) { > + ret = ucsi_get_connector_status(con, true); > + if (ret) { > dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n", > __func__, ret); > clear_bit(EVENT_PENDING, &con->ucsi->flags); > @@ -1611,14 +1615,11 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > } > > /* Get the status */ > - command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num); > - ret = ucsi_send_command(ucsi, command, &con->status, sizeof(con->status)); > - if (ret < 0) { > + ret = ucsi_get_connector_status(con, false); > + if (ret) { > dev_err(ucsi->dev, "con%d: failed to get status\n", con->num); > - ret = 0; > goto out; > } > - ret = 0; /* ucsi_send_command() returns length on success */ > > if (ucsi->ops->connector_status) > ucsi->ops->connector_status(con); > -- > 2.45.2 > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v2 1/2] usb: typec: ucsi: Helper for Get Connector Status command 2024-09-03 14:53 ` [RFC PATCH v2 1/2] usb: typec: ucsi: Helper for Get Connector Status command Heikki Krogerus 2024-09-04 5:17 ` Abhishek Pandit-Subedi @ 2024-09-12 10:08 ` Dmitry Baryshkov 1 sibling, 0 replies; 8+ messages in thread From: Dmitry Baryshkov @ 2024-09-12 10:08 UTC (permalink / raw) To: Heikki Krogerus Cc: Abhishek Pandit-Subedi, Jameson Thies, Benson Leung, Bartosz Szpila, Pilla, Siva sai kumar, linux-usb On Tue, Sep 03, 2024 at 05:53:41PM GMT, Heikki Krogerus wrote: > That command is executed from several locations in the > driver, so providing a dedicated helper function for it. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/ucsi/ucsi.c | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC PATCH v2 2/2] usb: typec: ucsi: Convert connector specific commands to bitmaps 2024-09-03 14:53 [RFC PATCH v2 0/2] usb: typec: ucsi: New way to handle GET_CONNECTOR_STATUS Heikki Krogerus 2024-09-03 14:53 ` [RFC PATCH v2 1/2] usb: typec: ucsi: Helper for Get Connector Status command Heikki Krogerus @ 2024-09-03 14:53 ` Heikki Krogerus 2024-09-04 5:16 ` Abhishek Pandit-Subedi 2024-09-12 12:04 ` Dmitry Baryshkov 1 sibling, 2 replies; 8+ messages in thread From: Heikki Krogerus @ 2024-09-03 14:53 UTC (permalink / raw) To: Abhishek Pandit-Subedi Cc: Jameson Thies, Benson Leung, Bartosz Szpila, Pilla, Siva sai kumar, linux-usb This is hopefully a bit less hacky, but it still needs work. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/usb/typec/ucsi/psy.c | 28 ++--- drivers/usb/typec/ucsi/trace.h | 28 ++--- drivers/usb/typec/ucsi/ucsi.c | 108 ++++++++-------- drivers/usb/typec/ucsi/ucsi.h | 195 ++++++++++++++++------------- drivers/usb/typec/ucsi/ucsi_acpi.c | 7 +- 5 files changed, 192 insertions(+), 174 deletions(-) diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c index e623d80e177c..4d5398afa7a2 100644 --- a/drivers/usb/typec/ucsi/psy.c +++ b/drivers/usb/typec/ucsi/psy.c @@ -55,8 +55,8 @@ static int ucsi_psy_get_online(struct ucsi_connector *con, union power_supply_propval *val) { val->intval = UCSI_PSY_OFFLINE; - if (con->status.flags & UCSI_CONSTAT_CONNECTED && - (con->status.flags & UCSI_CONSTAT_PWR_DIR) == TYPEC_SINK) + if (UCSI_CONSTAT(con, CONNECTED) && + (UCSI_CONSTAT(con, PWR_DIR) == TYPEC_SINK)) val->intval = UCSI_PSY_FIXED_ONLINE; return 0; } @@ -66,7 +66,7 @@ static int ucsi_psy_get_voltage_min(struct ucsi_connector *con, { u32 pdo; - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { case UCSI_CONSTAT_PWR_OPMODE_PD: pdo = con->src_pdos[0]; val->intval = pdo_fixed_voltage(pdo) * 1000; @@ -89,7 +89,7 @@ static int ucsi_psy_get_voltage_max(struct ucsi_connector *con, { u32 pdo; - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { case UCSI_CONSTAT_PWR_OPMODE_PD: if (con->num_pdos > 0) { pdo = con->src_pdos[con->num_pdos - 1]; @@ -117,7 +117,7 @@ static int ucsi_psy_get_voltage_now(struct ucsi_connector *con, int index; u32 pdo; - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { case UCSI_CONSTAT_PWR_OPMODE_PD: index = rdo_index(con->rdo); if (index > 0) { @@ -145,7 +145,7 @@ static int ucsi_psy_get_current_max(struct ucsi_connector *con, { u32 pdo; - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { case UCSI_CONSTAT_PWR_OPMODE_PD: if (con->num_pdos > 0) { pdo = con->src_pdos[con->num_pdos - 1]; @@ -173,9 +173,7 @@ static int ucsi_psy_get_current_max(struct ucsi_connector *con, static int ucsi_psy_get_current_now(struct ucsi_connector *con, union power_supply_propval *val) { - u16 flags = con->status.flags; - - if (UCSI_CONSTAT_PWR_OPMODE(flags) == UCSI_CONSTAT_PWR_OPMODE_PD) + if (UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) val->intval = rdo_op_current(con->rdo) * 1000; else val->intval = 0; @@ -185,11 +183,9 @@ static int ucsi_psy_get_current_now(struct ucsi_connector *con, static int ucsi_psy_get_usb_type(struct ucsi_connector *con, union power_supply_propval *val) { - u16 flags = con->status.flags; - val->intval = POWER_SUPPLY_USB_TYPE_C; - if (flags & UCSI_CONSTAT_CONNECTED && - UCSI_CONSTAT_PWR_OPMODE(flags) == UCSI_CONSTAT_PWR_OPMODE_PD) + if (UCSI_CONSTAT(con, CONNECTED) && + UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) val->intval = POWER_SUPPLY_USB_TYPE_PD; return 0; @@ -197,18 +193,18 @@ static int ucsi_psy_get_usb_type(struct ucsi_connector *con, static int ucsi_psy_get_charge_type(struct ucsi_connector *con, union power_supply_propval *val) { - if (!(con->status.flags & UCSI_CONSTAT_CONNECTED)) { + if (!(UCSI_CONSTAT(con, CONNECTED))) { val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE; return 0; } /* The Battery Charging Cabability Status field is only valid in sink role. */ - if ((con->status.flags & UCSI_CONSTAT_PWR_DIR) != TYPEC_SINK) { + if (UCSI_CONSTAT(con, PWR_DIR) != TYPEC_SINK) { val->intval = POWER_SUPPLY_CHARGE_TYPE_UNKNOWN; return 0; } - switch (UCSI_CONSTAT_BC_STATUS(con->status.pwr_status)) { + switch (UCSI_CONSTAT(con, BC_STATUS)) { case UCSI_CONSTAT_BC_NOMINAL_CHARGING: val->intval = POWER_SUPPLY_CHARGE_TYPE_STANDARD; break; diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h index a0d3a934d3d9..41701dee7056 100644 --- a/drivers/usb/typec/ucsi/trace.h +++ b/drivers/usb/typec/ucsi/trace.h @@ -40,8 +40,8 @@ DEFINE_EVENT(ucsi_log_command, ucsi_reset_ppm, ); DECLARE_EVENT_CLASS(ucsi_log_connector_status, - TP_PROTO(int port, struct ucsi_connector_status *status), - TP_ARGS(port, status), + TP_PROTO(int port, struct ucsi_connector *con), + TP_ARGS(port, con), TP_STRUCT__entry( __field(int, port) __field(u16, change) @@ -55,14 +55,14 @@ DECLARE_EVENT_CLASS(ucsi_log_connector_status, ), TP_fast_assign( __entry->port = port - 1; - __entry->change = status->change; - __entry->opmode = UCSI_CONSTAT_PWR_OPMODE(status->flags); - __entry->connected = !!(status->flags & UCSI_CONSTAT_CONNECTED); - __entry->pwr_dir = !!(status->flags & UCSI_CONSTAT_PWR_DIR); - __entry->partner_flags = UCSI_CONSTAT_PARTNER_FLAGS(status->flags); - __entry->partner_type = UCSI_CONSTAT_PARTNER_TYPE(status->flags); - __entry->request_data_obj = status->request_data_obj; - __entry->bc_status = UCSI_CONSTAT_BC_STATUS(status->pwr_status); + __entry->change = UCSI_CONSTAT(con, CHANGE); + __entry->opmode = UCSI_CONSTAT(con, PWR_OPMODE); + __entry->connected = UCSI_CONSTAT(con, CONNECTED); + __entry->pwr_dir = UCSI_CONSTAT(con, PWR_DIR); + __entry->partner_flags = UCSI_CONSTAT(con, PARTNER_FLAGS); + __entry->partner_type = UCSI_CONSTAT(con, PARTNER_TYPE); + __entry->request_data_obj = UCSI_CONSTAT(con, RDO); + __entry->bc_status = UCSI_CONSTAT(con, BC_STATUS); ), TP_printk("port%d status: change=%04x, opmode=%x, connected=%d, " "sourcing=%d, partner_flags=%x, partner_type=%x, " @@ -73,13 +73,13 @@ DECLARE_EVENT_CLASS(ucsi_log_connector_status, ); DEFINE_EVENT(ucsi_log_connector_status, ucsi_connector_change, - TP_PROTO(int port, struct ucsi_connector_status *status), - TP_ARGS(port, status) + TP_PROTO(int port, struct ucsi_connector *con), + TP_ARGS(port, con) ); DEFINE_EVENT(ucsi_log_connector_status, ucsi_register_port, - TP_PROTO(int port, struct ucsi_connector_status *status), - TP_ARGS(port, status) + TP_PROTO(int port, struct ucsi_connector *con), + TP_ARGS(port, con) ); DECLARE_EVENT_CLASS(ucsi_log_register_altmode, diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index 71cf64a424d9..b03eace5a8f0 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -649,10 +649,11 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient) static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack) { u64 command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num); - struct ucsi *ucsi = con->ucsi; + size_t size = min(UCSI_GET_CONNECTOR_STATUS_SIZE, UCSI_MAX_DATA_LENGTH(con->ucsi)); int ret; - ret = ucsi_send_command_common(ucsi, command, &con->status, sizeof(con->status), conn_ack); + ret = ucsi_send_command_common(con->ucsi, command, &con->status, size, conn_ack); + return ret < 0 ? ret : 0; } @@ -666,8 +667,7 @@ static int ucsi_read_pdos(struct ucsi_connector *con, if (is_partner && ucsi->quirks & UCSI_NO_PARTNER_PDOS && - ((con->status.flags & UCSI_CONSTAT_PWR_DIR) || - !is_source(role))) + (UCSI_CONSTAT(con, PWR_DIR) || !is_source(role))) return 0; command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num); @@ -995,16 +995,16 @@ static int ucsi_check_connector_capability(struct ucsi_connector *con) } typec_partner_set_pd_revision(con->partner, - UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags)); + UCSI_SPEC_REVISION_TO_BCD(UCSI_CONCAP(con, PARTNER_PD_REVISION))); return ret; } static void ucsi_pwr_opmode_change(struct ucsi_connector *con) { - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { case UCSI_CONSTAT_PWR_OPMODE_PD: - con->rdo = con->status.request_data_obj; + con->rdo = UCSI_CONSTAT(con, RDO); typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD); ucsi_partner_task(con, ucsi_get_src_pdos, 30, 0); ucsi_partner_task(con, ucsi_check_altmodes, 30, HZ); @@ -1028,7 +1028,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con) static int ucsi_register_partner(struct ucsi_connector *con) { - u8 pwr_opmode = UCSI_CONSTAT_PWR_OPMODE(con->status.flags); + u8 pwr_opmode = UCSI_CONSTAT(con, PWR_OPMODE); struct typec_partner_desc desc; struct typec_partner *partner; @@ -1037,7 +1037,7 @@ static int ucsi_register_partner(struct ucsi_connector *con) memset(&desc, 0, sizeof(desc)); - switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) { + switch (UCSI_CONSTAT(con, PARTNER_TYPE)) { case UCSI_CONSTAT_PARTNER_TYPE_DEBUG: desc.accessory = TYPEC_ACCESSORY_DEBUG; break; @@ -1055,6 +1055,9 @@ static int ucsi_register_partner(struct ucsi_connector *con) desc.identity = &con->partner_identity; desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD; + if (con->ucsi->version >= UCSI_VERSION_2_1) + desc.pd_revision = UCSI_SPEC_REVISION_TO_BCD(UCSI_CONCAP(con, PARTNER_PD_REVISION)); + partner = typec_register_partner(con->port, &desc); if (IS_ERR(partner)) { dev_err(con->ucsi->dev, @@ -1089,7 +1092,7 @@ static void ucsi_partner_change(struct ucsi_connector *con) enum usb_role u_role = USB_ROLE_NONE; int ret; - switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) { + switch (UCSI_CONSTAT(con, PARTNER_TYPE)) { case UCSI_CONSTAT_PARTNER_TYPE_UFP: case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP: u_role = USB_ROLE_HOST; @@ -1105,8 +1108,8 @@ static void ucsi_partner_change(struct ucsi_connector *con) break; } - if (con->status.flags & UCSI_CONSTAT_CONNECTED) { - switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) { + if (UCSI_CONSTAT(con, CONNECTED)) { + switch (UCSI_CONSTAT(con, PARTNER_TYPE)) { case UCSI_CONSTAT_PARTNER_TYPE_DEBUG: typec_set_mode(con->port, TYPEC_MODE_DEBUG); break; @@ -1114,14 +1117,14 @@ static void ucsi_partner_change(struct ucsi_connector *con) typec_set_mode(con->port, TYPEC_MODE_AUDIO); break; default: - if (UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) == - UCSI_CONSTAT_PARTNER_FLAG_USB) + if (UCSI_CONSTAT(con, PARTNER_FLAGS) == + UCSI_CONSTAT_PARTNER_FLAG_USB) typec_set_mode(con->port, TYPEC_STATE_USB); } } /* Only notify USB controller if partner supports USB data */ - if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) & UCSI_CONSTAT_PARTNER_FLAG_USB)) + if (!(UCSI_CONSTAT(con, PARTNER_FLAGS) & UCSI_CONSTAT_PARTNER_FLAG_USB)) u_role = USB_ROLE_NONE; ret = usb_role_switch_set_role(con->usb_role_sw, u_role); @@ -1132,7 +1135,7 @@ static void ucsi_partner_change(struct ucsi_connector *con) static int ucsi_check_connection(struct ucsi_connector *con) { - u8 prev_flags = con->status.flags; + u8 prev_state = UCSI_CONSTAT(con, CONNECTED); int ret; ret = ucsi_get_connector_status(con, false); @@ -1141,10 +1144,9 @@ static int ucsi_check_connection(struct ucsi_connector *con) return ret; } - if (con->status.flags == prev_flags) - return 0; - - if (con->status.flags & UCSI_CONSTAT_CONNECTED) { + if (UCSI_CONSTAT(con, CONNECTED)) { + if (prev_state) + return 0; ucsi_register_partner(con); ucsi_pwr_opmode_change(con); ucsi_partner_change(con); @@ -1200,6 +1202,7 @@ static void ucsi_handle_connector_change(struct work_struct *work) work); struct ucsi *ucsi = con->ucsi; enum typec_role role; + u16 change; int ret; mutex_lock(&con->lock); @@ -1212,14 +1215,15 @@ static void ucsi_handle_connector_change(struct work_struct *work) goto out_unlock; } - trace_ucsi_connector_change(con->num, &con->status); + trace_ucsi_connector_change(con->num, con); if (ucsi->ops->connector_status) ucsi->ops->connector_status(con); - role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR); + change = UCSI_CONSTAT(con, CHANGE); + role = UCSI_CONSTAT(con, PWR_DIR); - if (con->status.change & UCSI_CONSTAT_POWER_DIR_CHANGE) { + if (change & UCSI_CONSTAT_POWER_DIR_CHANGE) { typec_set_pwr_role(con->port, role); /* Complete pending power role swap */ @@ -1227,12 +1231,12 @@ static void ucsi_handle_connector_change(struct work_struct *work) complete(&con->complete); } - if (con->status.change & UCSI_CONSTAT_CONNECT_CHANGE) { + if (change & UCSI_CONSTAT_CONNECT_CHANGE) { typec_set_pwr_role(con->port, role); ucsi_port_psy_changed(con); ucsi_partner_change(con); - if (con->status.flags & UCSI_CONSTAT_CONNECTED) { + if (UCSI_CONSTAT(con, CONNECTED)) { ucsi_register_partner(con); ucsi_partner_task(con, ucsi_check_connection, 1, HZ); if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE) @@ -1240,8 +1244,7 @@ static void ucsi_handle_connector_change(struct work_struct *work) if (con->ucsi->cap.features & UCSI_CAP_CABLE_DETAILS) ucsi_partner_task(con, ucsi_check_cable, 1, HZ); - if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == - UCSI_CONSTAT_PWR_OPMODE_PD) { + if (UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) { ucsi_partner_task(con, ucsi_register_partner_pdos, 1, HZ); ucsi_partner_task(con, ucsi_check_connector_capability, 1, HZ); } @@ -1250,11 +1253,10 @@ static void ucsi_handle_connector_change(struct work_struct *work) } } - if (con->status.change & UCSI_CONSTAT_POWER_OPMODE_CHANGE || - con->status.change & UCSI_CONSTAT_POWER_LEVEL_CHANGE) + if (change & (UCSI_CONSTAT_POWER_OPMODE_CHANGE | UCSI_CONSTAT_POWER_LEVEL_CHANGE)) ucsi_pwr_opmode_change(con); - if (con->partner && con->status.change & UCSI_CONSTAT_PARTNER_CHANGE) { + if (con->partner && (change & UCSI_CONSTAT_PARTNER_CHANGE)) { ucsi_partner_change(con); /* Complete pending data role swap */ @@ -1262,10 +1264,10 @@ static void ucsi_handle_connector_change(struct work_struct *work) complete(&con->complete); } - if (con->status.change & UCSI_CONSTAT_CAM_CHANGE) + if (change & UCSI_CONSTAT_CAM_CHANGE) ucsi_partner_task(con, ucsi_check_altmodes, 1, HZ); - if (con->status.change & UCSI_CONSTAT_BC_CHANGE) + if (change & UCSI_CONSTAT_BC_CHANGE) ucsi_port_psy_changed(con); out_unlock: @@ -1425,7 +1427,7 @@ static int ucsi_dr_swap(struct typec_port *port, enum typec_data_role role) goto out_unlock; } - partner_type = UCSI_CONSTAT_PARTNER_TYPE(con->status.flags); + partner_type = UCSI_CONSTAT(con, PARTNER_TYPE); if ((partner_type == UCSI_CONSTAT_PARTNER_TYPE_DFP && role == TYPEC_DEVICE) || (partner_type == UCSI_CONSTAT_PARTNER_TYPE_UFP && @@ -1469,7 +1471,7 @@ static int ucsi_pr_swap(struct typec_port *port, enum typec_role role) goto out_unlock; } - cur_role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR); + cur_role = UCSI_CONSTAT(con, PWR_DIR); if (cur_role == role) goto out_unlock; @@ -1492,8 +1494,7 @@ static int ucsi_pr_swap(struct typec_port *port, enum typec_role role) mutex_lock(&con->lock); /* Something has gone wrong while swapping the role */ - if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) != - UCSI_CONSTAT_PWR_OPMODE_PD) { + if (UCSI_CONSTAT(con, PWR_OPMODE) != UCSI_CONSTAT_PWR_OPMODE_PD) { ucsi_reset_connector(con, true); ret = -EPROTO; } @@ -1561,19 +1562,18 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) if (ret < 0) goto out_unlock; - if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP) + if (UCSI_CONCAP(con, OPMODE_DRP)) cap->data = TYPEC_PORT_DRD; - else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DFP) + else if (UCSI_CONCAP(con, OPMODE_DFP)) cap->data = TYPEC_PORT_DFP; - else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP) + else if (UCSI_CONCAP(con, OPMODE_UFP)) cap->data = TYPEC_PORT_UFP; - if ((con->cap.flags & UCSI_CONCAP_FLAG_PROVIDER) && - (con->cap.flags & UCSI_CONCAP_FLAG_CONSUMER)) + if (UCSI_CONCAP(con, PROVIDER) && UCSI_CONCAP(con, CONSUMER)) cap->type = TYPEC_PORT_DRP; - else if (con->cap.flags & UCSI_CONCAP_FLAG_PROVIDER) + else if (UCSI_CONCAP(con, PROVIDER)) cap->type = TYPEC_PORT_SRC; - else if (con->cap.flags & UCSI_CONCAP_FLAG_CONSUMER) + else if (UCSI_CONCAP(con, CONSUMER)) cap->type = TYPEC_PORT_SNK; cap->revision = ucsi->cap.typec_version; @@ -1581,9 +1581,9 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) cap->svdm_version = SVDM_VER_2_0; cap->prefer_role = TYPEC_NO_PREFERRED_ROLE; - if (con->cap.op_mode & UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY) + if (UCSI_CONCAP(con, OPMODE_AUDIO_ACCESSORY)) *accessory++ = TYPEC_ACCESSORY_AUDIO; - if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY) + if (UCSI_CONCAP(con, OPMODE_DEBUG_ACCESSORY)) *accessory = TYPEC_ACCESSORY_DEBUG; cap->driver_data = con; @@ -1621,10 +1621,13 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) goto out; } + printk("%s: con%d: 0x%lx\n", __func__, con->num, + UCSI_CONSTAT(con, PWR_OPMODE)); + if (ucsi->ops->connector_status) ucsi->ops->connector_status(con); - switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) { + switch (UCSI_CONSTAT(con, PARTNER_TYPE)) { case UCSI_CONSTAT_PARTNER_TYPE_UFP: case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP: u_role = USB_ROLE_HOST; @@ -1641,9 +1644,8 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) } /* Check if there is already something connected */ - if (con->status.flags & UCSI_CONSTAT_CONNECTED) { - typec_set_pwr_role(con->port, - !!(con->status.flags & UCSI_CONSTAT_PWR_DIR)); + if (UCSI_CONSTAT(con, CONNECTED)) { + typec_set_pwr_role(con->port, UCSI_CONSTAT(con, PWR_DIR)); ucsi_register_partner(con); ucsi_pwr_opmode_change(con); ucsi_port_psy_changed(con); @@ -1654,7 +1656,7 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) } /* Only notify USB controller if partner supports USB data */ - if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) & UCSI_CONSTAT_PARTNER_FLAG_USB)) + if (!(UCSI_CONSTAT(con, PARTNER_FLAGS) & UCSI_CONSTAT_PARTNER_FLAG_USB)) u_role = USB_ROLE_NONE; ret = usb_role_switch_set_role(con->usb_role_sw, u_role); @@ -1664,16 +1666,14 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) ret = 0; } - if (con->partner && - UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == - UCSI_CONSTAT_PWR_OPMODE_PD) { + if (con->partner && UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) { ucsi_register_device_pdos(con); ucsi_get_src_pdos(con); ucsi_check_altmodes(con); ucsi_check_connector_capability(con); } - trace_ucsi_register_port(con->num, &con->status); + trace_ucsi_register_port(con->num, con); out: fwnode_handle_put(cap->fwnode); diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h index 4a017eb6a65b..0429f8b71465 100644 --- a/drivers/usb/typec/ucsi/ucsi.h +++ b/drivers/usb/typec/ucsi/ucsi.h @@ -4,6 +4,7 @@ #define __DRIVER_USB_TYPEC_UCSI_H #include <linux/bitops.h> +#include <linux/bitmap.h> #include <linux/completion.h> #include <linux/device.h> #include <linux/power_supply.h> @@ -95,26 +96,30 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num); /* -------------------------------------------------------------------------- */ /* Commands */ -#define UCSI_PPM_RESET 0x01 -#define UCSI_CANCEL 0x02 -#define UCSI_CONNECTOR_RESET 0x03 -#define UCSI_ACK_CC_CI 0x04 -#define UCSI_SET_NOTIFICATION_ENABLE 0x05 -#define UCSI_GET_CAPABILITY 0x06 -#define UCSI_GET_CONNECTOR_CAPABILITY 0x07 -#define UCSI_SET_UOM 0x08 -#define UCSI_SET_UOR 0x09 -#define UCSI_SET_PDM 0x0a -#define UCSI_SET_PDR 0x0b -#define UCSI_GET_ALTERNATE_MODES 0x0c -#define UCSI_GET_CAM_SUPPORTED 0x0d -#define UCSI_GET_CURRENT_CAM 0x0e -#define UCSI_SET_NEW_CAM 0x0f -#define UCSI_GET_PDOS 0x10 -#define UCSI_GET_CABLE_PROPERTY 0x11 -#define UCSI_GET_CONNECTOR_STATUS 0x12 -#define UCSI_GET_ERROR_STATUS 0x13 -#define UCSI_GET_PD_MESSAGE 0x15 +#define UCSI_PPM_RESET 0x01 +#define UCSI_CANCEL 0x02 +#define UCSI_CONNECTOR_RESET 0x03 +#define UCSI_ACK_CC_CI 0x04 +#define UCSI_SET_NOTIFICATION_ENABLE 0x05 +#define UCSI_GET_CAPABILITY 0x06 +#define UCSI_GET_CAPABILITY_SIZE 128 +#define UCSI_GET_CONNECTOR_CAPABILITY 0x07 +#define UCSI_GET_CONNECTOR_CAPABILITY_SIZE 32 +#define UCSI_SET_UOM 0x08 +#define UCSI_SET_UOR 0x09 +#define UCSI_SET_PDM 0x0a +#define UCSI_SET_PDR 0x0b +#define UCSI_GET_ALTERNATE_MODES 0x0c +#define UCSI_GET_CAM_SUPPORTED 0x0d +#define UCSI_GET_CURRENT_CAM 0x0e +#define UCSI_SET_NEW_CAM 0x0f +#define UCSI_GET_PDOS 0x10 +#define UCSI_GET_CABLE_PROPERTY 0x11 +#define UCSI_GET_CABLE_PROPERTY_SIZE 64 +#define UCSI_GET_CONNECTOR_STATUS 0x12 +#define UCSI_GET_CONNECTOR_STATUS_SIZE 152 +#define UCSI_GET_ERROR_STATUS 0x13 +#define UCSI_GET_PD_MESSAGE 0x15 #define UCSI_CONNECTOR_NUMBER(_num_) ((u64)(_num_) << 16) #define UCSI_COMMAND(_cmd_) ((_cmd_) & 0xff) @@ -250,42 +255,6 @@ struct ucsi_capability { u16 typec_version; } __packed; -/* Data structure filled by PPM in response to GET_CONNECTOR_CAPABILITY cmd. */ -struct ucsi_connector_capability { - u8 op_mode; -#define UCSI_CONCAP_OPMODE_DFP BIT(0) -#define UCSI_CONCAP_OPMODE_UFP BIT(1) -#define UCSI_CONCAP_OPMODE_DRP BIT(2) -#define UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY BIT(3) -#define UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY BIT(4) -#define UCSI_CONCAP_OPMODE_USB2 BIT(5) -#define UCSI_CONCAP_OPMODE_USB3 BIT(6) -#define UCSI_CONCAP_OPMODE_ALT_MODE BIT(7) - u32 flags; -#define UCSI_CONCAP_FLAG_PROVIDER BIT(0) -#define UCSI_CONCAP_FLAG_CONSUMER BIT(1) -#define UCSI_CONCAP_FLAG_SWAP_TO_DFP BIT(2) -#define UCSI_CONCAP_FLAG_SWAP_TO_UFP BIT(3) -#define UCSI_CONCAP_FLAG_SWAP_TO_SRC BIT(4) -#define UCSI_CONCAP_FLAG_SWAP_TO_SINK BIT(5) -#define UCSI_CONCAP_FLAG_EX_OP_MODE(_f_) \ - (((_f_) & GENMASK(13, 6)) >> 6) -#define UCSI_CONCAP_EX_OP_MODE_USB4_GEN2 BIT(0) -#define UCSI_CONCAP_EX_OP_MODE_EPR_SRC BIT(1) -#define UCSI_CONCAP_EX_OP_MODE_EPR_SINK BIT(2) -#define UCSI_CONCAP_EX_OP_MODE_USB4_GEN3 BIT(3) -#define UCSI_CONCAP_EX_OP_MODE_USB4_GEN4 BIT(4) -#define UCSI_CONCAP_FLAG_MISC_CAPS(_f_) \ - (((_f_) & GENMASK(17, 14)) >> 14) -#define UCSI_CONCAP_MISC_CAP_FW_UPDATE BIT(0) -#define UCSI_CONCAP_MISC_CAP_SECURITY BIT(1) -#define UCSI_CONCAP_FLAG_REV_CURR_PROT_SUPPORT BIT(18) -#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_) \ - (((_f_) & GENMASK(20, 19)) >> 19) -#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(_f_) \ - UCSI_SPEC_REVISION_TO_BCD(UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_)) -} __packed; - struct ucsi_altmode { u16 svid; u32 mid; @@ -311,49 +280,103 @@ struct ucsi_cable_property { u8 latency; } __packed; -/* Data structure filled by PPM in response to GET_CONNECTOR_STATUS command. */ -struct ucsi_connector_status { - u16 change; -#define UCSI_CONSTAT_EXT_SUPPLY_CHANGE BIT(1) -#define UCSI_CONSTAT_POWER_OPMODE_CHANGE BIT(2) -#define UCSI_CONSTAT_PDOS_CHANGE BIT(5) -#define UCSI_CONSTAT_POWER_LEVEL_CHANGE BIT(6) -#define UCSI_CONSTAT_PD_RESET_COMPLETE BIT(7) -#define UCSI_CONSTAT_CAM_CHANGE BIT(8) -#define UCSI_CONSTAT_BC_CHANGE BIT(9) -#define UCSI_CONSTAT_PARTNER_CHANGE BIT(11) -#define UCSI_CONSTAT_POWER_DIR_CHANGE BIT(12) -#define UCSI_CONSTAT_CONNECT_CHANGE BIT(14) -#define UCSI_CONSTAT_ERROR BIT(15) - u16 flags; -#define UCSI_CONSTAT_PWR_OPMODE(_f_) ((_f_) & GENMASK(2, 0)) +/* Get Connector Capability Fields. */ +#define UCSI_CONCAP_OPMODE DECLARE_BITFIELD(0, 0, 8) +#define UCSI_CONCAP_OPMODE_DFP DECLARE_BITFIELD(0, 0, 1) +#define UCSI_CONCAP_OPMODE_UFP DECLARE_BITFIELD(0, 1, 1) +#define UCSI_CONCAP_OPMODE_DRP DECLARE_BITFIELD(0, 2, 1) +#define UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY DECLARE_BITFIELD(0, 3, 1) +#define UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY DECLARE_BITFIELD(0, 4, 1) +#define UCSI_CONCAP_OPMODE_USB2 DECLARE_BITFIELD(0, 5, 1) +#define UCSI_CONCAP_OPMODE_USB3 DECLARE_BITFIELD(0, 6, 1) +#define UCSI_CONCAP_OPMODE_ALT_MODE DECLARE_BITFIELD(0, 7, 1) +#define UCSI_CONCAP_PROVIDER DECLARE_BITFIELD(0, 8, 1) +#define UCSI_CONCAP_CONSUMER DECLARE_BITFIELD(0, 9, 1) +#define UCSI_CONCAP_SWAP_TO_DFP DECLARE_BITFIELD(0x110, 10, 1) +#define UCSI_CONCAP_SWAP_TO_UFP DECLARE_BITFIELD(0x110, 11, 1) +#define UCSI_CONCAP_SWAP_TO_SRC DECLARE_BITFIELD(0x110, 12, 1) +#define UCSI_CONCAP_SWAP_TO_SNK DECLARE_BITFIELD(0x110, 13, 1) +#define UCSI_CONCAP_EXT_OPMODE DECLARE_BITFIELD(0x200, 14, 8) +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN2 DECLARE_BITFIELD(0x200, 14, 1) +#define UCSI_CONCAP_EXT_OPMODE_EPR_SRC DECLARE_BITFIELD(0x200, 15, 1) +#define UCSI_CONCAP_EXT_OPMODE_EPR_SINK DECLARE_BITFIELD(0x200, 16, 1) +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN3 DECLARE_BITFIELD(0x200, 17, 1) +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN4 DECLARE_BITFIELD(0x200, 18, 1) +#define UCSI_CONCAP_MISC DECLARE_BITFIELD(0x200, 22, 4) +#define UCSI_CONCAP_MISC_FW_UPDATE DECLARE_BITFIELD(0x200, 22, 1) +#define UCSI_CONCAP_MISC_SECURITY DECLARE_BITFIELD(0x200, 23, 1) +#define UCSI_CONCAP_REV_CURR_PROT_SUPPORT DECLARE_BITFIELD(0x200, 26, 1) +#define UCSI_CONCAP_PARTNER_PD_REVISION DECLARE_BITFIELD(0x210, 27, 2) + +/* Get Connector Status Fields. */ +#define UCSI_CONSTAT_CHANGE DECLARE_BITFIELD(0, 0, 16) +#define UCSI_CONSTAT_PWR_OPMODE DECLARE_BITFIELD(0, 16, 3) #define UCSI_CONSTAT_PWR_OPMODE_NONE 0 #define UCSI_CONSTAT_PWR_OPMODE_DEFAULT 1 #define UCSI_CONSTAT_PWR_OPMODE_BC 2 #define UCSI_CONSTAT_PWR_OPMODE_PD 3 #define UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5 4 #define UCSI_CONSTAT_PWR_OPMODE_TYPEC3_0 5 -#define UCSI_CONSTAT_CONNECTED BIT(3) -#define UCSI_CONSTAT_PWR_DIR BIT(4) -#define UCSI_CONSTAT_PARTNER_FLAGS(_f_) (((_f_) & GENMASK(12, 5)) >> 5) +#define UCSI_CONSTAT_CONNECTED DECLARE_BITFIELD(0, 19, 1) +#define UCSI_CONSTAT_PWR_DIR DECLARE_BITFIELD(0, 20, 1) +#define UCSI_CONSTAT_PARTNER_FLAGS DECLARE_BITFIELD(0, 21, 8) #define UCSI_CONSTAT_PARTNER_FLAG_USB 1 #define UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE 2 -#define UCSI_CONSTAT_PARTNER_TYPE(_f_) (((_f_) & GENMASK(15, 13)) >> 13) +#define UCSI_CONSTAT_PARTNER_TYPE DECLARE_BITFIELD(0, 29, 3) #define UCSI_CONSTAT_PARTNER_TYPE_DFP 1 #define UCSI_CONSTAT_PARTNER_TYPE_UFP 2 -#define UCSI_CONSTAT_PARTNER_TYPE_CABLE 3 /* Powered Cable */ -#define UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP 4 /* Powered Cable */ +#define UCSI_CONSTAT_PARTNER_TYPE_CABLE 3 /* Powered Cable */ +#define UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP 4 /* Powered Cable */ #define UCSI_CONSTAT_PARTNER_TYPE_DEBUG 5 #define UCSI_CONSTAT_PARTNER_TYPE_AUDIO 6 - u32 request_data_obj; - - u8 pwr_status; -#define UCSI_CONSTAT_BC_STATUS(_p_) ((_p_) & GENMASK(1, 0)) +#define UCSI_CONSTAT_RDO DECLARE_BITFIELD(0, 32, 32) +#define UCSI_CONSTAT_BC_STATUS DECLARE_BITFIELD(0, 64, 2) #define UCSI_CONSTAT_BC_NOT_CHARGING 0 #define UCSI_CONSTAT_BC_NOMINAL_CHARGING 1 #define UCSI_CONSTAT_BC_SLOW_CHARGING 2 #define UCSI_CONSTAT_BC_TRICKLE_CHARGING 3 -} __packed; +#define UCSI_CONSTAT_PD_VERSION DECLARE_BITFIELD(0x120, 70, 16) + +/* Connector Status Change Bits. */ +#define UCSI_CONSTAT_EXT_SUPPLY_CHANGE BIT(1) +#define UCSI_CONSTAT_POWER_OPMODE_CHANGE BIT(2) +#define UCSI_CONSTAT_PDOS_CHANGE BIT(5) +#define UCSI_CONSTAT_POWER_LEVEL_CHANGE BIT(6) +#define UCSI_CONSTAT_PD_RESET_COMPLETE BIT(7) +#define UCSI_CONSTAT_CAM_CHANGE BIT(8) +#define UCSI_CONSTAT_BC_CHANGE BIT(9) +#define UCSI_CONSTAT_PARTNER_CHANGE BIT(11) +#define UCSI_CONSTAT_POWER_DIR_CHANGE BIT(12) +#define UCSI_CONSTAT_CONNECT_CHANGE BIT(14) +#define UCSI_CONSTAT_ERROR BIT(15) + +#define bitfield_read(_map_, _field_, _ver_) \ + ({ \ + struct bitfield f = (_field_); \ + WARN((_ver_) < f.version, \ + "Access to unsupported field.") ? 0 : \ + bitmap_read((_map_), f.offset, f.size); \ + }) + +#define DECLARE_BITFIELD(_ver_, _offset_, _size_) \ +(struct bitfield) { \ + .version = _ver_, \ + .offset = _offset_, \ + .size = _size_ \ +} + +struct bitfield { + const u16 version; + const u8 offset; + const u8 size; +}; + +/* Helpers to access cached command responses. */ +#define UCSI_CONCAP(_con_, _field_) \ + bitfield_read((_con_)->status, UCSI_CONCAP_##_field_, (_con_)->ucsi->version) + +#define UCSI_CONSTAT(_con_, _field_) \ + bitfield_read((_con_)->status, UCSI_CONSTAT_##_field_, (_con_)->ucsi->version) /* -------------------------------------------------------------------------- */ @@ -433,8 +456,10 @@ struct ucsi_connector { struct typec_capability typec_cap; - struct ucsi_connector_status status; - struct ucsi_connector_capability cap; + /* Cached command responses. */ + DECLARE_BITMAP(cap, UCSI_GET_CONNECTOR_CAPABILITY_SIZE); + DECLARE_BITMAP(status, UCSI_GET_CONNECTOR_STATUS_SIZE); + struct power_supply *psy; struct power_supply_desc psy_desc; u32 rdo; diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c index 7a5dff8d9cc6..48ee9e4cac3b 100644 --- a/drivers/usb/typec/ucsi/ucsi_acpi.c +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c @@ -143,7 +143,6 @@ static int ucsi_gram_read_message_in(struct ucsi *ucsi, void *val, size_t val_le u16 bogus_change = UCSI_CONSTAT_POWER_LEVEL_CHANGE | UCSI_CONSTAT_PDOS_CHANGE; struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); - struct ucsi_connector_status *status; int ret; ret = ucsi_acpi_read_message_in(ucsi, val, val_len); @@ -152,11 +151,9 @@ static int ucsi_gram_read_message_in(struct ucsi *ucsi, void *val, size_t val_le if (UCSI_COMMAND(ua->cmd) == UCSI_GET_CONNECTOR_STATUS && ua->check_bogus_event) { - status = (struct ucsi_connector_status *)val; - /* Clear the bogus change */ - if (status->change == bogus_change) - status->change = 0; + if (*(u16 *)val == bogus_change) + *(u16 *)val = 0; ua->check_bogus_event = false; } -- 2.45.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v2 2/2] usb: typec: ucsi: Convert connector specific commands to bitmaps 2024-09-03 14:53 ` [RFC PATCH v2 2/2] usb: typec: ucsi: Convert connector specific commands to bitmaps Heikki Krogerus @ 2024-09-04 5:16 ` Abhishek Pandit-Subedi 2024-09-05 12:36 ` Heikki Krogerus 2024-09-12 12:04 ` Dmitry Baryshkov 1 sibling, 1 reply; 8+ messages in thread From: Abhishek Pandit-Subedi @ 2024-09-04 5:16 UTC (permalink / raw) To: Heikki Krogerus Cc: Jameson Thies, Benson Leung, Bartosz Szpila, Pilla, Siva sai kumar, linux-usb On Tue, Sep 3, 2024 at 7:53 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > This is hopefully a bit less hacky, but it still needs work. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/ucsi/psy.c | 28 ++--- > drivers/usb/typec/ucsi/trace.h | 28 ++--- > drivers/usb/typec/ucsi/ucsi.c | 108 ++++++++-------- > drivers/usb/typec/ucsi/ucsi.h | 195 ++++++++++++++++------------- > drivers/usb/typec/ucsi/ucsi_acpi.c | 7 +- > 5 files changed, 192 insertions(+), 174 deletions(-) > > diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c > index e623d80e177c..4d5398afa7a2 100644 > --- a/drivers/usb/typec/ucsi/psy.c > +++ b/drivers/usb/typec/ucsi/psy.c > @@ -55,8 +55,8 @@ static int ucsi_psy_get_online(struct ucsi_connector *con, > union power_supply_propval *val) > { > val->intval = UCSI_PSY_OFFLINE; > - if (con->status.flags & UCSI_CONSTAT_CONNECTED && > - (con->status.flags & UCSI_CONSTAT_PWR_DIR) == TYPEC_SINK) > + if (UCSI_CONSTAT(con, CONNECTED) && > + (UCSI_CONSTAT(con, PWR_DIR) == TYPEC_SINK)) > val->intval = UCSI_PSY_FIXED_ONLINE; > return 0; > } > @@ -66,7 +66,7 @@ static int ucsi_psy_get_voltage_min(struct ucsi_connector *con, > { > u32 pdo; > > - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { > + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { > case UCSI_CONSTAT_PWR_OPMODE_PD: > pdo = con->src_pdos[0]; > val->intval = pdo_fixed_voltage(pdo) * 1000; > @@ -89,7 +89,7 @@ static int ucsi_psy_get_voltage_max(struct ucsi_connector *con, > { > u32 pdo; > > - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { > + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { > case UCSI_CONSTAT_PWR_OPMODE_PD: > if (con->num_pdos > 0) { > pdo = con->src_pdos[con->num_pdos - 1]; > @@ -117,7 +117,7 @@ static int ucsi_psy_get_voltage_now(struct ucsi_connector *con, > int index; > u32 pdo; > > - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { > + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { > case UCSI_CONSTAT_PWR_OPMODE_PD: > index = rdo_index(con->rdo); > if (index > 0) { > @@ -145,7 +145,7 @@ static int ucsi_psy_get_current_max(struct ucsi_connector *con, > { > u32 pdo; > > - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { > + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { > case UCSI_CONSTAT_PWR_OPMODE_PD: > if (con->num_pdos > 0) { > pdo = con->src_pdos[con->num_pdos - 1]; > @@ -173,9 +173,7 @@ static int ucsi_psy_get_current_max(struct ucsi_connector *con, > static int ucsi_psy_get_current_now(struct ucsi_connector *con, > union power_supply_propval *val) > { > - u16 flags = con->status.flags; > - > - if (UCSI_CONSTAT_PWR_OPMODE(flags) == UCSI_CONSTAT_PWR_OPMODE_PD) > + if (UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) > val->intval = rdo_op_current(con->rdo) * 1000; > else > val->intval = 0; > @@ -185,11 +183,9 @@ static int ucsi_psy_get_current_now(struct ucsi_connector *con, > static int ucsi_psy_get_usb_type(struct ucsi_connector *con, > union power_supply_propval *val) > { > - u16 flags = con->status.flags; > - > val->intval = POWER_SUPPLY_USB_TYPE_C; > - if (flags & UCSI_CONSTAT_CONNECTED && > - UCSI_CONSTAT_PWR_OPMODE(flags) == UCSI_CONSTAT_PWR_OPMODE_PD) > + if (UCSI_CONSTAT(con, CONNECTED) && > + UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) > val->intval = POWER_SUPPLY_USB_TYPE_PD; > > return 0; > @@ -197,18 +193,18 @@ static int ucsi_psy_get_usb_type(struct ucsi_connector *con, > > static int ucsi_psy_get_charge_type(struct ucsi_connector *con, union power_supply_propval *val) > { > - if (!(con->status.flags & UCSI_CONSTAT_CONNECTED)) { > + if (!(UCSI_CONSTAT(con, CONNECTED))) { > val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE; > return 0; > } > > /* The Battery Charging Cabability Status field is only valid in sink role. */ > - if ((con->status.flags & UCSI_CONSTAT_PWR_DIR) != TYPEC_SINK) { > + if (UCSI_CONSTAT(con, PWR_DIR) != TYPEC_SINK) { > val->intval = POWER_SUPPLY_CHARGE_TYPE_UNKNOWN; > return 0; > } > > - switch (UCSI_CONSTAT_BC_STATUS(con->status.pwr_status)) { > + switch (UCSI_CONSTAT(con, BC_STATUS)) { > case UCSI_CONSTAT_BC_NOMINAL_CHARGING: > val->intval = POWER_SUPPLY_CHARGE_TYPE_STANDARD; > break; > diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h > index a0d3a934d3d9..41701dee7056 100644 > --- a/drivers/usb/typec/ucsi/trace.h > +++ b/drivers/usb/typec/ucsi/trace.h > @@ -40,8 +40,8 @@ DEFINE_EVENT(ucsi_log_command, ucsi_reset_ppm, > ); > > DECLARE_EVENT_CLASS(ucsi_log_connector_status, > - TP_PROTO(int port, struct ucsi_connector_status *status), > - TP_ARGS(port, status), > + TP_PROTO(int port, struct ucsi_connector *con), > + TP_ARGS(port, con), > TP_STRUCT__entry( > __field(int, port) > __field(u16, change) > @@ -55,14 +55,14 @@ DECLARE_EVENT_CLASS(ucsi_log_connector_status, > ), > TP_fast_assign( > __entry->port = port - 1; > - __entry->change = status->change; > - __entry->opmode = UCSI_CONSTAT_PWR_OPMODE(status->flags); > - __entry->connected = !!(status->flags & UCSI_CONSTAT_CONNECTED); > - __entry->pwr_dir = !!(status->flags & UCSI_CONSTAT_PWR_DIR); > - __entry->partner_flags = UCSI_CONSTAT_PARTNER_FLAGS(status->flags); > - __entry->partner_type = UCSI_CONSTAT_PARTNER_TYPE(status->flags); > - __entry->request_data_obj = status->request_data_obj; > - __entry->bc_status = UCSI_CONSTAT_BC_STATUS(status->pwr_status); > + __entry->change = UCSI_CONSTAT(con, CHANGE); > + __entry->opmode = UCSI_CONSTAT(con, PWR_OPMODE); > + __entry->connected = UCSI_CONSTAT(con, CONNECTED); > + __entry->pwr_dir = UCSI_CONSTAT(con, PWR_DIR); > + __entry->partner_flags = UCSI_CONSTAT(con, PARTNER_FLAGS); > + __entry->partner_type = UCSI_CONSTAT(con, PARTNER_TYPE); > + __entry->request_data_obj = UCSI_CONSTAT(con, RDO); > + __entry->bc_status = UCSI_CONSTAT(con, BC_STATUS); > ), > TP_printk("port%d status: change=%04x, opmode=%x, connected=%d, " > "sourcing=%d, partner_flags=%x, partner_type=%x, " > @@ -73,13 +73,13 @@ DECLARE_EVENT_CLASS(ucsi_log_connector_status, > ); > > DEFINE_EVENT(ucsi_log_connector_status, ucsi_connector_change, > - TP_PROTO(int port, struct ucsi_connector_status *status), > - TP_ARGS(port, status) > + TP_PROTO(int port, struct ucsi_connector *con), > + TP_ARGS(port, con) > ); > > DEFINE_EVENT(ucsi_log_connector_status, ucsi_register_port, > - TP_PROTO(int port, struct ucsi_connector_status *status), > - TP_ARGS(port, status) > + TP_PROTO(int port, struct ucsi_connector *con), > + TP_ARGS(port, con) > ); > > DECLARE_EVENT_CLASS(ucsi_log_register_altmode, > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index 71cf64a424d9..b03eace5a8f0 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -649,10 +649,11 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient) > static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack) > { > u64 command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num); > - struct ucsi *ucsi = con->ucsi; > + size_t size = min(UCSI_GET_CONNECTOR_STATUS_SIZE, UCSI_MAX_DATA_LENGTH(con->ucsi)); > int ret; > > - ret = ucsi_send_command_common(ucsi, command, &con->status, sizeof(con->status), conn_ack); > + ret = ucsi_send_command_common(con->ucsi, command, &con->status, size, conn_ack); > + > return ret < 0 ? ret : 0; > } > > @@ -666,8 +667,7 @@ static int ucsi_read_pdos(struct ucsi_connector *con, > > if (is_partner && > ucsi->quirks & UCSI_NO_PARTNER_PDOS && > - ((con->status.flags & UCSI_CONSTAT_PWR_DIR) || > - !is_source(role))) > + (UCSI_CONSTAT(con, PWR_DIR) || !is_source(role))) > return 0; > > command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num); > @@ -995,16 +995,16 @@ static int ucsi_check_connector_capability(struct ucsi_connector *con) > } > > typec_partner_set_pd_revision(con->partner, > - UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags)); > + UCSI_SPEC_REVISION_TO_BCD(UCSI_CONCAP(con, PARTNER_PD_REVISION))); > > return ret; > } > > static void ucsi_pwr_opmode_change(struct ucsi_connector *con) > { > - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { > + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { > case UCSI_CONSTAT_PWR_OPMODE_PD: > - con->rdo = con->status.request_data_obj; > + con->rdo = UCSI_CONSTAT(con, RDO); > typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD); > ucsi_partner_task(con, ucsi_get_src_pdos, 30, 0); > ucsi_partner_task(con, ucsi_check_altmodes, 30, HZ); > @@ -1028,7 +1028,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con) > > static int ucsi_register_partner(struct ucsi_connector *con) > { > - u8 pwr_opmode = UCSI_CONSTAT_PWR_OPMODE(con->status.flags); > + u8 pwr_opmode = UCSI_CONSTAT(con, PWR_OPMODE); > struct typec_partner_desc desc; > struct typec_partner *partner; > > @@ -1037,7 +1037,7 @@ static int ucsi_register_partner(struct ucsi_connector *con) > > memset(&desc, 0, sizeof(desc)); > > - switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) { > + switch (UCSI_CONSTAT(con, PARTNER_TYPE)) { > case UCSI_CONSTAT_PARTNER_TYPE_DEBUG: > desc.accessory = TYPEC_ACCESSORY_DEBUG; > break; > @@ -1055,6 +1055,9 @@ static int ucsi_register_partner(struct ucsi_connector *con) > desc.identity = &con->partner_identity; > desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD; > > + if (con->ucsi->version >= UCSI_VERSION_2_1) > + desc.pd_revision = UCSI_SPEC_REVISION_TO_BCD(UCSI_CONCAP(con, PARTNER_PD_REVISION)); > + > partner = typec_register_partner(con->port, &desc); > if (IS_ERR(partner)) { > dev_err(con->ucsi->dev, > @@ -1089,7 +1092,7 @@ static void ucsi_partner_change(struct ucsi_connector *con) > enum usb_role u_role = USB_ROLE_NONE; > int ret; > > - switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) { > + switch (UCSI_CONSTAT(con, PARTNER_TYPE)) { > case UCSI_CONSTAT_PARTNER_TYPE_UFP: > case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP: > u_role = USB_ROLE_HOST; > @@ -1105,8 +1108,8 @@ static void ucsi_partner_change(struct ucsi_connector *con) > break; > } > > - if (con->status.flags & UCSI_CONSTAT_CONNECTED) { > - switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) { > + if (UCSI_CONSTAT(con, CONNECTED)) { > + switch (UCSI_CONSTAT(con, PARTNER_TYPE)) { > case UCSI_CONSTAT_PARTNER_TYPE_DEBUG: > typec_set_mode(con->port, TYPEC_MODE_DEBUG); > break; > @@ -1114,14 +1117,14 @@ static void ucsi_partner_change(struct ucsi_connector *con) > typec_set_mode(con->port, TYPEC_MODE_AUDIO); > break; > default: > - if (UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) == > - UCSI_CONSTAT_PARTNER_FLAG_USB) > + if (UCSI_CONSTAT(con, PARTNER_FLAGS) == > + UCSI_CONSTAT_PARTNER_FLAG_USB) > typec_set_mode(con->port, TYPEC_STATE_USB); > } > } > > /* Only notify USB controller if partner supports USB data */ > - if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) & UCSI_CONSTAT_PARTNER_FLAG_USB)) > + if (!(UCSI_CONSTAT(con, PARTNER_FLAGS) & UCSI_CONSTAT_PARTNER_FLAG_USB)) > u_role = USB_ROLE_NONE; > > ret = usb_role_switch_set_role(con->usb_role_sw, u_role); > @@ -1132,7 +1135,7 @@ static void ucsi_partner_change(struct ucsi_connector *con) > > static int ucsi_check_connection(struct ucsi_connector *con) > { > - u8 prev_flags = con->status.flags; > + u8 prev_state = UCSI_CONSTAT(con, CONNECTED); > int ret; > > ret = ucsi_get_connector_status(con, false); > @@ -1141,10 +1144,9 @@ static int ucsi_check_connection(struct ucsi_connector *con) > return ret; > } > > - if (con->status.flags == prev_flags) > - return 0; > - > - if (con->status.flags & UCSI_CONSTAT_CONNECTED) { > + if (UCSI_CONSTAT(con, CONNECTED)) { > + if (prev_state) > + return 0; > ucsi_register_partner(con); > ucsi_pwr_opmode_change(con); > ucsi_partner_change(con); > @@ -1200,6 +1202,7 @@ static void ucsi_handle_connector_change(struct work_struct *work) > work); > struct ucsi *ucsi = con->ucsi; > enum typec_role role; > + u16 change; > int ret; > > mutex_lock(&con->lock); > @@ -1212,14 +1215,15 @@ static void ucsi_handle_connector_change(struct work_struct *work) > goto out_unlock; > } > > - trace_ucsi_connector_change(con->num, &con->status); > + trace_ucsi_connector_change(con->num, con); > > if (ucsi->ops->connector_status) > ucsi->ops->connector_status(con); > > - role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR); > + change = UCSI_CONSTAT(con, CHANGE); > + role = UCSI_CONSTAT(con, PWR_DIR); > > - if (con->status.change & UCSI_CONSTAT_POWER_DIR_CHANGE) { > + if (change & UCSI_CONSTAT_POWER_DIR_CHANGE) { > typec_set_pwr_role(con->port, role); > > /* Complete pending power role swap */ > @@ -1227,12 +1231,12 @@ static void ucsi_handle_connector_change(struct work_struct *work) > complete(&con->complete); > } > > - if (con->status.change & UCSI_CONSTAT_CONNECT_CHANGE) { > + if (change & UCSI_CONSTAT_CONNECT_CHANGE) { > typec_set_pwr_role(con->port, role); > ucsi_port_psy_changed(con); > ucsi_partner_change(con); > > - if (con->status.flags & UCSI_CONSTAT_CONNECTED) { > + if (UCSI_CONSTAT(con, CONNECTED)) { > ucsi_register_partner(con); > ucsi_partner_task(con, ucsi_check_connection, 1, HZ); > if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE) > @@ -1240,8 +1244,7 @@ static void ucsi_handle_connector_change(struct work_struct *work) > if (con->ucsi->cap.features & UCSI_CAP_CABLE_DETAILS) > ucsi_partner_task(con, ucsi_check_cable, 1, HZ); > > - if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == > - UCSI_CONSTAT_PWR_OPMODE_PD) { > + if (UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) { > ucsi_partner_task(con, ucsi_register_partner_pdos, 1, HZ); > ucsi_partner_task(con, ucsi_check_connector_capability, 1, HZ); > } > @@ -1250,11 +1253,10 @@ static void ucsi_handle_connector_change(struct work_struct *work) > } > } > > - if (con->status.change & UCSI_CONSTAT_POWER_OPMODE_CHANGE || > - con->status.change & UCSI_CONSTAT_POWER_LEVEL_CHANGE) > + if (change & (UCSI_CONSTAT_POWER_OPMODE_CHANGE | UCSI_CONSTAT_POWER_LEVEL_CHANGE)) > ucsi_pwr_opmode_change(con); > > - if (con->partner && con->status.change & UCSI_CONSTAT_PARTNER_CHANGE) { > + if (con->partner && (change & UCSI_CONSTAT_PARTNER_CHANGE)) { > ucsi_partner_change(con); > > /* Complete pending data role swap */ > @@ -1262,10 +1264,10 @@ static void ucsi_handle_connector_change(struct work_struct *work) > complete(&con->complete); > } > > - if (con->status.change & UCSI_CONSTAT_CAM_CHANGE) > + if (change & UCSI_CONSTAT_CAM_CHANGE) > ucsi_partner_task(con, ucsi_check_altmodes, 1, HZ); > > - if (con->status.change & UCSI_CONSTAT_BC_CHANGE) > + if (change & UCSI_CONSTAT_BC_CHANGE) > ucsi_port_psy_changed(con); > > out_unlock: > @@ -1425,7 +1427,7 @@ static int ucsi_dr_swap(struct typec_port *port, enum typec_data_role role) > goto out_unlock; > } > > - partner_type = UCSI_CONSTAT_PARTNER_TYPE(con->status.flags); > + partner_type = UCSI_CONSTAT(con, PARTNER_TYPE); > if ((partner_type == UCSI_CONSTAT_PARTNER_TYPE_DFP && > role == TYPEC_DEVICE) || > (partner_type == UCSI_CONSTAT_PARTNER_TYPE_UFP && > @@ -1469,7 +1471,7 @@ static int ucsi_pr_swap(struct typec_port *port, enum typec_role role) > goto out_unlock; > } > > - cur_role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR); > + cur_role = UCSI_CONSTAT(con, PWR_DIR); > > if (cur_role == role) > goto out_unlock; > @@ -1492,8 +1494,7 @@ static int ucsi_pr_swap(struct typec_port *port, enum typec_role role) > mutex_lock(&con->lock); > > /* Something has gone wrong while swapping the role */ > - if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) != > - UCSI_CONSTAT_PWR_OPMODE_PD) { > + if (UCSI_CONSTAT(con, PWR_OPMODE) != UCSI_CONSTAT_PWR_OPMODE_PD) { > ucsi_reset_connector(con, true); > ret = -EPROTO; > } > @@ -1561,19 +1562,18 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > if (ret < 0) > goto out_unlock; > > - if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP) > + if (UCSI_CONCAP(con, OPMODE_DRP)) > cap->data = TYPEC_PORT_DRD; > - else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DFP) > + else if (UCSI_CONCAP(con, OPMODE_DFP)) > cap->data = TYPEC_PORT_DFP; > - else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP) > + else if (UCSI_CONCAP(con, OPMODE_UFP)) > cap->data = TYPEC_PORT_UFP; > > - if ((con->cap.flags & UCSI_CONCAP_FLAG_PROVIDER) && > - (con->cap.flags & UCSI_CONCAP_FLAG_CONSUMER)) > + if (UCSI_CONCAP(con, PROVIDER) && UCSI_CONCAP(con, CONSUMER)) > cap->type = TYPEC_PORT_DRP; > - else if (con->cap.flags & UCSI_CONCAP_FLAG_PROVIDER) > + else if (UCSI_CONCAP(con, PROVIDER)) > cap->type = TYPEC_PORT_SRC; > - else if (con->cap.flags & UCSI_CONCAP_FLAG_CONSUMER) > + else if (UCSI_CONCAP(con, CONSUMER)) > cap->type = TYPEC_PORT_SNK; > > cap->revision = ucsi->cap.typec_version; > @@ -1581,9 +1581,9 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > cap->svdm_version = SVDM_VER_2_0; > cap->prefer_role = TYPEC_NO_PREFERRED_ROLE; > > - if (con->cap.op_mode & UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY) > + if (UCSI_CONCAP(con, OPMODE_AUDIO_ACCESSORY)) > *accessory++ = TYPEC_ACCESSORY_AUDIO; > - if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY) > + if (UCSI_CONCAP(con, OPMODE_DEBUG_ACCESSORY)) > *accessory = TYPEC_ACCESSORY_DEBUG; > > cap->driver_data = con; > @@ -1621,10 +1621,13 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > goto out; > } > > + printk("%s: con%d: 0x%lx\n", __func__, con->num, > + UCSI_CONSTAT(con, PWR_OPMODE)); > + > if (ucsi->ops->connector_status) > ucsi->ops->connector_status(con); > > - switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) { > + switch (UCSI_CONSTAT(con, PARTNER_TYPE)) { > case UCSI_CONSTAT_PARTNER_TYPE_UFP: > case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP: > u_role = USB_ROLE_HOST; > @@ -1641,9 +1644,8 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > } > > /* Check if there is already something connected */ > - if (con->status.flags & UCSI_CONSTAT_CONNECTED) { > - typec_set_pwr_role(con->port, > - !!(con->status.flags & UCSI_CONSTAT_PWR_DIR)); > + if (UCSI_CONSTAT(con, CONNECTED)) { > + typec_set_pwr_role(con->port, UCSI_CONSTAT(con, PWR_DIR)); > ucsi_register_partner(con); > ucsi_pwr_opmode_change(con); > ucsi_port_psy_changed(con); > @@ -1654,7 +1656,7 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > } > > /* Only notify USB controller if partner supports USB data */ > - if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) & UCSI_CONSTAT_PARTNER_FLAG_USB)) > + if (!(UCSI_CONSTAT(con, PARTNER_FLAGS) & UCSI_CONSTAT_PARTNER_FLAG_USB)) > u_role = USB_ROLE_NONE; > > ret = usb_role_switch_set_role(con->usb_role_sw, u_role); > @@ -1664,16 +1666,14 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > ret = 0; > } > > - if (con->partner && > - UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == > - UCSI_CONSTAT_PWR_OPMODE_PD) { > + if (con->partner && UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) { > ucsi_register_device_pdos(con); > ucsi_get_src_pdos(con); > ucsi_check_altmodes(con); > ucsi_check_connector_capability(con); > } > > - trace_ucsi_register_port(con->num, &con->status); > + trace_ucsi_register_port(con->num, con); > > out: > fwnode_handle_put(cap->fwnode); > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > index 4a017eb6a65b..0429f8b71465 100644 > --- a/drivers/usb/typec/ucsi/ucsi.h > +++ b/drivers/usb/typec/ucsi/ucsi.h > @@ -4,6 +4,7 @@ > #define __DRIVER_USB_TYPEC_UCSI_H > > #include <linux/bitops.h> > +#include <linux/bitmap.h> > #include <linux/completion.h> > #include <linux/device.h> > #include <linux/power_supply.h> > @@ -95,26 +96,30 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num); > /* -------------------------------------------------------------------------- */ > > /* Commands */ > -#define UCSI_PPM_RESET 0x01 > -#define UCSI_CANCEL 0x02 > -#define UCSI_CONNECTOR_RESET 0x03 > -#define UCSI_ACK_CC_CI 0x04 > -#define UCSI_SET_NOTIFICATION_ENABLE 0x05 > -#define UCSI_GET_CAPABILITY 0x06 > -#define UCSI_GET_CONNECTOR_CAPABILITY 0x07 > -#define UCSI_SET_UOM 0x08 > -#define UCSI_SET_UOR 0x09 > -#define UCSI_SET_PDM 0x0a > -#define UCSI_SET_PDR 0x0b > -#define UCSI_GET_ALTERNATE_MODES 0x0c > -#define UCSI_GET_CAM_SUPPORTED 0x0d > -#define UCSI_GET_CURRENT_CAM 0x0e > -#define UCSI_SET_NEW_CAM 0x0f > -#define UCSI_GET_PDOS 0x10 > -#define UCSI_GET_CABLE_PROPERTY 0x11 > -#define UCSI_GET_CONNECTOR_STATUS 0x12 > -#define UCSI_GET_ERROR_STATUS 0x13 > -#define UCSI_GET_PD_MESSAGE 0x15 > +#define UCSI_PPM_RESET 0x01 > +#define UCSI_CANCEL 0x02 > +#define UCSI_CONNECTOR_RESET 0x03 > +#define UCSI_ACK_CC_CI 0x04 > +#define UCSI_SET_NOTIFICATION_ENABLE 0x05 > +#define UCSI_GET_CAPABILITY 0x06 > +#define UCSI_GET_CAPABILITY_SIZE 128 I don't think you use this yet. > +#define UCSI_GET_CONNECTOR_CAPABILITY 0x07 > +#define UCSI_GET_CONNECTOR_CAPABILITY_SIZE 32 > +#define UCSI_SET_UOM 0x08 > +#define UCSI_SET_UOR 0x09 > +#define UCSI_SET_PDM 0x0a > +#define UCSI_SET_PDR 0x0b > +#define UCSI_GET_ALTERNATE_MODES 0x0c > +#define UCSI_GET_CAM_SUPPORTED 0x0d > +#define UCSI_GET_CURRENT_CAM 0x0e > +#define UCSI_SET_NEW_CAM 0x0f > +#define UCSI_GET_PDOS 0x10 > +#define UCSI_GET_CABLE_PROPERTY 0x11 > +#define UCSI_GET_CABLE_PROPERTY_SIZE 64 > +#define UCSI_GET_CONNECTOR_STATUS 0x12 > +#define UCSI_GET_CONNECTOR_STATUS_SIZE 152 > +#define UCSI_GET_ERROR_STATUS 0x13 > +#define UCSI_GET_PD_MESSAGE 0x15 > > #define UCSI_CONNECTOR_NUMBER(_num_) ((u64)(_num_) << 16) > #define UCSI_COMMAND(_cmd_) ((_cmd_) & 0xff) > @@ -250,42 +255,6 @@ struct ucsi_capability { > u16 typec_version; > } __packed; > > -/* Data structure filled by PPM in response to GET_CONNECTOR_CAPABILITY cmd. */ > -struct ucsi_connector_capability { > - u8 op_mode; > -#define UCSI_CONCAP_OPMODE_DFP BIT(0) > -#define UCSI_CONCAP_OPMODE_UFP BIT(1) > -#define UCSI_CONCAP_OPMODE_DRP BIT(2) > -#define UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY BIT(3) > -#define UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY BIT(4) > -#define UCSI_CONCAP_OPMODE_USB2 BIT(5) > -#define UCSI_CONCAP_OPMODE_USB3 BIT(6) > -#define UCSI_CONCAP_OPMODE_ALT_MODE BIT(7) > - u32 flags; > -#define UCSI_CONCAP_FLAG_PROVIDER BIT(0) > -#define UCSI_CONCAP_FLAG_CONSUMER BIT(1) > -#define UCSI_CONCAP_FLAG_SWAP_TO_DFP BIT(2) > -#define UCSI_CONCAP_FLAG_SWAP_TO_UFP BIT(3) > -#define UCSI_CONCAP_FLAG_SWAP_TO_SRC BIT(4) > -#define UCSI_CONCAP_FLAG_SWAP_TO_SINK BIT(5) > -#define UCSI_CONCAP_FLAG_EX_OP_MODE(_f_) \ > - (((_f_) & GENMASK(13, 6)) >> 6) > -#define UCSI_CONCAP_EX_OP_MODE_USB4_GEN2 BIT(0) > -#define UCSI_CONCAP_EX_OP_MODE_EPR_SRC BIT(1) > -#define UCSI_CONCAP_EX_OP_MODE_EPR_SINK BIT(2) > -#define UCSI_CONCAP_EX_OP_MODE_USB4_GEN3 BIT(3) > -#define UCSI_CONCAP_EX_OP_MODE_USB4_GEN4 BIT(4) > -#define UCSI_CONCAP_FLAG_MISC_CAPS(_f_) \ > - (((_f_) & GENMASK(17, 14)) >> 14) > -#define UCSI_CONCAP_MISC_CAP_FW_UPDATE BIT(0) > -#define UCSI_CONCAP_MISC_CAP_SECURITY BIT(1) > -#define UCSI_CONCAP_FLAG_REV_CURR_PROT_SUPPORT BIT(18) > -#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_) \ > - (((_f_) & GENMASK(20, 19)) >> 19) > -#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(_f_) \ > - UCSI_SPEC_REVISION_TO_BCD(UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_)) > -} __packed; > - > struct ucsi_altmode { > u16 svid; > u32 mid; > @@ -311,49 +280,103 @@ struct ucsi_cable_property { > u8 latency; > } __packed; > > -/* Data structure filled by PPM in response to GET_CONNECTOR_STATUS command. */ > -struct ucsi_connector_status { > - u16 change; > -#define UCSI_CONSTAT_EXT_SUPPLY_CHANGE BIT(1) > -#define UCSI_CONSTAT_POWER_OPMODE_CHANGE BIT(2) > -#define UCSI_CONSTAT_PDOS_CHANGE BIT(5) > -#define UCSI_CONSTAT_POWER_LEVEL_CHANGE BIT(6) > -#define UCSI_CONSTAT_PD_RESET_COMPLETE BIT(7) > -#define UCSI_CONSTAT_CAM_CHANGE BIT(8) > -#define UCSI_CONSTAT_BC_CHANGE BIT(9) > -#define UCSI_CONSTAT_PARTNER_CHANGE BIT(11) > -#define UCSI_CONSTAT_POWER_DIR_CHANGE BIT(12) > -#define UCSI_CONSTAT_CONNECT_CHANGE BIT(14) > -#define UCSI_CONSTAT_ERROR BIT(15) > - u16 flags; > -#define UCSI_CONSTAT_PWR_OPMODE(_f_) ((_f_) & GENMASK(2, 0)) > +/* Get Connector Capability Fields. */ > +#define UCSI_CONCAP_OPMODE DECLARE_BITFIELD(0, 0, 8) > +#define UCSI_CONCAP_OPMODE_DFP DECLARE_BITFIELD(0, 0, 1) > +#define UCSI_CONCAP_OPMODE_UFP DECLARE_BITFIELD(0, 1, 1) > +#define UCSI_CONCAP_OPMODE_DRP DECLARE_BITFIELD(0, 2, 1) > +#define UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY DECLARE_BITFIELD(0, 3, 1) > +#define UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY DECLARE_BITFIELD(0, 4, 1) > +#define UCSI_CONCAP_OPMODE_USB2 DECLARE_BITFIELD(0, 5, 1) > +#define UCSI_CONCAP_OPMODE_USB3 DECLARE_BITFIELD(0, 6, 1) > +#define UCSI_CONCAP_OPMODE_ALT_MODE DECLARE_BITFIELD(0, 7, 1) > +#define UCSI_CONCAP_PROVIDER DECLARE_BITFIELD(0, 8, 1) > +#define UCSI_CONCAP_CONSUMER DECLARE_BITFIELD(0, 9, 1) > +#define UCSI_CONCAP_SWAP_TO_DFP DECLARE_BITFIELD(0x110, 10, 1) I'm guessing you put 0x110 because UCSI_VERSION_1_1 was really long but it's clearer what the values mean. We could have a few local #define of VER_ANY, VER_1_0, VER_1_1, VER_2_0, VER_2_1, and VER_3_0 to make this cleaner. > +#define UCSI_CONCAP_SWAP_TO_UFP DECLARE_BITFIELD(0x110, 11, 1) > +#define UCSI_CONCAP_SWAP_TO_SRC DECLARE_BITFIELD(0x110, 12, 1) > +#define UCSI_CONCAP_SWAP_TO_SNK DECLARE_BITFIELD(0x110, 13, 1) > +#define UCSI_CONCAP_EXT_OPMODE DECLARE_BITFIELD(0x200, 14, 8) > +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN2 DECLARE_BITFIELD(0x200, 14, 1) > +#define UCSI_CONCAP_EXT_OPMODE_EPR_SRC DECLARE_BITFIELD(0x200, 15, 1) > +#define UCSI_CONCAP_EXT_OPMODE_EPR_SINK DECLARE_BITFIELD(0x200, 16, 1) > +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN3 DECLARE_BITFIELD(0x200, 17, 1) > +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN4 DECLARE_BITFIELD(0x200, 18, 1) I think these should be defined as #define UCSI_CONCAP_EXT_OPMODE DECLARE_BITFIELD(0x200, 14, 8) as that's how they're defined in the spec (and matches the pattern of definitions in this file). One other goal for this refactor was to make sure it was obvious at the callsite when a field from a newer version was being used (so we could make sure version checks are being done). My suggestion: #define UCSI_CONCAP_V2_EXT_OPMODE DECLARE_BITFIELD(0x200, 14, 8) #define UCSI_CONCAP_V2(_con_, _field_) UCSI_CONCAP(_con_, V2_##_field_) #define UCSI_CONCAP_V2_1(...) #define UCSI_CONCAP_V3(...) This duplicates the versioning in declare_bitfield but also enforces that the caller must acknowledge they're using a field that requires a certain minimum version. > +#define UCSI_CONCAP_MISC DECLARE_BITFIELD(0x200, 22, 4) > +#define UCSI_CONCAP_MISC_FW_UPDATE DECLARE_BITFIELD(0x200, 22, 1) > +#define UCSI_CONCAP_MISC_SECURITY DECLARE_BITFIELD(0x200, 23, 1) > +#define UCSI_CONCAP_REV_CURR_PROT_SUPPORT DECLARE_BITFIELD(0x200, 26, 1) > +#define UCSI_CONCAP_PARTNER_PD_REVISION DECLARE_BITFIELD(0x210, 27, 2) > + > +/* Get Connector Status Fields. */ > +#define UCSI_CONSTAT_CHANGE DECLARE_BITFIELD(0, 0, 16) > +#define UCSI_CONSTAT_PWR_OPMODE DECLARE_BITFIELD(0, 16, 3) > #define UCSI_CONSTAT_PWR_OPMODE_NONE 0 > #define UCSI_CONSTAT_PWR_OPMODE_DEFAULT 1 > #define UCSI_CONSTAT_PWR_OPMODE_BC 2 > #define UCSI_CONSTAT_PWR_OPMODE_PD 3 > #define UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5 4 > #define UCSI_CONSTAT_PWR_OPMODE_TYPEC3_0 5 > -#define UCSI_CONSTAT_CONNECTED BIT(3) > -#define UCSI_CONSTAT_PWR_DIR BIT(4) > -#define UCSI_CONSTAT_PARTNER_FLAGS(_f_) (((_f_) & GENMASK(12, 5)) >> 5) > +#define UCSI_CONSTAT_CONNECTED DECLARE_BITFIELD(0, 19, 1) > +#define UCSI_CONSTAT_PWR_DIR DECLARE_BITFIELD(0, 20, 1) > +#define UCSI_CONSTAT_PARTNER_FLAGS DECLARE_BITFIELD(0, 21, 8) > #define UCSI_CONSTAT_PARTNER_FLAG_USB 1 > #define UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE 2 > -#define UCSI_CONSTAT_PARTNER_TYPE(_f_) (((_f_) & GENMASK(15, 13)) >> 13) > +#define UCSI_CONSTAT_PARTNER_TYPE DECLARE_BITFIELD(0, 29, 3) > #define UCSI_CONSTAT_PARTNER_TYPE_DFP 1 > #define UCSI_CONSTAT_PARTNER_TYPE_UFP 2 > -#define UCSI_CONSTAT_PARTNER_TYPE_CABLE 3 /* Powered Cable */ > -#define UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP 4 /* Powered Cable */ > +#define UCSI_CONSTAT_PARTNER_TYPE_CABLE 3 /* Powered Cable */ > +#define UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP 4 /* Powered Cable */ > #define UCSI_CONSTAT_PARTNER_TYPE_DEBUG 5 > #define UCSI_CONSTAT_PARTNER_TYPE_AUDIO 6 > - u32 request_data_obj; > - > - u8 pwr_status; > -#define UCSI_CONSTAT_BC_STATUS(_p_) ((_p_) & GENMASK(1, 0)) > +#define UCSI_CONSTAT_RDO DECLARE_BITFIELD(0, 32, 32) > +#define UCSI_CONSTAT_BC_STATUS DECLARE_BITFIELD(0, 64, 2) > #define UCSI_CONSTAT_BC_NOT_CHARGING 0 > #define UCSI_CONSTAT_BC_NOMINAL_CHARGING 1 > #define UCSI_CONSTAT_BC_SLOW_CHARGING 2 > #define UCSI_CONSTAT_BC_TRICKLE_CHARGING 3 > -} __packed; > +#define UCSI_CONSTAT_PD_VERSION DECLARE_BITFIELD(0x120, 70, 16) > + > +/* Connector Status Change Bits. */ > +#define UCSI_CONSTAT_EXT_SUPPLY_CHANGE BIT(1) > +#define UCSI_CONSTAT_POWER_OPMODE_CHANGE BIT(2) > +#define UCSI_CONSTAT_PDOS_CHANGE BIT(5) > +#define UCSI_CONSTAT_POWER_LEVEL_CHANGE BIT(6) > +#define UCSI_CONSTAT_PD_RESET_COMPLETE BIT(7) > +#define UCSI_CONSTAT_CAM_CHANGE BIT(8) > +#define UCSI_CONSTAT_BC_CHANGE BIT(9) > +#define UCSI_CONSTAT_PARTNER_CHANGE BIT(11) > +#define UCSI_CONSTAT_POWER_DIR_CHANGE BIT(12) > +#define UCSI_CONSTAT_CONNECT_CHANGE BIT(14) > +#define UCSI_CONSTAT_ERROR BIT(15) > + > +#define bitfield_read(_map_, _field_, _ver_) \ > + ({ \ > + struct bitfield f = (_field_); \ > + WARN((_ver_) < f.version, \ > + "Access to unsupported field.") ? 0 : \ > + bitmap_read((_map_), f.offset, f.size); \ > + }) Suggestion WARN((_ver_) < f.version, "Access to unsupported field at offset 0x%x (need version %04x)", f.offset, f.version) Will make it easier to debug which offsets are failing and why. I think we already print the current version during driver startup via dev_dbg. > + > +#define DECLARE_BITFIELD(_ver_, _offset_, _size_) \ > +(struct bitfield) { \ > + .version = _ver_, \ > + .offset = _offset_, \ > + .size = _size_ \ > +} > + > +struct bitfield { > + const u16 version; > + const u8 offset; > + const u8 size; > +}; > + > +/* Helpers to access cached command responses. */ > +#define UCSI_CONCAP(_con_, _field_) \ > + bitfield_read((_con_)->status, UCSI_CONCAP_##_field_, (_con_)->ucsi->version) This should be (_con_)->cap. > + > +#define UCSI_CONSTAT(_con_, _field_) \ > + bitfield_read((_con_)->status, UCSI_CONSTAT_##_field_, (_con_)->ucsi->version) > > /* -------------------------------------------------------------------------- */ > > @@ -433,8 +456,10 @@ struct ucsi_connector { > > struct typec_capability typec_cap; > > - struct ucsi_connector_status status; > - struct ucsi_connector_capability cap; > + /* Cached command responses. */ > + DECLARE_BITMAP(cap, UCSI_GET_CONNECTOR_CAPABILITY_SIZE); > + DECLARE_BITMAP(status, UCSI_GET_CONNECTOR_STATUS_SIZE); > + > struct power_supply *psy; > struct power_supply_desc psy_desc; > u32 rdo; > diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c > index 7a5dff8d9cc6..48ee9e4cac3b 100644 > --- a/drivers/usb/typec/ucsi/ucsi_acpi.c > +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c > @@ -143,7 +143,6 @@ static int ucsi_gram_read_message_in(struct ucsi *ucsi, void *val, size_t val_le > u16 bogus_change = UCSI_CONSTAT_POWER_LEVEL_CHANGE | > UCSI_CONSTAT_PDOS_CHANGE; > struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > - struct ucsi_connector_status *status; > int ret; > > ret = ucsi_acpi_read_message_in(ucsi, val, val_len); > @@ -152,11 +151,9 @@ static int ucsi_gram_read_message_in(struct ucsi *ucsi, void *val, size_t val_le > > if (UCSI_COMMAND(ua->cmd) == UCSI_GET_CONNECTOR_STATUS && > ua->check_bogus_event) { > - status = (struct ucsi_connector_status *)val; > - > /* Clear the bogus change */ > - if (status->change == bogus_change) > - status->change = 0; > + if (*(u16 *)val == bogus_change) > + *(u16 *)val = 0; > > ua->check_bogus_event = false; > } > -- > 2.45.2 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v2 2/2] usb: typec: ucsi: Convert connector specific commands to bitmaps 2024-09-04 5:16 ` Abhishek Pandit-Subedi @ 2024-09-05 12:36 ` Heikki Krogerus 0 siblings, 0 replies; 8+ messages in thread From: Heikki Krogerus @ 2024-09-05 12:36 UTC (permalink / raw) To: Abhishek Pandit-Subedi Cc: Jameson Thies, Benson Leung, Bartosz Szpila, Pilla, Siva sai kumar, linux-usb On Tue, Sep 03, 2024 at 10:16:36PM -0700, Abhishek Pandit-Subedi wrote: > On Tue, Sep 3, 2024 at 7:53 AM Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > This is hopefully a bit less hacky, but it still needs work. > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > --- > > drivers/usb/typec/ucsi/psy.c | 28 ++--- > > drivers/usb/typec/ucsi/trace.h | 28 ++--- > > drivers/usb/typec/ucsi/ucsi.c | 108 ++++++++-------- > > drivers/usb/typec/ucsi/ucsi.h | 195 ++++++++++++++++------------- > > drivers/usb/typec/ucsi/ucsi_acpi.c | 7 +- > > 5 files changed, 192 insertions(+), 174 deletions(-) > > > > diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c > > index e623d80e177c..4d5398afa7a2 100644 > > --- a/drivers/usb/typec/ucsi/psy.c > > +++ b/drivers/usb/typec/ucsi/psy.c > > @@ -55,8 +55,8 @@ static int ucsi_psy_get_online(struct ucsi_connector *con, > > union power_supply_propval *val) > > { > > val->intval = UCSI_PSY_OFFLINE; > > - if (con->status.flags & UCSI_CONSTAT_CONNECTED && > > - (con->status.flags & UCSI_CONSTAT_PWR_DIR) == TYPEC_SINK) > > + if (UCSI_CONSTAT(con, CONNECTED) && > > + (UCSI_CONSTAT(con, PWR_DIR) == TYPEC_SINK)) > > val->intval = UCSI_PSY_FIXED_ONLINE; > > return 0; > > } > > @@ -66,7 +66,7 @@ static int ucsi_psy_get_voltage_min(struct ucsi_connector *con, > > { > > u32 pdo; > > > > - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { > > + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { > > case UCSI_CONSTAT_PWR_OPMODE_PD: > > pdo = con->src_pdos[0]; > > val->intval = pdo_fixed_voltage(pdo) * 1000; > > @@ -89,7 +89,7 @@ static int ucsi_psy_get_voltage_max(struct ucsi_connector *con, > > { > > u32 pdo; > > > > - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { > > + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { > > case UCSI_CONSTAT_PWR_OPMODE_PD: > > if (con->num_pdos > 0) { > > pdo = con->src_pdos[con->num_pdos - 1]; > > @@ -117,7 +117,7 @@ static int ucsi_psy_get_voltage_now(struct ucsi_connector *con, > > int index; > > u32 pdo; > > > > - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { > > + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { > > case UCSI_CONSTAT_PWR_OPMODE_PD: > > index = rdo_index(con->rdo); > > if (index > 0) { > > @@ -145,7 +145,7 @@ static int ucsi_psy_get_current_max(struct ucsi_connector *con, > > { > > u32 pdo; > > > > - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { > > + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { > > case UCSI_CONSTAT_PWR_OPMODE_PD: > > if (con->num_pdos > 0) { > > pdo = con->src_pdos[con->num_pdos - 1]; > > @@ -173,9 +173,7 @@ static int ucsi_psy_get_current_max(struct ucsi_connector *con, > > static int ucsi_psy_get_current_now(struct ucsi_connector *con, > > union power_supply_propval *val) > > { > > - u16 flags = con->status.flags; > > - > > - if (UCSI_CONSTAT_PWR_OPMODE(flags) == UCSI_CONSTAT_PWR_OPMODE_PD) > > + if (UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) > > val->intval = rdo_op_current(con->rdo) * 1000; > > else > > val->intval = 0; > > @@ -185,11 +183,9 @@ static int ucsi_psy_get_current_now(struct ucsi_connector *con, > > static int ucsi_psy_get_usb_type(struct ucsi_connector *con, > > union power_supply_propval *val) > > { > > - u16 flags = con->status.flags; > > - > > val->intval = POWER_SUPPLY_USB_TYPE_C; > > - if (flags & UCSI_CONSTAT_CONNECTED && > > - UCSI_CONSTAT_PWR_OPMODE(flags) == UCSI_CONSTAT_PWR_OPMODE_PD) > > + if (UCSI_CONSTAT(con, CONNECTED) && > > + UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) > > val->intval = POWER_SUPPLY_USB_TYPE_PD; > > > > return 0; > > @@ -197,18 +193,18 @@ static int ucsi_psy_get_usb_type(struct ucsi_connector *con, > > > > static int ucsi_psy_get_charge_type(struct ucsi_connector *con, union power_supply_propval *val) > > { > > - if (!(con->status.flags & UCSI_CONSTAT_CONNECTED)) { > > + if (!(UCSI_CONSTAT(con, CONNECTED))) { > > val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE; > > return 0; > > } > > > > /* The Battery Charging Cabability Status field is only valid in sink role. */ > > - if ((con->status.flags & UCSI_CONSTAT_PWR_DIR) != TYPEC_SINK) { > > + if (UCSI_CONSTAT(con, PWR_DIR) != TYPEC_SINK) { > > val->intval = POWER_SUPPLY_CHARGE_TYPE_UNKNOWN; > > return 0; > > } > > > > - switch (UCSI_CONSTAT_BC_STATUS(con->status.pwr_status)) { > > + switch (UCSI_CONSTAT(con, BC_STATUS)) { > > case UCSI_CONSTAT_BC_NOMINAL_CHARGING: > > val->intval = POWER_SUPPLY_CHARGE_TYPE_STANDARD; > > break; > > diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h > > index a0d3a934d3d9..41701dee7056 100644 > > --- a/drivers/usb/typec/ucsi/trace.h > > +++ b/drivers/usb/typec/ucsi/trace.h > > @@ -40,8 +40,8 @@ DEFINE_EVENT(ucsi_log_command, ucsi_reset_ppm, > > ); > > > > DECLARE_EVENT_CLASS(ucsi_log_connector_status, > > - TP_PROTO(int port, struct ucsi_connector_status *status), > > - TP_ARGS(port, status), > > + TP_PROTO(int port, struct ucsi_connector *con), > > + TP_ARGS(port, con), > > TP_STRUCT__entry( > > __field(int, port) > > __field(u16, change) > > @@ -55,14 +55,14 @@ DECLARE_EVENT_CLASS(ucsi_log_connector_status, > > ), > > TP_fast_assign( > > __entry->port = port - 1; > > - __entry->change = status->change; > > - __entry->opmode = UCSI_CONSTAT_PWR_OPMODE(status->flags); > > - __entry->connected = !!(status->flags & UCSI_CONSTAT_CONNECTED); > > - __entry->pwr_dir = !!(status->flags & UCSI_CONSTAT_PWR_DIR); > > - __entry->partner_flags = UCSI_CONSTAT_PARTNER_FLAGS(status->flags); > > - __entry->partner_type = UCSI_CONSTAT_PARTNER_TYPE(status->flags); > > - __entry->request_data_obj = status->request_data_obj; > > - __entry->bc_status = UCSI_CONSTAT_BC_STATUS(status->pwr_status); > > + __entry->change = UCSI_CONSTAT(con, CHANGE); > > + __entry->opmode = UCSI_CONSTAT(con, PWR_OPMODE); > > + __entry->connected = UCSI_CONSTAT(con, CONNECTED); > > + __entry->pwr_dir = UCSI_CONSTAT(con, PWR_DIR); > > + __entry->partner_flags = UCSI_CONSTAT(con, PARTNER_FLAGS); > > + __entry->partner_type = UCSI_CONSTAT(con, PARTNER_TYPE); > > + __entry->request_data_obj = UCSI_CONSTAT(con, RDO); > > + __entry->bc_status = UCSI_CONSTAT(con, BC_STATUS); > > ), > > TP_printk("port%d status: change=%04x, opmode=%x, connected=%d, " > > "sourcing=%d, partner_flags=%x, partner_type=%x, " > > @@ -73,13 +73,13 @@ DECLARE_EVENT_CLASS(ucsi_log_connector_status, > > ); > > > > DEFINE_EVENT(ucsi_log_connector_status, ucsi_connector_change, > > - TP_PROTO(int port, struct ucsi_connector_status *status), > > - TP_ARGS(port, status) > > + TP_PROTO(int port, struct ucsi_connector *con), > > + TP_ARGS(port, con) > > ); > > > > DEFINE_EVENT(ucsi_log_connector_status, ucsi_register_port, > > - TP_PROTO(int port, struct ucsi_connector_status *status), > > - TP_ARGS(port, status) > > + TP_PROTO(int port, struct ucsi_connector *con), > > + TP_ARGS(port, con) > > ); > > > > DECLARE_EVENT_CLASS(ucsi_log_register_altmode, > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > > index 71cf64a424d9..b03eace5a8f0 100644 > > --- a/drivers/usb/typec/ucsi/ucsi.c > > +++ b/drivers/usb/typec/ucsi/ucsi.c > > @@ -649,10 +649,11 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient) > > static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack) > > { > > u64 command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num); > > - struct ucsi *ucsi = con->ucsi; > > + size_t size = min(UCSI_GET_CONNECTOR_STATUS_SIZE, UCSI_MAX_DATA_LENGTH(con->ucsi)); > > int ret; > > > > - ret = ucsi_send_command_common(ucsi, command, &con->status, sizeof(con->status), conn_ack); > > + ret = ucsi_send_command_common(con->ucsi, command, &con->status, size, conn_ack); > > + > > return ret < 0 ? ret : 0; > > } > > > > @@ -666,8 +667,7 @@ static int ucsi_read_pdos(struct ucsi_connector *con, > > > > if (is_partner && > > ucsi->quirks & UCSI_NO_PARTNER_PDOS && > > - ((con->status.flags & UCSI_CONSTAT_PWR_DIR) || > > - !is_source(role))) > > + (UCSI_CONSTAT(con, PWR_DIR) || !is_source(role))) > > return 0; > > > > command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num); > > @@ -995,16 +995,16 @@ static int ucsi_check_connector_capability(struct ucsi_connector *con) > > } > > > > typec_partner_set_pd_revision(con->partner, > > - UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags)); > > + UCSI_SPEC_REVISION_TO_BCD(UCSI_CONCAP(con, PARTNER_PD_REVISION))); > > > > return ret; > > } > > > > static void ucsi_pwr_opmode_change(struct ucsi_connector *con) > > { > > - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) { > > + switch (UCSI_CONSTAT(con, PWR_OPMODE)) { > > case UCSI_CONSTAT_PWR_OPMODE_PD: > > - con->rdo = con->status.request_data_obj; > > + con->rdo = UCSI_CONSTAT(con, RDO); > > typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD); > > ucsi_partner_task(con, ucsi_get_src_pdos, 30, 0); > > ucsi_partner_task(con, ucsi_check_altmodes, 30, HZ); > > @@ -1028,7 +1028,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con) > > > > static int ucsi_register_partner(struct ucsi_connector *con) > > { > > - u8 pwr_opmode = UCSI_CONSTAT_PWR_OPMODE(con->status.flags); > > + u8 pwr_opmode = UCSI_CONSTAT(con, PWR_OPMODE); > > struct typec_partner_desc desc; > > struct typec_partner *partner; > > > > @@ -1037,7 +1037,7 @@ static int ucsi_register_partner(struct ucsi_connector *con) > > > > memset(&desc, 0, sizeof(desc)); > > > > - switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) { > > + switch (UCSI_CONSTAT(con, PARTNER_TYPE)) { > > case UCSI_CONSTAT_PARTNER_TYPE_DEBUG: > > desc.accessory = TYPEC_ACCESSORY_DEBUG; > > break; > > @@ -1055,6 +1055,9 @@ static int ucsi_register_partner(struct ucsi_connector *con) > > desc.identity = &con->partner_identity; > > desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD; > > > > + if (con->ucsi->version >= UCSI_VERSION_2_1) > > + desc.pd_revision = UCSI_SPEC_REVISION_TO_BCD(UCSI_CONCAP(con, PARTNER_PD_REVISION)); > > + > > partner = typec_register_partner(con->port, &desc); > > if (IS_ERR(partner)) { > > dev_err(con->ucsi->dev, > > @@ -1089,7 +1092,7 @@ static void ucsi_partner_change(struct ucsi_connector *con) > > enum usb_role u_role = USB_ROLE_NONE; > > int ret; > > > > - switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) { > > + switch (UCSI_CONSTAT(con, PARTNER_TYPE)) { > > case UCSI_CONSTAT_PARTNER_TYPE_UFP: > > case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP: > > u_role = USB_ROLE_HOST; > > @@ -1105,8 +1108,8 @@ static void ucsi_partner_change(struct ucsi_connector *con) > > break; > > } > > > > - if (con->status.flags & UCSI_CONSTAT_CONNECTED) { > > - switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) { > > + if (UCSI_CONSTAT(con, CONNECTED)) { > > + switch (UCSI_CONSTAT(con, PARTNER_TYPE)) { > > case UCSI_CONSTAT_PARTNER_TYPE_DEBUG: > > typec_set_mode(con->port, TYPEC_MODE_DEBUG); > > break; > > @@ -1114,14 +1117,14 @@ static void ucsi_partner_change(struct ucsi_connector *con) > > typec_set_mode(con->port, TYPEC_MODE_AUDIO); > > break; > > default: > > - if (UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) == > > - UCSI_CONSTAT_PARTNER_FLAG_USB) > > + if (UCSI_CONSTAT(con, PARTNER_FLAGS) == > > + UCSI_CONSTAT_PARTNER_FLAG_USB) > > typec_set_mode(con->port, TYPEC_STATE_USB); > > } > > } > > > > /* Only notify USB controller if partner supports USB data */ > > - if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) & UCSI_CONSTAT_PARTNER_FLAG_USB)) > > + if (!(UCSI_CONSTAT(con, PARTNER_FLAGS) & UCSI_CONSTAT_PARTNER_FLAG_USB)) > > u_role = USB_ROLE_NONE; > > > > ret = usb_role_switch_set_role(con->usb_role_sw, u_role); > > @@ -1132,7 +1135,7 @@ static void ucsi_partner_change(struct ucsi_connector *con) > > > > static int ucsi_check_connection(struct ucsi_connector *con) > > { > > - u8 prev_flags = con->status.flags; > > + u8 prev_state = UCSI_CONSTAT(con, CONNECTED); > > int ret; > > > > ret = ucsi_get_connector_status(con, false); > > @@ -1141,10 +1144,9 @@ static int ucsi_check_connection(struct ucsi_connector *con) > > return ret; > > } > > > > - if (con->status.flags == prev_flags) > > - return 0; > > - > > - if (con->status.flags & UCSI_CONSTAT_CONNECTED) { > > + if (UCSI_CONSTAT(con, CONNECTED)) { > > + if (prev_state) > > + return 0; > > ucsi_register_partner(con); > > ucsi_pwr_opmode_change(con); > > ucsi_partner_change(con); > > @@ -1200,6 +1202,7 @@ static void ucsi_handle_connector_change(struct work_struct *work) > > work); > > struct ucsi *ucsi = con->ucsi; > > enum typec_role role; > > + u16 change; > > int ret; > > > > mutex_lock(&con->lock); > > @@ -1212,14 +1215,15 @@ static void ucsi_handle_connector_change(struct work_struct *work) > > goto out_unlock; > > } > > > > - trace_ucsi_connector_change(con->num, &con->status); > > + trace_ucsi_connector_change(con->num, con); > > > > if (ucsi->ops->connector_status) > > ucsi->ops->connector_status(con); > > > > - role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR); > > + change = UCSI_CONSTAT(con, CHANGE); > > + role = UCSI_CONSTAT(con, PWR_DIR); > > > > - if (con->status.change & UCSI_CONSTAT_POWER_DIR_CHANGE) { > > + if (change & UCSI_CONSTAT_POWER_DIR_CHANGE) { > > typec_set_pwr_role(con->port, role); > > > > /* Complete pending power role swap */ > > @@ -1227,12 +1231,12 @@ static void ucsi_handle_connector_change(struct work_struct *work) > > complete(&con->complete); > > } > > > > - if (con->status.change & UCSI_CONSTAT_CONNECT_CHANGE) { > > + if (change & UCSI_CONSTAT_CONNECT_CHANGE) { > > typec_set_pwr_role(con->port, role); > > ucsi_port_psy_changed(con); > > ucsi_partner_change(con); > > > > - if (con->status.flags & UCSI_CONSTAT_CONNECTED) { > > + if (UCSI_CONSTAT(con, CONNECTED)) { > > ucsi_register_partner(con); > > ucsi_partner_task(con, ucsi_check_connection, 1, HZ); > > if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE) > > @@ -1240,8 +1244,7 @@ static void ucsi_handle_connector_change(struct work_struct *work) > > if (con->ucsi->cap.features & UCSI_CAP_CABLE_DETAILS) > > ucsi_partner_task(con, ucsi_check_cable, 1, HZ); > > > > - if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == > > - UCSI_CONSTAT_PWR_OPMODE_PD) { > > + if (UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) { > > ucsi_partner_task(con, ucsi_register_partner_pdos, 1, HZ); > > ucsi_partner_task(con, ucsi_check_connector_capability, 1, HZ); > > } > > @@ -1250,11 +1253,10 @@ static void ucsi_handle_connector_change(struct work_struct *work) > > } > > } > > > > - if (con->status.change & UCSI_CONSTAT_POWER_OPMODE_CHANGE || > > - con->status.change & UCSI_CONSTAT_POWER_LEVEL_CHANGE) > > + if (change & (UCSI_CONSTAT_POWER_OPMODE_CHANGE | UCSI_CONSTAT_POWER_LEVEL_CHANGE)) > > ucsi_pwr_opmode_change(con); > > > > - if (con->partner && con->status.change & UCSI_CONSTAT_PARTNER_CHANGE) { > > + if (con->partner && (change & UCSI_CONSTAT_PARTNER_CHANGE)) { > > ucsi_partner_change(con); > > > > /* Complete pending data role swap */ > > @@ -1262,10 +1264,10 @@ static void ucsi_handle_connector_change(struct work_struct *work) > > complete(&con->complete); > > } > > > > - if (con->status.change & UCSI_CONSTAT_CAM_CHANGE) > > + if (change & UCSI_CONSTAT_CAM_CHANGE) > > ucsi_partner_task(con, ucsi_check_altmodes, 1, HZ); > > > > - if (con->status.change & UCSI_CONSTAT_BC_CHANGE) > > + if (change & UCSI_CONSTAT_BC_CHANGE) > > ucsi_port_psy_changed(con); > > > > out_unlock: > > @@ -1425,7 +1427,7 @@ static int ucsi_dr_swap(struct typec_port *port, enum typec_data_role role) > > goto out_unlock; > > } > > > > - partner_type = UCSI_CONSTAT_PARTNER_TYPE(con->status.flags); > > + partner_type = UCSI_CONSTAT(con, PARTNER_TYPE); > > if ((partner_type == UCSI_CONSTAT_PARTNER_TYPE_DFP && > > role == TYPEC_DEVICE) || > > (partner_type == UCSI_CONSTAT_PARTNER_TYPE_UFP && > > @@ -1469,7 +1471,7 @@ static int ucsi_pr_swap(struct typec_port *port, enum typec_role role) > > goto out_unlock; > > } > > > > - cur_role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR); > > + cur_role = UCSI_CONSTAT(con, PWR_DIR); > > > > if (cur_role == role) > > goto out_unlock; > > @@ -1492,8 +1494,7 @@ static int ucsi_pr_swap(struct typec_port *port, enum typec_role role) > > mutex_lock(&con->lock); > > > > /* Something has gone wrong while swapping the role */ > > - if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) != > > - UCSI_CONSTAT_PWR_OPMODE_PD) { > > + if (UCSI_CONSTAT(con, PWR_OPMODE) != UCSI_CONSTAT_PWR_OPMODE_PD) { > > ucsi_reset_connector(con, true); > > ret = -EPROTO; > > } > > @@ -1561,19 +1562,18 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > > if (ret < 0) > > goto out_unlock; > > > > - if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP) > > + if (UCSI_CONCAP(con, OPMODE_DRP)) > > cap->data = TYPEC_PORT_DRD; > > - else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DFP) > > + else if (UCSI_CONCAP(con, OPMODE_DFP)) > > cap->data = TYPEC_PORT_DFP; > > - else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP) > > + else if (UCSI_CONCAP(con, OPMODE_UFP)) > > cap->data = TYPEC_PORT_UFP; > > > > - if ((con->cap.flags & UCSI_CONCAP_FLAG_PROVIDER) && > > - (con->cap.flags & UCSI_CONCAP_FLAG_CONSUMER)) > > + if (UCSI_CONCAP(con, PROVIDER) && UCSI_CONCAP(con, CONSUMER)) > > cap->type = TYPEC_PORT_DRP; > > - else if (con->cap.flags & UCSI_CONCAP_FLAG_PROVIDER) > > + else if (UCSI_CONCAP(con, PROVIDER)) > > cap->type = TYPEC_PORT_SRC; > > - else if (con->cap.flags & UCSI_CONCAP_FLAG_CONSUMER) > > + else if (UCSI_CONCAP(con, CONSUMER)) > > cap->type = TYPEC_PORT_SNK; > > > > cap->revision = ucsi->cap.typec_version; > > @@ -1581,9 +1581,9 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > > cap->svdm_version = SVDM_VER_2_0; > > cap->prefer_role = TYPEC_NO_PREFERRED_ROLE; > > > > - if (con->cap.op_mode & UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY) > > + if (UCSI_CONCAP(con, OPMODE_AUDIO_ACCESSORY)) > > *accessory++ = TYPEC_ACCESSORY_AUDIO; > > - if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY) > > + if (UCSI_CONCAP(con, OPMODE_DEBUG_ACCESSORY)) > > *accessory = TYPEC_ACCESSORY_DEBUG; > > > > cap->driver_data = con; > > @@ -1621,10 +1621,13 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > > goto out; > > } > > > > + printk("%s: con%d: 0x%lx\n", __func__, con->num, > > + UCSI_CONSTAT(con, PWR_OPMODE)); > > + > > if (ucsi->ops->connector_status) > > ucsi->ops->connector_status(con); > > > > - switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) { > > + switch (UCSI_CONSTAT(con, PARTNER_TYPE)) { > > case UCSI_CONSTAT_PARTNER_TYPE_UFP: > > case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP: > > u_role = USB_ROLE_HOST; > > @@ -1641,9 +1644,8 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > > } > > > > /* Check if there is already something connected */ > > - if (con->status.flags & UCSI_CONSTAT_CONNECTED) { > > - typec_set_pwr_role(con->port, > > - !!(con->status.flags & UCSI_CONSTAT_PWR_DIR)); > > + if (UCSI_CONSTAT(con, CONNECTED)) { > > + typec_set_pwr_role(con->port, UCSI_CONSTAT(con, PWR_DIR)); > > ucsi_register_partner(con); > > ucsi_pwr_opmode_change(con); > > ucsi_port_psy_changed(con); > > @@ -1654,7 +1656,7 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > > } > > > > /* Only notify USB controller if partner supports USB data */ > > - if (!(UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) & UCSI_CONSTAT_PARTNER_FLAG_USB)) > > + if (!(UCSI_CONSTAT(con, PARTNER_FLAGS) & UCSI_CONSTAT_PARTNER_FLAG_USB)) > > u_role = USB_ROLE_NONE; > > > > ret = usb_role_switch_set_role(con->usb_role_sw, u_role); > > @@ -1664,16 +1666,14 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > > ret = 0; > > } > > > > - if (con->partner && > > - UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == > > - UCSI_CONSTAT_PWR_OPMODE_PD) { > > + if (con->partner && UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) { > > ucsi_register_device_pdos(con); > > ucsi_get_src_pdos(con); > > ucsi_check_altmodes(con); > > ucsi_check_connector_capability(con); > > } > > > > - trace_ucsi_register_port(con->num, &con->status); > > + trace_ucsi_register_port(con->num, con); > > > > out: > > fwnode_handle_put(cap->fwnode); > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > > index 4a017eb6a65b..0429f8b71465 100644 > > --- a/drivers/usb/typec/ucsi/ucsi.h > > +++ b/drivers/usb/typec/ucsi/ucsi.h > > @@ -4,6 +4,7 @@ > > #define __DRIVER_USB_TYPEC_UCSI_H > > > > #include <linux/bitops.h> > > +#include <linux/bitmap.h> > > #include <linux/completion.h> > > #include <linux/device.h> > > #include <linux/power_supply.h> > > @@ -95,26 +96,30 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num); > > /* -------------------------------------------------------------------------- */ > > > > /* Commands */ > > -#define UCSI_PPM_RESET 0x01 > > -#define UCSI_CANCEL 0x02 > > -#define UCSI_CONNECTOR_RESET 0x03 > > -#define UCSI_ACK_CC_CI 0x04 > > -#define UCSI_SET_NOTIFICATION_ENABLE 0x05 > > -#define UCSI_GET_CAPABILITY 0x06 > > -#define UCSI_GET_CONNECTOR_CAPABILITY 0x07 > > -#define UCSI_SET_UOM 0x08 > > -#define UCSI_SET_UOR 0x09 > > -#define UCSI_SET_PDM 0x0a > > -#define UCSI_SET_PDR 0x0b > > -#define UCSI_GET_ALTERNATE_MODES 0x0c > > -#define UCSI_GET_CAM_SUPPORTED 0x0d > > -#define UCSI_GET_CURRENT_CAM 0x0e > > -#define UCSI_SET_NEW_CAM 0x0f > > -#define UCSI_GET_PDOS 0x10 > > -#define UCSI_GET_CABLE_PROPERTY 0x11 > > -#define UCSI_GET_CONNECTOR_STATUS 0x12 > > -#define UCSI_GET_ERROR_STATUS 0x13 > > -#define UCSI_GET_PD_MESSAGE 0x15 > > +#define UCSI_PPM_RESET 0x01 > > +#define UCSI_CANCEL 0x02 > > +#define UCSI_CONNECTOR_RESET 0x03 > > +#define UCSI_ACK_CC_CI 0x04 > > +#define UCSI_SET_NOTIFICATION_ENABLE 0x05 > > +#define UCSI_GET_CAPABILITY 0x06 > > +#define UCSI_GET_CAPABILITY_SIZE 128 > I don't think you use this yet. True. > > +#define UCSI_GET_CONNECTOR_CAPABILITY 0x07 > > +#define UCSI_GET_CONNECTOR_CAPABILITY_SIZE 32 > > +#define UCSI_SET_UOM 0x08 > > +#define UCSI_SET_UOR 0x09 > > +#define UCSI_SET_PDM 0x0a > > +#define UCSI_SET_PDR 0x0b > > +#define UCSI_GET_ALTERNATE_MODES 0x0c > > +#define UCSI_GET_CAM_SUPPORTED 0x0d > > +#define UCSI_GET_CURRENT_CAM 0x0e > > +#define UCSI_SET_NEW_CAM 0x0f > > +#define UCSI_GET_PDOS 0x10 > > +#define UCSI_GET_CABLE_PROPERTY 0x11 > > +#define UCSI_GET_CABLE_PROPERTY_SIZE 64 > > +#define UCSI_GET_CONNECTOR_STATUS 0x12 > > +#define UCSI_GET_CONNECTOR_STATUS_SIZE 152 > > +#define UCSI_GET_ERROR_STATUS 0x13 > > +#define UCSI_GET_PD_MESSAGE 0x15 > > > > #define UCSI_CONNECTOR_NUMBER(_num_) ((u64)(_num_) << 16) > > #define UCSI_COMMAND(_cmd_) ((_cmd_) & 0xff) > > @@ -250,42 +255,6 @@ struct ucsi_capability { > > u16 typec_version; > > } __packed; > > > > -/* Data structure filled by PPM in response to GET_CONNECTOR_CAPABILITY cmd. */ > > -struct ucsi_connector_capability { > > - u8 op_mode; > > -#define UCSI_CONCAP_OPMODE_DFP BIT(0) > > -#define UCSI_CONCAP_OPMODE_UFP BIT(1) > > -#define UCSI_CONCAP_OPMODE_DRP BIT(2) > > -#define UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY BIT(3) > > -#define UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY BIT(4) > > -#define UCSI_CONCAP_OPMODE_USB2 BIT(5) > > -#define UCSI_CONCAP_OPMODE_USB3 BIT(6) > > -#define UCSI_CONCAP_OPMODE_ALT_MODE BIT(7) > > - u32 flags; > > -#define UCSI_CONCAP_FLAG_PROVIDER BIT(0) > > -#define UCSI_CONCAP_FLAG_CONSUMER BIT(1) > > -#define UCSI_CONCAP_FLAG_SWAP_TO_DFP BIT(2) > > -#define UCSI_CONCAP_FLAG_SWAP_TO_UFP BIT(3) > > -#define UCSI_CONCAP_FLAG_SWAP_TO_SRC BIT(4) > > -#define UCSI_CONCAP_FLAG_SWAP_TO_SINK BIT(5) > > -#define UCSI_CONCAP_FLAG_EX_OP_MODE(_f_) \ > > - (((_f_) & GENMASK(13, 6)) >> 6) > > -#define UCSI_CONCAP_EX_OP_MODE_USB4_GEN2 BIT(0) > > -#define UCSI_CONCAP_EX_OP_MODE_EPR_SRC BIT(1) > > -#define UCSI_CONCAP_EX_OP_MODE_EPR_SINK BIT(2) > > -#define UCSI_CONCAP_EX_OP_MODE_USB4_GEN3 BIT(3) > > -#define UCSI_CONCAP_EX_OP_MODE_USB4_GEN4 BIT(4) > > -#define UCSI_CONCAP_FLAG_MISC_CAPS(_f_) \ > > - (((_f_) & GENMASK(17, 14)) >> 14) > > -#define UCSI_CONCAP_MISC_CAP_FW_UPDATE BIT(0) > > -#define UCSI_CONCAP_MISC_CAP_SECURITY BIT(1) > > -#define UCSI_CONCAP_FLAG_REV_CURR_PROT_SUPPORT BIT(18) > > -#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_) \ > > - (((_f_) & GENMASK(20, 19)) >> 19) > > -#define UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(_f_) \ > > - UCSI_SPEC_REVISION_TO_BCD(UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV(_f_)) > > -} __packed; > > - > > struct ucsi_altmode { > > u16 svid; > > u32 mid; > > @@ -311,49 +280,103 @@ struct ucsi_cable_property { > > u8 latency; > > } __packed; > > > > -/* Data structure filled by PPM in response to GET_CONNECTOR_STATUS command. */ > > -struct ucsi_connector_status { > > - u16 change; > > -#define UCSI_CONSTAT_EXT_SUPPLY_CHANGE BIT(1) > > -#define UCSI_CONSTAT_POWER_OPMODE_CHANGE BIT(2) > > -#define UCSI_CONSTAT_PDOS_CHANGE BIT(5) > > -#define UCSI_CONSTAT_POWER_LEVEL_CHANGE BIT(6) > > -#define UCSI_CONSTAT_PD_RESET_COMPLETE BIT(7) > > -#define UCSI_CONSTAT_CAM_CHANGE BIT(8) > > -#define UCSI_CONSTAT_BC_CHANGE BIT(9) > > -#define UCSI_CONSTAT_PARTNER_CHANGE BIT(11) > > -#define UCSI_CONSTAT_POWER_DIR_CHANGE BIT(12) > > -#define UCSI_CONSTAT_CONNECT_CHANGE BIT(14) > > -#define UCSI_CONSTAT_ERROR BIT(15) > > - u16 flags; > > -#define UCSI_CONSTAT_PWR_OPMODE(_f_) ((_f_) & GENMASK(2, 0)) > > +/* Get Connector Capability Fields. */ > > +#define UCSI_CONCAP_OPMODE DECLARE_BITFIELD(0, 0, 8) > > +#define UCSI_CONCAP_OPMODE_DFP DECLARE_BITFIELD(0, 0, 1) > > +#define UCSI_CONCAP_OPMODE_UFP DECLARE_BITFIELD(0, 1, 1) > > +#define UCSI_CONCAP_OPMODE_DRP DECLARE_BITFIELD(0, 2, 1) > > +#define UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY DECLARE_BITFIELD(0, 3, 1) > > +#define UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY DECLARE_BITFIELD(0, 4, 1) > > +#define UCSI_CONCAP_OPMODE_USB2 DECLARE_BITFIELD(0, 5, 1) > > +#define UCSI_CONCAP_OPMODE_USB3 DECLARE_BITFIELD(0, 6, 1) > > +#define UCSI_CONCAP_OPMODE_ALT_MODE DECLARE_BITFIELD(0, 7, 1) > > +#define UCSI_CONCAP_PROVIDER DECLARE_BITFIELD(0, 8, 1) > > +#define UCSI_CONCAP_CONSUMER DECLARE_BITFIELD(0, 9, 1) > > +#define UCSI_CONCAP_SWAP_TO_DFP DECLARE_BITFIELD(0x110, 10, 1) > > I'm guessing you put 0x110 because UCSI_VERSION_1_1 was really long > but it's clearer what the values mean. > We could have a few local #define of VER_ANY, VER_1_0, VER_1_1, > VER_2_0, VER_2_1, and VER_3_0 to make this cleaner. Yes, that's better. > > +#define UCSI_CONCAP_SWAP_TO_UFP DECLARE_BITFIELD(0x110, 11, 1) > > +#define UCSI_CONCAP_SWAP_TO_SRC DECLARE_BITFIELD(0x110, 12, 1) > > +#define UCSI_CONCAP_SWAP_TO_SNK DECLARE_BITFIELD(0x110, 13, 1) > > +#define UCSI_CONCAP_EXT_OPMODE DECLARE_BITFIELD(0x200, 14, 8) > > +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN2 DECLARE_BITFIELD(0x200, 14, 1) > > +#define UCSI_CONCAP_EXT_OPMODE_EPR_SRC DECLARE_BITFIELD(0x200, 15, 1) > > +#define UCSI_CONCAP_EXT_OPMODE_EPR_SINK DECLARE_BITFIELD(0x200, 16, 1) > > +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN3 DECLARE_BITFIELD(0x200, 17, 1) > > +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN4 DECLARE_BITFIELD(0x200, 18, 1) > > I think these should be defined as #define UCSI_CONCAP_EXT_OPMODE > DECLARE_BITFIELD(0x200, 14, 8) as that's how they're defined in the > spec (and matches the pattern of definitions in this file). > > One other goal for this refactor was to make sure it was obvious at > the callsite when a field from a newer version was being used (so we > could make sure version checks are being done). My suggestion: > > #define UCSI_CONCAP_V2_EXT_OPMODE DECLARE_BITFIELD(0x200, 14, 8) > #define UCSI_CONCAP_V2(_con_, _field_) UCSI_CONCAP(_con_, V2_##_field_) > #define UCSI_CONCAP_V2_1(...) > #define UCSI_CONCAP_V3(...) > > This duplicates the versioning in declare_bitfield but also enforces > that the caller must acknowledge they're using a field that requires a > certain minimum version. Agreed. > > +#define UCSI_CONCAP_MISC DECLARE_BITFIELD(0x200, 22, 4) > > +#define UCSI_CONCAP_MISC_FW_UPDATE DECLARE_BITFIELD(0x200, 22, 1) > > +#define UCSI_CONCAP_MISC_SECURITY DECLARE_BITFIELD(0x200, 23, 1) > > +#define UCSI_CONCAP_REV_CURR_PROT_SUPPORT DECLARE_BITFIELD(0x200, 26, 1) > > +#define UCSI_CONCAP_PARTNER_PD_REVISION DECLARE_BITFIELD(0x210, 27, 2) > > + > > +/* Get Connector Status Fields. */ > > +#define UCSI_CONSTAT_CHANGE DECLARE_BITFIELD(0, 0, 16) > > +#define UCSI_CONSTAT_PWR_OPMODE DECLARE_BITFIELD(0, 16, 3) > > #define UCSI_CONSTAT_PWR_OPMODE_NONE 0 > > #define UCSI_CONSTAT_PWR_OPMODE_DEFAULT 1 > > #define UCSI_CONSTAT_PWR_OPMODE_BC 2 > > #define UCSI_CONSTAT_PWR_OPMODE_PD 3 > > #define UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5 4 > > #define UCSI_CONSTAT_PWR_OPMODE_TYPEC3_0 5 > > -#define UCSI_CONSTAT_CONNECTED BIT(3) > > -#define UCSI_CONSTAT_PWR_DIR BIT(4) > > -#define UCSI_CONSTAT_PARTNER_FLAGS(_f_) (((_f_) & GENMASK(12, 5)) >> 5) > > +#define UCSI_CONSTAT_CONNECTED DECLARE_BITFIELD(0, 19, 1) > > +#define UCSI_CONSTAT_PWR_DIR DECLARE_BITFIELD(0, 20, 1) > > +#define UCSI_CONSTAT_PARTNER_FLAGS DECLARE_BITFIELD(0, 21, 8) > > #define UCSI_CONSTAT_PARTNER_FLAG_USB 1 > > #define UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE 2 > > -#define UCSI_CONSTAT_PARTNER_TYPE(_f_) (((_f_) & GENMASK(15, 13)) >> 13) > > +#define UCSI_CONSTAT_PARTNER_TYPE DECLARE_BITFIELD(0, 29, 3) > > #define UCSI_CONSTAT_PARTNER_TYPE_DFP 1 > > #define UCSI_CONSTAT_PARTNER_TYPE_UFP 2 > > -#define UCSI_CONSTAT_PARTNER_TYPE_CABLE 3 /* Powered Cable */ > > -#define UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP 4 /* Powered Cable */ > > +#define UCSI_CONSTAT_PARTNER_TYPE_CABLE 3 /* Powered Cable */ > > +#define UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP 4 /* Powered Cable */ > > #define UCSI_CONSTAT_PARTNER_TYPE_DEBUG 5 > > #define UCSI_CONSTAT_PARTNER_TYPE_AUDIO 6 > > - u32 request_data_obj; > > - > > - u8 pwr_status; > > -#define UCSI_CONSTAT_BC_STATUS(_p_) ((_p_) & GENMASK(1, 0)) > > +#define UCSI_CONSTAT_RDO DECLARE_BITFIELD(0, 32, 32) > > +#define UCSI_CONSTAT_BC_STATUS DECLARE_BITFIELD(0, 64, 2) > > #define UCSI_CONSTAT_BC_NOT_CHARGING 0 > > #define UCSI_CONSTAT_BC_NOMINAL_CHARGING 1 > > #define UCSI_CONSTAT_BC_SLOW_CHARGING 2 > > #define UCSI_CONSTAT_BC_TRICKLE_CHARGING 3 > > -} __packed; > > +#define UCSI_CONSTAT_PD_VERSION DECLARE_BITFIELD(0x120, 70, 16) > > + > > +/* Connector Status Change Bits. */ > > +#define UCSI_CONSTAT_EXT_SUPPLY_CHANGE BIT(1) > > +#define UCSI_CONSTAT_POWER_OPMODE_CHANGE BIT(2) > > +#define UCSI_CONSTAT_PDOS_CHANGE BIT(5) > > +#define UCSI_CONSTAT_POWER_LEVEL_CHANGE BIT(6) > > +#define UCSI_CONSTAT_PD_RESET_COMPLETE BIT(7) > > +#define UCSI_CONSTAT_CAM_CHANGE BIT(8) > > +#define UCSI_CONSTAT_BC_CHANGE BIT(9) > > +#define UCSI_CONSTAT_PARTNER_CHANGE BIT(11) > > +#define UCSI_CONSTAT_POWER_DIR_CHANGE BIT(12) > > +#define UCSI_CONSTAT_CONNECT_CHANGE BIT(14) > > +#define UCSI_CONSTAT_ERROR BIT(15) > > + > > +#define bitfield_read(_map_, _field_, _ver_) \ > > + ({ \ > > + struct bitfield f = (_field_); \ > > + WARN((_ver_) < f.version, \ > > + "Access to unsupported field.") ? 0 : \ > > + bitmap_read((_map_), f.offset, f.size); \ > > + }) > > Suggestion WARN((_ver_) < f.version, "Access to unsupported field at > offset 0x%x (need version %04x)", f.offset, f.version) > Will make it easier to debug which offsets are failing and why. I > think we already print the current version during driver startup via > dev_dbg. Okay. > > + > > +#define DECLARE_BITFIELD(_ver_, _offset_, _size_) \ > > +(struct bitfield) { \ > > + .version = _ver_, \ > > + .offset = _offset_, \ > > + .size = _size_ \ > > +} > > + > > +struct bitfield { > > + const u16 version; > > + const u8 offset; > > + const u8 size; > > +}; > > + > > +/* Helpers to access cached command responses. */ > > +#define UCSI_CONCAP(_con_, _field_) \ > > + bitfield_read((_con_)->status, UCSI_CONCAP_##_field_, (_con_)->ucsi->version) > > This should be (_con_)->cap. Good catch. > > + > > +#define UCSI_CONSTAT(_con_, _field_) \ > > + bitfield_read((_con_)->status, UCSI_CONSTAT_##_field_, (_con_)->ucsi->version) > > > > /* -------------------------------------------------------------------------- */ > > > > @@ -433,8 +456,10 @@ struct ucsi_connector { > > > > struct typec_capability typec_cap; > > > > - struct ucsi_connector_status status; > > - struct ucsi_connector_capability cap; > > + /* Cached command responses. */ > > + DECLARE_BITMAP(cap, UCSI_GET_CONNECTOR_CAPABILITY_SIZE); > > + DECLARE_BITMAP(status, UCSI_GET_CONNECTOR_STATUS_SIZE); > > + > > struct power_supply *psy; > > struct power_supply_desc psy_desc; > > u32 rdo; > > diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c > > index 7a5dff8d9cc6..48ee9e4cac3b 100644 > > --- a/drivers/usb/typec/ucsi/ucsi_acpi.c > > +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c > > @@ -143,7 +143,6 @@ static int ucsi_gram_read_message_in(struct ucsi *ucsi, void *val, size_t val_le > > u16 bogus_change = UCSI_CONSTAT_POWER_LEVEL_CHANGE | > > UCSI_CONSTAT_PDOS_CHANGE; > > struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > > - struct ucsi_connector_status *status; > > int ret; > > > > ret = ucsi_acpi_read_message_in(ucsi, val, val_len); > > @@ -152,11 +151,9 @@ static int ucsi_gram_read_message_in(struct ucsi *ucsi, void *val, size_t val_le > > > > if (UCSI_COMMAND(ua->cmd) == UCSI_GET_CONNECTOR_STATUS && > > ua->check_bogus_event) { > > - status = (struct ucsi_connector_status *)val; > > - > > /* Clear the bogus change */ > > - if (status->change == bogus_change) > > - status->change = 0; > > + if (*(u16 *)val == bogus_change) > > + *(u16 *)val = 0; > > > > ua->check_bogus_event = false; > > } Thanks for the review. Br, -- heikki ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v2 2/2] usb: typec: ucsi: Convert connector specific commands to bitmaps 2024-09-03 14:53 ` [RFC PATCH v2 2/2] usb: typec: ucsi: Convert connector specific commands to bitmaps Heikki Krogerus 2024-09-04 5:16 ` Abhishek Pandit-Subedi @ 2024-09-12 12:04 ` Dmitry Baryshkov 1 sibling, 0 replies; 8+ messages in thread From: Dmitry Baryshkov @ 2024-09-12 12:04 UTC (permalink / raw) To: Heikki Krogerus Cc: Abhishek Pandit-Subedi, Jameson Thies, Benson Leung, Bartosz Szpila, Pilla, Siva sai kumar, linux-usb On Tue, Sep 03, 2024 at 05:53:42PM GMT, Heikki Krogerus wrote: > This is hopefully a bit less hacky, but it still needs work. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/ucsi/psy.c | 28 ++--- > drivers/usb/typec/ucsi/trace.h | 28 ++--- > drivers/usb/typec/ucsi/ucsi.c | 108 ++++++++-------- > drivers/usb/typec/ucsi/ucsi.h | 195 ++++++++++++++++------------- > drivers/usb/typec/ucsi/ucsi_acpi.c | 7 +- > 5 files changed, 192 insertions(+), 174 deletions(-) > > +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN2 DECLARE_BITFIELD(0x200, 14, 1) > +#define UCSI_CONCAP_EXT_OPMODE_EPR_SRC DECLARE_BITFIELD(0x200, 15, 1) > +#define UCSI_CONCAP_EXT_OPMODE_EPR_SINK DECLARE_BITFIELD(0x200, 16, 1) > +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN3 DECLARE_BITFIELD(0x200, 17, 1) > +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN4 DECLARE_BITFIELD(0x200, 18, 1) > +#define UCSI_CONCAP_MISC DECLARE_BITFIELD(0x200, 22, 4) > +#define UCSI_CONCAP_MISC_FW_UPDATE DECLARE_BITFIELD(0x200, 22, 1) > +#define UCSI_CONCAP_MISC_SECURITY DECLARE_BITFIELD(0x200, 23, 1) > +#define UCSI_CONCAP_REV_CURR_PROT_SUPPORT DECLARE_BITFIELD(0x200, 26, 1) > +#define UCSI_CONCAP_PARTNER_PD_REVISION DECLARE_BITFIELD(0x210, 27, 2) I'd second the suggestion to use VER_something macros. Another option might be to use something like DECLARE_BITFIELD_1_10 and DECLARE_BITFIELD_2_00 > +#define bitfield_read(_map_, _field_, _ver_) \ > + ({ \ > + struct bitfield f = (_field_); \ > + WARN((_ver_) < f.version, \ > + "Access to unsupported field.") ? 0 : \ > + bitmap_read((_map_), f.offset, f.size); \ > + }) > + > +#define DECLARE_BITFIELD(_ver_, _offset_, _size_) \ > +(struct bitfield) { \ > + .version = _ver_, \ > + .offset = _offset_, \ > + .size = _size_ \ > +} > + > +struct bitfield { > + const u16 version; > + const u8 offset; > + const u8 size; > +}; I think these names are too generic. What about ucsi_bitfield / UCSI_DECLARE_BITFIELD? > + > +/* Helpers to access cached command responses. */ > +#define UCSI_CONCAP(_con_, _field_) \ > + bitfield_read((_con_)->status, UCSI_CONCAP_##_field_, (_con_)->ucsi->version) > + > +#define UCSI_CONSTAT(_con_, _field_) \ > + bitfield_read((_con_)->status, UCSI_CONSTAT_##_field_, (_con_)->ucsi->version) > > /* -------------------------------------------------------------------------- */ > > @@ -433,8 +456,10 @@ struct ucsi_connector { > > struct typec_capability typec_cap; > > - struct ucsi_connector_status status; > - struct ucsi_connector_capability cap; > + /* Cached command responses. */ > + DECLARE_BITMAP(cap, UCSI_GET_CONNECTOR_CAPABILITY_SIZE); > + DECLARE_BITMAP(status, UCSI_GET_CONNECTOR_STATUS_SIZE); > + > struct power_supply *psy; > struct power_supply_desc psy_desc; > u32 rdo; > diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c > index 7a5dff8d9cc6..48ee9e4cac3b 100644 > --- a/drivers/usb/typec/ucsi/ucsi_acpi.c > +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c > @@ -143,7 +143,6 @@ static int ucsi_gram_read_message_in(struct ucsi *ucsi, void *val, size_t val_le > u16 bogus_change = UCSI_CONSTAT_POWER_LEVEL_CHANGE | > UCSI_CONSTAT_PDOS_CHANGE; > struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > - struct ucsi_connector_status *status; > int ret; > > ret = ucsi_acpi_read_message_in(ucsi, val, val_len); > @@ -152,11 +151,9 @@ static int ucsi_gram_read_message_in(struct ucsi *ucsi, void *val, size_t val_le > > if (UCSI_COMMAND(ua->cmd) == UCSI_GET_CONNECTOR_STATUS && > ua->check_bogus_event) { > - status = (struct ucsi_connector_status *)val; > - > /* Clear the bogus change */ > - if (status->change == bogus_change) > - status->change = 0; > + if (*(u16 *)val == bogus_change) > + *(u16 *)val = 0; > > ua->check_bogus_event = false; > } > -- > 2.45.2 > -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-09-12 12:04 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-03 14:53 [RFC PATCH v2 0/2] usb: typec: ucsi: New way to handle GET_CONNECTOR_STATUS Heikki Krogerus 2024-09-03 14:53 ` [RFC PATCH v2 1/2] usb: typec: ucsi: Helper for Get Connector Status command Heikki Krogerus 2024-09-04 5:17 ` Abhishek Pandit-Subedi 2024-09-12 10:08 ` Dmitry Baryshkov 2024-09-03 14:53 ` [RFC PATCH v2 2/2] usb: typec: ucsi: Convert connector specific commands to bitmaps Heikki Krogerus 2024-09-04 5:16 ` Abhishek Pandit-Subedi 2024-09-05 12:36 ` Heikki Krogerus 2024-09-12 12:04 ` Dmitry Baryshkov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.