Re: [PATCH 2/2] usb: typec: mux: Add ITE IT5205 Alternate Mode Passive MUX driver

[AngeloGioacchino Del Regno]

Il 19/01/24 12:44, Dmitry Baryshkov ha scritto:
> On Fri, 19 Jan 2024 at 12:46, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> > The ITE IT5202 is a USB Type-C Alternate Mode Passive MUX, used for
> > muxing the SBU lines of a Type-C port with DisplayPort altmode and
> > also providing an orientation switch.
> >
> > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > ---
> >   drivers/usb/typec/mux/Kconfig  |  10 ++
> >   drivers/usb/typec/mux/Makefile |   1 +
> >   drivers/usb/typec/mux/it5205.c | 292 +++++++++++++++++++++++++++++++++
> >   3 files changed, 303 insertions(+)
> >   create mode 100644 drivers/usb/typec/mux/it5205.c
> >
> > diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
> > index d2cb5e733e57..399c7b0983df 100644
> > --- a/drivers/usb/typec/mux/Kconfig
> > +++ b/drivers/usb/typec/mux/Kconfig
> > @@ -36,6 +36,16 @@ config TYPEC_MUX_INTEL_PMC
> >            control the USB role switch and also the multiplexer/demultiplexer
> >            switches used with USB Type-C Alternate Modes.
> >
> > +config TYPEC_MUX_IT5205
> > +       tristate "ITE IT5205 Type-C USB Alt Mode Passive MUX driver"
> > +       depends on I2C
> > +       select REGMAP_I2C
> > +       help
> > +         Driver for the ITE IT5205 Type-C USB Alternate Mode Passive MUX
> > +         which provides support for muxing DisplayPort and sideband signals
> > +         on a common USB Type-C connector.
> > +         If compiled as a module, the module will be named it5205.
> > +
> >   config TYPEC_MUX_NB7VPQ904M
> >          tristate "On Semiconductor NB7VPQ904M Type-C redriver driver"
> >          depends on I2C
> > diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile
> > index 57dc9ac6f8dc..bb96f30267af 100644
> > --- a/drivers/usb/typec/mux/Makefile
> > +++ b/drivers/usb/typec/mux/Makefile
> > @@ -4,6 +4,7 @@ obj-$(CONFIG_TYPEC_MUX_FSA4480)         += fsa4480.o
> >   obj-$(CONFIG_TYPEC_MUX_GPIO_SBU)       += gpio-sbu-mux.o
> >   obj-$(CONFIG_TYPEC_MUX_PI3USB30532)    += pi3usb30532.o
> >   obj-$(CONFIG_TYPEC_MUX_INTEL_PMC)      += intel_pmc_mux.o
> > +obj-$(CONFIG_TYPEC_MUX_IT5205)         += it5205.o
> >   obj-$(CONFIG_TYPEC_MUX_NB7VPQ904M)     += nb7vpq904m.o
> >   obj-$(CONFIG_TYPEC_MUX_PTN36502)       += ptn36502.o
> >   obj-$(CONFIG_TYPEC_MUX_WCD939X_USBSS)  += wcd939x-usbss.o
> > diff --git a/drivers/usb/typec/mux/it5205.c b/drivers/usb/typec/mux/it5205.c
> > new file mode 100644
> > index 000000000000..99203b8a086d
> > --- /dev/null
> > +++ b/drivers/usb/typec/mux/it5205.c
> > @@ -0,0 +1,292 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ITE IT5205 Type-C USB alternate mode passive mux
> > + *
> > + * Copyright (c) 2020 MediaTek Inc.
> > + * Copyright (c) 2024 Collabora Ltd.
> > + *                    AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > + *
> > + */
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/delay.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/regmap.h>
> > +#include <linux/usb/typec.h>
> > +#include <linux/usb/typec_mux.h>
> > +#include <linux/usb/typec_dp.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/gpio.h>
> > +#include <linux/usb/tcpm.h>
>
> I'd say, it is a usual custom to sort this list.

Oh, whoops. Totally forgot to do that, sorry - thanks for pointing that out!
Done for v2.

> [skipped]
>
> > +
> > +static int it5205_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *state)
> > +{
> > +       struct it5205 *it = typec_mux_get_drvdata(mux);
> > +       u8 val;
> > +
> > +       switch (state->mode) {
> > +       case TYPEC_STATE_USB:
> > +               val = IT5205_USB;
> > +               break;
> > +       case TYPEC_DP_STATE_C:
> > +               fallthrough;
> > +       case TYPEC_DP_STATE_E:
> > +               val = IT5205_DP;
> > +               break;
> > +       case TYPEC_DP_STATE_D:
> > +               val = IT5205_DP_USB;
> > +               break;
> > +       case TYPEC_STATE_SAFE:
> > +               fallthrough;
> > +       default:
> > +               val = 0;
> > +               break;
> > +       }
>
> Please add a check for state->altmode. All states at TYPEC_STATE_MODAL
> and above (which includes TYPEC_DP_STATE_[CDE]) are only relevant with
> connection to the particular typec->altmode SVID.

Done for v2.

> > +
> > +       return regmap_update_bits(it->regmap, IT5205_REG_MUXCR,
> > +                                 IT5205_DP_USB_CTRL_MASK, val);
> > +}
> > +
> > +static irqreturn_t it5205_irq_handler(int irq, void *data)
> > +{
> > +       struct it5205 *it = data;
> > +       int ret;
> > +       u32 val;
> > +
> > +       ret = regmap_read(it->regmap, IT5205_REG_ISR, &val);
> > +       if (ret)
> > +               return IRQ_NONE;
> > +
> > +       if (val & IT5205_ISR_CSBU_OVP) {
> > +               dev_warn(&it->client->dev, "Overvoltage detected!\n");
>
> Will it cut the voltage automatically?

As far as I understand, yes.
Unfortunately, there's no datasheet.

> > +
> > +               /* Reset CSBU */
> > +               regmap_update_bits(it->regmap, IT5205_REG_CSBUSR,
> > +                                  IT5205_CSBUSR_SWITCH, 0);
> > +               regmap_update_bits(it->regmap, IT5205_REG_CSBUSR,
> > +                                  IT5205_CSBUSR_SWITCH, IT5205_CSBUSR_SWITCH);
> > +       }
> > +
> > +       return IRQ_HANDLED;
> > +}
>
> The rest LGTM