[PATCH] platform/x86: Add new MeeGoPad ANX7428 Type-C Cross Switch driver

[PATCH] platform/x86: Add new MeeGoPad ANX7428 Type-C Cross Switch driver

[Hans de Goede]

Some MeeGoPad top-set boxes have an ANX7428 Type-C Switch for USB3.1 Gen 1
and DisplayPort over Type-C alternate mode support.

The ANX7428 has a microcontroller which takes care of the PD negotiation
and automatically sets the builtin Crosspoint Switch to send the right
signal to the 4 highspeed pairs of the Type-C connector. It also takes
care of HPD and AUX channel routing for DP alternate mode.

IOW the ANX7428 operates fully autonomous and to the x5-Z8350 SoC
things look like there simple is a USB-3 Type-A connector and a
separate DipslayPort connector. Except that the BIOS does not
power on the ANX7428 at boot (meh).

Add a driver to power on the ANX7428. This driver is added under
drivers/platform/x86 rather then under drivers/usb/typec for 2 reasons:

1. This driver is specificly written to work with how the ANX7428 is
described in the ACPI tables of the MeeGoPad x86 (Cherry Trail) devices.

2. This driver only powers on the ANX7428 and does not do anything wrt
its Type-C functionality. It should be possible to tell the controller
which data- and/or power-role to negotiate and to swap the role(s) after
negotiation but the MeeGoPad top-set boxes always draw their power from
a separate power-connector and they only support USB host-mode. So this
functionality is unnecessary and due to lack of documenation this
is tricky to support.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/Kconfig            |  11 ++
 drivers/platform/x86/Makefile           |   3 +
 drivers/platform/x86/meegopad_anx7428.c | 155 ++++++++++++++++++++++++
 3 files changed, 169 insertions(+)
 create mode 100644 drivers/platform/x86/meegopad_anx7428.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 6dbd40e2aeda..87946a7a51fe 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -641,6 +641,17 @@ config THINKPAD_LMI
 
 source "drivers/platform/x86/intel/Kconfig"
 
+config MEEGOPAD_ANX7428
+	tristate "MeeGoPad ANX7428 Type-C Switch"
+	depends on ACPI && GPIOLIB && I2C
+	help
+	  Some MeeGoPad top-set boxes have an ANX7428 Type-C Switch for
+	  USB3.1 Gen 1 and DisplayPort over Type-C alternate mode support.
+
+	  This driver takes care of powering on the ANX7428 on supported
+	  MeeGoPad top-set boxes. After this the ANX7428 takes care of Type-C
+	  connector orientation and PD alternate mode switching autonomously.
+
 config MSI_EC
 	tristate "MSI EC Extras"
 	depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 1de432e8861e..e05b9eb280b1 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -70,6 +70,9 @@ obj-$(CONFIG_YOGABOOK)		+= lenovo-yogabook.o
 # Intel
 obj-y				+= intel/
 
+# MeeGoPad
+obj-$(CONFIG_MEEGOPAD_ANX7428)	+= meegopad_anx7428.o
+
 # MSI
 obj-$(CONFIG_MSI_EC)		+= msi-ec.o
 obj-$(CONFIG_MSI_LAPTOP)	+= msi-laptop.o
diff --git a/drivers/platform/x86/meegopad_anx7428.c b/drivers/platform/x86/meegopad_anx7428.c
new file mode 100644
index 000000000000..b4cd5f8317fa
--- /dev/null
+++ b/drivers/platform/x86/meegopad_anx7428.c
@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * meegopad_anx7428.c - Driver to power on the Analogix ANX7428
+ * USB Type-C crosspoint switch on MeeGoPad top-set boxes.
+ *
+ * The MeeGoPad T8 and T9 are Cherry Trail top-set boxes which
+ * use an ANX7428 to provide a Type-C port with USB3.1 Gen 1 and
+ * DisplayPort over Type-C alternate mode support.
+ *
+ * The ANX7428 has a microcontroller which takes care of the PD
+ * negotiation and automatically sets the builtin Crosspoint Switch
+ * to send the right signal to the 4 highspeed pairs of the Type-C
+ * connector. It also takes care of HPD and AUX channel routing for
+ * DP alternate mode.
+ *
+ * IOW the ANX7428 operates fully autonomous and to the x5-Z8350 SoC
+ * things look like there simple is a USB-3 Type-A connector and a
+ * separate DipslayPort connector. Except that the BIOS does not
+ * power on the ANX7428 at boot. This driver takes care of powering
+ * on the ANX7428.
+ *
+ * It should be possible to tell the micro-controller which data- and/or
+ * power-role to negotiate and to swap the role(s) after negotiation
+ * but the MeeGoPad top-set boxes always draw their power from a separate
+ * power-connector and they only support USB host-mode. So this functionality
+ * is unnecessary and due to lack of documenation this is tricky to support.
+ *
+ * For a more complete ANX7428 driver see drivers/usb/misc/anx7418/ of
+ * the LineageOS kernel for the LG G5 (International) aka the LG H850:
+ * https://github.com/LineageOS/android_kernel_lge_msm8996/
+ *
+ * (C) Copyright 2024 Hans de Goede <hansg@kernel.org>
+ */
+
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/dmi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+
+/* Register addresses and fields */
+#define VENDOR_ID_L			0x00
+#define VENDOR_ID_H			0x01
+#define DEVICE_ID_L			0x02
+#define DEVICE_ID_H			0x03
+
+#define TX_STATUS			0x16
+#define STATUS_SUCCESS			BIT(0)
+#define STATUS_ERROR			BIT(1)
+#define OCM_STARTUP			BIT(7)
+
+static bool force;
+module_param(force, bool, 0444);
+MODULE_PARM_DESC(force, "Force the driver to probe on unknown boards");
+
+static const struct acpi_gpio_params enable_gpio = { 0, 0, false };
+static const struct acpi_gpio_params reset_gpio = { 1, 0, true };
+
+static const struct acpi_gpio_mapping meegopad_anx7428_gpios[] = {
+	{ "enable-gpios", &enable_gpio, 1 },
+	{ "reset-gpios", &reset_gpio, 1 },
+	{ }
+};
+
+static const struct dmi_system_id meegopad_anx7428_ids[] = {
+	{
+		/* Meegopad T08 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Default string"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Default string"),
+			DMI_MATCH(DMI_BOARD_NAME, "T3 MRD"),
+			DMI_MATCH(DMI_BOARD_VERSION, "V1.1"),
+		},
+	},
+	{ }
+};
+
+static int anx7428_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	const int max_tries = 10;
+	struct gpio_desc *gpio;
+	int i, ret, vendor;
+
+	if (!dmi_check_system(meegopad_anx7428_ids) && !force) {
+		dev_warn(dev, "Not probing unknown board, pass meegopad_anx7428.force=1 to probe");
+		return -ENODEV;
+	}
+
+	ret = devm_acpi_dev_add_driver_gpios(dev, meegopad_anx7428_gpios);
+	if (ret)
+		return ret;
+
+	/*
+	 * Set GPIOs to desired values while getting them, they are not needed
+	 * afterwards. Ordering and delays come from android_kernel_lge_msm8996.
+	 */
+	gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
+	if (IS_ERR(gpio))
+		return dev_err_probe(dev, PTR_ERR(gpio), "getting enable GPIO\n");
+
+	usleep_range(10000, 15000);
+
+	gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(gpio))
+		return dev_err_probe(dev, PTR_ERR(gpio), "getting reset GPIO\n");
+
+	/* Wait for the OCM (On Chip Microcontroller) to start */
+	for (i = 0; i < max_tries; i++) {
+		usleep_range(5000, 10000);
+
+		ret = i2c_smbus_read_byte_data(client, TX_STATUS);
+		if (ret < 0)
+			dev_err_probe(dev, ret, "reading status register\n");
+		else if (ret & OCM_STARTUP)
+			break;
+	}
+	if (i == max_tries)
+		return dev_err_probe(dev, -EIO,
+				     "On Chip Microcontroller did not start, status: 0x%02x\n",
+				     ret);
+
+	ret = i2c_smbus_read_word_data(client, VENDOR_ID_L);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "reading vendor-id register\n");
+	vendor = ret;
+
+	ret = i2c_smbus_read_word_data(client, DEVICE_ID_L);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "reading device-id register\n");
+
+	dev_info(dev, "Powered on ANX7428 id %04x:%04x\n", vendor, ret);
+	return 0;
+}
+
+static const struct acpi_device_id anx7428_acpi_match[] = {
+	{ "ANXO7418" }, /* ACPI says 7418 (max 2 DP lanes version) but HW is 7428 */
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, anx7428_acpi_match);
+
+static struct i2c_driver anx7428_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.acpi_match_table = anx7428_acpi_match,
+	},
+	.probe = anx7428_probe,
+};
+
+module_i2c_driver(anx7428_driver);
+
+MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
+MODULE_DESCRIPTION("MeeGoPad ANX7428 driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH] platform/x86: Add new MeeGoPad ANX7428 Type-C Cross Switch driver

[Andy Shevchenko]

On Sun, Feb 11, 2024 at 9:53 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Some MeeGoPad top-set boxes have an ANX7428 Type-C Switch for USB3.1 Gen 1
> and DisplayPort over Type-C alternate mode support.
>
> The ANX7428 has a microcontroller which takes care of the PD negotiation
> and automatically sets the builtin Crosspoint Switch to send the right
> signal to the 4 highspeed pairs of the Type-C connector. It also takes
> care of HPD and AUX channel routing for DP alternate mode.
>
> IOW the ANX7428 operates fully autonomous and to the x5-Z8350 SoC
> things look like there simple is a USB-3 Type-A connector and a
> separate DipslayPort connector. Except that the BIOS does not

DisplayPort

> power on the ANX7428 at boot (meh).
>
> Add a driver to power on the ANX7428. This driver is added under
> drivers/platform/x86 rather then under drivers/usb/typec for 2 reasons:

than

> 1. This driver is specificly written to work with how the ANX7428 is

specifically

> described in the ACPI tables of the MeeGoPad x86 (Cherry Trail) devices.
>
> 2. This driver only powers on the ANX7428 and does not do anything wrt
> its Type-C functionality. It should be possible to tell the controller
> which data- and/or power-role to negotiate and to swap the role(s) after
> negotiation but the MeeGoPad top-set boxes always draw their power from
> a separate power-connector and they only support USB host-mode. So this
> functionality is unnecessary and due to lack of documenation this

documentation

> is tricky to support.

...

> +/*
> + * meegopad_anx7428.c - Driver to power on the Analogix ANX7428

Keeping a filename inside the file is a burden in case the file gets
renamed in the future.

> + * USB Type-C crosspoint switch on MeeGoPad top-set boxes.
> + *
> + * The MeeGoPad T8 and T9 are Cherry Trail top-set boxes which
> + * use an ANX7428 to provide a Type-C port with USB3.1 Gen 1 and
> + * DisplayPort over Type-C alternate mode support.
> + *
> + * The ANX7428 has a microcontroller which takes care of the PD
> + * negotiation and automatically sets the builtin Crosspoint Switch
> + * to send the right signal to the 4 highspeed pairs of the Type-C
> + * connector. It also takes care of HPD and AUX channel routing for
> + * DP alternate mode.
> + *
> + * IOW the ANX7428 operates fully autonomous and to the x5-Z8350 SoC
> + * things look like there simple is a USB-3 Type-A connector and a
> + * separate DipslayPort connector. Except that the BIOS does not

DisplayPort

> + * power on the ANX7428 at boot. This driver takes care of powering
> + * on the ANX7428.
> + *
> + * It should be possible to tell the micro-controller which data- and/or
> + * power-role to negotiate and to swap the role(s) after negotiation
> + * but the MeeGoPad top-set boxes always draw their power from a separate
> + * power-connector and they only support USB host-mode. So this functionality
> + * is unnecessary and due to lack of documenation this is tricky to support.

documentation

> + * For a more complete ANX7428 driver see drivers/usb/misc/anx7418/ of
> + * the LineageOS kernel for the LG G5 (International) aka the LG H850:
> + * https://github.com/LineageOS/android_kernel_lge_msm8996/
> + *
> + * (C) Copyright 2024 Hans de Goede <hansg@kernel.org>
> + */
> +
> +#include <linux/acpi.h>

+ bits.h

> +#include <linux/delay.h>
> +#include <linux/dmi.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>

...

> +#define VENDOR_ID_L                    0x00
> +#define VENDOR_ID_H                    0x01
> +#define DEVICE_ID_L                    0x02
> +#define DEVICE_ID_H                    0x03

You use word (16-bit) access, why do we need to know these?
Just define them without suffixes (and if you want add a comment that
they are 16-bit LE).

...

> +       usleep_range(10000, 15000);

fsleep() ?

...

> +       /* Wait for the OCM (On Chip Microcontroller) to start */
> +       for (i = 0; i < max_tries; i++) {
> +               usleep_range(5000, 10000);
> +
> +               ret = i2c_smbus_read_byte_data(client, TX_STATUS);
> +               if (ret < 0)
> +                       dev_err_probe(dev, ret, "reading status register\n");
> +               else if (ret & OCM_STARTUP)
> +                       break;
> +       }
> +       if (i == max_tries)
> +               return dev_err_probe(dev, -EIO,
> +                                    "On Chip Microcontroller did not start, status: 0x%02x\n",
> +                                    ret);

Why not use read_poll_timeout() / readx_poll_timeout() (whichever suits better)?

...

> +static struct i2c_driver anx7428_driver = {
> +       .driver = {
> +               .name = KBUILD_MODNAME,

Strictly speaking this is an ABI and we don't want it to be changed in
case of filename change. Personally I _always_ prefer it be open
coded.

> +               .acpi_match_table = anx7428_acpi_match,
> +       },
> +       .probe = anx7428_probe,
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: Add new MeeGoPad ANX7428 Type-C Cross Switch driver

[Greg KH]

On Sun, Feb 11, 2024 at 08:53:07PM +0100, Hans de Goede wrote:
> +	dev_info(dev, "Powered on ANX7428 id %04x:%04x\n", vendor, ret);
> +	return 0;

When drivers work properly, they should be quiet, right?  Otherwise we
get a lot of kernel log spam for working systems :(

thanks,

greg k-h

Re: [PATCH] platform/x86: Add new MeeGoPad ANX7428 Type-C Cross Switch driver

[Greg KH]

On Sun, Feb 11, 2024 at 10:50:59PM +0200, Andy Shevchenko wrote:
> > +static struct i2c_driver anx7428_driver = {
> > +       .driver = {
> > +               .name = KBUILD_MODNAME,
> 
> Strictly speaking this is an ABI and we don't want it to be changed in
> case of filename change. Personally I _always_ prefer it be open
> coded.

No, it's not an abi, driver names can, and do, change, nothing should
ever rely on this.  So using KBUILD_MODNAME is fine.

thanks,

greg k-h

Re: [PATCH] platform/x86: Add new MeeGoPad ANX7428 Type-C Cross Switch driver

[Hans de Goede]

Hi Andy,

Thank you for the review.

I agree with all your review comments below and I have fixed them all
for v2 of the patch which I'm about to post.

Regards,

Hans



On 2/11/24 9:50 PM, Andy Shevchenko wrote:
> On Sun, Feb 11, 2024 at 9:53 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Some MeeGoPad top-set boxes have an ANX7428 Type-C Switch for USB3.1 Gen 1
>> and DisplayPort over Type-C alternate mode support.
>>
>> The ANX7428 has a microcontroller which takes care of the PD negotiation
>> and automatically sets the builtin Crosspoint Switch to send the right
>> signal to the 4 highspeed pairs of the Type-C connector. It also takes
>> care of HPD and AUX channel routing for DP alternate mode.
>>
>> IOW the ANX7428 operates fully autonomous and to the x5-Z8350 SoC
>> things look like there simple is a USB-3 Type-A connector and a
>> separate DipslayPort connector. Except that the BIOS does not
> 
> DisplayPort
> 
>> power on the ANX7428 at boot (meh).
>>
>> Add a driver to power on the ANX7428. This driver is added under
>> drivers/platform/x86 rather then under drivers/usb/typec for 2 reasons:
> 
> than
> 
>> 1. This driver is specificly written to work with how the ANX7428 is
> 
> specifically
> 
>> described in the ACPI tables of the MeeGoPad x86 (Cherry Trail) devices.
>>
>> 2. This driver only powers on the ANX7428 and does not do anything wrt
>> its Type-C functionality. It should be possible to tell the controller
>> which data- and/or power-role to negotiate and to swap the role(s) after
>> negotiation but the MeeGoPad top-set boxes always draw their power from
>> a separate power-connector and they only support USB host-mode. So this
>> functionality is unnecessary and due to lack of documenation this
> 
> documentation
> 
>> is tricky to support.
> 
> ...
> 
>> +/*
>> + * meegopad_anx7428.c - Driver to power on the Analogix ANX7428
> 
> Keeping a filename inside the file is a burden in case the file gets
> renamed in the future.
> 
>> + * USB Type-C crosspoint switch on MeeGoPad top-set boxes.
>> + *
>> + * The MeeGoPad T8 and T9 are Cherry Trail top-set boxes which
>> + * use an ANX7428 to provide a Type-C port with USB3.1 Gen 1 and
>> + * DisplayPort over Type-C alternate mode support.
>> + *
>> + * The ANX7428 has a microcontroller which takes care of the PD
>> + * negotiation and automatically sets the builtin Crosspoint Switch
>> + * to send the right signal to the 4 highspeed pairs of the Type-C
>> + * connector. It also takes care of HPD and AUX channel routing for
>> + * DP alternate mode.
>> + *
>> + * IOW the ANX7428 operates fully autonomous and to the x5-Z8350 SoC
>> + * things look like there simple is a USB-3 Type-A connector and a
>> + * separate DipslayPort connector. Except that the BIOS does not
> 
> DisplayPort
> 
>> + * power on the ANX7428 at boot. This driver takes care of powering
>> + * on the ANX7428.
>> + *
>> + * It should be possible to tell the micro-controller which data- and/or
>> + * power-role to negotiate and to swap the role(s) after negotiation
>> + * but the MeeGoPad top-set boxes always draw their power from a separate
>> + * power-connector and they only support USB host-mode. So this functionality
>> + * is unnecessary and due to lack of documenation this is tricky to support.
> 
> documentation
> 
>> + * For a more complete ANX7428 driver see drivers/usb/misc/anx7418/ of
>> + * the LineageOS kernel for the LG G5 (International) aka the LG H850:
>> + * https://github.com/LineageOS/android_kernel_lge_msm8996/
>> + *
>> + * (C) Copyright 2024 Hans de Goede <hansg@kernel.org>
>> + */
>> +
>> +#include <linux/acpi.h>
> 
> + bits.h
> 
>> +#include <linux/delay.h>
>> +#include <linux/dmi.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
> 
> ...
> 
>> +#define VENDOR_ID_L                    0x00
>> +#define VENDOR_ID_H                    0x01
>> +#define DEVICE_ID_L                    0x02
>> +#define DEVICE_ID_H                    0x03
> 
> You use word (16-bit) access, why do we need to know these?
> Just define them without suffixes (and if you want add a comment that
> they are 16-bit LE).
> 
> ...
> 
>> +       usleep_range(10000, 15000);
> 
> fsleep() ?
> 
> ...
> 
>> +       /* Wait for the OCM (On Chip Microcontroller) to start */
>> +       for (i = 0; i < max_tries; i++) {
>> +               usleep_range(5000, 10000);
>> +
>> +               ret = i2c_smbus_read_byte_data(client, TX_STATUS);
>> +               if (ret < 0)
>> +                       dev_err_probe(dev, ret, "reading status register\n");
>> +               else if (ret & OCM_STARTUP)
>> +                       break;
>> +       }
>> +       if (i == max_tries)
>> +               return dev_err_probe(dev, -EIO,
>> +                                    "On Chip Microcontroller did not start, status: 0x%02x\n",
>> +                                    ret);
> 
> Why not use read_poll_timeout() / readx_poll_timeout() (whichever suits better)?
> 
> ...
> 
>> +static struct i2c_driver anx7428_driver = {
>> +       .driver = {
>> +               .name = KBUILD_MODNAME,
> 
> Strictly speaking this is an ABI and we don't want it to be changed in
> case of filename change. Personally I _always_ prefer it be open
> coded.
> 
>> +               .acpi_match_table = anx7428_acpi_match,
>> +       },
>> +       .probe = anx7428_probe,
>> +};
>