2024-02-18 22:21:21

by Christian A. Ehrhardt

[permalink] [raw]
Subject: [RFC PATCH 0/6] UCSI backend API refactoring

The (new) API for UCSI backends has some problems:

The UCSI spec assumes a mailbox style communication:
1. The OPM fills data (UCSI_MESSAGE_OUT and UCSI_CONTROL)
into the mailbox memory region. In the ACPI case these are
simple host memory accesses.
2. The OPM notifies the PPM that a command should be executed.
In the ACPI case this is a call the _DSM ACPI function.
Within that function the UCSI_CONTROL and the UCSI_MESSAGE_OUT
data is transferred to something like the embedded controller
and then the PPM is notified e.g. by an SMI.
3. The PPM executes the command. As a result UCSI_MESSAGE_IN and
UCSI_CCI in the mailbox memory are updated and the OPM
is notified. In the ACPI case the notification is an SCI and
ACPI provides a handler that syncs UCSI_MESSAGE_IN and
UCSI_CCI from the EC into host memory before notifying the
OPM via the ACPI Notify() command.

The major problem is that steps 1 (update mailbox memory) and
step 2 (notify PPM and start command execution) are two different
steps. The current API only knows about a write function that
combines writing to mailbox memory and starting command execution.

This only works as long as the commands do not have arguments
that must be in UCSI_MESSAGE_OUT.

Additionally, Step 3 (at least in the ACPI case) above suggests
that UCSI_CCI and UCSI_MESSAGE_IN should generally be read from
mailbox memory. Except for some special cases the sync operation
via the _DSM-Read function in ACPI is not required and seems to
do more harm then good.

E.g. the zenbook quirks could be avoided if we didn't re-read the
UCSI_CCI and UCSI_MESSAGE_IN data from the embedded controller
on each read operation. The recent fix to the CCG backend seems
to address the same issue.

This change series proposes a new API with the following design
goals:
- Retain the nice features of the previous API redesign. In
particular the synchronous execution of commands in the
backend and the ability to handle quirks there.
- The API must still be able to handle all backends (obviously).
Thus each new API function is currently implemented in all
backends in the same change.
- Separate writing to mailbox data and starting command execution
to allow for commands with UCSI_MESSAGE_OUT data.
- Avoid hardware accesses (as opposed to mailbox memory accesses)
outside of the command execution context as much as possible.
- Avoid use of explicit mailbox offsets (that are subject to
change with later UCSI revisions) in the UCSI core.

The proposed new API features:
- read_data() and write_data() for access to the message data
fields. This is supposed to be a pure mailbox access that
even in the case of a write does not start a command.
- sync_cmd() and async_cmd() to write the command register and
start command execution.
- A cached version of the current contents of CCI in the core
UCSI structure. This value is only updated if a notification
is received from the hardware.
- poll_cci() to force an update of the cached CCI value from
hardware. This is required to poll for PPM reset completion.

CAVEATs:
- The series will certainly need more polishing etc. before
it can be accepted. I will do this provided that the series
is considered a good idea in general.
- The series compiles, passes smatch, sparse and checkpatch but
I only have ucsi_acpi hardware to test this. The other backends
(ccg, glink, stm32g0) will need testers and likely some fixes
as well.

So, what do you think?

Christian A. Ehrhardt (6):
usb: ucsi_glink: Fix endianness issues
ucsi_ccg: Cleanup endianness confusion
usb: typec: ucsi: Make Version a parameter to ucsi_register
usb: typec: ucsi: Cache current CCI value in struct ucsi
usb: typec: ucsi: Introdcue ->read_data and ->write_data
usb: typec: ucsi: Convert a?sync_write to a?sync_cmd

drivers/usb/typec/ucsi/ucsi.c | 53 ++++--------
drivers/usb/typec/ucsi/ucsi.h | 28 +++---
drivers/usb/typec/ucsi/ucsi_acpi.c | 119 ++++++++++++--------------
drivers/usb/typec/ucsi/ucsi_ccg.c | 107 ++++++++++++-----------
drivers/usb/typec/ucsi/ucsi_glink.c | 104 +++++++++++++++++-----
drivers/usb/typec/ucsi/ucsi_stm32g0.c | 80 ++++++++++++++---
6 files changed, 290 insertions(+), 201 deletions(-)

--
2.40.1



2024-02-18 22:21:39

by Christian A. Ehrhardt

[permalink] [raw]
Subject: [RFC PATCH 4/6] usb: typec: ucsi: Cache current CCI value in struct ucsi

All notification handlers already read the CCI value.
Reading it again from hardware in ucsi_execute_command()
opens races and it is not clear that this really works.
The zenbook quirk in ucsi_acpi.c is one case where a re-read
causes problems.

In the ACPI case it is not even necessary to sync the
mailbox memory. According to the intel whitepaper the
ACPI handler does this before it notifies the OS.
Thus simply read the CCI value in the notification handler
without any ACPI method calls.

During reset of the PPM, it may be necessary to poll UCSI_CCI
from hardware as the reset completion may not send a notification.
Provide a new ->poll_cci API function that actively updates the
CCI value from hardware for this case.

While there take into account that the raw CCI value read
from hardware is little endian and convert it to host byte
order explicitly.

Signed-off-by: Christian A. Ehrhardt <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.c | 11 +++---
drivers/usb/typec/ucsi/ucsi.h | 5 +++
drivers/usb/typec/ucsi/ucsi_acpi.c | 53 ++++++++-------------------
drivers/usb/typec/ucsi/ucsi_ccg.c | 38 +++++++------------
drivers/usb/typec/ucsi/ucsi_glink.c | 48 +++++++++++++++++++++---
drivers/usb/typec/ucsi/ucsi_stm32g0.c | 22 ++++++++++-
6 files changed, 101 insertions(+), 76 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index eb630447ba80..a4ae3d5966a0 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -140,10 +140,7 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
ret = ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
if (ret)
return ret;
-
- ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
- if (ret)
- return ret;
+ cci = READ_ONCE(ucsi->cci);

if (cmd != UCSI_CANCEL && cci & UCSI_CCI_BUSY)
return ucsi_exec_command(ucsi, UCSI_CANCEL);
@@ -1037,9 +1034,10 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
goto out;
}

- ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
+ ret = ucsi->ops->poll_cci(ucsi);
if (ret)
goto out;
+ cci = READ_ONCE(ucsi->cci);

/* If the PPM is still doing something else, reset it again. */
if (cci & ~UCSI_CCI_RESET_COMPLETE) {
@@ -1550,7 +1548,8 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
{
struct ucsi *ucsi;

- if (!ops || !ops->read || !ops->sync_write || !ops->async_write)
+ if (!ops || !ops->poll_cci || !ops->read || !ops->sync_write ||
+ !ops->async_write)
return ERR_PTR(-EINVAL);

ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 100e16b49814..55e5c5a09b32 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -55,6 +55,7 @@ struct dentry;

/**
* struct ucsi_operations - UCSI I/O operations
+ * @poll_cci: Update the cached CCI value from hardware. Required for reset.
* @read: Read operation
* @sync_write: Blocking write operation
* @async_write: Non-blocking write operation
@@ -65,6 +66,7 @@ struct dentry;
* return immediately after sending the data to the PPM.
*/
struct ucsi_operations {
+ int (*poll_cci)(struct ucsi *ucsi);
int (*read)(struct ucsi *ucsi, unsigned int offset,
void *val, size_t val_len);
int (*sync_write)(struct ucsi *ucsi, unsigned int offset,
@@ -371,6 +373,9 @@ struct ucsi {
/* The latest "Notification Enable" bits (SET_NOTIFICATION_ENABLE) */
u64 ntfy;

+ /* The current value of the CCI field. Synced by notifications. */
+ u32 cci;
+
/* PPM communication flags */
unsigned long flags;
#define EVENT_PENDING 0
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 710ddfc03ed0..3aedfe78fb65 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -24,7 +24,6 @@ struct ucsi_acpi {
struct completion complete;
unsigned long flags;
guid_t guid;
- u64 cmd;
bool dell_quirk_probed;
bool dell_quirk_active;
};
@@ -45,8 +44,7 @@ static int ucsi_acpi_dsm(struct ucsi_acpi *ua, int func)
return 0;
}

-static int ucsi_acpi_read(struct ucsi *ucsi, unsigned int offset,
- void *val, size_t val_len)
+static int ucsi_acpi_poll_cci(struct ucsi *ucsi)
{
struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
int ret;
@@ -55,6 +53,16 @@ static int ucsi_acpi_read(struct ucsi *ucsi, unsigned int offset,
if (ret)
return ret;

+ WRITE_ONCE(ucsi->cci, le32_to_cpu(*(__le32 *)(ua->base + UCSI_CCI)));
+
+ return 0;
+}
+
+static int ucsi_acpi_read(struct ucsi *ucsi, unsigned int offset,
+ void *val, size_t val_len)
+{
+ struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+
memcpy(val, ua->base + offset, val_len);

return 0;
@@ -66,7 +74,6 @@ static int ucsi_acpi_async_write(struct ucsi *ucsi, unsigned int offset,
struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);

memcpy(ua->base + offset, val, val_len);
- ua->cmd = *(u64 *)val;

return ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_WRITE);
}
@@ -100,34 +107,12 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
}

static const struct ucsi_operations ucsi_acpi_ops = {
+ .poll_cci = ucsi_acpi_poll_cci,
.read = ucsi_acpi_read,
.sync_write = ucsi_acpi_sync_write,
.async_write = ucsi_acpi_async_write
};

-static int
-ucsi_zenbook_read(struct ucsi *ucsi, unsigned int offset, void *val, size_t val_len)
-{
- struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
- int ret;
-
- if (offset == UCSI_VERSION || UCSI_COMMAND(ua->cmd) == UCSI_PPM_RESET) {
- ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ);
- if (ret)
- return ret;
- }
-
- memcpy(val, ua->base + offset, val_len);
-
- return 0;
-}
-
-static const struct ucsi_operations ucsi_zenbook_ops = {
- .read = ucsi_zenbook_read,
- .sync_write = ucsi_acpi_sync_write,
- .async_write = ucsi_acpi_async_write
-};
-
/*
* Some Dell laptops expect that an ACK command with the
* UCSI_ACK_CONNECTOR_CHANGE bit set is followed by a (separate)
@@ -177,19 +162,13 @@ ucsi_dell_sync_write(struct ucsi *ucsi, unsigned int offset,
}

static const struct ucsi_operations ucsi_dell_ops = {
+ .poll_cci = ucsi_acpi_poll_cci,
.read = ucsi_acpi_read,
.sync_write = ucsi_dell_sync_write,
.async_write = ucsi_acpi_async_write
};

static const struct dmi_system_id ucsi_acpi_quirks[] = {
- {
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
- DMI_MATCH(DMI_PRODUCT_NAME, "ZenBook UX325UA_UM325UA"),
- },
- .driver_data = (void *)&ucsi_zenbook_ops,
- },
{
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
@@ -203,11 +182,9 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
{
struct ucsi_acpi *ua = data;
u32 cci;
- int ret;

- ret = ua->ucsi->ops->read(ua->ucsi, UCSI_CCI, &cci, sizeof(cci));
- if (ret)
- return;
+ cci = le32_to_cpu(*(__le32 *)(ua->base + UCSI_CCI));
+ WRITE_ONCE(ua->ucsi->cci, cci);

if (UCSI_CCI_CONNECTOR(cci))
ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index d47f5e31c98a..55d0fe5324f4 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -194,7 +194,6 @@ struct ucsi_ccg_altmode {

#define CCGX_MESSAGE_IN_MAX 16
struct op_region {
- __le32 cci;
u8 message_in[CCGX_MESSAGE_IN_MAX];
};

@@ -338,7 +337,6 @@ static int ccg_op_region_update(struct ucsi_ccg *uc, u32 cci)
}

spin_lock(&uc->op_lock);
- data->cci = cpu_to_le32(cci);
if (UCSI_CCI_LENGTH(cci))
memcpy(&data->message_in, buf, size);
spin_unlock(&uc->op_lock);
@@ -556,32 +554,24 @@ static void ucsi_ccg_nvidia_altmode(struct ucsi_ccg *uc,
}
}

+static int ucsi_ccg_poll_cci(struct ucsi *ucsi)
+{
+ return 0;
+}
+
static int ucsi_ccg_read(struct ucsi *ucsi, unsigned int offset,
void *val, size_t val_len)
{
struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
- u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(offset);
struct ucsi_capability *cap;
struct ucsi_altmode *alt;
- int ret = 0;
-
- if (offset == UCSI_CCI) {
- spin_lock(&uc->op_lock);
- memcpy(val, &(uc->op_data).cci, val_len);
- spin_unlock(&uc->op_lock);
- } else if (offset == UCSI_MESSAGE_IN) {
- spin_lock(&uc->op_lock);
- memcpy(val, &(uc->op_data).message_in, val_len);
- spin_unlock(&uc->op_lock);
- } else {
- ret = ccg_read(uc, reg, val, val_len);
- }
-
- if (ret)
- return ret;

if (offset != UCSI_MESSAGE_IN)
- return ret;
+ return -EINVAL;
+
+ spin_lock(&uc->op_lock);
+ memcpy(val, &uc->op_data.message_in, val_len);
+ spin_unlock(&uc->op_lock);

switch (UCSI_COMMAND(uc->last_cmd_sent)) {
case UCSI_GET_CURRENT_CAM:
@@ -607,7 +597,7 @@ static int ucsi_ccg_read(struct ucsi *ucsi, unsigned int offset,
}
uc->last_cmd_sent = 0;

- return ret;
+ return 0;
}

static int ucsi_ccg_async_write(struct ucsi *ucsi, unsigned int offset,
@@ -620,9 +610,7 @@ static int ucsi_ccg_async_write(struct ucsi *ucsi, unsigned int offset,
* UCSI may read CCI instantly after async_write,
* clear CCI to avoid caller getting wrong data before we get CCI from ISR
*/
- spin_lock(&uc->op_lock);
- uc->op_data.cci = 0;
- spin_unlock(&uc->op_lock);
+ WRITE_ONCE(ucsi->cci, 0);

return ccg_write(uc, reg, val, val_len);
}
@@ -667,6 +655,7 @@ static int ucsi_ccg_sync_write(struct ucsi *ucsi, unsigned int offset,
}

static const struct ucsi_operations ucsi_ccg_ops = {
+ .poll_cci = ucsi_ccg_poll_cci,
.read = ucsi_ccg_read,
.sync_write = ucsi_ccg_sync_write,
.async_write = ucsi_ccg_async_write,
@@ -695,6 +684,7 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
if (ret)
goto err_clear_irq;
cci = le32_to_cpu(__cci);
+ WRITE_ONCE(uc->ucsi->cci, cci);

if (UCSI_CCI_CONNECTOR(cci))
ucsi_connector_change(uc->ucsi, UCSI_CCI_CONNECTOR(cci));
diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index 7a0b28536abd..d65ab81b4ed6 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -77,10 +77,8 @@ struct pmic_glink_ucsi {
u8 read_buf[UCSI_BUF_SIZE];
};

-static int pmic_glink_ucsi_read(struct ucsi *__ucsi, unsigned int offset,
- void *val, size_t val_len)
+static int pmic_glink_sync_read_buf(struct pmic_glink_ucsi *ucsi)
{
- struct pmic_glink_ucsi *ucsi = ucsi_get_drvdata(__ucsi);
struct ucsi_read_buf_req_msg req = {};
unsigned long left;
int ret;
@@ -106,7 +104,6 @@ static int pmic_glink_ucsi_read(struct ucsi *__ucsi, unsigned int offset,
goto out_unlock;
}

- memcpy(val, &ucsi->read_buf[offset], val_len);
ret = 0;

out_unlock:
@@ -115,6 +112,35 @@ static int pmic_glink_ucsi_read(struct ucsi *__ucsi, unsigned int offset,
return ret;
}

+static int pmic_glink_ucsi_poll_cci(struct ucsi *__ucsi)
+{
+ struct pmic_glink_ucsi *ucsi = ucsi_get_drvdata(__ucsi);
+ int ret;
+
+ ret = pmic_glink_sync_read_buf(ucsi);
+ if (ret)
+ return ret;
+
+ mutex_lock(&ucsi->lock);
+ WRITE_ONCE(__ucsi->cci,
+ le32_to_cpu(*(__le32 *)&ucsi->read_buf[UCSI_CCI]));
+ mutex_unlock(&ucsi->lock);
+
+ return 0;
+}
+
+static int pmic_glink_ucsi_read(struct ucsi *__ucsi, unsigned int offset,
+ void *val, size_t val_len)
+{
+ struct pmic_glink_ucsi *ucsi = ucsi_get_drvdata(__ucsi);
+
+ mutex_lock(&ucsi->lock);
+ memcpy(val, &ucsi->read_buf[offset], val_len);
+ mutex_unlock(&ucsi->lock);
+
+ return 0;
+}
+
static int pmic_glink_ucsi_locked_write(struct pmic_glink_ucsi *ucsi, unsigned int offset,
const void *val, size_t val_len)
{
@@ -187,6 +213,7 @@ static int pmic_glink_ucsi_sync_write(struct ucsi *__ucsi, unsigned int offset,
}

static const struct ucsi_operations pmic_glink_ucsi_ops = {
+ .poll_cci = pmic_glink_ucsi_poll_cci,
.read = pmic_glink_ucsi_read,
.sync_write = pmic_glink_ucsi_sync_write,
.async_write = pmic_glink_ucsi_async_write
@@ -221,11 +248,15 @@ static void pmic_glink_ucsi_notify(struct work_struct *work)
u32 cci;
int ret;

- ret = pmic_glink_ucsi_read(ucsi->ucsi, UCSI_CCI, &cci, sizeof(cci));
+ ret = pmic_glink_sync_read_buf(ucsi);
if (ret) {
- dev_err(ucsi->dev, "failed to read CCI on notification\n");
+ dev_err(ucsi->dev, "failed to sync read buf\n");
return;
}
+ mutex_lock(&ucsi->lock);
+ cci = le32_to_cpu(*(__le32 *)&ucsi->read_buf[UCSI_CCI]);
+ mutex_unlock(&ucsi->lock);
+ WRITE_ONCE(ucsi->ucsi->cci, cci);

con_num = UCSI_CCI_CONNECTOR(cci);
if (con_num) {
@@ -258,6 +289,11 @@ static void pmic_glink_ucsi_register(struct work_struct *work)
__le16 version;
int ret;

+ ret = pmic_glink_sync_read_buf(ucsi);
+ if (ret < 0) {
+ dev_err(ucsi->dev, "cannot sync read buf: %d\n", ret);
+ return;
+ }
ret = pmic_glink_ucsi_read(ucsi->ucsi, UCSI_VERSION, &version,
sizeof(version));
if (ret < 0) {
diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
index 112692c7a158..bab2363b73f4 100644
--- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -358,6 +358,20 @@ static int ucsi_stm32g0_read_from_hw(struct ucsi_stm32g0 *g0,
}

/* UCSI ops */
+static int ucsi_stm32g0_poll_cci(struct ucsi *ucsi)
+{
+ struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+ __le32 cci;
+ int ret;
+
+ ret = ucsi_stm32g0_read_from_hw(g0, UCSI_CCI, &cci, sizeof(cci));
+ if (ret)
+ return ret;
+ WRITE_ONCE(ucsi->cci, le32_to_cpu(cci));
+
+ return 0;
+}
+
static int ucsi_stm32g0_read(struct ucsi *ucsi, unsigned int offset,
void *val, size_t len)
{
@@ -424,15 +438,18 @@ static int ucsi_stm32g0_sync_write(struct ucsi *ucsi, unsigned int offset, const
static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
{
struct ucsi_stm32g0 *g0 = data;
+ __le32 __cci;
u32 cci;
int ret;

if (g0->suspended)
g0->wakeup_event = true;

- ret = ucsi_stm32g0_read(g0->ucsi, UCSI_CCI, &cci, sizeof(cci));
+ ret = ucsi_stm32g0_read_from_hw(g0, UCSI_CCI, &__cci, sizeof(__cci));
if (ret)
return IRQ_NONE;
+ cci = le32_to_cpu(__cci);
+ WRITE_ONCE(g0->ucsi->cci, cci);

if (UCSI_CCI_CONNECTOR(cci))
ucsi_connector_change(g0->ucsi, UCSI_CCI_CONNECTOR(cci));
@@ -445,6 +462,7 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)
}

static const struct ucsi_operations ucsi_stm32g0_ops = {
+ .poll_cci = ucsi_stm32g0_poll_cci,
.read = ucsi_stm32g0_read,
.sync_write = ucsi_stm32g0_sync_write,
.async_write = ucsi_stm32g0_async_write,
@@ -626,7 +644,7 @@ static int ucsi_stm32g0_probe_bootloader(struct ucsi *ucsi)
* Try to guess if the STM32G0 is running a UCSI firmware. First probe the UCSI FW at its
* i2c address. Fallback to bootloader i2c address only if firmware-name is specified.
*/
- ret = ucsi_stm32g0_read(ucsi, UCSI_VERSION, &ucsi_version, sizeof(ucsi_version));
+ ret = ucsi_stm32g0_read_from_hw(g0, UCSI_VERSION, &ucsi_version, sizeof(ucsi_version));
if (!ret || !g0->fw_name)
return ret;

--
2.40.1


2024-02-18 22:21:53

by Christian A. Ehrhardt

[permalink] [raw]
Subject: [RFC PATCH 2/6] ucsi_ccg: Cleanup endianness confusion

When storing the cci value in the op_region struct it is
converted from host to little endian. However, the value is
read from hardware that is little endian according to the spec
and it is never converted to host byte order. However, the
value is used as if it where in host byte order.

Additionally, the message_in buffer is a byte array.
Any endian interpretation depends on the current command and
must be done in the ocntext of that command.

While all the UCSI world seems to be little endian and there
are many other endian issues if this is not true, this
particular value is treated with endian awareness, so it should
at least be done correctly.

Add the missing conversion from little endian to host byte order
when reading the CCI value from hardware. Additionally, make the
message_in buffer an u8 array and adjust the size macro accordingly.

Signed-off-by: Christian A. Ehrhardt <[email protected]>
---
drivers/usb/typec/ucsi/ucsi_ccg.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index dda7c7c94e08..709295948c65 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -192,10 +192,10 @@ struct ucsi_ccg_altmode {
bool checked;
} __packed;

-#define CCGX_MESSAGE_IN_MAX 4
+#define CCGX_MESSAGE_IN_MAX 16
struct op_region {
__le32 cci;
- __le32 message_in[CCGX_MESSAGE_IN_MAX];
+ u8 message_in[CCGX_MESSAGE_IN_MAX];
};

struct ucsi_ccg {
@@ -678,6 +678,7 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(UCSI_CCI);
struct ucsi_ccg *uc = data;
u8 intr_reg;
+ __le32 __cci;
u32 cci = 0;
int ret = 0;

@@ -690,9 +691,10 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
else if (!(intr_reg & UCSI_READ_INT))
goto err_clear_irq;

- ret = ccg_read(uc, reg, (void *)&cci, sizeof(cci));
+ ret = ccg_read(uc, reg, (void *)&__cci, sizeof(__cci));
if (ret)
goto err_clear_irq;
+ cci = le32_to_cpu(__cci);

if (UCSI_CCI_CONNECTOR(cci))
ucsi_connector_change(uc->ucsi, UCSI_CCI_CONNECTOR(cci));
--
2.40.1


2024-02-18 22:22:09

by Christian A. Ehrhardt

[permalink] [raw]
Subject: [RFC PATCH 3/6] usb: typec: ucsi: Make Version a parameter to ucsi_register

Reading UCSI_VERSION is a special case as there is no
notification that syncs the data into host memory.

Read UCSI_VERSION only once during initialization and provide
it as a parameter to ucsi_register().

Signed-off-by: Christian A. Ehrhardt <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.c | 13 ++++---------
drivers/usb/typec/ucsi/ucsi.h | 2 +-
drivers/usb/typec/ucsi/ucsi_acpi.c | 8 +++++++-
drivers/usb/typec/ucsi/ucsi_ccg.c | 12 ++++++++++--
drivers/usb/typec/ucsi/ucsi_glink.c | 11 ++++++++++-
drivers/usb/typec/ucsi/ucsi_stm32g0.c | 26 ++++++++++++++++++++++----
6 files changed, 54 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ae105383e69e..eb630447ba80 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1581,18 +1581,13 @@ EXPORT_SYMBOL_GPL(ucsi_destroy);
/**
* ucsi_register - Register UCSI interface
* @ucsi: UCSI instance
+ * @version: The revision of the UCSI spec
*/
-int ucsi_register(struct ucsi *ucsi)
+int ucsi_register(struct ucsi *ucsi, u16 version)
{
- int ret;
-
- ret = ucsi->ops->read(ucsi, UCSI_VERSION, &ucsi->version,
- sizeof(ucsi->version));
- if (ret)
- return ret;
-
- if (!ucsi->version)
+ if (!version)
return -ENODEV;
+ ucsi->version = version;

/*
* Version format is JJ.M.N (JJ = Major version, M = Minor version,
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 7e35ffbe0a6f..100e16b49814 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -77,7 +77,7 @@ struct ucsi_operations {

struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops);
void ucsi_destroy(struct ucsi *ucsi);
-int ucsi_register(struct ucsi *ucsi);
+int ucsi_register(struct ucsi *ucsi, u16 version);
void ucsi_unregister(struct ucsi *ucsi);
void *ucsi_get_drvdata(struct ucsi *ucsi);
void ucsi_set_drvdata(struct ucsi *ucsi, void *data);
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 928eacbeb21a..710ddfc03ed0 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -226,6 +226,7 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
const struct dmi_system_id *id;
struct ucsi_acpi *ua;
struct resource *res;
+ u16 version;
acpi_status status;
int ret;

@@ -272,7 +273,12 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
return -ENODEV;
}

- ret = ucsi_register(ua->ucsi);
+ ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ);
+ if (ret)
+ return ret;
+ version = le16_to_cpu(*(__le16 *)(ua->base + UCSI_VERSION));
+
+ ret = ucsi_register(ua->ucsi, version);
if (ret) {
acpi_remove_notify_handler(ACPI_HANDLE(&pdev->dev),
ACPI_DEVICE_NOTIFY,
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 709295948c65..d47f5e31c98a 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -1356,7 +1356,7 @@ static int ccg_restart(struct ucsi_ccg *uc)
return status;
}

- status = ucsi_register(uc->ucsi);
+ status = ucsi_register(uc->ucsi, uc->ucsi->version);
if (status) {
dev_err(uc->dev, "failed to register the interface\n");
return status;
@@ -1422,6 +1422,7 @@ static int ucsi_ccg_probe(struct i2c_client *client)
struct ucsi_ccg *uc;
const char *fw_name;
int status;
+ __le16 version;

uc = devm_kzalloc(dev, sizeof(*uc), GFP_KERNEL);
if (!uc)
@@ -1477,7 +1478,14 @@ static int ucsi_ccg_probe(struct i2c_client *client)
goto out_ucsi_destroy;
}

- status = ucsi_register(uc->ucsi);
+ status = ccg_read(uc, CCGX_RAB_UCSI_DATA_BLOCK(UCSI_VERSION),
+ (u8 *)&version, sizeof(version));
+ if (status < 0) {
+ dev_err(uc->dev, "cannot read UCSI version - %d\n", status);
+ return status;
+ }
+
+ status = ucsi_register(uc->ucsi, le16_to_cpu(version));
if (status)
goto out_free_irq;

diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index 0a0d08467c56..7a0b28536abd 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -255,8 +255,17 @@ static void pmic_glink_ucsi_notify(struct work_struct *work)
static void pmic_glink_ucsi_register(struct work_struct *work)
{
struct pmic_glink_ucsi *ucsi = container_of(work, struct pmic_glink_ucsi, register_work);
+ __le16 version;
+ int ret;
+
+ ret = pmic_glink_ucsi_read(ucsi->ucsi, UCSI_VERSION, &version,
+ sizeof(version));
+ if (ret < 0) {
+ dev_err(ucsi->dev, "cannot read version: %d\n", ret);
+ return;
+ }

- ucsi_register(ucsi->ucsi);
+ ucsi_register(ucsi->ucsi, le16_to_cpu(version));
}

static void pmic_glink_ucsi_callback(const void *data, size_t len, void *priv)
diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
index 93d7806681cf..112692c7a158 100644
--- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -325,10 +325,10 @@ static int ucsi_stm32g0_fw_rcv(struct ucsi *ucsi, void *data, size_t len)
return ucsi_stm32g0_bl_rcv_woack(ucsi, data, len);
}

-/* UCSI ops */
-static int ucsi_stm32g0_read(struct ucsi *ucsi, unsigned int offset, void *val, size_t len)
+static int ucsi_stm32g0_read_from_hw(struct ucsi_stm32g0 *g0,
+ unsigned int offset,
+ void *val, size_t len)
{
- struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
struct i2c_client *client = g0->client;
u8 reg = offset;
struct i2c_msg msg[] = {
@@ -357,6 +357,15 @@ static int ucsi_stm32g0_read(struct ucsi *ucsi, unsigned int offset, void *val,
return 0;
}

+/* UCSI ops */
+static int ucsi_stm32g0_read(struct ucsi *ucsi, unsigned int offset,
+ void *val, size_t len)
+{
+ struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+
+ return ucsi_stm32g0_read_from_hw(g0, offset, val, len);
+}
+
static int ucsi_stm32g0_async_write(struct ucsi *ucsi, unsigned int offset, const void *val,
size_t len)
{
@@ -445,6 +454,7 @@ static int ucsi_stm32g0_register(struct ucsi *ucsi)
{
struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
struct i2c_client *client = g0->client;
+ __le16 version;
int ret;

/* Request alert interrupt */
@@ -455,7 +465,15 @@ static int ucsi_stm32g0_register(struct ucsi *ucsi)
return ret;
}

- ret = ucsi_register(ucsi);
+ ret = ucsi_stm32g0_read_from_hw(g0, UCSI_VERSION, &version,
+ sizeof(version));
+ if (ret) {
+ dev_err(g0->dev, "failed to read version number: %d\n", ret);
+ free_irq(client->irq, g0);
+ return ret;
+ }
+
+ ret = ucsi_register(ucsi, le16_to_cpu(version));
if (ret) {
dev_err_probe(g0->dev, ret, "ucsi_register failed\n");
free_irq(client->irq, g0);
--
2.40.1


2024-02-18 22:22:24

by Christian A. Ehrhardt

[permalink] [raw]
Subject: [RFC PATCH 5/6] usb: typec: ucsi: Introdcue ->read_data and ->write_data

With the previous refactoring ->read() is only used to read
from UCSI_MESSAGE_IN. So rename ->read() to ->read_data().

Before this change there is (at least for some backends)
no way to write to UCSI_MESSAGE_OUT without starting a command.
As a result it is not possible to send a command with additional
data on these backends. Currently, the UCSI core does not send
such commands but they are defined in the spec and presumably
will be required.

Introduce ->write_data() to write to UCSI_MESSAGE_OUT without
starting a command. This is symmetrical to ->read_data().

Signed-off-by: Christian A. Ehrhardt <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.c | 6 ++---
drivers/usb/typec/ucsi/ucsi.h | 7 ++---
drivers/usb/typec/ucsi/ucsi_acpi.c | 21 +++++++++++----
drivers/usb/typec/ucsi/ucsi_ccg.c | 18 ++++++++-----
drivers/usb/typec/ucsi/ucsi_glink.c | 37 +++++++++++++++++----------
drivers/usb/typec/ucsi/ucsi_stm32g0.c | 30 +++++++++++++++++-----
6 files changed, 82 insertions(+), 37 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index a4ae3d5966a0..df3fe04cb9cd 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -46,7 +46,7 @@ static int ucsi_read_message_in(struct ucsi *ucsi, void *buf,
if (ucsi->version <= UCSI_VERSION_1_2)
buf_size = clamp(buf_size, 0, 16);

- return ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, buf, buf_size);
+ return ucsi->ops->read_data(ucsi, buf, buf_size);
}

static int ucsi_acknowledge_command(struct ucsi *ucsi)
@@ -1548,8 +1548,8 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
{
struct ucsi *ucsi;

- if (!ops || !ops->poll_cci || !ops->read || !ops->sync_write ||
- !ops->async_write)
+ if (!ops || !ops->poll_cci || !ops->read_data || !ops->write_data ||
+ !ops->sync_write || !ops->async_write)
return ERR_PTR(-EINVAL);

ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 55e5c5a09b32..2ad68124511b 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -56,7 +56,8 @@ struct dentry;
/**
* struct ucsi_operations - UCSI I/O operations
* @poll_cci: Update the cached CCI value from hardware. Required for reset.
- * @read: Read operation
+ * @read_data: Read MESSAGE_IN data
+ * @write_data: Write MESSAGE_OUT data
* @sync_write: Blocking write operation
* @async_write: Non-blocking write operation
* @update_altmodes: Squashes duplicate DP altmodes
@@ -67,8 +68,8 @@ struct dentry;
*/
struct ucsi_operations {
int (*poll_cci)(struct ucsi *ucsi);
- int (*read)(struct ucsi *ucsi, unsigned int offset,
- void *val, size_t val_len);
+ int (*read_data)(struct ucsi *ucsi, void *val, size_t val_len);
+ int (*write_data)(struct ucsi *ucsi, const void *val, size_t val_len);
int (*sync_write)(struct ucsi *ucsi, unsigned int offset,
const void *val, size_t val_len);
int (*async_write)(struct ucsi *ucsi, unsigned int offset,
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 3aedfe78fb65..79b47b433e35 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -58,12 +58,21 @@ static int ucsi_acpi_poll_cci(struct ucsi *ucsi)
return 0;
}

-static int ucsi_acpi_read(struct ucsi *ucsi, unsigned int offset,
- void *val, size_t val_len)
+static int ucsi_acpi_read_data(struct ucsi *ucsi, void *val, size_t val_len)
{
struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);

- memcpy(val, ua->base + offset, val_len);
+ memcpy(val, ua->base + UCSI_MESSAGE_IN, val_len);
+
+ return 0;
+}
+
+static int ucsi_acpi_write_data(struct ucsi *ucsi, const void *val,
+ size_t val_len)
+{
+ struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+
+ memcpy(ua->base + UCSI_MESSAGE_OUT, val, val_len);

return 0;
}
@@ -108,7 +117,8 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,

static const struct ucsi_operations ucsi_acpi_ops = {
.poll_cci = ucsi_acpi_poll_cci,
- .read = ucsi_acpi_read,
+ .read_data = ucsi_acpi_read_data,
+ .write_data = ucsi_acpi_write_data,
.sync_write = ucsi_acpi_sync_write,
.async_write = ucsi_acpi_async_write
};
@@ -163,7 +173,8 @@ ucsi_dell_sync_write(struct ucsi *ucsi, unsigned int offset,

static const struct ucsi_operations ucsi_dell_ops = {
.poll_cci = ucsi_acpi_poll_cci,
- .read = ucsi_acpi_read,
+ .read_data = ucsi_acpi_read_data,
+ .write_data = ucsi_acpi_write_data,
.sync_write = ucsi_dell_sync_write,
.async_write = ucsi_acpi_async_write
};
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 55d0fe5324f4..d6026f61a0ed 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -559,16 +559,12 @@ static int ucsi_ccg_poll_cci(struct ucsi *ucsi)
return 0;
}

-static int ucsi_ccg_read(struct ucsi *ucsi, unsigned int offset,
- void *val, size_t val_len)
+static int ucsi_ccg_read_data(struct ucsi *ucsi, void *val, size_t val_len)
{
struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
struct ucsi_capability *cap;
struct ucsi_altmode *alt;

- if (offset != UCSI_MESSAGE_IN)
- return -EINVAL;
-
spin_lock(&uc->op_lock);
memcpy(val, &uc->op_data.message_in, val_len);
spin_unlock(&uc->op_lock);
@@ -600,6 +596,15 @@ static int ucsi_ccg_read(struct ucsi *ucsi, unsigned int offset,
return 0;
}

+static int ucsi_ccg_write_data(struct ucsi *ucsi, const void *val,
+ size_t val_len)
+{
+ struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
+ u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(UCSI_MESSAGE_OUT);
+
+ return ccg_write(uc, reg, val, val_len);
+}
+
static int ucsi_ccg_async_write(struct ucsi *ucsi, unsigned int offset,
const void *val, size_t val_len)
{
@@ -656,7 +661,8 @@ static int ucsi_ccg_sync_write(struct ucsi *ucsi, unsigned int offset,

static const struct ucsi_operations ucsi_ccg_ops = {
.poll_cci = ucsi_ccg_poll_cci,
- .read = ucsi_ccg_read,
+ .read_data = ucsi_ccg_read_data,
+ .write_data = ucsi_ccg_write_data,
.sync_write = ucsi_ccg_sync_write,
.async_write = ucsi_ccg_async_write,
.update_altmodes = ucsi_ccg_update_altmodes
diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index d65ab81b4ed6..9dab1b428ad9 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -75,6 +75,7 @@ struct pmic_glink_ucsi {
struct work_struct register_work;

u8 read_buf[UCSI_BUF_SIZE];
+ u8 write_buf[UCSI_BUF_SIZE];
};

static int pmic_glink_sync_read_buf(struct pmic_glink_ucsi *ucsi)
@@ -129,18 +130,28 @@ static int pmic_glink_ucsi_poll_cci(struct ucsi *__ucsi)
return 0;
}

-static int pmic_glink_ucsi_read(struct ucsi *__ucsi, unsigned int offset,
- void *val, size_t val_len)
+static int pmic_glink_ucsi_read_data(struct ucsi *__ucsi,
+ void *val, size_t val_len)
{
struct pmic_glink_ucsi *ucsi = ucsi_get_drvdata(__ucsi);

mutex_lock(&ucsi->lock);
- memcpy(val, &ucsi->read_buf[offset], val_len);
+ memcpy(val, &ucsi->read_buf[UCSI_MESSAGE_IN], val_len);
mutex_unlock(&ucsi->lock);

return 0;
}

+static int pmic_glink_ucsi_write_data(struct ucsi *__ucsi,
+ const void *val, size_t val_len)
+{
+ struct pmic_glink_ucsi *ucsi = ucsi_get_drvdata(__ucsi);
+
+ memcpy(&ucsi->write_buf[UCSI_MESSAGE_OUT], val, val_len);
+
+ return 0;
+}
+
static int pmic_glink_ucsi_locked_write(struct pmic_glink_ucsi *ucsi, unsigned int offset,
const void *val, size_t val_len)
{
@@ -148,10 +159,15 @@ static int pmic_glink_ucsi_locked_write(struct pmic_glink_ucsi *ucsi, unsigned i
unsigned long left;
int ret;

+ if (offset != UCSI_CONTROL || val_len != sizeof(u64))
+ return -EINVAL;
+ memcpy(ucsi->write_buf + UCSI_CONTROL, val, val_len);
+
req.hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_USBC);
req.hdr.type = cpu_to_le32(MSG_TYPE_REQ_RESP);
req.hdr.opcode = cpu_to_le32(UC_UCSI_WRITE_BUF_REQ);
- memcpy(&req.buf[offset], val, val_len);
+ memcpy(req.buf, ucsi->write_buf, UCSI_BUF_SIZE);
+ memset(ucsi->write_buf, 0, sizeof(ucsi->write_buf));

reinit_completion(&ucsi->write_ack);

@@ -214,7 +230,8 @@ static int pmic_glink_ucsi_sync_write(struct ucsi *__ucsi, unsigned int offset,

static const struct ucsi_operations pmic_glink_ucsi_ops = {
.poll_cci = pmic_glink_ucsi_poll_cci,
- .read = pmic_glink_ucsi_read,
+ .read_data = pmic_glink_ucsi_read_data,
+ .write_data = pmic_glink_ucsi_write_data,
.sync_write = pmic_glink_ucsi_sync_write,
.async_write = pmic_glink_ucsi_async_write
};
@@ -286,7 +303,6 @@ static void pmic_glink_ucsi_notify(struct work_struct *work)
static void pmic_glink_ucsi_register(struct work_struct *work)
{
struct pmic_glink_ucsi *ucsi = container_of(work, struct pmic_glink_ucsi, register_work);
- __le16 version;
int ret;

ret = pmic_glink_sync_read_buf(ucsi);
@@ -294,14 +310,9 @@ static void pmic_glink_ucsi_register(struct work_struct *work)
dev_err(ucsi->dev, "cannot sync read buf: %d\n", ret);
return;
}
- ret = pmic_glink_ucsi_read(ucsi->ucsi, UCSI_VERSION, &version,
- sizeof(version));
- if (ret < 0) {
- dev_err(ucsi->dev, "cannot read version: %d\n", ret);
- return;
- }

- ucsi_register(ucsi->ucsi, le16_to_cpu(version));
+ ucsi_register(ucsi->ucsi,
+ le16_to_cpu(*(__le16 *)(ucsi->read_buf + UCSI_VERSION)));
}

static void pmic_glink_ucsi_callback(const void *data, size_t len, void *priv)
diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
index bab2363b73f4..d68aca118e41 100644
--- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -372,18 +372,17 @@ static int ucsi_stm32g0_poll_cci(struct ucsi *ucsi)
return 0;
}

-static int ucsi_stm32g0_read(struct ucsi *ucsi, unsigned int offset,
- void *val, size_t len)
+static int ucsi_stm32g0_read_data(struct ucsi *ucsi, void *val, size_t len)
{
struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);

- return ucsi_stm32g0_read_from_hw(g0, offset, val, len);
+ return ucsi_stm32g0_read_from_hw(g0, UCSI_MESSAGE_IN, val, len);
}

-static int ucsi_stm32g0_async_write(struct ucsi *ucsi, unsigned int offset, const void *val,
- size_t len)
+static int ucsi_stm32g0_write_to_hw(struct ucsi_stm32g0 *g0,
+ unsigned int offset,
+ const void *val, size_t len)
{
- struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
struct i2c_client *client = g0->client;
struct i2c_msg msg[] = {
{
@@ -414,6 +413,22 @@ static int ucsi_stm32g0_async_write(struct ucsi *ucsi, unsigned int offset, cons
return 0;
}

+static int ucsi_stm32g0_write_data(struct ucsi *ucsi,
+ const void *val, size_t len)
+{
+ struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+
+ return ucsi_stm32g0_write_to_hw(g0, UCSI_MESSAGE_OUT, val, len);
+}
+
+static int ucsi_stm32g0_async_write(struct ucsi *ucsi, unsigned int offset,
+ const void *val, size_t len)
+{
+ struct ucsi_stm32g0 *g0 = ucsi_get_drvdata(ucsi);
+
+ return ucsi_stm32g0_write_to_hw(g0, offset, val, len);
+}
+
static int ucsi_stm32g0_sync_write(struct ucsi *ucsi, unsigned int offset, const void *val,
size_t len)
{
@@ -463,7 +478,8 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data)

static const struct ucsi_operations ucsi_stm32g0_ops = {
.poll_cci = ucsi_stm32g0_poll_cci,
- .read = ucsi_stm32g0_read,
+ .read_data = ucsi_stm32g0_read_data,
+ .write_data = ucsi_stm32g0_write_data,
.sync_write = ucsi_stm32g0_sync_write,
.async_write = ucsi_stm32g0_async_write,
};
--
2.40.1