On Thu, Oct 31, 2024 at 8:42 AM Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> wrote: > > That allows the fields in those command data structures to > be easily validated. If an unsupported field is accessed, a > warning is generated. > > This will not force UCSI version checks to be made in every > place where these data structures are accessed, but it will > make it easier to pinpoint issues that are caused by the > unconditional accesses to those fields, and perhaps more > importantly, allow those issues to be noticed immediately. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > --- > Hi, > > This was an RFC that I send earlier [1]. I used Dmitry's proposal > for the macros, so they are now UCSI_DECLARE_BITFIELD_Vx_x. > > [1] https://lore.kernel.org/linux-usb/20240903145342.3449969-3-heikki.krogerus@xxxxxxxxxxxxxxx/ > > thanks, > --- > drivers/usb/typec/ucsi/psy.c | 28 ++-- > drivers/usb/typec/ucsi/trace.h | 28 ++-- > drivers/usb/typec/ucsi/ucsi.c | 114 +++++++------ > drivers/usb/typec/ucsi/ucsi.h | 252 +++++++++++++++++------------ > drivers/usb/typec/ucsi/ucsi_acpi.c | 7 +- > 5 files changed, 235 insertions(+), 194 deletions(-) > > diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c > index 1c631c7855a9..62ac69730405 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 e430a0ca4a2b..2249fa8a01df 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -651,10 +651,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; > } > > @@ -668,8 +669,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); > @@ -997,16 +997,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); > @@ -1030,7 +1030,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; > > @@ -1039,7 +1039,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; > @@ -1057,6 +1057,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, > @@ -1067,13 +1070,11 @@ static int ucsi_register_partner(struct ucsi_connector *con) > > con->partner = partner; > > - if ((con->ucsi->version >= UCSI_VERSION_3_0) && > - (UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) & > - UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN4)) > + if (con->ucsi->version >= UCSI_VERSION_3_0 && > + UCSI_CONSTAT(con, PARTNER_FLAG_USB4_GEN4)) > typec_partner_set_usb_mode(partner, USB_MODE_USB4); > - else if ((con->ucsi->version >= UCSI_VERSION_2_0) && > - (UCSI_CONSTAT_PARTNER_FLAGS(con->status.flags) & > - UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN3)) > + else if (con->ucsi->version >= UCSI_VERSION_2_0 && > + UCSI_CONSTAT(con, PARTNER_FLAG_USB4_GEN3)) > typec_partner_set_usb_mode(partner, USB_MODE_USB4); > > return 0; > @@ -1100,7 +1101,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; > @@ -1116,8 +1117,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; > @@ -1125,14 +1126,13 @@ 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_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_FLAG_USB))) > u_role = USB_ROLE_NONE; > > ret = usb_role_switch_set_role(con->usb_role_sw, u_role); > @@ -1143,7 +1143,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); > @@ -1152,10 +1152,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); > @@ -1211,6 +1210,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); > @@ -1227,14 +1227,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 */ > @@ -1242,12 +1243,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) > @@ -1255,8 +1256,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); > } > @@ -1265,11 +1265,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 */ > @@ -1277,10 +1276,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: > @@ -1440,7 +1439,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 && > @@ -1484,7 +1483,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; > @@ -1507,8 +1506,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; > } > @@ -1576,19 +1574,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; > @@ -1596,9 +1593,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; > > if (UCSI_CONCAP_USB2_SUPPORT(con)) > @@ -1646,7 +1643,7 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > 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; > @@ -1663,9 +1660,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); > @@ -1676,7 +1672,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_FLAG_USB))) > u_role = USB_ROLE_NONE; > > ret = usb_role_switch_set_role(con->usb_role_sw, u_role); > @@ -1686,16 +1682,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 b82dc4c16a0d..831b79a6a6e4 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,27 +96,31 @@ 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_SET_SINK_PATH 0x1c > +#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_SET_SINK_PATH 0x1c > > #define UCSI_CONNECTOR_NUMBER(_num_) ((u64)(_num_) << 16) > #define UCSI_COMMAND(_cmd_) ((_cmd_) & 0xff) > @@ -127,7 +132,6 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num); > #define UCSI_CONNECTOR_RESET_HARD_VER_1_0 BIT(23) /* Deprecated in v1.1 */ > #define UCSI_CONNECTOR_RESET_DATA_VER_2_0 BIT(23) /* Redefined in v2.0 */ > > - > /* ACK_CC_CI bits */ > #define UCSI_ACK_CONNECTOR_CHANGE BIT(16) > #define UCSI_ACK_COMMAND_COMPLETE BIT(17) > @@ -251,50 +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; > - > -#define UCSI_CONCAP_USB2_SUPPORT(_con_) ((_con_)->cap.op_mode & UCSI_CONCAP_OPMODE_USB2) > -#define UCSI_CONCAP_USB3_SUPPORT(_con_) ((_con_)->cap.op_mode & UCSI_CONCAP_OPMODE_USB3) > -#define UCSI_CONCAP_USB4_SUPPORT(_con_) \ > - ((_con_)->ucsi->version >= UCSI_VERSION_2_0 && \ > - ((_con_)->cap.flags & (UCSI_CONCAP_EX_OP_MODE_USB4_GEN2 | \ > - UCSI_CONCAP_EX_OP_MODE_USB4_GEN3 | \ > - UCSI_CONCAP_EX_OP_MODE_USB4_GEN4))) > - > struct ucsi_altmode { > u16 svid; > u32 mid; > @@ -320,51 +280,143 @@ 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 UCSI_DECLARE_BITFIELD(0, 0, 8) > +#define UCSI_CONCAP_OPMODE_DFP UCSI_DECLARE_BITFIELD(0, 0, 1) > +#define UCSI_CONCAP_OPMODE_UFP UCSI_DECLARE_BITFIELD(0, 1, 1) > +#define UCSI_CONCAP_OPMODE_DRP UCSI_DECLARE_BITFIELD(0, 2, 1) > +#define UCSI_CONCAP_OPMODE_AUDIO_ACCESSORY UCSI_DECLARE_BITFIELD(0, 3, 1) > +#define UCSI_CONCAP_OPMODE_DEBUG_ACCESSORY UCSI_DECLARE_BITFIELD(0, 4, 1) > +#define UCSI_CONCAP_OPMODE_USB2 UCSI_DECLARE_BITFIELD(0, 5, 1) > +#define UCSI_CONCAP_OPMODE_USB3 UCSI_DECLARE_BITFIELD(0, 6, 1) > +#define UCSI_CONCAP_OPMODE_ALT_MODE UCSI_DECLARE_BITFIELD(0, 7, 1) > +#define UCSI_CONCAP_PROVIDER UCSI_DECLARE_BITFIELD(0, 8, 1) > +#define UCSI_CONCAP_CONSUMER UCSI_DECLARE_BITFIELD(0, 9, 1) > +#define UCSI_CONCAP_SWAP_TO_DFP UCSI_DECLARE_BITFIELD_V1_1(10, 1) > +#define UCSI_CONCAP_SWAP_TO_UFP UCSI_DECLARE_BITFIELD_V1_1(11, 1) > +#define UCSI_CONCAP_SWAP_TO_SRC UCSI_DECLARE_BITFIELD_V1_1(12, 1) > +#define UCSI_CONCAP_SWAP_TO_SNK UCSI_DECLARE_BITFIELD_V1_1(13, 1) > +#define UCSI_CONCAP_EXT_OPMODE UCSI_DECLARE_BITFIELD_V2_0(14, 8) > +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN2 UCSI_DECLARE_BITFIELD_V2_0(14, 1) > +#define UCSI_CONCAP_EXT_OPMODE_EPR_SRC UCSI_DECLARE_BITFIELD_V2_0(15, 1) > +#define UCSI_CONCAP_EXT_OPMODE_EPR_SINK UCSI_DECLARE_BITFIELD_V2_0(16, 1) > +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN3 UCSI_DECLARE_BITFIELD_V2_0(17, 1) > +#define UCSI_CONCAP_EXT_OPMODE_USB4_GEN4 UCSI_DECLARE_BITFIELD_V2_0(18, 1) > +#define UCSI_CONCAP_MISC UCSI_DECLARE_BITFIELD_V2_0(22, 4) > +#define UCSI_CONCAP_MISC_FW_UPDATE UCSI_DECLARE_BITFIELD_V2_0(22, 1) > +#define UCSI_CONCAP_MISC_SECURITY UCSI_DECLARE_BITFIELD_V2_0(23, 1) > +#define UCSI_CONCAP_REV_CURR_PROT_SUPPORT UCSI_DECLARE_BITFIELD_V2_0(26, 1) > +#define UCSI_CONCAP_PARTNER_PD_REVISION UCSI_DECLARE_BITFIELD_V2_1(27, 2) > + > +/* Helpers for USB capability checks. */ > +#define UCSI_CONCAP_USB2_SUPPORT(_con_) UCSI_CONCAP((_con_), OPMODE_USB2) > +#define UCSI_CONCAP_USB3_SUPPORT(_con_) UCSI_CONCAP((_con_), OPMODE_USB3) > +#define UCSI_CONCAP_USB4_SUPPORT(_con_) \ > + ((_con_)->ucsi->version >= UCSI_VERSION_2_0 && \ > + (UCSI_CONCAP((_con_), EXT_OPMODE_USB4_GEN2) | \ > + UCSI_CONCAP((_con_), EXT_OPMODE_USB4_GEN3) | \ > + UCSI_CONCAP((_con_), EXT_OPMODE_USB4_GEN4))) > + > +/* Get Connector Status Fields. */ > +#define UCSI_CONSTAT_CHANGE UCSI_DECLARE_BITFIELD(0, 0, 16) > +#define UCSI_CONSTAT_PWR_OPMODE UCSI_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_PARTNER_FLAG_USB 1 > -#define UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE 2 > -#define UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN3 4 > -#define UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN4 8 > -#define UCSI_CONSTAT_PARTNER_TYPE(_f_) (((_f_) & GENMASK(15, 13)) >> 13) > +#define UCSI_CONSTAT_CONNECTED UCSI_DECLARE_BITFIELD(0, 19, 1) > +#define UCSI_CONSTAT_PWR_DIR UCSI_DECLARE_BITFIELD(0, 20, 1) > +#define UCSI_CONSTAT_PARTNER_FLAGS UCSI_DECLARE_BITFIELD(0, 21, 8) > +#define UCSI_CONSTAT_PARTNER_FLAG_USB UCSI_DECLARE_BITFIELD(0, 21, 1) > +#define UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE UCSI_DECLARE_BITFIELD(0, 22, 1) > +#define UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN3 UCSI_DECLARE_BITFIELD(0, 23, 1) > +#define UCSI_CONSTAT_PARTNER_FLAG_USB4_GEN4 UCSI_DECLARE_BITFIELD(0, 24, 1) > +#define UCSI_CONSTAT_PARTNER_TYPE UCSI_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 UCSI_DECLARE_BITFIELD(0, 32, 32) > +#define UCSI_CONSTAT_BC_STATUS UCSI_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 UCSI_DECLARE_BITFIELD_V1_2(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 UCSI_DECLARE_BITFIELD_V1_1(_offset_, _size_) \ > + UCSI_DECLARE_BITFIELD(UCSI_VERSION_1_1, (_offset_), (_size_)) > +#define UCSI_DECLARE_BITFIELD_V1_2(_offset_, _size_) \ > + UCSI_DECLARE_BITFIELD(UCSI_VERSION_1_2, (_offset_), (_size_)) > +#define UCSI_DECLARE_BITFIELD_V2_0(_offset_, _size_) \ > + UCSI_DECLARE_BITFIELD(UCSI_VERSION_2_0, (_offset_), (_size_)) > +#define UCSI_DECLARE_BITFIELD_V2_1(_offset_, _size_) \ > + UCSI_DECLARE_BITFIELD(UCSI_VERSION_2_1, (_offset_), (_size_)) > +#define UCSI_DECLARE_BITFIELD_V3_0(_offset_, _size_) \ > + UCSI_DECLARE_BITFIELD(UCSI_VERSION_3_0, (_offset_), (_size_)) > + > +#define UCSI_DECLARE_BITFIELD(_ver_, _offset_, _size_) \ > +(struct ucsi_bitfield) { \ > + .version = _ver_, \ > + .offset = _offset_, \ > + .size = _size_, \ > +} > + > +struct ucsi_bitfield { > + const u16 version; > + const u8 offset; > + const u8 size; > +}; > + > +/** > + * ucsi_bitfield_read - Read a field from UCSI command response > + * @_map_: UCSI command response > + * @_field_: The field offset in the response data structure > + * @_ver_: UCSI version where the field was introduced > + * > + * Reads the fields in the command responses by first checking that the field is > + * valid with the UCSI interface version that is used in the system. > + * @_ver_ is the minimum UCSI version for the @_field_. If the UCSI interface is > + * older than @_ver_, a warning is generated. > + * > + * Caveats: > + * - Removed fields are not checked - @_ver_ is just the minimum UCSI version. > + * > + * Returns the value of @_field_, or 0 when the UCSI interface is older than > + * @_ver_. > + */ > +#define ucsi_bitfield_read(_map_, _field_, _ver_) \ > +({ \ > + struct ucsi_bitfield f = (_field_); \ > + WARN((_ver_) < f.version, \ > + "Access to unsupported field at offset 0x%x (need version %04x)", \ > + f.offset, f.version) ? 0 : \ > + bitmap_read((_map_), f.offset, f.size); \ > +}) > + > +/* Helpers to access cached command responses. */ > +#define UCSI_CONCAP(_con_, _field_) \ > + ucsi_bitfield_read((_con_)->status, UCSI_CONCAP_##_field_, (_con_)->ucsi->version) > + > +#define UCSI_CONSTAT(_con_, _field_) \ > + ucsi_bitfield_read((_con_)->status, UCSI_CONSTAT_##_field_, (_con_)->ucsi->version) Repeating my comment from the RFC (https://lore.kernel.org/linux-usb/CANFp7mXKR1zY85SLZ3vVf6Ozk0abho_4dXwvREOuY=-3q4t09w@xxxxxxxxxxxxxx/): <quote> 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. </quote> We can add this as a follow-on since this patch is already pretty large but this adds a layer of compile-time protection + a clearer signal during code review that the field being accessed needs to be protected by reading the version. > > /* -------------------------------------------------------------------------- */ > > @@ -444,8 +496,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 789f67dd9f81..5c5515551963 100644 > --- a/drivers/usb/typec/ucsi/ucsi_acpi.c > +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c > @@ -104,7 +104,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); > @@ -113,11 +112,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 > Looks like you missed addressing a couple of my comments in the RFC (https://lore.kernel.org/linux-usb/CANFp7mXKR1zY85SLZ3vVf6Ozk0abho_4dXwvREOuY=-3q4t09w@xxxxxxxxxxxxxx/) Aside from that, looks good to me.
