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@xxxxxxxxxxxxxxx>
> ---
> 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
