On Wed, Aug 28, 2024 at 9:15 AM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
>
> GET_CONNECTOR_STATUS fields don't align naturally, which
> makes handling it with a data structure a bit awkward. The
> members of struct ucsi_connector_status don't match the
> actual GET_CONNECTOR_STATUS fields.
>
> Instead of using the custom data structure, simply accessing
> the fields directly. This introduces a helper UCSI_FIELD()
> for that purpose.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/typec/ucsi/psy.c | 28 +++++------
> drivers/usb/typec/ucsi/trace.h | 22 ++++----
> drivers/usb/typec/ucsi/ucsi.c | 81 ++++++++++++++----------------
> drivers/usb/typec/ucsi/ucsi.h | 62 +++++++++++++----------
> drivers/usb/typec/ucsi/ucsi_acpi.c | 7 +--
> 5 files changed, 98 insertions(+), 102 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c
> index e623d80e177c..76a646d154c1 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_CONNECTED(con->status) &&
> + (UCSI_CONSTAT_PWR_DIR(con->status) == 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_PWR_OPMODE(con->status)) {
> 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_PWR_OPMODE(con->status)) {
> 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_PWR_OPMODE(con->status)) {
> 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_PWR_OPMODE(con->status)) {
> 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_PWR_OPMODE(con->status) == 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_CONNECTED(con->status) &&
> + UCSI_CONSTAT_PWR_OPMODE(con->status) == 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_CONNECTED(con->status))) {
> 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_PWR_DIR(con->status) != TYPEC_SINK) {
> val->intval = POWER_SUPPLY_CHARGE_TYPE_UNKNOWN;
> return 0;
> }
>
> - switch (UCSI_CONSTAT_BC_STATUS(con->status.pwr_status)) {
> + switch (UCSI_CONSTAT_BC_STATUS(con->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..ad51d684ea20 100644
> --- a/drivers/usb/typec/ucsi/trace.h
> +++ b/drivers/usb/typec/ucsi/trace.h
> @@ -40,7 +40,7 @@ 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_PROTO(int port, u8 *status),
> TP_ARGS(port, status),
> TP_STRUCT__entry(
> __field(int, port)
> @@ -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_CHANGE(status);
> + __entry->opmode = UCSI_CONSTAT_PWR_OPMODE(status);
> + __entry->connected = UCSI_CONSTAT_CONNECTED(status);
> + __entry->pwr_dir = UCSI_CONSTAT_PWR_DIR(status);
> + __entry->partner_flags = UCSI_CONSTAT_PARTNER_FLAGS(status);
> + __entry->partner_type = UCSI_CONSTAT_PARTNER_TYPE(status);
> + __entry->request_data_obj = UCSI_CONSTAT_RDO(status);
> + __entry->bc_status = UCSI_CONSTAT_BC_STATUS(status);
> ),
> TP_printk("port%d status: change=%04x, opmode=%x, connected=%d, "
> "sourcing=%d, partner_flags=%x, partner_type=%x, "
> @@ -73,12 +73,12 @@ 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_PROTO(int port, u8 *status),
> TP_ARGS(port, status)
> );
>
> DEFINE_EVENT(ucsi_log_connector_status, ucsi_register_port,
> - TP_PROTO(int port, struct ucsi_connector_status *status),
> + TP_PROTO(int port, u8 *status),
> TP_ARGS(port, status)
> );
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 2e6773715efe..399ea2700d19 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -652,7 +652,9 @@ static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack)
> struct ucsi *ucsi = con->ucsi;
> int ret;
>
> - ret = ucsi_send_command_common(ucsi, command, &con->status, sizeof(con->status), conn_ack);
> + ret = ucsi_send_command_common(ucsi, command, &con->status,
> + min(sizeof(con->status), UCSI_MAX_DATA_LENGTH(ucsi)),
> + conn_ack);
> return ret < 0 ? ret : 0;
> }
This seems equivalent to what we were doing previously (clamping to
UCSI_MAX_DATA_LENGTH based on version). Do we not want to use exact
sizes by version?
>
> @@ -666,8 +668,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_PWR_DIR(con->status) || !is_source(role)))
> return 0;
>
> command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num);
> @@ -971,9 +972,9 @@ static void ucsi_unregister_cable(struct ucsi_connector *con)
>
> static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
> {
> - switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) {
> + switch (UCSI_CONSTAT_PWR_OPMODE(con->status)) {
> case UCSI_CONSTAT_PWR_OPMODE_PD:
> - con->rdo = con->status.request_data_obj;
> + con->rdo = UCSI_CONSTAT_RDO(con->status);
> 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);
> @@ -996,7 +997,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_PWR_OPMODE(con->status);
> struct typec_partner_desc desc;
> struct typec_partner *partner;
>
> @@ -1005,7 +1006,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_PARTNER_TYPE(con->status)) {
> case UCSI_CONSTAT_PARTNER_TYPE_DEBUG:
> desc.accessory = TYPEC_ACCESSORY_DEBUG;
> break;
> @@ -1058,7 +1059,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_PARTNER_TYPE(con->status)) {
> case UCSI_CONSTAT_PARTNER_TYPE_UFP:
> case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
> u_role = USB_ROLE_HOST;
> @@ -1074,8 +1075,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_CONNECTED(con->status)) {
> + switch (UCSI_CONSTAT_PARTNER_TYPE(con->status)) {
> case UCSI_CONSTAT_PARTNER_TYPE_DEBUG:
> typec_set_mode(con->port, TYPEC_MODE_DEBUG);
> break;
> @@ -1083,14 +1084,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_PARTNER_FLAGS(con->status) ==
> + 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_PARTNER_FLAGS(con->status) & UCSI_CONSTAT_PARTNER_FLAG_USB))
> u_role = USB_ROLE_NONE;
>
> ret = usb_role_switch_set_role(con->usb_role_sw, u_role);
> @@ -1122,7 +1123,7 @@ static int ucsi_check_connector_capability(struct ucsi_connector *con)
>
> static int ucsi_check_connection(struct ucsi_connector *con)
> {
> - u8 prev_flags = con->status.flags;
> + u8 prev_state = UCSI_CONSTAT_CONNECTED(con->status);
> int ret;
>
> ret = ucsi_get_connector_status(con, false);
> @@ -1131,10 +1132,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_CONNECTED(con->status)) {
> + if (prev_state)
> + return 0;
> ucsi_register_partner(con);
> ucsi_pwr_opmode_change(con);
> ucsi_partner_change(con);
> @@ -1212,14 +1212,14 @@ 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->status);
>
> if (ucsi->ops->connector_status)
> ucsi->ops->connector_status(con);
>
> - role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR);
> + role = UCSI_CONSTAT_PWR_DIR(con->status);
>
> - if (con->status.change & UCSI_CONSTAT_POWER_DIR_CHANGE) {
> + if (UCSI_CONSTAT_CHANGE(con->status) & UCSI_CONSTAT_POWER_DIR_CHANGE) {
> typec_set_pwr_role(con->port, role);
>
> /* Complete pending power role swap */
> @@ -1227,12 +1227,12 @@ static void ucsi_handle_connector_change(struct work_struct *work)
> complete(&con->complete);
> }
>
> - if (con->status.change & UCSI_CONSTAT_CONNECT_CHANGE) {
> + if (UCSI_CONSTAT_CHANGE(con->status) & 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_CONNECTED(con->status)) {
> ucsi_register_partner(con);
> ucsi_partner_task(con, ucsi_check_connection, 1, HZ);
> ucsi_partner_task(con, ucsi_check_connector_capability, 1, HZ);
> @@ -1241,19 +1241,18 @@ 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_PWR_OPMODE(con->status) == UCSI_CONSTAT_PWR_OPMODE_PD)
> ucsi_partner_task(con, ucsi_register_partner_pdos, 1, HZ);
> } else {
> ucsi_unregister_partner(con);
> }
> }
>
> - if (con->status.change & UCSI_CONSTAT_POWER_OPMODE_CHANGE ||
> - con->status.change & UCSI_CONSTAT_POWER_LEVEL_CHANGE)
> + if (UCSI_CONSTAT_CHANGE(con->status) & (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 && UCSI_CONSTAT_CHANGE(con->status) & UCSI_CONSTAT_PARTNER_CHANGE) {
> ucsi_partner_change(con);
>
> /* Complete pending data role swap */
> @@ -1261,10 +1260,10 @@ static void ucsi_handle_connector_change(struct work_struct *work)
> complete(&con->complete);
> }
>
> - if (con->status.change & UCSI_CONSTAT_CAM_CHANGE)
> + if (UCSI_CONSTAT_CHANGE(con->status) & UCSI_CONSTAT_CAM_CHANGE)
> ucsi_partner_task(con, ucsi_check_altmodes, 1, HZ);
>
> - if (con->status.change & UCSI_CONSTAT_BC_CHANGE)
> + if (UCSI_CONSTAT_CHANGE(con->status) & UCSI_CONSTAT_BC_CHANGE)
> ucsi_port_psy_changed(con);
>
> out_unlock:
> @@ -1424,7 +1423,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_PARTNER_TYPE(con->status);
> if ((partner_type == UCSI_CONSTAT_PARTNER_TYPE_DFP &&
> role == TYPEC_DEVICE) ||
> (partner_type == UCSI_CONSTAT_PARTNER_TYPE_UFP &&
> @@ -1468,7 +1467,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_PWR_DIR(con->status);
>
> if (cur_role == role)
> goto out_unlock;
> @@ -1491,8 +1490,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_PWR_OPMODE(con->status) != UCSI_CONSTAT_PWR_OPMODE_PD) {
> ucsi_reset_connector(con, true);
> ret = -EPROTO;
> }
> @@ -1623,7 +1621,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_PARTNER_TYPE(con->status)) {
> case UCSI_CONSTAT_PARTNER_TYPE_UFP:
> case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
> u_role = USB_ROLE_HOST;
> @@ -1640,9 +1638,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_CONNECTED(con->status)) {
> + typec_set_pwr_role(con->port, UCSI_CONSTAT_PWR_DIR(con->status));
> ucsi_register_partner(con);
> ucsi_pwr_opmode_change(con);
> ucsi_port_psy_changed(con);
> @@ -1653,7 +1650,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_PARTNER_FLAGS(con->status) & UCSI_CONSTAT_PARTNER_FLAG_USB))
> u_role = USB_ROLE_NONE;
>
> ret = usb_role_switch_set_role(con->usb_role_sw, u_role);
> @@ -1663,15 +1660,13 @@ 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_PWR_OPMODE(con->status) == UCSI_CONSTAT_PWR_OPMODE_PD) {
> ucsi_register_device_pdos(con);
> ucsi_get_src_pdos(con);
> ucsi_check_altmodes(con);
> }
>
> - trace_ucsi_register_port(con->num, &con->status);
> + trace_ucsi_register_port(con->num, con->status);
>
> out:
> fwnode_handle_put(cap->fwnode);
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 63cc7f982663..dfbc0f6c1b9b 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/bitfield.h>
> #include <linux/completion.h>
> #include <linux/device.h>
> #include <linux/power_supply.h>
> @@ -312,48 +313,55 @@ struct ucsi_cable_property {
> } __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))
> +#define UCSI_CONSTAT_CHANGE(status) UCSI_FIELD(status, 0, 16)
> +#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_CONSTAT_PWR_OPMODE(status) UCSI_FIELD(status, 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(status) UCSI_FIELD(status, 19, 1)
> +#define UCSI_CONSTAT_PWR_DIR(status) UCSI_FIELD(status, 20, 1)
> +#define UCSI_CONSTAT_PARTNER_FLAGS(status) UCSI_FIELD(status, 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(status) UCSI_FIELD(status, 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(status) UCSI_FIELD(status, 32, 32)
> +#define UCSI_CONSTAT_BC_STATUS(status) UCSI_FIELD(status, 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(status) UCSI_FIELD(status, 70, 16)
> +
> +/*
> + * UCSI specific wrapper for FIELD_GET() that allows access to fields beyond
> + * 64-bits.
> + */
> +#define UCSI_FIELD(data, offset, size) \
> + ({ \
> + u8 m = ((offset) % 8); \
> + FIELD_GET((GENMASK((m + ((size) - 1)), m)), \
> + (*((u32 *)(&((u8 *)data)[((offset) / 8)])))); \
> + })
This macro could try to read beyond the bounds of the array. I made a
similar attempt at the macros in the past here:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5274219
I wasn't happy with how verbose things turned out so didn't end up
sending an RFC but that code could probably be cleaned up to fit into
what you're trying to accomplish in this patch.
>
> /* -------------------------------------------------------------------------- */
>
> @@ -433,7 +441,7 @@ struct ucsi_connector {
>
> struct typec_capability typec_cap;
>
> - struct ucsi_connector_status status;
> + u8 status[19];
I think it's still useful to declare a struct containing the array
otherwise the "19" just looks like a magic number.
> struct ucsi_connector_capability cap;
> struct ucsi_cable_property cable_prop;
> struct power_supply *psy;
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index 7a5dff8d9cc6..41158620f4da 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 (UCSI_CONSTAT_CHANGE(val) == bogus_change)
> + *(u16 *)val = 0;
>
> ua->check_bogus_event = false;
> }
> --
> 2.45.2
>
