Received-SPF: pass (google.com: domain of linux-kernel+bounces-31110-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240119104550.140061-1-angelogioacchino.delregno@collabora.com> <20240119104550.140061-3-angelogioacchino.delregno@collabora.com>
In-Reply-To: <20240119104550.140061-3-angelogioacchino.delregno@collabora.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Fri, 19 Jan 2024 13:44:06 +0200
Message-ID: <CAA8EJprroFtKvCeZtP_FySr5d6v4dogYQKFQvzPJLpwWBuMrKw@mail.gmail.com>
Subject: Re: [PATCH 2/2] usb: typec: mux: Add ITE IT5205 Alternate Mode
 Passive MUX driver
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cc: gregkh@linuxfoundation.org, robh+dt@kernel.org, 
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, 
	heikki.krogerus@linux.intel.com, matthias.bgg@gmail.com, 
	neil.armstrong@linaro.org, andersson@kernel.org, nathan@kernel.org, 
	luca.weiss@fairphone.com, tianping.fang@mediatek.com, 
	linux-usb@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 
	linux-mediatek@lists.infradead.org, kernel@collabora.com
Content-Type: text/plain; charset="UTF-8"

On Fri, 19 Jan 2024 at 12:46, AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> 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@collabora.com>
> ---
>  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@collabora.com>
> + *
> + */
> +#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.

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

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

> +
> +               /* 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


-- 
With best wishes
Dmitry