On 24-10-06 18:40:51, Dmitry Baryshkov wrote: > On Fri, Oct 04, 2024 at 04:57:38PM GMT, Abel Vesa wrote: > > The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C. > > It provides both altmode and orientation handling. > > > > Add a driver with support for the following modes: > > - DP 4lanes > > - DP 2lanes + USB3 > > - USB3 > > > > Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx> > > --- > > drivers/usb/typec/mux/Kconfig | 10 + > > drivers/usb/typec/mux/Makefile | 1 + > > drivers/usb/typec/mux/ps8830.c | 424 +++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 435 insertions(+) > > > > diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig > > index ce7db6ad30572a0a74890f5f11944fb3ff07f635..48613b67f1c5dacd14d54baf91c3066377cf97be 100644 > > --- a/drivers/usb/typec/mux/Kconfig > > +++ b/drivers/usb/typec/mux/Kconfig > > @@ -56,6 +56,16 @@ config TYPEC_MUX_NB7VPQ904M > > Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C > > redriver chip found on some devices with a Type-C port. > > > > +config TYPEC_MUX_PS8830 > > + tristate "Parade PS8830 Type-C retimer driver" > > + depends on I2C > > + depends on DRM || DRM=n > > + select DRM_AUX_BRIDGE if DRM_BRIDGE && OF > > + select REGMAP_I2C > > + help > > + Say Y or M if your system has a Parade PS8830 Type-C retimer chip > > + found on some devices with a Type-C port. > > + > > config TYPEC_MUX_PTN36502 > > tristate "NXP PTN36502 Type-C redriver driver" > > depends on I2C > > diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile > > index bb96f30267af05b33b9277dcf1cc0e1527d2dcdd..4b23b12cfe45a0ff8a37f38c7ba050f572d556e7 100644 > > --- a/drivers/usb/typec/mux/Makefile > > +++ b/drivers/usb/typec/mux/Makefile > > @@ -6,5 +6,6 @@ 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_PS8830) += ps8830.o > > obj-$(CONFIG_TYPEC_MUX_PTN36502) += ptn36502.o > > obj-$(CONFIG_TYPEC_MUX_WCD939X_USBSS) += wcd939x-usbss.o > > diff --git a/drivers/usb/typec/mux/ps8830.c b/drivers/usb/typec/mux/ps8830.c > > new file mode 100644 > > index 0000000000000000000000000000000000000000..58990f7860bf77ec12d00f0d3df3a40735bbf9d8 > > --- /dev/null > > +++ b/drivers/usb/typec/mux/ps8830.c > > @@ -0,0 +1,424 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Parade PS8830 usb retimer driver > > + * > > + * Copyright (C) 2024 Linaro Ltd. > > + */ > > + > > +#include <drm/bridge/aux-bridge.h> > > +#include <linux/clk.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/i2c.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/regmap.h> > > +#include <linux/regulator/consumer.h> > > +#include <linux/usb/typec_altmode.h> > > +#include <linux/usb/typec_dp.h> > > +#include <linux/usb/typec_mux.h> > > +#include <linux/usb/typec_retimer.h> > > + > > +struct ps8830_retimer { > > + struct i2c_client *client; > > + struct gpio_desc *reset_gpio; > > + struct regmap *regmap; > > + struct typec_switch_dev *sw; > > + struct typec_retimer *retimer; > > + struct clk *xo_clk; > > + struct regulator *vdd_supply; > > + struct regulator *vdd33_supply; > > + struct regulator *vdd33_cap_supply; > > + struct regulator *vddat_supply; > > + struct regulator *vddar_supply; > > + struct regulator *vddio_supply; > > + > > + struct typec_switch *typec_switch; > > + struct typec_mux *typec_mux; > > + > > + struct mutex lock; /* protect non-concurrent retimer & switch */ > > + > > + enum typec_orientation orientation; > > + unsigned long mode; > > + int cfg[3]; > > +}; > > + > > +static void ps8830_write(struct ps8830_retimer *retimer, int cfg0, int cfg1, int cfg2) > > +{ > > + if (cfg0 == retimer->cfg[0] && > > + cfg1 == retimer->cfg[1] && > > + cfg2 == retimer->cfg[2]) > > + return; > > + > > + retimer->cfg[0] = cfg0; > > + retimer->cfg[1] = cfg1; > > + retimer->cfg[2] = cfg2; > > + > > + regmap_write(retimer->regmap, 0x0, cfg0); > > + regmap_write(retimer->regmap, 0x1, cfg1); > > + regmap_write(retimer->regmap, 0x2, cfg2); > > +} > > This looks like a reimplementation of regcache. Is it really necessary? Sure, will use regcache. > > > + > > +static void ps8830_configure(struct ps8830_retimer *retimer, int cfg0, int cfg1, int cfg2) > > +{ > > + /* Write safe-mode config before switching to new config */ > > Why is this required? Data sheet says it needs to be configured into safe mode before switching modes. > > > + ps8830_write(retimer, 0x1, 0x0, 0x0); > > + > > + ps8830_write(retimer, cfg0, cfg1, cfg2); > > +} > > + > > +static int ps8380_set(struct ps8830_retimer *retimer) > > +{ > > + int cfg0 = 0x00; > > + int cfg1 = 0x00; > > + int cfg2 = 0x00; > > + > > + if (retimer->orientation == TYPEC_ORIENTATION_NONE || > > + retimer->mode == TYPEC_STATE_SAFE) { > > + ps8830_write(retimer, 0x1, 0x0, 0x0); > > + return 0; > > + } > > + > > + if (retimer->orientation == TYPEC_ORIENTATION_NORMAL) > > + cfg0 = 0x01; > > + else > > + cfg0 = 0x03; > > + > > + switch (retimer->mode) { > > + /* USB3 Only */ > > + case TYPEC_STATE_USB: > > + cfg0 |= 0x20; > > + break; > > + > > The TYPEC_DP clauses should only be used if state->alt->swid is set to > USB_TYPEC_DP_SID. Other altmodes share id space for their states. Make sense. Will check svid for DP. > > > + /* DP Only */ > > + case TYPEC_DP_STATE_C: > > + cfg1 = 0x85; > > + break; > > + > > + case TYPEC_DP_STATE_E: > > + cfg1 = 0x81; > > + break; > > + > > + /* DP + USB */ > > + case TYPEC_DP_STATE_D: > > + cfg0 |= 0x20; > > + cfg1 = 0x85; > > + break; > > CDE, please. Will do. > > > + > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + ps8830_configure(retimer, cfg0, cfg1, cfg2); > > + > > + return 0; > > +} > > + > > -- > With best wishes > Dmitry Thanks for reviewing. Abel