Message-ID: <5310962f-c0d8-4ada-bb95-b727a3c88b00@linaro.org>
Date: Sun, 29 Dec 2024 14:51:57 +0000
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Pengyu Luo <mitltlatltl@...il.com>
Cc: andersson@...nel.org, conor+dt@...nel.org, devicetree@...r.kernel.org,
 dmitry.baryshkov@...aro.org, gregkh@...uxfoundation.org,
 hdegoede@...hat.com, heikki.krogerus@...ux.intel.com,
 ilpo.jarvinen@...ux.intel.com, konradybcio@...nel.org, krzk+dt@...nel.org,
 linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-pm@...r.kernel.org, linux-usb@...r.kernel.org, nikita@...n.ru,
 platform-driver-x86@...r.kernel.org, robh@...nel.org, sre@...nel.org
Subject: Re: [PATCH 3/5] usb: typec: ucsi: add Huawei Matebook E Go (sc8280xp)
 ucsi driver

On 28/12/2024 14:38, Pengyu Luo wrote:
> On Sat, Dec 28, 2024 at 9:06 PM Bryan O'Donoghue <bryan.odonoghue@...aro.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@...il.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@...il.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"

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

Powered by blists - more mailing lists