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

[Hans de Goede <hdegoede@redhat.com>]

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,
>> +};
>