Re: [PATCH 3/5] usb: typec: ucsi: add Huawei Matebook E Go (sc8280xp) ucsi driver

[Pengyu Luo <mitltlatltl@gmail.com>]

On Sun, Dec 29, 2024 at 10:52 PM Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote:
> On 28/12/2024 14:38, Pengyu Luo wrote:
> > On Sat, Dec 28, 2024 at 9:06 PM Bryan O'Donoghue <bryan.odonoghue@linaro.org> 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@gmail.com>
> >>> ---
> >>>    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@gmail.com>
> >>> + */
> >>> +
> >>> +#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"
>

I will follow this in v2.

> >>>
> >>> +
> >>> +#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.
>

I got it

>
>
> >>> +             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.
>

Makes sense

>
> >
> >>> +             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.
>

Agree, I will drop -EIO to use -1 instead, and define -1 as NO_UPDATE.
It is also resonable when there is a EC transaction error.

Best wishes,
Pengyu