Re: [PATCH 3/5] usb: typec: ucsi: add Huawei Matebook E Go (sc8280xp) ucsi driver
From: Bryan O'Donoghue
Date: Sun Dec 29 2024 - 09:55:00 EST
On 28/12/2024 14:38, Pengyu Luo wrote:
On Sat, Dec 28, 2024 at 9:06 PM Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx> wrote:
On 27/12/2024 17:13, Pengyu Luo wrote:
The Huawei Matebook E Go (sc8280xp) tablet provides implements UCSI
interface in the onboard EC. Add the glue driver to interface the
platform's UCSI implementation.
Signed-off-by: Pengyu Luo <mitltlatltl@xxxxxxxxx>
---
drivers/usb/typec/ucsi/Kconfig | 9 +
drivers/usb/typec/ucsi/Makefile | 1 +
drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c | 481 ++++++++++++++++++++
3 files changed, 491 insertions(+)
create mode 100644 drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c
diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
index 680e1b87b..0d0f07488 100644
--- a/drivers/usb/typec/ucsi/Kconfig
+++ b/drivers/usb/typec/ucsi/Kconfig
@@ -78,4 +78,13 @@ config UCSI_LENOVO_YOGA_C630
To compile the driver as a module, choose M here: the module will be
called ucsi_yoga_c630.
+config UCSI_HUAWEI_GAOKUN
+ tristate "UCSI Interface Driver for Huawei Matebook E Go (sc8280xp)"
+ depends on EC_HUAWEI_GAOKUN
+ help
+ This driver enables UCSI support on the Huawei Matebook E Go tablet.
+
+ To compile the driver as a module, choose M here: the module will be
+ called ucsi_huawei_gaokun.
+
endif
diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
index aed41d238..0b400122b 100644
--- a/drivers/usb/typec/ucsi/Makefile
+++ b/drivers/usb/typec/ucsi/Makefile
@@ -22,3 +22,4 @@ obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
obj-$(CONFIG_UCSI_STM32G0) += ucsi_stm32g0.o
obj-$(CONFIG_UCSI_PMIC_GLINK) += ucsi_glink.o
obj-$(CONFIG_UCSI_LENOVO_YOGA_C630) += ucsi_yoga_c630.o
+obj-$(CONFIG_UCSI_HUAWEI_GAOKUN) += ucsi_huawei_gaokun.o
diff --git a/drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c b/drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c
new file mode 100644
index 000000000..84ed0407d
--- /dev/null
+++ b/drivers/usb/typec/ucsi/ucsi_huawei_gaokun.c
@@ -0,0 +1,481 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ucsi-huawei-gaokun - A UCSI driver for HUAWEI Matebook E Go (sc8280xp)
+ *
+ * reference: drivers/usb/typec/ucsi/ucsi_yoga_c630.c
+ * drivers/usb/typec/ucsi/ucsi_glink.c
+ * drivers/soc/qcom/pmic_glink_altmode.c
+ *
+ * Copyright (C) 2024 Pengyu Luo <mitltlatltl@xxxxxxxxx>
+ */
+
+#include <linux/auxiliary_bus.h>
+#include <linux/bitops.h>
+#include <linux/completion.h>
+#include <linux/container_of.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/of.h>
+#include <linux/string.h>
+#include <linux/workqueue_types.h>
+
+#include <linux/usb/pd_vdo.h>
+#include <drm/bridge/aux-bridge.h
Is there a reason you don't have strict include alphanumeric ordering here ?
These two is dp/alt mode related, so listing them out. Above of them are
general things.
OK. Unless there's an include dependency reason you need to, please just
sort the headers alphanumerically in order
#include <globals_first>
#include <globals_first_alpha>
#include "locals_next"
#include "locals_next_alpha_also"
+
+#include "ucsi.h"
+#include <linux/platform_data/huawei-gaokun-ec.h>
+
+
+#define EC_EVENT_UCSI 0x21
+#define EC_EVENT_USB 0x22
+
+#define GAOKUN_CCX_MASK GENMASK(1, 0)
+#define GAOKUN_MUX_MASK GENMASK(3, 2)
+
+#define GAOKUN_DPAM_MASK GENMASK(3, 0)
+#define GAOKUN_HPD_STATE_MASK BIT(4)
+#define GAOKUN_HPD_IRQ_MASK BIT(5)
+
+#define CCX_TO_ORI(ccx) (++ccx % 3)
Why do you increment the value of the enum ?
Seems strange.
EC's logic, it is just a trick. Qualcomm maps
0 1 2 to normal, reverse, none(no device insert)
typec lib maps 1 2 0 to that.
I'd suggest making the trick much more obvious.
Either with a comment or just mapping 1:1 between EC and Linux' view of
this data.
The main reason for that is to make it easier to
read/understand/maintain/debug.
+ port->svid = USB_SID_DISPLAYPORT;
+ if (port->ccx == USBC_CCX_REVERSE)
+ port->mode -= 6;
why minus six ?
needs a comment.
EC's logic. I don't know why, it is a quirk from Qualcomm or Huawei.
I will mention this.
Instead of hard-coding a mapping between the EC's mode and Linux' UCSI
enum - just make a define or an inline, ideally something with
switch(port->mode)
case GOAKUN_MODE_0:
val = LINUX_UCSI_MODE_X;
case GOAKUN_MODE_1:
val = LINUX_UCSI_MODE_Y;
}
That will make the mapping obvious and also ensure both to yourself and
to your reviewers that you have accounted for _all_ of the potential
mappings and if those mappings don't exist then the default: of your
switch statement should make some noise about it
dev_err(dev, "GOKUN UCSI mode %d unmapped\n"); or something like that.
+ break;
+ default:
+ break;
+ }
+
+ spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static int gaokun_ucsi_refresh(struct gaokun_ucsi *uec)
+{
+ struct gaokun_ucsi_reg ureg;
+ int ret, idx;
+
+ ret = gaokun_ec_ucsi_get_reg(uec->ec, (u8 *)&ureg);
+ if (ret)
+ return -EIO;
+
+ uec->port_num = ureg.port_num;
+ idx = GET_IDX(ureg.port_updt);
+
+ if (idx >= 0 && idx < ureg.port_num)
+ gaokun_ucsi_port_update(&uec->ports[idx], ureg.port_data);
Since you are checking the validity of the index, you should -EINVAL if
the index is out of range.
EC / pmic glink encode every port in a bit
0/1/2/4/... => ???/left/right/some port
I remap it to -1/0/1/2, to access specific port exceptions(-1) are not
harmful, later in gaokun_ucsi_altmode_notify_ind
if (idx < 0)
gaokun_ec_ucsi_pan_ack(uec->ec, idx);
else
gaokun_ucsi_handle_altmode(&uec->ports[idx]);
gaokun_ec_ucsi_pan_ack can handle exceptions.
gaokun_ucsi_refresh() can return
-EIO
-1
>=0
Where -EIO and -1 both trigger gaokun_ec_ucsi_pan_ack() in
gaokun_ucsi_altmode_notify_ind()
So if the index doesn't matter and < 0 => pan_ack() is OK or -EIO is not
returning meaningful error.
Either way strongly advise against mixing a negative index as having a
valid meaning with actual -E error codes...
As a reviewer doing a fist-pass this looks suspicous and implies more
thought/refinement should be done to the flow.
+
+ ucsi->quirks = UCSI_NO_PARTNER_PDOS | UCSI_DELAY_DEVICE_PDOS;
+
+ ssleep(3); /* EC can't handle UCSI properly in the early stage */
Could you not schedule work for + 3 seconds instead of sleeping here -
representing the required stall time in some sort of state machine ?
I see, I will check work schedule interface.
3 seconds is an incredibly long time for a computer to sleep.
This module will be loaded at about 5th second after power up, if not
sleep, we will receive something disharmonious, sleeping for 3 seconds is
a hack.
Yes it is. You could schedule some work to complete three seconds from
here instead of doing this long sleep here.
In fact you are registering a worker here right ?
In which case its pretty trivial to schedule some work on that worker
for three seconds hence ..
Please investigate.
---
bod